refactor: deduplicate update prompt logic

[shyndman]

Fixes #17

Extracts shared update prompt and git pull logic into _zush_do_update() helper function. Both _zush_prompt_available_update() and update-zush() now use this helper, eliminating 26 lines of duplicate code.

Changes

• Created _zush_do_update() helper function
• Simplified _zush_prompt_available_update() to call helper
• Simplified update-zush() to call helper
• Net reduction: 54 duplicate lines eliminated

Generated with Claude Code

[claude]

Code Review: PR #24 - Refactor: Deduplicate Update Prompt Logic

Thanks for this refactoring! The deduplication of the update prompt logic is a solid improvement that reduces code maintenance burden. Here's my detailed review:

✅ Strengths

1. Excellent Code Reuse: Successfully eliminates 54 lines of duplicate code by extracting common logic into _zush_do_update(). This is exactly the right approach for DRY (Don't Repeat Yourself) principles.

2. Consistent Behavior: Both _zush_prompt_available_update() and update-zush() now share identical update logic, preventing drift between the two code paths.

3. Clean Function Signature: The helper function is self-contained with no parameters needed, making it simple to use.

4. Proper Naming Convention: _zush_do_update() follows the existing codebase convention of prefixing internal functions with _zush_.

🐛 Potential Issues

1. Return Value Inconsistency (lib/update-check.zsh:81, 98)

   • The new _zush_do_update() returns 1 when user declines the update (line 81), but doesn't explicitly return on success (line 98)
   • In zsh, functions return the exit status of the last command executed
   • After cd "$current_dir" succeeds (typically exit code 0), the function returns 0 in the success case
   • Impact: This is actually fine for the current usage, as neither caller checks the return value. However, for clarity and future-proofing, consider explicitly returning 0 on success.

2. Missing Return Status Check (lib/update-check.zsh:87)

   • After cd "$ZUSH_HOME" || return 1, if cd fails, we return 1
   • However, the error case for git pull failure doesn't return a non-zero status - it just prints an error and continues to cd "$current_dir" before returning implicitly
   • Suggestion: Consider returning 1 after the git pull failure message for consistency

🔒 Security Considerations

1. Safe Command Execution: The git pull and cd operations are properly error-handled with 2>/dev/null and || return patterns.

2. No User Input Vulnerabilities: The read -r response is safely used in a case statement with no command injection risks.

3. File Operations: The rm -f "$ZUSH_UPDATE_AVAILABLE_FILE" is safe as the variable is controlled internally.

⚡ Performance Considerations

No performance concerns. The refactoring doesn't introduce any new operations or overhead - it's a pure extraction of existing logic.

🧪 Test Coverage

Issue: I couldn't find any test files in the repository.

Recommendation: Consider adding tests for this critical update mechanism, especially:

• Testing user responses (Y/n/no/empty)
• Testing git pull success/failure scenarios
• Testing directory navigation edge cases
• Verifying that the update-available file is properly removed

You might want to look into zsh testing frameworks like zunit or create simple integration tests.

📝 Minor Suggestions

1. Return Value Clarity: While not required, explicitly returning 0 on successful update would make the function's contract clearer

2. Function Documentation: Consider adding a brief comment above the function describing its behavior and return values

🎯 Overall Assessment

This is a well-executed refactoring that successfully achieves its goal. The code quality is good, follows existing conventions, and has no critical bugs. The minor suggestions above are purely for future maintainability and robustness.

Recommendation: ✅ Approve with minor suggestions

The PR is ready to merge as-is, though addressing the return value consistency would make the code more robust for future changes.

---

🤖 Review generated with Claude Code

[Copilot]

Pull request overview

This PR refactors the update prompt logic by extracting 26 lines of duplicate code into a new helper function _zush_do_update(). Both _zush_prompt_available_update() and update-zush() previously contained identical update prompt and git pull logic, which has now been consolidated into a single reusable helper.

Key changes:

• Introduced _zush_do_update() helper function to handle update prompts and git pull operations
• Simplified _zush_prompt_available_update() to call the helper instead of duplicating the update logic
• Simplified update-zush() to call the helper instead of duplicating the update logic
• Cleaned up trailing whitespace on lines 118 and 124

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The function doesn't return a non-zero status when git pull fails. Consider adding return 1 in the else block after the error message to properly signal failure. This would allow callers to handle the failure case if needed in the future.

Suggested change

	echo " ⚠️ Update failed. Try manually: cd ~/.config/zush && git pull"
	echo " ⚠️ Update failed. Try manually: cd ~/.config/zush && git pull"
	return 1

[claude]

PR Review: Refactor to Deduplicate Update Prompt Logic

✅ Overall Assessment

This is a solid refactoring that successfully eliminates code duplication while maintaining functionality. The PR achieves its stated goal of reducing 54 lines of duplicate code by extracting shared logic into a reusable helper function.

---

🎯 Code Quality & Best Practices

Strengths:

• ✅ Excellent DRY (Don't Repeat Yourself) implementation
• ✅ Consistent with existing codebase conventions (naming, structure, comments)
• ✅ Follows Zsh function naming pattern with _zush_ prefix for internal helpers
• ✅ Proper use of local variables to avoid scope pollution
• ✅ Maintains existing error handling patterns (|| return 1)
• ✅ Preserves all user-facing behavior and messages

Minor Suggestions:

1. Error handling in _zush_do_update() (lib/update-check.zsh:86-94)

   • The function changes directory but only restores $PWD in the success path
   • If cd "$ZUSH_HOME" fails, the function returns early without the final cd "$current_dir"
   • Consider adding cleanup to ensure we always return to the original directory

2. Return value handling (lib/update-check.zsh:73-98)

   • _zush_do_update() returns 1 when user declines, but returns nothing (implicitly 0) when update succeeds or fails
   • Consider explicit return values for all paths for better future extensibility

---

🐛 Potential Bugs or Issues

No critical bugs identified, but there's one edge case to consider:

Race condition consideration (lib/update-check.zsh:90):

• The file $ZUSH_UPDATE_AVAILABLE_FILE is removed after successful update
• If multiple shells are open and one updates, others might still have the flag file when they prompt
• Current behavior is acceptable (subsequent prompts would show "already up to date"), but worth documenting

---

⚡ Performance Considerations

No performance concerns. The refactoring:

• Reduces code size (good for startup time)
• Doesn't add extra function calls in hot paths
• Maintains existing async pattern for background checks
• Aligns with the project's performance goals (sub-200ms startup)

---

🔒 Security Concerns

No security issues identified. The code:

• ✅ Properly quotes variables ("$ZUSH_HOME", "$current_dir")
• ✅ Uses git pull --quiet 2>/dev/null to suppress potentially sensitive output
• ✅ Validates paths before cd operations
• ✅ Uses read -r to prevent backslash interpretation

---

🧪 Test Coverage

Observation: No automated tests exist in the repository for this functionality.

Recommendations for future work (not blocking for this PR):

• Manual testing should verify:
  • update-zush command works when update is available
  • update-zush shows "up to date" message when current
  • Declining update (n) properly postpones
  • Accepting update (Y/y/Enter) pulls latest changes
  • Update failure path shows error message
  • Automatic prompt on shell start works when update available

Given the project structure and shell scripting nature, manual testing is appropriate here.

---

📝 Documentation

• ✅ Clear function comment added: "Perform the update with user prompt"
• ✅ PR description well-written with clear summary of changes
• ✅ Fixes Deduplicate update prompt logic in update-check.zsh #17 properly referenced
• ℹ️ Consider adding inline comment explaining return value semantics if implementing suggestion Pull request for issue #1 #2 above

---

✨ Summary

This is ready to merge with only minor suggestions for enhancement. The refactoring successfully:

• Eliminates 54 lines of duplicate code
• Maintains existing functionality
• Follows project conventions
• Improves maintainability

The suggestions above are optional improvements that could be addressed in a follow-up if desired, but they don't block this PR.

Verdict: LGTM! 🚀

Great work on keeping the codebase clean and maintainable! 🦥

[claude]

Code Review: PR #24 - Refactor: Deduplicate Update Prompt Logic

Summary

This PR successfully addresses issue #17 by extracting duplicate update prompt logic into a shared helper function. The refactoring is clean and achieves its stated goal of reducing code duplication.

✅ Strengths

1. Excellent DRY Principle Application: Successfully eliminates 54 lines of duplicate code by extracting common logic into _zush_do_update() helper (lib/update-check.zsh:73-98)
2. Behavioral Consistency: Both _zush_prompt_available_update() and update-zush() now share identical update logic, ensuring consistent UX
3. Clean Implementation: The helper function is well-structured with clear control flow
4. Maintains Existing Behavior: The refactor preserves all existing functionality without introducing breaking changes

🔍 Code Quality Observations

Good Practices

• Proper use of local variables for scope isolation (lib/update-check.zsh:85, 94)
• Consistent error handling with fallback return codes
• Clear user-facing messages with emojis matching project style
• Proper cleanup of state file on successful update (lib/update-check.zsh:90)

Consistency with Codebase

• Follows existing naming convention for private functions (_zush_*)
• Matches the style and structure of other functions in the file
• Consistent with error handling patterns seen in lib/core.zsh

⚠️ Potential Issues & Recommendations

1. Return Code Inconsistency (Minor)

Location: lib/update-check.zsh:94

The function doesn't restore the original directory if cd "$ZUSH_HOME" fails (line 86), but does restore it after a successful/failed git pull. This could leave the shell in $ZUSH_HOME if the initial cd succeeds but git pull fails.

Recommendation: Consider adding directory restoration in all paths, or document this behavior.

2. Error Handling on Directory Restoration (Minor)

Location: lib/update-check.zsh:94

The cd "$current_dir" at the end doesn't check for failure. While unlikely to fail, it could leave the user in an unexpected location.

Recommendation: Consider cd "$current_dir" || true to prevent propagating errors, or wrap in conditional logic.

3. Silent Failure Mode (Minor)

Location: lib/update-check.zsh:87

The git pull --quiet 2>/dev/null suppresses all stderr, which might hide useful diagnostic information if the pull fails.

Current behavior: Users only see "⚠️ Update failed" without details
Consideration: This may be intentional for UX simplicity, but could make troubleshooting harder.

4. Race Condition in File Removal (Very Minor)

Location: lib/update-check.zsh:90

The update marker file is removed after successful pull but before directory restoration. If directory restoration somehow failed critically, the marker would be gone even though the user might not see success message.

Note: This is extremely unlikely and probably not worth addressing, but documenting for completeness.

🧪 Test Coverage

Observation: No test files exist in the repository (checked for *test* and *spec* patterns).

Recommendation: While shell script testing can be challenging, consider:

• Adding basic smoke tests for the update mechanism
• Testing edge cases like network failures, non-git directories, permission issues
• At minimum, documenting manual testing procedures in PR description

🔒 Security Considerations

✅ No security concerns identified:

• No command injection risks (variables are properly quoted)
• No arbitrary code execution vulnerabilities
• User input is handled safely via case statement
• Git operations use appropriate safety flags (--quiet)

⚡ Performance Considerations

✅ No performance impact:

• Function call overhead is negligible
• Actually improves maintainability which aids long-term performance
• No changes to background check mechanism (lib/update-check.zsh:36-62)

📝 Documentation

Minor Observation: The new _zush_do_update() function has a single-line comment describing its purpose (lib/update-check.zsh:72), which is consistent with the rest of the codebase.

🎯 Overall Assessment

Verdict: ✅ Approve with minor suggestions

This is a solid refactoring PR that achieves its goal effectively. The code quality is good, follows project conventions, and introduces no bugs or security issues. The minor recommendations above are optional improvements rather than blocking issues.

Impact:

• Improved maintainability ✅
• Reduced code duplication ✅
• Consistent update UX ✅
• No breaking changes ✅

Great work on this cleanup! The extraction makes future updates to the update flow much easier to maintain.

---

Review generated with Claude Code