Table Picker #40509

kamilmielnik · 2024-03-22T13:21:12Z

Closes #40298

Description

What's not included:

Questions and Models tabs (this is in progress and will be integrated as a separate task when ready)
Icon colors as per design (this requires a bit of refactoring, I extracted it as a separate task)
The new component is not used in join table picker yet
Tests have not been updated or added (separate issue, will be done once everything else is ready)
- It's therefore expected that tests in CI will fail. Type check and lint checks should pass though.

How to verify

Start a new GUI question/model or edit an existing one
Choose data source

Demo

2024-04-03.19-52-47.mp4

Additional context

Related Slack thread.

Target branch includes #41003, see Slack discussion.

mazameli

I'm reliably running into an error when reopening the picker after having selected a table. It seems to be specifically happening when picking a table from my local MySQL/MariaDB database. Recording here: https://www.loom.com/share/52b68a1e6ee04647b51d03c36dd2b7b9

kamilmielnik · 2024-04-04T08:30:37Z

I'm reliably running into an error when reopening the picker after having selected a table. It seems to be specifically happening when picking a table from my local MySQL/MariaDB database. Recording here: https://www.loom.com/share/52b68a1e6ee04647b51d03c36dd2b7b9

@mazameli Nice catch! I was also able to reproduce it with SQLite. Fixed!

Our type system assumed all tables belonged to a schema, but in some dbs schema can be null.
Fixed with 4f5c4e9 + 0c96d1c.

Edit: this is actually due to discrepancy in BE, see Slack discussion. I will revert these 2 aforementioned commits once BE is updated.

Edit 2: I reverted the 2 aforementioned commits and used #41003 by merging it into the target integration branch (data-picker). This is now fixed BE-side.

This reverts commit 0c96d1c.

This reverts commit 4f5c4e9.

mazameli

This is looking great, and I'm approving since it's targeting a dev branch rather than master, since of course we can't have this going into master without the Models or Questions tabs, which I understand will happen in other PRs.

uladzimirdev

Left minor comments on code, but I need to check locally how it works

uladzimirdev · 2024-04-09T05:27:38Z

frontend/src/metabase/common/components/DataPicker/utils.ts

