diff --git a/packages/web/src/app/(dashboard)/lore/LorePage.test.stories.tsx b/packages/web/src/app/(dashboard)/lore/LorePage.test.stories.tsx index e1a3647d9..52b5bb0e4 100644 --- a/packages/web/src/app/(dashboard)/lore/LorePage.test.stories.tsx +++ b/packages/web/src/app/(dashboard)/lore/LorePage.test.stories.tsx @@ -1,5 +1,5 @@ import type { Meta, StoryObj } from '@storybook/react'; -import { expect, within } from 'storybook/test'; +import { expect, userEvent, within } from 'storybook/test'; import LorePageClient from './LorePageClient'; import { memoryHandlers, FROZEN_NOW } from '@/mocks/memories'; @@ -65,3 +65,29 @@ export const RendersScopeSelector: Story = { }, }; +/** The page's own `?sort=` value, as a reload would read it. */ +// The browser's location, not useSearchParams(): Storybook's App Router mock +// never feeds `window.history` back into it. +const sortInAddressBar = () => new URLSearchParams(window.location.search).get('sort'); + +/** A sort choice is in the address bar the moment it is made, so a reload right after it keeps it. */ +export const SortChoiceIsInTheAddressBarAtOnce: Story = { + play: async ({ canvasElement, step }) => { + const canvas = within(canvasElement); + // Exact name: the phone-width control is labelled "Sort memories: ". + const sort = await canvas.findByRole('combobox', { name: 'Sort memories' }); + + await step('Choosing Recent read writes sort to the URL in the same step', async () => { + await userEvent.selectOptions(sort, 'Recent read'); + await expect(sortInAddressBar()).toBe('"last_opened_at"'); + await expect(sort).toHaveValue('last_opened_at'); + }); + + await step('Choosing Newest, the default, removes sort again', async () => { + await userEvent.selectOptions(sort, 'Newest'); + await expect(sortInAddressBar()).toBeNull(); + await expect(sort).toHaveValue('created_at'); + }); + }, +}; + diff --git a/packages/web/src/components/providers/MemorySidebarProvider.tsx b/packages/web/src/components/providers/MemorySidebarProvider.tsx index b24433c34..043c31c35 100644 --- a/packages/web/src/components/providers/MemorySidebarProvider.tsx +++ b/packages/web/src/components/providers/MemorySidebarProvider.tsx @@ -203,8 +203,8 @@ export function MemorySidebarProvider({ children }: MemorySidebarProviderProps) // than reimplemented for the alias. const urlMemoryId = searchParams.get('memoryId') ?? lessonParamMemoryId; // Closing the sheet dismisses the id locally rather than rewriting the URL — - // the pure `activeMemoryId` docblock has the why (a second router.replace in - // the same tick clobbers the Explorer's scope/filter write). + // the pure `activeMemoryId` docblock has the why (when URL writes went through + // the router, a second one in the same tick clobbered the Explorer's write). const [dismissedMemoryId, setDismissedMemoryId] = useState(null); const memoryId = activeMemoryId(urlMemoryId, dismissedMemoryId); const { data: memoryByIdLesson } = useMemoryById(memoryId); @@ -258,12 +258,12 @@ export function MemorySidebarProvider({ children }: MemorySidebarProviderProps) [setLessonRef], ); - // Only write the URL when a lesson is actually open. Calling this - // unconditionally (e.g. from LoreExplorer's scope-select handler, which - // closes any open lesson while also setting ?scope) would fire a second - // router.replace built from the *pre-scope* search params, clobbering the - // scope param that was set in the same tick. Guarding on lessonRef makes the - // close a no-op navigation when nothing is open. + // Only write the URL when a lesson is actually open. LoreExplorer's + // scope-select handler closes any open lesson while also setting ?scope; when + // URL writes went through the router, that second write was built from the + // *pre-scope* params and clobbered the scope. Writes now start from the live + // URL, and guarding on lessonRef still keeps the close a no-op when nothing + // is open. const closeLesson = useCallback((rawReason?: MemoryCloseReason) => { // Only a close that closed SOMETHING is an event. `LoreExplorer` calls this // on every filter/scope/status change whether or not a memory is open, so diff --git a/packages/web/src/lib/hooks/url-state-optimistic.ts b/packages/web/src/lib/hooks/url-state-optimistic.ts index 648e7c918..9d3159a60 100644 --- a/packages/web/src/lib/hooks/url-state-optimistic.ts +++ b/packages/web/src/lib/hooks/url-state-optimistic.ts @@ -1,13 +1,13 @@ /** * The optimistic layer behind {@link useUrlState}, shared by URL param KEY. * - * A URL write through the router lands a navigation later: the new search - * params reach `useSearchParams()` only once the router has finished its cycle, - * which for a dynamic page includes a server round trip. `useUrlState` hides - * that by showing the written value immediately. That layer used to be local - * `useState` per hook INSTANCE, so it only hid the wait from the component that - * wrote — every other component reading the same param kept the old value - * until the navigation landed. Two components reading `?filters=` (the + * A URL write reaches `useSearchParams()` a render after it is made: Next.js + * applies a `window.history` write inside a transition (and a router write, + * before `useUrlState` used `window.history`, only after a server round trip). + * `useUrlState` hides that by showing the written value immediately. That layer + * used to be local `useState` per hook INSTANCE, so it only hid the wait from + * the component that wrote — every other component reading the same param kept + * the old value until the write landed. Two components reading `?filters=` (the * Explorer and the detail panel applying a filter into it) disagreed for that * whole window, and the Explorer looked slow to react to the panel. * @@ -17,7 +17,7 @@ * node vitest project. * * An entry is dropped — the URL becomes the source of truth again — when: - * - the URL now holds the written value (the navigation landed); + * - the URL now holds the written value (the write landed); * - the pathname differs from the one it was written on (the reader left the * page the write was aimed at, so it can never land there); * - the last instance reading the key unmounts (nothing is left to show it, diff --git a/packages/web/src/lib/hooks/useDebouncedUrlState.ts b/packages/web/src/lib/hooks/useDebouncedUrlState.ts index ec49961cb..7a34c4048 100644 --- a/packages/web/src/lib/hooks/useDebouncedUrlState.ts +++ b/packages/web/src/lib/hooks/useDebouncedUrlState.ts @@ -7,13 +7,13 @@ * debounced, while the value it returns stays instantly responsive. * * ## Why this exists - * `useUrlState` writes the URL synchronously on every `setState` call. For a + * `useUrlState` writes the URL on every `setState` call (a microtask later). For a * value that changes rapidly — a text search box, a range slider — that means a - * `router.replace` per keystroke/drag. Each navigation re-renders the route - * subtree (every component subscribed to `useSearchParams`), which makes the - * control feel laggy and floods the URL/history with intermediate values. + * URL write per keystroke/drag. Each write re-renders every component + * subscribed to `useSearchParams`, which makes the control feel laggy and + * floods the URL with intermediate values (and the history, in `push` mode). * Wrapping the write in `startTransition` only lowers its priority; it does not - * stop the per-keystroke navigation. + * stop the per-keystroke write. * * This hook decouples the two concerns: * 1. **Responsiveness** — the returned value is backed by local `useState`, so diff --git a/packages/web/src/lib/hooks/useUrlState.spec.ts b/packages/web/src/lib/hooks/useUrlState.spec.ts index d5f1ac9f5..be149ec26 100644 --- a/packages/web/src/lib/hooks/useUrlState.spec.ts +++ b/packages/web/src/lib/hooks/useUrlState.spec.ts @@ -12,6 +12,9 @@ * - toAbsolute path normalisation * - buildUrl: param write, delete on default, preserve unrelated params * - isPathnameAllowed: exact match, prefix match, non-match, array overload + * - writeParamToHistory: the setter's URL write — replaceState vs pushState, + * built from the live location, synchronous (so a reload right after a + * choice keeps it), against a fake `window` * * The optimistic-update contract is documented in the comments below as a * spec for future integration tests, and the SSR contract is documented via @@ -25,6 +28,8 @@ import { toAbsolute, buildUrl, isPathnameAllowed, + writeParamToHistory, + type HistoryWindow, } from './useUrlState'; // ── serialise ───────────────────────────────────────────────────────────────── @@ -202,6 +207,151 @@ describe('isPathnameAllowed', () => { }); }); +// ── writeParamToHistory ─────────────────────────────────────────────────────── + +const ORIGIN = 'https://lorekit.test'; + +/** + * A fake `window` whose history calls move its location the way a browser's + * do, and record every call, so a test reads "the address bar" right after a + * write without a DOM. + */ +function fakeWindow(start: string) { + const calls: { method: 'pushState' | 'replaceState'; url: string }[] = []; + const location = { pathname: '/', search: '' }; + const go = (url: string) => { + const u = new URL(url, ORIGIN); + location.pathname = u.pathname; + location.search = u.search; + }; + go(start); + const history = { + pushState: (_data: unknown, _unused: string, url?: string | URL | null) => { + calls.push({ method: 'pushState', url: String(url) }); + go(String(url)); + }, + replaceState: (_data: unknown, _unused: string, url?: string | URL | null) => { + calls.push({ method: 'replaceState', url: String(url) }); + go(String(url)); + }, + }; + const win: HistoryWindow = { location, history }; + /** What a reload would load right now. */ + const addressBar = () => `${location.pathname}${location.search}`; + return { win, calls, addressBar }; +} + +/** What `useUrlState` reads back for `key` after loading `url` (a reload). */ +function readBack(url: string, key: string, defaultValue: T): T { + return deserialise(new URL(url, ORIGIN).searchParams.get(key), defaultValue); +} + +describe('writeParamToHistory', () => { + describe('replace mode', () => { + it('writes the new URL with replaceState and never pushState', () => { + const { win, calls } = fakeWindow('/lore'); + const url = writeParamToHistory(win, 'status', 'archived', 'active', 'replace'); + expect(url).toBe('/lore?status=%22archived%22'); + expect(calls).toEqual([{ method: 'replaceState', url: '/lore?status=%22archived%22' }]); + }); + + it('changes the address bar before it returns — no navigation to wait for', () => { + const { win, addressBar } = fakeWindow('/lore'); + writeParamToHistory(win, 'status', 'archived', 'active', 'replace'); + expect(addressBar()).toBe('/lore?status=%22archived%22'); + }); + }); + + describe('Recent read on the Explorer', () => { + it('choosing Recent read writes sort=%22last_opened_at%22, and a reload reads it back', () => { + const { win, calls, addressBar } = fakeWindow('/lore'); + writeParamToHistory(win, 'sort', 'last_opened_at', null, 'replace'); + expect(calls).toEqual([{ method: 'replaceState', url: '/lore?sort=%22last_opened_at%22' }]); + // The reload happens immediately: the address bar already holds the choice. + expect(addressBar()).toBe('/lore?sort=%22last_opened_at%22'); + expect(readBack(addressBar(), 'sort', null)).toBe('last_opened_at'); + }); + + it('choosing Newest (the default) again drops sort, and a reload reads the default', () => { + const { win, addressBar } = fakeWindow('/lore?sort=%22last_opened_at%22'); + writeParamToHistory(win, 'sort', null, null, 'replace'); + expect(addressBar()).toBe('/lore'); + expect(readBack(addressBar(), 'sort', null)).toBe(null); + }); + }); + + describe('push mode', () => { + it('adds a history entry with pushState when the URL changes', () => { + const { win, calls } = fakeWindow('/lore'); + writeParamToHistory(win, 'lesson', { scope: 'global', key: 'k' }, null, 'push'); + expect(calls).toHaveLength(1); + expect(calls[0].method).toBe('pushState'); + expect(readBack(calls[0].url, 'lesson', null)).toEqual({ scope: 'global', key: 'k' }); + }); + + it('replaces instead of pushing when the value is unchanged, leaving no empty Back step', () => { + const { win, calls } = fakeWindow('/lore?lesson=%7B%22scope%22%3A%22global%22%2C%22key%22%3A%22k%22%7D'); + writeParamToHistory(win, 'lesson', { scope: 'global', key: 'k' }, null, 'push'); + expect(calls.map((c) => c.method)).toEqual(['replaceState']); + }); + + it('pushes when clearing a set value back to its default', () => { + const { win, calls, addressBar } = fakeWindow('/lore?lesson=%22x%22'); + writeParamToHistory(win, 'lesson', null, null, 'push'); + expect(calls.map((c) => c.method)).toEqual(['pushState']); + expect(addressBar()).toBe('/lore'); + }); + + it('one handler that sets two keys adds one history entry (FilterBar "Clear all")', () => { + const { win, calls, addressBar } = fakeWindow('/lore?filters=%5B1%5D&retention=%7B%22a%22%3A1%7D'); + // Same task, as in one click handler: filters, then retention. + writeParamToHistory(win, 'filters', [], [], 'push'); + writeParamToHistory(win, 'retention', {}, {}, 'push'); + expect(calls.map((c) => c.method)).toEqual(['pushState', 'replaceState']); + expect(addressBar()).toBe('/lore'); + }); + + it('separate clicks each add their own history entry', async () => { + const { win, calls } = fakeWindow('/lore'); + writeParamToHistory(win, 'lesson', '"a"', null, 'push'); + await Promise.resolve(); // the click handler returned; the next click is a new task + writeParamToHistory(win, 'lesson', '"b"', null, 'push'); + expect(calls.map((c) => c.method)).toEqual(['pushState', 'pushState']); + }); + }); + + describe('builds from the live URL', () => { + it('keeps params already in the address bar', () => { + const { win, addressBar } = fakeWindow('/lore?scope=%22repo%3A%3Aa%2Fb%22&q=%22auth%22'); + writeParamToHistory(win, 'sort', 'opened_count', null, 'replace'); + const qs = new URL(addressBar(), ORIGIN).searchParams; + expect(qs.get('scope')).toBe('"repo::a/b"'); + expect(qs.get('q')).toBe('"auth"'); + expect(qs.get('sort')).toBe('"opened_count"'); + }); + + it('keeps a param written earlier in the same tick (two keys set by one handler)', () => { + const { win, addressBar } = fakeWindow('/lore'); + writeParamToHistory(win, 'status', 'archived', 'active', 'replace'); + writeParamToHistory(win, 'sort', 'last_opened_at', null, 'replace'); + expect(readBack(addressBar(), 'status', 'active')).toBe('archived'); + expect(readBack(addressBar(), 'sort', null)).toBe('last_opened_at'); + }); + + it('removes the param when set to its default and keeps the rest', () => { + const { win, addressBar } = fakeWindow('/lore?status=%22archived%22&sort=%22opened_count%22'); + writeParamToHistory(win, 'status', 'active', 'active', 'replace'); + expect(addressBar()).toBe('/lore?sort=%22opened_count%22'); + }); + + it('writes on the page the browser shows, keeping its other params (e.g. a Storybook iframe)', () => { + const { win, addressBar } = fakeWindow('/iframe.html?id=pages-lore--default&viewMode=story'); + writeParamToHistory(win, 'sort', 'last_opened_at', null, 'replace'); + expect(addressBar()).toBe('/iframe.html?id=pages-lore--default&viewMode=story&sort=%22last_opened_at%22'); + }); + }); +}); + /** * ── SSR contract (documented, not executed here) ────────────────────────────── * @@ -226,9 +376,10 @@ describe('isPathnameAllowed', () => { * ── Optimistic update contract (documented, not executed here) ───────────────── * * 1. User calls setState(next). - * 2. setOptimisticValue(next) fires synchronously → re-render shows `next` immediately. - * 3. router.replace(url) fires asynchronously → navigation starts. - * 4. When navigation completes, searchParams changes → urlValue becomes `next`. + * 2. writeOptimistic(next) fires synchronously → re-render shows `next` immediately. + * 3. A microtask later, writeParamToHistory writes the address bar (tested + * above); Next.js applies it to useSearchParams() in a transition. + * 4. On that render, searchParams changes → urlValue becomes `next`. * 5. useEffect detects serialise(urlValue) === serialise(optimisticValue) → sets * optimisticValue back to UNSET (URL is now the source of truth again). * 6. External URL changes (back button, direct link) update searchParams → urlValue diff --git a/packages/web/src/lib/hooks/useUrlState.ts b/packages/web/src/lib/hooks/useUrlState.ts index b9d056e39..d38519b3d 100644 --- a/packages/web/src/lib/hooks/useUrlState.ts +++ b/packages/web/src/lib/hooks/useUrlState.ts @@ -12,18 +12,22 @@ * Next.js renders a shell on the server and fills in the real URL-derived value * on the client, avoiding hydration mismatches. * + * ## Writing the URL + * `setState` writes the address bar with `window.history` ({@link writeParamToHistory}) + * a microtask after it returns — before any paint — so a reload right after a choice keeps it. + * A `router.replace` committed the URL only after a server render — seconds on a slow backend. + * * ## Optimistic updates - * URL navigation via `router.replace` is asynchronous: the new search params do - * not appear in `useSearchParams()` until the router has completed the navigation - * cycle (a React transition). Without optimistic state, users would see the old - * value for tens of milliseconds between clicking and the URL updating — causing + * The address bar changes synchronously, but Next.js applies the change to + * `useSearchParams()` inside a React transition, a render later. Without + * optimistic state, readers would show the old value for that render — causing * perceived lag and flicker. * * This hook solves it with a local `optimistic` state that is updated - * synchronously on every `setState` call, while the URL write happens in the - * background. When the URL finally reflects the new value (next render after - * navigation), `optimistic` is reset to `UNSET` so the URL is once again the - * source of truth. + * synchronously on every `setState` call, alongside the URL write. When + * `useSearchParams()` reflects the new value (the next render), + * `optimistic` is reset to `UNSET` so the URL is once again the source of + * truth. * * The optimistic value is shared by KEY across every mounted instance * (`url-state-optimistic.ts`), not held per instance: a write from one @@ -76,8 +80,9 @@ export interface UseUrlStateOptions { */ cleanOnPathname?: string | string[]; /** - * Navigation method. Use 'push' only when you need the back-button to undo - * state changes. 'replace' (default) keeps history clean. + * How a write enters the browser history. Use 'push' only when you need the + * back-button to undo state changes (`history.pushState`). 'replace' + * (default, `history.replaceState`) keeps history clean. * @default 'replace' */ navigationMode?: 'push' | 'replace'; @@ -127,6 +132,55 @@ export function buildUrl( return `${pathname}${qs ? `?${qs}` : ''}`; } +// One handler that sets several push-mode keys (FilterBar's "Clear all") is one +// user step, so only its first URL-changing write pushes; the rest replace that +// entry. Cleared on the next microtask, once the handler has returned. +let pushedThisTask = false; + +function notePush(): void { + if (pushedThisTask) return; + pushedThisTask = true; + queueMicrotask(() => { + pushedThisTask = false; + }); +} + +/** The parts of `window` a URL write touches — passed in so the write is testable without a DOM. */ +export interface HistoryWindow { + location: Pick; + history: Pick; +} + +/** + * Write one param to the address bar, synchronously, and return the URL written. + * @param mode - `push` adds a Back step when the value changes, once per task; `replace` never does. + * @returns The URL now in the address bar. + */ +export function writeParamToHistory( + win: HistoryWindow, + key: string, + next: T, + defaultValue: T, + mode: 'push' | 'replace', +): string { + // The LIVE location, not a render's useSearchParams() snapshot: a second + // write in the same tick then starts from the URL the first just wrote. + const params = new URLSearchParams(win.location.search); + const nextRaw = serialise(next) === serialise(defaultValue) ? null : serialise(next); + // Re-setting the current value pushes nothing, as router.push dedupes it. + const changed = params.get(key) !== nextRaw; + const url = buildUrl(win.location.pathname, params, key, next, defaultValue); + // window.history, not the router: Next.js syncs it into useSearchParams() + // without the server render a router.replace waits on before the URL commits. + if (mode === 'push' && changed && !pushedThisTask) { + win.history.pushState(null, '', url); + notePush(); + } else { + win.history.replaceState(null, '', url); + } + return url; +} + /** * Whether `pathname` matches any of the `allowed` paths (exact or prefix match). * All inputs are normalised to start with '/'. @@ -187,14 +241,6 @@ export function useUrlState( // catching up; falls back to the true URL value otherwise. const value = optimisticValue !== UNSET ? optimisticValue : urlValue; - // ── Navigate ref ─────────────────────────────────────────────────────────── - // Updated synchronously during render (no useEffect) so there is never a - // stale-closure window where setState uses the wrong navigate function. - const navigateRef = useRef['push']>( - navigationMode === 'push' ? router.push : router.replace, - ); - navigateRef.current = navigationMode === 'push' ? router.push : router.replace; - // ── setState ─────────────────────────────────────────────────────────────── const setState = useCallback>( (valueOrUpdater) => { @@ -207,16 +253,16 @@ export function useUrlState( ? (valueOrUpdater as (prev: T) => T)(prev) : valueOrUpdater; - // 1. Optimistic: immediate UI update in every instance reading `key`, no - // wait for the navigation round-trip. + // 1. Optimistic: immediate UI update in every instance reading `key`, + // before `useSearchParams()` catches up a render later. writeOptimistic(key, next, pathname); - // 2. Persist: update the URL asynchronously via the router. - const url = buildUrl(pathname, searchParams, key, next, defaultValue); - navigateRef.current(url, { scroll: false }); + // 2. Persist to the address bar, a microtask later: on a hard load Next.js + // patches window.history in an effect that runs after child mount effects. + queueMicrotask(() => writeParamToHistory(window, key, next, defaultValue, navigationMode)); }, // eslint-disable-next-line react-hooks/exhaustive-deps - [searchParams, pathname, key, defaultValue], + [searchParams, pathname, key, defaultValue, navigationMode], ); // ── cleanOnPathname effect ───────────────────────────────────────────────── diff --git a/packages/web/src/lib/open-lesson.ts b/packages/web/src/lib/open-lesson.ts index 13557b2c4..e7ab15ef9 100644 --- a/packages/web/src/lib/open-lesson.ts +++ b/packages/web/src/lib/open-lesson.ts @@ -103,11 +103,11 @@ export function resolveLessonParam(value: unknown, raw: string | null): LessonPa * The `memoryId` that is still open: the URL's param, unless the user has * dismissed that exact id. * - * Closing the sheet deliberately does NOT strip `?memoryId=` from the URL. That - * strip needed its own `router.replace`, built from the pre-mutation search - * params, and the Lore Explorer calls `closeLesson()` in the same tick as its - * own scope / filter writes — so the second navigation landed last and clobbered - * the scope or filter the user had just set. Dismissal is local state instead, + * Closing the sheet deliberately does NOT strip `?memoryId=` from the URL. When + * this was decided, that strip needed its own `router.replace`, built from the + * pre-mutation search params, and the Lore Explorer calls `closeLesson()` in the + * same tick as its own scope / filter writes — so the second navigation landed + * last and clobbered the scope or filter the user had just set. Dismissal is local state instead, * keyed BY id so a later deep link to a different memory still opens the sheet. * The cost is that the param survives in the URL, which only means a refresh * re-opens the deep link it names.