Add demographics Sankey diagram with cross-filtering#9
Conversation
✅ Deploy Preview for study-palette ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Adds a demographics Sankey visualization to the demo UI and wires it into the existing cross-filtering model (condition/procedure filters) by introducing new participant demographic fields and filter chips.
Changes:
- Extend demo participant generation and filtering to include sex, race, ethnicity, and smoking status; add Sankey data builder utilities.
- Add a new
DemographicsSankeycomponent (Nivo Sankey) and render it in demo mode; enable click-to-filter behavior. - Update FilterPanel and styling to display demographic filter chips; add test polyfill for
ResizeObserver.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/test-setup.ts | Adds ResizeObserver stub for Vitest/jsdom. |
| ui/src/index.css | Adds demographic chip styling and Sankey-specific layout styles. |
| ui/src/demoData.ts | Adds demographic fields, weighted generation, demographic filtering, and Sankey data builder. |
| ui/src/FilterPanel.tsx | Renders removable demographic filter chips and includes demographics in “has filters” logic. |
| ui/src/DemographicsSankey.tsx | New Nivo Sankey component with node labels and click-to-add-filter behavior. |
| ui/src/App.tsx | Computes Sankey data from filtered participants and renders the Sankey in demo mode. |
| ui/package.json | Adds @nivo/sankey dependency. |
| ui/bun.lockb | Adds a Bun binary lockfile artifact as part of dependency update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "dependencies": { | ||
| "@nivo/sankey": "^0.99.0", | ||
| "react": "^19.1.0", | ||
| "react-dom": "^19.1.0", | ||
| "recharts": "^3.7.0" |
There was a problem hiding this comment.
Dependency was added, but the repository’s existing Bun text lockfile (ui/bun.lock) was not updated (the new dependency only appears in bun.lockb). Please ensure the lockfile format the repo standardizes on is updated so installs are reproducible in CI and for other developers.
| <DemographicsSankey | ||
| data={sankeyData} | ||
| onFilterAdd={handleFilterAdd} | ||
| /> |
There was a problem hiding this comment.
New Sankey UI/behavior is introduced here (rendering + click-to-filter), but App.test.tsx doesn’t currently assert that the Demographics section renders in demo mode or that clicking a Sankey node updates the FilterPanel chips. Adding a couple of tests around rendering and filter interaction would help prevent regressions.
| const FILTER_KEY_MAP: Record<string, keyof ActiveFilters> = { | ||
| sex: "sex", | ||
| race: "race", | ||
| ethnicity: "ethnicity", | ||
| smokingStatus: "smokingStatus", | ||
| }; |
There was a problem hiding this comment.
FILTER_KEY_MAP is typed as Record<string, keyof ActiveFilters>, which implies every string key is valid. That defeats the runtime check and can hide mistakes when parsing dimKey. Use a narrower type (e.g., const FILTER_KEY_MAP = {...} as const and derive keyof typeof FILTER_KEY_MAP, or Partial<Record<string, keyof ActiveFilters>>) so unknown keys are correctly typed as undefined and handled explicitly.
| const nodes: SankeyNode[] = []; | ||
| const nodeIds = new Set<string>(); | ||
|
|
There was a problem hiding this comment.
nodeIds is populated but never used. This adds dead code and suggests a missing validation step. Either remove nodeIds entirely, or use it to validate that source/target IDs built for links exist in nodes (and drop/guard invalid ones).
| .sankey-container { | ||
| overflow: hidden; | ||
| } | ||
|
|
There was a problem hiding this comment.
.sankey-container is defined here but not used by any component (no matching className). Either wrap the Sankey in an element using this class (if overflow: hidden is needed), or remove the unused rule to avoid stale CSS.
| .sankey-container { | |
| overflow: hidden; | |
| } |
| display: flex; | ||
| justify-content: space-between; | ||
| padding: 0 10px 0 10px; | ||
| margin-right: 140px; |
There was a problem hiding this comment.
.sankey-labels uses a hard-coded margin-right: 140px, but the Sankey chart uses margin.right = 160 in DemographicsSankey.tsx, so the column labels are likely to drift out of alignment. Consider deriving both from the same value (or using a CSS layout that doesn’t depend on magic numbers) so responsive behavior is consistent.
| margin-right: 140px; | |
| margin-right: 160px; |
Summary
Test plan