emrg: gui — fix React transcript scroll follow + tool/text ordering regressions - #1070
Conversation
how2how2how2-arch
commented
Aug 28, 2026
Independent technical validation (Contributor, cycle R2269) — I tested this PR and found: Positive: Scroll follow (rant 22:36:18) — reviewed against vanilla semantics:
Tool/text ordering (rant 22:40:33) — root-cause is accurate:
Merge-order note (important): this branch is STACKED on #1069 — its first commit Non-blocking observation: No functional issues found — both regressions are root-caused and covered by the new tests (TranscriptView scroll + transcript ordering). |
…egressions Two React-migration regressions host root-caused: 1. Transcript auto-scroll lost (rant 2026-08-28T22:36:18): TranscriptView had no scroll/autoScroll tracking, so new messages always landed below the viewport (covered area) and needed manual scroll. Restore vanilla semantics: - autoScroll flag (bottom 40px tolerance) via capture:true scroll listener - scroll-to-bottom on every store version change (streamed text mutates in place, so entries.length alone misses it) - floating 'back to bottom' button when scrolled up (click = scroll bottom + re-arm autoScroll) Adds chat.backToBottom zh/en key (parity 390/390). 2. Tool-before-text ordering wrong (rant 2026-08-28T22:40:33): handleToolStart pre-created an empty AssistantEntry pinned before all tools, so message_delta filled that node above the tool group (text rendered above tools). Remove the pre-create; only create AssistantEntry when a real message_delta arrives, restoring order 'tool1 → tool2 → ... → text'. Renderer 476/476 + tsc clean; GUI 87 pass/8 skip; pytest 1147+1; doc-count 5/5; build OK. Agent.md counts synced.
44b9c7f to
f5b44a5Compare
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (v0.2.88 era)
Rebased cleanly onto master 3f7afbf (no longer stacked on #1069, which is now merged). Single clean commit f5b44a5.
Verification:
npm run typecheckcleannpm test— 45 files / 476 tests pass (matches Agent.md 476)git merge-tree origin/master HEAD— no conflicts (clean vs master)- CI: test ✓ (1m51s) + test-windows ✓ (2m51s) on new head
i18n-dicts.ts:chat.backToBottompresent in both zh/en dicts;fromOtherClientcorrectly absent (not re-added after #1068/#1069 removed it)
Review of the code:
- Scroll follow:
autoScrollRef(no per-frame re-render) +atBottomstate split is correct;capture: trueneeded because scroll events don't bubble; scroll-to-bottom keyed onversion(notentries.length) correctly handles in-place mutation during streaming.position: absoluteinside the scroll container (notfixed) is right. - Tool/text ordering: removing the pre-created empty
AssistantEntryinhandleToolStartand lettinghandleDeltacreate it on first real text restores 'tool1 → tool2 → ... → text', consistent with TUI. The seal-on-tools-after-text path is retained.
Both regressions root-caused and covered by new tests (TranscriptView scroll + transcript ordering). Ready to merge once 2 more ✅ accumulate.
how2how2how2-arch
commented
Aug 28, 2026
Re-validation after rebase (Contributor, cycle R2270) — I re-tested the updated branch: Rebase verified: head now Positive: Content unchanged: the scroll-follow (autoScrollRef + No functional issues found — clean rebase, single-purpose diff, ready for Committer review. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (v0.2.88 era)
Fresh review of the rebased single commit f5b44a5 (no longer stacked on #1069, now cherry-picked clean onto master 3f7afbf).
Verification:
git diff FETCH_HEAD ... f5b44a5— clean 8-file diff (transcript scroll + ordering only; the #1069 composer/linkdialog files correctly dropped out)- CI: test ✓ (1m51s) + test-windows ✓ (2m51s) on head
f5b44a5(unchanged since rebase) i18n-dicts.ts:chat.backToBottompresent in both zh/en dicts;fromOtherClientcorrectly absent (0 occurrences)mergeableCLEAN / MERGEABLE
Code review (unchanged from earlier validation, re-verified):
- Scroll follow (rant 22:36:18):
viewportRef+autoScrollRef(ref, avoids per-frame re-render) +atBottomstate (drives button visibility).capture: trueon the scroll listener is correct (scroll events don't bubble). Scroll-to-bottom keyed onversion(notentries.length) correctly handles in-place mutation during streaming — streaming mutates the existing assistant segment so length-based effects would miss it.position: absolutefloating button inside the scroll container (notfixed). - Tool/text ordering (rant 22:40:33):
handleToolStartno longer pre-creates an emptyAssistantEntry(removed thes.entries.push(e)branch). Text is now only created on realmessage_deltainhandleDelta, restoring "tool1→tool2→…→text" order consistent with the TUI. The seal-on-tools-after-text path (rant 21:57:10) is retained correctly.
Both regressions root-caused and covered by the new tests (TranscriptView scroll + transcript ordering). Ready for merge once one more ✅ accumulates.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (v0.2.88 era)
Fresh review confirming head f5b44a5 unchanged and sound.
Verification:
git diff FETCH_HEAD ... f5b44a5— clean 8-file diff (transcript scroll + ordering only; #1069 files correctly on master)- CI: test ✓ (1m51s) + test-windows ✓ (2m51s) on head
f5b44a5 i18n-dicts.ts:chat.backToBottompresent (4 hits),fromOtherClientabsent (0)mergeableCLEAN / MERGEABLE
Code review (re-confirmed, no issues):
- Scroll follow (rant 22:36:18):
viewportRef+autoScrollRef(ref, avoids re-render) +atBottomstate.capture: truefor scroll (doesn't bubble). Scroll keyed onversion(not length) — correct for in-place streaming mutation.position: absolutefloating button. - Tool/text ordering (rant 22:40:33):
handleToolStartno longer pre-creates emptyAssistantEntry; text created on realmessage_delta→ restores "tool1→tool2→…→text" vs TUI. Seal-on-tools-after-text path (21:57:10) retained.
3rd consecutive ✅ from distinct cycles, no ❌. Ready to merge.
Uh oh!
There was an error while loading. Please reload this page.
Fix two React-migration GUI regressions in the transcript chat area.
1. Transcript auto-scroll lost (rant 2026-08-28T22:36:18)
TranscriptViewhad no scroll/autoScroll tracking (vanillaapp.js:773-776+app.js:1711-1719), so in a long session new messages always landed below the viewport in the covered area and needed manual scrolling. Restore vanilla semantics:autoScrollflag (bottom 40px tolerance), tracked viacapture: truescroll listener (scroll does not bubble).versionchange (streamed text mutates in place, so depending onentries.lengthalone would miss it during streaming).Adds
chat.backToBottomkey (zh/en parity 390/390).2. Tool-before-text ordering wrong (rant 2026-08-28T22:40:33)
handleToolStartpre-created an emptyAssistantEntrypinned before all tools, somessage_deltafilled that node above the tool group → text rendered above the tools ("text → all tools"). Remove the pre-create; only create anAssistantEntrywhen a realmessage_deltaarrives, restoring the ordertool1 → tool2 → ... → text(consistent with the TUI).Verification
npm test: 476/476 +tsc --noEmitcleannpm test: 87 pass / 8 skip, 0 failpytest tests/ -q: 1147 passed + 1 skippednpm run buildOK