Revert "Drive env branding from extraBodyClass alone (#8755)"

This reverts commit 13358939ce.

lib/app/rendering.ts

@@ -26,6 +26,7 @@ import express, {Request, Response} from 'express';
 import _ from 'underscore';
 
 import type {Options as FrontendOptions} from '../../static/options.interfaces.js';
+import {AppArguments} from '../app.interfaces.js';
 import * as normalizer from '../clientstate-normalizer.js';
 import {GoldenLayoutRootStruct} from '../clientstate-normalizer.js';
 import type {ShortLinkMetaData} from '../handlers/handler.interfaces.js';
@@ -39,7 +40,7 @@ import {
     ServerDependencies,
     ServerOptions,
 } from './server.interfaces.js';
-import {getFaviconFilename, getLogoOverlayFilename} from './static-assets.js';
+import {getFaviconFilename} from './static-assets.js';
 import {isMobileViewer} from './url-handlers.js';
 
 // Heavy fields (compilers, libs, tools, ...) are absent on purpose — they are lazy-loaded.
@@ -66,10 +67,19 @@ const FRONTEND_INLINE_OPTION_KEYS = [
     'explainApiEndpoint',
 ] as const satisfies readonly (keyof ClientOptionsType & keyof FrontendOptions)[];
 
+/**
+ * Create rendering-related functions
+ * @param pugRequireHandler - Handler for Pug requires
+ * @param options - Server options
+ * @param dependencies - Server dependencies
+ * @param appArgs - App arguments
+ * @returns Rendering functions
+ */
 export function createRenderHandlers(
     pugRequireHandler: PugRequireHandler,
     options: ServerOptions,
     dependencies: ServerDependencies,
+    appArgs: AppArguments,
 ): {
     renderConfig: RenderConfigFunction;
     renderGoldenLayout: RenderGoldenLayoutHandler;
@@ -114,8 +124,7 @@ export function createRenderHandlers(
         options.storageSolution = options.storageSolution || storageSolution;
         options.require = pugRequireHandler;
         options.sponsors = sponsorConfig;
-        options.faviconFilename = getFaviconFilename(extraBodyClass);
-        options.logoOverlayFilename = getLogoOverlayFilename(extraBodyClass);
+        options.faviconFilename = getFaviconFilename(appArgs.devMode, appArgs.env);
         return options;
     };
 

lib/app/server-config.ts

@@ -137,10 +137,14 @@ export function setupLoggingMiddleware(isDevMode: boolean, router: Router): void
     );
 }
 
-const FAVICON_PATH_RE = /^\/favicon(-[a-z0-9-]+)?\.ico$/;
-
 function isFaviconRequest(req: Request): boolean {
-    return FAVICON_PATH_RE.test(req.path);
+    return [
+        'favicon.ico',
+        'favicon-beta.ico',
+        'favicon-dev.ico',
+        'favicon-staging.ico',
+        'favicon-suspend.ico',
+    ].includes(req.path);
 }
 
 /**

lib/app/server.interfaces.ts

@@ -59,7 +59,6 @@ export interface RenderConfig extends PugOptions {
     config?: GoldenLayoutRootStruct;
     metadata?: ShortLinkMetaData;
     faviconFilename: string;
-    logoOverlayFilename?: string;
     storedStateId?: string | false;
     require?: PugRequireHandler;
     sponsors?: Sponsors;

lib/app/server.ts

@@ -29,12 +29,7 @@ import {logger} from '../logger.js';
 import {createRenderHandlers} from './rendering.js';
 import {ServerDependencies, ServerOptions, WebServerResult} from './server.interfaces.js';
 import {setupBaseServerConfig, setupBasicRoutes, setupLoggingMiddleware} from './server-config.js';
-import {
-    getBrandingAssetDir,
-    setupStaticMiddleware,
-    setupWebPackDevMiddleware,
-    validateBrandingAssets,
-} from './static-assets.js';
+import {setupStaticMiddleware, setupWebPackDevMiddleware} from './static-assets.js';
 
 export {startListening} from './server-listening.js';
 export {isMobileViewer} from './url-handlers.js';
@@ -66,12 +61,11 @@ export async function setupWebServer(
         pugRequireHandler = path => `${options.staticRoot}/${path}`;
     }
 
-    await validateBrandingAssets(getBrandingAssetDir(appArgs.devMode, options.staticPath), options.extraBodyClass);
-
     const {renderConfig, renderGoldenLayout, embeddedHandler} = createRenderHandlers(
         pugRequireHandler,
         options,
         dependencies,
+        appArgs,
     );
 
     // Add healthcheck before logging middleware to prevent excessive log entries

lib/app/static-assets.ts

@@ -31,7 +31,6 @@ import urljoin from 'url-join';
 
 import {unwrap} from '../assert.js';
 import {logger} from '../logger.js';
-import {resolvePathFromAppRoot} from '../utils.js';
 import {PugRequireHandler, ServerOptions} from './server.interfaces.js';
 
 /**
@@ -109,41 +108,21 @@ export async function setupStaticMiddleware(options: ServerOptions, router: Rout
     return createDefaultPugRequireHandler(options.staticRoot, staticManifest);
 }
 
-export function getFaviconFilename(extraBodyClass: string): string {
-    return extraBodyClass ? `favicon-${extraBodyClass}.ico` : 'favicon.ico';
-}
-
-export function getLogoOverlayFilename(extraBodyClass: string): string | undefined {
-    return extraBodyClass ? `site-logo-${extraBodyClass}.svg` : undefined;
-}
-
 /**
- * Resolves the directory branding assets are actually served from. In dev mode the webpack
- * dev middleware serves them from public/ (they aren't on disk in staticPath); in production
- * webpack has copied public/ into staticPath (dist/static), so we validate there.
+ * Gets the appropriate favicon filename based on the environment
+ * @param isDevMode - Whether the app is running in development mode
+ * @param env - The environment names array
+ * @returns The favicon filename to use
  */
