EnxDev commented on PR #43330:
URL: https://github.com/apache/superset/pull/43330#issuecomment-5343465737
## EnxDev's Review Agent โ apache/superset#43330 ยท HEAD 356b916
**comment** โ Right fix, right pattern, real regression tests. But `state
=== Success` is a proxy for "we have columns for this SQL", and it misses the
reload/cached path โ the button is still enabled there with zero columns, which
is the same hole the PR sets out to close.
CI is green (67 passing, 0 failing); codecov reports all modified lines
covered. Title is Conventional-Commits valid.
### ๐ด Functional
- **`superset-frontend/src/SqlLab/components/SqlEditor/index.tsx:869`** ยท
_High_ โ A query can be `state === 'success'` with `results` absent from the
store: that exact combination is handled at `ResultSet/index.tsx:828`
(`query.cached || (query.state === QueryState.Success && !query.results)`),
which renders *Fetch data preview* / *Refetch results*. It's reachable after a
page reload โ `getInitialState.ts:147` hydrates `queries` from the bootstrap
payload, and the queries API exposes `results_key`, not the result rows
(`superset/queries/api.py:104`). In that state the new gate returns `true`, so
**Save dataset** is enabled while `columns={latestQuery?.results?.columns ||
[]}` (line 862) is `[]`.
Impact, for any user who reloads SQL Lab and clicks the button: the
**Overwrite** path PUTs `columns: []` with `override_columns=true`
(`SaveDatasetModal/index.tsx:178`, `:285`, `:293`), wiping the target dataset's
column metadata; the **Create** path makes a dataset whose Explore `form_data`
carries `all_columns: []`. Overwrite is the destructive one โ it silently drops
columns on an existing dataset.
Fix is the condition you already offered varfigstar โ gate on columns, not
on state:
```
canSaveDataset={
latestQuery?.state === QueryState.Success &&
!!latestQuery?.results?.columns?.length
}
```
That subsumes the failed-query case and covers cached/reloaded results
too. **regression test:** in `SqlEditor.test.tsx`, add a third case to
`setupWithQueryState` with `{ ...latestQuery, state: QueryState.Success,
results: undefined }` and assert the button is disabled โ it renders enabled at
this HEAD, which is the gap.
### ๐ก Should-fix
- **`SaveDatasetActionButton/index.tsx:33`, `SaveQuery/index.tsx:85`** โ
`canSaveDataset` defaults to `true`, so the guard fails open if a call site
omits it. There's exactly one call site (`SqlEditor/index.tsx:869`); make the
prop required on both components so a future consumer can't silently re-open
the hole.
- **`SqlEditor/index.tsx:869`** โ On the stale-SQL thread: agreed the
underlying bug predates this PR, so it's fine to leave. Worth noting the
affordance changed though โ the enabled state now *asserts* "the query ran
successfully", and after editing SQL post-success it shows *Save or Overwrite
Dataset* over columns that belong to different SQL. Gating on `results.columns`
doesn't fix that either; a follow-up would need to compare `latestQuery.sql`
against the editor's current SQL.
### ๐ Praise
- `SqlEditor.test.tsx:358-366` โ testing both directions (enabled on
`Success`, disabled on `Failed`) with `results` left populated from
`queries[0]` reproduces the actual bug: stale columns from an earlier success.
That's a real regression guard, not a smoke test.
- `SaveDatasetActionButton.test.tsx:60` โ hovering the wrapper is correct,
not a workaround: the core `Button` wraps disabled buttons in a `<span>`
precisely so the tooltip fires
(`packages/superset-ui-core/src/components/Button/index.tsx:246`).
### Note on the open thread
The DB-goes-down scenario raised in the thread doesn't cost anything today:
`START_QUERY` sets `latestQueryId` to the new query (`reducers/sqlLab.ts:386`),
so a failed re-run immediately makes `latestQuery` the failed one and `columns`
resolves to `[]`. Before this PR that path produced a **column-less dataset**,
not a good one โ so disabling the button there is the correct outcome, not a
capability loss.
<!-- enxdev-review-agent:356b916 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 356b916._
--
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]
