Skip to content

fix(desktop): self-retry transient boundary errors, reactive edit composer context - #72867

Closed
Adolanium wants to merge 1 commit into
NousResearch:mainfrom
Adolanium:fix/desktop-thread-perf-correctness
Closed

fix(desktop): self-retry transient boundary errors, reactive edit composer context#72867
Adolanium wants to merge 1 commit into
NousResearch:mainfrom
Adolanium:fix/desktop-thread-perf-correctness

Conversation

@Adolanium

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes two correctness holes left by the session-switch perf work (#72504 / #72524), without giving back any of the perf wins.

1. Transient boundary errors can blank a turn mid-stream. MessageRenderBoundary swallows the transient useClientLookup ... out of bounds race and renders null until resetKey changes. #72504 narrowed resetKey to pure structure (right call, it stopped per-token reconciles of every turn), but that removed the implicit recovery: mid-turn the structure is stable, so a race during a stream left the turn blank until an unrelated message add/remove. The boundary now self-retries on a 0ms timer (rAF never fires in a parked renderer), bounded to 5 consecutive transient catches with the budget reset on successful recovery. A persistent failure falls back to the old wait-for-structure behavior, and non-transient errors still re-throw to the root boundary.

2. An open edit composer keeps a stale cwd / gateway / sessionId. #72524 moved the three values out of the messageComponents memo deps into a render-time ref so session switches stop reminting the component types. That keeps them fresh for a composer that mounts later, but a composer that is already open never re-reads the ref: a same-session change (the agent relocating the session's cwd via session.info, a gateway reconnect) leaves every ThreadMessageList prop referentially equal, so the memo'd list bails out. @-completions, slash completions, and OS-drop uploads then act on the old context. Thread now provides the three values through a memoized ThreadEditContext. Context propagates through the bail-out, the component type identity is untouched, and the transcript never remounts.

Related Issue

Fixes #72866

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Security fix
  • Documentation update
  • Tests (adding or improving test coverage)
  • Refactor (no behavior change)
  • New skill (bundled or hub)

Changes Made

  • apps/desktop/src/components/assistant-ui/message-render-boundary.tsx: bounded timer self-retry for transient lookup errors, budget reset on recovery, timer cleared on unmount and on structural reset.
  • apps/desktop/src/components/assistant-ui/message-render-boundary.test.tsx: 4 new tests (timer recovery without a resetKey change, retry cap, budget reset across streaks, no retry for non-transient errors). Existing resetKey and re-throw tests untouched.
  • apps/desktop/src/components/assistant-ui/thread/index.tsx: editContextRef replaced with a memoized ThreadEditContext provided around the list; the UserEditComposer wrapper reads it via useContext. Comments updated to explain why neither memo deps nor a ref can carry these values.
  • apps/desktop/src/components/assistant-ui/thread/edit-context.test.tsx: new. Same-session cwd change reaches the mounted composer (fails on main), session-switch control, and a perf invariant guard proving a cwd rerender does not remount any transcript DOM node.

How to Test

  1. npx vitest run --project ui src/components/assistant-ui/message-render-boundary.test.tsx src/components/assistant-ui/thread/edit-context.test.tsx
    • 2 files, 11 tests passed
  2. Proof of the bugs on main: with the two source files reverted, "recovers on the retry timer without a resetKey change", "resets the retry budget after a successful recovery", and "passes a same-session cwd change to the mounted edit composer" fail. With this branch, all pass.
  3. npx vitest run --project ui src/components/assistant-ui/
    • 27 files, 237 tests passed
  4. npm run typecheck clean. npx eslint on the touched files clean.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I 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 the vitest suites above and they pass. Full desktop e2e not run here. Change is isolated to the boundary retry + edit context delivery, plus tests.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) - or N/A (code comments on both fixed paths)
  • 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) per the compatibility guide - React class boundary + context only, no platform API. Reproduced and tested on Windows.
  • I've updated tool descriptions/schemas if I changed tool behavior - or N/A

Screenshots / Logs

# On main (both source files reverted): the repro tests fail
 Tests  3 failed | 8 passed (11)

