fix: prevent scroll jump when closing thread in narrow layout - #41159
fix: prevent scroll jump when closing thread in narrow layout#41159sidd19898 wants to merge 3 commits into
Conversation
In narrow/responsive layout, the contextual bar aside was a flex child that took 100% width, squeezing the main chat body to 0 width. When the thread closed and the aside unmounted, the body expanded back, causing the VList virtualizer to re-measure and reset scroll position to top. Fix by wrapping the aside in an absolutely positioned Box when contextualbarPosition is absolute (narrow mode), so it overlays the body instead of affecting its flex layout. Also remove restoreFocus from ContextualbarDialog FocusScope to prevent unwanted focus restoration when closing the contextual bar. Fixes RocketChat#41132
|
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 |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (2)📚 Learning: 2026-03-27T14:52:56.865ZApplied to files:
📚 Learning: 2026-05-06T12:21:44.083ZApplied to files:
🔇 Additional comments (1)
WalkthroughRoomLayout's breakpoint filter now allows breakpoints with minViewportWidth to match even when borderBoxSize.inlineSize is undefined. The aside content rendering is updated to conditionally wrap content in an absolutely positioned Box (right-aligned, sized by contextualbarSize) when contextualbarPosition is 'absolute', otherwise rendering directly, both within Suspense. ChangesContextualbar layout updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/client/views/room/layout/RoomLayout.tsx (1)
68-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM overall; consider a named z-index token instead of a magic number.
The absolute-positioning wrap correctly isolates the aside from flex layout so the body no longer collapses/re-expands on mount/unmount, addressing the root cause described in the PR. Minor nit:
zIndex={100}is a hardcoded value — consider defining it as a constant or using fuselage's z-index tokens if available, for consistency with other overlay layers in the app.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/views/room/layout/RoomLayout.tsx` around lines 68 - 78, The absolute-positioned aside in RoomLayout correctly fixes the layout issue, but the hardcoded zIndex value should be replaced with a named token or shared constant for consistency. Update the Box wrapper in RoomLayout to use an existing fuselage z-index token or define a local constant for this overlay layer, so the layering is easier to maintain alongside other components using contextualbarPosition and aside.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui-client/src/components/Contextualbar/ContextualbarDialog.tsx`:
- Line 41: The FocusScope in ContextualbarDialog should keep focus restoration
when the contextual bar closes; adding only autoFocus is not enough. Update the
FocusScope usage in ContextualbarDialog to restore focus to the trigger on
close, and if scroll jumping is the concern, preserve restoreFocus while using
preventScroll rather than removing restoration behavior.
---
Nitpick comments:
In `@apps/meteor/client/views/room/layout/RoomLayout.tsx`:
- Around line 68-78: The absolute-positioned aside in RoomLayout correctly fixes
the layout issue, but the hardcoded zIndex value should be replaced with a named
token or shared constant for consistency. Update the Box wrapper in RoomLayout
to use an existing fuselage z-index token or define a local constant for this
overlay layer, so the layering is easier to maintain alongside other components
using contextualbarPosition and aside.
🪄 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: 51c1c9d0-e3f6-422c-a456-b932395ca77b
📒 Files selected for processing (2)
apps/meteor/client/views/room/layout/RoomLayout.tsxpackages/ui-client/src/components/Contextualbar/ContextualbarDialog.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (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:
packages/ui-client/src/components/Contextualbar/ContextualbarDialog.tsxapps/meteor/client/views/room/layout/RoomLayout.tsx
🧠 Learnings (2)
📚 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:
packages/ui-client/src/components/Contextualbar/ContextualbarDialog.tsxapps/meteor/client/views/room/layout/RoomLayout.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/ui-client/src/components/Contextualbar/ContextualbarDialog.tsxapps/meteor/client/views/room/layout/RoomLayout.tsx
There was a problem hiding this comment.
2 issues found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Fix for #41132
Summary by CodeRabbit