-
Notifications
You must be signed in to change notification settings - Fork 0
chore: optimize production docker image #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
Dockerfile
Outdated
| # Frontend dependencies and build | ||
| COPY frontend/package.json frontend/pnpm-lock.yaml frontend/ | ||
| RUN cd frontend && pnpm install | ||
| RUN cd frontend && pnpm install && pnpm build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P0] Build frontend after copying sources
pnpm build is executed right after only frontend/package.json and pnpm-lock.yaml are copied. The actual frontend sources (frontend/src, config files, etc.) are brought in later via COPY . ., so the build runs in an almost empty directory and will fail when Docker tries to build the image. This makes the production Docker image unbuildable. The source tree needs to be copied (or the build command moved) before running pnpm build.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex fix comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
- Moved the frontend build step to occur after the application source is copied, ensuring
pnpm buildruns with the full frontend source tree available DockerfileL35-L43
Testing
- ✅
pytest -q
| # Serve the pre-built frontend as static files | ||
| app.mount("/", StaticFiles(directory="frontend/dist", html=True), name="frontend") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Guard static mount when frontend artifacts absent
Mounting the frontend with StaticFiles(directory="frontend/dist", html=True) happens at import time and StaticFiles raises a RuntimeError if the directory does not exist. The repository ignores frontend/dist, so running the backend locally (uvicorn api.server:app) or executing tests that import api.server now crashes unless the frontend was built beforehand. Consider conditionally mounting the static files or falling back when frontend/dist is missing so development and non-Docker runs continue to work.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex fix comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
- Added a guard around the frontend static mount, using
Pathto ensure the directory exists before mounting and prevent runtime errors whenfrontend/distis missing api/server.pyL1-L8 api/server.pyL299-L302 - Reordered the Docker build steps to install frontend dependencies separately and build the frontend only after copying the project sources, ensuring a successful build DockerfileL35-L41
Testing
- ✅
pytest -q
# Conflicts: # Dockerfile
Streamlined Dockerfile to minimize image size by excluding dev files and clarified build/run steps. Enhanced README with detailed environment variable settings, Docker usage instructions, and a docker-compose example for quick setup.
Summary
Testing
pytest -qhttps://chatgpt.com/codex/tasks/task_e_68c458c476e48326bad89486af0bfcd2