Add editable header/footer with page number support (Phase 4.1)#110
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds header/footer support across the docs package: model extensions (optional header/footer containers and pageNumber inline flag), editor routing/edit context, store APIs and Yorkie serialization, pagination helpers, canvas rendering of header/footer (with page-number substitution), and tests/docs/tasks updates describing implementation steps. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Canvas as DocCanvas
participant TextEd as TextEditor
participant Doc as Doc (model)
participant Store as DocStore / Yorkie
User->>Canvas: click at (x,y)
Canvas->>TextEd: resolveClickTarget(px,py) -> EditContext
TextEd->>Doc: setEditContext(header|footer)
Doc->>Store: ensureHeader/ensureFooter() if missing (may full-commit)
TextEd->>Doc: insertPageNumber / edit ops (context-aware)
Doc->>Store: commit mutation (in-body or full-tree for HF)
Store-->>Doc: updated Document state
Doc-->>TextEd: updated layout data (headerLayout/footerLayout)
TextEd->>Canvas: render(updated layouts, cursors, selections, editContext)
Canvas-->>User: drawn page with header/footer and page numbers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 |
Contributor
Verification: verify:selfResult: ✅ PASS in 113.3s
Verification: verify:integrationResult: ✅ PASS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/docs/src/view/editor.ts (2)
811-834:⚠️ Potential issue | 🟠 MajorUndo/redo rehomes the caret to the body even in header/footer mode.
These handlers always move the cursor to
doc.document.blocks[0], but they do not reset the current edit context. After undo/redo in header/footer mode, the UI can stay in header/footer editing while the cursor points at a body block, which leaves no visible caret and sends the next keystroke to the wrong region.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/editor.ts` around lines 811 - 834, The undoFn and redoFn move the cursor to doc.document.blocks[0] but don’t update the editor’s current edit context, causing header/footer mode to remain while the caret is in the body; after calling docStore.undo()/redo(), doc.refresh(), and cursor.moveTo(...) update the editor’s editing context to match the block you moved to (e.g., derive the region from the target block and call the existing API that switches edit region such as setCurrentEditContext / setEditingRegion / enterHeaderFooterMode as appropriate) so the UI and caret are consistent; apply the same change to both undoFn and redoFn and clear layoutCache/trigger render as before.
472-494:⚠️ Potential issue | 🟠 MajorBlock-level multi-selection in header/footer still targets body blocks.
This helper uses
Doc.getBlockIndex(), which is context-aware, but then dereferencesdoc.document.blocks[i]. Applying block styles, list toggles, indent, or outdent to a multi-block header/footer selection will operate on body paragraphs instead.Suggested fix
- for (let i = lo; i <= hi; i++) { - const b = doc.document.blocks[i]; + const contextBlocks = doc.getContextBlocks(); + for (let i = lo; i <= hi; i++) { + const b = contextBlocks[i];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/editor.ts` around lines 472 - 494, The loop incorrectly dereferences doc.document.blocks[i], which ignores header/footer context and causes multi-block ops to target body blocks; instead resolve each block via the Doc API that respects context (use the doc methods that return a block by id/index rather than direct array access) — e.g., use the block id from doc.getBlockIndex(range.anchor.blockId)/range.focus.blockId and call the context-aware getter (doc.getBlock or doc.getBlockById/getBlockByIndex) for each index, then pass that block into fn and markDirty(b.id) so header/footer selections operate on the correct blocks.packages/docs/src/model/document.ts (1)
354-400:⚠️ Potential issue | 🟠 MajorCross-block inline styling still writes into the body array.
getBlockIndex()is now region-aware, but this branch still readsthis._document.blocks[i]. In header/footer mode, selecting across multiple header/footer paragraphs and applying bold/link/color will mutate body blocks instead of the active region.Suggested fix
- for (let i = fromBlockIdx; i <= toBlockIdx; i++) { - const block = this._document.blocks[i]; + const contextBlocks = this.getContextBlocks(); + for (let i = fromBlockIdx; i <= toBlockIdx; i++) { + const block = contextBlocks[i];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/model/document.ts` around lines 354 - 400, The cross-block styling loop is reading this._document.blocks[i] (mutating body) instead of the currently active region; replace that direct access with a region-aware accessor (e.g. call a region-aware helper like getBlockByIndex(i) or implement getRegionBlockAtIndex(i) that returns the block from the active region) so the loop uses that block instance for table handling, applyStyleToBlock, store.updateBlock and store.updateTableCell; update all references in the loop (block variable, block.id, cell.blocks iteration) to use the region-aware block and ensure _blockParentMap/getBlockIndex usage remains unchanged.packages/frontend/src/app/docs/yorkie-doc-store.ts (1)
423-426:⚠️ Potential issue | 🟠 Major
getBlock()still ignores header/footer blocks.The write paths now accept header/footer block IDs, but
getBlock()only searchesdocument.blocks. That makes this store behave differently fromMemDocStoreand will break any read-before-write path on a header/footer block. Reuse the same region-aware lookup here.🤖 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 423 - 426, getBlock currently only searches document.blocks and therefore ignores header/footer regions; update getBlock (and use getDocument) to perform a region-aware lookup like MemDocStore by checking header/footer (or other region collections) in addition to document.blocks—e.g., search document.headerBlocks, document.footerBlocks (or the document.regions/map) for a block with the matching id and return it if found, falling back to document.blocks to preserve existing behavior.packages/docs/src/view/text-editor.ts (1)
79-124:⚠️ Potential issue | 🟠 MajorHeader/footer mode isn’t threaded through the rest of the editor yet.
Adding
editContexthere is only half the change: helpers likehandleDocStart(),handleDocEnd(),selectAll(),getSelectedBlocks(),getVisualLineRange(), andgetPixelForPosition()still read the body layout/body block list. In header/footer mode that makesEndcollapse to offset0, Cmd/Ctrl+Home/End jump into the body, and copy/cut/select-all operate on the wrong region. Please switch these paths togetContextBlocks()plus the header/footer layouts wheneditContext !== 'body'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 79 - 124, The editContext flag was added but not propagated into navigation and selection helpers; update handleDocStart, handleDocEnd, selectAll, getSelectedBlocks, getVisualLineRange, and getPixelForPosition to use getContextBlocks() instead of the body block list and to use getHeaderLayout()/getFooterLayout() when this.getEditContext() !== 'body' (fall back to this.getLayout()/this.getPaginatedLayout() for body). Ensure each helper checks this.editContext (or calls getEditContext()), selects the appropriate layout via getHeaderLayout/getFooterLayout when in header/footer mode, and passes that layout plus the result of getContextBlocks() into any layout-dependent calculations so cursor movement, Home/End, and selection operate on the active context.
🧹 Nitpick comments (3)
packages/docs/test/model/document.test.ts (1)
604-622: Assert default margin inensureHeader()/ensureFooter()tests.These tests confirm creation, but not the default
marginFromEdgecontract.Proposed test enhancement
-import { createEmptyBlock, getBlockText } from '../../src/model/types.js'; +import { + createEmptyBlock, + getBlockText, + DEFAULT_HEADER_MARGIN_FROM_EDGE, +} from '../../src/model/types.js'; expect(doc.document.header).toBeDefined(); expect(doc.document.header!.blocks).toHaveLength(1); expect(doc.document.header!.blocks[0].type).toBe('paragraph'); + expect(doc.document.header!.marginFromEdge).toBe(DEFAULT_HEADER_MARGIN_FROM_EDGE); expect(doc.document.footer).toBeDefined(); expect(doc.document.footer!.blocks).toHaveLength(1); + expect(doc.document.footer!.marginFromEdge).toBe(DEFAULT_HEADER_MARGIN_FROM_EDGE);As per coding guidelines "
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests for critical business logic and edge cases".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/test/model/document.test.ts` around lines 604 - 622, Add assertions in the two tests that verify the default marginFromEdge value when ensureHeader() / ensureFooter() creates the header/footer: after calling doc.ensureHeader() assert doc.document.header!.marginFromEdge equals the expected default constant (e.g., DEFAULT_MARGIN or numeric value used by Doc), and after doc.ensureFooter() assert doc.document.footer!.marginFromEdge equals that same default; reference the ensureHeader, ensureFooter methods and the document.header/footer.marginFromEdge property so the test enforces the marginFromEdge contract.packages/docs/test/model/types.test.ts (1)
194-211: Add an explicit assertion forfooter.marginFromEdge.The test sets
footer.marginFromEdgebut does not verify it; asserting it will better lock the header/footer shape contract.Proposed test tweak
expect(doc.footer).toBeDefined(); expect(doc.footer!.blocks).toHaveLength(1); + expect(doc.footer!.marginFromEdge).toBe(48);As per coding guidelines "
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests for critical business logic and edge cases".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/test/model/types.test.ts` around lines 194 - 211, The test "should include header and footer in Document type" sets footer.marginFromEdge but doesn't assert it; update the test to include an explicit assertion for footer.marginFromEdge (e.g., add expect(doc.footer!.marginFromEdge).toBe(48)) alongside the existing header/footer checks so the Document shape contract for footer.marginFromEdge is verified; locate the test case that constructs Document with createEmptyBlock() and add the assertion there.packages/docs/src/view/cursor.ts (1)
33-38: Move/update the JSDoc aboveisVisible()for accuracy.The comment above this segment describes pixel coordinate resolution, but the method returns only cursor visibility. A small doc placement/update will avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/cursor.ts` around lines 33 - 38, The JSDoc above isVisible() is inaccurate: update the comment to describe visibility (e.g., "Return whether the cursor is visible.") and attach it directly to the isVisible() method, and relocate the existing pixel-coordinate/paginated description to the actual method that computes pixel coordinates (the function that resolves cursor x/y for rendering) so the pixel-resolution doc sits with its corresponding function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/docs/docs-header-footer.md`:
- Around line 95-100: The fenced code blocks are unlabeled and trigger MD040;
update each backticked block containing snippets (the blocks showing the
headerLayout/footerLayout/bodyLayout/paginated lines, the "for each page:"
steps, and the "root/ header/ footer/ body" tree) to include a language
identifier such as "text" (e.g., replace ``` with ```text) so markdownlint stops
flagging them; apply this change to the occurrences at the shown ranges
(including the other instances around the 127-135 and 262-275 sections).
In `@docs/tasks/active/20260408-header-footer-todo.md`:
- Line 15: The markdown has a heading level jump: the "### Task 1: Data Model —
HeaderFooter type, Document extension, pageNumber inline" heading is H3 directly
after the H1; change that heading to H2 ("## Task 1: Data Model — HeaderFooter
type, Document extension, pageNumber inline") or add the missing intermediate H2
heading above it so the document follows proper heading hierarchy and resolves
the markdownlint error.
In `@packages/docs/src/store/memory.ts`:
- Around line 206-218: The table-editing code still calls findBlock (which only
searches this.doc.blocks) so tables in header/footer will fail; update the
table-related methods to use the region-aware lookup by replacing calls to
findBlock with findBlockInAnyArray, or modify findBlock to delegate to
findBlockInAnyArray and return the block (and index) appropriately, and ensure
any table mutation code that expects the parent array uses the returned
blocks/index or explicitly rejects tables outside the body.
In `@packages/docs/src/view/doc-canvas.ts`:
- Around line 426-442: renderRunWithPageNumber currently only swaps the
displayed text but leaves layout geometry (run.width, charOffsets, and
subsequent run.x positions) based on the original placeholder, causing overlaps
and incorrect hit-testing; update this by performing a per-page layout pass for
the substituted text: create a new LayoutRun (or call the existing layout
routine used elsewhere) for the substituted string (using the same inline style)
to compute width and charOffsets, replace or use that computed run when calling
renderRun, and adjust any subsequent runs' x positions on the line based on the
new width so all geometry (width, charOffsets, and following run.x values)
matches the rendered page-number string. Ensure you reference
renderRunWithPageNumber and renderRun and reuse the same layout helper used for
normal runs so geometry and hit-testing remain correct.
In `@packages/docs/src/view/text-editor.ts`:
- Around line 2929-2937: The fallback currently always returns the end of the
last block (using hfLayout.blocks[hfLayout.blocks.length - 1]), which causes
clicks above the first header/footer line to jump to the document end; modify
the logic in the text-to-position resolution (in
packages/docs/src/view/text-editor.ts around the fallback block) to detect when
the computed localY is above the first rendered line (use hfLayout.blocks[0] /
firstBlock and its first line/runs) and clamp to the document start by returning
the first block's id with offset 0 and an appropriate lineAffinity (e.g.,
'forward'), otherwise keep the existing last-block fallback behavior.
- Around line 2858-2939: getHFPositionFromMouse currently uses run.text.length
to compute offsets and binary-search positions, which is incorrect for
page-number inlines whose rendered length varies per page; change the logic to
(1) record the clicked pageIndex when scanning paginatedLayout pages, (2) when
accumulating charsBefore and when returning offsets use the inline's
logical/token length (e.g., run.inline.logicalLength or run.inline.tokenLength)
instead of run.text.length so offsets reflect the document model, and (3) when
measuring widths for hit-testing keep using the rendered string for that
specific page (e.g., generate the rendered run string for the determined
pageIndex or call the renderer that substitutes page numbers) so the x-position
math remains accurate while the returned offset is based on the inline's logical
length (use run.inline and run.text only for rendering/measurement, but use
run.inline.logical length for offset arithmetic).
In `@packages/frontend/src/app/docs/yorkie-doc-store.ts`:
- Around line 792-798: findTableIndex currently only looks in document.body so
tables in header/footer are not found; update findTableIndex (used by
insertTableRow, deleteTableRow, updateTableCell) to locate the block across all
regions (body, header, footer) and compute the correct treeIdx using the same
region-specific offset logic as bodyTreeOffset/getDocument routing — e.g.,
detect which region contains the blockId, set bodyIdx to that region's block
index, and compute treeIdx by adding the region's tree offset (reuse or add a
header/footer offset helper analogous to bodyTreeOffset) so header/footer tables
are routed and mutated correctly.
---
Outside diff comments:
In `@packages/docs/src/model/document.ts`:
- Around line 354-400: The cross-block styling loop is reading
this._document.blocks[i] (mutating body) instead of the currently active region;
replace that direct access with a region-aware accessor (e.g. call a
region-aware helper like getBlockByIndex(i) or implement
getRegionBlockAtIndex(i) that returns the block from the active region) so the
loop uses that block instance for table handling, applyStyleToBlock,
store.updateBlock and store.updateTableCell; update all references in the loop
(block variable, block.id, cell.blocks iteration) to use the region-aware block
and ensure _blockParentMap/getBlockIndex usage remains unchanged.
In `@packages/docs/src/view/editor.ts`:
- Around line 811-834: The undoFn and redoFn move the cursor to
doc.document.blocks[0] but don’t update the editor’s current edit context,
causing header/footer mode to remain while the caret is in the body; after
calling docStore.undo()/redo(), doc.refresh(), and cursor.moveTo(...) update the
editor’s editing context to match the block you moved to (e.g., derive the
region from the target block and call the existing API that switches edit region
such as setCurrentEditContext / setEditingRegion / enterHeaderFooterMode as
appropriate) so the UI and caret are consistent; apply the same change to both
undoFn and redoFn and clear layoutCache/trigger render as before.
- Around line 472-494: The loop incorrectly dereferences doc.document.blocks[i],
which ignores header/footer context and causes multi-block ops to target body
blocks; instead resolve each block via the Doc API that respects context (use
the doc methods that return a block by id/index rather than direct array access)
— e.g., use the block id from
doc.getBlockIndex(range.anchor.blockId)/range.focus.blockId and call the
context-aware getter (doc.getBlock or doc.getBlockById/getBlockByIndex) for each
index, then pass that block into fn and markDirty(b.id) so header/footer
selections operate on the correct blocks.
In `@packages/docs/src/view/text-editor.ts`:
- Around line 79-124: The editContext flag was added but not propagated into
navigation and selection helpers; update handleDocStart, handleDocEnd,
selectAll, getSelectedBlocks, getVisualLineRange, and getPixelForPosition to use
getContextBlocks() instead of the body block list and to use
getHeaderLayout()/getFooterLayout() when this.getEditContext() !== 'body' (fall
back to this.getLayout()/this.getPaginatedLayout() for body). Ensure each helper
checks this.editContext (or calls getEditContext()), selects the appropriate
layout via getHeaderLayout/getFooterLayout when in header/footer mode, and
passes that layout plus the result of getContextBlocks() into any
layout-dependent calculations so cursor movement, Home/End, and selection
operate on the active context.
In `@packages/frontend/src/app/docs/yorkie-doc-store.ts`:
- Around line 423-426: getBlock currently only searches document.blocks and
therefore ignores header/footer regions; update getBlock (and use getDocument)
to perform a region-aware lookup like MemDocStore by checking header/footer (or
other region collections) in addition to document.blocks—e.g., search
document.headerBlocks, document.footerBlocks (or the document.regions/map) for a
block with the matching id and return it if found, falling back to
document.blocks to preserve existing behavior.
---
Nitpick comments:
In `@packages/docs/src/view/cursor.ts`:
- Around line 33-38: The JSDoc above isVisible() is inaccurate: update the
comment to describe visibility (e.g., "Return whether the cursor is visible.")
and attach it directly to the isVisible() method, and relocate the existing
pixel-coordinate/paginated description to the actual method that computes pixel
coordinates (the function that resolves cursor x/y for rendering) so the
pixel-resolution doc sits with its corresponding function.
In `@packages/docs/test/model/document.test.ts`:
- Around line 604-622: Add assertions in the two tests that verify the default
marginFromEdge value when ensureHeader() / ensureFooter() creates the
header/footer: after calling doc.ensureHeader() assert
doc.document.header!.marginFromEdge equals the expected default constant (e.g.,
DEFAULT_MARGIN or numeric value used by Doc), and after doc.ensureFooter()
assert doc.document.footer!.marginFromEdge equals that same default; reference
the ensureHeader, ensureFooter methods and the
document.header/footer.marginFromEdge property so the test enforces the
marginFromEdge contract.
In `@packages/docs/test/model/types.test.ts`:
- Around line 194-211: The test "should include header and footer in Document
type" sets footer.marginFromEdge but doesn't assert it; update the test to
include an explicit assertion for footer.marginFromEdge (e.g., add
expect(doc.footer!.marginFromEdge).toBe(48)) alongside the existing
header/footer checks so the Document shape contract for footer.marginFromEdge is
verified; locate the test case that constructs Document with createEmptyBlock()
and add the assertion there.
🪄 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: 3bce218b-90de-4ae6-9ac2-6fc968eebb06
📒 Files selected for processing (19)
docs/design/docs/docs-header-footer.mddocs/tasks/active/20260325-docs-wordprocessor-todo.mddocs/tasks/active/20260408-header-footer-todo.mdpackages/docs/src/index.tspackages/docs/src/model/document.tspackages/docs/src/model/types.tspackages/docs/src/store/memory.tspackages/docs/src/store/store.tspackages/docs/src/view/cursor.tspackages/docs/src/view/doc-canvas.tspackages/docs/src/view/editor.tspackages/docs/src/view/pagination.tspackages/docs/src/view/text-editor.tspackages/docs/src/view/theme.tspackages/docs/test/model/document.test.tspackages/docs/test/model/types.test.tspackages/docs/test/store/memory.test.tspackages/docs/test/view/pagination.test.tspackages/frontend/src/app/docs/yorkie-doc-store.ts
Comment on lines
+95
to
+100
| ``` | ||
| headerLayout = header ? computeLayout(header.blocks, ctx, contentWidth) : null | ||
| footerLayout = footer ? computeLayout(footer.blocks, ctx, contentWidth) : null | ||
| bodyLayout = computeLayout(doc.blocks, ctx, contentWidth) | ||
| paginated = paginateLayout(bodyLayout, pageSetup) | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (MD040).
These fences are currently unlabeled and trigger markdownlint warnings.
Proposed fix
-```
+```text
headerLayout = header ? computeLayout(header.blocks, ctx, contentWidth) : null
footerLayout = footer ? computeLayout(footer.blocks, ctx, contentWidth) : null
bodyLayout = computeLayout(doc.blocks, ctx, contentWidth)
paginated = paginateLayout(bodyLayout, pageSetup)
-```
+```
-```
+```text
for each page:
1. Viewport culling
2. Draw shadow + page background
3. [NEW] Clip header area → draw header runs
4. [NEW] Clip footer area → draw footer runs
5. Clip content area → draw selections, text, cursor (existing)
6. Draw peer cursors (existing)
-```
+```
-```
+```text
root
├── header (optional)
│ └── block[type=paragraph]
│ └── inline[style=...]
│ └── text
├── footer (optional)
│ └── block[type=paragraph]
│ └── inline[style=...]
│ └── text
└── body
└── block[type=paragraph]
└── ...
-```
+```Also applies to: 127-135, 262-275
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 95-95: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/docs/docs-header-footer.md` around lines 95 - 100, The fenced
code blocks are unlabeled and trigger MD040; update each backticked block
containing snippets (the blocks showing the
headerLayout/footerLayout/bodyLayout/paginated lines, the "for each page:"
steps, and the "root/ header/ footer/ body" tree) to include a language
identifier such as "text" (e.g., replace ``` with ```text) so markdownlint stops
flagging them; apply this change to the occurrences at the shown ranges
(including the other instances around the 127-135 and 262-275 sections).
|
|
||
| --- | ||
|
|
||
| ### Task 1: Data Model — HeaderFooter type, Document extension, pageNumber inline |
There was a problem hiding this comment.
Fix the heading level jump.
This starts at H3 immediately after the H1, which is what markdownlint is flagging here. Make it an H2 or insert the missing intermediate heading.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 15-15: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tasks/active/20260408-header-footer-todo.md` at line 15, The markdown
has a heading level jump: the "### Task 1: Data Model — HeaderFooter type,
Document extension, pageNumber inline" heading is H3 directly after the H1;
change that heading to H2 ("## Task 1: Data Model — HeaderFooter type, Document
extension, pageNumber inline") or add the missing intermediate H2 heading above
it so the document follows proper heading hierarchy and resolves the
markdownlint error.
Comment on lines
+426
to
+442
| private renderRunWithPageNumber( | ||
| run: LayoutRun, | ||
| lineX: number, | ||
| lineY: number, | ||
| lineHeight: number, | ||
| pageNumber: number, | ||
| ): void { | ||
| if (run.inline.style.pageNumber) { | ||
| const substituted = { | ||
| ...run, | ||
| text: String(pageNumber), | ||
| inline: { ...run.inline, text: String(pageNumber) }, | ||
| }; | ||
| this.renderRun(substituted, lineX, lineY, lineHeight); | ||
| } else { | ||
| this.renderRun(run, lineX, lineY, lineHeight); | ||
| } |
There was a problem hiding this comment.
Page-number substitution doesn't update layout geometry.
This only swaps the rendered text. run.width, charOffsets, and every following run's x still describe the one-character placeholder, so page 10+/100+ will overlap adjacent text and cursor/selection hit-testing stays wrong. This needs per-page layout against the substituted string, not just a paint-time text replacement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/docs/src/view/doc-canvas.ts` around lines 426 - 442,
renderRunWithPageNumber currently only swaps the displayed text but leaves
layout geometry (run.width, charOffsets, and subsequent run.x positions) based
on the original placeholder, causing overlaps and incorrect hit-testing; update
this by performing a per-page layout pass for the substituted text: create a new
LayoutRun (or call the existing layout routine used elsewhere) for the
substituted string (using the same inline style) to compute width and
charOffsets, replace or use that computed run when calling renderRun, and adjust
any subsequent runs' x positions on the line based on the new width so all
geometry (width, charOffsets, and following run.x values) matches the rendered
page-number string. Ensure you reference renderRunWithPageNumber and renderRun
and reuse the same layout helper used for normal runs so geometry and
hit-testing remain correct.
Comment on lines
+2858
to
+2939
| /** | ||
| * Resolve mouse position to a DocPosition within header/footer layout. | ||
| */ | ||
| private getHFPositionFromMouse(e: MouseEvent): (DocPosition & { lineAffinity: 'forward' | 'backward' }) | undefined { | ||
| const hfLayout = this.editContext === 'header' ? this.getHeaderLayout() : this.getFooterLayout(); | ||
| if (!hfLayout) return undefined; | ||
| const hf = this.editContext === 'header' ? this.doc.document.header : this.doc.document.footer; | ||
| if (!hf) return undefined; | ||
|
|
||
| const rect = this.container.getBoundingClientRect(); | ||
| const s = this.getScaleFactor(); | ||
| const canvasX = (e.clientX - rect.left + this.container.scrollLeft) / s; | ||
| const canvasY = (e.clientY - rect.top - this.getCanvasOffsetTop()) / s + this.container.scrollTop / s; | ||
|
|
||
| const paginatedLayout = this.getPaginatedLayout(); | ||
| const pageX = getPageXOffset(paginatedLayout, this.getCanvasWidth()); | ||
| const { margins } = paginatedLayout.pageSetup; | ||
|
|
||
| // Find which page was clicked to get the base Y | ||
| let baseY = 0; | ||
| for (const page of paginatedLayout.pages) { | ||
| const pageY = getPageYOffset(paginatedLayout, page.pageIndex); | ||
| if (canvasY >= pageY && canvasY <= pageY + page.height) { | ||
| if (this.editContext === 'header') { | ||
| baseY = getHeaderYStart(paginatedLayout, page.pageIndex, hf.marginFromEdge); | ||
| } else { | ||
| baseY = getFooterYStart(paginatedLayout, page.pageIndex, hfLayout.totalHeight, hf.marginFromEdge); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Convert to layout-local coordinates | ||
| const localX = canvasX - pageX - margins.left; | ||
| const localY = canvasY - baseY; | ||
| const ctx = this.getCtx(); | ||
|
|
||
| // Find the closest block and line | ||
| for (const lb of hfLayout.blocks) { | ||
| for (let li = 0; li < lb.lines.length; li++) { | ||
| const line = lb.lines[li]; | ||
| const lineTop = lb.y + line.y; | ||
| const lineBottom = lineTop + line.height; | ||
|
|
||
| if (localY >= lineTop && localY < lineBottom) { | ||
| // Find character offset within this line | ||
| let charsBefore = 0; | ||
| for (let k = 0; k < li; k++) { | ||
| for (const r of lb.lines[k].runs) charsBefore += r.text.length; | ||
| } | ||
| for (const run of line.runs) { | ||
| const runEnd = run.x + run.width; | ||
| if (localX <= runEnd) { | ||
| // Binary search within run for exact character | ||
| ctx.font = buildFont(run.inline.style.fontSize, run.inline.style.fontFamily, run.inline.style.bold, run.inline.style.italic); | ||
| let bestOffset = 0; | ||
| for (let c = 0; c <= run.text.length; c++) { | ||
| const w = ctx.measureText(run.text.slice(0, c)).width; | ||
| if (run.x + w > localX) break; | ||
| bestOffset = c; | ||
| } | ||
| return { blockId: lb.block.id, offset: charsBefore + bestOffset, lineAffinity: 'backward' as const }; | ||
| } | ||
| charsBefore += run.text.length; | ||
| } | ||
| // Past end of line | ||
| return { blockId: lb.block.id, offset: charsBefore, lineAffinity: 'backward' as const }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fallback: last block, end of text | ||
| const lastBlock = hfLayout.blocks[hfLayout.blocks.length - 1]; | ||
| if (lastBlock) { | ||
| let totalChars = 0; | ||
| for (const line of lastBlock.lines) { | ||
| for (const run of line.runs) totalChars += run.text.length; | ||
| } | ||
| return { blockId: lastBlock.block.id, offset: totalChars, lineAffinity: 'backward' as const }; | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Rendered page numbers will produce the wrong document offsets here.
getHFPositionFromMouse() accumulates offsets with run.text.length, but header/footer runs now include per-page page-number substitution. The rendered digit count is page-dependent, while the underlying token is a single logical inline, so clicks after a page-number token can return offsets that drift across pages and can exceed the real block length. This resolver needs to map back to logical inline offsets rather than rendered string length.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/docs/src/view/text-editor.ts` around lines 2858 - 2939,
getHFPositionFromMouse currently uses run.text.length to compute offsets and
binary-search positions, which is incorrect for page-number inlines whose
rendered length varies per page; change the logic to (1) record the clicked
pageIndex when scanning paginatedLayout pages, (2) when accumulating charsBefore
and when returning offsets use the inline's logical/token length (e.g.,
run.inline.logicalLength or run.inline.tokenLength) instead of run.text.length
so offsets reflect the document model, and (3) when measuring widths for
hit-testing keep using the rendered string for that specific page (e.g.,
generate the rendered run string for the determined pageIndex or call the
renderer that substitutes page numbers) so the x-position math remains accurate
while the returned offset is based on the inline's logical length (use
run.inline and run.text only for rendering/measurement, but use
run.inline.logical length for offset arithmetic).
Comment on lines
+792
to
798
| /** Returns body array index and tree-adjusted index for a table block. */ | ||
| private findTableIndex(tableBlockId: string): { bodyIdx: number; treeIdx: number } { | ||
| const currentDoc = this.getDocument(); | ||
| const index = currentDoc.blocks.findIndex((b) => b.id === tableBlockId); | ||
| if (index === -1) throw new Error(`Table block not found: ${tableBlockId}`); | ||
| return index; | ||
| return { bodyIdx: index, treeIdx: index + this.bodyTreeOffset(currentDoc) }; | ||
| } |
There was a problem hiding this comment.
Header/footer tables are still unreachable from the Yorkie table APIs.
findTableIndex() only resolves body blocks, so insertTableRow, deleteTableRow, updateTableCell, etc. still fail for tables living in a header or footer. With header/footer now using the normal block model, these mutations need the same region routing as the text/block APIs.
🤖 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 792 - 798,
findTableIndex currently only looks in document.body so tables in header/footer
are not found; update findTableIndex (used by insertTableRow, deleteTableRow,
updateTableCell) to locate the block across all regions (body, header, footer)
and compute the correct treeIdx using the same region-specific offset logic as
bodyTreeOffset/getDocument routing — e.g., detect which region contains the
blockId, set bodyIdx to that region's block index, and compute treeIdx by adding
the region's tree offset (reuse or add a header/footer offset helper analogous
to bodyTreeOffset) so header/footer tables are routed and mutated correctly.
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 (2)
packages/docs/src/view/editor.ts (1)
812-835:⚠️ Potential issue | 🟠 MajorClear the old selection when undo/redo exits header/footer editing.
Both handlers reset the context back to
'body'and re-home the caret, but they leaveselection.rangeuntouched. If the previous selection still points into a header/footer block, the UI shows a body cursor while later style commands keep mutating the hidden header/footer selection.Suggested fix
if (docStore.canUndo()) { docStore.undo(); doc.refresh(); textEditor?.setEditContext('body'); + selection.setRange(null); layoutCache = undefined; if (doc.document.blocks.length > 0) { cursor.moveTo({ blockId: doc.document.blocks[0].id, offset: 0 }); }if (docStore.canRedo()) { docStore.redo(); doc.refresh(); textEditor?.setEditContext('body'); + selection.setRange(null); layoutCache = undefined; if (doc.document.blocks.length > 0) { cursor.moveTo({ blockId: doc.document.blocks[0].id, offset: 0 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/editor.ts` around lines 812 - 835, Undo and redo handlers (undoFn and redoFn) switch edit context back to 'body' but do not clear the previous selection, which can point into header/footer blocks; update both handlers to explicitly clear the old selection (e.g., set selection.range = undefined or call selection.clear()) immediately after textEditor?.setEditContext('body') so the UI caret and subsequent style commands operate on the new body context; keep this change in undoFn and redoFn alongside the existing cursor.moveTo and render calls.packages/docs/src/model/document.ts (1)
240-259:⚠️ Potential issue | 🟠 MajorRoute header/footer split inserts through a region-aware store path.
When this branch runs in
'header'or'footer',blockIndexis relative to the active region, butthis.store.insertBlock(blockIndex + 1, newBlock)still inserts intodocument.blocksin the current stores. Pressing Enter after an HR/page-break in a header/footer will therefore create the new paragraph in the body instead of next to the current header/footer block. This needs a context-awareDocStoreinsert API before the model can treat header/footer as a true peer of the body.As per coding guidelines, "All document behavior must go through the
DocStoreinterface; do not bypass with ad-hoc persistence."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/model/document.ts` around lines 240 - 259, The insert is using this.store.insertBlock(blockIndex + 1, newBlock) which ignores header/footer region context and thus inserts into the body; change it to use the region-aware DocStore API (e.g. an insert that accepts a store path/region and index) so the new paragraph is inserted into the same region as the block. Replace the direct call to this.store.insertBlock in the horizontal-rule/page-break branch with the DocStore region-aware insert (for example this.store.insertBlockAtPath(regionPath, blockIndex + 1, newBlock) or equivalent), obtaining regionPath from the same context helpers (the logic that computes blockIndex / this.getContextBlocks or a getActiveRegionPath helper), and keep generateBlockId(), newBlock shape, this.refresh(), and other surrounding logic intact; do not bypass DocStore persistence.
♻️ Duplicate comments (1)
packages/frontend/src/app/docs/yorkie-doc-store.ts (1)
795-800:⚠️ Potential issue | 🟠 Major
findTableIndex()still can't see header/footer tables.This lookup only scans
currentDoc.blocks, soinsertTableRow,deleteTableRow,insertTableColumn,deleteTableColumn,updateTableCell, andupdateTableAttrsstill fail for a table stored indocument.header.blocksordocument.footer.blocks. Header/footer block editing is region-aware elsewhere now; the table APIs need the same region lookup and tree-index calculation.🤖 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 795 - 800, The findTableIndex method only searches getDocument().blocks and misses tables in header/footer; update findTableIndex(tableBlockId) to search document.header.blocks, document.footer.blocks and document.blocks, returning bodyIdx as the index within the matched region and treeIdx computed using bodyTreeOffset(currentDoc) plus the offset appropriate to the region (i.e., if header, treeIdx should be index; if body, treeIdx should be index + bodyTreeOffset(currentDoc); if footer, treeIdx should be index + bodyTreeOffset(currentDoc) + body length or whatever offset logic bodyTreeOffset implies). Ensure you still throw an error if not found and keep using getDocument() and bodyTreeOffset(currentDoc) for the offset calculation.
🧹 Nitpick comments (1)
packages/docs/src/view/editor.ts (1)
113-284: ThreadlineAffinityinto the new header/footer caret helpers.These helpers resolve visual positions from
(blockId, offset)alone, so an exact soft-wrap boundary is still ambiguous: the same offset can belong to either the previous or next visual line. That means header/footer caret/selection painting can land on the wrong line even if the logical cursor position is correct. The new path should carry the same wrap-disambiguation data as the body-side cursor/selection helpers.Based on learnings, "include and consistently thread
lineAffinitythrough the relevant selection/text-editor helpers ... so wrap-boundary offsets resolve to the correct visual line for Home/End and Cmd+Backspace navigation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/editor.ts` around lines 113 - 284, The header/footer caret and selection helpers (computeHFCursorPixel and computeHFSelectionRects) must thread a lineAffinity value through the DocPosition/selectionRange handling so soft-wrap boundary offsets resolve to the same visual line as the body helpers; update the signatures to accept position.lineAffinity and selectionRange.anchor/ focus with lineAffinity, use lineAffinity when deciding which line contains an offset (i.e., when offset equals a run/line boundary prefer previous or next line based on affinity) while computing cursorX/cursorLineY and when computing lineSelStart/lineSelEnd and run-local offsets (x0/x1), and ensure any startOffset/endOffset normalization preserves and applies lineAffinity when choosing lines to build layoutRects and mapping to pages.
🤖 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/editor.ts`:
- Around line 394-412: The header/footer layout is currently computed from
stored blocks containing the '#' page-number placeholder (see
headerLayout/footerLayout and computeLayout), causing incorrect metrics when
page numbers become multi-digit; update the logic to substitute the actual page
number text before measuring: for each page call the same substitution used by
DocCanvas.renderRunWithPageNumber (or at minimum measure the rendered width of
String(pageNumber) for the page and inject that width into the layout
calculation) and then call computeLayout per-page so headerLayout/footerLayout
reflect the true substituted text; ensure you reference the same token
replacement routine used by DocCanvas.renderRunWithPageNumber to keep
measurement and paint consistent.
---
Outside diff comments:
In `@packages/docs/src/model/document.ts`:
- Around line 240-259: The insert is using this.store.insertBlock(blockIndex +
1, newBlock) which ignores header/footer region context and thus inserts into
the body; change it to use the region-aware DocStore API (e.g. an insert that
accepts a store path/region and index) so the new paragraph is inserted into the
same region as the block. Replace the direct call to this.store.insertBlock in
the horizontal-rule/page-break branch with the DocStore region-aware insert (for
example this.store.insertBlockAtPath(regionPath, blockIndex + 1, newBlock) or
equivalent), obtaining regionPath from the same context helpers (the logic that
computes blockIndex / this.getContextBlocks or a getActiveRegionPath helper),
and keep generateBlockId(), newBlock shape, this.refresh(), and other
surrounding logic intact; do not bypass DocStore persistence.
In `@packages/docs/src/view/editor.ts`:
- Around line 812-835: Undo and redo handlers (undoFn and redoFn) switch edit
context back to 'body' but do not clear the previous selection, which can point
into header/footer blocks; update both handlers to explicitly clear the old
selection (e.g., set selection.range = undefined or call selection.clear())
immediately after textEditor?.setEditContext('body') so the UI caret and
subsequent style commands operate on the new body context; keep this change in
undoFn and redoFn alongside the existing cursor.moveTo and render calls.
---
Duplicate comments:
In `@packages/frontend/src/app/docs/yorkie-doc-store.ts`:
- Around line 795-800: The findTableIndex method only searches
getDocument().blocks and misses tables in header/footer; update
findTableIndex(tableBlockId) to search document.header.blocks,
document.footer.blocks and document.blocks, returning bodyIdx as the index
within the matched region and treeIdx computed using bodyTreeOffset(currentDoc)
plus the offset appropriate to the region (i.e., if header, treeIdx should be
index; if body, treeIdx should be index + bodyTreeOffset(currentDoc); if footer,
treeIdx should be index + bodyTreeOffset(currentDoc) + body length or whatever
offset logic bodyTreeOffset implies). Ensure you still throw an error if not
found and keep using getDocument() and bodyTreeOffset(currentDoc) for the offset
calculation.
---
Nitpick comments:
In `@packages/docs/src/view/editor.ts`:
- Around line 113-284: The header/footer caret and selection helpers
(computeHFCursorPixel and computeHFSelectionRects) must thread a lineAffinity
value through the DocPosition/selectionRange handling so soft-wrap boundary
offsets resolve to the same visual line as the body helpers; update the
signatures to accept position.lineAffinity and selectionRange.anchor/ focus with
lineAffinity, use lineAffinity when deciding which line contains an offset
(i.e., when offset equals a run/line boundary prefer previous or next line based
on affinity) while computing cursorX/cursorLineY and when computing
lineSelStart/lineSelEnd and run-local offsets (x0/x1), and ensure any
startOffset/endOffset normalization preserves and applies lineAffinity when
choosing lines to build layoutRects and mapping to pages.
🪄 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: c086d3d2-cdfd-4c15-a38b-9054bebd67d9
📒 Files selected for processing (7)
packages/docs/src/model/document.tspackages/docs/src/view/cursor.tspackages/docs/src/view/editor.tspackages/docs/src/view/text-editor.tspackages/docs/test/model/document.test.tspackages/docs/test/model/types.test.tspackages/frontend/src/app/docs/yorkie-doc-store.ts
✅ Files skipped from review due to trivial changes (2)
- packages/docs/src/view/cursor.ts
- packages/docs/test/model/document.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/docs/test/model/types.test.ts
- packages/docs/src/view/text-editor.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/docs/src/view/text-editor.ts (1)
2861-2943:⚠️ Potential issue | 🟠 MajorUse logical offsets, not rendered page-number text length.
getHFPositionFromMouse()still does its offset math withrun.text.length. That breaks as soon as a header/footer run contains the new page-number token, because the rendered digit count varies by page while the document model only has one logical inline/token there. Clicks after the token can drift past the real block offset and misplace the cursor/selection on later pages. Keeprun.textfor width measurement, but basecharsBeforeand returned offsets on the logical inline length instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 2861 - 2943, getHFPositionFromMouse currently accumulates character offsets using run.text.length which counts rendered characters (breaks with page-number tokens); change the offset math to use the logical inline length on the model (e.g., use run.inline.length or the model's logical-length property) while still using run.text for width measurement and ctx.measureText; update every place that adds run.text.length to charsBefore and totalChars and the returned offset to use the inline logical length instead (keep run.text for computing run widths in the binary search).
🤖 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/text-editor.ts`:
- Around line 98-99: Add a route-through helper that returns the active layout
(body vs header/footer) and use it everywhere visual-line and page-line
navigation depends on layout: implement a method like getActiveLayout() that
checks getHeaderLayout()/getFooterLayout() and falls back to getLayout(), then
replace direct calls to this.getLayout() inside getVisualLineRange(),
getPixelForPosition(), moveVertical() (and the similar logic around lines
121-124) to call this.getActiveLayout() so Home/End/Cmd+Backspace and
ArrowUp/ArrowDown resolve against the correct header/footer/body layout.
- Around line 848-851: When switching out of header/footer mode (the branch that
calls setEditContext('body') when target === 'body' and this.editContext !==
'body'), clear any stale selection state so old header/footer anchors/ranges
can't persist; explicitly reset the selection anchor/range (e.g.
this.selectionAnchor = null; this.selectionRange = null or call an existing
this.clearSelection()/resetSelectionState()) and/or mark the next click as a
plain click so the first body click doesn't try to reuse the previous context.
This ensures downstream logic (findIndex, the header/footer delete/merge path)
only sees anchors in the active editContext and prevents mixed-context ranges.
---
Duplicate comments:
In `@packages/docs/src/view/text-editor.ts`:
- Around line 2861-2943: getHFPositionFromMouse currently accumulates character
offsets using run.text.length which counts rendered characters (breaks with
page-number tokens); change the offset math to use the logical inline length on
the model (e.g., use run.inline.length or the model's logical-length property)
while still using run.text for width measurement and ctx.measureText; update
every place that adds run.text.length to charsBefore and totalChars and the
returned offset to use the inline logical length instead (keep run.text for
computing run widths in the binary search).
🪄 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: cfd30948-73af-4ac0-b7da-09a0371679fb
📒 Files selected for processing (2)
packages/docs/src/store/memory.tspackages/docs/src/view/text-editor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/docs/src/store/memory.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/docs/src/view/text-editor.ts (3)
98-99:⚠️ Potential issue | 🟠 MajorRoute
getPixelForPositionandmoveVerticalthrough the active layout.The
getActiveLayout()helper was added and is used bygetVisualLineRange(), butgetPixelForPosition()(line 3217) still usesthis.getLayout()directly. WhilemoveVerticalhas an early return for header/footer contexts (line 2694),getPixelForPositionis also called bygetLinkAtCursorPosition()and other methods that could be invoked while editing header/footer, potentially returning incorrect pixel coordinates.Consider updating
getPixelForPositionto usegetActiveLayout()or add appropriate guards.Also applies to: 117-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 98 - 99, The getPixelForPosition method currently calls this.getLayout() and can return wrong coordinates when the editor is in header/footer mode; update getPixelForPosition to route through the existing getActiveLayout() helper (the same helper used by getVisualLineRange()) and fall back to null/early-return if no active layout is returned, so callers like getLinkAtCursorPosition and moveVertical receive correct coordinates; similarly ensure moveVertical’s header/footer early-return logic remains consistent with getActiveLayout behavior and replace any remaining direct this.getLayout() uses (or add guards) in the same file to prevent header/footer miscalculations.
848-851:⚠️ Potential issue | 🟠 MajorClear stale selection state when leaving header/footer mode.
When clicking the body to exit header/footer editing, the selection is not cleared. This leaves a stale header/footer selection range that could cause issues with subsequent operations like Shift+click extending selection across contexts.
🛠️ Proposed fix
if (target === 'body' && this.editContext !== 'body') { // Click on body exits header/footer editing this.setEditContext('body'); + this.selection.setRange(null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 848 - 851, When switching out of header/footer mode in the branch that checks if (target === 'body' && this.editContext !== 'body'), after calling this.setEditContext('body') also clear any stale selection state belonging to the header/footer context so it cannot be reused (e.g. call this.clearSelection() or this.setSelection(null) or reset this.selectionRange/headerFooterSelection to null). Locate the selection state variables or helper methods (names like selection, selectionRange, currentSelection, clearSelection, setSelection) in the same class and reset them immediately after this.setEditContext('body') so Shift+click or other selection operations don't extend a stale range.
2917-2937:⚠️ Potential issue | 🟠 MajorPage-number tokens cause offset drift in
getHFPositionFromMouse.The offset accumulation uses
run.text.length(the rendered string length), but for page-number inlines, the rendered length varies per page (e.g., "1" vs "10" vs "100") while the logical model has a fixed token length. Clicks after a page-number token will compute incorrect offsets on multi-digit pages.The resolution requires mapping rendered positions back to logical inline offsets rather than using the rendered string length.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 2917 - 2937, In getHFPositionFromMouse, the code accumulates offsets using the rendered run.text.length which drifts for page-number inlines; instead compute and use the inline's logical/model length when updating charsBefore and map measured rendered character indices back to logical offsets. Replace uses of run.text.length (and the return computation charsBefore + bestOffset) with the inline/model-aware values: consult run.inline (e.g., a tokenLength or logicalLength property or a render-to-model offset mapper like run.inline.mapRenderedIndexToLogicalIndex) to increment charsBefore and to convert bestOffset (the measured rendered index) into the corresponding logical offset before returning { blockId: lb.block.id, offset: ... , lineAffinity: 'backward' }. Ensure the per-run binary/linear search measures text width as before but translates rendered indices to model indices via run.inline mapping rather than assuming 1:1 with run.text.length.
🤖 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/text-editor.ts`:
- Around line 2146-2157: The cursor positioning can pick a deleted block because
findIndex may return -1 for focus after a multi-block merge; update the logic
around this.selection.range.anchor/focus and cursorTarget so missing blocks are
handled: call this.doc.getContextBlocks(), compute aIdx and fIdx, then if either
index is -1 fall back to the other existing block's id (e.g., if fIdx === -1 use
a.blockId, if aIdx === -1 use f.blockId); otherwise keep the existing aIdx <=
fIdx comparison to choose between { blockId: a.blockId, offset: a.offset } and {
blockId: f.blockId, offset: f.offset }, then call
this.cursor.moveTo(cursorTarget).
---
Duplicate comments:
In `@packages/docs/src/view/text-editor.ts`:
- Around line 98-99: The getPixelForPosition method currently calls
this.getLayout() and can return wrong coordinates when the editor is in
header/footer mode; update getPixelForPosition to route through the existing
getActiveLayout() helper (the same helper used by getVisualLineRange()) and fall
back to null/early-return if no active layout is returned, so callers like
getLinkAtCursorPosition and moveVertical receive correct coordinates; similarly
ensure moveVertical’s header/footer early-return logic remains consistent with
getActiveLayout behavior and replace any remaining direct this.getLayout() uses
(or add guards) in the same file to prevent header/footer miscalculations.
- Around line 848-851: When switching out of header/footer mode in the branch
that checks if (target === 'body' && this.editContext !== 'body'), after calling
this.setEditContext('body') also clear any stale selection state belonging to
the header/footer context so it cannot be reused (e.g. call
this.clearSelection() or this.setSelection(null) or reset
this.selectionRange/headerFooterSelection to null). Locate the selection state
variables or helper methods (names like selection, selectionRange,
currentSelection, clearSelection, setSelection) in the same class and reset them
immediately after this.setEditContext('body') so Shift+click or other selection
operations don't extend a stale range.
- Around line 2917-2937: In getHFPositionFromMouse, the code accumulates offsets
using the rendered run.text.length which drifts for page-number inlines; instead
compute and use the inline's logical/model length when updating charsBefore and
map measured rendered character indices back to logical offsets. Replace uses of
run.text.length (and the return computation charsBefore + bestOffset) with the
inline/model-aware values: consult run.inline (e.g., a tokenLength or
logicalLength property or a render-to-model offset mapper like
run.inline.mapRenderedIndexToLogicalIndex) to increment charsBefore and to
convert bestOffset (the measured rendered index) into the corresponding logical
offset before returning { blockId: lb.block.id, offset: ... , lineAffinity:
'backward' }. Ensure the per-run binary/linear search measures text width as
before but translates rendered indices to model indices via run.inline mapping
rather than assuming 1:1 with run.text.length.
🪄 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: 2e6baf4d-e1ff-4a7c-9a8e-f45aa4906aba
📒 Files selected for processing (1)
packages/docs/src/view/text-editor.ts
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/docs/src/view/text-editor.ts (3)
2163-2174:⚠️ Potential issue | 🔴 CriticalCursor target may reference a deleted block after multi-block merge.
After a multi-block deletion that merges blocks (line 2158), the focus block may no longer exist. The
findIndexon line 2171 will return-1for the deleted block, and the comparisonaIdx <= fIdx(line 2172) will incorrectly evaluate, causingcursorTargetto reference the non-existent focus block.🐛 Proposed fix
const blocks = this.doc.getContextBlocks(); const aIdx = blocks.findIndex((b) => b.id === a.blockId); const fIdx = blocks.findIndex((b) => b.id === f.blockId); - cursorTarget = aIdx <= fIdx ? { blockId: a.blockId, offset: a.offset } : { blockId: f.blockId, offset: f.offset }; + // After merge, the later block may not exist; use the surviving block + if (fIdx === -1) { + cursorTarget = { blockId: a.blockId, offset: a.offset }; + } else if (aIdx === -1) { + cursorTarget = { blockId: f.blockId, offset: f.offset }; + } else { + cursorTarget = aIdx <= fIdx ? { blockId: a.blockId, offset: a.offset } : { blockId: f.blockId, offset: f.offset }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 2163 - 2174, After multi-block deletions the focus block may have been removed so computing aIdx/fIdx via this.doc.getContextBlocks().findIndex can return -1 and lead cursorTarget to reference a non-existent block; update the logic around this.selection.range.anchor/focus to first resolve aIdx and fIdx, check for -1, and if either index is -1 fallback to the remaining valid block (or clamp to a nearest valid index in the blocks array) before choosing cursorTarget, then call this.cursor.moveTo with that validated blockId and offset; use the existing symbols this.selection.range.anchor, this.selection.range.focus, this.doc.getContextBlocks(), findIndex results, and this.cursor.moveTo when implementing the guard and fallback.
865-868:⚠️ Potential issue | 🟠 MajorClear stale selection state when exiting header/footer mode via body click.
When clicking on the body to exit header/footer editing, the selection is not cleared. The existing selection may still reference header/footer blocks, which can cause issues in subsequent operations that assume the selection is within the active context.
🐛 Proposed fix
if (target === 'body' && this.editContext !== 'body') { // Click on body exits header/footer editing this.setEditContext('body'); + this.selection.setRange(null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 865 - 868, When handling a body click that switches context from header/footer to body (the if block that checks target === 'body' and calls this.setEditContext('body')), also clear any stale selection so it doesn't still reference header/footer blocks; update the block to call the editor's selection-clearing API (e.g. call this.clearSelection() or assign this.selection = null / this.setSelection(null) immediately after this.setEditContext('body')) so selection state is reset when leaving header/footer mode.
2934-2951:⚠️ Potential issue | 🟠 MajorPage-number token offsets may drift across pages.
The offset calculation in
getHFPositionFromMouseusesrun.text.lengthwhich reflects the rendered page number (e.g., "10" = 2 chars on page 10). However, a page-number token should have a constant logical length (1) in the document model. Clicks after a page-number token will return incorrect offsets that vary by page, causing cursor positioning inconsistencies.This needs to map back to logical inline offsets rather than rendered string lengths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 2934 - 2951, The click-to-offset code in getHFPositionFromMouse is using run.text.length for logical offsets, which causes page-number tokens to drift; change both places that add run.text.length (the charsBefore accumulation before the loop and inside the loop after measuring) to use the run's logical/inline length instead (e.g., run.inline.length or run.inline.tokenLength), and when mapping the measured character index bestOffset back to a document offset convert the rendered-character index into the run's logical offset (for example by summing logical lengths of the run's character/token units up to bestOffset, treating a page-number token as length 1); update the return to use charsBefore + that logical offset so cursor positions are stable across pages.
🧹 Nitpick comments (1)
packages/docs/src/view/text-editor.ts (1)
845-862: Clarify the double-click behavior for entering header/footer mode.The first double-click on an inactive header/footer region enters edit mode and positions the cursor at offset 0, without performing word selection. This is a deliberate UX choice. Subsequent double-clicks within the active region will perform normal word selection.
Consider adding a brief comment explaining this intentional two-stage behavior for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 845 - 862, The double-click handling intentionally uses a two-stage UX: the first double-click on an inactive header/footer (checked via clickCount, target, and editContext) enters edit mode, sets hfActivePageIndex, calls ensureHeader/ensureFooter, setEditContext('header'|'footer') and moves the cursor to offset 0 without selecting a word, while subsequent double-clicks in the active region perform normal word selection; add a concise inline comment above this block (referencing clickCount, target, editContext, hfActivePageIndex, ensureHeader/ensureFooter, setEditContext, cursor.moveTo) explaining this deliberate behavior so future maintainers understand why the first double-click positions the caret instead of selecting text.
🤖 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/docs/src/view/text-editor.ts`:
- Around line 2163-2174: After multi-block deletions the focus block may have
been removed so computing aIdx/fIdx via this.doc.getContextBlocks().findIndex
can return -1 and lead cursorTarget to reference a non-existent block; update
the logic around this.selection.range.anchor/focus to first resolve aIdx and
fIdx, check for -1, and if either index is -1 fallback to the remaining valid
block (or clamp to a nearest valid index in the blocks array) before choosing
cursorTarget, then call this.cursor.moveTo with that validated blockId and
offset; use the existing symbols this.selection.range.anchor,
this.selection.range.focus, this.doc.getContextBlocks(), findIndex results, and
this.cursor.moveTo when implementing the guard and fallback.
- Around line 865-868: When handling a body click that switches context from
header/footer to body (the if block that checks target === 'body' and calls
this.setEditContext('body')), also clear any stale selection so it doesn't still
reference header/footer blocks; update the block to call the editor's
selection-clearing API (e.g. call this.clearSelection() or assign this.selection
= null / this.setSelection(null) immediately after this.setEditContext('body'))
so selection state is reset when leaving header/footer mode.
- Around line 2934-2951: The click-to-offset code in getHFPositionFromMouse is
using run.text.length for logical offsets, which causes page-number tokens to
drift; change both places that add run.text.length (the charsBefore accumulation
before the loop and inside the loop after measuring) to use the run's
logical/inline length instead (e.g., run.inline.length or
run.inline.tokenLength), and when mapping the measured character index
bestOffset back to a document offset convert the rendered-character index into
the run's logical offset (for example by summing logical lengths of the run's
character/token units up to bestOffset, treating a page-number token as length
1); update the return to use charsBefore + that logical offset so cursor
positions are stable across pages.
---
Nitpick comments:
In `@packages/docs/src/view/text-editor.ts`:
- Around line 845-862: The double-click handling intentionally uses a two-stage
UX: the first double-click on an inactive header/footer (checked via clickCount,
target, and editContext) enters edit mode, sets hfActivePageIndex, calls
ensureHeader/ensureFooter, setEditContext('header'|'footer') and moves the
cursor to offset 0 without selecting a word, while subsequent double-clicks in
the active region perform normal word selection; add a concise inline comment
above this block (referencing clickCount, target, editContext,
hfActivePageIndex, ensureHeader/ensureFooter, setEditContext, cursor.moveTo)
explaining this deliberate behavior so future maintainers understand why the
first double-click positions the caret instead of selecting text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d10eac4c-35e4-4e68-a824-122c1f31d7fa
📒 Files selected for processing (2)
packages/docs/src/view/editor.tspackages/docs/src/view/text-editor.ts
✅ Files skipped from review due to trivial changes (1)
- packages/docs/src/view/editor.ts
5 tasks
hackerwins
added a commit
that referenced
this pull request
Apr 11, 2026
DOCX import/export shipped in PR #114 (merged to main as bbce164 on 2026-04-11) as part of v0.3.2. The 20260410-docx-import-export-todo.md plan was the execution blueprint for that work — every task from Phase 1 prerequisite features through Phase 4 frontend integration exists in the codebase, but only the last five checkboxes were ever ticked because the execution pipeline ran end-to-end faster than the manual checkbox update. The plan is accurate in spirit; mark all items complete and move it to archive/2026/04/. Also regenerate docs/tasks/archive/README.md via `pnpm tasks:index`. The index picks up two entries that were physically archived in PR #110 (header/footer and page break task plans) but never made it into the README because the index regenerator wasn't run at that time. Restoring them now keeps the published archive list consistent with the filesystem. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
This was referenced Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pageNumber: true) substituted per page during renderingChanges
HeaderFootertype,Document.header/footer,InlineStyle.pageNumberDocStore/MemDocStorewithfindBlockInAnyArrayfor cross-region block operationsEditContext,getContextBlocks(),ensureHeader/Footer()getHeaderYStart/getFooterYStart,resolveClickTargetbodyTreeOffsetfor tree path correctionTest plan
pnpm --filter @wafflebase/docs test)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Persistence & Undo
Style