feat: warn before leaving system configuration with unsaved changes - #218
Merged
Conversation
The configuration screen stages every edit across every section into a single draft behind one Save. There was no unload handler and no in-app navigation guard, so navigating away, using the back button, or refreshing threw all of it away with no prompt. The sticky bar reported unsaved changes but did nothing to protect them. The guard uses two mechanisms because neither covers the other's cases. beforeunload handles a reload, a tab close, and navigation to another origin, none of which the router ever sees. useBlocker handles in-app navigation and the back button, which never reach beforeunload. It settles the blocker with window.confirm rather than the app's own dialog. The blocker has to resolve synchronously against the pending navigation, and an awaited dialog lets the route change through before the answer arrives. The in-app dialog stays where it can be awaited safely, such as the discard button. This is why the app now mounts createBrowserRouter instead of BrowserRouter: useBlocker throws outside a data router. The route table, the basename, and the RequireAuth and PublicAuthRoute guards are unchanged. AuthProvider, SessionExpiryHandler, and the error boundary moved into a root route element, since they have to sit inside the router but outside the routes, and the router itself is created once at module scope. SystemConfig's tests now render through a memory data router for the same reason. Their assertions are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #138.
This is the issue I deferred out of #207, because doing it properly needs a router change and I did not want that buried in a nine-issue PR.
The problem
The configuration screen stages every edit, across every section, into one draft behind a single Save. There was no unload handler and no in-app navigation guard, so navigating away, using the browser back button, or refreshing discarded all of it silently. The sticky bar reported unsaved changes but did nothing to protect them.
Two mechanisms
Neither covers the other's cases, so the guard uses both:
beforeunload— reload, tab close, navigation to another origin. The router never sees these.useBlocker— in-app navigation and the back button. These never reachbeforeunload.The blocker settles with
window.confirmrather than the app's ownConfirmProviderdialog. The blocker has to resolve synchronously against the pending navigation, and an awaited dialog lets the route change through before the answer arrives. The in-app dialog stays where it can be awaited safely, such as the Discard button added in #207.The router change
useBlockerthrows outside a data router, and the app mounted<BrowserRouter>+<Routes>. SoApp.tsxnow builds acreateBrowserRouterroute table.What is deliberately unchanged: every path, the
basenamefrom runtime config,RequireAuth,PublicAuthRoute, the not-found catch-all inside the shell, and the error boundary keyed on the pathname.What moved:
AuthProvider,SessionExpiryHandler, andErrorBoundaryare now a root route element. They have to sit inside the router but outside the routes, and the router is created once at module scope, so they cannot wrapRouterProvider.main.tsxgets simpler: theBrowserRouterand its basename move intoApp.SystemConfig's tests render throughcreateMemoryRouterfor the same reason the app needs a data router. Their assertions are unchanged.Checks
npm run lint,npm run typecheck,npm run format:check,npm test(403 tests), andnpm run buildall pass. I ran the build as well as the tests here, since the router change affects app bootstrap and there are no App-level tests to catch a wiring mistake.Four new tests: navigation allowed when nothing is staged, the operator kept on the screen when they decline, navigation proceeding when they accept, and
beforeunloadmarking a reload as needing confirmation only while dirty.Worth a careful look
This is the one PR in this batch that changes app bootstrap rather than a screen. The behaviour should be identical, but it is the change I would most want a second pair of eyes on, particularly the
/consolebasename build (npm run build:console) against a real deployment.