fix(ui): export a typed MOCK_SUBGRAPHS map from the mock data module (#1763) - #2131
fix(ui): export a typed MOCK_SUBGRAPHS map from the mock data module (#1763)#2131branarakic wants to merge 1 commit into
Conversation
…1763) `mocks/provider.ts` reached for the per-context-graph sub-graph override through `(mock as any).MOCK_SUBGRAPHS`, but `mocks/data.ts` never exported that symbol. The optional lookup kept mock mode from crashing — it always fell through to the empty list — so the only visible symptom was a deterministic Vite warning on every production build: src/ui/mocks/provider.ts (23:24): "MOCK_SUBGRAPHS" is not exported by "src/ui/mocks/data.ts", imported by "src/ui/mocks/provider.ts". and the per-CG override the provider's own comment advertised could never fire. Exports the map rather than deleting the lookup, so the documented extension point actually works. It is typed against the real `/api/sub-graph/list` response shape (`{ contextGraphId, subGraphs: SubGraphInfo[] }`) and mirrors the sibling `MOCK_PARTICIPANTS: Record<string, ...>` pattern two functions up, which lets the `as any` go. `cg:supply-chain-eu` is deliberately left without an entry so the empty-list fallback stays exercised by a real mock CG rather than only by a synthetic id. Verified: the missing-export warning reproduces on `main` and is gone here; `tsc --noEmit` is clean; the full node-ui suite passes (155 files, 2195 tests). Closes #1763 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: WARNING: failed to clean up stale arg0 temp dirs: Permission denied (os error 13)
| // Only CGs whose sub-graph UI is worth exercising need an entry — the | ||
| // provider falls back to an empty list for anything absent, which is what | ||
| // `cg:supply-chain-eu` deliberately exercises. | ||
| export const MOCK_SUBGRAPHS: Record< |
There was a problem hiding this comment.
MINOR — Only 1 of the 3 fetchSubGraphs consumers goes through the mock provider — mock mode now self-contradicts
The PR's premise is that "the per-CG override the provider's comment advertised could never fire." After this fix it fires for exactly one of the three consumers, because the other two bypass api-wrapper entirely:
src/ui/views/ProjectView.tsx:447→api.fetchSubGraphs(...)(wrapped → mock path). This is the Overview Subgraphs stat.src/ui/components/SubGraphBar.tsx:12,223→import { fetchSubGraphs } from '../api.js'(direct, unwrapped).src/ui/views/project/components/subgraph.tsx:2,64(SubGraphOverviewGrid) →import { fetchSubGraphs } from '../../../api.js'(direct, unwrapped).
In mock mode the daemon is unreachable by definition (/api/status failed detection), so the two direct callers still reject: SubGraphBar swallows it (.catch(() => {}) → subGraphs = [] → merged.length === 0 → return null), and SubGraphOverviewGrid sets fetchError and renders the failure/teaching empty state. Only the wrapped Overview stat picks up MOCK_SUBGRAPHS.
Net effect for a demo-mode operator: the Overview stat strip reads Subgraphs: 2 (overview.tsx:524-526, subGraphCount.toLocaleString()), and clicking through to the Subgraph Explorer for the same CG shows no chip row and the "couldn't load subgraphs / no subgraphs yet" body. Before this PR all three read 0 / empty, which was at least self-consistent.
The new test suite structurally cannot catch this: it calls mockApi.fetchSubGraphs in isolation and never exercises a consumer, so "provider-level tests" (issue AC #3) pass while the rendered surfaces disagree. Either route SubGraphBar + SubGraphOverviewGrid through api-wrapper (the same "Codex review bug F" fix already applied to the Overview stat), or drop the two entries whose only observable effect today is to desync the stat from the page it links to.
Concrete failure scenario
Stop the daemon, load the UI (mock mode latches), open the Pharma Drug Interactions project. Overview stat strip shows Subgraphs 2; navigate to the Subgraph Explorer tab for the same CG and it renders zero chips plus the empty/failed state. On testnet-canary both read 0.
Self-review: raised by an independent reviewer pass, then verified against the PR head before posting.
| contextGraphId: 'cg:climate-science', | ||
| subGraphs: [ | ||
| { | ||
| name: 'Arctic Ice', |
There was a problem hiding this comment.
MINOR — name: 'Arctic Ice' is a sub-graph name the daemon can never emit (whitespace is rejected)
SubGraphInfo.name is the sub-graph slug, not a display label. Every consumer treats it that way: SubGraphBar.tsx builds slug: sg.name and profile.forSubGraph(sg.name); SubGraphOverviewGrid matches it against entity.subGraphs membership; lib/subGraphs.ts::isUserFacingSubGraph compares it against RESERVED_SUB_GRAPH_SLUGS. The human-readable label is supposed to arrive from the profile binding — displayName: binding.displayName ?? sg.name.
packages/core/src/constants.ts:650 (validateSubGraphName) rejects /[<>"{}|^\\s]/— whitespace included. SocreateSubGraph(cg, 'Arctic Ice')throwsInvalid sub-graph name "Arctic Ice": Sub-graph name contains characters unsafe for IRIs, and subGraphDeregistrationSparql (publisher/src/metadata.ts) asserts the same guard. There is no reachable state in which /api/sub-graph/listreturns this row. The fixture's own comment claims it "mirrors the real/api/sub-graph/list response shape"; this row does not. Use a real slug (arctic-ice`) and let the profile binding supply "Arctic Ice".
(Capitals are legal, so Interactions/Contraindications are representable — but they are the same slug-vs-display-label conflation, and they are what the demo renders as chip labels.)
Same defect class, fold in here: the climate entry omits createdBy and createdAt. generateSubGraphRegistration (publisher/src/metadata.ts:1785-1794) always emits both quads, and dkg-agent-cg-registry.ts:937 declares createdBy: string as required on the daemon's own return type — the route at cli/src/daemon/routes/context-graph.ts:1004-1010 passes them straight through. A response with no createdBy is likewise unreachable.
Concrete failure scenario
Take the fixture as the reference for the endpoint and try to reproduce it against a live node: agent.createSubGraph('cg:climate-science', 'Arctic Ice') throws Invalid sub-graph name "Arctic Ice". Meanwhile mock mode renders a chip whose slug contains a space, a state real data cannot reach, and whose profile.forSubGraph('Arctic Ice') can never match a daemon-emitted binding.
Self-review: raised by an independent reviewer pass, then verified against the PR head before posting.
| subGraphs: [ | ||
| { | ||
| name: 'Interactions', | ||
| uri: 'cg:pharma-drug-interactions/interactions', |
There was a problem hiding this comment.
MINOR — uri drops the did:dkg:context-graph: prefix and its tail contradicts name
Real sub-graph URIs are did:dkg:context-graph:<cgId>/<name> — contextGraphSubGraphUri (packages/core/src/constants.ts:563) and generateSubGraphRegistration (packages/publisher/src/metadata.ts:1785), which mints the registration subject from exactly that template. The daemon's list route echoes the stored sg.uri verbatim and keys its counts off the first path segment after did:dkg:context-graph:<cg>/, so in production uri === \did:dkg:context-graph:${contextGraphId}/${name}`` is an invariant.
The fixture breaks it twice over: the prefix is missing ('cg:pharma-drug-interactions/interactions'), and the trailing segment is not name — interactions vs 'Interactions' (line 220), arctic-ice vs 'Arctic Ice' (line 243).
No current consumer reads SubGraphInfo.uri (grepped packages/node-ui/src — zero hits), so there is no broken code path today; the cost is fixture fidelity for a module whose stated job is "mirroring the real /api/sub-graph/list response shape", and a demo that would silently look correct to the next person who adds a uri-derived lookup.
Concrete failure scenario
A later change resolves the named graph to query it, or derives the slug as uri.split('/').pop(). Against a real node both work; against the fixture the graph URI matches nothing and the derived slug (interactions) is not the slug the rest of the UI uses (Interactions), so the demo path silently diverges from production.
Self-review: raised by an independent reviewer pass, then verified against the PR head before posting.
| expect(result).toEqual({ contextGraphId: 'cg:does-not-exist', subGraphs: [] }); | ||
| }); | ||
|
|
||
| it('keys every override by its own contextGraphId, matching the real response shape', () => { |
There was a problem hiding this comment.
MINOR — The "matching the real response shape" test asserts nothing about the shape
The case is titled keys every override by its own contextGraphId, matching the real response shape, but the only per-sub-graph assertions are typeof subGraph.name === 'string', typeof subGraph.uri === 'string' and Number.isFinite(...) on the two counts (lines 46-49). That is satisfied by any two non-empty strings — it is precisely why the two shape defects above ship green.
The invariants that actually define the response are cheap to assert here:
expect(subGraph.uri).toBe(\did:dkg:context-graph:${id}/${subGraph.name}`)— matchescontextGraphSubGraphUri/generateSubGraphRegistration`.expect(validateSubGraphName(subGraph.name).valid).toBe(true)—@origintrail-official/dkg-core, the same guardcreateSubGraphruns.expect(typeof subGraph.createdBy).toBe('string')— required on the daemon's ownlistSubGraphsreturn type.
As written, the suite pins the export against regression (which it does well) but gives no protection on the thing its title claims to cover.
Concrete failure scenario
Someone adds a fourth mock sub-graph with name: 'my/sub graph' and uri: 'nonsense'. Every assertion in this test passes, the fixture is impossible for the daemon to produce, and nothing in CI notices.
Self-review: raised by an independent reviewer pass, then verified against the PR head before posting.
| // contract so the export cannot regress back into an untyped lookup. | ||
| describe('mockApi.fetchSubGraphs (GH#1763)', () => { | ||
| it('exports a typed MOCK_SUBGRAPHS map from the mock data module', () => { | ||
| expect(MOCK_SUBGRAPHS).toBeDefined(); |
There was a problem hiding this comment.
MINOR — Test 1 is subsumed by the other tests and passes for null and {}
Test 1 (lines 12-15) adds a named test to the report but no detection power beyond what tests 2, 3, 5 and 6 already have.
It is not literally dead — I checked the runtime semantics rather than assuming, because it matters here. Under vite-node a named import of a missing export resolves to undefined at runtime rather than throwing a link error (confirmed with a minimal repro: expect(MOCK_SUBGRAPHS).toBeDefined() → AssertionError: expected undefined to be defined, no import-time failure). So test 1 does fire when the export is deleted.
The problem is that so does everything else. In that same state, test 2 throws on MOCK_SUBGRAPHS['cg:pharma-drug-interactions'], test 3 throws on line 29, test 5 throws on Object.entries(undefined), and test 6 throws on Object.keys(undefined) — verified end to end: Tests 5 failed | 1 passed. Test 1 is a strict subset.
And its own assertions are weaker than they look:
expect(x).toBeDefined()only checks!== undefined, so it passes fornull.expect(typeof x).toBe('object')also passes fornull(typeof null === 'object').
So export const MOCK_SUBGRAPHS = null as any satisfies test 1 in full, as does = {}. Neither is a state anyone wants. If the intent is "the export exists and is a usable map," expect(Object.keys(MOCK_SUBGRAPHS).length).toBeGreaterThan(0) is one line and actually distinguishes those cases; otherwise the test is fine to drop, since the export's existence is implied by every other test in the file.
Concrete failure scenario
Someone reduces the fixture to export const MOCK_SUBGRAPHS = {} while removing the two CG entries. Test 1's name — "exports a typed MOCK_SUBGRAPHS map from the mock data module" — reports green, as it also would for null as any. A reader scanning the suite output concludes the typed-export contract is verified; the only test that actually noticed the change was test 2.
Self-review: raised by an independent reviewer pass, then verified against the PR head before posting.
Self-reviewIndependent multi-lens review (correctness / test-quality), then a verification pass to refute each finding before posting. 8 raised on this PR, 5 survived — posted inline. This PR came out worst of the three, and the theme is consistent: the fixture claims to mirror the real Posted
Findings 2–4 compound: the fixture is wrong, and the test that claims to check the fixture's shape is the reason nothing caught it. Raised and rejected
🤖 Generated with Claude Code |
| contextGraphId: 'cg:pharma-drug-interactions', | ||
| subGraphs: [ | ||
| { | ||
| name: 'Interactions', |
There was a problem hiding this comment.
🔴 Bug: Mock sub-graph names use labels instead of daemon slugs
What's wrong
The mock API is now exercising a different contract than /api/sub-graph/list. SubGraphInfo.name is not just a label in this UI; it is the slug used for filtering, profile lookup, selection state, and API calls. These new values can drift from real sub-graph membership and one of them is invalid by the daemon's own validator, so mock mode can show sub-graphs that cannot behave like real sub-graphs.
Example
In mock mode, fetchSubGraphs('cg:climate-science') returns { name: 'Arctic Ice', uri: 'cg:climate-science/arctic-ice' }. A user selecting that sub-graph makes the UI treat Arctic Ice as the subGraphName, but that name cannot be created or queried against the daemon because whitespace is invalid; locally derived maps keyed by arctic-ice also will not match.
Suggested direction
Return daemon-valid slug names from the mock API and put human-readable labels somewhere the UI already expects display metadata.
For Agents
In packages/node-ui/src/ui/mocks/data.ts, make MOCK_SUBGRAPHS[*].subGraphs[*].name use the real daemon slug (interactions, contraindications, arctic-ice) and keep display labels in the profile binding or description path. Update mock-provider-subgraphs.test.ts to assert slug names and include at least one case proving name matches the URI tail / validates as a daemon subGraphName.
Summary
mocks/provider.tsreached for the per-context-graph sub-graph override through(mock as any).MOCK_SUBGRAPHS, butmocks/data.tsnever exported that symbol. The optional lookup kept mock mode from crashing — it always fell through to the empty list — so the only visible symptom was a deterministic Vite warning on every production build:…and the per-CG override the provider's own comment advertised could never fire.
Changes
mocks/data.ts— exportMOCK_SUBGRAPHS, typed against the real/api/sub-graph/listresponse shape ({ contextGraphId, subGraphs: SubGraphInfo[] }).mocks/provider.ts— read it directly; theas anygoes away.test/mock-provider-subgraphs.test.ts— new provider-level coverage.Why export rather than delete the lookup. The issue allows either, but its third acceptance criterion asks that "mock-mode fallback and override behavior have provider-level tests" — deleting satisfies the build warning while leaving that criterion unmeetable. Exporting also mirrors the sibling
MOCK_PARTICIPANTS: Record<string, {...}>pattern two functions up.cg:supply-chain-euis deliberately left without an entry, so the empty-list fallback stays exercised by a real mock CG rather than only by a synthetic id.Test Plan
tsc --noEmitclean.Related Issues
Closes #1763
🤖 Generated with Claude Code