bito-code-review[bot] commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r4079425617
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Blank pane during async view load</b></div>
<div id="fix">
When `northPaneViewId` is set but the view is not yet registered (extension
loads async after mount, per the comment on `useViews`), this branch renders
nothing: the ternary falls through to `showEmptyState && !hasSqlStatement` and
then to `queryPane()` only when `showEmptyState` is falsy. With an empty editor
and `showEmptyState` true, the whole north pane blanks out until registration
completes. Consider falling back to `queryPane()` while the view is pending.
</div>
</div>
<small><i>Code Review Run #b6e90e</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/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:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate tab id in store</b></div>
<div id="fix">
Appending `{ ...extraQueryEditor1, northPaneViewId }` to
`mockInitialState.sqlLab.queryEditors` duplicates id 'diekd23': fixtures'
`initialState` already contains `extraQueryEditor1` (fixtures.ts:719). The
store then holds an impossible two-tabs-one-id state, and any id-based lookup
resolves the plain entry, shadowing the view-carrying one. Stamp the existing
entry via `.map` so setup matches the scenario described.
</div>
</div>
<small><i>Code Review Run #b6e90e</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]
