[WEB-4311] fix: membership data handling and state reversal on error#7205
[WEB-4311] fix: membership data handling and state reversal on error#7205sriramveeraghanta merged 1 commit intopreviewfrom
Conversation
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
WalkthroughThe changes introduce shallow copying of member data before updates in both project and workspace member stores to prevent unintended mutations. Additionally, a new computed function for retrieving sorted workspace member IDs is added, and an existing getter is refactored to use this function for improved code reuse and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant WorkspaceMemberStore
Caller->>WorkspaceMemberStore: getWorkspaceMemberIds(workspaceSlug)
WorkspaceMemberStore->>WorkspaceMemberStore: Compute member IDs (exclude bots, prioritize current user)
WorkspaceMemberStore-->>Caller: Sorted member ID list
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
web/core/store/member/base-project-member.store.ts (1)
305-320:⚠️ Potential issueRollback misses several fields & may leave stale state for non-current users.
projectUserInfois reverted only for the current user.
For every other user an optimistic write is still present after a failure, so UIs that rely onprojectUserInfowill keep showing the wrong role.
workspaceProjectsPermissionsis reset withmembershipBeforeUpdate.original_role, not with the previously captured permission (permissionBeforeUpdate).
original_rolecan beundefined(e.g. first update), which would wipe the permission entry.Diff suggestion:
-const permissionBeforeUpdate = isCurrentUser - ? this.rootStore.user.permission.getProjectRoleByWorkspaceSlugAndProjectId(workspaceSlug, projectId) - : undefined; +const permissionBeforeUpdate = + this.rootStore.user.permission.getProjectRoleByWorkspaceSlugAndProjectId( + workspaceSlug, + projectId, + ); @@ - if (isCurrentUser) { - set( - this.rootStore.user.permission.workspaceProjectsPermissions, - [workspaceSlug, projectId], - membershipBeforeUpdate?.original_role - ); - set( - this.rootStore.user.permission.projectUserInfo, - [workspaceSlug, projectId, "role"], - permissionBeforeUpdate - ); - } + // revert workspace-level permission only for current user + if (isCurrentUser) { + set( + this.rootStore.user.permission.workspaceProjectsPermissions, + [workspaceSlug, projectId], + permissionBeforeUpdate, + ); + } + // projectUserInfo should be reverted for **all** users + set( + this.rootStore.user.permission.projectUserInfo, + [workspaceSlug, projectId, "role"], + permissionBeforeUpdate, + );
🧹 Nitpick comments (1)
web/core/store/member/workspace-member.store.ts (1)
226-236: Variable name & rollback typo reduce clarity
originalProjectMemberDatadoesn’t match the workspace context and is referenced in the catch block. Rename to avoid confusion and ensure the rollback uses the renamed variable.-const originalProjectMemberData = { ...this.workspaceMemberMap?.[workspaceSlug]?.[userId] }; +const originalWorkspaceMemberData = { ...this.workspaceMemberMap?.[workspaceSlug]?.[userId] }; ... - set(this.workspaceMemberMap, [workspaceSlug, userId], originalProjectMemberData); + set(this.workspaceMemberMap, [workspaceSlug, userId], originalWorkspaceMemberData);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/core/store/member/base-project-member.store.ts(1 hunks)web/core/store/member/workspace-member.store.ts(4 hunks)
🔇 Additional comments (3)
web/core/store/member/base-project-member.store.ts (1)
276-277: 👍 Good call on keeping an immutable snapshot of the membership.A shallow spread copy of the original membership lets you roll back fast without leaking proxies from MobX.
LGTM.web/core/store/member/workspace-member.store.ts (2)
97-101: Empty array vs.null– changed semantics, verify callers.
workspaceMemberIdsnow returns[]when a slug is present but no members exist, whereas it previously returnednull.
If any consumers relied on the oldnullcheck (if (!ids) …) they will now treat “no members” as “members loaded”, which can break loading spinners or pagination logic.
Please audit the usages.
115-124: Nice reuse & bot filtering.Extracting the logic into
getWorkspaceMemberIdsand filtering out bots declutters the getter and is consistent with the project-member store.
* refactor: move web utils to packages * fix: build and lint errors * chore: update drag handle plugin * chore: update table cell type to fix build errors * fix: build errors * chore: sync few changes * feat: add pi base url to constants * chore: update all util imports in ee folder * chore: refactor web utils imports * chore: update imports * fix: build errors * fix: build errors * fix: update utils import * chore: minor fixes related to duplicate assets imports * chore: update duplicate assets service * [WEB-4316] chore: new endpoints to download an asset (#7207) * chore: new endpoints to download an asset * chore: add exception handling * [WEB-4323] refactor: Analytics refactor (#7213) * chore: updated label for epics * chore: improved export logic * refactor: move csvConfig to export.ts and clean up export logic * refactor: remove unused CSV export logic from WorkItemsInsightTable component * refactor: streamline data handling in InsightTable component for improved rendering * feat: add translation for "No. of {entity}" and update priority chart y-axis label to use new translation * refactor: cleaned up some component and added utilitites * feat: add "at_risk" translation to multiple languages in translations.json files * refactor: update TrendPiece component to use new status variants for analytics * fix: adjust TrendPiece component logic for on-track and off-track status * refactor: use nullish coalescing operator for yAxis.dx in line and scatter charts * feat: add "at_risk" translation to various languages in translations.json files * feat: add "no_of" translation to various languages in translations.json files * feat: update "at_risk" translation in Ukrainian, Vietnamese, and Chinese locales in translations.json files * refactor: rename insightsFields to ANALYTICS_INSIGHTS_FIELDS and update analytics tab import to use getAnalyticsTabs function * feat: update AnalyticsWrapper to use i18n for titles and add new translation for "no_of" in Russian * fix: update yAxis labels and offsets in various charts to use new translation key and improve layout * feat: define AnalyticsTab interface and refactor getAnalyticsTabs function for improved type safety * fix: update AnalyticsTab interface to use TAnalyticsTabsBase for improved type safety * fix: add whitespace-nowrap class to TableHead for improved header layout in DataTable component * [WEB-4311] fix: membership data handling and state reversal on error (#7205) * [WEB-4231] Pie chart tooltip #7192 * fix: build errors * fix: utils imports * chore: fix build errors * yarn lock file update --------- Co-authored-by: Aaryan Khandelwal <65252264+aaryan610@users.noreply.github.com> Co-authored-by: JayashTripathy <76092296+JayashTripathy@users.noreply.github.com> Co-authored-by: sriram veeraghanta <veeraghanta.sriram@gmail.com>
Description
Type of Change
Summary by CodeRabbit