gsadmin/pulse
1
0
Fork 0
mirror of https://github.com/rcourtman/Pulse.git synced 2026-10-24 12:42:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

Stop Workloads row focus staging a stale shell scroll restore (#2736)

Browse Source 
Stacked on #2723 (its one commit is the parent here). Whichever lands
first, the other still merges.

## Problem

Selecting a Workloads row called `capturePendingAppShellRestoreTop()`,
which staged the `.app-scroll-shell` position for `App.tsx` to restore
on the next route change. That made sense when row focus wrote
`?resource=` into the URL. 86ac84f68e made selection local, but the
capture stayed. So the staged position now waits for whatever route
change comes next. The same-path route-state scheduler only re-stages
from a scrolled shell, so at scrollTop 0 the old value survives.

Reproduced on a mock estate (embedded build of main, /proxmox/overview,
1440x900 and 390x844), with every scripted `scrollTop` write on the
shell recorded:

| Steps | main | this branch |
| --- | --- | --- |
| Open a guest drawer at scrollTop 3000, scroll to 0, set Type to VMs |
URL becomes `?type=vm`, then the shell jumps 0 → 3000 (74 ms later on
desktop, 126 ms on phone) | stays at 0, no scripted write |
| Same scroll path, no drawer | stays at 0 | stays at 0 |
| Drawer, then filter changed at scrollTop 500 | stays at 500 (the
scheduler re-stages) | stays at 500 |
| Drawer, then Proxmox Storage sub-tab or Docker tab | 0 (destinations
are shorter than the stale offset) | 0 |

## Change

- `useWorkloadSelectionState.ts`: drop the capture. Row focus already
holds its place through `preserveScrollableAncestorVerticalOffset`;
opening a drawer still leaves the shell at 3000 with the URL unchanged.
The now-identical `setSelectedGuestIdState` folds into
`setSelectedGuestId`.
- `appShellScrollRestoration.ts`: delete the capture helper, which has
no callers left, and its branch-coverage test. `routeStateNavigation.ts`
is now the only producer, and it stages right before it navigates.
- A regression test in `useWorkloadSelectionState.test.ts` toggles a row
at 3000, returns to 0 and runs a same-path filter write. It fails on the
parent with `expected 3000 to be null`.
- Contract prose in performance-and-scalability and unified-resources no
longer describes the old capture handoff.

## Verification

- Before/after browser probe at both widths on pulse-dev. A `?resource=`
deep link still opens and reveals the drawer, closing it keeps the
position, and there were no page errors. Receipt:
`frontend-modern/browser-verification/b3c60348af17e8cc.json`.
- prettier, eslint and `tsc --noEmit` are clean.
- Targeted vitest: 556 files passed. `docsLinks.test.ts` timed out at
host load ~76 and passes on its own (104/104 across the five related
files).
- Full pre-commit and pre-push batteries passed.
This commit is contained in:
rcourtman
2026-10-08 03:41:25 +01:00
committed by GitHub
parent 31fcc4f4d6 c99028bc82
commit 986cead30c
7 changed files with 115 additions and 114 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/release-control/v6/internal/subsystems/performance-and-scalability.md
+10 -5
View File
@@ -2463,7 +2463,13 @@ through `preserveScrollableAncestorVerticalOffset` in
opened detail through `summaryTableFocus.ts`, so opening a focused workload
does not look like a full page reload. The root `frontend-modern/src/App.tsx`
shell applies a pending `frontend-modern/src/utils/appShellScrollRestoration.ts`
restore only when the route itself changes. The shared same-path scheduler must also own cleanup
restore on the next route change, whatever caused it, so only the shared
same-path route-state scheduler may stage one, from the shell position at the
moment it navigates. Local row focus stages none: a position staged without a
navigation waits for an unrelated one, and before this rule a Type filter
change made at the top of the Proxmox overview jumped the shell back down to
the row toggled earlier, because the scheduler stages nothing from scrollTop 0.
The shared same-path scheduler must also own cleanup
for every deferred scroll-restore timeout and animation frame it creates, so
route-state cleanup cannot leave hot-path replay work running after the owning
surface unmounts.
@@ -2483,10 +2489,9 @@ That same hot-path ownership now also covers deliberate inline-detail reveal.
When a focused workload or infrastructure row opens its inline detail, the hot
path may preserve scroll across same-route state writes, but the actual reveal
must still flow through the shared contextual-focus and summary-table helpers.
Direct row toggles that already have the row in view must capture the current
app-shell scroll position before the focus write and let the remounted root
shell restore that position, so the interaction stays anchored instead of
looking like a full refresh. Once the drawer is mounted, the shared reveal
Direct row toggles that already have the row in view stay anchored through
that contextual-focus offset preservation, not through a staged app-shell
restore, so the interaction does not look like a full refresh. Once the drawer is mounted, the shared reveal
helper must still take over whenever the opened detail would land below the
fold, marking that movement as deliberate so route-state restore does not
replay over it and then scrolling only enough to keep the row header plus the
docs/release-control/v6/internal/subsystems/unified-resources.md
+7 -4
View File
@@ -4124,10 +4124,13 @@ inline drawer through local component state, most through
while the Docker hosts and Proxmox nodes tables keep their own selection
signal, and write no route state. An infrastructure table that moves drawer selection
into route state must tag the opened detail with the canonical active resource
ID; direct toggles of a row already in view must capture the current
`.app-scroll-shell` position through
`frontend-modern/src/utils/appShellScrollRestoration.ts`, so the remounted root
shell in `frontend-modern/src/App.tsx` stays anchored, and then hand off to the
ID; its route write must go through the shared same-path route-state scheduler
in `frontend-modern/src/utils/routeStateNavigation.ts`, which alone stages the
`.app-scroll-shell` position in
`frontend-modern/src/utils/appShellScrollRestoration.ts` at the moment it
navigates, so the root shell in `frontend-modern/src/App.tsx` stays anchored.
The row toggle itself stages nothing: the root shell applies a staged position
on the next route change, whatever caused it. The table must then hand off to the
shared summary-table/contextual-focus helpers when the opened drawer would fall
below the fold. That reveal scrolls only enough to keep the row header plus the
start of the detail visible, never leaving the drawer clipped or hard-centering
frontend-modern/browser-verification/b3c60348af17e8cc.json
+47
View File
@@ -0,0 +1,47 @@
{
"version": 1,
"base_sha": "03767f1e956c68c7b826635990638ea45fd9ddbd",
"verified_at": "2026-10-08T00:45:00Z",
"result": "passed",
"changed_paths": [
"frontend-modern/src/components/Workloads/useWorkloadSelectionState.ts",
"frontend-modern/src/utils/appShellScrollRestoration.ts"
],
"content_sha256": {
"frontend-modern/src/components/Workloads/useWorkloadSelectionState.ts": "4b125b316052a801d9db7568cd584e7165b73995795c1943770bd9133c363f3c",
"frontend-modern/src/utils/appShellScrollRestoration.ts": "b20770a6edd2196ba436de1aadf1bb8dcdf84baf0da492afa6e5255ebfd91fd5"
},
"routes": [
"/proxmox/overview",
"/proxmox/overview?type=vm",
"/proxmox/overview?resource=Analytics%3Apve34%3A740",
"/proxmox/storage",
"/docker/overview"
],
"viewports": [
{
"width": 1440,
"height": 900
},
{
"width": 390,
"height": 844
}
],
"states": [
"Embedded build of this exact tree (both runtime files checked by sha256 on the test host) on a mock DEMO_MODE stack on pulse-dev; baseline is an embedded build of origin/main 7f45148d15 run with the same probe script, which instruments every scripted scrollTop write on .app-scroll-shell",
"Baseline repro at 1440x900 and 390x844: guest drawer opened at shell scrollTop 3000 (URL unchanged), shell scrolled to 0, Type filter set to VMs; URL became ?type=vm and 74 ms (desktop) / 126 ms (phone) later a scripted write moved the shell to 3000",
"This tree, same steps at both widths: shell stays at 0 after ?type=vm, no scripted shell write",
"Controls unchanged on both builds: same scroll path without a drawer stays at 0; filter clicked at scrollTop 500 after a drawer toggle keeps 500 (phone: 41 after the Filters sheet opens); Proxmox Storage sub-tab (both widths) and Docker tab (desktop; on phones the tab is not in the visible nav) after a drawer toggle land at 0",
"Opening a guest drawer keeps the shell at 3000 with the URL unchanged (local selection, contextual-focus offset preservation)",
"?resource= deep link opens the matching guest drawer and reveals it (shell 2960 desktop, 3143 phone); closing it keeps that position",
"No page errors at either width"
],
"interactions": [
"Row disclosure button click (desktop) and row tap (phone) to open and close a guest drawer",
"Type filter VMs button (desktop inline group; phone via the toolbar Filters sheet)",
"Proxmox Storage sub-tab link and Docker platform tab link",
"Drawer close button on a deep-linked drawer",
"rAF sampling of .app-scroll-shell scrollTop and URL for 2.5 s after each action"
]
}
frontend-modern/src/components/Workloads/__tests__/useWorkloadSelectionState.test.ts
+46
View File
@@ -3,6 +3,14 @@ import { createSignal } from 'solid-js';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import type { WorkloadGuest } from '@/types/workloads';
import {
clearPendingAppShellRestoreTop,
readPendingAppShellRestoreTop,
} from '@/utils/appShellScrollRestoration';
import {
ROUTE_STATE_REPLACE_OPTIONS,
createRouteStateNavigateScheduler,
} from '@/utils/routeStateNavigation';
import { resolveWorkloadResourceSelection } from '../workloadSelectionModel';
import { useWorkloadSelectionState } from '../useWorkloadSelectionState';
@@ -302,4 +310,42 @@ describe('useWorkloadSelectionState', () => {
expect(result.selectedGuestId()).toBeNull();
expect(navigateSpy).not.toHaveBeenCalled();
});
it('stages no app-shell restore for a local row toggle that a later route change could replay', () => {
locationSearch = '';
clearPendingAppShellRestoreTop();
const shell = document.createElement('div');
shell.className = 'app-scroll-shell';
shell.scrollTop = 3000;
document.body.appendChild(shell);
const [filteredGuests] = createSignal<WorkloadGuest[]>([]);
const { result } = renderHook(() =>
useWorkloadSelectionState({
filteredGuests,
}),
);
result.setSelectedGuestId('cluster-a:node-1:101');
vi.runAllTimers();
expect(result.selectedGuestId()).toBe('cluster-a:node-1:101');
expect(readPendingAppShellRestoreTop()).toBeNull();
result.setSelectedGuestId(null);
vi.runAllTimers();
expect(readPendingAppShellRestoreTop()).toBeNull();
// Back at the top, a filter change writes the URL. The scheduler stages a
// restore only from a scrolled shell, so App.tsx would replay anything the
// row toggle had staged and jump the page back to 3000.
shell.scrollTop = 0;
const navigate = vi.fn();
const scheduler = createRouteStateNavigateScheduler(navigate, () => '/proxmox/overview');
scheduler.schedule('/proxmox/overview?type=vm');
vi.runAllTimers();
expect(navigate).toHaveBeenCalledWith('/proxmox/overview?type=vm', ROUTE_STATE_REPLACE_OPTIONS);
expect(readPendingAppShellRestoreTop()).toBeNull();
expect(navigateSpy).not.toHaveBeenCalled();
});
});
frontend-modern/src/components/Workloads/useWorkloadSelectionState.ts
+5 -9
View File
@@ -4,7 +4,6 @@ import { createEffect, createSignal, type Accessor } from 'solid-js';
import { preserveScrollableAncestorVerticalOffset } from '@/components/shared/contextualFocus';
import { useSummaryPageInteractionState } from '@/components/shared/summaryTableFocus';
import type { WorkloadGuest } from '@/types/workloads';
import { capturePendingAppShellRestoreTop } from '@/utils/appShellScrollRestoration';
import {
workloadsHasHoveredWorkload,
resolveWorkloadResourceSelection,
@@ -58,30 +57,27 @@ export function useWorkloadSelectionState(options: UseWorkloadsSelectionStateOpt
summaryInteraction.setClearSurfaceRootRef(element);
};
const setSelectedGuestIdState = (id: string | null) => {
// Row focus is local: it never navigates, so it must not stage an app-shell
// restore that the next unrelated route change would replay.
const setSelectedGuestId = (id: string | null) => {
preserveScrollableAncestorVerticalOffset(tableWrapperRef(), () => {
setSelectedGuestIdRaw(id);
});
};
const setSelectedGuestId = (id: string | null) => {
capturePendingAppShellRestoreTop();
setSelectedGuestIdState(id);
};
createEffect(() => {
if (options.routeStateEnabled && !options.routeStateEnabled()) return;
const resourceId = resolveWorkloadResourceSelection(location.search);
if (!resourceId) {
if (handledResourceId() !== null) {
setSelectedGuestIdState(null);
setSelectedGuestId(null);
setHandledResourceId(null);
}
return;
}
if (resourceId !== handledResourceId()) {
setSelectedGuestIdState(resourceId);
setSelectedGuestId(resourceId);
setHandledResourceId(resourceId);
}
});
frontend-modern/src/utils/__tests__/appShellScrollRestoration.branchcov0712c.test.ts
-84
View File
@@ -1,84 +0,0 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import {
capturePendingAppShellRestoreTop,
clearPendingAppShellRestoreTop,
readPendingAppShellRestoreTop,
} from '@/utils/appShellScrollRestoration';
describe('capturePendingAppShellRestoreTop – branch coverage', () => {
beforeEach(() => {
clearPendingAppShellRestoreTop();
document.body.innerHTML = '';
});
afterEach(() => {
vi.unstubAllGlobals();
document.body.innerHTML = '';
});
it('returns early without capturing when document is undefined (SSR guard arm)', () => {
vi.stubGlobal('document', undefined);
capturePendingAppShellRestoreTop();
expect(readPendingAppShellRestoreTop()).toBeNull();
});
it('does not capture when no .app-scroll-shell element exists (!shell short-circuit arm)', () => {
capturePendingAppShellRestoreTop();
expect(readPendingAppShellRestoreTop()).toBeNull();
});
it('does not capture when the shell is at the top (scrollTop === 0 hits the <= 0 boundary arm)', () => {
const shell = document.createElement('div');
shell.className = 'app-scroll-shell';
shell.scrollTop = 0;
document.body.appendChild(shell);
capturePendingAppShellRestoreTop();
expect(readPendingAppShellRestoreTop()).toBeNull();
});
it('does not capture when the shell reports a negative scrollTop (defensive <= 0 arm)', () => {
// Real browsers never report a negative scrollTop, so inject a stub
// element via querySelector to force the <= 0 guard's negative side and
// confirm the exact selector contract.
const stubShell = { scrollTop: -10 } as unknown as HTMLElement;
const querySpy = vi.spyOn(document, 'querySelector').mockReturnValue(stubShell);
capturePendingAppShellRestoreTop();
expect(readPendingAppShellRestoreTop()).toBeNull();
expect(querySpy).toHaveBeenCalledWith('.app-scroll-shell');
querySpy.mockRestore();
});
it('captures the exact scrollTop when the shell is scrolled past the top (happy path)', () => {
const shell = document.createElement('div');
shell.className = 'app-scroll-shell';
shell.scrollTop = 327;
document.body.appendChild(shell);
capturePendingAppShellRestoreTop();
expect(readPendingAppShellRestoreTop()).toBe(327);
});
it('preserves a previously captured value when a later capture hits a guard arm (no-overwrite)', () => {
const scrolled = document.createElement('div');
scrolled.className = 'app-scroll-shell';
scrolled.scrollTop = 150;
document.body.appendChild(scrolled);
capturePendingAppShellRestoreTop();
expect(readPendingAppShellRestoreTop()).toBe(150);
scrolled.remove();
capturePendingAppShellRestoreTop();
expect(readPendingAppShellRestoreTop()).toBe(150);
});
});
frontend-modern/src/utils/appShellScrollRestoration.ts
-12
View File
@@ -1,21 +1,9 @@
let pendingAppShellRestoreTop: number | null = null;
const APP_SCROLL_SHELL_SELECTOR = '.app-scroll-shell';
export const schedulePendingAppShellRestoreTop = (scrollTop: number): void => {
pendingAppShellRestoreTop = Math.max(0, scrollTop);
};
export const capturePendingAppShellRestoreTop = (): void => {
if (typeof document === 'undefined') {
return;
}
const shell = document.querySelector<HTMLElement>(APP_SCROLL_SHELL_SELECTOR);
if (!shell || shell.scrollTop <= 0) {
return;
}
schedulePendingAppShellRestoreTop(shell.scrollTop);
};
export const readPendingAppShellRestoreTop = (): number | null => pendingAppShellRestoreTop;
export const clearPendingAppShellRestoreTop = (): void => {