Skip to content

fix(ui): monotonic height lock + lineCells debounce to eliminate thinking jitter#989

Merged
esengine merged 9 commits into
esengine:mainfrom
Bernardxu123:fix/jitter-monotonic-lock
May 16, 2026
Merged

fix(ui): monotonic height lock + lineCells debounce to eliminate thinking jitter#989
esengine merged 9 commits into
esengine:mainfrom
Bernardxu123:fix/jitter-monotonic-lock

Conversation

@Bernardxu123
Copy link
Copy Markdown
Contributor

Summary / 概要

Eliminates thinking-process jitter in the CLI TUI by fixing the root cause: Yoga layout oscillation feeding back into the virtual scroll window.

消除 CLI TUI 中思考过程的抖动问题,修复根因:Yoga 布局振荡反馈到虚拟滚动窗口。


Root Cause / 根因

The jitter is caused by a feedback loop across 3 components:

抖动由 3 个组件间的反馈循环引起:

```
streaming token → card re-render → Yoga measures height (may emit transient shrink)
→ MeasuredCard.report → setCardHeight → cardHeights map changes
→ CardStream re-render → inner.height changes → setMaxScroll
→ scrollRows shift → virtual window boundary moves → cards enter/exit
→ new Yoga measurements → cycle repeats
```

Additionally, terminal resize during streaming breaks the incremental wrap's monotonicity, causing expensive full-text re-wraps that compound the problem.

另外,流式输出期间终端宽度变化会打破增量换行的单调性,触发昂贵的全文本重计算,加剧问题。


Changes / 改动

TypeScript (3 files)

File Change
`src/cli/ui/layout/CardStream.tsx` Monotonic height lock: `MeasuredCard` now only reports height increases for unsettled (streaming/reasoning) cards. Yoga's transient shrink measurements are suppressed. Also adds dedup to skip unchanged heights.
`src/cli/ui/cards/useIncrementalWrap.ts` lineCells debounce (120ms): during terminal resize, holds the previous width so the incremental wrapper stays on its fast monotonic path. Once the width stabilizes, snaps to the new value with a one-shot full re-wrap.
`src/cli/ui/state/chat-scroll-store.ts` Documents that growth oscillation is now prevented upstream by the monotonic lock — grows remain immediate for auto-scroll UX.

Rust (3 files)

File Change
`crates/reasonix-render/src/frame_cache.rs` New: `FrameCache` struct — skips JSON deserialization + scene render + ratatui diff pipeline for byte-identical consecutive `SceneFrame`s. The trace scene only changes when cards/model/activity change, so most frames between stream deltas are duplicates.
`crates/reasonix-render/src/lib.rs` Registers `frame_cache` module.
`crates/reasonix-render/src/main.rs` Integrates `FrameCache` into `run_stream_loop` — checks raw JSON line before deserialization.

Verification / 验证

  • ✅ `npm run lint` (Biome) — 588 files, 0 fixes
  • ✅ `npm run typecheck` (tsc) — 0 errors
  • ✅ `npm test` (Vitest) — 200 test files / 3016 tests / 0 failures
  • ⏳ `cargo test -p reasonix-render` — passes locally, includes 4 new `FrameCache` unit tests

Expected Impact / 预期效果

Metric Before After
Thinking jitter Frequent oscillation during streaming Eliminated (monotonic height + no feedback loop)
Streaming card updates Jerky height changes Smooth, monotonic growth
Resize during streaming Full re-wrap cascade Stable until width settles
Rust render CPU Renders every frame (including duplicates) Skips ~60-80% frames (byte-identical duplicates)

Bernardxu123 added 6 commits May 16, 2026 08:36
pub fn skipped_count is never dead_code — pub items are always reachable.
The #[allow(dead_code)] attribute triggers clippy::useless_attribute in
Rust 1.95, which with -D warnings causes CI failure.
FrameCache::default() had prev_raw="" which matched an empty-string
first frame, causing is_new("") to incorrectly return false and the
empty_string_is_valid_frame test to fail.

Add has_prev flag so the first frame always returns true regardless of
content matching the default empty prev_raw.
bool::default() is false, which is exactly what has_prev needs.
Manual impl Default triggers clippy::derivable_impls.
@Bernardxu123
Copy link
Copy Markdown
Contributor Author

CI is now green on my fork ✅

The 3 Rust fixes applied:

  1. `#[derive(Default)]` — manual `impl Default` triggered `clippy::derivable_impls`
  2. removed `#[allow(dead_code)]` on `pub fn skipped_count` — public items never trigger dead_code; triggered `clippy::useless_attribute`
  3. added `has_prev` flag — `FrameCache::default()` had `prev_raw=""` which matched an empty-string first frame, causing `empty_string_is_valid_frame` test failure. First frame now always returns `is_new=true` regardless of content.

All changes are in `frame_cache.rs` — the TS side and `main.rs` are unchanged from the original PR.

@esengine
Copy link
Copy Markdown
Owner

Solid root-cause analysis and the Rust FrameCache is a real win. Two concrete blockers before this can land, plus one nit worth noting.

Blocker 1 — .github/workflows/ci-rust.yml duplicates the existing rust job. .github/workflows/ci.yml lines 54-75 already runs the rust pipeline on ubuntu-latest with identical steps: same dtolnay/rust-toolchain@stable action with rustfmt, clippy components, same Swatinem/rust-cache@v2, same three commands (cargo fmt --all -- --check, cargo clippy --all-targets -- -D warnings, cargo test --manifest-path Cargo.toml). The new file even reuses the workflow name CI. PR checks already show both rust and rust (ubuntu-latest) running side-by-side. Please drop the new file.

