Phase 1 (#4662): address Codex + Opus gate findings
Codex (CORE regression): the parallelized loadDir('.') started BEFORE the
_switchGen stale-switch guard; empty-session switches reuse the session id, so a
superseded switch's /api/list could paint the wrong profile's tree. Reverted to
guard-before-workspace-render ordering (the up-front skeletons already give the
immediate cross-surface feedback; the parallel gain was marginal).
Opus findings:
- [blocker] Workspace skeleton stranded permanently when the new profile has no
bound workspace (loadDir skipped → nothing clears #fileTree). Added
clearWorkspaceTreeSkeleton() (only empties when a skeleton is present) and call
it on the no-workspace path in BOTH switch branches.
- [low-med] Session-list skeleton stranded if an inline-rename input or row
action menu was open (renderSessionListFromCache early-returns before the DOM
swap). Dismiss _renamingSid + closeSessionActionMenu() before showing the
skeleton.
- [low] No-op self-switch flashed skeleton→restore (activateCurrentProfile
doesn't pre-check). Added an early-return when name === S.activeProfile.
Tests: +4 static assertions for the self-switch guard, rename/menu dismissal,
no-workspace skeleton clear, and the clear-helper definition. 22 pass.
This commit is contained in:
1 parent
4abdb54ede
commit
adcd06ff76
3 files changed
+97
-14
No files matched your search
+37
-9
@@ -5897,6 +5897,11 @@ window.addEventListener('resize',()=>{
|
||||
});
|
||||
|
||||
async function switchToProfile(name) {
|
||||
// No-op self-switch guard: bail before showing any loading skeleton if we're
|
||||
// already on this profile, so paths like activateCurrentProfile() (which
|
||||
// doesn't pre-check) can't flash a skeleton→restore for a click that changes
|
||||
// nothing. (#4662 Opus gate)
|
||||
if (name && name === S.activeProfile) return;
|
||||
S._pendingSessionToolsets=null;
|
||||
// Profile switches are per-client cookie/TLS scoped, so a running stream in
|
||||
// the current session can safely continue while this tab moves to another
|
||||
@@ -5923,6 +5928,14 @@ async function switchToProfile(name) {
|
||||
// instant the switch begins so the user never stares at the wrong profile's
|
||||
// data, and gets consistent loading feedback across the whole surface — not
|
||||
// just the spinning chip. The real renders below overwrite these.
|
||||
//
|
||||
// First dismiss any open inline-rename or row action menu: renderSessionList
|
||||
// FromCache() early-returns (no DOM swap) while _renamingSid or
|
||||
// _sessionActionMenu is set, which would otherwise strand the skeleton AND
|
||||
// defeat the failure-path restore (#4662 Opus gate). A profile switch is a
|
||||
// context change where dismissing those transient affordances is correct.
|
||||
if (typeof _renamingSid !== 'undefined' && _renamingSid) _renamingSid = null;
|
||||
if (typeof closeSessionActionMenu === 'function') closeSessionActionMenu();
|
||||
if (typeof showSessionListSkeleton === 'function') showSessionListSkeleton();
|
||||
const _workspaceVisibleAtStart = typeof _workspacePanelMode !== 'undefined' && _workspacePanelMode !== 'closed';
|
||||
if (_workspaceVisibleAtStart && typeof showWorkspaceTreeSkeleton === 'function') showWorkspaceTreeSkeleton();
|
||||
@@ -6049,22 +6062,37 @@ async function switchToProfile(name) {
|
||||
// new profile-scoped session.
|
||||
syncTopbar();
|
||||
await renderSessionList();
|
||||
// Safety net: if the new session has no workspace, newSession() won't have
|
||||
// painted the file tree — clear the up-front skeleton so it can't strand
|
||||
// (#4662 Opus gate). No-op when a real tree already rendered.
|
||||
if ((!S.session || !S.session.workspace) && typeof clearWorkspaceTreeSkeleton === 'function') {
|
||||
clearWorkspaceTreeSkeleton();
|
||||
}
|
||||
showToast(t('profile_switched_new_conversation', name));
|
||||
} else {
|
||||
// No messages yet — refresh the list and workspace tree in place. These
|
||||
// two fetches (/api/sessions and /api/list) are independent, so kick them
|
||||
// off together rather than serially: the slow session-list fetch should
|
||||
// not also delay the workspace tree resolving. The _switchGen guard below
|
||||
// still discards everything if a newer switch superseded us. (#4662)
|
||||
// No messages yet — refresh the list and topbar in place, then the
|
||||
// workspace tree. The loading skeletons shown up front (top of this
|
||||
// function) already give immediate cross-surface feedback, so we keep the
|
||||
// workspace refresh AFTER the stale-switch guard: loadDir() paints the
|
||||
// file tree as soon as its fetch resolves with only a session-id check,
|
||||
// and empty-session switches reuse the same session id — so starting it
|
||||
// before the guard could let an older switch's /api/list paint over a
|
||||
// newer one (Codex gate #4662). renderSessionList() is the slow fetch and
|
||||
// has its own internal generation guard, so awaiting it first is fine.
|
||||
const workspaceVisible = typeof _workspacePanelMode !== 'undefined' && _workspacePanelMode !== 'closed';
|
||||
const listLoad = renderSessionList();
|
||||
const dirLoad = (S.session && S.session.workspace) ? loadDir('.') : null;
|
||||
await listLoad;
|
||||
await renderSessionList();
|
||||
if (_switchGen !== _profileSwitchGeneration) return;
|
||||
syncTopbar();
|
||||
// Refresh workspace file tree so the right panel shows the new
|
||||
// profile's workspace, not the previous one (#1214).
|
||||
if (dirLoad && workspaceVisible) await dirLoad;
|
||||
if (S.session && S.session.workspace) {
|
||||
const dirLoad = loadDir('.');
|
||||
if (workspaceVisible) await dirLoad;
|
||||
} else if (typeof clearWorkspaceTreeSkeleton === 'function') {
|
||||
// New profile has no bound workspace — clear the up-front skeleton so it
|
||||
// doesn't strand (#4662 Opus gate).
|
||||
clearWorkspaceTreeSkeleton();
|
||||
}
|
||||
showToast(t('profile_switched', name));
|
||||
}
|
||||
|
||||
|
||||
@@ -475,6 +475,18 @@ function showWorkspaceTreeSkeleton(){
|
||||
tree.style.display = '';
|
||||
}
|
||||
|
||||
// Clear a stranded workspace-tree skeleton (#4662 Opus gate). showWorkspaceTreeSkeleton()
|
||||
// is shown up front on a profile switch, but the real loadDir('.') that would
|
||||
// replace it is skipped when the new profile has no bound workspace — leaving a
|
||||
// shimmering skeleton forever. Call this on the no-workspace path so the tree
|
||||
// empties instead. Only touches #fileTree when it still holds a skeleton, so
|
||||
// it can't clobber a real render.
|
||||
function clearWorkspaceTreeSkeleton(){
|
||||
const tree = $('fileTree');
|
||||
if(!tree) return;
|
||||
if(tree.querySelector('.skeleton-tree')) tree.innerHTML = '';
|
||||
}
|
||||
|
||||
async function loadDir(path, opts={}){
|
||||
const preservePreview=!!(opts&&opts.preservePreview);
|
||||
const refreshExpanded=!!(opts&&opts.refreshExpanded);
|
||||
|
||||
@@ -38,18 +38,53 @@ class TestSwitchWiring:
|
||||
assert "showWorkspaceTreeSkeleton()" in body
|
||||
assert "_workspaceVisibleAtStart" in body
|
||||
|
||||
def test_parallelizes_list_and_workspace_refresh(self):
|
||||
def test_workspace_refresh_after_switch_guard(self):
|
||||
# The workspace tree refresh (loadDir) must run AFTER the stale-switch
|
||||
# generation guard — loadDir paints the tree with only a session-id
|
||||
# check, and empty-session switches reuse the session id, so starting it
|
||||
# before the guard could let an older switch paint over a newer one
|
||||
# (Codex gate #4662). The skeletons shown up front already provide the
|
||||
# immediate cross-surface feedback.
|
||||
body = _switch_body()
|
||||
# The non-sessionInProgress branch kicks off both fetches before awaiting.
|
||||
assert "const listLoad = renderSessionList();" in body
|
||||
assert "loadDir('.')" in body
|
||||
assert "await listLoad;" in body
|
||||
guard = "if (_switchGen !== _profileSwitchGeneration) return;"
|
||||
# In the non-sessionInProgress branch, the loadDir('.') call must come
|
||||
# after an occurrence of the guard.
|
||||
dir_idx = body.index("const dirLoad = loadDir('.');")
|
||||
guard_before = body.rfind(guard, 0, dir_idx)
|
||||
assert guard_before != -1, "loadDir('.') must be preceded by the switch-generation guard"
|
||||
|
||||
def test_restores_real_content_on_failure(self):
|
||||
body = _switch_body()
|
||||
catch = body[body.index("} catch (e) {"):]
|
||||
assert "renderSessionListFromCache()" in catch, "failed switch must restore real list"
|
||||
|
||||
def test_noop_self_switch_early_returns(self):
|
||||
# Opus gate #4662: a switch to the already-active profile must bail before
|
||||
# showing a skeleton (activateCurrentProfile() doesn't pre-check), else it
|
||||
# flashes skeleton→restore.
|
||||
body = _switch_body()
|
||||
head = body[: body.index("showSessionListSkeleton()")]
|
||||
assert "name === S.activeProfile" in head, "missing no-op self-switch early-return"
|
||||
assert "return;" in head
|
||||
|
||||
def test_dismisses_rename_and_menu_before_skeleton(self):
|
||||
# Opus gate #4662: renderSessionListFromCache() early-returns while
|
||||
# _renamingSid / _sessionActionMenu is set — which would strand the
|
||||
# skeleton. switchToProfile must dismiss both before showing it.
|
||||
body = _switch_body()
|
||||
pre = body[: body.index("showSessionListSkeleton()")]
|
||||
assert "_renamingSid = null" in pre, "must clear inline-rename state before skeleton"
|
||||
assert "closeSessionActionMenu()" in pre, "must close row action menu before skeleton"
|
||||
|
||||
def test_clears_workspace_skeleton_when_no_workspace(self):
|
||||
# Opus gate #4662 (blocker): if the new profile has no bound workspace the
|
||||
# real loadDir is skipped, so the up-front workspace skeleton must be
|
||||
# explicitly cleared or it strands forever.
|
||||
body = _switch_body()
|
||||
assert body.count("clearWorkspaceTreeSkeleton()") >= 2, (
|
||||
"both switch branches must clear a stranded workspace skeleton"
|
||||
)
|
||||
|
||||
|
||||
class TestSessionsWiring:
|
||||
def test_skeleton_flag_cleared_on_real_render(self):
|
||||
@@ -69,6 +104,14 @@ class TestWorkspaceWiring:
|
||||
assert "const _WS_SKELETON_ROWS" in WORKSPACE
|
||||
assert "function showWorkspaceTreeSkeleton(" in WORKSPACE
|
||||
|
||||
def test_skeleton_clear_helper_defined(self):
|
||||
# The strand-clear helper must exist and only empty #fileTree when it
|
||||
# still holds a skeleton (so it can't clobber a real render).
|
||||
assert "function clearWorkspaceTreeSkeleton(" in WORKSPACE
|
||||
idx = WORKSPACE.index("function clearWorkspaceTreeSkeleton(")
|
||||
body = WORKSPACE[idx: idx + 400]
|
||||
assert ".skeleton-tree" in body, "clear helper must check for a skeleton before emptying"
|
||||
|
||||
|
||||
class TestSkeletonCss:
|
||||
def test_core_classes_present(self):
|
||||
|
||||
Reference in new issue
Block a user