Test#40
Merged
AlanRockefeller merged 5 commits intomainfrom Feb 7, 2026
Merged
Conversation
WalkthroughRelease bump to 1.5.5: adds a centralized deletion engine with undo/rollback and recycle/permanent-delete flows, background save offload with _saveFinished signaling, rotation/crop math fixes, adaptive highlight-recovery, UI/dialog enhancements, cache invalidation helpers, and extensive tests for deletion and recovery. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant EditorUI as ImageEditorDialog
participant AppCtrl as AppController
participant SaveExec as Background Executor
participant Handler as _on_save_finished
participant UI as UIState
User->>EditorUI: Click Save / press S
EditorUI->>AppCtrl: save_edited_image()
EditorUI->>UI: set isSaving = true
AppCtrl->>SaveExec: offload save task
SaveExec-->>AppCtrl: emit _saveFinished(result)
AppCtrl->>Handler: _on_save_finished(result)
Handler->>UI: set isSaving = false, refresh/select
Handler->>EditorUI: close editor (auto)
sequenceDiagram
participant User as User
participant GridUI as ThumbnailGridView
participant Loupe as LoupeView
participant AppCtrl as AppController
participant DeleteEngine as _delete_indices
participant Recycle as RecycleBin
participant UI as UIState
User->>GridUI: request delete (selection / cursor / batch)
GridUI->>AppCtrl: delete_current_image() / grid_delete_at_cursor() / delete_batch_images()
AppCtrl->>DeleteEngine: _delete_indices(indices, action_type)
DeleteEngine->>UI: optimistic sync_ui_state()
DeleteEngine->>Recycle: attempt move to recycle
alt all moved
DeleteEngine->>AppCtrl: schedule deferred refresh
AppCtrl->>UI: refresh_image_list()
else some failures
DeleteEngine->>User: prompt confirm_permanent_delete
alt user confirms
DeleteEngine->>Recycle: permanently_delete_image_files
else user cancels
DeleteEngine->>AppCtrl: rollback UI/model
end
end
DeleteEngine->>User: show status / undo info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 6
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)
1018-1068:⚠️ Potential issue | 🟠 MajorGate editor cleanup and image reselection to prevent disrupting in-progress edits.
When a user navigates to a different image during a background save,
_on_save_finishedshould not clear the new image's editor state or jump the selection back to the saved image. Currently,self.image_editor.clear()executes unconditionally (line 1031), destroying any edits in progress on the new image. Additionally, the image reselection logic (lines 1037–1055) attempts to find and select the originally saved image regardless of whether the user has moved on. Gate both of these operations oneditor_still_on_same_imageto preserve the new editing session.🛠️ Suggested fix
- # 2. Clear Editor State (release memory) - self.image_editor.clear() + # 2. Clear Editor State (release memory) only if still on initiating image + if editor_still_on_same_image: + self.image_editor.clear() - # 4. Find and Select the saved image - new_index = self.current_index # Default to keeping selection if not found - - # Try to find by exact path match - if saved_path: + # 4. Find and Select the saved image only if still on initiating image + new_index = self.current_index # Default to keeping selection if not found + if editor_still_on_same_image and saved_path: try: target_resolve = saved_path.resolve()
🤖 Fix all issues with AI agents
In @.gitignore:
- Line 92: The .gitignore entry "test*" is too broad and may exclude legitimate
files/directories starting with "test"; replace it with targeted patterns (for
example "test_output*", "test_temp*", "test_debug*") or explicitly ignore the
specific temp artifacts you intend to exclude, and if you need to ignore only
root-level artifacts consider anchoring or directory-specific patterns (e.g., a
dedicated pattern for the temp artifact names rather than the generic "test*").
In `@faststack/app.py`:
- Around line 2858-2925: The loop currently calls self._move_to_recycle for JPG
and RAW together which can orphan RAW if JPG fails; change the logic so you call
recycled_jpg = self._move_to_recycle(jpg_path) first, check its truthiness, and
only then call self._move_to_recycle(raw_path) (if raw_path exists); if you must
keep the existing order instead, ensure that any RAW moved while JPG failed is
undone by moving RAW back from the recycle bin and logging/marking the failure.
Update references in this block (functions/vars: _move_to_recycle, recycled_jpg,
recycled_raw, failed_recycles, successfully_deleted, delete_history,
undo_history) so RAW moves occur only after JPG success or are rolled back when
JPG fails.
- Around line 2817-2844: The in-memory deletion in _delete_indices removes
entries from self.image_files but leaves self._path_to_index stale until the
deferred refresh; call self._rebuild_path_to_index() immediately after the
in-memory removals and UI sync so the path->index map reflects the new list, and
also call self._rebuild_path_to_index() inside the rollback path where
removed_items are restored so the map is updated after restoration (reference
methods/attributes: _delete_indices, _rebuild_path_to_index, self.image_files,
self._path_to_index, removed_items, previous_index).
In `@faststack/imaging/editor.py`:
- Around line 647-648: The current code captures orig_h, orig_w but only
recenters the box after expand-rotation and fails to rotate the crop-box when
straighten_angle != 0; update the logic that computes the transformed crop
rectangle (used around the orig_h, orig_w calculation and duplicated in the
block covering lines ~702-742) to: compute the original image center from
orig_h, orig_w, create the four crop-box corner coordinates relative to that
center, apply a 2D rotation by straighten_angle to each corner, translate those
rotated corners into the expanded canvas coordinate system (using the same
translation used for the image expansion/recentering), then derive the final
crop bounds by taking min/max of the rotated-translated corner coordinates;
ensure this replaces the existing “recentering only” step and is used wherever
the code currently recenters the box after expand-rotation.
In `@faststack/qml/Main.qml`:
- Around line 1362-1372: The Delete and Quit button calls
uiState.cleanupRecycleBins() without checking for null; wrap the call to
uiState.cleanupRecycleBins() in a guard (if (uiState) { ... }) so the cleanup is
only invoked when uiState is defined, then proceed to set
allowCloseWithRecycleBins, call recycleBinCleanupDialog.close(), and Qt.quit()
as before; update the Button's onClicked handler to use this null-check around
the uiState call referencing uiState.cleanupRecycleBins().
In `@faststack/tests/test_deletion_unification.py`:
- Around line 184-201: The test currently mocks mock_controller._delete_indices
after calling mock_controller.delete_current_image(), making the "not called"
assertion meaningless; move the mock setup so mock_controller._delete_indices =
Mock() is assigned before invoking mock_controller.delete_current_image(), keep
the existing mock for mock_controller.get_batch_count_for_current_image and
mock_controller.main_window, then call delete_current_image and assert
mock_controller.main_window.show_delete_batch_dialog.assert_called_once_with(5)
and mock_controller._delete_indices.call_count == 0 to ensure deletion was
deferred to the dialog.
🧹 Nitpick comments (2)
faststack/tests/test_reactive_delete.py (1)
83-114: Remove unusedtmp_pathparameter.The
tmp_pathfixture parameter is not used in this test function—img_pathis derived fromapp_controller.image_dirinstead.Proposed fix
-def test_reactive_delete_fallback_cancelled(app_controller, tmp_path): +def test_reactive_delete_fallback_cancelled(app_controller):faststack/tests/test_loupe_delete.py (1)
77-82: Prefix unusedtsvariable with underscore.The timestamp variable is unpacked but not used in assertions. Prefix it with an underscore to indicate it's intentionally unused.
Proposed fix
- action, record, ts = mock_controller.undo_history[0] + action, record, _ts = mock_controller.undo_history[0]
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/tests/test_editor_integration.py (1)
88-93:⚠️ Potential issue | 🟠 Major
save_edited_imageis now async — this assertion is racy.
save_edited_image()was refactored to offload the actualsave_image()call to_save_executor(a realThreadPoolExecutor). The method returns immediately after submitting the task, sosave_image.assert_called_once()at line 91 may execute before the background thread invokessave_image.This test can intermittently pass or fail depending on thread scheduling. To fix, either:
- Mock
_save_executorso the work runs synchronously, or- Wait for the future to complete before asserting.
Option 1: Make save executor synchronous in tests
# 5. save_edited_image try: + # Mock executor to run synchronously for deterministic test + from unittest.mock import patch as _patch + with _patch.object(self.controller._save_executor, 'submit', side_effect=lambda fn, *a, **kw: (fn(), MagicMock())[1]) as mock_submit: + # Or simpler: just verify the method exists and the intent self.controller.save_edited_image() - self.controller.image_editor.save_image.assert_called_once() + # save_image is called asynchronously; for a unit test, verify + # that submit was called on the executor instead except AttributeError: self.fail("AppController is missing method 'save_edited_image'")A cleaner approach would be to inject/mock the
_save_executorduringsetUpwith a synchronous executor:from concurrent.futures import ThreadPoolExecutor # Replace with a single-threaded immediate executor or mock # so save_image runs synchronously in tests
🤖 Fix all issues with AI agents
In `@faststack/app.py`:
- Around line 2993-2999: The rollback loop inserts restored items using their
original indices but does so in ascending order which lets earlier insertions
shift later targets; change the loop that processes items_to_restore in the
delete/rollback code to iterate in descending original index order (i.e., sort
or reverse items_to_restore by idx before inserting) so each insertion doesn't
affect positions of yet-to-be-inserted items, still clamping insert_idx with
min(idx, len(self.image_files)); reference items_to_restore, self.image_files
and refresh_image_list() when making the change.
- Around line 2905-2910: The except block catching (OSError, shutil.Error) for
undoing the JPG move currently calls log.error which loses the traceback; change
that call to log.exception so the exception traceback is recorded while
preserving the same message about failing to undo JPG move for jpg_path.name
(the except block handling undo_err for jpg_path). Keep the existing message
text and variable usage, only replace log.error(...) with log.exception(...) to
match other exception logging in this file.
In `@faststack/repro_type_error.py`:
- Around line 4-6: Replace the hardcoded Windows path passed to sys.path.insert
with a computed repository root derived from the current file location: use
__file__ (via pathlib.Path(__file__).resolve().parent or .parents[n]) to find
the repo root and insert that string into sys.path with the existing
sys.path.insert call so the import path is portable across environments.
- Around line 15-26: The repro script uses a broad catch "except Exception as
e:" around the call to editor._apply_edits(img) which triggers BLE001; to
suppress this for the debugging harness, append a per-line flake8 suppression to
that except line (change "except Exception as e:" to "except Exception as e: #
noqa: BLE001") and ensure the try/except block around editor._apply_edits and
the debug prints covers the intended lines (the block containing the
print("Calling _apply_edits..."), the call to editor._apply_edits, the result
prints, and the except).
In `@faststack/tests/test_deletion_unification.py`:
- Around line 19-23: The mock_controller fixture currently accepts qapp but
never references it, triggering ARG001; inside the mock_controller function
(fixture name: mock_controller) add a no-op reference to qapp (for example
assign it to a throwaway variable or assert it) so the QApplication fixture is
retained and the linter sees qapp as used; keep the reference as a single line
near the top of the fixture so it has no runtime effect but satisfies Ruff.
🧹 Nitpick comments (3)
faststack/tests/test_editor_rotation.py (1)
270-272: Addstrict=tozip()for Ruff B905 compliance.
This keeps the helper assertions explicit about length matching.🔧 Suggested fix (apply to both helpers)
- assert all(abs(a - b) <= 1 for a, b in zip(c1, c2)), f"{msg}: {c1} != {c2}" + assert all(abs(a - b) <= 1 for a, b in zip(c1, c2, strict=True)), f"{msg}: {c1} != {c2}"Also applies to: 313-315
faststack/app.py (2)
946-1000: Background save offloading looks well-designed.The signal-based hop from the background thread to the main thread is correct for Qt, and the
_save_initiated_pathguard against "surprise close" on navigation is a nice touch. A couple of minor notes from static analysis:
- Line 984:
log.exception(f"Unexpected error during save: {e}")— the{e}is redundant sincelog.exceptionalready appends the traceback including the exception. Uselog.exception("Unexpected error during save")instead.- Line 980: Ruff TRY300 suggests moving the success return into an
elseblock of thetry, though this is a style nit.Minor cleanup
try: result = self.image_editor.save_image( write_developed_jpg=write_sidecar, developed_path=dev_path ) return {"success": True, "result": result} except RuntimeError as e: return {"success": False, "error": str(e)} except Exception as e: - log.exception(f"Unexpected error during save: {e}") + log.exception("Unexpected error during save") return {"success": False, "error": "Failed to save image"}
956-958: Double-save guard relies on UI state — consider a dedicated lock or flag.
self.ui_state.isSavingis a Qt property likely bound to QML. If the background thread hasn't finished but the UI flag is somehow toggled externally (e.g., QML binding reset), a second save could be submitted. A simpleself._save_in_progressboolean checked and set atomically would be more robust than relying on the UI property. That said, this is low-risk since only this method setsisSavingtoTrue.
Merged
This was referenced Mar 28, 2026
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 by CodeRabbit
New Features
Bug Fixes
UI Improvements
Documentation & Tests