fix(webui): replace cross-profile empty sessions on profile switch - #5460
ruizanthony wants to merge 2 commits into
Conversation
|
| Filename | Overview |
|---|---|
| static/sessions.js | Introduces _openSidebarSession (consolidating all session-open entry points), _ensureSidebarSessionProfile (cross-profile switch before loadSession), and localStorage persistence for _showAllProfiles. Core logic is sound; the return value of _ensureSidebarSessionProfile is not checked by the caller, so a failed switch still proceeds to loadSession (already flagged in prior review). |
| static/panels.js | Adds cross-profile mismatch detection post-switch, the _openingExistingSidebarSession fast-path branch, and _openProfileSwitchSessionBrowser helper. The new _openingExistingSidebarSession branch omits syncTopbar(), which the other two success branches both call explicitly; profile-specific topbar state (model selector, workspace picker) may lag until loadSession completes. |
| tests/test_profile_switch_ux.py | Adds three new regression tests covering cross-profile session replacement, the session-browser helper presence and ordering, and desktop/mobile UI path coverage. Tests are static-analysis based and well-structured. |
| tests/test_issue1611_session_profile_filtering.py | Adds three new tests: profile-switch-before-open ordering, localStorage persistence for all-profiles toggle, and absence of _showAllProfiles reset in switchToProfile. All correctly track the new code paths. |
| tests/test_issue1700_parallel_profile_switch.py | Updates sessionInProgress declaration extraction to use regex instead of string search, correctly handling the change from const to let. |
| tests/test_issue3603_external_session_import_gate.py | Refactors external-session import gate tests to verify the shared _openSidebarSession helper rather than per-callsite checks. Correctly reflects the consolidated helper pattern. |
| tests/test_issue4662_profile_switch_skeleton_static.py | Introduces _show_session_skeleton_call_idx helper to handle the new showSessionListSkeleton(name) call signature; updates string searches to be more robust. No logic concerns. |
| tests/test_firefox_sidebar_scroll_stability.py | Updates two string assertions to match the new _setShowAllProfiles() calls with deferWhileInteracting:false. Straightforward tracking update. |
| tests/test_session_lineage_collapse.py | Updates two assertions to reflect the consolidated _openSidebarSession helper replacing the inline loadSession calls for lineage/child sessions. |
| tests/test_session_touch_actions.py | Updates touch gesture test to search for _openSidebarSession instead of loadSession in the gesture-finish block. Correctly tracks the refactored open path. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant SidebarRow as Sidebar Row click
participant OSS as _openSidebarSession
participant ESP as _ensureSidebarSessionProfile
participant STP as switchToProfile
participant API as /api/profile/switch
participant LS as loadSession
User->>SidebarRow: click cross-profile session
SidebarRow->>OSS: _openSidebarSession(session)
OSS->>ESP: _ensureSidebarSessionProfile(session)
ESP->>ESP: "check _showAllProfiles && profile mismatch"
ESP->>ESP: "set _profileSwitchOpeningExistingSession=true"
ESP->>STP: await switchToProfile(targetProfile)
STP->>STP: "capture _openingExistingSidebarSession=true (local const)"
STP->>STP: "sessionInProgress=true (S.session exists)"
STP->>API: POST /api/profile/switch
API-->>STP: active, is_default, ...
STP->>STP: skip profile-mismatch check (sessionInProgress already true)
STP->>STP: "skip S.session retag (sessionInProgress=true)"
STP->>STP: renderSessionList() + showToast()
STP-->>ESP: returns
ESP->>ESP: "finally: _profileSwitchOpeningExistingSession=false"
ESP-->>OSS: return (success/failure)
OSS->>LS: loadSession(session.session_id, loadOpts)
LS-->>OSS: session loaded
OSS->>OSS: renderSessionListFromCache()
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant SidebarRow as Sidebar Row click
participant OSS as _openSidebarSession
participant ESP as _ensureSidebarSessionProfile
participant STP as switchToProfile
participant API as /api/profile/switch
participant LS as loadSession
User->>SidebarRow: click cross-profile session
SidebarRow->>OSS: _openSidebarSession(session)
OSS->>ESP: _ensureSidebarSessionProfile(session)
ESP->>ESP: "check _showAllProfiles && profile mismatch"
ESP->>ESP: "set _profileSwitchOpeningExistingSession=true"
ESP->>STP: await switchToProfile(targetProfile)
STP->>STP: "capture _openingExistingSidebarSession=true (local const)"
STP->>STP: "sessionInProgress=true (S.session exists)"
STP->>API: POST /api/profile/switch
API-->>STP: active, is_default, ...
STP->>STP: skip profile-mismatch check (sessionInProgress already true)
STP->>STP: "skip S.session retag (sessionInProgress=true)"
STP->>STP: renderSessionList() + showToast()
STP-->>ESP: returns
ESP->>ESP: "finally: _profileSwitchOpeningExistingSession=false"
ESP-->>OSS: return (success/failure)
OSS->>LS: loadSession(session.session_id, loadOpts)
LS-->>OSS: session loaded
OSS->>OSS: renderSessionListFromCache()
Reviews (7): Last reviewed commit: "fix(webui): open all-profile sidebar row..." | Re-trigger Greptile
|
Read SummaryThe stale-session-after-switch upload failure is real. def _session_visible_to_active_profile(session) -> bool:
"""Return whether an upload target session belongs to the active profile."""
session_profile = getattr(session, 'profile', None)
...
return _profiles_match(session_profile, _get_active_profile_name())and returns Code referenceThe new gate in if (!sessionInProgress && S.session) {
const currentSessionProfile = (typeof S.session.profile === 'string' && S.session.profile.trim())
? S.session.profile.trim() : 'default';
sessionProfileMatchesTarget = (typeof _profileMatchesActiveProfile === 'function')
? _profileMatchesActiveProfile(currentSessionProfile, targetActiveProfile)
: (currentSessionProfile === targetActiveProfile || ...);
if (!sessionProfileMatchesTarget) sessionInProgress = true;
}Two things I checked that make this correct rather than fragile:
One thing worth confirming
VerificationThe added |
🔬 Gate certification — RED ⛔ (behavior is sound; 3 brittle STATIC tests misalign with the improved code — CI red)Certified head: What I ran (rebased worktree
|
| Gate | Result |
|---|---|
| Rebase onto current master | ✅ git apply clean |
| Codex (reproduce) | SAFE TO SHIP — 0 findings; switch logic sound, failures are brittle-static-test misalignment |
| Full pytest suite | 5 failed / 11827 passed — 2 known env flakes (nous, issue4536) + the 3 brittle static tests below |
| PR's own test | ✅ test_profile_switch_ux.py passes |
Findings — 3 brittle static tests, behavior VERIFIED intact
⛔ CI-red (3 brittle static tests; I confirmed each is a string-match artifact, not behavior loss):
test_issue4662...::test_switch_post_suppresses_generic_timeout_toast— slices a ±200-char window aroundbody.index("/api/profile/switch"). The PR inserted a comment block containing/api/profile/switchBEFORE the actualapi(..., timeoutToast: false)call, soindexfinds the comment and the window misses the call.timeoutToast: falseIS still on the switch call (panels.js:6484) — behavior intact. Fix: anchor on theapi('/api/profile/switch'call (e.g. scope toapi(+ the path, orrindex), not a naive first-indexwindow.test_issue4662...::test_shows_session_skeleton_up_front— asserts the literal"showSessionListSkeleton()". The PR changed the call toshowSessionListSkeleton(name)(parameterized) + guarded withtypeof … === 'function'(panels.js:6471) — skeleton still shown. Fix: assertshowSessionListSkeleton((allow an argument).test_issue1700...::test_frontend_treats_active_or_pending_session_as_in_progress— slicesfn.find("const sessionInProgress") : fn.find("try {", ...). The PR changedconst sessionInProgress→let sessionInProgress(because it legitimately reassigns it at 6498:sessionInProgress = true). Sofind("const …")returns −1 → empty slice → assert fails. TheS.session.active_stream_idin-progress guard IS fully intact (panels.js:6451-6454) — behavior sound. Fix: matchsessionInProgresswithout theconstprefix (acceptlet/const).
✅ Behavior sound: the switch fix (replace-only for a cross-profile empty session vs same-profile retag) is correct; timeoutToast:false, skeleton, and the active_stream_id/pending in-progress guard are all still wired. Codex SAFE (0 findings). Positive nesquena-hermes read ("diagnosis correct, targets the right seam").
Recommendation to the next agent
RED — gate-fail/changes-requested (CI-red, test-only fixes): update the 3 brittle static assertions to match the improved code — anchor timeoutToast on the api('/api/profile/switch' call, accept showSessionListSkeleton( with an arg, and match sessionInProgress regardless of let/const. The PR's behavior is verified correct (Codex SAFE + its own test + the guards all intact) — this is purely static-test brittleness that the legitimate refactor exposed. Fast to fix. Once green, it's a crown-jewel profile-switching reliability fix (stale-session upload-404). concept 4/5. Author @ruizanthony (T1). crit=3. (Gate lesson: static source-string tests that slice fixed windows / match exact call-literals / assume const are brittle — a legitimate refactor breaks them without any behavior change; when a static test fails, verify the actual behavior is intact before treating it as a defect, and fix the test to be structure-tolerant.)
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy. Rebased onto current master; all 3 CI failures confirmed brittle-static-test misalignment (timeoutToast:false@6484, showSessionListSkeleton(name)@6471, active_stream_id guard@6451-54 all intact), Codex SAFE 0 findings. Cert valid for sha:51a5a7b9.
b215127 to
be92ca1
Compare
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
Fixed in Changes made:
Local validation:
Remote validation on |
🔬 Gate certification — GREEN ✅ · CONVERGED (brittle tests fixed + profile-match logic refined)Certified head: What I ran (rebased worktree
|
| Gate | Result |
|---|---|
| Rebase onto current master | ✅ git apply clean; three-dot diff = panels.js + 3 tests ONLY |
| Codex (reproduce) | SAFE TO SHIP — gated the rebased worktree, 0 findings |
| Full pytest suite | 2 failed / 11830 passed — both non-defects (nous env flake + test_issue4536 isolation flake) |
| Previously-brittle tests | ✅ 31/31 pass (test_issue4662 + test_issue1700 + test_profile_switch_ux) |
Findings
✅ Brittle tests fixed + logic refined, behavior sound: the 3 static tests I flagged (timeoutToast window-slice, showSessionListSkeleton() literal, const sessionInProgress) are updated to match the refactor. The re-push also refined the profile-match seam: targetActiveProfile = S.activeProfile || 'default' (simpler than || data.active || name), and the cross-profile empty-session match uses (currentSessionProfile === targetActiveProfile || (currentSessionProfile === 'default' && !!S.activeProfileIsDefault)). Codex verified the replace-only decision for a cross-profile empty session is still correct (empty session whose recorded profile ≠ target → replace; same-profile/has-messages → retag/keep), no misclassification of default-profile sessions, and timeoutToast:false + skeleton + active_stream_id in-progress guard all intact. 0 findings.
ℹ️ Stale-base false-alarm noted (resolved): a cross-merge diff (my old fail-sha → new head, spanning the #5454 merge) appeared to show a style.css -9 removing the transparent-event keyframe — but the three-dot diff vs current master confirms #5460 touches ONLY panels.js + tests; #5454's animation removal is intact in the rebased worktree (0 keyframe refs). Not a revert.
Recommendation to the next agent
Ready to merge — use branch gate-rebase/5460-cross-profile-empty-session (sha:6a93b00d), NOT the PR's stale head be92ca13. Converged: the 3 brittle tests are fixed and the profile-match logic refinement is sound (Codex SAFE + 31/31 + suite green bar 2 known flakes, all switch guards intact). Crown-jewel profile-switching reliability (fixes the cross-profile empty-session upload-404). Backend/JS logic — a quick profile-switch-then-upload smoke confirms, but the mechanism + guards are verified. concept 4/5. Credit @ruizanthony (co-authored). crit=3. (Gate note: checked the three-dot diff to dismiss a cross-merge style.css false-alarm — #5454 intact.)
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy. Rebased onto current master; 3 brittle tests verified fixed (31/31), profile-match logic refinement Codex-SAFE (replace-decision + guards intact), stale-base style.css false-alarm dismissed via three-dot diff. Cert valid for sha:6a93b00d.
be92ca1 to
a456dbd
Compare
|
Follow-up pushed in Additional UX fix:
Local validation:
Remote validation on |
ed1a917 to
4e2445b
Compare
4e2445b to
deec3cb
Compare
deec3cb to
d0ee292
Compare
Re-review — scope expanded past the certified head (
|
…open all-profile rows under owning profile Clean rebase of ruizanthony's #5460 (rebase-first). Co-authored-by: ruizanthony <ruizanthony@users.noreply.github.com>
🔬 Gate certification — GREEN ✅ (re-gated at expanded head d0ee292)Certified head: What I ran (rebased worktree
|
| Gate | Result |
|---|---|
| Rebase onto current master | ✅ git apply clean; three-dot diff = panels.js + sessions.js + 8 test files |
| Codex (reproduce) | SAFE TO SHIP — gated the rebased worktree, 0 findings |
| Full pytest suite | 2 failed / 11859 passed — both non-defects (nous env flake + test_issue4536 isolation flake) |
| Touched profile-isolation tests | ✅ 131/131 (test_profile_switch_ux, #1611 filtering, #3603 import-gate, #1700 parallel, #4662 skeleton, lineage) |
Findings
✅ Expanded behavior sound, profile isolation strengthened: (A) the cross-profile empty-session replace (my prior GREEN) is unchanged. (B) the new _ensureSidebarSessionProfile(session): when _showAllProfiles and the clicked session's profile ≠ active, it await switchToProfile(targetProfile) BEFORE loadSession() — so a cross-profile row opens under its OWNING profile (no misattribution to the active profile), gated on _showAllProfiles + _profileMatchesActiveProfile (skips if already matching), coordinated via a _profileSwitchOpeningExistingSession flag. Codex verified: no cross-profile misattribution/leak, the external-session-import-gate (#3603) is NOT weakened, profile-filtering (#1611) still scopes rows correctly, the coordination flag doesn't desync on switch failure, and the empty-session-replace + parallel-switch (#1700) + skeleton (#4662) paths don't regress. _showAllProfiles persists via localStorage safely. 0 findings, 131/131 touched tests.
Recommendation to the next agent
Ready to merge — use branch gate-rebase/5460-cross-profile-empty-session (sha:5e5b44e7), NOT the PR's stale head d0ee2928. Supersedes my earlier GREEN @be92ca13 (scope expanded). The added "open all-profile row under owning profile" behavior actually strengthens the crown-jewel profile-switching isolation (a cross-profile session opens under its owner), with the import-gate + profile-filtering verified intact. Codex SAFE + 131/131 touched + suite green bar 2 known flakes. Profile-switching surface — a quick all-profiles-view cross-profile click smoke confirms, but the switch-before-load + isolation are verified. concept 4/5. Credit @ruizanthony (co-authored). crit=3.
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy. Re-gated at expanded head d0ee292; the new open-under-owning-profile behavior verified isolation-strengthening (switch-before-load, import-gate + filtering intact, coordination flag safe), Codex SAFE + 131/131 touched + suite green bar 2 known flakes. Cert valid for sha:5e5b44e7.
Release — replace cross-profile empty sessions on profile switch (#5460)
|
Shipped in v0.51.848 — thanks @ruizanthony! 🎉 Cross-profile empty-session replacement on profile switch is live: switching profiles with an empty current chat no longer leaves a stale session id that 404s your next upload, and clicking a session owned by another profile now switches to that profile before loading (instead of 404ing). Full crown-jewel gate: Codex SAFE + Opus SAFE (no cross-profile data exposure/mis-scoping; empty-session replacement only fires for a genuinely-empty differing-profile session; no regression to the profile-switch skeleton/parallel-switch race/session-profile filter/external-import gate), full suite green (2 unrelated env flakes), 54/54 targeted. Credited via Co-authored-by. |
…sessions on profile switch
…n profile switch
Summary
S.session.session_idafter/api/profile/switchsucceeds.S.session.session_idand the backend correctly rejects sessions outside the active profile.Why
The profile switch path previously reused/retagged an empty current session in place. If that session belonged to the previous profile, later attachment uploads used the stale
session_id;api/upload.pythen returned404 Session not foundunder the new profile cookie. Users had to hard reload before uploading.Tests
node --check static/panels.jspython -m pytest tests/test_profile_switch_ux.py::TestParallelizedFetches::test_cross_profile_empty_session_is_replaced_before_mutation_or_upload tests/test_chat_upload_attachment_paths.py -q -o 'addopts='