Sign up
View stargazersSign up

fix(desktop): long annotation strokes stay cheap to draw#24573

open2
main
GitHub fork: fix/i8059-annotation-draw-tool-performs-o-n-work-p
opened16d agobyOonly21mil
3 files changed
View on GitHubTwo-way sync enabledPR last imported 3h ago

Problem

With the Draw tool in the desktop preview's annotation mode, every pointer move copied all points drawn so far, mapped them into two coordinate arrays, spread those into Math.min/Math.max, and rebuilt the whole SVG path string. One stroke of n points cost O(n²), and the spread threw RangeError once a stroke outgrew the engine's argument limit.

Fixes #8059

Change

Stroke geometry moves into apps/desktop/src/preview/AnnotationStroke.ts, a small module with no Electron imports so it can be tested directly. Each move now does constant work:

  • The point is pushed onto the array the stroke target already holds.
  • Bounds widen against the new point only. No spreads remain.
  • The stroke is drawn as a run of <path> elements of up to 100 curve segments each, grouped under one <g data-stroke-id>. Only the current chunk is extended. A boundary move finalizes that chunk and starts a new short path, keeping the path text parsed per move bounded. Each new path starts at the exact point where the previous one ended, preserving the curve and smoothing.

Stored points and bounds and the smoothing math match the old code. Erase still finds the stroke by data-stroke-id, now on the group.

Rejected alternatives:

  • Coalescing repaints with requestAnimationFrame. Chromium already delivers about one pointermove per frame (about 1,170 moves in each 20-second stroke below, at 60 frames per second), so it would save nothing.
  • Dropping samples by distance. That changes the recorded stroke.
  • One path with an appended string. The JS gets cheap, but setting d still makes Chromium parse the whole path on every move.

Scope and approval

Bug fix for #8059, which has no maintainer triage yet. It fits the focused obvious-bug route: each pointer move in a Draw stroke costs more as the stroke grows, and the bounds spread can throw on a long stroke. The fix changes no stored points, bounds, smoothing or drawing flow. The same approach in #8270 was closed only for missing desktop evidence (comment), and this PR adds that evidence. If you would rather reopen #8270, I'm happy to close this one.

Verification

vp test run apps/desktop/src/preview/AnnotationStroke.test.ts: 2 passed. One replays a 230-point stroke and checks at every length that the joined paths and bounds equal the old full-rescan output. The other draws 250,000 points. With the module swapped for base's path and bounds code, both fail: base makes one path, and its 250,000-point bounds throw RangeError: Maximum call stack size exceeded at Math.min(...xs).

The focused test against base's code:

Focused stroke tests fail against base geometry

The same test on the candidate:

Focused stroke tests pass on the candidate

Desktop on Linux x64 (Electron 44.9.3): production builds (vp run build:desktop) of base and candidate, run under Xvfb with a copy of real user data. In a thread's Browser panel I opened a local page, turned on Annotate, picked Draw and drew the same scripted stroke with real X pointer input. Metrics come from the preview page over CDP, three runs per build at similar host load, plus one stroke twice as long:

points<path> elementslongest dscript timemain-thread task timeframe gap p99
base1,174 to 1,200124.7 KB0.27 to 0.36 s1.1 to 1.7 s16.8 ms
candidate1,171 to 1,173122.1 KB0.09 s0.84 to 0.88 s16.8 ms
base, 2x stroke2,322147.7 KB0.88 s3.0 s16.8 ms
candidate, 2x stroke2,345242.1 KB0.18 s1.7 s16.8 ms

Base script time grew 2.8x when the stroke doubled. Candidate time doubled. At these lengths neither build drops frames on this machine apart from one host stall in a base run, so this fixes cost growth, not a visible stutter.

The finished stroke on base:

Finished annotation stroke on base

The same stroke on the candidate:

Finished annotation stroke on candidate

Zoomed 4x at three points where one candidate path hands off to the next (top row base, bottom row candidate). The joins render the same:

Base and candidate stroke joins enlarged four times

Both builds drawing the stroke, played at 2x (base left, candidate right):

Base and candidate drawing the same stroke at twice real time

Erase still removes the whole stroke: over CDP the page held 24 stroke paths in one group before one Erase click and none after.

Erase on the candidate, before and after one click on the stroke:

Candidate stroke before and after one erase click

After Attach on the candidate, the annotation appears in the composer:

Attached candidate annotation in the composer

vp run check:changed main: format, lint, knip, typecheck and tests pass.

Not checked:

  • macOS and Windows. The change is renderer-only TypeScript; #8270's author posted macOS captures of the same approach.
  • The RangeError inside the app. In Node the spread fails between 125,000 and 130,000 arguments, which at 60 points per second is over half an hour of continuous drawing. The unit test covers it.

Prior PRs

  • #8270 by lnieuwenhuis took the same approach (in-place points, running bounds, chunked paths) and was closed for missing Electron evidence.
  • tannerpolley's comment on #8059 proposed the same three changes and the evidence shape used here.

Generated with Claude Opus 5.5 in T3 Code Nightly

Fixes pingdotgg/t3code#8059

Checks

21 successful · 8 skipped
All checks passed
942ca4b

Added 1 commit

  • fix(desktop): long annotation strokes stay cheap to drawc408a72
MmacroscopeappBot16d ago
Synced from GitHub

Approvability

Verdict: Approved at c408a72

Macroscope's review found this PR approvable — This is a focused desktop annotation performance fix that bounds SVG path updates and replaces repeated bounds scans with constant-time updates while preserving the existing annotation data and curve. The accompanying tests cover equivalence with the prior geometry and very long strokes.

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

CcoderabbitaiBot16d ago
Synced from GitHub

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The annotation draw path now tracks stroke points and bounds incrementally. It splits SVG paths after 100 segments. PickPreload uses the geometry helpers, and tests compare incremental results with full-rescan references, including a 250,000-point stroke.

Changes: Annotation stroke drawing

Layer / File(s)Summary
Incremental stroke geometry and validation
apps/desktop/src/preview/AnnotationStroke.ts, apps/desktop/src/preview/AnnotationStroke.test.ts		New helpers initialize stroke geometry, append points, update bounds, split paths at 100 segments, and return padded bounds. Tests compare incremental geometry with full-rescan references and check a 250,000-point stroke.
Preview draw-path integration
apps/desktop/src/preview/PickPreload.ts		The preview draw path uses the geometry helpers, stores strokes in SVG groups, creates a new path when a segment closes, and removes the group for strokes with at most one point.

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: External image Minimal · up to 942ca

No merge-blocking issue was identified in the annotation stroke change; normal validation remains appropriate.

Pre-merge checks: Passed 3 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check nameStatusExplanationResolution
Out of Scope Changes checkInconclusiveThe whole-PR summary lists only stroke-geometry changes: AnnotationStroke.ts (+69), AnnotationStroke.test.ts (+73), and PickPreload.ts (+41/-50). The incremental diff from the previous reviewed head c…Provide the whole-PR diff of apps/desktop/src/preview/PickPreload.ts from merge base ade473 to head 942ca4b. If the voice-dictation hunks and the unrelated comment rewrites are not in that diff, record PASS. If they are in that diff, rebase…
Passed Passed checks: 3 passed

Check nameStatusExplanation
Title checkPassedThe title "fix(desktop): long annotation strokes stay cheap to draw" matches the main change. The PR moves stroke geometry into AnnotationStroke.ts so each pointer move does constant work, which remov…
Linked Issues checkPassedDirectly linked issue [#8059] has these active coding requirements: append points in place, maintain bounds incrementally, remove unbounded Math.min(...array) and Math.max(...array) calls, and keep pe…
Description checkPassedThe description covers the problem, change, scope and approval rationale, and verification results. It includes detailed test results and the required visual evidence for the UI change. It also identi…
Full details: Title check

Explanation

The title "fix(desktop): long annotation strokes stay cheap to draw" matches the main change. The PR moves stroke geometry into AnnotationStroke.ts so each pointer move does constant work, which removes the O(n²) cost and the Math.min/Math.max spread RangeError for long strokes. The title uses a conventional commit prefix, is one sentence, and avoids file lists and vague terms.

Full details: Linked Issues check

Explanation

Directly linked issue [#8059] has these active coding requirements: append points in place, maintain bounds incrementally, remove unbounded Math.min(...array) and Math.max(...array) calls, and keep per-move cost bounded. The head version of apps/desktop/src/preview/AnnotationStroke.ts meets these requirements. addStrokePoint pushes onto geometry.points and updates minX/minY/maxX/maxY from the new point only. strokeBounds uses those running values, so no spread remains. The open path is extended by one Q segment per move. Every STROKE_SEGMENTS_PER_PATH (100) segments, the path closes and a new path starts at the midpoint, so the text parsed per move stays bounded. AnnotationStroke.test.ts (head) checks at every length in a 230-point stroke that the joined paths and bounds equal a full-rescan reference. It also draws 250,000 points and checks bounds and open-path length. The previous review confirmed that PickPreload.ts calls these helpers in the draw branch and keeps data-stroke-id on the group for erase. The incremental diff from the previous head to head was truncated before the stroke-call hunks, so the draw-branch wiring was not re-read at head. Residual uncertainty: low.

Full details: Description check

Explanation

The description covers the problem, change, scope and approval rationale, and verification results. It includes detailed test results and the required visual evidence for the UI change. It also identifies untested platforms and names the model and harness used.

Full details: Out of Scope Changes check

Explanation

The whole-PR summary lists only stroke-geometry changes: AnnotationStroke.ts (+69), AnnotationStroke.test.ts (+73), and PickPreload.ts (+41/-50). The incremental diff from the previous reviewed head c408a72 to head 942ca4b adds unrelated code to PickPreload.ts. That code includes a voice-dictation subsystem (new imports of ANNOTATION_VOICE_* channels and DesktopPreviewAnnotationVoice* types, voice button, voice row, and onVoiceState handler), a changed startAnnotation(sendEnabled, voice) signature, and several rewritten comments unrelated to strokes. The author attributes these to a merge of main. The +41/-50 figure does not match the added voice code, so the whole-PR diff may not reflect this head. The whole-PR diff of PickPreload.ts against merge base ade473 was not retrieved within the inspection limit, so it is not known whether these hunks belong to this PR. Missing evidence: the whole-PR diff of apps/desktop/src/preview/PickPreload.ts from ade473 to 942ca4b, or confirmation that ade473 already contains the voice and comment changes.

Resolution

Provide the whole-PR diff of apps/desktop/src/preview/PickPreload.ts from merge base ade473 to head 942ca4b. If the voice-dictation hunks and the unrelated comment rewrites are not in that diff, record PASS. If they are in that diff, rebase or merge the branch so that the PR diff contains only stroke-geometry and draw-path changes for #8059, and remove the unrelated hunks.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
External image Recent review info

External image Run configuration

External image Commits

Reviewing files that changed from the base of the PR and between c408a728d6fe68be3be77540686ab8fc35b1d536 and 942ca4b3b7c8d53635924ff7d9b327c39eb156e2.

External image Files selected for processing: 1

  • apps/desktop/src/preview/PickPreload.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Added 1 commit

  • Merge main into fix/i8059-annotation-draw-tool-performs-o-n-work-p942ca4b
Oonly21mil1d ago
Synced from GitHub

Note

🤖 Opus 5.5 and Sol 6.1 replying on behalf of Victor.

Merged main (942ca4b3b7) to clear the conflict in PickPreload.ts, where main changed startAnnotation to take (sendEnabled, voice) right next to the helpers this PR moves into AnnotationStroke.ts; the merge keeps main's signature and drops the moved helpers. The earlier Agent context budget failure came from GitHub's old merge with an older main: this PR changes no agent context, and with main merged in the budget matches main exactly. vp run check:changed and the desktop suite (1,030 tests) pass on the merge.