feat: consume dmv deck.gl bulk annotations; drop clustering UI - #405
feat: consume dmv deck.gl bulk annotations; drop clustering UI#405igoroctaviano wants to merge 10 commits into
Conversation
Depend on dicom-microscopy-viewer@b9925eb (feat/bulk-ann-deckgl). Remove obsolete clustering settings/state and wire measurement limitValues via getAnnotationGroupMeasurementRange.
|
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript | Aug 1, 2026 4:16p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
… null measurement range - pin dicom-microscopy-viewer to 51a7c31 (code-review fixes: affine resolution, render triggers, view-change LOD refresh, rotation support, measurement filter wiring) - remove the corrupted 'set this to true or false' placeholder pnpm wrote into pnpm-workspace.yaml and document that the tarball-URL allowBuilds entry must track the dependency pin - handle getAnnotationGroupMeasurementRange returning null before the viewer has fetched measurement values
Code review pass — fixes pushed in 575bb42
|
… baseURL fixes - explicitly send measurement: undefined when 'None' is selected so the viewer deactivates measurement-based filtering (previously only the color was reset and the filter stayed active) - bump dicom-microscopy-viewer to b0fcc04: LOADING_STARTED/ENDED are now always paired (prevents a stuck global loading indicator when a group is hidden mid-load) and the streaming fallback uses client.baseURL
Review round 2 (2774481)Second review pass. One real bug found and fixed, plus a dmv bump:
Known limitation (unchanged): if a measurement is selected before the viewer has fetched measurement values, ✅ biome clean · tsc clean · production build OK · 37/37 tests pass (the |
The dicom-microscopy-viewer dependency is now installed from a git commit and must run its prepare script to produce dist/. Build scripts remain gated by the allowBuilds allowlist in pnpm-workspace.yaml, so dropping --ignore-scripts keeps supply-chain protection.
The previous pin failed to prepare on GitHub Actions because dmv's babel exclude on /(node_modules)/ matched the setup-pnpm store path, skipping import.meta transforms. dmv 6041fe0 scopes the babel rule with include: src instead.
|
Visit the preview URL for this PR (updated for commit 41dd860): https://idc-external-006--pr405-feat-bump-dmv-bulk-a-anpo4smb.web.app (expires Sat, 08 Aug 2026 16:19:04 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 88aacecd98ba54d2f9c8d201a9444e43d1ad8307 |
Rule githubactions:S6505 flags pnpm install without --ignore-scripts, but the git-hosted dicom-microscopy-viewer needs its prepare script to build dist/ at install time. Scripts stay gated per-package by the allowBuilds allowlist in pnpm-workspace.yaml. Revert once dmv is pinned to a published npm release.
CI green (298472a)All checks now pass. Three issues were blocking CI, all specific to installing dicom-microscopy-viewer from a git commit instead of a published npm release:
Note: |
Drives the real app in a WebGL (SwiftShader) browser and compares the deck.gl bulk-annotation overlay against a committed screenshot baseline, so rendering regressions are caught automatically. Default target is the ~396k-polygon TCGA-02-0001 "Nuclei" group on the public IDC proxy. - playwright.config.ts: deterministic software WebGL2, per-OS baselines, serves a prod build baked with the new e2e config, retries in CI - e2e/: whole-slide visual-regression spec plus a memory/OOM guardrail and a toggle-off smoke test; Linux baseline generated via the pinned Playwright container (e2e/update-snapshots-docker.sh) - .github/workflows/e2e-visual.yml: runs the suite inside that same container so it matches the committed baselines - public/config/e2e.js: proxy-backed config with 429/5xx retry
serve@14 crashes under path-to-regexp v8 (pathToRegExp.compile is not a function), which would make Playwright's webServer fail to start in CI. scripts/serve-e2e.mjs is a dependency-free static server with SPA fallback for /studies/... deep links.
- serve-e2e.mjs: open()+fstat instead of existsSync on request paths (Sonar S6549), reject directories so createReadStream cannot hang - playwright.config.ts: drop the no-op reporter ternary (Sonar S3923) - Exclude e2e/, serve-e2e.mjs, and playwright.config.ts from Sonar and DeepSource — they are test infrastructure, not production app code - Address remaining DeepSource nits in the Playwright helpers
The whole-slide screenshot was flaky for two independent reasons: 1. /studies/:uid sometimes redirected to a different SM series (this study has four slides, each with its own Nuclei group) — pin the DX1 series UID in the URL. 2. WSI tile decoding varies run-to-run — hide non-WebGL canvases and chrome before toHaveScreenshot so only the deck.gl overlay is compared. Regenerated the Linux baseline against the production e2e build.
|



Summary
Companion to dicom-microscopy-viewer#271.
Bulk Microscopy Simple Annotations are now rendered by a deck.gl overlay inside dmv, so slim no longer needs the OpenLayers clustering controls.
Changes
dicom-microscopy-viewer@b9925eb(feat/bulk-ann-deckgl) via GitHub (allowBuilds for prepare/webpack)SlideViewerstate /constructViewerslimitValuesthroughgetAnnotationGroupMeasurementRange(new dmv API) so the existing slider drivesDataFilterExtensionfiltering once measurements are hydratedNotes
"dicom-microscopy-viewer": "^0.49.0"and drop the git allowBuilds entry.Test plan
tsc --noEmit, production build, unit tests (pre-push)REACT_APP_CONFIG=example pnpm start→ open the IDC ANN series, toggle large polygon groups, confirm progressive paint / no OOM, measurement slider + ROI pick/zoom still work