Improve crop#100
Conversation
|
Warning Review limit reached
More reviews will be available in 24 minutes and 50 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR adds full ChangesStack End Index Tracking
Crop Overlay Viewport-Space Refactor
Minor Standalone Fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@codex review |
![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: 0daad9f919
ℹ️ 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".
| cropRotationPivotX = ((box[0] + box[2]) / 2000) | ||
| cropRotationPivotY = ((box[1] + box[3]) / 2000) |
There was a problem hiding this comment.
When the crop box is off-center and the user straightens, moving the QML rotation origin to the crop center makes the image rotate around a different point than the saved render: execute_crop ultimately uses the editor mapping that rotates content around the source center (_rotated_content_point subtracts src_w / 2 and src_h / 2). The fixed viewport crop frame will therefore preview different pixels than the crop that gets committed, so off-center straighten+crop can save the wrong region.
Useful? React with 👍 / 👎.
| meta = self.sidecar.get_metadata( | ||
| img.path, | ||
| create=False, | ||
| migrate=False, | ||
| ) |
There was a problem hiding this comment.
For folders with older sidecars that still store uploaded flags under legacy keys, this lookup now returns None instead of letting get_metadata migrate/find the entry, so “Jump to Last Uploaded” can report no upload or jump to an earlier image even though an uploaded image exists. The sidecar API reserves migrate=False for bulk grid-style reads, while this is a user action that depends on accurate flags.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
faststack/app.py (1)
7805-7806:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInsert partial rollbacks at shifted positions.
_rollback_ui_items()can receive only failed items while earlier deleted items remain removed. In that case inserting at the originalidxplaces restored items too far right, while the stack/batch restoration below shifts indices as if the list were compressed.🐛 Proposed fix
- for idx, img in sorted(items, key=lambda x: x[0]): - self.image_files.insert(min(idx, len(self.image_files)), img) + restored_original_indices = {idx for idx, _ in items} + still_deleted = sorted( + {idx for idx, _ in job.removed_items} - restored_original_indices + ) + for idx, img in sorted(items, key=lambda x: x[0]): + insert_idx = idx - sum(1 for d in still_deleted if d < idx) + self.image_files.insert(min(insert_idx, len(self.image_files)), img)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faststack/app.py` around lines 7805 - 7806, In the _rollback_ui_items() method around the loop that processes sorted items, the issue is that inserting at the original idx position doesn't account for items that may have already been deleted from earlier rollbacks. Instead of inserting directly at min(idx, len(self.image_files)), calculate the effective insertion position by counting how many items with indices less than idx are still missing from self.image_files, then subtract that count from idx to get the correct shifted position where the item should be reinserted into the current compressed list state.faststack/qml/Components.qml (2)
906-914:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop stale rotation timer updates when resetting crop state.
resetCropRotation()is used from index/source/crop-exit paths, but a runningrotationThrottleTimercan still callset_straighten_angle(pendingRotation, ...)after the reset. Stop the queued update in the reset helper so a stale angle cannot be applied to the next crop/current image.Proposed fix
function resetCropRotation() { + clearPendingRotation(0) cropRotation = 0 cropRotationPivotX = 0.5 cropRotationPivotY = 0.5 }Also applies to: 927-931
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faststack/qml/Components.qml` around lines 906 - 914, The rotationThrottleTimer can continue executing after resetCropRotation() is called, causing stale rotation updates to be applied. In the resetCropRotation() function, explicitly stop the rotationThrottleTimer by calling its stop() method to prevent the onTriggered handler from executing any pending set_straighten_angle() calls after the crop state has been reset. This ensures stale angle values cannot be applied to the next crop or current image.
1109-1147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t skip crop hit-testing after a non-knob click in rotate mode.
The
else if (isFullImage)binds to the rotate-knobif. When rotate mode is active but the click misses the knob, no crop branch setscropDragMode, thenbeginCropInteraction()starts a drag with"none"or stale state. Make the normal crop hit-test run after the optional knob check unless the knob path returned.Proposed fix
- // If crop box is full image, always start a new crop - else if (isFullImage) { + // If crop box is full image, always start a new crop + if (isFullImage) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faststack/qml/Components.qml` around lines 1109 - 1147, The normal crop hit-testing logic starting with the `else if (isFullImage)` check is incorrectly nested as an `else` branch of the rotate-knob check in the `if (mainMouseArea.isRotating && cropOverlay.visible && rotateKnob.visible)` block. This causes the normal crop logic to be skipped entirely when rotate mode is active but the knob is not clicked. Move the `else if (isFullImage)` block and the subsequent `else if (inside)` checks outside of the `else` relationship so they execute independently after the rotate-knob check completes, unless the rotate-knob path has already returned. This ensures crop hit-testing always runs when a knob click is missed in rotate mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@faststack/qml/Components.qml`:
- Around line 619-623: The aspect ratio constraint in
applyAspectRatioConstraint() is not being adjusted when dimensions are swapped
by _dimensionsAreSwapped(). When rectW and rectH are swapped in the
cropViewRectForBox() method (and also in the location referenced at lines
1456-1460), you need to also swap the aspect ratio constraint values (such as
the pixelAspectRatio or related aspect ratio variables) to ensure the fixed
aspect ratio is maintained after the dimension swap. This prevents a 16:9 aspect
ratio from inverting to 9:16 when _dimensionsAreSwapped() becomes true. Apply
this fix wherever dimensions are being swapped and aspect ratio constraints are
involved.
---
Outside diff comments:
In `@faststack/app.py`:
- Around line 7805-7806: In the _rollback_ui_items() method around the loop that
processes sorted items, the issue is that inserting at the original idx position
doesn't account for items that may have already been deleted from earlier
rollbacks. Instead of inserting directly at min(idx, len(self.image_files)),
calculate the effective insertion position by counting how many items with
indices less than idx are still missing from self.image_files, then subtract
that count from idx to get the correct shifted position where the item should be
reinserted into the current compressed list state.
In `@faststack/qml/Components.qml`:
- Around line 906-914: The rotationThrottleTimer can continue executing after
resetCropRotation() is called, causing stale rotation updates to be applied. In
the resetCropRotation() function, explicitly stop the rotationThrottleTimer by
calling its stop() method to prevent the onTriggered handler from executing any
pending set_straighten_angle() calls after the crop state has been reset. This
ensures stale angle values cannot be applied to the next crop or current image.
- Around line 1109-1147: The normal crop hit-testing logic starting with the
`else if (isFullImage)` check is incorrectly nested as an `else` branch of the
rotate-knob check in the `if (mainMouseArea.isRotating && cropOverlay.visible &&
rotateKnob.visible)` block. This causes the normal crop logic to be skipped
entirely when rotate mode is active but the knob is not clicked. Move the `else
if (isFullImage)` block and the subsequent `else if (inside)` checks outside of
the `else` relationship so they execute independently after the rotate-knob
check completes, unless the rotate-knob path has already returned. This ensures
crop hit-testing always runs when a knob click is missed in rotate mode.
🪄 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: 857659ed-8b0a-4709-bf72-ef8034cc0c8a
📒 Files selected for processing (5)
faststack/app.pyfaststack/deletion_types.pyfaststack/qml/Components.qmlfaststack/qml/Main.qmlfaststack/updater.py
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation