Review 4284
#4284 fix(core): preserve multi-line caret navigation in ChatComposerInput by Lee-Dongwook (bucket: contributor)
2a02ed236a834e79e82923e30d0d42f0611a8df0 <- every claim below was verified at this commit
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full WHY: behavior and keyboard semantics change, and a static visual job cannot drive both visible interaction endpoints.
Prior review: none — GitHub returned no submitted reviews or inline review comments at gate 1.
Main movement: the refreshed head merges current main; no later main commit touches the changed files.
WHY 1: ArrowUp always recalled history, even when the caret was inside a multi-line draft. WHY 2: Someone editing an earlier line had the draft replaced instead of moving the caret, losing their place and having to recover it. WHY 3: A chat composer must let people edit a multi-line message without its history shortcut taking over normal text navigation.
USER-FACING PROBLEM: A person editing a multi-line chat draft presses ArrowUp to move within the draft and gets a previous message instead. PROBLEM SEVERITY: harmful friction — the draft is recoverable, but the person loses editing context and has to navigate back.
VERDICT: clear
The composer now checks where the browser selection sits before using a vertical arrow for history. ArrowUp recalls only at the start; ArrowDown moves forward only at the end. A fully selected recalled message still spans both boundaries, so repeated history navigation keeps working while ordinary caret movement stays with the browser.
SOLUTION (1 decision · 60 source/code lines of 245 additions; 141 additions are tests)
- Gate existing history navigation on selection-at-start/end checks; keep recalled-message selection behavior unchanged. [the fix]
BURDEN: low — two pure DOM Range helpers and one key-handler guard; no state, Effect, listener, timer, observer, API, or shared-system owner. BURDEN MATCH: proportionate — a contained keyboard defect is fixed in the existing input owner with focused regression coverage.
VERDICT: clear
OWNER: ChatComposerInput, which owns its built-in contenteditable keyboard and history behavior.
TIER 1: no shared collection-focus primitive applies; this is text-editor caret behavior.
TIER 2: none.
SEAMS: the ChatComposerInput-inside-ChatComposer story was driven; a custom input replaces this owner and intentionally owns its own keyboard behavior.
BEHAVIOR UNIT: pure utility — boundary detection lives in chatComposerSelection.ts; the component keeps only the history transition.
| Seam | Driven result |
|---|---|
built-in ChatComposerInput in ChatComposer
|
boundary recall and mid-text caret movement work |
custom ChatComposer.input
|
not applicable — it replaces ChatComposerInput and its history owner |
VERDICT: clear
Every existing caller using the default hasHistory behavior gets normal ArrowUp/Down caret movement inside drafts. Builders change nothing; there is no opt-in or public surface. Landing this newly exposes no sibling inconsistency because custom inputs already own their own key handling.
VERDICT: clear
No public API change. The two exported helper functions are internal to chatComposerSelection.ts and are not added to a package barrel. No new surface ossifies.
VERDICT: clear
Structural only: the exact diff adds no StyleX, token, theme target, CSS, class, or style-driving prop line.
VERDICT: clear
- API — no signature, export, prop, default, or call-site meaning changes.
- Visual — yes, intentionally: after ArrowUp from mid-text the current two-line draft remains instead of being visibly replaced. The body states, “This preserves the browser’s default vertical caret movement within multi-line drafts.” Outer geometry stayed 496×52 in the before/after capture.
- Theme — no target, token, variable, selector, or override changes.
- Behaviour — intentionally changed only for ArrowUp/Down away from the relevant text boundary. Start-boundary recall, fully-selected history navigation, pending-draft restoration, empty-history behavior, and
hasHistory={false}remain supported. Thefixtitle and patch changeset match that risk.
VERDICT: clear
Effects: zero added, changed, or moved. Listeners/observers/timers: zero. Render/state: no new React pass.
The boundary helper adds full-content copying to a keydown path. A browser operation-count probe on both builds measured:
| Case | N | Before clone calls / copied chars / mutations | After clone calls / copied chars / mutations |
|---|---|---|---|
| ArrowUp mid-draft | 10 / 1,000 / 10,000 | 0 / 0 / 1 | 2 / N / 0 |
| ArrowUp at start | 10 / 1,000 / 10,000 | 0 / 0 / 1 | 2 / N / 1 |
At the start boundary, the user-visible outcome is the same history recall but the new Range.cloneContents() path copies the whole draft before the existing serialization and DOM mutation. Every person pressing a boundary arrow on a long draft pays that additional full-draft copy. The public value?: string and editable impose no length cap (ChatComposerInput.tsx:161–168); 10,000 is a sampled stress point, not a claim about common length. Evidence: operation-count results, measured in core-chatcomposerinput--message-history against both exact SHAs. Source inspection and instrumented Chromium counts independently confirm the scaling.
VERDICT: BLOCKS — the same boundary recall now adds O(N) whole-draft copying on every boundary arrow key, without a count-based regression invariant
VISUAL CHECK: manual frames required
WHY: the acceptance behavior is visible; the exact-head Stable visual regression job is green, but the static MessageHistory story does not drive the ArrowUp transition.
| Sensor | Before | After |
|---|---|---|
| Build | 89abbb3edc1463b114793befc84f6f3f0dbfabe3 |
2a02ed236a834e79e82923e30d0d42f0611a8df0 |
| Story | core-chatcomposerinput--message-history |
same |
| Theme / mode / direction | neutral / light / LTR | same |
| Viewport / DPR | 900×400 / 1 | same |
| Media | forced colors off; reduced motion off; fine pointer; hover | same |
| Semantic state | focused “Message input”, after ArrowUp from offset 2 in line 1 | same |
| Subject geometry | 496×52, visible and in viewport | same |
| Readiness / errors | fonts loaded; 0 animations; no page or Storybook errors | same |
| Case | Before | After | Difference |
|---|---|---|---|
| ArrowUp from offset 2 in the first line |  |
 |
