Skip to content

fix(diagnostics): measure reconciled IME typing latency - #17171

Merged
nwparker merged 5 commits into
stablyai:mainfrom
nwparker:nwparker/fix-16950-ime-latency-diagnostic
Aug 30, 2026
Merged

nwparker merged 5 commits into
stablyai:mainfrom
nwparker:nwparker/fix-16950-ime-latency-diagnostic

Conversation

@nwparker

@nwparker nwparker commented Aug 29, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

The typing probe now times composed text when it actually reaches the terminal, instead of timing IME key presses that send no text.

Root cause

The prior diagnostic watched echoing keydowns rather than xterm's reconciled composition commit. It therefore recorded zero CJK samples, dropped fast-burst inputs, and could attribute one input's output to another.

The fix observes the existing reconciled xterm composition-session event—the point where committed text has an owner—and keeps uncertain overlaps explicit rather than manufacturing exact latency.

What changed

  • Observe reconciled xterm composition-session commits, including Linux IBus flows where raw compositionend.data is empty.
  • Report direct and IME inputs separately.
  • Split exact latency into input-to-dispatch, dispatch-to-parse, and parse-to-paint.
  • Keep overlaps and attribution gaps in an explicit ambiguous-burst report instead of biasing percentiles.
  • Settle IME source accounting after event propagation so stale/rebound and queue-rejected commits are excluded without losing successfully rerouted prevented commits.
  • Freeze elapsed time on stop and dispose every listener, wrapper, and pending observation.
  • Cap pending inputs at 64 and retained metric series at 2,000.
  • Keep the diagnostic in a cohesive typing-latency/ feature folder with concrete module names.

Linked issue

Fixes #16950

Shared acceptance checklist: #17172.

Visual proof

N/A — this changes an opt-in DevTools diagnostic API and has no user-facing UI. Reports and lifecycle behavior were manually inspected; no visual artifact is claimed.

Exact-head verification

Published head: d1f14c8a0a721b5a8aa28aea094f6224e22b7777

  • Merged current main normally with no changed-file overlap.
  • pnpm install --frozen-lockfile: passed.
  • Focused diagnostic suite: 8 files / 72 tests passed.
  • Registered terminal-input.ime-and-synthetic-forwarding slice: 8 files / 178 tests passed.
  • pnpm tc: passed.
  • pnpm run check:code-quality:changed: zero code, type-aware, and React Doctor findings across 18 changed code files.
  • Reliability manifest: 99 gates valid.
  • Formatting and PR-range whitespace checks: passed.

Coverage includes isolated direct/IME input, empty-data reconciliation, mixed-source ambiguity, timeout/cap gaps, UTF-8 byte accounting, stop/drain cleanup, frozen duration, stale prevented commits, successful prevented reroutes, queue-cap settlement, and once-only propagation settlement.

Native-machine status

Host Verdict Evidence / gap
m4air live / native result unverifiable Orca 1.4.192 was reachable; ABC was the selected input source, so no native CJK claim is made.
awin native result unverifiable The earlier remote runtime was unavailable; absence of contact was not treated as process exit.
SSH openclaw live / graphical result unverifiable IBus is installed, but DISPLAY and WAYLAND_DISPLAY are unset.

Recorded Linux IBus traces prove the reconciliation contract. A fresh graphical native run remains unverifiable.

Readiness and code elegance

Exact-head audit found zero proven P0/P1/P2 findings; counsel was unnecessary because there were no candidates.

  • Fixes attribution at xterm's reconciled composition event rather than adding a second IME state machine.
  • Reuses isImeOwnedKeyboardEvent; capture + bubble settlement distinguishes stale prevented commits from successfully rerouted prevented commits.
  • Instrumentation is inert until start().
  • Pending attribution is capped at 64; each metric series is capped at 2,000.
  • stop() removes all three window listeners and xterm listeners, conditionally restores terminal.write, clears queues and pane references, and freezes duration.
  • No polling, subprocess, IPC/RPC, remote-wire, persistence, mobile, Git-provider, platform-specific, or worktree-only behavior was added.
  • No changed-file overlap with current main; merge-tree and exact diff are clean.

Residual test gaps, not proven defects: no explicit injected-throw rollback test for startup hooks, dynamic pane creation after sampling begins, or parsed output that never paints.

AI disclosure

AI-assisted implementation, test construction, and review were used. Claims above are tied to deterministic tests, exact-head gates, or explicitly named residual gaps.

Agent skill upstream boundary

  • Not applicable.

