Skip to content

fix: guard finalizeThinkingCard with session ID check#1366

Closed
JKJameson wants to merge 1 commit into
nesquena:masterfrom
JKJameson:fix/session-streaming-tab-switch
Closed

fix: guard finalizeThinkingCard with session ID check#1366
JKJameson wants to merge 1 commit into
nesquena:masterfrom
JKJameson:fix/session-streaming-tab-switch

Conversation

@JKJameson
Copy link
Copy Markdown
Contributor

Without this check, switching browser tabs while a stream is running causes finalizeThinkingCard() to operate on the wrong session's thinking card DOM, the card belongs to the stream that started it, not the session currently displayed in the tab. The guard ensures finalize only runs when the live assistant turn's session matches the current session.

Without this check, switching browser tabs while a stream is running causes
finalizeThinkingCard() to operate on the wrong session's thinking card DOM —
the card belongs to the stream that started it, not the session currently
displayed in the tab. The guard ensures finalize only runs when the live
assistant turn's session matches the current session.
@nesquena-hermes
Copy link
Copy Markdown
Collaborator

Shipped in v0.50.250 via batch release PR #1368 (merge ffd11037). Thanks @JKJameson! 🙏

Live at https://github.com/nesquena/hermes-webui/releases/tag/v0.50.250.

What landed

Your guard at finalizeThinkingCard() ships, paired with three pre-release fixes the release agent and Opus identified during review:

  1. Per-site dataset.sessionId stamps. Your guard reads _guardTurn.dataset.sessionId and compares against S.session.session_id, but no code in the repo set that attribute. Without the stamps, the guard's comparison was always undefined !== "<sid>" (always true), and finalizeThinkingCard() always early-returned — every assistant turn's thinking card would have stayed open forever. The release agent added if(S.session) <var>.dataset.sessionId=S.session.session_id; at all 3 sites in static/ui.js that create liveAssistantTurn (lines ~3179, 3551, 4339).

  2. Regression test (tests/test_pr1366_finalize_thinking_card_guard.py). Walks static/ui.js with regex, finds every <var>.id='liveAssistantTurn' assignment, and asserts a matching <var>.dataset.sessionId= stamp follows within ~500 chars. If a future PR adds a 4th creation site without the stamp, this test fails with a clear error message. Locks the invariant in.

  3. Behavioral verification. Live runtime check via the new SSE liveness section in webui_qa_agent.sh confirmed the streaming UI works correctly post-deploy.

Known limitations (tracked as follow-ups)

Opus flagged that the guard catches the stale-A-turn-in-DOM case (your scenario) but does NOT fully prevent A's late callback from finalizing B's thinking card when both A and B have live streams concurrently. A complete fix would have finalizeThinkingCard(streamSessionId) accept the stream's session_id as a parameter. Out of scope for this release; what shipped is a strict improvement over master.

Lessons captured in ~/WebUI/docs/agent-memory/sse-handler-pre-release.md (now: any guard that depends on a DOM/state attribute must be paired with a regression test that pins the attribute setters).

JKJameson pushed a commit to JKJameson/hermes-webui that referenced this pull request Apr 30, 2026
Without this check, switching browser tabs while a stream is running
causes finalizeThinkingCard() to operate on the wrong session's
thinking card DOM — the card belongs to the stream that started it,
not the session currently displayed in the tab. The guard ensures
finalize only runs when the live assistant turn's session matches
the current session.

Co-authored-by: Josh <josh@fyul.link>
JKJameson pushed a commit to JKJameson/hermes-webui that referenced this pull request Apr 30, 2026
Bundles 2 PRs:
- nesquena#1366 fix: guard finalizeThinkingCard with session ID check (with pre-release fix)
- nesquena#1367 fix(clarify-sse): stale-detector health timer (Opus SHOULD-FIX from v0.50.249)

Pre-release fix on nesquena#1366: the contributor's guard depends on
liveAssistantTurn.dataset.sessionId, but no code in the repo sets
that attribute. Without the fix, the guard would always early-return
(undefined !== sid is always true), breaking the streaming UI
completely — every assistant turn's thinking card would stay open
forever. Added per-site stamps at all 3 places that create
liveAssistantTurn in static/ui.js, plus a regression test that fails
any future creation site that forgets the stamp.
GeoffBao pushed a commit to GeoffBao/hermes-webui that referenced this pull request May 1, 2026
Without this check, switching browser tabs while a stream is running
causes finalizeThinkingCard() to operate on the wrong session's
thinking card DOM — the card belongs to the stream that started it,
not the session currently displayed in the tab. The guard ensures
finalize only runs when the live assistant turn's session matches
the current session.

Co-authored-by: Josh <josh@fyul.link>
GeoffBao pushed a commit to GeoffBao/hermes-webui that referenced this pull request May 1, 2026
Bundles 2 PRs:
- nesquena#1366 fix: guard finalizeThinkingCard with session ID check (with pre-release fix)
- nesquena#1367 fix(clarify-sse): stale-detector health timer (Opus SHOULD-FIX from v0.50.249)

Pre-release fix on nesquena#1366: the contributor's guard depends on
liveAssistantTurn.dataset.sessionId, but no code in the repo sets
that attribute. Without the fix, the guard would always early-return
(undefined !== sid is always true), breaking the streaming UI
completely — every assistant turn's thinking card would stay open
forever. Added per-site stamps at all 3 places that create
liveAssistantTurn in static/ui.js, plus a regression test that fails
any future creation site that forgets the stamp.
JKJameson pushed a commit to JKJameson/hermes-webui that referenced this pull request May 1, 2026
Resolve conflicts:
- api/models.py: keep all imports (read_session_lineage_metadata, normalize_agent_session_source, wal); keep all METADATA_FIELDS (compression_anchor + parent_session_id lineage); normalize_agent_session_source() spreads correctly
- static/ui.js: keep WAL recoveredBadge + all buttons (edit/tts/fork/copy/retry); keep session-guard in finalizeThinkingCard; keep S.session guard for turn.dataset.sessionId (nesquena#1366)
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