fix(ux): spamming cmd + , no longer stack opening settings#2757
fix(ux): spamming cmd + , no longer stack opening settings#2757jamesx0416 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAppSidebarLayout component now tracks the current pathname using ChangesSettings navigation history management
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ac55c27. Configure here.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Approved Simple UX fix that adds conditional You can customize Macroscope's approvability policy. Learn more. |
jamesx0416
force-pushed
the
t3code/01790fbd
branch
from
ac55c27 to
4ec0d32
Compare
macroscopeapp
Bot
dismissed
their stale review
Dismissing prior approval to re-evaluate 4ec0d32
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/components/AppSidebarLayout.tsx`:
- Line 50: The replace decision uses pathnameRef.current.startsWith("/settings")
which incorrectly matches paths like "/settings-foo"; update the condition used
when calling navigate({ to: "/settings", replace: ... }) to perform a
boundary-safe check (e.g. test that pathnameRef.current is exactly "/settings"
or matches the "/settings" segment boundary such as using a regex like
/^\/settings(\/|$)/ or checking startsWith("/settings/")) so only true settings
routes trigger replace; locate the call in AppSidebarLayout where navigate and
pathnameRef.current are used and replace the startsWith check with the
boundary-safe check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 77a90cc7-57b7-4e7a-9b9f-651716d83758
📒 Files selected for processing (1)
apps/web/src/components/AppSidebarLayout.tsx
| const unsubscribe = onMenuAction((action) => { | ||
| if (action === "open-settings") { | ||
| void navigate({ to: "/settings" }); | ||
| void navigate({ to: "/settings", replace: pathnameRef.current.startsWith("/settings") }); |
There was a problem hiding this comment.
Use a boundary-safe settings route check before replace.
Line 50 uses startsWith("/settings"), which also matches paths like /settings-foo. That can replace history unexpectedly outside the settings section.
Proposed fix
- void navigate({ to: "/settings", replace: pathnameRef.current.startsWith("/settings") });
+ const isInSettings =
+ pathnameRef.current === "/settings" ||
+ pathnameRef.current.startsWith("/settings/");
+ void navigate({ to: "/settings", replace: isInSettings });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void navigate({ to: "/settings", replace: pathnameRef.current.startsWith("/settings") }); | |
| const isInSettings = | |
| pathnameRef.current === "/settings" || | |
| pathnameRef.current.startsWith("/settings/"); | |
| void navigate({ to: "/settings", replace: isInSettings }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/components/AppSidebarLayout.tsx` at line 50, The replace
decision uses pathnameRef.current.startsWith("/settings") which incorrectly
matches paths like "/settings-foo"; update the condition used when calling
navigate({ to: "/settings", replace: ... }) to perform a boundary-safe check
(e.g. test that pathnameRef.current is exactly "/settings" or matches the
"/settings" segment boundary such as using a regex like /^\/settings(\/|$)/ or
checking startsWith("/settings/")) so only true settings routes trigger replace;
locate the call in AppSidebarLayout where navigate and pathnameRef.current are
used and replace the startsWith check with the boundary-safe check.
1 participant
What Changed
Replaced repeated settings-route pushes from the desktop menu action when the current route is already under
/settings.Why
Pressing Cmd+, or using the macOS Settings menu item while already in settings could push duplicate settings entries onto browser history. That forced users to press Escape or Back multiple times to leave settings. Replacing the current history entry for repeated settings opens keeps the escape/back path to a single step.
UI Changes
No visual UI changes. This only changes settings navigation history behavior.
Before:
Screen.Recording.2026-05-19.at.5.17.24.pm.mp4
After:
Screen.Recording.2026-05-19.at.5.18.13.pm.mp4
Checklist
Note
Low Risk
Low risk UI navigation tweak: only changes the
open-settingsdesktop menu action to usereplacewhen already on a/settingsroute, reducing duplicate history entries.Overview
Prevents the desktop menu
open-settingsaction from repeatedly pushing/settingsonto browser history.AppSidebarLayoutnow tracks the currentpathnameviauseLocationand conditionally callsnavigatewithreplace: truewhen the user is already under/settings, keeping back/escape behavior to a single step.Reviewed by Cursor Bugbot for commit 4ec0d32. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Replace history entry instead of stacking when navigating to settings
In AppSidebarLayout.tsx, the
open-settingsmenu action now callsnavigatewithreplace: truewhen the current pathname already starts with/settings, preventing duplicate history entries from accumulating. AuseReftracks the current pathname inside the callback to avoid stale closure issues.Macroscope summarized 4ec0d32.
Summary by CodeRabbit