Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change adds changelog-based release automation and updates contributor and CI setup. It also adds download manifests, duration tracking, progress reporting, cache isolation, pagination state, task-context feature selection, export-path recording, tests, and documentation. ChangesDownload runtime and client APIs
Release automation and project setup
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
.github/requirements-dev.txt (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the dev requirements or add a reproducible constraints file.
Unpinned dependencies can resolve to different releases on different Python matrix jobs and across future runs.
curl_cffikeeps Python 3.9-compatible releases below v0.14, while v0.14+ requires Python 3.10+, so thetest_api3.9 job can install a different release than the 3.10/3.13/3.14 jobs. Pin tested versions or enforce a shared constraints file before adding new matrix entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/requirements-dev.txt around lines 1 - 10, Make the dependencies in requirements-dev.txt reproducible by pinning each tested package to explicit compatible versions, including a curl_cffi version below 0.14 for Python 3.9, or add and apply one shared constraints file across all test_api Python matrix jobs. Ensure the selected versions install consistently on Python 3.9, 3.10, 3.13, and 3.14.src/jmcomic/api.py (1)
98-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe four entry points repeat the same lifecycle block.
download_album,download_photo,download_album_async, anddownload_photo_asynceach repeat the identical sequence: readperf_counter, openjm_task_context, open the downloader, calladd_features, download, check exceptions, then call_finish_download_result. Only the download type, the entity kind, andawaitdiffer. Consider one sync helper and one async helper that take the download type and a callable. This reduces the chance that a future change updates three of the four sites.The current logic is correct, so treat this as optional cleanup.
Also applies to: 127-137, 207-216, 240-249
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jmcomic/api.py` around lines 98 - 108, Optionally consolidate the duplicated lifecycle logic from download_album, download_photo, download_album_async, and download_photo_async into one synchronous and one asynchronous helper. Have each helper accept the download type and a callable for the entity-specific download operation, while preserving feature registration, conditional exception checks, timing, task context, and _finish_download_result behavior.src/jmcomic/jm_downloader.py (1)
11-49: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
record_download_durationrequires the entity as a positional argument.The wrapper reads
entity = args[1]. If a caller passes the entity by keyword, for exampledownloader.download_by_image_detail(image=img)ordownloader.download_album(album_id='123'),args[1]raisesIndexError. All current callers pass positionally, so this is a robustness gap rather than a live defect.♻️ Proposed fix to tolerate keyword calls
def record_download_duration(context_key: str, clock=None): def decorator(func): def get_time(): return perf_counter() if clock is None else clock() + def read_entity(args, kwargs): + if len(args) > 1: + return args[1] + for value in kwargs.values(): + if isinstance(value, Downloadable): + return value + return None + if inspect.iscoroutinefunction(func): `@wraps`(func) async def async_wrapper(*args, **kwargs): - entity = args[1] + entity = read_entity(args, kwargs)Apply the same change to the synchronous wrapper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jmcomic/jm_downloader.py` around lines 11 - 49, Update record_download_duration’s async_wrapper and wrapper to resolve the entity from either the expected positional argument or its corresponding keyword argument, including calls such as image= or album_id=. Preserve the existing detail-call detection, context reuse, timing, and duration assignment behavior after resolving the entity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 28-42: Reorder the release workflow so the Build step runs before
Create Release, ensuring publication only occurs after a successful package
build. Apply the same ordering change to the corresponding steps in the release
workflow configuration, preserving the existing build and release settings.
- Around line 29-42: Update the release workflow action references for
softprops/action-gh-release and pypa/gh-action-pypi-publish in both release
workflows to reviewed, full immutable commit SHAs, replacing the mutable v2 and
release/v1 tags while preserving their existing configuration.
In @.github/workflows/test_api.yml:
- Line 39: Add .github/requirements-dev.txt to the on.push.paths filters in both
.github/workflows/test_api.yml (39-39) and .github/workflows/test_html.yml
(39-39), so changes to the shared development requirements trigger both test
matrices.
In `@assets/docs/sources/tutorial/14_async_usage.md`:
- Around line 259-260: Correct the Markdown table separator row in the section
containing the header “字段 | 它告诉你什么” so it defines exactly two columns, matching
the header row. Keep the table content and alignment unchanged.
In `@src/jmcomic/api.py`:
- Around line 19-21: Update _finish_download_result to avoid requiring every
downloader to populate manifest_dict[detail], and ensure DownloadResult.manifest
also handles missing manifests safely rather than raising KeyError. Preserve
duration updates when a manifest exists and keep returned download results valid
for custom download_album or download_photo implementations without manifest
lifecycle calls.
In `@src/jmcomic/jm_feature.py`:
- Around line 42-50: Update the tutorial examples for the current feature API:
change should_invoke calls to pass only when, and update _adapt_plugin_kwargs
examples to use the when parameter instead of the obsolete from argument. Remove
any feature_from usage while preserving the documented hook behavior.
In `@src/jmcomic/jm_plugin.py`:
- Line 804: Update both Img2pdfPlugin.invoke at src/jmcomic/jm_plugin.py:804-804
and LongImgPlugin.invoke at src/jmcomic/jm_plugin.py:887-887 consistently:
either guard downloader.record_export_filepath(...) when downloader is None, or
remove the None default and require downloader in both invoke signatures.
Preserve successful export behavior when a downloader is provided.
---
Nitpick comments:
In @.github/requirements-dev.txt:
- Around line 1-10: Make the dependencies in requirements-dev.txt reproducible
by pinning each tested package to explicit compatible versions, including a
curl_cffi version below 0.14 for Python 3.9, or add and apply one shared
constraints file across all test_api Python matrix jobs. Ensure the selected
versions install consistently on Python 3.9, 3.10, 3.13, and 3.14.
In `@src/jmcomic/api.py`:
- Around line 98-108: Optionally consolidate the duplicated lifecycle logic from
download_album, download_photo, download_album_async, and download_photo_async
into one synchronous and one asynchronous helper. Have each helper accept the
download type and a callable for the entity-specific download operation, while
preserving feature registration, conditional exception checks, timing, task
context, and _finish_download_result behavior.
In `@src/jmcomic/jm_downloader.py`:
- Around line 11-49: Update record_download_duration’s async_wrapper and wrapper
to resolve the entity from either the expected positional argument or its
corresponding keyword argument, including calls such as image= or album_id=.
Preserve the existing detail-call detection, context reuse, timing, and duration
assignment behavior after resolving the entity.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b94a071e-5b44-4981-8aa4-bb9282205773
📒 Files selected for processing (39)
.github/CONTRIBUTING.md.github/release.py.github/requirements-dev.txt.github/workflows/benchmark.yml.github/workflows/download.yml.github/workflows/download_dispatch.yml.github/workflows/export_favorites.yml.github/workflows/release.yml.github/workflows/release_auto.yml.github/workflows/test_api.yml.github/workflows/test_html.yml.gitignoreCHANGELOG.mdassets/docs/mkdocs.ymlassets/docs/sources/index.mdassets/docs/sources/tutorial/0_common_usage.mdassets/docs/sources/tutorial/11_log_custom.mdassets/docs/sources/tutorial/14_async_usage.mdassets/docs/sources/tutorial/4_module_custom.mdassets/docs/sources/tutorial/6_plugin.mdassets/docs/sources/tutorial/7_advance.mdassets/docs/sources/tutorial/9_custom_download_dir_name.mdsrc/jmcomic/__init__.pysrc/jmcomic/api.pysrc/jmcomic/jm_async_client.pysrc/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_client_impl.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_entity.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_plugin.pysrc/jmcomic/jm_toolkit.pytests/test_jmcomic/test_jm_async_client.pytests/test_jmcomic/test_jm_async_feature.pytests/test_jmcomic/test_jm_client.pytests/test_jmcomic/test_jm_download_manifest.pytests/test_jmcomic/test_jm_feature.pytests/test_jmcomic/test_jm_release.pytests/test_jmcomic/test_jm_task_context.py
💤 Files with no reviewable changes (3)
- assets/docs/sources/index.md
- assets/docs/sources/tutorial/7_advance.md
- assets/docs/sources/tutorial/4_module_custom.md
| - name: Create Release | ||
| uses: softprops/action-gh-release@v2 | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| with: | ||
| tag_name: ${{ steps.tb.outputs.tag }} | ||
| body_path: release_body.txt | ||
|
|
||
| - name: Build | ||
| run: | | ||
| python -m pip install build | ||
| python -m build | ||
|
|
||
| - name: Publish PYPI | ||
| - name: Release PYPI | ||
| uses: pypa/gh-action-pypi-publish@release/v1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Build the distribution before creating the GitHub Release.
Create Release runs before Build. If the build fails, the workflow leaves a public tag and release without a built package. Move Build before Create Release. Apply the same ordering in .github/workflows/release_auto.yml.
This failure mode is outside the recovery case documented in .github/CONTRIBUTING.md Lines 101-107.
Proposed workflow order
- - name: Create Release
- uses: softprops/action-gh-release@v2
- env:
- GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- with:
- tag_name: ${{ steps.tb.outputs.tag }}
- body_path: release_body.txt
-
- name: Build
run: |
python -m pip install build
python -m build
+
+ - name: Create Release
+ uses: softprops/action-gh-release@v2
+ env:
+ GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+ with:
+ tag_name: ${{ steps.tb.outputs.tag }}
+ body_path: release_body.txt📝 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.
| - name: Create Release | |
| uses: softprops/action-gh-release@v2 | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| with: | |
| tag_name: ${{ steps.tb.outputs.tag }} | |
| body_path: release_body.txt | |
| - name: Build | |
| run: | | |
| python -m pip install build | |
| python -m build | |
| - name: Publish PYPI | |
| - name: Release PYPI | |
| uses: pypa/gh-action-pypi-publish@release/v1 | |
| - name: Build | |
| run: | | |
| python -m pip install build | |
| python -m build | |
| - name: Create Release | |
| uses: softprops/action-gh-release@v2 | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| with: | |
| tag_name: ${{ steps.tb.outputs.tag }} | |
| body_path: release_body.txt | |
| - name: Release PYPI | |
| uses: pypa/gh-action-pypi-publish@release/v1 |
🧰 Tools
🪛 zizmor (1.29.0)
[info] 29-29: action functionality is already included by the runner (superfluous-actions): use gh release in a script step
(superfluous-actions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 28 - 42, Reorder the release
workflow so the Build step runs before Create Release, ensuring publication only
occurs after a successful package build. Apply the same ordering change to the
corresponding steps in the release workflow configuration, preserving the
existing build and release settings.
| uses: softprops/action-gh-release@v2 | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| with: | ||
| tag_name: ${{ steps.tb.outputs.tag }} | ||
| body_path: release_body.txt | ||
|
|
||
| - name: Build | ||
| run: | | ||
| python -m pip install build | ||
| python -m build | ||
|
|
||
| - name: Publish PYPI | ||
| - name: Release PYPI | ||
| uses: pypa/gh-action-pypi-publish@release/v1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-remote https://github.com/softprops/action-gh-release refs/tags/v2
git ls-remote https://github.com/pypa/gh-action-pypi-publish refs/tags/release/v1Repository: hect0x7/JMComic-Crawler-Python
Length of output: 222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Workflow file context:\n'
sed -n '1,90p' .github/workflows/release.yml 2>/dev/null || true
printf '\nAll action uses in workflows:\n'
rg -n 'uses:\s*[^#]+' .github/workflows || trueRepository: hect0x7/JMComic-Crawler-Python
Length of output: 3084
🌐 Web query:
softprops action-gh-release v2 documentation permissions releases token write
💡 Result:
To use softprops/action-gh-release, you must configure your GitHub Actions workflow with the necessary permissions to allow the action to create releases [1][2]. Required Permissions The action requires the contents: write permission [1][3]. If you also use the discussion_category_name input, you must additionally provide the discussions: write permission [1][3]. You can set these permissions at the workflow or job level in your YAML configuration: permissions: contents: write If enabling discussions: permissions: contents: write discussions: write Important Token Considerations 1. Default Token: By default, the action uses the standard GITHUB_TOKEN [2][4]. Ensure that your repository settings (under Actions > General > Workflow Permissions) allow for Read and Write access [5]. 2. Personal Access Token (PAT): If you are running the workflow on the release event (e.g., on: { release: { types: [published] } }), you must provide a personal access token instead of the default GITHUB_TOKEN, because the default token will not trigger subsequent workflows [1][2]. 3. Overriding: You can explicitly pass a custom token using the token input if needed [2][6]. A non-empty explicit token will override the default GITHUB_TOKEN [4][6]. Note that v2 of this action is no longer maintained [2]. While the permission requirements remain the same, it is recommended to review the project's current status for migration paths [2].
Citations:
- 1: https://github.com/softprops/action-gh-release/blob/v2/README.md
- 2: https://github.com/softprops/action-gh-release
- 3: https://github.com/softprops/action-gh-release/blob/master/README.md
- 4: https://github.com/softprops/action-gh-release?tab=readme-ov-file
- 5: Getting 403 on this workflow softprops/action-gh-release#366
- 6: https://github.com/softprops/action-gh-release/blob/master/action.yml
🌐 Web query:
pypa gh-action-pypi-publish release/v1 OIDC token packages publish README
💡 Result:
The pypa/gh-action-pypi-publish action (specifically the release/v1 branch) supports Trusted Publishing, which uses OpenID Connect (OIDC) to authenticate with PyPI without needing manual API tokens or passwords [1][2][3]. To use OIDC/Trusted Publishing with this action, you must follow these requirements: 1. Configure Trusted Publishing on PyPI: Your project on PyPI must already be configured to trust your GitHub repository as a publisher [1][3]. 2. Set Permissions: The GitHub Actions job running the publish step must have the id-token: write permission enabled [1][4]. 3. Omit Credentials: Do not provide an explicit username or password (or API token) to the action [1][2]. Example configuration for your GitHub Actions workflow: jobs: pypi-publish: name: Upload release to PyPI runs-on: ubuntu-latest environment: name: pypi permissions: id-token: write # Mandatory for Trusted Publishing steps: #... retrieve your distributions (e.g., download-artifact)... - name: Publish package distributions to PyPI uses: pypa/gh-action-pypi-publish@release/v1 Additionally, this action automatically generates and uploads PEP 740-compatible attestations using Sigstore, which uses the same OIDC identity [1][5]. If you need to use a self-hosted runner, Trusted Publishing is generally expected to work, provided it is configured correctly with GitHub [1][3].
Citations:
- 1: https://github.com/pypa/gh-action-pypi-publish?tab=readme-ov-file
- 2: https://github.com/pypa/gh-action-pypi-publish
- 3: https://github.com/marketplace/actions/pypi-publish
- 4: https://docs.pypi.org/trusted-publishers/using-a-publisher/
- 5: https://packaging.python.org/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Inspect Git references:\n'
git ls-remote https://github.com/softprops/action-gh-release refs/tags/v2 || true
git ls-remote https://github.com/softprops/action-gh-release refs/heads/v2 || true
git ls-remote https://github.com/softprops/action-gh-release refs/tags/v1 || true
git ls-remote https://github.com/pypa/gh-action-pypi-publish refs/tags/release/v1 || true
git ls-remote https://github.com/pypa/gh-action-pypi-publish refs/heads/release/v1 || true
printf '\nWorkflow permission blocks:\n'
rg -n -C 3 'permissions:|id-token|contents|Create Release|Release PYPI|GitHub Release|pypi-publish|action-gh-release' .github/workflows || trueRepository: hect0x7/JMComic-Crawler-Python
Length of output: 3297
Pin the release actions to immutable commit SHAs.
softprops/action-gh-release@v2 runs with contents: write, and pypa/gh-action-pypi-publish@release/v1 runs with id-token: write for PyPI Trusted Publishing. Update both uses in .github/workflows/release.yml and .github/workflows/release_auto.yml to reviewed full commit SHAs instead of mutable tags.
🧰 Tools
🪛 zizmor (1.29.0)
[info] 29-29: action functionality is already included by the runner (superfluous-actions): use gh release in a script step
(superfluous-actions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 29 - 42, Update the release
workflow action references for softprops/action-gh-release and
pypa/gh-action-pypi-publish in both release workflows to reviewed, full
immutable commit SHAs, replacing the mutable v2 and release/v1 tags while
preserving their existing configuration.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_jmcomic/test_jm_release.py (1)
117-124: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove
workflow_dispatchfromrelease_auto.yml.
release_auto.ymldeclares a manual dispatch, but its job condition only matchesgithub.event.head_commit.message, which aworkflow_dispatchevent does not provide. This makes the manual execution path inrelease_auto.ymlskipped unlessrelease.ymlis used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jmcomic/test_jm_release.py` around lines 117 - 124, Remove the workflow_dispatch trigger declaration from release_auto.yml, leaving only the push trigger used by the existing release job condition. Update the release workflow test around test_workflow_keeps_master_v_prefix_trigger_without_generated_notes to assert that workflow_dispatch is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/docs/sources/tutorial/13_export_and_feature.md`:
- Line 185: Update the diagram step around option.invoke to match the
implemented invocation in invoke_plugin: show the registered plugin class,
adapted arguments, event extras, and plugin metadata, or clearly label the
simplified call as pseudocode.
In `@tests/test_jmcomic/test_jm_download_manifest.py`:
- Around line 240-246: Update test_export_plugins_allow_omitting_downloader to
create an isolated TemporaryDirectory and build all temporary file paths with
os.path.join instead of fixed /tmp literals. Replace the hard-coded paths in
ContractOption and the cases, preserving each plugin’s expected relative
filenames and assertions within the temporary directory.
---
Outside diff comments:
In `@tests/test_jmcomic/test_jm_release.py`:
- Around line 117-124: Remove the workflow_dispatch trigger declaration from
release_auto.yml, leaving only the push trigger used by the existing release job
condition. Update the release workflow test around
test_workflow_keeps_master_v_prefix_trigger_without_generated_notes to assert
that workflow_dispatch is absent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9828a414-921f-4e41-92a4-3ea71b39d757
📒 Files selected for processing (12)
.github/workflows/release.yml.github/workflows/release_auto.yml.github/workflows/test_api.yml.github/workflows/test_html.ymlassets/docs/sources/tutorial/13_export_and_feature.mdassets/docs/sources/tutorial/14_async_usage.mdsrc/jmcomic/api.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_plugin.pytests/test_jmcomic/test_jm_download_manifest.pytests/test_jmcomic/test_jm_release.pytests/test_jmcomic/test_jm_task_context.py
💤 Files with no reviewable changes (1)
- tests/test_jmcomic/test_jm_task_context.py
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/test_html.yml
- .github/workflows/test_api.yml
- .github/workflows/release_auto.yml
- src/jmcomic/jm_plugin.py
- .github/workflows/release.yml
- assets/docs/sources/tutorial/14_async_usage.md
- src/jmcomic/api.py
- src/jmcomic/jm_downloader.py
| └→ _adapt_plugin_kwargs(from, when) # 动态生成插件参数 | ||
| └→ pdf.should_invoke('after_album') → True ✓ 执行! | ||
| └→ _adapt_plugin_kwargs(option, when) # 动态生成插件参数 | ||
| └→ option.invoke(pdf, kwargs) # 调用pdf插件，传入参数 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the implemented plugin invocation in the diagram.
Line [185] shows option.invoke(pdf, kwargs), but src/jmcomic/jm_feature.py:77-141 invokes option.invoke_plugin(...) with the registered plugin class, adapted arguments, event extras, and plugin metadata. Replace this step with the actual call or label it as pseudocode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@assets/docs/sources/tutorial/13_export_and_feature.md` at line 185, Update
the diagram step around option.invoke to match the implemented invocation in
invoke_plugin: show the registered plugin class, adapted arguments, event
extras, and plugin metadata, or clearly label the simplified call as pseudocode.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jmcomic/jm_downloader.py (1)
273-277: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake manifest absence safe at the public boundary.
record_export_filepathraises when no active manifest exists. Export plugin paths call it without guaranteeing the caller set up a manifest. Make it non-throwing when no manifest is active.
DownloadResult.manifest,DownloadResult.duration, and_finish_download_resultassumeself.downloader.manifest_dict[self.detail]exists. Initialize a manifest before creating the result, or return safe missing-state values for these result accesses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jmcomic/jm_downloader.py` around lines 273 - 277, Update record_export_filepath so a missing resolve_manifest_detail result returns safely without raising. Also adjust _finish_download_result and DownloadResult.manifest/ duration accesses to handle absent self.downloader.manifest_dict[self.detail], preferably by initializing the manifest before result creation or returning safe missing-state values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jmcomic/jm_downloader.py`:
- Line 13: Resolve the Ruff RUF003 warnings in the comments near the
parameter-parsing logic by replacing the ambiguous full-width comma and
semicolon punctuation with ASCII equivalents. Update the affected comments at
the referenced locations and do not alter the surrounding implementation.
---
Outside diff comments:
In `@src/jmcomic/jm_downloader.py`:
- Around line 273-277: Update record_export_filepath so a missing
resolve_manifest_detail result returns safely without raising. Also adjust
_finish_download_result and DownloadResult.manifest/ duration accesses to handle
absent self.downloader.manifest_dict[self.detail], preferably by initializing
the manifest before result creation or returning safe missing-state values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d0c0c62-fd20-400e-9091-436c7cdd0757
📒 Files selected for processing (2)
src/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_downloader.py
|
|
||
| def record_download_duration(context_key: str, clock=None): | ||
| def decorator(func): | ||
| # 装饰时只解析一次参数名，关键字调用无需在每次执行时重复 inspect。 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the reported Ruff RUF003 warnings.
Ruff flags ambiguous full-width punctuation in the comments at Lines [13], [20], [30], [37], [48], and [55]. Replace ， and ； with ASCII punctuation, or configure an explicit exception.
Also applies to: 20-20, 30-30, 37-37, 48-48, 55-55
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 13-13: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jmcomic/jm_downloader.py` at line 13, Resolve the Ruff RUF003 warnings in
the comments near the parameter-parsing logic by replacing the ambiguous
full-width comma and semicolon punctuation with ASCII equivalents. Update the
affected comments at the referenced locations and do not alter the surrounding
implementation.
Source: Linters/SAST tools
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jmcomic/jm_async_downloader.py (1)
183-183: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the new Ruff
RUF003warnings.Replace ambiguous full-width punctuation in these comments with ASCII punctuation, or configure an explicit exception.
Also applies to: 199-201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jmcomic/jm_async_downloader.py` at line 183, Update the comments around the asynchronous image download flow and lines 199-201 to replace ambiguous full-width punctuation with equivalent ASCII punctuation, resolving Ruff RUF003 without changing runtime behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jmcomic/jm_downloader.py`:
- Around line 30-35: Update the timing wrapper around the detail download flow
in jm_downloader.py so detail.duration is assigned in a finally path, including
when the wrapped photo or image download raises. Preserve the shared
timing-context behavior for nested detail calls, and add tests covering failed
detail attempts with a recorded duration.
---
Outside diff comments:
In `@src/jmcomic/jm_async_downloader.py`:
- Line 183: Update the comments around the asynchronous image download flow and
lines 199-201 to replace ambiguous full-width punctuation with equivalent ASCII
punctuation, resolving Ruff RUF003 without changing runtime behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea549967-7496-4c17-a60e-5f874ba83570
📒 Files selected for processing (2)
src/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_downloader.py
| # 顶层 ID 下载负责完整耗时，内部 detail 调用复用同一个计时上下文。 | ||
| if detail_call and get_jm_task_context().get(context_key) is not None: | ||
| return await func(*args, **kwargs) | ||
|
|
||
| started_at = get_time() | ||
| with jm_task_context(**{context_key: started_at}): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Record durations for failed detail attempts.
Both wrapper branches assign detail.duration only after the wrapped call returns. If a photo or image download raises, the assignment is skipped. The failure handlers retain the entity with duration=None. Record detail duration in a finally path and add failure-case tests. (github.com)
Also applies to: 47-51
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 30-30: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jmcomic/jm_downloader.py` around lines 30 - 35, Update the timing wrapper
around the detail download flow in jm_downloader.py so detail.duration is
assigned in a finally path, including when the wrapped photo or image download
raises. Preserve the shared timing-context behavior for nested detail calls, and
add tests covering failed detail attempts with a recorded duration.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/docs/sources/tutorial/13_export_and_feature.md`:
- Line 115: Update the internal link in the note near the “download_album” and
“download_photo” references to target the generated anchor for the “2.5 智能适配规则”
heading, replacing the incorrect `#25` fragment with the heading’s actual anchor.
In `@src/jmcomic/jm_plugin.py`:
- Around line 756-767: Make lazy progress initialization atomic in
ProgressDownloader.__init__/before_photo and
AsyncProgressDownloader.before_photo. At src/jmcomic/jm_plugin.py lines 756-767,
add and use a dedicated instance lock around the progress None check and
start_progress call; apply the same guard at lines 852-863, preserving the async
superclass flow so concurrent chapters retain all task entries.
- Around line 954-956: Update redirect_log_to_file to detach existing jm_logger
handlers without calling close(), since they are not plugin-owned. Preserve the
detached handlers in a local or instance-level reference, and restore them when
the redirected logging configuration is no longer active, maintaining the
logger’s prior state.
- Around line 810-817: Update stop_progress so progress.stop() is invoked for
both interactive and disabled progress before clearing self.progress; remove the
progress.console.is_interactive guard while preserving the existing
unregister_progress ordering and None handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 97c5293c-35a0-4869-a3f2-2eb76b508467
⛔ Files ignored due to path filters (1)
assets/docs/sources/images/download_progress_terminal.pngis excluded by!**/*.png
📒 Files selected for processing (22)
CHANGELOG.mdassets/docs/sources/index.mdassets/docs/sources/tutorial/0_common_usage.mdassets/docs/sources/tutorial/12_domain_strategy.mdassets/docs/sources/tutorial/13_export_and_feature.mdassets/docs/sources/tutorial/15_download_progress.mdassets/docs/sources/tutorial/6_plugin.mdsrc/jmcomic/api.pysrc/jmcomic/cl.pysrc/jmcomic/jm_async_client.pysrc/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_client_impl.pysrc/jmcomic/jm_client_interface.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_entity.pysrc/jmcomic/jm_plugin.pysrc/jmcomic/jm_toolkit.pytests/test_jmcomic/test_jm_async_client.pytests/test_jmcomic/test_jm_cli.pytests/test_jmcomic/test_jm_client.pytests/test_jmcomic/test_jm_download_progress.py
💤 Files with no reviewable changes (2)
- src/jmcomic/cl.py
- tests/test_jmcomic/test_jm_cli.py
🚧 Files skipped from review as they are similar to previous changes (4)
- assets/docs/sources/tutorial/6_plugin.md
- src/jmcomic/api.py
- src/jmcomic/jm_async_downloader.py
- src/jmcomic/jm_downloader.py
| ``` | ||
|
|
||
| > 💡 **提示**：同一个 Feature，通过 `download_album` 和 `download_photo` 调用时会自动适配不同的导出行为，详见下方 [智能适配规则](#25-智能适配规则)。 | ||
| > 💡 **提示**：同一个 Feature，通过 `download_album` 和 `download_photo` 调用时会自动适配不同的导出行为，详见下方 [智能适配规则](#25)。 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and inspect the referenced markdown file and relevant heading/link context without executing repository code.
if [ -f "assets/docs/sources/tutorial/13_export_and_feature.md" ]; then
echo "== file exists =="
wc -l assets/docs/sources/tutorial/13_export_and_feature.md
echo
echo "== lines 105-125 =="
sed -n '105,125p' assets/docs/sources/tutorial/13_export_and_feature.md | nl -ba -v105
echo
echo "== heading occurrences around smart adaptation rule =="
rg -n "智能适配规则|2\.5|25" assets/docs/sources/tutorial/13_export_and_feature.md || true
else
echo "referenced file not found"
fiRepository: hect0x7/JMComic-Crawler-Python
Length of output: 306
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lines 105-125 =="
sed -n '105,125p' assets/docs/sources/tutorial/13_export_and_feature.md | cat -n
echo
echo "== heading and link occurrences =="
rg -n "智能适配规则|### .*2\.5|### .*25|download_album|download_photo|\[`#25`\]" assets/docs/sources/tutorial/13_export_and_feature.md || trueRepository: hect0x7/JMComic-Crawler-Python
Length of output: 2574
Fix the internal link fragment.
The ### 2.5 智能适配规则 heading does not create the #25 anchor that this link targets, so the link should point directly at the target heading.
Proposed fix
-> 💡 **提示**：同一个 Feature，通过 `download_album` 和 `download_photo` 调用时会自动适配不同的导出行为，详见下方 [智能适配规则](`#25`)。
+> 💡 **提示**：同一个 Feature，通过 `download_album` 和 `download_photo` 调用时会自动适配不同的导出行为，详见下方 [智能适配规则](`#25-智能适配规则`)。📝 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.
| > 💡 **提示**：同一个 Feature，通过 `download_album` 和 `download_photo` 调用时会自动适配不同的导出行为，详见下方 [智能适配规则](#25)。 | |
| > 💡 **提示**：同一个 Feature，通过 `download_album` 和 `download_photo` 调用时会自动适配不同的导出行为，详见下方 [智能适配规则](`#25-智能适配规则`)。 |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 115-115: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@assets/docs/sources/tutorial/13_export_and_feature.md` at line 115, Update
the internal link in the note near the “download_album” and “download_photo”
references to target the generated anchor for the “2.5 智能适配规则” heading,
replacing the incorrect `#25` fragment with the heading’s actual anchor.
Source: Linters/SAST tools
| def before_photo(self, photo): | ||
| super().before_photo(photo) | ||
| if getattr(self, 'progress', None) is None: | ||
| self.start_progress() | ||
| with self.progress_lock: | ||
| self.chapter_done[photo.id] = 0 | ||
| self.chapter_total[photo.id] = len(photo) | ||
| self.chapter_tasks[photo.id] = self.progress.add_task( | ||
| f'[cyan]章节-{self.display_id(photo.id)}[/cyan]', | ||
| total=len(photo), | ||
| ) | ||
| self.refresh_progress() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded lazy start_progress in both progress downloaders. Both classes check self.progress for None and then call start_progress without an atomic guard. start_progress replaces progress_lock, album_total, album_done, chapter_done, chapter_total, and chapter_tasks. Concurrent chapters can therefore both initialize, and the losing chapter's task entry disappears, which makes after_image raise KeyError.
src/jmcomic/jm_plugin.py#L756-L767: wrap thegetattr(self, 'progress', None) is Nonecheck and thestart_progress()call inProgressDownloader.before_photowith a dedicated instance lock created in__init__.src/jmcomic/jm_plugin.py#L852-L863: apply the same guard inAsyncProgressDownloader.before_photo, whereawait super().before_photo(photo)creates the interleaving point between the check and the initialization.
📍 Affects 1 file
src/jmcomic/jm_plugin.py#L756-L767(this comment)src/jmcomic/jm_plugin.py#L852-L863
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jmcomic/jm_plugin.py` around lines 756 - 767, Make lazy progress
initialization atomic in ProgressDownloader.__init__/before_photo and
AsyncProgressDownloader.before_photo. At src/jmcomic/jm_plugin.py lines 756-767,
add and use a dedicated instance lock around the progress None check and
start_progress call; apply the same guard at lines 852-863, preserving the async
superclass flow so concurrent chapters retain all task entries.
| def stop_progress(self): | ||
| progress = getattr(self, 'progress', None) | ||
| if progress is None: | ||
| return | ||
| self.unregister_progress(progress) | ||
| if progress.console.is_interactive: | ||
| progress.stop() | ||
| self.progress = None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
rich Progress disable=True does start() begin the Live display and is stop() required
💡 Result:
When you initialize a Rich Progress object with disable=True, calling start does not begin the Live display [1][2]. The disable flag effectively prevents the underlying Live display from being started [1][2]. Regarding whether stop is required: while calling start on a disabled Progress object does not initiate a display, it is still standard best practice to use the Progress object as a context manager (using the with statement) [3][4]. This ensures that all lifecycle methods are handled correctly regardless of the disable state [3][4]. If you choose to manage the lifecycle manually (without a context manager), you should still call stop to ensure consistency and prevent potential resource leaks or unexpected behavior, even if the display is disabled [3][4]. In summary: 1. start does not start the Live display when disable=True [1][2]. 2. stop should still be called (or the context manager used) to maintain proper object lifecycle management [3][4].
Citations:
- 1: https://rich.readthedocs.io/en/stable/%5Fmodules/rich/progress.html
- 2: https://github.com/willmcgugan/rich/blob/master/rich/progress.py
- 3: https://rich.readthedocs.io/en/stable/progress.html
- 4: https://github.com/Textualize/rich/blob/fc41075a/docs/source/progress.rst
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'jm_plugin\.py|.*progress.*' . | sed 's#^\./##' | head -50
echo
echo "== jm_plugin.py around new_rich_progress and stop_progress =="
if [ -f src/jmcomic/jm_plugin.py ]; then
nl -ba src/jmcomic/jm_plugin.py | sed -n '760,840p'
fi
echo
echo "== rich dependency/version references =="
for f in pyproject.toml setup.py setup.cfg requirements*.txt Pipfile poetry.lock; do
[ -f "$f" ] && { echo "--- $f"; rg -n "rich|jmcomic" "$f" || true; }
done
echo
echo "== rich availability/version =="
python3 - <<'PY'
import importlib.util, sys
spec = importlib.util.find_spec("rich")
print("available:", bool(spec), "spec:", spec)
if spec:
import rich
print("version:", getattr(rich, "__version__", None))
try:
import inspect
print("progress start line:", inspect.getsourcefile(rich.progress.Progress.start))
except Exception as e:
print("progress start source exception:", type(e).__name__, e)
PYRepository: hect0x7/JMComic-Crawler-Python
Length of output: 610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== jm_plugin.py around progress == "
python3 - <<'PY'
from pathlib import Path
p=Path('src/jmcomic/jm_plugin.py')
lines=p.read_text(encoding='utf-8').splitlines()
for i in range(735, 835):
if i <= len(lines):
print(f"{i:5}: {lines[i-1]}")
PY
echo
echo "== rich dependency/version references =="
for f in pyproject.toml setup.py setup.cfg requirements*.txt requirements.txt Pipfile poetry.lock; do
[ -f "$f" ] && { echo "--- $f"; grep -n -i "rich\|jmcomic" "$f" || true; }
done
echo
echo "== rich availability/version/source behavior probe =="
python3 - <<'PY'
import importlib.util, inspect, sys
spec = importlib.util.find_spec("rich")
print("available:", bool(spec))
print("spec:", spec)
if spec:
import rich
print("version:", getattr(rich, "__version__", None))
from rich.console import Console
from rich.progress import Progress
console = Console(force_terminal=True, width=80)
prog = Progress(console=console, disable=True)
print("disabled progress disable:", prog.disable)
print("disabled progress console is_interactive:", console.is_interactive)
# Inspect the relevant source without executing repository code.
src = inspect.getsource(prog.start)
print("start source:")
for num, line in enumerate(src.splitlines(), 1):
if "if self.disable" in line or "super" in line or "live" in line.lower() or "__enter__" in line:
print(f"{num}: {line}")
PYRepository: hect0x7/JMComic-Crawler-Python
Length of output: 5439
🌐 Web query:
Rich progress.py Progress start() disable self.live is None stop() source
💡 Result:
In the Rich library, the Progress class manages progress bar displays, often utilizing a Live object to handle rendering [1][2]. The start and stop methods control the lifecycle of this progress display, with internal checks to ensure they behave correctly based on the configuration of the Progress instance [1][3]. Specifically, the start and stop methods check the self.disable attribute [1][2]: start: def start(self) -> None: """Start the progress display.""" if not self.disable: self.live.start(refresh=True) stop: def stop(self) -> None: """Stop the progress display.""" if not self.disable: self.live.stop if not self.console.is_interactive and not self.console.is_jupyter: self.console.print When a Progress instance is initialized with disable=True, the disable attribute is set accordingly [1][4]. Because these methods verify if the instance is disabled before interacting with self.live, calling start or stop on a disabled Progress instance will not initiate or interact with the Live display, effectively preventing any output [1][5][2]. The self.live object is initialized during the instantiation of the Progress class and is managed by the Progress instance itself; it is not None by default, but its active use is gated by the disable flag [1][4][2]. If you are observing an issue where self.live appears to be None, it is not the standard behavior of the library, as the Progress constructor assigns a Live object to self.live [1][2].
Citations:
- 1: https://github.com/Textualize/rich/blob/main/rich/progress.py
- 2: https://github.com/Textualize/rich/blob/9d8f9a37/rich/progress.py
- 3: https://github.com/Textualize/rich/blob/master/rich/progress.py
- 4: https://rich.readthedocs.io/en/stable/reference/progress.html
- 5: Disabled Progress should not start attached Live Textualize/rich#1125
🌐 Web query:
"rich.progress" "def start" "if not self.disable" "console.print"
💡 Result:
In the rich library's Progress class, the start method checks the disable attribute before starting the live display [1][2]. The implementation is as follows: def start(self) -> None: """Start the progress display.""" if not self.disable: self.live.start(refresh=True) This check ensures that if the Progress instance is initialized with disable=True, the live display (which handles rendering the progress bar) is not started [1][3][4]. Regarding your mention of console.print, the Progress class manages an internal Console object [5][6]. While the start method does not directly call console.print, the stop method contains logic that conditionally calls console.print when the progress display is finished, depending on whether the console is interactive or running in a Jupyter environment [1][2][7]. Some users have noted that when a progress bar is disabled, certain behaviors related to these console operations can still occur, sometimes resulting in unexpected output or blank lines [8].
Citations:
- 1: https://github.com/willmcgugan/rich/blob/master/rich/progress.py
- 2: https://github.com/Textualize/rich/blob/9d8f9a37/rich/progress.py
- 3: https://rich.readthedocs.io/en/latest/reference/progress.html
- 4: https://rich.readthedocs.io/en/stable/_modules/rich/progress.html
- 5: https://rich.readthedocs.io/en/stable/progress.html
- 6: https://github.com/Textualize/rich/blob/fc41075a/docs/source/progress.rst
- 7: https://github.com/Textualize/rich/blob/4d6d631a/rich/progress.py
- 8: [BUG] disabled progress bar has unexpected behavior that breaks doctests Textualize/rich#3103
Call progress.stop() even for disabled progress.
start_progress() calls progress.start() regardless of disable, and Progress.start() only avoids starting Live when disable=True. Progress.stop() also checks disable before any lifecycle cleanup, so the non-interactive path must call stop() before clearing self.progress.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jmcomic/jm_plugin.py` around lines 810 - 817, Update stop_progress so
progress.stop() is invoked for both interactive and disabled progress before
clearing self.progress; remove the progress.console.is_interactive guard while
preserving the existing unregister_progress ordering and None handling.
| for old_handler in jm_logger.handlers[:]: | ||
| jm_logger.removeHandler(old_handler) | ||
| old_handler.close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not close logging handlers that the plugin does not own.
redirect_log_to_file removes every handler from the shared jmcomic logger and calls close() on each one. The plugin does not own those handlers. A host application that attached its own handler loses it permanently, and any later write to that handler fails because the stream is closed. The plugin also never restores the previous state.
Detach the handlers without closing them, and keep a reference so the previous configuration can be restored.
🛡️ Proposed fix for handler ownership
- for old_handler in jm_logger.handlers[:]:
- jm_logger.removeHandler(old_handler)
- old_handler.close()
+ self.replaced_handlers = jm_logger.handlers[:]
+ for old_handler in self.replaced_handlers:
+ jm_logger.removeHandler(old_handler)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jmcomic/jm_plugin.py` around lines 954 - 956, Update redirect_log_to_file
to detach existing jm_logger handlers without calling close(), since they are
not plugin-owned. Preserve the detached handlers in a local or instance-level
reference, and restore them when the redirected logging configuration is no
longer active, maintaining the logger’s prior state.
1 participant
Summary by CodeRabbit
New Features
Documentation
Release