fix: remove debug console.log#39655
Conversation
- Remove leftover console.log from ZoomConfigControl.tsx that polluted browser console on every Base Width slider drag (superset#39622) - Add missing service-worker.js COPY to Dockerfile lean stage so it's included in production images (superset#39431) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Code Review Agent Run #48eafaActionable Suggestions - 0Review 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 |
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
|
||
| # Copy compiled things from previous stages | ||
| COPY --from=superset-node /app/superset/static/assets superset/static/assets | ||
| COPY --from=superset-node /app/superset/static/service-worker.js superset/static/service-worker.js |
There was a problem hiding this comment.
Suggestion: This unconditional COPY will break Docker builds when frontend compilation is skipped (for example DEV_MODE: "true" in docker-compose.yml and docker-compose-light.yml), because /app/superset/static/service-worker.js is not generated in the superset-node stage in that mode. Make this copy conditional on production/frontend build output (or ensure the file is always created) to avoid build-time "file not found" failures. [possible bug]
Severity Level: Critical 🚨
- ❌ Docker dev image build fails with DEV_MODE set true.
- ❌ docker-compose-light test image build fails under DEV_MODE true.
- ⚠️ Any build using DEV_MODE=true breaks at Docker COPY.Steps of Reproduction ✅
1. In `docker-compose.yml`, note the build args for the main `superset` service at
`docker-compose.yml:6-15`, where `DEV_MODE: "true"` is passed into the Docker build via
the shared `x-common-build` section.
2. In `Dockerfile` superset-node-ci stage (`Dockerfile:9-33` from the `FROM
--platform=${BUILDPLATFORM} node:20-trixie-slim AS superset-node-ci` section), see that it
only creates the directories `/app/superset/static/assets` and
`/app/superset/translations` (lines 31-33) and does not create
`/app/superset/static/service-worker.js`.
3. In the `superset-node` stage (`Dockerfile:56-65`), observe that the frontend build
(`npm run ${BUILD_CMD}`) is executed only when `DEV_MODE="false"` (conditional RUN at
lines 59-65). When `DEV_MODE="true"` (as in docker-compose), the build is skipped and no
`service-worker.js` is generated under `/app/superset/static/`.
4. In the `python-common` stage (`Dockerfile:45-119`), the new line `COPY
--from=superset-node /app/superset/static/service-worker.js
superset/static/service-worker.js` at lines 104-106 unconditionally copies a single file
from the `superset-node` stage. With `DEV_MODE="true"`,
`/app/superset/static/service-worker.js` does not exist in `superset-node`, causing
`docker build` invoked by `docker compose up` (with default `target: dev` and `DEV_MODE:
"true"`) to fail with a "no source files were specified" / "file not found" error on that
COPY instruction.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Dockerfile
**Line:** 205:205
**Comment:**
*Possible Bug: This unconditional `COPY` will break Docker builds when frontend compilation is skipped (for example `DEV_MODE: "true"` in `docker-compose.yml` and `docker-compose-light.yml`), because `/app/superset/static/service-worker.js` is not generated in the `superset-node` stage in that mode. Make this copy conditional on production/frontend build output (or ensure the file is always created) to avoid build-time "file not found" failures.
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
|
|
||
| # Copy compiled things from previous stages | ||
| COPY --from=superset-node /app/superset/static/assets superset/static/assets | ||
| COPY --from=superset-node /app/superset/static/service-worker.js superset/static/service-worker.js |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The new COPY of /app/superset/static/service-worker.js from the superset-node stage assumes this file exists, but when DEV_MODE="true" (the default in docker-compose dev builds), npm run ${BUILD_CMD} is skipped and the service worker is never generated, causing the multi-stage build (including the default dev target) to fail with a missing source file.
Suggestion: Make the service-worker COPY conditional on non-dev builds (e.g. only for lean/prod targets) or ensure a placeholder service-worker.js is created in the superset-node stage when DEV_MODE="true" so the cross-stage COPY always has a valid source.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** Dockerfile
**Line:** 205:205
**Comment:**
*HIGH: The new COPY of `/app/superset/static/service-worker.js` from the `superset-node` stage assumes this file exists, but when `DEV_MODE="true"` (the default in docker-compose dev builds), `npm run ${BUILD_CMD}` is skipped and the service worker is never generated, causing the multi-stage build (including the default `dev` target) to fail with a missing source file.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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
|
The flagged issue is correct. The unconditional COPY will cause Docker build failures when DEV_MODE="true" because /app/superset/static/service-worker.js is not generated in the superset-node stage in that mode. To resolve, ensure the file is always created by adding RUN touch /app/superset/static/service-worker.js in the superset-node stage after the conditional frontend build. Checked other comments: there are no other review comments on this PR. Dockerfile |
|
I already opened a PR to address the missing |
|
Hi! Two small cleanup items here: removing a debug console.log statement and ensuring service-worker.js is included in the Docker image build. Both are low-risk changes. Thanks for considering! |
|
@7487 I mean you can remove the change that fixes missing |
|
All Right～ |
hainenber
changed the title
2 participants
Summary
Two small fixes for the Superset frontend build:
fix: Remove leftover debug console.log in ZoomConfigControl
Removes a debug
console.logstatement accidentally left in theonBaseWidthChangehandler. This pollutes the browser console on every Base Width slider drag. Fixes #39622.fix: Add service-worker.js to Dockerfile lean stage
The webpack build outputs
service-worker.jsone directory aboveassets/, but onlyassets/was being copied in the lean Docker image stage. This causes a 404 when the browser tries to register the service worker in subdirectory deployments. Fixes #39431.