-
Notifications
You must be signed in to change notification settings - Fork 2
BA-2111-FE-edit-existing-message #188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
Warning Rate limit exceeded@Ronan-Fernandes has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThe pull request introduces several enhancements across multiple components and modules. Key changes include adding a popover menu to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (11)
packages/design-system/components/icons/DownloadIcon/index.tsx (1)
5-28: LGTM! Consider adding JSDoc documentation.The implementation follows best practices with:
- Proper typing using FC
- Consistent styling with design system
- Theme-aware color implementation using currentColor
- Proper prop spreading for customization
Consider adding JSDoc documentation to describe the component's purpose and usage:
+/** + * DownloadIcon - A download icon component that follows the design system specifications. + * @param {SvgIconProps} props - Material-UI SvgIcon props for customization + */ const DownloadIcon: FC<SvgIconProps> = ({ sx, ...props }) => (packages/design-system/components/icons/CopyIcon/index.tsx (1)
5-46: LGTM! Consider adding JSDoc documentation for consistency.The implementation follows the same best practices as DownloadIcon:
- Proper typing using FC
- Consistent styling with design system
- Theme-aware color implementation
- Proper prop spreading
Consider adding JSDoc documentation to match DownloadIcon:
+/** + * CopyIcon - A copy icon component that follows the design system specifications. + * @param {SvgIconProps} props - Material-UI SvgIcon props for customization + */ const CopyIcon: FC<SvgIconProps> = ({ sx, ...props }) => (packages/components/modules/messages/MessageUpdate/types.ts (1)
6-11: Consider adding JSDoc comments and runtime validation.The interface is well-structured, but could benefit from:
- JSDoc comments explaining the purpose of each prop
- Runtime validation for the message prop to ensure required fields are present
Example JSDoc and validation:
import { z } from 'zod' /** * Props for the MessageUpdate component * @property message - The message data to be updated * @property onCancel - Callback when update is cancelled * @property SocialInput - Optional custom input component * @property SocialInputProps - Props for the custom input */ export interface MessageUpdateProps { message: MessageItemFragment$data onCancel: () => void SocialInput?: FC<SocialInputProps> SocialInputProps?: Partial<SocialInputProps> } // Runtime validation schema export const messageUpdatePropsSchema = z.object({ message: z.object({ id: z.string(), content: z.string(), }).passthrough(), onCancel: z.function(), SocialInput: z.function().optional(), SocialInputProps: z.record(z.any()).optional(), })packages/components/modules/__shared__/UpdateSubmitActions/index.tsx (1)
Line range hint
14-21: Verify aria-label values are descriptive enoughWhile the implementation looks good, consider making the default aria-labels more descriptive:
- 'save comment edit' → 'Save edited content'
- 'cancel comment edit' → 'Cancel editing'
packages/components/modules/__shared__/ActionsOverlay/types.ts (1)
30-30: Consider a more descriptive prop nameThe prop name
useTreedotsMenuOverlaycould be more descriptive. Consider alternatives like:
usePopoverMenurenderAsPopoveruseMenuOverlaypackages/components/modules/messages/graphql/mutations/MessageUpdate.ts (1)
37-43: Consider handling empty errors arrayThe error handling looks good, but consider adding a success toast notification when there are no errors.
onCompleted: (response, errors) => { - errors?.forEach((error) => { - sendToast(error.message, { type: 'error' }) - }) + if (errors?.length) { + errors.forEach((error) => { + sendToast(error.message, { type: 'error' }) + }) + } else { + sendToast('Message updated successfully', { type: 'success' }) + }packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx (2)
48-54: Implement copy message functionality securelyThe copy message functionality needs implementation. Consider using the Clipboard API with proper error handling:
const copyMessage = async () => { try { await navigator.clipboard.writeText(message?.content || ''); sendToast('Message copied to clipboard', { type: 'success' }); } catch (error) { sendToast('Failed to copy message', { type: 'error' }); } };Would you like me to help implement this feature?
32-37: Consider using design system typography stylesInstead of inline styles, consider defining these typography styles in the design system for consistency:
-sx={{ - maxWidth: '100%', - whiteSpace: 'pre-wrap', - wordBreak: 'normal', - overflowWrap: 'anywhere', -}} +variant="message"packages/components/modules/messages/MessageUpdate/index.tsx (1)
124-130: Consider debouncing the focus effect.The useEffect for input focus might trigger unnecessarily. Consider debouncing or adding proper dependencies.
useEffect(() => { + const timer = setTimeout(() => { if (inputRef.current) { const { length } = inputRef.current.value inputRef.current.setSelectionRange(length, length) inputRef.current.focus() } + }, 100) + return () => clearTimeout(timer) }, [inputRef])packages/components/modules/__shared__/ActionsOverlay/index.tsx (2)
193-202: Consider extracting styles to styled components.The inline styles for the popover container could be moved to a styled component for better maintainability.
+const PopoverContainer = styled(ActionOverlayTooltipContainer)` + position: unset; + margin-left: 4px; + margin-right: 4px; + border-radius: 500px; + align-self: baseline; +` {useTreedotsMenuOverlay - ? { - sx: { - position: 'unset', - marginX: '4px', - borderRadius: '500px', - alignSelf: 'baseline', - }, - } + ? { component: PopoverContainer } : {}}
283-283: Consider adding a delay to the close handler.The immediate close on mouse leave might be too aggressive. Consider adding a small delay.
- onMouseLeave={handleClosePopover} + onMouseLeave={() => { + const timer = setTimeout(handleClosePopover, 150) + return () => clearTimeout(timer) + }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 8b54883 and 12ec533e358b4618ed03e9a2e4215d04b4af13a7.
⛔ Files ignored due to path filters (2)
packages/components/__generated__/MembersListFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/MessageUpdateMutation.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (16)
packages/components/modules/__shared__/ActionsOverlay/index.tsx(9 hunks)packages/components/modules/__shared__/ActionsOverlay/types.ts(1 hunks)packages/components/modules/__shared__/SocialInput/SubmitActions/types.ts(1 hunks)packages/components/modules/__shared__/SocialInput/styled.tsx(1 hunks)packages/components/modules/__shared__/UpdateSubmitActions/index.tsx(2 hunks)packages/components/modules/__shared__/UpdateSubmitActions/types.ts(1 hunks)packages/components/modules/__shared__/index.ts(1 hunks)packages/components/modules/comments/CommentUpdate/index.tsx(3 hunks)packages/components/modules/messages/MessageUpdate/index.tsx(1 hunks)packages/components/modules/messages/MessageUpdate/types.ts(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx(2 hunks)packages/components/modules/messages/graphql/mutations/MessageUpdate.ts(1 hunks)packages/components/schema.graphql(7 hunks)packages/design-system/components/icons/CopyIcon/index.tsx(1 hunks)packages/design-system/components/icons/DownloadIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(1 hunks)
🔇 Additional comments (12)
packages/components/modules/__shared__/index.ts (1)
12-13: LGTM! Good job generalizing the component.The renaming from
CommentUpdateSubmitActionstoUpdateSubmitActionsimproves reusability by making it more generic. The addition of thecancelAriaLabelprop also shows good attention to accessibility.Let's verify the component renaming impact:
✅ Verification successful
Renaming verification successful - all references updated
The component has been successfully renamed from
CommentUpdateSubmitActionstoUpdateSubmitActionswith no lingering references to the old name. The new component is properly used in bothMessageUpdateandCommentUpdatecomponents.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old component name # and verify the new component usage # Check for any remaining references to the old name echo "Checking for old component name references..." rg "CommentUpdateSubmitActions" # Check new component usage echo "Checking new component usage..." rg "UpdateSubmitActions"Length of output: 1578
packages/design-system/components/icons/index.ts (1)
10-11: LGTM! New icon exports follow the established patterns.The new exports for
CopyIconandDownloadIconmaintain consistency with:
- Default export pattern
- Alphabetical ordering
packages/components/modules/__shared__/UpdateSubmitActions/types.ts (1)
1-7: LGTM! Good accessibility improvement.The interface renaming makes it more generic and reusable. The addition of
cancelAriaLabelimproves accessibility by providing screen reader context for the cancel action.packages/components/modules/__shared__/SocialInput/SubmitActions/types.ts (1)
Line range hint
1-14: LGTM! Good consistency across interfaces.The addition of
cancelAriaLabelmaintains consistency with theUpdateSubmitActionsinterface while preserving existing icon customization capabilities.packages/components/modules/__shared__/SocialInput/styled.tsx (1)
Line range hint
3-12: Verify layout implications of full-width form.Setting
width: 100%ensures the form fills its container, but please verify:
- Parent containers can accommodate full width
- No horizontal scrollbars appear in responsive layouts
- Sticky positioning works as expected with the new width
✅ Verification successful
Full-width form implementation is safe and well-tested
The
width: 100%implementation is verified to be safe because:
- Component is part of the design system with standardized layout handling
- Successfully used in multiple contexts (messages, comments, replies)
- Sticky positioning is intentional for maintaining form visibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for parent containers that might be affected by full-width form rg -l "SocialInput.*Form" | xargs rg -A 5 "styled|width|max-width"Length of output: 68
Script:
#!/bin/bash # Search for SocialInput component usage and imports echo "=== SocialInput Usage ===" rg -l "SocialInput" echo -e "\n=== Form Imports ===" rg "import.*Form.*SocialInput" echo -e "\n=== Related Styled Components ===" fd "styled.tsx" --exec cat {} \; --exec echo -e "\n---\n" \; --base-directory packages/components/modules/__shared__/SocialInput/Length of output: 2106
Script:
#!/bin/bash # Check implementation details echo "=== SocialInputWithForm Implementation ===" cat packages/components/modules/__shared__/SocialInput/__storybook__/SocialInputWithForm/index.tsx echo -e "\n=== Main Component Implementation ===" cat packages/components/modules/__shared__/SocialInput/index.tsx echo -e "\n=== Storybook Stories ===" cat packages/components/modules/__shared__/SocialInput/__storybook__/stories.tsxLength of output: 5651
packages/components/modules/__shared__/UpdateSubmitActions/index.tsx (1)
7-11: Good generalization of the component name and props!The renaming from
CommentUpdateSubmitActionstoUpdateSubmitActionsmakes the component more reusable. The addition ofcancelAriaLabelprop improves accessibility.packages/components/modules/messages/graphql/mutations/MessageUpdate.ts (1)
1-51: Add unit tests for the mutation hookPlease add unit tests to cover:
- Successful mutation
- GraphQL error handling
- Network error handling
- Toast notifications
Would you like me to help generate the test cases?
packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx (1)
72-75: Implement delete message functionalityThe delete message functionality is missing. Consider adding:
- Confirmation dialog before deletion
- Loading state during deletion
- Error handling
Would you like me to help implement the delete functionality with these considerations?
packages/components/schema.graphql (3)
106-106: LGTM! Good addition of participant count.Adding a non-nullable
participantsCountfield to theChatRoomtype is a good practice for performance optimization, as it avoids the need to count participants on the client side.
166-178: LGTM! Well-structured message editing types.The
ChatRoomEditMessageInputandChatRoomEditMessagePayloadtypes follow GraphQL best practices:
- Input includes required fields (
id,content)- Payload includes error handling
- Proper use of Relay edge pattern for the updated message
221-228: LGTM! Good use of enumeration for roles.Converting the role field from String to
ChatRoomParticipantRolesenum improves type safety and documentation.packages/components/modules/comments/CommentUpdate/index.tsx (1)
11-11: LGTM! Good component reuse.The refactoring to use the shared
UpdateSubmitActionscomponent improves code reuse and maintainability.Also applies to: 69-69, 140-146
packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider implementing proper error handling.
The TODO comment indicates missing error handling. Consider implementing a proper error handling strategy.
- // TODO: handle errors
- console.error(errors)
- return
+ const errorMessage = errors.map(error => error.message).join(', ')
+ // Using your error handling system, e.g.:
+ showErrorNotification(errorMessage)
+ returnCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement proper error handling for mutation errors.
Similar to the previous comment, implement proper error handling for mutation errors instead of just logging to console.
- // TODO: handle errors
- onError: console.error,
+ onError: (error) => {
+ // Using your error handling system, e.g.:
+ showErrorNotification(error.message)
+ },Committable suggestion skipped: line range outside the PR's diff.
packages/components/modules/__shared__/ActionsOverlay/index.tsx
Outdated
Show resolved
Hide resolved
tsl-ps2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Some more comments:
- The designs have a "Edited" label for messages. Was it part of this story to implement it?
- If I remember correctly, the edit time out was on a different story, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo Threedots?
packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx
Outdated
Show resolved
Hide resolved
12ec533 to
9c54e3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/components/modules/messages/MessageUpdate/index.tsx (1)
102-104:⚠️ Potential issueImplement proper error handling.
The error handling is currently logging to console. Consider implementing proper error handling using a notification system.
packages/components/modules/__shared__/ActionsOverlay/index.tsx (1)
240-270: 🛠️ Refactor suggestionEnhance accessibility for popover menu.
The popover menu lacks proper keyboard navigation support.
🧹 Nitpick comments (5)
packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx (2)
52-52: Implement copy message functionality.The copy message action is currently a TODO. Consider implementing it using the Clipboard API.
Would you like me to provide an implementation for the copy message functionality?
88-88: Consider width handling for edit mode.The width style is only applied in edit mode. This could cause layout shifts when toggling between view/edit modes.
Consider maintaining consistent width in both modes:
-{...(isEditMode && { sx: { width: '100%' } })} +sx={{ width: '100%' }}packages/components/modules/messages/MessageUpdate/index.tsx (1)
124-130: Add error handling for input focus.The input focus logic doesn't handle cases where the ref is null or the input is not available.
Add error handling:
useEffect(() => { - if (inputRef.current) { + try { + if (!inputRef.current) return; const { length } = inputRef.current.value inputRef.current.setSelectionRange(length, length) inputRef.current.focus() + } catch (error) { + console.warn('Failed to focus input:', error) + } - } }, [inputRef])packages/components/schema.graphql (2)
166-170: Consider adding validation constraints.The input type could benefit from validation constraints on the content field.
Consider adding a maxLength directive or similar constraint to prevent extremely long messages:
input ChatRoomEditMessageInput { id: ID! content: String! @constraint(maxLength: 1000) clientMutationId: String }
172-178: Add field for tracking edit history.The payload type could include a field for tracking edit history or metadata.
Consider adding fields to track edit timestamps and history:
type ChatRoomEditMessagePayload { errors: [ErrorType] _debug: DjangoDebug message: MessageEdge clientMutationId: String editMetadata: EditMetadata } type EditMetadata { editedAt: DateTime! previousContent: String editCount: Int! }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 12ec533e358b4618ed03e9a2e4215d04b4af13a7 and 9c54e3cd2ddaf5268743ea26de943032ca164dcf.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/MessageUpdateMutation.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (16)
packages/components/modules/__shared__/ActionsOverlay/index.tsx(9 hunks)packages/components/modules/__shared__/ActionsOverlay/types.ts(1 hunks)packages/components/modules/__shared__/SocialInput/SubmitActions/types.ts(1 hunks)packages/components/modules/__shared__/SocialInput/styled.tsx(1 hunks)packages/components/modules/__shared__/UpdateSubmitActions/index.tsx(2 hunks)packages/components/modules/__shared__/UpdateSubmitActions/types.ts(1 hunks)packages/components/modules/__shared__/index.ts(1 hunks)packages/components/modules/comments/CommentUpdate/index.tsx(3 hunks)packages/components/modules/messages/MessageUpdate/index.tsx(1 hunks)packages/components/modules/messages/MessageUpdate/types.ts(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx(2 hunks)packages/components/modules/messages/graphql/mutations/MessageUpdate.ts(1 hunks)packages/components/schema.graphql(2 hunks)packages/design-system/components/icons/CopyIcon/index.tsx(1 hunks)packages/design-system/components/icons/DownloadIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/components/modules/shared/SocialInput/styled.tsx
- packages/components/modules/shared/SocialInput/SubmitActions/types.ts
- packages/components/modules/shared/ActionsOverlay/types.ts
- packages/components/modules/shared/UpdateSubmitActions/types.ts
- packages/components/modules/shared/index.ts
- packages/design-system/components/icons/CopyIcon/index.tsx
- packages/components/modules/comments/CommentUpdate/index.tsx
- packages/design-system/components/icons/index.ts
- packages/components/modules/messages/MessageUpdate/types.ts
- packages/components/modules/shared/UpdateSubmitActions/index.tsx
- packages/components/modules/messages/graphql/mutations/MessageUpdate.ts
- packages/design-system/components/icons/DownloadIcon/index.tsx
🔇 Additional comments (2)
packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx (1)
59-61: Consider adding validation before entering edit mode.The edit action directly sets edit mode without any validation. Consider checking if the message is editable (e.g., not too old, not deleted).
packages/components/modules/__shared__/ActionsOverlay/index.tsx (1)
34-34: Fix typo in prop name.The prop name
useTreedotsMenuOverlaycontains a typo. It should beuseThreeDotsMenuOverlay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add loading state to prevent double submission.
While there's a check for isMutationInFlight, the form isn't disabled during submission.
Add form-level disabled state:
const onSubmit = async (data: SocialUpsertForm) => {
if (isMutationInFlight) return
+ form.setIsSubmitting(true)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was envisioning this change to better separate concerns between different variations. so it's easier to add different possibilities later on (or to improve customization)
const ActionsOverlay = forwardRef<HTMLDivElement, ActionOverlayProps>(
...
if (deviceHasHover && isHoveringItem) {
if (hoverOverlayMode === "default") return <DefaultHoverOverlay offsetRight={offsetRight} offsetTop={offsetTop} enableDelete={enableDelete} handleLongPressItemOptionsClose={handleLongPressItemOptionsClose} />
if (hoverOverlayMode === "three-dots-menu") return <ThreeDotsMenuHoverOverlay offsetRight={offsetRight} offsetTop={offsetTop} enableDelete={enableDelete} handleLongPressItemOptionsClose={handleLongPressItemOptionsClose} />
...
essentially we would just move both (the default and the new three dots one) hover overlays for desktop devices into separate components so it's cleaner
another option would be adding an overridable component on top so we could allow customization (maybe we can do this in the future)
const ActionsOverlay = forwardRef<HTMLDivElement, ActionOverlayProps>(
{HoverOverlayComponent = DefaultHoverOverlay}
...
if (deviceHasHover && isHoveringItem) {
// HoverOverlayComponent would be just a wrapper that renders DefaultHoverOverlay or ThreeDotsMenuHoverOverlay depending on the hoverOverlayMode
return <HoverOverlayComponent offsetRight={offsetRight} offsetTop={offsetTop} enableDelete={enableDelete} handleLongPressItemOptionsClose={handleLongPressItemOptionsClose} />
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we apply the suggestion of creating a ThreeDotsMenuHoverOverlay then all customization logic would be there, we would only need to pass hoverOverlayMode === "three-dots-menu"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need at least the flexDirection.
because this style is specific to the messages and changes according to the isOwnMessage variable
packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be cool to update the storybook to add this new mode
packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsx
9c54e3c to
faf424d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/components/modules/__shared__/ActionsOverlay/index.tsx (1)
46-51: 🛠️ Refactor suggestionAdd keyboard navigation support for popover.
The popover should support keyboard navigation for better accessibility.
+const handleKeyDown = (e: React.KeyboardEvent) => { + if (e.key === 'Escape') { + handleClosePopover() + } +}
🧹 Nitpick comments (10)
packages/components/modules/__shared__/ActionsOverlay/ThreeDotsMenuHoverOverlay/types.ts (1)
12-17: Consider improving popover type definition.The popover configuration could be enhanced for better reusability and type safety:
- Extract popover config to a separate reusable interface
- Consider using a more specific type than
HTMLElementforopenpropertyExample refactor:
+ interface PopoverConfig { + open: HTMLButtonElement | null // or another specific element type + onOpen: (event: MouseEvent<HTMLElement>) => void + onClose: () => void + setOpen: Dispatch<SetStateAction<HTMLButtonElement | null>> + } interface ThreeDotsMenuHoverOverlayProps extends Pick< ActionOverlayProps, 'actions' | 'offsetRight' | 'offsetTop' | 'enableDelete' | 'isDeletingItem' > { handleDeleteDialogOpen: () => void handleClosePopover: () => void - popover: { - open: HTMLElement | null - onOpen: (event: MouseEvent<HTMLElement>) => void - onClose: () => void - setOpen: Dispatch<SetStateAction<HTMLElement | null>> - } + popover: PopoverConfig }packages/components/modules/__shared__/ActionsOverlay/DefaultHoverOverlay/index.tsx (3)
31-33: Add key prop when returning null in map.When mapping over actions and returning null, a key prop should still be provided to help React's reconciliation process.
- if (!hasPermission) return null + if (!hasPermission) return null as JSX.Element
34-39: Consider memoizing the click handler.The
handleClickfunction is recreated on every render. Consider usinguseCallbackto memoize it.+ const handleClick = useCallback(() => { - onClick?.() - if (closeOnClick) { - handleLongPressItemOptionsClose() - } + onClick?.() + if (closeOnClick) { + handleLongPressItemOptionsClose() + } + }, [onClick, closeOnClick, handleLongPressItemOptionsClose])
8-48: Consider performance optimization opportunities.The component could benefit from performance optimizations:
- Memoize the component using
React.memo- Memoize the actions map using
useMemo- const DefaultHoverOverlay: FC<DefaultHoverOverlayProps> = ({ + const DefaultHoverOverlay: FC<DefaultHoverOverlayProps> = React.memo(({ // ... props - }) => ( + }) => { + const renderedActions = useMemo(() => + actions?.map(({ label, icon, onClick, disabled, hasPermission, closeOnClick }) => { + // ... existing map logic + }), + [actions, handleLongPressItemOptionsClose] + ) + + return ( <ActionOverlayTooltipContainer offsetRight={offsetRight} offsetTop={offsetTop} aria-label="actions overlay" > {enableDelete && ( <IconButton onClick={handleDeleteDialogOpen} disabled={isDeletingItem} aria-label="delete item" > <TrashCanIcon /> </IconButton> )} - {actions?.map(/* ... */)} + {renderedActions} </ActionOverlayTooltipContainer> - ) + )} + ))packages/components/modules/__shared__/ActionsOverlay/ThreeDotsMenuHoverOverlay/index.tsx (2)
24-29: Consider using theme spacing for consistency.Replace hardcoded margin and border radius values with theme spacing units for better maintainability and consistency.
sx={{ position: 'unset', - marginX: '4px', - borderRadius: '500px', + marginX: 0.5, + borderRadius: (theme) => theme.shape.borderRadius * 6, alignSelf: 'baseline', }}
31-33: Add tooltip for better accessibility.Consider adding a tooltip to the three dots icon button to improve user experience, especially for users with assistive technologies.
- <IconButton onClick={popover.onOpen} aria-label="Show menu options"> - <ThreeDotsIcon sx={{ fontSize: '18px' }} /> - </IconButton> + <Tooltip title="Show menu options"> + <IconButton onClick={popover.onOpen} aria-label="Show menu options"> + <ThreeDotsIcon sx={{ fontSize: '18px' }} /> + </IconButton> + </Tooltip>packages/components/modules/__shared__/ActionsOverlay/index.tsx (2)
15-17: Consider updating imports to use a barrel file.The new overlay components could be imported from a barrel file (e.g.,
./overlays/index.ts) to improve code organization and maintainability.
232-238: Consider using styled components for conditional styles.Instead of using inline styles with conditional spreading, consider using styled components for better maintainability and reusability.
+const Container = styled(Box)<{ isThreeDotsMenu: boolean }>` + ${({ isThreeDotsMenu }) => + isThreeDotsMenu && + css` + width: 100%; + display: flex; + justify-content: flex-end; + `} +` -<Box +<Container ref={ref} onMouseEnter={() => setIsHoveringItem(true)} onMouseLeave={handleClosePopover} position="relative" {...longPressHandlers()} - {...(hoverOverlayMode === HOVER_OVERLAY_MODES.THREE_DOTS_MENU && { - sx: { - width: '100%', - display: 'flex', - justifyContent: 'flex-end', - }, - })} + isThreeDotsMenu={hoverOverlayMode === HOVER_OVERLAY_MODES.THREE_DOTS_MENU} {...ContainerProps} ->packages/components/schema.graphql (2)
166-170: Consider adding validation constraints to the input type.The
ChatRoomEditMessageInputtype could benefit from validation constraints:
- Add a maximum length constraint for the
contentfield- Consider making
clientMutationIdnon-nullable for better traceabilityinput ChatRoomEditMessageInput { id: ID! - content: String! + content: String! @constraint(maxLength: 1000) - clientMutationId: String + clientMutationId: String! }
172-178: Consider adding pagination to the message edge.The
ChatRoomEditMessagePayloadreturns a single message edge. Consider adding pagination support for consistency with other similar types in the schema.type ChatRoomEditMessagePayload { """May contain more than one error for same field.""" errors: [ErrorType] _debug: DjangoDebug - message: MessageEdge + message: MessageConnection clientMutationId: String }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 9c54e3cd2ddaf5268743ea26de943032ca164dcf and faf424d79431bdf3648d80987c54db8b0951ee1f.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/MessageUpdateMutation.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (22)
packages/components/modules/__shared__/ActionsOverlay/DefaultHoverOverlay/index.tsx(1 hunks)packages/components/modules/__shared__/ActionsOverlay/DefaultHoverOverlay/types.ts(1 hunks)packages/components/modules/__shared__/ActionsOverlay/ThreeDotsMenuHoverOverlay/index.tsx(1 hunks)packages/components/modules/__shared__/ActionsOverlay/ThreeDotsMenuHoverOverlay/types.ts(1 hunks)packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsx(2 hunks)packages/components/modules/__shared__/ActionsOverlay/constants.ts(1 hunks)packages/components/modules/__shared__/ActionsOverlay/index.tsx(8 hunks)packages/components/modules/__shared__/ActionsOverlay/types.ts(2 hunks)packages/components/modules/__shared__/SocialInput/SubmitActions/types.ts(1 hunks)packages/components/modules/__shared__/SocialInput/styled.tsx(1 hunks)packages/components/modules/__shared__/UpdateSubmitActions/index.tsx(2 hunks)packages/components/modules/__shared__/UpdateSubmitActions/types.ts(1 hunks)packages/components/modules/__shared__/index.ts(1 hunks)packages/components/modules/comments/CommentUpdate/index.tsx(3 hunks)packages/components/modules/messages/MessageUpdate/index.tsx(1 hunks)packages/components/modules/messages/MessageUpdate/types.ts(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx(2 hunks)packages/components/modules/messages/graphql/mutations/MessageUpdate.ts(1 hunks)packages/components/schema.graphql(2 hunks)packages/design-system/components/icons/CopyIcon/index.tsx(1 hunks)packages/design-system/components/icons/DownloadIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/design-system/components/icons/index.ts
- packages/components/modules/shared/SocialInput/SubmitActions/types.ts
- packages/components/modules/shared/SocialInput/styled.tsx
- packages/components/modules/comments/CommentUpdate/index.tsx
- packages/components/modules/shared/UpdateSubmitActions/types.ts
- packages/design-system/components/icons/DownloadIcon/index.tsx
- packages/components/modules/messages/MessageUpdate/types.ts
- packages/design-system/components/icons/CopyIcon/index.tsx
- packages/components/modules/shared/ActionsOverlay/types.ts
- packages/components/modules/shared/UpdateSubmitActions/index.tsx
- packages/components/modules/shared/index.ts
- packages/components/modules/messages/graphql/mutations/MessageUpdate.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Component Test Applications and Packages
- GitHub Check: Lint
🔇 Additional comments (11)
packages/components/modules/__shared__/ActionsOverlay/constants.ts (1)
1-4: LGTM! Well-structured constant definition.The
HOVER_OVERLAY_MODESconstant is well-defined with clear, descriptive mode names and proper type safety usingas const.packages/components/modules/__shared__/ActionsOverlay/DefaultHoverOverlay/types.ts (1)
3-10: LGTM! Well-structured TypeScript interface.The interface is well-designed with:
- Proper use of Pick utility type for type safety
- Clear handler method signatures
- Appropriate prop selection from parent interface
packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsx (1)
41-68: LGTM! Well-structured story implementation.The story provides a clear example of the ThreeDotsMenu variant with appropriate actions and icons.
packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx (2)
66-72: Add loading state for download action.The download attachments action should have a loading state to prevent multiple clicks.
22-22: Consider limiting concurrent edit modes.Based on the previous discussion in PR comments, while allowing multiple messages in edit mode is acceptable, it might be worth tracking this as a potential future improvement for better UX.
✅ Verification successful
Current implementation follows established pattern
The codebase consistently uses independent edit mode states for messages and comments, which aligns with the current acceptance of multiple concurrent edits. The suggestion to track this as a future improvement remains valid but doesn't require immediate changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing implementations of edit mode limitations rg -l "isEditMode.*useState.*false" --type ts --type tsxLength of output: 86
Script:
#!/bin/bash # Search for edit mode related state management with proper file types rg -t typescript -t javascriptreact -t typescriptreact "isEditMode.*useState" -A 2 # Search for any global edit mode management rg -t typescript -t javascriptreact -t typescriptreact "(editMode|isEditing).*context" -A 2 # Look for components potentially managing multiple edit states rg -t typescript -t javascriptreact -t typescriptreact "const.*\[.*(editMode|isEditing).*\].*useState" -A 2Length of output: 393
Script:
#!/bin/bash # Search for edit mode state management rg "isEditMode.*useState|const.*editMode.*useState" --type-add 'ts:*.{ts,tsx}' -t ts -A 2 # Look for any global state or context related to edit modes rg "editMode.*Context|isEditing.*Context" --type-add 'ts:*.{ts,tsx}' -t ts -A 2 # Search for components managing edit states rg "const.*\[.*edit.*\].*useState" --type-add 'ts:*.{ts,tsx}' -t ts -A 2Length of output: 899
packages/components/modules/messages/MessageUpdate/index.tsx (3)
102-104: Consider implementing proper error handling.The TODO comment indicates missing error handling. Consider implementing a proper error handling strategy.
114-115: Implement proper error handling for mutation errors.Similar to the previous comment, implement proper error handling for mutation errors instead of just logging to console.
90-92: Add loading state to prevent double submission.While there's a check for
isMutationInFlight, the form isn't disabled during submission.packages/components/modules/__shared__/ActionsOverlay/index.tsx (2)
35-35: Consider extracting hover overlay rendering logic.The hover overlay rendering logic could be extracted into a separate component to improve readability and maintainability. This aligns with the previous suggestion to better separate concerns between different variations.
-if (hoverOverlayMode === HOVER_OVERLAY_MODES.DEFAULT) { - return ( - <DefaultHoverOverlay - {...{ - offsetRight, - offsetTop, - enableDelete, - isDeletingItem, - handleDeleteDialogOpen, - actions, - handleLongPressItemOptionsClose, - }} - /> - ) -} - -if (hoverOverlayMode === HOVER_OVERLAY_MODES.THREE_DOTS_MENU) { - return ( - <ThreeDotsMenuHoverOverlay - {...{ - offsetRight, - offsetTop, - enableDelete, - isDeletingItem, - handleDeleteDialogOpen, - actions, - handleClosePopover, - popover, - }} - /> - ) -} +return ( + <HoverOverlayComponent + mode={hoverOverlayMode} + props={{ + offsetRight, + offsetTop, + enableDelete, + isDeletingItem, + handleDeleteDialogOpen, + actions, + handleLongPressItemOptionsClose, + handleClosePopover, + popover, + }} + /> +)Also applies to: 189-220
Line range hint
1-245: Update storybook with new hover overlay mode.Please update the storybook to showcase the new hover overlay mode.
Run this script to verify if the storybook has been updated:
#!/bin/bash # Description: Check if storybook has been updated with new hover overlay mode fd "stories.tsx" packages/components/modules/__shared__/ActionsOverlay/__storybook__ --exec rg -l "THREE_DOTS_MENU"packages/components/schema.graphql (1)
695-695: LGTM! The mutation is correctly placed.The
chatRoomEditMessagemutation is correctly placed in the schema and follows the established pattern of other mutations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add keyboard navigation support.
The menu should support keyboard navigation for better accessibility. Consider adding onKeyDown handler for Escape key to close the menu.
- <Popover open={popover.open} onClose={handleClosePopover}>
+ <Popover
+ open={popover.open}
+ onClose={handleClosePopover}
+ onKeyDown={(e) => {
+ if (e.key === 'Escape') {
+ handleClosePopover()
+ }
+ }}
+ >Committable suggestion skipped: line range outside the PR's diff.
packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add error handling for focus management.
The focus management code should handle potential null references and errors gracefully.
useEffect(() => {
- if (inputRef.current) {
- const { length } = inputRef.current.value
- inputRef.current.setSelectionRange(length, length)
- inputRef.current.focus()
- }
+ try {
+ if (inputRef.current) {
+ const { length } = inputRef.current.value
+ inputRef.current.setSelectionRange(length, length)
+ inputRef.current.focus()
+ }
+ } catch (error) {
+ console.error('Failed to set input focus:', error)
+ }
}, [inputRef])📝 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.
| useEffect(() => { | |
| if (inputRef.current) { | |
| const { length } = inputRef.current.value | |
| inputRef.current.setSelectionRange(length, length) | |
| inputRef.current.focus() | |
| } | |
| }, [inputRef]) | |
| useEffect(() => { | |
| try { | |
| if (inputRef.current) { | |
| const { length } = inputRef.current.value | |
| inputRef.current.setSelectionRange(length, length) | |
| inputRef.current.focus() | |
| } | |
| } catch (error) { | |
| console.error('Failed to set input focus:', error) | |
| } | |
| }, [inputRef]) |
faf424d to
8dda9b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/components/modules/__shared__/ActionsOverlay/index.tsx (1)
46-51: 🛠️ Refactor suggestionConsider adding keyboard navigation support.
The popover menu should support keyboard navigation for better accessibility.
🧹 Nitpick comments (6)
packages/components/schema.graphql (3)
166-170: Consider adding validation constraints for message content.The
ChatRoomEditMessageInputtype should include:
- Maximum content length validation
- Content format validation
180-190: Enhance subscription payloads with metadata.Consider adding:
timestampfield for each subscription payloadactorfield to identify who triggered the updatemetadatafield for additional context
1095-1103: Enhance schema security with additional directives.Consider adding:
- Rate limiting directives for mutations
- Input sanitization directives
- Authentication directives for sensitive operations
Example directive definitions:
directive @rateLimit( max: Int! window: String! ) on FIELD_DEFINITION directive @sanitizeInput on ARGUMENT_DEFINITION directive @requireAuth( requires: [String!] ) on FIELD_DEFINITIONpackages/components/modules/__shared__/ActionsOverlay/index.tsx (3)
8-8: Add JSDoc documentation for the newhoverOverlayModeprop.Consider adding JSDoc documentation to describe the purpose and available options for the new
hoverOverlayModeprop.+/** + * @param {HOVER_OVERLAY_MODES} hoverOverlayMode - Controls the display mode of the hover overlay. + * Available options: + * - DEFAULT: Shows actions inline + * - THREE_DOTS_MENU: Shows actions in a popover menu + */Also applies to: 15-17, 35-35
85-85: Add error handling for popover operations.Consider wrapping the
popover.onClose()calls in try-catch blocks to handle potential errors gracefully.const handleDeleteDialogClose = () => { setIsDeleteDialogOpen(false) setIsHoveringItem(false) - popover.onClose() + try { + popover.onClose() + } catch (error) { + console.error('Failed to close popover:', error) + } }Also applies to: 96-96
229-229: Consider using a type-safe approach for conditional styling.The conditional styling could be more type-safe by using a styled component or a constant for the styles.
+const threeDotsModeStyles = { + width: '100%', + display: 'flex', + justifyContent: 'flex-end', +} as const; {...(hoverOverlayMode === HOVER_OVERLAY_MODES.THREE_DOTS_MENU && { - sx: { - width: '100%', - display: 'flex', - justifyContent: 'flex-end', - }, + sx: threeDotsModeStyles, })}Also applies to: 232-238
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between faf424d79431bdf3648d80987c54db8b0951ee1f and 8dda9b8.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/MessageUpdateMutation.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (23)
packages/components/__mocks__/next-font.ts(1 hunks)packages/components/modules/__shared__/ActionsOverlay/DefaultHoverOverlay/index.tsx(1 hunks)packages/components/modules/__shared__/ActionsOverlay/DefaultHoverOverlay/types.ts(1 hunks)packages/components/modules/__shared__/ActionsOverlay/ThreeDotsMenuHoverOverlay/index.tsx(1 hunks)packages/components/modules/__shared__/ActionsOverlay/ThreeDotsMenuHoverOverlay/types.ts(1 hunks)packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsx(2 hunks)packages/components/modules/__shared__/ActionsOverlay/constants.ts(1 hunks)packages/components/modules/__shared__/ActionsOverlay/index.tsx(8 hunks)packages/components/modules/__shared__/ActionsOverlay/types.ts(2 hunks)packages/components/modules/__shared__/SocialInput/SubmitActions/types.ts(1 hunks)packages/components/modules/__shared__/SocialInput/styled.tsx(1 hunks)packages/components/modules/__shared__/UpdateSubmitActions/index.tsx(2 hunks)packages/components/modules/__shared__/UpdateSubmitActions/types.ts(1 hunks)packages/components/modules/__shared__/index.ts(1 hunks)packages/components/modules/comments/CommentUpdate/index.tsx(3 hunks)packages/components/modules/messages/MessageUpdate/index.tsx(1 hunks)packages/components/modules/messages/MessageUpdate/types.ts(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx(2 hunks)packages/components/modules/messages/graphql/mutations/MessageUpdate.ts(1 hunks)packages/components/schema.graphql(9 hunks)packages/design-system/components/icons/CopyIcon/index.tsx(1 hunks)packages/design-system/components/icons/DownloadIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/components/modules/shared/SocialInput/SubmitActions/types.ts
- packages/design-system/components/icons/index.ts
- packages/components/modules/shared/ActionsOverlay/constants.ts
- packages/components/modules/shared/ActionsOverlay/types.ts
- packages/components/modules/shared/index.ts
- packages/components/modules/shared/UpdateSubmitActions/types.ts
- packages/components/modules/shared/ActionsOverlay/DefaultHoverOverlay/types.ts
- packages/design-system/components/icons/CopyIcon/index.tsx
- packages/components/modules/messages/MessageUpdate/types.ts
- packages/components/modules/comments/CommentUpdate/index.tsx
- packages/design-system/components/icons/DownloadIcon/index.tsx
- packages/components/modules/shared/UpdateSubmitActions/index.tsx
- packages/components/modules/shared/SocialInput/styled.tsx
- packages/components/modules/shared/ActionsOverlay/storybook/stories.tsx
- packages/components/modules/shared/ActionsOverlay/ThreeDotsMenuHoverOverlay/types.ts
- packages/components/modules/messages/graphql/mutations/MessageUpdate.ts
- packages/components/modules/shared/ActionsOverlay/DefaultHoverOverlay/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Lint
- GitHub Check: Component Test Applications and Packages
🔇 Additional comments (15)
packages/components/schema.graphql (2)
99-113: LGTM! ChatRoom type is well-structured.The ChatRoom type includes all necessary fields for chat functionality with proper pagination support and unread message tracking.
689-715: Verify backward compatibility of schema changes.The new message editing mutations and types should be checked for breaking changes:
✅ Verification successful
Schema changes are backward compatible ✅
The new message editing mutations follow the established patterns and naming conventions. The changes are purely additive and don't modify any existing types or operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential breaking changes in the schema # Look for existing usages of the Message type rg -l "type Message.*{" --type graphql # Look for existing mutations related to messages rg -l "chatRoom.*Message.*Payload" --type graphql # Look for existing message-related fragments rg -l "fragment.*on Message" --type graphqlLength of output: 296
Script:
#!/bin/bash # Search for Message type definitions and usages fd -e graphql -x rg -l "type\s+Message\s*\{" # Search for message-related mutations with context fd -e graphql -x rg -A 5 "chatRoom.*Message" # Search for fragments and queries using Message fd -e graphql -x rg -l "(fragment|query).*Message" # Look for any Message-related input types fd -e graphql -x rg -l "input.*Message.*Input"Length of output: 1160
packages/components/__mocks__/next-font.ts (1)
2-2: LGTM!The addition of the export statement follows the best practice of making all mock exports available to importers.
packages/components/modules/__shared__/ActionsOverlay/ThreeDotsMenuHoverOverlay/index.tsx (2)
34-34: Add keyboard navigation support.The menu should support keyboard navigation for better accessibility. Consider adding
onKeyDownhandler for Escape key to close the menu.
1-66: Well-structured implementation!The component follows best practices:
- Proper permission checks for actions
- Consistent styling with MUI components
- Good separation of concerns
- Proper prop types and defaults
packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx (4)
53-54: Implement copy message functionality.The copy message action is currently unimplemented. Consider using the Clipboard API for a better user experience.
70-71: Add loading state for download action.The download attachments action should have a loading state to prevent multiple clicks.
24-43: LGTM! Well-structured message content rendering.The
renderMessageContentfunction properly handles both view and edit modes with appropriate styling for text wrapping.
76-77: Implement delete message functionality.The delete message handler is currently unimplemented.
Let's verify if there's a delete mutation available:
packages/components/modules/messages/MessageUpdate/index.tsx (4)
20-74: Excellent documentation!The component documentation is thorough and includes:
- Clear description of the component's purpose
- BaseApp feature notice
- Extension examples with code
- Proper JSDoc formatting
102-104: Implement proper error handling.The error handling is currently using console.error. Consider implementing proper error handling for both validation and mutation errors.
Also applies to: 114-115
90-92: Add loading state to prevent double submission.While there's a check for
isMutationInFlight, the form isn't disabled during submission.
124-130: Add error handling for focus management.The focus management code should handle potential null references and errors gracefully.
packages/components/modules/__shared__/ActionsOverlay/index.tsx (2)
156-158: LGTM! Good use of MUI theme color.The update to use
text.primarycolor follows MUI theme conventions and improves text visibility.
189-220: Update storybook to showcase the new hover overlay mode.Would be cool to update the storybook at
packages/components/modules/__shared__/ActionsOverlay/__storybook__/stories.tsxto demonstrate the new mode.
| type Message implements Node { | ||
| """The ID of the object""" | ||
| id: ID! | ||
| created: DateTime! | ||
| content: String | ||
| user: User | ||
| profile: Profile | ||
| role: ChatRoomParticipantRoles | ||
| hasArchivedRoom: Boolean! | ||
| verb: Verbs | ||
| room: ChatRoom | ||
| inReplyTo: Message | ||
| extraData: JSONString | ||
| pk: Int! | ||
| } | ||
|
|
||
| """An enumeration.""" | ||
| enum ChatRoomParticipantRoles { | ||
| """member""" | ||
| MEMBER | ||
|
|
||
| """admin""" | ||
| ADMIN | ||
| actionObject: Node | ||
| isRead(profileId: ID): Boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add audit fields for tracking message edits.
The Message type should include:
isEditedflageditedAttimestamporiginalContent(if edit history is required)
29fc3e4 to
4d05239
Compare
|


__package_name__package update -v __package_version__Summary by CodeRabbit
New Features
ActionsOverlaycomponent.DefaultHoverOverlayandThreeDotsMenuHoverOverlaycomponents for action overlays.Improvements
Bug Fixes