feat(macos): embed SessionLedger app icon metadata - #388
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummaryAdds macOS application icon metadata and embeds Must FixNone identified. Should FixNone identified. Consider
Approve / Request ChangesApprove. WalkthroughThe macOS packaging script now emits additional ChangesmacOS packaging
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packaging/macos/package-app.sh`:
- Around line 51-54: Update the bundle metadata near CFBundleIconFile by
removing CFBundleIconName, since the package includes only the standalone
AppIcon.icns and no asset catalog. Keep CFBundleIconFile set to AppIcon.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 25a1e834-60f8-4373-b503-37f0880cde0b
📒 Files selected for processing (1)
packaging/macos/package-app.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (32)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: race smoke + channel/cancel model · windows-latest
- GitHub Check: clean-host smoke · Windows unsigned portable install
- GitHub Check: race smoke + channel/cancel model · macos-latest
- GitHub Check: jemalloc default-on · windows default build
- GitHub Check: session-ledger build · windows-latest
- GitHub Check: sl-daemon build · windows-latest
- GitHub Check: jemalloc default-on · unix default build
- GitHub Check: sl-daemon · repository builder image offline build / sl-daemon · repository builder image offline build
- GitHub Check: compression ratio gate
- GitHub Check: jemalloc hard · feature build
- GitHub Check: alloc profile hard · dhat smoke
- GitHub Check: daemon graph hard · tokio graph
- GitHub Check: cargo deny check
- GitHub Check: cargo audit
- GitHub Check: load macro gate · macro routes smoke
- GitHub Check: update check hard · sl-daemon tests
- GitHub Check: update check hard · root SelfCheck wrapper
- GitHub Check: fuzz blocking · SelfCheck
- GitHub Check: loom permutation · daemon mpsc
- GitHub Check: loom permutation · daemon pipeline
- GitHub Check: loom permutation · daemon broadcast
- GitHub Check: miri permutation · race_model
- GitHub Check: fuzz blocking · sustained 30s
- GitHub Check: loom permutation · daemon shutdown
- GitHub Check: loom permutation · hermetic wrappers
- GitHub Check: pipeline perf regression gate
- GitHub Check: loom permutation · core models
- GitHub Check: shuttle permutation · cargo test shuttle_permutation
- GitHub Check: visual contract · WCAG AA
- GitHub Check: browser e2e · axe · responsive · visual
- GitHub Check: prepare
⚠️ CI failures not shown inline (2)
GitHub Check: Summary: The current Mergify configuration is invalid
Conclusion: failure
* Invalid condition 'author=dependabot[bot] | renovate[bot]' @ root → pull_request_rules → item 1 → conditions → item 0 → author=dependabot[bot] | renovate[bot]
```
Invalid GitHub login
```
* Invalid condition 'author=trunk-io[bot] | mergify[bot] | github-actions[bot]' @ root → pull_request_rules → item 2 → conditions → item 0 → author=trunk-io[bot] | mergify[bot] | github-actions[bot]
```
Invalid GitHub login
```
* Invalid condition 'age>=30d' @ root → pull_request_rules → item 8 → conditions → item 2 → age>=30d
```
Invalid attribute
```
* Extra inputs are not permitted @ root → pull_request_rules → item 0 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 1 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 3 → actions → request_reviews → github_accounts
GitHub Check: Mergify Merge Queue: The current Mergify configuration is invalid
Conclusion: failure
* Invalid condition 'author=dependabot[bot] | renovate[bot]' @ root → pull_request_rules → item 1 → conditions → item 0 → author=dependabot[bot] | renovate[bot]
```
Invalid GitHub login
```
* Invalid condition 'author=trunk-io[bot] | mergify[bot] | github-actions[bot]' @ root → pull_request_rules → item 2 → conditions → item 0 → author=trunk-io[bot] | mergify[bot] | github-actions[bot]
```
Invalid GitHub login
```
* Invalid condition 'age>=30d' @ root → pull_request_rules → item 8 → conditions → item 2 → age>=30d
```
Invalid attribute
```
* Extra inputs are not permitted @ root → pull_request_rules → item 0 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 1 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 3 → actions → request_reviews → github_accounts
🔇 Additional comments (2)
packaging/macos/package-app.sh (2)
49-50: LGTM!Also applies to: 55-56
66-75: LGTM!
| <key>CFBundleIconFile</key> | ||
| <string>AppIcon</string> | ||
| <key>CFBundleIconName</key> | ||
| <string>AppIcon</string> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '1,120p' packaging/macos/package-app.sh | cat -n
echo
echo "== repository icon-related files =="
git ls-files | rg -i 'AppIcon|iconutil|package-app|\.icns|\.car|Assets\.car|xcassets' || true
echo
echo "== search CFBundleIconName/CFBundleIconFile references =="
rg -n "CFBundleIcon(Name|File)|Assets\.car|AppIcon\.icns|iconutil" . || trueRepository: KooshaPari/SessionLedger
Length of output: 3993
🌐 Web query:
Apple Info.plist CFBundleIconName CFBundleIconFile Assets.car AppIcon.icns key reference
💡 Result:
In modern Apple development, application icons are primarily managed through Asset Catalogs, which are compiled into a binary file named Assets.car located in the app bundle's Contents/Resources directory [1][2]. The Info.plist keys CFBundleIconName and CFBundleIconFile serve different roles based on the platform version and the icon technology used [3][4][1]. CFBundleIconName (String) This is the modern key used to reference an icon asset within an Assets.car file [3][1]. When provided, the system looks inside the compiled asset catalog for an icon set with the specified name [3][5]. This key is essential for modern macOS (including the Liquid Glass icon system introduced in macOS 26) and iOS applications that use Asset Catalogs [2][6][7]. CFBundleIconFile (String) This is a legacy key that identifies a standalone icon file (typically AppIcon.icns) located in the bundle's resources directory [3][8][4]. While older versions of macOS and legacy apps rely on this for icon display, it is often included alongside CFBundleIconName to ensure backward compatibility for older operating systems that do not support modern asset catalogs or the newer icon formats [1][9][10]. Relationship and Usage 1. Assets.car: This is the compiled output of an asset catalog (e.g.,.xcassets or.icon files). It acts as a container for various icon resolutions, layers, and, in newer versions, advanced graphical materials [2][6]. 2. AppIcon.icns: This remains the standard for legacy macOS icon support. Developers often include both the.icns file and the Assets.car file in their bundle to support both older and newer OS versions [1][9][10]. 3. Modern Best Practice: When building apps with modern Xcode versions, you should use an Asset Catalog to manage your icons. Xcode handles the compilation of these assets into Assets.car and automatically configures the appropriate Info.plist keys [11][2]. If you are building outside of Xcode, you must ensure the Assets.car is correctly generated (often using actool) and that your Info.plist correctly identifies the icon name to match the entry within that catalog [9][2][6]. In summary, CFBundleIconName is the modern standard for referencing icons from an asset catalog, whereas CFBundleIconFile is a legacy reference for standalone.icns files [3][1]. For cross-version compatibility, many developers bundle both and define both keys in the Info.plist [9][10].
Citations:
- 1: http://www.geoffprewett.com/blog/software/AnatomyOfAnAppBundle.html
- 2: https://jimeh.io/how-to-add-apples-new-liquid-glass-icons-to-applications-67e0ff054de9
- 3: https://developer.apple.com/library/archive/documentation/General/Reference/InfoPlistKeyReference/Articles/CoreFoundationKeys.html
- 4: https://web.archive.org/web/20160406212251/developer.apple.com/library/ios/documentation/General/Reference/InfoPlistKeyReference/Articles/CoreFoundationKeys.html
- 5: https://developer.apple.com/documentation/bundleresources/information-property-list/cfbundleiconname
- 6: https://www.hendrik-erz.de/post/supporting-liquid-glass-icons-in-apps-without-xcode
- 7: https://developer.apple.com/forums/thread/92638
- 8: https://developer.apple.com/library/archive/documentation/CoreFoundation/Conceptual/CFBundles/BundleTypes/BundleTypes.html
- 9: https://successfulsoftware.net/2025/09/26/updating-application-icons-for-macos-26-tahoe-and-liquid-glass/
- 10: https://mjtsai.com/blog/2025/08/08/separate-icons-for-macos-tahoe-vs-earlier/
- 11: https://developer.apple.com/documentation/xcode/configuring-your-app-icon
Include the full icon metadata with the standalone .icns.
This bundle creates only Contents/Resources/AppIcon.icns, and CFBundleIconName points to an icon asset in an asset catalog that is not packaged. Remove CFBundleIconName as the comment suggests, or add the matching Assets.car; CFBundleIconFile=AppIcon alone is sufficient for the .icns output.
🤖 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 `@packaging/macos/package-app.sh` around lines 51 - 54, Update the bundle
metadata near CFBundleIconFile by removing CFBundleIconName, since the package
includes only the standalone AppIcon.icns and no asset catalog. Keep
CFBundleIconFile set to AppIcon.
Auto-merge: routine maintenance lane
Adds AppIcon metadata and embeds the tracked iconset into unsigned SessionLedger.app bundles when iconutil is available.
Validation: built a temporary bundle with a fake executable, iconutil produced AppIcon.icns, and plutil validated Info.plist.