Add docs mobile zoom-to-fit for narrow viewports#90
Merged
Conversation
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Wire computeScaleFactor through the entire rendering pipeline: compute logical canvas width, scale scroll positions, hide rulers when scaled, pass scaleFactor to DocCanvas.render and TextEditor, and convert between physical and logical coordinates for cursor positioning. Co-Authored-By: Claude Opus 4.6 <[email protected]>
When scaleFactor < 1, canvas width should match the viewport width instead of the page width. The page is already scaled to fit via ctx.scale(), so the canvas only needs to be as wide as the viewport. This ensures the scaled page is centered with MOBILE_PADDING on each side.
📝 WalkthroughWalkthroughAdds a mobile "zoom-to-fit" feature for the Canvas-based document editor: new scale utilities (MOBILE_PADDING, computeScaleFactor), scaled rendering via DocCanvas.render(scaleFactor), editor-level scaling of spacers/scroll/coordinates, TextEditor hit-test inversion, and ruler hide/show controls; accompanied by docs and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor (editor.ts)
participant DocCanvas as DocCanvas (doc-canvas.ts)
participant CanvasCtx as Canvas Context
participant TextEditor as TextEditor (text-editor.ts)
Editor->>Editor: computeScaleFactor(containerWidth, pageWidth)
Editor->>Editor: adjust spacer/scroll geometry using scaleFactor
Editor->>Editor: if scaleFactor < 1 then ruler.hide(), canvas.style.top = 0
Editor->>DocCanvas: render(..., scaleFactor)
DocCanvas->>CanvasCtx: ctx.save()
DocCanvas->>CanvasCtx: ctx.scale(scaleFactor, scaleFactor)
DocCanvas->>CanvasCtx: ctx.translate(...) and draw pages
DocCanvas->>CanvasCtx: ctx.restore()
Note over TextEditor: User mouse/touch input
TextEditor->>TextEditor: s = getScaleFactor()
TextEditor->>TextEditor: divide mouse x,y and scroll by s
TextEditor->>Editor: convert to document position (unscaled)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 101.4s
Verification: verify:integrationResult: ✅ PASS |
The getCanvasWidth callback for TextEditor was using max(viewportWidth, pageWidth) when scaled, producing a logical canvas width of ~1943px. But paint() uses viewportWidth (~375px) as the physical canvas width, giving a logical width of ~893px. This mismatch caused getPageXOffset to compute different page positions for rendering vs hit-testing, so clicks landed far from the expected document position.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/docs/src/view/editor.ts (1)
456-468:⚠️ Potential issue | 🔴 CriticalApply
scaleFactorto link-popover rect coordinates in viewport conversion.
getLinkAtCursorPosition()returns unscaled logical document coordinates (confirmed by its comment and implementation usinggetPixelForPosition()). The conversion at lines 456–468 mixes these logical coordinates with CSS-pixel viewport coordinates fromgetBoundingClientRect(). WhenscaleFactor≠ 1, this causes a unit mismatch. The rect values must either be multiplied byscaleFactorbefore adding tocanvasRectoffsets, orcanvasRectmust be divided byscaleFactorto work in logical space:const s = textEditorRef.getScaleFactor(); // Add getter if not exposed const scaledRect = { x: linkInfo.rect.x * s, y: linkInfo.rect.y * s, width: linkInfo.rect.width * s, height: linkInfo.rect.height * s, }; cursorLinkChangeCallback({ href: linkInfo.href, rect: { x: canvasRect.left + scaledRect.x, y: canvasRect.top + (scaledRect.y - scrollY), ... }, });Alternatively, convert
canvasRectandscrollYto logical space by dividing byscaleFactor. Either approach aligns with the scale-aware coordinate conversions elsewhere (e.g.,updateDragSelection,getPositionFromMouse).🤖 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 456 - 468, The link-popover rect is mixing unscaled logical coordinates from textEditorRef.getLinkAtCursorPosition() with CSS pixels from canvas.getBoundingClientRect(), so multiply linkInfo.rect (x, y, width, height) by the editor scaleFactor (obtainable via textEditorRef.getScaleFactor() or a new getter) before adding canvasRect offsets and applying scrollY, or alternatively convert canvasRect and scrollY into logical space by dividing by scaleFactor; update the code around cursorLinkChangeCallback handling (where getLinkAtCursorPosition() is used) to perform this scale-aware conversion so the returned rect is in viewport pixels consistent with canvasRect.
🧹 Nitpick comments (3)
packages/docs/test/view/scale.test.ts (1)
28-30: Strengthen the zero-width edge-case assertion.This currently only checks positivity. Please also assert finiteness and upper bound to catch
Infinity/NaNregressions early.Suggested test hardening
it('handles zero container width', () => { - expect(computeScaleFactor(0, PAGE_WIDTH)).toBeGreaterThan(0); + const factor = computeScaleFactor(0, PAGE_WIDTH); + expect(factor).toBeGreaterThan(0); + expect(factor).toBeLessThanOrEqual(1); + expect(Number.isFinite(factor)).toBe(true); });As per coding guidelines, "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/scale.test.ts` around lines 28 - 30, The test in scale.test.ts only checks computeScaleFactor(0, PAGE_WIDTH) > 0, which misses Infinity/NaN regressions; update the test to capture the result (e.g., const scale = computeScaleFactor(0, PAGE_WIDTH)) and assert Number.isFinite(scale), expect(scale).toBeGreaterThan(0), and assert an explicit reasonable upper bound (e.g., expect(scale).toBeLessThan(1e9)) to guard against Infinity or runaway values; reference computeScaleFactor and PAGE_WIDTH when making the change.packages/docs/src/view/text-editor.ts (1)
736-740: Add defensive scale-factor clamping before division.Both paths divide by
sdirectly. A transient0/non-finite value from upstream state would produce invalid coordinates and unstable selection behavior.Defensive clamp suggestion
- const s = this.getScaleFactor(); + const s = Math.max(Number.EPSILON, this.getScaleFactor());Also applies to: 1758-1761
🤖 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 736 - 740, Clamp the scale factor returned by getScaleFactor() before using it for division to avoid divide-by-zero or non-finite values; capture it into s, validate it (e.g., if !isFinite(s) || s === 0 then set s = a small positive epsilon like 1e-6 or fallback to 1), and then compute x, y, and scrollY using that clamped s so paginatedPixelToPosition receives valid coordinates; update both occurrences (the block using getScaleFactor() around variables s, x, y, scrollY and the similar code at lines ~1758-1761) and ensure downstream math uses the clamped s.docs/design/docs-mobile-zoom-to-fit.md (1)
43-45: Align design formula with zero-width guard behavior.The pseudocode currently divides by
pageWidthunconditionally. It should document the same guard used in implementation (pageWidth <= 0 => 1) to avoid ambiguous/invalid math in the spec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/docs-mobile-zoom-to-fit.md` around lines 43 - 45, The pseudocode for scaleFactor computes pageWidth and then divides by it unconditionally; update the doc so pageWidth uses the same zero-width guard as the implementation (e.g., set pageWidth = paginatedLayout.pages[0]?.width ?? 0 and then replace any direct use with a guarded value where pageWidth <= 0 yields 1) and then compute scaleFactor with that guarded denominator (referencing pageWidth, scaleFactor, paginatedLayout.pages[0]?.width, and MOBILE_PADDING in the description).
🤖 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-mobile-zoom-to-fit.md`:
- Around line 40-60: The markdown fenced code blocks in the doc file lack
language identifiers which trips markdownlint MD040; update each fenced block
shown (the blocks containing the MOBILE_PADDING/scaleFactor snippet, the canvas
render snippet using ctx.save/ctx.scale/ctx.translate, and the other code
examples around getScaleFactor, scaled spacer height, and ruler.show/hide) by
adding appropriate language tags (e.g., ```text, ```typescript) so all fenced
code blocks are annotated and MD040 passes; ensure you keep the existing block
contents and only add the language identifiers.
In `@docs/tasks/active/20260328-docs-mobile-zoom-to-fit-todo.md`:
- Line 1: Create the missing paired lessons file named
20260328-docs-mobile-zoom-to-fit-lessons.md to accompany
20260328-docs-mobile-zoom-to-fit-todo.md; in that file include a concise summary
of what was implemented, key design decisions, challenges encountered (e.g.,
handling viewport scaling, pinch vs double-tap, performance tradeoffs), the
final implementation steps or code references, and any follow-up todos or
testing notes so the task pair follows the project's paired-file guideline.
In `@packages/docs/src/view/doc-canvas.ts`:
- Around line 86-89: In DocCanvas.render(), the viewport culling mixes physical
and logical coordinates: when scaleFactor != 1 you must convert viewportHeight
into logical coordinates before computing visibleBottom; update the calculation
that derives visibleBottom (used alongside scrollY) to use viewportHeight /
scaleFactor so visibleBottom = scrollY + viewportHeight / scaleFactor, ensuring
culling uses the same logical coordinate space as scrollY and scaleFactor.
In `@packages/docs/src/view/editor.ts`:
- Around line 485-489: The width conversion currently uses vw/pw/physical and
Math.max which selects the unscaled page width; instead compute the rendered
canvas width the same way paint() does (use the canvasWidth calculation that
factors in scaleFactor and viewport width) and return that value converted back
to logical space; update the helper that computes physical/returned width (the
code using vw, pw, physical and scaleFactor) to reuse the canvasWidth logic from
paint() and have getCursorScreenRect() (and the duplicated search-path copy)
call that shared helper so all callers share the same rendered/canvas coordinate
space.
In `@packages/docs/src/view/text-editor.ts`:
- Around line 1758-1761: getLinkHrefAtMouse is still being hit-tested with raw
client coordinates causing Ctrl/Cmd+click to target wrong links when zoomed;
convert to using the inverted/scaled coordinates (use the computed s, x, y,
scrollY values from getScaleFactor(), container.scrollLeft/Top and
getCanvasOffsetTop()) before calling getLinkHrefAtMouse (or update
getLinkHrefAtMouse to accept scaled coords) so hit-testing matches the
selection/caret mapping; reference getLinkHrefAtMouse, getScaleFactor,
getCanvasOffsetTop, container.scrollLeft/scrollTop, and the local x/y/scrollY
variables when making the change.
---
Outside diff comments:
In `@packages/docs/src/view/editor.ts`:
- Around line 456-468: The link-popover rect is mixing unscaled logical
coordinates from textEditorRef.getLinkAtCursorPosition() with CSS pixels from
canvas.getBoundingClientRect(), so multiply linkInfo.rect (x, y, width, height)
by the editor scaleFactor (obtainable via textEditorRef.getScaleFactor() or a
new getter) before adding canvasRect offsets and applying scrollY, or
alternatively convert canvasRect and scrollY into logical space by dividing by
scaleFactor; update the code around cursorLinkChangeCallback handling (where
getLinkAtCursorPosition() is used) to perform this scale-aware conversion so the
returned rect is in viewport pixels consistent with canvasRect.
---
Nitpick comments:
In `@docs/design/docs-mobile-zoom-to-fit.md`:
- Around line 43-45: The pseudocode for scaleFactor computes pageWidth and then
divides by it unconditionally; update the doc so pageWidth uses the same
zero-width guard as the implementation (e.g., set pageWidth =
paginatedLayout.pages[0]?.width ?? 0 and then replace any direct use with a
guarded value where pageWidth <= 0 yields 1) and then compute scaleFactor with
that guarded denominator (referencing pageWidth, scaleFactor,
paginatedLayout.pages[0]?.width, and MOBILE_PADDING in the description).
In `@packages/docs/src/view/text-editor.ts`:
- Around line 736-740: Clamp the scale factor returned by getScaleFactor()
before using it for division to avoid divide-by-zero or non-finite values;
capture it into s, validate it (e.g., if !isFinite(s) || s === 0 then set s = a
small positive epsilon like 1e-6 or fallback to 1), and then compute x, y, and
scrollY using that clamped s so paginatedPixelToPosition receives valid
coordinates; update both occurrences (the block using getScaleFactor() around
variables s, x, y, scrollY and the similar code at lines ~1758-1761) and ensure
downstream math uses the clamped s.
In `@packages/docs/test/view/scale.test.ts`:
- Around line 28-30: The test in scale.test.ts only checks computeScaleFactor(0,
PAGE_WIDTH) > 0, which misses Infinity/NaN regressions; update the test to
capture the result (e.g., const scale = computeScaleFactor(0, PAGE_WIDTH)) and
assert Number.isFinite(scale), expect(scale).toBeGreaterThan(0), and assert an
explicit reasonable upper bound (e.g., expect(scale).toBeLessThan(1e9)) to guard
against Infinity or runaway values; reference computeScaleFactor and PAGE_WIDTH
when making the change.
🪄 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: 7f0b3269-8e2d-4ea5-a1d5-554b2203aaea
📒 Files selected for processing (9)
docs/design/docs-mobile-zoom-to-fit.mddocs/tasks/active/20260328-docs-mobile-zoom-to-fit-todo.mdpackages/docs/src/index.tspackages/docs/src/view/doc-canvas.tspackages/docs/src/view/editor.tspackages/docs/src/view/ruler.tspackages/docs/src/view/scale.tspackages/docs/src/view/text-editor.tspackages/docs/test/view/scale.test.ts
| @@ -0,0 +1,555 @@ | |||
| # Docs Mobile Zoom-to-Fit Implementation Plan | |||
There was a problem hiding this comment.
Add the paired -lessons.md file for this active task.
This TODO appears without a corresponding lessons file in the same task pair.
As per coding guidelines, "For non-trivial tasks, use paired files in docs/tasks/active/: YYYYMMDD-<slug>-todo.md and YYYYMMDD-<slug>-lessons.md."
If you want, I can draft a starter 20260328-docs-mobile-zoom-to-fit-lessons.md based on this implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tasks/active/20260328-docs-mobile-zoom-to-fit-todo.md` at line 1, Create
the missing paired lessons file named
20260328-docs-mobile-zoom-to-fit-lessons.md to accompany
20260328-docs-mobile-zoom-to-fit-todo.md; in that file include a concise summary
of what was implemented, key design decisions, challenges encountered (e.g.,
handling viewport scaling, pinch vs double-tap, performance tradeoffs), the
final implementation steps or code references, and any follow-up todos or
testing notes so the task pair follows the project's paired-file guideline.
There was a problem hiding this comment.
Actionable comments posted: 5
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/doc-canvas.ts (1)
82-95:⚠️ Potential issue | 🔴 CriticalUse logical viewport height for page culling.
scrollYis already converted back to document-space, butvisibleBottomstill adds the physicalviewportHeight. In zoom-to-fit mode that culls pages afterscrollY + viewportHeighteven though the canvas can displayviewportHeight / scaleFactorlogical pixels, so the lower part of multi-page documents renders blank.💡 Suggested fix
- const visibleTop = scrollY; - const visibleBottom = scrollY + viewportHeight; + const visibleTop = scrollY; + const logicalViewportHeight = scaleFactor < 1 + ? viewportHeight / scaleFactor + : viewportHeight; + const visibleBottom = scrollY + logicalViewportHeight;🤖 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 82 - 95, visibleBottom is computed in physical pixels while scrollY is in document-space, causing incorrect culling at non-1 scale; update the visibleBottom calculation to use logical viewport height by dividing viewportHeight by scaleFactor (i.e. visibleBottom = scrollY + viewportHeight / scaleFactor) so page culling (the loop over paginatedLayout.pages using getPageYOffset and page.height) uses matching coordinate spaces with visibleTop/scrollY and scaleFactor.packages/docs/src/view/editor.ts (1)
186-229:⚠️ Potential issue | 🟠 MajorPreserve logical scroll position when
scaleFactorchanges.Line 229 starts interpreting
container.scrollTopasscrollTop / scaleFactor, but the existing physicalscrollTopis never renormalized when the factor changes. On resize/orientation changes the viewport jumps to a different document position instead of staying on the same logical content.🤖 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 186 - 229, When scaleFactor changes we must preserve the logical scroll position by renormalizing container.scrollTop; compute the logical scroll (oldLogical = container.scrollTop / previousScaleFactor) before updating scaleFactor-driven layout, then after computing the new scaleFactor and before doing any scroll-dependent ops (e.g., setting spacer.style.height/marginTop, computing cursorTop/cursorBottom, and deriving scrollY), set container.scrollTop = oldLogical * scaleFactor (clamped >=0). Add a persistent previousScaleFactor variable (or reuse an existing one) to compare and update (previousScaleFactor = scaleFactor) so subsequent renders can renormalize correctly; reference symbols: scaleFactor, previousScaleFactor (new), container.scrollTop, spacer.style.height/marginTop, docCanvas.resize, cursor.getPixelPosition, and scrollY.
🤖 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-mobile-zoom-to-fit.md`:
- Around line 40-45: The fenced code blocks in this document are missing
language tags (MD040); update each block to include the appropriate language
specifier (e.g., ```ts) — specifically add language tags to the blocks
containing MOBILE_PADDING/pageWidth/scaleFactor, the rendering snippet with
ctx.save()/ctx.scale()/ctx.translate()/ctx.restore(), the pointer/coordinate
code using this.getScaleFactor()/e.clientX/e.clientY, the spacer scroll math
with spacer.style.height and scrollY, and the final conditional with
ruler.hide()/ruler.show(); ensure every triple-backtick fence before these
snippets is changed to include the language tag.
- Around line 43-45: The scaleFactor calculation can become negative when
containerWidth < 2 * MOBILE_PADDING and/or when pageWidth is <= 0; update the
docs to clamp scaleFactor to the [0,1] range and document guarding against
invalid pageWidth by using pageWidth = paginatedLayout.pages[0]?.width ?? 0 and
then computing scaleFactor = Math.max(0, Math.min(1, (containerWidth -
MOBILE_PADDING * 2) / pageWidth)) (and note that callers should handle pageWidth
<= 0 or treat such pages as not renderable).
In `@docs/tasks/active/20260328-docs-mobile-zoom-to-fit-todo.md`:
- Around line 1-25: Create the missing lessons file paired with the todo entry
by adding docs/tasks/active/20260328-docs-mobile-zoom-to-fit-lessons.md
(matching the slug and date in the todo file), and populate it with a brief
summary of what was implemented, key design decisions (scale computation with
computeScaleFactor and MOBILE_PADDING, ctx.scale usage in doc-canvas.render,
hit-test coordinate inversion in text-editor), testing notes (unit tests in
view/scale.test.ts and any Vitest setup), known pitfalls/edge-cases (ruler
visibility, spacer/scroll interactions), and a short retro/checklist for
follow-ups and learnings so the repo follows the paired todo/lessons guideline.
In `@packages/docs/src/view/editor.ts`:
- Around line 788-801: getCursorScreenRect is using an unscaled physicalWidth
(Math.max(viewportWidth, pageWidth)) so when scaleFactor < 1
cursor.getPixelPosition computes offsets against the larger page width; change
the physicalWidth selection to prefer the viewport width in scaled mode: compute
viewportWidth from (container.parentElement ?? container).clientWidth (or
bounding rect width) and set physicalWidth = scaleFactor < 1 ? viewportWidth :
Math.max((container.parentElement ?? container).getBoundingClientRect().width,
paginatedLayout.pages[0]?.width ?? 0); keep the existing logicalWidth
calculation and pass that to cursor.getPixelPosition so the returned rect aligns
correctly in scaled mode.
In `@packages/docs/src/view/text-editor.ts`:
- Around line 734-741: The mouse-to-logical coordinate conversion done in
updateDragSelection (compute x, y using getScaleFactor(), container offsets and
scroll) is correct, but getLinkHrefAtMouse still passes raw physical coordinates
into paginatedPixelToPosition; update getLinkHrefAtMouse (and the similar block
around the other occurrence at the referenced range) to apply the same
conversion: compute scaled logical x/y (use
this.container.getBoundingClientRect(), this.getScaleFactor(),
this.getCanvasOffsetTop(), and this.container.scrollTop as in
updateDragSelection) and pass those logical coordinates into
paginatedPixelToPosition/getLinkHrefAtMouse’s hit-test so link resolution works
correctly in zoom-to-fit/zoomed modes.
---
Outside diff comments:
In `@packages/docs/src/view/doc-canvas.ts`:
- Around line 82-95: visibleBottom is computed in physical pixels while scrollY
is in document-space, causing incorrect culling at non-1 scale; update the
visibleBottom calculation to use logical viewport height by dividing
viewportHeight by scaleFactor (i.e. visibleBottom = scrollY + viewportHeight /
scaleFactor) so page culling (the loop over paginatedLayout.pages using
getPageYOffset and page.height) uses matching coordinate spaces with
visibleTop/scrollY and scaleFactor.
In `@packages/docs/src/view/editor.ts`:
- Around line 186-229: When scaleFactor changes we must preserve the logical
scroll position by renormalizing container.scrollTop; compute the logical scroll
(oldLogical = container.scrollTop / previousScaleFactor) before updating
scaleFactor-driven layout, then after computing the new scaleFactor and before
doing any scroll-dependent ops (e.g., setting spacer.style.height/marginTop,
computing cursorTop/cursorBottom, and deriving scrollY), set container.scrollTop
= oldLogical * scaleFactor (clamped >=0). Add a persistent previousScaleFactor
variable (or reuse an existing one) to compare and update (previousScaleFactor =
scaleFactor) so subsequent renders can renormalize correctly; reference symbols:
scaleFactor, previousScaleFactor (new), container.scrollTop,
spacer.style.height/marginTop, docCanvas.resize, cursor.getPixelPosition, and
scrollY.
🪄 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: 36596104-077d-4d56-9f82-feb82235f648
📒 Files selected for processing (9)
docs/design/docs-mobile-zoom-to-fit.mddocs/tasks/active/20260328-docs-mobile-zoom-to-fit-todo.mdpackages/docs/src/index.tspackages/docs/src/view/doc-canvas.tspackages/docs/src/view/editor.tspackages/docs/src/view/ruler.tspackages/docs/src/view/scale.tspackages/docs/src/view/text-editor.tspackages/docs/test/view/scale.test.ts
Comment on lines
+43
to
+45
| pageWidth = paginatedLayout.pages[0]?.width ?? 0 | ||
| scaleFactor = Math.min(1, (containerWidth - MOBILE_PADDING * 2) / pageWidth) | ||
| ``` |
There was a problem hiding this comment.
Document a lower-bound clamp for scaleFactor to avoid negative scale.
As written, the formula can go below 0 when containerWidth < 2 * MOBILE_PADDING, which would imply a flipped/invalid render scale. The design doc should explicitly clamp to [0, 1] (and ideally guard pageWidth <= 0).
Suggested doc-only diff
-pageWidth = paginatedLayout.pages[0]?.width ?? 0
-scaleFactor = Math.min(1, (containerWidth - MOBILE_PADDING * 2) / pageWidth)
+pageWidth = paginatedLayout.pages[0]?.width ?? 0
+availableWidth = Math.max(0, containerWidth - MOBILE_PADDING * 2)
+scaleFactor = pageWidth > 0 ? Math.min(1, availableWidth / pageWidth) : 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pageWidth = paginatedLayout.pages[0]?.width ?? 0 | |
| scaleFactor = Math.min(1, (containerWidth - MOBILE_PADDING * 2) / pageWidth) | |
| ``` | |
| pageWidth = paginatedLayout.pages[0]?.width ?? 0 | |
| availableWidth = Math.max(0, containerWidth - MOBILE_PADDING * 2) | |
| scaleFactor = pageWidth > 0 ? Math.min(1, availableWidth / pageWidth) : 1 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/docs-mobile-zoom-to-fit.md` around lines 43 - 45, The scaleFactor
calculation can become negative when containerWidth < 2 * MOBILE_PADDING and/or
when pageWidth is <= 0; update the docs to clamp scaleFactor to the [0,1] range
and document guarding against invalid pageWidth by using pageWidth =
paginatedLayout.pages[0]?.width ?? 0 and then computing scaleFactor =
Math.max(0, Math.min(1, (containerWidth - MOBILE_PADDING * 2) / pageWidth)) (and
note that callers should handle pageWidth <= 0 or treat such pages as not
renderable).
Comment on lines
+1
to
+25
| # Docs Mobile Zoom-to-Fit Implementation Plan | ||
|
|
||
| > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. | ||
|
|
||
| **Goal:** Scale the Canvas-based docs editor to fit mobile viewports when the container is narrower than the page width. | ||
|
|
||
| **Architecture:** Compute a scale factor from container width vs page width, apply `ctx.scale()` in the Canvas render pass, and invert coordinates in hit-testing. Layout/pagination engines remain unchanged. | ||
|
|
||
| **Tech Stack:** TypeScript, Canvas 2D API, Vitest | ||
|
|
||
| **Spec:** `docs/design/docs-mobile-zoom-to-fit.md` | ||
|
|
||
| --- | ||
|
|
||
| ## File Map | ||
|
|
||
| | File | Action | Responsibility | | ||
| |------|--------|---------------| | ||
| | `packages/docs/src/view/scale.ts` | Create | `computeScaleFactor()` pure function + `MOBILE_PADDING` constant | | ||
| | `packages/docs/test/view/scale.test.ts` | Create | Unit tests for scale factor calculation | | ||
| | `packages/docs/src/view/doc-canvas.ts` | Modify | Accept `scaleFactor` in `render()`, apply `ctx.scale` | | ||
| | `packages/docs/src/view/text-editor.ts` | Modify | Accept `getScaleFactor` callback, invert coordinates in hit-test methods | | ||
| | `packages/docs/src/view/ruler.ts` | Modify | Add `hide()`/`show()` methods | | ||
| | `packages/docs/src/view/editor.ts` | Modify | Compute scale factor, wire it through, scale spacer/scroll, hide ruler | | ||
| | `packages/docs/src/index.ts` | Modify | Export `computeScaleFactor` and `MOBILE_PADDING` | |
There was a problem hiding this comment.
Add the paired lessons file for this task.
This adds a non-trivial *-todo.md entry, but the matching docs/tasks/active/20260328-docs-mobile-zoom-to-fit-lessons.md file is missing.
As per coding guidelines, docs/tasks/active/*-{todo,lessons}.md: "For non-trivial tasks, use paired files in docs/tasks/active/: YYYYMMDD-<slug>-todo.md and YYYYMMDD-<slug>-lessons.md."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tasks/active/20260328-docs-mobile-zoom-to-fit-todo.md` around lines 1 -
25, Create the missing lessons file paired with the todo entry by adding
docs/tasks/active/20260328-docs-mobile-zoom-to-fit-lessons.md (matching the slug
and date in the todo file), and populate it with a brief summary of what was
implemented, key design decisions (scale computation with computeScaleFactor and
MOBILE_PADDING, ctx.scale usage in doc-canvas.render, hit-test coordinate
inversion in text-editor), testing notes (unit tests in view/scale.test.ts and
any Vitest setup), known pitfalls/edge-cases (ruler visibility, spacer/scroll
interactions), and a short retro/checklist for follow-ups and learnings so the
repo follows the paired todo/lessons guideline.
- doc-canvas: convert viewportHeight to logical coords in viewport culling (visibleBottom = scrollY + viewportHeight / scaleFactor) - editor: fix getCursorScreenRect physicalWidth to use viewportWidth in scaled mode, matching paint() canvas width calculation - text-editor: apply scale inversion to getLinkHrefAtMouse so Ctrl+click link detection works correctly in zoom-to-fit mode
Add language tags to fenced code blocks in design doc (MD040), create paired lessons file, and add design doc to README index. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/docs/src/view/doc-canvas.ts (1)
65-65: GuardscaleFactorbefore using it in culling/transform.Line 65 accepts any numeric value;
0, negative, or non-finite values can break culling and rendering. Add a guard once at method entry and use the sanitized value below.Proposed patch
render( @@ - scaleFactor: number = 1, + scaleFactor: number = 1, ): void { + const safeScaleFactor = Number.isFinite(scaleFactor) && scaleFactor > 0 ? scaleFactor : 1; + if (safeScaleFactor !== scaleFactor) { + console.warn(`DocCanvas.render received invalid scaleFactor (${scaleFactor}); falling back to 1.`); + } + @@ - const visibleBottom = scrollY + viewportHeight / scaleFactor; + const visibleBottom = scrollY + viewportHeight / safeScaleFactor; @@ - if (scaleFactor !== 1) { - this.ctx.scale(scaleFactor, scaleFactor); + if (safeScaleFactor !== 1) { + this.ctx.scale(safeScaleFactor, safeScaleFactor); }As per coding guidelines: "Use meaningful error messages that help with debugging" and "Avoid deeply nested code by using early returns or guard clauses".
🤖 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` at line 65, Validate and sanitize the incoming scaleFactor at the start of the function in doc-canvas.ts (where scaleFactor: number = 1 is accepted) before any culling or transform logic; if scaleFactor is <= 0, NaN, or not finite, replace it with a safe default (e.g., 1) or throw a clear error message, and then use that sanitized local variable for all subsequent culling/transform operations to avoid passing invalid values through functions like the culling or transform routines.
🤖 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/view/doc-canvas.ts`:
- Line 65: Validate and sanitize the incoming scaleFactor at the start of the
function in doc-canvas.ts (where scaleFactor: number = 1 is accepted) before any
culling or transform logic; if scaleFactor is <= 0, NaN, or not finite, replace
it with a safe default (e.g., 1) or throw a clear error message, and then use
that sanitized local variable for all subsequent culling/transform operations to
avoid passing invalid values through functions like the culling or transform
routines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee562706-5677-409d-8a6d-d373b53835f7
📒 Files selected for processing (6)
docs/design/README.mddocs/design/docs-mobile-zoom-to-fit.mddocs/tasks/active/20260328-docs-mobile-zoom-to-fit-lessons.mdpackages/docs/src/view/doc-canvas.tspackages/docs/src/view/editor.tspackages/docs/src/view/text-editor.ts
✅ Files skipped from review due to trivial changes (3)
- docs/design/README.md
- docs/tasks/active/20260328-docs-mobile-zoom-to-fit-lessons.md
- docs/design/docs-mobile-zoom-to-fit.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/docs/src/view/text-editor.ts
- packages/docs/src/view/editor.ts
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
ctx.scale()based zoom-to-fit when viewport is narrower than page width (816px)(containerWidth - 32px padding) / pageWidth, capped at 1.0Design
See
docs/design/docs-mobile-zoom-to-fit.mdfor the full design spec.This is phase 1 (viewing) of mobile docs support. Phase 2 (reflow + editing) will follow.
Changed files
packages/docs/src/view/scale.tscomputeScaleFactor()utilitypackages/docs/src/view/doc-canvas.tsscaleFactorinrender(), applyctx.scalepackages/docs/src/view/ruler.tshide()/show()methodspackages/docs/src/view/text-editor.tspackages/docs/src/view/editor.tspackages/docs/test/view/scale.test.tsTest plan
pnpm verify:fastpasses🤖 Generated with Claude Code
Summary by CodeRabbit