Repository navigation
Extract CmuxFoundation package (modular refactor, wave 1) - #5055
Conversation
First leaf of the modular refactor. New CmuxFoundation SwiftPM package (Swift 6 strict, no dependencies) for shared low-level utilities, seeded with cmuxJavaScriptStringLiteral moved out of AppDelegate. The app target depends on it via a local package reference and imports it. Behavior is unchanged; the package has unit tests via Swift Testing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extracts a JavaScript string literal encoding utility into a new local Swift package ChangesCmuxFoundation Package Extraction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (16 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 |
Lists the package under the workspace Packages group so it shows in the Xcode navigator alongside the other local packages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR extracts
Confidence Score: 3/5The call-site migration introduces a double-optional that renders Optional(...) inside the JavaScript template on every non-nil input, causing a ReferenceError in the web view at runtime. The extraction itself is structurally sound, but the migrated call site in AppDelegate and the canonical examples in the doc-comment and README all use the same double-optional pattern. Because javaScriptStringLiteral returns String?, optional chaining on String? produces String??, and ?? 'null' leaves the result as String? — so string interpolation in the JS script body produces Optional('...') rather than the bare literal, breaking evaluateJavaScript calls whenever awaitingInputId is non-nil. Sources/AppDelegate.swift (the live call site), Packages/CmuxFoundation/Sources/CmuxFoundation/String+JavaScriptStringLiteral.swift and README.md (doc-comment and usage examples that propagate the pattern). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
App["cmux App Target\n(AppDelegate.swift)"]
CF["CmuxFoundation\n(Packages/CmuxFoundation)"]
Str["String+JavaScriptStringLiteral\n.javaScriptStringLiteral returns String?"]
JS["WKWebView\nevaluateJavaScript(script)"]
App -->|"import CmuxFoundation"| CF
CF --> Str
App -->|"awaitingInputId.flatMap -> String?"| Str
App -->|"interpolates into JS template"| JS
style App fill:#f5f5f5,stroke:#333
style CF fill:#dbeafe,stroke:#2563eb
style Str fill:#dbeafe,stroke:#2563eb
style JS fill:#fef9c3,stroke:#ca8a04
Reviews (3): Last reviewed commit: "Update AppDelegate call site to String.j..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard let data = try? JSONSerialization.data(withJSONObject: [value]), | ||
| let arrayLiteral = String(data: data, encoding: .utf8), | ||
| arrayLiteral.count >= 2 else { | ||
| return nil | ||
| } | ||
| return String(arrayLiteral.dropFirst().dropLast()) |
There was a problem hiding this comment.
JSONSerialization does not escape U+2028 (LINE SEPARATOR) or U+2029 (PARAGRAPH SEPARATOR) by default. Both characters are legal in JSON strings, but in ECMAScript ≤ 2018 they are line terminators that immediately end a string literal, producing a syntax error. A WKWebView backed by JavaScriptCore or V8 on older hosts evaluating setValue("\u2028") would throw. The doc-comment says the result is "ready to splice into JS source", but that claim is not always true. Either add a post-processing step that replaces \u2028/\u2029 with their \u escape sequences, or narrow the doc-comment to document the limitation and add a test that pins the current (unescaped) behavior.
| .testTarget( | ||
| name: "CmuxFoundationTests", | ||
| dependencies: ["CmuxFoundation"] | ||
| ), |
There was a problem hiding this comment.
swiftSettings as the library target. With swift-tools-version: 6.0, Swift 6 language mode is the default for all targets, so StrictConcurrency is already active — but ExistentialAny and InternalImportsByDefault are opt-in upcoming features and will not apply in the test target. Future tests that use bare existentials (Protocol instead of any Protocol) or rely on import-visibility semantics will silently diverge from production behavior. Adding the same settings block keeps the compilation environment uniform.
| .testTarget( | |
| name: "CmuxFoundationTests", | |
| dependencies: ["CmuxFoundation"] | |
| ), | |
| .testTarget( | |
| name: "CmuxFoundationTests", | |
| dependencies: ["CmuxFoundation"], | |
| swiftSettings: [ | |
| .swiftLanguageMode(.v6), | |
| .enableUpcomingFeature("ExistentialAny"), | |
| .enableUpcomingFeature("InternalImportsByDefault"), | |
| ] | |
| ), |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Foundation helpers should be ergonomic extensions on existing types, not bare top-level functions. Replace the public func cmuxJavaScriptStringLiteral with String.javaScriptStringLiteral and update the call site. Behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Completes the extension reshape: the call site now uses awaitingInputId?.javaScriptStringLiteral instead of the removed free function. (The previous commit moved the helper but a failed edit left this call site referencing the deleted symbol.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| completion: @escaping ([String: String]) -> Void | ||
| ) { | ||
| let expectedInputIdLiteral = cmuxJavaScriptStringLiteral(awaitingInputId) ?? "null" | ||
| let expectedInputIdLiteral = awaitingInputId?.javaScriptStringLiteral ?? "null" |
There was a problem hiding this comment.
The PR deletes cmuxJavaScriptStringLiteral(_:) from AppDelegate.swift and replaces it with the String.javaScriptStringLiteral property on CmuxFoundation, but this call site was never migrated. Because awaitingInputId is String?, the correct replacement is awaitingInputId?.javaScriptStringLiteral ?? "null". As written, the build will fail with "use of unresolved identifier 'cmuxJavaScriptStringLiteral'".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| completion: @escaping ([String: String]) -> Void | ||
| ) { | ||
| let expectedInputIdLiteral = cmuxJavaScriptStringLiteral(awaitingInputId) ?? "null" | ||
| let expectedInputIdLiteral = awaitingInputId?.javaScriptStringLiteral ?? "null" |
There was a problem hiding this comment.
String?, not String, so \(expectedInputIdLiteral) in the template prints Optional("\"someId\"") rather than the bare literal. The JS engine receives const expectedInputId = Optional("\"someId\""); and throws a ReferenceError on every call where awaitingInputId is non-nil. Use flatMap to flatten the chain and get a plain String.
| let expectedInputIdLiteral = awaitingInputId?.javaScriptStringLiteral ?? "null" | |
| let expectedInputIdLiteral = awaitingInputId.flatMap(\.javaScriptStringLiteral) ?? "null" |
| /// ```swift | ||
| /// "a\"b".javaScriptStringLiteral // -> "\"a\\\"b\"" | ||
| /// webView.evaluateJavaScript("setValue(\(id?.javaScriptStringLiteral ?? "null"))") | ||
| /// ``` |
There was a problem hiding this comment.
id?.javaScriptStringLiteral ?? "null" where id: String?. Because javaScriptStringLiteral itself returns String?, optional chaining produces String??, and ?? unwraps only the outer level — the result is String?, not String. When that String? is interpolated into a JS template it renders as Optional(...), not the bare literal. The canonical call pattern should use flatMap to flatten the nested optional.
| /// ```swift | |
| /// "a\"b".javaScriptStringLiteral // -> "\"a\\\"b\"" | |
| /// webView.evaluateJavaScript("setValue(\(id?.javaScriptStringLiteral ?? "null"))") | |
| /// ``` | |
| /// ```swift | |
| /// "a\"b".javaScriptStringLiteral // -> "\"a\\\"b\"" | |
| /// webView.evaluateJavaScript("setValue(\(id.flatMap(\.javaScriptStringLiteral) ?? "null"))") | |
| /// ``` |
- Add Send Ctrl-F to Terminal passthrough (manaflow-ai#5011, force-stop CC agents) - Fix sidebar worktree spawn worktree-setup-as-input bug (manaflow-ai#5032) - Add boundary-aware ranking layer for command-palette fuzzy search - Open extension browser as pane tab + polish (manaflow-ai#5053) - Move sidebar kind selection to titlebar menu, fix clipped tooltip and floor sidebar width (manaflow-ai#5045) - Extract CmuxFoundation package — modular refactor wave 1 (manaflow-ai#5055) - Center empty sidebar-extension state, fade host bottom edge (manaflow-ai#5057) - Align titlebar accessory hints (manaflow-ai#5059) - Restore sidebar minimum width (manaflow-ai#5062) Conflicts resolved: - cmux.xcodeproj/project.pbxproj: merge fork's CMUXSettingsCore + CMUXSessionDaemon package refs with upstream's new CmuxFoundation package reference and product dependency.
This branch was successfully deployed
1 participant
First leaf of the modular refactor. Introduces `CmuxFoundation`, a dependency-free SwiftPM package for shared low-level utilities, and moves `cmuxJavaScriptStringLiteral` into it. The app target depends on it via a local package reference and imports it. Behavior is unchanged.
This proves the extraction pattern end to end on the lowest-risk surface: a new package with a Swift 6 strict manifest, Swift Testing unit tests, Xcode local-package wiring, and the app target forwarding to the package. Later waves follow the same shape.
Changes:
No user-facing change.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Extracted
CmuxFoundation, a dependency-free SwiftPM package for shared low-level utilities, and moved the JS string literal helper into it asString.javaScriptStringLiteral. The app now imports the package; behavior is unchanged.Packages/CmuxFoundation(Swift 6; no deps;ExistentialAny,InternalImportsByDefault).cmuxJavaScriptStringLiteral(_:)withString.javaScriptStringLiteral; fixed theAppDelegatecall site.Written for commit c22f751. Summary will update on new commits.
Summary by CodeRabbit