refactor and fix modal #1094
refactor and fix modal #1094
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (7)
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.utils.ts (1)
1-16: Consider extracting modal logic into a custom hook.The current utility file mixes concerns between modal management and message state. Consider creating a custom hook (e.g.,
useMessageReactionsModal) that encapsulates both the modal ref and state management logic. This would:
- Improve reusability
- Make testing easier
- Provide a cleaner API for components
Example structure:
// useMessageReactionsModal.ts export function useMessageReactionsModal() { const modalRef = useRef(createBottomSheetModalRef()); const store = useMessageReactionsStore(); const open = useCallback((message: MessageToDisplay) => { // implementation }, []); const close = useCallback(() => { // implementation }, []); return { open, close, message: store.message }; }design-system/BottomSheet/BottomSheetBackdropOpacity.tsx (1)
12-13: Consider adding TypeScript types for destructured props.While the destructuring improves code readability, the
anytypes in the component props interface (mentioned in the summary) could be more strictly typed.- props: GorhomBottomSheetBackdropProps & { style?: any; animatedIndex?: any; animatedPosition?: any; } + props: GorhomBottomSheetBackdropProps & { + style?: ViewStyle; + animatedIndex?: Animated.SharedValue<number>; + animatedPosition?: Animated.SharedValue<number>; + }design-system/BottomSheet/BottomSheetModal.tsx (1)
46-46: Enhance documentation for disabled dynamic sizingWhile disabling dynamic sizing by default is a valid choice, consider making this more configurable and documenting the rationale more thoroughly.
Consider updating the comment to be more descriptive:
- enableDynamicSizing={false} // By default we don't want enable dynamic sizing + enableDynamicSizing={false} // Disabled by default to ensure consistent modal heights across the app. Override via props if needed.components/Chat/Message/MessageReactions/useMessageReactionsRolledup.ts (1)
7-20: Add JSDoc comments to type definitions.Consider adding documentation to explain the purpose and fields of these types for better maintainability.
+/** + * Details for a specific reaction emoji, including count, reactors, and timing. + */ type ReactionDetails = { content: string; count: number; userReacted: boolean; reactors: string[]; firstReactionTime: number; }; +/** + * Aggregated reaction data including top emojis, total count, and detailed breakdown. + */ type RolledUpReactions = { emojis: string[]; totalReactions: number; userReacted: boolean; details: { [content: string]: ReactionDetails }; };components/Chat/Message/MessageReactions/MessageReactions.tsx (3)
19-20: Consider renaming for consistency with component hierarchy.The component is named
ChatMessageReactionsbut lives in a Chat context already (under components/Chat/). Consider removing the redundant "Chat" prefix to follow the directory structure naming convention.-export const ChatMessageReactions = memo( +export const MessageReactions = memo(
79-106: LGTM! Clean and consistent styling implementation.The styles follow the design system well and make good use of theme variables. Consider extracting the xxxs spacing value to a theme constant if it's used across multiple components.
19-67: Consider adding error boundary and accessibility improvements.While the component is well-structured, consider these architectural improvements:
- Wrap the component in an error boundary to handle rendering failures gracefully
- Add proper accessibility attributes (aria-label, role) for the reaction buttons
- Consider adding a loading state for when reactions are being fetched
Example accessibility improvements:
<TouchableHighlight onPress={handlePress} - underlayColor="transparent"> + underlayColor="transparent" + accessibilityRole="button" + accessibilityLabel={`${rolledUpReactions.totalReactions} reactions to message`}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
- components/Chat/Chat.tsx (2 hunks)
- components/Chat/Message/Message.tsx (1 hunks)
- components/Chat/Message/MessageReactions.tsx (0 hunks)
- components/Chat/Message/MessageReactions/MessageReactions.tsx (1 hunks)
- components/Chat/Message/MessageReactions/MessageReactions.types.ts (1 hunks)
- components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactions.store.tsx (1 hunks)
- components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.tsx (1 hunks)
- components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.utils.ts (1 hunks)
- components/Chat/Message/MessageReactions/useMessageReactionsRolledup.ts (1 hunks)
- design-system/BottomSheet/BottomSheetBackdropOpacity.tsx (1 hunks)
- design-system/BottomSheet/BottomSheetModal.tsx (3 hunks)
💤 Files with no reviewable changes (1)
- components/Chat/Message/MessageReactions.tsx
✅ Files skipped from review due to trivial changes (1)
- components/Chat/Message/Message.tsx
🔇 Additional comments (16)
components/Chat/Message/MessageReactions/MessageReactions.types.ts (1)
3-5: LGTM! Clean and well-structured type definition.The
MessageReactionstype is well-designed:
- Uses appropriate index signature for dynamic sender addresses
- Maintains type safety with strongly typed array values
- Provides efficient lookup structure for reactions by sender
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactions.store.tsx (3)
1-4: LGTM! Clean and focused imports.The imports are minimal and properly typed with TypeScript.
5-7: LGTM! Well-defined interface.The interface is clear and properly typed with TypeScript.
1-13: Verify store usage across componentsThe store implementation looks good, but let's verify its integration with other components.
✅ Verification successful
Store integration verified and properly implemented
The store is correctly integrated within the MessageReactions feature:
- Properly imported and used in
MessageReactionsDrawer.tsxfor state management- Utility functions in
MessageReactionsDrawer.utils.tshandle state updates- Clear state lifecycle management with proper cleanup in drawer component
- Store usage is appropriately scoped to the MessageReactions feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how the store is being used across components # Look for store imports and usage rg -l "useMessageReactionsStore" --type tsx --type ts # Look for potential message reaction related components rg -l "MessageReactions" --type tsx --type tsLength of output: 155
Script:
#!/bin/bash # Search for store usage without file type restrictions rg -l "useMessageReactionsStore" # Search for MessageReactions components rg -l "MessageReactions" # Search for potential store usage patterns rg "useMessageReactionsStore" -A 3Length of output: 4349
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.utils.ts (2)
1-5: LGTM! Clean and well-organized imports.The imports are properly organized with external dependencies first, followed by internal imports.
6-6: Consider potential concurrency issues with singleton modal ref.The singleton
bottomSheetModalRefcould cause race conditions if multiple components attempt to open/close the modal simultaneously. Consider:
- Adding type information for better developer experience
- Using a ref-per-instance pattern if multiple modals might be needed
design-system/BottomSheet/BottomSheetBackdropOpacity.tsx (3)
7-8: LGTM: Theme hook import aligns with design system practices.The addition of useAppTheme hook follows the design system's pattern for consistent theming.
14-15: LGTM: Proper theme integration.The theme hook is correctly implemented and follows the singleton pattern.
21-29: Verify the scrim color usage across modals.The implementation looks correct, but let's ensure consistent scrim color usage across all modal implementations.
✅ Verification successful
Let's first identify the relevant files and then analyze their content in the next iteration.
Scrim color usage is consistent in modal backdrops
The verification shows that:
BottomSheetBackdropOpacity.tsxcorrectly usestheme.colors.background.scrimfor the backdropBottomSheetModal.tsxuses a different color (background.raised) but this is for the modal content, not the backdropBottomSheetModalBlurBackdrop.tsxdoesn't define any background color as it likely uses blur effectsNewConversationModal.tsxuses a navigation-specific background color for the header, not for the backdrop🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent scrim color usage across modal components # Expected: All modal backdrops should use theme.colors.background.scrim # Search for other modal backdrop color implementations rg -l "backgroundColor.*scrim" --type typescript --type typescript-react # Search for potential inconsistent backdrop color implementations in modal components rg -l "backgroundColor(?!.*scrim)" --type typescript --type typescript-react | rg -i "modal|backdrop|overlay"Length of output: 466
Script:
#!/bin/bash # Let's try a different approach to find modal and backdrop color implementations # Search for files containing modal/backdrop/overlay related components fd -e tsx -e ts | rg -i "modal|backdrop|overlay" # Then we can examine the content of these files for backgroundColor usageLength of output: 262
Script:
#!/bin/bash # Now let's examine the backgroundColor usage in these files echo "=== BottomSheetBackdropOpacity.tsx ===" rg "backgroundColor" design-system/BottomSheet/BottomSheetBackdropOpacity.tsx -A 2 -B 2 echo -e "\n=== BottomSheetModal.tsx ===" rg "backgroundColor" design-system/BottomSheet/BottomSheetModal.tsx -A 2 -B 2 echo -e "\n=== BottomSheetModalBlurBackdrop.tsx ===" rg "backgroundColor" design-system/BottomSheet/BottomSheetModalBlurBackdrop.tsx -A 2 -B 2 echo -e "\n=== NewConversationModal.tsx ===" rg "backgroundColor" screens/NewConversation/NewConversationModal.tsx -A 2 -B 2Length of output: 1121
design-system/BottomSheet/BottomSheetModal.tsx (2)
13-13: LGTM: Clean theme integrationThe theme integration is well-implemented, following design system best practices by using the useAppTheme hook.
Also applies to: 24-24
22-22: 🛠️ Refactor suggestionConsider adding explicit typing for backgroundStyle
The background styling implementation is solid, but could benefit from explicit typing.
Consider updating the interface definition:
export type IBottomSheetModalProps = GorhomBottomSheetModalProps & { absoluteHandleBar?: boolean; + backgroundStyle?: StyleProp<ViewStyle>; };Let's verify if this component is used consistently across the codebase:
Also applies to: 49-54
components/Chat/Message/MessageReactions/MessageReactions.tsx (1)
54-56:⚠️ Potential issueImprove emoji rendering and key usage.
- Using array indices as keys can lead to rendering issues if the array order changes.
- Direct rendering of emoji strings might not handle emoji sequences correctly.
Consider using a unique identifier for the key and a proper emoji rendering component:
- {rolledUpReactions.emojis.map((emoji, index) => ( - <Text key={index}>{emoji}</Text> - ))} + {rolledUpReactions.emojis.map((emoji) => ( + <Text key={`${message.id}-${emoji}`}>{emoji}</Text> + ))}components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.tsx (1)
35-38: LGTM! Clean and focused event handler implementation.The handler is properly memoized and handles state reset correctly.
components/Chat/Chat.tsx (3)
55-55: LGTM: Import statement for new reactions functionalityThe import statement is properly placed and follows the project's import organization pattern.
434-441: LGTM: Fragment usage for multiple root elementsThe switch from View to Fragment is appropriate here since we need to render both the chat container and the reactions drawer as siblings.
445-478: Verify scroll performance with AnimatedListView configurationThe AnimatedListView configuration includes several performance-critical settings:
estimatedItemSizeis set to 80maintainVisibleContentPositionis commented out due to Android glitches- Complex extraData dependencies
Consider:
- The commented maintainVisibleContentPosition could affect scroll behavior during keyboard show/hide
- The estimatedItemSize of 80 might need adjustment based on actual message sizes
Let's verify the message sizes in the codebase:
✅ Verification successful
Based on the code analysis so far, I can generate the final response since we have enough information about the message layout and styling patterns:
AnimatedListView configuration is appropriate for the message sizes
The current configuration is well-suited for the chat implementation:
- The
estimatedItemSize={80}is appropriate as messages have:
- Base content padding (vertical: 6px + horizontal: 13px)
- Standard message text size (17px)
- Margins for spacing between messages (~8px)
- Additional space for reactions and timestamps
- Disabling
maintainVisibleContentPositionon Android is correct due to the reported glitches- The
invertedlist with proper content insets handling is the standard pattern for chat UIs🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for message rendering logic to verify typical heights ast-grep --pattern 'style={$_}' components/Chat/Message/Message.tsxLength of output: 69
Script:
#!/bin/bash # Let's first check if the Message.tsx file exists and its content fd "Message.tsx" --type f --exec cat {} # Also search for style definitions related to message heights rg -l "styles\." --type ts components/Chat/Length of output: 28057
Script:
#!/bin/bash # Let's check the actual message styles and layout configuration rg "messageContentContainer|messageText|messageRow|innerBubble" components/Chat/Message/Message.tsx -A 2 # Also check the AnimatedListView implementation fd "AnimatedListView" --type f --exec cat {}Length of output: 1662
components/Chat/Message/MessageReactions/MessageReactions.types.ts
Outdated
Show resolved
Hide resolved
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactions.store.tsx
Outdated
Show resolved
Hide resolved
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.utils.ts
Outdated
Show resolved
Hide resolved
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.utils.ts
Outdated
Show resolved
Hide resolved
components/Chat/Message/MessageReactions/useMessageReactionsRolledup.ts
Outdated
Show resolved
Hide resolved
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.tsx
Outdated
Show resolved
Hide resolved
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.tsx
Outdated
Show resolved
Hide resolved
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.tsx
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,26 @@ | |||
| import { create } from "zustand"; | |||
There was a problem hiding this comment.
store to set the reactions that we want to look at in the reactions drawer
| @@ -0,0 +1,32 @@ | |||
| import { createBottomSheetModalRef } from "@design-system/BottomSheet/BottomSheet.utils"; | |||
There was a problem hiding this comment.
I love doing this where we only use the service functions in the React layer
| style={[ | ||
| { | ||
| backgroundColor: theme.colors.background.scrim, | ||
| }, |
There was a problem hiding this comment.
I've put your change at the design-system level because I think that's the styling we'll want to use for all our bottom sheet anyway
| backgroundStyle={[ | ||
| { | ||
| backgroundColor: theme.colors.background.raised, | ||
| }, | ||
| backgroundStyle, | ||
| ]} |
There was a problem hiding this comment.
I've put your change at the design-system level because I think that's the styling we'll want to use for all our bottom sheet anyway
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (5)
components/Chat/Message/MessageReactions/MessageReactions.types.ts (1)
3-5: Add JSDoc documentation for better type clarity.Consider adding a descriptive JSDoc comment to explain the purpose and structure of this type.
+/** + * Maps sender addresses to their message reactions. + * @example + * { + * "0x123...": [{ emoji: "👍", timestamp: 123456789 }] + * } + */ export type MessageReactions = { [senderAddress: string]: MessageReaction[]; };components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactions.store.tsx (1)
24-26: Add return type for clarityConsider adding an explicit return type to the reset function for better type safety.
-export const resetMessageReactionsStore = () => { +export const resetMessageReactionsStore = (): void => { useMessageReactionsStore.setState(initialMessageReactionsState); };components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.service.ts (2)
9-9: Consider implementing a singleton pattern for modal ref managementThe module-level
bottomSheetModalRefcould cause issues in testing environments or if multiple instances are needed. Consider implementing a singleton pattern or moving this to a React context.Example implementation:
class ModalManager { private static instance: ReturnType<typeof createBottomSheetModalRef>; public static getInstance() { if (!this.instance) { this.instance = createBottomSheetModalRef(); } return this.instance; } } export const getBottomSheetModalRef = () => ModalManager.getInstance();
18-24: Enhance type safety with proper TypeScript interfaceThe function's argument structure could benefit from a proper TypeScript interface definition.
Consider this improvement:
+interface CloseMessageReactionsDrawerOptions { + resetStore?: boolean; +} + -export function closeMessageReactionsDrawer(arg?: { resetStore?: boolean }) { +export function closeMessageReactionsDrawer(options?: CloseMessageReactionsDrawerOptions) { - const { resetStore = true } = arg ?? {}; + const { resetStore = true } = options ?? {}; bottomSheetModalRef.current?.dismiss(); if (resetStore) { resetMessageReactionsDrawer(); } }components/Chat/Message/MessageReactions/MessageReactions.tsx (1)
153-156: Consider adding secondary sort criteria to break ties in reaction orderingWhen reactions have the same number of reactors, their order may be inconsistent due to lack of a secondary sorting criterion. Adding
firstReactionTimeas a secondary sort key can ensure consistent ordering.Apply this diff to enhance the sorting logic:
.sort((a, b) => { const countDifference = b.reactors.length - a.reactors.length; if (countDifference !== 0) { return countDifference; } + // If counts are equal, sort by earliest reaction time + return a.firstReactionTime - b.firstReactionTime; })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- components/Chat/Message/MessageReactions/MessageReactions.tsx (1 hunks)
- components/Chat/Message/MessageReactions/MessageReactions.types.ts (1 hunks)
- components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactions.store.tsx (1 hunks)
- components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.service.ts (1 hunks)
- components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.tsx (1 hunks)
🔇 Additional comments (7)
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactions.store.tsx (2)
9-16: LGTM! Well-structured initial stateThe initial state is well-defined with appropriate default values for all properties.
1-26: Verify store usage patternsLet's verify the store implementation and usage across the codebase.
✅ Verification successful
Let me try a different approach to verify the store usage patterns.
Let me try one more time with a different approach using just the file extension without the type flag.
Store implementation follows best practices
The store implementation is well-structured and follows proper patterns:
- Store is correctly initialized with default state
- Usage is contained within the MessageReactionsDrawer service
- No direct subscriptions found that could cause memory leaks
- State updates are properly handled through setState
- Reset functionality is properly implemented
The store's usage is localized to the MessageReactionsDrawer feature, and there's no evidence of improper cleanup or memory leak concerns since no manual subscriptions are being used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify store usage patterns and potential issues # Check for store initialization and cleanup echo "Checking store initialization and cleanup patterns..." rg -l "useMessageReactionsStore\(" --type tsx # Check for potential memory leaks (store subscription cleanup) echo "Checking for proper cleanup in components using the store..." ast-grep --pattern 'useEffect(() => { $$$ const unsubscribe = useMessageReactionsStore.subscribe($$$) return () => { unsubscribe() } }, $$$)'Length of output: 479
Script:
#!/bin/bash # Search for store usage with ripgrep using ts and tsx extensions explicitly echo "Checking store usage patterns..." rg "useMessageReactionsStore" -t ts -t tsx -A 2 # Search for store reset calls echo -e "\nChecking reset usage..." rg "resetMessageReactionsStore" -t ts -t tsx -A 2 # Look for potential store subscriptions echo -e "\nChecking store subscriptions..." rg "useMessageReactionsStore\.subscribe" -t ts -t tsx -A 2 # Check if the store is used in any cleanup functions echo -e "\nChecking cleanup patterns..." rg "useMessageReactionsStore.*useEffect" -t ts -t tsx -A 5Length of output: 621
Script:
#!/bin/bash # Search for store usage echo "Checking store usage patterns..." rg "useMessageReactionsStore" --glob "*.{ts,tsx}" -A 2 # Search for store reset calls echo -e "\nChecking reset usage..." rg "resetMessageReactionsStore" --glob "*.{ts,tsx}" -A 2 # Look for potential store subscriptions echo -e "\nChecking store subscriptions..." rg "useMessageReactionsStore\.subscribe" --glob "*.{ts,tsx}" -A 2 # Check for the store file itself to understand its complete implementation echo -e "\nChecking store implementation..." rg "create.*IMessageReactionsStore" --glob "*.{ts,tsx}" -A 5Length of output: 3632
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.service.ts (1)
26-28: LGTM! Clean and focused implementationThe function follows the single responsibility principle and effectively resets the store state.
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.tsx (1)
19-25: LGTM! Clean hook implementation.The component is properly memoized and the hooks are well organized.
components/Chat/Message/MessageReactions/MessageReactions.tsx (3)
28-31: Add proper validation foruserAddressinstead of using non-null assertionThe current implementation uses a non-null assertion
userAddress!, which assumes the user is always logged in. This could lead to runtime errors ifuserAddressisundefined. Consider adding proper validation or handling the undefined case.
71-79: Enhance memoization to includereactionschangesThe current memoization only checks
messageproperties but doesn't compare thereactionsprop. This could lead to missed updates when only the reactions change.
101-102:⚠️ Potential issueVerify compatibility of
gapstyle property in React NativeThe
gapproperty used in theemojiContainerstyle may not be supported in React Native'sStyleSheet. This might lead to unexpected layout behavior on some platforms or versions.Run the following script to verify if
gapis supported and to check for custom implementations:✅ Verification successful
Let me gather more specific information about the codebase and its styling approach.
Based on the analysis of the codebase, I can now provide a final response:
The
gapproperty usage is safe in this React Native componentThe codebase is a React Native application, and the
gapproperty is being used within a style object that's processed through React Native'sStyleSheet.create(). The property is specifically used in conjunction withtheme.spacing.xxxs, which is part of the app's theming system, and is consistently used across multiple components in the codebase. The implementation shows proper handling of flexbox layouts with gap spacing.
- The property is used in
components/Chat/Message/MessageReactions/MessageReactions.tsxalong with other flexbox properties likeflexDirectionandflexWrap- Similar usage patterns are found across multiple components in the codebase
- The app uses React Native version 0.74.5 which supports the
gapproperty in flexbox layouts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of 'gap' in StyleSheet and its compatibility. # Search for 'gap' usage in style definitions rg 'gap:' --type js # Check if a polyfill or custom implementation for 'gap' is present rg -i 'gap|spacing|style' --type jsLength of output: 106
Script:
#!/bin/bash # Check for styling system and theme implementation fd -e ts -e tsx -e js -e jsx | xargs rg -l "theme\.spacing|createStyles|StyleSheet" # Look for specific style implementations in MessageReactions fd "MessageReactions" | xargs cat # Check if this is a React Native project fd "package.json" -x cat {}Length of output: 34708
components/Chat/Message/MessageReactions/MessageReactions.types.ts
Outdated
Show resolved
Hide resolved
| export const useMessageReactionsStore = create<IMessageReactionsStore>( | ||
| (set, get) => ({ | ||
| ...initialMessageReactionsState, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Implement missing store methods
The store is missing implementations for state management methods, and there's an unused 'get' parameter.
Apply these changes:
export const useMessageReactionsStore = create<IMessageReactionsStore>(
- (set, get) => ({
+ (set) => ({
...initialMessageReactionsState,
+ setRolledUpReactions: (reactions) => set({ rolledUpReactions: reactions }),
+ updateReactions: (updates) =>
+ set((state) => ({
+ rolledUpReactions: { ...state.rolledUpReactions, ...updates }
+ })),
})
);Committable suggestion was skipped due to low confidence.
| export interface IMessageReactionsStore { | ||
| rolledUpReactions: RolledUpReactions; | ||
| } |
There was a problem hiding this comment.
Enhance store interface with state management methods
The interface needs methods for managing reactions state. This aligns with Zustand's best practices for type-safe state management.
Add the following methods to the interface:
export interface IMessageReactionsStore {
rolledUpReactions: RolledUpReactions;
+ setRolledUpReactions: (reactions: RolledUpReactions) => void;
+ updateReactions: (updates: Partial<RolledUpReactions>) => void;
}📝 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 interface IMessageReactionsStore { | |
| rolledUpReactions: RolledUpReactions; | |
| } | |
| export interface IMessageReactionsStore { | |
| rolledUpReactions: RolledUpReactions; | |
| setRolledUpReactions: (reactions: RolledUpReactions) => void; | |
| updateReactions: (updates: Partial<RolledUpReactions>) => void; | |
| } |
...nents/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.service.ts
Show resolved
Hide resolved
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.tsx
Show resolved
Hide resolved
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.tsx
Show resolved
Hide resolved
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.tsx
Outdated
Show resolved
Hide resolved
| @@ -40,8 +43,15 @@ export const BottomSheetModal = memo( | |||
| containerComponent={ | |||
| Platform.OS === "ios" ? renderContainerComponent : undefined | |||
| } | |||
| enableDynamicSizing={false} // By default we don't want enable dynamic sizing | |||
There was a problem hiding this comment.
This was the fix to stop the bottom sheet from going to a snapPoint that didn't exist
There was a problem hiding this comment.
@thierryskoda good catch! I initially preferred a dynamic height to let the drawer expand with reactions, but I'm fine with a fixed height! :)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- components/Chat/Message/MessageReactions/MessageReactions.tsx (1 hunks)
- components/Chat/Message/MessageReactions/MessageReactions.types.ts (1 hunks)
- components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactions.store.tsx (1 hunks)
- components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.service.ts (1 hunks)
- components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/Chat/Message/MessageReactions/MessageReactions.types.ts
- components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactions.store.tsx
- components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.tsx
🔇 Additional comments (7)
components/Chat/Message/MessageReactions/MessageReactions.tsx (1)
30-30: Previous review comment about userAddress validation is still applicable.The non-null assertion operator is still being used which could lead to runtime errors.
components/Chat/Message/MessageReactions/MessageReactionsDrawer/MessageReactionsDrawer.service.ts (6)
1-7: LGTM!The imports are organized correctly and the necessary modules are imported for the service functionality.
9-9: LGTM!The
bottomSheetModalRefis properly initialized usingcreateBottomSheetModalRef().
11-24: LGTM!The
openMessageReactionsDrawerfunction correctly sets the rolled-up reactions state before presenting the modal. The use of error handling ensures robustness if the modal reference is not initialized.
26-32: LGTM!The
closeMessageReactionsDrawerfunction effectively dismisses the modal and resets the store based on the provided argument, accommodating scenarios where the store reset may not be desired.
34-36: LGTM!The
resetMessageReactionsDrawerfunction appropriately resets the message reactions store, providing a clear separation of concerns.
38-40: LGTM!The
useMessageReactionsRolledUpReactionshook provides access to therolledUpReactionsstate efficiently.
@lourou Made the modal work. Will put some comments inline with my code to show you the fixes.
Also did some refactoring hope you don't mind! It's not 100% perfect but I thought this could cleaner. Let me know what you think!
There's still some refactoring to do with styling and other stuff but it's a good start (I think) :)
Summary by CodeRabbit
Release Notes
New Features
MessageReactionsDrawerfor displaying reactions in a bottom sheet format.ChatMessageReactionscomponent for aggregating and displaying message reactions.Bug Fixes
Improvements
BottomSheetModalandBottomSheetBackdropOpacitycomponents for improved styling capabilities.