Skip to content

fix(server): resolve the VS Code IPC registry path per call - #1621

Merged
backnotprop merged 2 commits into
backnotprop:mainfrom
gwynnnplaine:fix/browser-ipc-registry-per-call
Sep 27, 2026
Merged

backnotprop merged 2 commits into
backnotprop:mainfrom
gwynnnplaine:fix/browser-ipc-registry-per-call

Conversation

@gwynnnplaine

@gwynnnplaine gwynnnplaine commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

close #1502

Root cause

packages/server/browser.ts computed the VS Code IPC registry path once, at module load:

const IPC_REGISTRY = path.join(getPlannotatorDataDir(), "vscode-ipc.json");

A PLANNOTATOR_DATA_DIR set or changed after browser.ts was imported left tryVscodeIpc reading vscode-ipc.json from the old data directory, so opening the review in VS Code silently fell through to the next strategy. #1490 fixed the same import-time freeze in the other server modules; this module was left over.

Fix

Resolve the path on every call (ipcRegistryPath()), the same shape #1490 used. No caching is needed: the registry is read once per browser-open attempt.

tryVscodeIpc is now exported so the fallback can be tested directly, without driving openBrowser through PLANNOTATOR_BROWSER / BROWSER and without any risk of launching a real browser from the test.

Tests

Two tests in packages/server/browser.test.ts. The module is imported statically, before any PLANNOTATOR_DATA_DIR is set, which is the shape of the bug:

  • reads the registry from a PLANNOTATOR_DATA_DIR set after import: a registry in a temp data dir routes the current workspace to a local stand-in IPC server, which must receive /open?url=….
  • follows a later change to PLANNOTATOR_DATA_DIR: switching data dirs between two calls routes the second call to the second server.

PLANNOTATOR_DATA_DIR is set inside each test and restored through createTestEnvironment. Each stand-in server is scoped to its test and stopped with an awaited stop(true).

Verified:

  • Both tests fail with the import-time constant restored and pass with the fix.
  • bun test packages/server/browser.test.ts: 16/16 pass on 20 consecutive runs.
  • bun test (full suite): 5364 pass, 0 fail.
  • bun run typecheck: passes.

Out of scope

apps/vscode-extension/src/extension.ts:12 has the same import-time pattern for the writer side. It runs in the VS Code extension host, a separate process that is not covered by #1502, so I left it unchanged.


This PR was written with the help of an AI agent (Pi).

browser.ts froze vscode-ipc.json at import time, so a PLANNOTATOR_DATA_DIR set or changed after import left tryVscodeIpc reading the old data directory. Resolve the path on every call, the same pattern backnotprop#1490 applied to the other server modules.

Closes backnotprop#1502
@gwynnnplaine

Copy link
Copy Markdown
Contributor Author

CI failed... 🇨🇿 ing...

@backnotprop
backnotprop merged commit 31a0078 into backnotprop:main Sep 27, 2026
24 checks passed
@backnotprop

Copy link
Copy Markdown
Owner

Thanks @gwynnnplaine! Reviewed, tested, and merged. The earlier CI failure was a network timeout in the Windows smoke job, unrelated to your change. This will go out in the next release.

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.

browser.ts IPC_REGISTRY still captures the data dir at import

2 participants