Sign up
View stargazersSign up

fix(desktop): stop annotation draw strokes doing quadratic work#8270

closed3
main
GitHub fork: fix/annotation-draw-quadratic-stroke
openedAug 26, 2026byLlnieuwenhuis
3 files changed
View on GitHubTwo-way sync enabledPR last imported 20d ago

Problem

Every pointermove during a freehand annotation stroke copied the whole points array, rebuilt the bounds by mapping all points into two coordinate arrays spread into Math.min/Math.max, and regenerated the entire SVG path string. A stroke of n points therefore cost O(n^2) with a matching pile of short-lived allocation on the preview renderer's main thread, and past the engine's argument limit (~100k points on Node 25 here) the spread throws RangeError.

Fixes #8059

Fix

Stroke geometry moves into apps/desktop/src/preview/AnnotationStroke.ts, an electron-free sibling module in the same spirit as AnnotationKeyboard.ts, so it can be unit-tested without an Electron preload context. Per point the work is now O(1): the point is pushed onto the array the stroke target already holds instead of copying it, the min/max extents widen against the new point alone (no spread anywhere), and the SVG is split into 100-segment chunks: completed paths stay frozen while only the active chunk is rewritten. PickPreload.ts just carries the geometry alongside the active stroke.

Output is byte-identical to the old helpers, so nothing about the rendered stroke, its smoothing, or the persisted points/bounds shape changes. Sample decimation and rAF-coalescing the repaint were deliberately left out — they change what gets recorded/painted, and this fix does not need them.

Verification

  • vp test run apps/desktop/src/preview/AnnotationStroke.test.ts - 5 passed. The new test replays a 200-point stroke and asserts the incremental path and bounds equal the old full-rescan implementations at every prefix length, and draws a 250,000-point stroke with correct bounds (the old code would both throw RangeError here and blow the test timeout).
  • Mutation-checked the oracle: flipping the quadratic-segment condition in extendStroke fails the equivalence test, so it is not vacuous.
  • tsgo --noEmit in apps/desktop - no errors (only pre-existing Effect suggestions in unrelated files).
  • vp lint and vp fmt --check on the three touched files - clean.
  • vp pack in apps/desktop - preview-pick-preload.cjs builds with the new module inlined.

Not run: repo-wide checks (CI owns those), and no in-app browser pass, since this needs a live Electron preview session and the change is output-identical by construction and by test.

Implemented with Claude Opus via the Claude Code harness.

Note

Low Risk Preview-only performance refactor with oracle tests and output-identical geometry; main behavioral nuance is in-place points mutation during an active draw.

Overview Fixes O(n²) work on every pointermove while drawing freehand annotation strokes by moving geometry into a new AnnotationStroke module with O(1) per-point updates.

Instead of copying the points array, rescanning all coordinates with Math.min/Math.max spreads, and rebuilding the full SVG d string each move, PickPreload now keeps StrokeGeometry: points are pushed in place on the stroke target, extents widen against the new point only, and the path grows by concatenating one segment. Rendered strokes use an SVG <g> with multiple chunk <path> elements so only the active chunk’s d is rewritten (bounded re-parse cost); finalized chunks are frozen when STROKE_CHUNK_SEGMENTS (100) is reached.

Tests compare incremental path/bounds to naive full-rescan oracles at every prefix length, verify chunk handoff/reassembly, and stress 250k points without RangeError. Persisted stroke shape and smoothing stay byte-identical to the old helpers; target.points keeps the same array reference while drawing instead of replacing it each move.

Reviewed by Cursor Bugbot for commit abe7bb7f75514863ee1a1898e647d1bb4bb851dd. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix annotation draw strokes to use incremental O(1) geometry instead of quadratic rescan

  • Adds AnnotationStroke.ts with incremental helpers (beginStroke, extendStroke, strokePath, strokeChunkPath, takeClosedChunk, strokeBounds) that maintain running path and bounds state so each appended point costs O(1) instead of rescanning all prior points
  • Splits each stroke's SVG path into chunks of 100 segments (STROKE_CHUNK_SEGMENTS); closed chunks are handed off as immutable <path> elements and only the active chunk's d is rewritten per pointer move
  • Updates PickPreload.ts to render each stroke as a <g data-stroke-id> containing one or more <path> chunks, replacing the old pathFromPoints/strokeBounds full-rescan functions
  • Adds AnnotationStroke.test.ts validating incremental results against naive reference implementations, including 250k-point stress tests
  • Behavioral Change: strokes now render as <g> with multiple <path> children instead of a single <path>; single-point strokes remove the entire group rather than a lone path; data-stroke-id moves from <path> to <g>

Macroscope summarized abe7bb7.

Summary by CodeRabbit

  • Performance
    • Improved freehand drawing responsiveness, especially for long strokes with many points.
    • Large strokes are now rendered in manageable segments, reducing delays and preventing rendering failures.
    • Stroke bounds and paths update incrementally as you draw, providing smoother pointer movement.
  • Bug Fixes
    • Improved handling of very large annotations that could previously exceed rendering limits or become slow to process.

Checks

15 successful · 9 skipped
All checks passed
f2a7c97

Added 1 commit

  • fix(desktop): stop annotation draw strokes doing quadratic worka64af81
CcoderabbitaiBotAug 26, 2026
Synced from GitHub

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 54209c12-5365-4f45-b91d-e29257d7c853

📥 Commits

Reviewing files that changed from the base of the PR and between 6c583620ff7ad3235b135af7107c0543467eecfa and f2a7c970ca3ae4d113baae74c1a3952f1adadaae.

📒 Files selected for processing (3)
  • apps/desktop/src/preview/AnnotationStroke.test.ts
  • apps/desktop/src/preview/AnnotationStroke.ts
  • apps/desktop/src/preview/PickPreload.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

Adds incremental freehand stroke geometry with running bounds and chunked SVG paths. Updates the draw tool to use the geometry helpers during pointer events. Adds equivalence, chunking, single-point, in-place mutation, and 250,000-point tests.

Changes

Annotation stroke performance

Layer / File(s)Summary
Incremental stroke geometry and validation
apps/desktop/src/preview/AnnotationStroke.ts, apps/desktop/src/preview/AnnotationStroke.test.ts		The new helpers append points, update bounds, build quadratic paths, and split paths into 100-segment chunks. Tests compare incremental output with full-rescan oracles and cover large strokes.
Chunked draw-tool integration
apps/desktop/src/preview/PickPreload.ts		The Draw tool uses incremental geometry and renders only the active chunk. Completed chunks are added to an SVG group.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to f2a7c

Freehand annotations now update incrementally and render in bounded SVG chunks, improving performance for long strokes while retaining the existing stored geometry and visual output. No current merge-blocking risk remains.

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant Pointer
3 participant PickPreload
4 participant AnnotationStroke
5 participant SVG
6 Pointer->>PickPreload: Send draw pointer event
7 PickPreload->>AnnotationStroke: Extend stroke geometry
8 AnnotationStroke-->>PickPreload: Return bounds and chunk paths
9 PickPreload->>SVG: Update active chunk and append closed chunks
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly identifies the desktop annotation draw performance fix and matches the main change.
Description check✅ PassedThe description clearly explains the problem, implementation, verification, scope, and omitted optimizations. It does not use the template headings or checklist, and it omits UI screenshots or video, …
Linked Issues check✅ PassedThe PR satisfies issue #8059 by appending points in place, maintaining bounds incrementally, removing unbounded spread operations, and avoiding full path reconstruction through chunked rendering. Repa…
Out of Scope Changes check✅ PassedThe changes remain within the linked issue scope. The new geometry module, equivalence tests, stress tests, and chunked SVG rendering directly support the performance and argument-limit fix.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

