🪟 🎉 Add column selection UI to new stream table #21058

josephkmh · 2023-01-05T11:35:17Z

What

Closes #20584

Adds column selection UI to the new stream table design.

field.selection.in.new.table.mov

How

Adds a column to the new StreamFieldsTable for a "Sync" checkbox for each top-level field
Updates the stream config when the checkbox is toggled
Adds a checkbox in the header to select/deselect all fields
Does some conditional checks based on syncMode and destinationSyncMode - primary keys and cursors in these modes should not be deselected
Adds unit tests for various scenarios of selecting/deselecting fields

josephkmh · 2023-01-12T11:01:32Z

airbyte-webapp/src/components/connection/CatalogTree/CatalogSection.tsx

+  const onToggleFieldSelected = useCallback(
+    (fieldPath: string[], isSelected: boolean) => {
+      if (!config) {
+        return;
+      }
+      const updatedConfig = updateFieldSelected({ config, fields, fieldPath, isSelected, numberOfFieldsInStream });
+      updateStreamWithConfig(updatedConfig);
+    },
+    [config, fields, numberOfFieldsInStream, updateStreamWithConfig]
+  );


This has been extracted to streamConfigHelpers to make unit testing easier (see streamConfigHelpers.test.ts)

josephkmh · 2023-01-12T22:38:23Z

...webapp/src/components/connection/CatalogTree/streamConfigHelpers/streamConfigHelpers.test.ts

+const FIELD_ONE: SyncSchemaField = {
+  path: ["field_one"],
+  cleanedName: "field_one",
+  key: "field_one",
+  type: "string",
+};
+const FIELD_TWO: SyncSchemaField = {
+  path: ["field_two"],
+  cleanedName: "field_two",
+  key: "field_two",
+  type: "string",
+};
+const FIELD_THREE: SyncSchemaField = {
+  cleanedName: "field_three",
+  type: "todo",
+  key: "field_three",
+  path: ["field_three"],
+};


I redefined FIELD_ONE, FIELD_TWO and FIELD_THREE in this file, which is one of the reasons for the large diff.

josephkmh · 2023-01-12T22:44:38Z

airbyte-webapp/src/components/connection/CatalogTree/CatalogSection.tsx

+  const fields = useMemo(() => {
+    const traversedFields = traverseSchemaToField(stream?.jsonSchema, stream?.name);
+    return traversedFields.sort(naturalComparatorBy((field) => field.cleanedName));
+  }, [stream?.jsonSchema, stream?.name]);
+


This has just been moved up in the file, not changed!

edmundito

Code looks good but there were a few issues I noticed during testing.

Can you follow up with Nico about the "Sync" column? I'm not sure if that is outdated design so I'm not sure what exactly is the intent there (previous designs had a switch instead of checkbox)

The field count needs to appear in the main table. Is this intended to be done separately?

I tried just unselecting my primary key and saving and got an error. But the error is a little vague. It works for one stream, but what about multiple streams? One option is to invalidate the selected primary key immediately so that the user has to update the key right away (Probably can be addressed separately.)

After you delete a connection and go back to it (we call this readonly mode), you can still check and uncheck the checkboxes:

When I check and uncheck the boxes, the save changes button is enabled instead of going back to being disabled:

Let me know which ones you want to address in this PR and which will be addressed separately.

...nents/connection/CatalogTree/next/StreamDetailsPanel/StreamFieldsTable/StreamFieldsTable.tsx

edmundito · 2023-01-16T14:28:53Z

...ection/CatalogTree/next/StreamDetailsPanel/StreamFieldsTable/SyncFieldCell/SyncFieldCell.tsx

+        <CheckBox
+          checkboxSize="sm"
+          checked={isFieldSelected}
+          onChange={() => handleFieldToggle(field.path, !isFieldSelected)}
+        />
+      )}
+      {isDisabled && (
+        <Tooltip control={<CheckBox checkboxSize="sm" disabled checked={isFieldSelected} readOnly />}>


Nit: I'm always in favor of rendering the same component instead of splitting the logic into two render calls. Meaning that an alternative would be to create the CheckBox in a variable with all the possible fields, then if disabled, wrap it in a tooltip.

I think I understand, and I think I generally agree when there's a lot of JSX, but I'm not sure if it would be any clearer in this instance. This component's render function basically does one thing only: checks the boolean isDisabled and returns one or the other bit of JSX. If I split that into another variable, wouldn't that same logic be repeated in the variable? What would the benefit be?

edmundito · 2023-01-16T14:30:29Z

...webapp/src/components/connection/CatalogTree/streamConfigHelpers/streamConfigHelpers.test.ts

        selectedFields: [],
        fieldSelectionEnabled: false,
      });
    });
  });
 });
