Night Mode #2364
Open
Night Mode #2364
Conversation
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
Contributor
WalkthroughAdds a Night Mode feature: translations, persistent user setting and toggles, document Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsModal
participant UserSettings
participant Main
participant Renderer
participant NightModeLayer
participant Canvas
User->>SettingsModal: Click Night Mode toggle
SettingsModal->>UserSettings: toggleNightMode()
UserSettings->>UserSettings: update localStorage `settings.nightMode`
UserSettings->>Main: add/remove 'night' class on documentElement
SettingsModal->>Renderer: dispatch 'night-mode-changed'
Renderer->>NightModeLayer: request redraw / RefreshGraphicsEvent
User->>Canvas: Move mouse
Canvas->>NightModeLayer: mousemove -> update cursor pos
NightModeLayer->>Canvas: renderLayer()
Note right of NightModeLayer: when nightMode enabled\n draw dark overlay + city lights + spotlight
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
resources/lang/en.json(1 hunks)src/client/Main.ts(1 hunks)src/client/UserSettingModal.ts(2 hunks)src/client/graphics/GameRenderer.ts(2 hunks)src/client/graphics/layers/NightModeLayer.ts(1 hunks)src/client/graphics/layers/SettingsModal.ts(2 hunks)src/core/game/UserSettings.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/GameRenderer.tssrc/client/graphics/layers/NightModeLayer.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/GameRenderer.tssrc/client/Main.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/en.json
🧬 Code graph analysis (4)
src/client/UserSettingModal.ts (1)
src/client/LangSelector.ts (1)
translateText(254-274)
src/client/graphics/GameRenderer.ts (1)
src/client/graphics/layers/NightModeLayer.ts (1)
NightModeLayer(6-83)
src/client/graphics/layers/SettingsModal.ts (2)
src/client/InputHandler.ts (1)
RefreshGraphicsEvent(74-74)src/client/LangSelector.ts (1)
translateText(254-274)
src/client/graphics/layers/NightModeLayer.ts (3)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/UserSettings.ts (1)
UserSettings(6-214)src/client/graphics/TransformHandler.ts (1)
TransformHandler(15-295)
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Author
|
@coderabbitai generate docstrings |
Contributor
|
Warning Docstrings generation is disabled for your repository or organization. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 70903 @kkissdev I'll generate comprehensive docstrings for all the Night Mode feature code. Here's the documented version: NightModeLayer.tsimport { UserSettings } from "../../../core/game/UserSettings";
import { TransformHandler } from "../TransformHandler";
import { Layer } from "./Layer";
/**
* Rendering layer that creates a night mode effect with a flashlight-style spotlight
* following the mouse cursor. When enabled, darkens the entire screen except for
* an illuminated circular area around the cursor position.
*
* The flashlight effect uses a tile-based rendering approach where brightness
* decreases with distance from the cursor, creating a smooth gradient falloff.
*/
export class NightModeLayer implements Layer {
/**
* RGB color values for the darkness overlay.
* @private
*/
private darkenColor: [number, number, number] = [0, 0, 0];
/**
* Alpha transparency value for the darkness overlay (0.0 - 1.0).
* Separated from darkenColor for more readable code.
* @private
*/
private darkenAlpha: number = 0.8;
/**
* Radius of the flashlight effect in game tiles.
* Determines how large the illuminated area appears around the cursor.
* @private
*/
private flashlightRadius: number = 50;
/**
* Instance of UserSettings for reading night mode preferences.
* @private
*/
private userSettingsInstance = new UserSettings();
/**
* Current mouse X coordinate relative to the canvas.
* @private
*/
private mouseX: number = 0;
/**
* Current mouse Y coordinate relative to the canvas.
* @private
*/
private mouseY: number = 0;
/**
* Handles mouse movement events to update the flashlight position.
* Calculates cursor position relative to the canvas bounding rectangle.
*
* @param event - The mouse movement event
* @private
*/
private handleMouseMove(event: MouseEvent) {
const rect = this.transformHandler.boundingRect();
this.mouseX = event.clientX - rect.left;
this.mouseY = event.clientY - rect.top;
}
/**
* Lifecycle method called during layer initialization.
* Currently unused for this layer.
*/
init(): void {}
/**
* Lifecycle method called on each game tick.
* Currently unused for this layer.
*/
tick(): void {}
/**
* Lifecycle method called when a redraw is needed.
* Currently unused for this layer.
*/
redraw(): void {}
/**
* Creates a new NightModeLayer instance.
* Applies or removes the "night" CSS class based on user settings
* and registers a mousemove listener to track cursor position.
*
* @param transformHandler - Handler for canvas transformations and dimensions
*/
constructor(private transformHandler: TransformHandler) {
if (this.userSettingsInstance.nightMode()) {
document.documentElement.classList.add("night");
} else {
document.documentElement.classList.remove("night");
}
document.addEventListener("mousemove", (e) => this.handleMouseMove(e));
}
/**
* Renders the night mode overlay with flashlight effect.
*
* When night mode is enabled:
* 1. Fills the entire screen with a dark overlay
* 2. Iterates over tiles in a grid around the cursor position
* 3. Calculates distance from each tile to the cursor
* 4. Renders illuminated tiles with brightness based on distance
*
* The brightness factor creates a gradient falloff, with tiles closer
* to the cursor appearing brighter than those further away.
*
* @param context - The 2D rendering context to draw on
*/
renderLayer(context: CanvasRenderingContext2D): void {
if (!this.userSettingsInstance.nightMode()) return;
const width = this.transformHandler.width();
const height = this.transformHandler.boundingRect().height;
const cellSize = this.transformHandler.scale;
// Fill the entire screen with dark
context.fillStyle = `rgba(${this.darkenColor[0]}, ${this.darkenColor[1]}, ${this.darkenColor[2]}, ${this.darkenAlpha})`;
context.fillRect(0, 0, width, height);
// Calculate the bounding box for tiles within flashlight radius
const startX =
Math.floor(
Math.max(this.mouseX - this.flashlightRadius * cellSize, 0) / cellSize,
) * cellSize;
const endX =
Math.ceil(
Math.min(this.mouseX + this.flashlightRadius * cellSize, width) /
cellSize,
) * cellSize;
const startY =
Math.floor(
Math.max(this.mouseY - this.flashlightRadius * cellSize, 0) / cellSize,
) * cellSize;
const endY =
Math.ceil(
Math.min(this.mouseY + this.flashlightRadius * cellSize, height) /
cellSize,
) * cellSize;
// Render illuminated tiles in a grid pattern
for (let y = startY; y < endY; y += cellSize) {
for (let x = startX; x < endX; x += cellSize) {
// Calculate distance from mouse in tile units
const dist = Math.hypot(
(this.mouseX - (x + cellSize / 2)) / cellSize,
(this.mouseY - (y + cellSize / 2)) / cellSize,
);
// Determine brightness factor based on distance (0 at edge, 1 at center)
const brightness = Math.max(0, 1 - dist / this.flashlightRadius);
if (brightness > 0) {
// Render illuminated tile with warm yellowish tint
context.fillStyle = `rgba(200,200,130,${(this.darkenAlpha / 2) * brightness})`;
context.fillRect(x, y, cellSize, cellSize);
}
}
}
}
}UserSettings.ts (new methods only)/**
* Gets the current night mode setting from localStorage.
* Night mode creates a flashlight effect that darkens the map except
* for an illuminated area around the cursor.
*
* @returns {boolean} True if night mode is enabled, false otherwise. Defaults to false.
*/
nightMode() {
return this.get("settings.nightMode", false);
}
/**
* Toggles the night mode setting and updates the document root CSS class.
* When enabled, adds the "night" class to enable night mode styling.
* When disabled, removes the "night" class to restore normal appearance.
*
* The setting is persisted to localStorage for future sessions.
*/
toggleNightMode() {
this.set("settings.nightMode", !this.nightMode());
if (this.nightMode()) {
document.documentElement.classList.add("night");
} else {
document.documentElement.classList.remove("night");
}
}UserSettingModal.ts (new method only)/**
* Handles the toggle event for the Night Mode setting.
*
* Validates the toggle payload, updates the localStorage setting,
* applies or removes the "night" CSS class on the document root,
* and dispatches a custom "night-mode-changed" event to notify
* other components of the state change.
*
* @param e - Custom event containing the checked state in e.detail.checked
*/
toggleNightMode(e: CustomEvent<{ checked: boolean }>) {
const enabled = e.detail?.checked;
if (typeof enabled !== "boolean") {
console.warn("Unexpected toggle event payload", e);
return;
}
this.userSettings.set("settings.nightMode", enabled);
if (enabled) {
document.documentElement.classList.add("night");
} else {
document.documentElement.classList.remove("night");
}
this.dispatchEvent(
new CustomEvent("night-mode-changed", {
detail: { nightMode: enabled },
bubbles: true,
composed: true,
}),
);
}SettingsModal.ts (new method only)/**
* Handles the Night Mode toggle button click in the in-game settings modal.
*
* Toggles the night mode setting via UserSettings, emits a RefreshGraphicsEvent
* to update the rendering layers, and requests a UI update to reflect the new state.
*
* @private
*/
private onToggleNightModeButtonClick() {
this.userSettings.toggleNightMode();
this.eventBus.emit(new RefreshGraphicsEvent());
this.requestUpdate();
}GameRenderer.ts (import section comment)/**
* Night mode rendering layer that creates a flashlight effect
* with darkness overlay and illuminated cursor area.
*/
import { NightModeLayer } from "./layers/NightModeLayer";These docstrings follow TypeScript/JSDoc conventions and provide clear explanations of:
The documentation makes it easy for future developers to understand the Night Mode feature architecture and behavior. |
Author
|
@coderabbitai What tests could I add to make sure this functions optimally? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 679 🏁 Script executed: Length of output: 7227 @kkissdev Great question! Here are comprehensive test recommendations for the Night Mode feature, organized by category and following your project's Jest patterns: 1. Unit Tests for UserSettingsCreate import { UserSettings } from "../src/core/game/UserSettings";
describe("UserSettings - Night Mode", () => {
let userSettings: UserSettings;
beforeEach(() => {
// Mock localStorage
const localStorageMock = (() => {
let store: { [key: string]: string } = {};
return {
getItem: (key: string) => store[key] || null,
setItem: (key: string, value: string) => { store[key] = value; },
clear: () => { store = {}; },
};
})();
Object.defineProperty(global, 'localStorage', { value: localStorageMock, writable: true });
// Mock document.documentElement.classList
const classListMock = {
add: jest.fn(),
remove: jest.fn(),
};
Object.defineProperty(document.documentElement, 'classList', {
value: classListMock,
writable: true,
});
userSettings = new UserSettings();
localStorage.clear();
});
test("nightMode returns false by default", () => {
expect(userSettings.nightMode()).toBe(false);
});
test("nightMode returns stored value from localStorage", () => {
localStorage.setItem("settings.nightMode", "true");
expect(userSettings.nightMode()).toBe(true);
localStorage.setItem("settings.nightMode", "false");
expect(userSettings.nightMode()).toBe(false);
});
test("toggleNightMode enables night mode when disabled", () => {
expect(userSettings.nightMode()).toBe(false);
userSettings.toggleNightMode();
expect(userSettings.nightMode()).toBe(true);
expect(document.documentElement.classList.add).toHaveBeenCalledWith("night");
});
test("toggleNightMode disables night mode when enabled", () => {
localStorage.setItem("settings.nightMode", "true");
userSettings.toggleNightMode();
expect(userSettings.nightMode()).toBe(false);
expect(document.documentElement.classList.remove).toHaveBeenCalledWith("night");
});
test("toggleNightMode persists state to localStorage", () => {
userSettings.toggleNightMode();
expect(localStorage.getItem("settings.nightMode")).toBe("true");
userSettings.toggleNightMode();
expect(localStorage.getItem("settings.nightMode")).toBe("false");
});
});2. Unit Tests for NightModeLayerCreate import { NightModeLayer } from "../src/client/graphics/layers/NightModeLayer";
import { TransformHandler } from "../src/client/graphics/TransformHandler";
describe("NightModeLayer", () => {
let layer: NightModeLayer;
let mockTransformHandler: jest.Mocked<TransformHandler>;
let mockContext: jest.Mocked<CanvasRenderingContext2D>;
beforeEach(() => {
// Mock localStorage
const localStorageMock = (() => {
let store: { [key: string]: string } = {};
return {
getItem: (key: string) => store[key] || null,
setItem: (key: string, value: string) => { store[key] = value; },
clear: () => { store = {}; },
};
})();
Object.defineProperty(global, 'localStorage', { value: localStorageMock, writable: true });
// Mock document
const classListMock = {
add: jest.fn(),
remove: jest.fn(),
};
Object.defineProperty(document.documentElement, 'classList', {
value: classListMock,
writable: true,
});
// Mock addEventListener
document.addEventListener = jest.fn();
// Mock TransformHandler
mockTransformHandler = {
width: jest.fn().mockReturnValue(1920),
boundingRect: jest.fn().mockReturnValue({
left: 0,
top: 0,
height: 1080
}),
scale: 20,
} as any;
// Mock Canvas Context
mockContext = {
fillStyle: "",
fillRect: jest.fn(),
} as any;
localStorage.clear();
});
test("adds 'night' class to document when night mode enabled", () => {
localStorage.setItem("settings.nightMode", "true");
layer = new NightModeLayer(mockTransformHandler);
expect(document.documentElement.classList.add).toHaveBeenCalledWith("night");
});
test("removes 'night' class from document when night mode disabled", () => {
localStorage.setItem("settings.nightMode", "false");
layer = new NightModeLayer(mockTransformHandler);
expect(document.documentElement.classList.remove).toHaveBeenCalledWith("night");
});
test("registers mousemove event listener on construction", () => {
layer = new NightModeLayer(mockTransformHandler);
expect(document.addEventListener).toHaveBeenCalledWith(
"mousemove",
expect.any(Function)
);
});
test("does not render when night mode disabled", () => {
localStorage.setItem("settings.nightMode", "false");
layer = new NightModeLayer(mockTransformHandler);
layer.renderLayer(mockContext);
expect(mockContext.fillRect).not.toHaveBeenCalled();
});
test("renders dark overlay when night mode enabled", () => {
localStorage.setItem("settings.nightMode", "true");
layer = new NightModeLayer(mockTransformHandler);
layer.renderLayer(mockContext);
// Should fill entire screen with dark overlay
expect(mockContext.fillRect).toHaveBeenCalledWith(0, 0, 1920, 1080);
expect(mockContext.fillStyle).toContain("rgba(0, 0, 0, 0.8)");
});
test("renders flashlight effect around mouse position", () => {
localStorage.setItem("settings.nightMode", "true");
layer = new NightModeLayer(mockTransformHandler);
// Simulate mouse move to center
const mouseEvent = new MouseEvent("mousemove", {
clientX: 960,
clientY: 540,
});
document.dispatchEvent(mouseEvent);
layer.renderLayer(mockContext);
// Should render multiple tiles (dark overlay + illuminated tiles)
expect(mockContext.fillRect).toHaveBeenCalled();
expect(mockContext.fillRect.mock.calls.length).toBeGreaterThan(1);
});
});3. Integration Tests for UserSettingModalCreate import { UserSettingModal } from "../src/client/UserSettingModal";
describe("UserSettingModal - Night Mode", () => {
let modal: UserSettingModal;
beforeEach(() => {
// Mock localStorage
const localStorageMock = (() => {
let store: { [key: string]: string } = {};
return {
getItem: (key: string) => store[key] || null,
setItem: (key: string, value: string) => { store[key] = value; },
clear: () => { store = {}; },
};
})();
Object.defineProperty(global, 'localStorage', { value: localStorageMock, writable: true });
// Mock classList
const classListMock = {
add: jest.fn(),
remove: jest.fn(),
};
Object.defineProperty(document.documentElement, 'classList', {
value: classListMock,
writable: true,
});
modal = new UserSettingModal();
localStorage.clear();
});
test("toggleNightMode enables night mode on valid toggle event", () => {
const event = new CustomEvent("toggle", {
detail: { checked: true },
});
modal.toggleNightMode(event);
expect(localStorage.getItem("settings.nightMode")).toBe("true");
expect(document.documentElement.classList.add).toHaveBeenCalledWith("night");
});
test("toggleNightMode disables night mode on valid toggle event", () => {
localStorage.setItem("settings.nightMode", "true");
const event = new CustomEvent("toggle", {
detail: { checked: false },
});
modal.toggleNightMode(event);
expect(localStorage.getItem("settings.nightMode")).toBe("false");
expect(document.documentElement.classList.remove).toHaveBeenCalledWith("night");
});
test("toggleNightMode dispatches night-mode-changed event", () => {
const dispatchSpy = jest.spyOn(modal, "dispatchEvent");
const event = new CustomEvent("toggle", {
detail: { checked: true },
});
modal.toggleNightMode(event);
expect(dispatchSpy).toHaveBeenCalledWith(
expect.objectContaining({
type: "night-mode-changed",
detail: { nightMode: true },
})
);
});
test("toggleNightMode handles invalid event payload gracefully", () => {
const consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation();
const event = new CustomEvent("toggle", {
detail: { checked: "invalid" }, // Wrong type
}) as any;
modal.toggleNightMode(event);
expect(consoleWarnSpy).toHaveBeenCalledWith(
"Unexpected toggle event payload",
event
);
// Should not modify localStorage
expect(localStorage.getItem("settings.nightMode")).toBeNull();
consoleWarnSpy.mockRestore();
});
});4. Performance TestsAdd to describe("NightModeLayer - Performance", () => {
test("rendering completes within reasonable time for large canvas", () => {
localStorage.setItem("settings.nightMode", "true");
mockTransformHandler.width.mockReturnValue(3840); // 4K width
mockTransformHandler.boundingRect.mockReturnValue({
left: 0,
top: 0,
height: 2160 // 4K height
});
layer = new NightModeLayer(mockTransformHandler);
const startTime = performance.now();
layer.renderLayer(mockContext);
const endTime = performance.now();
// Rendering should complete in under 16ms (60fps)
expect(endTime - startTime).toBeLessThan(16);
});
test("flashlight radius limits tile iteration count", () => {
localStorage.setItem("settings.nightMode", "true");
layer = new NightModeLayer(mockTransformHandler);
// Set mouse at edge of screen
const mouseEvent = new MouseEvent("mousemove", {
clientX: 0,
clientY: 0,
});
document.dispatchEvent(mouseEvent);
mockContext.fillRect.mockClear();
layer.renderLayer(mockContext);
const callCount = mockContext.fillRect.mock.calls.length;
// Should only render tiles within flashlight radius, not entire screen
const maxTilesInRadius = Math.PI * 50 * 50; // π * r²
expect(callCount).toBeLessThan(maxTilesInRadius * 1.5); // Add buffer for grid cells
});
});5. Edge Case Testsdescribe("NightModeLayer - Edge Cases", () => {
test("handles mouse position at canvas boundaries", () => {
localStorage.setItem("settings.nightMode", "true");
layer = new NightModeLayer(mockTransformHandler);
// Mouse at far right edge
const mouseEvent = new MouseEvent("mousemove", {
clientX: 1920,
clientY: 1080,
});
document.dispatchEvent(mouseEvent);
expect(() => layer.renderLayer(mockContext)).not.toThrow();
});
test("handles negative mouse coordinates", () => {
localStorage.setItem("settings.nightMode", "true");
layer = new NightModeLayer(mockTransformHandler);
mockTransformHandler.boundingRect.mockReturnValue({
left: 100,
top: 100,
height: 1080,
});
// Mouse outside canvas (negative relative position)
const mouseEvent = new MouseEvent("mousemove", {
clientX: 50,
clientY: 50,
});
document.dispatchEvent(mouseEvent);
expect(() => layer.renderLayer(mockContext)).not.toThrow();
});
test("handles zero scale in TransformHandler", () => {
localStorage.setItem("settings.nightMode", "true");
mockTransformHandler.scale = 0;
layer = new NightModeLayer(mockTransformHandler);
// Should not cause division by zero
expect(() => layer.renderLayer(mockContext)).not.toThrow();
});
});Test ExecutionRun the tests with: npm test # Run all tests
npm test UserSettings # Run specific test file
npm test -- --coverage # Generate coverage report
npm test -- --watch # Watch mode for developmentKey Testing BenefitsThese tests ensure:
The tile-based rendering and mouse tracking are the most critical areas for performance testing, as they execute every frame. Focus on ensuring the flashlight radius effectively limits computation. |
Author
|
@coderabbitai I want each city in the game to be illuminated (with a pattern I will tinker with) like the satellite images of Earth with all the lights. Where can i find the location of each city? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2273 🏁 Script executed: Length of output: 8174 🏁 Script executed: Length of output: 17744 🏁 Script executed: Length of output: 12295 🏁 Script executed: Length of output: 7568 @kkissdev Perfect! I can help you find where cities are located so you can add illumination to them. Here's what you need to know: City Location InformationCities are represented as Key Methods:
Implementation in NightModeLayerYou'll need to add game access to your import { UserSettings } from "../../../core/game/UserSettings";
import { TransformHandler } from "../TransformHandler";
import { Layer } from "./Layer";
import { UnitType } from "../../../core/game/Game";
import { GameView } from "../../../core/game/GameView";
export class NightModeLayer implements Layer {
private darkenColor: [number, number, number] = [0, 0, 0];
private darkenAlpha: number = 0.8;
private flashlightRadius: number = 50;
private userSettingsInstance = new UserSettings();
private mouseX: number = 0;
private mouseY: number = 0;
// Add game reference
private game: GameView | null = null;
private handleMouseMove(event: MouseEvent) {
const rect = this.transformHandler.boundingRect();
this.mouseX = event.clientX - rect.left;
this.mouseY = event.clientY - rect.top;
}
init(): void {}
tick(): void {}
redraw(): void {}
constructor(
private transformHandler: TransformHandler,
game?: GameView // Add game parameter
) {
this.game = game || null;
if (this.userSettingsInstance.nightMode()) {
document.documentElement.classList.add("night");
} else {
document.documentElement.classList.remove("night");
}
document.addEventListener("mousemove", (e) => this.handleMouseMove(e));
}
// New method to set game reference after construction
setGame(game: GameView) {
this.game = game;
}
renderLayer(context: CanvasRenderingContext2D): void {
if (!this.userSettingsInstance.nightMode()) return;
const width = this.transformHandler.width();
const height = this.transformHandler.boundingRect().height;
const cellSize = this.transformHandler.scale;
// Fill the entire screen with dark
context.fillStyle = `rgba(${this.darkenColor[0]}, ${this.darkenColor[1]}, ${this.darkenColor[2]}, ${this.darkenAlpha})`;
context.fillRect(0, 0, width, height);
// ===== NEW: Render city lights =====
if (this.game) {
this.renderCityLights(context, cellSize);
}
// Render flashlight effect around mouse
this.renderFlashlight(context, width, height, cellSize);
}
/**
* Renders illumination for all cities on the map.
* Creates a glow effect similar to satellite images of Earth at night.
*/
private renderCityLights(
context: CanvasRenderingContext2D,
cellSize: number
): void {
// Get all cities in the game
const cities = this.game!.units(UnitType.City);
for (const city of cities) {
// Get city position
const tileRef = city.tile();
const cityX = this.game!.x(tileRef);
const cityY = this.game!.y(tileRef);
// Convert tile coordinates to screen coordinates
const screenX = cityX * cellSize;
const screenY = cityY * cellSize;
// Get city level for scaling the light effect
const cityLevel = city.level();
// Render city glow - you can customize this pattern
this.renderCityGlow(context, screenX, screenY, cellSize, cityLevel);
}
}
/**
* Renders a glow effect for a single city.
* Customize this method to achieve your desired lighting pattern.
*/
private renderCityGlow(
context: CanvasRenderingContext2D,
x: number,
y: number,
cellSize: number,
level: number
): void {
// Example 1: Simple bright square (like a satellite image)
const lightRadius = 2 + level; // Larger cities have bigger glow
for (let dy = -lightRadius; dy <= lightRadius; dy++) {
for (let dx = -lightRadius; dx <= lightRadius; dx++) {
const distance = Math.sqrt(dx * dx + dy * dy);
if (distance <= lightRadius) {
// Brightness decreases with distance
const brightness = 1 - (distance / lightRadius);
const alpha = (this.darkenAlpha * 0.8) * brightness;
context.fillStyle = `rgba(255, 220, 150, ${alpha})`;
context.fillRect(
x + dx * cellSize,
y + dy * cellSize,
cellSize,
cellSize
);
}
}
}
// Example 2: Add a brighter core
context.fillStyle = `rgba(255, 255, 200, ${this.darkenAlpha * 0.9})`;
context.fillRect(x, y, cellSize, cellSize);
}
/**
* Renders the flashlight effect around the mouse cursor.
* Extracted from original renderLayer for better organization.
*/
private renderFlashlight(
context: CanvasRenderingContext2D,
width: number,
height: number,
cellSize: number
): void {
const startX =
Math.floor(
Math.max(this.mouseX - this.flashlightRadius * cellSize, 0) / cellSize,
) * cellSize;
const endX =
Math.ceil(
Math.min(this.mouseX + this.flashlightRadius * cellSize, width) /
cellSize,
) * cellSize;
const startY =
Math.floor(
Math.max(this.mouseY - this.flashlightRadius * cellSize, 0) / cellSize,
) * cellSize;
const endY =
Math.ceil(
Math.min(this.mouseY + this.flashlightRadius * cellSize, height) /
cellSize,
) * cellSize;
for (let y = startY; y < endY; y += cellSize) {
for (let x = startX; x < endX; x += cellSize) {
const dist = Math.hypot(
(this.mouseX - (x + cellSize / 2)) / cellSize,
(this.mouseY - (y + cellSize / 2)) / cellSize,
);
const brightness = Math.max(0, 1 - dist / this.flashlightRadius);
if (brightness > 0) {
context.fillStyle = `rgba(200,200,130,${(this.darkenAlpha / 2) * brightness})`;
context.fillRect(x, y, cellSize, cellSize);
}
}
}
}
}Update GameRenderer.tsYou'll need to pass the game instance when creating the layer: // In src/client/graphics/GameRenderer.ts
const nightModeLayer = new NightModeLayer(transformHandler);
// Later, when game is available (in a method that has access to game):
nightModeLayer.setGame(game);Alternative Lighting PatternsHere are some pattern ideas you can experiment with: Pattern 1: Radial Gradient (Soft Glow)private renderCityGlow(context: CanvasRenderingContext2D, x: number, y: number, cellSize: number, level: number): void {
const gradient = context.createRadialGradient(
x + cellSize / 2, y + cellSize / 2, 0,
x + cellSize / 2, y + cellSize / 2, cellSize * (2 + level)
);
gradient.addColorStop(0, 'rgba(255, 230, 150, 0.9)');
gradient.addColorStop(0.5, 'rgba(255, 200, 100, 0.5)');
gradient.addColorStop(1, 'rgba(255, 180, 80, 0)');
context.fillStyle = gradient;
context.fillRect(x - cellSize * 2, y - cellSize * 2, cellSize * 5, cellSize * 5);
}Pattern 2: Grid Pattern (Street Lights)private renderCityGlow(context: CanvasRenderingContext2D, x: number, y: number, cellSize: number, level: number): void {
const gridSize = 1 + level;
for (let i = -gridSize; i <= gridSize; i++) {
for (let j = -gridSize; j <= gridSize; j++) {
if ((i + j) % 2 === 0) { // Checkerboard pattern
context.fillStyle = 'rgba(255, 220, 120, 0.7)';
context.fillRect(x + i * cellSize, y + j * cellSize, cellSize, cellSize);
}
}
}
}Pattern 3: Pulsing Effect (Animated)private renderCityGlow(context: CanvasRenderingContext2D, x: number, y: number, cellSize: number, level: number): void {
// Use game tick or timestamp for animation
const pulse = (Math.sin(Date.now() / 1000) + 1) / 2; // 0 to 1
const alpha = 0.5 + pulse * 0.4;
context.fillStyle = `rgba(255, 220, 150, ${alpha})`;
context.fillRect(x, y, cellSize * (1 + level), cellSize * (1 + level));
}Adding Lights to SAMs and NukesSimilarly, you can add lights to other structures based on their height in trajectory: private renderStructureLights(context: CanvasRenderingContext2D, cellSize: number): void {
// SAM Launchers
const sams = this.game!.units(UnitType.SAMLauncher);
for (const sam of sams) {
const tileRef = sam.tile();
const x = this.game!.x(tileRef) * cellSize;
const y = this.game!.y(tileRef) * cellSize;
// Red warning lights
context.fillStyle = 'rgba(255, 50, 50, 0.6)';
context.fillRect(x, y, cellSize, cellSize);
}
// Nukes in flight (if above certain height)
const nukes = this.game!.units(
UnitType.AtomBomb,
UnitType.HydrogenBomb,
UnitType.MIRV
);
for (const nuke of nukes) {
const trajectoryIndex = nuke.trajectoryIndex();
const trajectory = nuke.trajectory();
if (trajectoryIndex < trajectory.length) {
const currentTile = trajectory[trajectoryIndex].tile;
const x = this.game!.x(currentTile) * cellSize;
const y = this.game!.y(currentTile) * cellSize;
// Bright white glow for nukes
context.fillStyle = 'rgba(255, 255, 255, 0.9)';
context.fillRect(x - cellSize, y - cellSize, cellSize * 3, cellSize * 3);
}
}
}This gives you complete access to all city locations and their coordinates. You can now tinker with the lighting patterns to achieve the satellite image effect you're looking for! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
resources/images/NightModeIconWhite.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
.coderabbit.yaml(1 hunks)resources/lang/en.json(1 hunks)src/client/graphics/GameRenderer.ts(2 hunks)src/client/graphics/layers/SettingsModal.ts(3 hunks)tests/NightModeLayer.test.ts(1 hunks)tests/NightModeUserSettingModal.test.ts(1 hunks)tests/NightModeUserSettings.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .coderabbit.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/graphics/layers/SettingsModal.ts
- src/client/graphics/GameRenderer.ts
- resources/lang/en.json
🧰 Additional context used
🧬 Code graph analysis (2)
tests/NightModeUserSettings.test.ts (1)
src/core/game/UserSettings.ts (1)
UserSettings(6-214)
tests/NightModeLayer.test.ts (2)
src/client/graphics/layers/NightModeLayer.ts (1)
NightModeLayer(6-84)src/client/graphics/TransformHandler.ts (1)
TransformHandler(15-295)
🪛 GitHub Actions: 🧪 CI
tests/NightModeUserSettings.test.ts
[error] Jest test environment is not configured for DOM manipulation; reference errors: 'document is not defined' in tests that attempt to access DOM APIs.
tests/NightModeUserSettingModal.test.ts
[error] Support for ES module syntax (import statements) missing in Jest configuration, causing syntax error: Cannot use import statement outside a module.
Comment on lines
110
to
126
| test("renders flashlight effect around mouse position", () => { | ||
| localStorage.setItem("settings.nightMode", "true"); | ||
| layer = new NightModeLayer(mockTransformHandler); | ||
|
|
||
| // Simulate mouse move to center | ||
| const mouseEvent = new MouseEvent("mousemove", { | ||
| clientX: 960, | ||
| clientY: 540, | ||
| }); | ||
| document.dispatchEvent(mouseEvent); | ||
|
|
||
| layer.renderLayer(mockContext); | ||
|
|
||
| // Should render multiple tiles (dark overlay + illuminated tiles) | ||
| expect(mockContext.fillRect).toHaveBeenCalled(); | ||
| expect(mockContext.fillRect.mock.calls.length).toBeGreaterThan(1); | ||
| }); |
Contributor
There was a problem hiding this comment.
Mouse event test doesn't properly verify flashlight position.
The test mocks document.addEventListener at line 39, which prevents the layer's mousemove handler from being registered. When document.dispatchEvent(mouseEvent) is called at line 119, the event isn't handled, so the mouse position remains at (0, 0) rather than the intended (960, 540).
To properly test mouse tracking, store the listener and invoke it directly:
+ let mouseMoveHandler: ((event: MouseEvent) => void) | undefined;
+
// Mock addEventListener
- document.addEventListener = jest.fn();
+ document.addEventListener = jest.fn((event: string, handler: any) => {
+ if (event === 'mousemove') {
+ mouseMoveHandler = handler;
+ }
+ });
// ... in beforeEach ...
test("renders flashlight effect around mouse position", () => {
localStorage.setItem("settings.nightMode", "true");
layer = new NightModeLayer(mockTransformHandler);
// Simulate mouse move to center
const mouseEvent = new MouseEvent("mousemove", {
clientX: 960,
clientY: 540,
});
- document.dispatchEvent(mouseEvent);
+ mouseMoveHandler?.(mouseEvent);
layer.renderLayer(mockContext);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/NightModeLayer.test.ts around lines 110 to 126, the mousemove
dispatched event isn't reaching the layer because document.addEventListener was
mocked earlier and prevented the handler from being registered; capture the
listener when addEventListener is mocked (save the callback stored for the
"mousemove" event) and invoke that saved listener directly with a fabricated
event having clientX: 960 and clientY: 540 (or stop mocking addEventListener so
the real handler registers) before calling layer.renderLayer(mockContext) so the
layer's internal mouse position is updated and the assertions verify the
flashlight at the intended coordinates.
Comment on lines
+127
to
+148
| test("rendering completes within reasonable time for large canvas", () => { | ||
| localStorage.setItem("settings.nightMode", "true"); | ||
|
|
||
| mockTransformHandler = { | ||
| width: jest.fn().mockReturnValue(3840), // 4k | ||
| boundingRect: jest.fn().mockReturnValue({ | ||
| left: 0, | ||
| top: 0, | ||
| height: 2160, // 4k | ||
| }), | ||
| scale: 20, | ||
| } as any; | ||
|
|
||
| layer = new NightModeLayer(mockTransformHandler); | ||
|
|
||
| const startTime = performance.now(); | ||
| layer.renderLayer(mockContext); | ||
| const endTime = performance.now(); | ||
|
|
||
| // Rendering should complete in under 16ms (60fps) | ||
| expect(endTime - startTime).toBeLessThan(16); | ||
| }); |
Contributor
There was a problem hiding this comment.
Performance test with hard-coded threshold is fragile.
The test expects rendering to complete in under 16ms, but this threshold is environment-dependent and will cause flaky failures on slower CI runners or under system load.
Consider one of these alternatives:
- Remove the hard threshold and make it informational:
- // Rendering should complete in under 16ms (60fps)
- expect(endTime - startTime).toBeLessThan(16);
+ // Log rendering time for informational purposes
+ console.log(`Rendering time: ${endTime - startTime}ms`);- Use a much more generous threshold:
- expect(endTime - startTime).toBeLessThan(16);
+ // Ensure rendering doesn't hang (generous threshold for CI)
+ expect(endTime - startTime).toBeLessThan(100);- Move to a separate performance test suite that runs optionally and reports metrics rather than asserting hard limits.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("rendering completes within reasonable time for large canvas", () => { | |
| localStorage.setItem("settings.nightMode", "true"); | |
| mockTransformHandler = { | |
| width: jest.fn().mockReturnValue(3840), // 4k | |
| boundingRect: jest.fn().mockReturnValue({ | |
| left: 0, | |
| top: 0, | |
| height: 2160, // 4k | |
| }), | |
| scale: 20, | |
| } as any; | |
| layer = new NightModeLayer(mockTransformHandler); | |
| const startTime = performance.now(); | |
| layer.renderLayer(mockContext); | |
| const endTime = performance.now(); | |
| // Rendering should complete in under 16ms (60fps) | |
| expect(endTime - startTime).toBeLessThan(16); | |
| }); | |
| test("rendering completes within reasonable time for large canvas", () => { | |
| localStorage.setItem("settings.nightMode", "true"); | |
| mockTransformHandler = { | |
| width: jest.fn().mockReturnValue(3840), // 4k | |
| boundingRect: jest.fn().mockReturnValue({ | |
| left: 0, | |
| top: 0, | |
| height: 2160, // 4k | |
| }), | |
| scale: 20, | |
| } as any; | |
| layer = new NightModeLayer(mockTransformHandler); | |
| const startTime = performance.now(); | |
| layer.renderLayer(mockContext); | |
| const endTime = performance.now(); | |
| // Log rendering time for informational purposes | |
| console.log(`Rendering time: ${endTime - startTime}ms`); | |
| }); |
🤖 Prompt for AI Agents
In tests/NightModeLayer.test.ts around lines 127-148, the test uses a hard 16ms
render threshold which is flaky on CI; change the test to avoid asserting a
strict 16ms limit: compute duration as now, then if an env var like
RUN_PERF_TESTS is set perform a much more generous assertion (e.g.,
expect(duration).toBeLessThan(200)) for optional/perf runs, otherwise remove the
failing assertion and replace it with a console.info/log of the duration (or a
non-failing expect that duration is a finite number) so normal unit runs never
fail due to environment-dependent timing.
Comment on lines
+150
to
+169
| test("flashlight radius limits tile iteration count", () => { | ||
| localStorage.setItem("settings.nightMode", "true"); | ||
| layer = new NightModeLayer(mockTransformHandler); | ||
|
|
||
| // Set mouse at edge of screen | ||
| const mouseEvent = new MouseEvent("mousemove", { | ||
| clientX: 0, | ||
| clientY: 0, | ||
| }); | ||
| document.dispatchEvent(mouseEvent); | ||
|
|
||
| mockContext.fillRect.mockClear(); | ||
| layer.renderLayer(mockContext); | ||
|
|
||
| const callCount = mockContext.fillRect.mock.calls.length; | ||
|
|
||
| // Should only render tiles within flashlight radius, not entire screen | ||
| const maxTilesInRadius = Math.PI * 50 * 50; // π * r² | ||
| expect(callCount).toBeLessThan(maxTilesInRadius * 1.5); // Add buffer for grid cells | ||
| }); |
Contributor
There was a problem hiding this comment.
Same event handling issue as previous test.
The mousemove event dispatched at line 159 won't be handled due to the mocked addEventListener. The mouse position will remain at (0, 0) instead of the intended edge position.
Apply the same fix as suggested for lines 110-126: capture the listener in the mock and invoke it directly rather than using document.dispatchEvent.
Additionally, the assertion at line 168 uses a 1.5x buffer which seems arbitrary. Consider either:
- Tightening the bound if you want to verify optimization (e.g., 1.1x)
- Using a more principled upper bound based on the grid layout (tiles are rectangular, not circular)
- Adding a comment explaining why 1.5x is the expected overhead
🤖 Prompt for AI Agents
In tests/NightModeLayer.test.ts around lines 150 to 169, the dispatched
MouseEvent won't reach the handler because addEventListener is mocked; capture
the mousemove listener from the mock addEventListener when setting up the test
and invoke that captured callback directly with a synthetic event having
clientX/clientY at the screen edge so the layer sees the intended mouse
position; then adjust the assertion guard — either tighten the multiplier to 1.1
if you want a stricter bound or replace the magic 1.5x with a computed upper
bound based on tile width/height (or add a comment explaining why 1.5x is
acceptable) and update the expect accordingly.
Comment on lines
1
to
99
| import { UserSettingModal } from "../src/client/UserSettingModal"; | ||
|
|
||
| describe("UserSettingModal - Night Mode", () => { | ||
| let modal: UserSettingModal; | ||
|
|
||
| beforeEach(() => { | ||
| // Mock localStorage | ||
| const localStorageMock = (() => { | ||
| let store: { [key: string]: string } = {}; | ||
| return { | ||
| getItem: (key: string) => store[key] || null, | ||
| setItem: (key: string, value: string) => { | ||
| store[key] = value; | ||
| }, | ||
| clear: () => { | ||
| store = {}; | ||
| }, | ||
| }; | ||
| })(); | ||
| Object.defineProperty(global, "localStorage", { | ||
| value: localStorageMock, | ||
| writable: true, | ||
| }); | ||
|
|
||
| // Mock classList | ||
| const classListMock = { | ||
| add: jest.fn(), | ||
| remove: jest.fn(), | ||
| }; | ||
| Object.defineProperty(document.documentElement, "classList", { | ||
| value: classListMock, | ||
| writable: true, | ||
| }); | ||
|
|
||
| modal = new UserSettingModal(); | ||
| localStorage.clear(); | ||
| }); | ||
|
|
||
| test("toggleNightMode enables night mode on valid toggle event", () => { | ||
| const event = new CustomEvent("toggle", { | ||
| detail: { checked: true }, | ||
| }); | ||
|
|
||
| modal.toggleNightMode(event); | ||
|
|
||
| expect(localStorage.getItem("settings.nightMode")).toBe("true"); | ||
| expect(document.documentElement.classList.add).toHaveBeenCalledWith( | ||
| "night", | ||
| ); | ||
| }); | ||
|
|
||
| test("toggleNightMode disables night mode on valid toggle event", () => { | ||
| localStorage.setItem("settings.nightMode", "true"); | ||
| const event = new CustomEvent("toggle", { | ||
| detail: { checked: false }, | ||
| }); | ||
|
|
||
| modal.toggleNightMode(event); | ||
|
|
||
| expect(localStorage.getItem("settings.nightMode")).toBe("false"); | ||
| expect(document.documentElement.classList.remove).toHaveBeenCalledWith( | ||
| "night", | ||
| ); | ||
| }); | ||
|
|
||
| test("toggleNightMode dispatches night-mode-changed event", () => { | ||
| const dispatchSpy = jest.spyOn(modal, "dispatchEvent"); | ||
| const event = new CustomEvent("toggle", { | ||
| detail: { checked: true }, | ||
| }); | ||
|
|
||
| modal.toggleNightMode(event); | ||
|
|
||
| expect(dispatchSpy).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| type: "night-mode-changed", | ||
| detail: { nightMode: true }, | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| test("toggleNightMode handles invalid event payload gracefully", () => { | ||
| const consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(); | ||
| const event = new CustomEvent("toggle", { | ||
| detail: { checked: "invalid" }, // Wrong type | ||
| }) as any; | ||
|
|
||
| modal.toggleNightMode(event); | ||
|
|
||
| expect(consoleWarnSpy).toHaveBeenCalledWith( | ||
| "Unexpected toggle event payload", | ||
| event, | ||
| ); | ||
| // Should not modify localStorage | ||
| expect(localStorage.getItem("settings.nightMode")).toBeNull(); | ||
|
|
||
| consoleWarnSpy.mockRestore(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Configure Jest to support ES module imports and DOM manipulation.
The pipeline failure indicates Jest cannot parse ES module syntax. This requires proper Jest configuration for TypeScript/ES modules plus DOM support.
Add or update your Jest configuration:
+ preset: 'ts-jest',
+ testEnvironment: 'jsdom',
+ transform: {
+ '^.+\\.tsx?$': 'ts-jest',
+ },Alternatively, if using Babel:
+ testEnvironment: 'jsdom',
+ transform: {
+ '^.+\\.(js|jsx|ts|tsx)$': 'babel-jest',
+ },Ensure the necessary dependencies are installed: npm install -D jest ts-jest @types/jest jsdom (or equivalent for Babel).
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: 🧪 CI
[error] Support for ES module syntax (import statements) missing in Jest configuration, causing syntax error: Cannot use import statement outside a module.
🤖 Prompt for AI Agents
In tests/NightModeUserSettingModal.test.ts around lines 1 to 99, the test
failures are caused by Jest not configured for ES modules/TypeScript and DOM
APIs; update Jest config to use a transformer (ts-jest or babel-jest) that
handles ES modules/TS, set testEnvironment to "jsdom", and if using Node ESM set
"extensionsToTreatAsEsm" or use babel-jest with "presets":
["@babel/preset-env","@babel/preset-typescript"] so imports parse correctly;
install and add dev-deps (jest, ts-jest or babel-jest, @types/jest, jsdom) and
adjust package.json scripts to run jest; after changing config, re-run tests to
validate DOM APIs (document.documentElement.classList) are supported.
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
tests/NightModeLayer.test.ts (5)
145-161: Mousemove event isn't reaching the layer's handler.The mock at line 50 (
document.addEventListener = jest.fn()) prevents the layer's mousemove handler from being registered. Whendocument.dispatchEvent(mouseEvent)is called at line 154, the event isn't handled, leaving the mouse position at (0, 0) rather than the intended (960, 540).To fix, capture and invoke the listener directly:
+ let mouseMoveHandler: ((event: MouseEvent) => void) | undefined; + // Mock document const classListMock = { add: jest.fn(), remove: jest.fn(), }; const documentMock = { documentElement: { classList: classListMock, }, - addEventListener: jest.fn(), + addEventListener: jest.fn((event: string, handler: any) => { + if (event === 'mousemove') { + mouseMoveHandler = handler; + } + }), dispatchEvent: jest.fn(), };Then in the test:
// Simulate mouse move to center const mouseEvent = new MouseEvent("mousemove", { clientX: 960, clientY: 540, }); - document.dispatchEvent(mouseEvent); + mouseMoveHandler?.(mouseEvent);
162-183: Performance threshold is too strict and environment-dependent.The 16ms threshold at line 182 will cause flaky failures on slower CI runners or under system load, as rendering performance varies significantly across environments.
Consider making this test informational rather than asserting a hard limit:
- // Rendering should complete in under 16ms (60fps) - expect(endTime - startTime).toBeLessThan(16); + // Log rendering time for performance tracking + const duration = endTime - startTime; + console.log(`4K render time: ${duration.toFixed(2)}ms`); + // Ensure rendering completes (generous threshold for CI) + expect(duration).toBeLessThan(100);
185-204: Same mousemove event handling issue.The mousemove event dispatched at line 194 won't update the layer's mouse position due to the mocked
addEventListener. Apply the same listener-capturing fix suggested for lines 145-161.Additionally, the 1.5x buffer at line 203 seems arbitrary—consider either tightening it (e.g., 1.1x) or adding a comment explaining why this overhead is expected.
205-217: Mousemove event won't update internal state.The mousemove event at line 214 won't update the layer's internal mouse position due to the mocked
addEventListener. Apply the listener-capturing fix suggested for lines 145-161.
219-241: Mock reassignment after construction has no effect.The
mockTransformHandleris reassigned at lines 223-231 after the layer is constructed at line 221. The layer holds a reference to the original mock, so this reassignment is ineffective.Either remove the reassignment if the original mock is sufficient:
test("handles negative mouse coordinates", () => { localStorage.setItem("settings.nightMode", "true"); layer = new NightModeLayer(mockTransformHandler); - mockTransformHandler = { - width: jest.fn().mockReturnValue(1920), - boundingRect: jest.fn().mockReturnValue({ - left: 0, - top: 0, - height: 1080, - }), - scale: 20, - } as any; - // Mouse outside canvas (negative relative position)Or move it before construction if different values are needed:
test("handles negative mouse coordinates", () => { localStorage.setItem("settings.nightMode", "true"); + + mockTransformHandler = { + width: jest.fn().mockReturnValue(1920), + boundingRect: jest.fn().mockReturnValue({ + left: 0, + top: 0, + height: 1080, + }), + scale: 20, + } as any; layer = new NightModeLayer(mockTransformHandler);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
tests/NightModeLayer.test.ts(1 hunks)tests/NightModeUserSettingModal.test.ts(1 hunks)tests/NightModeUserSettings.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/NightModeUserSettings.test.ts (1)
src/core/game/UserSettings.ts (1)
UserSettings(6-214)
tests/NightModeLayer.test.ts (2)
src/client/graphics/layers/NightModeLayer.ts (1)
NightModeLayer(6-84)src/client/graphics/TransformHandler.ts (1)
TransformHandler(15-333)
🪛 Biome (2.1.2)
tests/NightModeUserSettingModal.test.ts
[error] 18-18: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (5)
tests/NightModeUserSettings.test.ts (1)
1-86: Test coverage is comprehensive and well-structured.The test suite effectively validates:
- Default night mode state
- Reading from localStorage
- Toggling behavior with proper classList and localStorage updates
- State persistence across toggles
The mock setup is clean and isolates the unit under test appropriately.
tests/NightModeUserSettingModal.test.ts (1)
71-131: Test cases thoroughly validate night mode toggle behavior.The tests properly cover:
- Valid toggle events (enable/disable)
- Custom event dispatching with correct detail payload
- Graceful handling of invalid payloads with warning
tests/NightModeLayer.test.ts (3)
79-106: Tests correctly validate initialization behavior.These tests properly verify:
- The "night" CSS class is added/removed based on localStorage state
- The mousemove event listener is registered during construction
108-143: Rendering tests are well-structured.The disabled rendering test validates the early return path, and the dark overlay test uses a clever fillStyle tracking approach to verify both the rendering order and the correct RGBA values.
243-251: Good defensive edge case test.Testing zero scale prevents potential division-by-zero bugs and ensures graceful handling of degenerate transform states.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/NightModeUserSettingModal.test.ts(1 hunks)
🔇 Additional comments (3)
tests/NightModeUserSettingModal.test.ts (3)
1-26: Mock setup looks good; shadowing issue resolved.The mocks for
litandlit/decorators.jsare minimal but sufficient for unit testingUserSettingModalin a Node environment. The parameter shadowing issue from the previous review (line 18) has been correctly addressed by renamingconstructortoctor.
71-112: LGTM! Core scenarios are well covered.The three main test cases effectively validate:
- Enabling night mode (localStorage update + CSS class addition)
- Disabling night mode (localStorage update + CSS class removal)
- Event dispatch with correct payload
The test structure is clear, and expectations are precise.
32-69: Test setup correctly instantiates UserSettingModal.Verification confirms that
UserSettingModaldoes not define an explicit constructor and therefore uses the implicit default no-arg constructor. The class extendsLitElementand initializes fields at instance creation time. The test setup on line 67 (modal = new UserSettingModal();) is correct and will function as intended with the providedlocalStorageanddocumentmocks.
Comment on lines
+114
to
+130
| test("toggleNightMode handles invalid event payload gracefully", () => { | ||
| const consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(); | ||
| const event = new CustomEvent("toggle", { | ||
| detail: { checked: "invalid" }, // Wrong type | ||
| }) as any; | ||
|
|
||
| modal.toggleNightMode(event); | ||
|
|
||
| expect(consoleWarnSpy).toHaveBeenCalledWith( | ||
| "Unexpected toggle event payload", | ||
| event, | ||
| ); | ||
| // Should not modify localStorage | ||
| expect(localStorage.getItem("settings.nightMode")).toBeNull(); | ||
|
|
||
| consoleWarnSpy.mockRestore(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider additional edge case tests for robustness.
The invalid payload test correctly validates defensive behavior (warning + no state change). To further strengthen test coverage, consider adding tests for:
event.detailisnullorundefinedevent.detail.checkedis other falsy values (null,undefined,0,"")eventitself isnull/undefined(if the method is exposed publicly)- Toggling night mode multiple times in sequence (idempotency)
These additions would ensure toggleNightMode handles all edge cases gracefully.
Example additional test:
test("toggleNightMode handles missing detail gracefully", () => {
const consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation();
const event = new CustomEvent("toggle", {
detail: undefined,
}) as any;
modal.toggleNightMode(event);
expect(consoleWarnSpy).toHaveBeenCalled();
expect(localStorage.getItem("settings.nightMode")).toBeNull();
consoleWarnSpy.mockRestore();
});🤖 Prompt for AI Agents
In tests/NightModeUserSettingModal.test.ts around lines 114 to 130, add
additional unit tests to cover missing/invalid payload edge cases and
idempotency: create separate tests that pass event.detail as null/undefined,
event.detail.checked as other falsy values (null, undefined, 0, ""), and event
itself as null/undefined (if public), asserting console.warn is called and
localStorage remains unchanged; also add a test that toggles night mode multiple
times in sequence asserting state flips only when valid checked values are
provided and repeated toggles are idempotent; for each test spy on console.warn,
call modal.toggleNightMode with the specific invalid input, assert the warning
and no localStorage mutation (or correct mutation for valid toggles), and
restore the spy after each test.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/client/graphics/GameRenderer.ts(3 hunks)src/client/graphics/layers/NightModeLayer.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/NightModeLayer.tssrc/client/graphics/GameRenderer.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/GameRenderer.ts
🧬 Code graph analysis (2)
src/client/graphics/layers/NightModeLayer.ts (3)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/UserSettings.ts (1)
UserSettings(6-214)src/client/graphics/TransformHandler.ts (1)
TransformHandler(15-333)
src/client/graphics/GameRenderer.ts (1)
src/client/graphics/layers/NightModeLayer.ts (1)
NightModeLayer(7-180)
🪛 GitHub Actions: 🧪 CI
src/client/graphics/GameRenderer.ts
[error] 326-326: TS2339: Property 'setGame' does not exist on type 'typeof NightModeLayer'.
🔇 Additional comments (1)
src/client/graphics/layers/NightModeLayer.ts (1)
15-15: Inject UserSettings instead of creating a new instance.Creating a new
UserSettingsinstance here could lead to inconsistency if settings are modified through other instances elsewhere in the codebase. The settings should be shared.Accept
UserSettingsas a constructor parameter, similar to how other layers receive their dependencies:constructor( private transformHandler: TransformHandler, + private userSettings: UserSettings, game?: GameView, ) { this.game = game ?? null; - if (this.userSettingsInstance.nightMode()) { + if (this.userSettings.nightMode()) { document.documentElement.classList.add("night"); } else { document.documentElement.classList.remove("night"); } document.addEventListener("mousemove", this.mouseMoveHandler); }Update the instantiation in GameRenderer.ts accordingly:
-new NightModeLayer(transformHandler), +new NightModeLayer(transformHandler, userSettings, game),Then update
renderLayerto usethis.userSettingsinstead ofthis.userSettingsInstance.⛔ Skipped due to learnings
Learnt from: devalnor Repo: openfrontio/OpenFrontIO PR: 1195 File: src/client/graphics/layers/AlertFrame.ts:18-18 Timestamp: 2025-06-20T20:11:00.965Z Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.Learnt from: devalnor Repo: openfrontio/OpenFrontIO PR: 1195 File: src/client/graphics/layers/AlertFrame.ts:18-18 Timestamp: 2025-06-20T20:11:00.965Z Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in components like OptionsMenu, EventsDisplay, and AlertFrame. This is the established pattern and should be followed for consistency rather than using dependency injection.
Comment on lines
+164
to
+178
| for (let y = startY; y < endY; y += cellSize) { | ||
| for (let x = startX; x < endX; x += cellSize) { | ||
| const dist = Math.hypot( | ||
| (this.mouseX - (x + cellSize / 2)) / cellSize, | ||
| (this.mouseY - (y + cellSize / 2)) / cellSize, | ||
| ); | ||
|
|
||
| const brightness = Math.max(0, 1 - dist / this.flashlightRadius); | ||
|
|
||
| if (brightness > 0) { | ||
| context.fillStyle = `rgba(200,200,130,${(this.darkenAlpha / 2) * brightness})`; | ||
| context.fillRect(x, y, cellSize, cellSize); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Performance consideration: flashlight rendering complexity.
Similar to city glow, the flashlight effect uses nested loops that could iterate many times per frame depending on flashlightRadius and cellSize. While this is for a single cursor location (unlike multiple cities), it still runs every frame.
Consider using a radial gradient for better performance:
private renderFlashlight(
context: CanvasRenderingContext2D,
width: number,
height: number,
cellSize: number,
): void {
const radius = this.flashlightRadius * cellSize;
const gradient = context.createRadialGradient(
this.mouseX, this.mouseY, 0,
this.mouseX, this.mouseY, radius
);
gradient.addColorStop(0, `rgba(200, 200, 130, ${this.darkenAlpha / 2})`);
gradient.addColorStop(1, 'rgba(200, 200, 130, 0)');
context.fillStyle = gradient;
context.fillRect(
Math.max(0, this.mouseX - radius),
Math.max(0, this.mouseY - radius),
Math.min(width, radius * 2),
Math.min(height, radius * 2)
);
}This eliminates the nested loops while achieving a similar visual effect with much better performance.
🤖 Prompt for AI Agents
In src/client/graphics/layers/NightModeLayer.ts around lines 164 to 178, the
current flashlight implementation iterates nested loops per frame which is
expensive; replace the cell-by-cell drawing with a single canvas radial
gradient: create a radial gradient centered at (this.mouseX, this.mouseY) with
inner radius 0 and outer radius this.flashlightRadius * cellSize, add two color
stops (opaque-ish at 0 using rgba(200,200,130,this.darkenAlpha/2) and fully
transparent at 1), set context.fillStyle to that gradient, and fill only the
bounding rectangle covering the gradient (clamped to canvas bounds) so the
effect matches visually but avoids per-cell loops for much better performance.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/graphics/GameRenderer.ts (2)
246-287: Critical: nightModeLayer is undefined when passed to GameRenderer constructor.At line 246,
NightModeLayeris constructed inline within thelayersarray, but at line 287 you're passingnightModeLayeras a parameter, which is never defined. This will cause a ReferenceError at runtime.Apply this diff to fix:
const samRadiusLayer = new SAMRadiusLayer( game, eventBus, transformHandler, uiState, ); + + const nightModeLayer = new NightModeLayer(game, transformHandler); const fpsDisplay = document.querySelector("fps-display") as FPSDisplay; // ... rest of fpsDisplay setup ... const layers: Layer[] = [ new TerrainLayer(game, transformHandler), new TerritoryLayer(game, eventBus, transformHandler, userSettings), new RailroadLayer(game, transformHandler), structureLayer, samRadiusLayer, new UnitLayer(game, eventBus, transformHandler), new FxLayer(game), new UILayer(game, eventBus, transformHandler), new StructureIconsLayer(game, eventBus, uiState, transformHandler), - new NightModeLayer(game, transformHandler), + nightModeLayer, new NameLayer(game, transformHandler, eventBus),
293-330: Redundant setGame call after passing game to constructor.At line 246,
gameis passed to theNightModeLayerconstructor, but at line 330,setGame(this.game)is called again. This is redundant since the layer already has the game reference from construction.Consider removing the
setGamecall if the game reference doesn't change:this.canvas.addEventListener("contextrestored", () => { this.redraw(); rafId = requestAnimationFrame(() => this.renderGame()); }); - this.nightModeLayer.setGame(this.game); }Alternatively, if
setGameserves a purpose (e.g., the game reference might be updated), document why it's needed despite being passed at construction.
♻️ Duplicate comments (10)
tests/NightModeLayer.test.ts (5)
145-161: Critical: mouseMoveHandler is undefined.At line 154, the test references
mouseMoveHandlerwhich is never defined. This test will fail with a ReferenceError.The past review comment suggested capturing the listener from the mock. Apply this fix:
beforeEach(() => { + let mouseMoveHandler: ((event: MouseEvent) => void) | undefined; + // Mock document const classListMock = { add: jest.fn(), remove: jest.fn(), }; const documentMock = { documentElement: { classList: classListMock, }, - addEventListener: jest.fn(), + addEventListener: jest.fn((event: string, handler: any) => { + if (event === 'mousemove') { + mouseMoveHandler = handler; + } + }), dispatchEvent: jest.fn(), };Then the test at line 154 will work correctly. You'll also need to make
mouseMoveHandleraccessible in the test scope (e.g., declare it outside beforeEach).
162-183: Performance test with hard 16ms threshold will be flaky on CI.The test expects rendering to complete in under 16ms (60fps), but this is environment-dependent and will cause false failures on slower CI runners or under system load.
Apply one of these fixes:
- Remove the hard threshold (recommended):
const startTime = performance.now(); layer.renderLayer(mockContext); const endTime = performance.now(); - // Rendering should complete in under 16ms (60fps) - expect(endTime - startTime).toBeLessThan(16); + // Log rendering time for monitoring (no hard assertion) + console.info(`4K rendering time: ${endTime - startTime}ms`); + expect(endTime - startTime).toBeLessThan(200); // Generous sanity check
- Make it optional based on environment variable for dedicated performance testing.
185-204: Mouse event won't be handled due to mocked addEventListener.At line 194,
document.dispatchEvent(mouseEvent)is called, but becauseaddEventListeneris mocked as a simplejest.fn(), the layer's mousemove handler was never actually registered. The event dispatch will have no effect, and the mouse position will remain at (0, 0).Apply the same fix suggested for lines 145-161: capture the listener in the
addEventListenermock and invoke it directly instead of usingdispatchEvent.
205-217: Same event handling issue—mouse position won't update.The
dispatchEventat line 214 won't update the layer's internal mouse position due to the mockedaddEventListener.Apply the listener-capturing fix suggested for lines 145-161.
219-241: mockTransformHandler reassignment after layer construction has no effect.At lines 223-231,
mockTransformHandleris reassigned after the layer is constructed at line 221. The layer holds a reference to the original mock, so this reassignment is ineffective.Either move the reassignment before construction:
test("handles negative mouse coordinates", () => { localStorage.setItem("settings.nightMode", "true"); - layer = new NightModeLayer(mockTransformHandler); mockTransformHandler = { width: jest.fn().mockReturnValue(1920), boundingRect: jest.fn().mockReturnValue({ left: 0, top: 0, height: 1080, }), scale: 20, } as any; + layer = new NightModeLayer(mockTransformHandler);Or remove it if the original mock values are sufficient.
src/client/graphics/layers/NightModeLayer.ts (5)
25-30: Memory leak: mousemove listener is never cleaned up.The event listener registered at line 29 is never removed. If
NightModeLayerinstances are created/destroyed during the application lifecycle, these listeners will accumulate.Based on learnings, the Layer system doesn't have a disconnectedCallback or unmount mechanism, but you should still add cleanup capability:
+ private mouseMoveHandler = (e: MouseEvent) => this.handleMouseMove(e); + constructor( private game: GameView | null, private transformHandler: TransformHandler, ) { - document.addEventListener("mousemove", (e) => this.handleMouseMove(e)); + document.addEventListener("mousemove", this.mouseMoveHandler); } + + // Call this method when the layer is no longer needed + destroy(): void { + document.removeEventListener("mousemove", this.mouseMoveHandler); + }Note: Even though the Layer interface doesn't mandate a
destroymethod (as per learnings), adding it allows manual cleanup when needed. The GameRenderer should call this when appropriate.
32-33: Remove empty comment.Lines 32-33 contain an incomplete comment with no content. Remove it.
- // New method to set game reference after construction - renderLayer(context: CanvasRenderingContext2D): void {
58-81: City lights won't follow map pan—use worldToScreenCoordinates.Lines 72-73 convert tile coordinates to screen by simply multiplying by
cellSize, which doesn't account for the pan offset. When users pan the map, city lights will remain stationary instead of moving with the map.Use
TransformHandler.worldToScreenCoordinates()like other layers:+ import { Cell } from "../../../core/game/Cell"; // Add import if not present + for (const city of cities) { const tileRef = city.tile(); const cityX = this.game!.x(tileRef); const cityY = this.game!.y(tileRef); - // Convert tile coordinates to screen coordinates - const screenX = cityX * cellSize; - const screenY = cityY * cellSize; + // Convert world coordinates to screen coordinates (accounts for pan and zoom) + const screenPos = this.transformHandler.worldToScreenCoordinates( + new Cell(cityX, cityY) + ); + const screenX = screenPos.x; + const screenY = screenPos.y; const cityLevel = city.level(); this.renderCityGlow(context, screenX, screenY, cellSize, cityLevel); }
87-119: Performance: nested loops scale quadratically with city level.The nested loops iterate
(2 * lightRadius + 1)²times per city. For a level-5 city, that's 961 iterations. With many cities rendering every frame, this will degrade performance.Use a radial gradient to eliminate the loops:
private renderCityGlow( context: CanvasRenderingContext2D, x: number, y: number, cellSize: number, level: number, ): void { - // Example 1: Simple bright square (like a satellite image) - const lightRadius = 5 + level * 2; // Larger cities have bigger glow - - for (let dy = -lightRadius; dy <= lightRadius; dy++) { - for (let dx = -lightRadius; dx <= lightRadius; dx++) { - const distance = Math.sqrt(dx * dx + dy * dy); - if (distance <= lightRadius) { - // Brightness decreases with distance - const brightness = 1 - distance / lightRadius; - const alpha = this.darkenAlpha * 0.8 * brightness; - - context.fillStyle = `rgba(255, 220, 150, ${alpha})`; - context.fillRect( - x + dx * cellSize, - y + dy * cellSize, - cellSize, - cellSize, - ); - } - } - } - - // Example 2: Add a brighter core - context.fillStyle = `rgba(255, 255, 200, ${this.darkenAlpha * 0.9})`; - context.fillRect(x, y, cellSize, cellSize); + const lightRadius = (5 + level * 2) * cellSize; + const gradient = context.createRadialGradient( + x + cellSize / 2, y + cellSize / 2, 0, + x + cellSize / 2, y + cellSize / 2, lightRadius + ); + gradient.addColorStop(0, `rgba(255, 255, 200, ${this.darkenAlpha * 0.9})`); + gradient.addColorStop(0.5, `rgba(255, 220, 150, ${this.darkenAlpha * 0.6})`); + gradient.addColorStop(1, 'rgba(255, 220, 150, 0)'); + + context.fillStyle = gradient; + context.fillRect(x - lightRadius, y - lightRadius, lightRadius * 2, lightRadius * 2); }
125-166: Performance: flashlight rendering uses expensive nested loops.While this is for a single cursor location (unlike multiple cities), the nested loops at lines 151-165 iterate many times per frame. Though less critical than city glow, this still impacts performance unnecessarily.
Use a radial gradient for better performance:
private renderFlashlight( context: CanvasRenderingContext2D, width: number, height: number, cellSize: number, ): void { - const startX = - Math.floor( - Math.max(this.mouseX - this.flashlightRadius * cellSize, 0) / cellSize, - ) * cellSize; - const endX = - Math.ceil( - Math.min(this.mouseX + this.flashlightRadius * cellSize, width) / - cellSize, - ) * cellSize; - - const startY = - Math.floor( - Math.max(this.mouseY - this.flashlightRadius * cellSize, 0) / cellSize, - ) * cellSize; - const endY = - Math.ceil( - Math.min(this.mouseY + this.flashlightRadius * cellSize, height) / - cellSize, - ) * cellSize; - - for (let y = startY; y < endY; y += cellSize) { - for (let x = startX; x < endX; x += cellSize) { - const dist = Math.hypot( - (this.mouseX - (x + cellSize / 2)) / cellSize, - (this.mouseY - (y + cellSize / 2)) / cellSize, - ); - - const brightness = Math.max(0, 1 - dist / this.flashlightRadius); - - if (brightness > 0) { - context.fillStyle = `rgba(200,200,130,${(this.darkenAlpha / 2) * brightness})`; - context.fillRect(x, y, cellSize, cellSize); - } - } - } + const radius = this.flashlightRadius * cellSize; + const gradient = context.createRadialGradient( + this.mouseX, this.mouseY, 0, + this.mouseX, this.mouseY, radius + ); + + gradient.addColorStop(0, `rgba(200, 200, 130, ${this.darkenAlpha / 2})`); + gradient.addColorStop(1, 'rgba(200, 200, 130, 0)'); + + context.fillStyle = gradient; + context.fillRect( + Math.max(0, this.mouseX - radius), + Math.max(0, this.mouseY - radius), + Math.min(width - this.mouseX + radius, radius * 2), + Math.min(height - this.mouseY + radius, radius * 2) + ); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/client/graphics/GameRenderer.ts(5 hunks)src/client/graphics/layers/NightModeLayer.ts(1 hunks)tests/NightModeLayer.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/NightModeLayer.tssrc/client/graphics/GameRenderer.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/NightModeLayer.tssrc/client/graphics/GameRenderer.ts
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/graphics/layers/NightModeLayer.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/client/graphics/layers/NightModeLayer.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/GameRenderer.ts
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/client/graphics/GameRenderer.ts
🧬 Code graph analysis (3)
src/client/graphics/layers/NightModeLayer.ts (5)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/UserSettings.ts (1)
UserSettings(6-214)src/core/game/GameView.ts (1)
GameView(448-774)src/client/graphics/TransformHandler.ts (1)
TransformHandler(15-333)src/core/game/UnitImpl.ts (2)
trajectoryIndex(363-365)trajectory(367-369)
src/client/graphics/GameRenderer.ts (1)
src/client/graphics/layers/NightModeLayer.ts (1)
NightModeLayer(7-210)
tests/NightModeLayer.test.ts (2)
src/client/graphics/layers/NightModeLayer.ts (2)
NightModeLayer(7-210)init(21-21)src/client/graphics/TransformHandler.ts (1)
TransformHandler(15-333)
🔇 Additional comments (3)
src/client/graphics/GameRenderer.ts (1)
26-26: LGTM!The import is correctly placed and follows the existing import pattern.
src/client/graphics/layers/NightModeLayer.ts (1)
15-23: LGTM!The
handleMouseMovemethod correctly converts viewport coordinates to canvas-relative coordinates, addressing the past review concern. The empty lifecycle methods are appropriate stubs for the Layer interface.tests/NightModeLayer.test.ts (1)
79-97: Tests expect CSS class toggling that NightModeLayer doesn't implement.The tests at lines 79-97 expect the
NightModeLayerconstructor to add/remove the "night" CSS class fromdocument.documentElement, but reviewing the constructor code (lines 25-30 in NightModeLayer.ts), it only registers a mousemove listener and doesn't manipulate the CSS class.Based on the past review comments and the UserSettings.ts snippet, CSS class management should be handled by
UserSettings.toggleNightMode(), not by the layer constructor.Remove these tests or update them to reflect the actual behavior:
- test("adds 'night' class to document when night mode enabled", () => { - localStorage.setItem("settings.nightMode", "true"); - - layer = new NightModeLayer(mockTransformHandler); - - expect(document.documentElement.classList.add).toHaveBeenCalledWith( - "night", - ); - }); - - test("removes 'night' class from document when night mode disabled", () => { - localStorage.setItem("settings.nightMode", "false"); - - layer = new NightModeLayer(mockTransformHandler); - - expect(document.documentElement.classList.remove).toHaveBeenCalledWith( - "night", - ); - });If CSS class management is needed in tests, it should be tested in the UserSettings test suite instead.
Likely an incorrect or invalid review comment.
Author
|
@coderabbitai Could you help with the creation of a mock game? (in tests) |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20035 @kkissdev Absolutely! Here's a comprehensive guide for creating mock game objects for testing your NightModeLayer. Mock Game Factory for TestsCreate a test utility file import { GameView } from "../../src/core/game/GameView";
import { Unit, UnitType } from "../../src/core/game/Game";
import { TileRef } from "../../src/core/game/GameMap";
/**
* Creates a mock Unit object for testing.
* Useful for mocking cities, SAMs, nukes, etc.
*/
export function createMockUnit(
type: UnitType,
x: number,
y: number,
level: number = 1,
id: number = Math.floor(Math.random() * 10000)
): jest.Mocked<Unit> {
const tileRef = x * 1000 + y; // Simple tile ref encoding
return {
id: jest.fn().mockReturnValue(id),
type: jest.fn().mockReturnValue(type),
tile: jest.fn().mockReturnValue(tileRef),
level: jest.fn().mockReturnValue(level),
lastTile: jest.fn().mockReturnValue(tileRef),
isUnit: jest.fn().mockReturnValue(true),
isActive: jest.fn().mockReturnValue(true),
isMarkedForDeletion: jest.fn().mockReturnValue(false),
isOverdueDeletion: jest.fn().mockReturnValue(false),
hasTrainStation: jest.fn().mockReturnValue(false),
// Add other methods as needed with reasonable defaults
owner: jest.fn(),
info: jest.fn(),
markForDeletion: jest.fn(),
delete: jest.fn(),
move: jest.fn(),
setOwner: jest.fn(),
touch: jest.fn(),
hash: jest.fn().mockReturnValue(id),
toUpdate: jest.fn(),
} as any;
}
/**
* Creates a mock GameView object for testing.
* Allows you to specify cities and other units.
*/
export function createMockGameView(
cities: Array<{ x: number; y: number; level?: number }> = [],
otherUnits: Record<UnitType, Array<{ x: number; y: number }>> = {}
): jest.Mocked<Partial<GameView>> {
// Create mock city units
const cityUnits = cities.map((city, index) =>
createMockUnit(UnitType.City, city.x, city.y, city.level ?? 1, index)
);
// Create other units (SAMs, nukes, etc.)
const allUnits: Unit[] = [...cityUnits];
Object.entries(otherUnits).forEach(([unitType, positions]) => {
positions.forEach((pos, index) => {
allUnits.push(
createMockUnit(
parseInt(unitType) as UnitType,
pos.x,
pos.y,
1,
1000 + index
)
);
});
});
// Mock the units() method to filter by type
const unitsMock = jest.fn((type: UnitType) => {
if (type === UnitType.City) {
return cityUnits;
}
return allUnits.filter((u) => u.type() === type);
});
// Mock x() and y() methods to decode TileRef
const xMock = jest.fn((tileRef: TileRef) => Math.floor(tileRef / 1000));
const yMock = jest.fn((tileRef: TileRef) => tileRef % 1000);
// Mock ref() to encode x,y to TileRef
const refMock = jest.fn((x: number, y: number) => x * 1000 + y);
return {
units: unitsMock,
x: xMock,
y: yMock,
ref: refMock,
} as any;
}
/**
* Creates a simple mock game with a grid of cities.
* Useful for quick test setup.
*/
export function createMockGameWithCityGrid(
rows: number = 3,
cols: number = 3,
spacing: number = 10
): jest.Mocked<Partial<GameView>> {
const cities = [];
for (let row = 0; row < rows; row++) {
for (let col = 0; col < cols; col++) {
cities.push({
x: col * spacing,
y: row * spacing,
level: Math.floor(Math.random() * 5) + 1, // Random level 1-5
});
}
}
return createMockGameView(cities);
}Usage in Your NightModeLayer TestsUpdate import { NightModeLayer } from "../src/client/graphics/layers/NightModeLayer";
import { TransformHandler } from "../src/client/graphics/TransformHandler";
import { UnitType } from "../src/core/game/Game";
import {
createMockGameView,
createMockGameWithCityGrid,
} from "./util/MockGame";
describe("NightModeLayer with Cities", () => {
let layer: NightModeLayer;
let mockTransformHandler: jest.Mocked<TransformHandler>;
let mockContext: jest.Mocked<CanvasRenderingContext2D>;
beforeEach(() => {
// Setup localStorage mock
const localStorageMock = (() => {
let store: { [key: string]: string } = {};
return {
getItem: (key: string) => store[key] || null,
setItem: (key: string, value: string) => {
store[key] = value;
},
clear: () => {
store = {};
},
};
})();
Object.defineProperty(global, "localStorage", {
value: localStorageMock,
writable: true,
});
// Mock document
const classListMock = {
add: jest.fn(),
remove: jest.fn(),
};
Object.defineProperty(document.documentElement, "classList", {
value: classListMock,
writable: true,
});
document.addEventListener = jest.fn();
// Mock TransformHandler
mockTransformHandler = {
width: jest.fn().mockReturnValue(1920),
boundingRect: jest.fn().mockReturnValue({
left: 0,
top: 0,
height: 1080,
}),
scale: 20,
} as any;
// Mock Canvas Context
mockContext = {
fillStyle: "",
fillRect: jest.fn(),
createRadialGradient: jest.fn().mockReturnValue({
addColorStop: jest.fn(),
}),
} as any;
localStorage.clear();
});
test("renders city lights when night mode enabled", () => {
localStorage.setItem("settings.nightMode", "true");
// Create a mock game with 3 cities
const mockGame = createMockGameView([
{ x: 10, y: 10, level: 1 },
{ x: 20, y: 15, level: 3 },
{ x: 30, y: 20, level: 5 },
]);
layer = new NightModeLayer(mockTransformHandler, mockGame as any);
layer.renderLayer(mockContext);
// Should render dark overlay + city lights + flashlight
expect(mockContext.fillRect).toHaveBeenCalled();
// Verify cities were queried
expect(mockGame.units).toHaveBeenCalledWith(UnitType.City);
// Verify x() and y() were called to get city positions
expect(mockGame.x).toHaveBeenCalled();
expect(mockGame.y).toHaveBeenCalled();
});
test("renders larger glow for higher level cities", () => {
localStorage.setItem("settings.nightMode", "true");
const mockGame = createMockGameView([
{ x: 10, y: 10, level: 1 }, // Small city
{ x: 20, y: 20, level: 5 }, // Large city
]);
layer = new NightModeLayer(mockTransformHandler, mockGame as any);
mockContext.fillRect.mockClear();
layer.renderLayer(mockContext);
// Count how many light tiles were rendered
// Higher level cities should have more tiles illuminated
const fillRectCalls = mockContext.fillRect.mock.calls;
expect(fillRectCalls.length).toBeGreaterThan(1);
});
test("handles game with no cities gracefully", () => {
localStorage.setItem("settings.nightMode", "true");
const mockGame = createMockGameView([]); // No cities
layer = new NightModeLayer(mockTransformHandler, mockGame as any);
expect(() => layer.renderLayer(mockContext)).not.toThrow();
expect(mockGame.units).toHaveBeenCalledWith(UnitType.City);
});
test("renders multiple cities in a grid pattern", () => {
localStorage.setItem("settings.nightMode", "true");
// Create 3x3 grid of cities
const mockGame = createMockGameWithCityGrid(3, 3, 10);
layer = new NightModeLayer(mockTransformHandler, mockGame as any);
layer.renderLayer(mockContext);
// Should query for cities
expect(mockGame.units).toHaveBeenCalledWith(UnitType.City);
// Should call x() and y() 9 times (once per city)
expect(mockGame.x).toHaveBeenCalled();
expect(mockGame.y).toHaveBeenCalled();
});
test("renders city lights with SAM launchers", () => {
localStorage.setItem("settings.nightMode", "true");
const mockGame = createMockGameView(
[{ x: 10, y: 10, level: 1 }], // 1 city
{
[UnitType.SAMLauncher]: [
{ x: 15, y: 15 },
{ x: 20, y: 20 },
],
}
);
layer = new NightModeLayer(mockTransformHandler, mockGame as any);
layer.renderLayer(mockContext);
// Verify both cities and SAMs were queried
expect(mockGame.units).toHaveBeenCalledWith(UnitType.City);
// If you implement SAM lights, also check:
// expect(mockGame.units).toHaveBeenCalledWith(UnitType.SAMLauncher);
});
test("updates city lights when setGame is called", () => {
localStorage.setItem("settings.nightMode", "true");
layer = new NightModeLayer(mockTransformHandler);
// Initially no game
layer.renderLayer(mockContext);
const initialCalls = mockContext.fillRect.mock.calls.length;
// Set game with cities
const mockGame = createMockGameView([
{ x: 10, y: 10, level: 1 },
{ x: 20, y: 20, level: 3 },
]);
layer.setGame(mockGame as any);
mockContext.fillRect.mockClear();
layer.renderLayer(mockContext);
// Should now render city lights
expect(mockGame.units).toHaveBeenCalledWith(UnitType.City);
});
});Alternative: Using Real Game SetupIf you want integration tests with a real game instance, you can use the existing import { setup } from "./util/Setup";
import { PlayerInfo, PlayerType, UnitType } from "../src/core/game/Game";
import { NightModeLayer } from "../src/client/graphics/layers/NightModeLayer";
describe("NightModeLayer Integration Tests", () => {
test("renders real game cities", async () => {
// Create a real game
const game = await setup(
"plains",
{
infiniteGold: true,
instantBuild: true,
},
[new PlayerInfo("player1", PlayerType.Human, "player1", "player1")]
);
const player = game.player("player1");
player.conquer(game.ref(10, 10));
player.conquer(game.ref(20, 20));
// Build cities
// (Use your construction execution logic here)
// Create layer with real game
const mockTransformHandler = {
width: jest.fn().mockReturnValue(1920),
boundingRect: jest.fn().mockReturnValue({
left: 0,
top: 0,
height: 1080,
}),
scale: 20,
} as any;
const layer = new NightModeLayer(mockTransformHandler, game);
const mockContext = {
fillStyle: "",
fillRect: jest.fn(),
} as any;
localStorage.setItem("settings.nightMode", "true");
layer.renderLayer(mockContext);
// Verify cities were rendered
const cities = game.units(UnitType.City);
expect(cities.length).toBeGreaterThan(0);
expect(mockContext.fillRect).toHaveBeenCalled();
});
});Key BenefitsMock Approach:
Real Game Approach:
I recommend using mocks for unit tests and real game setup for integration tests. The mock factory gives you the flexibility to test edge cases quickly! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/client/graphics/layers/NightModeLayer.ts (2)
35-36: Remove leftover comment.The comment "New method to set game reference after construction" on lines 35-36 is a remnant from a previous implementation and should be removed since no method follows it.
Apply this diff:
- // New method to set game reference after construction - renderLayer(context: CanvasRenderingContext2D): void {
165-206: Consider radial gradient for better flashlight performance.The nested loops at lines 191-205 iterate over each cell in the flashlight radius every frame. With
flashlightRadius = 50andcellSize ≈ 2, this could be ~10,000 iterations per frame (100×100 grid). A previous review suggested using a radial gradient instead, which would eliminate the loops entirely.Replace the nested loops with a radial gradient:
private renderFlashlight( context: CanvasRenderingContext2D, width: number, height: number, cellSize: number, ): void { - const startX = - Math.floor( - Math.max(this.mouseX - this.flashlightRadius * cellSize, 0) / cellSize, - ) * cellSize; - const endX = - Math.ceil( - Math.min(this.mouseX + this.flashlightRadius * cellSize, width) / - cellSize, - ) * cellSize; - - const startY = - Math.floor( - Math.max(this.mouseY - this.flashlightRadius * cellSize, 0) / cellSize, - ) * cellSize; - const endY = - Math.ceil( - Math.min(this.mouseY + this.flashlightRadius * cellSize, height) / - cellSize, - ) * cellSize; - - for (let y = startY; y < endY; y += cellSize) { - for (let x = startX; x < endX; x += cellSize) { - const dist = Math.hypot( - (this.mouseX - (x + cellSize / 2)) / cellSize, - (this.mouseY - (y + cellSize / 2)) / cellSize, - ); - - const brightness = Math.max(0, 1 - dist / this.flashlightRadius); - - if (brightness > 0) { - context.fillStyle = `rgba(200,200,130,${(this.darkenAlpha / 2) * brightness})`; - context.fillRect(x, y, cellSize, cellSize); - } - } - } + const radius = this.flashlightRadius * cellSize; + const gradient = context.createRadialGradient( + this.mouseX, this.mouseY, 0, + this.mouseX, this.mouseY, radius + ); + + gradient.addColorStop(0, `rgba(200, 200, 130, ${this.darkenAlpha / 2})`); + gradient.addColorStop(1, 'rgba(200, 200, 130, 0)'); + + context.fillStyle = gradient; + const x = Math.max(0, this.mouseX - radius); + const y = Math.max(0, this.mouseY - radius); + const w = Math.min(width - x, radius * 2); + const h = Math.min(height - y, radius * 2); + context.fillRect(x, y, w, h); }This achieves the same visual effect with a single fill operation instead of thousands of iterations per frame.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
resources/lang/en.json(1 hunks)src/client/graphics/GameRenderer.ts(2 hunks)src/client/graphics/layers/NightModeLayer.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/NightModeLayer.tssrc/client/graphics/GameRenderer.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/NightModeLayer.tssrc/client/graphics/GameRenderer.ts
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/graphics/layers/NightModeLayer.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/client/graphics/layers/NightModeLayer.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/GameRenderer.ts
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/client/graphics/GameRenderer.ts
🧬 Code graph analysis (2)
src/client/graphics/layers/NightModeLayer.ts (5)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/UserSettings.ts (1)
UserSettings(6-214)src/core/game/GameView.ts (1)
GameView(448-774)src/client/graphics/TransformHandler.ts (1)
TransformHandler(15-333)src/core/game/Game.ts (1)
Cell(313-335)
src/client/graphics/GameRenderer.ts (1)
src/client/graphics/layers/NightModeLayer.ts (1)
NightModeLayer(7-210)
🪛 GitHub Actions: 🧪 CI
src/client/graphics/layers/NightModeLayer.ts
[error] 40-41: TypeError: Cannot read properties of undefined (reading 'width')
[error] 40-41: TypeError: Cannot read properties of undefined (reading 'width')
[error] 40-41: TypeError: Cannot read properties of undefined (reading 'width')
[error] 40-41: TypeError: Cannot read properties of undefined (reading 'width')
[error] 154-154: ReferenceError: mouseMoveHandler is not defined
[error] 154-154: ReferenceError: mouseMoveHandler is not defined
[error] 154-154: ReferenceError: mouseMoveHandler is not defined
[error] 40-41: TypeError: Cannot read properties of undefined (reading 'width')
[error] 40-41: TypeError: Cannot read properties of undefined (reading 'width')
🔇 Additional comments (4)
resources/lang/en.json (1)
315-316: LGTM!The translation keys are clear, well-formed, and follow the existing naming convention. The description accurately conveys that Night Mode is a purely aesthetic feature.
src/client/graphics/GameRenderer.ts (2)
25-25: LGTM!The import statement is correct and follows the project's import conventions.
248-248: LGTM!The NightModeLayer is properly instantiated with both required parameters (
gameandtransformHandler). The previous issues with static method calls have been resolved by passing the game reference directly to the constructor.src/client/graphics/layers/NightModeLayer.ts (1)
37-55: Critical: City lights won't render due to missing await, plus defensive coding needed.Three issues here:
Critical rendering bug: Line 50 calls
renderCityLights(), which in turn calls the asyncrenderCityGlow()at line 137 without awaiting it. SincerenderLayer()is synchronous, the city glows will never appear or will render unpredictably. You need to either make the entire rendering pipeline async (not recommended for canvas rendering), or switch to synchronous rendering (use cached bitmaps synchronously).Pipeline failures: The test errors show
transformHandler.width()is undefined at lines 40-41. Add defensive checks to handle cases where transformHandler might not be fully initialized.Solution 1 (Recommended): Eagerly pre-generate all glow bitmaps during initialization and use them synchronously:
+ init(): void { + // Pre-generate glow bitmaps for all city levels + this.preGenerateGlowBitmaps(); + } + + private preGenerateGlowBitmaps(): void { + const cellSize = this.transformHandler?.scale ?? 1; + for (let level = 0; level <= this.maxCityLightLevel; level++) { + this.createGlowBitmap(level, cellSize).then(bitmap => { + this.glowBitmaps.set(level, bitmap); + }); + } + } + renderLayer(context: CanvasRenderingContext2D): void { if (!this.userSettingsInstance.nightMode()) return; + if (!this.transformHandler) return; const width = this.transformHandler.width(); const height = this.transformHandler.boundingRect().height;Then make
renderCityGlowsynchronous:- private async renderCityGlow( + private renderCityGlow( context: CanvasRenderingContext2D, x: number, y: number, cellSize: number, level: number, - ): Promise<void> { - const glow = await this.getGlowBitmap(level, cellSize); + ): void { + const cappedLevel = Math.min(level, this.maxCityLightLevel); + const glow = this.glowBitmaps.get(cappedLevel); + if (!glow) return; // Not yet cached - const cappedLevel = this.maxCityLightLevel; const radius = (10 + cappedLevel * 2) * cellSize; context.drawImage(glow, x - radius, y - radius); }⛔ Skipped due to learnings
Learnt from: NewYearNewPhil Repo: openfrontio/OpenFrontIO PR: 2230 File: src/client/graphics/GameRenderer.ts:269-277 Timestamp: 2025-10-18T11:00:57.142Z Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Comment on lines
+64
to
+79
| private async getGlowBitmap( | ||
| level: number, | ||
| cellSize: number, | ||
| ): Promise<ImageBitmap> { | ||
| const cappedLevel = this.maxCityLightLevel; | ||
|
|
||
| // Check cache first | ||
| const cached = this.glowBitmaps.get(cappedLevel); | ||
| if (cached) return cached; | ||
|
|
||
| // Not in cache → generate, store, and return | ||
| const bitmap = await this.createGlowBitmap(cappedLevel, cellSize); | ||
| this.glowBitmaps.set(cappedLevel, bitmap); | ||
|
|
||
| return bitmap; | ||
| } |
Contributor
There was a problem hiding this comment.
The level parameter is ignored—glow bitmaps are only cached for maxCityLightLevel.
The getGlowBitmap() method accepts a level parameter but immediately replaces it with cappedLevel = this.maxCityLightLevel (line 68), meaning all cities get the same sized glow regardless of their actual level. This defeats the purpose of passing cityLevel from line 134.
If you want uniform city lights, remove the unused parameter. If you want level-based scaling, use the actual level (capped to max):
private async getGlowBitmap(
level: number,
cellSize: number,
): Promise<ImageBitmap> {
- const cappedLevel = this.maxCityLightLevel;
+ const cappedLevel = Math.min(level, this.maxCityLightLevel);
// Check cache first
const cached = this.glowBitmaps.get(cappedLevel);This way, level-1 cities get smaller glows than level-15 cities, and the cache stores one bitmap per level (0-15), not just one bitmap for all cities.
🤖 Prompt for AI Agents
In src/client/graphics/layers/NightModeLayer.ts around lines 64–79, the method
currently ignores the incoming level and always uses this.maxCityLightLevel for
caching and bitmap creation; change it to cap the provided level (e.g. const
cappedLevel = Math.min(level, this.maxCityLightLevel)) and use that cappedLevel
as the cache key and when calling createGlowBitmap so each level has its own
bitmap (or remove the level parameter if you deliberately want uniform glows).
evanpelle
reviewed
Nov 11, 2025
Collaborator
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
Thanks, but I believe this feature adds too much complexity for the value it provides.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Description:
This adds a layer which makes the mouse act as a flashlight. It is implemented into user settings.
Updates to do:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
sysrex
Screenshots
Night Mode, No Dark Mode
Night Mode, Dark Mode
In-Game Settings
Main Menu Settings
Summary by CodeRabbit
New Features
Localization
Tests
Chores