Skip to content

fix(tui): focus-aware key routing and streaming auto-scroll - #713

Merged
avoidwork merged 21 commits into
mainfrom
fix/fix-tui-event-capture-auto-scroll
Aug 9, 2026
Merged

fix(tui): focus-aware key routing and streaming auto-scroll#713
avoidwork merged 21 commits into
mainfrom
fix/fix-tui-event-capture-auto-scroll

Conversation

@avoidwork

@avoidwork avoidwork commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Description

The TUI's app-level useInput() hook now routes key events based on focus state, allowing the message list to receive key events and auto-scroll during streaming. Additionally, streaming bubbles now publish scroll-to-bottom events via pub/sub, and message list bubble borders have been removed for performance.

Type of Change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactor (no functional changes)
  • Performance improvement
  • CI / build / tooling

Testing

  • All 1085 tests pass with no failures
  • Lint passes with 0 warnings and 0 errors
  • Manual testing performed:
    • Tab toggles focus between inputBar and message list
    • Escape interrupts streaming (from inputBar) or quits app (from message list)
    • Up/down/pageUp/pageDown navigate when message list is focused
    • Key events bubble to message list when inputBar is focused, enabling auto-scroll during streaming

Coverage

  • Line coverage maintained (84.76% overall, no regression)

Checklist

  • npm run lint passes
  • Tests pass with maintained line coverage
  • No forbidden patterns used
  • Conventional Commit style applied

- proposal.md: motivation and scope for fixing auto-scroll
- design.md: technical approach for focus-aware key routing
- specs/tui-event-routing/spec.md: 3 requirements, 6 scenarios
- tasks.md: 5 implementation groups, 10 tasks
@avoidwork avoidwork added the bug Something isn't working label Aug 9, 2026
@avoidwork avoidwork self-assigned this Aug 9, 2026
@avoidwork avoidwork added the bug Something isn't working label Aug 9, 2026
Restructure useInput() to only intercept keys relevant to the focused
panel, allowing key events to bubble through to the message list when
the inputBar is focused. This fixes auto-scroll during streaming
responses that was blocked by the app-level handler capturing all keys.

- Global keys (Tab, Escape) always handled at app level
- InputBar focused: only intercept Tab, Escape, history nav; pass
  through all other keys to child components
- Message list focused: intercept navigation keys for manual scroll;
  pass through everything else

Signed-off-by: madz
Move completed change to openspec/changes/archive/2026-08-09-fix-tui-event-capture-auto-scroll.

