codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3524252845
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -415,6 +416,15 @@ export function dbReducer(
[action.payload.name]: action.payload.checked,
};
}
+ if (action.payload.name === 'cache_timeout') {
+ const val = Number(action.payload.value);
+ return {
+ ...trimmedState,
+ cache_timeout: Number.isNaN(val)
+ ? undefined
+ : String(Math.max(-1, val)),
+ };
+ }
Review Comment:
**Suggestion:** When the chart cache timeout input is cleared, `Number('')`
becomes `0`, so the reducer stores `0` instead of removing the value. This
makes it impossible to return to the documented “use global default if
undefined” behavior via normal clearing. Handle empty string explicitly before
numeric coercion. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Clearing chart cache timeout forces cache to never expire.
- ⚠️ Admins cannot restore global cache timeout by clearing field.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the UI, open a database connection edit modal (DatabaseConnectionForm
rendered from
`superset-frontend/src/features/databases/DatabaseModal/index.tsx`, lines
1940-1979) and
go to “Advanced → Performance” where the chart cache timeout input is
rendered by
`ExtraOptions`.
2. In `ExtraOptions`
(`superset-frontend/src/features/databases/DatabaseModal/ExtraOptions.tsx`,
lines
360-399), locate the “Chart cache timeout” input: `<Input type="number"
name="cache_timeout" min={-1} value={db?.cache_timeout ?? ''} ...
onChange={onInputChangeValidateTimeout} />`. Its helper text states that 0
means cache
never expires, -1 bypasses the cache, and “undefined” defaults to the global
timeout.
3. Clear the “Chart cache timeout” field so the browser input value becomes
`''`. The
change handler `onInputChangeValidateTimeout` (ExtraOptions.tsx lines 77-85)
computes
`Number(target.value)`, which for `''` is `0`, and only blocks values less
than `-1`.
Since `0 < -1` is false, it forwards the event to `onInputChange`.
4. `DatabaseConnectionForm` forwards this event to the reducer via
`onInputChange`
(index.tsx lines 1940-21), dispatching `ActionType.InputChange` with `name:
'cache_timeout', value: ''`. In `dbReducer`’s `InputChange` case (index.tsx
lines 153-168;
diff lines 413-426), the reducer runs:
- `const val = Number(action.payload.value);` → `val = 0` for `''`.
- `cache_timeout: Number.isNaN(val) ? undefined : String(Math.max(-1,
val)),`
Since `val` is `0`, `Number.isNaN(val)` is false and `Math.max(-1, 0)` is
`0`, so it
stores `cache_timeout = '0'` in state. On save, this is persisted to
`Database.cache_timeout` (`superset/models/core.py` line 27) and
validated by
`DatabaseConnectionSchema.cache_timeout` (`superset/databases/schemas.py`
lines
1185-1189) which allows `0`. As a result, clearing the field sets an
explicit zero
timeout instead of `None`, making it impossible via normal UI clearing to
return to the
documented “use global timeout when undefined” behavior referenced in
`cache_timeout_description` (`superset/databases/schemas.py` lines 90-94).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=57af5dc73ba34b44bc34b88baaa815ed&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=57af5dc73ba34b44bc34b88baaa815ed&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/databases/DatabaseModal/index.tsx
**Line:** 419:427
**Comment:**
*Logic Error: When the chart cache timeout input is cleared,
`Number('')` becomes `0`, so the reducer stores `0` instead of removing the
value. This makes it impossible to return to the documented “use global default
if undefined” behavior via normal clearing. Handle empty string explicitly
before numeric coercion.
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%2F38490&comment_hash=7320fde01ce15943fcb61d3e0dcd0fbd5b360095fee289f85f86a29e3cf987ef&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=7320fde01ce15943fcb61d3e0dcd0fbd5b360095fee289f85f86a29e3cf987ef&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]
