Skip to content

Commit 5412f70

Browse files
committed
refactor(favicon): eliminate duplicate icon-set map, add getDeploymentEnv() test coverage
Self-review pass, not a review-comment fix: - app/api/favicon/route.ts maintained its own env-to-svg-path map (FAVICON_DESTINATIONS) duplicating ICON_SETS in ee/whitelabeling/metadata.ts. The two could silently drift apart if an asset path ever changed in only one place. Exported ICON_SETS from metadata.ts (and the ee/whitelabeling barrel) and had the route reuse it directly instead. - Added lib/core/config/env-flags.test.ts covering getDeploymentEnv()'s resolution chain: each real AWS Secrets Manager value (dev/staging/prod), the APPCONFIG_ENVIRONMENT production->prod mapping, the NODE_ENV fallback, precedence ordering between the three signals, and an unrecognized-value case bucketing safely to development rather than throwing. This logic went through three rounds of review fixes: worth a regression net now that it's settled.
1 parent 1833d92 commit 5412f70

4 files changed

Lines changed: 91 additions & 16 deletions

File tree

apps/sim/app/api/favicon/route.ts

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,10 @@
11
import { type NextRequest, NextResponse } from 'next/server'
22
import { getDeploymentEnv } from '@/lib/core/config/env-flags'
33
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
4-
import { getBrandConfig } from '@/ee/whitelabeling/branding'
4+
import { getBrandConfig, ICON_SETS } from '@/ee/whitelabeling'
55

66
export const dynamic = 'force-dynamic'
77

8-
const FAVICON_DESTINATIONS: Record<ReturnType<typeof getDeploymentEnv>, string> = {
9-
development: '/icon-dev.svg',
10-
staging: '/icon-staging.svg',
11-
production: '/icon.svg',
12-
}
13-
148
/**
159
* Redirect target for the legacy `/favicon.ico` path (rewritten here in
1610
* `next.config.ts`), resolved per-request rather than baked into
@@ -23,12 +17,14 @@ const FAVICON_DESTINATIONS: Record<ReturnType<typeof getDeploymentEnv>, string>
2317
* that shared build and would freeze whichever tier happened to be active
2418
* then (never the tier the container actually ends up running as).
2519
*
26-
* A whitelabeled deployment's own favicon always wins here too, matching
27-
* `generateBrandedMetadata()` (`ee/whitelabeling/metadata.ts`) — a tenant on
28-
* a dev/staging environment should never see Sim's internal tinted mark.
20+
* Reuses {@link ICON_SETS} from `ee/whitelabeling/metadata.ts` rather than
21+
* maintaining a second env-to-path map here, so the two can't drift apart. A
22+
* whitelabeled deployment's own favicon always wins here too, matching
23+
* `generateBrandedMetadata()` — a tenant on a dev/staging environment should
24+
* never see Sim's internal tinted mark.
2925
*/
3026
export const GET = withRouteHandler(async (request: NextRequest) => {
3127
const brand = getBrandConfig()
32-
const destination = brand.faviconUrl || FAVICON_DESTINATIONS[getDeploymentEnv()]
28+
const destination = brand.faviconUrl || ICON_SETS[getDeploymentEnv()].svg
3329
return NextResponse.redirect(new URL(destination, request.url))
3430
})

apps/sim/ee/whitelabeling/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,5 @@ export type { OrganizationWhitelabelSettings } from '@/lib/branding/types'
22
export type { BrandConfig, ThemeColors } from './branding'
33
export { getBrandConfig, useBrandConfig } from './branding'
44
export { generateThemeCSS } from './inject-theme'
5-
export { generateBrandedMetadata, generateStructuredData } from './metadata'
5+
export { generateBrandedMetadata, generateStructuredData, ICON_SETS } from './metadata'
66
export { generateOrgThemeCSS, mergeOrgBrandConfig } from './org-branding-utils'

