Skip to content

fix(cli): call defaultRegistryDirectory() when resolving the cache file - #454

Merged
Devin Rousso (dcrousso) merged 1 commit into
microsoft:mainfrom
mertcanaltin:fix-update-check-cache
Aug 24, 2026
Merged

fix(cli): call defaultRegistryDirectory() when resolving the cache file#454
Devin Rousso (dcrousso) merged 1 commit into
microsoft:mainfrom
mertcanaltin:fix-update-check-cache

Conversation

@mertcanaltin

@mertcanaltin Mert Can Altin (mertcanaltin) commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

I am interested in the playwright-cli version command because this version control code worked once one day and set a note for update control, but this control does not write the version to the document and always goes to control

I called the defaultRegistryDirectory() method after starting the writer for the doc, and it didn’t go to the web for version control; it read the cache doc.

What is broken

The update check is meant to run at most once a day, cached in cli-update-check.json. That cache file is never written, so every CLI invocation hits registry.npmjs.org and re-runs the installed-skill check.

Root cause

registry.defaultRegistryDirectory became a function in microsoft/playwright#41942 (2026-07-23) and reached this repo with the roll to 1.63.0-alpha-2026-08-05 (#442). cacheFile() still used it as a string:

const dir = process.env.PLAYWRIGHT_CLI_INSTALLATION_FOR_TEST || registry.defaultRegistryDirectory;
return path.join(dir, 'cli-update-check.json');

path.join() throws a TypeError, which readCache() and writeCache() swallow in their bare catch {} — so the failure reads as "no cache yet" and the check is always stale.

Impact

Measured with --version, before → after:

network before after
normal ~480ms ~125ms
firewall that drops packets to npm ~1650ms ~125ms

The second row is the 1500ms AbortController timeout in fetchLatestVersion(), paid on every command.

Why CI did not catch it

runCli() always sets PLAYWRIGHT_CLI_INSTALLATION_FOR_TEST, so the || short-circuits and the broken branch never runs. The added test points HOME at a temp directory, leaves that variable empty, and asserts the cache file is written.

Changes

  • call registry.defaultRegistryDirectory()
  • regression test covering the default cache path
  • hoist cacheFile() out of the try blocks so only I/O and parse errors are swallowed there, instead of masking a path-computation bug as "no cache" (happy to drop this if you prefer the fix alone)

asist: Claude opus 5

@mertcanaltin

Copy link
Copy Markdown
Contributor Author

and command if using cache fast to ~370ms all command.

`registry.defaultRegistryDirectory` became a function upstream in
microsoft/playwright#41942 and arrived here with the roll to
1.63.0-alpha-2026-08-05. `cacheFile()` still used it as a string, so
`path.join()` threw, `readCache()`/`writeCache()` swallowed the
TypeError, and the update check was never cached: every single CLI
invocation fetched the npm registry and re-ran the installed-skill
check.

Locally that is ~480ms per command instead of ~125ms.

Tests always set PLAYWRIGHT_CLI_INSTALLATION_FOR_TEST, so the default
branch was never exercised. Add a regression test that points HOME at a
temp directory and asserts the cache file is written.

Also hoist `cacheFile()` out of the try blocks so only I/O and parse
failures are swallowed there, instead of masking a path-computation bug
as "no cache".
@mertcanaltin

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@dcrousso
Devin Rousso (dcrousso) merged commit 60cb176 into microsoft:main Aug 24, 2026
4 checks passed
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