scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-07 08:27:12 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Fix list-view sort by making preferences the unit of mutation

Browse source 
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.
This commit is contained in:
Bryan Helmkamp 2026-05-24 13:22:45 -04:00
parent 596b105b06
commit aa65b79e57
No known key found for this signature in database
3 changed files with 91 additions and 47 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

22
apps/fabro-web/app/routes/runs.preferences.test.tsx
View file

@ -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");

23
apps/fabro-web/app/routes/runs.test.tsx
View file

@ -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,

93
apps/fabro-web/app/routes/runs.tsx
View file

@ -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<Storage, "setItem"> | 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<ToggleableColumn>) => updateParam("hide", serializeHiddenColumns(next)),
[updateParam],
(next: Set<ToggleableColumn>) =>
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]);