Skip to content

fix: prevent thread sidebar from getting stuck #40048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ZunedKhan07 wants to merge 1 commit into from
Closed

fix: prevent thread sidebar from getting stuck #40048

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 62 additions & 77 deletions apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {
useUserId,
useRoomToolbox,
} from '@rocket.chat/ui-contexts';
import { createPortal } from 'react-dom';

import ThreadChat from './components/ThreadChat';
import ThreadSkeleton from './components/ThreadSkeleton';
Expand All @@ -42,6 +41,13 @@ const Thread = ({ tmid }: ThreadProps) => {
closeTab();
},
});
if (
mainMessageQueryResult.isSuccess &&
(!mainMessageQueryResult.data || mainMessageQueryResult.data.t === 'rm')
) {
closeTab();
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with Cubic

return null;
}
Comment on lines +44 to +50
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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:

🏁 Script executed:

head -100 apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx | tail -60

Repository: 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.


const t = useTranslation();
const dispatchToastMessage = useToastMessageDispatch();
Expand Down Expand Up @@ -83,85 +89,64 @@ const Thread = ({ tmid }: ThreadProps) => {
closeTab();
};

const isExpanded = canExpand && expanded;
const portalTarget = isExpanded ? document.getElementById('main-content') : null;

const threadContent = (
<Contextualbar
rcx-thread-view
className={
isExpanded
? css`
max-width: 855px !important;
@media (min-width: 780px) and (max-width: 1135px) {
max-width: calc(100% - var(--sidebar-width)) !important;
}
`
: undefined
}
position='absolute'
display='flex'
flexDirection='column'
width='full'
overflow='hidden'
zIndex={100}
insetBlock={0}
border='none'
>
<ContextualbarHeader>
<ContextualbarBack onClick={handleGoBack} />
{(mainMessageQueryResult.isLoading && <Skeleton width='100%' />) ||
(mainMessageQueryResult.isSuccess && <ThreadTitle mainMessage={mainMessageQueryResult.data} />) ||
null}
<ContextualbarActions>
{canExpand && (
<ContextualbarAction
name={expanded ? 'arrow-collapse' : 'arrow-expand'}
title={expanded ? t('Collapse') : t('Expand')}
onClick={handleToggleExpand}
/>
)}
<ContextualbarAction
name={following ? 'bell' : 'bell-off'}
title={following ? t('Following') : t('Not_Following')}
disabled={!mainMessageQueryResult.isSuccess || toggleFollowingMutation.isPending}
onClick={handleToggleFollowing}
/>
<ContextualbarClose onClick={handleClose} />
</ContextualbarActions>
</ContextualbarHeader>

{(mainMessageQueryResult.isLoading && <ThreadSkeleton />) ||
(mainMessageQueryResult.isSuccess && (
<ChatProvider tmid={tmid}>
<ThreadChat mainMessage={mainMessageQueryResult.data} />
</ChatProvider>
)) ||
null}
</Contextualbar>
);

return (
<ContextualbarDialog>
<ContextualbarInnerContent>
{portalTarget ? (
createPortal(
<>
<ModalBackdrop
className={css`
position: absolute !important;
`}
onClick={handleBackdropClick}
/>
{threadContent}
</>,
portalTarget,
)
) : (
<Box flexGrow={1} position='relative'>
{threadContent}
</Box>
)}
{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'}
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Suggested change
position={expanded ? 'fixed' : 'absolute'}
position={canExpand && expanded ? 'fixed' : 'absolute'}
Fix with Cubic

Comment on lines +95 to +109
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

display='flex'
flexDirection='column'
width='full'
overflow='hidden'
zIndex={100}
insetBlock={0}
border='none'
>
<ContextualbarHeader>
<ContextualbarBack onClick={handleGoBack} />
{(mainMessageQueryResult.isLoading && <Skeleton width='100%' />) ||
(mainMessageQueryResult.isSuccess && <ThreadTitle mainMessage={mainMessageQueryResult.data} />) ||
null}
<ContextualbarActions>
{canExpand && (
<ContextualbarAction
name={expanded ? 'arrow-collapse' : 'arrow-expand'}
title={expanded ? t('Collapse') : t('Expand')}
onClick={handleToggleExpand}
/>
)}
<ContextualbarAction
name={following ? 'bell' : 'bell-off'}
title={following ? t('Following') : t('Not_Following')}
disabled={!mainMessageQueryResult.isSuccess || toggleFollowingMutation.isPending}
onClick={handleToggleFollowing}
/>
<ContextualbarClose onClick={handleClose} />
</ContextualbarActions>
</ContextualbarHeader>

{(mainMessageQueryResult.isLoading && <ThreadSkeleton />) ||
(mainMessageQueryResult.isSuccess && (
<ChatProvider tmid={tmid}>
<ThreadChat mainMessage={mainMessageQueryResult.data} />
</ChatProvider>
)) ||
null}
</Contextualbar>
</Box>
</ContextualbarInnerContent>
</ContextualbarDialog>
);
Expand Down