fix: upgrade reboot failed - #876
Conversation
….3.3 - Fix lifecycle management conflicts during update installation on macOS - Add update state tracking to prevent hook interference during quit - Implement platform-specific update logic for macOS vs other platforms - Add isUpdatingInProgress() method to upgrade presenter interface - Update version from 0.3.5 to 0.3.3 for upgrade testing - Ensure Command+Q works properly during update installation
…uring update - Replace direct presenter imports with event-based communication - Add UPDATE_STATE_CHANGED event to broadcast update status to lifecycle manager - Add SET_APPLICATION_QUITTING event to control window closing behavior - Ensure proper window quit state management during macOS update process - Remove dynamic imports that could cause timing issues
- Apply lifecycle and window management fixes to all platforms, not just macOS - Set update flags and application quitting state for Windows/Linux too - Use consistent timeout approach across platforms for reliable quit behavior - Remove platform-specific logic that could leave Windows/Linux vulnerable - Update version to 0.3.6 after successful testing
WalkthroughIntroduces update-in-progress state handling across main process: adds new events for update state and quitting, tracks update state in UpgradePresenter and LifecycleManager, wires WindowPresenter to reflect quitting state, and adjusts quit-and-install flow with delays, platform-specific calls, and a force-quit timeout. Also bumps package version to 0.3.6 and updates presenter type definitions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Renderer
participant UP as UpgradePresenter
participant EV as Events Bus
participant LP as LifecyclePresenter
participant WP as WindowPresenter
participant AU as autoUpdater
participant App as Electron App
UI->>UP: User triggers "Install and Restart"
UP->>EV: UPDATE_EVENTS.WILL_RESTART
UP->>UP: setUpdatingFlag(true)
UP-->>EV: UPDATE_EVENTS.STATE_CHANGED (updating=true)
UP->>EV: WINDOW_EVENTS.SET_APPLICATION_QUITTING { isQuitting: true }
EV-->>WP: Notify isQuitting=true
WP->>WP: setApplicationQuitting(true)
Note over UP,AU: 500ms delay before quitAndInstall
UP->>AU: quitAndInstall(...platform-specific...)
rect rgba(230,250,230,0.5)
App->>LP: before-quit
LP->>LP: if isUpdateInProgress => allow quit (skip hooks)
end
AU-->>App: Exit and relaunch
alt Error during quitAndInstall
AU--x UP: Error
UP->>UP: setUpdatingFlag(false)
UP-->>EV: UPDATE_EVENTS.STATE_CHANGED (updating=false)
UP->>EV: WINDOW_EVENTS.SET_APPLICATION_QUITTING { isQuitting: false }
EV-->>WP: Notify isQuitting=false
WP->>WP: setApplicationQuitting(false)
UP-->>UI: Notify error
end
par Safety timeout
Note over UP,App: 30s force-quit timeout
UP-->>App: app.quit() (force)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Add UPDATE_EVENTS.STATE_CHANGED and WINDOW_EVENTS.SET_APPLICATION_QUITTING - Replace hardcoded event strings with proper constants from events.ts - Improve event system consistency and maintainability - Ensure proper type safety for inter-presenter communication
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/events.ts (2)
68-70: Use English for comments per guidelines.Replace Chinese comments with concise English to keep logs/comments consistent.
Apply:
- WILL_RESTART: 'update:will-restart', // 准备重启 - STATE_CHANGED: 'update:state-changed' // 更新状态变化（用于生命周期管理通信） + WILL_RESTART: 'update:will-restart', // Will restart to apply update + STATE_CHANGED: 'update:state-changed' // Update state changed (for lifecycle coordination)
76-76: Use English for comments per guidelines.Apply:
- SET_APPLICATION_QUITTING: 'window:set-application-quitting', // 设置应用退出状态 + SET_APPLICATION_QUITTING: 'window:set-application-quitting', // Set application quitting flagsrc/main/presenter/windowPresenter/index.ts (1)
155-159: Nit: comment language.Switch the comment to English to follow repo logging/comment rules.
Apply:
- // 监听更新进程设置应用退出状态的事件 + // Listen for update process toggling the application quitting statesrc/main/presenter/upgradePresenter/index.ts (1)
480-491: Good: broadcast update-state to lifecycle; minor idea.Broadcasting via
STATE_CHANGEDis correct. Consider also logging a structured info entry (logger + context) for observability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
package.json(1 hunks)src/main/events.ts(1 hunks)src/main/presenter/lifecyclePresenter/index.ts(5 hunks)src/main/presenter/upgradePresenter/index.ts(3 hunks)src/main/presenter/windowPresenter/index.ts(1 hunks)src/shared/types/presenters/legacy.presenters.d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
**/*.{js,jsx,ts,tsx}: 使用 OxLint 进行代码检查
Log和注释使用英文书写
Files:
src/shared/types/presenters/legacy.presenters.d.tssrc/main/presenter/lifecyclePresenter/index.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/upgradePresenter/index.tssrc/main/events.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-logging.mdc)
**/*.{ts,tsx}: 始终使用 try-catch 处理可能的错误
提供有意义的错误信息
记录详细的错误日志
优雅降级处理
日志应包含时间戳、日志级别、错误代码、错误描述、堆栈跟踪（如适用）、相关上下文信息
日志级别应包括 ERROR、WARN、INFO、DEBUG
不要吞掉错误
提供用户友好的错误信息
实现错误重试机制
避免记录敏感信息
使用结构化日志
设置适当的日志级别
Files:
src/shared/types/presenters/legacy.presenters.d.tssrc/main/presenter/lifecyclePresenter/index.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/upgradePresenter/index.tssrc/main/events.ts
src/shared/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
共享类型定义放在
shared目录
Files:
src/shared/types/presenters/legacy.presenters.d.ts
**/*.{ts,tsx,js,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use English for all logs and comments
Files:
src/shared/types/presenters/legacy.presenters.d.tssrc/main/presenter/lifecyclePresenter/index.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/upgradePresenter/index.tssrc/main/events.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Enable and adhere to strict TypeScript typing (avoid implicit any, prefer precise types)
Files:
src/shared/types/presenters/legacy.presenters.d.tssrc/main/presenter/lifecyclePresenter/index.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/upgradePresenter/index.tssrc/main/events.ts
src/shared/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place shared types, utilities, constants, and IPC contract definitions under src/shared/
Files:
src/shared/types/presenters/legacy.presenters.d.ts
src/{main,renderer}/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/electron-best-practices.mdc)
src/{main,renderer}/**/*.ts: Use context isolation for improved security
Implement proper inter-process communication (IPC) patterns
Optimize application startup time with lazy loading
Implement proper error handling and logging for debugging
Files:
src/main/presenter/lifecyclePresenter/index.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/upgradePresenter/index.tssrc/main/events.ts
src/main/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/electron-best-practices.mdc)
Use Electron's built-in APIs for file system and native dialogs
Files:
src/main/presenter/lifecyclePresenter/index.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/upgradePresenter/index.tssrc/main/events.ts
src/main/**/*.{ts,js,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
主进程代码放在
src/main
Files:
src/main/presenter/lifecyclePresenter/index.tssrc/main/presenter/windowPresenter/index.tssrc/main/presenter/upgradePresenter/index.tssrc/main/events.ts
🧠 Learnings (1)
📚 Learning: 2025-08-28T12:01:03.917Z
Learnt from: hllshiro
PR: ThinkInAIXYZ/deepchat#803
File: src/main/presenter/trayPresenter.ts:0-0
Timestamp: 2025-08-28T12:01:03.917Z
Learning: In the DeepChat application, calling app.quit() directly will automatically trigger the LifecycleManager's shutdown handling through Electron's before-quit event interception, so explicit calls to LifecycleManager.requestShutdown() are not needed when simply quitting the application.
Applied to files:
src/main/presenter/lifecyclePresenter/index.ts
🧬 Code graph analysis (4)
src/main/presenter/lifecyclePresenter/index.ts (2)
src/main/eventbus.ts (1)
eventBus(151-151)src/main/events.ts (1)
UPDATE_EVENTS(64-70)
src/main/presenter/windowPresenter/index.ts (2)
src/main/eventbus.ts (1)
eventBus(151-151)src/main/events.ts (1)
WINDOW_EVENTS(73-92)
src/main/presenter/upgradePresenter/index.ts (3)
src/main/eventbus.ts (1)
eventBus(151-151)src/main/events.ts (2)
UPDATE_EVENTS(64-70)WINDOW_EVENTS(73-92)src/renderer/src/events.ts (2)
UPDATE_EVENTS(47-52)WINDOW_EVENTS(55-60)
src/main/events.ts (1)
src/renderer/src/events.ts (1)
WINDOW_EVENTS(55-60)
🪛 Biome (2.1.2)
src/main/presenter/upgradePresenter/index.ts
[error] 417-418: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
🔇 Additional comments (6)
src/shared/types/presenters/legacy.presenters.d.ts (1)
970-971: Type addition aligns with implementation footprint.Adding
isUpdatingInProgress(): booleantoIUpgradePresenteris consistent with the PR’s new update-state flow.src/main/events.ts (1)
63-70: Renderer doesn’t reference the new main-only events; no additions needed.src/main/presenter/lifecyclePresenter/index.ts (3)
36-37: Good: explicit lifecycle flag for updates.Tracking
isUpdateInProgressin the manager is a clean way to short-circuit shutdown hooks during updater-driven quits.
68-70: Good: subscribed to update state.
setupUpdateStateListener()correctly wires update-state changes into lifecycle decisions.
420-426: Shutdown interception correctly skips hooks during updater quit.Early-returning from
before-quitwhenisUpdateInProgressavoids fighting the updater. Nice.src/main/presenter/upgradePresenter/index.ts (1)
65-66: State flag addition is appropriate.
_isUpdatingcleanly represents the installer phase; good that you expose a getter.
| // Execute quit and install update for all platforms | ||
| private _doQuitAndInstall(): void { | ||
| console.log('准备退出并安装更新') | ||
| console.log('Preparing to quit and install update') | ||
| try { | ||
| // 发送即将重启的消息 | ||
| // Send restart notification to all windows | ||
| eventBus.sendToRenderer(UPDATE_EVENTS.WILL_RESTART, SendTarget.ALL_WINDOWS) | ||
| // 通知需要完全退出应用 | ||
| eventBus.sendToMain(WINDOW_EVENTS.FORCE_QUIT_APP) | ||
| autoUpdater.quitAndInstall() | ||
| // 如果30秒还没完成，就强制退出重启 | ||
|
|
||
| // Set flags to prevent lifecycle and window management interference | ||
| console.log('Update installation: setting application state for proper quit behavior') | ||
| this.setUpdatingFlag(true) | ||
| eventBus.sendToMain(WINDOW_EVENTS.SET_APPLICATION_QUITTING, { isQuitting: true }) | ||
|
|
||
| // Platform-specific quit and install behavior | ||
| if (process.platform === 'darwin') { | ||
| console.log('macOS update: calling quitAndInstall with forceRunAfter=true') | ||
| // Delay to ensure message delivery completion | ||
| setTimeout(() => { | ||
| autoUpdater.quitAndInstall(false, true) // silent=false, forceRunAfter=true | ||
| }, 500) | ||
| } else { | ||
| console.log(`${process.platform} update: calling quitAndInstall`) | ||
| // For Windows/Linux, still use shorter delay but same approach | ||
| setTimeout(() => { | ||
| autoUpdater.quitAndInstall() | ||
| }, 500) | ||
| } | ||
|
|
||
| // Force quit if installation doesn't complete within 30 seconds | ||
| setTimeout(() => { | ||
| console.log('Update installation timeout, force quit') | ||
| app.quit() // Exit trigger: upgrade | ||
| }, 30000) | ||
| } catch (e) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden quitAndInstall path: catch async errors and clear timers.
Errors thrown inside the delayed callbacks won’t be caught by the outer try/catch, and force-quit timers should be cleared once quit begins.
Apply:
- // Platform-specific quit and install behavior
- if (process.platform === 'darwin') {
- console.log('macOS update: calling quitAndInstall with forceRunAfter=true')
- // Delay to ensure message delivery completion
- setTimeout(() => {
- autoUpdater.quitAndInstall(false, true) // silent=false, forceRunAfter=true
- }, 500)
- } else {
- console.log(`${process.platform} update: calling quitAndInstall`)
- // For Windows/Linux, still use shorter delay but same approach
- setTimeout(() => {
- autoUpdater.quitAndInstall()
- }, 500)
- }
+ // Delay to ensure message delivery completion, then call quitAndInstall safely
+ const invokeQuitAndInstall = () => {
+ try {
+ if (process.platform === 'darwin') {
+ console.log('macOS update: calling quitAndInstall with forceRunAfter=true')
+ autoUpdater.quitAndInstall(false, true) // silent=false, forceRunAfter=true
+ } else {
+ console.log(`${process.platform} update: calling quitAndInstall`)
+ autoUpdater.quitAndInstall()
+ }
+ } catch (err) {
+ console.error('quitAndInstall threw synchronously', err)
+ this.setUpdatingFlag(false)
+ eventBus.sendToMain(WINDOW_EVENTS.SET_APPLICATION_QUITTING, { isQuitting: false })
+ eventBus.sendToRenderer(UPDATE_EVENTS.ERROR, SendTarget.ALL_WINDOWS, {
+ error: err instanceof Error ? err.message : String(err)
+ })
+ }
+ }
+ const quitTimer = setTimeout(invokeQuitAndInstall, 500)
@@
- // Force quit if installation doesn't complete within 30 seconds
- setTimeout(() => {
+ // Force quit if installation doesn't complete within 30 seconds
+ const forceQuitTimer = setTimeout(() => {
console.log('Update installation timeout, force quit')
app.quit() // Exit trigger: upgrade
- }, 30000)
+ }, 30000)
+ app.once('will-quit', () => {
+ clearTimeout(forceQuitTimer)
+ clearTimeout(quitTimer)
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Execute quit and install update for all platforms | |
| private _doQuitAndInstall(): void { | |
| console.log('准备退出并安装更新') | |
| console.log('Preparing to quit and install update') | |
| try { | |
| // 发送即将重启的消息 | |
| // Send restart notification to all windows | |
| eventBus.sendToRenderer(UPDATE_EVENTS.WILL_RESTART, SendTarget.ALL_WINDOWS) | |
| // 通知需要完全退出应用 | |
| eventBus.sendToMain(WINDOW_EVENTS.FORCE_QUIT_APP) | |
| autoUpdater.quitAndInstall() | |
| // 如果30秒还没完成，就强制退出重启 | |
| // Set flags to prevent lifecycle and window management interference | |
| console.log('Update installation: setting application state for proper quit behavior') | |
| this.setUpdatingFlag(true) | |
| eventBus.sendToMain(WINDOW_EVENTS.SET_APPLICATION_QUITTING, { isQuitting: true }) | |
| // Platform-specific quit and install behavior | |
| if (process.platform === 'darwin') { | |
| console.log('macOS update: calling quitAndInstall with forceRunAfter=true') | |
| // Delay to ensure message delivery completion | |
| setTimeout(() => { | |
| autoUpdater.quitAndInstall(false, true) // silent=false, forceRunAfter=true | |
| }, 500) | |
| } else { | |
| console.log(`${process.platform} update: calling quitAndInstall`) | |
| // For Windows/Linux, still use shorter delay but same approach | |
| setTimeout(() => { | |
| autoUpdater.quitAndInstall() | |
| }, 500) | |
| } | |
| // Force quit if installation doesn't complete within 30 seconds | |
| setTimeout(() => { | |
| console.log('Update installation timeout, force quit') | |
| app.quit() // Exit trigger: upgrade | |
| }, 30000) | |
| } catch (e) { | |
| // Delay to ensure message delivery completion, then call quitAndInstall safely | |
| const invokeQuitAndInstall = () => { | |
| try { | |
| if (process.platform === 'darwin') { | |
| console.log('macOS update: calling quitAndInstall with forceRunAfter=true') | |
| autoUpdater.quitAndInstall(false, true) // silent=false, forceRunAfter=true | |
| } else { | |
| console.log(`${process.platform} update: calling quitAndInstall`) | |
| autoUpdater.quitAndInstall() | |
| } | |
| } catch (err) { | |
| console.error('quitAndInstall threw synchronously', err) | |
| this.setUpdatingFlag(false) | |
| eventBus.sendToMain(WINDOW_EVENTS.SET_APPLICATION_QUITTING, { isQuitting: false }) | |
| eventBus.sendToRenderer(UPDATE_EVENTS.ERROR, SendTarget.ALL_WINDOWS, { | |
| error: err instanceof Error ? err.message : String(err) | |
| }) | |
| } | |
| } | |
| const quitTimer = setTimeout(invokeQuitAndInstall, 500) | |
| // Force quit if installation doesn't complete within 30 seconds | |
| const forceQuitTimer = setTimeout(() => { | |
| console.log('Update installation timeout, force quit') | |
| app.quit() // Exit trigger: upgrade | |
| }, 30000) | |
| app.once('will-quit', () => { | |
| clearTimeout(forceQuitTimer) | |
| clearTimeout(quitTimer) | |
| }) |
🧰 Tools
🪛 Biome (2.1.2)
[error] 417-418: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
resolve auto-update restart issues
Summary by CodeRabbit