Replies: 15 comments
Review Comment 1: RPC Client Setup — Verified & DetailedI verified the exact RPC client pattern against the codebase. There are currently 3 dedicated
All three are structurally identical. Other routes (Grants, Divisions, etc.) use inline Confirmed blocker: The data-sources route file is correctly structured for RPC inference: it uses Recommended revalidate: 60s (not the default 300s) since this is an operational monitoring dashboard where staleness matters. The grants dashboard also uses 60s. |
Review Comment 2: Grants FK — Pipeline-Based Backfill Is Correct, But More Invasive Than DescribedVerified the full grants sync pipeline. Key finding:
Files that need changes (more than the plan implies):
Backfill: re-running Risk: The |
Review Comment 3: Sidebar Placement — 20 Items Is Already CrowdedThe "Dashboards" section in Adding "Data Sources" makes 21. The items are loosely alphabetical but not strictly grouped by domain. Recommendation: Place it between "Data Quality" and "Divisions" (both D-words, and it's semantically related to Data Quality and Source Checks). But this is a band-aid — the sidebar needs subcategories long-term: This isn't in scope for this plan, but worth noting as tech debt. Filing as a separate issue would be appropriate. |
Review Comment 4: Empty States & Data Availability — Critical for LaunchVerified the existing empty-state patterns across dashboards. Three patterns exist: Pattern A (Grants): Explicit empty state card with actionable message {stats.total === 0 && (
<div className="rounded-lg border border-border/60 p-8 text-center text-muted-foreground my-6">
<p className="text-lg font-medium mb-2">No grants imported yet</p>
<p className="text-sm">Use the grant import pipeline to sync grants...</p>
</div>
)}Pattern B (Data Quality): Early return with CLI command suggestion if (snapshots.length === 0) {
return (<div><DataSourceBanner source="api" />
<p>No snapshots yet. Run <code>crux sys quality snapshot</code>...</p>
</div>);
}Pattern C (Source Check): Graceful fallback — renders with empty data, no special UI. For the Data Sources dashboard, Pattern B is most appropriate because:
Concrete empty states needed:
|
Review Comment 5: Publisher Entity Resolution — Server-Side Pattern RequiredConfirmed that The grants dashboard solves this with a function resolveEntityName(stableId: string): string {
const entity = getTypedEntityById(stableId);
return entity?.title ?? stableId;
}
function enrichWithNames(grants: RpcGrantRow[]): GrantRow[] {
return grants.map((g) => ({
...g,
organizationName: resolveEntityName(g.organizationId),
granteeName: g.granteeId ? resolveEntityName(g.granteeId) : undefined,
}));
}For the Data Sources dashboard, apply the same pattern: function enrichDataSources(sources: RpcDataSource[]): DataSourceRow[] {
return sources.map((s) => ({
...s,
publisherName: s.publisherEntityId
? resolveEntityName(s.publisherEntityId)
: null,
publisherHref: s.publisherEntityId
? getEntityHref(s.publisherEntityId)
: null,
}));
}Then the client table renders Publisher coverage: 11 of 14 manifests have |
Review Comment 6: Detail Page — rawContent Hazard & Simplified ApproachThe What NOT to do: // BAD — fetches potentially 50MB in the server component
export default async function DataSourceDetailPage({ params }) {
const snapshot = await client[':id'].snapshots.latest.$get(); // 50MB!
return <SnapshotPreview content={snapshot.rawContent} />;
}What to do instead — two-tier approach: Tier 1 (server-rendered, always visible): Fetch the detail + snapshot list (metadata only, no rawContent): // Server component fetches lightweight data
const [source, snapshots] = await Promise.all([
client[':id'].$get({ param: { id } }), // ~1KB
client[':id'].snapshots.$get({ param: { id } }), // ~5KB metadata
]);Tier 2 (client-side, on-demand): A "Show preview" button triggers a client-side fetch: "use client"
function SnapshotPreview({ sourceId }: { sourceId: string }) {
const [content, setContent] = useState<string | null>(null);
const [loading, setLoading] = useState(false);
const handleLoad = async () => {
setLoading(true);
const res = await fetch(`/api/data-sources/${sourceId}/snapshots/latest`);
const data = await res.json();
setContent(data.rawContent?.slice(0, 5000)); // TRUNCATE client-side
setLoading(false);
};
if (!content) return <button onClick={handleLoad}>Show preview</button>;
return <pre className="max-h-64 overflow-y-auto text-xs">{content}</pre>;
}Even better — add a server-side truncation option: A future enhancement could add a For PR 2 MVP, the simplest approach is: Don't show rawContent preview at all. Show metadata (date, records, hash, mapping validity) + a "View raw JSON" link to the API endpoint ( |
Review Comment 7: sourceSchema Is Always Null — Column Mapping Table Needs RedesignVerified: // snapshot-capture.ts lines 55-75
await syncDataSource({
id: manifest.sourceId,
name: manifest.name,
// ...
columnMapping: Object.fromEntries(
manifest.schema.fields.filter(f => f.internalField).map(f => [f.sourceName, f.internalField!])
),
verificationConfig: manifest.verification ? { ... } : undefined,
// sourceSchema is NOT set — always null in DB
});Impact on the detail page Column Mapping section: The plan proposes showing "Source Column → Internal Field, Type, Transform". But:
Options:
For PR 2, option 1 is sufficient. Option 2 could be bundled if convenient. |
Review Comment 8: Staleness Logic — Static Sources Need Distinct TreatmentThe plan's Verified behavior: All 14 manifests have explicit Revised logic (already in the plan, confirming it's correct): type Freshness = 'fresh' | 'due' | 'overdue' | 'static' | 'unknown';
function computeFreshness(lastSnapshotAt: string | null, updateFrequency: string | null): Freshness {
if (updateFrequency === 'static') return 'static'; // ← distinct state
if (!lastSnapshotAt || !updateFrequency) return 'unknown';
// ... rest
}Badge rendering:
Sorting: For the "default sort by overdue first" behavior: This surfaces problems first while pushing static/healthy sources to the bottom. No |
Review Comment 9: Data Quality Dashboard — Use
|
Review Comment 10: Aggregate Stats — Need a
|
Review Comment 11: Verification Stats — Blocked Until Grants FK ShipsThe plan proposes
SELECT g.data_source_id,
COUNT(*) as total,
COUNT(*) FILTER (WHERE v.verdict = 'confirmed') as confirmed,
COUNT(*) FILTER (WHERE v.verdict = 'contradicted') as contradicted
FROM source_check_verdicts v
JOIN grants g ON v.record_id = g.id AND v.record_type = 'grant'
WHERE g.data_source_id IS NOT NULL
GROUP BY g.data_source_id
Dependency chain: PR 2 (Grants FK) → backfill → verification-stats endpoint → Phase C dashboard integration. This is correctly placed in the deferred work. The plan should explicitly note that the Source-Check cross-link card in Phase C can only show data source health (staleness/mapping), NOT verification coverage, until the grants FK + stats endpoint ship. Simpler Phase C card (no verification dependency): This only needs data from the existing |
Review Comment 12: Grants Table UI — Don't Link Public Page to Internal DashboardThe plan proposes showing Problem: The Revised approach for the grants table source column: Public grants table (
Individual grant detail page (
Filter addition (regardless of link behavior):
|
Review Comment 13: Deferred Items — Priority RankingFor clarity, here's a ranked list of the deferred work items from the plan, ordered by value/effort ratio: High Value, Low Effort (do next after MVP)
Medium Value, Medium Effort (do if demand exists)
Low Value or High Risk (defer indefinitely)
|
Review Comment 14: Final Implementation ChecklistConsolidating all findings into a concrete checklist per PR: PR 1: Data Sources DashboardCreate:
Modify:
Key decisions:
PR 2: Detail Page + Grants FKCreate:
Modify:
Key decisions:
Phase C (lightweight, after PR 1 + PR 2)
|
|
Branch: Cross-link: data sources cluster. This discussion is one of three that together describe a single data-sources subsystem but currently don't reference each other. Worth treating them as coordinated. Siblings: #2928 (Websites as Data Feeds) is the upstream extraction half — registers org websites as structured feeds and extracts facts from dated page snapshots into TableBase/FactBase with full provenance (the |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Context
The data sources system (Phases 1-4, PR #3572 + #3571) shipped with full backend infrastructure but zero frontend visibility:
data_sources+source_snapshotstables in PG (14 grant sources registered)/api/data-sources/(list, detail, sync, snapshots CRUD)crux tb data-sources list|show|snapshot|verify|healthThe problem: Users cannot browse data sources, see snapshot history, check freshness, or understand verification results without terminal access. All monitoring and debugging requires CLI.
This plan was red-teamed by 4 parallel review agents (technical feasibility, UX coherence, data-layer correctness, scope/risk). The findings are incorporated below, with specific blockers and risks called out.
Architecture: Two PRs, Three Phases
Based on review feedback, the original 7-workstream plan is scoped down to a focused MVP (2 PRs) plus deferred enhancements. Key cuts based on review:
<pre>with truncated text + API link instead.publisherEntityId; lower priority.PR 1: Data Sources Dashboard (core visibility)
Goal: Make all data sources visible in the UI for the first time.
Files to Create
apps/web/src/app/internal/data-sources/page.tsx/wiki/E<allocated>apps/web/src/app/internal/data-sources/data-sources-content.tsxapps/web/src/app/internal/data-sources/data-sources-table.tsx"use client"— DataTable with search/filterscontent/docs/internal/data-sources-dashboard.mdxFiles to Modify
apps/web/tsconfig.json"@wiki-server/data-sources-route"path alias (BLOCKING — without this, RPC types don't work)apps/web/src/lib/wiki-server.tsgetDataSourcesRpcClient()+ export inferred typesapps/web/src/components/mdx-components.tsxDataSourcesContentapps/web/src/lib/wiki-nav.tsDashboard Layout
Design Decisions (from review)
4 stat cards, not 6 — "Active Sources" is redundant when most are active; "Total Snapshots" is operational minutiae for the detail page. Use
grid-cols-2 md:grid-cols-4.Staleness computation — computed client-side using inline
Math.round((Date.now() - d.getTime()) / 86400000)(nodate-fns— not installed in this project). Sources withupdateFrequency = 'static'show a neutral gray "static" badge, NOT "fresh" — a dataset published once in 2022 shouldn't look recently refreshed.Publisher resolution — EntityLink is a server component and cannot be used inside the
"use client"table. Follow the grants-dashboard pattern: resolve publisher names/hrefs in the server component, pass plain strings to the client table.Empty state — if no data sources are synced yet, show a centered card: "No data sources registered yet. Run
pnpm crux tb data-sources snapshot --allto populate."Freshness colors: fresh=emerald, due=amber, overdue=red, static=gray, unknown=gray. Status colors: active=blue, archived=muted, defunct=red (distinct from freshness to avoid confusion).
Format badge colors: CSV=blue, HTML=purple, JSON=green, Spreadsheet=orange.
Staleness Logic
Tests Required
computeFreshness()— all 5 states including edge cases (null dates, null frequency, static, exact boundary values)PR 2: Data Source Detail Page + Grants FK
Goal: Enable drill-down into individual sources and link grants to their data sources.
Part A: Detail Page
Files to create:
apps/web/src/app/internal/data-sources/[id]/page.tsxNote: This is a standard App Router dynamic route, NOT Pattern A (no entity ID per source). With only 14 sources, this is appropriate. All detail content lives in
page.tsxdirectly.Detail page sections:
columnMappingJSONB). No Type/Transform columns —sourceSchemais currently always null in the database. Add Type/Transform columns whensourceSchemais populated in a future PR.GET /:id/snapshots. Most recent row highlighted.GET /api/data-sources/{id}/snapshots/latestAPI endpoint + a<pre className="max-h-64 overflow-y-auto">showing the first 2000 characters with "..." truncation. This avoids building a CSV/HTML/JSON parser.Blocker from review: rawContent can be up to 50MB. The detail page MUST NOT fetch the full snapshot in the server component. The raw preview should be a client-side fetch triggered by a "Show preview" button, fetching only when requested.
Part B: Grants FK + Backfill
Migration:
apps/wiki-server/drizzle/01XX_add_data_source_id_to_grants.sqlThis is safe — nullable column + FK is a catalog-only change, no table rewrite.
Drizzle schema update: Add
dataSourceId: text("data_source_id").references(() => dataSources.id, { onDelete: "set null" })to the grants table inschema.ts.Backfill strategy (REVISED based on review):
The original URL-pattern-matching approach was flagged as unreliable:
fetchUrl: nulluse various fallback URLsRevised approach — pipeline-based backfill:
dataSourceIdtoSyncGrantItemSchemain the grants routedataSourceIdtotoSyncGrant()insync.ts— set from the GrantSource's manifestsourceIdpnpm crux import-grants sync— this re-syncs all grants and populatesdataSourceIddeterministically from the source that parsed themThis eliminates the fragile URL-pattern backfill entirely.
Grants table UI enhancement:
Modify the "source" column in
grants-table.tsx:dataSourceIdset: show source name as text (not linked to internal page from public route) + unobtrusive freshness dot (green/amber/red)sourceURL: current behavior (plain link)/internal/data-sources/[id]Grant sync pipeline changes (more invasive than initially described):
apps/wiki-server/src/routes/tablebase/grants.ts: UpdateSyncGrantItemSchema,formatRow(), insert values,onConflictDoUpdatesetcrux/lib/grant-import/sync.ts: PassdataSourceIdfrom manifest to sync payloadapps/wiki-server/src/schema.ts: Add column to Drizzle schemaTests required:
dataSourceIdPhase C: Cross-Link Cards (Lightweight Enhancement)
After PR 1 and PR 2 ship, add minimal integration to existing dashboards:
Source-Check Coverage dashboard — add one card:
Data Quality dashboard — add one card using the existing
extraJSONB column (no migration needed):The
data_quality_snapshots.extraJSONB column was explicitly designed for "future metrics without migration" — use it to storedataSourcesActive,dataSourcesStalecounts.Deferred Work (Future PRs, If Demand Exists)
GET /:id/snapshots/latest/preview?rows=20GET /api/data-sources/:id/verification-statspublisherEntityId; needs API query filtersGET /api/data-sources//data-feedspagesourceSchemais always null in DBcaptureSourceSnapshot()to populatesourceSchemaBlockers & Risks Identified by Review
Blockers (must address during implementation)
@wiki-server/data-sources-routetsconfig path aliasapps/web/tsconfig.jsondate-fnsnot installeddifferenceInDays()doesn't existMath.round((Date.now() - d.getTime()) / 86400000)EntityLinkis a server component"use client"DataTablesourceSchemaalways null in DB<pre>Risks
dataSourceIddata_sourcestable empty in productioncrux tb data-sources snapshot --allruns before dashboard launchAPI Work Required (Not Just UI)
The plan requires server-side changes beyond pure frontend work:
getDataSourcesRpcClient()in wiki-server.tsdataSourceIdon grants Drizzle schema + migrationdataSourceIdinSyncGrantItemSchema+formatRow()+ insert/conflictGET /api/data-sources/statsaggregate endpointGET /api/data-sources/:id/verification-statsGET /api/data-sources/Estimated Scope
Open Questions
Should the detail page be a Pattern A dashboard per source (allocate E-numbers for each data source)? Review says no — 14 dynamic routes is fine. But this means data source detail pages won't appear in search/navigation the way wiki pages do.
Should we add a
GET /api/data-sources/statsaggregate endpoint in PR 1? This would provide total snapshots, total records, stale count server-side rather than computing in the UI. Clean but adds API surface.Is the grants pipeline re-run approach acceptable for backfill? It will re-sync all ~10K+ grants, which takes a few minutes. Alternative: write a one-time script that reads the GrantSource registry to map existing grants by their deterministic ID hash.
Priority of the cross-link cards (Phase C)? They're low effort but the source-check and data-quality dashboards are already dense. Worth adding?
All reactions