Remove role based perms, fetch cosmetics.json from api#1640
Conversation
|
Warning Rate limit exceeded@evanpelle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
WalkthroughThis change removes all role-based logic and static role data from the cosmetics and privilege-checking system. Cosmetics data and access checks now use only flare-based permissions, with cosmetics fetched dynamically from an API. All references to roles, role groups, and static product imports are eliminated across client, server, and schema code. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API
participant Server
participant PrivilegeRefresher
User->>Frontend: Requests cosmetics
Frontend->>API: GET /cosmetics.json
API-->>Frontend: Cosmetics JSON (with flares, no roles)
Frontend->>Frontend: Filter patterns by user flares
User->>Frontend: Attempts to use a pattern or flag
Frontend->>Server: Request with pattern/flag and user flares
Server->>PrivilegeRefresher: Get current PrivilegeChecker
PrivilegeRefresher->>Server: Return checker (flare-based)
Server->>Server: Check permission (flares only)
Server-->>Frontend: Allow or deny
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/core/CustomFlag.ts (1)
36-37: Remove unnecessary mask checkThe
maskvariable is always a non-empty string, so the conditionif (!mask) continue;will never be true.const mask = `/flags/custom/${layerName}.svg`; - if (!mask) continue;src/client/Cosmetics.ts (1)
59-69: Consider caching cosmetics dataThe
getCosmeticsfunction fetches data from the API on every call. Since cosmetics data likely doesn't change frequently, consider caching the result to improve performance.Would you like me to implement a simple caching mechanism with a TTL (time-to-live) for the cosmetics data?
src/core/CosmeticSchemas.ts (1)
16-16: Update outdated commentThe comment references
resources/cosmetics/cosmetics.jsonwhich has been removed. Update it to reflect that cosmetics are now fetched from an API endpoint.-// Schema for resources/cosmetics/cosmetics.json +// Schema for cosmetics data fetched from APIsrc/server/Worker.ts (1)
47-47: Use URL constructor for robust path joiningDirect string concatenation assumes
jwtIssuer()has no trailing slash. Use URL constructor for proper path handling.- const endpoint = config.jwtIssuer() + "/cosmetics.json"; + const endpoint = new URL("/cosmetics.json", config.jwtIssuer()).toString();src/server/Privilege.ts (1)
130-144: Prefix unused parameters with underscoreThe NoOpPrivilegeChecker correctly implements the bypass behavior, but unused parameters should follow TypeScript naming conventions.
export class NoOpPrivilegeChecker implements PrivilegeChecker { isPatternAllowed( - base64: string, - flares: readonly string[] | undefined, + _base64: string, + _flares: readonly string[] | undefined, ): true | "restricted" | "unlisted" | "invalid" { return true; } isCustomFlagAllowed( - flag: string, - flares: readonly string[] | undefined, + _flag: string, + _flares: readonly string[] | undefined, ): true | "restricted" | "invalid" { return true; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
resources/cosmetics/cosmetics.json(0 hunks)resources/cosmetics/roles.txt(0 hunks)src/client/Cosmetics.ts(2 hunks)src/client/TerritoryPatternsModal.ts(12 hunks)src/client/graphics/GameRenderer.ts(1 hunks)src/client/graphics/layers/ChatModal.ts(2 hunks)src/client/graphics/layers/EmojiTable.ts(2 hunks)src/client/graphics/layers/PlayerPanel.ts(2 hunks)src/client/graphics/layers/RadialMenu.ts(2 hunks)src/core/CosmeticSchemas.ts(1 hunks)src/core/CustomFlag.ts(1 hunks)src/server/Privilege.ts(6 hunks)src/server/Worker.ts(5 hunks)tests/server/Privilege.customFlag.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- resources/cosmetics/cosmetics.json
- resources/cosmetics/roles.txt
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: andrewNiziolek
PR: openfrontio/OpenFrontIO#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.
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#786
File: src/server/Client.ts:16-18
Timestamp: 2025-06-12T23:22:55.328Z
Learning: Undefined values for `roles`, `flares`, and `pattern` are intentionally used to mark an unauthenticated client, whereas empty arrays/strings indicate an authenticated user with no roles or flares. Avoid suggesting non-optional types for these fields.
src/client/graphics/GameRenderer.ts (1)
Learnt from: VariableVince
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.
src/client/graphics/layers/ChatModal.ts (1)
Learnt from: VariableVince
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.
src/client/graphics/layers/PlayerPanel.ts (1)
Learnt from: VariableVince
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.
src/client/graphics/layers/EmojiTable.ts (1)
Learnt from: VariableVince
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.
src/client/graphics/layers/RadialMenu.ts (2)
Learnt from: VariableVince
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.
Learnt from: VariableVince
PR: #1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like if (params === undefined || params.selected === null) rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
tests/server/Privilege.customFlag.test.ts (1)
Learnt from: Aotumuri
PR: #709
File: src/client/Main.ts:276-296
Timestamp: 2025-05-16T12:06:01.732Z
Learning: In the OpenFrontIO codebase, checkPermission() function's return values need to be handled with defensive checks using Array.isArray(), even though its TypeScript signature indicates it returns arrays. Removing these checks breaks functionality.
src/core/CustomFlag.ts (1)
Learnt from: Aotumuri
PR: #786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
src/client/Cosmetics.ts (1)
Learnt from: Aotumuri
PR: #786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
src/server/Worker.ts (5)
Learnt from: scottanderson
PR: #1161
File: src/server/jwt.ts:0-0
Timestamp: 2025-06-19T19:31:29.475Z
Learning: In Zod v4, the correct import is import { z } from "zod/v4" and z.prettifyError(error) is a built-in utility method for formatting ZodError into readable, multi-line strings. This is different from Zod v3.x which required different error handling approaches.
Learnt from: Aotumuri
PR: #786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
Learnt from: scottanderson
PR: #784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as z.infer<typeof PlayerStatsSchema> where PlayerStatsSchema has .optional() applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Learnt from: scottanderson
PR: #784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Learnt from: scottanderson
PR: #786
File: src/server/Client.ts:16-18
Timestamp: 2025-06-12T23:22:55.328Z
Learning: Undefined values for roles, flares, and pattern are intentionally used to mark an unauthenticated client, whereas empty arrays/strings indicate an authenticated user with no roles or flares. Avoid suggesting non-optional types for these fields.
src/client/TerritoryPatternsModal.ts (5)
Learnt from: scottanderson
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.
Learnt from: VariableVince
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.
Learnt from: scottanderson
PR: #786
File: src/client/TerritoryPatternsModal.ts:299-299
Timestamp: 2025-06-22T05:33:39.581Z
Learning: In the OpenFrontIO codebase, territory patterns used by PatternDecoder are curated by the development team and stored in a config file in source control, making them trusted data with minimal risk of malformation that would cause PatternDecoder to throw errors.
Learnt from: devalnor
PR: #1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Learnt from: devalnor
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.
src/core/CosmeticSchemas.ts (4)
Learnt from: Aotumuri
PR: #786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
Learnt from: scottanderson
PR: #784
File: src/core/game/StatsImpl.ts:44-53
Timestamp: 2025-05-21T04:10:59.706Z
Learning: In Zod, when a schema has .optional() applied at the object level, the TypeScript type inferred using z.infer already includes undefined in the union type. Therefore, when using such a type as a return type annotation, there's no need to explicitly add | undefined.
Learnt from: scottanderson
PR: #1161
File: src/server/jwt.ts:0-0
Timestamp: 2025-06-19T19:31:29.475Z
Learning: In Zod v4, the correct import is import { z } from "zod/v4" and z.prettifyError(error) is a built-in utility method for formatting ZodError into readable, multi-line strings. This is different from Zod v3.x which required different error handling approaches.
Learnt from: scottanderson
PR: #784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as z.infer<typeof PlayerStatsSchema> where PlayerStatsSchema has .optional() applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
src/server/Privilege.ts (3)
Learnt from: Aotumuri
PR: #709
File: src/client/Main.ts:276-296
Timestamp: 2025-05-16T12:06:01.732Z
Learning: In the OpenFrontIO codebase, checkPermission() function's return values need to be handled with defensive checks using Array.isArray(), even though its TypeScript signature indicates it returns arrays. Removing these checks breaks functionality.
Learnt from: scottanderson
PR: #786
File: src/server/Client.ts:16-18
Timestamp: 2025-06-12T23:22:55.328Z
Learning: Undefined values for roles, flares, and pattern are intentionally used to mark an unauthenticated client, whereas empty arrays/strings indicate an authenticated user with no roles or flares. Avoid suggesting non-optional types for these fields.
Learnt from: Ble4Ch
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.
🧬 Code Graph Analysis (8)
src/client/graphics/layers/ChatModal.ts (1)
src/client/InputHandler.ts (1)
CloseViewEvent(64-64)
src/client/graphics/layers/PlayerPanel.ts (1)
src/client/InputHandler.ts (1)
CloseViewEvent(64-64)
src/client/graphics/layers/EmojiTable.ts (1)
src/client/InputHandler.ts (1)
CloseViewEvent(64-64)
src/client/graphics/layers/RadialMenu.ts (1)
src/client/InputHandler.ts (1)
CloseViewEvent(64-64)
tests/server/Privilege.customFlag.test.ts (2)
src/core/CosmeticSchemas.ts (1)
Cosmetics(39-39)src/server/Privilege.ts (1)
PrivilegeCheckerImpl(15-128)
src/client/Cosmetics.ts (3)
src/core/ApiSchemas.ts (1)
UserMeResponse(49-49)src/core/CosmeticSchemas.ts (3)
Pattern(40-40)Cosmetics(39-39)CosmeticsSchema(17-38)src/client/jwt.ts (1)
getApiBase(18-23)
src/client/TerritoryPatternsModal.ts (3)
src/core/CosmeticSchemas.ts (1)
Pattern(40-40)src/client/Cosmetics.ts (1)
handlePurchase(27-57)src/core/PatternDecoder.ts (1)
PatternDecoder(1-59)
src/core/CosmeticSchemas.ts (1)
src/core/Schemas.ts (2)
PatternSchema(225-225)RequiredPatternSchema(207-224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (18)
src/client/graphics/layers/PlayerPanel.ts (2)
16-16: Clean import additionGood addition of
CloseViewEventto the existing import from InputHandler.
170-172: Consistent event handling patternThe CloseViewEvent listener follows the same clean pattern as the existing MouseUpEvent handler. This creates a unified way to close the player panel.
src/client/graphics/GameRenderer.ts (1)
186-186: Necessary initialization callGood addition of
initEventBus()call for ChatModal. This follows the same pattern as emojiTable and ensures the CloseViewEvent listener is properly registered during renderer setup.src/client/graphics/layers/ChatModal.ts (2)
9-9: Clean import additionProper addition of
CloseViewEventimport for the new event handling functionality.
176-182: Well-implemented event handlerThe
initEventBus()method is cleanly implemented with:
- Proper conditional check using
!this.hidden- Consistent arrow function syntax
- Appropriate method call to
close()This creates a clean integration with the global close event system.
src/client/graphics/layers/EmojiTable.ts (2)
8-8: Clean import updateGood addition of
CloseViewEventto the existing import statement from InputHandler.
52-56: Consistent event handling patternThe CloseViewEvent listener is cleanly integrated into the existing
initEventBus()method. The conditional check with!this.hiddenand call tohideTable()follows the same pattern as other UI components.src/client/graphics/layers/RadialMenu.ts (2)
4-4: Clean import additionProper addition of
CloseViewEventimport for the new event handling functionality.
106-108: Consistent event handling implementationThe CloseViewEvent listener is cleanly added to the existing
init()method. The arrow function syntax and call tohideRadialMenu()follows the same pattern established in other UI components.tests/server/Privilege.customFlag.test.ts (1)
1-96: Test updates correctly reflect the flare-based permission modelThe tests have been properly updated to:
- Use
PrivilegeCheckerImplclass- Pass only flares (no roles)
- Test flare-based access control
- Include proper test coverage for various permission scenarios
src/core/CosmeticSchemas.ts (1)
1-42: Well-structured schema definitionsThe schemas are properly defined using Zod v4 with:
- Clear type definitions for Product, Pattern, and Cosmetics
- Proper use of nullable and optional fields
- Good separation of concerns
src/server/Privilege.ts (2)
4-13: Good interface extraction for testabilityExtracting the PrivilegeChecker interface improves the design by enabling easier testing and alternative implementations.
74-76: Good defensive programming with optional chainingThe null-safe access for
cosmetics.flag?.layersandcosmetics.flag?.colorprevents runtime errors when flag data is absent.src/client/TerritoryPatternsModal.ts (5)
104-116: Simplified tooltip logic for product-based patternsThe tooltip now correctly shows purchase prompt only for patterns with products, aligning with the new product-based access model.
158-171: Well-implemented purchase button integrationThe conditional rendering of purchase buttons for locked patterns is clean and the non-null assertion is safe within the guarded block.
41-41: Good lifecycle management with isActive flagThe isActive flag prevents rendering when modal is closed, and proper cleanup of event listeners and ResizeObserver prevents memory leaks.
Also applies to: 223-223, 237-245
345-359: Excellent performance optimization with preview cachingThe caching mechanism prevents redundant canvas operations for pattern previews. Module-level cache appropriately persists across component instances.
Also applies to: 397-399
327-342: Clean hover state management for purchase tooltipsThe hover handlers correctly show tooltips only for patterns with products, providing good user feedback for purchasable items.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
force-pushed
the
evan-fetch-cosmetics
branch
from
d756506 to
e09c587
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
resources/cosmetics/cosmetics.json(0 hunks)resources/cosmetics/roles.txt(0 hunks)src/client/Cosmetics.ts(2 hunks)src/client/TerritoryPatternsModal.ts(5 hunks)src/core/CosmeticSchemas.ts(1 hunks)src/core/CustomFlag.ts(1 hunks)src/server/Privilege.ts(6 hunks)src/server/PrivilegeRefresher.ts(1 hunks)src/server/Worker.ts(3 hunks)tests/server/Privilege.customFlag.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- resources/cosmetics/roles.txt
- resources/cosmetics/cosmetics.json
✅ Files skipped from review due to trivial changes (2)
- src/core/CustomFlag.ts
- src/server/Worker.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/client/Cosmetics.ts
- tests/server/Privilege.customFlag.test.ts
- src/core/CosmeticSchemas.ts
- src/client/TerritoryPatternsModal.ts
- src/server/Privilege.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: andrewNiziolek
PR: openfrontio/OpenFrontIO#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.
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#786
File: src/server/Client.ts:16-18
Timestamp: 2025-06-12T23:22:55.328Z
Learning: Undefined values for `roles`, `flares`, and `pattern` are intentionally used to mark an unauthenticated client, whereas empty arrays/strings indicate an authenticated user with no roles or flares. Avoid suggesting non-optional types for these fields.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (3)
src/server/PrivilegeRefresher.ts (3)
1-8: LGTM! Clear and descriptive imports.The alias
FailOpenPrivilegeCheckermakes the security behavior more explicit than the originalNoOpPrivilegeCheckername.
17-20: LGTM! Clean dependency injection.Good use of TypeScript's automatic property assignment in constructor parameters.
32-37: LGTM! Proper fallback logic.The fail-open behavior is correctly implemented. The null check will work properly once the field initialization is fixed.
evanpelle
force-pushed
the
evan-fetch-cosmetics
branch
from
e09c587 to
62ffc85
Compare
evanpelle
force-pushed
the
evan-fetch-cosmetics
branch
from
62ffc85 to
c2cb1e7
Compare
evanpelle
force-pushed
the
evan-fetch-cosmetics
branch
from
c2cb1e7 to
22c611e
Compare
evanpelle
force-pushed
the
evan-fetch-cosmetics
branch
from
22c611e to
1bf5079
Compare
evanpelle
force-pushed
the
evan-fetch-cosmetics
branch
from
1bf5079 to
52b8cee
Compare
evanpelle
force-pushed
the
evan-fetch-cosmetics
branch
2 times, most recently
from
3387344 to
ead47dd
Compare
|
|
scottanderson
left a comment
scottanderson
left a comment
There was a problem hiding this comment.
I only had time to review about half of this one right now. I'll take another look at this later.
scottanderson
left a comment
scottanderson
left a comment
There was a problem hiding this comment.
Can we make the necessary changes to cosmetics.json as a static file, before moving it out to the API? This PR does more than just fetch the file.
evanpelle
force-pushed
the
evan-fetch-cosmetics
branch
from
a3ec9ad to
1769338
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/client/Cosmetics.ts (2)
12-24: Address data mutation and iteration performance issuesThe code has two issues that were flagged in previous reviews:
- Data mutation: Line 16 directly mutates
patternData.product, which affects the shared cosmetics data for other callers- Unnecessary garbage generation: Using
Object.entries()creates temporary arrays when direct iteration would be more efficientApply this diff to fix both issues:
- for (const [name, patternData] of Object.entries(cosmetics.patterns)) { + for (const name in cosmetics.patterns) { + const patternData = cosmetics.patterns[name]; const hasAccess = playerFlares.has(`pattern:${name}`); if (hasAccess) { // Remove product info because player already has access. - patternData.product = null; - patterns.push(patternData); + patterns.push({ ...patternData, product: null }); } else if (patternData.product !== null) { // Player doesn't have access, but product is available for purchase. patterns.push(patternData);
63-80: Error handling approach needs adjustmentThe function currently swallows all errors and returns a fallback object. As noted in previous reviews, this prevents callers from knowing when something went wrong.
Apply this diff to let callers handle errors appropriately:
-async function getCosmetics(): Promise<Cosmetics> { +async function getCosmetics(): Promise<Cosmetics | undefined> { try { const response = await fetch(`${getApiBase()}/cosmetics.json`); if (!response.ok) { - throw new Error(`HTTP error! status: ${response.status}`); + console.error(`HTTP error! status: ${response.status}`); + return; } const result = CosmeticsSchema.safeParse(await response.json()); if (!result.success) { - throw new Error(`Invalid cosmetics: ${result.error.message}`); + console.error(`Invalid cosmetics: ${result.error.message}`); + return; } return result.data; } catch (error) { console.error("Error getting cosmetics:", error); - return { - patterns: {}, - }; + return; } }You'll also need to update the
patternsfunction to handle the undefined case:export async function patterns( userMe: UserMeResponse | null, ): Promise<Pattern[]> { const cosmetics = await getCosmetics(); + if (!cosmetics) { + return []; + } const patterns: Pattern[] = [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitignore(1 hunks)resources/cosmetics/roles.txt(0 hunks)src/client/Cosmetics.ts(1 hunks)src/client/TerritoryPatternsModal.ts(6 hunks)src/client/graphics/layers/ChatModal.ts(1 hunks)src/client/graphics/layers/EmojiTable.ts(2 hunks)src/client/graphics/layers/PlayerPanel.ts(1 hunks)src/core/CosmeticSchemas.ts(1 hunks)src/core/CustomFlag.ts(4 hunks)src/server/Privilege.ts(6 hunks)src/server/PrivilegeRefresher.ts(1 hunks)src/server/Worker.ts(5 hunks)tests/server/Privilege.customFlag.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- resources/cosmetics/roles.txt
🚧 Files skipped from review as they are similar to previous changes (8)
- .gitignore
- tests/server/Privilege.customFlag.test.ts
- src/server/PrivilegeRefresher.ts
- src/core/CustomFlag.ts
- src/server/Worker.ts
- src/core/CosmeticSchemas.ts
- src/client/TerritoryPatternsModal.ts
- src/server/Privilege.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: andrewNiziolek
PR: openfrontio/OpenFrontIO#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.
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#786
File: src/server/Client.ts:16-18
Timestamp: 2025-06-12T23:22:55.328Z
Learning: Undefined values for `roles`, `flares`, and `pattern` are intentionally used to mark an unauthenticated client, whereas empty arrays/strings indicate an authenticated user with no roles or flares. Avoid suggesting non-optional types for these fields.
📚 Learning: in src/client/main.ts, during game start in the handlejoinlobby callback, ui elements are hidden usi...
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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/EmojiTable.tssrc/client/graphics/layers/PlayerPanel.tssrc/client/graphics/layers/ChatModal.ts
📚 Learning: in the openfrontio codebase, json files should be imported using standard import syntax without impo...
Learnt from: Aotumuri
PR: openfrontio/OpenFrontIO#786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
Applied to files:
src/client/Cosmetics.ts
🪛 Biome (2.1.2)
src/client/graphics/layers/EmojiTable.ts
[error] 56-56: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 56-58: Expected a statement but instead found '=======
ead47dd (remote cosmetics)'.
Expected a statement here.
(parse)
src/client/graphics/layers/ChatModal.ts
[error] 188-190: Expected a statement but instead found '=======
initEventBus()'.
Expected a statement here.
(parse)
[error] 191-192: Expected a statement but instead found '>>>>>>> 07e275a (Allow additional modals to close when clicking the Escape key (#1604))'.
Expected a statement here.
(parse)
[error] 192-192: numbers cannot be followed by identifiers directly after
an identifier cannot appear here
(parse)
🪛 GitHub Check: 🔍 ESLint
src/client/graphics/layers/EmojiTable.ts
[failure] 51-51:
Parsing error: Merge conflict marker encountered.
src/client/graphics/layers/ChatModal.ts
[failure] 185-185:
Parsing error: Merge conflict marker encountered.
🪛 GitHub Actions: 🧪 CI
src/client/graphics/layers/EmojiTable.ts
[error] 51-51: TS1185: Merge conflict marker encountered.
[error] 57-57: TS1185: Merge conflict marker encountered.
[error] 58-58: TS1185: Merge conflict marker encountered.
[error] 52-52: TS2552: Cannot find name 'CloseViewEvent'. Did you mean 'CloseEvent'?
src/client/graphics/layers/ChatModal.ts
[error] 185-185: TS1185: Merge conflict marker encountered.
[error] 189-189: TS1185: Merge conflict marker encountered.
[error] 192-192: TS1185: Merge conflict marker encountered.
[error] 176-176: TS2393: Duplicate function implementation.
[error] 186-186: TS2393: Duplicate function implementation.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (2)
src/client/Cosmetics.ts (2)
8-10: Clean refactor to dynamic cosmetics fetchingThe switch from static imports to dynamic API fetching aligns well with the PR objectives. The flare-based access control is simpler and more maintainable than the previous role system.
27-61: Purchase handling looks clean and robustThe error handling with specific 401 response handling and user-friendly alerts is well implemented. The function structure is clear and follows good TypeScript practices.
| <<<<<<< HEAD | ||
| eventBus.on(CloseViewEvent, (e) => { | ||
| if (!this.hidden) { | ||
| this.hideTable(); | ||
| } | ||
| }); | ||
| ======= | ||
| >>>>>>> ead47dd1 (remote cosmetics) |
There was a problem hiding this comment.
Fix merge conflict markers.
The file has unresolved merge conflict markers that prevent compilation. Based on the changes removing CloseViewEvent handling, the conflicted code should be removed entirely.
Apply this diff to resolve the merge conflict:
-<<<<<<< HEAD
-
- eventBus.on(CloseViewEvent, (e) => {
- if (!this.hidden) {
- this.hideTable();
- }
- });
-=======
->>>>>>> ead47dd1 (remote cosmetics)📝 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.
| <<<<<<< HEAD | |
| eventBus.on(CloseViewEvent, (e) => { | |
| if (!this.hidden) { | |
| this.hideTable(); | |
| } | |
| }); | |
| ======= | |
| >>>>>>> ead47dd1 (remote cosmetics) |
🧰 Tools
🪛 Biome (2.1.2)
[error] 56-56: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 56-58: Expected a statement but instead found '=======
ead47dd (remote cosmetics)'.
Expected a statement here.
(parse)
🪛 GitHub Check: 🔍 ESLint
[failure] 51-51:
Parsing error: Merge conflict marker encountered.
🪛 GitHub Actions: 🧪 CI
[error] 51-51: TS1185: Merge conflict marker encountered.
[error] 57-57: TS1185: Merge conflict marker encountered.
[error] 58-58: TS1185: Merge conflict marker encountered.
[error] 52-52: TS2552: Cannot find name 'CloseViewEvent'. Did you mean 'CloseEvent'?
🤖 Prompt for AI Agents
In src/client/graphics/layers/EmojiTable.ts between lines 51 and 58, there are
unresolved merge conflict markers around the CloseViewEvent event handler.
Remove the entire block containing the eventBus.on(CloseViewEvent, ...) listener
along with the conflict markers to resolve the conflict and ensure the file
compiles correctly.
| this.eventBus.on(CloseViewEvent, (e) => { | ||
| this.hide(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate event listener.
There's already a CloseViewEvent listener at lines 178-180 that does the same thing. This duplicate listener is unnecessary and could cause hide() to be called multiple times for the same event.
Apply this diff to remove the duplicate listener:
- this.eventBus.on(CloseViewEvent, (e) => {
- this.hide();
- });📝 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.
| this.eventBus.on(CloseViewEvent, (e) => { | |
| this.hide(); | |
| }); | |
| // (Lines 178–180: original CloseViewEvent listener) | |
| this.eventBus.on(CloseViewEvent, (e) => { | |
| this.hide(); | |
| }); | |
| // Duplicate listener at lines 182–184 has been removed |
🤖 Prompt for AI Agents
In src/client/graphics/layers/PlayerPanel.ts around lines 182 to 184, there is a
duplicate event listener for CloseViewEvent that calls this.hide(), which is
already handled by a listener at lines 178 to 180. Remove the listener at lines
182 to 184 to avoid calling hide() multiple times for the same event.
1769338 to
7d1a847
Compare
evanpelle
force-pushed
the
evan-fetch-cosmetics
branch
from
00ad42c to
69d20dc
Compare
evanpelle
force-pushed
the
evan-fetch-cosmetics
branch
2 times, most recently
from
317da47 to
0caabd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/server/Privilege.customFlag.test.ts (3)
5-8: Replace throw-away stub with an explicit Jest mockUsing an inline function that always throws hides why the decoder is called if it ever is.
A Jest mock makes intent clearer and gives you an assertion point:const dummyPatternDecoder = jest.fn(() => { throw new Error("Pattern decoding should not be reached in flag tests"); });
30-76: Collapse repetitive cases withit.eachMany assertions differ only by input tuple. Parameterising them cuts noise and documents the matrix succinctly:
it.each([ ["!b-b", [], true], ["!a-b", ["cosmetic:flags"], true], // … ])("flag %s with flares %j → %s", (flag, flares, expected) => { expect(checker.isCustomFlagAllowed(flag, flares)).toBe(expected); });
9-26: Optional: give every mock layer/color an explicitflaresarrayLeaving
flaresundefined relies on implicit “unrestricted” semantics.
Being explicit (flares: []) makes the intent unambiguous and guards against future schema changes that might treatundefineddifferently from an empty list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignore(1 hunks)resources/cosmetics/roles.txt(0 hunks)src/client/Cosmetics.ts(1 hunks)src/client/TerritoryPatternsModal.ts(6 hunks)src/core/CosmeticSchemas.ts(1 hunks)src/core/CustomFlag.ts(4 hunks)src/server/Privilege.ts(6 hunks)src/server/PrivilegeRefresher.ts(1 hunks)src/server/Worker.ts(5 hunks)tests/server/Privilege.customFlag.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- resources/cosmetics/roles.txt
🚧 Files skipped from review as they are similar to previous changes (8)
- .gitignore
- src/core/CustomFlag.ts
- src/core/CosmeticSchemas.ts
- src/server/PrivilegeRefresher.ts
- src/server/Worker.ts
- src/server/Privilege.ts
- src/client/Cosmetics.ts
- src/client/TerritoryPatternsModal.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: andrewNiziolek
PR: openfrontio/OpenFrontIO#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.
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#786
File: src/server/Client.ts:16-18
Timestamp: 2025-06-12T23:22:55.328Z
Learning: Undefined values for `roles`, `flares`, and `pattern` are intentionally used to mark an unauthenticated client, whereas empty arrays/strings indicate an authenticated user with no roles or flares. Avoid suggesting non-optional types for these fields.
📚 Learning: in the openfrontio codebase, `checkpermission()` function's return values need to be handled with de...
Learnt from: Aotumuri
PR: openfrontio/OpenFrontIO#709
File: src/client/Main.ts:276-296
Timestamp: 2025-05-16T12:06:01.732Z
Learning: In the OpenFrontIO codebase, `checkPermission()` function's return values need to be handled with defensive checks using `Array.isArray()`, even though its TypeScript signature indicates it returns arrays. Removing these checks breaks functionality.
Applied to files:
tests/server/Privilege.customFlag.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (1)
tests/server/Privilege.customFlag.test.ts (1)
2-2: Confirm correct export nameDouble-check that
PrivilegeCheckerImplis exported as a named export fromsrc/server/Privilege.ts.
If the implementation switched to a default export during the refactor, this import will break the tests.
| }; | ||
|
|
||
| const checker = new PrivilegeChecker(mockCosmetics, dummyPatternDecoder); | ||
| const checker = new PrivilegeCheckerImpl(mockCosmetics, dummyPatternDecoder); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Instantiate checker per test to avoid shared state
PrivilegeCheckerImpl might cache results internally.
Creating a single instance for the entire suite risks cross-test leakage.
Wrap construction in a beforeEach:
let checker: PrivilegeCheckerImpl;
beforeEach(() => {
checker = new PrivilegeCheckerImpl(mockCosmetics, dummyPatternDecoder);
});🤖 Prompt for AI Agents
In tests/server/Privilege.customFlag.test.ts at line 28, the
PrivilegeCheckerImpl instance is created once and shared across tests, risking
state leakage. To fix this, remove the direct instantiation at line 28 and
instead declare a checker variable outside tests, then instantiate it inside a
beforeEach block so each test gets a fresh instance.
Ack, it's a lot of work to split up. |
evanpelle
force-pushed
the
evan-fetch-cosmetics
branch
from
0caabd2 to
423b602
Compare
evanpelle
force-pushed
the
evan-fetch-cosmetics
branch
from
423b602 to
bf0ce15
Compare
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
) ## Description: * Fetch cosmetics.json from api * Remove all role based perms, we are only using flares now * Created Priviledge refresher which periodically polls /cosmetics.json endpoint. ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced - [x] I have read and accepted the CLA agreement (only required once). ## Please put your Discord username so you can be contacted if a bug or regression is found: evan
3 participants
Description:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan