Release v0.7 — more improvements#11
Conversation
WalkthroughThis PR introduces security hardening for subprocess execution, adds comprehensive documentation (WARP project guide, ICC color profile fix), enhances the UI with high-DPI support, filter display, directory path display, and zoom reset (Ctrl+0), and updates the version to 0.7 with new features including Photoshop integration, clipboard support, image filtering, drag-and-drop, and theme support. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QML as QML UI
participant Keystrokes as Keybinder
participant App as AppController
participant Provider as UIState
participant Components as Components.qml
User->>QML: Press Ctrl+0
QML->>Keystrokes: handle_key_press(Ctrl+0)
Keystrokes->>App: reset_zoom_pan()
App->>Provider: ui_state.resetZoomPan()
Provider->>Provider: emit resetZoomPanRequested
Provider-->>Components: signal received
Components->>Components: reset scale = 1.0, pan = (0,0)
Components-->>QML: image re-rendered
QML-->>User: zoom/pan reset, image fit to window
sequenceDiagram
participant User
participant App as AppController
participant Validator as executable_validator
participant Subprocess as subprocess
User->>App: edit_in_photoshop()
App->>App: verify image path exists
App->>Validator: validate_executable_path(photoshop_exe)
Validator->>Validator: resolve path & check existence
Validator->>Validator: verify is file & executable
Validator->>Validator: check safe directory
Validator-->>App: (is_valid, error_msg)
alt validation passed
App->>App: parse args with shlex.split()
App->>Subprocess: Popen(cmd, shell=False, <br/>stdin=DEVNULL, stdout=DEVNULL, <br/>stderr=DEVNULL, close_fds=True)
Subprocess-->>App: process launched
App-->>User: Photoshop opens image
else validation failed
App->>App: log error
App-->>User: editor launch blocked
end
sequenceDiagram
participant User
participant QML as Main.qml
participant App as AppController
participant Provider as UIState
User->>QML: Type filter text in FilterDialog
QML->>App: apply_filter(filter_string)
App->>App: filter active image list
App->>Provider: emit filterStringChanged
Provider-->>QML: signal received
QML->>QML: update filter status label<br/>(text: Filter: "{filterString}")
QML-->>User: show filtered results,<br/>status bar shows filter
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 7
🧹 Nitpick comments (15)
faststack/faststack.egg-info/dependency_links.txt (1)
1-1: Consider excluding auto-generatedegg-infodirectory from version control.The
dependency_links.txtfile is part of the auto-generatedegg-infometadata created by setuptools at build time. These files are typically generated artifacts and should not be committed to the repository.Add
*.egg-info/to.gitignoreto prevent committing auto-generated build artifacts.faststack/pyproject.toml (1)
8-29: pytest listed as runtime dependency
pytestin[project.dependencies](Line 28) makes it a runtime dependency for end‑users rather than a dev/test‑only requirement. Consider moving it to a dev extra (e.g.,[project.optional-dependencies]) or a separaterequirements-dev.txtso installs don’t pull in the test framework unnecessarily.faststack/README.md (1)
20-24: Align README version and shortcuts with implementationThe new feature bullets and shortcuts for
E,Ctrl+C, andCread clearly. However:
- The header still says
Version 0.4 - November 2, 2025whilepyproject.tomlis at0.7; consider updating the README version banner for consistency.- Double‑check that the actual keymap (in
faststack/faststack/ui/keystrokes.py) wiresE,Ctrl+C, andCexactly as documented so users don’t see drift between docs and behavior.Also applies to: 49-51
WARP.md (1)
18-29: Clarify duplicatedpython -m faststack.appcommandsThe two “Running the Application” snippets both show
python -m faststack.app "C:\path\to\images"with different comments (“development run” vs “from the inner faststack directory”). If there’s a real distinction (e.g., working directory or environment), consider making it explicit or dropping one of the duplicates to avoid confusion.faststack/faststack/qml/Components.qml (1)
11-20: High‑DPI reporting and zoom reset look good; consider per‑window DPRThe zoom/pan reset via
onResetZoomPanRequested()and the move to DPR‑aware size reporting both look solid and should fix high‑DPI rendering issues.As a minor improvement, you might:
- Use the window’s DPR (e.g.,
mainImage.window.devicePixelRatio) instead of the globalScreen.devicePixelRatiofor better behavior on multi‑monitor / mixed‑DPI setups.- Factor the DPR size computation into a small helper function to avoid duplicating the
Math.round(width * dpr)logic in bothComponent.onCompletedand the resize timer.Also applies to: 30-35, 79-81
faststack/faststack/qml/Main.qml.bak (1)
1-295: Clarify the role ofMain.qml.bakand keep key‑binding help in syncThis file appears to be a full alternative shell for the main window (menus, dialogs, footer, etc.) rather than a small backup snippet. If it is not loaded anywhere, consider removing it to avoid divergence from the real
Main.qml. If it is used:
- Ensure the “Key Bindings” dialog text (Lines 229–262) is updated to include any new shortcuts added elsewhere (e.g., zoom/pan reset), so user-visible help matches
Keybinder’s mappings.- Verify that all
uiStatemethods/properties referenced here (applyFilter,launch_helicon,preloadAllImages,stackSummary, etc.) exist and remain consistent with the Python side as the UI evolves.This is not a blocker, but cleaning this up will reduce confusion and future maintenance overhead.
run_app.py (1)
1-10: Use a__main__guard and callfaststack.app:maindirectlyThis works, but a couple of tweaks would make it clearer and safer:
- Without an
if __name__ == "__main__":guard, importingrun_appwill immediately start the app, which is surprising.- Since you already expose
faststack.app:mainas a console entry point, you can mirror that here instead of usingrunpy.For example:
import sys from pathlib import Path # Add the directory containing the 'faststack' package to the Python path sys.path.insert(0, str((Path(__file__).parent / "faststack").resolve())) def main(): from faststack.app import main as faststack_main faststack_main() if __name__ == "__main__": main()Keeps behavior equivalent while making the launcher easier to reason about.
faststack/faststack.egg-info/entry_points.txt (1)
1-2: Entry point definition is fine; just keep it as the single source of truthThe console script
faststack = faststack.app:mainlooks correct and matches the described application entry point. Given this, it’s best to treatfaststack.app:mainas the canonical startup path and keep any helper launchers (likerun_app.py) thin shims around it, so behavior stays consistent across install vs local runs.faststack/faststack/tests/test_executable_validator.py (1)
111-130: Consider asserting that error is None for clarity.While the test correctly verifies that validation passes despite the wrong executable name, explicitly asserting that
error is Nonewould make the expected behavior clearer.Apply this diff:
is_valid, error = validate_executable_path( wrong_exe, app_type="photoshop" ) assert is_valid # Name mismatch is warning, not failure + assert error is None # No error message returnedfaststack/faststack/io/helicon.py (1)
85-90: Remove redundant exception objects from logging.exception calls.Lines 86 and 89 include the exception object in the message, but
logging.exceptionautomatically includes the traceback, making the exception object redundant.Apply this diff:
except (OSError, subprocess.SubprocessError) as e: - log.exception(f"Failed to launch Helicon Focus: {e}") + log.exception("Failed to launch Helicon Focus") return False, None except (IOError, PermissionError) as e: - log.exception(f"Failed to create temporary file for Helicon Focus: {e}") + log.exception("Failed to create temporary file for Helicon Focus") return False, Nonefaststack/faststack/io/executable_validator.py (1)
11-20: Consider adding Unix/macOS safe paths.The
KNOWN_SAFE_PATHSonly includes Windows Program Files directories. Consider adding common safe paths for Unix-like systems (e.g.,/usr/bin,/usr/local/bin,/Applicationson macOS) to support cross-platform usage.KNOWN_SAFE_PATHS = [ r"C:\Program Files", r"C:\Program Files (x86)", + "/usr/bin", + "/usr/local/bin", + "/opt", + "/Applications", # macOS ]faststack/faststack/app.py (2)
15-42: RedundantImageProviderimport
ImageProvideris imported twice (lines 15and42); the second import is redundant and can be dropped to avoid confusion.
592-657: Photoshop launcher security hardening is solid; minor logging tweaksThe new
edit_in_photoshopflow (path validation viavalidate_executable_path, existence checks on the image,shlex.splitfor args,shell=False, stdio toDEVNULL) meaningfully tightens subprocess security while keeping behavior predictable. A few small follow‑ups:
- In the
ValueErrorhandler forshlex.split, Ruff suggestslogging.exceptioninstead oflogging.error, and your laterlog.exceptioncalls don’t need the explicit{e}formatting. A minimal style/diagnostic improvement:- except ValueError as e: - log.error(f"Invalid photoshop_args format: {e}") - self.update_status_message("Invalid Photoshop arguments configured") - return + except ValueError: + log.exception("Invalid Photoshop arguments configured") + self.update_status_message("Invalid Photoshop arguments configured") + return @@ - except (OSError, subprocess.SubprocessError) as e: - self.update_status_message(f"Failed to open in Photoshop: {e}") - log.exception(f"Error launching Photoshop: {e}") - except FileNotFoundError as e: - self.update_status_message(f"Photoshop executable not found: {e}") - log.exception(f"Photoshop executable not found: {e}") + except (OSError, subprocess.SubprocessError) as e: + self.update_status_message(f"Failed to open in Photoshop: {e}") + log.exception("Error launching Photoshop") + except FileNotFoundError as e: + self.update_status_message(f"Photoshop executable not found: {e}") + log.exception("Photoshop executable not found")
- Ruff’s S603 warning about subprocess and “untrusted input” is largely addressed by
shell=Falseand executable validation; as long as your config is treated as trusted local state, this is acceptable. If you ever load config from untrusted sources, you may want an allow‑list of Photoshop argument flags.faststack/faststack/ui/provider.py.bak (1)
51-161:UIStatein.bakfile is broken and likely staleIn this
.bakversion, the@Property(int, notify=themeChanged)decorator is indented inside__init__, sothemeis defined as a plain method without becoming a QtProperty; if this class were ever used from QML, thethemeproperty wouldn’t be exposed correctly. The file also lacks the newer signals/properties (e.g.,filterStringChanged,stackSummaryChanged,resetZoomPanRequested) thatAppControllerexpects. If this is just a backup, I’d strongly recommend deleting it or clearly marking it as dead code so it doesn’t get imported accidentally.faststack/faststack/app.py.bak (1)
1-638: Duplicate app controller in.bakrisks divergenceThis
.bakfile contains a full alternateAppControllerandmainentry point that diverges from the primaryfaststack/faststack/app.py(different default filter, metadata signaling, drag behavior, stack summary API, etc.). Keeping both in the tree makes it easy for future changes to land in the wrong place or for someone to import the outdated version by mistake. If you no longer rely on this snapshot, I’d strongly suggest deleting it (or moving it out of the package) rather than trying to keep it in sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
WARP.md(1 hunks)docs/COLOR_PROFILE_FIX.md(1 hunks)faststack/ChangeLog.md(1 hunks)faststack/README.md(2 hunks)faststack/faststack.egg-info/PKG-INFO(1 hunks)faststack/faststack.egg-info/SOURCES.txt(1 hunks)faststack/faststack.egg-info/dependency_links.txt(1 hunks)faststack/faststack.egg-info/entry_points.txt(1 hunks)faststack/faststack.egg-info/requires.txt(1 hunks)faststack/faststack.egg-info/top_level.txt(1 hunks)faststack/faststack/app.py(10 hunks)faststack/faststack/app.py.bak(1 hunks)faststack/faststack/io/executable_validator.py(1 hunks)faststack/faststack/io/helicon.py(3 hunks)faststack/faststack/qml/Components.qml(4 hunks)faststack/faststack/qml/FilterDialog.qml(1 hunks)faststack/faststack/qml/Main.qml(3 hunks)faststack/faststack/qml/Main.qml.bak(1 hunks)faststack/faststack/tests/test_executable_validator.py(1 hunks)faststack/faststack/ui/keystrokes.py(1 hunks)faststack/faststack/ui/keystrokes.py.bak(1 hunks)faststack/faststack/ui/provider.py(4 hunks)faststack/faststack/ui/provider.py.bak(1 hunks)faststack/pyproject.toml(1 hunks)run_app.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
faststack/faststack/ui/provider.py (1)
faststack/faststack/app.py (1)
get_filter_string(141-143)
faststack/faststack/app.py (2)
faststack/faststack/io/executable_validator.py (1)
validate_executable_path(23-95)faststack/faststack/ui/provider.py (1)
resetZoomPan(276-278)
🪛 Ruff (0.14.5)
faststack/faststack/io/helicon.py
69-69: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
76-76: subprocess call: check for execution of untrusted input
(S603)
84-84: Consider moving this statement to an else block
(TRY300)
86-86: Redundant exception object included in logging.exception call
(TRY401)
89-89: Redundant exception object included in logging.exception call
(TRY401)
faststack/faststack/io/executable_validator.py
47-47: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
91-91: Do not catch blind exception: Exception
(BLE001)
92-92: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
110-110: Consider moving this statement to an else block
(TRY300)
faststack/faststack/tests/test_executable_validator.py
40-40: Unused lambda argument: self
(ARG005)
62-62: Unused lambda argument: self
(ARG005)
65-65: Unpacked variable is_valid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
65-65: Unpacked variable error is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
122-122: Unused lambda argument: self
(ARG005)
126-126: Unpacked variable error is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
faststack/faststack/app.py
632-632: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
641-641: subprocess call: check for execution of untrusted input
(S603)
653-653: Redundant exception object included in logging.exception call
(TRY401)
656-656: Redundant exception object included in logging.exception call
(TRY401)
🔇 Additional comments (31)
faststack/faststack.egg-info/top_level.txt (1)
1-1: Top-level package marker looks correct
faststackas the sole top-level entry matches the package name; no issues here.faststack/faststack.egg-info/SOURCES.txt (1)
1-14: SOURCES list is consistent with metadataThis looks like a normal, tool-generated SOURCES list for the distribution; nothing stands out as problematic.
docs/COLOR_PROFILE_FIX.md (1)
1-121: ICC color management doc looks solidThe problem description, root cause, and proposed ICC handling pipeline are clear and technically coherent; nothing here conflicts with typical Pillow/TurboJPEG usage. No blocking changes needed from this doc alone.
faststack/faststack/ui/keystrokes.py (1)
44-47: Ctrl+0 →reset_zoom_panwiring looks good; ensure target method exists and is documentedThe new modifier mapping:
self.modifier_key_map = { (Qt.Key_C, Qt.ControlModifier): "copy_path_to_clipboard", (Qt.Key_0, Qt.ControlModifier): "reset_zoom_pan", }is consistent with the existing design:
handle_key_presswill call_call("reset_zoom_pan"), which prefers a QML method oncontroller.main_windowand falls back tocontroller.reset_zoom_pan.Two follow-ups to double-check:
AppController(and/or the QML root) must actually expose areset_zoom_pan()callable for this to be effective.- Any in-app “Key Bindings” / help text should include the new
Ctrl+0shortcut so users discover it.Functionally, the addition itself is fine.
faststack/ChangeLog.md (1)
3-38: LGTM! Well-documented release notes.The changelog clearly documents the new features, security enhancements, and fixes. The security section is particularly thorough, documenting the executable validation, subprocess hardening, and input validation improvements.
faststack/faststack/qml/Main.qml (3)
83-88: LGTM! Clear visual indication of active filter.The yellow bold text provides good visual feedback when a filter is active, and the visibility binding ensures it's only shown when needed.
203-215: LGTM! Well-structured directory path display.The implementation correctly:
- Uses spacers for centering
- Elides long paths in the middle (preserving start/end)
- Constrains maximum width
- Aligns vertically in the title bar
276-276: LGTM! Help text updated consistently.The Ctrl+0 shortcut documentation is clear and matches the implemented behavior.
faststack/faststack/tests/test_executable_validator.py (6)
14-18: LGTM! Correct test for empty path validation.The test properly verifies that an empty path is rejected with an appropriate error message.
21-25: LGTM! Correct test for nonexistent file validation.The test properly verifies that a nonexistent file is rejected with an appropriate error message.
28-48: LGTM! Comprehensive mocking for valid path test.The test properly mocks all required path checks to simulate a valid Photoshop executable in a safe location.
70-83: LGTM! Correct test for non-executable file validation.The test properly verifies that a .txt file is rejected as non-executable on Windows.
86-95: LGTM! Correct test for Windows executable detection.The test properly verifies that
_is_executablecorrectly identifies .exe files as executable and .txt files as non-executable on Windows.
98-108: LGTM! Appropriate test for subpath logic.The test uses mocking to verify the
_is_subpathlogic, which is appropriate given the difficulty of testing with real filesystem paths.faststack/faststack/io/helicon.py (3)
30-39: LGTM! Excellent security hardening with executable validation.The addition of
validate_executable_pathensures that the Helicon Focus executable is validated before execution, checking for existence, executability, and safe location.
48-52: LGTM! Defensive file existence checking.The per-file existence check with warning and skip behavior prevents invalid paths from being written to the temporary file.
75-83: LGTM! Excellent subprocess security hardening.The implementation correctly:
- Uses a list for the command (no shell parsing)
- Explicitly sets
shell=Falseto prevent injection- Redirects file descriptors to DEVNULL
- Uses
close_fds=Trueto prevent descriptor leakagefaststack/faststack/qml/FilterDialog.qml (3)
1-23: LGTM! Well-structured theme-aware dialog.The dialog properly integrates with the Material theme and adapts its background based on dark/light mode.
36-51: LGTM! Proper text field bindings and keyboard handling.The TextField correctly:
- Binds to filterDialog.filterString
- Updates the property on text changes
- Handles Return key to accept the dialog
- Provides focus and text selection
62-69: LGTM! Correct initialization on dialog open.The onOpened handler properly loads the current filter string, populates the TextField, and sets focus with text selected for easy editing.
faststack/faststack/io/executable_validator.py (2)
98-103: LGTM! Correct platform-specific executable detection.The function properly handles Windows (.exe extension) and Unix-like (execute permission) systems.
106-112: LGTM! Robust subpath detection.The function correctly uses
resolve()andrelative_to()to determine if a path is a subpath of another, with proper exception handling.faststack/faststack/ui/provider.py (4)
61-63: LGTM! Well-named signals for new UI features.The three new signals clearly communicate their purpose:
resetZoomPanRequestedfor zoom reset functionalitystackSummaryChangedfor stack updatesfilterStringChangedfor filter state changes
154-162: LGTM! Correct property naming convention.Renaming from
get_stack_summarytostackSummaryfollows proper QML property naming conventions and uses the appropriatestackSummaryChangedsignal for notifications.
174-182: LGTM! Well-designed read-only properties.Both properties are correctly implemented:
filterStringprovides read-only access with change notificationcurrentDirectoryis appropriately marked as constant since the directory doesn't change at runtime
275-278: LGTM! Clean slot implementation for zoom reset.The
resetZoomPanslot correctly emits the signal for QML to handle the zoom/pan reset operation.faststack/faststack.egg-info/requires.txt (1)
1-8: Version constraints are secure; no issues found.Security advisory verification confirms that all specified package versions avoid known vulnerabilities. The numpy 2.0+ minimum version constraint explicitly avoids older CVEs (all found advisories affect versions < 1.22), and Pillow 10.0+ is well above the patched version (8.1.2). Other packages show no security advisories. The requirements are appropriately configured.
faststack/faststack/app.py (4)
121-157: Filter string signal wiring looks goodEmitting
self.ui_state.filterStringChangedin bothapply_filterandclear_filterkeeps QML in sync with the active filename filter and matches the new footer UI. No functional issues seen; just ensure theUIState.filterStringproperty is bound to this signal as expected in QML.
175-185: Resize logging clarification is safe and helpfulThe updated log message explicitly noting “(physical pixels)” improves diagnostics for HiDPI issues without changing behavior. No further action needed.
356-371: Stack summary signaling is consistent with new UIEmitting
ui_state.stackSummaryChangedinend_current_stackandclear_all_stacks, and documenting thatclear_all_stacks()already handles the signal inlaunch_helicon, keeps the stack summary dialog reactive while avoiding double-emits. This matches the described QML wiring forstackSummary.Also applies to: 393-415, 453-461
669-675: Zoom/pan reset slot: verify backend zoom state stays consistentThe
reset_zoom_panslot cleanly delegates toui_state.resetZoomPan()and gives user feedback via the status bar, which is exactly what you want for Ctrl+0. One thing to double‑check is that QML also updatesisZoomed(and thusAppController.is_zoomed) when handlingresetZoomPanRequested, soget_display_info()and caching logic reflect the “fit to window” state after a reset.
| Metadata-Version: 2.4 | ||
| Name: faststack | ||
| Version: 0.6 | ||
| Summary: Ultra-fast JPG Viewer for Focus Stacking Selection | ||
| Author-email: Alan Rockefeller <alanrockefeller@gmail.com> | ||
| Classifier: Programming Language :: Python :: 3 | ||
| Classifier: License :: OSI Approved :: MIT License | ||
| Classifier: Operating System :: Microsoft :: Windows | ||
| Requires-Python: >=3.11 | ||
| Description-Content-Type: text/markdown | ||
| License-File: LICENSE | ||
| Requires-Dist: PySide6<7.0,>=6.0 | ||
| Requires-Dist: PyTurboJPEG<2.0,>=1.8 | ||
| Requires-Dist: numpy<3.0,>=2.0 | ||
| Requires-Dist: cachetools<6.0,>=5.0 | ||
| Requires-Dist: watchdog<5.0,>=4.0 | ||
| Requires-Dist: typer<1.0,>=0.12 | ||
| Requires-Dist: Pillow<11.0,>=10.0 | ||
| Requires-Dist: pytest<9.0,>=8.0 | ||
| Dynamic: license-file | ||
|
|
||
| # FastStack | ||
|
|
||
| # Version 0.4 - November 2, 2025 | ||
| # By Alan Rockefeller |
There was a problem hiding this comment.
PKG-INFO version is out of sync with pyproject
This PKG-INFO still declares Version: 0.6 while pyproject.toml is at 0.7. That mismatch can cause confusion and packaging issues if this egg-info is used as a source of truth.
Recommend regenerating the egg-info/PKG-INFO from the updated pyproject.toml (e.g., via your normal build/install step) so the recorded version and embedded README content are in sync with the 0.7 release.
🤖 Prompt for AI Agents
In faststack/faststack.egg-info/PKG-INFO around lines 1-25, the recorded Version
(0.6) and embedded metadata are out of sync with pyproject.toml which is at 0.7;
regenerate the egg-info so it reflects pyproject.toml by removing or cleaning
the existing faststack.egg-info and re-running your project’s standard
packaging/build/install step to recreate PKG-INFO (ensuring Version shows 0.7
and the README/license content matches).
| try: | ||
| path = Path(exe_path).resolve() | ||
| except (ValueError, OSError) as e: | ||
| log.error(f"Invalid path format: {exe_path}: {e}") | ||
| return False, f"Invalid path format: {e}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prefer logging.exception for error logging.
Line 47 should use log.exception instead of log.error to include the full traceback, which is helpful for debugging path format errors.
Apply this diff:
try:
path = Path(exe_path).resolve()
except (ValueError, OSError) as e:
- log.error(f"Invalid path format: {exe_path}: {e}")
+ log.exception(f"Invalid path format: {exe_path}")
return False, f"Invalid path format: {e}"📝 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.
| try: | |
| path = Path(exe_path).resolve() | |
| except (ValueError, OSError) as e: | |
| log.error(f"Invalid path format: {exe_path}: {e}") | |
| return False, f"Invalid path format: {e}" | |
| try: | |
| path = Path(exe_path).resolve() | |
| except (ValueError, OSError) as e: | |
| log.exception(f"Invalid path format: {exe_path}") | |
| return False, f"Invalid path format: {e}" |
🧰 Tools
🪛 Ruff (0.14.5)
47-47: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In faststack/faststack/io/executable_validator.py around lines 44 to 48, the
exception handler currently uses log.error which omits the traceback; replace
log.error with log.exception and keep the same message and return value so the
full traceback is recorded for debugging while still returning False and the
error string.
| # Check for suspicious paths (potential directory traversal, etc.) | ||
| try: | ||
| normalized = os.path.normpath(exe_path) | ||
| if ".." in normalized or normalized != str(path): | ||
| log.warning(f"Suspicious path detected: {exe_path}") | ||
| except Exception as e: | ||
| log.error(f"Error normalizing path: {e}") | ||
| return False, f"Path validation error: {e}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Improve exception handling specificity.
The broad Exception catch on line 91 should be more specific, and line 92 should use log.exception instead of log.error for better debugging.
Apply this diff:
# Check for suspicious paths (potential directory traversal, etc.)
try:
normalized = os.path.normpath(exe_path)
if ".." in normalized or normalized != str(path):
log.warning(f"Suspicious path detected: {exe_path}")
- except Exception as e:
- log.error(f"Error normalizing path: {e}")
+ except (ValueError, OSError) as e:
+ log.exception("Error normalizing path")
return False, f"Path validation error: {e}"🧰 Tools
🪛 Ruff (0.14.5)
91-91: Do not catch blind exception: Exception
(BLE001)
92-92: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In faststack/faststack/io/executable_validator.py around lines 86 to 93, the
code currently catches a broad Exception when normalizing the path and logs with
log.error; change the except to catch more specific errors (at minimum TypeError
and ValueError) and replace log.error with log.exception so the traceback is
captured; keep the same return False, f"Path validation error: {e}" behavior for
those specific exceptions and allow any other unexpected exceptions to propagate
rather than being swallowed.
| # Build command list safely | ||
| args = [helicon_exe, "-i", str(tmp_path.resolve())] | ||
|
|
||
| # Parse additional args safely using shlex (handles quotes and escapes properly) | ||
| extra_args = config.get("helicon", "args") | ||
| if extra_args: | ||
| args.extend(extra_args.split()) | ||
| try: | ||
| # Use shlex to properly parse arguments with quotes/escapes | ||
| # On Windows, use posix=False to handle Windows-style paths | ||
| parsed_args = shlex.split(extra_args, posix=(os.name != 'nt')) | ||
| args.extend(parsed_args) | ||
| except ValueError as e: | ||
| log.error(f"Invalid helicon args format: {e}") | ||
| return False, None |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prefer logging.exception over logging.error for consistency.
Line 69 uses log.error but should use log.exception to include the full traceback, consistent with the error handling elsewhere in the file.
Apply this diff:
except ValueError as e:
- log.error(f"Invalid helicon args format: {e}")
+ log.exception(f"Invalid helicon args format: {e}")
return False, None🧰 Tools
🪛 Ruff (0.14.5)
69-69: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In faststack/faststack/io/helicon.py around lines 57 to 70 the except block logs
a parsing error with log.error but should call log.exception to include the
traceback; update the except block to use log.exception(f"Invalid helicon args
format: {e}") (leave the return False, None intact) so the full exception info
is recorded consistently with other handlers.
| MouseArea { | ||
| anchors.fill: parent | ||
| property point lastMousePos: Qt.point(0, 0) | ||
| onPressed: function(mouse) { | ||
| lastMousePos = Qt.point(mouse.x, mouse.y) | ||
| } | ||
| onPositionChanged: function(mouse) { | ||
| var delta = Qt.point(mouse.x - lastMousePos.x, mouse.y - lastMousePos.y) | ||
| root.x += delta.x | ||
| root.y += delta.y | ||
| } | ||
| } |
There was a problem hiding this comment.
Window drag logic will “run away” because lastMousePos is never updated
In the MouseArea drag handler, lastMousePos is set only in onPressed, and delta is always computed from that original point:
onPressed: function(mouse) {
lastMousePos = Qt.point(mouse.x, mouse.y)
}
onPositionChanged: function(mouse) {
var delta = Qt.point(mouse.x - lastMousePos.x, mouse.y - lastMousePos.y)
root.x += delta.x
root.y += delta.y
}Because lastMousePos is not updated after applying delta, subsequent events keep adding the total movement since press, causing exaggerated motion (the window moves farther than the cursor). Update lastMousePos inside onPositionChanged after applying the delta, or switch to DragHandler/Qt.startSystemMove(root) to avoid this.
🤖 Prompt for AI Agents
In faststack/faststack/qml/Main.qml.bak around lines 120 to 131, the MouseArea
drag logic computes delta from a fixed lastMousePos set only on onPressed,
causing cumulative (runaway) movement; update lastMousePos after applying the
delta in onPositionChanged (set lastMousePos = Qt.point(mouse.x, mouse.y)) so
each movement is relative to the previous event, or alternatively replace the
manual logic with a DragHandler or call Qt.startSystemMove(root) on press to
handle window dragging correctly.
| def test_suspicious_path_with_traversal(): | ||
| """Test that paths with directory traversal are flagged.""" | ||
| suspicious_path = r"C:\Program Files\..\Windows\System32\malware.exe" | ||
|
|
||
| with patch('faststack.io.executable_validator.Path') as mock_path: | ||
| mock_path_instance = MagicMock() | ||
| mock_path.return_value.resolve.return_value = mock_path_instance | ||
| mock_path_instance.exists.return_value = True | ||
| mock_path_instance.is_file.return_value = True | ||
| mock_path_instance.suffix.lower.return_value = '.exe' | ||
| mock_path_instance.name = "malware.exe" | ||
| mock_path_instance.__str__ = lambda self: r"C:\Windows\System32\malware.exe" | ||
|
|
||
| # The normalized path will differ from input, triggering warning | ||
| is_valid, error = validate_executable_path(suspicious_path) | ||
| # Should still pass but with a warning logged | ||
| # (in production, you might want to make this fail) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add assertion to verify suspicious path behavior.
The test calls validate_executable_path but doesn't assert the return values. Since the comment indicates it "should still pass but with a warning logged," add an assertion to verify this behavior.
Apply this diff:
# The normalized path will differ from input, triggering warning
- is_valid, error = validate_executable_path(suspicious_path)
+ is_valid, error = validate_executable_path(suspicious_path)
+ # Warning is logged, but validation doesn't fail for path normalization differences
+ assert is_valid or not is_valid # Current behavior: may pass or fail
# Should still pass but with a warning logged
# (in production, you might want to make this fail)Or better, clarify the expected behavior:
# The normalized path will differ from input, triggering warning
- is_valid, error = validate_executable_path(suspicious_path)
- # Should still pass but with a warning logged
- # (in production, you might want to make this fail)
+ with patch('faststack.io.executable_validator._is_subpath', return_value=False):
+ is_valid, error = validate_executable_path(suspicious_path)
+ # Warning is logged for suspicious path, but doesn't fail with allow_custom_paths=True
+ assert is_valid # Default allow_custom_paths=True means it passes with warningCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.5)
62-62: Unused lambda argument: self
(ARG005)
65-65: Unpacked variable is_valid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
65-65: Unpacked variable error is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
| """Maps Qt Key events to application actions.""" | ||
|
|
||
| import logging | ||
| from PySide6.QtCore import Qt | ||
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| class Keybinder: | ||
| def __init__(self, main_window): | ||
| self.main_window = main_window | ||
| self.key_map = { | ||
| # Navigation | ||
| Qt.Key.Key_J: self.main_window.next_image, | ||
| Qt.Key.Key_Right: self.main_window.next_image, | ||
| Qt.Key.Key_K: self.main_window.prev_image, | ||
| Qt.Key.Key_Left: self.main_window.prev_image, | ||
|
|
||
| # View Mode | ||
| Qt.Key.Key_G: self.main_window.toggle_grid_view, | ||
|
|
||
| # Metadata | ||
| Qt.Key.Key_Space: self.main_window.toggle_current_flag, | ||
| Qt.Key.Key_X: self.main_window.toggle_current_reject, | ||
|
|
||
| # Stacking | ||
| Qt.Key.Key_BracketLeft: self.main_window.begin_new_stack, | ||
| Qt.Key.Key_BracketRight: self.main_window.end_current_stack, | ||
|
|
||
| # Actions | ||
| Qt.Key.Key_S: self.main_window.toggle_selection, | ||
| Qt.Key.Key_Enter: self.main_window.launch_helicon, | ||
| Qt.Key.Key_Return: self.main_window.launch_helicon, | ||
|
|
||
| # Stack Management | ||
| Qt.Key.Key_C: self.main_window.clear_all_stacks, | ||
| } | ||
|
|
||
| def handle_key_press(self, event): | ||
| """Handles a key press event from the main window.""" | ||
| log.info(f"Key pressed: {event.key()}") | ||
| action = self.key_map.get(event.key()) | ||
| if action: | ||
| action() | ||
| return True | ||
| return False |
There was a problem hiding this comment.
🧩 Analysis chain
Stale backup keymap and noisy logging
This .bak Keybinder looks like an earlier snapshot:
- It doesn’t handle modifiers, so shortcuts like
Ctrl+0orCtrl+C(mentioned elsewhere in the PR/README) can’t be distinguished from plain0/Cif this code were used. - New actions like “Edit in Photoshop” (
E) and “Copy image path” (Ctrl+C) aren’t mapped here. log.infoon every key press will generate a lot of noise in normal use.
If this file is only a backup, consider either removing it or clearly marking it as outdated to avoid confusion. If it’s meant to be used, I’d recommend:
- Incorporating
event.modifiers()into the key lookup (or usingQKeySequence) so you can support combinations likeCtrl+0andCtrl+C. - Dropping the log level for per‑keypress logging to DEBUG.
🌐 Web query:
In PySide6, what is the recommended way to distinguish key presses with modifiers (e.g., Ctrl+0, Ctrl+C) when handling QKeyEvent, and is using QKeySequence preferable to manual key/modifier checks?
💡 Result:
Short answer: prefer using QKeySequence/QShortcut (or QKeyEvent::keyCombination() / QKeyEvent::matches) rather than manually testing key + modifiers.
Why / how:
- QKeySequence (and QShortcut) express shortcuts in a platform- and layout-aware way (e.g. "Ctrl+C", standard shortcuts, translations). Use QKeySequence or QAction/QShortcut to connect shortcuts to actions. [1][3]
- When you handle raw QKeyEvent, use event.keyCombination() or event.matches(QKeySequence.StandardKey) to compare the event to a sequence/standard shortcut instead of manually checking event.key() + event.modifiers(), which avoids layout/shift/platform pitfalls. [2]
References:
- QKeySequence / keyboard-layout and standard-shortcuts documentation. [1]
- QKeyEvent (keyCombination(), matches(), modifiers()) docs. [2]
- QShortcut (recommended for wiring shortcuts to actions). [3]
🏁 Script executed:
#!/bin/bash
# Find keystrokes.py files in the repo
find . -name "*keystrokes*" -type f 2>/dev/null | head -20Length of output: 81
🏁 Script executed:
#!/bin/bash
# Search for any imports or references to keystrokes.bak
rg -n "keystrokes\.bak" --type py 2>/dev/null || echo "No explicit .bak references found"Length of output: 33
🏁 Script executed:
#!/bin/bash
# Check if .bak file is imported anywhere
rg -n "from.*keystrokes|import.*keystrokes" --type py 2>/dev/null | head -10Length of output: 75
🏁 Script executed:
#!/bin/bash
# Look for mentions of E key, Ctrl+C, Ctrl+0 in README or doc files
find . -name "README*" -o -name "*.md" | xargs grep -l "Ctrl+C\|Ctrl+0\|Edit.*Photoshop\|Copy.*path" 2>/dev/null | head -5Length of output: 46
🏁 Script executed:
#!/bin/bash
# Get the actual keystrokes.py implementation
cat faststack/faststack/ui/keystrokes.pyLength of output: 2901
🏁 Script executed:
#!/bin/bash
# Check README for feature mentions
grep -A 5 -B 5 "Ctrl+C\|Ctrl+0\|Edit.*Photoshop\|Copy.*path" faststack/README.md | head -30Length of output: 1136
🏁 Script executed:
#!/bin/bash
# Check ChangeLog for feature mentions
grep -A 2 -B 2 "Ctrl+C\|Ctrl+0\|Edit\|Copy" faststack/ChangeLog.md | head -40Length of output: 546
Remove the unused backup file
Verification confirms the .bak file is stale and never used—it's not imported anywhere in the codebase. The actual keystrokes.py correctly implements all features with proper modifier handling (Ctrl+C, Ctrl+0, E key), using the recommended approach of checking event.modifiers() against a separate modifier_key_map.
Delete faststack/faststack/ui/keystrokes.py.bak to keep the repository clean.
🤖 Prompt for AI Agents
In faststack/faststack/ui/keystrokes.py.bak lines 1-45, this is a stale backup
file that is not used or imported anywhere; delete the file from the working
tree and repository (e.g., git rm faststack/faststack/ui/keystrokes.py.bak) and
commit the change so the backup is removed from the repo; verify there are no
remaining references to this filename in the codebase before pushing.
1 participant
devicePixelRatioin display size calculations.
Summary by CodeRabbit
Release Notes v0.7
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.