[Frontend] Warn before an in-app navigation discards an unsaved edit (needs a data router)

claude-bot commented

2026-08-31 05:15:16 +00:00

Collaborator

Severity: LOW. Split from #410, which is now closed — this issue owns the remaining behaviour outright rather than being a blocker on it.

What is missing

The session summary and transcript editors warn before a tab close or reload, and nothing else. Navigating within the app — clicking through to another session, say — still discards an unsaved edit silently.

#410 delivered the rest of the dirty-state work in PR #463: the shared useUnsavedChangesWarning hook, the Beat Planner's confirm-before-discard on its own session dropdown, and the Shelf's 1.5s debounced auto-save for tonight-notes. This is the one behaviour that could not be built at the time.

Why it could not be built

App.jsx mounts <BrowserRouter> — the declarative router. React Router's useBlocker calls useDataRouterContext("useBlocker"), which invariant-throws when there is no data router:

function useDataRouterContext(hookName) {
  let ctx = React.useContext(DataRouterContext);
  invariant(ctx, getDataRouterConsoleError(hookName));
  return ctx;
}

So no amount of work inside a component can block in-app navigation while the app routes this way. beforeunload covers tab close and reload; an internal route change never fires it. The fix is a routing-layer migration, which is why it was split out.

Scope

Migrate App.jsx from <BrowserRouter> + <Routes> to createBrowserRouter + <RouterProvider>, then use useBlocker in useUnsavedChangesWarning (or a sibling hook) for the summary and transcript editors, keeping beforeunload for the tab-close case a data router does not help with.

Worth knowing on the way:

Route-level errorElement becomes available, which may subsume some current per-page error handling.
useNavigate, useParams, useLocation and <Link> are all unaffected.
19 test files mount MemoryRouter (verified 2026-09-01 — an earlier version of this issue said "~40", which was wrong). They keep working, since MemoryRouter stays supported; only a test that wants to exercise blocking needs createMemoryRouter.
Browsers ignore the string a beforeunload handler returns, so the tab-close prompt is generic and always will be. useBlocker can show custom UI, but only for in-app navigation. That is a browser constraint, not scope.

Timing — why v4.4.0

Two corrections to the rationale this issue previously carried, because it would have misled whoever picked this up:

It said the routing layer "is being reworked [in v4.4.0] anyway, and doing it twice would be wasted." That does not hold. BrowserRouter → createBrowserRouter is a mechanism change; if v4.4.0 rewrites the route list, it rewrites it inside whatever mechanism is current. Migrating early would be inherited, not redone. v4.4.0's own description does not actually mention routing.
The real reason to defer is risk placement, not duplicated effort. This is a refactor touching every route definition, with no user-visible defect of its own, and the loss it prevents — an unsaved textarea draft discarded by in-app navigation — is the smallest in its class. Landing that inside v4.1.0, a milestone whose entire purpose is "data is never lost", would have introduced more regression surface than it removed. It sits better alongside other frontend structural work.

Pull it forward if in-app navigation loss actually bites someone. The case people reported — Ctrl+W with focus in the Shelf notes during live play — is already covered, and that field auto-saves regardless.

Acceptance criteria

App.jsx uses createBrowserRouter / RouterProvider.
Every existing route still resolves, and the full frontend suite passes.
The session summary and transcript editors warn before an in-app navigation discards an unsaved edit, with beforeunload retained for tab close and reload.
A test exercises the blocking path via createMemoryRouter, and is confirmed to fail without the blocker.

