Skip to content

feat(update): oclif hook to notify of updated versions of CLI#374

Merged
mttdnt merged 14 commits into
mainfrom
feature/sdk-636/update-check
Feb 10, 2026
Merged

feat(update): oclif hook to notify of updated versions of CLI#374
mttdnt merged 14 commits into
mainfrom
feature/sdk-636/update-check

Conversation

@mttdnt
Copy link
Copy Markdown
Contributor

@mttdnt mttdnt commented Feb 2, 2026

Description

Update notifier runs as a init hook. As per the original implementation, waits 12 hours between pulling latest version and notifying user if there is a more up to date version.

What to review

New implementation matches intent of original update notifier

Testing

Wrote more integration style test, tried to mock as little as possible

@mttdnt mttdnt requested review from binoy14 and rexxars February 2, 2026 21:33
@mttdnt mttdnt requested a review from a team as a code owner February 2, 2026 21:34
@linear
Copy link
Copy Markdown

linear Bot commented Feb 2, 2026

SDK-636 Implement update check for CLI

Migrate existing functionality of checking if there is a new version available and showing prompt.

Reference implementation: https://github.com/oclif/plugin-warn-if-update-available

set: mockGetUserConfigSet,
})),
isCi: vi.fn(),
subdebug: vi.fn(() => mockDebug),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is really needed, but there wasnt much else to assert on

Comment thread packages/@sanity/cli/src/hooks/init/checkForUpdates.ts
@mttdnt mttdnt force-pushed the feature/sdk-636/update-check branch from 11ffd9b to 882a993 Compare February 3, 2026 02:19
@claude
Copy link
Copy Markdown

claude Bot commented Feb 3, 2026

Claude finished @mttdnt's task in 1m 34s —— View job


PR Review Complete

  • Read repository guidelines
  • Review changed files
  • Check for issues
  • Post findings

Issues Found

1. Package Name Mismatch (Critical)

File: updateChecker.ts:40 and showNotificationUpdate.ts:23-24

The code uses config.name which is @sanity/cli, but notification suggests updating sanity package. When fetching from npm, it checks for updates to @sanity/cli, but tells users to run npm update sanity or yarn upgrade sanity. This will confuse users.

Fix: Either:

  • Use @sanity/cli consistently (fetch it, suggest updating it)
  • Or if the published package is actually sanity, use that everywhere

2. Missing Test for Global NPM Install

File: checkForUpdates.test.ts:278

Test line 278 comment says "Couldn't find a way to reliably setup a fixture with a global install" but only tests yarn global. Missing test for npm install -g sanity scenario.

Fix: Add test case for global npm install scenario to ensure npm install -g sanity command is suggested correctly.

3. Timeout Cleanup Edge Case

File: promiseRaceWithTimeout.ts:23

The condition if (timeoutId && result !== null) means timeout is only cleared when promise wins. But the finally block already clears it. The inner clearTimeout (line 24) is redundant and adds confusion.

Fix: Remove lines 22-25. The finally block handles all cleanup.

4. Blocking Hook Behavior

File: checkForUpdates.ts:12

Hook awaits updateChecker(), which can block CLI startup for up to 300ms on timeout. Per review comment from mttdnt (line 12), this was a concern but not resolved.

Fix: Either make it non-blocking with void updateChecker(config).catch(...) or document why blocking is acceptable.


@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 3, 2026

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 77.03% 7005 / 9093
🔵 Statements 76.56% 7220 / 9430
🔵 Functions 73.76% 1198 / 1624
🔵 Branches 63.45% 3225 / 5082
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/@sanity/cli-core/src/util/createExpiringConfig.ts 94.11% 100% 75% 94.11% 50, 52
packages/@sanity/cli/src/hooks/init/checkForUpdates.ts 100% 100% 100% 100%
packages/@sanity/cli/src/util/promiseRaceWithTimeout.ts 100% 83.33% 100% 100%
packages/@sanity/cli/src/util/update/fetchLatestVersion.ts 100% 75% 100% 100%
packages/@sanity/cli/src/util/update/getUpdateCommand.ts 100% 100% 100% 100%
packages/@sanity/cli/src/util/update/isInstalledUsingYarn.ts 0% 0% 0% 0% 4-10
packages/@sanity/cli/src/util/update/showNotificationUpdate.ts 100% 75% 100% 100%
packages/@sanity/cli/src/util/update/updateChecker.ts 100% 100% 100% 100%
packages/@sanity/cli/src/util/update/updateCheckerDebug.ts 100% 100% 100% 100%
Generated in workflow #2070 for commit 8a89d1f by the Vitest Coverage Report Action

