-
Notifications
You must be signed in to change notification settings - Fork 177
fix(project): prevent duplicate confirm modal when creating new proje… #683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ct (CircuitVerse#399) - add optional skipConfirm parameter to clearProject() - set projectSaved = true before window.location.assign() in newProject() - add unit test to assert confirmOption is called only once
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThree variants of project.ts have been updated to modify the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
v1/src/simulator/src/data/project.ts (2)
141-153: SameskipConfirmbehavior as v0 — looks goodThis matches the v0 implementation; see the v0 comment on
clearProjectfor details. No additional issues here.
158-170: SamenewProjectflow as v0 — considerawait clearProject(true)This is identical to the v0
newProjectchanges; the same suggestion about awaitingclearProject(true)for future‑proofing applies.
🧹 Nitpick comments (3)
v0/src/simulator/src/data/project.ts (1)
158-170: Consider awaitingclearProject(true)for clarity/future‑proofingRight now
clearProject(true)executes synchronously on this path, so not awaiting it is functionally safe. However, sinceclearProjectisasync, a future change that addsawaitto itsskipConfirmpath could introduce subtle ordering bugs (e.g., if more async cleanup is added).I’d recommend:
- clearProject(true) + await clearProject(true)to make the intent explicit and robust to future changes.
src/simulator/src/data/project.ts (2)
123-132: onbeforeunload no longer warns users about unsaved changesChanging
onbeforeunloadto always returnundefinedremoves both the explicitalertand the browser’s native “Leave site?” prompt. Unsaved data is still written tolocalStoragefor recovery, but users now leave the page without any warning if recovery fails or is unexpected.If the goal was only to avoid extra dialogs when starting a new project, this is a broader UX change that affects all navigation paths (tab close, URL change, back/forward, etc.). Please confirm this is intentional; otherwise you may want to keep a lightweight warning here or gate this behavior specifically for the new‑project flow (where you already set
projectSaved = trueand clearonbeforeunload).
136-164: clearProjectskipConfirmand newProject flow look correct; consider awaitingThe
skipConfirmparameter and its usage innewProjectbehave as intended:newProjectnow shows a single confirmation and then callsclearProject(true)without triggering a second dialog, while also clearingrecover, markingprojectSaved = true, and disablingonbeforeunloadbefore navigating.As in v0/v1, I’d recommend:
- clearProject(true) + await clearProject(true)in
newProject(Line 160) to keep the order of operations robust ifclearProjectlater gains additional async work in theskipConfirmpath.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/simulator/src/data/project.ts(2 hunks)v0/src/simulator/src/data/project.ts(2 hunks)v1/src/simulator/src/data/project.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v1/src/simulator/src/data/project.ts (2)
src/simulator/src/data/project.ts (1)
clearProject(138-146)v0/src/simulator/src/data/project.ts (1)
clearProject(145-153)
🔇 Additional comments (1)
v0/src/simulator/src/data/project.ts (1)
141-153: clearProjectskipConfirmbehavior looks correctThe
skipConfirmparameter with short‑circuiting (skipConfirm || await confirmOption(...)) cleanly preserves existing behavior while allowing callers (likenewProject) to bypass the dialog. No functional issues spotted here.

Fix: Prevent Duplicate Confirmation Popup When Creating New Project (#399)
resolves the issue where users were seeing two confirmation dialogs when creating a new project.
Summary of Fixes
Files Updated
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.