Test upstream #37586
Open
Test upstream #37586
+749
−337,659
Conversation
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
datavigilante
requested review from
Antonio-RiveroMartnez,
betodealmeida,
craig-rueda,
dpgaspar,
eschutho,
geido,
kgabryje,
michael-s-molina,
mistercrunch,
nytai,
rusackas,
sadpandajoe and
villebro
as code owners
Contributor
|
Bito Automatic Review Skipped - Large PR |
github-actions
bot
added
i18n
github-actions
bot
added
plugins
dependencies:npm
github_actions
✅ 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}
| return ( | ||
| <StyledBlurredSection id={id}> | ||
| <StyledBlurredSection> | ||
| {children} | ||
| <img className="blur" src="/img/community/blur.png" alt="Blur" /> |
Contributor
There was a problem hiding this comment.
Suggestion: Accessibility bug: the decorative image has an actual alt text ("Blur") which will be announced by screen readers and create noise for assistive technology; mark it as purely decorative by using an empty alt attribute and aria-hidden="true". [possible bug]
Severity Level: Major ⚠️
- ⚠️ Screen readers announce decorative image in docs UI.
- ⚠️ Accessibility audits flag decorative image text.
Suggested change
| <img className="blur" src="/img/community/blur.png" alt="Blur" /> | |
| <img className="blur" src="/img/community/blur.png" alt="" aria-hidden="true" /> |
Steps of Reproduction ✅
1. Open the component source at docs/src/components/BlurredSection.tsx and locate the
image at line 48 (`<img className="blur" src="/img/community/blur.png" alt="Blur" />`).
The image element and alt text are present in the component definition starting at line 44
(function `BlurredSection`).
2. Create a minimal test that imports this component, e.g. add
`docs/src/components/BlurredSection.test.tsx` with:
- import BlurredSection from 'docs/src/components/BlurredSection';
- render(<BlurredSection>Test</BlurredSection>) using React Testing Library.
This will render the DOM that includes the img element coming from
docs/src/components/BlurredSection.tsx:48.
3. After render, query the DOM for `document.querySelector('img.blur')` and observe the
attribute `alt === "Blur"`. This demonstrates the attribute is present in runtime DOM and
will be exposed to assistive tech.
4. Run an accessibility check (e.g., axe-core / NVDA) against the rendered DOM. The
decorative image's non-empty alt text will be surfaced to screen readers and accessibility
reports as an announced resource — the noisy announcement is the issue. Because the
element is purely decorative (styling/decoration in StyledBlurredSection), changing alt to
empty and aria-hidden="true" prevents the announcement.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/src/components/BlurredSection.tsx
**Line:** 48:48
**Comment:**
*Possible Bug: Accessibility bug: the decorative image has an actual alt text ("Blur") which will be announced by screen readers and create noise for assistive technology; mark it as purely decorative by using an empty alt attribute and aria-hidden="true".
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.| @@ -94,38 +94,28 @@ const StyledSectionHeaderH2 = styled(StyledSectionHeader)` | |||
| `; | |||
|
|
|||
| interface SectionHeaderProps { | |||
| level: 'h1' | 'h2'; | |||
| level: any; | |||
Contributor
There was a problem hiding this comment.
Suggestion: The level prop is typed as any, which removes compile-time checks and can allow non-renderable values to be passed (e.g., objects), leading to React runtime errors "Invalid element type". Replace any with a precise component/element type so only valid intrinsic tags or components are allowed. [type error]
Severity Level: Major ⚠️
- ❌ Docs header rendering crashes on invalid `level` values.
- ⚠️ Runtime errors when consumers pass wrong prop types.
Suggested change
| level: any; | |
| level: keyof JSX.IntrinsicElements | ((props: any) => JSX.Element); |
Steps of Reproduction ✅
1. Edit a page that uses the SectionHeader component (import from
docs/src/components/SectionHeader.tsx).
2. In a consumer file (plain JS or a non-TS-checked MDX file), render the component with
an invalid `level` value, e.g.:
<SectionHeader level={{}} title="Test" />.
3. At runtime the code at docs/src/components/SectionHeader.tsx:109 (`const Heading =
level;`) and at line 116 (`<Heading className="title">{title}</Heading>`) will attempt to
use the object as a React element type and React will throw "Invalid element type"
(runtime error).
4. Because the prop is declared `any`, TypeScript compile-time checks do not prevent this
misuse when the component is consumed from plain JS/MDX, so the error surfaces at runtime
rather than at compile-time.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/src/components/SectionHeader.tsx
**Line:** 97:97
**Comment:**
*Type Error: The `level` prop is typed as `any`, which removes compile-time checks and can allow non-renderable values to be passed (e.g., objects), leading to React runtime errors "Invalid element type". Replace `any` with a precise component/element type so only valid intrinsic tags or components are allowed.
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.| title: string; | ||
| subtitle?: string | ReactNode; | ||
| dark?: boolean; | ||
| link?: string; | ||
| } | ||
|
|
||
| const SectionHeader = ({ | ||
| level, |
Contributor
There was a problem hiding this comment.
Suggestion: level is required by the interface but not given a runtime default; if the component is invoked from plain JS or a consumer omits level, Heading will be undefined and React will throw "Invalid element type". Provide a safe default (e.g., 'h2') in the destructuring so the component always has a valid heading tag. [possible bug]
Severity Level: Major ⚠️
- ❌ Pages using SectionHeader without `level` may fail to render.
- ⚠️ Documentation/MDX consumers are most affected.
Suggested change
| level, | |
| level = 'h2', |
Steps of Reproduction ✅
1. Import SectionHeader from docs/src/components/SectionHeader.tsx into a runtime-only
consumer (for example an MDX page or plain JS file) and render it without the `level`
prop:
<SectionHeader title="Hello" subtitle="sub" />.
2. At runtime the destructured `level` will be `undefined`; execution reaches
docs/src/components/SectionHeader.tsx:109 (`const Heading = level;`) and later line 116
where `<Heading ...>` is rendered.
3. React receives `undefined` as the element type and throws "Invalid element type" at
render time, causing the page to fail to render.
4. TypeScript enforcement prevents omission in TS code, but non-TS consumers (MDX/plain
JS) can omit `level`, so providing a safe default (e.g., 'h2') avoids these runtime
failures.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/src/components/SectionHeader.tsx
**Line:** 104:104
**Comment:**
*Possible Bug: `level` is required by the interface but not given a runtime default; if the component is invoked from plain JS or a consumer omits `level`, `Heading` will be undefined and React will throw "Invalid element type". Provide a safe default (e.g., `'h2'`) in the destructuring so the component always has a valid heading tag.
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.| } | ||
|
|
||
| const SectionHeader = ({ | ||
| level, | ||
| title, | ||
| subtitle, | ||
| dark, | ||
| link, | ||
| }: SectionHeaderProps) => { | ||
| const Heading = level; |
Contributor
There was a problem hiding this comment.
Suggestion: The code assigns const Heading = level and then uses <Heading ...> without validating that level is a valid element type; if an invalid value is passed it will throw at render time. Add a runtime guard that falls back to a safe tag (e.g., 'h2') when level isn't a string or function component. [runtime error]
Severity Level: Major ⚠️
- ❌ Runtime rendering error for invalid `level` values.
- ⚠️ Consumer code (MDX/plain JS) more at risk.
Suggested change
| const Heading = level; | |
| const Heading = (typeof level === 'string' || typeof level === 'function') ? level : 'h2'; |
Steps of Reproduction ✅
1. Render SectionHeader with an invalid runtime `level`, for example a boolean or object,
from a non-TS consumer:
<SectionHeader level={false} title="X" />.
2. Execution reaches docs/src/components/SectionHeader.tsx:109 (`const Heading = level;`)
and then line 116 where `<Heading ...>` is used.
3. React attempts to render a non-string/non-function as a component and throws "Invalid
element type", producing a visible runtime error and broken page.
4. Adding a runtime guard (as suggested) prevents the crash by substituting a safe
fallback tag ('h2') when `level` isn't a valid element type.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/src/components/SectionHeader.tsx
**Line:** 109:109
**Comment:**
*Runtime Error: The code assigns `const Heading = level` and then uses `<Heading ...>` without validating that `level` is a valid element type; if an invalid value is passed it will throw at render time. Add a runtime guard that falls back to a safe tag (e.g., `'h2'`) when `level` isn't a string or function component.
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.
docs/src/pages/community.tsx
Outdated
| itemLayout="horizontal" | ||
| dataSource={communityLinks} | ||
| renderItem={({ url, title, description, image, ariaLabel }) => ( | ||
| <List.Item className="item"> |
Contributor
There was a problem hiding this comment.
Suggestion: React list items rendered from communityLinks do not set a key prop on each <List.Item>, which will cause React reconciliation warnings and can lead to incorrect UI updates when the list changes; add a stable key (for example key={url}) to each List.Item. [possible bug]
Severity Level: Major ⚠️
- ⚠️ React console warning on community page renders.
- ⚠️ Potential incorrect list reconciliation when items change.
Suggested change
| <List.Item className="item"> | |
| <List.Item key={url} className="item"> |
Steps of Reproduction ✅
1. Start the docs dev server and navigate to the Community page that renders the Ant
Design List from docs/src/pages/community.tsx (render starts at lines 209-236).
2. Open the browser developer console. On initial render React will log a warning "Each
child in a list should have a unique 'key' prop" referencing the List.Item elements
because renderItem returns array children without a key (see List.Item at lines 214-234).
3. The warning is visible on every page load and will also surface if communityLinks
becomes dynamic (reordering, additions), potentially causing unexpected reconciliation
behavior.
4. Adding a stable key (for example key={url}) on the List.Item in
docs/src/pages/community.tsx eliminates the console warning and prevents possible
incorrect UI updates when the list changes.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/src/pages/community.tsx
**Line:** 214:214
**Comment:**
*Possible Bug: React list items rendered from `communityLinks` do not set a `key` prop on each `<List.Item>`, which will cause React reconciliation warnings and can lead to incorrect UI updates when the list changes; add a stable `key` (for example `key={url}`) to each `List.Item`.
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.
docker/apt-install.sh
Outdated
|
|
||
| # Ensure this script is run as root | ||
| if [[ ${EUID} -ne 0 ]]; then | ||
| if [[ $EUID -ne 0 ]]; then |
Contributor
There was a problem hiding this comment.
Suggestion: Using the bash-only variable EUID with set -u can cause the script to fail if it's executed under a shell that doesn't define EUID (for example if someone runs sh docker/apt-install.sh), producing an unbound variable error; use a POSIX-safe check via id -u which works regardless of shell. [possible bug]
Severity Level: Major ⚠️
- ❌ Docker image builds may fail during apt setup step.
- ⚠️ CI steps that invoke script with `sh` may error.
- ⚠️ Local manual runs using `sh` will exit unexpectedly.
Suggested change
| if [[ $EUID -ne 0 ]]; then | |
| if [ "$(id -u)" -ne 0 ]; then |
Steps of Reproduction ✅
1. From the repository root, invoke the script with a POSIX shell instead of relying on
the shebang:
run: sh docker/apt-install.sh curl
(entry point: docker/apt-install.sh; see `set -euo pipefail` at line 18).
2. Execution reaches the root check at the lines shown in the file (if block at lines
21-24).
3. Because `set -u` is active (line 18), expansion of an undefined shell-only variable
`$EUID` triggers an "unbound variable" error in /bin/sh (dash) before the if-body can
evaluate, causing the script to exit with failure.
4. Expected observable behavior: script exits immediately with an unbound-variable error
(e.g., "docker/apt-install.sh: 21: EUID: parameter not set") instead of printing the
intended "This script must be run as root" message.
Note: The file has a bash shebang (#!/usr/bin/env bash), so executing as
./docker/apt-install.sh uses bash and avoids the issue; the failure occurs specifically
when callers invoke the script via `sh docker/apt-install.sh` or other non-bash
interpreters.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docker/apt-install.sh
**Line:** 21:21
**Comment:**
*Possible Bug: Using the bash-only variable `EUID` with `set -u` can cause the script to fail if it's executed under a shell that doesn't define `EUID` (for example if someone runs `sh docker/apt-install.sh`), producing an unbound variable error; use a POSIX-safe check via `id -u` which works regardless of shell.
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.
docker/docker-bootstrap.sh
Outdated
| uv pip install -e . | ||
| fi | ||
| echo "Reinstalling the app in editable mode" | ||
| uv pip install -e . |
Contributor
There was a problem hiding this comment.
Suggestion: Using uv as a wrapper command before pip is unknown and will fail if uv is not a real executable that forwards to pip; in the DEV_MODE block you already checked for uv existence but invoking uv pip install -e . is fragile and nonstandard — use python -m pip install -e . (or plain pip) to ensure pip is invoked reliably. [possible bug]
Severity Level: Major ⚠️
- ⚠️ Dev-mode editable reinstall may fail for some developer environments.
- ⚠️ Affects developer workflow only; production images rarely use this path.
Suggested change
| uv pip install -e . | |
| python -m pip install -e . |
Steps of Reproduction ✅
1. Build or open the image that uses docker/docker-bootstrap.sh and run the script as root
with DEV_MODE enabled. In the script at docker/docker-bootstrap.sh:22-26 the DEV_MODE
branch is executed.
2. Ensure an executable named `uv` is present in PATH but it is NOT a pip wrapper (for
reproduction create /usr/local/bin/uv with no forwarding behavior). The check at
docker/docker-bootstrap.sh:23 uses `command -v uv` and will succeed.
3. Run the script (for example: docker run --entrypoint /bin/bash <image> -c
"DEV_MODE=true /app/docker/docker-bootstrap.sh app"). The script reaches lines shown at
docker/docker-bootstrap.sh:24-25 and executes `uv pip install -e .`.
4. Observe `uv` being invoked with argument `pip` which will likely error (exit non-zero
or print usage), causing the script to fail because `set -e` is enabled at the top of
docker/docker-bootstrap.sh. This demonstrates the fragile assumption that `uv` forwards to
pip.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docker/docker-bootstrap.sh
**Line:** 25:25
**Comment:**
*Possible Bug: Using `uv` as a wrapper command before `pip` is unknown and will fail if `uv` is not a real executable that forwards to pip; in the DEV_MODE block you already checked for `uv` existence but invoking `uv pip install -e .` is fragile and nonstandard — use `python -m pip install -e .` (or plain `pip`) to ensure pip is invoked reliably.
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.
docker/docker-bootstrap.sh
Outdated
| else | ||
| pip install --no-cache-dir -r "${REQUIREMENTS_LOCAL}" | ||
| fi | ||
| uv pip install --no-cache-dir -r "${REQUIREMENTS_LOCAL}" |
Contributor
There was a problem hiding this comment.
Suggestion: Unconditional call to uv pip install --no-cache-dir -r "${REQUIREMENTS_LOCAL}" assumes uv exists; that will cause the script to fail on images without uv. Add a fallback to pip (or use python -m pip) if uv is missing. [possible bug]
Severity Level: Critical 🚨
- ❌ Container startup blocked when local overrides present.
- ⚠️ Developer local dependency installation disabled unexpectedly.
Suggested change
| uv pip install --no-cache-dir -r "${REQUIREMENTS_LOCAL}" | |
| if command -v uv > /dev/null 2>&1; then | |
| uv pip install --no-cache-dir -r "${REQUIREMENTS_LOCAL}" | |
| else | |
| python -m pip install --no-cache-dir -r "${REQUIREMENTS_LOCAL}" | |
| fi |
Steps of Reproduction ✅
1. Place a local overrides file at /app/docker/requirements-local.txt inside the image
(this makes the condition at docker/docker-bootstrap.sh:51 true).
2. Start the container that runs docker/docker-bootstrap.sh (script has `set -e` at top).
The script reaches the block at docker/docker-bootstrap.sh:50-56.
3. Because the code at docker/docker-bootstrap.sh:53 unconditionally runs `uv pip install
--no-cache-dir -r "${REQUIREMENTS_LOCAL}"`, if `uv` is not installed in the image the
shell will report `command not found` and exit immediately.
4. Observe container startup failure and that local requirement overrides are not applied;
fixing to fall back to python -m pip or pip prevents this breakage.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docker/docker-bootstrap.sh
**Line:** 53:53
**Comment:**
*Possible Bug: Unconditional call to `uv pip install --no-cache-dir -r "${REQUIREMENTS_LOCAL}"` assumes `uv` exists; that will cause the script to fail on images without `uv`. Add a fallback to `pip` (or use `python -m pip`) if `uv` is missing.
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.
docker/entrypoints/run-server.sh
Outdated
| --worker-class ${SERVER_WORKER_CLASS:-gthread} \ | ||
| --threads ${SERVER_THREADS_AMOUNT:-20} \ | ||
| --log-level "${GUNICORN_LOGLEVEL:-info}" \ | ||
| --log-level "${GUNICORN_LOGLEVEL:info}" \ |
Contributor
There was a problem hiding this comment.
Suggestion: Incorrect parameter expansion operator: the code uses ${GUNICORN_LOGLEVEL:info} which is substring syntax and will cause a shell "bad substitution" error or unexpected behavior; replace it with the POSIX/Bash default-value operator ${GUNICORN_LOGLEVEL:-info} so the default "info" is used when the variable is unset or empty. [possible bug]
Severity Level: Critical 🚨
- ❌ Gunicorn fails to start (docker/entrypoints/run-server.sh:29).
- ⚠️ Container entrypoint crashes on startup.
Suggested change
| --log-level "${GUNICORN_LOGLEVEL:info}" \ | |
| --log-level "${GUNICORN_LOGLEVEL:-info}" \ |
Steps of Reproduction ✅
1. Run the entrypoint script directly: execute bash docker/entrypoints/run-server.sh (file
path: docker/entrypoints/run-server.sh). The shebang is "#!/usr/bin/env bash" at the top
of the file.
2. When the shell parses the gunicorn invocation it expands the token on line 29
(docker/entrypoints/run-server.sh:29). The script contains --log-level
"${GUNICORN_LOGLEVEL:info}" at that line.
3. If GUNICORN_LOGLEVEL is unset (common default deployment case) the expansion becomes
${GUNICORN_LOGLEVEL:info} which is parsed as a substring expansion with a non-numeric
offset ('info') and causes a shell "bad substitution" error while parsing the script.
4. Observed outcome: script aborts before launching gunicorn; confirm error message
referencing docker/entrypoints/run-server.sh:29 and "bad substitution". This prevents
application startup.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docker/entrypoints/run-server.sh
**Line:** 29:29
**Comment:**
*Possible Bug: Incorrect parameter expansion operator: the code uses `${GUNICORN_LOGLEVEL:info}` which is substring syntax and will cause a shell "bad substitution" error or unexpected behavior; replace it with the POSIX/Bash default-value operator `${GUNICORN_LOGLEVEL:-info}` so the default "info" is used when the variable is unset or empty.
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.|
|
||
| # We need at least 3GB of free mem... | ||
| MIN_MEM_FREE_GB=3 | ||
| MIN_MEM_FREE_KB=$(($MIN_MEM_FREE_GB*1000000)) |
Contributor
There was a problem hiding this comment.
Suggestion: Incorrect conversion from GB to KB: MIN_MEM_FREE_KB multiplies GB by 1,000,000 instead of 1024*1024 (1,048,576), so the script uses a lower threshold than intended and may not warn when free memory is below the actual GB threshold. [logic error]
Severity Level: Major ⚠️
- ⚠️ Developer Docker memory nag may not trigger appropriately.
- ⚠️ Local frontend startup guidance may be misleading.
Suggested change
| MIN_MEM_FREE_KB=$(($MIN_MEM_FREE_GB*1000000)) | |
| MIN_MEM_FREE_KB=$(($MIN_MEM_FREE_GB*1024*1024)) |
Steps of Reproduction ✅
1. Run the script file docker/frontend-mem-nag.sh (this file) from the repo root; the
script sets MIN_MEM_FREE_GB at the top and computes MIN_MEM_FREE_KB at line 23
(`docker/frontend-mem-nag.sh:23`).
2. The script defines echo_mem_warn() (starts at `docker/frontend-mem-nag.sh:25`) which
reads /proc/meminfo into MEM_FREE_KB and compares it against MIN_MEM_FREE_KB at the
conditional on `docker/frontend-mem-nag.sh:29`.
3. On a system with ~3.10 GB free (for example MEM_FREE_KB ≈ 3,100,000), the current code
computes MIN_MEM_FREE_KB=3,000,000 (using *1,000,000). Because 3,100,000 > 3,000,000 the
script will not print the insufficient-memory warning even though the correct threshold
using 1024*1024 would be 3,145,728 KB and the system should have been warned.
4. Observe that no warning is emitted (the else branch prints "Memory check Ok [...]" at
`docker/frontend-mem-nag.sh:44`) even though a correct GB-to-KB conversion would have
caused the warning. This demonstrates the incorrect multiplier at
`docker/frontend-mem-nag.sh:23` causes missed nags.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docker/frontend-mem-nag.sh
**Line:** 23:23
**Comment:**
*Logic Error: Incorrect conversion from GB to KB: `MIN_MEM_FREE_KB` multiplies GB by 1,000,000 instead of 1024*1024 (1,048,576), so the script uses a lower threshold than intended and may not warn when free memory is below the actual GB threshold.
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.
github-actions
bot
removed
doc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION