Content Feed New Post #209
Conversation
rodrigonahid
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThe pull request introduces multiple exports and new components within the content-feed module. It adds a GraphQL mutation for creating content posts along with a corresponding hook. New React components are included for displaying a content feed, handling image uploads through drag-and-drop, and creating new posts. Additionally, styled components, types, and constants have been implemented to support routing, form management, and validation logic using libraries like react-hook-form and zod. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant N as NewContentPost
participant G as GraphQL Service
participant R as Router
participant T as Notifier
U->>N: Fill form and click submit
N->>G: Commit mutation with form data
G-->>N: Return success or error response
alt Error in mutation
N->>T: Display error toast
else Mutation successful
N->>T: Display success toast
N->>R: Redirect to the new post page
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/index.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🪛 GitHub Check: Build and Lint Packagespackages/components/index.ts[failure] 7-7: [failure] 6-6: [failure] 5-5: [failure] 4-4: [failure] 3-3: [failure] 2-2: [failure] 1-1: ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 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: 7
🧹 Nitpick comments (8)
packages/components/modules/content-feed/NewContentPost/types.ts (1)
1-4: Consider separating image types for better type safety.The
imagesfield allows mixed types (string[], File[], Blob[]) which could lead to type-checking issues. Consider using a discriminated union or separate fields for different image types.export interface ContentPostCreateForm { content: string - images?: string[] | File[] | Blob[] + images?: { + urls?: string[] + files?: (File | Blob)[] + } }packages/components/modules/content-feed/NewContentPost/constants.ts (1)
1-1: Consider using path alias for imports.The import path is verbose. Consider using a path alias to simplify imports.
-import { ContentPostCreateForm } from '@baseapp-frontend/components/modules/content-feed/NewContentPost/types' +import { ContentPostCreateForm } from '@/types'packages/components/modules/content-feed/ContentFeed/index.tsx (2)
10-35: Add TypeScript interface and improve accessibility.The component would benefit from TypeScript interface definition and accessibility improvements.
+ interface ContentFeedProps { + className?: string; + } - const ContentFeed = () => { + const ContentFeed: React.FC<ContentFeedProps> = ({ className }) => { const router = useRouter() const onNewPost = useCallback(() => { router.push('/new-post') }, [router]) return ( - <RootContainer> + <RootContainer className={className}> <HeaderContainer> <Typography component="h4" variant="h4"> Content Feed </Typography> <Button variant="outlined" color="inherit" onClick={onNewPost} disableRipple sx={{ maxWidth: 'fit-content' }} + aria-label="Create new post" > New Post </Button> </HeaderContainer> </RootContainer> ) }
23-31: Consider adding loading state to the button.Since the button triggers a navigation action, it would be good to add a loading state to prevent multiple clicks.
+ const [isNavigating, setIsNavigating] = useState(false); const onNewPost = useCallback(() => { + setIsNavigating(true); router.push('/new-post') }, [router]) <Button variant="outlined" color="inherit" onClick={onNewPost} disableRipple + disabled={isNavigating} sx={{ maxWidth: 'fit-content' }} aria-label="Create new post" > New Post </Button>packages/components/modules/content-feed/ContentFeedImage/styled.tsx (2)
3-22: Consider enhancing scrollbar accessibilityThe scrollbar styling looks good, but consider adding
@media (prefers-reduced-motion: reduce)to disable smooth scrolling for users who prefer reduced motion.export const ContentFeedImageContainer = styled(Box)(({ theme }) => ({ display: 'flex', marginBottom: '16px', overflow: 'auto', paddingBottom: '6px', + '@media (prefers-reduced-motion: reduce)': { + '::-webkit-scrollbar': { + scrollBehavior: 'auto', + }, + }, '::-webkit-scrollbar': { height: '6px', },
56-70: Add hover state color contrast for accessibilityThe RemoveFileButton maintains the same background color on hover, which might not provide enough visual feedback for users.
export const RemoveFileButton = styled('button')(({ theme }) => ({ position: 'absolute', top: '4px', right: '4px', width: '28px', height: '28px', display: 'flex', alignItems: 'center', justifyContent: 'center', borderRadius: '0 6px 0 6px', backgroundColor: theme.palette.grey[800], '&:hover': { - backgroundColor: theme.palette.grey[800], + backgroundColor: theme.palette.grey[900], + opacity: 0.9, }, }))packages/components/modules/content-feed/NewContentPost/index.tsx (1)
89-98: Add loading state feedbackThe LoadingButton should provide visual feedback about the loading state to improve user experience.
<LoadingButton color="inherit" type="submit" loading={isMutationInFlight} disabled={!isDirty || !isValid || isMutationInFlight} + loadingPosition="start" + startIcon={<></>} sx={{ maxWidth: 'fit-content', justifySelf: 'end' }} > Publish </LoadingButton>packages/components/modules/content-feed/ContentFeedImage/index.tsx (1)
38-39: Define constants outside componentMove constants outside the component to prevent recreation on each render.
+const DEFAULT_IMAGE_FORMATS = 'image/png, image/gif, image/jpeg' +const DEFAULT_IMAGE_MAX_SIZE = 10 * 1024 * 1024 // 10MB + const ContentFeedImage = ({ form }: IContentFeedImageProps) => { const [selectedUploadedFile, setSelectedUploadedFiles] = useState<File>() const [isDragging, setIsDragging] = useState(false) - - const DEFAULT_IMAGE_FORMATS = 'image/png, image/gif, image/jpeg' - const DEFAULT_IMAGE_MAX_SIZE = 10 * 1024 * 1024
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/components/index.ts(1 hunks)packages/components/modules/content-feed/ContentFeed/index.tsx(1 hunks)packages/components/modules/content-feed/ContentFeed/styled.tsx(1 hunks)packages/components/modules/content-feed/ContentFeedImage/index.tsx(1 hunks)packages/components/modules/content-feed/ContentFeedImage/styled.tsx(1 hunks)packages/components/modules/content-feed/NewContentPost/constants.ts(1 hunks)packages/components/modules/content-feed/NewContentPost/index.tsx(1 hunks)packages/components/modules/content-feed/NewContentPost/styled.tsx(1 hunks)packages/components/modules/content-feed/NewContentPost/types.ts(1 hunks)packages/components/modules/content-feed/graphql/mutations/ContentPostCreate.ts(1 hunks)packages/components/modules/content-feed/index.ts(1 hunks)
⏰ 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 (4)
packages/components/modules/content-feed/NewContentPost/types.ts (1)
6-8: LGTM! Well-structured type for file uploads.The template literal type for dynamic keys is a good approach for handling multiple file uploads.
packages/components/modules/content-feed/index.ts (1)
1-7: LGTM! Clean export structure.The export structure is well-organized and follows common patterns.
packages/components/index.ts (1)
5-5: LGTM! Consistent module export pattern.The content-feed module export follows the established pattern and maintains alphabetical order.
packages/components/modules/content-feed/graphql/mutations/ContentPostCreate.ts (1)
4-22: LGTM! Well-structured GraphQL mutation.The mutation is well-defined with proper error handling and type safety. The structure follows GraphQL best practices by including both success and error cases in the response.
| export const CONTENT_POST_CREATE_FORM_VALIDATION = z.object({ | ||
| content: z.string(), | ||
| images: z.array(z.instanceof(File)), | ||
| } satisfies Record<keyof ContentPostCreateForm, unknown>) |
There was a problem hiding this comment.
Validation schema doesn't match type definition.
The validation schema only allows File[] while the type allows string[] | File[] | Blob[]. This mismatch could cause runtime errors.
export const CONTENT_POST_CREATE_FORM_VALIDATION = z.object({
content: z.string(),
- images: z.array(z.instanceof(File)),
+ images: z.array(
+ z.union([z.string(), z.instanceof(File), z.instanceof(Blob)])
+ ).optional(),
} satisfies Record<keyof ContentPostCreateForm, unknown>)📝 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 CONTENT_POST_CREATE_FORM_VALIDATION = z.object({ | |
| content: z.string(), | |
| images: z.array(z.instanceof(File)), | |
| } satisfies Record<keyof ContentPostCreateForm, unknown>) | |
| export const CONTENT_POST_CREATE_FORM_VALIDATION = z.object({ | |
| content: z.string(), | |
| images: z.array( | |
| z.union([z.string(), z.instanceof(File), z.instanceof(Blob)]) | |
| ).optional(), | |
| } satisfies Record<keyof ContentPostCreateForm, unknown>) |
| export const RootContainer = styled(Box)(() => ({ | ||
| dispaly: 'flex', | ||
| width: '600px', | ||
| alignSelf: 'center', | ||
| flexDirection: 'column', | ||
| })) |
There was a problem hiding this comment.
Fix typo in display property.
The display property is misspelled as dispaly which will cause the flex layout to fail.
export const RootContainer = styled(Box)(() => ({
- dispaly: 'flex',
+ display: 'flex',
width: '600px',
alignSelf: 'center',
flexDirection: 'column',
}))📝 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 RootContainer = styled(Box)(() => ({ | |
| dispaly: 'flex', | |
| width: '600px', | |
| alignSelf: 'center', | |
| flexDirection: 'column', | |
| })) | |
| export const RootContainer = styled(Box)(() => ({ | |
| display: 'flex', | |
| width: '600px', | |
| alignSelf: 'center', | |
| flexDirection: 'column', | |
| })) |
| export const ButtonContainer = styled(Box)(() => ({ | ||
| display: 'flex', | ||
| width: 'fit-content', | ||
| flexDirection: 'row', | ||
| justifyContent: 'space-between', | ||
| gap: '10px', | ||
| })) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant justifyContent property.
The justifyContent: 'space-between' is unnecessary when using gap for spacing between buttons. This combination could lead to unexpected spacing behavior.
export const ButtonContainer = styled(Box)(() => ({
display: 'flex',
width: 'fit-content',
flexDirection: 'row',
- justifyContent: 'space-between',
gap: '10px',
}))📝 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 ButtonContainer = styled(Box)(() => ({ | |
| display: 'flex', | |
| width: 'fit-content', | |
| flexDirection: 'row', | |
| justifyContent: 'space-between', | |
| gap: '10px', | |
| })) | |
| export const ButtonContainer = styled(Box)(() => ({ | |
| display: 'flex', | |
| width: 'fit-content', | |
| flexDirection: 'row', | |
| gap: '10px', | |
| })) |
| const onSubmit = handleSubmit((data: ContentPostCreateForm) => { | ||
| const uploadables: UploadableContentPostFiles = {} | ||
| const imageKeys: string[] = [] | ||
|
|
||
| if (data.images) { | ||
| data.images.forEach((image, index) => { | ||
| uploadables[`image-${index}`] = image as File | ||
| imageKeys.push(`image-${index}`) | ||
| }) | ||
| } | ||
|
|
||
| commitMutation({ | ||
| variables: { | ||
| input: { | ||
| content: data.content, | ||
| images: imageKeys, | ||
| }, | ||
| }, | ||
| uploadables, | ||
| onCompleted(response) { | ||
| const errors = response.contentPostCreate?.errors | ||
| if (errors) { | ||
| sendToast('Something went wrong', { type: 'error' }) | ||
| setFormRelayErrors(formReturn, errors) | ||
| } else { | ||
| reset({ content: '' }) | ||
| sendToast('Post Created Successfully', { type: 'success' }) | ||
| router.push(`/content-post/${response.contentPostCreate?.contentPost?.node?.id}`) | ||
| } | ||
| }, | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Add error handling for mutation failures
The mutation error handling only covers validation errors but doesn't handle network or other GraphQL errors.
commitMutation({
variables: {
input: {
content: data.content,
images: imageKeys,
},
},
uploadables,
onCompleted(response) {
const errors = response.contentPostCreate?.errors
if (errors) {
sendToast('Something went wrong', { type: 'error' })
setFormRelayErrors(formReturn, errors)
} else {
reset({ content: '' })
sendToast('Post Created Successfully', { type: 'success' })
router.push(`/content-post/${response.contentPostCreate?.contentPost?.node?.id}`)
}
},
+ onError(error) {
+ sendToast('Failed to create post. Please try again.', { type: 'error' })
+ console.error('Post creation failed:', error)
+ },
})📝 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 onSubmit = handleSubmit((data: ContentPostCreateForm) => { | |
| const uploadables: UploadableContentPostFiles = {} | |
| const imageKeys: string[] = [] | |
| if (data.images) { | |
| data.images.forEach((image, index) => { | |
| uploadables[`image-${index}`] = image as File | |
| imageKeys.push(`image-${index}`) | |
| }) | |
| } | |
| commitMutation({ | |
| variables: { | |
| input: { | |
| content: data.content, | |
| images: imageKeys, | |
| }, | |
| }, | |
| uploadables, | |
| onCompleted(response) { | |
| const errors = response.contentPostCreate?.errors | |
| if (errors) { | |
| sendToast('Something went wrong', { type: 'error' }) | |
| setFormRelayErrors(formReturn, errors) | |
| } else { | |
| reset({ content: '' }) | |
| sendToast('Post Created Successfully', { type: 'success' }) | |
| router.push(`/content-post/${response.contentPostCreate?.contentPost?.node?.id}`) | |
| } | |
| }, | |
| }) | |
| }) | |
| const onSubmit = handleSubmit((data: ContentPostCreateForm) => { | |
| const uploadables: UploadableContentPostFiles = {} | |
| const imageKeys: string[] = [] | |
| if (data.images) { | |
| data.images.forEach((image, index) => { | |
| uploadables[`image-${index}`] = image as File | |
| imageKeys.push(`image-${index}`) | |
| }) | |
| } | |
| commitMutation({ | |
| variables: { | |
| input: { | |
| content: data.content, | |
| images: imageKeys, | |
| }, | |
| }, | |
| uploadables, | |
| onCompleted(response) { | |
| const errors = response.contentPostCreate?.errors | |
| if (errors) { | |
| sendToast('Something went wrong', { type: 'error' }) | |
| setFormRelayErrors(formReturn, errors) | |
| } else { | |
| reset({ content: '' }) | |
| sendToast('Post Created Successfully', { type: 'success' }) | |
| router.push(`/content-post/${response.contentPostCreate?.contentPost?.node?.id}`) | |
| } | |
| }, | |
| onError(error) { | |
| sendToast('Failed to create post. Please try again.', { type: 'error' }) | |
| console.error('Post creation failed:', error) | |
| }, | |
| }) | |
| }) |
| <Typography variant="caption" sx={{ color: 'text.secondary' }}> | ||
| Max. File Size: 15MB | ||
| </Typography> |
There was a problem hiding this comment.
Fix inconsistent max file size message
The UI shows 15MB limit but the code enforces 10MB limit.
<Typography variant="caption" sx={{ color: 'text.secondary' }}>
- Max. File Size: 15MB
+ Max. File Size: 10MB
</Typography>📝 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.
| <Typography variant="caption" sx={{ color: 'text.secondary' }}> | |
| Max. File Size: 15MB | |
| </Typography> | |
| <Typography variant="caption" sx={{ color: 'text.secondary' }}> | |
| Max. File Size: 10MB | |
| </Typography> |
| <Box width="100%" position="relative" height="500px" mb="24px"> | ||
| <Image | ||
| src={URL.createObjectURL(selectedUploadedFile)} | ||
| alt={selectedUploadedFile.name} | ||
| fill | ||
| style={{ objectFit: 'cover', borderRadius: '8px', height: '100%', width: '100%' }} | ||
| onLoad={() => URL.revokeObjectURL(URL.createObjectURL(selectedUploadedFile))} | ||
| /> | ||
| </Box> |
There was a problem hiding this comment.
Potential memory leak in Image component
The onLoad callback creates and revokes URLs, but if the component unmounts before the image loads, the URL might not be revoked.
+ useEffect(() => {
+ if (selectedUploadedFile) {
+ const objectUrl = URL.createObjectURL(selectedUploadedFile)
+ return () => URL.revokeObjectURL(objectUrl)
+ }
+ }, [selectedUploadedFile])
+
return (
<Box width="100%" position="relative" height="500px" mb="24px">
<Image
- src={URL.createObjectURL(selectedUploadedFile)}
+ src={objectUrl}
alt={selectedUploadedFile.name}
fill
style={{ objectFit: 'cover', borderRadius: '8px', height: '100%', width: '100%' }}
- onLoad={() => URL.revokeObjectURL(URL.createObjectURL(selectedUploadedFile))}
/>
</Box>
)Committable suggestion skipped: line range outside the PR's diff.
| if (files) { | ||
| for (let i = 0; i < files?.length; i += 1) { | ||
| if ( | ||
| files![i] && | ||
| DEFAULT_IMAGE_MAX_SIZE && | ||
| files![i].size > DEFAULT_IMAGE_MAX_SIZE | ||
| ) { | ||
| sendToast( | ||
| `This file is too large (max ${DEFAULT_IMAGE_MAX_SIZE / 1024 / 1024}MB).`, | ||
| { | ||
| type: 'error', | ||
| }, | ||
| ) | ||
| } else { | ||
| field.onChange([...formFiles, ...files]) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize file size validation
The current implementation might process files even after finding an oversized file. Consider breaking early and handling multiple files more efficiently.
if (files) {
+ const oversizedFiles = Array.from(files).filter(file => file.size > DEFAULT_IMAGE_MAX_SIZE)
+ if (oversizedFiles.length > 0) {
+ sendToast(
+ `${oversizedFiles.length} file(s) exceed the size limit (max ${DEFAULT_IMAGE_MAX_SIZE / 1024 / 1024}MB).`,
+ { type: 'error' }
+ )
+ return
+ }
- for (let i = 0; i < files?.length; i += 1) {
- if (
- files![i] &&
- DEFAULT_IMAGE_MAX_SIZE &&
- files![i].size > DEFAULT_IMAGE_MAX_SIZE
- ) {
- sendToast(
- `This file is too large (max ${DEFAULT_IMAGE_MAX_SIZE / 1024 / 1024}MB).`,
- {
- type: 'error',
- },
- )
- } else {
- field.onChange([...formFiles, ...files])
- }
- }
+ field.onChange([...formFiles, ...files])
}
rodrigonahid
force-pushed
the
content-feed-new-post
branch
from
957515f to
ae03fd9
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
packages/components/modules/content-feed/web/NewContentPost/types.ts (1)
1-4: Consider runtime type validation for images array.While the type definition is correct, consider adding runtime validation for the
imagesarray since it accepts multiple types (string[] | File[] | Blob[]). This can help prevent type-related issues during runtime.Consider using zod for runtime validation:
const imageValidator = z.array( z.union([z.string(), z.instanceof(File), z.instanceof(Blob)]) )packages/components/index.ts (1)
1-7: Consider explicit exports for better dependency tracking.While wildcard exports are convenient, they can make it harder to track dependencies and could lead to unintended exports. Consider using explicit exports to make dependencies more visible.
Example:
export { ContentFeed, ContentFeedImage, NewContentPost } from './modules/content-feed/web'packages/components/modules/content-feed/web/ContentFeed/styled.tsx (2)
3-8: Consider responsive width for better mobile support.Consider using a responsive width instead of a fixed 600px to better support different screen sizes.
Apply this diff to make the width responsive:
export const RootContainer = styled(Box)(() => ({ display: 'flex', - width: '600px', + width: '100%', + maxWidth: '600px', alignSelf: 'center', flexDirection: 'column', }))
18-24: Remove redundant justifyContent with gap.The
ButtonContaineruses bothgapandjustifyContent: 'space-between'which might conflict. Since you're usinggapfor spacing,space-betweenis unnecessary.Apply this diff to remove the redundant property:
export const ButtonContainer = styled(Box)(() => ({ display: 'flex', width: 'fit-content', flexDirection: 'row', - justifyContent: 'space-between', gap: '10px', }))packages/components/modules/content-feed/web/ContentFeed/index.tsx (1)
10-35: Consider TypeScript interface and accessibility improvements.The component looks good but could benefit from:
- Adding a TypeScript interface for future props
- Improving accessibility with ARIA labels
Apply these improvements:
+interface ContentFeedProps { + // Add future props here +} -const ContentFeed = () => { +const ContentFeed: React.FC<ContentFeedProps> = () => { const router = useRouter() const onNewPost = useCallback(() => { router.push('/new-post') }, [router]) return ( <RootContainer> <HeaderContainer> - <Typography component="h4" variant="h4"> + <Typography component="h1" variant="h4"> Content Feed </Typography> <Button variant="outlined" color="inherit" onClick={onNewPost} disableRipple sx={{ maxWidth: 'fit-content' }} + aria-label="Create new post" > New Post </Button> </HeaderContainer> </RootContainer> ) }packages/components/modules/content-feed/web/ContentFeedImage/styled.tsx (1)
8-21: Add cross-browser support for custom scrollbar styling.The scrollbar styling only targets WebKit browsers. Consider adding Firefox (scrollbar-*) and standard scrollbar styling for better cross-browser compatibility.
'::-webkit-scrollbar': { height: '6px', }, '::-webkit-scrollbar-track': { boxShadow: `inset 0 0 1px ${theme.palette.grey[400]}`, borderRadius: '10px', }, '::-webkit-scrollbar-thumb': { background: theme.palette.grey[400], borderRadius: '10px', }, '::-webkit-scrollbar-thumb:hover': { background: theme.palette.grey[600], }, +scrollbarWidth: 'thin', +scrollbarColor: `${theme.palette.grey[400]} ${theme.palette.grey[200]}`,packages/components/modules/content-feed/web/NewContentPost/index.tsx (1)
89-97: Prevent layout shift during loading state.The LoadingButton might cause layout shift when transitioning to loading state. Consider setting a minimum width.
<LoadingButton color="inherit" type="submit" loading={isMutationInFlight} disabled={!isDirty || !isValid || isMutationInFlight} - sx={{ maxWidth: 'fit-content', justifySelf: 'end' }} + sx={{ maxWidth: 'fit-content', justifySelf: 'end', minWidth: '100px' }} > Publish </LoadingButton>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/components/index.ts(1 hunks)packages/components/modules/content-feed/common/graphql/mutations/ContentPostCreate.ts(1 hunks)packages/components/modules/content-feed/common/index.ts(1 hunks)packages/components/modules/content-feed/web/ContentFeed/index.tsx(1 hunks)packages/components/modules/content-feed/web/ContentFeed/styled.tsx(1 hunks)packages/components/modules/content-feed/web/ContentFeedImage/index.tsx(1 hunks)packages/components/modules/content-feed/web/ContentFeedImage/styled.tsx(1 hunks)packages/components/modules/content-feed/web/NewContentPost/constants.ts(1 hunks)packages/components/modules/content-feed/web/NewContentPost/index.tsx(1 hunks)packages/components/modules/content-feed/web/NewContentPost/styled.tsx(1 hunks)packages/components/modules/content-feed/web/NewContentPost/types.ts(1 hunks)packages/components/modules/content-feed/web/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/modules/content-feed/common/index.ts
🔇 Additional comments (5)
packages/components/modules/content-feed/web/NewContentPost/types.ts (1)
6-8: LGTM! Good use of template literal type.The template literal type for dynamic image keys is a good use of TypeScript's type system features.
packages/components/modules/content-feed/web/index.ts (1)
1-6: LGTM! Well-organized exports.The exports are well-organized, with components exported individually and types re-exported separately. This provides good flexibility for importing specific components or all types as needed.
packages/components/modules/content-feed/web/NewContentPost/constants.ts (1)
5-8: LGTM! Good use of satisfies operator.The use of the
satisfiesoperator ensures type safety while maintaining type inference. This is a good TypeScript practice.packages/components/modules/content-feed/common/graphql/mutations/ContentPostCreate.ts (2)
5-23: Well-structured GraphQL mutation with proper error handling.The mutation is well-designed with:
- Proper error handling structure
- Clear field selection
- Good type safety
25-26: Clean hook implementation using react-relay.The hook implementation is clean and properly typed.
| export const CONTENT_POST_CREATE_FORM_VALIDATION = z.object({ | ||
| content: z.string(), | ||
| images: z.array(z.instanceof(File)), | ||
| } satisfies Record<keyof ContentPostCreateForm, unknown>) |
There was a problem hiding this comment.
Align validation schema with type definition.
The validation schema for images only accepts File instances, but the ContentPostCreateForm type allows string[] and Blob[] as well. This mismatch could cause runtime validation errors.
Update the validation schema to match the type:
export const CONTENT_POST_CREATE_FORM_VALIDATION = z.object({
content: z.string(),
images: z.array(
z.union([z.string(), z.instanceof(File), z.instanceof(Blob)])
),
} satisfies Record<keyof ContentPostCreateForm, unknown>)| export const RootContainer = styled(Box)(() => ({ | ||
| dispaly: 'flex', | ||
| width: '600px', | ||
| alignSelf: 'center', | ||
| flexDirection: 'column', | ||
| })) |
There was a problem hiding this comment.
Fix typo in display property and consider responsive width.
- The
displayproperty has a typo ('dispaly'). - Consider using a responsive width instead of a fixed 600px to better support different screen sizes.
Apply this diff to fix the typo and make the width responsive:
export const RootContainer = styled(Box)(() => ({
- dispaly: 'flex',
+ display: 'flex',
- width: '600px',
+ width: '100%',
+ maxWidth: '600px',
alignSelf: 'center',
flexDirection: 'column',
}))📝 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 RootContainer = styled(Box)(() => ({ | |
| dispaly: 'flex', | |
| width: '600px', | |
| alignSelf: 'center', | |
| flexDirection: 'column', | |
| })) | |
| export const RootContainer = styled(Box)(() => ({ | |
| display: 'flex', | |
| width: '100%', | |
| maxWidth: '600px', | |
| alignSelf: 'center', | |
| flexDirection: 'column', | |
| })) |
packages/components/modules/content-feed/web/ContentFeedImage/styled.tsx
Show resolved
Hide resolved
| const DEFAULT_IMAGE_FORMATS = 'image/png, image/gif, image/jpeg' | ||
| const DEFAULT_IMAGE_MAX_SIZE = 10 * 1024 * 1024 |
There was a problem hiding this comment.
Fix inconsistency in max file size.
The displayed max file size (15MB) doesn't match the actual validation limit (10MB).
- const DEFAULT_IMAGE_MAX_SIZE = 10 * 1024 * 1024
+ const DEFAULT_IMAGE_MAX_SIZE = 15 * 1024 * 1024Also applies to: 110-112
rodrigonahid
force-pushed
the
content-feed-new-post
branch
from
ae03fd9 to
6c551c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/components/modules/content-feed/web/NewContentPost/index.tsx (1)
52-72:⚠️ Potential issueAdd error handling for network failures.
The mutation error handling only covers API response errors but doesn't handle network failures or unexpected errors.
commitMutation({ variables: { input: { content: data.content, images: imageKeys, }, }, uploadables, + onError: (error) => { + sendToast('Failed to create post. Please try again.', { type: 'error' }); + }, onCompleted(response) { const errors = response.contentPostCreate?.errors if (errors) { sendToast('Something went wrong', { type: 'error' }) setFormRelayErrors(formReturn, errors) } else { reset({ content: '' }) sendToast('Post Created Successfully', { type: 'success' }) router.push(`/content-post/${response.contentPostCreate?.contentPost?.node?.id}`) } }, })
🧹 Nitpick comments (3)
packages/components/modules/content-feed/web/NewContentPost/index.tsx (2)
93-93: Consider adding visual feedback when images are attached.The current disabled state is based on content validity, but users might not understand why the button is disabled if they've only attached images without text.
disabled={!isDirty || !isValid || isMutationInFlight}Consider enhancing the UI to provide clearer feedback about form validation state.
102-110: Improve accessibility by adding aria attributes.The text input could benefit from ARIA attributes to improve accessibility for screen reader users.
<TextField name="content" type="text" placeholder="What is on your mind?" multiline rows={4} control={control} + aria-label="Post content" + aria-required="true" />packages/components/schema.graphql (1)
482-490: Consider implementing additional interfaces for ContentPost.The ContentPost type currently doesn't implement interfaces like CommentsInterface or ReactionsInterface, which would enable commenting and reactions on posts. Consider adding these interface implementations to provide consistent functionality across content types.
-type ContentPost implements Node { +type ContentPost implements Node & CommentsInterface & ReactionsInterface { author: User content: String! """The ID of the object""" id: ID! pk: Int! contentImages: [ContentPostImage] + commentsCount: CommentsCount! + comments(offset: Int, before: String, after: String, first: Int, last: Int, q: String, orderBy: String): CommentConnection + isCommentsEnabled: Boolean! + reactionsCount: ReactionsCount + reactions(offset: Int, before: String, after: String, first: Int, last: Int, id: ID): ReactionConnection + isReactionsEnabled: Boolean! + myReaction(profileId: ID): Reaction }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/components/index.ts(1 hunks)packages/components/modules/content-feed/common/graphql/mutations/ContentPostCreate.ts(1 hunks)packages/components/modules/content-feed/common/index.ts(1 hunks)packages/components/modules/content-feed/web/ContentFeed/index.tsx(1 hunks)packages/components/modules/content-feed/web/ContentFeed/styled.tsx(1 hunks)packages/components/modules/content-feed/web/ContentFeedImage/index.tsx(1 hunks)packages/components/modules/content-feed/web/ContentFeedImage/styled.tsx(1 hunks)packages/components/modules/content-feed/web/NewContentPost/constants.ts(1 hunks)packages/components/modules/content-feed/web/NewContentPost/index.tsx(1 hunks)packages/components/modules/content-feed/web/NewContentPost/styled.tsx(1 hunks)packages/components/modules/content-feed/web/NewContentPost/types.ts(1 hunks)packages/components/modules/content-feed/web/index.ts(1 hunks)packages/components/schema.graphql(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/components/modules/content-feed/web/ContentFeed/index.tsx
- packages/components/modules/content-feed/web/ContentFeed/styled.tsx
- packages/components/modules/content-feed/common/index.ts
- packages/components/modules/content-feed/web/index.ts
- packages/components/modules/content-feed/common/graphql/mutations/ContentPostCreate.ts
- packages/components/index.ts
- packages/components/modules/content-feed/web/NewContentPost/styled.tsx
- packages/components/modules/content-feed/web/NewContentPost/types.ts
- packages/components/modules/content-feed/web/NewContentPost/constants.ts
- packages/components/modules/content-feed/web/ContentFeedImage/styled.tsx
- packages/components/modules/content-feed/web/ContentFeedImage/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (1)
packages/components/modules/content-feed/web/NewContentPost/index.tsx (1)
74-76: Add confirmation dialog for unsaved changes.Users might accidentally lose their work when clicking cancel. Consider adding a confirmation dialog when there are unsaved changes.
const onCancel = useCallback(() => { + if (isDirty) { + const confirmed = window.confirm('You have unsaved changes. Are you sure you want to leave?'); + if (!confirmed) return; + } router.push('/feed') }, [router])
| type ContentPost implements Node { | ||
| author: User | ||
| content: String! | ||
| """The ID of the object""" | ||
| id: ID! | ||
| pk: Int! | ||
| contentImages: [ContentPostImage] | ||
| } | ||
| type ContentPostConnection { | ||
| """Pagination data for this connection.""" | ||
| pageInfo: PageInfo! | ||
| """Contains the nodes in this connection.""" | ||
| edges: [ContentPostEdge]! | ||
| totalCount: Int | ||
| edgeCount: Int | ||
| } | ||
| input ContentPostCreateInput { | ||
| content: String! | ||
| images: [String] | ||
| clientMutationId: String | ||
| } | ||
| type ContentPostCreatePayload { | ||
| """May contain more than one error for same field.""" | ||
| errors: [ErrorType] | ||
| _debug: DjangoDebug | ||
| contentPost: ContentPostEdge | ||
| clientMutationId: String | ||
| } | ||
| """A Relay edge containing a `ContentPost` and its cursor.""" | ||
| type ContentPostEdge { | ||
| """The item at the end of the edge""" | ||
| node: ContentPost | ||
| """A cursor for use in pagination""" | ||
| cursor: String! | ||
| } | ||
| type ContentPostImage implements Node { | ||
| image: String | ||
| post: ContentPost! | ||
| """The ID of the object""" | ||
| id: ID! | ||
| pk: Int! | ||
| } |
There was a problem hiding this comment.
Remove duplicate type definitions.
The ContentPost related types are defined twice in the schema - once at lines 482-532 and again at lines 1891-1932. This duplication should be removed to prevent confusion and potential conflicts.
Remove these duplicate type definitions at lines 1891-1932 as they're identical to the ones already defined earlier in the file.
| contentPostCreate(input: ContentPostCreateInput!): ContentPostCreatePayload | ||
| contentPosts(offset: Int, before: String, after: String, first: Int, last: Int): ContentPostConnection | ||
| commentCreate(input: CommentCreateInput!): CommentCreatePayload |
There was a problem hiding this comment.
Separate query and mutation operations.
Line 781 includes contentPosts as a field in the Mutation type, but this appears to be a query operation. Queries should be defined in the Query type, not the Mutation type.
type Mutation {
// other mutations...
contentPostCreate(input: ContentPostCreateInput!): ContentPostCreatePayload
- contentPosts(offset: Int, before: String, after: String, first: Int, last: Int): ContentPostConnection
// other mutations...
}The contentPosts query is already correctly defined in the Query type at line 1453.
📝 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.
| contentPostCreate(input: ContentPostCreateInput!): ContentPostCreatePayload | |
| contentPosts(offset: Int, before: String, after: String, first: Int, last: Int): ContentPostConnection | |
| commentCreate(input: CommentCreateInput!): CommentCreatePayload | |
| type Mutation { | |
| // other mutations... | |
| contentPostCreate(input: ContentPostCreateInput!): ContentPostCreatePayload | |
| commentCreate(input: CommentCreateInput!): CommentCreatePayload | |
| // other mutations... | |
| } |
| content: String! | ||
| images: [String] | ||
| clientMutationId: String | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider validating content field at the schema level.
The ContentPostCreateInput doesn't specify any validation constraints on the content field. Consider adding directives to ensure content meets minimum quality requirements.
🌐 Web query:
Does GraphQL support field validation directives like minLength or pattern?
💡 Result:
Yes, GraphQL supports field validation directives like minLength or pattern through custom schema directives and third-party libraries, though these are not part of the core GraphQL specification. Here's how it works:
Built-in vs. Custom Directives
- Core GraphQL provides basic type validation (e.g.,
String,Int) but lacks advanced constraints likeminLength[7][9]. - Custom Directives extend this functionality:
- Libraries like
graphql-constraint-directiveadd validation rules via@constraint[3][4][5]:input ReviewInput { email: String! @constraint(format: "email", maxLength: 255) password: String! @constraint(minLength: 8) }
- Apollo Server uses packages like
apollo-validation-directivesfor validators such as@stringLengthand@listLength[1].
- Libraries like
Common Validation Patterns
| Directive | Use Case | Example |
|---|---|---|
@constraint |
Multi-purpose validation | @constraint(min: 5, pattern: "^\\d+$")[4][6] |
@stringLength |
String length bounds | @stringLength(min: 3, max: 50)[1] |
@listLength |
Array size limits | @listLength(max: 10)[1] |
@pattern |
Regex matching | @pattern(regexp: "^[A-Za-z]+$")[1][4] |
Implementation Methods
- Schema-First:
const { constraintDirective } = require('graphql-constraint-directive'); const schema = makeExecutableSchema({ typeDefs, schemaDirectives: { constraint: constraintDirective } });
- Code-First (Apollo/GraphQL .NET):
Field<StringGraphType>("username") .ApplyDirective("length", "min", 5, "max", 20)[2][6];
These solutions inject validation errors into resolver contexts while keeping schema documentation self-contained[1][4]. Popular implementations include:
- Apollo's validation directives[1]
.NET's LengthDirective[2]- Java's SmallRye mapping Bean Validation to constraints[6]
For production use, always combine directive-based validation with additional server-side checks for security[3][5].
Citations:
- 1: https://github.com/profusion/apollo-validation-directives
- 2: https://graphql-dotnet.github.io/docs/getting-started/directives/
- 3: https://escape.tech/blog/graphql-input-validation-and-sanitization/
- 4: https://github.com/confuser/graphql-constraint-directive/blob/master/README.md
- 5: https://www.apollographql.com/blog/graphql-validation-using-directives
- 6: https://smallrye.io/smallrye-graphql/latest/directives/
- 7: https://graphql.org/learn/validation/
- 8: https://github.com/IvanGoncharov/graphql-constraints-spec/blob/master/README.md
- 9: https://tailcall.run/graphql/graphql-directives/
- 10: https://boopathi.blog/graphql-directives-an-introduction
Action Required: Implement Schema-Level Validation for the "content" Field
The current schema for the ContentPostCreateInput defines the "content" field simply as a String without any validation constraints. Although GraphQL’s core specification doesn’t include advanced validation like minLength or pattern, you can enforce these rules using custom directives (e.g., via libraries such as graphql-constraint-directive or similar tools).
- Suggestion: Enhance the "content" field with a custom directive (for example,
@constraint(minLength: 10)) to enforce minimum quality requirements. - Action: Integrate the appropriate validation library and update the schema accordingly to provide early input validation at the schema level.
rodrigonahid
force-pushed
the
content-feed-new-post
branch
from
6c551c8 to
f1707b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/modules/content-feed/web/ContentFeedImage/styled.tsx (1)
42-54: Use theme colors instead of hardcoded color values for consistency.The MiniatureFileWrapper component uses a hardcoded "black" color for its hover state border, which could lead to inconsistent styling when the theme changes or in different color modes (like dark mode).
export const MiniatureFileWrapper = styled(Box)(({ theme }) => ({ position: 'relative', flexShrink: 0, width: '80px', height: '80px', border: `2px solid ${theme.palette.grey[200]}`, borderRadius: '12px', padding: '4px', display: 'inline-block', '&:hover': { - border: `2px solid black`, + border: `2px solid ${theme.palette.grey[900]}`, }, }))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/components/index.ts(1 hunks)packages/components/modules/content-feed/common/graphql/mutations/ContentPostCreate.ts(1 hunks)packages/components/modules/content-feed/common/index.ts(1 hunks)packages/components/modules/content-feed/web/ContentFeed/index.tsx(1 hunks)packages/components/modules/content-feed/web/ContentFeed/styled.tsx(1 hunks)packages/components/modules/content-feed/web/ContentFeedImage/index.tsx(1 hunks)packages/components/modules/content-feed/web/ContentFeedImage/styled.tsx(1 hunks)packages/components/modules/content-feed/web/NewContentPost/constants.ts(1 hunks)packages/components/modules/content-feed/web/NewContentPost/index.tsx(1 hunks)packages/components/modules/content-feed/web/NewContentPost/styled.tsx(1 hunks)packages/components/modules/content-feed/web/NewContentPost/types.ts(1 hunks)packages/components/modules/content-feed/web/index.ts(1 hunks)packages/components/schema.graphql(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/components/modules/content-feed/common/index.ts
- packages/components/modules/content-feed/web/ContentFeed/index.tsx
- packages/components/modules/content-feed/web/index.ts
- packages/components/modules/content-feed/web/NewContentPost/types.ts
- packages/components/modules/content-feed/web/ContentFeed/styled.tsx
- packages/components/index.ts
- packages/components/modules/content-feed/web/NewContentPost/styled.tsx
- packages/components/modules/content-feed/common/graphql/mutations/ContentPostCreate.ts
- packages/components/modules/content-feed/web/ContentFeedImage/index.tsx
- packages/components/modules/content-feed/web/NewContentPost/constants.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (9)
packages/components/modules/content-feed/web/ContentFeedImage/styled.tsx (4)
56-70: Enhance button accessibility with proper focus states.The RemoveFileButton lacks proper focus state styling for keyboard navigation. Also, the hover state uses the same color as the default state, providing no visual feedback on interaction.
export const RemoveFileButton = styled('button')(({ theme }) => ({ position: 'absolute', top: '4px', right: '4px', width: '28px', height: '28px', display: 'flex', alignItems: 'center', justifyContent: 'center', borderRadius: '0 6px 0 6px', backgroundColor: theme.palette.grey[800], + '&:focus-visible': { + outline: `2px solid ${theme.palette.primary.main}`, + outlineOffset: '2px', + }, '&:hover': { - backgroundColor: theme.palette.grey[800], + backgroundColor: theme.palette.grey[900], }, }))
72-83: LGTM! Good implementation of the drop zone container.The DropFilesContainer component is well-structured with proper spacing, theming, and layout properties. The dashed border and centered content create a clear visual indication of the drop zone area.
3-22: Good implementation of scrollable container with custom scrollbar styling.The ContentFeedImageContainer has well-defined scrollbar customization that improves user experience while maintaining theme consistency.
24-40: Good implementation of file upload button components.Both the AddFileButton and AddFileWrapper components are well-designed with proper theming, dimensions, and hover states.
packages/components/modules/content-feed/web/NewContentPost/index.tsx (2)
52-71: Add error handling for network failures.This mutation call doesn't include an
onErrorcallback to handle scenarios such as network failures, timeouts, or unexpected exceptions. This is a repeat of a previous review comment; adding anonErrorcallback would help display a user-friendly message or perform alternate error-handling logic.
74-76: Prompt user for confirmation before discarding unsaved changes.If the form is dirty and the user clicks "Cancel," they could lose their input without warning. A confirmation prompt (e.g., with
window.confirm) would help avoid accidental data loss. This is a repeat of a previous review comment.packages/components/schema.graphql (3)
502-506: Enforce schema-level validation on the “content” field.The
contentfield has no minimum length or pattern constraints. Consider adding a custom directive or adopting a library likegraphql-constraint-directiveto validate essential constraints (e.g. minimum length, restricted characters) at the schema level. This is a repeat of a previous review comment.
780-781: Separate query from mutation.
contentPostsin theMutationtype appears more like a query operation and might already exist in theQuerytype at line 1453. Consider removing or relocating it to theQuerytype. This is a repeat of a previous review comment.
1891-1932: Remove duplicate type definitions forContentPostand related entities.These lines repeat the same definitions for
ContentPost,ContentPostConnection, andContentPostImagethat were previously defined at lines 482-532. Keeping duplicate schema definitions can cause confusion and potential conflicts. Remove or consolidate them into a single definition. This is a repeat of a previous review comment.
| @@ -0,0 +1,7 @@ | |||
| export * from './modules/__shared__' | |||
| export * from './modules/comments' | |||
There was a problem hiding this comment.
I think this file can be removed.
There was a problem hiding this comment.
yes @rodrigonahid please remove it
| RemoveFileButton, | ||
| } from './styled' | ||
|
|
||
| interface IContentFeedImageProps { |
There was a problem hiding this comment.
Lets move this to a types file and then add to the index file.
Also, this types seems a little bit weird. Why is images type equals to never[]?
There was a problem hiding this comment.
let's also avoid I prefix on type interfaces
|
| @@ -0,0 +1,7 @@ | |||
| export * from './modules/__shared__' | |||
| export * from './modules/comments' | |||
There was a problem hiding this comment.
yes @rodrigonahid please remove it
| const router = useRouter() | ||
|
|
||
| const onNewPost = useCallback(() => { | ||
| router.push('/new-post') |
There was a problem hiding this comment.
can we make this route configurable? just in case clients need this in different routes. We can keep using '/new-post' as default
| RemoveFileButton, | ||
| } from './styled' | ||
|
|
||
| interface IContentFeedImageProps { |
There was a problem hiding this comment.
let's also avoid I prefix on type interfaces
| form.setValue('images', updatedFiles as never, { shouldValidate: true }) | ||
| } | ||
|
|
||
| const handleDragEnter = (e: React.DragEvent) => { |
There was a problem hiding this comment.
lets import DragEvent directly, right now it's undefined
| </Box> | ||
| )} | ||
|
|
||
| {(isDragging || !formFiles?.length) && ( |
There was a problem hiding this comment.
I wonder if we could've reused the design-system Dropzone component
__package_name__package update -v __package_version__Summary by CodeRabbit
ContentFeed,ContentFeedImage, andNewContentPosthave been added for improved functionality.