Conversation
📝 WalkthroughWalkthroughAdds a tabbed UserDetail UI with Profile, Projects, and Partners tabs, new React components and GraphQL queries/fragments for listing projects and partners, a consolidated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus review on:
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/scenes/Users/Detail/Tabs/Projects/UserProjectPanel/UserProjectList.graphql (1)
15-17: Remove redundant fragment wrapperThe
userProjectDataGridRowfragment only spreadsprojectDataGridRowwithout adding any fields. This adds unnecessary indirection.- ...userProjectDataGridRow + ...projectDataGridRowThen remove the redundant fragment definition:
-fragment userProjectDataGridRow on Project { - ...projectDataGridRow -}src/scenes/Users/Detail/Tabs/Profile/UserDetailProfile.tsx (2)
81-89: Consider adding accessibility attributes to the time display.The LocalTime component correctly handles timezone formatting, but could benefit from accessibility improvements for screen readers.
- return <>{formatted}</>; + return <span title={`Local time in ${timezone}`}>{formatted}</span>;
99-127: Consider simplifying the loading state logic.The DisplayProperty wrapper has complex loading logic that could be streamlined. The skeleton structure with two Typography components may not perfectly match the actual content structure.
const DisplayProperty = (props: DisplaySimplePropertyProps) => !props.value && !props.loading ? null : ( <DisplaySimpleProperty variant="body1" {...{ component: 'div' }} {...props} loading={ - props.loading ? ( - <> - <Typography variant="body2"> - <Skeleton width="10%" /> - </Typography> - <Typography variant="body1"> - <Skeleton width="40%" /> - </Typography> - </> - ) : null + props.loading ? <Skeleton width="60%" /> : null } LabelProps={{ color: 'textSecondary', variant: 'body2', ...props.LabelProps, }} ValueProps={{ color: 'textPrimary', ...props.ValueProps, }} /> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/api/schema/typePolicies/typePolicies.base.ts(1 hunks)src/scenes/Users/Detail/Tabs/Profile/UserDetailProfile.tsx(1 hunks)src/scenes/Users/Detail/Tabs/Projects/UserDetailProjects.tsx(1 hunks)src/scenes/Users/Detail/Tabs/Projects/UserProjectPanel/UserProjectList.graphql(1 hunks)src/scenes/Users/Detail/Tabs/Projects/UserProjectPanel/UserProjectsPanel.tsx(1 hunks)src/scenes/Users/Detail/UserDetail.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CarsonF
PR: SeedCompany/cord-field#1604
File: src/scenes/Root/Notifications/Notifications.tsx:35-51
Timestamp: 2024-10-21T15:15:09.113Z
Learning: In the `Notifications` component (`src/scenes/Root/Notifications/Notifications.tsx`), Apollo Client automatically updates the notification's unread status in the cache when the mutation returns the updated notification, so manually updating the cache in the `update` function is unnecessary.
src/scenes/Users/Detail/UserDetail.tsx (2)
Learnt from: CarsonF
PR: SeedCompany/cord-field#1604
File: src/components/Feature.tsx:12-19
Timestamp: 2024-10-21T15:17:35.618Z
Learning: In `src/components/Feature.tsx`, the `Feature` component uses `useFeatureFlagPayload` and the `match` prop to determine feature availability, which differs from the `useFeatureEnabled` hook.
Learnt from: CarsonF
PR: SeedCompany/cord-field#1696
File: src/scenes/FieldZones/CreateFieldZone/CreateFieldZone.tsx:0-0
Timestamp: 2025-06-20T17:50:15.534Z
Learning: In this codebase, GraphQL documents (like CreateFieldZoneDocument, CreateFieldRegionDocument, etc.) are already properly typed, and Apollo hooks (useMutation, useQuery, etc.) automatically infer the correct types from these typed documents. Therefore, explicit generic type parameters on Apollo hooks are unnecessary and should not be suggested.
src/scenes/Users/Detail/Tabs/Profile/UserDetailProfile.tsx (1)
Learnt from: CarsonF
PR: SeedCompany/cord-field#1604
File: src/components/Feature.tsx:12-19
Timestamp: 2024-10-21T15:17:35.618Z
Learning: In `src/components/Feature.tsx`, the `Feature` component uses `useFeatureFlagPayload` and the `match` prop to determine feature availability, which differs from the `useFeatureEnabled` hook.
🧬 Code Graph Analysis (3)
src/scenes/Users/Detail/Tabs/Projects/UserDetailProjects.tsx (2)
src/components/Tabs/TabPanelContent.tsx (1)
TabPanelContent(6-22)src/scenes/Users/Detail/Tabs/Projects/UserProjectPanel/UserProjectsPanel.tsx (1)
UserProjectsPanel(23-61)
src/scenes/Users/Detail/UserDetail.tsx (8)
src/hooks/useQueryParams.ts (3)
makeQueryHandler(183-247)withDefault(92-104)EnumParam(69-85)src/components/Comments/CommentsContext.tsx (1)
useComments(65-71)src/components/Dialog/useDialog.tsx (1)
useDialog(4-33)src/components/Error/Error.tsx (1)
Error(57-116)src/components/Tabs/TabsContainer.tsx (1)
TabsContainer(4-16)src/scenes/Users/Detail/Tabs/Profile/UserDetailProfile.tsx (1)
UserDetailProfile(18-79)src/scenes/Users/Detail/Tabs/Projects/UserDetailProjects.tsx (1)
UserDetailProjects(4-10)src/scenes/Users/Edit/EditUser.tsx (1)
EditUser(13-57)
src/scenes/Users/Detail/Tabs/Profile/UserDetailProfile.tsx (3)
src/common/displayEnums.ts (1)
labelsFrom(16-19)src/components/PartnerListItemCard/PartnerListItemCard.tsx (1)
PartnerListItemCard(37-85)src/components/DisplaySimpleProperty/DisplaySimpleProperty.tsx (2)
DisplaySimplePropertyProps(4-12)DisplaySimpleProperty(14-50)
🪛 Biome (1.9.4)
src/scenes/Users/Detail/UserDetail.tsx
[error] 11-11: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run
🔇 Additional comments (6)
src/api/schema/typePolicies/typePolicies.base.ts (1)
88-92: LGTM!The type policy addition follows the existing pattern used for
Partner.projectsand correctly configures Apollo cache to avoid page merging for infinite scroll functionality.src/scenes/Users/Detail/Tabs/Projects/UserDetailProjects.tsx (1)
1-11: LGTM!Clean wrapper component that properly uses
TabPanelContentfor consistent tab panel styling.src/scenes/Users/Detail/UserDetail.tsx (1)
124-124: Verify EditUser rendering logic changeThe
EditUsercomponent now renders outside the error check, meaning it will render even when there's an error (if user data exists from cache). Is this intentional?If this change is unintentional, consider moving it back inside the error check:
- {user ? <EditUser user={user} {...editUserState} /> : null} + {user ? <EditUser user={user} {...editUserState} /> : null} </> )}src/scenes/Users/Detail/Tabs/Profile/UserDetailProfile.tsx (3)
18-79: Well-structured component with good separation of concerns.The main component structure is clean and follows React best practices. The use of optional chaining for undefined user data is appropriate, and the conditional rendering for the partners section is correctly implemented.
91-97: Time update interval is reasonable but consider performance implications.The 1-second update interval is appropriate for a time display. The
useIntervalhook from ahooks should handle cleanup properly when the component unmounts.
42-42: labelsFrom handles empty/undefined values correctlyThe
labelsFromutility insrc/common/displayEnums.tsdefaultsvaluesto an empty array (values ?? []) and returns an empty string when there are no roles, solabelsFrom(RoleLabels)(user?.roles.value)will gracefully display nothing for undefined or empty role lists. No changes are needed.
| const { userId = '' } = useParams(); | ||
|
|
||
| const [dataGridProps] = useDataGridSource({ | ||
| query: UserProjectListDocument, | ||
| variables: { userId }, | ||
| listAt: 'user.projects', | ||
| initialInput: { | ||
| sort: 'name', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Handle missing userId gracefully
Defaulting userId to an empty string could cause the GraphQL query to fail or return unexpected results. Consider adding explicit error handling for this case.
export const UserProjectsPanel = () => {
- const { userId = '' } = useParams();
+ const { userId } = useParams();
+
+ if (!userId) {
+ return (
+ <Typography color="error" sx={{ p: 2 }}>
+ User ID is required
+ </Typography>
+ );
+ }
const [dataGridProps] = useDataGridSource({
query: UserProjectListDocument,
variables: { userId },🤖 Prompt for AI Agents
In src/scenes/Users/Detail/Tabs/Projects/UserProjectPanel/UserProjectsPanel.tsx
around lines 24 to 33, the userId is defaulted to an empty string which may
cause the GraphQL query to fail or behave unexpectedly. Instead of defaulting,
add explicit error handling to check if userId is missing or undefined before
running the query. If userId is not present, handle this case gracefully by
either showing an error message or skipping the query execution to prevent
invalid requests.
src/scenes/Users/Detail/Tabs/Projects/UserProjectPanel/UserProjectsPanel.tsx
Show resolved
Hide resolved
rdonigian
force-pushed
the
users-projects-grid
branch
from
8ccdc83 to
f697c21
Compare
rdonigian
force-pushed
the
users-projects-grid
branch
from
f697c21 to
ec1e88b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/scenes/Users/Detail/UserDetail.tsx (1)
8-8: RenameErrorimport to avoid shadowing the globalErrorconstructor.This shadows the global
Errorconstructor, which could cause confusion. Consider renaming toErrorDisplayor similar.-import { Error } from '~/components/Error'; +import { Error as ErrorDisplay } from '~/components/Error';And update the usage on line 48:
- <Error error={error}> + <ErrorDisplay error={error}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/api/schema/typePolicies/typePolicies.base.ts(1 hunks)src/scenes/Users/Detail/Tabs/Partners/UserDetailPartners.tsx(1 hunks)src/scenes/Users/Detail/Tabs/Partners/UserPartnerPanel/UserPartnerList.graphql(1 hunks)src/scenes/Users/Detail/Tabs/Partners/UserPartnerPanel/UserPartnersPanel.tsx(1 hunks)src/scenes/Users/Detail/Tabs/Profile/UserDetailProfile.graphql(1 hunks)src/scenes/Users/Detail/Tabs/Profile/UserDetailProfile.tsx(1 hunks)src/scenes/Users/Detail/Tabs/Projects/UserDetailProjects.tsx(1 hunks)src/scenes/Users/Detail/Tabs/Projects/UserProjectPanel/UserProjectList.graphql(1 hunks)src/scenes/Users/Detail/Tabs/Projects/UserProjectPanel/UserProjectsPanel.tsx(1 hunks)src/scenes/Users/Detail/UserDetail.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/scenes/Users/Detail/Tabs/Projects/UserDetailProjects.tsx
- src/api/schema/typePolicies/typePolicies.base.ts
- src/scenes/Users/Detail/Tabs/Projects/UserProjectPanel/UserProjectsPanel.tsx
- src/scenes/Users/Detail/Tabs/Projects/UserProjectPanel/UserProjectList.graphql
- src/scenes/Users/Detail/Tabs/Profile/UserDetailProfile.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/scenes/Users/Detail/Tabs/Partners/UserDetailPartners.tsx (2)
src/components/Tabs/TabPanelContent.tsx (1)
TabPanelContent(6-22)src/scenes/Users/Detail/Tabs/Partners/UserPartnerPanel/UserPartnersPanel.tsx (1)
UserPartnersPanel(23-61)
src/scenes/Users/Detail/UserDetail.tsx (7)
src/hooks/useQueryParams.ts (3)
makeQueryHandler(183-247)withDefault(92-104)EnumParam(69-85)src/components/UserPhoto/UserPhoto.tsx (1)
UserPhoto(22-106)src/components/Tabs/TabsContainer.tsx (1)
TabsContainer(4-16)src/components/Tabs/Tab.tsx (1)
Tab(5-13)src/scenes/Users/Detail/Tabs/Profile/UserDetailProfile.tsx (1)
UserDetailProfile(28-105)src/scenes/Users/Detail/Tabs/Projects/UserDetailProjects.tsx (1)
UserDetailProjects(4-10)src/scenes/Users/Detail/Tabs/Partners/UserDetailPartners.tsx (1)
UserDetailPartners(4-10)
src/scenes/Users/Detail/Tabs/Partners/UserPartnerPanel/UserPartnersPanel.tsx (3)
src/components/Grid/useDataGridSource.tsx (1)
useDataGridSource(87-452)src/components/Grid/DefaultDataGridStyles.tsx (2)
DefaultDataGridStyles(36-80)flexLayout(98-138)src/components/PartnersDataGrid/PartnerColumns.tsx (3)
PartnerToolbar(109-120)PartnerColumns(27-94)PartnerInitialState(96-107)
🪛 Biome (2.1.2)
src/scenes/Users/Detail/UserDetail.tsx
[error] 8-8: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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). (1)
- GitHub Check: run
🔇 Additional comments (5)
src/scenes/Users/Detail/Tabs/Partners/UserPartnerPanel/UserPartnerList.graphql (1)
1-13: LGTM!The query structure is well-designed, following the established pattern for data grid queries with proper pagination fields and fragment composition.
src/scenes/Users/Detail/Tabs/Profile/UserDetailProfile.graphql (1)
1-4: LGTM!Clean fragment composition that reuses existing fragments, avoiding field duplication.
src/scenes/Users/Detail/Tabs/Partners/UserDetailPartners.tsx (1)
1-10: LGTM!Simple and consistent wrapper component following the same pattern as
UserDetailProjects.src/scenes/Users/Detail/Tabs/Partners/UserPartnerPanel/UserPartnersPanel.tsx (1)
23-61: LGTM!The component follows the established data grid pattern consistently with
UserProjectsPanel. The memoization of slots and slotProps is correctly implemented.src/scenes/Users/Detail/UserDetail.tsx (1)
22-24: Well-structured tab implementation with URL-based state management.The use of
makeQueryHandlerwithEnumParamfor persisting tab state in the URL is a clean pattern that enables deep linking and browser history support.Also applies to: 94-119
| {!!user?.partners.items.length && ( | ||
| <Tab label="Partners" value="partners" /> | ||
| )} | ||
| </TabList> | ||
| <TabPanel value="profile"> | ||
| {user && <UserDetailProfile user={user} />} | ||
| </TabPanel> | ||
| <TabPanel value="projects"> | ||
| <UserDetailProjects /> | ||
| </TabPanel> | ||
| {!!user?.partners.items.length && ( | ||
| <TabPanel value="partners"> | ||
| <UserDetailPartners /> | ||
| </TabPanel> | ||
| )} |
There was a problem hiding this comment.
Handle edge case when partners tab is selected but user has no partners.
If a user navigates directly to ?tab=partners via URL but the user has no partners, the TabContext will have value="partners" with no matching Tab or TabPanel rendered, potentially causing a blank content area.
Consider resetting the tab to 'profile' when partners are empty but the tab is set to 'partners':
+ useEffect(() => {
+ if (filters.tab === 'partners' && !user?.partners.items.length) {
+ setFilters({ tab: 'profile' });
+ }
+ }, [filters.tab, user?.partners.items.length, setFilters]);Alternatively, always render the Partners tab/panel but show an empty state when there are no partners.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/scenes/Users/Detail/UserDetail.tsx around lines 103-117, the current
logic conditionally renders the Partners Tab and TabPanel only when
user?.partners.items.length is truthy, which leaves TabContext.value="partners"
unmatched (blank) if the URL sets ?tab=partners but the user has no partners;
update the component to detect when the active tab is "partners" and partners
list is empty and programmatically reset the active tab to "profile" (or
fallback to a valid tab) OR always render the Partners TabPanel but show an
explicit empty-state message when partners are empty so the TabContext always
has a matching TabPanel for the active value.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {user && <UserDetailProfile user={user} />} | ||
| </TabPanel> | ||
| <TabPanel value="projects"> | ||
| <UserDetailProjects /> |
There was a problem hiding this comment.
The UserDetailProjects component is not passed the userId prop that it needs to fetch projects. While the component internally uses useParams() to get the userId, the GraphQL query requires this variable. Verify that the query is receiving the correct userId parameter from the URL params.
| <UserDetailProjects /> | |
| <UserDetailProjects userId={userId} /> |
| <DisplayProperty | ||
| label="Status" | ||
| value={user.status.value} | ||
| loading={!user} |
There was a problem hiding this comment.
The loading prop is set to !user which will always be false since the component requires user as a prop and TypeScript ensures it's not undefined. This prop should be removed or the component should handle loading state differently.
| <DisplayProperty | ||
| label="Gender" | ||
| value={labelFrom(GenderLabels)(user.gender.value)} | ||
| loading={!user} |
There was a problem hiding this comment.
The loading prop is set to !user which will always be false since the component requires user as a prop and TypeScript ensures it's not undefined. This prop should be removed or the component should handle loading state differently.
| <DisplayProperty | ||
| label="Email" | ||
| value={user.email.value} | ||
| loading={!user} |
There was a problem hiding this comment.
The loading prop is set to !user which will always be false since the component requires user as a prop and TypeScript ensures it's not undefined. This prop should be removed or the component should handle loading state differently.
| <DisplayProperty | ||
| label="Title" | ||
| value={user.title.value} | ||
| loading={!user} |
There was a problem hiding this comment.
The loading prop is set to !user which will always be false since the component requires user as a prop and TypeScript ensures it's not undefined. This prop should be removed or the component should handle loading state differently.
| <DisplayProperty | ||
| label="Roles" | ||
| value={labelsFrom(RoleLabels)(user.roles.value)} | ||
| loading={!user} |
There was a problem hiding this comment.
The loading prop is set to !user which will always be false since the component requires user as a prop and TypeScript ensures it's not undefined. This prop should be removed or the component should handle loading state differently.
| <LocalTime timezone={user.timezone.value.name} /> | ||
| ) : null | ||
| } | ||
| loading={!user} |
There was a problem hiding this comment.
The loading prop is set to !user which will always be false since the component requires user as a prop and TypeScript ensures it's not undefined. This prop should be removed or the component should handle loading state differently.
| <DisplayProperty | ||
| label="Phone" | ||
| value={user.phone.value} | ||
| loading={!user} |
There was a problem hiding this comment.
The loading prop is set to !user which will always be false since the component requires user as a prop and TypeScript ensures it's not undefined. This prop should be removed or the component should handle loading state differently.
| <DisplayProperty | ||
| label="About" | ||
| value={user.about.value} | ||
| loading={!user} |
There was a problem hiding this comment.
The loading prop is set to !user which will always be false since the component requires user as a prop and TypeScript ensures it's not undefined. This prop should be removed or the component should handle loading state differently.
2 participants
Adds a Projects table prefiltered by
userIdto the User detail and also creates a new tab list whereProfileis separated out fromProjects.