sadpandajoe commented on code in PR #44373:
URL: https://github.com/apache/superset/pull/44373#discussion_r4049546012
##########
superset-frontend/src/core/editors/AceEditorProvider.test.tsx:
##########
@@ -188,3 +194,37 @@ test('selection callback receives correct range format',
async () => {
{ start: { line: 0, column: 0 }, end: { line: 0, column: 10 } },
]);
});
+
+test('onChange emits once per task with the final value when the editor fires
several change events', async () => {
+ // With several cursors Ace applies a keystroke per selection, so react-ace
+ // reports an intermediate document value (first cursor applied) and then
+ // the final one within the same task. The provider must coalesce them:
+ // an intermediate value rendered back into the controlled `value` prop
+ // makes react-ace call editor.setValue() mid-keystroke, which collapses
+ // the multi-selection and moves a cursor to the document end.
+ const onChange = jest.fn();
+ renderEditor({ onChange });
+
+ const editorOnChange = mockEditorProps.onChange as (v: string) => void;
+ expect(editorOnChange).toBeDefined();
+
+ editorOnChange('abcX');
+ editorOnChange('abcX\ndefX');
+
+ expect(onChange).not.toHaveBeenCalled();
+
+ await waitFor(() => expect(onChange).toHaveBeenCalledTimes(1));
+ expect(onChange).toHaveBeenCalledWith('abcX\ndefX');
Review Comment:
If the scheduling flag stops resetting after a flush, every later keystroke
is silently buffered without reaching the consumer, but both new tests still
pass because they exercise only the first flush. Could this test send another
change after awaiting the first flush and assert a second callback with the new
value?
##########
superset-frontend/src/core/editors/AceEditorProvider.tsx:
##########
@@ -277,6 +277,58 @@ const AceEditorProvider = forwardRef<EditorHandle,
EditorProps>(
new Map(),
);
+ // Ace emits one session change event per cursor while several cursors are
+ // active: a two-cursor keystroke produces an intermediate document value
+ // (first cursor applied) followed by the final one. Each emission is a
+ // candidate React state update; when React renders with an intermediate
+ // value while the editor already holds the final one, react-ace's
+ // componentDidUpdate sees getValue() !== props.value and calls
+ // editor.setValue() mid-keystroke, collapsing the multi-selection and
+ // moving a cursor to the document end. Buffer the latest value and flush
+ // once per task so only the final document value reaches the consumer.
+ const pendingValueRef = useRef<string | null>(null);
+ const flushScheduledRef = useRef(false);
+ const onChangeRef = useRef(onChange);
+
+ useEffect(() => {
+ onChangeRef.current = onChange;
+ }, [onChange]);
+
+ const flushPendingValue = useCallback(() => {
+ flushScheduledRef.current = false;
+ const pending = pendingValueRef.current;
+ pendingValueRef.current = null;
+ if (pending !== null) {
+ onChangeRef.current(pending);
+ }
+ }, []);
+
+ useEffect(
+ () => () => {
+ // A keystroke can be in flight when the editor unmounts (tab switch);
+ // deliver its value rather than dropping it silently.
+ if (flushScheduledRef.current) {
+ if (typeof queueMicrotask === 'function') {
+ queueMicrotask(flushPendingValue);
+ } else {
+ flushPendingValue();
+ }
+ }
+ },
+ [flushPendingValue],
+ );
+
+ const handleChange = useCallback(
+ (nextValue: string) => {
+ pendingValueRef.current = nextValue;
+ if (!flushScheduledRef.current) {
+ flushScheduledRef.current = true;
+ queueMicrotask(flushPendingValue);
Review Comment:
Backspace emits Ace's cursor-change event before its document-change event,
so SQL Lab's cursor-position Redux update can render before this microtask
while `value` still contains the old SQL. React-ace then replaces the edited
document with the old value and replaces it again when the flush arrives,
adding whole-document changes to the edit/undo stream; could the controlled
value stay current during those cursor-triggered renders, with a regression
test for that ordering?
--
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]
