Skip to content

fix(app): keep window open when a chosen save does not complete#2361

Open
wkentaro wants to merge 1 commit into
mainfrom
fix/can-continue-discards-on-failed-save
Open

fix(app): keep window open when a chosen save does not complete#2361
wkentaro wants to merge 1 commit into
mainfrom
fix/can-continue-discards-on-failed-save

Conversation

@wkentaro

Copy link
Copy Markdown
Owner

When the unsaved-changes prompt appears (on close or file-switch) and the user chooses Save, the app previously proceeded unconditionally — even when the save never actually happened. _can_continue() returned True right after calling _save_label_file() without checking whether the file was written, so the caller went on to discard the in-memory annotations.

_save_label_file() only clears the dirty flag (mark_clean()) on a successful save_labels(). It silently no-ops when the Save-As dialog is cancelled (empty path) and returns without clearing on a write failure (LabelFileError). In both cases the annotations were lost even though the user asked to save them.

The fix returns the real dirty state (return not self._is_changed), so a save that did not complete aborts the close/switch and keeps the window open with the work intact.

Test plan

  • test_close_choose_save_but_cancel_save_dialog_keeps_window_open — Save chosen, Save-As dialog cancelled: window stays open, still dirty, no file written
  • test_close_choose_save_but_write_fails_keeps_window_open — Save chosen, save_labels fails: window stays open, still dirty
  • both new tests fail on the pre-fix return True and pass with the fix
  • full suite green: uv run pytest (809 passed)

@wkentaro

Copy link
Copy Markdown
Owner Author

This was generated by AI during PR processing.

Verdict: recommend-merge

A tight, correct fix. _can_continue() previously returned True unconditionally after choosing Save, telling every caller (closeEvent, dropEvent, close_file, the open-file/open-dir/file-list/import paths) it was safe to proceed even when the save never completed, silently discarding the annotations. The change return not self._is_changed proceeds only when the file actually ended up clean.

Verification:

  • Logic traced: _save_label_file() clears _is_changed (via mark_clean()) only when save_labels() succeeds; a cancelled Save-As dialog (empty path) and a failed write both leave _is_changed True, so the window correctly stays open. The top-of-method if not self._is_changed: return True guard means the Save branch is only reachable when dirty, so there is no stale-flag edge.
  • The two new @pytest.mark.gui e2e tests (cancelled dialog, failed write) were confirmed to fail against the old return True code and pass with the fix; all 7 tests in the file pass.
  • Branch is up to date with main (0 commits behind), MERGEABLE/CLEAN, all 11 CI checks green. No rebase or push this tick, so the existing green is authoritative.

follow-ups: #2362 (pre-existing: closeEvent persists window geometry even on an aborted close — does not block this merge).

@wkentaro wkentaro added the recommend-merge pr: Agent finalized and endorses it: review and merge label Jul 17, 2026
_can_continue() returned True unconditionally after the user chose Save
in the unsaved-changes prompt, even when _save_label_file() did not
actually persist the file (a cancelled Save-As dialog, or a failed
write). The caller then discarded the in-memory annotations. Return the
real dirty state instead so a save that did not clear the flag aborts
the close/switch and keeps the annotations.
@wkentaro
wkentaro force-pushed the fix/can-continue-discards-on-failed-save branch from e1bb1ae to dd113a2 Compare July 18, 2026 11:54
@wkentaro

Copy link
Copy Markdown
Owner Author

This was generated by AI during PR processing.

Re-affirming recommend-merge. This branch was rebased onto current main (73d25ef) in a bulk refresh, which is why the previous verdict read as stale (its commit post-dates the earlier verdict comment). The rebase was mechanical: the branch is now a single commit on main HEAD, the fix content is unchanged from the earlier review, and CI is green across all 11 configs (lint + 3 OS x Python 3.12/3.13/3.14). No rebase or push needed this tick; the prior verdict stands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

recommend-merge pr: Agent finalized and endorses it: review and merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant