Added a sort feature#68
Merged
AlanRockefeller merged 6 commits intomainfrom Apr 11, 2026
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds controller-level sorting (default/filename/date), QML menu controls, UIState binding/signal, sorting/filtering/remapping logic that preserves or clears stacks with user confirmation, grid/thumbnail refresh handling, unit tests and fixture, plus minor IO and watcher/helicon tweaks. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant QML as QML UI
participant Ctrl as AppController
participant State as UIState
participant Model as Image Model
User->>QML: Select sort option
QML->>Ctrl: set_sort_mode(mode)
rect rgba(100,150,200,0.5)
Note over Ctrl: Validate mode\nSimulate reorder\nCheck stacks contiguity
end
Ctrl->>Model: _filtered_sorted_copy(mode)
Model-->>Ctrl: sorted image list
alt stacks non-contiguous
Ctrl->>User: prompt to clear stacks (QMessageBox)
User-->>Ctrl: confirm / cancel
end
Ctrl->>Ctrl: remap ranges / update indices\nrefresh thumbnail/grid if needed
Ctrl->>State: emit sortModeChanged
State->>QML: notify sortMode property
QML->>User: update selection UI and displayed images
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
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: 2
🧹 Nitpick comments (2)
ChangeLog.md (1)
19-19: Make the sort release note more specificLine 19 is accurate, but adding the available sort modes would make release notes clearer for users.
Suggested wording
-- Added a feature to sort the photos. +- Added photo sorting options: Default, By Filename, and By Date (Newest first).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ChangeLog.md` at line 19, The release note "Added a feature to sort the photos." is too vague—update the ChangeLog entry to list the available photo sort modes (e.g., "date (newest/oldest), name (A→Z/Z→A), and size (largest/smallest)") so users know what sorting options were added; edit the line that currently reads "Added a feature to sort the photos." in ChangeLog.md to a more specific sentence that names the supported sort modes and any default behavior.faststack/tests/test_sort_mode.py (1)
11-51: Consolidate duplicatedapp_controllerfixture to avoid test drift.This fixture largely duplicates
faststack/tests/test_reactive_delete.py:9-51. Moving it to a sharedconftest.pyfixture will reduce divergence and keep controller test setup consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/test_sort_mode.py` around lines 11 - 51, The app_controller fixture in this test duplicates setup used elsewhere; extract the fixture into a shared conftest.py to avoid drift by moving the entire pytest fixture named app_controller that constructs an AppController (uses QCoreApplication.instance()/QCoreApplication, image_dir tmp_path, mock_engine) and the patched dependencies (Watcher, SidecarManager, Prefetcher, ByteLRUCache, config, ThumbnailProvider, ThumbnailModel, ThumbnailPrefetcher, ThumbnailCache, Keybinder, UIState) into conftest.py, then import/use that fixture in this test; ensure the shared fixture still applies the same post-creation mocks on the controller (controller.refresh_image_list, controller.update_status_message, controller.sync_ui_state, controller.image_cache, controller.prefetcher, controller._thumbnail_model with rowCount, controller._thumbnail_prefetcher, controller._path_resolver, controller.dataChanged, controller.ui_state) so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ChangeLog.md`:
- Line 372: The ChangeLog entry for version "0.8.0" is malformed because the
header and its bullet are on the same line; update the line that currently reads
"## [0.8.0] - 2025-11-20- Backspace key now deletes images (in addition to
Delete key). Control-Z restores." by splitting it into a proper header "##
[0.8.0] - 2025-11-20" followed by a separate bullet line "- Backspace key now
deletes images (in addition to Delete key). Control-Z restores." so the markdown
header and content are on distinct lines.
In `@faststack/tests/test_sort_mode.py`:
- Around line 146-153: The test currently creates p_missing (gone.jpg) but never
deletes it, so the "missing file" branch isn't exercised; update the test around
ImageFile and _populate to remove or unlink p_missing before calling _populate
(or before constructing imgs used for date sort) so that
ImageFile(path=p_missing, timestamp=0.0) truly represents a missing/failed stat;
ensure the rest of the test still writes ok.jpg and calls
_populate(app_controller, imgs) to verify behavior without filesystem access to
gone.jpg.
---
Nitpick comments:
In `@ChangeLog.md`:
- Line 19: The release note "Added a feature to sort the photos." is too
vague—update the ChangeLog entry to list the available photo sort modes (e.g.,
"date (newest/oldest), name (A→Z/Z→A), and size (largest/smallest)") so users
know what sorting options were added; edit the line that currently reads "Added
a feature to sort the photos." in ChangeLog.md to a more specific sentence that
names the supported sort modes and any default behavior.
In `@faststack/tests/test_sort_mode.py`:
- Around line 11-51: The app_controller fixture in this test duplicates setup
used elsewhere; extract the fixture into a shared conftest.py to avoid drift by
moving the entire pytest fixture named app_controller that constructs an
AppController (uses QCoreApplication.instance()/QCoreApplication, image_dir
tmp_path, mock_engine) and the patched dependencies (Watcher, SidecarManager,
Prefetcher, ByteLRUCache, config, ThumbnailProvider, ThumbnailModel,
ThumbnailPrefetcher, ThumbnailCache, Keybinder, UIState) into conftest.py, then
import/use that fixture in this test; ensure the shared fixture still applies
the same post-creation mocks on the controller (controller.refresh_image_list,
controller.update_status_message, controller.sync_ui_state,
controller.image_cache, controller.prefetcher, controller._thumbnail_model with
rowCount, controller._thumbnail_prefetcher, controller._path_resolver,
controller.dataChanged, controller.ui_state) so behavior remains identical.
🪄 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: 7dab5f82-cc9d-47d1-81a1-fef08513c353
📒 Files selected for processing (5)
ChangeLog.mdfaststack/app.pyfaststack/qml/Main.qmlfaststack/tests/test_sort_mode.pyfaststack/ui/provider.py
![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: 78c7f895fb
ℹ️ 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
+1137
to
+1138
| if self.sort_mode == "filename": | ||
| filtered.sort(key=lambda img: img.path.name.lower()) |
There was a problem hiding this comment.
The new sort path reorders self.image_files, but existing self.stacks/self.batches (which are stored as positional index ranges) are left unchanged. In the scenario where a user defines stacks or batches and then changes sort mode, subsequent index-based actions (for example Helicon launch or batch deletion) will target different files than the user originally selected, because those ranges now point at different rows after sorting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
faststack/tests/test_sort_mode.py (1)
188-191: Make the no-op assertion resilient to fixture-side calls.At Line 191,
assert_not_called()can become flaky ifsync_ui_statewas touched before this assertion. Compare call count before vs after this action instead.Proposed test hardening
- app_controller.set_sort_mode("default") # already default - - # sync_ui_state should not be called again (the fixture's mock tracks calls) - app_controller.sync_ui_state.assert_not_called() + before = app_controller.sync_ui_state.call_count + app_controller.set_sort_mode("default") # already default + # Ensure no additional call was made + assert app_controller.sync_ui_state.call_count == before🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/test_sort_mode.py` around lines 188 - 191, The test currently uses app_controller.sync_ui_state.assert_not_called() which is flaky if the fixture touched sync_ui_state earlier; instead capture the prior call count (e.g., prev = app_controller.sync_ui_state.call_count) before calling app_controller.set_sort_mode("default") and then assert that app_controller.sync_ui_state.call_count == prev to ensure no additional calls were made; update the assertion around the set_sort_mode call to use call_count comparison rather than assert_not_called(), referencing app_controller.set_sort_mode and app_controller.sync_ui_state.
🤖 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 717-755: The code snapshots and remaps stacks using
self.image_files (the filtered view) then writes the rebuilt ranges into
sidecar.data.stacks, which can drop hidden members; before calling
self._apply_filter_to_cached_list() capture the canonical/unfiltered image list
(or its paths) and use that to compute old_stack_paths via
_resolve_ranges_to_paths and to rebuild stacks via _rebuild_ranges_from_paths,
and use the canonical _path_to_index when resolving stack_start_index;
alternatively, if you cannot access an unfiltered list, prevent persisting stack
remaps while a filter is active (i.e., skip writing to sidecar.data.stacks when
a filter is applied). Ensure you update references to image_files usage for
stack remapping to the canonical list and only call sidecar.save() when
remapping against that canonical list.
In `@faststack/tests/conftest.py`:
- Around line 44-69: The fixture currently exits the patch context before tests
run because it uses "return controller" inside the "with (patch(...)):" block;
change that "return controller" to "yield controller" so the patch context stays
active for the duration of the test execution (keep the same "with
(patch(...)):" block and the controller setup, just replace return with yield to
turn the fixture into a generator-style fixture).
In `@faststack/tests/test_sort_mode.py`:
- Around line 305-322: The current test only checks that a_idx and b_idx are
included in batch_indices, which misses over-inclusive remaps; replace the two
inclusion asserts with a strict equality check: build the expected set from the
located indices (a_idx and b_idx found via app_controller.image_files) and
assert batch_indices == expected (using the existing variables batch_indices,
a_idx, b_idx, and app_controller.batches/image_files) so the test fails if any
extra indices are present.
- Around line 344-349: The test currently verifies app_controller.sort_mode and
that app_controller.image_files[app_controller.stack_start_index].path.name ==
"a.jpg", but doesn't assert the actual remapped index; add an explicit assertion
that app_controller.stack_start_index == 2 (the expected remapped index under
date sort) immediately after the sort is applied so the test confirms both the
index remapping and the pointed image identity (use the existing app_controller,
stack_start_index, and image_files symbols to locate where to add the
assertion).
---
Nitpick comments:
In `@faststack/tests/test_sort_mode.py`:
- Around line 188-191: The test currently uses
app_controller.sync_ui_state.assert_not_called() which is flaky if the fixture
touched sync_ui_state earlier; instead capture the prior call count (e.g., prev
= app_controller.sync_ui_state.call_count) before calling
app_controller.set_sort_mode("default") and then assert that
app_controller.sync_ui_state.call_count == prev to ensure no additional calls
were made; update the assertion around the set_sort_mode call to use call_count
comparison rather than assert_not_called(), referencing
app_controller.set_sort_mode and app_controller.sync_ui_state.
🪄 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: 8019d8ee-1e02-4ea2-9699-3e7c4e3a7b22
📒 Files selected for processing (7)
ChangeLog.mdfaststack/app.pyfaststack/io/deletion.pyfaststack/io/helicon.pyfaststack/io/watcher.pyfaststack/tests/conftest.pyfaststack/tests/test_sort_mode.py
💤 Files with no reviewable changes (1)
- faststack/io/helicon.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
faststack/app.py (1)
716-720:⚠️ Potential issue | 🟠 MajorFiltered sort still mutates stack membership from a partial list.
At Line 717 and Line 752, stack remap is computed from
self.image_files(filtered view). Hidden members are dropped fromself.stacksin-memory, and that truncated state can later be persisted by unrelated stack actions. Please avoid stack-preserving remap when a filter is active (or remap against an unfiltered canonical list).💡 Safer fallback (block stack-preserving remap while filter is active)
if have_stacks: + if self._filter_enabled: + if not self._confirm_clear_stacks_for_sort(): + return + clear_stacks = True + else: new_order = self._simulate_sorted_list(mode) new_path_to_idx = { self._key(img.path): i for i, img in enumerate(new_order) } if not self._stacks_stay_contiguous(new_path_to_idx): if not self._confirm_clear_stacks_for_sort(): return # user cancelled — no state changed clear_stacks = TrueAlso applies to: 752-756
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 716 - 720, The stack-preserving remap currently uses the filtered view self.image_files and thus can drop hidden stack members; in the block that computes old_stack_paths via self._resolve_ranges_to_paths(self.stacks) (and the later remap logic around where stacks are rewritten), detect when a filter is active (e.g., self.image_files is a filtered subset compared to the canonical/unfiltered list) and in that case avoid performing the stack remap — either set old_stack_paths = [] (skip remap) or compute remap against the unfiltered canonical image list instead (call _resolve_ranges_to_paths on the canonical list); apply the same guard to the later remap computation so no hidden members are dropped when a filter is active.
🤖 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`:
- Line 849: The for-loop using zip(indices, indices[1:]) should be replaced with
itertools.pairwise(indices) to avoid slicing and satisfy Ruff rules; update the
loop in the function/method where "for a, b in zip(indices, indices[1:]):"
appears (in app.py around the logic handling indices) to "for a, b in
pairwise(indices):" and add "from itertools import pairwise" to the module
imports (or import pairwise in the same scope), ensuring the codebase runs on
Python >=3.10.
---
Duplicate comments:
In `@faststack/app.py`:
- Around line 716-720: The stack-preserving remap currently uses the filtered
view self.image_files and thus can drop hidden stack members; in the block that
computes old_stack_paths via self._resolve_ranges_to_paths(self.stacks) (and the
later remap logic around where stacks are rewritten), detect when a filter is
active (e.g., self.image_files is a filtered subset compared to the
canonical/unfiltered list) and in that case avoid performing the stack remap —
either set old_stack_paths = [] (skip remap) or compute remap against the
unfiltered canonical image list instead (call _resolve_ranges_to_paths on the
canonical list); apply the same guard to the later remap computation so no
hidden members are dropped when a filter is active.
🪄 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: f4d5c7ee-b785-4559-b0e1-bcf96a68bf27
📒 Files selected for processing (3)
faststack/app.pyfaststack/tests/conftest.pyfaststack/tests/test_sort_mode.py
🚧 Files skipped from review as they are similar to previous changes (2)
- faststack/tests/conftest.py
- faststack/tests/test_sort_mode.py
Merged
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
Behavior / UX
Tests
Documentation