BA-2211: delete chat messages #208
Conversation
pt-tsl
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThis pull request introduces a new GraphQL mutation for deleting chat room messages. It adds input and output types for the deletion operation and updates the Changes
Possibly related PRs
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__/web/ActionsOverlay/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/messages/web/MessagesList/MessagesGroup/SystemMessage/styled.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (3)
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: 3
🧹 Nitpick comments (1)
packages/components/modules/messages/graphql/mutations/MessageDelete.ts (1)
25-51: Consider adding error type validation.While the error handling is good, consider adding type validation for error messages to ensure consistent error display.
const commit = (config: UseMutationConfig<MessageDeleteMutation>) => commitMutation({ ...config, onCompleted: (response, errors) => { errors?.forEach((error) => { + const errorMessage = typeof error.message === 'string' + ? error.message + : 'An unknown error occurred' - sendToast(error.message, { type: 'error' }) + sendToast(errorMessage, { type: 'error' }) }) config?.onCompleted?.(response, errors) }, onError: (error) => { + const errorMessage = typeof error.message === 'string' + ? error.message + : 'An unknown error occurred' - sendToast(error.message, { type: 'error' }) + sendToast(errorMessage, { type: 'error' }) config?.onError?.(error) }, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ChatRoomMessagesListPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ChatRoomQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/MessageDeleteMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/MessageItemFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/MessageUpdateMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/SendMessageMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/useMessagesListSubscription.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
packages/authentication/package.json(1 hunks)packages/components/modules/__shared__/ActionsOverlay/index.tsx(4 hunks)packages/components/modules/__shared__/ActionsOverlay/types.ts(2 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx(4 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/types.ts(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/index.tsx(1 hunks)packages/components/modules/messages/graphql/fragments/MessageItem.ts(1 hunks)packages/components/modules/messages/graphql/mutations/MessageDelete.ts(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(3 hunks)packages/config/package.json(1 hunks)packages/design-system/package.json(1 hunks)packages/eslint-plugin/package.json(1 hunks)packages/graphql/package.json(1 hunks)packages/provider/package.json(1 hunks)packages/test/package.json(1 hunks)packages/utils/package.json(1 hunks)packages/wagtail/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (40)
packages/test/package.json (3)
17-22: Explicit Dependency Version Updates in "dependencies" SectionThe dependency versions for
@emotion/cache,@emotion/react,@mui/material,@tanstack/react-query, andjs-cookiehave been updated to explicit version numbers. This change improves build reproducibility and dependency management. Please verify that these versions align with those used in other packages to avoid any version conflicts.
25-25: Explicit Peer Dependency Version for ReactThe peer dependency for
reactis now pinned to"18.3.1". Confirm that this specific version meets compatibility requirements across your application and consider if a semver range might be more appropriate for flexibility.
30-43: Pinned Dev Dependency VersionsThe development dependencies now use specific version numbers for packages such as
@testing-library/jest-dom,@testing-library/react,@testing-library/user-event, and others. This consistency should help avoid ambiguity during development and testing. It is advisable to test that these updates integrate well with your configuration (like TypeScript, Jest, and Babel) without introducing any unexpected issues.packages/design-system/package.json (5)
17-27: Explicit Dependency Version Updates for Core PackagesThe versions for core design system dependencies (e.g.,
@emotion/cache,@emotion/react,@emotion/styled,@mui/icons-material,@mui/lab,@mui/material,@mui/material-nextjs,@mui/system,@mui/x-date-pickers, and@storybook/react) have been updated from catalog references to explicit version numbers. This change improves reproducibility and clarity in version management. Please verify that these versions are compatible with the rest of the ecosystem in your project.
29-33: Explicit Dependency Version Updates for Additional LibrariesThe dependencies for
jotai,lodash,next, andreact-hook-formare now specified with explicit versions. Note that the version fornextis a canary release (14.3.0-canary.24); please confirm that using a canary version is intentional and that any associated risks have been considered.
39-40: Peer Dependency Version UpdatesThe peer dependencies for
reactandreact-domhave been updated to"18.3.1". Ensure that these versions align with your overall project requirements and are compatible with other parts of your application.
43-45: Updated Babel Presets in DevDependenciesThe Babel presets (
@babel/preset-env,@babel/preset-react, and@babel/preset-typescript) now use explicit version numbers. This should help maintain consistent builds and eliminate reliance on catalog references.
49-71: Explicit Version Updates for Storybook, Tailwind, and Related ToolsThe devDependencies related to Storybook (including various addons and compiler configurations), Tailwind, and TypeScript have been updated with fixed version numbers. These changes are expected to improve build reproducibility and stability in the development workflow. Please verify that these updates integrate seamlessly with your current tooling and processes.
packages/components/modules/messages/graphql/fragments/MessageItem.ts (1)
8-8: LGTM! Addition ofdeletedfield supports message deletion functionality.The new field is correctly placed and will enable tracking of message deletion states.
packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/types.ts (1)
8-8: LGTM! Addition ofisGroupprop enables different deletion flows for individual and group chats.The optional boolean prop aligns with the PR requirements for showing different deletion confirmation messages based on chat type.
packages/components/modules/__shared__/ActionsOverlay/types.ts (2)
3-3: LGTM! Import of ConfirmDialogProps from design system.The import supports the new DeleteDialogProps type.
31-31: LGTM! Addition of DeleteDialogProps enables customizable confirmation dialogs.The optional Partial type allows flexible configuration of the deletion confirmation dialog.
packages/components/modules/messages/graphql/mutations/MessageDelete.ts (1)
7-23: LGTM! Well-structured GraphQL mutation for message deletion.The mutation is correctly defined with proper input type, response fields, and error handling.
packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/index.tsx (1)
98-103: LGTM! Proper prop passing implementation.The
isGroupprop is correctly passed to the MessageItem component, enabling different delete confirmation messages for group and individual chats.packages/components/modules/__shared__/ActionsOverlay/index.tsx (1)
101-117: LGTM! Enhanced dialog customization.The addition of
DeleteDialogPropsimproves component reusability by allowing customization of the confirmation dialog. The simplified delete button label maintains consistency across the application.packages/eslint-plugin/package.json (1)
19-27: Verify dependency version compatibility.Please verify the following:
- Consider updating eslint to the latest version for security patches
- The specified TypeScript version 5.4.5 seems to be from the future
packages/provider/package.json (3)
16-18: Dependency Version Updates for Providers
The explicit version updates for@tanstack/react-queryandlodashensure that the package now uses reproducible versions rather than catalog references. This change improves predictability during builds.
21-22: Peer Dependency Update
Updating thereactpeer dependency to"18.3.1"aligns this package with the versions being used in other parts of the repository. Make sure that all consuming packages are compatible with this version.
24-28: Dev Dependency Version Standardization
The updates for@types/lodash,@types/react, andtypescriptnow use specific version numbers. This explicit versioning will help avoid accidental incompatibilities and is consistent with the overall dependency management strategy in the repository.packages/config/package.json (1)
13-28: DevDependencies Explicit Versioning
This hunk replaces catalog-style versioning with explicit versions (using caret notation where appropriate) for ESLint, Prettier, and related tools. This not only improves clarity but also allows minor updates for non-breaking changes. Overall, the change is well aligned with the dependency management improvements seen across the project.packages/graphql/package.json (3)
16-23: GraphQL Dependencies Updated
All key dependencies—graphql,graphql-ws,isomorphic-ws,js-cookie,react-relay,relay-connection-handler-plus,relay-runtime, andrelay-test-utils—have been updated to explicit versions. This clear specification will help ensure that the GraphQL utilities behave consistently across environments.
26-27: Uniform Peer Dependency for React
With the update to"react": "18.3.1"in peerDependencies, this package now matches the version expected in other packages. Consistency here reduces integration headaches.
32-39: DevDependencies for GraphQL Tools
The explicit version updates for various type definitions and tools (includingrelay-compilerandtypescript) provide better build predictability and a more stable development environment.packages/authentication/package.json (3)
17-22: Authentication Dependencies Upgrade
Updating dependencies such as@hookform/resolvers,@tanstack/react-query,jotai,js-cookie,react-hook-form, andzodto specific versions removes ambiguity from the build process and aligns this package with the versioning strategy used elsewhere in the repository.
25-27: Peer Dependency Revision for React
The update of thereactpeer dependency to"18.3.1"ensures that both the authentication package and its consumers are in sync with the version across the codebase.
33-47: DevDependencies Consolidation
Specifying explicit versions for testing libraries, type definitions, and build tools (such as@faker-js/faker,@testing-librarypackages,babel-jest,jest,ts-jest, etc.) minimizes the risk of version-related issues during development and tests. This is a strong move toward consistency and stability in your development environment.packages/utils/package.json (4)
17-18: Core Dependency Updates in Utils
Changingaxiosto version"1.7.2"along with updating dependencies such asexpo-constantsandexpo-secure-storeprovides a more predictable dependency landscape. Explicit versioning here reduces uncertainty across different environments.
20-21: Critical Library Version Updates and Caution on Canary Release
Updates to dependencies likejs-cookie,lodash,luxon,next,react-hook-form,react-native, andzustandnow use explicit versions. Note that while using"14.3.0-canary.24"fornextmight be acceptable if bleeding-edge features are required, this version should be monitored to avoid potential production instability.
34-35: Consistent Peer Dependency for React
The peer dependency update forreactto"18.3.1"maintains consistency with changes in other packages and should help prevent version mismatch issues.
40-57: DevDependencies Alignment for Utils
The explicit version updates for testing libraries and TypeScript-related tools add extra stability and transparency to your development process. By moving away from catalog references, you significantly reduce unexpected version shifts during development or CI builds.packages/wagtail/package.json (3)
22-28: Explicit Dependency Versions Set in Dependencies SectionThe dependency versions have been updated from catalog references to fixed versions (e.g.
"@mui/material": "5.15.19","axios": "1.7.2", etc.). This enhances clarity and reproducibility of builds. Please double-check that these versions are fully compatible with other packages in the ecosystem.
34-34: Updated Peer Dependency for ReactThe peer dependency for React is now set to
"18.3.1". Ensure that this version aligns with the expectations across all consuming packages.
37-84: Updated DevDependencies with Explicit Version NumbersAll devDependencies (e.g. Babel presets, Storybook addons, Cypress, TypeScript, and Webpack tools) have been updated to specific version numbers. This is great for consistency. It would be beneficial to verify that these updates do not introduce any incompatibilities during local builds or testing.
packages/components/package.json (3)
24-50: Explicit Dependency Versions in Components PackageThe dependencies have been updated with explicit version numbers (e.g.
"@emotion/cache": "11.11.0","@mui/material": "5.15.19","react-hook-form": "7.51.5", etc.). This update improves version traceability and build reproducibility. Please ensure that these versions remain consistent with those used in related packages across the repository.
56-57: Peer Dependencies Version UpdateThe peerDependencies for React and ReactDOM are now
"18.3.1". Confirm that this upgrade harmonizes with the other packages consuming React to avoid potential rendering or lifecycle issues.
60-121: Comprehensive Update of DevDependenciesThe devDependencies block now lists explicit versions for a wide range of tools including Babel, Storybook, Cypress, Jest, and Webpack. The consistency across these toolchain versions is beneficial; however, it is advisable to run the full test suite to ensure that none of these version bumps introduce unexpected side effects in development or CI.
packages/components/schema.graphql (4)
157-160: New Input Type for Deleting Chat MessagesThe input type
ChatRoomDeleteMessageInputis added with requiredidand an optionalclientMutationId. This aligns with the deletion functionality described in the PR objectives. Consider adding a descriptive comment for documentation purposes if this schema is publicly exposed.
162-168: New Output Type for Chat Message DeletionThe addition of
ChatRoomDeleteMessagePayloadprovides a structured response for the deletion mutation, including error details and the deleted message edge. This is consistent with other mutation payload definitions in the schema.
665-666: Augmenting the Message Type with a Deleted FlagA new field
deleted: Boolean!has been added to theMessagetype. This flag is critical to implement the display behavior for deleted messages (showing different texts for the deleter and others). Please verify that all client queries using this fragment properly handle this field.
717-717: Introducing the chatRoomDeleteMessage MutationThe mutation
chatRoomDeleteMessage(input: ChatRoomDeleteMessageInput!): ChatRoomDeleteMessagePayloadis added to the Mutation type. This update meets the PR’s objective for implementing chat message deletion. Ensure that the corresponding resolver implements the proper behavior (updating message text, triggering a toast notification, etc.).
...ges/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx
Outdated
Show resolved
Hide resolved
...ges/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx
Outdated
Show resolved
Hide resolved
...ges/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx
Outdated
Show resolved
Hide resolved
...ges/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx
Outdated
Show resolved
Hide resolved
...ges/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/graphql/mutations/MessageDelete.ts
Outdated
Show resolved
Hide resolved
pt-tsl
force-pushed
the
BA-2211-delete-chat-message
branch
from
dc76a75 to
8d4fe5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (2)
32-49:⚠️ Potential issueFix error handling and success notification.
The message deletion implementation has several issues:
- Success notification uses 'error' type instead of 'success'
- Error handling is missing
Apply this diff to fix the issues:
const [commitUpdate, isMutationInFlight] = useMessageDeleteMutation() const onDeleteClick = async () => { if (isMutationInFlight) return commitUpdate({ variables: { input: { id: message.id, }, }, onCompleted: (response, errors) => { if (!errors) { - sendToast('Your message was deleted', { type: 'error' }) + sendToast('Your message was deleted', { type: 'success' }) + } else { + sendToast('Failed to delete message', { type: 'error' }) + console.error('Delete message error:', errors) } }, + onError: (error) => { + sendToast('Failed to delete message', { type: 'error' }) + console.error('Delete message error:', error) + }, }) }
114-115:⚠️ Potential issueUpdate loading state and remove TODO comment.
The loading state is not reflected in the UI, and there's an unnecessary TODO comment.
Apply this diff to fix the issues:
- handleDeleteItem={() => onDeleteClick()} // TODO: Implement delete message - isDeletingItem={false} + handleDeleteItem={() => onDeleteClick()} + isDeletingItem={isMutationInFlight}
🧹 Nitpick comments (4)
packages/components/schema.graphql (4)
157-160: Add Documentation for New Input Type "ChatRoomDeleteMessageInput".
The new input type successfully defines the required id field and optional clientMutationId used for the delete operation. For maintainability and clarity, consider adding descriptive comments (docstrings) for the type and its fields (e.g., “Input for deleting a chat message”) to ensure consistent documentation across schema types.
162-168: Review the "ChatRoomDeleteMessagePayload" Definition.
The payload type is defined with standard fields such as errors, _debug, deletedMessage, and clientMutationId. Like the input type, adding a brief docstring describing the purpose of this payload (e.g., “Payload returned after attempting to delete a chat message”) would improve understandability for future maintainers.
665-666: Clarify the "deleted" Field in the Message Type.
A new field "deleted: Boolean!" has been added to the Message type to indicate whether a message is deleted. Consider adding an explanatory docstring (for example, “Indicates if the message has been deleted; true if deleted, false otherwise”) to align with the clarity of the rest of the schema.
193-195: Review New Type "ChatRoomOnMessage".
The new type “ChatRoomOnMessage” (with a single field "message: MessageEdge") likely serves as a subscription payload for message deletion events. Consider adding a short description to the type and its field to clarify its purpose, for example indicating that it is used to notify subscribers of a change to a specific message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/components/__generated__/useMessagesListSubscription.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx(4 hunks)packages/components/modules/messages/graphql/subscriptions/useMessagesListSubscription.tsx(1 hunks)packages/components/schema.graphql(5 hunks)
🔇 Additional comments (4)
packages/components/modules/messages/graphql/subscriptions/useMessagesListSubscription.tsx (1)
7-7: LGTM! Subscription field updated to handle all message events.The change from
chatRoomOnNewMessagetochatRoomOnMessageappropriately broadens the subscription scope to handle all message events, including deletions, which aligns with the PR's message deletion functionality.Let's verify that this subscription field exists in the schema:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the subscription field exists in the schema # Expected: The field should be defined in the schema # Search for the subscription field definition rg -A 5 "type Subscription" .Length of output: 565
LGTM! Verified subscription schema update.
The schema file confirms that the subscription field "chatRoomOnMessage" is defined as expected, which aligns perfectly with broadening the message events scope to include deletions.packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (1)
77-77: Review spacing in message content.The space after BlockIcon could affect the layout of non-deleted messages.
Consider moving the space inside the conditional:
- {deletedMessage ? <BlockIcon sx={{ fontSize: '20px', color: 'grey.500' }} /> : undefined}{' '} + {deletedMessage ? <><BlockIcon sx={{ fontSize: '20px', color: 'grey.500' }} /> </> : undefined}✅ Verification successful
Spacing may affect layout when no deleted message icon is rendered.
The current implementation always appends a space even if deletedMessage is false, which may disrupt the intended layout for non-deleted messages. Consider conditionally including the space only when rendering the BlockIcon to avoid unnecessary spacing.
- File: packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx
- Suggestion: Move the space inside the condition to ensure it’s only rendered for deleted messages.
packages/components/schema.graphql (2)
716-718: Verify the Mutation "chatRoomDeleteMessage" Implementation.
The mutation is correctly added to the Mutation type with the appropriate input and payload types. Ensure that the backend resolver logic (and associated authorization checks) correctly processes this mutation as intended by the PR objectives. If not already done, validate that clients calling this mutation receive proper error feedback via the errors field.
197-199: Examine New Type "ChatRoomOnMessagesCountUpdate".
This type currently only includes a field "profile: Profile". Depending on its intended use for real-time updates (for instance towards unread counts or overall message counts), verify that this type includes all necessary information. Adding a descriptive comment on its intended purpose is recommended to ensure that future changes remain aligned with its design.
pt-tsl
force-pushed
the
BA-2211-delete-chat-message
branch
from
8d4fe5f to
7364c50
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (1)
45-45: 🛠️ Refactor suggestionUpdate success message to match design requirements.
The success message should be "Message Deleted" according to the PR objectives, not "Your message was deleted".
- sendToast('Your message was deleted', { type: 'error' }) + sendToast('Message Deleted', { type: 'success' })
🧹 Nitpick comments (3)
packages/components/schema.graphql (3)
157-160: Consider adding description for ChatRoomDeleteMessageInput.Add documentation comments to describe the purpose and usage of this input type, similar to other types in the schema.
+""" +Input type for deleting a message in a chat room. +""" input ChatRoomDeleteMessageInput { + """The ID of the message to delete.""" id: ID! + """Optional client mutation ID.""" clientMutationId: String }
162-168: Consider adding description for ChatRoomDeleteMessagePayload.Add documentation comments to describe the purpose and fields of this payload type.
+""" +Payload type returned after deleting a message in a chat room. +""" type ChatRoomDeleteMessagePayload { """May contain more than one error for same field.""" errors: [ErrorType] _debug: DjangoDebug + """The deleted message.""" deletedMessage: MessageEdge + """Optional client mutation ID.""" clientMutationId: String }
665-665: Consider adding description for the deleted field.Add a documentation comment to describe the purpose of the deleted field in the Message type.
+ """Indicates whether the message has been deleted.""" deleted: Boolean!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/components/__generated__/MessageDeleteMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/useMessagesListSubscription.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx(4 hunks)packages/components/modules/messages/graphql/mutations/MessageDelete.ts(1 hunks)packages/components/modules/messages/graphql/subscriptions/useMessagesListSubscription.tsx(1 hunks)packages/components/schema.graphql(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/modules/messages/graphql/subscriptions/useMessagesListSubscription.tsx
- packages/components/modules/messages/graphql/mutations/MessageDelete.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (1)
77-79: Verify spacing in deleted message display.The space after BlockIcon was questioned in past reviews. Please clarify if this spacing is intentional and matches the design requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/CHANGELOG.md (1)
3-7: Enhance the changelog entry to better reflect the scope of changes.The current entry could be more descriptive about the implemented features. Consider expanding it to:
- Enabled chat room messages deletion + Enabled chat room messages deletion: + - Added message deletion via three-dot menu overlay + - Implemented deletion confirmation dialogs + - Added deleted message states + - Integrated toast notifications for deletions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
pt-tsl
force-pushed
the
BA-2211-delete-chat-message
branch
from
71d0a2a to
48631a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (1)
45-45:⚠️ Potential issueUse 'success' toast type for successful deletion.
The toast notification uses 'error' type for a successful deletion, which may confuse users.
- sendToast('Your message was deleted', { type: 'error' }) + sendToast('Your message was deleted', { type: 'success' })
🧹 Nitpick comments (1)
packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (1)
92-92: Track unimplemented features.There are TODO comments for unimplemented features:
- Copy message functionality
- Download attachments functionality
Consider creating issues to track these features if they're planned for future implementation.
Would you like me to help create issues for tracking these features?
Also applies to: 109-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ChatRoomMessagesListPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ChatRoomQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/MessageDeleteMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/MessageItemFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/MessageUpdateMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/SendMessageMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/useMessagesListSubscription.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/__shared__/ActionsOverlay/index.tsx(4 hunks)packages/components/modules/__shared__/ActionsOverlay/types.ts(2 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx(4 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/types.ts(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/index.tsx(1 hunks)packages/components/modules/messages/graphql/fragments/MessageItem.ts(1 hunks)packages/components/modules/messages/graphql/mutations/MessageDelete.ts(1 hunks)packages/components/modules/messages/graphql/subscriptions/useMessagesListSubscription.tsx(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/types.ts
- packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/index.tsx
- packages/components/CHANGELOG.md
- packages/components/modules/messages/graphql/fragments/MessageItem.ts
- packages/components/modules/messages/graphql/subscriptions/useMessagesListSubscription.tsx
- packages/components/modules/shared/ActionsOverlay/types.ts
- packages/components/modules/shared/ActionsOverlay/index.tsx
- packages/components/package.json
- packages/components/modules/messages/graphql/mutations/MessageDelete.ts
- packages/components/schema.graphql
🔇 Additional comments (1)
packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (1)
77-78: Explain spacing after BlockIcon.The space after the BlockIcon is intentional to provide visual separation between the icon and the "message deleted" text. This follows standard UI practices for icon-text combinations.
pt-tsl
force-pushed
the
BA-2211-delete-chat-message
branch
from
48631a6 to
1973a0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/modules/messages/common/graphql/mutations/MessageDelete.ts (1)
24-50: LGTM! Robust error handling in custom hook.The
useMessageDeleteMutationhook implementation:
- Returns mutation commit function and loading state
- Properly handles errors with toast notifications
- Maintains original config callbacks
However, consider adding JSDoc comments to document the hook's parameters and return values.
Add documentation:
+/** + * Hook for deleting chat room messages + * @returns [commitMutation, isMutationInFlight] + * - commitMutation: Function to execute the delete mutation + * - isMutationInFlight: Boolean indicating if mutation is in progress + */ export const useMessageDeleteMutation = (): [
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/components/__generated__/MessageDeleteMutation.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (7)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/__shared__/web/ActionsOverlay/index.tsx(5 hunks)packages/components/modules/__shared__/web/ActionsOverlay/types.ts(2 hunks)packages/components/modules/messages/common/graphql/fragments/MessageItem.ts(1 hunks)packages/components/modules/messages/common/graphql/mutations/MessageDelete.ts(1 hunks)packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx(1 hunks)packages/components/modules/messages/common/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
🔇 Additional comments (8)
packages/components/modules/messages/common/graphql/fragments/MessageItem.ts (1)
8-8: LGTM! Addeddeletedfield for message deletion status.The addition of the
deletedfield toMessageItemFragmentaligns with the PR objectives and enables tracking of message deletion status for UI display.packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx (1)
7-7: LGTM! Renamed subscription to handle all message events.The subscription name change from
chatRoomOnNewMessagetochatRoomOnMessagebetter reflects its broader scope of handling all message events, including deletions.packages/components/modules/__shared__/web/ActionsOverlay/types.ts (1)
30-30: LGTM! Added props for delete functionality control.The new props enhance the
ActionOverlayPropsinterface:
disableDelete: Allows conditional control over delete actionDeleteDialogProps: Enables customization of the confirmation dialogBoth props are optional, maintaining backward compatibility.
Also applies to: 33-33
packages/components/modules/messages/common/graphql/mutations/MessageDelete.ts (1)
7-22: LGTM! Well-structured GraphQL mutation.The mutation is correctly structured with:
- Input validation through
ChatRoomDeleteMessageInput- Proper error handling fields
- Fragment reuse for message data
packages/components/modules/messages/common/index.ts (1)
10-11: LGTM!The new exports for message deletion and update mutations are correctly placed and follow the established pattern in the file.
packages/components/modules/__shared__/web/ActionsOverlay/index.tsx (3)
27-33: LGTM!The new props
disableDeleteandDeleteDialogPropsare well-defined with appropriate default values. These additions enhance the component's flexibility for message deletion scenarios.
169-179: LGTM!The delete button changes are well-implemented:
- Correctly handles both loading and disabled states
- Uses simplified "Delete" text as per design requirements
99-116: Update dialog content to match requirements.The current implementation uses a generic confirmation message, but the PR requirements specify different messages for individual and group chats.
Please ensure that the
DeleteDialogPropspassed from the parent component includes the appropriate content based on the chat type.Example usage:
const dialogProps = { content: isGroupChat ? "Are you sure you want to delete this message? All members in this group will no longer see it." : "Are you sure you want to delete this message? The other person will no longer see it." };
pt-tsl
force-pushed
the
BA-2211-delete-chat-message
branch
2 times, most recently
from
9c57ad3 to
aeaaca2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/components/modules/__shared__/web/ActionsOverlay/__storybook__/ActionsOverlay.mdx (1)
49-49: Enhance example usage.While the example correctly uses the new
showDeleteButtonprop, consider enhancing it to:
- Demonstrate the new
disableDeleteButtonprop- Show a more relevant example aligned with the chat message deletion use case
Apply this diff to improve the example:
title='Button', - showDeleteButton={true} + showDeleteButton={true} + disableDeleteButton={false} handleDeleteItem={() => {}}Additionally, consider adding a second example specifically for chat message deletion:
export const ChatMessageActionsOverlay = () => { const [isDeleting, setIsDeleting] = React.useState(false); const handleDelete = async () => { setIsDeleting(true); // Simulated deletion await new Promise(resolve => setTimeout(resolve, 1000)); setIsDeleting(false); }; return ( <ActionsOverlay title='Message' showDeleteButton={true} disableDeleteButton={isDeleting} isDeletingItem={isDeleting} handleDeleteItem={handleDelete} > <div>Example chat message</div> </ActionsOverlay> ); };packages/components/modules/__shared__/web/ActionsOverlay/DefaultHoverOverlay/index.tsx (2)
28-28: Enhance accessibility with more descriptive aria-label.The current aria-label "delete item" is generic. Consider making it more descriptive to improve accessibility.
- aria-label="delete item" + aria-label="delete message"
24-32: Add tooltip to delete button.Consider adding a tooltip to the delete button to enhance user experience, similar to how it's done for other action buttons.
{showDeleteButton && ( <IconButton onClick={handleDeleteDialogOpen} disabled={isDeletingItem || disableDeleteButton} - aria-label="delete message" + aria-label="delete message" + title="Delete message" > <TrashCanIcon /> </IconButton> )}packages/components/modules/__shared__/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/index.tsx (1)
58-72: Enhance delete button accessibility.The delete menu item implementation looks good, but could benefit from improved accessibility.
Consider these accessibility enhancements:
<MenuItem onClick={handleDeleteDialogOpen} disabled={isDeletingItem || disableDeleteButton} color="disabled" + aria-label="Delete message" + role="menuitem" > <TrashCanIcon sx={{ color: disableDeleteButton ? 'text.disabled' : 'error.main' }} /> <Typography variant="body2" color={disableDeleteButton ? 'text.disabled' : 'error.main'} + aria-hidden="true" > Delete </Typography> </MenuItem>packages/components/schema.graphql (1)
1-1821: Consider these architectural points for the implementation.Based on the PR objectives, ensure the implementation includes:
UI Components:
- Confirmation dialog with different messages for individual vs group chats
- Toast notification using the existing info icon and timed snack-bar
- Gray font styling for deleted message text
State Management:
- Track message deletion state using the new
deletedfield- Update UI immediately on successful deletion
- Handle optimistic updates for better UX
Error Handling:
- Handle network errors during deletion
- Show appropriate error messages in the toast
- Revert optimistic updates on failure
Accessibility:
- Ensure the delete button is keyboard accessible
- Add appropriate ARIA labels
- Maintain focus management during confirmation dialog
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/components/__generated__/MessageDeleteMutation.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/__shared__/web/ActionsOverlay/DefaultHoverOverlay/index.tsx(2 hunks)packages/components/modules/__shared__/web/ActionsOverlay/DefaultHoverOverlay/types.ts(1 hunks)packages/components/modules/__shared__/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/index.tsx(2 hunks)packages/components/modules/__shared__/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/types.ts(1 hunks)packages/components/modules/__shared__/web/ActionsOverlay/__storybook__/ActionsOverlay.mdx(2 hunks)packages/components/modules/__shared__/web/ActionsOverlay/index.tsx(6 hunks)packages/components/modules/__shared__/web/ActionsOverlay/types.ts(2 hunks)packages/components/modules/comments/web/CommentItem/index.tsx(1 hunks)packages/components/modules/messages/common/graphql/fragments/MessageItem.ts(1 hunks)packages/components/modules/messages/common/graphql/mutations/MessageDelete.ts(1 hunks)packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx(1 hunks)packages/components/modules/messages/common/index.ts(1 hunks)packages/components/modules/messages/web/ChatRoomsList/ChatRoomItem/index.tsx(1 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx(3 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/types.ts(1 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(71 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/components/modules/messages/common/graphql/fragments/MessageItem.ts
- packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx
- packages/components/package.json
- packages/components/modules/messages/common/index.ts
- packages/components/modules/messages/common/graphql/mutations/MessageDelete.ts
- packages/components/CHANGELOG.md
- packages/components/modules/shared/web/ActionsOverlay/types.ts
- packages/components/modules/shared/web/ActionsOverlay/index.tsx
🔇 Additional comments (13)
packages/components/modules/__shared__/web/ActionsOverlay/DefaultHoverOverlay/types.ts (1)
6-11: LGTM! Props updated to support message deletion functionality.The interface changes align well with the PR objectives by adding
showDeleteButtonanddisableDeleteButtonprops to control the delete button's visibility and state.packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/types.ts (1)
8-8: LGTM! Added prop to support group chat functionality.The optional
isGroupprop enables different delete confirmation messages for group vs individual chats, as specified in the PR objectives.packages/components/modules/__shared__/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/types.ts (1)
8-13: LGTM! Props updated consistently with DefaultHoverOverlay.The interface changes maintain consistency with
DefaultHoverOverlayby using the same props (showDeleteButtonanddisableDeleteButton) to control the delete button's visibility and state.packages/components/modules/__shared__/web/ActionsOverlay/DefaultHoverOverlay/index.tsx (1)
12-14: LGTM! Delete button implementation aligns with requirements.The implementation correctly uses the new props to control the delete button's visibility and disabled state.
Also applies to: 24-32
packages/components/modules/__shared__/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/index.tsx (1)
15-17: LGTM! Props refactoring improves clarity.The prop renaming from
enableDeletetoshowDeleteButtonand addition ofdisableDeleteButtonmakes the component's API more intuitive and consistent with the PR objectives.packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx (1)
100-100: LGTM! Proper propagation of group chat context.The
isGroupprop is correctly passed to MessageItem, enabling contextual delete confirmation messages.packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (2)
66-68: LGTM! Proper styling for deleted messages.The color change for deleted messages aligns with the design requirements.
81-83: LGTM! Clear visual indication of deleted messages.The BlockIcon and spacing provide a clear visual indication of deleted messages.
packages/components/modules/comments/web/CommentItem/index.tsx (1)
130-130: LGTM! Consistent prop naming across components.The prop rename from
enableDeletetoshowDeleteButtonmaintains consistency with other components.packages/components/schema.graphql (4)
157-160: LGTM! Input type is well-defined.The
ChatRoomDeleteMessageInputtype correctly requires the message ID and includes an optionalclientMutationId.
162-168: LGTM! Output type handles errors appropriately.The
ChatRoomDeleteMessagePayloadtype includes:
- Error handling via
errorsfield- Debug information
- The deleted message edge
- Client mutation ID for tracking
665-665: LGTM! Message type updated correctly.The
deletedfield addition to theMessagetype enables proper tracking of message deletion state.
717-717: LGTM! Mutation is properly defined.The
chatRoomDeleteMessagemutation is correctly added to the schema with proper input and output types.
packages/components/modules/__shared__/web/ActionsOverlay/__storybook__/ActionsOverlay.mdx
Show resolved
Hide resolved
...components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx
Show resolved
Hide resolved
...components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx
Show resolved
Hide resolved
packages/components/modules/messages/web/ChatRoomsList/ChatRoomItem/index.tsx
Show resolved
Hide resolved
pt-tsl
force-pushed
the
BA-2211-delete-chat-message
branch
2 times, most recently
from
25a1670 to
8fb8dfa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (1)
38-53:⚠️ Potential issueFix toast notification type and enhance error handling.
The toast notification uses 'error' type for a successful deletion, which is incorrect.
Apply this fix:
const onDeleteClick = async () => { if (isMutationInFlight) return commitUpdate({ variables: { input: { id: message.id, }, }, onCompleted: (response, errors) => { - if (!errors) { - sendToast('Your message was deleted', { type: 'error' }) + if (errors?.length) { + sendToast('Failed to delete message', { type: 'error' }) + return } + sendToast('Your message was deleted', { type: 'info' }) }, + onError: (error) => { + console.error('Delete message error:', error) + sendToast('Failed to delete message', { type: 'error' }) + } }) }
🧹 Nitpick comments (3)
packages/components/schema.graphql (1)
665-665: Add description for the deleted field.The
deletedfield in the Message type should have a description documenting its purpose.+ """Indicates whether the message has been deleted.""" deleted: Boolean!packages/components/modules/messages/common/graphql/mutations/MessageDelete.ts (1)
33-47: Enhance error handling in the mutation.The error handling could be more specific and provide better user feedback.
const commit = (config: UseMutationConfig<MessageDeleteMutation>) => commitMutation({ ...config, onCompleted: (response, errors) => { + if (response.chatRoomDeleteMessage?.errors?.length) { + response.chatRoomDeleteMessage.errors.forEach((error) => { + sendToast(`Failed to delete message: ${error.messages.join(', ')}`, { type: 'error' }) + }) + return + } errors?.forEach((error) => { sendToast(error.message, { type: 'error' }) }) config?.onCompleted?.(response, errors) }, onError: (error) => { - sendToast(error.message, { type: 'error' }) + sendToast('Failed to delete message. Please try again.', { type: 'error' }) + console.error('Delete message error:', error) config?.onError?.(error) }, })packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (1)
81-83: Add aria-label for deleted message icon.The BlockIcon needs an aria-label for better accessibility.
-{deletedMessage && <BlockIcon sx={{ fontSize: '20px', color: 'grey.500' }} />} +{deletedMessage && ( + <BlockIcon + sx={{ fontSize: '20px', color: 'grey.500' }} + aria-label="Message deleted" + /> +)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/components/__generated__/MessageDeleteMutation.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/__shared__/web/ActionsOverlay/DefaultHoverOverlay/index.tsx(2 hunks)packages/components/modules/__shared__/web/ActionsOverlay/DefaultHoverOverlay/types.ts(1 hunks)packages/components/modules/__shared__/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/index.tsx(2 hunks)packages/components/modules/__shared__/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/types.ts(1 hunks)packages/components/modules/__shared__/web/ActionsOverlay/__storybook__/ActionsOverlay.mdx(2 hunks)packages/components/modules/__shared__/web/ActionsOverlay/index.tsx(6 hunks)packages/components/modules/__shared__/web/ActionsOverlay/types.ts(2 hunks)packages/components/modules/comments/web/CommentItem/index.tsx(1 hunks)packages/components/modules/messages/common/graphql/fragments/MessageItem.ts(1 hunks)packages/components/modules/messages/common/graphql/mutations/MessageDelete.ts(1 hunks)packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx(1 hunks)packages/components/modules/messages/common/index.ts(1 hunks)packages/components/modules/messages/web/ChatRoomsList/ChatRoomItem/index.tsx(1 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx(4 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/types.ts(1 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/components/package.json
- packages/components/CHANGELOG.md
- packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/types.ts
- packages/components/modules/messages/common/graphql/fragments/MessageItem.ts
- packages/components/modules/shared/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/types.ts
- packages/components/modules/comments/web/CommentItem/index.tsx
- packages/components/modules/messages/web/ChatRoomsList/ChatRoomItem/index.tsx
- packages/components/modules/shared/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/index.tsx
- packages/components/modules/shared/web/ActionsOverlay/storybook/ActionsOverlay.mdx
- packages/components/modules/shared/web/ActionsOverlay/DefaultHoverOverlay/types.ts
- packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx
- packages/components/modules/shared/web/ActionsOverlay/index.tsx
- packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx
- packages/components/modules/messages/common/index.ts
- packages/components/modules/shared/web/ActionsOverlay/types.ts
- packages/components/modules/shared/web/ActionsOverlay/DefaultHoverOverlay/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
🔇 Additional comments (8)
packages/components/schema.graphql (6)
157-160: LGTM! Input type is well-defined.The
ChatRoomDeleteMessageInputtype is correctly structured with required message ID and optional clientMutationId.
162-168: LGTM! Payload type follows best practices.The
ChatRoomDeleteMessagePayloadtype follows GraphQL best practices by including:
- Error handling through the errors field
- Debug information
- The deleted message as an edge type
- Client mutation ID for optimistic updates
1666-1666: LGTM! Subscription rename reflects broader scope.The subscription rename from
chatRoomOnNewMessagetochatRoomOnMessagebetter reflects its broader scope of handling all message-related events.
157-168: LGTM! Well-structured mutation types for message deletion.The input and payload types follow GraphQL best practices and maintain consistency with other mutations in the schema:
- Input type correctly requires message ID
- Payload includes standard error handling
- Returns deleted message as an edge for proper cursor-based pagination
665-665: LGTM! Message type correctly updated for deletion status.The addition of the required
deletedboolean field aligns with the PR objectives, enabling different message displays for deleted content.
1666-1666: LGTM! Subscription naming improved for better semantics.The rename from
chatRoomOnNewMessagetochatRoomOnMessagebetter reflects its broader scope of handling all message events, including deletions.packages/components/modules/messages/common/graphql/mutations/MessageDelete.ts (1)
7-22: LGTM! Well-structured GraphQL mutation.The mutation is well-structured with proper input validation and includes the MessageItemFragment for consistent data shape.
packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (1)
121-127: LGTM! Delete button implementation follows requirements.The delete button implementation correctly:
- Shows only for own messages
- Disables for deleted messages
- Includes confirmation dialog with context-aware message
* Default social input send behavior * Versioning
pt-tsl
force-pushed
the
BA-2211-delete-chat-message
branch
from
b18899e to
473e062
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (1)
38-53:⚠️ Potential issueEnhance error handling and fix toast type.
The current implementation has two issues:
- Using 'error' toast type for successful deletion
- Missing comprehensive error handling
Apply this fix:
const onDeleteClick = async () => { if (isMutationInFlight) return commitUpdate({ variables: { input: { id: message.id, }, }, onCompleted: (response, errors) => { - if (!errors) { - sendToast('Your message was deleted', { type: 'error' }) + if (errors?.length) { + sendToast('Failed to delete message', { type: 'error' }) + return } + sendToast('Your message was deleted', { type: 'info' }) }, + onError: (error) => { + console.error('Delete message error:', error) + sendToast('Failed to delete message', { type: 'error' }) + } }) }
🧹 Nitpick comments (1)
packages/components/schema.graphql (1)
162-168: Payload Type for Deletion MutationThe added
ChatRoomDeleteMessagePayloadtype is well structured. It includes fields for capturing errors, debugging data, and returning thedeletedMessageas aMessageEdge, along with theclientMutationId. Consider documenting the nullability ofdeletedMessageto clarify its expected state when the deletion operation fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/__shared__/web/ActionsOverlay/DefaultHoverOverlay/index.tsx(2 hunks)packages/components/modules/__shared__/web/ActionsOverlay/DefaultHoverOverlay/types.ts(1 hunks)packages/components/modules/__shared__/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/index.tsx(2 hunks)packages/components/modules/__shared__/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/types.ts(1 hunks)packages/components/modules/__shared__/web/ActionsOverlay/__storybook__/ActionsOverlay.mdx(2 hunks)packages/components/modules/__shared__/web/ActionsOverlay/__storybook__/stories.tsx(2 hunks)packages/components/modules/__shared__/web/ActionsOverlay/index.tsx(6 hunks)packages/components/modules/__shared__/web/ActionsOverlay/types.ts(2 hunks)packages/components/modules/comments/web/CommentItem/index.tsx(1 hunks)packages/components/modules/messages/common/graphql/fragments/MessageItem.ts(1 hunks)packages/components/modules/messages/common/graphql/mutations/MessageDelete.ts(1 hunks)packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx(1 hunks)packages/components/modules/messages/common/index.ts(1 hunks)packages/components/modules/messages/web/ChatRoomsList/ChatRoomItem/index.tsx(1 hunks)packages/components/modules/messages/web/ChatRoomsList/index.tsx(5 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/SystemMessage/styled.tsx(1 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx(4 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/types.ts(1 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/modules/messages/web/MessagesList/MessagesGroup/SystemMessage/styled.tsx
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/components/CHANGELOG.md
- packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/types.ts
- packages/components/package.json
- packages/components/modules/messages/common/graphql/fragments/MessageItem.ts
- packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx
- packages/components/modules/comments/web/CommentItem/index.tsx
- packages/components/modules/shared/web/ActionsOverlay/DefaultHoverOverlay/index.tsx
- packages/components/modules/shared/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/types.ts
- packages/components/modules/shared/web/ActionsOverlay/DefaultHoverOverlay/types.ts
- packages/components/modules/messages/common/index.ts
- packages/components/modules/shared/web/ActionsOverlay/storybook/ActionsOverlay.mdx
- packages/components/modules/messages/web/ChatRoomsList/ChatRoomItem/index.tsx
- packages/components/modules/shared/web/ActionsOverlay/ThreeDotsMenuHoverOverlay/index.tsx
- packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx
- packages/components/modules/messages/common/graphql/mutations/MessageDelete.ts
- packages/components/modules/shared/web/ActionsOverlay/types.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (12)
packages/components/modules/__shared__/web/ActionsOverlay/__storybook__/stories.tsx (1)
22-22: LGTM!The prop renaming from
enableDeletetoshowDeleteButtonis consistent with the component's updated API.Also applies to: 45-45
packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (2)
64-68: LGTM!Good implementation of message color handling for deleted state.
55-57: LGTM!Well-implemented dynamic delete dialog content that adapts based on chat type.
packages/components/modules/messages/web/ChatRoomsList/index.tsx (3)
96-101: LGTM!Well-documented workaround for the Virtuoso component's endReached event bug. The solution to remount the component is clean and effective.
56-63: LGTM!Good addition of
network-onlyfetch policy to ensure fresh data is fetched when searching or changing tabs.Also applies to: 70-77, 85-92
151-160: LGTM!Clean refactoring of empty state handling with simplified conditional logic.
packages/components/modules/__shared__/web/ActionsOverlay/index.tsx (2)
25-28: LGTM!Good improvements to the component's API:
- More semantic prop names (
showDeleteButtoninstead ofenableDelete)- Granular control over delete button state with
disableDeleteButton- Enhanced dialog customization with
DeleteDialogPropsAlso applies to: 33-33
177-177: LGTM!Good simplification of the delete button label to just "Delete" instead of including the title.
packages/components/schema.graphql (4)
157-160: Input Type for Deletion MutationThe new
ChatRoomDeleteMessageInputis defined correctly with a requiredidand an optionalclientMutationId. This structure aligns with the Relay mutation pattern.
665-665: Deletion Flag on Message TypeThe addition of the
deleted: Boolean!field in theMessagetype enables tracking the deletion state of messages. Please ensure that your resolvers correctly update this flag so that the UI can distinguish between "You deleted this message" and "This message was deleted." for other users.
717-717: Addition of Delete MutationThe mutation
chatRoomDeleteMessage(input: ChatRoomDeleteMessageInput!): ChatRoomDeleteMessagePayloadis now part of theMutationtype. Make sure that the backend resolver correctly implements the deletion logic, updates the message’s status, and returns the expected payload.
193-195: Renaming of Subscription for Message EventsThe new type
ChatRoomOnMessagereplaces the previous subscription name (chatRoomOnNewMessage). This renaming broadens the event scope and should be reflected consistently in the subscription resolvers and client-side consumers.
pt-tsl
force-pushed
the
BA-2211-delete-chat-message
branch
from
473e062 to
34b9312
Compare
|
Acceptance Criteria
Guardrail
The snackbar will not have the action button for now
Design Link: https://www.figma.com/design/XRD6wSl1m8Kz6XUcAy5CLp/BaseApp---WEB?node-id=7763-127162&t=MOMbprMUFediIBZh-0
Note: The snackbar has been implemented in another PR
#207
Summary by CodeRabbit
New Features
Updates