kokhlo commented on PR #44982:
URL: https://github.com/apache/superset/pull/44982#issuecomment-5992457838
Thanks for the review — taking the first two, pushing a follow-up commit.
**Duplicate option values** — fixed: `getRefreshFrequencyOptions` now tracks
collected intervals in a `Set` and skips an entry whose seconds were already
seen, so a deployment repeating `[60, "1 minute"]` cannot produce duplicate
React keys or two simultaneously-checked radios. The first occurrence wins, and
`'600'` (string spelling the same interval) is treated as the same value rather
than a new one. Covered by `keeps the first of two entries sharing an interval`.
**Conflicting `presets` naming** — fixed: the local inside
`getRefreshFrequencyOptions` is now `baseOptions`; `presets` remains the
component-level name for the list with `Custom` filtered out, which is the only
place `isPresetValue` consumes.
On the coercion point, the loose conversion was a real hole and the
follow-up closes it: `Number(null)`, `Number('')` and `Number([])` are all `0`,
and `0` is the legitimate "Don't refresh" interval, so those values used to
pass validation as a real option. The value is now only converted when it is a
`number` or a non-blank `string`; anything else becomes `NaN` and the entry is
dropped. Pinned by `rejects values that coerce to a real interval`.
**Docstrings on new tests** — not taking this one. `BITO.md` is not in the
repository, and no test function in this tree carries a docstring (checked
`RefreshIntervalModal.test.tsx`, `Header/HeadlessAutoRefresh.test.tsx` and the
sibling suites under `src/dashboard/components`): the file-level ASF header is
the convention. Adding per-test prose only to this PR would make it
inconsistent with everything around it. Happy to add them if the maintainers
actually want that convention introduced.
--
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]
