fix: prevent thread sidebar from getting stuck#40048
fix: prevent thread sidebar from getting stuck#40048ZunedKhan07 wants to merge 1 commit intoRocketChat:developfrom
Conversation
|
|
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 |
WalkthroughSingle file modification in the Thread component that removes portal-based rendering, introduces a validation guard for missing or removed messages, and refactors positioning logic for expanded thread display states. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx">
<violation number="1" location="apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:48">
P1: Added conditional early return before later hooks breaks hook ordering and performs `closeTab()` side effect during render.</violation>
<violation number="2" location="apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:109">
P2: Gate fixed positioning with `canExpand` as well, otherwise persisted `expanded` state can force expanded layout when expansion is unavailable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| mainMessageQueryResult.isSuccess && | ||
| (!mainMessageQueryResult.data || mainMessageQueryResult.data.t === 'rm') | ||
| ) { | ||
| closeTab(); |
There was a problem hiding this comment.
P1: Added conditional early return before later hooks breaks hook ordering and performs closeTab() side effect during render.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx, line 48:
<comment>Added conditional early return before later hooks breaks hook ordering and performs `closeTab()` side effect during render.</comment>
<file context>
@@ -42,6 +41,13 @@ const Thread = ({ tmid }: ThreadProps) => {
+ mainMessageQueryResult.isSuccess &&
+ (!mainMessageQueryResult.data || mainMessageQueryResult.data.t === 'rm')
+ ) {
+ closeTab();
+ return null;
+ }
</file context>
| ` | ||
| : undefined | ||
| } | ||
| position={expanded ? 'fixed' : 'absolute'} |
There was a problem hiding this comment.
P2: Gate fixed positioning with canExpand as well, otherwise persisted expanded state can force expanded layout when expansion is unavailable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx, line 109:
<comment>Gate fixed positioning with `canExpand` as well, otherwise persisted `expanded` state can force expanded layout when expansion is unavailable.</comment>
<file context>
@@ -83,85 +89,64 @@ const Thread = ({ tmid }: ThreadProps) => {
+ `
+ : undefined
+ }
+ position={expanded ? 'fixed' : 'absolute'}
+ display='flex'
+ flexDirection='column'
</file context>
| position={expanded ? 'fixed' : 'absolute'} | |
| position={canExpand && expanded ? 'fixed' : 'absolute'} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Around line 95-109: Derive a single boolean isExpanded = canExpand && expanded
and use that everywhere instead of checking expanded alone: use isExpanded for
the ModalBackdrop render condition, for the Box position prop (static vs
relative), and for the Contextualbar position prop (fixed vs absolute) as well
as the className branch so the fixed layout never activates when canExpand is
false; replace occurrences of expanded in these layout branches with isExpanded
to ensure expansion is gated by canExpand.
- Around line 44-50: The early return based on mainMessageQueryResult
short-circuits hook calls and calls closeTab during render; instead derive a
boolean (e.g., shouldCloseDeletedThread = mainMessageQueryResult.isSuccess &&
(!mainMessageQueryResult.data || mainMessageQueryResult.data.t === 'rm')) before
any hooks, then call useTranslation, useToastMessageDispatch,
useLayoutContextualBarExpanded, useLocalStorage, useUserId, and
useToggleFollowingThreadMutation unconditionally; move the closeTab() call into
a useEffect that watches shouldCloseDeletedThread and runs closeTab() when it
becomes true; finally, after the hooks block, conditionally return null if
shouldCloseDeletedThread is true.
🪄 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: 88cdad48-c860-4465-8ff6-cfb99cbd13b0
📒 Files selected for processing (1)
apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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 (5)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
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').
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-02-24T19:36:55.089Z
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').
Applied to files:
apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx
📚 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-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
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
| if ( | ||
| mainMessageQueryResult.isSuccess && | ||
| (!mainMessageQueryResult.data || mainMessageQueryResult.data.t === 'rm') | ||
| ) { | ||
| closeTab(); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
React documentation: can a component conditionally return before hooks that were called on a previous render, and are side effects such as state updates/navigation allowed during render?
💡 Result:
No, a React component cannot conditionally return before hooks that were called on a previous render. Hooks must be called at the top level of the component, in the same order on every render, before any early returns. A conditional early return before all hooks would violate the Rules of Hooks, causing React to lose track of hook state and leading to bugs or errors like "Rendered fewer hooks than expected." Side effects such as state updates (e.g., setState) or navigation during render are not allowed. Render must be a pure function that computes and returns JSX without mutating state, DOM, or performing I/O. Such side effects belong in event handlers or useEffect/useLayoutEffect. Workarounds: - For conditional logic before hooks: Split into an outer component that checks the condition and returns null or renders an inner component containing the hooks. - For side effects: Move them to useEffect (for post-render) or event handlers.
Citations:
- 1: https://legacy.reactjs.org/docs/hooks-rules.html
- 2: https://react.dev/reference/eslint-plugin-react-hooks/lints/rules-of-hooks
- 3: https://react.dev/reference/rules/rules-of-hooks
- 4: https://stackoverflow.com/questions/54938236/can-you-early-return-with-react-hooks
- 5: https://beta.reactjs.org/learn/synchronizing-with-effects
- 6: Warning: Can't perform a React state update on a component that hasn't mounted yet. This indicates that you have a side-effect in your render function that asynchronously later calls tries to update the component. Move this work to useEffect instead. software-mansion/react-native-screens#2876
🏁 Script executed:
head -100 apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx | tail -60Repository: RocketChat/Rocket.Chat
Length of output: 1481
Move this deleted-message close path below the hook declarations.
The conditional check at lines 44–50 short-circuits before useTranslation, useToastMessageDispatch, useLayoutContextualBarExpanded, useLocalStorage, useUserId, and useToggleFollowingThreadMutation. This violates React's Rules of Hooks—if the query state changes on a later render, the component will call fewer hooks than before, causing React to lose track of hook state. Additionally, line 48 performs a render-phase side effect by calling closeTab(), which is not allowed.
Derive a shouldCloseDeletedThread flag from the query result, call all hooks unconditionally, move the side effect to a useEffect, and conditionally return null after the hook block.
🤖 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` around lines
44 - 50, The early return based on mainMessageQueryResult short-circuits hook
calls and calls closeTab during render; instead derive a boolean (e.g.,
shouldCloseDeletedThread = mainMessageQueryResult.isSuccess &&
(!mainMessageQueryResult.data || mainMessageQueryResult.data.t === 'rm')) before
any hooks, then call useTranslation, useToastMessageDispatch,
useLayoutContextualBarExpanded, useLocalStorage, useUserId, and
useToggleFollowingThreadMutation unconditionally; move the closeTab() call into
a useEffect that watches shouldCloseDeletedThread and runs closeTab() when it
becomes true; finally, after the hooks block, conditionally return null if
shouldCloseDeletedThread is true.
| {canExpand && expanded && <ModalBackdrop onClick={handleBackdropClick} />} | ||
| <Box flexGrow={1} position={expanded ? 'static' : 'relative'}> | ||
| <Contextualbar | ||
| rcx-thread-view | ||
| className={ | ||
| canExpand && expanded | ||
| ? css` | ||
| max-width: 855px !important; | ||
| @media (min-width: 780px) and (max-width: 1135px) { | ||
| max-width: calc(100% - var(--sidebar-width)) !important; | ||
| } | ||
| ` | ||
| : undefined | ||
| } | ||
| position={expanded ? 'fixed' : 'absolute'} |
There was a problem hiding this comment.
Gate the fixed positioning with canExpand too.
Line 95 uses canExpand && expanded, but Lines 96 and 109 still switch the layout to static/fixed when expanded alone is true. Because expanded is persisted in local storage, reopening the thread in a layout where expansion is unavailable can still render the fixed view without its backdrop or expand/collapse affordance. Derive a single isExpanded = canExpand && expanded and use it for all layout branches.
💡 Suggested fix
const canExpand = useLayoutContextualBarExpanded();
const [expanded, setExpanded] = useLocalStorage('expand-threads', false);
+const isExpanded = canExpand && expanded;
...
- {canExpand && expanded && <ModalBackdrop onClick={handleBackdropClick} />}
- <Box flexGrow={1} position={expanded ? 'static' : 'relative'}>
+ {isExpanded && <ModalBackdrop onClick={handleBackdropClick} />}
+ <Box flexGrow={1} position={isExpanded ? 'static' : 'relative'}>
<Contextualbar
rcx-thread-view
className={
- canExpand && expanded
+ isExpanded
? css`
max-width: 855px !important;
`@media` (min-width: 780px) and (max-width: 1135px) {
max-width: calc(100% - var(--sidebar-width)) !important;
}
`
: undefined
}
- position={expanded ? 'fixed' : 'absolute'}
+ position={isExpanded ? 'fixed' : 'absolute'}
display='flex'🤖 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` around lines
95 - 109, Derive a single boolean isExpanded = canExpand && expanded and use
that everywhere instead of checking expanded alone: use isExpanded for the
ModalBackdrop render condition, for the Box position prop (static vs relative),
and for the Contextualbar position prop (fixed vs absolute) as well as the
className branch so the fixed layout never activates when canExpand is false;
replace occurrences of expanded in these layout branches with isExpanded to
ensure expansion is gated by canExpand.
|
Hi there, thanks for the contribution! 🚀 💯 Closing this because the changes doesn't reflect the description of the pull request so that's an invalid contribution. Questions? Help needed? Feature Requests?
|
2 participants
Proposed changes
This PR fixes the issue where clicking on a "Message removed" placeholder opens a thread view that becomes unresponsive and cannot be closed.
Fix Details:
Before:
After:
Issue(s)
Closes #39908
Steps to test or reproduce
Go to any channel with a deleted message ("Message removed").
Click on the deleted message.
Verify that no thread opens.
Click on a valid message thread.
Switch between multiple threads.
Ensure:
Further comments
This fix ensures better UI consistency and prevents users from getting stuck in an unresponsive thread view.
The solution avoids unnecessary rendering of thread components for invalid messages and ensures proper cleanup of thread state.
No breaking changes introduced.
Summary by CodeRabbit