fix: macOS icon sizes and organize assets folder#296
fix: macOS icon sizes and organize assets folder#296buraian wants to merge 2 commits intorequestly:masterfrom
Conversation
- Add prep-icons.sh script to generate icon.icns from AppIcon.png - Add README.md documenting asset files - Regenerate icon.icns with proper Retina sizes for dock/app switcher - Delete unused files (AppIcon.icns, icon.svg)
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds assets/README.md documenting the assets directory layout, icon files (AppIcon.png, platform-specific icons, templates), auxiliary files (entitlements.mac.plist, requirement.rqset, assets.d.ts), an ASCII tree, and instructions to regenerate macOS icons. Adds assets/prep-icons.sh, a shell script that validates AppIcon.png, creates a temporary .iconset with multiple sizes via sips (including Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (2)
assets/prep-icons.sh (2)
9-12: Consider adding a macOS platform guard near the top.
sipsandiconutilare macOS-only. Running the script on Linux produces a confusingcommand not founderror rather than a clear message.♻️ Proposed fix
+if [ "$(uname)" != "Darwin" ]; then + echo "Error: This script requires macOS (sips and iconutil are macOS-only tools)" + exit 1 +fi + SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/prep-icons.sh` around lines 9 - 12, Add a macOS platform guard at the top of the script to check the OS (e.g., using uname or $OSTYPE) and exit with a clear error if not Darwin, so macOS-only commands like sips and iconutil are not invoked on Linux; modify the script around the existing variables (SCRIPT_DIR, SOURCE, OUTPUT, ICONSET) to perform the check before any sips/iconutil usage and print a descriptive message such as "This script requires macOS (Darwin) — sips and iconutil are not available" before exiting with a non-zero status.
7-7: Add anEXITtrap to ensure cleanup on early failure.With
set -e, if anysipscall fails, execution jumps past Line 44, leaving a partialICONSETdirectory behind. The next run'srm -rfrecovers, but anEXITtrap is cleaner.♻️ Proposed fix
set -e +trap 'rm -rf "$ICONSET"' EXITWith this in place, the explicit
rm -rfon Line 44 can be kept or removed (becomes a no-op).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/prep-icons.sh` at line 7, Add an EXIT trap to remove the temporary ICONSET directory on any early exit: after ICONSET is defined in the script, install a trap like trap 'rm -rf "$ICONSET"' EXIT so any failure (e.g., a failing sips call) will clean up the partial ICONSET; ensure ICONSET is exported or in scope for the trap and keep or remove the explicit rm -rf at the end as desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/README.md`:
- Line 5: The markdown directory-tree fenced block is missing a language tag
which triggers MD040; edit the README's directory-tree fenced code block that
contains "assets/" (the block with the three backticks) and change the opening
fence to include the text language identifier (```text) so the block becomes a
fenced code block with a language tag, then keep the closing triple backticks
as-is to silence the markdownlint warning.
---
Nitpick comments:
In `@assets/prep-icons.sh`:
- Around line 9-12: Add a macOS platform guard at the top of the script to check
the OS (e.g., using uname or $OSTYPE) and exit with a clear error if not Darwin,
so macOS-only commands like sips and iconutil are not invoked on Linux; modify
the script around the existing variables (SCRIPT_DIR, SOURCE, OUTPUT, ICONSET)
to perform the check before any sips/iconutil usage and print a descriptive
message such as "This script requires macOS (Darwin) — sips and iconutil are not
available" before exiting with a non-zero status.
- Line 7: Add an EXIT trap to remove the temporary ICONSET directory on any
early exit: after ICONSET is defined in the script, install a trap like trap 'rm
-rf "$ICONSET"' EXIT so any failure (e.g., a failing sips call) will clean up
the partial ICONSET; ensure ICONSET is exported or in scope for the trap and
keep or remove the explicit rm -rf at the end as desired.
Fixes #153
Summary
icon.icnswith all required Retina sizesprep-icons.shscript to generateicon.icnsfromAppIcon.pngREADME.mddocumenting all asset filesAppIcon.icns,icon.svg)Test Plan
Screenshot Verification
Summary by CodeRabbit
Documentation
Chores