sadpandajoe commented on code in PR #44858:
URL: https://github.com/apache/superset/pull/44858#discussion_r4152266692
##########
superset-frontend/src/explore/components/controls/TextAreaControl.tsx:
##########
@@ -157,6 +175,27 @@ function TextAreaControl({
[hotkeys],
);
+ // Pick up an `initialValue` change that didn't originate from this
+ // editor's own typing (handleChange above would have already updated
+ // lastEditorValueRef for that case), without remounting the Ace instance
+ // or losing the user's current cursor position.
+ useEffect(() => {
+ const nextValue = initialValue ?? value;
+ const editor = editorRef.current;
+ if (
+ editor &&
+ nextValue !== undefined &&
+ nextValue !== lastEditorValueRef.current &&
Review Comment:
If initialValue changes from A to B while the async editor is loading, Ace
mounts with B but this ref still holds A; a later external reset to A is then
ignored and leaves B visible. Should onEditorLoad initialize the tracked value
from the editor buffer so that reset is not mistaken for a typing echo?
##########
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:
##########
@@ -341,24 +354,38 @@ export default function CRUDCollection({
return mStr.localeCompare(nStr);
};
+ // Sort the live, edited collection rather than propsCollection, so
+ // an edit that hasn't round-tripped back through onChange yet
+ // isn't dropped when a sort is applied.
+ sortedArray = [...collectionArray];
sortedArray.sort((a: CollectionItem, b: CollectionItem) =>
compareSort(a[col] as Sort, b[col] as Sort),
);
if (newSortOrder === SortOrderEnum.Desc) {
sortedArray.reverse();
}
} else {
- const { collectionArray: resetArray } =
- createKeyedCollection(propsCollection);
- sortedArray = resetArray;
+ // Restore the pre-sort order, but take each row's current value
+ // from the live `collection` map (not propsCollection) so an edit
+ // made while sorted survives clearing the sort. Any row not part
+ // of the tracked order (e.g. added while sorted) is appended.
+ const trackedIds = new Set(unsortedOrderRef.current);
+ sortedArray = unsortedOrderRef.current
+ .filter(id => collection[id])
+ .map(id => collection[id]);
Review Comment:
Agreed—after a source sync changes the column order while sorted, clearing
the sort restores the old order, and the next edit sends that order back to the
parent. Should the restore order track changes to the underlying collection as
well?
##########
superset-frontend/src/explore/components/controls/TextAreaControl.tsx:
##########
@@ -137,15 +162,8 @@ function TextAreaControl({
);
const onEditorLoad = useCallback(
- (editor: {
- commands: {
- addCommand: (cmd: {
- name: string;
- bindKey: { win: string; mac: string };
- exec: () => void;
- }) => void;
- };
- }) => {
+ (editor: AceEditorHandle) => {
+ editorRef.current = editor;
Review Comment:
Agreed—opening and closing the modal leaves this ref pointing at its
destroyed editor, so a later external update misses the visible inline editor.
Should the two editor instances be tracked separately, with an open → close →
external-update regression test?
--
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]
