Repository navigation
Add CEF (Chromium Embedded Framework) as alternative browser engine - #3989
Open
ericwang520 wants to merge 11 commits into
Open
ericwang520 wants to merge 11 commits into
ericwang520 wants to merge 11 commits into
Conversation
|
@ericwang520 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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 vendored CEF provisioning, a CMUXCEF Swift package and public bridge, an Objective‑C++ bridge and browser embedders, Swift API surface and panels, helper subprocess entrypoints and embed/codesign scripts, a runtime installer/UI, demo app and tests, docs, localization, and Xcode wiring. ChangesCEF Engine Integration
Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Adds the CMUXCEF Swift package under CEF/ that wraps Chromium
Embedded Framework 146.0.10 (Chrome runtime) so cmux can run a
Chromium-based browser engine alongside WKWebView. The package is
self-contained:
* Public Swift surface — CEFEngine, CEFBrowser, CEFProfile,
CEFEngineConfig — built around an ObjC++ bridge
(CEF/Sources/CMUXCEFBridge) that owns the CEF C++ types so
Swift never sees a CefRefPtr.
* Two helper executables (CMUXCEFHelper, CMUXCEFHelperRenderer)
in CEF/Sources/CMUXCEFHelper{,Renderer}/main.mm for the macOS
Chrome multi-process layout.
* CMUXCEFDemoApp — standalone NSSplitView demo that exercises
Path B NSView reparent embedding (CefBrowserView extracted out
of CefWindow and pinned into an AppKit container). Useful as a
regression target separate from the cmux integration.
* vendor/ — cef.lock.json lock-file, fetch_cef.sh fetcher that
downloads the prebuilt framework via the lock entry, plus a
JSON schema for the lock format. The Chromium Embedded
Framework binary itself is NOT committed.
* Scripts/embed_cef_into_cmux.sh — Xcode "Run Script" phase
invoked by Phase 2 to rsync the framework + build/install the
three helper .app bundles (cmux Helper, cmux Helper (GPU),
cmux Helper (Renderer)) with Chrome-runtime entitlements
(JIT, library validation off, disable-executable-page-
protection, allow-dyld-environment-variables). Helper bundle
ids are derived from $PRODUCT_BUNDLE_IDENTIFIER so the
macOS helper-to-app IPC prefix lookup resolves correctly.
Confidence: medium
Scope-risk: narrow — the package compiles standalone, has no
consumers yet, and is opt-in via the feature flag landing in
Phase 2.
Tested: swift build inside CEF/ produces the helpers and demo
binary; the demo renders three Chromium browsers in an
NSSplitView. xcodebuild for cmux is not exercised here (Phase 2).
Not-tested: Local cmux integration / UI tests — Phase 2 wires
consumers and is verified separately.
Wires the CMUXCEF SwiftPM package (vendored in the previous commit)
into cmux as an opt-in browser engine alongside WKWebView, gated on a
Debug → Browser Engine toggle (BrowserEngineKind, persisted in
UserDefaults). cmux still ships WKWebView as the default and only
production-supported path; CEF is experimental and DEBUG-only.
Surfaces added:
* BrowserEngineKind + BrowserEngineBackedPanel abstraction so the
same Workspace-level browser-pane creation path can pick between
WKWebView and CEF panes at construction time without a thread
of branches.
* CEFBrowserPanel / CEFBrowserPanelView — mirrors the surface of
BrowserPanel that the Panel protocol requires, plus a minimal
toolbar (back / forward / reload / address bar / DevTools).
Heavier WKWebView features (find-in-page, popup, profile
isolation) are intentionally not duplicated here in v1.
* Workspace+CEFBrowser — split out of Workspace.swift so the new
engine can be exercised without touching the WKWebView branches
line-by-line.
* AppDelegate+CEF — lazy CEFEngine.start when the user flips the
flag mid-session; idempotent.
* CrApplication (NSApplication subclass) + NSPrincipalClass entry
in Info.plist. Chromium's Chrome runtime requires CrAppProtocol
(`isHandlingSendEvent` / `setHandlingSendEvent:`); without it,
CEF aborts during the first event dispatch.
* Bridge plumbing: CefSettings.external_message_pump = true plus
OnScheduleMessagePumpWork that dispatches CefDoMessageLoopWork
onto the main queue. cmux's main thread runs SwiftUI /
NSApplication's loop, NOT CefRunMessageLoop, so the host has to
pump CEF itself or every helper Mojo bootstrap stalls 15s and
suicides.
Build / packaging:
* GhosttyTabs.xcodeproj — adds the CMUXCEF Swift package
dependency, links libCMUXCEF into cmux, adds the "Embed CEF"
Run Script phase that invokes CEF/Scripts/embed_cef_into_cmux.sh.
* scripts/reload.sh — patches the tagged Debug app's helper
Info.plist CFBundleIdentifiers to <app-id>.helper{,.gpu,
.renderer} so the Chrome-runtime helper-IPC prefix lookup
matches the tagged main app bundle id (otherwise the helpers
spawn but never establish their compositor channels).
* Resources/cmux.debug.entitlements — JIT, allow-unsigned-
executable-memory, disable-library-validation for the V8
runtime under hardened runtime.
Profile isolation: every CEFBrowserPanel currently funnels through
CefRequestContext::GetGlobalContext(). Chrome runtime in CEF 146
rejects custom cache_path values supplied via
CefRequestContext::CreateContext (chrome_browser_context.cc:116),
which leaves the browser attached to a half-initialised context
that never ships compositor frames. Real per-profile isolation will
ride on top of g_browser_process->profile_manager() in a follow-up.
Constraint: Do not run local tests or direct xcodebuild in this repo;
CI and tagged reload own verification.
Constraint: WKWebView remains the only default-on engine; CEF code
paths are gated by `#if canImport(CMUXCEF)` + the engine flag.
Confidence: medium
Scope-risk: moderate — adds a second browser engine + Chromium
process tree. All entry points sit behind a feature flag; no
existing surface changes when the flag is off.
Directive: New CEF browser surface work should go through the
BrowserEngineBackedPanel protocol so WKWebView vs CEF stays a
single branch at Workspace creation time, not many.
Tested: Tagged-build reload (cef-reparent tag); CEF tab renders
google.com / chrome://extensions in the cmux pane, mouse-event
routing reaches the renderer.
Not-tested: Local unit/UI tests and local xcodebuild, per repository
and task policy.
Contributor
Greptile SummaryThis PR adds Chromium Embedded Framework (CEF 146) as an opt-in alternative browser engine for cmux, gated behind a Debug → Browser Engine flag. WKWebView remains the default; the CEF path is experimental, DEBUG-only, and requires macOS 15.0+.
Confidence Score: 3/5Merging adds the CEF infrastructure behind a DEBUG-only flag with WKWebView unchanged, but
Sources/BrowserEngineKind.swift needs to be split before installer follow-up work begins; the runtime download, subprocess commands, filesystem locator, and AppKit progress UI should each live in their own file. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User (Debug Menu)
participant cmuxApp as cmuxApp.swift
participant Installer as CEFRuntimeInstaller
participant Workspace as Workspace.swift
participant Panel as CEFBrowserPanel
participant Engine as CEFEngine
participant Bridge as CMUXCEFBridge (ObjC++)
User->>cmuxApp: Select CEF in Browser Engine menu
cmuxApp->>Installer: ensureInstalledAfterUserConfirmation()
Installer->>Installer: confirmInstall() NSAlert sheet
Installer->>Installer: download() via URLSession.download(from:)
Installer->>Installer: verifyTarballMetadata() SHA1 + size
Installer->>Installer: runCommand(tar / cp / install_name_tool / codesign)
Installer-->>cmuxApp: "installed = true"
cmuxApp->>cmuxApp: "browserEngineRaw = cef"
User->>Workspace: New Browser Tab
Workspace->>Workspace: "BrowserEngineKind.current == .cef"
Workspace->>Workspace: registerCEFBrowserSplit or registerCEFBrowserSurface
Workspace->>Panel: CEFBrowserPanel init
Workspace-->>Workspace: return nil
Panel->>Panel: activate() lazy on first visible
Panel->>Engine: startCEFEngineIfNeeded() if not running
Engine->>Bridge: dlopen libcef RTLD_NOW RTLD_GLOBAL
Engine->>Bridge: CefInitialize()
Panel->>Bridge: makeEmbeddableBrowser(profile:initialURL:)
Bridge-->>Panel: CEFBrowser with embeddableView NSView
Panel-->>Workspace: NSView embedded in pane hierarchy
Reviews (10): Last reviewed commit: "fix: avoid CEF message pump re-entry" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #if DEBUG | ||
| cmuxDebugLog("cef.dlopen.ok path=\(cefFw)") | ||
| #endif | ||
| do { |
Contributor
There was a problem hiding this comment.
NSLog bypasses unified logging
Two NSLog(...) calls on lines 62 and 105 are not wrapped in #if DEBUG and will fire in Release builds. The logging rule requires either using unified Logger (os_log) or guarding raw NSLog with #if DEBUG. A dlopen failure and a CEF engine startup failure are permanent-diagnostic events — they should go through os_log so they appear in the system log with the CMUXCEF subsystem rather than on raw stderr, and so they respect the user's privacy on release builds.
Rule Used: Flag production Swift diagnostics that bypass unif... (source)
Comment on lines
+32
to
+40
| @MainActor | ||
| final class CEFBrowserPanel: BrowserEngineBackedPanel { | ||
|
|
||
| // MARK: Panel protocol — engine-agnostic metadata | ||
|
|
||
| let id: UUID = UUID() | ||
| let panelType: PanelType = .browser | ||
| @Published private(set) var displayTitle: String | ||
| @Published private(set) var displayIcon: String? = "globe" |
Contributor
There was a problem hiding this comment.
@Published / ObservableObject pattern in new code; @Observable is the correct modern shape
CEFBrowserPanel is a new @MainActor final class with multiple @Published properties observed via @ObservedObject in CEFBrowserPanelView. The cmux-swiftui-state-layout rule flags new ObservableObject/@Published where @Observable (macOS 14 Observation framework) is available. @Observable eliminates per-property @Published declarations, reduces body-level over-invalidation, and avoids the @ObservedObject wrapper on the call site.
Suggested change
| @MainActor | |
| final class CEFBrowserPanel: BrowserEngineBackedPanel { | |
| // MARK: Panel protocol — engine-agnostic metadata | |
| let id: UUID = UUID() | |
| let panelType: PanelType = .browser | |
| @Published private(set) var displayTitle: String | |
| @Published private(set) var displayIcon: String? = "globe" | |
| @MainActor | |
| @Observable | |
| final class CEFBrowserPanel: BrowserEngineBackedPanel { | |
| // MARK: Panel protocol — engine-agnostic metadata | |
| let id: UUID = UUID() | |
| let panelType: PanelType = .browser | |
| private(set) var displayTitle: String | |
| private(set) var displayIcon: String? = "globe" |
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| .appendingPathComponent( | ||
| "Contents/Frameworks/Chromium Embedded Framework.framework/Versions/A/Chromium Embedded Framework" | ||
| ).path | ||
| if dlopen(cefFw, RTLD_LAZY | RTLD_GLOBAL) == nil { |
Contributor
There was a problem hiding this comment.
RTLD_LAZY used where the comment mandates RTLD_NOW
The block comment directly above this call (lines 41–46) explains that RTLD_NOW | RTLD_GLOBAL is required so every libcef symbol is bound before any CefString::FromString call in the bridge fires — otherwise the bridge jumps to PC=0. The actual call passes RTLD_LAZY | RTLD_GLOBAL, which defers symbol resolution to first use and leaves the exact crash the comment guards against reachable on the first CEF bridge call.
Suggested change
| if dlopen(cefFw, RTLD_LAZY | RTLD_GLOBAL) == nil { | |
| if dlopen(cefFw, RTLD_NOW | RTLD_GLOBAL) == nil { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CEF/CEFArtifacts`:
- Line 1: The file CEF/CEFArtifacts contains a hard-coded, machine-specific
absolute path "/Users/wanghuangruei/Documents/cmux/CEF/Frameworks"; remove this
literal and replace it with a portable reference (e.g., a relative path, a
project-root-based lookup, or an environment variable like CEF_FRAMEWORKS_DIR)
so CI and other developers don't rely on your home directory; update any
consumers that read CEF/CEFArtifacts to fall back to a sensible default or fail
with a clear error if the env var/relative path is not set.
In `@CEF/INTEGRATION.md`:
- Line 33: Step 2 uses the wrong package path casing: replace occurrences of the
lowercase path string `cmux/cef` with the correct repository path `cmux/CEF` so
instructions match the actual directory name and won't fail on case-sensitive
filesystems; update the text in INTEGRATION.md wherever `cmux/cef` is referenced
to `cmux/CEF`.
- Around line 89-92: Update the Step 4 checklist in CEF/INTEGRATION.md to
reference the actual script added in this PR
(CEF/Scripts/embed_cef_into_cmux.sh) instead of
Scripts/embed_helpers_into_bundle.sh; change any wording that says the script
will be landed later to reflect that embed_cef_into_cmux.sh is present in this
branch and provide the correct relative path and filename so the setup is
reproducible from this branch.
In `@CEF/Scripts/embed_cef_into_cmux.sh`:
- Around line 46-63: The entitlements plist (ENT) is created unconditionally
with debug-only keys; modify embed_cef_into_cmux.sh to generate/select
entitlements based on SWIFT_CFG: create two separate plist templates (e.g.,
ENT_DEBUG via mktemp and ENT_RELEASE via mktemp) where ENT_DEBUG contains the
permissive keys (com.apple.security.get-task-allow, disable-library-validation,
allow-dyld-environment-variables, etc.) and ENT_RELEASE contains only minimal
production-safe keys, then set ENT to the appropriate file before
codesign/embedding by checking the SWIFT_CFG variable (used earlier) and using
ENT in the existing signing flow so release builds use the minimal plist and
debug builds use the permissive plist.
- Around line 36-39: The BUILD_BIN assignment is brittle; replace the hardcoded
path construction in embed_cef_into_cmux.sh and instead set BUILD_BIN by
invoking swift build --show-bin-path with the proper configuration (use
SWIFT_CFG) to get the actual SwiftPM binary directory, then update HELPER_BIN
and RENDERER_BIN to point to CMUXCEFHelper and CMUXCEFHelperRenderer inside that
BUILD_BIN; keep ARCH and CEF_ROOT unchanged if still needed elsewhere but do not
rely on ${CEF_ROOT}/.build/... for the binary path—use BUILD_BIN derived from
swift build --show-bin-path to locate the helpers.
In `@CEF/Sources/CMUXCEF/CEFEngine.swift`:
- Around line 97-103: The shutdown() sets config = nil but leaves start() able
to pass its init guard and attempt a forbidden re-initialization; add a
permanent one-way flag (e.g., isTerminated or didShutdown) on CEFEngine, set it
true inside shutdown(), and then have CEFEngine.start() check this flag and
refuse/early-return if true (in addition to the existing config guard);
reference the existing shutdown(), start(), and config symbols so the new
boolean is created alongside them and used to enforce a single-process lifetime.
In `@CEF/Sources/CMUXCEFBridge/CMUXCEFBridge.mm`:
- Around line 510-512: The code currently sets cachePath = [root
stringByAppendingPathComponent:@"Default"] while all profile names are bound to
CefRequestContext::GetGlobalContext(), which risks deleting the shared Default
cache for isolated-* profiles; update the logic around
CefRequestContext::GetGlobalContext(), cachePath and profile name handling so
you only target the shared Default cache when the profile truly uses the global
context or the profile name equals "Default" — otherwise compute a per-profile
cache path (e.g., use the actual profile name instead of "Default") and skip any
deletion of the Default path for isolated-* profiles; apply the same guard to
the similar block around the code referenced at the later 529-533 region.
In `@CEF/Sources/CMUXCEFDemoApp/main.swift`:
- Around line 309-316: removePaneAction is removing the pane container twice:
first via last.embeddableView?.superview?.removeFromSuperview() and again by
removing splitView.arrangedSubviews.last; stop the double-removal by removing
the container exactly once. Change removePaneAction to (a) capture the container
view from last.embeddableView?.superview (or from paneContainers if you track
it), call splitView.removeArrangedSubview(container) and
container.removeFromSuperview() once (or just removeFromSuperview if already
removed from arrangedSubviews elsewhere), and keep browsers.popLast() and
paneContainers in sync with that single removal; avoid calling
splitView.removeArrangedSubview on splitView.arrangedSubviews.last
unconditionally to prevent removing a different pane.
In `@CEF/vendor/fetch_cef.sh`:
- Around line 223-239: The current extract logic uses rm -rf on "${out_dir}"
then mv which can race with concurrent builds; instead write into the temp tree
(tmp_root) as you do, then atomically replace the destination by first moving
the staged directory to a sibling temporary name (e.g. mv "${staged}"
"${out_dir}.tmp"), then perform an atomic rename to the final location (e.g. mv
-T "${out_dir}.tmp" "${out_dir}" or an equivalent atomic rename fallback), and
only then remove tmp_root; remove the prior rm -rf "${out_dir}" step and adjust
the same pattern for the other occurrence (the block around lines 262-268),
ensuring cleanup of tmp_root on failure.
- Around line 65-68: In the option parsing case for --dest in fetch_cef.sh (the
block that currently does "--dest) shift; DEST=\"$1\"; ;;"), first validate that
a following argument exists before shifting—e.g., check $# or test that $1 is
set—so we do not trigger an unbound-variable error under set -u; if the arg is
missing, print the documented argument error message and exit with the script’s
arg-error exit code instead of shifting and assigning to DEST.
In `@Sources/AppDelegate`+CEF.swift:
- Around line 41-52: The dlopen call using RTLD_LAZY reintroduces the
deferred-symbol-resolution crash described in the comment; update the dlopen
invocation that uses the cefFw variable to pass RTLD_NOW | RTLD_GLOBAL instead
of RTLD_LAZY so symbol resolution happens eagerly before any
CefString::FromString bridge calls (locate the dlopen(...) check near cefFw and
change the binding flag).
In `@Sources/BrowserEngineKind.swift`:
- Around line 40-46: The computed property BrowserEngineKind.current should
guard against returning .cef when CEF isn't available: after reading the
persisted raw value and constructing kind, if kind == .cef and
BrowserEngineKind.isCEFAvailable == false, return .default instead; update the
logic in BrowserEngineKind.current (the raw parsing/guard block that uses
userDefaultsKey) to explicitly check isCEFAvailable and fall back to .default
for unavailable CEF.
In `@Sources/cmuxApp.swift`:
- Around line 328-329: The use of Image(systemName: isCurrent ? "checkmark" :
"") should be changed to always supply a valid symbol and toggle visibility
instead; replace the ternary with Image(systemName: "checkmark") and control
visibility with .opacity(isCurrent ? 1 : 0) (or .hidden() conditionally) while
keeping the existing .frame(width: 16) so you avoid empty-symbol warnings —
update the Image(...) expression currently using isCurrent and the empty string
to this visibility-toggling pattern.
In `@Sources/Panels/CEFBrowserPanel.swift`:
- Around line 315-355: The DEBUG logging in cefBrowserDidStartLoading,
cefBrowserDidFinishLoading, cefBrowser(_:didChangeTitle:),
cefBrowser(_:didChangeURL:), and cefBrowser(_:didFailLoad:) currently emits raw
URLs, titles, and error objects; replace those cmuxDebugLog calls to avoid
sensitive payloads and instead log only non-sensitive metadata (e.g., panel id
prefix via id.uuidString.prefix(5), event kind/category, and lengths or coarse
details such as URL host only or byte/character counts, and error type/class
without message/stack). Update each affected function
(cefBrowserDidStartLoading, cefBrowserDidFinishLoading,
cefBrowser(_:didChangeTitle:), cefBrowser(_:didChangeURL:),
cefBrowser(_:didFailLoad:)) to redact the raw values and emit minimal metadata
per the OmnibarSuggestionsView guideline.
- Around line 96-119: The initializer currently accepts renderInitialNavigation
but discards it; add a stored property (e.g., renderInitialNavigation: Bool) on
CEFBrowserPanel, assign the initializer parameter to that property inside init,
and update activate() to check that property before performing the initial
navigation/creating the browser with initialURL so callers that defer first
navigation are respected; ensure references to initialURL and displayTitle
remain unchanged and only gate the creation/navigation on the new
renderInitialNavigation flag.
In `@Sources/Panels/PanelContentView.swift`:
- Around line 56-68: The CEFBrowser-specific branch in PanelContentView
references CEFBrowserPanel and CEFBrowserPanelView which are conditionally
compiled; wrap the entire else-if branch that checks "else if let cefPanel =
panel as? CEFBrowserPanel { ... CEFBrowserPanelView(... ) }" with `#if`
canImport(CMUXCEF) / `#endif` so the cast and view creation are only compiled when
the CMUXCEF module is available, leaving other branches unchanged.
In `@Sources/Workspace.swift`:
- Around line 10081-10101: The CEF detour paths call registerCEFBrowserSplit and
then unconditionally return nil, which loses success/failure semantics and
prevents callers (e.g., newBrowserSplit callers like createBrowserToRight and
duplicateBrowserToRight) from executing post-create logic (tab reordering,
insertAtEnd, initialRequest, bypassInsecureHTTPHostOnce). Change the CEF
branches so they return a non-nil success value instead of nil: have
registerCEFBrowserSplit return a meaningful indicator (e.g., the created
pane/surface or a Bool) and propagate that value from both CEF branches in
newBrowserSplit, then adjust callers that expect a BrowserPanel to treat the
success indicator appropriately (or adapt types so newBrowserSplit still returns
the expected type while signalling success). Ensure the second CEF branch (the
block around the lines analogous to 10195–10209) is changed the same way so
insertAtEnd, initialRequest and bypassInsecureHTTPHostOnce handling still runs
on success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9fce37d0-2f37-4e9b-8c94-048c54d017eb
📒 Files selected for processing (36)
.gitignoreCEF/CEFArtifactsCEF/INTEGRATION.mdCEF/Package.swiftCEF/README.mdCEF/Scripts/codesign_dev.shCEF/Scripts/embed_cef_into_cmux.shCEF/Sources/CMUXCEF/CEFBrowser.swiftCEF/Sources/CMUXCEF/CEFEngine.swiftCEF/Sources/CMUXCEF/CEFEngineConfig.swiftCEF/Sources/CMUXCEF/CEFProfile.swiftCEF/Sources/CMUXCEFBridge/CMUXCEFBridge.mmCEF/Sources/CMUXCEFBridge/include/CMUXCEFBridge.hCEF/Sources/CMUXCEFDemoApp/main.swiftCEF/Sources/CMUXCEFHelper/main.mmCEF/Sources/CMUXCEFHelperRenderer/main.mmCEF/Tests/CMUXCEFTests/CEFEngineTests.swiftCEF/vendor/README.mdCEF/vendor/cef.lock.jsonCEF/vendor/cef.lock.schema.jsonCEF/vendor/fetch_cef.shGhosttyTabs.xcodeproj/project.pbxprojResources/Info.plistResources/cmux.debug.entitlementsSources/AppDelegate+CEF.swiftSources/AppDelegate.swiftSources/BrowserEngineKind.swiftSources/CrApplication.swiftSources/Panels/BrowserEngineBackedPanel.swiftSources/Panels/CEFBrowserPanel.swiftSources/Panels/CEFBrowserPanelView.swiftSources/Panels/PanelContentView.swiftSources/Workspace+CEFBrowser.swiftSources/Workspace.swiftSources/cmuxApp.swiftscripts/reload.sh
Comment on lines
+10081
to
+10101
| // Engine fork: when the user has enabled CEF in Debug → Browser | ||
| // Engine, route through a parallel registration path that | ||
| // constructs a ``CEFBrowserPanel`` and registers it as the new | ||
| // pane's surface. The CEF path returns nil because callers of | ||
| // ``newBrowserSplit`` expect a ``BrowserPanel`` (WKWebView) and | ||
| // CEF-specific follow-up (extensions UI, devtools, etc.) is | ||
| // wired in later PRs. See | ||
| // ``Prototypes/cef-webview/notes/cmux-integration-plan.md``. | ||
| if BrowserEngineKind.current == .cef && BrowserEngineKind.isCEFAvailable { | ||
| _ = registerCEFBrowserSplit( | ||
| fromPaneId: paneId, | ||
| orientation: orientation, | ||
| insertFirst: insertFirst, | ||
| url: url, | ||
| preferredProfileID: preferredProfileID, | ||
| focus: focus, | ||
| creationPolicy: creationPolicy, | ||
| initialDividerPosition: initialDividerPosition | ||
| ) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Preserve success semantics in the CEF detours.
Both CEF branches return nil after successful registration, so callers can’t distinguish success from failure. In this file, that already drops post-create behavior (e.g., tab reordering in createBrowserToRight / duplicateBrowserToRight), and Line 10195 also bypasses insertAtEnd, initialRequest, and bypassInsecureHTTPHostOnce inputs.
Also applies to: 10195-10209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Workspace.swift` around lines 10081 - 10101, The CEF detour paths
call registerCEFBrowserSplit and then unconditionally return nil, which loses
success/failure semantics and prevents callers (e.g., newBrowserSplit callers
like createBrowserToRight and duplicateBrowserToRight) from executing
post-create logic (tab reordering, insertAtEnd, initialRequest,
bypassInsecureHTTPHostOnce). Change the CEF branches so they return a non-nil
success value instead of nil: have registerCEFBrowserSplit return a meaningful
indicator (e.g., the created pane/surface or a Bool) and propagate that value
from both CEF branches in newBrowserSplit, then adjust callers that expect a
BrowserPanel to treat the success indicator appropriately (or adapt types so
newBrowserSplit still returns the expected type while signalling success).
Ensure the second CEF branch (the block around the lines analogous to
10195–10209) is changed the same way so insertAtEnd, initialRequest and
bypassInsecureHTTPHostOnce handling still runs on success.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (7)
CEF/Sources/CMUXCEFBridge/CMUXCEFBridge.mm (1)
521-535:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid deleting the shared
Defaultcache while global-context fallback is active.This concern was previously raised and remains unresolved. All profile names currently bind to
CefRequestContext::GetGlobalContext()and Line 511 forcescachePathtoDefault. The deletion logic on Lines 529-533 will remove that shared cache directory for anyisolated-*profile, affecting all active profiles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CEF/Sources/CMUXCEFBridge/CMUXCEFBridge.mm` around lines 521 - 535, The destroyProfileForName: method currently unconditionally schedules deletion of bridge.cachePath for names with the "isolated-" prefix, which can remove the shared "Default" cache when profiles fallback to CefRequestContext::GetGlobalContext(); update destroyProfileForName: (and related CMUXCEFProfileBridge) to first detect whether the bridge is using the global request context or the cachePath equals @"Default" and, if so, skip the removal; otherwise proceed with the dispatch_after removal. Reference the destroyProfileForName: method, the CMUXCEFProfileBridge instance (_byName[name]), the bridge.cachePath value, and the global CefRequestContext::GetGlobalContext() (or a bridge.requestContextIsGlobal flag if you add one) when implementing the conditional check.Sources/Panels/PanelContentView.swift (1)
56-68:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winWrap CEF-specific branch in
#if canImport(CMUXCEF)guard.The
CEFBrowserPanelcast andCEFBrowserPanelViewinstantiation will fail to compile when the CMUXCEF package is not available, since those types are conditionally compiled. Wrap this entireelse ifbranch with#if canImport(CMUXCEF)and#endif.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/PanelContentView.swift` around lines 56 - 68, Wrap the CEF-specific branch so it only compiles when the CMUXCEF module is available: surround the entire `else if let cefPanel = panel as? CEFBrowserPanel { ... CEFBrowserPanelView(...) }` branch with `#if canImport(CMUXCEF)` and `#endif`, ensuring references to CEFBrowserPanel and CEFBrowserPanelView are inside that conditional to avoid build errors when CMUXCEF is not present.CEF/INTEGRATION.md (2)
33-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix package path casing in setup instructions.
Line 33 should use
cmux/CEF(notcmux/cef) to avoid failures on case-sensitive filesystems.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CEF/INTEGRATION.md` at line 33, Update the setup instruction that currently reads "Navigate to `cmux/cef` and add the package" to use the correct casing `cmux/CEF` so the path is accurate on case-sensitive filesystems; edit the text that contains the literal `cmux/cef` (search for that exact string) and replace it with `cmux/CEF`.
83-92:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate Step 4 to reference the actual embed script in this branch.
Lines 83-92 still point to
embed_helpers_into_bundle.shand say it “will land later.” Please reference the script that exists in this PR (CEF/Scripts/embed_cef_into_cmux.sh) so the checklist is executable as-is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CEF/INTEGRATION.md` around lines 83 - 92, Update Step 4 in CEF/INTEGRATION.md to reference the actual script in this branch by replacing the call to "Scripts/embed_helpers_into_bundle.sh" with "CEF/Scripts/embed_cef_into_cmux.sh" (and update the three-target invocation to use that path), and change the accompanying note that currently says the script “will land later” to state that the embed script is present in this PR (CEF/Scripts/embed_cef_into_cmux.sh) so the checklist is executable as-is.Sources/BrowserEngineKind.swift (1)
40-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard persisted
.cefbehind runtime availability.Line 45 currently returns
.cefeven whenBrowserEngineKind.isCEFAvailable == false. Fall back to.defaultfor that case to avoid unsupported persisted state.Proposed fix
public static var current: BrowserEngineKind { let raw = UserDefaults.standard.string(forKey: userDefaultsKey) guard let raw, let kind = BrowserEngineKind(rawValue: raw) else { return .default } + if kind == .cef, !isCEFAvailable { + return .default + } return kind }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/BrowserEngineKind.swift` around lines 40 - 46, The getter BrowserEngineKind.current reads a persisted raw value and returns the enum case directly, but it must not return .cef when BrowserEngineKind.isCEFAvailable is false; update the guard/return logic to check isCEFAvailable before returning .cef so that if the persisted raw maps to .cef but isCEFAvailable == false the code returns .default instead (use BrowserEngineKind(rawValue: raw) to resolve the case, then if kind == .cef && !BrowserEngineKind.isCEFAvailable return .default; otherwise return the resolved kind).Sources/AppDelegate+CEF.swift (1)
41-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse eager symbol binding for CEF framework load.
Line 51 still uses
RTLD_LAZY, which contradicts the nearby requirement to bind exports before first bridgeCefStringusage. Switch toRTLD_NOW | RTLD_GLOBAL.Proposed fix
- if dlopen(cefFw, RTLD_LAZY | RTLD_GLOBAL) == nil { + if dlopen(cefFw, RTLD_NOW | RTLD_GLOBAL) == nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/AppDelegate`+CEF.swift around lines 41 - 52, The dlopen call that loads the CEF framework uses RTLD_LAZY but the comment and surrounding logic require eager binding; change the flags in the dlopen invocation that references cefFw to use RTLD_NOW | RTLD_GLOBAL instead of RTLD_LAZY so all libcef symbols are bound before any CefString::FromString bridge calls execute.Sources/Workspace.swift (1)
10081-10101:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve success semantics and parameter parity in the CEF branches.
Both CEF detours register a surface/split and then return
nil, soguard letcallers treat successful creation as failure. The surface detour also dropsinitialRequest,insertAtEnd, andbypassInsecureHTTPHostOnce, causing behavior drift from the WK path. Return a non-nil success signal (or a shared creation result type) and propagate equivalent inputs through the CEF path.Also applies to: 10195-10209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Workspace.swift` around lines 10081 - 10101, The CEF detour in newBrowserSplit currently calls registerCEFBrowserSplit and returns nil, which breaks callers using `guard let` and also drops parameters (`initialRequest`, `insertAtEnd`, `bypassInsecureHTTPHostOnce`, etc.), so update the CEF branch to mirror the WK path: pass through all original inputs (initialRequest, insertAtEnd, bypassInsecureHTTPHostOnce, initialDividerPosition, preferredProfileID, focus, creationPolicy, orientation, paneId) into the CEF registration call(s) (e.g., registerCEFBrowserSplit) and return the same success signal / creation result type that the WK path returns (or introduce and return the shared creation result type used by the function) instead of nil; apply the same changes to the second CEF branch referenced near the later block (the other CEF detour) so both preserve parameter parity and non-nil success semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CEF/README.md`:
- Around line 14-31: The README's directory tree fenced code block (the block
that starts with "cmux-cef/") is missing a language identifier and triggers
MD040; fix it by adding a language tag (e.g., "text") to the opening ``` so the
block becomes ```text and re-run the linter to confirm the MD040 warning is
resolved.
In `@CEF/Sources/CMUXCEFDemoApp/main.swift`:
- Line 67: pendingExtensionFolders is populated by loadUnpackedExtensionAction()
but never used when starting CEF because boot() is called with
extensionDirectories: [], so queued folders never reach CEF and the "dead"
button/alert is misleading; update the startup flow so boot() receives the
queued folders (e.g., pass pendingExtensionFolders.map { $0.path } or add a
parameter to boot to consume pendingExtensionFolders and clear it after use) and
adjust loadUnpackedExtensionAction()/UI alert so it either starts CEF when
extensions are added or shows correct guidance (e.g., change alert text or
enable a "Start CEF" action) to ensure the queued extension paths are actually
consumed before/when CEF starts.
- Around line 76-78: Replace the force-try when resolving the Application
Support directory so the app doesn't hard-crash: change the let support = try!
FileManager.default.url(...) expression to a do/catch (or a try? with explicit
nil check) that captures any thrown error from FileManager.default.url and
forwards a descriptive message and the error to the demo's existing startup
failure path (instead of aborting). Locate the symbol "support" and the
FileManager.default.url call in main.swift, handle the error by invoking the
app's startup-failure handler with context (e.g., "Failed to locate Application
Support directory") so the demo exits cleanly with the error details.
In `@CEF/vendor/cef.lock.schema.json`:
- Line 3: The schema $id currently points to the old Prototypes/cef-webview URL;
update the JSON "$id" value in CEF/vendor/cef.lock.schema.json (the "$id"
property) to
"https://github.com/manaflow-ai/cmux/CEF/vendor/cef.lock.schema.json" so schema
resolvers and editors reference the repository's canonical path; locate the
"$id" key in the file and replace the old path with the new one exactly as
shown.
In `@Sources/AppDelegate.swift`:
- Around line 999-1000: startCEFEngineIfNeeded() is being invoked before the
XCTest guard, which breaks the intended test startup gating; move or
conditionally call startCEFEngineIfNeeded() only when not running under tests by
wrapping it with a check like if !isRunningUnderXCTest (or an explicit test
opt-in env var) so that startCEFEngineIfNeeded() is skipped during XCTest runs;
update the code path that currently calls startCEFEngineIfNeeded() near the
isRunningUnderXCTest guard (referencing startCEFEngineIfNeeded and
isRunningUnderXCTest) to ensure heavy CEF startup only occurs in normal app
launches.
In `@Sources/Panels/CEFBrowserPanelView.swift`:
- Around line 37-46: The view currently calls panel.activate() inside its .task
unconditionally, which boots CEF for off-window/keep-alive instances; modify the
CEFBrowserPanelView .task to first check isVisibleInUI (guard or if) and only
set addressText and call try panel.activate() when isVisibleInUI is true,
otherwise skip activation (but still allow keeping state). Also update any
observer handler for Notification.Name.commandPaletteVisibilityDidChange to
ignore notifications targeting other instances by verifying notification.object
(or another identifying token) matches self or by checking isVisibleInUI before
responding, and only set activationError when activation actually ran.
In `@Sources/Workspace`+CEFBrowser.swift:
- Around line 54-64: The new Bonsplit.Tab created from CEFBrowserPanel currently
snapshots displayTitle/isLoading/isDirty but never updates; subscribe the tab to
the CEFBrowserPanel's change events so subsequent updates to displayTitle,
displayIcon, isLoading and isDirty are propagated to the Bonsplit tab UI. Locate
where panels[panel.id] = panel and the Bonsplit.Tab is constructed (the
Bonsplit.Tab initializer and SurfaceKind.browser usage) and add a
listener/observer on the CEFBrowserPanel (or its change publisher) to update the
existing tab entry (and panelTitles[panel.id] if needed) whenever the panel's
properties change; ensure you remove the subscription when the panel is removed
to avoid leaks.
---
Duplicate comments:
In `@CEF/INTEGRATION.md`:
- Line 33: Update the setup instruction that currently reads "Navigate to
`cmux/cef` and add the package" to use the correct casing `cmux/CEF` so the path
is accurate on case-sensitive filesystems; edit the text that contains the
literal `cmux/cef` (search for that exact string) and replace it with
`cmux/CEF`.
- Around line 83-92: Update Step 4 in CEF/INTEGRATION.md to reference the actual
script in this branch by replacing the call to
"Scripts/embed_helpers_into_bundle.sh" with "CEF/Scripts/embed_cef_into_cmux.sh"
(and update the three-target invocation to use that path), and change the
accompanying note that currently says the script “will land later” to state that
the embed script is present in this PR (CEF/Scripts/embed_cef_into_cmux.sh) so
the checklist is executable as-is.
In `@CEF/Sources/CMUXCEFBridge/CMUXCEFBridge.mm`:
- Around line 521-535: The destroyProfileForName: method currently
unconditionally schedules deletion of bridge.cachePath for names with the
"isolated-" prefix, which can remove the shared "Default" cache when profiles
fallback to CefRequestContext::GetGlobalContext(); update destroyProfileForName:
(and related CMUXCEFProfileBridge) to first detect whether the bridge is using
the global request context or the cachePath equals @"Default" and, if so, skip
the removal; otherwise proceed with the dispatch_after removal. Reference the
destroyProfileForName: method, the CMUXCEFProfileBridge instance
(_byName[name]), the bridge.cachePath value, and the global
CefRequestContext::GetGlobalContext() (or a bridge.requestContextIsGlobal flag
if you add one) when implementing the conditional check.
In `@Sources/AppDelegate`+CEF.swift:
- Around line 41-52: The dlopen call that loads the CEF framework uses RTLD_LAZY
but the comment and surrounding logic require eager binding; change the flags in
the dlopen invocation that references cefFw to use RTLD_NOW | RTLD_GLOBAL
instead of RTLD_LAZY so all libcef symbols are bound before any
CefString::FromString bridge calls execute.
In `@Sources/BrowserEngineKind.swift`:
- Around line 40-46: The getter BrowserEngineKind.current reads a persisted raw
value and returns the enum case directly, but it must not return .cef when
BrowserEngineKind.isCEFAvailable is false; update the guard/return logic to
check isCEFAvailable before returning .cef so that if the persisted raw maps to
.cef but isCEFAvailable == false the code returns .default instead (use
BrowserEngineKind(rawValue: raw) to resolve the case, then if kind == .cef &&
!BrowserEngineKind.isCEFAvailable return .default; otherwise return the resolved
kind).
In `@Sources/Panels/PanelContentView.swift`:
- Around line 56-68: Wrap the CEF-specific branch so it only compiles when the
CMUXCEF module is available: surround the entire `else if let cefPanel = panel
as? CEFBrowserPanel { ... CEFBrowserPanelView(...) }` branch with `#if
canImport(CMUXCEF)` and `#endif`, ensuring references to CEFBrowserPanel and
CEFBrowserPanelView are inside that conditional to avoid build errors when
CMUXCEF is not present.
In `@Sources/Workspace.swift`:
- Around line 10081-10101: The CEF detour in newBrowserSplit currently calls
registerCEFBrowserSplit and returns nil, which breaks callers using `guard let`
and also drops parameters (`initialRequest`, `insertAtEnd`,
`bypassInsecureHTTPHostOnce`, etc.), so update the CEF branch to mirror the WK
path: pass through all original inputs (initialRequest, insertAtEnd,
bypassInsecureHTTPHostOnce, initialDividerPosition, preferredProfileID, focus,
creationPolicy, orientation, paneId) into the CEF registration call(s) (e.g.,
registerCEFBrowserSplit) and return the same success signal / creation result
type that the WK path returns (or introduce and return the shared creation
result type used by the function) instead of nil; apply the same changes to the
second CEF branch referenced near the later block (the other CEF detour) so both
preserve parameter parity and non-nil success semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5b88e2f-085f-4515-bdef-3f71d9475252
📒 Files selected for processing (36)
.gitignoreCEF/CEFArtifactsCEF/INTEGRATION.mdCEF/Package.swiftCEF/README.mdCEF/Scripts/codesign_dev.shCEF/Scripts/embed_cef_into_cmux.shCEF/Sources/CMUXCEF/CEFBrowser.swiftCEF/Sources/CMUXCEF/CEFEngine.swiftCEF/Sources/CMUXCEF/CEFEngineConfig.swiftCEF/Sources/CMUXCEF/CEFProfile.swiftCEF/Sources/CMUXCEFBridge/CMUXCEFBridge.mmCEF/Sources/CMUXCEFBridge/include/CMUXCEFBridge.hCEF/Sources/CMUXCEFDemoApp/main.swiftCEF/Sources/CMUXCEFHelper/main.mmCEF/Sources/CMUXCEFHelperRenderer/main.mmCEF/Tests/CMUXCEFTests/CEFEngineTests.swiftCEF/vendor/README.mdCEF/vendor/cef.lock.jsonCEF/vendor/cef.lock.schema.jsonCEF/vendor/fetch_cef.shGhosttyTabs.xcodeproj/project.pbxprojResources/Info.plistResources/cmux.debug.entitlementsSources/AppDelegate+CEF.swiftSources/AppDelegate.swiftSources/BrowserEngineKind.swiftSources/CrApplication.swiftSources/Panels/BrowserEngineBackedPanel.swiftSources/Panels/CEFBrowserPanel.swiftSources/Panels/CEFBrowserPanelView.swiftSources/Panels/PanelContentView.swiftSources/Workspace+CEFBrowser.swiftSources/Workspace.swiftSources/cmuxApp.swiftscripts/reload.sh
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
CEF/Sources/CMUXCEFDemoApp/main.swift (2)
67-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQueued extension folders never reach CEF startup.
pendingExtensionFoldersis populated byloadUnpackedExtensionAction()butboot()always starts CEF withextensionDirectories: [], and nothing consumes the queued paths later. The button is currently dead UI and the alert text is misleading.Also applies to: 265-288, 99-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CEF/Sources/CMUXCEFDemoApp/main.swift` at line 67, pendingExtensionFolders populated by loadUnpackedExtensionAction() is never used because boot() hardcodes extensionDirectories: [], so pass the queued paths into CEF startup and update the UI flow: modify boot(extensionDirectories: [URL]?) / or change boot() to read pendingExtensionFolders and supply them to the CEF initializer (use pendingExtensionFolders when non-empty, or merge with any caller-provided dirs), and ensure loadUnpackedExtensionAction() either triggers boot() after adding paths or updates the alert text to say the extension was queued (not loaded) so the button is not a dead UI; touch symbols: pendingExtensionFolders, loadUnpackedExtensionAction(), boot(), and any alert message string.
315-327:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the pane container only once.
last.embeddableView?.superview?.removeFromSuperview()already detaches the arranged subview. The follow-upsplitView.arrangedSubviews.lastblock can then remove a second pane, leavingbrowsersandpaneContainersout of sync with the actual UI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CEF/Sources/CMUXCEFDemoApp/main.swift` around lines 315 - 327, The removePaneAction currently removes the same pane twice: calling last.embeddableView?.superview?.removeFromSuperview() and then removing splitView.arrangedSubviews.last again; update removePaneAction to remove the specific pane's arrangedSubview exactly once by locating the embeddableView's superview (e.g., let paneView = last.embeddableView?.superview), call splitView.removeArrangedSubview(paneView) and paneView.removeFromSuperview() (or if paneView is nil fall back to removing the last arrangedSubview), then call last.close(), pop paneContainers in sync with browsers, recalc activePaneIndex using browsers.count - 1, and keep splitView.adjustSubviews() and refreshActivePaneHighlight(); ensure you reference removePaneAction, browsers, paneContainers, splitView, and embeddableView when applying the change.Sources/cmuxApp.swift (1)
339-341:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a stable SF Symbol and toggle visibility instead of an empty symbol.
Line 340 passes
""toImage(systemName:), which can trigger runtime symbol warnings. Keep a valid symbol and hide it visually when not selected.Suggested fix
- Image(systemName: isCurrent ? "checkmark" : "") - .frame(width: 16) + Image(systemName: "checkmark") + .opacity(isCurrent ? 1 : 0) + .frame(width: 16)#!/bin/bash # Verify no empty SF Symbol name usages remain in Swift files. rg -nP 'Image\s*\(\s*systemName:\s*""\s*\)' --type=swift🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/cmuxApp.swift` around lines 339 - 341, Replace the empty SF Symbol usage in the HStack by always providing a valid symbol to Image(systemName:) (e.g., "checkmark") and control visibility with SwiftUI modifiers instead of an empty string: keep the existing isCurrent boolean and use it to set .opacity(isCurrent ? 1 : 0) or .hidden() on the Image(systemName: "checkmark") so the symbol is valid at runtime but only visible when isCurrent is true.Sources/Workspace.swift (1)
10081-10100:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't treat successful CEF creation as
nil.Line 10099 and Line 10207 still return
nilafter successful CEF registration, so callers can't distinguish success from failure. That drops follow-up work in existing paths like browser reordering, andcreatePanel(from:inPane:)will also treat restored CEF browser tabs as failed creations. Preserve a non-nil success result here and make sure the CEF path still carriesinitialRequest,insertAtEnd, andbypassInsecureHTTPHostOncesemantics forward.Also applies to: 10194-10208
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Workspace.swift` around lines 10081 - 10100, The CEF branch in newBrowserSplit currently calls registerCEFBrowserSplit(...) and returns nil which hides successful creation; update the CEF path (the registerCEFBrowserSplit call in Workspace.newBrowserSplit and the identical block around registerCEFBrowserSplit at the other location) to return a proper non-nil BrowserPanel/creation result instead of nil so callers can detect success (affecting createPanel(from:inPane:) and browser reordering); ensure the returned result preserves and forwards initialRequest, insertAtEnd/insertFirst semantics and bypassInsecureHTTPHostOnce flags from the original parameters so the rest of the creation flow receives the same metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CEF/Scripts/embed_cef_into_cmux.sh`:
- Around line 80-97: The generated helper Info.plists in write_plist currently
set LSMinimumSystemVersion to "15.0", which blocks helpers on macOS 14.x; update
the write_plist function so the
<key>LSMinimumSystemVersion</key><string>...</string> value matches the
advertised runtime floor (e.g., "14.0") or is derived from a manifest/env
variable, ensuring helper bundles declare the same minimum macOS version as the
package manifest.
In `@CEF/Sources/CMUXCEFBridge/CMUXCEFBridge.mm`:
- Around line 527-535: The current code uses a fixed 2-second dispatch_after to
remove the isolated-* profile cache (variables cachePath, usesSharedDefaultCache
and the branch for name hasPrefix:@"isolated-"), which is a race; change it to
wait for the real CEF request-context/profile destruction notification or
completion callback instead of a timed delay. Locate the logic around the
isolated-* branch in CMUXCEFBridge.mm and replace the dispatch_after with a
deterministic synchronization: subscribe to or pass a completion block from the
code that destroys the CefRequestContext (or observe the CEF UI-thread
callback/notification that signals context teardown), then perform
[[NSFileManager defaultManager] removeItemAtPath:cachePath error:nil] only after
that callback fires on the UI thread; ensure you still skip deletion when
usesSharedDefaultCache is true.
- Around line 398-408: Remove the NSTimer-based 30Hz fallback and stop calling
_cmuxCefPumpTick: on a wall-clock schedule; instead wire CEF's own scheduling by
relying on the OnScheduleMessagePumpWork / CEF message-pump callbacks to drive
CefDoMessageLoopWork on the main thread (or post a CEF-owned task via
CefPostTask/CefPostDelayedTask if scheduling is required) and ensure the
existing OnScheduleMessagePumpWork handler in CMUXCEFBridge triggers
CefDoMessageLoopWork rather than using fixed NSTimer polling; delete the
scheduledTimerWithTimeInterval... invocation and any references to
_cmuxCefPumpTick: so the integration uses CEF-owned scheduling only.
In `@CEF/vendor/README.md`:
- Line 64: The README references a non-existent file MIGRATION_PLAN.md in the
dogfood checklist for CEF updates (Phase 8); either create MIGRATION_PLAN.md
containing the Phase 8 migration details or update the reference in
CEF/vendor/README.md to point to an existing document (or remove the link) and
ensure the text around "Phase 8" and the "dogfood checklist" correctly reflects
the new target; update any Markdown link syntax that currently points to
MIGRATION_PLAN.md so it resolves to the created or existing file.
In `@Sources/BrowserEngineKind.swift`:
- Around line 254-273: The errorDescription computed property in
BrowserEngineKind returns plain English literals; replace each case's returned
string (cases: unsupportedArchitecture, insufficientDiskSpace,
downloadedFileMissing, downloadedFileSizeMismatch, downloadedFileHashMismatch,
expectedExtractedDirectoryMissing, expectedFrameworkMissing, commandFailed) with
a localized wrapper using String(localized: "cefRuntime.installError.<case>",
defaultValue: "<defaultValue>") and put the existing message text (using "\(…)"
interpolation for values like architecture, available/required, got/expected,
path, message) into defaultValue so the NSAlert uses localized strings
consistent with the rest of the installer UI.
- Around line 1-842: The file is too large and mixes responsibilities; split it
into focused files: keep BrowserEngineKind, BrowserEngineKind.current,
displayLabel, and isCEFAvailable in Sources/BrowserEngineKind.swift; extract
CEFRuntimeDescriptor and CEFLockfile decoding (CEFRuntimeDescriptor.current,
fallbackCurrent, bundledLockfileDescriptor, downloadURL, requiredFreeBytes) into
Sources/CEFRuntime/CEFRuntimeDescriptor.swift; move CEFRuntimeLocation and
CEFRuntimeLocator (applicationSupportRoot, installedLocation, bundledLocation,
resolvedLocation) into Sources/CEFRuntime/CEFRuntimeLocator.swift; move
CEFRuntimeInstaller class and its helpers (installRuntime*, hasEnoughDiskSpace,
verifyTarballMetadata, installRuntimeSynchronously, download,
restructureFramework, createOrReplaceSymlink, runCommand, availableCapacity,
sha1Hex, downloadPercentage, CEFRuntimeInstallerError/CEFRuntimeInstallPhase)
plus CEFRuntimeDownloadDelegate and CEFRuntimeInstallProgressReporter into
Sources/CEFRuntime/CEFRuntimeInstaller.swift; and move the AppKit UI
CEFRuntimeInstallProgressPresenter into
Sources/CEFRuntime/CEFRuntimeInstallProgressPresenter.swift so UI, installer
logic, networking/delegate, parsing, locator, and enum are each isolated and the
BrowserEngineKind enum no longer transitively imports AppKit or networking.
- Around line 681-700: The installer progress NSWindow (the window property
created and used by show()) lacks a stable cmux identifier and is not registered
with the cmux close-shortcut router; set window.identifier =
NSUserInterfaceItemIdentifier("cmux.installer.progress") right after the
NSWindow is created, and register the window with the cmux close-shortcut router
(e.g. call cmuxWindowShouldOwnCloseShortcut.register(window) or the app's
equivalent registration API) so Cmd+W is owned by this sheet/standalone window;
also ensure you unregister the window in deinit or windowWillClose to avoid
dangling registrations.
---
Duplicate comments:
In `@CEF/Sources/CMUXCEFDemoApp/main.swift`:
- Line 67: pendingExtensionFolders populated by loadUnpackedExtensionAction() is
never used because boot() hardcodes extensionDirectories: [], so pass the queued
paths into CEF startup and update the UI flow: modify boot(extensionDirectories:
[URL]?) / or change boot() to read pendingExtensionFolders and supply them to
the CEF initializer (use pendingExtensionFolders when non-empty, or merge with
any caller-provided dirs), and ensure loadUnpackedExtensionAction() either
triggers boot() after adding paths or updates the alert text to say the
extension was queued (not loaded) so the button is not a dead UI; touch symbols:
pendingExtensionFolders, loadUnpackedExtensionAction(), boot(), and any alert
message string.
- Around line 315-327: The removePaneAction currently removes the same pane
twice: calling last.embeddableView?.superview?.removeFromSuperview() and then
removing splitView.arrangedSubviews.last again; update removePaneAction to
remove the specific pane's arrangedSubview exactly once by locating the
embeddableView's superview (e.g., let paneView =
last.embeddableView?.superview), call splitView.removeArrangedSubview(paneView)
and paneView.removeFromSuperview() (or if paneView is nil fall back to removing
the last arrangedSubview), then call last.close(), pop paneContainers in sync
with browsers, recalc activePaneIndex using browsers.count - 1, and keep
splitView.adjustSubviews() and refreshActivePaneHighlight(); ensure you
reference removePaneAction, browsers, paneContainers, splitView, and
embeddableView when applying the change.
In `@Sources/cmuxApp.swift`:
- Around line 339-341: Replace the empty SF Symbol usage in the HStack by always
providing a valid symbol to Image(systemName:) (e.g., "checkmark") and control
visibility with SwiftUI modifiers instead of an empty string: keep the existing
isCurrent boolean and use it to set .opacity(isCurrent ? 1 : 0) or .hidden() on
the Image(systemName: "checkmark") so the symbol is valid at runtime but only
visible when isCurrent is true.
In `@Sources/Workspace.swift`:
- Around line 10081-10100: The CEF branch in newBrowserSplit currently calls
registerCEFBrowserSplit(...) and returns nil which hides successful creation;
update the CEF path (the registerCEFBrowserSplit call in
Workspace.newBrowserSplit and the identical block around registerCEFBrowserSplit
at the other location) to return a proper non-nil BrowserPanel/creation result
instead of nil so callers can detect success (affecting
createPanel(from:inPane:) and browser reordering); ensure the returned result
preserves and forwards initialRequest, insertAtEnd/insertFirst semantics and
bypassInsecureHTTPHostOnce flags from the original parameters so the rest of the
creation flow receives the same metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3062c182-b813-4b3a-983d-800bd869ddda
📒 Files selected for processing (22)
CEF/INTEGRATION.mdCEF/Package.swiftCEF/README.mdCEF/Scripts/embed_cef_into_cmux.shCEF/Sources/CMUXCEFBridge/CMUXCEFBridge.mmCEF/Sources/CMUXCEFDemoApp/main.swiftCEF/vendor/README.mdCEF/vendor/cef.lock.schema.jsonGhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/AppDelegate+CEF.swiftSources/AppDelegate.swiftSources/BrowserEngineKind.swiftSources/CrApplication.swiftSources/Panels/CEFBrowserPanel.swiftSources/Panels/CEFBrowserPanelView.swiftSources/Workspace+CEFBrowser.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/BrowserConfigTests.swiftscripts/setup.sh
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/cmuxApp.swift (1)
340-341:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid empty SF Symbol names in menu rows.
Line 340 uses an empty SF Symbol name for the unchecked state, which can produce runtime symbol warnings.
#!/bin/bash # Verify whether empty systemName usage still exists in SwiftUI Image initializers. rg -nP 'Image\s*\(\s*systemName:\s*[^)]*""[^)]*\)' --type=swiftSuggested fix
- Image(systemName: isCurrent ? "checkmark" : "") - .frame(width: 16) + Image(systemName: "checkmark") + .opacity(isCurrent ? 1 : 0) + .frame(width: 16)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/cmuxApp.swift` around lines 340 - 341, The code passes an empty string to Image(systemName:) when isCurrent is false which triggers SF Symbol warnings; update the view to avoid constructing Image with an empty name by conditionally rendering the Image only when isCurrent is true (e.g., if isCurrent { Image(systemName: "checkmark").frame(width: 16) } else { EmptyView() or a Spacer().frame(width: 16) }) so the layout keeps width but no empty systemName is used; refer to the existing Image(systemName:) call and the isCurrent boolean to locate and replace the problematic line.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@GhosttyTabs.xcodeproj/project.pbxproj`:
- Around line 1578-1580: The "Embed CEF" build phase is currently attached to
the app target unconditionally; update the project.pbxproj so the "Embed CEF"
PBXShellScriptBuildPhase (named "Embed CEF") is only included in Debug build
configurations: remove it from the target's general buildPhases list and instead
add its reference only to the Debug configuration's buildPhases (or adjust each
configuration-specific buildPhases arrays so Release/Distribution configs do not
reference "Embed CEF"); apply the same gating for the other occurrences noted
around the 1793-1814 block to ensure the phase runs only for Debug builds.
In `@README.md`:
- Line 323: Add a short explanatory sentence or two below the existing setup.sh
description that describes what reload.sh does and what the --tag local-dev
argument means: mention that reload.sh restarts/reloads services or containers
configured by the project (or refreshes local development environment) and that
--tag local-dev targets the local development image or configuration named
"local-dev"; reference the existing entries for setup.sh and use the exact
script and flag names reload.sh and --tag local-dev so users understand when and
why to run this command.
---
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 340-341: The code passes an empty string to Image(systemName:)
when isCurrent is false which triggers SF Symbol warnings; update the view to
avoid constructing Image with an empty name by conditionally rendering the Image
only when isCurrent is true (e.g., if isCurrent { Image(systemName:
"checkmark").frame(width: 16) } else { EmptyView() or a Spacer().frame(width:
16) }) so the layout keeps width but no empty systemName is used; refer to the
existing Image(systemName:) call and the isCurrent boolean to locate and replace
the problematic line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3604ca33-69b6-4c58-a326-0d0e1c6faea7
📒 Files selected for processing (7)
GhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/Panels/PanelContentView.swiftSources/Workspace.swiftSources/cmuxApp.swift
Comment on lines
+679
to
+688
| private nonisolated static func sha1Hex(of url: URL) throws -> String { | ||
| let handle = try FileHandle(forReadingFrom: url) | ||
| defer { try? handle.close() } | ||
| var hasher = Insecure.SHA1() | ||
| while true { | ||
| let data = handle.readData(ofLength: 1024 * 1024) | ||
| if data.isEmpty { break } | ||
| hasher.update(data: data) | ||
| } | ||
| return hasher.finalize().map { String(format: "%02x", $0) }.joined() |
Contributor
There was a problem hiding this comment.
Insecure.SHA1 used to verify an executable before dlopen
sha1Hex(of:) uses CryptoKit.Insecure.SHA1 — Apple's own API name signals this hash is cryptographically weak. The result is used in verifyTarballMetadata as the sole integrity gate for a ~282 MB binary that is subsequently dlopen'd directly into the cmux process. SHA1 chosen-prefix collisions are feasible (Shattered, 2017), so a compromised build CDN could serve a modified binary that passes this check. CryptoKit.SHA256 is available, costs no extra dependency, and eliminates the collision surface on the exact path where binary authenticity matters most.
Comment on lines
+1
to
+10
| import Foundation | ||
| import AppKit | ||
| import CryptoKit | ||
| import SwiftUI | ||
|
|
||
| /// Which browser engine a new browser pane should be created with. | ||
| /// | ||
| /// cmux ships with two browser engines that can be selected via the | ||
| /// **Debug → Browser Engine** menu: | ||
| /// |
Contributor
There was a problem hiding this comment.
BrowserEngineKind.swift exceeds 800 lines and mixes six or more independent concerns
This 871-line file bundles the public BrowserEngineKind enum, CEFRuntimeDescriptor (download metadata), CEFLockfile JSON parsing, CEFRuntimeLocation/CEFRuntimeLocator (filesystem lookup), CEFRuntimeInstallPhase/CEFRuntimeInstallerError, CEFRuntimeInstaller (~370 lines of network download, subprocess orchestration, and SHA1 verification), CEFRuntimeInstallProgressPresenter (AppKit UI), CEFRuntimeDownloadDelegate (URLSession delegate), and a ProcessInfo extension. Each of these is independently testable and belongs in its own file or a narrowly-scoped CEFRuntime package. As-is, a unit test for verifyTarballMetadata must compile the entire AppKit progress presenter.
File Used: .github/review-bot-rules/swift-file-package-boundaries.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
teamleaderleo
added
the
S2: major
teamleaderleo
added
the
area: browser
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds Chromium Embedded Framework (CEF) as an alternative browser engine alongside WKWebView, gated behind a Debug → Browser Engine flag. WKWebView remains the default and production path; CEF is experimental, DEBUG-only for now.
Follows the same two-phase model the earlier CEFWebView vendoring used:
Vendor CEF Chrome runtime Swift package— adds the self-containedCEF/SwiftPM package (CEFEngine / CEFBrowser / CEFProfile Swift surface + ObjC++ bridge + two helper executables + standaloneCMUXCEFDemoAppregression target). No cmux consumers in this commit; package is standalone.Phase 2: wire CEF as alternative browser engine behind Debug menu flag— wires the package into cmux throughBrowserEngineKind(UserDefaults flag flipped from Debug menu), aBrowserEngineBackedPanelabstraction so Workspace creation can pick the engine at one branch,CrApplication(NSApplication subclass withCrAppProtocol— required by Chrome runtime),external_message_pump/OnScheduleMessagePumpWorkpump driven from the main run loop, and helper-bundle-id alignment so Chromium's macOS helper IPC prefix lookup matches the host app id.Known scope limits (called out in the Phase 2 commit message)
CefRequestContext::GetGlobalContext(). Chrome runtime in CEF 146 rejects every customcache_pathsupplied viaCefRequestContext::CreateContext(chrome_browser_context.cc:116), so real per-profile isolation has to go throughg_browser_process->profile_manager()in a follow-up.Test plan
chrome://extensions/default URL is opted-in)swift run --package-path CEF CMUXCEFDemoApprenders three Chromium browsersSummary by cubic
Adds CEF as an alternative browser engine behind Debug → Browser Engine, with a first‑use runtime download and verification;
WKWebViewremains the default. CEF is experimental, DEBUG‑only, and available only on supported macOS.New Features
CMUXCEF(CEF 146) with an ObjC++ bridge, two helper executables, and a demo; lockfile‑driven SDK viavendor/fetch_cef.sh; Xcode “Embed CEF” build script.BrowserEngineKindwith a clean compile‑time fallback whenCMUXCEFisn’t linked.CrApplicationprincipal class and external message pump; lazy engine start; first‑use runtime installer (size/SHA1, disk‑space gate, menu progress) with tests and localized strings; helper bundle IDs aligned to the app ID for tagged builds; DEBUG entitlements for V8 JIT and unsigned memory.Bug Fixes
CMUXCEFSwiftPM package builds on Xcode 16.4.Written for commit 88f888d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Chores