[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 <fe-dev-1@roboco.tech>
Co-authored-by: Frontend Documenter <fe-doc@roboco.tech>

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.)

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 <PageRefreshProvider>{ui}</PageRefreshProvider>;
+}
+
+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(<JournalsView />));
+    });
+
+    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(<JournalsView />));
+    });
+
+    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(<JournalsView />));
+      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(<JournalsView />));
+    });
+    searchParams = new URLSearchParams("");
+    await act(async () => {
+      rerender(withPageRefresh(<JournalsView />));
+    });
+
+    expect(mockReplace).not.toHaveBeenCalled();
+  });
+});

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;