# With this branch
 Test Files  2 passed (2)
      Tests  11 passed (11)

# Full assistant-ui directory
 Test Files  27 passed (27)
      Tests  237 passed (237)

…poser context

Two correctness holes left by the session-switch perf work (NousResearch#72504 / NousResearch#72524):

1. MessageRenderBoundary only cleared a swallowed transient useClientLookup
   error when the structural resetKey changed. Mid-turn, ids/roles/count are
   stable, so a lookup race during a stream left the boundary rendering null
   for the rest of the turn. The boundary now self-retries on a 0ms timer
   (rAF never fires in a parked renderer), bounded to 5 consecutive
   transient catches with the budget reset on recovery; the structural
   resetKey path is unchanged, and non-transient errors still re-throw.

2. cwd / gateway / sessionId were removed from the messageComponents memo
   deps and read through a render-time ref so session switches stop
   reminting the component types. But a mounted UserEditComposer only
   reads that ref when it renders, and a same-session change (cwd remap,
   gateway reconnect) leaves every ThreadMessageList prop referentially
   equal, so the memo'd list bails out and the open composer keeps stale
   values: @-completions, slash completions, and OS-drop uploads target
   the old cwd / gateway / session. Thread now provides the three values
   through a memoized ThreadEditContext; context propagates through the
   bail-out, the component type identity is untouched, and the transcript
   never remounts.
@alt-glitch alt-glitch added type/bug Something isn't working 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 labels Jul 27, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. Current main still has both premises: MessageRenderBoundary only clears an error after resetKey changes (apps/desktop/src/components/assistant-ui/message-render-boundary.tsx:34-37), while that key is structural-only (apps/desktop/src/components/assistant-ui/thread/list.tsx:152-162); and the open edit composer reads context from editContextRef inside a component map that intentionally excludes cwd, gateway, and sessionId from its memo dependencies (apps/desktop/src/components/assistant-ui/thread/index.tsx:101-132). ThreadMessageList is memoized (apps/desktop/src/components/assistant-ui/thread/list.tsx:533), so the same-session stale-prop path is real.

The proposed bounded retry preserves non-transient rethrows and the context provider preserves the stable component-map identity. The regression tests target those two behaviors, including the no-transcript-remount invariant. The PR base is an ancestor of current main; the only later neighboring change is 8c92983fde in thread/list.tsx, which this PR does not edit, so this appears mechanically salvageable.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

One PR addresses #72866. #72867 covers both reported root causes by adding bounded self-retries for transient lookup failures and reactive edit-context propagation for an already-open composer, while preserving stable transcript component identity.

Related pull requests

Suggested consolidation

Keep #72867 open with a salvage path: retain both the bounded transient-error retry and the reactive ThreadEditContext change together with their regression tests. It is the only PR in this complex, directly addresses both independent causes in #72866, and has no competing PRs to close as duplicates.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I72866(["issue #72866 (open)"])
    P72867["PR #72867 (open)"]
    P72867 -->|best fix| I72866
    class I72866 open
    class P72867 open
    class P72867 best
    class P72867 target
    click I72866 "https://github.com/NousResearch/hermes-agent/issues/72866"
    click P72867 "https://github.com/NousResearch/hermes-agent/pull/72867"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 22 kB of PR diffs, 11 kB of issue/PR text, 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@OutThisLife

Copy link
Copy Markdown
Collaborator

Superseded by #81232.

Consolidated with #64310 into one PR: both fix the same useClientLookup ... out of bounds store race, at complementary layers (your message-local self-retry plus the root-boundary recovery), and the two branches conflict on message-render-boundary.test.tsx. Your commit was cherry-picked, so authorship is preserved in git history, and you're credited in the new PR body.

Both of your fixes carried over unchanged — the bounded self-retry and the reactive ThreadEditContext. Your repro tests are what made the consolidation verifiable: 25/25 focused, and 36 files / 304 tests green across assistant-ui/. Thanks.

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

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

[Bug]: desktop thread: transient boundary error blanks a turn mid-stream, open edit composer keeps stale cwd/gateway/sessionId

5 participants