kgabryje commented on PR #41551:
URL: https://github.com/apache/superset/pull/41551#issuecomment-5165080487
# Round 2 — verification of the response batch, re-pinned to `91706dff80`
Re-ran the same reviewers over `075241a2f7..3b16cc409e`, then re-verified
everything at `91706dff80` after you pushed the staleness-edge commit mid-pass.
Every claim below re-checked at that head myself. Thanks for the
commit-by-commit disposition — it made this a fast pass.
**Most of the batch is right, and the headline fixes are solid.** Two things
need another look: the batch introduces **two regressions of its own**, both in
code added to fix round-1 findings, and one deferral rests on a premise the
source contradicts.
---
## Confirmed fixed
**The re-selection wedge, and the test is genuinely load-bearing.** The
reducer guard closes both routes. I walked the state machine and it holds on
every path: exit → reselect the same version works (`CLEAR_VERSION_PREVIEW`
nulls `preview`, so the `?.` makes the comparison miss), close-panel → reopen
works, A → B → A works, and restore-while-previewing works because
`confirmRestore` clears the preview before dispatching `versionRestored`.
`appliedVersionRef` is nulled on both the restore and exit branches, and inside
`apply()` the staleness check precedes the ref write, so a superseded apply
can't set it.
More usefully: **the adopted test was run against a reverted reducer and it
fails.**
```
$ npx jest src/features/versionHistory/previewReselect # with the guard
commented out
✕ re-selecting the already-previewed version does not re-enter applying
Expected: false
Received: true
> 125 | expect(applying()).toBe(false);
$ git checkout -- .../reducer.ts && npx jest .../previewReselect
Tests: 1 passed
```
Not a tautology. Suites also stay green — the round-1 commands go 274 → 277
with no regressions, and 186/186 including the newly-touched
`DatasourceControl` and `Tab` suites.
Also verified: `Control.tsx:87-91` marked; `UPDATE_CHART_TITLE` logged with
the constant genuinely pinned to `exploreActions` by a test — and it has
exactly one dispatcher (the editable title's `onSave`, which only fires when
actually dirty), so it can't manufacture phantom entries; `Tab.tsx:159-166`
gated; `hydrateWith` symmetric across entry/exit/restore with **no fourth call
site**; `ActivityOperation` `'update'` reaching `describeRecord`'s generic tail
with no dead branch; the docs caveat now honest about preview fidelity.
A re-sweep for unmarked `setControlValue` inside effects across `src/`,
`packages/` and `plugins/` now comes back **clean for core explore code**. The
only residue is `plugin-chart-cartodiagram/.../OlChartMap.tsx:217`, which is
structurally unmarkable through the plugin bridge — consistent with what you
noted.
**And `91706dff80` closed one I was about to file.** My draft had the
probe's reset boundary as a Low: `probeIdRef` was bumped when the reset's *own*
probe launched rather than at `fetchPage` entry, so on an A → B switch A's
probe could resolve before B's page-0 response and repopulate `newestGroup`.
Moving the bump to `:115` is exactly the fix. Dropped — codeant got there first.
---
## 1. `308f5cb859` over-cancels: a search keystroke silently drops the
post-restore rehydration
Still present at `91706dff80` (`ExploreVersionHistory.tsx:189`, cleanup
`:226`, `refreshActivity` in deps at `:232`).
The `cancelled` flag is scoped to the **effect run**, so its cleanup fires
on every re-run, not only on unmount. The dependency array includes
`refreshActivity`, whose identity is not stable:
- `refreshActivity` = `activity.refresh` = `useCallback(…, [fetchPage])`
(`useVersionActivity.ts:249-251`)
- `fetchPage` = `useCallback(…, [entityType, uuid, include, q])`
- `q` is the **debounced search term** (`ExploreVersionHistory.tsx:153-159`)
So every time the debounce fires on a new search string — or the include
filter changes — the effect re-runs and cancels the in-flight
`fetchExploreRehydrationData`. And it never re-issues:
`lastRestoreCountRef.current` was already synced, so the re-run takes the `else
if` branch.
**Repro:** restore a version, then type in the panel's search box within the
rehydration round trip (~200–800 ms). The timeline shows the new "Restored
version" entry and the success toast fired, but the chart still renders the
**pre-restore** state — and the failure toast is suppressed too, so there's no
signal. Only a reload recovers. That's worse than the bug the commit fixed,
because the page silently contradicts itself.
The goal is right — an unmount or a save-as slice swap really should
invalidate the hydrate. The issue is binding the guard to *effect identity*
rather than to the thing being guarded. **The correct pattern is the one you
used twice in this same batch**: a dedicated counter bumped only by unmount or
a newer restore, as in the newest-self probe and the dashboard preview's
unmount cleanup. A `restoreHydrationIdRef`, or a resolve-time re-check that
`sliceId`/`uuid` still match, closes it.
## 2. `d704f6ae88` makes an Edit Dataset reconciliation invisible to the
restore dirty gate
Still present at `91706dff80` (`DatasourceControl/index.tsx:276`).
The mark is correct for a dataset **swap**: `ChangeDatasourceModal:187`
calls `onChange(...)`, which routes through `Control.tsx` and emits the primary
non-programmatic `SET_FIELD_VALUE` for `datasource`, so the log still goes
dirty and the derived granularity write is rightly suppressed as a duplicate.
But `handleDatasourceSave` also serves **Edit Dataset**, and that route
passes no `onChange` — `DatasourceModal` receives only `onDatasourceSave`
(`DatasourceControl/index.tsx:595-600`). It dispatches `changeDatasource`,
which emits `SET_DATASOURCE` + `UPDATE_FORM_DATA_BY_DATASOURCE`, neither of
which the middleware records.
So when editing dataset metadata invalidates the chart's time column
(`savedDatasource.type === 'table' && !isGranularitySqlaTemporal`), the chart's
`granularity_sqla` is rewritten and **that write was the only session entry on
this path** — now suppressed. The form has changed while `useVersionActions`
sees an empty log, so a restore rehydrates Explore and discards the reconciled
value with no warning. Before this commit that path blocked restore.
The new tests do exercise Edit Dataset — `openAndSaveChanges` clicks
`edit-dataset` — but assert only that the fourth argument is `{programmatic:
true}`, never the resulting dirty behaviour.
The derived write can stay programmatic; the dirty signal needs to account
for the primary datasource reconciliation. Which is really an argument for the
baseline-diffing fix you've already queued, rather than another point patch.
## 3. The dashboard `extra_editors` deferral — the payload does carry the
field
Still `editors`-only at `91706dff80` (`permissionUtils.ts:57-63`). I'd push
back on the deferral, because the fix looks frontend-only rather than
backend-first. The chain:
- `superset/dashboards/api.py:612-614` sets `result["extra_editors"] =
get_extra_editor_subject_ids(dash)` **after** `schema.dump(dash)`, so the
`columns`/`DASHBOARD_GET_COLUMNS` projection never strips it
- `useDashboard` spreads `...dashboard` wholesale
(`hooks/apiResources/dashboards.ts:52-65`)
- `hydrate.ts:358` spreads `...dashboard` into `dashboardInfo`
The field is present exactly where `canUserEditDashboard` runs. It's gated
on `EXTRA_EDITORS_RESOLVER` being configured — which is precisely the
deployment where the bug bites, so the claim fails in the only case that
matters.
Your other point stands and is the better framing: `canUserEditDashboard`
has the same omission platform-wide on the Edit button, so widening the shared
predicate to intersect `editors ∪ extra_editors` (and typing the field on
`Dashboard`) corrects Edit, the menu and restore together. That's a frontend
change, and it doesn't need to wait on a backend one.
---
## On the deferrals
Characterisations read as honest, with two scope notes:
- **Session-log false positives.** "Over-blocking, not data loss" is
accurate for the append-only/latching half. But finding 2 above shows the
framing can't cover the dirty system as a whole — the same log is the restore
safety signal, and programmatic suppression makes it fail *open*, not closed.
Also still open in this family: `AnnotationLayerControl/index.tsx:108-116`
dispatches an unmarked `setControlValue` from its validation-sync effect, so a
failing annotation query can create a phantom edit on an untouched chart.
- **Preview scalars.** The membership half genuinely needs the backend
shape. The **scalar** half doesn't: the generic snapshot already projects every
version-table scalar (`versioning/queries.py:491-501`), including
description/theme/certification/published/slug — preview just doesn't apply
them. Likewise "Open as new" is a copy-endpoint contract issue
(`DashboardCopySchema` accepts only title/CSS/metadata + `duplicate_slices`),
not a snapshot-shape dependency, and `version-history.mdx:92-96` still says it
copies "the selected version" without qualification. Both are frontend/schema
changes that don't have to wait.
- **Wire types.** The two you named remain, and the set is slightly larger:
the chart version table permits null for uuid, name, datasource id/type and viz
type, all typed non-null on the client. Still type hygiene — worth modelling
the whole shape once rather than the two examples.
One small consequence of the rename fix worth knowing: renaming a chart and
renaming it back now leaves an entry (collapsed to one, since both carry
`controlName: 'slice_name'`) and blocks restore where round-1 code didn't.
That's clearly the right trade — silently destroying a rename is far worse —
but it slightly widens the latching problem until baseline diffing lands.
Also noting, since it changed the reading: the `?edit=true` justification
comment says previews can only be entered from view mode, but the gate tests
`hasUnsavedChanges`, not `editMode` — a user can be in edit mode with nothing
dirty and preview from there. Forcing `false` is still right; the stated reason
just isn't the invariant.
Nothing above touches `canOverwriteSlice`, the restore funnel, the scoped
`VERSION_RESTORED` consumers or the flag-off path — all re-confirmed untouched
by this batch.
--
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]
