bito-code-review[bot] commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r4081164337
##########
superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx:
##########
@@ -397,6 +400,93 @@ describe('SqlEditor', () => {
).toBeInTheDocument();
});
+ test('renders a registered northPane view in place of the editor', async ()
=> {
+ const { queryEditor } = mockedProps;
+ // The fixture has no tabViewId, so the component falls back to the id;
+ // mirror that here to derive the same persistence key.
+ const storageKey = `sqllab.northPaneView.${queryEditor.id}`;
+ localStorage.setItem(storageKey, 'test.northPane');
+ const disposable = views.registerView(
+ { id: 'test.northPane', name: 'Test North Pane' },
+ ViewLocations.sqllab.northPane,
+ () => <div data-test="np-view">NorthPane content</div>,
+ );
+
+ try {
+ const { findByTestId, queryByTestId } = setup(mockedProps, store);
+ expect(await findByTestId('np-view')).toBeInTheDocument();
+ // The default SQL editor pane is replaced, not rendered alongside.
+ expect(queryByTestId('react-ace')).not.toBeInTheDocument();
+ } finally {
+ disposable.dispose();
+ localStorage.removeItem(storageKey);
+ }
+ });
+
+ test('opens the northPane view carried on the query editor and persists the
per-tab key', async () => {
+ const { queryEditor } = mockedProps;
+ // The fixture has no tabViewId, so the component falls back to the id.
+ const storageKey = `sqllab.northPaneView.${queryEditor.id}`;
+ const disposable = views.registerView(
+ { id: 'test.northPane', name: 'Test North Pane' },
+ ViewLocations.sqllab.northPane,
+ () => <div data-test="np-view">NorthPane content</div>,
+ );
+
+ try {
+ // createTab({ northPaneViewId }) stamps the view onto the tab's own
+ // state, so nothing shared between tabs is involved.
+ const { findByTestId, queryByTestId } = setup(
+ {
+ ...mockedProps,
+ queryEditor: { ...queryEditor, northPaneViewId: 'test.northPane' },
+ },
+ store,
+ );
+ expect(await findByTestId('np-view')).toBeInTheDocument();
+ expect(queryByTestId('react-ace')).not.toBeInTheDocument();
+ // The chosen view is persisted under the per-tab key so it survives a
+ // reload that rehydrates the editor without the field.
+ expect(localStorage.getItem(storageKey)).toEqual('test.northPane');
+ } finally {
+ disposable.dispose();
+ localStorage.removeItem(storageKey);
+ }
+ });
+
+ test('does not open a northPane view requested for a different tab', () => {
+ const { queryEditor } = mockedProps;
+ const storageKey = `sqllab.northPaneView.${queryEditor.id}`;
+ const disposable = views.registerView(
+ { id: 'test.northPane', name: 'Test North Pane' },
+ ViewLocations.sqllab.northPane,
+ () => <div data-test="np-view">NorthPane content</div>,
+ );
+ // Another tab was created with the view; this one mounts as a plain tab
+ // (as happens when a reload restores several tabs at once).
+ const otherTabStore = createStore({
+ ...mockInitialState,
+ sqlLab: {
+ ...mockInitialState.sqlLab,
+ queryEditors: [
+ ...mockInitialState.sqlLab.queryEditors,
+ { ...extraQueryEditor1, northPaneViewId: 'test.northPane' },
+ ],
Review Comment:
<!-- Bito Reply -->
The update to the fixture is correct. By using `.map` to update the existing
entry instead of appending a new one, you avoid duplicating the tab ID and
ensure the store state correctly reflects the intended scenario without
shadowing the view-carrying entry.
##########
superset-frontend/src/SqlLab/components/SqlEditor/index.tsx:
##########
@@ -1048,6 +1127,29 @@ const SqlEditor: FC<Props> = ({
>
<Skeleton active />
</div>
+ ) : northPaneViewId &&
+ northPaneViews.some(v => v.id === northPaneViewId) ? (
Review Comment:
<!-- Bito Reply -->
The change correctly addresses the issue where the north pane would appear
blank while waiting for an asynchronous view registration. By falling back to
`queryPane()` in the ternary condition, the editor now maintains a consistent
UI state during the loading process.
**superset-frontend/src/SqlLab/components/SqlEditor/index.tsx**
```
+ ) : northPaneViewId &&
+ northPaneViews.some(v => v.id === northPaneViewId) ? (
```
--
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]