+  schemaItem: NotebookDataPickerFolderItem | null,
+  tableItem: NotebookDataPickerValueItem | null,
+) => {
+  return [dbItem?.id, schemaItem?.id, tableItem?.id].join("-");


IDK but what if all of them are null?

Suggested change

return [dbItem?.id, schemaItem?.id, tableItem?.id].join("-");

return [dbItem?.id, schemaItem?.id, tableItem?.id].filter(Boolean").join("-");

If we add filter(Boolean) then we lose information about which entity id is missing.

uladzimirdev · 2024-04-09T05:30:41Z

frontend/src/metabase/common/components/DataPicker/hooks/useAutoSelectOnlyItem.ts

+  const onlyItem = hasOnly1Item ? items[0] : undefined;
+
+  useEffect(
+    function autoSelectOnlyItem() {


is it a named function for easier debugging?

I really wanted to name this effect for informative purposes.
I could leave a comment, but IMO name is better.

uladzimirdev · 2024-04-09T05:31:36Z

frontend/src/metabase/common/components/DataPicker/hooks/useAutoSelectOnlyItem.ts

+ * @returns true when there's only 1 item.
+ */
+export const useAutoSelectOnlyItem = <Item>(
+  items: Item[] | undefined,


nit: I expect such hooks to accept exactly Item[] without handing such undefined cases. Can we pass default value if items is undefined?

It's easier to handle undefined here.

If we do this:

Suggested change

items: Item[] | undefined,

items: Item[] = [],

then the effect will run on every render.

If we do this:

Suggested change

items: Item[] | undefined,

items: Item[] = EMPTY_ARRAY,

then it's impossible to type EMPTY_ARRAY due to this hook having a generic interface.

If we do this:

Suggested change

items: Item[] | undefined,

items: Item[] = getProperlyTypedEmptyArray(),

then it's an overkill.

If we provide the default value from outside, then we pollute all usages of this hook with that logic.
And it does not fit really well there, because ItemList component relies on items being undefined and not an empty array.

uladzimirdev · 2024-04-09T05:35:54Z

frontend/src/metabase/common/components/DataPicker/components/DataPickerModal.tsx

+  );
+
+  const tabs: [
+    EntityTab<NotebookDataPickerValueItem["model"]>,


can you please clarify this type? It's not clear why do we expect a tuple with a type and destructured type array tbh

it's because we always expect at least one tab

I'm not a big fan of using types to enforce this.
Actually, I'm not a big fan of enforcing this at all 🙃

✔️ Simplified: 470c461

uladzimirdev · 2024-04-09T05:35:58Z

frontend/src/metabase/common/components/DataPicker/components/DataPickerModal.tsx

+      tabs={tabs}
+      title={t`Pick your starting data`}
+      onClose={onClose}
+      onConfirm={_.noop} // onConfirm is unused when options.hasConfirmButtons is falsy


so can't we pass undefined instead and handle it on EntrityPickerModal level?

We could. I think it's probably better to get rid of options.hasConfirmButtons and make it depend on the presence of onConfirm prop though. It might be a controversial change so I'll submit it as a separate PR: #41172

...c/metabase/common/components/EntityPicker/components/EntityPickerModal/EntityPickerModal.tsx

* Fix nil schema in /api/table/:id/query_metadata * Fix nil schema in /api/table/:id/query_metadata * Fix nil schema in /api/table/:id/query_metadata * Fix nil schema in /api/table/:id/query_metadata * Fix nil schema in /api/table/:id/query_metadata * Revert "Fix nil schema in /api/table/:id/query_metadata" This reverts commit 38ac092. * Revert "Fix nil schema in /api/table/:id/query_metadata" This reverts commit 2b8fe78. * Revert "Fix nil schema in /api/table/:id/query_metadata" This reverts commit 219506f. * Revert "Fix nil schema in /api/table/:id/query_metadata" This reverts commit 9430478. * Revert "Fix nil schema in /api/table/:id/query_metadata" This reverts commit 8c6cdd9. * Table Picker (#40509) * Generic types * Generic types * Generic types + make generateKey a prop * Generic types * Extract AutoScrollBox.styled.tsx * Move AutoScrollBox to a separate directory * Generic types * Generic types * Generic types * Unhookify searchFilter * Generic types * Generic types * Generic types * Generic types * Generic types * Generic types * Generic types * Generic types * Finish dealing with a cast * Destructure import * Inline type * Generic types * Remove cast * Remove commented code * Remove redundant fallback * Move CollectionPicker out of EntityPicker * Avoid as unknown * Remove TODOs * Rename TisFolder to IsFolder * Fix any * Fix anys * Remove NestedItemPicker's storybook * Remove a cast * Remove a cast * Remove a cast * Revert "Remove a cast" This reverts commit b762d0f. * Use extends SearchModelType instead of extends string everywhere * Revert "Use extends SearchModelType instead of extends string everywhere" This reverts commit 708190d. * Fix SearchResult["available_models"] type * Handle options.allowCreateNew * Add missing description attribute to Database type * Move allowCreateNew to CollectionPickerOptions * Add missing description attribute in Database mock * Add types * Add TableList component * Add SchemaList component * Fix naming * Add DatabaseList component * Add NotebookDataItemPickerResolver * Fix typing * Add TablePicker and NotebookDataPickerModal * Update types * Fix types * Add folder type * Fix initial state * Update title * Make query model-dependent * Render tables * Fix schema icon * Adjust key generation * Rename utilts to utils * Use ItemList error prop * Use value prop * Rename NotebookDataPickerModal to DataPickerModal * Automatically open datapicker if there is no value * Highlight current item * Rename value to initial value * Fix item highlighting * Rework TablePicker state * Leave TODOs for names * Fix selecting items * Fix highlighting selected item * Hide confirmation button * Add fetchMetadata to tableApi * Fix Table['schema'] type * Fetch table metadata upon selection * Remove options from DataPickerModal * Fix collectionId typing in Question * Add collectionId prop * Remove title prop * Add todo * Fix confirmation button * Remove old DataSourceSelector * Bring back useSchemaListQuery * Fix crash * Fix picker not opening * Hide dbs list and schemas list if there's only 1 * Remove unused ref * Remove unused DataPickerListResolver * Remove unused types * Change conditional rendering to allow for loading state * Revert collection-related changes * Use RTK * Avoid using stale data * Fix highlighted db * Remove todo * Remove unused type * Remove useSchemaListQuery * Rename Value to TablePickerValue * Move isValueEqual to utils and rename it to isTablePickerValueEqual * Improve TODO * Remove unused options prop * Introduce tablePickerValueFromTable * Avoid having 2 table metadata requests * Update comment * Update comment * Remove unused function * Allow null in tablePickerValueFromTable * Extract helpers * Rename schemaId to schemaName * Remove description * Improve naming * Simplify types * Add explanatory comments * Fall back to empty state when there are no items * Account for null schemas * Account for null schema in tests * Revert "Account for null schema in tests" This reverts commit 0c96d1c. * Revert "Account for null schemas" This reverts commit 4f5c4e9. * Improve typing around schema name * Refactor auto-select logic * Refactor * Avoid comment * Make useAutoSelectOnlyItem generic * Do not enforce presence of at least 1 tab with TypeScript * Fix types * Remove obsolete comment * Post-merge fix * Integrate QuestionPicker in DataPicker (#42268) * Add question and model pickers to Data Picker * Add model attribute to data picker value * Introduce DataPickerValue * Start with the right tab when opening data picker modal * Do not use stale sourceCard value * Fix switching tabs when a table is selected * Use undefined instead of null for consistency * Do not show models/questions tabs when there are no models/questions * Fix selecting models/questions in search results * Remove dead code * Remove dead code * Ignore selecting collections * Refactor useHasModels and useHasQuestions into useAvailableData * Use models param to speed up the query * Do not require `onConfirm` prop in `EntityPickerModal` (#42317) * Make onConfirm prop optional * Add a test for onConfirm prop assertion * Table does not appear selected in single-schema dbs (#42336) * Convert useAutoSelectOnlyItem arguments to param object * Disable auto-selection when there already is a selection * Use `DataPicker` in `JoinTablePicker` (#42330) * Refactor DataStep: - rename handleTableSelect to handleTableChange - extract value and memoize it * Remove redundant fragment * Add new DataPicker props * Use DataPickerModal in JoinTablePicker * Remove initialTab prop since the default tab is always the models tab anyway * Limit table selection to particular database * Limit search results to a given database * Support databaseId in useAvailableData * Filter collection items by databaseId * Use databaseId prop only for CollectionItemList component * Fix types * Fix filtering * Use useLatest for onChange * Fix typing * Introduce shouldShowItem and use it instead of databaseId prop * Improve PickerInfo type * Use MLv2 to generate DataPickerModal value in JoinTablePicker * Fix title * Add isModel to TableDisplayInfo * Add a hack to populate query metadata with info that table is a model * Remove NewQuestionView & QuestionDataSelector (#42562) * Temporarily disable recents tab in the data picker * Update Data Picker tests (#42565) * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Fix metadata loading * Update test * Update test * Update tests * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update tests * Update test * Update test * Update test and make it pass * Update test * Update tests * Update test * Use entityPickerModal() instead of modal() * Use entityPickerModal() instead of modal() * Update test * Update test * Update test * Revert change that broke tests * Update test * Update test * Update test * Update test * Update test * Update test * Remove test * Update test * Update test * Update test * Update test * Update test * Update test * Update tests * Update test * Update test * Rename createDatabaseIdItemFilter to createShouldShowItem * Update test * Update and skip a test * Remove test * Update tests * Update tests * Update tests * Remove test * Update tests * Update test * Update test * Update test * Extract utils * Use helpers * Extract helpers * Update test * Update test * Update test * Update test * Update test * Filter out collections that do not have card or dataset * Improve filtering * Always show root and personal collections, see #42687 * Remove failing assertion * Update tests * Use entityPickerModalTab * Accept strings in pickEntity * Use entityPickerModalLevel * Use entityPickerModalTab * Use entityPickerModalTab * Update tests * Update test * Update tests * Update test * Fix assertion * Update test * Update tests * Update test * Update tests * Update tests * Update test * Update test * Update tests - ensure database list gets loaded like in a real app * Update test * Update test * Integrate Recents tab in Data Picker (#42821) * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Fix metadata loading * Update test * Update test * Update tests * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update test * Update tests * Update test * Update test * Update test and make it pass * Update test * Update tests * Update test * Use entityPickerModal() instead of modal() * Use entityPickerModal() instead of modal() * Update test * Update test * Update test * Revert change that broke tests * Update test * Update test * Update test * Update test * Update test * Update test * Remove test * Update test * Update test * Update test * Update test * Update test * Update test * Update tests * Update test * Update test * Rename createDatabaseIdItemFilter to createShouldShowItem * Update test * Update and skip a test * Remove test * Update tests * Update tests * Update tests * Remove test * Update tests * Update test * Update test * Update test * Extract utils * Use helpers * Extract helpers * Update test * Update test * Update test * Update test * Update test * Filter out collections that do not have card or dataset * Improve filtering * Always show root and personal collections, see #42687 * Remove failing assertion * Update tests * Use entityPickerModalTab * Accept strings in pickEntity * Use entityPickerModalLevel * Use entityPickerModalTab * Use entityPickerModalTab * Update tests * Update test * Update tests * Update test * Fix assertion * Update test * Update tests * Update test * Update tests * Update tests * Update test * Update test * Update tests - ensure database list gets loaded like in a real app * Update test * Enable recents tab and update tests * Update test * Update tests * Add Recents-related assertions * Update test * Clean up mocks --------- Co-authored-by: Alexander Polyankin <alexander.polyankin@metabase.com>

kamilmielnik added 30 commits March 20, 2024 19:58

Generic types

43948f8

Generic types

d44c342

Generic types + make generateKey a prop

ce7eb49

Generic types

cd615a9

Extract AutoScrollBox.styled.tsx

8bff3d6

Move AutoScrollBox to a separate directory

ba07db2

Generic types

afbc0e7

Generic types

2178006

Generic types

be9e2ef

Unhookify searchFilter

fbe5e0c

Generic types

159e0a2

Generic types

143e144

Generic types

e02fcc6

Generic types

1ef76e6

Generic types

6a696f0

Generic types

ce25ad2

Generic types

0668286

Generic types

123146c

Finish dealing with a cast

1f0dbb5

Destructure import

b5ecb49

Inline type

e9b42cf

Generic types

74dc0b8

Remove cast

24134c0

Remove commented code

2ee97ae

Remove redundant fallback

787d2ea

Merge branch 'master' into refactor/collection-picker

58ca975

Merge branch 'master' into refactor/collection-picker

16616e0

Move CollectionPicker out of EntityPicker

7d1dfa9

Avoid as unknown

a301cac

Remove TODOs

6f9f00d

kamilmielnik requested a review from iethree April 3, 2024 13:23

mazameli requested changes Apr 3, 2024

View reviewed changes

kamilmielnik added 4 commits April 4, 2024 14:42

Merge branch 'data-picker' into 40298-data-source-picker

c31ef33

Fall back to empty state when there are no items

8a2fb07

Account for null schemas

4f5c4e9

Account for null schema in tests

0c96d1c

kamilmielnik requested a review from mazameli April 4, 2024 08:30

kamilmielnik added 8 commits April 4, 2024 17:46

Revert "Account for null schema in tests"

ea7fc72

This reverts commit 0c96d1c.

Revert "Account for null schemas"

2c17e57

This reverts commit 4f5c4e9.

Merge branch 'data-picker' into 40298-data-source-picker

0973280

Improve typing around schema name

a36096e

Refactor auto-select logic

d2e009d

Refactor

f3fa044

Avoid comment

73fd669

Make useAutoSelectOnlyItem generic

57a40c3

kamilmielnik mentioned this pull request Apr 4, 2024

Make table schema non-nullable #41023

Closed

ranquild approved these changes Apr 8, 2024

View reviewed changes

mazameli approved these changes Apr 8, 2024

View reviewed changes

uladzimirdev reviewed Apr 9, 2024

View reviewed changes

kamilmielnik added 3 commits April 9, 2024 16:36

Merge branch 'data-picker' into 40298-data-source-picker

b1a0c7d

Do not enforce presence of at least 1 tab with TypeScript

470c461

Fix types

62cf0bd

uladzimirdev approved these changes Apr 9, 2024

View reviewed changes

iethree reviewed Apr 9, 2024

View reviewed changes

...c/metabase/common/components/EntityPicker/components/EntityPickerModal/EntityPickerModal.tsx Outdated Show resolved Hide resolved

Remove obsolete comment

26ad7d9

kamilmielnik merged commit 5f7f09c into data-picker Apr 9, 2024
56 of 57 checks passed

kamilmielnik deleted the 40298-data-source-picker branch April 9, 2024 11:11

kamilmielnik mentioned this pull request Apr 9, 2024

Table Picker #40298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table Picker #40509

Table Picker #40509

kamilmielnik commented Mar 22, 2024 •

edited

mazameli left a comment

kamilmielnik commented Apr 4, 2024 •

edited

mazameli left a comment

uladzimirdev left a comment

uladzimirdev Apr 9, 2024

kamilmielnik Apr 9, 2024

uladzimirdev Apr 9, 2024

kamilmielnik Apr 9, 2024

uladzimirdev Apr 9, 2024

kamilmielnik Apr 9, 2024

uladzimirdev Apr 9, 2024

iethree Apr 9, 2024

kamilmielnik Apr 9, 2024

uladzimirdev Apr 9, 2024

kamilmielnik Apr 9, 2024

	return [dbItem?.id, schemaItem?.id, tableItem?.id].join("-");
	return [dbItem?.id, schemaItem?.id, tableItem?.id].filter(Boolean").join("-");

	items: Item[] \| undefined,
	items: Item[] = getProperlyTypedEmptyArray(),

Table Picker #40509

Table Picker #40509

Conversation

kamilmielnik commented Mar 22, 2024 • edited

Description

How to verify

Demo

Additional context

mazameli left a comment

Choose a reason for hiding this comment

kamilmielnik commented Apr 4, 2024 • edited

mazameli left a comment

Choose a reason for hiding this comment

uladzimirdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamilmielnik commented Mar 22, 2024 •

edited

kamilmielnik commented Apr 4, 2024 •

edited