feat(client): render the Steam account in the account UI (OPE-42) - #4750
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSteam identity data is added to the user schema, rendered through a new Steam header component, and integrated into account modal linking, settings, account, and logout views. Steam-primary detection and schema parsing are covered by tests. ChangesSteam account support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant UserMeResponseSchema
participant AccountModal
participant SteamUserHeader
UserMeResponseSchema->>AccountModal: Provide optional Steam identity
AccountModal->>SteamUserHeader: Pass Steam user data
SteamUserHeader-->>AccountModal: Render name and avatar
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 1
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)
129-137: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat Steam as a primary account mode throughout the modal.
Adding
me.steamtoisLinkedAccount()exposes the normal linked-account tabs, including Settings. For a Steam user without a verified email,renderSettingsTab()can still show email and Google linking CTAs. Also, the Steam branch is checked after Discord, Google, and email, so it is skipped whenever another identity is present.Use one Steam-primary check to suppress account-linking UI and evaluate the Steam branch before the other identities, while keeping currency and logout.
Also applies to: 542-550
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/AccountModal.ts` around lines 129 - 137, Update isLinkedAccount and the modal rendering flow to use a single Steam-primary check: evaluate the Steam identity before Discord, Google, and email, and suppress account-linking UI—including the email/Google CTAs in renderSettingsTab—for Steam users without removing currency or logout controls. Reuse the same Steam check at the referenced Steam branch so mixed identities still follow Steam-primary behavior.
🧹 Nitpick comments (1)
tests/core/ApiSchemas.test.ts (1)
18-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the complete Steam identity.
This test checks only
steamId. Also assertpersonaNameandavatarUrlso the fields consumed bySteamUserHeaderare covered.Suggested assertion
- expect(parsed.user.steam?.steamId).toBe("77"); + expect(parsed.user.steam).toEqual({ + steamId: "77", + personaName: "P", + avatarUrl: "https://a", + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/ApiSchemas.test.ts` around lines 18 - 27, Extend the assertions in the “UserMeResponseSchema steam identity” test to validate the parsed Steam identity’s personaName and avatarUrl in addition to steamId. Use the values supplied in the fixture and keep the existing schema parsing and steamId assertion unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/components/baseComponents/stats/SteamUserHeader.ts`:
- Around line 12-20: Update SteamUserHeader’s avatar error handling to track the
broken-avatar condition as reactive state instead of only hiding the existing
image element. In the data setter, detect changes to avatarUrl and reset that
state so a newly received avatar is rendered visibly; ensure the template’s
image visibility reflects the state rather than persisting the prior element’s
inline display change.
---
Outside diff comments:
In `@src/client/AccountModal.ts`:
- Around line 129-137: Update isLinkedAccount and the modal rendering flow to
use a single Steam-primary check: evaluate the Steam identity before Discord,
Google, and email, and suppress account-linking UI—including the email/Google
CTAs in renderSettingsTab—for Steam users without removing currency or logout
controls. Reuse the same Steam check at the referenced Steam branch so mixed
identities still follow Steam-primary behavior.
---
Nitpick comments:
In `@tests/core/ApiSchemas.test.ts`:
- Around line 18-27: Extend the assertions in the “UserMeResponseSchema steam
identity” test to validate the parsed Steam identity’s personaName and avatarUrl
in addition to steamId. Use the values supplied in the fixture and keep the
existing schema parsing and steamId assertion unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8d96aa1-05a0-4ce7-8751-1695f2a0642b
📒 Files selected for processing (5)
resources/lang/en.jsonsrc/client/AccountModal.tssrc/client/components/baseComponents/stats/SteamUserHeader.tssrc/core/ApiSchemas.tstests/core/ApiSchemas.test.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
…PE-42) renderSettingsTab() rendered the bind-email/link-Google prompt whenever a player had no verified email, which included Steam-primary users (who now reach the tab since isLinkedAccount() includes me?.steam). That surfaces account-linking UI, contradicting v1's approved "display-only" Steam intent (linking is scoped separately to OPE-16). Add isSteamPrimary() and suppress the prompt for them; non-Steam users are unaffected since isSteamPrimary() is false for discord/google/email/guest accounts.
The @error handler hid a broken avatar by imperatively setting style.display = "none" on the <img>. Lit reuses the same <img> element across renders of a component instance, so once an avatar failed to load the element stayed hidden even after data later changed to a working avatar. Track the broken state via a @State _avatarError flag instead, reset it whenever new data arrives, and gate the <img> render on avatar && !this._avatarError.
Celant
left a comment
Celant
left a comment
There was a problem hiding this comment.
Static review (I didn't run the suites), read alongside the API side in openfrontio/infra#475. The contracts match exactly on both ends — { steamId: string, personaName: string|null, avatarUrl: string|null } — and steam being .optional() means the client stays compatible with the pre-deploy backend, so the stated rollout order is right and safe in both directions. The component is a faithful mirror of DiscordUserHeader (light DOM via createRenderRoot, same Tailwind classes, same translateText alt-key convention), and the en.json key is in correct alphabetical position.
Two things I'd fix before merge: the header renders completely empty on the null-profile path the API explicitly supports, and isSteamPrimary() checks presence rather than primacy in a way that will bite when OPE-16 lands.
Inline comments below, roughly in severity order.
|
|
||
| render() { | ||
| if (!this._data) return html``; | ||
| const name = this._data.personaName ?? ""; |
There was a problem hiding this comment.
A Steam user with no cached profile gets a completely blank identity card.
With personaName: null and avatarUrl: null the avatar block is skipped and name is "", so this renders an empty <div>. That's the exact fallback the API side explicitly supports — infra#475 returns {steamId, personaName: null, avatarUrl: null} whenever GetPlayerSummaries failed — and it's more reachable than it looks, because that PR has no negative cache, so a Steam outage leaves the row absent indefinitely.
Combined with the me?.steam branch of renderLoggedInAs() rendering only currency + logout, the player ends up seeing a "Connected as" card containing nothing at all — arguably a worse impression than the anonymous-guest state this PR is fixing.
Suggest a fallback: a translated "Steam player" string, or the steamId. Note discord-user-header deliberately falls back to Discord's default avatar rather than rendering nothing.
|
|
||
| // Steam is the primary (and only, in v1) identity for a Steam user — no | ||
| // linking UI (email/Google) is offered for them; see renderSettingsTab. | ||
| private isSteamPrimary(): boolean { |
There was a problem hiding this comment.
This checks presence, not primacy.
It returns true for any user with user.steam. Equivalent today since v1 has no linking — but once OPE-16 lands, a Discord- or email-primary user who links Steam will silently lose the email-binding UI at L239 (hasEmail || this.isSteamPrimary()). The method name and the comment both assert "primary", so worth making the code say it:
private isSteamPrimary(): boolean {
const me = this.userMeResponse?.user;
return !!me?.steam && !me.discord && !me.google && !me.email;
}One line now versus a confusing regression later. renderLoggedInAs() already gets this right by putting the steam branch last in the if/else chain — worth matching here.
| // "connected as" view with no way to retry. | ||
| return ( | ||
| !!(me?.discord ?? me?.google ?? me?.email) || | ||
| !!(me?.discord ?? me?.google ?? me?.email ?? me?.steam) || |
There was a problem hiding this comment.
?? only falls through on null/undefined, so an empty-string email short-circuits the chain and a Steam user would be treated as not-linked. Pre-existing, but appending steam lengthens the chain and makes the trap marginally more reachable. || behaves identically for real values and has no edge case.
| ${this.renderLogoutButton()} | ||
| </div> | ||
| `; | ||
| } else if (me?.steam) { |
There was a problem hiding this comment.
Worth reading together with my comment on SteamUserHeader.ts L31: when the profile cache is empty, this branch is the only thing rendered under "Connected as", and it contains no identity — just a currency balance and a logout button. A name fallback in the header fixes both cases at once.
The reasoning for dropping the Discord/Google CTA here is sound, and putting this branch last in the chain is the right call.
| <div class="p-[3px] rounded-full bg-gray-500"> | ||
| <img | ||
| class="w-12 h-12 rounded-full block" | ||
| src="${avatar}" |
There was a problem hiding this comment.
The avatar won't render at all until the CSP img-src allowlist (OPE-41) ships. The @error handler degrades gracefully, so this is fine on its own — but stacked on the null-personaName case above, a CSP block is what turns "name only" into "nothing at all".
One thing that'll make OPE-41 slower than expected: there's no img-src CSP anywhere in OpenFrontIO or infra source, so it's being set at the edge/proxy. Worth locating that before starting OPE-41 rather than during it.
| } | ||
|
|
||
| describe("UserMeResponseSchema steam identity", () => { | ||
| it("accepts a user.steam identity", () => { |
There was a problem hiding this comment.
Coverage doesn't reach the parts that changed behavior.
This is the only new test, and it's a schema happy path. Neither rendering decision the PR actually makes is covered:
SteamUserHeaderhiding the avatar on@error, and resetting_avatarErrorwhendatais reassigned. That reset is a deliberate design per the comment on it, so it's worth locking in against a future refactor.AccountModal'sme?.steambranch ofrenderLoggedInAs(), andisSteamPrimary()gating the email binding.
A schema case for {personaName: null, avatarUrl: null} would also be worth adding right here, since that's the documented API fallback and the trigger for the blank-header issue.
| export const SteamUserSchema = z.object({ | ||
| steamId: z.string(), | ||
| personaName: z.string().nullable(), | ||
| avatarUrl: z.string().nullable(), |
There was a problem hiding this comment.
Nit: could be z.url() here and in the API's copy. It's server-controlled (the value comes from Steam) and <img src> won't execute a javascript: URL, so this is defense-in-depth rather than a live issue.
| set data(v: SteamUser | null) { | ||
| this._data = v; | ||
| this._avatarError = false; | ||
| this.requestUpdate(); |
There was a problem hiding this comment.
Nit: redundant — _data is @state, so the assignment above already schedules an update. It's copied verbatim from DiscordUserHeader, so it's consistent; noting only that it isn't load-bearing in either.
… test (OPE-42) Address code review on #4750: - SteamUserHeader: fall back to a translated "Steam player" name when personaName is null/empty, so a failed GetPlayerSummaries lookup no longer renders a blank "Connected as" card. - AccountModal.isSteamPrimary(): check that Steam is the sole identity (no discord/google/email) rather than merely present, so a future linked account doesn't silently lose the email-binding UI. - AccountModal.isLinkedAccount(): swap ?? for || in the identity chain so an empty-string field doesn't short-circuit the check. - ApiSchemas.test.ts: cover the documented API fallback shape where user.steam.personaName/avatarUrl are null.
|
Addressed the review in
Consciously skipped: the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/AccountModal.ts`:
- Line 135: Resolve the lint conflict while preserving empty-string-as-absent
behavior: in src/client/AccountModal.ts lines 135-135, update the identity-field
expression to use explicit Boolean checks for me?.discord, me?.google,
me?.email, and me?.steam; in
src/client/components/baseComponents/stats/SteamUserHeader.ts lines 31-32,
replace the personaName fallback with a ternary that falls back when personaName
is null or empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cbcf076e-09ff-4962-9a53-90c8a050ca32
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/client/AccountModal.tssrc/client/components/baseComponents/stats/SteamUserHeader.tstests/core/ApiSchemas.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/core/ApiSchemas.test.ts
- resources/lang/en.json
Celant
left a comment
Celant
left a comment
There was a problem hiding this comment.
Re-review of 3482a2e → 28eba15.
Verified: tsc --noEmit clean, vitest run tests/core/ApiSchemas.test.ts 2/2 passing, prettier --check clean on all five changed files.
Resolved:
- Blank identity card — fixed with a translated
steam_user_header.default_name. Using||rather than??also catches an empty-string persona, which is the right call here. isSteamPrimary()presence-vs-primacy — fixed; the method now matches its own name and comment.??→||inisLinkedAccount()— fixed.- Schema null-case — test added and passing.
One gap didn't move, and it's slightly larger than it was: rendering-level test coverage. See the inline on the test file. I'd treat it as the remaining item rather than a blocker.
Cosmetic: the PR description still describes the component as "persona name + avatar" with no mention of the fallback or the primacy semantics — worth a one-line update, since the description is what a reviewer of the OPE-16 follow-up will read first.
| expect(parsed.user.steam?.steamId).toBe("77"); | ||
| }); | ||
|
|
||
| it("accepts a user.steam identity with null personaName/avatarUrl (GetPlayerSummaries fallback)", () => { |
There was a problem hiding this comment.
Good — this pins the documented API fallback, and it's the case that triggered the blank-header issue.
The gap from my earlier comment didn't move though, and this commit made it slightly larger: both new tests are schema-level, and nothing covers the rendering paths —
SteamUserHeader's@erroravatar hiding, and the_avatarErrorreset ondatareassignment (still a deliberate design with no guard on it).AccountModal'sme?.steambranch ofrenderLoggedInAs().- The two behaviors this commit just added: the fallback name, and the primacy check.
That last one is what I'd most want locked in — see my comment on AccountModal.ts L144.
| // linking UI (email/Google) is offered for them; see renderSettingsTab. | ||
| private isSteamPrimary(): boolean { | ||
| const me = this.userMeResponse?.user; | ||
| return !!me?.steam && !me.discord && !me.google && !me.email; |
There was a problem hiding this comment.
Resolved — this now says what the method name and the comment above it claim.
Worth a unit test though, and it's cheap: this is pure logic over a plain object, no Lit rendering involved. It's also precisely the thing that will silently regress when OPE-16 lands and a Discord-primary user can suddenly have user.steam set — which is the scenario the original version got wrong. A four-case table test (steam-only, steam+discord, steam+email, no-steam) would pin it permanently.
| render() { | ||
| if (!this._data) return html``; | ||
| const name = | ||
| this._data.personaName || translateText("steam_user_header.default_name"); |
There was a problem hiding this comment.
Resolved. Choosing || over ?? here is the right call — it means an empty-string persona falls back too, not just null, so the component can't render a nameless card regardless of what the API sends.
|
Re-review follow-ups addressed in
Consciously not adding the |
Celant
left a comment
Celant
left a comment
There was a problem hiding this comment.
Re-review of 28eba15 → 7e2d500.
Verified: tsc --noEmit clean, npx vitest run tests/client/accountIdentity.test.ts tests/core/ApiSchemas.test.ts → 8 passed (2 files), prettier --check clean on all changed files.
isSteamPrimaryUser is extracted and pinned by a 6-case table — two more than I suggested, and the two additions (no steam, undefined user) are the right ones to have. This was the case I most wanted locked in ahead of OPE-16, so that's the important one done.
Two small things below: a naming/placement nit on the new module, and the rendering-level coverage that's still open. Neither is blocking.
| // identity is present AND no other identity is linked. Guards the account- | ||
| // linking UI so a future OPE-16 Discord/email-primary user who links Steam | ||
| // isn't misclassified. | ||
| export function isSteamPrimaryUser( |
There was a problem hiding this comment.
Nit on placement, not on the extraction — extracting was right.
accountIdentity.ts is the only lowercase-initial module in src/client/ (the sole other one is vite-env.d.ts, which is a conventional ambient-decl name). Every sibling is PascalCase: AccountModal.ts, Auth.ts, Api.ts, ClientEnv.ts, …
Two ways to square it, either is fine:
- Rename to
AccountIdentity.ts(and the test to match). - Or fold the function into
src/client/Utils.ts, which is already where comparable pure client helpers live and are unit-tested fromtests/client/—getActiveModifiersis the direct precedent, tested intests/client/DoomsdayModifierBadge.test.ts. A dedicated module is a fair amount of ceremony for a one-line predicate.
I'd lean toward Utils.ts since the precedent is exact, but the rename is the zero-risk option.
| const email = "player@example.com"; | ||
|
|
||
| describe("isSteamPrimaryUser", () => { | ||
| it.each<[string, User | undefined, boolean]>([ |
There was a problem hiding this comment.
Good table — undefined user and no steam are the two I'd have missed, and this is now permanently pinned against the OPE-16 regression.
The rendering-level half of my earlier coverage note is still open though:
SteamUserHeader's@erroravatar hiding, and the_avatarErrorreset ondatareassignment.AccountModal'sme?.steambranch ofrenderLoggedInAs().
Flagging it once more only because the PR description says "built TDD where testable", and these are testable in this repo's existing setup — 17 files under tests/client/ already do DOM/component work. tests/client/components/FluentSlider.test.ts and tests/client/clan/ClanModal.rendering.test.ts are the closest models.
The _avatarError reset in particular is a deliberate design decision with a comment explaining it and nothing enforcing it, which is exactly the shape of thing that gets refactored away. Fine to defer — but worth saying so in the description rather than leaving it implied.
…ity module (OPE-42) - isLinkedAccount: use ?? with steam ordered before email (repo lint forbids ||) so an empty-string email can't hide a set steam identity. - SteamUserHeader: explicit length check instead of || for the name fallback. - Rename accountIdentity.ts -> AccountIdentity.ts (client modules are PascalCase). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BWxUzYb2uqjcBFQuNSJhMy
|
CI follow-up (
Rendering-level coverage ( |
|
@coderabbitai review The single actionable item from the previous review (the |
|
✅ Action performedReview finished.
|
Celant
left a comment
Celant
left a comment
There was a problem hiding this comment.
Re-review of 7e2d500 → e9b2d9a.
Verified: tsc --noEmit clean, eslint clean on all three changed source files (the rule that drove this commit), vitest 8/8 across both test files, prettier --check clean.
I paid particular attention here, because prefer-nullish-coalescing pushes directly against both || changes I'd asked for — a lint fix like this is an easy place to silently undo a semantic fix. Both survived, and one came out better than what I suggested. Details inline.
I also checked the case-only rename, since those are a classic "works on Windows, breaks on Linux CI" trap: git recorded it as a true rename and git ls-files shows exactly one AccountIdentity.ts and one AccountIdentity.test.ts, correctly cased, with no lingering lowercase entry. Clean.
One fragility note on L139 below — not a defect, and not blocking.
| if (!this._data) return html``; | ||
| // Fall back on an empty or null persona (explicit check rather than `||`, | ||
| // which the repo lint rule forbids). | ||
| const name = this._data.personaName?.trim() |
There was a problem hiding this comment.
This is better than the || I originally suggested, not just a lint-compliant substitute for it. ?.trim() as the test covers three cases where || only covered two: null, "", and whitespace-only — so a persona of " " now falls back instead of rendering an invisible name. Steam does permit padded persona names, so that third case is real rather than theoretical.
Trivial aside, no change needed: the rendered value is the untrimmed personaName, so " Bob " renders with its padding. That's arguably correct (preserve the name as the user set it) and HTML collapses it anyway — just noting the trim is used purely as a predicate.
| // `??` (repo lint rule) with `steam` ordered before `email`: a present | ||
| // steam identity is reached before an empty-string `email` can short- | ||
| // circuit the chain and mis-classify a Steam user as not-linked. | ||
| !!(me?.discord ?? me?.google ?? me?.steam ?? me?.email) || |
There was a problem hiding this comment.
Correct, and I verified the reasoning holds: discord, google, and steam are all objects, so email is the only operand that can be present-but-falsy. Moving it last means an empty-string email can no longer short-circuit ahead of a real Steam identity. The case I raised — {steam, email: ""} → misclassified as not-linked — is fixed.
(The one residual is a sole identity of email: "", which still evaluates false. That's unchanged from main and unrelated to Steam, so out of scope here.)
The fragility, which is why I'm commenting rather than just approving: correctness now depends on operand order, and the only thing protecting it is the comment above. Someone alphabetizing the chain, or adding a sixth identity in the natural-reading slot, silently reintroduces the bug — and no test fails.
You've already built the fix for this shape of problem in this same PR. AccountIdentity.ts now exists and is table-tested; lifting this predicate into it as e.g. isLinkedIdentity(user) with a {steam, email: ""} → true case would convert a comment-guarded invariant into an enforced one, exactly as you did for isSteamPrimaryUser. Cheap, and it retires the comment.
Entirely reasonable to defer — the current code is correct today.
…ccountModal steam branch (OPE-42) Covers the rendering-level gaps flagged on PR #4750: SteamUserHeader's persona-name fallback (null/empty), the avatar @error state hiding the <img>, and the deliberate _avatarError reset on `data` reassignment. Also adds a focused AccountModal test asserting the Steam-primary logged-in view (steam-user-header shown, no login/link CTAs, currency + logout rendered) mirroring the ClanModal.rendering.test.ts approach.
|
Rendering coverage added rather than deferred (
Each was sanity-checked to fail without the behavior. Full suite green (2224 tests), no source changes. That closes every point from both reviews. |
Celant
left a comment
Celant
left a comment
There was a problem hiding this comment.
Re-review of e9b2d9a → 7e462da.
Verified: tsc --noEmit clean, eslint clean, prettier --check clean, and both new files run green — vitest run tests/client/components/SteamUserHeader.test.ts tests/client/AccountModal.rendering.test.ts → 6 passed (2 files).
This closes the coverage gap I'd raised three times. Worth calling out specifically: the _avatarError reset test is the one I described as "a deliberate design decision with a comment explaining it and nothing enforcing it" — it's now enforced, and the test even points back at the setter so the next person understands why it exists. That's the right way to pin an invariant.
The SteamUserHeader file in particular is well-shaped: it tests behaviour through the public surface (data in, rendered DOM out) rather than reaching into internals, so it won't fight a refactor.
Two notes below — one small gap, one about durability of the modal test. Neither is blocking, and neither needs to hold the PR.
Also confirming: the optional isLinkedIdentity extraction from my last review isn't in here. That was explicitly optional and this was the higher-value work, so no complaint — just flagging that it stays open rather than having been silently dropped.
| expect(header.querySelector("img")).toBeNull(); | ||
| }); | ||
|
|
||
| it("falls back to the default name for an empty-string persona name", async () => { |
There was a problem hiding this comment.
Small gap: this covers "", but not whitespace-only (" ") — and whitespace-only is precisely the case that makes ?.trim() better than the || it replaced. Right now the implementation handles a padded persona correctly and nothing would notice if someone simplified the condition back to a plain truthiness check.
One line, same shape as this test:
it("falls back to the default name for a whitespace-only persona name", async () => {
header.data = { steamId: "…7", personaName: " ", avatarUrl: null };
await header.updateComplete;
expect(header.textContent).toContain("steam_user_header.default_name");
});Steam does permit padded persona names, so this isn't a hypothetical input.
| // No login CTAs (Discord/Google login buttons, email field) — those only | ||
| // render on the logged-out `renderLoginOptions()` screen. | ||
| const text = modal.textContent ?? ""; | ||
| expect(text).not.toContain("main.login_discord"); |
There was a problem hiding this comment.
This is the durability concern with the negative assertions, and it's worth a moment because it decides whether this test still means anything in a year.
They do have teeth today — I traced it: renderLoginOptions() emits main.login_discord (AccountModal.ts L716) and is reached whenever !isLinkedAccount(), so a guest render would produce this string and fail the assertion. (Traced rather than mutation-tested, to be clear about what I actually ran.)
The fragility is that they're asserting on translation-key string literals. Rename main.login_discord in AccountModal.ts and this assertion doesn't fail — it silently passes forever, and the test quietly stops testing the thing it was written for. Negative assertions against string constants degrade to no-ops exactly when someone refactors, which is when you most want them.
The cheapest fix that also buys coverage: add the complement case — a Discord user asserting account_modal.link_google is present. That does three things at once: it pins the key positively (so a rename breaks something loudly), it covers the "Discord/email users are unaffected" direction that motivated the primacy fix in the first place, and it exercises the other branch of renderLoggedInAs().
Separately, and much smaller: setLoggedInUser reaches through as unknown as casts into userMeResponse/isLoadingUser, so a rename of either private field breaks the test as a false failure. The comment notes this mirrors ClanModalTestUtils, so it's consistent with the repo — just noting the coupling exists.
…(OPE-42) Two gaps from review on 7e462da: - SteamUserHeader: cover a whitespace-only persona name. This is the case `?.trim()` buys over a plain truthiness check, and Steam permits padded persona names. Mutation-checked: dropping `?.trim()` fails this test and only this test. - AccountModal: add the Discord complement case. The existing assertions are negative checks against translation-key literals, which silently decay into no-ops if a key is renamed. Asserting `account_modal.link_google` is present for a Discord user pins the key positively, and covers the direction the isSteamPrimary() primacy fix was about — a non-Steam user keeps their account-linking UI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8QeDJaeAHrZN5NSjhTEYf
…rap (OPE-84) isLinkedAccount()'s `??` chain was correct only while `email` stayed last: `email` is the sole operand that can be present-but-falsy (empty string), and `??` only falls through on null/undefined. Adding `steam` in OPE-42 is what made that ordering load-bearing — before it, three operands with `email` last were safe by construction. Nothing enforced the invariant but a comment. Rather than pin the ordering with a test, remove the dependence: explicit presence checks are order-independent, so a fifth identity can be added in any slot. Behaviour is unchanged for every input, including an empty-string email on its own (still not an identity). Lives in AccountIdentity.ts alongside isSteamPrimaryUser and is table-tested. The CrazyGames arm stays in the component — it reads component state, not the API response. Mutation-checked: restoring the ordering-dependent chain fails the `steam + empty-string email` case and only that case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8QeDJaeAHrZN5NSjhTEYf
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
1 participant
Implements OPE-42 (client side). A Steam-authenticated desktop player currently shows as an anonymous guest (random name + Login button) despite being logged in. This renders their Steam account.
What
SteamUserschema +user.steamonUserMeResponseSchema(core/ApiSchemas.ts), matching the API's{ steamId, personaName: string|null, avatarUrl: string|null }.<steam-user-header>component (mirrorsdiscord-user-header): persona name + avatar. Falls back to a translated "Steam player" name when the profile is null (the API's documented fallback whenGetPlayerSummariesfailed), and hides the avatar<img>on load error — so the "Connected as" card is never blank.AccountModal: recognizesuser.steamas a signed-in account, renders the Steam header, and hides the Login button + link CTAs for a Steam-primary user (including the email-binding prompt in Settings), while keeping currency + logout.isSteamPrimary()checks primacy (steam && !discord && !google && !email), so it stays correct once OPE-16 linking lands. Discord/Google/email/guest users are unaffected.Notes
user.steam(infra) — inert until that ships (user.steamis.optional()), so land infra first.Built TDD where testable;
tsc/eslint clean; full-branch review clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01BWxUzYb2uqjcBFQuNSJhMy