Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🪟 🎉 Disable deselection of cursor field/primary key in the UI #20844

Merged
merged 17 commits into from
Jan 10, 2023

Conversation

josephkmh
Copy link
Contributor

@josephkmh josephkmh commented Dec 22, 2022

What

Closes #20586

This PR automatically adds special handling of cursor fields and primary keys with regards to field selection in the airbyte UI.

How

  • Disables the deselection of the primary key(s) or cursor field, instead displaying a tooltip with an explanation
  • In case a user selects a cursor or primary key that was previously deselected, the UI automatically selects this field
  • Extracts logic from useCallback() functions into pure functions, adds unit tests to cover these

Recommended reading order

  1. FieldRow.tsx - where the pk & cursor field deselection is disabled
  2. CatalogSection.tsx - where useCallback() logic has been extracted to pure functions
  3. streamConfigHelpers - the pure functions that return a new stream config

@josephkmh josephkmh changed the title 🪟 🎉 Disable deselection of cursor field/primary key 🪟 🎉 Disable deselection of cursor field/primary key in the UI Dec 22, 2022
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 22, 2022
@josephkmh josephkmh marked this pull request as ready for review January 3, 2023 12:29
@josephkmh josephkmh requested a review from a team as a code owner January 3, 2023 12:29
@josephkmh josephkmh requested a review from ambirdsall January 3, 2023 12:29
@josephkmh josephkmh requested a review from teallarson January 5, 2023 18:34
Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't found why in the code yet, but it doesn't look like this takes into account source-defined cursors and primary keys.

Screenshot 2023-01-05 at 3 43 42 PM

(^ this example is from a Mailchimp source. The disabled rows are part of my composite primary key that I configured, but my cursor (timestamp) can still be toggled.)

@josephkmh josephkmh requested a review from teallarson January 5, 2023 22:55
@octavia-squidington-iv octavia-squidington-iv removed the area/platform issues related to the platform label Jan 9, 2023
@josephkmh josephkmh requested a review from teallarson January 9, 2023 16:09
Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code lgtm and tested locally.

I think it would be prudent to add a test case specifically for source-defined cursors and source-defined primary keys not being able to be deselected, but I don't necessarily think it's blocking.

Comment on lines +14 to +15
const key = JSON.stringify(selectedFieldInfo.fieldPath);
set.add(key);
Copy link
Contributor Author

@josephkmh josephkmh Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This converts the fieldPath (e.g. ['path', 'to', 'field']) into a string (e.g. "[\"path\",\"to\",\"field\"]") so that we can add it as a unique value to the set. It gets converted back into an array before returning.

This conversion between String[] > String > String[] might seem expensive, but it allows us use a Set to avoid duplicates when merging, which avoids a lot of nested comparisons of arrays.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a reasonable tradeoff. It sounds like it would still be more efficient for a large sync catalog or a sync catalog with a lot of changes than what this was doing before, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, at least as efficient, and in the best case more efficient 👍

@josephkmh
Copy link
Contributor Author

Thanks @tealjulia! I made one more minor adjustment here FYI d193819

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable deselection of cursor field/primary key
3 participants