refactor: consolidate experiment list selection state #8860

ashtonG · 2024-02-20T19:53:35Z

Description

This refactors some items related to the experiment list selection ahead of fixing the bulk action apis:

selection state (selected/excluded experiment ids, selectAll state) is consolidated into a single object. Previously, selected experiment ids, excluded experiment ids, and the selectall state were stored separately. This is problematic -- because the selected experiment ids were the source of truth for which experiments were selected, the stored list would grow as the user paged through the results when in selectall mode. we now determine the final list of selected elements on the fly.
grid selection is now fully determined from the id selection. previously, the grid selection and id selection were maintained separately. this required a lot of code ensuring they stayed in sync. we now only derive the grid selection from the selected id list.
handleSelectionChange now no longer requires row ranges for add-all or remove-all change types. this allows for functions and effects that solely dealt with these change types to no longer rely on the experiments array.
project experiment list settings is no longer using useSettings. because the grid selection now only updates when the settings updates, the act of updating the selection felt slower than when using useSettings. we now directly interface with the settings store at the project level -- global settings still use useSettings.
new hook: useTypedParams. In order to keep the functionality of the compare parameter, we introduce a useTypedParams hook which allows typesafe parsing and setting of query params similar to the settings store api. this should allow us to finally fully migrate off of useSettings.

Test Plan

Parameter/setttings behavior:

navigate to a project experiment list with over 20 experiments in the list
turn off infinite scroll and reload the page
the experiment list should remain paginated
click onto the second page
the url should contain the query parameter page=1
reload the page
the experiment list should load the second page
click back onto the first page
the url should no longer contain the page query parameter
toggle the comparison view on
the url should contain the query parameter compare=true
reload the page
the experiment list should be in comparison mode
remove the query parameter compare=true and reload the page
the experiment list should remain in comparison mode
add the query paramter compare=false and reload the page
the experiment list should not be in comparison mode

Selection behavior:

navigate to a project experiment list with over 20 experiments in the list
select a single experiment in the list
reload the page
the experiment should remain selected
navigate to the next page
select a single experiment in the list
navigate to the first page
the selection status at the top of the table should show that two experiments are selected
select an experiment, hold shift, and select another experiment
all experiments between the first and second should be selected
in the selection column menu, choose select all
the selection status at the top of the table should show that all experiments are selected
deselect an experiment by clicking a checkbox
the selection status at the top of the table should respond likewise

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

this is base work for WEB-1937

netlify · 2024-02-20T19:53:52Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`5748bf4`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/65f35b0e04b2640008e861ed
😎 Deploy Preview	https://deploy-preview-8860--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-02-20T20:35:41Z

Codecov Report

Attention: Patch coverage is 46.84096% with 244 lines in your changes are missing coverage. Please review.

Project coverage is 42.84%. Comparing base (f73fd09) to head (5748bf4).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8860      +/-   ##
==========================================
- Coverage   47.48%   42.84%   -4.65%     
==========================================
  Files        1168      849     -319     
  Lines      176317   137164   -39153     
  Branches     2351     2385      +34     
==========================================
- Hits        83729    58771   -24958     
+ Misses      92430    78235   -14195     
  Partials      158      158

Flag	Coverage Δ
harness	`?`
web	`42.96% <46.84%> (+0.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...t/src/pages/F_ExpList/F_ExperimentList.settings.ts	`100.00% <100.00%> (+55.97%)`	⬆️
webui/react/src/utils/asValueObject.ts	`95.30% <100.00%> (ø)`
webui/react/src/components/ExperimentMoveModal.tsx	`66.36% <50.00%> (ø)`
...act/src/pages/F_ExpList/glide-table/GlideTable.tsx	`16.19% <12.50%> (-0.04%)`	⬇️
webui/react/src/utils/observable.ts	`90.21% <41.66%> (-7.35%)`	⬇️
webui/react/src/hooks/useTypedParams.ts	`94.07% <94.07%> (ø)`
...c/pages/F_ExpList/glide-table/ColumnPickerMenu.tsx	`20.73% <27.77%> (-0.18%)`	⬇️
...src/pages/F_ExpList/glide-table/TableActionBar.tsx	`24.56% <20.00%> (-0.05%)`	⬇️
...bui/react/src/pages/F_ExpList/F_ExperimentList.tsx	`11.59% <8.13%> (+1.19%)`	⬆️

... and 320 files with indirect coverage changes

EmilyBonar

I'm still working my way through the code, but I found some problems while running through the test plan.

Going to the second page and reloading brings me back to first page
Changing query param to compare=false didn’t work, it changed itself back to true and comparison view stayed open
Holding shift while selecting an experiment didn’t seem to do anything; either way any selections off the page got replaced
Similarly. if I select all experiments, deselect one (A), switch pages, and deselect another experiment (B), the first experiment I deselected (A) gets re-selected

ashtonG · 2024-02-23T16:10:11Z

@EmilyBonar just pushed a fix that should address selections across pages and the compare param not persisting properly. This should also mitigate one source of the page being reset. could you confirm when you have the time?

EmilyBonar · 2024-02-28T18:40:09Z

The compare view and deselection across pages (bullet point 4) bugs are fixed. The bug with the page number getting reset on refresh is still present.

I'm still not entirely sure what the shift behavior is supposed to be, but when I follow this section of the test plan:

the selection status at the top of the table should show that one experiment is selected

hold shift and select an experiment

the experiment should be added to the selection

The selection status shows that 2 experiments are selected, and the next experiment seems to be added to the selection whether or not shift is held.

ashtonG · 2024-03-01T20:42:45Z

updated the test plan -- i was thinking of the native glide table selection behavior, i've replaced it with how shift selections work currently. I've also pushed a potential fix for the page being reset by changing how we detect if the scroll took (previously we assumed that the table would be set up after a certain amount of renders with the glide table mounted, now we wait for the row in question to be in bounds) can you verify?

EmilyBonar · 2024-03-04T21:09:03Z

The page reset bug is fixed! The only step of the test plan I'm still having trouble with is

the selection status at the top of the table should show that one experiment is selected

When I reach that step I see "2 of [total] experiments selected"

ashtonG · 2024-03-04T21:52:30Z

@EmilyBonar updated the test plans again -- looking at the main branch that behavior is acceptable.

EmilyBonar

Test plan passes and everything looks good! Just some code style comments

webui/react/src/pages/F_ExpList/F_ExperimentList.tsx

EmilyBonar · 2024-03-06T15:13:10Z

webui/react/src/pages/F_ExpList/F_ExperimentList.tsx

+      cleanup = projectSettingsObs.subscribe((ps) => {
+        if (ps.isLoaded) {
+          cleanup();
+          initFormset(ps);
+        }
+      });


nit: this part is hard to follow, a comment would be helpful.

webui/react/src/pages/F_ExpList/F_ExperimentList.tsx

EmilyBonar · 2024-03-06T15:26:33Z

webui/react/src/pages/F_ExpList/F_ExperimentList.tsx

@@ -293,6 +316,36 @@ const F_ExperimentList: React.FC<Props> = ({ project }) => {
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [isLoadingSettings]);

+  useEffect(() => {


nit: this whole useEffect could use more commenting

will probably refactor this pattern to be clearer -- it's the same setup as the formset effect, but it's very verbose and we're doing a weird thing with the cleanup reference in order to get around subscribe only being called when the observable changes.

Yeah, it's confusing in the other spot too, especially the cleanup

@EmilyBonar refactored the effects -- is this easier to understand?

Yeah that's a lot better, thanks!

This refactors some items related to the experiment list selection ahead of fixing the bulk action apis: 1) selection state (selected/excluded experiment ids, selectAll state) is consolidated into a single object. Previously, selected experiment ids, excluded experiment ids, and the selectall state were stored separately. This is problematic -- because the selected experiment ids were the source of truth for which experiments were selected, the stored list would grow as the user paged through the results when in selectall mode. we now determine the final list of selected elements on the fly. 2) grid selection is now fully determined from the id selection. previously, the grid selection and id selection were maintained separately. this required a lot of code ensuring they stayed in sync. we now only derive the grid selection from the selected id list. 3) handleSelectionChange now no longer requires row ranges in an `add-all` or `remove-all` change type. this allows for functions and effects that solely dealt with these change types to no longer rely on the experiments array. 4) project experiment list settings is no longer using useSettings. because the grid selection only updates when the settings updates, the act of updating the selection felt worse when using useSettings. we now directly interface with the settings store at the project level -- global settings still use useSettings. 5) new hook: useTypedParams. In order to keep the functionality of the `compare` parameter, we introduce a useTypedParams hook which allows typesafe parsing and setting of query params similar to the settings store api. this should allow us to finally fully migrate off of useSettings.

ashtonG requested a review from a team as a code owner February 20, 2024 19:53

ashtonG requested a review from gt2345 February 20, 2024 19:53

cla-bot bot added the cla-signed label Feb 20, 2024

ashtonG requested review from EmilyBonar and removed request for gt2345 February 20, 2024 19:53

ashtonG force-pushed the chore/WEB-1937/select-consolidation branch from 5054cc5 to 6334dee Compare February 20, 2024 20:32

EmilyBonar suggested changes Feb 21, 2024

View reviewed changes

ashtonG force-pushed the chore/WEB-1937/select-consolidation branch from e42bf9c to 68a5c6d Compare March 1, 2024 16:57

EmilyBonar approved these changes Mar 6, 2024

View reviewed changes

ashtonG mentioned this pull request Mar 13, 2024

refactor: change GlideTable into a reusable component (ET-25) #8956

Merged

4 tasks

ashtonG added 6 commits March 14, 2024 16:16

fix selection count display when paginated

a379b7f

fix multipage selections and compare hijacking, mitigate page erasure

024e6c3

improve pagination position restoration behavior

bcbef69

refactor settings handoff pattern

d94d4d3

refactor selection map generation

5748bf4

ashtonG force-pushed the chore/WEB-1937/select-consolidation branch from 647390e to 5748bf4 Compare March 14, 2024 20:16

ashtonG merged commit 8bf280d into main Mar 18, 2024
69 of 81 checks passed

ashtonG deleted the chore/WEB-1937/select-consolidation branch March 18, 2024 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: consolidate experiment list selection state #8860

refactor: consolidate experiment list selection state #8860

ashtonG commented Feb 20, 2024 •

edited

Loading

netlify bot commented Feb 20, 2024 •

edited

Loading

codecov bot commented Feb 20, 2024 •

edited

Loading

EmilyBonar left a comment

ashtonG commented Feb 23, 2024

EmilyBonar commented Feb 28, 2024

ashtonG commented Mar 1, 2024

EmilyBonar commented Mar 4, 2024

ashtonG commented Mar 4, 2024

EmilyBonar left a comment

EmilyBonar Mar 6, 2024

EmilyBonar Mar 6, 2024

ashtonG Mar 6, 2024

EmilyBonar Mar 6, 2024

ashtonG Mar 7, 2024

EmilyBonar Mar 11, 2024

refactor: consolidate experiment list selection state #8860

refactor: consolidate experiment list selection state #8860

Conversation

ashtonG commented Feb 20, 2024 • edited Loading

Description

Test Plan

Checklist

Ticket

netlify bot commented Feb 20, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

codecov bot commented Feb 20, 2024 • edited Loading

Codecov Report

EmilyBonar left a comment

Choose a reason for hiding this comment

ashtonG commented Feb 23, 2024

EmilyBonar commented Feb 28, 2024

ashtonG commented Mar 1, 2024

EmilyBonar commented Mar 4, 2024

ashtonG commented Mar 4, 2024

EmilyBonar left a comment

Choose a reason for hiding this comment

EmilyBonar Mar 6, 2024

Choose a reason for hiding this comment

EmilyBonar Mar 6, 2024

Choose a reason for hiding this comment

ashtonG Mar 6, 2024

Choose a reason for hiding this comment

EmilyBonar Mar 6, 2024

Choose a reason for hiding this comment

ashtonG Mar 7, 2024

Choose a reason for hiding this comment

EmilyBonar Mar 11, 2024

Choose a reason for hiding this comment

ashtonG commented Feb 20, 2024 •

edited

Loading

netlify bot commented Feb 20, 2024 •

edited

Loading

codecov bot commented Feb 20, 2024 •

edited

Loading