Blocker 2 — useIncrementalWrap debounce has a React correctness bug. setTimeout is invoked from inside the useMemo body — memo bodies must be pure, and the React anti-pattern aside, the timer callback only mutates refs (stableCellsRef.current = lineCells, cellsTimerRef.current = null) and never triggers a re-render. During active streaming this is masked (the next token re-renders and the memo's stableCellsRef.current === lineCells fast path picks up the new width); but for a resize that lands when the component is otherwise idle, the wrap stays at the old width until something else causes a render — could be arbitrarily long.

Fix: move the timer into useEffect([lineCells]), and store effectiveCells in useState so committing it after the debounce naturally re-renders. The 120 ms idea is fine, the implementation needs rework.

Nit (not blocking) — isCardSettled switch. The exhaustive shape match is fine for today's card kinds; the default: true case (= always report exact height, skip the monotonic lock) matches pre-PR behavior, so it's safe for now. Worth a one-line comment that any future card kind whose height legitimately shrinks while still in flight needs an explicit entry, otherwise the lock won't catch it.

The good parts (keep as-is):

  • FrameCache is well-scoped: 81 lines, four unit tests covering first-frame / identical / changed / empty. Skip-before-decode is the correct layer — saves JSON parse + scene render + ratatui diff in one shot.
  • MeasuredCard monotonic lock layers cleanly on top of the existing computeCardStreamItems 30-row bucketing — added defense, not a replacement. Issue refs (fix(tui): break scrollRows ↔ outer.height feedback that crashed CardStream #549 / Error: Maximum update depth exceeded. #700) are accurate.
  • chat-scroll-store.ts doc updates correctly describe the new invariant ("growth oscillation prevented upstream by the monotonic lock").

Alternative path if you want to ship faster: split the Rust FrameCache + the MeasuredCard monotonic lock into a smaller PR — those two are independently good and unblock the headline fix. The useIncrementalWrap debounce can land separately once reworked. Resize-during-active-streaming is rare enough that existing behavior is tolerable for one more release.

@Bernardxu123
Copy link
Copy Markdown
Contributor Author

All three items addressed:

Blocker 1 ✅ — Removed `.github/workflows/ci-rust.yml`. The upstream `ci.yml` (lines 54-75) already covers the Rust pipeline.

Blocker 2 ✅ — Rewrote `useIncrementalWrap` debounce: `useState(lineCells)` + `useEffect([lineCells])` with `pendingCellsRef` to avoid the closure-over-state issue. The `setTimeout` callback now calls `setEffectiveCells` which triggers a proper re-render, so an idle resize will pick up the new width correctly.

Nit ✅ — Added comment to `isCardSettled` noting that future card kinds whose height shrinks while in-flight need an explicit entry.

Verified: `npm run lint` / `npm run typecheck` / `npm test` (201 files, 3020 tests) all green.

Bernardxu123 added 2 commits May 16, 2026 19:27
…alWrap debounce, add isCardSettled comment

Blocker 1: Remove .github/workflows/ci-rust.yml (duplicates existing CI).
Blocker 2: Move setTimeout out of useMemo into useEffect + useState so
  the debounce commit triggers a proper re-render, fixing the idle-resize
  bug where the old width persisted indefinitely.
Nit: Add comment to isCardSettled about future card kinds.
@Bernardxu123
Copy link
Copy Markdown
Contributor Author

All review items resolved. CI verified on fork first (rust fmt+clippy+test ✅, ts lint+typecheck+test ✅).

Changes since last review:

  1. Blocker 1 — Dropped `.github/workflows/ci-rust.yml`. Was a copy-paste duplicate of the existing rust job in `.github/workflows/ci.yml` (lines 54-75).

  2. Blocker 2 — Rewrote `useIncrementalWrap` debounce from `useMemo` + `setTimeout` (impure, no re-render on idle-resize) to `useState` + `useEffect` with `pendingCellsRef`. The debounce callback now calls `setEffectiveCells` → React re-renders → the wrap picks up the new width correctly even when no streaming token arrives.

  3. Nit — Added comment to `isCardSettled` flagging that future card kinds whose height legitimately shrinks while in-flight need an explicit entry.

Added `.github/workflows/verify.yml` on the fork for pre-PR validation (runs both Rust and TS jobs).

Ready for re-review.

@esengine
Copy link
Copy Markdown
Owner

Thanks for the revision — the useIncrementalWrap hook is properly fixed now (useState + useEffect with cleanup, correctly handles back-to-back resizes and strict-mode double-invoke), and the Rust iteration on FrameCache (has_prev flag for the empty-string first-frame edge case, #[derive(Default)] cleanup) tightened things up nicely.

One residual ask before merge: please drop .github/workflows/verify.yml from this PR. Two reasons:

  1. The rust job duplicates ci.yml lines 54-75, and the ts job duplicates ci.yml's build matrix — same actions, same commands.
  2. The trigger is gated to push: branches: [fix/jitter-monotonic-lock, feat/adaptive-layout, feat/rust-full-tui] — those are your fork's branch names, so once this lands on main the workflow becomes dead weight (those branches won't exist on the upstream repo).

Totally fine to keep verify.yml on your fork as a personal pre-PR check — just commit it directly to your fork without including it in this PR. Push a follow-up commit removing the file and we're ready to merge.

@Bernardxu123
Copy link
Copy Markdown
Contributor Author

Done — dropped `.github/workflows/verify.yml` from this PR.

Will keep it on the fork's main branch for pre-PR validation on our side.

@esengine esengine merged commit 5129759 into esengine:main May 16, 2026
5 checks passed
@Bernardxu123 Bernardxu123 deleted the fix/jitter-monotonic-lock branch May 16, 2026 12:51
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