feat(#170): add remove member functionality#193
feat(#170): add remove member functionality#193iamitprakash merged 19 commits intoRealDevSquad:developfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdds team member removal and self-leave functionality: new TeamsApi methods (removeFromTeam, getUserRoles), team role enum/types, UI components for confirmation dialogs and buttons, role-based gating in team members and header, and activity-to-UI mappings for new activity types with query invalidations and navigation updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant H as Teams Header
participant LTB as LeaveTeamButton
participant LTD as LeaveTeamDialog
participant API as TeamsApi.removeFromTeam
participant QC as QueryClient
participant R as Router
participant T as Toast
U->>H: Open team view
H->>LTB: Render (if not owner/POC)
U->>LTB: Click "Leave Team"
LTB->>LTD: Open confirm dialog
U->>LTD: Confirm
LTD->>API: DELETE /teams/{teamId}/members/{userId}
API-->>LTD: 204/OK
LTD->>QC: Invalidate team/teams/tasks queries
LTD->>T: Show success
LTD->>R: Navigate /dashboard
API-->>LTD: Error
LTD->>T: Show error
sequenceDiagram
autonumber
actor A as Admin User
participant TM as Team Members List
participant LTD as LeaveTeamDialog
participant API as TeamsApi.removeFromTeam
participant QC as QueryClient
participant T as Toast
A->>TM: Open members
TM->>TM: Compute isAdmin via getUserRoles
A->>TM: Click "Remove from team" (eligible member)
TM->>LTD: Open confirm dialog
A->>LTD: Confirm
LTD->>API: DELETE /teams/{teamId}/members/{memberId}
API-->>LTD: 204/OK
LTD->>QC: Invalidate team/teams/tasks
LTD->>T: Show success
API-->>LTD: Error
LTD->>T: Show error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing touches🧪 Generate unit tests
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 |
coderabbitai
bot
added
the
feature task
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Loss of Error Context in Mutation Error Handler ▹ view | |
 |
Missing Removed Member Information in Activity Log ▹ view | |
 |
Insecure Role Validation ▹ view | ✅ Fix detected |
|
Missing Loading State ▹ view | ✅ Fix detected |
 |
Complex Conditional Logic in JSX ▹ view | ✅ Fix detected |
|
Missing actor in bulk task reassignment log ▹ view | |
 |
Cache invalidation logic needs abstraction ▹ view | |
|
Unhandled Submit Error State ▹ view | |
 |
Inefficient Query Invalidation Pattern ▹ view | ✅ Fix detected |
|
Inconsistent Container Usage ▹ view | ✅ Fix detected |
Files scanned
| File Path | Reviewed |
|---|---|
| api/teams/teams.enum.ts | ✅ |
| modules/teams/components/leave-team-dialog.tsx | ✅ |
| modules/teams/components/teams-layout-header.tsx | ✅ |
| api/teams/teams.api.ts | ✅ |
| components/leave-team-button.tsx | ✅ |
| api/teams/teams.type.ts | ✅ |
| lib/team-utils.ts | ✅ |
| modules/teams/team-members.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ queryKey: TeamsApi.getTeamById.key({ teamId }) }) | ||
| queryClient.invalidateQueries({ | ||
| queryKey: TeamsApi.getTeamById.key({ teamId, member: true }), | ||
| }) | ||
| queryClient.invalidateQueries({ | ||
| queryKey: TeamsApi.getTeams.key, | ||
| }) | ||
| queryClient.invalidateQueries({ | ||
| queryKey: TasksApi.getTasks.key(), | ||
| }) |
There was a problem hiding this comment.
Cache invalidation logic needs abstraction 
Tell me more
What is the issue?
The cache invalidation logic in the onSuccess callback is not abstracted into a separate function, making it hard to maintain and reuse.
Why this matters
Multiple cache invalidation calls scattered in the component increase complexity and make it difficult to maintain consistency across similar operations. If cache invalidation rules change, multiple components would need updates.
Suggested change ∙ Feature Preview
Extract cache invalidation into a separate utility function:
const invalidateTeamCaches = (queryClient: QueryClient, teamId: string) => {
queryClient.invalidateQueries({ queryKey: TeamsApi.getTeamById.key({ teamId }) })
queryClient.invalidateQueries({ queryKey: TeamsApi.getTeamById.key({ teamId, member: true }) })
queryClient.invalidateQueries({ queryKey: TeamsApi.getTeams.key })
queryClient.invalidateQueries({ queryKey: TasksApi.getTasks.key() })
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| onError: () => { | ||
| toast.error('Failed to leave team') | ||
| }, |
There was a problem hiding this comment.
Loss of Error Context in Mutation Error Handler 
Tell me more
What is the issue?
The error callback in the mutation only displays a generic error message without including the actual error details.
Why this matters
Without the actual error information, debugging issues becomes more difficult as developers won't know the specific reason for the failure.
Suggested change ∙ Feature Preview
onError: (error: Error) => {
toast.error(`Failed to leave team: ${error.message}`)
console.error('Leave team error:', error)
},Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| if (isAuthLoading) { | ||
| return null | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| }, | ||
| enabled: !!userId, | ||
| }) | ||
| const isAdmin = userRole?.roles.find((role) => role.role_name == TeamRoles.ADMIN) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| queryClient.invalidateQueries({ | ||
| queryKey: TeamsApi.getTeamById.key({ teamId, member: true }), | ||
| }) | ||
| queryClient.invalidateQueries({ | ||
| queryKey: TeamsApi.getTeams.key, | ||
| }) | ||
| queryClient.invalidateQueries({ | ||
| queryKey: TasksApi.getTasks.key(), | ||
| }) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
modules/teams/team-members.tsx
Outdated
| {member.id !== user.id && | ||
| member.id !== data?.created_by && | ||
| member.id !== data?.poc_id ? ( | ||
| <LeaveTeamDialog | ||
| title="Remove Member" | ||
| description="Are you sure you want to remove this member from the team? They will lose access to all tasks and team information." | ||
| buttonText="Remove Member" | ||
| open={activeDialogMemberId === member.id} | ||
| onOpenChange={(open) => { | ||
| if (open) { | ||
| setActiveDialogMemberId(member.id) | ||
| } else { | ||
| setActiveDialogMemberId(null) | ||
| } | ||
| }} | ||
| onSubmit={() => { | ||
| removeMemberMutation.mutate({ | ||
| teamId, | ||
| memberId: member.id, | ||
| }) | ||
| }} | ||
| > | ||
| <DropdownMenuItem | ||
| onSelect={(e) => { | ||
| e.preventDefault() | ||
| setActiveDialogMemberId(member.id) | ||
| }} | ||
| > | ||
| Remove from team | ||
| </DropdownMenuItem> | ||
| </LeaveTeamDialog> | ||
| ) : ( | ||
| <></> |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| case TeamActivityActions.MEMBER_REMOVED_FROM_TEAM: | ||
| return { | ||
| icon: UserMinus, | ||
| title: 'Member removed from team', | ||
| description: `${activity.performed_by_name} removed a member from team ${activity.team_name}`, |
There was a problem hiding this comment.
Missing Removed Member Information in Activity Log 
Tell me more
What is the issue?
The description for MEMBER_REMOVED_FROM_TEAM doesn't include the name of the removed member, making it impossible to track who was removed from the team.
Why this matters
Without knowing which member was removed, the activity log loses its audit value and administrators cannot effectively track membership changes.
Suggested change ∙ Feature Preview
Modify the description to include the removed member's name using the expected activity.removed_member_name property:
case TeamActivityActions.MEMBER_REMOVED_FROM_TEAM:
return {
icon: UserMinus,
title: 'Member removed from team',
description: `${activity.performed_by_name} removed ${activity.removed_member_name} from team ${activity.team_name}`,
date,
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
lib/team-utils.ts
Outdated
| return { | ||
| icon: Plus, | ||
| title: 'Tasks reassigned to team', | ||
| description: `${activity.task_count} tasks assigned to team ${activity.team_name}`, |
There was a problem hiding this comment.
Missing actor in bulk task reassignment log
Tell me more
What is the issue?
The log message for TASKS_REASSIGNED_TO_TEAM doesn't indicate who performed the reassignment action.
Why this matters
Without knowing who performed the task reassignment, it becomes difficult to audit and trace responsibility for bulk task modifications in the system.
Suggested change ∙ Feature Preview
description: `${activity.performed_by_name} reassigned ${activity.task_count} tasks to team ${activity.team_name}`,Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| return ( | ||
| <div className="flex items-center justify-between pt-6 pb-8"> | ||
| <h2 className="text-2xl font-bold">{team?.name}</h2> | ||
| <CreateTodoButton | ||
| defaultData={{ | ||
| assignee: { label: team?.name ?? '', value: teamId, type: USER_TYPE_ENUM.TEAM }, | ||
| }} | ||
| /> | ||
| <div> | ||
| <CreateTodoButton | ||
| defaultData={{ | ||
| assignee: { label: team?.name ?? '', value: teamId, type: USER_TYPE_ENUM.TEAM }, | ||
| }} | ||
| /> | ||
| {!isOwnerOrPOC ? <LeaveTeamButton teamId={teamId} /> : null} | ||
| </div> | ||
| </div> | ||
| ) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/teams/team-members.tsx (2)
128-128: Search placeholder claims “role” filtering but code doesn’t support it.Either implement role-based filtering or update the placeholder. Quick fix below updates the copy.
- placeholder="Search by name, role or tasks count" + placeholder="Search by name or tasks count"If you want role search, I can add it once member roles are included in
TeamDto.users[].Also applies to: 84-89
151-153: Skeleton colSpan should match column count (avoids table layout jitter).When
isAdminis false, header has 4 columns but skeleton spans 5.- <TableCell colSpan={5}> + <TableCell colSpan={isAdmin ? 5 : 4}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
api/teams/teams.api.ts(2 hunks)api/teams/teams.enum.ts(1 hunks)api/teams/teams.type.ts(4 hunks)components/leave-team-button.tsx(1 hunks)lib/team-utils.ts(2 hunks)modules/teams/components/leave-team-dialog.tsx(1 hunks)modules/teams/components/teams-layout-header.tsx(3 hunks)modules/teams/team-members.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Hariom01010
PR: Real-Dev-Squad/todo-frontend#189
File: modules/teams/team-members.tsx:63-63
Timestamp: 2025-08-26T08:14:08.375Z
Learning: In modules/teams/team-members.tsx, the LeaveTeamDialog components use a shared boolean state (showLeaveTeamDialog) which causes all dialog instances across all rows to open simultaneously when any "Remove from team" option is clicked. This requires per-row state management using selectedMemberId to track which specific member's dialog should be open.
📚 Learning: 2025-08-26T08:14:08.375Z
Learnt from: Hariom01010
PR: Real-Dev-Squad/todo-frontend#189
File: modules/teams/team-members.tsx:63-63
Timestamp: 2025-08-26T08:14:08.375Z
Learning: In modules/teams/team-members.tsx, the LeaveTeamDialog components use a shared boolean state (showLeaveTeamDialog) which causes all dialog instances across all rows to open simultaneously when any "Remove from team" option is clicked. This requires per-row state management using selectedMemberId to track which specific member's dialog should be open.
Applied to files:
modules/teams/components/leave-team-dialog.tsxmodules/teams/components/teams-layout-header.tsxcomponents/leave-team-button.tsxmodules/teams/team-members.tsx
📚 Learning: 2025-08-26T07:14:25.788Z
Learnt from: Hariom01010
PR: Real-Dev-Squad/todo-frontend#189
File: modules/teams/components/teams-layout-header.tsx:44-51
Timestamp: 2025-08-26T07:14:25.788Z
Learning: In the todo-frontend application, teams are only visible to people who are part of the team. Non-members cannot access team pages, so membership checks in components like teams-layout-header are unnecessary since access control is handled at a higher level.
Applied to files:
modules/teams/components/teams-layout-header.tsxmodules/teams/team-members.tsx
🧬 Code graph analysis (6)
modules/teams/components/leave-team-dialog.tsx (2)
components/ui/alert-dialog.tsx (7)
AlertDialog(124-124)AlertDialogTrigger(127-127)AlertDialogContent(128-128)AlertDialogHeader(129-129)AlertDialogTitle(131-131)AlertDialogDescription(132-132)AlertDialogFooter(130-130)components/ui/button.tsx (1)
Button(59-59)
modules/teams/components/teams-layout-header.tsx (2)
hooks/useAuth.ts (1)
useAuth(13-36)components/leave-team-button.tsx (1)
LeaveTeamButton(13-64)
components/leave-team-button.tsx (4)
hooks/useAuth.ts (1)
useAuth(13-36)api/teams/teams.api.ts (1)
TeamsApi(14-101)api/tasks/tasks.api.ts (1)
TasksApi(17-77)modules/teams/components/leave-team-dialog.tsx (1)
LeaveTeamDialog(23-59)
api/teams/teams.api.ts (2)
lib/api-client.ts (1)
apiClient(6-10)api/teams/teams.type.ts (1)
UserRole(142-146)
modules/teams/team-members.tsx (4)
hooks/useAuth.ts (1)
useAuth(13-36)api/teams/teams.api.ts (1)
TeamsApi(14-101)api/tasks/tasks.api.ts (1)
TasksApi(17-77)modules/teams/components/leave-team-dialog.tsx (1)
LeaveTeamDialog(23-59)
api/teams/teams.type.ts (1)
components/teams/TeamActivity.tsx (1)
TeamActivity(58-65)
🪛 GitHub Actions: CI
lib/team-utils.ts
[error] 78-78: Type error: Property 'MEMBER_REMOVED_FROM_TEAM' does not exist on type 'typeof TeamActivityActions'.
🔇 Additional comments (8)
api/teams/teams.enum.ts (1)
1-5: TeamRoles enum looks good and scoped.Clear, string-valued roles; exported for reuse. No issues.
modules/teams/components/leave-team-dialog.tsx (1)
33-35: Controlled AlertDialog is correctly scoped per member viaactiveDialogMemberId; no changes needed.modules/teams/components/teams-layout-header.tsx (1)
24-27: Good: guards and role-based gating for leave action.Auth + team loading handled; owner/POC protected from leaving. Looks correct.
api/teams/teams.api.ts (2)
73-78: removeFromTeam endpoint wiring looks correct.DELETE path and arg names are clear. Void return is appropriate.
79-89: getUserRoles addition is consistent with types and client config.Query key and GET path are fine; return type matches UserRole.
components/leave-team-button.tsx (1)
51-55: No changes needed:DEFAULT_USER.idis an empty string (falsy), so the existingif (user?.id)guard correctly prevents using a bogus ID.modules/teams/team-members.tsx (1)
189-219: Per-row dialog state fix looks good; prevents all dialogs opening at once.
activeDialogMemberIdgating onmember.idcorrectly scopes the dialog open state to the selected row.api/teams/teams.type.ts (1)
134-146: Confirm backend contract forUserRole/UserTeamRole.Verify that
role_namealigns withTeamRolesenum values and thatscopeis always'TEAM'; otherwise add a union or zod parser.I can add a zod schema and runtime parsing if the backend returns wider shapes.
| removeFromTeam: { | ||
| key: ({ teamId }: { teamId: string }) => ['TeamsApi.removeFromTeam', teamId], | ||
| fn: async ({ teamId, memberId }: { teamId: string; memberId: string }): Promise<void> => { | ||
| await apiClient.delete(`/v1/teams/${teamId}/members/${memberId}`) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: include memberId in the key for better cache scoping.
If you later use mutation keys or logs based on keys, scoping by memberId can help.
Apply:
- key: ({ teamId }: { teamId: string }) => ['TeamsApi.removeFromTeam', teamId],
+ key: ({ teamId, memberId }: { teamId: string; memberId?: string }) => [
+ 'TeamsApi.removeFromTeam',
+ teamId,
+ memberId,
+ ],Call sites don’t rely on this today, so safe to skip if unnecessary.
📝 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.
| removeFromTeam: { | |
| key: ({ teamId }: { teamId: string }) => ['TeamsApi.removeFromTeam', teamId], | |
| fn: async ({ teamId, memberId }: { teamId: string; memberId: string }): Promise<void> => { | |
| await apiClient.delete(`/v1/teams/${teamId}/members/${memberId}`) | |
| }, | |
| }, | |
| removeFromTeam: { | |
| key: ({ teamId, memberId }: { teamId: string; memberId?: string }) => [ | |
| 'TeamsApi.removeFromTeam', | |
| teamId, | |
| memberId, | |
| ], | |
| fn: async ({ teamId, memberId }: { teamId: string; memberId: string }): Promise<void> => { | |
| await apiClient.delete(`/v1/teams/${teamId}/members/${memberId}`) | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In api/teams/teams.api.ts around lines 73 to 78 the cache/mutation key for
removeFromTeam is currently only scoped by teamId which loses granularity for
per-member operations; update the key to include memberId (e.g.
['TeamsApi.removeFromTeam', teamId, memberId]) and adjust the key function
signature to accept memberId ({ teamId, memberId }: { teamId: string; memberId:
string }) so callers that generate the key pass both values; no other behavior
changes required.
api/teams/teams.type.ts
Outdated
| action: TeamActivityActions.TASKS_REASSIGNED_TO_TEAM | ||
| performed_by_name: string | ||
| task_count: string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a numeric type for task_count.
Counts should be numbers to avoid downstream parsing/pluralization issues.
-export type TaskReassignActivity = BaseActivity & {
+export type TaskReassignActivity = BaseActivity & {
action: TeamActivityActions.TASKS_REASSIGNED_TO_TEAM
performed_by_name: string
- task_count: string
+ task_count: number
}📝 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.
| action: TeamActivityActions.TASKS_REASSIGNED_TO_TEAM | |
| performed_by_name: string | |
| task_count: string | |
| } | |
| export type TaskReassignActivity = BaseActivity & { | |
| action: TeamActivityActions.TASKS_REASSIGNED_TO_TEAM | |
| performed_by_name: string | |
| task_count: number | |
| } |
🤖 Prompt for AI Agents
In api/teams/teams.type.ts around lines 114 to 117, the task_count field is
declared as a string which can cause downstream parsing and pluralization bugs;
change its type to a numeric type (number) so counters are handled as numbers
throughout the codebase, update any related interfaces/usages/tests to expect a
number and ensure any serialization/parsing converts incoming string values to
numbers before assigning to task_count.
| queryClient.invalidateQueries({ | ||
| queryKey: TasksApi.getTasks.key(), | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Invalidate all task queries using a prefix key (current key risks missing variants).
Using TasksApi.getTasks.key() expands to ['TasksApi.getTasks', undefined], which may not match queries with params. Invalidate via a stable prefix instead.
- queryClient.invalidateQueries({
- queryKey: TasksApi.getTasks.key(),
- })
+ queryClient.invalidateQueries({ queryKey: ['TasksApi.getTasks'] })🤖 Prompt for AI Agents
In components/leave-team-button.tsx around lines 29-31, the call to
queryClient.invalidateQueries currently passes TasksApi.getTasks.key() which
expands to ['TasksApi.getTasks', undefined] and can miss queries with params;
change the invalidate to target the stable prefix instead (e.g.,
invalidateQueries with the prefix key ['TasksApi.getTasks'] or use a predicate
that checks query.queryKey[0] === 'TasksApi.getTasks') so all task query
variants are invalidated.
components/leave-team-button.tsx
Outdated
| toast.success('Leave Team Successfully') | ||
| router.push('/dashboard') | ||
| }, | ||
| onError: () => { | ||
| toast.error('Failed to leave team') |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Polish toast copy; align with UX tense/casing.
Minor, but improves UX consistency.
- toast.success('Leave Team Successfully')
+ toast.success('Left team successfully.')- toast.error('Failed to leave team')
+ toast.error('Failed to leave team.')📝 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.
| toast.success('Leave Team Successfully') | |
| router.push('/dashboard') | |
| }, | |
| onError: () => { | |
| toast.error('Failed to leave team') | |
| toast.success('Left team successfully.') | |
| router.push('/dashboard') | |
| }, | |
| onError: () => { | |
| toast.error('Failed to leave team.') |
🤖 Prompt for AI Agents
In components/leave-team-button.tsx around lines 32 to 36, the success and error
toast messages use inconsistent tense/casing; update them to match UX guidelines
by using past tense and sentence case (e.g., change success to "Left team
successfully" or "Successfully left team" and keep error as "Failed to leave
team" or "Could not leave team" to match tone), and ensure both messages follow
the same casing and tense conventions used across the app.
| return ( | ||
| <LeaveTeamDialog | ||
| title="Leave Team" | ||
| description="Are you sure you want to leave this team? You will lose access to its tasks and members." | ||
| buttonText="Leave Team" | ||
| open={showLeaveTeamDialog} | ||
| onOpenChange={setShowLeaveTeamDialog} | ||
| onSubmit={() => { | ||
| if (user?.id) { | ||
| leaveTeamMutation.mutate({ teamId, memberId: user.id }) | ||
| } else { | ||
| toast.error('User ID not found') | ||
| } | ||
| }} | ||
| > |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Prevent double-submit; disable confirm while mutation is pending.
Dialog closes immediately and confirm remains clickable until onClick runs; add a disabled/loading affordance.
Apply in this file:
<LeaveTeamDialog
title="Leave Team"
description="Are you sure you want to leave this team? You will lose access to its tasks and members."
- buttonText="Leave Team"
+ buttonText={leaveTeamMutation.isPending ? 'Leaving…' : 'Leave Team'}
open={showLeaveTeamDialog}
- onOpenChange={setShowLeaveTeamDialog}
+ onOpenChange={setShowLeaveTeamDialog}
+ confirmDisabled={leaveTeamMutation.isPending}And extend the dialog component to support confirmDisabled (and optionally confirmVariant="destructive"):
// modules/teams/components/leave-team-dialog.tsx
export const LeaveTeamDialog = ({
title,
description,
buttonText,
open,
onSubmit,
onOpenChange,
children,
+ confirmDisabled,
+ confirmVariant = 'default',
}: LeaveTeamDialogProps) => {
...
- <Button
- variant="default"
+ <Button
+ variant={confirmVariant}
type="submit"
+ disabled={confirmDisabled}
onClick={() => {
onSubmit()
onOpenChange(false)
}}
>If you prefer, I can open a follow-up PR to wire this across all usages.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/leave-team-button.tsx around lines 43–57, the confirm button
allows double-submit and the dialog closes immediately; wire the mutation
loading state into the dialog and prevent closing while pending. Change the
submit flow to use leaveTeamMutation.mutateAsync (or mutate with
onSuccess/onError) so you only close the dialog on success, and pass a new prop
confirmDisabled={leaveTeamMutation.isLoading} (and confirmVariant="destructive")
into LeaveTeamDialog so the confirm button is disabled/indicates loading while
the mutation is pending.
| <Button | ||
| variant="default" | ||
| type="submit" | ||
| onClick={() => { | ||
| onSubmit() | ||
| onOpenChange(false) | ||
| }} | ||
| > | ||
| {buttonText} | ||
| </Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the confirm button type="button" (not submit) to avoid unintended form submits.
This dialog isn’t inside a form; submit type can trigger parent form submissions.
Apply:
- <Button
- variant="default"
- type="submit"
+ <Button
+ variant="default"
+ type="button"
onClick={() => {
onSubmit()
onOpenChange(false)
}}
>📝 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.
| <Button | |
| variant="default" | |
| type="submit" | |
| onClick={() => { | |
| onSubmit() | |
| onOpenChange(false) | |
| }} | |
| > | |
| {buttonText} | |
| </Button> | |
| <Button | |
| variant="default" | |
| type="button" | |
| onClick={() => { | |
| onSubmit() | |
| onOpenChange(false) | |
| }} | |
| > | |
| {buttonText} | |
| </Button> |
🤖 Prompt for AI Agents
In modules/teams/components/leave-team-dialog.tsx around lines 45 to 54 the
confirm Button is declared with type="submit", which can cause unintended parent
form submissions; change the button to type="button" and keep the onClick
handler that calls onSubmit() and onOpenChange(false) so it behaves as a normal
dialog action without triggering form submits.
| {!isOwnerOrPOC ? <LeaveTeamButton teamId={teamId} /> : null} | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor JSX simplification.
Use logical && instead of ternary with null.
Apply:
- {!isOwnerOrPOC ? <LeaveTeamButton teamId={teamId} /> : null}
+ {!isOwnerOrPOC && <LeaveTeamButton teamId={teamId} />}📝 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.
| {!isOwnerOrPOC ? <LeaveTeamButton teamId={teamId} /> : null} | |
| </div> | |
| {!isOwnerOrPOC && <LeaveTeamButton teamId={teamId} />} | |
| </div> |
🤖 Prompt for AI Agents
In modules/teams/components/teams-layout-header.tsx around lines 45 to 46,
replace the ternary rendering `{!isOwnerOrPOC ? <LeaveTeamButton teamId={teamId}
/> : null}` with the simpler logical AND form `!isOwnerOrPOC && <LeaveTeamButton
teamId={teamId} />`; keep props and surrounding JSX unchanged.
| }, | ||
| enabled: !!userId, | ||
| }) | ||
| const isAdmin = userRole?.roles.find((role) => role.role_name == TeamRoles.ADMIN) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compute admin as a boolean and use strict equality.
Avoid relying on truthiness of an object from find; prefer some with ===.
- const isAdmin = userRole?.roles.find((role) => role.role_name == TeamRoles.ADMIN)
+ const isAdmin = !!userRole?.roles.some((role) => role.role_name === TeamRoles.ADMIN)📝 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 isAdmin = userRole?.roles.find((role) => role.role_name == TeamRoles.ADMIN) | |
| const isAdmin = !!userRole?.roles.some((role) => role.role_name === TeamRoles.ADMIN) |
🤖 Prompt for AI Agents
In modules/teams/team-members.tsx around line 77, the code currently assigns
isAdmin from find which returns an object (truthiness-based) and uses loose
equality; change it to compute isAdmin as a boolean using Array.prototype.some
and strict equality (===) against TeamRoles.ADMIN so isAdmin is true/false
reliably and comparisons use ===.
| queryClient.invalidateQueries({ | ||
| queryKey: TasksApi.getTasks.key(), | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Invalidate all task queries using a prefix key (current key risks missing variants).
Same rationale as in LeaveTeamButton.
- queryClient.invalidateQueries({
- queryKey: TasksApi.getTasks.key(),
- })
+ queryClient.invalidateQueries({ queryKey: ['TasksApi.getTasks'] })📝 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.
| queryClient.invalidateQueries({ | |
| queryKey: TasksApi.getTasks.key(), | |
| }) | |
| // Invalidate all variants of the getTasks query by prefixing the key | |
| - queryClient.invalidateQueries({ | |
| - queryKey: TasksApi.getTasks.key(), | |
| queryClient.invalidateQueries({ queryKey: ['TasksApi.getTasks'] }) |
🤖 Prompt for AI Agents
In modules/teams/team-members.tsx around lines 113–115, the current
invalidateQueries call uses the exact task query key and may miss variant
queries; change it to invalidate all task variants by passing exact: false (e.g.
queryClient.invalidateQueries({ queryKey: TasksApi.getTasks.key(), exact: false
})) so every task-related query that shares the key prefix is invalidated.
…0/todo-frontend into feat(team)/remove-member
|
could you please stack it over #192 as it contains changes of that PR too |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| const { data: userRole, isLoading: isUserRoleLoading } = useQuery({ | ||
| queryKey: TeamsApi.getUserRoles.key({ teamId, userId: userId }), | ||
| queryFn: () => { | ||
| return TeamsApi.getUserRoles.fn({ teamId, userId }) |
There was a problem hiding this comment.
There's a potential type safety issue here. The userId variable could be undefined, but TeamsApi.getUserRoles.fn() expects a string parameter. This would cause a runtime error if the function is called with undefined.
Consider adding a null check before making the API call:
return userId
? TeamsApi.getUserRoles.fn({ teamId, userId })
: Promise.reject('No user ID available')This ensures the function only executes with valid parameters and provides a clear error message when the user ID is missing.
| return TeamsApi.getUserRoles.fn({ teamId, userId }) | |
| return userId ? TeamsApi.getUserRoles.fn({ teamId, userId }) : Promise.reject('No user ID available') |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
modules/teams/team-members.tsx
Outdated
| <DropdownMenuContent> | ||
| <DropdownMenuItem>Change Role</DropdownMenuItem> | ||
| <DropdownMenuItem>Remove from team</DropdownMenuItem> | ||
| {member.id !== user.id && |
There was a problem hiding this comment.
There's a potential null reference issue on line 189 where user.id is accessed directly. If user is undefined during loading states, this will cause a runtime error. Consider using the optional chaining operator to safely access the id property:
{member.id !== user?.id &&This ensures the comparison only happens when user is defined, preventing the application from crashing during loading states.
| {member.id !== user.id && | |
| {member.id !== user?.id && |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| }, | ||
| enabled: !!userId, | ||
| }) | ||
| const isAdmin = userRole?.roles.find((role) => role.role_name == TeamRoles.ADMIN) |
There was a problem hiding this comment.
The comparison for admin role detection is using loose equality (==) which can lead to unexpected behavior due to type coercion. For enum comparisons, strict equality (===) is recommended to ensure type safety:
const isAdmin = userRole?.roles.find((role) => role.role_name === TeamRoles.ADMIN)This ensures the comparison only succeeds when both the value and type match exactly, preventing potential bugs that could arise from type coercion.
| const isAdmin = userRole?.roles.find((role) => role.role_name == TeamRoles.ADMIN) | |
| const isAdmin = userRole?.roles.find((role) => role.role_name === TeamRoles.ADMIN) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
- add shimmer for isAuthLoading state - moved the condition check in onSubmit in leave team button to a different function for better readability
|
You’re welcome to review this PR if you’d like, but if it’s too much effort to compare changes with #192, we can hold off until #192 is merged. |
you can read about stack pr from here https://blog.logrocket.com/using-stacked-pull-requests-in-github/ |
successful mutation
- use dialog instead of alert dialog to close modal when clicked outside
…into feat(team)/leave-team
…0/todo-frontend into feat(team)/remove-team
the user is removed
- add enum for scope instead of hardcoding value - define types for api parameters
…0/todo-frontend into feat(team)/remove-member
4 participants
Date: 29 Aug 2025
Developer Name: @Hariom01010
Issue Ticket Number
Closes #170
Description
Add remove member from team functionality.
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Normal Users:
Admins:
admins.mp4
Test Coverage
Screenshot 1
Additional Notes
NOTE: the PR is built upon the leave-team thus showing 297 changes. Once that PR is merged, this will only show the changes made in this PR
Description by Korbit AI
What change is being made?
Add remove member functionality to Teams: introduce new API endpoints (removeFromTeam, getUserRoles), new enums and types for team roles and user roles, UI components and dialogs for removing members and leaving teams, and integrate role checks and removal flows into the team members list and team header.
Why are these changes being made?
Enable proper team membership management by allowing admins and authorized users to remove members and view member roles, improving workflow and activity tracking for team changes. If applicable, note any design decisions or trade-offs (e.g., using a shared LeaveTeamDialog for removal flows) in a brief, non-technical summary.