Handle desktop close confirmation requests#8607
Draft
benceruleanlu wants to merge 1 commit intomainfrom
Draft
Conversation
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/04/2026, 08:32:13 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests: ✅ PassedResults: 508 passed, 0 failed, 0 flaky, 8 skipped (Total: 516) 📊 Browser Reports
|
Contributor
📝 WalkthroughWalkthroughThis change refactors the Electron close-handling flow by introducing a dedicated close request handler that checks settings and prompts users for confirmation when modified workflows exist, while simplifying the Quit menu command. Changes
Sequence DiagramsequenceDiagram
participant User
participant ElectronAPI as Electron API
participant Handler as Close Handler
participant SettingStore
participant DialogService
participant App
User->>ElectronAPI: Trigger close request
ElectronAPI->>Handler: onCloseRequested event
Handler->>SettingStore: Load settings
SettingStore-->>Handler: Settings loaded
Handler->>Handler: Check UnloadConfirmation setting
alt UnloadConfirmation enabled
Handler->>DialogService: Prompt user confirmation
DialogService-->>User: Show confirmation dialog
User->>DialogService: User responds
DialogService-->>Handler: Confirmation result
alt User confirms
Handler->>ElectronAPI: Allow close
else User cancels
Handler->>ElectronAPI: Prevent close
end
else UnloadConfirmation disabled
Handler->>ElectronAPI: Allow close
end
ElectronAPI->>App: Close application (if allowed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.5 kB (baseline 22.5 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 838 kB (baseline 838 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 69 kB (baseline 69 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 410 kB (baseline 410 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 12 added / 12 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 3.47 kB (baseline 3.47 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 37.8 kB (baseline 37.8 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.1 MB (baseline 2.1 MB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 11 added / 11 removed Utilities & Hooks — 234 kB (baseline 234 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 12 added / 12 removed Vendor & Third-Party — 9.37 MB (baseline 9.37 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.08 MB (baseline 7.08 MB) • 🔴 +630 BBundles that do not match a named category
Status: 49 added / 49 removed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/extensions/core/electronAdapter.ts`:
- Around line 36-62: The catch block in handleCloseRequest currently calls
electronAPI.respondToCloseRequest(true) which may allow closing despite errors;
change it to deny close on error when unsaved workflows exist by checking
workflowStore.modifiedWorkflows.length and calling
electronAPI.respondToCloseRequest(false) if there are modified workflows (or
always deny close as the safest fallback), and log the error via log.error as
before; ensure you still await electronAPI.respondToCloseRequest(...) and
reference handleCloseRequest, settingStore, workflowStore, electronAPI,
dialogService, and log when making the change.
🧹 Nitpick comments (1)
src/extensions/core/electronAdapter.ts (1)
36-62: Prefer a function declaration forhandleCloseRequest.This is a pure helper and can be declared as a function instead of a const arrow for consistency with repo conventions.
♻️ Refactor suggestion
- const handleCloseRequest = async () => { + async function handleCloseRequest() { try { await settingStore.load() @@ - } + }Based on learnings: Prefer pure function declarations over function expressions (e.g., use function foo() { ... } instead of const foo = () => { ... }) for pure functions in the repository.
Comment on lines
+36
to
+62
| const handleCloseRequest = async () => { | ||
| try { | ||
| await settingStore.load() | ||
|
|
||
| const confirmOnClose = settingStore.get('Comfy.Window.UnloadConfirmation') | ||
| if (!confirmOnClose) { | ||
| await electronAPI.respondToCloseRequest(true) | ||
| return | ||
| } | ||
|
|
||
| if (workflowStore.modifiedWorkflows.length === 0) { | ||
| await electronAPI.respondToCloseRequest(true) | ||
| return | ||
| } | ||
|
|
||
| const confirmed = await dialogService.confirm({ | ||
| message: t('desktopMenu.confirmQuit'), | ||
| title: t('desktopMenu.quit'), | ||
| type: 'default' | ||
| }) | ||
|
|
||
| await electronAPI.respondToCloseRequest(confirmed === true) | ||
| } catch (error) { | ||
| log.error('Failed to handle close request.', error) | ||
| await electronAPI.respondToCloseRequest(true) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t default to allowing close on error.
In the catch block, respondToCloseRequest(true) can close the app even when unsaved workflows exist (e.g., if settings load or the confirm dialog fails). Safer fallback: deny close when there are modified workflows.
🔧 Suggested safer fallback
} catch (error) {
log.error('Failed to handle close request.', error)
- await electronAPI.respondToCloseRequest(true)
+ const hasModified = workflowStore.modifiedWorkflows.length > 0
+ await electronAPI.respondToCloseRequest(!hasModified)
}📝 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.
Suggested change
| const handleCloseRequest = async () => { | |
| try { | |
| await settingStore.load() | |
| const confirmOnClose = settingStore.get('Comfy.Window.UnloadConfirmation') | |
| if (!confirmOnClose) { | |
| await electronAPI.respondToCloseRequest(true) | |
| return | |
| } | |
| if (workflowStore.modifiedWorkflows.length === 0) { | |
| await electronAPI.respondToCloseRequest(true) | |
| return | |
| } | |
| const confirmed = await dialogService.confirm({ | |
| message: t('desktopMenu.confirmQuit'), | |
| title: t('desktopMenu.quit'), | |
| type: 'default' | |
| }) | |
| await electronAPI.respondToCloseRequest(confirmed === true) | |
| } catch (error) { | |
| log.error('Failed to handle close request.', error) | |
| await electronAPI.respondToCloseRequest(true) | |
| } | |
| } | |
| const handleCloseRequest = async () => { | |
| try { | |
| await settingStore.load() | |
| const confirmOnClose = settingStore.get('Comfy.Window.UnloadConfirmation') | |
| if (!confirmOnClose) { | |
| await electronAPI.respondToCloseRequest(true) | |
| return | |
| } | |
| if (workflowStore.modifiedWorkflows.length === 0) { | |
| await electronAPI.respondToCloseRequest(true) | |
| return | |
| } | |
| const confirmed = await dialogService.confirm({ | |
| message: t('desktopMenu.confirmQuit'), | |
| title: t('desktopMenu.quit'), | |
| type: 'default' | |
| }) | |
| await electronAPI.respondToCloseRequest(confirmed === true) | |
| } catch (error) { | |
| log.error('Failed to handle close request.', error) | |
| const hasModified = workflowStore.modifiedWorkflows.length > 0 | |
| await electronAPI.respondToCloseRequest(!hasModified) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/extensions/core/electronAdapter.ts` around lines 36 - 62, The catch block
in handleCloseRequest currently calls electronAPI.respondToCloseRequest(true)
which may allow closing despite errors; change it to deny close on error when
unsaved workflows exist by checking workflowStore.modifiedWorkflows.length and
calling electronAPI.respondToCloseRequest(false) if there are modified workflows
(or always deny close as the safest fallback), and log the error via log.error
as before; ensure you still await electronAPI.respondToCloseRequest(...) and
reference handleCloseRequest, settingStore, workflowStore, electronAPI,
dialogService, and log when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
pnpm lintpnpm typecheckLinked PRs
Refs Comfy-Org/desktop#1585
┆Issue is synchronized with this Notion page by Unito
Summary by CodeRabbit
Bug Fixes