apps/desktop/src/preview/PickPreload.tsline 1055Outdated · a64af81Open thread
……@@ -1072,15 +1050,9 @@ function startAnnotation(): void {
1083- activeStroke.path.setAttribute("d", pathFromPoints(activeStroke.target.points));
1053+ extendStroke(activeStroke.geometry, { x: event.clientX, y: event.clientY });
1054+ activeStroke.target.bounds = strokeBounds(activeStroke.geometry, activeStroke.target.width);
1055+ activeStroke.path.setAttribute("d", strokePath(activeStroke.geometry));
MmacroscopeappBoton line 1055Aug 26, 2026

🟠 High preview/PickPreload.ts:1055

Long strokes still incur quadratic cumulative path-string and SVG rendering work, which can jank or stall the preview. strokePath returns the entire accumulated d value, and setAttribute("d", ...) at every onPointerMove therefore makes the renderer consume all prior commands again for each new point; use an incremental rendering strategy instead of replacing the full path on every point, and cover the DOM update path in the benchmark.

✅ Resolved in abe7bb7f75514863ee1a1898e647d1bb4bb851dd

LlnieuwenhuisAug 26, 2026

Deliberately out of scope for this PR. The defect in #8059 is the quadratic JS-side work (array copies, bounds rescans, path-string rebuilds) and the RangeError from spreading unbounded arrays into Math.min/Math.max — both are eliminated here, and per-point JS cost is now O(1) with byte-identical output.

The residual cost you're pointing at is the browser re-parsing the full d attribute on each setAttribute. Fixing that requires either rAF coalescing (changes what is painted mid-stroke) or splitting the stroke across multiple <path> elements (risks visible double-draw seams at joints with non-opaque strokes). Neither is behavior-preserving, so it belongs in its own change — as your approvability summary also concludes ("should be tracked separately"). Keeping this PR to one concern.

MmacroscopeappBotAug 26, 2026

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

LlnieuwenhuisAug 26, 2026

Now addressed in abe7bb7f7, superseding my earlier scope note. Strokes render as chunked <path> elements under one <g data-stroke-id>: every STROKE_CHUNK_SEGMENTS (100) curve segments, the growing chunk is finalized and a fresh path starts at the committed midpoint, so a pointermove only rewrites the current chunk's d — the renderer re-parses a bounded amount of path text per move instead of the whole stroke.

Cuts land on midpoints of the quadratic chain, which is tangent-continuous there, and stroke paint is opaque, so chunk seams are pixel-identical to the uncut path. Eraser and overlay hit-testing keep working because data-stroke-id and the overlay attribute moved to the group and isAnnotationNode uses closest(). The test suite now asserts chunk reassembly equals the uncut prefix byte-for-byte, seams hand over at identical coordinates, and the per-move rewrite length stays bounded while the full path grows.

MmacroscopeappBotAug 26, 2026

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

MmacroscopeappBotAug 26, 2026
Synced from GitHub

Approvability

Verdict: Approved at abe7bb7

Macroscope's review found this PR approvable — This is a focused desktop performance fix that replaces per-pointermove rescanning with incremental geometry while preserving the existing annotation payload and drawing workflow. Its internal SVG chunking is localized and covered by equivalence and stress tests, keeping the overall risk low.

You can add or adjust custom eligibility rules. Learn more.

Added 1 commit

  • fix(desktop): bound the path text a draw stroke re-parses per moveabe7bb7

Added 1 commit

  • Merge branch 'main' into fix/annotation-draw-quadratic-strokef2a7c97
JJulius MarmingeSep 30, 2026

Note

This comment is posted by Julius' dot

This changes the annotation renderer from one SVG path to multiple chunks, but the PR explicitly says no actual Electron preview pass was run. The geometry tests establish path and bounds equivalence without demonstrating rendering or responsiveness while drawing. Closing for the missing UI evidence required by the verification rule. Please attach before/after screenshots and a short recording of a long stroke in the desktop preview, showing the rendered joins and drawing response, then request reconsideration.

LlnieuwenhuisOct 1, 2026
Synced from GitHub

Actual Electron verification is now complete for f2a7c970ca3ae4d113baae74c1a3952f1adadaae, compared with baseline 6c583620ff7ad3235b135af7107c0543467eecfa. No additional source changes were necessary.

Tested on macOS 27.2 arm64, Electron 44.1.0 / Chromium 152.0.7977.65, using the built desktop preview preload and an isolated local fixture. Installed Playwright Electron automation supplied the same 1,200-point long drawing gesture to both builds. This is actual Electron desktop-preview capture. Both recordings show continuous rendered joins and responsive drawing at this length; the candidate crosses multiple 100-segment chunk boundaries. Observed rAF gap p95 was 9.9ms baseline and 10ms candidate, maximum 10.4ms in both. These observations do not establish a frame-rate speedup; the bounded-work regression is covered by the geometry tests, including 250,000 points.

Fresh geometry tests: 5/5 passed. Desktop typecheck, targeted lint/format and candidate preload build passed. The final candidate was restored and rebuilt after the baseline comparison. A separate reviewer inspected both recordings and stills; the annotation toolbar covers a small portion of the image.

Baseline:

Candidate:

34-second candidate recording:

https://github.com/user-attachments/assets/ca0b88f9-34b0-4644-9b6e-33190282ba80

Could you reconsider and reopen this PR with the requested desktop evidence?

Verification and review: GPT-6 Astra through Codex; CLI-driven Electron capture.