feat: migrate self-update logic to kaishin v0.2.3#47
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR migrates the self-update mechanism from the ChangesSelf-Update Library Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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 migrates the application's self-update functionality to the kaishin library, replacing custom implementation logic. The review identifies several areas for improvement, including refactoring the Checker struct to avoid holding a tokio::runtime::Runtime to reduce resource overhead, and addressing regressions in update state management and the handling of the _non_interactive flag.
| pub struct Checker { | ||
| inner: kaishin::Checker, | ||
| rt: tokio::runtime::Runtime, | ||
| } | ||
|
|
||
| /// Saves the update check state to the persistent state file atomically. | ||
| pub fn save_check_state(state: &UpdateCheckState) -> Result<()> { | ||
| if let Some(p) = state_path() { | ||
| if let Some(parent) = p.parent() { | ||
| std::fs::create_dir_all(parent)?; | ||
| let json = serde_json::to_string(state)?; | ||
| let mut tmp = tempfile::NamedTempFile::new_in(parent)?; | ||
| use std::io::Write; | ||
| tmp.write_all(json.as_bytes())?; | ||
| tmp.persist(&p)?; | ||
| } | ||
| impl Checker { | ||
| pub fn new() -> Result<Self> { | ||
| let opts = kaishin::KaishinOptions::new( | ||
| "yukimemi", | ||
| env!("CARGO_PKG_NAME"), | ||
| env!("CARGO_PKG_NAME"), | ||
| env!("CARGO_PKG_VERSION"), | ||
| ); | ||
| let inner = kaishin::Checker::new(env!("CARGO_PKG_NAME"), opts); | ||
| let rt = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build()?; | ||
| Ok(Self { inner, rt }) | ||
| } |
There was a problem hiding this comment.
The Checker struct currently owns a tokio::runtime::Runtime. Since this runtime is only used for the blocking check_and_save call, keeping it alive in the struct (and thus in the AutoUpdateHandle in main.rs until the program exits) is unnecessary resource overhead. Additionally, because Runtime is not Clone, it forces redundant recreation of the checker in main.rs when spawning the background thread.
Consider refactoring Checker to not hold the Runtime, and instead create a temporary runtime within check_and_save and run_self_update. This makes the struct lightweight and infallible to initialize.
pub struct Checker {
inner: kaishin::Checker,
}
impl Checker {
pub fn new() -> Self {
let opts = kaishin::KaishinOptions::new(
"yukimemi",
env!("CARGO_PKG_NAME"),
env!("CARGO_PKG_NAME"),
env!("CARGO_PKG_VERSION"),
);
Self {
inner: kaishin::Checker::new(env!("CARGO_PKG_NAME"), opts),
}
}| pub fn check_and_save(&self) -> Result<kaishin::LatestRelease> { | ||
| self.rt | ||
| .block_on(async { self.inner.check_and_save().await }) | ||
| } |
There was a problem hiding this comment.
If the Runtime is removed from the Checker struct, it should be created locally within this method. This ensures that the runtime is only active during the network call.
pub fn check_and_save(&self) -> Result<kaishin::LatestRelease> {
let rt = tokio::runtime::Builder::new_current_thread()
.enable_all()
.build()?;
rt.block_on(self.inner.check_and_save())
}| let state = updater::load_check_state(); | ||
| let now = std::time::SystemTime::now(); | ||
| let current = env!("CARGO_PKG_VERSION").to_string(); | ||
| let checker = updater::Checker::new().ok()?; |
There was a problem hiding this comment.
If updater::Checker::new() is refactored to be infallible (by moving the runtime creation), this line can be simplified.
| let checker = updater::Checker::new().ok()?; | |
| let checker = updater::Checker::new(); |
| } | ||
|
|
||
| let (tx, rx) = std::sync::mpsc::channel(); | ||
| let checker_clone = updater::Checker::new().ok()?.interval(interval); |
There was a problem hiding this comment.
This line creates a second Checker instance just to move it into the background thread. If updater::Checker is refactored to be lightweight and implement Clone, you could simply clone the existing instance. Even without Clone, recreating it is much cheaper if it doesn't initialize a tokio runtime.
| let checker_clone = updater::Checker::new().ok()?.interval(interval); | |
| let checker_clone = updater::Checker::new().interval(interval); |
| let res = rx.recv_timeout(std::time::Duration::from_secs(1)); | ||
| let now_unix = std::time::SystemTime::now() | ||
| .duration_since(std::time::SystemTime::UNIX_EPOCH) | ||
| .map(|d| d.as_secs()) | ||
| .unwrap_or(0); | ||
|
|
||
| let mut state = updater::load_check_state().unwrap_or(updater::UpdateCheckState { | ||
| last_checked_unix: 0, | ||
| last_known_latest: None, | ||
| last_known_url: None, | ||
| }); | ||
|
|
||
| state.last_checked_unix = now_unix; | ||
|
|
||
| if let Ok(Ok(latest)) = res { | ||
| state.last_known_latest = Some(latest.tag_name.clone()); | ||
| state.last_known_url = Some(latest.html_url.clone()); | ||
| let _ = updater::save_check_state(&state); | ||
| if updater::is_update_available(¤t, &latest.tag_name).unwrap_or(false) { | ||
| eprintln!("\n{}", updater::format_update_banner(¤t, &latest)); | ||
| } | ||
| } else { | ||
| // Even on timeout or error, update the last_checked_unix to avoid constant checking. | ||
| let _ = updater::save_check_state(&state); | ||
| eprintln!("\n{}", checker.format_banner(&latest)); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a regression in the update check state management. The previous implementation (lines 339-362 in the old version) ensured that the "last checked" timestamp was updated and saved even if the background check timed out or failed. This prevented the tool from attempting a check on every subsequent run in slow network conditions. The new implementation removes this logic, which may lead to repeated check attempts if the 1-second timeout is frequently hit.
| pub fn run_self_update(yes: bool, check_only: bool, _non_interactive: bool) -> Result<()> { | ||
| let opts = kaishin::KaishinOptions::new( | ||
| "yukimemi", | ||
| env!("CARGO_PKG_NAME"), | ||
| env!("CARGO_PKG_NAME"), | ||
| env!("CARGO_PKG_VERSION"), | ||
| ); | ||
|
|
||
| /// Fetches the latest release information from the GitHub API. | ||
| pub fn check_latest_release() -> Result<LatestRelease> { | ||
| let url = "https://api.github.com/repos/yukimemi/renri/releases/latest"; | ||
| let client = reqwest::blocking::Client::builder() | ||
| .user_agent(format!("renri/{}", env!("CARGO_PKG_VERSION"))) | ||
| .timeout(Duration::from_secs(5)) | ||
| let rt = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build()?; | ||
| let res = client.get(url).send()?; | ||
| if !res.status().is_success() { | ||
| return Err(anyhow!("GitHub releases API returned {}", res.status())); | ||
| } | ||
| let release: LatestRelease = res.json()?; | ||
| Ok(release) | ||
| } | ||
|
|
||
| /// Returns the path to the update check state file in the user's data directory. | ||
| fn state_path() -> Option<PathBuf> { | ||
| dirs::data_dir().map(|d| d.join("renri").join("last_update_check.json")) | ||
| rt.block_on(async { kaishin::run_self_update(&opts, yes, check_only).await }) | ||
| } |
There was a problem hiding this comment.
The _non_interactive parameter is currently unused. The previous implementation used this flag to bail early if an update was available but the environment was non-interactive and --yes was not provided. Ignoring this flag may lead to the tool attempting to prompt for confirmation in environments where no terminal is available, potentially causing hangs or failures.
pub fn run_self_update(yes: bool, check_only: bool, non_interactive: bool) -> Result<()> {
if !yes && non_interactive {
anyhow::bail!("non-interactive mode: use `--yes` to proceed with update");
}
let opts = kaishin::KaishinOptions::new(
"yukimemi",
env!("CARGO_PKG_NAME"),
env!("CARGO_PKG_NAME"),
env!("CARGO_PKG_VERSION"),
);
let rt = tokio::runtime::Builder::new_current_thread()
.enable_all()
.build()?;
rt.block_on(kaishin::run_self_update(&opts, yes, check_only))
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/main.rs`:
- Around line 301-315: When scheduling a background refresh you currently drop
any known cached update; instead capture checker.cached_update() before spawning
the thread and carry that value through the Pending path (e.g., extend
AutoUpdateHandle::Pending to include a cached_latest: Option<_>). Create the
channel and spawn the thread as you do with checker_clone.check_and_save(), but
return Some(AutoUpdateHandle::Pending { checker, rx, cached_latest }) so callers
can fall back to cached_latest when rx times out or returns an error; update the
consumer logic that resolves Pending to use cached_latest on timeout/failure.
Ensure identifiers referenced: checker.should_check(), checker.cached_update(),
AutoUpdateHandle::Pending, checker_clone, check_and_save(), tx/rx, and interval.
In `@src/updater.rs`:
- Around line 75-79: The test test_checker_init is brittle because
Checker::should_check() depends on persisted updater state; change the test to
only assert that Checker::new() returns Ok (i.e., construction succeeds) OR
ensure the test isolates state by creating a temporary/unique state directory
(or clearing kaishin's state) before creating the Checker, then call
checker.should_check() deterministically; update test code to use only
unwrap()/is_ok() on Checker::new() if you don’t want to manipulate state, or add
setup/teardown that points Checker to a temp dir so should_check() yields a
stable value.
- Around line 12-23: The run_self_update function currently ignores the
_non_interactive argument causing `renri --non-interactive self-update` to still
prompt; update run_self_update to propagate the non-interactive flag to kaishin
by passing the actual boolean (rename _non_interactive to non_interactive) into
kaishin::run_self_update, or explicitly detect non_interactive and return an Err
with a clear message if kaishin::run_self_update does not support
non-interactive mode; adjust the call site in run_self_update so the third
parameter is used instead of being discarded.
🪄 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: f5f93693-5789-4c36-8bb6-00d9c4885c4c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.kata/applied.tomlCargo.tomlsrc/main.rssrc/updater.rs
Why: - To consolidate self-update logic into a universal library \kaishin\. - To improve code maintainability and standardize update banners. Changes (addressing reviews): - Refactored \updater::Checker\ to avoid persistent \ okio::runtime::Runtime\ to reduce resource overhead. - Propagated \ on_interactive\ flag to \kaishin::run_self_update\ to honor \ enri --non-interactive\. - Improved \AutoUpdateHandle\ to carry \cached_latest\ so banners can fallback to cached state on timeout. - Fixed brittle tests by removing dependency on persistent state.
yukimemi
force-pushed
the
feat/migrate-to-kaishin
branch
from
98d51f5 to
0f37856
Compare
|
Thank you @gemini-code-assist and @coderabbitai for the thorough reviews! I have addressed all the identified issues in this push:
|
|
Thank you for the update, @yukimemi. The refactoring to avoid persistent runtime overhead and the improvements to the |
|
I'll trigger a fresh review to verify the updated changes. (˶ᵔ ᵕ ᵔ˶) ♡ ✅ Actions performedReview triggered.
|
1 participant
This PR migrates the internal self-update logic to the universal library \kaishin.
Key changes:
enri's blocking execution.
Summary by CodeRabbit