Prevent image duplication when splitting blocks near images#159
Conversation
Yorkie Tree's SplitElement deep-copies all attributes to split siblings. For image inlines this creates ghost duplicates because the empty sibling inherits image.* attributes. The bug manifests when resolveOffset maps the split position to the image inline's end boundary (due to its <= semantics), causing splitLevel=2 to split through the image element. Add resolveOffsetForSplit (block-helpers) and resolveBlockNodeOffsetForSplit (yorkie-doc-store) that skip the image inline when charOffset equals textLen, redirecting the split to the start of the next inline instead. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
📝 WalkthroughWalkthroughThis change introduces a new offset-resolution utility, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 119.9s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/docs/src/store/block-helpers.ts (1)
148-164: Logic looks correct and is well-covered by the new tests.The three guards (image-styled, charOffset at end, has next inline) precisely match the bug condition described in the PR objectives. Note that the image-last-in-block case is intentionally not skipped here because there is no next inline — the cache path's
applySplitBlockstill puts that image intobeforevia theinlineEnd <= offsetcomparison (verified by the new test at lines 344–358), so this asymmetry is safe for the cache path. See the related concern inyorkie-doc-store.tsfor the tree path.Optional: tighten the doc comment to call out this design intent explicitly.
📝 Suggested comment refinement (optional)
/** * Like `resolveOffset` but treats an offset at the exact end of an * image inline as belonging to the NEXT inline instead. This prevents * `splitLevel=2` from splitting through the image element. + * + * No skip is performed when the image is the last inline in the block, + * because `applySplitBlock` (offset-based) already places such an image + * into the `before` block correctly. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/store/block-helpers.ts` around lines 148 - 164, The implementation of resolveOffsetForSplit is correct, but the doc comment should be tightened to explicitly state the design intent: clarify that when an offset is exactly at the end of an image inline we treat it as belonging to the NEXT inline to avoid splitLevel=2 splitting through images, and note the intentional asymmetry for image-at-end-of-block (the cache path's applySplitBlock uses inlineEnd <= offset to keep that image in `before`, while the tree path in yorkie-doc-store.ts handles it differently); update the comment above resolveOffsetForSplit to mention these specifics and reference applySplitBlock and the related logic in yorkie-doc-store.ts so future readers understand the rationale.packages/docs/test/store/block-helpers.test.ts (1)
272-359: Test coverage is solid for the new helper and the cache path.The five
resolveOffsetForSplitcases plus the twoapplySplitBlockboundary cases comprehensively pin the cache-side behavior, including the deliberate non-skip when the image is the final inline.toEqual(IMG)on lines 337 and 353 correctly uses deep equality, so thecloneBlock-via-JSON.parseround-trip inapplySplitBlockdoesn’t cause false negatives.One gap to consider: there is no test at the
YorkieDocStorelayer covering split at end-of-block when the image is the last inline. Given the tree-path concern raised inyorkie-doc-store.ts(no skip is possible, sosplitLevel=2is still applied through the image element), a small integration test there would lock down the absence of a ghost image in the tree representation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/test/store/block-helpers.test.ts` around lines 272 - 359, Add an integration test in yorkie-doc-store.ts (or the YorkieDocStore test suite) that exercises splitting a block when the image is the final inline to ensure no ghost image/tree-path bug; create a block with text then an image inline (use the same IMG shape), call the store-level split method (the YorkieDocStore API that triggers splitLevel=2 / the tree split), and assert the resulting document tree contains the image only in the before-block and that the after-block has no image inline and no stray tree node; this mirrors the cache-level tests for resolveOffsetForSplit and applySplitBlock but validates YorkieDocStore’s split path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/docs/src/store/block-helpers.ts`:
- Around line 148-164: The implementation of resolveOffsetForSplit is correct,
but the doc comment should be tightened to explicitly state the design intent:
clarify that when an offset is exactly at the end of an image inline we treat it
as belonging to the NEXT inline to avoid splitLevel=2 splitting through images,
and note the intentional asymmetry for image-at-end-of-block (the cache path's
applySplitBlock uses inlineEnd <= offset to keep that image in `before`, while
the tree path in yorkie-doc-store.ts handles it differently); update the comment
above resolveOffsetForSplit to mention these specifics and reference
applySplitBlock and the related logic in yorkie-doc-store.ts so future readers
understand the rationale.
In `@packages/docs/test/store/block-helpers.test.ts`:
- Around line 272-359: Add an integration test in yorkie-doc-store.ts (or the
YorkieDocStore test suite) that exercises splitting a block when the image is
the final inline to ensure no ghost image/tree-path bug; create a block with
text then an image inline (use the same IMG shape), call the store-level split
method (the YorkieDocStore API that triggers splitLevel=2 / the tree split), and
assert the resulting document tree contains the image only in the before-block
and that the after-block has no image inline and no stray tree node; this
mirrors the cache-level tests for resolveOffsetForSplit and applySplitBlock but
validates YorkieDocStore’s split path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7090160-9dbd-4f94-949a-ba686dad3b4a
📒 Files selected for processing (4)
packages/docs/src/index.tspackages/docs/src/store/block-helpers.tspackages/docs/test/store/block-helpers.test.tspackages/frontend/src/app/docs/yorkie-doc-store.ts
1 participant
Summary
SplitElementdeep-copies all attributes to split siblings — for image inlines this creates ghost duplicates withimage.*attributes but no text contentsplitLevel=2to the next inlineRoot Cause
resolveOffsetuses<=boundary semantics, so offsets at an image inline's end still resolve to that image inline. WhensplitLevel=2splits through an image element, Yorkie'sSplitElementDeepCopy()s all attributes (includingimage.*) to the empty split sibling, creating a ghost image.Changes
packages/docs/src/store/block-helpers.tsresolveOffsetForSplit()— skips image inline at end boundarypackages/docs/src/index.tspackages/frontend/.../yorkie-doc-store.tsresolveBlockNodeOffsetForSplit()— Yorkie Tree node equivalentpackages/docs/test/store/block-helpers.test.tsTest plan
pnpm verify:fastpasses (all 587 docs tests + 162 frontend tests + 107 backend tests)resolveOffsetForSplitcorrectly skips image inline whencharOffset === textLenresolveOffsetForSplitdoes NOT skip for text inlines or when image is last inlineapplySplitBlock) correctly handles image boundaries without adjustment🤖 Generated with Claude Code
Summary by CodeRabbit