fix: align CTk tree smoke test and harden download/executor handling#2
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
建立 ui_ctk.py 骨架（__init__ 狀態初始化、smoke/full UI 分支、run/on_closing/log_message/notify_* 方法、stub 方法）， 並新增 test_ui_ctk.py smoke 測試（含 TCL_LIBRARY 自動修正）共 4 個測試全部通過。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
新增 _build_download_controls、choose_download_path、update_thread_count、toggle_panels 方法， 並補充 download_selected / toggle_pause stub，使 test_has_download_controls 等 8 項測試全數通過。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…oggle_pause 文字 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… btn Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…column jitter Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces ttkbootstrap UI with a CustomTkinter (CTk) implementation (ui_ctk), adds CTk-based FileTree and DownloadsPanel tokenization, CI now uses pyproject/uv with expanded matrix, adds app_utils helpers (executor rebuild, tk config, partial-file cleanup), backend partial-download cleanup, many tests, and detailed migration/design docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as WebsiteCopierCtk
participant Backend
participant Queue as ProcessingQueues
participant Downloads as DownloadsPanel
User->>UI: Enter URL & Start Scan
UI->>Backend: start_scan(url)
Backend->>Queue: enqueue dir/file items
Queue->>UI: poll and deliver items
UI->>UI: _rebuild_visible() / _sync_rows()
User->>UI: Select files, Start Download
UI->>Downloads: ensure(item)
Downloads->>Backend: download_file(url, path)
Backend->>Downloads: progress updates / completion
Downloads->>User: update status/progress
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip You can disable sequence diagrams in the walkthrough.Disable the |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| self.ui_manager.pause_event.wait() | ||
| if cancel_event is not None and cancel_event.is_set(): | ||
| self.ui_manager.log_message(f"[Download] Canceled: {file_name}") | ||
| _cleanup_partial_file(file_path) |
There was a problem hiding this comment.
Partial file cleanup attempted while file handle open
Medium Severity
_cleanup_partial_file is called inside the with open(file_path, "wb") as file_handle: block, meaning the file is still open when os.remove() is attempted. On Windows, this will raise an OSError (file in use) that is silently swallowed, so the partial file is never actually deleted — defeating the purpose of the cleanup.
Additional Locations (1)
| self.executor.shutdown(wait=False, cancel_futures=True) | ||
| self.executor = ThreadPoolExecutor(max_workers=self.max_workers) | ||
| # Shutdown old executor after new one is ready | ||
| old_executor.shutdown(wait=True, cancel_futures=False) |
There was a problem hiding this comment.
Executor shutdown blocks UI thread with wait=True
Medium Severity
old_executor.shutdown(wait=True, cancel_futures=False) in update_thread_count is called from a UI callback (the threads CTkOptionMenu command). If downloads are in progress on the old executor, wait=True blocks the main/UI thread until all running tasks complete, freezing the entire UI for potentially a very long time.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the Index Ripper application by modernizing the UI, improving resource management, and hardening the executor lifecycle. These changes collectively aim to provide a more robust and user-friendly experience. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a substantial set of changes, migrating the UI to CustomTkinter, hardening various parts of the application like download cleanup and executor lifecycle, and modernizing the project setup with pyproject.toml. The introduction of new tests and detailed planning documents is a great step towards improving maintainability. My review has identified a critical UI blocking issue, a high-severity packaging bug, and a medium-severity maintainability concern that should be addressed.
| self.executor.shutdown(wait=False, cancel_futures=True) | ||
| self.executor = ThreadPoolExecutor(max_workers=self.max_workers) | ||
| # Shutdown old executor after new one is ready | ||
| old_executor.shutdown(wait=True, cancel_futures=False) |
There was a problem hiding this comment.
The shutdown(wait=True) call will block the main UI thread until all tasks in the old executor have completed. This can freeze the application, especially if there are long-running downloads in progress. To avoid this, you should either use wait=False or run the shutdown in a background thread.
| old_executor.shutdown(wait=True, cancel_futures=False) | |
| old_executor.shutdown(wait=False, cancel_futures=False) |
| dev = ["pytest>=8.0.0"] | ||
|
|
||
| [project.scripts] | ||
| index-ripper = "ui_ctk:main" |
There was a problem hiding this comment.
The script entry point is configured as ui_ctk:main, but the ui_ctk.py file does not contain a main function. The main entry point logic is in index_ripper.py. This will cause the installed script to fail when executed.
| index-ripper = "ui_ctk:main" | |
| index-ripper = "index_ripper:main" |
| def _configure_tk_libraries() -> None: | ||
| """Set Tcl/Tk library env vars for uv-managed Python when missing.""" | ||
| if os.environ.get("TCL_LIBRARY") and os.environ.get("TK_LIBRARY"): | ||
| return | ||
|
|
||
| for prefix in (sys.base_prefix, sys.prefix): | ||
| lib_root = os.path.join(prefix, "lib") | ||
| tcl_library = os.path.join(lib_root, "tcl8.6") | ||
| tk_library = os.path.join(lib_root, "tk8.6") | ||
| if os.path.isfile(os.path.join(tcl_library, "init.tcl")) and os.path.isfile( | ||
| os.path.join(tk_library, "tk.tcl") | ||
| ): | ||
| os.environ.setdefault("TCL_LIBRARY", tcl_library) | ||
| os.environ.setdefault("TK_LIBRARY", tk_library) | ||
| return |
There was a problem hiding this comment.
Hardcoding the Tcl/Tk version numbers (tcl8.6, tk8.6) can make the application brittle. If a future Python version in the supported range (>=3.11) bundles a different version of Tcl/Tk (e.g., 8.7), this logic will fail. It would be more robust to dynamically find the library directories using glob.
For example:
import glob
# ...
lib_root = os.path.join(prefix, "lib")
tcl_dirs = glob.glob(os.path.join(lib_root, "tcl8.*"))
tk_dirs = glob.glob(os.path.join(lib_root, "tk8.*"))
if tcl_dirs and tk_dirs:
tcl_library = sorted(tcl_dirs)[-1]
tk_library = sorted(tk_dirs)[-1]
# ... rest of the logic![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
ui_ttkb.py (1)
855-882:⚠️ Potential issue | 🟠 MajorFiltering invalidates the search snapshot.
This rebuilds the visible tree, but
full_tree_backupstill points at the pre-filter scan. After toggling file-type filters, clearing the search box can therefore resurrect rows the user just hid. Clear or rebuild the snapshot here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui_ttkb.py` around lines 855 - 882, apply_filters rebuilds the visible tree but does not update the saved snapshot, so self.full_tree_backup still references the pre-filter state; update/clear the snapshot inside apply_filters (after repopulating the tree) by either clearing self.full_tree_backup or rebuilding it from the current tree contents (e.g. replace with a fresh list from self._all_tree_items() or equivalent) so that subsequent search clears or toggles use the current filtered view; modify apply_filters accordingly and ensure any access to full_tree_backup remains consistent with the existing folders/files locks.backend.py (1)
312-328:⚠️ Potential issue | 🟠 MajorMove partial-file cleanup out of the active
open()context.
_cleanup_partial_file(file_path)is called beforefile_handlehas been closed. That works on Unix, but on Windowsos.remove()will fail for an open file, so the cancel/stop paths can still leave stale partial downloads behind. Exit thewith open(...)block first, then delete the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend.py` around lines 312 - 328, The partial-file cleanup (_cleanup_partial_file(file_path)) is being called while the file is still open inside the with open(file_path, "wb") as file_handle: block (in the download loop handling cancel_event and self.should_stop), which will fail on Windows; modify the logic in the function that contains this loop so that when a cancel or stop condition is detected you break/return from the with-block first (closing file_handle) and perform _cleanup_partial_file(file_path) only after exiting the with context, and still call self.ui_manager.update_download_status(file_path, "Canceled") (with the same AttributeError guard) after the file is closed.README.md (1)
134-151:⚠️ Potential issue | 🟡 MinorMinor: Inconsistent numbered list format in macOS guide.
The numbered list uses
1)format but should use1.for consistency with markdown best practices, or all items should use incremental numbers (1, 2, 3, 4) instead of repeated1).📝 Suggested fix
-1) Try Finder (recommended first): +1. Try Finder (recommended first): -1) Remove quarantine attribute (Gatekeeper flag) from Terminal: +2. Remove quarantine attribute (Gatekeeper flag) from Terminal: -1) Grant executable permission (inside the .app Contents/MacOS): +3. Grant executable permission (inside the .app Contents/MacOS): -1) Launch from Terminal (to observe logs for troubleshooting): +4. Launch from Terminal (to observe logs for troubleshooting):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 134 - 151, The numbered list in the macOS guide uses repeated "1)" entries; update the list to use proper incremental numeric ordering (e.g., "1.", "2.", "3.", "4.") or at minimum change each "1)" to "1." for consistency with Markdown; update the block that starts with "Try Finder (recommended first):" and the subsequent steps that show the quarantine xattr, chmod, and "Launch from Terminal" instructions so the items read as a correctly-ordered numbered list (reference the visible list items such as "Try Finder (recommended first):", the xattr command line, the chmod command line, and "Launch from Terminal").README_zh.md (1)
134-151:⚠️ Potential issue | 🟡 MinorMinor: Inconsistent numbered list format in macOS guide.
Same issue as in README.md - the numbered list uses repeated
1)instead of sequential numbers.📝 Suggested fix
-1) 使用 Finder 的方式（建議先試） +1. 使用 Finder 的方式（建議先試） -1) 從終端機移除隔離屬性（Gatekeeper 標記） +2. 從終端機移除隔離屬性（Gatekeeper 標記） -1) 賦予執行權限（進入 .app 內部的 Contents/MacOS） +3. 賦予執行權限（進入 .app 內部的 Contents/MacOS） -1) 直接從終端機啟動（可觀察輸出訊息，有助除錯） +4. 直接從終端機啟動（可觀察輸出訊息，有助除錯）🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README_zh.md` around lines 134 - 151, The numbered steps in the macOS guide use repeated "1)" instead of sequential numbering; update the list so each step is numbered sequentially (e.g., 1), 2), 3), 4)) to match README.md formatting and improve readability—locate the block referencing Finder opening, xattr command, chmod command, and terminal launch (mentions `IndexRipper.app`, `xattr -dr com.apple.quarantine`, and `chmod +x "/path/to/IndexRipper.app/Contents/MacOS/IndexRipper"`) and renumber those four list items in order.
🧹 Nitpick comments (3)
test_ui_theme.py (1)
27-28: Avoid asserting that a helper name is absent.This bakes an implementation detail into the suite. A harmless compatibility shim or deprecation wrapper would now fail the test even if the public theming behavior is unchanged.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_ui_theme.py` around lines 27 - 28, The test should stop asserting that the helper name apply_bootstrap_theme is absent; instead verify public theming behavior or API surface that matters. Update test_apply_bootstrap_theme_not_present to either be removed or replaced with a behavioral assertion (e.g., calling the public theme application function or checking rendered output/config after applying a theme) so it no longer depends on the absence of the internal symbol apply_bootstrap_theme.ui_theme.py (1)
103-103: Consider cross-platform font fallback for "SF Pro Text".The font "SF Pro Text" is macOS-specific and won't be available on Windows/Linux. Tkinter/CTk typically falls back to a system default font, but this may result in inconsistent UI appearance across platforms.
Consider specifying a font family tuple with fallbacks:
💡 Suggested improvement
- font=("SF Pro Text", 11, "bold"), + font=("SF Pro Text", "Segoe UI", "Ubuntu", 11, "bold"),Does CustomTkinter CTkFont support font fallback tuples?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui_theme.py` at line 103, Replace the macOS-only font tuple font=("SF Pro Text", 11, "bold") with a cross-platform fallback family tuple such as ("SF Pro Text", "Helvetica", "Arial", "Sans-Serif", 11, "bold") (or use CTkFont creation if you centralize fonts), updating every occurrence where font=("SF Pro Text", 11, "bold") is used so widgets get the first available family; ensure you supply the size and weight in the same tuple or via CTkFont to match existing usage.test_backend.py (1)
141-166: The new partial-file cleanup paths still have no regression test.This test is skipped, so the cancel/error cleanup added in
backend.pyis still manual-only. A pure unit test here can mocksession.get()/iter_content()and assert the partial target is removed without touching the network.If helpful, I can draft that mocked cleanup test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_backend.py` around lines 141 - 166, Unskip and implement a unit test for the new partial-file cleanup in backend.download_file by mocking ui.session.get to return a response whose iter_content triggers a simulated cancellation or exception, then assert the partial file (e.g., "test.txt.part" or the actual temp name used by download_file) is removed after download_file returns/raises; specifically mock MagicMock response.iter_content to raise an exception or yield then clear ui.pause_event to simulate cancel, ensure response.raise_for_status is a no-op, call backend.download_file with the target path and then assert os.path.exists(subdir) is True but the partial file does not exist, and clean up any created files. Use existing symbols ui.session.get, ui.pause_event, and backend.download_file to locate and modify the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 27-28: The CI step "Run unit tests" currently runs a hardcoded
pytest list and omits the new UI regression tests; update the pytest invocation
used in that step (the command starting with "uv run python -m pytest ...") to
either run the full test suite (e.g., "python -m pytest -v") or explicitly
include the missing UI tests by adding "test_ui_ttkb.py" and the CTk smoke test
file to the arguments so the new regressions are executed in CI. Ensure the
updated command still uses the same step name and preserves verbosity.
In `@index_ripper.py`:
- Around line 1188-1192: The UI is blocking because update_thread_count()
replaces self.executor then calls old_executor.shutdown(wait=True); change the
shutdown to be non-blocking by calling old_executor.shutdown(wait=False,
cancel_futures=False) so the old ThreadPoolExecutor drains in the background
instead of freezing the main thread — modify the shutdown call that follows the
reassignment of self.executor (the old_executor variable) accordingly.
In `@ui_ctk.py`:
- Around line 1306-1310: The download target currently discards the original
scanned file's source path (the earlier-stored path variable) and writes every
file directly under self.download_path, causing name collisions; update the
logic that builds file_path (used by downloads_panel.ensure and passed to
backend.download_file) to incorporate the original source path components before
the sanitized filename (e.g., use safe_join(self.download_path, [path,
safe_name]) or join the source path's directory with safe_name), so each file is
placed under its scanned folder hierarchy rather than all in self.download_path;
keep using sanitize_filename for the filename and pass the resulting file_path
and safe_name to downloads_panel.ensure and backend.download_file as before.
- Around line 1224-1233: The current logic bails out when a file's extension
filter is unchecked, preventing creation of its TreeNode so it can never be
restored; change the block that checks var = self.file_types.get(ext) to always
proceed with adding the node (via the existing creation path after
_add_file_type_filter) and, if var is not None and not var.get(), initialize the
created TreeNode as hidden (e.g., set node.hidden = True) instead of
returning—this preserves the node for _on_type_filter_changed (which toggles
node.hidden) to unhide it later; reference normalize_extension,
_add_file_type_filter, file_type_counts, file_type_widgets, file_types,
TreeNode, and _on_type_filter_changed when making the change.
- Around line 1414-1420: The UI's update_thread_count only mutates
self.max_workers but does not resize the actual ThreadPoolExecutor
(self.executor); fix by recreating the executor when thread count changes:
compute and set the new max (as update_thread_count already does), then
atomically replace self.executor with a new
concurrent.futures.ThreadPoolExecutor(max_workers=self.max_workers), shutting
down the old executor (use shutdown(wait=False) or wait=True depending on
desired behavior) to avoid leaking threads; ensure any code that submits tasks
(the place that uses self.executor.submit) continues to reference self.executor
so it will use the new pool.
In `@ui_ttkb.py`:
- Around line 1238-1252: The drag-selection uses _all_tree_items() which builds
items by popping from the end (reverse of UI order), so on_tree_drag_select can
compute the wrong anchor→target range; replace the reversed traversal with a
display-order list built by recursively walking the tree using
tree.get_children() (preserving child order) and use that list in
on_tree_drag_select to compute indices and selection. Update or replace
_all_tree_items() (or add a new helper referenced by on_tree_drag_select) to
recursively gather items via get_children() in the displayed order, then use
that ordered list when computing a = items.index(self.drag_anchor_item) and b =
items.index(target). Ensure behavior and exception handling in
on_tree_drag_select remain unchanged.
- Around line 461-468: In update_thread_count, avoid shutting down the current
ThreadPoolExecutor before replacing it to prevent a race where download_selected
submits to a shut executor; instead create a new
ThreadPoolExecutor(max_workers=self.max_workers) first, atomically swap it into
self.executor (store the old executor in a temp variable), then call
shutdown(wait=False, cancel_futures=True) on the old executor; ensure you still
update self.max_workers and self.threads_var as before.
---
Outside diff comments:
In `@backend.py`:
- Around line 312-328: The partial-file cleanup
(_cleanup_partial_file(file_path)) is being called while the file is still open
inside the with open(file_path, "wb") as file_handle: block (in the download
loop handling cancel_event and self.should_stop), which will fail on Windows;
modify the logic in the function that contains this loop so that when a cancel
or stop condition is detected you break/return from the with-block first
(closing file_handle) and perform _cleanup_partial_file(file_path) only after
exiting the with context, and still call
self.ui_manager.update_download_status(file_path, "Canceled") (with the same
AttributeError guard) after the file is closed.
In `@README_zh.md`:
- Around line 134-151: The numbered steps in the macOS guide use repeated "1)"
instead of sequential numbering; update the list so each step is numbered
sequentially (e.g., 1), 2), 3), 4)) to match README.md formatting and improve
readability—locate the block referencing Finder opening, xattr command, chmod
command, and terminal launch (mentions `IndexRipper.app`, `xattr -dr
com.apple.quarantine`, and `chmod +x
"/path/to/IndexRipper.app/Contents/MacOS/IndexRipper"`) and renumber those four
list items in order.
In `@README.md`:
- Around line 134-151: The numbered list in the macOS guide uses repeated "1)"
entries; update the list to use proper incremental numeric ordering (e.g., "1.",
"2.", "3.", "4.") or at minimum change each "1)" to "1." for consistency with
Markdown; update the block that starts with "Try Finder (recommended first):"
and the subsequent steps that show the quarantine xattr, chmod, and "Launch from
Terminal" instructions so the items read as a correctly-ordered numbered list
(reference the visible list items such as "Try Finder (recommended first):", the
xattr command line, the chmod command line, and "Launch from Terminal").
In `@ui_ttkb.py`:
- Around line 855-882: apply_filters rebuilds the visible tree but does not
update the saved snapshot, so self.full_tree_backup still references the
pre-filter state; update/clear the snapshot inside apply_filters (after
repopulating the tree) by either clearing self.full_tree_backup or rebuilding it
from the current tree contents (e.g. replace with a fresh list from
self._all_tree_items() or equivalent) so that subsequent search clears or
toggles use the current filtered view; modify apply_filters accordingly and
ensure any access to full_tree_backup remains consistent with the existing
folders/files locks.
---
Nitpick comments:
In `@test_backend.py`:
- Around line 141-166: Unskip and implement a unit test for the new partial-file
cleanup in backend.download_file by mocking ui.session.get to return a response
whose iter_content triggers a simulated cancellation or exception, then assert
the partial file (e.g., "test.txt.part" or the actual temp name used by
download_file) is removed after download_file returns/raises; specifically mock
MagicMock response.iter_content to raise an exception or yield then clear
ui.pause_event to simulate cancel, ensure response.raise_for_status is a no-op,
call backend.download_file with the target path and then assert
os.path.exists(subdir) is True but the partial file does not exist, and clean up
any created files. Use existing symbols ui.session.get, ui.pause_event, and
backend.download_file to locate and modify the test.
In `@test_ui_theme.py`:
- Around line 27-28: The test should stop asserting that the helper name
apply_bootstrap_theme is absent; instead verify public theming behavior or API
surface that matters. Update test_apply_bootstrap_theme_not_present to either be
removed or replaced with a behavioral assertion (e.g., calling the public theme
application function or checking rendered output/config after applying a theme)
so it no longer depends on the absence of the internal symbol
apply_bootstrap_theme.
In `@ui_theme.py`:
- Line 103: Replace the macOS-only font tuple font=("SF Pro Text", 11, "bold")
with a cross-platform fallback family tuple such as ("SF Pro Text", "Helvetica",
"Arial", "Sans-Serif", 11, "bold") (or use CTkFont creation if you centralize
fonts), updating every occurrence where font=("SF Pro Text", 11, "bold") is used
so widgets get the first available family; ensure you supply the size and weight
in the same tuple or via CTkFont to match existing usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b40a30b0-b052-4470-a0c4-4880d047131c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.github/workflows/ci.yml.gitignoreREADME.mdREADME_zh.mdbackend.pydocs/plans/2026-03-09-ctk-migration-design.mddocs/plans/2026-03-09-ctk-migration.mddocs/plans/2026-03-10-filetree-redesign-design.mddocs/plans/2026-03-10-filetree-redesign.mddocs/plans/2026-03-10-ui-redesign-design.mddocs/plans/2026-03-10-ui-redesign.mdindex_ripper.pyindex_ripper_ttkb.pypyproject.tomltest_backend.pytest_ui_ctk.pytest_ui_downloads.pytest_ui_theme.pytest_ui_ttkb.pyui_ctk.pyui_downloads.pyui_theme.pyui_ttkb.py
| def update_thread_count(self, _event=None) -> None: | ||
| try: | ||
| self.max_workers = int(self.threads_var.get()) | ||
| self.max_workers = max(1, min(10, int(self.threads_var.get()))) | ||
| except ValueError: | ||
| self.max_workers = 5 | ||
| self.threads_var.set(str(self.max_workers)) | ||
| self.executor.shutdown(wait=False, cancel_futures=True) | ||
| self.executor = ThreadPoolExecutor(max_workers=self.max_workers) |
There was a problem hiding this comment.
Publish the replacement executor before shutting down the old pool.
There is still a handoff window here where download_selected() can submit to self.executor after it has been shut down but before the new pool is assigned, which raises RuntimeError. Swap in the new executor first, then retire the old one.
Suggested change
- self.executor.shutdown(wait=False, cancel_futures=True)
- self.executor = ThreadPoolExecutor(max_workers=self.max_workers)
+ old_executor = self.executor
+ self.executor = ThreadPoolExecutor(max_workers=self.max_workers)
+ old_executor.shutdown(wait=False, cancel_futures=True)📝 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.
| def update_thread_count(self, _event=None) -> None: | |
| try: | |
| self.max_workers = int(self.threads_var.get()) | |
| self.max_workers = max(1, min(10, int(self.threads_var.get()))) | |
| except ValueError: | |
| self.max_workers = 5 | |
| self.threads_var.set(str(self.max_workers)) | |
| self.executor.shutdown(wait=False, cancel_futures=True) | |
| self.executor = ThreadPoolExecutor(max_workers=self.max_workers) | |
| def update_thread_count(self, _event=None) -> None: | |
| try: | |
| self.max_workers = max(1, min(10, int(self.threads_var.get()))) | |
| except ValueError: | |
| self.max_workers = 5 | |
| self.threads_var.set(str(self.max_workers)) | |
| old_executor = self.executor | |
| self.executor = ThreadPoolExecutor(max_workers=self.max_workers) | |
| old_executor.shutdown(wait=False, cancel_futures=True) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui_ttkb.py` around lines 461 - 468, In update_thread_count, avoid shutting
down the current ThreadPoolExecutor before replacing it to prevent a race where
download_selected submits to a shut executor; instead create a new
ThreadPoolExecutor(max_workers=self.max_workers) first, atomically swap it into
self.executor (store the old executor in a temp variable), then call
shutdown(wait=False, cancel_futures=True) on the old executor; ensure you still
update self.max_workers and self.threads_var as before.
| def on_tree_drag_select(self, event): | ||
| try: | ||
| if not self.drag_anchor_item: | ||
| return | ||
| target = self.tree.identify_row(event.y) | ||
| if not target: | ||
| return | ||
| items = self._all_tree_items() | ||
| if self.drag_anchor_item not in items or target not in items: | ||
| return | ||
| a = items.index(self.drag_anchor_item) | ||
| b = items.index(target) | ||
| lo, hi = (a, b) if a <= b else (b, a) | ||
| self.tree.selection_set(items[lo : hi + 1]) | ||
| except tk.TclError: |
There was a problem hiding this comment.
Drag selection is using a reversed traversal.
_all_tree_items() pops from the end of a stack, so siblings are enumerated in the opposite order from the tree UI. The anchor-to-target range here can therefore select the wrong files. Build that range from get_children() recursively in display order instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui_ttkb.py` around lines 1238 - 1252, The drag-selection uses
_all_tree_items() which builds items by popping from the end (reverse of UI
order), so on_tree_drag_select can compute the wrong anchor→target range;
replace the reversed traversal with a display-order list built by recursively
walking the tree using tree.get_children() (preserving child order) and use that
list in on_tree_drag_select to compute indices and selection. Update or replace
_all_tree_items() (or add a new helper referenced by on_tree_drag_select) to
recursively gather items via get_children() in the displayed order, then use
that ordered list when computing a = items.index(self.drag_anchor_item) and b =
items.index(target). Ensure behavior and exception handling in
on_tree_drag_select remain unchanged.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/plans/2026-03-13-code-review-fixes.md (1)
62-70: Consider movingimport osto module level.The
import osstatement is placed inside the function body (line 65). While this works, Python style conventions (PEP 8) recommend placing imports at the module level unless there's a specific reason for lazy loading. Sinceosis a standard library module with negligible import cost, moving it to the top ofbackend.pywould be more idiomatic.♻️ Proposed refactor
At the top of
backend.py(after existing imports):import osThen update the helper:
def _cleanup_partial_file(file_path: str) -> None: """Remove partial download file if it exists.""" try: - import os if os.path.exists(file_path): os.remove(file_path) except OSError: pass # Best-effort cleanup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-13-code-review-fixes.md` around lines 62 - 70, The helper _cleanup_partial_file currently imports os inside the function; move the import to module level in backend.py (add import os alongside existing imports) and remove the local import from inside _cleanup_partial_file so the function simply uses os.path.exists and os.remove; this aligns with PEP8 and avoids repeated local imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-03-13-code-review-fixes.md`:
- Line 13: Task headings skip h2 and use h3 causing an MD001 violation—update
the Markdown headings "Task 1: Fix test_has_treeview Failure", "Task 2: ...",
"Task 3: ...", and "Task 4: ..." to use an h2 (##) level instead of h3 (###) so
heading hierarchy is incremental; apply the same change for the Task 2, Task 3,
and Task 4 headings mentioned in the document to satisfy the MD001 static
analysis hint.
---
Nitpick comments:
In `@docs/plans/2026-03-13-code-review-fixes.md`:
- Around line 62-70: The helper _cleanup_partial_file currently imports os
inside the function; move the import to module level in backend.py (add import
os alongside existing imports) and remove the local import from inside
_cleanup_partial_file so the function simply uses os.path.exists and os.remove;
this aligns with PEP8 and avoids repeated local imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3c5340c-58d5-4761-a387-fdca725afbf8
📒 Files selected for processing (1)
docs/plans/2026-03-13-code-review-fixes.md
|
|
||
| --- | ||
|
|
||
| ### Task 1: Fix test_has_treeview Failure |
There was a problem hiding this comment.
Fix heading level hierarchy.
The heading jumps from h1 to h3, skipping h2. Markdown best practices require incremental heading levels.
📝 Proposed fix
---
-### Task 1: Fix test_has_treeview Failure
+## Task 1: Fix test_has_treeview FailureApply the same fix to Task 2 (line 51), Task 3 (line 136), and Task 4 (line 177).
As per coding guidelines, this matches the static analysis hint MD001.
📝 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.
| ### Task 1: Fix test_has_treeview Failure | |
| ## Task 1: Fix test_has_treeview Failure |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-13-code-review-fixes.md` at line 13, Task headings skip h2
and use h3 causing an MD001 violation—update the Markdown headings "Task 1: Fix
test_has_treeview Failure", "Task 2: ...", "Task 3: ...", and "Task 4: ..." to
use an h2 (##) level instead of h3 (###) so heading hierarchy is incremental;
apply the same change for the Task 2, Task 3, and Task 4 headings mentioned in
the document to satisfy the MD001 static analysis hint.
Code reviewFound 2 issues:
Lines 1186 to 1194 in de95211
Lines 16 to 17 in de95211 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
- Move old_executor.shutdown() to a daemon thread to avoid blocking the Tk main thread (and deadlocking when downloads are paused) - Add main() to ui_ctk.py so pyproject.toml entry point works Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
… dirs - backend.py: move _cleanup_partial_file calls outside the `with open()` block so os.remove succeeds on Windows (file handle closed first) - ui_ctk.py: update_thread_count now rebuilds the executor (was a no-op) - ui_ctk.py: download_selected preserves subfolder structure and creates target directories via os.makedirs, matching legacy behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
…tection
- ui_ctk.py: always create TreeNode for filtered-out types (set hidden
instead of skipping), so re-enabling the filter recovers files
- backend.py: remove unreliable link_text.endswith("/") directory
heuristic that could misclassify files as directories
- test_ui_ctk.py: rename test_has_treeview to test_has_file_tree to
match what it actually asserts
- index_ripper.py, index_ripper_ttkb.py: dynamically detect Tcl/Tk
library dirs instead of hardcoding tcl8.6/tk8.6
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ui_ctk.py (1)
1231-1233:⚠️ Potential issue | 🟠 MajorType-filter logic is still lossy during scan.
Line 1233 still returns before creating a
TreeNodewhen the extension is unchecked. Those files never entertree_nodes, so toggling the filter back on cannot restore them.Suggested fix
var = self.file_types.get(ext) - if var is not None and not var.get(): - return + hidden = var is not None and not var.get() _icon, group = self._file_icon_and_group(file_name, file_type) node_id = self._next_node_id() node = TreeNode( node_id=node_id, parent_id=parent_id, name=file_name, kind="file", full_path=full_path or "", size=size or "", file_type=file_type or "", icon_group=group, checked=full_path in self.checked_items, + hidden=hidden, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui_ctk.py` around lines 1231 - 1233, The filter branch currently returns early when an extension is unchecked, preventing creation of the associated TreeNode and so losing the file from tree_nodes; instead of returning in the block that checks self.file_types.get(ext) and var.get(), always create and insert the TreeNode into the tree_nodes collection but mark it as filtered/hidden (e.g., set a node.filtered or call node.hide()) so it is excluded from visible display while still present for toggling the filter back on; update the code around the var = self.file_types.get(ext) check and the subsequent TreeNode creation (and any places that evaluate visibility) to honor this filtered flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend.py`:
- Around line 316-324: The pause wait blocks cancellation/stop handling; change
the logic around self.ui_manager.pause_event.wait() so you check cancel_event
and self.should_stop before and repeatedly while paused (e.g., loop while
ui_manager.pause_event.is_set(): if cancel_event is set or self.should_stop then
set aborted/abort_reason and break; otherwise wait with a short timeout or
sleep). Update the block that references self.ui_manager.pause_event.wait(),
cancel_event, and self.should_stop to perform pre-checks and use a
non-blocking/timeout-based pause loop so cancel/stop can interrupt a paused
worker.
In `@ui_ctk.py`:
- Around line 407-409: The on_closing method currently calls
self.executor.shutdown(wait=False) and self.window.destroy() which can leave
in-flight tasks running and still calling UI after teardown; modify on_closing
to first signal running tasks to stop (e.g., set a shared stop flag or call your
existing stop/cleanup method), cancel pending futures from the executor, call
self.executor.shutdown(wait=True, timeout=<short>) or poll for termination with
a short timeout, and only when all workers have acknowledged shutdown (or the
timeout elapses) call self.window.destroy(); use the on_closing method name,
self.executor.shutdown, and any existing task-stop flag or stop/resume helper to
implement this sequence so background workers no longer invoke UI callbacks
after destroy.
---
Duplicate comments:
In `@ui_ctk.py`:
- Around line 1231-1233: The filter branch currently returns early when an
extension is unchecked, preventing creation of the associated TreeNode and so
losing the file from tree_nodes; instead of returning in the block that checks
self.file_types.get(ext) and var.get(), always create and insert the TreeNode
into the tree_nodes collection but mark it as filtered/hidden (e.g., set a
node.filtered or call node.hide()) so it is excluded from visible display while
still present for toggling the filter back on; update the code around the var =
self.file_types.get(ext) check and the subsequent TreeNode creation (and any
places that evaluate visibility) to honor this filtered flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 597a9fef-d549-4d31-a54d-127aeab9ca24
📒 Files selected for processing (3)
backend.pyindex_ripper.pyui_ctk.py
| self.ui_manager.pause_event.wait() | ||
| if cancel_event is not None and cancel_event.is_set(): | ||
| self.ui_manager.log_message(f"[Download] Canceled: {file_name}") | ||
| try: | ||
| self.ui_manager.update_download_status( | ||
| file_path, "Canceled" | ||
| ) | ||
| except AttributeError: | ||
| pass | ||
| return False | ||
| aborted = True | ||
| abort_reason = "canceled" | ||
| break | ||
| if self.should_stop: | ||
| self._log(f"[Download] Stopping download for {file_name}") | ||
| return False | ||
| aborted = True | ||
| abort_reason = "stopped" | ||
| break |
There was a problem hiding this comment.
Cancellation/stop checks are blocked while paused.
At Line 316, pause_event.wait() runs before the cancel/stop checks. If a download is paused and then canceled/stopped, the worker can remain blocked indefinitely and never reach Lines 317-324.
Suggested fix
with open(file_path, "wb") as file_handle:
for data in response.iter_content(block_size):
- self.ui_manager.pause_event.wait()
- if cancel_event is not None and cancel_event.is_set():
- aborted = True
- abort_reason = "canceled"
- break
- if self.should_stop:
- aborted = True
- abort_reason = "stopped"
- break
+ while not self.ui_manager.pause_event.wait(timeout=0.2):
+ if cancel_event is not None and cancel_event.is_set():
+ aborted = True
+ abort_reason = "canceled"
+ break
+ if self.should_stop:
+ aborted = True
+ abort_reason = "stopped"
+ break
+ if aborted:
+ break
+ if cancel_event is not None and cancel_event.is_set():
+ aborted = True
+ abort_reason = "canceled"
+ break
+ if self.should_stop:
+ aborted = True
+ abort_reason = "stopped"
+ break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend.py` around lines 316 - 324, The pause wait blocks cancellation/stop
handling; change the logic around self.ui_manager.pause_event.wait() so you
check cancel_event and self.should_stop before and repeatedly while paused
(e.g., loop while ui_manager.pause_event.is_set(): if cancel_event is set or
self.should_stop then set aborted/abort_reason and break; otherwise wait with a
short timeout or sleep). Update the block that references
self.ui_manager.pause_event.wait(), cancel_event, and self.should_stop to
perform pre-checks and use a non-blocking/timeout-based pause loop so
cancel/stop can interrupt a paused worker.
| def on_closing(self) -> None: | ||
| self.executor.shutdown(wait=False) | ||
| self.window.destroy() |
There was a problem hiding this comment.
Closing the window does not stop in-flight backend work.
Lines 407-409 destroy the UI without signaling stop/resume first. Downloads/scans may continue in background and still attempt UI callbacks after teardown.
Suggested fix
def on_closing(self) -> None:
- self.executor.shutdown(wait=False)
+ self.backend.should_stop = True
+ self.pause_event.set()
+ self.scan_pause_event.set()
+ self.executor.shutdown(wait=False, cancel_futures=True)
self.window.destroy()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui_ctk.py` around lines 407 - 409, The on_closing method currently calls
self.executor.shutdown(wait=False) and self.window.destroy() which can leave
in-flight tasks running and still calling UI after teardown; modify on_closing
to first signal running tasks to stop (e.g., set a shared stop flag or call your
existing stop/cleanup method), cancel pending futures from the executor, call
self.executor.shutdown(wait=True, timeout=<short>) or poll for termination with
a short timeout, and only when all workers have acknowledged shutdown (or the
timeout elapses) call self.window.destroy(); use the on_closing method name,
self.executor.shutdown, and any existing task-stop flag or stop/resume helper to
implement this sequence so background workers no longer invoke UI callbacks
after destroy.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend.py (1)
313-322:⚠️ Potential issue | 🟠 MajorCancellation/stop is still blocked while paused.
At Line 314,
pause_event.wait()blocks before the checks at Line 315-Line 322. If a download is paused and then canceled/stopped, the worker can remain stuck and never reach abort handling/cleanup.Suggested fix
with open(file_path, "wb") as file_handle: for data in response.iter_content(block_size): - self.ui_manager.pause_event.wait() + while not self.ui_manager.pause_event.wait(timeout=0.2): + if cancel_event is not None and cancel_event.is_set(): + aborted = True + abort_reason = "canceled" + break + if self.should_stop: + aborted = True + abort_reason = "stopped" + break + if aborted: + break if cancel_event is not None and cancel_event.is_set(): aborted = True abort_reason = "canceled" break if self.should_stop: aborted = True abort_reason = "stopped" break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend.py` around lines 313 - 322, The current pause uses self.ui_manager.pause_event.wait() which blocks indefinitely and prevents the subsequent cancel_event/self.should_stop checks from running; change the pause logic inside the response.iter_content loop to a responsive wait: replace the single blocking call with a loop that while the pause_event is set repeatedly checks cancel_event.is_set() and self.should_stop and sets aborted/abort_reason and breaks if needed, otherwise calls pause_event.wait(timeout=<small interval>) so the worker can wake to handle cancellations/stops; reference response.iter_content(block_size), self.ui_manager.pause_event.wait(), cancel_event, self.should_stop, aborted, and abort_reason when making the change.ui_ctk.py (1)
407-409:⚠️ Potential issue | 🟠 MajorShutdown sequence should signal workers before destroying UI.
At Line 407-Line 409, the window is destroyed without setting stop/unpause signals. In-flight workers can continue and still attempt UI callbacks after teardown.
Suggested fix
def on_closing(self) -> None: - self.executor.shutdown(wait=False) + self.backend.should_stop = True + self.pause_event.set() + self.scan_pause_event.set() + self.executor.shutdown(wait=False, cancel_futures=True) self.window.destroy()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui_ctk.py` around lines 407 - 409, on_closing currently calls self.executor.shutdown(wait=False) and self.window.destroy() without signalling worker threads; update on_closing to first set whatever stop/unpause synchronization primitives your workers listen to (e.g. self.stop_event.set() and/or self.unpause_event.set()), then call executor.shutdown(wait=True) or wait briefly for tasks to acknowledge the stop, and only after workers have been signalled and allowed to exit safely call self.window.destroy(); keep the references to on_closing, self.executor.shutdown, and self.window.destroy so reviewers can locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend.py`:
- Around line 340-342: In the stop branch where you currently only call
self._log(f"[Download] Stopping download for {file_name}") and return False,
also set the download's UI status to "Stopped" so the row isn't left ambiguous;
e.g., call the existing status-update API (for example
self._set_download_status(file_name, "Stopped") or update the in-memory record
like self.downloads[file_name].status = "Stopped") before logging/returning
and/or emit the UI update event (e.g., self._notify_ui or similar) so the
frontend reflects the stopped state.
In `@test_ui_ctk.py`:
- Around line 13-15: Replace the hardcoded "tcl8.6" discovery with the shared
dynamic discovery routine: remove the os.path.join/_tcl_dir block and invoke the
existing _configure_tk_libraries function from index_ripper.py to locate and
validate tcl*/tk* directories and set TCL_LIBRARY/TK_LIBRARY before importing
_tkinter; ensure you call _configure_tk_libraries (the function name) early in
test_ui_ctk.py so TCL_LIBRARY is set when _tkinter is imported.
---
Duplicate comments:
In `@backend.py`:
- Around line 313-322: The current pause uses self.ui_manager.pause_event.wait()
which blocks indefinitely and prevents the subsequent
cancel_event/self.should_stop checks from running; change the pause logic inside
the response.iter_content loop to a responsive wait: replace the single blocking
call with a loop that while the pause_event is set repeatedly checks
cancel_event.is_set() and self.should_stop and sets aborted/abort_reason and
breaks if needed, otherwise calls pause_event.wait(timeout=<small interval>) so
the worker can wake to handle cancellations/stops; reference
response.iter_content(block_size), self.ui_manager.pause_event.wait(),
cancel_event, self.should_stop, aborted, and abort_reason when making the
change.
In `@ui_ctk.py`:
- Around line 407-409: on_closing currently calls
self.executor.shutdown(wait=False) and self.window.destroy() without signalling
worker threads; update on_closing to first set whatever stop/unpause
synchronization primitives your workers listen to (e.g. self.stop_event.set()
and/or self.unpause_event.set()), then call executor.shutdown(wait=True) or wait
briefly for tasks to acknowledge the stop, and only after workers have been
signalled and allowed to exit safely call self.window.destroy(); keep the
references to on_closing, self.executor.shutdown, and self.window.destroy so
reviewers can locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfdf13c4-75f0-4a70-9741-877914a4907a
📒 Files selected for processing (5)
backend.pyindex_ripper.pyindex_ripper_ttkb.pytest_ui_ctk.pyui_ctk.py
🚧 Files skipped from review as they are similar to previous changes (1)
- index_ripper_ttkb.py
| else: | ||
| self._log(f"[Download] Stopping download for {file_name}") | ||
| return False |
There was a problem hiding this comment.
Set explicit UI status on stop path.
At Line 340-Line 342, stopped downloads only log a message. Consider updating status (e.g., "Stopped") so rows don’t stay visually ambiguous.
Suggested fix
else:
self._log(f"[Download] Stopping download for {file_name}")
+ try:
+ self.ui_manager.update_download_status(file_path, "Stopped")
+ except AttributeError:
+ pass
return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend.py` around lines 340 - 342, In the stop branch where you currently
only call self._log(f"[Download] Stopping download for {file_name}") and return
False, also set the download's UI status to "Stopped" so the row isn't left
ambiguous; e.g., call the existing status-update API (for example
self._set_download_status(file_name, "Stopped") or update the in-memory record
like self.downloads[file_name].status = "Stopped") before logging/returning
and/or emit the UI update event (e.g., self._notify_ui or similar) so the
frontend reflects the stopped state.
- Move _configure_tk_libraries to app_utils.configure_tk_libraries (was duplicated in index_ripper.py and index_ripper_ttkb.py) - Move _cleanup_partial_file to app_utils.cleanup_partial_file and remove TOCTOU os.path.exists check before os.remove - Extract rebuild_executor() to app_utils — used by index_ripper.py, ui_ctk.py (was duplicated inline, ui_ttkb.py had different semantics) - Extract build_download_path() to app_utils — consolidates sanitize + safe_join pattern used by both download_selected implementations - Simplify backend.py download_file: eliminate stringly-typed abort_reason, re-check flags directly; merge duplicate RequestException/IOError handlers into one block - Memoize _filter_tree_by_term matches() to avoid O(N^2) re-evaluation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend.py (2)
327-329:⚠️ Potential issue | 🟡 MinorSet explicit
"Stopped"status on stop path.The stop branch only logs; it should also update row status for consistency.
Suggested fix
else: self._log(f"[Download] Stopping download for {file_name}") + try: + self.ui_manager.update_download_status(file_path, "Stopped") + except AttributeError: + pass return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend.py` around lines 327 - 329, The stop branch currently only logs via self._log(f"[Download] Stopping download for {file_name}") and returns False; you should also set the row's status to "Stopped" before returning so the DB/state remains consistent. Locate the same status-update call used elsewhere in this class (e.g., the method or code that marks downloads as completed/failed) and invoke it in this branch to set the download row/status to "Stopped" for file_name, then return False.
304-306:⚠️ Potential issue | 🟠 MajorPaused workers can become uncancelable.
pause_event.wait()blocks before abort checks, so cancel/stop won’t be observed while paused.Suggested fix
with open(file_path, "wb") as file_handle: for data in response.iter_content(block_size): - self.ui_manager.pause_event.wait() - if (cancel_event is not None and cancel_event.is_set()) or self.should_stop: - break + while not self.ui_manager.pause_event.wait(timeout=0.2): + if (cancel_event is not None and cancel_event.is_set()) or self.should_stop: + break + if (cancel_event is not None and cancel_event.is_set()) or self.should_stop: + break if not data: break#!/bin/bash set -euo pipefail nl -ba backend.py | sed -n '300,324p'Expected result:
pause_event.wait()appears before abort checks in the current code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend.py` around lines 304 - 306, The pause logic currently calls self.ui_manager.pause_event.wait() which can block indefinitely and prevents checking cancel_event or self.should_stop; change it to a short-timeout loop that repeatedly waits with a small timeout (e.g., pause_event.wait(timeout=...) or equivalent) and after each timeout checks cancel_event.is_set() and self.should_stop so aborts are observed while paused; update the block around self.ui_manager.pause_event.wait(), cancel_event and self.should_stop to implement this polling-with-timeout approach so canceling/stopping can interrupt a paused worker.ui_ctk.py (1)
407-409:⚠️ Potential issue | 🟠 MajorShutdown sequence should signal workers before destroying UI.
Set stop/resume signals first; otherwise background tasks can outlive the window.
Suggested fix
def on_closing(self) -> None: - self.executor.shutdown(wait=False) + self.backend.should_stop = True + self.pause_event.set() + self.scan_pause_event.set() + self.executor.shutdown(wait=False, cancel_futures=True) self.window.destroy()#!/bin/bash set -euo pipefail nl -ba ui_ctk.py | sed -n '404,412p'Expected result: current block shows no
backend.should_stop = True,pause_event.set(), orscan_pause_event.set().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui_ctk.py` around lines 407 - 409, on_closing currently destroys the UI before signalling background workers; update on_closing to first set backend.should_stop = True and call pause_event.set() and scan_pause_event.set() (or the equivalent stop/unblock signals your worker threads watch), then call self.executor.shutdown(wait=True) to let workers exit cleanly, and finally call self.window.destroy(); locate the method on_closing and modify the order to set the stop/paused signals before executor.shutdown and window.destroy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui_ctk.py`:
- Around line 1315-1316: download_selected uses safe_name when calling
downloads_panel.ensure and backend.download_file but never defines it, causing a
NameError; fix by computing a safe file name before queuing the download (e.g.,
derive safe_name from the selected item's metadata or the URL using
os.path.basename or the existing UI sanitizer method if one exists), ensure you
sanitize/unique-ify it (call into downloads_panel.ensure or a helper like
sanitize_filename) and then pass that safe_name into downloads_panel.ensure and
executor.submit(self.backend.download_file, url, file_path, safe_name,
cancel_event); add an import for os if you use os.path.basename.
---
Duplicate comments:
In `@backend.py`:
- Around line 327-329: The stop branch currently only logs via
self._log(f"[Download] Stopping download for {file_name}") and returns False;
you should also set the row's status to "Stopped" before returning so the
DB/state remains consistent. Locate the same status-update call used elsewhere
in this class (e.g., the method or code that marks downloads as
completed/failed) and invoke it in this branch to set the download row/status to
"Stopped" for file_name, then return False.
- Around line 304-306: The pause logic currently calls
self.ui_manager.pause_event.wait() which can block indefinitely and prevents
checking cancel_event or self.should_stop; change it to a short-timeout loop
that repeatedly waits with a small timeout (e.g., pause_event.wait(timeout=...)
or equivalent) and after each timeout checks cancel_event.is_set() and
self.should_stop so aborts are observed while paused; update the block around
self.ui_manager.pause_event.wait(), cancel_event and self.should_stop to
implement this polling-with-timeout approach so canceling/stopping can interrupt
a paused worker.
In `@ui_ctk.py`:
- Around line 407-409: on_closing currently destroys the UI before signalling
background workers; update on_closing to first set backend.should_stop = True
and call pause_event.set() and scan_pause_event.set() (or the equivalent
stop/unblock signals your worker threads watch), then call
self.executor.shutdown(wait=True) to let workers exit cleanly, and finally call
self.window.destroy(); locate the method on_closing and modify the order to set
the stop/paused signals before executor.shutdown and window.destroy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43add91b-ebf5-475a-ad43-ead73f8a9e65
📒 Files selected for processing (5)
app_utils.pybackend.pyindex_ripper.pyindex_ripper_ttkb.pyui_ctk.py
🚧 Files skipped from review as they are similar to previous changes (1)
- index_ripper_ttkb.py
- Move all source modules into src/index_ripper/ with proper package structure - Split ui_ctk.py (1500 LOC) into app.py, ui/filetree.py, ui/filters.py - Move tests into tests/ directory, consolidate test_app_utils + test_security_utils - Remove legacy code: index_ripper.py (old UI), ui_ttkb.py, pyside_poc.py, POC_MATRIX.md - Remove ttkbootstrap dependency - Add hatchling build-system for src layout - Update CI, PyInstaller, and README for new structure - Fix: duplicate macOS keyboard bindings, expand folders by default - All 42 tests pass, smoke test and self-test verified Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
New Electron desktop app in electron-app/ with full feature parity: Architecture: - Main process: scanner (cheerio), downloader (Node http/https + p-queue), task queue - Renderer: React 19 + TypeScript + Zustand + Tailwind CSS + shadcn/ui - IPC bridge with typed channels, preload contextBridge Features: - Split-panel UI: file tree (left) + downloads/logs/preview (right) - Virtual-scrolled file tree with @tanstack/react-virtual - Multi-site task queue with tab switching - File type filtering with checkbox chips - Search filtering across file names and paths - Shift+click range selection - Sort by name, size, or type - Per-file download progress with cancel/retry - Scan/download pause/resume - File preview (images + text) via double-click - Indeterminate progress during URL discovery - Toast notifications (sonner) - Dark mode with Slate palette - Thin overlay scrollbars - Custom right-click context menu - Keyboard shortcuts: Ctrl+A, Ctrl+F, Escape Build: electron-vite + electron-builder (Windows/macOS/Linux) Tests: 41 tests pass (vitest) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| def test_default_download_folder(self): | ||
| out = default_download_folder("https://example.com/a/b", "/tmp") | ||
| self.assertTrue(out.endswith("example.com")) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
In general, to fix incomplete URL substring sanitization, you should avoid doing string in/endswith checks on unparsed URLs or URL-derived paths to infer security properties about the host. Instead, parse the URL with a standard library (like urllib.parse) and assert on the structured components (e.g., hostname), or, in this case, derive expectations about the path in a deterministic and explicit way.
For this specific test, we don’t need to inspect substrings of the resulting path. We only need to verify that default_download_folder derives a folder name from the host example.com. The safest change inside tests/test_utils.py is to remove the brittle .endswith("example.com") check and replace it with a check on the final path component using os.path.basename. This still verifies behavior (the last path segment equals "example.com") but avoids the flagged pattern and is clearer about what we care about. Concretely, in test_default_download_folder we will change line 29 from self.assertTrue(out.endswith("example.com")) to self.assertEqual(os.path.basename(out), "example.com"). No new imports are required because os is already imported at the top of the file.
| @@ -26,7 +26,7 @@ | ||
|
|
||
| def test_default_download_folder(self): | ||
| out = default_download_folder("https://example.com/a/b", "/tmp") | ||
| self.assertTrue(out.endswith("example.com")) | ||
| self.assertEqual(os.path.basename(out), "example.com") | ||
|
|
||
|
|
||
| class TestSecurityUtils(unittest.TestCase): |
2 participants
Summary
test_ui_ctk.pysmoke assertion to match CTk FileTree architecture (tree_nodes,tree_roots,tree_scroll_frame) instead of legacytreebackend.pyfor cancel, stop, and error paths to prevent stale files on diskindex_ripper.pyby swapping executor first and then shutting down old executor safelyTest Plan
uv run pytest test_ui_ctk.py::TestWebsiteCopierCtkSmoke::test_has_treeview -vuv run pytest test_backend.py -vuv run pytest test_ui_ctk.py -vuv run pytest -vuv run python -m py_compile index_ripper.py backend.py test_ui_ctk.pyNote
Medium Risk
Moderate risk because it changes build/release automation and alters download cancellation/error handling plus UI entrypoints/executor lifecycle, which could affect packaging and runtime behavior across OSes.
Overview
CI now runs unit tests before building artifacts and the build job depends on the new
testjob; packaging is simplified touv sync+uv run pyinstallerwith--collect-all customtkinteracross platforms.Runtime hardening:
backend.download_file()now best-effort deletes partial files on cancel/stop/error, directory scanning treats links ending with/in eitherhrefor anchor text as directories, andindex_ripper.py/index_ripper_ttkb.pysetTCL_LIBRARY/TK_LIBRARYautomatically for uv-managed Python.UI/testing updates: default entrypoint switches
index_ripper.pytoWebsiteCopierCtk(including--ui-smoke), CTk smoke tests are added/updated, executor resizing in the legacy UI is made safer, and docs/README are updated to reflectuv sync, Python 3.11+, and the CustomTkinter-based UI.Written by Cursor Bugbot for commit d7026e5. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests