fix(downloader): prevent static and dynamic cover collisions - #773
Open
H-TTTTT wants to merge 1 commit into
Open
fix(downloader): prevent static and dynamic cover collisions#773H-TTTTT wants to merge 1 commit into
H-TTTTT wants to merge 1 commit into
Conversation
Reviewer's Guide调整封面下载文件名处理，避免静态封面与动态封面之间的冲突，同时保留已有命名方式，并新增关于并发 JPEG 下载与重复运行的回归测试。 封面下载冲突预防的时序图sequenceDiagram
participant Downloader
participant Filesystem
Downloader->>Downloader: download_cover(item, name, static_suffix, dynamic_suffix)
Downloader->>Downloader: static_url = item[static_cover]
Downloader->>Downloader: dynamic_url = item[dynamic_cover]
Downloader->>Downloader: both_covers = all(static_cover, static_url, dynamic_cover, dynamic_url)
alt static_cover download
Downloader->>Filesystem: is_exists(actual_root.with_name(name.static_suffix))
Filesystem-->>Downloader: false
Downloader->>Downloader: tasks.append(static_url, temp_static_path, static_path)
end
Downloader->>Downloader: legacy_dynamic = actual_root.with_name(name.dynamic_suffix)
Downloader->>Downloader: typed_dynamic = actual_root.with_name(name_dynamic.dynamic_suffix)
loop normalized dynamic outputs
Downloader->>Filesystem: is_exists(typed_dynamic.with_suffix(suffix))
Filesystem-->>Downloader: exists or not
end
Downloader->>Downloader: dynamic_exists = any(is_exists(legacy_dynamic), is_exists(typed_dynamic), normalized_dynamic_exists)
Downloader->>Downloader: dynamic_path = typed_dynamic if both_covers else legacy_dynamic
alt dynamic_cover download
Downloader->>Filesystem: is_exists(legacy_dynamic)
Filesystem-->>Downloader: false
Downloader->>Filesystem: is_exists(typed_dynamic)
Filesystem-->>Downloader: false
Downloader->>Downloader: tasks.append(dynamic_url, temp_dynamic_path(dynamic_path.name), dynamic_path)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your Experience访问你的 dashboard 以：
Getting HelpOriginal review guide in EnglishReviewer's GuideAdjusts cover-download filename handling to avoid collisions between static and dynamic covers, preserves legacy naming, and adds regression tests around concurrent JPEG downloads and reruns. Sequence diagram for cover download collision preventionsequenceDiagram
participant Downloader
participant Filesystem
Downloader->>Downloader: download_cover(item, name, static_suffix, dynamic_suffix)
Downloader->>Downloader: static_url = item[static_cover]
Downloader->>Downloader: dynamic_url = item[dynamic_cover]
Downloader->>Downloader: both_covers = all(static_cover, static_url, dynamic_cover, dynamic_url)
alt static_cover download
Downloader->>Filesystem: is_exists(actual_root.with_name(name.static_suffix))
Filesystem-->>Downloader: false
Downloader->>Downloader: tasks.append(static_url, temp_static_path, static_path)
end
Downloader->>Downloader: legacy_dynamic = actual_root.with_name(name.dynamic_suffix)
Downloader->>Downloader: typed_dynamic = actual_root.with_name(name_dynamic.dynamic_suffix)
loop normalized dynamic outputs
Downloader->>Filesystem: is_exists(typed_dynamic.with_suffix(suffix))
Filesystem-->>Downloader: exists or not
end
Downloader->>Downloader: dynamic_exists = any(is_exists(legacy_dynamic), is_exists(typed_dynamic), normalized_dynamic_exists)
Downloader->>Downloader: dynamic_path = typed_dynamic if both_covers else legacy_dynamic
alt dynamic_cover download
Downloader->>Filesystem: is_exists(legacy_dynamic)
Filesystem-->>Downloader: false
Downloader->>Filesystem: is_exists(typed_dynamic)
Filesystem-->>Downloader: false
Downloader->>Downloader: tasks.append(dynamic_url, temp_dynamic_path(dynamic_path.name), dynamic_path)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我在这里给出一些高层次的反馈：
- 动态封面存在性相关的逻辑（
legacy_dynamic、typed_dynamic、normalized_dynamic_exists、dynamic_exists）已经有点复杂了；可以考虑把这部分提取到一个小的辅助方法中，以更清晰地区分不同的文件名/规范化场景，并降低download_cover中的认知负担。 both_covers = all((self.static_cover, static_url, self.dynamic_cover, dynamic_url))这个检查在隐式地混合配置开关和 URL 是否存在；可以考虑让这个条件表达得更明确一些（例如，将开关和元数据检查拆开），这样能让意图更清晰，并在条目结构发生变化时避免一些隐蔽的 bug。
给 AI 代理的提示
请根据这次代码审查中的评论进行修改：
## 总体评论
- 动态封面存在性相关的逻辑（`legacy_dynamic`、`typed_dynamic`、`normalized_dynamic_exists`、`dynamic_exists`）已经有点复杂了；可以考虑把这部分提取到一个小的辅助方法中，以更清晰地区分不同的文件名/规范化场景，并降低 `download_cover` 中的认知负担。
- `both_covers = all((self.static_cover, static_url, self.dynamic_cover, dynamic_url))` 这个检查在隐式地混合配置开关和 URL 是否存在；可以考虑让这个条件表达得更明确一些（例如，将开关和元数据检查拆开），这样能让意图更清晰，并在条目结构发生变化时避免一些隐蔽的 bug。帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进之后的审查。
Original comment in English
Hey - I've left some high level feedback:
- The dynamic cover existence logic (
legacy_dynamic,typed_dynamic,normalized_dynamic_exists,dynamic_exists) is getting dense; consider extracting it into a small helper method to clarify the different filename/normalization cases and reduce cognitive load indownload_cover. - The
both_covers = all((self.static_cover, static_url, self.dynamic_cover, dynamic_url))check implicitly mixes configuration flags and presence of URLs; consider making this condition more explicit (e.g., separate flags vs. metadata checks) to make the intent clearer and avoid subtle bugs if item structure changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The dynamic cover existence logic (`legacy_dynamic`, `typed_dynamic`, `normalized_dynamic_exists`, `dynamic_exists`) is getting dense; consider extracting it into a small helper method to clarify the different filename/normalization cases and reduce cognitive load in `download_cover`.
- The `both_covers = all((self.static_cover, static_url, self.dynamic_cover, dynamic_url))` check implicitly mixes configuration flags and presence of URLs; consider making this condition more explicit (e.g., separate flags vs. metadata checks) to make the intent clearer and avoid subtle bugs if item structure changes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_dynamicstem when both cover types are downloaded.png,.jpeg, and.webpdynamic outputs on later runs.webpfilesTesting
Fixes #420
Summary by Sourcery
调整封面下载逻辑，避免静态封面和动态封面之间的文件名冲突，同时保持现有的命名行为。
Bug Fixes（错误修复）:
.webp文件以及规范化后的 PNG/JPEG/WEBP 变体）在后续运行中能够被正确检测并跳过下载。Enhancements（增强功能）:
_dynamic文件名主干，以区分两类输出文件。Tests（测试）:
Original summary in English
Summary by Sourcery
Adjust cover download logic to avoid filename collisions between static and dynamic covers while preserving existing naming behavior.
Bug Fixes:
.webpfiles and normalized PNG/JPEG/WEBP variants, are correctly detected and skipped on subsequent runs.Enhancements:
_dynamicfilename stem when both static and dynamic covers are downloaded to keep outputs separate.Tests: