Repository navigation
Fix inline VS Code command palette and WebSocket startup - #5595
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds web-content-first routing for Inline VS Code command-palette shortcuts (Cmd/Cmd+Shift) when the focused page matches the serve-web origin, and refactors VSCode CLI launch configuration to prefer a cached code-server binary from serve-web caches with updated environment/arguments and tests. ChangesInline VS Code Command Palette Web-Content-First Routing
Cached code-server launch selection
Sequence DiagramsequenceDiagram
participant User
participant CmuxWebView
participant ShortcutRoutingSupport
participant VSCodeServeWebController
participant WebKit
User->>CmuxWebView: Key press (Cmd/Cmd+Shift)
CmuxWebView->>ShortcutRoutingSupport: shouldRouteInlineVSCodeCommandPaletteShortcutThroughWebContentFirst(event,pageURL)
ShortcutRoutingSupport->>VSCodeServeWebController: isServeWebURL(pageURL)
VSCodeServeWebController->>VSCodeServeWebController: urlsShareLoopbackOrigin(pageURL,serveWebURL)
VSCodeServeWebController-->>ShortcutRoutingSupport: true
ShortcutRoutingSupport-->>CmuxWebView: policy match (true)
CmuxWebView->>WebKit: super.performKeyEquivalent / super.keyDown
WebKit-->>User: Command palette handled in page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 15❌ Failed checks (1 warning, 14 inconclusive)
✅ 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 |
![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 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 659bd5a. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 659bd5ac05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if shouldRouteInlineVSCodeCommandPaletteShortcutThroughWebContentFirst(event, pageURL: url) { | ||
| _ = super.performKeyEquivalent(with: event) | ||
| return finish(true) |
There was a problem hiding this comment.
When an inline VS Code page receives Cmd+Shift+P but WKWebView.performKeyEquivalent returns false, this branch still returns true, so AppKit will not continue to the later keyDown path that was added to forward the shortcut to WebKit. In that case the event is swallowed before either VS Code's DOM key handler or cmux's fallback can handle it; this matters for WebKit paths where arbitrary page shortcuts are delivered via keyDown rather than claimed as key equivalents. Preserve the super.performKeyEquivalent result or explicitly forward keyDown before marking the event handled.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes two independent regressions in inline VS Code: WebSocket startup failures caused by the
Confidence Score: 5/5Safe to merge. The changes are well-scoped, fully injectable for testing, and cover both the launch and shortcut routing paths with dedicated tests. Both changed behaviours — VS Code binary selection and command palette shortcut routing — are exercised by new tests using injected dependencies. The shortcut routing is layered defensively across AppDelegate and both CmuxWebView hook points. The cached binary discovery gracefully falls back through lru.json → mtime scan → code-tunnel, and all path components are validated before use. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant CmuxWebView
participant VSCodeServeWebController
participant WebKit
Note over User,WebKit: Cmd+Shift+P while VS Code serve-web page is focused
User->>AppDelegate: sendEvent (Cmd+Shift+P)
AppDelegate->>VSCodeServeWebController: isServeWebURL(pageURL)
VSCodeServeWebController-->>AppDelegate: true
AppDelegate-->>AppDelegate: return false (don't consume)
AppDelegate->>CmuxWebView: performKeyEquivalent
CmuxWebView->>VSCodeServeWebController: isServeWebURL(url)
VSCodeServeWebController-->>CmuxWebView: true
CmuxWebView->>WebKit: super.performKeyEquivalent
WebKit-->>CmuxWebView: handled
CmuxWebView-->>AppDelegate: finish(true)
Reviews (5): Last reviewed commit: "Keep inline VS Code availability cheap" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func isServeWebURL(_ candidateURL: URL?) -> Bool { | ||
| guard let candidateURL else { return false } | ||
| let serveWebURL = queue.sync { | ||
| self.serveWebURL | ||
| } | ||
| return Self.urlsShareLoopbackOrigin(candidateURL, serveWebURL) | ||
| } |
There was a problem hiding this comment.
queue.sync on keyboard-event hot path
isServeWebURL is called on every keyboard shortcut evaluation from the main thread (via CmuxWebView.performKeyEquivalent and keyDown). The queue.sync blocks the main thread until the serial queue can service the read, turning every keystroke into a main-thread contention point. While the critical section is tiny, this pattern goes against the project's rule of avoiding new blocking synchronization in production Swift. The existing class already tracks serveWebURL behind a serial queue; an actor-isolated property (or a @MainActor-cached snapshot updated asynchronously when serveWebURL changes) would expose the same information without blocking the event-dispatch thread.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
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.
Actionable comments posted: 2
🤖 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/App/TerminalDirectoryOpenSupport.swift`:
- Line 729: Add an inline comment immediately above the guard that reads "guard
lhs.port == rhs.port, lhs.port != nil else { return false }" in
TerminalDirectoryOpenSupport.swift explaining that we intentionally require
explicit, non-nil ports (rather than treating absent ports as default 80/443)
because this check targets VS Code serve-web instances which bind to ephemeral
explicit ports (e.g., 54321), so treating missing ports as defaults would be
incorrect for our use case.
- Around line 547-553: Add a doc comment for the new public method
isServeWebURL(_:) explaining its purpose: it checks whether the supplied URL
matches the origin of the currently running VS Code serve-web instance (i.e.,
compares loopback origin against the stored serveWebURL using
urlsShareLoopbackOrigin). Place the comment immediately above the
isServeWebURL(_:) declaration, describe parameters and return value (returns
true when the candidate URL shares the loopback origin with serveWebURL, false
otherwise), and note that nil candidateURL returns false.
🪄 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: 4befe689-6731-43d7-9e2a-fe178b7cde89
📒 Files selected for processing (5)
Sources/App/ShortcutRoutingSupport.swiftSources/App/TerminalDirectoryOpenSupport.swiftSources/AppDelegate.swiftSources/Panels/CmuxWebView.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
| func isServeWebURL(_ candidateURL: URL?) -> Bool { | ||
| guard let candidateURL else { return false } | ||
| let serveWebURL = queue.sync { | ||
| self.serveWebURL | ||
| } | ||
| return Self.urlsShareLoopbackOrigin(candidateURL, serveWebURL) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Add documentation for the new public method.
The isServeWebURL(_:) method is a new public API but lacks a doc comment explaining its purpose and behavior. A brief comment would clarify that it checks whether a given URL matches the origin of the currently-running VS Code serve-web instance.
📝 Suggested documentation
+ /// Returns `true` if the candidate URL shares the same loopback origin as the
+ /// currently running VS Code serve-web instance. Origin matching requires both
+ /// URLs to be HTTP with the same explicit port on loopback addresses.
+ ///
+ /// - Parameter candidateURL: The URL to test, typically a browser's current page URL.
+ /// - Returns: `true` if the candidate matches the serve-web origin, `false` otherwise.
func isServeWebURL(_ candidateURL: URL?) -> Bool {🤖 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/App/TerminalDirectoryOpenSupport.swift` around lines 547 - 553, Add a
doc comment for the new public method isServeWebURL(_:) explaining its purpose:
it checks whether the supplied URL matches the origin of the currently running
VS Code serve-web instance (i.e., compares loopback origin against the stored
serveWebURL using urlsShareLoopbackOrigin). Place the comment immediately above
the isServeWebURL(_:) declaration, describe parameters and return value (returns
true when the candidate URL shares the loopback origin with serveWebURL, false
otherwise), and note that nil candidateURL returns false.
| rhs.scheme?.lowercased() == "http" else { | ||
| return false | ||
| } | ||
| guard lhs.port == rhs.port, lhs.port != nil else { return false } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Document the strict port requirement.
The port check requires both URLs to have explicit non-nil ports. This is stricter than typical origin comparison (which treats http://host/ as equivalent to http://host:80/). While this is appropriate for VS Code serve-web (which always binds to an ephemeral port like 54321), an inline comment would clarify why we require explicit ports rather than allowing default-port URLs.
📝 Suggested clarifying comment
+ // Require both URLs to have explicit non-nil ports. VS Code serve-web always
+ // binds to an ephemeral port (e.g., :54321), so both the stored serve-web URL
+ // and any matching browser navigation will have explicit ports. This strict
+ // check avoids false matches with default-port (implicit :80) localhost URLs.
guard lhs.port == rhs.port, lhs.port != nil else { return false }🤖 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/App/TerminalDirectoryOpenSupport.swift` at line 729, Add an inline
comment immediately above the guard that reads "guard lhs.port == rhs.port,
lhs.port != nil else { return false }" in TerminalDirectoryOpenSupport.swift
explaining that we intentionally require explicit, non-nil ports (rather than
treating absent ports as default 80/443) because this check targets VS Code
serve-web instances which bind to ephemeral explicit ports (e.g., 54321), so
treating missing ports as defaults would be incorrect for our use case.
lawrencecchen
changed the title
91cc595 to
bfd5319
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfd531914d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| process.arguments = launchConfiguration.argumentsPrefix + [ | ||
| "serve-web", | ||
| "--accept-server-license-terms", | ||
| "--host", "127.0.0.1", | ||
| "--port", "0", |
There was a problem hiding this comment.
When the cached code-server path is selected, argumentsPrefix is empty and this shared list is the only place server flags are added. The PR summary says the handshake fix depends on --disable-websocket-compression, but neither the cached code-server nor the code-tunnel serve-web fallback receives that flag here, so the Management/ExtensionHost WebSockets can still negotiate compression and fail in the inline WKWebView path this change is meant to fix.
Useful? React with 👍 / 👎.
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/App/TerminalDirectoryOpenSupport.swift`:
- Around line 356-363: The availability check is performing heavy file I/O via
preferredCachedCodeServerURL during TerminalDirectoryOpenTarget.isAvailable
(which still calls launchConfiguration(...)); remove synchronous cache discovery
from the hot UI path by ensuring preferredCachedCodeServerURL is not called from
isAvailable: either (a) make isAvailable return quickly without invoking
preferredCachedCodeServerURL and let launchConfiguration/prepareLaunch
asynchronously query preferredCachedCodeServerURL off the main/UI queue, or (b)
change preferredCachedCodeServerURL to early-return nil when called on the UI
thread and schedule its full disk work on a background queue, then have
launchConfiguration observe the async result; update references in
TerminalDirectoryOpenSupport.swift (the calls around
preferredCachedCodeServerURL and the launchConfiguration invocation) accordingly
so all heavy file I/O (enumerating serve-web cache, reading
product.json/lru.json, stat-ing bin/code-server) happens off the UI isolation
path.
🪄 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: bff73396-5608-497f-8224-6e54edd1c5cb
📒 Files selected for processing (2)
Sources/App/TerminalDirectoryOpenSupport.swiftcmuxTests/OmnibarAndToolsTests.swift
lawrencecchen
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9794961afd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isExecutableAtPath(codeServerURL.path) { | ||
| return codeServerURL |
There was a problem hiding this comment.
For users whose ~/.vscode/cli/serve-web cache contains an executable server from a different VS Code build (for example after code serve-web auto-downloaded a newer server while the installed app is still older), this returns the first LRU entry without checking that the cache id matches the current VS Code app commit. A mismatched direct code-server can still print the Web UI URL and then fail during startup, so launchServeWebProcess treats the launch as successful and never falls back to the wrapper path that would resolve the correct server.
Useful? React with 👍 / 👎.
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/AppDelegate.swift`:
- Around line 14514-14517: Pre-compute a Set of current tab IDs to avoid the
nested linear scan: create tabIds from tabManager.tabs (e.g., using map(\.id)
into a Set) and then redefine eligibleIds to filter candidateIds by checking
tabIds.contains(id) and !existingAnchorIds.contains(id); update the logic that
currently uses tabManager.tabs.contains(where:) so it uses tabIds.contains
instead.
🪄 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: b18e66f9-9365-4bbb-9d87-cadbbaa87eb6
📒 Files selected for processing (1)
Sources/AppDelegate.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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/AppDelegate.swift`:
- Around line 14514-14517: Pre-compute a Set of current tab IDs to avoid the
nested linear scan: create tabIds from tabManager.tabs (e.g., using map(\.id)
into a Set) and then redefine eligibleIds to filter candidateIds by checking
tabIds.contains(id) and !existingAnchorIds.contains(id); update the logic that
currently uses tabManager.tabs.contains(where:) so it uses tabIds.contains
instead.
🪄 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: b18e66f9-9365-4bbb-9d87-cadbbaa87eb6
📒 Files selected for processing (1)
Sources/AppDelegate.swift
🛑 Comments failed to post (1)
Sources/AppDelegate.swift (1)
14514-14517: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider pre-computing tab IDs as a Set to avoid O(n×m) complexity.
The filter performs a linear
contains(where:)scan oftabManager.tabsfor each candidate ID. While the practical impact is minimal given thatcandidateIdsis typically small (user-selected workspaces), the pattern can be optimized:let tabIds = Set(tabManager.tabs.map(\.id)) let eligibleIds = candidateIds.filter { id in tabIds.contains(id) && !existingAnchorIds.contains(id) }This reduces complexity from O(n×m) to O(n+m).
🤖 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/AppDelegate.swift` around lines 14514 - 14517, Pre-compute a Set of current tab IDs to avoid the nested linear scan: create tabIds from tabManager.tabs (e.g., using map(\.id) into a Set) and then redefine eligibleIds to filter candidateIds by checking tabIds.contains(id) and !existingAnchorIds.contains(id); update the logic that currently uses tabManager.tabs.contains(where:) so it uses tabIds.contains instead.Source: Coding guidelines
This branch was successfully deployed
1 participant
Summary
code-serverbinary for inline VS Code startup; keepcode-tunnel serve-webas the cache-miss fallback.Root cause
code-serverworks in WKWebView with normal WebSocket negotiation.code-tunnel serve-webwrapper. In verbose mode it logsserver (upgrade expected but low level API in use) websocket upgrade failed; the browser sees the WebSocket open, sends the first VS Code auth frame, then gets a 1006 close and later reports the remote-connection handshake timeout.code-serverbypasses that wrapper bridge and lets the Management and ExtensionHost sockets complete.Testing
git diff --check HEAD~4..HEAD./scripts/ensure-ghosttykit.sh && xcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-cvix-rootcause test -only-testing:cmuxTests/VSCodeCLILaunchConfigurationBuilderTests -only-testing:cmuxTests/VSCodeServeWebURLBuilderTests -only-testing:cmuxTests/AppDelegateShortcutRoutingTests/testInlineVSCodeCommandPaletteShortcutRoutesThroughWebContentForTrackedServeWebOrigin -only-testing:cmuxTests/AppDelegateShortcutRoutingTests/testInlineVSCodeCommandPaletteShortcutDoesNotRouteForUntrackedLocalhostPage -only-testing:cmuxTests/AppDelegateShortcutRoutingTests/testInlineVSCodeCommandPaletteShortcutDoesNotRouteUnrelatedShortcut./scripts/reload-cloud.sh --tag cvrxcvrxWKWebView verification onsurface:2: direct cachedcode-serverloaded/Users/lawrence/fun; Management and ExtensionHost sockets connected;browser errors listreturnedNo browser errors.Regression commits
Note
Medium Risk
Changes inline VS Code process launch and global Cmd+Shift+P routing; origin matching limits palette regressions but WebSocket/startup behavior still depends on the new binary selection path.
Overview
Fixes inline VS Code by changing how serve-web is launched and how Cmd+Shift+P is routed.
Launch:
VSCodeCLILaunchConfigurationBuildernow prefers VS Code’s cachedcode-serverbinary under the user data folder’scli/serve-webcache (honoringlru.json, then newest executable), and only falls back tocode-tunnelwith aserve-webargument prefix when no cache hit. Directcode-serverruns withoutELECTRON_RUN_AS_NODE; the tunnel wrapper still sets it.VSCodeServeWebControllerbuilds process arguments from that prefix instead of always embeddingserve-web. Inline availability checks only thatcode-tunnelexists (no cache scan on menu/palette).Shortcuts: When the focused browser URL matches the tracked serve-web loopback origin (
VSCodeServeWebController.isServeWebURL), the configured command-palette shortcut is forwarded to WebKit inCmuxWebViewand not handled by cmux’s app-level palette inAppDelegate. Other localhost pages and non-palette shortcuts are unchanged.Tests cover launch binary selection and shortcut routing boundaries.
Reviewed by Cursor Bugbot for commit 7e40494. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Tests