-
-
Notifications
You must be signed in to change notification settings - Fork 12
feat: new version check #222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a version-check feature to the CLI: introduces semver dependency, implements utilities to fetch the latest npm version, wires a pre-action hook with a --no-version-check option across commands, and skips private packages in info resolution. Prints a notice when a newer version is available; network errors are ignored. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Claude finished @ymc9's task —— View job PR Review: feat: new version checkTodo List:
Implementation OverviewThis PR adds version checking functionality to the ZenStack CLI. The implementation:
Code Quality & Best Practices✅ Positive aspects:
Potential Bugs & Issues🔍 Found issues:
Performance Considerations✅ Good performance practices:
💡 Suggestions:
Security Concerns✅ Security aspects handled well:
Test Coverage❌ Missing test coverage:
📝 Recommended tests:
RecommendationsHigh Priority:
Medium Priority: Low Priority: Overall AssessmentThis is a solid implementation of version checking functionality with good architectural decisions. The main concern is the logic bug that would prevent the version check from working correctly with the Recommendation: Approve after addressing the version check logic bug and adding basic test coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new version checking feature to the CLI that notifies users when a newer version is available.
- Implements version checking functionality that fetches the latest version from npm registry
- Adds a
--no-version-checkoption to all CLI commands to allow users to opt out - Integrates version checking as a pre-action hook that runs before each command
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/cli/src/utils/version-utils.ts | Adds version checking utilities including fetching latest version from npm registry |
| packages/cli/src/index.ts | Integrates version checking into CLI commands with opt-out option |
| packages/cli/package.json | Adds semver dependency and type definitions for version comparison |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/cli/package.json (1)
41-41: Declare Node engine to guarantee fetch + AbortSignal.timeout supportThe new version check relies on global fetch and AbortSignal.timeout, which are reliably available from Node 18.17+. Consider declaring the engine to prevent runtime surprises.
Add to package.json (top-level):
{
"engines": { "node": ">=18.17.0" }
}packages/cli/src/utils/version-utils.ts (4)
7-8: Select dist-tag dynamically instead of hard-coding 'next'Stable users may be prompted to install pre-releases. Pick 'latest' for stable installs and 'next' for pre-release installs.
Apply this diff:
-const CHECK_VERSION_TIMEOUT = 2000; -const VERSION_CHECK_TAG = 'next'; +const CHECK_VERSION_TIMEOUT = 2000; +const DEFAULT_VERSION_CHECK_TAG = 'latest'; @@ -export async function checkNewVersion() { +export async function checkNewVersion() { const currVersion = getVersion(); let latestVersion: string; try { - latestVersion = await getLatestVersion(); + const tag = currVersion && semver.prerelease(currVersion) ? 'next' : 'latest'; + latestVersion = await getLatestVersion(tag); } catch { // noop return; } @@ -export async function getLatestVersion() { - const fetchResult = await fetch('https://registry.npmjs.org/@zenstackhq/cli', { +export async function getLatestVersion(tag: 'latest' | 'next' = DEFAULT_VERSION_CHECK_TAG) { + const fetchResult = await fetch('https://registry.npmjs.org/@zenstackhq/cli', { headers: { accept: 'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*' }, signal: AbortSignal.timeout(CHECK_VERSION_TIMEOUT), }); @@ if (fetchResult.ok) { const data: any = await fetchResult.json(); - const latestVersion = data?.['dist-tags']?.[VERSION_CHECK_TAG]; + const latestVersion = data?.['dist-tags']?.[tag]; if (typeof latestVersion === 'string' && semver.valid(latestVersion)) { return latestVersion; } }Also applies to: 20-33, 35-46
30-32: Make the update notice actionableProvide a one-liner the user can copy-paste.
- console.log(`A newer version ${colors.cyan(latestVersion)} is available.`); + const tagHint = semver.prerelease(latestVersion) ? 'next' : 'latest'; + console.log(`A newer version ${colors.cyan(latestVersion)} is available. Run "npm i -g @zenstackhq/cli@${tagHint}" to update.`);
36-39: Respect custom npm registriesHonor npm_config_registry/NPM_CONFIG_REGISTRY before falling back to the public registry.
- const fetchResult = await fetch('https://registry.npmjs.org/@zenstackhq/cli', { + const base = (process.env.npm_config_registry || process.env.NPM_CONFIG_REGISTRY || 'https://registry.npmjs.org').replace(/\/$/, ''); + const fetchResult = await fetch(`${base}/@zenstackhq/cli`, { headers: { accept: 'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*' }, signal: AbortSignal.timeout(CHECK_VERSION_TIMEOUT), });
23-28: Surface failures when debuggingKeep default silent behavior, but emit details when ZENSTACK_DEBUG is set.
- } catch { - // noop - return; - } + } catch (e) { + if (process.env.ZENSTACK_DEBUG) { + console.debug('[version-check] failed:', e); + } + return; + }packages/cli/src/index.ts (2)
54-61: Reduce repetition when adding the --no-version-check optionDefine a small helper to attach the option, or (if acceptable with Commander constraints) make it a global option inherited by subcommands. This cuts maintenance overhead when adding new commands.
Example:
const withVersionCheck = <T extends Command>(cmd: T) => cmd.addOption(noVersionCheckOption); withVersionCheck(program.command('generate') /* ... */);Also applies to: 71-71, 83-83, 91-91, 99-99, 107-107, 119-119, 129-129, 136-136, 142-142
145-149: Gate version check for CI/silent/non-TTY to avoid noisy output and latencySkip the network call when not helpful.
- program.hook('preAction', async (_thisCommand, actionCommand) => { - if (actionCommand.getOptionValue('versionCheck')) { - await checkNewVersion(); - } - }); + program.hook('preAction', async (_thisCommand, actionCommand) => { + const enabled = actionCommand.getOptionValue('versionCheck'); + const silent = Boolean(actionCommand.getOptionValue('silent')); + const inCI = process.env.CI === 'true' || process.env.CI === '1'; + const isTTY = process.stdout.isTTY; + if (enabled && !silent && !inCI && isTTY) { + await checkNewVersion(); + } + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/cli/package.json(1 hunks)packages/cli/src/index.ts(6 hunks)packages/cli/src/utils/version-utils.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/cli/package.jsonpackages/cli/src/utils/version-utils.tspackages/cli/src/index.ts
🧬 Code graph analysis (1)
packages/cli/src/index.ts (1)
packages/cli/src/utils/version-utils.ts (1)
checkNewVersion(20-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test (20.x)
🔇 Additional comments (2)
packages/cli/package.json (2)
41-41: semver runtime dependency addition looks goodUsing semver at runtime for version comparison is appropriate. Version range ^7.7.2 matches the TS types added below.
49-49: @types/semver devDependency looks goodTypings align with the runtime version; no issues.
|
Claude finished @ymc9's task —— View job PR Review: feat: new version checkTodo List:
Implementation OverviewThis PR adds version checking functionality to the ZenStack CLI. The implementation:
Code Quality & Best Practices✅ Positive aspects:
Potential Bugs & Issues🔍 Found issues:
Performance Considerations✅ Good performance practices:
💡 Suggestions:
Security Concerns✅ Security aspects handled well:
Test Coverage❌ Missing test coverage:
📝 Recommended tests:
RecommendationsHigh Priority:
Medium Priority: Low Priority: Overall AssessmentThis is a solid implementation of version checking functionality with good architectural decisions. The main concern is the logic bug that would prevent the version check from working correctly with the Recommendation: Approve after addressing the version check logic bug and adding basic test coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/cli/src/actions/info.ts(1 hunks)packages/cli/src/index.ts(6 hunks)packages/cli/src/utils/version-utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/src/utils/version-utils.ts
- packages/cli/src/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/cli/src/actions/info.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x)
🔇 Additional comments (1)
packages/cli/src/actions/info.ts (1)
60-62: Skipping private packages is correct for workspace setupsGood call—this avoids listing non-publishable, workspace-only packages.
Summary by CodeRabbit
New Features
Bug Fixes
Chores