Repository navigation
fix(sidebar): use dedicated setting for port link browser preference - #2219
Conversation
Port links were reusing the PR-link preference (openSidebarPullRequestLinksInCmuxBrowser), causing inconsistent behavior when users toggled that setting. Adds a dedicated openSidebarPortLinksInCmuxBrowser setting with its own toggle in Settings so port and PR link behavior can be controlled independently. Addresses review feedback from #1844
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces a new configurable setting that allows users to control whether sidebar port links open in the cmux browser. The implementation spans localization strings, settings infrastructure, UI controls, and link-opening behavior routing. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR fixes a long-standing bug where Confidence Score: 5/5Safe to merge — the core bug fix is correct and all integration points (model, binding, UI, reset, localisation) are complete. The change is self-contained: one wrong settings key swapped for the right one, backed by a properly defined constant and accessor. All call sites (ContentView, SettingsView, reset-defaults) are updated consistently. No regressions to existing PR-link behaviour. The only open item is a P2 toggle-ordering nit that has no functional consequence. No files require special attention; Sources/cmuxApp.swift has the minor placement concern noted in the inline comment. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User clicks port in sidebar] --> B[openPortLink called]
B --> C{openSidebarPortLinksInCmuxBrowser?}
C -- true --> D[tabManager.openBrowser]
D -- success --> E[Opens in cmux browser pane]
D -- nil / failure --> F[NSWorkspace.shared.open]
F --> G[Opens in system browser]
C -- false --> G
H[User clicks PR link in sidebar] --> I[openPullRequestLink called]
I --> J{openSidebarPullRequestLinksInCmuxBrowser?}
J -- true --> K[tabManager.openBrowser]
K -- success --> L[Opens in cmux browser pane]
K -- nil --> M[NSWorkspace.shared.open]
M --> N[Opens in system browser]
J -- false --> N
subgraph Settings
S1[openSidebarPortLinksInCmuxBrowser toggle]
S2[openSidebarPullRequestLinksInCmuxBrowser toggle]
end
S1 -.controls.-> C
S2 -.controls.-> J
Reviews (1): Last reviewed commit: "fix(sidebar): use dedicated setting for ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| SettingsCardRow( | ||
| String(localized: "settings.app.openSidebarPortLinks", defaultValue: "Open Sidebar Port Links in cmux Browser"), | ||
| subtitle: openSidebarPortLinksInCmuxBrowser | ||
| ? String(localized: "settings.app.openSidebarPortLinks.subtitleOn", defaultValue: "Port clicks open inside cmux browser.") | ||
| : String(localized: "settings.app.openSidebarPortLinks.subtitleOff", defaultValue: "Port clicks open in your default browser.") | ||
| ) { | ||
| Toggle("", isOn: $openSidebarPortLinksInCmuxBrowser) | ||
| .labelsHidden() | ||
| .controlSize(.small) | ||
| } | ||
| .disabled(sidebarHideAllDetails) |
There was a problem hiding this comment.
The existing PR links behavior toggle (openSidebarPRLinks) is placed immediately after the showPullRequest visibility toggle — behavior follows visibility. The new port links toggle is placed in the PR links block rather than adjacent to showPorts, so the order ends up:
openSidebarPRLinks ← behavior for PR
openSidebarPortLinks ← behavior for port [NEW]
showSSH
showPorts ← visibility for port (3 rows later)
For consistency, consider moving openSidebarPortLinks to sit directly after showPorts (mirroring the PR pattern where openSidebarPRLinks sits directly after showPullRequest). This also makes the UX more discoverable — a user enabling port forwarding visibility will find the link-behavior toggle right below it.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Panels/BrowserPanel.swift">
<violation number="1" location="Sources/Panels/BrowserPanel.swift:556">
P2: Migrate the new port-link preference from the old PR-link setting when the new key is absent, otherwise existing users get port links forced back to the default.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if defaults.object(forKey: openSidebarPortLinksInCmuxBrowserKey) == nil { | ||
| return defaultOpenSidebarPortLinksInCmuxBrowser | ||
| } | ||
| return defaults.bool(forKey: openSidebarPortLinksInCmuxBrowserKey) |
There was a problem hiding this comment.
P2: Migrate the new port-link preference from the old PR-link setting when the new key is absent, otherwise existing users get port links forced back to the default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/BrowserPanel.swift, line 556:
<comment>Migrate the new port-link preference from the old PR-link setting when the new key is absent, otherwise existing users get port links forced back to the default.</comment>
<file context>
@@ -549,6 +552,13 @@ enum BrowserLinkOpenSettings {
}
+ static func openSidebarPortLinksInCmuxBrowser(defaults: UserDefaults = .standard) -> Bool {
+ if defaults.object(forKey: openSidebarPortLinksInCmuxBrowserKey) == nil {
+ return defaultOpenSidebarPortLinksInCmuxBrowser
+ }
</file context>
| if defaults.object(forKey: openSidebarPortLinksInCmuxBrowserKey) == nil { | |
| return defaultOpenSidebarPortLinksInCmuxBrowser | |
| } | |
| return defaults.bool(forKey: openSidebarPortLinksInCmuxBrowserKey) | |
| if defaults.object(forKey: openSidebarPortLinksInCmuxBrowserKey) != nil { | |
| return defaults.bool(forKey: openSidebarPortLinksInCmuxBrowserKey) | |
| } | |
| if defaults.object(forKey: openSidebarPullRequestLinksInCmuxBrowserKey) != nil { | |
| return defaults.bool(forKey: openSidebarPullRequestLinksInCmuxBrowserKey) | |
| } | |
| return defaultOpenSidebarPortLinksInCmuxBrowser |
…anaflow-ai#2219) Port links were reusing the PR-link preference (openSidebarPullRequestLinksInCmuxBrowser), causing inconsistent behavior when users toggled that setting. Adds a dedicated openSidebarPortLinksInCmuxBrowser setting with its own toggle in Settings so port and PR link behavior can be controlled independently. Addresses review feedback from manaflow-ai#1844 Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This branch was successfully deployed
1 participant
Summary
openSidebarPortLinksInCmuxBrowsersetting so port links and PR links can be controlled independently in SettingsopenPortLinkwas reusingopenSidebarPullRequestLinksInCmuxBrowser, meaning toggling the PR-link preference also affected port-click behavior (identified by Greptile, CodeRabbit, and cubic in feat(sidebar): make listening ports clickable to open in browser #1844)Test plan
Summary by CodeRabbit
Summary by cubic
Port links now have their own browser preference, separate from PR links. This fixes unexpected behavior and lets users choose where port clicks open without affecting PR links.
New Features
openSidebarPortLinksInCmuxBrowserwith a Settings toggle and localized strings for 16 languages.Bug Fixes
openSidebarPullRequestLinksInCmuxBrowser; default remains opening in the cmux browser unless toggled.Written for commit 10d7fe6. Summary will update on new commits.