feat(cli): non-blocking "new version available" notice (24h-cached npm check, opt-out, CI-safe)#181
Conversation
WalkthroughAdds a non-blocking update notice: the CLI now checks npm for a newer version on a 24-hour cache, prints one stderr advisory when needed, skips the check in the documented cases, and exposes an env var to disable it. The new behavior is wired into ChangesUpdate notification feature
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
DOCUMENTATION.md (1)
443-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc omits the
completioncommand skip.The
preActionhook insrc/index.tsalso skips the update check for thecompletioncommand (its stdout is shell-evaluated), per the PR objectives, but this section only lists CI/non-TTY/--output json/--dry-run/opt-out as skip conditions.📝 Suggested doc addition
check is skipped in CI, when stderr is not a TTY, under `--output json` / -`--dry-run`, and entirely when `TESTSPRITE_NO_UPDATE_NOTIFIER` is set. Any +`--dry-run`, for `completion` (its stdout is shell-evaluated), and entirely +when `TESTSPRITE_NO_UPDATE_NOTIFIER` is set. Any failure is silent: the notice can never break or delay a command. This is the🤖 Prompt for 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. In `@DOCUMENTATION.md` around lines 443 - 446, The update-check documentation is missing one skip condition: the preAction hook in src/index.ts also bypasses the notice for the completion command because its stdout is shell-evaluated. Update the section describing when the notifier is skipped so it includes completion alongside the existing CI, non-TTY, --output json, --dry-run, and TESTSPRITE_NO_UPDATE_NOTIFIER cases, using the same terminology as the preAction logic.
🤖 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.
Nitpick comments:
In `@DOCUMENTATION.md`:
- Around line 443-446: The update-check documentation is missing one skip
condition: the preAction hook in src/index.ts also bypasses the notice for the
completion command because its stdout is shell-evaluated. Update the section
describing when the notifier is skipped so it includes completion alongside the
existing CI, non-TTY, --output json, --dry-run, and
TESTSPRITE_NO_UPDATE_NOTIFIER cases, using the same terminology as the preAction
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e9df77cd-cdc1-4ec9-93c3-752b2a0c0e9a
📒 Files selected for processing (4)
DOCUMENTATION.mdsrc/index.tssrc/lib/update-check.test.tssrc/lib/update-check.ts
|
Approved — the implementation meets the issue #122 conditions exactly (presence-style opt-out that fully silences before any fs/network work, CI/TTY/json/dry-run gates, 24h cache, single stderr line, never-throw). The DOCUMENTATION.md "Update notice" section is exactly the disclosure we asked for; we'll add a README pointer to it on our side. Conflicting only because #12's NO_COLOR row landed in the same env-var table — rebase onto main and this merges. |
…m check, opt-out, CI-safe)
df46387 to
1742a90
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/update-check.ts (1)
266-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
latestKnownis written but never consumed.The cache persists
latestKnown(Line 268, schema at Line 71), yet no code path reads it back to render a notice. BecauseshouldCheckForUpdategates purely onlastCheckMs, the advisory only ever fires on the single invocation that performs the probe; subsequent runs within the TTL are gated out even when a newer version is known. On a failed probe the field is also dropped. This is consistent with the documented "at most once per 24h" behavior, so it's not a defect — but the stored field is currently dead data. Consider either surfacing the cachedlatestKnownon gated runs (gh/npmupdate-notifierstyle) or dropping the field to keep the cache shape honest.🤖 Prompt for 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. In `@src/lib/update-check.ts` around lines 266 - 272, The update cache currently persists latestKnown in writeUpdateCheckCache, but that value is never read back when shouldCheckForUpdate gates the probe. Update the update-check flow in update-check.ts so the cached latestKnown is consumed on later runs to render the notice even when the network check is skipped, or remove latestKnown from the cache/schema if you intend to keep the current once-per-24h behavior. Make the change in the code path around writeUpdateCheckCache and the subsequent semver check so the cache shape matches actual usage.
🤖 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.
Nitpick comments:
In `@src/lib/update-check.ts`:
- Around line 266-272: The update cache currently persists latestKnown in
writeUpdateCheckCache, but that value is never read back when
shouldCheckForUpdate gates the probe. Update the update-check flow in
update-check.ts so the cached latestKnown is consumed on later runs to render
the notice even when the network check is skipped, or remove latestKnown from
the cache/schema if you intend to keep the current once-per-24h behavior. Make
the change in the code path around writeUpdateCheckCache and the subsequent
semver check so the cache shape matches actual usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a58b1f9-0e54-482c-b3fc-bf3e547f980c
📒 Files selected for processing (4)
DOCUMENTATION.mdsrc/index.tssrc/lib/update-check.test.tssrc/lib/update-check.ts
✅ Files skipped from review due to trivial changes (1)
- DOCUMENTATION.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index.ts
- src/lib/update-check.test.ts
|
Rebased onto main. Resolved the env-var table conflict by keeping both rows (NO_COLOR and TESTSPRITE_NO_UPDATE_NOTIFIER). No code changes; ready to merge. |
What does this PR do?
Users on stale CLIs silently miss fixes: nothing ever tells them a newer
release exists. This adds the standard gh/npm-style advisory: at most once per
24 hours (cache in
~/.testsprite/update-check.json), the CLI asks the npmregistry for the
latestdist-tag and prints ONE stderr line when it isstrictly newer than the running version. Hand-rolled (~150 lines, zero new
dependencies), hard 1.5s fetch timeout, and total failure-tolerance: any error
anywhere (network, fs, clock, even the stderr sink) is swallowed — the notice
can never break, delay, or change the exit code of a real command.
All three triage conditions are met:
TESTSPRITE_NO_UPDATE_NOTIFIER(any non-empty value) fully silences it;only, at most once per 24h, the only outbound call besides the API);
--output json, under--dry-run, and for shell-eval'd output paths.Related issue
Fixes #122
Type of change
Checklist
mainbranch.npm run lintandnpm run format:checkpass.npm run typecheckpasses.npm testpasses and coverage stays at or above the 80% gate.Notes for reviewers
Every effect is injected (env, fetch, clock, fs, TTY, stderr) — the 15 unit
tests run with zero real network/fs except one real-fs cache round-trip in a
tmpdir. Semver comparison is numeric with prerelease-sorts-older semantics;
unparseable versions compare equal so garbage can never produce a false
notice. A failed probe still stamps the cache, so a dead registry is retried
at most once per 24h window. The
update-notifiernpm package was consideredand rejected (dependency tree); this stays dep-free.
Summary by CodeRabbit