Improve editor reuse safety and harden crop/save session handling (thanks to DJShortCut for these suggestions!)#66
Merged
AlanRockefeller merged 6 commits intomainfrom Apr 8, 2026
Merged
Conversation
include _edits_rev in save session tokens and _on_save_finished() comparisons so editor state is preserved if the user keeps editing while a save is in flight, while normal post-save cleanup still runs when the revision is unchanged prevent load_image_for_editing() from reusing a matched preview-only session when float_image is missing, forcing a full reload for flows that need the master buffer harden toggle_crop_mode() entry by requiring a valid current image, reusing _block_if_saving() for consistent save-in-flight behavior, and ensuring the editor is loaded before enabling crop mode add targeted reopening tests covering: forced reload when float_image is missing crop mode blocked while saving crop mode blocked when no image is selected editor state preserved when _edits_rev advances during save normal editor clear when _edits_rev is unchanged
WalkthroughSave tokens now include the editor's edit-revision (_edits_rev) and _on_save_finished() requires a matching revision before clearing editor state; load_image_for_editing() refuses reuse when a matched session lacks a float_image; crop mode entry/exit was rewritten to validate save/load state and explicitly reset backend/frontend crop state. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant App as App/Controller
participant Editor as ImageEditor
participant Storage as Save/Worker
UI->>App: toggle_crop_mode()
alt exiting crop
App->>Editor: set_crop_box(None)
App->>Editor: reset straighten_angle
App->>Editor: kick preview worker
App->>UI: update_status_message("Exited crop")
else entering crop
App->>App: verify image key not in _saving_keys
App->>Editor: load_image_for_editing()
alt load success
App->>Editor: set ui_state.isCropping = True
App->>Editor: set_crop_box(None)
App->>Editor: reset straighten_angle
App->>Editor: kick preview worker
App->>UI: update_status_message("Crop ready")
else load failure or no image
App->>UI: update_status_message("No image to crop")
end
end
UI->>App: save_edited_image()
App->>Storage: submit save (session_token includes _edits_rev)
Storage-->>App: save finished
App->>App: _on_save_finished(reconstruct token with current _edits_rev)
alt token matches current _edits_rev
App->>Editor: clear()
else token revision mismatch
Note right of App: do not clear editor state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
faststack/tests/test_editor_reopening.py (1)
222-239: Consider adding assertion for truthy result.The test verifies
load_imagewas called and result is not_REUSED, but doesn't explicitly assert the result is truthy (indicating successful load). Whileload_imageis mocked to returnTrue, addingself.assertTrue(res)would make the test's intent clearer and catch regressions if the return path changes.Suggested improvement
# Must perform a real reload, not _REUSED self.controller.image_editor.load_image.assert_called_once() + self.assertTrue(res) # Load succeeded self.assertIsNot(res, AppController._REUSED)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/test_editor_reopening.py` around lines 222 - 239, The test test_reuse_blocked_when_float_image_is_none should explicitly assert the returned result is truthy; after calling res = self.controller.load_image_for_editing() add an assertion that res is True (e.g. self.assertTrue(res)) to make the intent clear and ensure the successful load path (load_image) actually yields a truthy result rather than just asserting load_image was called and res is not AppController._REUSED.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/app.py`:
- Around line 6849-6872: The UI branch that enters/exits crop mode updates
ui_state.currentCropBox but does not clear the backend edit state, so a stale
crop survives in image_editor.current_edits and can be saved later; fix by
clearing or resetting the editor's crop param whenever toggling crop mode (both
when cancelling and when starting): call image_editor.set_edit_param("crop_box",
None) or set a full-frame box (matching the reset used for
ui_state.currentCropBox) and ensure image_editor.current_edits no longer
contains a lingering "crop_box" entry; update the cancel branch alongside the
code path that sets ui_state.isCropping and the path after
load_image_for_editing() to keep UI and backend edits in sync.
---
Nitpick comments:
In `@faststack/tests/test_editor_reopening.py`:
- Around line 222-239: The test test_reuse_blocked_when_float_image_is_none
should explicitly assert the returned result is truthy; after calling res =
self.controller.load_image_for_editing() add an assertion that res is True (e.g.
self.assertTrue(res)) to make the intent clear and ensure the successful load
path (load_image) actually yields a truthy result rather than just asserting
load_image was called and res is not AppController._REUSED.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b07124d0-d71c-4a36-8106-9c8d389a733f
📒 Files selected for processing (2)
faststack/app.pyfaststack/tests/test_editor_reopening.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
faststack/app.py (1)
6852-6855:⚠️ Potential issue | 🟠 MajorMirror this backend crop reset into
cancel_crop_mode().This toggle path now clears the backend crop, but the Escape path still goes through
cancel_crop_mode()at Lines 6833-6843 and leavesimage_editor.current_edits["crop_box"]intact. A later normal save can still export that stale crop even though the UI is back at a full-frame box.Suggested fix
def cancel_crop_mode(self): """Cancel crop mode without applying changes.""" if self.ui_state.isCropping: self.ui_state.isCropping = False self.ui_state.currentCropBox = [0, 0, 1000, 1000] + self.image_editor.set_crop_box(None) # Ensure preview rotation is cleared self.image_editor.set_edit_param("straighten_angle", 0.0) # Force QML to refresh if it's showing provider preview frames self.ui_refresh_generation += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 6852 - 6855, The cancel_crop_mode() path currently does not clear the backend crop state, leaving image_editor.current_edits["crop_box"] stale; update cancel_crop_mode() to mirror the toggle logic by calling image_editor.set_crop_box(None) and image_editor.set_edit_param("straighten_angle", 0.0) (and optionally update_status_message("Crop cancelled")) so the backend crop and preview rotation are cleared whenever cancel_crop_mode() is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/app.py`:
- Around line 6869-6881: When load_image_for_editing() returns a reused session,
resetting backend state via image_editor.set_crop_box(None) and
image_editor.set_edit_param("straighten_angle", 0.0) can leave the displayed
frame stale; after those calls (and after _sync_editor_state_from_session() if
it was used), explicitly kick a fresh preview/render so the UI shows the reset
state (i.e., call the class's preview scheduling/render method right after the
resets — e.g., self.schedule_preview() or self.render_preview(), whichever the
codebase uses — so the crop mode is opened on the newly-reset frame).
---
Duplicate comments:
In `@faststack/app.py`:
- Around line 6852-6855: The cancel_crop_mode() path currently does not clear
the backend crop state, leaving image_editor.current_edits["crop_box"] stale;
update cancel_crop_mode() to mirror the toggle logic by calling
image_editor.set_crop_box(None) and
image_editor.set_edit_param("straighten_angle", 0.0) (and optionally
update_status_message("Crop cancelled")) so the backend crop and preview
rotation are cleared whenever cancel_crop_mode() is invoked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f14e9e26-fcf5-49d4-9657-6b861a3ee76c
📒 Files selected for processing (2)
faststack/app.pyfaststack/tests/test_editor_reopening.py
✅ Files skipped from review due to trivial changes (1)
- faststack/tests/test_editor_reopening.py
…er() after resetting the backend crop and straighten angle parameters
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/app.py (1)
1773-1835:⚠️ Potential issue | 🟡 MinorRefresh variant state even when only
_edits_revchanged.With the new token shape, “same image/session, newer edits” now falls into the same branch as “user navigated away.” That branch never calls
refresh_image_list(), so the new backup is missing from_variant_mapeven thoughvariantBadgesChangedis emitted right after. Consider splitting “same session, different revision” from true navigation so you keep the live editor session but still refresh backup/variant state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 1773 - 1835, The save logic treats any mismatch in the new current_session_token (including only a changed _edits_rev) as "navigated away" and skips refresh_image_list(), so backups created by a live editor revision don't update _variant_map; change the branch logic to detect "same image/session but different _edits_rev" (compare current_image_key, view_override_kind, and image_editor.session_id for equality while allowing _edits_rev to differ) and in that case call refresh_image_list() and update the variant state (but do NOT close or clear the editor/state), otherwise keep the existing "navigated away" behavior (pop_path on saved_path). Update references: current_session_token, save_session_token, _edits_rev, refresh_image_list, image_editor, and editor_was_open.
♻️ Duplicate comments (1)
faststack/app.py (1)
6848-6882:⚠️ Potential issue | 🟠 MajorUse one shared crop-cancel path.
toggle_crop_mode()now clears backend crop state, but Escape still goes throughcancel_crop_mode(), and that method still does not callset_crop_box(None)or kick a fresh preview. Canceling with Esc can therefore leave a stalecrop_boxbehind and save/reopen the wrong frame.Suggested direction
+ def _exit_crop_mode(self, message: str = "Crop cancelled"): + self.ui_state.isCropping = False + self.ui_state.currentCropBox = (0, 0, 1000, 1000) + self.image_editor.set_crop_box(None) + self.image_editor.set_edit_param("straighten_angle", 0.0) + self._kick_preview_worker() + self.update_status_message(message) + `@Slot`() def cancel_crop_mode(self): - """Cancel crop mode without applying changes.""" - if self.ui_state.isCropping: - self.ui_state.isCropping = False - self.ui_state.currentCropBox = [0, 0, 1000, 1000] - # Ensure preview rotation is cleared - self.image_editor.set_edit_param("straighten_angle", 0.0) - # Force QML to refresh if it's showing provider preview frames - self.ui_refresh_generation += 1 - self.ui_state.currentImageSourceChanged.emit() - self.update_status_message("Crop cancelled") + """Cancel crop mode without applying changes.""" + if self.ui_state.isCropping: + self._exit_crop_mode() `@Slot`() def toggle_crop_mode(self): """Toggle crop mode on/off.""" if self.ui_state.isCropping: - # Exiting crop mode: cleanup - self.ui_state.isCropping = False - self.ui_state.currentCropBox = (0, 0, 1000, 1000) - # Ensure backend crop state and preview rotation are cleared when exiting - self.image_editor.set_crop_box(None) - self.image_editor.set_edit_param("straighten_angle", 0.0) - self._kick_preview_worker() - self.update_status_message("Crop cancelled") + self._exit_crop_mode()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 6848 - 6882, toggle_crop_mode() performs backend cleanup (calls image_editor.set_crop_box(None) and _kick_preview_worker()) when exiting crop mode but cancel_crop_mode() does not, leaving stale crop state; consolidate the cancel path by moving the cleanup into a single helper or by making cancel_crop_mode() call the same cleanup sequence used in toggle_crop_mode(): clear ui_state.isCropping and ui_state.currentCropBox, call image_editor.set_crop_box(None), reset straighten angle via image_editor.set_edit_param("straighten_angle", 0.0), and call _kick_preview_worker(); update references to ui_state.currentAspectRatioIndex/aspectRatioNames only when entering crop mode and ensure cancel_crop_mode() mirrors the exact exit behavior so Escape and toggle both clear backend crop state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@faststack/app.py`:
- Around line 1773-1835: The save logic treats any mismatch in the new
current_session_token (including only a changed _edits_rev) as "navigated away"
and skips refresh_image_list(), so backups created by a live editor revision
don't update _variant_map; change the branch logic to detect "same image/session
but different _edits_rev" (compare current_image_key, view_override_kind, and
image_editor.session_id for equality while allowing _edits_rev to differ) and in
that case call refresh_image_list() and update the variant state (but do NOT
close or clear the editor/state), otherwise keep the existing "navigated away"
behavior (pop_path on saved_path). Update references: current_session_token,
save_session_token, _edits_rev, refresh_image_list, image_editor, and
editor_was_open.
---
Duplicate comments:
In `@faststack/app.py`:
- Around line 6848-6882: toggle_crop_mode() performs backend cleanup (calls
image_editor.set_crop_box(None) and _kick_preview_worker()) when exiting crop
mode but cancel_crop_mode() does not, leaving stale crop state; consolidate the
cancel path by moving the cleanup into a single helper or by making
cancel_crop_mode() call the same cleanup sequence used in toggle_crop_mode():
clear ui_state.isCropping and ui_state.currentCropBox, call
image_editor.set_crop_box(None), reset straighten angle via
image_editor.set_edit_param("straighten_angle", 0.0), and call
_kick_preview_worker(); update references to
ui_state.currentAspectRatioIndex/aspectRatioNames only when entering crop mode
and ensure cancel_crop_mode() mirrors the exact exit behavior so Escape and
toggle both clear backend crop state.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/app.py`:
- Around line 6848-6854: When clearing the backend rotation via
image_editor.set_edit_param("straighten_angle", 0.0) in these branches, also
update the UI-side crop/rotation state so the overlay/slider doesn't remain
stale; call the existing _reset_crop_settings() (or if you prefer explicit
updates, set the relevant ui_state fields the same way) before incrementing
ui_refresh_generation and calling _kick_preview_worker(), so
ui_state.currentCropBox and the straighten angle shown in the UI are kept in
sync with the image_editor change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+6848
to
+6854
| self.ui_state.currentCropBox = (0, 0, 1000, 1000) | ||
| # Ensure backend crop state and preview rotation are cleared | ||
| self.image_editor.set_crop_box(None) | ||
| self.image_editor.set_edit_param("straighten_angle", 0.0) | ||
| # Force QML to refresh if it's showing provider preview frames | ||
| # Notify UI and kick fresh render | ||
| self.ui_refresh_generation += 1 | ||
| self.ui_state.currentImageSourceChanged.emit() | ||
| self._kick_preview_worker() |
There was a problem hiding this comment.
Keep the crop UI state in sync when these branches zero out rotation.
Line 6851 and Line 6887 reset image_editor's straighten_angle, but they do not reset the corresponding ui_state fields. That means the preview can snap back to 0° while the crop overlay/slider still shows the old angle until some other sync path runs. _reset_crop_settings() already handles this UI-side reset, so crop enter/cancel should do the same here.
🔧 Proposed fix
if self.ui_state.isCropping:
self.ui_state.isCropping = False
self.ui_state.currentCropBox = (0, 0, 1000, 1000)
# Ensure backend crop state and preview rotation are cleared
self.image_editor.set_crop_box(None)
self.image_editor.set_edit_param("straighten_angle", 0.0)
+ if hasattr(self.ui_state, "straighten_angle"):
+ self.ui_state.straighten_angle = 0.0
+ if hasattr(self.ui_state, "cropRotation"):
+ self.ui_state.cropRotation = 0.0
# Notify UI and kick fresh render
self.ui_refresh_generation += 1
self._kick_preview_worker()
self.update_status_message("Crop cancelled")
@@
self.ui_state.aspectRatioNames = [r["name"] for r in ASPECT_RATIOS]
self.ui_state.currentAspectRatioIndex = 0
# Reset rotation to 0 when starting fresh crop mode
self.image_editor.set_edit_param("straighten_angle", 0.0)
+ if hasattr(self.ui_state, "straighten_angle"):
+ self.ui_state.straighten_angle = 0.0
+ if hasattr(self.ui_state, "cropRotation"):
+ self.ui_state.cropRotation = 0.0
self._kick_preview_worker()
self.update_status_message("Crop mode: Drag to select area, Enter to crop")Also applies to: 6881-6888
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/app.py` around lines 6848 - 6854, When clearing the backend
rotation via image_editor.set_edit_param("straighten_angle", 0.0) in these
branches, also update the UI-side crop/rotation state so the overlay/slider
doesn't remain stale; call the existing _reset_crop_settings() (or if you prefer
explicit updates, set the relevant ui_state fields the same way) before
incrementing ui_refresh_generation and calling _kick_preview_worker(), so
ui_state.currentCropBox and the straighten angle shown in the UI are kept in
sync with the image_editor 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.
include _edits_rev in save session tokens and _on_save_finished()
comparisons so editor state is preserved if the user keeps editing
while a save is in flight, while normal post-save cleanup still runs
when the revision is unchanged
prevent load_image_for_editing() from reusing a matched preview-only
session when float_image is missing, forcing a full reload for flows
that need the master buffer
harden toggle_crop_mode() entry by requiring a valid current image,
reusing _block_if_saving() for consistent save-in-flight behavior, and
ensuring the editor is loaded before enabling crop mode
add targeted reopening tests covering:
forced reload when float_image is missing
crop mode blocked while saving
crop mode blocked when no image is selected
editor state preserved when _edits_rev advances during save
normal editor clear when _edits_rev is unchanged
Summary by CodeRabbit
Bug Fixes
Tests