refactor: Remove the global mutation observer #2788
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Walkthrough
The updates remove custom mutation observer logic from dashboard components and centralize DOM portal management within the `DokanModal` component. Both `UpgradeModal` and `VideoPopup` are refactored to use `DokanModal`, which now ensures the portal root exists and allows explicit disabling of header and footer sections. Minor logic improvements are also included for modal rendering and mutation observer efficiency.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------|
| src/admin/dashboard/components/Dashboard.tsx,<br>src/dashboard/index.tsx | Removed custom mutation observer logic that added classes and styles to modal-related DOM nodes. |
| src/admin/dashboard/pages/modules/UpgradeModal.tsx,<br>src/admin/dashboard/pages/modules/VideoPopup.tsx | Refactored to use `DokanModal` instead of the package `Modal`, simplifying modal structure and centralizing behavior. |
| src/components/modals/DokanModal.tsx | Added portal root management with `useEffect`, allowed `dialogHeader`/`dialogFooter` to be `false`, improved conditional rendering, fixed import. |
| src/intelligence/components/DokanAI.tsx | Added guard clause to mutation observer to skip logic when modal is closed; replaced modal usage with `DokanModal` and reorganized modal content structure. |
| src/admin/dashboard/pages/modules/CategorySelector.tsx,<br>src/components/WpDatePicker.tsx | Replaced `@headlessui/react` popover components with a custom `Popover` component using explicit state and anchor management. |
| src/components/Popover.tsx,<br>src/components/index.tsx | Added new `Popover` component re-exporting `@wordpress/components` Popover; exported it in component index barrel file. |
| src/admin/dashboard/style.scss | Added `.dokan-lite-module-select-category-popover` CSS class with `top-5 !important` style. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant UpgradeModal
participant DokanModal
participant DOM
User->>UpgradeModal: Triggers modal open
UpgradeModal->>DokanModal: Renders with dialogContent, dialogHeader=false, dialogFooter=false
DokanModal->>DOM: Ensures portal root exists and has dokan-layout class
DokanModal->>User: Displays modal content
User->>DokanModal: Triggers close
DokanModal->>UpgradeModal: Calls onClose callbackPossibly related PRs
Suggested labels
Suggested reviewers
Poem
|
![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
🔭 Outside diff range comments (1)
src/intelligence/components/DokanAI.tsx (1)
44-51: Handle cross-origin and missing iframe errors in getElementAccessing
iframe.contentDocument/contentWindowcan throw runtime errors (e.g., cross-origin restrictions) or returnnullif the iframe isn’t yet loaded. Add checks and atry/catchto gracefully fall back to querying the main document.• File:
src/intelligence/components/DokanAI.tsx
Lines: 44–51const getElement = ( ifr: string, selector: string ) => { - const iframe = document.getElementById( ifr ) as HTMLIFrameElement; - const doc = iframe.contentDocument || iframe.contentWindow?.document; - return ( - doc?.querySelector( `[data-id="${ selector }"]` ) || - document.querySelector( `#${ selector }` ) - ); + const iframe = document.getElementById(ifr) as HTMLIFrameElement | null; + if (!iframe) { + return document.querySelector(`#${selector}`); + } + + let doc: Document | null = null; + try { + doc = iframe.contentDocument || iframe.contentWindow?.document || null; + } catch (error) { + console.warn(`Unable to access iframe "${ifr}":`, error); + } + + if (!doc) { + return document.querySelector(`#${selector}`); + } + + return ( + doc.querySelector(`[data-id="${selector}"]`) || + document.querySelector(`#${selector}`) + ); };
🧹 Nitpick comments (3)
src/intelligence/components/DokanAI.tsx (2)
287-314: Consider more targeted mutation observer scope.While the guard clause is a good optimization, the mutation observer is still watching the entire
document.body, which could be expensive for large DOM trees. Consider a more targeted approach:-useMutationObserver( - document.body, - ( mutations ) => { +useMutationObserver( + document.querySelector('#wpwrap') || document.body, + ( mutations ) => {This would limit the observation scope to the WordPress admin area, reducing the performance impact.
298-299: Remove TypeScript ignore comment.The
@ts-ignorecomment indicates a type issue that should be addressed rather than suppressed.- // @ts-ignore - for ( const node of mutation.addedNodes ) { + for ( const node of Array.from(mutation.addedNodes) ) {This provides proper type safety by converting the NodeList to an array.
src/admin/dashboard/pages/modules/UpgradeModal.tsx (1)
4-4: Remove unused import.The
Modalimport from@getdokan/dokan-uiis no longer used since the component has been refactored to useDokanModal.Apply this diff to remove the unused import:
-import { Modal } from '@getdokan/dokan-ui';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/admin/dashboard/components/Dashboard.tsx(0 hunks)src/admin/dashboard/pages/modules/UpgradeModal.tsx(2 hunks)src/admin/dashboard/pages/modules/VideoPopup.tsx(3 hunks)src/components/modals/DokanModal.tsx(5 hunks)src/dashboard/index.tsx(0 hunks)src/intelligence/components/DokanAI.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- src/admin/dashboard/components/Dashboard.tsx
- src/dashboard/index.tsx
⏰ 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). (4)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (2, 3)
🔇 Additional comments (8)
src/intelligence/components/DokanAI.tsx (1)
290-292: Performance optimization looks good!The guard clause prevents unnecessary DOM manipulation when the modal is closed, which is a good optimization. This aligns with the PR objective of refactoring mutation observer usage.
src/admin/dashboard/pages/modules/VideoPopup.tsx (2)
5-5: LGTM! Clean import addition.The import of
DokanModalfrom the components module aligns with the refactor to centralize modal management.
36-54: Excellent refactor to centralized modal component.The migration from
ModaltoDokanModalis well-executed:
- Appropriate namespace for video popups
- Header and footer correctly disabled
- Video content properly passed via
dialogContentprop- Fixed width styling maintained
This change successfully centralizes modal behavior and removes the need for custom mutation observer logic.
src/admin/dashboard/pages/modules/UpgradeModal.tsx (1)
14-252: Excellent refactor to centralized modal component.The migration to
DokanModalis well-structured:
- Clean content extraction to
modalContentvariable improves readability- Proper namespace and styling configuration
- Header and footer correctly disabled to match the custom design
- Modal content structure preserved intact
This change successfully centralizes modal behavior while maintaining the existing user experience.
src/components/modals/DokanModal.tsx (4)
3-3: Good change to relative import.Moving the
kebabCaseimport to a relative path improves code organization and reduces external dependencies.
53-64: Excellent portal management solution.The
useEffecthook properly centralizes the portal root management by:
- Ensuring the
#headlessui-portal-rootelement exists- Adding the required
dokan-layoutclass for consistent styling- Creating the element if it doesn't exist
- Running on modal open state changes
This successfully replaces the scattered mutation observer logic mentioned in the PR objectives.
66-68: Good defensive programming.The safe handling of
kebabCaseprevents runtime errors if the utility function is unavailable.
172-198: Clean conditional footer rendering.The footer conditional logic correctly implements the pattern:
- Renders nothing when
dialogFooterisfalse- Renders custom footer when provided
- Falls back to default footer with cancel/confirm buttons
This provides the flexibility needed for different modal use cases.
…e usage in related components
20 tasks
… style management
…omization and consistency across components
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/admin/dashboard/pages/modules/UpgradeModal.tsx(2 hunks)src/admin/dashboard/pages/modules/VideoPopup.tsx(3 hunks)src/components/modals/DokanModal.tsx(6 hunks)src/intelligence/components/DokanAI.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/admin/dashboard/pages/modules/VideoPopup.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/admin/dashboard/pages/modules/UpgradeModal.tsx
- src/components/modals/DokanModal.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/intelligence/components/DokanAI.tsx
[error] 502-502: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 560-560: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ 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). (4)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (3, 3)
🔇 Additional comments (3)
src/intelligence/components/DokanAI.tsx (3)
290-293: Good optimization: Early return guard for mutation observerThe early return when the modal is not open prevents unnecessary DOM processing and improves performance by avoiding mutation observer callbacks when they're not needed.
356-656: Modal API migration looks correctThe migration from the old
Modalcomponent toDokanModalwith the new prop-based API (dialogHeader,dialogContent,dialogFooter) appears to be implemented correctly. The functionality is preserved while adapting to the new component structure.
658-699: Regenerate modal migration implemented correctlyThe regenerate confirmation modal has been successfully migrated to use
DokanModalwith the new API. The header and footer are properly disabled (dialogHeader={ false }anddialogFooter={ false }) while maintaining the same functionality.
Dokan Modal usagesThe usages overall from Dokan lite and Dokan pro components. here are lists of all usages.
Pull RequestsThe updated list of DokanModal usages from requested pull request components (PR getdokan/dokan-pro/pull/4440).
|
mrabbani
added
👍 Dev Review Done
and removed
In Progress
* feat: add custom Popover component using WordPress UI * feat: integrate Popover component into WpDatePicker for enhanced date selection * feat: enhance CategorySelector with custom Popover for category selection
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/admin/dashboard/pages/modules/CategorySelector.tsx (1)
29-30: Consider adding proper TypeScript typing for the popover anchor.The
popoverAnchorstate lacks proper typing, which led to the@ts-ignoreusage later.Apply this diff to add proper typing:
- const [ popoverAnchor, setPopoverAnchor ] = useState(); + const [ popoverAnchor, setPopoverAnchor ] = useState<HTMLElement | null>(null);src/components/WpDatePicker.tsx (1)
18-19: Add proper TypeScript typing for the popover anchor.The
popoverAnchorstate lacks proper typing, which led to the@ts-ignoreusage.Apply this diff to add proper typing:
- const [ popoverAnchor, setPopoverAnchor ] = useState(); + const [ popoverAnchor, setPopoverAnchor ] = useState<HTMLElement | null>(null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/admin/dashboard/pages/modules/CategorySelector.tsx(3 hunks)src/admin/dashboard/style.scss(1 hunks)src/components/Popover.tsx(1 hunks)src/components/WpDatePicker.tsx(3 hunks)src/components/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/admin/dashboard/style.scss
- src/components/Popover.tsx
- src/components/index.tsx
⏰ 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). (4)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (2)
src/components/WpDatePicker.tsx (2)
9-15: Well-designed Props interface extension.The Props interface properly extends DatePickerProps and adds the necessary optional styling props. This is a clean approach that maintains type safety while allowing customization.
57-60: Good use of twMerge for class name composition.The usage of
twMergefor combining class names is appropriate and handles Tailwind CSS class conflicts properly.Also applies to: 63-66
Comment on lines
+73
to
+77
| onClose={ () => { | ||
| setIsVisible( ! isVisible ); | ||
| } } | ||
| onFocusOutside={ () => { | ||
| setIsVisible( ! isVisible ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the close handler logic.
The onClose and onFocusOutside handlers use the same toggle logic, which is redundant. When closing, you should always set isVisible to false.
Apply this diff to simplify the close logic:
onClose={ () => {
- setIsVisible( ! isVisible );
+ setIsVisible( false );
} }
onFocusOutside={ () => {
- setIsVisible( ! isVisible );
+ setIsVisible( false );
} }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onClose={ () => { | |
| setIsVisible( ! isVisible ); | |
| } } | |
| onFocusOutside={ () => { | |
| setIsVisible( ! isVisible ); | |
| onClose={ () => { | |
| setIsVisible(false); | |
| } } | |
| onFocusOutside={ () => { | |
| setIsVisible(false); | |
| } } |
🤖 Prompt for AI Agents
In src/admin/dashboard/pages/modules/CategorySelector.tsx around lines 73 to 77,
the onClose and onFocusOutside handlers currently toggle isVisible, which is
redundant and incorrect for closing actions. Change both handlers to directly
set isVisible to false instead of toggling it, ensuring the component always
closes properly when these events occur.
Comment on lines
+34
to
+41
| { /* eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions */ } | ||
| <div | ||
| className="inline-flex items-center gap-x-1 text-sm/6 text-gray-900" | ||
| onClick={ () => { | ||
| setIsVisible( ! isVisible ); | ||
| } } | ||
| // @ts-ignore | ||
| ref={ setPopoverAnchor } |
There was a problem hiding this comment.
Address accessibility violations and improve TypeScript typing.
The component has several accessibility and typing issues:
- Accessibility violations: The eslint-disable comments indicate missing keyboard navigation and improper interactive element usage.
- TypeScript suppression: The
@ts-ignorecomment suggests improper typing for the ref callback.
Apply this diff to fix the accessibility and typing issues:
- { /* eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions */ }
- <div
+ <button
+ type="button"
className="inline-flex items-center gap-x-1 text-sm/6 text-gray-900"
onClick={ () => {
setIsVisible( ! isVisible );
} }
- // @ts-ignore
- ref={ setPopoverAnchor }
+ ref={ setPopoverAnchor as React.RefCallback<HTMLButtonElement> }
>And update the closing tag:
- </div>
+ </button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { /* eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions */ } | |
| <div | |
| className="inline-flex items-center gap-x-1 text-sm/6 text-gray-900" | |
| onClick={ () => { | |
| setIsVisible( ! isVisible ); | |
| } } | |
| // @ts-ignore | |
| ref={ setPopoverAnchor } | |
| <button | |
| type="button" | |
| className="inline-flex items-center gap-x-1 text-sm/6 text-gray-900" | |
| onClick={() => { | |
| setIsVisible(!isVisible); | |
| }} | |
| ref={setPopoverAnchor as React.RefCallback<HTMLButtonElement>} | |
| > | |
| {/* …existing button contents… */} | |
| </button> |
🤖 Prompt for AI Agents
In src/admin/dashboard/pages/modules/CategorySelector.tsx around lines 34 to 41,
the div used as an interactive element lacks proper keyboard event handlers and
semantic roles, causing accessibility violations, and the ref callback has
improper typing requiring a @ts-ignore. Replace the div with a button element or
add appropriate role="button" and tabIndex={0}, implement onKeyDown to handle
keyboard interactions like Enter and Space keys, and remove the eslint-disable
comments. For the ref, define the callback with correct TypeScript typing to
avoid using @ts-ignore. Also, update the closing tag accordingly to match the
new element.
Comment on lines
+51
to
+56
| onClose={ () => { | ||
| setIsVisible( ! isVisible ); | ||
| } } | ||
| onFocusOutside={ () => { | ||
| setIsVisible( ! isVisible ); | ||
| } } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the close handler logic.
The close handlers use redundant toggle logic. When closing, you should always set isVisible to false.
Apply this diff to simplify the close logic:
onClose={ () => {
- setIsVisible( ! isVisible );
+ setIsVisible( false );
} }
onFocusOutside={ () => {
- setIsVisible( ! isVisible );
+ setIsVisible( false );
} }🤖 Prompt for AI Agents
In src/components/WpDatePicker.tsx around lines 51 to 56, the onClose and
onFocusOutside handlers currently toggle the isVisible state, which is redundant
and incorrect for closing actions. Change both handlers to explicitly set
isVisible to false instead of toggling it, ensuring the date picker always
closes properly when these events occur.
Comment on lines
+34
to
42
| { /* eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions */ } | ||
| <div | ||
| className={ props?.pickerToggleClassName ?? '' } | ||
| onClick={ () => { | ||
| setIsVisible( ! isVisible ); | ||
| } } | ||
| // @ts-ignore | ||
| ref={ setPopoverAnchor } | ||
| > |
There was a problem hiding this comment.
Address accessibility violations and improve TypeScript typing.
Similar to the CategorySelector component, this has accessibility and typing issues:
- Accessibility violations: Missing keyboard navigation and improper interactive element usage.
- TypeScript suppression: The
@ts-ignorecomment indicates improper typing for the ref callback.
Apply this diff to fix the accessibility and typing issues:
- { /* eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions */ }
- <div
+ <button
+ type="button"
className={ props?.pickerToggleClassName ?? '' }
onClick={ () => {
setIsVisible( ! isVisible );
} }
- // @ts-ignore
- ref={ setPopoverAnchor }
+ ref={ setPopoverAnchor as React.RefCallback<HTMLButtonElement> }
>
{ props.children ?? '' }
- </div>
+ </button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { /* eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions */ } | |
| <div | |
| className={ props?.pickerToggleClassName ?? '' } | |
| onClick={ () => { | |
| setIsVisible( ! isVisible ); | |
| } } | |
| // @ts-ignore | |
| ref={ setPopoverAnchor } | |
| > | |
| <button | |
| type="button" | |
| className={ props?.pickerToggleClassName ?? '' } | |
| onClick={ () => { | |
| setIsVisible(!isVisible); | |
| } } | |
| ref={ setPopoverAnchor as React.RefCallback<HTMLButtonElement> } | |
| > | |
| { props.children ?? '' } | |
| </button> |
🤖 Prompt for AI Agents
In src/components/WpDatePicker.tsx around lines 34 to 42, fix accessibility by
replacing the div with a button or adding keyboard event handlers to support
keyboard navigation, ensuring it behaves as an interactive element. Remove the
@ts-ignore comment by properly typing the ref callback to match React's expected
types for refs, improving TypeScript compliance.
|
Need to also review by @mrabbani. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
Summary by CodeRabbit
Refactor
Bug Fixes
Style