[WEB-4981] fix: analytics modal portal#7852
Conversation
|
Warning Rate limit exceeded@anmolsinghbhatia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors the Work Items modal to use a new ModalPortal component. Introduces a portal package with constants, types, ModalPortal, and PortalWrapper, plus Storybook stories. Updates Propel build/exports to expose the portal entry. Adjusts tsdown config to include the portal module. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant WorkItemsModal as WorkItemsModal
participant ModalPortal as ModalPortal
participant PortalWrapper as PortalWrapper
participant DOM as Document Body
User->>WorkItemsModal: Open modal
WorkItemsModal->>ModalPortal: isOpen, width/position, fullScreen, onClose
ModalPortal->>PortalWrapper: portalId (default "full-screen-portal")
PortalWrapper->>DOM: Ensure/find portal container
PortalWrapper-->>ModalPortal: Portal node ready
ModalPortal->>DOM: Render overlay + content in portal
alt Overlay click enabled
User-->>ModalPortal: Click overlay
ModalPortal->>WorkItemsModal: onClose()
end
alt Escape enabled
User-->>ModalPortal: Press Escape
ModalPortal->>WorkItemsModal: onClose()
end
User->>WorkItemsModal: Toggle fullscreen
WorkItemsModal->>ModalPortal: fullScreen=true/false (re-render with styles)
User->>WorkItemsModal: Close modal
WorkItemsModal->>ModalPortal: isOpen=false
ModalPortal-->>DOM: Unmount content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
…ane/plane into fix-analytics-modal-portal
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the analytics modal portal behavior by implementing a new reusable ModalPortal component to replace the previous manual portal implementation. The issue was that clicking outside the modal did not close it as expected.
Key Changes
- Created a new portal component system with proper overlay click handling and keyboard escape functionality
- Replaced manual
createPortalusage in analytics modal with the newModalPortalcomponent - Added comprehensive portal components with configurable width, position, and interaction options
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/propel/src/portal/* | New portal component system with ModalPortal and PortalWrapper components |
| packages/propel/package.json | Added portal export to package.json |
| packages/propel/tsdown.config.ts | Added portal to build configuration |
| apps/web/core/components/analytics/work-items/modal/index.tsx | Replaced manual portal implementation with new ModalPortal component |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/propel/src/portal/modal-portal.tsx (2)
101-106: Add aria-modal for accessibility.Mark the dialog as modal for assistive tech.
Apply this diff:
- const content = ( - <div - className={cn("fixed inset-0 h-full w-full overflow-y-auto", className)} - style={{ zIndex: MODAL_Z_INDEX }} - role="dialog" - > + const content = ( + <div + className={cn("fixed inset-0 h-full w-full overflow-y-auto", className)} + style={{ zIndex: MODAL_Z_INDEX }} + role="dialog" + aria-modal="true" + >
59-60: Set initial focus inside the modal to improve keyboard UX.Leverage contentRef to focus the modal container when opened.
Apply these diffs:
- Add focus on open:
+ useEffect(() => { + if (isOpen) { + contentRef.current?.focus(); + } + }, [isOpen]);
- Make the content container focusable:
- <div ref={contentRef} className={cn(modalClasses)} style={{ zIndex: MODAL_Z_INDEX + 1 }} role="document"> + <div + ref={contentRef} + className={cn(modalClasses)} + style={{ zIndex: MODAL_Z_INDEX + 1 }} + role="document" + tabIndex={-1} + >Also applies to: 114-116, 99-99
packages/propel/src/portal/portal-wrapper.tsx (1)
1-4: Use isomorphic layout effect to avoid SSR warnings.Prevents "useLayoutEffect does nothing on the server" warnings.
Apply this diff:
-import React, { useLayoutEffect, useState, useMemo } from "react"; +import React, { useLayoutEffect, useEffect, useState, useMemo } from "react"; @@ -export const PortalWrapper: React.FC<PortalWrapperProps> = ({ +const useIsomorphicLayoutEffect = typeof window !== "undefined" ? useLayoutEffect : useEffect; + +export const PortalWrapper: React.FC<PortalWrapperProps> = ({ @@ - useLayoutEffect(() => { + useIsomorphicLayoutEffect(() => {Also applies to: 36-36
packages/propel/src/portal/constants.ts (1)
14-25: Harden enum-to-class mappings withsatisfies.Adding
satisfies Record<…>keeps the literal values while forcing the object to stay in sync with the enum whenever a new width/position is introduced.export const PORTAL_WIDTH_CLASSES = { [EPortalWidth.QUARTER]: "w-1/4 min-w-80 max-w-96", [EPortalWidth.HALF]: "w-1/2 min-w-96 max-w-2xl", [EPortalWidth.THREE_QUARTER]: "w-3/4 min-w-96 max-w-5xl", [EPortalWidth.FULL]: "w-full", -} as const; +} as const satisfies Record<EPortalWidth, string>; export const PORTAL_POSITION_CLASSES = { [EPortalPosition.LEFT]: "left-0", [EPortalPosition.RIGHT]: "right-0", [EPortalPosition.CENTER]: "left-1/2 -translate-x-1/2", -} as const; +} as const satisfies Record<EPortalPosition, string>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web/core/components/analytics/work-items/modal/index.tsx(2 hunks)packages/propel/package.json(1 hunks)packages/propel/src/portal/constants.ts(1 hunks)packages/propel/src/portal/index.ts(1 hunks)packages/propel/src/portal/modal-portal.tsx(1 hunks)packages/propel/src/portal/portal-wrapper.tsx(1 hunks)packages/propel/src/portal/portal.stories.tsx(1 hunks)packages/propel/src/portal/types.ts(1 hunks)packages/propel/tsdown.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/web/core/components/analytics/work-items/modal/index.tsx (2)
packages/propel/src/portal/modal-portal.tsx (1)
ModalPortal(44-121)apps/web/core/components/analytics/work-items/modal/content.tsx (1)
WorkItemsModalMainContent(23-81)
packages/propel/src/portal/portal.stories.tsx (2)
packages/propel/src/portal/modal-portal.tsx (1)
ModalPortal(44-121)packages/propel/src/portal/portal-wrapper.tsx (1)
PortalWrapper(25-84)
packages/propel/src/portal/portal-wrapper.tsx (2)
packages/propel/src/portal/types.ts (1)
PortalWrapperProps(8-13)packages/propel/src/portal/constants.ts (1)
DEFAULT_PORTAL_ID(27-27)
packages/propel/src/portal/modal-portal.tsx (3)
packages/propel/src/portal/types.ts (1)
ModalPortalProps(15-27)packages/propel/src/portal/constants.ts (4)
DEFAULT_PORTAL_ID(27-27)PORTAL_WIDTH_CLASSES(14-19)PORTAL_POSITION_CLASSES(21-25)MODAL_Z_INDEX(28-28)packages/propel/src/portal/portal-wrapper.tsx (1)
PortalWrapper(25-84)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Build and lint web apps
🔇 Additional comments (6)
packages/propel/package.json (1)
38-38: Portal subpath export added — looks good. Please verify TS type resolution.The export path is correct and aligns with the new tsdown entry.
Please confirm that consumers can import both values and types from "@plane/propel/portal" (e.g., tsc resolves index.d.ts for the subpath). If not, consider conditional exports with a "types" field per subpath.
packages/propel/tsdown.config.ts (1)
24-24: Build entry added — LGTM.Including "src/portal/index.ts" in tsdown ensures dist/portal/* is emitted alongside d.ts.
packages/propel/src/portal/index.ts (1)
1-4: Central re-exports are clean and minimal.This consolidates the public API as expected.
apps/web/core/components/analytics/work-items/modal/index.tsx (2)
34-41: Switch to ModalPortal fixes outside-click close behavior — nice.Passing onClose to ModalPortal with overlay defaults addresses the bug.
Manually verify: overlay click and Escape key both close; fullscreen toggle persists across re-open as intended (you reset it on close).
43-51: Container styling consistent across peek/fullscreen.The conditional border/rounding works with the portal’s fullScreen handling.
packages/propel/src/portal/types.ts (1)
15-27: Public types look good.Once components import these interfaces (see suggestions), the API surface will be consistent.
Description
This PR fixes the behaviour of the analytics modal portal. Earlier, clicking outside the modal did not work as expected.
Type of Change
Media
Summary by CodeRabbit