Skip to content

fix(web): write URL state with window.history so reloads keep it - #722

Merged
mthines merged 7 commits into
mainfrom
fix/url-state-history-replace
Oct 22, 2026
Merged

mthines merged 7 commits into
mainfrom
fix/url-state-history-replace

Conversation

@dash0-dev

@dash0-dev dash0-dev Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Why

A sort chosen on /lore reached the URL only after router.replace's server render returned — 4.3 s on #718's preview — so a reload inside that window brought the Explorer back to Newest.

What changed

  • useUrlState's setter writes the address bar with window.history.replaceState (pushState in push mode), built from the live URL; Next syncs it into useSearchParams without a server render.
  • Push mode adds one Back step per click, so FilterBar's "Clear all", which sets two push-mode keys, stays one step.
  • The write runs one microtask after setState, so on a hard load it lands after Next has patched window.history.
  • Unit tests for the write, and an Explorer interaction story that fails on the old router setter.

How to verify

  • cd packages/web && npx vitest run src/lib/hooks/useUrlState.spec.ts, then on the preview pick Recent read and reload at once — it stays Recent read.

Notes for reviewers

The microtask deferral (last commit) came after the final local review pass, so it has had no independent review. The cleanOnPathname / cleanOnUnmount clean-ups still go through the router.

🧪 UI verification spec — run against the preview deployment

Target: preview
Format: intent

Spec 1: R2 — A sort choice survives a reload made right after it

Changed: the URL write behind the Lore Explorer's Sort memories control (useUrlState now writes window.history)
Start: /lore
Preconditions:

  • The signed-in account has at least one lesson, so the Explorer list renders.
  • The viewport is desktop width (the phone-width Sort control is labelled "Sort memories: ").
    Steps:
  1. Open the Lore Explorer and wait for the lesson list.
  2. [must-follow] Choose Recent read in the Sort memories control.
  3. [must-follow] Within 300 ms of step 2 — without waiting for any request — read the page URL, then reload the page.
  4. Choose Newest in the Sort memories control, restoring the default.
    Expected:
  • The page URL read in step 3 carries sort=%22last_opened_at%22.
  • After the reload in step 3 the Sort memories control shows "Recent read".
  • After step 4 the page URL carries no sort parameter.
    Hints:
  • {role: "combobox", name: "Sort memories"} — packages/web/src/components/lore/SortSelect.tsx:88
  • The write — packages/web/src/lib/hooks/useUrlState.ts (writeParamToHistory)
    Out of scope:
  • Which lesson appears first under Recent read; the order is the API's (PR feat(explorer): add a Recent read sort — last deliberate agent read first #718).

Spec 2: R4 — Removing a filter is one Back step, and Back restores it

Changed: the push-mode URL write behind the Explorer filter bar (filters, navigationMode: 'push')
Start: /lore?filters=%5B%7B%22field%22%3A%22kind%22%2C%22operator%22%3A%22in%22%2C%22values%22%3A%5B%22lesson%22%5D%7D%5D
Preconditions:

  • The signed-in account has at least one lesson.
  • The viewport is desktop width.
    Steps:
  1. Open the start URL and wait for the filter bar to show the kind filter pill.
  2. [must-follow] Click the pill's Remove filter button.
  3. [must-follow] Press the browser's Back button once.
    Expected:
  • After step 2 the filter bar shows no "Remove filter" button and the page URL no longer contains %22kind%22.
  • After step 3 the page URL is the start URL again (its filters value contains %22kind%22).
  • After step 3 the filter bar shows the kind filter pill again, with its "Remove filter" button.
    Hints:
  • {role: "button", name: /^Remove filter: /} — packages/web/src/components/lore/FilterPill.tsx:327
  • filters is push mode — packages/web/src/components/lore/LoreExplorer.tsx:478

mthines and others added 6 commits October 22, 2026 04:39
…eeps them

useUrlState's setter wrote the URL with router.replace, which commits the
address bar only once the route's server render returns. On PR #718's
preview that render took 4.3 s, and a reload inside that window brought
the Explorer back to Newest (aw-verify F-1).

