fix(ujust): only run brew bundle when enabling bluefin-cli#358
Conversation
Previously the bluefin-cli recipe always ran 'brew bundle' after toggling bling, which meant disabling bling would reinstall all Homebrew CLI packages unnecessarily. Now the recipe checks whether bling is already installed before toggling, and only runs brew bundle when enabling (i.e., bling was not previously installed). Closes #164
📝 WalkthroughWalkthroughThe ChangesBluefin CLI
🎯 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 |
hanthor
left a comment
There was a problem hiding this comment.
Reviewed: Only runs brew bundle when explicitly enabling bluefin-cli. Correct fix — prevents unnecessary brew operations on every ujust invocation.
🤖 Copilot Test ReportBranch: Test Results
ChangeExpands
|
| PR | Approach |
|---|---|
| #358 (this) | Full shell expansion — detects shell type, checks if bling was installed, skips brew on disable |
| #362 | One-liner: ublue-bling && brew bundle — simple short-circuit, brew only runs if bling enable succeeds |
These cannot both merge. #358 is the more correct fix (brew should be skipped when disabling). #362 is a partial fix (brew skips on bling failure, but still runs on disable if bling 'succeeds' at disabling).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
system_files/bluefin/usr/share/ublue-os/just/system.just (1)
31-33: 💤 Low valueConsider using conventional boolean naming for clarity.
The variable
BLING_WAS_INSTALLEDuses inverted logic:0means installed and1means not installed. This is contrary to typical boolean conventions where0usually means false/no and1means true/yes.While the logic is correct, using a name like
BLING_WAS_NOT_INSTALLEDor inverting the values would make the code more intuitive and easier to maintain.♻️ Proposed refactor for clarity
# Check if bling is currently installed before toggling - BLING_WAS_INSTALLED=1 + BLING_WAS_NOT_INSTALLED=1 if [ -f "${TARGET_FILE}" ] && grep -q "source ${BLING_SRC_DIR}/${BLING_SCRIPT}" "${TARGET_FILE}" 2>/dev/null; then - BLING_WAS_INSTALLED=0 + BLING_WAS_NOT_INSTALLED=0 fi # Toggle bling (ublue-bling will remove if installed, add if not) ublue-bling # Only install brew packages when enabling (bling was not installed before) - if [ "${BLING_WAS_INSTALLED}" -ne 0 ]; then + if [ "${BLING_WAS_NOT_INSTALLED}" -ne 0 ]; then brew bundle --file=/usr/share/ublue-os/homebrew/cli.Brewfile 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 `@system_files/bluefin/usr/share/ublue-os/just/system.just` around lines 31 - 33, The variable BLING_WAS_INSTALLED uses inverted logic (set to 1 by default, 0 when installed); rename it to BLING_WAS_NOT_INSTALLED or flip its boolean values to match conventional semantics and update the conditional accordingly — locate the initialization and check around BLING_WAS_INSTALLED (and its use with TARGET_FILE, BLING_SRC_DIR, BLING_SCRIPT) and either change the name to BLING_WAS_NOT_INSTALLED and invert the assignment/condition, or keep the name and swap the assigned values so 1=true (installed) and 0=false (not installed) throughout the script to preserve behavior while making intent clear.
🤖 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.
Nitpick comments:
In `@system_files/bluefin/usr/share/ublue-os/just/system.just`:
- Around line 31-33: The variable BLING_WAS_INSTALLED uses inverted logic (set
to 1 by default, 0 when installed); rename it to BLING_WAS_NOT_INSTALLED or flip
its boolean values to match conventional semantics and update the conditional
accordingly — locate the initialization and check around BLING_WAS_INSTALLED
(and its use with TARGET_FILE, BLING_SRC_DIR, BLING_SCRIPT) and either change
the name to BLING_WAS_NOT_INSTALLED and invert the assignment/condition, or keep
the name and swap the assigned values so 1=true (installed) and 0=false (not
installed) throughout the script to preserve behavior while making intent clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 64106387-182c-4fdb-8911-f199a8b2ed94
📒 Files selected for processing (1)
system_files/bluefin/usr/share/ublue-os/just/system.just
Summary
Fixes #164:
ujust bluefin-clino longer reinstalls Homebrew CLI packages when disabling bling.Problem
The
bluefin-clirecipe always ranbrew bundle --file=/usr/share/ublue-os/homebrew/cli.Brewfileafter toggling bling. When a user disabled bling, the recipe would:Fix
Before toggling bling, the recipe now checks whether bling is currently installed (by looking for the bling source line in the shell config). It only runs
brew bundlewhen bling was not installed — meaning the user is enabling bling.Closes #164
Test plan
ujust bluefin-cliwhen bling is enabled should disable bling without reinstalling brew packagesujust bluefin-cliwhen bling is disabled should enable bling and install brew packagesSummary by CodeRabbit