feat: add beta opt-in setting for pre-release updates

[pedramamini]

Summary

• Adds a new Include beta and release candidate updates toggle in Settings → General
• When enabled, update checks include pre-release versions (beta, rc, alpha, dev, canary)
• Syncs the setting to electron-updater's allowPrerelease for auto-update support
• Documents the pre-release channel in docs/configuration.md

Test plan

• Toggle "Include beta and release candidate updates" on in Settings
• Open the Update Check modal (Help → Check for Updates) and verify pre-releases appear
• Toggle off and verify only stable releases are shown
• Confirm startup update check respects the setting

[claude]

PR Review: Beta Opt-in Setting for Pre-release Updates

Summary

This PR adds a user-facing toggle to opt into beta/pre-release updates. The implementation is clean and follows the project's architecture patterns well. Overall, this is a solid implementation with just a few minor suggestions for improvement.

---

✅ Strengths

1. Architecture & Pattern Adherence

• Correctly follows the settings persistence pattern in useSettings.ts (state + wrapper + IPC)
• Properly syncs setting to electron-updater via setAllowPrerelease()
• IPC handlers follow existing conventions in system.ts
• Documentation is thorough and user-friendly in docs/configuration.md

2. Code Quality

• Clean separation of concerns (GitHub API check vs. electron-updater)
• Good parameter naming (includePrerelease is clear and consistent)
• Appropriate default values (false = stable only, which is safer)
• Proper TypeScript typing throughout

3. User Experience

• Clear UI with FlaskConical icon for experimental features
• Setting placement in General tab makes sense
• Warning text about stability is appropriate
• Update modal properly re-checks when setting changes (useEffect on line 53-55)

---

🔍 Issues & Suggestions

Issue 1: Missing Test Coverage ⚠️

Severity: Medium

The PR adds no tests for:

• Setting persistence/loading in useSettings.ts
• checkForUpdates() with includePrerelease parameter
• Pre-release regex pattern matching in update-checker.ts:128

Recommendation:

// Example test for update-checker.ts
describe('fetchReleases', () => {
  it('should filter out pre-releases when includePrerelease is false', async () => {
    // Mock releases with mix of stable and pre-release
    const releases = [
      { tag_name: 'v1.0.0', prerelease: false, draft: false },
      { tag_name: 'v1.0.0-rc.1', prerelease: true, draft: false },
      { tag_name: 'v0.9.0-beta', prerelease: true, draft: false },
    ];
    
    const filtered = await fetchReleases(false);
    expect(filtered).toHaveLength(1);
    expect(filtered[0].tag_name).toBe('v1.0.0');
  });
});

Issue 2: Version Comparison Logic Doesn't Handle Pre-release Suffixes

Severity: Low-Medium

The compareVersions() function in update-checker.ts:91-106 only compares the numeric parts:

function parseVersion(version: string): number[] {
  const cleaned = version.replace(/^v/, '');
  return cleaned.split('.').map(n => parseInt(n, 10) || 0);
}

Problem: v1.0.0-rc.1 and v1.0.0 are treated as identical (both parse to [1, 0, 0]). This could cause issues with:

• Incorrect version counting in countVersionsBehind()
• Potentially confusing "versions behind" calculations when mixing stable and pre-release

Example:

Current: v1.0.0-rc.1
Latest: v1.0.0 (stable)

compareVersions('v1.0.0', 'v1.0.0-rc.1') returns 0 (equal)
// Should return 1 (stable > rc)

Recommendation:
Use semantic versioning comparison (e.g., semver package) or extend the comparison logic to handle pre-release identifiers properly:

npm install semver

import semver from 'semver';

function compareVersions(a: string, b: string): number {
  return semver.compare(semver.coerce(a) || a, semver.coerce(b) || b);
}

Issue 3: Race Condition in App.tsx Startup Check

Severity: Low

In App.tsx:1093-1104, the startup update check depends on both settingsLoaded and enableBetaUpdates:

useEffect(() => {
  if (settingsLoaded && checkForUpdatesOnStartup) {
    const timer = setTimeout(async () => {
      const result = await window.maestro.updates.check(enableBetaUpdates);
      // ...
    }, 2000);
    return () => clearTimeout(timer);
  }
}, [settingsLoaded, checkForUpdatesOnStartup, enableBetaUpdates]);

Issue: If enableBetaUpdates is still loading when settingsLoaded becomes true, the effect might fire twice (once with default value, once with loaded value).

Recommendation:
Either:

1. Wait for all settings to load before checking, or
2. Add a guard to prevent duplicate checks

const hasCheckedOnStartup = useRef(false);

useEffect(() => {
  if (settingsLoaded && checkForUpdatesOnStartup && !hasCheckedOnStartup.current) {
    hasCheckedOnStartup.current = true;
    // ... rest of logic
  }
}, [settingsLoaded, checkForUpdatesOnStartup, enableBetaUpdates]);

Issue 4: Inconsistent Prerelease Pattern Handling

Severity: Low

The regex pattern /-(rc|beta|alpha|dev|canary)/i in update-checker.ts:128 checks both:

1. The prerelease flag from GitHub API
2. The tag name suffix

Potential edge case: GitHub might mark a release as prerelease: false even if it has -beta in the tag name (manual tagging mistake).

Current logic:

return !r.prerelease && !prereleasePattern.test(r.tag_name);

This is actually correct (defensive), but the comment could be clearer.

Recommendation:
Update comment to explain the defensive strategy:

// Filter defensively: check both GitHub's prerelease flag AND tag naming
// to catch manually-tagged releases that might not have prerelease flag set
return !r.prerelease && !prereleasePattern.test(r.tag_name);

---

🔒 Security Considerations

No significant security concerns. The implementation:

• ✅ Doesn't introduce command injection risks
• ✅ Properly uses GitHub API (read-only, public data)
• ✅ Validates releases through existing electron-updater flow
• ✅ Doesn't expose sensitive data

---

🚀 Performance Considerations

No performance issues. The changes:

• ✅ Don't add extra API calls (reuses existing checkForUpdates())
• ✅ Setting sync is O(1) operation
• ✅ Regex filtering is minimal overhead
• ✅ React hooks are properly memoized

---

📋 Test Plan Checklist

The PR's test plan is good, but I'd add:

• Toggle setting on → verify pre-releases appear in modal
• Toggle setting off → verify only stable releases shown
• Verify startup check respects setting
• Add: Verify setting persists across app restart
• Add: Test with mixed stable/pre-release versions available
• Add: Test version comparison with pre-release suffixes (e.g., v1.0.0 vs v1.0.0-rc.1)
• Add: Verify electron-updater downloads correct version when beta enabled

---

📖 Documentation

Excellent documentation! The docs/configuration.md addition is:

• ✅ Clear and comprehensive
• ✅ Includes helpful table of version suffixes
• ✅ Has appropriate warning about stability
• ✅ Explains how to revert to stable

Suggestion: Consider adding a screenshot showing the toggle in Settings.

---

🎯 Recommendations Summary

High Priority

1. Add unit tests for version comparison and prerelease filtering logic

Medium Priority

2. Fix version comparison to properly handle pre-release suffixes (use semver library)
3. Add test coverage for settings persistence

Low Priority

4. Add guard against duplicate startup checks
5. Clarify defensive filtering logic in comments
6. Add screenshots to documentation

---

Final Verdict

✅ APPROVE with suggestions

This is a well-implemented feature that follows project conventions and has thorough documentation. The main concern is the version comparison logic which doesn't properly handle pre-release suffixes. This could lead to confusing "versions behind" counts when mixing stable and pre-release versions.

I recommend:

1. Addressing the version comparison issue before merge
2. Adding basic test coverage for the new filtering logic
3. Considering the other low-priority suggestions for future improvements

Great work overall! 🎉

[claude]

Code Review Summary

This PR adds a beta opt-in setting for pre-release updates. Overall, the implementation is solid and well-tested, but I have identified a few concerns that should be addressed.

---

✅ Strengths

Excellent Test Coverage

• Comprehensive unit tests for both the update checker and system handlers
• Tests cover the new includePrerelease parameter thoroughly
• Edge cases are well-tested (defaults, all prerelease types, draft filtering)
• Good test naming and structure

Good Documentation

• Clear documentation added to docs/configuration.md
• Warning box appropriately alerts users to pre-release risks
• Helpful table explaining pre-release version types

Clean Implementation

• Settings persistence follows the established pattern in useSettings.ts
• IPC handlers properly typed and consistent with existing code
• Proper use of the FlaskConical icon for beta settings

---

