Skip to content

fix(chat): resolve a history resume once, where every entry point can see it - #7471

Merged
bolichen97 merged 1 commit into
mainfrom
fix/resume-history-blind-resolve
Sep 1, 2026
Merged

fix(chat): resolve a history resume once, where every entry point can see it#7471
bolichen97 merged 1 commit into
mainfrom
fix/resume-history-blind-resolve

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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 ok cannot tell a
usable resume from one that bounces. PR #3640 taught ONE call site -- the
sidebar's Older Sessions row -- to read the returned surface and say so. Four
siblings kept resolving blind:

  • website/src/pages/ChatPage.tsx handleResumeSession (the "Continue a
    previous chat" list above the composer) -- unwrapped, no surface check
  • website/src/components/notifications/NotificationDetailPanel.tsx -- awaited
    the dispatch with no .unwrap(), so it could not read surface at all
  • website/src/components/commandPalette/providers/recentsProvider.ts
  • website/src/components/commandPalette/providers/sessionsProvider.ts

Blind was not merely silent at the ChatPage site. handleResumeSession performs
a 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.fulfilled reducer short-circuits, so activeSlot still names
the 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 deleteSlot moved them to an unrelated peer, while
the 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 common
failure is not even that shape. api.resumeChatSlot throws on any non-2xx
(j() in api/client.ts), so a 404 / 409 / 5xx / dropped connection lands on
resumeFromHistory.rejected, and every caller swallowed it: ChatPage's
catch {}, the palette providers' void dispatch, the notification panel's
console 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.

  1. One post-resolve check, three recording sites, one field.
    resumeFromHistory records {key, title, surface, reason} on the slice from
    its own cases: fulfilled + undisplayable surface, fulfilled + !ok, and
    rejected. reason: 'surface' | 'failed' separates the two ways a resume
    disappoints, because they need different sentences. No caller re-derives
    anything.
  2. One render site, chosen for visibility. ChatPage renders the notice with
    its pane-level banners (uploadError / sidError / pinStatus), OUTSIDE the
    split / !activeSlot / transcript ternary. That position matters: a palette or
    notification resume calls navigate('/chat') unconditionally, so it can arrive
    with NO active slot, and the ternary's !activeSlot arm renders only the empty
    state -- 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: historyOpen
    defaults to false (ChatSidebar.tsx), so the Older Sessions pane starts
    CLOSED 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 two
    component-less providers narrate at all. The sidebar's copy is removed, so
    nothing double-narrates.
  3. The destructive half is gated. handleResumeSession returns before the
    swap cleanup unless result.ok && isChatPageSurface(result.surface) -- the
    tab and its drafts survive a resume that did not happen.
  4. The notification panel navigates whatever the outcome. .unwrap() is
    added so its catch is reachable for the diagnostic, but it does NOT gate the
    navigation: /chat is where the explanation renders, so going there is how
    the user learns what happened. Staying put is what made the button look dead.
  5. The message lost the sidebar. fix(dashboard): show a notice when a chat sidebar resume can't be displayed #3640's string read "can't be opened from
    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_chat and
    reworded 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.
  6. The surface label is resolved, never interpolated raw. The wire surface
    is 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, slotChannelLabel for a channel key,
    surfaceLabel(findSurfaceBySlotMode(surface)) for a registered surface
    (covers member -> "Crew Members"), and when none of those resolve, a
    sentence that names no surface -- and does not say "surface" either, which is
    vocabulary a user meets only in settings prose (UX review).
  7. Ordering moved with the check. The sidebar's component-local sequence ref
    could only order its OWN clicks; keying on the thunk's requestId
    (lastResumeRequestId, set on pending) also orders a palette resume racing
    a 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

The three unresumable-resume sentences at the shipped container width

Real pixels: the shipped ErrorNotice with the real i18n strings, at the exact
pane-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_resume publishes a live slot (get_or_create_slot +
push_slots_update) before the client can evaluate the returned surface, and
/api/sessions is fetched with exclude_open=1, so a blind resume also drops
the 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 mode on
/api/sessions rows (SessionCatalogProjection.list_sessions emits key,
messages, modified, created, title, agent, memory_mode, folder_id --
no mode), which is a separate server change. This PR stops the destruction and
gives 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 recorded
    for an undisplayable surface, for ok: false, and for a rejected request; that
    a displayable resume clears rather than narrates; that pending clears a stale
    notice; 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 cleanup
    does not run (no deleteChatSlot, draft intact) for an undisplayable surface
    or an ok: false answer, and still runs for a displayable one. Removing the
    gate line turns 2 of the 3 red.
  • website/src/test/ChatPage.unresumableNotice.test.tsx (7) -- mounts the real
    page: 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 the
    slice. 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 --noEmit clean; eslint on every touched file reports 0 errors;
node scripts/i18n-check.mjs ok on all 11 sub-checks including catalog parity,
dead keys and the key-reference gate; gen-pseudolocale.mjs re-run so en-XA
tracks the three keys.

5. Any other suggestions on the work

  • Earlier revisions of this description claimed Frontend Lint & Type Check
    was 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 main
    measured exactly 659 at 41011a748^). fix(ci): burn one eslint warning down to the ratchet ceiling #7480 burned that warning back down and
    merged at 05:46Z. Rebased onto 7308a8b30, this branch now measures 652
    warnings
    with npx eslint src/, so the lane passes on its own merit and no
    ratchet exemption is needed. Every touched file still produces the same warning
    count as its base version -- this diff adds zero either way.
  • The notice's placement moved out of the sidebar, which is a change to an
    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.
  • Not gating the notification panel's navigate on result.ok is a deliberate
    divergence 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.
  • handleResumeSession is inline in ChatPage and not exported, so its test
    mounts the same body against the real thunks and reducer rather than importing
    it, following the precedent documented in ChatPage.handleFork.test.tsx. If
    the callback body changes, that test file must change with it. Exporting the
    handler (or lifting the swap into a thunk) would remove the duplication.
  • The UX suggestion on Four sibling resumeFromHistory call sites still resolve blind for non-chat surfaces (follow-up to #3640) #5925 of "offer an exit" is still not implemented, though
    this PR now uses the registry lookup that would power it
    (findSurfaceBySlotMode). Routing the user to the surface that owns the
    session is reachable once /api/sessions carries mode; the fallback for a
    mode that resolves to no advertised surface is a product decision.

Pattern harvest

Rule candidate: eslint

Pattern: await dispatch(<asyncThunk>(...)) inside a try/catch WITHOUT
.unwrap(). A thunk dispatch promise resolves even when the thunk rejected, so
the catch is unreachable dead code and the statements after the await run on a
failure. 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

@chenmingwei23
chenmingwei23 requested a review from a team September 1, 2026 04:03
@chenmingwei23
chenmingwei23 requested a review from a team as a code owner September 1, 2026 04:03
@chenmingwei23
chenmingwei23 requested a review from cixuuz September 1, 2026 04:03
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of 241e9ee615c00ef860af130a6b77d01ce016ee76 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • pages.chatPage.could_not_open_this_session — "Try again." instructs an action with no adjacent affordance for palette/notification arrivals (their retry control is no longer on screen after navigate('/chat')), and it's wrong advice for a permanently gone session. Either add a Retry action to the ErrorNotice or end the sentence at "Couldn't open "{{title}}"."

[UX-REVIEWED] 241e9ee

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 241e9ee615c00ef860af130a6b77d01ce016ee76 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified. The two palette providers both navigate('/chat') after dispatching, so the shared render site genuinely covers them; historyOpen is component state defaulting closed; dashboard_source, isChatPageSurface, slotChannelLabel, findSurfaceBySlotMode, and surfaceLabel all exist; temp-screenshots/<feature>/ is the PR template's sanctioned deliverable path. Counts I ran: 5 resumeFromHistory dispatch sites, all now covered by the slice cases with zero left blind; unresumableResume has exactly one real consumer (ChatPage) as the deliberate convergence point; the deeper server-side cause is named and declared out of scope in the description.

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 ships

Intent: 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.

  1. Resuming an unusable session no longer deletes the current tab and its half-typed draft — justified (the reported defect)
  2. Failed resumes (404/5xx, ok:false) now show a message instead of a dead click — justified, declared
  3. All five resume entry points share one outcome recorded on the store — justified; 5 dispatch sites counted, 0 left blind, per-caller check deleted
  4. Notice moved from the sidebar's history pane to the chat pane top — justified move: the pane defaults closed, so 3 of 4 entry points could never see it
  5. Notification Resume navigates to /chat whatever the outcome — declared; that page is where the explanation renders
  6. Only the latest of racing resume clicks narrates, across surfaces — justified (was per-component only)
  7. Copy reworded, sidebar reference dropped, 2 new sentences in 12 catalogs — mandated i18n invariant
  8. Surface named by localized label, never the raw wire value — justified ("a Session session" mislabel)
  9. Screenshot committed under temp-screenshots/ — sanctioned by the PR template

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 241e9ee615c00ef860af130a6b77d01ce016ee76 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 241e9ee615c00ef860af130a6b77d01ce016ee76 — this comment is updated in place on each push.

Review details

No blocking issues; one advisory finding.

FINDING — website/src/store/chatSlice.ts:5303 — the resumeFromHistory.fulfilled success branch (if (action.payload.ok) {) is the one branch not guarded by state.lastResumeRequestId, so when a displayable resume A (clicked first, slow) resolves after an undisplayable resume B (clicked second, fast) records unresumableResume for B, A's success path switches activeSlot to A but never clears the notice — leaving a stale "B can't be opened" banner above chat A → Fix: in the success branch, clear state.unresumableResume (and/or return early when action.meta.requestId !== state.lastResumeRequestId).

[OPUS-REVIEWED] 241e9ee

Verdict parsed from the review's SHA-scoped output markers for commit 241e9ee615c00ef860af130a6b77d01ce016ee76.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 241e9ee615c00ef860af130a6b77d01ce016ee76: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 241e9ee615c00ef860af130a6b77d01ce016ee76 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- website/src/pages/ChatPage.tsx:952 -- "r.key.startsWith('dashboard')" mislabels persisted Crew Member sessions as Dashboard sessions -> Fix: prefer the registered surface label before the dashboard-key fallback.
[GPT-REVIEWED] 241e9ee

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 241e9ee615c00ef860af130a6b77d01ce016ee76: <one-sentence reason>

@chenmingwei23
chenmingwei23 force-pushed the fix/resume-history-blind-resolve branch from 92c3705 to 9714eec Compare September 1, 2026 04:41
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 1, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/resume-history-blind-resolve branch from 9714eec to 8b0de85 Compare September 1, 2026 05:02
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

UX Review disposition -- both items FIXED at 8b0de85c6

Watch: "an ok: false resume shows nothing anywhere, and the notification button still navigates on it" -- fixed, and the finding was sharper than it looks.

Correct on both halves, and it caught an overclaim in my own PR body: I wrote that adding .unwrap() stops the notification panel navigating on a failed resume. That is only true for a rejected thunk. ok: false is a fulfilled payload, so unwrap() resolves and the navigation went ahead -- exactly as you describe. The body is corrected.

Tracing it further: api.resumeChatSlot throws on any non-2xx (j() in api/client.ts), so the common failure -- 404 / 409 / 5xx / dropped connection -- lands on resumeFromHistory.rejected, which recorded nothing either. Every caller swallowed it: ChatPage's catch {}, the palette providers' void dispatch, the notification panel's console log. So the silent class was bigger than ok: false.

Fix, taking your "smallest fix" shape:

  • unresumableResume gains a reason: 'surface' | 'failed' discriminator, recorded in three places under the same requestId ordering guard: fulfilled + undisplayable surface, fulfilled + !ok, and rejected.
  • reason: 'failed' gets its own sentence and names no surface -- nothing was resumed, so claiming one would be a guess: Couldn't open "X" -- the resume failed. Try again.
  • The notification navigate is not gated on result.ok, which is the one place I diverged from your suggestion. /chat is where the shared notice renders, so navigating is how the user learns what happened; gating it would restore the dead click on the failure path and need a second render site to compensate. .unwrap() stays, for the diagnostic. If you would rather the button stay put and narrate locally, say so and I will add the second site instead.

Suggestion: raw wire surface interpolated into localized copy -- fixed.

Right, and its empty case was worse: it fell through to the session_source label and rendered "it's a Session session", the exact mislabel #3640's own comment warned about. The label is now resolved rather than interpolated, in order: localized dashboard label for a dashboard* key, slotChannelLabel for a channel key, surfaceLabel(findSurfaceBySlotMode(surface)) for a registered surface (this is the mapping you asked for -- it covers member -> "Crew Members"), and when none resolve, a sentence that names no surface at all: "X" can't be opened in chat -- it belongs to another surface. No machine vocabulary can reach the copy on any path.

All three sentences, rendered:

The three unresumable-resume sentences

Coverage added: 2 store cases (ok: false and rejected each record reason: 'failed'), mutation-verified red with the two new branches removed; and 3 mounted-page cases (failure sentence names no surface, an unregistered surface gets the surface-free sentence and never the word subagent, a registered surface renders "Crew Members").

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/resume-history-blind-resolve branch from 8b0de85 to 1cdec4d Compare September 1, 2026 05:17
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

GPT disposition -- BLOCKING finding is CORRECT and FIXED at 1cdec4d5d

Older-session resume with no active slot -> reducer records failure -> !activeSlot branch bypasses this notice -> click remains silent.

Reproduced and fixed. The notice was nested inside the third arm of
... ? <split> : !activeSlot ? <EmptyState> : <transcript + composer>, so with no
tab open ChatPage renders only the empty state and the notice was not in the tree.
That is not a corner: both command-palette providers and the notification panel
call navigate('/chat') unconditionally, so "no active slot" is precisely the
state they can arrive in -- the finding lands on the two entry points that have no
component of their own and therefore depend entirely on this render site.

It also makes the "one always-visible site" claim in my own PR body false as
written, and that is corrected there too.

Fix, as you prescribed: the notice moved above the ternary, into the
pane-level banner group beside uploadError / sidError / pinStatus -- an
existing convention for exactly this (banners that must show in every view state)
rather than a new position I invented. It now uses their mx-4 mt-2 mb-0
container instead of the composer's centred content-width recipe.

Regression test, mutation-verified: a new case in
website/src/test/ChatPage.unresumableNotice.test.tsx mounts the real page with
activeSlot: null, asserts the notice renders, and asserts the empty state
("What can I do for you?") is present so it cannot silently be re-testing the
transcript branch. Putting the notice back inside the transcript branch turns that
case red with Unable to find an element by: [data-testid="unresumable-resume-error"];
hoisted, all 7 cases in the file pass.

tsc --noEmit clean, eslint 0 errors on touched files, 279 tests across the six
affected suites pass. The screenshot in the body is re-captured at the new
container recipe and re-pinned to 1cdec4d5d.

Note on the remaining red Frontend Lint & Type Check: that is main's own content,
not this diff -- current main and this branch both produce 660 warnings against a
ratchet of 659, and main was exactly 659 before #7319 added a test file carrying an
unused ReactNode import. Details in section 5 of the description.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/resume-history-blind-resolve branch from 1cdec4d to ec5e318 Compare September 1, 2026 05:28
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

UX Review (PASS) suggestion dispositions -- 2 of 3 FIXED at ec5e318d5, 1 deferred with a decision for the maintainer

1. "surface" is jargon in the fallback string -- FIXED. Right, and the key name carried the same jargon, so both moved together:
this_session_belongs_to_another_surface -> this_session_is_not_a_chat_session, reading "X" can't be opened in chat -- it isn't a chat session. Renamed rather than reworded-in-place so the key and the copy cannot drift. All 12 catalogs plus the regenerated pseudolocale. The mounted-page test now asserts the rendered text does not contain the word surface, so the jargon cannot come back unnoticed.

2. "-- the resume failed" restates "Couldn't open" -- FIXED. Tightened to exactly your wording: Couldn't open "X". Try again. The test asserts the restatement is absent.

Both strings re-captured; the screenshot in the description is re-pinned to ec5e318d5.

3. Notification panel: retry lives in the panel the user is navigated away from -- ACCEPTED, deferred, and I would like your call on landing it here.

The observation is correct and it is the strongest of the three: for reason: 'failed' the useful next action is retry, the retry control is the panel's own "Resume chat" button, and navigating to a banner that says "Try again" puts the fix one navigation back. Your refinement is also better than either of the two positions we have each argued: navigate for the surface case, where the destination genuinely is the explanation, and stay put for failed, where the repair is local.

Why I have not folded it in unprompted:

  • It adds a second render site for this notice, which is the exact property the architecture was reduced to avoid, and which GPT's blocking finding on the previous head just validated in the other direction (one site, hoisted above every view branch).
  • It needs the two sites not to double-narrate: the slice has already recorded reason: 'failed' by then, so the local path must clearUnresumableResume() or the same message waits on /chat for the user's next visit. That local-versus-shared clearing interaction is new state coupling, arriving on a PR that took three review rounds to converge and whose lanes are now all clean.
  • It is a genuine product improvement rather than a defect in what ships here: the failed resume is explained today, just not in the most repairable place.

So: happy to implement it in this PR if you want it now (roughly 15 lines in NotificationDetailPanel plus the clear-dispatch and one test), or to file it as a follow-up against this issue. Maintainer's call -- I did not want to grow the diff after convergence without one.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

UX Review (PASS) round-5 suggestion dispositions -- both DEFERRED, with the blocking constraint named for each. No push: the head is unchanged at ec5e318d5.

1. "names where the session lives but offers no path there" -- deferred, and the example given is the one case that cannot be linked.

This is the "offer an exit" ask from the issue itself, which section 3 of the description already declares out of scope. Worth adding the specific reason it is not a small addition, because it is not the obvious one:

findSurfaceBySlotMode() returns a descriptor with a route, so a link looks reachable. But only two builtins declare a slotMode at all -- '' (Sessions) and 'member' (Crew Members, surfaces/builtins.tsx). So:

  • "Open in Dashboard", the case in the suggestion, has no route to offer. No registered surface declares slotMode: 'dashboard' -- which is exactly why that label comes from the dashboard* key-prefix heuristic rather than the registry.
  • The one case that is routable, member, sits behind previewFlag: PREVIEW_CREW, and its own comment says the page "errors out on paths that are still being built". Offering a link into it would send the user somewhere that breaks unless they have opted in at Developer > Feature Previews.

So the honest version of this feature needs a fallback rule for a mode that resolves to no advertised surface -- a product decision, not a mechanical addition, and the same one flagged when this issue was first triaged. Deferred rather than half-built.

2. "Try again." on a permanent 404 -- deferred; the status is not readable where the copy is chosen.

The finding is right about the experience. The premise that the status is available is not, and it is worth being precise since it changes the size of the fix:

api.resumeChatSlot throws ApiError, which does carry status (api/apiError.ts:25). But resumeFromHistory is a plain createAsyncThunk with no rejectWithValue, so RTK serializes the thrown error with miniSerializeError -- and that keeps name / message / stack / code only. action.error.status is undefined in the reducer, so resumeFromHistory.rejected genuinely cannot branch on 404-versus-503 today.

Making it readable means giving the thunk a rejectWithValue contract, which changes what .unwrap() throws for every caller -- ChatPage's catch, the notification panel's logError, and the sidebar. That is a thunk-signature change in service of one copy nuance, on a diff that has already taken four review rounds to converge, so I am not folding it in unprompted. Reasonable as a follow-up, and it would pair naturally with (1) since both want richer outcome data on the same field.

Both recorded for the maintainer. Neither is a defect in what ships: a failed resume is explained, and an undisplayable one names where the session lives -- what is missing is the next click, in both cases.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
… 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.
@chenmingwei23
chenmingwei23 force-pushed the fix/resume-history-blind-resolve branch from ec5e318 to 241e9ee Compare September 1, 2026 06:04
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

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.

test/test_irq.py::test_an_entry_joining_after_a_partial_fire_serves_its_own_floor failed with AssertionError: assert False on this line:

# 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:

  • The margin is real wall clock, and it is 10 ms: _COALESCE = 0.01, and the helper's own docstring says _settle() "advance[s] wall clock past _COALESCE". The assertion needs comment:2's floor to still be OPEN, so LESS than 10 ms may elapse between the previous _verdict call and this one. On a loaded runner sharing cores with xdist workers, a >10 ms gap between two consecutive Python calls is ordinary -- the floor closes on its own and behind returns Report instead of Skip. There is no injected clock to hold it.
  • This diff contains zero Python. All seven files are under website/.
  • It passes locally on this exact branch: .venv/bin/python -m pytest test/test_irq.py -x -q -n 4 -k joining_after_a_partial_fire -> 1 passed.
  • The test is new: it arrived with fix(irq): give a coalescing entry its own floor instead of the window's age #7431 (dd9e002bf, "fix(irq): give a coalescing entry its own floor instead of the window's age"), which merged shortly before this run.

Coverage Gate carries no independent failure -- its log reads backend-test=failure -- failing closed, and PR Readiness aggregates both. So the one flaky shard is the single root cause of all three reds.

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.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

UX Review (PASS) round-6 disposition -- suggestion ACCEPTED on the merits, held off this head deliberately. No push.

pages.chatPage.could_not_open_this_session -- "Try again." instructs an action with no adjacent affordance [...] and it's wrong advice for a permanently gone session. Either add a Retry action to the ErrorNotice or end the sentence at Couldn't open "{{title}}".

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 navigate('/chat'), and for a deleted session retrying fails identically every time -- so the sentence promises an affordance in one case and a different outcome in the other. Ending it at Couldn't open "{{title}}". removes a false instruction rather than adding a new control, which is the same shape as the two copy fixes taken from your round-4 pass.

I am NOT pushing it onto this head, for a reason specific to where the PR now sits rather than any disagreement:

  • The head is otherwise fully converged -- 61 checks green, 0 failures, all five lanes clean and marker-pinned to 241e9ee61, every advisory across six rounds dispositioned. The only thing outstanding is one backend shard finishing.
  • A push to drop one clause re-rolls all five non-deterministic lanes and restarts the full CI run, including the test_irq.py timing flake documented above. That is a real chance of a fresh spurious block bought for a copy subtraction.

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 Retry action on ErrorNotice alternative stays deferred with the round-4 item it belongs to: it needs the notice to carry a re-dispatch of the original resume, which is a new capability on a shared primitive rather than a copy change.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 1, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.

@bolichen97
bolichen97 enabled auto-merge (squash) September 1, 2026 21:26
@bolichen97
bolichen97 merged commit a977c1f into main Sep 1, 2026
110 of 119 checks passed
@bolichen97
bolichen97 deleted the fix/resume-history-blind-resolve branch September 1, 2026 21:28
@bolichen97

Copy link
Copy Markdown
Collaborator

One unanswered finding from review worth fixing before/after merge:

GPT finding on ChatPage.tsx:952 — the new label resolver's r.key.startsWith('dashboard') branch wins for any persisted dashboard-origin session, including Crew Member threads. On the server, a real Crew Member session's history key is dashboard:member-<slug> (see chat_persistence.py:407,860 + members.py:76 DM_SLOT_KEY_PREFIX), so the startsWith('dashboard') check fires before the member-specific branch and the session gets labeled "Dashboard" instead of "Crew Members" — contradicting this PR's own §3.6 claim that it covers member -> Crew Members.

The new test (key: 'member-ada') doesn't catch this because /api/sessions never produces a bare member-ada key for a dashboard-origin row — the real shape is dashboard:member-ada, which the test never exercises.

Suggested fix: move the member-registry lookup ahead of the dashboard prefix fallback, so a dashboard:member-* key resolves to "Crew Members" before the generic dashboard branch catches it.

Not blocking (cosmetic mislabel, no data loss), but worth closing out since it contradicts the PR description's own claim.

@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Four sibling resumeFromHistory call sites still resolve blind for non-chat surfaces (follow-up to #3640)

2 participants