Intentional: old / msg becomes aaa / bbb; source is the PR body sentence quoted under BREAKING. |
Both frames were opened through the image reader and rechecked against the browser state. Chromium event evidence: before prevented default and selected old\nmsg; after did not prevent default, retained aaa\nbbb, moved the caret, and kept focus in the textbox. Start-boundary ArrowUp still recalled and selected old\nmsg on both builds. Moving to the middle of recalled text then pressing ArrowDown restored the draft before, but correctly left the recalled text for native caret movement after.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
- Checked in Chromium: the textbox retained its “Message input” accessible label and focus; mid-text ArrowUp/Down stayed native, while boundary history remained keyboard-reachable.
- Auto-covered on this exact head:
pr-a11yandpr-rtlpassed; no a11y-baseline entry was added. - Source check: no ARIA, role, focus owner, user/AT string, locale, or direction line changed.
VERDICT: clear
| Slot | Verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | clear |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | BLOCKS — whole-draft copy scales with N on boundary keydown |
| VISUAL | clear |
| A11Y & I18N | clear |
GOAL: met — Chromium changed from preventing ArrowUp and replacing aaa\nbbb with history to preserving the draft and native caret movement; boundary recall and focus remained intact.
DISPOSITION: whole-draft cloneContents() on boundary keydown → blocks now; avoid content-sized copying and bank a count-based invariant before merge.
ADVICE: bounded outcome criteria — the boundary decision must not copy work proportional to the whole draft, and a focused operation-count test should stay flat as draft length grows.
AUTHOR CAN PROCEED: yes — ownership and behavior are settled; implementation choice is theirs.
WORST OUTCOME: “Every person pressing a boundary arrow on a long draft pays that additional full-draft copy.” → request changes
JUDGEMENT NEEDED: none — defect fix against the existing contract.
request changes
- [BLOCKS]
isRangeEmptyclones the selected contents for both boundary checks → anyone navigating history in a long draft pays O(N) copying on each boundary keydown; before/after operation counts are 0→N copied characters at N=10, 1,000, and 10,000. ·packages/core/src/Chat/chatComposerSelection.ts:66(const contents = range.cloneContents();) Independent confirmation: source inspection plus instrumented Chromium counts.
Thanks — the caret and history behavior is right in Chromium. One thing before this lands: both boundary helpers clone the editor contents, so an ArrowUp/Down at a boundary now copies the entire draft before the existing history work. Could we avoid content-sized copying in this keydown path and bank a count-based regression?
If you'd rather talk it through with someone, we're in Discord.
[Reviewed by Robohands]
-
packages/core/src/Chat/chatComposerSelection.ts:66— This copies the full draft on every boundary arrow key. Might need a non-copying boundary check.
- A real-browser empty draft represented as
<br>still recalled history at the start on both builds. - The focused Vitest file passed 58/58; pre-existing
act(...)warnings did not come from the added history cases. - All exact-head GitHub Actions jobs completed green; only Vercel deployment and review-gated statuses remain non-green.
TIME total 22m setup/rules 1m brief, critic, measurement harness, versions, repo guidance install/build/server 1m fast-install 40s + build 2s + supervised server 5s (warm main reused: yes) browser/a11y 2m before/after receipts, three behavior paths, empty draft, multi-N operation counts focused tests <1m one Vitest file, 58/58 in 7s code/history 7m issue/PR overlap, main movement, diff, owners, rubric, architecture critique + wiki 7m three presentations; public record and evidence publication remote CI wait 4m 241s; all Actions jobs settled green waste 1m corrected stale wiki filenames and retried image transport
- Safari and Firefox behavior was not run.
- Vercel’s deployment log root cause; its GitHub status is failed on this exact head.
Posted as drafted: request changes. Gate 1 lacked the prior-review checkpoint and exposed an unreproducible public measurement; gate 2 used the wrong section order; gate 3 passed. No maintainer edits.