Refactor DynamicDataTable by BenjaminCharmes · Pull Request #1914 · datalab-org/datalab

BenjaminCharmes · 2026-07-13T12:52:05Z

Summary

DynamicDataTable is now a thin, generic wrapper over PrimeVue DataTable — no more hardcoded field names, v-else-if filter chains, or central getComponentProps registry
Column descriptors own their body component, filter component, match function, and options provider; adding a new item type only requires editing the consumer
Extracted reusable filter components (TextFilter, MultiSelectFilter, SingleSelectFilter, DateRangeFilter, CreatorsAndGroupsFilter) and shared match/options helpers into utils/filterMatchers.js
Dynamic FilterService matcher registration from descriptors, scoped per dataType to avoid global collisions
Cleanup: allUsers passed as an explicit prop, body events forwarded generically via column.body.events, filterOptionsCache receives store.state rather than the full store

codecov · 2026-07-13T12:56:15Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.27%. Comparing base (cd0b699) to head (7fd5426).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1914   +/-   ##
=======================================
  Coverage   80.27%   80.27%           
=======================================
  Files          83       83           
  Lines        7616     7616           
=======================================
  Hits         6114     6114           
  Misses       1502     1502

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cypress · 2026-07-13T13:15:45Z

datalab Run #5304

Run Properties: Passed #5304 • 3a2178649f ℹ️: Merge 7fd5426635916e3640d339c33fa7947da093c00d into cd0b699a322e311dc99febc8945d...

Project	`datalab`
Branch Review	`bc/refactor-dynamicdatatable`
Run status	`Passed #5304`
Run duration	`22m 24s`
Commit	`3a2178649f ℹ️: Merge 7fd5426635916e3640d339c33fa7947da093c00d into cd0b699a322e311dc99febc8945d...`
Committer	`Ben Charmes`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`548`
View all changes introduced in this branch ↗︎

BenjaminCharmes · 2026-07-27T14:27:52Z

@ml-evs I forgot to ping you for a review!

ml-evs · 2026-08-09T13:06:17Z

Also pining @DianaAliabieva and @davidwaroquiers for review here as they have been working on this component and will likely need to base their changes on this!

ml-evs

Thanks for this @BenjaminCharmes, looks great! Would you mind incorporating the changes from #1910 before we merge this? I had a look and not entirely sure how I would do it with the current conflicts. Thanks!

BenjaminCharmes · 2026-08-10T08:23:07Z

Thanks for this @BenjaminCharmes, looks great! Would you mind incorporating the changes from #1910 before we merge this? I had a look and not entirely sure how I would do it with the current conflicts. Thanks!

Yes, I'll have a look !

… API response Only bump item last_modified when a version is actually minted Add some testing and handling of millisecond edge cases when rehydrating date from db Refactor UTC handler into separate function Update relevant table tests to include new column Tidy up comments Remove misleading comment (cherry picked from commit e9c7f6b)

(cherry picked from commit b338055)

Reintroduce chemform (cherry picked from commit d236324)

…e preferences so new columns default to shown (cherry picked from commit 2b8f5aa)

(cherry picked from commit 009c154)

(cherry picked from commit fe9666c)

(cherry picked from commit b041e54)

…tatable # Conflicts: # pydatalab/src/pydatalab/routes/v0_1/items.py # webapp/src/components/CollectionInformation.vue # webapp/src/components/DynamicDataTable.vue

BenjaminCharmes · 2026-08-11T14:39:33Z

@ml-evs Everything should be good now, let me know if you need anything else!

…atalab-org#1914) Adapts the clear-filters column/button to the new columns-driven filter setup from datalab-org#1914: extracts default-filters construction into getDefaultFilters() so it can be reused both on created() and when clearing, adds hasActiveFilters, and updates the 4 component tests for the extra header column.

davidwaroquiers · 2026-08-12T05:56:53Z

Also pining @DianaAliabieva and @davidwaroquiers for review here as they have been working on this component and will likely need to base their changes on this!

Hi @ml-evs

Thank you for pinning us. I don't have time to make a thorough review right now (currently on holidays) but here is maybe my comment after a very quick look. This is a very nice work to make the Dynamic data table more generic and thanks @BenjaminCharmes for this. I was wondering if the associated table buttons could (or should) also be somehow described declaratively in the actual tables rather than being hardcoded in DynamicDataTable/DynamicDataTableButtons. Somehow the goal of the PR is to extract all references to actual tables from the DynamicDataTable component to the actual components (e.g. EquipmentTable, SampleTable, ...). What do you think ?

I can make a more thorough review when I'm back in the week of 24-28 August but if you'd prefer to get this merged sooner, please go ahead. Regarding PRs that are somehow related to this one (from me and/or @DianaAliabieva), we will refactor them based on these changes, I think the best is to get this through and rebase/refactor our own PRs.

DianaAliabieva

Hello! I tried to review this PR!
For me everything looks great, only one thing about lost filter values
Have a nice day!

DianaAliabieva · 2026-08-14T15:40:26Z

