Repository navigation
Trim release bundle size - #7589
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds deflate-based compression and serving for markdown and diff viewer JavaScript assets, including asset resolution, staging, decompression, and HTTP headers. It also adds release bundle Mach-O stripping before codesigning, with CI, nightly, release, shell, and Swift test coverage. ChangesDeflated Asset Compression and Serving
Release Bundle Symbol Stripping
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DiffViewerServer as diff-viewer-server
participant AssetStore as Staged .deflate Assets
Client->>DiffViewerServer: GET /assets/mod.mjs
DiffViewerServer->>AssetStore: Resolve mod.mjs to mod.mjs.deflate
AssetStore-->>DiffViewerServer: Return compressed bytes
DiffViewerServer-->>Client: 200 response with Content-Encoding: deflate
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 passed)
✨ 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 |
Greptile SummaryThis PR trims the macOS release bundle by compressing viewer assets and stripping bundled binaries. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "Use failable deflated fixture decoding" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let rawURL = sourceDirectory.appendingPathComponent(relativePath, isDirectory: false) | ||
| if FileManager.default.fileExists(atPath: rawURL.path) { | ||
| return rawURL | ||
| } | ||
| let deflatedURL = sourceDirectory.appendingPathComponent(relativePath + ".deflate", isDirectory: false) | ||
| if FileManager.default.fileExists(atPath: deflatedURL.path) { | ||
| return deflatedURL | ||
| } |
There was a problem hiding this comment.
When an incremental bundle contains both foo.mjs and foo.mjs.deflate, the enumeration collapses them to one logical path and this resolver returns the raw file first. The copied asset then serves stale or uncompressed bytes for the same request path instead of the compressed artifact the release bundle expects.
| let rawURL = sourceDirectory.appendingPathComponent(relativePath, isDirectory: false) | |
| if FileManager.default.fileExists(atPath: rawURL.path) { | |
| return rawURL | |
| } | |
| let deflatedURL = sourceDirectory.appendingPathComponent(relativePath + ".deflate", isDirectory: false) | |
| if FileManager.default.fileExists(atPath: deflatedURL.path) { | |
| return deflatedURL | |
| } | |
| let rawURL = sourceDirectory.appendingPathComponent(relativePath, isDirectory: false) | |
| let deflatedURL = sourceDirectory.appendingPathComponent(relativePath + ".deflate", isDirectory: false) | |
| let hasRaw = FileManager.default.fileExists(atPath: rawURL.path) | |
| let hasDeflated = FileManager.default.fileExists(atPath: deflatedURL.path) | |
| if hasRaw && hasDeflated { | |
| throw CLIError(message: "Bundled diff viewer asset has both raw and deflated variants: \(relativePath)") | |
| } | |
| if hasDeflated { | |
| return deflatedURL | |
| } | |
| if hasRaw { | |
| return rawURL | |
| } |
| let rawURL = sourceDirectory.appendingPathComponent(relativePath, isDirectory: false) | ||
| if FileManager.default.fileExists(atPath: rawURL.path) { | ||
| return rawURL | ||
| } | ||
| let deflatedURL = sourceDirectory.appendingPathComponent(relativePath + ".deflate", isDirectory: false) | ||
| if FileManager.default.fileExists(atPath: deflatedURL.path) { | ||
| return deflatedURL | ||
| } |
There was a problem hiding this comment.
foo.mjs and foo.mjs.deflate, the logical path is collapsed to foo.mjs, but this resolver still returns the raw file first. That can copy, hash, and serve stale or uncompressed bytes for the same module URL even though the compressed artifact is present. Prefer the .deflate file when both variants exist so the logical asset path resolves to the release artifact.
| let rawURL = sourceDirectory.appendingPathComponent(relativePath, isDirectory: false) | |
| if FileManager.default.fileExists(atPath: rawURL.path) { | |
| return rawURL | |
| } | |
| let deflatedURL = sourceDirectory.appendingPathComponent(relativePath + ".deflate", isDirectory: false) | |
| if FileManager.default.fileExists(atPath: deflatedURL.path) { | |
| return deflatedURL | |
| } | |
| let deflatedURL = sourceDirectory.appendingPathComponent(relativePath + ".deflate", isDirectory: false) | |
| if FileManager.default.fileExists(atPath: deflatedURL.path) { | |
| return deflatedURL | |
| } | |
| let rawURL = sourceDirectory.appendingPathComponent(relativePath, isDirectory: false) | |
| if FileManager.default.fileExists(atPath: rawURL.path) { | |
| return rawURL | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmuxTests/BrowserPanelTests.swift (1)
793-800: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the redundant
evaluateJavaScriptassertion.The
moduleLoadedmessage-handler assertion at line 792 already validates the full module/worker/WASM execution result ("module-ok:js-ok:wasm-ok"). The follow-upevaluateJavaScript("document.body.dataset.loaded || ''")block at lines 793-799 re-checks the same value through a second round-trip, adding test latency without increasing coverage. If this was intended to be removed per the change plan, it should be; if intentionally kept as a belt-and-suspenders check, consider adding a brief comment explaining why both assertions are needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/BrowserPanelTests.swift` around lines 793 - 800, Remove the redundant evaluateJavaScript assertion and its evaluated expectation from the test, keeping the existing moduleLoaded message-handler assertion as the sole validation of the full module/worker/WASM result.CLI/cmux_open.swift (1)
7602-7644: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider content-keying the main "pierre-diffs" asset directory like the app asset directory already is.
The app asset directory is content-hashed (
diffViewerAppAssetContentKey, lines 7658-7683) specifically so different webview bundles never clobber each other in the shared per-uid/tmp/cmux-diff-viewer-<uid>cache. The main asset directory name ("pierre-diffs-1.2.7-trees-1.0.0-beta.4", line 7604) is still a fixed literal, not content-keyed. Now that assets can transition between raw and.deflaterepresentations under the same pinned version string (this PR's whole premise), a future release that recompresses the same vendor bundle version without bumping that string can leave a stale raw variant sitting alongside the new.deflatevariant in the same shared directory indefinitely (pruneDiffViewerFilesonly sweeps.html/.patch/manifest/session/lock/cache files, neverassets/subdirectories). This doesn't break correctness today (the manifest is built from fresh copy results, not a directory rescan), but it's an unbounded, never-cleaned disk-growth path across upgrades.Reusing the existing
diffViewerAppAssetContentKey-style hashing for the main asset directory name would close this gap with the same pattern already proven for the app assets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/cmux_open.swift` around lines 7602 - 7644, The main diff viewer asset directory in ensureDiffViewerAssets should be content-keyed rather than using the fixed assetDirectoryName literal. Reuse the existing diffViewerAppAssetContentKey-style hashing for the bundled main asset source, use the resulting name consistently for targetDirectory and all returned module URLs, and preserve the existing asset-copy and validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/DeflatedAssetTestSupport.swift`:
- Around line 10-14: Update DeflatedAssetTestSupport.loadText to construct the
string with failable String(bytes:encoding:) using the decompressed data, and
propagate a decoding failure through the existing throws behavior instead of
silently replacing invalid UTF-8.
---
Outside diff comments:
In `@CLI/cmux_open.swift`:
- Around line 7602-7644: The main diff viewer asset directory in
ensureDiffViewerAssets should be content-keyed rather than using the fixed
assetDirectoryName literal. Reuse the existing
diffViewerAppAssetContentKey-style hashing for the bundled main asset source,
use the resulting name consistently for targetDirectory and all returned module
URLs, and preserve the existing asset-copy and validation behavior.
In `@cmuxTests/BrowserPanelTests.swift`:
- Around line 793-800: Remove the redundant evaluateJavaScript assertion and its
evaluated expectation from the test, keeping the existing moduleLoaded
message-handler assertion as the sole validation of the full module/worker/WASM
result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6205722-5261-4c79-8f87-a4848c7c0a20
📒 Files selected for processing (7)
CLI/CMUXCLI+DiffViewerBundledAssets.swiftCLI/cmux_open.swiftSources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserPanelTests.swiftcmuxTests/CMUXOpenCommandTests.swiftcmuxTests/DeflatedAssetTestSupport.swift
| static func loadText(path: String) throws -> String { | ||
| let data = try Data(contentsOf: URL(fileURLWithPath: path)) | ||
| let decompressed = try (data as NSData).decompressed(using: .zlib) as Data | ||
| return String(decoding: decompressed, as: UTF8.self) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Prefer failable String(bytes:encoding:) over String(decoding:as:).
String(decoding:as:) never fails; invalid UTF-8 silently becomes replacement characters instead of surfacing a decode error, which could mask a real decompression/corruption bug in the deflated fixture.
♻️ Proposed fix
- return String(decoding: decompressed, as: UTF8.self)
+ guard let text = String(bytes: decompressed, encoding: .utf8) else {
+ throw CocoaError(.fileReadCorruptFile)
+ }
+ return text📝 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.
| static func loadText(path: String) throws -> String { | |
| let data = try Data(contentsOf: URL(fileURLWithPath: path)) | |
| let decompressed = try (data as NSData).decompressed(using: .zlib) as Data | |
| return String(decoding: decompressed, as: UTF8.self) | |
| } | |
| static func loadText(path: String) throws -> String { | |
| let data = try Data(contentsOf: URL(fileURLWithPath: path)) | |
| let decompressed = try (data as NSData).decompressed(using: .zlib) as Data | |
| guard let text = String(bytes: decompressed, encoding: .utf8) else { | |
| throw CocoaError(.fileReadCorruptFile) | |
| } | |
| return text | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 10-10: A file is read from a path built from runtime/request input via FileManager.contents(atPath:), Data(contentsOf:), or String(contentsOfFile:). An attacker can supply '../' sequences or absolute paths to read files outside the intended directory (path traversal). Validate and canonicalize the path, reject '..' components, and confine reads to an allow-listed base directory (e.g. resolve with URL(fileURLWithPath:relativeTo:) and verify the resolved path is still inside the base) before reading.
Context: Data(contentsOf: URL(fileURLWithPath: path))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(path-traversal-file-read-request-input-swift)
🪛 SwiftLint (0.65.0)
[Warning] 13-13: Prefer failable String(bytes:encoding:) initializer when converting Data to String
(optional_data_string_conversion)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/DeflatedAssetTestSupport.swift` around lines 10 - 14, Update
DeflatedAssetTestSupport.loadText to construct the string with failable
String(bytes:encoding:) using the decompressed data, and propagate a decoding
failure through the existing throws behavior instead of silently replacing
invalid UTF-8.
Source: Linters/SAST tools
# Conflicts: # cmux.xcodeproj/project.pbxproj
This branch was successfully deployed
1 participant
Summary
.deflatediff-viewer assets through the CLI HTTP server and WKURLSchemeHandler withContent-Encoding: deflateSize impact
.deflatefiles underContents/Resources/markdown-viewerscripts/strip-release-bundle.shbefore signing, coveringContents/MacOS/cmux,Contents/Resources/bin/cmux, cmux plugins, andlibcmux_*.dylibVerification
git diff --check./tests/test_compress_markdown_viewer_assets.sh./tests/test_strip_release_bundle.sh./scripts/reload-cloud.sh --tag sztrim(Blacksmith run https://github.com/manaflow-ai/cmux/actions/runs/28918574932)diff-viewer-serversmoke serveddiffs.mjs.deflateas/assets/diffs.mjswithContent-Encoding: deflate;curl --compresseddecoded 479312 bytesLocal Swift test execution was blocked by the cmuxterm-hq guard against local
xcodebuild test; PR CI should run the focused XCTest coverage added here.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches release signing prep and diff/markdown asset HTTP delivery; regressions could break the diff viewer or signed release binaries, though CI guards and smoke tests mitigate this.
Overview
Shrinks shipped macOS bundles by deflating bundled markdown/diff viewer JS and stripping cmux-owned Mach-O binaries before release signing.
Compressed web assets: The markdown-viewer compress script now walks all nested
.js/.mjsfiles (not only top-level), writes sibling*.deflateblobs, and deletes the raw sources. Runtime paths stay the same: the CLI diff-viewer HTTP server,BrowserPanelcustom URL scheme, and bundled asset copy/hash logic prefer.deflateon disk, map URLs without the suffix, and setContent-Encoding: deflate. In-app markdown loading uses explicit zlib inflate for deflated text assets. Diff-viewer asset helpers move intoCMUXCLI+DiffViewerBundledAssets.swift.Release pipeline: New
scripts/strip-release-bundle.shruns in nightly and stable release workflows immediately before codesign, stripping local symbols from the app binary, bundled CLI, cmux plug-ins, andlibcmux_*.dylib(not third-party frameworks). CI adds guard tests for compression and stripping behavior.Reviewed by Cursor Bugbot for commit cbabd7b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Compresses all markdown/diff viewer JS modules and strips macOS binaries to shrink the app bundle, saving ~27.91 MiB before DMG compression. Prefers
.deflateassets end-to-end while keeping URLs stable and serving them with the correctContent-Encodingin both the CLI server and the webview.New Features
.deflatefor allmarkdown-viewer.js/.mjs(skip existing.deflate); serve withContent-Encoding: deflatewhile keeping original URLs.scripts/strip-release-bundle.shin nightly/release to strip cmux-owned Mach-O binaries before codesigning; CI runs guard tests for compression and stripping.Bug Fixes
.deflate, dedupe raw vs deflated, and keep the on-disk suffix without changing request paths..deflatefiles aren’t rewritten.Written for commit cbabd7b. Summary will update on new commits.
Summary by CodeRabbit