Forward slides editor scale to docs text-box click math#256
Conversation
The slides text-box container is sized in host pixels (frame * scale) but the text layout is computed in logical pixels (contentWidth = frame.w). TextEditor.getPositionFromMouse divides the click x by getScaleFactor() to recover the logical coord, but the docs initializeTextBox shim hard-coded that getter to 1. At any scale != 1 (i.e. every real session, since scale = hostWidth / SLIDE_WIDTH) the click x stayed in host pixels and was compared against run.x values in logical pixels — including the alignment offset applied by applyAlignment. The mismatch dropped clicks into the "localX < firstRun.x" branch of findPositionAtPixel, especially for center- and right-aligned paragraphs whose runs sit far from the layout origin, snapping the caret to offset 0. Add a scale option to TextBoxEditorOptions, plumb it through the TextEditor's getScaleFactor shim, and forward the slide zoom from mountSlidesTextBox. Cover the regression with a slides-side test that mounts the real factory at scale=0.5 and verifies clicks on visually rendered glyphs (right- and left-aligned) hit non-zero offsets — failing pre-fix, passing post-fix.
Replace `toBeGreaterThan(0)` and `toBeGreaterThan(7)` with exact equalities. The loose bounds would still pass if a future change introduced a scale-direction inversion (e.g. `() => 1/scale`) or a partial scale propagation — both would produce wrong-but-nonzero offsets. Exact offsets lock in the post-fix character that the snap math lands on. Also reword the flushRaf comment to match what it actually does (one frame wait, not "a few sequential flushes").
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an optional ChangesScale-aware text-box click positioning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/docs/src/view/text-box-editor.ts`:
- Around line 255-256: The assignment to scale uses opts.scale ?? 1 which
permits 0, NaN, or Infinity; update the initialization of scale (where
opts.scale is read) to validate that opts.scale is a finite positive number
(e.g. Number.isFinite(opts.scale) && opts.scale > 0) and otherwise fall back to
1 before any pointer math or calls to getScaleFactor; adjust any downstream uses
in this module (e.g., click-to-position conversion routines) to rely on the
validated local scale variable.
In `@packages/slides/test/view/editor/text-box-click-scale.test.ts`:
- Around line 129-130: Update the stale inline comment near the SCALE constant
in text-box-click-scale.test.ts: replace or rewrite the "offset > 7" remark to
accurately describe the current assertion (the test now expects strict equality
to 10 via toBe(10)), explaining why 10 is the expected value for SCALE = 0.5 (or
simply remove the misleading fragment and add a short why note). Locate the
comment adjacent to the SCALE = 0.5 declaration and ensure it documents the
reason why the test expects exactly 10 rather than a loose inequality.
🪄 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: 35e40dc1-3371-40b6-934e-7ee048de3327
📒 Files selected for processing (5)
docs/tasks/active/20260517-slides-textbox-click-scale-lessons.mddocs/tasks/active/20260517-slides-textbox-click-scale-todo.mdpackages/docs/src/view/text-box-editor.tspackages/slides/src/view/editor/text-box-editor.tspackages/slides/test/view/editor/text-box-click-scale.test.ts
| const scale = opts.scale ?? 1; | ||
|
|
There was a problem hiding this comment.
Validate scale before using it in pointer math.
At Line 255, opts.scale ?? 1 allows 0, NaN, or Infinity, which can break click-to-position conversion when getScaleFactor is consumed downstream. Guard for finite positive values before assigning scale.
Proposed fix
- const scale = opts.scale ?? 1;
+ const scale = Number.isFinite(opts.scale) && (opts.scale as number) > 0
+ ? (opts.scale as number)
+ : 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.
| const scale = opts.scale ?? 1; | |
| const scale = Number.isFinite(opts.scale) && (opts.scale as number) > 0 | |
| ? (opts.scale as number) | |
| : 1; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/docs/src/view/text-box-editor.ts` around lines 255 - 256, The
assignment to scale uses opts.scale ?? 1 which permits 0, NaN, or Infinity;
update the initialization of scale (where opts.scale is read) to validate that
opts.scale is a finite positive number (e.g. Number.isFinite(opts.scale) &&
opts.scale > 0) and otherwise fall back to 1 before any pointer math or calls to
getScaleFactor; adjust any downstream uses in this module (e.g.,
click-to-position conversion routines) to rely on the validated local scale
variable.
There was a problem hiding this comment.
Skipping. CLAUDE.md ("Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees. Only validate at system boundaries") applies here: mountSlidesTextBox is internal and computes scale = hostWidth / SLIDE_WIDTH from editor options. The sibling dpr = opts.dpr ?? 1 field at the same boundary has no such guard either, so adding one here would create an inconsistent precedent. If a caller does pass a bogus scale the right fix is to make that an error at the call site, not silently coerce here.
There was a problem hiding this comment.
@hackerwins, understood — the internal call-site guarantee and the parity with dpr make the silent-coercion guard unnecessary here. Happy to defer any scale validation to the entry point if it ever becomes user-facing. Thanks for the context!
✏️ Learnings added
Learnt from: hackerwins
Repo: wafflebase/wafflebase PR: 256
File: packages/docs/src/view/text-box-editor.ts:255-256
Timestamp: 2026-05-17T05:23:41.445Z
Learning: In wafflebase/wafflebase, per CLAUDE.md, do NOT add error-handling, fallbacks, or silent-coercion validation for scenarios that can't happen at internal boundaries. Trust internal code and framework guarantees; only validate at true system boundaries. This applies specifically to internal options like `scale` and `dpr` in `TextBoxEditorOptions` (`packages/docs/src/view/text-box-editor.ts`) where the callers (e.g. `mountSlidesTextBox`) are internal and compute values from controlled inputs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: hackerwins
Repo: wafflebase/wafflebase PR: 65
File: packages/docs/src/view/text-editor.ts:484-490
Timestamp: 2026-03-22T11:11:01.689Z
Learning: In packages/docs/src/view/*, when computing selection boundaries and wrap-aware cursor/navigation behavior, include and consistently thread `lineAffinity` through the relevant selection/text-editor helpers. Concretely: extend selection-related endpoints (e.g., in `selection.ts`) to carry `lineAffinity` alongside `blockId` and `offset`, and pass it into `findPageForPosition` so selection boundary calculations use the correct visual line context. Likewise, thread `lineAffinity` through `getVisualLineRange()`, `getVisualLineStart()`, and `getVisualLineEnd()` so wrap-boundary offsets resolve to the correct visual line for Home/End and Cmd+Backspace navigation.
Verification: verify:selfResult: ✅ PASS in 196.8s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The post-tightening commit changed the assertion to `toBe(10)` but left the comment referencing the old `offset > 7` shape. Rewrite to describe the current strict-equality assertion and why 10 is the post-fix value (logical x = 80 lands on the 10th boundary after binary-snap).
Sibling of the x-axis scale bug: TextEditor.getPositionFromMouse computes `(clientY - rect.top - canvasOffsetTop) / scale`. The first fix only scaled `getScaleFactor`; the shim's `getCanvasOffsetTop` kept returning the raw logical `-Theme.pageGap`, but the formula subtracts it from a host-pixel quantity. At scale != 1 every click y picked up an extra `(1 - scale) * pageGap / scale` logical pixels of bias — at scale = 0.5 that's 40 px, enough to skip ~2 paragraphs. User symptom: clicking paragraph 2 puts the caret in paragraph 3. Multiply the offset by `scale` so it lives in the same host-pixel space as `clientY - rect.top`. At scale = 1 this is a no-op, so the docs full-document factory is unaffected. Cover with a 4-paragraph regression test that asserts the clicked paragraph id at scale = 0.5 (pre-fix: 'p3', post-fix: 'p2').
The slides editor's onDoubleClick listener lives on `overlay`, and
the text-box editor mounts its container as an overlay child. A
dblclick *inside* an active text-box bubbles up here, hits the same
element id in topmostUnderPoint(), and enterEditMode treats it as
"switch edit target" — calling exitEditMode('commit') and re-mounting
the text-box. The re-mount resets the docs cursor to offset 0, which
wipes the word selection the inner TextEditor's second mousedown
just made. User-visible symptom: double-click selects a word, then
the caret jumps to the start of the first paragraph.
Bail out of onDoubleClick when `hit === this.editingElementId` and
let the inner docs TextEditor own the dblclick. Double-clicking to
switch to a *different* text-box still works because the hit-target
differs from the editing element.
Add a regression test that wraps the mockTextBox factory to count
mount calls: a second dblclick on the already-editing element must
not bump the count.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/tasks/active/20260517-slides-textbox-click-scale-todo.md (1)
8-8: 💤 Low valueMinor: Hyphenate the compound adjective.
The phrase "center / right aligned paragraphs" should use hyphens: "center- and right-aligned paragraphs" or "center/right-aligned paragraphs" for consistency with standard English compound adjective formatting.
✏️ Suggested fix
-symptom is especially obvious on center / right aligned paragraphs: +symptom is especially obvious on center- and right-aligned paragraphs:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/tasks/active/20260517-slides-textbox-click-scale-todo.md` at line 8, Replace the phrase "center / right aligned paragraphs" with a hyphenated compound adjective such as "center- and right-aligned paragraphs" (or alternatively "center/right-aligned paragraphs") wherever it appears (specifically update the sentence containing "symptom is especially obvious on center / right aligned paragraphs") to follow standard compound-adjective formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/tasks/active/20260517-slides-textbox-click-scale-todo.md`:
- Line 8: Replace the phrase "center / right aligned paragraphs" with a
hyphenated compound adjective such as "center- and right-aligned paragraphs" (or
alternatively "center/right-aligned paragraphs") wherever it appears
(specifically update the sentence containing "symptom is especially obvious on
center / right aligned paragraphs") to follow standard compound-adjective
formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d8c79d2-9496-44ab-92f9-ca719a9b1362
📒 Files selected for processing (3)
docs/tasks/active/20260517-slides-textbox-click-scale-todo.mdpackages/docs/src/view/text-box-editor.tspackages/slides/test/view/editor/text-box-click-scale.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/docs/src/view/text-box-editor.ts
1 participant
Summary
frame * scale) but the text layout is computed in logical pixels (contentWidth = frame.w). The docsTextEditor.getPositionFromMousedivides click x bygetScaleFactor()to recover logical coords, butinitializeTextBoxhard-coded that getter to() => 1— so at any scale ≠ 1 (every real session, sincescale = hostWidth / SLIDE_WIDTH) clicks stayed in host pixels and were compared againstrun.xvalues in logical pixels (which include the alignment offset fromapplyAlignment). The mismatch dropped clicks into thelocalX < firstRun.xbranch offindPositionAtPixel, snapping the caret to offset 0 — especially obvious on center- and right-aligned paragraphs whose runs sit far from the layout origin.scale?: numberoption toTextBoxEditorOptions(defaults to 1 → no change for the full-document docs editor), wire it into theTextEditor'sgetScaleFactorshim, and forwardopts.scalefrommountSlidesTextBox.mountSlidesTextBoxatscale=0.5and verifies clicks on visually rendered glyphs (right- and left-aligned) hit exact post-snap offsets — failing pre-fix on both, passing post-fix.Test plan
pnpm verify:fastgreen() => scaleback to() => 1)pnpm dev, click visible glyphs, caret lands at clicked character (not start of block)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation