unset pattern on logout #1873
unset pattern on logout #1873
Conversation
WalkthroughAdded null handling in TerritoryPatternsModal.onUserMe: when userMeResponse is null, it clears the selected pattern in userSettings, resets the internal selectedPattern to undefined, then continues to load patterns and run the existing restoration logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant M as TerritoryPatternsModal
participant US as UserService
participant S as UserSettings
participant PS as PatternsService
U->>M: Open modal
M->>US: onUserMe()
US-->>M: userMeResponse (nullable)
alt userMeResponse is null
M->>S: Clear selectedPattern
note right of M: Set internal selectedPattern = undefined
M->>PS: Load patterns
PS-->>M: patterns
M->>M: Run existing restoration logic
else userMeResponse not null
M->>PS: Load patterns
PS-->>M: patterns
M->>M: Run existing restoration logic
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 (
|
github-project-automation
bot
moved this from Triage
to Complete
in OpenFront Release Management
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
98-109: Bug: inconsistent null/undefined checks forpattern.productElsewhere you use
product !== nullto mean “locked/purchasable.” Here you use!== undefined, which will also treat free patterns (null) as blocked. Tooltip and blocking will be inconsistent.Apply this diff to align semantics:
- if (this.hoveredPattern && this.hoveredPattern.product !== undefined) { + if (this.hoveredPattern && this.hoveredPattern.product !== null) {Also recommend typing
Pattern['product']as a strict union:Product | null(notundefined) and using=== null/!== nullconsistently.
🧹 Nitpick comments (2)
src/client/TerritoryPatternsModal.ts (2)
112-168: DRY up “locked” logic and make intent explicitYou repeat
pattern.product === null/!== nullchecks across classes, click handler, and purchase button. Compute once to avoid drift and make the code easier to read.private renderPatternButton(pattern: Pattern): TemplateResult { - const isSelected = this.selectedPattern?.name === pattern.name; + const isSelected = this.selectedPattern?.name === pattern.name; + const isLocked = pattern.product !== null; // product present => needs purchase return html` <div style="flex: 0 1 calc(25% - 1rem); max-width: calc(25% - 1rem);"> <button class="border p-2 rounded-lg shadow text-black dark:text-white text-left w-full ${isSelected ? "bg-blue-500 text-white" : "bg-gray-100 hover:bg-gray-200 dark:bg-gray-800 dark:hover:bg-gray-700"} - ${pattern.product !== null ? "opacity-50 cursor-not-allowed" : ""}" - @click=${() => - pattern.product === null && this.selectPattern(pattern)} + ${isLocked ? "opacity-50 cursor-not-allowed" : ""}" + @click=${() => !isLocked && this.selectPattern(pattern)} @mouseenter=${(e: MouseEvent) => this.handleMouseEnter(pattern, e)} @mousemove=${(e: MouseEvent) => this.handleMouseMove(e)} @mouseleave=${() => this.handleMouseLeave()} > ... </button> - ${pattern.product !== null + ${isLocked ? html` <button class="w-full mt-2 px-3 py-1 bg-green-500 hover:bg-green-600 text-white text-xs font-medium rounded transition-colors" @click=${(e: Event) => { e.stopPropagation(); handlePurchase(pattern.product!.priceId); }} > ${translateText("territory_patterns.purchase")} (${pattern.product!.price}) </button> ` : null} </div> `; }
248-256: Nit: use self-closing img tagMinor HTML hygiene for Lit templates.
- <img src="${generatePreviewDataUrl(pattern, width, height)}"></img> + <img src="${generatePreviewDataUrl(pattern, width, height)}" />
📜 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(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
PR: openfrontio/OpenFrontIO#1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/TerritoryPatternsModal.ts
⏰ 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). (2)
- GitHub Check: Deploy to openfront.dev
- GitHub Check: 🔬 Test
🔇 Additional comments (2)
src/client/TerritoryPatternsModal.ts (2)
49-52: Good fix: clearing selected pattern on logout matches the PR objectiveSetting both UserSettings and local state to undefined on null user is correct and prevents stale selection from blocking gameplay.
53-59: patterns() accepts null—no extra guard neededThe
patternsfunction insrc/client/Cosmetics.tsis declared asexport async function patterns( userMe: UserMeResponse | null, ): Promise<Map<string, Pattern>> { … }so it already accepts
null. Likewise,UserSettings.setSelectedPatternNametakesstring | undefined, so passingundefinedhere is fine.Optional: if you want to skip the patterns fetch for logged-out users, you could early-return after clearing state—but it’s not required for type safety.
## Description: During logout, the pattern was still set, so you would fail to join any game. This PR clears & unsets the chosen pattern on logout. ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: evan
## Description: During logout, the pattern was still set, so you would fail to join any game. This PR clears & unsets the chosen pattern on logout. ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: evan
Description:
During logout, the pattern was still set, so you would fail to join any game. This PR clears & unsets the chosen pattern on logout.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan