Pull replay files from api #1725
Conversation
WalkthroughA new method Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Archive
participant Config
participant PublicAPI
Client->>Archive: readGameRecord(gameId)
Archive->>Archive: Try primary R2 storage read
alt Success
Archive-->>Client: Return RedactedGameRecord
else Failure or empty
Archive->>Config: replayUrl(gameId)
Archive->>PublicAPI: Fetch game record from fallback URL
alt Success
PublicAPI-->>Archive: JSON RedactedGameRecord
Archive-->>Client: Return RedactedGameRecord
else Failure
Archive-->>Client: Return error string
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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}
scottanderson
added
Feature - Frontend
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/server/Archive.ts (2)
184-184: Add schema validation for response data.The response JSON is cast to
GameRecordwithout validation, which could cause runtime errors if the API returns unexpected data structure.
184-184: Critical: Missing schema validation for API responseThe response JSON is cast to GameRecord without validation, creating unsafe type assumptions. Based on past comments, GameRecordSchema validation fails with API responses due to missing fields like
persistentIDand differentintent.typestructures.You need to either:
- Fix the API to return data matching GameRecordSchema, or
- Create a separate schema for API responses, or
- Add proper error handling for schema mismatches
- return await response.json(); + const json = await response.json(); + try { + return GameRecordSchema.parse(json); + } catch (parseError) { + log.info(`${gameId}: API response doesn't match expected schema: ${parseError}`); + return null; + }
🧹 Nitpick comments (1)
src/server/Archive.ts (1)
161-202: Consider adding timeout and retry logicThe fallback function lacks timeout protection and retry logic for network requests. Consider adding:
- Request timeout to prevent hanging
- Simple retry logic for transient failures
- More specific error handling for different HTTP status codes
+const FALLBACK_TIMEOUT = 10000; // 10 seconds + export async function readGameRecordFallback( gameId: GameID, ): Promise<GameRecord | null> { try { - const response = await fetch(config.replayFallbackUrl(gameId), { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), FALLBACK_TIMEOUT); + + const response = await fetch(config.replayFallbackUrl(gameId), { headers: { Accept: "application/json", }, + signal: controller.signal, }); + + clearTimeout(timeoutId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/configuration/DefaultConfig.ts(1 hunks)src/core/configuration/DevConfig.ts(2 hunks)src/server/Archive.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/configuration/DefaultConfig.ts
🧰 Additional context used
🧠 Learnings (8)
📓 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.
📚 Learning: in the codebase, playerstats is defined as `z.infer` where playerstatssche...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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).
Applied to files:
src/core/configuration/DevConfig.tssrc/server/Archive.ts
📚 Learning: in the codebase, playerstats is defined as a type inferred from a zod schema that is marked as optio...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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).
Applied to files:
src/core/configuration/DevConfig.tssrc/server/Archive.ts
📚 Learning: in the openfrontio codebase, usersettings instances are created directly with `new usersettings()` i...
Learnt from: devalnor
PR: openfrontio/OpenFrontIO#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.
Applied to files:
src/core/configuration/DevConfig.ts
📚 Learning: the playerstats type from archiveschemas already includes undefined in its definition, making explic...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:44-53
Timestamp: 2025-05-21T04:10:59.706Z
Learning: The PlayerStats type from ArchiveSchemas already includes undefined in its definition, making explicit union types with undefined (PlayerStats | undefined) redundant.
Applied to files:
src/core/configuration/DevConfig.tssrc/server/Archive.ts
📚 Learning: when working with zod schemas in the openfrontio project, values can be marked as possibly undefined...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:189-209
Timestamp: 2025-05-18T23:43:38.150Z
Learning: When working with zod schemas in the OpenFrontIO project, values can be marked as possibly undefined by zod even if logic suggests they should always be defined. It's better to preserve the `| undefined` union types in method signatures to align with zod's type inference, even if runtime checks throw errors for invalid cases.
Applied to files:
src/server/Archive.ts
📚 Learning: in zod, when a schema has `.optional()` applied at the object level, the typescript type inferred us...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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`.
Applied to files:
src/server/Archive.ts
📚 Learning: in zod v4, the correct import is `import { z } from "zod/v4"` and z.prettifyerror(error) is a built-...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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.
Applied to files:
src/server/Archive.ts
🔇 Additional comments (9)
src/core/configuration/DevConfig.ts (4)
3-3: LGTM!The
GameIDimport is needed for the new method parameter type.
46-48: LGTM!Clean implementation using template literal. The hardcoded URL approach is appropriate for dev config.
3-3: LGTM: Import addition is correctThe GameID import is properly added for the new method parameter type.
46-48: LGTM: Clean implementation of fallback URL methodThe method correctly implements the interface with a hardcoded URL appropriate for development configuration. The template string interpolation is clean and typed.
src/server/Archive.ts (5)
134-137: LGTM!Good fallback handling when R2 returns empty response body. The warning log helps with debugging.
144-158: LGTM!Smart change to use info level logging since R2 failures are no longer terminal with the fallback mechanism in place.
185-201: LGTM!Consistent error handling pattern with appropriate info level logging for the fallback mechanism.
134-137: Good handling of undefined response bodyThe explicit check for undefined response body with appropriate logging and fallback call improves robustness.
144-158: LGTM: Error handling improvementsThe change to log at
infolevel instead oferroraddresses previous feedback and makes sense for fallback scenarios. The consistent error handling structure is maintained.
|
Thanks for the quick PR for this. Please note the supported keywords at https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue I have manually linked the referenced ticket, so no need to do it on this PR. |
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/server/Archive.ts (1)
218-218: Add schema validation for API response.The function performs unsafe casting of the JSON response to
GameRecord. Based on past review discussions, this should be validated with a proper schema to prevent runtime errors.Consider implementing the schema validation approach discussed in previous reviews, or use a more lenient schema specifically for API responses if the full
GameRecordschema is too strict.
🧹 Nitpick comments (2)
src/server/Archive.ts (2)
36-36: Fix typo in function name.The function name has a typo:
stripPerisistentIdsshould bestripPersistentIds.Apply this diff to fix the typo:
- await archiveFullGameToR2(stripPerisistentIds(gameRecord)); + await archiveFullGameToR2(stripPersistentIds(gameRecord));
178-192: Consider the impact of reducing error logging level.The error logging was changed from
errortoinfolevel. While this reduces noise in logs, it might make debugging R2 connectivity issues harder to spot. Consider ifwarnlevel would be more appropriate for actual failures.The fallback mechanism implementation looks solid and provides good resilience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/Schemas.ts(1 hunks)src/server/Archive.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/core/Schemas.ts
🧰 Additional context used
🧠 Learnings (9)
📓 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.
📚 Learning: in the codebase, playerstats is defined as `z.infer` where playerstatssche...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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).
Applied to files:
src/server/Archive.ts
📚 Learning: in the codebase, playerstats is defined as a type inferred from a zod schema that is marked as optio...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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).
Applied to files:
src/server/Archive.ts
📚 Learning: when working with zod schemas in the openfrontio project, values can be marked as possibly undefined...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:189-209
Timestamp: 2025-05-18T23:43:38.150Z
Learning: When working with zod schemas in the OpenFrontIO project, values can be marked as possibly undefined by zod even if logic suggests they should always be defined. It's better to preserve the `| undefined` union types in method signatures to align with zod's type inference, even if runtime checks throw errors for invalid cases.
Applied to files:
src/server/Archive.ts
📚 Learning: in zod, when a schema has `.optional()` applied at the object level, the typescript type inferred us...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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`.
Applied to files:
src/server/Archive.ts
📚 Learning: the playerstats type from archiveschemas already includes undefined in its definition, making explic...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:44-53
Timestamp: 2025-05-21T04:10:59.706Z
Learning: The PlayerStats type from ArchiveSchemas already includes undefined in its definition, making explicit union types with undefined (PlayerStats | undefined) redundant.
Applied to files:
src/server/Archive.ts
📚 Learning: in zod v4, the correct import is `import { z } from "zod/v4"` and z.prettifyerror(error) is a built-...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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.
Applied to files:
src/server/Archive.ts
📚 Learning: in the openfrontio project, the correct path for icons from src/client/index.html is ../../resources...
Learnt from: tnhnblgl
PR: openfrontio/OpenFrontIO#875
File: src/client/index.html:396-402
Timestamp: 2025-05-26T09:52:52.465Z
Learning: In the OpenFrontIO project, the correct path for icons from src/client/index.html is ../../resources/icons/, not /static/icons/ as the build process handles the path mapping differently.
Applied to files:
src/server/Archive.ts
📚 Learning: in the openfrontio codebase, for unit tests it's acceptable to let exceptions (like json parsing err...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#1441
File: tests/LangCode.test.ts:19-32
Timestamp: 2025-07-15T17:20:17.831Z
Learning: In the OpenFrontIO codebase, for unit tests it's acceptable to let exceptions (like JSON parsing errors) propagate and fail the test naturally rather than adding explicit error handling with try-catch blocks, as the test framework will catch them and fail the test appropriately.
Applied to files:
src/server/Archive.ts
🧬 Code Graph Analysis (1)
src/server/Archive.ts (2)
src/core/Schemas.ts (3)
GameRecord(560-560)AnalyticsRecord(555-555)GameID(21-21)src/core/Util.ts (1)
replacer(287-289)
🪛 GitHub Actions: 🧪 CI
src/server/Archive.ts
[error] 98-98: TypeScript error TS2322: Type '{ clientID: string; username: string; pattern: string | undefined; flag: string | undefined; }[]' is not assignable to type with required property 'persistentID'. Property 'persistentID' is missing but required.
🔇 Additional comments (3)
src/server/Archive.ts (3)
57-68: LGTM!Clean implementation that extracts analytics data by excluding the
turnsfield. The destructuring approach is readable and type-safe.
108-109: LGTM!Good refactoring - the function now has a clear single responsibility by accepting pre-processed
AnalyticsRecordinstead of extracting analytics data internally.
146-147: LGTM!Good separation of concerns - the archiving function now focuses solely on storage while data sanitization is handled by the caller.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/Archive.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: the playerstats type from archiveschemas already includes undefined in its definition, making explic...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:44-53
Timestamp: 2025-05-21T04:10:59.706Z
Learning: The PlayerStats type from ArchiveSchemas already includes undefined in its definition, making explicit union types with undefined (PlayerStats | undefined) redundant.
Applied to files:
src/server/Archive.ts
📚 Learning: in the codebase, playerstats is defined as `z.infer` where playerstatssche...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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).
Applied to files:
src/server/Archive.ts
📚 Learning: in the codebase, playerstats is defined as a type inferred from a zod schema that is marked as optio...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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).
Applied to files:
src/server/Archive.ts
📚 Learning: when working with zod schemas in the openfrontio project, values can be marked as possibly undefined...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:189-209
Timestamp: 2025-05-18T23:43:38.150Z
Learning: When working with zod schemas in the OpenFrontIO project, values can be marked as possibly undefined by zod even if logic suggests they should always be defined. It's better to preserve the `| undefined` union types in method signatures to align with zod's type inference, even if runtime checks throw errors for invalid cases.
Applied to files:
src/server/Archive.ts
📚 Learning: in zod, when a schema has `.optional()` applied at the object level, the typescript type inferred us...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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`.
Applied to files:
src/server/Archive.ts
📚 Learning: in zod v4, the correct import is `import { z } from "zod/v4"` and z.prettifyerror(error) is a built-...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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.
Applied to files:
src/server/Archive.ts
📚 Learning: in the openfrontio project, the correct path for icons from src/client/index.html is ../../resources...
Learnt from: tnhnblgl
PR: openfrontio/OpenFrontIO#875
File: src/client/index.html:396-402
Timestamp: 2025-05-26T09:52:52.465Z
Learning: In the OpenFrontIO project, the correct path for icons from src/client/index.html is ../../resources/icons/, not /static/icons/ as the build process handles the path mapping differently.
Applied to files:
src/server/Archive.ts
📚 Learning: in the openfrontio codebase, for unit tests it's acceptable to let exceptions (like json parsing err...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#1441
File: tests/LangCode.test.ts:19-32
Timestamp: 2025-07-15T17:20:17.831Z
Learning: In the OpenFrontIO codebase, for unit tests it's acceptable to let exceptions (like JSON parsing errors) propagate and fail the test naturally rather than adding explicit error handling with try-catch blocks, as the test framework will catch them and fail the test appropriately.
Applied to files:
src/server/Archive.ts
📚 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:
src/server/Archive.ts
🧬 Code Graph Analysis (1)
src/server/Archive.ts (2)
src/core/Schemas.ts (5)
GameRecord(560-560)AnalyticsRecord(555-555)RedactedGameRecord(573-573)GameID(21-21)RedactedGameRecordSchema(562-572)src/core/Util.ts (1)
replacer(287-289)
🔇 Additional comments (8)
src/server/Archive.ts (8)
36-36: LGTM! Proper data redaction before archiving.The preprocessing approach correctly separates analytics data from full game data and removes sensitive information before storage.
Also applies to: 43-43
64-75: LGTM! Clean implementation with proper type safety.The function clearly extracts analytics data while excluding turns, using readable destructuring syntax.
77-113: LGTM! Addresses past review feedback effectively.The function correctly:
- Fixes the typo in function name
- Uses proper RedactedGameRecord return type
- Cleanly removes persistentID fields using functional mapping
- Uses underscore convention for unused destructured variables
115-116: LGTM! Proper type alignment with preprocessing.The function signature correctly accepts AnalyticsRecord, making the archiving process more type-safe and purposeful.
Also applies to: 124-124
149-149: LGTM! Enhanced data privacy with RedactedGameRecord.The function signature change ensures sensitive data cannot be accidentally archived to full game storage.
Also applies to: 153-154
167-167: LGTM! Excellent resilience improvements.The changes provide:
- Consistent RedactedGameRecord return type
- Proper fallback when R2 response is empty
- Appropriate info-level logging for expected failures
- Centralized validation through validateRecord helper
The fallback mechanism ensures users can replay games even when primary storage fails.
Also applies to: 175-200
202-243: LGTM! Well-implemented HTTP fallback mechanism.The function demonstrates good practices:
- Proper HTTP headers and status checking
- Content type validation before parsing
- Appropriate info-level error logging for fallback scenarios
- Consistent schema validation using validateRecord helper
245-258: LGTM! Clean validation helper with proper error handling.The function correctly:
- Centralizes schema validation logic
- Uses safeParse for graceful error handling
- Formats errors with z.prettifyError (consistent with project patterns)
- Provides clear return type semantics
Can we improve the error that is displayed to the user? In the scenario where parsing fails, I think we should say something to the effect of: |
Co-authored-by: Scott Anderson <[email protected]>
|
Sure, we can clean that up. Right now Also, what’s about localization? Should the error be a translation key or a raw string? I haven’t messed with the localization system yet, so I’m not sure how it’s wired. |
I normally prefer errors as values, because caught errors are always
Logged errors can be in english. Anything shown to the user should be put into the |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
src/client/JoinPrivateLobbyModal.ts (2)
244-246: Localize the version mismatch message (add a translation key)Hardcoded English text breaks i18n. Use translateText and add a key (e.g., record.version_mismatch) to en.json.
- this.message = - "This game was created with a different version. Cannot join."; + this.message = translateText("record.version_mismatch");
4-4: Update JoinLobbyEvent and imports for RedactedGameRecordTo keep types in sync when the server sends a redacted record, switch to using
RedactedGameRecordin the modal and widen the event’sgameRecordfield:• In
src/client/JoinPrivateLobbyModal.ts(and other spots casting the event), update:-import { GameInfo, GameRecord } from "../core/Schemas"; +import { GameInfo, RedactedGameRecord } from "../core/Schemas"; … -const gameRecord = archiveData.gameRecord as GameRecord; +const gameRecord = archiveData.gameRecord as RedactedGameRecord;• In
src/client/Main.ts, addRedactedGameRecordto the import and change theJoinLobbyEventinterface:-import { GameStartInfo, GameRecord } from "../core/Schemas"; +import { GameStartInfo, GameRecord, RedactedGameRecord } from "../core/Schemas"; export interface JoinLobbyEvent { clientID: string; gameID: string; gameStartInfo?: GameStartInfo; - gameRecord?: GameRecord; + gameRecord?: GameRecord | RedactedGameRecord; }This ensures any lobby-join event can carry either full or redacted records without TypeScript errors.
src/server/Archive.ts (1)
149-163: Improve error logging consistency for full-game archivingMatch the detailed logging used elsewhere (message, stack, name). This helps debugging incidents.
- } catch (error) { - log.error(`error saving game ${gameRecord.info.gameID}`); - throw error; + } catch (error: unknown) { + if (!(error instanceof Error)) { + log.error( + `${gameRecord.info.gameID}: Error writing full game to R2. Non-Error type: ${String(error)}` + ); + throw error; + } + const { message, stack, name } = error; + log.error(`${gameRecord.info.gameID}: Error writing full game to R2: ${error}`, { + message, + stack, + name, + ...(error && typeof error === "object" ? error : {}), + }); + throw error; }
🧹 Nitpick comments (2)
src/client/JoinPrivateLobbyModal.ts (1)
236-247: Prefer typed discriminated unions over matching error stringsString matching is brittle. Consider returning a discriminated union from the server like:
- { exists: true; gameRecord: RedactedGameRecord }
- { exists: false; error: "not_found" | "read_failed" | "invalid_data" | "version_mismatch"; details?: string }
Client can branch on error codes, and translation keys can map 1:1 to codes.
I can draft the shared Response schema and zod validators on both sides.
Also applies to: 264-273
src/server/Archive.ts (1)
33-44: Optional: avoid mutating input gameRecordSetting gameRecord.gitCommit mutates the caller’s object. Consider building derived objects with the gitCommit injected instead, to keep functions pure.
export async function archive(gameRecord: GameRecord) { try { - gameRecord.gitCommit = config.gitCommit(); + const gitCommit = config.gitCommit(); // Archive to R2 - await archiveAnalyticsToR2(stripTurns(gameRecord)); + await archiveAnalyticsToR2(stripTurns({ ...gameRecord, gitCommit })); // Archive full game if there are turns if (gameRecord.turns.length > 0) { log.info( `${gameRecord.info.gameID}: game has more than zero turns, attempting to write to full game to R2`, ); - await archiveFullGameToR2(stripPersistentIds(gameRecord)); + await archiveFullGameToR2(stripPersistentIds({ ...gameRecord, gitCommit })); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.json(1 hunks)src/client/JoinPrivateLobbyModal.ts(1 hunks)src/server/Archive.ts(6 hunks)src/server/Worker.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/Worker.ts
🧰 Additional context used
🧠 Learnings (12)
📓 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.
📚 Learning: 2025-06-09T02:20:43.637Z
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/JoinPrivateLobbyModal.ts
📚 Learning: 2025-05-21T04:10:59.706Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:44-53
Timestamp: 2025-05-21T04:10:59.706Z
Learning: The PlayerStats type from ArchiveSchemas already includes undefined in its definition, making explicit union types with undefined (PlayerStats | undefined) redundant.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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).
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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).
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-18T23:43:38.150Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:189-209
Timestamp: 2025-05-18T23:43:38.150Z
Learning: When working with zod schemas in the OpenFrontIO project, values can be marked as possibly undefined by zod even if logic suggests they should always be defined. It's better to preserve the `| undefined` union types in method signatures to align with zod's type inference, even if runtime checks throw errors for invalid cases.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-21T04:10:59.706Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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`.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-06-19T19:31:29.475Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-26T09:52:52.465Z
Learnt from: tnhnblgl
PR: openfrontio/OpenFrontIO#875
File: src/client/index.html:396-402
Timestamp: 2025-05-26T09:52:52.465Z
Learning: In the OpenFrontIO project, the correct path for icons from src/client/index.html is ../../resources/icons/, not /static/icons/ as the build process handles the path mapping differently.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-07-15T17:20:17.831Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#1441
File: tests/LangCode.test.ts:19-32
Timestamp: 2025-07-15T17:20:17.831Z
Learning: In the OpenFrontIO codebase, for unit tests it's acceptable to let exceptions (like JSON parsing errors) propagate and fail the test naturally rather than adding explicit error handling with try-catch blocks, as the test framework will catch them and fail the test appropriately.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-16T12:06:01.732Z
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:
src/server/Archive.ts
📚 Learning: 2025-06-07T13:15:55.439Z
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/server/Archive.ts
🧬 Code Graph Analysis (2)
src/client/JoinPrivateLobbyModal.ts (1)
src/client/Utils.ts (1)
translateText(82-137)
src/server/Archive.ts (2)
src/core/Schemas.ts (5)
GameRecord(560-560)AnalyticsRecord(555-555)RedactedGameRecord(573-573)GameID(21-21)RedactedGameRecordSchema(562-572)src/core/Util.ts (1)
replacer(287-289)
🔇 Additional comments (5)
src/client/JoinPrivateLobbyModal.ts (1)
264-273: Good: map backend error strings to localized messagesThe new branches translate API error states to user-friendly messages. This matches the server’s error-as-value approach and uses translateText correctly.
src/server/Archive.ts (4)
2-2: Import z from zod/v4 for prettifyErrorz.prettifyError is a v4 utility. Importing from "zod/v4" aligns with project learnings and avoids runtime surprises.
-import { z } from "zod"; +import { z } from "zod/v4";If the repo already aliases v4 under "zod", keep as-is; otherwise prefer explicit "zod/v4" for clarity.
Likely an incorrect or invalid review comment.
77-113: Redaction looks correct and minimalPlayers are rebuilt without persistentID using destructuring, while preserving required fields. Clean and readable.
165-200: Nice: fallback to API when R2 body is empty or read failsGood use of info-level logs for non-fatal paths and validation before returning. This will reduce flake.
252-265: Good: strict schema validation with clear error messagesvalidateRecord returns typed data or a concise error string. This keeps exceptions out of the control flow and fits the “errors as values” approach.
|
I couldn't implement everything you suggested, but I did improve the error display. It's now translatable and specifies the exact type - replay missing, failed to fetch from the API, or failed to parse. I couldn't figure out a clean way to pass detailed error info with translations without switching to exceptions. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/server/Archive.ts (5)
36-44: Avoid mutating input; build a local record with commit (immutability)Do not change
gameRecordin-place. Compose a new object withgitCommitand pass that through. Safer and easier to reason about.- gameRecord.gitCommit = config.gitCommit(); - // Archive to R2 - await archiveAnalyticsToR2(stripTurns(gameRecord)); + const recordWithCommit: GameRecord = { + ...gameRecord, + gitCommit: config.gitCommit(), + }; + // Archive to R2 + await archiveAnalyticsToR2(stripTurns(recordWithCommit)); @@ - await archiveFullGameToR2(stripPersistentIds(gameRecord)); + await archiveFullGameToR2(stripPersistentIds(recordWithCommit));
149-163: Unify error handling/log details with other R2 writesThis catch logs only a short message and drops context. Mirror the richer pattern used above so we keep message/stack/name in logs.
- } catch (error) { - log.error(`error saving game ${gameRecord.info.gameID}`); - throw error; + } catch (error: unknown) { + if (!(error instanceof Error)) { + log.error( + `${gameRecord.info.gameID}: Error writing full game to R2. Non-Error type: ${String(error)}`, + ); + throw error; + } + const { message, stack, name } = error; + log.error(`${gameRecord.info.gameID}: Error writing full game to R2: ${error}`, { + message, + stack, + name, + ...(error && typeof error === "object" ? error : {}), + }); + throw error; }
165-168: Tighten return type with a string-literal unionRight now:
Promise<RedactedGameRecord | string>. Prefer a typed union so callers can exhaustively handle all cases at compile time.-export async function readGameRecord( - gameId: GameID, -): Promise<RedactedGameRecord | string> { +export async function readGameRecord( + gameId: GameID, +): Promise<RedactedGameRecord | ReadGameRecordError> {Add this type (outside the shown range) near the imports:
// Place near imports in this file type ReadGameRecordError = | "Game not found" | "Failed to read record" | "Failed to parse record data";
253-266: Right choice to validate with Zod; consider log level for expected parse failuresValidation and prettified error are solid. If replay version mismatches are expected/noisy (e.g., until v25), consider
warnorinfoinstead oferrorto reduce log noise.- log.error(`${gameId}: Error parsing game record: ${error}`); + log.warn(`${gameId}: Error parsing game record: ${error}`);If you do lower severity now, plan to revert to
erroronce old formats are no longer common.
64-75: Add a narrow error union type across all read helpers (stronger typing end-to-end)To fully leverage typed unions, apply the same
ReadGameRecordErrorreturn inreadGameRecordFallbackandvalidateRecordas well. This keeps the whole flow strictly typed and self-documenting.-export async function readGameRecordFallback( - gameId: GameID, -): Promise<RedactedGameRecord | string> { +export async function readGameRecordFallback( + gameId: GameID, +): Promise<RedactedGameRecord | ReadGameRecordError> { @@ -function validateRecord( +function validateRecord( json: unknown, gameId: GameID, -): RedactedGameRecord | string { +): RedactedGameRecord | ReadGameRecordError {Also applies to: 77-113, 115-147, 149-163, 165-200, 202-252, 253-266
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/Archive.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 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.
📚 Learning: 2025-05-21T04:10:59.706Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:44-53
Timestamp: 2025-05-21T04:10:59.706Z
Learning: The PlayerStats type from ArchiveSchemas already includes undefined in its definition, making explicit union types with undefined (PlayerStats | undefined) redundant.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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).
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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).
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-18T23:43:38.150Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:189-209
Timestamp: 2025-05-18T23:43:38.150Z
Learning: When working with zod schemas in the OpenFrontIO project, values can be marked as possibly undefined by zod even if logic suggests they should always be defined. It's better to preserve the `| undefined` union types in method signatures to align with zod's type inference, even if runtime checks throw errors for invalid cases.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-21T04:10:59.706Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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`.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-06-19T19:31:29.475Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-26T09:52:52.465Z
Learnt from: tnhnblgl
PR: openfrontio/OpenFrontIO#875
File: src/client/index.html:396-402
Timestamp: 2025-05-26T09:52:52.465Z
Learning: In the OpenFrontIO project, the correct path for icons from src/client/index.html is ../../resources/icons/, not /static/icons/ as the build process handles the path mapping differently.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-07-15T17:20:17.831Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#1441
File: tests/LangCode.test.ts:19-32
Timestamp: 2025-07-15T17:20:17.831Z
Learning: In the OpenFrontIO codebase, for unit tests it's acceptable to let exceptions (like JSON parsing errors) propagate and fail the test naturally rather than adding explicit error handling with try-catch blocks, as the test framework will catch them and fail the test appropriately.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-16T12:06:01.732Z
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:
src/server/Archive.ts
📚 Learning: 2025-06-07T13:15:55.439Z
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/server/Archive.ts
🔇 Additional comments (5)
src/server/Archive.ts (5)
64-75: LGTM: analytics view builder is clean and typedThe
stripTurnshelper returns a minimalAnalyticsRecordwithout turns. Clear, side‑effect free.
77-113: LGTM: PII redaction for players is correct
persistentIDis fully removed via structured mapping, not masked. This matches past guidance and keeps the redacted type sound.
115-147: LGTM: analytics archiving and error handlingGood use of
replacer, consistent keying (.jsonsuffix), and structured logging. Re-throws are correct for caller handling.
175-182: LGTM: R2 read path with graceful fallbackEmpty body detection, robust info-level logging, and delegation to the public API fallback are all sensible for resilience.
Also applies to: 186-199
2-10: Zod version verified
The project depends on zod ^4.0.5, so you’re on v4 andz.prettifyErroris available. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/server/Archive.ts (2)
38-46: Archive analytics + full game: OK; consider parallelizing writesCurrent sequencing is correct. As an optional optimization, you can fire both uploads concurrently when turns > 0:
const tasks = [archiveAnalyticsToR2(stripTurns(gameRecord))]; if (gameRecord.turns.length > 0) { tasks.push(archiveFullGameToR2(stripPersistentIds(gameRecord))); } await Promise.all(tasks);
255-268: Centralized validation + clear error stringsvalidateRecord keeps parsing/validation concerns in one place, returning typed data or a small string union. Consider codifying the error strings into a union type to improve type safety across server and client:
// e.g., in a shared types module export type ArchivedRecordError = | "Game not found" | "Failed to read record" | "Failed to parse record data"; // Then use: Promise<RedactedGameRecord | ArchivedRecordError>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/Schemas.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)src/server/Archive.ts(6 hunks)src/server/Worker.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/configuration/DefaultConfig.ts
- src/core/Schemas.ts
🧰 Additional context used
🧠 Learnings (14)
📓 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.
📚 Learning: 2025-05-21T04:10:59.706Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:44-53
Timestamp: 2025-05-21T04:10:59.706Z
Learning: The PlayerStats type from ArchiveSchemas already includes undefined in its definition, making explicit union types with undefined (PlayerStats | undefined) redundant.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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).
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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).
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-18T23:43:38.150Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:189-209
Timestamp: 2025-05-18T23:43:38.150Z
Learning: When working with zod schemas in the OpenFrontIO project, values can be marked as possibly undefined by zod even if logic suggests they should always be defined. It's better to preserve the `| undefined` union types in method signatures to align with zod's type inference, even if runtime checks throw errors for invalid cases.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-21T04:10:59.706Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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`.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-06-19T19:31:29.475Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-26T09:52:52.465Z
Learnt from: tnhnblgl
PR: openfrontio/OpenFrontIO#875
File: src/client/index.html:396-402
Timestamp: 2025-05-26T09:52:52.465Z
Learning: In the OpenFrontIO project, the correct path for icons from src/client/index.html is ../../resources/icons/, not /static/icons/ as the build process handles the path mapping differently.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-07-15T17:20:17.831Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#1441
File: tests/LangCode.test.ts:19-32
Timestamp: 2025-07-15T17:20:17.831Z
Learning: In the OpenFrontIO codebase, for unit tests it's acceptable to let exceptions (like JSON parsing errors) propagate and fail the test naturally rather than adding explicit error handling with try-catch blocks, as the test framework will catch them and fail the test appropriately.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-16T12:06:01.732Z
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:
src/server/Archive.ts
📚 Learning: 2025-06-07T13:15:55.439Z
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/server/Archive.ts
📚 Learning: 2025-06-02T14:27:37.609Z
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.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-06-09T02:20:43.637Z
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/server/Worker.ts
🧬 Code Graph Analysis (1)
src/server/Worker.ts (1)
src/core/Util.ts (1)
replacer(288-290)
🪛 Biome (2.1.2)
src/server/Worker.ts
[error] 229-229: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
🪛 GitHub Check: 🔍 ESLint
src/server/Worker.ts
[failure] 230-230:
Expected object keys to be in ascending order. 'error' should be before 'success'.
🪛 GitHub Actions: 🧪 CI
src/server/Worker.ts
[error] 232-232: TypeScript error TS1117: An object literal cannot have multiple properties with the same name.
🔇 Additional comments (10)
src/server/Worker.ts (2)
18-18: Global JSON replacer import — good choiceThis aligns server JSON serialization with BigInt-safe responses consistently.
77-77: Enable Express JSON replacer — nice and cleanSetting app.set("json replacer", replacer) removes the need for manual stringify. Good, idiomatic Express.
src/server/Archive.ts (8)
2-2: Consistent Zod importUsing named import
zmatches repo standards and works with z.prettifyError.
4-10: Use of RedactedGameRecord and schema is appropriateImporting and validating against RedactedGameRecord avoids PII leakage and keeps types honest.
66-77: stripTurns: concise and clearClean extraction of analytics fields. No unnecessary data retained.
79-115: stripPersistentIds removes PII correctlyOmitting persistentID via destructuring is clean and type-safe. Good use of a redacted type.
117-129: BigInt-safe JSON body for analytics — goodUsing the shared replacer keeps serialization consistent across endpoints.
151-165: Full-game archive path and BigInt-safe body — goodKey path matches the reader; replacer applied. All good.
167-202: R2 read with graceful fallback — solid
- Empty body check before parsing is good.
- On errors, falling back to public API keeps replays available for local dev.
204-253: Fallback fetch: timeouts + content-type check are robustAbortSignal.timeout and strict JSON content-type check are good safeguards. Validation is centralized.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/client/JoinPrivateLobbyModal.ts (2)
243-254: Localize “Version mismatch” messageThis user-facing string bypasses translateText, which breaks i18n and contradicts the PR goal that all UI text is localized.
Apply this diff:
- this.message = - "This game was created with a different version. Cannot join."; + this.message = translateText("replay.error.version_mismatch");Optionally include details via params if you expose safe fields:
// example if details has { serverHash, clientHash } this.message = translateText("replay.error.version_mismatch", { // serverHash, clientHash });Per team standards, update en.json only; other locales will be handled by translators.
130-141: Bug: lobby id cleared before dispatching "leave-lobby"close() clears lobbyIdInput.value (Line 122). You then dispatch leave-lobby using the cleared value. Capture it first.
- public closeAndLeave() { - this.close(); - this.hasJoined = false; - this.message = ""; - this.dispatchEvent( - new CustomEvent("leave-lobby", { - detail: { lobby: this.lobbyIdInput.value }, - bubbles: true, - composed: true, - }), - ); - } + public closeAndLeave() { + const lobby = this.lobbyIdInput.value; + this.close(); + this.hasJoined = false; + this.message = ""; + this.dispatchEvent( + new CustomEvent("leave-lobby", { + detail: { lobby }, + bubbles: true, + composed: true, + }), + ); + }
♻️ Duplicate comments (1)
src/client/JoinPrivateLobbyModal.ts (1)
269-271: Good: translate error key and short-circuit once handledUsing translateText(archiveData.error) and returning early matches the earlier guidance to send translation keys from the server. This keeps UI strings localized and avoids falling through to “not found.”
🧹 Nitpick comments (2)
src/client/JoinPrivateLobbyModal.ts (2)
270-273: Fallback when translation key is missingIf the server sends a key not yet in en.json, translateText returns the key itself. Show a generic localized message instead of the raw key/server string.
Apply this diff:
- } else if (archiveData.error) { - this.message = translateText(archiveData.error); - return true; - } + } else if (archiveData.error) { + const key = archiveData.error; + const localized = translateText(key); + this.message = + localized !== key ? localized : translateText("replay.error.generic"); + return true; + }Ensure en.json has replay.error.generic.
26-27: Use browser-friendly timer typeUse ReturnType for compatibility in browser builds.
- private playersInterval: NodeJS.Timeout | null = null; + private playersInterval: ReturnType<typeof setInterval> | null = null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/JoinPrivateLobbyModal.ts(1 hunks)src/core/Schemas.ts(1 hunks)src/server/Worker.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/Worker.ts
- src/core/Schemas.ts
🧰 Additional context used
🧠 Learnings (6)
📓 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.
📚 Learning: 2025-06-09T02:20:43.637Z
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/JoinPrivateLobbyModal.ts
📚 Learning: 2025-06-02T14:27:37.609Z
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.
Applied to files:
src/client/JoinPrivateLobbyModal.ts
📚 Learning: 2025-06-02T14:27:23.893Z
Learnt from: andrewNiziolek
PR: openfrontio/OpenFrontIO#1007
File: resources/lang/he.json:138-138
Timestamp: 2025-06-02T14:27:23.893Z
Learning: andrewNiziolek's project uses community translation for internationalization. When updating map names or other user-facing text, they update the keys programmatically but wait for community translators to provide accurate translations in each language rather than using machine translations.
Applied to files:
src/client/JoinPrivateLobbyModal.ts
📚 Learning: 2025-07-12T08:41:35.101Z
Learnt from: Aotumuri
PR: openfrontio/OpenFrontIO#1357
File: resources/lang/de.json:523-540
Timestamp: 2025-07-12T08:41:35.101Z
Learning: In OpenFrontIO project localization files, always check the en.json source file before flagging potential spelling errors in other language files, as some keys may intentionally use non-standard spellings that need to be consistent across all translations.
Applied to files:
src/client/JoinPrivateLobbyModal.ts
📚 Learning: 2025-05-30T03:53:52.231Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#949
File: resources/lang/en.json:8-10
Timestamp: 2025-05-30T03:53:52.231Z
Learning: For the OpenFrontIO project, do not suggest updating translation files in resources/lang/*.json except for en.json. The project has a dedicated translation team that handles all other locale files.
Applied to files:
src/client/JoinPrivateLobbyModal.ts
🧬 Code Graph Analysis (1)
src/client/JoinPrivateLobbyModal.ts (1)
src/client/Utils.ts (1)
translateText(82-137)
🔇 Additional comments (1)
src/client/JoinPrivateLobbyModal.ts (1)
241-242: Ignore schema refactor—error is already a strict literal union
TheWorkerApiArchivedGameLobbySchemadefineserroras exactly"Game not found"or"Version mismatch". It does not accept arbitrary strings, so no change to make it a tighter z.enum is needed. Also, there are noreplay.error.*keys in your i18n files, so replacing the schema literals with those keys isn’t applicable.Likely an incorrect or invalid review comment.
Description:
This PR introduces a graceful fallback path for loading replay files.
If fetching a replay from Cloudflare R2 fails, the server now retries via the public endpoint
https://api.openfront.io/game/{gameId}, so developers can replay games locally.Closes issue #1571
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
aaa4xu