-export function getBrandingAssetDir(devMode: boolean, staticPath: string): string {
-    return devMode ? resolvePathFromAppRoot('public') : staticPath;
-}
-
-export async function validateBrandingAssets(staticPath: string, extraBodyClass: string): Promise<void> {
-    if (!extraBodyClass) return;
-    const required = [getFaviconFilename(extraBodyClass), getLogoOverlayFilename(extraBodyClass)].filter(
-        (f): f is string => f !== undefined,
-    );
-    const missing: string[] = [];
-    for (const filename of required) {
-        try {
-            await fs.access(path.join(staticPath, filename));
-        } catch (e: unknown) {
-            const err = e as NodeJS.ErrnoException;
-            if (err.code === 'ENOENT') missing.push(filename);
-            else throw err;
-        }
+export function getFaviconFilename(isDevMode: boolean, env?: string[]): string {
+    if (isDevMode) {
+        return 'favicon-dev.ico';
     }
-    if (missing.length > 0) {
-        throw new Error(
-            `Missing branding assets for extraBodyClass='${extraBodyClass}' in ${staticPath}: ${missing.join(', ')}`,
-        );
+    if (env?.includes('beta')) {
+        return 'favicon-beta.ico';
     }
+    if (env?.includes('staging')) {
+        return 'favicon-staging.ico';
+    }
+    return 'favicon.ico';
 }

public/favicon-winprod.ico

@@ -1 +0,0 @@
-favicon.ico

public/favicon-winstaging.ico

@@ -1 +0,0 @@
-favicon-staging.ico

public/favicon-wintest.ico

@@ -1 +0,0 @@
-favicon-dev.ico

public/site-logo-winprod.svg

@@ -1 +0,0 @@
-site-logo.svg

public/site-logo-winstaging.svg

@@ -1 +0,0 @@
-site-logo-staging.svg

public/site-logo-wintest.svg

@@ -1 +0,0 @@
-site-logo-dev.svg

test/app/rendering-tests.ts

@@ -27,6 +27,7 @@ import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest';
 
 import {createRenderHandlers} from '../../lib/app/rendering.js';
 import {PugRequireHandler, ServerDependencies, ServerOptions} from '../../lib/app/server.interfaces.js';
+import {AppArguments} from '../../lib/app.interfaces.js';
 
 // Mock dependencies
 vi.mock('../../lib/app/url-handlers.js', () => ({
@@ -74,6 +75,31 @@ describe('Rendering Module', () => {
         let mockPugRequireHandler: PugRequireHandler;
         let mockOptions: ServerOptions;
         let mockDependencies: ServerDependencies;
+        const mockAppArgs: AppArguments = {
+            rootDir: '/test/root',
+            env: ['test'],
+            port: 10240,
+            gitReleaseName: 'test-release',
+            releaseBuildNumber: '123',
+            wantedLanguages: ['c++'],
+            doCache: true,
+            fetchCompilersFromRemote: true,
+            ensureNoCompilerClash: false,
+            prediscovered: undefined,
+            discoveryOnly: undefined,
+            staticPath: undefined,
+            metricsPort: undefined,
+            useLocalProps: true,
+            propDebug: false,
+            tmpDir: undefined,
+            isWsl: false,
+            devMode: false,
+            loggingOptions: {
+                debug: false,
+                suppressConsoleLog: false,
+                paperTrailIdentifier: 'test',
+            },
+        };
 
         beforeEach(() => {
             mockPugRequireHandler = vi.fn((file: string) => `/static/${file}`);
@@ -108,7 +134,12 @@ describe('Rendering Module', () => {
         });
 
         it('should create renderConfig function that correctly merges options', () => {
-            const {renderConfig} = createRenderHandlers(mockPugRequireHandler, mockOptions, mockDependencies);
+            const {renderConfig} = createRenderHandlers(
+                mockPugRequireHandler,
+                mockOptions,
+                mockDependencies,
+                mockAppArgs,
+            );
 
             const result = renderConfig({userOption: 'value'});
 
@@ -130,7 +161,12 @@ describe('Rendering Module', () => {
         });
 
         it('should set extraBodyClass to "embedded" when embedded is true', () => {
-            const {renderConfig} = createRenderHandlers(mockPugRequireHandler, mockOptions, mockDependencies);
+            const {renderConfig} = createRenderHandlers(
+                mockPugRequireHandler,
+                mockOptions,
+                mockDependencies,
+                mockAppArgs,
+            );
 
             const result = renderConfig({embedded: true});
 
@@ -138,7 +174,12 @@ describe('Rendering Module', () => {
         });
 
         it('should filter URL options to only allow whitelisted properties', () => {
-            const {renderConfig} = createRenderHandlers(mockPugRequireHandler, mockOptions, mockDependencies);
+            const {renderConfig} = createRenderHandlers(
+                mockPugRequireHandler,
+                mockOptions,
+                mockDependencies,
+                mockAppArgs,
+            );
 
             const urlOptions = {
                 readOnly: 'true',
@@ -166,7 +207,12 @@ describe('Rendering Module', () => {
         it('should generate slides for mobile viewer', () => {
             // Skip complex mock setup due to TypeScript issues
             // Just verify we can call it without error
-            const {renderConfig} = createRenderHandlers(mockPugRequireHandler, mockOptions, mockDependencies);
+            const {renderConfig} = createRenderHandlers(
+                mockPugRequireHandler,
+                mockOptions,
+                mockDependencies,
+                mockAppArgs,
+            );
 
             // This test is simplified to avoid complex mocking issues
             expect(renderConfig).toBeDefined();
@@ -174,7 +220,12 @@ describe('Rendering Module', () => {
         });
 
         it('should create renderGoldenLayout function that renders correct template', () => {
-            const {renderGoldenLayout} = createRenderHandlers(mockPugRequireHandler, mockOptions, mockDependencies);
+            const {renderGoldenLayout} = createRenderHandlers(
+                mockPugRequireHandler,
+                mockOptions,
+                mockDependencies,
+                mockAppArgs,
+            );
 
             const mockConfig = {};
             const mockMetadata = {};
@@ -219,7 +270,12 @@ describe('Rendering Module', () => {
         });
 
         it('should create embeddedHandler function that renders embed template', () => {
-            const {embeddedHandler} = createRenderHandlers(mockPugRequireHandler, mockOptions, mockDependencies);
+            const {embeddedHandler} = createRenderHandlers(
+                mockPugRequireHandler,
+                mockOptions,
+                mockDependencies,
+                mockAppArgs,
+            );
 
             const mockReq = {
                 query: {foo: 'bar'},

test/app/server-tests.ts

@@ -98,7 +98,7 @@ describe('Server Module', () => {
                 httpRoot: '',
                 sentrySlowRequestMs: 500,
                 manifestPath: '/mocked/dist', // Use absolute path for testing
-                extraBodyClass: '',
+                extraBodyClass: 'test-class',
                 maxUploadSize: '1mb',
             };
 
@@ -155,9 +155,9 @@ describe('Server Module', () => {
             const embeddedConfig = renderConfig({embedded: true});
             expect(embeddedConfig).toHaveProperty('extraBodyClass', 'embedded');
 
-            // When embedded is false the configured extraBodyClass is passed through
+            // When embedded is false
             const normalConfig = renderConfig({embedded: false});
-            expect(normalConfig).toHaveProperty('extraBodyClass', '');
+            expect(normalConfig).toHaveProperty('extraBodyClass', 'test-class');
         });
 
         it('should include mobile viewer slides when needed', async () => {

test/app/static-assets-tests.ts

@@ -22,18 +22,9 @@
 // ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 // POSSIBILITY OF SUCH DAMAGE.
 
-import path from 'node:path';
+import {describe, expect, it, vi} from 'vitest';
 
-import {afterAll, beforeAll, describe, expect, it, vi} from 'vitest';
-
-import {
-    createDefaultPugRequireHandler,
-    getBrandingAssetDir,
-    getFaviconFilename,
-    getLogoOverlayFilename,
-    validateBrandingAssets,
-} from '../../lib/app/static-assets.js';
-import {resolvePathFromAppRoot} from '../../lib/utils.js';
+import {createDefaultPugRequireHandler, getFaviconFilename} from '../../lib/app/static-assets.js';
 
 // Mock the logger
 vi.mock('../../lib/logger.js', () => ({
@@ -121,91 +112,38 @@ describe('Static assets', () => {
     });
 
     describe('getFaviconFilename', () => {
-        it('uses the default favicon when no body class is set', () => {
-            expect(getFaviconFilename('')).toBe('favicon.ico');
+        it('should prioritize dev environment over other environments', () => {
+            // Dev mode favicon should be used regardless of environment flags
+            expect(getFaviconFilename(true, [])).toContain('dev');
+            expect(getFaviconFilename(true, ['beta'])).toContain('dev');
+            expect(getFaviconFilename(true, ['staging'])).toContain('dev');
+            expect(getFaviconFilename(true, ['beta', 'staging'])).toContain('dev');
         });
 
-        it('derives the favicon name from the body class', () => {
-            expect(getFaviconFilename('dev')).toBe('favicon-dev.ico');
-            expect(getFaviconFilename('beta')).toBe('favicon-beta.ico');
-            expect(getFaviconFilename('staging')).toBe('favicon-staging.ico');
-            expect(getFaviconFilename('anything-else')).toBe('favicon-anything-else.ico');
-        });
-    });
+        it('should select appropriate favicon based on environment', () => {
+            // Test specific environments when not in dev mode
+            const environments = [
+                {env: ['beta'], expected: 'beta'},
+                {env: ['staging'], expected: 'staging'},
+                {env: [], expected: 'favicon.ico'},
+            ];
 
-    describe('getLogoOverlayFilename', () => {
-        it('returns undefined when no body class is set', () => {
-            expect(getLogoOverlayFilename('')).toBeUndefined();
+            for (const {env, expected} of environments) {
+                const result = getFaviconFilename(false, env);
+                if (expected === 'favicon.ico') {
+                    expect(result).toBe(expected);
+                } else {
+                    expect(result).toContain(expected);
+                }
+            }
         });
 
-        it('derives the overlay filename from the body class', () => {
-            expect(getLogoOverlayFilename('dev')).toBe('site-logo-dev.svg');
-            expect(getLogoOverlayFilename('beta')).toBe('site-logo-beta.svg');
-            expect(getLogoOverlayFilename('intern')).toBe('site-logo-intern.svg');
-        });
-    });
-
-    describe('validateBrandingAssets', () => {
-        let tmpDir: string;
-
-        beforeAll(async () => {
-            const fs = await import('node:fs/promises');
-            const os = await import('node:os');
-            tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'ce-branding-test-'));
-            await fs.writeFile(path.join(tmpDir, 'favicon-good.ico'), '');
-            await fs.writeFile(path.join(tmpDir, 'site-logo-good.svg'), '');
-            await fs.writeFile(path.join(tmpDir, 'favicon-partial.ico'), '');
-        });
-
-        afterAll(async () => {
-            const fs = await import('node:fs/promises');
-            await fs.rm(tmpDir, {recursive: true, force: true});
-        });
-
-        it('is a no-op when no body class is set', async () => {
-            await expect(validateBrandingAssets('/nonexistent/path', '')).resolves.toBeUndefined();
-        });
-
-        it('passes when both assets exist', async () => {
-            await expect(validateBrandingAssets(tmpDir, 'good')).resolves.toBeUndefined();
-        });
-
-        it('throws listing every missing asset', async () => {
-            await expect(validateBrandingAssets(tmpDir, 'missing')).rejects.toThrow(/favicon-missing\.ico/);
-            await expect(validateBrandingAssets(tmpDir, 'missing')).rejects.toThrow(/site-logo-missing\.svg/);
-        });
-
-        it('throws when only one asset is missing', async () => {
-            await expect(validateBrandingAssets(tmpDir, 'partial')).rejects.toThrow(/site-logo-partial\.svg/);
-        });
-    });
-
-    describe('getBrandingAssetDir', () => {
-        it('uses staticPath in production mode', () => {
-            expect(getBrandingAssetDir(false, '/dist/static')).toBe('/dist/static');
-        });
-
-        it('uses the public source dir in dev mode (assets are served from there, not staticPath)', () => {
-            // Regression: in dev the webpack middleware serves branding from public/, so validating
-            // staticPath (the source dir, which has no branding assets) spuriously crashes startup.
-            expect(getBrandingAssetDir(true, '/dist/static')).toBe(resolvePathFromAppRoot('public'));
-        });
-    });
-
-    describe('branding assets ship for every deployed environment', () => {
-        // Guards the dev path end-to-end: getBrandingAssetDir(devMode) points here and the real
-        // files must exist, so a missing asset or a wrong directory fails the build, not just prod.
-        const publicDir = getBrandingAssetDir(true, '/unused');
-
-        it.each([
-            'dev',
-            'beta',
-            'staging',
-            'winprod',
-            'winstaging',
-            'wintest',
-        ])('has favicon + logo overlay for %s', async extraBodyClass => {
-            await expect(validateBrandingAssets(publicDir, extraBodyClass)).resolves.toBeUndefined();
+        it('should handle environment arrays with mixed values', () => {
+            // When multiple environments are specified, there should be a consistent priority
+            expect(getFaviconFilename(false, ['beta', 'staging'])).toContain('beta');
+            expect(getFaviconFilename(false, ['staging', 'beta'])).toContain('beta');
+            expect(getFaviconFilename(false, ['other', 'beta'])).toContain('beta');
+            expect(getFaviconFilename(false, ['other', 'staging'])).toContain('staging');
         });
     });
 });

views/logo.pug

@@ -1,4 +1,8 @@
 a.navbar-brand(href=httpRoot title="Compiler Explorer", style="position: relative")
   img(src=staticRoot + "site-logo.svg" alt="Compiler Explorer logo" height="50" width="165")
-  if logoOverlayFilename
-    img(src=staticRoot + logoOverlayFilename alt="" aria-hidden="true" height="50" width="165" style="position: absolute; top: 0; right: 0;")
+  if extraBodyClass === "beta"
+    img(src=staticRoot + "site-logo-beta.svg" alt="Compiler Explorer logo (beta)" height="50" width="165" style="position: absolute; top: 0; right: 0;")
+  else if extraBodyClass === "staging"
+    img(src=staticRoot + "site-logo-staging.svg" alt="Compiler Explorer logo (staging)" height="50" width="165" style="position: absolute; top: 0; right: 0;")
+  else if extraBodyClass === "dev"
+    img(src=staticRoot + "site-logo-dev.svg" alt="Compiler Explorer logo (dev)" height="50" width="165" style="position: absolute; top: 0; right: 0;")