refactor: consolidate viewport hooks and remove defineProperty matchMedia stub (#28460)

Follow-up to review feedback on #28387.

That PR added three hooks for one feature (`useIsBelowLgViewport`,
`useIsBelowMdViewport`, and the local single-use
`useRightPanelNarrowSuppression`) and stubbed `window.matchMedia` in
tests with `Object.defineProperty`.

- Replace the two single-purpose viewport hooks with one generic
`useMediaQuery(query)`. Callers pass the shared Tailwind-aligned query
constants from `utils/mobile.ts`. The one new hook replaces the two
deleted ones (net -1).
- Inline `useRightPanelNarrowSuppression` into `AgentChatPage`, its only
consumer, and drop its `renderHook` unit suite. The behavior stays
covered by the narrow-viewport stories; the widening-restore case moved
into the `NarrowingSuppressesExpandedPanel` play function.
- Rework `testHelpers/matchMedia.ts` to install the stub with `spyOn`
from `storybook/test` instead of `Object.defineProperty`. The helper is
story-only now (stories run in real Chromium; jsdom has no
`matchMedia`), so any future unit test needing a stub should use
`vi.stubGlobal` directly.
- Encode the feedback in the canonical FE contract so it gets caught
during development: `.claude/docs/FRONTEND_PATTERNS.md` now bans new
React hooks when an existing hook, a plain function, or component state
suffices (FE3) and bans replacing browser globals with
`Object.defineProperty` in tests or stories (FE9: `vi.stubGlobal` /
`spyOn`), and notes that `renderHook` suites for stateful UI hooks
belong in the consuming component's story (FE1). The `frontend-review`
skill checklist flags all three. `site/AGENTS.md` is unchanged since it
already defers to the patterns doc.

Validation: `pnpm check`, `pnpm format:check`, `pnpm lint` (biome,
types, knip, circular deps, compiler check), `AgentChatPage.test.ts` (70
passed), story runs for `AgentChatPage.stories.tsx` and
`WorkspacePill.stories.tsx` (49 passed in Chromium).

> Xum acted on Mike's behalf (@ibetitsmike).

<!-- xum-attribution: model=claude-opus-4-6 thinking=high -->

.claude/docs/FRONTEND_PATTERNS.md

@@ -30,6 +30,9 @@ function actually exercises the interaction. Jest/RTL tests are for pure logic
 - When a component depends on the current time or date, accept it as a prop or
   via context instead of reading `new Date()` or `Date.now()` internally, so
   stories render deterministically without mocking globals.
+- `renderHook` suites for stateful UI hooks are interaction tests, not pure
+  logic. Cover that behavior through the story of the component that uses the
+  hook.
 
 **Incorrect (interaction test in Jest/RTL):**
 
@@ -89,6 +92,8 @@ const config: ChatModel = parseConfig(data);
   of existing ones.
 - Use existing wrapped primitives (Combobox, dialogs, tables) instead of
   hand-assembling the underlying pieces they already wrap.
+- Do not introduce a new React hook when an existing hook, a plain function,
+  or component state can express the logic.
 - Delete dead code and unreachable branches instead of carrying them along.
 - Keep the PR scoped to one change. Move unrelated cleanups, renames, and
   drive-by refactors to separate PRs.
@@ -209,6 +214,9 @@ Decide where logic goes before reaching for `useEffect`:
   is readable on its own. Share the entity fixture, not a pre-wired query
   object.
 - Query keys in mocks follow FE7: import the constant.
+- Never replace browser globals with `Object.defineProperty` in tests or
+  stories. Use `vi.stubGlobal` in unit tests and `spyOn` from
+  `storybook/test` in stories.
 
 ## FE10: Tests assert observable behavior
 

.claude/skills/frontend-review/SKILL.md

@@ -38,14 +38,18 @@ before they see the PR.
   user-visible behavior? Then a changed or added `.stories.tsx` must exist,
   and its `play` function must perform the new interaction (open the menu,
   submit the form), not merely render. Interaction tests added to `.test.tsx`
-  files are a FAIL unless they cover pure logic.
+  files are a FAIL unless they cover pure logic; `renderHook` suites for
+  stateful UI hooks count as interaction tests and belong in the consuming
+  component's story.
 - **FE2 (types)**: Search the diff for `any`, `as unknown as`, non-null
   assertions in any form (`x!.y`, `items[0]!`, `fn()!`, `value! as T`), and
   new `as` casts. Check that API data uses types from `api/typesGenerated.ts`.
 - **FE3 (reuse/scope)**: For each new component, hook, or helper, search
   `site/src/components/` and sibling folders for an existing equivalent.
   Flag near-duplicates, hand-assembled versions of wrapped primitives, dead
-  branches, and unrelated changes bundled into the diff.
+  branches, and unrelated changes bundled into the diff. Flag new React hooks
+  that an existing hook, a plain function, or component state could replace;
+  several new single-use hooks in one diff is a FAIL.
 - **FE4 (comments)**: Read every comment line the diff adds or edits. Flag
   any comment that restates the identifier, assertion, or control flow.
   Verify surviving comments are factually correct.
@@ -68,7 +72,10 @@ before they see the PR.
   reads.
 - **FE9 (fixtures)**: Flag inline entity literals that duplicate or deviate
   from `Mock*` fixtures in `site/src/testHelpers/`, and shared pre-wired
-  query objects instead of per-story inline `{ key, data }` wiring.
+  query objects instead of per-story inline `{ key, data }` wiring. Flag any
+  `Object.defineProperty` replacement of a browser global in tests or
+  stories: unit tests stub with `vi.stubGlobal`, stories mock existing
+  globals with `spyOn` from `storybook/test`.
 - **FE10 (test queries)**: Flag `querySelector`, class-name substring
   matches, geometry assertions, `behavior: "smooth"` dependence, and
   locale-less `toLocaleString()` in changed tests and stories.

site/src/hooks/useIsBelowLgViewport.ts

@@ -1,14 +0,0 @@
-import { useSyncExternalStore } from "react";
-import {
-	belowLgViewportMediaQuery,
-	createMediaQuerySubscribe,
-	isBelowLgViewport,
-} from "#/utils/mobile";
-
-const subscribeBelowLgViewport = createMediaQuerySubscribe(
-	belowLgViewportMediaQuery,
-);
-
-export const useIsBelowLgViewport = (): boolean => {
-	return useSyncExternalStore(subscribeBelowLgViewport, isBelowLgViewport);
-};

site/src/hooks/useIsBelowMdViewport.ts

@@ -1,14 +0,0 @@
-import { useSyncExternalStore } from "react";
-import {
-	belowMdViewportMediaQuery,
-	createMediaQuerySubscribe,
-	isBelowMdViewport,
-} from "#/utils/mobile";
-
-const subscribeBelowMdViewport = createMediaQuerySubscribe(
-	belowMdViewportMediaQuery,
-);
-
-export const useIsBelowMdViewport = (): boolean => {
-	return useSyncExternalStore(subscribeBelowMdViewport, isBelowMdViewport);
-};

site/src/hooks/useMediaQuery.ts

@@ -0,0 +1,22 @@
+import { useCallback, useSyncExternalStore } from "react";
+
+/**
+ * Subscribes to a CSS media query and returns whether it currently
+ * matches, re-rendering on change. Pass a shared query constant from
+ * `utils/mobile.ts` so breakpoints stay aligned with Tailwind
+ * utilities.
+ */
+export const useMediaQuery = (query: string): boolean => {
+	const subscribe = useCallback(
+		(onStoreChange: () => void) => {
+			const mediaQuery = window.matchMedia(query);
+			mediaQuery.addEventListener("change", onStoreChange);
+			return () => mediaQuery.removeEventListener("change", onStoreChange);
+		},
+		[query],
+	);
+	return useSyncExternalStore(
+		subscribe,
+		() => window.matchMedia(query).matches,
+	);
+};

site/src/pages/AgentsPage/AgentChatPage.stories.tsx

@@ -1934,6 +1934,13 @@ export const NarrowingSuppressesExpandedPanel: Story = {
 		expect(
 			canvas.queryByRole("tab", { name: "Summary" }),
 		).not.toBeInTheDocument();
+
+		// Widening again restores the persisted panel, still expanded.
+		narrowingMedia?.setMatches(belowLgViewportMediaQuery, false);
+		await waitFor(() => {
+			expect(canvas.getByRole("tab", { name: "Summary" })).toBeVisible();
+		});
+		expect(messagesRegion.checkVisibility()).toBe(false);
 	},
 };
 

