Repository navigation
Underline composing (uncommitted) IME text in Docs (issue #342) - #514
Conversation
Design note for issue wafflebase#342. Composing (uncommitted) IME text is drawn with no underline, so there is no cue that it is mid-composition; native text fields and Google Docs underline it. The composing text already lives on a view-local render path (injectComposingInline splices it into the block layout without touching the model, see wafflebase#318/wafflebase#332 and wafflebase#346), so the underline is a paint-time decoration of that injected run and never reaches the document model. The note proposes a view-local `composing` marker set by composingStyleFrom and a thin solid underline drawn in renderRun, covering both the browser IME and software-Hangul assembler paths. Design-note-first; implementation to follow on this branch.
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds and indexes a design document for rendering view-local underlines on uncommitted IME composing text, covering the ChangesIME composing underline design
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/design/docs/docs-ime-composing-underline.md (1)
130-133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtend tests to cover lifecycle teardown and underline semantics.
The plan does not explicitly test that commit, abort, and blur clear the synthetic run, nor that painting uses a 1px solid line in the resolved text color. Add lifecycle/integration assertions and verify
lineWidth,strokeStyle, and the absence of further underline calls after the composing context is cleared.Also applies to: 140-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/docs/docs-ime-composing-underline.md` around lines 130 - 133, Extend the lifecycle and integration tests described around the composition teardown flow to cover commit, abort, and blur clearing the composing context and removing the synthetic run. Add paint assertions that the underline uses a 1px line width and the resolved text color, and verify no further underline drawing calls occur after the context is cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/docs/docs-ime-composing-underline.md`:
- Around line 72-91: Remove the composing flag from the persisted InlineStyle
definition. Introduce a render-only wrapper or equivalent view-local metadata
type for the synthetic inline created by composingStyleFrom and
injectComposingInline, and have the layout/painting path read composing from
that wrapper without allowing it into document, clipboard, clone, or export
data.
---
Nitpick comments:
In `@docs/design/docs/docs-ime-composing-underline.md`:
- Around line 130-133: Extend the lifecycle and integration tests described
around the composition teardown flow to cover commit, abort, and blur clearing
the composing context and removing the synthetic run. Add paint assertions that
the underline uses a 1px line width and the resolved text color, and verify no
further underline drawing calls occur after the context is cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 507fd04b-269e-4669-a07f-b12d8df20969
📒 Files selected for processing (2)
docs/design/README.mddocs/design/docs/docs-ime-composing-underline.md
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Move the view-local composing marker from InlineStyle onto LayoutRun, the view-only run type, so it is not type-reachable from any model, clipboard, clone, or DOCX/PDF export path — leak-proof by construction rather than by convention. injectComposingInline returns the injected inline's index and layoutBlock tags the resulting runs; composingStyleFrom is unchanged and renderRun reads run.composing. Also expand the test plan per review: assert the composing underline paints a 1px solid stroke in the resolved text color, and that commit / abort / blur clear the run so no further underline is drawn once the composing context is gone.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design/docs/docs-ime-composing-underline.md (1)
111-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid double-stroking composing underlines. If the run already has
style.underline, a second 1px stroke at the same y can darken or thicken the line; use a singlestyle.underline || run.composingstroke instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/docs/docs-ime-composing-underline.md` around lines 111 - 123, Update renderRun’s underline rendering to use one stroke when either style.underline or run.composing is set, via the combined condition style.underline || run.composing. Preserve the existing baselineY + 2 position, resolved text color, and computed x/width, while avoiding a separate composing-underline draw.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/docs/docs-ime-composing-underline.md`:
- Around line 169-176: The documentation currently tests persistence only after
composition ends; add coverage while ComposingContext remains non-empty by
serializing the document and checking persisted, clipboard, and DOCX-PDF export
inputs are unchanged and contain no composing marker. Keep the existing
post-composition assertions intact and target the active-composition test flow
near the referenced composing lifecycle cases.
---
Outside diff comments:
In `@docs/design/docs/docs-ime-composing-underline.md`:
- Around line 111-123: Update renderRun’s underline rendering to use one stroke
when either style.underline or run.composing is set, via the combined condition
style.underline || run.composing. Preserve the existing baselineY + 2 position,
resolved text color, and computed x/width, while avoiding a separate
composing-underline draw.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fafc40e4-0100-4216-80dd-85e3eddbce0d
📒 Files selected for processing (1)
docs/design/docs/docs-ime-composing-underline.md
Add a test-plan case that serializes / copies / exports the document while ComposingContext is non-empty and asserts the model, clipboard, and export inputs are unchanged with no composing marker — proving the live preview never mutates persisted data mid-composition, not only after commit.
|
@semimikoh Thanks for your report. Could you implement in this PR? |
Composing IME text was drawn plainly, with no cue that it is mid-composition; native fields and Google Docs underline it (issue wafflebase#342). The composing text already lives on a view-local render path (injectComposingInline splices it into the block layout without touching the model), so this adds only the underline decoration on top. The marker lives on LayoutRun, the per-render view type — never on the persisted Inline / InlineStyle — so it cannot leak into the model, clipboard, clone, or DOCX/PDF export. injectComposingInline returns the injected inline's index; layoutBlock tags every run derived from it (so a wrapped composition tags each sub-run); renderRun paints a 2px solid underline in the resolved text color at baselineY + 2 when run.composing is set (2px matches the caret width). composingStyleFrom is unchanged, and both the browser IME and software-Hangul assembler paths are covered.
1px read as too faint against text; 2px matches the caret's cursorWidth so the composing cue carries the same visual weight as the caret already on screen.
hackerwins
left a comment
There was a problem hiding this comment.
Thanks for your contribution.
Summary
Draw an underline under uncommitted (composing) IME text in the Docs editor,
and remove it automatically when the composition commits or aborts — matching
native text fields and Google Docs (issue #342).
This PR lands both the design note and its implementation on the same branch.
Design (
docs/design/docs/docs-ime-composing-underline.md): the composingtext already lives on a view-local render path (spliced into the block layout by
injectComposingInline, never written to the model — see #332/#346). Theunderline is a paint-time decoration of that injected run, keyed off a
view-local marker that lives on
LayoutRun(NOT on the persistedInline/InlineStyle), so it is leak-proof by construction.Implementation:
injectComposingInlinenow returns{ inlines, composingIndex }— the indexof the spliced composing inline.
LayoutRungains a view-localcomposing?: boolean;layoutBlocktags everyrun derived from the composing inline (so a wrapped composition tags each
sub-run).
renderRunpaints a 2px solid underline in the run's resolved text color atbaselineY + 2whenrun.composingis set. (A 1px line was tried first butread as too faint against text; 2px matches the caret's
cursorWidth, so thecomposing cue carries the same visual weight as the caret already on screen.)
composingStyleFromis unchanged; the marker never touches the model,clipboard, clone, or DOCX/PDF export.
Both the browser IME path and the software-Hangul assembler path go through
injectComposingInline, so both are covered.Why
Composing IME text was drawn plainly, with no cue that it is mid-composition.
Every native field and Google Docs underline composing text. The fix is small
and isolated because #318 (PR #332) and the composing-preview wiring (PR #346)
already moved composing text onto the view-local render path; this PR only adds
the underline decoration on top.
Keeping the marker on
LayoutRun(a per-render view structure) rather than onthe persisted style types means it cannot leak into the model or any export
path — addressing the review feedback on the design note.
Linked Issues
Fixes #342
Author checklist
Verification
Local
pnpm verify:fastruns green.Test plan
Unit tests (Vitest,
packages/docs):injectComposingInlinereturns the index of the spliced composing inline(mid-inline, boundary, end-of-block, empty block;
-1for empty text).layoutBlocktags exactly the composing run(s); a composition that wrapsacross lines tags every sub-run; no composing context → no run tagged.
renderRunstrokes a single 2px solid underline in the resolved text color atbaselineY + 2for a composing run; a plain run strokes none.Manual smoke (
pnpm dev/ docs demo): type Korean; the composing syllable showsa 2px underline that disappears on commit.
Risk Assessment
committed text, other formatting, or measurement/wrapping. Scoped to docs.
composingmarker is view-local (LayoutRun)and never persisted or exported.
Notes for Reviewers
commit/abort. See the attached screen recording (Korean IME) for the live
behavior; the paint-layout unit test asserts the stroke geometry/width/color.
Width note: 1px looked too faint, so it was set to 2px to match the caret.
highlight are intentionally out of scope (see the note's Non-Goals).
with Claude Code (Claude Opus 4.8). I reviewed every line and own the
technical choices.