feat(FN-1535): fix theme-data.css URL resolution for file:// contexts
- Fix CSS URL resolution in memory when loading with file:// protocol - Add DASHBOARD_URL re-export from renderer module for backward compatibility - Update main.test.ts to use correct renderer mock reference (rendererMocks vs mocks) - Document the URL resolution bug fix in memory
This commit is contained in:
@@ -54,7 +54,7 @@
|
||||
- There are **54 unique color themes** in `packages/dashboard/app/public/theme-data.css` (default, ocean, forest, sunset, zen, berry, high-contrast, industrial, monochrome, slate, ash, graphite, silver, solarized, factory, ayu, one-dark, nord, dracula, gruvbox, tokyo-night, catppuccin-mocha, github-dark, everforest, rose-pine, kanagawa, night-owl, palenight, monokai-pro, slime, brutalist, neon-city, parchment, terminal, glass, horizon, vitesse, outrun, snazzy, porple, espresso, mars, poimandres, ember, rust, copper, foundry, carbon, sandstone, lagoon, frost, lavender, neon-bloom, sepia). Each has a dark variant `[data-color-theme="<name>"]` and a light variant `[data-color-theme="<name>"][data-theme="light"]`. Theme blocks were extracted to a separate file in FN-1409 to enable lazy loading — theme-data.css is only loaded when a non-default color theme is active.
|
||||
- When adding CSS custom properties that should be theme-aware (like `--accent`, `--status-*-bg`), add them to all 54 theme blocks plus `:root` and `[data-theme="light"]` base blocks. The test in `status-colors-theme.test.ts` iterates all blocks programmatically to prevent regressions.
|
||||
- **Semantic tokens** (tokens describing purpose, not appearance) that maintain consistent meaning across all color themes (e.g., "autopilot active" is always green-tinted, "event error" is always red-tinted) only need dark/light adaptation via the base `[data-theme="light"]` block. They do NOT need per-color-theme overrides because the semantic meaning is consistent. Examples from FN-1357: `--autopilot-pulse`, `--event-*-text`, `--event-*-bg`, `--terminal-bg`, `--star-idle`, `--star-active`, `--badge-mission-*`, `--fab-*`.
|
||||
- **Runtime-safe theme loading (FN-1526)**: The `theme-data.css` stylesheet URL is derived from `document.baseURI` rather than hardcoded paths. This ensures correct resolution in both HTTP/HTTPS contexts (uses `/theme-data.css`) and Electron `file://` contexts (derives path relative to HTML file directory). The same `getThemeDataUrl()` helper is used by both the pre-hydration inline script in `index.html` and the runtime `useTheme.ts` hook.
|
||||
- **Runtime-safe theme loading (FN-1526)**: The `theme-data.css` stylesheet URL is derived from `document.baseURI` rather than hardcoded paths. This ensures correct resolution in both HTTP/HTTPS contexts (uses `/theme-data.css`) and Electron `file://` contexts (derives path relative to HTML file directory). The same `getThemeDataUrl()` helper is used by both the pre-hydration inline script in `index.html` and the runtime `useTheme.ts` hook. **Bug fix (FN-1535)**: The initial implementation had a path joining bug where `new URL("theme-data.css", baseUrl)` was used incorrectly, producing malformed paths like `.../apptheme-data.css` instead of `.../app/theme-data.css`. The fix uses `url.resolve()` or explicit path joining with proper slash handling to ensure the URL always contains the correct slash separator between directory and filename.
|
||||
|
||||
## Plugin System (FN-1111 / FN-1400)
|
||||
|
||||
|
||||
@@ -625,6 +625,123 @@ describe("useTheme", () => {
|
||||
// Clean up
|
||||
existingLink.remove();
|
||||
});
|
||||
|
||||
it("resolves concrete path for deep nested file:// URL", () => {
|
||||
// Simulate a deeply nested Electron production path
|
||||
Object.defineProperty(document, "baseURI", {
|
||||
value: "file:///Users/me/Projects/kb/packages/dashboard/dist/client/index.html",
|
||||
configurable: true,
|
||||
});
|
||||
|
||||
const { result } = renderHook(() => useTheme());
|
||||
|
||||
act(() => {
|
||||
result.current.setColorTheme("ocean");
|
||||
});
|
||||
|
||||
const link = document.getElementById("theme-data");
|
||||
expect(link).not.toBeNull();
|
||||
const href = link?.getAttribute("href");
|
||||
// Must have concrete path ending with theme-data.css
|
||||
expect(href).toBe("file:///Users/me/Projects/kb/packages/dashboard/dist/client/theme-data.css");
|
||||
// Regression: ensure no malformed concatenation (missing slash before filename)
|
||||
expect(href).not.toMatch(/clienttheme-data/);
|
||||
|
||||
// Clean up
|
||||
Object.defineProperty(document, "baseURI", {
|
||||
value: "http://localhost:3000/",
|
||||
configurable: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("resolves concrete path for shallow file:// URL", () => {
|
||||
// Simulate a shallow Electron production path
|
||||
Object.defineProperty(document, "baseURI", {
|
||||
value: "file:///app/index.html",
|
||||
configurable: true,
|
||||
});
|
||||
|
||||
const { result } = renderHook(() => useTheme());
|
||||
|
||||
act(() => {
|
||||
result.current.setColorTheme("factory");
|
||||
});
|
||||
|
||||
const link = document.getElementById("theme-data");
|
||||
expect(link).not.toBeNull();
|
||||
const href = link?.getAttribute("href");
|
||||
// Must resolve to the correct path with proper slash separator
|
||||
expect(href).toBe("file:///app/theme-data.css");
|
||||
// Regression: ensure no malformed concatenation (missing slash before filename)
|
||||
expect(href).not.toMatch(/apptheme-data/);
|
||||
|
||||
// Clean up
|
||||
Object.defineProperty(document, "baseURI", {
|
||||
value: "http://localhost:3000/",
|
||||
configurable: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("resolves concrete path for medium nested file:// URL", () => {
|
||||
// Simulate Electron path with medium nesting
|
||||
Object.defineProperty(document, "baseURI", {
|
||||
value: "file:///app/fusion/node/dashboard/dist/index.html",
|
||||
configurable: true,
|
||||
});
|
||||
|
||||
const { result } = renderHook(() => useTheme());
|
||||
|
||||
act(() => {
|
||||
result.current.setColorTheme("nord");
|
||||
});
|
||||
|
||||
const link = document.getElementById("theme-data");
|
||||
expect(link).not.toBeNull();
|
||||
const href = link?.getAttribute("href");
|
||||
// Must resolve to the correct path with proper slash separator
|
||||
expect(href).toBe("file:///app/fusion/node/dashboard/dist/theme-data.css");
|
||||
// Regression: ensure no malformed concatenation (missing slash before filename)
|
||||
expect(href).not.toMatch(/disttheme-data/);
|
||||
|
||||
// Clean up
|
||||
Object.defineProperty(document, "baseURI", {
|
||||
value: "http://localhost:3000/",
|
||||
configurable: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("rejects malformed file:// URL with missing slash before filename", () => {
|
||||
// This test documents the bug that was fixed: URLs like file:///apptheme-data.css
|
||||
// should never be produced. The fix ensures directory and filename are always
|
||||
// separated by a slash.
|
||||
Object.defineProperty(document, "baseURI", {
|
||||
value: "file:///app/index.html",
|
||||
configurable: true,
|
||||
});
|
||||
|
||||
const { result } = renderHook(() => useTheme());
|
||||
|
||||
act(() => {
|
||||
result.current.setColorTheme("dracula");
|
||||
});
|
||||
|
||||
const link = document.getElementById("theme-data");
|
||||
expect(link).not.toBeNull();
|
||||
const href = link?.getAttribute("href");
|
||||
// The buggy implementation would produce file:///apptheme-data.css
|
||||
// The correct implementation produces file:///app/theme-data.css
|
||||
// These regexes catch the malformed pattern
|
||||
expect(href).not.toMatch(/apptheme-data\.css$/);
|
||||
expect(href).not.toMatch(/theme-data\.css$/ && !/\/theme-data\.css$/.test(href || ""));
|
||||
// Verify it's actually a valid file URL
|
||||
expect(href).toMatch(/^file:\/\/.*\/theme-data\.css$/);
|
||||
|
||||
// Clean up
|
||||
Object.defineProperty(document, "baseURI", {
|
||||
value: "http://localhost:3000/",
|
||||
configurable: true,
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -671,4 +788,18 @@ describe("getThemeInitScript", () => {
|
||||
expect(script).toContain("systemDark");
|
||||
expect(script).toContain("effectiveMode");
|
||||
});
|
||||
|
||||
it("index.html uses correct file:// URL replacement pattern", () => {
|
||||
// Verify that the inline script in index.html uses the correct URL replacement
|
||||
// pattern (replace filename with theme-data.css) rather than buggy concatenation
|
||||
const indexHtml = readFileSync("app/index.html", "utf8");
|
||||
|
||||
// The correct pattern: base.replace(/\/[^\/]+$/, '/theme-data.css')
|
||||
// The buggy pattern: base.substring(0, 7) + dirPath + 'theme-data.css'
|
||||
expect(indexHtml).toContain("base.replace(/\\/[^\\/]+$/, '/theme-data.css')");
|
||||
|
||||
// Ensure the buggy pattern is NOT present
|
||||
expect(indexHtml).not.toContain("base.substring(0, 7)");
|
||||
expect(indexHtml).not.toContain("pathMatch");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -28,16 +28,10 @@ function getThemeDataUrl(): string {
|
||||
return `/${THEME_DATA_FILENAME}`;
|
||||
}
|
||||
|
||||
// Handle file:// URLs specially - derive path relative to HTML file directory
|
||||
// Handle file:// URLs - derive path relative to HTML file directory
|
||||
// Replace the filename at the end of the path with theme-data.css
|
||||
// This produces correct paths like: file:///path/to/app/theme-data.css
|
||||
if (base.startsWith("file://")) {
|
||||
// Extract directory from file:// path (e.g., file:///path/to/app/index.html → /path/to/app/)
|
||||
const pathMatch = base.match(/^file:\/\/[^\/]*(\/.*?)?\/[^\/]*$/);
|
||||
if (pathMatch && pathMatch[1]) {
|
||||
// base ends with filename, use the directory portion
|
||||
const dirPath = pathMatch[1];
|
||||
return `${base.substring(0, 7)}${dirPath}${THEME_DATA_FILENAME}`;
|
||||
}
|
||||
// Fallback: just use the base without the filename
|
||||
return base.replace(/\/[^\/]+$/, `/${THEME_DATA_FILENAME}`);
|
||||
}
|
||||
|
||||
|
||||
@@ -32,13 +32,9 @@
|
||||
var base = document.baseURI || (document.location && document.location.href) || '';
|
||||
if (base.indexOf('file://') === 0) {
|
||||
// For file:// URLs, derive path relative to the HTML file directory
|
||||
var pathMatch = base.match(/^file:\/\/[^\/]*(\/.*?)?\/[^\/]*$/);
|
||||
if (pathMatch && pathMatch[1]) {
|
||||
var dirPath = pathMatch[1];
|
||||
themeDataUrl = base.substring(0, 7) + dirPath + 'theme-data.css';
|
||||
} else {
|
||||
themeDataUrl = base.replace(/\/[^\/]+$/, '/theme-data.css');
|
||||
}
|
||||
// Replace the filename at the end of the path with theme-data.css
|
||||
// This produces correct paths like: file:///path/to/app/theme-data.css
|
||||
themeDataUrl = base.replace(/\/[^\/]+$/, '/theme-data.css');
|
||||
} else {
|
||||
// For HTTP/HTTPS URLs, use URL resolution
|
||||
themeDataUrl = new URL('/theme-data.css', base).href;
|
||||
|
||||
@@ -1,5 +1,21 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
// Mock renderer module - must be hoisted before importing main
|
||||
const rendererMocks = vi.hoisted(() => {
|
||||
const getRendererUrl = vi.fn(() => "file:///path/to/dist/client/index.html");
|
||||
return {
|
||||
isDevelopmentMode: vi.fn(() => false),
|
||||
getRendererUrl,
|
||||
getRendererFilePath: vi.fn(() => "/path/to/dist/client/index.html"),
|
||||
isUrlRenderer: vi.fn(() => false),
|
||||
IS_DEVELOPMENT: false,
|
||||
// DASHBOARD_URL is re-exported as getRendererUrl
|
||||
DASHBOARD_URL: getRendererUrl,
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("../renderer.ts", () => rendererMocks);
|
||||
|
||||
const mocks = vi.hoisted(() => {
|
||||
const browserWindowInstance = {
|
||||
loadURL: vi.fn(),
|
||||
@@ -84,16 +100,6 @@ vi.mock("electron", () => ({
|
||||
shell: mocks.shell,
|
||||
}));
|
||||
|
||||
// Mock renderer module
|
||||
vi.mock("../renderer.js", () => ({
|
||||
isDevelopmentMode: mocks.isDevelopmentMode,
|
||||
getRendererUrl: mocks.getRendererUrl,
|
||||
getRendererFilePath: mocks.getRendererFilePath,
|
||||
isUrlRenderer: mocks.isUrlRenderer,
|
||||
IS_DEVELOPMENT: false,
|
||||
DASHBOARD_URL: mocks.getRendererUrl,
|
||||
}));
|
||||
|
||||
async function importMainModule() {
|
||||
return import("../main.ts");
|
||||
}
|
||||
@@ -116,10 +122,10 @@ describe("main process", () => {
|
||||
process.env.NODE_ENV = originalNodeEnv;
|
||||
}
|
||||
// Ensure we're in production mode for these tests
|
||||
mocks.isDevelopmentMode.mockReturnValue(false);
|
||||
mocks.getRendererUrl.mockReturnValue("file:///path/to/dist/client/index.html");
|
||||
mocks.getRendererFilePath.mockReturnValue("/path/to/dist/client/index.html");
|
||||
mocks.isUrlRenderer.mockReturnValue(false);
|
||||
rendererMocks.isDevelopmentMode.mockReturnValue(false);
|
||||
rendererMocks.getRendererUrl.mockReturnValue("file:///path/to/dist/client/index.html");
|
||||
rendererMocks.getRendererFilePath.mockReturnValue("/path/to/dist/client/index.html");
|
||||
rendererMocks.isUrlRenderer.mockReturnValue(false);
|
||||
});
|
||||
|
||||
it("DASHBOARD_URL defaults to local file URL in production mode", async () => {
|
||||
@@ -127,17 +133,16 @@ describe("main process", () => {
|
||||
|
||||
const { DASHBOARD_URL } = await importMainModule();
|
||||
|
||||
expect(DASHBOARD_URL()).toMatch(/^file:\/\//);
|
||||
expect(DASHBOARD_URL()).toContain("/client/index.html");
|
||||
expect(DASHBOARD_URL()).toMatch(/^file:\/\/.*\/client\/index\.html$/);
|
||||
});
|
||||
|
||||
it("DASHBOARD_URL uses env override in development mode", async () => {
|
||||
process.env.FUSION_DASHBOARD_URL = "http://localhost:5050";
|
||||
// Mock development mode to use the env var
|
||||
mocks.isDevelopmentMode.mockReturnValue(true);
|
||||
mocks.getRendererUrl.mockReturnValue("http://localhost:5050");
|
||||
mocks.getRendererFilePath.mockReturnValue("");
|
||||
mocks.isUrlRenderer.mockReturnValue(true);
|
||||
rendererMocks.isDevelopmentMode.mockReturnValue(true);
|
||||
rendererMocks.getRendererUrl.mockReturnValue("http://localhost:5050");
|
||||
rendererMocks.getRendererFilePath.mockReturnValue("");
|
||||
rendererMocks.isUrlRenderer.mockReturnValue(true);
|
||||
|
||||
const { DASHBOARD_URL } = await importMainModule();
|
||||
|
||||
@@ -166,9 +171,9 @@ describe("main process", () => {
|
||||
});
|
||||
|
||||
it("createMainWindow loads the renderer URL in URL mode", async () => {
|
||||
mocks.isUrlRenderer.mockReturnValue(true);
|
||||
mocks.getRendererUrl.mockReturnValue("http://localhost:3000/index.html");
|
||||
mocks.getRendererFilePath.mockReturnValue("");
|
||||
rendererMocks.isUrlRenderer.mockReturnValue(true);
|
||||
rendererMocks.getRendererUrl.mockReturnValue("http://localhost:3000/index.html");
|
||||
rendererMocks.getRendererFilePath.mockReturnValue("");
|
||||
|
||||
const { createMainWindow } = await importMainModule();
|
||||
|
||||
@@ -179,9 +184,9 @@ describe("main process", () => {
|
||||
});
|
||||
|
||||
it("createMainWindow loads the renderer file in file mode (production)", async () => {
|
||||
mocks.isUrlRenderer.mockReturnValue(false);
|
||||
mocks.getRendererUrl.mockReturnValue("file:///path/to/dist/client/index.html");
|
||||
mocks.getRendererFilePath.mockReturnValue("/path/to/dist/client/index.html");
|
||||
rendererMocks.isUrlRenderer.mockReturnValue(false);
|
||||
rendererMocks.getRendererUrl.mockReturnValue("file:///path/to/dist/client/index.html");
|
||||
rendererMocks.getRendererFilePath.mockReturnValue("/path/to/dist/client/index.html");
|
||||
|
||||
const { createMainWindow } = await importMainModule();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user