feat/roi-selector-plugin#62
Conversation
✅ Deploy Preview for biongff-vizarr ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR adds an optional ROI Selector plugin package (@biongff/roi-selector) and introduces a small “deck extension” mechanism in the core viewer so plugins can render overlays and receive click/hover events.
Changes:
- Add a deck.gl extension registry in
@biongff/vizarr(overlays + delegated click/hover/cursor), plus helper atoms for Z-slice and image bounds. - Add a new workspace package
roi-selectorimplementing ROI draw/edit/save/export UI and the corresponding viewer extension hook. - Wire the app to lazy-load the optional plugin, update workspace/build configuration, and adjust Vite config for optional workspace packages.
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| viewer/src/state.ts | Adds extension interfaces/atoms and Z/bounds utilities; changes viewport atom type to plain width/height. |
| viewer/src/index.tsx | Re-exports new extension + Z/bounds utilities for plugin consumption. |
| viewer/src/hooks/useAxisNavigation.tsx | Simplifies hook API to rely only on deckRef. |
| viewer/src/components/VizarrViewer.tsx | Allows rendering children on top of the viewer (plugin UI mounting point). |
| viewer/src/components/Viewer.tsx | Renders plugin polygon overlays and delegates click/hover/cursor to registered extensions; tracks viewport size. |
| sites/app/vite.config.js | Adds Vite plugin to stub out disabled optional workspace packages; optionally aliases roi-selector in dev. |
| sites/app/src/roi-selector.d.ts | Ambient TS declaration to avoid type errors when plugin is absent. |
| sites/app/src/App.tsx | Lazy-loads ROI plugin and gates rendering behind URL roi=1 + plugin availability. |
| sites/app/package.json | Declares @biongff/roi-selector as an optional workspace dependency. |
| roi-selector/vite.config.js | New package Vite library build (ES/CJS) + DTS generation. |
| roi-selector/tsconfig.json | New package TS config for strict typechecking. |
| roi-selector/src/useRoiDeckExtension.ts | Registers overlays and click/hover handlers into the viewer extension registry. |
| roi-selector/src/state.ts | ROI atoms and ROI helper utilities (normalize/clamp/colors). |
| roi-selector/src/index.tsx | Exports RoiSelector component and ROI state/utilities. |
| roi-selector/src/hooks/useRoiFields.ts | Central ROI lifecycle logic: draw/edit/save/delete/visibility + field syncing. |
| roi-selector/src/components/SavedRoiList.tsx | UI for collapsible list of saved ROIs + “copy all”. |
| roi-selector/src/components/SavedRoiItem.tsx | UI for a single ROI row (visibility/go-to/copy/edit/delete). |
| roi-selector/src/components/RoiDrawControls.tsx | UI controls for draw/save/discard/update/cancel. |
| roi-selector/src/components/RoiCoordinateFields.tsx | Coordinate entry fields (with optional Z range fields). |
| roi-selector/src/RoiSelector.tsx | Main plugin panel: layout, go-to navigation, and clipboard export. |
| roi-selector/package.json | New workspace package manifest (deps/peers/exports/scripts). |
| pnpm-workspace.yaml | Adds roi-selector workspace package. |
| pnpm-lock.yaml | Adds lock entries for the new workspace package and optional dependency linkage. |
| package.json | Adds build script for roi-selector and includes it in the root build pipeline. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
Hi @LucaAnce. The selector works really well, the user experience is smooth, and I think you've done a great job of organising the code into logical components. One question I had is, it seems as though communication between the main viewer is using global state (e.g. via jotai atoms). Did you consider any other methods of communication, e.g. via props or callback functions? I'll be honest and say I am not 100% familiar with the workings of deckgl, so it may be that global state is a necessary evil, however I do have a few concerns with the use of global state (I sympathise with you as it is already being used heavily in the viewer itself, which I must say I think is already overkill, but I think extending the global state to an external plugin is a step further). In my experience global state makes it difficult to reason about relationships and dependencies, increases the chance of bugs and significantly increases the amount of effort it takes to solve these bugs. I'm hoping to also take steps to decrease the reliance of the main viewer on global state. |
…ty, and add conversion helpers
|
Hi @AlexSurtees, regarding your concern:
I definitely see your point, due to lack of experience I went for the easiest solution could find, as I'm not sure what is the best solution or the alternatives we have in the first place. I tried some AI-assited brainstorming and by filtering out and re-elaborating a bit 2 things pop up as possible here:
What do we think? |
Thanks Luca, I think keeping the viewer and plugins as decoupled as possible is important, so would favour the first option. We can keep an eye on rendering performance etc. I've been thinking about what needs to be exposed by the viewer in order for the plug-in to work, just sounding this out so you can tell me if you think it's feasible: Currently the viewer and plugin share the deckExtensions atom, the deck.gl layer is constructed from this. I'm not quite grasping 100% how the handleClick/handleHover methods are changed depending on whether the plugin is active/user is drawing a region. However, could we change it so that:
Then we can explicitly pass the props from viewer to deckgl. I think this has a few nice advantages:
Does this sound like a feasible solution? Any concerns? |
|
Sounds like a reasonable solution to me. Will try to have to implement your suggestions and share any issue that might come to my mind in the process, thanks a lot! |
…, added support for t axis, z and t coordinates optional based on axis being present
…ewer communication
|
@AlexSurtees a summary of the latest changes for your convenience:
Let me know if this implementation solves your concerns with using global states, and if it looks clean/clear enough or what I can improve on. |
There was a problem hiding this comment.
This is a good step in the right direction, could you comment on whether something like this would work:
e.g. in sites/app/App.tsx
const [additionalLayers, setAdditionalLayers] = useState([])
const [onCanvasClick, setOnCanvasClick] = useState(defaultOnCanvasClick)
<Vizarr ... additionalLayers={additionalLayers} onCanvasClick={onCanvasClick}>
<RoiSelector setAdditionalLayers={setAdditionalLayers} setOnCanvasClick={setOnCanvasClick}>
could even wrap it up in e.g.
pluginProps = {
additionalLayers: ,
onCanvasClick: ...,
...
}
so that the boundary between the plugin and viewer is explicit, and the viewer is not aware of 'plugins'. There aren't that many layers to the app so it should just be 1 or 2 layers to drill the props down. Maybe there is something that I'm missing that stops this from working.
There was a problem hiding this comment.
Hey Alex, I see what you propose here. It also feels to me that the App would be the preferred place to logically bridge the viewer with the plugins.
I find however a potential complication with your idea. The plugin needs to read from viewer (eg. viewport, imageBounds, zInfo...) and write to it (eg. setViewState, setZSlice, ...), but App lacks access to viewer state which lives in jotai atoms inside Vizarr's Provider. So basically we would need the viewer to expose everything at the App level.
Currently ViewerPluginApi (in ViewerPluginContext) exposes everything needed, and the PluginBridge in VizarrViewer allows to manage multiple plugins naturally by regsitration via custom pluginid. In the end the Viewer is still 'unaware'/not directly managing the plugins as we wanted.
So overall I understand what you are aiming for, and I see the value in it being more explicit, but also it might make the things more complicated possibly. what do you think?
There was a problem hiding this comment.
Hi Luca, I see your point. I'm going to take a bit of time to digest and maybe chat to Dave/Daniela and get back to you ASAP.
There was a problem hiding this comment.
Hi Luca, after discussion with Dave and Daniela we are keen for the state to be lifted up to the app level. We think this will be the most simple/explicit while also providing a good level of flexibility for plugins/other integrations. I'm conscious you mentioned complications with lifting the state up - do you foresee any serious difficulties with doing this? This is something I can support on if it's significant refactoring/structural changes to the Viewer/App.
There was a problem hiding this comment.
Hi Alex, ok I will go for the changes. It shouldn't be too complicated I will try to make it as clean and straightforward as possible, a second look on it when done would be great! :)
There was a problem hiding this comment.
@AlexSurtees pushed the change. Let me know if this is the intended direction and what can be improved. Tried to summarize main logic (also put in the general description):
The ROI selector plugin is integrated at the App level, which acts as the bridge between the viewer and the plugin(s). App owns all ROI state (eg saved/pending ROIs) and receives viewer state (image bounds, viewport, navigation callbacks) from Vizarr via the onViewerStateChange callback. It produces deck.gl layers and interaction handlers, then passes those back to Vizarr as props. This keeps the viewer and roi-selector fully decoupled, and the App had explicit control over all state and wiring.
|
@AlexSurtees There is one last feature for the ROI plugin which would be the possibility to import ROIs saved in the zarr through fractal, this would close the circle of the plugin lifecyle. I am working on that right now and thought it might be best to not add the changes in this PR since it is a moderately large set, and the discussion here is still ongoing and now focusing on the core logic on how plugin should be integarted. For now I created a separate branch and I plan a follow-up PR to this one, but if you would prefer to have everything here lmk I can merge those changes. |
| export default defineConfig(({ mode }) => ({ | ||
| plugins: [react()], | ||
| resolve: { | ||
| alias: { | ||
| ...(mode === "development" | ||
| ? { | ||
| "@biongff/vizarr": path.resolve(__dirname, "../../viewer/src/index.tsx"), | ||
| } | ||
| : {}), | ||
| const source = process.env.VIZARR_DATA || "https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.1/6001253.zarr"; | ||
|
|
||
| export default defineConfig(({ mode }) => { | ||
| return { | ||
| base: "./", | ||
| plugins: [react()], | ||
| resolve: { | ||
| alias: { | ||
| "@biongff/vizarr": path.resolve(__dirname, "../../viewer/src/index.tsx"), | ||
| "@biongff/roi-selector": path.resolve(__dirname, "../../roi-selector/src/index.tsx"), | ||
| }, |
There was a problem hiding this comment.
Had issues when using the plugin in Fractal without these changes, am I missing something?
… fractal integration
…gins orchestartor
This pull request introduces a new package,
@biongff/roi-selector, which provides a user interface for selecting, editing, and managing Regions of Interest (ROIs) within vizarr. The implementation is in plugin-style, and includes a React component for ROI selection, coordinate input, drawing controls, and a saved ROI list, along with all supporting logic and UI components.The plugin allows to:
The most important changes are:
ROI Selector implementation:
RoiSelectorReact component, which provides a collapsible panel for drawing, editing, saving, copying, and navigating ROIs on the image viewer.RoiCoordinateFields), drawing controls (RoiDrawControls), and a list of saved ROIs (SavedRoiList,SavedRoiItem).New ROI Selector package:
roi-selectorpackage to the workspace (pnpm-workspace.yaml) and updated the rootpackage.jsonand build scripts to include building this package.pnpm-lock.yamlto include all dependencies, devDependencies, and workspace links for@biongff/roi-selector, and made it an optional dependency for the main app.roi-selector/package.jsonwith all necessary dependencies, peer dependencies, build scripts, and export fields for module consumption.Plugin Integration Logic
The ROI selector plugin is integrated at the App level, which acts as the bridge between the viewer and the plugin(s). App owns all ROI state (eg saved/pending ROIs) and receives viewer state (image bounds, viewport, navigation callbacks) from Vizarr via the onViewerStateChange callback. It produces deck.gl layers and interaction handlers, then passes those back to Vizarr as props. This keeps the viewer and roi-selector fully decoupled, and the App had explicit control over all state and wiring.
Notes
roiboolean parameter is injected in the URL and has to be manually set to 1 to enable the UI controls of the roi plugin.Sidenote
First web-dev contribution, I have a lot to learn I know, be lenient pls 🙏🙇