codeant-ai-for-open-source[bot] commented on code in PR #35083:
URL: https://github.com/apache/superset/pull/35083#discussion_r3454564245
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -2289,57 +2304,60 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
initialValue={resource?.sql}
key={currentAlert?.id}
/>
- </StyledInputContainer>
+ </ModalFormField>
<div
className="inline-container wrap"
css={css`
gap: ${theme.sizeUnit}px;
`}
>
- <StyledInputContainer css={noMarginBottom}>
- <div className="control-label" css={inputSpacer}>
- {t('Trigger Alert If...')}
- <span className="required">*</span>
- </div>
- <div className="input-container">
- <Select
- ariaLabel={t('Condition')}
- onChange={onConditionChange}
- placeholder={t('Condition')}
- value={
- currentAlert?.validator_config_json?.op ||
- undefined
- }
- options={CONDITIONS}
- />
- </div>
- </StyledInputContainer>
- <StyledInputContainer css={noMarginBottom}>
- <div className="control-label">
- {t('Value')}{' '}
- {!conditionNotNull && (
- <span className="required">*</span>
- )}
- </div>
- <div className="input-container">
- <InputNumber
- disabled={conditionNotNull}
- type="number"
- name="threshold"
- value={
- currentAlert?.validator_config_json
- ?.threshold !== undefined &&
- !conditionNotNull
- ? currentAlert.validator_config_json
- .threshold
- : ''
- }
- min={0}
- placeholder={t('Value')}
- onChange={onThresholdChange}
- />
- </div>
- </StyledInputContainer>
+ <ModalFormField
+ label={t('Trigger Alert If...')}
+ required
+ error={
+ validationStatus[Sections.Alert]?.hasErrors &&
+ !currentAlert?.validator_config_json?.op
+ ? t('Condition is required')
+ : undefined
+ }
+ >
+ <Select
+ ariaLabel={t('Condition')}
+ onChange={onConditionChange}
+ placeholder={t('Condition')}
+ value={
+ currentAlert?.validator_config_json?.op ||
+ undefined
+ }
+ options={CONDITIONS}
+ />
+ </ModalFormField>
+ <ModalFormField
+ label={t('Value')}
+ required={!conditionNotNull}
+ error={
+ validationStatus[Sections.Alert]?.hasErrors &&
+ !conditionNotNull &&
+ !currentAlert?.validator_config_json?.threshold
+ ? t('Value is required')
+ : undefined
+ }
+ >
Review Comment:
**Suggestion:** The required check for the numeric threshold uses a falsy
test, so a valid value of `0` is treated as missing and incorrectly shows a
validation error. Compare explicitly against `undefined`/`null` instead of
using `!value`. [falsy zero check]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Alert threshold value zero incorrectly flagged as missing.
- ⚠️ Users see misleading validation message configuring alert conditions.
- ⚠️ Confusing UX on Alerts & Reports creation modal.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Alerts & Reports list page implemented in
`superset-frontend/src/pages/AlertReportList/index.tsx` and click the "Add"
button, which
calls `handleAlertEdit(null)` and opens `AlertReportModal`
(`AlertReportModal.tsx`).
2. In `AlertReportModal` (`AlertReportModal.tsx`, Collapse items starting
around line
142), ensure you are creating an Alert (`isReport === false`) so the "Alert
condition"
panel is rendered.
3. In the "Alert condition" section (ModalFormField for "Value" around lines
237–245),
select a condition that requires a numeric threshold (so `conditionNotNull`
is false),
enter `0` in the "Value" field, but leave another alert condition field
invalid (e.g.
leave "Database" empty so `validateAlertSection()` at lines 25–42 pushes an
error).
4. Click Save so `validateAll()` (lines 78–84) runs, setting
`validationStatus[Sections.Alert].hasErrors` to true; observe that the
"Value" field shows
`t('Value is required')` even though
`currentAlert.validator_config_json.threshold` is
`0`, because the error expression at lines 239–243 uses
`!currentAlert?.validator_config_json?.threshold`, treating zero as missing.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=53e5f240971d4b4ba81d4b356674a1c0&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=53e5f240971d4b4ba81d4b356674a1c0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/AlertReportModal.tsx
**Line:** 2339:2345
**Comment:**
*Falsy Zero Check: The required check for the numeric threshold uses a
falsy test, so a valid value of `0` is treated as missing and incorrectly shows
a validation error. Compare explicitly against `undefined`/`null` instead of
using `!value`.
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%2F35083&comment_hash=facfcb295acce74ceb90195f394880ea3c2d7802e38be026a246aa6d79e61afa&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35083&comment_hash=facfcb295acce74ceb90195f394880ea3c2d7802e38be026a246aa6d79e61afa&reaction=dislike'>👎</a>
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -2688,57 +2712,51 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
) : (
<Loading size="s" muted position="normal" />
)}
- </StyledInputContainer>
- <StyledInputContainer>
- <div className="control-label">
- {t('Log retention')}
- <span className="required">*</span>
- </div>
- <div className="input-container">
- <Select
- ariaLabel={t('Log retention')}
- placeholder={t('Log retention')}
- onChange={onLogRetentionChange}
- value={currentAlert?.log_retention}
- options={RETENTION_OPTIONS}
- sortComparator={propertyComparator('value')}
- />
- </div>
- </StyledInputContainer>
- <StyledInputContainer css={noMarginBottom}>
- {isReport ? (
- <>
- <div className="control-label">
- {t('Working timeout')}
- <span className="required">*</span>
- </div>
- <div className="input-container">
- <NumberInput
- min={1}
- name="working_timeout"
- value={currentAlert?.working_timeout || ''}
- placeholder={t('Time in seconds')}
- onChange={onTimeoutVerifyChange}
- timeUnit={t('seconds')}
- />
- </div>
- </>
- ) : (
- <>
- <div className="control-label">{t('Grace
period')}</div>
- <div className="input-container">
- <NumberInput
- min={1}
- name="grace_period"
- value={currentAlert?.grace_period || ''}
- placeholder={t('Time in seconds')}
- onChange={onTimeoutVerifyChange}
- timeUnit={t('seconds')}
- />
- </div>
- </>
- )}
- </StyledInputContainer>
+ </ModalFormField>
+ <ModalFormField
+ label={t('Log retention')}
+ required
+ error={
+ validationStatus[Sections.Schedule]?.hasErrors &&
+ !currentAlert?.log_retention
+ ? t('Log retention is required')
+ : undefined
+ }
Review Comment:
**Suggestion:** `log_retention` allows `0` ("None"), but this required check
treats `0` as empty and shows an error even when the selected value is valid.
Use an explicit null/undefined check so zero is accepted. [falsy zero check]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Log retention value zero shown as invalid in schedule.
- ⚠️ Users misled about acceptable retention options for reports.
- ⚠️ Confusing validation UX in schedule panel of alert modal.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Alerts & Reports list page
(`superset-frontend/src/pages/AlertReportList/index.tsx`) and create or edit
a report to
open `AlertReportModal` (`AlertReportModal.tsx`).
2. In the "Schedule" panel of `AlertReportModal` (Collapse item keyed
`'schedule'` around
lines 2683–2760), set a valid crontab in `AlertReportCronScheduler` and a
valid working
timeout so that schedule validation should pass (`validateScheduleSection()`
at lines
45–54 only checks `crontab` and `working_timeout`).
3. In the "Log retention" field rendered inside `ModalFormField` at lines
2717–2724,
choose an option whose underlying value is `0` (the "None"/no retention
option, mapped to
`currentAlert.log_retention === 0`).
4. Trigger validation (e.g. click Save so `validateAll()` runs and
`validationStatus[Sections.Schedule].hasErrors` may be true because of other
schedule
errors during editing) and observe that the `error` expression at lines
2719–2723
(`!currentAlert?.log_retention`) treats `0` as falsy and shows "Log
retention is
required", even though selecting zero is valid and not checked in
`validateScheduleSection()`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=90dacd96ade541b8b605bbaa88efcdfb&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=90dacd96ade541b8b605bbaa88efcdfb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/AlertReportModal.tsx
**Line:** 2719:2724
**Comment:**
*Falsy Zero Check: `log_retention` allows `0` ("None"), but this
required check treats `0` as empty and shows an error even when the selected
value is valid. Use an explicit null/undefined check so zero is accepted.
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%2F35083&comment_hash=6bee6802abc820db7f41f914881750071281438ae231e878a6c2cd03b9dec8f3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35083&comment_hash=6bee6802abc820db7f41f914881750071281438ae231e878a6c2cd03b9dec8f3&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/AlertReportList/index.tsx:
##########
@@ -405,6 +455,16 @@ function AlertList({
onClick: handleEdit,
}
: null,
+ allowEdit
+ ? {
+ label: 'trigger-now-action',
+ tooltip: t('Trigger Now'),
+ placement: 'bottom',
+ icon: 'ThunderboltOutlined',
+ loading: executingIds.has(original.id),
+ onClick: () => handleExecuteReport(original),
+ }
Review Comment:
**Suggestion:** The action object passes a `loading` property, but
`ListViewActionProps`/`ActionButton` do not implement a loading prop, so no
loading UI or disable behavior is applied. This breaks the intended UX and
leaves the action still clickable while executing. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ "Trigger Now" button shows no loading/disabled state while running.
- ⚠️ Users can keep clicking despite in-flight execution requests.
- ⚠️ Inconsistent UX compared to other loading-aware buttons in app.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Alerts & Reports list
(`superset-frontend/src/pages/AlertReportList/index.tsx`) so that the
Actions column
renders `ListViewActionsBar` with `actions` constructed at lines 41–79,
including the
"Trigger Now" action object when `allowEdit` is true.
2. Note that each action is typed as `ListViewActionProps` when passed to
`<ListViewActionsBar actions={actions as ListViewActionProps[]} />` at lines
81–83, and
`ListViewActionProps` is an alias for `ActionProps` exported from
`src/components/ListView/ActionsBar.tsx` (re-exported via
`src/components/ListView/index.ts` and `src/components/index.ts`).
3. Inspect `ActionProps` in `ActionsBar.tsx` (lines 47–53) and observe it
declares only
`label`, `tooltip`, `placement`, `icon`, and `onClick`—there is no `loading`
prop in this
API surface, and the `ActionsBar` component simply forwards `...rest` to
`ActionButton`.
4. When `loading: executingIds.has(original.id)` is set on the "Trigger Now"
action at
line 466, this extra prop is not part of `ActionProps` and there is no
guarantee
`ActionButton` uses it to render a spinner or disable the button, so the
user sees no
visual loading state and the action remains clickable even while
`executeReport()` (in
`useExecuteReportSchedule.ts`) is in flight.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c0b54d7670d14f0aab2e5b8263684948&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=c0b54d7670d14f0aab2e5b8263684948&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/AlertReportList/index.tsx
**Line:** 466:466
**Comment:**
*Api Mismatch: The action object passes a `loading` property, but
`ListViewActionProps`/`ActionButton` do not implement a loading prop, so no
loading UI or disable behavior is applied. This breaks the intended UX and
leaves the action still clickable while executing.
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%2F35083&comment_hash=73d2c3ab5180b8410a54f92cf6b235a854333eb9acb0f0a159b0e05c889066a3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35083&comment_hash=73d2c3ab5180b8410a54f92cf6b235a854333eb9acb0f0a159b0e05c889066a3&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/AlertReportList/index.tsx:
##########
@@ -254,6 +259,51 @@ function AlertList({
[alerts, setResourceCollection, updateResource],
);
+ const handleExecuteReport = useCallback(
+ async (alert: AlertObject) => {
+ const alertId = alert.id;
+ if (!alertId || executingIds.has(alertId)) {
+ return;
+ }
+
+ // Add to executing set
+ setExecutingIds(prev => new Set(prev).add(alertId));
+
Review Comment:
**Suggestion:** This click guard reads `executingIds` from the current
render and then updates state asynchronously, so rapid double-clicks before
rerender can pass the check twice and fire duplicate execute requests. Make the
check/update atomic in a functional state update (or disable the action
immediately) to avoid this race. [race condition]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Rapid double-click triggers duplicate /api/v1/report/{id}/execute.
- ❌ Users may receive duplicate alert/report emails or notifications.
- ⚠️ Extra load on Celery workers from redundant executions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Navigate to the Alerts & Reports list implemented in
`superset-frontend/src/pages/AlertReportList/index.tsx` and let `AlertList`
render the
Actions column with the "Trigger Now" action (actions array built around
lines 41–79,
using `handleExecuteReport`).
2. For a report row where `allowEdit` is true, quickly double-click the
"Trigger Now"
action, which calls `handleExecuteReport(original)` twice before React
re-renders
(`handleExecuteReport` defined at lines 123–165).
3. On the first click, `executingIds` is an empty `Set`, so the guard `if
(!alertId ||
executingIds.has(alertId))` at lines 125–127 passes, and
`setExecutingIds(prev => new
Set(prev).add(alertId))` at line 131 schedules an update but does not
synchronously change
the captured `executingIds`.
4. On the second rapid click, the same closure still sees the old
`executingIds` (still
empty), so the guard again passes and schedules a second `setExecutingIds`,
resulting in
two concurrent calls to `executeReport(alertId, ...)` (lines 134–153) and
two POSTs to
`/api/v1/report/{id}/execute` (hook in `useExecuteReportSchedule.ts`),
causing duplicate
immediate executions and notifications for the same report.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f6bcfc4a73a54adcb78884f676e5ab61&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=f6bcfc4a73a54adcb78884f676e5ab61&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/AlertReportList/index.tsx
**Line:** 264:271
**Comment:**
*Race Condition: This click guard reads `executingIds` from the current
render and then updates state asynchronously, so rapid double-clicks before
rerender can pass the check twice and fire duplicate execute requests. Make the
check/update atomic in a functional state update (or disable the action
immediately) to avoid this race.
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%2F35083&comment_hash=ad4e3921fe4ae377024afe8ed9e8183f408d5f6f906a78b45b4dc137d810ca90&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35083&comment_hash=ad4e3921fe4ae377024afe8ed9e8183f408d5f6f906a78b45b4dc137d810ca90&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: [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]
