-
Notifications
You must be signed in to change notification settings - Fork 10
fix(ui): export a typed MOCK_SUBGRAPHS map from the mock data module (#1763) #2131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: testnet-canary
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import type { NotificationsFeedResponse } from '../api.js'; | ||
| import type { NotificationsFeedResponse, SubGraphInfo } from '../api.js'; | ||
|
|
||
| export const MOCK_STATUS = { | ||
| name: 'my-dkg-node', | ||
|
|
@@ -198,3 +198,53 @@ export const MOCK_SESSIONS = { | |
| }, | ||
| ], | ||
| }; | ||
|
|
||
| // GH#1763 — mock sub-graph lists keyed by CG id, mirroring the real | ||
| // `/api/sub-graph/list` response shape. `provider.ts` previously reached for | ||
| // this map through `(mock as any).MOCK_SUBGRAPHS`, which the module never | ||
| // exported: the optional lookup kept mock mode from crashing, but the | ||
| // per-CG override the comment promised could never fire and the production | ||
| // Vite build emitted a missing-export warning on every run. | ||
| // | ||
| // 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< | ||
| string, | ||
| { contextGraphId: string; subGraphs: SubGraphInfo[] } | ||
| > = { | ||
| 'cg:pharma-drug-interactions': { | ||
| contextGraphId: 'cg:pharma-drug-interactions', | ||
| subGraphs: [ | ||
| { | ||
| name: 'Interactions', | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Bug: Mock sub-graph names use labels instead of daemon slugs What's wrong Example Suggested direction For Agents |
||
| uri: 'cg:pharma-drug-interactions/interactions', | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MINOR — Real sub-graph URIs are The fixture breaks it twice over: the prefix is missing ( No current consumer reads Concrete failure scenarioA later change resolves the named graph to query it, or derives the slug as Self-review: raised by an independent reviewer pass, then verified against the PR head before posting. |
||
| description: 'Pairwise drug interaction records', | ||
| createdBy: 'did:dkg:agent:0x1111111111111111111111111111111111111111', | ||
| createdAt: '2026-04-02T09:15:00Z', | ||
| entityCount: 148, | ||
| tripleCount: 1721, | ||
| }, | ||
| { | ||
| name: 'Contraindications', | ||
| uri: 'cg:pharma-drug-interactions/contraindications', | ||
| createdBy: 'did:dkg:agent:0x2222222222222222222222222222222222222222', | ||
| createdAt: '2026-04-05T16:40:00Z', | ||
| entityCount: 79, | ||
| tripleCount: 604, | ||
| }, | ||
| ], | ||
| }, | ||
| 'cg:climate-science': { | ||
| contextGraphId: 'cg:climate-science', | ||
| subGraphs: [ | ||
| { | ||
| name: 'Arctic Ice', | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MINOR —
(Capitals are legal, so Same defect class, fold in here: the climate entry omits Concrete failure scenarioTake the fixture as the reference for the endpoint and try to reproduce it against a live node: Self-review: raised by an independent reviewer pass, then verified against the PR head before posting. |
||
| uri: 'cg:climate-science/arctic-ice', | ||
| description: 'Sea-ice extent projections', | ||
| entityCount: 32, | ||
| tripleCount: 410, | ||
| }, | ||
| ], | ||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { MOCK_CONTEXT_GRAPHS, MOCK_SUBGRAPHS } from '../src/ui/mocks/data.js'; | ||
| import { mockApi } from '../src/ui/mocks/provider.js'; | ||
|
|
||
| // GH#1763 — `provider.ts` used to read `(mock as any).MOCK_SUBGRAPHS`, a symbol | ||
| // `data.ts` never exported. The optional lookup stopped mock mode from | ||
| // crashing, so the only visible symptom was a Vite missing-export warning on | ||
| // every production build — and the per-CG override the provider's own comment | ||
| // advertised silently could not work. These tests pin both halves of the | ||
| // 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(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MINOR — Test 1 is subsumed by the other tests and passes for 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 The problem is that so does everything else. In that same state, test 2 throws on And its own assertions are weaker than they look:
So Concrete failure scenarioSomeone reduces the fixture to Self-review: raised by an independent reviewer pass, then verified against the PR head before posting. |
||
| expect(typeof MOCK_SUBGRAPHS).toBe('object'); | ||
| }); | ||
|
|
||
| it('returns the per-context-graph override when one is defined', async () => { | ||
| const result = await mockApi.fetchSubGraphs('cg:pharma-drug-interactions'); | ||
|
|
||
| expect(result).toEqual(MOCK_SUBGRAPHS['cg:pharma-drug-interactions']); | ||
| expect(result.contextGraphId).toBe('cg:pharma-drug-interactions'); | ||
| expect(result.subGraphs).toHaveLength(2); | ||
| expect(result.subGraphs.map((s) => s.name)).toEqual(['Interactions', 'Contraindications']); | ||
| }); | ||
|
|
||
| it('falls back to an empty list for a context graph with no override', async () => { | ||
| // `cg:supply-chain-eu` is a real mock CG deliberately left out of | ||
| // MOCK_SUBGRAPHS so the fallback path stays covered. | ||
| expect(MOCK_SUBGRAPHS['cg:supply-chain-eu']).toBeUndefined(); | ||
|
|
||
| const result = await mockApi.fetchSubGraphs('cg:supply-chain-eu'); | ||
|
|
||
| expect(result).toEqual({ contextGraphId: 'cg:supply-chain-eu', subGraphs: [] }); | ||
| }); | ||
|
|
||
| it('falls back to an empty list for an entirely unknown context graph', async () => { | ||
| const result = await mockApi.fetchSubGraphs('cg:does-not-exist'); | ||
|
|
||
| expect(result).toEqual({ contextGraphId: 'cg:does-not-exist', subGraphs: [] }); | ||
| }); | ||
|
|
||
| it('keys every override by its own contextGraphId, matching the real response shape', () => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MINOR — The "matching the real response shape" test asserts nothing about the shape The case is titled The invariants that actually define the response are cheap to assert here:
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 scenarioSomeone adds a fourth mock sub-graph with Self-review: raised by an independent reviewer pass, then verified against the PR head before posting. |
||
| for (const [id, entry] of Object.entries(MOCK_SUBGRAPHS)) { | ||
| expect(entry.contextGraphId).toBe(id); | ||
| for (const subGraph of entry.subGraphs) { | ||
| expect(typeof subGraph.name).toBe('string'); | ||
| expect(typeof subGraph.uri).toBe('string'); | ||
| expect(Number.isFinite(subGraph.entityCount)).toBe(true); | ||
| expect(Number.isFinite(subGraph.tripleCount)).toBe(true); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| it('only overrides context graphs that mock mode actually lists', () => { | ||
| const knownCgIds = new Set(MOCK_CONTEXT_GRAPHS.contextGraphs.map((cg) => cg.id)); | ||
| for (const id of Object.keys(MOCK_SUBGRAPHS)) { | ||
| expect(knownCgIds.has(id)).toBe(true); | ||
| } | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-wrapperentirely: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/statusfailed detection), so the two direct callers still reject:SubGraphBarswallows it (.catch(() => {})→subGraphs = []→merged.length === 0→return null), andSubGraphOverviewGridsetsfetchErrorand renders the failure/teaching empty state. Only the wrapped Overview stat picks upMOCK_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 read0/ empty, which was at least self-consistent.The new test suite structurally cannot catch this: it calls
mockApi.fetchSubGraphsin isolation and never exercises a consumer, so "provider-level tests" (issue AC #3) pass while the rendered surfaces disagree. Either routeSubGraphBar+SubGraphOverviewGridthroughapi-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 Interactionsproject. Overview stat strip showsSubgraphs 2; navigate to the Subgraph Explorer tab for the same CG and it renders zero chips plus the empty/failed state. Ontestnet-canaryboth read 0.Self-review: raised by an independent reviewer pass, then verified against the PR head before posting.