Skip to content

GRE-298: protect Sources autosaves during browser navigation - #87

Merged
poitee merged 4 commits into
codex/audit-autosavefrom
codex/gre-298-back-navigation
Sep 26, 2026
Merged

poitee merged 4 commits into
codex/audit-autosavefrom
codex/gre-298-back-navigation

Conversation

@poitee

@poitee poitee commented Sep 24, 2026 •

Copy link
Copy Markdown
Owner

Browser Back could leave Sources while an import-rule or kit-selection save was still pending. If that save failed, the editor unmounted and its retry control disappeared. Switching Builds through ?profile= could replace the editor without changing the route path.

Use a React Router data router so one navigation blocker covers Back, Forward, links, and programmatic route changes. It waits for the existing autosave registries before leaving Sources or Plan, and keeps the current editor open when a save fails. The Build picker and bundle import also wait before changing the selected Build directly. The existing beforeunload warnings remain for document exits.

This PR is stacked on #83, which introduces the autosave registries and unload warnings. Review against codex/audit-autosave; merge #83 first.

Validation: the regression failed before the guard because Back immediately moved to /builds; it now passes for Back, Forward, links, programmatic navigation, failed-save retry, and same-route profile changes. The full web suite passed (261 files, 1,385 tests), as did the web production build, typecheck, and changed-file lint. A combined app-shell test proves that a Sources link currently invokes the registry flush twice but sends one autosave API write. In an isolated local app, a browser navigated from /builds?profile=1 to /sources?profile=1, then Back and Forward, with no page errors. Pending-save browser behavior is covered by the data-router integration test; no printer operation was run.


Note

Medium Risk
Changes core routing (data router) and navigation/save ordering for Sources and Plan; incorrect blocking or flush behavior could strand users or lose edits, though behavior is heavily tested.

Overview
Adds BuildSaveNavigationGuard, which uses React Router’s useBlocker (enabled by migrating the app shell from BrowserRouter to a data router in main.tsx) to pause leaving Sources or Plan until pending import-rule and kit-manifest saves finish. Failed saves cancel navigation and show a toast so editors and retry UI stay mounted.

PlanPicker and shared kit import now flushSaves before changing the selected Build (?profile= or route). Autosave hooks keep a stable registered flush when save callbacks change so navigation blockers don’t call stale handlers.

Broad regression coverage: Back/Forward, links, programmatic nav, failed-save retry, same-route profile switches, and coalesced double-flush to one API write.

Reviewed by Cursor Bugbot for commit bc0a799. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 19a44019-7e9d-485d-913f-f099740a2b35

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Build switch loses navigation destination
    • activatePlan and import now navigate before updating selection so useProfileUrlSync cannot replace the blocked destination.
  • ✅ Fixed: Import flush runs after upload
    • flushSaves now runs before pick/upload so a failed save aborts the import instead of orphaning a created plan.

Create PR

Or push these changes by commenting:

@cursor push f5ec039e64
Preview (f5ec039e64)
diff --git a/web/apps/web/src/components/PlanPicker.tsx b/web/apps/web/src/components/PlanPicker.tsx
--- a/web/apps/web/src/components/PlanPicker.tsx
+++ b/web/apps/web/src/components/PlanPicker.tsx
@@ -166,7 +166,7 @@
     setActionTargetId(null);
   };
 
-  const selectAfterSaving = async (id: number): Promise<boolean> => {
+  const flushEditorIfNeeded = async (id: number): Promise<boolean> => {
     if (
       id !== selectedProfileId &&
       (isSourcesPath(location.pathname) || isPlanPath(location.pathname))
@@ -178,14 +178,23 @@
         return false;
       }
     }
+    return true;
+  };
+
+  const selectAfterSaving = async (id: number): Promise<boolean> => {
+    if (!(await flushEditorIfNeeded(id))) return false;
     setSelectedProfileId(id);
     touchMutation.mutate(id);
     return true;
   };
 
   const activatePlan = async (id: number): Promise<boolean> => {
-    if (!(await selectAfterSaving(id))) return false;
+    if (!(await flushEditorIfNeeded(id))) return false;
+    // Navigate before updating selection. setSelectedProfileId → useProfileUrlSync
+    // would setSearchParams while useBlocker holds this transition and replace the
+    // pending destination (stay on Plan, or drop location.state).
     navigate(buildRoute(id), { replace: true });
+    touchMutation.mutate(id);
     return true;
   };
 

diff --git a/web/apps/web/src/hooks/useImportSharedBuild.ts b/web/apps/web/src/hooks/useImportSharedBuild.ts
--- a/web/apps/web/src/hooks/useImportSharedBuild.ts
+++ b/web/apps/web/src/hooks/useImportSharedBuild.ts
@@ -13,9 +13,19 @@
   const navigate = useNavigate();
   const location = useLocation();
   const flushSaves = useFlushBuildPageSaves();
-  const { setSelectedProfileId, reloadProfiles } = useProfileSelection();
+  const { reloadProfiles } = useProfileSelection();
 
   return useCallback(async () => {
+    // Persist the open editor before pick/upload so a failed save aborts the
+    // import instead of orphaning a plan that already exists on the server.
+    if (isSourcesPath(location.pathname) || isPlanPath(location.pathname)) {
+      try {
+        await flushSaves();
+      } catch (e) {
+        toast.error(e instanceof Error ? e.message : String(e));
+        return;
+      }
+    }
     const picked = await pickKitBundle();
     if (!picked) {
       toast.message("Import cancelled");
@@ -27,19 +37,18 @@
         toast.error("Import did not create a plan");
         return;
       }
-      if (isSourcesPath(location.pathname) || isPlanPath(location.pathname)) {
-        await flushSaves();
-      }
       stashKitImportResult(result);
-      setSelectedProfileId(result.profile_id);
+      // Load the new plan before navigating so URL → selection reconcile can
+      // apply ?profile= without a prior setSelectedProfileId (which would race
+      // the save blocker via useProfileUrlSync setSearchParams).
+      await reloadProfiles();
       navigate(buildRoute(result.profile_id), {
         replace: true,
         state: { kitImport: result },
       });
-      void reloadProfiles();
       toast.success(`Imported “${result.profile_name}”`);
     } catch (e) {
       toast.error(e instanceof Error ? e.message : String(e));
     }
-  }, [flushSaves, location.pathname, navigate, reloadProfiles, setSelectedProfileId]);
+  }, [flushSaves, location.pathname, navigate, reloadProfiles]);
 }

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit 2217c99. Configure here.

Comment thread web/apps/web/src/components/PlanPicker.tsx
Comment thread web/apps/web/src/hooks/useImportSharedBuild.ts
@cursor

cursor Bot commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_edc12cab-a7bc-41ba-80bd-272b9b811261)

@poitee
poitee merged commit 3229c43 into codex/audit-autosave Sep 26, 2026
6 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