fix: Address auto toggle of ann group#382
Open
igoroctaviano wants to merge 1 commit intomasterfrom
Open
Conversation
|
|
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript | Apr 30, 2026 9:44a.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.
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.



fix: Auto-toggle all annotation groups of an ANN series on load
Summary
Closes the regression reported in #260 (latest comment) where opening a Microscopy Bulk Simple Annotation (ANN) series via URL only toggled the visibility of the first annotation group instead of all annotation groups belonging to that series.
This PR also fixes a related latent bug in the SR ROI visibility handler that was mutating the
visibleRoiUIDs/selectedRoiUIDsSetreferences in place (rather than cloning), which can cause React to bail out of re-rendering on auto-load of SR-derived URLs.Affected behavior
/studies/{studyUID}/series/{annSeriesUID}where the ANN series contains multiple annotation groups.Root cause
SlideViewer.loadDerivedDatasethad two compounding issues in theMicroscopyBulkSimpleAnnotationbranch:find()instead offilter()— introduced in #360. When the original behavior from #261 (forEachover all annotation groups) was changed to add aseriesInstanceUIDfilter,forEachwas replaced withfind(), so only the first matching annotation group was ever toggled.handleAnnotationGroupVisibilityChangecallsvolumeViewer.showAnnotationGroup()and re-throws after notifying. Even after switching back tofilter().forEach(), a throw on any subsequent group (e.g. a dmvsetAnnotationGroupStyleissue) aborted the iteration, leaving only the first group toggled. The per-grouprunValidations({ dialog: true })was also designed for manual user toggles, not for automatic load-time iteration over many groups.A separate latent bug in
handleAnnotationVisibilityChange(the SR ROI handler) was mutating the existingSetreference insidesetState, which can cause React to bail out of re-rendering since the new state holds the same reference as the previous one.Changes
src/components/SlideViewer.tsxloadDerivedDataset—MicroscopyBulkSimpleAnnotationbranch:find()withfilter()so all annotation groups whoseseriesInstanceUIDmatches the URL series are picked up, not just the first.handleAnnotationGroupVisibilityChangefor the auto-load path:volumeViewer.showAnnotationGroup(uid)directly with a per-grouptry/catch, so a failure on one group no longer aborts the iteration.runValidations({ dialog: true })modal (which is intended for manual interactive toggles, not auto-load over N groups).setStateat the end that adds all successfully-shown UIDs tovisibleAnnotationGroupUIDs, avoiding any chance of intermediatesetState/re-render interleavings dropping updates.logger.debuglines (auto-load Microscopy Bulk Simple Annotation: found N matching annotation group(s) out of M total for series "..."andshowing X/N annotation group(s)) pluslogger.errorper-group failures for easier triage.handleAnnotationVisibilityChange(SR ROIs) —Setmutation fix:state.visibleRoiUIDsandstate.selectedRoiUIDs(new Set(state.X)) inside the functionalsetStateupdater instead of mutating the same reference. This is the same pattern already used by the ANN handler and by the rest of the visibility handlers in the file.Diff scope
Test plan
Tested locally via
bun devagainst IDC test data with the following ANN URLs (referenced in the issue):https://andrey-slim-test.web.app/studies/1.2.826.0.1.3680043.8.498.24144283661646178704245748389871574522/series/1.2.826.0.1.3680043.10.511.3.40735706884016056933166099558854806— multi-group ANN that previously only toggled the first group; now all toggle on.found N/M,showing X/N, plus any per-groupfailed to auto-show…errors that previously broke iteration).handleAnnotationGroupVisibilityChangepath is untouched for user interaction).bun lint/ TypeScript compile).Risk / rollback
Set-cloning change is strictly safer than the previous in-place mutation (matches the pattern used elsewhere in the same component) and does not change the public API.src/components/SlideViewer.tsx.Related