Migrate multi-profile support to upstream contextId routing#8
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances WebSocket connection handling by introducing a local reference to the active socket and implementing guards in the onopen and onclose handlers to prevent stale connections from affecting the global state. It also includes updated unit tests to validate this behavior. Feedback indicates that the onmessage handler should also include a similar guard to ensure that only the current connection processes incoming commands, thereby avoiding potential race conditions.
| thisWs.onmessage = async (event) => { | ||
| try { | ||
| const command = JSON.parse(event.data as string) as Command; | ||
| const result = await handleCommand(command); | ||
| ws?.send(JSON.stringify(result)); | ||
| thisWs.send(JSON.stringify(result)); | ||
| } catch (err) { | ||
| console.error('[opencli] Message handling error:', err); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The onmessage handler should include the ws !== thisWs guard, consistent with the logic added to onopen and onclose. Without this guard, a stale WebSocket connection could still process and respond to commands if the daemon sends them before the connection is fully closed (e.g., during a race condition where a newer connection has already taken over the global ws pointer). This prevents potential duplicate command execution or state conflicts.
| thisWs.onmessage = async (event) => { | |
| try { | |
| const command = JSON.parse(event.data as string) as Command; | |
| const result = await handleCommand(command); | |
| ws?.send(JSON.stringify(result)); | |
| thisWs.send(JSON.stringify(result)); | |
| } catch (err) { | |
| console.error('[opencli] Message handling error:', err); | |
| } | |
| }; | |
| thisWs.onmessage = async (event) => { | |
| if (ws !== thisWs) return; | |
| try { | |
| const command = JSON.parse(event.data as string) as Command; | |
| const result = await handleCommand(command); | |
| thisWs.send(JSON.stringify(result)); | |
| } catch (err) { | |
| console.error('[opencli] Message handling error:', err); | |
| } | |
| }; |
Summary
Implements the multi-profile migration decision from #6 by rebasing the work onto upstream's
contextIdBrowser Bridge model and keeping only the minimal local fix we still need.This PR intentionally does not carry forward the old custom multi-profile routing stack:
profileId/extensionsroutingdaemon-routing.ts/config.tsprofileLabelsync-storage UXThe only code delta is the stale WebSocket close guard in the extension background connection lifecycle.
What changed
thisWsinsideconnect().onopen,onmessage,onclose, andonerrorto that specific instance.onopen/onclosecallbacks when a newer WebSocket has already replaced the module-levelwspointer.extension/dist/background.js.Why
Upstream now has the correct long-term multi-profile architecture using:
contextIdextensionProfilessrc/browser/profile.tsopencli profile listopencli profile rename <contextId> <alias>opencli profile use <profile>--profile <alias-or-contextId>/OPENCLI_PROFILEKeeping our old custom routing caused repeated merge conflicts in high-churn files. This PR reduces the migration to a small, low-conflict patch while preserving the real connection bug fix.
Verification
npx vitest run --project extension extension/src/background.test.ts npm run typecheck -- --pretty false npm --prefix extension run typecheck npm --prefix extension run buildAll passed locally.
Follow-up docs
Per #6, BillMend operation docs should be updated after this lands so account-sensitive examples use explicit upstream aliases, for example:
Aliases should be configured with:
Closes #6.