Draft
Conversation
EricZhuXYZ
requested review from
betodealmeida,
eschutho,
geido,
kgabryje,
michael-s-molina,
mistercrunch,
rusackas,
sadpandajoe and
villebro
as code owners
Contributor
Code Review Agent Run #ebe69cActionable Suggestions - 0Additional Suggestions - 3
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
codeant-ai-for-open-source
Bot
added
the
size:XXL
Contributor
Sequence DiagramThis PR removes the XLSX export path from the explore endpoint and keeps export authorization focused on CSV downloads only. The core runtime change is that CSV requests now perform a direct CSV permission check before generating and returning file content. sequenceDiagram
participant Client
participant ExploreAPI
participant Security
participant VizEngine
Client->>ExploreAPI: Request explore data with csv option
ExploreAPI->>Security: Check csv download permission
alt Permission granted
ExploreAPI->>VizEngine: Generate csv from chart data
VizEngine-->>ExploreAPI: Csv payload
ExploreAPI-->>Client: Return csv download
else Permission denied
ExploreAPI-->>Client: Return forbidden error
end
Generated by CodeAnt AI |
github-actions
Bot
added
risk:db-migration
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+42
to
+44
| # Switch to Tsinghua Debian mirror for faster apt downloads in China | ||
| RUN sed -i 's/deb.debian.org/mirrors.tuna.tsinghua.edu.cn/g' /etc/apt/sources.list.d/debian.sources 2>/dev/null \ | ||
| || sed -i 's/deb.debian.org/mirrors.tuna.tsinghua.edu.cn/g' /etc/apt/sources.list 2>/dev/null \ |
Contributor
There was a problem hiding this comment.
Suggestion: This unconditionally rewrites Debian sources to a regional mirror, so apt-get update can fail in environments where that mirror is blocked or unavailable. Make the mirror host configurable and keep the default as deb.debian.org so builds remain reliable globally. [possible bug]
Severity Level: Critical 🚨
- ❌ CI docker-build jobs fail during apt update.
- ❌ Local docker compose setup cannot build images.
- ⚠️ Node and Python stages both affected.
Suggested change
| # Switch to Tsinghua Debian mirror for faster apt downloads in China | |
| RUN sed -i 's/deb.debian.org/mirrors.tuna.tsinghua.edu.cn/g' /etc/apt/sources.list.d/debian.sources 2>/dev/null \ | |
| || sed -i 's/deb.debian.org/mirrors.tuna.tsinghua.edu.cn/g' /etc/apt/sources.list 2>/dev/null \ | |
| # Allow overriding Debian mirror for faster regional builds | |
| ARG DEBIAN_MIRROR_HOST="deb.debian.org" | |
| RUN sed -i "s|deb.debian.org|${DEBIAN_MIRROR_HOST}|g" /etc/apt/sources.list.d/debian.sources 2>/dev/null \ | |
| || sed -i "s|deb.debian.org|${DEBIAN_MIRROR_HOST}|g" /etc/apt/sources.list 2>/dev/null \ |
Steps of Reproduction ✅
1. Trigger a normal image build path used today, e.g. CI `docker buildx build` in
`.github/workflows/superset-frontend.yml:48` (target `superset-node-ci` at line 51) or
local `docker compose up --build` documented in
`docs/admin_docs/installation/docker-compose.mdx:82`.
2. Build executes `Dockerfile:42-45` (and again `Dockerfile:113-116`) which
unconditionally rewrites Debian host to `mirrors.tuna.tsinghua.edu.cn`.
3. Subsequent package installs call `/app/docker/apt-install.sh` (`Dockerfile:48`,
`Dockerfile:204`, `Dockerfile:258`), and that script runs `apt-get update -qq` at
`docker/apt-install.sh:39`.
4. In networks where Tsinghua mirror is blocked/unavailable, `apt-get update` fails and
the Docker build stops, breaking both CI image builds and local compose bootstrap.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Dockerfile
**Line:** 42:44
**Comment:**
*Possible Bug: This unconditionally rewrites Debian sources to a regional mirror, so `apt-get update` can fail in environments where that mirror is blocked or unavailable. Make the mirror host configurable and keep the default as `deb.debian.org` so builds remain reliable globally.
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.
Comment on lines
+126
to
+129
| RUN pip install --no-cache-dir --upgrade uv -i https://pypi.tuna.tsinghua.edu.cn/simple | ||
|
|
||
| # Configure uv to use Tsinghua mirror for all subsequent installs | ||
| ENV UV_INDEX_URL=https://pypi.tuna.tsinghua.edu.cn/simple |
Contributor
There was a problem hiding this comment.
Suggestion: Python package installation is pinned to a single regional index for both pip and uv, so dependency installation fails when that index is inaccessible. Use a configurable index URL with a global default to avoid build outages outside that region. [possible bug]
Severity Level: Critical 🚨
- ❌ Python dependency installation fails across build stages.
- ❌ GitHub docker workflow cannot publish built images.
- ⚠️ Developer docker compose bootstrap blocked by index reachability.
Suggested change
| RUN pip install --no-cache-dir --upgrade uv -i https://pypi.tuna.tsinghua.edu.cn/simple | |
| # Configure uv to use Tsinghua mirror for all subsequent installs | |
| ENV UV_INDEX_URL=https://pypi.tuna.tsinghua.edu.cn/simple | |
| ARG PYPI_INDEX_URL="https://pypi.org/simple" | |
| RUN pip install --no-cache-dir --upgrade uv -i ${PYPI_INDEX_URL} | |
| # Configure uv to use the configured package index | |
| ENV UV_INDEX_URL=${PYPI_INDEX_URL} |
Steps of Reproduction ✅
1. Start a standard Docker build flow used by project automation:
`.github/workflows/docker.yml:67-89` builds images, and developers run `docker compose up
--build` per `docs/admin_docs/installation/docker-compose.mdx:82`.
2. During `python-base` stage, `Dockerfile:126` installs `uv` via a hardcoded Tsinghua
PyPI URL, then `Dockerfile:129` sets global `UV_INDEX_URL` to the same mirror.
3. Later dependency steps rely on `uv` index configuration (`Dockerfile:187`, `247`,
`275`, `277`, `287`, `296` all run `uv pip install ...`).
4. If `https://pypi.tuna.tsinghua.edu.cn/simple` is inaccessible, `pip install uv` or any
later `uv pip install` fails, aborting image creation.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Dockerfile
**Line:** 126:129
**Comment:**
*Possible Bug: Python package installation is pinned to a single regional index for both `pip` and `uv`, so dependency installation fails when that index is inaccessible. Use a configurable index URL with a global default to avoid build outages outside that region.
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.
Comment on lines
+48
to
+50
| const mockStore = createStore( | ||
| (state = { common: { locale } }) => state, | ||
| ); |
Contributor
There was a problem hiding this comment.
Suggestion: Creating the Redux store during every render causes a brand-new store instance each time ChartWrapper re-renders, which forces connected children to resubscribe/reset and can lead to avoidable render churn. Initialize the store once with a lazy state initializer so the store identity stays stable across renders. [logic error]
Severity Level: Major ⚠️
- ⚠️ Cartodiagram charts recreate Redux store every rerender.
- ⚠️ Embedded chart subtree resubscribes through Provider repeatedly.
- ⚠️ Map interactions may feel slower with many overlays.
Suggested change
| const mockStore = createStore( | |
| (state = { common: { locale } }) => state, | |
| ); | |
| const [mockStore] = useState(() => | |
| createStore((state = { common: { locale } }) => state), | |
| ); |
Steps of Reproduction ✅
1. Open a Cartodiagram chart in Superset Explore/Dashboard; this viz is registered in
`superset-frontend/src/visualizations/presets/MainPreset.ts:199-211` and loaded via
`loadChart` in
`superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/index.ts:71-77`.
2. Rendering reaches `CartodiagramPlugin` (`.../src/CartodiagramPlugin.tsx:55-57`) which
mounts `OlChartMap`; `OlChartMap` then creates `ChartLayer`
(`.../src/components/OlChartMap.tsx:37-50` in second effect chunk).
3. `ChartLayer` renders embedded charts with `ReactDOM.render` in
`.../src/components/ChartLayer.tsx:186-194` (and update path `222-230`), which calls
`createChartComponent` in `.../src/util/chartUtil.tsx:41-48`, returning `ChartWrapper`.
4. `ChartWrapper` first renders, then re-renders after `setChart`
(`.../src/components/ChartWrapper.tsx:39, 42-44`); on each render it executes
`createStore` at `:48-50`, creating a new Redux store identity and forcing provider
subtree resubscription churn.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-cartodiagram/src/components/ChartWrapper.tsx
**Line:** 48:50
**Comment:**
*Logic Error: Creating the Redux store during every render causes a brand-new store instance each time `ChartWrapper` re-renders, which forces connected children to resubscribe/reset and can lead to avoidable render churn. Initialize the store once with a lazy state initializer so the store identity stays stable across renders.
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.
Comment on lines
+95
to
+109
| ({ | ||
| isSuccess, | ||
| isError, | ||
| data, | ||
| }: { | ||
| isSuccess: boolean; | ||
| isError: boolean; | ||
| data?: CatalogOption[]; | ||
| }) => { | ||
| if (isSuccess) { | ||
| onSuccess?.(data || EMPTY_CATALOGS, forceRefresh); | ||
| } | ||
| if (isError) { | ||
| onError?.(result.error as ClientErrorObject); | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: The lazy query error handler reads result.error from the hook state instead of the error returned by the trigger promise. During a failed refetch, result.error can still be stale or undefined, so the callback may receive the wrong error payload and show an incorrect message. Use the error field from the resolved trigger result. [logic error]
Severity Level: Major ⚠️
- ⚠️ Catalog refresh may show wrong error details.
- ⚠️ SQL Lab selector error messaging becomes unreliable.
- ⚠️ Datasource/Table selector diagnostics become less actionable.
Suggested change
| ({ | |
| isSuccess, | |
| isError, | |
| data, | |
| }: { | |
| isSuccess: boolean; | |
| isError: boolean; | |
| data?: CatalogOption[]; | |
| }) => { | |
| if (isSuccess) { | |
| onSuccess?.(data || EMPTY_CATALOGS, forceRefresh); | |
| } | |
| if (isError) { | |
| onError?.(result.error as ClientErrorObject); | |
| } | |
| ({ | |
| isSuccess, | |
| isError, | |
| data, | |
| error, | |
| }: { | |
| isSuccess: boolean; | |
| isError: boolean; | |
| data?: CatalogOption[]; | |
| error?: ClientErrorObject; | |
| }) => { | |
| if (isSuccess) { | |
| onSuccess?.(data || EMPTY_CATALOGS, forceRefresh); | |
| } | |
| if (isError) { | |
| onError?.(error as ClientErrorObject); | |
| } | |
| } |
Steps of Reproduction ✅
1. Open a screen using `DatabaseSelector` (used in
`src/SqlLab/components/SqlEditorLeftBar/index.tsx:137`, `:190`,
`src/components/TableSelector/index.tsx:353`, and
`src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:1865`).
2. In `src/components/DatabaseSelector/index.tsx:487`, click refresh
(`onClick={refetchCatalogs}`), which calls `useCatalogs().refetch` from
`src/hooks/apiResources/catalogs.ts:116-118`.
3. `refetch` executes `fetchData(dbId, true)` and then `trigger({ dbId, forceRefresh
}).then(...)` at `catalogs.ts:94`; when the request fails, callback enters `isError`
branch at `catalogs.ts:107`.
4. The error callback currently passes `result.error` (`catalogs.ts:108`) instead of the
lazy trigger result error. This can be stale/undefined for that request, so
`DatabaseSelector` onError (`index.tsx:80-87`) may show generic/wrong error handling.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/hooks/apiResources/catalogs.ts
**Line:** 95:109
**Comment:**
*Logic Error: The lazy query error handler reads `result.error` from the hook state instead of the error returned by the `trigger` promise. During a failed refetch, `result.error` can still be stale or undefined, so the callback may receive the wrong error payload and show an incorrect message. Use the `error` field from the resolved trigger result.
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.
Comment on lines
+184
to
186
| merge: (currentCache: QueryResult, newItems: QueryResult) => { | ||
| currentCache.result.push(...newItems.result); | ||
| }, |
Contributor
There was a problem hiding this comment.
Suggestion: The pagination merge only appends result and never refreshes count, but the UI uses count to decide whether to keep loading more pages. If total rows change between requests (common in query history), this stale value can cause premature stop or repeated extra fetches. Update count from each new page before appending results. [logic error]
Severity Level: Major ⚠️
- ⚠️ SQL Lab history can stop before all rows.
- ⚠️ Infinite scroll can trigger unnecessary extra requests.
- ❌ Query history list may be incomplete for users.
Suggested change
| merge: (currentCache: QueryResult, newItems: QueryResult) => { | |
| currentCache.result.push(...newItems.result); | |
| }, | |
| merge: (currentCache: QueryResult, newItems: QueryResult) => { | |
| currentCache.count = newItems.count; | |
| currentCache.result.push(...newItems.result); | |
| }, |
Steps of Reproduction ✅
1. Open SQL Lab Query History with backend persistence enabled; `QueryHistory` calls
`useEditorQueriesQuery({ editorId, pageIndex })` at
`superset-frontend/src/SqlLab/components/QueryHistory/index.tsx:80-85`.
2. Scroll to bottom so pagination runs; the effect at `index.tsx:113-117` loads next pages
while `loadedDataCount < totalCount`, where `totalCount` is `data?.count` from
`index.tsx:111`.
3. The RTK query cache merge at
`superset-frontend/src/hooks/apiResources/queries.ts:184-186` appends `result` but never
updates `count`, so cache keeps first page's total.
4. If later page responses return a different total (realistic in query history as rows
are added/removed while viewing), pagination decisions use stale `totalCount`, causing
either premature stop (missing rows) or extra repeated fetches.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/hooks/apiResources/queries.ts
**Line:** 184:186
**Comment:**
*Logic Error: The pagination merge only appends `result` and never refreshes `count`, but the UI uses `count` to decide whether to keep loading more pages. If total rows change between requests (common in query history), this stale value can cause premature stop or repeated extra fetches. Update `count` from each new page before appending results.
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.
Comment on lines
+123
to
130
| serializeQueryArgs: ({ | ||
| queryArgs: { dbId, schema }, | ||
| }: { | ||
| queryArgs: Pick<FetchTablesQueryParams, 'dbId' | 'schema'>; | ||
| }) => ({ | ||
| dbId, | ||
| schema, | ||
| }), |
Contributor
There was a problem hiding this comment.
Suggestion: The cache key for table queries ignores the selected catalog, so switching catalogs while keeping the same database and schema can reuse stale cached table results from the previous catalog instead of fetching fresh data. Include catalog in serializeQueryArgs so cache entries are isolated per catalog. [logic error]
Severity Level: Major ⚠️
- ❌ Add Dataset table list can show wrong catalog tables.
- ❌ Datasource Editor may bind users to incorrect table.
- ⚠️ Users must manual refresh to recover correct list.
Suggested change
| serializeQueryArgs: ({ | |
| queryArgs: { dbId, schema }, | |
| }: { | |
| queryArgs: Pick<FetchTablesQueryParams, 'dbId' | 'schema'>; | |
| }) => ({ | |
| dbId, | |
| schema, | |
| }), | |
| serializeQueryArgs: ({ | |
| queryArgs: { dbId, catalog, schema }, | |
| }: { | |
| queryArgs: Pick<FetchTablesQueryParams, 'dbId' | 'catalog' | 'schema'>; | |
| }) => ({ | |
| dbId, | |
| catalog, | |
| schema, | |
| }), |
Steps of Reproduction ✅
1. Open a real TableSelector entry flow such as Add Dataset left panel
(`superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx:206`) or
Datasource Editor
(`superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:2062`),
both of which render `<TableSelector>`.
2. In `TableSelector`, table loading uses `useTables({ dbId, catalog: currentCatalog,
schema: currentSchema })`
(`superset-frontend/src/components/TableSelector/index.tsx:189-192`), which calls
`useTablesQuery` in `tables.ts`
(`superset-frontend/src/hooks/apiResources/tables.ts:197-199`).
3. The request includes catalog in URL params (`catalog_name`) (`tables.ts:116`), but
cache key serialization only uses `{ dbId, schema }` (`tables.ts:123-130`), so different
catalogs with same db+schema share one cache entry.
4. Reproduce by selecting catalog A + schema S, then switching catalog and re-selecting
schema S (catalog reset behavior is in `TableSelector` `internalCatalogChange`,
`index.tsx:274-281`): `useTablesQuery` can serve cached results from catalog A instead of
fetching catalog B tables, showing stale table options.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/hooks/apiResources/tables.ts
**Line:** 123:130
**Comment:**
*Logic Error: The cache key for table queries ignores the selected catalog, so switching catalogs while keeping the same database and schema can reuse stale cached table results from the previous catalog instead of fetching fresh data. Include `catalog` in `serializeQueryArgs` so cache entries are isolated per catalog.
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.
| { | ||
| skip: !(hasValidator && dbId && sql), | ||
| selectFromResult: ({ isLoading, isError, error, data }) => { | ||
| selectFromResult: ({ |
Contributor
There was a problem hiding this comment.
Suggestion: The selector builds a new array with data.map(...) on every selector run, which breaks RTK Query's shallow memoization for selectFromResult and can cause unnecessary re-renders on unrelated Redux updates. Cache the mapped annotations by data reference inside a selector factory so the returned data reference stays stable when the source data has not changed. [performance]
Severity Level: Major ⚠️
- ⚠️ SQL Lab editor does extra annotation recomputation work.
- ⚠️ Frequent selection updates can trigger unnecessary editor updates.
| true, | ||
| ) | ||
| .then(({ data }) => { | ||
| .then(({ data }: { data?: TableOptionsData }) => { |
Contributor
There was a problem hiding this comment.
Suggestion: The lazy RTK query trigger resolves with { isSuccess, isError, data, error } even on failures, so relying on .catch(...) means API errors are silently ignored and errorPayload is never set. Handle isError inside .then(...) to surface failures correctly. [logic error]
Severity Level: Major ⚠️
- ⚠️ SQL Lab schema-load failures show no error banner.
- ⚠️ Users retry blindly when tables API fails.
| if (dbId && (!result.currentData || forceRefresh)) { | ||
| trigger({ dbId, catalog, forceRefresh }).then( | ||
| ({ isSuccess, isError, data }) => { | ||
| ({ |
Contributor
There was a problem hiding this comment.
Suggestion: The lazy query error handler is wired to result.error (state from useSchemasQuery) instead of the error returned by the current trigger(...) call. When a refetch fails, this can pass a stale/undefined error object to onError, causing incorrect error reporting. Capture error from the trigger result and pass that to onError. [logic error]
Severity Level: Major ⚠️
- ⚠️ Schema refresh may lose backend error details.
- ⚠️ DatabaseSelector may show generic error fallback message.
Member
|
@EricZhuXYZ please update the PR description. The title says that you are trying to update the agents instructions, yet there are a lot of files being touched that doe not have to do with the agent instructions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
CodeAnt-AI Description
Update LLM/Agent docs, restrict CSV export permission, and speed local image builds for China
What Changed
Impact
✅ Clearer CSV download permission errors✅ Faster Docker builds in China✅ MCP developer guide available for AI assistant tooling💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.