review: split useViewState into a read-only data hook

Both bugbot comments pointed at the same root cause: useViewState
returns a fresh object every render, and PullRequestLayout was calling
it just to read isChapterViewed — instantiating four mutation hooks it
never used and tagging them as a useMemo dep that never stabilized.

Adds useViewStateData(runId) that returns memoized Sets + helpers + the
loading/error fields. useViewState now wraps it and adds the four
mutations on top, sharing the same query cache. PullRequestLayout uses
useViewStateData and depends on chapterIdSet (a stable reference) for
the viewedCount memo, so the count actually caches across renders and
no unused mutation hooks get instantiated.

ChaptersList still calls useViewState since it needs both reads and
mutations. The two hooks share the same query key, so there's still
exactly one network fetch per runId.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: dastratakos

web/src/lib/use-view-state.ts

@@ -42,30 +42,59 @@ const postKeyChangeView = (id: string) =>
 const deleteKeyChangeView = (id: string) =>
   jsonFetch<unknown>(`/api/key-change-view/${encodeURIComponent(id)}`, { method: "DELETE" });
 
-export interface UseViewStateResult {
-  markChapterViewed: (chapterId: string) => void;
-  unmarkChapterViewed: (chapterId: string) => void;
+export interface UseViewStateDataResult {
+  /** Stable reference; mutates only when the underlying query data changes. */
+  chapterIdSet: ReadonlySet<string>;
+  /** Stable reference; mutates only when the underlying query data changes. */
+  keyChangeIdSet: ReadonlySet<string>;
   isChapterViewed: (chapterId: string) => boolean;
-  markKeyChangeChecked: (keyChangeId: string) => void;
-  unmarkKeyChangeChecked: (keyChangeId: string) => void;
   isKeyChangeChecked: (keyChangeId: string) => boolean;
   isLoading: boolean;
   error: unknown;
 }
 
-export function useViewState(runId: string): UseViewStateResult {
-  const queryClient = useQueryClient();
-  const queryKey = useMemo(() => viewStateQueryKey(runId), [runId]);
+export interface UseViewStateResult extends UseViewStateDataResult {
+  markChapterViewed: (chapterId: string) => void;
+  unmarkChapterViewed: (chapterId: string) => void;
+  markKeyChangeChecked: (keyChangeId: string) => void;
+  unmarkKeyChangeChecked: (keyChangeId: string) => void;
+}
 
+/**
+ * Read-only view-state for components that just need to know what's viewed.
+ * Returns memoized Sets so callers can include them as effect/memo deps
+ * without re-running on every render. Components that also need to mutate
+ * view-state should use `useViewState` instead — calling this hook avoids
+ * instantiating the four mutation hooks.
+ */
+export function useViewStateData(runId: string): UseViewStateDataResult {
   const { data, isLoading, error } = useQuery<ViewState>({
-    queryKey,
+    queryKey: viewStateQueryKey(runId),
     queryFn: () => fetchViewState(runId),
     enabled: runId !== "",
   });
 
   const chapterIdSet = useMemo(() => new Set(data?.chapterIds ?? []), [data?.chapterIds]);
   const keyChangeIdSet = useMemo(() => new Set(data?.keyChangeIds ?? []), [data?.keyChangeIds]);
 
+  return useMemo(
+    () => ({
+      chapterIdSet,
+      keyChangeIdSet,
+      isChapterViewed: (id: string) => chapterIdSet.has(id),
+      isKeyChangeChecked: (id: string) => keyChangeIdSet.has(id),
+      isLoading,
+      error,
+    }),
+    [chapterIdSet, keyChangeIdSet, isLoading, error],
+  );
+}
+
+export function useViewState(runId: string): UseViewStateResult {
+  const queryClient = useQueryClient();
+  const queryKey = useMemo(() => viewStateQueryKey(runId), [runId]);
+  const data = useViewStateData(runId);
+
   // Optimistic-update helpers. Await cancelQueries before writing so any
   // in-flight refetch can't resolve and overwrite the optimistic cache.
   // onError rolls back to the snapshot; onSettled invalidates to reconcile
@@ -136,13 +165,10 @@ export function useViewState(runId: string): UseViewStateResult {
   });
 
   return {
+    ...data,
     markChapterViewed: markChapterMutation.mutate,
     unmarkChapterViewed: unmarkChapterMutation.mutate,
-    isChapterViewed: (id) => chapterIdSet.has(id),
     markKeyChangeChecked: markKeyChangeMutation.mutate,
     unmarkKeyChangeChecked: unmarkKeyChangeMutation.mutate,
-    isKeyChangeChecked: (id) => keyChangeIdSet.has(id),
-    isLoading,
-    error,
   };
 }

web/src/routes/pull-request-layout.tsx

@@ -4,7 +4,7 @@ import { useMemo, useState } from "react";
 import { SectionLabel } from "@/components/pull-request/section-label";
 import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip";
 import type { ChaptersResponse } from "@/lib/api-types";
-import { jsonFetch, useViewState } from "@/lib/use-view-state";
+import { jsonFetch, useViewStateData } from "@/lib/use-view-state";
 import { cn } from "@/lib/utils";
 import { ChaptersIndexPage } from "./chapters-index-page";
 
@@ -99,15 +99,17 @@ export function PullRequestLayout({ runId }: { runId: string }) {
   const [activeTab, setActiveTab] = useState<PrTab>(PR_TAB.CHAPTERS);
 
   // Lift the viewed count out of ChaptersIndexPage so the tab strip can render
-  // "X/N viewed" — same hook instance, same query key, no extra network calls.
-  const view = useViewState(runId);
+  // "X/N viewed". Read-only hook avoids instantiating the four mutation hooks
+  // we don't use here, and chapterIdSet is a stable reference so the memo
+  // actually caches across renders.
+  const { chapterIdSet } = useViewStateData(runId);
   const chapters = data?.chapters;
   const viewedCount = useMemo(() => {
     if (!chapters) return 0;
     let n = 0;
-    for (const c of chapters) if (view.isChapterViewed(c.externalId)) n++;
+    for (const c of chapters) if (chapterIdSet.has(c.externalId)) n++;
     return n;
-  }, [chapters, view]);
+  }, [chapters, chapterIdSet]);
 
   // Mirrors hosted's chapterCountLabel: just the total when nothing's been
   // viewed yet, otherwise "X/N viewed". Drops the count entirely if the