rusackas commented on PR #37107:
URL: https://github.com/apache/superset/pull/37107#issuecomment-3830945528
## Bot Review Comments Assessment
I've audited all 28 bot review comments against the actual code on this
branch. Here's the consolidated assessment:
### Already Fixed in Later Commits (5 comments)
These were flagged on earlier commits but addressed in subsequent commits on
this branch:
| Bot | File | Issue | Status |
|-----|------|-------|--------|
| codeant-ai | ChartRenderer.tsx:238 | setDataMask ignores prop | Fixed in
b6529df — reverted to match original behavior |
| codeant-ai | ChartRenderer.tsx:315 | Drill-to-detail uses stale viz_type |
Actually an *improvement* — PR uses derived `vizType` instead of
`formData.viz_type` |
| codeant-ai | CollectionTable:118 | onCellChange id typed as number | Fixed
— now `string \| number` |
| bito | Field/index.tsx:38 | Missing default for inline prop | Fixed — now
`inline = false` |
| bito | Datasource/types.ts:76 | onCellChange val typed as boolean | Fixed
— now `unknown` |
### Pre-existing Bugs FIXED by This PR (7 comments)
These bots correctly identified real bugs — but they existed in the original
JS and were **fixed** during this migration:
| Bot | File | Issue | What Changed |
|-----|------|-------|-------------|
| bito | reducer.ts:61 | SET_REPORT filterField mismatch | Original used raw
`filterField` (`dashboard_id`); new code correctly maps to `dashboard`/`chart` |
| bito | actions.ts:143 | Object.keys(charts) unguarded | Original had no
isEmpty check; new code adds `!isEmpty(charts)` guard |
| bito | actions.ts | addReport lacks error handling | Original had no
`.catch()`; new code adds error toast |
| bito | actions.ts | editReport lacks error handling | Same — added
`.catch()` |
| bito | actions.ts | deleteActiveReport `.finally()` bug | Original used
`.finally()` (success toast on failure!); new code correctly uses
`.then()`/`.catch()` |
| bito | actions.ts:213 | Grammar: "active" → "activate" | Fixed typo |
| bito | reducer.ts:152 | alerts_reports key undefined | Original used
`dashboard \|\| chart` for all types; new code uses `report.id` for alerts |
### False Positives (2 comments)
| Bot | File | Issue | Why False |
|-----|------|-------|-----------|
| codeant-ai | chartAction.ts:259 | ChartDataResponse unused | Bot
misidentified — the interface `ChartDataRequestResponse` IS used as return type
for `legacyChartDataRequest()`, `v1ChartDataRequest()`, and
`getChartDataRequest()` |
| bito | actions.ts:122 | Error message says "dashboard" | Already improved
— new code uses generic "There was an issue fetching reports." |
### Pre-existing Issues Migrated Unchanged (3 comments)
These are real observations, but the code is unchanged from the original JS
— out of scope for a TS migration PR:
| Bot | File | Issue |
|-----|------|-------|
| codeant-ai | chartAction.ts:112 | `POST_CHART_FORM_DATA` is dead code
(never dispatched) |
| bito | chartAction.ts:581/615 | `annotation.overrides` mutation |
| bito | chartAction.ts:926 | `json.result[0]` array access (but a length
check exists above) |
### Intentional/Justified Decisions (11 comments)
These are deliberate choices made during the TypeScript migration:
- **ChartRenderer.tsx `as any` (line 540)**: Documented with
`eslint-disable` comment — hooks type mismatch between `ChartHooks` and `Hooks`
interfaces requires this cast until the plugin API types are unified.
- **exploreReducer.ts SliceUpdatedAction owners type (line 155)**:
Intentionally handles both `number[]` and `{value, label}[]` formats with a
union type.
- **useResultsPane.tsx type assertions (lines 77, 95)**: Necessary casts
from `ensureIsArray` return type.
- **FilterValue.tsx async assertion**: Uses `Parameters<typeof
waitForAsyncData>[0]` which correctly resolves to `AsyncEvent`.
- **ExploreChartHeader.test.tsx double assertion**: Standard test pattern
for partial mock props.
- **DatasourceEditor.tsx CurrencyControl no-op onChange (line 2066)**:
TypeScript requires the prop; original JSX simply omitted it (equivalent to
undefined). The Field wrapper handles actual state management.
- **logger.test.ts location mocking**: Standard `Object.defineProperty`
pattern for JSDOM.
- **logger.test.ts event_type expectation**: New test file with correct
expectations.
- **ExploreChartHeader.test.tsx test expectation**: Tests the current
component behavior.
### Summary
- **0 new bugs** introduced by this PR
- **7 pre-existing bugs fixed** during migration
- **5 issues already addressed** in later commits
- **3 pre-existing patterns** carried forward unchanged (out of scope)
- **2 false positives** from bots
- **11 intentional decisions** that are correct as-is
--
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]
