fix bug in batches where sometimes it would grab the wrong photo when uploading a batch of images#45
Merged
AlanRockefeller merged 10 commits intomainfrom Mar 6, 2026
Merged
fix bug in batches where sometimes it would grab the wrong photo when uploading a batch of images#45AlanRockefeller merged 10 commits intomainfrom
AlanRockefeller merged 10 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoved many test/repro/smoke artifacts and a pytest transcript; added per-image "todo" metadata, UI bindings, and keybinding; changed cache eviction callback semantics and eviction-time context; extended AppController deletion/rollback logic and added a safe executor shutdown helper. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI / QML
participant App as AppController
participant Cache as ByteLRUCache
participant Exec as ThreadPoolExecutor
participant Worker as WorkerThread
UI->>App: toggle_todo() or delete action
App->>Cache: mutate / __delitem__ (may trigger eviction)
Cache->>Cache: detect pressure, build eviction info
Cache->>App: call _on_cache_evict(key, value, info)
App->>Exec: submit DeleteJob (captures pre-mutation snapshots)
Exec->>Worker: run DeleteJob (may recompute batches)
Worker-->>App: report completion / enable rollback restore
sequenceDiagram
participant App as AppController
participant Safe as _safe_shutdown_executor
participant Exec as ThreadPoolExecutor
participant Log as Logger
App->>Safe: _safe_shutdown_executor(executor, name, wait?, cancel_futures?)
Safe->>Exec: executor.shutdown(wait=..., cancel_futures=...)
Exec->>Exec: cancel/finish pending work
Exec-->>Safe: return/raise
Safe->>Log: log shutdown outcome
Safe-->>App: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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: 741eeaa92b
ℹ️ 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".
Comment on lines
+4331
to
+4333
| if new_batches != self.batches: | ||
| self.batches = new_batches | ||
| self._invalidate_batch_cache() |
There was a problem hiding this comment.
This eagerly rewrites self.batches, but non-batch delete jobs never store a pre-delete batch snapshot (DeleteJob.saved_batches is only populated in delete_batch_images), so rollback paths restore image_files without restoring batch ranges. In practice, if a user has batches defined and performs a normal delete that is later undone/cancelled (or fails and is rolled back), batch indices remain shifted and subsequent batch operations can target the wrong photos.
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
🤖 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 4302-4334: Before mutating self.batches in the delete path, take a
snapshot (e.g., saved_batches and saved_batch_start_index) and use that snapshot
to restore self.batches if the delete is rolled back or undone; ensure the
mutation logic that builds new_batches (including _shift, first_ok/last_ok logic
and the call to self._invalidate_batch_cache()) writes only after the snapshot
is taken, and add restoration of saved_batches and saved_batch_start_index in
the pending_delete undo path so batch markers are re-aligned whenever
image_files are restored.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
faststack/all_verification_results.txtfaststack/all_verification_results_utf8.txtfaststack/app.pyfaststack/check_daemon.pyfaststack/check_scipy.pyfaststack/integration_results.txtfaststack/integration_traceback.txtfaststack/path_check.txtfaststack/repro_cache_lock.pyfaststack/repro_daemon_bug.pyfaststack/repro_imports.pyfaststack/repro_type_error.pyfaststack/rotation_error.txtfaststack/test_prespawn_strategy.pyfaststack/thumbnail_view/model.pyfaststack/traceback.txtfaststack/verify_cache_fix.pyfaststack/verify_wb.pyscripts/smoke_verify.py
💤 Files with no reviewable changes (11)
- faststack/check_scipy.py
- faststack/repro_imports.py
- faststack/verify_cache_fix.py
- scripts/smoke_verify.py
- faststack/repro_daemon_bug.py
- faststack/all_verification_results_utf8.txt
- faststack/repro_type_error.py
- faststack/test_prespawn_strategy.py
- faststack/check_daemon.py
- faststack/verify_wb.py
- faststack/repro_cache_lock.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
faststack/app.py (1)
4308-4338:⚠️ Potential issue | 🟠 MajorBatch reindexing is still not rollback-safe for non-batch deletes.
Line 4308 mutates
self.batchesoptimistically, but no snapshot is attached to theDeleteJobin_delete_indices. If the operation is undone/cancelled/fails,image_filescan be restored while batch ranges remain shifted, which can still target the wrong photos.🛠️ Proposed fix
@@ - # --- PHASE 1: OPTIMISTIC UI UPDATE (instant, no I/O) --- + # --- PHASE 1: OPTIMISTIC UI UPDATE (instant, no I/O) --- + # Snapshot batch state for rollback/undo before any mutation. + saved_batches = list(self.batches) if self.batches is not None else None + saved_batch_start_index = self.batch_start_index @@ - self._pending_delete_jobs[job_id] = DeleteJob( + self._pending_delete_jobs[job_id] = DeleteJob( job_id=job_id, removed_items=removed_items, action_type=action_type, timestamp=timestamp, cancel_event=cancel_event, previous_index=previous_index, images_to_delete=images_to_delete, ) + self._pending_delete_jobs[job_id].saved_batches = saved_batches + self._pending_delete_jobs[job_id].saved_batch_start_index = saved_batch_start_index@@ def undo_delete(self): if action_type == "pending_delete": job_id = action_data job = self._pending_delete_jobs.get(job_id) @@ self._rebuild_path_to_index() + + # Restore batch markers/ranges to pre-delete state. + if getattr(job, "saved_batches", None) is not None: + self.batches = job.saved_batches + self.batch_start_index = getattr(job, "saved_batch_start_index", None) + self._invalidate_batch_cache() + self.sync_ui_state()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 4308 - 4338, The batch reindexing currently mutates self.batches inside the delete flow which can leave batch ranges shifted if a non-batch delete is undone or fails; modify _delete_indices (and the DeleteJob it creates) to take and store a snapshot of the original self.batches before any mutation, perform the reindexing only after the delete has committed (or else apply the mutation in a transactional/commit hook), and on cancel/failure restore the saved snapshot back to self.batches and call self._invalidate_batch_cache(); reference self.batches, DeleteJob, _delete_indices, and _invalidate_batch_cache when locating and implementing this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@faststack/app.py`:
- Around line 4308-4338: The batch reindexing currently mutates self.batches
inside the delete flow which can leave batch ranges shifted if a non-batch
delete is undone or fails; modify _delete_indices (and the DeleteJob it creates)
to take and store a snapshot of the original self.batches before any mutation,
perform the reindexing only after the delete has committed (or else apply the
mutation in a transactional/commit hook), and on cancel/failure restore the
saved snapshot back to self.batches and call self._invalidate_batch_cache();
reference self.batches, DeleteJob, _delete_indices, and _invalidate_batch_cache
when locating and implementing this change.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
faststack/app.py (1)
4422-4423:⚠️ Potential issue | 🟠 MajorPending-delete undo still doesn’t restore saved batch state.
Line 4422 and Line 4423 persist batch snapshots, but the
"pending_delete"branch inundo_delete()restores onlyimage_files/current_index. If undo happens mid-flight, batches can stay shifted/cleared and later target the wrong images.🛠️ Suggested fix
@@ def undo_delete(self): self._rebuild_path_to_index() + if job.saved_batches is not None: + self.batches = [b[:] for b in job.saved_batches] + self.batch_start_index = job.saved_batch_start_index + self._invalidate_batch_cache() self.sync_ui_state()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 4422 - 4423, The "pending_delete" branch in undo_delete() only restores image_files and current_index but omits restoring the saved batch state (saved_batches/pre_batch_snapshot and saved_batch_start_index/pre_batch_start_snapshot), so undo mid-flight leaves batches shifted; update the pending_delete path in undo_delete() to also restore the saved_batches (use pre_batch_snapshot) and saved_batch_start_index (use pre_batch_start_snapshot), and ensure any batch-related pointers or offsets that depend on saved_batches/current_index are recalculated or reassigned so subsequent operations target the correct images.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@faststack/app.py`:
- Around line 4422-4423: The "pending_delete" branch in undo_delete() only
restores image_files and current_index but omits restoring the saved batch state
(saved_batches/pre_batch_snapshot and
saved_batch_start_index/pre_batch_start_snapshot), so undo mid-flight leaves
batches shifted; update the pending_delete path in undo_delete() to also restore
the saved_batches (use pre_batch_snapshot) and saved_batch_start_index (use
pre_batch_start_snapshot), and ensure any batch-related pointers or offsets that
depend on saved_batches/current_index are recalculated or reassigned so
subsequent operations target the correct images.
…ic from _rollback_ui_items but missed the batch restore block.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ChangeLog.md (1)
14-14: Nit: use more formal language consistent with the rest of the changelog.
"mess up"is colloquial compared to the surrounding entries. Consider replacing with"corrupt"or"invalidate".✏️ Proposed wording
-- Fixed a bug where deleting an image could mess up the batch selection ranges if the delete was cancelled, failed, or undone. +- Fixed a bug where deleting an image could corrupt the batch selection ranges if the delete was cancelled, failed, or undone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ChangeLog.md` at line 14, Update the ChangeLog entry that currently reads "Fixed a bug where deleting an image could mess up the batch selection ranges if the delete was cancelled, failed, or undone." to use formal wording by replacing the colloquial phrase "mess up" with a more professional term such as "corrupt" or "invalidate" (e.g., "Fixed a bug where deleting an image could corrupt the batch selection ranges if the delete was cancelled, failed, or undone.").
🤖 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 4422-4423: The current rollback restores the entire pre-delete
snapshot (job.saved_batches and job.saved_batch_start_index) even for partial
rollbacks, which can reintroduce stale/out-of-range spans; in _rollback_ui_items
detect whether the rollback is full by comparing restored = {idx for idx,_ in
items} with original = {idx for idx,_ in job.removed_items} and only do the
direct restore of self.batches = [b[:] for b in job.saved_batches] and
self.batch_start_index = job.saved_batch_start_index when restored == original;
otherwise compute still_deleted = sorted(original - restored) and rebuild
self.batches from job.saved_batches using a helper like
recompute_batches_after_deletions(job.saved_batches, still_deleted), then call
self._invalidate_batch_cache() in both branches to refresh caches.
- Around line 4297-4307: The shift/reindex math uses unvalidated sorted_indices
which may contain out-of-range values; before computing deleted_set or shift,
capture the original_count = len(self.image_files) (before any removals) and
compute validated_sorted = sorted(i for i in sorted_indices if 0 <= i <
original_count), then use deleted_set = set(validated_sorted) and compute shift
= sum(1 for d in validated_sorted if d < previous_index); use deleted_set and
shift (and validated_sorted) everywhere you currently reference sorted_indices
(including the previous_index membership check and the shift computation) so
only valid indices affect current_index.
---
Nitpick comments:
In `@ChangeLog.md`:
- Line 14: Update the ChangeLog entry that currently reads "Fixed a bug where
deleting an image could mess up the batch selection ranges if the delete was
cancelled, failed, or undone." to use formal wording by replacing the colloquial
phrase "mess up" with a more professional term such as "corrupt" or "invalidate"
(e.g., "Fixed a bug where deleting an image could corrupt the batch selection
ranges if the delete was cancelled, failed, or undone.").
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
faststack/tests/thumbnail_view/test_folder_stats.py (1)
273-273: Add one positivetodobucket assertion.These updates verify tuple arity, but they only cover
todo_ratio == 0.0. Please add one case with{"todo": True}so regressions in todo accumulation are caught.Proposed test addition
+ def test_single_file_todo(self): + """Test with single todo file.""" + jpg_files = ["a.jpg"] + entries = {"a": {"uploaded": False, "stacked": False, "todo": True}} + + buckets = _compute_coverage_buckets(jpg_files, entries, num_buckets=1) + + assert len(buckets) == 1 + assert buckets[0] == (0.0, 0.0, 1.0)Also applies to: 283-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/thumbnail_view/test_folder_stats.py` at line 273, Add an assertion that covers a positive todo bucket so we don't only test todo_ratio == 0.0: after the existing buckets check (variable buckets) insert an assertion that when the input includes {"todo": True} one of the tuples equals (uploaded_ratio, stacked_ratio, 1.0) — specifically assert buckets[...] == (1.0, 0.0, 1.0) for the case that previously asserted (1.0, 0.0, 0.0); apply the same additional assertion in the other similar test block that mirrors the assertion at the other location so both places validate todo accumulation.
🤖 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 4386-4422: The batch_start_index isn't adjusted after deletions;
update it after you compute new_batches: if pre_batch_start_snapshot is not None
then if pre_batch_start_snapshot is in deleted_set set self.batch_start_index =
None (start was deleted) else set self.batch_start_index =
_shift(pre_batch_start_snapshot) to reindex it for removed entries; if the
reindexed value differs from the current self.batch_start_index call
self._invalidate_batch_cache() (same cache invalidation path used for
self.batches). Ensure you reference the existing _shift helper,
pre_batch_start_snapshot, deleted_set, and self._invalidate_batch_cache() when
implementing this change.
- Around line 5110-5138: The _on_cache_evict callback currently has a default
info=None which prevents _detect_arity from recognizing the handler's third
parameter so _fire_evict only calls it with (key, value); remove the default so
the signature is def _on_cache_evict(self, key, value, info): to ensure the
evict info dict is passed through; update references to info usage (e.g.,
eviction_usage = info.get(...)) to assume info is provided and adjust any
callers or tests expecting None accordingly, and run tests that exercise
_fire_evict, _detect_arity, and thrash detection to verify behavior.
In `@faststack/imaging/cache.py`:
- Around line 93-110: _detect_arity currently counts only required positional
parameters (checks p.default is inspect.Parameter.empty), so bound callbacks
like def f(key, value, info=None) are seen as arity=2; change the counting logic
in _detect_arity to count all parameters that can accept positional arguments
(kinds POSITIONAL_ONLY or POSITIONAL_OR_KEYWORD) regardless of defaults, so a
signature with info=None yields count=3 and self._on_evict_arity will be >=3 and
_fire_evict will pass the info arg; update the generator in _detect_arity
accordingly and keep references to _detect_arity, self._on_evict_arity, and
_fire_evict when making the change.
---
Nitpick comments:
In `@faststack/tests/thumbnail_view/test_folder_stats.py`:
- Line 273: Add an assertion that covers a positive todo bucket so we don't only
test todo_ratio == 0.0: after the existing buckets check (variable buckets)
insert an assertion that when the input includes {"todo": True} one of the
tuples equals (uploaded_ratio, stacked_ratio, 1.0) — specifically assert
buckets[...] == (1.0, 0.0, 1.0) for the case that previously asserted (1.0, 0.0,
0.0); apply the same additional assertion in the other similar test block that
mirrors the assertion at the other location so both places validate todo
accumulation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9899064a-0e36-4b48-bed8-6b1d4bfbc2b8
📒 Files selected for processing (13)
faststack/app.pyfaststack/imaging/cache.pyfaststack/models.pyfaststack/qml/FilterDialog.qmlfaststack/qml/Main.qmlfaststack/qml/ThumbnailGridView.qmlfaststack/qml/ThumbnailTile.qmlfaststack/tests/test_cache_replacement_callback.pyfaststack/tests/thumbnail_view/test_folder_stats.pyfaststack/thumbnail_view/folder_stats.pyfaststack/thumbnail_view/model.pyfaststack/ui/keystrokes.pyfaststack/ui/provider.py
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
faststack/ui/provider.py (1)
1619-1633: ReuserefreshRecycleBinStats()fromcleanupRecycleBins().This slot now defines the canonical recycle-bin notify set, but
cleanupRecycleBins()still emits the same three signals manually on Lines 1631-1633. Calling the helper there keeps the refresh path from drifting.♻️ Suggested cleanup
`@Slot`() def cleanupRecycleBins(self): """Deletes all tracked recycle bins.""" self.app_controller.cleanup_recycle_bins() - - self.recycleBinStatsTextChanged.emit() - self.recycleBinDetailedTextChanged.emit() - self.hasRecycleBinItemsChanged.emit() + self.refreshRecycleBinStats()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/ui/provider.py` around lines 1619 - 1633, cleanupRecycleBins() currently duplicates the signal emissions that refreshRecycleBinStats() already encapsulates; replace the three manual emits in cleanupRecycleBins() with a single call to self.refreshRecycleBinStats() after self.app_controller.cleanup_recycle_bins() so the canonical notify logic lives in refreshRecycleBinStats() (update cleanupRecycleBins to call refreshRecycleBinStats rather than emitting recycleBinStatsTextChanged, recycleBinDetailedTextChanged, and hasRecycleBinItemsChanged directly).
🤖 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/qml/Main.qml`:
- Around line 1019-1023: The footer Label shows "Todo since " when
uiState.todoDate is an empty string; update the Label's text binding to check
uiState.todoDate (normalized from EntryMetadata.todo_date) and render "Todo
since {uiState.todoDate}" only when todoDate is non-empty, otherwise render
"Todo" (or an empty string if you prefer hiding the label), keeping the existing
visibility logic that uses uiState.imageCount and uiState.isTodo; locate the
Label block and change its text expression to branch on uiState.todoDate while
leaving the visible condition unchanged.
---
Nitpick comments:
In `@faststack/ui/provider.py`:
- Around line 1619-1633: cleanupRecycleBins() currently duplicates the signal
emissions that refreshRecycleBinStats() already encapsulates; replace the three
manual emits in cleanupRecycleBins() with a single call to
self.refreshRecycleBinStats() after self.app_controller.cleanup_recycle_bins()
so the canonical notify logic lives in refreshRecycleBinStats() (update
cleanupRecycleBins to call refreshRecycleBinStats rather than emitting
recycleBinStatsTextChanged, recycleBinDetailedTextChanged, and
hasRecycleBinItemsChanged directly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c2ecd94-f9cf-4807-9442-80f23c4e783f
📒 Files selected for processing (2)
faststack/qml/Main.qmlfaststack/ui/provider.py
|
Directionally the batch fix looks useful, but this PR is still harder to review than it needs to be because it mixes |
Owner
Author
|
@coderabbitai I made some updates - please re-check the issues you have identified to see if they are still present. |
|
✅ Actions performedReview triggered.
|
Owner
Author
Yea you are right - I made some changes last week and forgot to close out the PR and push to main before I made some more yesterday. |
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
2 participants
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
Chores