GRE-298: protect Sources autosaves during browser navigation - #87
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
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.
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.
Bugbot couldn't run - usage limit reachedBugbot 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) |


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
beforeunloadwarnings 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=1to/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’suseBlocker(enabled by migrating the app shell fromBrowserRouterto a data router inmain.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
flushSavesbefore 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.