Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughClan tags are separated from usernames and threaded through validation, storage, transport, server censorship, game models, UI rendering, leaderboards, and tests; displayName is derived from username + clanTag and used everywhere instead of parsing tags from usernames. Changes
sequenceDiagram
participant UI as Client UI
participant Input as UsernameInput
participant Transport as Client Transport
participant Server as Game Server
participant Game as GameRunner
participant Archive as Archive
UI->>Input: getUsername() / getClanTag()
Input-->>UI: { username, clanTag }
UI->>Transport: joinGame({ username, clanTag })
Transport->>Server: ClientJoinMessage(username, clanTag)
Server->>Server: censor(username, clanTag) -> { username', clanTag' }
Server->>Game: start/rejoin with sanitized players (username', clanTag')
Game->>Game: new PlayerInfo(..., clanTag')
Game->>Archive: archive game with players including clanTag
Archive->>Archive: store leaderboard entries with username + clanTag
UI->>UI: formatPlayerDisplayName(username, clanTag) -> displayName
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/components/leaderboard/LeaderboardPlayerList.ts (1)
428-437:⚠️ Potential issue | 🟡 MinorSticky footer is missing the clan tag badge.
The player row renders a clan tag badge + username (lines 244–252). The sticky "Your ranking" footer shows only
username— the clan tag badge is absent. A user with a clan tag will see it in the table but not in the footer bar.🔧 Suggested fix — add clan tag badge to the footer
<div class="flex-1 flex flex-col ml-4"> <span class="text-[10px] uppercase font-bold text-blue-200/60 leading-tight" >${translateText("leaderboard_modal.your_ranking")}</span > - <span class="font-bold text-white text-base" - >${this.currentUserEntry.username}</span - > + <div class="flex items-center gap-2 mt-0.5"> + ${this.currentUserEntry.clanTag + ? html`<div + class="px-2 py-0.5 rounded bg-white/20 border border-white/30 text-[10px] font-bold text-white shrink-0" + > + ${this.currentUserEntry.clanTag} + </div>` + : ""} + <span class="font-bold text-white text-base truncate" + >${this.currentUserEntry.username}</span + > + </div> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/leaderboard/LeaderboardPlayerList.ts` around lines 428 - 437, The sticky footer in LeaderboardPlayerList displays this.currentUserEntry.username but omits the clan tag badge; update the footer markup inside the "Your ranking" block to render the same clan tag badge used by the player row (reuse the same element/component or helper used earlier in this file — e.g., the clan badge markup or renderClanTag/renderPlayerBadge helper that the player row uses) and place it immediately before the username so users with a clan tag see the badge in the sticky footer as well.src/core/ApiSchemas.ts (1)
160-171:⚠️ Potential issue | 🟡 MinorAlign
PlayerLeaderboardEntrySchema.clanTagnullability withRankedLeaderboardEntrySchema.clanTag.Both schemas use the same ranked leaderboard API endpoint. Server code in
Privilege.tsexplicitly returnsclanTag: string | null, and the client transformation at line 77 ofLeaderboardPlayerList.tshandles null values (entry.clanTag ?? undefined). Since the API sendsnullfor censored or missing clan tags,PlayerLeaderboardEntrySchema.clanTagshould use.nullable().optional()instead of just.optional()to match the actual data source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/ApiSchemas.ts` around lines 160 - 171, PlayerLeaderboardEntrySchema currently defines clanTag as optional only, but the API and server (see Privilege.ts) return clanTag: string | null and the client transform (LeaderboardPlayerList.ts handling entry.clanTag ?? undefined) expects nullable values; update PlayerLeaderboardEntrySchema so the clanTag field uses .nullable().optional() to match RankedLeaderboardEntrySchema and the actual API shape (search for the PlayerLeaderboardEntrySchema declaration to modify).
🧹 Nitpick comments (3)
tests/GameInfoRanking.test.ts (1)
111-123: Consider assertingclanTagis correctly propagated.The test data adds
clanTag: "X"to playerp1, but the "summarizes players correctly" test only checksusername. It's worth verifying the clanTag is also surfaced on the resultingPlayerInfoobject.➕ Suggested addition
expect(p1.username).toBe("Alice"); + expect(p1.clanTag).toBe("X"); expect(p1.flag).toBe("USA");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/GameInfoRanking.test.ts` around lines 111 - 123, The test "summarizes players correctly" in GameInfoRanking.test.ts should assert that the clanTag from the input player is propagated into the produced PlayerInfo; after locating p1 (from Ranking.sortedBy(RankType.ConquestHumans)), add an assertion that p1.clanTag equals "X" to validate clanTag propagation (or, if the mapping logic is missing, update Ranking.sortedBy / the PlayerInfo mapping to copy clanTag from the source player before adding the test assertion).src/core/validations/username.ts (1)
1-2:translateTextimport from client-sideUtilsin acoremodule.This function lives under
src/core/validations/but imports from../../client/Utils. IfvalidateClanTagis ever called on the server side,translateText(which depends on the DOM) will break. The existingvalidateUsernamealready has this coupling, so it's a pre-existing concern — just noting it here for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/validations/username.ts` around lines 1 - 2, validateClanTag imports translateText from a client-side Utils (same issue exists for validateUsername) which will break when run server-side; fix by removing the direct client import and using a server-safe translation util or dependency-injecting the translator: either move or re-implement translateText into a core/server-safe module (used by validateClanTag and validateUsername) or change those functions to accept a translate function parameter and update callers to pass the appropriate client or server implementation, then update imports/references to use the new core-safe symbol or injected parameter.tests/Privilege.test.ts (1)
94-98: Missing test: clean clan tag gets uppercased.The
censorWithMatcherimplementation (Privilege.ts line 98) callsclanTag.toUpperCase(). But this test uses"COOL"which is already uppercase. Consider adding a test with a mixed-case tag like"Cool"to verify it returns"COOL".Suggested test
test("preserves clean clan tag when username is profane", () => { const result = checker.censor("hitler", "COOL"); expect(result.clanTag).toBe("COOL"); expect(shadowNames).toContain(result.username); }); + + test("uppercases clean clan tag", () => { + const result = checker.censor("CoolPlayer", "Cool"); + expect(result.username).toBe("CoolPlayer"); + expect(result.clanTag).toBe("COOL"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Privilege.test.ts` around lines 94 - 98, Add a test to assert that a clean clan tag is uppercased by censorWithMatcher: call checker.censor with a profane username (e.g., "hitler") and a mixed-case clan tag like "Cool" and expect result.clanTag toEqual "COOL" (and still assert the username is shadowed via shadowNames), since Privilege.ts's censorWithMatcher calls clanTag.toUpperCase().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/components/LobbyPlayerView.ts`:
- Around line 374-385: getClientDisplayName is seeding createRandomName with the
full display name (formatPlayerDisplayName result, including clan tag) which
differs from GameView's PlayerView constructor that seeds createRandomName with
this.data.name (raw username), causing inconsistent anonymous names; change
getClientDisplayName to pass client.username to createRandomName (still using
PlayerType.Human and falling back to full) so the same seed is used in lobby and
in-game.
In `@src/client/UsernameInput.ts`:
- Around line 150-169: The clan-tag validation in validateClanTag (in
src/core/validations/username.ts) returns the wrong translation key for tags
that exceed MAX_CLAN_TAG_LENGTH; update the branch that checks for tag length >
MAX_CLAN_TAG_LENGTH to return translateText("username.tag_too_long") (instead of
"username.tag_too_short") so the error message correctly indicates a tag is too
long; verify the validateClanTag function's length check and its returned error
key are consistent and update any related unit tests that assert the expected
translation key.
In `@src/core/game/GameView.ts`:
- Around line 656-675: The current per-iteration check uses a displayName
fallback inside the gu.updates[GameUpdateType.Player].forEach and may adopt the
first matching pu when _myPlayer is null; change this to collect candidate
updates first (e.g., push pu into a local candidates array when the
isMyPlayerUpdate condition matches) and remove the immediate assignment inside
the forEach; after the loop, if candidates.length === 1 then set pu.name and
pu.displayName using this._myUsername and
formatPlayerDisplayName(this._myUsername, this._myClanTag) (otherwise do nothing
or log/skip) so you only claim an update when exactly one candidate matches,
preserving the existing post-loop single-match guard behavior; reference
symbols: isMyPlayerUpdate check, _myClientID, _myPlayer,
formatPlayerDisplayName, gu.updates[GameUpdateType.Player], PlayerType.Human.
In `@src/core/validations/username.ts`:
- Around line 57-62: The second length check in the username validation uses the
wrong translation key: in src/core/validations/username.ts change the return for
the MAX_CLAN_TAG_LENGTH branch to use translateText("username.tag_too_long")
instead of "username.tag_too_short" (refer to MIN_CLAN_TAG_LENGTH,
MAX_CLAN_TAG_LENGTH and translateText in that file to locate the check); then
add the new "username.tag_too_long" entry to each locale file that contains
"username.tag_too_short" (bg.json, en.json, fr.json, id.json, ja.json, nl.json,
ru.json, tr.json, uk.json, zh-CN.json) with appropriate translations.
In `@src/server/GamePreviewBuilder.ts`:
- Line 7: External archived usernames like "[TAG] Username" are being rejected
by UsernameSchema and causing ExternalGameInfoSchema.safeParse() to fail; change
the username field in ExternalGameInfoSchema to use z.string().optional()
instead of UsernameSchema.optional() so legacy names with brackets pass
validation; update the username property in
GamePreviewBuilder/ExternalGameInfoSchema (where UsernameSchema is referenced)
and keep existing fallback logic in parseWinner and stripClanTagFromUsername
intact so missing or legacy usernames are handled by clientId fallback.
In `@tests/Censor.test.ts`:
- Around line 63-67: The test "rejects too long clan tag" currently expects the
incorrect error "username.tag_too_short"; update the expectation to
"username.tag_too_long" to match the corrected behavior of validateClanTag (in
function validateClanTag which checks against MAX_CLAN_TAG_LENGTH). Ensure the
test uses validateClanTag("A".repeat(MAX_CLAN_TAG_LENGTH + 1)) and asserts
res.isValid is false and res.error === "username.tag_too_long".
---
Outside diff comments:
In `@src/client/components/leaderboard/LeaderboardPlayerList.ts`:
- Around line 428-437: The sticky footer in LeaderboardPlayerList displays
this.currentUserEntry.username but omits the clan tag badge; update the footer
markup inside the "Your ranking" block to render the same clan tag badge used by
the player row (reuse the same element/component or helper used earlier in this
file — e.g., the clan badge markup or renderClanTag/renderPlayerBadge helper
that the player row uses) and place it immediately before the username so users
with a clan tag see the badge in the sticky footer as well.
In `@src/core/ApiSchemas.ts`:
- Around line 160-171: PlayerLeaderboardEntrySchema currently defines clanTag as
optional only, but the API and server (see Privilege.ts) return clanTag: string
| null and the client transform (LeaderboardPlayerList.ts handling entry.clanTag
?? undefined) expects nullable values; update PlayerLeaderboardEntrySchema so
the clanTag field uses .nullable().optional() to match
RankedLeaderboardEntrySchema and the actual API shape (search for the
PlayerLeaderboardEntrySchema declaration to modify).
---
Nitpick comments:
In `@src/core/validations/username.ts`:
- Around line 1-2: validateClanTag imports translateText from a client-side
Utils (same issue exists for validateUsername) which will break when run
server-side; fix by removing the direct client import and using a server-safe
translation util or dependency-injecting the translator: either move or
re-implement translateText into a core/server-safe module (used by
validateClanTag and validateUsername) or change those functions to accept a
translate function parameter and update callers to pass the appropriate client
or server implementation, then update imports/references to use the new
core-safe symbol or injected parameter.
In `@tests/GameInfoRanking.test.ts`:
- Around line 111-123: The test "summarizes players correctly" in
GameInfoRanking.test.ts should assert that the clanTag from the input player is
propagated into the produced PlayerInfo; after locating p1 (from
Ranking.sortedBy(RankType.ConquestHumans)), add an assertion that p1.clanTag
equals "X" to validate clanTag propagation (or, if the mapping logic is missing,
update Ranking.sortedBy / the PlayerInfo mapping to copy clanTag from the source
player before adding the test assertion).
In `@tests/Privilege.test.ts`:
- Around line 94-98: Add a test to assert that a clean clan tag is uppercased by
censorWithMatcher: call checker.censor with a profane username (e.g., "hitler")
and a mixed-case clan tag like "Cool" and expect result.clanTag toEqual "COOL"
(and still assert the username is shadowed via shadowNames), since
Privilege.ts's censorWithMatcher calls clanTag.toUpperCase().
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/client/JoinLobbyModal.ts (1)
532-537:JSON.stringifyfor deep equality is fragile — prefer field-level comparison.
JSON.stringifypreserves insertion order, so if the server ever sends the same config with properties in a different order (e.g., after a server refactor), the comparison would report a false inequality and trigger an unnecessary re-render. While not a correctness bug (the update is idempotent), it is a subtle maintenance hazard.♻️ Proposed field-level comparison
- if ( - JSON.stringify(this.gameConfig) !== JSON.stringify(lobby.gameConfig) - ) { - this.gameConfig = lobby.gameConfig; - } + const gc = lobby.gameConfig; + const cur = this.gameConfig; + const configChanged = + cur === null || + cur.gameMap !== gc.gameMap || + cur.gameMode !== gc.gameMode || + cur.maxPlayers !== gc.maxPlayers || + cur.playerTeams !== gc.playerTeams || + cur.disableNations !== gc.disableNations || + cur.gameType !== gc.gameType; + if (configChanged) { + this.gameConfig = gc; + }Adjust fields as needed to cover the full
GameConfigshape — or extract agameConfigsEqualhelper alongsideclientInfoListsEqualinsrc/core/Util.tsfor reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/JoinLobbyModal.ts` around lines 532 - 537, Replace the fragile JSON.stringify comparison between this.gameConfig and lobby.gameConfig with a field-level equality check: either call a new helper gameConfigsEqual (add it alongside clientInfoListsEqual in Util.ts) or inline comparisons of the GameConfig fields used by the UI, then only assign this.gameConfig = lobby.gameConfig when the helper returns false; reference this.gameConfig and lobby.gameConfig in the change and ensure the helper covers the full GameConfig shape to avoid order-dependent false negatives.src/core/Util.ts (1)
360-363: Nitpick:?? undefinedis a no-op here.
clanTagisstring | undefinedfromClientInfo, soleft.clanTag ?? undefinedalways equalsleft.clanTag. The coercion only does anything fornull, which can't appear here.♻️ Simplify the comparison
- (left.clanTag ?? undefined) !== (right.clanTag ?? undefined) + left.clanTag !== right.clanTag🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Util.ts` around lines 360 - 363, The comparison uses a redundant nullish coalescing on clanTag; in the conditional inside Util.ts that checks left.clientID, left.username, and clan tags (references: left.clientID, right.clientID, left.username, right.username, left.clanTag, right.clanTag), remove the no-op "?? undefined" and compare clanTag directly (e.g., use left.clanTag !== right.clanTag) or simplify the whole if to directly compare the three fields without coercion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/components/LobbyPlayerView.ts`:
- Around line 320-327: PlayerInfo construction is failing because
ClientInfo.clanTag is string | undefined but PlayerInfo expects string | null;
update the call site (the new PlayerInfo(...) invocation) to convert undefined
to null (e.g., use the nullish coalescing operator on c.clanTag) so the passed
value is exactly string | null and satisfies the PlayerInfo constructor
signature.
---
Nitpick comments:
In `@src/client/JoinLobbyModal.ts`:
- Around line 532-537: Replace the fragile JSON.stringify comparison between
this.gameConfig and lobby.gameConfig with a field-level equality check: either
call a new helper gameConfigsEqual (add it alongside clientInfoListsEqual in
Util.ts) or inline comparisons of the GameConfig fields used by the UI, then
only assign this.gameConfig = lobby.gameConfig when the helper returns false;
reference this.gameConfig and lobby.gameConfig in the change and ensure the
helper covers the full GameConfig shape to avoid order-dependent false
negatives.
In `@src/core/Util.ts`:
- Around line 360-363: The comparison uses a redundant nullish coalescing on
clanTag; in the conditional inside Util.ts that checks left.clientID,
left.username, and clan tags (references: left.clientID, right.clientID,
left.username, right.username, left.clanTag, right.clanTag), remove the no-op
"?? undefined" and compare clanTag directly (e.g., use left.clanTag !==
right.clanTag) or simplify the whole if to directly compare the three fields
without coercion.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/server/GameServer.ts (1)
284-287: Identity update is locked once prestart fires — confirm this is intended.
hasStarted()returns_hasStarted || _hasPrestarted, so!this.hasStarted()isfalsefor the ~2-second window betweenprestart()andstart(). Anyjoinmessage that arrives during that window with an updated clan tag will have the identity silently dropped.This is almost certainly fine in practice (2-second window, rare race), but worth a comment in the code so the next maintainer doesn't wonder why a pre-game identity update was ignored.
💡 Optional inline note
- if (identityUpdate && !this.hasStarted()) { + // hasStarted() is true once prestart fires, so identity is locked + // from that point on — this is intentional. + if (identityUpdate && !this.hasStarted()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/GameServer.ts` around lines 284 - 287, The identity update block silently ignores updates if this.hasStarted() is true (which returns _hasStarted || _hasPrestarted), causing join messages received in the prestart→start window to drop clan tag/username changes; add a concise inline comment next to the identityUpdate handling (around the if (identityUpdate && !this.hasStarted()) block in GameServer.ts) explaining that prestart locks identity for the short prestart→start window (≈2s) and that this behavior is intentional to avoid races, so future maintainers understand why updates are skipped; if intended otherwise, change the condition to use only _hasStarted or adjust timing in prestart()/start() instead.src/server/Worker.ts (1)
338-374: Therejoinvsjoin-path identity split is correct — worth a brief comment.The
rejoinmessage path (line 338) intentionally skips censorship and identity update: the client is mid-game, and the identity set on first join (already censored) is the one that must stay. Thejoin-path rejoin (line 367) applies the latest censored identity, but only before prestart (GameServer.rejoinClientguards with!hasStarted()).This is the right design, but the
rejoinbranch has no comment explaining whyidentityUpdateis absent, which may confuse a future reader. Consider a brief note:💡 Optional inline note on `rejoin` path
if (clientMsg.type === "rejoin") { + // No identity update for reconnects: the username/clanTag + // set at initial join is the canonical in-game identity. log.info("rejoining game", {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/Worker.ts` around lines 338 - 374, Add a short clarifying comment above the early "rejoin" branch (the block that calls gm.rejoinClient(ws, persistentId, clientMsg.gameID, clientMsg.lastTurn)) explaining that this path intentionally avoids applying privilegeRefresher.get().censor() or updating the client's identity because it represents an in-progress game where the original censored identity from first join must be preserved; also note that the later join-path rejoin (the gm.rejoinClient call with lastTurn 0 and the censored identity) is used only for prestart reconnections and will apply updated censorship/identity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/server/GameServer.ts`:
- Around line 284-287: The identity update block silently ignores updates if
this.hasStarted() is true (which returns _hasStarted || _hasPrestarted), causing
join messages received in the prestart→start window to drop clan tag/username
changes; add a concise inline comment next to the identityUpdate handling
(around the if (identityUpdate && !this.hasStarted()) block in GameServer.ts)
explaining that prestart locks identity for the short prestart→start window
(≈2s) and that this behavior is intentional to avoid races, so future
maintainers understand why updates are skipped; if intended otherwise, change
the condition to use only _hasStarted or adjust timing in prestart()/start()
instead.
In `@src/server/Worker.ts`:
- Around line 338-374: Add a short clarifying comment above the early "rejoin"
branch (the block that calls gm.rejoinClient(ws, persistentId, clientMsg.gameID,
clientMsg.lastTurn)) explaining that this path intentionally avoids applying
privilegeRefresher.get().censor() or updating the client's identity because it
represents an in-progress game where the original censored identity from first
join must be preserved; also note that the later join-path rejoin (the
gm.rejoinClient call with lastTurn 0 and the censored identity) is used only for
prestart reconnections and will apply updated censorship/identity.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/core/ApiSchemas.ts (2)
25-25:LeaderboardClanTagSchemais wider than needed in both size and character set.
max(10)is double theStrictClanTagSchemacap of 5. Legacy clan tags were always alphanumeric and at most 5 characters, so 10 allows values that have never been valid.- No character whitelist means the schema silently accepts control characters, angle brackets, or emoji that could cause display or injection issues downstream.
♻️ Tighter legacy schema
-const LeaderboardClanTagSchema = z.string().trim().min(1).max(10); +const LeaderboardClanTagSchema = z.string().trim().regex(/^[a-zA-Z0-9 \[\]_.]{1,5}$/);If the broader character set is intentional for a specific set of known legacy records, documenting that in a comment would help clarify the intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/ApiSchemas.ts` at line 25, LeaderboardClanTagSchema is too permissive: reduce the max length to match legacy cap and restrict allowed characters to a safe whitelist (e.g., alphanumeric) to prevent control chars, angle brackets, or emoji; update the zod schema definition for LeaderboardClanTagSchema (and reference StrictClanTagSchema for the exact legacy constraints) to use .max(5) and a .regex/transform enforcing the legacy character set, and if the broader rules were intentional, add a clear comment above LeaderboardClanTagSchema explaining why legacy records need a wider charset.
21-24:UsernameSchemabranch in the union is unreachable for any invalid-username input.Because
UsernameSchema(min(3),max(27), restricted charset) is a strict subset of the fallbackz.string().min(1).max(64), every string that passes the first branch also passes the second. The fallback branch is the only one that ever "catches" values thatUsernameSchemarejects — but by then the output type is stillstring, so there is no runtime difference. The union simplifies to just the fallback.♻️ Proposed simplification
const LeaderboardUsernameSchema = z .string() .transform(stripClanTagFromUsername) - .pipe(z.union([UsernameSchema, z.string().min(1).max(64)])); + .pipe(z.string().min(1).max(64));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/ApiSchemas.ts` around lines 21 - 24, The union in LeaderboardUsernameSchema makes the UsernameSchema branch unreachable because z.string().min(1).max(64) always accepts what UsernameSchema would, so replace the union with a single schema; update LeaderboardUsernameSchema to use .pipe(z.string().min(1).max(64)) (keeping the existing .transform(stripClanTagFromUsername)) or, if you intended to enforce UsernameSchema specifically, replace the union with .pipe(UsernameSchema) instead—locate LeaderboardUsernameSchema and adjust the .pipe(...) target accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/ApiSchemas.ts`:
- Around line 15-24: stripClanTagFromUsername currently returns an empty string
for inputs like "[ABC]" which then fails LeaderboardUsernameSchema; update the
transform so that if stripping results in an empty string it returns the
original trimmed input (i.e., fallback to the pre-strip trimmed username) so the
union (UsernameSchema or fallback string) can validate correctly; locate
stripClanTagFromUsername and LeaderboardUsernameSchema in ApiSchemas.ts and
implement the non-empty fallback behavior (or perform the check inside the
transform) referencing the existing function name and the
LeaderboardUsernameSchema/UsernameSchema symbols.
- Line 13: Remove the duplicate StrictClanTagSchema declaration and reuse the
existing ClanTagSchema from ./Schemas: delete the const StrictClanTagSchema
definition, add ClanTagSchema to the import list from "./Schemas", and replace
any usages of StrictClanTagSchema in this file (e.g., where the API schema
references it) with ClanTagSchema; also remove any now-unused local
definitions/imports to keep imports clean.
---
Nitpick comments:
In `@src/core/ApiSchemas.ts`:
- Line 25: LeaderboardClanTagSchema is too permissive: reduce the max length to
match legacy cap and restrict allowed characters to a safe whitelist (e.g.,
alphanumeric) to prevent control chars, angle brackets, or emoji; update the zod
schema definition for LeaderboardClanTagSchema (and reference
StrictClanTagSchema for the exact legacy constraints) to use .max(5) and a
.regex/transform enforcing the legacy character set, and if the broader rules
were intentional, add a clear comment above LeaderboardClanTagSchema explaining
why legacy records need a wider charset.
- Around line 21-24: The union in LeaderboardUsernameSchema makes the
UsernameSchema branch unreachable because z.string().min(1).max(64) always
accepts what UsernameSchema would, so replace the union with a single schema;
update LeaderboardUsernameSchema to use .pipe(z.string().min(1).max(64))
(keeping the existing .transform(stripClanTagFromUsername)) or, if you intended
to enforce UsernameSchema specifically, replace the union with
.pipe(UsernameSchema) instead—locate LeaderboardUsernameSchema and adjust the
.pipe(...) target accordingly.
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/ApiSchemas.ts`:
- Around line 13-22: stripClanTagFromUsername currently returns an empty string
for inputs like "[ABC]" which then fails LeaderboardUsernameSchema's .min(1)
check and breaks parsing; modify the logic so that after removing a clan tag you
return the original username when the stripped result is empty (i.e., in
stripClanTagFromUsername return original input if strip yields ""), or implement
that fallback in the transform used by LeaderboardUsernameSchema so the
transformed value is never empty.
iiamlewis
added
the
Backend
ryanbarlow97
force-pushed
the
clantag
branch
2 times, most recently
from
2cbb973 to
964f5cd
Compare
evanpelle
reviewed
Mar 16, 2026
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Properly split out clantags and usernames, a clantag should not be part of a username.
https://api.openfront.dev/game/ojkqZFb2
https://api.openfront.dev/game/MF32BkVc
requires;
https://github.com/openfrontio/infra/pull/264
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n