Polish sidebar extensions and titlebar chrome#5017
Conversation
|
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:
📝 WalkthroughWalkthroughSessionPersistencePolicy sidebar minimums lowered to 159. Titlebar accessory layout refactored to use a new visibleContentWidth helper and traffic-light metrics adjusted. Minimal-mode hit-region outer leading padding reduced to 0. CMUX createTerminalSurface signature removed initialInput; host helpers, tests, ContentView extension action URL validation and workspace/surface guards, and responsive sidebar host views updated. ChangesSidebar & Titlebar sizing and hit-region changes
CMUX extension actions and ContentView handlers
Sequence Diagram(s)sequenceDiagram
participant Client
participant ContentView
participant CmuxSidebarHost
participant SessionPersistencePolicy
Client->>ContentView: send extension action (createTerminal/createBrowser/openURL)
ContentView->>ContentView: validate URL / lookup workspace
ContentView->>CmuxSidebarHost: dispatch CMUXSidebarAction (createTerminalSurface/createBrowserSurface)
CmuxSidebarHost->>SessionPersistencePolicy: read minimumSidebarWidth for layout decisions
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 2 warnings)
✅ Passed checks (12 passed)
✨ 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 tightens the ExtensionKit sidebar integration: it enforces per-action permission scopes for URL-bearing browser actions, removes the terminal
Confidence Score: 5/5All changes are well-scoped and safe to merge; the security improvements are correctly implemented and thoroughly tested. The multi-scope enforcement for URL-bearing browser actions is implemented consistently at both the client session layer and the XPC host dispatch layer, with dedicated unit tests for each. The titlebar decoupling removes fragile imperative sync calls in favor of a declarative SwiftUI overlay — simpler than what it replaces with updated tests pinning the new constants. The reload.sh artifact cleanup correctly leverages set -euo pipefail and the INTERRUPTED log scan closes the gap noted in the PR description. No files require special attention. Important Files Changed
Reviews (14): Last reviewed commit: "Scope workspace titlebar overlay to chro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let contentSize = TitlebarControlsLayoutMetrics.contentSize(config: style.config) | ||
| let visibleContentWidth = TitlebarControlsLayoutMetrics.visibleContentWidth(config: style.config) |
There was a problem hiding this comment.
visibleContentWidth is computed twice per updateSize() call — once inside contentSize(config:) (which calls visibleContentWidth internally) and once again as a separate local. Since this is not a hot path the impact is negligible, but the redundancy is easy to eliminate by computing visibleContentWidth first and threading it into contentSize.
| let contentSize = TitlebarControlsLayoutMetrics.contentSize(config: style.config) | |
| let visibleContentWidth = TitlebarControlsLayoutMetrics.visibleContentWidth(config: style.config) | |
| let visibleContentWidth = TitlebarControlsLayoutMetrics.visibleContentWidth(config: style.config) | |
| let contentSize = TitlebarControlsLayoutMetrics.contentSize(config: style.config) |
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
cmuxTests/SidebarWidthPolicyTests.swift (1)
11-114: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffConsider converting to Swift Testing when touching XCTest tests.
Per coding guidelines, when touching an existing XCTest test, convert in place:
XCTestCasesubclass becomes@Suite struct; test methods become@Testfunctions;XCTAssertEqual(a, b)becomes#expect(a == b).While the changes here are minimal (updating 3 numeric literals), the guideline is clear about converting when touching XCTest tests. However, given that this PR only updates literal values to match constant changes, a full framework conversion would significantly expand scope.
Consider either:
- Converting just the two touched test methods to Swift Testing (if feasible with partial migration), or
- Deferring the conversion to a separate refactoring effort focused on test framework migration.
As per coding guidelines: "When touching an existing XCTest test, convert in place:
XCTestCasesubclass becomes@Suite struct;func testFoo()becomes@Test func foo();XCTAssertEqual(a, b)becomes#expect(a == b)".🤖 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 `@cmuxTests/SidebarWidthPolicyTests.swift` around lines 11 - 114, The test file still uses XCTest; convert the XCTest-based SidebarWidthPolicyTests to Swift Testing by changing the XCTestCase subclass (SidebarWidthPolicyTests) into an `@Suite` struct, convert each test method (e.g., testDefaultMinimumSidebarWidthIsPersistedProductDefault, testContentViewClampKeepsMinimumSidebarWidth, testContentViewClampCanUseSmallerConfiguredMinimumSidebarWidth, etc.) into `@Test` functions, and replace XCTest assertions: XCTAssertEqual(...) -> `#expect`(... == ...), XCTAssertTrue(...) -> `#expect`(...), and XCTAssertFalse(...) -> `#expect`(!( ... )). Update any setup/teardown or UserDefaults usage to work inside the `@Test` functions as needed so the semantics remain identical.
🤖 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.
Outside diff comments:
In `@cmuxTests/SidebarWidthPolicyTests.swift`:
- Around line 11-114: The test file still uses XCTest; convert the XCTest-based
SidebarWidthPolicyTests to Swift Testing by changing the XCTestCase subclass
(SidebarWidthPolicyTests) into an `@Suite` struct, convert each test method (e.g.,
testDefaultMinimumSidebarWidthIsPersistedProductDefault,
testContentViewClampKeepsMinimumSidebarWidth,
testContentViewClampCanUseSmallerConfiguredMinimumSidebarWidth, etc.) into `@Test`
functions, and replace XCTest assertions: XCTAssertEqual(...) -> `#expect`(... ==
...), XCTAssertTrue(...) -> `#expect`(...), and XCTAssertFalse(...) -> `#expect`(!(
... )). Update any setup/teardown or UserDefaults usage to work inside the `@Test`
functions as needed so the semantics remain identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1fba00f-e966-4f4c-ac89-1cacc84df2bd
📒 Files selected for processing (3)
Sources/SessionPersistence.swiftSources/Update/UpdateTitlebarAccessory.swiftcmuxTests/SidebarWidthPolicyTests.swift
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cmuxTests/SidebarWidthPolicyTests.swift`:
- Around line 19-24: The tests in SidebarWidthPolicyTests.swift still use
XCTestCase and XCTAssert-style APIs; convert the XCTestCase subclass
(SidebarWidthPolicyTests) into an in-place `@Suite` struct, change each func
test...() into `@Test` func <name>() and replace XCTAssertEqual / other XCTest
assertions with Swift Testing expectations (e.g. XCTAssertEqual(...) ->
`#expect`(actual).toEqual(expected) or try `#require`(...) where appropriate);
update imports if needed (remove XCTest, add import SwiftTesting) and ensure any
setup/teardown methods are converted to `@Before/`@After or equivalent Swift
Testing hooks.
In `@Sources/ContentView.swift`:
- Around line 11133-11140: The function cmuxSidebarExtensionRequiredHTTPURL
currently accepts opaque HTTP(S) strings because it only checks the scheme;
change it to parse the URL into URLComponents (e.g., URLComponents(url: url,
resolvingAgainstBaseURL: false)) and require a non-empty host
(components.host?.isEmpty == false) before returning the URL; otherwise return
nil—this ensures opaque URLs like "https:example.com" are rejected.
In `@Sources/SessionPersistence.swift`:
- Around line 18-19: The constants defaultMinimumSidebarWidth and
minimumSidebarWidth in SessionPersistence.swift are currently set to 171 but
should reflect the PR objective of a 180px minimum; update both static let
defaultMinimumSidebarWidth and static let minimumSidebarWidth to 180 so the
default and enforced minimum match the intended 180 value.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a60b4c4c-b9b7-4719-88b9-5fbbcc675adc
📒 Files selected for processing (6)
Packages/CmuxExtensionKit/Sources/CmuxExtensionKit/Sidebar/CMUXSidebarAction.swiftPackages/CmuxExtensionKit/Sources/CmuxExtensionKit/Sidebar/CmuxSidebarHost.swiftPackages/CmuxExtensionKit/Tests/CmuxExtensionKitTests/CmuxExtensionKitTests.swiftSources/ContentView.swiftSources/SessionPersistence.swiftcmuxTests/SidebarWidthPolicyTests.swift
| 171, | ||
| accuracy: 0.001 | ||
| ) | ||
| XCTAssertEqual( | ||
| SessionPersistencePolicy.resolvedMinimumSidebarWidth(defaults: defaults), | ||
| 190, | ||
| 171, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Convert touched XCTest tests to Swift Testing per repo policy.
This file is modified but still uses XCTestCase. The repo rule requires in-place migration when touching existing XCTest tests (unless UI tests).
As per coding guidelines: “When touching an existing XCTest test, convert in place: XCTestCase subclass becomes @Suite struct; func testFoo() becomes @Test func foo(); XCTAssert... becomes #expect(...) / try #require(...).”
Also applies to: 31-31
🤖 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 `@cmuxTests/SidebarWidthPolicyTests.swift` around lines 19 - 24, The tests in
SidebarWidthPolicyTests.swift still use XCTestCase and XCTAssert-style APIs;
convert the XCTestCase subclass (SidebarWidthPolicyTests) into an in-place
`@Suite` struct, change each func test...() into `@Test` func <name>() and replace
XCTAssertEqual / other XCTest assertions with Swift Testing expectations (e.g.
XCTAssertEqual(...) -> `#expect`(actual).toEqual(expected) or try `#require`(...)
where appropriate); update imports if needed (remove XCTest, add import
SwiftTesting) and ensure any setup/teardown methods are converted to
`@Before/`@After or equivalent Swift Testing hooks.
| static let defaultMinimumSidebarWidth: Double = 171 | ||
| static let minimumSidebarWidth: Double = 171 |
There was a problem hiding this comment.
Minimum width constant does not match the stated product target.
Lines 18–19 set the minimum to 171, but this PR’s objective says the default minimum should be reduced to 180 (from 190). This changes behavior more than intended and will clamp narrower than specified.
Proposed fix
- static let defaultMinimumSidebarWidth: Double = 171
- static let minimumSidebarWidth: Double = 171
+ static let defaultMinimumSidebarWidth: Double = 180
+ static let minimumSidebarWidth: Double = 180🤖 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 `@Sources/SessionPersistence.swift` around lines 18 - 19, The constants
defaultMinimumSidebarWidth and minimumSidebarWidth in SessionPersistence.swift
are currently set to 171 but should reflect the PR objective of a 180px minimum;
update both static let defaultMinimumSidebarWidth and static let
minimumSidebarWidth to 180 so the default and enforced minimum match the
intended 180 value.
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 `@Sources/CMUXInstalledExtensionSidebarHostView.swift`:
- Around line 705-709: The limitedAccessActionButtons function currently accepts
identity: AppExtensionIdentity? but then calls
keepLimitedAccess(identity:effectiveGrant:) which requires a non-optional
AppExtensionIdentity; fix by either changing limitedAccessActionButtons
signature to accept a non-optional AppExtensionIdentity (AppExtensionIdentity)
and update all call sites, or safely unwrap the optional inside
limitedAccessActionButtons (e.g., guard let identity = identity else {
disable/hide the “Keep Limited” button or return } ) and then call
keepLimitedAccess(identity: identity, effectiveGrant: effectiveGrant); ensure
only the non-optional identity is passed to
keepLimitedAccess(identity:effectiveGrant:).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23bbdaca-68a5-4b22-b87f-4744d71fc3ce
📒 Files selected for processing (3)
Examples/SampleSidebarExtensionApp/SampleSidebarExtension/Model/SidebarInsightModel.swiftSources/CMUXInstalledExtensionSidebarHostView.swiftSources/ContentView.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/SessionPersistence.swift (1)
18-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSidebar minimum constants are below the PR’s stated product target.
Line 18 and Line 19 set the persisted/default floor to
159, but this PR’s objective says the product minimum should be171 pt. That will clamp sidebars narrower than intended and makes the updated tests codify the wrong floor.Proposed fix
- static let defaultMinimumSidebarWidth: Double = 159 - static let minimumSidebarWidth: Double = 159 + static let defaultMinimumSidebarWidth: Double = 171 + static let minimumSidebarWidth: Double = 171🤖 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 `@Sources/SessionPersistence.swift` around lines 18 - 19, The persisted/default minimum sidebar width constants in SessionPersistence.swift are still set to 159; update both static let defaultMinimumSidebarWidth and static let minimumSidebarWidth to 171 (the PR’s specified product minimum) so the app and tests use the correct 171 pt floor when clamping sidebar widths.
🤖 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.
Duplicate comments:
In `@Sources/SessionPersistence.swift`:
- Around line 18-19: The persisted/default minimum sidebar width constants in
SessionPersistence.swift are still set to 159; update both static let
defaultMinimumSidebarWidth and static let minimumSidebarWidth to 171 (the PR’s
specified product minimum) so the app and tests use the correct 171 pt floor
when clamping sidebar widths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b343bbe-56e6-454e-a94d-8d86cc583256
📒 Files selected for processing (5)
Sources/CMUXInstalledExtensionSidebarHostView.swiftSources/SessionPersistence.swiftSources/Update/UpdateTitlebarAccessory.swiftcmuxTests/SidebarWidthPolicyTests.swiftcmuxTests/UpdatePillReleaseVisibilityTests.swift
azooz2003-bit
changed the title
azooz2003-bit
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
Dismissed per maintainer instruction: CodeRabbit feedback is non-blocking for this PR after CI and dogfood approval.
1 participant
Summary
Verification
git diff --checkjq empty Resources/Localizable.xcstringsswift test --package-path Packages/CmuxExtensionKitswift test --package-path Packages/CMUXExtensionClientxcodebuild -project Examples/SampleSidebarExtensionApp/SampleSidebarExtensionApp.xcodeproj -scheme CMUXExtKitSampleSidebarApp -configuration Debug -derivedDataPath /tmp/cmux-pr5017-sample CODE_SIGNING_ALLOWED=NO build./scripts/reload.sh --tag reload-build-guard --swift-frontend-workaroundreload-build-guardand screenshot-verified the titlebar/title/sidebar placementDogfood
Open the tagged build and keep the left sidebar visible. The accessory controls should stay next to the traffic lights, the workspace title should sit in the titlebar band immediately to the right of the sidebar divider, and enabled sidebar extensions should still render through ExtensionKit with their permission review flow.