Add word-processor-style pagination to docs editor#59
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Replace pagePaddingX/pagePaddingTop with page rendering constants (pageGap, shadow, canvasBackground). Reset layout origin to (0,0) so the pagination engine can position pages independently. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add paginateLayout() that splits a continuous DocumentLayout into discrete pages based on PageSetup dimensions and margins. Handles page overflow, marginTop suppression at page tops, landscape orientation, and oversized lines. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Rewrite DocCanvas for paginated page-by-page rendering with shadows, gaps, and content-area clipping. Update editor.ts to derive content width from PageSetup and run paginateLayout. Update cursor, selection, and text-editor to use paginated coordinate mapping. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add pagination types (PageSetup, PaginatedLayout, LayoutPage, PageLine) and functions to index.ts exports. Remove positionToPixel, pixelToPosition, and findLayoutBlock from layout.ts — these are replaced by paginated equivalents in pagination.ts.
Pre-populate the demo with 16 paragraphs including a styled title to demonstrate multi-page rendering with shadows, gaps, and content-area clipping.
When the cursor moves outside the visible viewport (e.g. via arrow keys, typing, or Enter), the container now scrolls to keep it in view with a 20px margin.
Cursor and selection rects are computed in absolute document coordinates but the page rendering applies -scrollY to page positions. Apply the same scroll offset to cursor.y and selection rect.y so they stay aligned with the text.
The canvas is full document height inside a scrollable container, so the container natively handles scroll offset. The renderer was also subtracting scrollY from page positions, causing a double- offset: clicks mapped to pre-scroll positions and the cursor drifted on scroll. Now all drawing uses absolute coordinates. scrollY is only used for viewport culling (skip off-screen pages) and auto-scroll.
When moving vertically at a page boundary, the calculated target Y coordinate falls into the page gap/margin area (232px total), which maps back to the current line. Detect this case and jump to the first/last line of the adjacent page instead. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Caution Review failedPull request was closed or merged during review 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:
📝 WalkthroughWalkthroughImplements word-processor-style pagination for a Canvas-based document editor. Introduces PageSetup data model with paper sizes and margins, adds a pagination engine that splits layout lines across pages, refactors rendering to draw centered pages with drop shadows and gaps, and replaces coordinate mapping with page-aware position/pixel conversion functions. Changes
Sequence DiagramsequenceDiagram
participant TextEditor
participant Editor
participant Layout
participant Pagination
participant DocCanvas
TextEditor->>Editor: content changed / scroll event
Editor->>Layout: computeLayout(blocks, ctx, contentWidth)
activate Layout
Layout->>Layout: walk blocks, measure text, wrap lines
Layout-->>Editor: DocumentLayout
deactivate Layout
Editor->>Pagination: paginateLayout(layout, pageSetup)
activate Pagination
Pagination->>Pagination: split layout lines into pages<br/>apply margins & gaps
Pagination-->>Editor: PaginatedLayout
deactivate Pagination
Editor->>DocCanvas: render(paginatedLayout, scrollY, canvasWidth, ...)
activate DocCanvas
DocCanvas->>DocCanvas: for each visible page
DocCanvas->>DocCanvas: draw page background & shadow
DocCanvas->>DocCanvas: clip to page content area
DocCanvas->>DocCanvas: render text runs & selection
DocCanvas->>DocCanvas: render cursor if visible
DocCanvas-->>TextEditor: canvas pixels
deactivate DocCanvas
TextEditor->>Pagination: paginatedPixelToPosition(paginatedLayout, px, py, ...)
activate Pagination
Pagination->>Pagination: find clicked page<br/>resolve line & character offset
Pagination-->>TextEditor: {blockId, offset}
deactivate Pagination
TextEditor->>TextEditor: update cursor position<br/>trigger undo/redo/styling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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 |
Contributor
Verification: verify:selfResult: ✅ PASS in 89.0s
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: 3
🧹 Nitpick comments (9)
docs/tasks/active/20260321-docs-pagination-todo.md (1)
1275-1280: Minor: Adverb repetition in verification checklist.Lines 1276-1277 use "correctly" twice in adjacent bullet points. Consider varying the wording for better readability.
- Text wraps correctly within page content area -- Cursor positions correctly across pages +- Cursor positions accurately across pages - Selection highlights work across page boundaries🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/tasks/active/20260321-docs-pagination-todo.md` around lines 1275 - 1280, Two adjacent checklist bullets repeat the adverb "correctly" ("Text wraps correctly within page content area" and "Cursor positions correctly across pages"); change one to avoid repetition — for example, update "Text wraps correctly within page content area" to "Text wraps properly within the page content area" or "Text wraps as expected within the page content area" (or similarly rephrase the "Cursor positions..." bullet) so the checklist reads more varied and natural.packages/docs/src/store/memory.ts (1)
65-67: Consider returning a clonedPageSetupfor consistency with other getters.
getDocument()andgetBlock()return deep clones to prevent external mutation of internal state. However,getPageSetup()returns thepageSetupobject directly (viaresolvePageSetup) when it exists, which could allow callers to mutate the store's internal state.♻️ Proposed fix for defensive cloning
getPageSetup(): PageSetup { - return resolvePageSetup(this.doc.pageSetup); + const setup = resolvePageSetup(this.doc.pageSetup); + return JSON.parse(JSON.stringify(setup)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/store/memory.ts` around lines 65 - 67, getPageSetup currently returns the resolved PageSetup directly which permits external mutation; update the getPageSetup method to return a deep clone of the resolved page setup (matching the defensive cloning used by getDocument and getBlock). Locate getPageSetup and call the same deep-clone utility used elsewhere in this file (e.g., the project's deepClone/cloneDeep/structuredClone helper) on the value returned by resolvePageSetup(this.doc.pageSetup) before returning it so callers cannot mutate internal store state.packages/docs/test/view/pagination.test.ts (1)
151-189: Add a page-2 position-mapping regression case.These assertions only pin
findPageForPosition()to page 1 andpaginatedPixelToPosition()near the first page/gap. A regression in split-block offsets on page 2 would still slip through, even though cross-page cursor movement is one of the main behaviors in this PR.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/view/pagination.test.ts` around lines 151 - 189, Add a regression test that verifies mapping behavior on the second page: create a DocumentLayout (using mockBlock/mockLine) whose totalHeight forces content onto page 2 via paginateLayout with DEFAULT_PAGE_SETUP, then assert findPageForPosition returns pageIndex === 1 for a position located on the split block on page 2 and assert paginatedPixelToPosition maps a pixel coordinate near the second page's content/gap back to the correct blockId; use the existing helper names (findPageForPosition, paginatedPixelToPosition, paginateLayout, DEFAULT_PAGE_SETUP, mockBlock, mockLine, DocumentLayout) so the test catches regressions in split-block offsets across pages.packages/docs/src/view/selection.ts (2)
230-230: Hardcoded fallback height should use a theme constant.The fallback height of
24is a magic number. Consider usingTheme.defaultFontSize * 1.5or another derived constant for consistency with the rest of the theming system.♻️ Suggested fix
- return { x: pageX + pageLine.x, y: pageY + pageLine.y, height: 24 }; + return { x: pageX + pageLine.x, y: pageY + pageLine.y, height: Theme.defaultFontSize * 1.5 };This would require importing
Themefrom./theme.js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/selection.ts` at line 230, Replace the hardcoded fallback height (24) in the object returned from the function that computes selection rects so it uses the theme constant instead; import Theme from "./theme.js" and change the height to Theme.defaultFontSize * 1.5 (or another derived Theme constant) where the return currently reads { x: pageX + pageLine.x, y: pageY + pageLine.y, height: 24 } so the layout uses themed font sizing.
183-231: Code duplication withTextEditor.getPixelForPosition.This
positionToPagePixelimplementation is nearly identical togetPixelForPositionintext-editor.ts(lines 592-630). Consider extracting this shared logic into a single exported helper function inpagination.tsto improve maintainability.🤖 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 183 - 231, positionToPagePixel duplicates the logic in TextEditor.getPixelForPosition; extract the shared computation into a single exported helper (e.g., computePixelForPosition or getPixelForPositionOnPage) in pagination.ts and have both positionToPagePixel and TextEditor.getPixelForPosition call it. The helper should accept the same inputs used here (PaginatedLayout, DocumentLayout, CanvasRenderingContext2D, canvasWidth, blockId, offset) and return { x, y, height } | undefined, encapsulating finding the page (findPageForPosition), accumulating charsBeforeLine, iterating runs to compute localOff, setting ctx.font via buildFont, measuring text width, and the end-of-line fallback; update imports and remove the duplicated code blocks in positionToPagePixel and getPixelForPosition so both delegate to the new exported helper in pagination.ts.packages/docs/src/view/doc-canvas.ts (1)
98-104: Cursor visibility check may exclude valid cursor positions at the bottom edge.The cursor visibility condition checks
cursor.y < pageY + margins.top + contentHeight, which uses a strict less-than comparison. If the cursor is positioned exactly at the bottom edge of the content area, it would be excluded. Consider using<=for the upper bound to include cursors at the exact boundary.♻️ Suggested fix
if (cursor?.visible) { if (cursor.y >= pageY + margins.top && - cursor.y < pageY + margins.top + contentHeight) { + cursor.y <= pageY + margins.top + contentHeight - cursor.height) { this.ctx.fillStyle = Theme.cursorColor; this.ctx.fillRect(cursor.x, cursor.y, Theme.cursorWidth, cursor.height); }🤖 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 98 - 104, The visibility check for the cursor in render logic (the block referencing cursor, pageY, margins.top, and contentHeight) uses a strict upper-bound comparison ("cursor.y < pageY + margins.top + contentHeight") which excludes a cursor exactly on the bottom edge; change the condition to use a non-strict comparison (<=) for the upper bound so the code that sets this.ctx.fillStyle = Theme.cursorColor and calls this.ctx.fillRect(cursor.x, cursor.y, Theme.cursorWidth, cursor.height) will draw cursors positioned exactly at the bottom boundary.packages/docs/src/view/pagination.ts (2)
236-248: Inconsistent character count sources.Line 247 uses
run.text.lengthwhile other parts of the codebase (e.g., line 230, 252) userun.charEnd - run.charStart. While these should be equivalent, using the latter consistently would be safer and more aligned with the data model.♻️ Suggested fix
- charsBeforeRun += run.text.length; + charsBeforeRun += run.charEnd - run.charStart;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/pagination.ts` around lines 236 - 248, The code uses run.text.length in several places but the rest of the module uses the canonical character-count fields run.charEnd - run.charStart; replace all uses of run.text.length in this block (the charWidth calculation, clampedOffset bounds, and the charsBeforeRun increment) with (run.charEnd - run.charStart) so charWidth = run.width / Math.max(1, run.charEnd - run.charStart), clamp against run.charEnd - run.charStart, and increment charsBeforeRun by run.charEnd - run.charStart to keep character counting consistent with the data model.
147-160: Edge case:targetLineIndexnot updated correctly when offset is past all lines.When the
offsetis beyond all characters in the block (e.g., cursor at end of block), the loop completes without breaking, andtargetLineIndexends up beinglb.lines.length - 1(due to line 159). However, line 159 setstargetLineIndex = lion every iteration, so after the loop, it holds the last valid index. This works, but assigning unconditionally inside the loop is slightly confusing.Consider restructuring for clarity:
♻️ Suggested improvement
let charCount = 0; - let targetLineIndex = 0; + let targetLineIndex = lb.lines.length - 1; // Default to last line for (let li = 0; li < lb.lines.length; li++) { const lineChars = lb.lines[li].runs.reduce( (sum, r) => sum + (r.charEnd - r.charStart), 0, ); if (charCount + lineChars >= offset) { targetLineIndex = li; break; } charCount += lineChars; - targetLineIndex = li; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/pagination.ts` around lines 147 - 160, The current loop updates targetLineIndex unconditionally on each iteration which is confusing and risks misinterpretation when offset is past all characters; update the logic in the loop that iterates over lb.lines (variables: charCount, targetLineIndex, offset, li) so that targetLineIndex is only set when the offset falls within the current line (inside the if branch), and after the loop set targetLineIndex = Math.max(0, lb.lines.length - 1) when no break occurred (i.e., offset beyond all lines) to clearly and deterministically pick the last line.packages/docs/src/view/text-editor.ts (1)
592-630: Duplicate logic withSelection.positionToPagePixel.As noted in
selection.ts, thisgetPixelForPositionimplementation duplicates the logic. Consider consolidating into a shared helper.The hardcoded fallback height
24on line 629 should also be derived from theme constants.🤖 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 592 - 630, getPixelForPosition duplicates Selection.positionToPagePixel logic and uses a hardcoded fallback height 24; extract the shared coordinate-calculation into a single helper (e.g., positionToPagePixel or positionToPixel) that both getPixelForPosition and Selection.positionToPagePixel call, reusing existing utilities used here (findPageForPosition, getPaginatedLayout, getLayout, getCtx, getPageXOffset/getPageYOffset) to compute pageX/pageY/char measurement, and replace the literal 24 with the theme constant (e.g., theme.lineHeight or THEME.DEFAULT_LINE_HEIGHT) so the fallback height comes from the theme. Ensure the new helper returns {x,y,height} and update both call sites to use it.
🤖 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/model/types.ts`:
- Around line 178-180: resolvePageSetup currently returns the shared
DEFAULT_PAGE_SETUP reference when setup is undefined, allowing callers to mutate
process-wide defaults; change resolvePageSetup to return a copy instead (e.g.,
shallow or deep clone depending on PageSetup's depth) so callers receive an
independent PageSetup object; locate resolvePageSetup and DEFAULT_PAGE_SETUP in
types.ts and replace the direct return of DEFAULT_PAGE_SETUP with a cloned
object (e.g., {...DEFAULT_PAGE_SETUP} or a deep clone utility) so mutations to
the resolved setup do not affect the global default.
In `@packages/docs/src/view/cursor.ts`:
- Around line 73-81: The empty-line fallback uses a hard-coded height of 24
causing incorrect cursor sizing; update the fallback return in the cursor
calculation (the branch after checking lastRun in cursor code referencing
pageLine, pageX, pageY, this.visible) to use the computed line height
pageLine.line.height instead of 24 so empty paragraphs respect
fontSize/lineHeight and scrolling bounds are correct.
In `@packages/docs/src/view/editor.ts`:
- Around line 74-80: The canvas and all page-coordinate math must use the
resolved page width instead of the viewport: use
resolvePageSetup(doc.document.pageSetup) and getEffectiveDimensions(pageSetup)
to derive the page width (and contentWidth) and set the docCanvas size and any
layout/hit-testing/cursor/selection calculations to that width; update the
computeLayout call (and other places noted around lines ~96-128 and ~168-169) to
pass the page/content width and ensure docCanvas.getContext() drawing, hitTest,
and cursor positioning functions consume the same pageWidth/contentWidth so
coordinates are computed against the true page size rather than the viewport.
---
Nitpick comments:
In `@docs/tasks/active/20260321-docs-pagination-todo.md`:
- Around line 1275-1280: Two adjacent checklist bullets repeat the adverb
"correctly" ("Text wraps correctly within page content area" and "Cursor
positions correctly across pages"); change one to avoid repetition — for
example, update "Text wraps correctly within page content area" to "Text wraps
properly within the page content area" or "Text wraps as expected within the
page content area" (or similarly rephrase the "Cursor positions..." bullet) so
the checklist reads more varied and natural.
In `@packages/docs/src/store/memory.ts`:
- Around line 65-67: getPageSetup currently returns the resolved PageSetup
directly which permits external mutation; update the getPageSetup method to
return a deep clone of the resolved page setup (matching the defensive cloning
used by getDocument and getBlock). Locate getPageSetup and call the same
deep-clone utility used elsewhere in this file (e.g., the project's
deepClone/cloneDeep/structuredClone helper) on the value returned by
resolvePageSetup(this.doc.pageSetup) before returning it so callers cannot
mutate internal store state.
In `@packages/docs/src/view/doc-canvas.ts`:
- Around line 98-104: The visibility check for the cursor in render logic (the
block referencing cursor, pageY, margins.top, and contentHeight) uses a strict
upper-bound comparison ("cursor.y < pageY + margins.top + contentHeight") which
excludes a cursor exactly on the bottom edge; change the condition to use a
non-strict comparison (<=) for the upper bound so the code that sets
this.ctx.fillStyle = Theme.cursorColor and calls this.ctx.fillRect(cursor.x,
cursor.y, Theme.cursorWidth, cursor.height) will draw cursors positioned exactly
at the bottom boundary.
In `@packages/docs/src/view/pagination.ts`:
- Around line 236-248: The code uses run.text.length in several places but the
rest of the module uses the canonical character-count fields run.charEnd -
run.charStart; replace all uses of run.text.length in this block (the charWidth
calculation, clampedOffset bounds, and the charsBeforeRun increment) with
(run.charEnd - run.charStart) so charWidth = run.width / Math.max(1, run.charEnd
- run.charStart), clamp against run.charEnd - run.charStart, and increment
charsBeforeRun by run.charEnd - run.charStart to keep character counting
consistent with the data model.
- Around line 147-160: The current loop updates targetLineIndex unconditionally
on each iteration which is confusing and risks misinterpretation when offset is
past all characters; update the logic in the loop that iterates over lb.lines
(variables: charCount, targetLineIndex, offset, li) so that targetLineIndex is
only set when the offset falls within the current line (inside the if branch),
and after the loop set targetLineIndex = Math.max(0, lb.lines.length - 1) when
no break occurred (i.e., offset beyond all lines) to clearly and
deterministically pick the last line.
In `@packages/docs/src/view/selection.ts`:
- Line 230: Replace the hardcoded fallback height (24) in the object returned
from the function that computes selection rects so it uses the theme constant
instead; import Theme from "./theme.js" and change the height to
Theme.defaultFontSize * 1.5 (or another derived Theme constant) where the return
currently reads { x: pageX + pageLine.x, y: pageY + pageLine.y, height: 24 } so
the layout uses themed font sizing.
- Around line 183-231: positionToPagePixel duplicates the logic in
TextEditor.getPixelForPosition; extract the shared computation into a single
exported helper (e.g., computePixelForPosition or getPixelForPositionOnPage) in
pagination.ts and have both positionToPagePixel and
TextEditor.getPixelForPosition call it. The helper should accept the same inputs
used here (PaginatedLayout, DocumentLayout, CanvasRenderingContext2D,
canvasWidth, blockId, offset) and return { x, y, height } | undefined,
encapsulating finding the page (findPageForPosition), accumulating
charsBeforeLine, iterating runs to compute localOff, setting ctx.font via
buildFont, measuring text width, and the end-of-line fallback; update imports
and remove the duplicated code blocks in positionToPagePixel and
getPixelForPosition so both delegate to the new exported helper in
pagination.ts.
In `@packages/docs/src/view/text-editor.ts`:
- Around line 592-630: getPixelForPosition duplicates
Selection.positionToPagePixel logic and uses a hardcoded fallback height 24;
extract the shared coordinate-calculation into a single helper (e.g.,
positionToPagePixel or positionToPixel) that both getPixelForPosition and
Selection.positionToPagePixel call, reusing existing utilities used here
(findPageForPosition, getPaginatedLayout, getLayout, getCtx,
getPageXOffset/getPageYOffset) to compute pageX/pageY/char measurement, and
replace the literal 24 with the theme constant (e.g., theme.lineHeight or
THEME.DEFAULT_LINE_HEIGHT) so the fallback height comes from the theme. Ensure
the new helper returns {x,y,height} and update both call sites to use it.
In `@packages/docs/test/view/pagination.test.ts`:
- Around line 151-189: Add a regression test that verifies mapping behavior on
the second page: create a DocumentLayout (using mockBlock/mockLine) whose
totalHeight forces content onto page 2 via paginateLayout with
DEFAULT_PAGE_SETUP, then assert findPageForPosition returns pageIndex === 1 for
a position located on the split block on page 2 and assert
paginatedPixelToPosition maps a pixel coordinate near the second page's
content/gap back to the correct blockId; use the existing helper names
(findPageForPosition, paginatedPixelToPosition, paginateLayout,
DEFAULT_PAGE_SETUP, mockBlock, mockLine, DocumentLayout) so the test catches
regressions in split-block offsets across pages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3bde9676-c952-40cb-a2e6-5322979cdc40
📒 Files selected for processing (20)
docs/design/README.mddocs/design/docs-pagination.mddocs/design/docs.mddocs/tasks/active/20260321-docs-pagination-todo.mdpackages/docs/demo.tspackages/docs/src/index.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/layout.tspackages/docs/src/view/pagination.tspackages/docs/src/view/selection.tspackages/docs/src/view/text-editor.tspackages/docs/src/view/theme.tspackages/docs/test/model/types.test.tspackages/docs/test/store/memory.test.tspackages/docs/test/view/pagination.test.ts
When dragging near the top or bottom edge of the viewport, scroll the container proportionally to the distance from the edge. Uses a requestAnimationFrame loop for smooth continuous scrolling while the mouse button is held. Co-Authored-By: Claude Opus 4.6 <[email protected]>
- resolvePageSetup now returns a shallow copy to prevent callers from mutating the shared DEFAULT_PAGE_SETUP object - Use pageLine.line.height instead of hard-coded 24 for empty-line cursor fallback Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/docs/src/model/types.ts (2)
178-185: HardenresolvePageSetupfor partial nested input.This currently assumes
paperSizeandmarginsare always fully present. A defensive merge with defaults makes runtime behavior safer for migrated or malformed persisted payloads.🛡️ Proposed hardening
export function resolvePageSetup(setup: PageSetup | undefined): PageSetup { - const resolved = setup ?? DEFAULT_PAGE_SETUP; return { - paperSize: { ...resolved.paperSize }, - orientation: resolved.orientation, - margins: { ...resolved.margins }, + paperSize: { + ...DEFAULT_PAGE_SETUP.paperSize, + ...(setup?.paperSize ?? {}), + }, + orientation: setup?.orientation ?? DEFAULT_PAGE_SETUP.orientation, + margins: { + ...DEFAULT_PAGE_SETUP.margins, + ...(setup?.margins ?? {}), + }, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/model/types.ts` around lines 178 - 185, resolvePageSetup assumes nested objects paperSize and margins are fully present; harden it by performing a defensive merge with DEFAULT_PAGE_SETUP for those nested fields. In resolvePageSetup, after computing resolved = setup ?? DEFAULT_PAGE_SETUP, build paperSize as { ...DEFAULT_PAGE_SETUP.paperSize, ...resolved.paperSize } and margins as { ...DEFAULT_PAGE_SETUP.margins, ...resolved.margins } while keeping orientation from resolved, so partially-provided PageSetup values inherit missing nested properties from DEFAULT_PAGE_SETUP.
166-170: Prefersatisfiesoveras PaperSizeassertions for preset typing.Using assertion casts here is less safe than a
satisfiesconstraint, which preserves literal inference and catches shape drift at compile time.♻️ Proposed refactor
export const PAPER_SIZES = { - LETTER: { name: 'Letter', width: 816, height: 1056 } as PaperSize, - A4: { name: 'A4', width: 794, height: 1123 } as PaperSize, - LEGAL: { name: 'Legal', width: 816, height: 1344 } as PaperSize, -} as const; + LETTER: { name: 'Letter', width: 816, height: 1056 }, + A4: { name: 'A4', width: 794, height: 1123 }, + LEGAL: { name: 'Legal', width: 816, height: 1344 }, +} as const satisfies Record<'LETTER' | 'A4' | 'LEGAL', PaperSize>;TypeScript 5.9.3 supports the
satisfiesoperator. This aligns with using type annotations to improve code clarity and catch errors early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/model/types.ts` around lines 166 - 170, The PAPER_SIZES object uses unsafe "as PaperSize" assertions; replace those per-entry assertions with a single TypeScript "satisfies" constraint so the object keeps literal inference while still being checked against the PaperSize shape. Update the declaration of PAPER_SIZES to keep the "as const" literal narrowing but apply "satisfies" with an appropriate Record (or specific keys) of PaperSize (referencing PAPER_SIZES and PaperSize) so any shape drift is caught at compile time.
🤖 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/model/types.ts`:
- Around line 178-185: resolvePageSetup assumes nested objects paperSize and
margins are fully present; harden it by performing a defensive merge with
DEFAULT_PAGE_SETUP for those nested fields. In resolvePageSetup, after computing
resolved = setup ?? DEFAULT_PAGE_SETUP, build paperSize as {
...DEFAULT_PAGE_SETUP.paperSize, ...resolved.paperSize } and margins as {
...DEFAULT_PAGE_SETUP.margins, ...resolved.margins } while keeping orientation
from resolved, so partially-provided PageSetup values inherit missing nested
properties from DEFAULT_PAGE_SETUP.
- Around line 166-170: The PAPER_SIZES object uses unsafe "as PaperSize"
assertions; replace those per-entry assertions with a single TypeScript
"satisfies" constraint so the object keeps literal inference while still being
checked against the PaperSize shape. Update the declaration of PAPER_SIZES to
keep the "as const" literal narrowing but apply "satisfies" with an appropriate
Record (or specific keys) of PaperSize (referencing PAPER_SIZES and PaperSize)
so any shape drift is caught at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a900a43-9bcb-4521-ac37-d463f5f9fbb2
📒 Files selected for processing (2)
packages/docs/src/model/types.tspackages/docs/src/view/cursor.ts
Two scenarios for the canvas-based document editor: multi-page pagination with text flow across page boundaries, and styled text with inline formatting and block alignment. Adds @wafflebase/docs as a frontend dependency. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Frontend now depends on @wafflebase/docs, so CI must build the docs package before the frontend build step. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The 'waffledocs' alias is not committed yet; use the full pnpm --filter command instead. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Build library packages before browser tests in Docker. Register docs scenario IDs and wait for docs section ready state. Regenerate all visual baselines with the new document editor scenarios. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Replace mismatched baselines with actual output from CI Docker environment to fix font rendering differences between local Apple Silicon QEMU emulation and CI native amd64. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/frontend/src/app/harness/visual/docs-scenarios.tsx (1)
170-173: Bubble upScenarioState, not just a boolean.
onReadyChangecollapsesloadinganderrorinto the samefalsevalue. Once a card hitserror, the parent section can only keepdata-visual-docs-ready="false", so the browser harness falls back to a timeout instead of surfacing the actual initialization failure. Pass the fullScenarioStateup here and expose an aggregate section state.Also applies to: 179-181, 251-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/harness/visual/docs-scenarios.tsx` around lines 170 - 173, Change the onReadyChange handler to pass the full ScenarioState instead of a boolean so callers can distinguish loading vs error; update the prop type from onReadyChange: (id: string, ready: boolean) => void to onReadyChange: (id: string, state: ScenarioState) => void, and change all places that call onReadyChange inside this file (the component handling DocsScenario and any child card initializers) to produce and forward the appropriate ScenarioState object (e.g., { loading: boolean, error?: Error | string, ready?: boolean }) so the parent/section can compute an aggregate section state and expose it (ensure names like onReadyChange, ScenarioState, DocsScenario and the parent section component that consumes the callback are updated accordingly).
🤖 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/frontend/src/app/harness/visual/docs-scenarios.tsx`:
- Around line 170-173: Change the onReadyChange handler to pass the full
ScenarioState instead of a boolean so callers can distinguish loading vs error;
update the prop type from onReadyChange: (id: string, ready: boolean) => void to
onReadyChange: (id: string, state: ScenarioState) => void, and change all places
that call onReadyChange inside this file (the component handling DocsScenario
and any child card initializers) to produce and forward the appropriate
ScenarioState object (e.g., { loading: boolean, error?: Error | string, ready?:
boolean }) so the parent/section can compute an aggregate section state and
expose it (ensure names like onReadyChange, ScenarioState, DocsScenario and the
parent section component that consumes the callback are updated accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba991f13-7d5c-469a-9aba-de3b575d43cc
⛔ Files ignored due to path filters (13)
packages/frontend/tests/visual/baselines/harness-visual.browser.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.docs-multi-page.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.docs-multi-page.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.docs-multi-page.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.docs-multi-page.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.docs-styled-text.desktop.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.docs-styled-text.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.docs-styled-text.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.docs-styled-text.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.mobile.dark.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.mobile.pngis excluded by!**/*.pngpackages/frontend/tests/visual/baselines/harness-visual.browser.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
packages/docs/README.mdpackages/documentation/README.mdpackages/frontend/package.jsonpackages/frontend/scripts/verify-visual-browser.mjspackages/frontend/src/app/harness/visual/docs-scenarios.tsxpackages/frontend/src/app/harness/visual/page.tsxscripts/verify-browser-lanes.mjsscripts/verify-self.mjs
✅ Files skipped from review due to trivial changes (3)
- packages/frontend/package.json
- packages/documentation/README.md
- packages/docs/README.md
Canvas width is now Math.max(viewportWidth, pageWidth) so the page is never clipped when the viewport is narrower than the paper size. All coordinate math (cursor, selection, hit-testing) uses the same canvas width. Also fix resolvePageSetup tests for defensive copy. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Stop cursor blink and render selection in inactive gray on blur. Restore cursor blink and active blue selection on focus. Adds selectionColorInactive to Theme and focused flag to DocCanvas. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The docs editor visual scenarios cause persistent baseline mismatches between local Docker (aarch64 QEMU) and CI (native amd64) due to Canvas font rendering differences. Remove them from the visual test harness to unblock CI. The docs package remains fully functional — visual regression coverage can be added later with a dedicated docs-specific harness. Co-Authored-By: Claude Opus 4.6 <[email protected]>
hackerwins
added a commit
that referenced
this pull request
Mar 21, 2026
Add page-based layout to the Canvas document editor, splitting content into discrete pages with configurable paper size, margins, and orientation. Key changes: - Data model: PageSetup (paper size, orientation, margins) with Letter/A4/Legal presets, stored in DocStore - Pagination engine: line-level page splitting with overflow to next page, page gap/shadow rendering - Coordinate mapping: bidirectional conversion between paginated pixel coordinates and document positions - Cursor: cross-page arrow key movement, auto-scroll to keep cursor visible, blink stops on blur - Selection: page-aware selection rects, inactive gray on blur - Mouse: click/drag across pages, auto-scroll during drag near container edges - Canvas sizing: Math.max(viewport, pageWidth) prevents clipping on narrow viewports - Build pipeline: sheets build step added to verify-browser-lanes for Docker-based visual tests - README.md added for docs and documentation packages Co-authored-by: Claude Opus 4.6 <[email protected]>
This was referenced Mar 22, 2026
10 tasks
This was referenced Apr 5, 2026
6 tasks
This was referenced May 3, 2026
4 tasks
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
PageSetupdata model (paper size, orientation, margins) with Letter/A4/Legal presetspaginateLayout()engine that splits continuous document layout into discrete pages at line boundariesChanges
packages/docs/src/view/pagination.ts— pagination engine withpaginateLayout(), coordinate mapping helperspackages/docs/src/view/doc-canvas.ts— page-based rendering with shadow, gap, viewport cullingpackages/docs/src/view/editor.ts— wire pagination into render pipeline, auto-scrollpackages/docs/src/view/text-editor.ts— paginated coordinate mapping, cross-page cursor movementpackages/docs/src/view/cursor.ts,selection.ts— page-aware pixel positionspackages/docs/src/model/types.ts—PageSetup,PageMargins,PaperSizetypesdocs/design/docs-pagination.md— design documentTest plan
packages/docs/test/view/pagination.test.ts)packages/docs/test/model/types.test.ts)packages/docs/test/store/memory.test.ts)pnpm verify:fastpasses (lint + all unit tests)pnpm verify:selfpasses (builds + entropy check)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests