Skip to content

Commit 1e42a73

Browse files
committed
fix(favicon): revert marketing-ISR-breaking force-dynamic, fix manifest env-tint
Addresses the fourth round of review findings on PR #5588: - Cursor (High, confirmed via testing, not just code review): the previous commit's `export const dynamic = 'force-dynamic'` on app/layout.tsx would have forced the entire marketing site ((landing) route group - blog/library posts, changelog, solutions pages, all with their own `revalidate` exports) into per-request rendering, breaking their Lighthouse/LCP budget. My earlier "verified via build, no regression" claim was itself wrong: my local build masked this because NEXT_PUBLIC_APP_URL=localhost makes isHosted() false, which makes app/layout.tsx render next-runtime-env's RuntimePublicEnvScript - which unconditionally calls unstable_noStore() - already forcing marketing pages dynamic before my change even ran, for a completely unrelated pre-existing reason. Rebuilt with NEXT_PUBLIC_APP_URL=https://www.sim.ai (matching real hosted deployments, isHosted() true, the safe script path) to get an unmasked answer: confirmed force-dynamic was the actual regression (marketing pages went from ● to ƒ), and confirmed the revert restores them to ○ with their normal 1h revalidate window. Documented the accepted tradeoff instead: marketing-page favicon metadata may lag by up to one revalidate window after a deploy (bounded, self-healing at the next ISR regeneration, which runs in the live server process after bootstrap.ts has loaded secrets) - the actual workspace app (this feature's real audience) is already fully dynamic today (session/auth), so it needs no explicit marker. - Cursor (Medium): app/manifest.ts's non-whitelabeled branch still hardcoded production favicon paths instead of ICON_SETS[getDeploymentEnv()], so PWA install/home-screen icons stayed production-tinted while the HTML favicon was correctly env-tinted. Now uses the same per-tier set.
1 parent 37a3b05 commit 1e42a73

2 files changed

Lines changed: 27 additions & 40 deletions

File tree

apps/sim/app/layout.tsx

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -24,33 +24,28 @@ export const viewport: Viewport = {
2424
],
2525
}
2626

27-
/**
28-
* Forces every route under this layout to render dynamically. Every real page
29-
* in this app already renders dynamically today (session/workspace state), so
30-
* this changes nothing currently — but it's the difference between "happens
31-
* to be true" and "guaranteed": {@link generateBrandedMetadata} reads
32-
* `getDeploymentEnv()` for the favicon set, which depends on `process.env`
33-
* being populated by `bootstrap.ts`'s runtime secrets load, and
34-
* `generateMetadata()` alone does not force per-request evaluation — Next
35-
* still prerenders it at build time for any route that's otherwise static or
36-
* ISR-eligible. Without this, a future static/ISR page added under this
37-
* layout would silently get build-time (production-tier) favicon metadata
38-
* baked in, with no error. Matches the `force-dynamic` already declared on
39-
* `app/manifest.ts` and `app/api/favicon/route.ts`.
40-
*/
41-
export const dynamic = 'force-dynamic'
42-
4327
/**
4428
* Not a static `export const metadata` object: {@link generateBrandedMetadata}
4529
* reads `getDeploymentEnv()` for the favicon set, which depends on
4630
* `process.env` being populated by `bootstrap.ts`'s runtime secrets load. A
4731
* static object is resolved once at module-evaluation time and Next.js treats
48-
* it as truly constant (eligible for build-time resolution on any
49-
* statically-rendered route sharing this layout) — `generateMetadata()`
50-
* forces the same per-request resolution `app/api/favicon/route.ts` already
51-
* uses. Paired with the `dynamic = 'force-dynamic'` export above, which
52-
* guarantees that resolution actually happens per-request rather than only
53-
* "happening to" because every current page is otherwise dynamic.
32+
* it as truly constant (eligible for build-time resolution) —
33+
* `generateMetadata()` at least makes resolution part of the page's own
34+
* render rather than a frozen module-level value.
35+
*
36+
* Deliberately NOT paired with `export const dynamic = 'force-dynamic'` on
37+
* this (root) layout: every `(landing)` marketing page sets its own
38+
* `revalidate` and relies on being statically generated /
39+
* incrementally revalidated for its Lighthouse/LCP budget (see
40+
* `app/(landing)/CLAUDE.md`) — forcing the root layout dynamic would force
41+
* the entire marketing site dynamic too, which is a much larger regression
42+
* than a favicon staying on its previous tier for up to one revalidate
43+
* window after a deploy. The actual workspace app (this feature's real
44+
* audience) is already fully dynamic today (session/auth), so
45+
* `generateMetadata()` already resolves per-request there without needing
46+
* the marker. Marketing pages accept bounded staleness instead: the favicon
47+
* self-corrects at the next ISR regeneration, which runs in the live server
48+
* process (after `bootstrap.ts` has loaded secrets), not at build time.
5449
*/
5550
export function generateMetadata(): Metadata {
5651
return generateBrandedMetadata()

apps/sim/app/manifest.ts

Lines changed: 10 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
import type { MetadataRoute } from 'next'
2-
import { getBrandConfig } from '@/ee/whitelabeling'
2+
import { getDeploymentEnv } from '@/lib/core/config/env-flags'
3+
import { getBrandConfig, ICON_SETS } from '@/ee/whitelabeling'
34

45
export const dynamic = 'force-dynamic'
56

67
export default function manifest(): MetadataRoute.Manifest {
78
const brand = getBrandConfig()
9+
const icons = ICON_SETS[getDeploymentEnv()]
810

911
return {
1012
name:
@@ -21,26 +23,16 @@ export default function manifest(): MetadataRoute.Manifest {
2123
theme_color: brand.theme?.primaryColor || '#33C482',
2224
orientation: 'portrait-primary',
2325
// A whitelabeled deployment's install/home-screen icon should be the
24-
// tenant's own brand, not Sim's — same precedence as the HTML favicon
25-
// slots in generateBrandedMetadata() (ee/whitelabeling/metadata.ts).
26+
// tenant's own brand, not Sim's; otherwise it's the same per-environment
27+
// set as the HTML favicon slots in generateBrandedMetadata()
28+
// (ee/whitelabeling/metadata.ts), so a staging/dev PWA install doesn't
29+
// look like prod either.
2630
icons: brand.faviconUrl
2731
? [{ src: brand.faviconUrl, sizes: 'any', type: 'image/png' }]
2832
: [
29-
{
30-
src: '/favicon/android-chrome-192x192.png',
31-
sizes: '192x192',
32-
type: 'image/png',
33-
},
34-
{
35-
src: '/favicon/android-chrome-512x512.png',
36-
sizes: '512x512',
37-
type: 'image/png',
38-
},
39-
{
40-
src: '/favicon/apple-touch-icon.png',
41-
sizes: '180x180',
42-
type: 'image/png',
43-
},
33+
{ src: icons.android192, sizes: '192x192', type: 'image/png' },
34+
{ src: icons.android512, sizes: '512x512', type: 'image/png' },
35+
{ src: icons.apple, sizes: '180x180', type: 'image/png' },
4436
],
4537
categories: ['productivity', 'developer', 'business'],
4638
shortcuts: [

0 commit comments

Comments
 (0)