mikebridge commented on PR #43633:
URL: https://github.com/apache/superset/pull/43633#issuecomment-5455522001
Ran a five-lens review at `25d6621`. It independently confirms
@aminghadersohi's three threads and both codeant threads, and adds two things
worth doing before merge — no blocker, and the fix for the main one is a single
line.
**High — the stale `datasource` closure is a real data-loss race,
reproduced.** The certification `onChange` at `DatasourceEditor.tsx:1645-1652`
does `onDatasourceChange({ ...datasource, extra:
setDatasetCertification(datasource.extra, updated) })` with the `datasource`
from the keystroke's render; `TextControl` commits after `FAST_DEBOUNCE`, so
any sibling edit that lands in that window is overwritten. The `itemRef` guard
from #43255 only protects `item`, which here is the derived certification
object, so this is that bug class one level up. Control run at PR head,
editor-level, no sleeps: Certified by → Details passes; Certified by → Default
URL passes; **Default URL → Certified by fails — `default_endpoint` is
`undefined` in the final `onChange`.** Replacing the handler body with
`setDatasource(prev => ({ ...prev, extra: setDatasetCertification(prev.extra,
updated) }))` makes it pass with the others green. An updater-form sibling of
`onDatasourcePropChange` would cove
r this and the same latent shape in the `EditorsSelector` spread further down.
**High (please verify by hand) — the raw Extra editor is uncontrolled, so
editing it after filling the fields wipes the certification.**
`TextAreaControl` passes `defaultValue` and never `value`; react-ace applies
`defaultValue` only on mount, and `key={name}` is `undefined` here because
`Field` doesn't forward `name`. So after the fields write `extra`, the Extra
editor still shows the old JSON; the moment the user touches it (say, to add
`warning_markdown`) it commits that stale text through the Basic fieldset,
certification drops out of state, and both fields visibly reset to empty. One
lens traced this and nobody ran it, hence the ask. A `key` counter bumped only
inside the certification `onChange` (not on Extra's own edits, or the cursor
resets) is the minimal fix; the metric pattern — `hydrateMetricExtra` on load,
`buildExtraJsonObject` at save — is the structural one.
**Medium**
- Malformed Extra: `getDatasetCertification` returns `{}` (fields render
editable) while `setDatasetCertification` returns the string untouched — so
typed values look accepted and are silently dropped on Save. The
not-overwriting is right; surface it: render the two `Field`s `disabled` with
`errorMessage={t('Fix the Extra JSON to edit certification')}`, or push it into
`errors` so Save is blocked.
- The 500 ms sleeps in `DatasourceModal.test.tsx` serialise the edits, so
the test only exercises the one ordering that can't race. The sibling
`DatasourceEditorMetricCertification.test.tsx` already shows the shape —
`userEvent.type` both fields back-to-back, assert inside `waitFor` — and its
header comment describes exactly this bug. Written that way, the dataset test
becomes the red test for the fix above.
- Helper tests: the `...existingCertification` spread at `:78` could be
deleted with nothing failing; either add the case (unknown key inside
`certification` survives) or drop it. Also untested: non-object
`extra`/`certification`, the UI's real clear input (`''`/`''` rather than
`{}`), details-only writes.
- Agree with Amin: `feat(dataset):`, and "Introduces new feature" ticked.
Screenshots for the "Changes UI" box would help too.
- CI: `sharded-jest-tests (3)` is the `SqlEditor.test.tsx` "not a valid
selector" flake — same failure on #43619 and other unrelated runs; the rest of
the rollup's reds are the superseded run. Needs a green re-run, nothing to fix.
**Small**
- The clear path deletes the whole `certification` object, taking unknown
sub-keys the write path preserves (Amin's point) — pick one behaviour and pin
it.
- `JSON.stringify(parsedExtra, null, 2)` reflows the user's Extra on every
keystroke while the editor sits in the same column; on a dataset with no
`extra`, type-then-clear persists `"{}"`.
- `Record<string, unknown> &` on `DatasetCertification` isn't needed for
`Fieldset` and lets a misspelled key through.
- Certification renders above Basic; placing it next to the Extra textarea
it derives from makes the relationship visible, and the Extra help text still
says to hand-write the certification JSON.
- The `extra.certification` contract was checked against
`CertificationMixin`, `DatasetCertifiedFilter`, the list badge and
import/export — compatible, no UPDATING.md needed.
Nice: `parseExtra` keeping absent (`{}`) apart from malformed (`undefined`)
is the right call; the modal test asserting the PUT payload rather than
component state pins the real contract; reusing the metric field labels and
i18n strings is exactly the confusion this resolves.
_Review produced by five AI lenses (react, preset, committer, clean-code,
tidy-first), one of which ran the closure-race control in a scratch worktree;
posted by @mikebridge after reading. Full report in the spec repo._
--
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]