The setter now writes with window.history.replaceState (pushState in push
mode, deduped like router.push). Next.js syncs both into useSearchParams
without a server render, so the URL holds the choice as soon as setState
returns. The write builds from the live location, so two keys set in one
tick both land. The clean-up effects keep using the router.

Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com>
Interaction story on the Explorer: choosing Recent read must put
sort=%22last_opened_at%22 in window.location in the same step, and
Newest must remove it. Fails against the router.replace setter
(sort reads null), passes with the window.history write.

Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com>
FilterBar's Clear all sets filters and then retention, both push-mode,
in one click. Writing each with pushState left two Back steps, the first
landing on a state the reader never saw. Within one task only the first
URL-changing push-mode write pushes; later ones replace that entry, as
the router used to collapse same-tick navigations.

Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com>
…ines they explain

Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com>
…e write path

url-state-optimistic's header and the two MemorySidebarProvider guards
explained a router round trip and a same-tick clobber that the
window.history write from the live URL no longer has. The guards stay;
their reasons now read in the past tense.

Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com>
Next.js installs its window.history patch in an App Router effect. On a
hard load a child's mount effect in the same commit runs first, so a
setter called there (OrganizationManager clearing a stale ?org=) would
hit the raw History API and Next's router would never see the write.
React runs a commit's mount effects in one synchronous flush, so one
microtask later the patch is in; still before any paint or reload.

Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com>
@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

The dashboard preview for this PR — redeployed on each push that changes the web app.

Project Deployment Actions Updated
lorekit Ready Ready Preview Oct 22, 2026 5:19am UTC

Preview always points at this PR's latest commit; Deployment is the immutable build of f5b20e1. · Comment /web-preview to force a redeploy. · Workflow logs

Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com>
@dash0-dev

dash0-dev Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

Verify (preview) — ⚠️ verified with open items

Every acceptance criterion and guardrail holds on the PR's preview. A sort choice is in the URL as soon as it is made (0 ms after the select), an immediate reload keeps it, push mode is one Back step even when one click writes two keys, and a sort change no longer makes any server round trip. The open items are two pre-existing backend latency findings outside this PR. No code was changed (preview run).

PR mthines/lorekit#722 · no ticket
Commit verified f5b20e1 (branch fix/url-state-history-replace)
Environment preview · https://lorekit-pr-722-mads-thines-projects.vercel.app · web preview → production Supabase api (web-only diff)
Telemetry dash0-mcp · dataset default · 0 loop(s)
Run 2026-10-22T05:39Z → 2026-10-22T06:15Z

Acceptance criteria

ID Criterion Checks Browser Telemetry Evidence
AC-1 Replace mode writes with replaceState, never pushState pass (CI) pass seen history.length 2 → 2 after a sort change; S-5
AC-2 Recent read on /lore writes sort=%22last_opened_at%22, which a reload reads back pass (CI) pass seen URL read 0 ms after the select; reload GET /lore?sort=%22last_opened_at%22 200, control "Recent read"; S-1
AC-3 Push mode adds a history entry when the URL changes pass (CI) pass seen remove filter: history.length 2 → 3, one Back restores the exact start URL; S-3
AC-4 The default removes the param; params already in the URL are kept pass (CI) pass seen scope kept beside sort; Newest restores the exact start URL; S-5
AC-5 The setter does not navigate through the router pass (CI) pass confirms no /lore request after a sort change; 0 RSC GET /lore?sort= spans (main: 19 in 7 days)
AC-6 Web unit tests, typecheck and lint pass pass — — CI on f5b20e1: Typecheck, Test & Lint ✓, Storybook interaction + visual ✓, Playwright e2e ✓

Guardrails: exported signature unchanged (no useUrlState / UseUrlStateOptions line in the diff; CI typecheck green) · llms.txt not in the diff · no .skip / .only added.

Edge cases

ID Probe Result Evidence
EC-1 Sort change, navigate to Insights, Back pass Back → /lore?sort=%22last_opened_at%22 (+198 ms), "Recent read", Explorer list; S-7
EC-2 "Clear all" over a filter + a retention condition (two push keys, one click) pass history.length 2 → 3; one Back → exact start URL, 2 pills; S-8
EC-3 Hard load /settings/organization?org=<unknown>, leave, Back pass org cleaned ~100 ms after load; Back → /settings/organization, "Your organizations", 0 page errors; S-9
EC-4 List request → 500 on Recent read pass URL carries sort 0 ms after the select; "Failed to load memories. Please refresh."; reload keeps Recent read; S-10
EC-5 Offline sort change pass URL carries sort=%22opened_count%22 0 ms after the select while offline; 0 page errors; S-11
EC-6 Five sort picks in 929 ms pass URL is the last pick at once; history.length unchanged; last 200 list call last_opened_at; S-12
Not applicable (7)

Findings

