BA-1777: multiple profiles member list #135
Conversation
|
WalkthroughThe changes in this pull request introduce a new member management system within a React application. Key components include Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/components/modules/profiles/Members/MemberListItem/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (17)
packages/components/modules/profiles/Members/utils.ts (1)
1-2: LGTM! Consider adding input validation for robustness.The implementation correctly capitalizes the first letter while converting the rest to lowercase. However, consider adding input validation for more robust error handling.
Consider this enhanced version:
-export const capitalizeFirstLetter = (string: string) => - string.charAt(0).toUpperCase() + string.slice(1).toLowerCase() +export const capitalizeFirstLetter = (text: string): string => { + if (!text) return ''; + return text.charAt(0).toUpperCase() + text.slice(1).toLowerCase(); +}The improvements include:
- More descriptive parameter name
- Explicit return type
- Null/empty string handling
- Block syntax for better readability
packages/components/modules/profiles/Members/constants.ts (2)
1-2: Add documentation for pagination constantsConsider adding JSDoc comments to explain the rationale behind these pagination values and their usage in the member list.
+/** Number of additional members to load when clicking "Load More" */ export const NUMBER_OF_MEMBERS_TO_LOAD_NEXT = 5 +/** Initial number of members to display when the list first loads */ export const NUMBER_OF_MEMBERS_ON_FIRST_LOAD = 10
3-7: LGTM! Consider adding enum documentationThe enum values are well-structured and follow consistent naming conventions. The inclusion of an 'inactive' status shows good forward thinking.
Consider adding JSDoc documentation to explain the enum's purpose and usage:
+/** + * Represents the possible states of a profile member + * - ACTIVE: Member has accepted the invitation and has full access + * - PENDING: Member has been invited but hasn't accepted yet + * - INACTIVE: Member's access has been revoked or they've left + */ export enum MemberStatuses { active = 'ACTIVE', pending = 'PENDING', inactive = 'INACTIVE', }packages/components/modules/profiles/Members/MemberItem/types.ts (2)
9-11: Consider removing the Hungarian notation prefix.The interface name uses the 'I' prefix (Hungarian notation), which is not a common convention in modern TypeScript. Consider renaming
IMemberPersonalInformationtoMemberPersonalInformationto align with contemporary TypeScript naming conventions.-export interface IMemberPersonalInformation extends BoxProps { +export interface MemberPersonalInformation extends BoxProps { isActive: boolean }
1-20: Consider type composition for better reusability.The types are well-structured, but consider extracting common member-related types (like the role and status) into a shared types file if they're used across multiple components. This would improve maintainability and reusability.
packages/components/modules/profiles/Members/MemberItem/styled.tsx (1)
5-11: Consider adding semantic HTML and accessibility attributes.While the layout implementation is clean, consider enhancing accessibility by:
- Using semantic HTML through the
componentprop- Adding appropriate ARIA attributes
export const MemberItemContainer = styled(Box)(({ theme }) => ({ + // Forward component prop to render as a semantic list item + component: 'li', display: 'flex', gap: theme.spacing(1.5), alignItems: 'center', justifyContent: 'space-between', padding: theme.spacing(1.5, 0), + // Add role for accessibility + role: 'listitem', }))packages/design-system/components/inputs/TextField/index.tsx (1)
12-12: Nice optimization of the media query implementation!The direct usage of the theme callback in useMediaQuery is more efficient than separate useTheme + useMediaQuery calls while maintaining the same behavior.
Consider adding a brief comment explaining the breakpoint behavior for future maintainers:
+ // Switches to small size on viewports smaller than 'sm' breakpoint (600px by default) const isMobile = useMediaQuery<Theme>((theme) => theme.breakpoints.down('sm'))packages/components/modules/profiles/Members/types.ts (2)
8-14: Add JSDoc documentation and consider prop refinements.While the interface is well-structured, consider these improvements:
- Add JSDoc documentation to describe the purpose and usage of each prop
- Consider adding validation for
membersContainerHeight(e.g., minimum height)- The
LoadingStatePropsseems redundant sinceLoadingStatealready uses these propsConsider this improvement:
+/** + * Props for the MemberList component + * @property MemberItem - Component to render individual member items + * @property meRef - GraphQL fragment reference for user data + * @property LoadingState - Component to render loading state + * @property LoadingStateProps - Props for loading state + * @property membersContainerHeight - Optional height for the members container (in pixels) + */ export interface MemberListProps { MemberItem: FC<MemberItemProps> meRef: UserMembersListFragment$key LoadingState: FC<LoadingStateProps> - LoadingStateProps: LoadingStateProps + LoadingStateProps: LoadingStateProps + membersContainerHeight?: number & { __brand: 'positive' } // Type refinement for positive numbers - membersContainerHeight?: number }
26-26: LGTM! Consider adding documentation.Good use of TypeScript's utility types to create a variant of MemberListProps without the GraphQL reference.
Add documentation to clarify the purpose:
+/** Props for UserMembers component, excluding GraphQL reference */ export interface UserMembersProps extends Omit<MemberListProps, 'meRef'> {}packages/components/modules/profiles/graphql/queries/UserMembersList.ts (1)
16-17: Consider implementing cursor caching for better UXTo optimize the pagination experience:
- Consider implementing cursor caching to maintain list position during navigation
- Monitor the default page size of 10 in production - adjust based on actual usage patterns
- Implement virtual scrolling if the total member count tends to be large
This will help maintain smooth performance as the member list grows.
Also applies to: 22-24
packages/components/modules/profiles/Members/MemberItem/index.tsx (2)
15-22: Consider adding prop validationThe props structure looks good, but consider adding runtime validation for critical props like
memberandstatus. This would help catch issues earlier in development.+import PropTypes from 'prop-types' const MemberItem: FC<MemberItemProps> = ({ member, memberRole, status, avatarProps = {}, avatarWidth = 40, avatarHeight = 40, }) => { // ... component implementation } +MemberItem.propTypes = { + member: PropTypes.object.isRequired, + memberRole: PropTypes.string.isRequired, + status: PropTypes.oneOf(Object.values(MemberStatuses)).isRequired, + avatarProps: PropTypes.object, + avatarWidth: PropTypes.number, + avatarHeight: PropTypes.number, +}
23-24: Consider adding error boundary or fallback UIThe null check is good, but consider adding a fallback UI or error boundary to handle cases where member data might be invalid or incomplete.
-if (!member) return null +if (!member) return <Typography color="error">Invalid member data</Typography>packages/components/modules/profiles/Members/index.tsx (2)
16-26: Consider using a more predictable key pattern.While using the array index as a key is acceptable for static lists, consider using a more predictable pattern for better React reconciliation.
- key={index} // eslint-disable-line react/no-array-index-key + key={`member-skeleton-${index}`} // More descriptive and still deterministic
66-73: Consider adding an error boundary.Since this component uses Suspense for loading states, consider wrapping it with an error boundary to handle potential rendering errors gracefully.
// In a separate ErrorBoundary component class MembersErrorBoundary extends React.Component<{children: React.ReactNode}> { // Implementation of error boundary } // Usage in this component <MembersErrorBoundary> <Suspense fallback={<InitialLoadingState />}> <Members {...props} /> </Suspense> </MembersErrorBoundary>packages/components/modules/profiles/Members/MembersList/index.tsx (2)
14-24: Enhance type safety for component props.Consider improving type safety by:
- Explicitly defining the type for
meRefinstead of implicitly typing it- Creating an interface for
LoadingStatePropsto better type the accepted properties- LoadingStateProps = {}, + LoadingStateProps: Partial<DefaultLoadingState['props']> = {},
84-88: Clarify member count calculation.The addition of 1 to the total count (for the owner) could be more explicit:
- {(data.profile?.members?.totalCount ?? 0) + 1} members + const totalMembers = (data.profile?.members?.totalCount ?? 0) + 1; // Including owner + {totalMembers} {totalMembers === 1 ? 'member' : 'members'}packages/components/package.json (1)
24-109: Consider using caret ranges consistentlyWhile most dependencies use caret ranges (^), some don't. Consider using them consistently across all dependencies for better maintenance, particularly for:
reactandreact-dompeer dependencies- dev dependencies like
css-loader
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
packages/components/__generated__/UserMembersListFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/UserMembersListPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/userMembersListPaginationRefetchable.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
packages/components/modules/profiles/Members/MemberItem/index.tsx(1 hunks)packages/components/modules/profiles/Members/MemberItem/styled.tsx(1 hunks)packages/components/modules/profiles/Members/MemberItem/types.ts(1 hunks)packages/components/modules/profiles/Members/MembersList/index.tsx(1 hunks)packages/components/modules/profiles/Members/constants.ts(1 hunks)packages/components/modules/profiles/Members/index.tsx(1 hunks)packages/components/modules/profiles/Members/styled.tsx(1 hunks)packages/components/modules/profiles/Members/types.ts(1 hunks)packages/components/modules/profiles/Members/utils.ts(1 hunks)packages/components/modules/profiles/graphql/queries/UserMembersList.ts(1 hunks)packages/components/modules/profiles/index.ts(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(57 hunks)packages/design-system/components/inputs/TextField/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/schema.graphql
🔇 Additional comments (21)
packages/components/modules/profiles/index.ts (1)
6-6: LGTM! Export follows consistent patterns.
The export statement is well-placed and follows the established pattern for component exports in this module.
Let's verify the Members component exists at the specified path:
✅ Verification successful
✅ Export statement is correctly implemented
The verification confirms that:
- The Members component exists at
packages/components/modules/profiles/Members/index.tsx - It properly exports a default export as
MembersSuspended - The export statement in the barrel file correctly references this component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Members component exists and exports correctly
# Check if the Members component file exists
fd --type f "^index\.(tsx?|jsx?)$" --parent-directory "Members" packages/components/modules/profiles
# Verify the component exports a default export
ast-grep --pattern 'export default $_' packages/components/modules/profiles/Members/index.ts*
Length of output: 573
Script:
#!/bin/bash
# Let's verify the Members component existence and structure
# First, check if the file exists
fd "^Members/index\.tsx?$" packages/components/modules/profiles
# Then examine its content to understand the component structure
rg -A 5 "^export default" packages/components/modules/profiles/Members/index.tsx
Length of output: 180
packages/components/modules/profiles/Members/styled.tsx (2)
1-1: LGTM! Clean import statement.
The imports are minimal and correctly sourced from Material-UI.
3-7: LGTM! Well-structured skeleton component.
The MemberItemSkeleton component follows Material-UI best practices:
- Uses theme spacing for consistent border radius
- Appropriate height for a list item
- Responsive width handling
Let's verify the consistency of skeleton usage across member-related components:
✅ Verification successful
Let me gather more information about the member-related components and their skeleton implementations.
Skeleton implementation is consistent across components
The verification shows that both MemberItemSkeleton and ProfileMenuItemSkeleton follow identical implementation patterns:
- Same height (52px)
- Same responsive width (100%)
- Same theme-based border radius (theme.spacing(0.75))
This consistency in skeleton styling maintains a uniform loading state appearance throughout the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent skeleton implementation across member components
# Expected: Find related member components and their loading states
# Search for member-related components
rg -t tsx "MemberItem|MemberList" --glob "packages/components/modules/profiles/**"
# Check for other skeleton implementations
ast-grep --pattern 'styled(Skeleton)'
Length of output: 444
Script:
#!/bin/bash
# Search for member-related components with proper file extension
rg -t typescript -t javascriptreact -t typescriptreact "MemberItem|MemberList" --glob "packages/components/modules/profiles/**" -A 5
# Check the implementation of ProfileMenuItemSkeleton for consistency
cat packages/components/modules/profiles/ProfilePopover/ProfilesList/LoadingState/styled.tsx
Length of output: 460
packages/components/modules/profiles/Members/MemberItem/types.ts (2)
1-7: LGTM! Well-organized imports with proper type safety.
The imports are properly structured, utilizing both Material-UI types and generated GraphQL fragment types for type safety.
13-20: Verify null handling and document special cases.
The interface looks well-structured, but there are a few points to consider:
- The
memberprop allows null/undefined values. Ensure proper null checks are implemented in the component. - The special case of
'owner'inmemberRoleshould be documented for clarity.
✅ Verification successful
Null handling is properly implemented
The implementation in MemberItem/index.tsx and its usage in MembersList/index.tsx show proper null handling:
MemberItemcomponent has an explicit null check:if (!member) return null- The parent
MembersListcomponent safely accesses nested properties using optional chaining:member?.user?.profile - The 'owner' role case is used consistently in the codebase for the profile owner
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify null handling in MemberItem component implementation
# Expected: Find null checks for the member prop
# Search for the component implementation
ast-grep --pattern 'const MemberItem = ({ member, memberRole }: MemberItemProps) => {
$$$
}'
# Search for null checks
rg -A 5 'member(\s+)===(\s+)null|member(\s+)\?|member(\s+)&&'
Length of output: 160
Script:
#!/bin/bash
# Let's try a broader search for the MemberItem component implementation
fd "MemberItem" --type f --exec cat {}
# Also search for any usage of the member prop with different patterns
rg "member[^A-Za-z]" -A 5
Length of output: 8931
packages/components/modules/profiles/Members/MemberItem/styled.tsx (2)
1-3: LGTM! Clean and focused imports.
The imports are well-organized and specific to the needs of the file.
13-21: Enhance visual feedback and accessibility.
The current implementation has two potential improvements:
- The opacity value of 0.6 might not provide sufficient contrast for accessibility. Consider using a higher value or a different visual treatment.
- Adding a transition effect would make the opacity change smoother.
export const MemberPersonalInformation = styled(Box, {
shouldForwardProp: (prop) => prop !== 'isActive',
})<IMemberPersonalInformation>(({ isActive, theme }) => ({
- opacity: isActive ? 1 : 0.6,
+ opacity: isActive ? 1 : 0.75, // Increased for better contrast
+ transition: theme.transitions.create('opacity'), // Smooth transition
display: 'flex',
gap: theme.spacing(1.5),
alignItems: 'center',
justifyContent: 'space-between',
}))packages/components/modules/profiles/Members/types.ts (1)
1-6: LGTM! Well-organized imports with clear separation of concerns.
The imports are properly structured, separating React types, design system components, and local/generated types.
packages/components/modules/profiles/graphql/queries/UserMembersList.ts (2)
22-41: LGTM! Well-implemented pagination setup
The pagination implementation follows Relay best practices:
- Proper use of @connection directive with a unique key
- Includes necessary pageInfo fields
- Correct cursor-based pagination structure
- Appropriate filtering setup
3-10: Verify valid values for orderByStatus parameter
The query accepts an orderByStatus parameter, but we should verify the allowed values to prevent runtime errors.
✅ Verification successful
The orderByStatus parameter is correctly implemented
The verification shows that:
- The
orderByStatusparameter is properly defined in the schema as a String type - It has a default value of "custom" in the fragment
- It's used consistently across the codebase for member list ordering
- The parameter is properly connected to Relay's @connection directive with appropriate filters
The implementation follows GraphQL and Relay best practices, and no issues were found with the parameter usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GraphQL schema definitions or usage of orderByStatus
rg -t graphql "enum.*Status|orderByStatus.*=|orderByStatus.*:.*String" -A 5
Length of output: 109
Script:
#!/bin/bash
# Search for GraphQL schema definitions or usage of orderByStatus in all file types
rg "enum.*Status|orderByStatus.*=|orderByStatus.*:.*String" -A 5
# Also search for any GraphQL schema files
fd "\.(graphql|gql)$"
Length of output: 6257
packages/components/modules/profiles/Members/MemberItem/index.tsx (2)
53-53: LGTM!
The component export follows React conventions.
1-14: Consider organizing imports for better maintainability
Consider organizing imports into the following groups:
- React/External libraries
- Material-UI components
- Internal components/utilities
- Types/constants
+// External libraries
import { FC } from 'react'
+import { readInlineData } from 'react-relay'
+// Design system
import { AvatarWithPlaceholder } from '@baseapp-frontend/design-system'
+// Material UI
import { Box, Button, Typography } from '@mui/material'
-import { readInlineData } from 'react-relay'
+// Types and fragments
import { ProfileItemFragment$key } from '../../../../__generated__/ProfileItemFragment.graphql'
import { ProfileItemFragment } from '../../graphql/queries/ProfileItem'
+
+// Constants and utilities
import { MemberStatuses } from '../constants'
import { capitalizeFirstLetter } from '../utils'
import { MemberItemContainer, MemberPersonalInformation } from './styled'
import { MemberItemProps } from './types'Let's verify the relative import paths are correct:
✅ Verification successful
Based on the search results, I can now generate the final response:
Import paths are correctly structured and files exist
The verification confirms that all imported files exist in the codebase:
ProfileItemFragment.graphql.tsexists atpackages/components/__generated__/ProfileItemFragment.graphql.tsstyled.tsxexists atpackages/components/modules/profiles/Members/MemberItem/styled.tsxtypes.tsexists atpackages/components/modules/profiles/Members/MemberItem/types.ts- The relative paths in the imports correctly reflect the project structure
While the suggested import organization would improve readability, all current imports are valid and functional.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify import paths exist
# Expected: All imported files should exist
# Check for ProfileItemFragment
fd -e ts -e tsx "ProfileItemFragment.graphql.ts"
# Check for styled components
fd -e ts -e tsx "styled.tsx" --full-path ".*profiles/Members/MemberItem/"
# Check for types
fd -e ts -e tsx "types.ts" --full-path ".*profiles/Members/MemberItem/"
Length of output: 495
Script:
#!/bin/bash
# Let's try a different approach to verify the imports
# Check for all files in the profiles/Members directory
fd -e ts -e tsx . --full-path ".*/profiles/Members"
# Search for styled components file
fd "styled.tsx"
# Search for types file
fd "types.ts"
# Verify the graphql query path
fd "ProfileItem" --type f
# Check the actual content structure
find . -type d | grep -i "profiles"
Length of output: 11122
packages/components/modules/profiles/Members/index.tsx (3)
1-14: LGTM! Well-organized imports following best practices.
The imports are properly structured with clear separation between external and internal dependencies. The use of specific imports from React and proper typing from GraphQL generated files is commendable.
50-75: Verify the default height against design specifications.
The default membersContainerHeight of 400px should be verified against the design specifications mentioned in the PR objectives. Additionally, consider extracting this value to a constants file for better maintainability.
+ // In constants.ts
+ export const DEFAULT_MEMBERS_CONTAINER_HEIGHT = 400
+
// In this file
- membersContainerHeight = 400,
+ membersContainerHeight = DEFAULT_MEMBERS_CONTAINER_HEIGHT,34-37: Verify the 'custom' order implementation.
The orderByStatus: 'custom' parameter suggests a specific ordering implementation. Please ensure this matches the backend implementation.
✅ Verification successful
Let me search for more context about the ordering implementation.
Let me search for more specific implementation details about the member ordering.
Let me try again with correct file type flags.
Based on the gathered information, I can now provide a final response:
The 'custom' ordering parameter is properly implemented
The orderByStatus: 'custom' parameter is correctly used in the GraphQL query and is properly integrated with Relay's pagination system. The implementation shows:
- It's a default value in the GraphQL schema
- It's used consistently in the member listing query
- It's properly handled by Relay's connection configuration with appropriate filters
The parameter is used alongside standard pagination parameters (count, cursor) and is properly typed in the generated GraphQL types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of custom ordering
ast-grep --pattern 'enum $_OrderByStatus {
$$$
custom
$$$
}'
# Search for references to custom ordering
rg -A 5 "orderByStatus.*custom"
Length of output: 2775
Script:
#!/bin/bash
# Search for enum definitions related to member status or ordering
ast-grep --pattern 'enum $_ {
$$$
}'
# Search for any member status or ordering related types/interfaces
rg -A 5 "type.*Member|interface.*Member|enum.*Member"
# Look for any schema definitions that might contain the ordering logic
fd "schema.ts|schema.graphql" --exec cat {}
Length of output: 44716
Script:
#!/bin/bash
# Search for member ordering related code
rg -A 5 "orderByStatus" --type ts --type tsx
# Look for any constants or configurations related to member ordering
rg -A 5 "NUMBER_OF_MEMBERS_ON_FIRST_LOAD|custom.*order" --type ts --type tsx
Length of output: 179
Script:
#!/bin/bash
# Search for member ordering related code
rg -A 5 "orderByStatus"
# Look for any constants or configurations related to member ordering
rg -A 5 "NUMBER_OF_MEMBERS_ON_FIRST_LOAD"
# Search for any member-related constants
fd "constants.ts" --exec cat {}
Length of output: 20206
packages/components/modules/profiles/Members/MembersList/index.tsx (3)
1-13: LGTM! Well-organized imports.
The imports are properly organized with clear separation between external and internal dependencies.
89-102: LGTM! Well-implemented virtualization.
The Virtuoso configuration is properly set up with:
- Appropriate overscan for smooth scrolling
- Efficient endReached callback
- Good use of virtualization for performance
1-107: Verify handling of pending invitations.
The implementation aligns well with the PR objectives, but let's verify the handling of pending invitations:
✅ Verification successful
Pending invitations are properly handled
The implementation correctly handles pending invitations:
MemberStatusesenum includespending = 'PENDING'statusMemberItemcomponent displays pending status by capitalizing it in a button whenstatus !== MemberStatuses.active- The main list component properly renders all members regardless of their status, with special handling for active members (showing divider)
- The virtualized list ensures efficient rendering of both active and pending members
The code meets the requirements for handling pending invitations without any issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for pending invitation handling in the codebase
rg -l "MemberStatuses.pending" --type ts
Length of output: 40
Script:
#!/bin/bash
# Let's first check the MemberStatuses enum definition
rg -A 5 "export enum MemberStatuses" --type ts
# Also check for any other status-related handling
rg -A 5 "status.*=.*pending" --type ts
rg -A 5 "status.*===.*pending" --type ts
# Let's see the constants file mentioned in the import
cat "packages/components/modules/profiles/Members/constants.ts"
Length of output: 784
Script:
#!/bin/bash
# Let's check how MemberItem handles different statuses
ast-grep --pattern 'interface $_ {
$$$
status: $_
$$$
}'
# Also check MemberItem implementation
fd "MemberItem" --type f --exec cat {}
Length of output: 879
Script:
#!/bin/bash
# Let's check the actual implementation of MemberItem component
cat packages/components/modules/profiles/Members/MemberItem/index.tsx
# And check if there are any specific UI elements for pending status
rg -A 5 "pending" packages/components/modules/profiles/Members/MemberItem/
Length of output: 1870
packages/components/package.json (3)
47-48: Verify React 18.3.1 compatibility
The update to React 18.3.1 is significant. Ensure all components are compatible with this version, particularly:
- Concurrent features usage
- Strict mode compliance
- Effect cleanup implementations
#!/bin/bash
# Description: Check for React compatibility issues
# Search for potential compatibility issues
echo "Checking for legacy lifecycle methods..."
rg "componentWillMount|componentWillReceiveProps|componentWillUpdate"
echo "Checking for strict mode issues..."
rg "findDOMNode|UNSAFE_"
echo "Checking effect cleanup patterns..."
ast-grep --pattern 'useEffect(() => { $$$ }, $$$)'51-109: Verify testing infrastructure compatibility
The comprehensive update of testing dependencies requires verification:
- Jest 29.7.0 configuration compatibility
- Cypress 13.x integration
- Storybook 8.x setup
#!/bin/bash
# Description: Verify testing configuration compatibility
# Check Jest configuration
echo "Checking Jest configuration..."
rg "jest.config"
# Check Cypress configuration
echo "Checking Cypress configuration..."
rg "cypress.config"
# Check Storybook configuration
echo "Checking Storybook configuration..."
rg "\.storybook.*\.(js|ts)"24-40: Verify compatibility of major version updates
Several critical dependencies have significant version updates that warrant careful verification:
@tanstack/react-queryv5: Major version update that may have breaking changes@mui/laband@mui/material: Ensure UI components remain consistent- Latest versions of form handling libraries (
react-hook-form,zod)
✅ Verification successful
Dependencies update appears safe to proceed
Based on the codebase analysis:
-
React Query usage is primarily in authentication flows and follows standard patterns (useQuery/useMutation). The v5 upgrade should be manageable as the usage is well-contained.
-
MUI components are extensively used but mainly for basic components (Button, TextField, Dialog, etc.) which are stable across versions. The @mui/lab usage is minimal (only 2 files).
-
Form handling libraries (react-hook-form and zod) are used consistently with resolvers pattern, and the version updates are minor/patch.
Recommendations:
- Run the test suite after the update
- Pay special attention to authentication flows during QA
- Review MUI component styling, especially in the design-system package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in major dependency updates
# Check changelogs for breaking changes
echo "Checking @tanstack/react-query changelog for breaking changes..."
curl -s "https://raw.githubusercontent.com/TanStack/query/main/CHANGELOG.md" | head -n 50
echo "Checking MUI changelog for breaking changes..."
curl -s "https://raw.githubusercontent.com/mui/material-ui/master/CHANGELOG.md" | head -n 50
# Search for existing query usage patterns that might be affected
echo "Checking usage of react-query hooks..."
rg -A 2 "useQuery|useMutation|useInfiniteQuery"
Length of output: 3003
Script:
#!/bin/bash
# Let's check for actual usage of react-query and MUI components to assess impact
# Check for react-query usage patterns
echo "Checking react-query usage patterns..."
rg -l "from '@tanstack/react-query'"
# Check for specific react-query hooks usage
echo -e "\nChecking specific query hooks..."
rg -A 2 "useQuery\(|useMutation\(|useInfiniteQuery\("
# Check MUI component usage
echo -e "\nChecking MUI component usage..."
rg -l "from '@mui/material'"
rg -l "from '@mui/lab'"
# Check for specific form handling patterns
echo -e "\nChecking form handling patterns..."
rg -l "from 'react-hook-form'"
rg -l "from '@hookform/resolvers/zod'"
Length of output: 17819
| export interface UserMembersSuspendedProps { | ||
| MemberItem?: FC<MemberItemProps> | ||
| LoadingState?: FC<LoadingStateProps> | ||
| LoadingStateProps?: LoadingStateProps | ||
| title?: string | ||
| subtitle?: string | ||
| InitialLoadingState?: FC | ||
| membersContainerHeight?: number | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for InitialLoadingState and string props.
The interface could benefit from stronger typing:
InitialLoadingStateshould specify its props type- String props could benefit from more specific types or validation
Consider these improvements:
+/** Props for the suspended state of UserMembers component */
export interface UserMembersSuspendedProps {
MemberItem?: FC<MemberItemProps>
LoadingState?: FC<LoadingStateProps>
LoadingStateProps?: LoadingStateProps
- title?: string
- subtitle?: string
+ title?: NonEmptyString // Custom type to ensure non-empty string
+ subtitle?: NonEmptyString
- InitialLoadingState?: FC
+ InitialLoadingState?: FC<{ isLoading: boolean }> // Specify expected props
membersContainerHeight?: number
}
+// Add this type to ensure non-empty strings
+type NonEmptyString = string & { __brand: 'nonEmpty' }Committable suggestion skipped: line range outside the PR's diff.
| orderByStatus: { type: "String", defaultValue: "custom" } | ||
| ) { | ||
| profile { | ||
| ...ProfileItemFragment |
There was a problem hiding this comment.
Import missing ProfileItemFragment
The fragment references ProfileItemFragment but there's no corresponding import statement.
Add the following import at the top of the file:
import { graphql } from 'react-relay'
+import { ProfileItemFragment } from '../fragments/ProfileItemFragment'Also applies to: 30-30
| <Box> | ||
| <Button variant="soft" color="inherit"> | ||
| {status === MemberStatuses.active | ||
| ? capitalizeFirstLetter(memberRole) | ||
| : capitalizeFirstLetter(status)} | ||
| </Button> | ||
| </Box> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance button accessibility
The button showing member role/status could benefit from improved accessibility and visual feedback.
<Box>
<Button
variant="soft"
color="inherit"
+ aria-label={`Member ${status === MemberStatuses.active ? 'role' : 'status'}`}
+ title={status === MemberStatuses.active
+ ? `Member role: ${capitalizeFirstLetter(memberRole)}`
+ : `Status: ${capitalizeFirstLetter(status)}`}
+ disabled={status !== MemberStatuses.active}
>
{status === MemberStatuses.active
? capitalizeFirstLetter(memberRole)
: capitalizeFirstLetter(status)}
</Button>
</Box>📝 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.
| <Box> | |
| <Button variant="soft" color="inherit"> | |
| {status === MemberStatuses.active | |
| ? capitalizeFirstLetter(memberRole) | |
| : capitalizeFirstLetter(status)} | |
| </Button> | |
| </Box> | |
| <Box> | |
| <Button | |
| variant="soft" | |
| color="inherit" | |
| aria-label={`Member ${status === MemberStatuses.active ? 'role' : 'status'}`} | |
| title={status === MemberStatuses.active | |
| ? `Member role: ${capitalizeFirstLetter(memberRole)}` | |
| : `Status: ${capitalizeFirstLetter(status)}`} | |
| disabled={status !== MemberStatuses.active} | |
| > | |
| {status === MemberStatuses.active | |
| ? capitalizeFirstLetter(memberRole) | |
| : capitalizeFirstLetter(status)} | |
| </Button> | |
| </Box> |
| const members = useMemo( | ||
| () => | ||
| data?.profile?.members?.edges.filter((edge) => edge?.node).map((edge) => edge?.node) || [], | ||
| [data?.profile?.members?.edges], | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and null handling in data processing.
The current implementation could benefit from:
- Explicit typing for the edge and node structure
- More explicit null checks in the filter condition
- const members = useMemo(
- () =>
- data?.profile?.members?.edges.filter((edge) => edge?.node).map((edge) => edge?.node) || [],
- [data?.profile?.members?.edges],
- )
+ const members = useMemo(
+ () =>
+ data?.profile?.members?.edges
+ .filter((edge): edge is NonNullable<typeof edge> =>
+ edge !== null && edge.node !== null)
+ .map((edge) => edge.node) || [],
+ [data?.profile?.members?.edges],
+ )📝 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 members = useMemo( | |
| () => | |
| data?.profile?.members?.edges.filter((edge) => edge?.node).map((edge) => edge?.node) || [], | |
| [data?.profile?.members?.edges], | |
| ) | |
| const members = useMemo( | |
| () => | |
| data?.profile?.members?.edges | |
| .filter((edge): edge is NonNullable<typeof edge> => | |
| edge !== null && edge.node !== null) | |
| .map((edge) => edge.node) || [], | |
| [data?.profile?.members?.edges], | |
| ) |
| const renderMemberItem = (member: any, index: number) => { | ||
| if (!member) return null | ||
| if ( | ||
| member.status === MemberStatuses.active && | ||
| members[index - 1]?.status !== MemberStatuses.active | ||
| ) { | ||
| return ( | ||
| <> | ||
| <Divider /> | ||
| <MemberItem member={data.profile} memberRole="owner" status={MemberStatuses.active} /> | ||
| <MemberItem | ||
| member={member?.user?.profile} | ||
| memberRole={member?.role} | ||
| status={member?.status} | ||
| /> | ||
| </> | ||
| ) | ||
| } | ||
| return ( | ||
| <MemberItem | ||
| member={member?.user?.profile} | ||
| memberRole={member?.role} | ||
| status={member?.status} | ||
| /> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor member rendering logic for better maintainability.
Several improvements could be made to the renderMemberItem function:
- Replace
anytype with proper interface for member parameter - Extract the owner member rendering logic to a separate component
- Simplify the conditional rendering logic
- const renderMemberItem = (member: any, index: number) => {
+ interface MemberNode {
+ user?: {
+ profile: Profile;
+ };
+ role: string;
+ status: MemberStatuses;
+ }
+
+ const renderMemberItem = (member: MemberNode, index: number) => {Committable suggestion skipped: line range outside the PR's diff.
hpmoreira05
requested review from
Hercilio1,
anicioalexandre and
deboracosilveira
| <Divider /> | ||
| <MemberItem member={data.profile} memberRole="owner" status={MemberStatuses.active} /> |
There was a problem hiding this comment.
You can add an if statement inside the return to avoid repeating the following component:
<MemberItem
member={member?.user?.profile}
memberRole={member?.role}
status={member?.status}
/>
| const DefaultInitialLoadingState: FC = () => ( | ||
| <> | ||
| {Array.from({ length: NUMBER_OF_MEMBERS_ON_FIRST_LOAD }).map((_, index) => ( | ||
| <MemberItemSkeleton | ||
| key={index} // eslint-disable-line react/no-array-index-key | ||
| variant="rectangular" | ||
| sx={{ mb: 0.5 }} | ||
| /> | ||
| ))} | ||
| </> | ||
| ) |
There was a problem hiding this comment.
The BA packages avoid having two different FCs in the same file. I suggest you to create a component file for the loading state.
There was a problem hiding this comment.
@hpmoreira05 let's move that to modules/profiles/Members/DefaultInitialLoadingState
There was a problem hiding this comment.
I would make a simpler loading state, maybe just using the component, and wait for the designers to have a skeleton, maybe on the next versions
packages/components/schema.graphql
Outdated
| @@ -1,26 +1,42 @@ | |||
| """Exposes a URL that specifies the behaviour of this scalar.""" | |||
There was a problem hiding this comment.
All comments were updated. I think that's an IDE configuration issue. I guess you didn't change this in propose, so I would revert these unneeded changes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/components/modules/profiles/Members/index.tsx (2)
40-40: Consider making orderByStatus configurable.The 'custom' value is hardcoded, which might limit the component's reusability.
Consider adding it to the component props with 'custom' as the default value:
+ type OrderByStatus = 'custom' | 'status' | 'name'; + interface UserMembersProps { + orderByStatus?: OrderByStatus; + // ... existing props + } const data = useLazyLoadQuery<IUserMembersListPaginationQuery>(UserMembersListPaginationQuery, { profileId: currentProfile?.id || '', count: NUMBER_OF_MEMBERS_ON_FIRST_LOAD, - orderByStatus: 'custom', + orderByStatus: orderByStatus || 'custom', })
62-62: Consider moving hardcoded values to constants.The default membersContainerHeight value (400) should be defined in the constants file for better maintainability.
Move this value to the constants file:
// constants.ts export const NUMBER_OF_MEMBERS_ON_FIRST_LOAD = ... + export const DEFAULT_MEMBERS_CONTAINER_HEIGHT = 400 // index.tsx + import { DEFAULT_MEMBERS_CONTAINER_HEIGHT } from './constants' const MembersSuspended: FC<UserMembersSuspendedProps> = ({ // ... - membersContainerHeight = 400, + membersContainerHeight = DEFAULT_MEMBERS_CONTAINER_HEIGHT, })packages/components/modules/profiles/Members/MembersList/index.tsx (2)
14-20: Add JSDoc documentation for the component props.Consider adding comprehensive documentation for the component's props to improve maintainability and developer experience.
+/** + * MembersList component displays a paginated list of members with virtualization + * @param {Object} props + * @param {any} props.userRef - Relay reference to the user data + * @param {FC} props.MemberItem - Component to render individual member items + * @param {FC} props.LoadingState - Component to render loading state + * @param {Object} props.LoadingStateProps - Props to pass to LoadingState component + * @param {number} props.membersContainerHeight - Height of the members container + */ const MembersList: FC<MemberListProps> = ({
97-101: Improve null safety in member count display.The current implementation could benefit from more explicit null handling.
- {data.members?.totalCount && ( + {(data.members?.totalCount ?? 0) > 0 && ( <Typography variant="subtitle2" mb={4}> - {(data.members?.totalCount ?? 0) + 1} members + {`${(data.members?.totalCount ?? 0) + 1} ${ + data.members?.totalCount === 0 ? 'member' : 'members' + }`} </Typography> )}packages/components/modules/profiles/graphql/queries/UserMembersList.ts (2)
3-16: Ensure consistent default value fororderByStatusIn the
UserMembersListPaginationQuery, the$orderByStatusvariable does not have a default value, whereas in theUserMembersListFragment, it defaults to"custom". To maintain consistency and prevent potential issues, consider adding a default value to$orderByStatusin the query.Apply this diff to set the default value:
query UserMembersListPaginationQuery( $count: Int = 10 $cursor: String - $orderByStatus: String + $orderByStatus: String = "custom" $profileId: ID! ) {
28-28: Align connection key with fragment name for consistencyThe connection key in the
@connectiondirective is"UserMembersFragment_members", but the fragment is namedUserMembersListFragment. For clarity and to avoid potential caching issues, it's advisable to match the connection key with the fragment name.Apply this diff to update the connection key:
@connection( - key: "UserMembersFragment_members", + key: "UserMembersListFragment_members", filters: ["orderByStatus"] ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
packages/components/__generated__/UserMembersListFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/UserMembersListPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/userMembersListPaginationRefetchable.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (4)
packages/components/modules/profiles/Members/MembersList/index.tsx(1 hunks)packages/components/modules/profiles/Members/index.tsx(1 hunks)packages/components/modules/profiles/Members/types.ts(1 hunks)packages/components/modules/profiles/graphql/queries/UserMembersList.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/modules/profiles/Members/types.ts
🔇 Additional comments (4)
packages/components/modules/profiles/Members/index.tsx (3)
1-16: LGTM! Well-organized imports.
The imports are properly structured and follow good practices by separating external and internal dependencies.
17-27: Move DefaultInitialLoadingState to a separate file.
Per the team's convention and previous review comments, functional components should be in separate files.
Consider moving this component to packages/components/modules/profiles/Members/LoadingState/index.tsx.
71-78: LGTM! Good use of Suspense.
The component properly implements Suspense for handling loading states and provides a customizable InitialLoadingState.
packages/components/modules/profiles/graphql/queries/UserMembersList.ts (1)
18-48: Fragment definition is well-structured
The UserMembersListFragment is properly defined with appropriate use of pagination, argument definitions, and relay directives. The inclusion of member details and nested fragments is correctly implemented.
| const data = useLazyLoadQuery<IUserMembersListPaginationQuery>(UserMembersListPaginationQuery, { | ||
| profileId: currentProfile?.id || '', | ||
| count: NUMBER_OF_MEMBERS_ON_FIRST_LOAD, | ||
| orderByStatus: 'custom', | ||
| }) |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Empty string is not a valid profileId input
Looking at the generated query file, the profileId parameter is defined as ID! (non-nullable ID type) in the GraphQL schema. Using an empty string as a fallback could lead to runtime errors. The component already has a null check for the query response (if (!data.profile) return null), but it should prevent the query from being executed with invalid input.
packages/components/modules/profiles/Members/index.tsx: Add guard clause before query execution
🔗 Analysis chain
Consider handling undefined currentProfile case.
The query is executed with an empty string when currentProfile is undefined, which might cause unnecessary GraphQL requests.
Consider adding a guard clause:
- const data = useLazyLoadQuery<IUserMembersListPaginationQuery>(UserMembersListPaginationQuery, {
- profileId: currentProfile?.id || '',
- count: NUMBER_OF_MEMBERS_ON_FIRST_LOAD,
- orderByStatus: 'custom',
- })
+ if (!currentProfile?.id) return null;
+
+ const data = useLazyLoadQuery<IUserMembersListPaginationQuery>(UserMembersListPaginationQuery, {
+ profileId: currentProfile.id,
+ count: NUMBER_OF_MEMBERS_ON_FIRST_LOAD,
+ orderByStatus: 'custom',
+ })The previous attempt didn't yield results, possibly due to the query being defined in a different format or location. Let's search for the query file and examine how it's used in the codebase to make an informed decision about the empty profileId handling.
Let's examine the generated query file to understand if empty string is a valid input, and check the component's implementation for existing error handling to make a more informed decision about the suggestion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the GraphQL query handles empty profileId
ast-grep --pattern 'query UserMembersListPaginationQuery($profileId: String!) {
$$$
}'
Length of output: 90
Script:
#!/bin/bash
# Let's first locate the GraphQL query file
fd -e graphql -e ts -e tsx UserMembersListPaginationQuery
# Then examine the query definition and its usage
rg -l "UserMembersListPaginationQuery" --type ts --type tsx
# Also check how currentProfile is used in the component
rg -A 5 "const.*currentProfile.*useCurrentProfile" packages/components/modules/profiles/Members/index.tsx
Length of output: 603
Script:
#!/bin/bash
# Let's check the generated query file to understand the schema requirements
cat packages/components/__generated__/UserMembersListPaginationQuery.graphql.ts
# Also check if there's any error handling or null checks after the query
rg -A 10 "useLazyLoadQuery.*UserMembersListPaginationQuery" packages/components/modules/profiles/Members/index.tsx
# Check if there are any other components using similar pattern
rg "profileId:.*\?\." --type ts --type tsx
Length of output: 10810
| const renderMemberItem = (member: any, index: number) => { | ||
| if (!member) return null | ||
| if ( | ||
| (member.status === MemberStatuses.active && | ||
| members[index - 1]?.status !== MemberStatuses.active) || | ||
| (member.status === MemberStatuses.active && !members[index - 1]?.status) | ||
| ) { | ||
| return ( | ||
| <> | ||
| <Divider /> | ||
| <MemberItem member={data} memberRole="owner" status={MemberStatuses.active} /> | ||
| <MemberItem | ||
| member={member?.user?.profile} | ||
| memberRole={member?.role} | ||
| status={member?.status} | ||
| /> | ||
| </> | ||
| ) | ||
| } | ||
| if (member.status !== MemberStatuses.active && !members[index + 1]?.status) { | ||
| return ( | ||
| <> | ||
| <MemberItem | ||
| member={member?.user?.profile} | ||
| memberRole={member?.role} | ||
| status={member?.status} | ||
| /> | ||
| <Divider /> | ||
| <MemberItem member={data} memberRole="owner" status={MemberStatuses.active} /> | ||
| </> | ||
| ) | ||
| } | ||
| return ( | ||
| <MemberItem | ||
| member={member?.user?.profile} | ||
| memberRole={member?.role} | ||
| status={member?.status} | ||
| /> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the member rendering logic for better maintainability.
The current implementation has complex conditional logic and repeated code. Consider refactoring to:
- Extract the owner member rendering into a separate component
- Simplify the conditions for showing dividers
- Use an early return pattern
- const renderMemberItem = (member: any, index: number) => {
+ const OwnerMember = () => (
+ <MemberItem member={data} memberRole="owner" status={MemberStatuses.active} />
+ )
+
+ const renderMemberItem = (member: any, index: number) => {
if (!member) return null
- if (
- (member.status === MemberStatuses.active &&
- members[index - 1]?.status !== MemberStatuses.active) ||
- (member.status === MemberStatuses.active && !members[index - 1]?.status)
- ) {
- return (
- <>
- <Divider />
- <MemberItem member={data} memberRole="owner" status={MemberStatuses.active} />
- <MemberItem
- member={member?.user?.profile}
- memberRole={member?.role}
- status={member?.status}
- />
- </>
- )
- }
- if (member.status !== MemberStatuses.active && !members[index + 1]?.status) {
- return (
- <>
- <MemberItem
- member={member?.user?.profile}
- memberRole={member?.role}
- status={member?.status}
- />
- <Divider />
- <MemberItem member={data} memberRole="owner" status={MemberStatuses.active} />
- </>
- )
- }
+
+ const showOwnerBefore = member.status === MemberStatuses.active &&
+ (!members[index - 1]?.status || members[index - 1]?.status !== MemberStatuses.active)
+
+ const showOwnerAfter = member.status !== MemberStatuses.active &&
+ !members[index + 1]?.status
+
+ if (!showOwnerBefore && !showOwnerAfter) {
+ return (
+ <MemberItem
+ member={member?.user?.profile}
+ memberRole={member?.role}
+ status={member?.status}
+ />
+ )
+ }
+
return (
- <MemberItem
- member={member?.user?.profile}
- memberRole={member?.role}
- status={member?.status}
- />
+ <>
+ {showOwnerBefore && (
+ <>
+ <Divider />
+ <OwnerMember />
+ </>
+ )}
+ <MemberItem
+ member={member?.user?.profile}
+ memberRole={member?.role}
+ status={member?.status}
+ />
+ {showOwnerAfter && (
+ <>
+ <Divider />
+ <OwnerMember />
+ </>
+ )}
+ </>
)
}📝 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 renderMemberItem = (member: any, index: number) => { | |
| if (!member) return null | |
| if ( | |
| (member.status === MemberStatuses.active && | |
| members[index - 1]?.status !== MemberStatuses.active) || | |
| (member.status === MemberStatuses.active && !members[index - 1]?.status) | |
| ) { | |
| return ( | |
| <> | |
| <Divider /> | |
| <MemberItem member={data} memberRole="owner" status={MemberStatuses.active} /> | |
| <MemberItem | |
| member={member?.user?.profile} | |
| memberRole={member?.role} | |
| status={member?.status} | |
| /> | |
| </> | |
| ) | |
| } | |
| if (member.status !== MemberStatuses.active && !members[index + 1]?.status) { | |
| return ( | |
| <> | |
| <MemberItem | |
| member={member?.user?.profile} | |
| memberRole={member?.role} | |
| status={member?.status} | |
| /> | |
| <Divider /> | |
| <MemberItem member={data} memberRole="owner" status={MemberStatuses.active} /> | |
| </> | |
| ) | |
| } | |
| return ( | |
| <MemberItem | |
| member={member?.user?.profile} | |
| memberRole={member?.role} | |
| status={member?.status} | |
| /> | |
| ) | |
| } | |
| const OwnerMember = () => ( | |
| <MemberItem member={data} memberRole="owner" status={MemberStatuses.active} /> | |
| ) | |
| const renderMemberItem = (member: any, index: number) => { | |
| if (!member) return null | |
| const showOwnerBefore = member.status === MemberStatuses.active && | |
| (!members[index - 1]?.status || members[index - 1]?.status !== MemberStatuses.active) | |
| const showOwnerAfter = member.status !== MemberStatuses.active && | |
| !members[index + 1]?.status | |
| if (!showOwnerBefore && !showOwnerAfter) { | |
| return ( | |
| <MemberItem | |
| member={member?.user?.profile} | |
| memberRole={member?.role} | |
| status={member?.status} | |
| /> | |
| ) | |
| } | |
| return ( | |
| <> | |
| {showOwnerBefore && ( | |
| <> | |
| <Divider /> | |
| <OwnerMember /> | |
| </> | |
| )} | |
| <MemberItem | |
| member={member?.user?.profile} | |
| memberRole={member?.role} | |
| status={member?.status} | |
| /> | |
| {showOwnerAfter && ( | |
| <> | |
| <Divider /> | |
| <OwnerMember /> | |
| </> | |
| )} | |
| </> | |
| ) | |
| } |
This reverts commit cfd9bf9.
… BA-1777-fe-multiple-profiles-member-list
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/components/modules/profiles/index.ts(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(60 hunks)packages/design-system/components/inputs/TextField/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/components/modules/profiles/index.ts
- packages/components/schema.graphql
- packages/design-system/components/inputs/TextField/index.tsx
🔇 Additional comments (1)
packages/components/package.json (1)
52-112: Verify compatibility of development dependencies.
The large number of development dependencies with caret (^) versions could lead to inconsistent development environments across the team. Consider using a package.json lock file or pinning versions.
packages/components/package.json
Outdated
| "@tailwindcss/typography": "^0.5.13", | ||
| "@testing-library/cypress": "^10.0.2", | ||
| "@testing-library/jest-dom": "^6.4.6", | ||
| "@testing-library/react": "^16.0.0", |
There was a problem hiding this comment.
Version mismatch between React and Testing Library.
@testing-library/react v16.0.0 is significantly older than your React v18.3.1. This version mismatch could lead to compatibility issues or missing features in your tests.
- "@testing-library/react": "^16.0.0",
+ "@testing-library/react": "^14.2.1"📝 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.
| "@testing-library/react": "^16.0.0", | |
| "@testing-library/react": "^14.2.1", |
packages/components/package.json
Outdated
| "graphql": "^16.8.1", | ||
| "js-cookie": "^3.0.5", | ||
| "luxon": "^3.4.4", | ||
| "next": "14.3.0-canary.24", |
There was a problem hiding this comment.
Avoid using Next.js canary versions in production.
The Next.js version 14.3.0-canary.24 is a pre-release version that may contain unstable features. Consider using a stable version instead.
- "next": "14.3.0-canary.24",
+ "next": "14.3.0"Committable suggestion skipped: line range outside the PR's diff.
packages/components/package.json
Outdated
| "@hookform/resolvers": "^3.6.0", | ||
| "@mui/lab": "^5.0.0-alpha.170", | ||
| "@mui/material": "^5.15.19", | ||
| "@mui/system": "^5.15.19", | ||
| "@storybook/react": "^8.2.8", | ||
| "@tanstack/react-query": "^5.45.1", | ||
| "framer-motion": "^11.5.4", | ||
| "graphql": "catalog:graphql", | ||
| "js-cookie": "catalog:", | ||
| "luxon": "catalog:", | ||
| "next": "catalog:", | ||
| "react-hook-form": "catalog:", | ||
| "react-relay": "catalog:graphql", | ||
| "react-virtuoso": "catalog:", | ||
| "relay-runtime": "catalog:graphql", | ||
| "graphql": "^16.8.1", | ||
| "js-cookie": "^3.0.5", | ||
| "luxon": "^3.4.4", | ||
| "next": "14.3.0-canary.24", | ||
| "react-hook-form": "^7.51.5", | ||
| "react-relay": "^16.2.0", | ||
| "react-virtuoso": "^4.7.11", | ||
| "relay-runtime": "^16.2.0", | ||
| "use-long-press": "^3.2.0", | ||
| "zod": "catalog:", | ||
| "zustand": "catalog:" | ||
| "zod": "^3.23.8", | ||
| "zustand": "^4.5.2" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider pinning critical dependencies.
Using caret (^) for critical dependencies like MUI, React Query, and GraphQL could lead to unexpected breaking changes. Consider pinning these versions to exact numbers for better stability.
Example changes:
- "@mui/material": "^5.15.19",
- "@mui/system": "^5.15.19",
- "@tanstack/react-query": "^5.45.1",
- "graphql": "^16.8.1",
+ "@mui/material": "5.15.19",
+ "@mui/system": "5.15.19",
+ "@tanstack/react-query": "5.45.1",
+ "graphql": "16.8.1",📝 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.
| "@hookform/resolvers": "^3.6.0", | |
| "@mui/lab": "^5.0.0-alpha.170", | |
| "@mui/material": "^5.15.19", | |
| "@mui/system": "^5.15.19", | |
| "@storybook/react": "^8.2.8", | |
| "@tanstack/react-query": "^5.45.1", | |
| "framer-motion": "^11.5.4", | |
| "graphql": "catalog:graphql", | |
| "js-cookie": "catalog:", | |
| "luxon": "catalog:", | |
| "next": "catalog:", | |
| "react-hook-form": "catalog:", | |
| "react-relay": "catalog:graphql", | |
| "react-virtuoso": "catalog:", | |
| "relay-runtime": "catalog:graphql", | |
| "graphql": "^16.8.1", | |
| "js-cookie": "^3.0.5", | |
| "luxon": "^3.4.4", | |
| "next": "14.3.0-canary.24", | |
| "react-hook-form": "^7.51.5", | |
| "react-relay": "^16.2.0", | |
| "react-virtuoso": "^4.7.11", | |
| "relay-runtime": "^16.2.0", | |
| "use-long-press": "^3.2.0", | |
| "zod": "catalog:", | |
| "zustand": "catalog:" | |
| "zod": "^3.23.8", | |
| "zustand": "^4.5.2" | |
| "@hookform/resolvers": "^3.6.0", | |
| "@mui/lab": "^5.0.0-alpha.170", | |
| "@mui/material": "5.15.19", | |
| "@mui/system": "5.15.19", | |
| "@storybook/react": "^8.2.8", | |
| "@tanstack/react-query": "5.45.1", | |
| "framer-motion": "^11.5.4", | |
| "graphql": "16.8.1", | |
| "js-cookie": "^3.0.5", | |
| "luxon": "^3.4.4", | |
| "next": "14.3.0-canary.24", | |
| "react-hook-form": "^7.51.5", | |
| "react-relay": "^16.2.0", | |
| "react-virtuoso": "^4.7.11", | |
| "relay-runtime": "^16.2.0", | |
| "use-long-press": "^3.2.0", | |
| "zod": "^3.23.8", | |
| "zustand": "^4.5.2" |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/components/modules/profiles/Members/MemberItem/index.tsx (2)
15-24: Consider explicit typing for default avatarPropsWhile the current implementation works, it would be more maintainable to explicitly type the default value.
- avatarProps = {}, + avatarProps: Partial<AvatarProps> = {},
36-40: Add fallbacks for missing data and improve URL displayConsider handling missing data gracefully and formatting the URL path for better readability.
<Box> - <Typography variant="subtitle2">{memberProfile.name}</Typography> - <Typography variant="caption">{memberProfile?.urlPath?.path}</Typography> + <Typography variant="subtitle2">{memberProfile.name || 'Unnamed Member'}</Typography> + <Typography variant="caption"> + {memberProfile?.urlPath?.path + ? `@${memberProfile.urlPath.path}` + : 'No profile URL'} + </Typography> </Box>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/components/modules/profiles/Members/MemberItem/index.tsx(1 hunks)
🔇 Additional comments (4)
packages/components/modules/profiles/Members/MemberItem/index.tsx (4)
1-14: LGTM! Well-organized imports and type declarations.
The imports are logically grouped and all necessary dependencies are properly imported.
28-35: Improve avatar placeholder handling
Instead of using an empty string as fallback for missing image URL, consider using the member's initials.
<AvatarWithPlaceholder
width={avatarWidth}
height={avatarHeight}
- src={memberProfile?.image?.url ?? ''}
+ src={memberProfile?.image?.url}
+ fallback={memberProfile?.name?.[0]?.toUpperCase()}
alt="Profile avatar"
color="secondary"
{...avatarProps}
/>42-48: Enhance button accessibility
The button showing member role/status needs improved accessibility and visual feedback.
<Box>
<Button
variant="soft"
color="inherit"
+ aria-label={`Member ${status === MemberStatuses.active ? 'role' : 'status'}`}
+ title={status === MemberStatuses.active
+ ? `Member role: ${capitalizeFirstLetter(memberRole)}`
+ : `Status: ${capitalizeFirstLetter(status)}`}
+ disabled={status !== MemberStatuses.active}
>
{status === MemberStatuses.active
? capitalizeFirstLetter(memberRole)
: capitalizeFirstLetter(status)}
</Button>
</Box>15-51: Verify integration with member list requirements
Let's ensure this component properly integrates with the scrollable member list requirement mentioned in the PR objectives.
✅ Verification successful
MemberItem component properly integrates with scrollable list requirements
The MemberItem component is correctly implemented for integration with the parent MembersList component, which uses react-virtuoso for efficient scrolling and virtualization. The MembersList component properly handles:
- Virtual scrolling with configurable container height
- Dynamic member rendering through itemContent prop
- Proper pagination with load-more functionality
- Consistent styling and layout of member items
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the parent MembersList component implements proper scrolling containment
ast-grep --pattern 'const MembersList = $_: FC<$_> => {
$$$
return (
<$_Container $$$>
$$$
style={$_}
$$$
</$_>
)
}'
Length of output: 150
Script:
#!/bin/bash
# Let's try a broader search for MembersList component and its container implementation
rg -l "MembersList" --type tsx
# Then let's look for any styled components or containers that might handle scrolling
rg -l "styled\(.+Container\)" --type tsx
Length of output: 129
Script:
#!/bin/bash
# Search for MembersList component without file type restriction
rg -l "MembersList"
# Look for styled containers in the same directory as MemberItem
fd -e ts -e tsx -e js -e jsx . packages/components/modules/profiles/Members/
Length of output: 1180
Script:
#!/bin/bash
# Let's check the MembersList implementation and its container styling
cat packages/components/modules/profiles/Members/MembersList/index.tsx
cat packages/components/modules/profiles/Members/styled.tsx
Length of output: 3670
| ProfileRoles, | ||
| } from '../../../../__generated__/UserMembersListFragment.graphql' | ||
|
|
||
| export interface IMemberPersonalInformation extends BoxProps { |
There was a problem hiding this comment.
let's avoid adding the I prefix
| if ( | ||
| (member.status === MemberStatuses.active && | ||
| members[index - 1]?.status !== MemberStatuses.active) || | ||
| (member.status === MemberStatuses.active && !members[index - 1]?.status) |
There was a problem hiding this comment.
could we move some of these conditions to a constant? just so we can read better what the rule is, something like:
const isActiveMember = member.status === MemberStatuses.active
const isPreviousMemberInactive = members[index - 1]?.status !== MemberStatuses.active
const isPreviousMemberUndefined = !members[index - 1]?.status
const isFirstActiveMember = (isActiveMember && isPreviousMemberInactive) || (isActiveMember && isPreviousMemberUndefined)
| const DefaultInitialLoadingState: FC = () => ( | ||
| <> | ||
| {Array.from({ length: NUMBER_OF_MEMBERS_ON_FIRST_LOAD }).map((_, index) => ( | ||
| <MemberItemSkeleton | ||
| key={index} // eslint-disable-line react/no-array-index-key | ||
| variant="rectangular" | ||
| sx={{ mb: 0.5 }} | ||
| /> | ||
| ))} | ||
| </> | ||
| ) |
There was a problem hiding this comment.
@hpmoreira05 let's move that to modules/profiles/Members/DefaultInitialLoadingState
|
|
||
| return ( | ||
| <> | ||
| {data.members?.totalCount && ( |
There was a problem hiding this comment.
I think we could simplify that a little bit
| {data.members?.totalCount && ( | |
| <Typography variant="subtitle2" mb={4}> | |
| {data.members?.totalCount ?? 1} members | |
| </Typography> |
|
|
||
| const MembersList: FC<MemberListProps> = ({ | ||
| userRef, | ||
| MemberItem = DefaultMemberItem, |
There was a problem hiding this comment.
we're missing MemberItemProps here
| const DefaultInitialLoadingState: FC = () => ( | ||
| <> | ||
| {Array.from({ length: NUMBER_OF_MEMBERS_ON_FIRST_LOAD }).map((_, index) => ( | ||
| <MemberItemSkeleton | ||
| key={index} // eslint-disable-line react/no-array-index-key | ||
| variant="rectangular" | ||
| sx={{ mb: 0.5 }} | ||
| /> | ||
| ))} | ||
| </> | ||
| ) |
There was a problem hiding this comment.
I would make a simpler loading state, maybe just using the component, and wait for the designers to have a skeleton, maybe on the next versions
| </MemberPersonalInformation> | ||
|
|
||
| <Box> | ||
| <Button variant="soft" color="inherit"> |
There was a problem hiding this comment.
let's remove the pointer-events here since it should not be interactive sx={{pointerEvents: "none"}}
| ) | ||
| } | ||
|
|
||
| const renderMemberItem = (member: any, index: number) => { |
There was a problem hiding this comment.
we can avoid the any by typing it like this:
// on Members/types.ts
type Members = NonNullable<UserMembersListFragment$data['members']>
type MembersEdges = Members['edges']
export type MemberNode = NonNullable<MembersEdges[number]>['node']
// on MembersList/index.tsx
const renderMemberItem = (member: MemberNode, index: number) => {
| avatarWidth = 40, | ||
| avatarHeight = 40, | ||
| }) => { | ||
| const memberProfile = useFragment<ProfileItemFragment$key>(ProfileItemFragment, member) |
There was a problem hiding this comment.
I think ideally this fragment should be related to members (aka ProfileUserRole on the backend) instead of profile fragment, we would use a MembersItemFragment. So the list query would be something like this:
export const UserMembersListFragment = graphql`
fragment UserMembersListFragment on Profile
@refetchable(queryName: "userMembersListPaginationRefetchable")
@argumentDefinitions(
count: { type: "Int", defaultValue: 10 }
cursor: { type: "String" }
orderByStatus: { type: "String", defaultValue: "custom" }
) {
members(first: $count, after: $cursor, orderByStatus: $orderByStatus)
@connection(key: "UserMembersFragment_members", filters: ["orderByStatus"]) {
totalCount
edges {
node {
...MemberItemFragment
}
}
pageInfo {
endCursor
hasNextPage
}
}
}
`
them the members fragment something like:
export const MemberItemFragment = graphql`
fragment MemberItemFragment on Member // need to create this interface
@argumentDefinitions(avatarSize: { type: "Int", defaultValue: 100 }) {
id
user {
profile {
...ProfileItemFragment
}
}
role
status
}
`
@nossila what do you think? I think we mentioned that on our last sidebar
There was a problem hiding this comment.
then if we do that, we avoid needing to pass over the member?.role and member?.status since we would define those fields on the MemberItem fragment
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/components/schema.graphql (1)
925-925: Verify the implementation of theorderByStatusargumentThe
membersfield in theProfiletype now accepts anorderByStatusargument. Make sure this argument is implemented correctly and that it aligns with the ordering conventions used elsewhere in the schema. Consider if a more genericorderByparameter might enhance flexibility.packages/components/modules/profiles/Members/MemberListItem/index.tsx (1)
18-75: Simplify conditional logic for better readabilityThe conditional statements determining
isFirstActiveMemberandisLastMemberInactivecan be streamlined to enhance code clarity.Consider simplifying the
isFirstActiveMembercondition:- const isFirstActiveMember = - (isActiveMember && isPreviousMemberInactive) || (isActiveMember && isPreviousMemberUndefined) + const isFirstActiveMember = isActiveMember && (!prevMemberFragment?.status || prevMemberFragment.status !== MemberStatuses.active)Similarly, refine
isLastMemberInactivefor consistency:- const isLastMemberInactive = !isActiveMember && isNextMemberUndefined + const isLastInactiveMember = !isActiveMember && (!nextMemberFragment?.status || nextMemberFragment.status === MemberStatuses.active)These changes will make the conditions more concise and easier to understand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
packages/components/__generated__/MemberItemFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/UserMembersListFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/UserMembersListPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/userMembersListPaginationRefetchable.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (11)
packages/components/modules/profiles/Members/MemberItem/index.tsx(1 hunks)packages/components/modules/profiles/Members/MemberItem/styled.tsx(1 hunks)packages/components/modules/profiles/Members/MemberItem/types.ts(1 hunks)packages/components/modules/profiles/Members/MemberListItem/index.tsx(1 hunks)packages/components/modules/profiles/Members/MemberListItem/types.ts(1 hunks)packages/components/modules/profiles/Members/MembersList/index.tsx(1 hunks)packages/components/modules/profiles/Members/index.tsx(1 hunks)packages/components/modules/profiles/Members/types.ts(1 hunks)packages/components/modules/profiles/graphql/queries/MemberItem.ts(1 hunks)packages/components/modules/profiles/graphql/queries/UserMembersList.ts(1 hunks)packages/components/schema.graphql(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/components/modules/profiles/Members/MemberItem/types.ts
- packages/components/modules/profiles/Members/MemberItem/styled.tsx
- packages/components/modules/profiles/Members/index.tsx
- packages/components/modules/profiles/graphql/queries/UserMembersList.ts
- packages/components/modules/profiles/Members/types.ts
- packages/components/modules/profiles/Members/MemberItem/index.tsx
🔇 Additional comments (8)
packages/components/schema.graphql (3)
700-700: Ensure consistent usage of the level argument in notifications fields
The notifications fields now include a level argument of type NotificationsNotificationLevelChoices in both the NotificationsInterface and User types. Verify that this change is consistently applied and that all relevant queries and mutations are updated accordingly.
Also applies to: 1400-1400
735-749: Confirm the introduction of the new notification levels enumeration
The NotificationsNotificationLevelChoices enumeration has been introduced, replacing BaseappNotificationsNotificationLevelChoices. Ensure that all related types and fields, such as Notification and notifications, are using the new enumeration correctly.
Use the script provided earlier to confirm that the old enumeration is no longer in use.
598-598: Verify the replacement of the level field enumeration
The level field in the Notification type now uses NotificationsNotificationLevelChoices instead of BaseappNotificationsNotificationLevelChoices. Ensure that all references to the old enumeration have been updated to the new one to maintain consistency.
Run the following script to check for any remaining references to the old enumeration:
packages/components/modules/profiles/graphql/queries/MemberItem.ts (1)
1-14: Fragment definition is correct and well-structured
The MemberItemFragment is properly defined on the ProfileUserRole type. It includes all necessary fields such as id, user, role, and status, and correctly incorporates the ProfileItemFragment spread within the profile field.
packages/components/modules/profiles/Members/MemberListItem/types.ts (1)
1-14: Type definitions are appropriately declared
The MemberListItemProps interface is well-defined, with all required properties accurately typed. The imports for MemberItemFragment$key, UserMembersListFragment$data, and MemberItemProps are correctly referenced, ensuring strong type safety.
packages/components/modules/profiles/Members/MembersList/index.tsx (3)
70-71: Ensure accurate member count display
The total number of members is calculated by adding 1 to account for the owner. Verifying that data.members?.totalCount is defined before adding improves reliability.
Consider updating the member count display:
- {(data.members?.totalCount ?? 0) + 1} members
+ {data.members?.totalCount ? data.members.totalCount + 1 : 1} membersThis change ensures that the count is accurate even if totalCount is undefined.
28-31: 🛠️ Refactor suggestion
Enhance type safety and explicit null handling
The current implementation of the members array could benefit from improved type safety and explicit null checks.
Consider updating the members derivation:
const members = useMemo(() => {
return (
data?.members?.edges
?.filter((edge): edge is NonNullable<typeof edge> => edge !== null && edge.node !== null)
.map((edge) => edge.node) || []
);
}, [data?.members?.edges]);This change explicitly checks for null values and refines the type, which improves reliability and reduces potential runtime errors.
45-54: 🛠️ Refactor suggestion
Specify the type for member in renderMemberItem function
The renderMemberItem function currently uses member: any, which weakens type safety.
Define a specific type for MemberNode and use it in the function signature:
type MemberNode = NonNullable<UserMembersListFragment$data['members']>['edges'][number]['node'];
const renderMemberItem = (member: MemberNode, index: number) => (
// function body
);This ensures that the member parameter is strongly typed, enhancing code robustness.
packages/components/schema.graphql
Outdated
| interface MemberInterface { | ||
| """The ID of the object""" | ||
| id: ID! | ||
| member: ProfileUserRole | ||
| } |
There was a problem hiding this comment.
Potential recursive type definition in MemberInterface
The MemberInterface includes a member field of type ProfileUserRole, which is the type that implements this interface. This creates a circular reference that may lead to infinite recursion or issues with schema parsing and code generation tools.
Consider refactoring the member field to prevent the recursive self-reference. You might rename the field or change its type to avoid the circular dependency.
packages/components/schema.graphql
Outdated
| type ProfileUserRole implements MemberInterface { | ||
| """The ID of the object""" | ||
| id: ID! | ||
| user: User! | ||
| role: ProfileRoles | ||
| status: ProfileRoleStatus | ||
| member: ProfileUserRole |
There was a problem hiding this comment.
Avoid recursive self-reference in ProfileUserRole type
The ProfileUserRole type includes a member field of type ProfileUserRole, causing a recursive type definition. This can lead to infinite loops or errors in schema parsing tools.
Consider modifying the member field to reference a different type or remove it if it's not necessary. This will prevent potential recursion issues and improve schema reliability.
| prevMember, | ||
| nextMember, | ||
| MemberItemComponent, | ||
| memberItemComponentProps, |
There was a problem hiding this comment.
let's use capital letter for these kind of props
| <Virtuoso | ||
| style={{ height: membersContainerHeight }} | ||
| data={members} | ||
| overscan={NUMBER_OF_MEMBERS_TO_LOAD_NEXT} |
There was a problem hiding this comment.
lets not overscan for now, on large lists can cause Maximum call stack size exceeded error
This reverts commit d18d6fe.
… BA-1777-fe-multiple-profiles-member-list
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/components/schema.graphql (1)
919-929: LGTM: Members field enhanced with ordering capabilityThe addition of the
orderByparameter to themembersfield aligns with the PR objectives for member management and follows GraphQL conventions. This change enables flexible sorting of members in the UI.Consider documenting the supported ordering options in the schema using a dedicated enum type (e.g.,
MemberOrderBy) to:
- Make valid sorting options self-documenting
- Provide better type safety
- Enable better tooling support (autocomplete, validation)
packages/components/modules/profiles/Members/MemberListItem/index.tsx (2)
22-28: Consider extracting complex conditions into helper functions.The position and status calculations are complex but could be more readable if extracted into helper functions.
+ const isActiveMember = (member: typeof memberFragment) => member.status === MemberStatuses.active + const isPreviousMemberInactive = (prev: typeof prevMemberFragment) => + !prev?.status || prev.status !== MemberStatuses.active + - const isActiveMember = memberFragment.status === MemberStatuses.active - const isPreviousMemberInactive = prevMemberFragment?.status !== MemberStatuses.active - const isPreviousMemberUndefined = !prevMemberFragment?.status - const isFirstActiveMember = - (isActiveMember && isPreviousMemberInactive) || (isActiveMember && isPreviousMemberUndefined) + const isFirstActiveMember = isActiveMember(memberFragment) && + isPreviousMemberInactive(prevMemberFragment)
10-80: Consider component composition for better maintainability.The component currently handles multiple responsibilities:
- Data management with fragments
- Position/status logic
- Rendering different layouts
Consider splitting these concerns into smaller, more focused components:
- A container component for data management
- A pure component for rendering
- A hook for position/status logic
This would improve testability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/components/modules/profiles/Members/MemberListItem/index.tsx(1 hunks)packages/components/schema.graphql(5 hunks)
🔇 Additional comments (5)
packages/components/schema.graphql (1)
592-592: LGTM: Notification level enum renamed consistently
The renaming of BaseappNotificationsNotificationLevelChoices to NotificationsNotificationLevelChoices is applied consistently across the schema while maintaining the same enum values. This change improves naming clarity by removing the redundant "Baseapp" prefix.
Also applies to: 694-694, 729-742
packages/components/modules/profiles/Members/MemberListItem/index.tsx (4)
1-9: LGTM! Well-organized imports.
The imports are logically grouped and all are being used within the component.
16-16: Use PascalCase for component props.
As mentioned in a previous review, component props should use PascalCase naming convention.
- memberItemComponentProps,
+ MemberItemComponentProps,18-20: Verify fragment usage across the codebase.
The fragment usage looks correct, but let's verify that the MemberItemFragment is consistently used across the codebase.
✅ Verification successful
The MemberItemFragment usage is consistent across the codebase
The fragment is correctly defined in graphql/queries/MemberItem.ts and consistently used with useFragment hook in the component. All related type definitions and imports are properly set up, and the fragment is correctly spread in the parent queries.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of MemberItemFragment
# Expected: All usages should follow the same pattern with useFragment
# Search for MemberItemFragment usage
rg "MemberItemFragment" -A 2
Length of output: 8547
40-41: Ensure props spreading doesn't override critical props.
The spread operator {...memberItemComponentProps} is placed after specific props, which means it could potentially override critical values like member, memberRole, and status.
Also applies to: 46-47, 59-60, 66-67, 77-78
| memberRole="owner" | ||
| status={MemberStatuses.active} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting hardcoded values to constants.
The "owner" role is hardcoded in multiple places. Consider moving it to the constants file for better maintainability.
// In ../constants.ts
+ export const MemberRoles = {
+ OWNER: 'owner',
+ } as const;
// In this file
- memberRole="owner"
+ memberRole={MemberRoles.OWNER}Also applies to: 64-65
anicioalexandre
changed the title
As a user, on the BaseApp Profile,I would like to I want to have a page where I can see the members of a profile, In order to be able to manage the members.
Original Story:
Acceptance Criteria
Context
This story will be about creating the option to access the members page, creating the structure of the members page and also creating the members modal and the relationship with the profiles
Business Rules - Members List - 5
Given a user has navigated to the Main Profile Settings Page for any profile, then they should see the "Members" option in the side menu.
After clicking the Members option, the user should be able to see a list of the members for the active profile with the following properties
Show a count of the total members (active and pending invite)
Pending invite members should be displayed in a light gray font with a pending status.
The members list will have its scrolling behavior inside its component, this means the page will not scroll if you do it inside the member list component.
Current behavior
Expected behavior
Code Snippet
Design Link: https://www.figma.com/design/XRD6wSl1m8Kz6XUcAy5CLp/BaseApp---WEB?node-id=3660-64822&node-type=frame&t=MBvMfObBLgXLqFQv-0
Template: https://bitbucket.org/silverlogic/baseapp-frontend-template/pull-requests/113
BA BE Package: silverlogic/baseapp-backend#182
Summary by CodeRabbit
Release Notes
New Features
MemberItemcomponent for displaying member information.MembersListcomponent for paginated member display.MemberListItemcomponent for rendering individual member details in a list.Enhancements
Constants
Utility
Documentation
Version Update
0.0.22to0.0.23.