Skip to content

Fix image inline bugs in docs editor#123

Merged
hackerwins merged 1 commit into
mainfrom
fix/docs-image-inline-bugs
Apr 12, 2026
Merged

Fix image inline bugs in docs editor#123
hackerwins merged 1 commit into
mainfrom
fix/docs-image-inline-bugs

Conversation

@hackerwins

@hackerwins hackerwins commented Apr 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Enter key duplicating images: getSplitPointStyle copied image data to empty split sides; now strips image from inherited style
  • Text invisible next to images: applyInsertText merged text into image-styled inlines; now splits into separate inlines
  • Arrow keys resized instead of navigating: Image key handler used arrows for resize; now deselects and moves cursor (Shift+Arrow extends selection)
  • Cursor stuck at image left edge: Cursor/selection pixel calculation used measureText('\uFFFC') ≈ 0px; now uses run.width for image runs
  • No selection highlight on images: Added semi-transparent blue overlay on top of opaque images in both body and table cell renderers
  • Yorkie tree corruption: Yorkie store inserted text into image tree nodes; now creates adjacent inline nodes
  • Body vs table cell duplication: Unified insertText/deleteText in document.ts to use applyInsertText/applyDeleteText via updateBlockInStore, removing the body-vs-table-cell branching

Test plan

  • Insert image in docs editor, press Enter — image should not duplicate
  • Type text before and after an image — text should be visible
  • Click image, press ArrowRight/Left — cursor moves to image edges
  • Shift+Arrow to select across image — blue highlight visible on image
  • Same operations inside a table cell
  • Ctrl+A to select all, Delete — image should be deleted

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Images now display selection highlights when selected in documents.
  • Bug Fixes

    • Fixed text insertion behavior when editing content adjacent to images.
    • Arrow keys now move the cursor around images instead of resizing them.

@coderabbitai

coderabbitai Bot commented Apr 12, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@hackerwins has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 18 minutes and 17 seconds before requesting another review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6d7cffb1-0b3e-4b92-8e98-e69c74153add

📥 Commits

Reviewing files that changed from the base of the PR and between 56ea685 and 13ce0d1.

📒 Files selected for processing (8)
  • packages/docs/src/model/document.ts
  • packages/docs/src/store/block-helpers.ts
  • packages/docs/src/view/doc-canvas.ts
  • packages/docs/src/view/editor.ts
  • packages/docs/src/view/peer-cursor.ts
  • packages/docs/src/view/selection.ts
  • packages/docs/src/view/table-renderer.ts
  • packages/frontend/src/app/docs/yorkie-doc-store.ts
📝 Walkthrough

Walkthrough

The 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

Cohort / File(s) Summary
Core Mutation Logic
packages/docs/src/model/document.ts, packages/docs/src/store/block-helpers.ts, packages/frontend/src/app/docs/yorkie-doc-store.ts
Centralized insertText and deleteText through shared helpers; enhanced applyInsertText to detect image inlines and split them rather than insert text into them, creating new inlines with image style removed; updated yorkie-doc-store to construct adjacent text inlines when target is an image.
Rendering and Selection
packages/docs/src/view/doc-canvas.ts, packages/docs/src/view/table-renderer.ts
Extended table rendering to accept selectionRects and focused parameters; added overlay selection highlighting for image inlines when they intersect with selection rectangles using theme colors.
Cursor and Selection Positioning
packages/docs/src/view/editor.ts, packages/docs/src/view/peer-cursor.ts, packages/docs/src/view/selection.ts
Updated cursor X positioning and selection-rect computation to treat image runs as fixed-width segments using run.width instead of font-based text measurement; removed Arrow-key image resize behavior and instead move cursor (deselecting the image).

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 The image now stands proud and tall,
Text won't invade its style at all,
Selection rects dance in colors bright,
Cursor moves swift, left and right,
No resize keys for images we keep—
Just hop and select, selection runs deep!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the primary objective of the changeset: fixing multiple image inline bugs in the docs editor across text insertion, deletion, cursor positioning, selection rendering, and keyboard handling.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/docs-image-inline-bugs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

