-
Notifications
You must be signed in to change notification settings - Fork 141
Feat: dockerfile to build the app #632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds Docker support via a two-stage Dockerfile (build + runtime), provides Docker-related guidance in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer / CI
participant Docker as Docker Engine
participant Builder as Builder Image (node:22-alpine)
participant Runtime as Runtime Image (node:22-alpine)
note over Dev,Docker #f0f4c3: Build phase
Dev->>Docker: docker build (Dockerfile)
Docker->>Builder: create builder stage
Builder->>Builder: install pnpm & deps\nrun pnpm build -> .output
note over Docker,Runtime #e3f2fd: Assemble runtime
Docker->>Runtime: copy .output, configs, .env
note over Docker,Runtime #c8e6c9: Run phase
Dev->>Docker: docker run -d -p 3000:3000 ...
Docker->>Runtime: start container (pnpm start)
Runtime->>Runtime: app listens on 0.0.0.0:3000
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Dockerfile (3)
6-10: Combine consecutiveRUNcommands to reduce image layers.Multiple single-line
RUNcommands create extra layers. Consider grouping related operations:
- Lines 8–10: Combine
apk update,apk upgrade, andapk add gitinto one command.- Lines 37–38: Combine pnpm and npm-run-all installations in the runtime stage.
# install pnpm & git -RUN npm install -g pnpm -RUN apk update && \ - apk upgrade && \ - apk add --no-cache git +RUN npm install -g pnpm && \ + apk update && \ + apk upgrade && \ + apk add --no-cache gitAnd in the runtime stage:
# install pnpm -RUN npm install -g pnpm -RUN npm install -g npm-run-all +RUN npm install -g pnpm npm-run-allAlso applies to: 37-38
2-2: Consider running the container as a non-root user for security.Both the builder and runtime stages run as the root user by default. This increases the attack surface if the container is compromised. Add a non-root user (e.g.,
nodeor a custom user) to each stage:# Stage 1: builder FROM node:22-alpine AS builder WORKDIR /app +# Create non-root user for build +RUN addgroup -g 1001 -S nodejs && \ + adduser -S nodejs -u 1001 + # install pnpm & git RUN npm install -g pnpm RUN apk update && \ apk upgrade && \ apk add --no-cache git RUN git init # copy lock, package files and configs COPY package.json pnpm-lock.yaml ./ COPY run-jiti.js ./ COPY src/features/build-info/script-to-generate-json.ts src/features/build-info/build-info.gen.json ./src/features/build-info/ COPY prisma/schema.prisma ./prisma/ +USER nodejs RUN pnpm install --frozen-lockfile --prod=false # copy source COPY . . # build .output ENV NODE_OPTIONS=--max-old-space-size=4096 RUN pnpm build # Stage 2: runtime FROM node:22-alpine AS runtime +# Create non-root user for runtime +RUN addgroup -g 1001 -S nodejs && \ + adduser -S nodejs -u 1001 WORKDIR /app # install pnpm RUN npm install -g pnpm RUN npm install -g npm-run-all ## copy output build and package.json from builder COPY --from=builder /app/.output ./.output COPY --from=builder /app/package.json ./package.json COPY --from=builder /app/src/features/build-info/build-info.gen.json ./src/features/build-info/build-info.gen.json COPY --from=builder /app/src/features/build-info/script-to-generate-json.ts ./src/features/build-info/script-to-generate-json.ts +RUN chown -R nodejs:nodejs /app # install only production dependencies +USER nodejs RUN pnpm install --prodAlso applies to: 31-31, 52-60
41-41: Fix comment formatting.Line 41 uses
##(two hashes) instead of the conventional single#for comments:-## copy output build and package.json from builder +# copy output build and package.json from builder
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🔬 Tests (22)
- GitHub Check: 🔬 Tests (24)
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (2)
Dockerfile (2)
12-12: Clarify the purpose ofgit initin the builder stage.Line 12 initializes a git repository, but it's unclear why this is needed since no
.gitdirectory or git commands appear afterward. If this is unused, it should be removed to keep the image lean.Can you explain the purpose of
git init? If it's not needed for the build process, I recommend removing it to reduce layer overhead.
37-38: Verify thatnpm-run-allis actually required.Line 38 installs
npm-run-allglobally in the runtime stage, but theCMD(line 60) only runspnpm startwithout any indication that npm-run-all is used. If it's not needed by the startup script or your application, removing it will reduce image size.Can you confirm whether
npm-run-allis needed at runtime, or if it can be safely removed?Also applies to: 60-60
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.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Dockerfile (3)
7-7: Prefer corepack over npm install for pnpm management.Node.js 22 includes
corepack, a built-in package manager that can manage pnpm without explicitly installing it. Using corepack is more maintainable and follows Node.js ecosystem best practices.Apply this diff to use corepack in both stages:
-# install pnpm & git -RUN npm install -g pnpm +# install git RUN apk update && \ - apk upgrade && \ apk add --no-cache gitAnd in the runtime stage:
-# install pnpm -RUN npm install -g pnpm npm-run-all +# enable corepack +RUN corepack enable && \ + npm install -g npm-run-allThen ensure your
package.jsonhas a"packageManager"field like:{ "packageManager": "[email protected]" }Also applies to: 37-37
8-10: Remove apk upgrade from multi-stage build for better layer caching.
apk upgradein a Docker layer can cause cache invalidation issues and is unnecessary when you're only adding specific packages. Useapk add --no-cachealone to ensure reproducible builds and better caching.Apply this diff to simplify the apk installation:
-RUN apk update && \ - apk upgrade && \ +RUN apk add --no-cache gitThis removes the redundant
apk updateandapk upgradecalls.
26-26: Consider whether NODE_OPTIONS should also apply to the runtime stage.
NODE_OPTIONS=--max-old-space-size=4096is set in the builder stage (line 26) to help with memory during the build, but it only takes effect during build time. If your production application needs this setting, ensure it's also set in the runtime stage (after line 49).If the setting is needed for runtime, add it to the runtime stage:
ENV NODE_ENV=production +ENV NODE_OPTIONS=--max-old-space-size=4096 ENV HOST=0.0.0.0 ENV PORT=3000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 🔬 Tests (24)
- GitHub Check: 🔬 Tests (lts/*)
- GitHub Check: 🔬 Tests (22)
- GitHub Check: Playwright E2E Tests
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.
Actionable comments posted: 1
♻️ Duplicate comments (3)
Dockerfile (3)
12-12:⚠️ Duplicate:git initremains unnecessary and should be removed.This issue was previously flagged as critical. When launching containers, networks must be created explicitly; Docker does not auto-create custom networks. The
git initcommand initializes a repository that is never used and adds unnecessary build overhead. No build scripts, pnpm commands, or dependencies require a.gitdirectory.Apply this diff to remove the line:
RUN apk update && \ apk upgrade && \ apk add --no-cache git -RUN git init
46-46:⚠️ Duplicate: Do not copy.envinto the Docker image — critical security risk.This issue was previously flagged as critical. Copying environment files into the image bakes secrets into image layers permanently, making them available to anyone with image access. This violates security best practices.
Environment configuration should be injected at runtime via:
- Environment variable flags (
docker run -e)- Bind mounts or volumes
- Secret management systems (Docker Secrets, Kubernetes Secrets, CI/CD injection)
Apply this diff to remove the line:
RUN npm install -g pnpm npm-run-all -COPY .env ./
50-54:⚠️ Duplicate: Avoid copyingnode_modulesfrom builder to runtime — platform incompatibility risk.This issue was previously flagged as major. Copying compiled
node_modulesacross stages can cause runtime failures if native modules (bcrypt, sharp, etc.) were compiled for the builder's platform and don't match the runtime environment.Reinstall dependencies in the runtime stage using the frozen lockfile instead:
-## copy output build and package.json from builder COPY --from=builder /app/.output ./.output COPY --from=builder /app/package.json ./package.json COPY --from=builder /app/pnpm-lock.yaml ./pnpm-lock.yaml -COPY --from=builder /app/node_modules ./node_modules + +RUN pnpm install --frozen-lockfile --prodThis ensures dependencies are properly compiled for the runtime environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env.example(1 hunks)Dockerfile(1 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.example
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🔬 Tests (lts/*)
- GitHub Check: 🔬 Tests (24)
- GitHub Check: Playwright E2E Tests
5b60f6e to
16a2e4b
Compare
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/api/rpc.$.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🔬 Tests (22)
- GitHub Check: 🔬 Tests (24)
- GitHub Check: Playwright E2E Tests
e34335d to
2c102ae
Compare
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.
2c102ae to
2ded2a1
Compare
2ded2a1 to
3b5bd2f
Compare
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/api/rpc.$.ts(1 hunks)vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/api/rpc.$.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Playwright E2E Tests
- GitHub Check: 🔬 Tests (lts/*)
package.json
Outdated
| "dk:start": "docker compose start", | ||
| "dk:stop": "docker compose stop", | ||
| "dk:clear": "docker compose down --volumes", | ||
| "dk:build": "docker build -t start-ui-web . && docker run -d --network start-ui-web_default -p 3000:3000 --name start-ui-web-container start-ui-web", |
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.
The idea is to build a Docker image to deploy on some Docker running platform like k8s, we don't really want a script in package.json but more of a command to copy/paste in the README (just the docker build -t start-ui-web . part I think, and then how to use this built docker image
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.
tell me if in the readme.md section Dockerfile, the description and command match with your suggestion
| ENV PORT=3000 | ||
|
|
||
| # install pnpm | ||
| RUN npm install -g pnpm npm-run-all |
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.
Why is npm-run-all needed?
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.
run-p not found if I don't install npm run all
3b5bd2f to
16a2e4b
Compare
8a454d1 to
a0ab4c7
Compare
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
70-70: Clarify .env file prerequisites and timing.The note about the .env file is helpful but doesn't specify whether the file must be created before running
docker build. Add context to prevent build failures from missing environment variables.Apply this diff for clarity:
-During the build process, it automatically uses your project's .env file. +> [!NOTE] +> Ensure your `.env` file exists and is properly configured in the project root before running `docker build`. The Docker build process will copy and use it to configure the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🔬 Tests (lts/*)
|


Description
build with Dockerfile
Summary by CodeRabbit
Documentation
Chores