Skip to content

Conversation

@Fabio1988
Copy link
Contributor

This would improve the docker image by 50%, from 1.30GB to 640 MB

Just a POC, untested.. Had this locally from ages, maybe we can use some inspiration from it @Mygod ?

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a 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 automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements a multi-stage Docker build to optimize the ReactMap image size, reducing it from 1.30GB to 640MB (approximately 50% reduction). The approach separates the build environment from the runtime environment, ensuring only production dependencies and built artifacts are included in the final image.

Key changes:

  • Introduced multi-stage Dockerfile with separate builder and runtime stages
  • Added comprehensive .dockerignore file to exclude unnecessary files from build context
  • Updated GitHub Actions checkout to v4 with explicit fetch-depth configuration

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File Description
Dockerfile Implements multi-stage build with builder stage for compilation and minimal runtime stage with only production dependencies and built artifacts
.dockerignore Adds exclusion patterns for development files, tests, logs, and build artifacts to reduce build context size
.github/workflows/docker.yml Updates checkout action to v4 and explicitly sets shallow clone depth

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1 to +2
# Do NOT ignore .git - needed in the image for version metadata
# .git
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment states that .git is needed in the image for version metadata, but the actual version metadata comes from the .gitsha and .gitref files created by the GitHub workflow, not from the .git directory itself. The comment is misleading and should be updated to clarify that .git can be ignored because the workflow explicitly creates .gitsha and .gitref files.

Suggested change
# Do NOT ignore .git - needed in the image for version metadata
# .git
# Version metadata in the image comes from .gitsha and .gitref files created by the GitHub workflow,
# so the .git directory itself is not required for version metadata.

Copilot uses AI. Check for mistakes.
Comment on lines +13 to +14
with:
fetch-depth: 1
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fetch-depth: 1 parameter is redundant as it's already the default behavior for actions/checkout@v4. While not harmful, it can be removed to simplify the configuration.

Copilot uses AI. Check for mistakes.
Comment on lines +23 to +26
COPY packages ./packages
COPY server ./server
COPY public ./public
COPY ReactMap.js ./ReactMap.js
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copies at lines 22-26 are redundant since line 29 uses 'COPY . .' which copies everything anyway. This negates the layer caching benefit that the earlier selective copies were supposed to provide. Either remove lines 23-26 and keep only package.json and yarn.lock before the full copy, or remove line 29 entirely if all necessary files are already copied individually.

Suggested change
COPY packages ./packages
COPY server ./server
COPY public ./public
COPY ReactMap.js ./ReactMap.js

Copilot uses AI. Check for mistakes.
COPY public ./public
COPY ReactMap.js ./ReactMap.js

# Copy remaining source needed for build (excluding node_modules via .dockerignore)
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .dockerignore file excludes 'dist' on line 7, but the runtime stage attempts to copy it from the builder stage. If the dist directory is built during the build process, this works correctly. However, if there's a pre-existing dist directory in the source, it would be excluded from the builder stage, which is the intended behavior. The comment on line 28 of the Dockerfile should clarify that dist is intentionally excluded and will be generated by 'yarn build'.

Suggested change
# Copy remaining source needed for build (excluding node_modules via .dockerignore)
# Copy remaining source needed for build; .dockerignore intentionally excludes node_modules and any pre-existing dist, which will be regenerated by 'yarn build'

Copilot uses AI. Check for mistakes.

# Copy production node_modules and built assets from builder
COPY --from=builder /home/node/node_modules ./node_modules
COPY --from=builder /home/node/package.json ./package.json
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The yarn.lock file should be copied to the runtime stage along with package.json to maintain dependency version integrity. While it's not strictly necessary for runtime, it ensures that any operations involving yarn in the container use the exact same dependency versions as the builder stage.

Suggested change
COPY --from=builder /home/node/package.json ./package.json
COPY --from=builder /home/node/package.json ./package.json
COPY --from=builder /home/node/yarn.lock ./yarn.lock

Copilot uses AI. Check for mistakes.
Comment on lines +46 to +48
# Install yarn in runtime if you need it (keeps compatibility).
RUN npm install -g [email protected]

Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Installing yarn globally in the runtime stage may be unnecessary if the application doesn't need to run any yarn commands at runtime. Since the production dependencies are already installed in the builder stage and copied over, consider removing this installation to further reduce image size and attack surface. Only keep it if runtime yarn commands are actually required.

Suggested change
# Install yarn in runtime if you need it (keeps compatibility).
RUN npm install -g [email protected]

Copilot uses AI. Check for mistakes.
COPY --from=builder /home/node/dist ./dist
COPY --from=builder /home/node/ReactMap.js ./ReactMap.js
COPY --from=builder /home/node/packages ./packages
COPY --from=builder /home/node/config ./config
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .gitsha and .gitref files created by the GitHub workflow (lines 15-18 in docker.yml) are not explicitly copied to the runtime stage. These files are needed by server/src/services/checkForUpdates.js to determine the current version when running in Docker. Add these files to the runtime stage copy operations.

Suggested change
COPY --from=builder /home/node/config ./config
COPY --from=builder /home/node/config ./config
COPY --from=builder /home/node/.gitsha ./.gitsha
COPY --from=builder /home/node/.gitref ./.gitref

Copilot uses AI. Check for mistakes.
@Mygod
Copy link
Collaborator

Mygod commented Dec 31, 2025

This poc is too minimal and the benefit is too marginal so I will not work on this.

@Mygod Mygod added the Won't Fix This will not be worked on label Dec 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Won't Fix This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants