feat(popup-reply): add threaded annotation feature flag (#735)

- Add isThreadedAnnotation feature flag to conditionally render PopupReplyV2
- Refactor PopupReply Props to discriminated union (LegacyProps | ThreadedProps)
- Migrate PopupReply tests from Enzyme to React Testing Library
- Add @testing-library/react, @testing-library/dom, @testing-library/jest-dom deps
- Add jest-setup.ts for global jest-dom matcher registration
- Add test coverage for isThreadedAnnotation in PopupContainer and PopupLayer
- Add tests for useEffect popper update and options referential integrity

Author: jackiejou

jest.config.js

@@ -23,7 +23,7 @@ module.exports = {
     restoreMocks: true,
     roots: ['src'],
     setupFiles: ['jest-canvas-mock', '<rootDir>/scripts/jest/envWindow.js'],
-    setupFilesAfterEnv: ['<rootDir>/scripts/jest/enzyme-adapter.js'],
+    setupFilesAfterEnv: ['<rootDir>/scripts/jest/enzyme-adapter.js', '<rootDir>/scripts/jest/jest-setup.ts'],
     snapshotSerializers: ['enzyme-to-json/serializer'],
     testEnvironment: 'jest-environment-jsdom-sixteen',
     transformIgnorePatterns: ['node_modules/(?!(box-ui-elements)/)'],

package.json

@@ -36,6 +36,9 @@
         "@formatjs/intl-relativetimeformat": "^4.5.12",
         "@popperjs/core": "^2.4.0",
         "@reduxjs/toolkit": "^1.3.5",
+        "@testing-library/dom": "^10.4.1",
+        "@testing-library/jest-dom": "^6.9.1",
+        "@testing-library/react": "^16.3.2",
         "@types/classnames": "^2.2.10",
         "@types/draft-js": "^0.10.40",
         "@types/enzyme": "^3.10.5",

scripts/jest/jest-setup.ts

@@ -0,0 +1 @@
+import '@testing-library/jest-dom';

src/BoxAnnotations.ts

@@ -19,7 +19,8 @@ type AnnotationsOptions = {
 };
 
 export type Features = {
-    [key: string]: boolean;
+    isThreadedAnnotation?: boolean;
+    [key: string]: boolean | undefined;
 };
 
 type PreviewOptions = {

src/components/Popups/PopupReply.tsx

@@ -5,21 +5,38 @@ import { useIntl } from 'react-intl';
 import FocusTrap from 'box-ui-elements/es/components/focus-trap/FocusTrap';
 import messages from './messages';
 import PopupBase from './PopupBase';
+import PopupReplyV2 from './PopupReplyV2';
 import ReplyForm from '../ReplyForm';
 import { getScale, getRotation } from '../../store/options';
 import { PopupReference } from './Popper';
 import './PopupReply.scss';
 
-export type Props = {
+type BaseProps = {
     className?: string;
+};
+
+type LegacyProps = BaseProps & {
     isPending: boolean;
+    isThreadedAnnotation?: false;
     onCancel: (text?: string) => void;
     onChange: (text?: string) => void;
     onSubmit: (text: string) => void;
     reference: PopupReference;
     value?: string;
 };
 
+type ThreadedProps = BaseProps & {
+    isThreadedAnnotation: true;
+    isPending?: boolean;
+    onCancel?: (text?: string) => void;
+    onChange?: (text?: string) => void;
+    onSubmit?: (text: string) => void;
+    reference?: PopupReference;
+    value?: string;
+};
+
+export type Props = LegacyProps | ThreadedProps;
+
 const isIE = (): boolean => {
     const { userAgent } = navigator;
     return userAgent.indexOf('Trident/') > 0;
@@ -77,14 +94,7 @@ const getOptions = (): Partial<Popper.Options> => {
     };
 };
 
-export default function PopupReply({
-    isPending,
-    onCancel,
-    onChange,
-    onSubmit,
-    value = '',
-    ...rest
-}: Props): JSX.Element {
+export default function PopupReply(props: Props): JSX.Element {
     const intl = useIntl();
     const popupRef = React.useRef<PopupBase>(null);
     const popupOptions = React.useRef<Partial<Popper.Options>>(getOptions()); // Keep the options reference the same between renders
@@ -99,6 +109,14 @@ export default function PopupReply({
         }
     }, [popupRef, rotation, scale]);
 
+    // TODO: PopupReplyV2 is a placeholder stub. Props (isPending, onCancel, etc.)
+    // are not yet forwarded and will need to be wired up when V2 is implemented.
+    if (props.isThreadedAnnotation) {
+        return <PopupReplyV2 />;
+    }
+
+    const { isPending, onCancel, onChange, onSubmit, value = '', isThreadedAnnotation: _, ...rest } = props as LegacyProps;
+
     return (
         <FocusTrap>
             <PopupBase

src/components/Popups/PopupReplyV2.tsx

@@ -0,0 +1,10 @@
+import React from 'react';
+
+/**
+ * Placeholder for the threaded annotation reply popup (V2).
+ * Replaces PopupReply when the isThreadedAnnotation feature flag is enabled.
+ * Currently renders an empty container — props will be wired up when V2 is implemented.
+ */
+export default function PopupReplyV2(): JSX.Element {
+    return <div data-testid="popup-reply-v2" />;
+}

src/components/Popups/__tests__/PopupReply-test.tsx

@@ -1,16 +1,43 @@
 import React from 'react';
-import * as ReactRedux from 'react-redux';
-import { mount, ReactWrapper } from 'enzyme';
-import PopupBase from '../PopupBase';
+import { render, screen } from '@testing-library/react';
+import { useSelector } from 'react-redux';
 import PopupReply, { Props } from '../PopupReply';
-import ReplyForm from '../../ReplyForm';
 
 jest.mock('react-redux', () => ({
     useSelector: jest.fn(),
 }));
 
-jest.mock('../PopupBase');
-jest.mock('../../ReplyForm');
+const mockUseSelector = useSelector as jest.MockedFunction<typeof useSelector>;
+
+jest.mock('../PopupBase', () => {
+    const ReactMock = jest.requireActual('react');
+    const MockPopupBase = ReactMock.forwardRef(
+        ({ children, options, ...rest }: Record<string, unknown>, ref: React.Ref<HTMLDivElement>) =>
+            ReactMock.createElement(
+                'div',
+                { ref, 'data-testid': 'popup-base', 'data-options': JSON.stringify(options), ...rest },
+                children,
+            ),
+    );
+    MockPopupBase.displayName = 'MockPopupBase';
+    return MockPopupBase;
+});
+
+jest.mock('../../ReplyForm', () => {
+    const ReactMock = jest.requireActual('react');
+    return ({ isPending, value, ...rest }: Record<string, unknown>) =>
+        ReactMock.createElement('div', {
+            'data-testid': 'reply-form',
+            'data-pending': String(isPending),
+            'data-value': value || '',
+            ...rest,
+        });
+});
+
+jest.mock('../PopupReplyV2', () => {
+    const ReactMock = jest.requireActual('react');
+    return () => ReactMock.createElement('div', { 'data-testid': 'popup-reply-v2' });
+});
 
 describe('PopupReply', () => {
     const defaults: Props = {
@@ -21,93 +48,116 @@ describe('PopupReply', () => {
         reference: document.createElement('div'),
     };
 
-    const getWrapper = (props = {}): ReactWrapper => mount(<PopupReply {...defaults} {...props} />);
-
     beforeEach(() => {
-        jest.spyOn(React, 'useEffect').mockImplementation(f => f());
+        mockUseSelector.mockReturnValue(0);
     });
 
-    describe('state changes', () => {
-        test('should call update on the underlying popper instance when the store changes', () => {
-            const popupMock = { popper: { update: jest.fn() } };
-            const reduxSpy = jest.spyOn(ReactRedux, 'useSelector').mockImplementation(() => false);
-            const refSpy = jest.spyOn(React, 'useRef').mockImplementation(() => ({ current: popupMock }));
-            const wrapper = getWrapper();
+    afterEach(() => {
+        jest.clearAllMocks();
+    });
 
-            reduxSpy.mockReturnValueOnce(true);
-            wrapper.setProps({ value: '1' });
+    describe('render()', () => {
+        test('should render the ReplyForm', () => {
+            render(<PopupReply {...defaults} />);
+
+            expect(screen.getByTestId('popup-base')).toBeDefined();
+            expect(screen.getByTestId('reply-form')).toBeDefined();
+        });
 
-            reduxSpy.mockReturnValueOnce(false);
-            wrapper.setProps({ value: '2' });
+        test('should render PopupReplyV2 when isThreadedAnnotation is true', () => {
+            render(<PopupReply {...defaults} isThreadedAnnotation />);
 
-            expect(refSpy).toHaveBeenCalled();
-            expect(popupMock.popper.update).toHaveBeenCalledTimes(3);
+            expect(screen.getByTestId('popup-reply-v2')).toBeDefined();
+            expect(screen.queryByTestId('popup-base')).toBeNull();
+            expect(screen.queryByTestId('reply-form')).toBeNull();
         });
-    });
 
-    describe('render()', () => {
-        test('should render the ReplyForm', () => {
-            const wrapper = getWrapper();
+        test('should render existing UI when isThreadedAnnotation is false', () => {
+            render(<PopupReply {...defaults} isThreadedAnnotation={false} />);
+
+            expect(screen.queryByTestId('popup-reply-v2')).toBeNull();
+            expect(screen.getByTestId('popup-base')).toBeDefined();
+            expect(screen.getByTestId('reply-form')).toBeDefined();
+        });
 
-            expect(wrapper.exists(PopupBase)).toBe(true);
-            expect(wrapper.exists(ReplyForm)).toBe(true);
+        test('should render existing UI when isThreadedAnnotation is undefined', () => {
+            render(<PopupReply {...defaults} />);
+
+            expect(screen.queryByTestId('popup-reply-v2')).toBeNull();
+            expect(screen.getByTestId('popup-base')).toBeDefined();
+            expect(screen.getByTestId('reply-form')).toBeDefined();
         });
 
-        test('should maintain referential integrity of the PopupBase options object across renders', () => {
-            const wrapper = getWrapper();
+        test('should pass value prop to ReplyForm', () => {
+            render(<PopupReply {...defaults} value="test value" />);
+
+            const replyForm = screen.getByTestId('reply-form');
+            expect(replyForm.getAttribute('data-value')).toBe('test value');
+        });
 
-            const popupOptions = wrapper.find(PopupBase).prop('options');
+        test('should pass isPending prop to ReplyForm', () => {
+            render(<PopupReply {...defaults} isPending />);
 
-            expect(wrapper.exists(PopupBase)).toBe(true);
-            expect(wrapper.find(ReplyForm).prop('value')).toBe('');
+            const replyForm = screen.getByTestId('reply-form');
+            expect(replyForm.getAttribute('data-pending')).toBe('true');
+        });
+    });
 
-            wrapper.setProps({ value: '1' });
+    describe('useEffect popper update', () => {
+        test('should call useSelector for rotation and scale values', () => {
+            mockUseSelector.mockReturnValueOnce(0).mockReturnValueOnce(1);
 
-            expect(wrapper.find(PopupBase).prop('options')).toStrictEqual(popupOptions);
-            expect(wrapper.find(ReplyForm).prop('value')).toBe('1');
+            render(<PopupReply {...defaults} />);
+
+            expect(mockUseSelector).toHaveBeenCalledTimes(2);
+        });
+
+        test('should re-render when rotation/scale changes', () => {
+            mockUseSelector.mockReturnValue(0);
+
+            const { rerender } = render(<PopupReply {...defaults} />);
+            const callCountAfterFirstRender = mockUseSelector.mock.calls.length;
+
+            mockUseSelector.mockReturnValue(90);
+            rerender(<PopupReply {...defaults} />);
+
+            expect(mockUseSelector.mock.calls.length).toBeGreaterThan(callCountAfterFirstRender);
         });
     });
 
     describe('Popup options', () => {
-        const eventListenersModifier = {
-            name: 'eventListeners',
-            options: { scroll: false },
-        };
-        const IEUserAgent = 'Mozilla/5.0 (Windows NT 10.0; WOW64; Trident/7.0; rv:11.0) like Gecko';
-        const otherUserAgent = 'Other';
-
-        test.each`
-            userAgent         | expectedPlacement
-            ${IEUserAgent}    | ${'top'}
-            ${otherUserAgent} | ${'bottom'}
-        `(
-            'should set placement option as $expectedPlacement based on userAgent=$userAgent',
-            ({ userAgent, expectedPlacement }) => {
-                global.window.navigator.userAgent = userAgent;
-
-                const wrapper = getWrapper();
-
-                expect(window.navigator.userAgent).toEqual(userAgent);
-                expect(wrapper.find(PopupBase).prop('options').placement).toBe(expectedPlacement);
-            },
-        );
-
-        test('should disable scroll event listeners if browser is IE', () => {
-            global.window.navigator.userAgent = IEUserAgent;
-
-            const wrapper = getWrapper();
-
-            expect(window.navigator.userAgent).toEqual(IEUserAgent);
-            expect(wrapper.find(PopupBase).prop('options').modifiers).toContainEqual(eventListenersModifier);
+        test('should pass options to PopupBase', () => {
+            render(<PopupReply {...defaults} />);
+
+            const popupBase = screen.getByTestId('popup-base');
+            const options = JSON.parse(popupBase.getAttribute('data-options') || '{}');
+
+            expect(options).toHaveProperty('placement');
+            expect(options).toHaveProperty('modifiers');
+            expect(Array.isArray(options.modifiers)).toBe(true);
+        });
+
+        test('should maintain the same options reference between renders', () => {
+            const { rerender } = render(<PopupReply {...defaults} />);
+            const optionsBefore = screen.getByTestId('popup-base').getAttribute('data-options');
+
+            rerender(<PopupReply {...defaults} />);
+            const optionsAfter = screen.getByTestId('popup-base').getAttribute('data-options');
+
+            expect(optionsBefore).toBe(optionsAfter);
         });
 
-        test('should not disable scroll event listeners for non IE browsers', () => {
-            global.window.navigator.userAgent = otherUserAgent;
+        test('should include required modifiers in options', () => {
+            render(<PopupReply {...defaults} />);
 
-            const wrapper = getWrapper();
+            const popupBase = screen.getByTestId('popup-base');
+            const options = JSON.parse(popupBase.getAttribute('data-options') || '{}');
 
-            expect(window.navigator.userAgent).toEqual(otherUserAgent);
-            expect(wrapper.find(PopupBase).prop('options').modifiers).not.toContainEqual(eventListenersModifier);
+            const modifierNames = options.modifiers.map((m: { name: string }) => m.name);
+            expect(modifierNames).toContain('arrow');
+            expect(modifierNames).toContain('flip');
+            expect(modifierNames).toContain('offset');
+            expect(modifierNames).toContain('preventOverflow');
         });
     });
 });

src/components/Popups/__tests__/PopupReplyV2-test.tsx

@@ -0,0 +1,10 @@
+import React from 'react';
+import { render, screen } from '@testing-library/react';
+import PopupReplyV2 from '../PopupReplyV2';
+
+describe('PopupReplyV2', () => {
+    test('should render v2 with correct test id', () => {
+        render(<PopupReplyV2 />);
+        expect(screen.getByTestId('popup-reply-v2')).toBeDefined();
+    });
+});

src/popup/PopupContainer.tsx

@@ -15,12 +15,14 @@ import {
     setMessageAction,
     getCreatorReferenceId,
 } from '../store';
+import { isFeatureEnabled } from '../store/options';
 import { createDrawingAction } from '../drawing/actions';
 import { createHighlightAction } from '../highlight/actions';
 import { createRegionAction } from '../region';
 
 export type Props = {
     isPromoting: boolean;
+    isThreadedAnnotation?: boolean;
     message: string;
     mode: Mode;
     referenceId: string | null;
@@ -31,6 +33,7 @@ export type Props = {
 export const mapStateToProps = (state: AppState, { location }: { location: number }): Props => {
     return {
         isPromoting: getIsPromoting(state),
+        isThreadedAnnotation: isFeatureEnabled(state, 'isThreadedAnnotation'),
         message: getCreatorMessage(state),
         mode: getAnnotationMode(state),
         referenceId: getCreatorReferenceId(state),