Repository navigation
feat: add compact pdisk previews to node tables - #4348
StekPerepolnen wants to merge 11 commits into
Conversation
ab6fe90 to
a140b42
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical clipping and moderate keyboard-focus issues remain in compact preview expansion.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an opt-in Compact PDisk preview experiment for node tables, reducing DOM elements while retaining expandable disk controls, settings persistence, sizing, and test coverage.
Changes:
- Adds compact SVG previews for PDisks and VDisks.
- Adds persisted experiment settings and dynamic column sizing.
- Updates shared disk colors, accessibility text, virtualization integration, and tests.
File summaries
| File | Summary |
|---|---|
tests/suites/paginatedTable/paginatedTable.test.ts |
Stabilizes pagination assertions. |
tests/suites/nodes/pdisksPreviewMocks.ts |
Adds preview API mocks. |
tests/suites/nodes/pdisksPreview.test.ts |
Covers preview rendering, refresh, scrolling, and settings. |
src/store/reducers/settings/constants.ts |
Registers the preview setting and default. |
src/containers/UserSettings/settings.tsx |
Adds the experiment to Settings. |
src/containers/UserSettings/i18n/en.json |
Adds experiment translations. |
src/containers/UserSettings/__test__/settings.test.ts |
Tests experiment availability and defaults. |
src/containers/Storage/utils/useStorageColumnsSettings.ts |
Calculates preview column sizing. |
src/containers/Storage/PDisks/PDisksPreview.tsx |
Implements compact previews and expansion. Review findings: expanded controls can be clipped by the overflow-hidden cell, and focus is not restored after collapsing. |
src/containers/Storage/PDisks/PDisksPreview.test.tsx |
Tests grouping, inversion, refresh, and highlighting. |
src/containers/Storage/PDisks/PDisksPreview.scss |
Styles previews and popup containers. |
src/containers/Storage/PDisks/i18n/index.ts |
Registers preview translations. |
src/containers/Storage/PDisks/i18n/en.json |
Adds preview accessibility text. |
src/containers/Storage/PaginatedStorageNodesTable/columns/types.ts |
Extends column settings. |
src/containers/Storage/PaginatedStorageNodesTable/columns/columns.tsx |
Selects preview or legacy rendering. |
src/containers/Nodes/NodesTable.tsx |
Sets consistent PDisk row height. |
src/containers/Nodes/getNodes.ts |
Propagates sizing metadata. |
src/components/DiskStateProgressBar/DiskStateProgressBar.tsx |
Exposes shared color modifiers. |
src/components/DiskStateProgressBar/DiskStateProgressBar.scss |
Uses shared disk color styles. |
src/components/DiskStateProgressBar/diskStateColors.scss |
Centralizes disk state colors. |
Review details
Suppressed comments (1)
src/containers/Storage/PDisks/PDisksPreview.tsx:283
- When the expanded control is closed, the currently focused PDisk/VDisk link is unmounted by this state update, but focus is never restored to the compact preview button. Keyboard users therefore lose their focus position after pressing Enter to collapse the group; retain a ref to the preview button and restore focus after closing.
setDetailsOpened(false);
- Files reviewed: 20/48 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3d3370a to
ff3e597
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical compilation/test failures and moderate focus and column-sizing issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/containers/Storage/PaginatedStorageNodesTable/columns/columns.tsx:46
- This selects the preview renderer but only changes the column's base width.
ResizeablePaginatedTablelater overlays any persisted PDisks size from the existing column-width setting onto every column, so a previously stored width can keep this column at the old expanded (or too-small) size and the experiment toggle will not actually update it. Please bypass or version the persisted PDisks width when switching preview mode.
const Disks = columnsSettings?.pDisksPreviewEnabled ? PDisksPreview : PDisks;
src/containers/Storage/utils/useStorageColumnsSettings.ts:72
- The preview width is initialized from the first paginated response only. Because
MaximumSlotsPerDiskandMaximumDisksPerNodeare optional,prepareStorageNodesResponsecan fall back to maxima from just that chunk; if a later chunk contains more disks or slots, this latchedpreviewColumnWidthstays too small and those previews overflow the reserved column. Accumulate maxima across fetched chunks or otherwise provide a global fallback before fixing the width.
setPreviewColumnWidth(
getPDisksPreviewColumnWidth({
maxSlotsPerDisk: maxSlots,
maxDisksPerNode: maxDisks,
}),
);
- Files reviewed: 19/47 changed files
- Comments generated: 3
- Review effort level: Lite
66beb55 to
ec7370b
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Three moderate findings remain regarding popup ownership, accessible expansion semantics, and column-width recalculation.
Review details
Suppressed comments (3)
src/containers/Storage/PDisks/PDisksPreview.tsx:294
highlightedDiskis shared by all preview items, but each item'scloseDetailsclears it unconditionally. BecausedetailsOpenedis per item, two previews can be expanded simultaneously; collapsing one item therefore hides another item's open popup. Track the highlighted owner (or clear only when this item owns the highlight) so one group's collapse cannot close a different group's popup.
setDetailsOpened(false);
setHighlightedDisk(undefined);
}, [setHighlightedDisk]);
src/containers/Storage/PDisks/PDisksPreview.tsx:317
- This button is the control that expands the disk group, but it is removed and replaced by a layout-only
Flexwithoutaria-expanded/aria-controls. Screen readers therefore receive neither the expanded state nor a semantic collapse target, even though keyboard users can activate the revealed links. Keep a stable toggle (or expose an equivalent focusable button) and associate it with the expanded content.
<Button
view="flat"
className={b('control')}
src/containers/Storage/utils/useStorageColumnsSettings.ts:66
- When the Nodes page initially renders,
PDisksis not in the default column set, so the first request can contain no disk data. If the backend omits the optionalMaximumSlotsPerDisk/MaximumDisksPerNodemetadata,prepareStorageNodesResponsefalls back to 1 for both values; the!pDiskWidthguard then permanently caches those widths, so selectingPDisksor enabling the preview later leaves the compact column too narrow and expanded controls overflow. Recompute or merge the dimensions when disk data/metadata arrives (while retaining the maximum across virtualized chunks), and reset them when the requested table identity changes.
setPDiskWidth(calculatedPDiskWidth);
setPDiskContainerWidth(calculatedPDiskContainerWidth);
- Files reviewed: 16/44 changed files
- Comments generated: 0 new
- Review effort level: Lite
482cc22 to
f198395
Compare
f3b6c1b to
31e5a4d
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Clear stale highlight state when a disk disappears during refresh.
Review details
Suppressed comments (1)
src/containers/Storage/PDisks/PDisksPreview.tsx:243
- When a VDisk or PDisk disappears during a refresh, its
HoverPopupunmounts without callingonHidePopup, sohighlightedDiskcan retain the removed ID. This check only recognizes IDs in the currentvDisks; after removal it preserves the stale value, and if the same disk later reappears its popup receivesshowPopup={true}and opens without hover. Track the highlight owner or clear the shared highlight when the current ID is no longer present in the row data.
const clearOwnHighlight = React.useCallback(() => {
setHighlightedDisk((current) =>
current === id || vDisks.some((disk) => disk.StringifiedId === current)
? undefined
: current,
- Files reviewed: 16/44 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate issues affect refresh state, keyboard focus for partial data, and persisted column widths.
Review details
Suppressed comments (4)
src/containers/Storage/PDisks/PDisksPreview.tsx:376
- When a VDisk or PDisk owns
highlightedDisk, removing it from refreshedpDisks/vDisksonly unmounts the child; this state is never pruned. If the same ID appears in a later refresh, the new child receivesshowPopup={true}and its popup opens without hover or focus. Clear or validate the highlighted ID whenever the incoming disk lists change.
const [highlightedDisk, setHighlightedDisk] = React.useState<string | undefined>();
src/containers/Storage/PDisks/PDisksPreview.tsx:247
- Keyboard focus transfer assumes
PDiskalways renders an anchor.PDiskrenders a<span>when either identifier is missing, sodiskLinkRef.currentis null; after the preview button is unmounted,target?.focus()does nothing and keyboard users lose their focus target. Keep a focusable expanded target for this valid partial-data case.
const target = detailsOpened ? diskLinkRef.current : previewRef.current;
target?.focus({preventScroll: true});
src/containers/Storage/PaginatedStorageNodesTable/columns/columns.tsx:51
- Because these tables use
ResizeablePaginatedTable, thewidthsupplied here is replaced by any saved PDisks width before it reaches the data table (updateColumnsWidthprefers the persisted value). A user who has a stored PDisks width from the old layout will therefore not get the compact width after enabling the experiment (and a compact stored width can make expanded controls overlap the next column after disabling it). Make the persisted width mode-aware or otherwise reset/ignore it for this experiment, and cover a seeded persisted-width case.
width: columnsSettings?.pDiskContainerWidth,
src/containers/Storage/utils/useStorageColumnsSettings.ts:38
PAGNATEDis misspelled; please rename this constant toPAGINATED_TABLE_CELL_HORIZONTAL_PADDING(and its use below) to match the repository'sPaginatedterminology.
const PAGNATED_TABLE_CELL_HORIZONTAL_PADDING = 10;
- Files reviewed: 20/48 changed files
- Comments generated: 0 new
- Review effort level: Lite
Node tables with many PDisks and VDisks create many DOM elements for individual disk controls. Add Compact PDisk previews, an experiment disabled by default, to display collapsed disk groups as compact SVG summaries.
Clicking a preview expands the existing disk controls; clicking an expanded disk collapses the group. Keyboard activation transfers focus to the expanded PDisk and back to the preview. Mouse clicks do not transfer focus. Preview colors use the application's base status palette, allocation respects the inversion setting, and refreshed data updates both summaries and open details.
Column widths adapt to the selected mode and retain disk and slot maxima across fetched pages. The experiment is available in Settings and persists through the existing settings system.
PDiskgains an optional link ref for keyboard focus transfer.Screen-reader limitation: expanded disk controls retain link semantics and do not announce their collapse action or expanded state.
Validation:
npm run typecheck,npm run lint(existing warnings only), andnpm test -- --runInBand: 219 suites / 1935 tests passed on this branch.npm run build:embedded,npm run package, andgit diff --checkpassed.