Uh oh!
There was an error while loading. Please reload this page.
perf(frontend): cut per-frame work in the hero canvas, backdrop and graph (#111) - #492
perf(frontend): cut per-frame work in the hero canvas, backdrop and graph (#111)#492AndresL230 wants to merge 3 commits into
Conversation
…raph (#111) Four hot spots, no visual change. Half of #111 was already fixed on main (landing canvas reduced-motion, per-node shadowBlur, the floating-cards NodeList, the cached spotlight rect) and is untouched here. **Hero canvas link pass — 0.353ms -> 0.097ms per frame (3.6x).** An uncapped double loop measured all 25,425 pairs of the 226 projected nodes every frame to draw the ~213 that qualify — 99.2% of the distance checks were wasted. A node can only link within 70*sc px, so binning into a grid whose cell is the largest such radius means every possible partner sits in the 3x3 block around it. Extracted to lib/linkPairs.ts so the fast version can be PROVED equivalent to the obvious one rather than believed to be: the test compares them across 40 random clouds plus dense, sparse, negative-coordinate and degenerate cases, and asserts the emission ORDER matches too — these are translucent overlapping strokes, so order decides the composited image. **AtmosphericBackdrop — 14 gradients per frame -> 14 blits, at half the rate.** It rebuilt 14 full-viewport radial gradients every frame at 60fps, app-wide, mounted twice by ShellFrame. Everything a gradient depends on (colour, radius, depth-derived alpha) is fixed when the orb is created; only x/y move. Each orb is now baked once into an offscreen canvas and blitted, and the loop is capped at ~30fps — ambient drift moves well under a pixel per frame. Re-bakes only if the device pixel ratio changes. The reduced-motion still-frame path is preserved exactly, including the behaviour that turning reduced-motion on mid-session stops the loop. **KnowledgeGraph2D — three separate problems.** Ticks now coalesce into at most one React render per animation frame; d3's timer can fire more than once per frame and each render reconciled the entire SVG. This does not touch the deterministic settle: simulation.tick() does not dispatch tick events, so the reduced-motion/test-mode path still renders once, synchronously. setTooltipPos was the unconditional first statement of onPointerMove, so moving the mouse across empty canvas re-rendered the whole graph to update a value nothing reads unless a node is hovered; it is now gated on `hovered`, with the position seeded on pointer-enter so the first frame still lands in the right place. Edge rendering and the link filter both did O(E x N) linear scans; both now use a Map/Set. **Images.** Intrinsic width/height on the 19 brand-icon sites (a fixed-size asset whose box was only pinned in inline CSS, so nothing reserved it before styles resolved), and decoding="async" on the lazily-loaded ones. next/image is deliberately not adopted — zero usages repo-wide and it needs a Cloudflare Workers loader of its own. **framer-motion.** HowItWorks (~54 motion.* elements, the landing page's only consumer) is now next/dynamic, so the library leaves the critical path for a section below the fold. NOT `ssr: false` — the markup still server-renders. Study.tsx and Calendar.tsx are deliberately left alone: both wrap their screen's PRIMARY content in the motion element, so deferring it would defer the content itself, which is worse than the bundle cost it saves. One lint-suppression count moves (KnowledgeGraph2D react-hooks/refs 13 -> 14): building the id->node Map is one more use of a ref-derived value during render, in a file where that pattern is already baselined 13 times. Verified in isolation that the map is the only cause. The trade is one more instance of an existing suppressed pattern for removing a per-frame O(E x N) scan. part of #111 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reached
Next review available in:53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds spatial link-pair generation, cached canvas sprites, coalesced knowledge graph renders, dynamic loading for ChangesFrontend rendering performance
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant KnowledgeGraph2D
participant Simulation
participant requestAnimationFrame
participant Canvas
Simulation->>KnowledgeGraph2D: emit simulation tick
KnowledgeGraph2D->>requestAnimationFrame: queue one render
requestAnimationFrame->>KnowledgeGraph2D: execute render callback
KnowledgeGraph2D->>Canvas: draw resolved nodes and edges
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 463facd | Commit Preview URL Branch Preview URL | Jul 31 2026, 03:47 AM |
Two review findings. The spatial grid packed (gx, gy) into one integer, `((gx + 1024) << 12) | (gy + 1024)`, on the assumption both stay inside a bound. linkPairs is an exported general-purpose helper with no such precondition: a small `reach` makes cells small, the grid coordinates blow past the bound, two different cells hash together, the same bucket is visited twice inside one 3x3 scan — and the pair is emitted TWICE. Reproduced in review: points = [(0,0,sc1), (0,5000,sc1), (0.5,5000.4,sc1)], reach 1 naive -> 1 pair; binned -> the same pair, duplicated Not reachable from the hero canvas (its coordinates and scales keep the grid tiny), but the file claims equivalence for all inputs, and "not reachable today" is not a property an exported helper should rely on. Now a Map-of-columns, which has no bound to blow. Same speed: 0.356ms -> 0.100ms, 3.5x, unchanged from the packed version. The repro is now a regression test. The nodeById comment claimed memoising would trade one lint violation for another. Measured during review: `useMemo` reports the SAME react-hooks/refs count, so that reasoning was simply wrong. The real reason not to memoise is that `simNodes` is a ref's array mutated in place — its identity would not change when its contents do, which makes it an unsound dependency. Comment now says that instead. part of #111 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#111) The throttle returned before the physics step, so orb positions advanced once per PAINTED frame rather than once per rAF — at 30fps that is half the original drift speed. A visible change, in a PR whose whole claim is that there isn't one. Each step is now scaled by how many 60fps-equivalent frames actually elapsed, so the apparent speed matches the 60fps original at any cadence. Clamped to 4 frames so a backgrounded tab (or the very first frame, when lastPaint is still 0) cannot teleport the field on resume. Caught by reading PR #490, which independently hit the same trap and handled it — my own review pass checked the throttle for stalls and for the reduced-motion still frame, but not for the drift rate. part of #111 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndresL230
commented
Jul 31, 2026
|
| #490 | #492 (this) | |
|---|---|---|
| hero link pass | axis-reject + squared distance — still O(N²), cheaper constant | spatial binning, sub-quadratic, extracted + equivalence-tested |
| KnowledgeGraph2D ticks | direct DOM writes, zero React work per tick | rAF-coalesced forceRerender — React still reconciles once per frame |
| backdrop | sprites + 30fps + pauses on visibilitychange | sprites + 30fps, no visibility pause |
| HowItWorks | next/dynamicssr: false + placeholder | next/dynamic, SSR kept |
| Study.tsx | motion subtree extracted to StudyMotion.tsx | left alone deliberately |
| suppressions baseline | pruned 13 → 12 | grown 13 → 14 |
| local E2E lane | could not run (machine unprovisioned) | ran green, repeatedly |
Where #490 is better: the direct-DOM-write graph is the deeper fix and is what the issue's own proposal recommended; the visibilitychange pause is a real win this PR lacks; extracting StudyMotion is more than I did; and pruning the suppressions baseline beats growing it.
Where this PR is better: the link pass is asymptotically better, not just cheaper per pair, and its equivalence to the naive version is proved by test rather than asserted. ssr: false on HowItWorks removes a landing-page section from the server-rendered HTML — a crawler regression on the one page that needs SEO. And this branch has actually been through the local E2E lane.
Reading #490 also caught a real bug in this PR that my own review missed: my 30fps cap advanced the orb physics once per painted frame, so drift ran at half speed — a visual change, in a PR claiming none. #490 handled that explicitly ("drift speed preserved via elapsed-frame scaling"). Fixed in 463facd.
My suggestion, for whoever decides: take #490 as the base — it is the more thorough pass and the graph fix is better — and port two things onto it from here: the binned lib/linkPairs.ts with its equivalence suite, and dropping ssr: false from the HowItWorks dynamic import. But that is a call for @AndresL230, not for me.
…Works copy - AtmosphericBackdrop: disabling reduced motion mid-session now restarts the parked RAF loop (tick() parks itself on a still frame; the old always-running loop resumed implicitly, so the backdrop stayed frozen). - Landing: drop ssr:false from the HowItWorks dynamic import (#492 review) — next/dynamic still splits the motion stack into its own chunk, but the section's marketing copy is back in the server-rendered HTML; verified "Upload Your Materials" present in the prerendered index.html. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Brings in #341 (FullHeightScreen viewport-math), #290 (approval gate) and #288 (hero card). The globals.css conflict is resolved by keeping BOTH added blocks: this branch's .study-mode-enter and main's .pending-* beat. Ordering invariants re-verified after the merge — .card--hero still follows .card, and .pending-* still precedes the .anim-d* delay utilities (the animation shorthand resets animation-delay, so those have to come later to win). Also fixes the intermittent frontend-lane CI failure, which is unrelated to either PR but was red-flagging both. FlashcardImportModal.test.tsx wiped document.body in afterEach but never unmounted; with `globals: false` in vitest.config.ts, @testing-library/react does not auto-register its cleanup, so React kept the tree and its pending scheduler work alive past the end of the file and jsdom teardown landed it on a missing `window`: ReferenceError: window is not defined vitest counts those as unhandled errors and exits non-zero even with a fully green suite — which is exactly how #492 failed with 416/416 tests passing. Now calls cleanup() before the body sweep (both are needed; the sweep clears the Dialog/ToastProvider portal siblings cleanup() leaves behind). Verified 3/3 clean full-suite runs, exit 0, zero unhandled errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* perf(frontend): #111 runtime animations + lazy image loading - Landing hero: squared-distance link pass with axis rejects (no more Math.hypot over all ~27k node pairs/frame); framer-motion split out of the landing chunk via next/dynamic (HowItWorks) with a layout-stable placeholder. - AtmosphericBackdrop: orb gradients pre-baked to offscreen sprites (re-baked only on DPR change), RAF throttled to ~30fps with drift speed preserved, loop paused while the tab is hidden. - KnowledgeGraph2D: simulation ticks write node/edge positions directly to the DOM (zero React work per tick), tooltip setState only on open/reseed with direct style writes while shown, id->node Map replaces O(E*N) find(); testmode tests read the new transform position carrier. eslint-suppressions baseline pruned (13->12). - Images: loading="lazy" + decoding="async" on all raw <img> sites; intrinsic width/height added where the rendered box is fixed (TopNav/SideNav logos, Settings avatar/banner) and deliberately omitted for natural-aspect user uploads (Social attachments, Admin screenshots). - Study: framer-motion subtree moved to StudyMotion.tsx behind next/dynamic (ssr:false) with visually identical fallbacks; the #383 skipAnimations test seam moved with it. Verified: 55 files / 399 vitest tests, eslint 0 errors, tsc clean, production build green. Local E2E lane not runnable on this machine (unprovisioned: no backend/.env, supabase CLI/config.toml mismatch, bash 3.2 parser bug in scripts/lib/local-common.sh:99); e2e.yml covers the lane on push to main. Closes#111 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(perf): #490 review round — sprite memory cap, un-gated Study panes, eager nav logos - AtmosphericBackdrop: cap sprite backing resolution at 512px (was full device resolution — ~100 MB of canvas memory at DPR 2 across 14 orbs); the soft gradients upscale indistinguishably. Also repaint the static frame on resize under reduced motion (canvas.width resets cleared it). - Study: the mode transition no longer routes pane content through the lazy motion chunk — next/dynamic's fallback can't carry children, which gated the panes behind the chunk fetch, and un-gating them remounted the pane mid-session, wiping state (caught by Study.test.tsx). Replaced with a keyed CSS enter-fade (`study-mode-enter`) that needs no chunk; AnimatePresence initial={false} semantics preserved via render-phase state derivation. StudyMotion.tsx keeps only the toggle highlight and the #383 test seam. - TopNav/SideNav: drop loading="lazy" from the always-visible nav logos (lazy only delays permanent chrome); keep decoding + dimensions. - Landing: document the ssr:false SEO tradeoff on the HowItWorks split. Verified: 399/399 vitest, eslint 0 errors, tsc clean, production build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(perf): #490 round 2 — revive backdrop on un-reduce, SSR the HowItWorks copy - AtmosphericBackdrop: disabling reduced motion mid-session now restarts the parked RAF loop (tick() parks itself on a still frame; the old always-running loop resumed implicitly, so the backdrop stayed frozen). - Landing: drop ssr:false from the HowItWorks dynamic import (#492 review) — next/dynamic still splits the motion stack into its own chunk, but the section's marketing copy is back in the server-rendered HTML; verified "Upload Your Materials" present in the prerendered index.html. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: AndresL230 <190146319+AndresL230@users.noreply.github.com>
AndresL230
commented
Jul 31, 2026
Superseded by #490, which is merged. @AndresL230's call was to take Jose's PR as the base, port anything worthwhile from here, verify it, and merge — which is what happened. Recording what carried over and what didn't: Carried over: the Deliberately not carried over: the spatially-binned Also not carried over: the rAF-coalesced tick render. #490 writes simulation positions directly to the DOM, which is strictly better — and is what issue #111's own proposal recommended. The one thing this PR did that #490 hadn't: run the full local E2E cycle. That has now been run against the merged head — 35/35 journeys, oracles clean. Opening this without checking for an existing open PR on the same issue was my mistake. |
Four hot spots, no visual change. Half of #111 was already fixed on main and is untouched here — the landing canvas already respects reduced-motion, per-node
shadowBluris gone (0 hits repo-wide), the floating-cards RAF already resolves its NodeList once, and the spotlight rect is already cached. Those claims in the issue body are stale; this PR is only the genuinely remaining work.1. Hero canvas link pass — measured 0.353ms → 0.097ms per frame (3.6×)
An uncapped double loop measured all 25,425 pairs of the 226 projected nodes every frame, to draw the ~213 that actually qualify. 99.2% of the distance checks were wasted.
A node can only link within
70 * scpx, so binning into a grid whose cell is the largest such radius means every possible partner sits in the 3×3 block around it and nothing else is ever measured.Extracted to
lib/linkPairs.tsso "identical output" is provable rather than asserted:linkPairs.test.tscompares it to the all-pairs reference across 40 random clouds plus dense, sparse, negative-coordinate, asymmetric and degenerate cases — and checks the emission order matches, because these are translucent overlapping strokes and order decides the composited image. One test also guards against a future "simplification" quietly restoring the double loop.2. AtmosphericBackdrop — 14 gradients per frame → 14 blits, at half the rate
It rebuilt 14 full-viewport radial gradients every frame at 60fps, app-wide, mounted twice by
ShellFrame. Everything a gradient depends on — colour, radius, depth-derived alpha — is fixed when the orb is created; only x/y move.Each orb is baked once into an offscreen canvas and blitted; the loop is capped at ~30fps (ambient drift moves well under a pixel per frame). Re-bakes only when the device pixel ratio actually changes.
The reduced-motion still-frame path is preserved exactly, including the existing behaviour that switching reduced-motion on mid-session stops the loop.
3. KnowledgeGraph2D — three problems
.on("tick", forceRerender)reconciled the whole SVG per physics tick. Ticks now coalesce into at most one render per animation frame. This does not touch the deterministic settle:simulation.tick()does not dispatch tick events (only the internal timer does), so the reduced-motion/test-mode path still renders once, synchronously, and stays deterministic.setTooltipPoswas the unconditional first statement ofonPointerMove— moving the mouse over empty canvas re-rendered the entire graph to update a value nothing reads unlesshoveredis set. Now gated, with the position seeded on pointer-enter so the first frame still lands correctly..find()per endpoint per edge per frame; the link filter did two.some()scans per edge. Both now use a Map/Set.The offscreen
IntersectionObserverpause and unmount cleanup are preserved, and the queued tick render is cancelled on unmount so it can't fire against a dead tree.4. Images and framer-motion
Intrinsic
width/heighton the 19 brand-icon sites (a fixed-size asset whose box was only pinned in inline CSS, so nothing reserved it before styles resolved), anddecoding="async"on the lazily-loaded ones.next/imagedeliberately not adopted, per your call.HowItWorks(~54motion.*, the landing page's only framer-motion consumer) is nownext/dynamic, so the library leaves the critical path for a section below the fold. Notssr: false— the markup still server-renders.Study.tsxandCalendar.tsxare deliberately left alone. The brief listed them, but both wrap their screen's primary content in the motion element — deferring it would defer the content itself, which is worse than the bundle cost it saves.Calendar.test.tsx's framer-motion mock stays green either way.One suppression count moves
KnowledgeGraph2Dreact-hooks/refs13 → 14. Building the id→node Map is one more use of a ref-derived value during render, in a file where that pattern is already baselined 13 times. I verified in isolation that the Map is the sole cause (restoring.find()returns it to 13), and tried three formulations — all cost the same. The trade is one more instance of an existing suppressed pattern in exchange for deleting a per-frame O(E·N) scan.Gates
tsc --noEmitclean ·npm run lint0 errors (35 warnings, one fewer than before) ·npx vitest run59 files, 415 testspart of #111
Summary by CodeRabbit
Performance Improvements
Tests