ID Severity Kind What happened Status
T-1 medium performance pre-existing: 10 of 57 /lore server renders took over 1 s (max 4.4 s), each held by one production Supabase call during render (memories/scopes, a tag-count HEAD) open
T-2 medium performance pre-existing: 18 of 417 production api spans took over 1 s (max 6.3 s) in two bursts, almost all IO wait (auth, then the list query) open

Fixes

No code changes.

Screenshots

not attached (every capture shows the test account's email in the sidebar). 20 images are kept locally: S-1, S-2 Spec 1 · S-3, S-4 Spec 2 · S-5, S-6 Spec 3 · S-7 EC-1 · S-8 EC-2 · S-9 EC-3 · S-10 EC-4 · S-11 EC-5 · S-12 EC-6, plus each spec's other step shots.

Under the hood

  • AC-1 seen — lore.sort_changed last_opened_at on /lore?scope=… → URL becomes ?scope=…&sort=%22last_opened_at%22 in the same page load → POST /functions/v1/memories/list 200 (749 ms) → api lorekit.memories 200 (213 ms) → lorekit_memory_list (135 ms)
  • AC-2 seen — lore.sort_changed → list call already sent from /lore?sort=%22last_opened_at%22 20 ms later → reload → web GET /lore?sort=%22last_opened_at%22 200 (779 ms) → list call still on that sort, 200
  • AC-3 seen — lore.filter_changed remove → list call on /lore → Back → list call on /lore?filters=[kind in lesson] 200, one page load, no document request
  • AC-4 seen — scope kept beside sort; picking the default removed sort
  • AC-5 confirms — 0 RSC GET /lore?sort= spans across both windows; the 6 document GET /lore?sort= spans follow the probes' own full reloads
  • [receipt] verdict: confirms

Open and escalated

  • T-1 / T-2 — production Supabase latency (pre-existing, not this PR). The /lore server render blocks on memories/scopes and a tag-count query, and the api spends seconds in IO wait during bursts. Since fix(web): write URL state with window.history so reloads keep it #722, the sort/filter URL no longer waits on this; the first page load still does. Options: stop the /lore render blocking on those reads (stream them, or fetch client-side), or investigate the burst latency in Supabase. Recommendation: track it separately; it does not block this PR.

Lessons

  • Written: verify-lessons::agent0-reset-wipes-agent-dir-restore-brief-from-pr (new); verify-lessons::url-state-write-commits-after-rsc (corrected for the window.history write).
  • Promotion-eligible: none at seen_count ≥ 3 seen by this run.

Generated by /aw-verify · full artifacts in .agent/fix/url-state-history-replace/verify/

@mthines
mthines marked this pull request as ready for review October 22, 2026 09:02
@mthines
mthines self-requested a review as a code owner October 22, 2026 09:02
@dash0-dev

dash0-dev Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

Darkplane Automerge did not approve this pull request. Score 55.73 of 100, threshold 75. Blast radius 62, Evidence quality 80, Change footprint 25.

Darkplane Automerge did not approve this pull request. It scored 55.73 of 100, and 75 was required.

  • 🟡 Blast radius scored 62.
    The core change is in useUrlState.ts. setState now writes the URL through window.history instead of router.replace/push. That hook is shared by LoreExplorer.tsx, LessonDetailSheet.tsx, MemorySidebarProvider.tsx, AuditLogFeed.tsx, OrganizationManager.tsx and useDebouncedUrlState.ts, so URL and back-button behavior shifts across several dashboard pages. The change is client-only, has no data or schema effects, and a revert undoes it cleanly. The other edits are mostly comments, plus a spec file (+154/-3) and a story file (+27/-1).
  • 🔴 Change footprint scored 25.
    useUrlState.ts is a core shared hook, and the PR replaces router.replace/router.push with new hand-written window.history logic. That logic adds a microtask write, a once-per-task push guard and a new exported writeParamToHistory. The other files (open-lesson.ts, url-state-optimistic.ts, useDebouncedUrlState.ts, MemorySidebarProvider.tsx) are mostly small comment edits. About 185 lines are tests, which limits the risk, but the central change is not mechanical.
The other criterion
  • 🟢 Evidence quality scored 80.
    The change adds unit tests in useUrlState.spec.ts and an Explorer story in LorePage.test.stories.tsx that fails on the old setter. On head f5b20e1, Typecheck/Test/Lint, Storybook interaction, Playwright e2e and the Vercel preview all pass, and a preview verification run reports its acceptance criteria and edge cases passing. There is no human review, and the verification report comes from the same bot author, which holds the score below the top band.

An AI evaluator produced these scores from the diff, the repository history, and the pull request text. The pull request text is author-controlled, so treat the scores as signals rather than proof. Reviewed for commit f5b20e1.

Was this decision right? Give feedback

@mthines

mthines commented Oct 22, 2026

Copy link
Copy Markdown
Owner

@dash0 > /review-loop

@dash0-dev

dash0-dev Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

⚡ Running Agent0 | Execute…
View Agent0 | Execute in Agent0

@dash0-dev

dash0-dev Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

✅ No issues found

Moves useUrlState's URL write from router.replace to a microtask window.history call. The write path, push dedupe and Next 15.3 history sync check out; the deferred write has no hook-level test.

1 advisory finding below the confidence bar — see Less certain below.

Less certain (1) — advisory, below the confidence bar
Finding Where
1 suggestion: Only writeParamToHistory is tested, called synchronously. The microtask deferral and the push dedupe across two setState calls are not.
Confidence 79
packages/web/src/lib/hooks/useUrlState.ts:262
Review details
Gate Details
Description vs. code ✅ Description matches the diff; comment-only edits in four files go unmentioned but are immaterial.
Prior review feedback ✅ Earlier review comments are resolved.
Documentation ✅ Docblocks and PR body explain the history write, push dedupe and microtask deferral.
Self-review signals ✅ No debug logs, leftover TODOs, or unreviewed stubs.
Code review ✅ The multi-lens review found no blocking issues.
Run Details
Mode full · 331 lines in delta · tier deep · depth checkout
Anomaly ⚠️ finders and verifier ran in one context, one after another (no sub-agent isolation); step markers started after the memory and gates steps; knowledge list had a second page, not read — 2 prepare-time anomalies (reviewer identity unknown (no --reviewer-login / PR_REVIEWER_LOGIN; /user 401s here) — relation defaulted to cross, +1 more)
Checked 7 of 7 changed files read · 3 possible issues → 0 confirmed → 0 posted
Reach 7 changed exports · 12 dependent files · 2 checked · 10 not checked · 2 changed files imported by 4 files, not traced
useUrlState — 7 consumer files · 5 checked
MemorySidebarProvider — 4 consumer files · 0 checked
resolveLessonParam — 2 consumer files · 0 checked
buildUrl — 1 consumer file · 0 checked
UseUrlStateOptions — 1 consumer file · 0 checked
+2 more
packages/web/src/lib/hooks/url-state-optimistic.ts — imported by 2 files
packages/web/src/lib/hooks/useDebouncedUrlState.ts — imported by 2 files
Found produced 3 → posted inline 0 · cleared 0 · carried forward 0 · deferred 0 · below-bar 1
Dropped 1 below-bar
Standards ran - 2 governing docs, 65 bullets; no violation found in the diff
Measurability ran - abbreviated: skill head read only; client state fix, no new surface, no gap raised
Integrations none
Memories no relevance rules or lessons consulted

Nothing to report — severity, 0 files skipped.

pr-reviewer · commit f5b20e1 · full review · how these findings are produced · updated Oct 22, 2026 9:09am UTC

@dash0-dev

dash0-dev Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

review-loop on #722 converged at iteration 1 of 5. No commit was pushed, because nothing needed changing.

  • Review: PASS, 0 new actionable findings, 0 review threads, on head f5b20e1. Report: fix(web): write URL state with window.history so reloads keep it #722 (comment)
  • Review source: the pr-reviewer bundle run through a general sub-agent, installed on demand. It ran serially in one context, so there was no independent verifier. No type check or test run was possible there.
  • CI at exit: green on f5b20e1. The skipping rows are path-filtered jobs.
  • PR description: unchanged, since no code changed.
  • UI verify (step 1.6): not run. VERCEL_AUTOMATION_BYPASS_SECRET is unset in this sandbox, and the /lore specs need a signed-in session. The existing verify report on f5b20e1 covers the browser run.
  • Merge, undraft, ci-auto-fix: not requested or needed.

Two advisory notes from the reviewer, below the posting threshold (not blocking):

  • The microtask-deferred write inside setState (useUrlState.ts:262) has no hook-level test. Only writeParamToHistory is tested, called synchronously.
  • Two comments still describe a router navigation: useDebouncedUrlState.ts:100 and MemorySidebarProvider.tsx:154.

Per the PR description, the last commit's microtask deferral has had no independent review before this pass.

@mthines
mthines merged commit b56cde2 into main Oct 22, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant