BA-2007: social page follow unfollow #165
Conversation
pt-tsl
requested review from
Hercilio1,
anicioalexandre,
deboracosilveira and
Ronan-Fernandes
|
|
Warning Rate limit exceeded@pt-tsl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughThis pull request introduces significant enhancements to the Changes
Sequence DiagramsequenceDiagram
participant User
participant ProfileComponent
participant FollowToggleButton
participant BlockButtonWithDialog
participant GraphQL Server
User->>ProfileComponent: View Profile
ProfileComponent->>GraphQL Server: Fetch Profile Data
GraphQL Server-->>ProfileComponent: Return Profile Information
ProfileComponent->>FollowToggleButton: Render Follow Button
ProfileComponent->>BlockButtonWithDialog: Render Block Button
User->>FollowToggleButton: Click Follow/Unfollow
FollowToggleButton->>GraphQL Server: Send Follow Toggle Mutation
GraphQL Server-->>FollowToggleButton: Update Follow Status
User->>BlockButtonWithDialog: Click Block/Unblock
BlockButtonWithDialog->>GraphQL Server: Send Block Toggle Mutation
GraphQL Server-->>BlockButtonWithDialog: Update Block Status
Possibly Related PRs
Suggested Reviewers
Poem
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: 10
🧹 Nitpick comments (20)
packages/design-system/components/icons/BlockIcon/index.tsx (2)
6-6: Consider adding JSDoc documentation for styling options.While the implementation is correct, adding documentation about the default styles and customization options would improve developer experience.
+/** + * BlockIcon component with default size of 24px and grey.800 color. + * Styles can be customized using the sx prop. + */ const BlockIcon: FC<SvgIconProps> = ({ sx, ...props }) => (
7-20: Add ARIA attributes for better accessibility.The SVG implementation is clean and follows best practices. Consider adding ARIA attributes for better accessibility.
- <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none"> + <svg + xmlns="http://www.w3.org/2000/svg" + width="24" + height="24" + viewBox="0 0 24 24" + fill="none" + role="img" + aria-label="block icon" + >packages/design-system/components/icons/ThreeDotsIcon/index.tsx (1)
6-30: Consider enhancing accessibility with ARIA attributes.For better screen reader support, consider adding
role="img"andaria-label="more options"to the SVG element.<SvgIcon sx={{ fontSize: 24, color: 'action.active', ...sx }} {...props}> - <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none"> + <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" role="img" aria-label="more options">packages/design-system/CHANGELOG.md (1)
3-8: Consider adding more context to the changelog entry.While the entry correctly documents the component moves, it would be helpful to add:
- The rationale for moving these components to the design system
- Any breaking changes or migration steps if applicable
## 0.0.25 ### Patch Changes -Moved BlockIcon, UnblockIcon, ThreeDotsIcon and ImageWithFallback from baseapp-frontend-template +Moved BlockIcon, UnblockIcon, ThreeDotsIcon and ImageWithFallback from baseapp-frontend-template to consolidate icon components in the design system + +Note: No breaking changes. Existing imports should be updated to use the new package location.packages/design-system/components/icons/UnblockIcon/index.tsx (1)
1-3: Consider removing theFCtype.The
FCtype is being phased out in modern React as it doesn't provide additional type safety and can hide type issues. Consider using a direct function type instead.-import { FC } from 'react' import { SvgIcon, SvgIconProps } from '@mui/material' -const UnblockIcon: FC<SvgIconProps> = ({ sx, ...props }) => ( +const UnblockIcon = ({ sx, ...props }: SvgIconProps) => (packages/design-system/components/images/ImageWithFallback/types.ts (1)
3-11: Add JSDoc comments and type validationConsider the following improvements:
- Add JSDoc comments to document the purpose and usage of each prop
- Consider using a union type for
typeandfallbackTypeto restrict to valid MIME typesHere's a suggested implementation:
import { ImageProps } from 'next/image' +type ImageMimeType = 'image/webp' | 'image/png' | 'image/jpeg' | 'image/gif' + +/** + * Props for the ImageWithFallback component + * @extends {ImageProps} Extends Next.js Image component props + */ export interface ImageWithFallbackProps extends ImageProps { + /** Primary image source URL */ src: string + /** Fallback image source URL used when primary format is not supported */ fallbackSrc: string + /** Primary image MIME type (defaults to 'image/webp') */ - type?: string + type?: ImageMimeType + /** Fallback image MIME type (defaults to 'image/png') */ - fallbackType?: string + fallbackType?: ImageMimeType + /** Alternative text for accessibility */ alt: string + /** Image width in pixels */ width: number + /** Image height in pixels */ height: number }packages/design-system/components/images/ImageWithFallback/index.tsx (1)
19-28: Consider adding loading optimization propsThe component could benefit from additional props to optimize loading behavior:
Consider adding these props with their TypeScript types:
const ImageWithFallback: FC<ImageWithFallbackProps> = ({ src, fallbackSrc, type = 'image/webp', fallbackType = 'image/png', alt, width, height, + loading = 'lazy', + quality = 75, + placeholder = 'blur', + blurDataURL, ...props }) => (packages/components/modules/profiles/BlockButtonWithDialog/index.tsx (2)
44-47: Consider surfacing a message when the action is disallowed.
Right now, if isMutationInFlight is true or currentProfileId/target is missing, the function just returns. It could be useful to display a toast message or otherwise indicate that the action cannot proceed to improve clarity.
72-93: Review menu blocking/unblocking flows.
The button triggers the confirm dialog, but you might also confirm the block/unblock action from the menu for a seamless UX. Confirm there's no duplication or confusion for users who first see the button label and then additional dialog steps.packages/components/modules/profiles/ProfileComponent/index.tsx (2)
58-69: Consider reusing a shared utility for formatting large numbers.
The logic for abbreviating numbers (1k, 1.5k, etc.) could be extracted into a utility or hook for reusability and to keep the component cleaner.
171-173: Avoid repeated code for desktop/mobile menus.
Some menu actions (share, block/unblock) appear in both desktop (StyledMenu) and mobile (SwipeableDrawer). Consider extracting them into a single shared component or function for better DRY compliance.packages/components/modules/profiles/ProfileComponent/types.ts (1)
3-5: Consider adding JSDoc documentation for better maintainability.The interface is well-typed using Relay's generated types. Consider adding JSDoc documentation to explain the purpose of the interface and the profile prop.
+/** + * Props for the ProfileComponent that displays user profile information + */ export interface ProfileComponentProps { + /** The profile data retrieved via Relay fragment */ profile?: ProfileComponentFragment$key | null }packages/components/modules/profiles/graphql/queries/FollowToggleUpdatableFragment.ts (1)
3-8: LGTM! Consider adding fields for count formatting.The fragment correctly uses the @updatable directive and includes essential fields. However, to support the requirement of formatting numbers ≥1000 as "1k", consider adding formatted string fields from the backend.
fragment FollowToggleUpdatableFragment on Profile @updatable { isFollowedByMe followersCount + formattedFollowersCount }packages/components/modules/profiles/graphql/mutations/FollowToggle.ts (1)
3-17: LGTM! Consider adding error handling.The mutation structure is well-designed and aligns perfectly with the follow/unfollow feature requirements. The inclusion of
isFollowedByMeandfollowersCountfields will enable proper UI updates, and theFollowToggleUpdatableFragmentensures real-time data consistency.Consider adding an error field in the response to handle potential follow/unfollow failures gracefully:
mutation FollowToggleMutation($input: FollowToggleInput!) { followToggle(input: $input) { + error { + message + } follow { node { target {packages/components/modules/profiles/graphql/mutations/BlockToggle.ts (1)
3-19: Add rate limiting and validation checks.The mutation structure is solid, particularly the use of
@deleteRecordfor proper cleanup. However, given the sensitive nature of blocking functionality, consider implementing additional security measures.Recommendations for the backend implementation:
- Add rate limiting to prevent abuse
- Implement validation to prevent self-blocking
- Add proper error handling for cases like:
- Blocking already blocked users
- Blocking system accounts
- Blocking when user lacks permissions
Example error handling addition:
mutation BlockToggleMutation($input: BlockToggleInput!) { blockToggle(input: $input) { + error { + code + message + } block {packages/components/modules/profiles/BlockButtonWithDialog/styled.tsx (1)
6-11: Enhance button accessibility.The styled component implementation is clean and follows MUI best practices. The conditional styling based on
isBlockedstate is well-implemented.Consider these accessibility improvements:
export const ActionButton = styled(Button)<ActionButtonProps>(({ theme, isBlocked = false }) => ({ backgroundColor: isBlocked ? theme.palette.grey[800] : theme.palette.error.main, + '&:focus-visible': { + outline: `2px solid ${theme.palette.primary.main}`, + outlineOffset: '2px', + }, '&:hover': { backgroundColor: isBlocked ? theme.palette.grey[900] : theme.palette.error.dark, }, + '&.Mui-disabled': { + backgroundColor: theme.palette.action.disabledBackground, + color: theme.palette.action.disabled, + }, }))Also, ensure the button includes proper
aria-labelwhen used:<ActionButton aria-label={isBlocked ? 'Unblock user' : 'Block user'} // ... other props />packages/components/modules/profiles/graphql/queries/ProfileComponent.ts (2)
9-11: Consider making image dimensions configurableThe image dimensions are hardcoded in the fragment. This limits the component's reusability across different contexts where different image sizes might be needed.
Consider using variables for dimensions:
- image(height: 96, width: 96) { + image(height: $avatarHeight, width: $avatarWidth) { url } - bannerImage(height: 290, width: 868) { + bannerImage(height: $bannerHeight, width: $bannerWidth) { url }Also applies to: 12-14
18-18: Consider using a constant for the permission stringThe permission string "change" is hardcoded. Consider extracting it to a constant to maintain consistency and ease updates.
+const PROFILE_CHANGE_PERMISSION = 'change' as const; + export const ProfileComponentFragment = graphql` fragment ProfileComponentFragment on Profile { - canChange: hasPerm(perm: "change") + canChange: hasPerm(perm: ${PROFILE_CHANGE_PERMISSION})packages/components/modules/profiles/BlockButtonWithDialog/constants.ts (1)
1-14: Consider adding i18n support for dialog textsThe dialog texts are well-structured and clearly communicate the implications of blocking/unblocking. However, to support international users, consider wrapping these strings in translation functions.
Example implementation:
export const BLOCK_UNBLOCK_DIALOG_TEXTS = { block: { - title: 'Block', + title: t('block.title'), content: - "This person won't be able to send you messages, find your profile or content. They won't see your comments and reactions, and won't be able to mention or follow your profile. We won't notify them that they have been blocked.", + t('block.content'), - action: 'Block', + action: t('block.action'), }, // Similar changes for unblock... }packages/components/modules/profiles/ProfileComponent/styled.tsx (1)
4-15: Consider using 'border-box' for consistent sizingUsing 'content-box' for box-sizing might lead to unexpected layout behavior when padding is applied. Consider using 'border-box' for more predictable sizing.
export const ProfileContainer = styled(Box)(({ theme }) => ({ - boxSizing: 'content-box', + boxSizing: 'border-box', // ...rest of the styles }))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
packages/components/__generated__/BlockToggleFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/BlockToggleMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/FollowToggleMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/FollowToggleUpdatableFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ProfileComponentFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ProfileUpdateMutation.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
packages/components/CHANGELOG.md(2 hunks)packages/components/modules/profiles/BlockButtonWithDialog/constants.ts(1 hunks)packages/components/modules/profiles/BlockButtonWithDialog/index.tsx(1 hunks)packages/components/modules/profiles/BlockButtonWithDialog/styled.tsx(1 hunks)packages/components/modules/profiles/BlockButtonWithDialog/types.ts(1 hunks)packages/components/modules/profiles/FollowToggleButton/index.tsx(1 hunks)packages/components/modules/profiles/FollowToggleButton/types.ts(1 hunks)packages/components/modules/profiles/ProfileComponent/index.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/styled.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/types.ts(1 hunks)packages/components/modules/profiles/graphql/mutations/BlockToggle.ts(1 hunks)packages/components/modules/profiles/graphql/mutations/FollowToggle.ts(1 hunks)packages/components/modules/profiles/graphql/mutations/ProfileUpdate.ts(1 hunks)packages/components/modules/profiles/graphql/queries/BlockToggle.ts(1 hunks)packages/components/modules/profiles/graphql/queries/FollowToggleUpdatableFragment.ts(1 hunks)packages/components/modules/profiles/graphql/queries/ProfileComponent.ts(1 hunks)packages/components/modules/profiles/index.ts(1 hunks)packages/components/package.json(2 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/icons/BlockIcon/index.tsx(1 hunks)packages/design-system/components/icons/ThreeDotsIcon/index.tsx(1 hunks)packages/design-system/components/icons/UnblockIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(2 hunks)packages/design-system/components/images/ImageWithFallback/__storybook__/stories.tsx(1 hunks)packages/design-system/components/images/ImageWithFallback/index.tsx(1 hunks)packages/design-system/components/images/ImageWithFallback/types.ts(1 hunks)packages/design-system/components/images/index.ts(1 hunks)packages/design-system/index.ts(1 hunks)packages/design-system/package.json(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- packages/wagtail/CHANGELOG.md
- packages/design-system/components/images/ImageWithFallback/storybook/stories.tsx
- packages/design-system/components/icons/index.ts
- packages/design-system/package.json
- packages/wagtail/package.json
🔇 Additional comments (19)
packages/components/package.json (2)
4-4: LGTM: Version bump is appropriate
The minor version increment from 0.0.36 to 0.0.37 aligns with the addition of new features (follow/unfollow functionality).
35-35: Verify numbro dependency version and add documentation
The addition of numbro aligns with the requirement to format follower counts (e.g., "1k"). However:
- Let's verify we're using the latest stable version
- Consider documenting the usage in the README
packages/design-system/components/icons/BlockIcon/index.tsx (3)
1-3: LGTM! Clean and minimal imports.
The imports are well-organized and include only the necessary types and components.
5-5: LGTM! Well-typed component definition.
The component is properly typed with FC and implements clean props destructuring.
24-24: LGTM! Clean export statement.
The default export is appropriate for a single component file.
packages/design-system/components/icons/ThreeDotsIcon/index.tsx (1)
5-31: LGTM! Clean implementation of ThreeDotsIcon component.
The component follows Material-UI best practices and properly implements SVG icon patterns.
packages/design-system/components/icons/UnblockIcon/index.tsx (3)
5-6: LGTM! Props handling and default styling are well implemented.
The component correctly handles prop spreading and style merging, following Material-UI best practices.
30-30: LGTM! Export statement is correct.
The default export is appropriate for a single component file and matches the usage pattern in the icons index file.
1-30: Verify component export in the design system.
Let's ensure this component is properly exported and accessible for the social feature implementation.
✅ Verification successful
Let me check the export and usage details more thoroughly before making the final determination.
Component is properly exported and actively used
The UnblockIcon component is correctly exported in the design system's main icons index file and is actively used in the BlockButtonWithDialog component. The implementation shows proper integration with the blocking/unblocking functionality, where the icon is used appropriately in both the button and dialog contexts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify UnblockIcon export and usage
# Check if the icon is exported in the main index file
rg -l "export.*UnblockIcon" packages/design-system/
# Check for any existing usage of the UnblockIcon
rg -l "UnblockIcon" packages/
Length of output: 410
Script:
#!/bin/bash
# Check the export statement in the icons index file
cat packages/design-system/components/icons/index.ts
# Check how the UnblockIcon is being used in the BlockButtonWithDialog
cat packages/components/modules/profiles/BlockButtonWithDialog/index.tsx
Length of output: 5367
packages/design-system/components/images/index.ts (1)
1-1: LGTM!
The export statement is clean and follows the standard module export pattern.
packages/design-system/index.ts (1)
29-29: LGTM! Verify the images module contents.
The new export statement follows the established pattern and is correctly placed in the components section.
Let's verify the images module exists and contains the expected components:
✅ Verification successful
✓ Images module is properly implemented and used
The verification confirms that:
- The
imagesmodule is correctly structured with proper exports ImageWithFallbackcomponent is well-implemented with:- TypeScript types and proper props interface
- Next.js Image integration with fallback support
- Storybook documentation
- Active usage in the profiles module
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the images module exists and contains expected components
# Check if the images directory exists and show its contents
echo "Checking images module structure:"
ls -la packages/design-system/components/images/
# Look for ImageWithFallback component implementation
echo -e "\nVerifying ImageWithFallback component:"
rg -t typescript "ImageWithFallback" packages/design-system/components/images/
Length of output: 570
Script:
#!/bin/bash
# Let's verify the contents of the images module and its components
# Check the content of the images/index.ts file
echo "Content of images/index.ts:"
cat packages/design-system/components/images/index.ts
# Check the ImageWithFallback component files
echo -e "\nFiles in ImageWithFallback directory:"
ls -la packages/design-system/components/images/ImageWithFallback/
# Check the implementation file content
echo -e "\nContent of ImageWithFallback implementation:"
cat packages/design-system/components/images/ImageWithFallback/*.{ts,tsx} 2>/dev/null
# Search for any usage of ImageWithFallback in the codebase
echo -e "\nUsages of ImageWithFallback in the codebase:"
rg "ImageWithFallback" --type-add 'ts:*.{ts,tsx}' -t ts
Length of output: 3862
packages/components/modules/profiles/BlockButtonWithDialog/index.tsx (2)
58-59: Verify error handling covers all cases.
The error toast is shown for each error returned by the mutation, but if multiple errors occur, the user experience might be repetitive. Consider grouping them into one toast or summarizing.
109-120: Double-check consistent labeling in dialog text.
The text changes based on isBlockedByMe, but ensure the user-facing language is consistently representative of the action (like “Block {name}?” or “Unblock {name}?”).
packages/components/modules/profiles/index.ts (1)
3-4: Confirm usage of export type * patterns.
Re-exporting all types is convenient but risks collisions or confusion for consumers. Verify that it won’t shadow or conflict with other exports in the application.
packages/components/modules/profiles/graphql/queries/BlockToggle.ts (1)
3-12: Verify security implications of exposing block status.
The fragment is well-structured and includes necessary fields. However, let's verify the security implications of exposing the isBlockedByMe field.
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient information about the block functionality implementation:
Exposing block status is properly implemented with necessary controls
The implementation shows that:
- The
isBlockedByMefield is only exposed through authenticated mutations (BlockToggleMutation) - Block status is used within a controlled UI component (
BlockButtonWithDialog) that handles the blocking functionality - The mutation requires both
actorObjectIdandtargetObjectId, ensuring proper authorization context - The block status is used to conditionally render UI elements, preventing blocked users from interacting with profiles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any access control mechanisms around block status
# Search for GraphQL resolvers or policies related to block status
rg -t ts -t js "isBlockedByMe.*resolve"
# Search for any authentication/authorization checks
rg -t ts -t js "authorize.*block"
Length of output: 77
Script:
#!/bin/bash
# Let's try different search patterns to find security-related code
# Search for block-related resolvers
rg -t ts -t js "class.*Block.*Resolver" -A 5
# Search for block-related mutations or queries
rg -t ts -t js "@(Query|Mutation).*block" -A 5
# Search for block-related authorization checks
rg -t ts -t js "(checkAuth|requireAuth|isAuth).*block" -i -A 5
# Search for block-related middleware or guards
rg -t ts -t js "@(UseGuards|Middleware).*block" -i -A 5
# Search for any GraphQL definitions involving blocks
rg -t ts -t js "type.*Block.*{" -A 5
Length of output: 349
Script:
#!/bin/bash
# Let's try a different approach to find security implementations
# Search for the BlocksInterface definition
rg -t ts -t js "interface BlocksInterface" -A 10
# Look for implementations of isBlockedByMe
rg -t ts -t js "isBlockedByMe" -A 5
# Search for block-related mutations with fixed regex
rg -t ts -t js "mutation.*block" -i -A 5
# Look for auth decorators in the profiles module
fd -e ts -e js . packages/components/modules/profiles -x rg -l "@(Auth|Guard)"
# Search for block-related types without curly brace
rg -t ts -t js "type.*Block" -A 5
Length of output: 30725
packages/components/modules/profiles/graphql/mutations/ProfileUpdate.ts (1)
6-15: Consider adding input validation
The mutation accepts a ProfileUpdateInput without visible validation. Consider adding runtime validation to ensure data integrity before sending to the server.
Let's check if there's any validation in place:
Consider using a validation library like Zod or Yup to validate the input before mutation execution:
const profileUpdateSchema = z.object({
name: z.string().min(1),
biography: z.string().optional(),
// ... other fields
});packages/components/modules/profiles/graphql/queries/ProfileComponent.ts (1)
16-17: Verify follower count formatting implementation
The fragment correctly includes followersCount and followingCount, but remember that the PR requirements specify numbers ≥1000 should be abbreviated (e.g., "1k").
Let's check if the formatting is implemented:
Consider using the numbro library (mentioned in the AI summary as a new dependency) for consistent number formatting:
import numbro from 'numbro';
const formatCount = (count: number) =>
numbro(count).format({
average: true,
mantissa: 1,
abbreviations: { thousand: 'k' }
});✅ Verification successful
Let me search for the formatFollowCount implementation since we found its usage in the ProfileComponent.
Let me try searching without the file type restriction since it's not supported.
Let me check the complete implementation of the formatFollowCount function to see if it handles the abbreviation requirement correctly.
Number formatting is already properly implemented ✅
The formatFollowCount function correctly implements the number formatting requirement using the numbro library:
- Numbers ≤1000 are shown as-is
- Numbers >1000 are abbreviated using
numbrowith appropriate formatting:- For numbers <1.05M: uses
average: true(e.g., "1k", "100k") - For numbers ≥1.05M: uses
average: true, mantissa: 1for more precision (e.g., "1.5M")
- For numbers <1.05M: uses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for number formatting implementation
rg -A 5 "followersCount|followingCount"
Length of output: 11836
Script:
#!/bin/bash
# Search for formatFollowCount implementation
rg -A 5 "formatFollowCount" --type typescript
Length of output: 80
Script:
#!/bin/bash
# Search for formatFollowCount implementation
rg -A 5 "formatFollowCount"
Length of output: 1844
Script:
#!/bin/bash
# Get the complete implementation of formatFollowCount
rg -A 10 "const formatFollowCount"
Length of output: 1051
packages/components/CHANGELOG.md (2)
7-8: LGTM! Changes align with PR objectives.
The changelog entries accurately reflect the implementation of the follow/unfollow feature and related component migrations.
9-10: Verify design system compatibility.
The update to design system version 0.0.25 needs verification to ensure it contains the necessary UI components for the follow/unfollow feature.
✅ Verification successful
Design system update 0.0.25 is compatible with follow/unfollow feature
The design system provides the necessary UI components for implementing the follow/unfollow feature:
- IconButton component that supports loading states
- Material UI button base components through dependencies
- Recent update (0.0.25) focuses on icons and doesn't introduce breaking changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check design system package for required UI components
# Check package.json in design-system for version
fd -t f "package.json" -x grep -l "0.0.25" {}
# Look for button-related components that might be used by FollowToggle
rg -t tsx -t ts "Button" packages/design-system/
Length of output: 160
Script:
#!/bin/bash
# Let's try a different approach to find button components and their usage
# Look for button-related files and components without specifying file type
rg -l "Button" packages/design-system/src/
# Check the actual version in design-system's package.json
cat packages/design-system/package.json
# Look for any follow/unfollow related components or implementations
rg -l "follow|Follow" packages/design-system/src/
Length of output: 3414
Script:
#!/bin/bash
# Let's check the actual directory structure and content
# Find the source directory for design-system
fd -t d "design-system"
# List contents of the design-system package
ls -R packages/design-system/
# Look for button components in any directory under design-system
rg -l "Button" packages/design-system/
Length of output: 14549
Script:
#!/bin/bash
# Let's check the button components and their implementations
# Check IconButton implementation which might be used for follow/unfollow
cat packages/design-system/components/buttons/IconButton/index.tsx
# Check if there are any follow-related components or implementations
rg -l "follow|Follow|FollowToggle" packages/design-system/components/
# Check the CHANGELOG for any relevant changes
cat packages/design-system/CHANGELOG.md
Length of output: 6410
| <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none"> | ||
| <path | ||
| fillRule="evenodd" | ||
| clipRule="evenodd" | ||
| d="M7 10.75C6.30921 10.75 5.75 11.3092 5.75 12V19C5.75 19.6908 6.30921 20.25 7 20.25H17C17.6908 20.25 18.25 19.6908 18.25 19V12C18.25 11.3092 17.6908 10.75 17 10.75H7ZM4.25 12C4.25 10.4808 5.48079 9.25 7 9.25H17C18.5192 9.25 19.75 10.4808 19.75 12V19C19.75 20.5192 18.5192 21.75 17 21.75H7C5.48079 21.75 4.25 20.5192 4.25 19V12Z" | ||
| fill="currentColor" | ||
| /> | ||
| <path | ||
| fillRule="evenodd" | ||
| clipRule="evenodd" | ||
| d="M7.40684 5.83932C7.92584 3.78467 9.77395 2.25 12 2.25C14.6232 2.25 16.75 4.37679 16.75 7C16.75 7.41421 16.4142 7.75 16 7.75C15.5858 7.75 15.25 7.41421 15.25 7C15.25 5.20521 13.7948 3.75 12 3.75C10.486 3.75 9.21816 4.79333 8.86116 6.20668C8.75972 6.60828 8.35192 6.8516 7.95032 6.75016C7.54872 6.64872 7.3054 6.24092 7.40684 5.83932Z" | ||
| fill="currentColor" | ||
| /> | ||
| <path | ||
| fillRule="evenodd" | ||
| clipRule="evenodd" | ||
| d="M16 6.25C16.4142 6.25 16.75 6.58579 16.75 7V10C16.75 10.4142 16.4142 10.75 16 10.75C15.5858 10.75 15.25 10.4142 15.25 10V7C15.25 6.58579 15.5858 6.25 16 6.25Z" | ||
| fill="currentColor" | ||
| /> | ||
| </svg> | ||
| </SvgIcon> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes to the icon.
While the SVG implementation is correct, consider adding accessibility attributes to improve screen reader support.
const UnblockIcon = ({ sx, ...props }: SvgIconProps) => (
- <SvgIcon sx={{ fontSize: 24, color: 'grey.800', ...sx }} {...props}>
+ <SvgIcon
+ sx={{ fontSize: 24, color: 'grey.800', ...sx }}
+ aria-label="unblock user"
+ titleAccess="Unblock user"
+ {...props}
+ >Committable suggestion skipped: line range outside the PR's diff.
| <picture> | ||
| <source srcSet={src} type={type} /> | ||
| <source srcSet={fallbackSrc} type={fallbackType} /> | ||
| <Image src={src} alt={alt} width={width} height={height} {...props} /> | ||
| </picture> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve fallback handling and add error handling
The implementation has a few potential improvements:
- The final
Imagecomponent should usefallbackSrcinstead ofsrcas the last resort - Consider adding error handling for failed image loads
- Add loading state handling
Here's a suggested implementation:
- <picture>
- <source srcSet={src} type={type} />
- <source srcSet={fallbackSrc} type={fallbackType} />
- <Image src={src} alt={alt} width={width} height={height} {...props} />
- </picture>
+ <picture>
+ <source srcSet={src} type={type} />
+ <source srcSet={fallbackSrc} type={fallbackType} />
+ <Image
+ src={fallbackSrc}
+ alt={alt}
+ width={width}
+ height={height}
+ onError={(e) => {
+ console.error('Image failed to load:', e);
+ // Handle error state if needed
+ }}
+ {...props}
+ />
+ </picture>📝 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.
| <picture> | |
| <source srcSet={src} type={type} /> | |
| <source srcSet={fallbackSrc} type={fallbackType} /> | |
| <Image src={src} alt={alt} width={width} height={height} {...props} /> | |
| </picture> | |
| ) | |
| <picture> | |
| <source srcSet={src} type={type} /> | |
| <source srcSet={fallbackSrc} type={fallbackType} /> | |
| <Image | |
| src={fallbackSrc} | |
| alt={alt} | |
| width={width} | |
| height={height} | |
| onError={(e) => { | |
| console.error('Image failed to load:', e); | |
| // Handle error state if needed | |
| }} | |
| {...props} | |
| /> | |
| </picture> | |
| ) |
| <ProfileContentContainer> | ||
| <AvatarWithPlaceholder | ||
| className="self-start justify-self-center" | ||
| width={96} | ||
| height={96} | ||
| src={profile?.image?.url} | ||
| /> | ||
| <ProfileDescriptionContainer> |
There was a problem hiding this comment.
Add meaningful alt text for the profile avatar.
Currently, it doesn't pass a descriptive alt message for accessibility. Provide a user-friendly alt text, for instance, "Profile avatar of {profile.name}".
| export interface FollowToggleButtonProps { | ||
| targetId: string | ||
| isFollowedByMe: boolean | null | undefined | ||
| currentProfileId?: string | ||
| profileRef?: any | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen the type for profileRef.
Instead of “any”, consider specifying a stronger type (e.g., a Relay fragment reference or a specific profile GraphQL type), ensuring compile-time safety and clearer usage.
| currentProfileId?: string | ||
| isMenu?: boolean |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making currentProfileId required
The currentProfileId is marked as optional, but it seems crucial for determining whether to show the block button (you shouldn't be able to block yourself). Consider making it required to prevent potential runtime issues.
export interface BlockButtonWithDialogProps {
target: BlockToggleFragment$key
- currentProfileId?: string
+ currentProfileId: string
isMenu?: boolean📝 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.
| currentProfileId?: string | |
| isMenu?: boolean | |
| currentProfileId: string | |
| isMenu?: boolean |
| export const StyledMenu = styled((props: MenuProps) => ( | ||
| <Menu | ||
| elevation={0} | ||
| anchorOrigin={{ | ||
| vertical: 'bottom', | ||
| horizontal: 'right', | ||
| }} | ||
| transformOrigin={{ | ||
| vertical: 'top', | ||
| horizontal: 'right', | ||
| }} | ||
| {...props} | ||
| /> | ||
| ))(({ theme }) => ({ | ||
| '& .MuiPaper-root': { | ||
| minWidth: 190, | ||
| '& .MuiMenuItem-root': { | ||
| '& .MuiSvgIcon-root': { | ||
| marginRight: theme.spacing(1.5), | ||
| }, | ||
| '&:active': { | ||
| backgroundColor: alpha(theme.palette.primary.main, theme.palette.action.selectedOpacity), | ||
| }, | ||
| }, | ||
| ...theme.applyStyles('dark', { | ||
| color: theme.palette.grey[300], | ||
| }), | ||
| }, | ||
| })) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add hover state styles for menu items
The menu items lack hover state styles, which is important for better user interaction feedback.
'& .MuiMenuItem-root': {
'& .MuiSvgIcon-root': {
marginRight: theme.spacing(1.5),
},
+ '&:hover': {
+ backgroundColor: alpha(theme.palette.primary.main, theme.palette.action.hoverOpacity),
+ },
'&:active': {
backgroundColor: alpha(theme.palette.primary.main, theme.palette.action.selectedOpacity),
},
},📝 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.
| export const StyledMenu = styled((props: MenuProps) => ( | |
| <Menu | |
| elevation={0} | |
| anchorOrigin={{ | |
| vertical: 'bottom', | |
| horizontal: 'right', | |
| }} | |
| transformOrigin={{ | |
| vertical: 'top', | |
| horizontal: 'right', | |
| }} | |
| {...props} | |
| /> | |
| ))(({ theme }) => ({ | |
| '& .MuiPaper-root': { | |
| minWidth: 190, | |
| '& .MuiMenuItem-root': { | |
| '& .MuiSvgIcon-root': { | |
| marginRight: theme.spacing(1.5), | |
| }, | |
| '&:active': { | |
| backgroundColor: alpha(theme.palette.primary.main, theme.palette.action.selectedOpacity), | |
| }, | |
| }, | |
| ...theme.applyStyles('dark', { | |
| color: theme.palette.grey[300], | |
| }), | |
| }, | |
| })) | |
| export const StyledMenu = styled((props: MenuProps) => ( | |
| <Menu | |
| elevation={0} | |
| anchorOrigin={{ | |
| vertical: 'bottom', | |
| horizontal: 'right', | |
| }} | |
| transformOrigin={{ | |
| vertical: 'top', | |
| horizontal: 'right', | |
| }} | |
| {...props} | |
| /> | |
| ))(({ theme }) => ({ | |
| '& .MuiPaper-root': { | |
| minWidth: 190, | |
| '& .MuiMenuItem-root': { | |
| '& .MuiSvgIcon-root': { | |
| marginRight: theme.spacing(1.5), | |
| }, | |
| '&:hover': { | |
| backgroundColor: alpha(theme.palette.primary.main, theme.palette.action.hoverOpacity), | |
| }, | |
| '&:active': { | |
| backgroundColor: alpha(theme.palette.primary.main, theme.palette.action.selectedOpacity), | |
| }, | |
| }, | |
| ...theme.applyStyles('dark', { | |
| color: theme.palette.grey[300], | |
| }), | |
| }, | |
| })) |
| export const ProfileContentContainer = styled(Box)(({ theme }) => ({ | ||
| display: 'grid', | ||
| gridTemplateColumns: '1fr', | ||
| gridTemplateRows: 'min-content auto', | ||
| gap: theme.spacing(3), | ||
| position: 'relative', | ||
| top: -48, | ||
| justifyContent: 'center', | ||
| [theme.breakpoints.up('sm')]: { | ||
| position: 'static', | ||
| top: 0, | ||
| gridTemplateColumns: 'max-content 480px', | ||
| gridTemplateRows: '1fr', | ||
| }, | ||
| })) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use theme spacing for positioning
The hardcoded top value (-48) should use theme spacing for consistency and maintainability.
export const ProfileContentContainer = styled(Box)(({ theme }) => ({
// ...other styles
- top: -48,
+ top: theme.spacing(-6), // assuming 1 spacing unit = 8px
// ...rest of the styles
}))Committable suggestion skipped: line range outside the PR's diff.
| const updatedIsFollowedByMe = follow?.node?.target?.isFollowedByMe ?? false | ||
| const updatedFollowersCount = | ||
| follow?.node?.target?.followersCount ?? updatableData.followersCount - 1 |
There was a problem hiding this comment.
Improve null handling in store updates
The fallback logic for followersCount assumes decrementing by 1, which might be incorrect if the API call failed. Consider using the current value as fallback.
- const updatedFollowersCount =
- follow?.node?.target?.followersCount ?? updatableData.followersCount - 1
+ const updatedFollowersCount =
+ follow?.node?.target?.followersCount ?? updatableData.followersCount📝 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 updatedIsFollowedByMe = follow?.node?.target?.isFollowedByMe ?? false | |
| const updatedFollowersCount = | |
| follow?.node?.target?.followersCount ?? updatableData.followersCount - 1 | |
| const updatedIsFollowedByMe = follow?.node?.target?.isFollowedByMe ?? false | |
| const updatedFollowersCount = | |
| follow?.node?.target?.followersCount ?? updatableData.followersCount |
| return ( | ||
| <Button | ||
| onClick={toggleFollow} | ||
| startIcon={ | ||
| isFollowedByMe ? <Iconify icon="ci:check-all" /> : <Iconify icon="mingcute:add-line" /> | ||
| } | ||
| variant={isFollowedByMe ? 'soft' : 'contained'} | ||
| color={isFollowedByMe ? 'inherit' : 'primary'} | ||
| disabled={isMutationInFlight} | ||
| size="medium" | ||
| > | ||
| {isFollowedByMe ? 'Following' : 'Follow'} | ||
| </Button> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state indicator
The button should show a loading indicator when the mutation is in flight for better user feedback.
<Button
onClick={toggleFollow}
startIcon={
+ isMutationInFlight ? (
+ <CircularProgress size={16} color="inherit" />
+ ) :
isFollowedByMe ? (
<Iconify icon="ci:check-all" />
) : (
<Iconify icon="mingcute:add-line" />
)
}
variant={isFollowedByMe ? 'soft' : 'contained'}
color={isFollowedByMe ? 'inherit' : 'primary'}
disabled={isMutationInFlight}
size="medium"
>
- {isFollowedByMe ? 'Following' : 'Follow'}
+ {isMutationInFlight ? 'Loading...' : isFollowedByMe ? 'Following' : 'Follow'}
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <Button | |
| onClick={toggleFollow} | |
| startIcon={ | |
| isFollowedByMe ? <Iconify icon="ci:check-all" /> : <Iconify icon="mingcute:add-line" /> | |
| } | |
| variant={isFollowedByMe ? 'soft' : 'contained'} | |
| color={isFollowedByMe ? 'inherit' : 'primary'} | |
| disabled={isMutationInFlight} | |
| size="medium" | |
| > | |
| {isFollowedByMe ? 'Following' : 'Follow'} | |
| </Button> | |
| ) | |
| return ( | |
| <Button | |
| onClick={toggleFollow} | |
| startIcon={ | |
| isMutationInFlight ? ( | |
| <CircularProgress size={16} color="inherit" /> | |
| ) : | |
| isFollowedByMe ? ( | |
| <Iconify icon="ci:check-all" /> | |
| ) : ( | |
| <Iconify icon="mingcute:add-line" /> | |
| ) | |
| } | |
| variant={isFollowedByMe ? 'soft' : 'contained'} | |
| color={isFollowedByMe ? 'inherit' : 'primary'} | |
| disabled={isMutationInFlight} | |
| size="medium" | |
| > | |
| {isMutationInFlight ? 'Loading...' : isFollowedByMe ? 'Following' : 'Follow'} | |
| </Button> | |
| ) |
| onCompleted: (response, errors) => { | ||
| errors?.forEach((error) => { | ||
| sendToast(error.message, { type: 'error' }) | ||
| }) | ||
| if (errors) { | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider sanitizing error messages
Error messages from the server might contain sensitive information. Consider mapping server errors to user-friendly messages.
+ const getErrorMessage = (error: string) => {
+ // Map known error codes/messages to user-friendly messages
+ if (error.includes('not_found')) return 'User not found';
+ if (error.includes('permission_denied')) return 'You cannot perform this action';
+ return 'An error occurred. Please try again.';
+ }
onCompleted: (response, errors) => {
errors?.forEach((error) => {
- sendToast(error.message, { type: 'error' })
+ sendToast(getErrorMessage(error.message), { type: 'error' })
})Committable suggestion skipped: line range outside the PR's diff.
pt-tsl
force-pushed
the
BA-2007-social-page-follow-unfollow
branch
from
fa24530 to
908c142
Compare
pt-tsl
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/modules/profiles/FollowToggleButton/index.tsx (3)
15-25: Add prop validation for required fields.Consider adding runtime checks for required props, especially
targetIdandcurrentProfileId, to fail fast and provide clear error messages during development.const FollowToggleButton: FC<FollowToggleButtonProps> = ({ targetId, isFollowedByMe, currentProfileId, profileRef, }) => { + if (!targetId || !currentProfileId) { + console.error('FollowToggleButton: targetId and currentProfileId are required'); + return null; + } const [commitMutation, isMutationInFlight] = useMutation<FollowToggleMutation>(FollowToggleMutationQuery)
47-56: Improve null handling for profileRef.The current implementation silently returns when profileRef is null. Consider logging this scenario to help with debugging.
commitLocalUpdate(environment, (store: RecordSourceSelectorProxy) => { if (profileRef == null) { + console.warn('FollowToggleButton: profileRef is null, skipping store update'); return }
69-80: Enhance button accessibility.Add aria-label to improve screen reader experience.
<Button onClick={toggleFollow} + aria-label={isFollowedByMe ? 'Unfollow user' : 'Follow user'} startIcon={ isFollowedByMe ? <Iconify icon="ci:check-all" /> : <Iconify icon="mingcute:add-line" /> } variant={isFollowedByMe ? 'soft' : 'contained'} color={isFollowedByMe ? 'inherit' : 'primary'} disabled={isMutationInFlight} size="medium" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
packages/components/__generated__/BlockToggleFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/BlockToggleMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/FollowToggleMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/FollowToggleUpdatableFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ProfileComponentFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ProfileUpdateMutation.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
packages/components/CHANGELOG.md(2 hunks)packages/components/modules/profiles/BlockButtonWithDialog/constants.ts(1 hunks)packages/components/modules/profiles/BlockButtonWithDialog/index.tsx(1 hunks)packages/components/modules/profiles/BlockButtonWithDialog/styled.tsx(1 hunks)packages/components/modules/profiles/BlockButtonWithDialog/types.ts(1 hunks)packages/components/modules/profiles/FollowToggleButton/index.tsx(1 hunks)packages/components/modules/profiles/FollowToggleButton/types.ts(1 hunks)packages/components/modules/profiles/ProfileComponent/index.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/styled.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/types.ts(1 hunks)packages/components/modules/profiles/graphql/mutations/BlockToggle.ts(1 hunks)packages/components/modules/profiles/graphql/mutations/FollowToggle.ts(1 hunks)packages/components/modules/profiles/graphql/mutations/ProfileUpdate.ts(1 hunks)packages/components/modules/profiles/graphql/queries/BlockToggle.ts(1 hunks)packages/components/modules/profiles/graphql/queries/FollowToggleUpdatableFragment.ts(1 hunks)packages/components/modules/profiles/graphql/queries/ProfileComponent.ts(1 hunks)packages/components/modules/profiles/index.ts(1 hunks)packages/components/package.json(2 hunks)packages/design-system/.storybook/preview.ts(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/icons/BlockIcon/index.tsx(1 hunks)packages/design-system/components/icons/ThreeDotsIcon/index.tsx(1 hunks)packages/design-system/components/icons/UnblockIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(2 hunks)packages/design-system/components/images/ImageWithFallback/__storybook__/stories.tsx(1 hunks)packages/design-system/components/images/ImageWithFallback/index.tsx(1 hunks)packages/design-system/components/images/ImageWithFallback/types.ts(1 hunks)packages/design-system/components/images/index.ts(1 hunks)packages/design-system/index.ts(1 hunks)packages/design-system/package.json(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (30)
- packages/design-system/components/images/index.ts
- packages/design-system/index.ts
- packages/design-system/package.json
- packages/components/modules/profiles/graphql/mutations/FollowToggle.ts
- packages/components/modules/profiles/graphql/queries/FollowToggleUpdatableFragment.ts
- packages/components/modules/profiles/ProfileComponent/types.ts
- packages/components/modules/profiles/graphql/queries/BlockToggle.ts
- packages/design-system/components/images/ImageWithFallback/types.ts
- packages/design-system/components/icons/UnblockIcon/index.tsx
- packages/wagtail/CHANGELOG.md
- packages/components/modules/profiles/graphql/queries/ProfileComponent.ts
- packages/design-system/components/icons/index.ts
- packages/components/package.json
- packages/design-system/components/icons/BlockIcon/index.tsx
- packages/components/modules/profiles/FollowToggleButton/types.ts
- packages/wagtail/package.json
- packages/components/modules/profiles/BlockButtonWithDialog/styled.tsx
- packages/components/modules/profiles/graphql/mutations/ProfileUpdate.ts
- packages/components/modules/profiles/index.ts
- packages/components/modules/profiles/BlockButtonWithDialog/constants.ts
- packages/design-system/components/icons/ThreeDotsIcon/index.tsx
- packages/components/modules/profiles/BlockButtonWithDialog/types.ts
- packages/design-system/components/images/ImageWithFallback/index.tsx
- packages/components/modules/profiles/ProfileComponent/index.tsx
- packages/components/modules/profiles/BlockButtonWithDialog/index.tsx
- packages/components/modules/profiles/ProfileComponent/styled.tsx
- packages/components/modules/profiles/graphql/mutations/BlockToggle.ts
- packages/design-system/CHANGELOG.md
- packages/design-system/components/images/ImageWithFallback/storybook/stories.tsx
- packages/components/CHANGELOG.md
🔇 Additional comments (5)
packages/design-system/.storybook/preview.ts (1)
34-35: LGTM! Verify the ImageWithFallback component setup
The addition of 'ImageWithFallback' to the story sorting order is well-organized under the "Images" section and maintains the existing sorting structure.
Let's verify the existence of the referenced component and its stories:
✅ Verification successful
The ImageWithFallback component is properly set up
The component is correctly implemented with all necessary files in place:
- Component implementation:
packages/design-system/components/images/ImageWithFallback/index.tsx - Type definitions:
packages/design-system/components/images/ImageWithFallback/types.ts - Storybook stories:
packages/design-system/components/images/ImageWithFallback/__storybook__/stories.tsx - Properly exported in:
packages/design-system/components/images/index.ts
The component is also being used in the codebase (packages/components/modules/profiles/ProfileComponent/index.tsx), making its inclusion in the Storybook sorting order appropriate and necessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ImageWithFallback component and its stories exist
# Expected: Find the component file and its storybook stories
echo "Checking for ImageWithFallback component and stories..."
fd -t f "ImageWithFallback" packages/design-system/components/images
# Verify storybook story imports the component
rg -l "import.*ImageWithFallback" packages/design-system/components/images -g "*.stories.*"
Length of output: 283
Script:
#!/bin/bash
# Let's try a broader search for the ImageWithFallback component
echo "Searching for ImageWithFallback component across the repository..."
fd ImageWithFallback
echo -e "\nSearching for ImageWithFallback imports or usage..."
rg -l "ImageWithFallback"
echo -e "\nSearching for any story files in design-system package..."
fd -e stories.tsx -e stories.ts packages/design-system
Length of output: 1324
packages/components/modules/profiles/FollowToggleButton/index.tsx (4)
1-14: LGTM! Imports are well-organized and complete.
The imports are logically grouped and include all necessary dependencies for the component's functionality.
38-44: Previous comment about error message sanitization still applies.
The error messages from the server are still being displayed directly to the user without sanitization.
57-59: Previous comment about follower count fallback still applies.
The fallback logic for followersCount assumes decrementing by 1, which might be incorrect if the API call failed.
68-81: Previous comment about loading state indicator still applies.
The button should show a loading indicator when the mutation is in flight.
anicioalexandre
force-pushed
the
BA-2007-social-page-follow-unfollow
branch
from
908c142 to
c826533
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
packages/components/modules/profiles/ProfileComponent/FollowToggleButton/types.ts (1)
1-6: Avoid usinganyforprofileRefand ensure strictly typed props.Currently,
profileRefis declared asany. Using a more specific type will improve type safety and clarity. For example, ifprofileRefreferences a Relay fragment or a local store record, consider defining or importing the appropriate type from the generated GraphQL types or a custom interface.packages/components/modules/profiles/ProfileComponent/FollowToggleButton/index.tsx (3)
15-20: Consider adding inline comments or docstrings to clarify prop usage.While the props are self-explanatory, it might help future maintainers if there was a short comment explaining each prop’s purpose, particularly around
currentProfileIdandprofileRef, which can be undefined.
26-29: Consider disabling the button whencurrentProfileIdis missing.You already return early when
currentProfileIdis not present, but the button is still rendered. To avoid user confusion, you could disable or hide the button whencurrentProfileIdis missing to reflect that toggling follow is unavailable.
37-44: Handle multiple errors more gracefully.When multiple errors occur, you display each in a separate toast. Consider presenting them in a more user-friendly manner (e.g., combining the error messages into a single toast) to avoid overwhelming the user with multiple pop-ups.
packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/styled.tsx (1)
6-11: Use a semantic color name that aligns with the component's intent.
While using the error palette for unblocked states might be intentional, consider whether the red hue accurately conveys the user’s action (i.e., blocking vs. unblocking). If the unblocked state is typically not an error, you may want to apply a palette color that more closely reflects this status.packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/types.ts (2)
5-7: Consider normalizing theisBlockedtype definition.
Havingboolean | nullcan introduce edge cases whereisBlockedis explicitlynull. If the code does not handle anullscenario differently fromfalse, a simplerbooleantype may be sufficient.
9-15: Add clear documentation for optional callbacks.
BecausehandleErrorandhandleCloseMenuare optional, adding concise JSDoc or TS doc comments clarifying the conditions under which these callbacks are invoked would help with maintainability.packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/constants.ts (1)
1-14: Consider externalizing text for i18n support.
As you expand into different locales or languages, storing these strings in an i18n framework or resource dictionary can streamline translation and maintenance.packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/index.tsx (4)
45-47: Ensure consistent blocking logic whenisBlockedByMeis toggled quickly.
Although you guard against concurrent mutation by checkingisMutationInFlight, a user with a high-latency connection might manage to trigger multiple clicks quickly. Consider adding a brief disabled state after the action completes or a short throttle/debounce in the UI to further prevent conflicting requests.
72-94: Minor layout improvement: unify button usage.
For consistency, consider using yourActionButtonin place ofButtonto maintain uniform styling, unless the design specifically calls for a different button style here.
97-101: Emphasize user context in the dialog title.
Appending the user’s name is a helpful detail. To improve clarity, you might also include the user’s handle if one exists, ensuring they know precisely who is being blocked/unblocked.
105-106: Encourage final confirmation text for potential irreversible actions.
Blocking can be perceived as significant. If the design allows, consider adding “Are you sure?” or a similarly strong confirmation phrase to emphasize that it may be a permanent or serious action.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
packages/components/__generated__/BlockToggleFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/BlockToggleMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/FollowToggleMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/FollowToggleUpdatableFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ProfileComponentFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ProfileUpdateMutation.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
packages/components/CHANGELOG.md(2 hunks)packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/constants.ts(1 hunks)packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/index.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/styled.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/types.ts(1 hunks)packages/components/modules/profiles/ProfileComponent/FollowToggleButton/index.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/FollowToggleButton/types.ts(1 hunks)packages/components/modules/profiles/ProfileComponent/index.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/styled.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/types.ts(1 hunks)packages/components/modules/profiles/graphql/mutations/BlockToggle.ts(1 hunks)packages/components/modules/profiles/graphql/mutations/FollowToggle.ts(1 hunks)packages/components/modules/profiles/graphql/mutations/ProfileUpdate.ts(1 hunks)packages/components/modules/profiles/graphql/queries/BlockToggle.ts(1 hunks)packages/components/modules/profiles/graphql/queries/FollowToggleUpdatableFragment.ts(1 hunks)packages/components/modules/profiles/graphql/queries/ProfileComponent.ts(1 hunks)packages/components/modules/profiles/index.ts(1 hunks)packages/components/package.json(2 hunks)packages/design-system/.storybook/preview.ts(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/icons/BlockIcon/index.tsx(1 hunks)packages/design-system/components/icons/ThreeDotsIcon/index.tsx(1 hunks)packages/design-system/components/icons/UnblockIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(2 hunks)packages/design-system/components/images/ImageWithFallback/__storybook__/stories.tsx(1 hunks)packages/design-system/components/images/ImageWithFallback/index.tsx(1 hunks)packages/design-system/components/images/ImageWithFallback/types.ts(1 hunks)packages/design-system/components/images/index.ts(1 hunks)packages/design-system/index.ts(1 hunks)packages/design-system/package.json(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (26)
- packages/design-system/components/icons/index.ts
- packages/components/modules/profiles/ProfileComponent/types.ts
- packages/design-system/package.json
- packages/components/package.json
- packages/design-system/index.ts
- packages/components/modules/profiles/graphql/queries/BlockToggle.ts
- packages/design-system/.storybook/preview.ts
- packages/wagtail/package.json
- packages/design-system/components/images/index.ts
- packages/components/modules/profiles/graphql/queries/FollowToggleUpdatableFragment.ts
- packages/components/modules/profiles/graphql/queries/ProfileComponent.ts
- packages/design-system/components/icons/ThreeDotsIcon/index.tsx
- packages/design-system/components/images/ImageWithFallback/types.ts
- packages/components/modules/profiles/index.ts
- packages/design-system/components/images/ImageWithFallback/storybook/stories.tsx
- packages/components/modules/profiles/graphql/mutations/BlockToggle.ts
- packages/wagtail/CHANGELOG.md
- packages/design-system/components/icons/BlockIcon/index.tsx
- packages/design-system/CHANGELOG.md
- packages/components/modules/profiles/graphql/mutations/ProfileUpdate.ts
- packages/components/CHANGELOG.md
- packages/design-system/components/icons/UnblockIcon/index.tsx
- packages/components/modules/profiles/ProfileComponent/index.tsx
- packages/design-system/components/images/ImageWithFallback/index.tsx
- packages/components/modules/profiles/graphql/mutations/FollowToggle.ts
- packages/components/modules/profiles/ProfileComponent/styled.tsx
🔇 Additional comments (3)
packages/components/modules/profiles/ProfileComponent/FollowToggleButton/index.tsx (1)
61-62: Ensure data consistency when updating local records.
The local update modifies isFollowedByMe and followersCount without a concurrent refresh. If the server state changes or if multiple follow/unfollow requests occur in quick succession, there is a risk of stale data. Consider adding a refetch or a subscription to keep local data in sync.
packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/index.tsx (2)
56-65: Verify that multiple errors display as intended.
You're looping over each error to display separate toast messages. If the number of errors grows, the user might be flooded with messages. Please verify whether a single aggregated message is more appropriate or if multiple toasts are desired.
109-119: Leverage a consistent color scheme for the CTA.
You conditionally style the ActionButton with isBlocked. This is good, but if the user interface commonly uses a distinctive color for destructive actions, confirm that this color aligns with the rest of the product design.
pt-tsl
force-pushed
the
BA-2007-social-page-follow-unfollow
branch
from
05fb7d3 to
861efb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/components/modules/profiles/ProfileComponent/index.tsx (2)
110-120: Consider adjusting the alt text for accessibility.While
"Home Banner"may be a descriptive label, users might benefit from more personal context, such as"Profile banner for {profile?.name}". This is especially valuable for screen readers.
80-80: Use configuration-driven navigation.Hardcoding
'/user/settings'might make future refactoring more difficult if this path changes. Consider centralizing route definitions in a configuration to maintain consistency throughout the app.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
packages/components/__generated__/BlockToggleFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/BlockToggleMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/FollowToggleMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/FollowToggleUpdatableFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ProfileComponentFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ProfileUpdateMutation.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
packages/components/CHANGELOG.md(2 hunks)packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/constants.ts(1 hunks)packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/index.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/styled.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/types.ts(1 hunks)packages/components/modules/profiles/ProfileComponent/FollowToggleButton/index.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/FollowToggleButton/types.ts(1 hunks)packages/components/modules/profiles/ProfileComponent/index.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/styled.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/types.ts(1 hunks)packages/components/modules/profiles/graphql/mutations/BlockToggle.ts(1 hunks)packages/components/modules/profiles/graphql/mutations/FollowToggle.ts(1 hunks)packages/components/modules/profiles/graphql/mutations/ProfileUpdate.ts(1 hunks)packages/components/modules/profiles/graphql/queries/BlockToggle.ts(1 hunks)packages/components/modules/profiles/graphql/queries/FollowToggleUpdatableFragment.ts(1 hunks)packages/components/modules/profiles/graphql/queries/ProfileComponent.ts(1 hunks)packages/components/modules/profiles/index.ts(1 hunks)packages/components/package.json(2 hunks)packages/design-system/.storybook/preview.ts(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/icons/BlockIcon/index.tsx(1 hunks)packages/design-system/components/icons/ThreeDotsIcon/index.tsx(1 hunks)packages/design-system/components/icons/UnblockIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(2 hunks)packages/design-system/components/images/ImageWithFallback/__storybook__/stories.tsx(1 hunks)packages/design-system/components/images/ImageWithFallback/index.tsx(1 hunks)packages/design-system/components/images/ImageWithFallback/types.ts(1 hunks)packages/design-system/components/images/index.ts(1 hunks)packages/design-system/index.ts(1 hunks)packages/design-system/package.json(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (29)
- packages/wagtail/CHANGELOG.md
- packages/design-system/index.ts
- packages/design-system/package.json
- packages/components/modules/profiles/graphql/queries/FollowToggleUpdatableFragment.ts
- packages/components/modules/profiles/graphql/mutations/FollowToggle.ts
- packages/wagtail/package.json
- packages/design-system/.storybook/preview.ts
- packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/styled.tsx
- packages/design-system/components/images/ImageWithFallback/types.ts
- packages/design-system/components/images/index.ts
- packages/design-system/components/images/ImageWithFallback/storybook/stories.tsx
- packages/design-system/components/icons/BlockIcon/index.tsx
- packages/design-system/components/icons/UnblockIcon/index.tsx
- packages/components/modules/profiles/graphql/queries/ProfileComponent.ts
- packages/design-system/components/icons/ThreeDotsIcon/index.tsx
- packages/design-system/CHANGELOG.md
- packages/components/modules/profiles/graphql/mutations/BlockToggle.ts
- packages/components/modules/profiles/ProfileComponent/types.ts
- packages/components/modules/profiles/graphql/queries/BlockToggle.ts
- packages/design-system/components/images/ImageWithFallback/index.tsx
- packages/design-system/components/icons/index.ts
- packages/components/modules/profiles/index.ts
- packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/constants.ts
- packages/components/CHANGELOG.md
- packages/components/modules/profiles/graphql/mutations/ProfileUpdate.ts
- packages/components/package.json
- packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/types.ts
- packages/components/modules/profiles/ProfileComponent/BlockButtonWithDialog/index.tsx
- packages/components/modules/profiles/ProfileComponent/styled.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Lint
- GitHub Check: Component Test Applications and Packages
🔇 Additional comments (3)
packages/components/modules/profiles/ProfileComponent/index.tsx (3)
50-56: Ensure handling of missing environment variables.If
NEXT_PUBLIC_APP_BASE_URLis not defined,urlwill be concatenated withundefined, potentially confusing users. Provide a fallback domain or gracefully handle the case where this environment variable is missing.Would you like me to generate a shell script to verify whether
NEXT_PUBLIC_APP_BASE_URLis defined across all relevant build configurations or code references?
58-69: Review formatting logic for large follow counts.Currently, the function abbreviates any count greater than 1000 and uses a threshold of 1,050,000 for further formatting. Confirm that these thresholds match your specification (for example, "1k" for 1,000 and "1M" for 1,000,000).
123-128: Add descriptive alt text to the avatar for accessibility.The avatar is missing a meaningful
altprop. A prior review comment highlighted this concern. Provide an alt text like"Profile avatar of {profile.name}"to enhance accessibility.
| targetId?: string | ||
| isFollowedByMe: boolean | null | undefined | ||
| currentProfileId?: string | ||
| profileRef?: any |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any for profileRef.
Strong typing is beneficial to prevent runtime errors and improve code readability. If you're passing a Relay fragment reference, use the generated TypeScript types rather than any.
-profileRef?: any
+profileRef?: FollowToggleButton_profileRef$keyCommittable suggestion skipped: line range outside the PR's diff.
| errors?.forEach((error) => { | ||
| sendToast(error.message, { type: 'error' }) | ||
| }) | ||
| if (errors) { | ||
| return | ||
| } | ||
| const follow = response.followToggle?.follow | ||
|
|
||
| commitLocalUpdate(environment, (store: RecordSourceSelectorProxy) => { | ||
| if (profileRef == null) { | ||
| return | ||
| } | ||
|
|
||
| const { updatableData } = store.readUpdatableFragment( | ||
| FollowToggleUpdatableFragment, | ||
| profileRef, | ||
| ) | ||
|
|
||
| const updatedIsFollowedByMe = follow?.node?.target?.isFollowedByMe ?? false | ||
| const updatedFollowersCount = | ||
| follow?.node?.target?.followersCount ?? updatableData.followersCount - 1 | ||
|
|
There was a problem hiding this comment.
Fix potential inconsistency in follower count updates.
When follow?.node?.target?.followersCount is null, the code defaults to subtracting 1, which may be incorrect if the user has just followed. This can lead to inaccuracies in the UI.
Below is a possible logic fix that increments or decrements based on the new isFollowedByMe state:
...
const follow = response.followToggle?.follow
...
const updatedIsFollowedByMe = follow?.node?.target?.isFollowedByMe ?? false
-const updatedFollowersCount =
- follow?.node?.target?.followersCount ?? updatableData.followersCount - 1
+let updatedFollowersCount = follow?.node?.target?.followersCount
+if (updatedFollowersCount == null) {
+ updatedFollowersCount = updatedIsFollowedByMe
+ ? updatableData.followersCount + 1
+ : updatableData.followersCount - 1
+}
updatableData.isFollowedByMe = updatedIsFollowedByMe
updatableData.followersCount = updatedFollowersCount
...📝 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.
| errors?.forEach((error) => { | |
| sendToast(error.message, { type: 'error' }) | |
| }) | |
| if (errors) { | |
| return | |
| } | |
| const follow = response.followToggle?.follow | |
| commitLocalUpdate(environment, (store: RecordSourceSelectorProxy) => { | |
| if (profileRef == null) { | |
| return | |
| } | |
| const { updatableData } = store.readUpdatableFragment( | |
| FollowToggleUpdatableFragment, | |
| profileRef, | |
| ) | |
| const updatedIsFollowedByMe = follow?.node?.target?.isFollowedByMe ?? false | |
| const updatedFollowersCount = | |
| follow?.node?.target?.followersCount ?? updatableData.followersCount - 1 | |
| errors?.forEach((error) => { | |
| sendToast(error.message, { type: 'error' }) | |
| }) | |
| if (errors) { | |
| return | |
| } | |
| const follow = response.followToggle?.follow | |
| commitLocalUpdate(environment, (store: RecordSourceSelectorProxy) => { | |
| if (profileRef == null) { | |
| return | |
| } | |
| const { updatableData } = store.readUpdatableFragment( | |
| FollowToggleUpdatableFragment, | |
| profileRef, | |
| ) | |
| const updatedIsFollowedByMe = follow?.node?.target?.isFollowedByMe ?? false | |
| let updatedFollowersCount = follow?.node?.target?.followersCount | |
| if (updatedFollowersCount == null) { | |
| updatedFollowersCount = updatedIsFollowedByMe | |
| ? updatableData.followersCount + 1 | |
| : updatableData.followersCount - 1 | |
| } |
pt-tsl
force-pushed
the
BA-2007-social-page-follow-unfollow
branch
2 times, most recently
from
d86535f to
fb5ab41
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/components/modules/profiles/ProfileComponent/FollowToggleButton/index.tsx (1)
39-60:⚠️ Potential issueFix follower count update logic.
The current logic for updating the follower count is inconsistent with the user action.
const updatedIsFollowedByMe = follow?.node?.target?.isFollowedByMe ?? false -const updatedFollowersCount = - follow?.node?.target?.followersCount ?? updatableData.followersCount - 1 +let updatedFollowersCount = follow?.node?.target?.followersCount +if (updatedFollowersCount == null) { + updatedFollowersCount = updatedIsFollowedByMe + ? updatableData.followersCount + 1 + : updatableData.followersCount - 1 +}
🧹 Nitpick comments (6)
packages/components/modules/profiles/ProfileComponent/FollowToggleButton/index.tsx (2)
15-25: Add prop validation for required fields.Consider adding runtime validation for required props to fail fast and provide clear error messages during development.
const FollowToggleButton: FC<FollowToggleButtonProps> = ({ targetId, isFollowedByMe, currentProfileId, profileRef, }) => { + if (process.env.NODE_ENV !== 'production') { + if (!targetId) console.warn('FollowToggleButton: targetId is required') + if (!currentProfileId) console.warn('FollowToggleButton: currentProfileId is required') + if (!profileRef) console.warn('FollowToggleButton: profileRef is required') + } const [commitMutation, isMutationInFlight] = useMutation<FollowToggleMutation>(FollowToggleMutationQuery)
68-81: Enhance button accessibility.Add ARIA labels to improve screen reader experience.
<Button onClick={toggleFollow} + aria-label={isFollowedByMe ? 'Unfollow user' : 'Follow user'} startIcon={ isFollowedByMe ? <Iconify icon="ci:check-all" /> : <Iconify icon="mingcute:add-line" /> }packages/components/modules/profiles/ProfileComponent/index.tsx (4)
56-67: Improve number formatting logicThe current implementation has magic numbers and complex conditions. Consider:
- Using constants for magic numbers
- Simplifying the mantissa precision logic
- Adding JSDoc for better documentation
+const THOUSAND = 1000 +const MILLION = 1000000 + +/** + * Formats follow count with K/M suffix for large numbers + * @param count - The number to format + * @returns Formatted string (e.g., 1.5K, 1M) + */ const formatFollowCount = (count?: number | null) => { if (!count || count === 0) { return 0 } - if (count <= 1000) { + if (count < THOUSAND) { return count } - if (count < 1050000) { - return numbro(count).format({ average: true }) - } - return numbro(count).format({ average: true, mantissa: 1 }) + return numbro(count).format({ + average: true, + mantissa: count >= MILLION ? 1 : 0 + }) }
69-103: Add error boundary for profile action buttonsThe profile action buttons should gracefully handle rendering errors to prevent the entire component from crashing.
Consider wrapping the buttons in an error boundary component:
// ProfileActionErrorBoundary.tsx class ProfileActionErrorBoundary extends React.Component { componentDidCatch(error: Error) { console.error('Error rendering profile actions:', error); } render() { return this.props.children; } } // Usage in renderProfileUpdatesButtons return ( <ProfileActionErrorBoundary> <div className="flex flex-row gap-2"> {/* existing button logic */} </div> </ProfileActionErrorBoundary> );
168-199: Reduce code duplication in menu itemsThe menu items are duplicated between desktop and mobile views. Consider extracting them into a separate component.
const ProfileMenuItems = ({ onShare, profile, onClose, currentProfileId }) => ( <> <MenuItem onClick={onShare} disableRipple> <ShareIcon /> Share profile </MenuItem> {profile && ( <BlockButtonWithDialog target={profile} handleCloseMenu={onClose} currentProfileId={currentProfileId} isMenu /> )} </> ); // Usage {!smDown && ( <StyledMenu anchorEl={anchorEl} open={open} onClose={handleClose}> <ProfileMenuItems onShare={handleShareClick} profile={profile} onClose={handleClose} currentProfileId={currentProfileId} /> </StyledMenu> )}
156-156: Handle null/undefined biography textAdd a fallback for when biography is null or undefined.
-<p className="prose-caption text-text-secondary">{profile?.biography}</p> +<p className="prose-caption text-text-secondary"> + {profile?.biography || 'No biography available'} +</p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/components/modules/profiles/ProfileComponent/FollowToggleButton/index.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/FollowToggleButton/types.ts(1 hunks)packages/components/modules/profiles/ProfileComponent/index.tsx(1 hunks)packages/components/modules/profiles/ProfileComponent/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/modules/profiles/ProfileComponent/types.ts
- packages/components/modules/profiles/ProfileComponent/FollowToggleButton/types.ts
🔇 Additional comments (2)
packages/components/modules/profiles/ProfileComponent/index.tsx (2)
120-126: Add meaningful alt text for the profile avatarThe avatar image is missing an alt text for accessibility.
33-34: Add prop validation for profileRefConsider adding runtime validation to ensure profileRef is properly initialized before being used with useFragment to prevent potential runtime errors.
| commitMutation({ | ||
| variables: { | ||
| input: { | ||
| targetObjectId: targetId, | ||
| actorObjectId: currentProfileId, | ||
| }, | ||
| }, | ||
| onCompleted: (response, errors) => { | ||
| errors?.forEach((error) => { | ||
| sendToast(error.message, { type: 'error' }) | ||
| }) | ||
| if (errors) { | ||
| return | ||
| } | ||
| const follow = response.followToggle?.follow | ||
|
|
||
| commitLocalUpdate(environment, (store: RecordSourceSelectorProxy) => { | ||
| if (profileRef == null) { | ||
| return | ||
| } | ||
|
|
||
| const { updatableData } = store.readUpdatableFragment( | ||
| FollowToggleUpdatableFragment, | ||
| profileRef, | ||
| ) | ||
|
|
||
| const updatedIsFollowedByMe = follow?.node?.target?.isFollowedByMe ?? false | ||
| const updatedFollowersCount = | ||
| follow?.node?.target?.followersCount ?? updatableData.followersCount - 1 | ||
|
|
||
| updatableData.isFollowedByMe = updatedIsFollowedByMe | ||
| updatableData.followersCount = updatedFollowersCount | ||
| }) | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add optimistic updates.
- Add network error handling using
onErrorcallback. - Implement optimistic updates for better UX.
commitMutation({
+ onError: (error) => {
+ sendToast('Failed to update follow status. Please try again.', { type: 'error' })
+ },
+ optimisticResponse: {
+ followToggle: {
+ follow: {
+ node: {
+ target: {
+ id: targetId,
+ isFollowedByMe: !isFollowedByMe,
+ followersCount: isFollowedByMe
+ ? updatableData.followersCount - 1
+ : updatableData.followersCount + 1
+ }
+ }
+ }
+ }
+ },
variables: {Committable suggestion skipped: line range outside the PR's diff.
| const handleShareClick = () => { | ||
| const path = profile?.urlPath?.path ?? `/profile/${profile?.id}` | ||
| const url = [process.env.NEXT_PUBLIC_APP_BASE_URL, path].join('') | ||
| navigator.clipboard.writeText(url) | ||
| sendToast('Profile URL copied to clipboard!', { type: 'success' }) | ||
| handleClose() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance clipboard operations with error handling and accessibility
The clipboard operation should:
- Handle potential failures
- Support keyboard accessibility
- Use proper ARIA attributes for screen readers
const handleShareClick = () => {
const path = profile?.urlPath?.path ?? `/profile/${profile?.id}`
const url = [process.env.NEXT_PUBLIC_APP_BASE_URL, path].join('')
- navigator.clipboard.writeText(url)
- sendToast('Profile URL copied to clipboard!', { type: 'success' })
+ navigator.clipboard.writeText(url)
+ .then(() => {
+ sendToast('Profile URL copied to clipboard!', { type: 'success' })
+ })
+ .catch((error) => {
+ console.error('Failed to copy URL:', error)
+ sendToast('Failed to copy URL to clipboard', { type: 'error' })
+ })
handleClose()
}📝 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 handleShareClick = () => { | |
| const path = profile?.urlPath?.path ?? `/profile/${profile?.id}` | |
| const url = [process.env.NEXT_PUBLIC_APP_BASE_URL, path].join('') | |
| navigator.clipboard.writeText(url) | |
| sendToast('Profile URL copied to clipboard!', { type: 'success' }) | |
| handleClose() | |
| } | |
| const handleShareClick = () => { | |
| const path = profile?.urlPath?.path ?? `/profile/${profile?.id}` | |
| const url = [process.env.NEXT_PUBLIC_APP_BASE_URL, path].join('') | |
| navigator.clipboard.writeText(url) | |
| .then(() => { | |
| sendToast('Profile URL copied to clipboard!', { type: 'success' }) | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to copy URL:', error) | |
| sendToast('Failed to copy URL to clipboard', { type: 'error' }) | |
| }) | |
| handleClose() | |
| } |
pt-tsl
force-pushed
the
BA-2007-social-page-follow-unfollow
branch
from
fb5ab41 to
e2dec1b
Compare
|
Acceptance Criteria
Context
This user story focuses on adding a follow/unfollow feature to the user profile page. The profile page will display the number of followers and following users. Additionally, a follow/unfollow button will be provided for users to follow or unfollow other profiles.
- Display Followers and Following Numbers:
- Follow/Unfollow Button:
Given that a user wants to follow or unfollow another user
When the user views another user's profile
Then display a follow/unfollow button (this button should not be visible if a user is looking at their own profile)
Given that the user clicks the follow button
When the following action is successful
Then update the follower count and change the button to "Unfollow"
Given that the user clicks the unfollow button
When the unfollow action is successful
Then update the follower count and change the button to "Follow"
Frontend
Backend
Design Link: https://www.figma.com/design/XRD6wSl1m8Kz6XUcAy5CLp/BaseApp-Minimal-v5.4.0?node-id=2520-129613&t=Fzsf5WBt3yBHdzxa-0
Summary by CodeRabbit
New Features
ProfileComponentfor displaying user profile information.FollowToggleButtonfor following/unfollowing users.BlockButtonWithDialogfor blocking/unblocking profiles.FollowTogglebutton for managing follow status.BlockIcon,UnblockIcon,ThreeDotsIcon.ImageWithFallbackcomponent for improved image handling.Improvements
Dependency Updates
@baseapp-frontend/design-systemto version 0.0.27.numbrolibrary.