-
Notifications
You must be signed in to change notification settings - Fork 4
Open file selections and metadata in Vol-E v2 #621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| // custom hook this, like `useOpenInCfe`? | ||
| const openInVole = React.useCallback(async (): Promise<void> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't end up breaking out a custom hook for this because:
- It uses the tiny utility function
getFileExtensionand I didn't feel like copying it over. - More importantly, it uses
openWindowWithMessage. Like I said in that function's docs, it may be useful for opening large collections of data in other apps, and I didn't want it isolated in another file just yet. - The implementation below is just on the edge of feeling okay to include inline, if
openInCfewas long enough to break out.
Any of the above could fall the other way for someone else or change in the future though, so I also kept the comment.
| "reselect": "4.0.x", | ||
| "string-natural-compare": "3.0.x" | ||
| "string-natural-compare": "3.0.x", | ||
| "uuid": "^13.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already present somewhere further down the dependency tree, so depending on it explicitly doesn't change much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on whether we need to consider url encoding? unsure if it would be an issue here
Also, extremely minor: Vole vs VolE for capitalization? It keeps making me think of rodents
I think we should avoid spelling it |
| * emphasize that this protocol is both message- and receiver-agnostic, and could be used to send | ||
| * large bundles of data to other apps as well. | ||
| */ | ||
| function openWindowWithMessage(openUrl: URL, message: any): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could message bit any more specific? Like perhaps Record<string, any> | undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That any is there to match the signature for postMessage, which can also take any type. Certainly no reason we can't make our own function more specific though.
| const allDetails = await fileSelection.fetchAllDetails(); | ||
| const details = allDetails.filter((detail) => { | ||
| const fileExt = getFileExtension(detail); | ||
| return fileExt === "zarr" || fileExt === ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sldy files are sometimes captured in the community as .sldy/ which would become "" here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getFileExtension is unaware of / and would get this right.
...which implies a bigger problem: this will fall over for valid Zarr images without an extension with a . somewhere else in their file path: .../john.doe/valid_zarr. I'll look at making that more robust, including special-casing .sldy/.
I went down a pretty significant URL encoding rabbit hole in the time this has been open. The results of that on the Vol-E side are at allen-cell-animated/vole-app#473. But my conclusions about encoding on this side are:
I made some small modifications with 4. in mind. |
Context
For a while, we've wanted a way to send more data to an opened Vol-E window than could practically fit in URL query parameters — namely, large selections of images and image metadata. Previous passes at this include #516, which got caught up in arcane browser security policies; and #566, which requires an intermediate server, and targets CFE rather than Vol-E. The implementation in this PR happens entirely locally, appears unlikely to run afoul of any cross-origin isolation, and has the convenient bonus of being backwards-compatible with versions of Vol-E that aren't expecting the extra protocol.
Changes
Here's how it works:
openInVolehandler always includes something in theurlquery param of the Vol-E URL it opens, which ensures Vol-E will always show something even if one of the steps below fails.storageiduniquely identifies the message BFF will send to Vol-E, andmsgoriginis the origin of the opening BFF window (the slice of the URL from the beginning through the port, i.e. https://bff.allencell.org), which helps with security.postMessageto send the value ofstorageidback to BFF, indicating that it's ready to receive the message content.postMessages back the extra data.allen-cell-animated/vole-app#467 is the counterpart PR on the Vol-E side. It gets into the details of how Vol-E handles the received message.
Testing
With BFF on this branch...
msgoriginandstorageidwill be in the URL but unused.msgoriginandstorageidshould still be there to send image metadata.VOLE_BASE_URLto http://localhost:9020/viewer, and start Vol-E locally on the branchfeature/receive-from-bff. Make a large selection of image files and choose "Open in Vol-E." Expectation: subject to the caveats described in Receive multiscenes and metadata from BFF allen-cell-animated/vole-app#467, Vol-E will open the whole image collection, starting on the image that was "focused" in BFF, and data from other columns in BFF will appear in Vol-E's "Metadata" panel.