Re-apply tenant lookup refactor with routing fix#949
Re-apply tenant lookup refactor with routing fix#949rchlfryn merged 14 commits intorevert-926-revert-1.7.0from
Conversation
|
Preview deployment: https://fryanxrevert-926-revert-1x7x0.preview.avy-fx.org |
be served on-demand
for graceful handling when DB data is missing.
b5dcbfc to
8323064
Compare
8323064 to
fbc5a89
Compare
busbyk
left a comment
There was a problem hiding this comment.
Couple error/404 scenarios here:
- valid tenant slug but tenant does not exist in payload
- should: 404 and show non-center scoped 404
- this branch/PR: notFound thrown in layout.tsx RSC but we don't have a custom not-found page one level up so next.js 404 page shown (black page with white 404 text)
- invalid tenant slug
- should: 404 and show non-center scoped 404
- this branch/PR: shows src/app/(frontend)/page.tsx -- I wonder if this is because of the dynamicParams change. But it might just be related to the middleware. I didn't get a chance to test on the target branch/PR.
Custom domain routing for both src/app/(frontend)/[center]/[slug]/page.tsx and src/app/(frontend)/[center]/[...segments]/page.tsx seem to work in this PR so that's good.
| if (!settings) { | ||
| return {} | ||
| } | ||
|
|
||
| const tenant = await resolveTenant(settings.tenant, { | ||
| select: { | ||
| name: true, | ||
| }, | ||
| }) | ||
|
|
||
| if (!tenant) { | ||
| return {} | ||
| } |
There was a problem hiding this comment.
Was this throwing an error without these empty object returns?
There was a problem hiding this comment.
Not in practice but it could happen in theory - these are added now that dynamicParams = true
With dynamicParams = false, Next.js 404s before reaching the route for slugs not in generateStaticParams, so generateMetadata was never called for invalid slugs.
With dynamicParams = true, generateMetadata can be called for any slug — including ones without settings/tenant in the DB. The layout's notFound() should prevent this, but Next.js may call generateMetadata in parallel with the layout, so these guards prevent a crash if notFound() hasn't fired yet.
There was a problem hiding this comment.
Yea good call that the RSC and generateMetadata are called in parallel. Would it make more sense to return some basic error state metadata instead of an empty object like:
{
title: 'Not found',
description: '...' // not sure what the description would be - we could omit this
}
It sounds like since the not-found page is nested within this layout, this metadata will be used if that's rendered.
There was a problem hiding this comment.
Good point! I'll update the fallback to return { title: 'Not found' } — since it's more useful metadata than an empty object, especially since the not-found page will inherit it. I'll leave out description since there's nothing meaningful to say there.
|
Two 404 issues you noted:
When Fix: Add
I think this is a middleware gap. When an unknown subdomain like Fix: Return a 404 from middleware when a subdomain doesn't match any known tenant. |
…gate to root domain even on invalid subdomains or custom domains
…e (frontend) route group is just for folder organization so (frontend)/not-found is at top level
…page.tsx since it's called in parallel with layout.tsx
…t to true for proper 404 handling
…etion string change for clarity + adding similar logic for matching custom domain for center not configured for production
|
@rchlfryn I pushed a few changes. Tried to be descriptive enough in my commit messages but let me know if any of those need further explanation. I'm happy to merge this into #947 if you think my commits look good. I'm interested in removing custom domain from the db and just depending on the custom domain configured in |
|
@busbyk I think changes look good! I will go ahead and squash and merge this into the revert and you can make a PR off of that. |
Description
Re-applies the tenant lookup refactor from #897 (which was reverted in #926). This replaces the dynamic Edge Config / API-based tenant resolution with a hardcoded
AVALANCHE_CENTERSlist as the single source of truth for valid tenant slugs, making middleware synchronous and deterministic.Also fixes a routing issue where pages were falling through to
[...segmentsNotFound]in preview.Why
dynamicParams = true?The routing works in two phases with different capabilities:
With
dynamicParams = false, there's a rigid third gatekeeper between them: the build-timegenerateStaticParams()output. That list must perfectly match what middleware sends — if it doesn't, for any reason, routes silently fall through to the catch-all instead of rendering the appropriate not-found page.With
dynamicParams = true, that rigid middle layer is removed. Middleware handles "is this slug valid?" and the layout handles "does this center have data?" Each layer does what it's best positioned to do with the information it has access to.generateStaticParams()still pre-renders centers in the DB — that's a performance optimization, not an access control mechanism.Why
notFound()instead ofinvariant?invariantthrows a regular error → 500-style error page.notFound()throws a Next.js-specific error → proper 404 page. WithdynamicParams = true, reaching the layout with a center that has no DB data is now an expected path (not a bug), so a 404 is the correct response.Reverts #926
Related Issues
#897
Key Changes
Newly added to fix false 404s
dynamicParams = true: Centers in the DB are pre-rendered at build time; others are served on-demand with graceful 404 handling d370ef0invariant→notFound(): Proper 404 pages instead of 500-style crashes when a center lacks DB data c34ee93How to test
nwac.localhost) — should render normallyAVALANCHE_CENTERSbut not in the DB — should show the not-found page, not crash