add player profile public toggle #2558
Conversation
|
|
WalkthroughAdds a public-profile toggle to the account modal with optimistic UI and rollback, a POST /users/@me client method to update the flag, and a new optional boolean Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (click)
participant UI as AccountModal UI
participant Client as jwt.updateUserMe
participant Server as Backend (/users/@me)
User->>UI: Toggle public-profile
UI->>UI: set isPublicOptimistic = newValue (render)
UI->>Client: updateUserMe({ public: newValue })
Client->>Server: POST /users/@me { public: newValue }
alt Server responds 204
Server-->>Client: 204 No Content
Client-->>UI: true
UI->>Client: getUserMe / loadPublicStatus (GET)
Client->>Server: GET /users/@me
Server-->>Client: user { public: newValue, ... }
Client-->>UI: updated user
UI->>UI: clear optimistic flag (final render)
else Failure / non-204
Server-->>Client: error / non-204
Client-->>UI: false
UI->>UI: revert isPublicOptimistic, show alert "Failed to update public setting"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (2)
src/core/ApiSchemas.ts (1)
45-57: Tighten Zod schema souser.publicis always a boolean in the outputRight now you have:
public: z.boolean().optional().default(false),In Zod 4, doing
.optional().default(false)on a boolean makes the inner typeboolean | undefined, so the inferredUserMeResponse["user"]["public"]can still beboolean | undefined, even though you always want to treat it as a simple flag.If the goal is “missing in payload ⇒ parsed as
falseand always a plain boolean in code”, you can drop.optional()and rely on.default(false):- public: z.boolean().optional().default(false), + public: z.boolean().default(false),This keeps the input lenient but simplifies the runtime and TypeScript shape.
Please double‑check this against the Zod 4 docs or a quick local parse test to confirm the inferred type matches what you want.
src/client/AccountModal.ts (1)
33-33: Consider wiringuserMeResponse/playerIddirectly fromgetUserMe()inopen()You already keep
userMeResponseandplayerIdin this element (via the global"userMeResponse"event) and then, inopen(), you do:this.isLoadingUser = true; void getUserMe() .then(async (userMe) => { if (userMe) { this.loggedInEmail = userMe.user.email ?? null; this.loggedInDiscord = userMe.user.discord?.global_name ?? null; if (this.playerId) { await this.loadFromApi(this.playerId); await this.loadPublicStatus(); } } else { this.loggedInEmail = null; this.loggedInDiscord = null; } this.isLoadingUser = false; this.requestUpdate(); })and
loadPublicStatus()depends onthis.userMeResponse?.player?.publicId.This works as long as something else in the app always fires the
"userMeResponse"event before the modal is opened, but it’s a bit fragile. You can make this component self‑contained and simpler to reason about by deriving everything fromgetUserMe()right here:
- Set
this.userMeResponse = userMe;- Set
this.playerId = userMe.player.publicId;- Then call
loadFromApi(this.playerId)andloadPublicStatus().For example:
- void getUserMe() - .then(async (userMe) => { - if (userMe) { - this.loggedInEmail = userMe.user.email ?? null; - this.loggedInDiscord = userMe.user.discord?.global_name ?? null; - if (this.playerId) { - await this.loadFromApi(this.playerId); - await this.loadPublicStatus(); - } - } else { + void getUserMe() + .then(async (userMe) => { + if (userMe) { + this.userMeResponse = userMe; + this.playerId = userMe.player.publicId; + this.loggedInEmail = userMe.user.email ?? null; + this.loggedInDiscord = userMe.user.discord?.global_name ?? null; + await this.loadFromApi(this.playerId); + await this.loadPublicStatus(); + } else { this.loggedInEmail = null; this.loggedInDiscord = null; }This keeps the data flow local and reduces hidden dependencies on global events.
Also,
loadPublicStatus()itself looks good: it composes over the existing data model, uses a small helper instead of adding inheritance or extra classes, and the union‑like presence/absence ofdata.useris a simple way to represent public vs private.If you adopt this change, please test that the stats and public toggle still load correctly both on first open after login and on subsequent opens, especially in cases where the
"userMeResponse"event might not have fired yet.Also applies to: 332-359, 371-396
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.json(1 hunks)src/client/AccountModal.ts(6 hunks)src/client/jwt.ts(1 hunks)src/core/ApiSchemas.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/graphics/layers/ControlPanel.ts:160-183
Timestamp: 2025-06-07T20:01:52.720Z
Learning: The user its-sii prefers simpler, more readable code over complex type-safe approaches when the benefits don't justify the added complexity. They value maintainability and clarity in code implementation.
📚 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 `z.infer<typeof PlayerStatsSchema>` where PlayerStatsSchema has `.optional()` applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Applied to files:
src/core/ApiSchemas.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/core/ApiSchemas.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/en.json
📚 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/AccountModal.ts
🧬 Code graph analysis (1)
src/client/AccountModal.ts (1)
src/client/jwt.ts (3)
updateUserMe(261-288)getUserMe(231-259)getApiBase(20-32)
🔇 Additional comments (3)
src/client/jwt.ts (1)
261-288:updateUserMeimplementation is consistent with existing JWT helpersThe flow (token check → POST
/users/@mewith JSON body → 401 clears token → 204 treated as success) matches the rest of this module and looks correct for updating thepublicflag.If you want easier debugging later, you could optionally log non-204 responses before returning
false, but that’s not required for correctness.resources/lang/en.json (1)
159-166: Newaccount_modalstrings line up with the UI usage and translation workflow
- Keys
make_public_profileandfailed_update_public_settingare clearly named and match how they’re used inAccountModal.publicProfileToggle().- Only
en.jsonis updated, which matches the project rule that non‑English locales are handled in separate translation PRs. Based on learnings, this is the expected process.src/client/AccountModal.ts (1)
129-129: Make the optimistic toggle independent of whether/users/@mereturnspublicThe optimistic flow is nice and simple, but there's a subtle coupling here:
loadPublicStatus()explicitly works around the fact thatGET /users/@medoes not return the public flag, and instead derives it from/public/player/{publicId}(public: !!data.user).- In the toggle handler, on success you do:
const success = await updateUserMe({ public: newValue }); if (success) { const updated = await getUserMe(); if (updated) { this.userMeResponse = updated; this.isPublicOptimistic = null; this.requestUpdate(); } }If
/users/@mestill doesn't includepublic,getUserMe()will overwrite whateverloadPublicStatus()wrote intouserMeResponse.user.public(and your schema default isfalse), so the toggle can snap back to the wrong state until the next time you recompute it.To decouple the UI from that backend detail and keep the state consistent:
- Trust the toggle value on success.
- Update
userMeResponselocally, and optionally kick offloadPublicStatus()in the background if you still want to verify via the public API.For example:
- const success = await updateUserMe({ public: newValue }); - if (success) { - const updated = await getUserMe(); - if (updated) { - this.userMeResponse = updated; - this.isPublicOptimistic = null; - this.requestUpdate(); - } - } else { + const success = await updateUserMe({ public: newValue }); + if (success) { + if (this.userMeResponse) { + this.userMeResponse = { + ...this.userMeResponse, + user: { + ...this.userMeResponse.user, + public: newValue, + }, + }; + } + this.isPublicOptimistic = null; + this.requestUpdate(); + // Optional: verify with the public profile API without blocking the UI. + void this.loadPublicStatus(); + } else { // Revert on failure this.isPublicOptimistic = null; alert( translateText("account_modal.failed_update_public_setting"), ); this.requestUpdate(); }This keeps the toggle logic simple and correct, regardless of when
/users/@megrows apublicfield. Verify that the current API behavior matches the assumption inloadPublicStatus()before implementing.Also applies to: 159-207
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/client/AccountModal.ts (3)
20-21: Scope of new “public profile” toggle (Discord-only) looks intentional but please confirmYou add
isPublicOptimisticstate and renderpublicProfileToggle()only inside the Discord login view. That means email‑only users never see this setting, even if they might also have a player profile.If leaderboards and public summaries are strictly tied to Discord identity, this is fine; otherwise, you may want to also show the toggle when logged in via email and
playerIdis present.Also applies to: 31-33, 108-133
332-351: open() + loadPublicStatus() rely onuserMeResponse/playerIdbeing pre‑populated elsewhereIn
open()you now do:
getUserMe().then(async (userMe) => { ... if (this.playerId) { await this.loadFromApi(this.playerId); await this.loadPublicStatus(); } ... })and
loadPublicStatus()readsthis.userMeResponse?.player?.publicId.
this.userMeResponseandthis.playerIdare only set via the globaluserMeResponseDOM event in the constructor, not from theuserMeyou just fetched here. If that event has not fired yet (or ever, on some flows),this.playerIdwill be null andloadPublicStatus()will early‑return. In that case the toggle will fall back to the defaultfalsestate even for users who are actually public.Two suggestions:
- When
userMeis available inopen(), deriveplayerIdanduserMeResponsedirectly from it, so this code path is self‑contained:.then(async (userMe) => { if (userMe) { + this.userMeResponse = userMe; + this.playerId = userMe.player?.publicId ?? null; this.loggedInEmail = userMe.user.email ?? null; this.loggedInDiscord = userMe.user.discord?.global_name ?? null; - if (this.playerId) { + if (this.playerId) { await this.loadFromApi(this.playerId); await this.loadPublicStatus(); }
- For readability, consider converting
open()toasync open()and usingtry/catchinstead of mixing.thenwithasync:public async open() { this.modalEl?.open(); this.isLoadingUser = true; this.requestUpdate(); try { const userMe = await getUserMe(); // ... same logic as above ... } catch (err) { console.warn("Failed to fetch user info in AccountModal.open():", err); } finally { this.isLoadingUser = false; this.requestUpdate(); } }This keeps the loading state, player data, and public status loading all in one clear async flow.
Also applies to: 371-396
371-396: Clarify behaviour ofloadPublicStatuson 404 / non‑OK responses
loadPublicStatus():
- Early‑returns if there is no
player.publicId.- On
response.ok, it parses JSON and setsuser.public = !!data.user.- On non‑OK (e.g., 404), it silently does nothing and leaves any existing
user.publicvalue unchanged.This means:
- If the player was previously public and is now private, and the backend responds 404 or similar,
user.publicmay staytruelocally until you refresh.- If you ever pre‑set
user.publicsomewhere else, a failing call here will not clear it.You might want to treat a 404 specifically as “not public” and reset the flag, while still ignoring true network / 5xx errors:
const response = await fetch( `${getApiBase()}/public/player/${this.userMeResponse.player.publicId}`, ); - if (response.ok) { + if (response.ok) { const data = await response.json(); if (this.userMeResponse) { this.userMeResponse = { ...this.userMeResponse, user: { ...this.userMeResponse.user, public: !!data.user, }, }; this.requestUpdate(); } - } + } else if (response.status === 404 && this.userMeResponse) { + this.userMeResponse = { + ...this.userMeResponse, + user: { + ...this.userMeResponse.user, + public: false, + }, + }; + this.requestUpdate(); + }That keeps the local state in sync with the public profile API, while still being lenient on genuine errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/AccountModal.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/graphics/layers/ControlPanel.ts:160-183
Timestamp: 2025-06-07T20:01:52.720Z
Learning: The user its-sii prefers simpler, more readable code over complex type-safe approaches when the benefits don't justify the added complexity. They value maintainability and clarity in code implementation.
📚 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/AccountModal.ts
🧬 Code graph analysis (1)
src/client/AccountModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/client/jwt.ts (3)
updateUserMe(261-288)getUserMe(231-259)getApiBase(20-32)
🔇 Additional comments (1)
src/client/AccountModal.ts (1)
159-206: Toggle success path may lose the updatedpublicvalue and flip UI back to "off"Inside
publicProfileToggle():
- Compute
actualValuefromthis.userMeResponse?.user?.public ?? false.- On change, set
isPublicOptimisticand callupdateUserMe({ public: newValue }).- On success, you call
getUserMe()and overwritethis.userMeResponsewith that result.If
/users/@medoes not return thepublicfield (as noted inloadPublicStatus()), then after a successful toggle the checkbox will appear unchecked even though the server flag is now true (or vice versa):
updateUserMesucceeds.getUserMe()returns a user withoutuser.public.this.userMeResponse.user.publicbecomesundefined.- Next render:
actualValuefalls back tofalse.Fix by either updating
userMeResponselocally with the new value:- const success = await updateUserMe({ public: newValue }); - if (success) { - const updated = await getUserMe(); - if (updated) { - this.userMeResponse = updated; - this.isPublicOptimistic = null; - this.requestUpdate(); - } + const success = await updateUserMe({ public: newValue }); + if (success) { + if (this.userMeResponse) { + this.userMeResponse = { + ...this.userMeResponse, + user: { + ...this.userMeResponse.user, + public: newValue, + }, + }; + } + this.isPublicOptimistic = null; + this.requestUpdate(); + } else {Or call
await this.loadPublicStatus()instead ofgetUserMe()to deriveuser.publicfrom the public profile endpoint.Also consider: disable the checkbox during the request to prevent overlapping toggles, and wrap the handler body in
try/catchto guard against unexpected throws.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/client/AccountModal.ts (3)
202-204: Consider a toast or inline message instead of alert().The
alert()blocks the UI and feels dated. A non-blocking toast notification or inline error message would provide better UX.
211-211: Inline style could use Tailwind classes.The dynamic background color uses an inline style. Consider using Tailwind's arbitrary value syntax or conditional classes for consistency with the rest of the codebase.
Example using Tailwind:
- <div - class="w-11 h-6 rounded-full peer peer-focus:outline-none peer-focus:ring-4 peer-focus:ring-red-300 after:content-[''] after:absolute after:top-[2px] after:left-[2px] after:bg-white after:border-gray-300 after:border after:rounded-full after:h-5 after:w-5 after:transition-all peer-checked:after:translate-x-full peer-checked:after:border-white" - style="background-color: ${checked ? "#4caf50" : "#d9534f"}" - ></div> + <div + class="w-11 h-6 rounded-full peer peer-focus:outline-none peer-focus:ring-4 peer-focus:ring-red-300 after:content-[''] after:absolute after:top-[2px] after:left-[2px] after:bg-white after:border-gray-300 after:border after:rounded-full after:h-5 after:w-5 after:transition-all peer-checked:after:translate-x-full peer-checked:after:border-white ${checked ? 'bg-[#4caf50]' : 'bg-[#d9534f]'}" + ></div>
346-366: Consider refactoring to fully async/await pattern.Mixing
.then(async (userMe) => {...})works but is less readable than makingopen()itself async. This would make the sequential data loading clearer.Example refactor:
- public open() { + public async open() { this.modalEl?.open(); this.isLoadingUser = true; + this.requestUpdate(); - void getUserMe() - .then(async (userMe) => { - if (userMe) { - this.loggedInEmail = userMe.user.email ?? null; - this.loggedInDiscord = userMe.user.discord?.global_name ?? null; - if (this.playerId) { - await this.loadFromApi(this.playerId); - await this.loadPublicStatus(); - } - } else { - this.loggedInEmail = null; - this.loggedInDiscord = null; - } - this.isLoadingUser = false; - this.requestUpdate(); - }) - .catch((err) => { - console.warn("Failed to fetch user info in AccountModal.open():", err); - this.isLoadingUser = false; - this.requestUpdate(); - }); - this.requestUpdate(); + try { + const userMe = await getUserMe(); + if (userMe) { + this.loggedInEmail = userMe.user.email ?? null; + this.loggedInDiscord = userMe.user.discord?.global_name ?? null; + if (this.playerId) { + await this.loadFromApi(this.playerId); + await this.loadPublicStatus(); + } + } else { + this.loggedInEmail = null; + this.loggedInDiscord = null; + } + } catch (err) { + console.warn("Failed to fetch user info in AccountModal.open():", err); + } finally { + this.isLoadingUser = false; + this.requestUpdate(); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/AccountModal.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/graphics/layers/ControlPanel.ts:160-183
Timestamp: 2025-06-07T20:01:52.720Z
Learning: The user its-sii prefers simpler, more readable code over complex type-safe approaches when the benefits don't justify the added complexity. They value maintainability and clarity in code implementation.
📚 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/AccountModal.ts
🧬 Code graph analysis (1)
src/client/AccountModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/client/jwt.ts (2)
updateUserMe(261-288)getApiBase(20-32)
🔇 Additional comments (2)
src/client/AccountModal.ts (2)
159-216: Well-implemented optimistic update pattern.The toggle correctly handles concurrent requests with both the
disabledattribute (line 174) and the guard check (line 180). The rollback on failure (lines 200-205) and immediate UI feedback provide good UX.
381-406: Verify the public endpoint returns the expected structure.The method infers public status by checking if
data.userexists in the response from/public/player/{publicId}. This assumption is fragile—if the API structure changes or returns partial data, the toggle state could be incorrect. Confirm that the endpoint consistently returns auserproperty when the profile is public, and that absence of this property reliably indicates a private profile.
|
please review |
| if (call.status === 401) { | ||
| clearToken(); | ||
| return false; | ||
| } | ||
| if (call.status === 204) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
nitpick: could you add a comment for what the codes mean for future readability?
3 participants
Description:
Closes #1754
This PR adds a toggle in the account modal that allows users who have logged in to control whether their identity (username, avatar) appears on public leaderboards and game summaries.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
furo18