(fix) - O3-4349 - open workspace in declared workspace group #1266
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -142 kB (-2.24%) Total Size: 6.19 MB
ℹ️ View Unchanged
|
vasharma05
reviewed
Jan 15, 2025
| @@ -301,7 +301,7 @@ export function launchWorkspace< | |||
| const workspace = getWorkspaceRegistration(name); | |||
| const currentWorkspaceGroup = store.getState().workspaceGroup; | |||
|
|
|||
| if (currentWorkspaceGroup && !currentWorkspaceGroup.members?.includes(name)) { | |||
| if (currentWorkspaceGroup && !(currentWorkspaceGroup.members?.includes(name) || workspace?.groups.includes(currentWorkspaceGroup.name))) { | |||
There was a problem hiding this comment.
Hi @chibongho, instead of adding conditions here, we have all the members of a workspaceGroup in the workspaceGroup store and the groups adds the given workspace to the members of a workspaceGroup at this place. Hence all the workspaces are in the end added in the to the workspaceGroup's members.
One thing that might be happening is that there might be a race condition going on in the above function, hence the group's definition might be overrided. This is something which must be looked into.
There was a problem hiding this comment.
@vasharma05 - is this something you are able to fix? Do you know where the proper fix is? @ibacher ? This is trying to fix a regression in our system that has been broken for the last few days and arose (we think) following this commit: a61deb8 . @ibacher any guidance appreciated.
There was a problem hiding this comment.
Yes @mseaton, I made the change. Let me have a look at it.
Thanks!
There was a problem hiding this comment.
@chibongho, we connected on a meeting, and it was working well at that time, right?
There was a problem hiding this comment.
@chibongho is not 100% available today, but he and I met about this and were able to reproduce it locally as well as on our testing server. I'm not sure why it may have appeared working at the time, but it is an issue for both of us now @vasharma05
There was a problem hiding this comment.
Looking into it @mseaton, will there be a way that I can use your implementation to reproduce the issue?
There was a problem hiding this comment.
@mseaton @chibongho, it's the race condition between the workspaceGroup registration by the ward-app and the custom workspace's group property.
Correction: It's not a race condition, actually the registerWorkspaceGroup overrides the already registered workspace group and it's members.
There was a problem hiding this comment.
@vasharma05 - shouldn't your PR remove this change made by @chibongho , if your follow-up change fixes things?
There was a problem hiding this comment.
Yes @mseaton, pushed the commit.
…roup is registered beforehand
UpdateThe workspace registration adds a new entry in the Workspace registering before the workspace group is registered
Screen.Recording.2025-01-15.at.22.40.06.movP.S. The change done by @chibongho was reverted before the above flow is recorded. |
|
@ibacher, requesting your review here. |
mseaton
reviewed
Jan 15, 2025
| @@ -164,28 +182,8 @@ function createNewWorkspaceGroupInfo(groupName: string): WorkspaceGroupRegistrat | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The function above this is no longer used or needed, right @vasharma05 - createNewWorkspaceGroupInfo ?
| @@ -164,28 +182,8 @@ function createNewWorkspaceGroupInfo(groupName: string): WorkspaceGroupRegistrat | |||
| } | |||
|
|
|||
| export function attachWorkspaceToGroup(workspaceName: string, groupName: string) { | |||
There was a problem hiding this comment.
Do we even need this new function now? All it does is delegate to another existing function with basically the same arguments, right?
Couldn't any code that calls attachWorkspaceToGroup(workspaceName, groupName) instead just call registerWorkspaceGroup({name: groupName, members: [workspaceName]}); ?
|
I think this is good to go to merge. Any objections @ibacher ? |
ibacher
approved these changes
Jan 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
We want to support both:
I think there was a regression with this commit that dropped support for 2. This PR adds it back.
Screenshots
"Visit Summary" is a workspace that we define in a PIH custom ESM. It declares that it belongs to the
ward-patientworkspace group.Before:
(notice the workspace action menu icons fail to load)
After:
Related Issue
Other
I don't know why, but as seen in the screenshot, the content of the workspace is blank when I run
yarn run:shellfromesm-core. It seems not related to this change. It also doesn't seem to be a real issue in dev3, or when runningyarn start --sources esm-ward-appin patient-management locally