refactor: extract app lifecycle modules from main/index.ts

[reachraza]

Summary

• Extract app lifecycle functionality into dedicated app-lifecycle/ module directory
• Apply dependency injection factory pattern for improved testability
• Reduce main/index.ts by ~200 lines
• Add comprehensive test coverage (48 new tests)

Test plan

• Run npm run dev and verify app starts correctly
• Move/resize window → quit → restart → verify window state persists
• Maximize window → quit → restart → verify opens maximized
• Start agent session → make it busy → press Cmd+Q → verify quit confirmation dialog appears
• Confirm quit → verify logs show cleanup messages (tunnel, web server, processes)
• Run npm run test to verify all tests pass

[claude]

PR Review: Extract app lifecycle modules from main/index.ts

This is an excellent refactoring that significantly improves code organization, testability, and maintainability. The dependency injection pattern is well-applied, and the comprehensive test coverage is outstanding.

✅ Strengths

1. Excellent Architecture & Design

• Dependency Injection: Clean factory pattern with well-defined interfaces makes the code highly testable
• Separation of Concerns: Each module has a single, clear responsibility
• Encapsulation: State is properly encapsulated within closures (e.g., quitConfirmed, watcher)
• Module Organization: The app-lifecycle/ directory is a logical grouping for these related concerns

2. Outstanding Test Coverage

• 48 comprehensive tests covering all major code paths
• Tests verify both happy paths and edge cases (null checks, error handling, state transitions)
• Mock strategy is appropriate and well-implemented
• Good use of beforeEach/afterEach for test isolation

3. Code Quality

• Excellent documentation with JSDoc comments
• Consistent naming conventions
• Type safety with TypeScript interfaces
• Proper error handling with try-catch and null checks

4. Backward Compatibility

• No breaking changes to external behavior
• All original functionality preserved
• Refactoring is transparent to other modules

🔍 Minor Issues & Suggestions

1. Potential IPC Handler Duplicate Registration (window-manager.ts:148-191)

The registerDevAutoUpdaterStubs() function uses try-catch blocks to silently ignore duplicate handler registration errors. While this works, it has some issues:

try {
    ipcMain.handle('updates:download', async () => { ... });
} catch {
    // Handler already registered, ignore
}

Issues:

• Silently catches ALL errors, not just duplicate registration errors
• Could mask real errors (e.g., memory issues, corrupted ipcMain)
• If createWindow() is called multiple times (which the comment acknowledges), the old handlers remain registered

Suggestions:

• Check if handler exists before registering: Use a module-level flag or check ipcMain.listenerCount()
• Only catch specific error types
• Consider ipcMain.removeHandler() before re-registering
• Alternative: Move stub registration to a separate initialization function called once

2. Error Handling in CLI Watcher (cli-watcher.ts:68)

Line 68 logs a generic error:

logger.error(`Failed to start CLI activity watcher: ${error}`, 'CliActivityWatcher');

Issue: Using template literal string coercion (${error}) may not capture the full error context.

Suggestion:

const message = error instanceof Error ? error.message : String(error);
logger.error(`Failed to start CLI activity watcher: ${message}`, 'CliActivityWatcher', 
    error instanceof Error ? { stack: error.stack } : {});

3. Type Safety in Error Handlers (error-handlers.ts:24)

The unhandledRejection handler parameter types could be more specific:

