-
Notifications
You must be signed in to change notification settings - Fork 11
Fix 'Send to Workspace' to work with new Sequencing Workspaces #1809
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
base: develop
Are you sure you want to change the base?
Conversation
| const dispatch = createEventDispatcher<{ | ||
| close: void; | ||
| save: { parcelId: number; workspaceId: number }; | ||
| save: { workspaceId: number; workspaceName: string }; |
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.
Do you need to pass the workspace name in this save event? It doesn't look like you're using it. I think ideally, the workspaceId should be enough.
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.
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.
Maybe I'm not fully understanding the flow of this. My comment is specifically for the save event. Your modal.ts change defines the save event type to only have a workspaceId so the types are currently mismatched.
'save', (e: CustomEvent<{ workspaceId: number }>)
As for the undefined workspace name, I think that's a different issue. The save event is dispatched after the modal has already been opened and rendered, so removing workspaceName from the event theoretically should be unrelated.
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.
Ah I'm following - so the save event fires from ExpansionPanelModal which allows the user to select a workspace from a dropdown and then that ID (and currently name) are passed to a WorkspaceTreeNode which is then used to make the NewWorkspaceSequenceModal (shown in the screenshot). I think the only reason the name is included in the save is so that the name is shown in that second modal when the directory tree is drawn
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.
Gooot it. Okay I think see it now. I think then that this line needs to be updated to also include the workspaceName as part of the event payload.
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.
Done!
duranb
left a comment
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.
Do you also mind adding permission checks to the workspace dropdown or submit button for if the user even has access to the workspace that they want to send the sequence to? There is a featurePermission.workspace() in utilities/permissions.ts that you can use to see if the user can upload to the selected workspace.
5bf6a6c to
27d9587
Compare
c7c353e to
149ea13
Compare

This pull-request updates the 'Send to Workspace' feature in plan expansion to let the user create a new file in a sequencing workspace loaded with the expansion result of an expanded sequence.
Screen.Recording.2025-10-16.at.11.17.30.AM.mov