⚠️ Issues & Recommendations

1. State Synchronization Race Condition (High Priority)

Location: src/renderer/App.tsx:1084-1089

useEffect(() => {
  if (settingsLoaded) {
    window.maestro.updates.setAllowPrerelease(enableBetaUpdates);
  }
}, [settingsLoaded, enableBetaUpdates]);

Problem: The setAllowPrerelease call happens in the renderer process, but electron-updater runs in the main process. There's a potential race condition where:

1. User toggles the setting
2. checkForUpdates is called immediately
3. The setAllowPrerelease IPC call may not have completed yet
4. electron-updater might still have the old setting

Recommendation: Consider one of these approaches:

• Make setAllowPrerelease synchronous (if possible)
• Add the setting to the update check IPC call itself so it's guaranteed to be current
• Document that the setting takes effect on the next update check (if the race condition is acceptable)

2. Potential Version Comparison Issue (Medium Priority)

Location: src/main/update-checker.ts:81-106

The compareVersions function only compares numeric parts but doesn't handle pre-release identifiers in semantic versioning.

Example:

• 1.2.0 vs 1.2.0-beta will compare as equal (both become [1, 2, 0])
• 1.2.0-rc.1 vs 1.2.0-rc.2 won't sort correctly

Impact: This could lead to incorrect ordering in the update list or incorrect "versions behind" count.

Recommendation: Consider using a proper semver library like semver (already a common Node.js dependency) or enhance the version comparison to handle pre-release identifiers according to SemVer spec (Section 11).

3. Missing Initial Sync (Medium Priority)

Location: src/main/auto-updater.ts:14

The allowPrerelease is initialized to false at module load time, but there's no code that loads the saved user preference on app startup.

autoUpdater.allowPrerelease = false;  // Always starts false

Problem: If the user enabled beta updates, quit the app, and restarted, the auto-updater would revert to stable-only until the renderer loads and syncs the setting.

Recommendation: Load the setting from persistent storage in initAutoUpdater to ensure the correct preference is set before any auto-update checks occur.

4. UI Re-check on Setting Change (Low Priority)

Location: src/renderer/components/UpdateCheckModal.tsx:52-54

useEffect(() => {
  checkForUpdates();
}, [enableBetaUpdates]);

This re-checks for updates whenever the setting changes while the modal is open.

Consideration: This is probably desired behavior, but it means:

• User opens update modal → sees stable updates
• User enables beta setting → modal immediately re-checks (good!)
• But the dependency on enableBetaUpdates means this effect runs on every render where the value changes

This is likely fine, but consider if you want explicit user action to re-check instead of automatic re-checking.

5. Type Safety Enhancement (Low Priority)

Location: src/main/ipc/handlers/system.ts:225

ipcMain.handle('updates:check', async (_event, includePrerelease: boolean = false) => {

The parameter has a default but isn't validated. Consider adding runtime type checking since this comes from the renderer:

const includePrerelease = typeof args === 'boolean' ? args : false;

---

🔒 Security

No security concerns identified. The feature:

• Doesn't introduce new attack surfaces
• Uses existing trusted GitHub API endpoints
• Properly handles user input (boolean flag)
• Maintains the existing "no auto-download" security model

---

🎯 Code Quality

The code follows Maestro's conventions:

• ✅ Settings persistence pattern is correct
• ✅ IPC handlers follow established structure
• ✅ Inline styles used for theme colors
• ✅ Proper use of useEffect dependencies
• ✅ No use of shell-based execution (security best practice)

---

📝 Minor Suggestions

1. Commit Message: The commit follows the feat: convention correctly ✓

2. Documentation: Consider adding a note about what happens if a user downgrades from beta to stable (they stay on beta until manually downloading stable)

3. Test Naming: In update-checker.test.ts:282, consider renaming "includes prerelease versions when includePrerelease is true" to be more specific about what it tests (e.g., "returns prereleases in correct order when includePrerelease is true")

---

🎬 Verdict

This is a well-implemented feature with good test coverage. The main concerns are:

1. The race condition between setting sync and update checks (should be fixed)
2. Version comparison not handling pre-release identifiers (should be addressed for correctness)
3. Missing initial setting load on app startup (should be fixed)

The other issues are minor and could be addressed in a follow-up if desired.

Recommendation: Request changes to address items #1 and #3 at minimum before merging.