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:
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Store as StoreModal
participant API as ClientAPI
participant Server as AppServer
participant Stripe as Stripe
User->>Store: open(options?) / select item
Store->>API: createCheckoutSession(priceId, colorPaletteName?)
API->>Server: POST /stripe/create-checkout-session { priceId, colorPaletteName? }
Server->>Stripe: create session with priceId
Stripe-->>Server: session.url
Server-->>API: { sessionUrl }
API-->>Store: sessionUrl
Store->>User: redirect to sessionUrl
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/FlagInput.ts (1)
106-144:⚠️ Potential issue | 🟠 MajorRace condition risk:
async updated()can cause stale renders.Lit does not await lifecycle methods. If
this.flagchanges rapidly, multiplefetchCosmetics()calls may resolve out of order, displaying the wrong flag. Additionally, callingfetchCosmetics()on every update cycle is inefficient.Consider:
- Track a request token to discard stale responses.
- Cache the cosmetics fetch result (e.g., in
connectedCallbackor a reactive controller).🛠️ Suggested approach with stale-response guard
+ private _updateToken = 0; + async updated() { + const token = ++this._updateToken; const preview = this.renderRoot.querySelector( "#flag-preview", ) as HTMLElement; if (!preview) return; if (this.showSelectLabel && this.isDefaultFlagValue(this.flag)) { preview.innerHTML = ""; return; } preview.innerHTML = ""; if (this.flag?.startsWith("flag:")) { const key = this.flag.slice("flag:".length); const cosmetics = await fetchCosmetics(); + if (token !== this._updateToken) return; // stale response const flagData = cosmetics?.flags?.[key]; // ... rest unchanged } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/FlagInput.ts` around lines 106 - 144, The updated() method can show stale flag images because fetchCosmetics() is called on every update and responses may arrive out of order; fix by caching the cosmetics result on the component (e.g., this._cosmetics) and initiating fetch once in connectedCallback (or only when _cosmetics is undefined), and add a per-request token/nonce in updated() (e.g., const req = ++this._cosmeticsRequestId) before awaiting fetchCosmetics(); after the await, verify the token still matches (if (req !== this._cosmeticsRequestId) return) before using flagData to render the image, and keep existing showSelectLabel / isDefaultFlagValue checks unchanged.
🧹 Nitpick comments (3)
src/core/CustomFlag.ts (1)
3-11: Consider adding a one-time deprecation warning for this no-op stub.Right now, stale callers fail silently. A one-time warning would help clean up remaining call sites safely.
♻️ Proposed change
+let warnedLegacyRenderPlayerFlag = false; + export function renderPlayerFlag( flag: string, target: HTMLElement, cosmetics: Cosmetics | undefined = undefined, ) { - // Old custom flags are no longer supported + // Old custom flags are no longer supported + if (!warnedLegacyRenderPlayerFlag) { + warnedLegacyRenderPlayerFlag = true; + console.warn("renderPlayerFlag is deprecated and now a no-op."); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/CustomFlag.ts` around lines 3 - 11, Add a one-time deprecation warning to the no-op renderPlayerFlag stub: declare a module-level boolean (e.g., warnedRenderPlayerFlag = false) and inside export function renderPlayerFlag(...) check if !warnedRenderPlayerFlag then call console.warn (or your app logger) with a concise message like "renderPlayerFlag is deprecated and now a no-op; remove callers" and set warnedRenderPlayerFlag = true; keep the function behavior otherwise unchanged so existing callers still no-op but emit the warning only once.src/client/FlagInputModal.ts (1)
33-37: Search usesflag.namefor both name and code fields.For cosmetic flags,
includedInSearchreceives{ name: flag.name, code: flag.name }. Users cannot search by the flag's key, only by display name. If this is intentional, this is fine. If users should be able to search by key, pass the key ascode.♻️ Optional: allow search by key
.filter(([key, flag]) => { - if (!this.includedInSearch({ name: flag.name, code: flag.name })) + if (!this.includedInSearch({ name: flag.name, code: key })) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/FlagInputModal.ts` around lines 33 - 37, The search call for cosmeticFlags incorrectly uses the flag display name for both name and code: change the includedInSearch invocation inside the cosmeticFlags computation to pass the entry key as code (use the Object.entries key variable from the [key, flag] tuple) so includedInSearch receives { name: flag.name, code: key }—locate the cosmeticFlags declaration in FlagInputModal.ts and update that object shape accordingly while keeping the rest of the filter (including flagRelationship(flag, this.userMe, null) === "owned") unchanged.src/client/Main.ts (1)
365-368: Remove duplicate store modal check.The same
instanceof StoreModalcheck and warning appears twice (lines 365-368 and 387-389). This looks like leftover code from refactoring.♻️ Suggested fix: remove duplicate block
this.storeModal = document.getElementById("page-item-store") as StoreModal; if (!this.storeModal || !(this.storeModal instanceof StoreModal)) { console.warn("Store modal element not found"); } // Attach listener to any pattern-input component document.querySelectorAll("pattern-input").forEach((patternInput) => { patternInput.addEventListener("pattern-input-click", () => { const patternsModal = document.getElementById( "territory-patterns-modal", ) as HTMLElement & { open?: () => void }; if (patternsModal && typeof patternsModal.open === "function") { patternsModal.open(); } }); }); if (isInIframe()) { const mobilePat = document.getElementById("pattern-input-mobile"); if (mobilePat) mobilePat.style.display = "none"; } - if (!this.storeModal || !(this.storeModal instanceof StoreModal)) { - console.warn("Store modal element not found"); - } - // We no longer need to manually manage...Also applies to: 387-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Main.ts` around lines 365 - 368, Remove the duplicated runtime check and warning for the store modal: keep a single assignment and instanceof verification for this.storeModal (the existing document.getElementById("page-item-store") as StoreModal and its instanceof StoreModal check) and delete the redundant block that repeats the same instanceof StoreModal check and console.warn; ensure only one conditional that logs "Store modal element not found" remains and all other usages rely on that single verified this.storeModal.
🤖 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/FlagButton.ts`:
- Around line 81-84: In the FlagButton component, the decorative flag image
currently uses alt=${this.flag.name} which exposes the raw slug to assistive
tech; update the <img> usage (referenced by this.flag.url and this.flag.name) to
either set alt="" and add aria-hidden="true" for a purely decorative image, or
replace the alt value with the translated/visible button label you already
render so the accessible name doesn’t include the raw slug.
In `@src/client/components/PurchaseButton.ts`:
- Around line 8-9: The component's non-null assertion on the product property
(product!: Product) can cause runtime errors when render accesses
this.product.price before the property is set; update the PurchaseButton
component to guard against undefined product in render and any methods that read
this.product (e.g., wherever this.product.price or this.product.id is used) by
returning a fallback UI or disabling the button when product is falsy, and
ensure event handlers (like the purchase click handler) similarly check for
product before proceeding.
In `@src/client/Cosmetics.ts`:
- Around line 200-219: The code currently clears localStorage when flagData is
undefined, but cosmetics can be null on fetch failure; update the logic in the
block using userSettings.getFlag(), cosmetics?.flags, and flagData so you only
remove the saved "flag" after a successful cosmetics load proves the key is
invalid: first ensure cosmetics (or cosmetics.flags) is present before treating
missing key as invalid, i.e. only consider flagData undefined as a reason to
clear localStorage when cosmetics is non-null and the key is absent; keep the
existing userMe/flares checks but guard the localStorage.removeItem("flag") call
behind a check that cosmetics was loaded successfully.
In `@src/client/graphics/layers/NameLayer.ts`:
- Around line 217-221: The flag string can contain prefixes like "flag:" which
produce broken paths when concatenated as "/flags/" + flag + ".svg"; create a
small reusable helper (e.g., getFlagUrl(flag: string): string) that normalizes
the flag value by stripping known prefixes such as "flag:" or "emoji:" and
returns either the original URL if flag.includes("/") or the normalized
"/flags/{name}.svg" path, then replace the inline logic in NameLayer (around
flagImg.src assignment) with a call to getFlagUrl(flag) and reuse that helper
from PlayerInfoOverlay as suggested.
In `@src/client/Store.ts`:
- Around line 337-353: In selectFlag, replace the toast message that currently
uses the storage key/name with the cosmetic display name: extract the raw key
(strip "flag:" prefix as already done into nameKey), then look up
this.cosmetics?.flags[nameKey]?.name (or call the same display-name helper used
by flag-button) and use that value in the message instead of the storage key;
fall back to the existing nameKey if no cosmetic entry exists, and keep using
translateText("territory_patterns.selected") and the same CustomEvent payload
and duration.
- Around line 41-55: connectedCallback registers an anonymous listener for
"userMeResponse" so disconnectedCallback cannot remove it; change this by
extracting the handler into a stable class property (e.g.,
this._onUserMeResponse or a bound method) and use that same reference when
calling document.addEventListener in connectedCallback and
document.removeEventListener in disconnectedCallback, ensuring
onUserMe(event.detail) is invoked from that handler and preventing stacked
handlers on reconnect.
In `@src/core/CosmeticSchemas.ts`:
- Line 79: The new required "flags" property in the Zod schema (flags:
z.record(z.string(), FlagSchema)) causes CosmeticsSchema.safeParse() to fail
because resources/cosmetics/cosmetics.json lacks that key; update the schema in
CosmeticSchemas.ts to make flags optional (e.g., change the field to use
.optional() such as flags: z.record(z.string(), FlagSchema).optional()) so
existing JSON parses successfully, referencing FlagSchema and
CosmeticsSchema.safeParse to locate where to change the validation;
alternatively, add a default empty record for flags if you prefer a non-optional
shape.
In `@src/core/Schemas.ts`:
- Around line 477-484: The validation refine on the schema currently allows
values that start with "flag:" but the error message still says "start with !";
update the message in the refine call (the object with key message) to reflect
the new accepted prefix—e.g., "Invalid flag: must be a valid country code or
start with 'flag:'"—so it matches the logic that checks val.startsWith("flag:")
and countryCodes.
In `@src/server/Privilege.ts`:
- Around line 218-228: The isFlagAllowed method uses the dictionary key (key) to
lookup the flag in this.cosmetics.flags but then checks flares against
found.name, causing mismatches; change the flare membership check to use the
same key used for lookup (e.g., flares.includes(`flag:${key}`) or include both
checks if needed) so flares are matched consistently, then return found.url as
before and keep the same error branches.
---
Outside diff comments:
In `@src/client/FlagInput.ts`:
- Around line 106-144: The updated() method can show stale flag images because
fetchCosmetics() is called on every update and responses may arrive out of
order; fix by caching the cosmetics result on the component (e.g.,
this._cosmetics) and initiating fetch once in connectedCallback (or only when
_cosmetics is undefined), and add a per-request token/nonce in updated() (e.g.,
const req = ++this._cosmeticsRequestId) before awaiting fetchCosmetics(); after
the await, verify the token still matches (if (req !== this._cosmeticsRequestId)
return) before using flagData to render the image, and keep existing
showSelectLabel / isDefaultFlagValue checks unchanged.
---
Nitpick comments:
In `@src/client/FlagInputModal.ts`:
- Around line 33-37: The search call for cosmeticFlags incorrectly uses the flag
display name for both name and code: change the includedInSearch invocation
inside the cosmeticFlags computation to pass the entry key as code (use the
Object.entries key variable from the [key, flag] tuple) so includedInSearch
receives { name: flag.name, code: key }—locate the cosmeticFlags declaration in
FlagInputModal.ts and update that object shape accordingly while keeping the
rest of the filter (including flagRelationship(flag, this.userMe, null) ===
"owned") unchanged.
In `@src/client/Main.ts`:
- Around line 365-368: Remove the duplicated runtime check and warning for the
store modal: keep a single assignment and instanceof verification for
this.storeModal (the existing document.getElementById("page-item-store") as
StoreModal and its instanceof StoreModal check) and delete the redundant block
that repeats the same instanceof StoreModal check and console.warn; ensure only
one conditional that logs "Store modal element not found" remains and all other
usages rely on that single verified this.storeModal.
In `@src/core/CustomFlag.ts`:
- Around line 3-11: Add a one-time deprecation warning to the no-op
renderPlayerFlag stub: declare a module-level boolean (e.g.,
warnedRenderPlayerFlag = false) and inside export function renderPlayerFlag(...)
check if !warnedRenderPlayerFlag then call console.warn (or your app logger)
with a concise message like "renderPlayerFlag is deprecated and now a no-op;
remove callers" and set warnedRenderPlayerFlag = true; keep the function
behavior otherwise unchanged so existing callers still no-op but emit the
warning only once.
🪄 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: 6ceaadfe-248b-4627-bee7-85c6b895fa71
📒 Files selected for processing (20)
index.htmlresources/lang/en.jsonsrc/client/Api.tssrc/client/Cosmetics.tssrc/client/FlagInput.tssrc/client/FlagInputModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/components/FlagButton.tssrc/client/components/PatternButton.tssrc/client/components/PurchaseButton.tssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/CosmeticSchemas.tssrc/core/CustomFlag.tssrc/core/Schemas.tssrc/server/Privilege.tstests/Privilege.test.ts
Comment on lines
+81
to
+84
| <img | ||
| src=${this.flag.url} | ||
| alt=${this.flag.name} | ||
| class="w-full h-full object-contain pointer-events-none" |
Contributor
There was a problem hiding this comment.
Avoid exposing the raw flag slug to assistive tech.
The button already has a text label. alt=${this.flag.name} adds the untranslated slug to the accessible name, so screen readers can announce values like ancient_rome. Use alt="" / aria-hidden="true" for a decorative image, or reuse the translated label here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/components/FlagButton.ts` around lines 81 - 84, In the FlagButton
component, the decorative flag image currently uses alt=${this.flag.name} which
exposes the raw slug to assistive tech; update the <img> usage (referenced by
this.flag.url and this.flag.name) to either set alt="" and add
aria-hidden="true" for a purely decorative image, or replace the alt value with
the translated/visible button label you already render so the accessible name
doesn’t include the raw slug.
Comment on lines
+8
to
+9
| @property({ type: Object }) | ||
| product!: Product; |
Contributor
There was a problem hiding this comment.
Add a guard for undefined product to prevent runtime error.
The product! assertion assumes product is always set before render. If the component renders before the property is assigned, accessing this.product.price will throw.
🛡️ Suggested fix
render() {
+ if (!this.product) return html``;
return html`
<div class="w-full mt-2">Also applies to: 31-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/components/PurchaseButton.ts` around lines 8 - 9, The component's
non-null assertion on the product property (product!: Product) can cause runtime
errors when render accesses this.product.price before the property is set;
update the PurchaseButton component to guard against undefined product in render
and any methods that read this.product (e.g., wherever this.product.price or
this.product.id is used) by returning a fallback UI or disabling the button when
product is falsy, and ensure event handlers (like the purchase click handler)
similarly check for product before proceeding.
Comment on lines
+337
to
+353
| private selectFlag(flagKey: string) { | ||
| localStorage.setItem("flag", flagKey); | ||
| this.dispatchEvent( | ||
| new CustomEvent("flag-change", { | ||
| detail: { flag: flagKey }, | ||
| bubbles: true, | ||
| composed: true, | ||
| }), | ||
| ); | ||
| const name = flagKey.startsWith("flag:") | ||
| ? flagKey.slice("flag:".length) | ||
| : flagKey; | ||
| window.dispatchEvent( | ||
| new CustomEvent("show-message", { | ||
| detail: { | ||
| message: `${name} ${translateText("territory_patterns.selected")}`, | ||
| duration: 2000, |
Contributor
There was a problem hiding this comment.
Use the cosmetic name, not the storage key, in the flag toast.
This message is built from flag:${key} storage data, so users can see ancient_rome selected even when the card label is different and translated. Look up this.cosmetics?.flags[key]?.name and reuse the same display-name helper as flag-button before showing the toast.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/Store.ts` around lines 337 - 353, In selectFlag, replace the toast
message that currently uses the storage key/name with the cosmetic display name:
extract the raw key (strip "flag:" prefix as already done into nameKey), then
look up this.cosmetics?.flags[nameKey]?.name (or call the same display-name
helper used by flag-button) and use that value in the message instead of the
storage key; fall back to the existing nameKey if no cosmetic entry exists, and
keep using translateText("territory_patterns.selected") and the same CustomEvent
payload and duration.
| ), | ||
| }) | ||
| .optional(), | ||
| flags: z.record(z.string(), FlagSchema), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json
from pathlib import Path
for path in Path(".").rglob("cosmetics.json"):
try:
data = json.loads(path.read_text())
except Exception as exc:
print(f"{path}: invalid json ({exc})")
continue
print(
f"{path}: flags_key_present={'flags' in data}, "
f"flags_is_object={isinstance(data.get('flags'), dict)}"
)
PYRepository: openfrontio/OpenFrontIO
Length of output: 149
🏁 Script executed:
sed -n '70,90p' src/core/CosmeticSchemas.tsRepository: openfrontio/OpenFrontIO
Length of output: 555
🏁 Script executed:
cat -n src/client/Cosmetics.ts | head -50Repository: openfrontio/OpenFrontIO
Length of output: 1630
🏁 Script executed:
cat -n src/client/Cosmetics.ts | grep -A 10 "CosmeticsSchema.safeParse"Repository: openfrontio/OpenFrontIO
Length of output: 611
🏁 Script executed:
rg "CosmeticsSchema" -B 2 -A 5Repository: openfrontio/OpenFrontIO
Length of output: 4937
Schema validation breaks all cosmetics because resources/cosmetics/cosmetics.json does not include the new required flags field.
Line 79 makes flags required, but the shipped cosmetics.json file does not have this field. When the client calls CosmeticsSchema.safeParse(), validation fails and returns null at line 68 in src/client/Cosmetics.ts, disabling all cosmetics (patterns, flags, color palettes). The server in PrivilegeRefresher.ts also fails with the same validation error. Either add flags to resources/cosmetics/cosmetics.json or make the field optional with .optional().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/CosmeticSchemas.ts` at line 79, The new required "flags" property in
the Zod schema (flags: z.record(z.string(), FlagSchema)) causes
CosmeticsSchema.safeParse() to fail because resources/cosmetics/cosmetics.json
lacks that key; update the schema in CosmeticSchemas.ts to make flags optional
(e.g., change the field to use .optional() such as flags: z.record(z.string(),
FlagSchema).optional()) so existing JSON parses successfully, referencing
FlagSchema and CosmeticsSchema.safeParse to locate where to change the
validation; alternatively, add a default empty record for flags if you prefer a
non-optional shape.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
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/client/Main.ts (1)
365-389:⚠️ Potential issue | 🟡 MinorDuplicate warning check for store modal.
The same
instanceof StoreModalcheck and warning appears twice (lines 366-368 and 387-389). Remove the duplicate.Proposed fix
this.storeModal = document.getElementById("page-item-store") as StoreModal; if (!this.storeModal || !(this.storeModal instanceof StoreModal)) { console.warn("Store modal element not found"); } // Attach listener to any pattern-input component document.querySelectorAll("pattern-input").forEach((patternInput) => { patternInput.addEventListener("pattern-input-click", () => { const patternsModal = document.getElementById( "territory-patterns-modal", ) as HTMLElement & { open?: () => void }; if (patternsModal && typeof patternsModal.open === "function") { patternsModal.open(); } }); }); if (isInIframe()) { const mobilePat = document.getElementById("pattern-input-mobile"); if (mobilePat) mobilePat.style.display = "none"; } - if (!this.storeModal || !(this.storeModal instanceof StoreModal)) { - console.warn("Store modal element not found"); - } - // We no longer need to manually manage the preview button as PatternInput handles it component-side.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Main.ts` around lines 365 - 389, Remove the duplicated store modal existence/type check: keep a single check of this.storeModal (assigned from document.getElementById("page-item-store")) against instanceof StoreModal and remove the second identical block; ensure the remaining check still logs the same warning ("Store modal element not found") and that any subsequent code uses this.storeModal as before.
♻️ Duplicate comments (3)
src/server/Privilege.ts (1)
218-228:⚠️ Potential issue | 🟡 MinorKey vs name mismatch in flare check still present.
The lookup uses
key(extracted fromflagRef), but the flare membership check usesfound.name. Ifcosmetics.flags["us"]hasname: "united_states", then a flare"flag:us"won't match"flag:united_states".Consider checking both or using the same identifier consistently:
🔧 Proposed fix
isFlagAllowed(flares: string[], flagRef: string): string { const key = flagRef.slice("flag:".length); const found = this.cosmetics.flags[key]; if (!found) throw new Error(`Flag ${key} not found`); - if (flares.includes("flag:*") || flares.includes(`flag:${found.name}`)) { + if (flares.includes("flag:*") || flares.includes(`flag:${key}`) || flares.includes(`flag:${found.name}`)) { return found.url; } throw new Error(`No flares for flag ${key}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/Privilege.ts` around lines 218 - 228, In isFlagAllowed, the flare membership check compares flares against found.name but the lookup used key (from flagRef), causing mismatches; update the check in isFlagAllowed to accept either identifier by testing flares.includes(`flag:${key}`) OR flares.includes(`flag:${found.name}`) (and still allow the wildcard `flag:*`), so the method consistently authorizes by key or name and throws only if neither matches.src/core/CosmeticSchemas.ts (1)
79-79:⚠️ Potential issue | 🔴 CriticalRequired
flagsfield may break validation if cosmetics.json is not updated.The schema makes
flagsrequired, but ifresources/cosmetics/cosmetics.jsonlacks this key,CosmeticsSchema.safeParse()will fail and disable all cosmetics. Either add aflagsobject to the JSON file or use.optional().default({})to gracefully handle missing data.#!/bin/bash # Check if cosmetics.json contains the required "flags" field fd 'cosmetics.json' resources --exec cat {} | jq 'has("flags")'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/CosmeticSchemas.ts` at line 79, The CosmeticsSchema currently requires the flags field (defined as flags: z.record(z.string(), FlagSchema)), which breaks validation when resources/cosmetics/cosmetics.json omits it; change the flags entry in the CosmeticsSchema to be optional with a default empty object (e.g., use .optional().default({}) on the z.record(...) for flags) so missing flags do not cause safeParse to fail and cosmetics remain enabled.src/client/components/FlagButton.ts (1)
81-94:⚠️ Potential issue | 🟡 MinorUse accessible alt text instead of raw slug.
The
alt=${this.flag.name}exposes the raw identifier (like"ancient_rome") to screen readers. Since you already havetranslateCosmetic, use it for the alt text, or usealt=""witharia-hidden="true"if the image is purely decorative alongside the visible label.🔧 Proposed fix
<img src=${this.flag.url} - alt=${this.flag.name} + alt=${this.translateCosmetic("flags", this.flag.name)} class="w-full h-full object-contain pointer-events-none" draggable="false" loading="lazy"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/FlagButton.ts` around lines 81 - 94, The img in the FlagButton component currently uses raw slug via this.flag.name for alt; update the alt to use the human-friendly translation (call translateCosmetic(this.flag.name) or the component's translation helper) so screen readers get meaningful text, or if the image is purely decorative remove alt text (alt="") and add aria-hidden="true"; adjust the img element inside FlagButton's render/JSX where src/alt are set (keep the `@error` fallback logic intact).
🧹 Nitpick comments (1)
src/client/components/FlagButton.ts (1)
35-45: Smart translation fallback.The helper nicely detects untranslated keys (where translation equals the key) and converts
snake_caseto "Title Case" as a readable fallback. One small note: ifnameis empty string,word[0]would throw. Currently unlikely givenCosmeticNameSchemaregex, but a guard could be defensive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/FlagButton.ts` around lines 35 - 45, translateCosmetic currently assumes non-empty words and will call word[0] which can throw if name is an empty string; update translateCosmetic to defensively handle empty name/words by checking name (and each word) before accessing word[0]—e.g., if name is falsy return an empty string or name, and when mapping words ensure word.length>0 before using word[0] (keep existing filter but add a top-level guard in translateCosmetic to avoid accessing indices on an empty name).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/client/Main.ts`:
- Around line 365-389: Remove the duplicated store modal existence/type check:
keep a single check of this.storeModal (assigned from
document.getElementById("page-item-store")) against instanceof StoreModal and
remove the second identical block; ensure the remaining check still logs the
same warning ("Store modal element not found") and that any subsequent code uses
this.storeModal as before.
---
Duplicate comments:
In `@src/client/components/FlagButton.ts`:
- Around line 81-94: The img in the FlagButton component currently uses raw slug
via this.flag.name for alt; update the alt to use the human-friendly translation
(call translateCosmetic(this.flag.name) or the component's translation helper)
so screen readers get meaningful text, or if the image is purely decorative
remove alt text (alt="") and add aria-hidden="true"; adjust the img element
inside FlagButton's render/JSX where src/alt are set (keep the `@error` fallback
logic intact).
In `@src/core/CosmeticSchemas.ts`:
- Line 79: The CosmeticsSchema currently requires the flags field (defined as
flags: z.record(z.string(), FlagSchema)), which breaks validation when
resources/cosmetics/cosmetics.json omits it; change the flags entry in the
CosmeticsSchema to be optional with a default empty object (e.g., use
.optional().default({}) on the z.record(...) for flags) so missing flags do not
cause safeParse to fail and cosmetics remain enabled.
In `@src/server/Privilege.ts`:
- Around line 218-228: In isFlagAllowed, the flare membership check compares
flares against found.name but the lookup used key (from flagRef), causing
mismatches; update the check in isFlagAllowed to accept either identifier by
testing flares.includes(`flag:${key}`) OR flares.includes(`flag:${found.name}`)
(and still allow the wildcard `flag:*`), so the method consistently authorizes
by key or name and throws only if neither matches.
---
Nitpick comments:
In `@src/client/components/FlagButton.ts`:
- Around line 35-45: translateCosmetic currently assumes non-empty words and
will call word[0] which can throw if name is an empty string; update
translateCosmetic to defensively handle empty name/words by checking name (and
each word) before accessing word[0]—e.g., if name is falsy return an empty
string or name, and when mapping words ensure word.length>0 before using word[0]
(keep existing filter but add a top-level guard in translateCosmetic to avoid
accessing indices on an empty name).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba1195ca-7aec-480b-a423-947cc65560da
📒 Files selected for processing (20)
index.htmlresources/lang/en.jsonsrc/client/Api.tssrc/client/Cosmetics.tssrc/client/FlagInput.tssrc/client/FlagInputModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/components/FlagButton.tssrc/client/components/PatternButton.tssrc/client/components/PurchaseButton.tssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/CosmeticSchemas.tssrc/core/CustomFlag.tssrc/core/Schemas.tssrc/server/Privilege.tstests/Privilege.test.ts
✅ Files skipped from review due to trivial changes (5)
- src/client/LangSelector.ts
- resources/lang/en.json
- tests/Privilege.test.ts
- src/client/components/PurchaseButton.ts
- src/client/FlagInputModal.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/client/components/PatternButton.ts
- src/client/graphics/layers/PlayerInfoOverlay.ts
- index.html
- src/client/graphics/layers/NameLayer.ts
- src/client/Api.ts
- src/core/CustomFlag.ts
- src/client/FlagInput.ts
- src/core/Schemas.ts
- src/client/Cosmetics.ts
- src/client/Store.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/Schemas.ts (1)
499-509:⚠️ Potential issue | 🟠 MajorSplit flag ref validation from resolved player flag data.
src/server/Privilege.tsnow turnsflag:*refs intofound.url, butPlayerCosmeticsSchema.flagstill reusesFlagSchema, which only accepts country codes orflag:refs. Any API-boundary parse ofPlayerCosmeticsSchemawill reject purchased flags until the output schema is widened or separated from the input ref schema.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Schemas.ts` around lines 499 - 509, PlayerCosmeticsSchema currently reuses FlagSchema (which only allows country codes or flag:* refs) but the server resolves refs to an object containing found.url; create a new schema (e.g., PlayerFlagSchema or ResolvedFlagSchema) that accepts either the original FlagSchema input OR the resolved object shape (at minimum { url: z.string() }), then replace PlayerCosmeticsSchema.flag to use that union/alternate schema instead of FlagSchema; reference PlayerCosmeticsSchema, FlagSchema and the server ref-resolution flow (Privilege.ts) when making the change.
♻️ Duplicate comments (6)
src/client/components/FlagButton.ts (1)
81-84:⚠️ Potential issue | 🟡 MinorHide the decorative flag image from assistive tech.
The button already has a text label.
alt=${this.flag.name}adds the raw slug to the accessible name.🛠️ Possible fix
<img src=${this.flag.url} - alt=${this.flag.name} + alt="" + aria-hidden="true" class="w-full h-full object-contain pointer-events-none"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/FlagButton.ts` around lines 81 - 84, The decorative flag image in FlagButton's render should be hidden from assistive tech because the button already provides a text label; update the <img> used in the FlagButton component (the one using this.flag.url / this.flag.name) to be purely decorative by setting an empty alt attribute and marking it hidden from AT (e.g., alt="" and aria-hidden="true") so the slug name is not added to the accessible name.src/core/Schemas.ts (1)
477-483:⚠️ Potential issue | 🟡 MinorUpdate the validation message to match
flag:.The refine already accepts
flag:, but the error text still tells users to start with!.🛠️ Possible fix
- { message: "Invalid flag: must be a valid country code or start with !" }, + { + message: "Invalid flag: must be a valid country code or start with flag:", + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Schemas.ts` around lines 477 - 483, The validation error message in the .refine(...) validator (the function that checks val === undefined || val === "" || val.startsWith("flag:") || countryCodes.includes(val)) is incorrect; update the message object to reflect "flag:" instead of "!" so it matches the acceptance logic (e.g., change { message: "Invalid flag: must be a valid country code or start with !" } to mention "flag:"), locating the message next to the refine call in Schemas.ts.src/client/components/PurchaseButton.ts (1)
8-9:⚠️ Potential issue | 🟡 MinorGuard
productbefore first render.The non-null assertion still lets
<purchase-button>throw if it renders before.productis assigned.🛠️ Possible fix
private handleClick(e: Event) { e.stopPropagation(); + if (!this.product) return; this.onPurchase?.(); } render() { + if (!this.product) return html``; return html`Also applies to: 18-20, 23-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/PurchaseButton.ts` around lines 8 - 9, The component's non-null asserted property product in PurchaseButton can still cause runtime errors if the element renders before product is assigned; make product optional (remove the ! assertion) and guard all usages in render and lifecycle methods (e.g., the render() function, any getters or event handlers referencing product) by returning a safe fallback UI or disabled button when product is undefined, and only access product properties after checking for its presence; update references noted around lines for product and related blocks (product property declaration, render branches, and any methods using product) to use defensive checks instead of assuming it exists.src/server/Privilege.ts (1)
218-227:⚠️ Potential issue | 🟠 MajorAuthorize with the looked-up flag key.
The lookup uses
key, but the entitlement check still usesfound.name. If those ever differ, a validflag:${key}purchase is rejected on the server.🛠️ Possible fix
- if (flares.includes("flag:*") || flares.includes(`flag:${found.name}`)) { + if ( + flares.includes("flag:*") || + flares.includes(`flag:${key}`) || + flares.includes(`flag:${found.name}`) + ) { return found.url; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/Privilege.ts` around lines 218 - 227, Summary: The entitlement check in isFlagAllowed uses found.name but should authorize using the looked-up key. In isFlagAllowed(flares: string[], flagRef: string) replace the flares.includes check that uses found.name with one that uses the computed key (e.g., flares.includes(`flag:${key}`)) so purchases for flag:${key} are accepted; keep the existing flag:* check and still return found.url on success.src/client/Store.ts (2)
42-47:⚠️ Potential issue | 🟠 MajorKeep a stable
userMeResponsehandler and remove it on disconnect.This listener is still anonymous, so every reconnect adds another
onUserMe()/fetchCosmetics()path.Suggested fix
+ private _onUserMeResponse = (event: Event) => { + this.onUserMe((event as CustomEvent<UserMeResponse | false>).detail); + }; + connectedCallback() { super.connectedCallback(); - document.addEventListener( - "userMeResponse", - (event: CustomEvent<UserMeResponse | false>) => { - this.onUserMe(event.detail); - }, - ); + document.addEventListener("userMeResponse", this._onUserMeResponse); window.addEventListener("pattern-selected", this._onPatternSelected); } disconnectedCallback() { super.disconnectedCallback(); + document.removeEventListener("userMeResponse", this._onUserMeResponse); window.removeEventListener("pattern-selected", this._onPatternSelected); }Also applies to: 51-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Store.ts` around lines 42 - 47, The anonymous event listener for "userMeResponse" causes duplicate handlers on reconnect; change to a stable, named handler method/property on the Store class (e.g., this.handleUserMeResponse = (event: CustomEvent<UserMeResponse | false>) => this.onUserMe(event.detail)), register that named handler instead of an anonymous function, and call document.removeEventListener("userMeResponse", this.handleUserMeResponse) during disconnect/cleanup; apply the same pattern to the other anonymous listener(s) around lines 51-53 (e.g., the cosmetics/fetch handler) so each has a stored handler reference and is removed on disconnect.
307-314:⚠️ Potential issue | 🟡 MinorUse the flag display name in the toast.
This message uses the storage key, so users can see internal ids instead of the label shown on the card.
Suggested fix
- const name = flagKey.startsWith("flag:") - ? flagKey.slice("flag:".length) - : flagKey; + const nameKey = flagKey.startsWith("flag:") + ? flagKey.slice("flag:".length) + : flagKey; + const displayName = this.cosmetics?.flags[nameKey]?.name ?? nameKey; window.dispatchEvent( new CustomEvent("show-message", { detail: { - message: `${name} ${translateText("territory_patterns.selected")}`, + message: `${displayName} ${translateText("territory_patterns.selected")}`, duration: 2000, }, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Store.ts` around lines 307 - 314, The toast currently uses the storage key (flagKey) via the local variable name, showing internal IDs; replace this with the flag's user-facing label/display name used on the card (e.g., fetch the flag object via your flags map or a helper like getFlagByKey and use its displayName/label) and pass that display name into the message instead of name so the toast shows the human-readable flag label alongside translateText("territory_patterns.selected").
🤖 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/FlagInputModal.ts`:
- Around line 33-37: The filter for cosmeticFlags currently tests only
flag.name, which misses translated/prettified labels shown by FlagButton; update
the filter used in cosmeticFlags to call the same label formatter the UI uses
(the displayed label used by FlagButton) and pass that to this.includedInSearch,
falling back to the raw slug (flag.name) if no formatted label exists—i.e.,
compute displayLabel = <use the FlagButton/label-format helper or
flag.displayName or a prettify(flag.name)>, then call this.includedInSearch({
name: displayLabel, code: flag.name }) (keep flagRelationship check unchanged).
In `@src/client/Store.ts`:
- Around line 124-145: The code is incorrectly skipping entries where rel ===
"owned", preventing owned/default patterns from being rendered and selected;
update the loop condition(s) in Store.ts to only skip blocked entries (remove
the rel === "owned" check) so owned and default patterns are pushed into
buttons, keep the existing .requiresPurchase={rel === "purchasable"} logic, and
apply the same change to the analogous loop around lines 177-190 so
selectPattern, pattern-button rendering, and handlePurchase callbacks still work
for owned items.
In `@tests/CosmeticRelationship.test.ts`:
- Around line 133-146: The test title is misleading: it claims to verify that a
wildcard is preferred over a specific flare but the input only provides the
wildcard flare; update the test to match one of two fixes: either rename the
test description to something like "wildcard grants ownership" to reflect
current behavior, or modify the test's user flares (via makeUserMe) to include
both ["pattern:*","pattern:stripes:red"] so it actually asserts preference;
locate the test using the cosmeticRelationship call and makeUserMe helper and
apply one of these changes accordingly.
---
Outside diff comments:
In `@src/core/Schemas.ts`:
- Around line 499-509: PlayerCosmeticsSchema currently reuses FlagSchema (which
only allows country codes or flag:* refs) but the server resolves refs to an
object containing found.url; create a new schema (e.g., PlayerFlagSchema or
ResolvedFlagSchema) that accepts either the original FlagSchema input OR the
resolved object shape (at minimum { url: z.string() }), then replace
PlayerCosmeticsSchema.flag to use that union/alternate schema instead of
FlagSchema; reference PlayerCosmeticsSchema, FlagSchema and the server
ref-resolution flow (Privilege.ts) when making the change.
---
Duplicate comments:
In `@src/client/components/FlagButton.ts`:
- Around line 81-84: The decorative flag image in FlagButton's render should be
hidden from assistive tech because the button already provides a text label;
update the <img> used in the FlagButton component (the one using this.flag.url /
this.flag.name) to be purely decorative by setting an empty alt attribute and
marking it hidden from AT (e.g., alt="" and aria-hidden="true") so the slug name
is not added to the accessible name.
In `@src/client/components/PurchaseButton.ts`:
- Around line 8-9: The component's non-null asserted property product in
PurchaseButton can still cause runtime errors if the element renders before
product is assigned; make product optional (remove the ! assertion) and guard
all usages in render and lifecycle methods (e.g., the render() function, any
getters or event handlers referencing product) by returning a safe fallback UI
or disabled button when product is undefined, and only access product properties
after checking for its presence; update references noted around lines for
product and related blocks (product property declaration, render branches, and
any methods using product) to use defensive checks instead of assuming it
exists.
In `@src/client/Store.ts`:
- Around line 42-47: The anonymous event listener for "userMeResponse" causes
duplicate handlers on reconnect; change to a stable, named handler
method/property on the Store class (e.g., this.handleUserMeResponse = (event:
CustomEvent<UserMeResponse | false>) => this.onUserMe(event.detail)), register
that named handler instead of an anonymous function, and call
document.removeEventListener("userMeResponse", this.handleUserMeResponse) during
disconnect/cleanup; apply the same pattern to the other anonymous listener(s)
around lines 51-53 (e.g., the cosmetics/fetch handler) so each has a stored
handler reference and is removed on disconnect.
- Around line 307-314: The toast currently uses the storage key (flagKey) via
the local variable name, showing internal IDs; replace this with the flag's
user-facing label/display name used on the card (e.g., fetch the flag object via
your flags map or a helper like getFlagByKey and use its displayName/label) and
pass that display name into the message instead of name so the toast shows the
human-readable flag label alongside
translateText("territory_patterns.selected").
In `@src/core/Schemas.ts`:
- Around line 477-483: The validation error message in the .refine(...)
validator (the function that checks val === undefined || val === "" ||
val.startsWith("flag:") || countryCodes.includes(val)) is incorrect; update the
message object to reflect "flag:" instead of "!" so it matches the acceptance
logic (e.g., change { message: "Invalid flag: must be a valid country code or
start with !" } to mention "flag:"), locating the message next to the refine
call in Schemas.ts.
In `@src/server/Privilege.ts`:
- Around line 218-227: Summary: The entitlement check in isFlagAllowed uses
found.name but should authorize using the looked-up key. In
isFlagAllowed(flares: string[], flagRef: string) replace the flares.includes
check that uses found.name with one that uses the computed key (e.g.,
flares.includes(`flag:${key}`)) so purchases for flag:${key} are accepted; keep
the existing flag:* check and still return found.url on success.
🪄 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: c9cb583b-f30a-48f2-989e-1fb4f54a092f
📒 Files selected for processing (21)
index.htmlresources/lang/en.jsonsrc/client/Api.tssrc/client/Cosmetics.tssrc/client/FlagInput.tssrc/client/FlagInputModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/components/FlagButton.tssrc/client/components/PatternButton.tssrc/client/components/PurchaseButton.tssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/CosmeticSchemas.tssrc/core/CustomFlag.tssrc/core/Schemas.tssrc/server/Privilege.tstests/CosmeticRelationship.test.tstests/Privilege.test.ts
✅ Files skipped from review due to trivial changes (4)
- src/client/LangSelector.ts
- src/client/graphics/layers/PlayerInfoOverlay.ts
- tests/Privilege.test.ts
- resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (7)
- src/client/components/PatternButton.ts
- index.html
- src/client/Api.ts
- src/client/FlagInput.ts
- src/client/graphics/layers/NameLayer.ts
- src/client/Main.ts
- src/client/Cosmetics.ts
Comment on lines
+33
to
+37
| const cosmeticFlags = Object.entries(this.cosmetics?.flags ?? {}) | ||
| .filter(([, flag]) => { | ||
| if (!this.includedInSearch({ name: flag.name, code: flag.name })) | ||
| return false; | ||
| return flagRelationship(flag, this.userMe, null) === "owned"; |
Contributor
There was a problem hiding this comment.
Search with the same label the user sees.
This filter only checks the raw slug in flag.name. flag-button renders a translated / prettified label, so queries like Ancient Rome or localized names stop matching. Use the displayed label for search, and keep the raw key as fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/FlagInputModal.ts` around lines 33 - 37, The filter for
cosmeticFlags currently tests only flag.name, which misses translated/prettified
labels shown by FlagButton; update the filter used in cosmeticFlags to call the
same label formatter the UI uses (the displayed label used by FlagButton) and
pass that to this.includedInSearch, falling back to the raw slug (flag.name) if
no formatted label exists—i.e., compute displayLabel = <use the
FlagButton/label-format helper or flag.displayName or a prettify(flag.name)>,
then call this.includedInSearch({ name: displayLabel, code: flag.name }) (keep
flagRelationship check unchanged).
Comment on lines
+124
to
+145
| if (rel === "blocked" || rel === "owned") { | ||
| continue; | ||
| } | ||
| const isDefaultPattern = pattern === null; | ||
| const isSelected = | ||
| (isDefaultPattern && this.selectedPattern === null) || | ||
| (!isDefaultPattern && | ||
| this.selectedPattern && | ||
| this.selectedPattern.name === pattern?.name && | ||
| (this.selectedPattern.colorPalette?.name ?? null) === | ||
| (colorPalette?.name ?? null)); | ||
| buttons.push(html` | ||
| <pattern-button | ||
| .pattern=${pattern} | ||
| .colorPalette=${this.cosmetics?.colorPalettes?.[ | ||
| colorPalette?.name ?? "" | ||
| ] ?? null} | ||
| .requiresPurchase=${rel === "purchasable"} | ||
| .selected=${isSelected} | ||
| .onSelect=${(p: PlayerPattern | null) => this.selectPattern(p)} | ||
| .onPurchase=${(p: Pattern, cp: ColorPalette | null) => | ||
| handlePurchase(p, cp)} |
Contributor
There was a problem hiding this comment.
Do not filter out owned entries in these grids.
This component still keeps selection state and callbacks for owned cosmetics, but both loops continue on rel === "owned". As written, the default pattern and all already-owned items never render here, so users cannot switch back to them from this modal.
Suggested fix
- if (rel === "blocked" || rel === "owned") {
+ if (rel === "blocked") {
continue;
}
…
- if (rel === "blocked" || rel === "owned") continue;
+ if (rel === "blocked") continue;Also applies to: 177-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/Store.ts` around lines 124 - 145, The code is incorrectly skipping
entries where rel === "owned", preventing owned/default patterns from being
rendered and selected; update the loop condition(s) in Store.ts to only skip
blocked entries (remove the rel === "owned" check) so owned and default patterns
are pushed into buttons, keep the existing .requiresPurchase={rel ===
"purchasable"} logic, and apply the same change to the analogous loop around
lines 177-190 so selectPattern, pattern-button rendering, and handlePurchase
callbacks still work for owned items.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/Main.ts (1)
638-658:⚠️ Potential issue | 🟠 Major
#purchase-completedstill treats every cosmetic like a pattern.This PR adds flag checkout in
src/client/Cosmetics.ts, but this branch always writesparams.get("cosmetic")intoterritoryPatternwithsetSelectedPatternName(). A successful flag purchase will not select the new flag, and it can also leave a flag key in pattern storage. Please branch on the returned cosmetic type/ref before updating settings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Main.ts` around lines 638 - 658, The handler unconditionally treats any purchased cosmetic as a pattern by calling this.userSettings.setSelectedPatternName(patternName); change it to check the cosmetic type/ref returned in the params (the same value added by your Cosmetics.ts checkout change) and only call setSelectedPatternName when the cosmetic type equals "pattern"; for "flag" cosmetics do not write into the pattern storage—either call the appropriate flag-setting method on userSettings (e.g., setSelectedFlagName or setSelectedTerritoryFlag if present) or skip updating settings entirely—so update the branch around patternName/token handling to branch on the cosmetic type/ref before mutating userSettings.
♻️ Duplicate comments (8)
src/client/components/FlagButton.ts (1)
81-84:⚠️ Potential issue | 🟡 MinorUse a decorative alt here.
The button already shows the flag name as text.
alt=${this.flag.name}makes screen readers read raw slugs likeancient_rome. Usealt=""plusaria-hidden="true"here, or reuse the translated label.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/FlagButton.ts` around lines 81 - 84, Update the decorative flag image in the FlagButton component so screen readers don't read the raw slug: replace alt=${this.flag.name} with an empty alt attribute alt="" and add aria-hidden="true" on the <img>, or alternatively set alt to the translated label used elsewhere; locate the image usage in FlagButton.ts (the <img src=${this.flag.url} ... /> where this.flag.name is used) and apply one of these accessibility fixes.src/client/FlagInputModal.ts (1)
33-37:⚠️ Potential issue | 🟡 MinorSearch still uses the raw slug, not the label users see.
includedInSearch()only getsflag.name, but<flag-button>shows a translated / title-cased label. Searches likeAncient Romeor localized names will still miss cosmetic flags. Reuse the same label formatter here, with the raw slug as fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/FlagInputModal.ts` around lines 33 - 37, The search filter for cosmeticFlags currently passes only the raw slug (flag.name) into includedInSearch, causing mismatches with the UI label; update the cosmeticFlags filter to compute the display label the same way <flag-button> does (use the shared flag label formatter used by the component, falling back to the raw slug/flag.name if the formatter returns empty) and pass that display label into includedInSearch, leaving the ownership check using flagRelationship(flag, this.userMe, null) unchanged.src/client/Cosmetics.ts (1)
239-257:⚠️ Potential issue | 🟠 MajorDo not clear the saved flag when cosmetics failed to load.
flagDataisundefinedboth for an unknown key and forfetchCosmetics()returningnull. In the second case this permanently removeslocalStorage["flag"]after one bad/cosmetics.jsonresponse. Only clear the saved value after a successful cosmetics load proves the key is invalid.Minimal guard
if (flag?.startsWith("flag:")) { const key = flag.slice("flag:".length); const flagData = cosmetics?.flags?.[key]; - if (!flagData) { + if (cosmetics && !flagData) { flag = undefined; - } else { + } else if (flagData) { const userMe = await getUserMe(); if (userMe) { const flares = userMe.player.flares ?? []; const hasWildcard = flares.includes("flag:*"); if (!hasWildcard && !flares.includes(`flag:${flagData.name}`)) { flag = undefined; } } } - if (flag === undefined) { + if (cosmetics && flag === undefined) { localStorage.removeItem("flag"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Cosmetics.ts` around lines 239 - 257, The code clears localStorage["flag"] even when cosmetics failed to load because flagData is undefined for both missing key and a null/missing cosmetics object; change the logic in Cosmetics.ts so you only treat the key as invalid when cosmetics were actually loaded (e.g. cosmetics !== null/undefined) — specifically, only set flag = undefined when flagData is undefined AND cosmetics is present, and only call localStorage.removeItem("flag") in that same case; keep the existing checks around cosmetics?.flags, the flag variable, the flagData local, and the getUserMe() permission check intact.src/core/CosmeticSchemas.ts (1)
76-79:⚠️ Potential issue | 🔴 CriticalPlease verify the shipped cosmetics JSON now has
flags.
fetchCosmetics()hard-fails on schema mismatch. If the data file still omits this new required key, both client and server fall back tonullcosmetics and all patterns/flags disappear. Either shipflags: {}in the data, or make this field default to an empty record here.Run this read-only check. Expected result: every cosmetics JSON prints
has_flags=Trueandflags_type=dict.#!/bin/bash set -euo pipefail python - <<'PY' import json from pathlib import Path for path in Path("resources").rglob("cosmetics.json"): data = json.loads(path.read_text()) print(f"{path}: has_flags={'flags' in data}, flags_type={type(data.get('flags')).__name__}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/CosmeticSchemas.ts` around lines 76 - 79, fetchCosmetics() is failing when shipped cosmetics JSON lacks the new required "flags" key; update the CosmeticsSchema (the export named CosmeticsSchema in src/core/CosmeticSchemas.ts) so "flags" is not a hard-required field — either make it optional and supply a default empty record (e.g., z.record(z.string(), FlagSchema).optional().default({})) or use z.record(...).default({}) so missing files parse as an empty object; alternatively, ensure the shipped cosmetics JSON files include "flags": {} to satisfy the current schema (verify with the provided read-only check).src/client/Store.ts (4)
307-317:⚠️ Potential issue | 🟡 MinorToast shows raw key instead of display name.
The flag toast message uses the storage key (e.g.,
ancient_rome) rather than the cosmetic's display name. Look upthis.cosmetics?.flags[nameKey]?.namefor a user-friendly label.🔧 Suggested fix
private selectFlag(flagKey: string) { localStorage.setItem("flag", flagKey); this.dispatchEvent( new CustomEvent("flag-change", { detail: { flag: flagKey }, bubbles: true, composed: true, }), ); const name = flagKey.includes(":") ? flagKey.slice(flagKey.indexOf(":") + 1) : flagKey; + const displayName = this.cosmetics?.flags?.[name]?.name ?? name; window.dispatchEvent( new CustomEvent("show-message", { detail: { - message: `${name} ${translateText("territory_patterns.selected")}`, + message: `${displayName} ${translateText("territory_patterns.selected")}`, duration: 2000, }, }), ); this.close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Store.ts` around lines 307 - 317, The toast currently displays the raw storage key (`flagKey`/`name`) instead of the user-friendly label; extract the flag key used in cosmetics lookup (e.g., call it nameKey by stripping prefix same as current slice logic) and use this.cosmetics?.flags[nameKey]?.name when present, falling back to the existing name if not found, then pass that display string into the CustomEvent message; reference the existing variables flagKey and name (and add nameKey) and update the message construction to prefer the cosmetics display name.
40-54:⚠️ Potential issue | 🟠 MajorEvent listener leak:
userMeResponsehandler is never removed.The anonymous function passed to
addEventListenercannot be removed indisconnectedCallback(). Each reconnect stacks another handler.🔧 Suggested fix: extract handler to a class property
+ private _onUserMeResponse = (event: Event) => { + this.onUserMe((event as CustomEvent<UserMeResponse | false>).detail); + }; + connectedCallback() { super.connectedCallback(); - document.addEventListener( - "userMeResponse", - (event: CustomEvent<UserMeResponse | false>) => { - this.onUserMe(event.detail); - }, - ); + document.addEventListener("userMeResponse", this._onUserMeResponse); window.addEventListener("pattern-selected", this._onPatternSelected); } disconnectedCallback() { super.disconnectedCallback(); + document.removeEventListener("userMeResponse", this._onUserMeResponse); window.removeEventListener("pattern-selected", this._onPatternSelected); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Store.ts` around lines 40 - 54, The anonymous listener for "userMeResponse" in connectedCallback causes a leak because it can't be removed in disconnectedCallback; extract the handler into a class property (e.g., this._onUserMeResponse) that calls this.onUserMe(event.detail), register that property with document.addEventListener("userMeResponse", this._onUserMeResponse) in connectedCallback, and remove it in disconnectedCallback with document.removeEventListener("userMeResponse", this._onUserMeResponse); keep the existing _onPatternSelected usage as-is.
124-126:⚠️ Potential issue | 🟠 MajorUsers cannot select owned patterns.
Filtering out
rel === "owned"prevents users from switching to patterns they already own. They should be able to select any owned item, not just purchasable ones.🔧 Suggested fix
- if (rel === "blocked" || rel === "owned") { + if (rel === "blocked") { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Store.ts` around lines 124 - 126, The current loop in Store.ts is skipping items with rel === "owned", which prevents users from selecting patterns they already own; update the condition that currently reads if (rel === "blocked" || rel === "owned") { continue; } to only skip blocked items (i.e., remove the rel === "owned" branch) so owned items are not filtered out—look for the loop/conditional using the rel variable in Store.ts (the block containing if (rel === "blocked" || rel === "owned") continue;) and change it to only continue on rel === "blocked".
177-178:⚠️ Potential issue | 🟠 MajorUsers cannot select owned flags; also hoist
localStorageread.Same issue as patterns: filtering
"owned"prevents selecting already-owned flags.Additionally,
localStorage.getItem("flag")is called on every loop iteration. Move it outside the loop.🔧 Suggested fix
private renderFlagGrid(): TemplateResult { const buttons: TemplateResult[] = []; const flags = Object.entries(this.cosmetics?.flags ?? {}); + const selectedFlag = localStorage.getItem("flag") ?? ""; for (const [key, flag] of flags) { const rel = flagRelationship( flag, this.userMeResponse, this.affiliateCode, ); - if (rel === "blocked" || rel === "owned") continue; - const selectedFlag = localStorage.getItem("flag") ?? ""; + if (rel === "blocked") continue; buttons.push(html`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Store.ts` around lines 177 - 178, The loop in Store.ts incorrectly skips "owned" by using if (rel === "blocked" || rel === "owned") continue, which prevents users from selecting flags, and it calls localStorage.getItem("flag") inside each iteration; change the filter to only skip "blocked" (remove the rel === "owned" check) so owned flags remain selectable, and hoist the localStorage read by assigning const selectedFlag = localStorage.getItem("flag") ?? "" once before the loop (replace the in-loop usage of selectedFlag accordingly).
🧹 Nitpick comments (1)
src/client/Store.ts (1)
342-345:asyncis unnecessary here.
refresh()doesn'tawaitanything. Consider removingasyncfor clarity, or returnthis.updateCompleteif callers need to wait for the render.♻️ Option A: remove async
- public async refresh() { + public refresh() { this.requestUpdate(); }♻️ Option B: return updateComplete promise
public async refresh() { this.requestUpdate(); + return this.updateComplete; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Store.ts` around lines 342 - 345, The refresh() method is marked async but does not await anything; remove the unnecessary async modifier from the Store.refresh() declaration and keep the body calling this.requestUpdate(), or alternatively change refresh() to return this.updateComplete so callers can await completion (i.e., replace async declaration with a non-async method or return this.updateComplete from refresh()); update the Store.refresh() signature and implementation accordingly, referencing the existing requestUpdate() and updateComplete members.
🤖 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/FlagButton.ts`:
- Around line 53-57: The outer wrapper in the FlagButton component currently
includes the "no-crazygames" class which hides the entire flag card; remove
"no-crazygames" from the container div in FlagButton and instead add that class
only to the purchase-only UI element (e.g., the purchase CTA/button or the
renderPurchaseCTA/renderPurchaseUI element) so the shared flag picker (used by
flag-input-modal) remains visible on CrazyGames.
In `@src/client/Cosmetics.ts`:
- Around line 238-260: Existing bare flag slugs must be migrated to the new
prefixed form before returning; detect when flag is truthy and does not
startWith("flag:") or startWith("country:"), then attempt to resolve it against
cosmetics?.flags (i.e. check if cosmetics?.flags?.[flag] exists) and if found
replace flag with `flag:${flag}` and run the same permission validation flow
(use getUserMe() and flares check as in the flagged branch), otherwise clear it
from localStorage (localStorage.removeItem("flag")). Update the code around
userSettings.getFlag(), the flag variable, cosmetics?.flags and the getUserMe()
permission checks so legacy unprefixed values are converted and validated before
the function returns.
In `@src/client/Store.ts`:
- Around line 29-33: In class Store, mark the private property isActive with the
`@state`() decorator (like selectedPattern/selectedColor/activeTab) so it becomes
an explicit reactive field; update the imports to include `@state` if not already
present and ensure the decorator sits immediately above the isActive declaration
in Store to keep reactivity consistent with other stateful properties.
---
Outside diff comments:
In `@src/client/Main.ts`:
- Around line 638-658: The handler unconditionally treats any purchased cosmetic
as a pattern by calling this.userSettings.setSelectedPatternName(patternName);
change it to check the cosmetic type/ref returned in the params (the same value
added by your Cosmetics.ts checkout change) and only call setSelectedPatternName
when the cosmetic type equals "pattern"; for "flag" cosmetics do not write into
the pattern storage—either call the appropriate flag-setting method on
userSettings (e.g., setSelectedFlagName or setSelectedTerritoryFlag if present)
or skip updating settings entirely—so update the branch around patternName/token
handling to branch on the cosmetic type/ref before mutating userSettings.
---
Duplicate comments:
In `@src/client/components/FlagButton.ts`:
- Around line 81-84: Update the decorative flag image in the FlagButton
component so screen readers don't read the raw slug: replace
alt=${this.flag.name} with an empty alt attribute alt="" and add
aria-hidden="true" on the <img>, or alternatively set alt to the translated
label used elsewhere; locate the image usage in FlagButton.ts (the <img
src=${this.flag.url} ... /> where this.flag.name is used) and apply one of these
accessibility fixes.
In `@src/client/Cosmetics.ts`:
- Around line 239-257: The code clears localStorage["flag"] even when cosmetics
failed to load because flagData is undefined for both missing key and a
null/missing cosmetics object; change the logic in Cosmetics.ts so you only
treat the key as invalid when cosmetics were actually loaded (e.g. cosmetics !==
null/undefined) — specifically, only set flag = undefined when flagData is
undefined AND cosmetics is present, and only call
localStorage.removeItem("flag") in that same case; keep the existing checks
around cosmetics?.flags, the flag variable, the flagData local, and the
getUserMe() permission check intact.
In `@src/client/FlagInputModal.ts`:
- Around line 33-37: The search filter for cosmeticFlags currently passes only
the raw slug (flag.name) into includedInSearch, causing mismatches with the UI
label; update the cosmeticFlags filter to compute the display label the same way
<flag-button> does (use the shared flag label formatter used by the component,
falling back to the raw slug/flag.name if the formatter returns empty) and pass
that display label into includedInSearch, leaving the ownership check using
flagRelationship(flag, this.userMe, null) unchanged.
In `@src/client/Store.ts`:
- Around line 307-317: The toast currently displays the raw storage key
(`flagKey`/`name`) instead of the user-friendly label; extract the flag key used
in cosmetics lookup (e.g., call it nameKey by stripping prefix same as current
slice logic) and use this.cosmetics?.flags[nameKey]?.name when present, falling
back to the existing name if not found, then pass that display string into the
CustomEvent message; reference the existing variables flagKey and name (and add
nameKey) and update the message construction to prefer the cosmetics display
name.
- Around line 40-54: The anonymous listener for "userMeResponse" in
connectedCallback causes a leak because it can't be removed in
disconnectedCallback; extract the handler into a class property (e.g.,
this._onUserMeResponse) that calls this.onUserMe(event.detail), register that
property with document.addEventListener("userMeResponse",
this._onUserMeResponse) in connectedCallback, and remove it in
disconnectedCallback with document.removeEventListener("userMeResponse",
this._onUserMeResponse); keep the existing _onPatternSelected usage as-is.
- Around line 124-126: The current loop in Store.ts is skipping items with rel
=== "owned", which prevents users from selecting patterns they already own;
update the condition that currently reads if (rel === "blocked" || rel ===
"owned") { continue; } to only skip blocked items (i.e., remove the rel ===
"owned" branch) so owned items are not filtered out—look for the
loop/conditional using the rel variable in Store.ts (the block containing if
(rel === "blocked" || rel === "owned") continue;) and change it to only continue
on rel === "blocked".
- Around line 177-178: The loop in Store.ts incorrectly skips "owned" by using
if (rel === "blocked" || rel === "owned") continue, which prevents users from
selecting flags, and it calls localStorage.getItem("flag") inside each
iteration; change the filter to only skip "blocked" (remove the rel === "owned"
check) so owned flags remain selectable, and hoist the localStorage read by
assigning const selectedFlag = localStorage.getItem("flag") ?? "" once before
the loop (replace the in-loop usage of selectedFlag accordingly).
In `@src/core/CosmeticSchemas.ts`:
- Around line 76-79: fetchCosmetics() is failing when shipped cosmetics JSON
lacks the new required "flags" key; update the CosmeticsSchema (the export named
CosmeticsSchema in src/core/CosmeticSchemas.ts) so "flags" is not a
hard-required field — either make it optional and supply a default empty record
(e.g., z.record(z.string(), FlagSchema).optional().default({})) or use
z.record(...).default({}) so missing files parse as an empty object;
alternatively, ensure the shipped cosmetics JSON files include "flags": {} to
satisfy the current schema (verify with the provided read-only check).
---
Nitpick comments:
In `@src/client/Store.ts`:
- Around line 342-345: The refresh() method is marked async but does not await
anything; remove the unnecessary async modifier from the Store.refresh()
declaration and keep the body calling this.requestUpdate(), or alternatively
change refresh() to return this.updateComplete so callers can await completion
(i.e., replace async declaration with a non-async method or return
this.updateComplete from refresh()); update the Store.refresh() signature and
implementation accordingly, referencing the existing requestUpdate() and
updateComplete members.
🪄 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: fe407759-a589-423e-a8e8-8d16a1a2b805
📒 Files selected for processing (22)
index.htmlresources/lang/en.jsonsrc/client/Api.tssrc/client/Cosmetics.tssrc/client/FlagInput.tssrc/client/FlagInputModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/components/FlagButton.tssrc/client/components/PatternButton.tssrc/client/components/PurchaseButton.tssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/CosmeticSchemas.tssrc/core/CustomFlag.tssrc/core/Schemas.tssrc/core/game/UserSettings.tssrc/server/Privilege.tstests/CosmeticRelationship.test.tstests/Privilege.test.ts
✅ Files skipped from review due to trivial changes (5)
- src/client/LangSelector.ts
- tests/Privilege.test.ts
- resources/lang/en.json
- src/core/CustomFlag.ts
- tests/CosmeticRelationship.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/client/components/PatternButton.ts
- index.html
- src/client/Api.ts
- src/client/FlagInput.ts
- src/client/components/PurchaseButton.ts
- src/server/Privilege.ts
- src/core/Schemas.ts
- src/client/TerritoryPatternsModal.ts
Comment on lines
+53
to
+57
| <div | ||
| class="no-crazygames flex flex-col items-center justify-between gap-2 p-3 bg-white/5 backdrop-blur-sm border rounded-xl w-48 h-full transition-all duration-200 ${this | ||
| .selected | ||
| ? "border-green-500 shadow-[0_0_15px_rgba(34,197,94,0.5)]" | ||
| : "hover:bg-white/10 hover:border-white/20 hover:shadow-xl border-white/10"}" |
Contributor
There was a problem hiding this comment.
Do not hide the whole flag card on CrazyGames.
hideCrazyGamesElements() removes every .no-crazygames node. This component is also used by flag-input-modal for free country flags, so putting that class on the outer wrapper can hide the full flag picker on CrazyGames, not just the purchase CTA. Keep the class off the shared card and apply it only to purchase-only UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/components/FlagButton.ts` around lines 53 - 57, The outer wrapper
in the FlagButton component currently includes the "no-crazygames" class which
hides the entire flag card; remove "no-crazygames" from the container div in
FlagButton and instead add that class only to the purchase-only UI element
(e.g., the purchase CTA/button or the renderPurchaseCTA/renderPurchaseUI
element) so the shared flag picker (used by flag-input-modal) remains visible on
CrazyGames.
Comment on lines
+238
to
+260
| let flag = userSettings.getFlag(); | ||
| if (flag?.startsWith("flag:")) { | ||
| const key = flag.slice("flag:".length); | ||
| const flagData = cosmetics?.flags?.[key]; | ||
| if (!flagData) { | ||
| flag = undefined; | ||
| } else { | ||
| const userMe = await getUserMe(); | ||
| if (userMe) { | ||
| const flares = userMe.player.flares ?? []; | ||
| const hasWildcard = flares.includes("flag:*"); | ||
| if (!hasWildcard && !flares.includes(`flag:${flagData.name}`)) { | ||
| flag = undefined; | ||
| } | ||
| } | ||
| } | ||
| if (flag === undefined) { | ||
| localStorage.removeItem("flag"); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| flag: userSettings.getFlag(), | ||
| flag, |
Contributor
There was a problem hiding this comment.
Legacy unprefixed flags still fall through unchanged.
This block only validates flag: refs. Older saved values like us or other bare slugs pass straight through, but getPlayerCosmetics() now resolves only flag: and country: refs. After upgrade, existing users lose that saved flag until they reselect it. Please migrate bare values here before returning flag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/Cosmetics.ts` around lines 238 - 260, Existing bare flag slugs
must be migrated to the new prefixed form before returning; detect when flag is
truthy and does not startWith("flag:") or startWith("country:"), then attempt to
resolve it against cosmetics?.flags (i.e. check if cosmetics?.flags?.[flag]
exists) and if found replace flag with `flag:${flag}` and run the same
permission validation flow (use getUserMe() and flares check as in the flagged
branch), otherwise clear it from localStorage (localStorage.removeItem("flag")).
Update the code around userSettings.getFlag(), the flag variable,
cosmetics?.flags and the getUserMe() permission checks so legacy unprefixed
values are converted and validated before the function returns.
Comment on lines
+29
to
+33
| private cosmetics: Cosmetics | null = null; | ||
| private userSettings: UserSettings = new UserSettings(); | ||
| private isActive = false; | ||
| private affiliateCode: string | null = null; | ||
| private userMeResponse: UserMeResponse | false = false; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if BaseModal.close() triggers requestUpdate or handles visibility
ast-grep --pattern $'class BaseModal {
$$$
close() {
$$$
}
$$$
}'
# Also check for any requestUpdate in BaseModal
rg -n "requestUpdate|isActive" --type ts -C 3 src/client/components/BaseModal.tsRepository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Find BaseModal class location
fd -e ts -e js | head -20Repository: openfrontio/OpenFrontIO
Length of output: 586
🏁 Script executed:
# Search for BaseModal in codebase
rg "class BaseModal" --type tsRepository: openfrontio/OpenFrontIO
Length of output: 154
🏁 Script executed:
# Look at Store.ts to understand the context
head -50 src/client/Store.tsRepository: openfrontio/OpenFrontIO
Length of output: 1750
🏁 Script executed:
# Find where isActive is used in Store.ts
rg -n "isActive" src/client/Store.ts -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 752
🏁 Script executed:
# Check BaseModal.close() implementation
sed -n '1,100p' src/client/components/BaseModal.ts | head -60Repository: openfrontio/OpenFrontIO
Length of output: 1844
🏁 Script executed:
# Search for refresh() method in Store.ts
rg -n "refresh\(\)|inline" src/client/Store.ts -B 2 -A 5Repository: openfrontio/OpenFrontIO
Length of output: 1607
🏁 Script executed:
# Full view of BaseModal.ts to understand close() and requestUpdate behavior
wc -l src/client/components/BaseModal.tsRepository: openfrontio/OpenFrontIO
Length of output: 105
🏁 Script executed:
# Check BaseModal.close() implementation and rest of the file
sed -n '50,150p' src/client/components/BaseModal.tsRepository: openfrontio/OpenFrontIO
Length of output: 2590
🏁 Script executed:
# Check if there's a public close() method in BaseModal
rg -n "public.*close\(\)|private.*close\(\)" src/client/components/BaseModal.ts -A 8Repository: openfrontio/OpenFrontIO
Length of output: 325
Mark isActive with @state() decorator for consistency and explicit reactivity.
The property isActive is checked in render() (line 225) but lacks the @state() decorator. Other reactive properties in this class (selectedPattern, selectedColor, activeTab) use @state(). While the modal currently works because BaseModal.close() triggers a re-render through its own @state() isModalOpen, it is clearer and more maintainable to mark isActive as reactive to match the established pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/Store.ts` around lines 29 - 33, In class Store, mark the private
property isActive with the `@state`() decorator (like
selectedPattern/selectedColor/activeTab) so it becomes an explicit reactive
field; update the imports to include `@state` if not already present and ensure
the decorator sits immediately above the isActive declaration in Store to keep
reactivity consistent with other stateful properties.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
src/server/Privilege.ts (1)
228-238:⚠️ Potential issue | 🟠 MajorFlare check uses
found.namebut lookup uses dictionary key.This was flagged previously: if
cosmetics.flags["us"]hasname: "united_states", then:
flagRef = "flag:us"→key = "us"→ lookup succeeds- But flare check needs
"flag:united_states"to matchThe flare should check against the key used for lookup, not
found.name:🐛 Proposed fix
isFlagAllowed(flares: string[], flagRef: string): string { const key = flagRef.slice("flag:".length); const found = this.cosmetics.flags[key]; if (!found) throw new Error(`Flag ${key} not found`); - if (flares.includes("flag:*") || flares.includes(`flag:${found.name}`)) { + if (flares.includes("flag:*") || flares.includes(`flag:${key}`)) { return found.url; } throw new Error(`No flares for flag ${key}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/Privilege.ts` around lines 228 - 238, The flare membership check in isFlagAllowed uses found.name while the dictionary lookup uses key, causing mismatches (e.g., cosmetics.flags["us"] with name "united_states"); change the check to compare against the lookup key instead of found.name by replacing the flares.includes(`flag:${found.name}`) test with flares.includes(`flag:${key}`) while keeping the existing flag:* branch and returning found.url on match.src/core/CosmeticSchemas.ts (1)
79-79:⚠️ Potential issue | 🔴 CriticalRequired
flagsfield may break cosmetics loading.This was flagged previously: if
resources/cosmetics/cosmetics.jsonlacks theflagskey,CosmeticsSchema.safeParse()fails and all cosmetics stop working.Either add
flagsto the JSON file or make it optional with a default:🐛 Option 1: Make flags optional with default
export const CosmeticsSchema = z.object({ colorPalettes: z.record(z.string(), ColorPaletteSchema).optional(), patterns: z.record(z.string(), PatternSchema), - flags: z.record(z.string(), FlagSchema), + flags: z.record(z.string(), FlagSchema).default({}), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/CosmeticSchemas.ts` at line 79, The CosmeticsSchema's required "flags" property causes safeParse to fail when the JSON omits it; update the schema in CosmeticSchemas.ts by making the "flags" field optional with a default empty record (use z.record(z.string(), FlagSchema).optional().default({}) or the equivalent z.optional(...).default({})) so cosmetics load when "flags" is missing; adjust any downstream code that assumes flags is always present if needed to rely on the default.src/client/FlagInputModal.ts (1)
33-37:⚠️ Potential issue | 🟡 MinorSearch filter uses raw slug instead of display label.
This was flagged previously: the filter checks
flag.name(the raw slug likeancient_rome), but<flag-button>renders a prettified label. Users searching "Ancient Rome" won't find it.Consider using a display-name formatter that matches what
<flag-button>shows:const displayName = flag.name.split("_").map(w => w.charAt(0).toUpperCase() + w.slice(1) ).join(" ");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/FlagInputModal.ts` around lines 33 - 37, The search filter currently tests the raw slug (flag.name) when building cosmeticFlags in FlagInputModal, so users searching the prettified label (as rendered by <flag-button>) won't match; update the filter inside the Object.entries(...).filter call to compute the display name (e.g., split on "_" and capitalize words) and call this.includedInSearch with that displayName (and/or include both displayName and flag.name) instead of only flag.name, keeping the ownership check via flagRelationship(flag, this.userMe, null) === "owned".src/client/Store.ts (3)
40-54:⚠️ Potential issue | 🟠 MajorListener leak: remove
userMeResponsehandler indisconnectedCallback.The anonymous function on lines 42-47 cannot be unregistered. If this element reconnects, handlers stack and
onUserMe()/fetchCosmetics()run multiple times per event. Store the handler as a class property and use the same reference for both add and remove.🔧 Suggested fix
+ private _onUserMeResponse = (event: Event) => { + this.onUserMe((event as CustomEvent<UserMeResponse | false>).detail); + }; + connectedCallback() { super.connectedCallback(); - document.addEventListener( - "userMeResponse", - (event: CustomEvent<UserMeResponse | false>) => { - this.onUserMe(event.detail); - }, - ); + document.addEventListener("userMeResponse", this._onUserMeResponse); window.addEventListener("pattern-selected", this._onPatternSelected); } disconnectedCallback() { super.disconnectedCallback(); + document.removeEventListener("userMeResponse", this._onUserMeResponse); window.removeEventListener("pattern-selected", this._onPatternSelected); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Store.ts` around lines 40 - 54, The anonymous listener passed to document.addEventListener("userMeResponse", ...) causes a leak because it cannot be removed; create a class property (e.g., this._onUserMeResponse) typed as (event: CustomEvent<UserMeResponse | false>) => void that calls this.onUserMe(event.detail), use that property in connectedCallback to add the listener and in disconnectedCallback to remove it, ensuring the same function reference is used so onUserMe()/fetchCosmetics() aren’t invoked multiple times after reconnection.
298-319:⚠️ Potential issue | 🟡 MinorUse cosmetic display name in the flag toast, not the storage key.
The toast message extracts
namefrom the storage key (e.g.ancient_rome). Consider looking upthis.cosmetics?.flags[nameKey]?.nameto show the proper display name to users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Store.ts` around lines 298 - 319, The toast uses the storage key (derived as `name`) instead of the user-facing cosmetic name; update selectFlag to compute the key portion (e.g. nameKey = flagKey.includes(":") ? flagKey.slice(flagKey.indexOf(":")+1) : flagKey), then lookup a displayName from this.cosmetics?.flags?.[nameKey]?.name (falling back to the original nameKey if missing) and use that displayName in the "show-message" CustomEvent detail; keep storing the original flagKey in localStorage and keep calling this.close().
124-126:⚠️ Potential issue | 🟠 MajorFiltering out owned items prevents users from selecting them.
Both loops
continuewhenrel === "owned", so already-owned patterns/flags and the default pattern never render in this modal. Users cannot switch back to an owned item from here.🔧 Suggested fix
- if (rel === "blocked" || rel === "owned") { + if (rel === "blocked") { continue; }And similarly for flags:
- if (rel === "blocked" || rel === "owned") continue; + if (rel === "blocked") continue;Also applies to: 177-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Store.ts` around lines 124 - 126, The loop currently skips items when rel === "owned", which prevents owned patterns/flags and the default pattern from rendering; update the conditional in the loops that reference rel (currently written as if (rel === "blocked" || rel === "owned") { continue; }) to only skip blocked items (e.g., if (rel === "blocked") { continue; }) so owned items and the default pattern are included; apply this change to both occurrences related to patterns and flags (the two places in Store.ts where rel is checked).
🧹 Nitpick comments (4)
src/client/FlagInput.ts (1)
105-130: Asyncupdated()may cause brief visual flash.Making
updated()async meansresolveFlagUrl()runs after render. The preview element will be empty until the promise resolves. For slow network fetches, users might see a flash or blank state.Consider storing the resolved URL in component state and triggering a re-render, or show a placeholder while loading.
♻️ Alternative: use state for resolved URL
+ `@state`() private resolvedFlagUrl: string | null = null; - async updated() { + async updated(changedProperties: Map<string, unknown>) { + if (changedProperties.has("flag")) { + this.resolvedFlagUrl = this.flag + ? await resolveFlagUrl(this.flag) + : null; + } const preview = this.renderRoot.querySelector("#flag-preview") as HTMLElement; if (!preview) return; // ... use this.resolvedFlagUrl instead of awaiting inline }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/FlagInput.ts` around lines 105 - 130, The async updated() causes the preview to be cleared while resolveFlagUrl(this.flag) awaits, producing a flash; instead, make updated() synchronous and move the async resolution into a separate method that sets a component state property (e.g., this.resolvedFlagUrl) after awaiting resolveFlagUrl(this.flag), then call requestUpdate(); change the rendering logic that targets "#flag-preview" (or uses renderRoot) to read this.resolvedFlagUrl and show a placeholder or keep the previous image until the new resolved URL is set; also preserve existing behavior around showSelectLabel and isDefaultFlagValue(this.flag) so you only clear the preview when appropriate.src/client/graphics/layers/NameLayer.ts (1)
217-222: Flag URL handling is now correct.The previous approach built URLs from raw flag refs and broke on
flag:prefixes. The new code assumesplayer.cosmetics.flagis already a resolved URL (handled byresolveFlagUrl()in Cosmetics.ts). This is cleaner.One note: the outer
if (player.cosmetics.flag)on line 215 already guards for truthiness, so the inner checkif (flag !== undefined && flag !== null)on line 217 is redundant.♻️ Remove redundant null check
if (player.cosmetics.flag) { const flag = player.cosmetics.flag; - if (flag !== undefined && flag !== null) { - const flagImg = document.createElement("img"); - applyFlagStyles(flagImg); - flagImg.src = flag; - nameDiv.appendChild(flagImg); - } + const flagImg = document.createElement("img"); + applyFlagStyles(flagImg); + flagImg.src = flag; + nameDiv.appendChild(flagImg); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/NameLayer.ts` around lines 217 - 222, The inner null/undefined check is redundant because the outer guard uses if (player.cosmetics.flag); remove the inner if (flag !== undefined && flag !== null) block and always create and append the flag image inside the outer branch—use the existing flag local (or player.cosmetics.flag) to set flagImg.src, call applyFlagStyles(flagImg), then nameDiv.appendChild(flagImg) (symbols: player.cosmetics.flag, flag, flagImg, applyFlagStyles, nameDiv).src/core/CustomFlag.ts (1)
3-12: Remove this file entirely — the function is not called anywhere.The search shows
renderPlayerFlagexists only as the export definition. No imports or calls remain in the codebase, so keeping this unused stub creates unnecessary dead code. Delete the file to keep the codebase clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/CustomFlag.ts` around lines 3 - 12, The file contains an unused legacy export renderPlayerFlag; remove this dead-code by deleting the renderPlayerFlag function and its module (the CustomFlag.ts file) and ensure there are no remaining imports or references to renderPlayerFlag in the repo; run the typecheck/build to confirm nothing breaks and remove any now-empty import statements that referenced it.src/core/Schemas.ts (1)
471-482: Remove the unreachableval === undefinedcheck.The
FlagNameschema isz.string(), so the refine callback only runs when the value is already a string. Theundefinedcheck is dead code — Zod rejects non-strings before reaching the refine, and when used as.optional()the refine is skipped entirely forundefined.🧹 Suggested cleanup
export const FlagName = z .string() .max(128) .refine( (val) => { - if (val === undefined || val === "") return true; + if (val === "") return true; return val.startsWith("flag:") || val.startsWith("country:"); }, { message: "Invalid flag: must start with country: or flag:", }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Schemas.ts` around lines 471 - 482, The refine callback on FlagName contains a dead check for val === undefined; because FlagName is declared with z.string(), the refine only receives strings (and optional undefined is handled by .optional() before refine), so remove the undefined check and simplify the predicate to only handle the empty-string case and the prefix checks (e.g., replace the body with: if (val === "") return true; return val.startsWith("flag:") || val.startsWith("country:")). Update the refine for the FlagName constant accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/FlagInputModal.ts`:
- Around line 33-37: The search filter currently tests the raw slug (flag.name)
when building cosmeticFlags in FlagInputModal, so users searching the prettified
label (as rendered by <flag-button>) won't match; update the filter inside the
Object.entries(...).filter call to compute the display name (e.g., split on "_"
and capitalize words) and call this.includedInSearch with that displayName
(and/or include both displayName and flag.name) instead of only flag.name,
keeping the ownership check via flagRelationship(flag, this.userMe, null) ===
"owned".
In `@src/client/Store.ts`:
- Around line 40-54: The anonymous listener passed to
document.addEventListener("userMeResponse", ...) causes a leak because it cannot
be removed; create a class property (e.g., this._onUserMeResponse) typed as
(event: CustomEvent<UserMeResponse | false>) => void that calls
this.onUserMe(event.detail), use that property in connectedCallback to add the
listener and in disconnectedCallback to remove it, ensuring the same function
reference is used so onUserMe()/fetchCosmetics() aren’t invoked multiple times
after reconnection.
- Around line 298-319: The toast uses the storage key (derived as `name`)
instead of the user-facing cosmetic name; update selectFlag to compute the key
portion (e.g. nameKey = flagKey.includes(":") ?
flagKey.slice(flagKey.indexOf(":")+1) : flagKey), then lookup a displayName from
this.cosmetics?.flags?.[nameKey]?.name (falling back to the original nameKey if
missing) and use that displayName in the "show-message" CustomEvent detail; keep
storing the original flagKey in localStorage and keep calling this.close().
- Around line 124-126: The loop currently skips items when rel === "owned",
which prevents owned patterns/flags and the default pattern from rendering;
update the conditional in the loops that reference rel (currently written as if
(rel === "blocked" || rel === "owned") { continue; }) to only skip blocked items
(e.g., if (rel === "blocked") { continue; }) so owned items and the default
pattern are included; apply this change to both occurrences related to patterns
and flags (the two places in Store.ts where rel is checked).
In `@src/core/CosmeticSchemas.ts`:
- Line 79: The CosmeticsSchema's required "flags" property causes safeParse to
fail when the JSON omits it; update the schema in CosmeticSchemas.ts by making
the "flags" field optional with a default empty record (use z.record(z.string(),
FlagSchema).optional().default({}) or the equivalent
z.optional(...).default({})) so cosmetics load when "flags" is missing; adjust
any downstream code that assumes flags is always present if needed to rely on
the default.
In `@src/server/Privilege.ts`:
- Around line 228-238: The flare membership check in isFlagAllowed uses
found.name while the dictionary lookup uses key, causing mismatches (e.g.,
cosmetics.flags["us"] with name "united_states"); change the check to compare
against the lookup key instead of found.name by replacing the
flares.includes(`flag:${found.name}`) test with flares.includes(`flag:${key}`)
while keeping the existing flag:* branch and returning found.url on match.
---
Nitpick comments:
In `@src/client/FlagInput.ts`:
- Around line 105-130: The async updated() causes the preview to be cleared
while resolveFlagUrl(this.flag) awaits, producing a flash; instead, make
updated() synchronous and move the async resolution into a separate method that
sets a component state property (e.g., this.resolvedFlagUrl) after awaiting
resolveFlagUrl(this.flag), then call requestUpdate(); change the rendering logic
that targets "#flag-preview" (or uses renderRoot) to read this.resolvedFlagUrl
and show a placeholder or keep the previous image until the new resolved URL is
set; also preserve existing behavior around showSelectLabel and
isDefaultFlagValue(this.flag) so you only clear the preview when appropriate.
In `@src/client/graphics/layers/NameLayer.ts`:
- Around line 217-222: The inner null/undefined check is redundant because the
outer guard uses if (player.cosmetics.flag); remove the inner if (flag !==
undefined && flag !== null) block and always create and append the flag image
inside the outer branch—use the existing flag local (or player.cosmetics.flag)
to set flagImg.src, call applyFlagStyles(flagImg), then
nameDiv.appendChild(flagImg) (symbols: player.cosmetics.flag, flag, flagImg,
applyFlagStyles, nameDiv).
In `@src/core/CustomFlag.ts`:
- Around line 3-12: The file contains an unused legacy export renderPlayerFlag;
remove this dead-code by deleting the renderPlayerFlag function and its module
(the CustomFlag.ts file) and ensure there are no remaining imports or references
to renderPlayerFlag in the repo; run the typecheck/build to confirm nothing
breaks and remove any now-empty import statements that referenced it.
In `@src/core/Schemas.ts`:
- Around line 471-482: The refine callback on FlagName contains a dead check for
val === undefined; because FlagName is declared with z.string(), the refine only
receives strings (and optional undefined is handled by .optional() before
refine), so remove the undefined check and simplify the predicate to only handle
the empty-string case and the prefix checks (e.g., replace the body with: if
(val === "") return true; return val.startsWith("flag:") ||
val.startsWith("country:")). Update the refine for the FlagName constant
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aac5e010-bf91-4b48-8846-ea5d197d26f1
📒 Files selected for processing (22)
index.htmlresources/lang/en.jsonsrc/client/Api.tssrc/client/Cosmetics.tssrc/client/FlagInput.tssrc/client/FlagInputModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/components/FlagButton.tssrc/client/components/PatternButton.tssrc/client/components/PurchaseButton.tssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/CosmeticSchemas.tssrc/core/CustomFlag.tssrc/core/Schemas.tssrc/core/game/UserSettings.tssrc/server/Privilege.tstests/CosmeticRelationship.test.tstests/Privilege.test.ts
✅ Files skipped from review due to trivial changes (4)
- src/client/LangSelector.ts
- index.html
- resources/lang/en.json
- tests/CosmeticRelationship.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/core/game/UserSettings.ts
- src/client/components/PatternButton.ts
- src/client/Api.ts
- tests/Privilege.test.ts
- src/client/components/PurchaseButton.ts
- src/client/Main.ts
- src/client/components/FlagButton.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.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME