Ba 2266 report profile #249
Conversation
… BA-2266-report-profile
… BA-2266-report-profile
luciobj
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThis pull request introduces multiple new features and modifications. A new GraphQL mutation ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant RB as ReportButtonWithDialog
participant GQ as GraphQL Server
participant MC as useReportCreateMutation
U->>RB: Click "Report" menu item
RB->>RB: Open multi-step report dialog
U->>RB: Select report type/subtype & enter details
RB->>GQ: Fetch report types (ReportTypeListQuery)
GQ-->>RB: Return report types
U->>RB: Submit report details
RB->>MC: Commit mutation (ReportCreateMutation)
MC->>GQ: Execute mutation with provided input
GQ-->>MC: Return success or error response
MC-->>RB: Callback with mutation result
alt Success
RB->>U: Show confirmation message
else Error
RB->>U: Display error toast via sendToast
end
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/web/ProfileComponent/BlockButtonWithDialog/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/profiles/common/graphql/mutations/ReportCreate.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/profiles/web/ProfileComponent/ReportButtonWithDialog/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 4
🧹 Nitpick comments (7)
packages/design-system/components/common/icons/__storybook__/Iconography.mdx (1)
2-2: Fix import path syntaxThere's a syntax issue with the import path that may cause confusion.
-import * as AllIcons from '..' +import * as AllIcons from '..'🧰 Tools
🪛 LanguageTool
[typographical] ~2-~2: Two consecutive dots
Context: ...book/blocks' import * as AllIcons from '..' <Meta title="@baseapp-frontend | des...(DOUBLE_PUNCTUATION)
packages/components/modules/profiles/web/ProfileComponent/index.tsx (1)
120-120: Ensure targetId is always definedWhile there's a conditional check for profile existence, it would be safer to ensure that the targetId is always defined when passed to ReportButtonWithDialog.
-{profile && <ReportButtonWithDialog handleClose={handleClose} targetId={profile?.id} />} +{profile && profile.id && <ReportButtonWithDialog handleClose={handleClose} targetId={profile.id} />}packages/components/modules/profiles/web/ProfileComponent/BlockButtonWithDialog/index.tsx (1)
74-111: Enhanced conditional rendering for menu vs. button contextsThe updated implementation provides clear visual distinction between menu item and standalone button presentations. The error color styling is consistently applied, improving user experience.
Consider refactoring the duplicated conditional logic for block/unblock states into a shared helper function to reduce repetition and improve maintainability.
+ const renderBlockContent = (isMenuItem = false) => { + const iconColor = isMenuItem ? 'error.main' : (isBlockedByMe ? 'inherit' : 'error.main'); + return isBlockedByMe ? ( + <> + <UnblockIcon sx={{ color: iconColor, marginRight: '5px' }} /> + {isMenuItem ? 'Unblock profile' : 'Unblock'} + </> + ) : ( + <> + <BlockIcon sx={{ color: 'error.main', marginRight: '5px' }} /> + Block profile + </> + ); + }; return ( <> {isMenu ? ( <MenuItem onClick={handleOpen} disableRipple> <Typography variant="body2" color="error.main" noWrap> - {isBlockedByMe ? ( - <> - <UnblockIcon sx={{ color: 'error.main', marginRight: '5px' }} /> - Unblock profile - </> - ) : ( - <> - <BlockIcon sx={{ color: 'error.main', marginRight: '5px' }} /> - Block profile - </> - )} + {renderBlockContent(true)} </Typography> </MenuItem> ) : ( <Button variant="contained" onClick={handleOpen} sx={{ justifyContent: 'center' }} size="medium" > <Typography variant="body2" color="inherit" noWrap> - {isBlockedByMe ? ( - <> - <UnblockIcon sx={{ color: 'inherit', marginRight: '5px' }} /> - Unblock - </> - ) : ( - <> - <BlockIcon sx={{ color: 'error.main', marginRight: '5px' }} /> - Block profile - </> - )} + {renderBlockContent()} </Typography> </Button> )}packages/components/modules/profiles/web/ProfileComponent/ReportButtonWithDialog/index.tsx (2)
24-42: Consider resetting form state when dialog opens/closesThe component initializes state but doesn't reset it when the dialog is reopened, which could lead to stale data.
const onClose = () => { handleClose() setCurrentStep('report') + setReportType(undefined) + setReportSubType(undefined) + setReportText('') } + const handleOpen = () => { + setReportType(undefined) + setReportSubType(undefined) + setReportText('') + setCurrentStep('report') + setIsReportModalOpen(true) + }And update the MenuItem click handler:
- <MenuItem onClick={() => setIsReportModalOpen(true)} disableRipple> + <MenuItem onClick={handleOpen} disableRipple>
74-196: Enhance the step navigation with back buttonsThe multi-step dialog doesn't provide a way to go back to previous steps, forcing users to cancel and restart if they make a mistake.
Add a function to handle going back:
const handleBack = () => { switch (currentStep) { case 'subTypes': setCurrentStep('report') setReportType(undefined) break case 'reportText': if (reportType?.subTypes?.edges?.length) { setCurrentStep('subTypes') setReportSubType(undefined) } else { setCurrentStep('report') setReportType(undefined) } setReportText('') break case 'summary': setCurrentStep('reportText') break default: break } }Then add back buttons to the appropriate steps, for example in the reportText step:
<> <Typography variant="h5">How would you describe the problem?</Typography> <Typography variant="body2"> Use the text field below to explain what the problem you are reporting is. </Typography> <Divider /> <TextField fullWidth multiline rows={4} value={reportText} onChange={(e) => setReportText(e.target.value)} placeholder="I find the post to be offensive..." /> + <Box display="flex" justifyContent="space-between"> + <Button onClick={handleBack}>Back</Button> - <Button onClick={() => setCurrentStep('summary')}>Confirm</Button> + <Button + onClick={() => setCurrentStep('summary')} + disabled={!reportText.trim()} + > + Confirm + </Button> + </Box> </>packages/components/schema.graphql (1)
1657-1669: Expanded ReportsCount with detailed categoriesThe ReportsCount type now includes specific categories for different types of reports, which enables more detailed reporting analytics.
However, there appears to be two spelling errors:
- Line 1665: "bulling" should be "bullying"
- Line 1668: "prostituition" should be "prostitution"
- bulling: Int + bullying: Int - prostituition: Int + prostitution: Intpackages/design-system/components/common/icons/FlagIcon/index.tsx (1)
1-36: Naming inconsistency: Component namedUnreadIconinFlagIcondirectoryThere's a naming mismatch between the component name (
UnreadIcon) and the directory/file name (FlagIcon). This could cause confusion for other developers. Consider renaming either the component or the directory to ensure consistency.Additionally, the icon appears to be a flag (based on the SVG paths), but is named
UnreadIcon. The naming should reflect the icon's visual appearance and purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/components/modules/profiles/common/graphql/mutations/ReportCreate.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/ReportTypeList.ts(1 hunks)packages/components/modules/profiles/web/ProfileComponent/BlockButtonWithDialog/index.tsx(2 hunks)packages/components/modules/profiles/web/ProfileComponent/ReportButtonWithDialog/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileComponent/ReportButtonWithDialog/styled.tsx(1 hunks)packages/components/modules/profiles/web/ProfileComponent/ReportButtonWithDialog/types.ts(1 hunks)packages/components/modules/profiles/web/ProfileComponent/index.tsx(3 hunks)packages/components/schema.graphql(15 hunks)packages/design-system/components/common/icons/FlagIcon/index.tsx(1 hunks)packages/design-system/components/common/icons/__storybook__/Iconography.mdx(1 hunks)packages/design-system/components/common/icons/index.ts(1 hunks)packages/design-system/components/common/index.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/design-system/components/common/index.ts
🧰 Additional context used
🪛 LanguageTool
packages/design-system/components/common/icons/__storybook__/Iconography.mdx
[typographical] ~2-~2: Two consecutive dots
Context: ...book/blocks' import * as AllIcons from '..' <Meta title="@baseapp-frontend | des...
(DOUBLE_PUNCTUATION)
🔇 Additional comments (18)
packages/components/modules/profiles/common/graphql/queries/ReportTypeList.ts (1)
3-27: Well-structured GraphQL query for report typesThis query is well-designed, fetching both top-level report types and their sub-types with the necessary fields for building a reporting UI. The parameters are appropriate for filtering by top-level only and for specifying a target object.
packages/design-system/components/common/icons/__storybook__/Iconography.mdx (1)
8-14: Clean implementation of dynamic icon galleryThe implementation elegantly displays all icons with proper formatting of their names by inserting spaces before uppercase letters and removing the "Icon" suffix. The primary color styling provides good visual consistency.
packages/components/modules/profiles/web/ProfileComponent/index.tsx (1)
106-122: Good refactoring of menu options into a reusable fragmentThe creation of a
menuOptionsfragment that's used in both desktop and mobile views improves code maintainability. The conditional rendering for the BlockButtonWithDialog and ReportButtonWithDialog components is a good practice.packages/components/modules/profiles/common/graphql/mutations/ReportCreate.ts (2)
1-22: GraphQL mutation is well-structured with proper type selectionsThe mutation is properly defined with the required field selections for creating reports. It returns the target's reports with primary key and creation date, which provides sufficient data for confirmation.
24-49: Clean mutation hook with proper error handlingThe hook implementation follows best practices by:
- Properly typing the return tuple
- Using the notification system for error handling
- Preserving the original config callbacks
This ensures errors are visible to the user while still allowing component-specific handling.
packages/components/modules/profiles/web/ProfileComponent/ReportButtonWithDialog/index.tsx (1)
1-23: Types are properly defined for GraphQL response dataGood use of NonNullable to ensure type safety when working with potentially nullable GraphQL response data.
packages/components/schema.graphql (10)
204-204: New field added for tracking added participantsGood addition to improve tracking of chat room updates.
328-328: Consistent field addition across typesAdding the same field to both
ChatRoomOnRoomUpdateandChatRoomUpdatePayloadtypes maintains consistency in the API.
501-512: Well-defined generic node deletion typesThe new
DeleteNodeInputandDeleteNodePayloadtypes are well-structured for generic node deletion operations.
1144-1144: Profile type now implements ReportsInterfaceThis change properly integrates the reporting functionality into the Profile type.
1153-1153: ReportsCount type update for ProfileThe field type change from JSONString to ReportsCount provides stronger typing and better structure.
1225-1226: Added report-related fields to ProfileThese fields enable querying reports associated with a profile and the current user's reports.
1616-1616: Updated Report.reportType to use new ReportType typeThis change from an enum to a structured type provides more flexibility for representing report types.
1634-1634: Updated ReportCreateInput to use reportTypeIdThis change aligns with the new ReportType structure, using IDs for references instead of enum values.
1680-1707: Well-structured ReportType and connection typesThe new ReportType implementation includes all necessary fields and supports hierarchical report types through the parentType and subTypes fields.
1413-1417: New query fields for report typesThe addition of reportTypes and allReportTypes queries provides the necessary endpoints for fetching report type data.
packages/design-system/components/common/icons/index.ts (1)
1-3: Good implementation of a barrel file for icon exports.The 'use client' directive is appropriately placed for client-side rendering, and the export pattern makes imports cleaner throughout the application.
packages/design-system/components/common/icons/FlagIcon/index.tsx (1)
5-34: The SVG implementation follows good practicesThe component correctly utilizes Material-UI's
SvgIcon, properly handles props spreading, and usescurrentColorfor flexibility. The default styling is also well defined.
| export const TypeButton = styled(Button)(({ theme }) => ({ | ||
| variant: 'text', | ||
| borderRadius: 0, | ||
| justifyContent: 'space-between', | ||
| color: theme.palette.text.primary, | ||
| backgroundColor: 'transparent', | ||
| '&:hover': { | ||
| backgroundColor: theme.palette.background.neutral, | ||
| boxShadow: 'none', | ||
| }, | ||
| })) |
There was a problem hiding this comment.
Incorrect usage of variant property in styled component
The variant property is incorrectly placed within the styling object. In Material-UI's styled components, component props like variant should be set when using the component, not in the styling definition.
Fix the styled component by removing the variant from the styling object:
export const TypeButton = styled(Button)(({ theme }) => ({
- variant: 'text',
borderRadius: 0,
justifyContent: 'space-between',
color: theme.palette.text.primary,
backgroundColor: 'transparent',
'&:hover': {
backgroundColor: theme.palette.background.neutral,
boxShadow: 'none',
},
}))Then when using the component, set the variant as a prop:
<TypeButton variant="text">Button Text</TypeButton>| return ( | ||
| <> | ||
| <MenuItem onClick={() => setIsReportModalOpen(true)} disableRipple> | ||
| <Typography variant="body2" color="error.main" noWrap> | ||
| <FlagIcon sx={{ color: 'error.main', marginRight: '5px' }} /> | ||
| Report profile | ||
| </Typography> | ||
| </MenuItem> | ||
| <Dialog open={isReportModalOpen} onClose={onClose}> | ||
| <Box display="flex" flexDirection="column" padding={4} gap={2}> | ||
| <Typography variant="subtitle1"> | ||
| {currentStep === 'confirmation' ? 'Check Report' : 'Report'} | ||
| </Typography> | ||
| {steps?.find((step) => step.name === currentStep)?.content} | ||
| </Box> | ||
| </Dialog> | ||
| </> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA attributes for accessibility
The dialog is missing important accessibility attributes to ensure it's usable by all users, including those with screen readers.
<Dialog
open={isReportModalOpen}
onClose={onClose}
+ aria-labelledby="report-dialog-title"
+ aria-describedby="report-dialog-description"
>
<Box display="flex" flexDirection="column" padding={4} gap={2}>
- <Typography variant="subtitle1">
+ <Typography variant="subtitle1" id="report-dialog-title">
{currentStep === 'confirmation' ? 'Check Report' : 'Report'}
</Typography>
- {steps?.find((step) => step.name === currentStep)?.content}
+ <div id="report-dialog-description">
+ {steps?.find((step) => step.name === currentStep)?.content}
+ </div>
</Box>
</Dialog>📝 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 ( | |
| <> | |
| <MenuItem onClick={() => setIsReportModalOpen(true)} disableRipple> | |
| <Typography variant="body2" color="error.main" noWrap> | |
| <FlagIcon sx={{ color: 'error.main', marginRight: '5px' }} /> | |
| Report profile | |
| </Typography> | |
| </MenuItem> | |
| <Dialog open={isReportModalOpen} onClose={onClose}> | |
| <Box display="flex" flexDirection="column" padding={4} gap={2}> | |
| <Typography variant="subtitle1"> | |
| {currentStep === 'confirmation' ? 'Check Report' : 'Report'} | |
| </Typography> | |
| {steps?.find((step) => step.name === currentStep)?.content} | |
| </Box> | |
| </Dialog> | |
| </> | |
| ) | |
| } | |
| return ( | |
| <> | |
| <MenuItem onClick={() => setIsReportModalOpen(true)} disableRipple> | |
| <Typography variant="body2" color="error.main" noWrap> | |
| <FlagIcon sx={{ color: 'error.main', marginRight: '5px' }} /> | |
| Report profile | |
| </Typography> | |
| </MenuItem> | |
| <Dialog | |
| open={isReportModalOpen} | |
| onClose={onClose} | |
| aria-labelledby="report-dialog-title" | |
| aria-describedby="report-dialog-description" | |
| > | |
| <Box display="flex" flexDirection="column" padding={4} gap={2}> | |
| <Typography variant="subtitle1" id="report-dialog-title"> | |
| {currentStep === 'confirmation' ? 'Check Report' : 'Report'} | |
| </Typography> | |
| <div id="report-dialog-description"> | |
| {steps?.find((step) => step.name === currentStep)?.content} | |
| </div> | |
| </Box> | |
| </Dialog> | |
| </> | |
| ) | |
| } |
| const onClose = () => { | ||
| handleClose() | ||
| setCurrentStep('report') | ||
| } | ||
|
|
||
| const handleReport = () => { | ||
| if (isMutationInFlight || !targetId) { | ||
| return | ||
| } | ||
| commitMutation({ | ||
| variables: { | ||
| input: { | ||
| reportSubject: reportText, | ||
| reportTypeId: reportType?.id, | ||
| targetObjectId: targetId, | ||
| }, | ||
| }, | ||
| onCompleted: (_, errors) => { | ||
| if (!errors) { | ||
| setCurrentStep('confirmation') | ||
| return | ||
| } | ||
| onClose() | ||
| }, | ||
| onError: () => { | ||
| onClose() | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for report submission
The handleReport function doesn't validate if a report type is selected or if report text is provided.
const handleReport = () => {
- if (isMutationInFlight || !targetId) {
+ if (isMutationInFlight || !targetId || !reportType) {
return
}
commitMutation({
variables: {
input: {
reportSubject: reportText,
reportTypeId: reportType?.id,
targetObjectId: targetId,
},
},
onCompleted: (_, errors) => {
if (!errors) {
setCurrentStep('confirmation')
return
}
onClose()
},
onError: () => {
onClose()
},
})
}📝 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 onClose = () => { | |
| handleClose() | |
| setCurrentStep('report') | |
| } | |
| const handleReport = () => { | |
| if (isMutationInFlight || !targetId) { | |
| return | |
| } | |
| commitMutation({ | |
| variables: { | |
| input: { | |
| reportSubject: reportText, | |
| reportTypeId: reportType?.id, | |
| targetObjectId: targetId, | |
| }, | |
| }, | |
| onCompleted: (_, errors) => { | |
| if (!errors) { | |
| setCurrentStep('confirmation') | |
| return | |
| } | |
| onClose() | |
| }, | |
| onError: () => { | |
| onClose() | |
| }, | |
| }) | |
| } | |
| const onClose = () => { | |
| handleClose() | |
| setCurrentStep('report') | |
| } | |
| const handleReport = () => { | |
| if (isMutationInFlight || !targetId || !reportType) { | |
| return | |
| } | |
| commitMutation({ | |
| variables: { | |
| input: { | |
| reportSubject: reportText, | |
| reportTypeId: reportType?.id, | |
| targetObjectId: targetId, | |
| }, | |
| }, | |
| onCompleted: (_, errors) => { | |
| if (!errors) { | |
| setCurrentStep('confirmation') | |
| return | |
| } | |
| onClose() | |
| }, | |
| onError: () => { | |
| onClose() | |
| }, | |
| }) | |
| } |
| export interface ReportButtonWithDialogProps { | ||
| targetId?: string | ||
| handleClose: () => void | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Interface looks good, but should targetId be optional?
The interface is clean and well-defined. However, I'm curious about the optionality of targetId. If a report always needs a target, it might be better to make this required. Otherwise, ensure the component has logic to handle the case when targetId is undefined.
🏁 Script executed:
#!/bin/bash
# Search for usages of ReportButtonWithDialog to see how targetId is handled
rg -A 5 "ReportButtonWithDialog" --type=tsxLength of output: 71
🏁 Script executed:
#!/bin/bash
# Re-run the search for usages of ReportButtonWithDialog specifically in .tsx files
rg -A 5 "ReportButtonWithDialog" -g "*.tsx"Length of output: 3130
Action: Clarify the targetId Requirement
The interface is defined as follows:
export interface ReportButtonWithDialogProps {
targetId?: string
handleClose: () => void
}After verifying the usage in packages/components/modules/profiles/web/ProfileComponent/index.tsx, we see that the component is rendered conditionally—only when profile exists—and targetId is passed as profile?.id. This indicates that when the component is rendered, a target ID is always provided. To avoid any accidental misuse of ReportButtonWithDialog where a report might lack a target, it would be more robust to make targetId a required prop.
Recommendations:
-
If the button is always meant to report on an entity: Adjust the interface so that
targetIdis required.export interface ReportButtonWithDialogProps { targetId: string handleClose: () => void }
-
If there’s a valid use case for rendering the component without an associated target: Ensure that the component has proper logic to handle the scenario when
targetIdis undefined.
Please review and confirm which approach best matches the intended behavior of the reporting feature.
✅ Social - Report Profile
Description
Acceptance Criteria
BaseApp Profile Page
Backend
Design Link: https://www.figma.com/design/XRD6wSl1m8Kz6XUcAy5CLp/BaseApp---WEB?node-id=3768-79982&node-type=section&t=7l8Nbem4bL7a0USV-0
Demo: https://www.loom.com/share/03ac73145fff4e12a513ee421d0d00b5
Summary by CodeRabbit
New Features
Refactor
Documentation