Show random pattern on end screen #1930
Conversation
WalkthroughRenames Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant WinModal
participant UserService as getUserMe
participant Cosmetics as fetchPatterns
participant PatternBtn as PatternButton
participant Purchase as handlePurchase
User->>WinModal: open()
activate WinModal
WinModal->>UserService: getUserMe()
UserService-->>WinModal: UserMeResponse|null
WinModal->>Cosmetics: fetchPatterns(userMe)
Cosmetics-->>WinModal: Map<string,Pattern>
WinModal->>WinModal: filter purchasable, pick random
WinModal->>PatternBtn: render PatternButton(pattern, onPurchase)
WinModal-->>User: show modal (fade-in)
User->>PatternBtn: click Purchase
PatternBtn-->>WinModal: onPurchase(priceId)
WinModal->>Purchase: handlePurchase(priceId)
Purchase-->>WinModal: success/fail
deactivate WinModal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
evanpelle
force-pushed
the
evan-pattern-end
branch
from
ed3acbc to
a00f637
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/Cosmetics.ts (1)
5-31: Fix null-access on flares and avoid mutating shared cosmetics objects
- If
userMeisnull,userMe?.player.flareswill still try to read.flaresoffundefinedand crash. Also,new Set(undefined)would throw. Guard both.- Clobbering
patternData.product = nullmutates the object stored incosmetics.patterns. That can leak into other consumers. Clone instead.Apply:
const patterns: Map<string, Pattern> = new Map(); - const playerFlares = new Set(userMe?.player.flares); + // userMe can be null; guard the entire chain and default to empty. + const playerFlares = new Set(userMe?.player?.flares ?? []); for (const name in cosmetics.patterns) { const patternData = cosmetics.patterns[name]; const hasAccess = playerFlares.has(`pattern:${name}`); if (hasAccess) { - // Remove product info because player already has access. - patternData.product = null; - patterns.set(name, patternData); + // Avoid mutating the source cosmetics object. + patterns.set(name, { ...patternData, product: null }); } else if (patternData.product !== null) { // Player doesn't have access, but product is available for purchase. patterns.set(name, patternData); }
🧹 Nitpick comments (4)
src/client/TerritoryPatternsModal.ts (4)
23-23: Initialize state to avoid strict property init errors and undefined readsInitialize
selectedPatterntonullso renders don’t seeundefinedand to satisfystrictPropertyInitialization.- @state() private selectedPattern: Pattern | null; + @state() private selectedPattern: Pattern | null = null;
97-104: Use Lit’s repeat + stable callbacks to avoid re-creating nodes and closures each render
- The loop builds an array and creates new arrow functions every render; that changes function identity and can cause unnecessary re-renders in child elements.
- Prefer the
repeatdirective keyed by pattern name, and hoist handlers as class fields.Diff inside this block:
- buttons.push(html` - <pattern-button - .pattern=${pattern} - .onSelect=${(p: Pattern | null) => this.selectPattern(p)} - .onPurchase=${(priceId: string) => handlePurchase(priceId)} - ></pattern-button> - `); + buttons.push(html` + <pattern-button + .pattern=${pattern} + .onSelect=${this.handleSelect} + .onPurchase=${this.handlePurchase} + ></pattern-button> + `);And add once (outside this range):
// imports import { repeat } from "lit/directives/repeat.js"; // class fields private handleSelect = (p: Pattern | null) => this.selectPattern(p); private handlePurchase = (priceId: string) => handlePurchase(priceId); // renderPatternGrid replacement for the for..of: const buttons = repeat( this.patterns, ([name]) => name, ([, pattern]) => html` <pattern-button .pattern=${pattern} .onSelect=${this.handleSelect} .onPurchase=${this.handlePurchase} ></pattern-button> `, );
111-116: Default/null tile is good; minor cleanup and consistency with Tailwind
- Keep the default tile first—good UX.
- You don’t need to pass
onPurchasewhen.pattern=${null}; it won’t be used.- Prefer Tailwind over inline style for the container above for consistency.
- <pattern-button - .pattern=${null} - .onSelect=${(p: Pattern | null) => this.selectPattern(p)} - .onPurchase=${(priceId: string) => handlePurchase(priceId)} - ></pattern-button> + <pattern-button + .pattern=${null} + .onSelect=${this.handleSelect} + ></pattern-button>Container (outside this range) suggestion:
<div class="flex flex-wrap gap-4 p-2 justify-center items-start"> ... </div>
145-150: Selection flow is clear; consider emitting a custom event for compositionPersist + refresh + close is fine. For better composition with parents (and tests), emit an event so listeners don’t need to reach into this element to track changes.
private selectPattern(pattern: Pattern | null) { this.userSettings.setSelectedPatternName(pattern?.name); this.selectedPattern = pattern; this.dispatchEvent(new CustomEvent("pattern-changed", { detail: { pattern } })); this.refresh(); this.close(); }I can also add a small test to assert the event fires with the right payload.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
resources/lang/en.json(1 hunks)src/client/Cosmetics.ts(1 hunks)src/client/TerritoryPatternsModal.ts(5 hunks)src/client/components/PatternButton.ts(1 hunks)src/client/graphics/layers/WinModal.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/components/PatternButton.ts
- src/client/graphics/layers/WinModal.ts
- resources/lang/en.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/TerritoryPatternsModal.ts (2)
src/core/CosmeticSchemas.ts (1)
Pattern(6-6)src/client/Cosmetics.ts (2)
fetchPatterns(5-31)handlePurchase(33-66)
🔇 Additional comments (2)
src/client/TerritoryPatternsModal.ts (2)
10-11: Good call: side-effect import for custom element + centralizing data accessImporting
./components/PatternButtonregisters the element, and pullingfetchPatterns/handlePurchasefrom one place keeps concerns separated. No issues spotted.
46-54: Logout path clears selection; restore path is safeSetting storage to
undefinedandselectedPatterntonullon logout is correct. Restoring from storage with?? nullis safe if the pattern was removed or becomes unavailable.If storage might contain a now-unpurchasable pattern, confirm desired UX: keep selection (even if hidden) vs. coerce to
null. Current code coerces tonull. If you want to show a “previously selected, now unavailable” tile, we can surface a disabled card instead.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/TerritoryPatternsModal.ts (2)
151-159: Unused method should be removed.The
renderPatternPreviewmethod appears to be dead code - it's only called in therefresh()method at line 162-166, but the result is stored in a local variablepreviewthat is never used. The actual preview rendering has been delegated to the PatternButton component.Remove the unused method and its invocation:
- private renderPatternPreview( - pattern?: string, - width?: number, - height?: number, - ): TemplateResult { - return html` - <img src="${generatePreviewDataUrl(pattern, width, height)}"></img> - `; - } public async refresh() { - const preview = this.renderPatternPreview( - this.selectedPattern?.pattern, - 48, - 48, - ); this.requestUpdate();
178-180: Dead code should be removed.These lines try to render to
this.previewButtonwhich is never set (always null). This appears to be leftover code from the previous implementation.Remove the dead code:
- if (this.previewButton === null) return; - render(preview, this.previewButton); - this.requestUpdate();Also remove the unused property declaration at line 21:
- public previewButton: HTMLElement | null = null;
🧹 Nitpick comments (3)
src/client/TerritoryPatternsModal.ts (3)
56-70: Easter egg implementation is functional but could be improved.The choco pattern easter egg works, but consider these improvements:
- The popup styling might not work without corresponding CSS
- The popup is appended directly to document.body without cleanup on component disconnect
Consider managing the popup lifecycle better:
private triggerChocoEasterEgg() { console.log("🍫 Choco pattern unlocked!"); this.showChocoPattern = true; const popup = document.createElement("div"); - popup.className = "easter-egg-popup"; + popup.className = "fixed top-4 left-1/2 transform -translate-x-1/2 bg-green-500 text-white px-6 py-3 rounded-lg shadow-lg z-50"; popup.textContent = "🎉 You unlocked the Choco pattern!"; document.body.appendChild(popup); setTimeout(() => { - popup.remove(); + popup?.remove(); }, 5000); this.requestUpdate(); }Also consider tracking and cleaning up the popup on disconnect:
+private easterEggPopup: HTMLElement | null = null; disconnectedCallback() { window.removeEventListener("keydown", this.handleKeyDown); + this.easterEggPopup?.remove(); super.disconnectedCallback(); }
108-109: Consider using Tailwind classes instead of inline styles.Since you're switching to Tailwind, use utility classes for consistency.
- <div - class="flex flex-wrap gap-4 p-2" - style="justify-content: center; align-items: flex-start;" - > + <div + class="flex flex-wrap gap-4 p-2 justify-center items-start" + >
88-90: Maintain awareness of global styles when using light DOMThe override of
createRenderRoot()inTerritoryPatternsModal.ts(lines 88–90) disables Shadow DOM, just like in many other components across the codebase. This is a deliberate choice to let Tailwind utility classes penetrate the component, but it comes at the cost of style encapsulation.• Add a brief comment above the override explaining that Shadow DOM is disabled intentionally for Tailwind integration.
• Be extra cautious with class names and selectors here to avoid unintended styling side-effects from global CSS.
• (Optional) Consider using more specific utility combinations or:host-scoped styles if you need partial encapsulation in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/client/TerritoryPatternsModal.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/TerritoryPatternsModal.ts (3)
src/core/CosmeticSchemas.ts (1)
Pattern(6-6)src/client/Cosmetics.ts (2)
fetchPatterns(5-31)handlePurchase(33-66)src/client/components/PatternButton.ts (1)
handlePurchase(57-62)
🔇 Additional comments (4)
src/client/TerritoryPatternsModal.ts (4)
10-11: LGTM! Clean imports and good component-based refactoring.The refactoring to use the reusable
PatternButtoncomponent and renaming tofetchPatternsimproves code organization and reusability.
23-23: Good type change fromundefinedtonullfor consistency.Using
nullinstead ofundefinedfor the default pattern state is more explicit and aligns better with the component's API.
111-114: Default pattern button looks good.Setting the pattern to
nullfor the default option is clean and consistent with the new typing approach.
97-103: PatternButton translation logic confirmedThe PatternButton component still defines a private translatePatternName method and calls translateText(
${prefix}.${patternName}) to localize each pattern name (and falls back to the default key), so name translation remains fully covered. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/TerritoryPatternsModal.ts (1)
121-128: Ensure translation keys are added toen.jsonThe modal and pattern buttons reference keys that aren’t defined in
resources/lang/en.json, which will break translations at runtime. Please add the following entries with appropriate English values:•
territory_patterns.title
•territory_patterns.pattern.default
•territory_patterns.purchaseThese are used in:
src/client/TerritoryPatternsModal.ts(lines 121–128)PatternButtoncomponents elsewhere
🧹 Nitpick comments (11)
src/client/TerritoryPatternsModal.ts (5)
22-22: Initialize reactive state to a safe default (null).Without an initializer, TypeScript’s strictPropertyInitialization may complain, and Lit will start with
undefinedrather than your intendednull.Apply this diff:
- @state() private selectedPattern: Pattern | null; + @state() private selectedPattern: Pattern | null = null;
2-2: Remove dead preview plumbing and unused imports. It’s no longer rendered.
previewButtonis never assigned, and the preview generation/rendering inrefresh()never reaches the DOM. This adds complexity and work on every refresh for no visible effect. Let’s remove it and the associated imports.Apply this diff:
-import { html, LitElement, render } from "lit"; +import { html, LitElement } from "lit"; @@ -import { renderPatternPreview } from "./components/PatternButton"; @@ - public previewButton: HTMLElement | null = null; + // (removed) previewButton: no longer needed; previews are handled by PatternButton tiles @@ - public async refresh() { - const preview = renderPatternPreview( - this.selectedPattern?.pattern ?? null, - 48, - 48, - ); - this.requestUpdate(); + public async refresh() { + this.requestUpdate(); @@ - if (this.previewButton === null) return; - render(preview, this.previewButton); - this.requestUpdate(); + // Preview injection removed; PatternButton handles previews. }Also applies to: 9-9, 20-21, 151-170
143-148: Avoid modal flicker: don’t re-open the modal during selection.
selectPattern()callsrefresh()(which opens the modal) and thenclose(). This can cause a quick reopen-close flicker. Persist selection and close directly.Apply this diff:
- private selectPattern(pattern: Pattern | null) { - this.userSettings.setSelectedPatternName(pattern?.name); - this.selectedPattern = pattern; - this.refresh(); - this.close(); - } + private selectPattern(pattern: Pattern | null) { + this.userSettings.setSelectedPatternName(pattern?.name); + this.selectedPattern = pattern; + // No need to refresh here; just close and let consumers react to settings. + this.close(); + this.requestUpdate(); + }
87-90: Light DOM rendering: confirm this is intentional and safe with your CSS strategy.Returning
thisfromcreateRenderRoot()opts out of Shadow DOM. This allows Tailwind/global CSS to style contents, but also allows global CSS to leak in. If isolation is desired later, prefer Shadow DOM and expose only needed parts via CSS parts or custom properties.Would you like a variant that keeps Shadow DOM while still allowing Tailwind tokens via CSS custom properties?
71-85: Easter egg popup has no styles defined; consider minimal inline styling to ensure it’s readable.You append a
divwith classeaster-egg-popup, but no styles are guaranteed at runtime. A tiny inline style can ensure basic visibility without adding global CSS.Example:
- popup.className = "easter-egg-popup"; - popup.textContent = "🎉 You unlocked the Choco pattern!"; + popup.className = "easter-egg-popup"; + popup.textContent = "🎉 You unlocked the Choco pattern!"; + Object.assign(popup.style, { + position: "fixed", + bottom: "16px", + left: "50%", + transform: "translateX(-50%)", + background: "rgba(0,0,0,0.8)", + color: "#fff", + padding: "8px 12px", + borderRadius: "8px", + zIndex: "9999", + fontWeight: "600", + });If you already have a Tailwind utility for this class elsewhere, feel free to ignore.
src/client/components/PatternButton.ts (6)
8-8: Match preview size with container: avoid extra scaling.Container is
w-[120px] h-[120px], butBUTTON_WIDTHis 150, causing browser downscaling every render. Align them to reduce work and sharpen pixels.Apply this diff:
-export const BUTTON_WIDTH = 150; +export const BUTTON_WIDTH = 120; // Keep in sync with container size
55-61: Buttons should be explicit and accessible: add type and ARIA.
- Default button type is “submit” inside forms; make it
type="button".- Mirror the disabled state with
aria-disabledfor ATs.Apply this diff:
- <button + <button + type="button" class="bg-white/90 border-2 border-black/10 rounded-lg p-2 cursor-pointer transition-all duration-200 w-full hover:bg-white hover:-translate-y-0.5 hover:shadow-lg hover:shadow-black/20 disabled:opacity-50 disabled:cursor-not-allowed disabled:hover:translate-y-0 disabled:hover:shadow-none" ?disabled=${isPurchasable} + aria-disabled="${isPurchasable ? "true" : "false"}" @click=${this.handleClick} > @@ - <button + <button + type="button" class="w-full px-4 py-2 bg-green-500 text-white border-0 rounded-md text-sm font-semibold cursor-pointer transition-colors duration-200 hover:bg-green-600" @click=${this.handlePurchase} >Also applies to: 84-91
99-115: Graceful fallback on decode errors and more helpful alt text.If decoding fails,
generatePreviewDataUrlreturns"", which produces a broken-image icon. Also, the alt text can include the pattern name for screen readers.Apply this diff:
export function renderPatternPreview( pattern: string | null, width: number, height: number, ): TemplateResult { if (pattern === null) { return renderBlankPreview(width, height); } - const dataUrl = generatePreviewDataUrl(pattern, width, height); - return html`<img - src="${dataUrl}" - alt="Pattern preview" - class="w-full h-full object-contain" - style="image-rendering: pixelated; image-rendering: -moz-crisp-edges; image-rendering: crisp-edges;" - />`; + const dataUrl = generatePreviewDataUrl(pattern, width, height); + if (!dataUrl) { + return renderBlankPreview(width, height); + } + const name = "pattern"; + return html`<img + src="${dataUrl}" + alt="${name} preview" + class="w-full h-full object-contain" + style="image-rendering: pixelated; image-rendering: -moz-crisp-edges; image-rendering: crisp-edges;" + />`; }If you want the actual name, you can pass it as a param or bind it at the call site.
21-24: Light DOM rendering: confirm trade-offs.As with the modal, returning
thisdisables Shadow DOM. If you hit style conflicts later, consider Shadow DOM with exposed parts (part=) for theming.
25-31: Minor: friendlier fallback for untranslated names.Current fallback capitalizes only the first char. Replacing dashes/underscores improves readability (e.g., "star_field" → "Star field").
Example:
- return patternName[0].toUpperCase() + patternName.substring(1); + const pretty = patternName.replace(/[-_]+/g, " "); + return pretty.charAt(0).toUpperCase() + pretty.slice(1);
153-216: Optional: add environment guard for tests/non-DOM contexts.If this runs in a test or SSR context without
document, canvas creation will fail. A quick guard avoids that.Example:
- // Create the canvas + // Create the canvas + if (typeof document === "undefined") { + return ""; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/client/Cosmetics.ts(2 hunks)src/client/TerritoryPatternsModal.ts(5 hunks)src/client/components/PatternButton.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/Cosmetics.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/client/components/PatternButton.ts (2)
src/client/TerritoryPatternsModal.ts (1)
customElement(13-171)src/core/PatternDecoder.ts (3)
PatternDecoder(1-56)scaledWidth(53-55)scaledHeight(49-51)
src/client/TerritoryPatternsModal.ts (3)
src/core/CosmeticSchemas.ts (1)
Pattern(6-6)src/client/Cosmetics.ts (2)
fetchPatterns(5-31)handlePurchase(33-66)src/client/components/PatternButton.ts (2)
handlePurchase(40-45)renderPatternPreview(99-114)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (2)
src/client/TerritoryPatternsModal.ts (1)
91-105: Grid rendering looks clean and composable.Good move to rely on
<pattern-button>for composition instead of duplicating layout/preview logic here.src/client/components/PatternButton.ts (1)
33-38: Good: selection logic uses composition.Blocking selection when purchasable and delegating purchase via a callback is a clean composition pattern. Keeps purchase flow outside this component.
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
Description:
To advertise patterns, show a random, purchasable pattern on the end screen.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan