feat: add file save confirmation on tab close#1982
Conversation
Greptile SummaryThis PR adds an unsaved-changes confirmation dialog that appears when a user closes a dirty file tab, wiring the new
Confidence Score: 3/5Two real defects on the new code path — no Cancel button in the dialog and silent data loss on save failure — need fixes before this ships to users. The dialog's missing Cancel button means a user who accidentally clicks the tab-close X has no way to abort; they must either save or discard. More critically, when the user chooses Save but the disk write fails, saveFile swallows the error and _confirmClose still resolves to 'proceed', so closeTab fires and the unsaved changes are lost with no notification. Both issues are on the primary new code path introduced by this PR. unsaved-changes-dialog.tsx (missing Cancel action) and the _confirmClose method in file-model-lifecycle-store.ts (save-error handling) need the most attention before merging.
|
| Filename | Overview |
|---|---|
| src/renderer/lib/components/unsaved-changes-dialog.tsx | New dialog component for unsaved-changes confirmation; missing a Cancel button, leaving users no explicit way to abort a tab close. |
| src/renderer/features/tasks/editor/stores/file-model-lifecycle-store.ts | Adds close-guard logic and _confirmClose; save errors are swallowed by saveFile, so a failed save still resolves to 'proceed' and closes the tab with data loss. |
| src/renderer/features/tasks/tabs/tab-manager-store.ts | Adds closeTabWithGuard and registerCloseHandler; handler slot is a single field so a second registration silently replaces the first. |
| src/renderer/features/tasks/view/unified-main-tab-bar.tsx | One-line change wiring onClose to closeTabWithGuard instead of closeTab; correct and safe. |
| src/renderer/app/modal-registry.ts | Registers UnsavedChangesDialog in the modal registry at xs size; straightforward addition. |
Sequence Diagram
sequenceDiagram
participant User
participant TabBar as UnifiedMainTabBar
participant TM as TabManagerStore
participant FMLS as FileModelLifecycleStore
participant Dialog as UnsavedChangesDialog
participant Registry as modelRegistry
User->>TabBar: click x on tab
TabBar->>TM: closeTabWithGuard(tabId)
TM->>FMLS: _closeHandler(tabId)
FMLS->>Registry: isDirty(uri)?
alt file is dirty
FMLS->>Dialog: showModal (fileName)
alt User clicks Save
Dialog->>FMLS: onSuccess('save')
FMLS->>FMLS: saveFile(path) [errors swallowed]
FMLS->>TM: closeTab(tabId) [always, even if save failed]
else User clicks Discard
Dialog->>FMLS: onSuccess('discard')
FMLS->>TM: closeTab(tabId)
else User dismisses overlay
Dialog->>FMLS: onClose()
Note over FMLS,TM: resolve('cancel') - tab stays open
end
else file is clean or non-file tab
FMLS->>TM: closeTab(tabId)
end
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/renderer/lib/components/unsaved-changes-dialog.tsx:28-34
**Missing "Cancel" button leaves no explicit escape path**
The dialog has `showCloseButton={false}` and only "Discard" and "Save" actions. "Discard" closes the tab without saving — that is not the same as cancelling the close gesture. If a user clicks the X tab button by mistake, there is no button to abort the operation and keep the tab open. The `onClose` callback exists (and correctly resolves to `'cancel'`), so all that is missing is a Cancel button wired to `onClose`.
### Issue 2 of 3
src/renderer/features/tasks/editor/stores/file-model-lifecycle-store.ts:229-232
**Save failure silently proceeds to tab close**
`saveFile` swallows all errors internally and always resolves, so `savePromise.then(() => resolve('proceed'))` always fires even when the disk write failed. If saving fails (permission error, disk full, network drive gone, etc.), the tab closes anyway and the user loses their changes without any indication. The save result or error should be checked before resolving to `'proceed'`.
### Issue 3 of 3
src/renderer/features/tasks/tabs/tab-manager-store.ts:538-540
**Second `registerCloseHandler` call silently replaces the first**
Only a single `_closeHandler` slot exists; a second call overwrites the first without warning. While only `FileModelLifecycleStore` registers a handler today, any future lifecycle store or plugin that needs to participate in close coordination will silently clobber the previous handler, leading to missed save-guards or missed cleanup. Consider composing handlers (e.g. an array) or at least asserting that no handler is already registered.
Reviews (1): Last reviewed commit: "feat: add file save confirmation on tab ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| </DialogContentArea> | ||
| <DialogFooter> | ||
| <Button variant="outline" onClick={() => onSuccess('discard')}> | ||
| Discard | ||
| </Button> | ||
| <Button onClick={() => onSuccess('save')}>Save</Button> | ||
| </DialogFooter> |
There was a problem hiding this comment.
The dialog has showCloseButton={false} and only "Discard" and "Save" actions. "Discard" closes the tab without saving — that is not the same as cancelling the close gesture. If a user clicks the X tab button by mistake, there is no button to abort the operation and keep the tab open. The onClose callback exists (and correctly resolves to 'cancel'), so all that is missing is a Cancel button wired to onClose.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/lib/components/unsaved-changes-dialog.tsx
Line: 28-34
Comment:
**Missing "Cancel" button leaves no explicit escape path**
The dialog has `showCloseButton={false}` and only "Discard" and "Save" actions. "Discard" closes the tab without saving — that is not the same as cancelling the close gesture. If a user clicks the X tab button by mistake, there is no button to abort the operation and keep the tab open. The `onClose` callback exists (and correctly resolves to `'cancel'`), so all that is missing is a Cancel button wired to `onClose`.
How can I resolve this? If you propose a fix, please make it concise.| onSuccess: (result) => { | ||
| const savePromise = result === 'save' ? this.saveFile(path) : Promise.resolve(); | ||
| void savePromise.then(() => resolve('proceed')); | ||
| }, |
There was a problem hiding this comment.
saveFile swallows all errors internally and always resolves, so savePromise.then(() => resolve('proceed')) always fires even when the disk write failed. If saving fails (permission error, disk full, network drive gone, etc.), the tab closes anyway and the user loses their changes without any indication. The save result or error should be checked before resolving to 'proceed'.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/editor/stores/file-model-lifecycle-store.ts
Line: 229-232
Comment:
**Save failure silently proceeds to tab close**
`saveFile` swallows all errors internally and always resolves, so `savePromise.then(() => resolve('proceed'))` always fires even when the disk write failed. If saving fails (permission error, disk full, network drive gone, etc.), the tab closes anyway and the user loses their changes without any indication. The save result or error should be checked before resolving to `'proceed'`.
How can I resolve this? If you propose a fix, please make it concise.| registerCloseHandler(handler: (tabId: string) => Promise<void>): void { | ||
| this._closeHandler = handler; | ||
| } |
There was a problem hiding this comment.
registerCloseHandler call silently replaces the first
Only a single _closeHandler slot exists; a second call overwrites the first without warning. While only FileModelLifecycleStore registers a handler today, any future lifecycle store or plugin that needs to participate in close coordination will silently clobber the previous handler, leading to missed save-guards or missed cleanup. Consider composing handlers (e.g. an array) or at least asserting that no handler is already registered.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/tabs/tab-manager-store.ts
Line: 538-540
Comment:
**Second `registerCloseHandler` call silently replaces the first**
Only a single `_closeHandler` slot exists; a second call overwrites the first without warning. While only `FileModelLifecycleStore` registers a handler today, any future lifecycle store or plugin that needs to participate in close coordination will silently clobber the previous handler, leading to missed save-guards or missed cleanup. Consider composing handlers (e.g. an array) or at least asserting that no handler is already registered.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
No description provided.