Uh oh!
There was an error while loading. Please reload this page.
feat(landing): interactive knowledge graph replaces the scroll section (#344 phase 2) - #513
Merged
Conversation
Interactive knowledge graph replaces the 340vh scroll-jacked HowItWorks, then three feature bands and a four-tile bento of real app surfaces. The structural argument is a density rhythm — the graph is the densest thing on the page, bands decompress, the bento re-energizes, and a closing band gives the CTA a run-up rather than ending on a grid tile. The content argument matters more than the visual one. Tutor chat, Notes, Gradebook and Flashcards are shipped surfaces the current six-feature list never mentions, so the page undersells the product more than it under-designs it. That is the likeliest root of #344's 'feels generic'. Live LLM generation on the public page was considered and rejected for now: most convincing option, but it puts an unauthenticated billable endpoint on the most-crawled page on the site. Recorded as a second pass behind a rate-limited endpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seven tasks: fixtures, layout+helix math, component, assembly, interaction, page wiring with the HowItWorks/catalog deletions, and the e2e journey. Self-review found one real gap and it is recorded in the plan rather than papered over: the spec lists drag alongside hover and expand, and Task 5 implements hover and the copy fade only. Dragging needs a pointer-capture and SVG coordinate-mapping decision (getScreenCTM) that deserves its own review gate, so it is called out as 5b/5c rather than hidden inside a step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Static, fully laid-out KnowledgeGraphDemo component: a course-picker chip row plus an inline SVG render of the selected course's graph via radialLayout. This is the parked frame reduced-motion visitors and the E2E lane get; the assembly animation is a later task. Registers the landing-graph E2E surface (docs/frontend-testids.md + eslint.config.mjs) and adds the .landing-page-scoped chip/copy CSS.
Adds the RAF-driven helical assembly to KnowledgeGraphDemo, extracted into an AssemblingGraph child keyed by course id so switching courses remounts (fresh progress state, unmount-driven RAF cleanup) instead of resetting state inside an effect — avoids the react-hooks/set-state-in-effect anti-pattern that the brief's literal snippet would have tripped. Parked (progress=1, full opacity, laid out) whenever IS_TEST_MODE or prefers-reduced-motion. Adds a guarded window.matchMedia stub to vitest.setup.ts (jsdom has none) defaulting to reduced-motion=true, so every KnowledgeGraphDemo unit test asserts the parked/complete frame — the correct target for reduced-motion visitors and the E2E lane alike. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
) Fix round 1: reading window.matchMedia directly in KnowledgeGraphDemo's render body computed "no preference" server-side (no window) but the real value client-side, producing a genuine React hydration mismatch for any reduced-motion visitor once this component mounts with SSR on. Adds usePrefersReducedMotion (frontend/src/lib/usePrefersReducedMotion.ts), following the same useSyncExternalStore + fixed getServerSnapshot pattern useIsMobile.ts already established for this bug class. Server snapshot defaults to true (assume reduced motion) rather than useIsMobile's false default: whichever direction is wrong pays a cost, and defaulting true means the cost lands on no-preference visitors (one extra replay of the entrance animation) rather than reduced-motion visitors (who would otherwise see a blank or mid-assembly graph on first paint). Also switches AssemblingGraph's progress from raw state to a value derived from `parked` at render time, so a post-hydration correction of `parked` (without a remount) resolves correctly in both directions instead of potentially getting stuck. Adds a renderToString -> hydrateRoot regression test using different matchMedia values across the two phases, verified to fail against the old render-body-read pattern before confirming it passes against the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fix round 2: usePrefersReducedMotion's module-level MediaQueryList cache (correct for production) survived across it() blocks within KnowledgeGraphDemo.test.tsx, so the round-0 test 'parks fully assembled when reduced motion is requested' inherited an earlier test's warmed cache instead of its own local matchMedia override. It still passed, but for the wrong reason -- false confidence, not regression protection. Reviewer proof: inverting that test's override to report "no preference" left the whole file green. Adds a file-level beforeEach calling __resetReducedMotionStoreForTests() so every test in the file cold-starts the cache and actually depends on its own window.matchMedia. Removes the now-redundant reset from the round-1 describe block's nested beforeEach (the mid-test reset between its two installReducedMotion() calls stays, since a beforeEach only runs once per test). Verified by the same inversion the reviewer used: fails whole-file with the override flipped, passes again once restored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hover a node to reveal its concept blurb below the graph; the first interaction fades the instructional copy for the rest of the session. Both `hovered` and `engaged` live in the KnowledgeGraphDemo parent, not in the AssemblingGraph child, since that child remounts (keyed by graph.id) on every course switch. engaged must survive that remount; hovered's home follows since the blurb paragraph renders in the parent too. AssemblingGraph just gets onNodeEnter/onNodeLeave callback props. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…#344) Final whole-branch review of the landing knowledge-graph demo turned up six findings. They interact, so this is one wave. #1 The helix played where nobody could see it. The assembly effect fired on mount, so it burned its full 1100ms during the hydration window — under the page's own intro overlay, alongside the hero canvas RAF — and every visitor who scrolled down found progress === 1 and a static picture. Gate the RAF on an IntersectionObserver on the section. The root margin is a NEGATIVE bottom inset, not the positive lead-in that looks natural: the section sits directly after a min-h-screen hero, so its top edge is at exactly 100vh and any positive bottom margin re-creates the bug at scroll 0. #6 No-preference first paint went blank. usePrefersReducedMotion correcting its SSR-safe `true` to the real `false` dropped progress onto a raw animatedProgress of 0 — a committed frame with the whole graph at opacity 0 — and gating #1 would have turned that into a section that stays blank until scrolled to. animatedProgress is now `number | null`, and null (the assembly has never run) reads back as 1. Leaving the viewport mid-assembly settles on the complete frame rather than freezing a half-faded one. #4 The helix threw the outer ring outside the viewBox. `1 + (1 - e) * 0.9` put depth-2 nodes at y = 693 against a 560-unit viewBox, chopped by the svg viewport at opacity ≈ 0.5. It now contracts (0.55x → 1x) instead of stretching, so the whole sweep lives inside the disc radialLayout already fits to the frame. helixEntry(target, centre, 1) === target is untouched. #3 Illegible on phones. One 900x560 viewBox at every width renders at 0.38 scale on a 390px viewport: 4.6 CSS px labels, a 213px-tall smudge. Added a GraphView descriptor and a 360x300 phone view selected by useIsMobile — 0.95 scale, 12.35 CSS px labels, 22.8px dots, 285px tall. #2 The engaged copy fade failed WCAG AA permanently (engaged never resets). --text at 0.35 over the paper bg is 2.20:1 against a 3:1 bar. The fade moves to the headline alone at 0.55 (3.88:1). The eyebrow is not faded: at 0.7rem it needs 4.5:1, and --brand-forest only holds that to alpha 0.86 — the review's suggested 0.75 is 3.58:1 and still fails. #5 The named SSR guard didn't guard. public-seo.spec.ts asserted only Metadata API output, which survives `ssr: false` on the dynamic import. Added a raw-HTML assertion on the graph section's server-rendered copy. Tests: layout.test.ts sweeps every node of every fixture across the whole t range in both views and asserts circle + label extents stay in frame (this is the test that would have caught #4); KnowledgeGraphDemo.test.tsx drives a fake IntersectionObserver and captured rAF to pin "complete before armed, never blank", "no RAF until on screen", and the WCAG ratios computed from the rendered opacity; landing-graph.spec.ts measures label/dot/height in CSS px at 390x844. Each was confirmed to fail against the pre-fix code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five visual/brand defects found by screenshotting the built page against the brand guide. Every automated gate was already green, so none of these were test-detectable. 1. TIER_COLOR was a fourth inlined copy of the mastery palette with four wrong literals — the landing page advertised different mastery colours than the product. Now consumes the canonical --state-* tokens (globals.css:80-89). The hero legend's four swatches get the same treatment (colours only). 2. The course root inherited its fixture tier's amber, so the section's focal point read as a warning. It now paints --brand-forest as an anchor; the fixture tier is unchanged. 3. The SVG used a hardcoded viewBox="0 0 900 560" stretched to a ~1184px container, reserving ~737px of height around content that clustered in the middle. The frame is now DERIVED (fitViewBox) from the drawn content — dot, label, halo — swept across the whole entry animation, unioned over all three fixtures so a chip click can't change the section height, once per breakpoint. Desktop resolves to "159 42 578 498", 36% narrower than the box it replaced. Fitted to the HELIX SWEEP, not the settled positions: helixEntry rotates 1.5 turns, so an outer node passes 0.925*maxRadius above and below the centre at ~83% opacity — 215 units against a settled extent of 58. A settled fit clips the assembly, which is #344 review #4 all over again. The bounds test is retargeted at the derived box, and a new paired test proves the sweep genuinely leaves the settled bounding box so that containment check stays load-bearing. The fit only pays off with a width cap — stretched to 1184px a tighter box renders at 2x and makes the section taller. Capped at md:max-w-[720px]. 4. The root's label lay along the outer-ring -> depth-1 edge, which crosses the root's x at cy + 0.349*ring in every fixture. It moves above the node on desktop; on the phone the ring is too small for that to clear the top child's label, so it stays below (where the diagonal doesn't reach it). All labels gain a paint-order halo in --bg-mesh. Two unreported collisions also fixed: "Hypothesis Tests" overlapped "Distributions" by ~21 CSS px at 390px, and the shipped phone geometry both overlapped and, under the fit, fell under the E2E height gate. New suite pins label/label, label/dot and label/edge clearance for every fixture in both views. 5. Left-aligned the graph to the headline's grid — the same width cap — so a left-aligned copy block no longer sits above a centred diagram. 503 unit tests pass (up 6), tsc and eslint clean. No new CSS, no new dependency, no assertion weakened. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ring phase was `offset = d % 2 === 0 ? step / 2 : 0` against a −π/2 start. Depth 2 holds exactly two nodes in all three fixtures, so `step = π`, the half slot is `π/2`, and both outer nodes resolved to exactly 0 and π — dead on the horizontal axis through the centre. The settled layout was therefore a flat ellipse (aspect 0.40) inside a near-circular entry sweep (0.86), and since the viewBox is fitted to the sweep (correctly — a settled fit clips the assembly), the graph filled only 45% of its own box height and left a dead band above and below it. Every ring now starts three quarters of a slot back from 3 o'clock, so its angles are odd multiples of `step/4 = π/(2·count)` and can never be a multiple of π: no ring of any size flattens onto the horizontal axis. The half-slot alternation is kept for what it was for — it now applies only when a ring holds the same node count as the one inside it, which is the only case that lines up into radial spokes. Depth 1 is bit-identical to before (−3·(2π/3)/4 is exactly −π/2), so the reviewed triangle and every constraint the mobile geometry was tuned against are untouched. Depth 2 moves to the NW↔SE diagonal; the mirrored diagonal is not equivalent — the fixtures hang their outer nodes off the 12 and 4 o'clock children, so SW↔NE drags an edge through the root label (−7.2 units of overlap, measured). Desktop, re-derived from the new geometry and still fitted to the sweep: `159 42 578 498` → `185 33 526 516`; settled aspect 0.436/0.405/0.418 → 1.006/0.910/0.951; the drawing fills 73% of the frame height, up from 45% (515 of 706 rendered px, up from 277 of 620). Phone: `-19 5 394 319` → `-4 -1 364 330`, aspect 0.485/0.424/0.450 → 1.006/0.844/0.910, fill 48% → 73%, and every E2E legibility bar clears wider than before (12.8px labels, 25.5px dots, 301px tall). Worst label clearance is unchanged at +29.2 units desktop and +0.8 mobile — the new angles introduce no new binding pair. The "sweep leaves the settled box" guard is restated in units per side (59.0/59.3 desktop, 33.2/33.9 mobile) instead of as a ratio: the ratio was a proxy that shrinks precisely when the rest state stops being flat, which is the fix. New tests pin the no-horizontal invariant over ring sizes 1–12, the spoke-breaking branch, and the settled fill for every fixture × view. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#344) `radialLayout` placed nodes on global concentric rings: a node's angle came from its index within its depth, with no relationship to where its parent sat. On the shipped 6-node fixtures that scattered the graph — `cs-sorting` landed 171 units from `cs-arrays` on a 232-unit ring, `cs-trees` was flung to the opposite corner — and the picture read as a lopsided diagonal smear. Tuning the ring angles cannot fix it; the previous wave tried. Now: BFS a spanning tree over the undirected `edges` (never `children`, which is dead data that disagrees with them), root at the centre, depth-1 spread evenly around the circle, and every deeper node placed one ring step from ITS OWN PARENT inside a wedge centred on that parent's outward direction. The tree governs position only — every entry in `graph.edges` is still drawn, cross edges included. Tree-edge length, desktop: max 170.9 → 116.0, mean 128.5 → 116.0. The section gets shorter at the 720px cap: 706px → 663px. The depth-1 ring phase moves forward by one slot (`+step/4`, still an odd multiple, so the no-horizontal proof is unchanged) so the two branch-bearing children sit at 4 and 8 o'clock and grow downward, away from the root's label band. `MOBILE_VIEW` is retuned, not optional: in a tree the flattest of three arms is always 30° off horizontal, so a child's dot lands inside its parent's own 13-character label unless `0.866·ring > 3.9·font + halo/2 + nodeR`. The ring grows 132 → 163 and `nodeR`/`fitPad` shrink to buy it; the phone renders at 11.2px labels / 20.9px dots / 285px tall, still over the E2E legibility gate but with thin margins, documented at both ends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…middle (#344) The radial tree put the course root at the frame's centre and spread its concepts around the full circle, so the drawing grew one arm up and two down — an inverted Y, or a root system. The product is called Sapling and the section headline is "Pick a course. Watch it grow." Only the ANGULAR DOMAIN changes. The BFS spanning tree, the parent-relative placement and the drawing of every edge (cross-edges included) are exactly as the previous wave left them. - depth-1 fans across the UPWARD half-plane, taking the interior gridlines of an (n+1)-way split of it: 45/90/135 degrees for three children. Never lands a child on the horizon at any fan size, and strictly further from it than the previous rule at every size. - seats are handed out outside-in, biggest subtree first, so the deep arms get the open sky and the composition stays mirror-symmetric. - the skeleton is laid out with the root at the origin and translated as a rigid body until its own bounding box is centred on the layout centre — the point helixEntry spirals around. The root ends up at the bottom-centre of the content and the sweep-fitted frame stays centred on the drawing. - both views retuned against the new geometry (the budget is now solved from the type scale, the label clearances and the phone's legibility floor, not inherited from the layout box), and the root's label moves below its circle in both: above is now the direction the plant grows in. Measured, desktop at the 720px cap: the <svg> is 637.6px tall against 662.8 (and 737 for the box this shipped with); cs210 fills 0.839 of the frame's width against 0.828; the worst label clearance goes 8.80 -> 30.14 units. Phone: 11.99px labels and 22.48px dots against 11.23/20.85, and the worst label clearance goes 0.99 -> 4.05 CSS px, retiring a margin that was inside the noise of whether Chromium paints a scrollbar. The frame's HEIGHT share does not improve (0.563 -> 0.532): fitViewBox fits to the entry sweep, the sweep is very nearly a disc, so dead vertical space is about (drawing width - drawing height)/2 for any layout and a canopy is wider than tall. Documented at the assertion that was relaxed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The frame is fitted to the helix sweep, not to the settled drawing — it has to be, or the assembly gets clipped mid-flight (#344 review #4). At 1.5 turns every node passed through every direction on the way in, so that sweep was very nearly a DISC and the fitted frame very nearly SQUARE around a canopy twice as wide as it is tall. The difference was dead paper: 162 and 136 CSS px of it above and below the drawing, which the previous wave measured across the whole layout family and logged as its closing concern — the lever is the sweep, not the layout. `helixEntry` now turns 0.25 (90°) and `ENTRY_CONTRACTION` is 1, so a node starts on the centre and the radial term collapses to the straight-line easing: the path is that line, rotated by a decaying quarter turn, and the widest swing happens where the node is nearest the centre. No layout, fixture, colour, copy, testid or component signature moved; every settled coordinate is byte-identical. desktop viewBox 161 17 603 534 -> 161 114 582 334 <svg> 720x637.6 -> 720x413.2 px (-224) band/side 162.2/136.0 -> 48.1/13.4 px phone viewBox -32 -25 443 365 -> -32 40 429 230 <svg> 332x273.5 -> 332x178.0 px (-95) band/side 63.7/54.2 -> 15.5/1.8 px The drawing did not shrink to get there — it grew, because the narrower frame renders every unit bigger: 669x339 -> 694x352 CSS px on desktop, 12.0 -> 12.4 px phone labels, 22.5 -> 23.2 px dots. Worst label clearance is unchanged in units (30.14 desktop, 5.40 phone) and better in pixels (36.0 -> 37.3, 4.05 -> 4.18). Tests: the mid-flight helix assertion is restated as shape rather than distance (>0.1 travel radii off the straight line, >5 deg of bearing swing; measured 0.172 and 11.25 deg) so it bites at 0.146 turns instead of passing anything; the sweep-vs-settled guard now asserts the top overhang with a number and builds the settled-fit box to show it clips, because the overhang is one-sided by construction (the fan's tips lift past the settled top when they rotate through the vertical; nothing can swing below the course code); the frame-height share floor goes 0.50 -> 0.75 (measured 0.851/0.903); and the phone's third bar stops measuring the FRAME's height (it was passing on the band) and measures the drawing's, with a frame floor kept at 170px. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 7e4ea24 | Commit Preview URL Branch Preview URL | Aug 02 2026, 07:04 AM |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughChangesLanding knowledge graph and product surfaces
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Visitor
participant LandingPage
participant KnowledgeGraphDemo
participant COURSE_GRAPHS
Visitor->>LandingPage: open landing page
LandingPage->>KnowledgeGraphDemo: load graph section
KnowledgeGraphDemo->>COURSE_GRAPHS: select course graph
COURSE_GRAPHS-->>KnowledgeGraphDemo: return nodes and edges
KnowledgeGraphDemo-->>Visitor: render responsive SVG graph
Visitor->>KnowledgeGraphDemo: select course or hover node
KnowledgeGraphDemo-->>Visitor: update graph and explanatory copy
LandingPage-->>Visitor: render feature bands and bento surfaces
Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/e2e/landing-graph.spec.ts (1)
87-102: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAdd
landing-graph-svgto the spec selector inventory.
landing-graph-svgis attached to the<svg>element inKnowledgeGraphDemo.tsxand documented indocs/frontend-testids.md, butlanding-graph.spec.tslines 11-15 still list onlylanding-graph-svg’s sibling selectors. Addlanding-graph-svgto the file’s selectors list so this E2E surface stays documented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/landing-graph.spec.ts` around lines 87 - 102, Add landing-graph-svg to the selector inventory near the existing landing graph selectors in landing-graph.spec.ts, preserving the current list and formatting so the SVG test surface is documented.Source: Coding guidelines
frontend/src/app/globals.css (1)
998-1003: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe headline transition is not gated on
prefers-reduced-motion.
AssemblingGraphparks the SVG assembly for reduced-motion visitors, but this 600ms opacity transition still runs for them. The chip transitions at Line 990 have the same gap. An opacity fade is a mild effect, so this is optional. If you want the section fully consistent with the parked contract, add a guard.♻️ Optional guard
.landing-page .landing-graph-headline { transition: opacity 600ms ease; } +@media (prefers-reduced-motion: reduce) {+ .landing-page .landing-graph-headline,+ .landing-page .landing-graph-chip { transition: none; }+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/app/globals.css` around lines 998 - 1003, Optionally gate the 600ms opacity transition on .landing-page .landing-graph-headline and the chip transitions near this rule with the existing prefers-reduced-motion handling, disabling transitions for reduced-motion users while preserving current animations otherwise.frontend/src/components/marketing/graph/KnowledgeGraphDemo.test.tsx (1)
96-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
installReducedMotionandinstallViewportnever restorewindow.matchMedia.Both helpers overwrite the global and leave it overwritten. The later describe blocks at Lines 194-230 and Lines 581-652 install nothing, so they run against whatever a previous block last installed. The assertions there still pass today only because the leaked state and the
vitest.setup.tsdefault happen to agree on "desktop, reduced motion". A reordering, a.only, or a change to one helper breaks unrelated tests.Save and restore the previous value.
♻️ Proposed refactor
+const originalMatchMedia = window.matchMedia;+afterEach(() => {+ window.matchMedia = originalMatchMedia;+});+ /** Controllable `window.matchMedia`, scoped to the reduced-motion query. */ function installReducedMotion(matches: boolean) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/marketing/graph/KnowledgeGraphDemo.test.tsx` around lines 96 - 114, Update installReducedMotion and the analogous installViewport helper to capture the existing window.matchMedia before replacing it, then provide restoration through each helper’s returned handle or cleanup path. Ensure every test that installs either mock restores the original value after use, so later describe blocks observe the environment established by vitest.setup.ts rather than leaked state.frontend/src/components/marketing/graph/layout.test.ts (1)
120-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive depth-1 from the BFS tree, not from
children.The file header at Lines 26-28 states
DemoNode.childrenis dead data that disagrees withedges, and that the layout must not read it. This test reads it. Ifchildrenlists an id thatedgesdoes not connect,p.get(id)returnsundefinedand the distance becomesNaN, which fails with an unclear message. ThebfsTreehelper already gives an independent depth map.♻️ Proposed refactor
it('separates nodes at the same depth', () => { const p = radialLayout(G, 800, 500); - const depth1 = G.nodes.find((n) => n.id === G.rootId)!.children;+ const { depth } = bfsTree(G);+ const depth1 = G.nodes.filter((n) => depth.get(n.id) === 1).map((n) => n.id); const pts = depth1.map((id) => p.get(id)!);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/marketing/graph/layout.test.ts` around lines 120 - 130, Update the “separates nodes at the same depth” test to derive depth-1 node IDs from the existing bfsTree helper and the graph’s edges rather than G.nodes[].children. Preserve the current point collection and distance assertions, while ensuring every tested ID is present in the BFS-derived layout input.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/marketing/graph/KnowledgeGraphDemo.tsx`:
- Around line 188-194: Add aria-live="polite" to the concept blurb paragraph so
screen readers announce its content changes. Limit this change to the blurb
element; defer keyboard focus handlers and node accessibility updates until the
graph nodes become focusable.
---
Nitpick comments:
In `@frontend/e2e/landing-graph.spec.ts`:
- Around line 87-102: Add landing-graph-svg to the selector inventory near the
existing landing graph selectors in landing-graph.spec.ts, preserving the
current list and formatting so the SVG test surface is documented.
In `@frontend/src/app/globals.css`:
- Around line 998-1003: Optionally gate the 600ms opacity transition on
.landing-page .landing-graph-headline and the chip transitions near this rule
with the existing prefers-reduced-motion handling, disabling transitions for
reduced-motion users while preserving current animations otherwise.
In `@frontend/src/components/marketing/graph/KnowledgeGraphDemo.test.tsx`:
- Around line 96-114: Update installReducedMotion and the analogous
installViewport helper to capture the existing window.matchMedia before
replacing it, then provide restoration through each helper’s returned handle or
cleanup path. Ensure every test that installs either mock restores the original
value after use, so later describe blocks observe the environment established by
vitest.setup.ts rather than leaked state.
In `@frontend/src/components/marketing/graph/layout.test.ts`:
- Around line 120-130: Update the “separates nodes at the same depth” test to
derive depth-1 node IDs from the existing bfsTree helper and the graph’s edges
rather than G.nodes[].children. Preserve the current point collection and
distance assertions, while ensuring every tested ID is present in the
BFS-derived layout input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b4c050a-e999-4666-84e5-d30485bc68bb
📒 Files selected for processing (17)
docs/frontend-testids.mddocs/superpowers/plans/2026-08-01-landing-graph-step1.mddocs/superpowers/specs/2026-08-01-landing-below-hero-design.mdfrontend/e2e/landing-graph.spec.tsfrontend/e2e/public-seo.spec.tsfrontend/eslint.config.mjsfrontend/src/app/(public)/page.tsxfrontend/src/app/globals.cssfrontend/src/components/marketing/HowItWorks.tsxfrontend/src/components/marketing/graph/KnowledgeGraphDemo.test.tsxfrontend/src/components/marketing/graph/KnowledgeGraphDemo.tsxfrontend/src/components/marketing/graph/courseGraphs.test.tsfrontend/src/components/marketing/graph/courseGraphs.tsfrontend/src/components/marketing/graph/layout.test.tsfrontend/src/components/marketing/graph/layout.tsfrontend/src/lib/usePrefersReducedMotion.tsfrontend/vitest.setup.ts
💤 Files with no reviewable changes (1)
- frontend/src/components/marketing/HowItWorks.tsx
Comment on lines
+188
to
+194
| <g | ||
| key={n.id} | ||
| data-testid={`landing-graph-node-${n.id}`} | ||
| opacity={h.opacity} | ||
| onMouseEnter={() => onNodeEnter(n.id)} | ||
| onMouseLeave={onNodeLeave} | ||
| > |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The concept blurb is reachable only with a pointer, and assistive tech cannot see the nodes.
Three related points:
- The
<g>elements exposeonMouseEnter/onMouseLeaveonly. Keyboard users and most touch users never see a blurb. - The parent
<svg>setsrole="img"(Line 394). That role makes the whole subtree presentational, so the node groups are removed from the accessibility tree even if they later become focusable. - The blurb
<p>(Lines 415-420) changes content without a live region, so a screen reader announces nothing when the text changes.
The PR defers keyboard node navigation, so a full fix can wait. A small step now is to add aria-live="polite" to the blurb paragraph, and to add onFocus/onBlur alongside the mouse handlers when the nodes become focusable.
♿ Minimal step for the live region
<p
data-testid="landing-graph-blurb"
+ aria-live="polite"
className="landing-graph-blurb font-inter text-[var(--text-dim)] mt-4 min-h-[1.5rem]"
>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/marketing/graph/KnowledgeGraphDemo.tsx` around lines
188 - 194, Add aria-live="polite" to the concept blurb paragraph so screen
readers announce its content changes. Limit this change to the blurb element;
defer keyboard focus handlers and node accessibility updates until the graph
nodes become focusable.
Deleting #features and HowItWorks took the middle of the page's atmosphere with them, and nothing replaced it. The hero carries mesh-blob--1 and --2. The old #features carried --3 and --2, and HowItWorks carried a dark-green scroll tint. The CTA carries --1 and --2 PLUS a top gradient that started at rgba(20,83,45,0.08) on its very first pixel — because it was designed to blend DOWN out of that dark-green tint. So after the deletions the page ran: atmospheric hero -> a completely flat graph section -> a CTA whose green tint faded in from bare paper. Two hard seams, one on each side of the new section. The graph section now carries its own blobs at lower opacity than the hero's, so the graph itself stays the focus, and the CTA's wash starts transparent and peaks below the boundary instead of on it. Found by spinning up the dev server and looking at the whole page, not the section in isolation — the section screenshots I had been judging could not show a seam, because a seam only exists between two things. 94 graph tests pass; tsc clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/marketing/graph/KnowledgeGraphDemo.tsx (1)
432-437: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReserve height for wrapping blurbs on mobile.
The graph is capped to
max-w-[420px], so narrow mobile blurbs can wrap and exceedmin-h-[1.5rem]. Add a width-aware height floor for the widest current blurbs, and include width-regression coverage forlanding-graph-blurb.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/marketing/graph/KnowledgeGraphDemo.tsx` around lines 432 - 437, Update the landing-graph-blurb element in KnowledgeGraphDemo to reserve enough responsive height for the widest blurbs when the graph narrows and text wraps, while preserving the existing styling. Add width-regression coverage targeting landing-graph-blurb that verifies the height floor at mobile and wider viewport widths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/marketing/graph/KnowledgeGraphDemo.tsx`:
- Line 322: Update the lazy-load placeholder sizing in the KnowledgeGraphDemo
section to use the desktop-resolved rendered height rather than the mobile 80vh
fallback. Ensure the placeholder accounts for the larger landing-graph-headline,
section padding, graph, and blurb content when the viewport changes before lazy
loading completes.
---
Outside diff comments:
In `@frontend/src/components/marketing/graph/KnowledgeGraphDemo.tsx`:
- Around line 432-437: Update the landing-graph-blurb element in
KnowledgeGraphDemo to reserve enough responsive height for the widest blurbs
when the graph narrows and text wraps, while preserving the existing styling.
Add width-regression coverage targeting landing-graph-blurb that verifies the
height floor at mobile and wider viewport widths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 51fc286b-4c16-4b17-a670-7c81df8465ce
📒 Files selected for processing (2)
frontend/src/app/(public)/page.tsxfrontend/src/components/marketing/graph/KnowledgeGraphDemo.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/(public)/page.tsx
Uh oh!
There was an error while loading. Please reload this page.
Step 2 of the below-hero redesign. The page now runs hero → graph → Universal Upload → Adaptive Quizzes → bento → Spaced Repetition → CTA. The three bands carry ONE arc — material in → practice → retention — rather than three disconnected pitches, and the closing band (not a grid tile) hands off to the CTA: a grid's last tile is a weak place to ask for a signup. Surfaces alternate sides, derived from position in `featureBands.tsx` rather than written down per band, so inserting a band can't silently put two surfaces in the same gutter. Every tile is a RECREATED PRODUCT SURFACE, never an icon over a heading over a sentence — the brand guide's hard anti-pattern. The seven recreations are faithful to the shipped screens: the upload modal's file rows and status wording, QuizPanel's radiogroup with its `A.` prefix and selected treatment, Study's rating trio, ChatPanel's asymmetric bubbles, the notetaker's linked-concept rail, Social's invite chip and sender names, and the gradebook's letter grade over real assignment rows. Brand constraints that shaped the code rather than just the CSS: - No glassmorphism. `.liquid-glass` is deliberately not reused; surfaces are solid warm paper with hairline borders. - Colour is state. Every mastery mark reads `TIER_COLOR` — the same map the graph section above paints its nodes with — and the only other hues are `--grade-*`. Unit-tested, so a raw hex can't creep back in. - Contrast forced two divergences from the app's own paint: the student bubble takes `--brand-forest` (6.4:1 under white) not `--accent` (4.04:1), and per-row grade letters keep `--text` lettering with the band on the border/dot, since `--grade-b` is 3.39:1 as a glyph. No new dependency, no framer-motion, no JS motion at all: the surfaces are static pictures, so `prefers-reduced-motion` and `IS_TEST_MODE` have no frame to park — there is only the complete one. The entrance is the page's existing `.landing-fade-up` observer, which degrades to "visible" rather than "invisible" when it never fires. All new CSS is inside the `.landing-page` scope and defines no tokens. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rail (#344) The graph section was the least convincing thing on a page whose bands and bento each recreate a whole product screen. It was six flat circles and six words, floating naked on the page background, left-aligned in a 1184px container with the right 40% of the field empty, and its entire hover payoff was one line of body text swapping under the drawing. It is now one surface, full container width, split the way the app's own Tree screen is split. - CHROME. `KNOWLEDGE GRAPH` in the bento's mono micro-label, and the selected course's real numbers on the right — `MA 242 · 19 concepts · 50% mastery`, all read off the fixture. - THE NODES ARE DIALS. A tier-tinted disc at the full node radius, a neutral track ring, the mastery arc swept clockwise from 12 o'clock, and a solid core. Every dimension is measured INWARD from `nodeRadius`, so the drawn footprint is byte-identical to the flat disc it replaces: `fitViewBox`, `labelBaselineY` and the phone's 5.40-unit label clearance are untouched, and a node group's first `<circle>` is still the tier-painted disc the E2E legibility gate measures. - A LEGEND, which is a comprehension fix rather than decoration: four `--state-*` hues were carrying the whole meaning of the picture and nothing on the page said what any of them meant. It carries counts, so it reads as a readout, and it names the tiers in the app's own words. - AN INSPECTOR RAIL replaces the bare hover line: name, tier as a labelled chip, blurb, mastery meter, and the neighbours listed with their own scores, with the hovered node's edges lit in the canvas beside it. Never empty — at rest it shows the course. - AMBIENT DRIFT, ~3 units on a 13–22s per-node cycle with a negative delay so nothing starts in phase. A CSS animation, not a rAF loop: it stays out of the assembly's frame budget (which the suite counts to prove the helix neither fires early nor replays), and rides the same `parked` switch, so reduced-motion visitors and the E2E lane get nodes exactly on their laid-out points. Off below the mobile breakpoint, where the frame pad is 2 units and the worst label clearance 5.40. The fixtures gain a numeric `mastery` per node and a `conceptCount` per course, held to the product's own cutoffs by a port of `backend/config.py::get_mastery_tier`, so a ring can never say 90% while the paint says "struggling" and the chrome's percentage can never contradict the root's own dial. The assembly, its viewport gate, the negative bottom rootMargin and the never-blank derivation are untouched. The phone gate's three bars improve (12.68 / 23.78 / 182.3 against 11 / 20 / 170) because the canvas is full-bleed horizontally — deliberately, and the CSS says why. 576 tests green (+16), tsc clean, eslint clean on src/ and e2e/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
frontend/src/components/marketing/graph/courseGraphs.ts (1)
86-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the unused
childrenfield.
neighbours(Line 135) andlayout.ts::spanningTreeboth state thatchildrenis dead data that disagrees with the edges. The field remains inDemoNodeand in every fixture row. A future contributor can read it as authoritative. Deleting it removes that risk and shrinks the fixtures.Keep it if a deferred click-to-expand task will consume it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/marketing/graph/courseGraphs.ts` around lines 86 - 90, Remove the unused children field from the DemoNode definition and delete it from every fixture row in courseGraphs.ts. Ensure neighbours and layout.ts::spanningTree remain the authoritative sources for graph relationships, unless a deferred click-to-expand feature is already planned to consume children.frontend/src/components/marketing/graph/KnowledgeGraphDemo.test.tsx (1)
913-921: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore
window.matchMediaafter this suite.
installMediaoverwrites the globalwindow.matchMediaand nothing restores it. This suite is currently last in the file, so no later test observes the stub. If a suite is appended after it, that suite inherits the stub and can fail for a non-obvious reason.♻️ Proposed teardown
describe('KnowledgeGraphDemo — ambient drift (`#344` step 3)', () => { + const realMatchMedia = window.matchMedia;+ afterEach(() => {+ window.matchMedia = realMatchMedia;+ });+ function driftingNodes() {Add
afterEachto thevitestimport if it is not already imported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/marketing/graph/KnowledgeGraphDemo.test.tsx` around lines 913 - 921, Restore the original window.matchMedia after each test in the suite using afterEach. Update the Vitest import to include afterEach if needed, capture the pre-test matchMedia implementation, and restore it in teardown after installMedia-based tests so later tests see the original global behavior.frontend/src/app/(public)/page.tsx (1)
591-594: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIndexing
FEATURE_BANDSpositionally couples the page to the array length.
FEATURE_BANDS[2]resolves toundefinedif the content array is trimmed to two entries, and spreadingundefinedintoFeatureBandthrows at render. The ordering intent (bento between band 2 and band 3) is also implicit.Rendering from the array keeps the count authoritative.
♻️ Proposed refactor
- <FeatureBand {...FEATURE_BANDS[0]} />- <FeatureBand {...FEATURE_BANDS[1]} />- <SurfaceBento />- <FeatureBand {...FEATURE_BANDS[2]} />+ {FEATURE_BANDS.map((band, i) => (+ <Fragment key={band.id}>+ {i === FEATURE_BANDS.length - 1 ? <SurfaceBento /> : null}+ <FeatureBand {...band} />+ </Fragment>+ ))}Import
Fragmentfromreact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/app/`(public)/page.tsx around lines 591 - 594, Refactor the page rendering around `FEATURE_BANDS` so it iterates over the array rather than accessing fixed indices, inserting `SurfaceBento` between the second and third band via the iteration position. Import and use React `Fragment` for the grouped band and bento elements, ensuring arrays with fewer than three entries render safely.frontend/src/components/marketing/FeatureBand.tsx (1)
89-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMark decorative product-surface mockups as
aria-hiddenfor screen readers.Both sites render recreated product surfaces (gradebook rows, chat bubbles, quiz UI) that are purely illustrative —
FeatureBand.test.tsx(lines 104-110) confirms these are "pictures, not widgets". Withoutaria-hidden, screen reader users hear the full mockup text as regular content, on top of the headline/body copy that already states the claim. Mark the surface wrapper (orSurfaceFrameitself) asaria-hidden="true"so assistive technology skips the illustrative content.
frontend/src/components/marketing/FeatureBand.tsx#L89: wrap<div className="landing-band-surface">{surface}</div>witharia-hidden="true"(or add it inside once, at theSurfaceFramewrapper).frontend/src/components/marketing/SurfaceBento.tsx#L62-L68: wrap each<div key={t.key} className={...}>{t.node}</div>witharia-hidden="true", or apply the fix once at the sharedSurfaceFramelevel so both call sites inherit it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/marketing/FeatureBand.tsx` at line 89, Mark the decorative product-surface mockups as aria-hidden="true" so screen readers skip their illustrative content. Update the wrapper in frontend/src/components/marketing/FeatureBand.tsx lines 89-89 and each surface wrapper in frontend/src/components/marketing/SurfaceBento.tsx lines 62-68, or apply the attribute once in the shared SurfaceFrame component if both call sites inherit it.frontend/src/components/marketing/FeatureBand.test.tsx (1)
64-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting surface-specific content, not just presence.
This test confirms a
.landing-surfaceelement exists for each band, but does not assert what each surface actually renders.SurfaceBento.test.tsxasserts specific content per tile (tutor bubbles, note link rows, gradebook rows). Apply the same pattern here forUploadSurface,QuizSurface, andReviewSurfaceso a regression that empties one of these surfaces (while.landing-surfacestill renders) is caught.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/marketing/FeatureBand.test.tsx` around lines 64 - 74, Extend the “renders each band” test around FEATURE_BANDS to assert surface-specific content for each rendered surface, not only the .landing-surface container. Follow the existing SurfaceBento.test.tsx pattern and verify distinctive content from UploadSurface, QuizSurface, and ReviewSurface so an empty surface fails the test.frontend/src/components/marketing/surfaces/NotesSurface.tsx (1)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the tier label from
tierinstead of duplicating it inword.
wordrestatestieras a capitalized string by hand for each entry. This is two independent sources of truth for one fact. A future edit totierwithout updatingwordproduces a silently wrong label, sincewordis a freestringwith no type link toMasteryTier.Derive the label from
tierat render time (e.g. a small capitalize helper, or a shared tier-label map ifcourseGraphs.tsalready exports mastery labels).♻️ Proposed refactor
-const LINKED: Array<{ label: string; tier: MasteryTier; word: string }> = [- { label: 'Eigenvalues', tier: 'struggling', word: 'Struggling' },- { label: 'Determinant', tier: 'unexplored', word: 'Unexplored' },- { label: 'Matrices', tier: 'learning', word: 'Learning' },-];+const LINKED: Array<{ label: string; tier: MasteryTier }> = [+ { label: 'Eigenvalues', tier: 'struggling' },+ { label: 'Determinant', tier: 'unexplored' },+ { label: 'Matrices', tier: 'learning' },+];++function tierWord(tier: MasteryTier) {+ return tier.charAt(0).toUpperCase() + tier.slice(1);+}Then render
tierWord(c.tier)instead ofc.word.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/marketing/surfaces/NotesSurface.tsx` around lines 14 - 18, Update the LINKED entries and their rendering so the mastery label is derived from each entry’s tier rather than stored as the independent word field. Remove word from the LINKED type and objects, add or reuse a tier-label helper/map, and render the derived label for each c.tier.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/app/`(public)/page.tsx:
- Around line 18-29: Update the KnowledgeGraphDemo loading placeholder to use a
breakpoint-aware CSS class instead of an inline 990px minHeight. Add the
placeholder class to the loading section, define the measured stacked-layout
height as the default in globals.css, and override it with 990px at min-width
900px so both layouts reserve their resolved heights.
In `@frontend/src/app/globals.css`:
- Line 1565: Remove the quotes around the single-word Spectral font family in
each affected font-family declaration, including the declarations near Spectral
at the visible locations and their duplicates. Preserve quotes around multi-word
families such as Playfair Display, JetBrains Mono, and DM Sans.
In `@frontend/src/components/marketing/surfaces/QuizSurface.tsx`:
- Around line 44-60: Update the static quiz preview in QuizSurface by removing
the radiogroup and radio roles from the options container and each option.
Preserve the existing visual picked state, and expose the selected choice as
static accessible text without presenting the options as interactive controls.
In `@frontend/src/components/marketing/surfaces/UploadSurface.tsx`:
- Around line 43-48: The mastery tier is currently conveyed only by decorative
StateDot colors. In frontend/src/components/marketing/surfaces/UploadSurface.tsx
at lines 43-48 and 70-75, update ConceptChip usage to expose each tier as
adjacent visually-hidden text or an aria-label; in
frontend/src/components/marketing/surfaces/QuizSurface.tsx at lines 32-38, add
the tier word beside the concept label, following NotesSurface’s existing
pattern.
---
Nitpick comments:
In `@frontend/src/app/`(public)/page.tsx:
- Around line 591-594: Refactor the page rendering around `FEATURE_BANDS` so it
iterates over the array rather than accessing fixed indices, inserting
`SurfaceBento` between the second and third band via the iteration position.
Import and use React `Fragment` for the grouped band and bento elements,
ensuring arrays with fewer than three entries render safely.
In `@frontend/src/components/marketing/FeatureBand.test.tsx`:
- Around line 64-74: Extend the “renders each band” test around FEATURE_BANDS to
assert surface-specific content for each rendered surface, not only the
.landing-surface container. Follow the existing SurfaceBento.test.tsx pattern
and verify distinctive content from UploadSurface, QuizSurface, and
ReviewSurface so an empty surface fails the test.
In `@frontend/src/components/marketing/FeatureBand.tsx`:
- Line 89: Mark the decorative product-surface mockups as aria-hidden="true" so
screen readers skip their illustrative content. Update the wrapper in
frontend/src/components/marketing/FeatureBand.tsx lines 89-89 and each surface
wrapper in frontend/src/components/marketing/SurfaceBento.tsx lines 62-68, or
apply the attribute once in the shared SurfaceFrame component if both call sites
inherit it.
In `@frontend/src/components/marketing/graph/courseGraphs.ts`:
- Around line 86-90: Remove the unused children field from the DemoNode
definition and delete it from every fixture row in courseGraphs.ts. Ensure
neighbours and layout.ts::spanningTree remain the authoritative sources for
graph relationships, unless a deferred click-to-expand feature is already
planned to consume children.
In `@frontend/src/components/marketing/graph/KnowledgeGraphDemo.test.tsx`:
- Around line 913-921: Restore the original window.matchMedia after each test in
the suite using afterEach. Update the Vitest import to include afterEach if
needed, capture the pre-test matchMedia implementation, and restore it in
teardown after installMedia-based tests so later tests see the original global
behavior.
In `@frontend/src/components/marketing/surfaces/NotesSurface.tsx`:
- Around line 14-18: Update the LINKED entries and their rendering so the
mastery label is derived from each entry’s tier rather than stored as the
independent word field. Remove word from the LINKED type and objects, add or
reuse a tier-label helper/map, and render the derived label for each c.tier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a396af98-9c3e-433f-bad6-7809a47b158c
📒 Files selected for processing (22)
docs/frontend-testids.mdfrontend/eslint.config.mjsfrontend/src/app/(public)/page.tsxfrontend/src/app/globals.cssfrontend/src/components/marketing/FeatureBand.test.tsxfrontend/src/components/marketing/FeatureBand.tsxfrontend/src/components/marketing/SurfaceBento.test.tsxfrontend/src/components/marketing/SurfaceBento.tsxfrontend/src/components/marketing/featureBands.tsxfrontend/src/components/marketing/graph/KnowledgeGraphDemo.test.tsxfrontend/src/components/marketing/graph/KnowledgeGraphDemo.tsxfrontend/src/components/marketing/graph/courseGraphs.test.tsfrontend/src/components/marketing/graph/courseGraphs.tsfrontend/src/components/marketing/graph/layout.test.tsfrontend/src/components/marketing/surfaces/GradebookSurface.tsxfrontend/src/components/marketing/surfaces/NotesSurface.tsxfrontend/src/components/marketing/surfaces/QuizSurface.tsxfrontend/src/components/marketing/surfaces/ReviewSurface.tsxfrontend/src/components/marketing/surfaces/RoomsSurface.tsxfrontend/src/components/marketing/surfaces/Surface.tsxfrontend/src/components/marketing/surfaces/TutorSurface.tsxfrontend/src/components/marketing/surfaces/UploadSurface.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/marketing/graph/layout.test.ts
Comment on lines
+18
to
+29
| const KnowledgeGraphDemo = dynamic( | ||
| () => import('@/components/marketing/graph/KnowledgeGraphDemo'), | ||
| { | ||
| // Placeholder height approximates the section's resolved height so nothing | ||
| // below shifts while the chunk loads. MEASURED, not guessed: the section | ||
| // resolves to 990px at every desktop width once it wears its product | ||
| // chrome (#344 step 3) — the `80vh` this carried was 27–40% short of that | ||
| // even before, and a viewport-relative value cannot track a section whose | ||
| // height is set by a fixed-width inspector rail. | ||
| loading: () => <section id="knowledge-graph" className="landing-section relative" style={{ minHeight: '990px' }} />, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The 990px placeholder is a desktop measurement applied at every width.
The comment states the section resolves to 990px at every desktop width. Below the 900px breakpoint the layout stacks: .landing-graph-split becomes one column and the rail moves under the canvas (globals.css Lines 1030-1038). The stacked height is not the desktop height. A fixed 990px placeholder therefore mis-reserves on phones, and the page shifts when the chunk resolves.
Consider a breakpoint-aware minimum height so the reserved box tracks the resolved layout at both widths.
🐛 Proposed fix using a CSS-driven placeholder
- loading: () => <section id="knowledge-graph" className="landing-section relative" style={{ minHeight: '990px' }} />,+ loading: () => (+ <section id="knowledge-graph" className="landing-section landing-graph-placeholder relative" />+ ),Then set the two measured heights in globals.css:
.landing-page .landing-graph-placeholder { min-height:1180px; }
`@media` (min-width:900px) {
.landing-page .landing-graph-placeholder { min-height:990px; }
}Replace 1180px with the measured stacked height.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| constKnowledgeGraphDemo=dynamic( | |
| ()=>import('@/components/marketing/graph/KnowledgeGraphDemo'), | |
| { | |
| // Placeholder height approximates the section's resolved height so nothing | |
| // below shifts while the chunk loads. MEASURED, not guessed: the section | |
| // resolves to 990px at every desktop width once it wears its product | |
| // chrome (#344 step 3) — the `80vh` this carried was 27–40% short of that | |
| // even before, and a viewport-relative value cannot track a section whose | |
| // height is set by a fixed-width inspector rail. | |
| loading: ()=><sectionid="knowledge-graph"className="landing-section relative"style={{minHeight: '990px'}}/>, | |
| }, | |
| ); | |
| constKnowledgeGraphDemo=dynamic( | |
| ()=>import('`@/components/marketing/graph/KnowledgeGraphDemo`'), | |
| { | |
| // Placeholder height approximates the section's resolved height so nothing | |
| // below shifts while the chunk loads. MEASURED, not guessed: the section | |
| // resolves to 990px at every desktop width once it wears its product | |
| // chrome (`#344` step 3) — the `80vh` this carried was 27–40% short of that | |
| // even before, and a viewport-relative value cannot track a section whose | |
| // height is set by a fixed-width inspector rail. | |
| loading: ()=>( | |
| <sectionid="knowledge-graph"className="landing-section landing-graph-placeholder relative"/> | |
| ), | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/app/`(public)/page.tsx around lines 18 - 29, Update the
KnowledgeGraphDemo loading placeholder to use a breakpoint-aware CSS class
instead of an inline 990px minHeight. Add the placeholder class to the loading
section, define the measured stacked-layout height as the default in
globals.css, and override it with 990px at min-width 900px so both layouts
reserve their resolved heights.
| .landing-page .landing-surface-question { | ||
| margin: 2px 0 4px; | ||
| font-family: var(--font-spectral), 'Spectral', Georgia, serif; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the quotes around Spectral to clear the Stylelint errors.
Stylelint reports font-family-name-quotes at Lines 1565, 1713, 1784 and 1836. Spectral is a single-word family name and needs no quotes. The multi-word names in the same declarations ('Playfair Display', 'JetBrains Mono', 'DM Sans') do need them and are not flagged.
🐛 Proposed fix
- font-family: var(--font-spectral), 'Spectral', Georgia, serif;+ font-family: var(--font-spectral), Spectral, Georgia, serif;Apply the same change at Lines 1565, 1713, 1784 and 1836.
Also applies to: 1713-1713, 1784-1784, 1836-1836
🧰 Tools
🪛 Stylelint (17.14.1)
[error] 1565-1565: Expected no quotes around "Spectral" (font-family-name-quotes)
(font-family-name-quotes)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/app/globals.css` at line 1565, Remove the quotes around the
single-word Spectral font family in each affected font-family declaration,
including the declarations near Spectral at the visible locations and their
duplicates. Preserve quotes around multi-word families such as Playfair Display,
JetBrains Mono, and DM Sans.
Source: Linters/SAST tools
Comment on lines
+44
to
+60
| <span className="landing-surface-options" role="radiogroup" aria-label="Answer options"> | ||
| {OPTIONS.map((o) => { | ||
| const picked = o.label === PICKED; | ||
| return ( | ||
| <span | ||
| key={o.label} | ||
| role="radio" | ||
| aria-checked={picked} | ||
| aria-label={`${o.label}. ${o.text}`} | ||
| className={`landing-surface-option${picked ? ' is-picked' : ''}`} | ||
| > | ||
| <span className="landing-surface-optionkey">{o.label}.</span> | ||
| {o.text} | ||
| </span> | ||
| ); | ||
| })} | ||
| </span> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"Locate QuizSurface.tsx:"
fd -a 'QuizSurface\.tsx$'.||trueechoecho"Git status/stat:"
git diff --stat ||true
file="$(fd 'QuizSurface\.tsx$'.| head -n 1 || true)"if [ -n"$file" ];thenechoecho"File: $file"
wc -l "$file"echoecho"Relevant lines:"
sed -n '1,120p'"$file"| nl -ba
fiechoecho"Search for similar quiz option patterns nearby:"
rg -n 'QuizSurface|landing-surface-options|role="radiogroup"|role="radio"|aria-checked|there is nothing to click|static' --glob '*.tsx' --glob '*.ts'.Repository: SaplingLearn/Sapling
Length of output: 468
🏁 Script executed:
#!/bin/bashset -euo pipefail
file="./frontend/src/components/marketing/surfaces/QuizSurface.tsx"echo"Relevant QuizSurface contents:"
line_count=0
while IFS= read -r r;do
line_count=$((line_count +1))printf'%5d\t%s\n'"$line_count""$r"done<"$file"echoecho"Accessibility role/pattern usages in repository:"
rg -n 'QuizSurface|landing-surface-options|role="radiogroup"|role="radio"|aria-checked|there is nothing to click|static|radiogroup' --glob '*.tsx' --glob '*.ts'.||trueRepository: SaplingLearn/Sapling
Length of output: 8525
🌐 Web query:
WAI-ARIA 1.2 radio widget focusable keyboard Space key role=radio
💡 Result:
In WAI-ARIA implementations, a radio widget (role=radio) must be focusable and keyboard accessible [1]. When a radio button receives focus—whether via Tab or arrow key navigation—the Space key is the standard, expected keyboard interaction to check the radio button if it is not already checked [2][1]. Key aspects of the WAI-ARIA radio widget keyboard behavior include: Focusability: If the role=radio is applied to an element that is not natively focusable (such as a div or span), you must use the tabindex attribute to make it focusable [1]. In patterns using a roving tabindex, only one radio button in the group is included in the page's tab sequence (tabindex=0), while the others are removed from the tab sequence (tabindex=-1) [3][4]. Alternatively, when using aria-activedescendant, the container element (radiogroup) is focusable, and it manages the focus state for its children [5][6][4]. Space Key Interaction: The Space key checks the focused radio button if it is not already checked [2][1]. In standard radio group implementations (not nested in a toolbar), selecting a new radio button automatically unchecks the previously checked one [2][1]. Toolbar Exception: When a radio group is nested within a toolbar, the behavior of the Space key may differ. In this specific context, the Space key is often used to change the selection (if the focused button is not already checked), but arrow key navigation is specifically designed to move focus without changing the checked state of the radio buttons, allowing users to navigate through the toolbar without inadvertently modifying the radio group's selection [2]. These keyboard conventions are established by the W3C Web Accessibility Initiative (WAI) Authoring Practices Guide (APG) to ensure consistent and predictable behavior for users of assistive technologies [2][7].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/radio_role
- 2: https://www.w3.org/WAI/ARIA/apg/patterns/radio/
- 3: https://www.w3.org/TR/2021/NOTE-wai-aria-practices-1.2-20211129/examples/radio/radio.html
- 4: https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/
- 5: https://www.w3.org/WAI/ARIA/apg/patterns/radio/examples/radio-activedescendant/
- 6: https://www.w3.org/TR/2021/NOTE-wai-aria-practices-1.2-20211129/examples/radio/radio-activedescendant.html
- 7: https://www.w3.org/TR/wai-aria-1.2/
Remove the interactive radio roles from the static quiz preview.
QuizSurface is documented as a static product picture with no elements to click, but role="radiogroup" and role="radio" expose these options as keyboard-focusable controls that should respond to Tab/Space. This gives assistive technology users a false interactive affordance. Keep the same visual state but remove the roles and mark the selected choice as static accessible text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/marketing/surfaces/QuizSurface.tsx` around lines 44 -
60, Update the static quiz preview in QuizSurface by removing the radiogroup and
radio roles from the options container and each option. Preserve the existing
visual picked state, and expose the selected choice as static accessible text
without presenting the options as interactive controls.
Comment on lines
+43
to
+48
| const CONCEPTS: Array<{ label: string; tier: MasteryTier }> = [ | ||
| { label: 'Vector Spaces', tier: 'mastered' }, | ||
| { label: 'Matrices', tier: 'learning' }, | ||
| { label: 'Eigenvalues', tier: 'struggling' }, | ||
| { label: 'Determinant', tier: 'unexplored' }, | ||
| ]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mastery tier is conveyed only through StateDot color at two sites; add a text alternative. Both sites pair a concept label with an aria-hiddenStateDot and never spell out the tier word nearby, breaking the contract documented in Surface.tsx's own StateDot comment ("the tier is always spelled out in adjacent text or is decorative repetition of it"). Screen-reader users get nothing for the tier, since the dot is hidden from the accessibility tree; colorblind sighted users cannot reliably distinguish tiers either, since color is the only channel.
frontend/src/components/marketing/surfaces/UploadSurface.tsx#L43-L75: add a visually-hidden tier label toConceptChip(or anaria-labelon the chip) so each concept's tier text exists in the accessible tree.frontend/src/components/marketing/surfaces/QuizSurface.tsx#L32-L38: add the tier word (e.g. "Struggling") next to "Eigenvalues" in the headrow, matching the pattern already used correctly inNotesSurface.tsx.
📍 Affects 2 files
frontend/src/components/marketing/surfaces/UploadSurface.tsx#L43-L48(this comment)frontend/src/components/marketing/surfaces/UploadSurface.tsx#L70-L75frontend/src/components/marketing/surfaces/QuizSurface.tsx#L32-L38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/marketing/surfaces/UploadSurface.tsx` around lines 43
- 48, The mastery tier is currently conveyed only by decorative StateDot colors.
In frontend/src/components/marketing/surfaces/UploadSurface.tsx at lines 43-48
and 70-75, update ConceptChip usage to expose each tier as adjacent
visually-hidden text or an aria-label; in
frontend/src/components/marketing/surfaces/QuizSurface.tsx at lines 32-38, add
the tier word beside the concept label, following NotesSurface’s existing
pattern.
AndresL230
commented
Aug 2, 2026
CollaboratorAuthor
Code reviewFound 1 issue:
Verified by mutation, not inspection: replacing The source fix is intact and correct — this is the guard around it that has eroded, so a future regression would land unnoticed. Sapling/frontend/src/components/marketing/graph/KnowledgeGraphDemo.tsx Lines 251 to 253 in 7e4ea24 Fix direction: arm a fake Below the reporting bar but worth folding in while the files are open: 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 2, 2026
This was referenced Aug 5, 2026
This was referenced Aug 23, 2026
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 freeto 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.
Phase 2 of #344. Replaces the 340vh scroll-jacked
HowItWorkswith an interactive knowledge-graph demo, and deletes the six-row feature catalog.Spec:
docs/superpowers/specs/2026-08-01-landing-below-hero-design.mdPlan:
docs/superpowers/plans/2026-08-01-landing-graph-step1.mdThis is step 1 of 2. The spec's bands + bento (sections 2–5) are deliberately not here — see "Deferred" below. At the end of this PR the page is coherent: hero → graph → CTA → footer.
What it does
Three course chips. Picking one assembles that course's concept graph along a helical entry path, from the course node at the base upward into a canopy. Hovering a node reveals its concept blurb and recedes the instructional copy. Static fixtures — no backend, no network, no LLM.
What it deletes
HowItWorks.tsx(677 lines of framer-motion), the Seed→Sprout→Tree SVGs, theAppWindowmock, the left-side step indicator, the drag/scroll hint copy, the per-step preview buttons, the six-row hairline catalog and its "— end of catalog" rule, and the.landing-feature-*CSS they orphaned. Every one of those is named in #344.The part worth reviewing carefully
Every automated gate was green while the section was visibly broken. 41 Playwright, 517 unit tests, oracles clean — none of it could see that the section was 80% empty, or that the mastery colours didn't match the product. It took seven rounds of screenshotting the built page:
#EF4444/#D97706shouting off warm paper--state-*tokensBrand conformance
globals.css:80-89defines the canonical knowledge-status palette, and its comment says it exists as the "Single source for the 3 previously-inlined copies in Dashboard / Tree / notetaker". This branch had added a fourth inlined copy with four different hexes — so the landing page was advertising different mastery colours than the app. Now fixed:#1B6C42var(--state-mastery)#D97706var(--state-progress)#EF4444var(--state-struggle)#9CA3AFvar(--state-neutral)The hero's legend card (
page.tsx:503-506) carried the same wrong literals and is converged too — colours only, no layout/copy/structure change. Fixing one without the other would leave the legend and the graph disagreeing one viewport apart.Correctness work that isn't visible
public-seo.spec.tsclaimed to guard this but didn't (it only asserts Metadata-API output, which is emitted either way). Added a raw-HTML assertion that actually fails if someone setsssr: false.window.matchMediain the render body produced a real mismatch for reduced-motion visitors. Replaced withusePrefersReducedMotion(useSyncExternalStore+ fixedgetServerSnapshot), mirroring the existinguseIsMobileprecedent — whose own doc comment warns about this exact bug class. Server snapshot defaults totrueso the population the OS setting protects never sees a wrong first frame.progressis 1 whenever the loop isn't armed, so no committed frame renders the graph at opacity 0.IntersectionObserver-gated. TherootMarginmust be negative-bottom: the section's top sits at exactly100vhafter themin-h-screenhero, so any positive margin arms it at scroll 0. Pinned by a sign test.Verification
tsc --noEmitclean;eslintclean.Deferred, deliberately
getScreenCTM()decision that deserves its own review gate. Recorded indocs/frontend-testids.md.childrenfixture field,.landing-graphclassname with no rule) are listed in the branch's execution ledger.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests