Conversation
The main JS chunk shrinks from 2,891 KB (732 KB gzip) to 1,679 KB (412 KB gzip) via: - Load the inlined sim worker through a dynamic import so its ~512 KB payload becomes a lazy chunk fetched at game start. The worker itself still uses Vite's ?worker&inline Blob mechanism, unchanged at runtime. - Replace lit-markdown with marked + the already-bundled DOMPurify. lit-markdown transitively pulled in sanitize-html, htmlparser2, postcss, and entities (~325 KB) to render news markdown. - Drop colorjs.io (~114 KB); it was only used for deltaE2000 in ColorAllocator, which colord's lab plugin already provides. - Fetch msdf-atlas.json (~319 KB) at runtime like the atlas PNG, preloaded in parallel with worker init. - Stop shipping Tailwind CSS twice: o-modal now adopts the document's stylesheet instead of importing styles.css?inline (~158 KB). - Lazy-load the render-settings debug GUI (lil-gui, ~46 KB chunk). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
WalkthroughThis PR refactors markdown rendering from lit-markdown to marked + DOMPurify, introduces shared CSS infrastructure for Shadow DOM, enables async font atlas preloading overlapped with worker initialization, makes worker creation asynchronous, and simplifies color distance computation using colord instead of colorjs.io. ChangesMarkdown Rendering Migration
Shadow DOM CSS Architecture
Async Asset Loading and Lazy GUI
Async Worker Initialization
Color Distance Computation Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/ClientGameRunner.ts (1)
529-545:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnhandled rejection if dynamic import fails.
When
import("./render/gl/debug/index")rejects (network error, chunk missing), the promise has no.catch()handler. This creates an unhandled promise rejection. The.finally()resetsdebugGuiLoading, but the error is swallowed with no user feedback.Add a
.catch()to log the failure:Proposed fix
import("./render/gl/debug/index") .then(({ createDebugGui }) => { debugGui = createDebugGui(view.getSettings()); debugGui.open(); }) + .catch((err) => { + console.error("Failed to load debug GUI:", err); + }) .finally(() => { debugGuiLoading = false; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/ClientGameRunner.ts` around lines 529 - 545, The dynamic import in the ToggleRenderDebugGuiEvent handler can reject and currently has no .catch(), causing unhandled promise rejections; update the import("./render/gl/debug/index") promise chain in the eventBus listener (the block that checks debugGui === null and uses debugGuiLoading) to add a .catch(err => { /* log error and optionally notify user */ }) before the .finally so errors are logged (e.g., console.error or the app logger) and the user can be notified, while keeping the existing .finally that resets debugGuiLoading; ensure you reference and preserve debugGui, debugGuiLoading, and createDebugGui semantics when adding the catch.
🧹 Nitpick comments (2)
src/client/components/baseComponents/SharedStyles.ts (1)
14-35: ⚡ Quick winPrevent stale stylesheet overwrites from overlapping async refreshes.
populate()is launched without coordination from both initial load and HMR. If two runs overlap, a slower older run can finish last and replace newer CSS with stale content.Proposed fix
let sheet: CSSStyleSheet | null = null; +let populateVersion = 0; async function populate(target: CSSStyleSheet): Promise<void> { + const version = ++populateVersion; const parts: string[] = []; for (const style of Array.from(document.querySelectorAll("style"))) { parts.push(style.textContent ?? ""); } @@ - await target.replace(parts.join("\n")); + const nextCss = parts.join("\n"); + if (version === populateVersion) { + await target.replace(nextCss); + } }Also applies to: 48-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/components/baseComponents/SharedStyles.ts` around lines 14 - 35, The populate function can suffer from race conditions where overlapping async runs overwrite newer CSS with stale results; fix by introducing a run token/version check: create a module-scoped incrementing runId (or attach a unique token to the target), capture it at the start of populate, and before calling target.replace(parts.join("\n")) verify the current runId/token still matches to abort stale results; apply the same pattern to the similar logic referenced around lines 48-51 (the other stylesheet refresh path) so only the last-invoked async run performs the replace.src/client/theme/ColorAllocator.ts (1)
91-98: Clarify/keep theminDeltaEcomment; consider optional cleanup oflchPlugin.The
minDeltaEcomment is accurate:colord’scolor.delta()(CIEDE2000) returns a value normalized to the 0..1 range, so using relative magnitudes is fine.
lchPluginis extended insrc/client/theme/ColorAllocator.ts; if LCH features aren’t used anywhere, removing that setup can simplify startup/code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/theme/ColorAllocator.ts` around lines 91 - 98, Keep the existing minDeltaE comment as-is (it correctly states that colord.delta() is CIEDE2000 normalized to 0..1) and ensure the minDeltaE function remains unchanged; separately, audit the file for any usage of LCH features and if none exist, remove the lchPlugin import/extension and its registration (look for lchPlugin and any calls that extend colord plugins or use LCH methods) to simplify startup – if you remove lchPlugin, also remove related tests/refs and run the build to confirm no runtime LCH usage remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/render/gl/passes/name-pass/AtlasData.ts`:
- Around line 28-40: preloadAtlasData currently assigns a rejecting fetch
promise to atlasDataPromise, which gets cached and prevents retries on transient
failures; update preloadAtlasData to attach a .catch handler to the promise
returned by fetch(assetUrl("atlases/msdf-atlas.json")) that clears
atlasDataPromise (and does not mutate atlasData) before rethrowing the error so
subsequent calls can retry, while keeping the current success path that sets
atlasData from the parsed json; operate on the existing identifiers
preloadAtlasData, atlasDataPromise, and atlasData to implement this behavior.
In `@src/core/worker/WorkerClient.ts`:
- Around line 320-323: The cleanup() method currently terminates the worker but
leaves instance state that lets other methods (which call
this.worker!.postMessage(...)) run against a terminated worker; update cleanup()
(in WorkerClient) to also set this.isInitialized = false and this.worker =
undefined (in addition to clearing messageHandlers and gameUpdateCallback) so
post-terminate guards fail and no further posts are attempted.
- Around line 76-79: initialize() can be re-entered and leak workers; add a
guard flag (e.g., this._initializing) and check existing this.worker to prevent
starting a new worker if one is already initialized or initialization is in
progress, set the flag at start and clear it on success/failure, and if a
dispose/teardown is in progress (e.g., this._disposing or a dispose() method
exists) either await its completion or throw/return early; ensure you attach the
event listener (handleWorkerMessage) only after the guard passes and that any
partially created worker is cleaned up on error so createGameWorker() cannot be
leaked and this.worker isn't overwritten unexpectedly.
---
Outside diff comments:
In `@src/client/ClientGameRunner.ts`:
- Around line 529-545: The dynamic import in the ToggleRenderDebugGuiEvent
handler can reject and currently has no .catch(), causing unhandled promise
rejections; update the import("./render/gl/debug/index") promise chain in the
eventBus listener (the block that checks debugGui === null and uses
debugGuiLoading) to add a .catch(err => { /* log error and optionally notify
user */ }) before the .finally so errors are logged (e.g., console.error or the
app logger) and the user can be notified, while keeping the existing .finally
that resets debugGuiLoading; ensure you reference and preserve debugGui,
debugGuiLoading, and createDebugGui semantics when adding the catch.
---
Nitpick comments:
In `@src/client/components/baseComponents/SharedStyles.ts`:
- Around line 14-35: The populate function can suffer from race conditions where
overlapping async runs overwrite newer CSS with stale results; fix by
introducing a run token/version check: create a module-scoped incrementing runId
(or attach a unique token to the target), capture it at the start of populate,
and before calling target.replace(parts.join("\n")) verify the current
runId/token still matches to abort stale results; apply the same pattern to the
similar logic referenced around lines 48-51 (the other stylesheet refresh path)
so only the last-invoked async run performs the replace.
In `@src/client/theme/ColorAllocator.ts`:
- Around line 91-98: Keep the existing minDeltaE comment as-is (it correctly
states that colord.delta() is CIEDE2000 normalized to 0..1) and ensure the
minDeltaE function remains unchanged; separately, audit the file for any usage
of LCH features and if none exist, remove the lchPlugin import/extension and its
registration (look for lchPlugin and any calls that extend colord plugins or use
LCH methods) to simplify startup – if you remove lchPlugin, also remove related
tests/refs and run the build to confirm no runtime LCH usage remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca397c03-6dfd-4879-a058-1a6151a9a5a1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
package.jsonsrc/client/ClientGameRunner.tssrc/client/Markdown.tssrc/client/NewsModal.tssrc/client/components/NewsBox.tssrc/client/components/baseComponents/Modal.tssrc/client/components/baseComponents/SharedStyles.tssrc/client/render/gl/index.tssrc/client/render/gl/passes/name-pass/AtlasData.tssrc/client/theme/ColorAllocator.tssrc/core/worker/WorkerClient.ts
Comment on lines
+28
to
+40
| export function preloadAtlasData(): Promise<void> { | ||
| atlasDataPromise ??= fetch(assetUrl("atlases/msdf-atlas.json")) | ||
| .then((response) => { | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch msdf-atlas.json: ${response.status}`); | ||
| } | ||
| return response.json(); | ||
| }) | ||
| .then((json) => { | ||
| atlasData = json as RawMsdfAtlas; | ||
| }); | ||
| return atlasDataPromise; | ||
| } |
Contributor
There was a problem hiding this comment.
Failed fetch is cached permanently; no retry possible.
If fetch rejects (network error, DNS failure, etc.), atlasDataPromise keeps the rejected promise. All future calls to preloadAtlasData() return that same rejection — no recovery path exists.
Consider resetting the promise on failure so transient errors can be retried:
Proposed fix
export function preloadAtlasData(): Promise<void> {
- atlasDataPromise ??= fetch(assetUrl("atlases/msdf-atlas.json"))
+ if (atlasDataPromise !== null) return atlasDataPromise;
+ atlasDataPromise = fetch(assetUrl("atlases/msdf-atlas.json"))
.then((response) => {
if (!response.ok) {
throw new Error(`Failed to fetch msdf-atlas.json: ${response.status}`);
}
return response.json();
})
.then((json) => {
atlasData = json as RawMsdfAtlas;
+ })
+ .catch((err) => {
+ atlasDataPromise = null; // allow retry on transient failure
+ throw err;
});
return atlasDataPromise;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/render/gl/passes/name-pass/AtlasData.ts` around lines 28 - 40,
preloadAtlasData currently assigns a rejecting fetch promise to
atlasDataPromise, which gets cached and prevents retries on transient failures;
update preloadAtlasData to attach a .catch handler to the promise returned by
fetch(assetUrl("atlases/msdf-atlas.json")) that clears atlasDataPromise (and
does not mutate atlasData) before rethrowing the error so subsequent calls can
retry, while keeping the current success path that sets atlasData from the
parsed json; operate on the existing identifiers preloadAtlasData,
atlasDataPromise, and atlasData to implement this behavior.
Comment on lines
+76
to
+79
| async initialize(): Promise<void> { | ||
| const worker = await createGameWorker(); | ||
| this.worker = worker; | ||
| worker.addEventListener("message", this.handleWorkerMessage.bind(this)); |
Contributor
There was a problem hiding this comment.
Guard initialize() against re-entry and dispose races.
At Line 76-79, each call creates a new worker with no guard. If initialize() is called twice (or overlaps with teardown), one worker can be leaked and state can be overwritten unexpectedly.
Suggested patch sketch
export class WorkerClient {
private worker: Worker | null = null;
+ private initPromise: Promise<void> | null = null;
+ private disposed = false;
@@
async initialize(): Promise<void> {
+ if (this.disposed) throw new Error("WorkerClient already disposed");
+ if (this.isInitialized) return;
+ if (this.initPromise) return this.initPromise;
+
- const worker = await createGameWorker();
- this.worker = worker;
- worker.addEventListener("message", this.workerMessageListener);
-
- return new Promise((resolve, reject) => {
+ this.initPromise = (async () => {
+ const worker = await createGameWorker();
+ if (this.disposed) {
+ worker.terminate();
+ throw new Error("WorkerClient disposed during initialization");
+ }
+ this.worker = worker;
+ worker.addEventListener("message", this.workerMessageListener);
+ await new Promise<void>((resolve, reject) => {
// existing init handshake...
- });
+ });
+ })();
+ try {
+ await this.initPromise;
+ } finally {
+ this.initPromise = null;
+ }
}
@@
cleanup() {
+ this.disposed = true;
// ...
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/worker/WorkerClient.ts` around lines 76 - 79, initialize() can be
re-entered and leak workers; add a guard flag (e.g., this._initializing) and
check existing this.worker to prevent starting a new worker if one is already
initialized or initialization is in progress, set the flag at start and clear it
on success/failure, and if a dispose/teardown is in progress (e.g.,
this._disposing or a dispose() method exists) either await its completion or
throw/return early; ensure you attach the event listener (handleWorkerMessage)
only after the guard passes and that any partially created worker is cleaned up
on error so createGameWorker() cannot be leaked and this.worker isn't
overwritten unexpectedly.
Comment on lines
320
to
323
| cleanup() { | ||
| this.worker.terminate(); | ||
| this.worker?.terminate(); | ||
| this.messageHandlers.clear(); | ||
| this.gameUpdateCallback = undefined; |
Contributor
There was a problem hiding this comment.
Reset worker state in cleanup() to avoid post-terminate runtime failures.
At Line 320-323, cleanup() terminates the worker, but it does not clear isInitialized or worker. After cleanup, methods like Line 115+ still pass the init guard and call this.worker!.postMessage(...), which can target a terminated worker.
Suggested patch
export class WorkerClient {
private worker: Worker | null = null;
+ private workerMessageListener = this.handleWorkerMessage.bind(this);
private isInitialized = false;
@@
async initialize(): Promise<void> {
const worker = await createGameWorker();
this.worker = worker;
- worker.addEventListener("message", this.handleWorkerMessage.bind(this));
+ worker.addEventListener("message", this.workerMessageListener);
@@
cleanup() {
- this.worker?.terminate();
+ if (this.worker) {
+ this.worker.removeEventListener("message", this.workerMessageListener);
+ this.worker.terminate();
+ this.worker = null;
+ }
+ this.isInitialized = false;
this.messageHandlers.clear();
this.gameUpdateCallback = undefined;
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/worker/WorkerClient.ts` around lines 320 - 323, The cleanup() method
currently terminates the worker but leaves instance state that lets other
methods (which call this.worker!.postMessage(...)) run against a terminated
worker; update cleanup() (in WorkerClient) to also set this.isInitialized =
false and this.worker = undefined (in addition to clearing messageHandlers and
gameUpdateCallback) so post-terminate guards fail and no further posts are
attempted.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
4 tasks
evanpelle
added a commit
that referenced
this pull request
Jun 12, 2026
…4242) ## Problem Since #4229, modals render unstyled in `npm run dev` (no black backdrop, no Tailwind styling). Production/staging is unaffected. `documentStylesSheet()` reads the document's `<style>` tags once, at module-eval time. In dev, Vite injects the Tailwind styles *during* module evaluation — after that read — so the shared constructed stylesheet ended up with 7 CSS rules instead of the full Tailwind sheet. In production the styles come from a `<link rel=stylesheet>` that is fetched by URL, so timing doesn't matter there. ## Fix If the document hasn't finished loading when the sheet is first created, re-populate it once on the window `load` event (which fires after the entry module graph — and therefore all style injection — completes). Constructed stylesheets are live, so already-rendered components pick the styles up without re-rendering. The existing HMR re-populate hook is unchanged. ## Test plan - [x] Reproduced in dev with headless Chromium: shared sheet had 7 rules, modal unstyled - [x] After fix: sheet has full Tailwind rules, solo modal renders with correct dark styling (screenshot-verified) - [x] `npx tsc --noEmit`, ESLint clean - [x] Client test suite: 458 tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cuts the main JS chunk from 2,891 KB (732 KB gzip) to 1,679 KB (412 KB gzip) by fixing two bundling issues and removing/replacing heavy dependencies. Measured with a per-module
renderedLengthanalysis of the rolldown output (its prod sourcemaps are malformed, so sourcemap-based tools misattribute sizes).index-*.js(min)index-*.js(gzip)Changes
?worker&inlinepayload is now reached through a dynamicimport(), so it lands in its own lazy chunk fetched when a game starts. The worker itself still uses Vite's inline Blob mechanism (with itsdata:URL fallback) — runtime instantiation is byte-for-byte unchanged.lit-markdownwithmarked+ the already-bundled DOMPurify (~380 KB). lit-markdown transitively pulled sanitize-html, htmlparser2, postcss, and two copies of entities into the client just to render news markdown. Newsrc/client/Markdown.tsmatches its image-stripping default.colorjs.io(~114 KB). It was only used for ΔE2000 distance inColorAllocator; colord's lab plugin (already imported there) provides the same CIEDE2000 via.delta(). Only relative magnitudes are compared, so allocation behavior is unchanged.msdf-atlas.json(~319 KB) fetched at runtime like the atlas PNG, preloaded in parallel with worker init inClientGameRunnerso game-load latency is unaffected.o-modalimportedstyles.css?inline, duplicating the emitted stylesheet as a JS string. It now adopts a constructed stylesheet built from the document's own CSS (HTTP-cache hit in prod,<style>tags + HMR re-sync in dev) viaSharedStyles.ts.gl/debug/*now load on first toggle (46 KB lazy chunk) instead of shipping in the main bundle.Also looked at the
import * as d3in RadialMenu (~84 KB) but left it: rolldown tree-shakes the metapackage well and all but ~2 KB is the genuine dependency closure of the selection/transition/shape/color APIs in use.Test plan
tsc --noEmitcleannpm run build-prodsucceeds; worker/debug chunks present inasset-manifest.jsonfor the R2 upload🤖 Generated with Claude Code