refactor: dedup setSessionId calls in session-switch path; add test coverage
Code review of 893f049 flagged 6 findings. This commit addresses them all:
#1 (real issue, production code): setSessionId was being called
TWICE per session switch - once explicitly from the cell-action
callback, and again from the prompt's _handleSwitchSession (which
also assigned _sessionId, called _resetStreamPointers, and set
_sessionSelect.value directly). The explicit setSessionId in the
cell-action callbacks (onCreateSession, onSwitchSession) is now
removed; the prompt's handlers do all of that bookkeeping through
a single setSessionId() call, which already implements the
dropdown + reset-stream-pointers logic.
#2 / #4 (test coverage): the existing switching test only checked
that the right HTTP call was made. New assertions: the prompt's
session-messages mock IS called again (history actually reloaded)
and the rendered history reflects the new session's content
(verifies the user sees the right messages, not stale DOM).
#3 (test coverage): new test for the error path. bind_existing
returns ok:false -> the prompt rolls the dropdown back to the
previous active id, leaves _sessionId unchanged, and shows a
Notification.error. This locks in the rollback behavior so a
future refactor can't silently break it.
#5 (test cleanup): mockedCallOpenCodeSessionMessages (the spy used
to verify history reload) is now declared at the top of the spec
file alongside the other mocked* variables, with a meaningful
name (mockedSessionMessages) instead of an inline jest.mocked()
that was hard to find.
#6 (source-doc): added a TODO(frontend) comment on
callOpenCodeSetActiveSession explaining why it is currently
unused (kept for a future 'switch between already-bound sessions'
flow that would avoid the bind round-trip) and warning a future
maintainer not to delete the corresponding server route without
removing the client function (or vice versa).
Tests: 77 jest (was 76), 73 pytest, build green.
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -210,8 +210,8 @@ export class OpenCodeCellActions extends Widget {
|
||||
if (!r.ok || !r.sessionId) {
|
||||
throw new Error(r.error || 'create_session failed');
|
||||
}
|
||||
// Sync the prompt's view of "current session id".
|
||||
this._prompt?.setSessionId(r.sessionId);
|
||||
// The prompt's _handleCreateSession calls setSessionId
|
||||
// internally; no need to set it here too.
|
||||
return r.sessionId;
|
||||
},
|
||||
onSwitchSession: async (sessionId: string) => {
|
||||
@@ -234,7 +234,8 @@ export class OpenCodeCellActions extends Widget {
|
||||
if (!r.ok) {
|
||||
return false;
|
||||
}
|
||||
this._prompt?.setSessionId(sessionId);
|
||||
// The prompt's _handleSwitchSession calls setSessionId
|
||||
// internally; no need to set it here too.
|
||||
return true;
|
||||
},
|
||||
onReloadHistory: async () => {
|
||||
|
||||
@@ -491,9 +491,9 @@ export class OpenCodeInlinePrompt extends Widget {
|
||||
}
|
||||
try {
|
||||
const newId = await this._options.onCreateSession();
|
||||
this._sessionId = newId;
|
||||
// Drop any in-progress streaming DOM (the new session is empty).
|
||||
this._resetStreamPointers();
|
||||
// setSessionId() handles _sessionId + dropdown + reset pointers
|
||||
// in one call; avoids duplicate work in this handler.
|
||||
this.setSessionId(newId);
|
||||
this._historyEl.textContent = '';
|
||||
// Re-fetch the global list so the new id is in the dropdown
|
||||
// and marked active.
|
||||
@@ -529,8 +529,9 @@ export class OpenCodeInlinePrompt extends Widget {
|
||||
try {
|
||||
const ok = await this._options.onSwitchSession(sessionId);
|
||||
if (ok) {
|
||||
this._sessionId = sessionId;
|
||||
this._resetStreamPointers();
|
||||
// setSessionId() handles _sessionId + dropdown + reset
|
||||
// pointers in one call; avoids duplicate work.
|
||||
this.setSessionId(sessionId);
|
||||
this._historyEl.textContent = '';
|
||||
if (this._options.onReloadHistory) {
|
||||
try {
|
||||
@@ -539,10 +540,6 @@ export class OpenCodeInlinePrompt extends Widget {
|
||||
console.warn('opencode_bridge: reload history failed', e);
|
||||
}
|
||||
}
|
||||
// Re-mark the now-active option as selected.
|
||||
if (this._sessionSelect) {
|
||||
this._sessionSelect.value = sessionId;
|
||||
}
|
||||
} else {
|
||||
// Roll back the select to the previous value.
|
||||
if (this._sessionSelect) {
|
||||
|
||||
Reference in New Issue
Block a user