Repository navigation
Split sidebar PR visibility from clickability - #3273
Merged
Merged
Conversation
Issue #3265 needs a red test that captures the current behavior before the sidebar row implementation changes. This UI test seeds a pull request over the control socket, disables the future clickability default, and asserts that a click should only select the workspace row without opening a browser surface. Constraint: Local test execution is disabled for this workspace; CI must prove the failure Rejected: Source-shape assertion against ContentView | does not verify runtime behavior Confidence: medium Scope-risk: narrow Reversibility: clean Directive: Keep this test behavior-focused; do not replace it with a source-text check Tested: Not run locally (per repo policy) Not-tested: Compile/runtime status until CI runs
Issue #3265 needs the sidebar pull-request row to split visibility from interactivity. This keeps PR metadata visible while allowing users to turn the link behavior off, preserving row selection and leaving the link-destination preference intact for when clickability is re-enabled. Constraint: The existing show/hide setting and managed settings.json surface must keep working Rejected: Reuse the browser-destination toggle as the clickability toggle | conflates visibility/interactivity and cannot preserve plain-text rows Confidence: medium Scope-risk: moderate Reversibility: clean Directive: Keep sidebar PR row interactivity separate from link-destination preferences Tested: JSON parse validation for Resources/Localizable.xcstrings and web/data/cmux-settings.schema.json Not-tested: Swift compilation/UI behavior until the final tagged reload build
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds a new "Make Sidebar PR Clickable" persisted setting (default: off) and wires it through storage, UI, localization, view snapshot, rendering, tests, and settings templates so sidebar PR review items can be shown as plain non-interactive text or as clickable links. ChangesSidebar PR Clickability Feature
Sequence DiagramsequenceDiagram
participant User
participant Settings as SettingsView
participant Store as UserDefaults
participant View as ContentView/TabItemView
participant PR as Sidebar PR Row
User->>Settings: Toggle "Make Sidebar PR Clickable"
Settings->>Store: Persist sidebarMakePullRequestClickable
Store-->>Settings: Acknowledgement
View->>Store: Read SidebarPullRequestClickabilitySettings.isClickable()
Store-->>View: Return boolean
View->>View: Update makesPullRequestsClickable
alt makesPullRequestsClickable = true
View->>PR: Render as Button (underlined, tooltip)
User->>PR: Click PR title -> open PR URL (external/browser)
else makesPullRequestsClickable = false
View->>PR: Render as plain text (non-interactive)
User->>PR: Click row -> select workspace (no browser surface)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Possibly related PRs
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 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 |
Contributor
Greptile SummaryThis PR introduces a new Confidence Score: 4/5Safe to merge; only P2 findings (dead-code ternary in the test helper and an orphaned localization key). No P0 or P1 findings. The two P2 issues — a dead-code ternary in waitForCurrentWorkspace and the orphaned settings.app.showPullRequests.subtitle localization key — are cosmetic and do not affect runtime behaviour or test correctness. cmuxUITests/SidebarPullRequestInteractivityUITests.swift (dead-code ternary) and Resources/Localizable.xcstrings (orphaned key). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SidebarTabItemSettingsSnapshot\ninit] -->|reads UserDefaults| B[SidebarPullRequestClickabilitySettings\n.isClickable]
B --> C{makesPullRequestsClickable?}
C -->|true| D[pullRequestRowView\nrenders Button\n+ underline + tooltip]
C -->|false| E[pullRequestRowView\nrenders plain HStack\nno tooltip, no button]
D -->|click| F[openPullRequestLink\nopens URL]
E -->|click| G[falls through to\nworkspace row selection]
H[SettingsView toggle\nsidebarMakePullRequestClickable] -->|persists| I[UserDefaults\nsidebarMakePullRequestClickable]
I --> B
J[settings.json\nsidebar.makePullRequestsClickable] -->|CmuxSettingsFileStore\nparses JSON bool| I
K[openSidebarPRLinks toggle] -->|disabled when| L{!sidebarMakePullRequestClickable\nOR !sidebarShowPullRequest}
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
cmuxUITests/SidebarPullRequestInteractivityUITests.swift (1)
137-144: Simplify redundant conditional return in workspace wait helper.
matched ? current : currentalways returns the same value, which makes thematchedvariable misleading.♻️ Suggested cleanup
private func waitForCurrentWorkspace(_ expectedWorkspaceId: String, timeout: TimeInterval) -> String? { var current: String? - let matched = pollUntil(timeout: timeout) { + _ = pollUntil(timeout: timeout) { current = socketCommand("current_workspace")?.trimmingCharacters(in: .whitespacesAndNewlines) return current == expectedWorkspaceId } - return matched ? current : current + return current }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/SidebarPullRequestInteractivityUITests.swift` around lines 137 - 144, The helper waitForCurrentWorkspace contains a redundant conditional return (matched ? current : current); remove the matched variable and simplify the logic by calling pollUntil to set current (or ignore its Bool result) and then return current directly. Update the waitForCurrentWorkspace function so it only declares var current, invokes pollUntil(...) to update current using socketCommand("current_workspace"), and returns current, eliminating the misleading matched Bool entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 5454-5471: The subtitle logic in
openSidebarPullRequestLinksSubtitle should early-return a message when
sidebarShowPullRequest is false so the disabled state reflects PR visibility
being off; update openSidebarPullRequestLinksSubtitle to first check
!sidebarShowPullRequest and return a localized "PR visibility is off" subtitle,
then fall through to the existing checks for sidebarMakePullRequestClickable and
openSidebarPullRequestLinksInCmuxBrowser to preserve current messages.
---
Nitpick comments:
In `@cmuxUITests/SidebarPullRequestInteractivityUITests.swift`:
- Around line 137-144: The helper waitForCurrentWorkspace contains a redundant
conditional return (matched ? current : current); remove the matched variable
and simplify the logic by calling pollUntil to set current (or ignore its Bool
result) and then return current directly. Update the waitForCurrentWorkspace
function so it only declares var current, invokes pollUntil(...) to update
current using socketCommand("current_workspace"), and returns current,
eliminating the misleading matched Bool entirely.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 145a1783-5bb6-4d91-af28-fb4cdee967c7
📒 Files selected for processing (7)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/TabManager.swiftSources/cmuxApp.swiftcmuxUITests/SidebarPullRequestInteractivityUITests.swiftweb/data/cmux-settings.schema.json
The sidebar can keep PR status visible without making the small PR fragment an accidental browser trigger. The existing split setting already provides a single clickability source of truth, so the change flips that default, updates generated schema/copy mirrors, and covers the default plain-text path. Constraint: Build and launch must use ./scripts/reload.sh with the issue tag; no direct xcodebuild invocation. Rejected: Add renderer-specific click guards | the PR row already has one Button-versus-plain-text seam driven by SidebarPullRequestClickabilitySettings. Confidence: high Scope-risk: narrow Directive: Keep SidebarPullRequestClickabilitySettings as the single default/resolution owner for sidebar PR clickability. Tested: jq empty Resources/Localizable.xcstrings; jq empty web/data/cmux-settings.schema.json; git diff --check; ./scripts/reload.sh --tag issue-3265-pr-clickable-toggle --launch Not-tested: Local XCTest/XCUITest suites per repository instruction to avoid local test runs.
Merged origin/main into the clickable pull-request toggle branch and resolved the two conflicts by preserving configurable PR row clickability while adopting the new canonical cmux schema split. Constraint: git pull origin main produced content conflicts in ContentView.swift and cmux-settings.schema.json Rejected: Keep the old full cmux-settings schema | origin/main moved settings validation to cmux.schema.json with a compatibility wrapper Confidence: medium Scope-risk: moderate Directive: Keep sidebar.makePullRequestsClickable in cmux.schema.json because cmux-settings.schema.json now only delegates to it Not-tested: Build verification pending after reapplying pre-pull local changes
The main sync introduced new CLI file references that reused the same PBX object identifiers as SidebarPullRequestInteractivityUITests. Xcode then interpreted a UI test file reference as a build phase entry and refused to open the project. Assigning fresh IDs to the UI test reference keeps both sides of the merge. Constraint: Xcode project object identifiers must be globally unique inside project.pbxproj Rejected: Remove SidebarPullRequestInteractivityUITests from the project | the test remains part of the cmuxUITests target and only its conflicting IDs were invalid Confidence: high Scope-risk: narrow Directive: When manually merging project.pbxproj, check duplicate PBX object identifiers before assuming plutil validity is enough Tested: ./scripts/reload.sh --tag issue-3265-pr-clickable-toggle Not-tested: Local test suites, per repo policy
The PR merge commit now includes accepted Swift file growth from this branch plus newer main. The CI guard failed because the checked-in budget still reflected the pre-merge line counts, so regenerate the budget from the current tree and keep the reductions for files that shrank. Constraint: workflow-guard-tests blocks PRs when any tracked Swift file exceeds .github/swift-file-length-budget.tsv Rejected: Split unrelated large Swift files in this conflict-fix branch | broad refactor would be riskier than accepting the already-merged debt Confidence: high Scope-risk: narrow Directive: Reduce these counts when follow-up cleanup shrinks the large files Tested: ./tests/test_ci_swift_file_length_budget.sh; python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Not-tested: Full local test suite, per repo policy
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Sources/cmuxApp.swift (1)
5356-5373:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSubtitle can still be misleading when PR visibility is off.
At Line 6406 the row is disabled when
!sidebarShowPullRequest, butopenSidebarPullRequestLinksSubtitlenever checks that state first, so users can see a clickability/browser subtitle that doesn’t match the disabled reason.Suggested fix
private var openSidebarPullRequestLinksSubtitle: String { + if !sidebarShowPullRequest { + return String( + localized: "settings.app.openSidebarPRLinks.subtitleHidden", + defaultValue: "Enable sidebar PR visibility to choose where PR links open." + ) + } if !sidebarMakePullRequestClickable { return String( localized: "settings.app.openSidebarPRLinks.subtitleDisabled", defaultValue: "Enable sidebar PR clickability to choose where PR links open." ) }Also applies to: 6406-6406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 5356 - 5373, openSidebarPullRequestLinksSubtitle can show clickability/browser text even when pull requests are hidden; update the getter to first check sidebarShowPullRequest and return a subtitle explaining PRs are hidden (or stating the feature is disabled for visibility) before evaluating sidebarMakePullRequestClickable and openSidebarPullRequestLinksInCmuxBrowser so the subtitle matches the disabled row state; locate the openSidebarPullRequestLinksSubtitle computed property and add an initial branch that returns the appropriate localized "PRs hidden" message when sidebarShowPullRequest is false.
🧹 Nitpick comments (2)
Sources/ContentView.swift (2)
14135-14142: ⚡ Quick winSimplify duplicated
Text(...)with.underline(_:).The
Group { if underlinesTitle { … } else { … } }block repeats the identicalTextconstructor. You can supply aBoolvariable to.underline()to control whether the underline is active, collapsing both branches into one line. This also shaves ~5 lines, partially helping the budget.♻️ Proposed simplification
- Group { - if underlinesTitle { - Text("\(pullRequest.label) #\(pullRequest.number)") - .underline() - } else { - Text("\(pullRequest.label) #\(pullRequest.number)") - } - } + Text("\(pullRequest.label) #\(pullRequest.number)") + .underline(underlinesTitle)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 14135 - 14142, The Group block duplicates the same Text initializer; replace the conditional branches by a single Text("\(pullRequest.label) #\(pullRequest.number)") and call .underline(underlinesTitle) on it so the underline state is driven by the underlinesTitle Bool (update the Group containing Text, referencing Group, underlinesTitle, Text, pullRequest.label, pullRequest.number, and .underline(_:)).
14135-14142: ⚡ Quick winSimplify duplicated
Text(...)using.underline(_:).The
Group { if underlinesTitle { … .underline() } else { … } }block repeats the sameTextconstructor. SwiftUI's.underline(_:color:)accepts aBool, reducing the branch to one line and saving ~5 lines.♻️ Proposed simplification
- Group { - if underlinesTitle { - Text("\(pullRequest.label) #\(pullRequest.number)") - .underline() - } else { - Text("\(pullRequest.label) #\(pullRequest.number)") - } - } + Text("\(pullRequest.label) #\(pullRequest.number)") + .underline(underlinesTitle)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 14135 - 14142, Replace the duplicated Text construction inside the Group by calling .underline(_:) with the existing boolean; specifically, remove the if/else block that repeats Text("\(pullRequest.label) #\(pullRequest.number)") and instead use a single Text(...) and apply .underline(underlinesTitle) so the view uses the underlinesTitle Bool to toggle the underline (referencing the underlinesTitle variable and the Text that builds from pullRequest.label and pullRequest.number).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 6383-6396: The file exceeds the Swift file-length budget due to
the new sidebar PR settings block; extract that block into a new small SwiftUI
view (e.g., SidebarPullRequestSettingsView) in a dedicated file and replace the
inline SettingsCardRow block with that view to reduce lines. Move the
SettingsCardRow/Toggle/subtitle logic (the SettingsCardRow initializer using
.json("sidebar.makePullRequestsClickable"), the localized subtitle and title
strings, the Toggle bound to sidebarMakePullRequestClickable, and the
.accessibilityIdentifier) into the new view, accept bindings for
sidebarMakePullRequestClickable, sidebarHideAllDetails, and
sidebarShowPullRequest, and ensure the original call site uses the new view and
preserves the .disabled(sidebarHideAllDetails || !sidebarShowPullRequest)
behavior and the trailing SettingsCardDivider.
In `@Sources/ContentView.swift`:
- Around line 14103-14152: Extract pullRequestRowView and pullRequestRowContent
into a new SwiftUI view type in a new file (e.g.,
SidebarPullRequestRowView.swift): create a value-driven struct (e.g.,
SidebarPullRequestRowView: View) that takes a
SidebarWorkspaceSnapshotBuilder.PullRequestDisplay and any needed
environment/props (pullRequestForegroundColor, a closure for
openPullRequestLink, and boolean for sidebarPullRequestsAreClickable) and
renders the same UI using PullRequestStatusIcon and calling
pullRequestStatusLabel for the status text; preserve the
Button-safeHelp/accessibilityIdentifier behavior when
sidebarPullRequestsAreClickable is true, and keep the same
font/opacity/truncation behavior. Replace the original
pullRequestRowView/pullRequestRowContent definitions in ContentView.swift with a
single line instantiation of the new SidebarPullRequestRowView passing the
necessary symbols (pullRequest, pullRequestForegroundColor, openPullRequestLink,
sidebarPullRequestsAreClickable).
- Around line 14103-14152: ContentView.swift exceeded the file-length budget
because the helper functions pullRequestRowView and pullRequestRowContent were
added inline; extract them into a new file as a standalone, value-driven SwiftUI
View (e.g., SidebarPullRequestRowView) that takes a
SidebarWorkspaceSnapshotBuilder.PullRequestDisplay and the derived values it
needs (pullRequestForegroundColor, pullRequestStatusLabel result, .isStale,
.status, .url, .label, .number, and the sidebarPullRequestsAreClickable flag) so
the view has only value inputs; move usage of PullRequestStatusIcon,
accessibilityIdentifier "SidebarPullRequestRow", safeHelp text, button behavior
(openPullRequestLink) and underline logic into that struct, update ContentView
to call the new SidebarPullRequestRowView with computed values, and delete the
two helper functions from ContentView.swift to bring the file back under budget.
---
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 5356-5373: openSidebarPullRequestLinksSubtitle can show
clickability/browser text even when pull requests are hidden; update the getter
to first check sidebarShowPullRequest and return a subtitle explaining PRs are
hidden (or stating the feature is disabled for visibility) before evaluating
sidebarMakePullRequestClickable and openSidebarPullRequestLinksInCmuxBrowser so
the subtitle matches the disabled row state; locate the
openSidebarPullRequestLinksSubtitle computed property and add an initial branch
that returns the appropriate localized "PRs hidden" message when
sidebarShowPullRequest is false.
---
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 14135-14142: The Group block duplicates the same Text initializer;
replace the conditional branches by a single Text("\(pullRequest.label)
#\(pullRequest.number)") and call .underline(underlinesTitle) on it so the
underline state is driven by the underlinesTitle Bool (update the Group
containing Text, referencing Group, underlinesTitle, Text, pullRequest.label,
pullRequest.number, and .underline(_:)).
- Around line 14135-14142: Replace the duplicated Text construction inside the
Group by calling .underline(_:) with the existing boolean; specifically, remove
the if/else block that repeats Text("\(pullRequest.label)
#\(pullRequest.number)") and instead use a single Text(...) and apply
.underline(underlinesTitle) so the view uses the underlinesTitle Bool to toggle
the underline (referencing the underlinesTitle variable and the Text that builds
from pullRequest.label and pullRequest.number).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2233bc30-7101-499b-866e-75ac904123cc
📒 Files selected for processing (7)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/ContentView.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/TabManager.swiftSources/cmuxApp.swiftcmuxTests/BrowserConfigTests.swift
✅ Files skipped from review due to trivial changes (2)
- cmuxTests/BrowserConfigTests.swift
- Resources/Localizable.xcstrings
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/KeyboardShortcutSettingsFileStore.swift
The previous budget refresh is intentionally undone. The PR clickability changes now stay within the existing Swift file-length budget by removing helper wrappers, sharing the existing BrowserLinkOpenSettings test fixture, and compacting simple settings parsing. Constraint: PR CI must pass without changing .github/swift-file-length-budget.tsv Rejected: Refresh Swift file length budget | user explicitly rejected accepting new line-count debt Confidence: high Scope-risk: narrow Directive: Keep future PR clickability changes within existing budget or split files intentionally Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv; ./tests/test_ci_swift_file_length_budget.sh; ./scripts/reload.sh --tag issue-3265-pr-clickable-toggle Not-tested: Full local test suite, per repo policy
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
13169-13187:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNon-clickable mode still renders PR rows as
Buttoncontrols.Line 13169 keeps the row wrapped in
Button, and Line 13185 only disables hit-testing. That prevents mouse clicks, but it can still present interactive button semantics (e.g., accessibility/keyboard), which conflicts with the intended “plain non-interactive text” mode.💡 Suggested fix (render non-clickable rows as plain content)
- Button(action: { openPullRequestLink(pullRequest.url) }) { - HStack(spacing: 4) { + let rowContent = HStack(spacing: 4) { PullRequestStatusIcon(status: pullRequest.status, color: pullRequestForegroundColor) Text("\(pullRequest.label) #\(pullRequestNumber)") .underline(settings.makesPullRequestsClickable) Text(pullRequestStatusLabel(pullRequest.status)) .lineLimit(1) Spacer(minLength: 0) } .font(.system(size: 10, weight: .semibold)) .foregroundColor(pullRequestForegroundColor) .opacity(pullRequest.isStale ? 0.5 : 1) - } - .buttonStyle(.plain) - .allowsHitTesting(settings.makesPullRequestsClickable) - .accessibilityIdentifier("SidebarPullRequestRow") - .safeHelp(settings.makesPullRequestsClickable ? String(localized: "sidebar.pullRequest.openTooltip", defaultValue: "Open \(pullRequest.label) #\(pullRequestNumber)") : "") + + Group { + if settings.makesPullRequestsClickable { + Button(action: { openPullRequestLink(pullRequest.url) }) { + rowContent + } + .buttonStyle(.plain) + .safeHelp(String(localized: "sidebar.pullRequest.openTooltip", defaultValue: "Open \(pullRequest.label) #\(pullRequestNumber)")) + } else { + rowContent + } + } + .accessibilityIdentifier("SidebarPullRequestRow")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 13169 - 13187, The PR row is always created as a Button and only has hit-testing disabled, which still exposes interactive semantics; change the rendering to branch on settings.makesPullRequestsClickable so that when clickable is true you create the existing Button(action: openPullRequestLink...) with the HStack content, and when clickable is false you render the same HStack as plain non-interactive content (no Button, no .buttonStyle or .allowsHitTesting), preserving modifiers like .font, .foregroundColor, .opacity, .accessibilityIdentifier("SidebarPullRequestRow") and .safeHelp (use the same tooltip string or empty string), and keep inner elements PullRequestStatusIcon, Text("\(pullRequest.label) #\(pullRequestNumber)"), and Text(pullRequestStatusLabel(...)) so the UI looks identical but has no interactive semantics when settings.makesPullRequestsClickable is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 13169-13187: The PR row is always created as a Button and only has
hit-testing disabled, which still exposes interactive semantics; change the
rendering to branch on settings.makesPullRequestsClickable so that when
clickable is true you create the existing Button(action: openPullRequestLink...)
with the HStack content, and when clickable is false you render the same HStack
as plain non-interactive content (no Button, no .buttonStyle or
.allowsHitTesting), preserving modifiers like .font, .foregroundColor, .opacity,
.accessibilityIdentifier("SidebarPullRequestRow") and .safeHelp (use the same
tooltip string or empty string), and keep inner elements PullRequestStatusIcon,
Text("\(pullRequest.label) #\(pullRequestNumber)"), and
Text(pullRequestStatusLabel(...)) so the UI looks identical but has no
interactive semantics when settings.makesPullRequestsClickable is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0601f752-7d48-4a51-991f-46e503c2544d
📒 Files selected for processing (4)
Sources/ContentView.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/cmuxApp.swiftcmuxTests/BrowserConfigTests.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/cmuxApp.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/BrowserConfigTests.swift
The row now switches between a real button and plain content instead of disabling hit testing on a button. Settings copy also distinguishes hidden PR visibility from disabled clickability, and the UI test helper reports timeout failure as nil. Constraint: Address PR review feedback without changing .github/swift-file-length-budget.tsv Rejected: Keep disabled-hit-test Button semantics | still exposed interactive accessibility behavior in non-clickable mode Confidence: high Scope-risk: narrow Directive: Non-clickable sidebar PR rows must remain plain content, not disabled or hit-test-suppressed controls Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv; ./tests/test_ci_swift_file_length_budget.sh; jq empty Resources/Localizable.xcstrings; ./scripts/reload.sh --tag issue-3265-pr-clickable-toggle Not-tested: Full local test suite, per repo policy
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 f5dffb2. Configure here.
| "value": "ステータスと番号付きのレビュー項目（PR/MRなど）を表示します。" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused localization key duplicates existing subtitle string
Low Severity
The new localization key "settings.app.showPullRequests.subtitle.basic" is added to Localizable.xcstrings but is never referenced anywhere in the Swift source code. Its English and Japanese values are identical to the already-used "settings.app.showPullRequests.subtitle" key. This is dead localization data that will confuse future contributors about whether the .basic variant is consumed elsewhere.
Reviewed by Cursor Bugbot for commit f5dffb2. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 55986-56001: The two localization keys
settings.app.showPullRequests.subtitle.basic and
settings.app.showPullRequests.subtitle currently contain identical EN and JA
strings; decide which behavior you intend and update accordingly: if they
represent distinct UI states (non-clickable vs clickable), change
settings.app.showPullRequests.subtitle.basic to copy that communicates the
non-interactive state (e.g., “Review items are shown as plain, non-clickable
text.”) in both languages; otherwise remove the unused key so only the active
key (settings.app.showPullRequests.subtitle) remains to avoid translator drift,
and ensure any UI code referencing either key (search for references to
settings.app.showPullRequests.subtitle.basic and
settings.app.showPullRequests.subtitle) is updated to use the chosen key.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b03b77e-c501-44cd-931c-82b6c25c9ff2
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/cmuxApp.swiftcmuxUITests/SidebarPullRequestInteractivityUITests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/cmuxApp.swift
- cmuxUITests/SidebarPullRequestInteractivityUITests.swift
Comment on lines
+55986
to
+56001
| "settings.app.showPullRequests.subtitle.basic": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Display review items (PR/MR/etc.) with status and number." | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "ステータスと番号付きのレビュー項目（PR/MRなど）を表示します。" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
settings.app.showPullRequests.subtitle.basic has identical content to the updated settings.app.showPullRequests.subtitle — clarify intent.
Both keys now share the same EN value ("Display review items (PR/MR/etc.) with status and number.") and the same JA value. Two outcomes are possible:
- Distinct UI states — if
.basicis rendered when clickability is off and the parent key is rendered when clickability is on, the identical text gives users no indication that anything differs between the two states. The.basicsubtitle should carry different copy that reflects non-interactive display (e.g., "Review items are shown as plain, non-clickable text."). - Same UI state — if only one of the two keys is actually used in the final render path, the other is dead string content. The used key should be kept and the other removed to avoid translator drift (future updates to one key may not be reflected in the other).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resources/Localizable.xcstrings` around lines 55986 - 56001, The two
localization keys settings.app.showPullRequests.subtitle.basic and
settings.app.showPullRequests.subtitle currently contain identical EN and JA
strings; decide which behavior you intend and update accordingly: if they
represent distinct UI states (non-clickable vs clickable), change
settings.app.showPullRequests.subtitle.basic to copy that communicates the
non-interactive state (e.g., “Review items are shown as plain, non-clickable
text.”) in both languages; otherwise remove the unused key so only the active
key (settings.app.showPullRequests.subtitle) remains to avoid translator drift,
and ensure any UI code referencing either key (search for references to
settings.app.showPullRequests.subtitle.basic and
settings.app.showPullRequests.subtitle) is updated to use the chosen key.
This was referenced May 4, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Default behavior update
New installs now show the PR badge/text in the sidebar, but the PR fragment is not clickable by default. Users who want PR links to open from the sidebar can enable Make Sidebar PR Clickable in Settings; this keeps the information visible while avoiding accidental browser navigation when aiming at the workspace row.
Testing
Resources/Localizable.xcstringsandweb/data/cmux-settings.schema.jsongit diff --check./scripts/reload.sh --tag issue-3265-pr-clickable-toggle --launchCloses #3265
Note
Low Risk
Low risk UI/settings change that alters how sidebar PR rows handle clicks; main risk is unintended interaction regressions or mismatched defaults in managed config/localizations.
Overview
Sidebar pull request display is split from interactivity: PR metadata can remain visible while click-to-open is now controlled by a new persisted setting (default off).
When clickability is disabled, sidebar PR rows render as plain text (no implicit link behavior) so clicks select the workspace row; when enabled, rows become
Buttons that open the PR URL and show the tooltip/underline.Settings/config plumbing is updated end-to-end: new toggle in Settings UI,
KeyboardShortcutSettingsFileStore+ JSON schema support, updated localizations/subtitles (including disabling the “open PR links in cmux browser” toggle unless PRs are shown and clickable), plus new unit/UI tests and Xcode project wiring for the added UI test.Reviewed by Cursor Bugbot for commit f5dffb2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Split sidebar PR visibility from clickability to reduce accidental navigations (closes #3265). PR metadata stays visible as plain text by default; enabling clickability turns rows into buttons that open links per your browser preference.
New Features
SidebarPullRequestClickabilitySettingspersisted as@AppStoragekeysidebarMakePullRequestClickable(default: false) with a new Settings toggle; “Open Sidebar PR Links in cmux Browser” is disabled unless PRs are shown and clickable, with clear subtitles when hidden or non-clickable; updated “Show Pull Requests” copy to only promise status/number.SidebarPullRequestRowaccessibility identifier.sidebar.makePullRequestsClickabletoweb/data/cmux.schema.jsonand wired throughKeyboardShortcutSettingsFileStore; tests includeSidebarPullRequestInteractivityUITestsplus unit coverage for default/stored clickability.Refactors
Written for commit f5dffb2. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization
Tests