cosmetic refactor#3628
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors the cosmetics system by introducing unified Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🧹 Nitpick comments (1)
src/client/graphics/layers/WinModal.ts (1)
159-160: Fetchmeand cosmetics in parallel here.These calls are independent. Awaiting them one after the other adds a full extra network round trip before the modal can open.
Suggested fix
async loadPatternContent() { - const me = await getUserMe(); - const cosmetics = await fetchCosmetics(); + const [me, cosmetics] = await Promise.all([ + getUserMe(), + fetchCosmetics(), + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/WinModal.ts` around lines 159 - 160, The code awaits getUserMe() and then fetchCosmetics() sequentially, causing unnecessary latency; change the implementation in WinModal.ts to run them in parallel (e.g., use Promise.all with the getUserMe and fetchCosmetics promises) and then destructure the results into me and cosmetics so both requests happen concurrently before continuing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/components/CosmeticInfo.ts`:
- Around line 25-26: The empty-state guard currently returns nothing even when
showAdFree is true, preventing the ad-free label/tooltip from rendering; update
the guard in the component's render/template logic (the check around the early
return at/near the lines referencing "nothing") so it only returns nothing when
there is truly no metadata AND showAdFree is false — i.e., include showAdFree in
the condition or invert the logic to allow rendering when showAdFree is true;
apply the same change to the second guard affecting the branch around lines
59-63 so the ad-free branch can render independently of other metadata.
In `@src/client/components/PatternPreview.ts`:
- Around line 17-22: The PatternPreview component currently always renders an
<img> with src set to generatePreviewDataUrl(pattern, width, height), which can
return an empty string on decode failure and produce a broken image; instead,
call generatePreviewDataUrl(...) and if it returns a falsy/empty string, return
renderBlankPreview() (preserving the same classes/attributes when rendering the
<img> case). Apply this same change to the second preview branch that currently
creates the <img> (the other occurrence referenced in the comment), replacing
usages that pass "" into src with a conditional that falls back to
renderBlankPreview().
In `@src/client/Store.ts`:
- Line 173: The code creates a new UserSettings instance for the flag lookup
instead of using the class property; change the call to use the existing
this.userSettings (i.e., use this.userSettings.getFlag() with the same nullish
fallback) so you don't instantiate a redundant UserSettings object and keep
lookups consistent; ensure this.userSettings is initialized before use and
preserve the default "" fallback.
---
Nitpick comments:
In `@src/client/graphics/layers/WinModal.ts`:
- Around line 159-160: The code awaits getUserMe() and then fetchCosmetics()
sequentially, causing unnecessary latency; change the implementation in
WinModal.ts to run them in parallel (e.g., use Promise.all with the getUserMe
and fetchCosmetics promises) and then destructure the results into me and
cosmetics so both requests happen concurrently before continuing.
🪄 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: 693d3830-c4da-4739-ab49-7177d486434e
📒 Files selected for processing (17)
src/client/AccountModal.tssrc/client/Cosmetics.tssrc/client/FlagInputModal.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/TokenLoginModal.tssrc/client/components/CosmeticButton.tssrc/client/components/CosmeticInfo.tssrc/client/components/FlagButton.tssrc/client/components/PatternButton.tssrc/client/components/PatternPreview.tssrc/client/graphics/layers/WinModal.tssrc/core/CosmeticSchemas.tstests/Privilege.test.tstests/ResolveCosmetics.test.ts
💤 Files with no reviewable changes (5)
- src/client/TokenLoginModal.ts
- src/client/AccountModal.ts
- src/client/Matchmaking.ts
- src/client/components/FlagButton.ts
- src/client/components/PatternButton.ts
Comment on lines
+17
to
+22
| return html`<img | ||
| src="${generatePreviewDataUrl(pattern, width, height)}" | ||
| alt="Pattern preview" | ||
| class="w-full h-full object-contain [image-rendering:pixelated] pointer-events-none" | ||
| draggable="false" | ||
| />`; |
Contributor
There was a problem hiding this comment.
Avoid rendering <img src=""> on preview decode failures.
generatePreviewDataUrl() already treats decode errors as recoverable, but this branch still feeds "" into src. That usually degrades into a broken image or a request back to the current document. Fall back to renderBlankPreview() instead.
Suggested fix
export function renderPatternPreview(
pattern: PlayerPattern | null,
width: number,
height: number,
): TemplateResult {
if (pattern === null) {
return renderBlankPreview();
}
+ const src = generatePreviewDataUrl(pattern, width, height);
+ if (!src) {
+ return renderBlankPreview();
+ }
return html`<img
- src="${generatePreviewDataUrl(pattern, width, height)}"
+ src="${src}"
alt="Pattern preview"
class="w-full h-full object-contain [image-rendering:pixelated] pointer-events-none"
draggable="false"
/>`;
}Also applies to: 83-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/components/PatternPreview.ts` around lines 17 - 22, The
PatternPreview component currently always renders an <img> with src set to
generatePreviewDataUrl(pattern, width, height), which can return an empty string
on decode failure and produce a broken image; instead, call
generatePreviewDataUrl(...) and if it returns a falsy/empty string, return
renderBlankPreview() (preserving the same classes/attributes when rendering the
<img> case). Apply this same change to the second preview branch that currently
creates the <img> (the other occurrence referenced in the comment), replacing
usages that pass "" into src with a conditional that falls back to
renderBlankPreview().
| </div>`; | ||
| } | ||
|
|
||
| const selectedFlag = new UserSettings().getFlag() ?? ""; |
Contributor
There was a problem hiding this comment.
Use existing this.userSettings instead of creating a new instance.
Line 173 creates new UserSettings() but the class already has this.userSettings as a property (line 32). Using the existing instance avoids redundant object creation and keeps flag lookups consistent.
Proposed fix
- const selectedFlag = new UserSettings().getFlag() ?? "";
+ const selectedFlag = this.userSettings.getFlag() ?? "";📝 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
| const selectedFlag = new UserSettings().getFlag() ?? ""; | |
| const selectedFlag = this.userSettings.getFlag() ?? ""; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/Store.ts` at line 173, The code creates a new UserSettings
instance for the flag lookup instead of using the class property; change the
call to use the existing this.userSettings (i.e., use
this.userSettings.getFlag() with the same nullish fallback) so you don't
instantiate a redundant UserSettings object and keep lookups consistent; ensure
this.userSettings is initialized before use and preserve the default ""
fallback.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
force-pushed
the
main-cosmetics-refactor
branch
from
b76165a to
a545e16
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/client/graphics/layers/WinModal.ts (1)
182-183: Non-null assertions are safe but could be more explicit.
rc.cosmetic!.product!relies on purchasable patterns always having non-nullcosmeticandproduct. This is true by design (default pattern is always "owned"), but a guard would make it clearer for future readers.Optional: Add early-continue guard
${selected.map( - (r) => html` + (r) => { + if (!r.cosmetic?.product) return nothing; + return html` <cosmetic-button .resolved=${r} .onPurchase=${(rc: ResolvedCosmetic) => - handlePurchase(rc.cosmetic!.product!, rc.colorPalette?.name)} + handlePurchase(rc.cosmetic!.product!, rc.colorPalette?.name)} ></cosmetic-button> - `, + `; + } )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/WinModal.ts` around lines 182 - 183, Replace the chained non-null assertions in the purchase handler with an explicit guard to make assumptions clear: before calling handlePurchase, verify that the ResolvedCosmetic instance has a cosmetic and that cosmetic.product exists (e.g., check rc.cosmetic && rc.cosmetic.product) and only then invoke handlePurchase(rc.cosmetic.product, rc.colorPalette?.name); reference the onPurchase attribute where rc is typed as ResolvedCosmetic and the handlePurchase function to locate the change and consider an early return/continue or conditional noop to keep behavior identical if either value is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/components/PatternPreview.ts`:
- Around line 100-104: The code in PatternPreview that creates a canvas and does
const ctx = canvas.getContext("2d") should not throw when ctx is null (e.g.,
SSR/workers); instead handle the null like other error paths by returning an
empty string. Replace the throw in the block that checks ctx with an early
return of "" (or the component's existing empty-string error return path),
ensuring the function/component (PatternPreview) continues to return "" when ctx
is null and does not crash non-browser environments.
In `@src/client/Cosmetics.ts`:
- Around line 232-234: The lookup for resolvedPalette using cp and
cosmetics.colorPalettes can yield null silently when
cosmetics.colorPalettes?.[cp.name] is undefined; update the code in the
Cosmetics class where resolvedPalette is computed to explicitly handle the
missing-key case: check if cp is non-null and the lookup returns undefined, then
either (a) log a warning including cp.name and cp.isArchived via the module
logger/console, and set resolvedPalette to a safe fallback (e.g., cp-derived
default or empty palette) or (b) throw/return early if a palette is required;
reference the resolvedPalette variable, the cp object and its name (cp.name),
and cosmetics.colorPalettes to locate and change the logic.
---
Nitpick comments:
In `@src/client/graphics/layers/WinModal.ts`:
- Around line 182-183: Replace the chained non-null assertions in the purchase
handler with an explicit guard to make assumptions clear: before calling
handlePurchase, verify that the ResolvedCosmetic instance has a cosmetic and
that cosmetic.product exists (e.g., check rc.cosmetic && rc.cosmetic.product)
and only then invoke handlePurchase(rc.cosmetic.product, rc.colorPalette?.name);
reference the onPurchase attribute where rc is typed as ResolvedCosmetic and the
handlePurchase function to locate the change and consider an early
return/continue or conditional noop to keep behavior identical if either value
is missing.
🪄 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: 80a6b35d-ebe1-4637-b8e9-a9cffd82a76f
📒 Files selected for processing (17)
src/client/AccountModal.tssrc/client/Cosmetics.tssrc/client/FlagInputModal.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/TokenLoginModal.tssrc/client/components/CosmeticButton.tssrc/client/components/CosmeticInfo.tssrc/client/components/FlagButton.tssrc/client/components/PatternButton.tssrc/client/components/PatternPreview.tssrc/client/graphics/layers/WinModal.tssrc/core/CosmeticSchemas.tstests/Privilege.test.tstests/ResolveCosmetics.test.ts
💤 Files with no reviewable changes (5)
- src/client/TokenLoginModal.ts
- src/client/AccountModal.ts
- src/client/Matchmaking.ts
- src/client/components/FlagButton.ts
- src/client/components/PatternButton.ts
✅ Files skipped from review due to trivial changes (4)
- src/client/PatternInput.ts
- tests/Privilege.test.ts
- src/core/CosmeticSchemas.ts
- tests/ResolveCosmetics.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/Store.ts
Comment on lines
+100
to
+104
| const canvas = document.createElement("canvas"); | ||
| canvas.width = width; | ||
| canvas.height = height; | ||
| const ctx = canvas.getContext("2d"); | ||
| if (!ctx) throw new Error("2D context not supported"); |
Contributor
There was a problem hiding this comment.
Throwing on missing 2D context may crash in non-browser environments.
ctx.getContext("2d") can return null in workers or SSR. Throwing here is abrupt. Consider returning "" like other error paths.
Suggested fix
const ctx = canvas.getContext("2d");
- if (!ctx) throw new Error("2D context not supported");
+ if (!ctx) {
+ console.error("2D context not supported");
+ return "";
+ }📝 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
| const canvas = document.createElement("canvas"); | |
| canvas.width = width; | |
| canvas.height = height; | |
| const ctx = canvas.getContext("2d"); | |
| if (!ctx) throw new Error("2D context not supported"); | |
| const canvas = document.createElement("canvas"); | |
| canvas.width = width; | |
| canvas.height = height; | |
| const ctx = canvas.getContext("2d"); | |
| if (!ctx) { | |
| console.error("2D context not supported"); | |
| return ""; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/components/PatternPreview.ts` around lines 100 - 104, The code in
PatternPreview that creates a canvas and does const ctx =
canvas.getContext("2d") should not throw when ctx is null (e.g., SSR/workers);
instead handle the null like other error paths by returning an empty string.
Replace the throw in the block that checks ctx with an early return of "" (or
the component's existing empty-string error return path), ensuring the
function/component (PatternPreview) continues to return "" when ctx is null and
does not crash non-browser environments.
a545e16 to
947a3f9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/client/Cosmetics.ts (1)
232-244:⚠️ Potential issue | 🟠 MajorDo not emit a colored variant after its palette lookup failed.
If
cpexists butcosmetics.colorPalettes?.[cp.name]is missing, this still returns a resolved entry withrelationshipfor the colored variant butcolorPalette: null. Downstream code then previews/selects the base pattern and drops the palette name from checkout.Proposed fix
const resolvedPalette = cp ? (cosmetics.colorPalettes?.[cp.name] ?? null) : null; + if (cp && !resolvedPalette) { + console.warn( + `Missing color palette "${cp.name}" for pattern "${patternKey}"`, + ); + continue; + } const key = cp ? `pattern:${patternKey}:${cp.name}` : `pattern:${patternKey}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Cosmetics.ts` around lines 232 - 244, The current code pushes a colored variant even when the palette lookup fails (cp exists but cosmetics.colorPalettes?.[cp.name] is undefined), resulting in colorPalette: null; update the logic around cp/resolvedPalette so that you only push the pattern entry when either cp is null (base variant) or when cp is present AND resolvedPalette is non-null; reference the variables cp, resolvedPalette, cosmetics.colorPalettes, pattern, patternKey and the result.push call — skip emitting the colored variant if resolvedPalette is null to avoid downstream preview/checkout issues.src/client/components/PatternPreview.ts (2)
17-22:⚠️ Potential issue | 🟡 MinorFall back to the blank preview when URL generation fails.
generatePreviewDataUrl(...)already returns""on decode errors. Rendering<img src="">here gives a broken preview instead of the existing blank state.Proposed fix
export function renderPatternPreview( pattern: PlayerPattern | null, width: number, height: number, ): TemplateResult { if (pattern === null) { return renderBlankPreview(); } + const src = generatePreviewDataUrl(pattern, width, height); + if (!src) { + return renderBlankPreview(); + } return html`<img - src="${generatePreviewDataUrl(pattern, width, height)}" + src="${src}" alt="Pattern preview" class="w-full h-full object-contain [image-rendering:pixelated] pointer-events-none" draggable="false" />`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/PatternPreview.ts` around lines 17 - 22, The PatternPreview component currently renders an <img> with src set from generatePreviewDataUrl(pattern, width, height) even when that helper returns an empty string; change PatternPreview to check the returned dataUrl first and only render the <img> when dataUrl is non-empty, otherwise render the existing blank preview placeholder (the same element shown when no pattern exists) so you avoid creating a broken <img src="">; locate this logic in PatternPreview and use generatePreviewDataUrl(...) result to branch the render.
100-104:⚠️ Potential issue | 🟡 MinorHandle missing 2D canvas context like the other recoverable preview errors.
Throwing here can crash tests or non-browser-like environments. This helper already uses
""as the safe failure result, so the null-context case should do the same.Proposed fix
const ctx = canvas.getContext("2d"); - if (!ctx) throw new Error("2D context not supported"); + if (!ctx) { + console.error("2D context not supported"); + return ""; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/PatternPreview.ts` around lines 100 - 104, In the canvas setup in PatternPreview.ts where you call canvas.getContext("2d") and assign to ctx, avoid throwing on a null 2D context; instead handle it like other recoverable preview errors by returning the safe failure result (an empty string) from the surrounding helper (the function that produces the preview image) rather than throwing; locate the canvas creation and the ctx null check (the canvas.getContext("2d") call and the if (!ctx) branch) and replace the throw new Error(...) with code that returns "" so tests and non-browser environments don't crash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/components/CosmeticButton.ts`:
- Around line 29-31: handleClick currently always calls onSelect(this.resolved)
which routes purchasable items into selection; change handleClick to branch on
this.resolved.relationship: when relationship === 'purchasable' do not call
onSelect but instead invoke a dedicated purchase handler (e.g., call a new prop
like onPurchase or onStartCheckout with this.resolved) to start checkout, and
when relationship is anything else call this.onSelect?.(this.resolved); update
the component props to accept and use that new purchase handler (and ensure
callers wired to start checkout in the store).
---
Duplicate comments:
In `@src/client/components/PatternPreview.ts`:
- Around line 17-22: The PatternPreview component currently renders an <img>
with src set from generatePreviewDataUrl(pattern, width, height) even when that
helper returns an empty string; change PatternPreview to check the returned
dataUrl first and only render the <img> when dataUrl is non-empty, otherwise
render the existing blank preview placeholder (the same element shown when no
pattern exists) so you avoid creating a broken <img src="">; locate this logic
in PatternPreview and use generatePreviewDataUrl(...) result to branch the
render.
- Around line 100-104: In the canvas setup in PatternPreview.ts where you call
canvas.getContext("2d") and assign to ctx, avoid throwing on a null 2D context;
instead handle it like other recoverable preview errors by returning the safe
failure result (an empty string) from the surrounding helper (the function that
produces the preview image) rather than throwing; locate the canvas creation and
the ctx null check (the canvas.getContext("2d") call and the if (!ctx) branch)
and replace the throw new Error(...) with code that returns "" so tests and
non-browser environments don't crash.
In `@src/client/Cosmetics.ts`:
- Around line 232-244: The current code pushes a colored variant even when the
palette lookup fails (cp exists but cosmetics.colorPalettes?.[cp.name] is
undefined), resulting in colorPalette: null; update the logic around
cp/resolvedPalette so that you only push the pattern entry when either cp is
null (base variant) or when cp is present AND resolvedPalette is non-null;
reference the variables cp, resolvedPalette, cosmetics.colorPalettes, pattern,
patternKey and the result.push call — skip emitting the colored variant if
resolvedPalette is null to avoid downstream preview/checkout issues.
🪄 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: 6ebe7bab-7a02-49dd-a386-2b403f1633a2
📒 Files selected for processing (17)
src/client/AccountModal.tssrc/client/Cosmetics.tssrc/client/FlagInputModal.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/TokenLoginModal.tssrc/client/components/CosmeticButton.tssrc/client/components/CosmeticInfo.tssrc/client/components/FlagButton.tssrc/client/components/PatternButton.tssrc/client/components/PatternPreview.tssrc/client/graphics/layers/WinModal.tssrc/core/CosmeticSchemas.tstests/Privilege.test.tstests/ResolveCosmetics.test.ts
💤 Files with no reviewable changes (5)
- src/client/TokenLoginModal.ts
- src/client/Matchmaking.ts
- src/client/AccountModal.ts
- src/client/components/FlagButton.ts
- src/client/components/PatternButton.ts
✅ Files skipped from review due to trivial changes (4)
- tests/Privilege.test.ts
- src/core/CosmeticSchemas.ts
- src/client/PatternInput.ts
- tests/ResolveCosmetics.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/components/CosmeticInfo.ts
Comment on lines
+29
to
+31
| private handleClick() { | ||
| this.onSelect?.(this.resolved); | ||
| } |
Contributor
There was a problem hiding this comment.
Route clicks by relationship, not always through selection.
Right now a normal click always calls onSelect. In the store, that means a purchasable cosmetic can be “selected” and close the modal instead of starting checkout. This component already has resolved.relationship, so it should enforce the split here.
Proposed fix
private handleClick() {
- this.onSelect?.(this.resolved);
+ if (this.resolved.relationship === "purchasable") {
+ this.onPurchase?.(this.resolved);
+ return;
+ }
+ this.onSelect?.(this.resolved);
}📝 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
| private handleClick() { | |
| this.onSelect?.(this.resolved); | |
| } | |
| private handleClick() { | |
| if (this.resolved.relationship === "purchasable") { | |
| this.onPurchase?.(this.resolved); | |
| return; | |
| } | |
| this.onSelect?.(this.resolved); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/components/CosmeticButton.ts` around lines 29 - 31, handleClick
currently always calls onSelect(this.resolved) which routes purchasable items
into selection; change handleClick to branch on this.resolved.relationship: when
relationship === 'purchasable' do not call onSelect but instead invoke a
dedicated purchase handler (e.g., call a new prop like onPurchase or
onStartCheckout with this.resolved) to start checkout, and when relationship is
anything else call this.onSelect?.(this.resolved); update the component props to
accept and use that new purchase handler (and ensure callers wired to start
checkout in the store).
947a3f9 to
2273837
Compare
evanpelle
force-pushed
the
main-cosmetics-refactor
branch
from
2273837 to
ff7b9a2
Compare
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/Cosmetics.ts (1)
232-244:⚠️ Potential issue | 🟠 MajorDo not emit a keyed palette variant when the palette lookup fails.
If
cpexists butcosmetics.colorPalettes[cp.name]is missing, this still returnspattern:${patternKey}:${cp.name}withcolorPalette: null. Callers then preview it without colors and passundefinedforcolorPaletteNameat checkout, which can target the wrong variant.🛠️ Proposed fix
const resolvedPalette = cp - ? (cosmetics.colorPalettes?.[cp.name] ?? null) + ? cosmetics.colorPalettes?.[cp.name] : null; + if (cp && !resolvedPalette) { + console.warn( + `Missing color palette "${cp.name}" for pattern "${patternKey}"`, + ); + continue; + } const key = cp ? `pattern:${patternKey}:${cp.name}` : `pattern:${patternKey}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Cosmetics.ts` around lines 232 - 244, The code currently builds a keyed variant even when the palette lookup fails (cp exists but cosmetics.colorPalettes[cp.name] is missing); change the emission logic so you only produce a keyed variant when the lookup succeeds. In the block that computes resolvedPalette and key (using cp, resolvedPalette, cosmetics.colorPalettes, patternKey and result.push), if cp is truthy but resolvedPalette is null/undefined, fall back to the unkeyed form (`pattern:${patternKey}`) (or skip emitting a palette-specific entry) and ensure colorPalette remains unset/null; only use the `pattern:${patternKey}:${cp.name}` key when resolvedPalette is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/components/CosmeticButton.ts`:
- Around line 65-70: In CosmeticButton's image error handler (the `@error`
callback), replace the hardcoded fallback path "/flags/xx.svg" with the result
of calling assetUrl('flags/xx.svg') so the fallback uses the same asset base
resolution as the primary flag; update the handler that currently uses img.src =
fallback to compute const fallback = assetUrl('flags/xx.svg') and assign that
value, ensuring the error handler references the same assetUrl function used
elsewhere in the component.
---
Duplicate comments:
In `@src/client/Cosmetics.ts`:
- Around line 232-244: The code currently builds a keyed variant even when the
palette lookup fails (cp exists but cosmetics.colorPalettes[cp.name] is
missing); change the emission logic so you only produce a keyed variant when the
lookup succeeds. In the block that computes resolvedPalette and key (using cp,
resolvedPalette, cosmetics.colorPalettes, patternKey and result.push), if cp is
truthy but resolvedPalette is null/undefined, fall back to the unkeyed form
(`pattern:${patternKey}`) (or skip emitting a palette-specific entry) and ensure
colorPalette remains unset/null; only use the `pattern:${patternKey}:${cp.name}`
key when resolvedPalette is present.
🪄 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: 828fa498-d3fc-4e2c-ad11-b7a7ed787a78
📒 Files selected for processing (17)
src/client/AccountModal.tssrc/client/Cosmetics.tssrc/client/FlagInputModal.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/TokenLoginModal.tssrc/client/components/CosmeticButton.tssrc/client/components/CosmeticInfo.tssrc/client/components/FlagButton.tssrc/client/components/PatternButton.tssrc/client/components/PatternPreview.tssrc/client/graphics/layers/WinModal.tssrc/core/CosmeticSchemas.tstests/Privilege.test.tstests/ResolveCosmetics.test.ts
💤 Files with no reviewable changes (5)
- src/client/TokenLoginModal.ts
- src/client/Matchmaking.ts
- src/client/AccountModal.ts
- src/client/components/FlagButton.ts
- src/client/components/PatternButton.ts
✅ Files skipped from review due to trivial changes (3)
- tests/Privilege.test.ts
- src/core/CosmeticSchemas.ts
- src/client/PatternInput.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/components/CosmeticInfo.ts
- src/client/TerritoryPatternsModal.ts
- src/client/components/PatternPreview.ts
Comment on lines
+65
to
+70
| @error=${(e: Event) => { | ||
| const img = e.currentTarget as HTMLImageElement; | ||
| const fallback = "/flags/xx.svg"; | ||
| if (img.src && !img.src.endsWith(fallback)) { | ||
| img.src = fallback; | ||
| } |
Contributor
There was a problem hiding this comment.
Use assetUrl() for the fallback flag image too.
The main flag URL can come from assetUrl(...), but the error handler falls back to a raw root path. On non-root deployments the first URL can work while the fallback still 404s.
🛠️ Proposed fix
+import { assetUrl } from "../../core/AssetUrls";
import { Flag, Pattern } from "../../core/CosmeticSchemas";
...
- const fallback = "/flags/xx.svg";
+ const fallback = assetUrl("flags/xx.svg");📝 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
| @error=${(e: Event) => { | |
| const img = e.currentTarget as HTMLImageElement; | |
| const fallback = "/flags/xx.svg"; | |
| if (img.src && !img.src.endsWith(fallback)) { | |
| img.src = fallback; | |
| } | |
| `@error`=${(e: Event) => { | |
| const img = e.currentTarget as HTMLImageElement; | |
| const fallback = assetUrl("flags/xx.svg"); | |
| if (img.src && !img.src.endsWith(fallback)) { | |
| img.src = fallback; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/components/CosmeticButton.ts` around lines 65 - 70, In
CosmeticButton's image error handler (the `@error` callback), replace the
hardcoded fallback path "/flags/xx.svg" with the result of calling
assetUrl('flags/xx.svg') so the fallback uses the same asset base resolution as
the primary flag; update the handler that currently uses img.src = fallback to
compute const fallback = assetUrl('flags/xx.svg') and assign that value,
ensuring the error handler references the same assetUrl function used elsewhere
in the component.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/client/components/PatternPreview.ts (2)
17-22:⚠️ Potential issue | 🟡 MinorHandle preview generation failure before rendering
<img>.
generatePreviewDataUrl(...)can return""(Line 85). Renderingsrc=""still creates a broken image path; fallback torenderBlankPreview()instead.Suggested fix
export function renderPatternPreview( pattern: PlayerPattern | null, width: number, height: number, ): TemplateResult { if (pattern === null) { return renderBlankPreview(); } + const src = generatePreviewDataUrl(pattern, width, height); + if (!src) { + return renderBlankPreview(); + } return html`<img - src="${generatePreviewDataUrl(pattern, width, height)}" + src="${src}" alt="Pattern preview" class="w-full h-full object-contain [image-rendering:pixelated] pointer-events-none" draggable="false" />`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/PatternPreview.ts` around lines 17 - 22, In PatternPreview, avoid rendering an <img> with an empty src by calling generatePreviewDataUrl(pattern, width, height) into a variable and, if it returns an empty string, call renderBlankPreview() instead of rendering the <img>; otherwise render the <img> with the generated data URL. Update the render path in the component (where generatePreviewDataUrl is currently inlined) to branch on the returned string and use renderBlankPreview() as the fallback.
100-104:⚠️ Potential issue | 🟡 MinorAvoid throwing when 2D context is unavailable.
Returning
""matches your existing recoverable error path and avoids hard crashes in unsupported environments.Suggested fix
const ctx = canvas.getContext("2d"); - if (!ctx) throw new Error("2D context not supported"); + if (!ctx) { + console.error("2D context not supported"); + return ""; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/PatternPreview.ts` around lines 100 - 104, In the PatternPreview rendering code where you create a canvas and call getContext("2d") (variables canvas, canvas.width/height, ctx), avoid throwing when ctx is null; instead follow the existing recoverable error path and return an empty string ("") so unsupported environments don't crash—replace the `if (!ctx) throw new Error("2D context not supported")` with logic that returns "" (and optionally logs a debug message) from the surrounding function.src/client/components/CosmeticButton.ts (2)
29-31:⚠️ Potential issue | 🟠 MajorRoute click by
relationshipinstead of always selecting.Right now a normal click always calls
onSelect, so purchasable cosmetics do not reliably start checkout from button click.Suggested fix
private handleClick() { - this.onSelect?.(this.resolved); + if (this.resolved.relationship === "purchasable") { + this.onPurchase?.(this.resolved); + return; + } + this.onSelect?.(this.resolved); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/CosmeticButton.ts` around lines 29 - 31, The click handler always calls onSelect(this.resolved); change handleClick to route based on this.resolved.relationship: if relationship === 'purchasable' call a purchase/checkout handler (e.g. this.onPurchase?.(this.resolved) or this.onStartCheckout?.(this.resolved)), otherwise call this.onSelect?.(this.resolved); update the component props to accept the new handler name you choose and ensure existing purchasers use it.
65-70:⚠️ Potential issue | 🟡 MinorUse
assetUrl()for the fallback flag path.Hardcoding
"/flags/xx.svg"can break when assets are served from a non-root base path.Suggested fix
+import { assetUrl } from "../../core/AssetUrls"; import { Flag, Pattern } from "../../core/CosmeticSchemas"; ... - const fallback = "/flags/xx.svg"; + const fallback = assetUrl("flags/xx.svg");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/CosmeticButton.ts` around lines 65 - 70, In CosmeticButton's image error handler replace the hardcoded fallback "/flags/xx.svg" with the asset helper so the path respects the app base: set img.src = assetUrl('/flags/xx.svg') (and add the corresponding import for assetUrl from your project's asset helper/Lit static helper). Update the `@error` handler code in CosmeticButton.ts to use assetUrl('/flags/xx.svg') instead of the literal string and ensure the assetUrl import is added at the top of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/components/PatternPreview.ts`:
- Around line 88-89: PatternPreview currently calls decoder.scaledWidth() and
decoder.scaledHeight(), but PatternDecoder exposes width, height and scale
fields; replace those calls by computing scaled sizes from decoder.width,
decoder.height and decoder.scale (e.g. scaledWidth = Math.round(decoder.width *
decoder.scale) and scaledHeight = Math.round(decoder.height * decoder.scale))
and update any downstream uses to accept these numeric values instead of method
calls (references: PatternPreview's use of
decoder.scaledWidth/decoder.scaledHeight and PatternDecoder's width/height/scale
fields).
---
Duplicate comments:
In `@src/client/components/CosmeticButton.ts`:
- Around line 29-31: The click handler always calls onSelect(this.resolved);
change handleClick to route based on this.resolved.relationship: if relationship
=== 'purchasable' call a purchase/checkout handler (e.g.
this.onPurchase?.(this.resolved) or this.onStartCheckout?.(this.resolved)),
otherwise call this.onSelect?.(this.resolved); update the component props to
accept the new handler name you choose and ensure existing purchasers use it.
- Around line 65-70: In CosmeticButton's image error handler replace the
hardcoded fallback "/flags/xx.svg" with the asset helper so the path respects
the app base: set img.src = assetUrl('/flags/xx.svg') (and add the corresponding
import for assetUrl from your project's asset helper/Lit static helper). Update
the `@error` handler code in CosmeticButton.ts to use assetUrl('/flags/xx.svg')
instead of the literal string and ensure the assetUrl import is added at the top
of the file.
In `@src/client/components/PatternPreview.ts`:
- Around line 17-22: In PatternPreview, avoid rendering an <img> with an empty
src by calling generatePreviewDataUrl(pattern, width, height) into a variable
and, if it returns an empty string, call renderBlankPreview() instead of
rendering the <img>; otherwise render the <img> with the generated data URL.
Update the render path in the component (where generatePreviewDataUrl is
currently inlined) to branch on the returned string and use renderBlankPreview()
as the fallback.
- Around line 100-104: In the PatternPreview rendering code where you create a
canvas and call getContext("2d") (variables canvas, canvas.width/height, ctx),
avoid throwing when ctx is null; instead follow the existing recoverable error
path and return an empty string ("") so unsupported environments don't
crash—replace the `if (!ctx) throw new Error("2D context not supported")` with
logic that returns "" (and optionally logs a debug message) from the surrounding
function.
🪄 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: 845fcbf5-2e11-4067-a33a-ed48cdd33b1e
📒 Files selected for processing (16)
src/client/AccountModal.tssrc/client/Cosmetics.tssrc/client/FlagInputModal.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/TokenLoginModal.tssrc/client/components/CosmeticButton.tssrc/client/components/CosmeticInfo.tssrc/client/components/FlagButton.tssrc/client/components/PatternButton.tssrc/client/components/PatternPreview.tssrc/client/graphics/layers/WinModal.tstests/Privilege.test.tstests/ResolveCosmetics.test.ts
💤 Files with no reviewable changes (5)
- src/client/Matchmaking.ts
- src/client/TokenLoginModal.ts
- src/client/AccountModal.ts
- src/client/components/FlagButton.ts
- src/client/components/PatternButton.ts
✅ Files skipped from review due to trivial changes (2)
- tests/Privilege.test.ts
- src/client/PatternInput.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/components/CosmeticInfo.ts
- src/client/TerritoryPatternsModal.ts
- src/client/Store.ts
Comment on lines
+88
to
+89
| const scaledWidth = decoder.scaledWidth(); | ||
| const scaledHeight = decoder.scaledHeight(); |
Contributor
There was a problem hiding this comment.
Fix scaled size computation; PatternDecoder does not expose these methods.
This code calls decoder.scaledWidth() / decoder.scaledHeight(), but src/core/PatternDecoder.ts exposes width, height, and scale fields. This is a blocker if left as-is.
Suggested fix
- const scaledWidth = decoder.scaledWidth();
- const scaledHeight = decoder.scaledHeight();
+ const scaledWidth = decoder.width << decoder.scale;
+ const scaledHeight = decoder.height << decoder.scale;📝 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
| const scaledWidth = decoder.scaledWidth(); | |
| const scaledHeight = decoder.scaledHeight(); | |
| const scaledWidth = decoder.width << decoder.scale; | |
| const scaledHeight = decoder.height << decoder.scale; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/components/PatternPreview.ts` around lines 88 - 89, PatternPreview
currently calls decoder.scaledWidth() and decoder.scaledHeight(), but
PatternDecoder exposes width, height and scale fields; replace those calls by
computing scaled sizes from decoder.width, decoder.height and decoder.scale
(e.g. scaledWidth = Math.round(decoder.width * decoder.scale) and scaledHeight =
Math.round(decoder.height * decoder.scale)) and update any downstream uses to
accept these numeric values instead of method calls (references:
PatternPreview's use of decoder.scaledWidth/decoder.scaledHeight and
PatternDecoder's width/height/scale fields).
4 tasks
4 tasks
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.
Description:
The motivation is to have a single "cosmetic-button" element, so we can abstract out the cosmetic types. This will make it much easier to add new cosmetic types in the future.
Unifies PatternButton and FlagButton into a single CosmeticButton component. Extracts a resolveCosmetics() function that flattens patterns × color palettes + flags into a ResolvedCosmetic[] with relationship status pre-computed, replacing duplicated resolution logic across four callers.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan