fix: close thread sidebar when main message is not found or already deleted#40042
fix: close thread sidebar when main message is not found or already deleted#40042adwait50 wants to merge 2 commits intoRocketChat:developfrom
Conversation
…eleted When a user clicks on a 'Message removed' placeholder that was part of a thread, the thread sidebar would open but become stuck — the close button would not work and switching to other threads would fail. This happened because useThreadMainMessageQuery throws 'Invalid main message' when the message no longer exists, putting the query in an error state. The component only handled isLoading and isSuccess states, never isError, so the sidebar rendered with no content and no cleanup. Fix: add a useEffect that calls closeTab() when the query enters an error state, ensuring the sidebar closes immediately if the thread main message cannot be found. Fixes RocketChat#40036
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughA single line import was added to the Thread component to enable proper state management for handling thread UI transitions, particularly addressing state persistence when interacting with deleted messages. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx`:
- Line 23: Add a useEffect inside the Thread component that watches
mainMessageQueryResult.isError and calls closeTab() when it becomes true; locate
the Thread component (where mainMessageQueryResult and closeTab are
defined/used) and after the existing hooks add: useEffect(() => { if
(mainMessageQueryResult?.isError) closeTab(); },
[mainMessageQueryResult?.isError, closeTab]); to ensure the sidebar closes when
the main message query errors out.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d5cf5de-baf7-4bd3-a96b-edefd79eafee
📒 Files selected for processing (1)
apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx
| useRoomToolbox, | ||
| } from '@rocket.chat/ui-contexts'; | ||
| import { createPortal } from 'react-dom'; | ||
| import { useEffect } from 'react'; |
There was a problem hiding this comment.
Critical: useEffect imported but never used; the PR's stated fix is not implemented.
The import is added but there is no corresponding useEffect hook in the component. According to the PR description, the fix should add a useEffect that monitors mainMessageQueryResult.isError and calls closeTab() to prevent the stuck sidebar when the thread's main message is deleted. However, this logic is completely missing from the implementation.
Currently, the component only handles isLoading and isSuccess states (lines 114-116, 135-141), leaving the isError state unhandled—which is the root cause of the bug this PR claims to fix.
🔧 Implement the missing error handling
Add the useEffect hook after line 45 to close the sidebar when the query fails:
const mainMessageQueryResult = useThreadMainMessageQuery(tmid, {
onDelete: () => {
closeTab();
},
});
+
+ useEffect(() => {
+ if (mainMessageQueryResult.isError) {
+ closeTab();
+ }
+ }, [mainMessageQueryResult.isError, closeTab]);
const t = useTranslation();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx` at line 23,
Add a useEffect inside the Thread component that watches
mainMessageQueryResult.isError and calls closeTab() when it becomes true; locate
the Thread component (where mainMessageQueryResult and closeTab are
defined/used) and after the existing hooks add: useEffect(() => { if
(mainMessageQueryResult?.isError) closeTab(); },
[mainMessageQueryResult?.isError, closeTab]); to ensure the sidebar closes when
the main message query errors out.
1 participant
Problem
When a user clicks on a "Message removed" placeholder that was part of a thread, the thread sidebar opens but becomes completely stuck:
Root Cause
useThreadMainMessageQuerythrows'Invalid main message'when the thread's main message no longer exists (already deleted). This puts the query into anisErrorstate.The
Threadcomponent only handledisLoadingandisSuccessstates — theisErrorstate was never accounted for, so the sidebar rendered with no content and no way to close or recover without a page reload.Fix
Added a
useEffectinThread.tsxthat callscloseTab()whenever the query enters an error state. This ensures the sidebar closes immediately and cleanly if the thread main message cannot be found, instead of getting stuck.Steps to Test
Related
Closes #40036
Summary by CodeRabbit