Test#27
Conversation
…yproject metadata
…yproject metadata
WalkthroughAdds guarded (optional) Qt imports, a new ImageEditor.reset_edits() API, safer background task submission and histogram re-queueing, buffer keepalive for QImage usage, reduced non-debug logging verbosity, packaging metadata removals, pytest config, and a logging reproduction script. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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: 1
🧹 Nitpick comments (7)
.gitignore (1)
27-27: Consider reorganizing entries for better logical grouping.The three new entries follow best practices (ignoring generated packaging metadata, runtime data, and deleted documentation files). However, their placement could be improved:
- Line 31 (
faststack.egg-info/): Should be in the "# Build outputs" section (around line 9–12) alongsidedist/,build/, and*.spec.- Line 30 (
var/): Currently placed after.mypy_cache. Consider moving it under a new "# Runtime/Data" section for clarity, or add an inline comment explaining its purpose.- Line 27 (
AGENTS.md): Placement here is logical (alongsideWARP.mdandprompt.md), but consider adding a comment if it's a generated or auto-managed file.🔎 Suggested reorganization
# Virtual environments .venv/ venv/ # Python cache __pycache__/ *.pyc # Build outputs dist/ build/ *.spec +faststack.egg-info/ # Logs *.log # OS cruft .DS_Store Thumbs.db # IDE .vscode/ .idea/ # Documentation/Generated prompt.md WARP.md AGENTS.md # Caches faststack/.mypy_cache/ .mypy_cache/ # Runtime/Data +var/Also applies to: 30-31
faststack/logging_setup.py (1)
15-20: Update docstring to reflect WARNING level.The docstring on line 19 states "sets to INFO to reduce noise," but the implementation now sets the level to WARNING when debug is False (line 40). Consider updating the docstring for consistency.
🔎 Proposed fix
def setup_logging(debug: bool = False): """Sets up logging to a rotating file in the app data directory. Args: - debug: If True, sets log level to DEBUG. Otherwise, sets to INFO to reduce noise. + debug: If True, sets log level to DEBUG. Otherwise, sets to WARNING to reduce noise. """faststack/imaging/prefetch.py (1)
14-19: Narrow exception handling to ImportError.The broad
except Exception:catches all exceptions, which can mask unexpected errors during import. Since the intent is to handle optional PySide6 dependencies, catchingImportErrorspecifically would be more precise and maintainable.🔎 Proposed fix
try: from PySide6.QtCore import QTimer from PySide6.QtGui import QImage -except Exception: +except ImportError: QTimer = None QImage = Nonetools/reproduce_issue.py (2)
1-4: Remove unused import and consider adding module-level logger.The
osmodule is imported but never used (Line 4). Additionally, based on learnings, every new Python module should expose a top-level logger vialog = logging.getLogger(__name__).Proposed fix
- import logging import sys -import os + +log = logging.getLogger(__name__)
6-8: Consider using__file__for more robust path resolution.Using
os.getcwd()is fragile as it depends on the working directory when the script is executed. A more robust approach would use the script's location.Proposed fix
+import os + # Mock the parts of the app needed for setup_logging # We need to make sure we can import faststack modules -sys.path.append(os.getcwd()) +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))faststack/imaging/editor.py (2)
14-17: Narrow the exception clause toImportError.Catching bare
Exceptionis overly broad and can mask unexpected errors. Since this is guarding an import,ImportError(orModuleNotFoundError) is more appropriate.Proposed fix
try: from PySide6.QtGui import QImage -except Exception: +except ImportError: QImage = None
558-570: MoveQImageavailability check before expensive computation.The check for
QImageavailability occurs after_apply_edits()completes, meaning expensive image processing work is done only to be discarded ifQImageis unavailable. Consider checking earlier to fail fast.Proposed fix
if base is None: return None + if QImage is None: + raise ImportError("PySide6.QtGui.QImage is required for get_preview_data_cached") + # Heavy computation outside lock using snapshot img = self._apply_edits(base, edits=edits, for_export=False) - if QImage is None: - raise ImportError("PySide6.QtGui.QImage is required for get_preview_data_cached") - # The image is in RGB mode after _apply_edits buffer = img.tobytes()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
faststack/tests/dummy_images/test.jpgis excluded by!**/*.jpg
📒 Files selected for processing (57)
.gitignoreChangeLog.mdLICENSEREADME.mdfaststack/__init__.pyfaststack/app.pyfaststack/config.pyfaststack/faststack.egg-info/PKG-INFOfaststack/faststack.egg-info/SOURCES.txtfaststack/faststack.egg-info/dependency_links.txtfaststack/faststack.egg-info/entry_points.txtfaststack/faststack.egg-info/requires.txtfaststack/faststack.egg-info/top_level.txtfaststack/faststack/AGENTS.mdfaststack/faststack/tests/test_output.txtfaststack/imaging/cache.pyfaststack/imaging/editor.pyfaststack/imaging/jpeg.pyfaststack/imaging/metadata.pyfaststack/imaging/prefetch.pyfaststack/io/executable_validator.pyfaststack/io/helicon.pyfaststack/io/indexer.pyfaststack/io/sidecar.pyfaststack/io/watcher.pyfaststack/logging_setup.pyfaststack/models.pyfaststack/qml/Components.qmlfaststack/qml/DeleteBatchDialog.qmlfaststack/qml/ExifDialog.qmlfaststack/qml/FilterDialog.qmlfaststack/qml/HistogramWindow.qmlfaststack/qml/ImageEditorDialog.qmlfaststack/qml/JumpToImageDialog.qmlfaststack/qml/Main.qmlfaststack/qml/SettingsDialog.qmlfaststack/qml/SingleChannelHistogram.qmlfaststack/test_output.txtfaststack/tests/benchmark_decode.pyfaststack/tests/benchmark_decode_bilinear.pyfaststack/tests/check_turbo.pyfaststack/tests/debug_metadata.pyfaststack/tests/test_cache.pyfaststack/tests/test_editor.pyfaststack/tests/test_editor_rotation.pyfaststack/tests/test_executable_validator.pyfaststack/tests/test_metadata.pyfaststack/tests/test_new_features.pyfaststack/tests/test_pairing.pyfaststack/tests/test_prefetch_logic.pyfaststack/tests/test_sidecar.pyfaststack/ui/keystrokes.pyfaststack/ui/provider.pyfaststack/verify_wb.pypyproject.tomlrequirements.txttools/reproduce_issue.py
💤 Files with no reviewable changes (7)
- faststack/faststack/AGENTS.md
- faststack/faststack.egg-info/requires.txt
- faststack/faststack.egg-info/dependency_links.txt
- faststack/faststack.egg-info/SOURCES.txt
- faststack/faststack.egg-info/PKG-INFO
- faststack/faststack.egg-info/entry_points.txt
- faststack/faststack.egg-info/top_level.txt
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-08T04:46:41.632Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: Applies to faststack/faststack/**/test_*.py : Place new test cases in `test_*.py` files located beside the modules being tested
Applied to files:
pyproject.toml
📚 Learning: 2025-12-08T04:46:41.632Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: Applies to faststack/faststack/**/test_*.py : Mock filesystem and Qt dependencies in tests to keep them deterministic
Applied to files:
pyproject.toml
📚 Learning: 2025-12-08T04:46:41.632Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: Applies to faststack/faststack/**/config.py : Document any new INI key in `config.py`
Applied to files:
pyproject.toml
📚 Learning: 2025-12-08T04:46:41.632Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: Applies to faststack/faststack/**/*.py : Every new Python module should expose a top-level `log = logging.getLogger(__name__)` logger
Applied to files:
tools/reproduce_issue.py
📚 Learning: 2025-12-08T04:46:41.632Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: Applies to faststack/faststack/**/app.py : `app.py` is the PySide6 entrypoint that coordinates image indexing, caching, and QML screens
Applied to files:
faststack/imaging/editor.pyfaststack/ui/provider.pyfaststack/app.pyfaststack/imaging/prefetch.py
🧬 Code graph analysis (2)
tools/reproduce_issue.py (1)
faststack/logging_setup.py (1)
setup_logging(15-53)
faststack/app.py (1)
faststack/imaging/editor.py (1)
reset_edits(198-202)
🪛 Ruff (0.14.10)
faststack/imaging/editor.py
16-16: Do not catch blind exception: Exception
(BLE001)
560-560: Avoid specifying long messages outside the exception class
(TRY003)
faststack/imaging/prefetch.py
17-17: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (12)
pyproject.toml (2)
10-10: LGTM - Minor metadata cleanup.Removing the email field from author information is acceptable.
41-46: LGTM - Pytest configuration looks good.The pytest configuration is well-structured with appropriate test discovery patterns and exclusions. Disabling the cache provider and doctest plugins via
addoptscan help with test reproducibility.faststack/logging_setup.py (1)
40-40: LGTM - Reduced logging verbosity.Changing the default log level from INFO to WARNING in non-debug mode will reduce log noise in production environments. This is a reasonable default for better signal-to-noise ratio in logs.
faststack/imaging/prefetch.py (1)
524-524: LGTM - Correct handling of optional QImage.The conditional format assignment properly handles the case where QImage is not available, defaulting to None. This aligns with the guarded import pattern introduced earlier in the file.
faststack/ui/provider.py (3)
28-29: LGTM - Proper buffer lifetime management.The keepalive deque prevents premature garbage collection of buffers used by QImage. Since QImage doesn't take ownership of the buffer when constructed from bytes, maintaining Python references is essential to prevent use-after-free issues. The maxlen=32 bound provides reasonable memory overhead.
43-44: LGTM - Smart zoom-aware image selection.Adding the
isZoomedcheck ensures that full-resolution images are requested when zoomed, avoiding low-resolution previews. This improves the user experience during zoomed editing operations.
73-73: LGTM - Correct keepalive buffer usage.Appending the buffer to the keepalive deque ensures the underlying memory remains valid while Qt uses the QImage. This is the correct pattern for preventing garbage collection of buffer data.
tools/reproduce_issue.py (1)
12-39: LGTM!The test logic properly resets logging state between runs by clearing and closing handlers, which prevents interference between test cases.
faststack/app.py (3)
2539-2547: LGTM!Good use of the new
reset_edits()public API instead of directly manipulatingcurrent_edits. This ensures thread-safety via the internal lock and properly increments the revision counter.
2651-2670: LGTM!The fallback logic correctly handles the case when preview data is unavailable by:
- Restoring the pending request
- Resetting inflight state
- Restarting the timer for retry
The
RuntimeErrorhandling for executor shutdown is appropriate defensive programming that prevents crashes during application exit.
2782-2788: LGTM!Consistent error handling pattern with the histogram executor. Properly catches
RuntimeErrorduring shutdown and clears the inflight flag to prevent the worker from appearing stuck.faststack/imaging/editor.py (1)
198-202: LGTM!Clean implementation of the new public API. Thread-safe via the lock, and properly increments the revision counter to invalidate cached previews.
| [project.optional-dependencies] | ||
| gui = ["PySide6>=6.0,<7.0"] | ||
| dev = ["pytest>=8.0,<9.0"] |
There was a problem hiding this comment.
Critical syntax error: TOML sections not valid in requirements.txt.
The [project.optional-dependencies] section uses TOML syntax, which is not valid in requirements.txt files. This will cause installation failures. The requirements.txt format only supports simple dependency specifications like package==version.
Based on the AI summary and pyproject.toml, it appears these optional dependencies should be defined in pyproject.toml under [project.optional-dependencies], not in requirements.txt.
🔎 Proposed fix
Option 1 (Recommended): Remove these lines from requirements.txt
If these optional dependencies are already properly defined in pyproject.toml (which they appear to be based on the AI summary), simply remove lines 6-10 from requirements.txt:
Pillow==10.* # fallback decode; keep it
-
-
-[project.optional-dependencies]
-gui = ["PySide6>=6.0,<7.0"]
-dev = ["pytest>=8.0,<9.0"]Option 2: Convert to valid requirements.txt extras syntax
If you want to keep optional dependencies in requirements.txt, use valid pip extras syntax (though this is less common with modern Python packaging):
-[project.optional-dependencies]
-gui = ["PySide6>=6.0,<7.0"]
-dev = ["pytest>=8.0,<9.0"]
+# Optional: pip install -r requirements-gui.txt
+# PySide6>=6.0,<7.0
+# Optional: pip install -r requirements-dev.txt
+# pytest>=8.0,<9.0However, Option 1 is strongly recommended since pyproject.toml is the modern, standard way to define optional dependencies.
📝 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.
| [project.optional-dependencies] | |
| gui = ["PySide6>=6.0,<7.0"] | |
| dev = ["pytest>=8.0,<9.0"] |
🤖 Prompt for AI Agents
requirements.txt lines 8-10: the file contains a TOML-style
[project.optional-dependencies] section which is invalid in requirements.txt;
remove the entire TOML section from requirements.txt (lines 8-10) so the file
only contains plain dependency specifiers, and ensure the optional dependencies
are declared in pyproject.toml under [project.optional-dependencies] instead
(or, if you must keep them in requirements.txt, convert them to valid pip extras
syntax).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
faststack/imaging/metadata.py (1)
52-57: Prefer the context manager for cleaner resource management.The manual
try/finallyachieves the same resource cleanup as thewithstatement but is more verbose. Since_getexif()returns the EXIF data (not a reference to the image), there's no benefit to closing the image before checking ifexifisNone.🔎 Proposed refactor using context manager
- img = Image.open(path) - try: + with Image.open(path) as img: exif = img._getexif() - finally: - img.close() -faststack/io/sidecar.py (1)
26-26: Move the import to module level.Importing
dataclassesinside the function is unconventional. Moving it to the top of the file with other imports improves readability.🔎 Proposed refactor
At the top of the file, after line 4:
import json +import dataclasses import loggingThen remove line 26 from the function:
try: # Handle legacy keys # Legacy 'flag' and 'reject' do not map to current EntryMetadata fields, # so they will be filtered out by valid_keys check below. # stack_id IS in the current model, so we keep it (don't delete it). # Filter out unknown keys - import dataclasses valid_keys = {f.name for f in dataclasses.fields(EntryMetadata)}faststack/io/executable_validator.py (1)
103-110: The logic is correct but slightly redundant.The early return for
.exefiles (lines 103-105) enables cross-platform testing with mocked Windows executables. However, the Windows branch (lines 107-108) now duplicates the.execheck. This is harmless but slightly redundant.🔎 Simplified logic (optional)
def _is_executable(path: Path) -> bool: """Check if a file is executable (has .exe extension on Windows).""" # Always accept .exe extension (mocked tests might run on Linux) if path.suffix.lower() == '.exe': return True - if os.name == 'nt': # Windows - return path.suffix.lower() == '.exe' - else: # Unix-like + # Unix-like: check execute permission + if os.name != 'nt': return os.access(path, os.X_OK) + + return Falsefaststack/imaging/editor.py (2)
14-17: Consider catchingImportErrorinstead ofException.The broad
Exceptioncatch could mask unexpected errors during import (e.g., syntax errors in PySide6 modules). For optional dependency handling,ImportErroris more precise and follows best practices.🔎 Proposed refinement
try: from PySide6.QtGui import QImage -except Exception: +except ImportError: QImage = None
558-561: Consider checking QImage availability earlier for fail-fast behavior.The QImage availability check occurs after the expensive
_apply_editscomputation. Moving this validation to the beginning ofget_preview_data_cachedwould fail immediately, saving CPU cycles when PySide6 is unavailable.🔎 Proposed fail-fast check
Move the check to the beginning of the method (after line 538):
def get_preview_data_cached(self, allow_compute: bool = True) -> Optional[DecodedImage]: """Return cached preview if available, otherwise compute and cache. Args: allow_compute: If False, returns None immediately if cache is stale (avoids blocking). """ + if QImage is None: + raise ImportError("PySide6.QtGui.QImage is required for get_preview_data_cached") + with self._lock: # Check cache validityThen remove lines 559-560.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignorefaststack/imaging/editor.pyfaststack/imaging/metadata.pyfaststack/io/executable_validator.pyfaststack/io/sidecar.pyfaststack/tests/test_sidecar.pypyproject.toml
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-08T04:46:41.632Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: Applies to faststack/faststack/io/**/*.py : Validate all user-supplied paths before invoking external tools such as Helicon Focus
Applied to files:
faststack/io/executable_validator.py
📚 Learning: 2025-12-08T04:46:41.632Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: Applies to faststack/faststack/**/app.py : `app.py` is the PySide6 entrypoint that coordinates image indexing, caching, and QML screens
Applied to files:
faststack/imaging/editor.py
📚 Learning: 2025-12-08T04:46:41.632Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: Applies to faststack/faststack/**/test_*.py : Place new test cases in `test_*.py` files located beside the modules being tested
Applied to files:
pyproject.toml
📚 Learning: 2025-12-08T04:46:41.632Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: Applies to faststack/faststack/**/test_*.py : Mock filesystem and Qt dependencies in tests to keep them deterministic
Applied to files:
pyproject.toml
📚 Learning: 2025-12-08T04:46:41.632Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: Applies to faststack/faststack/**/config.py : Document any new INI key in `config.py`
Applied to files:
pyproject.toml
🧬 Code graph analysis (3)
faststack/io/sidecar.py (1)
faststack/models.py (1)
EntryMetadata(15-25)
faststack/imaging/editor.py (1)
faststack/ui/provider.py (2)
whites(913-914)whites(917-920)
faststack/tests/test_sidecar.py (1)
faststack/io/sidecar.py (1)
save(83-112)
🪛 Ruff (0.14.10)
faststack/io/sidecar.py
31-31: Do not catch blind exception: Exception
(BLE001)
faststack/imaging/editor.py
16-16: Do not catch blind exception: Exception
(BLE001)
560-560: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (9)
faststack/io/sidecar.py (2)
13-33: LGTM! The metadata filtering logic is robust.The helper correctly filters legacy and unknown keys, providing a clean migration path. The broad exception catch (flagged by ruff) is justified here since this parses user-editable JSON that may contain arbitrary legacy data or corruption.
69-69: Correct usage of the new helper.Replacing direct
EntryMetadata(**meta)with_entrymetadata_from_json(meta)provides robust handling of legacy and unknown fields during sidecar loading.pyproject.toml (2)
10-10: LGTM!Removing the email field is a valid metadata change with no functional impact.
41-46: LGTM! Pytest configuration is well-structured.The test discovery paths, file patterns, and exclusions follow best practices and align with the project structure.
faststack/io/executable_validator.py (2)
69-70: LGTM! Stricter validation improves security.The change correctly enforces executable name matching when
allow_custom_paths=False, replacing a warning with a hard failure. This prevents execution of unexpectedly named executables.
92-93: LGTM! Correctly blocks suspicious paths.The hard failure on suspicious path detection when
allow_custom_paths=Falseprovides better protection against potential directory traversal attacks.faststack/tests/test_sidecar.py (2)
46-51: LGTM! Correctly validates legacy field handling.The test properly verifies that legacy fields (
flag,reject) are dropped whilestack_idis preserved, includingNonevalues.
61-70: LGTM! Save test correctly reflects model changes.The test now validates that
stack_idis properly persisted to JSON, aligning with the updatedEntryMetadatamodel.faststack/imaging/editor.py (1)
198-203: LGTM!The new
reset_edits()method provides a clean, thread-safe API for resetting edits and properly increments the revision counter to invalidate caches.
|
|
||
|
|
||
| def _apply_edits(self, img: Image.Image, edits: Optional[Dict[str, Any]] = None, *, for_export: bool = False) -> Image.Image: | ||
| def _apply_edits(self, img: Image.Image, edits: Optional[Dict[str, Any]] = None, *, for_export: bool = True) -> Image.Image: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for _apply_edits calls in the entire codebase excluding editor.py
rg -n '\._apply_edits\(' --glob '!faststack/imaging/editor.py' -A 2 -B 2Repository: AlanRockefeller/faststack
Length of output: 2322
🏁 Script executed:
# Check the current default value of for_export in _apply_edits method
sed -n '278p' faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 197
🏁 Script executed:
# Check git history to see what the previous default was
git log -p --follow -- faststack/imaging/editor.py | grep -A 10 "def _apply_edits" | head -30Repository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
# Check if the test files that call _apply_edits without for_export are working
# by examining test_editor_rotation.py to understand expected behavior
sed -n '125,135p' faststack/tests/test_editor_rotation.pyRepository: AlanRockefeller/faststack
Length of output: 395
🏁 Script executed:
# Check the test_new_features.py calls to see if they expect a specific behavior
sed -n '45,60p' faststack/tests/test_new_features.pyRepository: AlanRockefeller/faststack
Length of output: 623
Address breaking change from for_export=False to for_export=True default.
External code depends on the default value: verify_wb.py (lines 25, 48) and test_new_features.py (lines 52, 79) call _apply_edits() without explicitly passing for_export. Changing the default from False to True breaks these callers' behavior without code modification. Either revert the default or update all dependent call sites.
🤖 Prompt for AI Agents
In faststack/imaging/editor.py around line 278, the default for the _apply_edits
signature was changed to for_export=True which breaks external callers that rely
on the old default; revert the default back to for_export=False to preserve
backward compatibility, or (alternatively) update all internal and external call
sites (e.g., verify_wb.py lines ~25 and ~48 and test_new_features.py lines ~52
and ~79) to pass an explicit for_export value matching the intended behavior;
prefer reverting the default to False unless you can update and run all
dependent tests to ensure no behavior changes.
| arr = np.array(img, dtype=np.float32) | ||
| black_point = -blacks * 40 | ||
| white_point = 255 - whites * 40 | ||
| white_point = 255 + whites * 40 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual code around line 356
sed -n '350,360p' faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 545
🏁 Script executed:
# Now check the comment and auto_levels calculation around lines 510-530
sed -n '510,530p' faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 847
🏁 Script executed:
# Check for whites property usage in provider.py
rg -nP '\bwhites\b' faststack/ui/provider.py -C 3Repository: AlanRockefeller/faststack
Length of output: 711
Update the outdated comment to reflect the current formula.
The white_point formula is correctly implemented as 255 + whites * 40 (line 356), and the auto_levels calculation properly derives whites from this formula (lines 523-527). However, the comment on line 516 still references the old formula 255 - whites * 40 and must be updated to 255 + whites * 40 to match the actual code.
Required comment update
# Calculate parameters to map p_low->0 and p_high->255
# Logic matches _apply_edits:
# black_point = -blacks * 40
-# white_point = 255 - whites * 40
+# white_point = 255 + whites * 40Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In faststack/imaging/editor.py around lines 356 and 516 (also referencing
auto_levels logic at 523-527), the inline comment on line 516 still describes
the old formula `255 - whites * 40` while the implemented code uses `white_point
= 255 + whites * 40`; update the comment text on line 516 to state `255 + whites
* 40` so it matches the actual formula and the auto_levels derivation on lines
523-527.
| assert sm.data.last_index == 42 | ||
| assert len(sm.data.entries) == 2 |
There was a problem hiding this comment.
Remove duplicate assertions.
Lines 43-44 duplicate the assertions from lines 41-42. These should be removed.
🔎 Proposed fix
assert sm.data.last_index == 42
assert len(sm.data.entries) == 2
- assert sm.data.last_index == 42
- assert len(sm.data.entries) == 2
# flag and reject are legacy and not in current model, so they are dropped.📝 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.
| assert sm.data.last_index == 42 | |
| assert len(sm.data.entries) == 2 | |
| assert sm.data.last_index == 42 | |
| assert len(sm.data.entries) == 2 | |
| # flag and reject are legacy and not in current model, so they are dropped. |
🤖 Prompt for AI Agents
In faststack/tests/test_sidecar.py around lines 43-44, the two assertions are
duplicates of assertions at lines 41-42; remove the duplicate lines (assert
sm.data.last_index == 42 and assert len(sm.data.entries) == 2) so the test
contains only the original assertions and remains functionally equivalent.
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.