fix: auto-install updates in background and use updateService in AboutSection

[moinulmoin]

Changes

• Background update checks now auto-download, install, and relaunch the app with progress toasts
• AboutSection 'Check for Updates' button now uses updateService.checkForUpdatesManually() instead of duplicate logic
• Updated doc comments to accurately describe the auto-install behavior

Testing

• All 131 tests pass
• TypeScript and ESLint checks pass

[Copilot]

Pull request overview

This PR enhances the update mechanism by implementing automatic background updates with progress feedback. The changes streamline the update workflow to automatically download, install, and relaunch when updates are detected in the background, while manual checks continue to show a dialog for user confirmation.

Key Changes:

• Background update checks now automatically download and install updates with toast notifications for progress tracking
• AboutSection refactored to use the centralized updateService.checkForUpdatesManually() method instead of duplicated update logic
• Documentation updated to reflect the new auto-install behavior

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/services/updateService.ts	Added autoInstallUpdate() method for silent background updates with progress toasts; updated doc comments to describe auto-install behavior
src/components/sections/AboutSection.tsx	Removed duplicate update check logic and tauri-apps/plugin-updater import; now delegates to updateService.checkForUpdatesManually()

---

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

[Copilot]

The initial toast at line 138 is immediately replaced by the identical toast at line 145 when the 'Started' event fires. This creates redundant toast notifications. Consider removing the initial toast and relying solely on the event handler to show the first download notification.

[Copilot]

If relaunch() fails, the error will be caught by the catch block but the error message 'Update failed' may be misleading since the update itself succeeded but only the relaunch failed. Consider adding more specific error handling around the relaunch call to provide clearer feedback to users.

[claude]

PR Review: Auto-install Updates in Background

✅ Overall Assessment

This is a solid improvement that enhances the user experience by automating the update process. The code is well-structured and follows the project's conventions.

🎯 Code Quality & Best Practices

Strengths:

• ✅ Good separation of concerns - manual vs. background update flows are clearly differentiated
• ✅ Proper use of singleton pattern for UpdateService
• ✅ Clean code removal from AboutSection.tsx - eliminated duplicate logic
• ✅ Well-documented functions with accurate comments
• ✅ Follows TypeScript best practices with proper typing
• ✅ Good use of toast notifications for user feedback
• ✅ Consistent error handling patterns

Suggestions:

1. AboutSection.tsx:35-38 - Error handling improvement:

const handleCheckUpdate = async () => {
  setIsCheckingUpdate(true);
  await updateService.checkForUpdatesManually();
  setIsCheckingUpdate(false);
};

Consider wrapping in try-catch to ensure setIsCheckingUpdate(false) always runs:

const handleCheckUpdate = async () => {
  setIsCheckingUpdate(true);
  try {
    await updateService.checkForUpdatesManually();
  } finally {
    setIsCheckingUpdate(false);
  }
};

2. updateService.ts:143-155 - Progress event handling:
   The progress callback only handles Started and Finished events. Consider handling Progress events to show download percentage:

await update.downloadAndInstall((event) => {
  if (event.event === 'Started' && event.data.contentLength) {
    toast.info(`Downloading update ${update.version}...`, { 
      id: toastId,
      duration: Infinity 
    });
  } else if (event.event === 'Progress') {
    const percent = Math.round((event.data.chunkLength / event.data.contentLength) * 100);
    toast.info(`Downloading update ${update.version}... ${percent}%`, { 
      id: toastId,
      duration: Infinity 
    });
  } else if (event.event === 'Finished') {
    toast.info('Installing update, app will restart...', { 
      id: toastId,
      duration: Infinity 
    });
  }
});

🐛 Potential Issues

Critical:

1. updateService.ts:134-165 - No user consent for auto-install:

   • Background updates now automatically download and install without user consent
   • This could be problematic if:
     • User is on metered connection
     • User is in the middle of important work
     • Update size is large
   • Recommendation: Consider adding a setting like auto_install_updates: boolean (separate from check_updates_automatically) to let users control this behavior
   • Alternative: Show a non-blocking notification asking user to confirm before downloading

2. updateService.ts:157 - Relaunch timing:

   • relaunch() is called immediately after download completes
   • If user has unsaved work, this could cause data loss
   • Recommendation: Either:
     • Check for unsaved work before relaunching
     • Delay relaunch until app is idle
     • Show a "Restart Now / Later" dialog

🔒 Security Concerns

Low severity:

1. Update validation - Ensure Tauri's updater plugin validates signatures. This appears to be handled by the Tauri framework itself, but worth documenting.
2. Consider adding a retry limit for failed downloads to prevent infinite loops if something goes wrong.

