Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Sources/App/ShortcutRoutingSupport.swift
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,16 @@ func shouldRouteBrowserFindCommandEquivalentThroughWebContentFirst(
return true
}

func shouldRouteInlineVSCodeCommandPaletteShortcutThroughWebContentFirst(
_ event: NSEvent,
pageURL: URL?,
inlineVSCodeURLMatcher: (URL?) -> Bool = { VSCodeServeWebController.shared.isServeWebURL($0) },
shortcutForAction: (KeyboardShortcutSettings.Action) -> StoredShortcut = KeyboardShortcutSettings.shortcut(for:)
) -> Bool {
guard inlineVSCodeURLMatcher(pageURL) else { return false }
return shortcutForAction(.commandPalette).matches(event: event)
}

func cmuxOwningGhosttyView(for responder: NSResponder?) -> GhosttyNSView? {
guard let responder else { return nil }
if let ghosttyView = responder as? GhosttyNSView {
Expand Down
166 changes: 154 additions & 12 deletions Sources/App/TerminalDirectoryOpenSupport.swift
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,11 @@ enum TerminalDirectoryOpenTarget: String, CaseIterable {
func isAvailable(in environment: DetectionEnvironment = .live) -> Bool {
guard let applicationPath = applicationPath(in: environment) else { return false }
guard self == .vscodeInline else { return true }
return VSCodeCLILaunchConfigurationBuilder.launchConfiguration(
vscodeApplicationURL: URL(fileURLWithPath: applicationPath, isDirectory: true),
isExecutableAtPath: environment.isExecutableFileAtPath
) != nil
// Keep menu/palette availability cheap. Cached code-server discovery does
// disk I/O and belongs to the actual launch path on the launch queue.
let codeTunnelURL = URL(fileURLWithPath: applicationPath, isDirectory: true)
.appendingPathComponent("Contents/Resources/app/bin/code-tunnel", isDirectory: false)
return environment.isExecutableFileAtPath(codeTunnelURL.path)
}

func applicationURL(in environment: DetectionEnvironment = .live) -> URL? {
Expand Down Expand Up @@ -329,17 +330,61 @@ struct VSCodeCLILaunchConfiguration {
}

enum VSCodeCLILaunchConfigurationBuilder {
private struct VSCodeProductMetadata: Decodable {
let dataFolderName: String?
}

static func launchConfiguration(
vscodeApplicationURL: URL,
homeDirectoryURL: URL = FileManager.default.homeDirectoryForCurrentUser,
baseEnvironment: [String: String] = ProcessInfo.processInfo.environment,
isExecutableAtPath: (String) -> Bool = { FileManager.default.isExecutableFile(atPath: $0) }
isExecutableAtPath: (String) -> Bool = { FileManager.default.isExecutableFile(atPath: $0) },
dataAtURL: (URL) -> Data? = { try? Data(contentsOf: $0) },
contentsOfDirectoryAtURL: (URL) -> [URL] = { url in
(try? FileManager.default.contentsOfDirectory(
at: url,
includingPropertiesForKeys: [.contentModificationDateKey],
options: [.skipsHiddenFiles]
)) ?? []
},
contentModificationDateAtURL: (URL) -> Date? = { url in
(try? url.resourceValues(forKeys: [.contentModificationDateKey]))?.contentModificationDate
}
) -> VSCodeCLILaunchConfiguration? {
let contentsURL = vscodeApplicationURL.appendingPathComponent("Contents", isDirectory: true)
let environment = nodeSafeEnvironment(from: baseEnvironment)

if let codeServerURL = preferredCachedCodeServerURL(
contentsURL: contentsURL,
homeDirectoryURL: homeDirectoryURL,
isExecutableAtPath: isExecutableAtPath,
dataAtURL: dataAtURL,
contentsOfDirectoryAtURL: contentsOfDirectoryAtURL,
contentModificationDateAtURL: contentModificationDateAtURL
) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
var codeServerEnvironment = environment
codeServerEnvironment.removeValue(forKey: "ELECTRON_RUN_AS_NODE")
return VSCodeCLILaunchConfiguration(
executableURL: codeServerURL,
argumentsPrefix: [],
environment: codeServerEnvironment
)
}

let codeTunnelURL = contentsURL.appendingPathComponent("Resources/app/bin/code-tunnel", isDirectory: false)
guard isExecutableAtPath(codeTunnelURL.path) else { return nil }
var codeTunnelEnvironment = environment
codeTunnelEnvironment["ELECTRON_RUN_AS_NODE"] = "1"

return VSCodeCLILaunchConfiguration(
executableURL: codeTunnelURL,
argumentsPrefix: ["serve-web"],
environment: codeTunnelEnvironment
)
}

private static func nodeSafeEnvironment(from baseEnvironment: [String: String]) -> [String: String] {
var environment = baseEnvironment
environment["ELECTRON_RUN_AS_NODE"] = "1"
environment.removeValue(forKey: "VSCODE_NODE_OPTIONS")
environment.removeValue(forKey: "VSCODE_NODE_REPL_EXTERNAL_MODULE")
if let nodeOptions = environment["NODE_OPTIONS"] {
Expand All @@ -350,12 +395,87 @@ enum VSCodeCLILaunchConfigurationBuilder {
}
environment.removeValue(forKey: "NODE_OPTIONS")
environment.removeValue(forKey: "NODE_REPL_EXTERNAL_MODULE")

return VSCodeCLILaunchConfiguration(
executableURL: codeTunnelURL,
argumentsPrefix: [],
environment: environment
return environment
}

private static func preferredCachedCodeServerURL(
contentsURL: URL,
homeDirectoryURL: URL,
isExecutableAtPath: (String) -> Bool,
dataAtURL: (URL) -> Data?,
contentsOfDirectoryAtURL: (URL) -> [URL],
contentModificationDateAtURL: (URL) -> Date?
) -> URL? {
let dataFolderName = vscodeDataFolderName(
contentsURL: contentsURL,
dataAtURL: dataAtURL
)
let serveWebCacheURL = homeDirectoryURL
.appendingPathComponent(dataFolderName, isDirectory: true)
.appendingPathComponent("cli/serve-web", isDirectory: true)

if let orderedCacheIDs = serveWebLRUCacheIDs(
serveWebCacheURL: serveWebCacheURL,
dataAtURL: dataAtURL
) {
for cacheID in orderedCacheIDs {
let codeServerURL = serveWebCacheURL
.appendingPathComponent(cacheID, isDirectory: true)
.appendingPathComponent("bin/code-server", isDirectory: false)
if isExecutableAtPath(codeServerURL.path) {
return codeServerURL
Comment on lines +425 to +426

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Verify cached server build before launching it

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 👍 / 👎.

}
}
}

let candidates = contentsOfDirectoryAtURL(serveWebCacheURL)
.map {
$0.appendingPathComponent("bin/code-server", isDirectory: false)
}
.filter {
isExecutableAtPath($0.path)
}
.sorted { lhs, rhs in
let lhsDate = contentModificationDateAtURL(lhs) ?? .distantPast
let rhsDate = contentModificationDateAtURL(rhs) ?? .distantPast
if lhsDate != rhsDate {
return lhsDate > rhsDate
}
return lhs.path > rhs.path
}

return candidates.first
}

private static func vscodeDataFolderName(
contentsURL: URL,
dataAtURL: (URL) -> Data?
) -> String {
let productURL = contentsURL.appendingPathComponent("Resources/app/product.json", isDirectory: false)
guard let data = dataAtURL(productURL),
let product = try? JSONDecoder().decode(VSCodeProductMetadata.self, from: data),
let dataFolderName = product.dataFolderName,
isSafePathComponent(dataFolderName) else {
return ".vscode"
}
return dataFolderName
}

private static func serveWebLRUCacheIDs(
serveWebCacheURL: URL,
dataAtURL: (URL) -> Data?
) -> [String]? {
let lruURL = serveWebCacheURL.appendingPathComponent("lru.json", isDirectory: false)
guard let data = dataAtURL(lruURL),
let cacheIDs = try? JSONDecoder().decode([String].self, from: data) else {
return nil
}
return cacheIDs.filter(isSafePathComponent)
}

private static func isSafePathComponent(_ component: String) -> Bool {
guard !component.isEmpty, component != ".", component != ".." else { return false }
return component.rangeOfCharacter(from: CharacterSet(charactersIn: "/\\")) == nil
}
}

Expand Down Expand Up @@ -544,6 +664,14 @@ final class VSCodeServeWebController {
ensureServeWebURL(vscodeApplicationURL: vscodeApplicationURL, completion: completion)
}

func isServeWebURL(_ candidateURL: URL?) -> Bool {
guard let candidateURL else { return false }
let serveWebURL = queue.sync {
self.serveWebURL
}
return Self.urlsShareLoopbackOrigin(candidateURL, serveWebURL)
}
Comment on lines +667 to +673

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Blocking 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!

Comment on lines +667 to +673

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 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.


private func launchServeWebProcess(
vscodeApplicationURL: URL,
expectedGeneration: UInt64
Expand All @@ -563,7 +691,6 @@ final class VSCodeServeWebController {
let process = Process()
process.executableURL = launchConfiguration.executableURL
process.arguments = launchConfiguration.argumentsPrefix + [
"serve-web",
"--accept-server-license-terms",
"--host", "127.0.0.1",
"--port", "0",
Comment on lines 693 to 696

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Pass the WebSocket compression flag to VS Code

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 👍 / 👎.

Expand Down Expand Up @@ -711,6 +838,21 @@ final class VSCodeServeWebController {
private static func removeConnectionTokenFile(at url: URL) {
try? FileManager.default.removeItem(at: url)
}

private static func urlsShareLoopbackOrigin(_ lhs: URL, _ rhs: URL?) -> Bool {
guard let rhs else { return false }
guard lhs.scheme?.lowercased() == "http",
rhs.scheme?.lowercased() == "http" else {
return false
}
guard lhs.port == rhs.port, lhs.port != nil else { return false }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 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.

guard let lhsHost = BrowserInsecureHTTPSettings.normalizeHost(lhs.host ?? ""),
let rhsHost = BrowserInsecureHTTPSettings.normalizeHost(rhs.host ?? "") else {
return false
}
return RemoteLoopbackProxyAlias.isLoopbackHost(lhsHost)
&& RemoteLoopbackProxyAlias.isLoopbackHost(rhsHost)
}
Comment thread
cursor[bot] marked this conversation as resolved.
}

final class ServeWebOutputCollector {
Expand Down
8 changes: 8 additions & 0 deletions Sources/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12913,6 +12913,14 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
return true
}

if !hasFocusedAddressBarInShortcutContext,
Comment thread
cursor[bot] marked this conversation as resolved.
shouldRouteInlineVSCodeCommandPaletteShortcutThroughWebContentFirst(
event,
pageURL: shortcutEventBrowserPanel(event)?.webView.url
) {
return false
}

if matchConfiguredShortcut(event: event, action: .commandPalette) {
let targetWindow = commandPaletteTargetWindow ?? event.window ?? NSApp.keyWindow ?? NSApp.mainWindow
requestCommandPaletteCommands(preferredWindow: targetWindow, source: "shortcut.commandPalette")
Expand Down
16 changes: 16 additions & 0 deletions Sources/Panels/CmuxWebView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,11 @@ final class CmuxWebView: WKWebView {
}
}

if shouldRouteInlineVSCodeCommandPaletteShortcutThroughWebContentFirst(event, pageURL: url) {
_ = super.performKeyEquivalent(with: event)
return finish(true)
Comment on lines +714 to +716

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Do not consume unclaimed VS Code shortcuts

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 👍 / 👎.

}
Comment thread
cursor[bot] marked this conversation as resolved.

if !shouldRouteCommandEquivalentDirectlyToMainMenu(event) {
return finish(super.performKeyEquivalent(with: event))
}
Expand Down Expand Up @@ -785,6 +790,17 @@ final class CmuxWebView: WKWebView {
}
}

// Inline VS Code owns Cmd+Shift+P for its in-page command palette.
// If this path reaches keyDown, forward it to WebKit instead of cmux.
if event.modifierFlags.intersection(.deviceIndependentFlagsMask).contains(.command),
shouldRouteInlineVSCodeCommandPaletteShortcutThroughWebContentFirst(event, pageURL: url) {
#if DEBUG
route = "inlineVSCode"
#endif
super.keyDown(with: event)
return
}

// Some Cmd-based key paths in WebKit don't consistently invoke performKeyEquivalent.
// Route them through the same app-level shortcut handler as a fallback.
if event.modifierFlags.intersection(.deviceIndependentFlagsMask).contains(.command),
Expand Down
67 changes: 67 additions & 0 deletions cmuxTests/AppDelegateShortcutRoutingTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5761,6 +5761,73 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
}
}

func testInlineVSCodeCommandPaletteShortcutRoutesThroughWebContentForTrackedServeWebOrigin() {
let event = makeKeyEvent(
modifierFlags: [.command, .shift],
characters: "P",
charactersIgnoringModifiers: "p",
keyCode: 35
)
let pageURL = URL(string: "http://127.0.0.1:63266/?folder=%2FUsers%2Ftester%2Fproject")!

XCTAssertTrue(
shouldRouteInlineVSCodeCommandPaletteShortcutThroughWebContentFirst(
event,
pageURL: pageURL,
inlineVSCodeURLMatcher: { $0 == pageURL },
shortcutForAction: { action in
XCTAssertEqual(action, .commandPalette)
return StoredShortcut(key: "p", command: true, shift: true, option: false, control: false, keyCode: 35)
}
),
"Expected Cmd+Shift+P to stay inside inline VS Code when the focused browser URL belongs to the live serve-web process"
)
}

func testInlineVSCodeCommandPaletteShortcutDoesNotRouteForUntrackedLocalhostPage() {
let event = makeKeyEvent(
modifierFlags: [.command, .shift],
characters: "P",
charactersIgnoringModifiers: "p",
keyCode: 35
)
let pageURL = URL(string: "http://127.0.0.1:3000/?folder=%2FUsers%2Ftester%2Fproject")!

XCTAssertFalse(
shouldRouteInlineVSCodeCommandPaletteShortcutThroughWebContentFirst(
event,
pageURL: pageURL,
inlineVSCodeURLMatcher: { _ in false },
shortcutForAction: { _ in
StoredShortcut(key: "p", command: true, shift: true, option: false, control: false, keyCode: 35)
}
),
"A localhost page with a folder query must not steal cmux's command palette shortcut unless it is the tracked VS Code serve-web origin"
)
}

func testInlineVSCodeCommandPaletteShortcutDoesNotRouteUnrelatedShortcut() {
let event = makeKeyEvent(
modifierFlags: [.command],
characters: "l",
charactersIgnoringModifiers: "l",
keyCode: 37
)
let pageURL = URL(string: "http://127.0.0.1:63266/?folder=%2FUsers%2Ftester%2Fproject")!

XCTAssertFalse(
shouldRouteInlineVSCodeCommandPaletteShortcutThroughWebContentFirst(
event,
pageURL: pageURL,
inlineVSCodeURLMatcher: { $0 == pageURL },
shortcutForAction: { _ in
StoredShortcut(key: "p", command: true, shift: true, option: false, control: false, keyCode: 35)
}
),
"Only the configured command palette shortcut should bypass cmux for inline VS Code"
)
}

// MARK: - Non-Latin keyboard layout shortcut tests

func testCmdTWorksWithRussianKeyboardLayout() {
Expand Down
Loading
Loading