codeant-ai-for-open-source[bot] commented on code in PR #44858:
URL: https://github.com/apache/superset/pull/44858#discussion_r4152269544
##########
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:
Yes, this is a valid issue. `editorRef` is overwritten whenever either Ace
instance loads, and it is not cleared when the modal instance unmounts.
Consequently, after closing the modal, an external `initialValue` update can
target the destroyed modal editor and leave the visible inline editor stale.
Track the mounted editors independently and update every live instance:
```tsx
const inlineEditorRef = useRef<AceEditorHandle | null>(null);
const modalEditorRef = useRef<AceEditorHandle | null>(null);
const onEditorLoad = useCallback(
(editor: AceEditorHandle, inModal: boolean) => {
const ref = inModal ? modalEditorRef : inlineEditorRef;
ref.current = editor;
hotkeys?.forEach(keyConfig => {
editor.commands.addCommand({
name: keyConfig.name,
bindKey: { win: keyConfig.key, mac: keyConfig.key },
exec: keyConfig.func,
});
});
},
[hotkeys],
);
```
The modal renderer should clear its ref on unmount, and the synchronization
effect should iterate over both refs, preserving each instance’s cursor
position. A regression test should cover:
1. Mount the inline editor.
2. Open the modal, then close it.
3. Change `initialValue` externally.
4. Assert that the inline editor updates and no destroyed modal editor is
accessed.
This is worth fixing because the current single-ref implementation can cause
exactly the stale inline editor behavior described in the comment.
##########
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:
Yes. The restore order should be refreshed when the underlying
`propsCollection` changes while a sort is active. Otherwise, clearing the sort
uses IDs from the pre-sync collection, which can restore stale ordering and
mishandle added or removed rows.
A minimal fix is to update `unsortedOrderRef` from the newly received
collection whenever `propsCollection` changes, regardless of the current sort
state:
```tsx
useEffect(() => {
const { collection: newCollection, collectionArray: newCollectionArray } =
createKeyedCollection(propsCollection);
setCollection(newCollection);
setCollectionArray(newCollectionArray);
unsortedOrderRef.current = newCollectionArray.map(item => item.id);
}, [propsCollection]);
```
The existing `collectionArray`/`sort` effect can remain for tracking local
edits made while unsorted. This ensures a source sync establishes the new
canonical row order, while the clear-sort path still reads current row values
from `collection`, preserving edits made after the sync. A regression test
should cover syncing a reordered/changed collection while sorted, then clearing
the sort.
--
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]
