deephaven / web-client-ui

Deephaven Web Client UI
Apache License 2.0
28 stars 30 forks source link

feat: Partitioned Table UI Enhancements #2110

Closed AkshatJawne closed 1 month ago

AkshatJawne commented 1 month ago

Resolves #2079 ( Resolves #2066 , Resolves #2103 , Resolves #2104 , Resolves #2105 , Resolves #2106 , Resolves #2107 , Resolves #2108 , Resolves #2109 , Resolves #2049 ), Resolves #2120, Resolves #1904

Changes Implemented:

Sample Visuals: Default Partition Table View

Screenshot 2024-06-26 at 2 57 09 PM

Default Partition-aware source table view:

Screenshot 2024-06-26 at 2 56 59 PM
AkshatJawne commented 1 month ago

Looking into unit test failures

AkshatJawne commented 1 month ago

BTW, ready for review

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 23.72881% with 90 lines in your changes missing coverage. Please review.

Project coverage is 46.68%. Comparing base (e3aa392) to head (e588409). Report is 16 commits behind head on main.

Files Patch % Lines
packages/iris-grid/src/IrisGrid.tsx 5.12% 37 Missing :warning:
...ckages/iris-grid/src/IrisGridPartitionSelector.tsx 52.27% 20 Missing and 1 partial :warning:
...sehandlers/IrisGridPartitionedTableMouseHandler.ts 17.64% 14 Missing :warning:
...ges/iris-grid/src/IrisGridPartitionedTableModel.ts 0.00% 10 Missing :warning:
packages/iris-grid/src/IrisGridTableModel.ts 0.00% 4 Missing :warning:
...d/src/mousehandlers/IrisGridContextMenuHandler.tsx 0.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2110 +/- ## ========================================== + Coverage 46.60% 46.68% +0.08% ========================================== Files 679 691 +12 Lines 38408 38587 +179 Branches 9687 9622 -65 ========================================== + Hits 17901 18016 +115 - Misses 20455 20560 +105 + Partials 52 11 -41 ``` | [Flag](https://app.codecov.io/gh/deephaven/web-client-ui/pull/2110/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deephaven) | Coverage Δ | | |---|---|---| | [unit](https://app.codecov.io/gh/deephaven/web-client-ui/pull/2110/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deephaven) | `46.68% <23.72%> (+0.08%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deephaven#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

AkshatJawne commented 1 month ago

Looking into failures here

AkshatJawne commented 1 month ago

Weird that the failures are only occurring on chromium, but I believe the reason of the failure is that we expect to have the pickers accessible in 1.5s, but now, with the new logic, the pickers are disabled while the partitionConfig.mode === 'empty', which in some cases (like the vid below), is not the case.

The reason we do this is so that when we have an empty partition table, we wanted to have the pickers disabled. But even for a table with rows, the table will have 0 rows of length (and thus, will be considered empty) momentarily.

We could either change the benchmark to be 20000ms instead of 15000ms, or we could look at adding a specific "temporarily empty" config mode, which would somehow differ then the "empty" mode (which would now act as more of a forever empty mode).

The latter will be difficult to implement though, since the event listener listens for when the table is updated and then changes the mode accordingly, so I am not sure how before the event, it will differentiate between a table that will be forever empty, and one that is just empty until the data loads in.

@mofojed @dsmmcken any thoughts?

screen-capture (4).webm

mofojed commented 1 month ago

We've already got the loading state for when it's still loading. The empty state is for when the table is empty, there's no need for a forever empty state; an empty table could eventually tick in data (it could be in one second, one minute, one hour, we don't really know). That being said - the failures in the e2e tests are not with the partitioned tables? Where are you seeing that the disabled pickers is affecting the e2e tests? Look at the report attached: https://github.com/deephaven/web-client-ui/actions/runs/9845185336/artifacts/1682155129 Looking at the report it's looking for a context menu that does not appear for some reason.

AkshatJawne commented 1 month ago

No actually, that does not fix the issue we are facing with the spinner, ignore.

AkshatJawne commented 1 month ago

Been doing some more investigations, the firefox and cjromium failures here are not consistent. On one run they fail, but then if I re-run those jobs, they pass. The inconsistent behaviour makes me think there may be a race condition or some weird code circumstance in the code responsible for rendering the context menu.

The only change I made to the Context Menu was adding the View Constituent Table option, but I am unsure why that would result in the whole context menu not loading, need to dig more.

AkshatJawne commented 1 month ago

Yeah I have been trying to debug the e2e failures, but it is slightly tricky, will continue to debug and then we can debug together as well in our 1:1, will make the requested change with -1 rn.