Uh oh!
There was an error while loading. Please reload this page.
feat(graph-3d): Focused Minimal visual upgrade — labels, hover focus, recenter - #530
feat(graph-3d): Focused Minimal visual upgrade — labels, hover focus, recenter#530AndresL230 wants to merge 18 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pgrade Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ght halo Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… run warmup ticks under test/reduced-motion Round 2 on d2e59a9's camera-framing fix, which was unit-correct but had no runtime effect. Confirmed root cause via node_modules/3d-force-graph source plus live frame-by-frame logging against a running dev server: 1. onEngineStop fires synchronously *before* tickFrame's "update node positions" step writes that tick's x/y/z onto each node's Three.js object (both run inside the same synchronous layoutTick() call, stop callback first) — so fitting immediately (or even one rAF later) measures a stale/near-empty bbox and ends up far too close once the real, spread-out layout lands moments later. Fix: poll on requestAnimationFrame until two consecutive getGraphBbox() reads agree (frame-capped safety net) before calling zoomToFit. 2. Under cooldownTicks={0} (test/reduced-motion mode), warmupTicks was left at its default of 0 too, so the 3D force simulation never actually ran — nodes stayed at d3-force-3d's raw pre-simulation positions, an overlapping cluster. KnowledgeGraph2D already solves this the same way for its own reduced-motion path (`sim.alpha(1).tick(200).alpha(0).stop()`); mirrored it here via `warmupTicks={reducedMotion || IS_TEST_MODE ? 200 : 0}`. Verified visually against a dev server (:3010) proxying the held-up E2E stack's backend: before, /tree in 3D was a tiny illegible clump; after, four well-separated, readable node clusters filling most of the canvas, matching the 2D layout's structure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cel rAF on unmount Round 3 re-review of the round-2 camera-framing fix (mechanism, determinism, and tests independently verified) flagged one new Important issue and a hygiene nit introduced by the async polling window itself: 1. The bbox-stabilization poll's closure never re-checked that it still belonged to the current dataset. If nodes/edges changed while an old poll was mid-flight (up to MAX_FRAMES/~1s wide), the stale closure kept polling the live instance and could fire zoomToFit(400, 60) after — clobbering — the fresh poll's correct fit for the new dataset. Fixed with an epoch guard: pollEpochRef increments alongside didFitRef.current = false in the [nodes, edges] reset effect; handleEngineStop captures the epoch at poll start, and every scheduled frame callback bails immediately if pollEpochRef.current no longer matches. 2. Hygiene: no cancelAnimationFrame cleanup tied the poll to component lifetime, so an unmount mid-poll could leave up to ~60 dangling no-op rAF callbacks. Now tracks the current rAF id in pollRafIdRef and cancels it in an unmount-only effect. TDD: wrote the new test first against the pre-fix code (stashed the implementation), confirmed it failed exactly as described (stale poll fires zoomToFit(400, 60) for the old dataset), then restored the implementation and confirmed green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe opt-in 3D knowledge graph now uses custom spheres, halos, and labels. Shared helpers provide sizing, colors, themes, and adjacency. Hover interactions focus one-hop neighborhoods, links dim accordingly, and a reset control refits the camera. ChangesFocused Minimal 3D graph
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Pointer
participant KnowledgeGraph3D
participant VisualRegistry
participant ForceGraph3D
Pointer->>KnowledgeGraph3D: Hover a graph node
KnowledgeGraph3D->>VisualRegistry: Update node, label, and halo opacity
KnowledgeGraph3D->>ForceGraph3D: Recompute link color and width
ForceGraph3D-->>KnowledgeGraph3D: Render focused neighborhood
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 |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 987bfd7 | Commit Preview URL Branch Preview URL | Aug 19 2026, 09:09 PM |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/components/graph/KnowledgeGraph3D.test.tsx (2)
553-573: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the frame-cap test deterministic and pin the cap value.
Line 557 uses
Math.random()to produce a bbox that never repeats. The sibling test at Line 588 uses a monotonic counter for the same purpose, which is deterministic.The test also asserts only that
zoomToFitfired once. It does not assert how many frames elapsed. IfMAX_FRAMESregressed from 60 to 2, this test would still pass. Assert the read count to pin the cap.🧪 Proposed deterministic frame-cap test
- getGraphBboxMock.mockImplementation(() => ({ x: [0, Math.random()], y: [0, 0], z: [0, 0] }));+ let capCounter = 0;+ getGraphBboxMock.mockImplementation(() => ({ x: [0, ++capCounter], y: [0, 0], z: [0, 0] }));expect(zoomToFitSpy).toHaveBeenCalledTimes(1); expect(zoomToFitSpy).toHaveBeenCalledWith(400, 60); + // Pins MAX_FRAMES: the poll reads the bbox once per frame up to the cap.+ expect(getGraphBboxMock).toHaveBeenCalledTimes(60); expect(raf.queue.length).toBe(0); // no frame scheduled after the cap-triggered fit🤖 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/graph/KnowledgeGraph3D.test.tsx` around lines 553 - 573, Make the frame-cap test deterministic by replacing Math.random() in getGraphBboxMock with a monotonic counter so each bounding-box read differs predictably. Track the number of bbox reads and assert it equals the configured MAX_FRAMES value (60), while preserving the existing single fit and empty RAF queue assertions.
171-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the unmount cancellation path.
captureAnimationFramesalready spies oncancelAnimationFrameat Line 178, and the spy is exposed only throughrestore(). No test asserts that the unmount cleanup effect inKnowledgeGraph3D.tsx(Lines 202-206) cancels the pending frame. The PR lists that cleanup as a deliverable, so a regression that drops the effect would pass the current suite.Return the
cancelAnimationFramespy from the helper, then assert it in a new test.🧪 Proposed helper change and test
const caf = vi.spyOn(window, "cancelAnimationFrame").mockImplementation(() => {}); return { queue, + caf,it("cancels the in-flight bbox poll on unmount",()=>{getGraphBboxMock.mockReturnValue({x: [0,1],y: [0,1],z: [0,1]});constraf=captureAnimationFrames();const{ unmount }=render(<KnowledgeGraph3Dnodes={[makeNode()]}edges={[]}/>);act(()=>(lastProps!.onEngineStopas()=>void)());expect(raf.queue.length).toBe(1);unmount();expect(raf.caf).toHaveBeenCalledTimes(1);raf.restore();});🤖 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/graph/KnowledgeGraph3D.test.tsx` around lines 171 - 193, Update captureAnimationFrames to expose the cancelAnimationFrame spy, then add a KnowledgeGraph3D unmount test that starts the bbox polling frame through onEngineStop, unmounts the component, and asserts the cancellation spy was called. Restore the animation-frame spies after the assertion.frontend/src/components/graph/KnowledgeGraph3D.tsx (1)
303-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the link colors from the resolved theme instead of hardcoded RGB literals.
Lines 306, 311, and 312 hardcode
rgba(138, 131, 114, ...)andrgba(138, 154, 91, ...).138, 154, 91is#8a9a5b, which isFALLBACK_THEME.accent. Nodes and halos useresolveGraphTheme(), which reads--accentand--ink-200from CSS. If a deployment or an alternate color scheme overrides those variables, node colors follow the theme and link colors do not.Convert
theme.accentandtheme.dimto RGB components once, then build the rgba strings from them.🎨 Proposed theme-derived link colors
+ // Link colors follow the same resolved theme as nodes and halos.+ const linkRgb = React.useMemo(() => {+ const rgb = (hex: string) => {+ const c = new THREE.Color(hex);+ return `${Math.round(c.r * 255)}, ${Math.round(c.g * 255)}, ${Math.round(c.b * 255)}`;+ };+ return { lit: rgb(theme.accent), dim: rgb(theme.dim) };+ }, [theme]);+ const linkColor = React.useCallback( (l: object) => { const link = l as FG3DLink; - if (!hoverId) return `rgba(138, 131, 114, ${BASE_LINK_ALPHA})`;+ if (!hoverId) return `rgba(${linkRgb.dim}, ${BASE_LINK_ALPHA})`; const lit = linkEndId(link.source) === hoverId || linkEndId(link.target) === hoverId; - // Lit links take the sage accent (rgb of `#8a9a5b`); dimmed links- // fade to near-invisible warm gray.+ // Lit links take the resolved accent; dimmed links fade to the+ // resolved dim token. return lit - ? `rgba(138, 154, 91, ${LIT_LINK_ALPHA})`- : `rgba(138, 131, 114, ${DIM_LINK_ALPHA})`;+ ? `rgba(${linkRgb.lit}, ${LIT_LINK_ALPHA})`+ : `rgba(${linkRgb.dim}, ${DIM_LINK_ALPHA})`; }, - [hoverId],+ [hoverId, linkRgb], );If you accept this, update the literal expectations in
frontend/src/components/graph/KnowledgeGraph3D.test.tsxat Lines 471, 478, 479, and 481-483 to build the same strings fromFALLBACK_THEME.🤖 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/graph/KnowledgeGraph3D.tsx` around lines 303 - 315, Update the linkColor callback to use the resolved graph theme’s accent and dim colors instead of hardcoded RGB values, converting theme.accent and theme.dim to RGB components once and using them for the rgba strings. Preserve the existing hover, lit, and dim alpha behavior, and update the corresponding KnowledgeGraph3D.test.tsx expectations to construct the same values from FALLBACK_THEME.
🤖 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 `@docs/superpowers/plans/2026-08-05-3d-graph-focused-minimal.md`:
- Around line 937-947: Update the shell command around the E2E stack startup to
register an EXIT trap that runs make e2e-down before make e2e-up executes.
Preserve the existing test return-code collection and ensure teardown runs
exactly once, including when make e2e-up fails under set -e.
In `@frontend/src/components/graph/KnowledgeGraph3D.tsx`:
- Around line 243-285: Update nodeThreeObject to store the returned group
alongside sphereMat, halo, and label in visualsRef.current. Before replacing the
registry when the dataset changes and during component unmount, iterate the
existing entries and dispose both SphereGeometry instances, their materials, the
SpriteText canvas texture/material resources, and each group’s children; then
clear the registry before creating the new Map.
---
Nitpick comments:
In `@frontend/src/components/graph/KnowledgeGraph3D.test.tsx`:
- Around line 553-573: Make the frame-cap test deterministic by replacing
Math.random() in getGraphBboxMock with a monotonic counter so each bounding-box
read differs predictably. Track the number of bbox reads and assert it equals
the configured MAX_FRAMES value (60), while preserving the existing single fit
and empty RAF queue assertions.
- Around line 171-193: Update captureAnimationFrames to expose the
cancelAnimationFrame spy, then add a KnowledgeGraph3D unmount test that starts
the bbox polling frame through onEngineStop, unmounts the component, and asserts
the cancellation spy was called. Restore the animation-frame spies after the
assertion.
In `@frontend/src/components/graph/KnowledgeGraph3D.tsx`:
- Around line 303-315: Update the linkColor callback to use the resolved graph
theme’s accent and dim colors instead of hardcoded RGB values, converting
theme.accent and theme.dim to RGB components once and using them for the rgba
strings. Preserve the existing hover, lit, and dim alpha behavior, and update
the corresponding KnowledgeGraph3D.test.tsx expectations to construct the same
values from FALLBACK_THEME.
🪄 Autofix
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: 99d36f84-0079-4d42-a82d-ce69d01fc611
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
docs/superpowers/plans/2026-08-05-3d-graph-focused-minimal.mddocs/superpowers/specs/2026-08-05-3d-graph-focused-minimal-design.mdfrontend/eslint-suppressions.jsonfrontend/package.jsonfrontend/src/components/graph/KnowledgeGraph3D.test.tsxfrontend/src/components/graph/KnowledgeGraph3D.testmode.test.tsxfrontend/src/components/graph/KnowledgeGraph3D.tsxfrontend/src/components/graph/graph3dHelpers.test.tsfrontend/src/components/graph/graph3dHelpers.ts
| flock /tmp/claude-1000/sapling-e2e-stack.lock bash -c ' | ||
| set -e | ||
| export SAPLING_MODEL_MODE=<value from e2e.yml> | ||
| export SAPLING_FUNCTION_HANDLERS=<value from e2e.yml> | ||
| make e2e-up | ||
| rc=0 | ||
| (cd frontend && npx playwright test) || rc=$? | ||
| (cd backend && venv/bin/python -m e2e_oracles) || rc=$? | ||
| make e2e-down | ||
| exit $rc | ||
| ' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Ensure teardown runs when make e2e-up fails.
set -e exits before make e2e-down if make e2e-up fails. This can leave a partially started E2E stack running. Register teardown with an EXIT trap before startup.
Proposed fix
set -e
+ trap "make e2e-down || true" EXIT
export SAPLING_MODEL_MODE=<value from e2e.yml>
export SAPLING_FUNCTION_HANDLERS=<value from e2e.yml>
make e2e-up
rc=0
(cd frontend && npx playwright test) || rc=$?
(cd backend && venv/bin/python -m e2e_oracles) || rc=$?
- make e2e-down
exit $rc📝 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.
| flock /tmp/claude-1000/sapling-e2e-stack.lock bash -c ' | |
| set -e | |
| export SAPLING_MODEL_MODE=<valuefrome2e.yml> | |
| export SAPLING_FUNCTION_HANDLERS=<valuefrome2e.yml> | |
| make e2e-up | |
| rc=0 | |
| (cd frontend && npx playwright test) || rc=$? | |
| (cd backend && venv/bin/python -m e2e_oracles) || rc=$? | |
| make e2e-down | |
| exit $rc | |
| ' | |
| flock /tmp/claude-1000/sapling-e2e-stack.lock bash -c ' | |
| set -e | |
| trap "make e2e-down || true" EXIT | |
| export SAPLING_MODEL_MODE=<valuefrome2e.yml> | |
| export SAPLING_FUNCTION_HANDLERS=<valuefrome2e.yml> | |
| make e2e-up | |
| rc=0 | |
| (cd frontend && npx playwright test) || rc=$? | |
| (cd backend && venv/bin/python -m e2e_oracles) || rc=$? | |
| exit $rc | |
| ' |
🤖 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 `@docs/superpowers/plans/2026-08-05-3d-graph-focused-minimal.md` around lines
937 - 947, Update the shell command around the E2E stack startup to register an
EXIT trap that runs make e2e-down before make e2e-up executes. Preserve the
existing test return-code collection and ensure teardown runs exactly once,
including when make e2e-up fails under set -e.
| const nodeThreeObject = React.useCallback( | ||
| (raw: object) => { | ||
| const n = raw as FG3DNode; | ||
| const r = nodeRadius(n); | ||
| const color = baseNodeColor(n, theme); | ||
| const group = new THREE.Group(); | ||
| const sphereMat = new THREE.MeshLambertMaterial({ | ||
| color, | ||
| transparent: true, | ||
| opacity: NODE_OPACITY, | ||
| }); | ||
| group.add(new THREE.Mesh(new THREE.SphereGeometry(r, 24, 24), sphereMat)); | ||
| // Focus halo: slightly larger translucent accent sphere, hidden | ||
| // until this node is hovered or is the persistent highlightId. | ||
| const halo = new THREE.Mesh( | ||
| new THREE.SphereGeometry(r * 1.4, 16, 16), | ||
| new THREE.MeshBasicMaterial({ | ||
| color: theme.accent, | ||
| transparent: true, | ||
| opacity: 0.28, | ||
| depthWrite: false, | ||
| }), | ||
| ); | ||
| halo.visible = n.id === hoverRef.current || n.id === highlightRef.current; | ||
| group.add(halo); | ||
| const spec = labelSpec(n); | ||
| const label = new SpriteText(n.name); | ||
| label.textHeight = spec.textHeight; | ||
| label.fontWeight = spec.fontWeight; | ||
| label.color = theme.ink; | ||
| label.fontFace = '"JetBrains Mono", monospace'; | ||
| label.material.transparent = true; | ||
| label.position.set(0, -(r + spec.textHeight + 1.5), 0); | ||
| group.add(label); | ||
| visualsRef.current.set(n.id, { sphereMat, label, halo, baseColor: color }); | ||
| return group; | ||
| }, | ||
| [theme], | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Description: Check whether the repository already disposes Three.js resources anywhere, and whether SpriteText exposes a dispose path.set -euo pipefail
rg -n --type=ts --type=tsx -C3 '\.dispose\s*\(' frontend/src ||echo"no dispose() calls found in frontend/src"
fd -t d 'three-spritetext' frontend/node_modules --max-depth 3 --exec sh -c 'ls "$1"; sed -n "1,80p" "$1/dist/three-spritetext.d.ts" 2>/dev/null' _ {}Repository: SaplingLearn/Sapling
Length of output: 340
🏁 Script executed:
#!/bin/bash0bashset -euo pipefail
printf'Tracked KnowledgeGraph files:\n'
fd -i 'KnowledgeGraph3D\.(ts|tsx)$'.||trueprintf'\nGit file list for likely front-end components:\n'
git ls-files | rg 'frontend/src/(components|.*Graph|.*three|package\.json)$|componentst'printf'\nSearch dispose across tracked source files:\n'
git ls-files | rg '\.(ts|tsx)$'| xargs -r rg -n -- '-- dispose\(' or trueRepository: SaplingLearn/Sapling
Length of output: 289
🏁 Script executed:
#!/bin/bashset -euo pipefail
FILE="frontend/src/components/graph/KnowledgeGraph3D.tsx"printf'File line count:\n'
wc -l "$FILE"printf'\nRelevant section 160-300:\n'
sed -n '160,300p'"$FILE"| nl -ba -v160
printf'\nSearch for three-spritetext references and package manifests:\n'
rg -n "three-spritetext|SpriteText|FR3D|FG3DNode|visualsRef|nodeThreeObject|useFrame|useThree|useMemo""$FILE"||true
rg -n "three-spritetext|react-three-fiber|`@react-three`" frontend -g 'package.json' -g 'package-lock.json' -g 'pnpm-lock.yaml' -g 'yarn.lock'||trueRepository: SaplingLearn/Sapling
Length of output: 298
🏁 Script executed:
#!/bin/bashset -euo pipefail
FILE="frontend/src/components/graph/KnowledgeGraph3D.tsx"printf'Relevant section 160-300:\n'
sed -n '160,300p'"$FILE"| awk '{printf "%5d %s\n", NR+159, $0}'printf'\nImports and dependency references:\n'
sed -n '1,80p'"$FILE"| awk '{printf "%5d %s\n", NR, $0}'
rg -n "three-spritetext|SpriteText|FR3D|FG3DNode|visualsRef|nodeThreeObject|useFrame|useThree|useMemo|useEffect|React""$FILE"||trueprintf'\nDependency files:\n'forfin package.json frontend/package.json frontend/package-lock.json frontend/pnpm-lock.yaml frontend/yarn.lock frontend/package.json;doif [ -f"$f" ];thenprintf'\n--- %s ---\n'"$f"
rg -n "three-spritetext|react-three-fiber|`@react-three`|three""$f"||truefidoneprintf'\nSearch disposed resource handling across tracked source files:\n'
git ls-files | xargs -r rg -n -- '-- dispose\s*\('| rg '\.(ts|tsx)$'||trueRepository: SaplingLearn/Sapling
Length of output: 13950
🌐 Web query:
three-spritetext v1.10.0 source canvas dispose texture code
💡 Result:
In three-spritetext v1.10.0, the process of disposing of the previous texture when updating the canvas is handled within the library's internal update logic. When the text content or styling changes, the library generates a new canvas and updates the sprite's material. The disposal logic is implemented as follows: // Inject canvas into sprite if (this.material.map) this.material.map.dispose; // gc previous texture var texture = this.material.map = new THREE.Texture(canvas); texture.minFilter = THREE.LinearFilter; texture.colorSpace = THREE.SRGBColorSpace; By calling this.material.map.dispose, the library ensures that the old texture is properly removed from GPU memory before assigning the new texture created from the updated canvas [1]. This practice is essential for preventing memory leaks when frequently updating sprite text [1][2].
Citations:
- 1: Using three.js version >= 152 requires texture to specify "SRGBColorSpace"? vasturiano/three-spritetext#41
- 2: https://deepwiki.com/vasturiano/three-spritetext
Dispose every node visual object when the dataset changes.
nodeThreeObject creates per-node Three.js resources: the returned group, two meshes with SphereGeometry, two materials, and a SpriteText with a canvas-backed Texture. Line 178 reassigns the registry to a new Map on a new dataset, so those previous objects become unreachable while their GPU buffers remain allocated.
Track the complete visual entry in visualsRef.current, including the group, then dispose the geometries, textures, and materials before replacing the registry and on component unmount.
🤖 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/graph/KnowledgeGraph3D.tsx` around lines 243 - 285,
Update nodeThreeObject to store the returned group alongside sphereMat, halo,
and label in visualsRef.current. Before replacing the registry when the dataset
changes and during component unmount, iterate the existing entries and dispose
both SphereGeometry instances, their materials, the SpriteText canvas
texture/material resources, and each group’s children; then clear the registry
before creating the new Map.
…ion instant fit, warmupTicks pins Three Important findings from the whole-branch final review, plus a doc amendment, fixed in one wave: 1. Accent drift, one color one source: FALLBACK_THEME.accent was still the retired sage (#8a9a5b) while globals.css's --accent is now the brighter forest (#2d8f5c) — so the focus halo (theme.accent) rendered forest while its own lit edges (hardcoded sage rgb) rendered sage. Corrected FALLBACK_THEME.accent to #2d8f5c and added hexToRgbTriplet(), a small pure helper next to resolveGraphTheme, so linkColor derives the lit-link rgba from theme.accent instead of a second hardcoded copy that can drift. 2. Reduced-motion camera animation: both zoomToFit call sites (the engine-settle auto-fit and the manual recenter button) now gate their duration — `reducedMotion || IS_TEST_MODE ? 0 : 400` — since these are system-initiated camera flies and an animated 400ms fly is itself a motion violation on the paths that already zero cooldownTicks to eliminate motion. Padding unified to 60 across both call sites (recenter button previously used 40). 3. warmupTicks was untested: pinned 200 in both the reduced-motion test (KnowledgeGraph3D.test.tsx) and the test-mode determinism test (KnowledgeGraph3D.testmode.test.tsx), and 0 in a new default-motion-path test — plus new coverage for the reduced-motion/test-mode instant-fit gating on the recenter button (the auto-fit path shares the identical ternary, already exercised by the bbox-stabilization tests under default motion). Also amended docs/superpowers/specs/2026-08-05-3d-graph-focused-minimal-design.md's "Camera & motion" section to document both justified deviations (the bbox-stabilization auto-fit poll + epoch guard, and warmupTicks=200 under test/reduced-motion mirroring KnowledgeGraph2D's sim.tick(200) precedent), and replaced the stale #8a9a5b halo color reference with "the app accent token (--accent, currently #2d8f5c), resolved at mount". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndresL230
commented
Aug 6, 2026
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
Jose-Gael-Cruz-Lopez
commented
Aug 19, 2026
Code review — graph-3d Focused Minimal upgradeThis turns the 3D mode from bare library spheres into the Focused Minimal design: FindingsP1[P1] Every hover transition destroys and rebuilds every link mesh — constlinkWidth=React.useCallback((l: object)=>{constlink=lasFG3DLink;constbase=0.4+(link.strength||0)*0.6;if(!hoverId)returnbase;constlit=linkEndId(link.source)===hoverId||linkEndId(link.target)===hoverId;returnlit ? base+0.6 : base;},[hoverId],);
[P1] Two new "src/components/graph/KnowledgeGraph3D.tsx": {
"react-hooks/refs": {
"count": 2
}
},The Canopy Engineering Style Guide §7 states the ratchet explicitly — "don't fix the whole backlog to land a change, but never add to it" — and this PR's own plan repeats it at [P1] Branch is
P2[P2] constsphereMat=newTHREE.MeshLambertMaterial({
color,transparent: true,opacity: NODE_OPACITY,});The halo correctly reads the live refs ( [P2] A halo geometry + material is allocated for every node although at most two are ever visible — group.add(newTHREE.Mesh(newTHREE.SphereGeometry(r,24,24),sphereMat));// Focus halo: slightly larger translucent accent sphere, hidden// until this node is hovered or is the persistent highlightId.consthalo=newTHREE.Mesh(newTHREE.SphereGeometry(r*1.4,16,16),newTHREE.MeshBasicMaterial({
[P2] Each label is rasterised five times at construction — constlabel=newSpriteText(n.name);label.textHeight=spec.textHeight;label.fontWeight=spec.fontWeight;label.color=theme.ink;label.fontFace='"JetBrains Mono", monospace';Every P3[P3] The base and dim link colors are still hardcoded hex while the lit one derives from the theme — if(!hoverId)return`rgba(138, 131, 114, ${BASE_LINK_ALPHA})`;
...
: `rgba(138, 131, 114, ${DIM_LINK_ALPHA})`;
Test-plan checklist — per-item verdict
What's good
Verdict: solid design work with one real hover-path performance bug (P1), a ratchet violation (P1) and a rebase (P1) to clear before this leaves draft. Review-only pass — no code changed and nothing fixed. Conventions checked against the Canopy |
…nder-phase ref writes Review round on #530. P1 — linkWidth was a useCallback keyed on the hoverId state, so every pointer enter AND leave handed react-force-graph-3d a new function identity. three-forcegraph treats linkWidth as object-invalidating for links (linkDataMapper.clear() -> digest([]) -> scene.remove() + _deallocate() of every link mesh, all CylinderGeometry here since the widths are never zero), so a hover was two full teardowns/rebuilds of the whole link layer — the exact opposite of the spec's "no per-hover geometry rebuilds". linkWidth now reads hoverRef with an empty dep array; widths still follow the hover because linkColor's own identity change triggers the digest and its onUpdateObj re-reads the width accessor per link. P1 — both react-hooks/refs violations were real render-phase ref mutations. highlightId is now mirrored in an effect (the useRef initializer covers first mount; kapsule's digest is debounce(fn, 1), so the library cannot call nodeThreeObject before the commit's effects have run). The visuals registry is no longer a ref cleared inside a useMemo — a discarded concurrent render would still have cleared it and left the committed tree with an empty registry and hover focus a silent no-op. One memo now owns everything scoped to a dataset: graphData, the registry, and the shared halo geometry/material. P2 — nodeThreeObject applies the live focus state to nodes it builds, so a dataset refresh under an active hover (tutor graph_update, /tree filter) no longer renders half-focused until the pointer moves. Focus styling has a single writer (applyVisualState) shared with the focus pass. P2 — one unit-sphere halo geometry + material per dataset, scaled per node, instead of N pairs of which at most two are ever visible; main sphere back to 16 segments (the library's nodeResolution default) from 24. P2 — SpriteText labels use the (text, textHeight, color) constructor: three of the five canvas rasterisations per label were setter-driven. P3 — the resting and dimmed link rgb are derived from a new GraphTheme.link resolved from --ink-400 instead of two hand-copied rgba(138, 131, 114, …) literals, which contradicted FALLBACK_THEME's "the ONE place that hex is allowed to be hardcoded". Also re-applies #538's header warning about never mounting this component outside the KnowledgeGraph wrapper.
…an gaps Ports #538's work forward by hand so the eventual merge with main is a no-op on these files: both 3D graph test files now use the shared @/test-utils/mockNextDynamic helper (copied verbatim from main) instead of a local inline dynamic mock. The helper renders the resolved component with createElement rather than calling it as a function, so the stub's hooks land in their own fiber, and it accepts this component's loader, which resolves to a bare function component. Removes every eslint-suppressions.json entry this branch had added, and the pre-existing @typescript-eslint/no-explicit-any entry too, by fixing the causes rather than baselining them: - the react-force-graph-3d stub is a NAMED function inside forwardRef (react/display-name) that records props in an effect instead of the render body (react-hooks/globals — Testing Library flushes passive effects before render/rerender/act return, so tests still read the props synchronously); - the capture is a spelled-out CapturedProps type, not Record<string, any>. Net effect vs the merge base: one entry removed, none added. Test-plan gaps: - the frame-cap test drove the bbox with Math.random() and asserted only that zoomToFit fired once, so a MAX_FRAMES regression from 60 to 2 would have passed. Monotonic counter now, and the bbox read count is pinned to the cap. - captureAnimationFrames already spied on cancelAnimationFrame but nothing asserted the unmount cleanup. The spy and the frame ids are exposed and a test covers it. - new test that linkWidth's identity is stable across a hover while linkColor's is not, and that the same accessor reports the widened value. - new test that nodeThreeObject applies focus to nodes built AFTER onNodeHover — the real order; the existing tests build first. - direct unit tests for the newly exported hexToRgbTriplet.
Under `set -e` a failing `make e2e-up` exited the flock'd shell before the trailing `make e2e-down` could run, leaving a half-started stack holding the ports. Register `trap "make e2e-down || true" EXIT` first and drop the explicit teardown.
…e graph The only path that exercises the new highlightRef mirror effect is a highlightId change while mounted — first mount is covered by the useRef initializer, so mutation-testing showed the effect could be deleted outright with all tests still green. This also pins the effect ORDER: the mirror is declared before the re-assert effect, and moving it after makes the re-assert read the stale highlight (verified: all three mutations now fail).
Byte-identical to main in that region, so the shared-helper adoption merges cleanly instead of conflicting on comment wording. The 'why an await import inside a hoisted vi.mock factory' rationale already lives in the helper's own docblock.
vi.mock calls are hoisted, so the order is behaviour-neutral — but this block sat inside the exact hunk #538 rewrote on main (the next/dynamic mock), which made an otherwise clean merge conflict on a pure addition. Moved into untouched context above.
The plan's pre-review code sketches prescribe the exact five shapes the review round removed (hover-keyed linkWidth, a visualsRef cleared inside a useMemo, focus-blind nodeThreeObject, per-node halo pairs, hardcoded link rgb). Left unmarked, the doc reads as authoritative and invites a future reader to 'restore' them.
Jose-Gael-Cruz-Lopez
commented
Aug 19, 2026
Review fixes appliedEvery outstanding finding on this PR (human review + CodeRabbit) has been addressed and pushed. Major
Minor / nits
Not done — CodeRabbit's disposal finding is not valid
Verification — Fixes applied and verified locally against this branch head; each figure above is a command I ran, not an estimate. |
…-minimal # Conflicts: # frontend/eslint-suppressions.json
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests