Skip to content

[WEB-2443] fix: join project flicker #5602

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.

Sign up for GitHub

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

Merged
pushya22 merged 2 commits into from
Sep 16, 2024
Merged

[WEB-2443] fix: join project flicker #5602

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8831ce1
fix: join project flicker
anmolsinghbhatia Sep 13, 2024
6444cbd
fix: leave project project mutation and code refactor
anmolsinghbhatia Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions web/core/layouts/auth-layout/project-wrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,12 @@ export const ProjectAuthWrapper: FC<IProjectAuthWrapper> = observer((props) => {

// derived values
const projectExists = projectId ? getProjectById(projectId.toString()) : null;
const hasPermissionToCurrentProject = projectId
? allowPermissions(
[EUserPermissions.ADMIN, EUserPermissions.MEMBER, EUserPermissions.GUEST],
EUserPermissionsLevel.PROJECT
)
: undefined;
const hasPermissionToCurrentProject = allowPermissions(
[EUserPermissions.ADMIN, EUserPermissions.MEMBER, EUserPermissions.GUEST],
EUserPermissionsLevel.PROJECT,
workspaceSlug.toString(),
projectId.toString()
);

// check if the project member apis is loading
if (!projectMemberInfo && projectId && hasPermissionToCurrentProject === null)
Expand Down
6 changes: 5 additions & 1 deletion web/core/store/user/permissions.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,11 @@ export class UserPermissionStore implements IUserPermissionStore {
const response = await userService.joinProject(workspaceSlug, [projectId]);
if (response) {
runInAction(() => {
set(this.workspaceProjectsPermissions, [workspaceSlug, projectId], response);
set(
this.workspaceProjectsPermissions,
[workspaceSlug, projectId],
this.workspaceInfoBySlug(workspaceSlug)?.role
);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

Review and update permission setting logic for consistency

The usage of workspaceInfoBySlug in the changed code is consistent with its usage elsewhere in the codebase. However, the way workspaceProjectsPermissions is being set appears to be inconsistent with other occurrences in the same file.

Consider the following:

  1. In the changed code (line 251), you're using this.workspaceInfoBySlug(workspaceSlug)?.role.
  2. In other parts of the file (e.g., line 209), it's set directly with response.role.

To ensure consistency and robustness:

  1. Review other occurrences of setting workspaceProjectsPermissions and update them if necessary to align with this new approach.
  2. Add error handling for cases where workspaceInfoBySlug might return undefined. For example:
const role = this.workspaceInfoBySlug(workspaceSlug)?.role;
if (role) {
  set(this.workspaceProjectsPermissions, [workspaceSlug, projectId], role);
} else {
  console.error(`Failed to get role for workspace: ${workspaceSlug}`);
  // Consider how to handle this case - maybe set a default role or throw an error
}
  1. Ensure that this change is part of a deliberate shift from using direct API responses to using workspace roles for permissions, and update related documentation if necessary.
Analysis chain

Verify the consistency of the change and add error handling.

The change shifts from using the service response directly to utilizing a derived value (the role) from the workspace information. This may affect how permissions are managed and interpreted within the application.

Please verify if this change is consistent with the application's permission management logic by running the following script:

Also, the change seems to be missing error handling for the case when workspaceInfoBySlug returns undefined.

Consider adding error handling like this:

  runInAction(() => {
+   const role = this.workspaceInfoBySlug(workspaceSlug)?.role;
+   if (role) {
      set(
        this.workspaceProjectsPermissions,
        [workspaceSlug, projectId],
-       this.workspaceInfoBySlug(workspaceSlug)?.role
+       role
      );
+   } else {
+     console.error(`Failed to get role for workspace: ${workspaceSlug}`);
+   }
  });
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the consistency of the change with the application's permission management logic.

# Test 1: Search for other occurrences of setting `workspaceProjectsPermissions` state. 
# Expect: The logic should be consistent with the change.
rg --type typescript -A 5 $'set\(this\.workspaceProjectsPermissions'

# Test 2: Search for usage of `workspaceInfoBySlug` method. 
# Expect: The return value should be consistently used and handled.
rg --type typescript -A 5 $'workspaceInfoBySlug'

Length of output: 187

Script:

#!/bin/bash
# Description: Verify the consistency of the change with the application's permission management logic using generic search patterns.

# Find TypeScript files and search for setting workspaceProjectsPermissions
fd -e ts -e tsx | xargs grep -n -A 5 "set(this.workspaceProjectsPermissions"

# Find TypeScript files and search for usage of workspaceInfoBySlug method
fd -e ts -e tsx | xargs grep -n -A 5 "workspaceInfoBySlug"

Length of output: 8744

});
}
return response;
Expand Down