Repository navigation
feat: consume dmv deck.gl bulk annotations; drop clustering UI - #405
Open
igoroctaviano wants to merge 39 commits into
Open
igoroctaviano wants to merge 39 commits into
igoroctaviano wants to merge 39 commits into
Conversation
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript |  |
Oct 26, 2026 5:10p.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.
Collaborator
Author
Code review pass — fixes pushed in 575bb42
|
Collaborator
Author
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Visit the preview URL for this PR (updated for commit cec33a8): https://idc-external-006--pr405-feat-bump-dmv-bulk-a-gpdzekaa.web.app (expires Mon, 02 Nov 2026 17:12:34 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 88aacecd98ba54d2f9c8d201a9444e43d1ad8307 |
Collaborator
Author
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: |
📦 Firebase Preview - Using Published DMVThis preview is using the published
|
This was referenced Aug 14, 2026
🔗 Firebase Preview - Linked to DMV BranchThis preview is using a linked
|
4 similar comments
🔗 Firebase Preview - Linked to DMV BranchThis preview is using a linked
|
🔗 Firebase Preview - Linked to DMV BranchThis preview is using a linked
|
🔗 Firebase Preview - Linked to DMV BranchThis preview is using a linked
|
🔗 Firebase Preview - Linked to DMV BranchThis preview is using a linked
|
igoroctaviano
added a commit
that referenced
this pull request
Aug 14, 2026
The e2e visual regression workflow is for testing bulk annotation rendering, which belongs in the feat/bump-dmv-bulk-ann branch (PR #405), not in this viv-loader branch. This branch doesn't have the e2e test infrastructure (build:e2e, test:e2e scripts). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
igoroctaviano
added a commit
that referenced
this pull request
Aug 14, 2026
- Remove e2e-visual.yml workflow (belongs in PR #405, not this branch) - Add skipcq for void operator usage (JS-0098) - Add skipcq for high complexity functions (JS-R1005): - buildPointLayersFromGraphicData - buildPathLayersFromGraphicData - loadBulkAnnotationMetadataAndJobs - Rename short variables (JS-C1002): - g→typedGeom, t→geomType - b→bulkApi - d→typedDataset - Use regular strings instead of template literals (JS-R1004) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
fedorov
requested changes
Aug 14, 2026
fedorov
left a comment
fedorov
left a comment
Member
There was a problem hiding this comment.
Fill has incorrect behavior - appears only at certain zoom level. Also, it seems like the outline appearance changes across resolutions.
Small nitpick - it would be good to maintain consistent styling for the UI controls in the popup - was it intentional to change from rounded to squared items?
Member
|
Also - if you didn't notice from the video - it is quite annoying that the structure popup does not disappear when mouse pointer is out of the viewport! |
Member
|
Also, when multiple groups are being loaded, I do not think it helps that notifications stack up! 
|
Member
|
~~AND it appears that BoneMarrow bounding boxes are not rendered... ~~ -- looks like this might have been peculiar to one of the wg26 connectathon servers! |
🔗 Firebase Preview - Linked to DMV BranchThis preview is using a linked
|
igoroctaviano
added a commit
that referenced
this pull request
Aug 20, 2026
Address review feedback from fedorov on PR #405: 1. Fix tooltip not disappearing when mouse leaves viewport - Add onMouseLeaveViewport prop to SlideViewerContent - Hide HoveredRoiTooltip when mouse exits viewport area 2. Prevent notification stacking for annotation groups - Limit visible entries to 3 (MAX_VISIBLE_ENTRIES) - Prioritize active (loading) entries over settled ones - Show summary of hidden entries ("+N more loading, M completed") - Reduce linger time from 4s to 1.5s for completed entries 3. Restore UI styling consistency (rounded corners) - Add rounded corners to Radix UI button, input, popover, select - Match Ant Design's rounded styling used elsewhere in the app - Update AnnotationGroupLoadIndicator card to use borderRadius: 4px Co-Authored-By: Claude <noreply@anthropic.com>
Depend on dicom-microscopy-viewer@b9925eb (feat/bulk-ann-deckgl). Remove obsolete clustering settings/state and wire measurement limitValues via getAnnotationGroupMeasurementRange.
… 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
… 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
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.
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.
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
Replaces the antd-based Display Settings popover (color/opacity/fill/ measurement controls) with a dedicated component built on Tailwind CSS and Radix UI primitives (Popover, Slider, Switch), for a cleaner, more compact layout than antd's Row/Col grid allowed. Scoped narrowly to avoid disturbing anything else in the app: - New components live under src/components/ui/ and are only imported by the new AnnotationGroupDisplaySettings.tsx — ColorSlider.tsx and OpacitySlider.tsx (still antd-based) are untouched, since Segments, Optical Paths, and Mappings share them too. - Tailwind's preflight reset is disabled (tailwind.config.js) so it doesn't also reset antd's own markup elsewhere; border-box sizing is instead scoped to just this component tree via a `.dmv-ui` class (src/tailwind.css), applied on the popover trigger and content. - react-scripts 5 has built-in Tailwind support (auto-detects tailwind.config.js), so no craco postcss override was needed — craco's own postcss-loader patching turned out to be incompatible with craco-less's loader chain anyway.
…ed controls Disabling Tailwind's global `preflight` (to avoid resetting antd elsewhere) also stripped `border-style: solid` from the new Display Settings components. Every element still got the right border color and width, but without an explicit style the browser fell back to its native default border style for buttons/inputs — an inset/outset 3D bevel — which read as a legacy OS dialog rather than a flat, modern panel. Adds a proper scoped mini-preflight (box-sizing, border-style, and the button/input/select resets preflight normally provides) under a `.dmv-ui` class instead of skipping it outright. The scope selector is wrapped in `:where()` so it doesn't gain extra specificity over a plain utility class — Tailwind v3's `@layer` only orders rules in the output rather than using native CSS cascade layers, so without this a selector like `.dmv-ui button` (0,1,1) would beat `.bg-neutral-200` (0,1,0) regardless of source order, which is what made the Filled switch render with a fully transparent (invisible) track until fixed. Also adds subtle shadow-sm to inputs/slider thumbs/the select for a bit more depth, without introducing rounded corners.
pnpm 11's dependency status check fails when the git-hosted dicom-microscopy-viewer's tarball URL isn't in allowBuilds. Instead of hardcoding a SHA that goes stale, dynamically insert the resolved DMV_SHA into pnpm-workspace.yaml before install. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This reverts commit 60ad633.
Updated comments for clarity regarding package handling.
- Use setState callback when referencing previous state (AnnotationGroupItem) - Add explicit return undefined in useEffect (AnnotationGroupLoadIndicator) - Add skipcq comments for valid dynamic delete patterns (SlideViewer) - Use explicit React imports instead of namespace imports (ui components) - Use stable string keys for slider thumbs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Radix primitives are designed to be used with namespace imports for clarity and consistency. Added skipcq comments to suppress DeepSource JS-C1003 warnings. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The e2e visual regression tests were failing because they used the published npm version of dicom-microscopy-viewer instead of the linked git branch that contains bulk annotation features. This adds the same DMV branch resolution and linking logic that exists in deploy-to-firebase.yml: - Resolve DMV branch from PR body (dmv-branch:) or matching branch name - Dynamically update package.json and pnpm-workspace.yaml allowBuilds - Build DMV from git dependency if dist files are missing Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The Playwright container doesn't include the GitHub CLI, causing DMV branch resolution to silently fail. Also, the container's default shell is /bin/sh which doesn't support `set -o pipefail`. Changes: - Add step to install gh CLI before DMV branch resolution - Use `shell: bash` for steps that need bash features Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address review feedback from fedorov on PR #405: 1. Fix tooltip not disappearing when mouse leaves viewport - Add onMouseLeaveViewport prop to SlideViewerContent - Hide HoveredRoiTooltip when mouse exits viewport area 2. Prevent notification stacking for annotation groups - Limit visible entries to 3 (MAX_VISIBLE_ENTRIES) - Prioritize active (loading) entries over settled ones - Show summary of hidden entries ("+N more loading, M completed") - Reduce linger time from 4s to 1.5s for completed entries 3. Restore UI styling consistency (rounded corners) - Add rounded corners to Radix UI button, input, popover, select - Match Ant Design's rounded styling used elsewhere in the app - Update AnnotationGroupLoadIndicator card to use borderRadius: 4px Co-Authored-By: Claude <noreply@anthropic.com>
Change // comments to /** */ format to match project conventions. Co-Authored-By: Claude <noreply@anthropic.com>
igoroctaviano
added a commit
that referenced
this pull request
Aug 24, 2026
The e2e visual regression workflow is for testing bulk annotation rendering, which belongs in the feat/bump-dmv-bulk-ann branch (PR #405), not in this viv-loader branch. This branch doesn't have the e2e test infrastructure (build:e2e, test:e2e scripts). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
igoroctaviano
added a commit
that referenced
this pull request
Aug 24, 2026
- Remove e2e-visual.yml workflow (belongs in PR #405, not this branch) - Add skipcq for void operator usage (JS-0098) - Add skipcq for high complexity functions (JS-R1005): - buildPointLayersFromGraphicData - buildPathLayersFromGraphicData - loadBulkAnnotationMetadataAndJobs - Rename short variables (JS-C1002): - g→typedGeom, t→geomType - b→bulkApi - d→typedDataset - Use regular strings instead of template literals (JS-R1004) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
igoroctaviano
force-pushed
the
feat/bump-dmv-bulk-ann
branch
from
August 24, 2026 20:23
3ec1597 to
2392e52
Compare
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
🔗 Firebase Preview - Linked to DMV BranchThis preview is using a linked
|
Brings in latest security fixes and dependency updates from master while preserving dmv git reference for deck.gl bulk annotations feature branch.
🔗 Firebase Preview - Linked to DMV BranchThis preview is using a linked
|
Resolve merge conflicts keeping the feat/bump-dmv-bulk-ann branch's dicom-microscopy-viewer GitHub reference and regenerate lock file.
Add explicit version entry for @parcel/watcher@2.6.0 to allowBuilds. pnpm 11 requires exact version specifications for build script approval.
Resolved conflicts: - package.json: kept clsx, updated dcmjs to 0.52.0 - pnpm-lock.yaml: regenerated from master
- Remove redundant version-specific @parcel/watcher entry - The generic '@parcel/watcher': true covers all versions - Keep git-hosted DMV tarball entry without comments
Updated git dependency to latest DMV master (67e1cc4) which includes @parcel/watcher in allowBuilds, fixing CI build failures.
The caret version (^1.62.1) resolved to 1.64.0, which expects different browser binaries than the v1.62.1 container provides.
Replace the git SHA pin with the stable npm release that already includes the deck.gl bulk-annotation work.
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dmv-branch: feat/bulk-ann-deckgl
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