Conversation
BundleMonNo change in files bundle size Groups updated (1)
Final result: ✅ View report in BundleMon website ➡️ |
MatiasArriola
left a comment
MatiasArriola
left a comment
There was a problem hiding this comment.
I've just checked out this app and it's looking good and useful!
Some notes from my review:
- Tested it in Firefox. In my setup the app was not starting because of a
GPUShaderStage is undefinederror- I'd expect only the 3d visualizations to fail if the client does not have webgl enabled, not the whole app
- To get it working I went to
about:configand enableddom.webgpu.enabledandgfx.webrender.all(noting in case we want to add docs for firefox or something)
applicationfolder is not a standard we usually use. I see it's contents would belong todomain/usecases. (it happened to me that codex tends to create this application folder)Routeris there but it is not used.- We could consider using it, so in the future we could add some custom routes and share a direct url pointing to a metadata object
- Also we could remove dead code from the skeleton, like the ExamplePage.
d2-apiis not used.- It is interesting to see the dhis2-app-runtime
DataEnginealternative. I guess it is more flexible than d2-api, which is might be useful in this specific app, but it loses a lot of type safety. I have no problem with that, just mentioning we are not using the library which is our standard for data layer.
- It is interesting to see the dhis2-app-runtime
Also, I'm adding some notes from Claude. I did not check them thoroughly, but most look on point:
Should Fix
- Inconsistent dataSets support — src/domain/metadata/ResourceType.ts:25 omits dataSets from selectableResourceTypes, but BuildMetadataGraphUseCase and MetadataExplorerPage.tsx:14 handle it. Either dead code
or missing dropdown option. - N sequential HTTP requests — BuildMetadataGraphUseCase.ts:542-624 uses Future.sequential over independent per-id requests. Use Future.parallel or a single id:in:[…] filter.
- Unchecked as casts at data boundaries — BuildMetadataGraphUseCase.ts:48,124,194,252,364,457, MetadataDhis2Repository.ts:25,43,90, SystemDhis2Repository.ts:21, UserDhis2Repository.ts:24. Validate before
casting. - MetadataTestRepository.list returns empty regardless of query (MetadataTestRepository.ts:9-14) — graph use case is untestable.
- Webapp bypasses FutureData cancellation — MetadataExplorerPage.tsx:53-67 and MetadataGraphPanel.tsx:45-58,75-93 use .toPromise().then(), discarding the cancel function. Use .run() and call cancel from
useEffect cleanup. - Unsafe error handling — Dhis2App.tsx:14 does error as Error. Use instanceof check like api-futures.ts:15. Also verify App.tsx:9,18-19 legacy theme imports actually exist on the branch.
Recommendations
- Three.js module-level caches leak — MetadataGraphView3D.tsx:38-41 never disposes textures/geometries on unmount.
- BuildMetadataGraphUseCase is 732 lines — split into builder strategies, helpers, and entity types.
- Imperative accumulators in splitDataSetsByOverride, splitTopLevel, Identicon.ts:43-54 — convert to reduce.
- Duplicated request-id race-guard in MetadataExplorerPage and MetadataGraphPanel — extract or use proper cancellation.
- mergedGraph not memoized (MetadataGraphPanel.tsx:128) — invalidates the heavy 3D useMemo every render.
- Two competing fields parsers — splitTopLevel (MetadataTable.tsx:93) and normalizeFields (MetadataExplorerPage.tsx:196). Unify in domain/metadata/fields.ts.
- Silent empty results on shape mismatch — MetadataDhis2Repository.ts:89-95. Reject the future instead.
- Redundant Dhis2QueryParameters typing — pageSize?: number shadowed by index signature (MetadataDhis2Repository.ts:84-87).
- dangerouslySetInnerHTML in IdenticonAvatar.tsx:48 — safe today, but render as JSX to remove the trap.
- src/types/three.d.ts shadows real types — install @types/three and delete; would also remove the THREE as typeof THREE & {…} casts at MetadataGraphView3D.tsx:305-311.
- Double-escaped Windows paths in src/scripts/update-po.ts:60-64 — "\\" is two backslashes, should be "\".
Minor
- _props: {} in Dhis2App.tsx:5, App.tsx:21 — use Record<string, never>.
- registerNode callback ref recreated each render in MetadataGraphView.tsx:44-48.
- Math.min(200, …) duplicated in MetadataExplorerPage.tsx:49 and MetadataQueryBuilder.tsx:102 — extract MAX_PAGE_SIZE.
- Record<string, any> in vite.config.ts:67 — use import('vite').ProxyOptions.
- BuildStamp (App.tsx:71-73) transparent text still read by AT — add aria-hidden.
- feedbackDescriptionTemplate exported but unused (build-info.ts:16-19).
If you have any doubt or need help, just let me know!! I don't expect all the things mentioned to be fixed, but I hope it serves as a guide to improve the code.
Thanks @idelcano !!
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
…isolation, payload validation)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR migrates the legacy DHIS2 “skeleton app” into a “Metadata Visualizer” app: it replaces the old example/landing routes with a new metadata explorer (list + relationship graph), migrates DHIS2 API access to @dhis2/app-runtime’s DataEngine via repositories/use-cases, and modernizes build/dev tooling (Yarn 4, Node 22, build stamping, packaging scripts).
Changes:
- Replace legacy Router/Landing/Example pages with a new
MetadataExplorerPage+ 2D/3D graph panel and supporting UI/components. - Introduce Clean Architecture repositories/use-cases for metadata + system locale, backed by DHIS2
DataEngineadapters. - Update build pipeline (env/proxy, build commit/time injection, zip-build/update-po scripts) and repo tooling/config (Yarn 4, CI workflows, OpenSpec/Claude scaffolding).
Reviewed changes
Copilot reviewed 95 out of 105 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Loads env without prefix, adjusts DHIS2 dev proxy auth var, injects build commit/time, binds dev host. |
| src/webapp/utils/i18n-setup.ts | Centralizes i18n + RTL direction setup. |
| src/webapp/pages/Router.tsx | Removes legacy router (HashRouter + Switch + Example/Landing). |
| src/webapp/pages/metadata/MetadataExplorerPage.tsx | Adds metadata explorer page with query builder, results table, and graph panel. |
| src/webapp/pages/metadata/MetadataExplorerPage.css | Adds styling for metadata explorer, table, and graph views. |
| src/webapp/pages/landing/LandingPage.tsx | Removes legacy landing page. |
| src/webapp/pages/example/ExamplePage.tsx | Removes legacy example detail page. |
| src/webapp/pages/example/tests/ExamplePage.spec.tsx | Removes legacy ExamplePage test. |
| src/webapp/pages/example/tests/snapshots/ExamplePage.spec.tsx.snap | Removes legacy snapshot. |
| src/webapp/pages/app/Dhis2App.tsx | Simplifies bootstrap: resolves baseUrl, configures Provider (apiVersion 41), renders new App. |
| src/webapp/pages/app/App.tsx | Builds composition root from DataEngine, loads user + locale, wires MetadataExplorer route, shows build stamp. |
| src/webapp/hooks/useFuture.ts | Adds a cancellable hook for running Futures with React effects. |
| src/webapp/components/metadata/webgpu-polyfill.ts | Adds a global stub polyfill for GPUShaderStage to prevent 3D lazy chunk crashes. |
| src/webapp/components/metadata/MetadataTable.tsx | Adds results table with identicon avatars and row selection. |
| src/webapp/components/metadata/MetadataQueryBuilder.tsx | Adds query builder UI (type/fields/filters/paging controls). |
| src/webapp/components/metadata/MetadataGraphView.tsx | Adds 2D layout graph view rendering nodes/groups + SVG edges. |
| src/webapp/components/metadata/MetadataGraphPanel.tsx | Adds panel to fetch/merge graph data, lazy-load combos, and switch 2D/3D views with fallback. |
| src/webapp/components/metadata/IdenticonAvatar.tsx | Adds React renderer for identicons (async hash → SVG rects). |
| src/webapp/components/error-boundary/ErrorBoundary.tsx | Adds reusable error boundary with fallback renderer. |
| src/webapp/components/card-grid/MenuCard.tsx | Removes legacy menu card component. |
| src/webapp/components/card-grid/CardGrid.tsx | Removes legacy card grid component. |
| src/vite-env.d.ts | Declares build-time globals for commit/time. |
| src/utils/build-info.ts | Adds buildInfo + feedback description template using injected build metadata. |
| src/types/dhis2-app-runtime.ts | Adds DataEngine type alias derived from useDataEngine. |
| src/types/d2-api.ts | Removes legacy @eyeseetea/d2-api wrapper/types. |
| src/scripts/zip-build.ts | Adds Node script to zip the build output. |
| src/scripts/update-po.ts | Adds cross-platform msgmerge updater script for .po files. |
| src/scripts/example.ts | Removes legacy example script. |
| src/package.json | Adds a src/ package.json for tooling/linking. |
| src/domain/usecases/users/GetCurrentUserUseCase.ts | Updates FutureData import path. |
| src/domain/usecases/users/tests/GetCurrentUserUseCase.spec.ts | Adds a unit test for current-user use case via test composition root. |
| src/domain/usecases/system/GetUiLocaleUseCase.ts | Adds UI locale use case. |
| src/domain/usecases/metadata/ListMetadataUseCase.ts | Adds metadata list use case. |
| src/domain/usecases/metadata/ListCategoryOptionCombosUseCase.ts | Adds paginated category option combo listing use case. |
| src/domain/repositories/UserRepository.ts | Updates FutureData import path. |
| src/domain/repositories/SystemRepository.ts | Adds SystemRepository interface + UiLocaleSettings type. |
| src/domain/repositories/MetadataRepository.ts | Adds MetadataRepository interface. |
| src/domain/metadata/ResourceType.ts | Adds resource type union + labels + selectable list. |
| src/domain/metadata/pagination.ts | Adds MAX_PAGE_SIZE constant. |
| src/domain/metadata/MetadataQuery.ts | Adds metadata query type. |
| src/domain/metadata/MetadataItem.ts | Adds metadata item/list types. |
| src/domain/metadata/MetadataGraph.ts | Adds graph node/edge/group types and key builder. |
| src/domain/metadata/Identicon.ts | Adds identicon hashing + shape/svg building helpers. |
| src/domain/metadata/fields.ts | Adds helpers to parse/normalize DHIS2 fields strings. |
| src/domain/metadata/dataset-splits.ts | Adds helper to split data sets by categoryCombo override behavior. |
| src/domain/entities/generic/FutureData.ts | Adds FutureData alias in domain layer. |
| src/data/repositories/UserTestRepository.ts | Updates FutureData import path. |
| src/data/repositories/UserDhis2Repository.ts | Adds DataEngine-based me query repository with payload validation. |
| src/data/repositories/UserD2Repository.ts | Removes legacy d2-api-backed repository. |
| src/data/repositories/SystemTestRepository.ts | Adds test system repository returning default locale. |
| src/data/repositories/SystemDhis2Repository.ts | Adds DataEngine-based locale repository. |
| src/data/repositories/MetadataTestRepository.ts | Adds in-memory metadata repository for tests/use-cases. |
| src/data/repositories/MetadataDhis2Repository.ts | Adds DataEngine-based metadata list/get repository with payload guards. |
| src/data/dhis2-payload-guards.ts | Adds runtime validators for DHIS2 list/get envelopes and metadata item/pager shapes. |
| src/data/api-futures.ts | Replaces apiToFuture with promiseToFuture using AbortController cancellation. |
| src/CompositionRoot.ts | Wires new repositories + use cases (users/system/metadata) for webapp and tests. |
| src/app-config.ts | Updates app id/name and enriches feedback template with build metadata. |
| README.md | Updates quick start, scripts, and architecture description. |
| package.json | Updates package metadata, adds Yarn 4 engines/tooling, updates build scripts and dependencies. |
| openspec/specs/.gitkeep | Keeps specs directory structure tracked. |
| openspec/designs/wireframes/.gitkeep | Keeps design wireframes directory tracked. |
| openspec/designs/mockups/.gitkeep | Keeps design mockups directory tracked. |
| openspec/designs/exports/.gitkeep | Keeps design exports directory tracked. |
| openspec/config.yaml | Adds OpenSpec project context/rules and repo conventions. |
| openspec/changes/.gitkeep | Keeps changes directory tracked. |
| opencode.json | Adds agent configuration for OpenCode / multi-agent workflow. |
| i18n/es.po | Updates POT date and adds new strings (some untranslated). |
| i18n/en.pot | Updates extracted translation template for new UI strings. |
| .yarnrc.yml | Configures Yarn 4 with node-modules linker + pinned yarnPath. |
| .travis.yml | Updates CI to Node 22 + corepack + Yarn immutable install. |
| .nvmrc | Updates Node version to v22. |
| .husky/pre-push | Extends pre-push hook with optional Snyk checks. |
| .gitignore | Updates ignores (env, Yarn 4 artifacts, SBOM, Claude local settings). |
| .github/workflows/main.yml | Adds dependency-track workflows for PRs (Syft + Yarn4). |
| .github/workflows/main_syft.yml | Adds runner smoke + Syft SBOM workflow with dependency-track gating. |
| .github/workflows/main_CycloneDX_berry.yml | Adds runner smoke + CycloneDX (Yarn Berry) workflow with dependency-track gating. |
| .env.example | Renames auth var to DHIS2_AUTH. |
| .claude/skills/task-management/SKILL.md | Adds task-management workflow guidance. |
| .claude/skills/playwright-cli/SKILL.md | Adds playwright-cli usage documentation. |
| .claude/skills/pencil-design/SKILL.md | Adds Pencil design workflow documentation. |
| .claude/skills/openspec-propose/SKILL.md | Adds OpenSpec propose workflow documentation. |
| .claude/skills/openspec-explore/SKILL.md | Adds OpenSpec explore workflow documentation. |
| .claude/skills/openspec-archive-change/SKILL.md | Adds OpenSpec archive workflow documentation. |
| .claude/skills/openspec-apply-change/SKILL.md | Adds OpenSpec apply workflow documentation. |
| .claude/settings.local.json.template | Adds template for per-user Claude tool permissions. |
| .claude/settings.json | Adds Claude pre-tool hook for git commit architecture review. |
| .claude/commands/opsx/propose.md | Adds /opsx:propose command documentation. |
| .claude/commands/opsx/explore.md | Adds /opsx:explore command documentation. |
| .claude/commands/opsx/archive.md | Adds /opsx:archive command documentation. |
| .claude/commands/opsx/apply.md | Adds /opsx:apply command documentation. |
| .claude/CLAUDE.md | Adds project-wide conventions (Clean Architecture, i18n, testing, UI design gate, etc.). |
| .claude/agents/project-manager.md | Adds project-manager agent role definition. |
| .claude/agents/graphical-designer.md | Adds graphical-designer agent role definition. |
| .claude/agents/frontend-developer.md | Adds frontend-developer agent role definition. |
| .claude/agents/dhis2-integration-developer.md | Adds DHIS2-integration developer agent role definition. |
| .claude/agents/code-reviewer.md | Adds code-reviewer agent role definition. |
| .agents/agents/project-manager.md | Mirrors agent role definition for OpenCode agent config. |
| .agents/agents/graphical-designer.md | Mirrors agent role definition for OpenCode agent config. |
| .agents/agents/frontend-developer.md | Mirrors agent role definition for OpenCode agent config. |
| .agents/agents/dhis2-integration-developer.md | Mirrors agent role definition for OpenCode agent config. |
| .agents/agents/code-reviewer.md | Mirrors agent role definition for OpenCode agent config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <section className="metadata-query"> | ||
| <div className="metadata-query__row"> | ||
| <label className="metadata-query__label"> | ||
| Resource type | ||
| <select |
There was a problem hiding this comment.
User-facing labels/button text and placeholders are hard-coded (e.g. “Resource type”, “Fields”, “Fetch”). Project convention requires all UI strings to go through i18n.t(...) so they are extracted to .po/.pot files.
| if (!items.length) { | ||
| return <div className="metadata-table__empty">No results</div>; | ||
| } | ||
|
|
||
| return ( | ||
| <table className="metadata-table"> | ||
| <thead> | ||
| <tr> | ||
| <th className="metadata-table__cell metadata-table__cell--avatar">Avatar</th> | ||
| {columns.map(column => ( |
There was a problem hiding this comment.
This component renders hard-coded user-facing strings (“No results”, “Avatar”) without using i18n.t(...), so they won’t be included in localization extraction.
| <tr | ||
| key={item.id} | ||
| className={ | ||
| isSelected | ||
| ? "metadata-table__row metadata-table__row--active" | ||
| : "metadata-table__row" | ||
| } | ||
| onClick={() => onSelect(item)} | ||
| > |
There was a problem hiding this comment.
Clickable table rows (<tr onClick=...>) are not keyboard-accessible by default and don’t expose an interactive role. Consider using a focusable control inside the row (e.g. a <button> in the first cell) or adding tabIndex=0, appropriate role, and onKeyDown handling for Enter/Space.
| <button | ||
| type="button" | ||
| className="graph-node__action" | ||
| title="Open API" | ||
| onClick={() => onOpenApi?.(node)} | ||
| > | ||
| <OpenInNewIcon fontSize="small" /> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| className="graph-node__action" | ||
| title="Focus in graph" | ||
| onClick={() => onFocus?.(node)} | ||
| > | ||
| <CenterFocusStrongIcon fontSize="small" /> | ||
| </button> |
There was a problem hiding this comment.
Icon-only action buttons rely on title and hard-coded English strings (“Open API”, “Focus in graph”). For localization + accessibility, route these labels through i18n.t(...) and ensure each button has a stable accessible name (e.g. aria-label).
| const newEdges: GraphEdge[] = newNodes.map(node => ({ | ||
| from: comboKey, | ||
| to: node.key, | ||
| label: "categoryOptionCombos", | ||
| })); | ||
|
|
||
| const nodesByKey = new Map<string, GraphNode>(graph.nodes.map(node => [node.key, node])); | ||
| newNodes.forEach(node => nodesByKey.set(node.key, node)); | ||
|
|
||
| const groupId = "category-option-combos"; | ||
| const filteredGroups = graph.groups.filter(group => group.id !== groupId); | ||
| const group: GraphGroup = { | ||
| id: groupId, | ||
| title: "Category option combos", | ||
| nodeKeys: newNodes.map(node => node.key), | ||
| direction: "child", | ||
| }; |
There was a problem hiding this comment.
mergeCategoryOptionCombos introduces user-visible labels as raw strings (title: "Category option combos", edge label: "categoryOptionCombos"). These should be localized via i18n.t(...) (or mapped to existing translated labels) to keep the graph UI translatable.
| ### Clean architecture folder structure | ||
|
|
||
| - `src/domain`: Domain layer of the app (entities, use cases, repository definitions) | ||
| - `src/data`: Data of the app (repository implementations) | ||
| - `src/webapp/pages`: Main React components. | ||
| - `src/domain`: Domain layer (entities, resource types, repository definitions) | ||
| - `src/application`: Application layer (use cases) | ||
| - `src/data`: Infrastructure layer (DHIS2 data engine repositories) | ||
| - `src/webapp/pages`: Main React pages. |
There was a problem hiding this comment.
The “Clean architecture folder structure” section states there is a src/application layer, but the repo conventions (and actual code) place use cases under src/domain/usecases/ and explicitly avoid a top-level src/application/ folder. Please update this section to reflect the real structure.
| What it is: | ||
| - A DHIS2 web application that visualizes metadata dependencies and relationships | ||
| (data elements, datasets, organisation units, indicators, programs, etc.) | ||
| as interactive 3D graphs to help users explore and audit metadata. | ||
| - Runs inside a DHIS2 instance as a bundled web app (manifest.webapp), consuming | ||
| the DHIS2 Web API via @eyeseetea/d2-api. No custom backend — DHIS2 is the backend. | ||
|
|
||
| Tech stack: | ||
| - TypeScript 5.7 (strict, ESNext target, path alias `$/*` → `src/`) | ||
| - React 18 + React Router DOM 5 | ||
| - Vite 4 (build), Yarn 4 (package manager), Node 22 | ||
| - Vitest + @testing-library/react + jsdom (unit tests) | ||
| - Playwright (under src/tests/playwright, excluded from vitest) | ||
| - @dhis2/app-runtime, @dhis2/ui, @dhis2/d2-i18n | ||
| - @eyeseetea/d2-api (DHIS2 Web API client), @eyeseetea/d2-ui-components | ||
| - Material-UI v4 + styled-components + styled-jsx | ||
| - react-force-graph-3d + three.js (3D graph visualization) |
There was a problem hiding this comment.
This OpenSpec context says the app consumes the DHIS2 Web API via @eyeseetea/d2-api, but the current implementation has migrated to @dhis2/app-runtime’s DataEngine (and removed the @eyeseetea/d2-api dependency). Please update the tech stack/context to avoid misleading future changes.
| echo "snyk test failed; attempting authentication" | ||
| snyk auth || echo "snyk auth failed (non-blocking)" | ||
| snyk test || echo "snyk test failed (non-blocking)" | ||
| fi | ||
| echo "Tip: you can generate an HTML report with: snyk test --all-projects --dev --json-file-output=snyk.json || true && snyk-to-html -i snyk.json -o snyk-report.html -s (install snyk-to-html)" | ||
| else | ||
| echo "snyk not found. Install it to run snyk auth/test." |
There was a problem hiding this comment.
Running snyk auth from a git hook can be interactive and may block git push unexpectedly. Consider removing the auto-auth attempt from the hook (or gating behind an explicit env flag) and keep the hook to non-interactive checks only.
| echo "snyk test failed; attempting authentication" | |
| snyk auth || echo "snyk auth failed (non-blocking)" | |
| snyk test || echo "snyk test failed (non-blocking)" | |
| fi | |
| echo "Tip: you can generate an HTML report with: snyk test --all-projects --dev --json-file-output=snyk.json || true && snyk-to-html -i snyk.json -o snyk-report.html -s (install snyk-to-html)" | |
| else | |
| echo "snyk not found. Install it to run snyk auth/test." | |
| echo "snyk test failed (non-blocking)." | |
| echo "If authentication is required, run 'snyk auth' manually outside the git hook and re-run the check." | |
| fi | |
| echo "Tip: you can generate an HTML report with: snyk test --all-projects --dev --json-file-output=snyk.json || true && snyk-to-html -i snyk.json -o snyk-report.html -s (install snyk-to-html)" | |
| else | |
| echo "snyk not found. Install it to run snyk test." |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 95 out of 106 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function getProxy(env: Record<string, string>) { | ||
| const dhis2UrlVar = "VITE_DHIS2_BASE_URL"; | ||
| const dhis2AuthVar = "VITE_DHIS2_AUTH"; | ||
| const dhis2AuthVar = "DHIS2_AUTH"; | ||
| const targetUrl = env[dhis2UrlVar]; | ||
| const auth = env[dhis2AuthVar]; |
There was a problem hiding this comment.
env is built from process.env + loadEnv(...), so its values are string | undefined. The getProxy/resolveBuildTime signatures require Record<string, string>, which can cause TypeScript errors and also hides the need to handle missing values. Consider widening the parameter types (e.g. Record<string, string | undefined>) and keeping the runtime checks for required keys.
| const commit = (__APP_BUILD_COMMIT__ || "unknown").trim(); | ||
| const builtAt = (__APP_BUILD_TIME__ || "").trim(); |
There was a problem hiding this comment.
__APP_BUILD_COMMIT__ / __APP_BUILD_TIME__ are compile-time define replacements; if this module is ever evaluated in a non-Vite context (e.g. some test/tooling paths), referencing an undeclared global can throw a ReferenceError. Guard with typeof __APP_BUILD_COMMIT__ !== "undefined" (and same for time) before reading, or route the values through something that is always defined at runtime.
| <label className="metadata-query__label"> | ||
| Resource type | ||
| <select | ||
| className="metadata-query__input" |
There was a problem hiding this comment.
Several user-facing strings (e.g. the field labels like "Resource type" / "Fields") are hardcoded instead of going through i18n.t(...). This will prevent them from being extracted into i18n/*.po and breaks the repo’s i18n convention; please wrap these labels/placeholders in translations.
| export function splitTopLevelFields(value: string): string[] { | ||
| type State = { tokens: string[]; current: string; depth: number }; | ||
|
|
||
| const finalState = Array.from(value).reduce<State>( | ||
| (state, char) => { | ||
| if (char === "[") { | ||
| return { ...state, current: state.current + char, depth: state.depth + 1 }; | ||
| } | ||
| if (char === "]") { | ||
| return { | ||
| ...state, | ||
| current: state.current + char, | ||
| depth: Math.max(0, state.depth - 1), | ||
| }; | ||
| } | ||
| if (char === "," && state.depth === 0) { | ||
| const trimmed = state.current.trim(); | ||
| return { | ||
| ...state, | ||
| tokens: trimmed ? [...state.tokens, trimmed] : state.tokens, | ||
| current: "", | ||
| }; | ||
| } | ||
| return { ...state, current: state.current + char }; | ||
| }, | ||
| { tokens: [], current: "", depth: 0 } | ||
| ); | ||
|
|
||
| const lastToken = finalState.current.trim(); | ||
| return lastToken ? [...finalState.tokens, lastToken] : finalState.tokens; | ||
| } |
There was a problem hiding this comment.
splitTopLevelFields / getTopLevelFieldName implement non-trivial parsing rules (nested brackets, trimming, empty tokens). Given the repo already has unit tests for other domain utilities, this file should have focused tests covering nested selectors and edge cases to prevent regressions.
| <label className="metadata-query__checkbox"> | ||
| <input | ||
| type="checkbox" | ||
| checked={!value.paging} | ||
| onChange={event => | ||
| onChange({ ...value, paging: !event.target.checked, page: 1 }) | ||
| } | ||
| /> | ||
| Fetch all (paging=false) | ||
| </label> | ||
|
|
||
| <button className="metadata-query__button" type="button" onClick={onRun}> | ||
| Fetch | ||
| </button> |
There was a problem hiding this comment.
User-facing strings in this block (e.g. "Fetch all (paging=false)" and the "Fetch" button label) are hardcoded instead of using i18n.t(...). Please localize them so they get extracted into the .po files and remain consistent with the rest of the app.
Add yarn resolutions that force patched versions of transitive dependencies flagged by Dependency-Track: - postcss 8.4.29 -> 8.5.9 (closes CVE-2023-44270, GHSA-7fh5-64p2-3v2j) - brace-expansion 1.1.11 -> 1.1.14 and 2.0.1 -> 2.1.0 via per-parent pins on minimatch@3.1.5 and minimatch@5.1.9 (closes CVE-2025-5889, CVE-2026-33750, CVE-2026-25547, GHSA-f886-m6hf-6m8v, GHSA-v6h2-p8h4-qcjw) Verified locally: lint, tsc, 108 tests, yarn build, and a fresh SBOM generated with @cyclonedx/yarn-plugin-cyclonedx all green.
| if: github.event_name == 'pull_request' | ||
| uses: EyeSeeTea/github-workflows/.github/workflows/dependency-track-yarn4.yml@feature/add_bom_actions | ||
| secrets: inherit No newline at end of file |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 93 out of 104 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "name": "Metadata-visualizer", | ||
| "description": "DHIS2 metadata visualization app", |
There was a problem hiding this comment.
package.json has a capitalized package name ("Metadata-visualizer"). npm/Yarn package names must be lowercase; uppercase characters can cause install/publish validation errors. Rename it to a lowercase name (e.g. "metadata-visualizer") to avoid tooling failures.
| if (controller.signal.aborted || isAbortError(err)) { | ||
| throw Future.cancel(); | ||
| } |
There was a problem hiding this comment.
In promiseToFuture, abort handling uses throw Future.cancel() inside the promise .catch. That does not call the reject provided by Future.fromComputation, so the outer Future can remain pending (and the thrown cancellation can become an unhandled rejection). Prefer explicitly rejecting the computation with a Cancellation/abort result when the signal is aborted.
| export function assertMetadataItem(value: unknown, context: string): MetadataItem { | ||
| if (!isRecord(value) || !hasStringProp(value, "id")) { | ||
| throw new Dhis2PayloadError( | ||
| `${context}: expected metadata item with string "id", got ${describe(value)}` | ||
| ); |
There was a problem hiding this comment.
assertMetadataItem is declared to return MetadataItem, but it only validates id and the comment says the caller will add type. Since MetadataItem requires type, this signature is unsound and can mask missing type bugs. Consider returning a narrower type and only constructing MetadataItem after stamping type.
| if (!items.length) { | ||
| return <div className="metadata-table__empty">No results</div>; | ||
| } |
There was a problem hiding this comment.
User-facing text "No results" is currently hardcoded. Wrap it in i18n.t(...) so it’s translatable and included in gettext extraction.
| <tr> | ||
| <th className="metadata-table__cell metadata-table__cell--avatar">Avatar</th> | ||
| {columns.map(column => ( |
There was a problem hiding this comment.
The table header label "Avatar" is hardcoded user-facing text. Wrap it in i18n.t(...) to keep translations complete.
| server: { | ||
| port: parseInt(env.VITE_PORT), | ||
| host: "127.0.0.1", | ||
| proxy: proxy, |
There was a problem hiding this comment.
server.port: parseInt(env.VITE_PORT) can produce NaN if VITE_PORT is missing/empty, which will break vite startup in a non-obvious way. Consider providing a default (e.g. 8081) and/or validating the env var before parsing.
| type="button" | ||
| className="graph-node__action" | ||
| title="Open API" | ||
| onClick={() => onOpenApi?.(node)} | ||
| > |
There was a problem hiding this comment.
The button title "Open API" is user-facing text but isn’t translated. Wrap it in i18n.t(...) (and consider adding an aria-label for consistent screen-reader output).
| const filteredGroups = graph.groups.filter(group => group.id !== groupId); | ||
| const group: GraphGroup = { | ||
| id: groupId, | ||
| title: "Category option combos", | ||
| nodeKeys: newNodes.map(node => node.key), |
There was a problem hiding this comment.
mergeCategoryOptionCombos hardcodes a group title ("Category option combos") that is rendered in the UI. Wrap it in i18n.t(...) (or otherwise localize it) to keep translations complete.
Upgrade the Vite toolchain to close vite/esbuild/vitest CVEs and remove the node-stdlib-browser polyfill chain entirely, since the bundle does not actually reference Node core modules. - vite 4.5 -> 7.3 (closes CVE-2025-58751/58752/46565/32395/62522, CVE-2026-39365, GHSA-4w7w-66w2-5vf9, GHSA-93m4-6634-74q7, GHSA-859w-5945-r5v3, GHSA-g4jq-h2w9-997c, GHSA-jqfw-vq24-v9c3) - @vitejs/plugin-react 3 -> 4.7 - vite-plugin-checker 0.6 -> 0.12 - vitest 0.32 -> 3.2 (closes CVE-2025-24964) - esbuild 0.18 -> 0.27 via vite (closes GHSA-67mh-4wv8-2f99) - rollup 3.29 -> 4.60 via vite Drop vite-plugin-node-stdlib-browser and node-stdlib-browser: build and tests pass without any Node polyfill, which eliminates the whole crypto-browserify / sha.js / pbkdf2 / bn.js / elliptic transitive chain (closes CVE-2025-14505, GHSA-848j-6mx2-7j84 and others with no upstream fix). Drops 89 components from the SBOM. Verified: yarn install, yarn lint, yarn tsc --noEmit, yarn test (108/108), yarn build all green on Node 22.
- jsdom 21 -> 29.0.2: removes the http-proxy-agent -> @tootallnate/once@2 chain that no newer version of jsdom ships (closes CVE-2026-3449, GHSA-vpq2-c234-7xj6). - rimraf/glob 11.1.0 -> 13.0.6: Dependency-Track was flagging glob@11.1.0 under CVE-2025-64756 even though the CVE itself lists 11.1.0 as the patched version; bumping to 13.x avoids the faulty DT match. - node-gettext 2.1.0 -> 3.0.1: closes CVE-2024-21528 for the 2.x line. GHSA-g974-hxvm-x689 is flagged against 'all versions' with no upstream fix; remains a false positive because translation files are loaded from our own repo at build time, not user input. Remaining alerts after this commit are all non-exploitable in our context (eslint@8 RuleTester API we do not use, node-gettext prototype pollution on trusted build-time input, and a DT name-match false positive on plain brace-expansion vs the @isaacs scoped fork).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 93 out of 104 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - Runs inside a DHIS2 instance as a bundled web app (manifest.webapp), consuming | ||
| the DHIS2 Web API via @eyeseetea/d2-api. No custom backend — DHIS2 is the backend. | ||
|
|
There was a problem hiding this comment.
This context section says the app consumes the DHIS2 Web API via @eyeseetea/d2-api, but in this PR the data layer uses @dhis2/app-runtime’s DataEngine repositories instead. Please update the description to reflect the current integration approach.
| type="button" | ||
| className="graph-node__action" | ||
| title="Focus in graph" | ||
| onClick={() => onFocus?.(node)} | ||
| > |
There was a problem hiding this comment.
Button title text "Focus in graph" is user-facing but hardcoded. Wrap it with i18n.t(...) so it can be translated.
| const group: GraphGroup = { | ||
| id: groupId, | ||
| title: "Category option combos", | ||
| nodeKeys: newNodes.map(node => node.key), | ||
| direction: "child", |
There was a problem hiding this comment.
The group title "Category option combos" is displayed in the UI (via MetadataGraphView) but is hardcoded here. Consider using i18n.t(...) and/or resourceTypeLabels.categoryOptionCombos so it’s translated consistently.
| - TypeScript 5.7 (strict, ESNext target, path alias `$/*` → `src/`) | ||
| - React 18 + React Router DOM 5 | ||
| - Vite 4 (build), Yarn 4 (package manager), Node 22 |
There was a problem hiding this comment.
This file lists "Vite 4" in the tech stack, but the root package.json in this PR bumps Vite to a 7.x range. Please align the documented Vite version with the actual dependency to avoid confusion.
| import type { useDataEngine } from "@dhis2/app-runtime"; | ||
|
|
||
| export type DataEngine = ReturnType<typeof useDataEngine>; |
There was a problem hiding this comment.
import type { useDataEngine } cannot be referenced in a typeof type query. ReturnType<typeof useDataEngine> will fail to typecheck because useDataEngine was imported type-only. Import it as a value (regular import) or, if available in your @dhis2/app-runtime version, prefer importing the DataEngine type directly and exporting that.
| msgid "Click to focus - Right click opens the API" | ||
| msgstr "Click para enfocar - Boton derecho abre la API" |
There was a problem hiding this comment.
Spanish translation has a spelling/grammar issue: "Boton derecho" should be "Botón derecho" (accent) and typically "botón" is lowercase mid-sentence. Please correct this so the localized UI text is polished.
| type="button" | ||
| className="graph-node__action" | ||
| title="Open API" | ||
| onClick={() => onOpenApi?.(node)} | ||
| > |
There was a problem hiding this comment.
Button title text "Open API" is user-facing but hardcoded. Wrap it with i18n.t(...) so it can be translated.
| </label> | ||
|
|
||
| <button className="metadata-query__button" type="button" onClick={onRun}> | ||
| Fetch | ||
| </button> |
There was a problem hiding this comment.
Hardcoded UI strings in this section (e.g. "Fetch all (paging=false)" / "Fetch") should go through i18n.t(...) so they’re extracted and translated consistently.
| nodeId="id" | ||
| linkSource="source" | ||
| linkTarget="target" | ||
| nodeLabel={node => `${resourceTypeLabels[node.type] ?? node.type}: ${node.name}`} | ||
| linkLabel={link => link.label} |
There was a problem hiding this comment.
nodeLabel builds a tooltip string with a hardcoded ":" separator and non-translated template. Since this label is user-facing, consider localizing it via i18n.t(...) (e.g. using placeholders for type/name) to keep consistent translation behavior.
|
Hi @MatiasArriola Thank you for your review: I asked to claude to do a summary for all the changes:
Should Fix
Recommendations
Minor
Beyond the review A few things that were not on the list but landed in the same set of changes, in case it helps the reviewer's mental model:
All of the above is green locally against Node 22: yarn lint, yarn tsc --noEmit, yarn test (111 tests), yarn build. |
MatiasArriola
left a comment
MatiasArriola
left a comment
There was a problem hiding this comment.
Thanks @idelcano !! I see a lot of improvements here, and the items from the previous review were addressed.
📌 References
Basic app with basic errors fixed