Skip to content

fix(ux): spamming cmd + , no longer stack opening settings#2757

Open
jamesx0416 wants to merge 3 commits into
pingdotgg:mainfrom
jamesx0416:t3code/01790fbd
Open

fix(ux): spamming cmd + , no longer stack opening settings#2757
jamesx0416 wants to merge 3 commits into
pingdotgg:mainfrom
jamesx0416:t3code/01790fbd

Conversation

@jamesx0416

@jamesx0416 jamesx0416 commented May 19, 2026

Copy link
Copy Markdown
Contributor

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

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Note

Low Risk
Small navigation-only change in the desktop menu handler with no auth, data, or API impact.

Overview
Fixes desktop Settings / Cmd+, navigation so repeated opens while already in settings no longer push duplicate browser history entries.

AppSidebarLayout now reads the current pathname (via a ref so the menu callback stays stable) and calls navigate to /settings with replace: true when the active route already matches /settings or a child path. The first jump into settings from elsewhere still uses a normal push.

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

Note

Fix repeated cmd+, keypresses stacking duplicate settings history entries

When navigating to /settings via the menu action in AppSidebarLayout.tsx, the router now uses replace instead of push if the user is already on /settings or a subroute, preventing history stack buildup from repeated keypresses.

Macroscope summarized 916a1b2.

Summary by CodeRabbit

  • Bug Fixes
    • Improved sidebar navigation to prevent creating duplicate history entries when accessing the settings page from within the settings section, enhancing the back button behavior.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

AppSidebarLayout component now tracks the current pathname using useLocation and stores it in a mutable ref to keep it available to event handlers. The /settings navigation from the desktop menu now conditionally uses history replace when the user is already within the /settings route, avoiding redundant history entries.

Changes

Settings navigation history management

Layer / File(s) Summary
Pathname tracking and conditional navigation
apps/web/src/components/AppSidebarLayout.tsx
Imports updated to include useLocation and useRef. Component tracks current pathname and syncs it to pathnameRef on render. /settings menu navigation uses replace: true when pathname already starts with /settings, otherwise performs a normal push.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Poem

🐰 A sidebar so keen, now tracks where it's been,
Keeping history tidy, replace for settings, so nifty!
No redundant bounces, just smooth navigation pounces,
With refs and location hooks, this component now looks
Quite elegant indeed! 🌿✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title directly addresses the main change: preventing duplicate settings history entries when repeatedly opening settings via keyboard shortcut.
Description check ✅ Passed The PR description provides comprehensive coverage of all required template sections with clear explanations of changes, reasoning, and acknowledgment of UI impact.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels May 19, 2026
Comment thread apps/web/src/components/AppSidebarLayout.tsx Outdated
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes May 19, 2026
@macroscopeapp

macroscopeapp Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved

Straightforward UX fix that prevents browser history pollution when repeatedly pressing cmd+, to open settings. Changes are limited to navigation behavior with no broader runtime impact.

You can customize Macroscope's approvability policy. Learn more.

@macroscopeapp macroscopeapp Bot dismissed their stale review May 19, 2026 10:04

Dismissing prior approval to re-evaluate 4ec0d32

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes May 19, 2026
@jamesx0416

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between d1e85c4 and 4ec0d32.

📒 Files selected for processing (1)
  • apps/web/src/components/AppSidebarLayout.tsx

Comment thread apps/web/src/components/AppSidebarLayout.tsx Outdated
@jamesx0416 jamesx0416 changed the title Avoid stacking settings history entries fix(ux): spamming cmd + , no longer stack opening settings May 19, 2026
@macroscopeapp macroscopeapp Bot dismissed their stale review June 22, 2026 00:31

Dismissing prior approval to re-evaluate bc35b12

@github-actions github-actions Bot added size:S 10-29 changed lines (additions + deletions). and removed size:XS 0-9 changed lines (additions + deletions). labels Jun 22, 2026
@github-actions github-actions Bot added size:XS 0-9 changed lines (additions + deletions). and removed size:S 10-29 changed lines (additions + deletions). labels Jun 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant