Skip to content

Compute word and line boundaries lazily in MultilineInput key handlers - #1225

Open
nordicnode wants to merge 2 commits into
CodebuffAI:mainfrom
nordicnode:perf-multiline-input-lazy-boundaries
Open

Compute word and line boundaries lazily in MultilineInput key handlers#1225
nordicnode wants to merge 2 commits into
CodebuffAI:mainfrom
nordicnode:perf-multiline-input-lazy-boundaries

Conversation

@nordicnode

@nordicnode nordicnode commented Sep 2, 2026

Copy link
Copy Markdown

Summary

  • In cli/src/components/multiline-input.tsx, defer word and line boundary calculations (findLineStart, findLineEnd, findPreviousWordBoundary, findNextWordBoundary, and visual lineStarts) to the specific key branches that actually consume them.
  • Lazy evaluation: Previously, handleDeletionKeys and handleNavigationKeys eagerly executed 4 string scans backwards and forwards across the text buffer plus visual buffer queries at the top of their callbacks on every single keystroke.
  • Bypass for regular typing: For regular character typing (letters, digits, spaces, symbols, IME multi-byte input, plain enter, and basic backspace/delete), this caused unnecessary string traversals on every character before reaching the character handler. By computing boundaries only within their designated key branches (Ctrl+U, Alt+Backspace, Ctrl+W, Cmd+Delete, Alt+Delete, Ctrl+K, Alt+Left/Right, Home, End, Up, and Down), regular typing completely bypasses boundary scanning.
  • Consolidated getVisualLineInfo helper: Factored the visual line lookup logic into a single getVisualLineInfo() helper inside handleNavigationKeys, eliminating the 4-fold duplicated textRef.current cast across Home, End, Up, and Down while preserving full lazy execution.
  • Exported real boundary helpers: Exported findLineStart, findLineEnd, findPreviousWordBoundary, and findNextWordBoundary so they can be directly imported and exercised by unit tests without parallel copies.
  • Direct unit testing: In cli/src/components/__tests__/multiline-input.test.tsx, imported and directly tested the real boundary implementations across standard text, consecutive whitespace, empty strings, start/end bounds, clamping, and multiline inputs.

Test plan

  • bun test --config=/dev/null src/components/__tests__/multiline-input.test.tsx (77 passed, 0 failed)
  • bun run --cwd cli typecheck (0 errors)
  • bun freebuff/cli/build.ts 0.0.0-ci (successful binary build)
  • Binary smoke test: bun cli/scripts/smoke-binary.ts cli/bin/freebuff (OK)
  • Prettier check: bun x prettier --check cli/src/components/multiline-input.tsx cli/src/components/__tests__/multiline-input.test.tsx (All matched files use Prettier code style)

@codebuff-team

Copy link
Copy Markdown
Contributor

The core idea is sound: findLineStart/findLineEnd/findPreviousWordBoundary/findNextWordBoundary and lineStarts were being computed unconditionally at the top of handleDeletionKeys/handleNavigationKeys even though most keystrokes never reach those specific branches. Moving the computation into each branch (Ctrl+U, Alt+Backspace, Ctrl+W/Alt+Delete, Ctrl+K, Alt+Left/Right, Home/End, Up/Down) is a reasonable micro-optimization, and from a read-through the values are still derived from the same value/cursorPosition at the same point in the control flow, so I don't see a correctness regression in the tsx diff itself.

Two things hold this back from being portable as-is:

  1. The new tests in multiline-input.test.tsx reimplement findLineStart, findLineEnd, findPreviousWordBoundary, and findNextWordBoundary as standalone local functions rather than importing and exercising the actual implementations in multiline-input.tsx. They pass trivially regardless of what the real refactor does, so "74/74 tests pass" doesn't actually validate this change - it validates a parallel copy of the logic that was never at risk.

  2. The Home/End/Up/Down branches now each independently re-derive currentLineInfo/lineStarts via the same textRef.current cast, duplicating that block four times instead of factoring it into a small helper. Fine functionally, but it's exactly the kind of duplication a maintainer would ask to be cleaned up before merge.

The benchmark numbers (74ms vs 2ms for 100k keystrokes) are plausible directionally but not verifiable from a synthetic harness that isn't included in the diff - worth adding the benchmark script itself if you want that number to carry weight.

Worth iterating: extract the lineInfo lookup into one shared helper, and write tests that actually import and call the component's real boundary-finding logic (or exercise it through key events) rather than duplicating it.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Sep 3, 2026
@nordicnode

Copy link
Copy Markdown
Author

Thanks for the thorough review @codebuff-team! Both points have been addressed in the latest commit:

  1. Direct Import & Testing of Real Implementations:
    • Exported findLineStart, findLineEnd, findPreviousWordBoundary, and findNextWordBoundary from cli/src/components/multiline-input.tsx.
    • Removed the duplicate local implementations from multiline-input.test.tsx and updated the test suite to import and exercise the component's actual functions directly across normal text, consecutive whitespace, empty strings, start/end bounds, clamping, and multiline inputs (77/77 tests passing).
  2. Consolidated getVisualLineInfo Helper:
    • Factored the 4-fold duplicated currentLineInfo / lineStarts lookup across Home, End, Up, and Down into a single lazy getVisualLineInfo() helper inside handleNavigationKeys. It is only evaluated when those specific keys are pressed, maintaining clean DRY code without reintroducing eager computation during regular typing or lateral arrow navigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants