Fix arrow up/down and click pixel-to-position accuracy#106
Merged
Conversation
Pre-compute cumulative character pixel widths during layout so that paginatedPixelToPosition can use binary search instead of uniform character width approximation. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Replace uniform character width approximation with binary search on pre-computed per-character pixel offsets. Fixes Arrow Up/Down and mouse click landing on wrong character with proportional fonts.
Test that paginatedPixelToPosition correctly snaps to characters using pre-computed charOffsets with variable character widths.
Replace runtime ctx.measureText call with charOffsets lookup for position-to-pixel mapping, ensuring forward and reverse conversions use the same data source.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR implements precomputed per-character offset tracking for text runs in the docs renderer. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Layout as Layout Engine
participant CharOffset as Character Offsets
participant Pagination as Pagination Hit-Test
participant TextEditor as Text Editor
rect rgba(200, 150, 255, 0.5)
Note over Layout: Old Flow: Runtime Measurement
Layout->>Pagination: Each run without precomputed offsets
Pagination->>TextEditor: Per-substring measureText needed
TextEditor->>Pagination: Computed width for character position
end
rect rgba(150, 200, 255, 0.5)
Note over Layout: New Flow: Precomputed Offsets
Layout->>CharOffset: Compute charOffsets per run
CharOffset->>CharOffset: Canvas.measureText cumulative widths
CharOffset->>Layout: Return offsets array
Layout->>Pagination: Run with charOffsets
Pagination->>Pagination: Binary search over charOffsets
Pagination->>TextEditor: Character boundary located
TextEditor->>TextEditor: Direct array lookup: charOffsets[index]
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 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 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 109.4s
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.
🧹 Nitpick comments (2)
packages/docs/test/view/pagination.test.ts (1)
212-278: Add one round-trip regression for the forward path too.These cases pin
paginatedPixelToPosition(), but the original bug was the disagreement between reverse mapping andTextEditor.getPixelForPosition(). One public click→cursor→ArrowUp/Down orresolvePositionPixel()round-trip would protect the forward side as well.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 212 - 278, Add a forward-path round-trip test that after computing a position with paginatedPixelToPosition you convert that position back to pixels and then resolve pixels back to a position to ensure consistency: use the existing test helpers (mockRun, mockLineWithRuns, mockBlock, paginateLayout) to create a small run/line, call paginatedPixelToPosition to get pos, then call TextEditor.getPixelForPosition (or resolvePositionPixel/getPixelForPosition equivalent in this codebase) with that position to get pixelX and finally call paginatedPixelToPosition again with pixelX and assert the final offset equals the original offset; place this new it(...) in the same describe block so it guards the forward mapping as well as the reverse.packages/docs/src/view/table-layout.ts (1)
4-5: Unify table-cell hit testing aroundcharOffsets.Table runs now carry the same per-character metadata as top-level runs, but
TextEditor.resolveOffsetInCellAtXY()still maps table-cell clicks by remeasuring text in a separate loop. Keeping both implementations around makes tables the next place this cursor drift can come back.Also applies to: 95-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/table-layout.ts` around lines 4 - 5, TextEditor.resolveOffsetInCellAtXY() currently remeasures text in a separate loop instead of using the per-character metadata already present on table runs (charOffsets), causing inconsistent hit testing; replace the manual remeasurement with the same mapping used for top-level runs by reading run.charOffsets (or calling computeCharOffsets(...) where missing), applying applyAlignment(...) to adjust x for the run, and binary-searching or iterating charOffsets to find the clicked character index, removing the duplicate cachedMeasureText/remeasure loop and ensuring the resolved offset uses the run's existing charOffsets and alignment logic.
🤖 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/table-layout.ts`:
- Around line 4-5: TextEditor.resolveOffsetInCellAtXY() currently remeasures
text in a separate loop instead of using the per-character metadata already
present on table runs (charOffsets), causing inconsistent hit testing; replace
the manual remeasurement with the same mapping used for top-level runs by
reading run.charOffsets (or calling computeCharOffsets(...) where missing),
applying applyAlignment(...) to adjust x for the run, and binary-searching or
iterating charOffsets to find the clicked character index, removing the
duplicate cachedMeasureText/remeasure loop and ensuring the resolved offset uses
the run's existing charOffsets and alignment logic.
In `@packages/docs/test/view/pagination.test.ts`:
- Around line 212-278: Add a forward-path round-trip test that after computing a
position with paginatedPixelToPosition you convert that position back to pixels
and then resolve pixels back to a position to ensure consistency: use the
existing test helpers (mockRun, mockLineWithRuns, mockBlock, paginateLayout) to
create a small run/line, call paginatedPixelToPosition to get pos, then call
TextEditor.getPixelForPosition (or resolvePositionPixel/getPixelForPosition
equivalent in this codebase) with that position to get pixelX and finally call
paginatedPixelToPosition again with pixelX and assert the final offset equals
the original offset; place this new it(...) in the same describe block so it
guards the forward mapping as well as the reverse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b9cf184-e8d4-4ed2-9622-cba86c4dabe7
📒 Files selected for processing (6)
packages/docs/src/view/layout.tspackages/docs/src/view/pagination.tspackages/docs/src/view/table-layout.tspackages/docs/src/view/text-editor.tspackages/docs/test/view/pagination.test.tspackages/docs/test/view/peer-cursor.test.ts
hackerwins
added a commit
that referenced
this pull request
Apr 8, 2026
The task was completed in PR #106 but the plan file remained in active/. Move it to archive/2026/03/ alongside its todo file. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
7 tasks
7 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
charOffsets) inLayoutRunduring layoutpaginatedPixelToPositionwith binary search oncharOffsetscharOffsetsingetPixelForPositionfor forward/reverse consistencyProblem
Arrow Up/Down and mouse clicks land on the wrong character with proportional fonts. The reverse mapping (
paginatedPixelToPosition) usedrun.width / text.lengthas a uniform character width, while the forward mapping (getPixelForPosition) usedctx.measureText(). This asymmetry caused off-by-one cursor positioning.Approach
Option chosen: Pre-compute
charOffsetsin layout (Option 2 from task analysis)charOffsets: number[]toLayoutRun— cumulative pixel widths per charactercomputeCharOffsets()helper usingctx.measureText()during layout buildpaginatedPixelToPositionsnaps to nearest character boundarygetPixelForPositionnow reads fromcharOffsetsinstead of measuring at runtimeTest plan
pagination.test.tspnpm verify:fastpasses (lint + unit tests)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests