rbrooks/Quest-Board

Watch

Fork 0

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

New issue

Closed

opened 2026-08-25 20:44:50 +00:00 by claude-bot · 2 comments

claude-bot commented

2026-08-25 20:44:50 +00:00

Collaborator

Severity: MEDIUM

Found in the August 2026 session lifecycle review (#319).

Several of the app's main writing surfaces — the session summary/transcript editors, the Beat Planner's draft notes, and the Session Shelf's tonight-notes field — have no protection against losing unsaved text on navigation, tab close, or an in-app session switch, and at least one of them (Beat Planner) actively discards a draft as a side effect of a completely unrelated UI interaction.

Evidence

A repo-wide search for beforeunload/router-navigation-blocking in webapp/frontend/src returns zero hits — no editor in the app warns before a tab close or navigation away with unsaved changes.
webapp/frontend/src/pages/SessionDetail.jsx:329-338 (summary textarea, rows={10}) and :383-390 region (transcript textarea, rows={20}) — both hold their draft in local component state with no unload guard; navigating away discards the edit silently.
webapp/frontend/src/pages/CampaignPlanning.jsx:264-269 (handleSessionChange) — switching the session selected in the Beat Planner's own dropdown calls setDraft(s?.beat_notes || ""), unconditionally overwriting whatever the GM was mid-typing into the current draft with the (possibly empty) stored value for the newly-selected session — no prompt, no unsaved-changes check, and this is triggered by the tool's own control, not an external navigation.
webapp/frontend/src/pages/SessionShelf.jsx:549 and :648 — the tonight-notes fields save on onBlur; a Ctrl+W/tab-close while focus is still in the field never fires onBlur, so the last several minutes of typing during live play — the shelf being the primary at-the-table writing surface — is lost with no warning.

Failure scenario
A GM is running a live session with the Session Shelf open, jotting improvised notes into the tonight-notes field between scenes. A player asks them to quickly check something in another tab; the GM, multitasking, closes the shelf tab with Ctrl+W instead of switching tabs, with focus still in the notes field. onBlur never fires. Everything typed since the last accidental blur is gone, with no prompt of any kind.

Proposed fix
Add a shared "dirty state" hook used by all of these editors that (a) registers a beforeunload handler while there are unsaved changes, warning on tab close/reload, and (b) blocks/guards in-app navigation (session switches, page navigation) the same way. For Beat Planner specifically, handleSessionChange must check for an unsaved draft and prompt before overwriting it, not overwrite unconditionally. For the Shelf's tonight-notes field, switch from pure onBlur save to a debounced auto-save (or at minimum add the same beforeunload guard) so focus never being blurred doesn't silently drop text during live play.

Acceptance criteria

The session summary and transcript editors warn before navigating away with unsaved changes.
Beat Planner's session-switch dropdown does not silently discard an unsaved draft; it prompts or auto-saves first.
Session Shelf's tonight-notes field either auto-saves on a debounce or is covered by the same unsaved-changes guard, so a tab close mid-edit doesn't lose text.
A shared reusable "dirty state" utility exists rather than several independent implementations.

**Severity: MEDIUM** Found in the August 2026 session lifecycle review (#319). Several of the app's main writing surfaces — the session summary/transcript editors, the Beat Planner's draft notes, and the Session Shelf's tonight-notes field — have no protection against losing unsaved text on navigation, tab close, or an in-app session switch, and at least one of them (Beat Planner) actively discards a draft as a side effect of a completely unrelated UI interaction. **Evidence** - A repo-wide search for `beforeunload`/router-navigation-blocking in `webapp/frontend/src` returns zero hits — no editor in the app warns before a tab close or navigation away with unsaved changes. - `webapp/frontend/src/pages/SessionDetail.jsx:329-338` (summary textarea, `rows={10}`) and `:383-390` region (transcript textarea, `rows={20}`) — both hold their draft in local component state with no unload guard; navigating away discards the edit silently. - `webapp/frontend/src/pages/CampaignPlanning.jsx:264-269` (`handleSessionChange`) — switching the session selected in the Beat Planner's own dropdown calls `setDraft(s?.beat_notes || "")`, unconditionally overwriting whatever the GM was mid-typing into the current draft with the (possibly empty) stored value for the newly-selected session — no prompt, no unsaved-changes check, and this is triggered by the tool's own control, not an external navigation. - `webapp/frontend/src/pages/SessionShelf.jsx:549` and `:648` — the tonight-notes fields save on `onBlur`; a `Ctrl+W`/tab-close while focus is still in the field never fires `onBlur`, so the last several minutes of typing during live play — the shelf being the primary at-the-table writing surface — is lost with no warning. **Failure scenario** A GM is running a live session with the Session Shelf open, jotting improvised notes into the tonight-notes field between scenes. A player asks them to quickly check something in another tab; the GM, multitasking, closes the shelf tab with `Ctrl+W` instead of switching tabs, with focus still in the notes field. `onBlur` never fires. Everything typed since the last accidental blur is gone, with no prompt of any kind. **Proposed fix** Add a shared "dirty state" hook used by all of these editors that (a) registers a `beforeunload` handler while there are unsaved changes, warning on tab close/reload, and (b) blocks/guards in-app navigation (session switches, page navigation) the same way. For Beat Planner specifically, `handleSessionChange` must check for an unsaved draft and prompt before overwriting it, not overwrite unconditionally. For the Shelf's tonight-notes field, switch from pure `onBlur` save to a debounced auto-save (or at minimum add the same `beforeunload` guard) so focus never being blurred doesn't silently drop text during live play. **Acceptance criteria** - [ ] The session summary and transcript editors warn before navigating away with unsaved changes. - [ ] Beat Planner's session-switch dropdown does not silently discard an unsaved draft; it prompts or auto-saves first. - [ ] Session Shelf's tonight-notes field either auto-saves on a debounce or is covered by the same unsaved-changes guard, so a tab close mid-edit doesn't lose text. - [ ] A shared reusable "dirty state" utility exists rather than several independent implementations.

claude-bot added this to the v4.1.0 - Data Durability & Recovery milestone

2026-08-25 20:44:50 +00:00

claude-bot added the

frontend

enhancement

labels

2026-08-25 20:44:50 +00:00

rbrooks referenced this issue from a commit

2026-08-31 05:14:43 +00:00

feat(frontend): stop three editors losing unsaved work (#410)

claude-bot referenced this issue

2026-08-31 05:14:56 +00:00

feat(frontend): stop three editors losing unsaved work (#410) #463

claude-bot referenced this issue

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

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

claude-bot commented

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

Author

Collaborator

PR #463 covers three of the four criteria. Leaving this open for the fourth.

Criterion	Status
Beat Planner's dropdown doesn't silently discard a draft	Done — prompts first
Shelf tonight-notes auto-saves on a debounce	Done — 1.5s, still commits on blur
A shared reusable dirty-state utility exists	Done — `useUnsavedChangesWarning`
Summary/transcript editors warn before navigating away	Partial — tab close and reload only

Why the fourth is only partial

useBlocker calls useDataRouterContext, which invariant-throws without a data router, and App.jsx mounts <BrowserRouter>. No component-level work can block in-app navigation while the app routes this way — it needs createBrowserRouter/RouterProvider, i.e. every route moved.

Filed as #464 and put in v4.4.0, where the routing layer is being reworked anyway; doing it twice would be wasted. Raise it sooner if in-app navigation loss actually bites someone — the tab-close case, which is the one the issue's own failure scenario describes (Ctrl+W with focus in the Shelf notes), is now covered.

Worth knowing for whoever finishes this

Criterion 1 will land softer than it reads even after #464. Browsers ignore the string a beforeunload handler returns — the user gets a generic "Leave site?" dialog, and no app can supply that text. The useBlocker path can show custom UI, but only for in-app navigation; the tab-close prompt stays generic forever. That is a browser constraint, not something left undone.

On the two that shipped

Both were more than warnings. The Shelf's onBlur-only save was losing text during live play — that is the one worth the mutation check, and reverting the debounce fails exactly the two tests that type without ever blurring. The Beat Planner's dropdown was destroying drafts via the tool's own control, which is not something a warning would have helped with; it needed the prompt.

465 frontend tests pass, up from 457.

PR #463 covers three of the four criteria. **Leaving this open** for the fourth. | Criterion | Status | |---|---| | Beat Planner's dropdown doesn't silently discard a draft | **Done** — prompts first | | Shelf tonight-notes auto-saves on a debounce | **Done** — 1.5s, still commits on blur | | A shared reusable dirty-state utility exists | **Done** — `useUnsavedChangesWarning` | | Summary/transcript editors warn before navigating away | **Partial** — tab close and reload only | ## Why the fourth is only partial `useBlocker` calls `useDataRouterContext`, which `invariant`-throws without a data router, and `App.jsx` mounts `<BrowserRouter>`. **No component-level work can block in-app navigation while the app routes this way** — it needs `createBrowserRouter`/`RouterProvider`, i.e. every route moved. Filed as **#464** and put in **v4.4.0**, where the routing layer is being reworked anyway; doing it twice would be wasted. Raise it sooner if in-app navigation loss actually bites someone — the tab-close case, which is the one the issue's own failure scenario describes (`Ctrl+W` with focus in the Shelf notes), is now covered. ## Worth knowing for whoever finishes this Criterion 1 will land softer than it reads even *after* #464. Browsers ignore the string a `beforeunload` handler returns — the user gets a generic "Leave site?" dialog, and no app can supply that text. The `useBlocker` path can show custom UI, but only for in-app navigation; the tab-close prompt stays generic forever. That is a browser constraint, not something left undone. ## On the two that shipped Both were more than warnings. The Shelf's `onBlur`-only save was **losing text during live play** — that is the one worth the mutation check, and reverting the debounce fails exactly the two tests that type without ever blurring. The Beat Planner's dropdown was destroying drafts via the tool's own control, which is not something a warning would have helped with; it needed the prompt. 465 frontend tests pass, up from 457.

claude-bot referenced this issue from a commit

2026-08-31 05:25:21 +00:00

Merge PR #463: stop three editors losing unsaved work (#410)

claude-bot commented

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

Author

Collaborator

Closing. Split so the finished work lands in v4.1.0 and the one piece that needs a routing change moves to v4.4.0 as #464 — which now owns that behaviour outright rather than being a blocker on this issue.

Re-verified against the code today rather than trusting the earlier comment:

Criterion	Status
Beat Planner's dropdown doesn't silently discard a draft	Done — `handleSessionChange` confirms first (CampaignPlanning.jsx:299)
Shelf tonight-notes auto-saves on a debounce	Done — `NOTES_SAVE_DEBOUNCE_MS = 1500`, still commits on blur
A shared reusable dirty-state utility exists	Done — `useUnsavedChangesWarning`, used by all three editors
Summary/transcript editors warn before navigating away	Tab close and reload only → #464

Why this is a defensible close rather than a deferral

The failure scenario this issue was written around is the Ctrl+W with focus still in the Shelf's tonight-notes field during live play. That is covered — and that field now auto-saves on a 1.5s debounce regardless, so the text is usually already persisted before a close happens at all.

What remains is narrower than the original framing: an unsaved edit in the summary or transcript editor, discarded by navigating within the app. beforeunload cannot see an internal route change, and useBlocker invariant-throws without a data router, so it is a routing-layer migration rather than anything that can be done in these components.

That migration touches every route definition and has no user-visible defect of its own. Landing it inside v4.1.0 — a milestone whose whole point is "data is never lost" — would have added more regression surface than the loss it prevents. It belongs with other frontend structural work, which is where #464 now sits.

Two corrections carried over to #464

Both were in its body and would have misled whoever picked it up:

It said ~40 test files mount MemoryRouter. The actual count is 19.
Its timing rationale was "the routing layer is being reworked in v4.4.0 anyway, so doing it twice would be wasted." That does not hold — BrowserRouter → createBrowserRouter is a mechanism change, and if v4.4.0 rewrites the route list it does so inside whatever mechanism is current, so an early migration would be inherited rather than redone. The real reason to defer is risk placement, and #464 now says so.

One thing worth knowing for whoever finishes #464: even then, the tab-close prompt stays generic. Browsers ignore the string a beforeunload handler returns, so no app can supply that text. useBlocker can show custom UI, but only for in-app navigation. Browser constraint, not scope.

Closing. Split so the finished work lands in v4.1.0 and the one piece that needs a routing change moves to v4.4.0 as **#464** — which now owns that behaviour outright rather than being a blocker on this issue. Re-verified against the code today rather than trusting the earlier comment: | Criterion | Status | |---|---| | Beat Planner's dropdown doesn't silently discard a draft | **Done** — `handleSessionChange` confirms first ([CampaignPlanning.jsx:299](webapp/frontend/src/pages/CampaignPlanning.jsx#L299)) | | Shelf tonight-notes auto-saves on a debounce | **Done** — `NOTES_SAVE_DEBOUNCE_MS = 1500`, still commits on blur | | A shared reusable dirty-state utility exists | **Done** — `useUnsavedChangesWarning`, used by all three editors | | Summary/transcript editors warn before navigating away | **Tab close and reload only** → #464 | ## Why this is a defensible close rather than a deferral The failure scenario this issue was written around is the `Ctrl+W` with focus still in the Shelf's tonight-notes field during live play. That is covered — and that field now auto-saves on a 1.5s debounce regardless, so the text is usually already persisted before a close happens at all. What remains is narrower than the original framing: an unsaved edit in the summary or transcript editor, discarded by navigating *within* the app. `beforeunload` cannot see an internal route change, and `useBlocker` `invariant`-throws without a data router, so it is a routing-layer migration rather than anything that can be done in these components. That migration touches every route definition and has no user-visible defect of its own. Landing it inside v4.1.0 — a milestone whose whole point is "data is never lost" — would have added more regression surface than the loss it prevents. It belongs with other frontend structural work, which is where #464 now sits. ## Two corrections carried over to #464 Both were in its body and would have misled whoever picked it up: - It said ~40 test files mount `MemoryRouter`. The actual count is **19**. - Its timing rationale was *"the routing layer is being reworked in v4.4.0 anyway, so doing it twice would be wasted."* That does not hold — `BrowserRouter` → `createBrowserRouter` is a mechanism change, and if v4.4.0 rewrites the route list it does so inside whatever mechanism is current, so an early migration would be inherited rather than redone. The real reason to defer is risk placement, and #464 now says so. One thing worth knowing for whoever finishes #464: even then, the tab-close prompt stays generic. Browsers ignore the string a `beforeunload` handler returns, so no app can supply that text. `useBlocker` can show custom UI, but only for in-app navigation. Browser constraint, not scope.

claude-bot closed this issue

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

rbrooks referenced this issue from a commit

2026-09-06 05:49:49 +00:00

feat(frontend): a front door for Prep, and outputs that go somewhere (#374)

claude-bot referenced this issue

2026-09-06 05:50:12 +00:00

feat(webapp): the Prep lane: pickers, notes, re-open, a steerable sheet, a front door, and clocks (phase 3 of v4.3.0) #528

claude-bot referenced this issue

2026-09-06 05:50:43 +00:00

[Frontend] Fix the GM Workbench: buried entry point, wrong default tool, unreadable session picker, and dead-end outputs #374

rbrooks referenced this issue from a commit