fix: AppMenu unmounting after backdrop click#38972
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 |
|
WalkthroughAdds a DOM wrapper using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 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 |
NagajyothiChukka
changed the title
![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/marketplace/AppMenu.tsx`:
- Around line 16-37: Remove the inline explanatory comments in AppMenu.tsx (the
comments around menuWrapperRef and the MenuV2 wrapper) to comply with the "no
comments in implementation" rule; leave the code and refs as-is (menuWrapperRef,
useAppMenu, itemsList, onAction) and move any necessary explanation into the PR
description or external docs instead of inline comments.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/views/marketplace/AppMenu.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/marketplace/AppMenu.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/views/marketplace/AppMenu.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/marketplace/AppMenu.tsx (2)
apps/meteor/client/views/marketplace/hooks/useAppMenu.tsx (2)
useAppMenu(47-492)AppMenuOption(35-41)apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts (1)
isLoading(278-281)
🔇 Additional comments (2)
apps/meteor/client/views/marketplace/AppMenu.tsx (2)
2-12: Import and props typing updates look good.
38-59: Box wrapper + ref anchoring change is solid.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
NagajyothiChukka
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/client/views/marketplace/AppMenu.tsx (2)
18-18:menuWrapperRefis attached but never read — it is dead code.The DOM-stability fix comes entirely from the
Boxwrapper element persisting in the React tree. The ref value (menuWrapperRef.current) is never accessed in any callback, effect, or render path, so it contributes nothing to the fix and can be removed.🧹 Proposed cleanup
- const menuWrapperRef = useRef<HTMLDivElement>(null); - const { isLoading, isAdminUser, sections } = useAppMenu(app, isAppDetailsPage); ... - <Box ref={menuWrapperRef} display='inline-block'> + <Box display='inline-block'>Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/marketplace/AppMenu.tsx` at line 18, Remove the unused ref declaration menuWrapperRef and any ref={menuWrapperRef} attachment so the dead code is gone, keep the Box wrapper element that provides DOM stability, and also remove the now-unused useRef import; scan for any other similarly unused menu refs and delete them too (ensure only the Box wrapper remains in the render and no callbacks/effects reference menuWrapperRef).
4-4: Remove the unusedReactdefault import.
useRefandmemoare already imported as named exports; theReactdefault is never referenced. The project is configured with TypeScript's"jsx": "react-jsx"(React 18's automatic JSX transform), which does not require the default import for either JSX or named exports.Suggested change
-import React, { memo, useRef } from 'react'; +import { memo, useRef } from 'react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/marketplace/AppMenu.tsx` at line 4, Remove the unused default import `React` from the import statement in AppMenu.tsx; keep the named imports `memo` and `useRef` (i.e., change `import React, { memo, useRef } from 'react'` to only import the named exports) so the file uses the React 18 automatic JSX transform without an unnecessary default import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/views/marketplace/AppMenu.tsx`:
- Line 18: Remove the unused ref declaration menuWrapperRef and any
ref={menuWrapperRef} attachment so the dead code is gone, keep the Box wrapper
element that provides DOM stability, and also remove the now-unused useRef
import; scan for any other similarly unused menu refs and delete them too
(ensure only the Box wrapper remains in the render and no callbacks/effects
reference menuWrapperRef).
- Line 4: Remove the unused default import `React` from the import statement in
AppMenu.tsx; keep the named imports `memo` and `useRef` (i.e., change `import
React, { memo, useRef } from 'react'` to only import the named exports) so the
file uses the React 18 automatic JSX transform without an unnecessary default
import.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/views/marketplace/AppMenu.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/marketplace/AppMenu.tsx
🧠 Learnings (3)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Avoid code comments in the implementation
Applied to files:
apps/meteor/client/views/marketplace/AppMenu.tsx
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/client/views/marketplace/AppMenu.tsx
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/views/marketplace/AppMenu.tsx
🔇 Additional comments (1)
apps/meteor/client/views/marketplace/AppMenu.tsx (1)
35-55: Core fix LGTM —Boxwrapper correctly anchors the menu in the DOM.Wrapping
MenuV2in a stableBoxcontainer prevents the component from losing its DOM position when focus is restored after a modal backdrop click, which is the correct minimal fix for the described lifecycle conflict.
3 participants
Summary
The AppMenu (three-dot menu) currently unmounts and disappears if a user opens a modal from it and then clicks the backdrop to close that modal. This is a focus-management issue where the component loses its anchor in the DOM during the modal's lifecycle when focus attempts to restore.
The Fix
I’ve wrapped the MenuV2 component in a stable element.
Why this works:
Wrapping the menu in a persistent container provides a stable DOM anchor. Even when the modal backdrop is clicked and focus is shifted, the Box ensures the menu's position in the React tree remains occupied and stable. This prevents the component from unmounting during the focus-restoration phase.
Implementation Note
I initially attempted to resolve this by passing a ref directly to MenuV2. However, since MenuV2 does not currently support forwardRef, this triggered a TypeScript error (ts2322).
By wrapping it in a , I found that the structural stability of the wrapper alone provides the necessary DOM anchor to prevent the unmounting bug. Consequently, I was able to remove the useRef logic entirely, resulting in a cleaner, logic-free fix that keeps TypeScript happy and the DOM stable.
How to Verify
Go to the Marketplace and open an App's menu (three dots).
Trigger an action that opens a modal (e.g., an action requiring an add-on or a warning).
Click the dark backdrop or press ESC to close the modal.
Result: The three-dot menu now remains visible and properly anchored in the UI.
Screenshots
Before
After
Closes #38914
Summary by CodeRabbit