Neuroglancer short links #277
Conversation
|
Current screenshots: 
|
|
I have not looked too deeply at the code, but I think there are aspects of the UI/UX that l like from both. I like the "+ New Link" button in the Claude version and I like that it is not tucked away next to the search box. I like this version for the input form, since it is what I would expect from a link shortener. I also like that the link is clickable in this version. I am not sure why there is an outlined "URL" button in the modal, but that seems unnecessary / broken. I think the references to "Views" needs to be more consistent. It seems like the title of the second column of the views table should be changed to "Neuroglancer View". |
|
One note: When using our existing link shortener, I very very frequently make tweaks to my neuroglancer scene and then overwrite it. IIUC, both PRs have flaws:
In either case, it might be nice to alert the user when overwriting an existing link, and ask for confirmation. That's slightly more complicated, though. |
|
I agree with Jody that the “+ New Link” button in the Claude implementation is easier to find. I couldn't find the Codex one at first. I also like calling the page “Neuroglancer Links” better than “Views”. To Stuart's point above - in ngLinkQueries.tsx in the Claude implementation, the payload for the update mutation has the option to provide “state” but this hasn’t been implemented in the "edit" dialog. I do like the way the Codex UI allows you to toggle between pasting in a URL vs JSON state (via the outlined URL button). So I think that extending the Codex UI to the "edit" dialog and using the Claude update mutation would create the correct combination of being able to update the state in the link (also the short name, which is currently the only thing you can update for an NG link in the Claude version). A few other small notes: Codex
Claude
|
|
Thanks everyone! I think I rushed these out too quickly at the end of the year and both have major issues. I will concentrate on this PR as a starting point and get it fixed up. |
|
I think I've addressed all of the comments above, and made some other improvements besides:
|
|
@allison-truhlar Please review this commit in particular: allow search by URL in both Data Links and NG Links I took out the special URL handling because it didn't seem necessary for Data Links and it was interfering with NG Links. But maybe I missed something.. |
@krokicki The purpose of that code was to allow users to paste in the proxied path URL and have the data link table filter to the correct row. However, in looking at the code again, I found a better way to do this that doesn't break the Neuroglancer link searching. Now in the globalFilterFn, in the existing special handling case for the "File Path" column of the data link table, I include the proxied path URL in the array of values to search. See this commit: 5ba5088 |
|
@allison-truhlar Great idea on URL search. I just refactored it to keep TableCard generic. It now uses TanStack's column meta feature so that each column defines its own searchable values. This keeps domain-specific logic (like ProxiedPath) out of the shared component and makes it easy to extend for other tables. One thing I noticed when testing this: the Data Links table doesn't respect the user's path display setting when showing paths. Is that possible to do? |
| } | ||
| placeholder="https://neuroglancer-demo.appspot.com/#!{...}" | ||
| type="text" | ||
| value={neuroglancerUrl} |
There was a problem hiding this comment.
In edit mode, I'm not sure neuroglancerUrl is the correct value to pre-populate in this input. I think it should be left with only the placeholder text, or maybe pre-populated with the original Neuroglancer URL with state pasted in when the item was created. However, I don't think this is currently being saved. When I look at the editItem object, it does not have this information.
| className="text-foreground font-semibold" | ||
| htmlFor="neuroglancer-url" | ||
| > | ||
| Original Neuroglancer Link |
There was a problem hiding this comment.
In edit mode, I think I would change this title to something like "Neuroglancer link with updated state" or "Updated Neuroglancer link".
| state?: Record<string, unknown>; | ||
| url_base?: string; | ||
| short_name?: string; | ||
| short_key?: string; |
There was a problem hiding this comment.
I think short_key should be removed from this type, since the user does not submit a key to create the link, and it caused confusion for me when trying to understand why I could submit a short_name with non-alphanumeric characters and it wasn't being correctly validated by the backend.
| id="short-name" | ||
| onChange={(e: ChangeEvent<HTMLInputElement>) => | ||
| setShortName(e.target.value) | ||
| } |
There was a problem hiding this comment.
Could add some validation here to match the server validation - only alphanumeric characters, underscores, and hyphens. See NewFolderButton.tsx for validation example.
Could also add validation for the Neuroglancer URL value to match the server validation.
| short_key = _normalize_short_key(payload.short_key) | ||
| if short_key: | ||
| _validate_short_key(short_key) | ||
| short_name = payload.short_name.strip() if payload.short_name else None |
There was a problem hiding this comment.
short_name should also be validated for only alphanumeric characters and underscores and hyphens - I created a short name with a % character in it and resulting link does not work.
I also question whether short_key should be included in the NeuroglancerShortenRequest model - we do not provide the option for users to submit their own keys in the UI. Is there a possibility of adding this in the future? If we keep it, I suggest changing it to just "key" - I find having both a short_name and short_key a bit confusing - I thought short_key was referring to the user inputted short_name at first.
| <Route | ||
| element={ | ||
| <RequireAuth> | ||
| <NGLinks /> |
There was a problem hiding this comment.
<NgLinkProvider> could wrap NGLinks here, rather than being initialized higher up in the component tree at MainLayout.tsx. In this PR, the NGLink data is only used in the NGLinks component. We can move the provider higher up later, if we end up using it Neuroglancer links in the file browser.
| </TicketProvider> | ||
| </NotificationProvider> | ||
| </ProfileContextProvider> | ||
| <NGLinkProvider> |
There was a problem hiding this comment.
See other comment - this could be moved lower in the component tree to only wrap the NGLinks page component.
|
@krokicki In my testing, it looks like the Data Links table is respecting the user's path preferences. I checked both the displayed path and the path that gets copied from the Action menu for all three path preferences, and both seemed to be correct for me. Here's where the displayed path gets set based on preference, and here's where the path for copying gets set based on preference. |
Clickup id: 86adpkkgt
Implements Neuroglancer short links a la the FlyEM Shortener.
There is also a version of this PR implemented by Claude (#278), for comparison purposes. I think I prefer this one because it has simpler code, more features, and I like the UI better. But it is not reactive and needs some work.
@JaneliaSciComp/fileglancer @stuarteberg @StephanPreibisch