Update auth #2559
Update auth #2559
Conversation
WalkthroughRefactors client auth and API surface: removes Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant Auth
participant API
participant DiscordOAuth
User->>Client: Click "Login with Discord"
Client->>Auth: discordLogin()
Auth->>DiscordOAuth: Redirect to Discord (authorize)
DiscordOAuth->>Auth: Return auth code
Auth->>API: POST /auth/discord (code)
API->>Auth: Return JWT
Auth->>Auth: Cache JWT, store persistent ID
Client->>Auth: userAuth() / getUserMe()
Auth->>API: GET /users/@me (Bearer JWT)
API->>Auth: UserMeResponse
Auth->>Client: user data (userMeResponse)
alt Token expired or near expiry
Auth->>API: POST /refresh (current JWT)
API->>Auth: New JWT
Auth->>Auth: Update cache
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning, 2 inconclusive)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/AccountModal.ts (1)
144-153: "Log Out" text should use translation.Line 150 has hardcoded "Log Out" text.
- Log Out + ${translateText("account_modal.log_out") || "Log Out"}
🧹 Nitpick comments (7)
src/client/Cosmetics.ts (1)
8-10: Update cosmetics purchase to use Api/Auth helpers (minor improvement possible)Using
getApiBase()from./Apiand awaitinggetAuthHeader()from./AuthinsidehandlePurchaseis consistent with the new async auth flow and should work correctly with your existing 401 handling.If
getAuthHeader()can return an empty string for unauthenticated users, you could optionally short‑circuit before doing the Stripe request to avoid a guaranteed failing call:const response = await fetch( `${getApiBase()}/stripe/create-checkout-session`, { method: "POST", headers: { "Content-Type": "application/json", - authorization: await getAuthHeader(), + authorization: await getAuthHeader(), "X-Persistent-Id": getPersistentID(), },and, for example:
const authHeader = await getAuthHeader(); if (!authHeader) { alert("You are not logged in. Please log in to purchase a pattern."); return; }This is optional, as the current 401 path already informs the user.
Also applies to: 20-28
src/client/ClientGameRunner.ts (1)
223-223: Async method with no await.The
asyncmodifier was added tosaveGame, but the method body contains noawaitexpressions. The method is called at line 311 withoutawait, so the returned Promise is ignored.If async behavior is not needed yet, consider keeping it synchronous. If preparing for future async work, this is fine as-is.
src/client/LocalServer.ts (1)
180-238: Consider using await instead of .then() chains.Since
endGameis nowasync, you could simplify the Promise chain at lines 223-237 usingawait:- compress(jsonString) - .then((compressedData) => { - return fetch(`/${workerPath}/api/archive_singleplayer_game`, { + try { + const compressedData = await compress(jsonString); + await fetch(`/${workerPath}/api/archive_singleplayer_game`, { method: "POST", headers: { "Content-Type": "application/json", "Content-Encoding": "gzip", }, body: compressedData, - keepalive: true, // Ensures request completes even if page unloads + keepalive: true, }); - }) - .catch((error) => { - console.error("Failed to archive singleplayer game:", error); - }); + } catch (error) { + console.error("Failed to archive singleplayer game:", error); + }src/client/Main.ts (1)
107-107: Consider adding error handling for the async initialize call.The
initialize()method is now async, but at line 600 it's called withoutawaitor.catch(). If initialization fails, the error may go unnoticed.// Initialize the client when the DOM is loaded document.addEventListener("DOMContentLoaded", () => { - new Client().initialize(); + new Client().initialize().catch((err) => { + console.error("Client initialization failed:", err); + }); });src/client/Api.ts (1)
74-76: Silent catch may hide errors.The catch block returns
falsewithout logging the error. This could make debugging difficult when issues occur.} catch (e) { + console.warn("getUserMe: request failed", e); return false; }src/client/Auth.ts (2)
18-32: Consider validating the response structure.The function extracts
const json = await response.json(); - const { email } = json; - return email; + if (typeof json?.email === "string") { + return json.email; + } + console.warn("tempTokenLogin: unexpected response structure", json); + return null;
99-111: Consider awaitinglogOut()calls.The
logOut()calls on lines 102 and 109 are not awaited. While the function returnsfalseanyway, the logout cleanup may not complete before the caller proceeds. If this is intentional (fire-and-forget), consider adding a comment.if (iss !== getApiBase()) { // JWT was not issued by the correct server console.error('unexpected "iss" claim value'); - logOut(); + await logOut(); return false; } const myAud = getAudience(); if (myAud !== "localhost" && aud !== myAud) { // JWT was not issued for this website console.error('unexpected "aud" claim value'); - logOut(); + await logOut(); return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
resources/lang/en.json(3 hunks)src/client/AccountModal.ts(7 hunks)src/client/Api.ts(1 hunks)src/client/Auth.ts(1 hunks)src/client/ClientGameRunner.ts(2 hunks)src/client/Cosmetics.ts(2 hunks)src/client/JoinPrivateLobbyModal.ts(1 hunks)src/client/LocalServer.ts(2 hunks)src/client/Main.ts(5 hunks)src/client/Matchmaking.ts(2 hunks)src/client/StatsModal.ts(1 hunks)src/client/TerritoryPatternsModal.ts(3 hunks)src/client/TokenLoginModal.ts(2 hunks)src/client/graphics/PlayerIcons.ts(1 hunks)src/client/graphics/layers/NameLayer.ts(1 hunks)src/client/graphics/layers/WinModal.ts(3 hunks)src/client/jwt.ts(0 hunks)src/core/GameRunner.ts(2 hunks)src/core/Util.ts(2 hunks)src/core/game/UnitImpl.ts(2 hunks)src/core/validations/username.ts(2 hunks)src/server/MapPlaylist.ts(0 hunks)
💤 Files with no reviewable changes (2)
- src/server/MapPlaylist.ts
- src/client/jwt.ts
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/GameRunner.tssrc/client/ClientGameRunner.tssrc/client/Matchmaking.tssrc/client/LocalServer.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/core/GameRunner.tssrc/client/ClientGameRunner.tssrc/client/LocalServer.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/core/GameRunner.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/core/GameRunner.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/JoinPrivateLobbyModal.tssrc/client/graphics/layers/WinModal.tssrc/client/Matchmaking.tssrc/client/AccountModal.tssrc/client/TerritoryPatternsModal.tssrc/client/Main.ts
📚 Learning: 2025-08-19T11:00:55.422Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 1864
File: resources/maps/arabianpeninsula/manifest.json:13-170
Timestamp: 2025-08-19T11:00:55.422Z
Learning: In OpenFrontIO, nation names in map manifests are displayed directly in the UI without translation. They do not need to be added to resources/lang/en.json or processed through translateText(). This is the established pattern across all existing maps including Europe, World, Asia, Africa, and others.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/game/UnitImpl.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/core/game/UnitImpl.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/WinModal.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/client/AccountModal.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/client/TerritoryPatternsModal.ts
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/Main.ts
🧬 Code graph analysis (12)
src/core/GameRunner.ts (4)
src/core/game/Game.ts (1)
PlayerInfo(404-418)src/core/game/GameView.ts (1)
clientID(342-344)src/core/Util.ts (1)
sanitize(173-177)src/core/validations/username.ts (1)
censorNameWithClanTag(68-95)
src/client/ClientGameRunner.ts (1)
src/core/game/GameUpdates.ts (1)
WinUpdate(246-250)
src/client/TokenLoginModal.ts (1)
src/client/Auth.ts (1)
tempTokenLogin(18-32)
src/client/Auth.ts (4)
src/core/ApiSchemas.ts (2)
TokenPayload(34-34)TokenPayloadSchema(11-33)src/client/Api.ts (2)
getApiBase(78-90)getAudience(92-96)src/core/Base64.ts (1)
base64urlToUuid(24-37)src/client/Utils.ts (1)
generateCryptoRandomUUID(62-90)
src/client/Cosmetics.ts (1)
src/client/Auth.ts (1)
getAuthHeader(34-39)
src/core/validations/username.ts (1)
src/core/Util.ts (2)
sanitize(173-177)getClanTagOriginalCase(347-350)
src/client/graphics/layers/WinModal.ts (2)
src/client/Utils.ts (2)
getGamesPlayed(222-229)translateText(92-147)src/client/LangSelector.ts (1)
translateText(258-278)
src/client/Api.ts (2)
src/client/Auth.ts (2)
userAuth(72-144)logOut(41-65)src/core/ApiSchemas.ts (3)
PlayerProfileSchema(86-91)UserMeResponse(56-56)UserMeResponseSchema(45-55)
src/client/AccountModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/client/Api.ts (1)
fetchPlayerById(10-48)
src/client/LocalServer.ts (1)
src/client/Auth.ts (1)
getPersistentID(177-184)
src/client/TerritoryPatternsModal.ts (1)
src/client/LangSelector.ts (1)
translateText(258-278)
src/client/Main.ts (1)
src/client/Auth.ts (2)
userAuth(72-144)getPlayToken(170-174)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (39)
src/core/game/UnitImpl.ts (1)
71-80: Include Factory in build/capture stats for consistencyAdding
UnitType.Factoryto both the build and capture/lose stats switches lines up its behavior with the existing destroy stats case. This keeps Factory buildings consistent with other structures like City/Port/DefensePost. Looks good as-is.Also applies to: 188-201
src/client/graphics/PlayerIcons.ts (1)
164-173: Prevent alliance progress icon from shrinking in flex layoutsSetting
flexShrink = "0"on the wrapper is a good way to keep the alliance progress icon from being squashed inside flex containers and matches the behavior used elsewhere. No issues.src/client/JoinPrivateLobbyModal.ts (1)
7-7: Align JoinPrivateLobbyModal with new Api.getApiBase helperSwitching
getApiBaseto import from./Apimatches the new API utilities module without changing runtime behavior incheckArchivedGame. Looks fine.src/client/TokenLoginModal.ts (1)
3-3: Use new Auth.tempTokenLogin helper correctlyThe swap to
tempTokenLoginfrom./Authand assigning itsstring | nullresult tothis.emailfits the new helper’s contract. The retry/close logic remains intact and should behave the same as before for success/failure.Also applies to: 82-86
src/client/StatsModal.ts (1)
7-7: Use Api.getApiBase for stats leaderboard endpointImporting
getApiBasefrom./Apikeeps StatsModal on the new API utilities without altering the fetch logic. Change looks good.src/client/graphics/layers/NameLayer.ts (1)
429-466: Keep alliance progress wrapper from shrinking in flex layoutsAdding
allianceWrapper.style.flexShrink = "0";when updating the existing alliance icon keeps its width stable inside the flex container and aligns with the behavior increateAllianceProgressIcon. No issues.src/client/ClientGameRunner.ts (1)
29-29: Import source updated correctly.The import path change from
./Mainto./Authaligns with the Auth module refactoring in this PR.src/core/GameRunner.ts (1)
49-58: Good asymmetric username handling.The logic correctly handles the local player differently from other players:
- Local player: only
sanitize()- preserves their exact name- Other players:
censorNameWithClanTag()- censors profanity while preserving clan tagsThis prevents desync issues by keeping the local player's name unchanged while protecting others from seeing profane names.
src/core/Util.ts (1)
342-357: Clean refactoring with DRY helper.Good extraction of the
clanMatchhelper function. This allows bothgetClanTag(uppercase) andgetClanTagOriginalCase(original case) to share the same parsing logic without duplication.src/client/graphics/layers/WinModal.ts (3)
112-114: Tutorial shown to new players on loss - good UX.The condition
!this.isWin && getGamesPlayed() < 3shows the YouTube tutorial only when:
- Player lost (not a win)
- Player has played fewer than 3 games
This provides helpful onboarding for new players who are struggling.
126-146: Lazy loading iframe - good pattern.The iframe
srcis set conditionally based onisVisible, preventing the YouTube embed from loading until the modal is shown. This improves initial page load performance.
13-13: Verify getUserMe usage.
getUserMeis imported from../../Apibut I don't see it used in this file's render methods. It's used inloadPatternContent()at line 163, so the import is valid.resources/lang/en.json (1)
533-534: Translation keys added for new UI features.New keys
win_modal.youtube_tutorialandterritory_patterns.not_logged_inare properly added to support the new YouTube tutorial modal and login warning features.src/client/TerritoryPatternsModal.ts (2)
137-139: Conditional UI based on login state.Good pattern to show different UI elements based on authentication status. Logged-in users see the "My Skins" filter button, while logged-out users see a warning.
294-302: Login check logic.The
isLoggedIn()method checks for either Discord or email authentication. This handles both login methods correctly. The check foruserMeResponse === falseguards against unauthenticated state.src/core/validations/username.ts (2)
48-95: Well-documented function with clear logic.The
censorNameWithClanTagfunction handles all edge cases correctly:
- Sanitizes input first
- Checks clan tag and username separately for profanity
- Preserves non-profane clan tags while censoring profane usernames
- Removes profane clan tags entirely
The JSDoc examples are helpful for understanding the behavior. The logic prevents desync by preserving clan tags where possible.
74-76: Edge case: multiple clan tags.The
replacecall only removes the first match of[${clanTag}]. If a username somehow has multiple identical clan tags like[ABC][ABC]Name, only one gets removed.This is likely fine since the regex
/\[([a-zA-Z0-9]{2,5})\]/only matches the first occurrence anyway, so consistent behavior.src/client/Main.ts (3)
8-9: LGTM!Import changes correctly align with the new module split (Api.ts and Auth.ts).
272-279: LGTM!The async login check using
userAuth()is correctly implemented. The=== falsecomparison works well with theUserAuthunion type.
485-485: LGTM!Correctly awaits the new async
getPlayToken()function.src/client/AccountModal.ts (8)
8-9: LGTM!Imports correctly updated to use the new Api and Auth modules.
39-42: LGTM!Optional chaining correctly handles the case where user data may be incomplete.
61-74: LGTM!Good UX addition with a loading spinner while fetching user data. Translation function used correctly.
79-85: LGTM!Clean conditional rendering based on user authentication state.
120-131: LGTM!Clean composition using child components for stats and game list display.
133-142: LGTM!Game navigation correctly encodes the game ID and dispatches the appropriate event.
281-302: LGTM!Good loading state management. The immediate
requestUpdate()at line 301 ensures the loading spinner shows right away, while the handlers update state after the async operation completes.
315-331: LGTM!Clearer method naming and proper error handling for profile loading.
src/client/Api.ts (4)
1-8: LGTM!Clean imports with proper type-safe schema usage.
10-48: LGTM!Well-structured API call with proper authentication, URL encoding, Zod validation, and error handling. Using
falseas the error return type is a clean pattern for this use case.
92-96: LGTM!Domain extraction logic is correct for both production and localhost environments.
78-90: Build system correctly handles environment variable replacement—no action needed.The
process?.env?.API_DOMAINaccess is properly configured. The webpack DefinePlugin inwebpack.config.jsline 139 replacesprocess.env.API_DOMAINwith a JSON-stringified value at build time, and the npm scripts (dev:staging,dev:prod) set the value viacross-env. The optional chaining provides safe fallback behavior.src/client/Auth.ts (7)
1-12: LGTM!Clean imports and well-defined types. The
UserAuthunion type clearly expresses success/failure states.
14-16: LGTM!Simple Discord OAuth redirect implementation.
34-39: LGTM!Clean auth header generation with graceful fallback to empty string.
41-65: LGTM!Good use of
finallyblock to ensure local cleanup happens regardless of server response. Proper error logging included.
67-70: LGTM!Clean boolean wrapper around
userAuth().
169-174: LGTM!Clean fallback logic with appropriate security warning comment.
176-197: LGTM!Clean persistent ID management with appropriate fallback chain and security warnings.
| private renderAccountInfo() { | ||
| return html` | ||
| <div class="p-6"> | ||
| <div class="mb-4 text-center"> | ||
| <p class="text-white mb-4"> | ||
| Logged in with Discord as ${this.loggedInDiscord} | ||
| <div class="mb-4"> | ||
| <p class="text-white mb-4 text-center"> | ||
| Player ID: ${this.userMeResponse?.player?.publicId} | ||
| </p> | ||
| ${this.logoutButton()} | ||
| </div> | ||
| <div class="mb-4 text-center"> | ||
| <p class="text-white mb-4">${this.renderLoggedInAs()}</p> | ||
| </div> | ||
| <div class="flex flex-col items-center mt-2 mb-4"> | ||
| <discord-user-header | ||
| .data=${this.userMeResponse?.user?.discord ?? null} | ||
| ></discord-user-header> | ||
| <player-stats-tree-view | ||
| .statsTree=${this.statsTree} | ||
| ></player-stats-tree-view> | ||
| <hr class="w-2/3 border-gray-600 my-2" /> | ||
| <game-list | ||
| .games=${this.recentGames} | ||
| .onViewGame=${(id: string) => this.viewGame(id)} | ||
| ></game-list> | ||
| </div> | ||
| ${this.renderPlayerStats()} | ||
| </div> | ||
| `; | ||
| } |
There was a problem hiding this comment.
Missing translation for "Player ID:" text.
The string "Player ID:" on line 92 is hardcoded. Other UI text in this file uses translateText(). Consider adding this to the translation system for consistency.
- <p class="text-white mb-4 text-center">
- Player ID: ${this.userMeResponse?.player?.publicId}
+ <p class="text-white mb-4 text-center">
+ ${translateText("account_modal.player_id")}: ${this.userMeResponse?.player?.publicId}Add to en.json:
"player_id": "Player ID"🤖 Prompt for AI Agents
In src/client/AccountModal.ts around lines 87 to 106, the hardcoded label
"Player ID:" should be replaced with a call to the app's translation helper
(e.g., translateText('player_id')) so it uses the localization system; update
the template to render the translated string and ensure you add the key
"player_id": "Player ID" to en.json (or the appropriate locale files) so the
translation exists.
| private renderLoggedInAs(): TemplateResult { | ||
| const me = this.userMeResponse?.user; | ||
| if (me?.discord) { | ||
| return html`<p>Logged in as ${me.discord.global_name}</p> | ||
| ${this.renderLogoutButton()}`; | ||
| } else if (me?.email) { | ||
| return html`<p>Logged in as ${me.email}</p> | ||
| ${this.renderLogoutButton()}`; | ||
| } | ||
| return this.renderLoginOptions(); | ||
| } |
There was a problem hiding this comment.
Hardcoded strings should use translation.
"Logged in as" text on lines 111 and 114 is not translated. Consider using translateText() for i18n consistency.
- if (me?.discord) {
- return html`<p>Logged in as ${me.discord.global_name}</p>
+ if (me?.discord) {
+ return html`<p>${translateText("account_modal.logged_in_as", { name: me.discord.global_name })}</p>
${this.renderLogoutButton()}`;
- } else if (me?.email) {
- return html`<p>Logged in as ${me.email}</p>
+ } else if (me?.email) {
+ return html`<p>${translateText("account_modal.logged_in_as", { name: me.email })}</p>
${this.renderLogoutButton()}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private renderLoggedInAs(): TemplateResult { | |
| const me = this.userMeResponse?.user; | |
| if (me?.discord) { | |
| return html`<p>Logged in as ${me.discord.global_name}</p> | |
| ${this.renderLogoutButton()}`; | |
| } else if (me?.email) { | |
| return html`<p>Logged in as ${me.email}</p> | |
| ${this.renderLogoutButton()}`; | |
| } | |
| return this.renderLoginOptions(); | |
| } | |
| private renderLoggedInAs(): TemplateResult { | |
| const me = this.userMeResponse?.user; | |
| if (me?.discord) { | |
| return html`<p>${translateText("account_modal.logged_in_as", { name: me.discord.global_name })}</p> | |
| ${this.renderLogoutButton()}`; | |
| } else if (me?.email) { | |
| return html`<p>${translateText("account_modal.logged_in_as", { name: me.email })}</p> | |
| ${this.renderLogoutButton()}`; | |
| } | |
| return this.renderLoginOptions(); | |
| } |
🤖 Prompt for AI Agents
In src/client/AccountModal.ts around lines 108 to 118, the "Logged in as"
strings are hardcoded; replace them with the translation helper by wrapping the
phrase with translateText (e.g. translateText('logged_in_as')) and interpolate
the username into the translated string or concatenate as appropriate, keeping
the same branches for discord and email and still rendering the logout button;
also ensure translateText is imported at the top of the file if not already.
| <button | ||
| @click="${this.handleLogout}" | ||
| class="px-3 py-1 text-xs font-medium text-white bg-red-600 border border-transparent rounded-md hover:bg-red-700 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-red-500 transition-colors duration-200" | ||
| > | ||
| Clear | ||
| </button> | ||
| `; |
There was a problem hiding this comment.
"Clear" button text should use translation.
The "Clear" button text on line 226 is hardcoded. Consider using translateText() for consistency.
- Clear
+ ${translateText("account_modal.clear") || "Clear"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| @click="${this.handleLogout}" | |
| class="px-3 py-1 text-xs font-medium text-white bg-red-600 border border-transparent rounded-md hover:bg-red-700 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-red-500 transition-colors duration-200" | |
| > | |
| Clear | |
| </button> | |
| `; | |
| <button | |
| @click="${this.handleLogout}" | |
| class="px-3 py-1 text-xs font-medium text-white bg-red-600 border border-transparent rounded-md hover:bg-red-700 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-red-500 transition-colors duration-200" | |
| > | |
| ${translateText("account_modal.clear") || "Clear"} | |
| </button> | |
| `; |
🤖 Prompt for AI Agents
In src/client/AccountModal.ts around lines 222 to 228, the button label "Clear"
is hardcoded; replace the literal text with a call to translateText (e.g. use
${translateText('clear')} or the project's standard key) so the label is
localized, and ensure translateText is imported or referenced from the component
context if not already available.
| const now = Math.floor(Date.now() / 1000); | ||
| if ( | ||
| // About to expire in less than 5 minutes | ||
| (exp !== undefined && now >= exp + 5 * 60) || | ||
| // Created more than 30 minutes ago (to refresh deprecated long lived tokens) | ||
| (iat !== undefined && now >= iat + 1 * 10) | ||
| ) { | ||
| console.log("jwt expired or about to expire"); | ||
| if (!shouldRefresh) { | ||
| console.error("jwt expired and shouldRefresh is false"); | ||
| return false; | ||
| } | ||
| await refreshJwt(); | ||
|
|
||
| // Try to get login info agian after refreshing | ||
| return userAuth(false); | ||
| } |
There was a problem hiding this comment.
Expiration check logic appears inverted and has incorrect constants.
There are issues with the expiration checks:
-
Line 115: The comment says "About to expire in less than 5 minutes" but
now >= exp + 5 * 60checks if the token expired more than 5 minutes ago. Should benow >= exp - 5 * 60to check if expiring within 5 minutes. -
Line 117: The comment says "30 minutes" but
1 * 10equals 10 seconds, not 30 minutes (30 * 60 = 1800).
const now = Math.floor(Date.now() / 1000);
if (
// About to expire in less than 5 minutes
- (exp !== undefined && now >= exp + 5 * 60) ||
+ (exp !== undefined && now >= exp - 5 * 60) ||
// Created more than 30 minutes ago (to refresh deprecated long lived tokens)
- (iat !== undefined && now >= iat + 1 * 10)
+ (iat !== undefined && now >= iat + 30 * 60)
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const now = Math.floor(Date.now() / 1000); | |
| if ( | |
| // About to expire in less than 5 minutes | |
| (exp !== undefined && now >= exp + 5 * 60) || | |
| // Created more than 30 minutes ago (to refresh deprecated long lived tokens) | |
| (iat !== undefined && now >= iat + 1 * 10) | |
| ) { | |
| console.log("jwt expired or about to expire"); | |
| if (!shouldRefresh) { | |
| console.error("jwt expired and shouldRefresh is false"); | |
| return false; | |
| } | |
| await refreshJwt(); | |
| // Try to get login info agian after refreshing | |
| return userAuth(false); | |
| } | |
| const now = Math.floor(Date.now() / 1000); | |
| if ( | |
| // About to expire in less than 5 minutes | |
| (exp !== undefined && now >= exp - 5 * 60) || | |
| // Created more than 30 minutes ago (to refresh deprecated long lived tokens) | |
| (iat !== undefined && now >= iat + 30 * 60) | |
| ) { | |
| console.log("jwt expired or about to expire"); | |
| if (!shouldRefresh) { | |
| console.error("jwt expired and shouldRefresh is false"); | |
| return false; | |
| } | |
| await refreshJwt(); | |
| // Try to get login info agian after refreshing | |
| return userAuth(false); | |
| } |
🤖 Prompt for AI Agents
In src/client/Auth.ts around lines 112 to 128, the expiration checks are
inverted and use wrong constants: change the "about to expire in less than 5
minutes" test from now >= exp + 5 * 60 to now >= exp - 5 * 60, and change the
"created more than 30 minutes ago" test from now >= iat + 1 * 10 to now >= iat +
30 * 60; keep the undefined guards for exp/iat as-is and leave the refresh flow
intact.
| async function refreshJwt(): Promise<void> { | ||
| try { | ||
| console.log("Refreshing jwt"); | ||
| const response = await fetch(getApiBase() + "/refresh", { | ||
| method: "POST", | ||
| credentials: "include", | ||
| }); | ||
| if (response.status !== 200) { | ||
| console.error("Refresh failed", response); | ||
| logOut(); | ||
| return; | ||
| } | ||
| const json = await response.json(); | ||
| const { jwt } = json; | ||
| console.log("Refresh succeeded", jwt); | ||
| __jwt = jwt; | ||
| } catch (e) { | ||
| console.error("Refresh failed", e); | ||
| logOut(); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove JWT from log output.
Line 160 logs the JWT token to the console. This is a security risk as tokens visible in the console could be captured by malicious browser extensions or by someone with physical access.
const { jwt } = json;
- console.log("Refresh succeeded", jwt);
+ console.log("Refresh succeeded");
__jwt = jwt;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function refreshJwt(): Promise<void> { | |
| try { | |
| console.log("Refreshing jwt"); | |
| const response = await fetch(getApiBase() + "/refresh", { | |
| method: "POST", | |
| credentials: "include", | |
| }); | |
| if (response.status !== 200) { | |
| console.error("Refresh failed", response); | |
| logOut(); | |
| return; | |
| } | |
| const json = await response.json(); | |
| const { jwt } = json; | |
| console.log("Refresh succeeded", jwt); | |
| __jwt = jwt; | |
| } catch (e) { | |
| console.error("Refresh failed", e); | |
| logOut(); | |
| return; | |
| } | |
| } | |
| async function refreshJwt(): Promise<void> { | |
| try { | |
| console.log("Refreshing jwt"); | |
| const response = await fetch(getApiBase() + "/refresh", { | |
| method: "POST", | |
| credentials: "include", | |
| }); | |
| if (response.status !== 200) { | |
| console.error("Refresh failed", response); | |
| logOut(); | |
| return; | |
| } | |
| const json = await response.json(); | |
| const { jwt } = json; | |
| console.log("Refresh succeeded"); | |
| __jwt = jwt; | |
| } catch (e) { | |
| console.error("Refresh failed", e); | |
| logOut(); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/client/Auth.ts around lines 146 to 167, the code currently logs the full
JWT on successful refresh (line ~160) which exposes a sensitive token; remove
the JWT from any console or logger output and instead log a non-sensitive
success message (e.g., "Refresh succeeded") or nothing at all, or log a masked
indicator if you must show partial info (never full token); update the success
path to assign __jwt = jwt without printing jwt, and ensure no other log
statements in this block output the token or response body.
src/client/LocalServer.ts
Outdated
|
|
||
| console.log("!!! persistentID", getPersistentID()); |
There was a problem hiding this comment.
Debug log should be removed.
The log statement console.log("!!! persistentID", getPersistentID()) looks like leftover debug code. The !!! prefix suggests temporary debugging. Consider removing it before merging.
-
- console.log("!!! persistentID", getPersistentID());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("!!! persistentID", getPersistentID()); |
🤖 Prompt for AI Agents
In src/client/LocalServer.ts around lines 186 to 187, remove the leftover debug
log statement console.log("!!! persistentID", getPersistentID()); — either
delete this line entirely or replace it with a proper logger.debug call (without
the "!!!" prefix) if the persistent ID must be logged in production-quality
code.
| import { getPlayToken } from "./Auth"; | ||
| import "./components/Difficulties"; | ||
| import "./components/PatternButton"; | ||
| import { getPlayToken, JoinLobbyEvent } from "./Main"; | ||
| import { JoinLobbyEvent } from "./Main"; | ||
| import { translateText } from "./Utils"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -e ts -e tsx | head -20Repository: openfrontio/OpenFrontIO
Length of output: 600
🏁 Script executed:
find src/client -name "Auth.ts" -o -name "Matchmaking.ts"Repository: openfrontio/OpenFrontIO
Length of output: 111
🏁 Script executed:
cat -n src/client/Auth.tsRepository: openfrontio/OpenFrontIO
Length of output: 6906
🏁 Script executed:
sed -n '53,71p' src/client/Matchmaking.tsRepository: openfrontio/OpenFrontIO
Length of output: 699
🏁 Script executed:
wc -l src/client/Matchmaking.tsRepository: openfrontio/OpenFrontIO
Length of output: 96
🏁 Script executed:
cat -n src/client/Matchmaking.ts | head -80Repository: openfrontio/OpenFrontIO
Length of output: 2997
Await getPlayToken() in socket.onopen handler
getPlayToken() in Auth.ts is an async function returning Promise<string>, but at line 68 it's called without await:
this.socket.onopen = async () => {
// ...
this.socket?.send(
JSON.stringify({
type: "auth",
playToken: getPlayToken(), // ← passes Promise, not string
}),
);
};This sends "playToken":"[object Promise]" to the server instead of an actual token. Fix by awaiting the result:
this.socket.onopen = async () => {
console.log("Connected to matchmaking server");
setTimeout(() => {
this.connected = true;
this.requestUpdate();
}, 1000);
const playToken = await getPlayToken();
this.socket?.send(
JSON.stringify({
type: "auth",
playToken,
}),
);
};🤖 Prompt for AI Agents
In src/client/Matchmaking.ts around lines 5 to 9 and specifically in the
socket.onopen handler (near line 68), the code passes the Promise returned by
getPlayToken() to the server instead of the resolved string; update the onopen
handler to await getPlayToken() into a local const (e.g., const playToken =
await getPlayToken()) and then send the JSON with playToken (not the Promise),
preserving the existing connected flag and requestUpdate flow.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/client/Matchmaking.ts (1)
65-70: MissingawaitongetPlayToken()call.
getPlayToken()inAuth.tsis async and returnsPromise<string>. Calling it withoutawaitsends the Promise object (serialized as"[object Promise]") to the server instead of the actual token.Apply this fix:
- this.socket?.send( - JSON.stringify({ - type: "auth", - playToken: getPlayToken(), - }), - ); + const playToken = await getPlayToken(); + this.socket?.send( + JSON.stringify({ + type: "auth", + playToken, + }), + );src/client/AccountModal.ts (3)
91-93: Hardcoded "Player ID:" text.This string should use
translateText()for i18n consistency. Add a translation key likeaccount_modal.player_id.- Player ID: ${this.userMeResponse?.player?.publicId} + ${translateText("account_modal.player_id")}: ${this.userMeResponse?.player?.publicId}
108-118: Hardcoded "Logged in as" text.These strings should use
translateText()for i18n consistency.if (me?.discord) { - return html`<p>Logged in as ${me.discord.global_name}</p> + return html`<p>${translateText("account_modal.logged_in_as", { name: me.discord.global_name })}</p> ${this.renderLogoutButton()}`; } else if (me?.email) { - return html`<p>Logged in as ${me.email}</p> + return html`<p>${translateText("account_modal.logged_in_as", { name: me.email })}</p> ${this.renderLogoutButton()}`; }
222-228: Hardcoded "Clear" button text.This string should use
translateText()for i18n consistency.- Clear + ${translateText("account_modal.clear")}
🧹 Nitpick comments (4)
src/client/Main.ts (1)
599-601: Consider adding error handling for async initialization.Since
initialize()is now async, unhandled rejections could occur if it throws. Adding a.catch()improves robustness.document.addEventListener("DOMContentLoaded", () => { - new Client().initialize(); + new Client().initialize().catch((err) => { + console.error("Failed to initialize client:", err); + }); });src/client/Api.ts (3)
1-8: Tighten imports by using type-only for models
PlayerProfileandUserMeResponseare only used as types here. You can import them asimport typeto make intent clear and keep the runtime bundle a bit cleaner.-import { z } from "zod"; -import { - PlayerProfile, - PlayerProfileSchema, - UserMeResponse, - UserMeResponseSchema, -} from "../core/ApiSchemas"; +import { z } from "zod"; +import { + PlayerProfileSchema, + UserMeResponseSchema, +} from "../core/ApiSchemas"; +import type { PlayerProfile, UserMeResponse } from "../core/ApiSchemas";
49-77: Improve error visibility in getUserMeThe logic is sound, but most failures just return
falsewith no log, which makes debugging harder (non‑200 responses and exceptions are silent).Consider adding minimal logging for unexpected statuses and the catch block:
- if (response.status === 401) { - await logOut(); - return false; - } - if (response.status !== 200) return false; + if (response.status === 401) { + await logOut(); + return false; + } + if (response.status !== 200) { + console.warn( + "getUserMe: unexpected status", + response.status, + response.statusText, + ); + return false; + } @@ - } catch (e) { - return false; - } + } catch (e) { + console.error("getUserMe: request failed", e); + return false; + }
92-95: Consider edge cases for getAudience domain parsing
hostname.split(".").slice(-2).join(".")works for typicalfoo.example.com→example.comand forlocalhost, but it will collapseapp.example.co.uktoco.uk. If you expect such multi-part TLDs or custom domains, you may want a small helper or config-driven audience instead of hardcoding “last two labels”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
resources/lang/en.json(1 hunks)src/client/AccountModal.ts(7 hunks)src/client/Api.ts(1 hunks)src/client/Auth.ts(1 hunks)src/client/ClientGameRunner.ts(2 hunks)src/client/Cosmetics.ts(2 hunks)src/client/JoinPrivateLobbyModal.ts(1 hunks)src/client/LocalServer.ts(2 hunks)src/client/Main.ts(5 hunks)src/client/Matchmaking.ts(2 hunks)src/client/StatsModal.ts(1 hunks)src/client/TerritoryPatternsModal.ts(3 hunks)src/client/TokenLoginModal.ts(2 hunks)src/client/graphics/layers/WinModal.ts(1 hunks)src/client/jwt.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/client/jwt.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/client/ClientGameRunner.ts
- src/client/TokenLoginModal.ts
- resources/lang/en.json
- src/client/Auth.ts
- src/client/StatsModal.ts
- src/client/Cosmetics.ts
- src/client/LocalServer.ts
- src/client/JoinPrivateLobbyModal.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/client/TerritoryPatternsModal.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/TerritoryPatternsModal.tssrc/client/Matchmaking.tssrc/client/Main.tssrc/client/AccountModal.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/Matchmaking.ts
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/client/AccountModal.ts
🧬 Code graph analysis (4)
src/client/TerritoryPatternsModal.ts (1)
src/client/LangSelector.ts (1)
translateText(258-278)
src/client/Main.ts (1)
src/client/Auth.ts (2)
userAuth(72-144)getPlayToken(170-174)
src/client/AccountModal.ts (3)
src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (1)
translateText(92-147)src/client/Api.ts (1)
fetchPlayerById(10-48)
src/client/Api.ts (2)
src/client/Auth.ts (2)
userAuth(72-144)logOut(41-65)src/core/ApiSchemas.ts (3)
PlayerProfileSchema(86-91)UserMeResponse(56-56)UserMeResponseSchema(45-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (11)
src/client/TerritoryPatternsModal.ts (3)
137-139: Clean conditional rendering based on login state.This approach of rendering different UI elements based on login state is clear and easy to follow.
159-179: Good separation of rendering logic into dedicated helpers.Both methods are focused and use
translateText()correctly for i18n. The warning label styling clearly conveys the "not logged in" state.
294-302: Local login check is appropriate for UI rendering.This synchronous check on already-fetched
userMeResponsedata works well for determining what to render. It correctly identifies a logged-in user by checking for either Discord or email identity.src/client/graphics/layers/WinModal.ts (1)
13-13: Import path updated to new Api module.This aligns with the refactor that centralizes API utilities in
src/client/Api.ts.src/client/AccountModal.ts (2)
61-74: Good loading state with spinner.The loading indicator improves UX by showing users that data is being fetched. The translation key
account_modal.fetching_accountis properly used.
315-331: Clean async data loading with error handling.The
loadPlayerProfilemethod properly handles errors and updates the UI state. UsingfetchPlayerByIdfrom the new Api module keeps the logic centralized.src/client/Main.ts (4)
8-9: Imports updated for new Auth and Api modules.The transition to centralized authentication (
Auth.ts) and API utilities (Api.ts) is clean. This removes legacy token handling from Main.ts.
107-107:initialize()is now async.This change enables proper async/await patterns for authentication checks. The async signature aligns with the new
userAuth()andgetPlayToken()usage.
272-279: Async authentication check with proper flow.Using
await userAuth()to check login state before fetching user details is correct. The flow properly handles both logged-in and logged-out states.
485-485: Correctly awaitsgetPlayToken()when joining lobby.Unlike the issue in
Matchmaking.ts, this call properly awaits the async function, ensuring the actual token string is used.src/client/Api.ts (1)
10-48: fetchPlayerById implementation looks goodFlow is clear: auth first, then fetch with Bearer token, strict 200 check, then zod validation with good logging on failures. The return type
PlayerProfile | falsematches that contract cleanly. I don’t see issues here.
| export function getApiBase() { | ||
| const domainname = getAudience(); | ||
|
|
||
| if (domainname === "localhost") { | ||
| const apiDomain = process?.env?.API_DOMAIN; | ||
| if (apiDomain) { | ||
| return `https://${apiDomain}`; | ||
| } | ||
| return localStorage.getItem("apiHost") ?? "http://localhost:8787"; | ||
| } | ||
|
|
||
| return `https://api.${domainname}`; | ||
| } |
There was a problem hiding this comment.
Guard browser use of process in getApiBase
process?.env?.API_DOMAIN assumes a process global exists. In many browser builds (no Node polyfill), touching process like this can still throw a ReferenceError when getApiBase() runs. Adding a typeof process !== "undefined" guard (or using your bundler’s env helper) avoids a hard crash while keeping the override behavior.
-export function getApiBase() {
+export function getApiBase(): string {
const domainname = getAudience();
if (domainname === "localhost") {
- const apiDomain = process?.env?.API_DOMAIN;
- if (apiDomain) {
- return `https://${apiDomain}`;
- }
+ let apiDomain: string | null | undefined = null;
+
+ if (typeof process !== "undefined" && process.env?.API_DOMAIN) {
+ apiDomain = process.env.API_DOMAIN;
+ }
+
+ if (apiDomain) {
+ return `https://${apiDomain}`;
+ }
return localStorage.getItem("apiHost") ?? "http://localhost:8787";
}
return `https://api.${domainname}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function getApiBase() { | |
| const domainname = getAudience(); | |
| if (domainname === "localhost") { | |
| const apiDomain = process?.env?.API_DOMAIN; | |
| if (apiDomain) { | |
| return `https://${apiDomain}`; | |
| } | |
| return localStorage.getItem("apiHost") ?? "http://localhost:8787"; | |
| } | |
| return `https://api.${domainname}`; | |
| } | |
| export function getApiBase(): string { | |
| const domainname = getAudience(); | |
| if (domainname === "localhost") { | |
| let apiDomain: string | null | undefined = null; | |
| if (typeof process !== "undefined" && process.env?.API_DOMAIN) { | |
| apiDomain = process.env.API_DOMAIN; | |
| } | |
| if (apiDomain) { | |
| return `https://${apiDomain}`; | |
| } | |
| return localStorage.getItem("apiHost") ?? "http://localhost:8787"; | |
| } | |
| return `https://api.${domainname}`; | |
| } |
🤖 Prompt for AI Agents
In src/client/Api.ts around lines 78 to 90, the code accesses
process?.env?.API_DOMAIN which can throw a ReferenceError in browser builds
where process is undefined; change the check to first verify typeof process !==
"undefined" (or use the bundler env helper) before reading process.env, e.g.
only read API_DOMAIN when process exists and then fall back to localStorage or
the localhost default; keep the existing behavior but wrap the process access
with a typeof guard to prevent runtime crashes in the browser.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/AccountModal.ts (2)
35-48: Logged‑out event leaves stale userMeResponse and may not re‑renderWhen the
"userMeResponse"event has a falsydetail, you clearstatsTree/recentGamesbut never resetthis.userMeResponse, and you only callrequestUpdate()in theelsebranch. This can leave stale user info rendered after logout and means updates on a validdetailmay not trigger a re‑render.Consider resetting
userMeResponseand always callingrequestUpdate():document.addEventListener("userMeResponse", (event: Event) => { const customEvent = event as CustomEvent; if (customEvent.detail) { - this.userMeResponse = customEvent.detail as UserMeResponse; - if (this.userMeResponse?.player?.publicId === undefined) { - this.statsTree = null; - this.recentGames = []; - } - } else { - this.statsTree = null; - this.recentGames = []; - this.requestUpdate(); - } + this.userMeResponse = customEvent.detail as UserMeResponse; + if (this.userMeResponse?.player?.publicId === undefined) { + this.statsTree = null; + this.recentGames = []; + } + this.requestUpdate(); + } else { + this.userMeResponse = null; + this.statsTree = null; + this.recentGames = []; + this.requestUpdate(); + } });
258-279: HandlegetUserMe()returning “not logged in” to avoid stale user infoIn
open(), ifgetUserMe()returns a falsy value (likelyfalsein this codebase), you leavethis.userMeResponseand stats as‑is. That means if a user logs out server‑side, opening the account modal later can still show the old user’s info.Suggested change:
public open() { @@ - void getUserMe() - .then((userMe) => { - if (userMe) { - this.userMeResponse = userMe; - if (this.userMeResponse?.player?.publicId) { - this.loadPlayerProfile(this.userMeResponse.player.publicId); - } - } + void getUserMe() + .then((userMe) => { + if (userMe) { + this.userMeResponse = userMe; + if (this.userMeResponse?.player?.publicId) { + this.loadPlayerProfile(this.userMeResponse.player.publicId); + } + } else { + this.userMeResponse = null; + this.statsTree = null; + this.recentGames = []; + } this.isLoadingUser = false; this.requestUpdate(); })This keeps the modal view in sync with the real auth state.
♻️ Duplicate comments (5)
src/client/Api.ts (1)
115-127: Guard againstprocessReferenceError in browser.Accessing
process?.env?.API_DOMAINcan throw aReferenceErrorin browser builds whereprocessis undefined. Add atypeofguard before accessingprocess.export function getApiBase() { const domainname = getAudience(); if (domainname === "localhost") { - const apiDomain = process?.env?.API_DOMAIN; + const apiDomain = typeof process !== "undefined" ? process?.env?.API_DOMAIN : undefined; if (apiDomain) { return `https://${apiDomain}`; } return localStorage.getItem("apiHost") ?? "http://localhost:8787"; } return `https://api.${domainname}`; }src/client/AccountModal.ts (2)
222-227: Localize “Clear” button labelThe “Clear” button text is hardcoded; other labels in this modal use
translateText(). Please route this through the translation helper.For example:
<button @click="${this.handleLogout}" @@ > - Clear + ${translateText("account_modal.clear") || "Clear"} </button>And add
account_modal.cleartoen.json.
87-118: Fix nested<p>markup and localize Player ID / “Logged in as”Two issues in
renderAccountInfo/renderLoggedInAs:
renderAccountInfowraps${this.renderLoggedInAs()}inside a<p>, butrenderLoggedInAsitself returns<p>...</p>plus a button (or even the full login form). This produces invalid nested<p>and can break layout."Player ID:"and"Logged in as"are hardcoded; other strings in this file usetranslateText().You can fix both layout and i18n like this:
private renderAccountInfo() { return html` <div class="p-6"> - <div class="mb-4"> - <p class="text-white mb-4 text-center"> - Player ID: ${this.userMeResponse?.player?.publicId} - </p> - </div> - <div class="mb-4 text-center"> - <p class="text-white mb-4">${this.renderLoggedInAs()}</p> - </div> + <div class="mb-4"> + <p class="text-white mb-4 text-center"> + ${translateText("account_modal.player_id") || "Player ID"}: + ${this.userMeResponse?.player?.publicId} + </p> + </div> + <div class="mb-4 text-center"> + ${this.renderLoggedInAs()} + </div> @@ - private renderLoggedInAs(): TemplateResult { - const me = this.userMeResponse?.user; - if (me?.discord) { - return html`<p>Logged in as ${me.discord.global_name}</p> - ${this.renderLogoutButton()}`; - } else if (me?.email) { - return html`<p>Logged in as ${me.email}</p> - ${this.renderLogoutButton()}`; - } - return this.renderLoginOptions(); - } + private renderLoggedInAs(): TemplateResult { + const me = this.userMeResponse?.user; + + if (me?.discord) { + return html` + <p class="text-white mb-4"> + ${translateText("account_modal.logged_in_as", { + email: me.discord.global_name, + })} + </p> + ${this.renderLogoutButton()} + `; + } + + if (me?.email) { + return html` + <p class="text-white mb-4"> + ${translateText("account_modal.logged_in_as", { + email: me.email, + })} + </p> + ${this.renderLogoutButton()} + `; + } + + // Should not normally happen when renderAccountInfo() is used + return html``; + }Remember to add
account_modal.player_idandaccount_modal.logged_in_askeys inen.jsonif they are not there yet.src/client/Auth.ts (2)
96-118: JWT expiration checks don’t match comments (refresh may be too late / too often)The expiration logic seems inverted / mis‑scaled:
- Comment says “About to expire in less than 5 minutes” but you check
now >= exp + 5 * 60, which means “expired at least 5 minutes ago”.- Comment says “Created more than 30 minutes ago” but
iat + 1 * 10is 10 seconds, not 30 minutes.That can cause you to accept long‑expired tokens and to refresh far earlier than intended.
Suggested fix:
const now = Math.floor(Date.now() / 1000); if ( // About to expire in less than 5 minutes - (exp !== undefined && now >= exp + 5 * 60) || + (exp !== undefined && now >= exp - 5 * 60) || // Created more than 30 minutes ago (to refresh deprecated long lived tokens) - (iat !== undefined && now >= iat + 1 * 10) + (iat !== undefined && now >= iat + 30 * 60) ) {This aligns behavior with the comments and usual JWT practice.
For JWT access tokens, is it standard to trigger a refresh when `now` is close to `exp` (e.g. `now >= exp - 300`) and to compare `iat` in seconds against durations like 30 minutes (`30 * 60`)?
146-167: Do not log full JWTs to the console
refreshJwtcurrently logs the full JWT on success:console.log("Refresh succeeded", jwt);JWTs are sensitive bearer tokens; logging them risks exposure via browser devtools, log aggregation, or extensions.
Please remove the token from logs:
const json = await response.json(); const { jwt } = json; - console.log("Refresh succeeded", jwt); + console.log("Refresh succeeded"); __jwt = jwt;Also worth double‑checking that no other logs in this module emit JWT contents.
Are JSON Web Tokens (JWTs) considered sensitive credentials that should not be logged in plaintext in client or server logs?
🧹 Nitpick comments (1)
src/client/ClientGameRunner.ts (1)
223-223: Consider removing unnecessaryasynckeyword.The
saveGamemethod signature was changed toasync, but there are noawaitstatements in the method body. If no async operations are planned, theasynckeyword can be removed for clarity.- private async saveGame(update: WinUpdate) { + private saveGame(update: WinUpdate) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
resources/lang/en.json(1 hunks)src/client/AccountModal.ts(7 hunks)src/client/Api.ts(1 hunks)src/client/Auth.ts(1 hunks)src/client/ClientGameRunner.ts(2 hunks)src/client/Cosmetics.ts(2 hunks)src/client/JoinPrivateLobbyModal.ts(1 hunks)src/client/LocalServer.ts(2 hunks)src/client/Main.ts(5 hunks)src/client/Matchmaking.ts(2 hunks)src/client/StatsModal.ts(1 hunks)src/client/TerritoryPatternsModal.ts(3 hunks)src/client/TokenLoginModal.ts(2 hunks)src/client/graphics/layers/WinModal.ts(1 hunks)src/client/jwt.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/client/jwt.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/client/graphics/layers/WinModal.ts
- resources/lang/en.json
- src/client/Matchmaking.ts
- src/client/StatsModal.ts
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/JoinPrivateLobbyModal.tssrc/client/AccountModal.tssrc/client/Main.tssrc/client/TerritoryPatternsModal.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/ClientGameRunner.tssrc/client/LocalServer.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/client/AccountModal.ts
📚 Learning: 2025-06-07T13:15:55.439Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
Applied to files:
src/client/Cosmetics.ts
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/client/TerritoryPatternsModal.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/LocalServer.ts
🧬 Code graph analysis (7)
src/client/AccountModal.ts (3)
src/client/Utils.ts (1)
translateText(92-147)src/client/Auth.ts (1)
sendMagicLink(169-198)src/client/Api.ts (1)
fetchPlayerById(10-48)
src/client/TokenLoginModal.ts (1)
src/client/Auth.ts (1)
tempTokenLogin(18-32)
src/client/Cosmetics.ts (1)
src/client/Api.ts (1)
createCheckoutSession(79-113)
src/client/Main.ts (1)
src/client/Auth.ts (2)
userAuth(72-144)getPlayToken(201-205)
src/client/TerritoryPatternsModal.ts (1)
src/client/LangSelector.ts (1)
translateText(258-278)
src/client/Auth.ts (5)
src/core/ApiSchemas.ts (2)
TokenPayload(34-34)TokenPayloadSchema(11-33)src/client/Api.ts (2)
getApiBase(115-127)getAudience(129-133)src/client/TerritoryPatternsModal.ts (1)
isLoggedIn(294-302)src/core/Base64.ts (1)
base64urlToUuid(24-37)src/client/Utils.ts (1)
generateCryptoRandomUUID(62-90)
src/client/Api.ts (3)
src/client/Auth.ts (3)
userAuth(72-144)logOut(41-65)getAuthHeader(34-39)src/core/game/FetchGameMapLoader.ts (1)
url(41-49)src/core/ApiSchemas.ts (2)
UserMeResponse(56-56)UserMeResponseSchema(45-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (22)
src/client/TokenLoginModal.ts (1)
3-3: LGTM: Import and function call updated consistently.The import path change from
./jwtto./Authand the function rename fromtokenLogintotempTokenLoginalign with the authentication refactoring. The function signature and behavior remain unchanged.Also applies to: 82-82
src/client/JoinPrivateLobbyModal.ts (1)
7-7: LGTM: Import path updated.The
getApiBaseimport moved from./jwtto./Api, consistent with the authentication module refactoring. No functional changes.src/client/ClientGameRunner.ts (1)
29-29: LGTM: Import consolidated to Auth module.The
getPersistentIDimport now comes from./Authinstead of./Main, centralizing authentication logic. This aligns with the broader auth refactoring.src/client/Cosmetics.ts (1)
8-8: LGTM: Checkout flow refactored with improved abstraction.The Stripe checkout session creation now uses the
createCheckoutSessionhelper fromApi.ts, replacing manual fetch construction. This simplifies the code and centralizes API logic. Error handling remains appropriate.Also applies to: 19-29
src/client/TerritoryPatternsModal.ts (1)
137-140: LGTM: Login-aware UI rendering added.The modal now conditionally renders a "My Skins" button when logged in or a warning label when not logged in. The
isLoggedIn()helper correctly checks for Discord or email authentication. This improves UX by providing clear feedback on authentication status.Also applies to: 159-179, 294-302
src/client/LocalServer.ts (2)
20-20: LGTM: Import consolidated to Auth module.The
getPersistentIDimport now comes from./Auth, centralizing authentication logic and aligning with the refactoring.
180-237: LGTM: Async archive flow properly implemented.The
endGamemethod signature correctly changed toasyncto support the asynchronous game archival via fetch. The compression and POST request logic is appropriate, and error handling includescatchfor fetch failures.src/client/Main.ts (3)
8-9: LGTM: Import surface updated and public API cleaned up.The imports now use
getUserMefrom./ApiandgetPlayToken/userAuthfrom./Auth. The public exports were reduced to onlyincrementGamesPlayedandisInIframe, properly moving authentication concerns to dedicated modules.Also applies to: 39-39
107-107: LGTM: Async initialization supports new auth flow.The
initializemethod now returnsPromise<void>to support asynchronous user authentication. The login check usingawait userAuth()followed by conditionalgetUserMe()call is correctly implemented.Also applies to: 272-279
485-485: LGTM: Token retrieval updated to async pattern.The token now uses
await getPlayToken(), correctly fetching the token asynchronously via the new Auth module.src/client/Api.ts (4)
10-48: LGTM: Player profile fetch properly implemented.The
fetchPlayerByIdfunction correctly authenticates viauserAuth, constructs the API URL, sends the request with Bearer token, and validates the response withPlayerProfileSchema. Error handling returnsfalseon failures.
49-77: LGTM: User data fetch with proper error handling.The
getUserMefunction correctly authenticates, fetches user data, handles 401 by logging out, and validates the response. Error handling is appropriate.
79-113: LGTM: Stripe checkout session creation centralized.The
createCheckoutSessionfunction properly handles the POST request with authentication headers and JSON body. Error handling logs failures and returnsfalseappropriately.
129-133: LGTM: Domain extraction logic is correct.The
getAudiencefunction properly extracts the domain name by taking the last two hostname components. This works for both localhost and production domains.src/client/AccountModal.ts (4)
8-9: New Api/Auth imports look goodUsing
fetchPlayerById,getUserMe, and the new Auth helpers keeps auth/data access centralized and removes direct JWT handling from this component. No issues here.
61-74: Loading state UX is clear and consistentThe
isLoadingUserbranch with a simple message plus spinner works well and is already wired throughopen(); this is a nice UX improvement and matches the rest of the UI patterns.
120-142: Stats and game view wiring looks solid
renderPlayerStatsandviewGameare nicely separated: you passstatsTree/recentGamesdown and usehistory.pushStateplus ahashchangedispatch, which matches other navigation patterns in this repo. No issues spotted here.
292-305: Player profile loading via Api.fetchPlayerById is fine
loadPlayerProfilecleanly wrapsfetchPlayerById(publicId)and guards on a falsy return, updatingrecentGamesandstatsTreeplusrequestUpdate()in both success and failure paths. This matches the centralized Api pattern and looks good.src/client/Auth.ts (4)
72-85: userAuth recursion / refresh flow is reasonableThe pattern of caching
__jwt, callingrefreshJwt()when missing, and then recursing withshouldRefresh = falseis simple and avoids infinite loops because the second call bails out if__jwtis still empty. This is okay for this codebase.
200-228: Persistent ID helpers are simple and follow composition over inheritance
getPlayTokenandgetPersistentIDuse a union (UserAuth | false) and small helpers (getPersistentIDFromLocalStorage,base64urlToUuid,generateCryptoRandomUUID) instead of any class hierarchy, which fits well with a functional/composition style. The “do not expose this ID” comments are also a good reminder of sensitivity.
130-135: No changes needed – z.prettifyError is a valid Zod v4 APIThe code correctly uses
z.prettifyError(), which is part of the official Zod API available in zod v4+. The project uses zod ^4.0.5, so this approach is valid and safe.Likely an incorrect or invalid review comment.
169-198: sendMagicLink implementation is correct and follows industry best practicesThe function properly implements a magic-link authentication request: POST with JSON body containing email and redirectDomain,
credentials: "include"to enable cookie-based sessions after server verification, and appropriate error handling. This pattern matches standard implementations (Supabase, Firebase, Clerk) and aligns with security best practices for magic-link flows—specifically avoiding URL leakage of sensitive data and supporting httpOnly cookie-based session establishment.
| const success = await sendMagicLink(this.email); | ||
| if (success) { | ||
| alert( | ||
| translateText("account_modal.recovery_email_sent", { | ||
| email: this.email, | ||
| }), | ||
| }); | ||
|
|
||
| if (response.ok) { | ||
| alert( | ||
| translateText("account_modal.recovery_email_sent", { | ||
| email: this.email, | ||
| }), | ||
| ); | ||
| this.close(); | ||
| } else { | ||
| console.error( | ||
| "Failed to send recovery email:", | ||
| response.status, | ||
| response.statusText, | ||
| ); | ||
| alert("Failed to send recovery email. Please try again."); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error sending recovery email:", error); | ||
| alert("Error sending recovery email. Please try again."); | ||
| ); | ||
| } else { | ||
| alert("Failed to send recovery email"); | ||
| } |
There was a problem hiding this comment.
Translate new error alert for recovery email failures
You correctly localized the success alert with translateText("account_modal.recovery_email_sent", { email }), but the failure case still uses a hardcoded English string.
Consider:
- if (success) {
- alert(
- translateText("account_modal.recovery_email_sent", {
- email: this.email,
- }),
- );
- } else {
- alert("Failed to send recovery email");
- }
+ if (success) {
+ alert(
+ translateText("account_modal.recovery_email_sent", {
+ email: this.email,
+ }),
+ );
+ } else {
+ alert(
+ translateText("account_modal.recovery_email_failed") ||
+ "Failed to send recovery email",
+ );
+ }Remember to add account_modal.recovery_email_failed to en.json.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const success = await sendMagicLink(this.email); | |
| if (success) { | |
| alert( | |
| translateText("account_modal.recovery_email_sent", { | |
| email: this.email, | |
| }), | |
| }); | |
| if (response.ok) { | |
| alert( | |
| translateText("account_modal.recovery_email_sent", { | |
| email: this.email, | |
| }), | |
| ); | |
| this.close(); | |
| } else { | |
| console.error( | |
| "Failed to send recovery email:", | |
| response.status, | |
| response.statusText, | |
| ); | |
| alert("Failed to send recovery email. Please try again."); | |
| } | |
| } catch (error) { | |
| console.error("Error sending recovery email:", error); | |
| alert("Error sending recovery email. Please try again."); | |
| ); | |
| } else { | |
| alert("Failed to send recovery email"); | |
| } | |
| const success = await sendMagicLink(this.email); | |
| if (success) { | |
| alert( | |
| translateText("account_modal.recovery_email_sent", { | |
| email: this.email, | |
| }), | |
| ); | |
| } else { | |
| alert( | |
| translateText("account_modal.recovery_email_failed") || | |
| "Failed to send recovery email", | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/client/AccountModal.ts around lines 242 to 251, the failure alert uses a
hardcoded English string; replace it with
translateText("account_modal.recovery_email_failed") (or include interpolation
if needed) so the error message is localized, and add the new key
"account_modal.recovery_email_failed" to en.json with an appropriate English
message.
| export function discordLogin() { | ||
| window.location.href = `${getApiBase()}/auth/login/discord?redirect_uri=${window.location.href}`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OAuth 2.0 RFC 6749 redirect_uri URL encoding requirements
💡 Result:
Short answer — RFC 6749 requirements (summary):
- redirect_uri must be an absolute URI per RFC 3986 and MUST NOT contain a fragment. [1]
- The authorization request parameters (including redirect_uri when used in the authorization request) are serialized into the authorization endpoint URI’s query using application/x-www-form-urlencoded encoding (percent-encoding for reserved/unsafe chars). Appendix B / §4.1.1. [1]
- The redirection endpoint URI MAY include a query component; that query component MUST be retained when adding OAuth response parameters. [1]
- If the client registered a full redirect URI, the authorization server MUST compare the received redirect_uri using simple string comparison (i.e., exact match) per RFC 3986 rules. If only parts were registered, matching rules in §3.1.2 apply. [1]
- The token request’s redirect_uri parameter (when present) must be the same value used in the authorization request (and is required at the token endpoint if it was present in the auth request). [1]
- Later OAuth documents (e.g., JAR / OAuth 2.1 drafts) show examples of redirect_uri being percent-encoded in the authorization request query (e.g., https%3A%2F%2Fclient.example.com%2Fcb). This is consistent with application/x-www-form-urlencoded encoding. [2][3]
Sources:
[1] RFC 6749 (OAuth 2.0), Sections 3.1.2 and 4.1.1/4.1.3.
[2] RFC 9101 (JAR) example of authorization request encoding.
[3] IETF published HTML/errata copy of RFC 6749.
🌐 Web query:
encodeURIComponent redirect_uri OAuth query parameter best practice
💡 Result:
Short answer: Yes — percent-encode the redirect_uri when you put it into the authorization endpoint URL (in browser JS use encodeURIComponent on the full redirect URI). But do it exactly once, prefer encoding each query/value when you build nested redirect URIs, and make sure the decoded value matches a registered redirect URI on the authorization server (RFC requirement). (datatracker.ietf.org)
Practical best-practices (concise)
- Construct the client redirect URI first (including any app-specific query params), percent-encode it when inserting into the authorization request: e.g. ?redirect_uri=encodeURIComponent(redirectUri).
- Encode only once. Never double-encode the same URI (don’t call encodeURIComponent twice or percent-encode an already-encoded string). (datatracker.ietf.org)
- If the redirect_uri itself contains query parameters, either: encode the whole redirect_uri with encodeURIComponent, or build it from encoded components (encodeURIComponent for each value) to avoid ambiguous characters. (oauth.com)
- The authorization server must match the decoded redirect_uri against registered redirect URIs (often using exact string comparison). Therefore register the exact redirect URI you will send (or register scheme+host+path and only vary the query if the server allows). (datatracker.ietf.org)
- Always include state (CSRF protection) and avoid embedding sensitive secrets in redirect_uri or its query. (datatracker.ietf.org)
Minimal JS example
- const url = authEndpoint + '?response_type=code&client_id=' + encodeURIComponent(clientId) + '&redirect_uri=' + encodeURIComponent(redirectUri) + '&state=' + encodeURIComponent(state);
References
- RFC 6749 (OAuth 2.0) — redirect_uri handling & matching rules. [RFC 6749]. (datatracker.ietf.org)
- OAuth.com — constructing the authorization request (examples and guidance). (oauth.com)
If you want, I can check a specific provider’s rules (Google, GitHub, Azure) for any provider-specific encoding/matching quirks.
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc6749.html?utm_source=openai
- 2: https://datatracker.ietf.org/doc/html/rfc6749.html?utm_source=openai
- 3: https://www.oauth.com/oauth2-servers/pkce/authorization-request/?utm_source=openai
- 4: https://datatracker.ietf.org/doc/html/rfc6749.html?utm_source=openai
- 5: https://datatracker.ietf.org/doc/html/rfc6749.html?utm_source=openai
- 6: https://datatracker.ietf.org/doc/html/rfc6749.html?utm_source=openai
- 7: https://www.oauth.com/oauth2-servers/pkce/authorization-request/?utm_source=openai
Encode redirect URI when building Discord login URL
discordLogin interpolates window.location.href directly into the redirect_uri query parameter. Per RFC 6749 §4.1.1, authorization request parameters must be percent-encoded when serialized into the query string. If the current URL contains its own query parameters or fragment, this breaks parsing on the server side.
Encode the redirect URI:
export function discordLogin() {
- window.location.href = `${getApiBase()}/auth/login/discord?redirect_uri=${window.location.href}`;
+ const redirectUri = encodeURIComponent(window.location.href);
+ window.location.href = `${getApiBase()}/auth/login/discord?redirect_uri=${redirectUri}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function discordLogin() { | |
| window.location.href = `${getApiBase()}/auth/login/discord?redirect_uri=${window.location.href}`; | |
| } | |
| export function discordLogin() { | |
| const redirectUri = encodeURIComponent(window.location.href); | |
| window.location.href = `${getApiBase()}/auth/login/discord?redirect_uri=${redirectUri}`; | |
| } |
🤖 Prompt for AI Agents
In src/client/Auth.ts around lines 14 to 16, the redirect_uri is interpolated
raw into the query string which can break parsing if the current URL has query
params or fragments; update the URL construction to percent-encode the redirect
URI using encodeURIComponent(window.location.href) when building the Discord
login URL so the redirect_uri query parameter is properly serialized (e.g.,
replace the direct interpolation with an encoded redirect_uri).
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME