feat: Disable confirm button while requesting delete message #37276
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 63f096b The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughIntroduces a new DeleteMessageConfirmModal component and reworks the deletion flow to delegate deletion logic and UI state to that modal. requestMessageDeletion now opens the modal (passing resolve/reject). Tests and page-object helpers updated; confirm button is disabled while deletion is in progress. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RoomUI as Room UI
participant Modal as DeleteMessageConfirmModal
participant API as Server/API
participant Toast
User->>RoomUI: click "Delete" on message
RoomUI->>Modal: open(message, room, resolve, reject)
Modal->>User: show confirmation (confirm enabled)
User->>Modal: click confirm
Note right of Modal #f8f0d8: confirm button -> disabled
Modal->>API: canDeleteMessage && deleteMessage
alt deletion succeeds
API-->>Modal: success
Modal->>RoomUI: resolve()
Modal->>Toast: show success
Modal->>RoomUI: stop editing / refocus composer (if needed)
else deletion fails
API-->>Modal: error
Modal->>Toast: show error
Modal->>RoomUI: reject(error)
end
Modal->>RoomUI: onCancel / cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37276 +/- ##
===========================================
+ Coverage 67.92% 67.93% +0.01%
===========================================
Files 3356 3357 +1
Lines 114894 114897 +3
Branches 20754 20794 +40
===========================================
+ Hits 78040 78057 +17
+ Misses 34171 34155 -16
- Partials 2683 2685 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/client/views/room/modals/DeleteMessageConfirmModal/DeleteMessageConfirmModal.tsx (1)
59-59: Use JSX children syntax instead of children prop.React's canonical way to pass children is to use JSX elements between the component tags rather than the
childrenprop.Apply this diff to follow React best practices:
return ( <GenericModal variant='danger' title={t('Are_you_sure')} confirmText={t('Yes_delete_it')} - children={room ? t('The_message_is_a_discussion_you_will_not_be_able_to_recover') : t('You_will_not_be_able_to_recover')} onConfirm={deleteMessageMutation.mutate} onCancel={onCancel} confirmDisabled={deleteMessageMutation.isPending} - /> + > + {room ? t('The_message_is_a_discussion_you_will_not_be_able_to_recover') : t('You_will_not_be_able_to_recover')} + </GenericModal> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.changeset/moody-spoons-press.md(1 hunks)apps/meteor/client/lib/chats/flows/requestMessageDeletion.ts(2 hunks)apps/meteor/client/views/room/modals/DeleteMessageConfirmModal/DeleteMessageConfirmModal.tsx(1 hunks)apps/meteor/client/views/room/modals/DeleteMessageConfirmModal/index.ts(1 hunks)apps/meteor/tests/e2e/message-actions.spec.ts(1 hunks)apps/meteor/tests/e2e/page-objects/fragments/home-content.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/meteor/tests/e2e/page-objects/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Utilize and place Page Object implementations under apps/meteor/tests/e2e/page-objects/
Files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/tests/e2e/message-actions.spec.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/tests/e2e/message-actions.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Use descriptive test names that clearly communicate expected behavior
Use test.beforeAll() and test.afterAll() for setup and teardown
Use test.step() to organize complex test scenarios
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) consistently
Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Maintain test isolation between test cases
Ensure a clean state for each test execution
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/message-actions.spec.ts
🧠 Learnings (15)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize and place Page Object implementations under apps/meteor/tests/e2e/page-objects/
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Follow the Page Object Model pattern consistently
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Implement proper wait strategies for dynamic content
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Follow DRY by extracting reusable logic into helper functions or page objects
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
PR: RocketChat/Rocket.Chat#36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure a clean state for each test execution
Applied to files:
apps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Avoid code comments in the implementation
Applied to files:
apps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
apps/meteor/tests/e2e/message-actions.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/tests/e2e/message-actions.spec.ts
🧬 Code graph analysis (1)
apps/meteor/client/views/room/modals/DeleteMessageConfirmModal/DeleteMessageConfirmModal.tsx (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/client/lib/chats/ChatAPI.ts (1)
ChatAPI(108-163)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
🪛 Biome (2.1.2)
apps/meteor/client/views/room/modals/DeleteMessageConfirmModal/DeleteMessageConfirmModal.tsx
[error] 59-59: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🔇 Additional comments (5)
apps/meteor/tests/e2e/message-actions.spec.ts (1)
119-126: LGTM! Test refactoring improves maintainability.The test name is now more descriptive, and extracting the deletion steps into a helper function follows the DRY principle while maintaining the same test coverage.
Based on coding guidelines
apps/meteor/client/lib/chats/flows/requestMessageDeletion.ts (1)
31-42: LGTM! Clean separation of concerns.The refactoring delegates deletion logic to the modal component while maintaining the promise-based control flow. This improves maintainability by having the modal own both the UI state and deletion logic.
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts (2)
428-432: LGTM! Improved selector strategy.The refactoring to use role-based selectors scoped to
lastUserMessagefollows the coding guidelines and provides more stable, semantic element targeting.Based on coding guidelines
565-565: LGTM! Tests the core fix.The assertion validates that the delete button becomes disabled during the deletion request, which directly addresses the double-click issue described in the PR objectives.
apps/meteor/client/views/room/modals/DeleteMessageConfirmModal/DeleteMessageConfirmModal.tsx (1)
62-62: LGTM! Core fix correctly implemented.The
confirmDisabled={deleteMessageMutation.isPending}prop correctly disables the confirm button while the deletion request is in progress, preventing the double-click issue described in the PR objectives.
apps/meteor/client/views/room/modals/DeleteMessageConfirmModal/DeleteMessageConfirmModal.tsx
Show resolved
Hide resolved
dougfabris
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
dougfabris
added
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
CORE-1213
Summary by CodeRabbit