Copy link
Copy Markdown
Contributor

@binoy14 binoy14 left a comment

Choose a reason for hiding this comment

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

Might be worth looking at createExpiringConfig if it simplifies some things and other comments but overall is good

* Get the appropriate update command for the package manager
*/
export default function getUpdateCommand(pm: PackageManager): string {
const commands: Record<PackageManager, string> = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From looking at the original implementation, it checks if it's a global install vs local install. This forces a global install which is not something that is ideal. Also should it be install sanity ? 🤔

cc @rexxars

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah I see it checks for a global install, that I can update. In the original cli it uses @sanity/cli, but I agree seems like it should install sanity

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the changes to check for global vs local install. One thing I am unsure of is will oclif be pulling the version of @sanity/cli? If so will it be confusing to the user when suggesting to update the sanity package?

Comment thread packages/@sanity/cli/src/hooks/init/checkForUpdates.ts
Comment thread packages/@sanity/cli/src/util/update/updateChecker.ts
Comment thread packages/@sanity/cli/src/util/update/updateChecker.ts
return
}

const cache = store.get('updateCheck') as UpdateCache | undefined
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The store name does not seem to match the original CLI. We probably should keep it simple cliLastUpdateCheck is what og cli has but I think maybe if we create an expiring config this might not be needed.

vi.unstubAllEnvs()
originalEnv = {...process.env} as Record<string, string>
mockIsCi.mockReturnValue(false)
process.stdout.isTTY = true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can't this be vi.stubEnv('isTTY', true) that way we can remove the originalEnv thing


test('returns early if not TTY', async () => {
const {config} = await getCommandAndConfig('help')
const originalIsTTY = process.stdout.isTTY
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe use vi.stubEnv?

@socket-security
Copy link
Copy Markdown

socket-security Bot commented Feb 4, 2026

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addednpm/​is-installed-globally@​1.0.01001007180100

View full report

@mttdnt mttdnt force-pushed the feature/sdk-636/update-check branch from 581717b to 5bc86f7 Compare February 4, 2026 19:51
})

test('shows yarn global add command when globally installed via yarn', async () => {
mockIsInstalledGlobally.default = true
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Couldn't find a way to reliably setup a fixture with a global install. Let me know if you have any ideas.

@mttdnt mttdnt requested a review from binoy14 February 4, 2026 20:45
Comment thread packages/@sanity/cli/src/util/update/updateChecker.ts
try {
await updateChecker(config)
} catch (err) {
console.error(`Error checking for updates: ${err}`)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should use debug not console.error

command = getUpdateCommand(chosen)
}

const message = `Update available: ${currentVersion} → ${latestVersion}\n\nRun ${command} to update`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copying styles from current CLI

Suggested change
const message = `Update available: ${currentVersion}${latestVersion}\n\nRun ${command} to update`
const message = `Update available: ${styleText('dim', currentVersion)}${styleText('green', latestVersion)}\n\nRun ${styleText('cyan', command)} to update`

try {
const result = await Promise.race([
getLatestVersion(packageName),
new Promise<null>((resolve) => setTimeout(() => resolve(null), timeout)),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will cause the process to hang until the timeout is resolved. If you change the timeout to something larger like 60_000 and run a command that responds and exits immediately like --help see it getting hung. We basically need to clear this timeout

const mockIsInstalledGlobally = vi.hoisted(() => ({default: false}))
const mockIsInstalledUsingYarn = vi.hoisted(() => vi.fn())

// Mock dependencies
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Mock dependencies


// Cache for latest version from npm
const latestVersionCache = createExpiringConfig({
fetchValue: async () => fetchLatestVersionWithTimeout(config.name, CHECK_TIMEOUT),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this error be handled? i.e basically ignore it I don't want an error cause this to throw an uncaught exception

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It being handled in fetchLastestVersionTimeout, but I can just remove the throws in that function so it fails silently

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm I guess it's fine because the main functions catches it

@mttdnt mttdnt force-pushed the feature/sdk-636/update-check branch from e33300a to 287bb31 Compare February 6, 2026 19:49
@mttdnt mttdnt requested a review from binoy14 February 9, 2026 15:52
@mttdnt mttdnt merged commit 4172cbc into main Feb 10, 2026
33 checks passed
@mttdnt mttdnt deleted the feature/sdk-636/update-check branch February 10, 2026 13:17
@squiggler-legacy squiggler-legacy Bot mentioned this pull request Feb 10, 2026
This was referenced Mar 6, 2026
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.

2 participants