mikebridge commented on PR #41551: URL: https://github.com/apache/superset/pull/41551#issuecomment-5144564928
Thank you for this — the verification-focused framing caught things four defect sweeps missed, and the "Checked and clean" section saves everyone the re-litigation. Response batch is pushed as `b45c7b281d..3b16cc409e`; item by item: **1. The re-selection wedge — fixed in `b45c7b281d`, at the reducer as you suggested**, so both routes (the effect that never re-runs, and the dashboard hook's `appliedVersionRef` early-return) close at once. Your regression test is in as `previewReselect.test.tsx` with the assertions flipped to pin the fixed behaviour — it fails against the pre-fix reducer, and the attribution is in the file header. Agreed this was the one to fix before the flag turns on anywhere. **3. The mirror-pattern items:** - *Chart unmount guard* — this one was fixed between your review sha and now: `308f5cb859` (from the codeant round) cancels the restore rehydration on cleanup, with a regression test that unmounts mid-fetch. Your reading of `075241a2f7` was correct at that sha. - *Session-log dirty signal, false-negative half* — **fixed in `b9a06a1a9b`**: renames now reach the log via `UPDATE_CHART_TITLE`, constant pinned against the explore module like its siblings. The **false-positive half** (append-only log never diffed against originals, and Explore has no discard affordance) is real and stays open — an honest fix needs baseline diffing, which is more design than a review response should smuggle in. Tracked for the post-merge batch; the failure mode is over-blocking, not data loss. - *`Control.tsx` reset-on-hide* — marked programmatic in the same commit. Your point about the mark being opt-out (and plugin writes unable to opt in through `ExploreViewContainer:994`) is taken — noted on the follow-up list as a design smell rather than patched piecemeal here. - *Empty-tab edit link* — gated in `3b16cc409e`, same shape as the DashboardBuilder fix. - *Dashboard `extra_editors`* — agreed and already dispositioned with codeant earlier today: the payload doesn't carry the field, and `canUserEditDashboard` has the identical gap on the Edit button platform-wide, so the fix is backend-first (emit the field, widen the shared predicate, and Edit/menu/restore all correct together). On the backend follow-up list. **4. Smaller items:** the `?edit=true` exit and post-restore paths now pass the explicit `editMode` override (`3b16cc409e`) — entry, exit, and restore-reload are symmetric. The `loadMore`-vs-probe race got its own staleness counter, and `ActivityOperation` gained `'update'`. Of the wire-type drifts, the other two (`path` redaction to null, `query_context`) are noted with the type-hygiene follow-ups; the timeline memoization and i18n items were already on the post-merge list and your added detail is folded into it. The `include`-persists-across-entities behaviour is indeed pinned by a test — flagging it as deliberate-but-debatable; happy to reset it on close if you'd rather. **2. Preview fidelity** — this is the finding I think matters most long-term, and it isn't fully fixable frontend-side: the membership half needs the snapshot endpoint to return the validity-windowed membership set. For now `3b16cc409e` makes the docs honest about the approximation (the page previously claimed more than the code delivers, as you noted). The scalar half (description/theme/certification/published/slug in the snapshot type and the preview override, and the same fields on "Open as new") is queued behind that same snapshot-shape change so it lands as one coherent contract fix rather than field-by-field. **6. The nested-store architecture note** — this is the most useful paragraph anyone has written about this feature. You're right that the dashboard half's entire mechanism inventory (the cache, the invalidation effects, the save-signal listener, the dataMask replay, the gates-as-safety-critical) exists only because the live store holds historical data, and that it doesn't extend to a third entity kind. Recorded verbatim in the planning notes as the design direction to evaluate *before* dataset history rather than after. Not attempted here for the reason you gave — the feature works and is contained. **5. Flag-off visuals** — fair; a flag-off screenshot pass is on the list before the flag graduates, alongside your two specific always-on surfaces. The `exploreReducer.ts` editors/`.value` normalization bug is filed on our follow-up list as its own item with your analysis attached — agreed it predates this PR. Head with all of the above: `3b16cc409e`. _Response drafted and fixes implemented by Claude (AI) on behalf of @mikebridge._ -- 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]