⚡ Performance Considerations

Minor:

1. updateService.ts:143 - Toast with duration: Infinity:

   • Multiple infinite-duration toasts could clutter UI if something unexpected happens
   • Current error handling dismisses them, but consider adding a timeout fallback

2. updateService.ts:60-67 - localStorage check:

   • Good optimization to prevent excessive update checks
   • Working as intended

🧪 Test Coverage

Concerns:

1. Missing unit tests for the new autoInstallUpdate() method
2. AppContainer.test.tsx only mocks updateService.initialize() - doesn't test the new auto-install flow
3. No integration tests for the background update behavior

Recommendations:

• Add unit tests for autoInstallUpdate() covering:
  • Successful download and install
  • Download failure handling
  • Install failure handling
  • Toast notifications at each stage
• Add tests for the handleCheckUpdate() function in AboutSection
• Mock the Tauri updater plugin to test different scenarios

📝 Documentation

Good:

• ✅ Updated doc comments accurately describe new behavior
• ✅ PR description is clear about the changes

Suggestions:

• Consider documenting the UX implications of auto-install in a user-facing changelog
• Add inline comments explaining why certain toast IDs are used

🎯 Recommendations Summary

Before merge:

1. Add try-finally to handleCheckUpdate() for proper cleanup
2. Consider user consent mechanism for auto-install
3. Add safety check before relaunch() to prevent data loss

Future improvements:

1. Add progress percentage to download toast
2. Add unit tests for new functionality
3. Consider adding user preference for auto-install behavior

✨ Positive Notes

• Great improvement to DX - users will appreciate automatic updates
• Clean refactoring that reduces code duplication
• Well-structured code that's easy to understand and maintain
• Good use of existing patterns and conventions

Status: ✅ Approve with minor suggestions - the concerns about auto-install behavior should be addressed, but the code quality is good overall.

[claude]

Pull Request Review: Auto-Install Updates in Background

Summary

This PR adds automatic update downloading and installation with smart session detection to prevent interrupting active recordings. The implementation is well-thought-out with good error handling and user experience considerations.

---

✅ Strengths

1. Excellent User Experience Design

• Smart deferred relaunch when user is recording/transcribing
• Progress toasts keep user informed during download
• System notifications when update must wait for session to end
• "Just updated" confirmation toast with window focus

2. Good Error Handling

• Rollback of localStorage flag if relaunch fails (lines 60-61 in updateService.ts)
• Silent failure for background checks (no user annoyance)
• Graceful degradation if notification permissions denied
• Backend verification before relaunch as safety check (lines 42-52)

3. Clean Architecture

• Singleton pattern appropriate for update service
• Clear separation between manual and background checks
• Good use of retry logic with exponential backoff

---

🐛 Potential Issues

1. Race Condition in Session State Tracking (Medium Priority)

Location: useRecording.ts:92-95 and updateService.ts:27-34

Issue: The session state is tracked via frontend state, but there's a timing gap:

// In useRecording.ts
useEffect(() => {
  const isActive = state !== 'idle' && state !== 'error';
  updateService.setSessionActive(isActive);
}, [state]);

Problem:

• Frontend state changes are asynchronous
• Backend verification happens only at relaunch time (line 43)
• User could trigger recording between download finish and relaunch
• The 30-second retry delay may accumulate multiple pending update attempts

Recommendation:

// Add session state change listener in updateService
private sessionChangeCallbacks: Array<() => void> = [];

setSessionActive(active: boolean): void {
  const wasActive = this.isSessionActive;
  this.isSessionActive = active;
  
  // Only trigger callbacks on actual state change
  if (wasActive && !active && this.pendingRelaunch) {
    this.pendingRelaunch = false;
    this.performRelaunch();
  }
}

2. Missing Tauri Command Registration (Critical)

Location: updateService.ts:43

Issue: The code calls invoke('get_current_recording_state') but I don't see this command registered in the Rust backend.

Found in: src-tauri/src/commands/audio.rs - the command exists but verify it's registered in lib.rs

Verification needed:

# Check if command is registered
grep -r "get_current_recording_state" src-tauri/src/lib.rs

If not registered, add to invoke handler list.

3. Unhandled Promise in setSessionActive (Low Priority)

Location: updateService.ts:33

Issue:

if (!active && this.pendingRelaunch) {
  this.pendingRelaunch = false;
  this.performRelaunch(); // async function not awaited
}

This is in a synchronous function calling an async function without error handling.

Recommendation:

if (!active && this.pendingRelaunch) {
  this.pendingRelaunch = false;
  this.performRelaunch().catch(error => {
    console.error('Failed to relaunch after session ended:', error);
    toast.error('Update installed. Please restart manually.');
  });
}

