Skip to content

JMCP: Papers - jmcp/add-deployment-config-dockerfile-fly-tom-CSMbptzY#5

Closed
landigf wants to merge 1 commit into
mainfrom
jmcp/add-deployment-config-dockerfile-fly-tom-CSMbptzY
Closed

JMCP: Papers - jmcp/add-deployment-config-dockerfile-fly-tom-CSMbptzY#5
landigf wants to merge 1 commit into
mainfrom
jmcp/add-deployment-config-dockerfile-fly-tom-CSMbptzY

Conversation

@landigf

@landigf landigf commented Mar 24, 2026

Copy link
Copy Markdown
Owner

Review Summary

Change Summary

Adds Fly.io deployment configuration for the Papers Next.js app:

  • Dockerfile — multi-stage build (deps → build → run) targeting Next.js standalone output
  • fly.toml — Fly.io app config (papers, region iad, shared-cpu-1x/512 MB), health check at /papers/api/health
  • Health endpoint (apps/web/app/api/health/route.ts) — returns JSON { status, mode, timestamp }
  • next.config.ts — adds output: "standalone" and outputFileTracingRoot pointing to monorepo root
  • .dockerignore — standard exclusions

Validation Confidence: Medium-High

The structure is textbook-correct for a Next.js standalone + Fly deployment. The outputFileTracingRoot correctly resolves to the monorepo root, and the Dockerfile COPY paths align with the standalone output layout.

Risks

# Risk Severity Detail
1 Missing public/ folder copy in Dockerfile High Next.js standalone doesn't bundle public/. If any static assets exist (or are added later), the container will 404 on them. A COPY --from=build /app/apps/web/public ./apps/web/public line should be added even if the folder is currently empty — it's a silent future footgun.
2 Health endpoint imports getPapersConfig Medium If @papers/config reads env vars that aren't set (e.g. DB URLs), the health check could throw instead of returning 200. Fly's check would then mark the machine unhealthy and restart-loop. A health endpoint should ideally be dependency-free or catch errors gracefully.
3 min_machines_running = 0 Low Machines will scale to zero. First request after idle will cold-start (Node boot + Next.js init). Fine for a demo/staging app, but worth noting if uptime matters.
4 No secrets provisioned Info fly.toml only sets public env vars. Any secrets (DATABASE_URL, API keys) will need fly secrets set before first deploy — not captured anywhere in the diff.
5 node:23-slim is unpinned Low node:23-slim will float to the latest Node 23 patch. Pinning a digest or using an LTS tag (e.g. 22-slim) would improve reproducibility.

Verdict

Structurally sound. Risk #1 (missing public/ copy) should be fixed before merge — it's a one-liner and prevents a class of silent breakage. Risk #2 is worth hardening (wrap the config call in a try/catch or make health zero-dep). The rest are advisory.

@landigf

landigf commented Mar 25, 2026

Copy link
Copy Markdown
Owner Author

Closing: will integrate valuable features directly on main to avoid cascade merge conflicts.

@landigf landigf closed this Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant