[WEB-4335] improvement: optimize assignee grouping with improved member scope handling#7227
Conversation
…er scope handling
WalkthroughA new type alias Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Utils as getScopeMemberIds
participant Store
participant ProjectUtils as getProjectMemberIds
Caller->>Utils: getScopeMemberIds({ isWorkspaceLevel, projectId })
Utils->>Store: Fetch workspaceMemberIds, projectMemberIds
alt isWorkspaceLevel
Utils-->>Caller: Return workspace memberIds, includeNone: true
else projectId provided or projectMemberIds exist
Utils->>ProjectUtils: getProjectMemberIds(projectId)
ProjectUtils-->>Utils: Return project memberIds
Utils-->>Caller: Return project memberIds, includeNone: true
else
Utils-->>Caller: Return [], includeNone: true
end
sequenceDiagram
participant Caller
participant getAssigneeColumns
participant Utils as getScopeMemberIds
participant Store
participant Users
Caller->>getAssigneeColumns: getAssigneeColumns({ isWorkspaceLevel, projectId })
getAssigneeColumns->>Utils: getScopeMemberIds({ isWorkspaceLevel, projectId })
Utils-->>getAssigneeColumns: { memberIds, includeNone }
alt memberIds not empty
loop For each memberId
getAssigneeColumns->>Users: getUserById(memberId)
Users-->>getAssigneeColumns: user
getAssigneeColumns-->>getAssigneeColumns: Build column if user exists
end
end
alt includeNone
getAssigneeColumns-->>getAssigneeColumns: Add "None" column
end
getAssigneeColumns-->>Caller: Return columns
Suggested labels
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 (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/types/src/issues.d.ts (1)
223-226: Use a clearer name / add JSDoc
TGetColumnsactually models “scope options” (isWorkspaceLevel,projectId).
Something likeTScopeOptionsplus a short JSDoc block would make intent obvious to downstream consumers and type-hint tooltips.web/ce/components/issues/issue-layouts/utils.tsx (1)
55-58: Inline single-use variable
memberIdsis only used inside the workspace branch – inlining avoids an unnecessary alias:- const memberIds = workspaceMemberIds; - if (isWorkspaceLevel) { - return { memberIds: memberIds ?? [], includeNone: true }; + if (isWorkspaceLevel) { + return { memberIds: workspaceMemberIds ?? [], includeNone: true };web/core/components/issues/issue-layouts/utils.tsx (2)
40-44: Core now depends on CE utilities ‑ consider relocating shared helpers
web/coreimports from@/plane-web/components/...(the CE layer).
This reverses the normal dependency direction and may cause circular-bundle issues in Next.js.
MovegetScopeMemberIds,SpreadSheetPropertyIconMap, etc. to a neutral package (web/sharedor@plane/utils) and let both CE/Core import from there.
68-76: Remove unnecessary ternary (Biome warning)
includes()already returns a boolean:- ].includes(type) - ? true - : false; + ].includes(type);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/types/src/issues.d.ts(1 hunks)web/ce/components/issues/issue-layouts/utils.tsx(2 hunks)web/core/components/issues/issue-layouts/utils.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
web/ce/components/issues/issue-layouts/utils.tsx (4)
web/core/lib/store-context.tsx (1)
store(18-18)web/core/store/member/workspace-member.store.ts (1)
workspaceMemberIds(96-101)web/core/store/router.store.ts (1)
projectId(85-87)web/core/store/member/base-project-member.store.ts (1)
projectMemberIds(105-116)
web/core/components/issues/issue-layouts/utils.tsx (4)
web/core/lib/store-context.tsx (1)
store(18-18)web/ce/components/issues/issue-layouts/utils.tsx (1)
getScopeMemberIds(43-64)packages/types/src/issues.d.ts (1)
IGroupByColumn(228-235)packages/ui/src/avatar/avatar.tsx (1)
Avatar(116-169)
🪛 Biome (1.9.4)
web/core/components/issues/issue-layouts/utils.tsx
[error] 68-76: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
web/ce/components/issues/issue-layouts/utils.tsx (1)
38-64:includeNoneis hard-coded totrue– verify behaviour changeEvery return path sets
includeNone: true, so callers can no longer suppress the “None” column.
If the previous implementation allowedfalse, this is a breaking change.
Please confirm all usages (e.g.getAssigneeColumns) really expect this new invariant; otherwise re-introduce conditional logic or remove the flag entirely.
| const getAssigneeColumns = ({ isWorkspaceLevel, projectId }: TGetColumns): IGroupByColumn[] | undefined => { | ||
| // store values | ||
| const { getUserDetails } = store.memberRoot; | ||
| // derived values | ||
| const { memberIds, includeNone } = getScopeMemberIds({ isWorkspaceLevel, projectId }); | ||
| const assigneeColumns: IGroupByColumn[] = []; | ||
| const { | ||
| project: { projectMemberIds, getProjectMemberIds }, | ||
| getUserDetails, | ||
| } = store.memberRoot; | ||
| // if workspace level | ||
| if (isWorkspaceLevel) { | ||
| const { workspaceMemberIds } = store.memberRoot.workspace; | ||
| if (!workspaceMemberIds) return; | ||
| workspaceMemberIds.forEach((memberId) => { | ||
| const member = getUserDetails(memberId); | ||
| assigneeColumns.push({ | ||
| id: memberId, | ||
| name: member?.display_name || "", | ||
| icon: <Avatar name={member?.display_name} src={getFileURL(member?.avatar_url ?? "")} size="md" />, | ||
| payload: { assignee_ids: [memberId] }, | ||
| }); | ||
| }); | ||
| } else { | ||
| // if project level | ||
| const _projectMemberIds = projectId ? getProjectMemberIds(projectId, false) : projectMemberIds; | ||
| if (!_projectMemberIds) return; | ||
| // Map project member ids to group by assignee columns | ||
| _projectMemberIds.forEach((memberId) => { | ||
| const member = getUserDetails(memberId); | ||
| assigneeColumns.push({ | ||
| id: memberId, | ||
| name: member?.display_name || "", | ||
| icon: <Avatar name={member?.display_name} src={getFileURL(member?.avatar_url ?? "")} size="md" />, | ||
| payload: { assignee_ids: [memberId] }, | ||
| }); | ||
|
|
||
| if (!memberIds) return []; | ||
|
|
||
| memberIds.forEach((memberId) => { | ||
| const member = getUserDetails(memberId); | ||
| if (!member) return; | ||
| assigneeColumns.push({ | ||
| id: memberId, | ||
| name: member?.display_name || "", | ||
| icon: <Avatar name={member?.display_name} src={getFileURL(member?.avatar_url ?? "")} size="md" />, | ||
| payload: { assignee_ids: [memberId] }, | ||
| }); | ||
| }); | ||
| if (includeNone) { | ||
| assigneeColumns.push({ id: "None", name: "None", icon: <Avatar size="md" />, payload: {} }); | ||
| } | ||
| assigneeColumns.push({ id: "None", name: "None", icon: <Avatar size="md" />, payload: {} }); | ||
|
|
||
| return assigneeColumns; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard for empty member list is ineffective
memberIds is always an array (helper guarantees at least []), so
if (!memberIds) return []; never triggers. Early-return should test length:
- if (!memberIds) return [];
+ if (memberIds.length === 0) {
+ return includeNone ? [{ id: "None", name: "None", icon: <Avatar size="md" />, payload: {} }] : [];
+ }Ensures we skip the loop when there are no members and prevents unnecessary computation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getAssigneeColumns = ({ isWorkspaceLevel, projectId }: TGetColumns): IGroupByColumn[] | undefined => { | |
| // store values | |
| const { getUserDetails } = store.memberRoot; | |
| // derived values | |
| const { memberIds, includeNone } = getScopeMemberIds({ isWorkspaceLevel, projectId }); | |
| const assigneeColumns: IGroupByColumn[] = []; | |
| const { | |
| project: { projectMemberIds, getProjectMemberIds }, | |
| getUserDetails, | |
| } = store.memberRoot; | |
| // if workspace level | |
| if (isWorkspaceLevel) { | |
| const { workspaceMemberIds } = store.memberRoot.workspace; | |
| if (!workspaceMemberIds) return; | |
| workspaceMemberIds.forEach((memberId) => { | |
| const member = getUserDetails(memberId); | |
| assigneeColumns.push({ | |
| id: memberId, | |
| name: member?.display_name || "", | |
| icon: <Avatar name={member?.display_name} src={getFileURL(member?.avatar_url ?? "")} size="md" />, | |
| payload: { assignee_ids: [memberId] }, | |
| }); | |
| }); | |
| } else { | |
| // if project level | |
| const _projectMemberIds = projectId ? getProjectMemberIds(projectId, false) : projectMemberIds; | |
| if (!_projectMemberIds) return; | |
| // Map project member ids to group by assignee columns | |
| _projectMemberIds.forEach((memberId) => { | |
| const member = getUserDetails(memberId); | |
| assigneeColumns.push({ | |
| id: memberId, | |
| name: member?.display_name || "", | |
| icon: <Avatar name={member?.display_name} src={getFileURL(member?.avatar_url ?? "")} size="md" />, | |
| payload: { assignee_ids: [memberId] }, | |
| }); | |
| if (!memberIds) return []; | |
| memberIds.forEach((memberId) => { | |
| const member = getUserDetails(memberId); | |
| if (!member) return; | |
| assigneeColumns.push({ | |
| id: memberId, | |
| name: member?.display_name || "", | |
| icon: <Avatar name={member?.display_name} src={getFileURL(member?.avatar_url ?? "")} size="md" />, | |
| payload: { assignee_ids: [memberId] }, | |
| }); | |
| }); | |
| if (includeNone) { | |
| assigneeColumns.push({ id: "None", name: "None", icon: <Avatar size="md" />, payload: {} }); | |
| } | |
| assigneeColumns.push({ id: "None", name: "None", icon: <Avatar size="md" />, payload: {} }); | |
| return assigneeColumns; | |
| const getAssigneeColumns = ({ isWorkspaceLevel, projectId }: TGetColumns): IGroupByColumn[] | undefined => { | |
| // store values | |
| const { getUserDetails } = store.memberRoot; | |
| // derived values | |
| const { memberIds, includeNone } = getScopeMemberIds({ isWorkspaceLevel, projectId }); | |
| const assigneeColumns: IGroupByColumn[] = []; | |
| if (memberIds.length === 0) { | |
| return includeNone | |
| ? [{ id: "None", name: "None", icon: <Avatar size="md" />, payload: {} }] | |
| : []; | |
| } | |
| memberIds.forEach((memberId) => { | |
| const member = getUserDetails(memberId); | |
| if (!member) return; | |
| assigneeColumns.push({ | |
| id: memberId, | |
| name: member.display_name || "", | |
| icon: ( | |
| <Avatar | |
| name={member.display_name} | |
| src={getFileURL(member.avatar_url ?? "")} | |
| size="md" | |
| /> | |
| ), | |
| payload: { assignee_ids: [memberId] }, | |
| }); | |
| }); | |
| if (includeNone) { | |
| assigneeColumns.push({ | |
| id: "None", | |
| name: "None", | |
| icon: <Avatar size="md" />, | |
| payload: {}, | |
| }); | |
| } | |
| return assigneeColumns; | |
| }; |
🤖 Prompt for AI Agents
In web/core/components/issues/issue-layouts/utils.tsx around lines 273 to 296,
the guard condition checking if memberIds is falsy is ineffective because
memberIds is always an array. Replace the condition `if (!memberIds) return [];`
with a check for an empty array using `if (memberIds.length === 0) return [];`
to properly skip processing when there are no members.
…er scope handling (#7227)
2 participants
Description
Type of Change
Summary by CodeRabbit