process.on('unhandledRejection', (reason: unknown, promise: Promise<unknown>) => {

While unknown is safe, it's overly broad. Consider:

process.on('unhandledRejection', (reason: Error | unknown, promise: Promise<unknown>) => {

This documents that errors are the expected case while still handling other types.

4. Missing Logger Import Check in Tests

The test files mock the logger, but there's no verification that logger calls happen with the correct category/context. Consider adding assertions like:

expect(mockLogger.info).toHaveBeenCalledWith(
    expect.stringContaining('started'),
    'Startup'  // Verify the category
);

5. Potential Race Condition in Quit Flow (quit-handler.ts:98-119)

The before-quit handler is synchronous (as documented), but there's a potential race:

1. User presses Cmd+Q
2. event.preventDefault() is called
3. IPC sent to renderer: app:requestQuitConfirmation
4. If renderer is busy/frozen, user might press Cmd+Q again
5. This could send multiple confirmation requests

Suggestion: Add a flag to prevent multiple concurrent quit confirmation requests:

let isRequestingQuitConfirmation = false;

if (!state.quitConfirmed) {
    if (isRequestingQuitConfirmation) {
        event.preventDefault();
        return; // Already asking user
    }
    isRequestingQuitConfirmation = true;
    // ... existing code
}

Don't forget to reset the flag in both confirmation handlers.

6. Minor: Inconsistent Environment Variable Check

In main/index.ts:204, you define isDevelopment but in the old code it checked process.env.NODE_ENV === 'development'. The PR changes this to a local variable. Ensure this variable is set correctly at the top of the file (I see it's probably defined earlier, but good to verify consistency).

🔒 Security Considerations

✅ No security concerns identified:

• Proper context isolation maintained in window creation
• No new external inputs or user-controllable paths
• File watching is limited to internal application directory
• IPC handlers properly scoped

⚡ Performance Considerations

✅ Performance is good or improved:

• Factory pattern has negligible overhead
• File watching uses efficient fs.watch() (event-driven, not polling)
• Window state persistence unchanged
• No unnecessary re-renders or memory leaks observed

📝 Test Coverage Assessment

Coverage is excellent:

• ✅ All public methods tested
• ✅ Edge cases covered (null window, destroyed window, no watcher)
• ✅ Error paths tested
• ✅ State transitions verified
• ✅ IPC handler registration verified

Minor gap: No integration tests verifying the modules work together in main/index.ts. Consider adding a basic integration test that verifies:

• Factories are created with correct dependencies
• setup() methods are called
• Handlers are registered

🎯 Recommendations

Must Fix (before merge):

1. None - code is production-ready

Should Fix (high priority):

1. Improve registerDevAutoUpdaterStubs() to properly handle duplicate registration
2. Improve error logging in CLI watcher to capture full error context

Nice to Have (low priority):
3. Add race condition protection in quit flow
4. Add integration test for app lifecycle module coordination
5. Improve type specificity in error handlers

📊 Final Assessment

Overall Rating: 9/10 ⭐⭐⭐⭐⭐

This is a high-quality refactoring that meaningfully improves the codebase. The dependency injection pattern is textbook-perfect, the test coverage is exceptional, and the code organization makes future maintenance much easier.

The identified issues are minor and don't block merging. The refactoring achieves its stated goals:

• ✅ Reduced main/index.ts by ~200 lines
• ✅ Improved testability with dependency injection
• ✅ Added comprehensive test coverage (48 tests)
• ✅ Better code organization

Recommendation: APPROVE with minor suggested improvements 🚀

Great work on this refactoring! The code quality and testing discipline here sets a strong example for future refactoring work.

[claude]

Code Review - PR #230: Extract App Lifecycle Modules

This is an excellent refactoring PR that significantly improves code organization, testability, and maintainability. The extraction of app lifecycle functionality into dedicated modules with dependency injection is a solid architectural improvement.

✅ Strengths

1. Excellent Test Coverage (48 new tests)

   • All four modules have comprehensive unit tests
   • Tests cover both happy paths and error conditions
   • Good use of mocking and dependency injection patterns
   • Tests validate edge cases (null windows, destroyed windows, race conditions)

2. Clean Dependency Injection Pattern

   • Factory functions with explicit dependencies make the code highly testable
   • Clear separation of concerns between modules
   • Dependencies are passed as functions (getters) rather than direct references

3. Code Organization

   • Reduces main/index.ts by approximately 200 lines as advertised
   • Each module has a single, well-defined responsibility
   • Excellent TypeScript documentation with JSDoc comments
   • Proper use of TypeScript interfaces for type safety

4. Error Handling Improvements

   • Enhanced error logging with full stack traces
   • Race condition protection added to quit confirmation flow (quit-handler.ts:111-117)
   • Module-level flag prevents duplicate IPC handler registration (window-manager.ts:144-157)

Areas for Consideration

1. IPC Handler Registration (window-manager.ts:152-179)
The module-level devStubsRegistered flag is a good solution. However, if the window is recreated multiple times during app lifecycle, stub handlers are only registered on the first call. Consider adding logger.debug() when skipping registration to aid debugging.

2. Error Handling in CLI Watcher (cli-watcher.ts:68-73)
Good improvement with stack trace logging. However, if watcher fails to start, CLI playbook detection silently fails. This is acceptable graceful degradation, but consider exposing watcher status for health checks if needed.

3. Async Cleanup in Quit Handler (quit-handler.ts:148-190)
The fire-and-forget approach to cleanup is appropriate given Electron constraints (before-quit handlers must be synchronous). Long-running async operations may not complete before process exits, but this trade-off is acceptable and well-documented in comments.

4. Minor: Unrelated Changes
autorun-synopsis.md and synopsis.ts have trivial formatting changes. Ideally these would be in a separate commit for cleaner history.

Security, Performance, and Test Quality

Security: No concerns. Proper use of contextIsolation and nodeIntegration settings. IPC handlers are appropriately scoped.

Performance: No concerns. Dependency injection adds negligible overhead. File watching unchanged from original.

Tests: Exemplary test suite with 48 tests covering all scenarios including edge cases and race conditions.

Recommendations

1. Approve and Merge - This is high-quality refactoring work
2. Consider adding logger.debug() when skipping duplicate IPC handler registration
3. Consider documenting the trade-offs of fire-and-forget cleanup in architecture docs
4. Future enhancement: Consider health check endpoints that expose watcher status

Conclusion

This PR represents a significant quality improvement:

• Excellent separation of concerns
• Comprehensive test coverage
• Well-documented code
• Maintains backward compatibility
• Reduces technical debt

Rating: 9/10 - Outstanding refactoring work with only minor suggestions for future consideration.

Great job reachraza!