+
+describe(`${updateFieldSelected.name}`, () => {


I've been using this convention:

Function: #${myFunction.name}
Component: <${MyComponent.name} />

Not sure I quite follow here - are you suggesting that I put a # before the function name in the test title? Something like this?

describe(`#${updateFieldSelected.name}`, () => {

I can't find any other usage like this in the codebase, so I think I'm misunderstanding here.

josephkmh · 2023-01-16T14:58:37Z

Thanks for the review! I'll address the comments today/tomorrow.

The field count needs to appear in the main table. Is this intended to be done separately?

Yes, this is missing and I think I will actually leave it out for the moment to keep this PR as small as possible. It should be an easy follow-up PR: #21772

josephkmh · 2023-01-16T16:39:12Z

Can you follow up with Nico about the "Sync" column? I'm not sure if that is outdated design so I'm not sure what exactly is the intent there (previous designs had a switch instead of checkbox)

Confirmed with @Upmitt that checkboxes are correct, latest figma designs here

josephkmh · 2023-01-16T16:43:34Z

I tried just unselecting my primary key and saving and got an error. But the error is a little vague. It works for one stream, but what about multiple streams? One option is to invalidate the selected primary key immediately so that the user has to update the key right away (Probably can be addressed separately.)

This is definitely unexpected, and deselection of the primary key or cursor field should be prevented on the client side already:

Could you let me know steps to reproduce this?

josephkmh · 2023-01-16T16:59:37Z

When I check and uncheck the boxes, the save changes button is enabled instead of going back to being disabled

Good catch, this one is because the backend does not return selectedFields in stream.config if fieldSelectionEnabled: false. I'm checking with @mfsiega-airbyte if we should reliably return this field from the backend. If not, we can fix this by removing selectedFields: [] when fieldSelectionEnabled: false

Edit: following up, looks like we will need to do this validation client-side. So we'll have to conditionally delete selectedFields from the config if fieldSelectionEnabled is set to false: d311cb9

josephkmh · 2023-01-16T18:23:03Z

After you delete a connection and go back to it (we call this readonly mode), you can still check and uncheck the checkboxes:

Thanks for catching this! I did not have readonly mode on my radar. Here's a fix for that: 706163d

…rsor

josephkmh · 2023-01-19T12:00:15Z

I tried just unselecting my primary key and saving and got an error. But the error is a little vague. It works for one stream, but what about multiple streams? One option is to invalidate the selected primary key immediately so that the user has to update the key right away (Probably can be addressed separately.)

This has been fixed here: 427a3b6

dizel852

Overall - LGTM 👍
But have a question: do we support stream selection functionality during connection creation?

Since I got an error when tried to create a connection with unselected field:

josephkmh · 2023-01-19T22:01:07Z

@dizel852 thank you for pointing that out, it looks like a backend error that we're looking into 👍

josephkmh · 2023-01-21T20:32:17Z

@dizel852 this has been fixed in #21647 ! Thanks again for reporting that.

edmundito

I did some testing and noticed something 👀

...ection/CatalogTree/next/StreamDetailsPanel/StreamFieldsTable/SyncFieldCell/SyncFieldCell.tsx

edoang · 2023-04-17T12:09:34Z

@josephkmh i'm trying to test this feature on the latest version (0.44.0) but i don't find the way to select columns. I'm using postgres src and dest. am i missing something?

josephkmh · 2023-05-09T08:28:58Z

@edoang this is indeed only available in Airbyte cloud currently. It will be coming to OSS, but we don't have a timeline for that just yet!

josephkmh added 2 commits January 5, 2023 12:32

add column selection UI to new stream table

cc625d2

disable experiment by default

b79bb18

octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Jan 5, 2023

josephkmh and others added 8 commits January 5, 2023 15:59

add option to toggle all selected fields

a6c6b87

Merge branch 'master' into joey/column-selection-new-stream-table

94f331f

fix header styling

81a0330

Merge branch 'master' into joey/column-selection-new-stream-table

972be8f

fix missing dependencies

84db0c0

fix checkbox warnings

625bb89

add ability to select/deselect all, ignoring pk and cursor

9c6e31a

Merge branch 'master' into joey/column-selection-new-stream-table

e9d8a22

octavia-squidington-iv removed the area/platform issues related to the platform label Jan 11, 2023

josephkmh added 3 commits January 11, 2023 20:12

typo in merge

e6914ec

refactor method for single field toggle, add tests

59d0267

Merge branch 'master' into joey/column-selection-new-stream-table

b123ed3

josephkmh commented Jan 12, 2023

View reviewed changes

teallarson mentioned this pull request Jan 12, 2023

Add tooltips to overflow text on streams table #21253

Merged

josephkmh added 2 commits January 12, 2023 22:38

test that cursor & pk are selected when toggling all fields

c0278a4

support source defined pk & cursor

4b82714

josephkmh commented Jan 12, 2023

View reviewed changes

josephkmh changed the title ~~[DRAFT] 🪟 🎉 Add column selection UI to new stream table~~ 🪟 🎉 Add column selection UI to new stream table Jan 12, 2023

josephkmh marked this pull request as ready for review January 12, 2023 22:40

Merge branch 'master' into joey/column-selection-new-stream-table

25ebac7

josephkmh commented Jan 12, 2023

View reviewed changes

josephkmh requested review from edmundito and dizel852 January 12, 2023 22:46

edmundito reviewed Jan 16, 2023

View reviewed changes

josephkmh added 2 commits January 16, 2023 17:31

disable field selection checkboxes in readonly mode

706163d

omit selectedFields if field selection disabled

d311cb9

josephkmh and others added 2 commits January 19, 2023 11:20

Merge branch 'master' into joey/column-selection-new-stream-table

05e56af

disable deselection of parent field when child is part of pk or is cu…

427a3b6

…rsor

dizel852 reviewed Jan 19, 2023

View reviewed changes

Merge branch 'master' into joey/column-selection-new-stream-table

cf5e863

josephkmh requested review from edmundito and dizel852 January 24, 2023 08:37

josephkmh mentioned this pull request Jan 24, 2023

Add selected field count to new streams table #21772

Closed

Merge branch 'master' into joey/column-selection-new-stream-table

16d0b57

edmundito reviewed Jan 24, 2023

View reviewed changes

...ection/CatalogTree/next/StreamDetailsPanel/StreamFieldsTable/SyncFieldCell/SyncFieldCell.tsx Show resolved Hide resolved

josephkmh added 2 commits January 24, 2023 19:57

fix disabling checkboxes in readonly mode

22f08b4

missing dependency

b5769c8

edmundito approved these changes Jan 25, 2023

View reviewed changes

josephkmh merged commit 00e7336 into master Jan 25, 2023

josephkmh deleted the joey/column-selection-new-stream-table branch January 25, 2023 14:32

github-actions bot mentioned this pull request Jan 25, 2023

Bump helm chart version reference to 0.43.24 #21887

Closed

This was referenced Jan 27, 2023

Bump Airbyte version from 0.40.29 to 0.40.30 #21954

Closed

Bump Airbyte version from 0.40.29 to 0.40.30 #21991

Closed

Bump Airbyte version from 0.40.29 to 0.40.30 #22045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🎉 Add column selection UI to new stream table #21058

🪟 🎉 Add column selection UI to new stream table #21058

josephkmh commented Jan 5, 2023 •

edited

Loading

josephkmh Jan 12, 2023

josephkmh Jan 12, 2023

josephkmh Jan 12, 2023

edmundito left a comment

edmundito Jan 16, 2023

josephkmh Jan 16, 2023

edmundito Jan 16, 2023

josephkmh Jan 16, 2023

josephkmh commented Jan 16, 2023 •

edited

Loading

josephkmh commented Jan 16, 2023 •

edited

Loading

josephkmh commented Jan 16, 2023

josephkmh commented Jan 16, 2023 •

edited

Loading

josephkmh commented Jan 16, 2023

josephkmh commented Jan 19, 2023

dizel852 left a comment •

edited

Loading

josephkmh commented Jan 19, 2023

josephkmh commented Jan 21, 2023 •

edited

Loading

edmundito left a comment

edoang commented Apr 17, 2023

josephkmh commented May 9, 2023 •

edited

Loading

🪟 🎉 Add column selection UI to new stream table #21058

🪟 🎉 Add column selection UI to new stream table #21058

Conversation

josephkmh commented Jan 5, 2023 • edited Loading

What

How

josephkmh Jan 12, 2023

Choose a reason for hiding this comment

josephkmh Jan 12, 2023

Choose a reason for hiding this comment

josephkmh Jan 12, 2023

Choose a reason for hiding this comment

edmundito left a comment

Choose a reason for hiding this comment

edmundito Jan 16, 2023

Choose a reason for hiding this comment

josephkmh Jan 16, 2023

Choose a reason for hiding this comment

edmundito Jan 16, 2023

Choose a reason for hiding this comment

josephkmh Jan 16, 2023

Choose a reason for hiding this comment

josephkmh commented Jan 16, 2023 • edited Loading

josephkmh commented Jan 16, 2023 • edited Loading

josephkmh commented Jan 16, 2023

josephkmh commented Jan 16, 2023 • edited Loading

josephkmh commented Jan 16, 2023

josephkmh commented Jan 19, 2023

dizel852 left a comment • edited Loading

Choose a reason for hiding this comment

josephkmh commented Jan 19, 2023

josephkmh commented Jan 21, 2023 • edited Loading

edmundito left a comment

Choose a reason for hiding this comment

edoang commented Apr 17, 2023

josephkmh commented May 9, 2023 • edited Loading

josephkmh commented Jan 5, 2023 •

edited

Loading

josephkmh commented Jan 16, 2023 •

edited

Loading

josephkmh commented Jan 16, 2023 •

edited

Loading

josephkmh commented Jan 16, 2023 •

edited

Loading

dizel852 left a comment •

edited

Loading

josephkmh commented Jan 21, 2023 •

edited

Loading

josephkmh commented May 9, 2023 •

edited

Loading