bito-code-review[bot] commented on code in PR #44858:
URL: https://github.com/apache/superset/pull/44858#discussion_r4154832617
##########
superset-frontend/src/explore/components/controls/TextAreaControl.test.tsx:
##########
@@ -61,4 +62,139 @@ describe('TextArea', () => {
fireEvent.change(textArea, { target: { value: 'x' } });
expect(defaultProps.onChange).toHaveBeenCalledWith('x');
});
+
+ test('picks up an externally changed initialValue without remounting the
AceEditor', async () => {
+ function Wrapper() {
+ const [initialValue, setInitialValue] = useState('first');
+ return (
+ <>
+ <button
+ type="button"
+ onClick={() => setInitialValue('synced-from-source')}
+ >
+ sync
+ </button>
+ <TextAreaControl
+ name="expr"
+ language="sql"
+ initialValue={initialValue}
+ onChange={() => {}}
+ />
+ </>
+ );
+ }
+ const { container } = render(<Wrapper />);
+ await waitFor(() => {
+ expect(container.querySelector('.ace_text-input')).toBeInTheDocument();
+ });
+ // react-ace keeps a reference to the live editor instance on the
+ // container node's `env`, which lets us read the underlying Ace
+ // document directly; jsdom doesn't paint Ace's text layer, so the
+ // rendered DOM has no visible text to assert against.
+ const editorNode = container.querySelector('.ace_editor') as HTMLElement &
{
+ env: { editor: { getValue: () => string } };
+ };
+ expect(editorNode.env.editor.getValue()).toBe('first');
+
+ fireEvent.click(screen.getByText('sync'));
+
+ await waitFor(() => {
+ expect(editorNode.env.editor.getValue()).toBe('synced-from-source');
+ });
+ // Same editor instance picked up the new value; it was not remounted.
+ expect(container.querySelector('.ace_editor')).toBe(editorNode);
+ });
+
+ test('does not re-notify onChange when an external sync sets the editor
value', async () => {
+ const onChange = jest.fn();
+ function Wrapper() {
+ const [initialValue, setInitialValue] = useState('first');
+ return (
+ <>
+ <button
+ type="button"
+ onClick={() => setInitialValue('synced-from-source')}
+ >
+ sync
+ </button>
+ <TextAreaControl
+ name="expr"
+ language="sql"
+ initialValue={initialValue}
+ onChange={onChange}
+ />
+ </>
+ );
+ }
+ const { container } = render(<Wrapper />);
+ await waitFor(() => {
+ expect(container.querySelector('.ace_text-input')).toBeInTheDocument();
+ });
+ const editorNode = container.querySelector('.ace_editor') as HTMLElement &
{
+ env: { editor: { getValue: () => string } };
+ };
+
+ fireEvent.click(screen.getByText('sync'));
+
+ await waitFor(() => {
+ expect(editorNode.env.editor.getValue()).toBe('synced-from-source');
+ });
+ // Ace's setValue() fires its own change event; that echo must not be
+ // mistaken for the user typing and bounced back up through onChange.
+ expect(onChange).not.toHaveBeenCalled();
+ });
+
+ test('keeps syncing the inline editor after the edit-in-modal dialog is
opened and closed', async () => {
+ function Wrapper() {
+ const [initialValue, setInitialValue] = useState('first');
+ return (
+ <>
+ <button
+ type="button"
+ onClick={() => setInitialValue('synced-from-source')}
+ >
+ sync
+ </button>
+ <TextAreaControl
+ name="expr"
+ language="sql"
+ initialValue={initialValue}
+ onChange={() => {}}
+ />
+ </>
+ );
+ }
+ const { container } = render(<Wrapper />);
+ await waitFor(() => {
+ expect(container.querySelector('.ace_text-input')).toBeInTheDocument();
+ });
+ const inlineEditorNode = container.querySelector(
+ '.ace_editor',
+ ) as HTMLElement & {
+ env: { editor: { getValue: () => string } };
+ };
+
+ // Open, then close, the "edit in modal" dialog. Its own Ace instance
+ // mounts and is destroyed (destroyOnHidden) in the process; this used
+ // to leave the single shared editor ref pointing at that destroyed
+ // instance instead of the still-visible inline editor.
+ // ModalTrigger wraps its triggerNode in its own role="button" element,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Ambiguous role query</b></div>
<div id="fix">
This query matches two elements: `ModalTrigger` wraps `triggerNode` in its
own div[role=button] (`ModalTrigger/index.tsx`), and the nested antd `Button`
is also role button. Both have accessible name 'Edit sql in modal', so
`getByRole` throws 'Found multiple elements'. Click the wrapper via its stable
testid instead.
</div>
</div>
<small><i>Code Review Run #e907e4</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/TextAreaControl.test.tsx:
##########
@@ -61,4 +62,139 @@ describe('TextArea', () => {
fireEvent.change(textArea, { target: { value: 'x' } });
expect(defaultProps.onChange).toHaveBeenCalledWith('x');
});
+
+ test('picks up an externally changed initialValue without remounting the
AceEditor', async () => {
+ function Wrapper() {
+ const [initialValue, setInitialValue] = useState('first');
+ return (
+ <>
+ <button
+ type="button"
+ onClick={() => setInitialValue('synced-from-source')}
+ >
+ sync
+ </button>
+ <TextAreaControl
+ name="expr"
+ language="sql"
+ initialValue={initialValue}
+ onChange={() => {}}
+ />
+ </>
+ );
+ }
+ const { container } = render(<Wrapper />);
+ await waitFor(() => {
+ expect(container.querySelector('.ace_text-input')).toBeInTheDocument();
+ });
+ // react-ace keeps a reference to the live editor instance on the
+ // container node's `env`, which lets us read the underlying Ace
+ // document directly; jsdom doesn't paint Ace's text layer, so the
+ // rendered DOM has no visible text to assert against.
+ const editorNode = container.querySelector('.ace_editor') as HTMLElement &
{
+ env: { editor: { getValue: () => string } };
+ };
+ expect(editorNode.env.editor.getValue()).toBe('first');
+
+ fireEvent.click(screen.getByText('sync'));
+
+ await waitFor(() => {
+ expect(editorNode.env.editor.getValue()).toBe('synced-from-source');
+ });
+ // Same editor instance picked up the new value; it was not remounted.
+ expect(container.querySelector('.ace_editor')).toBe(editorNode);
+ });
+
+ test('does not re-notify onChange when an external sync sets the editor
value', async () => {
+ const onChange = jest.fn();
+ function Wrapper() {
+ const [initialValue, setInitialValue] = useState('first');
+ return (
+ <>
+ <button
+ type="button"
+ onClick={() => setInitialValue('synced-from-source')}
+ >
+ sync
+ </button>
+ <TextAreaControl
+ name="expr"
+ language="sql"
+ initialValue={initialValue}
+ onChange={onChange}
+ />
+ </>
+ );
+ }
+ const { container } = render(<Wrapper />);
+ await waitFor(() => {
+ expect(container.querySelector('.ace_text-input')).toBeInTheDocument();
+ });
+ const editorNode = container.querySelector('.ace_editor') as HTMLElement &
{
+ env: { editor: { getValue: () => string } };
+ };
+
+ fireEvent.click(screen.getByText('sync'));
+
+ await waitFor(() => {
+ expect(editorNode.env.editor.getValue()).toBe('synced-from-source');
+ });
+ // Ace's setValue() fires its own change event; that echo must not be
+ // mistaken for the user typing and bounced back up through onChange.
+ expect(onChange).not.toHaveBeenCalled();
+ });
+
+ test('keeps syncing the inline editor after the edit-in-modal dialog is
opened and closed', async () => {
+ function Wrapper() {
+ const [initialValue, setInitialValue] = useState('first');
+ return (
+ <>
+ <button
+ type="button"
+ onClick={() => setInitialValue('synced-from-source')}
+ >
+ sync
+ </button>
+ <TextAreaControl
+ name="expr"
+ language="sql"
+ initialValue={initialValue}
+ onChange={() => {}}
+ />
+ </>
+ );
+ }
+ const { container } = render(<Wrapper />);
+ await waitFor(() => {
+ expect(container.querySelector('.ace_text-input')).toBeInTheDocument();
+ });
+ const inlineEditorNode = container.querySelector(
+ '.ace_editor',
+ ) as HTMLElement & {
+ env: { editor: { getValue: () => string } };
+ };
+
+ // Open, then close, the "edit in modal" dialog. Its own Ace instance
+ // mounts and is destroyed (destroyOnHidden) in the process; this used
+ // to leave the single shared editor ref pointing at that destroyed
+ // instance instead of the still-visible inline editor.
+ // ModalTrigger wraps its triggerNode in its own role="button" element,
+ // so a <Button> triggerNode (as used here) yields two nested elements
+ // matching role "button" with the same accessible name; target the
+ // wrapper directly via its data-test id instead.
+ fireEvent.click(screen.getByTestId('span-modal-trigger'));
+ await waitFor(() => {
+ expect(container.querySelectorAll('.ace_editor')).toHaveLength(2);
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Portal outside container</b></div>
<div id="fix">
`ModalTrigger` renders antd `Modal`, which portals to `document.body`
(`Modal.tsx` passes no `getContainer` override), so the modal's Ace editor
never appears inside RTL's `container`; `toHaveLength(2)` times out. Query
`document` (or `screen`) for the modal editor count.
</div>
</div>
<small><i>Code Review Run #e907e4</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
