From 424c7a0e593b1339d8ae697ad99c73bde2986bf8 Mon Sep 17 00:00:00 2001 From: "roboco-app[bot]" <302741806+roboco-app[bot]@users.noreply.github.com> Date: Thu, 30 Jul 2026 19:46:01 +0000 Subject: [PATCH] [7f87f002] Audit and resolve eslint-disable in journals-view.tsx (#739) (#740) * [7f87f002] fix(panel): remove eslint-disable in journals-view mount effect Replace the empty-deps + eslint-disable-next-line react-hooks/exhaustive-deps mount-only localStorage-restore effect with a useRef mount-guard, so the deps array can honestly list router/searchParams/urlAgentId/urlType/urlTask while still only running the restore once per mount. Adds regression tests proving the restore never re-fires after a later filter clear, which a naive "just add the deps" fix would have broken. * [7f87f002] docs(frontend): document journals-view mount-guard eslint-disable fix --------- Co-authored-by: roboco-app[bot] <302741806+roboco-app[bot]@users.noreply.github.com> Co-authored-by: Frontend Developer 1 Co-authored-by: Frontend Documenter --- docs/frontend/hooks.md | 23 ++++ .../journals/__tests__/journals-view.test.tsx | 102 ++++++++++++++++++ .../src/components/journals/journals-view.tsx | 15 ++- 3 files changed, 136 insertions(+), 4 deletions(-) create mode 100644 panel/src/components/journals/__tests__/journals-view.test.tsx diff --git a/docs/frontend/hooks.md b/docs/frontend/hooks.md index 8480e499..bc552249 100644 --- a/docs/frontend/hooks.md +++ b/docs/frontend/hooks.md @@ -138,3 +138,26 @@ export function TaskDetail({ taskId }: { taskId: string | undefined }) { ``` No manual guard is needed before calling the hook — the `enabled: !!taskId` guard is built in and prevents wasted API calls and race conditions. + +## Mount-only effect audit: eslint-disable in `journals-view.tsx` + +Sentinel's `no_lint_suppressions` hygiene scan flagged `// eslint-disable-next-line react-hooks/exhaustive-deps` guarding the mount-only localStorage-restore effect in `JournalsViewContent` (`panel/src/components/journals/journals-view.tsx`). That effect restores the `agent`/`type`/`task` filters saved from a prior visit into the URL, but only on a fresh `/agents?tab=journals` visit that carries no query params yet — it must run exactly once per mount, never again, or it would clobber a later intentional "clear filters" action with stale saved state. + +### Audit result: fixed at the source, no waiver needed + +The suppression was removed by replacing the empty `[]` dependency array with a `useRef` mount-guard: + +```tsx +const hasRestoredRef = useRef(false); +useEffect(() => { + if (hasRestoredRef.current) return; + hasRestoredRef.current = true; + // ...restore-from-localStorage logic, reading urlAgentId/urlType/urlTask/searchParams/router +}, [urlAgentId, urlType, urlTask, searchParams, router]); +``` + +The ref guard, not the (now honest and complete) dependency array, is what enforces "exactly once per mount" — so `react-hooks/exhaustive-deps` is satisfied without changing the effect's actual behavior. This is the same idiom already used elsewhere in the panel for mount-only effects: `panel/src/components/scroll-restoration.tsx` (`hasRestored`) and `panel/src/components/a2a/a2a-transcript.tsx` (`hasScrolledRef`). Prefer this pattern over `eslint-disable` + `[]` for any new mount-only effect — it keeps the dependency array truthful for future maintainers while still only firing once. + +A naive fix that just added `router`/`searchParams` to the deps array **without** the ref guard would have been wrong: those values change across every navigation, so the effect would re-run on every subsequent URL change and re-apply the stale saved filters over a user's later, intentional filter clear. A regression suite, `panel/src/components/journals/__tests__/journals-view.test.tsx`, locks in the correct behavior: the restore fires once on a fresh no-param visit, is skipped when the URL already carries params, and — the key regression guard — never re-fires after mount even once the URL round-trips through params and back to empty. + +No entry was added to `.roboco/conventions.yml`'s `waivers:` list — the suppression was eliminated at the source, not exempted. (The backend cell's unrelated waiver entries already committed to that file were left untouched.) diff --git a/panel/src/components/journals/__tests__/journals-view.test.tsx b/panel/src/components/journals/__tests__/journals-view.test.tsx new file mode 100644 index 00000000..4a7bf620 --- /dev/null +++ b/panel/src/components/journals/__tests__/journals-view.test.tsx @@ -0,0 +1,102 @@ +import { describe, it, expect, beforeEach, vi } from "vitest"; +import { render, act } from "@testing-library/react"; +import type { ReactNode } from "react"; +import { PageRefreshProvider } from "@/components/providers"; + +// Sentinel flagged a react-hooks/exhaustive-deps suppression guarding the +// mount-only localStorage-restore effect. Fixed via a useRef mount guard +// instead of an empty deps array — this suite locks in that the restore +// fires exactly once and never re-fires to clobber a later, intentional +// filter clear (the hazard an honest-but-naive deps array would introduce). + +const mockReplace = vi.fn(); +// Stable per-render objects, same idiom as a2a-view.test.tsx — a fresh +// object/URLSearchParams on every render would make the effect's deps +// (searchParams, router) look "changed" even when the real URL hasn't +// moved, which would mask the very bug this test guards against. +const mockRouter = { replace: mockReplace, push: vi.fn() }; +let searchParams = new URLSearchParams(); +vi.mock("next/navigation", () => ({ + useRouter: () => mockRouter, + useSearchParams: () => searchParams, +})); + +vi.mock("@/hooks/use-agents", () => ({ + useAgents: () => ({ data: [], isLoading: false, refetch: vi.fn() }), +})); + +import { JournalsView } from "../journals-view"; + +const STORAGE_KEY = "roboco-journals-state"; + +function withPageRefresh(ui: ReactNode) { + return {ui}; +} + +describe("JournalsView — mount-only localStorage restore", () => { + beforeEach(() => { + mockReplace.mockClear(); + searchParams = new URLSearchParams(); + localStorage.clear(); + }); + + it("restores the saved agent filter into the URL on a fresh visit with no params", async () => { + localStorage.setItem( + STORAGE_KEY, + JSON.stringify({ agent: "be-dev-1", q: null, type: null, task: null }), + ); + + await act(async () => { + render(withPageRefresh()); + }); + + expect(mockReplace).toHaveBeenCalledTimes(1); + expect(mockReplace).toHaveBeenCalledWith("/agents?agent=be-dev-1"); + }); + + it("does not restore when the URL already carries params", async () => { + searchParams = new URLSearchParams("agent=be-qa"); + localStorage.setItem( + STORAGE_KEY, + JSON.stringify({ agent: "be-dev-1", q: null, type: null, task: null }), + ); + + await act(async () => { + render(withPageRefresh()); + }); + + expect(mockReplace).not.toHaveBeenCalled(); + }); + + it("never re-fires the restore after mount, even once params are cleared back to empty", async () => { + localStorage.setItem( + STORAGE_KEY, + JSON.stringify({ agent: "be-dev-1", q: null, type: null, task: null }), + ); + + let rerender!: (ui: ReactNode) => void; + await act(async () => { + const result = render(withPageRefresh()); + rerender = result.rerender; + }); + expect(mockReplace).toHaveBeenCalledTimes(1); + mockReplace.mockClear(); + + // Simulate the URL round-tripping to carry params (post-restore) and + // then a user intentionally clearing every filter — a fresh + // URLSearchParams("") is referentially new, matching how the real + // `next/navigation` value changes across an actual navigation, so this + // is the exact scenario the naive "add router/searchParams to deps" + // fix would break by re-restoring over the user's clear. + searchParams = new URLSearchParams("agent=be-dev-1"); + await act(async () => { + rerender(withPageRefresh()); + }); + searchParams = new URLSearchParams(""); + await act(async () => { + rerender(withPageRefresh()); + }); + + expect(mockReplace).not.toHaveBeenCalled(); + }); +}); diff --git a/panel/src/components/journals/journals-view.tsx b/panel/src/components/journals/journals-view.tsx index 9ad256eb..29a3e94d 100644 --- a/panel/src/components/journals/journals-view.tsx +++ b/panel/src/components/journals/journals-view.tsx @@ -1,6 +1,6 @@ "use client"; -import { Suspense, useCallback, useEffect, useState } from "react"; +import { Suspense, useCallback, useEffect, useRef, useState } from "react"; import { useSearchParams, useRouter } from "next/navigation"; import { useAgents } from "@/hooks/use-agents"; import { JournalEntryType } from "@/types"; @@ -61,8 +61,16 @@ function JournalsViewContent() { () => loadJournalsState()?.q ?? "", ); - // Restore from localStorage if URL has no params (fresh navigation) + // Restore from localStorage if URL has no params (fresh navigation). + // Guarded by a ref (not an empty deps array) so the deps list can honestly + // include everything the body reads while still firing exactly once per + // mount — otherwise the URL params clearing later (an intentional + // "remove filters" action) would re-trigger this and clobber that clear + // with the stale saved state. + const hasRestoredRef = useRef(false); useEffect(() => { + if (hasRestoredRef.current) return; + hasRestoredRef.current = true; const hasUrlParams = urlAgentId || urlType || urlTask; if (!hasUrlParams) { const saved = loadJournalsState(); @@ -74,8 +82,7 @@ function JournalsViewContent() { router.replace(`/agents?${params.toString()}`); } } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); // Intentionally only run on mount + }, [urlAgentId, urlType, urlTask, searchParams, router]); // Derive state from URL const selectedAgentId = urlAgentId;