macOS FMA - Icon Composer - #46735
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46735 +/- ##
==========================================
- Coverage 66.91% 66.91% -0.01%
==========================================
Files 2834 2835 +1
Lines 224966 224968 +2
Branches 11517 11517
==========================================
+ Hits 150527 150528 +1
- Misses 60783 60784 +1
Partials 13656 13656 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
WalkthroughThis PR adds Icon Composer as a new maintained application to the Fleet deployment system. It introduces the app's Homebrew manifest definition, registers it in the maintained apps catalog, and provides installation/uninstallation shell scripts that handle DMG mounting, application copying, and cleanup. CI/CD workflows are extended to detect icon-composer/darwin changes and conditionally remove pre-installed Xcode during validation. The frontend is updated with an SVG icon component and registry mapping to display Icon Composer in the software page. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ee/maintained-apps/outputs/icon-composer/darwin.json`:
- Around line 6-7: The install-state SQL for "exists" and "patched" is too broad
(matches any com.apple.IconComposer bundle); change both query strings to
restrict to the standalone app bundle by requiring the app record's path to be
'/Applications/Icon Composer.app' (e.g. add AND path = '/Applications/Icon
Composer.app' to the WHERE clauses that reference bundle_identifier =
'com.apple.IconComposer'). Ensure you update both the "exists" and "patched"
entries so Fleet only considers the managed standalone install.
- Line 19: The installer currently moves the live app aside before verifying the
staged copy; update the deployment flow in the script around TMPDIR/APPDIR so
you first copy and validate the new bundle into TMPDIR (verify file existence
and a successful sudo cp -R), only then atomically swap-in the new app (use sudo
mv to replace the live "/Applications/Icon Composer.app"), and if the swap or
verification fails restore the original from the backup (or abort without
removing the original). Ensure error checking sets non-zero exit codes on
failures, wrap the sequence that does sudo cp -R "$TMPDIR/Icon Composer.app"
"$APPDIR" and sudo mv "$APPDIR/Icon Composer.app" "$APPDIR/Icon
Composer.app.bkp" with checks that revert to the backup on failure, and keep
quit_and_track_application and relaunch_application calls unchanged except to
run quit before the verified swap and relaunch only after successful swap.
🪄 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: CHILL
Plan: Pro
Run ID: 8993dc83-58c6-4588-87e8-e833d7ccc9cd
⛔ Files ignored due to path filters (1)
website/assets/images/app-icon-icon-composer-60x60@2x.pngis excluded by!**/*.png
📒 Files selected for processing (7)
.github/workflows/test-fma-darwin-pr-only.yml.github/workflows/test-fma-darwin.ymlee/maintained-apps/inputs/homebrew/icon-composer.jsonee/maintained-apps/outputs/apps.jsonee/maintained-apps/outputs/icon-composer/darwin.jsonfrontend/pages/SoftwarePage/components/icons/IconComposer.tsxfrontend/pages/SoftwarePage/components/icons/index.ts
Restrict detection and patch checks for com.apple.IconComposer to exclude copies bundled inside Xcode.app by adding `path NOT LIKE '%/Xcode.app/%'` to both `exists` and `patched` SQL queries. This avoids false positives from the Xcode bundle and ensures version comparisons only apply to standalone installations.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
ee/maintained-apps/outputs/icon-composer/darwin.json (2)
19-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake app replacement transactional with rollback.
On Line 19, the installer still moves the live app before verifying the staged copy and has no restore path on failure. A failed copy/swap can leave the host without Icon Composer.
Suggested flow change
- if [ -d "$APPDIR/Icon Composer.app" ]; then - sudo mv "$APPDIR/Icon Composer.app" "$TMPDIR/Icon Composer.app.bkp" - fi - sudo cp -R "$TMPDIR/Icon Composer.app" "$APPDIR" + # 1) validate staged app exists + [ -d "$TMPDIR/Icon Composer.app" ] || exit 1 + # 2) stage into /Applications with temp name + sudo rm -rf "$APPDIR/Icon Composer.app.new" + sudo cp -R "$TMPDIR/Icon Composer.app" "$APPDIR/Icon Composer.app.new" || exit 1 + # 3) swap only after successful stage + if [ -d "$APPDIR/Icon Composer.app" ]; then + sudo mv "$APPDIR/Icon Composer.app" "$APPDIR/Icon Composer.app.bkp" || exit 1 + fi + sudo mv "$APPDIR/Icon Composer.app.new" "$APPDIR/Icon Composer.app" || { + [ -d "$APPDIR/Icon Composer.app.bkp" ] && sudo mv "$APPDIR/Icon Composer.app.bkp" "$APPDIR/Icon Composer.app" + exit 1 + } + sudo rm -rf "$APPDIR/Icon Composer.app.bkp"🤖 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 `@ee/maintained-apps/outputs/icon-composer/darwin.json` at line 19, The installer currently moves the live app before verifying the staged copy; modify the install flow around APPDIR/TMPDIR (where it currently does sudo mv "$APPDIR/Icon Composer.app" "$TMPDIR/Icon Composer.app.bkp" and sudo cp -R "$TMPDIR/Icon Composer.app" "$APPDIR") to be transactional: first ensure the staged "$TMPDIR/Icon Composer.app" is valid (exists, readable, and launches or at least contains expected Info.plist bundle identifier), then atomically swap by (1) moving the existing "$APPDIR/Icon Composer.app" to a backup (e.g., "$APPDIR/Icon Composer.app.bkp") only after verification succeeds, (2) moving the staged app into "$APPDIR" (using sudo mv), and on any failure during the swap restore the backup to "$APPDIR" and clean up staged files; also remove the backup only after successful install and relaunch_application('com.apple.IconComposer') succeeds. Ensure all operations use sudo where necessary and propagate non-zero exit codes to trigger the rollback.
6-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConstrain install-state queries to Fleet-managed path.
On Line 6 and Line 7, excluding
Xcode.appis not sufficient; these queries can still match non-managed installs outside/Applications/Icon Composer.app, causing state drift versus the uninstall/install target.Proposed minimal fix
- "exists": "SELECT 1 FROM apps WHERE bundle_identifier = 'com.apple.IconComposer' AND path NOT LIKE '%/Xcode.app/%';", - "patched": "SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM apps WHERE bundle_identifier = 'com.apple.IconComposer' AND path NOT LIKE '%/Xcode.app/%' AND version_compare(bundle_short_version, '1.2') < 0);" + "exists": "SELECT 1 FROM apps WHERE bundle_identifier = 'com.apple.IconComposer' AND path = '/Applications/Icon Composer.app';", + "patched": "SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM apps WHERE bundle_identifier = 'com.apple.IconComposer' AND path = '/Applications/Icon Composer.app' AND version_compare(bundle_short_version, '1.2') < 0);"🤖 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 `@ee/maintained-apps/outputs/icon-composer/darwin.json` around lines 6 - 7, The current SQL in the "exists" and "patched" entries still matches installs outside the Fleet-managed location; update both queries to constrain the path to the Fleet-managed install location (e.g., require path to start with or equal '/Applications/Icon Composer.app') instead of only excluding Xcode.app. Modify the "exists" and "patched" SELECTs that reference bundle_identifier = 'com.apple.IconComposer' and path NOT LIKE '%/Xcode.app/%' to add a path condition that limits results to the Fleet-managed path (for example path LIKE '/Applications/Icon Composer.app/%' or path = '/Applications/Icon Composer.app').
🤖 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.
Duplicate comments:
In `@ee/maintained-apps/outputs/icon-composer/darwin.json`:
- Line 19: The installer currently moves the live app before verifying the
staged copy; modify the install flow around APPDIR/TMPDIR (where it currently
does sudo mv "$APPDIR/Icon Composer.app" "$TMPDIR/Icon Composer.app.bkp" and
sudo cp -R "$TMPDIR/Icon Composer.app" "$APPDIR") to be transactional: first
ensure the staged "$TMPDIR/Icon Composer.app" is valid (exists, readable, and
launches or at least contains expected Info.plist bundle identifier), then
atomically swap by (1) moving the existing "$APPDIR/Icon Composer.app" to a
backup (e.g., "$APPDIR/Icon Composer.app.bkp") only after verification succeeds,
(2) moving the staged app into "$APPDIR" (using sudo mv), and on any failure
during the swap restore the backup to "$APPDIR" and clean up staged files; also
remove the backup only after successful install and
relaunch_application('com.apple.IconComposer') succeeds. Ensure all operations
use sudo where necessary and propagate non-zero exit codes to trigger the
rollback.
- Around line 6-7: The current SQL in the "exists" and "patched" entries still
matches installs outside the Fleet-managed location; update both queries to
constrain the path to the Fleet-managed install location (e.g., require path to
start with or equal '/Applications/Icon Composer.app') instead of only excluding
Xcode.app. Modify the "exists" and "patched" SELECTs that reference
bundle_identifier = 'com.apple.IconComposer' and path NOT LIKE '%/Xcode.app/%'
to add a path condition that limits results to the Fleet-managed path (for
example path LIKE '/Applications/Icon Composer.app/%' or path =
'/Applications/Icon Composer.app').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71b10982-bbc8-4663-802b-4e181d354fdc
📒 Files selected for processing (1)
ee/maintained-apps/outputs/icon-composer/darwin.json
|
@lukeheath @georgekarrv @getvictor can we get a review on this? It requires CODEOWNERS since its editing an existing workflow. The edit is small, just expanding on an already established pattern to account for a new app. |
| find /Applications -maxdepth 1 -iname "Xcode*.app" -type d | while read app; | ||
| do | ||
| echo "Removing $app..." | ||
| sudo rm -rf "$app" |
There was a problem hiding this comment.
No one else squeamish by this :D
Summary by CodeRabbit
New Features
Chores