Conversation
…h the center node
…isolation, payload validation)
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.
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).
Integrates the Vite 4→7 / Vitest upgrade, use-case migration to
domain/usecases, and combos lazy-load UI cleanup from chore/migrate-legacy
on top of the JSON Package Explorer tab added on this branch.
Conflict resolutions:
- vite.config.ts: drop nodePolyfills (Vite 7 upgrade); keep injectAppTitlePlugin.
- IdenticonAvatar: adopt JSX SVG rendering (no dangerouslySetInnerHTML);
keep type: string prop to avoid cascading into GraphNode.type.
- MetadataGraphPanel: adopt cleaned-up lazy combos UI (showLazyCombos gate,
simpler button); drop unused cocTotal / lazyButtonLabel.
- MetadataQueryBuilder: keep both MAX_PAGE_SIZE and i18n imports.
- MetadataExplorerPage: combine new JSON Package tab with the useFuture
refactor and MAX_PAGE_SIZE / ensureFields helpers.
- JsonPackageExplorer: use default import for MetadataGraphView3D (now
default-exported for React.lazy).
- i18n/en.pot, i18n/es.po: regenerated via yarn localize.
BundleMonNo change in files bundle size Groups updated (1)
Final result: ✅ View report in BundleMon website ➡️ |
There was a problem hiding this comment.
Pull request overview
This PR turns the skeleton app into a “Metadata Visualizer” by introducing new metadata exploration pages (instance metadata + JSON package dependency explorer), migrating DHIS2 API access to @dhis2/app-runtime’s DataEngine repositories wired via CompositionRoot, and updating build/tooling (Yarn 4, build stamping, packaging scripts, and CI workflow).
Changes:
- Add a new Metadata Explorer UI (query builder, results table, dependency graph) plus a JSON package explorer with graph visualizations.
- Replace legacy
@eyeseetea/d2-apiusage withDataEngine-based repositories and new domain/usecase/repository abstractions. - Update build/dev tooling: Yarn 4, zip build script, translation update script, Vite index title injection, and build commit/time stamping.
Reviewed changes
Copilot reviewed 98 out of 109 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Inject app title/build stamps at build-time; update proxy/auth env handling and dev server settings. |
| tsconfig.json | Add global TS types for Node/testing/vitest. |
| src/webapp/utils/i18n-setup.ts | Centralize i18n language + RTL/LTR direction setup. |
| src/webapp/pages/metadata/MetadataExplorerPage.tsx | New metadata explorer page (query builder, table, graph, tabs). |
| src/webapp/pages/metadata/JsonPackageExplorer.tsx | New JSON package browser with filtering + 2D/3D dependency graph view. |
| src/webapp/pages/landing/LandingPage.tsx | Remove old landing page. |
| src/webapp/pages/example/tests/snapshots/ExamplePage.spec.tsx.snap | Remove old example snapshot. |
| src/webapp/pages/example/tests/ExamplePage.spec.tsx | Remove old example test. |
| src/webapp/pages/example/ExamplePage.tsx | Remove old example page. |
| src/webapp/pages/app/Dhis2App.tsx | Simplify bootstrapping to baseUrl resolution + Provider setup; bump apiVersion. |
| src/webapp/pages/app/App.tsx | Build composition root from useDataEngine; wire i18n + new router/page; add build stamp. |
| src/webapp/pages/Router.tsx | Remove old router. |
| src/webapp/hooks/useFuture.ts | New hook to run/cancel Future requests tied to React deps. |
| src/webapp/components/metadata/webgpu-polyfill.ts | Polyfill/stub GPUShaderStage to prevent 3D chunk crashes in some browsers. |
| src/webapp/components/metadata/MetadataTable.tsx | New table component for metadata list rendering. |
| src/webapp/components/metadata/MetadataQueryBuilder.tsx | New query builder UI for type/fields/filters/paging. |
| src/webapp/components/metadata/MetadataGraphView.tsx | New 2D graph layout view with draggable canvas and node cards. |
| src/webapp/components/metadata/IdenticonAvatar.tsx | New identicon avatar renderer for metadata items. |
| src/webapp/components/error-boundary/ErrorBoundary.tsx | New reusable error boundary component. |
| src/webapp/components/card-grid/MenuCard.tsx | Remove old card-grid menu card. |
| src/webapp/components/card-grid/CardGrid.tsx | Remove old card grid. |
| src/vite-env.d.ts | Declare build-time globals injected by Vite define. |
| src/utils/build-info.ts | Centralize build commit/time info + feedback template. |
| src/types/dhis2-app-runtime.ts | Add DataEngine type alias (from useDataEngine). |
| src/types/d2-api.ts | Remove legacy @eyeseetea/d2-api typed wrapper. |
| src/scripts/zip-build.ts | New build output zipping script (Node-based). |
| src/scripts/update-po.ts | New cross-platform .po update helper using msgmerge when available. |
| src/scripts/example.ts | Remove old example script. |
| src/package.json | Add package.json under src/ (for linked local package usage). |
| src/domain/usecases/users/tests/GetCurrentUserUseCase.spec.ts | Add test for current-user use case through test composition root. |
| src/domain/usecases/users/GetCurrentUserUseCase.ts | Update FutureData import path. |
| src/domain/usecases/system/GetUiLocaleUseCase.ts | Add use case for retrieving UI locale settings. |
| src/domain/usecases/metadata/tests/BuildMetadataGraphUseCase.spec.ts | Add tests for metadata dependency graph construction. |
| src/domain/usecases/metadata/ListMetadataUseCase.ts | Add use case to list metadata by query. |
| src/domain/usecases/metadata/ListCategoryOptionCombosUseCase.ts | Add use case for listing category option combos by category combo. |
| src/domain/repositories/UserRepository.ts | Update FutureData import path. |
| src/domain/repositories/SystemRepository.ts | Add repository contract for locale settings. |
| src/domain/repositories/MetadataRepository.ts | Add repository contract for metadata list/get. |
| src/domain/metadata/pagination.ts | Introduce MAX_PAGE_SIZE for bounded paging. |
| src/domain/metadata/fields.ts | Add pure helpers for parsing/normalizing DHIS2 fields strings. |
| src/domain/metadata/dataset-splits.ts | Add helper to split datasets by category combo override behavior. |
| src/domain/metadata/ResourceType.ts | Define resource types/labels + type normalization helper. |
| src/domain/metadata/MetadataQuery.ts | Add domain query type for metadata listing. |
| src/domain/metadata/MetadataItem.ts | Add domain item/list types (with pager). |
| src/domain/metadata/MetadataGraph.ts | Add domain graph types + node-key helper. |
| src/domain/metadata/Identicon.ts | Add identicon hashing/shape/svg helpers for consistent avatars/textures. |
| src/domain/entities/generic/FutureData.ts | Introduce FutureData alias for Future<Error, D>. |
| src/data/repositories/UserTestRepository.ts | Update FutureData import path. |
| src/data/repositories/UserDhis2Repository.ts | Implement UserRepository via DataEngine + payload guards. |
| src/data/repositories/UserD2Repository.ts | Remove legacy D2 API-based repository. |
| src/data/repositories/SystemTestRepository.ts | Add system repo fake for tests. |
| src/data/repositories/SystemDhis2Repository.ts | Implement system locale retrieval via DataEngine. |
| src/data/repositories/MetadataTestRepository.ts | Add in-memory metadata repo fake with basic filter + paging emulation. |
| src/data/repositories/MetadataDhis2Repository.ts | Implement metadata list/get via DataEngine + payload guards. |
| src/data/dhis2-payload-guards.ts | Add runtime validation helpers for DHIS2 payloads at the data boundary. |
| src/data/api-futures.ts | Replace apiToFuture with promiseToFuture (AbortSignal-based cancellation). |
| src/app-config.ts | Update app id/name and enrich feedback with build info. |
| src/CompositionRoot.ts | Expand DI to metadata/system repos + use cases; switch to DataEngine-backed repos. |
| package.json | Rename app, migrate deps/tooling (Yarn 4, Vite upgrades, new libs for 3D graphs, scripts). |
| openspec/specs/.gitkeep | Keep specs directory tracked. |
| openspec/designs/wireframes/.gitkeep | Keep wireframes directory tracked. |
| openspec/designs/mockups/.gitkeep | Keep mockups directory tracked. |
| openspec/designs/exports/.gitkeep | Keep exports directory tracked. |
| openspec/config.yaml | Add OpenSpec project context/rules for contributors and automation. |
| openspec/changes/.gitkeep | Keep changes directory tracked. |
| opencode.json | Add agent configuration for automated workflows/review tooling. |
| index.html | Use %APP_TITLE% placeholder for runtime title injection. |
| i18n/es.po | Update POT creation timestamp + add new msgids for new UI strings. |
| i18n/en.pot | Regenerate template with new msgids for new UI strings. |
| README.md | Update setup/dev/build instructions for new tooling and scripts. |
| .yarnrc.yml | Configure Yarn 4 (node-modules linker + yarnPath). |
| .travis.yml | Update CI node version and Yarn install flags for Yarn 4. |
| .nvmrc | Bump Node version used by nvm. |
| .husky/pre-push | Add optional Snyk checks after existing gates. |
| .gitignore | Ignore .env, Yarn 4 folders, and local AI/Claude settings. |
| .github/workflows/main.yml | Add dependency-track workflow for Yarn 4 on PRs. |
| .env.example | Rename auth env var to DHIS2_AUTH. |
| .claude/skills/task-management/SKILL.md | Add internal workflow guidance for task management. |
| .claude/skills/playwright-cli/SKILL.md | Add internal Playwright CLI guidance. |
| .claude/skills/pencil-design/SKILL.md | Add internal Pencil design workflow guidance. |
| .claude/skills/openspec-propose/SKILL.md | Add internal OpenSpec propose workflow guidance. |
| .claude/skills/openspec-explore/SKILL.md | Add internal OpenSpec explore workflow guidance. |
| .claude/skills/openspec-archive-change/SKILL.md | Add internal OpenSpec archive workflow guidance. |
| .claude/skills/openspec-apply-change/SKILL.md | Add internal OpenSpec apply workflow guidance. |
| .claude/settings.local.json.template | Add template for local AI tool permissions config. |
| .claude/settings.json | Add hooks and automated pre-commit architecture review configuration. |
| .claude/commands/opsx/propose.md | Add command docs for propose workflow. |
| .claude/commands/opsx/explore.md | Add command docs for explore workflow. |
| .claude/commands/opsx/archive.md | Add command docs for archive workflow. |
| .claude/commands/opsx/apply.md | Add command docs for apply workflow. |
| .claude/agents/project-manager.md | Add agent role definition for PM workflows. |
| .claude/agents/graphical-designer.md | Add agent role definition for design workflows. |
| .claude/agents/frontend-developer.md | Add agent role definition for FE workflows. |
| .claude/agents/dhis2-integration-developer.md | Add agent role definition for DI workflows. |
| .claude/agents/code-reviewer.md | Add agent role definition for code review workflows. |
| .claude/CLAUDE.md | Add repo-wide conventions (Clean Architecture, tests, i18n, UI design gate, etc.). |
| .agents/agents/project-manager.md | Mirror agent role definition (agents directory). |
| .agents/agents/graphical-designer.md | Mirror agent role definition (agents directory). |
| .agents/agents/frontend-developer.md | Mirror agent role definition (agents directory). |
| .agents/agents/dhis2-integration-developer.md | Mirror agent role definition (agents directory). |
| .agents/agents/code-reviewer.md | Mirror agent role definition (agents directory). |
💡 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 OpenSpec context claims the app consumes the DHIS2 Web API via @eyeseetea/d2-api, but the PR migrates repositories to @dhis2/app-runtime’s DataEngine (e.g., UserDhis2Repository, MetadataDhis2Repository). Please update the OpenSpec description to match the actual integration approach so it doesn’t mislead future work.
| 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) |
There was a problem hiding this comment.
This OpenSpec tech-stack section says the project uses Vite 4, but package.json in this PR upgrades to Vite ^7.1.14. Please update the OpenSpec stack/version notes to match the repo so the generated guidance stays accurate.
| "name": "Metadata-visualizer", | ||
| "description": "DHIS2 metadata visualization app", |
There was a problem hiding this comment.
Package names published to npm must be lowercase. Metadata-visualizer contains uppercase characters and may break tooling that assumes npm naming rules (and your build/zip scripts use npm_package_name). Consider renaming to a lowercase name (e.g. metadata-visualizer).
| <div className="metadata-tabs" role="tablist" aria-label={i18n.t("Metadata views")}> | ||
| <button | ||
| type="button" | ||
| role="tab" | ||
| aria-selected={activeTab === "instance"} | ||
| className={ |
There was a problem hiding this comment.
The tabs are marked up with role="tablist"/role="tab", but the selected panel isn’t consistently exposed as a role="tabpanel" and the tabs aren’t linked to panels via aria-controls/id. This makes the tab UI hard to navigate with assistive tech. Consider adding proper tabpanel markup for both views and wire up id/aria-controls (and ideally roving tabIndex + arrow-key handling).
| if (!words.length) return type; | ||
|
|
||
| const [first, ...rest] = words; | ||
| return `${first}${rest.map(capitalizeWord).join("")}`; | ||
| } |
There was a problem hiding this comment.
getMetadataTypeLabel is used for user-visible labels (select options, table cells, node labels), but it currently normalizes input into lower camelCase (e.g. "data elements" → "dataElements") rather than a human-readable label. Consider either returning a title-cased label ("Data elements") or renaming the function to reflect that it normalizes keys, and using resourceTypeLabels (plus a fallback formatter) for display.
|
|
||
| dependency-track-yarn4: | ||
| 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.
The PR description says it only tests GitHub workflow actions, but this workflow change is part of a much larger feature/refactor (new metadata explorer UI, new repositories, build info stamping, Yarn 4 migration, etc.). Please update the PR title/description to reflect the real scope so reviewers/testers understand what’s being shipped.
| }, | ||
| server: { | ||
| port: parseInt(env.VITE_PORT), | ||
| host: "127.0.0.1", |
There was a problem hiding this comment.
server.host is hard-coded to 127.0.0.1, which prevents accessing the dev server from Docker/VMs or other devices on the LAN. If this wasn’t intentional, consider omitting host (Vite default) or making it configurable via an env var (e.g. VITE_HOST) so dev setups remain flexible.
| <tr | ||
| key={entry.key} | ||
| className={ | ||
| isSelected | ||
| ? "metadata-table__row metadata-table__row--active" | ||
| : "metadata-table__row" | ||
| } | ||
| onClick={() => setSelectedEntryKey(entry.key)} | ||
| > |
There was a problem hiding this comment.
Clickable <tr> rows are not keyboard-accessible by default (no focus target / Enter/Space activation), which is an accessibility issue. Consider moving the click handler to a <button>/<a> inside the row (or add tabIndex=0, appropriate role, and key handlers) so users can select entries via keyboard and screen readers get correct semantics.
| <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 <tr> rows are not keyboard-accessible by default, so selecting an item is mouse-only. Consider using a focusable control inside the row (button/link) or add tabIndex + key handlers so selection works via keyboard and conveys correct semantics to assistive tech.
| - `src/domain`: Domain layer (entities, resource types, repository definitions) | ||
| - `src/application`: Application layer (use cases) | ||
| - `src/data`: Infrastructure layer (DHIS2 data engine repositories) |
There was a problem hiding this comment.
README lists src/application as the location for use cases, but this repo’s conventions (and openspec/config.yaml + .claude/CLAUDE.md) state there is no src/application/ folder—use cases live under src/domain/usecases/. Update this section to reflect the actual folder structure so new contributors don’t create an application/ layer by mistake.
- Created src/domain/metadata/JsonPackageIndex.ts -- entity types (JsonPackageEntry, JsonPackageReference, JsonPackageIncomingReference, JsonPackageIndex, JsonPackageGraphMode, JsonTypeGraphPolicy), domain constants (typePriorityOrder, graphPolicyByCenterType), the indexJsonPackage function, and shared utilities - Created src/domain/usecases/metadata/BuildJsonPackageDependencyGraphUseCase.ts -- buildJsonPackageDependencyGraph function and all traversal/graph policy helpers - Deleted src/webapp/pages/metadata/json-package-utils.ts - Updated imports in JsonPackageExplorer.tsx and the test file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 110 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - `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.
This section says use cases live in src/application, but this repo’s convention is that use cases live under src/domain/usecases/ and there is no top-level src/application/ folder (see .claude/CLAUDE.md:74-76). Please update the README to match the actual folder structure.
| const queue = [centerKey]; | ||
|
|
||
| while (queue.length > 0) { | ||
| const fromKey = queue.shift(); | ||
| if (!fromKey) continue; | ||
| addRefsForNode(fromKey, key => { | ||
| if (key === centerKey) return; | ||
| queue.push(key); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Using queue.shift() inside the BFS loop makes traversal O(n²) due to array reindexing on every dequeue. For large JSON packages/graphs this can become a bottleneck. Prefer a queue with an index pointer (e.g. let head = 0; while (head < queue.length) { const key = queue[head++]; ... }) or a deque structure.
| </th> | ||
| <th className="metadata-table__cell">id</th> | ||
| <th className="metadata-table__cell">displayName</th> | ||
| <th className="metadata-table__cell">type</th> | ||
| </tr> |
There was a problem hiding this comment.
Hard-coded column headers (id, displayName, type) are user-visible UI strings but aren’t passed through i18n.t(...). Repo convention is that all user-facing strings go through i18n.t (see .claude/CLAUDE.md:90-91). Wrap these headers (or provide translated labels) to keep localization consistent.
| return ( | ||
| <div className="metadata-explorer"> | ||
| <div className="metadata-tabs" role="tablist" aria-label={i18n.t("Metadata views")}> | ||
| <button | ||
| type="button" | ||
| role="tab" | ||
| aria-selected={activeTab === "instance"} | ||
| className={ | ||
| activeTab === "instance" | ||
| ? "metadata-tab metadata-tab--active" | ||
| : "metadata-tab" | ||
| } | ||
| onClick={() => setActiveTab("instance")} | ||
| > | ||
| {i18n.t("Instance Metadata")} | ||
| </button> | ||
| <button | ||
| type="button" | ||
| role="tab" | ||
| aria-selected={activeTab === "load"} | ||
| className={ | ||
| activeTab === "load" | ||
| ? "metadata-tab metadata-tab--active" | ||
| : "metadata-tab" | ||
| } | ||
| onClick={() => setActiveTab("load")} | ||
| > | ||
| {i18n.t("JSON Package")} | ||
| </button> | ||
| </div> | ||
|
|
||
| {activeTab === "instance" ? ( | ||
| <> | ||
| <MetadataQueryBuilder | ||
| value={queryState} | ||
| onChange={setQueryState} | ||
| onTypeChange={handleTypeChange} | ||
| onRun={handleRun} | ||
| /> |
There was a problem hiding this comment.
Tabs use role="tablist"/role="tab", but the associated panels aren’t consistently marked up: the "load" view has role="tabpanel" while the "instance" view is just a fragment. For correct ARIA tabs, each tab should control a corresponding element with role="tabpanel" (with id/aria-controls + aria-labelledby), and inactive panels should be hidden.
| - 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) |
There was a problem hiding this comment.
Tech stack docs say “Vite 4”, but package.json in this PR upgrades Vite to ^7.1.14. Please update this OpenSpec context so it reflects the actual tooling versions developers should expect.
| return { | ||
| x1: fromRect.left + fromRect.width / 2 - containerRect.left, | ||
| y1: fromRect.top + fromRect.height / 2 - containerRect.top, | ||
| x2: toRect.left + toRect.width / 2 - containerRect.left, | ||
| y2: toRect.top + toRect.height / 2 - containerRect.top, | ||
| label: edge.label, |
There was a problem hiding this comment.
Edge coordinates are computed from DOM getBoundingClientRect() values but don’t account for the scroll offset of the scroll container. If the user pans/scrolls the canvas, fromRect.left - containerRect.left will shift while the SVG coordinate system is in scrolled content space, causing lines to misalign with nodes. Include scrollLeft/scrollTop in the coordinate math (and/or recompute on scroll).
| <tr | ||
| key={entry.key} | ||
| className={ | ||
| isSelected | ||
| ? "metadata-table__row metadata-table__row--active" | ||
| : "metadata-table__row" | ||
| } | ||
| onClick={() => setSelectedEntryKey(entry.key)} | ||
| > |
There was a problem hiding this comment.
The rows are clickable via onClick, but <tr> is not keyboard-focusable by default, so keyboard/screen-reader users can’t select an entry. Consider using a <button> in a cell or adding tabIndex, key handlers (Enter/Space), and appropriate ARIA to make selection accessible.
| const config: ProviderProps["config"] = { baseUrl, apiVersion: 41 }; | ||
|
|
There was a problem hiding this comment.
@dhis2/app-runtime will build API URLs as /api/<apiVersion>/.... Setting apiVersion: 41 will fail against older DHIS2 instances (e.g. the CI docker image in .travis.yml is 2.30-*, which won’t have /api/41). Please either keep apiVersion aligned with the minimum supported DHIS2 version (or make it configurable) and update CI/test infra accordingly.
|
This PR was already approved here: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 110 changed files in this pull request and generated 7 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. | ||
|
|
||
| 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 |
There was a problem hiding this comment.
This OpenSpec context says the app consumes the DHIS2 Web API via @eyeseetea/d2-api and that the build uses Vite 4, but the codebase now uses @dhis2/app-runtime DataEngine repositories and vite is pinned to v7 in package.json. Please update the OpenSpec context to match the current implementation so it remains a reliable reference.
| <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.
Rows are clickable via onClick on <tr>, but table rows aren’t keyboard-focusable by default, so keyboard users can’t select an item. Consider rendering a real interactive control (e.g. a <button> inside the first cell), or add tabIndex=0 + role="button" and an onKeyDown handler (Enter/Space) plus appropriate aria-selected semantics.
| export const resourceTypeLabels: Record<ResourceType, string> = { | ||
| dataElements: "Data elements", | ||
| dataSets: "Data sets", | ||
| categories: "Categories", | ||
| categoryCombos: "Category combos", | ||
| categoryOptions: "Category options", | ||
| categoryOptionCombos: "Category option combos", | ||
| }; |
There was a problem hiding this comment.
resourceTypeLabels contains user-facing strings, but it lives in src/domain/ where we can’t (and shouldn’t) depend on i18n. This makes the UI labels non-translatable and violates the project rule that user-facing strings go through i18n.t(...). Suggestion: move the label mapping to the webapp layer (or provide i18n keys here and translate in the UI).
| {selectableResourceTypes.map(type => ( | ||
| <option key={type} value={type}> | ||
| {resourceTypeLabels[type]} | ||
| </option> |
There was a problem hiding this comment.
The resource type option labels are rendered from resourceTypeLabels[type], which are plain English strings and won’t be translated. Since these are user-facing, please translate them (e.g. map ResourceType -> i18n.t(...) in the webapp layer) rather than rendering raw strings.
| <input | ||
| className="metadata-query__input" | ||
| type="text" | ||
| value={value.fields} | ||
| onChange={event => onChange({ ...value, fields: event.target.value })} | ||
| placeholder="id,displayName" | ||
| /> | ||
| </label> | ||
| </div> | ||
|
|
||
| <div className="metadata-query__row"> | ||
| <label className="metadata-query__label metadata-query__label--grow"> | ||
| {i18n.t("Filters (one per line or separated by ;)")} | ||
| <textarea | ||
| className="metadata-query__input metadata-query__textarea" | ||
| value={value.filters} | ||
| onChange={event => onChange({ ...value, filters: event.target.value })} | ||
| placeholder="displayName:ilike:malaria" | ||
| rows={2} |
There was a problem hiding this comment.
The input placeholders (e.g. "id,displayName" and "displayName:ilike:malaria") are user-facing strings but aren’t passed through i18n.t(...). Please translate placeholders as well, or remove them if they shouldn’t be localized.
| {onOpenApi && ( | ||
| <button | ||
| type="button" | ||
| className="graph-node__action" | ||
| title={i18n.t("Open API")} | ||
| onClick={() => onOpenApi(node)} | ||
| > | ||
| <OpenInNewIcon fontSize="small" /> | ||
| </button> | ||
| )} | ||
| {onFocus && ( | ||
| <button | ||
| type="button" | ||
| className="graph-node__action" | ||
| title={i18n.t("Focus in graph")} | ||
| onClick={() => onFocus(node)} | ||
| > | ||
| <CenterFocusStrongIcon fontSize="small" /> | ||
| </button> |
There was a problem hiding this comment.
These icon-only action buttons rely on title for their label. title is not a reliable accessible name for screen readers, so users may hear an unlabeled button. Please add an explicit aria-label (and/or visually-hidden text) for each action button.
| shapeRendering="crispEdges" | ||
| role="img" | ||
| aria-label={`${type} avatar`} | ||
| > |
There was a problem hiding this comment.
aria-label is built from the raw type string (e.g. "dataElements avatar"), which is both non-translated and not very friendly for assistive tech. Please use a human-readable, translated label (e.g. a localized type label + i18n template) for the avatar’s accessible name.
📌 References
📝 Implementation
📹 Screenshots/Screen capture
🔥 Notes to the tester
test github workflow actions only