Signed-off-by: madz
The ScrollView's onContentHeightChange callback doesn't fire reliably
when bubbles update via pub/sub (no parent re-render). Now the bubble
publishes a scroll-to-bottom event when its content grows during
streaming, and MessageList handles the actual scroll.
@avoidwork avoidwork changed the title fix: TUI event capture blocks message list auto-scroll (#712) fix(tui): focus-aware key routing and streaming auto-scroll Aug 9, 2026
@avoidwork

Copy link
Copy Markdown
Owner Author

Auto-Scroll Audit: Fundamental Flaws

1. remeasureItem then getBottomOffset — Race Condition

Location: src/tui/messageList.js:370-375

remeasureItem updates itemMeasureKeys state, triggering a future re-render where the MeasurableItem's useLayoutEffect fires. But getBottomOffset() is called immediately after — before that re-render happens. The height hasn't been measured yet. You're scrolling to a bottom offset calculated from stale height data.

Fix: Remove the remeasureItem call entirely. The onContentHeightChange callback already fires after the height has been measured (it's called from useLayoutEffect inside ControlledScrollView).

2. isUserScrollingRef is Computed Once and Never Refreshed

Location: src/tui/messageList.js:329-346

useEffect(() => {
    const checkScrollPosition = () => { ... };
    checkScrollPosition();
}, [scrollRef]); // ← only runs when scrollRef changes

This effect only runs when scrollRef itself changes (i.e., on mount). After that, isUserScrollingRef.current is never updated. If the user scrolls away during streaming, the ref stays false until the component re-mounts. The guard at line 361 is effectively dead code after the first render cycle.

Fix: Check the position directly in handleContentHeightChange without the ref, or use a useEffect with scrollOffset in the dependency array.

3. Dual Scroll Paths — Pub/Sub vs. onContentHeightChange

There are two independent mechanisms trying to scroll to bottom:

  1. Pub/sub (line 303-312): Bubbles publish "scroll-to-bottom"handleScrollToBottomsetScrollOffset(bottomOffset)
  2. onContentHeightChange (line 350-378): ScrollView callback → handleContentHeightChangesetScrollOffset(bottomOffset)

During streaming, both fire simultaneously. The pub/sub path has no throttle. The onContentHeightChange path has a 100ms throttle. They can race, causing the scroll offset to jump back and forth or get stuck at an intermediate value.

Fix: Pick one path. The onContentHeightChange callback is the authoritative source. Remove the pub/sub "scroll-to-bottom" mechanism entirely.

4. ControlledScrollView Uses marginTop: -scrollOffset — Ink's Diffing Can Fight It

Location: ink-scroll-view/dist/index.js:200

marginTop: -scrollOffset,

This is a layout trick — pushing content up via negative margin. But Ink reconciles the entire tree on every state change. When scrollOffset changes, Ink sees a different marginTop value and re-renders the entire content box. During streaming, where bubbles update via pub/sub without parent re-renders, the ScrollView's content box doesn't know its children changed until the next parent render. This creates a disconnect: the content has grown, but the ScrollView hasn't re-measured because the parent hasn't re-rendered.

Fix: This is inherent to how ControlledScrollView works. The real issue is that you're relying on pub/sub for streaming updates without triggering parent re-renders. Consider using a lightweight useEffect on the streaming content ref to trigger a re-render, or accept that streaming updates will be batched to the next parent render cycle.

5. scrollOffset State is the Single Source of Truth, But Never Synced Back from ScrollView

Location: src/tui/messageList.js:65

const [scrollOffset, setScrollOffset] = useState(0);

scrollOffset is set internally but never synced from the ScrollView's own state. If the user scrolls via keyboard (which would go through scrollBy on the ref), the ScrollView's internal scrollOffset changes, but your React state doesn't know. There's no onScroll callback wired up to sync back.

Fix: Wire up onScroll on the ControlledScrollView to keep your state in sync:

onScroll={(offset) => setScrollOffset(offset)}

Summary

# Flaw Severity Fix
1 remeasureItem then getBottomOffset race High Remove remeasureItem call
2 isUserScrollingRef never refreshed after mount High Check position inline in handler
3 Dual scroll paths (pub/sub + callback) Medium Use only onContentHeightChange
4 Pub/sub updates bypass ScrollView measurement Medium Accept batching or trigger parent re-render
5 No onScroll sync back to React state Low Add onScroll callback

@avoidwork

Copy link
Copy Markdown
Owner Author

Additional Audit Findings

6. focus: false on ControlledScrollView — Ink Defers Unfocused Renders

Location: src/tui/messageList.js:445

The scroll view is rendered with focus={false}. Ink ties visual updates to focus — when a component is unfocused, Ink may skip or defer rendering updates to it. The scroll offset is being set correctly in state, but Ink isn't pushing it to the terminal because the component is unfocused.

This explains why tabbing to the panel and back "fixes" the scroll: it forces Ink to re-render that panel with the current scrollOffset. The key event routing fix addresses input capture but doesn't solve the rendering visibility problem.

Fix: Consider focus={true} or a focus-aware toggle that enables the scroll view when content changes during streaming.

7. Throttle Check Drops Updates During Rapid Streaming

Location: src/tui/messageList.js:363-366

The 100ms throttle checks lastScrollTimeRef but doesn't account for React render batching. During rapid streaming, multiple onContentHeightChange callbacks fire within the throttle window. The first one passes the throttle and calls setScrollOffset, but subsequent ones are dropped — even though each represents a real height change that should be scrolled to.

The throttle is meant to reduce update frequency, but it silently drops scroll updates during the fastest part of the stream. The pub/sub scroll-to-bottom mechanism was added to compensate, but it creates a dual-scroll-path race condition (see my previous comment).

Fix: Either remove the throttle entirely (let setScrollOffset batch naturally), or track the last scrolled offset and only update if the new offset differs — preventing redundant updates without dropping real ones.

- Remove remeasureItem call from handleContentHeightChange (race condition)
- Replace stale isUserScrollingRef with inline scroll position check
- Remove dual scroll path (pub/sub scroll-to-bottom) — use onContentHeightChange only
- Add onScroll callback to sync ScrollView state back to React
- Clean up unused isUserScrollingRef declaration
… updates

- Remove focus=false from ControlledScrollView (Ink defers unfocused renders)
- Replace time-based throttle with offset-based dedup (prevents dropping real updates)
- Remove unused SCROLL_THROTTLE_MS constant
@avoidwork

Copy link
Copy Markdown
Owner Author

Why the re-render change was needed

The streaming auto-scroll fix required an additional change beyond just calling scrollToBottom(). Here is the architectural reason:

The problem: ControlledScrollView (from ink-scroll-view) only re-measures content height when the children array changes — i.e., when a new message is added, removed, or the list is cleared. Streaming content updates use pub/sub to update individual bubbles without triggering a parent re-render, so the children array never changes and the ScrollView never re-measures.

scrollToBottom() was being called on every streaming update, but it had no effect because the ScrollView had not re-measured and did not know the content had grown.

The fix: Calling _triggerRender() on each streaming content update forces the MessageList to re-render, which rebuilds the children array, which triggers the ScrollView re-measurement, which fires onContentHeightChange, which calls scrollToBottom().

This is the same mechanism that makes the scroll update when you tab between panels — tabbing causes a re-render, which rebuilds children, which triggers the measurement chain.

Key insight: The pub/sub pattern works great for bubble-level re-renders (only the changed bubble re-renders), but it bypasses the parent entirely, which means the ScrollView never knows content has grown. The re-render bridge is necessary to connect the two systems.

@avoidwork
avoidwork merged commit 61a74fc into main Aug 9, 2026
2 checks passed
@avoidwork
avoidwork deleted the fix/fix-tui-event-capture-auto-scroll branch August 9, 2026 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant