Skip to content

fix(ui): wrap draft notes by terminal cells so long CJK input stays visible - #682

Open
IAMLEIzZ wants to merge 2 commits into
modem-dev:mainfrom
IAMLEIzZ:dev_xiaoshidui
Open

fix(ui): wrap draft notes by terminal cells so long CJK input stays visible#682
IAMLEIzZ wants to merge 2 commits into
modem-dev:mainfrom
IAMLEIzZ:dev_xiaoshidui

Conversation

@IAMLEIzZ

@IAMLEIzZ IAMLEIzZ commented Aug 7, 2026

Copy link
Copy Markdown

Fixes #681

Summary

The draft review-note composer estimates its height with String#length (UTF-16 code units) in draftVisualLineCount, while the textarea wraps by terminal cells. For CJK text (1 code unit = 2 cells) the estimate stays at one row — and since the editor clamps its wrap count to the viewport height, a one-row-high composer never starts wrapping at all: the text scrolls horizontally and everything before the cursor disappears (see the issue for screenshots). English escapes this because code units == cells, so the estimate grows in step and un-clamps the editor.

  • give the composer wrapMode="char" so the wrapped row count is computable exactly (the default word wrap follows UAX Automate prebuilt npm releases #14 — e.g. kinsoku punctuation rules — which a small estimator cannot mirror)
  • rewrite draftVisualLineCount to pack grapheme clusters into terminal cells (a tab counts as 2 cells, matching the editor), with a fast path for printable ASCII lines
  • drop the stale line-count hint state: the estimate is now exact, and the correction channel was already defeated by the viewport clamp and a resetting effect
  • resize the composer in the same frame as each edit (flushSync around onInput), so the editor never sits in a content-taller-than-viewport state

Planned heights (measureAgentInlineNoteHeight) and mounted heights now come from the same exact function, keeping the row-windowed stream in lockstep.

One behavior change to call out: in the composer, English text now wraps at the exact box width (mid-word if needed) instead of at word boundaries.Saved note cards are unchanged — they still word-wrap via wrapText.

Repro

With a ~76-cell composer box and a 60-char / 120-cell CJK draft:

Row estimate Result
Before ceil(60/76) = 1 viewport clamps wrapping, text scrolls horizontally, only the tail visible
After 2 (cluster packing) full text visible, wrapped

Testing

  • bun run typecheck
  • bun run lint
  • bun run format:check
  • bun test ./src ./packages ./scripts ./test/cli ./test/session — 1970 pass; the one failure (src/ui/AppHost.watch.test.tsx) also fails on a clean checkout of the base commit, unrelated to this change
  • bun test test/pty/notes.test.ts — 15/15, including a new real-PTY case: type 86 cells of CJK, assert head and tail stay visible, save with Ctrl-S

New coverage:

  • AgentInlineNote.test.tsx: table-driven estimator cases (CJK, emoji, ZWJ sequences, combining marks, tabs, hard newlines, odd widths), plus parity tests that mount a real TextareaRenderable and assert its virtualLineCount equals the estimator for 23 inputs × 4 widths
  • AppHost stream-level regression: press c, type long CJK, assert the head of the text stays visible
  • PTY regression as described above

@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

PR author is not in the allowed authors list.

@vercel

vercel Bot commented Aug 7, 2026

Copy link
Copy Markdown

@IAMLEIzZ is attempting to deploy a commit to the Modem Team on Vercel.

A member of the Team first needs to authorize it.

@IAMLEIzZ IAMLEIzZ changed the title wrap draft notes by terminal cells so long CJK input stays visible fix(ui): wrap draft notes by terminal cells so long CJK input stays visible Aug 7, 2026

@benvinegar benvinegar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Requesting changes: bulk paste can re-enter onContentChange through flushSync, hit “Maximum update depth exceeded,” and truncate the draft. Please make the update non-reentrant and add bracketed-paste coverage.

This comment was generated by Pi using GPT-5.6 Sol

@IAMLEIzZ

IAMLEIzZ commented Aug 8, 2026

Copy link
Copy Markdown
Author

Requesting changes: bulk paste can re-enter onContentChange through flushSync, hit “Maximum update depth exceeded,” and truncate the draft. Please make the update non-reentrant and add bracketed-paste coverage.

This comment was generated by Pi using GPT-5.6 Sol

Fixed — dropped the per-keystroke flushSync so burst input batches into onerender, and added bracketed-paste + burst-input regression tests. (This crash actually predates the CJK fix; the old hint state flushed the same way.)

@IAMLEIzZ
IAMLEIzZ requested a review from benvinegar August 8, 2026 06:09
@IAMLEIzZ

IAMLEIzZ commented Aug 8, 2026

Copy link
Copy Markdown
Author

Both red checks look unrelated to this change:

  • Windows compatibility fails in test cleanup with EBUSY: resource busy or locked on rmSync of a temp dir (AppHost.workspace.test.tsx:34) — the same failure shows up on other PRs (e.g. runs 31244480282 and 31238789978), so it looks like a recurring Windows flake. Could you re-run the failed job?
  • Vercel just needs a team member to authorize the fork deployment.

Thanks!!!

…isible

The draft composer estimated its row count with String#length (UTF-16
code
units) while the textarea wraps by terminal cells. For CJK text (two
cells
per code unit) the estimate stayed at one row, and the editor clamps its
wrap count to the viewport height, so the one-row composer never started
wrapping and scrolled everything before the cursor out of view.

Give the composer wrapMode="char" so the wrap count is computable
exactly,
count rows by packing grapheme clusters into terminal cells, drop the
stale
line-count hint state, and resize the composer in the same frame as each
edit. Add editor-parity, component, stream, and PTY coverage.

@benvinegar benvinegar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Requesting changes: draftVisualLineCount still does not exactly match the textarea's width semantics for some grapheme clusters. It measures clusters through measureClusterWidth/string-width, while OpenTUI's native editor uses Bun's width result. For a 24-cell textarea, this input reproduces the undercount:

"HEAD-" + ("🇺🇸\u0301").repeat(10) + "-TAIL"

draftVisualLineCount returns 1, but a real TextareaRenderable reports 2 rows. Mounting AgentInlineNote at that width allocates one row and omits -TAIL from the frame. Keycap and heart emoji with an attached combining mark show the same mismatch (string-width reports 1 cell for the cluster while Bun/OpenTUI reports 2).

Please measure with the editor's width semantics and add this input to both the editor-parity table and a mounted visibility regression. Ordinary CJK wrapping, burst input, and bracketed paste otherwise worked in my validation, including the full unit, PTY integration, and TTY smoke suites on a merge with fresh main.

This comment was generated by Pi using GPT-5.6 Sol

@IAMLEIzZ

IAMLEIzZ commented Aug 8, 2026

Copy link
Copy Markdown
Author

Requesting changes: draftVisualLineCount still does not exactly match the textarea's width semantics for some grapheme clusters. It measures clusters through measureClusterWidth/string-width, while OpenTUI's native editor uses Bun's width result. For a 24-cell textarea, this input reproduces the undercount:

"HEAD-" + ("🇺🇸\u0301").repeat(10) + "-TAIL"

draftVisualLineCount returns 1, but a real TextareaRenderable reports 2 rows. Mounting AgentInlineNote at that width allocates one row and omits -TAIL from the frame. Keycap and heart emoji with an attached combining mark show the same mismatch (string-width reports 1 cell for the cluster while Bun/OpenTUI reports 2).

Please measure with the editor's width semantics and add this input to both the editor-parity table and a mounted visibility regression. Ordinary CJK wrapping, burst input, and bracketed paste otherwise worked in my validation, including the full unit, PTY integration, and TTY smoke suites on a merge with fresh main.

This comment was generated by Pi using GPT-5.6 Sol

Confirmed and reproduced — thanks. draftVisualLineCount no longer relies on JS width tables at all: it now measures through a shared native EditBuffer/EditorView, so the count matches the editor by construction on every runtime. Your input is added to both the editor-parity table and a mounted visibility regression; full unit, PTY, and TTY smoke suites pass.

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.

Draft note composer never wraps CJK text — one-line textarea scrolls horizontally

2 participants