apps/sim/ee/whitelabeling/metadata.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,12 @@ interface FaviconSet {
1616
* Same "sim" wordmark per {@link getDeploymentEnv}, background color only —
1717
* so a dev/staging tab is never mistaken for prod at a glance. Ignored
1818
* entirely when `brand.faviconUrl` is set (a whitelabeled deployment's own
19-
* favicon always wins, regardless of which tier it's running on). Kept in
20-
* sync with `FAVICON_ICO_DESTINATIONS` in `next.config.ts`, which handles
21-
* the legacy `/favicon.ico` path the same way.
19+
* favicon always wins, regardless of which tier it's running on). Exported
20+
* for `app/api/favicon/route.ts` (the legacy `/favicon.ico` redirect target)
21+
* to reuse `.svg` from, so the two never drift out of sync the way two
22+
* separately-maintained copies could.
2223
*/
23-
const ICON_SETS: Record<ReturnType<typeof getDeploymentEnv>, FaviconSet> = {
24+
export const ICON_SETS: Record<ReturnType<typeof getDeploymentEnv>, FaviconSet> = {
2425
development: {
2526
svg: '/icon-dev.svg',
2627
favicon16: '/favicon-dev/favicon-16x16.png',
Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { envRef } = vi.hoisted(() => ({
7+
envRef: { NODE_ENV: 'development' as string | undefined },
8+
}))
9+
10+
vi.mock('@/lib/core/config/env', () => ({
11+
get env() {
12+
return envRef
13+
},
14+
getEnv: (key: string) => process.env[key],
15+
isFalsy: (v: unknown) => v === false || v === 'false' || v === '0',
16+
isTruthy: (v: unknown) => v === true || v === 'true' || v === '1',
17+
}))
18+
19+
import { getDeploymentEnv } from '@/lib/core/config/env-flags'
20+
21+
describe('getDeploymentEnv', () => {
22+
const ENV_KEYS = [
23+
'OTEL_DEPLOYMENT_ENVIRONMENT',
24+
'DEPLOYMENT_ENVIRONMENT',
25+
'APPCONFIG_ENVIRONMENT',
26+
]
27+
28+
beforeEach(() => {
29+
for (const key of ENV_KEYS) delete process.env[key]
30+
envRef.NODE_ENV = 'development'
31+
})
32+
33+
afterEach(() => {
34+
for (const key of ENV_KEYS) delete process.env[key]
35+
})
36+
37+
it('resolves the dev tier from OTEL_DEPLOYMENT_ENVIRONMENT=dev', () => {
38+
process.env.OTEL_DEPLOYMENT_ENVIRONMENT = 'dev'
39+
expect(getDeploymentEnv()).toBe('development')
40+
})
41+
42+
it('resolves the staging tier from OTEL_DEPLOYMENT_ENVIRONMENT=staging', () => {
43+
process.env.OTEL_DEPLOYMENT_ENVIRONMENT = 'staging'
44+
expect(getDeploymentEnv()).toBe('staging')
45+
})
46+
47+
it('resolves the production tier from OTEL_DEPLOYMENT_ENVIRONMENT=prod', () => {
48+
process.env.OTEL_DEPLOYMENT_ENVIRONMENT = 'prod'
49+
expect(getDeploymentEnv()).toBe('production')
50+
})
51+
52+
it('maps APPCONFIG_ENVIRONMENT=production to the production tier when OTEL var is unset', () => {
53+
process.env.APPCONFIG_ENVIRONMENT = 'production'
54+
expect(getDeploymentEnv()).toBe('production')
55+
})
56+
57+
it('falls back to NODE_ENV when no deployment-tier env var is set', () => {
58+
envRef.NODE_ENV = 'production'
59+
expect(getDeploymentEnv()).toBe('production')
60+
})
61+
62+
it('defaults to development when nothing is set at all', () => {
63+
envRef.NODE_ENV = undefined
64+
expect(getDeploymentEnv()).toBe('development')
65+
})
66+
67+
it('prefers OTEL_DEPLOYMENT_ENVIRONMENT over DEPLOYMENT_ENVIRONMENT and APPCONFIG_ENVIRONMENT', () => {
68+
process.env.OTEL_DEPLOYMENT_ENVIRONMENT = 'staging'
69+
process.env.DEPLOYMENT_ENVIRONMENT = 'prod'
70+
process.env.APPCONFIG_ENVIRONMENT = 'production'
71+
expect(getDeploymentEnv()).toBe('staging')
72+
})
73+
74+
it('buckets an unrecognized tier value to development rather than throwing', () => {
75+
process.env.OTEL_DEPLOYMENT_ENVIRONMENT = 'some-future-tier'
76+
expect(getDeploymentEnv()).toBe('development')
77+
})
78+
})

0 commit comments

Comments
 (0)