bito-code-review[bot] commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r3453811409
##########
superset-frontend/packages/superset-core/src/sqlLab/index.ts:
##########
@@ -62,6 +62,13 @@ export interface Tab {
*/
id: string;
+ /**
+ * The stable backend-assigned ID for this tab (the tabstateview integer ID).
+ * Set once the tab has been persisted to the backend. Undefined for new tabs
+ * before the first backend sync.
+ */
+ backendId?: string;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Type mismatch with backend API</b></div>
<div id="fix">
The type `string` contradicts the docstring on line 66 which explicitly
states 'integer ID', and the backend API returns `tab_state_id: number` (see
`sqlLab.ts:46` and `sqlLab.ts:869`). This mismatch could cause type errors and
runtime issues.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset-frontend/packages/superset-core/src/sqlLab/index.ts (lines
65-71) ---
65: /**
66: * The stable backend-assigned ID for this tab (the tabstateview
integer ID).
67: * Set once the tab has been persisted to the backend. Undefined for
new tabs
68: * before the first backend sync.
69: */
70: - backendId?: string;
70: + backendId?: number;
71:
/**
```
</div>
</details>
</div>
<small><i>Code Review Run #68d738</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/index.tsx:
##########
@@ -271,6 +279,44 @@ const SqlEditor: FC<Props> = ({
const logAction = useLogAction({ queryEditorId: queryEditor.id });
const isActive = currentQueryEditorId === queryEditor.id;
+
+ // Re-renders when an extension registers a northPane view after async load.
+ const northPaneViews = useViews(ViewLocations.sqllab.northPane) || [];
+
+ // ID of the northPane view active for this tab, or null for the default
+ // SQL editor layout. Set by an extension via PENDING_NORTH_PANE_VIEW_KEY
+ // before calling createTab(); persisted per-tab in localStorage.
+ const [northPaneViewId, setNorthPaneViewId] = useState<string | null>(() => {
+ const pendingViewId = localStorage.getItem(PENDING_NORTH_PANE_VIEW_KEY);
+ if (pendingViewId) {
+ localStorage.removeItem(PENDING_NORTH_PANE_VIEW_KEY);
+ localStorage.setItem(NORTH_PANE_VIEW_KEY(queryEditor.id), pendingViewId);
+ return pendingViewId;
+ }
+ return localStorage.getItem(NORTH_PANE_VIEW_KEY(queryEditor.id));
+ });
+
+ useEffect(() => {
+ const persistKey = NORTH_PANE_VIEW_KEY(
+ queryEditor.tabViewId ?? queryEditor.id,
+ );
+ if (northPaneViewId) {
+ localStorage.setItem(persistKey, northPaneViewId);
+ } else {
+ localStorage.removeItem(persistKey);
+ }
+ }, [queryEditor.tabViewId, queryEditor.id, northPaneViewId]);
+
+ useEffect(() => {
+ const handler = (e: StorageEvent) => {
+ if (e.key === NORTH_PANE_VIEW_KEY(queryEditor.id)) {
+ setNorthPaneViewId(e.newValue || null);
+ }
+ };
+ window.addEventListener('storage', handler);
+ return () => window.removeEventListener('storage', handler);
+ }, [queryEditor.id]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for northPane feature</b></div>
<div id="fix">
The northPane view feature (lines 124-318) introduces new localStorage-based
state management with storage event listeners for cross-tab sync.
SqlEditor.test.tsx has zero test coverage for any northPane-related logic. Per
BITO.md rule [11730], new features should include comprehensive unit tests
covering success paths, error scenarios, and edge cases.
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Storage listener key mismatch</b></div>
<div id="fix">
The storage event listener on line 312 only watches
`NORTH_PANE_VIEW_KEY(queryEditor.id)`, but the persistence effect on lines
299-308 writes to `NORTH_PANE_VIEW_KEY(queryEditor.tabViewId ??
queryEditor.id)`. When `tabViewId` differs from `id`, cross-tab synchronization
via the `storage` event will silently miss updates to the persisted key,
breaking multi-tab state sync.
</div>
</div>
<small><i>Code Review Run #68d738</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]
