Skip to content

fix(frontend): sync completed chat attempts#236

Merged
warren618 merged 1 commit into
HKUDS:mainfrom
sambazhu:main
Jun 15, 2026
Merged

fix(frontend): sync completed chat attempts#236
warren618 merged 1 commit into
HKUDS:mainfrom
sambazhu:main

Conversation

@sambazhu

Copy link
Copy Markdown
Contributor

Summary

Why

Changes

Test Plan

  • Existing tests pass (pytest --ignore=agent/tests/e2e_backtest --tb=short -q)
  • New tests added (if applicable)
  • Tested manually (describe below)

Checklist

  • No changes to protected areas (src/agent/, src/session/, src/providers/) without prior discussion
  • No hardcoded values (API keys, file paths, magic numbers)
  • Code follows CONTRIBUTING.md guidelines
  • Documentation updated (if user-facing change)

@warren618 warren618 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.

Thanks @sambazhu — this is a useful, orthogonal addition. For context, two fixes for the same issue (#229) just merged: #234 (re-subscribe + replay SSE when returning to a running session) and #235 (responsive cancel). This PR targets a third, distinct gap — the UI staying in "streaming" after a run already committed because the SSE attempt.completed was missed (the long-standing replay race) — so it composes rather than overlaps. Verified the contract holds: sendMessage returns attempt_id (service emits attempt.created then runs in the background) and the assistant reply is stored with linked_attempt_id, so the poll key is correct.

A few things before merge:

  1. Make it a true fallback (avoid firing on the happy path). syncCompletedAttempt runs after every send and polls getSessionMessages 3× even when SSE finalized the turn normally; on a sub-2.4s run it then re-forces idle + a second getSessionMessages refresh. Could you gate the reconcile on the UI still being mid-stream — e.g. only clearStreaming()/setStatus("idle")/refresh when act().status === "streaming" (and skip the poll loop entirely once it's already idle)? That keeps it a no-op whenever SSE worked.

  2. Window scope. The 3×800ms (~2.4s) window only catches attempts that finish within it; a longer run whose completion event is genuinely missed stays stuck. Is the 2.4s the scenario you actually hit (a fast turn)? If so, a one-line note in the PR body is enough; if you meant to cover longer runs too, the reconnect/replay path from #234 is probably the better hook than a fixed poll budget.

  3. Note attempt.completed vs failed/cancelled. A failed or cancelled attempt also appends an assistant reply with linked_attempt_id, so this will reconcile those too — which is fine (and desirable), just confirming it's intended.

  4. Please fill in the PR description (what scenario/repro this fixes) and add a frontend test (vitest) for the sync path — e.g. a completed attempt present in getSessionMessages while the store is still streaming → status flips to idle and history refreshes. That locks the behavior in.

Happy to merge once the fallback guard + a test are in. 🙏

@warren618 warren618 merged commit 83c8b44 into HKUDS:main Jun 15, 2026
1 check passed
@warren618

Copy link
Copy Markdown
Collaborator

Merged, thanks! This is the SSE-completion-drop sync (poll for the persisted assistant message after sendMessage, then reconcile to idle). Heads up: the raw 2-way diff looked like it deleted the #234 reconnect block, but that's only because the branch forked from an older main — the 3-way merge preserved #234. Verified tsc + vite build + full vitest suite green on the merged tree. A one-line PR description would help reviewers next time. 🙏

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.

2 participants