feat(graph): replace 2D SVG knowledge graph with 3D WebGL (react-force-graph-3d)#92
Merged
Merged
Conversation
…e-graph-3d) Drop-in replacement for the 645-line custom SVG/d3-force component. Same module path, same export name, same prop interface — so the three callers (Tree.tsx, Learn.tsx, Dashboard.tsx) keep working without a single line of change. Backed by react-force-graph-3d (Three.js + WebGL). The library handles physics + rendering; this component is the data adapter and the styling callbacks (per-node colour shading, highlight, click). Implementation notes: - SSR: react-force-graph-3d touches `document` + `window` at module load. We `dynamic`-import it with `ssr: false` so Next.js doesn't try to render it on the server. Layout shift is avoided by sizing the wrapper div explicitly. - Bundle: the three.js + 3d-force-graph code is dynamically split, so it only ships to users who actually load /tree, /learn, or /dashboard. The rest of the app's First Load is unchanged. - Coordinate-mutation safety: react-force-graph-3d sets x/y/z/vx/vy/vz on every node in place. We shallow-clone the caller's nodes/edges so we don't mutate the upstream data, and strip the lib-mutated coordinate fields before handing a node back via onNodeClick. - Color helpers (`hashId`, `hexToHsl`, `shadeFor`) are copied verbatim from the old SVG version so per-course shading stays visually consistent across the rest of the app. - Node size scales with mastery_score (4..10) so the eye lands on what the student has built up. Edge width scales with `strength` so prerequisite chains read visually. Out of scope (kept as prop placeholders, ignored by the 3D backend): - Variants (orb / constellation / organism). Only Learn.tsx still passes one and it's rendering fine without it; the visual variant-switching can land in a follow-up if it matters again. - Comparison rings (partner mastery overlay). Not used by any current call site (defined in props but no caller passes it). - Mobile UX. Per the user's direction, desktop is the priority; mobile gets the 3D too and may feel slightly worse, but the feature still functions. Can degrade to a simpler representation in a follow-up if mobile use proves clunky. Pre-existing postcss/next vulnerability in npm audit is unchanged by this PR — not introduced here. 29 frontend tests pass. Type-check clean. Next.js production build succeeds for all 17 routes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
frontend | 1cef519 | Commit Preview URL Branch Preview URL |
May 05 2026, 09:37 PM |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughReplaces the D3/SVG KnowledgeGraph with a client-only 3D renderer using react-force-graph-3d/three, adds related dependencies and Next.js transpile config, updates the component for 3D data/callbacks and accessibility, and adds a comprehensive test suite for the new 3D adapter. Changes3D Visualization Upgrade
Sequence DiagramsequenceDiagram
participant User
participant KG as KnowledgeGraph Component
participant FG as ForceGraph3D Library
participant App as App Callback (onNodeClick)
User->>KG: click node control (or UI button)
KG->>FG: pass graphData, nodeColor, nodeVal, nodeLabel, onNodeClick
FG->>KG: emit nodeClick with library-node (with injected fields)
KG->>KG: resolve canonical GraphNode by id (strip lib fields)
KG->>App: call onNodeClick(canonical GraphNode)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/components/KnowledgeGraph.tsx (1)
135-135: 💤 Low valueOptional: avoid mixing default and named export.
Adding
export default KnowledgeGraphat line 233 alongside the namedexport function KnowledgeGraphat line 135 invites inconsistent import styles across the codebase (and trips up some tree-shaking heuristics). All current callers (Tree.tsx,Learn.tsx,Dashboard.tsx) use the named import, so the default export is unused.♻️ Drop the default export
-export default KnowledgeGraph;Also applies to: 233-233
🤖 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/KnowledgeGraph.tsx` at line 135, The file currently defines a named export "export function KnowledgeGraph" and also adds a default export at the bottom; remove the default export to avoid mixing named and default exports and to keep imports consistent with current callers (Tree.tsx, Learn.tsx, Dashboard.tsx). Locate the default export statement (e.g., "export default KnowledgeGraph") and delete it so the component is exported only as the named export "KnowledgeGraph".
🤖 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/KnowledgeGraph.tsx`:
- Around line 199-230: The 3D canvas in KnowledgeGraph (ForceGraph3D) is
inaccessible and ignores reduced-motion; update the KnowledgeGraph component to
1) add a brief annotated wrapper comment describing the AT limitation, 2) render
a DOM-based, keyboard/screen-reader reachable fallback list (e.g.,
visually-hidden or visible list) that maps graphData.nodes to focusable
items/buttons using each node's id/name and calls the existing
handleNodeClick(node) when activated so keyboard/AT users can trigger the same
behavior, and 3) honor prefers-reduced-motion by detecting
window.matchMedia('(prefers-reduced-motion: reduce)') and set cooldownTicks to 0
(or a much lower value) when reduced-motion is requested; reference
ForceGraph3D, graphData, handleNodeClick, and cooldownTicks when making these
changes and reuse any existing .sr-only / visually-hidden utility or add a small
inline style fallback for screen-reader-only content.
- Around line 186-197: The handler handleNodeClick currently strips only x/y/z
and still forwards library-injected fields (vx/vy/vz, fx/fy/fz, __threeObj);
instead, when onNodeClick is present resolve the original GraphNode from the
component's nodes prop by matching the node id (use the FG3DNode's id to find
the corresponding original GraphNode in nodes) and pass that original object to
onNodeClick so callers receive the clean, whitelisted GraphNode shape (ensure
you handle a missing id or not-found case by not calling onNodeClick).
---
Nitpick comments:
In `@frontend/src/components/KnowledgeGraph.tsx`:
- Line 135: The file currently defines a named export "export function
KnowledgeGraph" and also adds a default export at the bottom; remove the default
export to avoid mixing named and default exports and to keep imports consistent
with current callers (Tree.tsx, Learn.tsx, Dashboard.tsx). Locate the default
export statement (e.g., "export default KnowledgeGraph") and delete it so the
component is exported only as the named export "KnowledgeGraph".
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 153dfe5c-79b9-4e43-a780-219aaf3ca45b
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
frontend/package.jsonfrontend/src/components/KnowledgeGraph.tsx
…ld (PR #92) Workers Builds CI was failing on `feat/knowledge-graph-3d` while local `next build` and `opennextjs-cloudflare build` both passed. `react-force-graph-3d` and its transitive deps (`3d-force-graph`, `three-render-objects`, `d3-force-3d`, `react-kapsule`, `three`) are ESM-only and touch `window` at module load. We render the component via `next/dynamic({ ssr: false })`, but the OpenNext server bundle still needs Next to transpile these packages — otherwise the Worker tries to evaluate raw ESM with browser globals on cold start and crashes. Adding them to `transpilePackages` lets Next/OpenNext fully bundle and CommonJS-wrap them for the Worker runtime.
Pin the four bits of real adapter logic in the new component:
- graphData memo shallow-clones nodes/edges so the lib's mutation
of x/y/z/vx/vy/vz doesn't poison the caller's array.
- onNodeClick strips x/y/z from the lib-mutated node before
handing a clean GraphNode shape back to the caller.
- nodeColor returns "#ffffff" for the highlighted id, otherwise
an hsl()-format shade from shadeFor().
- nodeVal scales 4..10 with mastery_score (0 -> 4, 1 -> 10).
Plus an empty-data render-without-crash baseline.
Mocks react-force-graph-3d to capture the props passed to the lib so
tests can drive nodeColor/nodeVal/onNodeClick directly without
mounting a real WebGL canvas in jsdom. next/dynamic is mocked to
eager-resolve the loader so the mocked module is what renders.
Frontend suite: 34 passed (29 prior + 5 new). No production code
touched.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
react-force-graph-3d (and its three.js transitive deps) need the newer Node-API surface; v1 nodejs_compat falls short on Cloudflare Workers Builds even though local opennextjs-cloudflare succeeds. Also pin engines.node >=20 so CF picks a Node version matching the Workers runtime baseline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two intertwined fixes; landing together because they share the
KnowledgeGraph touch-points and the CF build investigation.
1. Revert wrangler.toml's compatibility_flags back to "nodejs_compat".
The prior change to nodejs_compat_v2 made the Cloudflare Workers
build fail in 0s — config-rejection, not a bundling failure. Per
Cloudflare's flag system, nodejs_compat_v2 is implicit when
compatibility_date >= 2024-09-23 (we're on 2025-06-01), so
declaring it explicitly is invalid. Back to the explicit v1 flag.
2. Address CodeRabbit's 2 actionable comments + 1 nitpick on the
3D KnowledgeGraph:
- Whitelist node-click via id-lookup. Old: blacklist x/y/z.
New: nodes.find(n => n.id === raw.id). Library-injected fields
(vx/vy/vz, fx/fy/fz, __threeObj, ...) no longer leak to callers.
Drops the click if the node disappeared between sim run and
click rather than synthesise a partial node.
- sr-only fallback list. WebGL canvas is opaque to AT; we now
render an aria-labeled <ul> of focusable buttons (one per node)
in a visually-hidden style so keyboard + screen-reader users
can reach the same activation behavior `onNodeClick` provides
to mouse users. Repo grep confirmed no existing sr-only
utility, so used inline-style SR_ONLY constant.
- prefers-reduced-motion. matchMedia hooked via useEffect with
SSR guard + Safari <14 addListener fallback. Cooldown drops to
0 ticks (no animation) when the user's OS setting requests it;
stays at the 120-tick default otherwise.
- Dropped the redundant default export. All callers (Tree.tsx,
Learn.tsx, Dashboard.tsx) use the named import; the default
was unused and invited inconsistent imports + tree-shaking
friction.
Tests:
- Updated test_onNodeClick_strips_coordinate_fields to additionally
inject vx/vy/vz/fx/fy/fz/__threeObj on the lib-mutated node and
assert the caller receives the original whitelisted shape.
- Added test_renders_sr_only_node_list (AT fallback present + clicks
call onNodeClick).
- Added test_reduced_motion_sets_cooldown_to_zero (matchMedia mock
for the reduced-motion query).
Frontend suite: 36 passed (was 34 + 2 net new). Type-check clean.
CF build is still failing as of this commit — separate fix for the
underlying bundle-vs-runtime incompatibility coming next.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Documents the failed `serverExternalPackages` attempt directly in
next.config.ts so a future maintainer doesn't repeat it.
Why it failed: Next.js's RSC bundler still resolves the module graph
at static-analysis time even when the runtime import is gated by
`dynamic({ssr:false})`. With these packages flagged as
serverExternalPackages, that resolution tried to evaluate the
ESM-only `react-force-graph-3d` (which crashes with `window is not
defined`) and broke the local Next.js build.
The transpilePackages config alone keeps local builds green. Whatever
is failing in Cloudflare Workers Builds happens INSIDE the CF
pipeline (logs only available on the CF dashboard, not via
GitHub's check-runs API — `output.text: null`, `annotations: 0`).
Surface the dashboard logs to make the next attempt grounded
instead of speculative.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/package.json`:
- Around line 5-7: Update the Node engine constraint in package.json so it
matches Next.js 16 requirements: change the "engines" -> "node" value from
">=20.0.0" to ">=20.9.0" to prevent installing unsupported Node 20.x versions;
locate the "engines" block in package.json and update the "node" string
accordingly.
In `@frontend/src/components/KnowledgeGraph.tsx`:
- Around line 271-275: The list currently renders a focusable <button> for every
node inside the KnowledgeGraph component even when onNodeClick is undefined,
producing dead keyboard controls; update the nodes.map rendering logic in
KnowledgeGraph so it only renders an interactive <button> when onNodeClick is
provided (calling onNodeClick(n) on click) and otherwise renders a
non-interactive element (e.g., a <span> or <div> with appropriate text content
and no tabIndex) so keyboard/AT users don’t encounter no-op controls.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d24bdd26-f259-47a8-8d34-3552759d20f8
📒 Files selected for processing (4)
frontend/next.config.tsfrontend/package.jsonfrontend/src/components/KnowledgeGraph.test.tsxfrontend/src/components/KnowledgeGraph.tsx
Cloudflare Workers Builds runs `npm clean-install` with npm 10.9.2. That hit EUSAGE on every build of PR #92: npm error Missing: @emnapi/runtime@1.10.0 from lock file npm error Missing: @emnapi/core@1.10.0 from lock file npm error Missing: esbuild@0.28.0 from lock file Cause: when react-force-graph-3d + three were installed locally, the generating npm version produced a lockfile that omits a few transitive deps that npm 10.9.2's strict `npm ci` requires. Same class of issue PR #67 hit during the docs-readme refresh. Fix: regenerated package-lock.json with `npx -p npm@10.9.2 npm install` so the lockfile matches what Cloudflare's runner expects. Then verified `npm ci` succeeds against the new lockfile (1061 packages, no errors). Local pipeline still clean against the new lockfile: - tsc --noEmit -> clean - vitest -> 36 passed - next build -> all 17 routes succeed - opennextjs-cloudflare build -> Worker saved The build-runtime config (transpilePackages, wrangler nodejs_compat, no engines.npm pin) is otherwise unchanged. The CF failure was purely lockfile-skew between npm versions, not a bundling or runtime issue. Future installs by anyone with npm >=11 should still work because the lockfile is npm-version-tolerant — only `npm ci` strict mode demanded the missing transitives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two real findings from CodeRabbit's re-review (range 4c18808..3ed8fcc): 1. engines.node was too lax. Bumped from ">=20.0.0" to ">=20.9.0" to match Next.js 16's actual minimum — Next 16's SSR/streaming changes depend on a Node 20.9 fix, so >=20.0.0 admitted versions that install fine but break at runtime. 2. sr-only buttons were dead controls when onNodeClick is undefined. The accessibility fallback in commit 3ed8fcc rendered a focusable <button> for every node unconditionally; for callers that don't pass an onNodeClick (none today, but a future caller could), AT users would Tab through buttons that do nothing — strictly worse UX than not having the controls at all. Now: render <button> only when onNodeClick is provided. Otherwise the <li> contains plain text, so AT still hears the node names in the list but no dead activation affordance. Test added — test_renders_sr_only_list_as_static_text_when_no_onNodeClick: asserts zero <button> elements + two <li>s with text content when the component renders without an onNodeClick handler. 37 tests pass (was 36 + 1 new). Type-check clean. The CF Workers Build went green on commit ae41a9f after the package-lock.json regeneration; this commit doesn't touch the build-system surface so it should land green too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Highlight color was pure white, which disappeared against the cream light theme (--ink-0: #faf8f3). Switched to the brand --accent (#8a9a5b) — a green that resolves visibly on both light cream and dark near-black backgrounds. Link color was generic gray; replaced with --ink-400 with alpha (rgba(138, 131, 114, 0.45)) so the warm-toned palette stays consistent across the site. Test updated to assert the new highlight value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address PR review feedback: 1. Drop unused props from public type. The 3D backend doesn't yet port `variant`, `comparison`, `comparisonColor`, `comparisonLabel`, `pauseWhenOffscreen`, so they're removed rather than silently ignored. Callers that pass them now get a TypeScript error. Updated the one site (`Learn.tsx` was passing `variant="organism"`). 2. Lazy useState initializer for reduced-motion. Previous `useState(false)` + effect-set caused a one-frame flash of physics for users who prefer reduced motion. Reading matchMedia in the initializer means the first paint is correct. 3. O(1) id lookup in handleNodeClick via memoized Map<id, GraphNode> instead of `nodes.find` per click. Cheap insurance for graphs with hundreds of nodes. 4. Pin npm to >=10.9.0 <11 in engines + add .npmrc with engine-strict=true. CF Workers Builds runs npm 10.9.2 in strict `npm ci`; npm 11+ rewrites the lockfile to v3 which fails on CF. This makes local installs fail fast instead of silently producing a broken lockfile commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The lazy useState initializer reads matchMedia synchronously, which
means SSR returns false and the client may compute true. That's safe
today because the value only flows into <ForceGraph3D>'s cooldownTicks
prop, and ForceGraph3D is dynamic({ ssr: false, loading: () => null })
— so it never reaches the SSR DOM and React has nothing to mismatch.
Document the constraint inline so a future contributor wiring
reducedMotion into the sr-only list or outer <div> doesn't ship a
hydration warning by accident.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous comment slightly oversold the mechanism. engine-strict=true gates `npm install` (the write path that regenerates the lockfile), not `npm ci` (which ignores engine-strict in npm >=7). The protection is "stop the bad commit at its source," not "make CF's npm ci pickier." Same setting, accurate description. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Defense-in-depth on top of engines.npm + engine-strict. CF Workers Builds runs npm ci with npm 10.9.2; lockfiles regenerated by npm 11+ pass local install but fail CF's npm ci with cryptic "Missing X from lock file" errors (see PR #92 for the prior incident). scripts/check-lockfile-version.mjs reads package-lock.json's lockfileVersion and exits non-zero if it's not 3, with a clear fix message pointing at the npm 10.9.x install. Wired up as `npm run check:lockfile`. Contributors can run it manually before pushing; future work can hook it into a pre-commit or a frontend CI workflow once one exists. Verified both branches: - lockfileVersion=3 → exits 0 ("✓") - lockfileVersion=4 → exits 1 with the diagnostic + fix message Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `<11` upper bound on engines.npm will block local installs once CF Workers Builds moves past npm 10.x. Document the unblock recipe inline (relax engines.npm + bump EXPECTED_VERSION in scripts/check-lockfile-version.mjs) so the next contributor doesn't have to spelunk for the rationale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 free
to 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.
TL;DR
Replace the existing 645-line SVG/d3-force
KnowledgeGraphcomponent with a 3D WebGL version powered byreact-force-graph-3d. Drop-in: same module path, same export name, same prop interface — the three callers (Tree.tsx,Learn.tsx,Dashboard.tsx) keep working with zero changes.What you'll see
Visit
/tree,/learn, or/dashboardand the graph is now in 3D space:onNodeClickcallback fires (selection panel opens, etc.) — same behavior as before.mastery_score(4..10 range) — the eye lands on what the student has built up.strengthso prerequisite chains read visually.Implementation notes
react-force-graph-3dtouchesdocument/windowat module load, so it'sdynamic-imported with{ ssr: false }. Next.js doesn't try to render it server-side./tree,/learn,/dashboard). The rest of the app's First Load JS is unaffected.react-force-graph-3dsetsx/y/z/vx/vy/vzon every node in place. We shallow-clone the caller'snodesandedgesso we don't mutate the upstream data, and strip the lib-mutated coordinate fields before handing a node back viaonNodeClick.hashId,hexToHsl,shadeFor) copied verbatim from the old version so per-course shading stays visually consistent.variant,comparison,pauseWhenOffscreen) which the 3D backend currently ignores but accepts for compat.What didn't make it (kept as prop placeholders)
orb,constellation,organism). OnlyLearn.tsxever passed one ("organism") and it renders fine without that hint. Variant-driven layout tweaks can land in a follow-up if the visual variant ever matters again.Trade-offs
Test plan
npx tsc --noEmit→ clean.npx vitest run→ 29 passed (no regressions).npx next build→ succeeds for all 17 routes./tree— hover/click nodes; verify selection panel opens; verify highlightId works (the suggested concept renders white)./learn— graph renders inside the chat side panel./dashboard— both call sites render.Out of scope for this PR (clean follow-ups)
RingGeometry.next↔postcss(independent of this PR; flagged for an upgrade follow-up).🤖 Generated with Claude Code
Summary by CodeRabbit