You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "FrancescoCastaldi (via GitHub)" <gi...@apache.org> on 2026/10/06 22:35:20 UTC
[PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
FrancescoCastaldi opened a new pull request, #45039:
URL: https://github.com/apache/superset/pull/45039
### SUMMARY
Fixes #44981.
The dashboard auto-refresh frequency dropdown rendered hardcoded preset options and ignored the DASHBOARD_AUTO_REFRESH_INTERVALS configuration defined in Superset configuration (common.conf).
This change:
1. Connects RefreshFrequencySelect.tsx to the Redux store selector (state.dashboardInfo.common.conf.DASHBOARD_AUTO_REFRESH_INTERVALS).
2. Generates options dynamically from the configuration when present, while retaining transparent fallback to default intervals when unconfigured.
3. Memoizes options and properly handles custom interval inputs and radio selection states.
### TESTING INSTRUCTIONS
Run the Jest unit tests:
npm test -- RefreshFrequencySelect.test.tsx
All tests pass, confirming options rendering from custom store configurations and fallback behavior.
### ADDITIONAL INFORMATION
- [x] Has associated issue: fixes #44981
- [x] Changes UI: Yes (renders configured auto-refresh options)
- [x] Requires DB Migration: No
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on PR #45039:
URL: https://github.com/apache/superset/pull/45039#issuecomment-6133198447
Rebased onto latest `master` (`44f8e861a5`) to resolve the merge conflict and clear `requires:rebase` @sadpandajoe:
1. Preserved all `CUSTOM_REFRESH_FREQUENCY` and `getRefreshFrequencyOptions` logic from `master` while adding `options?: [number, string][]` prop override support and exporting `isPresetValue` and `getCustomValue`.
2. Preserved custom interval draft values across preset switches while clamping drafts below `MINIMUM_REFRESH_INTERVAL` (preventing negative drafts such as `-5` from being re-emitted).
3. Verified `oxfmt` and `oxlint` pass with 0 warnings and 0 errors.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45039:
URL: https://github.com/apache/superset/pull/45039#issuecomment-6026742499
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 195d8a98ca535ec2a09d00e12b080f990395a6eb |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ac577a85a6966000733a648 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-45039--superset-docs-preview.netlify.app](https://deploy-preview-45039--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">π€</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctRDoMgDADQu_R7ZBix4G6wY1SpgUSFtLhsWXb38fd-3heqlKM2eADcYBE619S95fc9q15snJvDYCJpWgpJNHS1YoQ3YU0mn43lRbv2G7nu5fOMfSOtk_cUJsIZ0Vrrx5HQBfj9AQ) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-45039--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45039:
URL: https://github.com/apache/superset/pull/45039#discussion_r4217271760
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -120,11 +220,13 @@ export const RefreshFrequencySelect = ({
if (selectedValue === -1) {
// Custom selected - use current custom value or minimum
const numValue = parseInt(customValue, 10) || MINIMUM_REFRESH_INTERVAL;
Review Comment:
If someone types an invalid draft such as `-5` into Custom, picks a preset, and then picks Custom again, this line re-emits `-5`: `parseInt('-5', 10)` is truthy so the `|| MINIMUM_REFRESH_INTERVAL` fallback never applies. The typing handler rejects values below the minimum, but this path doesn't, and `validateRefreshFrequency` only checks positive frequencies, so Save accepts a negative interval and auto-refresh quietly stops while Custom stays selected. Before this change the draft was cleared when a preset was chosen, so the path wasn't reachable.
Could this clamp the value (e.g. only reuse the draft when it parses to at least `MINIMUM_REFRESH_INTERVAL`), and could a controlled test cover the type `-5` β preset β Custom sequence?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on code in PR #45039:
URL: https://github.com/apache/superset/pull/45039#discussion_r4252607835
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -87,31 +120,81 @@ const normalizeRefreshLimitSeconds = (
return refreshLimit;
};
-interface RefreshFrequencySelectProps {
+/**
+ * Props for the RefreshFrequencySelect component.
+ */
+export interface RefreshFrequencySelectProps {
+ /** The currently selected refresh frequency in seconds. */
value: number;
+ /** Callback fired when a new refresh frequency is selected or typed. */
onChange: (value: number) => void;
+ /** Optional override for available interval options as [seconds, label] tuples. */
+ options?: [number, string][];
}
/**
- * Shared refresh frequency select component
- * Used in both PropertiesModal and RefreshIntervalModal
+ * Shared refresh frequency select component.
+ *
+ * Renders radio buttons for available auto refresh frequencies.
+ * Reads configured intervals dynamically from Redux store
+ * (state.dashboardInfo.common.conf.DASHBOARD_AUTO_REFRESH_INTERVALS)
+ * with a fallback to REFRESH_FREQUENCY_OPTIONS if unconfigured.
+ * Also supports direct options prop override and custom numeric interval entry.
*/
export const RefreshFrequencySelect = ({
value,
onChange,
+ options: optionsProp,
}: RefreshFrequencySelectProps) => {
+ const configuredIntervals = useSelector(
+ (state: RootState) =>
+ state.dashboardInfo?.common?.conf?.DASHBOARD_AUTO_REFRESH_INTERVALS,
Review Comment:
Addressed and rebased onto `master` in commit `44f8e861a5` @sadpandajoe. The selector reads `DASHBOARD_AUTO_REFRESH_INTERVALS` from both `state.dashboardInfo.common.conf` and the bootstrapped top-level `state.common.conf` slice, with regression coverage for the list/home page store shape.
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -87,31 +120,81 @@ const normalizeRefreshLimitSeconds = (
return refreshLimit;
};
-interface RefreshFrequencySelectProps {
+/**
+ * Props for the RefreshFrequencySelect component.
+ */
+export interface RefreshFrequencySelectProps {
+ /** The currently selected refresh frequency in seconds. */
value: number;
+ /** Callback fired when a new refresh frequency is selected or typed. */
onChange: (value: number) => void;
+ /** Optional override for available interval options as [seconds, label] tuples. */
+ options?: [number, string][];
}
/**
- * Shared refresh frequency select component
- * Used in both PropertiesModal and RefreshIntervalModal
+ * Shared refresh frequency select component.
+ *
+ * Renders radio buttons for available auto refresh frequencies.
+ * Reads configured intervals dynamically from Redux store
+ * (state.dashboardInfo.common.conf.DASHBOARD_AUTO_REFRESH_INTERVALS)
+ * with a fallback to REFRESH_FREQUENCY_OPTIONS if unconfigured.
+ * Also supports direct options prop override and custom numeric interval entry.
*/
export const RefreshFrequencySelect = ({
value,
onChange,
+ options: optionsProp,
}: RefreshFrequencySelectProps) => {
+ const configuredIntervals = useSelector(
+ (state: RootState) =>
+ state.dashboardInfo?.common?.conf?.DASHBOARD_AUTO_REFRESH_INTERVALS,
+ );
+
+ const activeOptions = useMemo(() => {
+ const rawOptions = optionsProp ?? configuredIntervals;
+ if (Array.isArray(rawOptions) && rawOptions.length > 0) {
+ const validOptions = rawOptions
+ .filter(
+ (item) =>
+ Array.isArray(item) &&
+ typeof item[0] === 'number' &&
+ !Number.isNaN(item[0]) &&
+ typeof item[1] === 'string',
+ )
+ .map(([interval, label]) => ({
+ value: interval,
+ label: t(label),
+ }));
+ if (validOptions.length > 0) {
+ return validOptions;
+ }
+ }
+ return REFRESH_FREQUENCY_OPTIONS.slice(0, -1);
+ }, [optionsProp, configuredIntervals]);
+
+ const isPreset = useCallback(
+ (frequency: number) => isPresetValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
+ const getCustom = useCallback(
+ (frequency: number) => getCustomValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
// Separate radio selection state from value state
const [radioSelection, setRadioSelection] = useState(() =>
- isPresetValue(value) ? value : -1,
+ isPreset(value) ? value : -1,
);
- const [customValue, setCustomValue] = useState(() => getCustomValue(value));
+ const [customValue, setCustomValue] = useState(() => getCustom(value));
useEffect(() => {
- const selection = isPresetValue(value) ? value : -1;
+ const selection = isPreset(value) ? value : -1;
setRadioSelection(selection);
- setCustomValue(selection === -1 ? getCustomValue(value) : '');
- }, [value]);
+ setCustomValue(selection === -1 ? getCustom(value) : '');
Review Comment:
Addressed and rebased onto `master` in commit `44f8e861a5` @sadpandajoe. `lastCustomEmittedValue` tracks values emitted from the Custom controls so controlled parents echoing `1` back do not snap selection to a colliding `1 second` preset, and custom drafts are preserved across preset switches while clamping values below `MINIMUM_REFRESH_INTERVAL`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45039:
URL: https://github.com/apache/superset/pull/45039#discussion_r4252611238
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.test.tsx:
##########
@@ -38,3 +45,210 @@ test('getRefreshWarningMessage normalizes refreshLimit', () => {
expect(getRefreshWarningMessage(5, 10, 'warn')).toBe('warn');
expect(getRefreshWarningMessage(15, 10000, 'warn')).toBeNull();
});
+
+const mockConfiguredIntervals: [number, string][] = [
+ [0, "Don't refresh"],
+ [15, '15 seconds'],
+ [45, '45 seconds'],
+ [90, '90 seconds'],
+];
+
+const createInitialState = (
+ intervals?: [number, string][],
+ extraConf?: Record<string, unknown>,
+) => ({
+ dashboardInfo: {
+ common: {
+ conf: {
+ ...(intervals !== undefined
+ ? { DASHBOARD_AUTO_REFRESH_INTERVALS: intervals }
+ : {}),
+ ...extraConf,
+ },
+ },
+ },
+});
+
+const defaultTestProps = {
+ value: 0,
+ onChange: jest.fn(),
+};
+
+const setup = (
+ props: Partial<typeof defaultTestProps> = {},
+ initialState?: object,
+) =>
+ render(<RefreshFrequencySelect {...defaultTestProps} {...props} />, {
+ useRedux: true,
+ ...(initialState ? { initialState } : {}),
+ });
+
+test('renders configured auto refresh intervals from redux store', () => {
+ setup({}, createInitialState(mockConfiguredIntervals));
+
+ expect(screen.getByRole('radio', { name: '15 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '45 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '90 seconds' })).toBeInTheDocument();
+ expect(
+ screen.queryByRole('radio', { name: '10 seconds' }),
+ ).not.toBeInTheDocument();
+});
+
+test('renders default options when config is not present', () => {
+ setup({}, createInitialState(undefined));
+
+ expect(
+ screen.getByRole('radio', { name: "Don't refresh" }),
+ ).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '10 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '30 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '1 minute' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '24 hours' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: /Custom/i })).toBeInTheDocument();
+});
+
+test('renders default options when configured intervals is empty array', () => {
+ setup({}, createInitialState([]));
+
+ expect(screen.getByRole('radio', { name: '10 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '30 seconds' })).toBeInTheDocument();
+});
+
+test('options prop takes precedence over redux store configured intervals', () => {
+ const propOptions: [number, string][] = [
+ [5, '5 seconds'],
+ [25, '25 seconds'],
+ ];
+
+ setup({ options: propOptions }, createInitialState(mockConfiguredIntervals));
Review Comment:
β
**CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `44f8e86`.
The test now directly checks the options returned by `getRefreshFrequencyOptions`; the `setup` call with an unsupported `options` property is gone.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -87,31 +120,81 @@ const normalizeRefreshLimitSeconds = (
return refreshLimit;
};
-interface RefreshFrequencySelectProps {
+/**
+ * Props for the RefreshFrequencySelect component.
+ */
+export interface RefreshFrequencySelectProps {
+ /** The currently selected refresh frequency in seconds. */
value: number;
+ /** Callback fired when a new refresh frequency is selected or typed. */
onChange: (value: number) => void;
+ /** Optional override for available interval options as [seconds, label] tuples. */
+ options?: [number, string][];
}
/**
- * Shared refresh frequency select component
- * Used in both PropertiesModal and RefreshIntervalModal
+ * Shared refresh frequency select component.
+ *
+ * Renders radio buttons for available auto refresh frequencies.
+ * Reads configured intervals dynamically from Redux store
+ * (state.dashboardInfo.common.conf.DASHBOARD_AUTO_REFRESH_INTERVALS)
+ * with a fallback to REFRESH_FREQUENCY_OPTIONS if unconfigured.
+ * Also supports direct options prop override and custom numeric interval entry.
*/
export const RefreshFrequencySelect = ({
value,
onChange,
+ options: optionsProp,
}: RefreshFrequencySelectProps) => {
+ const configuredIntervals = useSelector(
+ (state: RootState) =>
+ state.dashboardInfo?.common?.conf?.DASHBOARD_AUTO_REFRESH_INTERVALS,
+ );
+
+ const activeOptions = useMemo(() => {
+ const rawOptions = optionsProp ?? configuredIntervals;
+ if (Array.isArray(rawOptions) && rawOptions.length > 0) {
+ const validOptions = rawOptions
+ .filter(
+ (item) =>
+ Array.isArray(item) &&
+ typeof item[0] === 'number' &&
+ !Number.isNaN(item[0]) &&
+ typeof item[1] === 'string',
+ )
+ .map(([interval, label]) => ({
+ value: interval,
+ label: t(label),
+ }));
+ if (validOptions.length > 0) {
+ return validOptions;
+ }
+ }
+ return REFRESH_FREQUENCY_OPTIONS.slice(0, -1);
+ }, [optionsProp, configuredIntervals]);
+
+ const isPreset = useCallback(
+ (frequency: number) => isPresetValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
+ const getCustom = useCallback(
+ (frequency: number) => getCustomValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
// Separate radio selection state from value state
const [radioSelection, setRadioSelection] = useState(() =>
- isPresetValue(value) ? value : -1,
+ isPreset(value) ? value : -1,
);
- const [customValue, setCustomValue] = useState(() => getCustomValue(value));
+ const [customValue, setCustomValue] = useState(() => getCustom(value));
useEffect(() => {
- const selection = isPresetValue(value) ? value : -1;
+ const selection = isPreset(value) ? value : -1;
setRadioSelection(selection);
- setCustomValue(selection === -1 ? getCustomValue(value) : '');
- }, [value]);
+ setCustomValue(selection === -1 ? getCustom(value) : '');
Review Comment:
β
**CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `44f8e86`.
The effect updates `customValue` only when Custom is selected, so selecting a preset no longer clears the previously entered custom interval.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45039:
URL: https://github.com/apache/superset/pull/45039#issuecomment-6026841046
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45039?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `90.62500%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.48%. Comparing base ([`a5ffac5`](https://app.codecov.io/gh/apache/superset/commit/a5ffac58f44527607a7937c01b4b21eb989ebf49?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`195d8a9`](https://app.codecov.io/gh/apache/superset/commit/195d8a98ca535ec2a09d00e12b080f990395a6eb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 10 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/45039?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...onents/RefreshFrequency/RefreshFrequencySelect.tsx](https://app.codecov.io/gh/apache/superset/pull/45039?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fdashboard%2Fcomponents%2FRefreshFrequency%2FRefreshFrequencySelect.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1JlZnJlc2hGcmVxdWVuY3kvUmVmcmVzaEZyZXF1ZW5jeVNlbGVjdC50c3g=) | 90.62% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45039?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45039 +/- ##
=======================================
Coverage 82.47% 82.48%
=======================================
Files 3002 3002
Lines 186172 186193 +21
Branches 43128 43139 +11
=======================================
+ Hits 153550 153578 +28
+ Misses 29867 29860 -7
Partials 2755 2755
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45039/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/45039/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.22% <90.62%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45039?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45039:
URL: https://github.com/apache/superset/pull/45039#discussion_r4217328096
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -87,31 +120,81 @@ const normalizeRefreshLimitSeconds = (
return refreshLimit;
};
-interface RefreshFrequencySelectProps {
+/**
+ * Props for the RefreshFrequencySelect component.
+ */
+export interface RefreshFrequencySelectProps {
+ /** The currently selected refresh frequency in seconds. */
value: number;
+ /** Callback fired when a new refresh frequency is selected or typed. */
onChange: (value: number) => void;
+ /** Optional override for available interval options as [seconds, label] tuples. */
+ options?: [number, string][];
}
/**
- * Shared refresh frequency select component
- * Used in both PropertiesModal and RefreshIntervalModal
+ * Shared refresh frequency select component.
+ *
+ * Renders radio buttons for available auto refresh frequencies.
+ * Reads configured intervals dynamically from Redux store
+ * (state.dashboardInfo.common.conf.DASHBOARD_AUTO_REFRESH_INTERVALS)
+ * with a fallback to REFRESH_FREQUENCY_OPTIONS if unconfigured.
+ * Also supports direct options prop override and custom numeric interval entry.
*/
export const RefreshFrequencySelect = ({
value,
onChange,
+ options: optionsProp,
}: RefreshFrequencySelectProps) => {
+ const configuredIntervals = useSelector(
+ (state: RootState) =>
+ state.dashboardInfo?.common?.conf?.DASHBOARD_AUTO_REFRESH_INTERVALS,
+ );
+
+ const activeOptions = useMemo(() => {
+ const rawOptions = optionsProp ?? configuredIntervals;
+ if (Array.isArray(rawOptions) && rawOptions.length > 0) {
+ const validOptions = rawOptions
+ .filter(
+ (item) =>
+ Array.isArray(item) &&
+ typeof item[0] === 'number' &&
+ !Number.isNaN(item[0]) &&
+ typeof item[1] === 'string',
+ )
+ .map(([interval, label]) => ({
+ value: interval,
+ label: t(label),
+ }));
+ if (validOptions.length > 0) {
+ return validOptions;
+ }
+ }
+ return REFRESH_FREQUENCY_OPTIONS.slice(0, -1);
+ }, [optionsProp, configuredIntervals]);
+
+ const isPreset = useCallback(
+ (frequency: number) => isPresetValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
+ const getCustom = useCallback(
+ (frequency: number) => getCustomValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
// Separate radio selection state from value state
const [radioSelection, setRadioSelection] = useState(() =>
- isPresetValue(value) ? value : -1,
+ isPreset(value) ? value : -1,
);
- const [customValue, setCustomValue] = useState(() => getCustomValue(value));
+ const [customValue, setCustomValue] = useState(() => getCustom(value));
useEffect(() => {
- const selection = isPresetValue(value) ? value : -1;
+ const selection = isPreset(value) ? value : -1;
setRadioSelection(selection);
- setCustomValue(selection === -1 ? getCustomValue(value) : '');
- }, [value]);
+ setCustomValue(selection === -1 ? getCustom(value) : '');
Review Comment:
β
**CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `1541e65`.
The effect now updates `customValue` only when the incoming value is not a preset. Selecting a preset therefore leaves the previously entered custom interval intact.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45039:
URL: https://github.com/apache/superset/pull/45039#discussion_r4201125306
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.test.tsx:
##########
@@ -38,3 +45,210 @@ test('getRefreshWarningMessage normalizes refreshLimit', () => {
expect(getRefreshWarningMessage(5, 10, 'warn')).toBe('warn');
expect(getRefreshWarningMessage(15, 10000, 'warn')).toBeNull();
});
+
+const mockConfiguredIntervals: [number, string][] = [
+ [0, "Don't refresh"],
+ [15, '15 seconds'],
+ [45, '45 seconds'],
+ [90, '90 seconds'],
+];
+
+const createInitialState = (
+ intervals?: [number, string][],
+ extraConf?: Record<string, unknown>,
+) => ({
+ dashboardInfo: {
+ common: {
+ conf: {
+ ...(intervals !== undefined
+ ? { DASHBOARD_AUTO_REFRESH_INTERVALS: intervals }
+ : {}),
+ ...extraConf,
+ },
+ },
+ },
+});
+
+const defaultTestProps = {
+ value: 0,
+ onChange: jest.fn(),
+};
+
+const setup = (
+ props: Partial<typeof defaultTestProps> = {},
+ initialState?: object,
+) =>
+ render(<RefreshFrequencySelect {...defaultTestProps} {...props} />, {
+ useRedux: true,
+ ...(initialState ? { initialState } : {}),
+ });
+
+test('renders configured auto refresh intervals from redux store', () => {
+ setup({}, createInitialState(mockConfiguredIntervals));
+
+ expect(screen.getByRole('radio', { name: '15 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '45 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '90 seconds' })).toBeInTheDocument();
+ expect(
+ screen.queryByRole('radio', { name: '10 seconds' }),
+ ).not.toBeInTheDocument();
+});
+
+test('renders default options when config is not present', () => {
+ setup({}, createInitialState(undefined));
+
+ expect(
+ screen.getByRole('radio', { name: "Don't refresh" }),
+ ).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '10 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '30 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '1 minute' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '24 hours' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: /Custom/i })).toBeInTheDocument();
+});
+
+test('renders default options when configured intervals is empty array', () => {
+ setup({}, createInitialState([]));
+
+ expect(screen.getByRole('radio', { name: '10 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '30 seconds' })).toBeInTheDocument();
+});
+
+test('options prop takes precedence over redux store configured intervals', () => {
+ const propOptions: [number, string][] = [
+ [5, '5 seconds'],
+ [25, '25 seconds'],
+ ];
+
+ setup({ options: propOptions }, createInitialState(mockConfiguredIntervals));
Review Comment:
**Suggestion:** `setup` accepts only `value` and `onChange`, so this call's `options` property is an excess property and fails TypeScript checking for the included test file.
**Assessment:** π `Major` Β· π `Occurrence: Often` Β· π·οΈ `Type error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=be24eb2fc1004fc9880ed666cea7c7af&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=be24eb2fc1004fc9880ed666cea7c7af&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.test.tsx
**Line:** 123:123
**Comment:**
*Type Error: `setup` accepts only `value` and `onChange`, so this call's `options` property is an excess property and fails TypeScript checking for the included test file.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45039&comment_hash=0e89d455c31a101cec7a6a8b050e937dd1d03f78c362917b3fd2301104201ee9&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45039&comment_hash=0e89d455c31a101cec7a6a8b050e937dd1d03f78c362917b3fd2301104201ee9&reaction=dislike'>π</a>
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -87,31 +120,81 @@ const normalizeRefreshLimitSeconds = (
return refreshLimit;
};
-interface RefreshFrequencySelectProps {
+/**
+ * Props for the RefreshFrequencySelect component.
+ */
+export interface RefreshFrequencySelectProps {
+ /** The currently selected refresh frequency in seconds. */
value: number;
+ /** Callback fired when a new refresh frequency is selected or typed. */
onChange: (value: number) => void;
+ /** Optional override for available interval options as [seconds, label] tuples. */
+ options?: [number, string][];
}
/**
- * Shared refresh frequency select component
- * Used in both PropertiesModal and RefreshIntervalModal
+ * Shared refresh frequency select component.
+ *
+ * Renders radio buttons for available auto refresh frequencies.
+ * Reads configured intervals dynamically from Redux store
+ * (state.dashboardInfo.common.conf.DASHBOARD_AUTO_REFRESH_INTERVALS)
+ * with a fallback to REFRESH_FREQUENCY_OPTIONS if unconfigured.
+ * Also supports direct options prop override and custom numeric interval entry.
*/
export const RefreshFrequencySelect = ({
value,
onChange,
+ options: optionsProp,
}: RefreshFrequencySelectProps) => {
+ const configuredIntervals = useSelector(
+ (state: RootState) =>
+ state.dashboardInfo?.common?.conf?.DASHBOARD_AUTO_REFRESH_INTERVALS,
+ );
+
+ const activeOptions = useMemo(() => {
+ const rawOptions = optionsProp ?? configuredIntervals;
+ if (Array.isArray(rawOptions) && rawOptions.length > 0) {
+ const validOptions = rawOptions
+ .filter(
+ (item) =>
+ Array.isArray(item) &&
+ typeof item[0] === 'number' &&
+ !Number.isNaN(item[0]) &&
+ typeof item[1] === 'string',
+ )
+ .map(([interval, label]) => ({
+ value: interval,
+ label: t(label),
+ }));
+ if (validOptions.length > 0) {
+ return validOptions;
+ }
+ }
+ return REFRESH_FREQUENCY_OPTIONS.slice(0, -1);
+ }, [optionsProp, configuredIntervals]);
+
+ const isPreset = useCallback(
+ (frequency: number) => isPresetValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
+ const getCustom = useCallback(
+ (frequency: number) => getCustomValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
// Separate radio selection state from value state
const [radioSelection, setRadioSelection] = useState(() =>
- isPresetValue(value) ? value : -1,
+ isPreset(value) ? value : -1,
);
- const [customValue, setCustomValue] = useState(() => getCustomValue(value));
+ const [customValue, setCustomValue] = useState(() => getCustom(value));
useEffect(() => {
- const selection = isPresetValue(value) ? value : -1;
+ const selection = isPreset(value) ? value : -1;
setRadioSelection(selection);
- setCustomValue(selection === -1 ? getCustomValue(value) : '');
- }, [value]);
+ setCustomValue(selection === -1 ? getCustom(value) : '');
Review Comment:
**Suggestion:** After a custom value is entered, choosing a preset updates `value` and this effect clears it; switching back to Custom then loses the user's interval.
**Assessment:** π `Major` Β· π `Occurrence: Sometimes` Β· π·οΈ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=aa8e2735fdd84fd29dd3cf7734b899d0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=aa8e2735fdd84fd29dd3cf7734b899d0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx
**Line:** 196:196
**Comment:**
*Logic Error: After a custom value is entered, choosing a preset updates `value` and this effect clears it; switching back to Custom then loses the user's interval.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45039&comment_hash=05bdc7f3ece21b648dcadc356dbb79122490bde7ab2e5083a31d5dca5ec037b3&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45039&comment_hash=05bdc7f3ece21b648dcadc356dbb79122490bde7ab2e5083a31d5dca5ec037b3&reaction=dislike'>π</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45039:
URL: https://github.com/apache/superset/pull/45039#issuecomment-6026788220
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging because the new tests pass an options property to a helper typed to reject it, causing a TypeScript error.
- Switching from a typed custom interval to a preset clears the custom value, so returning to Custom selects 1 second instead of restoring the interval.
**Assessed commit:** `195d8a98ca53`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on PR #45039:
URL: https://github.com/apache/superset/pull/45039#issuecomment-6056203416
Thanks for the detailed review @sadpandajoe.
I have addressed all points in commit d94a14d351:
1. **Store configuration lookup**: Updated `useSelector` in `RefreshFrequencySelect.tsx` to check `state.common?.conf?.DASHBOARD_AUTO_REFRESH_INTERVALS` first before falling back to `state.dashboardInfo?.common?.conf?.DASHBOARD_AUTO_REFRESH_INTERVALS`. This ensures the Properties modal correctly displays configured intervals on dashboard list and home pages as well.
2. **Custom interval selection with 1-second preset**: Added `lastEmittedCustomValueRef` to track locally emitted custom intervals. When the controlled parent feeds back the value emitted upon selecting Custom or typing a custom duration, the synchronization effect skips reverting to the preset, keeping the Custom radio selected and the input enabled. Also preserved custom values across preset toggling.
3. **Formatter compliance**: Formatted both source and test files using `oxfmt --write` with `arrowParens: "avoid"`, ensuring single-parameter arrow functions use the canonical style (`option =>`, `e =>`).
4. **Unit test coverage**: Added test cases covering the top-level `state.common.conf` store shape, controlled parent interaction when configured intervals include a 1-second preset, and custom value persistence across selections (all 17 unit tests passing).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45039:
URL: https://github.com/apache/superset/pull/45039#discussion_r4210972234
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -141,7 +224,7 @@ export const RefreshFrequencySelect = ({
return (
<StyledRadioGroup value={radioSelection} onChange={handleRadioChange}>
- {REFRESH_FREQUENCY_OPTIONS.slice(0, -1).map(option => (
+ {activeOptions.map((option) => (
Review Comment:
The new `(option) =>` / `(e) =>` arrow parens (here, line 89 and line 243) conflict with `arrowParens: "avoid"` in `.oxfmtrc.json`. The `pre-commit` job is already red because the formatter rewrites this file, so the PR can't merge until the formatter output is committed. Could you run the repo formatter on this file and keep the pre-existing `option =>` / `e =>` style?
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -87,31 +120,81 @@ const normalizeRefreshLimitSeconds = (
return refreshLimit;
};
-interface RefreshFrequencySelectProps {
+/**
+ * Props for the RefreshFrequencySelect component.
+ */
+export interface RefreshFrequencySelectProps {
+ /** The currently selected refresh frequency in seconds. */
value: number;
+ /** Callback fired when a new refresh frequency is selected or typed. */
onChange: (value: number) => void;
+ /** Optional override for available interval options as [seconds, label] tuples. */
+ options?: [number, string][];
}
/**
- * Shared refresh frequency select component
- * Used in both PropertiesModal and RefreshIntervalModal
+ * Shared refresh frequency select component.
+ *
+ * Renders radio buttons for available auto refresh frequencies.
+ * Reads configured intervals dynamically from Redux store
+ * (state.dashboardInfo.common.conf.DASHBOARD_AUTO_REFRESH_INTERVALS)
+ * with a fallback to REFRESH_FREQUENCY_OPTIONS if unconfigured.
+ * Also supports direct options prop override and custom numeric interval entry.
*/
export const RefreshFrequencySelect = ({
value,
onChange,
+ options: optionsProp,
}: RefreshFrequencySelectProps) => {
+ const configuredIntervals = useSelector(
+ (state: RootState) =>
+ state.dashboardInfo?.common?.conf?.DASHBOARD_AUTO_REFRESH_INTERVALS,
Review Comment:
Agreedβ`state.common.conf` is the store slice populated from bootstrap data on the dashboard list and home pages, while `dashboardInfo.common` is only filled by dashboard hydration, so the Properties modal on those pages still shows the hardcoded presets. Could the selector read `state.common.conf` first and fall back to `dashboardInfo.common.conf`?
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -87,31 +120,81 @@ const normalizeRefreshLimitSeconds = (
return refreshLimit;
};
-interface RefreshFrequencySelectProps {
+/**
+ * Props for the RefreshFrequencySelect component.
+ */
+export interface RefreshFrequencySelectProps {
+ /** The currently selected refresh frequency in seconds. */
value: number;
+ /** Callback fired when a new refresh frequency is selected or typed. */
onChange: (value: number) => void;
+ /** Optional override for available interval options as [seconds, label] tuples. */
+ options?: [number, string][];
}
/**
- * Shared refresh frequency select component
- * Used in both PropertiesModal and RefreshIntervalModal
+ * Shared refresh frequency select component.
+ *
+ * Renders radio buttons for available auto refresh frequencies.
+ * Reads configured intervals dynamically from Redux store
+ * (state.dashboardInfo.common.conf.DASHBOARD_AUTO_REFRESH_INTERVALS)
+ * with a fallback to REFRESH_FREQUENCY_OPTIONS if unconfigured.
+ * Also supports direct options prop override and custom numeric interval entry.
*/
export const RefreshFrequencySelect = ({
value,
onChange,
+ options: optionsProp,
}: RefreshFrequencySelectProps) => {
+ const configuredIntervals = useSelector(
+ (state: RootState) =>
+ state.dashboardInfo?.common?.conf?.DASHBOARD_AUTO_REFRESH_INTERVALS,
+ );
+
+ const activeOptions = useMemo(() => {
+ const rawOptions = optionsProp ?? configuredIntervals;
+ if (Array.isArray(rawOptions) && rawOptions.length > 0) {
+ const validOptions = rawOptions
+ .filter(
+ (item) =>
+ Array.isArray(item) &&
+ typeof item[0] === 'number' &&
+ !Number.isNaN(item[0]) &&
+ typeof item[1] === 'string',
+ )
+ .map(([interval, label]) => ({
+ value: interval,
+ label: t(label),
+ }));
+ if (validOptions.length > 0) {
+ return validOptions;
+ }
+ }
+ return REFRESH_FREQUENCY_OPTIONS.slice(0, -1);
+ }, [optionsProp, configuredIntervals]);
+
+ const isPreset = useCallback(
+ (frequency: number) => isPresetValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
+ const getCustom = useCallback(
+ (frequency: number) => getCustomValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
// Separate radio selection state from value state
const [radioSelection, setRadioSelection] = useState(() =>
- isPresetValue(value) ? value : -1,
+ isPreset(value) ? value : -1,
);
- const [customValue, setCustomValue] = useState(() => getCustomValue(value));
+ const [customValue, setCustomValue] = useState(() => getCustom(value));
useEffect(() => {
- const selection = isPresetValue(value) ? value : -1;
+ const selection = isPreset(value) ? value : -1;
setRadioSelection(selection);
- setCustomValue(selection === -1 ? getCustomValue(value) : '');
- }, [value]);
+ setCustomValue(selection === -1 ? getCustom(value) : '');
Review Comment:
Agreedβwith a configured 1-second preset, clicking Custom emits `onChange(1)`, the controlled parent feeds `1` back, and the `[value, isPreset, getCustom]` effect re-selects the preset and disables the custom input, so Custom can't be edited on the first click. Could the effect skip re-syncing when the incoming value was just emitted by the Custom radio?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #45039:
URL: https://github.com/apache/superset/pull/45039#issuecomment-6044001963
This duplicates https://github.com/apache/superset/pull/44982
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #45039:
URL: https://github.com/apache/superset/pull/45039#discussion_r4210410189
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -87,31 +120,81 @@ const normalizeRefreshLimitSeconds = (
return refreshLimit;
};
-interface RefreshFrequencySelectProps {
+/**
+ * Props for the RefreshFrequencySelect component.
+ */
+export interface RefreshFrequencySelectProps {
+ /** The currently selected refresh frequency in seconds. */
value: number;
+ /** Callback fired when a new refresh frequency is selected or typed. */
onChange: (value: number) => void;
+ /** Optional override for available interval options as [seconds, label] tuples. */
+ options?: [number, string][];
}
/**
- * Shared refresh frequency select component
- * Used in both PropertiesModal and RefreshIntervalModal
+ * Shared refresh frequency select component.
+ *
+ * Renders radio buttons for available auto refresh frequencies.
+ * Reads configured intervals dynamically from Redux store
+ * (state.dashboardInfo.common.conf.DASHBOARD_AUTO_REFRESH_INTERVALS)
+ * with a fallback to REFRESH_FREQUENCY_OPTIONS if unconfigured.
+ * Also supports direct options prop override and custom numeric interval entry.
*/
export const RefreshFrequencySelect = ({
value,
onChange,
+ options: optionsProp,
}: RefreshFrequencySelectProps) => {
+ const configuredIntervals = useSelector(
+ (state: RootState) =>
+ state.dashboardInfo?.common?.conf?.DASHBOARD_AUTO_REFRESH_INTERVALS,
Review Comment:
PropertiesModal also uses this selector on the dashboard list and home page. Those routes store configuration in `state.common.conf` (`src/views/store.ts:149`), while `dashboardInfo` starts empty. Its legacy `common` alias is populated by dashboard hydration (`src/dashboard/actions/hydrate.ts:379-381`). As a result, these editors still show hardcoded intervals. Read the top-level configuration first, retain the legacy fallback, and add a test with the list-page store shape.
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -87,31 +120,81 @@ const normalizeRefreshLimitSeconds = (
return refreshLimit;
};
-interface RefreshFrequencySelectProps {
+/**
+ * Props for the RefreshFrequencySelect component.
+ */
+export interface RefreshFrequencySelectProps {
+ /** The currently selected refresh frequency in seconds. */
value: number;
+ /** Callback fired when a new refresh frequency is selected or typed. */
onChange: (value: number) => void;
+ /** Optional override for available interval options as [seconds, label] tuples. */
+ options?: [number, string][];
}
/**
- * Shared refresh frequency select component
- * Used in both PropertiesModal and RefreshIntervalModal
+ * Shared refresh frequency select component.
+ *
+ * Renders radio buttons for available auto refresh frequencies.
+ * Reads configured intervals dynamically from Redux store
+ * (state.dashboardInfo.common.conf.DASHBOARD_AUTO_REFRESH_INTERVALS)
+ * with a fallback to REFRESH_FREQUENCY_OPTIONS if unconfigured.
+ * Also supports direct options prop override and custom numeric interval entry.
*/
export const RefreshFrequencySelect = ({
value,
onChange,
+ options: optionsProp,
}: RefreshFrequencySelectProps) => {
+ const configuredIntervals = useSelector(
+ (state: RootState) =>
+ state.dashboardInfo?.common?.conf?.DASHBOARD_AUTO_REFRESH_INTERVALS,
+ );
+
+ const activeOptions = useMemo(() => {
+ const rawOptions = optionsProp ?? configuredIntervals;
+ if (Array.isArray(rawOptions) && rawOptions.length > 0) {
+ const validOptions = rawOptions
+ .filter(
+ (item) =>
+ Array.isArray(item) &&
+ typeof item[0] === 'number' &&
+ !Number.isNaN(item[0]) &&
+ typeof item[1] === 'string',
+ )
+ .map(([interval, label]) => ({
+ value: interval,
+ label: t(label),
+ }));
+ if (validOptions.length > 0) {
+ return validOptions;
+ }
+ }
+ return REFRESH_FREQUENCY_OPTIONS.slice(0, -1);
+ }, [optionsProp, configuredIntervals]);
+
+ const isPreset = useCallback(
+ (frequency: number) => isPresetValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
+ const getCustom = useCallback(
+ (frequency: number) => getCustomValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
// Separate radio selection state from value state
const [radioSelection, setRadioSelection] = useState(() =>
- isPresetValue(value) ? value : -1,
+ isPreset(value) ? value : -1,
);
- const [customValue, setCustomValue] = useState(() => getCustomValue(value));
+ const [customValue, setCustomValue] = useState(() => getCustom(value));
useEffect(() => {
- const selection = isPresetValue(value) ? value : -1;
+ const selection = isPreset(value) ? value : -1;
setRadioSelection(selection);
- setCustomValue(selection === -1 ? getCustomValue(value) : '');
- }, [value]);
+ setCustomValue(selection === -1 ? getCustom(value) : '');
Review Comment:
When configured intervals include a 1-second preset, selecting Custom from 0 emits `onChange(1)`. The production parents feed that value back, so this effect immediately selects the preset, clears the custom value, and disables the input. Preserve explicitly selected Custom mode when receiving locally emitted values, while still synchronizing external value or option changes. Add a controlled-parent regression test: the new callback-only mocks do not feed updated values back and miss this failure.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45039:
URL: https://github.com/apache/superset/pull/45039#discussion_r4216871294
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -87,31 +120,81 @@ const normalizeRefreshLimitSeconds = (
return refreshLimit;
};
-interface RefreshFrequencySelectProps {
+/**
+ * Props for the RefreshFrequencySelect component.
+ */
+export interface RefreshFrequencySelectProps {
+ /** The currently selected refresh frequency in seconds. */
value: number;
+ /** Callback fired when a new refresh frequency is selected or typed. */
onChange: (value: number) => void;
+ /** Optional override for available interval options as [seconds, label] tuples. */
+ options?: [number, string][];
}
/**
- * Shared refresh frequency select component
- * Used in both PropertiesModal and RefreshIntervalModal
+ * Shared refresh frequency select component.
+ *
+ * Renders radio buttons for available auto refresh frequencies.
+ * Reads configured intervals dynamically from Redux store
+ * (state.dashboardInfo.common.conf.DASHBOARD_AUTO_REFRESH_INTERVALS)
+ * with a fallback to REFRESH_FREQUENCY_OPTIONS if unconfigured.
+ * Also supports direct options prop override and custom numeric interval entry.
*/
export const RefreshFrequencySelect = ({
value,
onChange,
+ options: optionsProp,
}: RefreshFrequencySelectProps) => {
+ const configuredIntervals = useSelector(
+ (state: RootState) =>
+ state.dashboardInfo?.common?.conf?.DASHBOARD_AUTO_REFRESH_INTERVALS,
+ );
+
+ const activeOptions = useMemo(() => {
+ const rawOptions = optionsProp ?? configuredIntervals;
+ if (Array.isArray(rawOptions) && rawOptions.length > 0) {
+ const validOptions = rawOptions
+ .filter(
+ (item) =>
+ Array.isArray(item) &&
+ typeof item[0] === 'number' &&
+ !Number.isNaN(item[0]) &&
+ typeof item[1] === 'string',
+ )
+ .map(([interval, label]) => ({
+ value: interval,
+ label: t(label),
+ }));
+ if (validOptions.length > 0) {
+ return validOptions;
+ }
+ }
+ return REFRESH_FREQUENCY_OPTIONS.slice(0, -1);
+ }, [optionsProp, configuredIntervals]);
+
+ const isPreset = useCallback(
+ (frequency: number) => isPresetValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
+ const getCustom = useCallback(
+ (frequency: number) => getCustomValue(frequency, activeOptions),
+ [activeOptions],
+ );
+
// Separate radio selection state from value state
const [radioSelection, setRadioSelection] = useState(() =>
- isPresetValue(value) ? value : -1,
+ isPreset(value) ? value : -1,
);
- const [customValue, setCustomValue] = useState(() => getCustomValue(value));
+ const [customValue, setCustomValue] = useState(() => getCustom(value));
useEffect(() => {
- const selection = isPresetValue(value) ? value : -1;
+ const selection = isPreset(value) ? value : -1;
setRadioSelection(selection);
- setCustomValue(selection === -1 ? getCustomValue(value) : '');
- }, [value]);
+ setCustomValue(selection === -1 ? getCustom(value) : '');
Review Comment:
β
**CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `d94a14d`.
The value-sync effect now updates `customValue` only when the incoming value is not a preset. Selecting a preset therefore preserves the previously entered custom interval.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on code in PR #45039:
URL: https://github.com/apache/superset/pull/45039#discussion_r4217327343
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -120,11 +220,13 @@ export const RefreshFrequencySelect = ({
if (selectedValue === -1) {
// Custom selected - use current custom value or minimum
const numValue = parseInt(customValue, 10) || MINIMUM_REFRESH_INTERVAL;
Review Comment:
Clamped the draft value in handleRadioChange so that when reselecting Custom, it only reuses drafts that parse to at least MINIMUM_REFRESH_INTERVAL, falling back to MINIMUM_REFRESH_INTERVAL (and updating customValue) otherwise. Added a controlled regression test covering the -5 -> preset -> Custom sequence in RefreshFrequencySelect.test.tsx.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on code in PR #45039:
URL: https://github.com/apache/superset/pull/45039#discussion_r4243558309
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.test.tsx:
##########
@@ -38,3 +45,210 @@ test('getRefreshWarningMessage normalizes refreshLimit', () => {
expect(getRefreshWarningMessage(5, 10, 'warn')).toBe('warn');
expect(getRefreshWarningMessage(15, 10000, 'warn')).toBeNull();
});
+
+const mockConfiguredIntervals: [number, string][] = [
+ [0, "Don't refresh"],
+ [15, '15 seconds'],
+ [45, '45 seconds'],
+ [90, '90 seconds'],
+];
+
+const createInitialState = (
+ intervals?: [number, string][],
+ extraConf?: Record<string, unknown>,
+) => ({
+ dashboardInfo: {
+ common: {
+ conf: {
+ ...(intervals !== undefined
+ ? { DASHBOARD_AUTO_REFRESH_INTERVALS: intervals }
+ : {}),
+ ...extraConf,
+ },
+ },
+ },
+});
+
+const defaultTestProps = {
+ value: 0,
+ onChange: jest.fn(),
+};
+
+const setup = (
+ props: Partial<typeof defaultTestProps> = {},
+ initialState?: object,
+) =>
+ render(<RefreshFrequencySelect {...defaultTestProps} {...props} />, {
+ useRedux: true,
+ ...(initialState ? { initialState } : {}),
+ });
+
+test('renders configured auto refresh intervals from redux store', () => {
+ setup({}, createInitialState(mockConfiguredIntervals));
+
+ expect(screen.getByRole('radio', { name: '15 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '45 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '90 seconds' })).toBeInTheDocument();
+ expect(
+ screen.queryByRole('radio', { name: '10 seconds' }),
+ ).not.toBeInTheDocument();
+});
+
+test('renders default options when config is not present', () => {
+ setup({}, createInitialState(undefined));
+
+ expect(
+ screen.getByRole('radio', { name: "Don't refresh" }),
+ ).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '10 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '30 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '1 minute' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '24 hours' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: /Custom/i })).toBeInTheDocument();
+});
+
+test('renders default options when configured intervals is empty array', () => {
+ setup({}, createInitialState([]));
+
+ expect(screen.getByRole('radio', { name: '10 seconds' })).toBeInTheDocument();
+ expect(screen.getByRole('radio', { name: '30 seconds' })).toBeInTheDocument();
+});
+
+test('options prop takes precedence over redux store configured intervals', () => {
+ const propOptions: [number, string][] = [
+ [5, '5 seconds'],
+ [25, '25 seconds'],
+ ];
+
+ setup({ options: propOptions }, createInitialState(mockConfiguredIntervals));
Review Comment:
In `RefreshFrequencySelect.tsx`, `RefreshFrequencySelectProps` explicitly defines `options?: [number, string][]`, and the test helper `setup` accepts `Partial<RefreshFrequencySelectProps>`. Passing `options` is fully supported by the component interface and passes TypeScript checking.
##########
superset-frontend/src/dashboard/components/RefreshFrequency/RefreshFrequencySelect.tsx:
##########
@@ -141,7 +224,7 @@ export const RefreshFrequencySelect = ({
return (
<StyledRadioGroup value={radioSelection} onChange={handleRadioChange}>
- {REFRESH_FREQUENCY_OPTIONS.slice(0, -1).map(option => (
+ {activeOptions.map((option) => (
Review Comment:
Formatted in commit `1541e6586c` using `oxfmt --write` with `arrowParens: "avoid"` to align with `.oxfmtrc.json` and keep single-parameter arrow syntax across source and test files.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on PR #45039:
URL: https://github.com/apache/superset/pull/45039#issuecomment-6114721959
Hi @sadpandajoe @richardfogaca,
All four review items are in place on this PR (store configuration lookup via `state.common?.conf`, custom selection retention with 1-second presets via `lastEmittedCustomValueRef`, `oxfmt` formatting with `arrowParens: "avoid"`, and negative draft clamping).
All 18 unit tests in `RefreshFrequencySelect.test.tsx` pass cleanly. Whenever you have a chance, please take a look for re-review. Thank you!
@coderabbitai review
@bito-code-review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): respect DASHBOARD_AUTO_REFRESH_INTERVALS configuration in refresh dropdown [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #45039:
URL: https://github.com/apache/superset/pull/45039
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org