**Severity: LOW.** Split from #410, which is now closed — this issue owns the remaining behaviour outright rather than being a blocker on it. ## What is missing The session summary and transcript editors warn before a **tab close or reload**, and nothing else. Navigating within the app — clicking through to another session, say — still discards an unsaved edit silently. #410 delivered the rest of the dirty-state work in PR #463: the shared `useUnsavedChangesWarning` hook, the Beat Planner's confirm-before-discard on its own session dropdown, and the Shelf's 1.5s debounced auto-save for tonight-notes. This is the one behaviour that could not be built at the time. ## Why it could not be built `App.jsx` mounts `<BrowserRouter>` — the declarative router. React Router's `useBlocker` calls `useDataRouterContext("useBlocker")`, which `invariant`-throws when there is no data router: ```js function useDataRouterContext(hookName) { let ctx = React.useContext(DataRouterContext); invariant(ctx, getDataRouterConsoleError(hookName)); return ctx; } ``` So **no amount of work inside a component can block in-app navigation while the app routes this way.** `beforeunload` covers tab close and reload; an internal route change never fires it. The fix is a routing-layer migration, which is why it was split out. ## Scope Migrate `App.jsx` from `<BrowserRouter>` + `<Routes>` to `createBrowserRouter` + `<RouterProvider>`, then use `useBlocker` in `useUnsavedChangesWarning` (or a sibling hook) for the summary and transcript editors, keeping `beforeunload` for the tab-close case a data router does not help with. Worth knowing on the way: - Route-level `errorElement` becomes available, which may subsume some current per-page error handling. - `useNavigate`, `useParams`, `useLocation` and `<Link>` are all unaffected. - **19 test files** mount `MemoryRouter` (verified 2026-09-01 — an earlier version of this issue said "~40", which was wrong). They keep working, since `MemoryRouter` stays supported; only a test that wants to exercise blocking needs `createMemoryRouter`. - Browsers ignore the string a `beforeunload` handler returns, so the tab-close prompt is generic and always will be. `useBlocker` can show custom UI, but only for in-app navigation. That is a browser constraint, not scope. ## Timing — why v4.4.0 Two corrections to the rationale this issue previously carried, because it would have misled whoever picked this up: - It said the routing layer *"is being reworked [in v4.4.0] anyway, and doing it twice would be wasted."* That does not hold. `BrowserRouter` → `createBrowserRouter` is a **mechanism** change; if v4.4.0 rewrites the route *list*, it rewrites it inside whatever mechanism is current. Migrating early would be inherited, not redone. v4.4.0's own description does not actually mention routing. - The real reason to defer is **risk placement**, not duplicated effort. This is a refactor touching every route definition, with no user-visible defect of its own, and the loss it prevents — an unsaved textarea draft discarded by in-app navigation — is the smallest in its class. Landing that inside v4.1.0, a milestone whose entire purpose is "data is never lost", would have introduced more regression surface than it removed. It sits better alongside other frontend structural work. Pull it forward if in-app navigation loss actually bites someone. The case people reported — `Ctrl+W` with focus in the Shelf notes during live play — is already covered, and that field auto-saves regardless. ## Acceptance criteria - [ ] `App.jsx` uses `createBrowserRouter` / `RouterProvider`. - [ ] Every existing route still resolves, and the full frontend suite passes. - [ ] The session summary and transcript editors warn before an **in-app** navigation discards an unsaved edit, with `beforeunload` retained for tab close and reload. - [ ] A test exercises the blocking path via `createMemoryRouter`, and is confirmed to fail without the blocker.

claude-bot added this to the v4.5.0 - UX Overhaul: Design Exploration & Rebuild milestone

2026-08-31 05:15:40 +00:00

claude-bot referenced this issue

2026-08-31 05:15:54 +00:00

[Frontend] Add a dirty-state guard to session, beat-planner, and shelf editors #410

claude-bot changed title from ~~[Frontend] Migrate to a data router so in-app navigation can be blocked~~ to [Frontend] Warn before an in-app navigation discards an unsaved edit (needs a data router)

2026-09-01 16:31:56 +00:00

claude-bot added the

frontend

label

2026-09-01 16:31:59 +00:00

claude-bot referenced this issue

2026-09-01 16:32:15 +00:00

[Frontend] Add a dirty-state guard to session, beat-planner, and shelf editors #410

claude-bot commented

2026-09-06 19:27:20 +00:00

Author

Collaborator

Picking this up as the first engineering lane of v4.4.0. It is a mechanism change with no dependence on the design direction, and the rebuild inherits whichever router mechanism is current, so it goes first. Scope exactly as the body: createBrowserRouter + RouterProvider, useBlocker in the unsaved-changes hook for the summary and transcript editors, beforeunload kept, a createMemoryRouter test that fails without the blocker.

Picking this up as the first engineering lane of v4.4.0. It is a mechanism change with no dependence on the design direction, and the rebuild inherits whichever router mechanism is current, so it goes first. Scope exactly as the body: `createBrowserRouter` + `RouterProvider`, `useBlocker` in the unsaved-changes hook for the summary and transcript editors, `beforeunload` kept, a `createMemoryRouter` test that fails without the blocker.

