From bf152350b575e538c2f20905209ffcbbfca2a59a Mon Sep 17 00:00:00 2001 From: William Thorsen Date: Mon, 2 Mar 2026 21:34:37 -0800 Subject: [PATCH 1/4] factory|feat: Persist visualization mode in query string parameter Add useVisualizationParam hook that reads, validates, and syncs a `visualization` query parameter, following the useSelectionParams pattern. The hook defaults to 'factory', accepts 'flow', strips invalid values on mount, and keeps the URL clean by omitting the param for the default value. VisualizationSwitcher now uses this hook instead of bare useState. --- .../components/VisualizationSwitcher.tsx | 7 +- .../__tests__/VisualizationSwitcher.test.tsx | 26 ++++++ .../__tests__/useVisualizationParam.test.ts | 93 +++++++++++++++++++ .../src/client/hooks/useVisualizationParam.ts | 51 ++++++++++ 4 files changed, 173 insertions(+), 4 deletions(-) create mode 100644 packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts create mode 100644 packages/factory/src/client/hooks/useVisualizationParam.ts diff --git a/packages/factory/src/client/components/VisualizationSwitcher.tsx b/packages/factory/src/client/components/VisualizationSwitcher.tsx index b8697155..b3702cfd 100644 --- a/packages/factory/src/client/components/VisualizationSwitcher.tsx +++ b/packages/factory/src/client/components/VisualizationSwitcher.tsx @@ -1,17 +1,16 @@ -import React, { useState } from 'react'; +import React from 'react'; import type { CanonicalRunStatus } from '../../shared/types/canonical.js'; +import { useVisualizationParam } from '../hooks/useVisualizationParam.js'; import { FlowDiagram } from './FlowDiagram/FlowDiagram.js'; import { GameCanvas } from './GameCanvas.js'; -type ActiveView = 'factory' | 'flow'; - interface VisualizationSwitcherProps { status: CanonicalRunStatus; } export function VisualizationSwitcher({ status }: VisualizationSwitcherProps): React.JSX.Element { - const [activeView, setActiveView] = useState('factory'); + const [activeView, setActiveView] = useVisualizationParam(); return ( <> diff --git a/packages/factory/src/client/components/__tests__/VisualizationSwitcher.test.tsx b/packages/factory/src/client/components/__tests__/VisualizationSwitcher.test.tsx index bfbf4e70..799a01e7 100644 --- a/packages/factory/src/client/components/__tests__/VisualizationSwitcher.test.tsx +++ b/packages/factory/src/client/components/__tests__/VisualizationSwitcher.test.tsx @@ -18,8 +18,12 @@ vi.mock('../FlowDiagram/FlowDiagram.js', () => ({ const { VisualizationSwitcher } = await import('../VisualizationSwitcher.js'); describe('VisualizationSwitcher', () => { + const replaceStateSpy = vi.spyOn(globalThis.history, 'replaceState'); + afterEach(() => { cleanup(); + globalThis.history.replaceState(null, '', '/'); + replaceStateSpy.mockClear(); }); it('renders factory view by default with data-view="factory"', () => { @@ -76,4 +80,26 @@ describe('VisualizationSwitcher', () => { expect(flowButton.className).toContain('active'); expect(factoryButton.className).not.toContain('active'); }); + + it('renders in flow view when URL has visualization=flow', () => { + globalThis.history.replaceState(null, '', '/?visualization=flow'); + + const status = createMockRunStatus(); + const { container } = render(); + + const canvasContainer = container.querySelector('.canvas-container'); + expect(canvasContainer?.dataset.view).toBe('flow'); + expect(screen.getByTestId('flow-diagram')).toBeInTheDocument(); + expect(screen.queryByTestId('game-canvas')).toBeNull(); + }); + + it('updates URL param when the Flow button is clicked', () => { + const status = createMockRunStatus(); + render(); + replaceStateSpy.mockClear(); + + fireEvent.click(screen.getByRole('button', { name: 'Flow' })); + + expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/?visualization=flow'); + }); }); diff --git a/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts b/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts new file mode 100644 index 00000000..822053a7 --- /dev/null +++ b/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts @@ -0,0 +1,93 @@ +import { renderHook } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useVisualizationParam } from '../useVisualizationParam.js'; + +describe('useVisualizationParam', () => { + const replaceStateSpy = vi.spyOn(globalThis.history, 'replaceState'); + + beforeEach(() => { + globalThis.history.replaceState(null, '', '/'); + replaceStateSpy.mockClear(); + }); + + afterEach(() => { + globalThis.history.replaceState(null, '', '/'); + }); + + it('returns "factory" when URL has no visualization param', () => { + const { result } = renderHook(() => useVisualizationParam()); + + expect(result.current[0]).toBe('factory'); + }); + + it('returns "factory" when URL has visualization=factory', () => { + globalThis.history.replaceState(null, '', '/?visualization=factory'); + + const { result } = renderHook(() => useVisualizationParam()); + + expect(result.current[0]).toBe('factory'); + }); + + it('returns "flow" when URL has visualization=flow', () => { + globalThis.history.replaceState(null, '', '/?visualization=flow'); + + const { result } = renderHook(() => useVisualizationParam()); + + expect(result.current[0]).toBe('flow'); + }); + + it('strips invalid value from URL on mount and returns "factory"', () => { + globalThis.history.replaceState(null, '', '/?visualization=unknown'); + replaceStateSpy.mockClear(); + + const { result } = renderHook(() => useVisualizationParam()); + + expect(result.current[0]).toBe('factory'); + expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/'); + }); + + it('setActiveView("flow") updates URL to include visualization=flow', () => { + const { result } = renderHook(() => useVisualizationParam()); + + result.current[1]('flow'); + + expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/?visualization=flow'); + }); + + it('setActiveView("factory") removes the visualization param from URL', () => { + globalThis.history.replaceState(null, '', '/?visualization=flow'); + replaceStateSpy.mockClear(); + + const { result } = renderHook(() => useVisualizationParam()); + + result.current[1]('factory'); + + expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/'); + }); + + it('preserves other query params when updating the visualization param', () => { + globalThis.history.replaceState(null, '', '/?project=alpha&ticket=T-1&run=run-a'); + replaceStateSpy.mockClear(); + + const { result } = renderHook(() => useVisualizationParam()); + + result.current[1]('flow'); + + expect(replaceStateSpy).toHaveBeenCalledWith( + null, + '', + '/?project=alpha&ticket=T-1&run=run-a&visualization=flow', + ); + }); + + it('setActiveView is stable across re-renders', () => { + const { result, rerender } = renderHook(() => useVisualizationParam()); + + const firstSetActiveView = result.current[1]; + rerender(); + const secondSetActiveView = result.current[1]; + + expect(firstSetActiveView).toBe(secondSetActiveView); + }); +}); diff --git a/packages/factory/src/client/hooks/useVisualizationParam.ts b/packages/factory/src/client/hooks/useVisualizationParam.ts new file mode 100644 index 00000000..57743a83 --- /dev/null +++ b/packages/factory/src/client/hooks/useVisualizationParam.ts @@ -0,0 +1,51 @@ +import { useCallback, useState } from 'react'; + +export type ActiveView = 'factory' | 'flow'; + +const PARAM_KEY = 'visualization'; +const VALID_VALUES = new Set(['factory', 'flow']); +const DEFAULT_VALUE: ActiveView = 'factory'; + +function isValidActiveView(value: string | null): value is ActiveView { + return value !== null && VALID_VALUES.has(value); +} + +export function useVisualizationParam(): [ActiveView, (view: ActiveView) => void] { + const [activeView, setActiveViewState] = useState(() => { + const params = new URLSearchParams(globalThis.location.search); + const raw = params.get(PARAM_KEY); + + if (isValidActiveView(raw)) { + return raw; + } + + // Strip invalid param from URL on mount. + // Calling replaceState inside the lazy initializer is intentional: it ensures + // the URL is cleaned before the first render, avoiding a flash of an invalid param. + if (raw !== null) { + params.delete(PARAM_KEY); + const search = params.toString(); + const url = search ? `${globalThis.location.pathname}?${search}` : globalThis.location.pathname; + globalThis.history.replaceState(null, '', url); + } + + return DEFAULT_VALUE; + }); + + const setActiveView = useCallback((view: ActiveView) => { + const params = new URLSearchParams(globalThis.location.search); + + if (view === DEFAULT_VALUE) { + params.delete(PARAM_KEY); + } else { + params.set(PARAM_KEY, view); + } + + const search = params.toString(); + const url = search ? `${globalThis.location.pathname}?${search}` : globalThis.location.pathname; + globalThis.history.replaceState(null, '', url); + setActiveViewState(view); + }, []); + + return [activeView, setActiveView]; +} From dd754d9267c639acc6480959a554a3929cae1b15 Mon Sep 17 00:00:00 2001 From: William Thorsen Date: Mon, 2 Mar 2026 21:39:41 -0800 Subject: [PATCH 2/4] factory|fix: Guard replaceState calls and add missing test coverage Wrap both replaceState calls in useVisualizationParam with try/catch to prevent crashes in sandboxed iframes or restricted WebView contexts. Add tests for: explicit factory param not triggering replaceState on mount, stripping invalid params while preserving other query params, and removing the URL param when switching back to factory view. --- .../__tests__/VisualizationSwitcher.test.tsx | 11 +++++++++++ .../hooks/__tests__/useVisualizationParam.test.ts | 14 +++++++++++++- .../src/client/hooks/useVisualizationParam.ts | 15 +++++++++++++-- 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/packages/factory/src/client/components/__tests__/VisualizationSwitcher.test.tsx b/packages/factory/src/client/components/__tests__/VisualizationSwitcher.test.tsx index 799a01e7..155b3f51 100644 --- a/packages/factory/src/client/components/__tests__/VisualizationSwitcher.test.tsx +++ b/packages/factory/src/client/components/__tests__/VisualizationSwitcher.test.tsx @@ -102,4 +102,15 @@ describe('VisualizationSwitcher', () => { expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/?visualization=flow'); }); + + it('removes URL param when the Factory button is clicked from flow view', () => { + globalThis.history.replaceState(null, '', '/?visualization=flow'); + const status = createMockRunStatus(); + render(); + replaceStateSpy.mockClear(); + + fireEvent.click(screen.getByRole('button', { name: 'Factory' })); + + expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/'); + }); }); diff --git a/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts b/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts index 822053a7..1e757d19 100644 --- a/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts +++ b/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts @@ -21,12 +21,14 @@ describe('useVisualizationParam', () => { expect(result.current[0]).toBe('factory'); }); - it('returns "factory" when URL has visualization=factory', () => { + it('returns "factory" when URL has visualization=factory and does not call replaceState', () => { globalThis.history.replaceState(null, '', '/?visualization=factory'); + replaceStateSpy.mockClear(); const { result } = renderHook(() => useVisualizationParam()); expect(result.current[0]).toBe('factory'); + expect(replaceStateSpy).not.toHaveBeenCalled(); }); it('returns "flow" when URL has visualization=flow', () => { @@ -47,6 +49,16 @@ describe('useVisualizationParam', () => { expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/'); }); + it('strips invalid value from URL on mount while preserving other params', () => { + globalThis.history.replaceState(null, '', '/?project=alpha&ticket=T-1&visualization=unknown'); + replaceStateSpy.mockClear(); + + const { result } = renderHook(() => useVisualizationParam()); + + expect(result.current[0]).toBe('factory'); + expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/?project=alpha&ticket=T-1'); + }); + it('setActiveView("flow") updates URL to include visualization=flow', () => { const { result } = renderHook(() => useVisualizationParam()); diff --git a/packages/factory/src/client/hooks/useVisualizationParam.ts b/packages/factory/src/client/hooks/useVisualizationParam.ts index 57743a83..19ffc7d4 100644 --- a/packages/factory/src/client/hooks/useVisualizationParam.ts +++ b/packages/factory/src/client/hooks/useVisualizationParam.ts @@ -26,7 +26,12 @@ export function useVisualizationParam(): [ActiveView, (view: ActiveView) => void params.delete(PARAM_KEY); const search = params.toString(); const url = search ? `${globalThis.location.pathname}?${search}` : globalThis.location.pathname; - globalThis.history.replaceState(null, '', url); + try { + globalThis.history.replaceState(null, '', url); + } catch { + // Ignore — URL cleanup is best-effort; restricted environments (sandboxed iframes) + // may disallow history manipulation. + } } return DEFAULT_VALUE; @@ -43,7 +48,13 @@ export function useVisualizationParam(): [ActiveView, (view: ActiveView) => void const search = params.toString(); const url = search ? `${globalThis.location.pathname}?${search}` : globalThis.location.pathname; - globalThis.history.replaceState(null, '', url); + + try { + globalThis.history.replaceState(null, '', url); + } catch { + // Ignore — URL persistence is best-effort in restricted environments. + } + setActiveViewState(view); }, []); From 7143f06cc1173c2d113c3b7626edde3dbbd62f3c Mon Sep 17 00:00:00 2001 From: William Thorsen Date: Mon, 2 Mar 2026 21:44:11 -0800 Subject: [PATCH 3/4] factory|tests: Test removal of visualization param with coexisting params Add hook-level test that verifies setActiveView('factory') correctly removes only the visualization query param while preserving project, ticket, and run params. This completes coverage for the coexistence acceptance criterion in both the add and remove directions. --- .../__tests__/useVisualizationParam.test.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts b/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts index 1e757d19..0ff0c492 100644 --- a/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts +++ b/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts @@ -78,6 +78,25 @@ describe('useVisualizationParam', () => { expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/'); }); + it('setActiveView("factory") removes the visualization param while preserving other params', () => { + globalThis.history.replaceState( + null, + '', + '/?project=alpha&ticket=T-1&run=run-a&visualization=flow', + ); + replaceStateSpy.mockClear(); + + const { result } = renderHook(() => useVisualizationParam()); + + result.current[1]('factory'); + + expect(replaceStateSpy).toHaveBeenCalledWith( + null, + '', + '/?project=alpha&ticket=T-1&run=run-a', + ); + }); + it('preserves other query params when updating the visualization param', () => { globalThis.history.replaceState(null, '', '/?project=alpha&ticket=T-1&run=run-a'); replaceStateSpy.mockClear(); From 5ca4468226692e5ca3dc4cb2558ec1a037ddde31 Mon Sep 17 00:00:00 2001 From: William Thorsen Date: Mon, 2 Mar 2026 21:48:39 -0800 Subject: [PATCH 4/4] factory|refactor: Simplify useVisualizationParam and align test hygiene Extract safeReplaceUrl helper to deduplicate URL-building and try/catch logic. Inline the isValidActiveView type guard and VALID_VALUES set since TypeScript narrows the union check naturally. Move replaceStateSpy reset from afterEach to beforeEach in VisualizationSwitcher tests for consistent clean-state initialization. --- .../__tests__/VisualizationSwitcher.test.tsx | 9 +++-- .../__tests__/useVisualizationParam.test.ts | 18 ++-------- .../src/client/hooks/useVisualizationParam.ts | 34 +++++++------------ 3 files changed, 22 insertions(+), 39 deletions(-) diff --git a/packages/factory/src/client/components/__tests__/VisualizationSwitcher.test.tsx b/packages/factory/src/client/components/__tests__/VisualizationSwitcher.test.tsx index 155b3f51..7d5ec621 100644 --- a/packages/factory/src/client/components/__tests__/VisualizationSwitcher.test.tsx +++ b/packages/factory/src/client/components/__tests__/VisualizationSwitcher.test.tsx @@ -1,5 +1,5 @@ import { cleanup, fireEvent, render, screen } from '@testing-library/react'; -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { createMockRunStatus } from '../../../__test-helpers__/fixtures.js'; @@ -20,12 +20,15 @@ const { VisualizationSwitcher } = await import('../VisualizationSwitcher.js'); describe('VisualizationSwitcher', () => { const replaceStateSpy = vi.spyOn(globalThis.history, 'replaceState'); - afterEach(() => { - cleanup(); + beforeEach(() => { globalThis.history.replaceState(null, '', '/'); replaceStateSpy.mockClear(); }); + afterEach(() => { + cleanup(); + }); + it('renders factory view by default with data-view="factory"', () => { const status = createMockRunStatus(); const { container } = render(); diff --git a/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts b/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts index 0ff0c492..dbdca7c7 100644 --- a/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts +++ b/packages/factory/src/client/hooks/__tests__/useVisualizationParam.test.ts @@ -79,22 +79,14 @@ describe('useVisualizationParam', () => { }); it('setActiveView("factory") removes the visualization param while preserving other params', () => { - globalThis.history.replaceState( - null, - '', - '/?project=alpha&ticket=T-1&run=run-a&visualization=flow', - ); + globalThis.history.replaceState(null, '', '/?project=alpha&ticket=T-1&run=run-a&visualization=flow'); replaceStateSpy.mockClear(); const { result } = renderHook(() => useVisualizationParam()); result.current[1]('factory'); - expect(replaceStateSpy).toHaveBeenCalledWith( - null, - '', - '/?project=alpha&ticket=T-1&run=run-a', - ); + expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/?project=alpha&ticket=T-1&run=run-a'); }); it('preserves other query params when updating the visualization param', () => { @@ -105,11 +97,7 @@ describe('useVisualizationParam', () => { result.current[1]('flow'); - expect(replaceStateSpy).toHaveBeenCalledWith( - null, - '', - '/?project=alpha&ticket=T-1&run=run-a&visualization=flow', - ); + expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '/?project=alpha&ticket=T-1&run=run-a&visualization=flow'); }); it('setActiveView is stable across re-renders', () => { diff --git a/packages/factory/src/client/hooks/useVisualizationParam.ts b/packages/factory/src/client/hooks/useVisualizationParam.ts index 19ffc7d4..6c2a87a1 100644 --- a/packages/factory/src/client/hooks/useVisualizationParam.ts +++ b/packages/factory/src/client/hooks/useVisualizationParam.ts @@ -3,11 +3,18 @@ import { useCallback, useState } from 'react'; export type ActiveView = 'factory' | 'flow'; const PARAM_KEY = 'visualization'; -const VALID_VALUES = new Set(['factory', 'flow']); const DEFAULT_VALUE: ActiveView = 'factory'; -function isValidActiveView(value: string | null): value is ActiveView { - return value !== null && VALID_VALUES.has(value); +// Replace the current history entry, ignoring failures in restricted environments +// such as sandboxed iframes. +function safeReplaceUrl(params: URLSearchParams): void { + const search = params.toString(); + const url = search ? `${globalThis.location.pathname}?${search}` : globalThis.location.pathname; + try { + globalThis.history.replaceState(null, '', url); + } catch { + // Ignore. + } } export function useVisualizationParam(): [ActiveView, (view: ActiveView) => void] { @@ -15,7 +22,7 @@ export function useVisualizationParam(): [ActiveView, (view: ActiveView) => void const params = new URLSearchParams(globalThis.location.search); const raw = params.get(PARAM_KEY); - if (isValidActiveView(raw)) { + if (raw === 'factory' || raw === 'flow') { return raw; } @@ -24,14 +31,7 @@ export function useVisualizationParam(): [ActiveView, (view: ActiveView) => void // the URL is cleaned before the first render, avoiding a flash of an invalid param. if (raw !== null) { params.delete(PARAM_KEY); - const search = params.toString(); - const url = search ? `${globalThis.location.pathname}?${search}` : globalThis.location.pathname; - try { - globalThis.history.replaceState(null, '', url); - } catch { - // Ignore — URL cleanup is best-effort; restricted environments (sandboxed iframes) - // may disallow history manipulation. - } + safeReplaceUrl(params); } return DEFAULT_VALUE; @@ -46,15 +46,7 @@ export function useVisualizationParam(): [ActiveView, (view: ActiveView) => void params.set(PARAM_KEY, view); } - const search = params.toString(); - const url = search ? `${globalThis.location.pathname}?${search}` : globalThis.location.pathname; - - try { - globalThis.history.replaceState(null, '', url); - } catch { - // Ignore — URL persistence is best-effort in restricted environments. - } - + safeReplaceUrl(params); setActiveViewState(view); }, []);