Verification: verify:self

Result: ✅ PASS in 113.7s

Lane Status Duration
sheets:build ✅ pass 12.8s
docs:build ✅ pass 8.1s
verify:fast ✅ pass 56.7s
frontend:build ✅ pass 15.5s
verify:frontend:chunks ✅ pass 0.3s
backend:build ✅ pass 4.6s
cli:build ✅ pass 1.7s
verify:entropy ✅ pass 13.9s

Verification: verify:integration

Result: ✅ PASS

@codecov

codecov Bot commented Apr 12, 2026

Copy link
Copy Markdown

@hackerwins
hackerwins force-pushed the fix/docs-image-inline-bugs branch from 07e1426 to 56ea685 Compare April 12, 2026 11:29

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 | 🟠 Major

Pass lineAffinity through positionToPagePixel.

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.ts to carry lineAffinity and pass it into findPageForPosition so 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 | 🟠 Major

Honor lineAffinity in 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 apply lineAffinity so 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 passing aria-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

📥 Commits

Reviewing files that changed from the base of the PR and between 4959bd2 and 07e1426.

📒 Files selected for processing (21)
  • docs/tasks/README.md
  • docs/tasks/active/20260411-ui-review-polish-lessons.md
  • docs/tasks/active/20260411-ui-review-polish-todo.md
  • packages/docs/src/model/document.ts
  • packages/docs/src/store/block-helpers.ts
  • packages/docs/src/view/doc-canvas.ts
  • packages/docs/src/view/editor.ts
  • packages/docs/src/view/peer-cursor.ts
  • packages/docs/src/view/selection.ts
  • packages/docs/src/view/table-renderer.ts
  • packages/frontend/src/app/Layout.tsx
  • packages/frontend/src/app/docs/docs-detail.tsx
  • packages/frontend/src/app/docs/docs-formatting-toolbar.tsx
  • packages/frontend/src/app/docs/yorkie-doc-store.ts
  • packages/frontend/src/app/documents/document-detail.tsx
  • packages/frontend/src/app/home/hero-section.tsx
  • packages/frontend/src/app/home/page.tsx
  • packages/frontend/src/app/home/why-section.tsx
  • packages/frontend/src/app/login/page.tsx
  • packages/frontend/src/components/formatting-toolbar.tsx
  • packages/frontend/src/components/ui/toolbar.tsx

Comment thread packages/docs/src/view/doc-canvas.ts Outdated
Comment thread packages/docs/src/view/editor.ts
Comment on lines +598 to +620
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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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]>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
packages/frontend/src/app/docs/yorkie-doc-store.ts (1)

598-626: ⚠️ Potential issue | 🟠 Major

Handle 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 via applyInsertText(...) 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

📥 Commits

Reviewing files that changed from the base of the PR and between 07e1426 and 56ea685.

📒 Files selected for processing (8)
  • packages/docs/src/model/document.ts
  • packages/docs/src/store/block-helpers.ts
  • packages/docs/src/view/doc-canvas.ts
  • packages/docs/src/view/editor.ts
  • packages/docs/src/view/peer-cursor.ts
  • packages/docs/src/view/selection.ts
  • packages/docs/src/view/table-renderer.ts
  • packages/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

@hackerwins
hackerwins force-pushed the fix/docs-image-inline-bugs branch from 56ea685 to 13ce0d1 Compare April 12, 2026 11:34
@hackerwins
hackerwins merged commit d50e76e into main Apr 12, 2026
1 check passed
@hackerwins
hackerwins deleted the fix/docs-image-inline-bugs branch April 12, 2026 11:35
hackerwins added a commit that referenced this pull request Apr 19, 2026
- random-axis-id (#127)
- axis-id-selection (#130)
- docs-image-editing Phase 1 (#121, #123)
- nested-tables (#128, #129)
- docs-mobile-toolbar (#132)
- release-v0.3.3 (#131)
- docs-image-editing lessons
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.

1 participant