Hide suspended members in assignee dropdown #8001
Conversation
WalkthroughThe changes extend the member dropdown component to accept a new Changes
Sequence DiagramsequenceDiagram
participant Base as MemberDropdownBase
participant Options as MemberOptions
participant UI as Render
Base->>Base: Derive selectedMemberIds from value
Base->>Options: Pass selectedMemberIds prop
Options->>Options: Create Set<selectedMemberIds> for O(1) lookup
Options->>Options: Map member list, compute isSuspended flag
Options->>Options: Filter: remove suspended (except if in selectedMemberIdsSet)
Options->>Options: Prioritize: sort selected members to top
Options->>UI: Render with updated option.isSuspended & disabled states
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes The changes span two related files with new filtering logic, Set-based optimization, type-guarding improvements, and prioritization sorting. While individual pieces are straightforward, the combination of suspension handling, conditional inclusion, and list reordering requires careful verification of the filtering and sorting behavior. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR improves the member selection dropdown by hiding suspended members (unless already assigned) and prioritizing selected members at the top of the list for easier access.
Key Changes:
- Hide suspended members from dropdown unless they are currently assigned
- Sort selected members to appear first in the dropdown list
- Pass selected member IDs to enable filtering and ordering logic
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/web/core/components/dropdowns/member/member-options.tsx | Implements filtering of suspended members, adds sorting to prioritize selected members, and optimizes repeated isUserSuspended calls |
| apps/web/core/components/dropdowns/member/base.tsx | Passes selected member IDs to the options component |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ) | ||
| } | ||
| disabled={isUserSuspended(option.value, workspaceSlug?.toString())} | ||
| disabled={option.isSuspended && !selectedMemberIdsSet.has(option.value)} |
There was a problem hiding this comment.
[nitpick] The disabled condition uses selectedMemberIdsSet.has(option.value) while line 173 uses !selected. These should be consistent - consider using the selected parameter from the render prop instead of rechecking the Set for better maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/core/components/dropdowns/member/member-options.tsx (2)
122-128: LGTM! Selected members correctly prioritized.The sort logic correctly surfaces selected members at the top of the list while maintaining stable ordering within each group (selected vs. unselected).
For improved readability, consider extracting the comparator:
+const prioritizeSelected = (a: typeof options[number], b: typeof options[number]) => { + const isASelected = selectedMemberIdsSet.has(a.value); + const isBSelected = selectedMemberIdsSet.has(b.value); + if (isASelected === isBSelected) return 0; + return isASelected ? -1 : 1; +}; + - ?.sort((a, b) => { - const isASelected = selectedMemberIdsSet.has(a.value); - const isBSelected = selectedMemberIdsSet.has(b.value); - - if (isASelected === isBSelected) return 0; - return isASelected ? -1 : 1; - }); + ?.sort(prioritizeSelected);
173-173: Verify consistency betweenselectedandselectedMemberIdsSet.Line 173 uses the
selectedprop from Headless UI's render function, while line 176 usesselectedMemberIdsSet.has(option.value). These should be equivalent since both derive from the parent'svalueprop, but using different sources for related decisions could introduce subtle bugs if the Combobox state updates independently.For consistency and to eliminate any risk of state desynchronization, consider using the same check in both places.
Apply this diff to use a consistent check:
className={({ active, selected }) => cn( "flex w-full select-none items-center justify-between gap-2 truncate rounded px-1 py-1.5", active && "bg-custom-background-80", selected ? "text-custom-text-100" : "text-custom-text-200", - option.isSuspended && !selected ? "cursor-not-allowed" : "cursor-pointer" + option.isSuspended && !selectedMemberIdsSet.has(option.value) ? "cursor-not-allowed" : "cursor-pointer" ) } disabled={option.isSuspended && !selectedMemberIdsSet.has(option.value)}Also applies to: 176-176
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/core/components/dropdowns/member/base.tsx(1 hunks)apps/web/core/components/dropdowns/member/member-options.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/core/components/dropdowns/member/member-options.tsx (1)
packages/propel/src/icons/suspended-user.tsx (1)
SuspendedUserIcon(5-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and lint web apps
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
apps/web/core/components/dropdowns/member/base.tsx (1)
180-180: LGTM! Clean value normalization.The ternary chain correctly normalizes the
valueprop into an array format thatMemberOptionsexpects, handling both single and multiple selection modes gracefully.apps/web/core/components/dropdowns/member/member-options.tsx (5)
32-32: LGTM! Efficient prop handling and lookup structure.The optional
selectedMemberIdsprop with a default empty array is well-typed, and converting it to a Set enables O(1) lookups throughout the filtering and rendering logic.Also applies to: 44-44, 90-90
94-96: LGTM! Filtering suspended members as intended.The logic correctly hides suspended users unless they're already selected, precisely matching the PR objective. The early return is efficient and the Set lookup is O(1).
105-118: LGTM! Consistent visual treatment for suspended users.The suspended state is correctly reflected in the UI through icon substitution, dimmed text color, and propagation to the option object for downstream rendering decisions.
121-121: LGTM! Proper type narrowing.The type guard correctly removes nulls introduced by the suspension filter and strengthens type safety for the subsequent sort and render operations.
182-186: LGTM! Clear visual indicator for suspended state.The "Suspended" pill provides clear feedback to users about member status, enhancing the user experience when reviewing assigned members.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68fa0d42d260832a88154355df1f4f58
Summary by CodeRabbit