+            }),
+          },
+        },
+        {


I think this file got missed during the filter migration. On main, these three columns all had filter: true:

{ field: "date", header: "Date", label: "Date", filter: true },
{ field: "creatorsAndGroups", ..., filter: true },
{ field: "blocks", ..., filter: true },
In the refactored version they only kept getValue/body for rendering the filter key is just gone, and none of DateRangeFilter, CreatorsAndGroupsFilter, or matchBlocks/blocksOptions are imported at the top of the file (only TextFilter and MultiSelectFilter made it in, for the id/type/status columns).

So they are lost kinda, maybe we should return them? or it was a plan to do like that?

I think the filters should indeed stay the same unless this is intentional ?

davidwaroquiers

Hi @BenjaminCharmes and @ml-evs

Thanks for the nice work, this was indeed a good idea to refactor this big chunk of code that was becoming more and more complex. I have a couple of small comments in the code, not directly related to this PR but that I believe are worth fixing here anyway (should be easy to fix, could even be in a separate PR if easier or want to completely separate concerns).

I already mentioned this in a comment, but a nice extension of this work would be to apply the same declarative approach to the toolbar buttons and bulk actions. The columns are now declared by each concrete table, while the toolbar and bulk actions are still centralized and selected through dataType-specific conditions. Each table could instead declare the actions it supports (including their label, visibility, disabled state, and handler) leaving the generic components responsible only for rendering them and managing shared UI. This could also be considered as a second phase or a separate PR if it feels too broad for this refactoring but I think it would make things even more clean and modular.

Happy to discuss if needed.

davidwaroquiers · 2026-08-24T11:44:30Z

      }

      return visibleItems.every((currentItem) => {
        const currentId = currentItem.item_id || currentItem.collection_id;


Not directly related to this PR itself but I found out while reviewing. This seems like a bug. Identifying a specific row is based either on item_id or collection_id but for the other tables (users, groups, access tokens), e.g. those of the admin view, the key for identifying a row is different (immutable_id, group_id, _id). This leads to a bug when selecting users/groups/tokens.

To reproduce the bug, go for example in users and select one user:

You get "1 selected" for the dropdown menu of bulk actions.

Then do select all (top left checkbox to select all):

And only the previously selected user stays selected, others stay unselected (and the dropdown menu shows "1 selected").

The easiest fix would be to add a id_key string to specify which key is used to identify a row (user/item/group/...) uniquely.

Although this is not directly related to this refactoring I think it would be good to fix it here already.

davidwaroquiers · 2026-08-24T11:50:11Z

+            }),
+          },
+        },
+        {


I think the filters should indeed stay the same unless this is intentional ?

davidwaroquiers · 2026-08-24T11:55:44Z

      ref="datatable"
      v-model:filters="filters"
      v-model:selection="itemsSelected"
      v-model:select-all="allSelected"


Not directly related to this PR but probably easy to fix it here (or in another PR).

Currently if you select all rows (using the top left checkbox) then deselecting just one row keeps the selected all checkbox checked.

I think allSelected should be recomputed both upon row selection and deselection.

Refactor DynamicDataTable

5741733

BenjaminCharmes added 3 commits July 13, 2026 15:31

Fix cypress tests

3caf17f

Fix cypress tests

7375cbb

Fix cypress tests

03614e4

BenjaminCharmes marked this pull request as ready for review July 18, 2026 17:31

BenjaminCharmes requested a review from ml-evs July 27, 2026 14:26

ml-evs mentioned this pull request Aug 9, 2026

adding a button to clean all filters #1975

Open

ml-evs added this to the v0.8.x milestone Aug 9, 2026

ml-evs requested review from DianaAliabieva and davidwaroquiers August 9, 2026 13:05

ml-evs mentioned this pull request Aug 9, 2026

Fixed position of the "actions" menu when items are selected #1958

Open

ml-evs requested changes Aug 9, 2026

View reviewed changes

ml-evs and others added 8 commits August 10, 2026 12:27

Let /save-item endpoint indicate that nothing was changed

231b45c

(cherry picked from commit b338055)

Add last modified column to collections table

e9855de

Reintroduce chemform (cherry picked from commit d236324)

Store which columns are available at the moment users save their tabl…

446c67b

…e preferences so new columns default to shown (cherry picked from commit 2b8f5aa)

Add a small test for searching groups

532c4eb

(cherry picked from commit 009c154)

Tidy up error handling after item versions

830b7a1

(cherry picked from commit fe9666c)

Backfill item last_modified from MongoDB _id

8a0671c

(cherry picked from commit b041e54)

Merge remote-tracking branch 'origin/main' into bc/refactor-dynamicda…

7fd5426

…tatable # Conflicts: # pydatalab/src/pydatalab/routes/v0_1/items.py # webapp/src/components/CollectionInformation.vue # webapp/src/components/DynamicDataTable.vue

BenjaminCharmes requested a review from ml-evs August 11, 2026 14:39

DianaAliabieva reviewed Aug 14, 2026

View reviewed changes

davidwaroquiers requested changes Aug 24, 2026

View reviewed changes

This was referenced Aug 24, 2026

Row identification in DynamicDataTable #2028

Open

Select all checkbox is not synchronized #2029

Open

Uh oh!

Conversation

BenjaminCharmes commented Jul 13, 2026

Summary

Uh oh!

codecov Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cypress Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

datalab Run #5304

Uh oh!

BenjaminCharmes commented Jul 27, 2026

Uh oh!

ml-evs commented Aug 9, 2026

Uh oh!

ml-evs left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminCharmes commented Aug 10, 2026

Uh oh!

BenjaminCharmes commented Aug 11, 2026

Uh oh!

davidwaroquiers commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DianaAliabieva left a comment

Choose a reason for hiding this comment

Uh oh!

DianaAliabieva Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

davidwaroquiers Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

davidwaroquiers left a comment

Choose a reason for hiding this comment

Uh oh!

davidwaroquiers Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

davidwaroquiers Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

davidwaroquiers Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Jul 13, 2026 •

edited

Loading

cypress Bot commented Jul 13, 2026 •

edited

Loading

davidwaroquiers commented Aug 12, 2026 •

edited

Loading