Checklist

  • Root attribution defect fixed at the reconciled composition owner
  • Existing xterm and IME ownership contracts reused
  • Bounded state and symmetric lifecycle cleanup verified
  • Cross-platform, SSH/remote, folder-workspace, mobile/backcompat, and provider boundaries reviewed
  • Exact-head frozen install, 72 focused tests, 178 reliability tests, typecheck, quality, manifest, formatting, and whitespace pass
  • Native graphical gap stated without overclaiming
  • Readiness and code-elegance reviews have zero findings
  • GitHub CI on published head d1f14c8a0a7 is green (21 successful, 9 skipped, 0 failed or pending); the diff is unchanged and the shared baseline repairs are merged into main.

@coderabbitai

coderabbitai Bot commented Aug 29, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ada25daf-dcc4-4e3c-9d29-3510adc70dbf

📥 Commits

Reviewing files that changed from the base of the PR and between 0689c13 and d1f14c8.

📒 Files selected for processing (8)
  • src/renderer/src/lib/typing-latency/diagnostic-lifecycle.test.ts
  • src/renderer/src/lib/typing-latency/diagnostic-summary.test.ts
  • src/renderer/src/lib/typing-latency/diagnostic-summary.ts
  • src/renderer/src/lib/typing-latency/diagnostic.ts
  • src/renderer/src/lib/typing-latency/echo-candidate-timeout.ts
  • src/renderer/src/lib/typing-latency/echo-instrumentation.test.ts
  • src/renderer/src/lib/typing-latency/echo-instrumentation.ts
  • src/renderer/src/lib/typing-latency/echo-observation.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The change adds a renderer typing-latency diagnostic. It captures direct keyboard and reconciled IME inputs, attributes terminal output to inputs, records exact and ambiguous latency observations, and aggregates source-specific metrics. It adds bounded sample windows, pane discovery, lifecycle controls, report generation, and a browser diagnostic bridge. It replaces the previous module locations and updates the application import. Tests cover input classification, echo attribution, lifecycle behavior, source tallies, sample limits, and Linux IME trace replay.

Merge Risk: 🔵 Low · up to d1f14

The diagnostic now measures reconciled IME latency with explicit ambiguity and cleanup handling, but active reports can still temporarily omit stale unmatched inputs until another input arrives or sampling stops. This may briefly undercount diagnostic samples, so merge is reasonable with explicit owner awareness or follow-up.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 48 functions across 19 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address issue #16950 by measuring reconciled IME commits, separating direct and IME sources, handling fast bursts and attribution gaps, separating latency phases, and correcting lifecycle …
Out of Scope Changes check ✅ Passed The deletions, module reorganization, exported API changes, and added tests support the typing-latency diagnostic objectives. No unrelated product behavior or feature area is identified.
Title check ✅ Passed The title clearly and concisely describes the primary change: measuring reconciled IME typing latency in diagnostics.
Description check ✅ Passed The description is detailed and relevant. It covers the motivation, changes, linked issues, visual proof, testing evidence, AI disclosure, scope boundaries, residual gaps, and checklist status. Some t…
Full details: Linked Issues check

Explanation

The changes address issue #16950 by measuring reconciled IME commits, separating direct and IME sources, handling fast bursts and attribution gaps, separating latency phases, and correcting lifecycle and stop-time accounting. The added tests cover the stated IME, ambiguity, timeout, cleanup, and timing requirements.

Full details: Description check

Explanation

The description is detailed and relevant. It covers the motivation, changes, linked issues, visual proof, testing evidence, AI disclosure, scope boundaries, residual gaps, and checklist status. Some template headings are renamed or omitted, but the required information is present.

  • Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Aug 29, 2026 •

Copy link
Copy Markdown

Greptile Summary

The PR replaces keydown-based typing diagnostics with reconciled direct/IME input attribution and decomposed dispatch, parse, and paint latency measurements.

  • Separates exact samples from ambiguous bursts and reports source-specific metrics.
  • Defers IME accounting until propagation establishes whether a prevented commit was delivered or discarded.
  • Bounds retained observations and pending candidates, freezes stopped durations, and cleans up diagnostic instrumentation.
  • Adds focused coverage for native IME reconciliation, attribution, accounting, and lifecycle behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
src/renderer/src/lib/typing-latency/diagnostic.ts Coordinates bounded input attribution, deferred IME accounting, reporting, and symmetric diagnostic lifecycle cleanup.
src/renderer/src/lib/typing-latency/input-events.ts Observes direct keys and reconciled composition commits, settling IME registrations after route propagation.
src/renderer/src/lib/typing-latency/echo-instrumentation.ts Correlates accepted terminal input with dispatch, parse, and paint stages while preserving ambiguous attribution.
src/renderer/src/components/terminal-pane/terminal-ime-composition-route.ts Exposes validated composition-session details without changing the route’s stale-transport ownership checks.
src/renderer/src/lib/typing-latency/diagnostic-lifecycle.test.ts Verifies the previously reported accounting defect across stale, rerouted, timed-out, and queue-capped IME commits.

Sequence Diagram

sequenceDiagram
    participant IME as Browser/xterm IME
    participant D as Typing diagnostic
    participant R as IME composition route
    participant T as Terminal input
    participant O as Output instrumentation
    IME->>D: composition-session-end (capture)
    D->>D: Record pending IME candidate
    IME->>R: composition-session-end (bubble)
    alt Current route accepts commit
        R->>T: terminal.input(committed text)
        T->>O: onData dispatch timestamp
        R-->>IME: preventDefault
        IME->>D: settle prevented after propagation
        D->>D: Retain delivered input accounting
    else Route drops stale/rebound commit
        R-->>IME: preventDefault without terminal.input
        IME->>D: settle prevented after propagation
        D->>D: Discard pending candidate and source accounting
    end
    O->>O: onWriteParsed
    O->>O: onRender
    O->>D: Exact sample or ambiguous burst
Loading

Reviews (4): Last reviewed commit: "fix(typing-diagnostic): bound echo settl..." | Re-trigger Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: cb92cc69-774c-4bb2-a05e-b393b34fb295

📥 Commits

Reviewing files that changed from the base of the PR and between de851d5 and 0689c13.

📒 Files selected for processing (21)
  • src/renderer/src/components/terminal-pane/terminal-ime-composition-route.ts
  • src/renderer/src/components/terminal-pane/terminal-ime-xterm-linux-native-trace-replay.test.ts
  • src/renderer/src/lib/typing-latency-diagnostic.ts
  • src/renderer/src/lib/typing-latency-echo-instrumentation.test.ts
  • src/renderer/src/lib/typing-latency-echo-instrumentation.ts
  • src/renderer/src/lib/typing-latency/census-probe.ts
  • src/renderer/src/lib/typing-latency/diagnostic-lifecycle.test.ts
  • src/renderer/src/lib/typing-latency/diagnostic-summary.test.ts
  • src/renderer/src/lib/typing-latency/diagnostic-summary.ts
  • src/renderer/src/lib/typing-latency/diagnostic.ts
  • src/renderer/src/lib/typing-latency/echo-instrumentation.test.ts
  • src/renderer/src/lib/typing-latency/echo-instrumentation.ts
  • src/renderer/src/lib/typing-latency/echo-observation.ts
  • src/renderer/src/lib/typing-latency/input-events.test.ts
  • src/renderer/src/lib/typing-latency/input-events.ts
  • src/renderer/src/lib/typing-latency/input-source.test.ts
  • src/renderer/src/lib/typing-latency/input-source.ts
  • src/renderer/src/lib/typing-latency/pane-target.ts
  • src/renderer/src/lib/typing-latency/sample-window.test.ts
  • src/renderer/src/lib/typing-latency/sample-window.ts
  • src/renderer/src/main.tsx
💤 Files with no reviewable changes (3)
  • src/renderer/src/lib/typing-latency-diagnostic.ts
  • src/renderer/src/lib/typing-latency-echo-instrumentation.ts
  • src/renderer/src/lib/typing-latency-echo-instrumentation.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.

