Skip to content

fix: improve slide-list overview preview sizing (#399) - #400

Open
igoroctaviano wants to merge 5 commits into
masterfrom
fix/399-overview-sizing
Open

fix: improve slide-list overview preview sizing (#399)#400
igoroctaviano wants to merge 5 commits into
masterfrom
fix/399-overview-sizing

Conversation

@igoroctaviano

@igoroctaviano igoroctaviano commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixes slide-list preview sizing when THUMBNAIL or large OVERVIEW images use the full-slide TotalPixelMatrix extent but the rendered PNG is much smaller (Heuristic for sizing overview needs improvements #399).
  • Adds computeOverviewPreviewResizeFactor to derive OverviewImageViewer.resizeFactor from the 100px preview tile and matrix dimensions (replaces the removed fixed 0.3 thumbnail heuristic from fix: Thumbnail fallback and multiple thumbnail using dicom tag browser #340).
  • Defers viewer mount until the preview container is laid out, then keeps the map sized via ResizeObserver.
  • Adds client-side overview mini-map clamping (clampOverviewMapInViewport) for the volume viewer until the published DMV bundle includes the same sizing fix.
  • Fixes mangled series UIDs from nested antd Menus and replaces nested slide Menu with a plain list.

Root cause

With resizeFactor: 1, DMV builds an extent from the full matrix while retrieveInstanceRendered returns a small overview/thumbnail PNG — the preview shows a tiny image or fails to fit the tile.

Related

  • Companion to ImagingDataCommons/dicom-microscopy-viewer#266 (DMV overview sizing + integer viewport rounding). After that PR is published, bump the dicom-microscopy-viewer dependency in Slim; the client-side clamp can remain as a safety net or be simplified.

Test plan

Closes #399

Compute OverviewImageViewer resizeFactor from preview tile dimensions and
TotalPixelMatrix size so THUMBNAIL/OVERVIEW extent matches the rendered PNG.
Restores fit after #340 removed the fixed 0.3 thumbnail heuristic (#399).
@deepsource-io

deepsource-io Bot commented Jul 15, 2026

Copy link
Copy Markdown

DeepSource Code Review

We reviewed changes in e1d5b26...2681183 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade   Security  

Reliability  

Complexity  

Hygiene  

Code Review Summary

Analyzer Status Updated (UTC) Details
JavaScript Aug 6, 2026 10:37p.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.

@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

Visit the preview URL for this PR (updated for commit 2681183):

https://idc-external-006--pr400-fix-399-overview-siz-6kevhuiw.web.app

(expires Thu, 13 Aug 2026 23:05:45 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 88aacecd98ba54d2f9c8d201a9444e43d1ad8307

Google Healthcare rejects fractional viewport=w,h (HTTP 400). Derive
resizeFactor so cols*factor and rows*factor are integers; fall back to
factor 1 when no integer downscale fits the slide-list tile.
@igoroctaviano igoroctaviano changed the title fix: improve slide-list overview preview sizing (#399) WIP fix: improve slide-list overview preview sizing (#399) Jul 15, 2026
@igoroctaviano igoroctaviano changed the title WIP fix: improve slide-list overview preview sizing (#399) fix: improve slide-list overview preview sizing (#399) Jul 15, 2026
@fedorov

fedorov commented Aug 5, 2026

Copy link
Copy Markdown
Member

@igoroctaviano this is how the second slide looks like right now... is this expected?

image

@igoroctaviano

Copy link
Copy Markdown
Collaborator Author

@igoroctaviano this is how the second slide looks like right now... is this expected?

image

Yes, if it's a wide and low case. The heuristic is using the slide dimensions to make it easier to see the overview while navigating. I can set it to be fixed height and width if that makes sense too.

@fedorov

fedorov commented Aug 6, 2026

Copy link
Copy Markdown
Member

I admit I do not know what is a good heuristic, but when overview has the same size as the actual image, I am not sure the heuristic is good. Why is wide-and-low taking entire width, but narrow-and-tall does not take the full height?

But after all, it is not a big deal - I think this is better than what we had before, so let's merge and refine later.

Prefer a shared viewport fraction on both axes and cap growth at 60% so
wide slides no longer spill to full width while tall slides get matching
height treatment.
@sonarqubecloud

sonarqubecloud Bot commented Aug 6, 2026

Copy link
Copy Markdown

@igoroctaviano

Copy link
Copy Markdown
Collaborator Author

Addressed the wide vs tall asymmetry in the overview mini-map heuristic (also mirrored in ImagingDataCommons/dicom-microscopy-viewer#266):

  • Prefer the same viewport fraction on both axes (45%)
  • Grow only toward a hard max of 60% so wide slides no longer spill to nearly full width
  • Apply the same min-side floor and max-fraction cap for tall slides

Wide and tall extremes now get matching treatment instead of width-first expansion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Heuristic for sizing overview needs improvements

2 participants