fix(dashboard): forward dead-key composition input to chat PTY - #76257
fix(dashboard): forward dead-key composition input to chat PTY#76257dplush wants to merge 10 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for adding a focused fallback for the verified dashboard composition gap.
Problems
web/src/lib/pty-composition.ts:35clearspendingfor every non-ESC xterm payload. The predicate does not compare incoming data with the pending committed text. ThereforeonCompositionEnd("ä")followed by unrelatednoteTerminalData("x")inside the 16 ms window dropsärather than sending the fallback.web/src/lib/pty-composition.test.tscovers a matching-ish"äx"input but not the unrelated-input loss case.
Suggested changes
- Correlate cancellation with the pending composition (including any deliberate chunk handling), rather than cancelling on arbitrary terminal data.
- Add a regression for a pending composition followed by unrelated xterm input before the timer fires.
This is an automated hermes-sweeper review.
| }, | ||
| noteTerminalData(data: string) { | ||
| // A non-protocol xterm input in the short grace window is its own | ||
| // composition delivery (possibly chunked or normalization-different). |
There was a problem hiding this comment.
This clears a pending "ä" for any non-ESC terminal payload, including an unrelated next keystroke such as "x" during the 16 ms grace interval; then the fallback never sends "ä". Please correlate cancellation with the pending composition and add that regression case.
SummaryTwo PRs address #76233 by adding a compositionend fallback for dead-key/IME text that xterm.js may not deliver through onData. #76257 uses a tested pending-commit helper with value-correlated cancellation, while #76337 suppresses its fallback after any printable payload observed during composition. Related pull requests
Duplicates#76257 and #76337 overlap at the issue and fallback level, but their mechanisms are not identical: #76257 uses correlated pending text, whereas #76337 counts arbitrary printable data during composition. Suggested consolidationKeep #76257 open with a salvage path centered on its extracted, value-correlated forwarder and regression coverage; validate the remaining ChatPage integration behavior against #76233. Choose that correlated composition contract as requested in the contributor reviews, and close #76337 as duplicate of #76257 because its alternative counter-based implementation retains the documented unrelated-input loss and adds no focused tests. Complex graphflowchart 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
I76233(["issue #76233 (open)"])
P76257["PR #76257 (open)"]
P76257 -->|best fix| I76233
class I76233 open
class P76257 open
class P76257 best
class P76257 target
click I76233 "https://github.com/NousResearch/hermes-agent/issues/76233"
click P76257 "https://github.com/NousResearch/hermes-agent/pull/76257"
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 2 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 14 kB of PR diffs, 4 kB of issue/PR text, 3 kB of discussion (5 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
Thanks for this fix! It was salvaged into #86761 (cherry-picked onto current main with your authorship preserved in the commit history) and is now merged. Closing since the work has landed. |
Summary
Fixes #76233. The dashboard chat now uses committed
compositionendtext as a narrowly delayed fallback when xterm does not emitonDatafor dead-key/IME input. Normal xterm input remains authoritative; the fallback is cancelled when xterm delivers input, cleaned up on unmount, and SGR mouse reports do not cancel it.Reproducer-first
On current main, the focused regression suite initially failed because the composition forwarder did not exist. The first commit adds that red regression; subsequent commits add the minimal forwarding and lifecycle handling.
Verification
npm test --workspace web -- --run src/lib/pty-composition.test.ts— 6 passednpm run typecheck --workspace web— passednpm run lint --workspace web -- src/lib/pty-composition.ts src/lib/pty-composition.test.ts src/pages/ChatPage.tsx— passed (existing repository warnings only)git diff --check origin/main— passed/opt/data/home/.local/bin/reviewer-claude-review diff origin/main --narrow— pass-with-followups; no blockersScope
Only dashboard PTY composition input and focused regression coverage. No API, persistence, auth, or model behavior changes.