feat: effects cosmetic category (transport-ship trail) + UI#4418
Open
evanpelle wants to merge 5 commits into
Open
feat: effects cosmetic category (transport-ship trail) + UI#4418evanpelle wants to merge 5 commits into
evanpelle wants to merge 5 commits into
Conversation
Add an "effects" cosmetic category alongside skins/flags, discriminated by `effectType` (only `transportShipTrail` today), whose visual config lives in `attributes` (solid/rainbow/pulse/gradient). Ownership via `effect:*` / `effect:<name>` flares; selection is a per-effectType map persisted in UserSettings and validated server-side in `isEffectAllowed`. UI: a Store "Effects" tab and a home "Effects" picker, both rendering effects grouped by effectType with a sub-header per type via a shared <effects-grid> Lit element. In-game WebGL rendering of the trail is deferred. Schema matches the production cosmetics.json shape (incl. `url`); a test parses the real effect entry. Core changes covered by schema/UserSettings/Privilege tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughAdds effect cosmetics across schema validation, selection storage, privilege checks, UI selection, store browsing, and client modal wiring. The PR also adds localization copy and tests for the new effect data and storage paths. ChangesEffects cosmetics flow
Sequence Diagram(s)sequenceDiagram
participant EffectsInput
participant Main
participant EffectsModal
participant fetchCosmetics
participant EffectsGrid
participant UserSettings
EffectsInput->>Main: effects-input-click
Main->>EffectsModal: open effects-modal
EffectsModal->>fetchCosmetics: fetchCosmetics()
fetchCosmetics-->>EffectsModal: Cosmetics
EffectsModal->>EffectsGrid: mode="select", cosmetics, userMeResponse
EffectsGrid->>UserSettings: getSelectedEffects()
UserSettings-->>EffectsGrid: selected effect refs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
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/Store.ts (1)
256-268: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a neutral affiliate empty-state message here.
This filter now serves effects too, but the empty state below still says
store.no_skins. An affiliate link for an effect can now show the wrong message when nothing is purchasable. Please switch this path to a generic affiliate/store-empty key.🤖 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/Store.ts` around lines 256 - 268, The affiliate grid empty state is still using a skin-specific message even though `renderAffiliateGrid()` now includes effects and other purchasable cosmetics. Update the empty-state key in `Store.renderAffiliateGrid()` to a neutral affiliate/store-empty localization key instead of `store.no_skins`, so the message matches any empty purchasable result.
🧹 Nitpick comments (1)
tests/Privilege.test.ts (1)
559-624: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueNice coverage — one extra case would lock the flare scope.
The exact-match path is tested in the happy direction, but nothing proves a name-scoped flare stays scoped. A short test where the user holds
effect:crimsonyet asks forspectrumshould be forbidden — this guards against a future change that accidentally widens the match.🧪 Suggested extra test
test("exact-match flare does not unlock a different effect", () => { const result = effectChecker.isAllowed(["effect:crimson"], { effects: { transportShipTrail: "spectrum" }, }); expect(result.type).toBe("forbidden"); });🤖 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/Privilege.test.ts` around lines 559 - 624, Add a missing negative test in Privilege.test.ts for effectChecker.isAllowed to verify exact-match flares stay scoped: when the caller has effect:crimson but requests transportShipTrail: spectrum, the result should be forbidden. Place it alongside the existing Effect validation in isAllowed tests to cover the effectChecker.isAllowed path and prevent broadening exact-match permissions.
🤖 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/EffectsInput.ts`:
- Around line 38-48: The async mount flow in connectedCallback can race with
disconnectedCallback, causing access to a nulled _abortController after
getPlayerCosmetics() resolves. Capture the AbortController in a local variable
at the start of connectedCallback, then after awaiting getPlayerCosmetics()
verify it is still the current controller before setting trailAttributes or
registering the window listener. Use the existing connectedCallback and
disconnectedCallback lifecycle methods in EffectsInput to guard against
teardown/navigation races and only read controller.signal when the instance is
still mounted.
---
Outside diff comments:
In `@src/client/Store.ts`:
- Around line 256-268: The affiliate grid empty state is still using a
skin-specific message even though `renderAffiliateGrid()` now includes effects
and other purchasable cosmetics. Update the empty-state key in
`Store.renderAffiliateGrid()` to a neutral affiliate/store-empty localization
key instead of `store.no_skins`, so the message matches any empty purchasable
result.
---
Nitpick comments:
In `@tests/Privilege.test.ts`:
- Around line 559-624: Add a missing negative test in Privilege.test.ts for
effectChecker.isAllowed to verify exact-match flares stay scoped: when the
caller has effect:crimson but requests transportShipTrail: spectrum, the result
should be forbidden. Place it alongside the existing Effect validation in
isAllowed tests to cover the effectChecker.isAllowed path and prevent broadening
exact-match permissions.
🪄 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
Run ID: 12a0ef6b-597f-4dd4-a257-a663a32a92a9
📒 Files selected for processing (19)
index.htmlresources/lang/en.jsonsrc/client/Api.tssrc/client/Cosmetics.tssrc/client/EffectsInput.tssrc/client/EffectsModal.tssrc/client/Main.tssrc/client/Store.tssrc/client/components/CosmeticButton.tssrc/client/components/EffectPreview.tssrc/client/components/EffectsGrid.tssrc/client/components/PlayPage.tssrc/core/CosmeticSchemas.tssrc/core/Schemas.tssrc/core/game/UserSettings.tssrc/server/Privilege.tstests/CosmeticSchemas.test.tstests/Privilege.test.tstests/UserSettings.test.ts
Comment on lines
+38
to
+48
| async connectedCallback() { | ||
| super.connectedCallback(); | ||
| this._abortController = new AbortController(); | ||
| const cosmetics = await getPlayerCosmetics(); | ||
| this.trailAttributes = | ||
| cosmetics.effects?.["transportShipTrail"]?.attributes ?? null; | ||
| window.addEventListener( | ||
| `${USER_SETTINGS_CHANGED_EVENT}:${EFFECTS_KEY}`, | ||
| this._onCosmeticSelected, | ||
| { signal: this._abortController.signal }, | ||
| ); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the async mount path against disconnects.
If getPlayerCosmetics() resolves after disconnectedCallback() runs, Line 47 reads this._abortController.signal after _abortController was nulled, which can throw during teardown/navigation. Capture the controller in a local variable and bail out if it is no longer current before mutating state or registering the listener.
Suggested fix
async connectedCallback() {
super.connectedCallback();
- this._abortController = new AbortController();
+ const abortController = new AbortController();
+ this._abortController = abortController;
const cosmetics = await getPlayerCosmetics();
+ if (this._abortController !== abortController) {
+ return;
+ }
this.trailAttributes =
cosmetics.effects?.["transportShipTrail"]?.attributes ?? null;
window.addEventListener(
`${USER_SETTINGS_CHANGED_EVENT}:${EFFECTS_KEY}`,
this._onCosmeticSelected,
- { signal: this._abortController.signal },
+ { signal: abortController.signal },
);
}📝 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.
Suggested change
| async connectedCallback() { | |
| super.connectedCallback(); | |
| this._abortController = new AbortController(); | |
| const cosmetics = await getPlayerCosmetics(); | |
| this.trailAttributes = | |
| cosmetics.effects?.["transportShipTrail"]?.attributes ?? null; | |
| window.addEventListener( | |
| `${USER_SETTINGS_CHANGED_EVENT}:${EFFECTS_KEY}`, | |
| this._onCosmeticSelected, | |
| { signal: this._abortController.signal }, | |
| ); | |
| async connectedCallback() { | |
| super.connectedCallback(); | |
| const abortController = new AbortController(); | |
| this._abortController = abortController; | |
| const cosmetics = await getPlayerCosmetics(); | |
| if (this._abortController !== abortController) { | |
| return; | |
| } | |
| this.trailAttributes = | |
| cosmetics.effects?.["transportShipTrail"]?.attributes ?? null; | |
| window.addEventListener( | |
| `${USER_SETTINGS_CHANGED_EVENT}:${EFFECTS_KEY}`, | |
| this._onCosmeticSelected, | |
| { signal: abortController.signal }, | |
| ); | |
| } |
🤖 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/EffectsInput.ts` around lines 38 - 48, The async mount flow in
connectedCallback can race with disconnectedCallback, causing access to a nulled
_abortController after getPlayerCosmetics() resolves. Capture the
AbortController in a local variable at the start of connectedCallback, then
after awaiting getPlayerCosmetics() verify it is still the current controller
before setting trailAttributes or registering the window listener. Use the
existing connectedCallback and disconnectedCallback lifecycle methods in
EffectsInput to guard against teardown/navigation races and only read
controller.signal when the instance is still mounted.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Match the production cosmetics.json shape: the effects catalog is now nested two levels — effects[effectType][effectName] — and the effectType value is snake_case `transport_ship_trail`. - CosmeticsSchema.effects -> record(effectType, record(name, Effect)) - catalog reads (resolveCosmetics, Privilege.isEffectAllowed, getPlayerCosmetics, getPlayerCosmeticsRefs) use effects[effectType][name]; drop the now-redundant effectType-mismatch branch - player-side (refs/resolved/UserSettings) stays flat-by-effectType (one effect per type) - rename effectType value + i18n key + EffectsInput lookup to transport_ship_trail - tests updated (nested fixtures, snake_case, parses the real nested blob) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/CosmeticSchemas.ts (1)
141-142: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConstrain
effectstoEffectTypeSchema
effectsstill accepts any string key, soeffects["bogus"]parses and the outer map no longer tracks the inner discriminator. Switch the outer record toz.partialRecord(EffectTypeSchema, z.record(z.string(), EffectSchema)); add asuperRefineonly if you also want to reject key/effectTypemismatches.🤖 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/core/CosmeticSchemas.ts` around lines 141 - 142, The `effects` field in `CosmeticSchemas` is still keyed by any string, so update the outer map to use `EffectTypeSchema` instead of a plain string key. In `CosmeticSchemas`, change the `effects` definition to `z.partialRecord(EffectTypeSchema, z.record(z.string(), EffectSchema)).optional()` so only valid effect types are accepted; add a `superRefine` only if you also need to validate that inner `effectType` values match their outer key.
🤖 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.
Nitpick comments:
In `@src/core/CosmeticSchemas.ts`:
- Around line 141-142: The `effects` field in `CosmeticSchemas` is still keyed
by any string, so update the outer map to use `EffectTypeSchema` instead of a
plain string key. In `CosmeticSchemas`, change the `effects` definition to
`z.partialRecord(EffectTypeSchema, z.record(z.string(),
EffectSchema)).optional()` so only valid effect types are accepted; add a
`superRefine` only if you also need to validate that inner `effectType` values
match their outer key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58dd465c-3341-4972-8f0e-d58eeee22117
📒 Files selected for processing (9)
resources/lang/en.jsonsrc/client/Cosmetics.tssrc/client/EffectsInput.tssrc/client/components/CosmeticButton.tssrc/core/CosmeticSchemas.tssrc/server/Privilege.tstests/CosmeticSchemas.test.tstests/Privilege.test.tstests/UserSettings.test.ts
✅ Files skipped from review due to trivial changes (1)
- resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/UserSettings.test.ts
- tests/CosmeticSchemas.test.ts
- tests/Privilege.test.ts
- src/client/EffectsInput.ts
- src/client/Cosmetics.ts
- src/server/Privilege.ts
- src/client/components/CosmeticButton.ts
…bute types
The served cosmetics.json nests effects as effects[effectType][effectName] with
a camelCase effectType outer key (transportShipTrail) and NO effectType field on
each effect. The previous discriminated-union-on-effectType rejected the whole
cosmetics payload.
- EffectSchema: drop the effectType field + discriminated union; effectType is
the outer catalog key (any string, so unknown effectTypes don't fail the parse)
- attributes: lenient { type: string, color?, color2? } so unknown attribute
types don't fail the parse
- carry effectType from the outer key onto ResolvedCosmetic + resolved
PlayerEffect (server + client)
- swatch renderer: neutral fallback for unknown attribute types; UI renders only
EFFECT_TYPES, so unknown effectTypes are ignored
- tests: nested fixtures, no effectType field, + tolerate-unknown cases
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
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/core/CosmeticSchemas.ts`:
- Around line 110-113: EffectSchema is over-constraining all effect entries by
always using TransportShipTrailAttributesSchema for attributes, which causes
unknown outer effect types to fail parsing. Update EffectSchema to branch on the
outer effectType key: keep the transportShipTrail-specific attributes schema for
known trail effects, and use a looser fallback attributes schema for any unknown
effect type so the catalog still parses without rejecting future entries.
🪄 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
Run ID: d08da30a-d509-4cb7-b68c-da4e7768932c
📒 Files selected for processing (11)
resources/lang/en.jsonsrc/client/Cosmetics.tssrc/client/EffectsInput.tssrc/client/components/CosmeticButton.tssrc/client/components/EffectPreview.tssrc/client/components/EffectsGrid.tssrc/core/CosmeticSchemas.tssrc/server/Privilege.tstests/CosmeticSchemas.test.tstests/Privilege.test.tstests/UserSettings.test.ts
✅ Files skipped from review due to trivial changes (1)
- resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/UserSettings.test.ts
- src/client/components/EffectPreview.ts
- src/client/EffectsInput.ts
- src/client/components/CosmeticButton.ts
- src/server/Privilege.ts
- src/client/components/EffectsGrid.ts
- src/client/Cosmetics.ts
- tests/Privilege.test.ts
Comment on lines
+110
to
+113
| export const EffectSchema = CosmeticSchema.extend({ | ||
| attributes: TransportShipTrailAttributesSchema, | ||
| url: z.string().optional(), | ||
| }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Unknown outer effect types still must look like transportShipTrail.
Line 111 hard-codes every effect entry to TransportShipTrailAttributesSchema. So effects.someFutureEffect.* only parses if its attributes also has the trail-style { type, color?, color2? } shape, which breaks the stated “unknown effect types should not fail the parse” contract and can reject the whole catalog at the client/server validation gates. Please validate known shapes from the outer effectType key and fall back to a loose attributes schema for unknown keys.
🤖 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/core/CosmeticSchemas.ts` around lines 110 - 113, EffectSchema is
over-constraining all effect entries by always using
TransportShipTrailAttributesSchema for attributes, which causes unknown outer
effect types to fail parsing. Update EffectSchema to branch on the outer
effectType key: keep the transportShipTrail-specific attributes schema for known
trail effects, and use a looser fallback attributes schema for any unknown
effect type so the catalog still parses without rejecting future entries.
Catalog lookups assumed effects[effectType][key] where key === effect.name, but selection and ownership flares are name-based; a catalog whose object key differs from the effect's name made the effect silently unselectable. Add findEffect() (key fast-path, name-field fallback) and use it at all three lookup sites (getPlayerCosmeticsRefs, getPlayerCosmetics, isEffectAllowed). Also register effects-input/effects-modal/effects-grid in LangSelector.applyTranslation so they re-render once translations load — without this the effects button rendered before i18n was ready and stayed stuck on the literal "effects.title". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/Privilege.ts (1)
261-269: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject unknown effect buckets before casting them to
EffectType.
CosmeticsSchemanow tolerates unknown outereffectskeys, but Line 267 force-casts every incoming key toEffectTypeand Line 294 returns it as if it were a supported value. If the catalog ever contains a future bucket,findEffect()will allow it through here and downstream code will see an out-of-unioneffectTypeeven though this PR only supportstransportShipTrail. Please validate the key against the supported effect-type set at this boundary, or widen the sharedPlayerEffectcontract everywhere instead of casting.Also applies to: 280-295
🤖 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/server/Privilege.ts` around lines 261 - 269, Reject unsupported effect buckets at the Privilege boundary instead of force-casting them to EffectType. In Privilege.ts, update the refs.effects handling in the loop around isEffectAllowed/findEffect so only supported keys (currently transportShipTrail) are accepted; unknown keys should be skipped or explicitly rejected before assigning to cosmetics.effects. Also make sure the returned value from findEffect and the PlayerEffect/effects contract stay aligned so downstream code never receives an out-of-union effectType from supported helpers like isEffectAllowed and findEffect.
🤖 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.
Outside diff comments:
In `@src/server/Privilege.ts`:
- Around line 261-269: Reject unsupported effect buckets at the Privilege
boundary instead of force-casting them to EffectType. In Privilege.ts, update
the refs.effects handling in the loop around isEffectAllowed/findEffect so only
supported keys (currently transportShipTrail) are accepted; unknown keys should
be skipped or explicitly rejected before assigning to cosmetics.effects. Also
make sure the returned value from findEffect and the PlayerEffect/effects
contract stay aligned so downstream code never receives an out-of-union
effectType from supported helpers like isEffectAllowed and findEffect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 696d0b3b-2fd2-4031-b950-559bb02283e2
📒 Files selected for processing (7)
resources/lang/en.jsonsrc/client/Cosmetics.tssrc/client/LangSelector.tssrc/core/CosmeticSchemas.tssrc/server/Privilege.tstests/CosmeticSchemas.test.tstests/Privilege.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/client/LangSelector.ts
- resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Privilege.test.ts
- src/client/Cosmetics.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a new
effectscosmetic category alongsideskins/flags. Each effect is discriminated byeffectType(onlytransportShipTrailtoday), whose visual config lives inattributes(solid/rainbow/pulse/gradient). Schema matches the production cosmetics.json shape exactly (incl. theurlfield).This PR is UI + taxonomy only — the in-game WebGL trail rendering is intentionally deferred.
UI
effectTypewith a sub-header per type, via a shared<effects-grid>Lit element (mode="select"for the picker,mode="purchase"for the store). The picker shows owned effects + a Default tile and persists per-type; the store shows purchasable effects.Data flow
effect:*/effect:<name>flares (reusescosmeticRelationship).effectTypemap persisted in UserSettings (settings.effects).isEffectAllowed, wired intoisAllowed.getPlayerCosmeticsRefs/getPlayerCosmeticsresolve effects the same way as skins/flags (kept-on-fetch-failure, server is authority).Tests
tsc --noEmit, ESLint, Prettier clean; full suite green.CosmeticSchemasparse tests (incl. parsing the realread_transport_trailentry),UserSettingsper-type selection, andPrivilegeeffect validation.Notes / follow-ups
transportShipTrailtype (friendlier than the id)./shop/purchase(purchaseWithCurrency) needs to learn"effect"for currency purchase of effects; the dollar/product purchase path already works. Client types were widened accordingly.🤖 Generated with Claude Code