Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds a new "teams" feature: SDK exports for Teams/TeamDetails views, multiple Teams UI components (views, dialogs, columns, member management), CSS modules, and client-demo routes/pages to surface teams under /:orgId/settings. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
rohanchkrabrty
changed the title
Coverage Report for CI Build 24183450372Coverage remained the same at 41.282%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
web/sdk/react/views-new/teams/components/add-member-menu.tsx (1)
41-51: DeferlistOrganizationUsersuntil the menu is used.This query fires on every editable team-details render, even if the user never opens the add-member menu. For larger orgs that is avoidable page-load work; tie it to menu-open or search interaction instead.
web/sdk/react/views-new/teams/components/team-columns.tsx (1)
41-48: Consider displaying "0 members" instead of hiding the count entirely.When
members_countis0(falsy), the cell rendersnull. This may confuse users who see an empty cell. Consider showing "0 members" for clarity.Proposed fix
cell: ({ getValue }) => { const value = getValue() as number; - return value ? ( + return ( <Text size="regular" variant="secondary"> {value} {value === 1 ? 'member' : 'members'} </Text> - ) : null; + ); }web/sdk/react/views-new/teams/components/add-team-dialog.tsx (1)
27-32: Redundant length constraints in validation schema.The
min(3)andmax(50)constraints are also enforced by the regex{3,50}. Consider removing the redundant.min()and.max()calls, or adjusting the regex to only validate character types.Proposed fix (keep regex, remove min/max)
name: yup .string() .required('Team name is required') - .min(3, 'Name must be at least 3 characters') - .max(50, 'Name must be at most 50 characters') .matches( /^[a-zA-Z0-9_-]{3,50}$/, "Only numbers, letters, '-', and '_' are allowed. Spaces are not allowed." )web/sdk/react/views-new/teams/team-details-view.tsx (2)
66-70: Module-level dialog handles are singletons shared across all component instances.These handles are created at module scope, meaning all instances of
TeamDetailsViewwould share the same dialog state. If this component is rendered multiple times (e.g., in different tabs or routes), dialogs could interfere with each other.Consider creating handles inside the component or using
useMemoto ensure instance isolation.Proposed fix - move handles inside component
+export function TeamDetailsView({ + teamId, + teamsLabel = 'Teams', + onNavigateToTeams, + onDeleteSuccess +}: TeamDetailsViewProps) { + const memberMenuHandle = useMemo(() => Menu.createHandle<MemberMenuPayload>(), []); + const editTeamDialogHandle = useMemo(() => Dialog.createHandle<EditTeamPayload>(), []); + const deleteTeamDialogHandle = useMemo(() => AlertDialog.createHandle<DeleteTeamPayload>(), []); + const removeMemberDialogHandle = useMemo(() => AlertDialog.createHandle<RemoveMemberPayload>(), []); + const { activeOrganization: organization } = useFrontier();And remove the module-level declarations.
446-446:teamActionsMenuHandleis also module-scoped.Similar to the other handles, this could cause issues with multiple component instances.
web/sdk/react/views-new/teams/teams-view.tsx (2)
39-42: Module-level handles cause shared state across component instances.These handles are created at module scope, so all instances of
TeamsViewshare the same dialog/menu state. If multipleTeamsViewcomponents are rendered (e.g., in different tabs or views), opening a dialog in one instance would affect others.Consider moving the handle creation inside the component with
useMemo:♻️ Suggested refactor
-const teamMenuHandle = Menu.createHandle<TeamMenuPayload>(); -const addTeamDialogHandle = Dialog.createHandle(); -const editTeamDialogHandle = Dialog.createHandle<EditTeamPayload>(); -const deleteTeamDialogHandle = AlertDialog.createHandle<DeleteTeamPayload>(); - export interface TeamsViewProps { title?: string; description?: string; @@ -50,6 +44,11 @@ export function TeamsView({ onTeamClick }: TeamsViewProps) { const [showOrgTeams, setShowOrgTeams] = useState(false); + const teamMenuHandle = useMemo(() => Menu.createHandle<TeamMenuPayload>(), []); + const addTeamDialogHandle = useMemo(() => Dialog.createHandle(), []); + const editTeamDialogHandle = useMemo(() => Dialog.createHandle<EditTeamPayload>(), []); + const deleteTeamDialogHandle = useMemo(() => AlertDialog.createHandle<DeleteTeamPayload>(), []);
105-116: Consider preventing duplicate error toasts on reference changes.If
teamsErrorchanges reference (e.g., due to query re-execution returning the same error), the toast will fire again. This is typically rare but can occur with some query configurations.♻️ Optional: Track shown errors with a ref
+const shownErrorRef = useRef<unknown>(null); + useEffect(() => { - if (teamsError) { + if (teamsError && teamsError !== shownErrorRef.current) { + shownErrorRef.current = teamsError; toastManager.add({ title: 'Something went wrong', description: teamsError instanceof Error ? teamsError.message : 'Failed to load teams', type: 'error' }); } }, [teamsError]);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a15d1dd-995d-4e4f-a937-349bd86136b9
📒 Files selected for processing (17)
web/apps/client-demo/src/Router.tsxweb/apps/client-demo/src/pages/Settings.tsxweb/apps/client-demo/src/pages/settings/TeamDetails.tsxweb/apps/client-demo/src/pages/settings/Teams.tsxweb/sdk/react/index.tsweb/sdk/react/views-new/teams/components/add-member-menu.tsxweb/sdk/react/views-new/teams/components/add-team-dialog.tsxweb/sdk/react/views-new/teams/components/delete-team-dialog.tsxweb/sdk/react/views-new/teams/components/edit-team-dialog.tsxweb/sdk/react/views-new/teams/components/member-columns.tsxweb/sdk/react/views-new/teams/components/remove-member-dialog.tsxweb/sdk/react/views-new/teams/components/team-columns.tsxweb/sdk/react/views-new/teams/index.tsweb/sdk/react/views-new/teams/team-details-view.module.cssweb/sdk/react/views-new/teams/team-details-view.tsxweb/sdk/react/views-new/teams/teams-view.module.cssweb/sdk/react/views-new/teams/teams-view.tsx
web/sdk/react/views-new/teams/components/delete-team-dialog.tsx
Outdated
Show resolved
Hide resolved
web/sdk/react/views-new/teams/components/delete-team-dialog.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/sdk/react/views-new/teams/components/add-member-menu.tsx (1)
73-105:⚠️ Potential issue | 🟠 MajorPrevent duplicate membership writes while add is in-flight.
addMembercan be clicked repeatedly before the member list refreshes, which can send duplicateaddGroupUserswrites for the same user.Proposed fix
-import { useCallback, useEffect, useMemo } from 'react'; +import { useCallback, useEffect, useMemo, useState } from 'react'; @@ export function AddMemberMenu({ teamId, canUpdateGroup, members, refetch }: AddMemberMenuProps) { const { activeOrganization: organization } = useFrontier(); + const [isAddingMember, setIsAddingMember] = useState(false); @@ const { mutate: addGroupUsers } = useMutation( FrontierServiceQueries.addGroupUsers, { onSuccess: () => { toastManager.add({ title: 'Member added', type: 'success' }); refetch(); }, onError: (err: Error) => { toastManager.add({ title: 'Something went wrong', description: err.message, type: 'error' }); + }, + onSettled: () => { + setIsAddingMember(false); } } ); @@ (userId: string) => { - if (!userId || !organization?.id || !teamId) return; + if (!userId || !organization?.id || !teamId || isAddingMember) return; + setIsAddingMember(true); addGroupUsers( create(AddGroupUsersRequestSchema, { id: teamId, orgId: organization.id, userIds: [userId] }) ); }, - [addGroupUsers, organization?.id, teamId] + [addGroupUsers, isAddingMember, organization?.id, teamId] );
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6396305-043f-4759-a8a1-71aa8346b1b5
📒 Files selected for processing (5)
web/sdk/react/views-new/teams/components/add-member-menu.tsxweb/sdk/react/views-new/teams/components/delete-team-dialog.tsxweb/sdk/react/views-new/teams/components/team-columns.tsxweb/sdk/react/views-new/teams/team-details-view.tsxweb/sdk/react/views-new/teams/teams-view.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/sdk/react/views-new/teams/components/delete-team-dialog.tsx
- web/sdk/react/views-new/teams/teams-view.tsx
| return value ? ( | ||
| <Text size="regular" variant="secondary"> | ||
| {value} {value === 1 ? 'member' : 'members'} | ||
| </Text> | ||
| ) : null; |
There was a problem hiding this comment.
Render 0 members instead of a blank cell.
Line 44 treats 0 as falsy and returns null, so teams with zero members show an empty value. That hides valid data in the table.
Suggested fix
cell: ({ getValue }) => {
const value = getValue() as number;
- return value ? (
+ if (value == null) return null;
+ return (
<Text size="regular" variant="secondary">
{value} {value === 1 ? 'member' : 'members'}
</Text>
- ) : null;
+ );
}📝 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.
| return value ? ( | |
| <Text size="regular" variant="secondary"> | |
| {value} {value === 1 ? 'member' : 'members'} | |
| </Text> | |
| ) : null; | |
| if (value == null) return null; | |
| return ( | |
| <Text size="regular" variant="secondary"> | |
| {value} {value === 1 ? 'member' : 'members'} | |
| </Text> | |
| ); |
| const { | ||
| data: membersData, | ||
| isLoading: isMembersLoading, | ||
| refetch: refetchMembers | ||
| } = useQuery( | ||
| FrontierServiceQueries.listGroupUsers, | ||
| create(ListGroupUsersRequestSchema, { | ||
| id: teamId || '', | ||
| orgId: organization?.id || '', | ||
| withRoles: true | ||
| }), | ||
| { enabled: !!organization?.id && !!teamId } | ||
| ); |
There was a problem hiding this comment.
Surface member-list query failures to users.
listGroupUsers failures are currently silent, unlike team/roles query failures.
Proposed fix
const {
data: membersData,
isLoading: isMembersLoading,
+ error: membersError,
refetch: refetchMembers
} = useQuery(
@@
);
+
+ useEffect(() => {
+ if (membersError) {
+ toastManager.add({
+ title: 'Something went wrong',
+ description: membersError.message,
+ type: 'error'
+ });
+ }
+ }, [membersError]);3 participants
Summary
@raystack/apsara-v1primitives underviews-new/ViewContainerandViewHeaderlayout components for consistent page structureDialog.createHandle,AlertDialog.createHandle,Menu.createHandle) across all revamped pagesreact/index.tsexports and client-demo app with routes and sidebar navigation