Repository navigation
Fix double-frame app icon in DMG installer - #1257
lawrencecchen wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR removes the AppIcon.icon asset and its Xcode project references, and changes CI/build packaging to use Homebrew-installed create-dmg with an explicit DMG creation flow (custom background, layout, codesigning) and updated notarization/stapling handling. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Script as build-sign-upload.sh
participant Codesign as Codesign Tool
participant CreateDMG as create-dmg (Homebrew)
participant Notary as Apple Notary Service
participant Stapler as stapler
participant Release as Release Storage
GH->>Script: run packaging job (nightly/release)
Script->>Codesign: codesign the .app
Codesign-->>Script: signed .app
Script->>CreateDMG: create-dmg with background, layout, app-drop link, codesign identity
CreateDMG-->>Script: generated .dmg
Script->>Notary: submit .dmg for notarization
Notary-->>Script: notarization status (approved)
Script->>Stapler: staple notarization ticket to .dmg
Stapler-->>Script: stapled .dmg validated
Script->>Release: upload final .dmg
Release-->>GH: artifact available
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR removes the
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Xcode Build] --> B{Icon Source?}
B -->|Before PR: AppIcon.icon\nXcode 16 Icon Composer| C[macOS dynamically adds\nbackground plate]
C --> D[Double-frame squircle\nvisible in DMG/Finder]
B -->|After PR: AppIcon.appiconset\nPre-rendered PNGs| E[Icon rendered as-is\nno dynamic plate added]
E --> F[Clean icon in DMG,\nDock unchanged]
Last reviewed commit: dabf964 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 96-99: The "Install build deps" step currently runs an unpinned
brew install create-dmg which allows version drift; update that step in both
release and nightly workflows to install a specific create-dmg version (e.g.,
use a pinned formula like create-dmg@<VERSION> or install from a fixed Homebrew
tap/formula URL) so builds are reproducible, and ensure any related cache or
verification steps are updated to expect the pinned version; modify the step
named "Install build deps" in both workflows accordingly.
In `@tests/test_ci_create_dmg_pinned.sh`:
- Around line 20-23: The current grep only detects "npm install ... create-dmg"
and misses other npm-based invocations; update the grep used on the variable
workflow (the grep -Eq call) to use a broader regex that matches any npm-based
invocation like "npx create-dmg", "npm exec create-dmg", "npm run ...
create-dmg" as well as "npm install ... create-dmg" so the test fails if any npm
runner is used instead of the Homebrew create-dmg; keep the check logic and
error message the same but replace the pattern to include those additional
command forms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b03ebae-9f35-445c-9511-39044ef8f61a
⛔ Files ignored due to path filters (1)
scripts/dmg-background.pngis excluded by!**/*.png
📒 Files selected for processing (4)
.github/workflows/nightly.yml.github/workflows/release.ymlscripts/build-sign-upload.shtests/test_ci_create_dmg_pinned.sh
| - name: Install build deps | ||
| if: steps.guard_release_assets.outputs.skip_all != 'true' | ||
| run: | | ||
| npm install --global "create-dmg@${CREATE_DMG_VERSION}" | ||
| brew install create-dmg |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "create-dmg install commands:"
rg -n --glob '.github/workflows/*.yml' '\bbrew install create-dmg\b'
echo
echo "create-dmg invocations:"
rg -n --glob '.github/workflows/*.yml' --glob 'scripts/*.sh' '\bcreate-dmg\b'
echo
echo "Pinning / reproducibility safeguards:"
rg -n 'brew pin|create-dmg@|brew extract|HOMEBREW_NO_AUTO_UPDATE|Brewfile' .github/workflows scripts tests || trueRepository: manaflow-ai/cmux
Length of output: 254
🏁 Script executed:
git ls-files | head -20Repository: manaflow-ai/cmux
Length of output: 675
🏁 Script executed:
find . -name "*.yml" -o -name "*.yaml" | head -20Repository: manaflow-ai/cmux
Length of output: 957
🏁 Script executed:
ls -la .github/ 2>/dev/null || echo ".github directory not found"Repository: manaflow-ai/cmux
Length of output: 372
🏁 Script executed:
cat .github/workflows/release.yml | grep -n "create-dmg" -C 2Repository: manaflow-ai/cmux
Length of output: 527
🏁 Script executed:
cat .github/workflows/nightly.yml | grep -n "create-dmg" -C 2Repository: manaflow-ai/cmux
Length of output: 431
🏁 Script executed:
grep -r "brew pin\|create-dmg@\|brew extract\|HOMEBREW_NO_AUTO_UPDATE\|Brewfile" .github/workflows scripts tests 2>/dev/null || echo "No pinning safeguards found"Repository: manaflow-ai/cmux
Length of output: 87
Pin create-dmg in release and nightly workflows to ensure reproducible signing.
Both release.yml (line 99) and nightly.yml (line 129) use unpinned brew install create-dmg. This allows the installed tool version to drift between runs, potentially altering signed/notarized release artifacts without any repository change. No pinning safeguards exist in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 96 - 99, The "Install build deps"
step currently runs an unpinned brew install create-dmg which allows version
drift; update that step in both release and nightly workflows to install a
specific create-dmg version (e.g., use a pinned formula like
create-dmg@<VERSION> or install from a fixed Homebrew tap/formula URL) so builds
are reproducible, and ensure any related cache or verification steps are updated
to expect the pinned version; modify the step named "Install build deps" in both
workflows accordingly.
| if grep -Eq 'npm install.*create-dmg' "$workflow"; then | ||
| echo "FAIL: $workflow should not use npm create-dmg (use Homebrew version)" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Broaden the npm regression check.
This only catches npm install ... create-dmg. A future switch to npx create-dmg or npm exec create-dmg would still pass this test while reintroducing the same npm-based toolchain.
Suggested fix
- if grep -Eq 'npm install.*create-dmg' "$workflow"; then
+ if grep -Eq '\b(npm (install|i|exec)|npx)\b.*\bcreate-dmg\b' "$workflow"; then
echo "FAIL: $workflow should not use npm create-dmg (use Homebrew version)"
exit 1
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_ci_create_dmg_pinned.sh` around lines 20 - 23, The current grep
only detects "npm install ... create-dmg" and misses other npm-based
invocations; update the grep used on the variable workflow (the grep -Eq call)
to use a broader regex that matches any npm-based invocation like "npx
create-dmg", "npm exec create-dmg", "npm run ... create-dmg" as well as "npm
install ... create-dmg" so the test fails if any npm runner is used instead of
the Homebrew create-dmg; keep the check logic and error message the same but
replace the pattern to include those additional command forms.
The Xcode 16 Icon Composer format (AppIcon.icon) causes macOS to dynamically render a background plate with depth effects behind the icon. In the DMG Finder view this plate is prominently visible. Removing it makes Xcode fall back to the pre-rendered AppIcon.appiconset PNGs, which produce a subtler standard system plate. Fixes #1256
720fb98 to
c9b7756
Compare
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Fix the double-frame app icon in the DMG installer window. macOS Finder adds a gray background plate behind icons with transparent corners. On cmux's light white icon, this creates a prominent double-frame (gray outer squircle behind the white icon squircle). The icon looks fine in the Dock because the plate is less visible at smaller sizes and darker rendering contexts.
Changes:
AppIcon.icon(Xcode 16 Icon Composer format) which added an additional dynamic platecreate-dmg(sindresorhus, no customization options) to Homebrewcreate-dmgwhich supports custom backgrounds, icon positioning, and layoutscripts/dmg-background.png) with a subtle chevron arrow. Against the dark background, the Finder plate becomes invisible and the icon renders cleanly with its full 3D depth/shadowscripts/build-sign-upload.shto use the same styled DMG layouttests/test_ci_create_dmg_pinned.shfor the new Homebrew-based setupTesting
Trigger a nightly build from this branch to verify the DMG icon renders without the double-frame:
Related
Summary by CodeRabbit