Skip to content

Add CLI update banner using axoupdater#943

Closed
dionysuzx wants to merge 1 commit into
mainfrom
cli-update-banner
Closed

Add CLI update banner using axoupdater#943
dionysuzx wants to merge 1 commit into
mainfrom
cli-update-banner

Conversation

@dionysuzx

Copy link
Copy Markdown
Member

This PR adds a lightweight update-notification flow to the clap-based CLI, modeled after Codex but using axoupdater. Highlights:

  • Cached latest version at with
  • Non-blocking refresh with a short timeout; background refresh even when cache is recent
  • Semver-aware comparison so prerelease/dev builds see newer stable releases
  • Banner printed to stderr to avoid breaking scriptable stdout
  • Tokio fs feature enabled for async cache IO

Development notes:

  • Lints: , docs format-lint: Checked 9 files in 1829µs. No fixes applied.
    website format-lint: Checking formatting...
    app format-lint: Checking formatting...
    website format-lint: All matched files use Prettier code style!
    app format-lint: All matched files use Prettier code style!
    docs format-lint: Checked 9 files in 2ms. No fixes applied.
    docs format-lint: Exited with code 0
    website format-lint: Checked 24 files in 4ms. No fixes applied.
    app format-lint: Checked 125 files in 8ms. No fixes applied.
    website format-lint: Checked 15 files in 4ms. No fixes applied.
    website format-lint: Exited with code 0
    app format-lint: Checked 45 files in 8ms. No fixes applied.
    app format-lint: Exited with code 0
    app check: Loading svelte-check in workspace: /Users/lucy/fun/kittynode-workspace/cli-update-banner/packages/app
    app check: Getting Svelte diagnostics...
    app check:
    website check: Loading svelte-check in workspace: /Users/lucy/fun/kittynode-workspace/cli-update-banner/website
    website check: Getting Svelte diagnostics...
    website check:
    app check: svelte-check found 0 errors and 0 warnings
    app check: Exited with code 0
    website check: svelte-check found 0 errors and 0 warnings
    website check: Exited with code 0 pass
  • Tests: passes locally

Let me know if you want the refresh interval or timeout tweaked.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

Comment thread packages/cli/Cargo.toml Outdated
@dionysuzx

Copy link
Copy Markdown
Member Author

@claude please review

@claude

claude Bot commented Oct 8, 2025

Copy link
Copy Markdown

Claude finished @dionysuzx's task —— View job


Code Review

I've reviewed the CLI update banner implementation. Here's my analysis:

Regarding the Tokio time Feature Issue

The Codex bot is correct - you need to add the time feature to Tokio. The code uses tokio::time::timeout at packages/cli/src/update_banner.rs:12, which requires the time feature to be explicitly enabled.

While the code may compile in certain scenarios (possibly due to transitive dependencies enabling the feature), relying on implicit feature enabling is fragile and not guaranteed. The feature should be explicitly declared to ensure compilation stability across different dependency graph resolutions.

Fix needed in packages/cli/Cargo.toml:22:

tokio = { version = "1.47.1", features = ["macros", "rt-multi-thread", "fs", "time"] }

Code Quality Review

Positives:

  • ✅ Well-structured module with clear separation of concerns
  • ✅ Good use of async/await with proper timeout handling
  • ✅ Sensible caching strategy (20-hour refresh interval)
  • ✅ Non-blocking background refresh is clever
  • ✅ Comprehensive unit tests covering edge cases
  • ✅ Semver-aware comparison handles prereleases correctly
  • ✅ Banner to stderr avoids breaking scriptable stdout
  • ✅ Error handling with proper logging

Issues & Suggestions:

  1. Logging inconsistency (packages/cli/src/update_banner.rs:34, 37, 45, 46)

    • Per CLAUDE.md guidelines: "Use info!() and error!() in sentence case without trailing periods"
    • Current: error!("Failed to update version: {e}");
    • Should be: error!("Failed to update version: {e}")
    • Same for line 37: error!("Update check timed out")
    • Lines 45-46 use debug!() which is good, but should also remove periods
  2. Potential race condition (packages/cli/src/update_banner.rs:41-48)

    • The background spawn clones version_file and then spawns a task that may write to the same file being read by other invocations
    • While unlikely to cause issues given the 20-hour interval, consider documenting this behavior or using a lock file
  3. Missing error context (packages/cli/src/update_banner.rs:100)

    • The eyre::eyre! wrapping is good, but could provide more context
    • Consider: eyre::eyre!("Failed to query latest version from GitHub: {e}")
  4. Hardcoded timeout values (packages/cli/src/update_banner.rs:29, 43)

    • The 2-second timeout is used twice. Consider extracting to a constant:
    const UPDATE_CHECK_TIMEOUT_SECS: u64 = 2;
  5. Test coverage

    • The tests are good but don't cover the async portions (network calls, file I/O)
    • This is acceptable given the testing philosophy, but consider adding a behavior test
  6. Documentation (packages/cli/src/update_banner.rs)

    • The module-level comment is minimal. Consider adding more detail about the caching strategy and update flow
  7. Banner message clarity (packages/cli/src/update_banner.rs:126-127)

    • Messages are clear and actionable ✅

Minor observations:

  • The unwrap_or_else on line 89 handles the version parse error gracefully
  • The background spawn pattern mirrors Codex behavior as intended
  • The integration into main.rs is clean and doesn't interfere with the update command itself

Summary

This is a solid implementation with good error handling and design. The critical issue is the missing time feature for Tokio - this should be fixed before merging. The logging style should also be updated to match project guidelines (remove trailing periods).


Job Run • Branch: cli-update-banner

@dionysuzx dionysuzx closed this Oct 9, 2025
@dionysuzx
dionysuzx deleted the cli-update-banner branch October 9, 2025 04:35
@dionysuzx
dionysuzx restored the cli-update-banner branch October 9, 2025 04:36
@dionysuzx
dionysuzx deleted the cli-update-banner branch October 9, 2025 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant