Fix image inline bugs in docs editor#123
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR refactors text insertion and deletion in documents to use centralized block helpers, adds special handling for image inlines to prevent text insertion into them by creating adjacent text inlines instead, and updates cursor positioning and selection rendering throughout the view layer to treat image runs as fixed-width segments rather than measuring text width. Changes
Sequence DiagramsequenceDiagram
participant User
participant Editor
participant BlockHelpers
participant Store
participant Renderer
User->>Editor: Insert text into image inline
activate Editor
Editor->>BlockHelpers: applyInsertText(block, offset, text)
activate BlockHelpers
BlockHelpers->>BlockHelpers: Detect target inline has image style
BlockHelpers->>BlockHelpers: Split inline into 3 parts (before/inserted/after)
BlockHelpers->>BlockHelpers: Remove image style from inserted part
BlockHelpers->>BlockHelpers: normalizeInlines()
BlockHelpers->>Editor: Return updated block
deactivate BlockHelpers
Editor->>Store: updateBlockInStore(block)
activate Store
Store->>Store: Persist block mutations
deactivate Store
Editor->>Renderer: refresh()
deactivate Editor
User->>Editor: Select content including image
activate Editor
Editor->>Renderer: renderTableContent(content, selectionRects, focused)
activate Renderer
Renderer->>Renderer: Draw image inline
Renderer->>Renderer: Check intersection with selectionRects
alt Image intersects selection
Renderer->>Renderer: Overlay selection color
end
deactivate Renderer
deactivate Editor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Verification: verify:selfResult: ✅ PASS in 113.7s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
07e1426 to
56ea685
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/docs/src/view/selection.ts (1)
170-178:⚠️ Potential issue | 🟠 MajorPass
lineAffinitythroughpositionToPagePixel.This helper still resolves
findPageForPosition(...)without an affinity, so an endpoint that lands exactly on a visual wrap boundary can map to the wrong line. The new image-width branch fixes the X offset inside image runs, but wrapped selections/search highlights can still gain or lose a rect unless start uses'forward'and end uses'backward'.Suggested fix
function positionToPagePixel( paginatedLayout: PaginatedLayout, layout: DocumentLayout, ctx: CanvasRenderingContext2D, canvasWidth: number, blockId: string, offset: number, + lineAffinity: 'forward' | 'backward', ): { x: number; y: number; height: number } | undefined { - const found = findPageForPosition(paginatedLayout, blockId, offset, layout); + const found = findPageForPosition( + paginatedLayout, + blockId, + offset, + layout, + lineAffinity, + );const startPixel = positionToPagePixel( - paginatedLayout, layout, ctx, canvasWidth, lb.block.id, blockStart, + paginatedLayout, layout, ctx, canvasWidth, lb.block.id, blockStart, 'forward', ); const endPixel = positionToPagePixel( - paginatedLayout, layout, ctx, canvasWidth, lb.block.id, blockEnd, + paginatedLayout, layout, ctx, canvasWidth, lb.block.id, blockEnd, 'backward', );Based on learnings: extend selection-related endpoints in
selection.tsto carrylineAffinityand pass it intofindPageForPositionso selection boundaries use the correct visual line context.Also applies to: 199-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/selection.ts` around lines 170 - 178, The positionToPagePixel helper currently calls findPageForPosition without a lineAffinity, causing endpoints on wrap boundaries to pick the wrong visual line; update positionToPagePixel (and its overloads/usages around the second block at lines ~199-214) to accept a lineAffinity parameter and pass it through into findPageForPosition; also update any callers of positionToPagePixel (selection endpoints and search/highlight code) so start positions use 'forward' and end positions use 'backward' when computing page pixels to preserve the intended visual wrap boundary behavior.packages/docs/src/view/peer-cursor.ts (1)
82-104:⚠️ Potential issue | 🟠 MajorHonor
lineAffinityin the table-cell branch.The regular-body path now disambiguates wrap boundaries, but the cell path still stops on the current line whenever
offsetRemaining === lineChars. In a wrapped cell, the caret/selection for a boundary offset still snaps to the previous visual line, so the new image behavior is still inconsistent inside tables.Suggested fix
for (let li = startLine; li < endLine; li++) { const line = cell.lines[li]; let lineChars = 0; for (const run of line.runs) { lineChars += run.text.length; } if (offsetRemaining <= lineChars) { + if ( + lineAffinity === 'forward' && + offsetRemaining === lineChars && + li < endLine - 1 + ) { + offsetRemaining = 0; + continue; + } targetLineIdx = li; lineHeight = line.height; let chars = 0;Based on learnings: in
packages/docs/src/view/*, wrap-aware cursor/selection helpers should applylineAffinityso wrap-boundary offsets resolve to the correct visual line.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/peer-cursor.ts` around lines 82 - 104, The cell branch that iterates cell.lines (using variables offsetRemaining, lineChars, targetLineIdx) currently treats offsetRemaining === lineChars as belonging to the current visual line; update that logic to honor the lineAffinity flag so wrap-boundary offsets resolve to the next visual line when affinity indicates forward. Concretely, when comparing offsetRemaining and lineChars inside the loop in the peer-cursor / table-cell path, treat equality as belonging to the next line if lineAffinity === "forward" (or equivalent enum/flag used in this code), adjust targetLineIdx and cursorX calculation accordingly (mirror the behavior used by the regular-body path), and ensure image-run handling and text-run measurement use the resolved line index. Ensure you reference and use the existing variables lineAffinity, offsetRemaining, lineChars, targetLineIdx, and cursorX so the change integrates with the current control flow.
🧹 Nitpick comments (1)
packages/frontend/src/components/ui/toolbar.tsx (1)
16-24: Add toolbar semantics on the root container.Since this is now the shared toolbar primitive, setting
role="toolbar"(and optionally passingaria-label) will improve screen-reader context across both editors.♿ Suggested tweak
return ( <div + role="toolbar" className={cn( "flex items-center gap-0.5 overflow-x-auto border-b bg-background px-2 py-1 whitespace-nowrap", className, )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/components/ui/toolbar.tsx` around lines 16 - 24, The root div of the toolbar component should expose toolbar semantics: set role="toolbar" on the root container (but allow callers to override by honoring any passed role prop), and ensure aria-label can be passed through (props are already spread via {...props} so keep that; if the component defines its own props type, add optional ariaLabel/aria-label to the prop interface). Update the JSX to apply role="toolbar" by default (e.g., role={props.role ?? "toolbar"}) and ensure aria-label is accepted/passed so screen readers get proper context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs/src/view/doc-canvas.ts`:
- Around line 503-517: The overlay check is using run.imageHeight !== undefined
which misses inline image runs that use a fallback height; change the condition
to detect image runs via run.inline?.style?.image (or equivalent presence of
run.inline.style.image) so the overlay path runs for any inline image regardless
of imageHeight; update the block that currently starts with if (run.imageHeight
!== undefined && selectionRects) to instead check for the inline image style
(e.g., if (run.inline && run.inline.style && run.inline.style.image &&
selectionRects)) and keep the existing intersection and fill logic unchanged.
In `@packages/docs/src/view/editor.ts`:
- Around line 1167-1186: The handler for Arrow keys clears selectedImage but
still prevents default and returns true for ArrowUp/ArrowDown, swallowing the
first vertical-arrow press; update the block handling key === 'ArrowUp' || key
=== 'ArrowDown' so that after setting selectedImage = null it does not call
e.preventDefault() or return true for Up/Down (allowing them to fall through to
TextEditor), while keeping the existing behavior for ArrowLeft/ArrowRight using
cursor.moveTo({ blockId, offset... }) and render(); ensure render() is still
called after deselecting the image when needed and only preventDefault/return
true for horizontal arrows.
In `@packages/frontend/src/app/docs/yorkie-doc-store.ts`:
- Around line 598-620: The insertion logic for image-styled inlines only handles
boundary offsets and misses the middle-offset case, causing Yorkie tree and
applyInsertText cache divergence; update the branch where
targetInline.style.image is true to detect a non-boundary charOffset (charOffset
> 0 && charOffset < targetInline.text.length) and split the existing
targetInline into left and right text inlines plus the image inline so
tree.editByPath inserts three nodes (leftTextInline, imageInline,
rightTextInline) and then insert the newNode at the correct position between
those nodes; use buildInlineNode to create left/right inlines, preserve the
original image style for the imageInline, and call tree.editByPath with the
sliced node list (referencing targetInline, charOffset, inlineIndex, blockIdx,
off) so the tree and the applyInsertText cache remain consistent.
---
Outside diff comments:
In `@packages/docs/src/view/peer-cursor.ts`:
- Around line 82-104: The cell branch that iterates cell.lines (using variables
offsetRemaining, lineChars, targetLineIdx) currently treats offsetRemaining ===
lineChars as belonging to the current visual line; update that logic to honor
the lineAffinity flag so wrap-boundary offsets resolve to the next visual line
when affinity indicates forward. Concretely, when comparing offsetRemaining and
lineChars inside the loop in the peer-cursor / table-cell path, treat equality
as belonging to the next line if lineAffinity === "forward" (or equivalent
enum/flag used in this code), adjust targetLineIdx and cursorX calculation
accordingly (mirror the behavior used by the regular-body path), and ensure
image-run handling and text-run measurement use the resolved line index. Ensure
you reference and use the existing variables lineAffinity, offsetRemaining,
lineChars, targetLineIdx, and cursorX so the change integrates with the current
control flow.
In `@packages/docs/src/view/selection.ts`:
- Around line 170-178: The positionToPagePixel helper currently calls
findPageForPosition without a lineAffinity, causing endpoints on wrap boundaries
to pick the wrong visual line; update positionToPagePixel (and its
overloads/usages around the second block at lines ~199-214) to accept a
lineAffinity parameter and pass it through into findPageForPosition; also update
any callers of positionToPagePixel (selection endpoints and search/highlight
code) so start positions use 'forward' and end positions use 'backward' when
computing page pixels to preserve the intended visual wrap boundary behavior.
---
Nitpick comments:
In `@packages/frontend/src/components/ui/toolbar.tsx`:
- Around line 16-24: The root div of the toolbar component should expose toolbar
semantics: set role="toolbar" on the root container (but allow callers to
override by honoring any passed role prop), and ensure aria-label can be passed
through (props are already spread via {...props} so keep that; if the component
defines its own props type, add optional ariaLabel/aria-label to the prop
interface). Update the JSX to apply role="toolbar" by default (e.g.,
role={props.role ?? "toolbar"}) and ensure aria-label is accepted/passed so
screen readers get proper context.
🪄 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: 6c8b8166-d51f-44c2-96a6-fdc0579e14aa
📒 Files selected for processing (21)
docs/tasks/README.mddocs/tasks/active/20260411-ui-review-polish-lessons.mddocs/tasks/active/20260411-ui-review-polish-todo.mdpackages/docs/src/model/document.tspackages/docs/src/store/block-helpers.tspackages/docs/src/view/doc-canvas.tspackages/docs/src/view/editor.tspackages/docs/src/view/peer-cursor.tspackages/docs/src/view/selection.tspackages/docs/src/view/table-renderer.tspackages/frontend/src/app/Layout.tsxpackages/frontend/src/app/docs/docs-detail.tsxpackages/frontend/src/app/docs/docs-formatting-toolbar.tsxpackages/frontend/src/app/docs/yorkie-doc-store.tspackages/frontend/src/app/documents/document-detail.tsxpackages/frontend/src/app/home/hero-section.tsxpackages/frontend/src/app/home/page.tsxpackages/frontend/src/app/home/why-section.tsxpackages/frontend/src/app/login/page.tsxpackages/frontend/src/components/formatting-toolbar.tsxpackages/frontend/src/components/ui/toolbar.tsx
| if (targetInline.style.image) { | ||
| // Image inlines must not absorb regular text. Insert a new | ||
| // inline node adjacent to the image node instead of putting | ||
| // text inside it. | ||
| const { image: _, ...plainStyle } = targetInline.style; | ||
| void _; | ||
| const newNode = buildInlineNode({ text, style: plainStyle }); | ||
| if (charOffset === 0) { | ||
| // Before image: insert new inline before the image node | ||
| tree.editByPath( | ||
| [blockIdx + off, inlineIndex], | ||
| [blockIdx + off, inlineIndex], | ||
| newNode, | ||
| ); | ||
| } else { | ||
| // After image: insert new inline after the image node | ||
| tree.editByPath( | ||
| [blockIdx + off, inlineIndex + 1], | ||
| [blockIdx + off, inlineIndex + 1], | ||
| newNode, | ||
| ); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Handle non-boundary image inline offsets to keep tree/cache behavior consistent.
This branch only inserts before/after the image node. If an image-styled inline already contains text and insertion happens at a middle offset, the Yorkie tree write and applyInsertText(...) cache write diverge.
💡 Suggested fix
if (targetInline.style.image) {
+ const atInlineBoundary =
+ charOffset === 0 || charOffset === targetInline.text.length;
+ if (!atInlineBoundary) {
+ // Preserve exact insertion semantics for legacy/corrupted image inlines.
+ const updatedBlock = applyInsertText(block, offset, text);
+ tree.editByPath(
+ [blockIdx + off],
+ [blockIdx + off + 1],
+ buildBlockNode(updatedBlock),
+ );
+ return;
+ }
+
// Image inlines must not absorb regular text. Insert a new
// inline node adjacent to the image node instead of putting
// text inside it.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (targetInline.style.image) { | |
| // Image inlines must not absorb regular text. Insert a new | |
| // inline node adjacent to the image node instead of putting | |
| // text inside it. | |
| const { image: _, ...plainStyle } = targetInline.style; | |
| void _; | |
| const newNode = buildInlineNode({ text, style: plainStyle }); | |
| if (charOffset === 0) { | |
| // Before image: insert new inline before the image node | |
| tree.editByPath( | |
| [blockIdx + off, inlineIndex], | |
| [blockIdx + off, inlineIndex], | |
| newNode, | |
| ); | |
| } else { | |
| // After image: insert new inline after the image node | |
| tree.editByPath( | |
| [blockIdx + off, inlineIndex + 1], | |
| [blockIdx + off, inlineIndex + 1], | |
| newNode, | |
| ); | |
| } | |
| } else { | |
| if (targetInline.style.image) { | |
| const atInlineBoundary = | |
| charOffset === 0 || charOffset === targetInline.text.length; | |
| if (!atInlineBoundary) { | |
| // Preserve exact insertion semantics for legacy/corrupted image inlines. | |
| const updatedBlock = applyInsertText(block, offset, text); | |
| tree.editByPath( | |
| [blockIdx + off], | |
| [blockIdx + off + 1], | |
| buildBlockNode(updatedBlock), | |
| ); | |
| return; | |
| } | |
| // Image inlines must not absorb regular text. Insert a new | |
| // inline node adjacent to the image node instead of putting | |
| // text inside it. | |
| const { image: _, ...plainStyle } = targetInline.style; | |
| void _; | |
| const newNode = buildInlineNode({ text, style: plainStyle }); | |
| if (charOffset === 0) { | |
| // Before image: insert new inline before the image node | |
| tree.editByPath( | |
| [blockIdx + off, inlineIndex], | |
| [blockIdx + off, inlineIndex], | |
| newNode, | |
| ); | |
| } else { | |
| // After image: insert new inline after the image node | |
| tree.editByPath( | |
| [blockIdx + off, inlineIndex + 1], | |
| [blockIdx + off, inlineIndex + 1], | |
| newNode, | |
| ); | |
| } | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/src/app/docs/yorkie-doc-store.ts` around lines 598 - 620,
The insertion logic for image-styled inlines only handles boundary offsets and
misses the middle-offset case, causing Yorkie tree and applyInsertText cache
divergence; update the branch where targetInline.style.image is true to detect a
non-boundary charOffset (charOffset > 0 && charOffset <
targetInline.text.length) and split the existing targetInline into left and
right text inlines plus the image inline so tree.editByPath inserts three nodes
(leftTextInline, imageInline, rightTextInline) and then insert the newNode at
the correct position between those nodes; use buildInlineNode to create
left/right inlines, preserve the original image style for the imageInline, and
call tree.editByPath with the sliced node list (referencing targetInline,
charOffset, inlineIndex, blockIdx, off) so the tree and the applyInsertText
cache remain consistent.
Images in the docs editor had several interrelated bugs stemming
from image data being treated as inheritable inline style rather
than distinct content:
- getSplitPointStyle copied image data to empty split sides, causing
Enter to duplicate images and typed text to vanish into image-styled
inlines
- applyInsertText merged regular text into image inlines, making it
invisible since the layout engine treats the entire inline as an
image segment
- Arrow keys on selected images resized instead of navigating; now
they deselect and move the cursor (Shift+Arrow extends selection)
- Cursor/selection pixel positions used measureText('\uFFFC') which
returns near-zero width; now uses run.width for image runs
- Selection overlay was drawn behind opaque images; added post-render
overlay for both body and table cell images
- Yorkie store inserted text into image tree nodes; now creates
adjacent inline nodes instead
- Unified insertText/deleteText in document.ts to always use
applyInsertText/applyDeleteText via updateBlockInStore, removing
body-vs-table-cell code duplication
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/frontend/src/app/docs/yorkie-doc-store.ts (1)
598-626:⚠️ Potential issue | 🟠 MajorHandle middle-offset inserts in image-styled inlines to keep tree/cache updates consistent.
When
0 < charOffset < targetInline.text.length, the Yorkie branch currently treats it as “after image” (Line 612-618), but cache update viaapplyInsertText(...)splits at the exact offset. That can diverge persisted tree vs cached block state for legacy/corrupted image inlines.💡 Proposed fix
if (targetInline.style.image) { + const atInlineBoundary = + charOffset === 0 || charOffset === targetInline.text.length; + if (!atInlineBoundary) { + // Keep Yorkie tree and cache semantics aligned with applyInsertText. + const updatedBlock = applyInsertText(block, offset, text); + tree.editByPath( + [blockIdx + off], + [blockIdx + off + 1], + buildBlockNode(updatedBlock), + ); + return; + } + // Image inlines must not absorb regular text. Insert a new // inline node adjacent to the image node instead of putting // text inside it. - const { image: _, ...plainStyle } = targetInline.style; - void _; + const { image: _image, ...plainStyle } = targetInline.style; + void _image; const newNode = buildInlineNode({ text, style: plainStyle }); - if (charOffset === 0) { + if (charOffset === 0) { // Before image: insert new inline before the image node tree.editByPath( [blockIdx + off, inlineIndex], [blockIdx + off, inlineIndex], newNode, ); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/docs/yorkie-doc-store.ts` around lines 598 - 626, The image-styled inline branch must handle middle-offset inserts (0 < charOffset < targetInline.text.length) by splitting the existing inline at charOffset before inserting the new text node so the persisted tree matches the cache updates; detect this condition using targetInline, charOffset and targetInline.text.length, create left/right inline nodes from the original text (reuse buildInlineNode with the original style minus image), then replace the single inline at inlineIndex with three nodes [left, newNode, right] via tree.editByPath (rather than the current before/after logic), keeping the existing before (charOffset===0) and after (charOffset===length) behaviors unchanged so applyInsertText and tree remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/frontend/src/app/docs/yorkie-doc-store.ts`:
- Around line 598-626: The image-styled inline branch must handle middle-offset
inserts (0 < charOffset < targetInline.text.length) by splitting the existing
inline at charOffset before inserting the new text node so the persisted tree
matches the cache updates; detect this condition using targetInline, charOffset
and targetInline.text.length, create left/right inline nodes from the original
text (reuse buildInlineNode with the original style minus image), then replace
the single inline at inlineIndex with three nodes [left, newNode, right] via
tree.editByPath (rather than the current before/after logic), keeping the
existing before (charOffset===0) and after (charOffset===length) behaviors
unchanged so applyInsertText and tree remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b302d3cd-f1da-445a-bc40-0be6ecfe1478
📒 Files selected for processing (8)
packages/docs/src/model/document.tspackages/docs/src/store/block-helpers.tspackages/docs/src/view/doc-canvas.tspackages/docs/src/view/editor.tspackages/docs/src/view/peer-cursor.tspackages/docs/src/view/selection.tspackages/docs/src/view/table-renderer.tspackages/frontend/src/app/docs/yorkie-doc-store.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/docs/src/view/doc-canvas.ts
- packages/docs/src/view/table-renderer.ts
- packages/docs/src/model/document.ts
- packages/docs/src/store/block-helpers.ts
- packages/docs/src/view/peer-cursor.ts
56ea685 to
13ce0d1
Compare
Summary
getSplitPointStylecopied image data to empty split sides; now stripsimagefrom inherited styleapplyInsertTextmerged text into image-styled inlines; now splits into separate inlinesmeasureText('\uFFFC')≈ 0px; now usesrun.widthfor image runsinsertText/deleteTextindocument.tsto useapplyInsertText/applyDeleteTextviaupdateBlockInStore, removing the body-vs-table-cell branchingTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes