kgabryje commented on PR #41551:
URL: https://github.com/apache/superset/pull/41551#issuecomment-5168190946
# Round 3 — verification of the seven-commit batch, re-pinned to `136d0fc826`
Reviewed `91706dff80..9197772542` with the same two reviewers, then
re-verified every finding at `136d0fc826` after you pushed the four-lens commit
mid-pass. Everything below is re-checked at that head.
**Disclosure:** `136d0fc826` itself (the
popup-block/in-flight-lock/URL-param/missing-uuid batch) landed after my
reviewers' pass and is **not** covered by this round. Both findings below
survive it unchanged.
**All three round-2 findings are properly fixed, the unflagged surface is
clean, and I could not find a third regression in most of the batch — but there
is one, in the restore-hydration guard, plus a new defect in the theme
adapter.**
---
## Confirmed fixed
**Round-2 finding 1 — the over-cancellation.** The guard is now split: a
cleanup effect keyed on `[sliceId, uuid]` bumps `restoreHydrationIdRef`, and a
per-fetch token is captured at the fetch. `refreshActivity` remains in the main
effect's deps but no longer cancels anything, which is exactly the distinction
that broke the previous attempt. Revert-proved: restored to the round-2 shape
the new test **fails** (`Number of calls: 0`), restored again 6/6 pass.
**Round-2 finding 2 — the Edit Dataset dirty gate.** Recording
`UPDATE_FORM_DATA_BY_DATASOURCE` rather than special-casing the derived write
is the better fix. Both routes now go dirty; on the swap route
`ChangeDatasourceModal` invokes `onDatasourceSave` before `onChange`, so the
two entries are consecutive with the same `controlName` and the reducer
genuinely collapses them. A full sweep found no automated producer of that
action type.
**Round-2 finding 3 — `extra_editors`.** Fixed, and splitting the unflagged
widening into #42708 is the right call. I diffed the two: functionally
identical on the three shared files, differing only in comment wording. One
note — the branch commit carries a **fourth** file (`canOverwriteSlice.ts`)
that #42708 doesn't, and the comment wording differs on the shared files, so
`2a0dae0e8e` will **conflict** rather than rebase away cleanly when the split
PR merges.
**The unflagged surface is clean, and I checked it rather than trusting the
"refactor" label.** `canOverwriteSlice`'s new form agrees with the old for
every input: the slice/`is_managed_externally` guards fire first,
`canOverwrite` and admin short-circuit in the same order, both collections
treat null/undefined as empty, and the `number | {id}` normaliser is unchanged.
`isUserDashboardEditor`'s widening is strict — all prior prerequisites
retained, membership only widened. Neither can narrow for any supported input.
**Both newly-recorded action types are safe** — the round-2 trap didn't
recur with the polarity flipped. `SET_EXPLORE_CONTROLS` (whose value is
confusingly `'UPDATE_EXPLORE_CONTROLS'`, now pinned by test) has one
dispatcher, gated on `action === 'POP'` plus two further conditions. Neither
fires on hydration, save-as, or a programmatic rebuild.
Also verified: the theme-lookup race fix (`266f6764a9`) — no await now
remains between the cache commit and `hydrateWith`, and I walked every
statement in that stretch; `AnnotationLayerControl:114-121` marked; the three
snapshot-nullability guards each address a real path. Suites 407 green across
three commands, nothing previously green regressed.
---
## 1. The new restore token no longer invalidates on a same-chart save —
Medium
This one my two reviewers disagreed on, so I settled it against the round-2
source directly.
`20627e44e1` correctly stops search/include churn from cancelling the
post-restore GET, but it narrows invalidation too far. The token is bumped
**only** by the cleanup on `[sliceId, uuid]` and when another restore begins.
The same-chart save branch calls `refreshActivity()` and never bumps it.
The round-2 implementation *did* invalidate on save — not by design, but
because `saveSignal` was in the effect's dependency array and the cleanup ran
on every re-run. So the fix removed a valid boundary along with the invalid
one. I confirmed this by reading the round-2 file at `91706dff80`: `saveSignal`
is in the deps, `return () => { cancelled = true; }` is the cleanup.
**Ordering:**
1. Restore succeeds and starts `fetchExploreRehydrationData(sliceId)`.
2. `confirmRestore` clears the preview, so Save is no longer held disabled.
3. The user edits and overwrites the same chart; the save rehydrates newer
server state and moves `saveSignal`.
4. The older GET — whose server read predates the overwrite — resolves. Its
token is still current, so `hydrateExplore` replaces the just-saved UI with the
stale restored data.
The server then holds the new save while the page shows the older restore
until reload, and a further save can persist edits made from that stale base.
The window needs the rehydration GET still in flight when the user saves, so
it's tight on a fast network and realistic on a slow one — I'd rate it Medium
on impact, lower on likelihood.
Capturing a save generation at fetch start and rejecting on mismatch, or
bumping `restoreHydrationIdRef` on a non-initial save signal, preserves the
boundary you had while still excluding search/include identity churn. There's
no save-mid-fetch test; the new test covers search and the older one covers
unmount.
## 2. `fetchDashboardTheme` overwrites the theme id it just parsed — Medium
```ts
const { id, result } = json as { id: number; result: Omit<DashboardTheme,
'id'> };
return { ...result, id };
```
`result.id` is definitely present and correct — `show_columns` includes
`"id"` (`superset/themes/api.py:95-109`) and the integration test asserts
`data["result"]["id"] == theme.id`. The adapter spreads that correct value and
then overwrites it with a top-level `id`.
I could **not** confirm from source whether FAB's `show` envelope carries a
top-level `id` — the package isn't installed in my checkout, and the only
superset test reading `data["id"]` is on a **POST**, where FAB certainly does
return one. So I'll state the two cases rather than assert one:
- if `show` has no top-level `id`, the theme is hydrated with `id:
undefined`;
- if it has one (FAB's documented show envelope suggests it does, typically
stringified), the numeric `result.id` is replaced by a string.
Either way the spread is wrong and the fix is the same: return `json.result`
as the complete `DashboardTheme`. `Header/index.tsx:290-298` reads that id in
production. Worth confirming against your pinned flask-appbuilder rather than
taking my reading — I flagged the same version caveat on #41550.
The tests can't catch this: `useDashboardVersionPreview.test.tsx` mocks
`fetchDashboardTheme` itself and returns an already-correct object, and
`api.test.ts` doesn't cover the adapter. An adapter-level fetch mock returning
`{ result: { id, theme_name, json_data } }` would.
---
## Two scope claims are still overstated
- **"Every snapshot scalar."** `is_managed_externally` and `external_url`
are in the dashboard shadow table and projected by the snapshot endpoint, but
are absent from both the snapshot interface and the hydrate overlay — while
restore, going through Continuum's `revert`, *does* restore them. Also
`published: null` previews as `false`, and a failed theme lookup shows the live
theme. Preview and restore therefore still don't agree on every scalar.
Excluding historical external-management state from permission gates may well
be deliberate — if so the claim, tests and docs should say so rather than
assert completeness.
- **"The whole wire shape."** Chart snapshots still lack typed `id`,
`datasource_name`, `certified_by`, `certification_details`,
`is_managed_externally`, `external_url`; dashboard snapshots lack `id`,
`is_managed_externally`, `external_url`. And `query_context` is still typed
required although `Slice.__versioned__` excludes it and `slices_version` has no
such column — so the server omits the key rather than sending null. The index
signature hides all of these. Related: the docs now say Open as new copies
chart query context, but `createChartFromSnapshot` can only conditionally
include a property the endpoint never supplies.
---
Worth saying plainly: three of the four items you self-found this round were
real, and finding the race your own scalars commit introduced — before I did —
is the thing that most improves my confidence in this branch. Naming the
pattern and proposing baseline diffing and `listenerMiddleware` as flag-on
gates is the right conclusion to draw from three batches of writer-enumeration
and ref-coordination bugs; both are what I'd have proposed. Finding 1 above is
arguably the fourth instance of exactly that class, which I'd treat as further
evidence for the second of those two rather than as an argument for another
point patch.
--
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]
