Conversation
|
ui docs preview (Available for 14 days) |
There was a problem hiding this comment.
🟡 Changes recommended
Callback-only tables are not marked controlled, and persisted state can override controlled values during initial rendering.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds controlled sort/filter state and read-only controls to ui.table.
Changes:
- Adds change callbacks and controlled-state synchronization.
- Adds read-only filtering and sorting behavior.
- Adds Python, JavaScript, E2E, and documentation coverage.
File summaries
| File | Description |
|---|---|
tests/utils.ts |
Adds grid interaction helpers. |
tests/ui_table.spec.ts |
Tests controlled updates and persistence. |
tests/app.d/ui_table.py |
Adds E2E table fixtures. |
plugins/ui/test/deephaven/ui/test_ui_table.py |
Tests Python props and normalization. |
plugins/ui/src/js/src/elements/UITable/UITableUtils.ts |
Adds callback-state conversion utilities. |
plugins/ui/src/js/src/elements/UITable/UITableUtils.test.ts |
Tests state conversions. |
plugins/ui/src/js/src/elements/UITable/UITableModel.ts |
Enforces read-only behavior. |
plugins/ui/src/js/src/elements/UITable/UITableModel.test.ts |
Tests read-only model behavior. |
plugins/ui/src/js/src/elements/UITable/UITableClearFilterKeyHandler.ts |
Blocks filter-clearing shortcuts. |
plugins/ui/src/js/src/elements/UITable/UITable.tsx |
Implements controlled state and read-only integration. |
plugins/ui/src/js/src/elements/UITable/UITable.test.tsx |
Tests controlled IrisGrid props. |
plugins/ui/src/js/package.json |
Updates Deephaven dependencies. |
plugins/ui/src/deephaven/ui/components/table.py |
Exposes the new Python API. |
plugins/ui/docs/snapshots/74d3276deb61d890485c9528425404e6.json |
Adds read-only example snapshot. |
plugins/ui/docs/snapshots/5be53133b01169b03ce3116c0274173f.json |
Updates quick-filter snapshot. |
plugins/ui/docs/snapshots/43147ace5cee510f96e24cc6a3275935.json |
Updates sort snapshots. |
plugins/ui/docs/components/table.md |
Documents controlled and read-only state. |
package-lock.json |
Locks updated dependencies. |
Review details
- Files reviewed: 17/30 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const isSortsControlled = sorts !== undefined && onSortsChange != null; | ||
| const isQuickFiltersControlled = | ||
| quickFilters !== undefined && onQuickFiltersChange != null; |
| sorts: hydratedControlledSorts, | ||
| isSortsControlled, | ||
| onSortsChange: onSortsChange == null ? undefined : handleSortsChange, | ||
| quickFilters: hydratedControlledQuickFilters, | ||
| isQuickFiltersControlled, |
| The `sorts` prop accepts: | ||
|
|
||
| - A single column name string (ascending sort) | ||
| - A `ui.TableSort` object | ||
| - A list mixing column names and `ui.TableSort` objects |
| Accepts a column name, TableSort, or list containing column names and | ||
| TableSort instances. If `on_sorts_change` is not provided, these are the |
…ters prop actually differs
…ilters. Breaking change implemented
….table Adds read-only states for ui.table quick filters and sorts so users can tell that controlled `quick_filters` and `sorts` are owned by the server. Implemented entirely in the plugin by overriding `isFilterable`, `isColumnSortable`, and `getClearFilterRange` on UITableModel. IrisGrid already gates all filter/sort interactions on those model methods, so this disables the interactions and applies the disabled styling without changes to @deephaven/iris-grid. `getClearFilterRange` needs its own override because the base implementation calls `isFilterable` on the wrapped model. Also blocks the two clear-filter paths that bypass the model: the Ctrl+E shortcut (via a key handler ordered ahead of IrisGrid's ClearFilterKeyHandler) and the dashboard CLEAR_ALL_FILTERS event. Updates the default/controlled prop tests and docstrings to match the removal of the mutual-exclusivity validation, and bumps @deephaven/* packages.
…back presence Merge default_quick_filters/default_sorts into the single quick_filters and sorts props. The props are controlled when the matching on_quick_filters_change/on_sorts_change callback is provided; otherwise they set the initial state and user changes are retained and persisted.
…tions Add e2e fixtures and tests for the remaining combinations of controlled vs uncontrolled `sorts` and `quick_filters`: - both controlled, verifying user changes round-trip through the change callbacks and back down as the new controlled values - `sorts` controlled with `quick_filters` uncontrolled, verifying a server update re-applies the sort and leaves the user's filter alone - `quick_filters` controlled with `sorts` uncontrolled, verifying the inverse Also fix `setGridQuickFilter`, which clicked the filter cell's dropdown icon and so opened the value picker instead of editing the cell in firefox and webkit. The quick filter never changed there, and the existing `t_default` baselines had recorded that state. The helper now clicks near the column's left edge and replaces any existing expression; the affected baselines are regenerated.
Add e2e fixtures and tests for each read-only prop, asserting that the locked down interaction is a no-op while the other one still works. The successful interaction afterwards proves the widget was live, so the unchanged value is the prop taking effect rather than a dropped interaction. Also switch `t_controlled` to the small explicit table the other sort/filter tests use. It was sorting the stocks table by `Size`, where many rows tie and leave the sorted row order unstable, so its screenshots were flaky.
a9b773e to
c2a18c7
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Controlled state can restore stale persisted values, and context-menu actions bypass both read-only modes.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
plugins/ui/src/js/src/elements/UITable/UITable.tsx:452
- The controlled-mode check contradicts the documented contract that providing the change callback makes the value controlled. With a callback but no value, these expressions pass
false, so IrisGrid applies the user's proposal locally instead of treating its default empty state as controlled. Base controlledness on callback presence; omitted values can remain the empty defaults.
const isSortsControlled = sorts !== undefined && onSortsChange != null;
const isQuickFiltersControlled =
quickFilters !== undefined && onQuickFiltersChange != null;
plugins/ui/src/deephaven/ui/components/table.py:322
- The implementation now accepts sort mappings, including the mappings emitted by
on_sorts_change, but this public parameter documentation still lists only names andTableSortinstances. Document mappings so users know callback state can be passed back directly.
Accepts a column name, TableSort, or list containing column names and
TableSort instances. If `on_sorts_change` is not provided, these are the
plugins/ui/src/js/src/elements/UITable/UITable.tsx:519
- Persisted state is copied here wholesale, and the initial merge later spreads it over the controlled server props. After a controlled sort/filter has been persisted, reloading can therefore display that stale client value instead of the component's newly initialized controlled value, with no later prop-identity change to correct it. Remove controlled fields from the persisted initial state so the controlled props win from the first render.
if (persisted != null) {
initialHydratedStateRef.current = persisted;
- Files reviewed: 17/51 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Persisted client state can override controlled server values during initial rendering.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
plugins/ui/src/js/src/elements/UITable/UITableClearFilterKeyHandler.ts:17
- No unit or end-to-end test exercises this clear-filters shortcut path; the read-only end-to-end test only attempts to edit a filter cell. Add a regression test that starts with an applied filter, sends
SHORTCUTS.TABLE.CLEAR_FILTERS, and verifies the filter remains unchanged when read-only, while an unrelated key is not consumed.
plugins/ui/src/deephaven/ui/components/table.py:322 - This public parameter documentation omits the newly supported mapping form even though
TableSortLike, normalization, and tests now expose it. Document that a mapping requirescolumnand may containdirectionandis_abs, including their defaults, so callback-produced sort mappings can be used as input without relying on undocumented behavior.
plugins/ui/src/js/src/elements/UITable/UITable.tsx:452
- The controlled-mode check contradicts the stated API contract that setting the change callback enables controlled mode. With
ui.table(on_sorts_change=...)orui.table(on_quick_filters_change=...)—both callback-only forms are explicitly tested on the Python side—the handler is forwarded but IrisGrid receivesis*Controlled: false, so user proposals continue mutating local state. Either treat an omitted value as an empty controlled value, or explicitly define and document callback-only observer semantics.
// Providing a change callback makes the corresponding value prop controlled.
const isSortsControlled = sorts !== undefined && onSortsChange != null;
const isQuickFiltersControlled =
quickFilters !== undefined && onQuickFiltersChange != null;
plugins/ui/src/js/src/elements/UITable/UITable.tsx:670
- Controlled sorts and quick filters are still overridden by persisted user state on the first render.
initialHydratedStatecontains persistedsorts/quickFilters, and the initial merge at lines 773-777 spreads that state after these server props, so reopening a controlled table can show stale persisted values until the server changes a prop. Exclude controlled keys from the persisted initial state or explicitly merge controlled values last, and add a persisted-state regression test.
sorts: hydratedControlledSorts,
isSortsControlled,
onSortsChange: onSortsChange == null ? undefined : handleSortsChange,
quickFilters: hydratedControlledQuickFilters,
isQuickFiltersControlled,
- Files reviewed: 18/52 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
ui docs preview (Available for 14 days) |
on_quick_filters_change,on_sorts_change,is_quick_filters_read_onlyandis_sorts_read_onlyprops onui.tableon_quick_filters_changeprop is set, theui.tableis in "controlled" mode. When unset, it's in "uncontrolled" mode (like default value, same behaviour as before this change)is_quick_filters_read_onlyso user cannot change the filters after they are set