Comment on lines +112 to +114
while (removeOldestTimedOut(entry, now)) {
dropped += 1
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Expire timed-out inputs before an active report.

removeOldestTimedOut runs only when a later keystroke arrives. If the final input has no terminal output, an active report() can show zero unmatched keystrokes after two seconds. stop() then adds that same pending input as unmatched. Expire each pane during reporting, or schedule expiry, so report-then-stop gives consistent accounting.

@nwparker

Copy link
Copy Markdown
Contributor Author

Status refresh: published head is d1f14c8a0a72 (not the older hash in the body). GitHub currently reports 21 passing and 9 intentionally skipped checks, with no failures or pending checks. Focused diagnostic/IME audits, typecheck, quality, and remote evidence remain as documented.

@nwparker

Copy link
Copy Markdown
Contributor Author

Review notes before merge

Two corrections to this PR's own claims, both verified against origin/main:

The bounds are not new hardening. MAX_PENDING = 64 already existed at typing-latency-echo-instrumentation.ts:54 and MAX_SAMPLES = 2000 at typing-latency-diagnostic.ts:37, with the same shift() idiom. The previous version could not grow without limit. The new bounds are correct and more complete — pendingCount, ignoredDispatches, parsedBatches and every series are all capped — but this is a carried-forward invariant, not a fix.

The "flat modules → directory reorganisation" framing is wrong, which matters because it invites the wrong review. Rename detection at 5% similarity finds exactly one genuine move (typing-latency-census-probe.ts → census-probe.ts, R090). Everything else is a rewrite: R037, R019, R008, C014. And 50% of the +2335 is test code (+1173). So this is a 682-line module rewritten with roughly four times its previous coverage, not files being shuffled. Splitting it into "move" then "change" would produce a ~400-line move and leave the rest indivisible.

The fix is real, and larger than described

Replayed against recorded hardware traces. The old isEchoingKey accepted key.length === 1 with no IME check — and on macOS every preedit keydown has a 1-character key with keyCode 229, so it did stamp t0, on the first preedit keystroke:

trace commit old t0 taken from inflation
macOS Korean 한 preedit ㅎ 579 ms
macOS Korean 한 / 글 / 한 preedit ㅎ/ㅡ/ㅎ 585 / 393 / 596 ms
macOS Pinyin 你好 preedit n 981 ms

So on macOS the probe was reporting 400–1000 ms of human composition time as typing latency, plus stale pendings timing out into unmatchedKeystrokes. On Linux it recorded nothing at all — key='Process' has length 7, and compositionend.data is empty. "Records no CJK samples" is accurate for Linux and understates macOS, where it produced confidently wrong numbers.

One change that does not belong here — follow-up filed

summarizeWorktreeNesting presents as a 78%-similar rename but changes worktree/SSH semantics: it now partitions by hostId, and swaps a local helper for the shared normalizeRuntimePathForComparison.

The hostId partition is a genuine improvement. The path swap is not: the deleted helper case-folded unconditionally, with the comment "Windows and default macOS filesystems are case-insensitive, so compare case-folded." normalizeRuntimePathForComparison case-folds only Windows paths and the WSL distro portion, never POSIX — verified at src/shared/cross-platform-path.ts:41-55. On case-insensitive APFS, nesting detection now under-counts.

Impact is one counter in an opt-in DevTools census, so this is not merge-blocking. But a worktree/SSH semantics change arriving inside an IME PR under a rename is exactly the pattern that makes large diffs unreviewable, and it is tracked separately rather than left implicit.

Mutation testing: 18 applied, 13 caught, 5 survived

Three survivors are real gaps, and two are worth a follow-up rather than blocking:

  • Ambiguity bounds — computing the lower bound from firstDispatch instead of lastDispatch survives. Those two bounds are the whole point of honest ambiguity reporting, and no test asserts they differ; they appear only as fixture inputs.
  • The capture/bubble settlement seam is never exercised end to end. diagnostic-lifecycle.test.ts mocks discardUndispatchedKeystroke and calls the listener directly rather than dispatching a real event, with settleAfterPropagation(true) passed explicitly every time. So the mechanism this PR describes as load-bearing — distinguishing stale prevented commits from rerouted ones — has no integration coverage. Two mutations survive there for that reason.
  • stopProbe setting stoppedAt = null survives: the arithmetic freeze is proven, the wiring is not.

The remaining two survivors are not gaps — one clause is doubly redundant (event.key === 'Process' is already excluded by both the keyCode 229 check and the length test), and one is a harness artifact.

Input path: clean

terminal-ime-composition-route.ts is the only production input-path file touched (+13/-7). It exports a previously-private type and renames a function so the diagnostic can reuse the existing parser instead of building a second IME state machine — which is the right call. The one logic edit replaces !Number.isSafeInteger(id) || id! <= 0 with a typeof narrowing to drop two non-null assertions; Number.isSafeInteger already returns false for non-numbers, so it is behaviourally identical. No risk to Pinyin, Japanese or Hanja routing, and nothing resembling the latched-flag failure mode this area has shipped before.

@nwparker
nwparker merged commit 8ffd067 into stablyai:main Aug 30, 2026
30 checks passed
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
* fix(diagnostics): measure terminal IME input

* fix(diagnostics): settle prevented IME inputs

* fix(typing-diagnostic): preserve sample attribution invariants

* fix(typing-diagnostic): bound echo settlement state
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.

[Bug]: The typing-latency probe records zero samples for CJK input

1 participant