Repository navigation
Improve update version checks and output - #4
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds version-aware self-update: fetches main/Cargo.toml from GitHub, parses and compares main vs local versions, and conditionally installs while reporting structured terminal output. Also adds workspace cleanup helper for webkit snapshots, bumps package version to 0.1.1, and updates docs/task notes. Changes
Sequence DiagramsequenceDiagram
actor CLI as CLI / User
participant Updater as SelfUpdate
participant GitHub as GitHub Main
participant Local as Local Binary
participant FS as File System
CLI->>Updater: mdv update
Updater->>GitHub: GET main/Cargo.toml (raw)
GitHub-->>Updater: manifest text (package.version)
Updater->>Updater: parse GitHub version
Updater->>Local: query current_version()
Local-->>Updater: local version
Updater->>Updater: compare versions -> decision
alt InstallDecision::Install
Updater->>GitHub: download main/bin/mdv
GitHub-->>Updater: binary bytes
Updater->>FS: write/replace executable
FS-->>Updater: write complete
Updater-->>CLI: "Successfully updated to vX.Y.Z"
else InstallDecision::Skip
Updater-->>CLI: "Already on vX.Y.Z" or "Latest install required: no"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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: 1
🧹 Nitpick comments (4)
src/io/self_update.rs (4)
192-202: Unused parameters inbuild_install_decision_message.The
_local_versionand_github_versionparameters are unused. If you don't plan to include version info in the decision message, consider removing them to simplify the signature.♻️ Proposed simplification
-fn build_install_decision_message( - _local_version: &str, - _github_version: &str, - needs_install: bool, -) -> String { +fn build_install_decision_message(needs_install: bool) -> String { if needs_install { "Latest install required: yes".to_string() } else { "Latest install required: no".to_string() } }Then update the call site at line 51:
- &build_install_decision_message(&local_version, &github_version, needs_install), + &build_install_decision_message(needs_install),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/io/self_update.rs` around lines 192 - 202, The function build_install_decision_message currently accepts unused parameters _local_version and _github_version; either remove those two parameters from the signature and update all call sites to call build_install_decision_message(needs_install) or, if you want version details in the message, use local_version and github_version inside the function to include them in the returned string; update the function declaration and every caller accordingly (look for build_install_decision_message usages) so the signature and calls stay consistent.
160-190: Simple manifest parser works for standard Cargo.toml but has edge case limitations.The parser handles the common
version = "0.1.1"format well. However, valid TOML edge cases could cause incorrect parsing:
- Single quotes:
version = '0.1.1'(TOML allows both quote styles)- Inline comments:
version = "0.1.1" # bump(would include# bumpin result)- Multi-line strings (unlikely for version, but valid TOML)
For this specific use case (parsing your own
Cargo.tomlwhere you control the format), this is likely fine. If you want robustness against arbitrary TOML, consider using thetomlcrate.🛡️ Optional: Handle inline comments
- return Ok(value.trim().trim_matches('"').to_string()); + let raw_value = value.trim(); + // Strip inline comments + let without_comment = raw_value.split('#').next().unwrap_or(raw_value).trim(); + return Ok(without_comment.trim_matches('"').trim_matches('\'').to_string());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/io/self_update.rs` around lines 160 - 190, parse_package_version_from_manifest currently does a brittle string parse that fails for valid TOML like single-quoted values and inline comments; replace the ad‑hoc parsing with a proper TOML parse (e.g., use the toml crate to deserialize into a struct with a package.version field) or, if you prefer minimal change, after finding the value for "version" strip surrounding either single or double quotes and remove any inline comment suffix (split on '#' outside quotes or simply take the part before '#' after trimming) so both version = '0.1.1' and version = "0.1.1" # comment are handled; update parse_package_version_from_manifest to use this approach and return the cleaned version string.
54-64: Consider simplifying the no-install explanation logic.The version comparison in
should_install_latest(line 48) already determines the decision, but lines 55-61 re-runcompare_versionsto explain why installation was skipped. This works correctly but duplicates the comparison logic.A cleaner approach would be to have
should_install_latestreturn an enum indicating the reason (e.g.,InstallDecision::AlreadyLatest,InstallDecision::LocalNewer,InstallDecision::BinaryIdentical), which would avoid recomputing and make the code more self-documenting.♻️ Optional: Return a reason enum instead of bool
enum InstallDecision { Install, BinaryIdentical, AlreadyLatest, LocalNewer, } fn decide_install( local_version: &str, github_version: &str, binaries_are_identical: bool, ) -> InstallDecision { if binaries_are_identical { return InstallDecision::BinaryIdentical; } match compare_versions(local_version, github_version) { Some(Ordering::Less) => InstallDecision::Install, Some(Ordering::Greater) => InstallDecision::LocalNewer, Some(Ordering::Equal) => InstallDecision::AlreadyLatest, None => InstallDecision::Install, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/io/self_update.rs` around lines 54 - 64, The current logic recomputes compare_versions in the no-install branch; change should_install_latest to return an enum (e.g., InstallDecision with variants Install, BinaryIdentical, AlreadyLatest, LocalNewer) instead of a bool, update callers to pattern-match that enum, and use the returned variant in the block that currently checks binaries_are_identical and compare_versions to choose which reporter.info message to emit (keep build_already_current_message(&local_version) and reporter.ok as-is); ensure the new enum and a helper (decide_install or similar) replace the duplicated compare_versions call so the decision reason is computed once.
89-100: Consider adding explicit request timeouts.The
ureq::getcalls in bothdownload_textanddownload_main_binarydon't specify a timeout. Without an explicit timeout, these requests can hang indefinitely if the network is slow or unresponsive. While the OS may eventually timeout at the socket level, this is unreliable and varies across systems. Setting explicit timeouts makes the behavior predictable and improves user experience.⏱️ Proposed fix to add explicit timeouts
fn download_text(url: &str) -> Result<String> { let response = ureq::get(url) .set("User-Agent", &format!("{REPOSITORY}/{}", current_version())) + .timeout(std::time::Duration::from_secs(30)) .call() .with_context(|| format!("failed to download mdv metadata from {url}"))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/io/self_update.rs` around lines 89 - 100, The HTTP requests in download_text and download_main_binary use ureq::get without timeouts; create and use a ureq::Agent (via ureq::AgentBuilder) configured with explicit connect and read timeouts (or use the request timeout methods if you prefer) and replace ureq::get(...) calls with agent.get(...) so both download_text and download_main_binary use the agent with configured timeouts (include current_version() in the User-Agent as before and pick sensible timeout values).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/io/self_update.rs`:
- Around line 245-253: parse_version_parts currently strips pre-release suffixes
(by splitting on '-') causing versions like "1.0.0-beta" to compare equal to
"1.0.0"; instead preserve the pre-release identifier so comparisons can treat
pre-releases as lower than their release. Modify parse_version_parts to return
both the numeric parts and an Option<String> for the pre-release (or a small
struct/enum), do not discard the suffix when splitting, and update the
comparison logic that consumes parse_version_parts (e.g., any compare_versions
function) to consider a Some(pre) < None (pre-release sorts before release) and
to lexicographically compare pre-release identifiers per semver rules. Ensure
function name parse_version_parts and the consumer comparator are updated
consistently.
---
Nitpick comments:
In `@src/io/self_update.rs`:
- Around line 192-202: The function build_install_decision_message currently
accepts unused parameters _local_version and _github_version; either remove
those two parameters from the signature and update all call sites to call
build_install_decision_message(needs_install) or, if you want version details in
the message, use local_version and github_version inside the function to include
them in the returned string; update the function declaration and every caller
accordingly (look for build_install_decision_message usages) so the signature
and calls stay consistent.
- Around line 160-190: parse_package_version_from_manifest currently does a
brittle string parse that fails for valid TOML like single-quoted values and
inline comments; replace the ad‑hoc parsing with a proper TOML parse (e.g., use
the toml crate to deserialize into a struct with a package.version field) or, if
you prefer minimal change, after finding the value for "version" strip
surrounding either single or double quotes and remove any inline comment suffix
(split on '#' outside quotes or simply take the part before '#' after trimming)
so both version = '0.1.1' and version = "0.1.1" # comment are handled; update
parse_package_version_from_manifest to use this approach and return the cleaned
version string.
- Around line 54-64: The current logic recomputes compare_versions in the
no-install branch; change should_install_latest to return an enum (e.g.,
InstallDecision with variants Install, BinaryIdentical, AlreadyLatest,
LocalNewer) instead of a bool, update callers to pattern-match that enum, and
use the returned variant in the block that currently checks
binaries_are_identical and compare_versions to choose which reporter.info
message to emit (keep build_already_current_message(&local_version) and
reporter.ok as-is); ensure the new enum and a helper (decide_install or similar)
replace the duplicated compare_versions call so the decision reason is computed
once.
- Around line 89-100: The HTTP requests in download_text and
download_main_binary use ureq::get without timeouts; create and use a
ureq::Agent (via ureq::AgentBuilder) configured with explicit connect and read
timeouts (or use the request timeout methods if you prefer) and replace
ureq::get(...) calls with agent.get(...) so both download_text and
download_main_binary use the agent with configured timeouts (include
current_version() in the User-Agent as before and pick sensible timeout values).
🪄 Autofix (Beta)
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
Run ID: 0e639355-c982-4f3c-a4fb-aff5b8b520c1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.agents/memory/lessons.md.agents/memory/todo.mdCargo.tomldocs/PRD.mdsrc/io/self_update.rs
… use it; add demo video file
8 tasks done
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
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
mdv updateprint a staged, install-like flow that checks GitHub main, shows GitHub vs local versions, and reportsLatest install required: yes|noSuccessfully updated to vX.Y.Z0.1.1Verification
cargo test --workspace --all-targets --all-featurescargo clippy --workspace --all-targets --all-features -- -D warningscargo build && <temp copy of target/debug/mdv> updateSummary by CodeRabbit
New Features
Documentation
Chores