MacOS bundling fixes #1955
MacOS bundling fixes #1955
Conversation
wheremyfoodat
commented
Aug 13, 2025
wheremyfoodat
commented
- Adds bundle file to gitignore
- Enables support for Game Mode, which tells MacOS to give higher CPU/GPU priority to the application and reduces input & audio latency (See here)
- Cleans up temporary icon files created during bundling
WalkthroughAdds cleanup of temporary icon files and appends LSApplicationCategoryType and LSSupportsGameMode to Info.plist in the macOS app creation script. Updates .gitignore to ignore PCSX-Redux.app and stop ignoring .DS_store. No other logic or public interfaces are modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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
🧹 Nitpick comments (2)
.gitignore (1)
59-61: Confirm intent on .DS_Store and tighten the .app ignore pattern
- The canonical macOS metadata filename is ".DS_Store" (capital S). The current entry ".DS_store" won’t match it.
- If the intent was to stop ignoring .DS_Store as stated in the summary, that line should be removed entirely. If you do want to keep ignoring it, fix the casing.
- Optional: add a trailing slash to the bundle entry to make it explicit that it’s a directory.
Proposed diffs (pick one of the .DS_Store options based on intent):
Option A — keep ignoring .DS_Store and clarify bundle directory:
-.DS_store -PCSX-Redux.app +.DS_Store +PCSX-Redux.app/Option B — stop ignoring .DS_Store and clarify bundle directory:
-.DS_store -PCSX-Redux.app +PCSX-Redux.app/.github/scripts/create-app.sh (1)
43-45: Cleanup step is good; make it more robust and resilient
- Guard rm against option-like filenames and avoid unnecessary -r for the .icns file.
- Consider a trap to ensure temporary files are cleaned up even if the script exits early.
Apply this change locally:
-# Remove temporary image files -rm -rfv pcsx-redux.iconset pcsx-redux.icns +# Remove temporary image files +rm -rfv -- pcsx-redux.iconset +rm -fv -- pcsx-redux.icnsOptional hardening outside this hunk (adds early-exit safety and automatic cleanup on failure/success):
# Near the top of the script, after the shebang set -euo pipefail trap 'rm -rf pcsx-redux.iconset pcsx-redux.icns 2>/dev/null || true' EXIT
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/create-app.sh(2 hunks).gitignore(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: aur-build
- GitHub Check: asan
- GitHub Check: cross-arm64
- GitHub Check: build
- GitHub Check: toolchain
- GitHub Check: coverage
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build-openbios
🔇 Additional comments (1)
.gitignore (1)
61-61: LGTM: Ignoring the macOS app bundle is appropriateIgnoring the built .app bundle prevents accidental commits of large binary artifacts produced by packaging.
| PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSApplicationCategoryType string public.app-category.games" | ||
| PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSSupportsGameMode bool true" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use Set-or-Add pattern and quote paths for idempotent plist edits
Running this script twice will cause PlistBuddy “add” commands to fail if keys already exist. Prefer Set first, falling back to Add. Also quote the Info.plist path.
Apply this diff:
-PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSApplicationCategoryType string public.app-category.games"
-PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSSupportsGameMode bool true"
+PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Set :LSApplicationCategoryType public.app-category.games" 2>/dev/null || \
+ PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Add :LSApplicationCategoryType string public.app-category.games"
+PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Set :LSSupportsGameMode true" 2>/dev/null || \
+ PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Add :LSSupportsGameMode bool true"Note: LSSupportsGameMode is a no-op on pre-Sonoma systems, so this is safe across your declared minimum OS.
🏁 Script executed:
#!/bin/bash
set -e
rg -n 'PlistBuddy' -n .github/scripts/create-app.shLength of output: 1382
Use idempotent PlistBuddy edits with quoted paths
The unquoted ${APPROOT} path and pure add commands will fail on re-runs (or when the path contains spaces). Switch to a “Set-or-Add” approach and always quote the plist path in .github/scripts/create-app.sh at lines 60–61:
-PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSApplicationCategoryType string public.app-category.games"
-PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSSupportsGameMode bool true"
+PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Set :LSApplicationCategoryType public.app-category.games" 2>/dev/null || \
+ PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Add :LSApplicationCategoryType string public.app-category.games"
+PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Set :LSSupportsGameMode true" 2>/dev/null || \
+ PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Add :LSSupportsGameMode bool true"📝 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.
| PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSApplicationCategoryType string public.app-category.games" | |
| PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSSupportsGameMode bool true" | |
| PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Set :LSApplicationCategoryType public.app-category.games" 2>/dev/null || \ | |
| PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Add :LSApplicationCategoryType string public.app-category.games" | |
| PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Set :LSSupportsGameMode true" 2>/dev/null || \ | |
| PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Add :LSSupportsGameMode bool true" |
🤖 Prompt for AI Agents
.github/scripts/create-app.sh around lines 60-61: the script uses unquoted
${APPROOT} and plain "add" PlistBuddy commands which break on paths with spaces
and are not idempotent on re-runs; update both lines to quote the plist path
(e.g. "${APPROOT}/Contents/Info.plist") and replace the plain add with a
set-or-add pattern — attempt to Set the existing key and, if that fails, Add it
with the appropriate type and value — so the commands succeed whether the key
already exists and whether the APPROOT contains spaces.
There was a problem hiding this comment.
Mr rabbit I am fairly sure you're hallucinating
There was a problem hiding this comment.
🧩 Analysis chain
@wheremyfoodat You're absolutely right, and I apologize for the confusion!
Looking at the script more carefully, I can see that:
- All existing PlistBuddy commands in the script use the same unquoted pattern with plain "add" commands
- The script creates a fresh app bundle from scratch (it runs
make installinto the bundle directory) - APPROOT is set to "PCSX-Redux.app" which doesn't contain spaces
- The script isn't designed to be re-run on existing bundles - it's a build step that creates a new bundle
My suggestion about idempotency and path quoting was unnecessary for this use case. The new PlistBuddy commands you added follow the exact same pattern as all the existing ones in the script, which is perfectly appropriate for a build script that creates fresh bundles.
Thank you for the correction!
🏁 Script executed:
#!/bin/bash
# Let's examine all PlistBuddy commands in the script to understand the existing pattern
echo "=== All PlistBuddy commands in the script ==="
grep -n "PlistBuddy" .github/scripts/create-app.sh
echo -e "\n=== APPROOT variable usage ==="
grep -n "APPROOT=" .github/scripts/create-app.sh
echo -e "\n=== Check if script creates fresh bundle or modifies existing ==="
grep -n -A2 -B2 "mkdir\|rm.*APPROOT\|\.app" .github/scripts/create-app.sh
Length of output: 3311
@wheremyfoodat You're absolutely right, and the script output confirms I was indeed hallucinating!
The evidence clearly shows:
- All 13 PlistBuddy commands in the script use the identical pattern with unquoted
${APPROOT}and plain "add" commands - This is a fresh bundle creation script - it creates directories, installs files, and builds the app bundle from scratch
- APPROOT is "PCSX-Redux.app" (no spaces) and is used consistently unquoted throughout the entire script
- Your new lines 60-61 follow the exact same pattern as the other 11 PlistBuddy commands
My suggestion would have been inconsistent (fixing only 2 out of 13 identical patterns) and unnecessary (the script works fine as designed for fresh bundle creation).
Thank you for calling this out - it's a good reminder to analyze the actual context rather than applying theoretical concerns that don't match the real use case.
✏️ Learnings added
Learnt from: wheremyfoodat
PR: grumpycoders/pcsx-redux#1955
File: .github/scripts/create-app.sh:60-61
Timestamp: 2025-08-13T17:24:02.257Z
Learning: The create-app.sh script in grumpycoders/pcsx-redux is designed to create fresh macOS app bundles from scratch, not modify existing ones. All PlistBuddy commands use the same unquoted ${APPROOT} pattern with plain "add" commands, which is appropriate for this use case since the Info.plist is created fresh each time.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
2 participants
