Find paths to external programs automatically#101
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR refactors batch state management around sidecar-backed helpers and a centralized ChangesBatch State Management and Metadata Propagation
Cross-Platform External Tool Auto-Detection
README Viewer, Dialog Improvements, and App Close Flow
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Owner
Author
|
@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: 84124ae5b0
ℹ️ 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
+1758
to
+1759
| active_keys = self._current_batch_metadata_keys() | ||
| images_to_update = list(self._all_images or self.image_files) |
There was a problem hiding this comment.
When a filter is active, _restore_batches_from_sidecar_flags() rebuilds self.batches only from the visible self.image_files, so active_keys contains only visible batch members. Persisting across self._all_images then sets batch=false for every hidden image whose key is not in active_keys, so adding/toggling a visible batch item while filtered silently removes previously batched hidden images from the sidecar selection.
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: 2
🧹 Nitpick comments (1)
faststack/config.py (1)
384-386: 💤 Low valuePotential KeyError if called with unknown tool name.
If
detect_external_tool_pathis called with atool_namenot in_TOOL_LABELS, line 385 will raise aKeyError, bypassing the intended warning. The function already returns([], [])for unknown tools at line 303, so this is unlikely in practice, but defensive handling would be cleaner.Suggested improvement
except (OSError, RuntimeError) as e: - log.warning("Error detecting %s path: %s", _TOOL_LABELS[tool_name], e) + log.warning("Error detecting %s path: %s", _TOOL_LABELS.get(tool_name, tool_name), e) return None🤖 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/config.py` around lines 384 - 386, The exception handler on line 385 accesses _TOOL_LABELS[tool_name] directly without checking if tool_name exists in the dictionary, which could raise a KeyError. Replace the direct dictionary access with the .get() method using a fallback value like "unknown tool" or the tool_name itself to ensure the warning logs properly even if an unexpected tool_name reaches this exception handler.
🤖 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/app.py`:
- Around line 1756-1782: The images_to_update selection in the
_persist_batch_flags method uses _all_images or self.image_files, but
active_keys only contains keys from the currently filtered image_files, causing
the method to write batch=False to hidden images when edits occur during
filtering. Change the images_to_update assignment to only iterate over the
filtered self.image_files instead of _all_images to ensure batch flag
persistence only affects visible images, preserving the batch membership of
hidden images when filters are cleared.
- Around line 8204-8205: The _finalize_batch_state() method calls with
batch_changed condition are persisting batch flag changes while delete
operations are still undoable, causing the persisted flags to be cleared before
undo can restore them. Modify the _finalize_batch_state() calls (at the
identified location and also at lines 8433-8435) to pass persist=False parameter
while the delete operation is still undoable. After the worker completes and the
delete is no longer undoable, you can persist the batch state changes by either
deferring the persist call or making a separate call to _finalize_batch_state()
with persist=True once the delete operation has been finalized.
---
Nitpick comments:
In `@faststack/config.py`:
- Around line 384-386: The exception handler on line 385 accesses
_TOOL_LABELS[tool_name] directly without checking if tool_name exists in the
dictionary, which could raise a KeyError. Replace the direct dictionary access
with the .get() method using a fallback value like "unknown tool" or the
tool_name itself to ensure the warning logs properly even if an unexpected
tool_name reaches this exception handler.
🪄 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: 1c1d7faa-d7f5-445d-81f0-49f141fe49bc
📒 Files selected for processing (10)
faststack/app.pyfaststack/config.pyfaststack/models.pyfaststack/qml/FilterDialog.qmlfaststack/qml/Main.qmlfaststack/qml/QuitBatchesDialog.qmlfaststack/qml/SettingsDialog.qmlfaststack/resources.pyfaststack/ui/provider.pypackaging/faststack.spec
💤 Files with no reviewable changes (1)
- faststack/qml/QuitBatchesDialog.qml
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
Improvements
UI Updates