Просмотр исходного кода

Merge pull request #11039 from growilabs/fix/unhandled-popover-error

fix(editor): Editor breaks when an ID created by useId() is passed to a  reactstrap Popover/Tooltip targets
mergify[bot] 1 день назад
Родитель
Сommit
b786d7f2eb

+ 23 - 0
apps/app/.claude/rules/ui-pitfalls.md

@@ -0,0 +1,23 @@
+# UI Pitfalls
+
+## useId() must not be passed to reactstrap `target` prop
+
+React's `useId()` generates IDs containing colons (`:r0:`, `:r1:`, `:r2:`). These are valid HTML `id` attributes but **invalid CSS selectors**.
+
+reactstrap's `findDOMElements()` resolves string targets via `document.querySelectorAll(target)`, which throws `DOMException: is not a valid selector` when the string contains colons.
+
+```tsx
+// ❌ WRONG: useId() output passed as string target
+const popoverTargetId = useId();
+<button id={popoverTargetId}>...</button>
+<Popover target={popoverTargetId} />  // → DOMException at componentDidMount
+
+// ✅ CORRECT: use ref — reactstrap resolves refs via .current, bypassing querySelectorAll
+const popoverTargetRef = useRef<HTMLButtonElement>(null);
+<button ref={popoverTargetRef}>...</button>
+<Popover target={popoverTargetRef} />
+```
+
+**Applies to all reactstrap components with a `target` prop**: `Popover`, `Tooltip`, `UncontrolledPopover`, `UncontrolledTooltip`, etc.
+
+**Safe uses of `useId()`**: `id=`, `htmlFor=`, `aria-labelledby=`, `aria-describedby=` — these use `getElementById` internally, which does not parse CSS.

+ 4 - 6
apps/app/src/client/components/Admin/AdminHome/AdminHome.tsx

@@ -1,5 +1,5 @@
 import type { FC } from 'react';
 import type { FC } from 'react';
-import { useId, useState } from 'react';
+import { useRef, useState } from 'react';
 import { useTranslation } from 'next-i18next';
 import { useTranslation } from 'next-i18next';
 import { CopyToClipboard } from 'react-copy-to-clipboard';
 import { CopyToClipboard } from 'react-copy-to-clipboard';
 import { Tooltip } from 'reactstrap';
 import { Tooltip } from 'reactstrap';
@@ -29,9 +29,7 @@ export const AdminHome: FC = () => {
     }, 500);
     }, 500);
   };
   };
 
 
-  // Generate CSS-safe ID by removing colons from useId() result
-  const copyButtonIdRaw = useId();
-  const copyButtonId = `copy-button-${copyButtonIdRaw.replace(/:/g, '')}`;
+  const copyIconRef = useRef<HTMLSpanElement>(null);
 
 
   return (
   return (
     <div data-testid="admin-home">
     <div data-testid="admin-home">
@@ -131,7 +129,7 @@ export const AdminHome: FC = () => {
                   onClick={(e) => e.preventDefault()}
                   onClick={(e) => e.preventDefault()}
                 >
                 >
                   <span
                   <span
-                    id={copyButtonId}
+                    ref={copyIconRef}
                     className="material-symbols-outlined"
                     className="material-symbols-outlined"
                     aria-hidden="true"
                     aria-hidden="true"
                   >
                   >
@@ -143,7 +141,7 @@ export const AdminHome: FC = () => {
               <Tooltip
               <Tooltip
                 placement="bottom"
                 placement="bottom"
                 isOpen={copyState === COPY_STATE.DONE}
                 isOpen={copyState === COPY_STATE.DONE}
-                target={copyButtonId}
+                target={copyIconRef}
                 fade={false}
                 fade={false}
               >
               >
                 {t('admin:admin_top:copy_prefilled_host_information:done')}
                 {t('admin:admin_top:copy_prefilled_host_information:done')}

+ 4 - 4
apps/app/src/client/components/PageEditor/EditorNavbar/EditingUserList.tsx

@@ -1,4 +1,4 @@
-import { type FC, useId, useState } from 'react';
+import { type FC, useRef, useState } from 'react';
 import type { EditingClient } from '@growi/editor';
 import type { EditingClient } from '@growi/editor';
 import { UserPicture } from '@growi/ui/dist/components';
 import { UserPicture } from '@growi/ui/dist/components';
 import { Popover, PopoverBody } from 'reactstrap';
 import { Popover, PopoverBody } from 'reactstrap';
@@ -31,7 +31,7 @@ const AvatarWrapper: FC<{
 };
 };
 
 
 export const EditingUserList: FC<Props> = ({ clientList, onUserClick }) => {
 export const EditingUserList: FC<Props> = ({ clientList, onUserClick }) => {
-  const popoverTargetId = useId();
+  const popoverTargetRef = useRef<HTMLButtonElement>(null);
   const [isPopoverOpen, setIsPopoverOpen] = useState(false);
   const [isPopoverOpen, setIsPopoverOpen] = useState(false);
 
 
   const togglePopover = () => setIsPopoverOpen(!isPopoverOpen);
   const togglePopover = () => setIsPopoverOpen(!isPopoverOpen);
@@ -56,7 +56,7 @@ export const EditingUserList: FC<Props> = ({ clientList, onUserClick }) => {
           <div className="ms-1">
           <div className="ms-1">
             <button
             <button
               type="button"
               type="button"
-              id={popoverTargetId}
+              ref={popoverTargetRef}
               className="btn border-0 bg-info-subtle rounded-pill p-0"
               className="btn border-0 bg-info-subtle rounded-pill p-0"
               onClick={togglePopover}
               onClick={togglePopover}
             >
             >
@@ -67,7 +67,7 @@ export const EditingUserList: FC<Props> = ({ clientList, onUserClick }) => {
             <Popover
             <Popover
               placement="bottom"
               placement="bottom"
               isOpen={isPopoverOpen}
               isOpen={isPopoverOpen}
-              target={popoverTargetId}
+              target={popoverTargetRef}
               toggle={togglePopover}
               toggle={togglePopover}
               trigger="legacy"
               trigger="legacy"
             >
             >

+ 8 - 4
apps/app/src/features/page-tree/components/SimpleItemContent.tsx

@@ -1,4 +1,4 @@
-import { useId } from 'react';
+import { useRef } from 'react';
 import Link from 'next/link';
 import Link from 'next/link';
 import { pathUtils } from '@growi/core/dist/utils';
 import { pathUtils } from '@growi/core/dist/utils';
 import { useTranslation } from 'next-i18next';
 import { useTranslation } from 'next-i18next';
@@ -26,7 +26,7 @@ export const SimpleItemContent = ({
   const shouldShowAttentionIcon =
   const shouldShowAttentionIcon =
     page.processData != null ? shouldRecoverPagePaths(page.processData) : false;
     page.processData != null ? shouldRecoverPagePaths(page.processData) : false;
 
 
-  const spanId = `path-recovery-${useId()}`;
+  const warningIconRef = useRef<HTMLSpanElement>(null);
 
 
   // When asLink is true, render the title as an anchor so that the browser
   // When asLink is true, render the title as an anchor so that the browser
   // recognizes it as a link (enables Ctrl/Cmd+click to open in new tab,
   // recognizes it as a link (enables Ctrl/Cmd+click to open in new tab,
@@ -48,12 +48,16 @@ export const SimpleItemContent = ({
       {shouldShowAttentionIcon && (
       {shouldShowAttentionIcon && (
         <>
         <>
           <span
           <span
-            id={spanId}
+            ref={warningIconRef}
             className="material-symbols-outlined mr-2 text-warning"
             className="material-symbols-outlined mr-2 text-warning"
           >
           >
             warning
             warning
           </span>
           </span>
-          <UncontrolledTooltip placement="top" target={spanId} fade={false}>
+          <UncontrolledTooltip
+            placement="top"
+            target={warningIconRef}
+            fade={false}
+          >
             {t('tooltip.operation.attention.rename')}
             {t('tooltip.operation.attention.rename')}
           </UncontrolledTooltip>
           </UncontrolledTooltip>
         </>
         </>