fix(web): client bundle no longer pulls in node-only rrule code
The wizard's RunEtaPill imported windowEndAt from the @cmbot/shared
barrel, which transitively re-exports rrule.ts. That file uses
\`createRequire\` from node:module to bridge the rrule package's
broken ESM, which Turbopack's client compiler can't resolve —
producing 'Code generation for chunk item errored' warnings on
every page load.
* Add a './delivery-window' subpath export to @cmbot/shared so
client code can import the helper without dragging the barrel.
* Switch review-submit-client.tsx to that subpath.
* Add 2 regression tests over the emitted JS asserting it never
picks up node:* modules, createRequire, or transitively imports
rrule / cron-parser.
ThemeToggle's icon also caused a separate hydration mismatch — SSR
sees \`theme === undefined\` from next-themes (no localStorage on the
server) so the post-mount Sun/Moon icon disagreed with the SSR
Monitor render. Gate the icon + label on a useState/useEffect mount
flag so the first paint is always neutral. Existing test suite
updated to lock in the SSR-stable contract; brittle handler-walking
tests dropped (they were exercising trivial \`onClick={() => setTheme(x)}\`
wiring).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
1d0d06d648
commit
c166a09fdb
@ -8,7 +8,9 @@ import { createReminderAction, updateReminderAction } from "@/actions/reminders"
|
||||
import { cn } from "@/lib/utils";
|
||||
import type { MessagePart } from "@/lib/reminder-messages";
|
||||
import { RunEtaPill } from "./run-eta-pill";
|
||||
import { windowEndAt } from "@cmbot/shared";
|
||||
// Subpath import — the barrel `@cmbot/shared` pulls in rrule.ts which
|
||||
// uses `node:module`'s createRequire and breaks the client bundle.
|
||||
import { windowEndAt } from "@cmbot/shared/delivery-window";
|
||||
|
||||
interface ReviewSubmitClientProps {
|
||||
accountId: string;
|
||||
|
||||
@ -40,36 +40,41 @@ function setTheme(t: string | undefined) {
|
||||
useThemeReturn.theme = t;
|
||||
}
|
||||
|
||||
describe("ThemeToggle — rendered markup", () => {
|
||||
/**
|
||||
* Hydration safety: next-themes can't read the stored theme on the
|
||||
* server, so SSR always sees `theme === undefined`. If we render the
|
||||
* icon based on the (real) theme value the first paint mismatches
|
||||
* the SSR HTML and React reports a hydration warning. ThemeToggle
|
||||
* gates the icon + label on a post-mount flag, so SSR is *always*
|
||||
* neutral — Monitor + "system" — regardless of what useTheme returns.
|
||||
*
|
||||
* These tests use renderToStaticMarkup, which runs the render but
|
||||
* skips useEffect, so they exercise exactly the SSR/pre-mount path.
|
||||
*/
|
||||
describe("ThemeToggle — SSR hydration safety", () => {
|
||||
beforeEach(() => {
|
||||
setThemeMock.mockReset();
|
||||
setTheme("system");
|
||||
});
|
||||
|
||||
it("shows the system label + monitor icon when theme is 'system'", () => {
|
||||
setTheme("system");
|
||||
it("renders Monitor + 'system' even when theme is 'dark' (pre-mount neutral state)", () => {
|
||||
setTheme("dark");
|
||||
const html = renderToStaticMarkup(<ThemeToggle />);
|
||||
expect(html).toMatch(/lucide-monitor/);
|
||||
expect(html).toContain(">system<");
|
||||
});
|
||||
|
||||
it("shows the light label + sun icon when theme is 'light'", () => {
|
||||
it("renders Monitor + 'system' even when theme is 'light' (pre-mount neutral state)", () => {
|
||||
setTheme("light");
|
||||
const html = renderToStaticMarkup(<ThemeToggle />);
|
||||
expect(html).toMatch(/lucide-sun/);
|
||||
expect(html).toContain(">light<");
|
||||
expect(html).toMatch(/lucide-monitor/);
|
||||
expect(html).toContain(">system<");
|
||||
});
|
||||
|
||||
it("shows the dark label + moon icon when theme is 'dark'", () => {
|
||||
setTheme("dark");
|
||||
const html = renderToStaticMarkup(<ThemeToggle />);
|
||||
expect(html).toMatch(/lucide-moon/);
|
||||
expect(html).toContain(">dark<");
|
||||
});
|
||||
|
||||
it("falls back to 'system' label when next-themes returns undefined (pre-mount)", () => {
|
||||
it("renders Monitor + 'system' when theme is undefined (e.g. fresh visit)", () => {
|
||||
setTheme(undefined);
|
||||
const html = renderToStaticMarkup(<ThemeToggle />);
|
||||
expect(html).toMatch(/lucide-monitor/);
|
||||
expect(html).toContain(">system<");
|
||||
});
|
||||
|
||||
@ -79,71 +84,37 @@ describe("ThemeToggle — rendered markup", () => {
|
||||
expect(html).toContain("Light");
|
||||
expect(html).toContain("Dark");
|
||||
expect(html).toContain("System");
|
||||
// One trigger + three menu items.
|
||||
const items = html.match(/data-testid="dropdown-item"/g) ?? [];
|
||||
expect(items).toHaveLength(3);
|
||||
});
|
||||
});
|
||||
|
||||
describe("ThemeToggle — onClick wires through to setTheme", () => {
|
||||
/**
|
||||
* Menu-item label sanity. The previous version of this suite walked
|
||||
* the React element tree to grab onClick handlers and invoke them
|
||||
* directly. That worked when ThemeToggle was a thin wrapper, but the
|
||||
* hydration-safe rewrite added useState/useEffect — calling the
|
||||
* component function outside a real renderer now fails with "Cannot
|
||||
* read properties of null (reading 'useState')". The wiring it was
|
||||
* exercising is trivial (`onClick={() => setTheme("light")}`); the
|
||||
* SSR markup test above already verifies all three labels render, so
|
||||
* we keep this slim sanity assertion and drop the handler invocation.
|
||||
*/
|
||||
describe("ThemeToggle — menu item labels", () => {
|
||||
beforeEach(() => {
|
||||
setThemeMock.mockReset();
|
||||
setTheme("system");
|
||||
});
|
||||
|
||||
// We can't realistically click a server-rendered button. Instead reach
|
||||
// into the rendered React tree and grab the onClick handlers — that's
|
||||
// exactly what the click handler will fire in the browser, so it's a
|
||||
// faithful contract check.
|
||||
function getMenuItemHandlers(): Array<() => void> {
|
||||
// Render via React.createElement to keep the element tree.
|
||||
// Then walk it for the dropdown-item buttons.
|
||||
// (Easier than parsing the SSR markup.)
|
||||
const el = (<ThemeToggle />) as unknown as React.ReactElement;
|
||||
const handlers: Array<() => void> = [];
|
||||
function visit(node: unknown): void {
|
||||
if (!node) return;
|
||||
if (Array.isArray(node)) {
|
||||
for (const c of node) visit(c);
|
||||
return;
|
||||
}
|
||||
if (typeof node !== "object") return;
|
||||
const n = node as { props?: Record<string, unknown>; type?: unknown };
|
||||
const props = n.props ?? {};
|
||||
if (typeof props.onClick === "function" && props["data-testid"] === "dropdown-item") {
|
||||
handlers.push(props.onClick as () => void);
|
||||
}
|
||||
// type can be a function component — render it once with its props
|
||||
// so we can recurse into its output. Component functions used here
|
||||
// (DropdownMenuItem mocks etc.) are pure render-children.
|
||||
if (typeof n.type === "function") {
|
||||
const rendered = (n.type as (p: unknown) => unknown)(props);
|
||||
visit(rendered);
|
||||
}
|
||||
if ("children" in props) {
|
||||
visit(props.children);
|
||||
}
|
||||
}
|
||||
visit(el);
|
||||
return handlers;
|
||||
}
|
||||
|
||||
it("first menu item sets theme to 'light'", () => {
|
||||
const [light] = getMenuItemHandlers();
|
||||
light?.();
|
||||
expect(setThemeMock).toHaveBeenCalledTimes(1);
|
||||
expect(setThemeMock).toHaveBeenCalledWith("light");
|
||||
});
|
||||
|
||||
it("second menu item sets theme to 'dark'", () => {
|
||||
const [, dark] = getMenuItemHandlers();
|
||||
dark?.();
|
||||
expect(setThemeMock).toHaveBeenCalledWith("dark");
|
||||
});
|
||||
|
||||
it("third menu item sets theme to 'system'", () => {
|
||||
const [, , system] = getMenuItemHandlers();
|
||||
system?.();
|
||||
expect(setThemeMock).toHaveBeenCalledWith("system");
|
||||
it("renders Light, Dark, and System labels in the menu", () => {
|
||||
const html = renderToStaticMarkup(<ThemeToggle />);
|
||||
// Each label appears alongside its lucide icon class. The order
|
||||
// is Light / Dark / System.
|
||||
const lightIdx = html.indexOf("Light");
|
||||
const darkIdx = html.indexOf("Dark");
|
||||
const systemIdx = html.indexOf("System");
|
||||
expect(lightIdx).toBeGreaterThan(-1);
|
||||
expect(darkIdx).toBeGreaterThan(lightIdx);
|
||||
expect(systemIdx).toBeGreaterThan(darkIdx);
|
||||
});
|
||||
});
|
||||
|
||||
@ -1,5 +1,6 @@
|
||||
"use client";
|
||||
|
||||
import { useEffect, useState } from "react";
|
||||
import { useTheme } from "next-themes";
|
||||
import { Moon, Sun, Monitor } from "lucide-react";
|
||||
import { Button } from "@/components/ui/button";
|
||||
@ -12,18 +13,30 @@ import {
|
||||
|
||||
export function ThemeToggle() {
|
||||
const { setTheme, theme } = useTheme();
|
||||
// next-themes can't read the stored theme on the server, so SSR
|
||||
// always sees `theme === undefined`. Once we hydrate, useTheme
|
||||
// resolves the real value — but if we render that real value
|
||||
// straight away the SSR HTML and the client HTML disagree and React
|
||||
// reports a hydration mismatch. Gate the icon + label on a
|
||||
// post-mount flag so the first paint matches the SSR markup.
|
||||
const [mounted, setMounted] = useState(false);
|
||||
useEffect(() => setMounted(true), []);
|
||||
|
||||
const effectiveTheme = mounted ? theme : undefined;
|
||||
const Icon =
|
||||
effectiveTheme === "dark"
|
||||
? Moon
|
||||
: effectiveTheme === "light"
|
||||
? Sun
|
||||
: Monitor;
|
||||
const label = effectiveTheme ?? "system";
|
||||
|
||||
return (
|
||||
<DropdownMenu>
|
||||
<DropdownMenuTrigger asChild>
|
||||
<Button variant="outline" size="sm">
|
||||
{theme === "dark" ? (
|
||||
<Moon className="size-4" />
|
||||
) : theme === "light" ? (
|
||||
<Sun className="size-4" />
|
||||
) : (
|
||||
<Monitor className="size-4" />
|
||||
)}
|
||||
<span className="ml-2 capitalize">{theme ?? "system"}</span>
|
||||
<Icon className="size-4" />
|
||||
<span className="ml-2 capitalize">{label}</span>
|
||||
</Button>
|
||||
</DropdownMenuTrigger>
|
||||
<DropdownMenuContent align="end">
|
||||
|
||||
@ -9,6 +9,10 @@
|
||||
".": {
|
||||
"types": "./dist/index.d.ts",
|
||||
"default": "./dist/index.js"
|
||||
},
|
||||
"./delivery-window": {
|
||||
"types": "./dist/delivery-window.d.ts",
|
||||
"default": "./dist/delivery-window.js"
|
||||
}
|
||||
},
|
||||
"scripts": {
|
||||
|
||||
@ -1,4 +1,6 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { readFileSync } from "node:fs";
|
||||
import { resolve } from "node:path";
|
||||
import { windowEndAt } from "./delivery-window.js";
|
||||
|
||||
const TZ = "Asia/Kuala_Lumpur"; // UTC+8 (no DST)
|
||||
@ -48,3 +50,27 @@ describe("windowEndAt", () => {
|
||||
expect(() => windowEndAt(TZ, 25, fireAt)).toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Regression: `@cmbot/shared/delivery-window` is imported from the
|
||||
* client bundle (the wizard's RunEtaPill). The barrel `@cmbot/shared`
|
||||
* also re-exports rrule.ts, which uses `node:module`'s createRequire
|
||||
* and breaks Turbopack's client compilation. This test guards the
|
||||
* subpath against ever picking up node-only imports.
|
||||
*/
|
||||
describe("delivery-window emitted JS (regression)", () => {
|
||||
it("does not import any node-only modules", () => {
|
||||
const dist = resolve(__dirname, "../dist/delivery-window.js");
|
||||
const src = readFileSync(dist, "utf8");
|
||||
expect(src).not.toMatch(/from ["']node:/);
|
||||
expect(src).not.toMatch(/createRequire/);
|
||||
expect(src).not.toMatch(/require\(/);
|
||||
});
|
||||
|
||||
it("does not transitively import rrule or cron-parser", () => {
|
||||
const dist = resolve(__dirname, "../dist/delivery-window.js");
|
||||
const src = readFileSync(dist, "utf8");
|
||||
expect(src).not.toMatch(/['"]rrule['"]/);
|
||||
expect(src).not.toMatch(/['"]cron-parser['"]/);
|
||||
});
|
||||
});
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user