From aa65b79e57b537cc4cbb54bbfb947d1bec0ff115 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 24 May 2026 13:22:45 -0400 Subject: [PATCH] Fix list-view sort by making preferences the unit of mutation Clicking a column header on /runs?view=list silently did nothing. handleSortClick called updateParam three times in a row; each call cloned a fresh URLSearchParams from the same closure-captured searchParams and invoked setSearchParams independently. React Router's setSearchParams calls don't merge in a single tick, so only the last one's params landed in the URL -- the sort change was overwritten by the trailing page-reset. setPageSize had the same shape and was also quietly losing the size change. Replace the per-key URL mutator with a reducer-shaped updatePreferences((prev) => next) that operates on the typed RunsWorkspacePreferences model. URL and localStorage are derived from the same next object via the existing converters, and each handler makes exactly one call -- so concurrent-update races are structurally impossible. Use setSearchParams((prev) => ...) so the updater reads the latest committed URL params instead of a closure. Add page to RunsWorkspacePreferences so the model describes the full URL view state; strip it before persisting to localStorage since page is ephemeral. Rename persistRunsWorkspaceSearchParams to persistRunsWorkspacePreferences to match what it now consumes. Guard the hydration useEffect with a useRef so it runs only on mount. Add a regression test that clicks a SortHeader and asserts the URL gains sort=status while preserving view=list&archived=1, and that a second click toggles direction=asc. --- .../app/routes/runs.preferences.test.tsx | 22 +++++ apps/fabro-web/app/routes/runs.test.tsx | 23 ++++- apps/fabro-web/app/routes/runs.tsx | 93 ++++++++++--------- 3 files changed, 91 insertions(+), 47 deletions(-) diff --git a/apps/fabro-web/app/routes/runs.preferences.test.tsx b/apps/fabro-web/app/routes/runs.preferences.test.tsx index 003e44ae1..d3ad1cc1f 100644 --- a/apps/fabro-web/app/routes/runs.preferences.test.tsx +++ b/apps/fabro-web/app/routes/runs.preferences.test.tsx @@ -244,6 +244,28 @@ describe("Runs workspace preference restoration", () => { expect(JSON.parse(storage.getItem(RUNS_PREFERENCES_STORAGE_KEY) ?? "{}").view).toBe("columns"); }); + test("clicking a sort header in list view updates the URL while preserving other params", async () => { + const { renderer, router } = await renderRuns("/runs?view=list&archived=1"); + + await act(async () => { + compositeByName(renderer, "SortHeader", (props) => props.sortKey === "status").props.onClick("status"); + }); + + expect(router.state.location.search).toContain("sort=status"); + expect(router.state.location.search).toContain("view=list"); + expect(router.state.location.search).toContain("archived=1"); + + // Clicking the same header again toggles direction to ascending. + await act(async () => { + compositeByName(renderer, "SortHeader", (props) => props.sortKey === "status").props.onClick("status"); + }); + + expect(router.state.location.search).toContain("sort=status"); + expect(router.state.location.search).toContain("direction=asc"); + expect(router.state.location.search).toContain("view=list"); + expect(router.state.location.search).toContain("archived=1"); + }); + test("changing filters and hidden columns persists them", async () => { const { renderer } = await renderRuns("/runs?view=list"); diff --git a/apps/fabro-web/app/routes/runs.test.tsx b/apps/fabro-web/app/routes/runs.test.tsx index 85e9a4dd0..0cea3dca4 100644 --- a/apps/fabro-web/app/routes/runs.test.tsx +++ b/apps/fabro-web/app/routes/runs.test.tsx @@ -5,7 +5,7 @@ import { buildBoardColumns, loadStoredRunsWorkspaceSearchParams, placeArchivedColumnLast, - persistRunsWorkspaceSearchParams, + persistRunsWorkspacePreferences, RUNS_PREFERENCES_STORAGE_KEY, runsQuickStartCommands, shouldRefreshBoardForEvent, @@ -279,11 +279,24 @@ describe("runs route workspace preferences", () => { test("persisting preferences omits page and stores canonical values", () => { const storage = new MemoryStorage(); - const params = new URLSearchParams( - "view=columns&search=abc&created=1d&sort=made-up&direction=asc&size=100&page=9&hide=unknown,workflow,repo", - ); - persistRunsWorkspaceSearchParams(params, storage); + persistRunsWorkspacePreferences( + { + version: 1, + view: "columns", + search: "abc", + repo: "all", + workflow: "all", + created: "1d", + archived: false, + sort: "created_at", + direction: "asc", + size: 100, + hide: "repo,workflow", + page: 9, + }, + storage, + ); expect(JSON.parse(storage.getItem(RUNS_PREFERENCES_STORAGE_KEY) ?? "{}")).toEqual({ version: 1, diff --git a/apps/fabro-web/app/routes/runs.tsx b/apps/fabro-web/app/routes/runs.tsx index 7d5878b86..9f6a2aec3 100644 --- a/apps/fabro-web/app/routes/runs.tsx +++ b/apps/fabro-web/app/routes/runs.tsx @@ -693,6 +693,8 @@ interface RunsWorkspacePreferences { direction: ListRunsDirectionEnum; size: number; hide: string; + // URL-only: never persisted to localStorage. + page: number; } function defaultRunsWorkspacePreferences(): RunsWorkspacePreferences { @@ -708,6 +710,7 @@ function defaultRunsWorkspacePreferences(): RunsWorkspacePreferences { direction: "desc", size: DEFAULT_LIST_PAGE_SIZE, hide: "", + page: 1, }; } @@ -754,6 +757,7 @@ function normalizeStoredRunsWorkspacePreferences(value: unknown): RunsWorkspaceP direction: parseDirection(stringValue(record.direction)), size: parsePageSize(typeof size === "number" || typeof size === "string" ? String(size) : null), hide: serializeHiddenColumns(hiddenColumns) ?? "", + page: 1, }; } @@ -770,6 +774,7 @@ function runsWorkspacePreferencesFromSearchParams(searchParams: URLSearchParams) direction: parseDirection(searchParams.get("direction")), size: parsePageSize(searchParams.get("size")), hide: serializeHiddenColumns(parseHiddenColumns(searchParams.get("hide"))) ?? "", + page: parsePage(searchParams.get("page")), }; } @@ -785,6 +790,7 @@ function runsWorkspacePreferencesToSearchParams(preferences: RunsWorkspacePrefer if (preferences.direction === "asc") params.set("direction", "asc"); if (preferences.size !== DEFAULT_LIST_PAGE_SIZE) params.set("size", String(preferences.size)); if (preferences.hide !== "") params.set("hide", preferences.hide); + if (preferences.page > 1) params.set("page", String(preferences.page)); return params; } @@ -821,16 +827,15 @@ export function resolveRunsWorkspaceSearchParams( return stored.toString() === "" ? urlSearchParams : stored; } -export function persistRunsWorkspaceSearchParams( - searchParams: URLSearchParams, +export function persistRunsWorkspacePreferences( + preferences: RunsWorkspacePreferences, storage: Pick | null = runsPreferencesStorage(), ) { if (storage == null) return; + // `page` is URL-only ephemeral view state; strip it before persisting. + const { page: _page, ...storable } = preferences; try { - storage.setItem( - RUNS_PREFERENCES_STORAGE_KEY, - JSON.stringify(runsWorkspacePreferencesFromSearchParams(searchParams)), - ); + storage.setItem(RUNS_PREFERENCES_STORAGE_KEY, JSON.stringify(storable)); } catch { // localStorage persistence is best effort only. } @@ -1821,55 +1826,59 @@ export default function Runs() { [searchParams], ); - const updateParam = useCallback( - (key: string, value: string | null) => { - const next = new URLSearchParams(searchParams); - if (value == null || value === "") { - next.delete(key); - } else { - next.set(key, value); - } - persistRunsWorkspaceSearchParams(next); - setSearchParams(next, { replace: true }); + const updatePreferences = useCallback( + (updater: (prev: RunsWorkspacePreferences) => RunsWorkspacePreferences) => { + setSearchParams( + (prevParams) => { + const next = updater(runsWorkspacePreferencesFromSearchParams(prevParams)); + persistRunsWorkspacePreferences(next); + return runsWorkspacePreferencesToSearchParams(next); + }, + { replace: true }, + ); }, - [searchParams, setSearchParams], + [setSearchParams], ); - const setQuery = (value: string) => updateParam("search", value || null); - const setRepoFilter = (value: string) => updateParam("repo", value === "all" ? null : value); - const setWorkflowFilter = (value: string) => updateParam("workflow", value === "all" ? null : value); - const setCreatedFilter = (value: CreatedFilter) => updateParam("created", value === "all" ? null : value); - const setIncludeArchived = (value: boolean) => updateParam("archived", value ? "1" : null); - const setView = (value: ViewMode) => updateParam("view", value === "columns" ? null : value); + const setQuery = (value: string) => + updatePreferences((prev) => ({ ...prev, search: value })); + const setRepoFilter = (value: string) => + updatePreferences((prev) => ({ ...prev, repo: value })); + const setWorkflowFilter = (value: string) => + updatePreferences((prev) => ({ ...prev, workflow: value })); + const setCreatedFilter = (value: CreatedFilter) => + updatePreferences((prev) => ({ ...prev, created: value })); + const setIncludeArchived = (value: boolean) => + updatePreferences((prev) => ({ ...prev, archived: value })); + const setView = (value: ViewMode) => + updatePreferences((prev) => ({ ...prev, view: value })); const setPage = useCallback( - (next: number) => updateParam("page", next > 1 ? String(next) : null), - [updateParam], + (next: number) => updatePreferences((prev) => ({ ...prev, page: next })), + [updatePreferences], ); const setPageSize = useCallback( - (next: number) => { - updateParam("size", next === DEFAULT_LIST_PAGE_SIZE ? null : String(next)); - updateParam("page", null); - }, - [updateParam], + (next: number) => updatePreferences((prev) => ({ ...prev, size: next, page: 1 })), + [updatePreferences], ); const setHiddenColumns = useCallback( - (next: Set) => updateParam("hide", serializeHiddenColumns(next)), - [updateParam], + (next: Set) => + updatePreferences((prev) => ({ ...prev, hide: serializeHiddenColumns(next) ?? "" })), + [updatePreferences], ); const handleSortClick = useCallback( - (key: ListRunsSortEnum) => { - if (sort === key) { - updateParam("direction", direction === "asc" ? null : "asc"); - } else { - updateParam("sort", key === "created_at" ? null : key); - updateParam("direction", null); - } - updateParam("page", null); - }, - [sort, direction, updateParam], + (key: ListRunsSortEnum) => + updatePreferences((prev) => + prev.sort === key + ? { ...prev, direction: prev.direction === "asc" ? "desc" : "asc", page: 1 } + : { ...prev, sort: key, direction: "desc", page: 1 }, + ), + [updatePreferences], ); + const hydratedFromStorage = useRef(false); useEffect(() => { + if (hydratedFromStorage.current) return; + hydratedFromStorage.current = true; if (searchParams === urlSearchParams) return; setSearchParams(searchParams, { replace: true }); }, [searchParams, urlSearchParams, setSearchParams]);