feat: aggregate release notes across intermediate versions on update - #160
Conversation
vantoan1511
left a comment
There was a problem hiding this comment.
Decision: REQUEST CHANGES
The feature is well-motivated and the core logic is correct. Two HIGH issues must be addressed before merge.
HIGH: H1 - Release notes are ordered newest-first (reversed chronology)
File: core/engine/src/updater.rs:131-145
The GitHub /releases?per_page=30 API returns releases in descending order (newest first). The loop appends them in iteration order, so the combined notes display the newest release at the top. For a user upgrading v0.6.0 to v0.7.1, the logical reading order is v0.7.0 first then v0.7.1. The unit test does not catch this because it only uses contains() without asserting order.
Fix: Collect matching entries, sort ascending by semver, then join.
HIGH: H2 - Clippy failure blocks CI
File: core/engine/src/updater.rs:45-53
cargo clippy -- -D warnings errors on the nested collapsible_if block. This will fail CI.
Fix: Collapse into a single if guard using && let-chain syntax or restructure.
MEDIUM
- M1: per_page=30 silently truncates for repos with >30 releases. Consider bumping to 100 or logging a warning.
- M2: Intermediate releases with no body are silently omitted. A placeholder would make gaps visible.
- M3: Add a test asserting ordering of sections (notes.find v0.7.0 < notes.find v0.7.1).
LOW
- L1: tag_name: String with #[serde(default)] should be Option.
- L2: The GitHub repo slug appears in 3 string literals. Extract as a const.
Validation
- cargo clippy -- -D warnings: FAIL (collapsible_if in updater.rs:45)
- cargo test: PASS (29/29)
- Build: PASS
|
Addressed review feedback:
|
Summary
Aggregates and displays release notes for all intermediate versions when an update skips versions (e.g., upgrading directly from v0.6.0 to v0.7.1).
Changes
Files Changed
Testing
pm run type-check\ (0 errors).
Related Issues
None