Uh oh!
There was an error while loading. Please reload this page.
test(e2e): journey — graph render integrity vs DB (#395) - #433
Conversation
📝 WalkthroughWalkthroughThe graph components now expose stable test IDs and node identifiers across 2D and 3D renderers. A new Playwright suite derives graph expectations from database rows and validates rendered nodes, edges, mastery tiers, labels, counts, and filters. ChangesGraph integrity validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant graphSpec as graph.spec.ts
participant database as Graph database
participant treePage as /tree page
participant graphDom as Graph DOM
graphSpec->>database: Load graph nodes, edges, and enrolled courses
graphSpec->>treePage: Navigate to /tree
treePage->>graphDom: Render SVG graph and accessibility node list
graphSpec->>graphDom: Validate IDs, counts, tiers, labels, opacity, and edges
graphSpec->>graphDom: Apply tier filters and verify node partitioning
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 | c308689 | Commit Preview URL Branch Preview URL | Jul 28 2026, 04:10 AM |
Loads /tree and asserts the rendered knowledge graph against raw-SQL reads of graph_nodes/graph_edges/enrollments (new read-only dbQuery seam in e2e/support/db.ts): - exact node/edge render counts vs DB (+1 subject-root hub per distinct enrolled course, +1 hub spoke per course node) — marked test.fixme pending open bug #355, which duplicates the CS subject root on the rich seed (two offerings of the same abstract course); the correct assertion is kept, not relaxed - every DB concept node renders exactly once, with the mastery class derived from its DB mastery_score (config.get_mastery_tier thresholds) encoded at the render layer (2D node-circle opacity) - the /tree tier filter partitions nodes exactly by the DB-derived classification Data assertions only — no force-layout geometry is read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review findings on PR #433: label-based node identification was a latent gap (graph_nodes names are only unique PER COURSE, so cross-course name or truncation-prefix collisions would fail a correct render — and the label-keyed opacity map could silently mask a wrong class), and bare structural/ARIA anchoring is not a sanctioned seam. Fix both by minting graph-surface testids per the doc's process: - KnowledgeGraph2D/3D a11y list: graph-node-items / graph-node-item (+ data-node-id per entry) / graph-node-activate - KnowledgeGraph2D SVG layer: graph-node (+ data-node-id), graph-node-circle (the opacity-encoding mark), graph-edge, graph-zoom-in/out/reset - register all of it in docs/frontend-testids.md (graph surface row now names the 2D/3D split) and add both files to the eslint enforcement array (every intrinsic button is tagged — no suppressions needed) The spec now identifies nodes exclusively by data-node-id and keys the opacity map by id; rendered label text is still asserted per node, just keyed by id instead of used as identity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ca9a2e7 to
c308689CompareThere was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/e2e/graph.spec.ts`:
- Around line 232-235: Replace the count-only assertion in
frontend/e2e/graph.spec.ts:232-235 with endpoint-pair validation covering every
expected DB edge and synthesized hub spoke, while preserving the existing
count-floor check if needed. In
frontend/src/components/KnowledgeGraph2D.tsx:448-459, add stable source- and
target-node-ID data attributes to each graph-edge line so the test can compare
rendered identities against expected pairs.
🪄 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: 3b170e05-e5bb-417d-a4f6-fbaa3a2c3c21
📒 Files selected for processing (5)
docs/frontend-testids.mdfrontend/e2e/graph.spec.tsfrontend/eslint.config.mjsfrontend/src/components/KnowledgeGraph2D.tsxfrontend/src/components/KnowledgeGraph3D.tsx
| // Edge floor: at least every DB edge + hub spoke is drawn (exact equality | ||
| // is #355-blocked — the duplicate hub adds surplus spokes, but a MISSING | ||
| // edge must still fail here). | ||
| expect(await svgEdges.count()).toBeGreaterThanOrEqual(g.expectedEdgeCount); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert expected edge identities, not only a count floor.
Bug #355 currently adds five surplus spokes, so >= g.expectedEdgeCount still passes if up to five expected edges disappear. Expose each SVG line’s source/target IDs, then assert every DB edge and synthesized hub spoke is present; this remains valid while the exact-count test is fixme.
frontend/e2e/graph.spec.ts#L232-L235: compare the rendered edge endpoint pairs against all expected DB-edge and hub-spoke pairs.frontend/src/components/KnowledgeGraph2D.tsx#L448-L459: add stable source and target node-ID data attributes to eachgraph-edgeline.
Proposed seam
<line
data-testid="graph-edge"
+ data-source-node-id={s.id}+ data-target-node-id={t.id}
x1={s.x}📝 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.
| // Edge floor: at least every DB edge + hub spoke is drawn (exact equality | |
| // is #355-blocked — the duplicate hub adds surplus spokes, but a MISSING | |
| // edge must still fail here). | |
| expect(awaitsvgEdges.count()).toBeGreaterThanOrEqual(g.expectedEdgeCount); | |
| <line | |
| key={i} | |
| data-testid="graph-edge" | |
| data-source-node-id={s.id} | |
| data-target-node-id={t.id} | |
| x1={s.x} | |
| y1={s.y} | |
| x2={t.x} | |
| y2={t.y} | |
| stroke="var(--text-muted)" | |
| strokeOpacity={op} | |
| strokeWidth={0.5+(l.strength||0.5)*1.2} | |
| strokeLinecap="round" | |
| /> |
📍 Affects 2 files
frontend/e2e/graph.spec.ts#L232-L235(this comment)frontend/src/components/KnowledgeGraph2D.tsx#L448-L459
🤖 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/graph.spec.ts` around lines 232 - 235, Replace the count-only
assertion in frontend/e2e/graph.spec.ts:232-235 with endpoint-pair validation
covering every expected DB edge and synthesized hub spoke, while preserving the
existing count-floor check if needed. In
frontend/src/components/KnowledgeGraph2D.tsx:448-459, add stable source- and
target-node-ID data attributes to each graph-edge line so the test can compare
rendered identities against expected pairs.
AndresL230
commented
Jul 28, 2026
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. (Panel findings fixed in-branch: the spec now anchors on id-keyed registered testids — graph-node-item + data-node-id on the a11y list, per docs/frontend-testids.md's Adding-a-surface process — which also structurally eliminates the latent label-collision gap; the db.ts helper converged on the merged queryRaw. Re-verified with a 3/3 confirmation cycle on the final content; the original 10/10×2 tally stands as acceptance evidence. The #355 acceptance test ships as test.fixme.) 🤖 Generated with Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
Summary
Journey #395: load the graph view (
/tree) and assert the RENDERED knowledge graph against the database read over raw SQL — data assertions only, no force-layout geometry (positions are never read).frontend/e2e/graph.spec.ts(3 tests):graph_nodesrow plus one subject-root hub per DISTINCT enrolled course; one rendered edge pergraph_edgesrow plus one hub spoke per course-linked node.test.fixme— red solely because of open bug Duplicate subject_root node in /api/graph → React duplicate-key warnings in KnowledgeGraph2D #355 (below); the correct assertion is kept, not relaxed, so this is the ready-made acceptance test for the Duplicate subject_root node in /api/graph → React duplicate-key warnings in KnowledgeGraph2D #355 fix.mastery_scoremaps through the canonical thresholds (backend/config.py::get_mastery_tier) to the class the 2D graph encodes as node-circle opacity./treeshows exactly the concepts whose DB score maps to that tier, and none other.frontend/e2e/support/db.ts::dbQuery, a read-only raw-SQL helper so journeys compute expected values from the DB without hand-rollingpgplumbing (inherits the loopback-only safety guard). No new testids: the spec anchors on the registeredgraph-containertestid plus structural/ARIA handles inside it (svg[aria-label="Knowledge graph"]<text>/<line>, the component's hidden a11y node list) — justification in the spec header; node identity matches on seeded concept names (DB data, not UI copy).frontend/e2e/global-setup.tsfix from the test(e2e): journey — seeded session → dashboard #386 branch verbatim (storageState now includes thesapling_userlocalStorage identity; cookie-only state left authed pages on an infinite skeleton).Bug #355 REPRODUCES on the migration-replayed local schema + rich seed
Probe (authed
GET /api/graph/rich-user-activeagainst the freshly seeded stack):rich-user-activeis enrolled in two offerings of the same abstract course (rich-off-cs101-f25+rich-off-cs101-s26), andgraph_service.get_graphsynthesizes onesubject_root__{course_id}per enrollment (loop over_user_enrolled_courses) instead of per distinct course — the CS subject root and its five hub spokes are emitted twice, exactly the duplicate-key defect #355 describes. This is the defect class this journey exists to catch: test 1 trips on it and istest.fixme(#355)with the correct assertions intact; tests 2–3 assert everything the duplication does not corrupt.Verification — 10 consecutive local runs
10/10 green. Each run:
2 passed, 1 skipped— the skip is thetest.fixme(#355)exact-count test; the two live tests passed on every run (~28s/run cold, ~11s/run warm). The full cycle was observed green twice back-to-back (20/20 runs total across both cycles).Cycle run under the shared stack lock:
make e2e-up→ probe → 10×npx playwright test e2e/graph.spec.ts→make e2e-down. A second probe on a volume carrying sibling-run residue reported the SAME single duplicate id (nodes_total=20 nodes_unique=19 dup_node_ids=["subject_root__rich-course-cs101"]) — totals drift with un-reset stack state, the dup signal does not; the spec itself is immune because every test truncates + re-seeds first.Part of #402, closes#395
🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Accessibility
Documentation