From adcd06ff76955ff44403580f41df21af58dc25c6 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Mon, 22 Jun 2026 06:06:10 +0000 Subject: [PATCH] Phase 1 (#4662): address Codex + Opus gate findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- static/panels.js | 46 ++++++++++++---- static/workspace.js | 12 +++++ ...ssue4662_profile_switch_skeleton_static.py | 53 +++++++++++++++++-- 3 files changed, 97 insertions(+), 14 deletions(-) diff --git a/static/panels.js b/static/panels.js index 3e5bfc10..922ad211 100644 --- a/static/panels.js +++ b/static/panels.js @@ -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)); } diff --git a/static/workspace.js b/static/workspace.js index 3259598a..187e6c26 100644 --- a/static/workspace.js +++ b/static/workspace.js @@ -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); diff --git a/tests/test_issue4662_profile_switch_skeleton_static.py b/tests/test_issue4662_profile_switch_skeleton_static.py index 2a57fac5..21ceee5f 100644 --- a/tests/test_issue4662_profile_switch_skeleton_static.py +++ b/tests/test_issue4662_profile_switch_skeleton_static.py @@ -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):