claude-bot referenced this issue

2026-09-06 19:27:23 +00:00

[Frontend] Adopt the React Compiler lint rules from eslint-plugin-react-hooks v7 #182

claude-bot referenced this issue

2026-09-06 19:30:20 +00:00

v4.4.0 build order: UX Overhaul in six phases, with two review checkpoints #535

rbrooks referenced this issue from a commit

2026-09-06 20:03:10 +00:00

refactor(frontend): mount the route table on a data router (#464)

rbrooks referenced this issue from a commit

2026-09-06 20:03:10 +00:00

fix(frontend): warn before an in-app navigation discards an unsaved edit (#464)

claude-bot referenced this issue

2026-09-06 20:03:13 +00:00

[Frontend] Shelf tonight-notes: the auto-save debounce is cancelled on unmount without flushing #536

claude-bot referenced this issue from a pull request that will close it,

2026-09-06 20:03:38 +00:00

fix(frontend): warn before an in-app navigation discards an unsaved edit (#464) #537

claude-bot commented

2026-09-06 20:04:27 +00:00

Author

Collaborator

Done in PR #537 (auto-merge armed).

App.jsx now mounts createBrowserRouter + RouterProvider (from react-router/dom), with the JSX route tree kept through createRoutesFromElements and exported for tests. Every route, redirect, the single AuthGuard and the catch-all are unchanged; AuthProvider and CampaignProvider moved into a pathless root layout route because the latter calls router hooks.
useUnsavedChangesPrompt (a sibling of the unload-only hook, which is unchanged) confirms before a pathname change while an edit is dirty, registers through the router's public blocker API, and no-ops without a data router so the ~30 MemoryRouter page tests keep working. ?tab= and #recording never prompt. Applied to the summary and transcript editors.
The createMemoryRouter test was seen to fail with the blocker removed (2 failed / 9 passed) and passes with it.
Beyond the brief, deliberately: a data router wraps the root match in its own error boundary and its default element prints a raw stack trace, in production too, so the migration alone would have bypassed the existing ErrorBoundary. The root route now carries an errorElement rendering the same fallback, with a test.

Found on the way and filed as #536: the Shelf's tonight-notes debounce is cancelled on unmount without flushing, which loses up to 1.5 s of typing today; a prompt would surface that loss, not prevent it.

Done in PR #537 (auto-merge armed). - `App.jsx` now mounts `createBrowserRouter` + `RouterProvider` (from `react-router/dom`), with the JSX route tree kept through `createRoutesFromElements` and exported for tests. Every route, redirect, the single `AuthGuard` and the catch-all are unchanged; `AuthProvider` and `CampaignProvider` moved into a pathless root layout route because the latter calls router hooks. - `useUnsavedChangesPrompt` (a sibling of the unload-only hook, which is unchanged) confirms before a pathname change while an edit is dirty, registers through the router's public blocker API, and no-ops without a data router so the ~30 `MemoryRouter` page tests keep working. `?tab=` and `#recording` never prompt. Applied to the summary and transcript editors. - The `createMemoryRouter` test was seen to fail with the blocker removed (2 failed / 9 passed) and passes with it. - **Beyond the brief, deliberately:** a data router wraps the root match in its own error boundary and its default element prints a raw stack trace, in production too, so the migration alone would have bypassed the existing `ErrorBoundary`. The root route now carries an `errorElement` rendering the same fallback, with a test. Found on the way and filed as #536: the Shelf's tonight-notes debounce is cancelled on unmount without flushing, which loses up to 1.5 s of typing today; a prompt would surface that loss, not prevent it.

claude-bot closed this issue

2026-09-06 20:26:44 +00:00

claude-bot referenced this issue from a commit

2026-09-06 20:26:45 +00:00

Merge PR #537: fix(frontend): warn before an in-app navigation discards an unsaved edit (#464)

rbrooks referenced this issue from a commit

2026-09-06 20:37:00 +00:00

fix(frontend): mint the unsaved-changes blocker key with useId, not a setState in an effect (#182)

rbrooks referenced this issue from a commit

2026-09-06 20:41:55 +00:00

fix(frontend): mint the unsaved-changes blocker key with useId, not a setState in an effect (#182)

claude-bot referenced this issue