Adding Cycling74 Max as Fleet-maintained apps - #46333
Conversation
WalkthroughAdds Cycling 74 Max to the maintained-apps system: a Homebrew input metadata file and apps catalog entry; a macOS recipe for version 9.1.4 including installer/uninstaller script refs, DMG URL, checksum, and embedded install/uninstall scripts; a frontend SVG icon component and mapping entry; and a shell-quoting helper plus tests to ensure generated uninstall scripts correctly quote paths containing apostrophes. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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/cycling74-max/darwin.json`:
- Line 19: The uninstall cleanup contains a single-quoted string with an
embedded apostrophe that breaks shell quoting for the trash invocation (ref
"57046f31"); update the offending call—trash $LOGGED_IN_USER
'~/Library/Application Support/Cycling '74'—to use proper quoting/escaping
(e.g., switch to double quotes or escape the apostrophe) so the path passed into
the trash function (see function trash and calls to trash) is a valid shell
string and bash -n passes.
- Line 20: The install script currently moves APPDIR/Max.app to
TMPDIR/Max.app.bkp and then runs critical commands (hdiutil attach, sudo cp -R,
hdiutil detach, sudo cp -R "$TMPDIR/Max.app" "$APPDIR") without checking errors
or restoring the backup; update the flow in the c80e9471 script to: check
hdiutil attach success (fail fast and exit non-zero on error), check the sudo cp
-R from MOUNT_POINT to TMPDIR and fail+cleanup on error, only move the existing
app to TMPDIR/Max.app.bkp after successful TMPDIR copy, check the final sudo cp
-R "$TMPDIR/Max.app" "$APPDIR" and if it fails restore TMPDIR/Max.app.bkp back
to "$APPDIR/Max.app" and exit non-zero, ensure hdiutil detach and TMPDIR cleanup
always run (use traps) and keep quit_and_track_application and
relaunch_application calls unchanged except relying on the exit status to avoid
leaving Max missing.
🪄 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: 1b0aff57-9947-46eb-95aa-4d21fdf8ae24
⛔ Files ignored due to path filters (1)
website/assets/images/app-icon-cycling74-max-60x60@2x.pngis excluded by!**/*.png
📒 Files selected for processing (5)
ee/maintained-apps/inputs/homebrew/cycling74-max.jsonee/maintained-apps/outputs/apps.jsonee/maintained-apps/outputs/cycling74-max/darwin.jsonfrontend/pages/SoftwarePage/components/icons/Max.tsxfrontend/pages/SoftwarePage/components/icons/index.ts
| ], | ||
| "refs": { | ||
| "57046f31": "#!/bin/bash\n\n# variables\nAPPDIR=\"/Applications/\"\nLOGGED_IN_USER=$(scutil <<< \"show State:/Users/ConsoleUser\" | awk '/Name :/ { print $3 }')\n# functions\n\ntrash() {\n local logged_in_user=\"$1\"\n local target_file=\"$2\"\n local timestamp=\"$(date +%Y-%m-%d-%s)\"\n local rand=\"$(jot -r 1 0 99999)\"\n\n # replace ~ with /Users/$logged_in_user\n if [[ \"$target_file\" == ~* ]]; then\n target_file=\"/Users/$logged_in_user${target_file:1}\"\n fi\n\n local trash=\"/Users/$logged_in_user/.Trash\"\n local file_name=\"$(basename \"${target_file}\")\"\n\n if [[ -e \"$target_file\" ]]; then\n echo \"removing $target_file.\"\n mv -f \"$target_file\" \"$trash/${file_name}_${timestamp}_${rand}\"\n else\n echo \"$target_file doesn't exist.\"\n fi\n}\n\nsudo rm -rf \"$APPDIR/Max.app\"\ntrash $LOGGED_IN_USER '/Users/Shared/Max 9'\ntrash $LOGGED_IN_USER '~/Documents/Max 9'\ntrash $LOGGED_IN_USER '~/Library/Application Support/Cycling '74'\ntrash $LOGGED_IN_USER '~/Library/Saved Application State/com.cycling74.Max.savedState'\n", | ||
| "c80e9471": "#!/bin/bash\n\n# variables\nAPPDIR=\"/Applications/\"\nTMPDIR=$(dirname \"$(realpath \"$INSTALLER_PATH\")\")\n# functions\n\nquit_and_track_application() {\n local bundle_id=\"$1\"\n local var_name=\"APP_WAS_RUNNING_$(echo \"$bundle_id\" | tr '.-' '__')\"\n local timeout_duration=10\n\n # check if the application is running\n local app_running\n app_running=$(osascript -e \"application id \\\"$bundle_id\\\" is running\" 2>/dev/null)\n if [[ \"$app_running\" != \"true\" ]]; then\n eval \"export $var_name=0\"\n return\n fi\n\n local console_user\n console_user=$(stat -f \"%Su\" /dev/console)\n if [[ -z \"$console_user\" || \"$console_user\" == \"root\" || \"$console_user\" == \"loginwindow\" ]]; then\n echo \"Not logged into a non-root GUI; skipping quitting application ID '$bundle_id'.\"\n eval \"export $var_name=0\"\n return\n fi\n\n # App was running, mark it for relaunch\n eval \"export $var_name=1\"\n echo \"Application '$bundle_id' was running; will relaunch after installation.\"\n\n echo \"Quitting application '$bundle_id'...\"\n\n # try to quit the application within the timeout period\n local quit_success=false\n SECONDS=0\n while (( SECONDS < timeout_duration )); do\n if osascript -e \"tell application id \\\"$bundle_id\\\" to quit\" >/dev/null 2>&1; then\n if ! pgrep -f \"$bundle_id\" >/dev/null 2>&1; then\n echo \"Application '$bundle_id' quit successfully.\"\n quit_success=true\n break\n fi\n fi\n sleep 1\n done\n\n if [[ \"$quit_success\" = false ]]; then\n echo \"Application '$bundle_id' did not quit.\"\n fi\n}\n\n\nrelaunch_application() {\n local bundle_id=\"$1\"\n local var_name=\"APP_WAS_RUNNING_$(echo \"$bundle_id\" | tr '.-' '__')\"\n local was_running\n\n # Check if the app was running before installation\n eval \"was_running=\\$$var_name\"\n if [[ \"$was_running\" != \"1\" ]]; then\n return\n fi\n\n local console_user\n console_user=$(stat -f \"%Su\" /dev/console)\n if [[ -z \"$console_user\" || \"$console_user\" == \"root\" || \"$console_user\" == \"loginwindow\" ]]; then\n echo \"Not logged into a non-root GUI; skipping relaunching application ID '$bundle_id'.\"\n return\n fi\n\n echo \"Relaunching application '$bundle_id'...\"\n\n # Launch the app in the logged-in user's GUI session. Apps launched by root\n # won't register with the user's Dock/GUI, so run 'open' as the console user.\n # Use 'launchctl asuser' to bootstrap into the console user's Mach namespace\n # and GUI session — 'sudo -u' alone doesn't do this, which can cause\n # LSOpenURLsWithRole() failures even when 'open' exits 0.\n local open_status=0\n if [[ $EUID -eq 0 ]]; then\n local console_uid\n console_uid=$(id -u \"$console_user\")\n /bin/launchctl asuser \"$console_uid\" sudo -u \"$console_user\" open -b \"$bundle_id\" >/dev/null 2>&1 || open_status=$?\n else\n open -b \"$bundle_id\" >/dev/null 2>&1 || open_status=$?\n fi\n\n if [[ $open_status -eq 0 ]]; then\n echo \"Application '$bundle_id' relaunched successfully.\"\n else\n echo \"Failed to relaunch application '$bundle_id'.\"\n fi\n}\n\n\n# extract contents\nMOUNT_POINT=$(mktemp -d /tmp/dmg_mount_XXXXXX)\nhdiutil attach -plist -nobrowse -readonly -mountpoint \"$MOUNT_POINT\" \"$INSTALLER_PATH\"\nsudo cp -R \"$MOUNT_POINT\"/* \"$TMPDIR\"\nhdiutil detach \"$MOUNT_POINT\"\n# copy to the applications folder\nquit_and_track_application 'com.cycling74.Max'\nif [ -d \"$APPDIR/Max.app\" ]; then\n\tsudo mv \"$APPDIR/Max.app\" \"$TMPDIR/Max.app.bkp\"\nfi\nsudo cp -R \"$TMPDIR/Max.app\" \"$APPDIR\"\nrelaunch_application 'com.cycling74.Max'\n" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ee/maintained-apps/outputs/cycling74-max/darwin.json"
echo "== File =="
wc -l "$FILE"
echo
echo "== Lines 1-120 (with numbers) =="
nl -ba "$FILE" | sed -n '1,120p'
echo
echo "== Search for error-handling patterns =="
rg -n "set -e|pipefail|set -u|trap |||\s*open_status|exit 1|fail|restore|bkp|Max\.app\.bkp|hdiutil|cp -R" "$FILE" || trueRepository: fleetdm/fleet
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ee/maintained-apps/outputs/cycling74-max/darwin.json"
echo "== File =="
wc -l "$FILE"
echo
echo "== Full file (numbered) =="
cat -n "$FILE"
echo
echo "== Search for error-handling patterns =="
rg -n "set -e|pipefail|set -u|trap|exit 1|bkp|Max\.app\.bkp|hdiutil|cp -R" "$FILE" || trueRepository: fleetdm/fleet
Length of output: 9496
Harden install script to restore the previous Max.app if the final copy fails (and fail fast on hdiutil/cp errors).
The c80e9471 script moves "$APPDIR/Max.app" to "$TMPDIR/Max.app.bkp" and then runs sudo cp -R "$TMPDIR/Max.app" "$APPDIR" with no error handling; if that copy (or earlier hdiutil/copy) fails, Max can be left missing because the script won’t stop or restore the backup.
💡 Proposed fix pattern (inside ref c80e9471)
+set -euo pipefail
+
+# ... after backup move
+if ! sudo cp -R "$TMPDIR/Max.app" "$APPDIR"; then
+ echo "Install copy failed; restoring previous app."
+ if [ -d "$TMPDIR/Max.app.bkp" ]; then
+ sudo mv "$TMPDIR/Max.app.bkp" "$APPDIR/Max.app"
+ fi
+ exit 1
+fi🤖 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/cycling74-max/darwin.json` at line 20, The install
script currently moves APPDIR/Max.app to TMPDIR/Max.app.bkp and then runs
critical commands (hdiutil attach, sudo cp -R, hdiutil detach, sudo cp -R
"$TMPDIR/Max.app" "$APPDIR") without checking errors or restoring the backup;
update the flow in the c80e9471 script to: check hdiutil attach success (fail
fast and exit non-zero on error), check the sudo cp -R from MOUNT_POINT to
TMPDIR and fail+cleanup on error, only move the existing app to
TMPDIR/Max.app.bkp after successful TMPDIR copy, check the final sudo cp -R
"$TMPDIR/Max.app" "$APPDIR" and if it fails restore TMPDIR/Max.app.bkp back to
"$APPDIR/Max.app" and exit non-zero, ensure hdiutil detach and TMPDIR cleanup
always run (use traps) and keep quit_and_track_application and
relaunch_application calls unchanged except relying on the exit status to avoid
leaving Max missing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46333 +/- ##
==========================================
- Coverage 66.86% 66.82% -0.05%
==========================================
Files 2763 2808 +45
Lines 221032 223579 +2547
Branches 10919 11346 +427
==========================================
+ Hits 147790 149403 +1613
- Misses 59841 60613 +772
- Partials 13401 13563 +162
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add shellQuote(s) to safely wrap uninstall filesystem paths so they can be passed as single shell arguments (escapes embedded single quotes). Update processUninstallArtifact to use shellQuote for RemoveFile, rmdir and trash invocations. Add tests (scripts_test.go) to verify quoting and to run bash -n on generated uninstall scripts (covers paths like "Cycling '74"). Update darwin.json output to reflect the escaped-apostrophe quoting. This prevents generated uninstall scripts from breaking when paths contain apostrophes and fixes a regression.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/maintained-apps/ingesters/homebrew/scripts.go (1)
123-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
shellQuotefor binary uninstall targets too.
fmt.Sprintf("'%s'", target)still breaks on apostrophes (same failure mode this PR fixes elsewhere). Please route this throughshellQuotefor consistency and correctness.Suggested patch
- sb.RemoveFile(fmt.Sprintf(`'%s'`, target)) + sb.RemoveFile(shellQuote(target))🤖 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/ingesters/homebrew/scripts.go` at line 123, The call sb.RemoveFile(fmt.Sprintf(`'%s'`, target)) still embeds the target with naive single quotes and fails on apostrophes—replace it to use the existing shell quoting helper by calling sb.RemoveFile(shellQuote(target)) so binary uninstall targets are quoted safely; update the invocation in the same block where sb.RemoveFile is used (referenced symbol: sb.RemoveFile and helper: shellQuote).
🤖 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.
Outside diff comments:
In `@ee/maintained-apps/ingesters/homebrew/scripts.go`:
- Line 123: The call sb.RemoveFile(fmt.Sprintf(`'%s'`, target)) still embeds the
target with naive single quotes and fails on apostrophes—replace it to use the
existing shell quoting helper by calling sb.RemoveFile(shellQuote(target)) so
binary uninstall targets are quoted safely; update the invocation in the same
block where sb.RemoveFile is used (referenced symbol: sb.RemoveFile and helper:
shellQuote).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10f6b965-f99c-43a0-8131-2b9760959751
📒 Files selected for processing (3)
ee/maintained-apps/ingesters/homebrew/scripts.goee/maintained-apps/ingesters/homebrew/scripts_test.goee/maintained-apps/outputs/cycling74-max/darwin.json
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/maintained-apps/outputs/cycling74-max/darwin.json
Add dedicated install and uninstall scripts for cycling74-max and wire them into the Homebrew input metadata. The input JSON now includes install_script_path and uninstall_script_path. The install script mounts the DMG non-interactively (auto-accepts the SLA), copies the app to /Applications (backs up existing app), and attempts to quit and relaunch the app in the console user's GUI session. The uninstall script removes Max.app and trashes related user files. Update outputs/darwin.json to point to the new script refs and include the script contents in refs.
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/inputs/homebrew/scripts/cycling74-max-install.sh`:
- Around line 39-44: The quit verification using pgrep -f "$bundle_id" is
unreliable; replace that check with an AppleScript liveness test via osascript
using the same bundle_id (e.g., osascript -e "tell application id \"$bundle_id\"
to (application id \"$bundle_id\" is running)") and only set quit_success=true
and break when that AppleScript returns false (not running); update the block
that references bundle_id and quit_success to remove the pgrep branch and use
the osascript running-check loop to ensure the app truly quit before proceeding.
In `@ee/maintained-apps/inputs/homebrew/scripts/cycling74-max-uninstall.sh`:
- Line 5: The script extracts LOGGED_IN_USER with scutil but does not guard for
empty/"root"/"loginwindow", and later uses it unquoted when calling functions
like trash() (calls around where LOGGED_IN_USER is passed), which can shift
positional parameters; add a validation after the LOGGED_IN_USER assignment that
checks if the value is non-empty and not one of root or loginwindow (if invalid,
exit or skip uninstall steps), and update all call sites that pass
LOGGED_IN_USER (e.g., the calls to trash() on the lines noted) to use quoted
"$LOGGED_IN_USER" so the variable expansion cannot break argument parsing.
🪄 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: 82e0970f-64f7-4a44-8ebe-3f5751cdb8b4
📒 Files selected for processing (4)
ee/maintained-apps/inputs/homebrew/cycling74-max.jsonee/maintained-apps/inputs/homebrew/scripts/cycling74-max-install.shee/maintained-apps/inputs/homebrew/scripts/cycling74-max-uninstall.shee/maintained-apps/outputs/cycling74-max/darwin.json
✅ Files skipped from review due to trivial changes (1)
- ee/maintained-apps/inputs/homebrew/cycling74-max.json
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/maintained-apps/outputs/cycling74-max/darwin.json
| if osascript -e "tell application id \"$bundle_id\" to quit" >/dev/null 2>&1; then | ||
| if ! pgrep -f "$bundle_id" >/dev/null 2>&1; then | ||
| echo "Application '$bundle_id' quit successfully." | ||
| quit_success=true | ||
| break | ||
| fi |
There was a problem hiding this comment.
Use AppleScript liveness check instead of pgrep -f for app termination.
On Line 40, pgrep -f "$bundle_id" does not reliably indicate whether com.cycling74.Max is still running. This can mark quit as successful while the app is still alive. Re-check with AppleScript (application id ... is running) before proceeding.
Suggested patch
while (( SECONDS < timeout_duration )); do
if osascript -e "tell application id \"$bundle_id\" to quit" >/dev/null 2>&1; then
- if ! pgrep -f "$bundle_id" >/dev/null 2>&1; then
+ local still_running
+ still_running=$(osascript -e "application id \"$bundle_id\" is running" 2>/dev/null)
+ if [[ "$still_running" != "true" ]]; then
echo "Application '$bundle_id' quit successfully."
quit_success=true
break
fi
fi📝 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.
| if osascript -e "tell application id \"$bundle_id\" to quit" >/dev/null 2>&1; then | |
| if ! pgrep -f "$bundle_id" >/dev/null 2>&1; then | |
| echo "Application '$bundle_id' quit successfully." | |
| quit_success=true | |
| break | |
| fi | |
| if osascript -e "tell application id \"$bundle_id\" to quit" >/dev/null 2>&1; then | |
| local still_running | |
| still_running=$(osascript -e "application id \"$bundle_id\" is running" 2>/dev/null) | |
| if [[ "$still_running" != "true" ]]; then | |
| echo "Application '$bundle_id' quit successfully." | |
| quit_success=true | |
| break | |
| fi |
🤖 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/inputs/homebrew/scripts/cycling74-max-install.sh` around
lines 39 - 44, The quit verification using pgrep -f "$bundle_id" is unreliable;
replace that check with an AppleScript liveness test via osascript using the
same bundle_id (e.g., osascript -e "tell application id \"$bundle_id\" to
(application id \"$bundle_id\" is running)") and only set quit_success=true and
break when that AppleScript returns false (not running); update the block that
references bundle_id and quit_success to remove the pgrep branch and use the
osascript running-check loop to ensure the app truly quit before proceeding.
|
|
||
| # variables | ||
| APPDIR="/Applications/" | ||
| LOGGED_IN_USER=$(scutil <<< "show State:/Users/ConsoleUser" | awk '/Name :/ { print $3 }') |
There was a problem hiding this comment.
Guard invalid console users and quote LOGGED_IN_USER at call sites.
If Line 5 yields empty/root/loginwindow, unquoted calls on Lines 31/32/36/37 can shift positional arguments and break trash() behavior. Add a user validity check and quote the first argument.
Suggested patch
LOGGED_IN_USER=$(scutil <<< "show State:/Users/ConsoleUser" | awk '/Name :/ { print $3 }')
+
+if [[ -z "$LOGGED_IN_USER" || "$LOGGED_IN_USER" == "root" || "$LOGGED_IN_USER" == "loginwindow" ]]; then
+ echo "No valid logged-in GUI user found; skipping user Trash cleanup."
+ LOGGED_IN_USER=""
+fi
@@
sudo rm -rf "$APPDIR/Max.app"
-trash $LOGGED_IN_USER '/Users/Shared/Max 9'
-trash $LOGGED_IN_USER '~/Documents/Max 9'
+if [[ -n "$LOGGED_IN_USER" ]]; then
+ trash "$LOGGED_IN_USER" '/Users/Shared/Max 9'
+ trash "$LOGGED_IN_USER" '~/Documents/Max 9'
@@
-trash $LOGGED_IN_USER "~/Library/Application Support/Cycling '74"
-trash $LOGGED_IN_USER '~/Library/Saved Application State/com.cycling74.Max.savedState'
+ trash "$LOGGED_IN_USER" "~/Library/Application Support/Cycling '74"
+ trash "$LOGGED_IN_USER" '~/Library/Saved Application State/com.cycling74.Max.savedState'
+fiAlso applies to: 31-32, 36-37
🤖 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/inputs/homebrew/scripts/cycling74-max-uninstall.sh` at
line 5, The script extracts LOGGED_IN_USER with scutil but does not guard for
empty/"root"/"loginwindow", and later uses it unquoted when calling functions
like trash() (calls around where LOGGED_IN_USER is passed), which can shift
positional parameters; add a validation after the LOGGED_IN_USER assignment that
checks if the value is non-empty and not one of root or loginwindow (if invalid,
exit or skip uninstall steps), and update all call sites that pass
LOGGED_IN_USER (e.g., the calls to trash() on the lines noted) to use quoted
"$LOGGED_IN_USER" so the variable expansion cannot break argument parsing.
Delete the shellQuote function and replace its uses in processUninstallArtifact with direct single-quote wrapping (fmt.Sprintf("'%s'", ...)/inline quotes). Also remove the corresponding tests (scripts_test.go) that validated shellQuote and generated script quoting. Affects ee/maintained-apps/ingesters/homebrew/scripts.go.
Addition of Cycling '74 Max to the Fleet-Maintained apps list.
Product information: https://cycling74.com/products/max
Homebrew formula: https://formulae.brew.sh/cask/cycling74-max
Thanks!
@allenhouchins
Summary by CodeRabbit