fix(installer): fix completions when legacy minimal was previously installed - #741
Conversation
📝 WalkthroughWalkthroughThe installer now manages zsh’s Changeszsh completion cache lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant ZshInit as zsh init
participant Compinit as compinit
participant Cache as .zcompdump
Installer->>Cache: clear cache during completion generation
ZshInit->>Compinit: run compinit
ZshInit->>Cache: remove stale cache when min is unregistered
ZshInit->>Compinit: rerun compinit
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install.sh (1)
612-621: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent failure path is inconsistent with the rest of the file's warning conventions.
The uninstall-side cache removal (Line 317) warns when
rm -ffails, but this generation-time clear swallows a failed removal silently. Since the comment already documents this as "as non-fatal as the rest of R9.3," a matching warning would improve observability with negligible cost, though impact is low since the stale dump gets a fresh chance to be cleared on the next regeneration/uninstall.♻️ Optional: warn on failed clear for consistency
if [ "$1" = zsh ] && [ -f "$zcompdump" ]; then if rm -f "$zcompdump" 2>/dev/null; then say " completions: cleared compinit dump cache $zcompdump" + else + say " completions: warning: could not clear compinit dump cache $zcompdump" fi 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 `@scripts/install.sh` around lines 612 - 621, Update the zsh compinit dump removal block in the generation path to warn when rm -f "$zcompdump" fails, matching the uninstall-side warning convention while keeping the failure non-fatal. Preserve the existing success message and conditional behavior.
🤖 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 `@scripts/install.sh`:
- Around line 612-621: Update the zsh compinit dump removal block in the
generation path to warn when rm -f "$zcompdump" fails, matching the
uninstall-side warning convention while keeping the failure non-fatal. Preserve
the existing success message and conditional behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f08cc7d-466d-4bca-9394-a90eb4806024
📒 Files selected for processing (3)
docs/specs/07-spec-installer/07-spec-installer.mdscripts/install.shscripts/install_test.sh
I think i was the only one hitting this lol
Summary by CodeRabbit
Bug Fixes
Tests