BA-2399 [RN] Comment Creation #246
Conversation
tsl-ps2
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThe pull request introduces a broad set of new exports, UI components, and TypeScript type definitions. In the shared modules, new component exports (such as ReactionButton) and updated import paths have been added. Several new native components and screens—like SubmitActions, SocialInput, SocialInputDrawer, SocialUpsertActions, Timestamp, and an extensive set for comments (including CommentCreate, CommentItem with reaction/reply buttons, Comments, and CommentsList)—are now available, along with corresponding types and styles. In the design system, new icon components and a SocialTextInput facility (with context, hooks, and HOC) have been added. Additionally, a dependency on Zustand was introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SID as SocialInputDrawer
participant BS as BottomSheet
participant Input as SocialInputBox
User->>SID: Tap to open drawer
SID->>BS: Trigger handleSheetChange & animate
BS-->>SID: Send animation state update
SID->>Input: Toggle focus/blur based on animation state
User->>Input: Enter text and submit
sequenceDiagram
participant User
participant CC as CommentCreate
participant Form as React Hook Form
participant Mutation as CommentCreateMutation
User->>CC: Enter comment & press submit
CC->>Form: Validate form inputs
CC->>Mutation: Trigger comment mutation (if not in progress)
Mutation-->>CC: Return success/failure response
CC->>Form: Reset form on success
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/__shared__/native/SocialInputBox/SubmitActions/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/__shared__/native/SocialInputBox/SubmitActions/types.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/__shared__/native/SocialInputBox/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
✨ 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 (
|
tsl-ps2
force-pushed
the
BA-2399-fe-packages-comment-creation
branch
from
7355200 to
8347b0c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (38)
packages/components/modules/notifications/native/NotificationsList/Divider/styles.ts (1)
5-25: The styles implementation is clean but needs a minor adjustment.The styles follow React Native styling patterns well, but there's an issue with the text style. The
alignItems: 'center'property typically works on container components, not directly on text elements.text: { - alignItems: 'center', + textAlign: 'center', },packages/components/modules/notifications/native/NotificationsList/Divider/index.tsx (1)
9-22: Consider enhancing component reusability.The Divider component has a good clean implementation, but it could be more reusable with a few improvements:
- The "Read" text is hardcoded, limiting reuse in other contexts
- The
styles.textdefined in the styles file isn't being usedConsider accepting the text as a prop with "Read" as the default value, and applying the text style:
-const Divider: FC = () => { +interface DividerProps { + text?: string; +} + +const Divider: FC<DividerProps> = ({ text = "Read" }) => { const theme = useTheme() const styles = createStyles(theme) return ( <View style={styles.container}> <View style={styles.line} /> - <View> - <Text variant="caption">Read</Text> + <View style={styles.text}> + <Text variant="caption">{text}</Text> </View> <View style={styles.line} /> </View> ) }packages/components/modules/notifications/native/NotificationsList/index.tsx (2)
55-61: Improve type safety for the notification parameter.The
anytype for the notification parameter should be replaced with a more specific type. Based on the imports, you're likely dealing with a fragment reference type.- const renderNotificationItem = (notification: any, index: number) => { + const renderNotificationItem = (notification: NotificationsListFragment['notifications']['edges'][0]['node'], index: number) => {
63-72: Consider adding a key to the fragment wrapper.While you've correctly added a key to the NotificationItem, React recommends keys for all list items. Since you're now returning a fragment with multiple children, add a key to the outer fragment or container.
- <> + <React.Fragment key={`notification-container-${notification.id}`}> {divider} <NotificationItem - key={`notification-${notification.id}`} notification={notification} refetch={refetchNotifications} {...NotificationItemProps} /> - </> + </React.Fragment>packages/design-system/components/native/inputs/SocialTextInput/context/useSocialTextInput/index.tsx (2)
7-15: Consider optimizing the selector for better performanceThe hook implementation follows good practices for context access and error handling. However, the selector
(state) => statereturns the entire state object, which could cause unnecessary re-renders in components using this hook if the state changes frequently.Consider allowing component-specific selectors:
-const useSocialTextInput = () => { +const useSocialTextInput = <T = any>(selector?: (state: any) => T) => { const store = useContext(SocialTextInputContext) if (!store) { throw new Error('Missing SocialTextInputProvider') } - return useStore(store, (state) => state) + return useStore(store, selector || ((state) => state)) }This would allow components to select only the parts of state they need:
// Example usage const { text } = useSocialTextInput(state => ({ text: state.text }));
1-17: Add JSDoc comments for better documentationAdding JSDoc comments would improve the documentation of this hook.
+/** + * Hook to access the SocialTextInput context + * @throws Error if used outside of SocialTextInputProvider + * @returns The current social text input state + */ const useSocialTextInput = () => { const store = useContext(SocialTextInputContext) if (!store) { throw new Error('Missing SocialTextInputProvider') } return useStore(store, (state) => state) }packages/components/modules/comments/native/CommentItem/CommentReplyButton/types.ts (1)
1-4: Missing VoidFunction type importThe interface references
VoidFunctiontype, but there's no import statement for it. WhileVoidFunctionis a standard TypeScript type (equivalent to() => void), it's better to explicitly define or import it for clarity.+// Either add this import +import { VoidFunction } from 'types' +// Or use the inline type export interface CommentReplyButtonProps { - onReply: VoidFunction + onReply: () => void commentId: string }packages/design-system/components/native/inputs/SocialTextInput/context/withSocialTextInputProvider/index.tsx (1)
5-11: Well-implemented HOC patternThe higher-order component (HOC) pattern is correctly implemented, providing SocialTextInput context to wrapped components while preserving their prop types.
The generic type constraint
<TProps extends {}>is somewhat redundant as{}is already the default constraint for object types in TypeScript. Consider simplifying to just<TProps>.-const withSocialTextInputProvider = - <TProps extends {}>(Component: FC<TProps>) => +const withSocialTextInputProvider = + <TProps>(Component: FC<TProps>) =>packages/components/modules/__shared__/native/Timestamp/index.tsx (1)
11-11: Consider adding date validationThere's no validation for the input date string. If an invalid date is provided, it could cause runtime errors.
- const dateTime = DateTime.fromISO(date) + const dateTime = DateTime.fromISO(date) + if (!dateTime.isValid) { + console.warn(`Invalid date provided to Timestamp component: ${date}`) + return <Text variant="caption" color="low">Invalid date</Text> + }packages/components/modules/comments/native/CommentCreate/types.ts (1)
10-11: Consider following React naming conventions for component propsProps that are React components typically follow a different naming convention than the component itself.
- SocialInputDrawer?: FC<SocialInputDrawerProps> - SocialInputDrawerProps?: Partial<SocialInputDrawerProps> + socialInputDrawerComponent?: FC<SocialInputDrawerProps> + socialInputDrawerProps?: Partial<SocialInputDrawerProps>This makes it clearer which variables are components and which are props, improving code readability.
packages/design-system/components/native/inputs/SocialTextInput/styles.ts (1)
1-41: Well-structured styles for the SocialTextInput component.The styles look comprehensive and follow React Native's styling patterns. Good use of theme colors for dynamic border colors based on focus state.
However, there appears to be redundancy in padding definitions (lines 31-36). Consider simplifying:
contentStyle: { lineHeight: 22, paddingHorizontal: 0, paddingVertical: 0, - paddingLeft: 0, - paddingRight: 0, - paddingTop: 0, - paddingBottom: 0, },Since
paddingHorizontalandpaddingVerticalalready set all sides to 0, the individual padding properties are redundant.packages/components/modules/comments/native/CommentItem/index.tsx (2)
29-32: Remove console.log before production deployment.The placeholder function for replying to comments contains a console.log statement that should be removed before deploying to production.
const replyToComment = () => { // TODO (in another story) - console.log('Reply button clicked') }
18-63: Well-structured CommentItem component with appropriate defaults.The component is well-organized and handles the case when comment data is unavailable. Good use of default props for customizable subcomponents (Timestamp, CommentReplyButton, CommentReactionButton).
Consider adding error handling for the Relay data fetching to improve robustness:
const [comment] = useRefetchableFragment<CommentItemRefetchQuery, CommentItem_comment$key>( CommentItemFragmentQuery, commentRef, ) + // Handle potential errors from data fetching + const handleError = (error) => { + // Appropriate error handling logic + }packages/components/modules/__shared__/native/SocialInputBox/types.ts (1)
17-17: Consider removinganytype for better type safetyThe
formprop usesanyas the second type parameter for UseFormReturn, which reduces type safety. Consider specifying a more precise type or usingunknownif the exact type cannot be determined.- form: UseFormReturn<SocialUpsertForm, any, undefined> + form: UseFormReturn<SocialUpsertForm, unknown, undefined>packages/components/modules/__shared__/native/SocialInputBox/SubmitActions/index.tsx (1)
18-18: Extract inline styles for better maintainabilityThe inline style in TouchableOpacity could be extracted to a separate styles object or file to improve code maintainability and readability.
+ import { StyleSheet } from 'react-native' // Later in the file, outside the component... + const styles = StyleSheet.create({ + submitButton: { + padding: 8 + } + }) // Then in the component... - <TouchableOpacity onPress={handleSubmit} style={{ padding: 8 }}> + <TouchableOpacity onPress={handleSubmit} style={styles.submitButton}>packages/components/modules/comments/native/CommentItem/CommentReactionButton/index.tsx (3)
21-22: Consider adding inline padding to the style sheet for consistencyThe inline style for padding could be moved to the style sheet for better maintainability and consistency.
- <IconButton onPress={handleReaction} style={{ paddingHorizontal: 0, paddingVertical: 5 }}> + <IconButton onPress={handleReaction} style={styles.iconButton}>And in the style sheet:
// Add to styles.ts export const createStyles = () => StyleSheet.create({ reactionContainer: { backgroundColor: 'transparent', display: 'flex', flexDirection: 'row', alignItems: 'center', gap: 4, }, iconButton: { paddingHorizontal: 0, paddingVertical: 5, }, })
22-22: Add accessibility labels to reaction iconsThe favorite icons lack accessibility labels, which makes it difficult for screen readers to convey their purpose.
- {target?.myReaction?.id ? <FavoriteSelectedIcon /> : <FavoriteIcon />} + {target?.myReaction?.id ? + <FavoriteSelectedIcon accessibilityLabel="Remove like" /> : + <FavoriteIcon accessibilityLabel="Like" /> + }
24-26: Handle undefined reaction countThe component doesn't handle the case where
target?.reactionsCount?.totalmight be undefined, which could lead to displaying "undefined" in the UI.- <Text variant="caption" color="low"> - {target?.reactionsCount?.total} - </Text> + <Text variant="caption" color="low"> + {target?.reactionsCount?.total || 0} + </Text>packages/components/modules/comments/native/CommentsList/types.ts (1)
6-11: Improve property naming for better clarityThe
CommentItemPropsproperty name is the same as the imported type, which could lead to confusion. Consider renaming for better clarity.export interface CommentsListProps { target: CommentsList_comments$key subscriptionsEnabled: boolean CommentItem?: FC<CommentItemProps> - CommentItemProps?: Partial<CommentItemProps> + commentItemCustomProps?: Partial<CommentItemProps> }Also, consider adding JSDoc comments to clarify the relationship between
CommentItemand the props customization:/** * Props for CommentsList component * @property {CommentsList_comments$key} target - GraphQL fragment reference for comments * @property {boolean} subscriptionsEnabled - Whether real-time updates are enabled * @property {FC<CommentItemProps>} [CommentItem] - Custom CommentItem component * @property {Partial<CommentItemProps>} [commentItemCustomProps] - Custom props to override defaults for CommentItem */ export interface CommentsListProps { // ... }packages/components/modules/comments/native/CommentItem/CommentReplyButton/index.tsx (2)
15-15: Move inline styles to the style sheetFor better maintainability, move the inline styles to the style sheet.
- <IconButton onPress={onReply} style={{ padding: 0 }}> + <IconButton onPress={onReply} style={styles.iconButton}>And in the style sheet:
// Add to styles.ts export const createStyles = () => StyleSheet.create({ replyContainer: { backgroundColor: 'transparent', display: 'flex', flexDirection: 'row', alignItems: 'center', gap: 4, }, iconButton: { padding: 0, }, })
18-20: Improve accessibility label for the reply textThe current aria-label "replies count {commentId}" might be misleading since the component displays "Reply" and not a count.
- <Text variant="caption" color="low" aria-label={`replies count ${commentId}`}> + <Text variant="caption" color="low" aria-label={`reply to comment ${commentId}`}> Reply </Text>packages/components/modules/__shared__/native/SocialInputBox/index.tsx (1)
12-44: Well-structured component with good compositionThe
SocialInputcomponent is well-structured, usingforwardRefappropriately to forward the ref to the underlying input component. The component composition approach with default components and overridable props is a good pattern for flexibility.A few suggestions:
- Consider adding JSDoc documentation to describe the component's purpose and usage
- You might want to add error handling for the form submission process
+/** + * SocialInput component for comment creation + * + * This component provides a composable social input box with customizable + * text input, action buttons, and submit functionality. + */ const SocialInput = forwardRef<NativeTextInput, SocialInputBoxProps>(packages/design-system/components/native/icons/FavoriteIcon/index.tsx (1)
8-28: Clean SVG icon implementation with proper themingThe
FavoriteIconcomponent correctly uses the theme system to determine colors based on theisActivestate. The SVG implementation follows best practices with proper viewBox and path definitions.For accessibility purposes, consider adding an
accessibilityLabelprop with a default value to ensure screen readers can properly identify this icon.const FavoriteIcon: FC<SvgIconProps> = ({ isActive = false, color, width = '18', height = '18', + accessibilityLabel = 'Favorite button', ...props }) => { const { colors } = useTheme() const defaultColor = color ?? colors.object.low const svgColor = isActive ? colors.primary.high : defaultColor return ( <Svg width={width} height={height} viewBox="0 0 18 18" color={svgColor} fill="none" + accessibilityLabel={accessibilityLabel} {...props}> <Path d="M9.00039 15.5999C7.50039 15.5999 1.65039 12.0749 1.65039 7.34985C1.65039 5.02485 3.45039 2.47485 6.22539 2.47485C7.50039 2.47485 8.40039 2.99985 9.00039 3.52485C9.60039 2.99985 10.5004 2.47485 11.7754 2.47485C14.4754 2.47485 16.3504 5.02485 16.3504 7.34985C16.3504 12.0749 10.5004 15.5999 9.00039 15.5999ZM6.22539 3.59985C4.20039 3.59985 2.77539 5.54985 2.77539 7.34985C2.77539 11.3999 7.95039 14.4749 8.92539 14.4749C9.90039 14.4749 15.0754 11.3999 15.0754 7.34985C15.0754 5.54985 13.7254 3.59985 11.6254 3.59985C10.4254 3.59985 9.67539 4.19985 9.30039 4.64985C9.07539 4.87485 8.62539 4.87485 8.47539 4.64985C8.17539 4.12485 7.42539 3.59985 6.22539 3.59985Z" fill="currentColor" /> </Svg> ) }packages/design-system/components/native/inputs/SocialTextInput/context/SocialTextInputProvider/index.tsx (2)
1-27: Good implementation of context provider with ZustandThe
SocialTextInputProvidercomponent correctly implements a React context provider with Zustand state management. The use ofuseRefto ensure the store is only created once is a good pattern.A few suggestions to improve this implementation:
- Consider adding memoization to prevent unnecessary re-renders when the context value changes
- Add type safety to ensure the context is not used outside of the provider
- Consider adding more detailed error handling or validation
-export const SocialTextInputContext = createContext<StoreApi<UseSocialTextInput> | null>(null) +export const SocialTextInputContext = createContext<StoreApi<UseSocialTextInput> | null>(null) + +// Helper function to ensure the context is used within a provider +export function useSocialTextInputContext() { + const context = useContext(SocialTextInputContext) + if (context === null) { + throw new Error('useSocialTextInputContext must be used within a SocialTextInputProvider') + } + return context +} const SocialTextInputProvider: FC<PropsWithChildren> = ({ children }) => { const storeRef = useRef<StoreApi<UseSocialTextInput>>() if (!storeRef.current) { storeRef.current = create<UseSocialTextInput>((set) => ({ ...INITIAL_SOCIAL_TEXT_INPUT_STATE, setSocialInputState: set, reset: () => set({ ...INITIAL_SOCIAL_TEXT_INPUT_STATE }), })) } + + // Use memoization to prevent unnecessary re-renders + const contextValue = useMemo(() => storeRef.current, []); return ( - <SocialTextInputContext.Provider value={storeRef.current}> + <SocialTextInputContext.Provider value={contextValue}> {children} </SocialTextInputContext.Provider> ) }Don't forget to add the missing imports:
-import { FC, PropsWithChildren, createContext, useRef } from 'react' +import { FC, PropsWithChildren, createContext, useContext, useMemo, useRef } from 'react'
3-3: Consider using a more specific import from ZustandInstead of importing from the root of Zustand, consider using a more specific import path to potentially reduce bundle size.
-import { StoreApi, create } from 'zustand' +import { createStore, type StoreApi } from 'zustand/vanilla'packages/design-system/components/native/icons/FavoriteSelectedIcon/index.tsx (2)
8-28: Consider semantic default color for inactive stateThe component correctly implements the FavoriteSelectedIcon with proper theming support. However, using
colors.error.mainas the default color for an inactive state might not be semantically appropriate for a favorite/like icon. Consider using a more neutral color from the theme (like gray or a secondary color) for the inactive state.- const defaultColor = color ?? colors.error.main + const defaultColor = color ?? colors.text.secondary // or another neutral color
21-26: Ensure accessibility with proper ARIA attributesThe SVG icon is missing accessibility attributes that would improve screen reader support.
<Svg width={width} height={height} viewBox="0 0 18 18" color={svgColor} fill="none" + accessibilityRole="image" + accessibilityLabel="Favorite Selected" {...props} >packages/components/modules/comments/native/CommentsList/index.tsx (2)
23-23: Avoid usinganytype for better type safetyThe
renderCommentItemfunction usesanyfor the comment parameter, which weakens type safety. Consider defining a proper type for the comment object.- const renderCommentItem = (comment: any) => { + const renderCommentItem = (comment: { id: string; [key: string]: any }) => {For even better type safety, create a dedicated Comment type that reflects the actual structure of your comment data.
40-46: Add loading and error states for improved UXThe component doesn't handle loading or error states from the
useCommentListhook. Consider adding loading indicators and error handling for a better user experience.- const { data: target } = useCommentList(targetRef) + const { data: target, loading, error } = useCommentList(targetRef) return ( <> {subscriptionsEnabled && <CommentsSubscription targetObjectId={target.id} />} + {loading && <LoadingIndicator />} + {error && <ErrorMessage error={error} />} + {!loading && !error && ( <View style={styles.listContainer}> { // TODO (another story): paginate properly comments.map((comment) => renderCommentItem(comment)) } </View> + )} </> )packages/components/modules/comments/native/index.ts (1)
1-20: Structured exports with commented placeholdersThe file correctly exports the implemented components while maintaining commented placeholders for future components. This approach clearly indicates what's currently available and what's planned.
For future consideration: Add JSDoc comments above each export group to explain the purpose of each component for better developer experience.
packages/components/modules/comments/native/Comments/index.tsx (1)
33-44: Consider component composition for better separation of concernsWhile the current implementation works, it combines the responsibilities of rendering comments, managing the comment create interface, and conditional rendering. Consider splitting these responsibilities into separate components for better maintainability.
// Example refactoring (conceptual) - return ( - <View style={[styles.rootContainer, styles.transparent]}> - <CommentCreate ref={commentCreateRef} targetObjectId={target.id} {...CommentCreateProps}> - <View style={styles.transparent}>{children}</View> - <CommentsList - target={target} - subscriptionsEnabled={subscriptionsEnabled} - {...CommentsListProps} - /> - </CommentCreate> - </View> - ) + // In a separate ContentWithComments component + const ContentWithComments = ({children, commentsList}) => ( + <View style={styles.transparent}> + {children} + {commentsList} + </View> + ); + + return ( + <View style={[styles.rootContainer, styles.transparent]}> + <CommentCreate ref={commentCreateRef} targetObjectId={target.id} {...CommentCreateProps}> + <ContentWithComments + commentsList={ + <CommentsList + target={target} + subscriptionsEnabled={subscriptionsEnabled} + {...CommentsListProps} + /> + } + > + {children} + </ContentWithComments> + </CommentCreate> + </View> + )packages/components/modules/__shared__/native/SocialInputDrawer/index.tsx (3)
27-29: Remove or replace console logs for production.Console logs should be removed or replaced with a proper logging utility before deploying to production.
const handleSheetChange = useCallback((index: number) => { - console.log('handleSheetChange', index) + // Consider using a proper logging utility here if needed }, [])
50-50: Extract magic numbers to named constants.The snap points are defined as magic numbers. Consider extracting them to named constants for better readability and maintainability.
+const COLLAPSED_HEIGHT = 80 +const EXPANDED_HEIGHT = 200 return ( <BottomSheet ref={bottomSheetRef} index={1} - snapPoints={[80, 200]} + snapPoints={[COLLAPSED_HEIGHT, EXPANDED_HEIGHT]}
33-39: Add error handling for bottomSheet operations.Operations on
bottomSheetRef.currentmight fail if the ref is not properly initialized. Consider adding error handling.if (to === 0) { if (ref && 'current' in ref) ref.current?.blur() - bottomSheetRef.current?.snapToIndex(1) + try { + bottomSheetRef.current?.snapToIndex(1) + } catch (error) { + // Handle or log error appropriately + } } else if (to === 2) { if (ref && 'current' in ref) ref.current?.focus() - bottomSheetRef.current?.snapToIndex(1) + try { + bottomSheetRef.current?.snapToIndex(1) + } catch (error) { + // Handle or log error appropriately + } }packages/components/modules/comments/native/CommentCreate/index.tsx (4)
23-24: Consider using UUID for client mutation IDs.Using a simple counter for client mutation IDs could lead to conflicts in a multi-user environment. Consider using a UUID library for more robust unique identifiers.
+import { v4 as uuidv4 } from 'uuid' -let nextClientMutationId = 0And then in the onSubmit function:
-nextClientMutationId += 1 -const clientMutationId = nextClientMutationId.toString() +const clientMutationId = uuidv4()
66-80: Improve error handling and avoid direct console.error usage.Consider implementing a more robust error handling strategy instead of direct console.error calls, especially for production code.
onCompleted: (response, errors) => { if (errors) { - console.error(errors) + // Consider using a proper error logging/handling service + handleErrors(errors) return } const mutationErrors = response?.commentCreate?.errors setFormRelayErrors(form, mutationErrors) if (!mutationErrors?.length) { form.reset() if (ref && 'current' in ref) ref.current?.blur() } }, - onError: console.error, + onError: (error) => { + // Consider using a proper error logging/handling service + handleErrors(error) + },
91-96: Extract magic numbers into named constants.There are magic numbers in the View's height calculation. Although there are comments explaining these values, they would be more maintainable as named constants.
+const HANDLE_HEIGHT = 26 +const HANDLE_PADDING = 12 +const BOTTOM_PADDING = 88 <View - style={{ height: textHeight + (showHandle ? 38 : 0) + 88 }} + style={{ height: textHeight + (showHandle ? HANDLE_HEIGHT + HANDLE_PADDING : 0) + BOTTOM_PADDING }} // This space is taken by the social input drawer // 38 is handle height (26) plus padding (12) padding />
44-81: Extract mutation logic into a custom hook for better separation of concerns.The mutation logic is quite complex and could be extracted into a custom hook to make the component more focused on rendering and user interaction.
Consider creating a hook like
useCommentCreateHandlerthat encapsulates all the mutation logic:// useCommentCreateHandler.ts export function useCommentCreateHandler(form: UseFormReturn<SocialUpsertForm>, targetObjectId: string, ref: React.RefObject<NativeTextInput>) { const { currentProfile } = useCurrentProfile() const [commitMutation, isMutationInFlight] = useCommentCreateMutation() const handleSubmit = useCallback(() => { if (isMutationInFlight) return const body = form.getValues('body') const clientMutationId = uuidv4() const connectionID = ConnectionHandler.getConnectionID( targetObjectId, 'CommentsList_comments', ) commitMutation({ variables: { input: { body, targetObjectId, profileId: currentProfile?.id, clientMutationId, }, connections: [connectionID], }, onCompleted: (response, errors) => { // Handle errors and reset form }, onError: (error) => { // Handle error }, }) }, [form, isMutationInFlight, targetObjectId, currentProfile, commitMutation]) return { handleSubmit, isMutationInFlight } }Then in your component:
- const [commitMutation, isMutationInFlight] = useCommentCreateMutation() - const onSubmit = () => { - // Mutation logic - } + const { handleSubmit, isMutationInFlight } = useCommentCreateHandler(form, targetObjectId, ref)
🛑 Comments failed to post (4)
packages/components/modules/__shared__/native/SocialUpsertActions/index.tsx (1)
7-15: 🛠️ Refactor suggestion
Add props for event handling and customization
The component currently lacks props for handling interactions. For a social action component, it should at minimum support:
- An
onPresshandler for when the emoji button is clicked- Optional customization props for the icon or color
- A potential
disabledstate-import { FC } from 'react' +import { FC } from 'react' import { IconButton } from '@baseapp-frontend/design-system/components/native/buttons' import { EmojiIcon } from '@baseapp-frontend/design-system/components/native/icons' import { useTheme } from '@baseapp-frontend/design-system/providers/native' -const SocialUpsertActions: FC = () => { +interface SocialUpsertActionsProps { + onPress?: () => void; + disabled?: boolean; + iconColor?: string; +} + +const SocialUpsertActions: FC<SocialUpsertActionsProps> = ({ + onPress, + disabled, + iconColor, +}) => { const theme = useTheme() return ( - <IconButton> - <EmojiIcon color={theme.colors.object.low} /> + <IconButton onPress={onPress} disabled={disabled}> + <EmojiIcon color={iconColor || theme.colors.object.low} /> </IconButton> ) }📝 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.import { FC } from 'react' import { IconButton } from '@baseapp-frontend/design-system/components/native/buttons' import { EmojiIcon } from '@baseapp-frontend/design-system/components/native/icons' import { useTheme } from '@baseapp-frontend/design-system/providers/native' interface SocialUpsertActionsProps { onPress?: () => void; disabled?: boolean; iconColor?: string; } const SocialUpsertActions: FC<SocialUpsertActionsProps> = ({ onPress, disabled, iconColor, }) => { const theme = useTheme() return ( <IconButton onPress={onPress} disabled={disabled}> <EmojiIcon color={iconColor || theme.colors.object.low} /> </IconButton> ) }packages/design-system/components/native/inputs/SocialTextInput/index.tsx (1)
33-59: 💡 Verification agent
🧩 Analysis chain
Clean component structure with gesture handling
Good implementation of the social text input with:
- Appropriate use of
NativeViewGestureHandlerto prevent interruption during typing- Dynamic styling based on focus state
- Clear placeholder text
- Proper event handling for focus/blur states
However, there's a TODO comment that should be addressed before finalizing this PR.
🏁 Script executed:
#!/bin/bash # Check if there are other TODOs in the codebase related to replies implementation grep -r "TODO: Replies" packages/Length of output: 139
SocialTextInput Implementation: Clean Structure & Minor TODO Pending
The SocialTextInput component is well-implemented with robust gesture handling using
NativeViewGestureHandler, appropriate focus/blur event handling, and dynamic styling. However, the remaining"TODO: Replies"comment in the code should be addressed (either by implementing the replies functionality or removing the comment) before finalizing the PR.
- Confirm that the reply functionality is implemented or planned elsewhere.
- Remove or update the TODO comment accordingly.
packages/components/modules/__shared__/native/SocialInputDrawer/index.tsx (1)
30-42: 🛠️ Refactor suggestion
Fix dependency array and remove console logs.
There are two issues in the
handleAnimatecallback:
- Including
bottomSheetRef.currentin the dependency array won't work as React doesn't detect changes to.currentproperties- Console logs should be removed in production code
const handleAnimate = useCallback( (index: number, to: number) => { - console.log('handleAnimate', index, to) if (to === 0) { if (ref && 'current' in ref) ref.current?.blur() bottomSheetRef.current?.snapToIndex(1) } else if (to === 2) { if (ref && 'current' in ref) ref.current?.focus() bottomSheetRef.current?.snapToIndex(1) } }, - [ref, bottomSheetRef.current], + [ref], )📝 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 handleAnimate = useCallback( (index: number, to: number) => { if (to === 0) { if (ref && 'current' in ref) ref.current?.blur() bottomSheetRef.current?.snapToIndex(1) } else if (to === 2) { if (ref && 'current' in ref) ref.current?.focus() bottomSheetRef.current?.snapToIndex(1) } }, [ref], )packages/components/modules/comments/native/CommentCreate/index.tsx (1)
61-61: 🛠️ Refactor suggestion
Add null safety check for currentProfile.id.
Ensure there's proper null safety when accessing
currentProfile.idto prevent potential runtime errors if the current profile is not available.input: { body, targetObjectId, - profileId: currentProfile?.id, + profileId: currentProfile?.id ?? null, clientMutationId, },📝 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.input: { body, targetObjectId, profileId: currentProfile?.id ?? null, clientMutationId, },
tsl-ps2
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (16)
packages/design-system/components/native/icons/FavoriteIcon/index.tsx (1)
20-28: Consider adding accessibility attributesWhile the SVG implementation looks good, consider enhancing accessibility by adding ARIA attributes for screen readers, especially since this will be used as an interactive element for reactions.
<Svg width={width} height={height} viewBox="0 0 18 18" color={svgColor} fill="none" {...props}> + <Path + d="M9.00039 15.5999C7.50039 15.5999 1.65039 12.0749 1.65039 7.34985C1.65039 5.02485 3.45039 2.47485 6.22539 2.47485C7.50039 2.47485 8.40039 2.99985 9.00039 3.52485C9.60039 2.99985 10.5004 2.47485 11.7754 2.47485C14.4754 2.47485 16.3504 5.02485 16.3504 7.34985C16.3504 12.0749 10.5004 15.5999 9.00039 15.5999ZM6.22539 3.59985C4.20039 3.59985 2.77539 5.54985 2.77539 7.34985C2.77539 11.3999 7.95039 14.4749 8.92539 14.4749C9.90039 14.4749 15.0754 11.3999 15.0754 7.34985C15.0754 5.54985 13.7254 3.59985 11.6254 3.59985C10.4254 3.59985 9.67539 4.19985 9.30039 4.64985C9.07539 4.87485 8.62539 4.87485 8.47539 4.64985C8.17539 4.12485 7.42539 3.59985 6.22539 3.59985Z" + fill="currentColor" + role="img" + aria-label={isActive ? "Favorited" : "Favorite"} + />packages/components/modules/__shared__/native/Timestamp/index.tsx (5)
10-14: Add error handling for invalid date inputs.The code assumes the provided date string is always a valid ISO format. Consider adding validation or error handling to gracefully manage invalid date strings.
const Timestamp: FC<TimestampProps> = ({ date }) => { const dateTime = DateTime.fromISO(date) + // Handle invalid date input + if (!dateTime.isValid) { + console.warn(`Invalid date format: ${date}`) + return <Text variant="caption" color="low">--</Text> + } const now = DateTime.now() const diffDays = now.startOf('day').diff(dateTime.startOf('day'), 'days').days
15-19: Consider extracting magic numbers as named constants.The "7" days threshold is a magic number that could be extracted as a named constant for better readability and maintainability.
+// Maximum number of days to show as "X days ago" instead of a formatted date +const MAX_DAYS_AGO = 7 + const Timestamp: FC<TimestampProps> = ({ date }) => { // ...existing code... let text if (diffDays === 0) text = 'Today' else if (diffDays === 1) text = 'Yesterday' - else if (diffDays <= 7) text = `${diffDays} days ago` + else if (diffDays <= MAX_DAYS_AGO) text = `${diffDays} days ago` else text = formatDate(dateTime, { toFormat: DATE_FORMAT[2] })
19-19: Document the purpose of DATE_FORMAT[2].The code uses DATE_FORMAT[2] without explanation. Consider adding a comment to explain what this format represents, or use a more descriptive constant.
- else text = formatDate(dateTime, { toFormat: DATE_FORMAT[2] }) + // DATE_FORMAT[2] represents the format "MMM d, yyyy" (e.g., "Jan 1, 2023") + else text = formatDate(dateTime, { toFormat: DATE_FORMAT[2] })
21-25: Consider adding accessibility enhancements.For better screen reader support, consider adding an aria-label with a more verbose description of the timestamp.
return ( - <Text variant="caption" color="low"> + <Text + variant="caption" + color="low" + accessibilityLabel={`Posted ${text.toLowerCase()}`}> {text} </Text> )
10-26: Add performance optimization with useMemo.The component recalculates the formatted date on each render. Consider using useMemo to optimize performance, especially if this component will be used in lists.
-import { FC } from 'react' +import { FC, useMemo } from 'react' // ...other imports... const Timestamp: FC<TimestampProps> = ({ date }) => { - const dateTime = DateTime.fromISO(date) - const now = DateTime.now() - const diffDays = now.startOf('day').diff(dateTime.startOf('day'), 'days').days - - let text - if (diffDays === 0) text = 'Today' - else if (diffDays === 1) text = 'Yesterday' - else if (diffDays <= 7) text = `${diffDays} days ago` - else text = formatDate(dateTime, { toFormat: DATE_FORMAT[2] }) + const text = useMemo(() => { + const dateTime = DateTime.fromISO(date) + if (!dateTime.isValid) { + console.warn(`Invalid date format: ${date}`) + return '--' + } + + const now = DateTime.now() + const diffDays = now.startOf('day').diff(dateTime.startOf('day'), 'days').days + + if (diffDays === 0) return 'Today' + if (diffDays === 1) return 'Yesterday' + if (diffDays <= 7) return `${diffDays} days ago` + return formatDate(dateTime, { toFormat: DATE_FORMAT[2] }) + }, [date]) return ( <Text variant="caption" color="low"> {text} </Text> ) }packages/design-system/components/native/inputs/SocialTextInput/index.tsx (3)
35-35: Address the TODO comment or create a tracking issue.There's a TODO comment regarding replies functionality. It's best to either implement this now or create a tracking issue to ensure it's not forgotten in future development.
36-57: Add accessibility properties to enhance user experience.The TextInput component lacks accessibility properties such as
accessibilityLabelandaccessibilityHint. Consider adding these to improve accessibility for users with screen readers.<PaperTextInput contentStyle={styles.contentStyle} mode="outlined" multiline outlineStyle={styles.outlineStyle} placeholder="Comment..." placeholderTextColor={theme.colors.object.low} + accessibilityLabel="Comment input" + accessibilityHint="Enter your comment here" ref={ref} render={(renderProps) => ( // ...existing code )} // ...rest of the props />
26-31: Consider documenting text truncation behavior.The height calculation limits the input to
maxLines * lineHeight, but there's no clear documentation on what happens when users type beyond this limit. Consider adding a comment explaining whether text is still accepted but scrolled, or if there are other constraints.packages/components/modules/comments/native/CommentItem/CommentReactionButton/index.tsx (1)
15-31: Consider moving inline styles to the styles file.The component looks good overall and implements the heart icon reaction functionality as specified in the acceptance criteria. However, there's an inline style on the IconButton that could be moved to the styles file for better maintainability:
- <IconButton onPress={handleReaction} style={{ paddingHorizontal: 0, paddingVertical: 5 }}> + <IconButton onPress={handleReaction} style={styles.iconButton}>And in styles.ts:
iconButton: { paddingHorizontal: 0, paddingVertical: 5, },packages/components/modules/comments/native/CommentsList/index.tsx (3)
16-17: Consider handling loading or error states.If
useCommentListhasn't finished fetching data or encounters an error,targetmay beundefined. Attempting to read fromtarget.idortarget.commentslater will result in a runtime error. It may be prudent to guard against undefined data with a conditional check or a loading placeholder.
23-34: Handle invalid comment items gracefully.If a comment node is invalid or lacking an
id, the current logic returnsnull. This is acceptable for a best-effort approach, but you could consider a fallback UI or a warning log to help debug unexpected data.
43-44: Add pagination support.There is a TODO comment for proper pagination. This is crucial for larger comment threads. Consider using a Relay pagination container or a strategy like infinite scrolling to handle numerous comments.
Would you like a sample implementation of Relay's pagination or an infinite scroll approach for a future iteration?
packages/components/modules/comments/native/CommentCreate/index.tsx (3)
45-81: Enhance error handling in the mutation callback.Currently, errors are only logged. Consider a more user-facing error handling mechanism (e.g., toast message) so that users receive direct feedback if a comment fails to post.
onCompleted: (response, errors) => { if (errors) { console.error(errors) + // Optionally provide a user-friendly error notification, e.g. + // showToast("Something went wrong posting your comment.") return } ...
75-77: Blur text input only if ref is available.This logic defensively checks for a ref before blurring. If there’s a chance the component might re-mount or the ref is not set, ensure graceful handling by adding a short-circuit or descriptive logging (if needed).
87-97: Consider leveraging KeyboardAvoidingView.Currently, you’re offsetting the ScrollView height. For a more native experience across multiple screen sizes and devices, using React Native’s KeyboardAvoidingView or additional layout libraries can provide smoother input interactions, especially in landscape or large form factors.
Would you like a quick example snippet to integrate
KeyboardAvoidingView?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
packages/components/modules/__shared__/common/index.ts(1 hunks)packages/components/modules/__shared__/native/SocialInputBox/SubmitActions/index.tsx(1 hunks)packages/components/modules/__shared__/native/SocialInputBox/SubmitActions/types.ts(1 hunks)packages/components/modules/__shared__/native/SocialInputBox/index.tsx(1 hunks)packages/components/modules/__shared__/native/SocialInputBox/types.ts(1 hunks)packages/components/modules/__shared__/native/SocialInputDrawer/DrawerHandle/index.tsx(1 hunks)packages/components/modules/__shared__/native/SocialInputDrawer/DrawerHandle/styles.ts(1 hunks)packages/components/modules/__shared__/native/SocialInputDrawer/index.tsx(1 hunks)packages/components/modules/__shared__/native/SocialInputDrawer/styles.ts(1 hunks)packages/components/modules/__shared__/native/SocialInputDrawer/types.ts(1 hunks)packages/components/modules/__shared__/native/SocialUpsertActions/index.tsx(1 hunks)packages/components/modules/__shared__/native/Timestamp/index.tsx(1 hunks)packages/components/modules/__shared__/native/Timestamp/types.ts(1 hunks)packages/components/modules/__shared__/native/index.ts(1 hunks)packages/components/modules/__shared__/web/ReactionButton/__storybook__/ReactionButtonWithQuery/index.tsx(1 hunks)packages/components/modules/__shared__/web/ReactionButton/__storybook__/stories.tsx(1 hunks)packages/components/modules/__shared__/web/index.ts(0 hunks)packages/components/modules/comments/native/CommentCreate/index.tsx(1 hunks)packages/components/modules/comments/native/CommentCreate/styles.ts(1 hunks)packages/components/modules/comments/native/CommentCreate/types.ts(1 hunks)packages/components/modules/comments/native/CommentItem/CommentReactionButton/index.tsx(1 hunks)packages/components/modules/comments/native/CommentItem/CommentReactionButton/styles.ts(1 hunks)packages/components/modules/comments/native/CommentItem/CommentReactionButton/types.ts(1 hunks)packages/components/modules/comments/native/CommentItem/CommentReplyButton/index.tsx(1 hunks)packages/components/modules/comments/native/CommentItem/CommentReplyButton/styles.ts(1 hunks)packages/components/modules/comments/native/CommentItem/CommentReplyButton/types.ts(1 hunks)packages/components/modules/comments/native/CommentItem/index.tsx(1 hunks)packages/components/modules/comments/native/CommentItem/styles.ts(1 hunks)packages/components/modules/comments/native/CommentItem/types.ts(1 hunks)packages/components/modules/comments/native/Comments/index.tsx(1 hunks)packages/components/modules/comments/native/Comments/styles.ts(1 hunks)packages/components/modules/comments/native/Comments/types.ts(1 hunks)packages/components/modules/comments/native/CommentsList/index.tsx(1 hunks)packages/components/modules/comments/native/CommentsList/styles.ts(1 hunks)packages/components/modules/comments/native/CommentsList/types.ts(1 hunks)packages/components/modules/comments/native/index.ts(1 hunks)packages/components/modules/comments/web/CommentItem/CommentReactionButton/index.tsx(1 hunks)packages/design-system/components/native/icons/EmojiIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/FavoriteIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/FavoriteSelectedIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/ReplyIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/SendMessageIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/index.ts(2 hunks)packages/design-system/components/native/inputs/SocialTextInput/context/SocialTextInputProvider/constants.ts(1 hunks)packages/design-system/components/native/inputs/SocialTextInput/context/SocialTextInputProvider/index.tsx(1 hunks)packages/design-system/components/native/inputs/SocialTextInput/context/SocialTextInputProvider/types.ts(1 hunks)packages/design-system/components/native/inputs/SocialTextInput/context/useSocialTextInput/index.tsx(1 hunks)packages/design-system/components/native/inputs/SocialTextInput/context/withSocialTextInputProvider/index.tsx(1 hunks)packages/design-system/components/native/inputs/SocialTextInput/index.tsx(1 hunks)packages/design-system/components/native/inputs/SocialTextInput/styles.ts(1 hunks)packages/design-system/components/native/inputs/SocialTextInput/types.ts(1 hunks)packages/design-system/components/native/inputs/index.ts(1 hunks)packages/design-system/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/modules/shared/web/index.ts
🚧 Files skipped from review as they are similar to previous changes (43)
- packages/components/modules/shared/native/Timestamp/types.ts
- packages/components/modules/comments/native/CommentsList/styles.ts
- packages/design-system/components/native/inputs/SocialTextInput/context/SocialTextInputProvider/constants.ts
- packages/components/modules/comments/native/CommentItem/CommentReactionButton/styles.ts
- packages/components/modules/comments/native/CommentItem/CommentReactionButton/types.ts
- packages/components/modules/comments/native/CommentCreate/styles.ts
- packages/components/modules/comments/web/CommentItem/CommentReactionButton/index.tsx
- packages/components/modules/shared/native/SocialInputDrawer/DrawerHandle/index.tsx
- packages/components/modules/shared/native/SocialInputDrawer/DrawerHandle/styles.ts
- packages/design-system/components/native/inputs/SocialTextInput/context/withSocialTextInputProvider/index.tsx
- packages/components/modules/comments/native/CommentItem/styles.ts
- packages/components/modules/comments/native/CommentItem/CommentReplyButton/types.ts
- packages/design-system/components/native/icons/SendMessageIcon/index.tsx
- packages/design-system/package.json
- packages/components/modules/shared/native/SocialInputBox/SubmitActions/types.ts
- packages/components/modules/shared/native/SocialInputBox/SubmitActions/index.tsx
- packages/components/modules/shared/native/SocialUpsertActions/index.tsx
- packages/components/modules/comments/native/CommentCreate/types.ts
- packages/design-system/components/native/icons/EmojiIcon/index.tsx
- packages/design-system/components/native/inputs/SocialTextInput/context/useSocialTextInput/index.tsx
- packages/components/modules/comments/native/CommentItem/CommentReplyButton/index.tsx
- packages/components/modules/comments/native/CommentItem/types.ts
- packages/components/modules/shared/native/SocialInputBox/types.ts
- packages/design-system/components/native/inputs/SocialTextInput/types.ts
- packages/design-system/components/native/icons/FavoriteSelectedIcon/index.tsx
- packages/components/modules/comments/native/Comments/styles.ts
- packages/components/modules/shared/web/ReactionButton/storybook/stories.tsx
- packages/components/modules/shared/native/SocialInputDrawer/index.tsx
- packages/components/modules/comments/native/CommentsList/types.ts
- packages/components/modules/comments/native/Comments/index.tsx
- packages/design-system/components/native/inputs/SocialTextInput/context/SocialTextInputProvider/types.ts
- packages/components/modules/shared/native/SocialInputDrawer/styles.ts
- packages/components/modules/shared/native/index.ts
- packages/design-system/components/native/inputs/SocialTextInput/styles.ts
- packages/design-system/components/native/icons/ReplyIcon/index.tsx
- packages/components/modules/comments/native/index.ts
- packages/design-system/components/native/inputs/index.ts
- packages/components/modules/shared/common/index.ts
- packages/design-system/components/native/inputs/SocialTextInput/context/SocialTextInputProvider/index.tsx
- packages/components/modules/shared/web/ReactionButton/storybook/ReactionButtonWithQuery/index.tsx
- packages/components/modules/shared/native/SocialInputBox/index.tsx
- packages/components/modules/shared/native/SocialInputDrawer/types.ts
- packages/design-system/components/native/icons/index.ts
🧰 Additional context used
🧬 Code Definitions (3)
packages/components/modules/__shared__/native/Timestamp/index.tsx (1)
packages/components/modules/__shared__/native/Timestamp/types.ts (1)
TimestampProps(1-3)
packages/components/modules/comments/native/CommentItem/CommentReactionButton/index.tsx (2)
packages/components/modules/comments/native/CommentItem/CommentReactionButton/types.ts (1)
CommentReactionButtonProps(3-5)packages/components/modules/comments/native/CommentItem/CommentReactionButton/styles.ts (1)
createStyles(3-12)
packages/design-system/components/native/inputs/SocialTextInput/index.tsx (2)
packages/design-system/components/native/inputs/SocialTextInput/types.ts (1)
SocialTextInputProps(8-15)packages/design-system/components/native/inputs/SocialTextInput/styles.ts (1)
createStyles(5-41)
🔇 Additional comments (15)
packages/design-system/components/native/icons/FavoriteIcon/index.tsx (4)
1-7: Import structure looks goodThe imports are well-organized with React imports first, followed by library imports, and then internal imports. This follows common React code organization practices.
8-14: Props structure is well-definedGood use of default values for
isActive,width, andheightprops. The component correctly leverages TypeScript's FC type with SvgIconProps for proper type checking.
15-19: Well-implemented theme integrationThe component correctly uses the theme system with the
useThemehook, and handles color logic appropriately, using a ternary operator to determine the active state color.
29-30: Default export is appropriateUsing default export for this component follows the established pattern for React components.
packages/components/modules/__shared__/native/Timestamp/index.tsx (1)
1-8: LGTM: Clean import organization.The imports are well-organized and appropriately grouped by source (React, internal packages, third-party library, and local files).
packages/design-system/components/native/inputs/SocialTextInput/index.tsx (3)
18-62: Well-structured component with good separation of concerns.The component follows React best practices with proper use of forwarded refs, custom hooks for state management, and clean separation of rendering logic. The dynamic height adjustment based on content size is a good user experience enhancement.
45-52: Good use of NativeViewGestureHandler for better input experience.Wrapping the text input with
NativeViewGestureHandlerprevents gesture interruptions during typing, which significantly improves the user experience. This is especially important in a social commenting context where smooth input experience is essential.
1-16: Well-organized imports with clear separation.The imports are well-organized with a clear separation between external dependencies and internal modules. This makes the code more readable and easier to maintain.
packages/components/modules/comments/native/CommentItem/CommentReplyButton/styles.ts (1)
3-12: LGTM! Clean styling implementation.The styling follows React Native best practices with a function-based pattern that creates a StyleSheet. The replyContainer has appropriate flex properties for displaying a row with centered alignment.
packages/components/modules/comments/native/CommentItem/index.tsx (2)
29-32: TODO and console.log should be addressed in future PRs.Note the placeholder implementation for reply functionality. This aligns with the PR objectives mentioning that threading for comments is not included in this implementation.
38-62: LGTM! Component structure follows React best practices.The component correctly renders the comment with user avatar, name, body text, timestamp, and interactive elements (reaction and reply buttons). This implementation satisfies the acceptance criteria for displaying comments with user information and timestamps.
packages/components/modules/comments/native/Comments/types.ts (1)
7-14: Well-structured props interface with good flexibility.The CommentsProps interface provides excellent composition capabilities by allowing custom component overrides and their respective props. This supports the acceptance criteria for implementing a comment creation feature with customizable components.
packages/components/modules/comments/native/CommentsList/index.tsx (2)
1-4: Imports look good.These imports neatly organize shared methods and components. No immediate concerns here.
18-21: Memoizing filter logic is appropriate but be mindful of potential null arrays.The usage of
useMemohere is a solid decision for performance, but confirm thattarget?.comments?.edgesis always an array ornull. If there's a chance it might be something else, consider a stronger null check.packages/components/modules/comments/native/CommentCreate/index.tsx (1)
8-17: Good use of zod + react-hook-form.This approach is robust for validating user input. It ensures comments adhere to the schema before submission.
|
|
||
| return ( | ||
| <> | ||
| {subscriptionsEnabled && <CommentsSubscription targetObjectId={target.id} />} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for target?.id before rendering subscription.
If target or target.id is undefined, <CommentsSubscription> rendering will fail. A defensive check can prevent runtime errors.
- {subscriptionsEnabled && <CommentsSubscription targetObjectId={target.id} />}
+ {subscriptionsEnabled && target?.id && (
+ <CommentsSubscription targetObjectId={target.id} />
+ )}📝 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.
| {subscriptionsEnabled && <CommentsSubscription targetObjectId={target.id} />} | |
| {subscriptionsEnabled && target?.id && ( | |
| <CommentsSubscription targetObjectId={target.id} /> | |
| )} |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/modules/comments/native/index.ts (1)
1-2: Add more detailed documentation for the module.While the current comment indicates the purpose of the file, consider enhancing it with more detailed JSDoc documentation. This would provide better context for developers about the exported components and their intended use.
-// exports native comments components +/** + * Native comment components for React Native applications. + * This module exports components for creating, displaying, and interacting with comments. + * @module comments/native + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/modules/comments/native/index.ts(1 hunks)
🔇 Additional comments (1)
packages/components/modules/comments/native/index.ts (1)
3-13: LGTM! Well-structured exports for the comments module.The export structure is clean, consistent, and follows best practices for TypeScript barrel exports. The pattern of exporting both components and their types makes it easy for consumers to import what they need.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/design-system/components/native/inputs/SocialTextInput/index.tsx (2)
63-69: Address missing dependency in theuseCallbackhooks.Static analysis points out that
propsis missing from the dependency array. To avoid re-creating these callbacks whenever unrelated props change, destructure only the needed props (e.g.,onBlur,onFocus) outside theuseCallbackor include specific fields in the dependency array instead of using the entirepropsobject.- const handleBlur = useCallback( - (args: any) => { - props.onBlur?.(args) - setSocialInputState({ isFocused: false }) - }, - [props.onBlur, setSocialInputState], - ) + const { onBlur } = props + const handleBlur = useCallback( + (args: any) => { + onBlur?.(args) + setSocialInputState({ isFocused: false }) + }, + [onBlur, setSocialInputState], + )Also applies to: 71-77
🧰 Tools
🪛 GitHub Check: Build and Lint Packages
[warning] 68-68:
React Hook useCallback has a missing dependency: 'props'. Either include it or remove the dependency array. However, 'props' will change when any prop changes, so the preferred fix is to destructure the 'props' object outside of the useCallback call and refer to those specific props inside useCallback
89-89: Consider localizing hardcoded placeholder text.Your placeholder
"Comment..."text is currently hardcoded. For better localization and user experience, consider extracting it into a translations file or using a localization library.Also applies to: 103-103
packages/components/modules/__shared__/native/SocialInputBox/SubmitActions/index.tsx (1)
26-37: Appropriate conditional rendering based on context.The component intelligently switches between TouchableOpacity (from bottom sheet) and IconButton based on the context in which it's used, ensuring compatibility with the bottom sheet environment when needed.
One minor improvement opportunity: the inline style
{ padding: 8 }could be moved to a separate styles object or file for better maintainability.- <TouchableOpacity disabled={disabled} onPress={handleSubmit} style={{ padding: 8 }}> + <TouchableOpacity disabled={disabled} onPress={handleSubmit} style={styles.touchableOpacity}>And add at the top of the file:
const styles = { touchableOpacity: { padding: 8 } }packages/components/modules/__shared__/native/SocialInputDrawer/index.tsx (3)
38-60: Platform-specific keyboard handling may be fragile.The keyboard event handling has platform-specific logic and calculates keyboard height manually on iOS. This approach might be fragile across different iOS versions or device types. Consider using a library like react-native-keyboard-aware-scroll-view or similar for more robust keyboard handling.
Also, the cleanup function could be simplified using array methods.
- return () => { - showListener.remove() - hideListener.remove() - } + return () => [showListener, hideListener].forEach(listener => listener.remove())
62-88: Redundant logic in sheet change handlers.Both
handleSheetChangeandhandleAnimatecontain very similar logic. Consider refactoring to reduce duplication:- const handleSheetChange = useCallback( - (index: number) => { - if (index !== 1) { - bottomSheetRef.current?.snapToIndex(1) - if (index === 0) { - if (ref && 'current' in ref) ref.current?.blur() - } else if (index === 2) { - if (ref && 'current' in ref) ref.current?.focus() - } - } - }, - [ref, bottomSheetRef], - ) - - const handleAnimate = useCallback( - (from: number, to: number) => { - if (to !== 1) { - bottomSheetRef.current?.snapToIndex(1) - } - if (to === 0) { - if (ref && 'current' in ref) ref.current?.blur() - } else if (to === 2) { - if (ref && 'current' in ref) ref.current?.focus() - } - }, - [ref, bottomSheetRef], - ) + const handleSheetIndexChange = useCallback( + (index: number) => { + if (index !== 1) { + bottomSheetRef.current?.snapToIndex(1) + if (index === 0) { + if (ref && 'current' in ref) ref.current?.blur() + } else if (index === 2) { + if (ref && 'current' in ref) ref.current?.focus() + } + } + }, + [ref, bottomSheetRef], + ) + + const handleSheetChange = useCallback( + (index: number) => handleSheetIndexChange(index), + [handleSheetIndexChange], + ) + + const handleAnimate = useCallback( + (_from: number, to: number) => handleSheetIndexChange(to), + [handleSheetIndexChange], + )
96-102: Hardcoded snap points and conditional rendering can be improved.The snap points are hardcoded values (80, 200) which may not work well across different screen sizes. Consider making these values relative to screen dimensions.
Also, the conditional rendering of the drawer handle can be simplified:
- handleComponent={showHandle ? DrawerHandle : null} + handleComponent={showHandle && DrawerHandle}packages/components/modules/comments/native/CommentCreate/index.tsx (2)
23-23: Client mutation ID variable could cause issues in certain environments.The
nextClientMutationIdvariable is declared outside the component, which is good for persistence across renders. However, this approach might cause issues in server-side rendering environments where the variable would be shared across all users.Consider using a more robust approach such as a UUID or timestamp-based ID:
- let nextClientMutationId = 0 + import { v4 as uuidv4 } from 'uuid';And then in the component:
- nextClientMutationId += 1 - const clientMutationId = nextClientMutationId.toString() + const clientMutationId = uuidv4()
87-97: Complex calculation for spacer view height.The calculation for the spacer view height (
textHeight + (showHandle ? 38 : 0) + 88) includes magic numbers and might be difficult to maintain. Consider extracting these values into named constants for better readability and maintainability.+ const HANDLE_HEIGHT = 26; + const HANDLE_PADDING = 12; + const BASE_HEIGHT = 88; + return ( <> <ScrollView style={styles.contentContainer}> {children} <View - style={{ height: textHeight + (showHandle ? 38 : 0) + 88 }} + style={{ height: textHeight + (showHandle ? HANDLE_HEIGHT + HANDLE_PADDING : 0) + BASE_HEIGHT }} // This space is taken by the social input drawer. // We add it here so that no scrollable content is hidden behind the social input drawer. // 38 is handle height (26) plus padding (12) />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
packages/components/modules/__shared__/native/SocialInputBox/SubmitActions/index.tsx(1 hunks)packages/components/modules/__shared__/native/SocialInputBox/SubmitActions/types.ts(1 hunks)packages/components/modules/__shared__/native/SocialInputBox/index.tsx(1 hunks)packages/components/modules/__shared__/native/SocialInputBox/types.ts(1 hunks)packages/components/modules/__shared__/native/SocialInputDrawer/index.tsx(1 hunks)packages/components/modules/comments/native/CommentCreate/index.tsx(1 hunks)packages/components/package.json(1 hunks)packages/design-system/components/native/inputs/SocialTextInput/index.tsx(1 hunks)packages/design-system/components/native/inputs/SocialTextInput/styles.ts(1 hunks)packages/design-system/components/native/inputs/SocialTextInput/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/design-system/components/native/inputs/SocialTextInput/types.ts
- packages/components/modules/shared/native/SocialInputBox/SubmitActions/types.ts
- packages/components/modules/shared/native/SocialInputBox/types.ts
- packages/components/modules/shared/native/SocialInputBox/index.tsx
🧰 Additional context used
🧬 Code Definitions (2)
packages/components/modules/__shared__/native/SocialInputBox/SubmitActions/index.tsx (1)
packages/components/modules/__shared__/native/SocialInputBox/SubmitActions/types.ts (1)
SubmitActionsProps(5-11)
packages/components/modules/comments/native/CommentCreate/index.tsx (3)
packages/components/modules/comments/native/CommentCreate/types.ts (1)
CommentCreateProps(7-12)packages/components/modules/comments/native/CommentCreate/styles.ts (1)
createStyles(3-11)packages/components/modules/__shared__/native/SocialInputDrawer/types.ts (1)
SocialInputDrawerProps(9-18)
🪛 GitHub Check: Build and Lint Packages
packages/design-system/components/native/inputs/SocialTextInput/index.tsx
[warning] 68-68:
React Hook useCallback has a missing dependency: 'props'. Either include it or remove the dependency array. However, 'props' will change when any prop changes, so the preferred fix is to destructure the 'props' object outside of the useCallback call and refer to those specific props inside useCallback
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Component Test Packages
🔇 Additional comments (16)
packages/design-system/components/native/inputs/SocialTextInput/index.tsx (1)
20-120: No immediate functional issues found.The rest of the component logic, especially the height management on content size changes, platform checks, and the gesture handler usage, appears sound.
🧰 Tools
🪛 GitHub Check: Build and Lint Packages
[warning] 68-68:
React Hook useCallback has a missing dependency: 'props'. Either include it or remove the dependency array. However, 'props' will change when any prop changes, so the preferred fix is to destructure the 'props' object outside of the useCallback call and refer to those specific props inside useCallbackpackages/components/package.json (1)
93-93:✅ Verification successful
Validate new dependency version.
Adding
"react-native-safe-area-context": "^5.3.0"appears logical. However, please confirm compatibility with your current React Native version and other packages to avoid any conflicts.
🏁 Script executed:
#!/bin/bash # Check if there's any known compatibility or peer dependencies grep "react-native" package.json rg "react-native" -A 5Length of output: 66392
Dependency version "react-native-safe-area-context" has been validated
After reviewing the dependency tree in both the workspace and lock files, the version "^5.3.0" is clearly in use and aligns with the current React Native version (0.76.7) along with other related dependencies. There are no indication of conflicts or peer dependency issues.
packages/design-system/components/native/inputs/SocialTextInput/styles.ts (1)
1-65: Overall styling approach looks good.The style definitions align well with your theme-based approach, supporting dynamic input height and focused states. This clean separation of styling enhances maintainability.
packages/components/modules/__shared__/native/SocialInputBox/SubmitActions/index.tsx (4)
1-8: Clean import structure with appropriate dependencies.The imports are well-organized, separating external libraries from internal components. The component correctly imports the TouchableOpacity from @gorhom/bottom-sheet which is compatible with the bottom sheet context.
11-17: Well-defined component with proper TypeScript typing.The component definition includes appropriate default values for optional props and follows React's functional component pattern with proper TypeScript typing.
18-24: Good use of theme system for consistent styling.The component properly utilizes the theme system to ensure consistent color application across the app, which is particularly important for indicating the disabled state.
40-40: Clean default export.Single default export follows the component pattern used in the project.
packages/components/modules/__shared__/native/SocialInputDrawer/index.tsx (3)
1-18: Clean import structure with appropriate dependencies.The imports are well-organized, correctly importing from design system and third-party libraries like @gorhom/bottom-sheet.
20-33: Well-structured component with appropriate use of forwardRef.Good use of forwardRef to pass the TextInput reference through the component hierarchy, which is important for managing focus.
104-119: Good implementation of keyboard-aware bottom sheet content.The component correctly handles the keyboard by adding a spacer view with the keyboard height. The comments explaining this behavior are helpful for future developers.
packages/components/modules/comments/native/CommentCreate/index.tsx (6)
1-22: Clean import structure with appropriate dependencies.The imports are well-organized and correctly import necessary hooks and utilities for form handling and mutations.
25-35: Well-structured component with appropriate use of forwardRef.Good use of forwardRef to pass the TextInput reference through the component hierarchy, and proper default values for optional props.
36-42: Clean form setup with proper validation.The form is properly set up with default values and validation schema using zod and react-hook-form.
83-85: Good use of hooks for UI state management.Using the
useSocialTextInputhook to manage text input height and focus state is a clean approach.
98-106: Good implementation of SocialInputDrawer with proper props.The SocialInputDrawer is properly implemented with the necessary props for form handling and submission.
112-112: Clean default export.Single default export follows the component pattern used in the project.
| const [commitMutation, isMutationInFlight] = useCommentCreateMutation() | ||
| const onSubmit = () => { | ||
| if (isMutationInFlight) return | ||
|
|
||
| nextClientMutationId += 1 | ||
| const clientMutationId = nextClientMutationId.toString() | ||
|
|
||
| const connectionID = ConnectionHandler.getConnectionID( | ||
| targetObjectId, | ||
| 'CommentsList_comments', | ||
| ) | ||
|
|
||
| commitMutation({ | ||
| variables: { | ||
| input: { | ||
| body, | ||
| targetObjectId, | ||
| profileId: currentProfile?.id, | ||
| clientMutationId, | ||
| }, | ||
| connections: [connectionID], | ||
| }, | ||
| onCompleted: (response, errors) => { | ||
| if (errors) { | ||
| console.error(errors) | ||
| return | ||
| } | ||
| const mutationErrors = response?.commentCreate?.errors | ||
| setFormRelayErrors(form, mutationErrors) | ||
|
|
||
| if (!mutationErrors?.length) { | ||
| form.reset() | ||
| if (ref && 'current' in ref) ref.current?.blur() | ||
| } | ||
| }, | ||
| onError: console.error, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handling needs improvement.
The error handling uses console.error which is not ideal for production code. Consider implementing a more robust error handling strategy and providing more user-friendly error messages.
- onError: console.error,
+ onError: (error) => {
+ // Log to your error tracking service
+ console.error('Failed to create comment:', error);
+ // Optionally show user-friendly error message
+ // showErrorToast('Failed to create comment. Please try again.');
+ },Also, in the onCompleted handler:
- if (errors) {
- console.error(errors)
- return
- }
+ if (errors) {
+ // Log to your error tracking service
+ console.error('Errors in comment creation response:', errors)
+ // Optionally show user-friendly error message
+ // showErrorToast('Failed to create comment. Please try again.');
+ return
+ }📝 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 [commitMutation, isMutationInFlight] = useCommentCreateMutation() | |
| const onSubmit = () => { | |
| if (isMutationInFlight) return | |
| nextClientMutationId += 1 | |
| const clientMutationId = nextClientMutationId.toString() | |
| const connectionID = ConnectionHandler.getConnectionID( | |
| targetObjectId, | |
| 'CommentsList_comments', | |
| ) | |
| commitMutation({ | |
| variables: { | |
| input: { | |
| body, | |
| targetObjectId, | |
| profileId: currentProfile?.id, | |
| clientMutationId, | |
| }, | |
| connections: [connectionID], | |
| }, | |
| onCompleted: (response, errors) => { | |
| if (errors) { | |
| console.error(errors) | |
| return | |
| } | |
| const mutationErrors = response?.commentCreate?.errors | |
| setFormRelayErrors(form, mutationErrors) | |
| if (!mutationErrors?.length) { | |
| form.reset() | |
| if (ref && 'current' in ref) ref.current?.blur() | |
| } | |
| }, | |
| onError: console.error, | |
| }) | |
| } | |
| const [commitMutation, isMutationInFlight] = useCommentCreateMutation() | |
| const onSubmit = () => { | |
| if (isMutationInFlight) return | |
| nextClientMutationId += 1 | |
| const clientMutationId = nextClientMutationId.toString() | |
| const connectionID = ConnectionHandler.getConnectionID( | |
| targetObjectId, | |
| 'CommentsList_comments', | |
| ) | |
| commitMutation({ | |
| variables: { | |
| input: { | |
| body, | |
| targetObjectId, | |
| profileId: currentProfile?.id, | |
| clientMutationId, | |
| }, | |
| connections: [connectionID], | |
| }, | |
| onCompleted: (response, errors) => { | |
| if (errors) { | |
| // Log to your error tracking service | |
| console.error('Errors in comment creation response:', errors) | |
| // Optionally show user-friendly error message | |
| // showErrorToast('Failed to create comment. Please try again.'); | |
| return | |
| } | |
| const mutationErrors = response?.commentCreate?.errors | |
| setFormRelayErrors(form, mutationErrors) | |
| if (!mutationErrors?.length) { | |
| form.reset() | |
| if (ref && 'current' in ref) ref.current?.blur() | |
| } | |
| }, | |
| onError: (error) => { | |
| // Log to your error tracking service | |
| console.error('Failed to create comment:', error); | |
| // Optionally show user-friendly error message | |
| // showErrorToast('Failed to create comment. Please try again.'); | |
| }, | |
| }) | |
| } |
Description
Acceptance Criteria
Basic Comment Creation
Since we wont have flat pages in React native and the Content feeds is not ready yet. We need a place to "host" comments in order to test them.
So we will create a comment route on the RN application to do so.
Business Rules
Create a "Comment" route where we will be able to test the comment component and its functionalities
Recreate the comment component for React Native
Users should be able to create a comment via an input box.
Comment Component Behavior
Given a user clicks on the input box, the mobile keyboard should slide up in order for the user to type in the content.
Given a user presses on the Enter Key in the keyboard, it should create a line break in the content.
Given a user has finished creating a comment, when they press the "send button", then the comment should be created.
Given the comment has been created, then the new comment should appear at the top of the comment list.
Reaction Component Behavior
Each comment should display the user’s avatar, name, and timestamp
Users should be able to react to the comment by pression the heart icon.
Given the user presses the Reaction Icon, the count should increase by 1
Given the same user presses the reaction icon again after having already reacted to the comment., the count should decrease by 1.
Guardrails
Threads wont be implemented for now.
Load only the first 5 comments. Pagination will be handled later
Design Link: https://www.figma.com/design/z5ZdVlKEMmGIX4GVnjwfxA/BaseApp---NATIVE?node-id=942-18665&t=jbYKzqIHwPMDjdiS-0
Notes
create a temporary model and inherit the Commentable model
we can reuse the reply logic (e.g.
useCommentReply) and the queries on the webApprovd
https://app.approvd.io/silverlogic/BA/stories/39781
Summary by CodeRabbit