site/src/pages/AgentsPage/AgentChatPage.test.ts

@@ -18,7 +18,6 @@ import {
 	MockWorkspaceAgent,
 	MockWorkspaceApp,
 } from "#/testHelpers/entities";
-import { setupMatchMedia } from "#/testHelpers/matchMedia";
 import {
 	buildInactiveChatQueueReconciliation,
 	draftInputStorageKeyPrefix,
@@ -32,7 +31,6 @@ import {
 	settlePromotedQueueHead,
 	submitEdit,
 	useConversationEditingState,
-	useRightPanelNarrowSuppression,
 	waitForPendingChatSettingsSyncs,
 } from "./AgentChatPage";
 import type { ChatMessageInputRef } from "./components/AgentChatInput";
@@ -1407,52 +1405,3 @@ describe("isChatAgentBindingUnresolved", () => {
 		);
 	});
 });
-
-describe("useRightPanelNarrowSuppression", () => {
-	const belowLgQuery = "(max-width: 1023px)";
-
-	const setupBelowLg = (initialBelowLg: boolean) => {
-		const media = setupMatchMedia({ [belowLgQuery]: initialBelowLg });
-		return {
-			setBelowLg: (value: boolean) => media.setMatches(belowLgQuery, value),
-		};
-	};
-
-	it("suppresses the panel when mounted below the lg breakpoint", () => {
-		setupBelowLg(true);
-		const { result } = renderHook(() => useRightPanelNarrowSuppression());
-		expect(result.current.suppressed).toBe(true);
-	});
-
-	it("does not suppress the panel when mounted at or above lg", () => {
-		setupBelowLg(false);
-		const { result } = renderHook(() => useRightPanelNarrowSuppression());
-		expect(result.current.suppressed).toBe(false);
-	});
-
-	it("suppresses on narrowing and clears on widening", () => {
-		const media = setupBelowLg(false);
-		const { result } = renderHook(() => useRightPanelNarrowSuppression());
-
-		act(() => media.setBelowLg(true));
-		expect(result.current.suppressed).toBe(true);
-
-		act(() => media.setBelowLg(false));
-		expect(result.current.suppressed).toBe(false);
-	});
-
-	it("stays cleared after an explicit clearSuppression until the next narrowing", () => {
-		const media = setupBelowLg(false);
-		const { result } = renderHook(() => useRightPanelNarrowSuppression());
-
-		act(() => media.setBelowLg(true));
-		expect(result.current.suppressed).toBe(true);
-
-		act(() => result.current.clearSuppression());
-		expect(result.current.suppressed).toBe(false);
-
-		act(() => media.setBelowLg(false));
-		act(() => media.setBelowLg(true));
-		expect(result.current.suppressed).toBe(true);
-	});
-});

