[Frontend] Add a dirty-state guard to session, beat-planner, and shelf editors #410
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
beforeunload/router-navigation-blocking inwebapp/frontend/srcreturns 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-390region (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 callssetDraft(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:549and:648— the tonight-notes fields save ononBlur; aCtrl+W/tab-close while focus is still in the field never firesonBlur, 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+Winstead of switching tabs, with focus still in the notes field.onBlurnever 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
beforeunloadhandler 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,handleSessionChangemust check for an unsaved draft and prompt before overwriting it, not overwrite unconditionally. For the Shelf's tonight-notes field, switch from pureonBlursave to a debounced auto-save (or at minimum add the samebeforeunloadguard) so focus never being blurred doesn't silently drop text during live play.Acceptance criteria
PR #463 covers three of the four criteria. Leaving this open for the fourth.
useUnsavedChangesWarningWhy the fourth is only partial
useBlockercallsuseDataRouterContext, whichinvariant-throws without a data router, andApp.jsxmounts<BrowserRouter>. No component-level work can block in-app navigation while the app routes this way — it needscreateBrowserRouter/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+Wwith 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
beforeunloadhandler returns — the user gets a generic "Leave site?" dialog, and no app can supply that text. TheuseBlockerpath 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.
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:
handleSessionChangeconfirms first (CampaignPlanning.jsx:299)NOTES_SAVE_DEBOUNCE_MS = 1500, still commits on bluruseUnsavedChangesWarning, used by all three editorsWhy this is a defensible close rather than a deferral
The failure scenario this issue was written around is the
Ctrl+Wwith 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.
beforeunloadcannot see an internal route change, anduseBlockerinvariant-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:
MemoryRouter. The actual count is 19.BrowserRouter→createBrowserRouteris 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
beforeunloadhandler returns, so no app can supply that text.useBlockercan show custom UI, but only for in-app navigation. Browser constraint, not scope.