Repository navigation
Vendor CEFWebView Swift package (Phase 1: chromium browser engine) - #2945
lawrencecchen wants to merge 2 commits into
Conversation
Phase 1 of an effort to replace WKWebView in BrowserPanel with a Chromium-backed engine via brennanMKE/CEFWebView. This commit only adds the vendored package and build infrastructure: no Xcode target wiring, no embedded framework, no helper apps yet. The cmux app is unchanged and the existing WKWebView path stays the default. What lands here: - vendor/CEFWebView/: package source (no .git, no demo WebView/, no Tests/) - Package.swift bumped to .macOS(.v14) to match cmux's deployment target - testTarget removed (Tests/ not vendored) - build_cpp.sh patched to use find -L so symlinked CEF distros work - scripts/setup-cefwebview.sh: idempotent script that downloads or symlinks a pinned CEF binary distribution into vendor/CEFWebView/CEF/, builds libcef_dll_wrapper.a, and produces vendor/CEFWebView/Frameworks/ - .gitignore: excludes the CEF binary distribution and built Frameworks/ - vendor/CEFWebView/INTEGRATION_NOTES.md: tracks remaining Xcode integration steps and the parity items still to port from BrowserPanel After scripts/setup-cefwebview.sh runs, the package compiles cleanly under swift build. Hooking it into GhosttyTabs.xcodeproj (framework embed phases, helper-app build phases, code signing) is Phase 2.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR vendors the CEFWebView Swift package and integrates it into the app: adds Objective‑C++ CEF bridge and helper entrypoints, Swift UI/state integration, multiple build/embed/diagnostic scripts, package manifest, docs, license, gitignore/attributes, and Xcode project changes to reference the local package and frameworks. Changes
Sequence DiagramssequenceDiagram
participant App as App
participant CEFApp as CEFApplication
participant CEFWrap as CEFWrapper (ObjC++)
participant CEF as CEF C++
App->>CEFApp: initialize()
CEFApp->>CEFWrap: initializeCEFWithError()
CEFWrap->>CEF: CefInitialize(settings)
CEF-->>CEFWrap: result
CEFWrap-->>CEFApp: BOOL success
loop Message pump
CEFApp->>CEFWrap: doMessageLoopWork()
CEFWrap->>CEF: CefDoMessageLoopWork()
end
App->>CEFApp: shutdown()
CEFApp->>CEFWrap: shutdown()
CEFWrap->>CEF: CefShutdown()
sequenceDiagram
participant SwiftUI as SwiftUI View
participant CEFView as CEFWebView (NSViewRepresentable)
participant Container as CEFBrowserContainer (NSView)
participant Host as CEFBrowserHost
participant CEFWrap as CEFWrapper (ObjC++)
participant CEF as CEF C++
SwiftUI->>CEFView: makeNSView()
CEFView->>Container: create flipped container
SwiftUI->>CEFView: updateNSView(url)
CEFView->>Host: init(parentView, url, state)
Host->>CEFWrap: createBrowserInView:url
CEFWrap->>CEF: CefBrowserHost::CreateBrowserSync
CEF-->>CEFWrap: browser
CEFWrap-->>Host: native NSView*
Host->>Container: embed nativeView
Host->>CEFWrap: loadURL()
sequenceDiagram
participant CefClient as ChromiumClient (C++)
participant Bridge as extern "C" bridge
participant MainQ as Main Queue
participant Swift as CEFApplication
participant State as CEFWebViewState
CefClient->>Bridge: NotifyHelperSpawned("renderer")
Bridge->>MainQ: dispatch
MainQ->>Swift: notifyHelperSpawned:
Swift->>State: rendererHelperSpawned = true
CefClient->>Bridge: NotifyHelperFailedWithLoadError(...)
Bridge->>MainQ: dispatch
MainQ->>Swift: notifyHelperFailed:...
Swift->>State: rendererHelperFailed = true, set error fields
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: eefdf6c037
ℹ️ 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".
| void OnAddressChange(CefRefPtr<CefBrowser> browser, | ||
| CefRefPtr<CefFrame> frame, | ||
| const CefString& url) override { | ||
| NSString* urlCopy = [NSString stringWithUTF8String:url.ToString().c_str()]; | ||
| NSLog(@"🔗 OnAddressChange: %@", urlCopy); |
There was a problem hiding this comment.
OnAddressChange fires for every frame, not just the main document. Because this handler unconditionally writes _currentURL, iframe/ad/tracker navigations can overwrite the top-level URL, which will surface an incorrect address in currentURL() and any Swift state derived from it on pages with subframes. Add a frame->IsMain() guard before updating _currentURL.
Useful? React with 👍 / 👎.
| private static func stableNavigationKey(_ url: URL) -> String { | ||
| url.absoluteString.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() |
There was a problem hiding this comment.
stableNavigationKey lowercases the full URL string before deduping binding updates. URL path/query case can be significant, so a navigation from a mixed-case route to a differently cased route can be treated as “unchanged,” causing updateNSView to skip loadURL and miss a real navigation.
Useful? React with 👍 / 👎.
| if let urlToLoad = url { | ||
| logger.info("🔍 Loading initial URL immediately: \(urlToLoad.absoluteString)") | ||
| host.loadURL(urlToLoad) |
There was a problem hiding this comment.
This block reloads the initial URL immediately after browser creation, but CEFBrowserHost.init already passes the same URL into CEFWrapper.createBrowser(...url:), which performs the initial navigation. Calling host.loadURL again duplicates the first request and can trigger duplicate load events/side effects on startup pages.
Useful? React with 👍 / 👎.
Greptile SummaryPhase 1 vendoring of
Confidence Score: 4/5Safe to merge as Phase 1 (no user-visible change), but The cmux app and all existing code paths are untouched. The one P1 finding ( vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm — sandbox disabled and global singleton design need attention before Phase 2.
|
| Filename | Overview |
|---|---|
| vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm | Core ObjC++ CEF bridge; no_sandbox = 1 disables process sandbox; global singleton g_browser/g_client limits to one browser instance; dead log initialization code at lines 429-430. |
| scripts/setup-cefwebview.sh | Idempotent CEF binary download + build script; minor: cmake deployment target (13.0) mismatches Package.swift (.macOS(.v14)). |
| vendor/CEFWebView/build.sh | build and release actions reference non-existent WebView/ demo directory (stripped during vendoring); uses ${BASH_SOURCE[0]} inside a #!/usr/bin/env zsh script. |
| vendor/CEFWebView/Package.swift | Correctly lowered to .macOS(.v14), testTarget removed; absolute-path cefFrameworksDirectory closure via #filePath is appropriate for SPM. |
| vendor/CEFWebView/Sources/CEFWebView/CEFWebView.swift | NSViewRepresentable wrapper with stable navigation key deduplication; deferred browser creation until first updateNSView with non-zero bounds is well-structured. |
| vendor/CEFWebView/Sources/CEFWebView/CEFBridge.swift | Lifecycle manager for CEF initialization and helper process status; uses buffered flags for helpers spawned before CEFBrowserHost attaches. |
| vendor/CEFWebView/build_cpp.sh | Patched with find -L for symlinked CEF distributions; framework restructuring and codesign removal look correct. |
| .gitignore | Correctly excludes vendor/CEFWebView/CEF/, Frameworks/, and .build/ from source control. |
Sequence Diagram
sequenceDiagram
participant App as cmux App (Phase 2)
participant Bridge as CEFBridge.swift
participant Wrapper as CEFWrapper.mm
participant CEF as Chromium CEF
App->>Bridge: CEFApplication.handleSubprocessIfNeeded()
Bridge->>Wrapper: executeSubprocessWithArgc()
Wrapper->>CEF: CefExecuteProcess()
CEF-->>Wrapper: exitCode < 0 (main process)
Wrapper-->>Bridge: returns
App->>Bridge: CEFApplication.shared.initialize()
Bridge->>Wrapper: initializeCEFWithError()
Wrapper->>Wrapper: EnsureCEFFrameworkLoaded()
Wrapper->>CEF: CefInitialize(settings, app)
CEF-->>Wrapper: true
Wrapper-->>Bridge: success
App->>Bridge: CEFBrowserHost(parentView:url:)
Bridge->>Wrapper: createBrowserInView(_:url:)
Wrapper->>CEF: CefBrowserHost::CreateBrowserSync()
CEF->>CEF: spawn GPU helper
CEF->>Wrapper: OnBeforeChildProcessLaunch(gpu-process)
Wrapper-->>Bridge: NSNotification CEFHelperSpawned
CEF->>CEF: spawn Renderer helper
CEF->>Wrapper: OnBeforeChildProcessLaunch(renderer)
Wrapper-->>Bridge: NSNotification CEFHelperSpawned
CEF-->>Wrapper: CefBrowser ptr
Wrapper-->>Bridge: NSView (browser view)
Reviews (1): Last reviewed commit: "Vendor CEFWebView Swift package for Chro..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| settings.no_sandbox = 1; | ||
| settings.external_message_pump = 1; |
There was a problem hiding this comment.
settings.no_sandbox = 1 unconditionally disables Chromium's subprocess sandbox. WKWebView enforces Apple's WebContent sandbox; removing it for a drop-in replacement is a meaningful security regression. This must be addressed before Phase 2 ships to users — at minimum, document the entitlements needed to re-enable sandboxing or add a #warning compile-time gate so it can't land in a release build silently.
|
|
||
| // Enable verbose logging | ||
| settings.log_severity = LOGSEVERITY_VERBOSE; | ||
| CefString(&settings.log_file).FromString("/tmp/cef_debug.log"); |
There was a problem hiding this comment.
settings.log_severity and settings.log_file are assigned here, then unconditionally overwritten at lines 539–545 with the same severity and a better path. The two lines here never take effect.
| // Enable verbose logging | |
| settings.log_severity = LOGSEVERITY_VERBOSE; | |
| CefString(&settings.log_file).FromString("/tmp/cef_debug.log"); | |
| settings.external_message_pump = 1; |
| // ─── Global State ─────────────────────────────────────────────────────────────── | ||
|
|
||
| static BOOL g_cefInitialized = NO; | ||
| static CefRefPtr<CefBrowser> g_browser; | ||
| static CefRefPtr<ChromiumClient> g_client; | ||
|
|
||
| /// Single loader per process. On macOS, main browser must call LoadInMain(); CEF helper | ||
| /// subprocesses (same binary, `--type=...`) must call LoadInHelper() — see cef_library_loader.h. | ||
| static CefScopedLibraryLoader g_cefLibraryLoader; |
There was a problem hiding this comment.
g_browser and g_client are process-wide globals. A second call to createBrowserInView:url: will overwrite g_client without releasing the previous instance, and all state queries (isLoading, currentURL, etc.) will silently operate on whichever browser was created last. cmux uses multiple browser panels across tabs/workspaces, so Phase 2 integration will need per-instance browser tracking before multi-panel use is safe.
| NC=$'\033[0m' # No Color | ||
|
|
||
| # Directories | ||
| PROJECT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" |
There was a problem hiding this comment.
${BASH_SOURCE[0]} is undefined in zsh
The shebang is #!/usr/bin/env zsh but BASH_SOURCE is a bash-only variable. In zsh it expands to empty string, so dirname "" returns . and PROJECT_DIR becomes $(pwd) rather than the script's directory. build.sh build / build.sh release will also fail unconditionally because $PROJECT_DIR/WebView (the demo app) was stripped during vendoring.
| PROJECT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| PROJECT_DIR="$(cd "$(dirname "$0")" && pwd)" |
| xcodebuild -configuration Release -target libcef_dll_wrapper -quiet | ||
| ) | ||
| fi | ||
|
|
There was a problem hiding this comment.
libcef_dll_wrapper.a is compiled with -DCMAKE_OSX_DEPLOYMENT_TARGET=13.0 while Package.swift declares .macOS(.v14). The linker will emit a deployment-target warning when the Swift package links against the static library. Aligning them prevents confusing build warnings.
| xcodebuild -configuration Release -target libcef_dll_wrapper -quiet | |
| ) | |
| fi | |
| cmake -G Xcode -DPROJECT_ARCH=arm64 -DCMAKE_OSX_DEPLOYMENT_TARGET=14.0 . >/dev/null |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (9)
vendor/CEFWebView/.gitignore (1)
10-10: Nit: add trailing slash toFrameworks.Without the trailing slash this pattern ignores both a directory and a file named
Frameworks. Sincescripts/setup-cefwebview.sh/build_cpp.shproduce a directory, preferFrameworks/to be explicit and consistent with the root.gitignoreentry (vendor/CEFWebView/Frameworks/).Proposed tweak
-Frameworks +Frameworks/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/CEFWebView/.gitignore` at line 10, Update the .gitignore entry so it explicitly ignores the Frameworks directory by changing the pattern "Frameworks" to "Frameworks/"; locate the entry in vendor/CEFWebView/.gitignore (the line containing "Frameworks") and replace it with "Frameworks/" to avoid accidentally ignoring a file named Frameworks and to match the root .gitignore style (vendor/CEFWebView/Frameworks/).vendor/CEFWebView/README.md (1)
1-7: Stub README is fine for Phase 1. Consider adding a pointer toINTEGRATION_NOTES.md/IMPLEMENTATION_GUIDE.mdandscripts/setup-cefwebview.shso downstream contributors can discover the setup flow from the package root.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/CEFWebView/README.md` around lines 1 - 7, The README for CEFWebView is a minimal stub; update it to include discoverability pointers to the integration/setup docs by adding short links or sentences referencing INTEGRATION_NOTES.md and/or IMPLEMENTATION_GUIDE.md and the setup script scripts/setup-cefwebview.sh so downstream contributors can find the setup flow from the package root (e.g., add a “Getting Started” or “Setup” line that names those files and where to find them).vendor/CEFWebView/Sources/CEFHelperRenderer/main.mm (1)
8-22: Consider gating the verboseNSLogargv dump to debug builds.Logging every
argv[i]unconditionally for every renderer subprocess spawn will spam the system log in release builds and diverges fromCEFHelper/main.mm, which doesn’t log at this level. Renderer argv on CEF can include sensitive-ish flags (channel IDs, seatbelt client IDs, etc.) — not a credential leak, but unnecessary for production.Suggested guard
`@autoreleasepool` { +#if DEBUG NSLog(@"🚀 CEFHelperRenderer main() called with argc=%d", argc); for (int i = 0; i < argc; i++) { NSLog(@" argv[%d]: %s", i, argv[i] ? argv[i] : "(null)"); } NSLog(@" Calling CEFWrapper.executeSubprocessWithArgc:argv:"); +#endif int code = [CEFWrapper executeSubprocessWithArgc:argc argv:argv]; +#if DEBUG NSLog(@" CEFWrapper.executeSubprocessWithArgc returned: %d", code); +#endif if (code >= 0) { +#if DEBUG NSLog(@"✅ CEFHelperRenderer exiting with code: %d", code); +#endif return code; } +#if DEBUG NSLog(@"❌ CEFHelperRenderer: not a subprocess, returning 1"); +#endif return 1; }Also worth aligning the two helper entrypoints (
CEFHelpervsCEFHelperRenderer) on a single logging style so renderer vs. GPU/utility console output is consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/CEFWebView/Sources/CEFHelperRenderer/main.mm` around lines 8 - 22, Gate the verbose NSLog argv dump in CEFHelperRenderer's main function so it only runs in debug builds: wrap the per-argument NSLog loop (the block that prints argv[i]) in a debug-only macro (e.g., `#if` DEBUG / `#ifdef` NDEBUG inverted as your project uses) or a build-config check, leaving the minimal high-level logs and the call to [CEFWrapper executeSubprocessWithArgc:argv] unchanged; also make the logging style consistent with the other helper entrypoint (CEFHelper main) by keeping only non-sensitive summary logs in release builds and moving detailed argv printing behind the debug guard.scripts/setup-cefwebview.sh (2)
11-11:FRAMEWORKSis declared but unused.
build_cpp.shcomputes its ownFRAMEWORKS_DIRfromSRCROOT. Drop this line (or pass it through) to quiet SC2034 and avoid implying the variable configures the sub-script.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-cefwebview.sh` at line 11, The variable FRAMEWORKS in scripts/setup-cefwebview.sh is declared but never used (triggering SC2034); either remove the FRAMEWORKS="$PKG_ROOT/Frameworks" line, or explicitly pass/export it into the sub-script that expects it (build_cpp.sh) so it becomes FRAMEWORKS_DIR there (build_cpp.sh computes FRAMEWORKS_DIR from SRCROOT); update the file to either drop the unused FRAMEWORKS assignment or export/forward it to build_cpp.sh to avoid the unused-variable warning and clarify intent.
36-38: Temporary tarball isn't cleaned up on failed extract.With
set -e, iftar -xjffails (corrupt download, disk full),rm -f "$TMP_TARBALL"on line 38 is skipped, leaving a partial ~200MB tarball invendor/CEFWebView/CEF/. On the next run, the$CEF_DISTdirectory check on line 27 still sees no extracted dir, so it re-downloads from scratch. Consider atrapor extracting to a staging dir and moving on success.🧹 Proposed cleanup
TMP_TARBALL="$CEF_ROOT/$CEF_TARBALL" + trap 'rm -f "$TMP_TARBALL"' EXIT curl -fL --retry 3 -o "$TMP_TARBALL" "$CEF_URL" tar -xjf "$TMP_TARBALL" -C "$CEF_ROOT" - rm -f "$TMP_TARBALL" + trap - EXIT + rm -f "$TMP_TARBALL"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-cefwebview.sh` around lines 36 - 38, The temporary tarball cleanup is skipped if tar fails, leaving large partial files; update the download/extract sequence around TMP_TARBALL, CEF_ROOT and the tar invocation so the temp file is always removed on error — e.g., set a trap to rm -f "$TMP_TARBALL" on EXIT/ERR or download to a staging file (TMP_TARBALL.tmp) and only move/rename it to TMP_TARBALL after a successful transfer, and extract into a staging directory before moving into CEF_ROOT on success; apply the change to the curl/tar/rm sequence that uses TMP_TARBALL, CEF_URL and tar -xjf to guarantee cleanup on failure.vendor/CEFWebView/build_cpp.sh (1)
27-31: Heads-up:build_cef_wrapperbuilds all CMake targets and sets no deployment target.When invoked from
scripts/setup-cefwebview.sh, the wrapper is already pre-built with-DCMAKE_OSX_DEPLOYMENT_TARGET=13.0and-target libcef_dll_wrapper, so this function'sxcodebuild -configuration Releasebecomes an effective no-op. But if someone runsbuild_cpp.shstandalone (as the vendoredIMPLEMENTATION_GUIDE.mdinstructs on L180-182), it will build every target in the generated Xcode project without pinning the deployment target — potentially producing binaries that don't match cmux'sMACOSX_DEPLOYMENT_TARGET = 14.0.Since upstream file, flagging as optional for awareness. If kept as-is, consider documenting that
setup-cefwebview.shis the canonical entrypoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/CEFWebView/build_cpp.sh` around lines 27 - 31, The current build_cpp.sh subshell runs a full Xcode build without pinning the macOS deployment target or restricting targets, which can produce binaries with a different MACOSX_DEPLOYMENT_TARGET than the rest of the project; change the subshell in build_cpp.sh so cmake is invoked with a fixed deployment target (e.g., -DCMAKE_OSX_DEPLOYMENT_TARGET=14.0) and invoke xcodebuild with an explicit target (e.g., -target libcef_dll_wrapper) and/or pass MACOSX_DEPLOYMENT_TARGET=14.0 to xcodebuild, or alternatively document that setup-cefwebview.sh is the canonical entrypoint that pre-builds with -target libcef_dll_wrapper and -DCMAKE_OSX_DEPLOYMENT_TARGET=13.0 so callers know to use it. Ensure references to libcef_dll_wrapper, build_cpp.sh, setup-cefwebview.sh and MACOSX_DEPLOYMENT_TARGET are used to locate where to apply the change.vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm (1)
428-430: Duplicate/deadlog_file+log_severityassignment —/tmp/cef_debug.logis never used.Lines 429-430 set
log_severity = LOGSEVERITY_VERBOSEandlog_file = /tmp/cef_debug.log, then lines 540-546 overwrite both with theNSCachesDirectory/com.chromium.webview/debug.logpath. The first block is a dead store; worse, the log message at line 572-576 points users to the cache path for diagnostics, which is correct, but anyone reading the source will waste time looking at/tmp/cef_debug.log. Remove the earlier assignments.🛠️ Proposed fix
- // Enable verbose logging - settings.log_severity = LOGSEVERITY_VERBOSE; - CefString(&settings.log_file).FromString("/tmp/cef_debug.log"); - // Also append command-line switches for extra verbosity CefRefPtr<CefCommandLine> command_line = CefCommandLine::GetGlobalCommandLine();Keep the cache-dir assignment around line 540+ as the single source of truth.
Also applies to: 540-546
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm` around lines 428 - 430, Remove the redundant early assignments to settings.log_severity and settings.log_file (the calls setting settings.log_severity = LOGSEVERITY_VERBOSE and CefString(&settings.log_file).FromString("/tmp/cef_debug.log")) since they are overwritten later by the NSCachesDirectory/com.chromium.webview/debug.log assignment; keep only the later cache-dir assignment for settings.log_file and its matching settings.log_severity so there is a single source of truth and no dead store in CEFWrapper.mm.vendor/CEFWebView/Sources/CEFWebView/CEFWebView.swift (1)
35-41:@Bindingaround an@Observablereference-type is redundant — drop the binding.
CEFWebViewStateis a@MainActor@observablefinal class, so it’s already a reference. Wrapping it in@Bindingdoesn’t give you anything@Observabledoesn’t, forces every call site (including your own#Previewat line 227) to pass$state, and makes the API harder to use fromNSViewRepresentablecontexts where the coordinator is the right ownership anchor. Prefer a plain stored property, and use@Bindableat the point of two-way UI binding if needed.🛠️ Proposed diff
public struct CEFWebView: NSViewRepresentable { `@Binding` var url: URL? - `@Binding` var state: CEFWebViewState - - public init(url: Binding<URL?>, state: Binding<CEFWebViewState>) { - self._url = url - self._state = state - } + var state: CEFWebViewState + + public init(url: Binding<URL?>, state: CEFWebViewState) { + self._url = url + self.state = state + }And in the preview at line 227:
-CEFWebView(url: $url, state: $state) +CEFWebView(url: $url, state: state)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/CEFWebView/Sources/CEFWebView/CEFWebView.swift` around lines 35 - 41, The CEFWebView currently declares "@Binding var state: CEFWebViewState" and accepts a Binding in init even though CEFWebViewState is a `@MainActor` `@Observable` reference type; remove the unnecessary binding: change the stored property to a plain "var state: CEFWebViewState" and update the initializer to take "state: CEFWebViewState" (adjust self.state assignment accordingly), then update all call sites (e.g., the Preview at line ~227) to pass the state instance (not $state) and adjust NSViewRepresentable/coordinator usage to own or reference the observable directly where two‑way binding is required.vendor/CEFWebView/build.sh (1)
119-136: Hard-codedCEF Version: 146.0.10will drift fromscripts/setup-cefwebview.sh.The pinned CEF version lives in
scripts/setup-cefwebview.sh; duplicating it here as a literal guaranteesRELEASE_NOTES.txtwill eventually lie about what actually shipped. Either derive it (scripts/setup-cefwebview.sh --print-version, or parsevendor/CEFWebView/CEF/VERSION/cef_version.h), or drop the line entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/CEFWebView/build.sh` around lines 119 - 136, The RELEASE_NOTES here-doc currently embeds a hard-coded "CEF Version: 146.0.10" which will drift; update the block that writes RELEASE_NOTES so the CEF version is derived at build time instead of literal: run the existing CEF setup script with its print-version option (or read the canonical CEF VERSION/cef_version source) to produce a CEF_VERSION variable, then interpolate that variable into the here-doc where "CEF Version:" is written (modify the RELEASE_NOTES assignment / the cat << EOF block accordingly) so the generated RELEASE_NOTES reflects the actual CEF version.
🤖 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/setup-cefwebview.sh`:
- Around line 15-19: The build currently allows overriding CEF_PLATFORM but
still hardcodes PROJECT_ARCH to arm64 when invoking the wrapper build, causing
arch mismatch; update the script to derive PROJECT_ARCH from the CEF_PLATFORM
variable (e.g., map "macosarm64" -> "arm64", "macosx64" or "macosintel" ->
"x86_64") and export/use that derived PROJECT_ARCH when invoking the wrapper
build (the place that supplies -DPROJECT_ARCH). Alternatively, if you only
support arm64, validate CEF_PLATFORM early and exit with an error if a non-arm64
platform is provided so the wrapper is not compiled with the wrong
-DPROJECT_ARCH.
In `@vendor/CEFWebView/build.sh`:
- Around line 1-4: The script enables errexit but not pipefail, so failures in
the xcodebuild | tee pipelines inside build() and release() are masked; update
vendor/CEFWebView/build.sh to enable pipefail (e.g., set -o pipefail) alongside
the existing set -e near the top of the file so that any failure of xcodebuild
in the pipelines inside build() and release() correctly propagates and triggers
the existing log_error/return 1 handling.
- Around line 36-46: The build_cef function currently hides all build_cpp.sh
output by redirecting to /dev/null; change it to capture and stream the output
to a log so users see errors and you can reference the log on failure. Invoke
./build_cpp.sh from inside build_cef but pipe its stdout/stderr through tee to a
persistent file (e.g., "$PROJECT_DIR/cef_build.log") so output is both shown and
saved, preserve the exit status, and when detecting failure call log_error
including the log file path (and any relevant helper scripts like
fix_cef_framework.sh) before returning 1; keep the surrounding
log_info/log_success behavior.
In `@vendor/CEFWebView/capture_logs.sh`:
- Around line 43-46: The sudo hint in capture_logs.sh can re-run with a relative
$0 that may not be found; update the fallback message in the block that runs
/usr/bin/log (the code referencing $BUNDLE_ID and echo "Try: sudo $0") to either
remove the "sudo $0" suggestion or replace it with a resolved absolute path
(e.g., use realpath or zsh "${0:A}") so sudo will locate the script, and also
clarify that log show often doesn't require root for user-owned subsystems (so
make the message conditional or less prescriptive). Ensure you update the echo
lines near the /usr/bin/log invocation to reference the resolved path variable
or drop the sudo suggestion accordingly.
In `@vendor/CEFWebView/fix_cef_framework.sh`:
- Around line 20-26: The script runs cd "$FRAMEWORK_PATH" without checking
failure which can cause ln -sf to operate in the wrong directory; fix by adding
a safe-shell header (e.g., set -euo pipefail) near the top and immediately after
cd "$FRAMEWORK_PATH" verify success (test the exit status or use a guarded form)
and exit with a non‑zero status if cd fails so the subsequent ln -sf
"Versions/Current/Chromium Embedded Framework", ln -sf
"Versions/Current/Resources", and the conditional ln -sf
"Versions/Current/Libraries" only run when FRAMEWORK_PATH was successfully
entered.
In `@vendor/CEFWebView/IMPLEMENTATION_GUIDE.md`:
- Around line 289-295: The sample uses an undefined urlInput in the TextField
onSubmit; introduce and use a state-bound string (e.g., add an `@State` var
urlInput = url?.absoluteString ?? "" inside the ContentView) and bind the
TextField to that state (TextField("URL", text: $urlInput)), then parse and
assign to url in the onSubmit closure (if let newURL = URL(string: urlInput) {
url = newURL }). This keeps the existing TextField, onSubmit, url, and urlInput
identifiers consistent and makes the example compile.
In `@vendor/CEFWebView/JIT.md`:
- Around line 30-32: Update the JIT.md guidance to stop recommending
com.apple.security.get-task-allow for Release builds: split entitlements into
"Debug" (com.apple.security.cs.allow-jit,
com.apple.security.cs.allow-unsigned-executable-memory,
com.apple.security.get-task-allow) and "Release"
(com.apple.security.cs.allow-jit, and only include
com.apple.security.cs.allow-unsigned-executable-memory if you have verified the
target CEF/V8 version actually requires it), remove get-task-allow from the
Release recommendation, correct the rationale for "task name port" errors to
state they are typically fixed by configuring the helper apps' own entitlements
plus com.apple.security.cs.disable-library-validation on the main app (not by
granting get-task-allow to the main target), and add a short note to verify
notarization rejection behavior and whether allow-unsigned-executable-memory is
still necessary for your CEF/V8 version.
In `@vendor/CEFWebView/Sources/CEFWebView/CEFBridge.swift`:
- Around line 196-219: The current logic unconditionally clears
lastMainFrameLoadErrorCode/lastMainFrameLoadErrorText when handling the
"renderer" helper failure, which allows a subsequent NotifyHelperFailed (no
error info) to clobber a previously-captured OnLoadError; change the branch that
handles mainFrameLoadErrorCode so you only assign to lastMainFrameLoadErrorCode
and lastMainFrameLoadErrorText when mainFrameLoadErrorCode is non-nil (i.e.,
don't set them to nil in the else), and rely on the existing
clearRendererFailureStateForNewNavigation() to explicitly reset these values at
navigation start; ensure applyAccumulatedHelperFlags still reads the preserved
values into CEFWebViewState so rendererFailureStatusLine retains the original
load error details.
In `@vendor/CEFWebView/Sources/CEFWebView/CEFWebView.swift`:
- Around line 58-72: The representable mutates observable state synchronously
inside makeNSView/updateNSView (e.g., state.initializationError,
state.setBrowserHost(host), state.currentURL = url) which SwiftUI forbids; fix
by deferring all state writes performed in CEFWebView.makeNSView and
CEFWebView.updateNSView to the next runloop on the main actor (wrap each state
mutation — including the error-path assignments and calls to
state.setBrowserHost and assignments to state.currentURL — in Task { `@MainActor`
in ... } or DispatchQueue.main.async { ... }) so mutations occur after the view
update completes.
In `@vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm`:
- Around line 225-230: Three handler methods (OnBeforeBrowse, OnLoadError,
OnRenderProcessTerminated) are missing the C++ override specifier which risks
silent signature mismatches; update the method definitions/declarations for
OnBeforeBrowse, OnLoadError and OnRenderProcessTerminated in CEFWrapper.mm to
append the override keyword (matching the style used by
OnTitleChange/OnAddressChange/OnLoadStart) so they explicitly override the base
class virtuals and will produce compile errors if the CEF signatures change.
---
Nitpick comments:
In `@scripts/setup-cefwebview.sh`:
- Line 11: The variable FRAMEWORKS in scripts/setup-cefwebview.sh is declared
but never used (triggering SC2034); either remove the
FRAMEWORKS="$PKG_ROOT/Frameworks" line, or explicitly pass/export it into the
sub-script that expects it (build_cpp.sh) so it becomes FRAMEWORKS_DIR there
(build_cpp.sh computes FRAMEWORKS_DIR from SRCROOT); update the file to either
drop the unused FRAMEWORKS assignment or export/forward it to build_cpp.sh to
avoid the unused-variable warning and clarify intent.
- Around line 36-38: The temporary tarball cleanup is skipped if tar fails,
leaving large partial files; update the download/extract sequence around
TMP_TARBALL, CEF_ROOT and the tar invocation so the temp file is always removed
on error — e.g., set a trap to rm -f "$TMP_TARBALL" on EXIT/ERR or download to a
staging file (TMP_TARBALL.tmp) and only move/rename it to TMP_TARBALL after a
successful transfer, and extract into a staging directory before moving into
CEF_ROOT on success; apply the change to the curl/tar/rm sequence that uses
TMP_TARBALL, CEF_URL and tar -xjf to guarantee cleanup on failure.
In `@vendor/CEFWebView/.gitignore`:
- Line 10: Update the .gitignore entry so it explicitly ignores the Frameworks
directory by changing the pattern "Frameworks" to "Frameworks/"; locate the
entry in vendor/CEFWebView/.gitignore (the line containing "Frameworks") and
replace it with "Frameworks/" to avoid accidentally ignoring a file named
Frameworks and to match the root .gitignore style
(vendor/CEFWebView/Frameworks/).
In `@vendor/CEFWebView/build_cpp.sh`:
- Around line 27-31: The current build_cpp.sh subshell runs a full Xcode build
without pinning the macOS deployment target or restricting targets, which can
produce binaries with a different MACOSX_DEPLOYMENT_TARGET than the rest of the
project; change the subshell in build_cpp.sh so cmake is invoked with a fixed
deployment target (e.g., -DCMAKE_OSX_DEPLOYMENT_TARGET=14.0) and invoke
xcodebuild with an explicit target (e.g., -target libcef_dll_wrapper) and/or
pass MACOSX_DEPLOYMENT_TARGET=14.0 to xcodebuild, or alternatively document that
setup-cefwebview.sh is the canonical entrypoint that pre-builds with -target
libcef_dll_wrapper and -DCMAKE_OSX_DEPLOYMENT_TARGET=13.0 so callers know to use
it. Ensure references to libcef_dll_wrapper, build_cpp.sh, setup-cefwebview.sh
and MACOSX_DEPLOYMENT_TARGET are used to locate where to apply the change.
In `@vendor/CEFWebView/build.sh`:
- Around line 119-136: The RELEASE_NOTES here-doc currently embeds a hard-coded
"CEF Version: 146.0.10" which will drift; update the block that writes
RELEASE_NOTES so the CEF version is derived at build time instead of literal:
run the existing CEF setup script with its print-version option (or read the
canonical CEF VERSION/cef_version source) to produce a CEF_VERSION variable,
then interpolate that variable into the here-doc where "CEF Version:" is written
(modify the RELEASE_NOTES assignment / the cat << EOF block accordingly) so the
generated RELEASE_NOTES reflects the actual CEF version.
In `@vendor/CEFWebView/README.md`:
- Around line 1-7: The README for CEFWebView is a minimal stub; update it to
include discoverability pointers to the integration/setup docs by adding short
links or sentences referencing INTEGRATION_NOTES.md and/or
IMPLEMENTATION_GUIDE.md and the setup script scripts/setup-cefwebview.sh so
downstream contributors can find the setup flow from the package root (e.g., add
a “Getting Started” or “Setup” line that names those files and where to find
them).
In `@vendor/CEFWebView/Sources/CEFHelperRenderer/main.mm`:
- Around line 8-22: Gate the verbose NSLog argv dump in CEFHelperRenderer's main
function so it only runs in debug builds: wrap the per-argument NSLog loop (the
block that prints argv[i]) in a debug-only macro (e.g., `#if` DEBUG / `#ifdef`
NDEBUG inverted as your project uses) or a build-config check, leaving the
minimal high-level logs and the call to [CEFWrapper
executeSubprocessWithArgc:argv] unchanged; also make the logging style
consistent with the other helper entrypoint (CEFHelper main) by keeping only
non-sensitive summary logs in release builds and moving detailed argv printing
behind the debug guard.
In `@vendor/CEFWebView/Sources/CEFWebView/CEFWebView.swift`:
- Around line 35-41: The CEFWebView currently declares "@Binding var state:
CEFWebViewState" and accepts a Binding in init even though CEFWebViewState is a
`@MainActor` `@Observable` reference type; remove the unnecessary binding: change
the stored property to a plain "var state: CEFWebViewState" and update the
initializer to take "state: CEFWebViewState" (adjust self.state assignment
accordingly), then update all call sites (e.g., the Preview at line ~227) to
pass the state instance (not $state) and adjust NSViewRepresentable/coordinator
usage to own or reference the observable directly where two‑way binding is
required.
In `@vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm`:
- Around line 428-430: Remove the redundant early assignments to
settings.log_severity and settings.log_file (the calls setting
settings.log_severity = LOGSEVERITY_VERBOSE and
CefString(&settings.log_file).FromString("/tmp/cef_debug.log")) since they are
overwritten later by the NSCachesDirectory/com.chromium.webview/debug.log
assignment; keep only the later cache-dir assignment for settings.log_file and
its matching settings.log_severity so there is a single source of truth and no
dead store in CEFWrapper.mm.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 431d5811-7093-4a50-b79f-8415a7500c43
📒 Files selected for processing (22)
.gitignorescripts/setup-cefwebview.shvendor/CEFWebView/.gitattributesvendor/CEFWebView/.gitignorevendor/CEFWebView/IMPLEMENTATION_GUIDE.mdvendor/CEFWebView/INTEGRATION_NOTES.mdvendor/CEFWebView/JIT.mdvendor/CEFWebView/LICENSEvendor/CEFWebView/Package.swiftvendor/CEFWebView/README.mdvendor/CEFWebView/Sources/CEFHelper/main.mmvendor/CEFWebView/Sources/CEFHelperRenderer/main.mmvendor/CEFWebView/Sources/CEFWebView/CEFBridge.swiftvendor/CEFWebView/Sources/CEFWebView/CEFWebView.swiftvendor/CEFWebView/Sources/CEFWebView/CEFWebViewState.swiftvendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mmvendor/CEFWebView/Sources/CEFWrapper/include/CEFWrapper.hvendor/CEFWebView/build.shvendor/CEFWebView/build_cpp.shvendor/CEFWebView/capture_logs.shvendor/CEFWebView/diagnose_renderer.shvendor/CEFWebView/fix_cef_framework.sh
| CEF_VERSION="${CEF_VERSION:-146.0.5+g4db0d88+chromium-146.0.7680.65}" | ||
| CEF_PLATFORM="${CEF_PLATFORM:-macosarm64}" | ||
| CEF_DIST="cef_binary_${CEF_VERSION}_${CEF_PLATFORM}_beta" | ||
| CEF_TARBALL="${CEF_DIST}.tar.bz2" | ||
| CEF_URL="https://cef-builds.spotifycdn.com/${CEF_TARBALL//+/%2B}" |
There was a problem hiding this comment.
Arch/platform override can desync from the wrapper build flags.
CEF_PLATFORM is env-overridable (e.g., macosx64 for Intel), but line 47 hardcodes -DPROJECT_ARCH=arm64. If someone overrides CEF_PLATFORM to an x86_64 build, the wrapper will still be compiled for arm64 against x86_64 CEF headers/libs and fail to link or produce a wrong-arch binary silently. Either derive PROJECT_ARCH from CEF_PLATFORM or refuse non-arm64 platforms explicitly.
🛠️ Suggested derivation
CEF_PLATFORM="${CEF_PLATFORM:-macosarm64}"
+case "$CEF_PLATFORM" in
+ macosarm64) PROJECT_ARCH="arm64" ;;
+ macosx64) PROJECT_ARCH="x86_64" ;;
+ *) echo "Unsupported CEF_PLATFORM: $CEF_PLATFORM" >&2; exit 1 ;;
+esacThen use -DPROJECT_ARCH="$PROJECT_ARCH" on line 47.
📝 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.
| CEF_VERSION="${CEF_VERSION:-146.0.5+g4db0d88+chromium-146.0.7680.65}" | |
| CEF_PLATFORM="${CEF_PLATFORM:-macosarm64}" | |
| CEF_DIST="cef_binary_${CEF_VERSION}_${CEF_PLATFORM}_beta" | |
| CEF_TARBALL="${CEF_DIST}.tar.bz2" | |
| CEF_URL="https://cef-builds.spotifycdn.com/${CEF_TARBALL//+/%2B}" | |
| CEF_VERSION="${CEF_VERSION:-146.0.5+g4db0d88+chromium-146.0.7680.65}" | |
| CEF_PLATFORM="${CEF_PLATFORM:-macosarm64}" | |
| case "$CEF_PLATFORM" in | |
| macosarm64) PROJECT_ARCH="arm64" ;; | |
| macosx64) PROJECT_ARCH="x86_64" ;; | |
| *) echo "Unsupported CEF_PLATFORM: $CEF_PLATFORM" >&2; exit 1 ;; | |
| esac | |
| CEF_DIST="cef_binary_${CEF_VERSION}_${CEF_PLATFORM}_beta" | |
| CEF_TARBALL="${CEF_DIST}.tar.bz2" | |
| CEF_URL="https://cef-builds.spotifycdn.com/${CEF_TARBALL//+/%2B}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-cefwebview.sh` around lines 15 - 19, The build currently allows
overriding CEF_PLATFORM but still hardcodes PROJECT_ARCH to arm64 when invoking
the wrapper build, causing arch mismatch; update the script to derive
PROJECT_ARCH from the CEF_PLATFORM variable (e.g., map "macosarm64" -> "arm64",
"macosx64" or "macosintel" -> "x86_64") and export/use that derived PROJECT_ARCH
when invoking the wrapper build (the place that supplies -DPROJECT_ARCH).
Alternatively, if you only support arm64, validate CEF_PLATFORM early and exit
with an error if a non-arm64 platform is provided so the wrapper is not compiled
with the wrong -DPROJECT_ARCH.
| #!/usr/bin/env zsh | ||
|
|
||
| set -e | ||
|
|
There was a problem hiding this comment.
Enable pipefail so xcodebuild | tee failures aren’t silently swallowed.
Under zsh with just set -e, the exit status of a pipeline is the last command’s (tee), so xcodebuild … | tee "$PROJECT_DIR/build.log" || { … } in build() (line 76) and release() (line 102) will report success even when xcodebuild fails — both log_error and return 1 become unreachable, and callers (CI, devs) see a green build with a log full of errors.
🛠️ Proposed fix
#!/usr/bin/env zsh
-set -e
+set -e
+set -o pipefail📝 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.
| #!/usr/bin/env zsh | |
| set -e | |
| #!/usr/bin/env zsh | |
| set -e | |
| set -o pipefail | |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 1-1: ShellCheck only supports sh/bash/dash/ksh/'busybox sh' scripts. Sorry!
(SC1071)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/CEFWebView/build.sh` around lines 1 - 4, The script enables errexit
but not pipefail, so failures in the xcodebuild | tee pipelines inside build()
and release() are masked; update vendor/CEFWebView/build.sh to enable pipefail
(e.g., set -o pipefail) alongside the existing set -e near the top of the file
so that any failure of xcodebuild in the pipelines inside build() and release()
correctly propagates and triggers the existing log_error/return 1 handling.
| build_cef() { | ||
| log_info "Building CEF dependencies..." | ||
|
|
||
| cd "$PROJECT_DIR" | ||
| ./build_cpp.sh > /dev/null 2>&1 || { | ||
| log_error "CEF build failed" | ||
| return 1 | ||
| } | ||
|
|
||
| log_success "CEF dependencies built" | ||
| } |
There was a problem hiding this comment.
Don’t send build_cpp.sh output to /dev/null.
./build_cpp.sh > /dev/null 2>&1 hides every CEF build error, including the most common failures (missing vendor/CEFWebView/CEF/, cmake/ninja errors, codesign failures from fix_cef_framework.sh). The user only sees CEF build failed and has to re-run manually to learn why. Tee it to a log like the Xcode steps do.
🛠️ Proposed fix
build_cef() {
log_info "Building CEF dependencies..."
cd "$PROJECT_DIR"
- ./build_cpp.sh > /dev/null 2>&1 || {
- log_error "CEF build failed"
+ ./build_cpp.sh 2>&1 | tee "$PROJECT_DIR/build_cpp.log" || {
+ log_error "CEF build failed. See build_cpp.log for details."
return 1
}
log_success "CEF dependencies built"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/CEFWebView/build.sh` around lines 36 - 46, The build_cef function
currently hides all build_cpp.sh output by redirecting to /dev/null; change it
to capture and stream the output to a log so users see errors and you can
reference the log on failure. Invoke ./build_cpp.sh from inside build_cef but
pipe its stdout/stderr through tee to a persistent file (e.g.,
"$PROJECT_DIR/cef_build.log") so output is both shown and saved, preserve the
exit status, and when detecting failure call log_error including the log file
path (and any relevant helper scripts like fix_cef_framework.sh) before
returning 1; keep the surrounding log_info/log_success behavior.
| /usr/bin/log show --predicate "processImagePath CONTAINS \"$BUNDLE_ID\" OR process == \"CEFHelper\" OR process == \"CEFHelperRenderer\" OR process == \"WebView\"" --debug --info --last 2m 2>/dev/null || { | ||
| echo "⚠️ Could not read system logs. You may need to run with elevated permissions." | ||
| echo "Try: sudo $0" | ||
| } |
There was a problem hiding this comment.
sudo $0 hint can silently re-run with a relative path.
If the user invoked this script as ./capture_logs.sh from another cwd or via zsh capture_logs.sh, $0 is a relative/bare name that sudo may not resolve on a fresh PATH. Also, log show normally does not require root for user-owned subsystems, so the elevation advice may be misleading. Consider either dropping the suggestion or using "$(realpath "$0")" / "${0:A}" (zsh).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/CEFWebView/capture_logs.sh` around lines 43 - 46, The sudo hint in
capture_logs.sh can re-run with a relative $0 that may not be found; update the
fallback message in the block that runs /usr/bin/log (the code referencing
$BUNDLE_ID and echo "Try: sudo $0") to either remove the "sudo $0" suggestion or
replace it with a resolved absolute path (e.g., use realpath or zsh "${0:A}") so
sudo will locate the script, and also clarify that log show often doesn't
require root for user-owned subsystems (so make the message conditional or less
prescriptive). Ensure you update the echo lines near the /usr/bin/log invocation
to reference the resolved path variable or drop the sudo suggestion accordingly.
| cd "$FRAMEWORK_PATH" | ||
| ln -sf "Versions/Current/Chromium Embedded Framework" "Chromium Embedded Framework" | ||
| ln -sf "Versions/Current/Resources" "Resources" | ||
|
|
||
| if [ -d "Versions/A/Libraries" ]; then | ||
| ln -sf "Versions/Current/Libraries" "Libraries" | ||
| fi |
There was a problem hiding this comment.
Guard the cd — on failure, rm/ln run in the wrong directory.
This script has no set -e, and cd "$FRAMEWORK_PATH" is unchecked (shellcheck SC2164). If the cd fails for any reason (e.g., the path contains an unexpected character, or a prior step left the framework in an odd state), the subsequent ln -sf "Versions/Current/..." commands will create symlinks in whatever cwd Xcode invoked the script from — silently corrupting the build directory.
🛡️ Proposed fix
-cd "$FRAMEWORK_PATH"
+cd "$FRAMEWORK_PATH" || { echo "❌ Failed to cd into $FRAMEWORK_PATH"; exit 1; }Alternatively, add set -euo pipefail near the top of the script.
📝 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.
| cd "$FRAMEWORK_PATH" | |
| ln -sf "Versions/Current/Chromium Embedded Framework" "Chromium Embedded Framework" | |
| ln -sf "Versions/Current/Resources" "Resources" | |
| if [ -d "Versions/A/Libraries" ]; then | |
| ln -sf "Versions/Current/Libraries" "Libraries" | |
| fi | |
| cd "$FRAMEWORK_PATH" || { echo "❌ Failed to cd into $FRAMEWORK_PATH"; exit 1; } | |
| ln -sf "Versions/Current/Chromium Embedded Framework" "Chromium Embedded Framework" | |
| ln -sf "Versions/Current/Resources" "Resources" | |
| if [ -d "Versions/A/Libraries" ]; then | |
| ln -sf "Versions/Current/Libraries" "Libraries" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 20-20: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/CEFWebView/fix_cef_framework.sh` around lines 20 - 26, The script runs
cd "$FRAMEWORK_PATH" without checking failure which can cause ln -sf to operate
in the wrong directory; fix by adding a safe-shell header (e.g., set -euo
pipefail) near the top and immediately after cd "$FRAMEWORK_PATH" verify success
(test the exit status or use a guarded form) and exit with a non‑zero status if
cd fails so the subsequent ln -sf "Versions/Current/Chromium Embedded
Framework", ln -sf "Versions/Current/Resources", and the conditional ln -sf
"Versions/Current/Libraries" only run when FRAMEWORK_PATH was successfully
entered.
|
|
||
| TextField("URL", text: .constant(url?.absoluteString ?? "")) | ||
| .onSubmit { | ||
| if let newURL = URL(string: urlInput) { | ||
| url = newURL | ||
| } | ||
| } |
There was a problem hiding this comment.
Broken example: urlInput is undefined.
The ContentView snippet reads from url?.absoluteString but then references urlInput in onSubmit. Either introduce an @State var urlInput = "" bound to the TextField, or parse from the TextField's binding directly. Minor, but this example won't compile if copy-pasted.
📝 Proposed fix
- TextField("URL", text: .constant(url?.absoluteString ?? ""))
- .onSubmit {
- if let newURL = URL(string: urlInput) {
- url = newURL
- }
- }
+ `@State` var urlInput = url?.absoluteString ?? ""
+ TextField("URL", text: $urlInput)
+ .onSubmit {
+ if let newURL = URL(string: urlInput) {
+ url = newURL
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/CEFWebView/IMPLEMENTATION_GUIDE.md` around lines 289 - 295, The sample
uses an undefined urlInput in the TextField onSubmit; introduce and use a
state-bound string (e.g., add an `@State` var urlInput = url?.absoluteString ?? ""
inside the ContentView) and bind the TextField to that state (TextField("URL",
text: $urlInput)), then parse and assign to url in the onSubmit closure (if let
newURL = URL(string: urlInput) { url = newURL }). This keeps the existing
TextField, onSubmit, url, and urlInput identifiers consistent and makes the
example compile.
| <!-- Allows debugger to attach and enables Mach IPC for helper processes --> | ||
| <key>com.apple.security.get-task-allow</key> | ||
| <true/> |
There was a problem hiding this comment.
Flag: get-task-allow should not ship in Release.
com.apple.security.get-task-allow lets any process attach a debugger and read memory from the signed binary; Apple rejects notarization when it’s present, and it materially weakens the Hardened Runtime guarantees cmux relies on elsewhere. The doc currently instructs readers to "Apply to both Debug and Release configurations" (line 43), which will bake this into shipped builds.
Recommend splitting the guidance:
- Debug entitlements:
allow-jit,allow-unsigned-executable-memory,get-task-allow. - Release entitlements:
allow-jitonly (andallow-unsigned-executable-memoryonly if V8 actually needs it on the target CEF version — many recent CEF/V8 builds no longer require it).
Also, the "Fixes 'Unable to obtain a task name port right' errors from CEF helper processes" rationale for get-task-allow on line 51 is misleading: those errors are typically resolved via the helper apps’ own entitlements plus com.apple.security.cs.disable-library-validation on the outer app for loading CEF’s unsigned helper bundle, not by granting get-task-allow to the main target. Worth correcting before anyone copies this into the Xcode target for Phase 2.
Does Apple notarization reject macOS apps that ship with com.apple.security.get-task-allow entitlement in release builds, and is com.apple.security.cs.allow-unsigned-executable-memory still required for recent CEF/Chromium V8 on macOS Hardened Runtime?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/CEFWebView/JIT.md` around lines 30 - 32, Update the JIT.md guidance to
stop recommending com.apple.security.get-task-allow for Release builds: split
entitlements into "Debug" (com.apple.security.cs.allow-jit,
com.apple.security.cs.allow-unsigned-executable-memory,
com.apple.security.get-task-allow) and "Release"
(com.apple.security.cs.allow-jit, and only include
com.apple.security.cs.allow-unsigned-executable-memory if you have verified the
target CEF/V8 version actually requires it), remove get-task-allow from the
Release recommendation, correct the rationale for "task name port" errors to
state they are typically fixed by configuring the helper apps' own entitlements
plus com.apple.security.cs.disable-library-validation on the main app (not by
granting get-task-allow to the main target), and add a short note to verify
notarization rejection behavior and whether allow-unsigned-executable-memory is
still necessary for your CEF/V8 version.
| case "renderer": | ||
| helperFailedRenderer = true | ||
| helperSpawnedRenderer = false | ||
| if let code = mainFrameLoadErrorCode { | ||
| lastMainFrameLoadErrorCode = code | ||
| lastMainFrameLoadErrorText = mainFrameLoadErrorText | ||
| } else { | ||
| lastMainFrameLoadErrorCode = nil | ||
| lastMainFrameLoadErrorText = nil | ||
| } | ||
| default: | ||
| cefLogger.warning("Unknown helper type from CEF: \(type)") | ||
| return | ||
| } | ||
|
|
||
| if let state = activeBrowserHost?.state { | ||
| applyAccumulatedHelperFlags(to: state) | ||
| if lowerType == "renderer" { | ||
| state.isLoading = false | ||
| } | ||
| } else { | ||
| cefLogger.debug("No activeBrowserHost.state yet; failure flags buffered for attach") | ||
| } | ||
| } |
There was a problem hiding this comment.
Renderer terminate-after-load-error clobbers the buffered OnLoadError details.
When the sequence is OnLoadError (fires NotifyHelperFailedWithLoadError with errorCode/errorText) followed by OnRenderProcessTerminated (fires NotifyHelperFailed with no code), the second call hits the else branch and overwrites lastMainFrameLoadErrorCode / lastMainFrameLoadErrorText back to nil. applyAccumulatedHelperFlags then propagates those nils into CEFWebViewState, so rendererFailureStatusLine downgrades from "Load failed: <text> (error <code>)" to the generic "Renderer process failed" and the user loses the only diagnostic the UI had.
🛠️ Proposed fix: only overwrite when a new error code is supplied
case "renderer":
helperFailedRenderer = true
helperSpawnedRenderer = false
if let code = mainFrameLoadErrorCode {
lastMainFrameLoadErrorCode = code
lastMainFrameLoadErrorText = mainFrameLoadErrorText
- } else {
- lastMainFrameLoadErrorCode = nil
- lastMainFrameLoadErrorText = nil
}
+ // else: keep whatever OnLoadError previously buffered so the UI
+ // can still show the real load failure after a renderer crash.Paired with this, consider a dedicated reset at navigation start (you already have clearRendererFailureStateForNewNavigation()), so the only path that nils these out is an explicit new navigation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/CEFWebView/Sources/CEFWebView/CEFBridge.swift` around lines 196 - 219,
The current logic unconditionally clears
lastMainFrameLoadErrorCode/lastMainFrameLoadErrorText when handling the
"renderer" helper failure, which allows a subsequent NotifyHelperFailed (no
error info) to clobber a previously-captured OnLoadError; change the branch that
handles mainFrameLoadErrorCode so you only assign to lastMainFrameLoadErrorCode
and lastMainFrameLoadErrorText when mainFrameLoadErrorCode is non-nil (i.e.,
don't set them to nil in the else), and rely on the existing
clearRendererFailureStateForNewNavigation() to explicitly reset these values at
navigation start; ensure applyAccumulatedHelperFlags still reads the preserved
values into CEFWebViewState so rendererFailureStatusLine retains the original
load error details.
| do { | ||
| logger.debug("🔧 Attempting CEF initialization...") | ||
| try CEFApplication.shared.initialize() | ||
| logger.debug("✅ CEFApplication.shared.initialize() succeeded") | ||
| } catch let error as NSError { | ||
| let errorDesc = "CEF Initialization Failed: \(error.localizedDescription)" | ||
| logger.error("❌ CEF initialization failed: \(errorDesc, privacy: .public)") | ||
| state.initializationError = errorDesc | ||
| return container | ||
| } catch { | ||
| let errorDesc = "CEF Initialization Failed: \(error)" | ||
| logger.error("❌ CEF initialization failed: \(errorDesc, privacy: .public)") | ||
| state.initializationError = errorDesc | ||
| return container | ||
| } |
There was a problem hiding this comment.
State is mutated during makeNSView / updateNSView, which SwiftUI disallows during a view update.
Writes like state.initializationError = errorDesc (lines 65, 70, 147, 151), state.setBrowserHost(host) / state.currentURL = url (lines 131, 134, 170) happen synchronously inside the representable’s make/update calls. Once any observing view reads these (@Observable tracks reads), SwiftUI will emit “Modifying state during view update, this will cause undefined behavior” and can trigger redundant re-entrant updates. Defer mutations to the next runloop turn.
🛠️ Suggested pattern
- state.initializationError = errorDesc
+ Task { `@MainActor` in state.initializationError = errorDesc }Apply the same Task { @mainactor in ... } (or DispatchQueue.main.async { ... }) wrap around every state.* write performed inside makeNSView and updateNSView, including state.setBrowserHost(host), state.currentURL = url, and both error-path assignments.
Also applies to: 121-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/CEFWebView/Sources/CEFWebView/CEFWebView.swift` around lines 58 - 72,
The representable mutates observable state synchronously inside
makeNSView/updateNSView (e.g., state.initializationError,
state.setBrowserHost(host), state.currentURL = url) which SwiftUI forbids; fix
by deferring all state writes performed in CEFWebView.makeNSView and
CEFWebView.updateNSView to the next runloop on the main actor (wrap each state
mutation — including the error-path assignments and calls to
state.setBrowserHost and assignments to state.currentURL — in Task { `@MainActor`
in ... } or DispatchQueue.main.async { ... }) so mutations occur after the view
update completes.
| void OnRenderProcessTerminated(CefRefPtr<CefBrowser> browser, | ||
| TerminationStatus status) { | ||
| NSLog(@"💥 OnRenderProcessTerminated: status=%d (0=normal, 1=abnormal, 2=crashed, 3=oom, 4=launch_failed)", (int)status); | ||
| NSLog(@"🔔 Calling NotifyHelperFailed(\"renderer\") from OnRenderProcessTerminated"); | ||
| NotifyHelperFailed("renderer"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List all CEF handler-looking methods in the vendored client and flag any without `override`.
rg -nU --type=cpp -C1 '\b(On[A-Z][A-Za-z]+)\s*\([^;]*?\)\s*\{' vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm \
| rg -v 'override'Repository: manaflow-ai/cmux
Length of output: 476
🏁 Script executed:
#!/bin/bash
# Find the class definition that contains OnRenderProcessTerminated
rg -n "class.*\{" vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm | head -20Repository: manaflow-ai/cmux
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Look for the class containing line 225 and check inheritance
sed -n '1,230p' vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm | grep -E "^(class|struct|:)" | tail -10Repository: manaflow-ai/cmux
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Search for all handler methods (On* pattern) in the entire file and note override status
rg -n "^\s+(void|bool|int|auto)\s+On[A-Z][A-Za-z0-9_]*\s*\(" vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mmRepository: manaflow-ai/cmux
Length of output: 767
🏁 Script executed:
#!/bin/bash
# Check the CEFWrapper.mm file structure - get lines around 225 with more context
sed -n '200,235p' vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm | cat -nRepository: manaflow-ai/cmux
Length of output: 2067
🏁 Script executed:
#!/bin/bash
# Get more context on the ChromiumClient class to understand all inherited interfaces
sed -n '100,135p' vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm | cat -nRepository: manaflow-ai/cmux
Length of output: 1386
🏁 Script executed:
#!/bin/bash
# Verify which methods are missing override - complete scan
rg -n "^\s+(void|bool|int|auto|CefRefPtr)\s+On[A-Z][A-Za-z0-9_]*\s*\(" vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm | while IFS=: read -r line method; do
# Check if this line has 'override' keyword
content=$(sed -n "${line}p; $((line+1))p; $((line+2))p" vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm)
if echo "$content" | grep -q "override"; then
echo "Line $line: ✓ $method (HAS override)"
else
echo "Line $line: ✗ $method (MISSING override)"
fi
doneRepository: manaflow-ai/cmux
Length of output: 1032
Add override to OnRenderProcessTerminated and other missing handler methods for C++ virtual method safety.
Without override, a signature mismatch against base class virtual methods (e.g., if CEF adds parameters in future rebases, which CEF has done historically) will compile silently and your handler will stop being called — you lose crash telemetry without warning.
Three methods in this file are missing override:
- Line 139:
OnBeforeBrowse(CefRequestHandler) - Line 188:
OnLoadError(CefLoadHandler) - Line 225:
OnRenderProcessTerminated(CefRequestHandler)
Add override to all three to match the pattern already used by other handlers in the same class (OnTitleChange, OnAddressChange, OnLoadStart, etc.).
Example fix for OnRenderProcessTerminated
- void OnRenderProcessTerminated(CefRefPtr<CefBrowser> browser,
- TerminationStatus status) {
+ void OnRenderProcessTerminated(CefRefPtr<CefBrowser> browser,
+ TerminationStatus status) override {📝 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.
| void OnRenderProcessTerminated(CefRefPtr<CefBrowser> browser, | |
| TerminationStatus status) { | |
| NSLog(@"💥 OnRenderProcessTerminated: status=%d (0=normal, 1=abnormal, 2=crashed, 3=oom, 4=launch_failed)", (int)status); | |
| NSLog(@"🔔 Calling NotifyHelperFailed(\"renderer\") from OnRenderProcessTerminated"); | |
| NotifyHelperFailed("renderer"); | |
| } | |
| void OnRenderProcessTerminated(CefRefPtr<CefBrowser> browser, | |
| TerminationStatus status) override { | |
| NSLog(@"💥 OnRenderProcessTerminated: status=%d (0=normal, 1=abnormal, 2=crashed, 3=oom, 4=launch_failed)", (int)status); | |
| NSLog(@"🔔 Calling NotifyHelperFailed(\"renderer\") from OnRenderProcessTerminated"); | |
| NotifyHelperFailed("renderer"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/CEFWebView/Sources/CEFWrapper/CEFWrapper.mm` around lines 225 - 230,
Three handler methods (OnBeforeBrowse, OnLoadError, OnRenderProcessTerminated)
are missing the C++ override specifier which risks silent signature mismatches;
update the method definitions/declarations for OnBeforeBrowse, OnLoadError and
OnRenderProcessTerminated in CEFWrapper.mm to append the override keyword
(matching the style used by OnTitleChange/OnAddressChange/OnLoadStart) so they
explicitly override the base class virtuals and will produce compile errors if
the CEF signatures change.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 22 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="scripts/setup-cefwebview.sh">
<violation number="1" location="scripts/setup-cefwebview.sh:23">
P2: Sibling checkout path goes up two levels, so local `cef-swift-mvp` reuse can fail and force unnecessary re-downloads.</violation>
<violation number="2" location="scripts/setup-cefwebview.sh:36">
P1: Verify the tarball checksum before extraction; currently the script executes a network download without integrity validation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| echo "⬇︎ Downloading CEF $CEF_VERSION ($CEF_PLATFORM)..." | ||
| echo " $CEF_URL" | ||
| TMP_TARBALL="$CEF_ROOT/$CEF_TARBALL" | ||
| curl -fL --retry 3 -o "$TMP_TARBALL" "$CEF_URL" |
There was a problem hiding this comment.
P1: Verify the tarball checksum before extraction; currently the script executes a network download without integrity validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/setup-cefwebview.sh, line 36:
<comment>Verify the tarball checksum before extraction; currently the script executes a network download without integrity validation.</comment>
<file context>
@@ -0,0 +1,57 @@
+ echo "⬇︎ Downloading CEF $CEF_VERSION ($CEF_PLATFORM)..."
+ echo " $CEF_URL"
+ TMP_TARBALL="$CEF_ROOT/$CEF_TARBALL"
+ curl -fL --retry 3 -o "$TMP_TARBALL" "$CEF_URL"
+ tar -xjf "$TMP_TARBALL" -C "$CEF_ROOT"
+ rm -f "$TMP_TARBALL"
</file context>
|
|
||
| # If a sibling cef-swift-mvp checkout already has the same distribution | ||
| # extracted, reuse it instead of re-downloading hundreds of MB. | ||
| SIBLING_CEF="$REPO_ROOT/../../cef-swift-mvp/third_party/cef/${CEF_DIST}" |
There was a problem hiding this comment.
P2: Sibling checkout path goes up two levels, so local cef-swift-mvp reuse can fail and force unnecessary re-downloads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/setup-cefwebview.sh, line 23:
<comment>Sibling checkout path goes up two levels, so local `cef-swift-mvp` reuse can fail and force unnecessary re-downloads.</comment>
<file context>
@@ -0,0 +1,57 @@
+
+# If a sibling cef-swift-mvp checkout already has the same distribution
+# extracted, reuse it instead of re-downloading hundreds of MB.
+SIBLING_CEF="$REPO_ROOT/../../cef-swift-mvp/third_party/cef/${CEF_DIST}"
+
+mkdir -p "$CEF_ROOT"
</file context>
| SIBLING_CEF="$REPO_ROOT/../../cef-swift-mvp/third_party/cef/${CEF_DIST}" | |
| SIBLING_CEF="$REPO_ROOT/../cef-swift-mvp/third_party/cef/${CEF_DIST}" |
What lands:
- GhosttyTabs.xcodeproj: register vendor/CEFWebView as
XCLocalSwiftPackageReference, add CEFWebView product dep on cmux
target, set FRAMEWORK_SEARCH_PATHS / LIBRARY_SEARCH_PATHS for
Debug + Release.
- Sources/CefDebugWindow.swift: NSWindow with CEFWebView, omnibar,
back/forward/reload, status line. Behind Debug menu only.
- Sources/cmuxApp.swift: Debug > Debug Windows > Chromium (CEF)…
- scripts/embed-cefwebview.sh: runs after reload.sh build. Embeds
Chromium Embedded Framework.framework, builds CEFHelper +
CEFHelperRenderer SPM products into "WebView Helper.app" /
"WebView Helper (Renderer).app" inside Contents/Frameworks/, and
inside-out ad-hoc signs (no --deep — would clobber CLI helper
signatures and trip amfi errno 163 on macOS 26 Tahoe).
Status (cef-spike build):
- CEF initialization, helper spawning, and network requests work
(renderer subprocess alive, google.com/accounts traffic in
~/Library/Caches/com.chromium.webview/debug.log).
- Visible NSView inside the SwiftUI window stays blank — the
known CEFWebView "white page" issue (see vendor/CEFWebView/
IMPLEMENTATION_GUIDE.md "Known Issues"). Needs an upstream fix
in CEFWebView or a switch to OSR/CALayerHost compositing before
this is dogfoodable.
Use: ./scripts/reload.sh --tag cef-spike then
./scripts/embed-cefwebview.sh "$APP_PATH"
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
Sources/CefDebugWindow.swift (1)
1-36: Consider gating the whole file behind#if DEBUG.The doc comment states this is a debug-only window, and the menu entry in
Sources/cmuxApp.swiftis DEBUG-only, but the controller class (and itsCEFWebView/CEF state references) currently compiles into release builds too. Wrapping the file contents in#if DEBUG/#endifkeeps the vendored CEF surface out of shipped binaries and matches the intent described in the header comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CefDebugWindow.swift` around lines 1 - 36, Wrap the entire file (including the import lines and the CefDebugWindowController/CefDebugView references) in a DEBUG-only compile guard so the vendored CEF surface is excluded from release builds; specifically surround the file contents with `#if` DEBUG and `#endif` so types like CefDebugWindowController, CefDebugView and the CEFWebView import are only compiled in debug builds, making sure the file still compiles cleanly when DEBUG is not defined.
🤖 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 1056-1068: The Release config (A5001083) currently includes
CEFWebView search paths and GhosttyTabs unconditionally lists the CEFWebView
package product, but the Embed Frameworks phase (A5001020) is empty so Release
builds will omit Chromium Embedded Framework.framework; fix by either (a) gating
the packageProductDependencies/search paths to Debug-only via an xcconfig or
conditional project setting so GhosttyTabs does not depend on CEFWebView in
Release, or (b) add a Run Script build phase (or populate A5001020) that calls
scripts/embed-cefwebview.sh "${TARGET_BUILD_DIR}/${WRAPPER_NAME}" in Release
builds so the Chromium Embedded Framework is embedded for distribution; update
the project’s packageProductDependencies and build phase settings accordingly
and ensure CEFWrapper links remain consistent.
- Line 11: Wrap the entire contents of CefDebugWindow.swift with a DEBUG-only
compile guard: add `#if` DEBUG at the very top of the file and `#endif` at the very
bottom so the file (referenced as CEF00201 / PBXBuildFile CEF00200) is excluded
from Release builds; ensure the import of CEFWebView and all types/extension
definitions remain inside the guard so no symbols from CefDebugWindow.swift are
referenced in non-DEBUG builds.
In `@scripts/embed-cefwebview.sh`:
- Around line 47-59: The removal step that currently runs rm -f on the framework
entries can leave real directories intact (causing broken symlinks); change the
rm invocation to use rm -rf for the three targets being removed ("Chromium
Embedded Framework", "Resources", "Libraries") so both files and directories are
removed before creating the symlinks, while keeping the existing guard around
"Versions/A/Libraries" and the subsequent ln -sf/ln -s logic intact.
- Around line 122-130: The nested signing loop in scripts/embed-cefwebview.sh
silently swallows errors (the `|| true` and `>/dev/null 2>&1` on the find|while
loop) and explicitly excludes `*.dylib`, which can leave Chromium Mach-O libs
unsigned; either remove the pre-pass entirely and rely on the single recursive
codesign call (`codesign --force --sign "$SIGN_ID" "$APP_FRAMEWORKS/Chromium
Embedded Framework.framework"`) or change the loop (the find/... | while read -r
f; do ... done block) to include `.dylib` files and surface failures (remove `||
true` and stop redirecting stderr/stdout so codesign errors are visible, or log
them to stderr) so nested signing failures are not hidden before the
framework-level codesign.
- Around line 33-37: The script currently hardcodes HELPER_BUILD_DIR to
"$PKG_ROOT/.build/arm64-apple-macosx/release" which breaks on Intel/universal
builds; change the logic that sets HELPER_BUILD_DIR (and thus HELPER_BIN and
RENDERER_BIN) to derive the actual build output directory from SwiftPM instead
(e.g. use swift build --show-bin-path or detect the correct subdirectory under
"$PKG_ROOT/.build" dynamically) and then keep the existing existence checks for
HELPER_BIN and RENDERER_BIN; update the code that defines HELPER_BUILD_DIR,
HELPER_BIN, and RENDERER_BIN and ensure the following checks [ -x "$HELPER_BIN"
] and [ -x "$RENDERER_BIN" ] use the resolved path.
In `@Sources/CefDebugWindow.swift`:
- Around line 66-72: The address bar TextField (binding urlText → url via
parseURL in the onSubmit) drifts because urlText is never updated from
navigation; add an .onChange(of: state.currentURL) handler near the TextField to
set urlText = state.currentURL (or its string) when navigation changes, but
avoid clobbering in-progress edits by gating updates with a FocusState/Bool
(e.g., addressBarFocused) so you only mirror state.currentURL into urlText when
the field is not focused; keep references to urlText, url, parseURL,
state.currentURL and the TextField block so the change is localized.
- Line 19: Replace bare UI string literals with localized variants using
String(localized: "key", defaultValue: "...") and add corresponding keys to
Resources/Localizable.xcstrings: update the window.title assignment in
CefDebugWindow (window.title = ...) to use String(localized: "cef.windowTitle",
defaultValue: "Chromium (CEF)"), replace the TextField placeholder "URL" with
String(localized: "cef.urlPlaceholder", defaultValue: "URL"), and change the
error headline "CEF initialization failed" to String(localized:
"cef.initFailedHeadline", defaultValue: "CEF initialization failed"); keep the
brand tokens "Chromium" and "CEF" verbatim inside the defaultValue strings and
add the three keys to Localizable.xcstrings.
In `@Sources/cmuxApp.swift`:
- Line 491: Replace the hard-coded button label "Chromium (CEF)…" with a
localized string; update the Button initializer to use String(localized:
"debug.menu.chromium_cef", defaultValue: "Chromium (CEF)…") (or another
appropriate localization key) so the user-facing label in the Button("Chromium
(CEF)…") call is localized; ensure you apply this change where the Button(...)
for the Debug menu is created.
---
Nitpick comments:
In `@Sources/CefDebugWindow.swift`:
- Around line 1-36: Wrap the entire file (including the import lines and the
CefDebugWindowController/CefDebugView references) in a DEBUG-only compile guard
so the vendored CEF surface is excluded from release builds; specifically
surround the file contents with `#if` DEBUG and `#endif` so types like
CefDebugWindowController, CefDebugView and the CEFWebView import are only
compiled in debug builds, making sure the file still compiles cleanly when DEBUG
is not defined.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87cf324c-30dc-43af-8938-c469b0247504
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojSources/CefDebugWindow.swiftSources/cmuxApp.swiftscripts/embed-cefwebview.sh
|
|
||
| /* Begin PBXBuildFile section */ | ||
| FE001101 /* FileExplorerStore.swift in Sources */ = {isa = PBXBuildFile; fileRef = FE001001 /* FileExplorerStore.swift */; }; | ||
| CEF00200 /* CefDebugWindow.swift in Sources */ = {isa = PBXBuildFile; fileRef = CEF00201 /* CefDebugWindow.swift */; }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i 'cefdebugwindow.swift' -x sed -n '1,40p' {}
echo "---"
fd -i 'cefdebugwindow.swift' -x rg -n '#if DEBUG|#endif|import CEFWebView' {}Repository: manaflow-ai/cmux
Length of output: 1417
🏁 Script executed:
fd -i 'cefdebugwindow.swift' -x cat -n {}Repository: manaflow-ai/cmux
Length of output: 5200
🏁 Script executed:
rg -n '#if DEBUG|#endif' --glob '**/CefDebugWindow.swift'Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
rg -n 'CEF' GhosttyTabs.xcodeproj/project.pbxproj | head -20Repository: manaflow-ai/cmux
Length of output: 1025
Wrap CefDebugWindow.swift entirely in #if DEBUG / #endif.
The file imports CEFWebView and is added unconditionally to the build sources (line 872). Since CEFWebView is not embedded in Release builds, the file must be guarded with #if DEBUG at the top and #endif at the end to prevent link failures.
Also applies to: 872-872
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@GhosttyTabs.xcodeproj/project.pbxproj` at line 11, Wrap the entire contents
of CefDebugWindow.swift with a DEBUG-only compile guard: add `#if` DEBUG at the
very top of the file and `#endif` at the very bottom so the file (referenced as
CEF00201 / PBXBuildFile CEF00200) is excluded from Release builds; ensure the
import of CEFWebView and all types/extension definitions remain inside the guard
so no symbols from CefDebugWindow.swift are referenced in non-DEBUG builds.
| FRAMEWORK_SEARCH_PATHS = ( | ||
| "$(inherited)", | ||
| "$(SRCROOT)/vendor/CEFWebView/Frameworks", | ||
| ); | ||
| INFOPLIST_FILE = Resources/Info.plist; | ||
| LD_RUNPATH_SEARCH_PATHS = ( | ||
| "$(inherited)", | ||
| "@executable_path/../Frameworks", | ||
| ); | ||
| LIBRARY_SEARCH_PATHS = ( | ||
| "$(inherited)", | ||
| "$(SRCROOT)/vendor/CEFWebView/Frameworks", | ||
| ); |
There was a problem hiding this comment.
CEFWebView is wired into Release builds but no Embed Frameworks step exists for Chromium Embedded Framework.framework.
The Release config (A5001083) gains the same FRAMEWORK_SEARCH_PATHS / LIBRARY_SEARCH_PATHS entries as Debug, and the CEFWebView product is listed in packageProductDependencies for the GhosttyTabs target unconditionally. The Embed Frameworks build phase (A5001020) is still empty, so a Release build produced via plain xcodebuild (without running scripts/embed-cefwebview.sh) will link CEFWrapper against Chromium Embedded Framework but ship a cmux.app missing that framework, yielding a dyld load failure at launch.
Per the PR description this is a Phase-2 spike not intended for dogfooding, but it's worth either (a) gating the package product dependency / search paths behind a Debug-only xcconfig, or (b) adding a Run Script phase to Release that invokes scripts/embed-cefwebview.sh "${TARGET_BUILD_DIR}/${WRAPPER_NAME}" so Release builds remain self-contained. Otherwise Release TestFlight/notarized archives will ship broken the moment anything in the app imports CEFWebView.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@GhosttyTabs.xcodeproj/project.pbxproj` around lines 1056 - 1068, The Release
config (A5001083) currently includes CEFWebView search paths and GhosttyTabs
unconditionally lists the CEFWebView package product, but the Embed Frameworks
phase (A5001020) is empty so Release builds will omit Chromium Embedded
Framework.framework; fix by either (a) gating the
packageProductDependencies/search paths to Debug-only via an xcconfig or
conditional project setting so GhosttyTabs does not depend on CEFWebView in
Release, or (b) add a Run Script build phase (or populate A5001020) that calls
scripts/embed-cefwebview.sh "${TARGET_BUILD_DIR}/${WRAPPER_NAME}" in Release
builds so the Chromium Embedded Framework is embedded for distribution; update
the project’s packageProductDependencies and build phase settings accordingly
and ensure CEFWrapper links remain consistent.
| HELPER_BUILD_DIR="$PKG_ROOT/.build/arm64-apple-macosx/release" | ||
| HELPER_BIN="$HELPER_BUILD_DIR/CEFHelper" | ||
| RENDERER_BIN="$HELPER_BUILD_DIR/CEFHelperRenderer" | ||
| [ -x "$HELPER_BIN" ] || { echo "❌ Missing $HELPER_BIN" >&2; exit 1; } | ||
| [ -x "$RENDERER_BIN" ] || { echo "❌ Missing $RENDERER_BIN" >&2; exit 1; } |
There was a problem hiding this comment.
Hardcoded arm64-apple-macosx path breaks on Intel/universal hosts.
HELPER_BUILD_DIR assumes SwiftPM's arm64 triple subdirectory. On an Intel Mac (or when someone sets ARCHS=x86_64), swift build -c release emits to x86_64-apple-macosx/release and the subsequent [ -x "$HELPER_BIN" ] check will fail. Resolve the path from swift build's own output instead.
🔧 Proposed fix
-swift build --package-path "$PKG_ROOT" -c release --product CEFHelper >/dev/null
-swift build --package-path "$PKG_ROOT" -c release --product CEFHelperRenderer >/dev/null
-HELPER_BUILD_DIR="$PKG_ROOT/.build/arm64-apple-macosx/release"
-HELPER_BIN="$HELPER_BUILD_DIR/CEFHelper"
-RENDERER_BIN="$HELPER_BUILD_DIR/CEFHelperRenderer"
+swift build --package-path "$PKG_ROOT" -c release --product CEFHelper >/dev/null
+swift build --package-path "$PKG_ROOT" -c release --product CEFHelperRenderer >/dev/null
+HELPER_BUILD_DIR="$(swift build --package-path "$PKG_ROOT" -c release --show-bin-path)"
+HELPER_BIN="$HELPER_BUILD_DIR/CEFHelper"
+RENDERER_BIN="$HELPER_BUILD_DIR/CEFHelperRenderer"📝 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.
| HELPER_BUILD_DIR="$PKG_ROOT/.build/arm64-apple-macosx/release" | |
| HELPER_BIN="$HELPER_BUILD_DIR/CEFHelper" | |
| RENDERER_BIN="$HELPER_BUILD_DIR/CEFHelperRenderer" | |
| [ -x "$HELPER_BIN" ] || { echo "❌ Missing $HELPER_BIN" >&2; exit 1; } | |
| [ -x "$RENDERER_BIN" ] || { echo "❌ Missing $RENDERER_BIN" >&2; exit 1; } | |
| HELPER_BUILD_DIR="$(swift build --package-path "$PKG_ROOT" -c release --show-bin-path)" | |
| HELPER_BIN="$HELPER_BUILD_DIR/CEFHelper" | |
| RENDERER_BIN="$HELPER_BUILD_DIR/CEFHelperRenderer" | |
| [ -x "$HELPER_BIN" ] || { echo "❌ Missing $HELPER_BIN" >&2; exit 1; } | |
| [ -x "$RENDERER_BIN" ] || { echo "❌ Missing $RENDERER_BIN" >&2; exit 1; } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/embed-cefwebview.sh` around lines 33 - 37, The script currently
hardcodes HELPER_BUILD_DIR to "$PKG_ROOT/.build/arm64-apple-macosx/release"
which breaks on Intel/universal builds; change the logic that sets
HELPER_BUILD_DIR (and thus HELPER_BIN and RENDERER_BIN) to derive the actual
build output directory from SwiftPM instead (e.g. use swift build
--show-bin-path or detect the correct subdirectory under "$PKG_ROOT/.build"
dynamically) and then keep the existing existence checks for HELPER_BIN and
RENDERER_BIN; update the code that defines HELPER_BUILD_DIR, HELPER_BIN, and
RENDERER_BIN and ensure the following checks [ -x "$HELPER_BIN" ] and [ -x
"$RENDERER_BIN" ] use the resolved path.
| ( | ||
| cd "$FW_PATH" | ||
| rm -f "Chromium Embedded Framework" Resources Libraries | ||
| ln -sf "Versions/Current/Chromium Embedded Framework" "Chromium Embedded Framework" | ||
| ln -sf "Versions/Current/Resources" "Resources" | ||
| if [ -d "Versions/A/Libraries" ]; then | ||
| ln -sf "Versions/Current/Libraries" "Libraries" | ||
| fi | ||
| if [ -d "Versions/Current" ] && [ ! -L "Versions/Current" ]; then | ||
| rm -rf "Versions/Current" | ||
| ln -s "A" "Versions/Current" | ||
| fi | ||
| ) |
There was a problem hiding this comment.
rm -f will silently leave stale Resources/Libraries directories when they aren't symlinks.
After cp -R of the already-restructured framework, these entries should arrive as symlinks and rm -f works. But on re-runs against an app where a prior codesign/rsync/manual edit converted Resources or Libraries into a real directory, rm -f is a no-op (it refuses to remove directories without -r), the subsequent ln -sf creates a broken link next to the existing directory, and the framework ends up malformed. Since the script markets itself as idempotent, use rm -rf for these specific entries (they are always rebuilt below):
🔧 Proposed fix
- rm -f "Chromium Embedded Framework" Resources Libraries
+ rm -rf "Chromium Embedded Framework" Resources LibrariesAlso, line 50 unconditionally recreates the Libraries symlink only when Versions/A/Libraries exists (line 52 guard) — that's fine; just the rm above needs hardening.
📝 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.
| ( | |
| cd "$FW_PATH" | |
| rm -f "Chromium Embedded Framework" Resources Libraries | |
| ln -sf "Versions/Current/Chromium Embedded Framework" "Chromium Embedded Framework" | |
| ln -sf "Versions/Current/Resources" "Resources" | |
| if [ -d "Versions/A/Libraries" ]; then | |
| ln -sf "Versions/Current/Libraries" "Libraries" | |
| fi | |
| if [ -d "Versions/Current" ] && [ ! -L "Versions/Current" ]; then | |
| rm -rf "Versions/Current" | |
| ln -s "A" "Versions/Current" | |
| fi | |
| ) | |
| ( | |
| cd "$FW_PATH" | |
| rm -rf "Chromium Embedded Framework" Resources Libraries | |
| ln -sf "Versions/Current/Chromium Embedded Framework" "Chromium Embedded Framework" | |
| ln -sf "Versions/Current/Resources" "Resources" | |
| if [ -d "Versions/A/Libraries" ]; then | |
| ln -sf "Versions/Current/Libraries" "Libraries" | |
| fi | |
| if [ -d "Versions/Current" ] && [ ! -L "Versions/Current" ]; then | |
| rm -rf "Versions/Current" | |
| ln -s "A" "Versions/Current" | |
| fi | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/embed-cefwebview.sh` around lines 47 - 59, The removal step that
currently runs rm -f on the framework entries can leave real directories intact
(causing broken symlinks); change the rm invocation to use rm -rf for the three
targets being removed ("Chromium Embedded Framework", "Resources", "Libraries")
so both files and directories are removed before creating the symlinks, while
keeping the existing guard around "Versions/A/Libraries" and the subsequent ln
-sf/ln -s logic intact.
| # Sign nested executables inside framework first (Versions/A/Helpers/* if any). | ||
| find "$APP_FRAMEWORKS/Chromium Embedded Framework.framework/Versions/A" \ | ||
| -type f -perm -u+x ! -name '*.dylib' 2>/dev/null \ | ||
| | while read -r f; do | ||
| codesign --force --sign "$SIGN_ID" "$f" >/dev/null 2>&1 || true | ||
| done | ||
| # Sign the framework itself. | ||
| codesign --force --sign "$SIGN_ID" \ | ||
| "$APP_FRAMEWORKS/Chromium Embedded Framework.framework" >/dev/null |
There was a problem hiding this comment.
Pre-pass codesign loop silently drops failures and excludes .dylibs.
Two concerns with the nested signing step:
|| true+>/dev/null 2>&1hides every signing failure in this loop, so a broken framework proceeds to the outercodesign --forcewith some internal Mach-Os still unsigned. Either drop the|| trueunderset -e, or at minimum log to stderr.! -name '*.dylib'skips.dylibs, but Chromium's framework includes several (e.g.libvk_swiftshader.dylib,libEGL.dylib,libGLESv2.dylibunderLibraries/). If the goal is to sign every nested Mach-O before the outer framework seal, exclude nothing and letcodesignhandle duplicates; the outercodesignon the framework will also cover them, so the pre-pass is really only needed for top-level helper executables/XPCs.
Consider either dropping the pre-pass entirely and relying on the single framework-level codesign --force at line 129 (which recurses), or including .dylibs and surfacing errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/embed-cefwebview.sh` around lines 122 - 130, The nested signing loop
in scripts/embed-cefwebview.sh silently swallows errors (the `|| true` and
`>/dev/null 2>&1` on the find|while loop) and explicitly excludes `*.dylib`,
which can leave Chromium Mach-O libs unsigned; either remove the pre-pass
entirely and rely on the single recursive codesign call (`codesign --force
--sign "$SIGN_ID" "$APP_FRAMEWORKS/Chromium Embedded Framework.framework"`) or
change the loop (the find/... | while read -r f; do ... done block) to include
`.dylib` files and surface failures (remove `|| true` and stop redirecting
stderr/stdout so codesign errors are visible, or log them to stderr) so nested
signing failures are not hidden before the framework-level codesign.
| backing: .buffered, | ||
| defer: false | ||
| ) | ||
| window.title = "Chromium (CEF)" |
There was a problem hiding this comment.
User-facing strings are not localized.
"Chromium (CEF)" window title, the "URL" TextField placeholder, and "CEF initialization failed" headline are bare literals. Per coding guidelines, all user-facing strings must go through String(localized: "key", defaultValue: "...") with entries in Resources/Localizable.xcstrings. "Chromium" / "CEF" are brand names and should stay untranslated inside the default value, but the surrounding UI chrome ("URL", "… initialization failed") should be localized.
As per coding guidelines: "All user-facing strings must be localized. Use String(localized: "key.name", defaultValue: "English text") for every string shown in the UI".
Also applies to: 66-66, 86-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/CefDebugWindow.swift` at line 19, Replace bare UI string literals
with localized variants using String(localized: "key", defaultValue: "...") and
add corresponding keys to Resources/Localizable.xcstrings: update the
window.title assignment in CefDebugWindow (window.title = ...) to use
String(localized: "cef.windowTitle", defaultValue: "Chromium (CEF)"), replace
the TextField placeholder "URL" with String(localized: "cef.urlPlaceholder",
defaultValue: "URL"), and change the error headline "CEF initialization failed"
to String(localized: "cef.initFailedHeadline", defaultValue: "CEF initialization
failed"); keep the brand tokens "Chromium" and "CEF" verbatim inside the
defaultValue strings and add the three keys to Localizable.xcstrings.
| TextField("URL", text: $urlText) | ||
| .textFieldStyle(.roundedBorder) | ||
| .onSubmit { | ||
| if let parsed = parseURL(urlText) { | ||
| url = parsed | ||
| } | ||
| } |
There was a problem hiding this comment.
urlText drifts out of sync with the actual page URL.
urlText is only written by the user; after navigation, redirects, back/forward, or clicks inside the page, state.currentURL updates but the address bar keeps showing whatever was last typed. Consider mirroring state.currentURL into urlText via .onChange(of: state.currentURL) (skipping updates while the field is focused, to avoid clobbering in-progress typing).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/CefDebugWindow.swift` around lines 66 - 72, The address bar TextField
(binding urlText → url via parseURL in the onSubmit) drifts because urlText is
never updated from navigation; add an .onChange(of: state.currentURL) handler
near the TextField to set urlText = state.currentURL (or its string) when
navigation changes, but avoid clobbering in-progress edits by gating updates
with a FocusState/Bool (e.g., addressBarFocused) so you only mirror
state.currentURL into urlText when the field is not focused; keep references to
urlText, url, parseURL, state.currentURL and the TextField block so the change
is localized.
| Button("Background Debug…") { | ||
| BackgroundDebugWindowController.shared.show() | ||
| } | ||
| Button("Chromium (CEF)…") { |
There was a problem hiding this comment.
Localize the new Debug menu label.
Line 491 adds a user-facing bare string literal ("Chromium (CEF)…"). Please localize it with String(localized:..., defaultValue:...).
Suggested patch
- Button("Chromium (CEF)…") {
+ Button(
+ String(
+ localized: "debug.menu.chromiumCef",
+ defaultValue: "Chromium (CEF)…"
+ )
+ ) {
CefDebugWindowController.shared.show()
}As per coding guidelines, “All user-facing strings must be localized. Use String(localized: "key.name", defaultValue: "English text") for every string shown in the UI.”
📝 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.
| Button("Chromium (CEF)…") { | |
| Button( | |
| String( | |
| localized: "debug.menu.chromiumCef", | |
| defaultValue: "Chromium (CEF)…" | |
| ) | |
| ) { | |
| CefDebugWindowController.shared.show() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` at line 491, Replace the hard-coded button label
"Chromium (CEF)…" with a localized string; update the Button initializer to use
String(localized: "debug.menu.chromium_cef", defaultValue: "Chromium (CEF)…")
(or another appropriate localization key) so the user-facing label in the
Button("Chromium (CEF)…") call is localized; ensure you apply this change where
the Button(...) for the Debug menu is created.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29cb75a40c
ℹ️ 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".
| A5001271 /* PostHog */, | ||
| A5001261 /* Bonsplit */, | ||
| A5001291 /* MarkdownUI */, | ||
| CEF00101 /* CEFWebView */, |
There was a problem hiding this comment.
Adding CEFWebView as an unconditional GhosttyTabs package dependency makes every cmux/cmux-unit build compile and link the CEF wrapper, which requires binaries under vendor/CEFWebView/Frameworks. Those artifacts are not in the repo (the directory is generated by scripts/setup-cefwebview.sh), and existing setup/CI flows build the app directly, so fresh checkouts will fail before tests can run. This dependency should be gated to the debug-only path or bootstrap CEF artifacts as part of standard setup/build workflows.
Useful? React with 👍 / 👎.
| echo "🔨 Building libcef_dll_wrapper..." | ||
| ( | ||
| cd "$CEF_ROOT/$CEF_DIST" | ||
| cmake -G Xcode -DPROJECT_ARCH=arm64 -DCMAKE_OSX_DEPLOYMENT_TARGET=13.0 . >/dev/null |
There was a problem hiding this comment.
The setup script hardcodes arm64 when building libcef_dll_wrapper, so generated CEF artifacts are single-arch. The release workflow builds a universal app (ARCHS="arm64 x86_64"), and the x86_64 link step will fail against arm64-only CEF libraries/frameworks. The script needs to produce universal (or per-arch) CEF outputs that match the app build architectures.
Useful? React with 👍 / 👎.
| void OnLoadingStateChange(CefRefPtr<CefBrowser> browser, | ||
| bool isLoading, bool canGoBack, bool canGoForward) override { | ||
| _isLoading = isLoading; | ||
| _canGoBack = canGoBack; | ||
| _canGoForward = canGoForward; |
There was a problem hiding this comment.
OnLoadingStateChange updates only internal C++ fields, but nothing here notifies CEFWebViewState; CEFBrowserHost.updateState() is currently called only on explicit commands (loadURL, goBack, goForward). That means normal in-page navigation/redirect activity can leave SwiftUI controls stale (loading indicator, back/forward enabled state, and related metadata). Bridge these callback updates back to Swift state so UI reflects live browser state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
3 issues 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="scripts/embed-cefwebview.sh">
<violation number="1" location="scripts/embed-cefwebview.sh:33">
P2: The helper build output path is hardcoded to `arm64`, so the script fails on other macOS architectures even when `swift build` succeeds.</violation>
</file>
<file name="GhosttyTabs.xcodeproj/project.pbxproj">
<violation number="1" location="GhosttyTabs.xcodeproj/project.pbxproj:667">
P1: CEFWebView is added as a package dependency but not linked in the target’s Frameworks build phase.</violation>
</file>
<file name="Sources/CefDebugWindow.swift">
<violation number="1" location="Sources/CefDebugWindow.swift:40">
P2: `urlText` is only written by user input; after navigation, redirects, or back/forward, `state.currentURL` updates but the address bar keeps showing whatever was last typed. Add `.onChange(of: state.currentURL)` to mirror the actual URL into `urlText` (skip updates while the field is focused to avoid clobbering in-progress typing).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| A5001271 /* PostHog */, | ||
| A5001261 /* Bonsplit */, | ||
| A5001291 /* MarkdownUI */, | ||
| CEF00101 /* CEFWebView */, |
There was a problem hiding this comment.
P1: CEFWebView is added as a package dependency but not linked in the target’s Frameworks build phase.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At GhosttyTabs.xcodeproj/project.pbxproj, line 667:
<comment>CEFWebView is added as a package dependency but not linked in the target’s Frameworks build phase.</comment>
<file context>
@@ -661,6 +664,7 @@
A5001271 /* PostHog */,
A5001261 /* Bonsplit */,
A5001291 /* MarkdownUI */,
+ CEF00101 /* CEFWebView */,
);
name = GhosttyTabs;
</file context>
| echo "==> Building CEFHelper + CEFHelperRenderer (release)" | ||
| swift build --package-path "$PKG_ROOT" -c release --product CEFHelper >/dev/null | ||
| swift build --package-path "$PKG_ROOT" -c release --product CEFHelperRenderer >/dev/null | ||
| HELPER_BUILD_DIR="$PKG_ROOT/.build/arm64-apple-macosx/release" |
There was a problem hiding this comment.
P2: The helper build output path is hardcoded to arm64, so the script fails on other macOS architectures even when swift build succeeds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/embed-cefwebview.sh, line 33:
<comment>The helper build output path is hardcoded to `arm64`, so the script fails on other macOS architectures even when `swift build` succeeds.</comment>
<file context>
@@ -0,0 +1,153 @@
+echo "==> Building CEFHelper + CEFHelperRenderer (release)"
+swift build --package-path "$PKG_ROOT" -c release --product CEFHelper >/dev/null
+swift build --package-path "$PKG_ROOT" -c release --product CEFHelperRenderer >/dev/null
+HELPER_BUILD_DIR="$PKG_ROOT/.build/arm64-apple-macosx/release"
+HELPER_BIN="$HELPER_BUILD_DIR/CEFHelper"
+RENDERER_BIN="$HELPER_BUILD_DIR/CEFHelperRenderer"
</file context>
|
|
||
| private struct CefDebugView: View { | ||
| @State private var url: URL? = URL(string: "https://www.google.com") | ||
| @State private var urlText: String = "https://www.google.com" |
There was a problem hiding this comment.
P2: urlText is only written by user input; after navigation, redirects, or back/forward, state.currentURL updates but the address bar keeps showing whatever was last typed. Add .onChange(of: state.currentURL) to mirror the actual URL into urlText (skip updates while the field is focused to avoid clobbering in-progress typing).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/CefDebugWindow.swift, line 40:
<comment>`urlText` is only written by user input; after navigation, redirects, or back/forward, `state.currentURL` updates but the address bar keeps showing whatever was last typed. Add `.onChange(of: state.currentURL)` to mirror the actual URL into `urlText` (skip updates while the field is focused to avoid clobbering in-progress typing).</comment>
<file context>
@@ -0,0 +1,128 @@
+
+private struct CefDebugView: View {
+ @State private var url: URL? = URL(string: "https://www.google.com")
+ @State private var urlText: String = "https://www.google.com"
+ @State private var state = CEFWebViewState()
+
</file context>
|
While reviewing recent paste-related changes (#2779 / #2904) I noticed three small artefacts that the CEF migration may inherit. Filing here rather than as separate issues because they all live on the WKWebView path and may be moot once CEF lands.
Happy to file these as separate issues if any of them is worth tracking independently. |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
4 participants
Summary
Phase 1 of replacing
WKWebViewinSources/Panels/BrowserPanel.swiftwith a Chromium-backed engine via brennanMKE/CEFWebView.This PR only adds the vendored package and the build script that produces its CEF binaries. The cmux app is unchanged. No Xcode target wiring, no embedded framework, no helper apps yet, no Swift code uses
import CEFWebView. ExistingWKWebViewpath is unaffected.vendor/CEFWebView/— vendored package (no.git, no demoWebView/app, noTests/).Package.swiftlowered to.macOS(.v14)to match cmux'sMACOSX_DEPLOYMENT_TARGET = 14.0.testTargetremoved (the upstreamTests/dir isn't vendored).build_cpp.shpatched withfind -Lso a symlinked CEF distribution is followed.scripts/setup-cefwebview.sh— idempotent. Downloads or links a pinned CEF binary distribution intovendor/CEFWebView/CEF/, buildslibcef_dll_wrapper.a, and producesvendor/CEFWebView/Frameworks/. Reuses a siblingcef-swift-mvp/third_party/cef/checkout when present so we don't re-download hundreds of MB..gitignoreexcludesvendor/CEFWebView/CEF/,Frameworks/, and.build/.vendor/CEFWebView/INTEGRATION_NOTES.mddocuments what's still needed to actually load Chromium inside cmux.Why this is split out
Past attempts (
task-cef-alloy,task-owl-chromium,task-raw-chromium) all crashed and never landed onmain. Crash classes were CEF Alloy view-teardown on close, OSR right-click crashes, and uncontrollable Chrome-runtime windows. Switching to CEFWebView's package layout (proper multi-process renderer/GPU helpers viaCEFHelper+CEFHelper (Renderer)) is intended to address those. Rather than redo all of it in one giant PR, this lands the dependency vendoring first so subsequent PRs can focus on the actual Xcode integration and the Swift parity work.Phase 2 (not in this PR)
Tracked in
vendor/CEFWebView/INTEGRATION_NOTES.md:vendor/CEFWebViewas anXCLocalSwiftPackageReferenceon thecmuxtarget inGhosttyTabs.xcodeproj.FRAMEWORK_SEARCH_PATHS/LIBRARY_SEARCH_PATHSto$(SRCROOT)/vendor/CEFWebView/Frameworks.Chromium Embedded Framework.frameworkplus a build script for thecmux Helper.app/cmux Helper (Renderer).appbundles.vendor/CEFWebView/fix_cef_framework.shas a post-embed script (Xcode flattens the framework's symlinks during embedding).Sources/BrowserEngine/with a CEF-backed alternative toBrowserPanel'sWKWebView, gated behind a Debug menu toggle until parity reaches the high-risk items: WebAuthn bridge, OAuthwindow.openerpreservation, per-profile cookie isolation, SSO/MDM auth challenge passthrough, and the fourWKUserScriptinjections (telemetry hooks, address-bar focus tracking, paste-as-plain-text tracking, WebAuthn).Testing
cd vendor/CEFWebView && swift buildsucceeds (Build complete!) afterscripts/setup-cefwebview.sh.Related
task-cef-alloy,task-owl-chromium,task-raw-chromiumSummary by cubic
Vendors and wires a Chromium-backed engine via
CEFWebView, adds setup/embed scripts, and exposes a Debug-only “Chromium (CEF)…” window.WKWebViewremains the default; BrowserPanel is unchanged.New Features
vendor/CEFWebViewinto Xcode as a local package and linked its products; set framework/library search paths.CefDebugWindow) with address bar, back/forward, and reload.scripts/embed-cefwebview.shto embedChromium Embedded Framework.frameworkand build/signCEFHelperandCEFHelperRendererhelper apps inside the app bundle.Dependencies
vendor/CEFWebView/(no demo app, no tests).Package.swifttargets.macOS(.v14); patchedbuild_cpp.shto usefind -L.scripts/setup-cefwebview.shto download/link a pinned CEF distribution, buildlibcef_dll_wrapper.a, and populatevendor/CEFWebView/Frameworks/(reuses a siblingcef-swift-mvp/third_party/cef/when available)..gitignoreto excludevendor/CEFWebView/CEF/,Frameworks/, and.build/.Written for commit 29cb75a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Chores