Fix crop bug#71
Conversation
WalkthroughChanges across the codebase restructure post-save workflows with index-preservation logic, introduce sidecar metadata updates marking images as edited, enhance crop-drag UI stability through image source freezing and zoom/pan locking, refactor thumbnail rendering for folder items, and extend coverage statistics to track an "edited" image bucket alongside existing upload/stack/todo metrics. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f30ecc304
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if saved_path: | ||
| self.sidecar.update_metadata(saved_path, {"edited": True}) |
There was a problem hiding this comment.
_on_save_finished now unconditionally calls self.sidecar.update_metadata(saved_path, {"edited": True}), but save_edited_image explicitly allows users to keep navigating while the background save runs, and _switch_to_directory replaces self.sidecar when folders change. If a save started in folder A finishes after the user moved to folder B, this line writes an absolute-key metadata entry into folder B’s faststack.json instead of updating folder A, which corrupts sidecar data and leaves the actual saved image unmarked in its own folder.
Useful? React with 👍 / 👎.
| readonly property bool isCropSourceFrozen: cropDragImageSource !== "" && ((mainMouseArea && mainMouseArea.isCropDragging) || (uiState && uiState.isCropping)) | ||
| readonly property string displayedImageSource: isCropSourceFrozen ? cropDragImageSource : requestedImageSource |
There was a problem hiding this comment.
The new freeze condition keeps displayedImageSource pinned whenever uiState.isCropping is true, not just during drag. Because crop mode now snapshots cropDragImageSource on entry, mainImage.source stays stuck on that initial frame for the entire crop session, so later uiState.currentImageSource updates (for example high-res swaps after zooming) are ignored until crop mode exits. That causes stale previews during cropping and defeats the intended “freeze during gesture” behavior.
Useful? React with 👍 / 👎.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 2029-2031: The metadata update only sets {"edited": True} so
edited_date is missing and no previous metadata is captured for undo; change the
flow in the code that updates sidecar metadata (the block using
self.sidecar.update_metadata(saved_path, {"edited": True})) to: 1) read the
current metadata via self.sidecar.get_metadata(saved_path) into old_meta, 2)
create new_meta by merging old_meta with edited=True and a timestamp
(edited_date = now()), 3) call self.sidecar.update_metadata(saved_path,
new_meta), and 4) record the old_meta as an undoable operation via the app's
undo/backup mechanism (e.g., self.undo_manager.record_metadata_change or the
same backup routine used for image data) so operations like crop, auto_levels,
and auto_white_balance can fully revert metadata; apply the same change at the
other same-pattern sites (the blocks used by crop, auto_levels,
auto_white_balance).
- Around line 2055-2056: The code currently only logs unexpected save payloads
at the log.warning("Save finished with unexpected result shape: %r", result)
branch, which can leave the UI stuck on the "Saving..." indicator; update that
branch to not only log but also invoke the same completion/cleanup path used for
normal successes (e.g., call the success handler or emit the
save-complete/notify-save-done routine that clears the UI state and resets any
in-progress flags), ensuring the client is notified that the save finished even
when the payload shape is malformed.
In `@faststack/qml/Components.qml`:
- Around line 51-57: The onIsCroppingChanged handler currently only calls
loupeView.freezeCropImageSource() / releaseCropImageSource() and misses
unwinding the crop drag state and locks when uiState.isCropping becomes false;
update onIsCroppingChanged (the function that reacts to uiState.isCropping) so
that when uiState.isCropping flips to false it also ensures any ongoing crop
drag is terminated by calling the same cleanup used in the Escape/local flow
(e.g., invoke endCropInteraction() or equivalent), set uiState.isCropDragging =
false, and clear any zoom/pan locks so onReleased doesn’t skip cleanup and the
view is fully unlocked.
- Around line 658-664: beginNewCrop currently writes raw normalized coordinates
(mx,my) into uiState.currentCropBox and calls setCropBoxStart even when the
pointer is outside the image; clamp mx and my to the image bounds before using
them so values stay within [0,1000] (or your normalized range) — e.g. compute
clampedMx = Math.max(0, Math.min(1000, mx)) and clampedMy = Math.max(0,
Math.min(1000, my)), pass those to setCropBoxStart and store
Math.round(clampedMx), Math.round(clampedMy) in uiState.currentCropBox, while
still setting cropStartX/cropStartY from the original mouseX/mouseY.
- Around line 60-75: The escape handler can let a previously-armed
rotationThrottleTimer fire and reapply pendingRotation after you revert/cancel;
update Keys.onEscapePressed to cancel any queued rotation work by stopping
rotationThrottleTimer and clearing the pendingRotation value (or resetting it to
a neutral value) before calling
mainMouseArea.endCropInteraction()/controller.cancel_crop_mode() and resetting
cropRotation; reference rotationThrottleTimer, pendingRotation, mainMouseArea,
and the Keys.onEscapePressed block so the timer is stopped and pendingRotation
cleared in both the rotate-revert and cancel branches.
🪄 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: 4fa806cc-5801-4250-b694-6323f4bbee5c
📒 Files selected for processing (6)
.codexfaststack/app.pyfaststack/io/sidecar.pyfaststack/qml/Components.qmlfaststack/qml/ThumbnailTile.qmlfaststack/thumbnail_view/folder_stats.py
| # 1. Update sidecar metadata FIRST so all following refreshes see it | ||
| if saved_path: | ||
| self.sidecar.update_metadata(saved_path, {"edited": True}) |
There was a problem hiding this comment.
Persist the full edited metadata and make it undoable.
These writes only set edited=True. That leaves edited_date blank in the metadata UI, and the undoable flows here (crop, auto_levels, auto_white_balance) will still show the image as edited after the backup is restored because the previous metadata state is never captured/restored.
Also applies to: 7376-7377, 7621-7622, 7717-7718, 7866-7867
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/app.py` around lines 2029 - 2031, The metadata update only sets
{"edited": True} so edited_date is missing and no previous metadata is captured
for undo; change the flow in the code that updates sidecar metadata (the block
using self.sidecar.update_metadata(saved_path, {"edited": True})) to: 1) read
the current metadata via self.sidecar.get_metadata(saved_path) into old_meta, 2)
create new_meta by merging old_meta with edited=True and a timestamp
(edited_date = now()), 3) call self.sidecar.update_metadata(saved_path,
new_meta), and 4) record the old_meta as an undoable operation via the app's
undo/backup mechanism (e.g., self.undo_manager.record_metadata_change or the
same backup routine used for image data) so operations like crop, auto_levels,
and auto_white_balance can fully revert metadata; apply the same change at the
other same-pattern sites (the blocks used by crop, auto_levels,
auto_white_balance).
| # Success reported but result shape unexpected | ||
| log.warning("Save finished with unexpected result shape: %r", result) |
There was a problem hiding this comment.
Clear the stuck “Saving...” status on malformed success results.
This branch only logs, so the UI can remain on the no-timeout "Saving..." message forever when a save reports success with an unexpected payload.
Proposed fix
else:
# Success reported but result shape unexpected
log.warning("Save finished with unexpected result shape: %r", result)
+ self.update_status_message(
+ "Save finished, but the result could not be processed.",
+ timeout=5000,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/app.py` around lines 2055 - 2056, The code currently only logs
unexpected save payloads at the log.warning("Save finished with unexpected
result shape: %r", result) branch, which can leave the UI stuck on the
"Saving..." indicator; update that branch to not only log but also invoke the
same completion/cleanup path used for normal successes (e.g., call the success
handler or emit the save-complete/notify-save-done routine that clears the UI
state and resets any in-progress flags), ensuring the client is notified that
the save finished even when the payload shape is malformed.
| function onIsCroppingChanged() { | ||
| if (uiState && uiState.isCropping) { | ||
| loupeView.freezeCropImageSource() | ||
| } else { | ||
| loupeView.releaseCropImageSource() | ||
| } | ||
| } |
There was a problem hiding this comment.
Always unwind drag state when crop mode exits.
This handler releases the frozen source, but it does not clear isCropDragging or the zoom/pan locks. If uiState.isCropping flips to false from any path other than the local Escape flow, onReleased will skip endCropInteraction() and the view can stay permanently locked.
💡 Proposed fix
function onIsCroppingChanged() {
if (uiState && uiState.isCropping) {
loupeView.freezeCropImageSource()
} else {
+ if (mainMouseArea && mainMouseArea.isCropDragging) {
+ mainMouseArea.endCropInteraction()
+ }
loupeView.releaseCropImageSource()
}
}📝 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.
| function onIsCroppingChanged() { | |
| if (uiState && uiState.isCropping) { | |
| loupeView.freezeCropImageSource() | |
| } else { | |
| loupeView.releaseCropImageSource() | |
| } | |
| } | |
| function onIsCroppingChanged() { | |
| if (uiState && uiState.isCropping) { | |
| loupeView.freezeCropImageSource() | |
| } else { | |
| if (mainMouseArea && mainMouseArea.isCropDragging) { | |
| mainMouseArea.endCropInteraction() | |
| } | |
| loupeView.releaseCropImageSource() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/qml/Components.qml` around lines 51 - 57, The onIsCroppingChanged
handler currently only calls loupeView.freezeCropImageSource() /
releaseCropImageSource() and misses unwinding the crop drag state and locks when
uiState.isCropping becomes false; update onIsCroppingChanged (the function that
reacts to uiState.isCropping) so that when uiState.isCropping flips to false it
also ensures any ongoing crop drag is terminated by calling the same cleanup
used in the Escape/local flow (e.g., invoke endCropInteraction() or equivalent),
set uiState.isCropDragging = false, and clear any zoom/pan locks so onReleased
doesn’t skip cleanup and the view is fully unlocked.
| Keys.onEscapePressed: (event) => { | ||
| if (uiState && uiState.isCropping) { | ||
| if (mainMouseArea.isRotating) { | ||
| // Revert rotation | ||
| mainMouseArea.cropRotation = mainMouseArea.cropStartRotation | ||
| if (controller) controller.set_straighten_angle(mainMouseArea.cropRotation, -1) | ||
|
|
||
|
|
||
| mainMouseArea.endCropInteraction() | ||
| mainMouseArea.isRotating = false | ||
| mainMouseArea.cropDragMode = "none" | ||
| mainMouseArea.isCropDragging = false | ||
| event.accepted = true | ||
| } else if (controller) { | ||
| mainMouseArea.endCropInteraction() | ||
| controller.cancel_crop_mode() | ||
| mainMouseArea.cropRotation = 0 // Reset local rotation | ||
| event.accepted = true | ||
| } |
There was a problem hiding this comment.
Cancel queued rotation updates before handling Escape.
rotationThrottleTimer may still be armed from the last drag sample. If Escape is pressed before it fires, the callback can re-send the old pendingRotation after the revert/cancel path runs.
💡 Proposed fix
Keys.onEscapePressed: (event) => {
if (uiState && uiState.isCropping) {
+ rotationThrottleTimer.stop()
if (mainMouseArea.isRotating) {
// Revert rotation
mainMouseArea.cropRotation = mainMouseArea.cropStartRotation
if (controller) controller.set_straighten_angle(mainMouseArea.cropRotation, -1)📝 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.
| Keys.onEscapePressed: (event) => { | |
| if (uiState && uiState.isCropping) { | |
| if (mainMouseArea.isRotating) { | |
| // Revert rotation | |
| mainMouseArea.cropRotation = mainMouseArea.cropStartRotation | |
| if (controller) controller.set_straighten_angle(mainMouseArea.cropRotation, -1) | |
| mainMouseArea.endCropInteraction() | |
| mainMouseArea.isRotating = false | |
| mainMouseArea.cropDragMode = "none" | |
| mainMouseArea.isCropDragging = false | |
| event.accepted = true | |
| } else if (controller) { | |
| mainMouseArea.endCropInteraction() | |
| controller.cancel_crop_mode() | |
| mainMouseArea.cropRotation = 0 // Reset local rotation | |
| event.accepted = true | |
| } | |
| Keys.onEscapePressed: (event) => { | |
| if (uiState && uiState.isCropping) { | |
| rotationThrottleTimer.stop() | |
| if (mainMouseArea.isRotating) { | |
| // Revert rotation | |
| mainMouseArea.cropRotation = mainMouseArea.cropStartRotation | |
| if (controller) controller.set_straighten_angle(mainMouseArea.cropRotation, -1) | |
| mainMouseArea.endCropInteraction() | |
| mainMouseArea.isRotating = false | |
| event.accepted = true | |
| } else if (controller) { | |
| mainMouseArea.endCropInteraction() | |
| controller.cancel_crop_mode() | |
| mainMouseArea.cropRotation = 0 // Reset local rotation | |
| event.accepted = true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/qml/Components.qml` around lines 60 - 75, The escape handler can
let a previously-armed rotationThrottleTimer fire and reapply pendingRotation
after you revert/cancel; update Keys.onEscapePressed to cancel any queued
rotation work by stopping rotationThrottleTimer and clearing the pendingRotation
value (or resetting it to a neutral value) before calling
mainMouseArea.endCropInteraction()/controller.cancel_crop_mode() and resetting
cropRotation; reference rotationThrottleTimer, pendingRotation, mainMouseArea,
and the Keys.onEscapePressed block so the timer is stopped and pendingRotation
cleared in both the rotate-revert and cancel branches.
| function beginNewCrop(mouseX, mouseY, mx, my) { | ||
| cropDragMode = "new" | ||
| cropStartX = mouseX | ||
| cropStartY = mouseY | ||
| setCropBoxStart(mx, my, mx, my) | ||
| uiState.currentCropBox = [Math.round(mx), Math.round(my), Math.round(mx), Math.round(my)] | ||
| } |
There was a problem hiding this comment.
Clamp the initial crop box to image bounds.
beginNewCrop() stores raw normalized coordinates even when the press starts outside the image. A click-release in the viewport margin can leave uiState.currentCropBox with values below 0 or above 1000.
💡 Proposed fix
function beginNewCrop(mouseX, mouseY, mx, my) {
cropDragMode = "new"
cropStartX = mouseX
cropStartY = mouseY
+ mx = Math.max(0, Math.min(1000, mx))
+ my = Math.max(0, Math.min(1000, my))
setCropBoxStart(mx, my, mx, my)
uiState.currentCropBox = [Math.round(mx), Math.round(my), Math.round(mx), Math.round(my)]
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/qml/Components.qml` around lines 658 - 664, beginNewCrop currently
writes raw normalized coordinates (mx,my) into uiState.currentCropBox and calls
setCropBoxStart even when the pointer is outside the image; clamp mx and my to
the image bounds before using them so values stay within [0,1000] (or your
normalized range) — e.g. compute clampedMx = Math.max(0, Math.min(1000, mx)) and
clampedMy = Math.max(0, Math.min(1000, my)), pass those to setCropBoxStart and
store Math.round(clampedMx), Math.round(clampedMy) in uiState.currentCropBox,
while still setting cropStartX/cropStartY from the original mouseX/mouseY.
1 participant
Summary by CodeRabbit
New Features
Improvements