Add CEF (Chromium) browser engine infrastructure#2073
Add CEF (Chromium) browser engine infrastructure#2073lawrencecchen wants to merge 27 commits intomainfrom
Conversation
Phase 0 of WebKit-to-Chromium migration: sets up the build system
and foundational types for embedding CEF alongside WebKit.
- vendor/cef-bridge/: C bridge layer (cef_bridge.h, cef_bridge.cpp)
wrapping CEF C++ API behind a plain C interface callable from
Swift via the bridging header. Currently stub implementations
that return CEF_BRIDGE_ERR_NOT_INIT until CEF is wired up.
- Sources/BrowserEngine/: Swift foundation types
- BrowserEngineType: enum for webkit/chromium engine selection
- CEFFrameworkManager: on-demand download, verification, and
extraction of the CEF framework at runtime (~120MB compressed)
- CEFRuntime: CEF lifecycle management (init, message loop, shutdown)
- CEFDownloadView: SwiftUI progress UI shown when CEF needs download
- Updated cmux-Bridging-Header.h to import cef_bridge.h
- Updated project.pbxproj to link libcef_bridge.a and add new sources
- Updated scripts/setup.sh to build the CEF bridge stub library
|
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:
📝 WalkthroughWalkthroughAdds Chromium Embedded Framework (CEF) integration: C bridge (header, implementation, Makefile), build/embed scripts and setup steps, Swift-side download manager/runtime/view to provision and initialize CEF, Xcode project updates to link the bridge, and UI/view wiring to opt into a Chromium engine in panels. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Swift App
participant Manager as CEFFrameworkManager
participant URLSession as URLSession
participant FS as File System
App->>Manager: query state / show CEFDownloadView
Manager->>FS: check for extracted framework & .cef-version
alt present & matches
Manager->>App: state = ready
else
Manager->>App: state = notDownloaded
end
App->>Manager: download()
Manager->>Manager: state = downloading(0)
Manager->>URLSession: start download (observe progress)
URLSession-->>Manager: progress updates
Manager->>App: state = downloading(progress)
URLSession-->>Manager: download complete
Manager->>Manager: state = extracting
Manager->>FS: extract archive (tar)
FS-->>Manager: extraction done
Manager->>FS: write .cef-version, remove archive
Manager->>App: state = ready
sequenceDiagram
participant App as Swift App
participant CEFRuntime as CEFRuntime
participant Manager as CEFFrameworkManager
participant Bridge as C ABI (cef_bridge)
participant FS as File System
App->>CEFRuntime: initialize()
CEFRuntime->>Manager: verify framework available
Manager-->>CEFRuntime: available / ready
CEFRuntime->>FS: resolve frameworkDir & helperPath, create cache dir
FS-->>CEFRuntime: paths ready
CEFRuntime->>Bridge: cef_bridge_initialize(frameworkDir, helperPath, cacheRoot)
Bridge-->>CEFRuntime: success
CEFRuntime->>CEFRuntime: isInitialized = true
CEFRuntime->>CEFRuntime: startMessageLoop() (~60Hz)
loop periodic
CEFRuntime->>Bridge: cef_bridge_do_message_loop_work()
end
CEFRuntime-->>App: initialize() returns true
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 lays the Phase 0 scaffolding for a WebKit-to-Chromium migration: a C bridge stub library ( There are three functional issues in
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CEFDownloadView
participant CEFFrameworkManager
participant URLSession
participant Disk
User->>CEFDownloadView: Switches to Chromium profile
CEFDownloadView->>CEFFrameworkManager: download(completion:)
CEFFrameworkManager->>URLSession: downloadTask(with: downloadURL)
URLSession-->>CEFFrameworkManager: progress updates (KVO)
CEFFrameworkManager-->>CEFDownloadView: state = .downloading(progress)
alt Download succeeds
URLSession-->>CEFFrameworkManager: completion(tempURL)
CEFFrameworkManager->>Disk: tar -xzf tempURL -C frameworksDir
Note over CEFFrameworkManager,Disk: ⚠️ No SHA256 check before extraction
Disk-->>CEFFrameworkManager: extraction done
CEFFrameworkManager->>Disk: write .cef-version stamp
CEFFrameworkManager-->>CEFDownloadView: state = .ready
CEFDownloadView->>CEFRuntime: initialize()
CEFRuntime->>CEFFrameworkManager: isAvailable?
CEFRuntime->>cef_bridge_initialize: framework/helper/cache paths
else User cancels
User->>CEFDownloadView: taps Cancel
CEFDownloadView->>CEFFrameworkManager: cancelDownload()
Note over CEFFrameworkManager: state = .notDownloaded ⚠️ task NOT cancelled
URLSession-->>CEFFrameworkManager: completion fires anyway
CEFFrameworkManager->>Disk: extracts framework (ignores cancellation)
end
Reviews (1): Last reviewed commit: "Add CEF (Chromium) browser engine infras..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func download(completion: @escaping (Result<Void, Error>) -> Void) { | ||
| guard state != .ready else { | ||
| completion(.success(())) | ||
| return | ||
| } | ||
|
|
||
| if case .downloading = state { return } | ||
|
|
||
| state = .downloading(progress: 0) | ||
|
|
||
| let session = URLSession( | ||
| configuration: .default, | ||
| delegate: nil, | ||
| delegateQueue: .main | ||
| ) | ||
|
|
||
| let task = session.downloadTask(with: Self.downloadURL) { [weak self] tempURL, response, error in | ||
| DispatchQueue.main.async { | ||
| guard let self else { return } | ||
|
|
||
| if let error { | ||
| self.state = .failed(message: error.localizedDescription) | ||
| completion(.failure(error)) | ||
| return | ||
| } | ||
|
|
||
| guard let tempURL else { | ||
| let err = NSError( | ||
| domain: "CEFFrameworkManager", | ||
| code: -1, | ||
| userInfo: [NSLocalizedDescriptionKey: "Download produced no file"] | ||
| ) | ||
| self.state = .failed(message: err.localizedDescription) | ||
| completion(.failure(err)) | ||
| return | ||
| } | ||
|
|
||
| self.state = .extracting | ||
| self.extractFramework(from: tempURL, completion: completion) | ||
| } | ||
| } | ||
|
|
||
| // Observe progress | ||
| let observation = task.progress.observe(\.fractionCompleted) { [weak self] progress, _ in | ||
| DispatchQueue.main.async { | ||
| self?.state = .downloading(progress: progress.fractionCompleted) | ||
| } | ||
| } | ||
|
|
||
| // Keep the observation alive until task completes | ||
| objc_setAssociatedObject(task, "progressObservation", observation, .OBJC_ASSOCIATION_RETAIN) | ||
|
|
||
| task.resume() | ||
| } | ||
|
|
||
| /// Cancel an in-progress download. | ||
| func cancelDownload() { | ||
| state = .notDownloaded | ||
| } |
There was a problem hiding this comment.
cancelDownload() doesn't cancel the URLSession task
cancelDownload() only resets state = .notDownloaded but never cancels the underlying URLSessionDownloadTask. The task continues running in the background and its completion handler will fire regardless — at which point it overwrites the cancelled state with .extracting and calls extractFramework, unconditionally installing the framework the user just cancelled.
The download task needs to be stored on self so it can be explicitly cancelled:
// Add as a stored property:
private var currentDownloadTask: URLSessionDownloadTask?
// In download():
currentDownloadTask = session.downloadTask(with: Self.downloadURL) { ... }
currentDownloadTask?.resume()
// In the completion handler, guard against cancellation:
guard self.state != .notDownloaded else { return }
// In cancelDownload():
func cancelDownload() {
currentDownloadTask?.cancel()
currentDownloadTask = nil
state = .notDownloaded
}| private func extractFramework( | ||
| from archiveURL: URL, | ||
| completion: @escaping (Result<Void, Error>) -> Void | ||
| ) { | ||
| DispatchQueue.global(qos: .userInitiated).async { [weak self] in | ||
| guard let self else { return } | ||
|
|
||
| do { | ||
| let fm = FileManager.default | ||
|
|
||
| // Ensure target directory exists | ||
| try fm.createDirectory( | ||
| at: self.frameworksDir, | ||
| withIntermediateDirectories: true | ||
| ) | ||
|
|
||
| // Remove existing framework if present | ||
| let destPath = self.frameworkPath | ||
| if fm.fileExists(atPath: destPath.path) { | ||
| try fm.removeItem(at: destPath) | ||
| } | ||
|
|
||
| // Extract tar.gz archive | ||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: "/usr/bin/tar") | ||
| process.arguments = [ | ||
| "-xzf", archiveURL.path, | ||
| "-C", self.frameworksDir.path | ||
| ] | ||
| try process.run() | ||
| process.waitUntilExit() | ||
|
|
||
| guard process.terminationStatus == 0 else { | ||
| throw NSError( | ||
| domain: "CEFFrameworkManager", | ||
| code: Int(process.terminationStatus), | ||
| userInfo: [NSLocalizedDescriptionKey: "Failed to extract CEF framework (tar exit \(process.terminationStatus))"] | ||
| ) | ||
| } | ||
|
|
||
| // Verify extraction produced the framework | ||
| guard fm.fileExists(atPath: destPath.path) else { | ||
| throw NSError( | ||
| domain: "CEFFrameworkManager", | ||
| code: -2, | ||
| userInfo: [NSLocalizedDescriptionKey: "Extraction completed but framework not found at expected path"] | ||
| ) | ||
| } | ||
|
|
||
| // Write version stamp | ||
| try Self.cefVersion.write( | ||
| to: self.versionStampPath, | ||
| atomically: true, | ||
| encoding: .utf8 | ||
| ) | ||
|
|
||
| // Clean up temp file | ||
| try? fm.removeItem(at: archiveURL) | ||
|
|
||
| DispatchQueue.main.async { | ||
| self.state = .ready | ||
| completion(.success(())) | ||
| } | ||
| } catch { | ||
| DispatchQueue.main.async { | ||
| self.state = .failed(message: error.localizedDescription) | ||
| completion(.failure(error)) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
expectedSHA256 is an empty string and there is no SHA256 verification step anywhere in extractFramework. A ~120 MB binary archive is downloaded from a GitHub Releases URL and piped directly into tar with no integrity check. Even when expectedSHA256 is eventually populated in a follow-up phase, no code will use it — the verification logic itself is absent.
Before calling tar, the archive should be hashed and compared:
// After receiving tempURL and before extracting:
let digest = SHA256.hash(data: try Data(contentsOf: archiveURL))
let hex = digest.map { String(format: "%02x", $0) }.joined()
guard hex == Self.expectedSHA256 else {
throw NSError(
domain: "CEFFrameworkManager", code: -3,
userInfo: [NSLocalizedDescriptionKey: "SHA256 mismatch: expected \(Self.expectedSHA256), got \(hex)"]
)
}(CryptoKit is available on macOS 10.15+.)
| static let downloadURL: URL = { | ||
| // TODO(phase0): replace with actual hosted URL | ||
| let base = "https://github.com/manaflow-ai/cmux-cef-framework/releases/download" | ||
| let tag = "v130.1.16" | ||
| let file = "cef-arm64.tar.gz" | ||
| return URL(string: "\(base)/\(tag)/\(file)")! | ||
| }() |
There was a problem hiding this comment.
arm64 only
The download URL is pinned to cef-arm64.tar.gz, so Intel Mac users will receive an incompatible binary (or nothing, once the release is published). The Makefile correctly builds a universal arm64 + x86_64 stub library, so the expectation appears to be universal support.
Consider selecting the archive based on the current architecture:
static let downloadURL: URL = {
let base = "https://github.com/manaflow-ai/cmux-cef-framework/releases/download"
let tag = "v130.1.16"
#if arch(arm64)
let file = "cef-arm64.tar.gz"
#else
let file = "cef-x86_64.tar.gz"
#endif
return URL(string: "\(base)/\(tag)/\(file)")!
}()| } | ||
|
|
||
| // Keep the observation alive until task completes | ||
| objc_setAssociatedObject(task, "progressObservation", observation, .OBJC_ASSOCIATION_RETAIN) |
There was a problem hiding this comment.
objc_setAssociatedObject used with a Swift String key
objc_setAssociatedObject requires a stable UnsafeRawPointer as its key — the pointer identity is what matters, not the value. A Swift String literal is a value type whose bridged pointer can vary between calls, so the association is not reliably keyed. The standard idiom is a static address:
private static var progressObservationKey: UInt8 = 0
// then in download():
objc_setAssociatedObject(task, &Self.progressObservationKey, observation, .OBJC_ASSOCIATION_RETAIN)| guard !isInitialized else { return true } | ||
| guard CEFFrameworkManager.shared.isAvailable else { return false } | ||
|
|
||
| let frameworkPath = CEFFrameworkManager.shared.frameworksDir.path |
There was a problem hiding this comment.
frameworksDir passed where framework-containing directory is expected, but variable name is confusing
cef_bridge_initialize expects framework_path to be "the directory containing 'Chromium Embedded Framework.framework'" — which is frameworksDir. The value is correct, but the local variable is named frameworkPath, which shadows the convention used in CEFFrameworkManager where frameworkPath refers to the .framework bundle itself. A future maintainer adding Phase 2 code could easily swap in frameworkPath (the wrong path) from CEFFrameworkManager while reading this call site.
| let frameworkPath = CEFFrameworkManager.shared.frameworksDir.path | |
| let frameworkContainerPath = CEFFrameworkManager.shared.frameworksDir.path |
...and update the cef_bridge_initialize call accordingly.
![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: 753892889f
ℹ️ 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".
| self.state = .extracting | ||
| self.extractFramework(from: tempURL, completion: completion) |
There was a problem hiding this comment.
download() transitions straight from receiving tempURL to extractFramework(...) without ever validating the archive hash, even though expectedSHA256 is defined for this purpose. In any run where the download is corrupted or the release asset is replaced, this code will still extract and potentially mark the framework as ready, which can load invalid/untrusted binaries once Chromium profiles are enabled.
Useful? React with 👍 / 👎.
| func cancelDownload() { | ||
| state = .notDownloaded |
There was a problem hiding this comment.
cancelDownload() only updates state and does not cancel the underlying URLSessionDownloadTask, so a canceled transfer can continue in the background and still invoke extraction/completion later. This causes the UI to report cancellation while the framework may still be written to disk and state can flip back to .extracting/.ready unexpectedly.
Useful? React with 👍 / 👎.
| localized: "cef.download.title", | ||
| defaultValue: "Chromium Engine Required" |
There was a problem hiding this comment.
This commit introduces new user-facing cef.download.* localization keys but does not add them (with English/Japanese translations) to Resources/Localizable.xcstrings, so non-English users will fall back to default English text in this flow. Per /workspace/cmux AGENTS.md, all user-visible strings must be represented in the localization catalog for supported languages.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/BrowserEngine/CEFFrameworkManager.swift">
<violation number="1" location="Sources/BrowserEngine/CEFFrameworkManager.swift:21">
P0: The downloaded CEF archive is extracted without SHA256 verification, so a tampered download could be installed as a trusted framework.</violation>
<violation number="2" location="Sources/BrowserEngine/CEFFrameworkManager.swift:29">
P2: The download asset is hardcoded to arm64. Select the archive by host architecture so Intel Macs don't fetch an incompatible framework.</violation>
<violation number="3" location="Sources/BrowserEngine/CEFFrameworkManager.swift:106">
P1: `cancelDownload()` does not cancel the active URLSession task, so downloads can continue and complete after the user cancels.</violation>
</file>
<file name="Sources/BrowserEngine/CEFRuntime.swift">
<violation number="1" location="Sources/BrowserEngine/CEFRuntime.swift:9">
P1: Enforce main-thread isolation for CEFRuntime lifecycle methods; currently callers can initialize/pump/shutdown off-main, which can break CEF message-loop pumping.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| static let cefVersion = "130.1.16+g03e8e4e+chromium-130.0.6723.117" | ||
|
|
||
| /// SHA256 of the compressed framework archive. | ||
| static let expectedSHA256 = "" // TODO(phase0): set after first hosted build |
There was a problem hiding this comment.
P0: The downloaded CEF archive is extracted without SHA256 verification, so a tampered download could be installed as a trusted framework.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/BrowserEngine/CEFFrameworkManager.swift, line 21:
<comment>The downloaded CEF archive is extracted without SHA256 verification, so a tampered download could be installed as a trusted framework.</comment>
<file context>
@@ -0,0 +1,247 @@
+ static let cefVersion = "130.1.16+g03e8e4e+chromium-130.0.6723.117"
+
+ /// SHA256 of the compressed framework archive.
+ static let expectedSHA256 = "" // TODO(phase0): set after first hosted build
+
+ /// URL to the hosted compressed CEF framework archive.
</file context>
| // Wrong version, needs re-download | ||
| try? fm.removeItem(at: frameworkPath) | ||
| } | ||
| state = .notDownloaded |
There was a problem hiding this comment.
P1: cancelDownload() does not cancel the active URLSession task, so downloads can continue and complete after the user cancels.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/BrowserEngine/CEFFrameworkManager.swift, line 106:
<comment>`cancelDownload()` does not cancel the active URLSession task, so downloads can continue and complete after the user cancels.</comment>
<file context>
@@ -0,0 +1,247 @@
+ // Wrong version, needs re-download
+ try? fm.removeItem(at: frameworkPath)
+ }
+ state = .notDownloaded
+ }
+
</file context>
| // TODO(phase0): replace with actual hosted URL | ||
| let base = "https://github.com/manaflow-ai/cmux-cef-framework/releases/download" | ||
| let tag = "v130.1.16" | ||
| let file = "cef-arm64.tar.gz" |
There was a problem hiding this comment.
P2: The download asset is hardcoded to arm64. Select the archive by host architecture so Intel Macs don't fetch an incompatible framework.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/BrowserEngine/CEFFrameworkManager.swift, line 29:
<comment>The download asset is hardcoded to arm64. Select the archive by host architecture so Intel Macs don't fetch an incompatible framework.</comment>
<file context>
@@ -0,0 +1,247 @@
+ // TODO(phase0): replace with actual hosted URL
+ let base = "https://github.com/manaflow-ai/cmux-cef-framework/releases/download"
+ let tag = "v130.1.16"
+ let file = "cef-arm64.tar.gz"
+ return URL(string: "\(base)/\(tag)/\(file)")!
+ }()
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@GhosttyTabs.xcodeproj/project.pbxproj`:
- Around line 940-947: The project currently links "-lcef_bridge" and sets
LIBRARY_SEARCH_PATHS to vendor/cef-bridge (so GhosttyTabs expects
vendor/cef-bridge/libcef_bridge.a) but there is no build phase or target that
produces libcef_bridge.a (the only producer is scripts/setup.sh), causing clean
builds to fail; add a proper build step by creating either (A) a new
external/build target or aggregate target that runs the bridge build and
produces libcef_bridge.a and add it as a Target Dependency of the GhosttyTabs
target, or (B) add a Shell Script Build Phase (in the GhosttyTabs target) that
runs the existing scripts/setup.sh (or the specific build commands from lines
87–94) to produce libcef_bridge.a before linking, and ensure the output file
path points to $(PROJECT_DIR)/vendor/cef-bridge/libcef_bridge.a so Xcode treats
it as a derived product.
In `@scripts/setup.sh`:
- Around line 89-94: The setup script currently skips building the CEF bridge
when "$CEF_BRIDGE_DIR/Makefile" is missing but continues to report "Setup
complete", which hides a deterministic failure; update the conditional around
the Makefile check so that if the Makefile is missing you print a clear error
(replace the warning echo) and exit with non-zero status, and also check the
exit code of make -C "$CEF_BRIDGE_DIR" clean all (or use set -e) and exit
non-zero with an error message if the build fails; reference the existing
"$CEF_BRIDGE_DIR/Makefile" existence check and the make invocation to implement
these early exits.
In `@Sources/BrowserEngine/CEFDownloadView.swift`:
- Around line 80-92: CEF error messages are being displayed using raw
error.localizedDescription in the .failed(message:) producers in
CEFFrameworkManager and violate the app localization pattern; update those
producers (or wrap at display in CEFDownloadView) to pass localized text using
String(localized: "cef.download.failed.details", defaultValue: "Download failed
(%@)", "\(error.localizedDescription)") or similar keys as used in
cmuxApp.swift, and add the corresponding entries to
Resources/Localizable.xcstrings; specifically locate uses of .failed(message:)
in CEFFrameworkManager and change the message to a localized
String(localized:..., defaultValue:...) that includes the
error.localizedDescription, or perform that wrapping in CEFDownloadView before
showing the Text(message).
In `@Sources/BrowserEngine/CEFFrameworkManager.swift`:
- Around line 121-163: The download task and extraction are currently
fire-and-forget; modify CEFFrameworkManager to store the active
URLSessionDownloadTask (task), its progress observation (observation) and a
generation token (e.g., downloadGeneration) when starting a download, cancel the
task and remove/terminate the observation and any spawned extraction Process in
cancelDownload(), and increment the generation to mark cancellations; in
extractFramework(from:completion:) accept/check the same generation token (or
have it accessible) and ignore any completion callbacks or state changes if the
token is stale, and ensure any spawned Process is terminated on cancel to
prevent a late .ready/.failed state or calling completion after cancellation.
- Around line 20-21: The code currently leaves static let expectedSHA256 = ""
and proceeds to call extractFramework(from:) without verifying integrity; update
the download-and-install flow to fail closed: first ensure
BrowserEngine.CEFFrameworkManager.expectedSHA256 is non-empty and, after
downloading the archive but before calling extractFramework(from:), compute the
SHA256 of the downloaded file and compare it to expectedSHA256; if
expectedSHA256 is unset or the digests don't match, log an error and abort the
extraction/install process (do not call extractFramework(from:) on mismatch).
Apply this check in the same download/install routine that currently calls
extractFramework(from:) (and in the code around lines 127–149 referenced in the
review).
- Around line 35-40: State.failed(message:) is being populated with raw English
NSLocalizedDescriptionKey literals; wrap each user-facing failure string in
String(localized: "key.name", defaultValue: "English text") before passing it
into State.failed(message:). Update every site that constructs State.failed(...)
(including the places that produce NSError userInfo with
NSLocalizedDescriptionKey) and replace the literal with the localized
initializer, and add corresponding keys and default values to
Resources/Localizable.xcstrings so the UI sees the localized strings.
In `@Sources/BrowserEngine/CEFRuntime.swift`:
- Around line 9-16: The CEFRuntime timer is being scheduled on the current run
loop which can be off-main or pause during tracking loops; update CEFRuntime to
ensure it is main-thread-bound and install messageLoopTimer on RunLoop.main with
.common modes (use RunLoop.main.add(_:forMode: .common) or equivalent) inside
initialize() and any restart logic so the timer always runs on the main run loop
and in common modes; reference CEFRuntime, initialize(), and messageLoopTimer
when making the change.
In `@vendor/cef-bridge/cef_bridge.h`:
- Around line 121-128: The current cef_bridge_jsdialog_callback signature only
returns a bool and cannot return the user-entered text required for prompt
dialogs; update the ABI so the callback can convey both success and the prompt
string (for example by changing typedef bool
(*cef_bridge_jsdialog_callback)(...) to a signature that includes an out
parameter or a struct return containing a bool success and a char* user_input),
ensure the implementation and any callers of cef_bridge_jsdialog_callback and
related uses in prompt handling correctly allocate/return the user_input string
and define ownership/cleanup semantics, and update any structs/types that
reference cef_bridge_jsdialog_callback so prompt dialogs can call the equivalent
of CefJSDialogCallback::Continue(bool success, const CefString& user_input)
through the bridge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfd65987-2cf3-43df-b9d1-2cbb8a4e5130
📒 Files selected for processing (11)
GhosttyTabs.xcodeproj/project.pbxprojSources/BrowserEngine/BrowserEngineType.swiftSources/BrowserEngine/CEFDownloadView.swiftSources/BrowserEngine/CEFFrameworkManager.swiftSources/BrowserEngine/CEFRuntime.swiftcmux-Bridging-Header.hscripts/setup.shvendor/cef-bridge/.gitignorevendor/cef-bridge/Makefilevendor/cef-bridge/cef_bridge.cppvendor/cef-bridge/cef_bridge.h
| if [ -f "$CEF_BRIDGE_DIR/Makefile" ]; then | ||
| make -C "$CEF_BRIDGE_DIR" clean all | ||
| echo "==> Built libcef_bridge.a (stub mode, no CEF framework)" | ||
| else | ||
| echo "==> Warning: vendor/cef-bridge/Makefile not found, skipping CEF bridge build" | ||
| fi |
There was a problem hiding this comment.
Fail setup when the bridge library cannot be produced.
If vendor/cef-bridge/Makefile is absent, the script still reaches “Setup complete” even though the app target now always links -lcef_bridge. That hides a deterministic setup problem and turns it into a later linker failure.
Suggested change
if [ -f "$CEF_BRIDGE_DIR/Makefile" ]; then
make -C "$CEF_BRIDGE_DIR" clean all
echo "==> Built libcef_bridge.a (stub mode, no CEF framework)"
else
- echo "==> Warning: vendor/cef-bridge/Makefile not found, skipping CEF bridge build"
+ echo "Error: vendor/cef-bridge/Makefile not found; cannot build libcef_bridge.a" >&2
+ exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -f "$CEF_BRIDGE_DIR/Makefile" ]; then | |
| make -C "$CEF_BRIDGE_DIR" clean all | |
| echo "==> Built libcef_bridge.a (stub mode, no CEF framework)" | |
| else | |
| echo "==> Warning: vendor/cef-bridge/Makefile not found, skipping CEF bridge build" | |
| fi | |
| if [ -f "$CEF_BRIDGE_DIR/Makefile" ]; then | |
| make -C "$CEF_BRIDGE_DIR" clean all | |
| echo "==> Built libcef_bridge.a (stub mode, no CEF framework)" | |
| else | |
| echo "Error: vendor/cef-bridge/Makefile not found; cannot build libcef_bridge.a" >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup.sh` around lines 89 - 94, The setup script currently skips
building the CEF bridge when "$CEF_BRIDGE_DIR/Makefile" is missing but continues
to report "Setup complete", which hides a deterministic failure; update the
conditional around the Makefile check so that if the Makefile is missing you
print a clear error (replace the warning echo) and exit with non-zero status,
and also check the exit code of make -C "$CEF_BRIDGE_DIR" clean all (or use set
-e) and exit non-zero with an error message if the build fails; reference the
existing "$CEF_BRIDGE_DIR/Makefile" existence check and the make invocation to
implement these early exits.
| case .failed(let message): | ||
| VStack(spacing: 8) { | ||
| Label(String( | ||
| localized: "cef.download.failed", | ||
| defaultValue: "Download failed" | ||
| ), systemImage: "exclamationmark.triangle.fill") | ||
| .foregroundStyle(.red) | ||
|
|
||
| Text(message) | ||
| .font(.caption) | ||
| .foregroundStyle(.secondary) | ||
| .multilineTextAlignment(.center) | ||
| .frame(maxWidth: 400) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 --type=swift 'enum State|case failed\(|\.failed\(' Sources/BrowserEngineRepository: manaflow-ai/cmux
Length of output: 3138
🏁 Script executed:
# Search for other places where error messages are displayed or localized
rg -n -C2 --type=swift 'localizedDescription|Text.*error|Text.*message' Sources/ \
| head -80Repository: manaflow-ai/cmux
Length of output: 5795
🏁 Script executed:
# Check if there are any custom error types with localization in CEFFrameworkManager
cat -n Sources/BrowserEngine/CEFFrameworkManager.swift | head -250Repository: manaflow-ai/cmux
Length of output: 10246
Apply the app's localization pattern to CEF error messages.
The .failed(message:) producers in CEFFrameworkManager (lines 132, 143, 238) use .localizedDescription directly, bypassing the app's localization rule. The codebase establishes a clear pattern in cmuxApp.swift (lines 4296, 4308): wrap error text with String(localized: "key.name", defaultValue: "Failed to... (\(error.localizedDescription))."). CEF errors shown in the UI must follow the same pattern: create localized strings with keys in Resources/Localizable.xcstrings before passing them to .failed(message:), or wrap the message at display time in CEFDownloadView.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/BrowserEngine/CEFDownloadView.swift` around lines 80 - 92, CEF error
messages are being displayed using raw error.localizedDescription in the
.failed(message:) producers in CEFFrameworkManager and violate the app
localization pattern; update those producers (or wrap at display in
CEFDownloadView) to pass localized text using String(localized:
"cef.download.failed.details", defaultValue: "Download failed (%@)",
"\(error.localizedDescription)") or similar keys as used in cmuxApp.swift, and
add the corresponding entries to Resources/Localizable.xcstrings; specifically
locate uses of .failed(message:) in CEFFrameworkManager and change the message
to a localized String(localized:..., defaultValue:...) that includes the
error.localizedDescription, or perform that wrapping in CEFDownloadView before
showing the Text(message).
| /// SHA256 of the compressed framework archive. | ||
| static let expectedSHA256 = "" // TODO(phase0): set after first hosted build |
There was a problem hiding this comment.
Fail closed on archive integrity before extracting.
expectedSHA256 is still empty and the download path never checks it before calling extractFramework(from:). That means any bytes fetched from the release endpoint get unpacked into Application Support and later treated as trusted runtime code. Please verify the digest before extraction and refuse to proceed while the pinned hash is unset.
Also applies to: 127-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/BrowserEngine/CEFFrameworkManager.swift` around lines 20 - 21, The
code currently leaves static let expectedSHA256 = "" and proceeds to call
extractFramework(from:) without verifying integrity; update the
download-and-install flow to fail closed: first ensure
BrowserEngine.CEFFrameworkManager.expectedSHA256 is non-empty and, after
downloading the archive but before calling extractFramework(from:), compute the
SHA256 of the downloaded file and compare it to expectedSHA256; if
expectedSHA256 is unset or the digests don't match, log an error and abort the
extraction/install process (do not call extractFramework(from:) on mismatch).
Apply this check in the same download/install routine that currently calls
extractFramework(from:) (and in the code around lines 127–149 referenced in the
review).
| enum State: Equatable { | ||
| case notDownloaded | ||
| case downloading(progress: Double) | ||
| case extracting | ||
| case ready | ||
| case failed(message: String) |
There was a problem hiding this comment.
Localize the failure messages before they hit State.failed(message:).
These NSLocalizedDescriptionKey strings are surfaced by the download UI, so raw English literals here skip the required localization path. Please wrap them in String(localized: ..., defaultValue: ...) and add the keys to Resources/Localizable.xcstrings.
🌐 Example
- userInfo: [NSLocalizedDescriptionKey: "Download produced no file"]
+ userInfo: [NSLocalizedDescriptionKey: String(
+ localized: "browserEngine.cef.download.noFile",
+ defaultValue: "Download produced no file"
+ )]As per coding guidelines, "All user-facing strings must be localized using String(localized: "key.name", defaultValue: "English text") for every string shown in the UI (labels, buttons, menus, dialogs, tooltips, error messages)."
Also applies to: 138-145, 205-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/BrowserEngine/CEFFrameworkManager.swift` around lines 35 - 40,
State.failed(message:) is being populated with raw English
NSLocalizedDescriptionKey literals; wrap each user-facing failure string in
String(localized: "key.name", defaultValue: "English text") before passing it
into State.failed(message:). Update every site that constructs State.failed(...)
(including the places that produce NSError userInfo with
NSLocalizedDescriptionKey) and replace the literal with the localized
initializer, and add corresponding keys and default values to
Resources/Localizable.xcstrings so the UI sees the localized strings.
| let session = URLSession( | ||
| configuration: .default, | ||
| delegate: nil, | ||
| delegateQueue: .main | ||
| ) | ||
|
|
||
| let task = session.downloadTask(with: Self.downloadURL) { [weak self] tempURL, response, error in | ||
| DispatchQueue.main.async { | ||
| guard let self else { return } | ||
|
|
||
| if let error { | ||
| self.state = .failed(message: error.localizedDescription) | ||
| completion(.failure(error)) | ||
| return | ||
| } | ||
|
|
||
| guard let tempURL else { | ||
| let err = NSError( | ||
| domain: "CEFFrameworkManager", | ||
| code: -1, | ||
| userInfo: [NSLocalizedDescriptionKey: "Download produced no file"] | ||
| ) | ||
| self.state = .failed(message: err.localizedDescription) | ||
| completion(.failure(err)) | ||
| return | ||
| } | ||
|
|
||
| self.state = .extracting | ||
| self.extractFramework(from: tempURL, completion: completion) | ||
| } | ||
| } | ||
|
|
||
| // Observe progress | ||
| let observation = task.progress.observe(\.fractionCompleted) { [weak self] progress, _ in | ||
| DispatchQueue.main.async { | ||
| self?.state = .downloading(progress: progress.fractionCompleted) | ||
| } | ||
| } | ||
|
|
||
| // Keep the observation alive until task completes | ||
| objc_setAssociatedObject(task, "progressObservation", observation, .OBJC_ASSOCIATION_RETAIN) | ||
|
|
||
| task.resume() |
There was a problem hiding this comment.
cancelDownload() currently leaves the work running.
The URLSessionDownloadTask and extraction flow are both fire-and-forget. cancelDownload() only flips state, so an in-flight transfer or extraction can still finish later and overwrite the canceled state with .ready or .failed. Persist the active task/process plus a generation token, cancel/terminate them on cancel, and ignore stale completions.
Also applies to: 167-169, 173-176, 232-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/BrowserEngine/CEFFrameworkManager.swift` around lines 121 - 163, The
download task and extraction are currently fire-and-forget; modify
CEFFrameworkManager to store the active URLSessionDownloadTask (task), its
progress observation (observation) and a generation token (e.g.,
downloadGeneration) when starting a download, cancel the task and
remove/terminate the observation and any spawned extraction Process in
cancelDownload(), and increment the generation to mark cancellations; in
extractFramework(from:completion:) accept/check the same generation token (or
have it accessible) and ignore any completion callbacks or state changes if the
token is stale, and ensure any spawned Process is terminated on cancel to
prevent a late .ready/.failed state or calling completion after cancellation.
| final class CEFRuntime { | ||
|
|
||
| static let shared = CEFRuntime() | ||
|
|
||
| private var messageLoopTimer: Timer? | ||
| private(set) var isInitialized = false | ||
|
|
||
| private init() {} |
There was a problem hiding this comment.
Pin the CEF pump to RunLoop.main in .common modes.
Timer.scheduledTimer registers on the current run loop in default mode. If initialize() is ever reached off-main, the pump lands on the wrong thread; even on main, it pauses during tracking loops like menus and drags, which will stall CEF work. Make CEFRuntime main-thread-bound and install the timer on RunLoop.main with .common.
🛠 Minimal fix
+@MainActor
final class CEFRuntime {
@@
func startMessageLoop() {
guard isInitialized, messageLoopTimer == nil else { return }
- messageLoopTimer = Timer.scheduledTimer(
- withTimeInterval: 1.0 / 60.0,
- repeats: true
- ) { _ in
+ let timer = Timer(
+ timeInterval: 1.0 / 60.0,
+ repeats: true
+ ) { _ in
cef_bridge_do_message_loop_work()
}
+ RunLoop.main.add(timer, forMode: .common)
+ messageLoopTimer = timer
}Also applies to: 66-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/BrowserEngine/CEFRuntime.swift` around lines 9 - 16, The CEFRuntime
timer is being scheduled on the current run loop which can be off-main or pause
during tracking loops; update CEFRuntime to ensure it is main-thread-bound and
install messageLoopTimer on RunLoop.main with .common modes (use
RunLoop.main.add(_:forMode: .common) or equivalent) inside initialize() and any
restart logic so the timer always runs on the main run loop and in common modes;
reference CEFRuntime, initialize(), and messageLoopTimer when making the change.
| /// JavaScript dialog callback (alert, confirm, prompt). | ||
| typedef bool (*cef_bridge_jsdialog_callback)( | ||
| cef_bridge_browser_t browser, | ||
| int dialog_type, // 0=alert, 1=confirm, 2=prompt | ||
| const char* message, | ||
| const char* default_value, | ||
| void* user_data | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Chromium Embedded Framework JSDialogHandler OnJSDialog prompt dialog how to return user-entered text to CEF, and does the response flow through CefJSDialogCallback::Continue with a string?
💡 Result:
To handle a prompt dialog in Chromium Embedded Framework (CEF) using JSDialogHandler::OnJSDialog and return user-entered text: 1. Implement CefJSDialogHandler and override OnJSDialog. 2. In OnJSDialog, when dialog_type is JSDialogType::JSDIALOG_TYPE_PROMPT, show your custom UI to collect user input (e.g., a text box pre-filled with default_prompt_text). 3. Call callback->Continue(true, user_input_string) to return the text to CEF, where true indicates success (OK pressed) and user_input_string is the entered text. For cancel, call callback->Continue(false, ""). 4. Return true from OnJSDialog to indicate you've handled it (and will call the callback). Yes, the response flows through CefJSDialogCallback::Continue with a string: its signature is Continue(bool success, const CefString& user_input), where user_input is specified for prompt dialogs. Example C++ code: bool MyJSDialogHandler::OnJSDialog(CefRefPtr browser, const CefString& origin_url, JSDialogType dialog_type, const CefString& message_text, const CefString& default_prompt_text, CefRefPtr callback, bool& suppress_message) { if (dialog_type == JSDialogType::JSDIALOG_TYPE_PROMPT) { // Show custom prompt dialog, get user_input from it std::string user_input = ShowCustomPrompt(message_text.ToString, default_prompt_text.ToString); callback->Continue(!user_input.empty, user_input); return true; } return false; // Use default for other dialogs } This applies to core CEF C++ API (confirmed in docs up to recent versions like 140). CefSharp .NET wrappers follow the same pattern via IJsDialogCallback.Continue(bool, string).
Citations:
- 1: https://cef-builds.spotifycdn.com/docs/124.0/classCefJSDialogHandler.html
- 2: https://cef-builds.spotifycdn.com/docs/124.3/classCefJSDialogHandler.html
- 3: https://cef-builds.spotifycdn.com/docs/140.0/classCefJSDialogHandler.html
- 4: https://cef-builds.spotifycdn.com/docs/114.2/classCefJSDialogCallback.html
- 5: https://magpcss.org/ceforum/viewtopic.php?t=18356
prompt() can't round-trip user input through this ABI.
cef_bridge_jsdialog_callback only returns bool, but CEF's proper prompt handling requires the callback to communicate back both a success indicator and the user-entered text. CEF's CefJSDialogCallback::Continue uses Continue(bool success, const CefString& user_input) to return both values for prompt dialogs. The current signature supports alert/confirm, but needs an out-parameter or restructured response mechanism to handle prompt correctly before this API becomes part of the public ABI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/cef-bridge/cef_bridge.h` around lines 121 - 128, The current
cef_bridge_jsdialog_callback signature only returns a bool and cannot return the
user-entered text required for prompt dialogs; update the ABI so the callback
can convey both success and the prompt string (for example by changing typedef
bool (*cef_bridge_jsdialog_callback)(...) to a signature that includes an out
parameter or a struct return containing a bool success and a char* user_input),
ensure the implementation and any callers of cef_bridge_jsdialog_callback and
related uses in prompt handling correctly allocate/return the user_input string
and define ownership/cleanup semantics, and update any structs/types that
reference cef_bridge_jsdialog_callback so prompt dialogs can call the equivalent
of CefJSDialogCallback::Continue(bool success, const CefString& user_input)
through the bridge.
Replace stub implementations with real CEF C++ API calls when CEF_BRIDGE_HAS_CEF is defined. The bridge now wraps CefClient, CefLifeSpanHandler, CefLoadHandler, CefDisplayHandler, and CefKeyboardHandler for navigation callbacks, title/URL changes, loading state, popup handling, and keyboard shortcut routing. - cef_bridge.cpp: full CefClient implementation with BridgeClient class routing events to C callback struct. Falls back to stubs when compiled without CEF headers. - scripts/build-cef-bridge.sh: downloads CEF 146 arm64 minimal distribution, builds libcef_dll_wrapper with CMake/Ninja, and compiles the bridge against real CEF headers. - Makefile: supports merging libcef_dll_wrapper.a into the bridge static library when CEF_ROOT and CEF_WRAPPER_LIB are set. - project.pbxproj: added FRAMEWORK_SEARCH_PATHS to link against Chromium Embedded Framework. Build verified with real CEF framework linked.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec048307ed
ℹ️ 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".
| "-framework", | ||
| "\"Chromium Embedded Framework\"", |
There was a problem hiding this comment.
scripts/setup.sh now builds only the stub libcef_bridge.a (no CEF framework), but this target always passes -framework "Chromium Embedded Framework" to the linker. On any machine/CI runner that does not already have that framework present under vendor/cef-bridge, the app link step fails before runtime feature flags matter. This should be conditional on a real-CEF build (or weak/optional linkage) so the documented setup path remains buildable.
Useful? React with 👍 / 👎.
| "/Chromium Embedded Framework.framework/Chromium Embedded Framework"; | ||
|
|
||
| CefScopedLibraryLoader library_loader; | ||
| if (!library_loader.LoadInMain()) { |
There was a problem hiding this comment.
This initialization path ignores the downloaded framework location: framework_path is computed into fwk_path but never used, and CefScopedLibraryLoader::LoadInMain() only loads from the expected app-bundle location. Because CEFFrameworkManager extracts to Application Support, real CEF builds can still fail initialization even after a successful download. The loader path must be aligned with where the framework is actually installed.
Useful? React with 👍 / 👎.
| } | ||
| // Note: BridgeBrowser is freed after OnBeforeClose fires | ||
| // For now, just delete it | ||
| delete bb; |
There was a problem hiding this comment.
CloseBrowser(true) is asynchronous, but cef_bridge_browser_destroy immediately frees bb. BridgeClient still holds owner_ and can invoke callbacks during shutdown, so this creates a use-after-free window and potential crash when running with real CEF enabled. Keep the bridge object alive until OnBeforeClose (or equivalent final callback) confirms teardown is complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/build-cef-bridge.sh (2)
71-79: Framework symlink may break on path changes.
ln -sfncreates an absolute symlink to the cached CEF framework. If the cache location changes (e.g., differentCMUX_CEF_CACHE_DIR), the symlink will become stale. This is acceptable for local development but may cause confusion.Consider adding a note in output or using a relative symlink if the paths allow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-cef-bridge.sh` around lines 71 - 79, The link_framework function currently creates an absolute symlink using ln -sfn from fw_src to fw_dst which can become stale if CEF cache paths (e.g., CEF_EXTRACT_DIR / CMUX_CEF_CACHE_DIR) move; update link_framework to prefer creating a relative symlink when fw_src and fw_dst share a common prefix (compute a relative path from fw_dst to fw_src) or, if not possible, print a clear warning message after linking that the symlink is absolute and may break if the cache directory changes; reference the existing symbols link_framework, fw_src, fw_dst and the ln -sfn call when making the change.
24-33: Consider verifying archive integrity after download.The PR description mentions SHA256 verification in
CEFFrameworkManager, but this build script downloads the archive without verifying its checksum. If an attacker compromises the CDN or performs a MITM attack, a malicious archive could be extracted and built.Consider adding a checksum verification step, or document that integrity is verified elsewhere in the workflow.
Example integrity check
+CEF_ARCHIVE_SHA256="<expected-sha256-hash>" + download_cef() { local archive archive="$CEF_CACHE_DIR/$(basename "$CEF_DOWNLOAD_URL")" mkdir -p "$CEF_CACHE_DIR" if [ ! -f "$archive" ]; then echo "==> Downloading CEF minimal distribution..." curl -fL -o "$archive" "$CEF_DOWNLOAD_URL" --progress-bar + echo "==> Verifying archive checksum..." + echo "$CEF_ARCHIVE_SHA256 $archive" | shasum -a 256 -c - fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-cef-bridge.sh` around lines 24 - 33, Add a checksum verification step after downloading "$archive" in scripts/build-cef-bridge.sh: compute the SHA256 of "$archive" (e.g., via sha256sum or shasum -a 256) and compare it against a trusted expected value provided by an environment variable (e.g., CEF_SHA256) or a shipped checksum file; if the computed digest does not match CEF_SHA256, log an error, remove the bad archive and exit non‑zero to stop extraction (also consider failing fast on curl errors with --fail). This ensures the download of "$CEF_DOWNLOAD_URL" is integrity-checked before the extraction into "$CEF_CACHE_DIR/extracted" using "$CEF_EXTRACT_DIR".vendor/cef-bridge/Makefile (1)
15-16: arm64-only build is intentional for CEF minimal distribution.The Makefile hardcodes
-arch arm64consistently with the CEF download strategy (macosarm64minimal distribution). This is intentional—the CEF bridge is built only for arm64. If x86_64 support becomes needed later, it would require switching to a multi-arch CEF distribution.Consider adding a comment in the Makefile to document this architectural constraint for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/cef-bridge/Makefile` around lines 15 - 16, Add a short explanatory comment above the CXXFLAGS entries that set -arch arm64 and -mmacosx-version-min=13.0 stating that the CEF bridge is intentionally built only for arm64 (macosarm64 minimal CEF distribution) and that adding x86_64 would require switching to a multi-arch CEF distribution; reference the CXXFLAGS lines that contain "-arch arm64" and "-mmacosx-version-min=13.0" so future maintainers understand the architectural constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build-cef-bridge.sh`:
- Around line 20-27: In download_cef(), avoid masking the subshell return value
by separating declaration and assignment of archive (use local archive;
archive="$(basename ...)" in two steps) and make the curl call robust by adding
--fail (curl -L --fail -o "$archive" "$CEF_DOWNLOAD_URL" --progress-bar) so HTTP
errors cause a non-zero exit; update references to the local variable archive
and keep CEF_DOWNLOAD_URL unchanged.
---
Nitpick comments:
In `@scripts/build-cef-bridge.sh`:
- Around line 71-79: The link_framework function currently creates an absolute
symlink using ln -sfn from fw_src to fw_dst which can become stale if CEF cache
paths (e.g., CEF_EXTRACT_DIR / CMUX_CEF_CACHE_DIR) move; update link_framework
to prefer creating a relative symlink when fw_src and fw_dst share a common
prefix (compute a relative path from fw_dst to fw_src) or, if not possible,
print a clear warning message after linking that the symlink is absolute and may
break if the cache directory changes; reference the existing symbols
link_framework, fw_src, fw_dst and the ln -sfn call when making the change.
- Around line 24-33: Add a checksum verification step after downloading
"$archive" in scripts/build-cef-bridge.sh: compute the SHA256 of "$archive"
(e.g., via sha256sum or shasum -a 256) and compare it against a trusted expected
value provided by an environment variable (e.g., CEF_SHA256) or a shipped
checksum file; if the computed digest does not match CEF_SHA256, log an error,
remove the bad archive and exit non‑zero to stop extraction (also consider
failing fast on curl errors with --fail). This ensures the download of
"$CEF_DOWNLOAD_URL" is integrity-checked before the extraction into
"$CEF_CACHE_DIR/extracted" using "$CEF_EXTRACT_DIR".
In `@vendor/cef-bridge/Makefile`:
- Around line 15-16: Add a short explanatory comment above the CXXFLAGS entries
that set -arch arm64 and -mmacosx-version-min=13.0 stating that the CEF bridge
is intentionally built only for arm64 (macosarm64 minimal CEF distribution) and
that adding x86_64 would require switching to a multi-arch CEF distribution;
reference the CXXFLAGS lines that contain "-arch arm64" and
"-mmacosx-version-min=13.0" so future maintainers understand the architectural
constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 995d529a-c332-4d70-889a-70809143801d
📒 Files selected for processing (5)
GhosttyTabs.xcodeproj/project.pbxprojscripts/build-cef-bridge.shvendor/cef-bridge/.gitignorevendor/cef-bridge/Makefilevendor/cef-bridge/cef_bridge.cpp
✅ Files skipped from review due to trivial changes (1)
- vendor/cef-bridge/.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- vendor/cef-bridge/cef_bridge.cpp
| download_cef() { | ||
| local archive="$CEF_CACHE_DIR/$(basename "$CEF_DOWNLOAD_URL")" | ||
| mkdir -p "$CEF_CACHE_DIR" | ||
|
|
||
| if [ ! -f "$archive" ]; then | ||
| echo "==> Downloading CEF minimal distribution..." | ||
| curl -L -o "$archive" "$CEF_DOWNLOAD_URL" --progress-bar | ||
| fi |
There was a problem hiding this comment.
Declare and assign separately; add --fail to curl.
Line 21 masks the return value of the subshell. Additionally, curl without --fail will succeed even if the server returns an HTTP error (e.g., 404), writing the error page to the archive file.
Proposed fix
download_cef() {
- local archive="$CEF_CACHE_DIR/$(basename "$CEF_DOWNLOAD_URL")"
+ local archive
+ archive="$CEF_CACHE_DIR/$(basename "$CEF_DOWNLOAD_URL")"
mkdir -p "$CEF_CACHE_DIR"
if [ ! -f "$archive" ]; then
echo "==> Downloading CEF minimal distribution..."
- curl -L -o "$archive" "$CEF_DOWNLOAD_URL" --progress-bar
+ curl -fL -o "$archive" "$CEF_DOWNLOAD_URL" --progress-bar
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| download_cef() { | |
| local archive="$CEF_CACHE_DIR/$(basename "$CEF_DOWNLOAD_URL")" | |
| mkdir -p "$CEF_CACHE_DIR" | |
| if [ ! -f "$archive" ]; then | |
| echo "==> Downloading CEF minimal distribution..." | |
| curl -L -o "$archive" "$CEF_DOWNLOAD_URL" --progress-bar | |
| fi | |
| download_cef() { | |
| local archive | |
| archive="$CEF_CACHE_DIR/$(basename "$CEF_DOWNLOAD_URL")" | |
| mkdir -p "$CEF_CACHE_DIR" | |
| if [ ! -f "$archive" ]; then | |
| echo "==> Downloading CEF minimal distribution..." | |
| curl -fL -o "$archive" "$CEF_DOWNLOAD_URL" --progress-bar | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 21-21: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build-cef-bridge.sh` around lines 20 - 27, In download_cef(), avoid
masking the subshell return value by separating declaration and assignment of
archive (use local archive; archive="$(basename ...)" in two steps) and make the
curl call robust by adding --fail (curl -L --fail -o "$archive"
"$CEF_DOWNLOAD_URL" --progress-bar) so HTTP errors cause a non-zero exit; update
references to the local variable archive and keep CEF_DOWNLOAD_URL unchanged.
- CEFBrowserView: NSView subclass that hosts a CEF browser instance. Provides loadURL, goBack, goForward, reload, zoom, JS execution, DevTools, find-in-page, and portal visibility management. Delegates navigation/display events via CEFBrowserViewDelegate protocol. - CEFRuntime: updated to resolve framework and helper paths from the app bundle's Frameworks/ directory. - vendor/cef-bridge/helper/: minimal CEF helper process that calls CefExecuteProcess for renderer/GPU subprocesses. - scripts/embed-cef.sh: copies CEF framework and helper app bundle into a built cmux .app for runtime use. Build verified.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fee4695ba9
ℹ️ 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".
| let task = session.downloadTask(with: Self.downloadURL) { [weak self] tempURL, response, error in | ||
| DispatchQueue.main.async { | ||
| guard let self else { return } |
There was a problem hiding this comment.
The URLSessionDownloadTask completion handler schedules follow-up work with DispatchQueue.main.async and returns immediately, but the tempURL file is only guaranteed to exist during the completion callback. In practice this can make extractFramework(from:) run after the system has already removed the temporary archive, producing intermittent extraction failures (No such file) even for successful downloads. Move or copy the archive to a durable path before the completion handler returns.
Useful? React with 👍 / 👎.
vendor/cef-bridge/cef_bridge.cpp
Outdated
| CefScopedLibraryLoader library_loader; | ||
| if (!library_loader.LoadInMain()) { |
There was a problem hiding this comment.
CefScopedLibraryLoader is a stack-local in cef_bridge_initialize, so it is destroyed when initialization returns. When CEF is enabled, this unloads the framework while the app still expects to call CEF APIs (browser creation, message loop, shutdown), which can lead to crashes or undefined behavior. The loader needs process-lifetime storage and should only be released during shutdown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
GhosttyTabs.xcodeproj/project.pbxproj (1)
944-968:⚠️ Potential issue | 🟠 Major
libcef_bridge.ais not produced by the Xcode build graph — clean builds fail.The linker settings add
-lcef_bridgeand search paths tovendor/cef-bridge, but there's no build phase that produceslibcef_bridge.a. The pipeline failure confirms this: "library 'cef_bridge' not found". The archive is only built byscripts/setup.sh(orbuild-cef-bridge.sh), which isn't integrated into the Xcode build.Add a Run Script Build Phase (before "Frameworks") that invokes the bridge build, or create an external build target that GhosttyTabs depends on. Example Run Script phase:
"$PROJECT_DIR/scripts/build-cef-bridge.sh" --stub-onlyWith output file:
$(PROJECT_DIR)/vendor/cef-bridge/libcef_bridge.a🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@GhosttyTabs.xcodeproj/project.pbxproj` around lines 944 - 968, The Xcode project links against -lcef_bridge and uses vendor/cef-bridge in FRAMEWORK_SEARCH_PATHS/LIBRARY_SEARCH_PATHS (see OTHER_LDFLAGS and the "-lcef_bridge" entry) but no build phase produces libcef_bridge.a; add a Run Script Build Phase (placed before the "Frameworks" phase) that invokes the bridge builder (e.g. call scripts/build-cef-bridge.sh --stub-only) or add an external target that builds the bridge, and set the script’s output file to $(PROJECT_DIR)/vendor/cef-bridge/libcef_bridge.a so Xcode knows the product exists and incremental/clean builds succeed.Sources/BrowserEngine/CEFRuntime.swift (1)
80-88:⚠️ Potential issue | 🟠 MajorPin CEF message pump timer to
RunLoop.mainin.commonmodes.
Timer.scheduledTimerregisters on the current run loop in default mode. IfstartMessageLoop()is ever called off-main, the timer lands on the wrong thread. Even on main, it pauses during tracking loops (menus, drags), which can stall CEF. MakeCEFRuntimemain-thread-bound and install the timer with.commonmode.🛠️ Recommended fix
+@MainActor final class CEFRuntime { // ... func startMessageLoop() { guard isInitialized, messageLoopTimer == nil else { return } - messageLoopTimer = Timer.scheduledTimer( - withTimeInterval: 1.0 / 60.0, - repeats: true - ) { _ in + let timer = Timer(timeInterval: 1.0 / 60.0, repeats: true) { _ in cef_bridge_do_message_loop_work() } + RunLoop.main.add(timer, forMode: .common) + messageLoopTimer = timer }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserEngine/CEFRuntime.swift` around lines 80 - 88, The CEFRuntime startMessageLoop currently uses Timer.scheduledTimer which can register on the wrong run loop and pause during tracking; update startMessageLoop() to enforce main-thread usage and install the timer into RunLoop.main with .common modes: ensure CEFRuntime is main-thread-bound (e.g. assert Thread.isMainThread or dispatch to DispatchQueue.main.sync/async) before creating messageLoopTimer, create the Timer with Timer(timeInterval: 1.0/60.0, repeats: true, block: { _ in cef_bridge_do_message_loop_work() }) and add it to RunLoop.main using RunLoop.main.add(messageLoopTimer!, forMode: .common), and keep messageLoopTimer for later invalidation.
🧹 Nitpick comments (5)
vendor/cef-bridge/helper/cef_helper.cpp (1)
10-14: Add diagnostic output on library load failure.When
LoadInHelper()fails, the process exits with code 1 silently. This makes debugging deployment issues difficult since there's no indication of what went wrong. Consider logging to stderr before returning.🔧 Suggested improvement
int main(int argc, char* argv[]) { CefScopedLibraryLoader library_loader; if (!library_loader.LoadInHelper()) { + fprintf(stderr, "CEF helper: failed to load CEF library\n"); return 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/cef-bridge/helper/cef_helper.cpp` around lines 10 - 14, When CefScopedLibraryLoader::LoadInHelper() fails in main(), add diagnostic output to stderr before returning so failures are visible; use the CefScopedLibraryLoader instance (library_loader) and the LoadInHelper call site to print a concise error message (including the function name and that LoadInHelper returned false) to stderr (e.g., using fprintf(stderr, ...)) prior to returning 1 so deploy/debug issues are easier to trace.scripts/embed-cef.sh (1)
73-74: Silently ignoring codesign failures may mask deployment issues.The
|| truesuppresses all codesign errors. While ad-hoc signing (--sign -) can fail in CI environments without a signing identity, completely silencing errors makes it hard to diagnose legitimate signing problems. Consider logging the failure while still allowing the script to continue:🔧 Suggested improvement
# Sign the helper -codesign --force --sign - "$HELPER_APP" 2>/dev/null || true +if ! codesign --force --sign - "$HELPER_APP" 2>&1; then + echo "Warning: codesign failed (continuing anyway)" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/embed-cef.sh` around lines 73 - 74, The codesign command for HELPER_APP currently swallows all errors via "|| true"; change it to capture and report failures while still allowing the script to continue: run the codesign invocation for HELPER_APP, check its exit status, and if non-zero write a descriptive error to stderr (including the exit code and any available output/context) so CI logs show the failure; do not remove the ability for the script to continue, but replace the silent suppression with an explicit error message tied to HELPER_APP and the codesign command.Sources/BrowserEngine/CEFRuntime.swift (2)
9-17: Add@MainActorto enforce documented thread requirements.The documentation states "Must be called from the main thread" but there's no compile-time enforcement. Adding
@MainActorensures callers are on the main thread and makes the thread-safety contract explicit.🔧 Suggested change
+@MainActor final class CEFRuntime { static let shared = CEFRuntime() private var messageLoopTimer: Timer? private(set) var isInitialized = false private(set) var initError: String?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserEngine/CEFRuntime.swift` around lines 9 - 17, The CEFRuntime class needs compile-time enforcement of its "must be called from the main thread" contract: annotate the class with `@MainActor` so all its members (including CEFRuntime.shared, the private init(), messageLoopTimer, isInitialized and initError) are executed on the main actor; update the declaration of CEFRuntime to be `@MainActor` final class CEFRuntime and ensure any callers that previously accessed CEFRuntime from background threads are updated to call it on the main actor or via Task { `@MainActor` in ... }.
48-58: Avoid force-unwrap onapplicationSupportDirectory.Line 52 uses
.first!which will crash ifurls(for:in:)returns an empty array. While this is unlikely for.applicationSupportDirectory, defensive coding is safer.🛡️ Suggested fix
let cacheRoot: String = { - let appSupport = FileManager.default.urls( - for: .applicationSupportDirectory, - in: .userDomainMask - ).first! + guard let appSupport = FileManager.default.urls( + for: .applicationSupportDirectory, + in: .userDomainMask + ).first else { + // Fallback to temp directory + return NSTemporaryDirectory() + "CEFCache" + } let bundleID = Bundle.main.bundleIdentifier ?? "com.cmuxterm.app" return appSupport .appendingPathComponent(bundleID) .appendingPathComponent("CEFCache") .path }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserEngine/CEFRuntime.swift` around lines 48 - 58, The closure that computes cacheRoot force-unwraps the first URL from FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask) which can crash; change the cacheRoot initializer to safely unwrap that URL (e.g., guard let appSupport = FileManager.default.urls(...).first else { /* choose fallback: use NSTemporaryDirectory() or FileManager.default.homeDirectoryForCurrentUser.appendingPathComponent("Library/Application Support") */ }) and use the unwrapped appSupport to build the path (symbol: cacheRoot closure and the call to FileManager.default.urls(...).first); also consider creating the directory if missing and logging an error if fallback is used.Sources/BrowserEngine/CEFBrowserView.swift (1)
13-13: Add@MainActorto enforce thread safety documentation.The class documents "All CEF operations must happen on the main thread" but doesn't enforce it. Adding
@MainActorprovides compile-time safety and makes the contract explicit.🔧 Suggested change
+@MainActor final class CEFBrowserView: NSView {Also applies to: 60-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserEngine/CEFBrowserView.swift` at line 13, Annotate the CEFBrowserView class with `@MainActor` to enforce that all of its methods/properties run on the main thread (since it already documents that CEF operations must happen on main), e.g., add the `@MainActor` attribute above the CEFBrowserView declaration and likewise add `@MainActor` to the other related classes/declarations referenced around lines 60-64 so the compiler enforces the main-thread contract for those types as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/embed-cef.sh`:
- Around line 19-26: The script hardcodes CEF_PLATFORM and compiles the helper
for arm64 only, and it uses CEF_WRAPPER_LIB without checking its existence;
change CEF_PLATFORM to be chosen at runtime (inspect uname -m/arch) and set the
helper compile flags accordingly (support x86_64 on Intel, arm64 on Apple
Silicon, or build a universal/fat binary via lipo when needed) by updating the
variables and the compile step that currently forces -arch arm64; also add an
existence check for CEF_WRAPPER_LIB (same style as the existing $CEF_FW check)
before using it and fail with a clear error if missing so the script won’t
assume the file is present.
---
Duplicate comments:
In `@GhosttyTabs.xcodeproj/project.pbxproj`:
- Around line 944-968: The Xcode project links against -lcef_bridge and uses
vendor/cef-bridge in FRAMEWORK_SEARCH_PATHS/LIBRARY_SEARCH_PATHS (see
OTHER_LDFLAGS and the "-lcef_bridge" entry) but no build phase produces
libcef_bridge.a; add a Run Script Build Phase (placed before the "Frameworks"
phase) that invokes the bridge builder (e.g. call scripts/build-cef-bridge.sh
--stub-only) or add an external target that builds the bridge, and set the
script’s output file to $(PROJECT_DIR)/vendor/cef-bridge/libcef_bridge.a so
Xcode knows the product exists and incremental/clean builds succeed.
In `@Sources/BrowserEngine/CEFRuntime.swift`:
- Around line 80-88: The CEFRuntime startMessageLoop currently uses
Timer.scheduledTimer which can register on the wrong run loop and pause during
tracking; update startMessageLoop() to enforce main-thread usage and install the
timer into RunLoop.main with .common modes: ensure CEFRuntime is
main-thread-bound (e.g. assert Thread.isMainThread or dispatch to
DispatchQueue.main.sync/async) before creating messageLoopTimer, create the
Timer with Timer(timeInterval: 1.0/60.0, repeats: true, block: { _ in
cef_bridge_do_message_loop_work() }) and add it to RunLoop.main using
RunLoop.main.add(messageLoopTimer!, forMode: .common), and keep messageLoopTimer
for later invalidation.
---
Nitpick comments:
In `@scripts/embed-cef.sh`:
- Around line 73-74: The codesign command for HELPER_APP currently swallows all
errors via "|| true"; change it to capture and report failures while still
allowing the script to continue: run the codesign invocation for HELPER_APP,
check its exit status, and if non-zero write a descriptive error to stderr
(including the exit code and any available output/context) so CI logs show the
failure; do not remove the ability for the script to continue, but replace the
silent suppression with an explicit error message tied to HELPER_APP and the
codesign command.
In `@Sources/BrowserEngine/CEFBrowserView.swift`:
- Line 13: Annotate the CEFBrowserView class with `@MainActor` to enforce that all
of its methods/properties run on the main thread (since it already documents
that CEF operations must happen on main), e.g., add the `@MainActor` attribute
above the CEFBrowserView declaration and likewise add `@MainActor` to the other
related classes/declarations referenced around lines 60-64 so the compiler
enforces the main-thread contract for those types as well.
In `@Sources/BrowserEngine/CEFRuntime.swift`:
- Around line 9-17: The CEFRuntime class needs compile-time enforcement of its
"must be called from the main thread" contract: annotate the class with
`@MainActor` so all its members (including CEFRuntime.shared, the private init(),
messageLoopTimer, isInitialized and initError) are executed on the main actor;
update the declaration of CEFRuntime to be `@MainActor` final class CEFRuntime and
ensure any callers that previously accessed CEFRuntime from background threads
are updated to call it on the main actor or via Task { `@MainActor` in ... }.
- Around line 48-58: The closure that computes cacheRoot force-unwraps the first
URL from FileManager.default.urls(for: .applicationSupportDirectory, in:
.userDomainMask) which can crash; change the cacheRoot initializer to safely
unwrap that URL (e.g., guard let appSupport =
FileManager.default.urls(...).first else { /* choose fallback: use
NSTemporaryDirectory() or
FileManager.default.homeDirectoryForCurrentUser.appendingPathComponent("Library/Application
Support") */ }) and use the unwrapped appSupport to build the path (symbol:
cacheRoot closure and the call to FileManager.default.urls(...).first); also
consider creating the directory if missing and logging an error if fallback is
used.
In `@vendor/cef-bridge/helper/cef_helper.cpp`:
- Around line 10-14: When CefScopedLibraryLoader::LoadInHelper() fails in
main(), add diagnostic output to stderr before returning so failures are
visible; use the CefScopedLibraryLoader instance (library_loader) and the
LoadInHelper call site to print a concise error message (including the function
name and that LoadInHelper returned false) to stderr (e.g., using
fprintf(stderr, ...)) prior to returning 1 so deploy/debug issues are easier to
trace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d708e64e-36a6-466c-affa-4f390effa9c2
📒 Files selected for processing (7)
GhosttyTabs.xcodeproj/project.pbxprojSources/BrowserEngine/CEFBrowserView.swiftSources/BrowserEngine/CEFRuntime.swiftscripts/embed-cef.shvendor/cef-bridge/helper/Info.plistvendor/cef-bridge/helper/cef_helper.cppvendor/cef-bridge/helper/cmux_helper
✅ Files skipped from review due to trivial changes (1)
- vendor/cef-bridge/helper/Info.plist
- BrowserProfileDefinition: added engineType field (.webkit default, backward compatible with existing Codable data). - BrowserProfileStore.createProfile: accepts engineType parameter. - BrowserPanel.init: resolves engineType from profile definition. For Chromium profiles, also creates a CEFBrowserView and initializes CEF runtime. The WKWebView is still created (to avoid breaking existing code paths), but the view layer will display the CEF view. Build verified.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59b55b5e30
ℹ️ 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".
| var displayName: String | ||
| let createdAt: Date | ||
| let isBuiltInDefault: Bool | ||
| var engineType: BrowserEngineType = .webkit |
There was a problem hiding this comment.
Adding engineType as a non-optional property on a synthesized Codable model breaks backward decoding for already-persisted profile JSON that lacks this key. In BrowserProfileStore.load(), that decode failure falls through to the else branch and immediately calls persist(), which overwrites stored profiles with only the built-in default profile, causing user profile data loss on upgrade from older builds.
Useful? React with 👍 / 👎.
| // MARK: - Configuration | ||
|
|
||
| /// CEF version pinned for this release of cmux. | ||
| static let cefVersion = "130.1.16+g03e8e4e+chromium-130.0.6723.117" |
There was a problem hiding this comment.
This runtime manager is pinned to CEF 130 (cefVersion and downloadURL), while the new bridge/tooling paths are pinned to CEF 146 (scripts/build-cef-bridge.sh and cef_bridge_get_version). If the bridge is compiled against 146 but runtime download installs 130, the app can initialize/load against an ABI-incompatible framework once real CEF is enabled, leading to init failures or crashes.
Useful? React with 👍 / 👎.
- BrowserPanelView: branches on panel.engineType to show either the existing WebViewRepresentable (WebKit) or the new CEFBrowserViewRepresentable (Chromium). Falls back to CEFDownloadView if CEF isn't initialized. - CEFBrowserViewRepresentable: NSViewRepresentable that hosts a CEFBrowserView in SwiftUI. - AppDelegate: added openChromiumBrowserAndFocusAddressBar() that finds or creates a Chromium profile and opens a browser tab. - ContentView: added "New Tab (Chromium Browser)" command palette entry (palette.newChromiumBrowserTab). Users can now open a Chromium-engine browser tab via the command palette. The tab shows the CEF download UI if the framework isn't available, or renders pages via Chromium if it is.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/Panels/BrowserPanel.swift (2)
2592-2642:⚠️ Potential issue | 🔴 CriticalThis exposes a Chromium surface before
BrowserPanelis engine-aware.This branch creates a visible
CEFBrowserView, but the panel still routes navigation and state throughwebView(performNavigation(),goBack()/goForward(),reload(), JS evaluation, find-in-page, zoom, loading/title observers, etc.), andcefView.delegateis never wired here. After the initialcreateBrowser(...), the visible Chromium view and the panel model will diverge immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 2592 - 2642, The CEFBrowserView is being created and shown before the panel switches its navigation/state handling from WKWebView to Chromium, causing divergence; when engineType == .chromium, delay exposing/using the CEFBrowserView until CEFRuntime is initialized and the panel is fully engine-aware, set cefBrowserView only after wiring the delegate and routing methods, and ensure you wire cefView.delegate (e.g. to self or a dedicated CEFBrowserDelegate) immediately after creating cefView and implement/forward navigation and state APIs (performNavigation, goBack, goForward, reload, JS evaluation, find-in-page, zoom, loading/title observers) so those calls are routed to the CEF browser when engineType == .chromium; if CEFRuntime fails to initialize, fall back to keeping webView as the authoritative view and do not create or present the CEFBrowserView.
320-325:⚠️ Potential issue | 🟠 MajorAdd explicit migration path for
engineTypeto prevent data loss on older profiles.
BrowserProfileDefinitionadds a new non-optional fieldvar engineType: BrowserEngineType = .webkit. In Swift, the synthesizedDecodabledecoder throwskeyNotFoundwhen decoding older payloads that omit this field—the default value in the struct definition does not apply during decoding. Sinceload()silently falls back to the built-in default on decode failure, existing user profiles will be dropped.Implement a custom
init(from:)that usesdecodeIfPresent(_:forKey:)forengineType, defaulting to.webkitif the key is missing:init(from decoder: Decoder) throws { let container = try decoder.container(keyedBy: CodingKeys.self) id = try container.decode(UUID.self, forKey: .id) displayName = try container.decode(String.self, forKey: .displayName) createdAt = try container.decode(Date.self, forKey: .createdAt) isBuiltInDefault = try container.decode(Bool.self, forKey: .isBuiltInDefault) engineType = try container.decodeIfPresent(BrowserEngineType.self, forKey: .engineType) ?? .webkit }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 320 - 325, Implement a custom Decodable initializer on BrowserProfileDefinition to avoid keyNotFound for the new engineType field: add init(from decoder: Decoder) that creates a keyed container (keyedBy: CodingKeys), decodes id, displayName, createdAt and isBuiltInDefault as before, and sets engineType using decodeIfPresent(BrowserEngineType.self, forKey: .engineType) ?? .webkit so older profiles without the key are migrated to .webkit; ensure CodingKeys includes engineType if needed.
🤖 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/Panels/BrowserPanel.swift`:
- Around line 378-387: The createProfile(named:engineType:) function currently
defaults engineType to .webkit which masks user selection; remove the default so
engineType is required (change signature to createProfile(named: String,
engineType: BrowserEngineType) -> BrowserProfileDefinition?) and update all
callers (e.g., the profile-creation flow in BrowserPanelView that currently
calls createProfile(named:)) to pass the selected BrowserEngineType from the UI,
ensuring the BrowserProfileDefinition is constructed with the explicit
engineType.
- Around line 1870-1878: The panel currently keeps engineType as an immutable
let and only creates cefBrowserView in init, so calling switchToProfile(_:)
(which replaces profileID, historyStore and the WKWebView) can leave the wrong
engine/view alive when the new profile requires a different engine; fix this by
making engineType mutable (change let engineType: BrowserEngineType to var
engineType: BrowserEngineType) and update switchToProfile(_:) to detect the new
engine for the incoming profile, update engineType, tear down or nil out the old
cefBrowserView when switching from Chromium→WebKit, and lazily create or replace
cefBrowserView when switching to Chromium (while also recreating the appropriate
webView/dataStore), ensuring webView, cefBrowserView and engineType stay
consistent after the profile switch.
---
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 2592-2642: The CEFBrowserView is being created and shown before
the panel switches its navigation/state handling from WKWebView to Chromium,
causing divergence; when engineType == .chromium, delay exposing/using the
CEFBrowserView until CEFRuntime is initialized and the panel is fully
engine-aware, set cefBrowserView only after wiring the delegate and routing
methods, and ensure you wire cefView.delegate (e.g. to self or a dedicated
CEFBrowserDelegate) immediately after creating cefView and implement/forward
navigation and state APIs (performNavigation, goBack, goForward, reload, JS
evaluation, find-in-page, zoom, loading/title observers) so those calls are
routed to the CEF browser when engineType == .chromium; if CEFRuntime fails to
initialize, fall back to keeping webView as the authoritative view and do not
create or present the CEFBrowserView.
- Around line 320-325: Implement a custom Decodable initializer on
BrowserProfileDefinition to avoid keyNotFound for the new engineType field: add
init(from decoder: Decoder) that creates a keyed container (keyedBy:
CodingKeys), decodes id, displayName, createdAt and isBuiltInDefault as before,
and sets engineType using decodeIfPresent(BrowserEngineType.self, forKey:
.engineType) ?? .webkit so older profiles without the key are migrated to
.webkit; ensure CodingKeys includes engineType if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f7b1a9a-be1c-4aa1-b4e0-781aeb4e7ef3
📒 Files selected for processing (1)
Sources/Panels/BrowserPanel.swift
| /// The browser engine type for this panel. | ||
| let engineType: BrowserEngineType | ||
|
|
||
| /// The underlying web view | ||
| private(set) var webView: WKWebView | ||
| private var websiteDataStore: WKWebsiteDataStore | ||
|
|
||
| /// The underlying CEF browser view (Chromium engine only, nil for WebKit). | ||
| private(set) var cefBrowserView: CEFBrowserView? |
There was a problem hiding this comment.
engineType is frozen for the lifetime of the panel.
switchToProfile(_:) changes profileID, historyStore, and the backing WKWebView, but engineType is a let and cefBrowserView is only created in init. Switching an existing tab between WebKit and Chromium will keep the old engine/view alive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanel.swift` around lines 1870 - 1878, The panel
currently keeps engineType as an immutable let and only creates cefBrowserView
in init, so calling switchToProfile(_:) (which replaces profileID, historyStore
and the WKWebView) can leave the wrong engine/view alive when the new profile
requires a different engine; fix this by making engineType mutable (change let
engineType: BrowserEngineType to var engineType: BrowserEngineType) and update
switchToProfile(_:) to detect the new engine for the incoming profile, update
engineType, tear down or nil out the old cefBrowserView when switching from
Chromium→WebKit, and lazily create or replace cefBrowserView when switching to
Chromium (while also recreating the appropriate webView/dataStore), ensuring
webView, cefBrowserView and engineType stay consistent after the profile switch.
When the CEF distribution is available in the cache, reload.sh now automatically embeds the framework and helper process into the built app bundle before launching. This enables Chromium browser tabs in tagged dev builds without manual steps.
There was a problem hiding this comment.
12 issues found across 17 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:9917">
P2: Localize the default Chromium profile name instead of hard-coding an English string.</violation>
</file>
<file name="scripts/build-cef-bridge.sh">
<violation number="1" location="scripts/build-cef-bridge.sh:26">
P2: Add `--fail` to `curl` so HTTP error responses do not get cached as valid CEF archives.</violation>
<violation number="2" location="scripts/build-cef-bridge.sh:59">
P2: `bridge` mode can fail when CEF is extracted but `libcef_dll_wrapper.a` is missing; check wrapper existence before selecting real-CEF build path.</violation>
</file>
<file name="scripts/embed-cef.sh">
<violation number="1" location="scripts/embed-cef.sh:40">
P2: Always replace the embedded CEF framework; skipping when it already exists can leave stale versions in the app bundle.</violation>
<violation number="2" location="scripts/embed-cef.sh:49">
P2: Rebuild invalidation is too narrow: only checking file existence can embed an outdated helper binary after source/dependency changes.</violation>
<violation number="3" location="scripts/embed-cef.sh:74">
P1: Do not suppress `codesign` errors; ignoring signing failures can ship a broken helper bundle undetected.</violation>
</file>
<file name="Sources/Panels/BrowserPanel.swift">
<violation number="1" location="Sources/Panels/BrowserPanel.swift:325">
P1: Adding `engineType` as a new non-optional Codable field without a decoding migration can make existing saved profiles fail to decode and be reset to defaults.</violation>
</file>
<file name="scripts/reload.sh">
<violation number="1" location="scripts/reload.sh:342">
P2: The CEF cache probe is version-pinned in `reload.sh`, so a CEF version/platform update can silently skip embedding. Use a dynamic probe instead of a hard-coded dist path.</violation>
</file>
<file name="GhosttyTabs.xcodeproj/project.pbxproj">
<violation number="1" location="GhosttyTabs.xcodeproj/project.pbxproj:961">
P1: Hard-linking `Chromium Embedded Framework` in `OTHER_LDFLAGS` breaks the documented stub/default setup where CEF is not bundled or preinstalled.</violation>
</file>
<file name="Sources/BrowserEngine/CEFBrowserView.swift">
<violation number="1" location="Sources/BrowserEngine/CEFBrowserView.swift:137">
P2: Clean up profile/callback resources when browser creation fails to avoid leaking native handles on retry.</violation>
<violation number="2" location="Sources/BrowserEngine/CEFBrowserView.swift:271">
P2: Return the actual `makeFirstResponder` result instead of always returning `true`.</violation>
</file>
<file name="Sources/BrowserEngine/CEFRuntime.swift">
<violation number="1" location="Sources/BrowserEngine/CEFRuntime.swift:36">
P2: Localize the new `initError` messages instead of hard-coded English strings.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| cp "$HELPER_SRC/Info.plist" "$HELPER_APP/Contents/Info.plist" | ||
|
|
||
| # Sign the helper | ||
| codesign --force --sign - "$HELPER_APP" 2>/dev/null || true |
There was a problem hiding this comment.
P1: Do not suppress codesign errors; ignoring signing failures can ship a broken helper bundle undetected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/embed-cef.sh, line 74:
<comment>Do not suppress `codesign` errors; ignoring signing failures can ship a broken helper bundle undetected.</comment>
<file context>
@@ -0,0 +1,78 @@
+cp "$HELPER_SRC/Info.plist" "$HELPER_APP/Contents/Info.plist"
+
+# Sign the helper
+codesign --force --sign - "$HELPER_APP" 2>/dev/null || true
+
+echo "==> CEF embedded successfully"
</file context>
| var displayName: String | ||
| let createdAt: Date | ||
| let isBuiltInDefault: Bool | ||
| var engineType: BrowserEngineType = .webkit |
There was a problem hiding this comment.
P1: Adding engineType as a new non-optional Codable field without a decoding migration can make existing saved profiles fail to decode and be reset to defaults.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/BrowserPanel.swift, line 325:
<comment>Adding `engineType` as a new non-optional Codable field without a decoding migration can make existing saved profiles fail to decode and be reset to defaults.</comment>
<file context>
@@ -322,6 +322,7 @@ struct BrowserProfileDefinition: Codable, Hashable, Identifiable, Sendable {
var displayName: String
let createdAt: Date
let isBuiltInDefault: Bool
+ var engineType: BrowserEngineType = .webkit
var slug: String {
</file context>
| if let existing = store.profiles.first(where: { $0.engineType == .chromium }) { | ||
| chromiumProfile = existing | ||
| } else { | ||
| guard let created = store.createProfile(named: "Chromium", engineType: .chromium) else { |
There was a problem hiding this comment.
P2: Localize the default Chromium profile name instead of hard-coding an English string.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/AppDelegate.swift, line 9917:
<comment>Localize the default Chromium profile name instead of hard-coding an English string.</comment>
<file context>
@@ -9904,6 +9904,32 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
+ if let existing = store.profiles.first(where: { $0.engineType == .chromium }) {
+ chromiumProfile = existing
+ } else {
+ guard let created = store.createProfile(named: "Chromium", engineType: .chromium) else {
+ return nil
+ }
</file context>
| guard let created = store.createProfile(named: "Chromium", engineType: .chromium) else { | |
| guard let created = store.createProfile( | |
| named: String(localized: "browser.profile.chromium.defaultName", defaultValue: "Chromium"), | |
| engineType: .chromium | |
| ) else { |
|
|
||
| if [ ! -f "$archive" ]; then | ||
| echo "==> Downloading CEF minimal distribution..." | ||
| curl -L -o "$archive" "$CEF_DOWNLOAD_URL" --progress-bar |
There was a problem hiding this comment.
P2: Add --fail to curl so HTTP error responses do not get cached as valid CEF archives.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/build-cef-bridge.sh, line 26:
<comment>Add `--fail` to `curl` so HTTP error responses do not get cached as valid CEF archives.</comment>
<file context>
@@ -0,0 +1,109 @@
+
+ if [ ! -f "$archive" ]; then
+ echo "==> Downloading CEF minimal distribution..."
+ curl -L -o "$archive" "$CEF_DOWNLOAD_URL" --progress-bar
+ fi
+
</file context>
| curl -L -o "$archive" "$CEF_DOWNLOAD_URL" --progress-bar | |
| curl -fL -o "$archive" "$CEF_DOWNLOAD_URL" --progress-bar |
| CEF_FW_PROBE="$CEF_CACHE_DIR/extracted/cef_binary_146.0.6+g68649e2+chromium-146.0.7680.154_macosarm64_minimal/Release/Chromium Embedded Framework.framework" | ||
| if [[ -d "$CEF_FW_PROBE" ]]; then |
There was a problem hiding this comment.
P2: The CEF cache probe is version-pinned in reload.sh, so a CEF version/platform update can silently skip embedding. Use a dynamic probe instead of a hard-coded dist path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/reload.sh, line 342:
<comment>The CEF cache probe is version-pinned in `reload.sh`, so a CEF version/platform update can silently skip embedding. Use a dynamic probe instead of a hard-coded dist path.</comment>
<file context>
@@ -335,6 +335,16 @@ if [[ -z "${APP_PATH}" || ! -d "${APP_PATH}" ]]; then
+EMBED_CEF_SCRIPT="$(dirname "$0")/embed-cef.sh"
+if [[ -x "$EMBED_CEF_SCRIPT" ]]; then
+ CEF_CACHE_DIR="${CMUX_CEF_CACHE_DIR:-$HOME/.cache/cmux/cef}"
+ CEF_FW_PROBE="$CEF_CACHE_DIR/extracted/cef_binary_146.0.6+g68649e2+chromium-146.0.7680.154_macosarm64_minimal/Release/Chromium Embedded Framework.framework"
+ if [[ -d "$CEF_FW_PROBE" ]]; then
+ "$EMBED_CEF_SCRIPT" "$APP_PATH" || echo "warning: CEF embedding failed (non-fatal)"
</file context>
| CEF_FW_PROBE="$CEF_CACHE_DIR/extracted/cef_binary_146.0.6+g68649e2+chromium-146.0.7680.154_macosarm64_minimal/Release/Chromium Embedded Framework.framework" | |
| if [[ -d "$CEF_FW_PROBE" ]]; then | |
| CEF_FW_PROBE="$(find "$CEF_CACHE_DIR/extracted" -maxdepth 3 -type d -path "*/Release/Chromium Embedded Framework.framework" -print -quit 2>/dev/null || true)" | |
| if [[ -n "$CEF_FW_PROBE" ]]; then |
| // Find the framework directory (parent of the .framework bundle) | ||
| let frameworkDir = resolveFrameworkDir() | ||
| guard let frameworkDir else { | ||
| initError = "CEF framework not found in app bundle or download cache" |
There was a problem hiding this comment.
P2: Localize the new initError messages instead of hard-coded English strings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/BrowserEngine/CEFRuntime.swift, line 36:
<comment>Localize the new `initError` messages instead of hard-coded English strings.</comment>
<file context>
@@ -12,57 +12,71 @@ final class CEFRuntime {
+ // Find the framework directory (parent of the .framework bundle)
+ let frameworkDir = resolveFrameworkDir()
+ guard let frameworkDir else {
+ initError = "CEF framework not found in app bundle or download cache"
+ return false
+ }
</file context>
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 `@Sources/Panels/BrowserPanelView.swift`:
- Around line 1201-1218: The UI currently prefers showing CEFDownloadView when
CEFRuntime.shared.isInitialized is false, which skips displaying
CEFRuntime.shared.initError; change the branching in BrowserPanelView to check
CEFRuntime.shared.initError before falling back to CEFDownloadView (or refactor
CEFRuntime into an explicit state enum like .downloading / .ready / .failed and
consume that in the view). Specifically, update the conditional logic around
CEFRuntime.shared.isInitialized and CEFRuntime.shared.initError so that if
initError is non-nil the error VStack renders (showing the localized
"cef.error.title" and Text(initError)) instead of the download view, ensuring
the CEFDownloadView is only shown when there is no initError and the runtime is
not initialized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86430ef9-d097-4469-a2fe-d33545fa7a69
📒 Files selected for processing (5)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/BrowserEngine/CEFBrowserViewRepresentable.swiftSources/ContentView.swiftSources/Panels/BrowserPanelView.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/BrowserEngine/CEFBrowserViewRepresentable.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f6b56bd01
ℹ️ 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 !CEFRuntime.shared.isInitialized { | ||
| CEFRuntime.shared.initialize() | ||
| } | ||
|
|
||
| if CEFRuntime.shared.isInitialized { |
There was a problem hiding this comment.
This Chromium bootstrap path only attempts CEFRuntime.shared.initialize() once during panel construction, and createBrowser(...) runs only if that first attempt already succeeded. On a clean install where CEF is downloaded from the in-app prompt, initialization initially fails (framework absent), and there is no follow-up path to re-run init/create after CEFFrameworkManager reaches .ready (the download view’s completion is a no-op), so the tab stays on the download/ready screen until the panel/app is recreated.
Useful? React with 👍 / 👎.
| if let bundleFW = Bundle.main.privateFrameworksPath { | ||
| let candidate = (bundleFW as NSString) | ||
| .appendingPathComponent("cmux Helper.app/Contents/MacOS/cmux Helper") | ||
| if FileManager.default.fileExists(atPath: candidate) { |
There was a problem hiding this comment.
resolveHelperPath() only looks for cmux Helper.app inside the app bundle’s Frameworks directory, but this commit does not add a normal Xcode build phase to copy that helper (it is only injected by the developer-only scripts/reload.sh path). As a result, regular xcodebuild/release artifacts will hit "CEF helper process not found", so Chromium initialization cannot succeed even after the framework download completes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e5e56377a
ℹ️ 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 [ ! -f "$HELPER_BIN" ]; then | ||
| echo "==> Building helper process..." | ||
| clang++ -std=c++20 -arch arm64 -mmacosx-version-min=13.0 \ |
There was a problem hiding this comment.
embed-cef.sh only compiles the helper when $HELPER_BIN is missing, but this commit also checks in vendor/cef-bridge/helper/cmux_helper, so normal runs skip rebuilding. In any build where that checked-in helper slice does not match the app’s target architecture (for example x86_64/universal builds), the copied helper cannot launch as a CEF subprocess at runtime. Please rebuild (or at least validate architecture) per embed invocation instead of gating solely on file existence.
Useful? React with 👍 / 👎.
| let err = NSError( | ||
| domain: "CEFFrameworkManager", | ||
| code: -1, | ||
| userInfo: [NSLocalizedDescriptionKey: "Download produced no file"] |
There was a problem hiding this comment.
This error string is hardcoded English, and CEFDownloadView surfaces failed(message) directly to users. As a result, download/extraction failures show untranslated text in non-English locales, which violates the project rule that all user-facing strings must be localized through Localizable.xcstrings.
Useful? React with 👍 / 👎.
The Xcode build links against a stub libcef_bridge.a (no CEF code) to avoid linker errors from the CEF wrapper's system symbol references. The real CEF bridge is built as a separate dylib (libcef_bridge.dylib) and embedded in the app bundle by embed-cef.sh. At runtime, CEFRuntime.initialize() calls dlopen on the dylib before calling any bridge functions. The dylib's symbols override the stub implementations, enabling real CEF rendering. This cleanly separates build-time (stub) from runtime (real CEF) and avoids the Xcode 26 linker compatibility issues.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdf47d1873
ℹ️ 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".
| let url = initialURL?.absoluteString ?? "https://www.google.com" | ||
| cefView.createBrowser(initialURL: url, cachePath: nil) |
There was a problem hiding this comment.
When engineType == .chromium, this initializer now creates a CEF browser with initialURL, but the existing initializer flow still performs the normal navigate(to:) path later, which drives the hidden WKWebView. For tabs opened with a non-nil initial URL (e.g. opening a link in a Chromium profile), this causes duplicate navigation in the wrong engine and can desynchronize omnibar/history/loading state from the visible Chromium surface. Gate initial navigation by engine so Chromium tabs don't trigger the WebKit load path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/BrowserEngine/CEFRuntime.swift">
<violation number="1" location="Sources/BrowserEngine/CEFRuntime.swift:37">
P2: Do not overwrite `initError` after `loadBridgeDylib()` fails; it masks specific `dlopen` errors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // Load the real CEF bridge dylib (replaces stub functions). | ||
| // The dylib is embedded in the app bundle by embed-cef.sh. | ||
| if !loadBridgeDylib() { | ||
| initError = "CEF bridge dylib not found in app bundle" |
There was a problem hiding this comment.
P2: Do not overwrite initError after loadBridgeDylib() fails; it masks specific dlopen errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/BrowserEngine/CEFRuntime.swift, line 37:
<comment>Do not overwrite `initError` after `loadBridgeDylib()` fails; it masks specific `dlopen` errors.</comment>
<file context>
@@ -31,6 +31,13 @@ final class CEFRuntime {
+ // Load the real CEF bridge dylib (replaces stub functions).
+ // The dylib is embedded in the app bundle by embed-cef.sh.
+ if !loadBridgeDylib() {
+ initError = "CEF bridge dylib not found in app bundle"
+ return false
+ }
</file context>
| initError = "CEF bridge dylib not found in app bundle" | |
| if initError == nil { initError = "CEF bridge dylib not found in app bundle" } |
- Added small "Chromium" badge in the bottom-right corner of Chromium browser panels so users can distinguish engine type. - Fixed input forwarding: CEFBrowserView.mouseDown now makes the CEF child view first responder and forwards the click event. Added acceptsFirstMouse so clicks work even when the pane isn't focused. This enables clicking links, typing in forms, and keyboard shortcuts in Chromium tabs.
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Panels/BrowserPanelView.swift">
<violation number="1" location="Sources/Panels/BrowserPanelView.swift:1202">
P2: Localize the new user-facing "Chromium" badge text instead of using a raw string literal.</violation>
<violation number="2" location="Sources/Panels/BrowserPanelView.swift:1208">
P2: The Chromium badge overlay can block clicks in the bottom-right of the browser view; make the badge non-interactive so web content remains clickable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ZStack(alignment: .bottomTrailing) { | ||
| CEFBrowserViewRepresentable(cefBrowserView: cefView) | ||
| .accessibilityIdentifier("ChromiumBrowserView") | ||
| Text("Chromium") |
There was a problem hiding this comment.
P2: Localize the new user-facing "Chromium" badge text instead of using a raw string literal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/BrowserPanelView.swift, line 1202:
<comment>Localize the new user-facing "Chromium" badge text instead of using a raw string literal.</comment>
<file context>
@@ -1196,8 +1196,17 @@ struct BrowserPanelView: View {
+ ZStack(alignment: .bottomTrailing) {
+ CEFBrowserViewRepresentable(cefBrowserView: cefView)
+ .accessibilityIdentifier("ChromiumBrowserView")
+ Text("Chromium")
+ .font(.system(size: 9, weight: .medium))
+ .foregroundStyle(.secondary)
</file context>
| Text("Chromium") | |
| Text(String(localized: "browser.engine.chromium.badge", defaultValue: "Chromium")) |
| .padding(.horizontal, 6) | ||
| .padding(.vertical, 2) | ||
| .background(.ultraThinMaterial, in: RoundedRectangle(cornerRadius: 4)) | ||
| .padding(8) |
There was a problem hiding this comment.
P2: The Chromium badge overlay can block clicks in the bottom-right of the browser view; make the badge non-interactive so web content remains clickable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/BrowserPanelView.swift, line 1208:
<comment>The Chromium badge overlay can block clicks in the bottom-right of the browser view; make the badge non-interactive so web content remains clickable.</comment>
<file context>
@@ -1196,8 +1196,17 @@ struct BrowserPanelView: View {
+ .padding(.horizontal, 6)
+ .padding(.vertical, 2)
+ .background(.ultraThinMaterial, in: RoundedRectangle(cornerRadius: 4))
+ .padding(8)
+ }
} else if !CEFRuntime.shared.isInitialized {
</file context>
| .padding(8) | |
| .padding(8) | |
| .allowsHitTesting(false) |
- autoFocusOmnibarIfBlank() now skips Chromium panels, preventing the address bar from stealing focus away from the CEF view. - DevTools button now calls CEFBrowserView.showDevTools() for Chromium panels, opening Chromium's built-in inspector in a separate window.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2021527ec0
ℹ️ 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".
| self.state = .extracting | ||
| self.extractFramework(from: tempURL, completion: completion) |
There was a problem hiding this comment.
This path moves straight from a completed download to extraction without validating the archive hash, even though expectedSHA256 is defined for this purpose. In the current flow, a corrupted or tampered download can still be unpacked into Application Support and then consumed by CEF initialization, which is a security and reliability risk. Add a checksum verification step (and fail when it is missing or mismatched) before switching to .extracting.
Useful? React with 👍 / 👎.
| ZStack(alignment: .bottomTrailing) { | ||
| CEFBrowserViewRepresentable(cefBrowserView: cefView) | ||
| .accessibilityIdentifier("ChromiumBrowserView") | ||
| Text("Chromium") |
There was a problem hiding this comment.
The "Chromium" overlay text is user-facing but hardcoded, so it will not be translated for non-English locales. This violates the repo’s localization policy for UI strings and creates inconsistent language in Chromium tabs. Please route this through String(localized:...) with entries in Localizable.xcstrings.
Useful? React with 👍 / 👎.
Instead of using Alloy runtime (bare web view) and trying to wire cmux's address bar to CEF, use Chrome runtime which provides its own built-in omnibar, back/forward buttons, and context menus. - Hide cmux's address bar for Chromium panels (Chrome UI has its own) - CEF creates a Chrome window, then we reparent its content view into cmux's panel view hierarchy - cef_bridge_browser_get_nsview walks up to the window's contentView to get the full Chrome UI (not just the web content) - CEF's Chrome window is hidden after reparenting
- Added --hide-tabstrip flag to remove Chrome's tab bar (cmux manages tabs via its sidebar) - After reparenting Chrome's content view into cmux's panel, notify CEF with WasHidden(false) + WasResized() so the compositor updates for the new window context
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdb7e83bad
ℹ️ 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".
| func cancelDownload() { | ||
| state = .notDownloaded |
There was a problem hiding this comment.
cancelDownload() only resets state and never cancels the URLSessionDownloadTask started in download(). If a user clicks Cancel while downloading, the network task still runs and can later move the manager to .extracting/.ready, which installs the framework despite explicit cancellation and leaves UI state inconsistent with actual work. Persist the active task and call cancel() (and treat cancellation as a non-failure terminal path).
Useful? React with 👍 / 👎.
| if panel.engineType == .chromium { | ||
| // CEF DevTools opens in a separate window via Chromium's built-in inspector | ||
| panel.cefBrowserView?.showDevTools() | ||
| return |
There was a problem hiding this comment.
This CEF-specific devtools path is only reachable through openDevTools() (the address-bar button handler), but Chromium panels hide the address bar in this commit, so users lose that entry point. The existing menu/keyboard toggle still routes to BrowserPanel.toggleDeveloperTools() (WebKit inspector path), so Chromium tabs have no reliable way to toggle CEF DevTools.
Useful? React with 👍 / 👎.
Reverted from Chrome runtime reparenting (which caused crashes and frozen content) back to Alloy runtime with parent_view. Wired cmux's address bar to CEF: - navigate(to:) routes to CEFBrowserView.loadURL for Chromium panels - goBack/goForward route to CEFBrowserView - CEF URL/title/canGoBack/canGoForward changes flow back to BrowserPanel via Combine subscriptions, updating the address bar and sidebar tab title in real time
Added a catch-all guard in setAddressBarFocused that blocks all auto-focus reasons for Chromium panels except explicit user taps on the omnibar. The main offender was "request.apply" fired by openChromiumBrowserAndFocusAddressBar -> focusBrowserAddressBar. Added debug logging (cef.focus.blocked) to track blocked attempts.
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Panels/BrowserPanel.swift">
<violation number="1" location="Sources/Panels/BrowserPanel.swift:3671">
P1: Chromium navigation returns before the shared security/history flow, bypassing insecure-HTTP warning checks and typed-navigation recording.</violation>
</file>
<file name="Sources/Panels/BrowserPanelView.swift">
<violation number="1" location="Sources/Panels/BrowserPanelView.swift:1300">
P2: Cmd+L/address-bar focus requests use `reason: "request.apply"`, but the new Chromium guard only allows `"omnibar.tap"`, so Chromium tabs can no longer focus the address bar via Cmd+L or other explicit focus requests. Allow `request.apply` (or explicit focus reasons) through this guard.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if engineType == .chromium, let cefView = cefBrowserView { | ||
| shouldRenderWebView = true | ||
| cefView.loadURL(url.absoluteString) | ||
| return | ||
| } | ||
| let request = URLRequest(url: url) | ||
| if shouldBlockInsecureHTTPNavigation(to: url) { | ||
| presentInsecureHTTPAlert(for: request, intent: .currentTab, recordTypedNavigation: recordTypedNavigation) |
There was a problem hiding this comment.
P1: Chromium navigation returns before the shared security/history flow, bypassing insecure-HTTP warning checks and typed-navigation recording.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/BrowserPanel.swift, line 3671:
<comment>Chromium navigation returns before the shared security/history flow, bypassing insecure-HTTP warning checks and typed-navigation recording.</comment>
<file context>
@@ -3641,6 +3667,12 @@ final class BrowserPanel: Panel, ObservableObject {
/// Navigate to a URL
func navigate(to url: URL, recordTypedNavigation: Bool = false) {
+ // For Chromium panels, navigate via CEF instead of WebKit
+ if engineType == .chromium, let cefView = cefBrowserView {
+ shouldRenderWebView = true
+ cefView.loadURL(url.absoluteString)
</file context>
Instead of reparenting Chrome's content view (breaks compositor) or using Alloy runtime (no Chrome UI), keep CEF's Chrome window as a child window of cmux's main window. The child window is positioned to overlay the panel area and moves/resizes with it. This gives us Chrome's full UI (address bar, back/forward, context menus, right-click) without breaking CEF's rendering pipeline. cmux's own address bar is hidden for Chromium panels. Also passes right-click through to the CEF window.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a90de9636c
ℹ️ 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".
| // Library loader must stay alive for the lifetime of CEF. | ||
| static CefScopedLibraryLoader library_loader; | ||
| if (!library_loader.LoadInMain()) { |
There was a problem hiding this comment.
cef_bridge_initialize computes fwk_path from the caller-provided framework_path but then ignores it and calls LoadInMain() without any path override. In the runtime-download flow, the framework is extracted to Application Support rather than the app bundle, so initialization can still fail after a successful download because the loader never targets that downloaded location. Please load using the resolved path (or equivalent path-aware loader call) so downloaded frameworks are actually usable.
Useful? React with 👍 / 👎.
| ); | ||
| runOnlyForDeploymentPostprocessing = 0; | ||
| shellPath = /bin/sh; | ||
| shellScript = "set -euo pipefail\nCEF_BRIDGE_SCRIPT=\"${SRCROOT}/scripts/build-cef-bridge.sh\"\nif [ ! -f \"$CEF_BRIDGE_SCRIPT\" ]; then\n echo \"error: missing CEF bridge build script at $CEF_BRIDGE_SCRIPT\" >&2\n exit 1\nfi\n/bin/bash \"$CEF_BRIDGE_SCRIPT\" stub\n"; |
There was a problem hiding this comment.
The target’s Build CEF Bridge phase always invokes build-cef-bridge.sh stub, which means standard Xcode/CI builds always compile the stub bridge artifacts. That leaves Chromium initialization paths permanently unavailable in those builds (the stub bridge reports not-initialized), even if the runtime framework and helper are present. The phase should not unconditionally force stub mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/BrowserEngine/CEFBrowserView.swift">
<violation number="1" location="Sources/BrowserEngine/CEFBrowserView.swift:292">
P1: Reattaching to a window never re-attaches the CEF child window for existing browsers, so Chromium can disappear after view/window reparenting.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Removed hidden=true so CEF actually creates its Chrome window - Made the Chrome window borderless (no title bar) and non-movable so it can't be dragged away from the panel - Added parent window move/resize observers so the Chrome window follows the cmux window when moved or resized - Fixed cleanup of notification observers
Chrome runtime creates an uncontrollable separate window that can't be reliably made borderless or immovable. Alloy runtime renders directly inside the panel view via parent_view. Simplified CEFBrowserView to the minimum needed: Alloy rendering, right-click forwarding, clean lifecycle management. Navigation wired through cmux's address bar.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89a4ad769f
ℹ️ 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".
| // For Chromium panels, navigate via CEF instead of WebKit | ||
| if engineType == .chromium, let cefView = cefBrowserView { | ||
| shouldRenderWebView = true | ||
| cefView.loadURL(url.absoluteString) | ||
| return |
There was a problem hiding this comment.
This Chromium fast path returns before shouldBlockInsecureHTTPNavigation(...) runs, so http:// navigations in Chromium tabs bypass the existing insecure-HTTP allowlist/prompt flow that WebKit tabs still use. Any omnibar/programmatic call to navigate(to:) on a Chromium profile will load directly instead of going through the safety gate.
Useful? React with 👍 / 👎.
| if focused, panel.engineType == .chromium, | ||
| reason != "omnibar.tap" { |
There was a problem hiding this comment.
The new guard only permits Chromium focus when the reason is "omnibar.tap", but explicit focus requests (including Cmd+L and openChromiumBrowserAndFocusAddressBar) are applied via reason "request.apply". That means these keyboard/menu focus actions are ignored on Chromium tabs, breaking expected omnibar focus behavior.
Useful? React with 👍 / 👎.
1 participant
Summary
Phase 0 of the WebKit-to-Chromium migration. Adds the build system infrastructure and foundational types for embedding CEF (Chromium Embedded Framework) alongside the existing WebKit browser engine.
vendor/cef-bridge/): Plain C API wrapping CEF's C++ interface, callable from Swift via the bridging header. Stub implementations for now (returnsCEF_BRIDGE_ERR_NOT_INITuntil CEF is wired up in Phase 2). Covers lifecycle, profiles, browser views, navigation, JS evaluation, DevTools, extensions, and portal visibility.webkit/chromiumenum for per-profile engine selection.libcef_bridge.alinked in both Debug/Release configs,setup.shbuilds the stub library.No behavior change. Build verified.
Testing
xcodebuild -scheme cmux -configuration Debugbuilds successfully with stub CEF bridge linked.Related
Summary by cubic
Adds the Chromium engine alongside WebKit via a CEF bridge and embeds Chromium tabs in the app. Profiles store engine type; if CEF isn’t installed the tab shows a download UI; WebKit remains the default.
New Features
vendor/cef-bridge/wraps CEF APIs when built with CEF (CEF_BRIDGE_HAS_CEF); stub mode returnsCEF_BRIDGE_ERR_NOT_INIT.CEFBrowserView: NSView hosting a Chromium browser with navigation, zoom, JS, DevTools, find-in-page, and visibility callbacks.CEFFrameworkManager(download/extract/verify),CEFRuntime(init + 60 Hz loop + shutdown),CEFDownloadView(progress UI).engineType;BrowserPanelcreatesCEFBrowserViewfor Chromium;BrowserPanelViewrenders it or the download UI; new command and palette entry (openChromiumBrowserAndFocusAddressBar(),palette.newChromiumBrowserTab) open Chromium tabs.Bug Fixes
@rpathand matched the stub framework compatibility version.parent_view, defer browser creation until the view has non-zero size, and set initial bounds so Chromium renders inside our view.--use-mock-keychain; disabledpersist_session_cookies.libcef_bridge.a, added stub framework builds for Xcode inbuild-cef-bridge.sh/setup.sh, and updatedembed-cef.shto the current CEF version.Written for commit b4e2ec5. Summary will update on new commits.
Summary by CodeRabbit
New Features
Chores