fix(web): fail loudly in production when NEXT_PUBLIC_API_URL is unset - #172
Open
Mamavee001 wants to merge 1 commit into
Open
fix(web): fail loudly in production when NEXT_PUBLIC_API_URL is unset#172Mamavee001 wants to merge 1 commit into
Mamavee001 wants to merge 1 commit into
Conversation
… (issue 5.9) apps/web/lib/api.ts's BROWSER_BASE fallback to http://localhost:8787 was unconditional - right for local dev, wrong (and previously actually broken production - see docs/FIXLOG.md, BUG-1.4) everywhere else, since NEXT_PUBLIC_* values are baked in at build time and a missing one isn't caught until a visitor's own browser silently tries to reach localhost on their own machine. - The localhost fallback now only applies when NODE_ENV !== "production". - A production build with NEXT_PUBLIC_API_URL unset throws at module load, in the browser bundle specifically (checked via `typeof window`, which is a reliable environment split here since Next.js produces genuinely separate server/browser bundles - the server bundle doesn't need this variable at all if API_URL is set, so it isn't punished for a client-only var it never uses). - The error message names the variable, states it must be set AND rebuilt (not just redeployed), and points at the prior incident. - docs/MAINNET.md's existing NEXT_PUBLIC_STELLAR_NETWORK footgun section gained a parallel callout for NEXT_PUBLIC_API_URL with the same "baked at build time, rebuild after changing" framing, plus the real incident history. .env.public.example got the matching inline comment. BUG-1.4's own "Fix" column already claimed this was "covered by the build-time env check," but the actual code (this file, before this change) still had the bare unconditional fallback - that fix was a deploy-checklist reminder, not an enforced guard, so the class of bug was never actually closed. This is the enforced version. No Node.js/npm/pnpm runtime is available in the environment this was authored in, and apps/web has no existing unit-test harness to hook a regression test into (only e2e Playwright specs, which don't exercise a production Next.js build's env-var baking). I verified the four branches (var set; unset+dev; unset+production+browser; unset+ production+server) by hand rather than by running anything. Please build with NEXT_PUBLIC_API_URL deliberately unset and NODE_ENV=production to confirm the throw fires with the right message, and confirm a normal `pnpm --filter @checkout/web dev` still works unaffected.
|
@Mamavee001 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
@Mamavee001 is attempting to deploy a commit to the determined's projects Team on Vercel. A member of the Team first needs to authorize it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #155
This already happened once - a real incident, not a hypothetical
While reading the surrounding code I found
docs/FIXLOG.md's BUG-1.4 (2026-07-14): a Vercel build once ran withNEXT_PUBLIC_API_URLunset, soapps/web/lib/api.ts'shttp://localhost:8787local-dev fallback got baked into the production client bundle, and every visitor's browser silently tried (and failed) to reachlocalhost:8787on their own machine - "Create link" just did nothing, no error naming the cause.That entry's own "Fix" column claims this is "covered by the build-time env check" - but the actual code, right up until this PR, still had the bare, unconditional
process.env.NEXT_PUBLIC_API_URL ?? "http://localhost:8787". The fix that actually shipped was a deploy-checklist reminder (MAINTAINER.md), not an enforced guard - so the class of bug was never really closed, just documented. This PR is the enforced version.What changed
apps/web/lib/api.ts'sBROWSER_BASE:localhost:8787fallback now only applies whenNODE_ENV !== "production".NEXT_PUBLIC_API_URLunset throws at module load, specifically in the browser bundle. I usedtypeof windowto distinguish server vs. browser at module-evaluation time rather than inside a function call - this is reliable here (not a runtime toggle) because Next.js produces genuinely separate server and browser bundles, and each evaluates this module's top level for the first time in its own real environment. The server bundle doesn't needNEXT_PUBLIC_API_URLat all ifAPI_URLis set (seeapiBase()'s own server branch, unchanged), so it isn't punished for a client-only variable it never actually uses - I deliberately kept this fix scoped to the browser-targeting-its-own-machine failure mode the issue names, not a broader "also crash the server ifAPI_URLis missing" change nobody asked for.NEXT_PUBLIC_*is baked at build time), and points at the prior incident for context.docs/MAINNET.md's existingNEXT_PUBLIC_STELLAR_NETWORKfootgun section gained a parallel callout forNEXT_PUBLIC_API_URLwith the same "baked at build time, rebuild after changing" framing plus the real incident history - and.env.public.examplegot the matching inline comment.Test plan
No Node.js/npm/pnpm runtime is available in the environment this was authored in, and
apps/webhas no existing unit-test harness to hook a regression test into (only e2e Playwright specs, which exercise a running dev/prod server rather than a production Next.js build's env-var baking specifically - not the right tool for this). I traced through the four branches by hand instead of running anything: variable set (any env); unset + non-production (dev fallback); unset + production + browser (throws); unset + production + server (falls back silently, same as before - server doesn't need this var ifAPI_URLis set).Please, before merging:
pnpm --filter @checkout/web buildwithNEXT_PUBLIC_API_URLdeliberately unset andNODE_ENV=productionand confirm the build/runtime actually throws with the expected message.pnpm --filter @checkout/api dev/pnpm --filter @checkout/web devnormally (noNEXT_PUBLIC_API_URLset, noNODE_ENV=production) and confirm local development is unaffected, per the issue's own "Done when" criterion.