perf(desktop): page memo, preview debounce, chart cap, sync dirty-tracking (#311)#345
Conversation
physercoe
left a comment
There was a problem hiding this comment.
The device-sensitive part is good — the PageView memo uses the default shallow comparator (any changed prop re-renders, so it can only affect perf, never correctness), prop identity is properly stabilized at the parent, and it does not reintroduce the reverted offscreen-unrender class. One real major before merge:
Major — silent lost write (dirty-tracking race)
In librarySync.ts:183,189 and annotationSync.ts:104,110 the push loop snapshots rows, awaits the network PUT, then writes back { dirty: false } unconditionally. If the user mutates that row during the in-flight PUT — realistic, since typing a note comment fires update() per keystroke and sync is multi-row/multi-round-trip — the write-back clears dirty, so the next sync skips the row and that edit never reaches the hub (permanent silent divergence). On main every sync re-PUTs, so the edit lands on the next pass; this is a regression. Fix: clear dirty only if the row is unchanged since push start (snapshot updatedAt/a generation counter and compare in the write-back).
Minors (judgment calls): multi-series line charts of differing length now render across a shared x-domain rather than each stretched full-width (ChartView.tsx:225,283) — visible change for late-joining series; downsampling (ChartView.tsx:156-210) is envelope-preserving but not surfaced (an aria/title hint when points.length > MAX_POINTS would satisfy "truncation must be signalled").
Nit: openWebTab handed to OpenLinkContext.Provider (ReadSurface.tsx) is recreated each render — wrap in useCallback to fully close the memo gap.
c2986c0 to
c26802d
Compare
|
Thanks for the careful review — all points addressed in c26802d (branch rebased onto current main, which pulled in #343's Major — silent lost write (dirty-tracking race). Fixed exactly as suggested. Both push loops now route the post-PUT write-back through a Minor — truncation signal. Done: when any series exceeds the 600-point cap, the chart's Minor — shared x-domain (kept, trade-off). You're right that multi-series charts of differing lengths now share one x-domain instead of each stretching full-width. Kept deliberately: per-series index-stretch was the mechanism that made overlaid run curves lie — a late-joining series at step 60 rendered its last point at the same x as a 100-step series' step 100, i.e. the overlay visually compared different x positions as if equal. The shared domain is what makes the downsampled envelope x-faithful (the issue's "x-position fidelity" requirement) and makes the overlay honest. For single-series charts and uniform-x data the rendering is pixel-identical to before; the visible change is confined to multi-series overlays with differing lengths/non-uniform x, where the old rendering was arguably wrong. CompareSurface (the named live-poll consumer) always supplies real x (steps), so its overlays are now correctly aligned. Nit — Re-verified after rebase: |
physercoe
left a comment
There was a problem hiding this comment.
Re-review of fix c26802d — all findings addressed. The writeBack helper clears dirty only on object-identity match (cur === r); both stores swap the row object on mutation, so an edit during the in-flight PUT keeps dirty:true and re-syncs next pass. No infinite-loop risk (dirty-cleared rows skipped next pass). Chart-cap surfaced via aria hint; shared x-domain guarded against divide-by-zero; openWebTab useCallback-d; the device-sensitive React.memo is untouched. Both librarySync + annotationSync fixed. (Awaiting CI green before merge.)
…ic-tail merges Since v0.3.79: - fix(terminal): tell xterm the pty is ConPTY on Windows so a repainting TUI's intermediate frames no longer pile up in the scrollback (2b26a16). - 8 epic-tail PRs merged: #341 terminal connect-phase + SSH split-duplicate, #342 PDF struct-tree a11y + annotation shortcuts, #343 token burn-down / plural / responsive, #344 PDF fit-page / rotation / hand-pan / ink, #345 perf (page memo, debounce, chart cap, sync dirty-tracking), #346 modal migration + dirty-close guards, #347 vault TOTP / ed25519 / coded errors, #348 read reconcile / AuthorNav / table nav / frame check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes #311
Implements the four remaining items from the maintainer's progress comments (everything else on the issue — table virtualization, offscreen un-render, debounced persistence,
useTstability — is already on main).What
PageViewReact.memo(ui/PdfCanvas.tsx) — wrappedPageViewinmemoand stabilized every prop it receives:onDestvia a latest-ref trampoline,onCreate/onRemove/onToolDoneviauseCallback, a sharedNO_ANNOSempty list, and structural sharing inannosByPageso an edit to one page's annotation keeps every other page'sannosarray reference-equal.tis already stable per language (3389529). Selection, the editor popover, IntersectionObserver lazy rendering, and highlight marks are untouched.surfaces/AuthorSurface.tsx) — the live preview no longer re-parses (react-markdown + rehype-highlight + KaTeX) on every keystroke; a smalluseDebouncedhook feeds it a 250ms trailing-debounced body, in step with the documents store's 400ms persistence debounce. The editor keeps the live value.ChartViewdownsampling (ui/ChartView.tsx) — series rendered as lines are capped at ~600 points via x-domain-bucketed min/max with exact endpoint pinning. Line points are now positioned by their real x (p.x ?? index) over one domain shared across series, so kept points land at their true positions (the old index spacing falls out as the uniform-x special case); y-domain is unchanged because bucket min/max preserves the global extremes. Bars/categorical rendering is untouched. CompareSurface's 8s live-polled charts get the cap automatically throughChartView.state/library.ts,state/annotations.ts,state/librarySync.ts,state/annotationSync.ts) —Reference/Annotationcarry a persisteddirtyflag: set on every local mutation (add/update/attachment/tag/collection ops; undo marks restored rows), cleared only by the successful-push write-back. The push loops skip linked, clean rows — no more full-library PUTs + write-backs when nothing changed. Unlinked rows always attempt the create/link path; a failed push leaves the row dirty so the next sync retries it; the push→pull local-wins flow and merge semantics are exactly as before (no optimistic-lock logic exists in these paths to alter — that lives in vaultputVault). Rows persisted before this change load as dirty, so the first sync after upgrade re-pushes once and establishes the clean baseline.Why — remaining main-thread stalls / wasted work from #311: every annotation edit re-rendered all PageViews; every editor keystroke re-parsed the whole preview; live polling grew chart series unboundedly; every sync re-PUT the whole library.
How verified
npm run build(sync:tokens +tsc --noEmit+ vite build) — green.scripts/lint-desktop-tokens.sh— clean (86 total off-token, ≤ baseline, no phantom tokens). No CSS/i18n/Rust changes.git diff origin/main --stat— only the 7 intended files. No IndexedDB migration (explicitly longer-term), no version/dependency changes.