mikebridge commented on PR #41551: URL: https://github.com/apache/superset/pull/41551#issuecomment-5166902368
# Round 2 response — pushed, head now `9197772542` *Generated by Claude (AI) on behalf of @mikebridge.* Thanks for the pass — all three findings were right, including the refutation you overturned. Seven commits, disposition below. The honest headline first: **while verifying this batch I found and fixed a regression in my own fix for one of your findings.** That's the second consecutive response batch to introduce a defect in code written to close the previous review, which I'm treating as a signal about the code's shape, not just two mistakes — more under "structural" below. ## Your findings **1. Rehydration over-cancel → `20627e44e1`.** Exactly your prescription: a `restoreHydrationIdRef` bound to the fetch, bumped only by unmount, a slice/uuid swap, or a newer restore — the same pattern as the probe counter. The regression test types into the search box mid-rehydration; verified load-bearing (fails against the reverted source, hydrate never called). **2. Edit Dataset dirty gate → `f741ee6a6b`.** Took your framing rather than a point patch: the middleware now records `UPDATE_FORM_DATA_BY_DATASOURCE` — the primary reconciliation both routes dispatch — instead of special-casing the derived write, which stays programmatic. On the swap route the two entries collapse (same `controlName`), so no double-count. The new test asserts the dirty behaviour, not the argument shape. Your `AnnotationLayerControl` catch is fixed in the same commit. **3. `extra_editors` — you were right, and it's now its own PR.** The refutation was wrong for the reason you gave: the field is attached after `schema.dump()`, so the projection never strips it. Since the widening is *not* behind `VERSION_HISTORY` — it changes the Edit-button gate for any deployment with a resolver — I've split it out rather than ship an unconditional permission change inside a flagged feature PR (round 1's lesson): **#42708**. This branch keeps an identical commit (`2a0dae0e8e`) so it works standalone; it rebases away when the split PR merges. `canOverwriteSlice` now reuses the shared helper instead of its inline copy. ## Your scope notes - **Preview scalars → `34764a2dc1`.** Description, slug, certification, published and theme now applied. Theme was the one needing work — the table stores `theme_id`, hydration wants the object — so there's a single lookup, skipped when it matches the live theme, falling back to the live theme rather than dropping the preview on failure. - **Wire types → `6d42538e41`.** Modelled the whole nullable shape for both snapshot types rather than the two named examples. That surfaced three live paths: the chart preview would request `undefined__undefined` for a null-viz version, `createChartFromSnapshot` would post a payload that fails server validation, and "open as new" would title a copy `null (copy from …)`. Each now fails or falls back with something actionable. - **Docs → same commit.** "Open as new" now states what each path actually copies (chart: built from the version; dashboard: a copy of the *live* dashboard with only title/CSS/metadata/layout applied — `DashboardCopySchema` accepts nothing else). The `?edit=true` comment now states the real invariant (the gate tests `hasUnsavedChanges`, not `editMode`). ## Found while verifying — two more, both fixed **4. My scalars commit introduced a race → `266f6764a9`.** The theme lookup awaited between committing `liveDataRef` and setting `appliedVersionRef` — the stretch that must stay synchronous, because the save-signal effect nulls the cache whenever no preview is applied yet. A save landing inside that await left a preview applied over a null cache; exit-preview only rehydrates `if (liveData)`, so closing it did nothing and the page kept presenting the snapshot as live until reload. Fixed by resolving the theme with the other pre-check awaits; regression test fails against the previous placement. **5. Another unrecorded session-log writer, same shape as your #2 → `9197772542`.** A sweep of the explore reducer against the middleware's recorded types turned up `SET_EXPLORE_CONTROLS`: browser back/forward through explore's own history entries rebuilds the entire control map with no control change emitted. Sequence: edit → save (clears the log) → Back to pre-save controls → restore discards the popped state silently. Now recorded under a `__history__` sentinel. The stated trade: stepping back to a state matching the saved chart now latches an entry and blocks restore where nothing differs — fail-closed where the gap failed open, and one more argument for the item below. ## Structural — proposed as the flag-on gate, not a merge gate Your round-1 verdict was "nothing blocks merging dark," and nothing since changes that. But three response batches have now each patched a writer-enumeration or ref-coordination bug, and I'd rather name the two fixes that end those classes than keep patching instances: 1. **Replace the session log with baseline diffing.** The codebase already has the mechanism: `getChartFormDiffs` (`src/utils/getChartFormDiffs/`), which the header's own unsaved-changes prompt uses (`ExploreChartHeader:271`). Diffing current vs. loaded form data can't miss a writer (fixes #2/#5's whole class) and can't latch phantom entries (rename A→B→A, history steps back to saved state), and it returns before/after values the log never had. Caveat to resolve before adopting: its `noisyKeys` exclusions and empty≈null semantics were tuned for a navigation prompt, where a false negative costs a dialog — for a restore gate it costs data, so the exclusion list needs re-examination, not wholesale reuse. 2. **Move the preview orchestration to `listenerMiddleware`** (already configured in `src/views/store.ts`). `useDashboardVersionPreview` now carries seven coordination refs, and commit `266f6764a9` is enforced by nothing but a comment saying "this stretch must stay synchronous." takeLatest-style cancellation makes that class unrepresentable. I'd do this before dataset history adds a third entity kind to the surface. Also acknowledged but deliberately not in this batch: the `programmatic` opt-out default can't be type-enforced through the control bridge (`HandlerFunction = (...args: unknown[]) => void` swallows the options argument — same reason your cartodiagram site is unmarkable). Inverting the default or typing the creator touches the shared chart-types package; separate PR. Suites green (238 across the touched areas), tsc clean, pre-commit clean at `9197772542`. -- 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]
