bito-code-review[bot] commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r4154937270
##########
superset-frontend/src/core/sqlLab/index.ts:
##########
@@ -440,7 +487,7 @@ const onDidCloseTab: typeof sqlLabApi.onDidCloseTab = (
action.queryEditor.dbId ?? 0,
action.queryEditor.catalog,
action.queryEditor.schema,
- true, // closed
+ { closed: true, backendId: resolveBackendId(action.queryEditor) },
Review Comment:
<!-- Bito Reply -->
The suggestion to fall back to the editor ID is intended to prevent the loss
of the backend ID when a tab is closed. Since the current implementation of
`resolveBackendId` returns `undefined` for local-only tabs, the reviewer's
concern is that listeners will receive a tab without a backend ID, potentially
reintroducing a previously identified bug. If the current behavior is
intentional to avoid that specific regression, you may choose to ignore this
suggestion.
**superset-frontend/src/core/sqlLab/index.ts**
```
- true, // closed
+ { closed: true, backendId: resolveBackendId(action.queryEditor) },
```
##########
superset-frontend/src/core/sqlLab/index.ts:
##########
@@ -557,6 +605,9 @@ const createTab: typeof sqlLabApi.createTab = async (
inheritedValues.queryLimit ?? common?.conf?.DEFAULT_SQLLAB_LIMIT,
autorun: false,
name,
+ ...(options?.northPaneViewId && {
+ northPaneViewId: options.northPaneViewId,
+ }),
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to plumb `northPaneViewId` through the `Tab` model
is based on the observation that the current implementation accepts the option
but does not persist it in the returned `Tab` object. If the application
architecture intentionally manages `northPaneViewId` via the `queryEditor`'s
Redux state rather than the `Tab` model, the suggestion may be unnecessary for
the intended data flow. You may choose to ignore this suggestion if the current
state management pattern is the established standard for this component.
**superset-frontend/src/core/sqlLab/index.ts**
```
+ ...(options?.northPaneViewId && {
+ northPaneViewId: options.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]
