fix: three production bugs the fixture work surfaced - #2820
Open
kushagrasarathe wants to merge 4 commits into
Open
fix: three production bugs the fixture work surfaced#2820kushagrasarathe wants to merge 4 commits into
kushagrasarathe wants to merge 4 commits into
Conversation
The intent to block internal tooling on production has been in the tree since February. It was written in the wrong place, so it never worked. `(mobile-ui)/dev/layout.tsx` called notFound() for every route outside a small allowlist. Inside a route group that renders the not-found UI and still answers 200, so peanut.me served the page and its chunks to anyone who asked. The pages under `src/app/dev/` sat outside that group and had no gate at all: `/dev/kyc-flows` rendered internal KYC flow diagrams to anonymous visitors. The check now runs in the proxy, which sees both route groups, returns a real status, and covers routes nobody has written yet. Measured on a production build: 17 dev routes went 200 to 404, and full-graph, payment-graph and safe-area stayed 200. The allowlist is the one the layout already had — nothing added, nothing removed. The host, not the build, decides. Blocking on the dev-and-preview flag alone would have taken /dev off staging, which 6830d7f turned on deliberately. peanut.me is the only host that blocks, and a build with no base URL reads as peanut.me, so the gate fails closed. The layout keeps its check. The native build is a static export with no proxy, so there it is the only gate; both now read the same predicate. The test walks src/app for every page.tsx, so a dev page added tomorrow is covered without anyone remembering to add it. Still open, out of scope here: the /dev chunks stay in the production bundle. The proxy blocks the routes, not the JavaScript.
…sers to signup When the user fetch failed, the error screen painted and then the app threw the person at /setup — the signup flow. During an outage that reads as "you have been logged out", which is the worst thing to tell someone whose money is behind the login. The redirect effect matched on "no user" and stopped there. No user has two causes, and the data already told them apart: the user query returns null for a 401 and throws for a 5xx or a network failure. So an error means the backend is down, not that the person is signed out. Reading the two apart is one condition. The 3 second hard-nav fallback made it worse. The soft redirect ran, the error screen rendered, and three seconds later the fallback replaced the location anyway. React clears that timer when the dependency changes; the spec asserts no timer survives. Measured before: error screen at 3128ms, gone to /setup at 7054ms. After: the screen holds past 11s, the URL never moves. A logged-out visitor still reaches /setup on the same timing as before, and a healthy load is unchanged. Jest could not import the layout at all, because it pulls in globals.css and there was no css mapper. Added one line; jest-transform-stub was already a dependency. Left alone: `isRedirecting.current` never resets, so a later legitimate redirect in the same mount is suppressed. Pre-existing, not reachable from this path.
…orever `/rewards` waited on the mascot loader with no way out when `GET /points` failed. The error branch below the loader was unreachable for that path. Two mistakes met. `pointsApi.getTierInfo` catches its own failure and resolves with `data: null`, so react-query never reports an error and never retries. The loading guard then read `!tierInfo?.data`, which stays true forever once the call has failed. The invites call throws properly, which is why the error branch looked live in review. The guards now split three states instead of two. A disabled query reports `isLoading` false while both queries wait on `user`, so `isLoading` cannot tell "not started" from "settled and empty". `isPending` can, and past that guard the request has settled, so missing data means it failed. The screen was only correct before because a parent covers for it: the (mobile-ui) layout holds every protected route on its own loader until `user` is truthy, using a different predicate than the queries use. The component is now correct on its own terms, and the spec pins it. The spec keys its EmptyState stub on the title. That component is shared — the page uses it for this failure and, further down, for "no invites yet" — so a bare test id matches both and the success case reads as a failure. Left alone, both worth their own change: a failed invites call still blanks the whole screen instead of dropping one section, and points failures never reach Sentry, which is why this survived.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
The dev-route lockdown missed src/app/dev/* on the production native build — a static export where proxy.ts never runs, so kyc-flows and loading-words answered in the app. Both route groups now share one gate: shouldBlockDevRoute keeps the flag and the predicate together so a future call site cannot use half of it. Also from review: the gate test now routes ALLOWED pages through the proxy instead of assuming they answer; the rewards error branch logs the settled response instead of a contentless null; the routes.ts comment describes the mechanism this PR actually shipped.
Contributor
Author
/code-review pass — 2026-08-26Fixed in 213588f (typecheck + 3,639 unit tests green):
Declined with reason: DEV_ROUTES_ENABLED inlines its own prod-domain literal (a 5th copy in the repo) because general.consts pulls viem into the edge bundle — consolidating means moving BASE_URL out of general.consts, out of scope here. The review's encoded-path bypass note (%64ev) rests on Next normalizing before matching, which current releases do; an integration probe on a prod build would pin it, also out of scope. The three original fixes themselves verified clean — no regression found in their happy/error paths. |
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
1 participant
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.
Summary
Three production bugs, found while building the fixture harness in #2819. None was caught by a test, by Sentry, or by review — each surfaced from looking at a real screen or a real log.
Kept apart from #2819 on purpose: that PR is tooling with no production behaviour, this one is ~130 lines that change what users see. If one of these is wrong during launch week, reverting it should not mean reverting a harness.
Base:
feat/design-system(PR #2813).1. Internal dev tooling answered 200 on peanut.me
The intent to block
/devon production has been in the tree since February (6830d7f3e). It was written in the wrong place, so it never worked.(mobile-ui)/dev/layout.tsxcallednotFound()for everything outside a small allowlist. Inside a route group that renders the not-found UI and still answers 200, so peanut.me served the page and its chunks to anyone who asked.Worse, the pages under
src/app/dev/sit outside that group and had no gate at all./dev/kyc-flowsrendered internal KYC flow diagrams to anonymous visitors on production.The check now runs in the proxy, which sees both route groups, returns a real status, and covers routes nobody has written yet. Measured on a production build: 17 dev routes went 200 → 404;
full-graph,payment-graphandsafe-areastayed 200. The allowlist is the one the layout already had — nothing added, nothing removed.The host decides, not the build. Blocking on the dev-and-preview flag alone would have taken
/devoff staging, which6830d7f3eenabled deliberately. peanut.me is the only host that blocks, and a build with no base URL reads as peanut.me, so the gate fails closed. The layout keeps its check because the native build is a static export with no proxy.A test walks
src/appfor everypage.tsx, so a dev page added tomorrow is covered without anyone remembering.2. A backend outage sent signed-in users to the signup flow
(mobile-ui)/layout.tsxrendersBackendErrorScreenwhen the user fetch fails. An effect earlier in the same file matched the same state with no error guard, calledrouter.replace('/setup'), and armed a 3-second hard-nav fallback behind it.Instrumented first paints, before:
During an outage that reads as "you have been logged out", which is the worst thing to tell someone whose money is behind the login.
"No user" has two causes and the data already told them apart: the user query returns null for a 401 and throws for a 5xx. One condition. After: the screen holds past 11 seconds and the URL never moves. A logged-out visitor still reaches
/setupon identical timing.Jest could not import the layout at all — no CSS mapper — so this had never been testable. One line added;
jest-transform-stubwas already a dependency.3. A failed points fetch spun forever
/rewardssat on the mascot loader with no way out whenGET /pointsfailed. The error branch below the loader was unreachable for that path.pointsApi.getTierInfocatches its own failure and resolves withdata: null, so react-query never reports an error and never retries. The loading guard read!tierInfo?.data, which stays true forever once the call has failed. The invites call throws properly, which is why the error branch looked live in review.The guards now split three states instead of two. A disabled query reports
isLoadingfalse while both queries wait onuser, soisLoadingcannot tell "not started" from "settled and empty".isPendingcan.The screen was only correct before because a parent covers for it, using a different predicate than the queries use. It is now correct on its own terms.
Verified
typecheckclean ·npx jest304 suites, 3639 tests ·prettierclean ·ds-lint --checkno metric increasedFound and deliberately not fixed
captureException; rewards onlyconsole.errors, andgetTierInfoswallows before even that. This is why bug 3 survivedisRedirecting.currentnever resets, so a later legitimate redirect in the same mount is suppressed/devchunks still reach production bundles. The proxy blocks the routes, not the JavaScript — that needs a build rule over every/devroute