Skip to content

fix(desktop): key response messages by id, not array index (#122721) - #123077

Open
Halldrix wants to merge 2 commits into
NousResearch:mainfrom
Halldrix:fix/desktop-session-message-bleed
Open

Halldrix wants to merge 2 commits into
NousResearch:mainfrom
Halldrix:fix/desktop-session-message-bleed

Conversation

@Halldrix

Copy link
Copy Markdown
Contributor

What does this PR do?

ResponseMessages computed a stable message id for every row it renders, then threw it away and keyed each child by its array index:

// before
{section.indices.map(index => (
  <ThreadPrimitive.MessageByIndex components={components} index={index} key={index} />
))}

When the transcript is replaced under a mounted turn — a REST re-hydration graft, a background transcript refresh, a session switch — React reuses the fiber at the same ordinal. Two upstream pieces make that reuse observable: MessageByIndex memoizes on index alone, and PartByIndexProvider keeps a per-index lastPartRef it hands back once the new message has fewer parts. The response rows then carry another message's mounted state.

This is reachable in production because TurnRow is keyed on the user message id (list.tsx:1459), so replacing a later assistant row does not remount the turn — and because toChatMessages synthesizes ids positionally (hydration.ts:491, ${timestamp}-${index}-${role}), a re-hydrated tail row routinely comes back with a different id at the same index. The writers are the graft at transcript-backfill.ts:119-137, the background refresh at use-background-sync.ts:282-298 and :306-372, and the compaction re-hydrate at gateway-event/status.ts:104.

The fix carries the id already computed alongside the index and keys on it, so a replaced row remounts. list.tsx, buildGroups, and hydration.ts are untouched: TurnRow and the standalone branch were already id-keyed, and re-synthesizing ids is a larger semantic change (ids address reactions, branch, rowId) that this does not require.

Related Issue

Refs #122721
Refs #121096

Not Fixes on either: see Exclusions. This closes the render-level keying defect, not the whole reported symptom.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • apps/desktop/src/components/assistant-ui/thread/response-group.tsx — ResponseSection.indices: number[] → rows: ResponseRow[]; the child key is now row.id instead of row.index. Six lines of production code.
  • apps/desktop/src/components/assistant-ui/thread/response-group.test.tsx — one regression case: a response row whose message is replaced at the same index must remount (old content absent, data-message-id updated, DOM node not reused).

How to Test

  1. cd apps/desktop && npx vitest run --project ui src/components/assistant-ui/thread/response-group.test.tsx — 3/3 pass with the fix.
  2. With only the source reverted (git checkout HEAD~1 -- apps/desktop/src/components/assistant-ui/thread/response-group.tsx), the new case fails: AssertionError: expected <div …> not to be <div …> at response-group.test.tsx:148. Verified on a bare-main worktree at 59004a6 and in place.
  3. npm run typecheck, eslint and prettier --check on both touched files are clean; the full test:ui suite is unchanged by this diff.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I've searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — N/A, TypeScript-only under apps/desktop. The desktop suite is the relevant one.
  • I've added tests for my changes (required for bug fixes)
  • I've tested on my platform: WSL2 (Ubuntu), Node v24.21.0, vitest 4.1.10

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) — or N/A. Pure renderer keying; no platform-specific paths.
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Exclusions

What this does NOT fix. #122721 reports a broad symptom under one title, and I traced at least three distinct mechanisms behind it. Only one is unclaimed, and this PR covers only that one:

Mechanism Status Where
message.reaction mutating the foreground transcript regardless of session Already covered Open PR #118749 (targets #118748)
A second unscoped message.start clobbering the shared stream pin, so A's events resolve to B Already covered Open PR #77826 (targets #46194 / #62823)
Response rows keyed by array index inheriting a replaced message's mounted state This PR —

The reporter's own hypothesis list ("background events may be applied to the active transcript") is not supported by the code: stream/tool/reasoning events are routed by explicit session_id, syncSessionStateToView refuses non-active sessions, and the TUI emitter stamps session_id on every turn frame. I did not add a broad session-state guard because there is nothing to guard.

The stale-text claim is not proven. My regression test asserts the row remounts. It does not assert that the previous message's parts were visible, and I could not reproduce that: MessagePrimitiveParts derives its ranges from the current message's parts, so on unmodified main the not.toContain('old content') assertions pass even though the node is reused. I probed three shapes (3-parts→1, tool→text, full transcript swap) and none leaked text. The node-identity assertion is the real, honest regression. If maintainers can show a shape where stale text actually renders, that is a separate, stronger test — not this PR.

Why not touch hydration.ts id synthesis. Ids are synthesized positionally, which is what makes them unstable across a re-hydrate. But ids address reactions, branch identity, and rowId; changing the synthesis is a much larger semantic change than this defect needs, and this fix makes React re-key correctly regardless. Revisit if a duplicate-id collision is ever demonstrated.

Why not touch list.tsx / buildGroups. TurnRow already keys on group.id (the first message id) and the standalone branch keys on the message id. The response path was the only index-keyed one; this makes it consistent. A latent pre-existing fragility I noticed but did not touch: structuralSignature joins ${index}:${id}:${role} and buildGroups splits on :, so a message id containing a colon would corrupt the parse. No current writer emits one.

…rch#122721)

ResponseMessages computed a stable id for every row in its signature and
then threw it away, keying each child MessageByIndex by its array index.
When the transcript is replaced (session switch, history backfill, LCM
compaction) React reuses the mounted fiber at the same ordinal, and
assistant-ui's PartByIndexProvider keeps a per-index lastPartRef that hands
back the PREVIOUS message's part once the new message has fewer parts. The
result is one conversation's blocks appearing in another's transcript with
the order scrambled -- the reported cross-session bleed and the duplicated
blocks of NousResearch#121096.

Carry the computed id alongside the index and key on it, so a replaced
message remounts instead of inheriting the old one's part cache.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Sep 25, 2026
@Halldrix
Halldrix marked this pull request as ready for review September 25, 2026 20:58

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants