bito-code-review[bot] commented on code in PR #41527:
URL: https://github.com/apache/superset/pull/41527#discussion_r3498213971
##########
superset-frontend/src/explore/components/controls/DateFilterControl/components/CommonFrame.tsx:
##########
@@ -16,26 +16,123 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { useState } from 'react';
import { t } from '@apache-superset/core/translation';
+import { InputNumber, Select } from '@superset-ui/core/components';
import { Radio } from '@superset-ui/core/components/Radio';
import {
COMMON_RANGE_OPTIONS,
- COMMON_RANGE_SET,
DateFilterTestKey,
} from 'src/explore/components/controls/DateFilterControl/utils';
import {
- CommonRangeType,
FrameComponentProps,
} from 'src/explore/components/controls/DateFilterControl/types';
+const PRESET_VALUES = new Set(COMMON_RANGE_OPTIONS.map(o => o.value as
string));
+
+/** Sentinel value used as the radio option value for the custom "Other" row.
*/
+const CUSTOM_SENTINEL = '__custom__';
+
+const UNIT_OPTIONS = [
+ { value: 'second', label: t('Seconds') },
+ { value: 'minute', label: t('Minutes') },
+ { value: 'hour', label: t('Hours') },
+ { value: 'day', label: t('Days') },
+ { value: 'week', label: t('Weeks') },
+ { value: 'month', label: t('Months') },
+ { value: 'quarter', label: t('Quarters') },
+ { value: 'year', label: t('Years') },
+];
+
+/**
+ * Parse "Last N unit(s)" into { n, unit }, returns null for preset-style
+ * strings without an explicit number (e.g. "Last day").
+ */
+function parseLastN(value: string): { n: number; unit: string } | null {
+ const m = value.match(
+ /^[Ll]ast\s+(\d+)\s+(second|minute|hour|day|week|month|quarter|year)s?$/i,
+ );
+ return m ? { n: Number(m[1]), unit: m[2].toLowerCase() } : null;
+}
+
+/** Build the canonical "Last N unit(s)" string from parts. */
+function buildLastN(n: number, unit: string): string {
+ return `Last ${n} ${unit}${n !== 1 ? 's' : ''}`;
+}
+
export function CommonFrame(props: FrameComponentProps) {
- let commonRange = 'Last week';
- if (COMMON_RANGE_SET.has(props.value as CommonRangeType)) {
- commonRange = props.value;
- } else {
- props.onChange(commonRange);
+ const isPreset = PRESET_VALUES.has(props.value);
+ const parsedCustom = parseLastN(props.value);
+ const isCustom = !isPreset && parsedCustom !== null;
+
+ // Local state for the custom inputs — persists across preset ↔ Other
switches.
+ // Default to 4 hours so the initial "Other" emission ("Last 4 hours") never
+ // accidentally matches a preset and causes the radio to snap back.
+ const [customN, setCustomN] = useState<number>(parsedCustom?.n ?? 4);
+ const [customUnit, setCustomUnit] = useState<string>(
+ parsedCustom?.unit ?? 'hour',
+ );
+
+ // If the current value doesn't match any known pattern, reset to a default.
+ if (!isPreset && !isCustom) {
+ props.onChange('Last week');
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing useEffect for side-effect in render</b></div>
<div id="fix">
`props.onChange('Last week')` is called during every render without a
`useEffect` guard. When `props.value` is an unrecognized string the component
calls `onChange` during render, which triggers a parent state update, which
re-renders this component — creating an unnecessary render cycle even if the
parent is robust. Move the logic into a `useEffect([props.value])` block with a
mount guard.
</div>
</div>
<small><i>Code Review Run #14bd76</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/DateFilterControl/utils/constants.ts:
##########
@@ -111,13 +119,10 @@ export const SINCE_MODE_OPTIONS: SelectOptionType[] = [
export const UNTIL_MODE_OPTIONS: SelectOptionType[] =
SINCE_MODE_OPTIONS.slice();
-export const COMMON_RANGE_SET: Set<CommonRangeType> = new Set([
- 'Last day',
- 'Last week',
- 'Last month',
- 'Last quarter',
- 'Last year',
-]);
+/** @deprecated Use {@link COMMON_RANGE_REGEX} for open-ended pattern
matching. */
+export const COMMON_RANGE_SET = {
+ has: (value: string) => COMMON_RANGE_REGEX.test(value),
+} as const;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Type incompatibility in deprecated export</b></div>
<div id="fix">
The new `COMMON_RANGE_SET` is an object with only a `.has()` method instead
of a proper `Set`. Callers expecting a `Set` (e.g., calling `.size`,
`.entries()`, or `.forEach()`) will receive TypeScript errors. Either export a
`Set<string>` constructed from the regex match results, or update the
deprecation comment and docblock to reflect that it is now a plain object
wrapper.
</div>
</div>
<small><i>Code Review Run #14bd76</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