site/src/pages/AgentsPage/AgentChatPage.tsx

@@ -65,12 +65,12 @@ import type { ChatMessagePart } from "#/api/typesGenerated";
 import { useProxy } from "#/contexts/ProxyContext";
 import { useAuthenticated } from "#/hooks/useAuthenticated";
 import { useAIGatewayEnabled } from "#/hooks/useEmbeddedMetadata";
-import { useIsBelowLgViewport } from "#/hooks/useIsBelowLgViewport";
+import { useMediaQuery } from "#/hooks/useMediaQuery";
 import {
 	getDefaultOrganizationName,
 	useDashboard,
 } from "#/modules/dashboard/useDashboard";
-import { isMobileViewport } from "#/utils/mobile";
+import { belowLgViewportMediaQuery, isMobileViewport } from "#/utils/mobile";
 import { pageTitle } from "#/utils/page";
 import { rewriteLocalhostURL } from "#/utils/portForward";
 import { createReconnectingWebSocket } from "#/utils/reconnectingWebSocket";
@@ -134,29 +134,6 @@ import {
 /** localStorage key controlling whether the right panel is visible. */
 export const RIGHT_PANEL_OPEN_KEY = "agents.right-panel-open";
 
-/**
- * Below the `lg` breakpoint, chat and the right panel are mutually
- * exclusive, so a panel left open on a wide window would hide chat as
- * soon as the window narrows. This suppresses the panel while narrow
- * without touching the persisted preference: widening restores the
- * panel, and an explicit user action (clearSuppression) overrides it.
- */
-export function useRightPanelNarrowSuppression(): {
-	suppressed: boolean;
-	clearSuppression: () => void;
-} {
-	const isBelowLg = useIsBelowLgViewport();
-	const [suppressed, setSuppressed] = useState(isBelowLg);
-	const [prevIsBelowLg, setPrevIsBelowLg] = useState(isBelowLg);
-	// Render-time state adjustment on breakpoint crossings; see
-	// https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes
-	if (isBelowLg !== prevIsBelowLg) {
-		setPrevIsBelowLg(isBelowLg);
-		setSuppressed(isBelowLg);
-	}
-	return { suppressed, clearSuppression: () => setSuppressed(false) };
-}
-
 const lastModelConfigIDStorageKey = "agents.last-model-config-id";
 
 const AGENT_BINDING_REPAIR_POLL_MS = 30_000;
@@ -886,15 +863,28 @@ const AgentChatPage: FC = () => {
 	const [sidebarPanelPreference, setSidebarPanelPreference] = useState(() => {
 		return localStorage.getItem(RIGHT_PANEL_OPEN_KEY) === "true";
 	});
-	const { suppressed: panelSuppressedOnNarrow, clearSuppression } =
-		useRightPanelNarrowSuppression();
+	// Below the lg breakpoint, chat and the right panel are mutually
+	// exclusive, so a panel left open on a wide window would hide chat
+	// as soon as the window narrows. Suppression hides the panel while
+	// narrow without touching the persisted preference: widening
+	// restores the panel, and an explicit toggle overrides it.
+	const isBelowLg = useMediaQuery(belowLgViewportMediaQuery);
+	const [panelSuppressedOnNarrow, setPanelSuppressedOnNarrow] =
+		useState(isBelowLg);
+	const [prevIsBelowLg, setPrevIsBelowLg] = useState(isBelowLg);
+	// Render-time state adjustment on breakpoint crossings; see
+	// https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes
+	if (isBelowLg !== prevIsBelowLg) {
+		setPrevIsBelowLg(isBelowLg);
+		setPanelSuppressedOnNarrow(isBelowLg);
+	}
 	// Canonical panel visibility: the persisted preference gated by the
 	// narrow-viewport suppression. Only this derived value may be
 	// rendered or handed to children; the raw preference stays local.
 	const showSidebarPanel = sidebarPanelPreference && !panelSuppressedOnNarrow;
 
 	const handleSetShowSidebarPanel = (next: boolean) => {
-		clearSuppression();
+		setPanelSuppressedOnNarrow(false);
 		setSidebarPanelPreference(next);
 		localStorage.setItem(RIGHT_PANEL_OPEN_KEY, String(next));
 	};

site/src/pages/AgentsPage/components/WorkspacePill.tsx

@@ -35,7 +35,7 @@ import {
 } from "#/components/Tooltip/Tooltip";
 import { useProxy } from "#/contexts/ProxyContext";
 import { useClipboard } from "#/hooks/useClipboard";
-import { useIsBelowMdViewport } from "#/hooks/useIsBelowMdViewport";
+import { useMediaQuery } from "#/hooks/useMediaQuery";
 import {
 	getTerminalHref,
 	getVSCodeHref,
@@ -47,6 +47,7 @@ import {
 	usePortsData,
 } from "#/modules/resources/usePortsData";
 import { cn } from "#/utils/cn";
+import { belowMdViewportMediaQuery } from "#/utils/mobile";
 import { getWorkspaceStatus, StatusIcon } from "./StatusIcon";
 import { MobilePortsPanel, PortsMenuItem } from "./WorkspacePillPorts";
 
@@ -98,7 +99,7 @@ export const WorkspacePill: FC<WorkspacePillProps> = ({
 	// Flyout sub-menus clip on mobile.
 	const [view, setView] = useState<"main" | "ports">("main");
 	const [focusPortsOnMain, setFocusPortsOnMain] = useState(false);
-	const isBelowMd = useIsBelowMdViewport();
+	const isBelowMd = useMediaQuery(belowMdViewportMediaQuery);
 	const showPortsView = view === "ports" && isBelowMd;
 
 	const portsData = usePortsData(

site/src/testHelpers/matchMedia.ts

@@ -1,11 +1,16 @@
+import { spyOn } from "storybook/test";
+
 /**
- * Replaces `window.matchMedia` with a controllable stub for tests and
- * stories. Queries listed in `initialMatches` report their configured
- * value; every other query delegates to the real `matchMedia` (or
- * reports `false` where none exists, e.g. jsdom) so unrelated
+ * Replaces `window.matchMedia` with a controllable stub for stories.
+ * Queries listed in `initialMatches` report their configured value;
+ * every other query delegates to the real `matchMedia` so unrelated
  * responsive components keep behaving truthfully. `setMatches` updates
  * a query and notifies its registered change listeners; `restore` puts
  * the original `window.matchMedia` back.
+ *
+ * Story-only: stories run in a real browser, so a real `matchMedia` to
+ * delegate to always exists. jsdom has no `matchMedia`, so unit tests
+ * must install their own stub with `vi.stubGlobal` instead.
  */
 export const setupMatchMedia = (initialMatches: Record<string, boolean>) => {
 	const matches = { ...initialMatches };
@@ -18,15 +23,11 @@ export const setupMatchMedia = (initialMatches: Record<string, boolean>) => {
 		}
 		return set;
 	};
-	const original = window.matchMedia;
-	const originalFn =
-		typeof original === "function" ? original.bind(window) : undefined;
-	Object.defineProperty(window, "matchMedia", {
-		configurable: true,
-		writable: true,
-		value: (query: string): MediaQueryList => {
-			if (!(query in matches) && originalFn) {
-				return originalFn(query);
+	const original = window.matchMedia.bind(window);
+	const spy = spyOn(window, "matchMedia").mockImplementation(
+		(query: string): MediaQueryList => {
+			if (!(query in matches)) {
+				return original(query);
 			}
 			return {
 				get matches() {
@@ -51,7 +52,7 @@ export const setupMatchMedia = (initialMatches: Record<string, boolean>) => {
 				removeListener: () => {},
 			} satisfies MediaQueryList;
 		},
-	});
+	);
 	return {
 		setMatches: (query: string, value: boolean) => {
 			matches[query] = value;
@@ -64,12 +65,6 @@ export const setupMatchMedia = (initialMatches: Record<string, boolean>) => {
 				}
 			}
 		},
-		restore: () => {
-			Object.defineProperty(window, "matchMedia", {
-				configurable: true,
-				writable: true,
-				value: original,
-			});
-		},
+		restore: () => spy.mockRestore(),
 	};
 };

site/src/utils/mobile.ts

@@ -8,19 +8,6 @@ export const isMobileViewport = (): boolean => {
 	return window.matchMedia("(max-width: 639px)").matches;
 };
 
-/**
- * Builds a `useSyncExternalStore` subscribe function that notifies on
- * changes to the given media query, so every viewport hook shares one
- * listener lifecycle implementation.
- */
-export const createMediaQuerySubscribe =
-	(query: string) =>
-	(onStoreChange: () => void): (() => void) => {
-		const mediaQuery = window.matchMedia(query);
-		mediaQuery.addEventListener("change", onStoreChange);
-		return () => mediaQuery.removeEventListener("change", onStoreChange);
-	};
-
 export const belowMdViewportMediaQuery = "(max-width: 767px)";
 
 /**
@@ -35,15 +22,10 @@ export const isBelowMdViewport = (): boolean => {
 	return window.matchMedia(belowMdViewportMediaQuery).matches;
 };
 
-export const belowLgViewportMediaQuery = "(max-width: 1023px)";
-
 /**
- * Returns `true` when the viewport width is below the `lg` Tailwind
- * breakpoint (< 1024 px). Use this to align with `lg:` Tailwind
- * utilities that switch between a side-by-side layout and a
- * single-panel-at-a-time layout (e.g. the Agents chat page's chat vs.
- * right panel split).
+ * Matches viewports below the `lg` Tailwind breakpoint (< 1024 px),
+ * aligning with `lg:` utilities that switch between a side-by-side
+ * layout and a single-panel-at-a-time layout (e.g. the Agents chat
+ * page's chat vs. right panel split).
  */
-export const isBelowLgViewport = (): boolean => {
-	return window.matchMedia(belowLgViewportMediaQuery).matches;
-};
+export const belowLgViewportMediaQuery = "(max-width: 1023px)";