A few minor bug fixes#47
Conversation
|
Warning Rate limit exceeded
⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughRelease version 1.6.0 adds thread-safe display state management with a new watcher refresh handler, optimizes thumbnail path lookups via hash mapping, adjusts prefetch radius calculations, extends the deletion data structure for stack-level restoration, and updates documentation and version numbers accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant FSW as File System Watcher
participant AC as AppController
participant DG as Display Cache
participant PF as Prefetch Engine
participant IL as Image List
FSW->>AC: File change detected
AC->>AC: _on_watcher_refresh() called (debounced)
AC->>AC: Clamp current_index if needed
AC->>DG: _bump_display_generation() under _display_lock
DG->>DG: Invalidate cached display
AC->>IL: Load updated image list
IL->>IL: Refresh from controller
AC->>PF: Update prefetch based on new index
PF->>PF: Recompute visible/prefetch ranges
AC->>AC: Ensure UI consistency
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 5
🧹 Nitpick comments (1)
faststack/thumbnail_view/model.py (1)
671-673: Reuse the shared path normalizer here.
faststack/io/utils.py:9-25already centralizes this logic asnormalize_path_key(), andfaststack/app.py:164-169uses that helper for controller-side keys. Reimplementing the normalization inline here makes_path_to_rowvulnerable to silent misses if the two copies ever drift.♻️ Suggested cleanup
- self._path_to_row[ - os.path.normcase(os.path.abspath(str(e.path))) - ] = i + self._path_to_row[normalize_path_key(e.path)] = i ... - key = os.path.normcase(os.path.abspath(str(path))) + key = normalize_path_key(path) return self._path_to_row.get(key, -1)Also extend the
faststack.io.utilsimport to includenormalize_path_key.Also applies to: 864-865
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/model.py` around lines 671 - 673, Replace the inline normalization in ThumbnailView's code that does os.path.normcase(os.path.abspath(str(e.path))) with the shared helper normalize_path_key from faststack.io.utils: import normalize_path_key and call it when populating self._path_to_row (both at the current assignment and the similar one at lines ~864-865) so all path keys use the centralized normalization logic.
🤖 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`:
- Around line 10-16: The changelog bullets repeat the same leading verbs
("Fixed" and "Improved"), making scanning harder; update several adjacent
entries in the ChangeLog bullets so their openings vary (e.g., swap one "Fixed"
to "Resolved" or "Restored" and one "Improved" to "Enhanced" or "Optimized")
while keeping the original meaning—target the lines that start with "Fixed image
list...", "Fixed undo and rollback...", "Fixed stack state...", and the
"Improved..." lines and adjust 2–3 of their leading words to different verbs for
variety.
In `@faststack/app.py`:
- Around line 988-1004: When re-selecting after external file changes, clear the
stale variant override so get_decoded_image() doesn't keep pointing at a missing
override; in the block that computes target_key via self._key(preserved_path)
and looks up new_idx in self._path_to_index, if new_idx is None (or if
self.image_files is empty) set self.view_override_path (and any related override
state) to None before clamping self.current_index and calling self._do_prefetch
and self.sync_ui_state so the UI will fall back to the main image instead of a
deleted override.
- Around line 986-1004: The watcher-driven refresh path calls
self.refresh_image_list() but does not update the image cache generation, so
get_decoded_image() (which keys by path+display_generation) can return stale
frames; after refresh_image_list() (and before _do_prefetch()/sync_ui_state())
increment or otherwise change self.display_generation (e.g., bump the integer
generation counter) so cached decoded images are invalidated and new frames will
be re-decoded.
- Around line 4289-4296: The code resets self.current_index to
job.previous_index which breaks the visible selection when only a subset of rows
are reinserted; instead compute how many items were inserted before the original
selection and adjust the index accordingly: after reinserting (items), sort them
the same way and compute inserted_before = sum(1 for idx,_ in items if idx <=
job.previous_index) then set self.current_index = min(job.previous_index +
inserted_before, len(self.image_files) - 1); update the block around
self.image_files.insert(...) and the assignment to self.current_index
(referencing image_files, items, job.previous_index, current_index, and
_bump_display_generation) to apply this correction.
In `@faststack/imaging/prefetch.py`:
- Around line 267-271: The directional split can produce zero or negative slots
for the travel direction when effective_radius is small; update the allocation
logic around effective_radius, self._direction_bias, ahead, and behind so each
side gets at least one slot in the travel direction and the opposite side is
non-negative: compute the raw biased value (e.g. behind_raw =
int(effective_radius * (1 - self._direction_bias))) then clamp and rebalance
(behind = max(1, behind_raw); ahead = effective_radius - behind; if ahead < 1:
ahead = 1; behind = effective_radius - ahead) and mirror the logic for the
backward-moving branch to ensure ahead/behind are never zero or negative while
preserving the total budget.
---
Nitpick comments:
In `@faststack/thumbnail_view/model.py`:
- Around line 671-673: Replace the inline normalization in ThumbnailView's code
that does os.path.normcase(os.path.abspath(str(e.path))) with the shared helper
normalize_path_key from faststack.io.utils: import normalize_path_key and call
it when populating self._path_to_row (both at the current assignment and the
similar one at lines ~864-865) so all path keys use the centralized
normalization logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c7b71a2-febd-449a-8d15-9be01f6848e5
📒 Files selected for processing (7)
ChangeLog.mdREADME.mdfaststack/app.pyfaststack/deletion_types.pyfaststack/imaging/prefetch.pyfaststack/thumbnail_view/model.pypyproject.toml
| - Fixed image list refresh after filesystem watcher events so the current image stays selected when possible. | ||
| - Fixed undo and rollback so deleted images are restored to the correct positions. | ||
| - Fixed stack state being lost or shifted incorrectly after delete and undo operations. | ||
| - Improved display cache invalidation so zoom, resize, filter, and edit changes refresh the correct image version. | ||
| - Improved prefetch behavior when zooming or resizing to reduce stale background work. | ||
| - Improved thumbnail lookup speed by adding a faster path-to-row mapping. | ||
| - Reduced chances of UI state getting out of sync after external file changes. |
There was a problem hiding this comment.
Vary the repeated bullet openings for easier scanning.
Several adjacent entries begin with the same verb (Fixed / Improved), which makes this release note section a bit repetitive to read. Rewording one or two bullets would make the changelog easier to skim.
Suggested wording tweak
-- Fixed image list refresh after filesystem watcher events so the current image stays selected when possible.
-- Fixed undo and rollback so deleted images are restored to the correct positions.
-- Fixed stack state being lost or shifted incorrectly after delete and undo operations.
-- Improved display cache invalidation so zoom, resize, filter, and edit changes refresh the correct image version.
-- Improved prefetch behavior when zooming or resizing to reduce stale background work.
-- Improved thumbnail lookup speed by adding a faster path-to-row mapping.
-- Reduced chances of UI state getting out of sync after external file changes.
+- Kept the current image selected when possible after filesystem watcher refresh events.
+- Corrected undo and rollback so deleted images return to the right positions.
+- Preserved stack state correctly across delete and undo operations.
+- Improved display cache invalidation so zoom, resize, filter, and edit changes refresh the correct image version.
+- Tuned prefetch behavior during zoom and resize to reduce stale background work.
+- Added a faster path-to-row mapping for thumbnail lookups.
+- Reduced the chance of UI state drifting after external file changes.📝 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.
| - Fixed image list refresh after filesystem watcher events so the current image stays selected when possible. | |
| - Fixed undo and rollback so deleted images are restored to the correct positions. | |
| - Fixed stack state being lost or shifted incorrectly after delete and undo operations. | |
| - Improved display cache invalidation so zoom, resize, filter, and edit changes refresh the correct image version. | |
| - Improved prefetch behavior when zooming or resizing to reduce stale background work. | |
| - Improved thumbnail lookup speed by adding a faster path-to-row mapping. | |
| - Reduced chances of UI state getting out of sync after external file changes. | |
| - Kept the current image selected when possible after filesystem watcher refresh events. | |
| - Corrected undo and rollback so deleted images return to the right positions. | |
| - Preserved stack state correctly across delete and undo operations. | |
| - Improved display cache invalidation so zoom, resize, filter, and edit changes refresh the correct image version. | |
| - Tuned prefetch behavior during zoom and resize to reduce stale background work. | |
| - Added a faster path-to-row mapping for thumbnail lookups. | |
| - Reduced the chance of UI state drifting after external file changes. |
🧰 Tools
🪛 LanguageTool
[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...re restored to the correct positions. - Fixed stack state being lost or shifted incor...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~15-~15: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...zing to reduce stale background work. - Improved thumbnail lookup speed by adding a fast...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ChangeLog.md` around lines 10 - 16, The changelog bullets repeat the same
leading verbs ("Fixed" and "Improved"), making scanning harder; update several
adjacent entries in the ChangeLog bullets so their openings vary (e.g., swap one
"Fixed" to "Resolved" or "Restored" and one "Improved" to "Enhanced" or
"Optimized") while keeping the original meaning—target the lines that start with
"Fixed image list...", "Fixed undo and rollback...", "Fixed stack state...", and
the "Improved..." lines and adjust 2–3 of their leading words to different verbs
for variety.
| # Insert in ascending index order so each insertion shifts subsequent | ||
| # indices correctly, restoring the original list positions. | ||
| for idx, img in sorted(items, key=lambda x: x[0]): | ||
| self.image_files.insert(min(idx, len(self.image_files)), img) | ||
|
|
||
| # Restore selection/focus (approximated) | ||
| self.current_index = min(job.previous_index, len(self.image_files) - 1) | ||
| self.display_generation += 1 | ||
| self._bump_display_generation() |
There was a problem hiding this comment.
Preserve the currently visible image on partial rollback.
Resetting current_index to job.previous_index is only correct for a full restore. If just a subset of deleted rows comes back, reinserting rows before the current selection shifts the surviving image right, so this can land on the wrong file.
💡 Safer selection restore
def _rollback_ui_items(self, items: List[Tuple[int, Any]], job: DeleteJob) -> None:
"""Restore items to the UI list in correct order."""
+ preserved_path = None
+ if self.image_files and 0 <= self.current_index < len(self.image_files):
+ preserved_path = self.image_files[self.current_index].path
+
# Insert in ascending index order so each insertion shifts subsequent
# indices correctly, restoring the original list positions.
for idx, img in sorted(items, key=lambda x: x[0]):
self.image_files.insert(min(idx, len(self.image_files)), img)
- # Restore selection/focus (approximated)
- self.current_index = min(job.previous_index, len(self.image_files) - 1)
+ if preserved_path is not None:
+ restored_idx = next(
+ (i for i, img in enumerate(self.image_files) if img.path == preserved_path),
+ None,
+ )
+ self.current_index = (
+ restored_idx
+ if restored_idx is not None
+ else min(job.previous_index, len(self.image_files) - 1)
+ )
+ else:
+ self.current_index = min(job.previous_index, len(self.image_files) - 1)
self._bump_display_generation()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/app.py` around lines 4289 - 4296, The code resets
self.current_index to job.previous_index which breaks the visible selection when
only a subset of rows are reinserted; instead compute how many items were
inserted before the original selection and adjust the index accordingly: after
reinserting (items), sort them the same way and compute inserted_before = sum(1
for idx,_ in items if idx <= job.previous_index) then set self.current_index =
min(job.previous_index + inserted_before, len(self.image_files) - 1); update the
block around self.image_files.insert(...) and the assignment to
self.current_index (referencing image_files, items, job.previous_index,
current_index, and _bump_display_generation) to apply this correction.
![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: 1ebbef30c7
ℹ️ 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 self._last_navigation_direction > 0: # Moving forward | ||
| behind = max(1, int(effective_radius * (1 - self._direction_bias))) | ||
| ahead = effective_radius - behind + 1 | ||
| ahead = effective_radius - behind |
There was a problem hiding this comment.
This change makes ahead/behind sum exactly to effective_radius, which means with prefetch_radius = 1 (allowed by settings) and forward navigation, behind becomes 1 and ahead becomes 0, so we stop prefetching the next image entirely. In that configuration the app now prefetches the previous frame instead of the one the user is about to open, which regresses navigation latency for low-radius users.
Useful? React with 👍 / 👎.
| key = os.path.normcase(os.path.abspath(str(path))) | ||
| return self._path_to_row.get(key, -1) |
There was a problem hiding this comment.
Switching find_image_index() from a .resolve() comparison to an abspath/normcase hash lookup breaks matches when the same file is referenced through different symlink aliases. ThumbnailModel navigation/refresh paths are canonicalized with .resolve(), but controller paths can still carry the launch alias, so the lookup can return -1 and fail to scroll/select the current image when entering grid view from symlinked directories.
Useful? React with 👍 / 👎.
1 participant
Summary by CodeRabbit
Release Notes - Version 1.6.0
New Features
Bug Fixes
Performance