fix(chat): resolve a history resume once, where every entry point can see it - #7471
Conversation
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS Every resume entry point now narrates its outcome in one always-visible spot with plain, surface-free copy — the dead click and the tab-eating swap are gone. Suggestions
[UX-REVIEWED] 241e9ee |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All claims verified. The two palette providers both First-Principles-Verdict: PASS A destructive, unexplainable dead click is fixed at the one place that knows the outcome, deleting the per-caller predicate instead of adding a fifth copy. What this change shipsIntent: stop a "resume this session" click from silently failing — and, on the chat page, from destroying the tab and draft the user is in. This is a FIX.
Net shape is subtractive where it counts: the sidebar's local notice, its predicate copy, and its sequence ref are deleted; the slice records raw facts once. The server-side residual (slot published before the surface check) is named and honestly deferred. [FIRST-PRINCIPLES-REVIEWED] 241e9ee |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Root-cause shape: the outcome predicate moves into the thunk's own lifecycle reducers, so all five entry points read one answer instead of four re-deriving it. [DESIGN-REVIEWED] 241e9ee |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsNo blocking issues; one advisory finding. FINDING — website/src/store/chatSlice.ts:5303 — the [OPUS-REVIEWED] 241e9ee Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/src/pages/ChatPage.tsx:952 -- False positive or not applicable? A repository writer can comment: |
92c3705 to
9714eec
Compare
9714eec to
8b0de85
Compare
UX Review disposition -- both items FIXED at
|
8b0de85 to
1cdec4d
Compare
GPT disposition -- BLOCKING finding is CORRECT and FIXED at
|
1cdec4d to
ec5e318
Compare
UX Review (PASS) suggestion dispositions -- 2 of 3 FIXED at
|
UX Review (PASS) round-5 suggestion dispositions -- both DEFERRED, with the blocking constraint named for each. No push: the head is unchanged at
|
… see it Resuming a persisted session succeeds on the wire whether or not the session's surface is one the chat page can display, so `ok` alone cannot tell a usable resume from one that bounces. #3640 taught a single call site -- the sidebar's history row -- to read the returned `surface`. Four siblings kept resolving blind: ChatPage's "Continue a previous chat" list, the notification panel's Resume button, and the `recents` / `sessions` command-palette providers. Blind was not merely silent at the ChatPage site. `handleResumeSession` performs a swap -- resume, then retire the tab being replaced -- and a resume that never took effect still ran the second half, closing the tab the user was in, discarding the text they had just typed into it, and bouncing them to an unrelated peer while the session they asked for never opened. `unwrap()` does not catch it: the thunk resolves for `ok: false` too, so an outright failed resume did the same. The predicates now live in `resumeFromHistory`'s own cases and record `{key, title, surface, reason}` on the slice. `reason` separates the two ways a resume disappoints, because they need different sentences: `surface` succeeded but landed somewhere the chat page cannot show, and `failed` did not succeed at all -- a rejected request (every non-2xx, so the likeliest failure of all) or a payload saying `ok: false`. Every caller swallowed that second class silently, which was the same dead click on a rarer path. ChatPage renders the notice with its pane-level banners, outside the split / no-slot / transcript ternary: a palette or notification resume can arrive with NO active slot, and that ternary's `!activeSlot` branch renders only the empty state, so a notice nested in the transcript branch was silent in exactly the condition those two paths create. It is deliberately not in the sidebar, where #3640 put it: that pane's Older Sessions section starts closed, so a notice inside it is invisible to everyone arriving from the other three paths. The two palette providers are plain modules with no component of their own, and a shared render site is what lets them narrate at all. The notification panel now navigates whatever the outcome, because that destination is where the explanation lives; staying put is what made the button look dead. The message moved with it, and lost the sidebar. #3640's string said "can't be opened from the chat sidebar", naming a surface three of the four entry points never touch. The key is re-namespaced to `pages.chatPage` and reworded to "can't be opened in chat" in all 12 catalogs, each keeping its shipped phrasing with only the sidebar clause replaced. The surface label is resolved, never interpolated raw: the wire value is a machine word, so it rendered lowercase vocabulary mid-sentence ("it's a subagent session") and its empty case read "it's a Session session". Now the localized dashboard label, the channel label, or the surface registry's own label -- and a sentence naming no surface when none of those resolve. Ordering moves with the check. The sidebar's local sequence ref could only order its own clicks; keying on the thunk's requestId orders a palette resume racing a sidebar one, which was unordered before.
ec5e318 to
241e9ee
Compare
Backend Tests (3.10, 2) red is a timing flake in a test that landed an hour ago -- not this diff. Failed jobs re-run.
# Still no _settle(): comment:3 lands inside comment:2's floor, which is what
# gives the two something to coalesce INTO.
behind = _verdict(probe, coalesce_secs=_COALESCE)
assert isinstance(behind, Skip)Why it is a flake rather than a defect:
Re-ran the failed jobs rather than touching code. Flagging it for the maintainer because the tight wall-clock margin will keep hitting unrelated PRs until the test gets an injected clock or a wider floor. |
UX Review (PASS) round-6 disposition -- suggestion ACCEPTED on the merits, held off this head deliberately. No push.
Both halves are right, and the second option is the better one. Worth saying plainly: "Try again." is a claim the code cannot back. For a palette or notification arrival the retry control left the screen with the I am NOT pushing it onto this head, for a reason specific to where the PR now sits rather than any disagreement:
So it is queued rather than dropped: a one-clause edit across 12 catalogs plus the pseudolocale, no code logic, no new affordance. It rides along with the next push this PR takes for any other reason, or lands as a follow-up if this merges as-is. Flagging it for the maintainer to green-light either way. The |
bolichen97
left a comment
There was a problem hiding this comment.
Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.
|
One unanswered finding from review worth fixing before/after merge: GPT finding on The new test ( Suggested fix: move the member-registry lookup ahead of the Not blocking (cosmetic mislabel, no data loss), but worth closing out since it contradicts the PR description's own claim. |

1. What is the problem?
Resuming a persisted session succeeds on the wire whether or not that session's
surface is one the chat page can display, so the thunk's
okcannot tell ausable resume from one that bounces. PR #3640 taught ONE call site -- the
sidebar's Older Sessions row -- to read the returned
surfaceand say so. Foursiblings kept resolving blind:
website/src/pages/ChatPage.tsxhandleResumeSession(the "Continue aprevious chat" list above the composer) -- unwrapped, no surface check
website/src/components/notifications/NotificationDetailPanel.tsx-- awaitedthe dispatch with no
.unwrap(), so it could not readsurfaceat allwebsite/src/components/commandPalette/providers/recentsProvider.tswebsite/src/components/commandPalette/providers/sessionsProvider.tsBlind was not merely silent at the ChatPage site.
handleResumeSessionperformsa SWAP: resume the picked session, then retire the tab it replaces. A resume that
answered with an undisplayable surface never performs the first half -- the
resumeFromHistory.fulfilledreducer short-circuits, soactiveSlotstill namesthe tab the user is in -- but the second half ran anyway: it deleted that tab and
discarded the text the user had just typed into it (the suggestions list only
appears once they type), then
deleteSlotmoved them to an unrelated peer, whilethe session they asked for never opened.
A second, larger silent class sits beside it: a resume that FAILS.
unwrap()does not catch
ok: false, because that is a fulfilled payload -- and the commonfailure is not even that shape.
api.resumeChatSlotthrows on any non-2xx(
j()inapi/client.ts), so a 404 / 409 / 5xx / dropped connection lands onresumeFromHistory.rejected, and every caller swallowed it: ChatPage'scatch {}, the palette providers'void dispatch, the notification panel'sconsole log. Same dead click, on the path users actually hit.
2. Why this issue matters to the user
Every one of these is a dead click on a session the product itself listed as
resumable, and the ChatPage one is worse than dead: press it and the tab you were
working in disappears along with your half-typed message, for a session that
never opens. The two palette providers cannot narrate anything on their own --
they are plain modules that return command descriptors, with no component -- so
without a shared answer they were structurally unable to explain themselves.
3. How our fix solves it
The chain, symptom back to cause: the outcome existed on the wire, but each
caller had to re-derive "did this resume leave me in a usable session" for
itself, so four of five never did, and the one that did put its answer somewhere
the other four cannot reach.
resumeFromHistoryrecords{key, title, surface, reason}on the slice fromits own cases:
fulfilled+ undisplayable surface,fulfilled+!ok, andrejected.reason: 'surface' | 'failed'separates the two ways a resumedisappoints, because they need different sentences. No caller re-derives
anything.
its pane-level banners (
uploadError/sidError/pinStatus), OUTSIDE thesplit / !activeSlot / transcriptternary. That position matters: a palette ornotification resume calls
navigate('/chat')unconditionally, so it can arrivewith NO active slot, and the ternary's
!activeSlotarm renders only the emptystate -- a notice nested in the transcript branch is silent in exactly the
condition those two entry points create. (It first shipped in the composer slot
and GPT caught the hole; disposition comment on this PR, regression test
below.) Deliberately not the sidebar either, where fix(dashboard): show a notice when a chat sidebar resume can't be displayed #3640 put it:
historyOpendefaults to
false(ChatSidebar.tsx), so the Older Sessions pane startsCLOSED and a notice inside it can only be seen by someone who had already
opened it. All four paths end on
/chat, which is what lets the twocomponent-less providers narrate at all. The sidebar's copy is removed, so
nothing double-narrates.
handleResumeSessionreturns before theswap cleanup unless
result.ok && isChatPageSurface(result.surface)-- thetab and its drafts survive a resume that did not happen.
.unwrap()isadded so its
catchis reachable for the diagnostic, but it does NOT gate thenavigation:
/chatis where the explanation renders, so going there is howthe user learns what happened. Staying put is what made the button look dead.
the chat sidebar", which names a surface three of the four entry points never
touch -- and after (2) the notice is not in the sidebar for anyone. The key is
re-namespaced to
pages.chatPage.this_session_cannot_be_opened_in_chatandreworded in all 12 catalogs; each translation keeps its shipped phrasing with
only the sidebar clause replaced, so the diff is one line per catalog. Caught
by looking at the rendered frame, not by a test.
surfaceis a machine value, so dropping it into localized copy rendered lowercase
vocabulary mid-sentence ("it's a subagent session"), and its empty case fell
through to the generic label and read "it's a Session session" -- the exact
mislabel fix(dashboard): show a notice when a chat sidebar resume can't be displayed #3640's own comment warned about. Now: the localized dashboard label
for a
dashboard*key,slotChannelLabelfor a channel key,surfaceLabel(findSurfaceBySlotMode(surface))for a registered surface(covers
member-> "Crew Members"), and when none of those resolve, asentence that names no surface -- and does not say "surface" either, which is
vocabulary a user meets only in settings prose (UX review).
could only order its OWN clicks; keying on the thunk's
requestId(
lastResumeRequestId, set onpending) also orders a palette resume racinga sidebar one, which was unordered before.
The slice stores raw facts rather than a finished sentence, because a reducer
cannot localize: the surface label is derived from the session KEY, and that
derivation lives at the render site.
The three sentences, as they render
Real pixels: the shipped
ErrorNoticewith the real i18n strings, at the exactpane-level container recipe ChatPage wraps them in. It is the notices alone rather
than a full page shot -- the PLACEMENT is proven by the mounted-page tests below,
one of which renders with no active slot and fails if the notice is nested in a
view branch. A live-session
capture was attempted first and abandoned: the isolated pod withholds its API
credential on a host without
lsof, so no authenticated page could be driven.Deliberately not in scope
The round-2 design-review residual on #3640 is real and untouched here:
api_chat_slot_resumepublishes a live slot (get_or_create_slot+push_slots_update) before the client can evaluate the returned surface, and/api/sessionsis fetched withexclude_open=1, so a blind resume also dropsthe row out of Older Sessions on the next refetch and leaves a slot the chat page
can neither show nor close. Removing that needs the session's persisted
modeon/api/sessionsrows (SessionCatalogProjection.list_sessionsemitskey,messages,modified,created,title,agent,memory_mode,folder_id--no
mode), which is a separate server change. This PR stops the destruction andgives the user an explanation; it does not claim to have removed that side
effect.
4. What tests we did
New, all mutation-verified red against the pre-fix code:
website/src/store/chatSlice.unresumableResume.test.ts(7) -- what is recordedfor an undisplayable surface, for
ok: false, and for a rejected request; thata displayable resume clears rather than narrates; that
pendingclears a stalenotice; that a superseded resume answering late is ignored; and that dismissing
does not disarm the ordering token. Removing the two failure branches turns the
2 failure cases red; removing the whole field turns all 7 red.
website/src/test/ChatPage.resumeSurfaceGate.test.tsx(3) -- the swap cleanupdoes not run (no
deleteChatSlot, draft intact) for an undisplayable surfaceor an
ok: falseanswer, and still runs for a displayable one. Removing thegate line turns 2 of the 3 red.
website/src/test/ChatPage.unresumableNotice.test.tsx(7) -- mounts the realpage: nothing renders with an empty slice field; the notice renders with NO
active slot (the state a palette or notification resume arrives in), asserting
the empty state is present so it cannot silently re-test the transcript branch;
the surface sentence names the session and its localized surface and never
contains the word "sidebar"; the failure sentence names no surface; an
unregistered surface gets the surface-free sentence and never leaks the machine
word
subagent; a registered surface renders "Crew Members"; Dismiss clears theslice. Nesting the notice back inside the transcript branch turns the no-slot
case red.
Regression runs (targeted, per this repo's CPU rules -- no full suite locally):
chatSlice.test.ts(250),ChatSliceCoverage(88),ChatSliceCoverageSecondPass(95),chatSlice.olderHistoryCursor(14),chatSlice.abortOlderOnSwitch(23),olderHistoryTrigger(13),ChatSidebar.offline(8),ChatPage.refusedPress(4),NotificationsPanel(11)-- all pass.
Gates:
tsc --noEmitclean;eslinton every touched file reports 0 errors;node scripts/i18n-check.mjsok on all 11 sub-checks including catalog parity,dead keys and the key-reference gate;
gen-pseudolocale.mjsre-run soen-XAtracks the three keys.
5. Any other suggestions on the work
Frontend Lint & Type Checkwas red on main's own content. That claim was correct when measured and is now
obsolete -- recorded here rather than quietly deleted. The branch was based on
a main that carried 660 warnings against a ratchet of 659 (the extra one arrived
with refactor(dashboard): own the queue-card action recipe once, and fix cancel losing a split-pane draft #7319's
website/src/test/useQueuedMessageActions.test.tsx, and mainmeasured exactly 659 at
41011a748^). fix(ci): burn one eslint warning down to the ratchet ceiling #7480 burned that warning back down andmerged at 05:46Z. Rebased onto
7308a8b30, this branch now measures 652warnings with
npx eslint src/, so the lane passes on its own merit and noratchet exemption is needed. Every touched file still produces the same warning
count as its base version -- this diff adds zero either way.
affordance fix(dashboard): show a notice when a chat sidebar resume can't be displayed #3640 shipped. The trade is argued in section 3 -- worth a
maintainer's eye if the sidebar placement was load-bearing for a reason not
visible in the code.
navigateonresult.okis a deliberatedivergence from the UX review's suggested shape, argued in the disposition
comment. If the preference is for that button to stay put and narrate locally,
that is a second render site and one more decision.
handleResumeSessionis inline inChatPageand not exported, so its testmounts the same body against the real thunks and reducer rather than importing
it, following the precedent documented in
ChatPage.handleFork.test.tsx. Ifthe callback body changes, that test file must change with it. Exporting the
handler (or lifting the swap into a thunk) would remove the duplication.
this PR now uses the registry lookup that would power it
(
findSurfaceBySlotMode). Routing the user to the surface that owns thesession is reachable once
/api/sessionscarriesmode; the fallback for amode that resolves to no advertised surface is a product decision.
Pattern harvest
Rule candidate: eslint
Pattern:
await dispatch(<asyncThunk>(...))inside atry/catchWITHOUT.unwrap(). A thunk dispatch promise resolves even when the thunk rejected, sothe
catchis unreachable dead code and the statements after the await run on afailure. Purely syntactic, no false positives worth an exemption -- and it is
exactly what hid the notification panel's failure path here.
Second candidate, same family: flag a call site that awaits an async thunk whose
fulfilled payload carries a CAPABILITY or OUTCOME discriminator (
surface,mode,kind,ok) and then performs a side effect without branching on it.All five resume sites had the discriminator in hand; four ignored it, and one of
those four used the ignored answer to delete the user's tab.
Not generalizable: the specific defect SHAPE above it -- a fix landing at the one
call site that reported the bug while its siblings keep the old behaviour, and
the fix's own affordance parked somewhere the siblings cannot reach -- is a review
habit rather than a lint. When a fix adds a predicate, ask where the predicate
lives, not whether the reporting site now passes.
Closes #5925