4. localStorage Dependency (Low Priority)

Location: updateService.ts:55, 105-106

Issue: Using localStorage for cross-session state may fail in some environments or if storage is cleared.

Recommendation: Consider using Tauri's store plugin (already in use elsewhere) for persistence:

// Use app.store instead of localStorage for cross-session state
const store = app.store('update-state');
store.set('just_updated', 'true');

---

🔒 Security Considerations

1. Update Verification ✅

• Good: Uses official Tauri updater plugin which handles signature verification
• The update download/install is secure by default

2. Notification Permissions ✅

• Properly requests permissions before sending notifications
• Graceful fallback if permissions denied

3. No User Data Exposure ✅

• No sensitive data in notification messages
• No PII in error logs

---

⚡ Performance Considerations

1. Memory Leak Risk (Low Priority)

Location: updateService.ts:261, 346

Issue: Multiple setTimeout calls could accumulate if session stays active:

setTimeout(() => this.autoInstallUpdate(update, retryCount + 1), RETRY_DELAY);

Recommendation: Store timeout IDs and clear on cleanup:

private pendingTimeouts: Set<number> = new Set();

private scheduleRetry(update: Update, retryCount: number): void {
  const timeoutId = window.setTimeout(() => {
    this.pendingTimeouts.delete(timeoutId);
    this.autoInstallUpdate(update, retryCount + 1);
  }, RETRY_DELAY);
  this.pendingTimeouts.add(timeoutId);
}

dispose(): void {
  // Clear pending retries
  this.pendingTimeouts.forEach(clearTimeout);
  this.pendingTimeouts.clear();
  // ... existing cleanup
}

2. Toast Duration: Infinity (Low Priority)

Location: updateService.ts:275, 279

Issue: Toasts with infinite duration could confuse users if app doesn't relaunch quickly.

Recommendation: Use reasonable max duration (30s) and update message:

toast.info('Downloading update (this may take a minute)...', { 
  id: toastId,
  duration: 30000 
});

---

🧪 Test Coverage

Missing Tests

The updateService.ts has no test coverage. Critical paths that need testing:

1. Session State Management

   • Test relaunch is deferred when session active
   • Test relaunch proceeds when session ends
   • Test multiple retry attempts

2. Error Scenarios

   • Test download failure handling
   • Test relaunch failure rollback (localStorage cleanup)
   • Test backend state check failure

3. Edge Cases

   • Test concurrent update checks
   • Test update available during recording
   • Test notification permission denied

Recommendation: Add src/services/updateService.test.ts:

describe('UpdateService', () => {
  describe('Session State Management', () => {
    it('should defer relaunch when session is active', async () => {
      // Test implementation
    });
    
    it('should relaunch when session ends with pending update', async () => {
      // Test implementation
    });
  });
  
  // ... more test suites
});

---

📋 Code Quality

Positive

• ✅ Excellent documentation with JSDoc comments
• ✅ Clear separation of concerns
• ✅ Consistent error handling patterns
• ✅ Good use of TypeScript types

Improvements Needed

• ⚠️ Consider extracting retry logic into separate function (DRY principle)
• ⚠️ Magic numbers (30000ms, 3 retries) should be constants with descriptive names
• ⚠️ Add TypeScript strict null checks for app.store() calls

Suggested refactor:

private static readonly MAX_SESSION_RETRIES = 3;
private static readonly SESSION_RETRY_DELAY_MS = 30000;
private static readonly TOAST_MAX_DURATION_MS = 30000;

---

🎯 Recommendations Summary

Must Fix (Before Merge)

1. ✅ Verify get_current_recording_state command is registered in Rust
2. ⚠️ Handle unhandled promise in setSessionActive
3. ⚠️ Add test coverage for critical update paths

Should Fix (Soon)

1. Clear pending timeouts on dispose to prevent memory leaks
2. Extract magic numbers to named constants
3. Use Tauri store instead of localStorage for persistence

Consider for Future

1. Add telemetry for update success/failure rates
2. Add user setting to control auto-install behavior
3. Add update rollback mechanism if new version crashes

---

✅ Final Verdict

Recommendation: Approve with Minor Changes

This is a solid implementation with good UX design and error handling. The main concerns are:

• Missing test coverage (should add before merge)
• Potential race condition with session state (low risk but worth addressing)
• Unhandled promise rejection (easy fix)

The code quality is high and follows project conventions well. Once the test coverage is added and the unhandled promise is fixed, this is ready to merge.

Great work on the session-aware update system! 🚀