From b632872eeff94ae17e720f6ec3a8b81e1240f883 Mon Sep 17 00:00:00 2001 From: Edwin <18327273+thinkbig1979@users.noreply.github.com> Date: Fri, 31 Jul 2026 18:28:35 +0200 Subject: [PATCH] fix(dashboard): restore sort/filter in the state initialiser, not an effect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dependabot's npm group PR (#22) fails frontend CI at Lint. The code it trips over is pre-existing and untouched by that PR — what changed is the linter. eslint-plugin-react-hooks moves ^7.0.1 -> ^7.1.1 in that group, and 7.1.x enforces react-hooks/set-state-in-effect: DashboardPage.tsx 77:20 error Calling setState synchronously within an effect can trigger cascading renders react-hooks/set-state-in-effect Left alone this fails every weekly npm group PR, so none of them get merged and Dependabot stops being worth having. The rule is pointing at something real rather than being noise, so this fixes the effect instead of disabling the rule. Reading localStorage in the useState initialiser gives the same result in one render instead of two: the mount effect rendered the defaults, then re-rendered with the stored values. Verified against #22's actual dependency set rather than against main's: git worktree add pr22 && pnpm install --frozen-lockfile node ./node_modules/eslint/bin/eslint.js . before this change: 1 error, exit 1 (the error quoted above, and it was the only one in the tree) after this change: 0 errors, exit 0 Also adds the first coverage of the localStorage-restore contract, which had none. Those two tests pin the restored values the page hands to StacksTab; they do NOT distinguish this implementation from the effect-based one, and say so — StacksTab is gated behind the loading state, so it first renders after effects have already flushed and both versions look identical from the DOM. Confirmed by reverting the fix with the tests in place: they still passed. The guard against a relapse is the lint rule itself. Gates: eslint exit 0, tsc -b clean, 823 tests in 80 files (821 baseline plus the two added here), build ok. Refs: agent-os-14b Co-Authored-By: Claude Opus 5 --- frontend/src/pages/DashboardPage.tsx | 19 +++++---- .../pages/__tests__/DashboardPage.test.tsx | 42 ++++++++++++++++++- 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/frontend/src/pages/DashboardPage.tsx b/frontend/src/pages/DashboardPage.tsx index cc8ccfa..6536677 100644 --- a/frontend/src/pages/DashboardPage.tsx +++ b/frontend/src/pages/DashboardPage.tsx @@ -65,19 +65,20 @@ export function DashboardPage() { const queryClient = useQueryClient() const { confirm, ConfirmComponent } = useConfirm() const [deletingStackId, setDeletingStackId] = useState(null) - const [sortBy, setSortBy] = useState('name') - const [statusFilter, setStatusFilter] = useState('all') + // Read the persisted sort/filter in the initialiser rather than in a mount + // effect. An effect would render once with the default and again with the + // stored value, which is both a visible flash of the wrong sort order and + // what react-hooks/set-state-in-effect flags. + const [sortBy, setSortBy] = useState( + () => (localStorage.getItem('dashboard-sort') as SortOption) || 'name', + ) + const [statusFilter, setStatusFilter] = useState( + () => (localStorage.getItem('dashboard-filter') as StatusFilter) || 'all', + ) const [isRefreshing, setIsRefreshing] = useState(false) const { containers: metricsContainers, aggregates, latestMetrics, isConnected: metricsConnected, ws: metricsWs } = useDashboardMetrics() - useEffect(() => { - const savedSort = localStorage.getItem('dashboard-sort') as SortOption - const savedFilter = localStorage.getItem('dashboard-filter') as StatusFilter - if (savedSort) setSortBy(savedSort) - if (savedFilter) setStatusFilter(savedFilter) - }, []) - useEffect(() => { localStorage.setItem('dashboard-sort', sortBy) localStorage.setItem('dashboard-filter', statusFilter) diff --git a/frontend/src/pages/__tests__/DashboardPage.test.tsx b/frontend/src/pages/__tests__/DashboardPage.test.tsx index 2a6409b..73520ea 100644 --- a/frontend/src/pages/__tests__/DashboardPage.test.tsx +++ b/frontend/src/pages/__tests__/DashboardPage.test.tsx @@ -70,8 +70,12 @@ vi.mock('@/components/dashboard/DashboardHeader', () => ({ vi.mock('@/components/dashboard/DashboardMetricsTab', () => ({ DashboardMetricsTab: () =>
, })) +// Surfaces the sort/filter the page hands down, so the localStorage-restore +// contract is observable without exercising StacksTab's internals. vi.mock('@/components/dashboard/StacksTab', () => ({ - StacksTab: () =>
, + StacksTab: ({ sortBy, statusFilter }: { sortBy: string; statusFilter: string }) => ( +
+ ), })) vi.mock('@/components/dashboard/ContainersOverviewTab', () => ({ ContainersOverviewTab: () =>
, @@ -144,6 +148,9 @@ function renderPage(route = '/') { describe('DashboardPage', () => { beforeEach(() => { vi.clearAllMocks() + // The page persists sort/filter on every change, so state leaks between + // tests unless this is cleared. + localStorage.clear() listDirectories.mockResolvedValue([]) listStacks.mockResolvedValue([]) dashboardStats.mockResolvedValue({ runningContainers: 0 }) @@ -243,6 +250,39 @@ describe('DashboardPage', () => { expect(screen.getByText('Create Your First Stack')).toBeInTheDocument() }) + // Pins the localStorage-restore contract itself, which had no coverage + // before agent-os-14b moved the read out of a mount effect and into the + // useState initialiser. These assert the restored values are what the page + // hands to StacksTab; they deliberately do NOT claim to distinguish the two + // implementations. They cannot: StacksTab is gated behind the loading state, + // so it first renders after effects have already flushed, and both versions + // look identical from the DOM. Verified by reverting the fix — these still + // passed. The guard against a relapse to setState-in-effect is the + // react-hooks/set-state-in-effect lint rule, not this file. + it('restores the persisted sort and filter', async () => { + localStorage.setItem('dashboard-sort', 'status') + localStorage.setItem('dashboard-filter', 'running') + listStacks.mockResolvedValue([makeStack({ id: 's1' })]) + listDirectories.mockResolvedValue([makeDir({ path: '/stacks/s1' })]) + + renderPage('/?tab=stacks') + + const tab = await screen.findByTestId('tab-stacks') + expect(tab).toHaveAttribute('data-sort-by', 'status') + expect(tab).toHaveAttribute('data-status-filter', 'running') + }) + + it('falls back to the defaults when nothing is persisted', async () => { + listStacks.mockResolvedValue([makeStack({ id: 's1' })]) + listDirectories.mockResolvedValue([makeDir({ path: '/stacks/s1' })]) + + renderPage('/?tab=stacks') + + const tab = await screen.findByTestId('tab-stacks') + expect(tab).toHaveAttribute('data-sort-by', 'name') + expect(tab).toHaveAttribute('data-status-filter', 'all') + }) + it('hides the Quick Start empty state once stacks exist', async () => { listStacks.mockResolvedValue([makeStack({ id: 's1' })]) listDirectories.mockResolvedValue([makeDir({ path: '/stacks/s1' })])