Refactor modal system: BaseModal renders shell, unified open(args) API#3923
Merged
Conversation
Contributor
WalkthroughThis PR refactors multiple modal components to use BaseModal hooks (modalConfig, renderHeaderSlot, renderBody, onOpen/onClose/onTabEnter) and updates modal lifecycle/close handling and token-based opening flows. Callsites were updated to pass option objects into open(args). ChangesModal refactor to hook-based rendering and lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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/JoinLobbyModal.ts (1)
108-108:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd missing
modalContainerClassproperty toBaseModal.The code references
this.modalContainerClassbut TypeScript compilation fails because this property is not defined inBaseModalor accessible to any subclass. This blocks deployment across all modal components.The property is used in 8 files (JoinLobbyModal at lines 108 and 227, plus Matchmaking, RankedModal, SinglePlayerModal, HostLobbyModal, AccountModal). Define
modalContainerClassas a protected property inBaseModalwith a value containing appropriate CSS classes for modal container styling.Also applies to: 227-227, and Matchmaking.ts:39, RankedModal.ts:83, SinglePlayerModal.ts:228, HostLobbyModal.ts:262, AccountModal.ts:76, AccountModal.ts:107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/JoinLobbyModal.ts` at line 108, Add a protected property modalContainerClass to the BaseModal class so subclasses (JoinLobbyModal, Matchmaking, RankedModal, SinglePlayerModal, HostLobbyModal, AccountModal) can access it; declare it as protected modalContainerClass: string = '<appropriate CSS classes for modal container styling>'; update BaseModal (the class defining common modal behavior and methods like render()/template rendering) to include this property so references like this.modalContainerClass compile cleanly across all modal components.
🧹 Nitpick comments (5)
src/client/JoinLobbyModal.ts (1)
289-291: ⚖️ Poor tradeoffTrack the signature migration to completion.
The
@ts-expect-errorsuppression and comment document intentional technical debt during the modal refactor rollout. Ensure this migration toopen({ lobbyId, lobbyInfo })is tracked and completed to avoid leaving partial migration state in the codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/JoinLobbyModal.ts` around lines 289 - 291, The JoinLobbyModal.open method currently uses a legacy positional signature and suppresses TypeScript errors; update the API to the new object signature open({ lobbyId, lobbyInfo }) by changing the JoinLobbyModal.open declaration accordingly (removing the `@ts-expect-error`), update its parameter typing to accept a single object with lobbyId: string and lobbyInfo?: GameInfo | PublicGameInfo, and then find and update all callers that invoke JoinLobbyModal.open(...) to pass an object instead of positional args; also adjust any overrides of BaseModal.open to match the BaseModal.open(args) signature and remove this migration comment/TODO once callers and types compile cleanly.src/client/TroubleshootingModal.ts (1)
215-226: ⚡ Quick winConsider using onClose() hook instead of overriding close().
The current
close()override duplicates logic fromBaseModal.close()(unregister handler, callonClose()). This creates maintenance risk if the base implementation changes.Since the only difference is navigating to
"page-help"instead of"page-play", consider:
- Moving the
showPagelogic toonClose()- Adding a protected
getInlineReturnPage()hook that defaults to"page-play"and returns"page-help"here🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/TroubleshootingModal.ts` around lines 215 - 226, The TroubleshootingModal.close() override duplicates BaseModal.close() behavior (unregisterEscapeHandler and onClose), which risks divergence; instead remove the override and implement the navigation change in onClose() or add a protected hook getInlineReturnPage() used by BaseModal.close(); specifically, update BaseModal.close() to call this.getInlineReturnPage() when determing which page to show for inline modals, add a protected getInlineReturnPage() defaulting to "page-play", and override getInlineReturnPage() in TroubleshootingModal to return "page-help" (or move the window.showPage("page-help") into TroubleshootingModal.onClose()), so you only change the return page logic without duplicating close() implementation.src/client/LeaderboardModal.ts (1)
43-70: 💤 Low valueNote: activeTab type safety reduced.
The refactor removes the local typed declaration
activeTab: "players" | "clans"and relies onBaseModal.activeTab: string. WhileBaseModal.setActiveTab()validates keys at runtime, TypeScript can no longer catch invalid tab references at compile time.Consider adding a generic type parameter to
BaseModal<TabKey extends string>for better type safety across modal implementations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/LeaderboardModal.ts` around lines 43 - 70, The refactor lost compile-time safety for the tab keys because LeaderboardModal now inherits BaseModal.activeTab as a plain string; make BaseModal generic (e.g. BaseModal<TabKey extends string>) and update its setActiveTab and activeTab declarations to use that generic so modal subclasses can constrain keys; then change LeaderboardModal to extend BaseModal<'players' | 'clans'> (and update any other modal classes similarly) so activeTab, setActiveTab, and local checks in functions like loadActiveTabData remain type-safe and TypeScript will catch invalid tab references.src/client/TokenLoginModal.ts (1)
88-105: 💤 Low valueSilent no-op when
open()is called without a token may surprise callers.If something other than
openWithToken()invokesopen(), it returns silently with no log or thrown error. That makes integration bugs hard to spot. A smallconsole.warn(or an explicitthrow) would make the contract clearer, sinceopenWithToken()is the only intended entry point.🧹 Suggested tiny diagnostic
public open(args?: Record<string, unknown>): void { if (!this.token) { + console.warn("TokenLoginModal.open() called without a token; use openWithToken() instead."); return; } super.open(args); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/TokenLoginModal.ts` around lines 88 - 105, The open() method in TokenLoginModal currently no-ops when this.token is falsy, which hides misuse; change it to surface the issue by either logging a warning or throwing an error: modify TokenLoginModal.open to detect missing token and call console.warn (or throw a descriptive Error) explaining that openWithToken() must be used (include the class/method name in the message), then return/throw; keep the existing super.open(args) path when this.token exists and do not alter onOpen/onClose behavior.src/client/UserSettingModal.ts (1)
351-353: 💤 Low valueListener lifecycle is balanced — small note on stale
keySequence.The
keydownlistener is added inonOpenand removed in bothonCloseanddisconnectedCallback, which is clean. One small thing:keySequenceis not reset on close, so a partial sequence (e.g."ev") from one session bleeds into the next. Easy to clear inonCloseif you want each open to start fresh.🧹 Optional reset on close
protected onClose(): void { window.removeEventListener("keydown", this.handleEasterEggKey); + this.keySequence = []; }Also applies to: 932-935
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/UserSettingModal.ts` around lines 351 - 353, The keySequence string used by the Easter-egg keyboard handler (handleEasterEggKey) is not reset when the modal closes, so partial sequences persist across opens; update UserSettingModal.onClose (and similarly in disconnectedCallback if present) to reset keySequence to an empty string (e.g., this.keySequence = "") in addition to removing the window keydown listener, ensuring each onOpen starts with a fresh sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/components/BaseModal.ts`:
- Around line 96-98: The render() method currently mutates state by assigning
this.activeTab when tabs exist; remove that side-effect from render() and ensure
initialization happens in a lifecycle or the existing open() method instead.
Specifically, delete the assignment block in render() that sets this.activeTab =
tabs[0].key and either add the initialization to willUpdate() (checking
tabs.length && this.activeTab === "") or rely solely on the existing logic in
open() (lines that set this.activeTab) so state changes do not occur during
render().
---
Outside diff comments:
In `@src/client/JoinLobbyModal.ts`:
- Line 108: Add a protected property modalContainerClass to the BaseModal class
so subclasses (JoinLobbyModal, Matchmaking, RankedModal, SinglePlayerModal,
HostLobbyModal, AccountModal) can access it; declare it as protected
modalContainerClass: string = '<appropriate CSS classes for modal container
styling>'; update BaseModal (the class defining common modal behavior and
methods like render()/template rendering) to include this property so references
like this.modalContainerClass compile cleanly across all modal components.
---
Nitpick comments:
In `@src/client/JoinLobbyModal.ts`:
- Around line 289-291: The JoinLobbyModal.open method currently uses a legacy
positional signature and suppresses TypeScript errors; update the API to the new
object signature open({ lobbyId, lobbyInfo }) by changing the
JoinLobbyModal.open declaration accordingly (removing the `@ts-expect-error`),
update its parameter typing to accept a single object with lobbyId: string and
lobbyInfo?: GameInfo | PublicGameInfo, and then find and update all callers that
invoke JoinLobbyModal.open(...) to pass an object instead of positional args;
also adjust any overrides of BaseModal.open to match the BaseModal.open(args)
signature and remove this migration comment/TODO once callers and types compile
cleanly.
In `@src/client/LeaderboardModal.ts`:
- Around line 43-70: The refactor lost compile-time safety for the tab keys
because LeaderboardModal now inherits BaseModal.activeTab as a plain string;
make BaseModal generic (e.g. BaseModal<TabKey extends string>) and update its
setActiveTab and activeTab declarations to use that generic so modal subclasses
can constrain keys; then change LeaderboardModal to extend BaseModal<'players' |
'clans'> (and update any other modal classes similarly) so activeTab,
setActiveTab, and local checks in functions like loadActiveTabData remain
type-safe and TypeScript will catch invalid tab references.
In `@src/client/TokenLoginModal.ts`:
- Around line 88-105: The open() method in TokenLoginModal currently no-ops when
this.token is falsy, which hides misuse; change it to surface the issue by
either logging a warning or throwing an error: modify TokenLoginModal.open to
detect missing token and call console.warn (or throw a descriptive Error)
explaining that openWithToken() must be used (include the class/method name in
the message), then return/throw; keep the existing super.open(args) path when
this.token exists and do not alter onOpen/onClose behavior.
In `@src/client/TroubleshootingModal.ts`:
- Around line 215-226: The TroubleshootingModal.close() override duplicates
BaseModal.close() behavior (unregisterEscapeHandler and onClose), which risks
divergence; instead remove the override and implement the navigation change in
onClose() or add a protected hook getInlineReturnPage() used by
BaseModal.close(); specifically, update BaseModal.close() to call
this.getInlineReturnPage() when determing which page to show for inline modals,
add a protected getInlineReturnPage() defaulting to "page-play", and override
getInlineReturnPage() in TroubleshootingModal to return "page-help" (or move the
window.showPage("page-help") into TroubleshootingModal.onClose()), so you only
change the return page logic without duplicating close() implementation.
In `@src/client/UserSettingModal.ts`:
- Around line 351-353: The keySequence string used by the Easter-egg keyboard
handler (handleEasterEggKey) is not reset when the modal closes, so partial
sequences persist across opens; update UserSettingModal.onClose (and similarly
in disconnectedCallback if present) to reset keySequence to an empty string
(e.g., this.keySequence = "") in addition to removing the window keydown
listener, ensuring each onOpen starts with a fresh sequence.
🪄 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: b1ffd9e2-ee86-470b-aa95-afceff304ea9
📒 Files selected for processing (14)
src/client/ClanModal.tssrc/client/FlagInputModal.tssrc/client/HelpModal.tssrc/client/JoinLobbyModal.tssrc/client/LanguageModal.tssrc/client/LeaderboardModal.tssrc/client/Main.tssrc/client/NewsModal.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/TokenLoginModal.tssrc/client/TroubleshootingModal.tssrc/client/UserSettingModal.tssrc/client/components/BaseModal.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/JoinLobbyModal.ts (1)
220-260:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExtra
</div>in the join form template.After the refactor that removed the outer wrapper, line 258 still has a
</div>that has no matching opening<div>inside the<form>. Counting tags from line 221:
<form>→<div flex-col gap-3>(A) →<div flex gap-2>(B) → input + paste<o-button>→</div>closes B → submit<o-button>→</div>closes A →</div>(extra) →</form>.Browsers/Lit's parser may silently absorb the stray close tag, but it's still malformed markup and easy to break the layout on a future tweak. Please remove it.
🐛 Proposed fix
</o-button> </div> - </div> </form>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/JoinLobbyModal.ts` around lines 220 - 260, The template returned in JoinLobbyModal's render contains an extra closing </div> inside the form (after the submit <o-button>), which produces mismatched tags; remove that stray </div> so the form structure is: <form> → div.flex-col (A) → div.flex (B) → </div> (closes B) → submit <o-button> → </div> (closes A) → </form>; update the template in the component (where joinLobbyFromInput, pasteFromClipboard and the lobbyIdInput are defined) to drop the redundant closing tag.
🧹 Nitpick comments (1)
src/client/JoinLobbyModal.ts (1)
263-273: ⚡ Quick winCreate a typed interface for
JoinLobbyModal.open()args to eliminate the unchecked cast.The current code uses
as GameInfo | PublicGameInfo | undefinedwithout type safety, andRecord<string, unknown>obscures the contract. Main.ts calls.open()with either{ lobbyId }or{ lobbyId, lobbyInfo }, so a local typed interface keeps the code self-documenting and removes the cast.Suggested change
+interface JoinLobbyModalOpenArgs { + lobbyId?: string; + lobbyInfo?: GameInfo | PublicGameInfo; +} + - protected onOpen(args?: Record<string, unknown>): void { - const lobbyId = typeof args?.lobbyId === "string" ? args.lobbyId : ""; - const lobbyInfo = args?.lobbyInfo as GameInfo | PublicGameInfo | undefined; + protected onOpen(args?: JoinLobbyModalOpenArgs): void { + const lobbyId = args?.lobbyId ?? ""; + const lobbyInfo = args?.lobbyInfo; if (lobbyId) {If other modals also use structured args (like Store.ts with
affiliateCode), apply the same pattern locally to each. MakingBaseModal.onOpengeneric across all modals would be a larger refactor—worth considering only if this pattern becomes widespread.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/JoinLobbyModal.ts` around lines 263 - 273, Replace the untyped args and unsafe cast in JoinLobbyModal.onOpen with a dedicated typed interface (e.g., JoinLobbyModalOpenArgs { lobbyId?: string; lobbyInfo?: GameInfo | PublicGameInfo }) and update the method signature to onOpen(args?: JoinLobbyModalOpenArgs): void so you can read args.lobbyInfo without using "as"; keep the existing logic that derives lobbyId, calls startTrackingLobby(lobbyId, lobbyInfo) and conditionally calls handleUrlJoin(lobbyId) when lobbyInfo is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/JoinLobbyModal.ts`:
- Around line 80-97: The header always uses "public_lobby.title" after
currentLobbyId is set; update renderHeaderSlot to pick the title based on
isPrivateLobby() when currentLobbyId exists: call modalHeader with title set to
translateText("private_lobby.title") if this.isPrivateLobby() is true, otherwise
translateText("public_lobby.title"), keeping the same onBack, ariaLabel, and
rightContent logic (rightContent stays the copy-button when this.currentLobbyId
&& this.isPrivateLobby()) so the header reflects private vs public lobbies
correctly.
---
Outside diff comments:
In `@src/client/JoinLobbyModal.ts`:
- Around line 220-260: The template returned in JoinLobbyModal's render contains
an extra closing </div> inside the form (after the submit <o-button>), which
produces mismatched tags; remove that stray </div> so the form structure is:
<form> → div.flex-col (A) → div.flex (B) → </div> (closes B) → submit <o-button>
→ </div> (closes A) → </form>; update the template in the component (where
joinLobbyFromInput, pasteFromClipboard and the lobbyIdInput are defined) to drop
the redundant closing tag.
---
Nitpick comments:
In `@src/client/JoinLobbyModal.ts`:
- Around line 263-273: Replace the untyped args and unsafe cast in
JoinLobbyModal.onOpen with a dedicated typed interface (e.g.,
JoinLobbyModalOpenArgs { lobbyId?: string; lobbyInfo?: GameInfo | PublicGameInfo
}) and update the method signature to onOpen(args?: JoinLobbyModalOpenArgs):
void so you can read args.lobbyInfo without using "as"; keep the existing logic
that derives lobbyId, calls startTrackingLobby(lobbyId, lobbyInfo) and
conditionally calls handleUrlJoin(lobbyId) when lobbyInfo is absent.
🪄 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: a2760d61-da5c-4f23-9596-06d74f3258df
📒 Files selected for processing (19)
src/client/AccountModal.tssrc/client/ClanModal.tssrc/client/FlagInputModal.tssrc/client/HelpModal.tssrc/client/HostLobbyModal.tssrc/client/JoinLobbyModal.tssrc/client/LanguageModal.tssrc/client/LeaderboardModal.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/NewsModal.tssrc/client/SinglePlayerModal.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/TokenLoginModal.tssrc/client/TroubleshootingModal.tssrc/client/UserSettingModal.tssrc/client/components/BaseModal.tssrc/client/components/RankedModal.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/client/LanguageModal.ts
- src/client/NewsModal.ts
- src/client/UserSettingModal.ts
- src/client/LeaderboardModal.ts
- src/client/Store.ts
- src/client/TerritoryPatternsModal.ts
- src/client/FlagInputModal.ts
- src/client/HelpModal.ts
- src/client/TroubleshootingModal.ts
- src/client/components/BaseModal.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/client/TokenLoginModal.ts (1)
95-98: ⚡ Quick winStart first login attempt immediately on open.
Line 97 delays the first auth try by 3 seconds every time. Call
tryLogin()once inonOpen()before the interval starts.Proposed change
protected onOpen(): void { clearInterval(this.retryInterval); + void this.tryLogin(); this.retryInterval = setInterval(() => this.tryLogin(), 3000); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/TokenLoginModal.ts` around lines 95 - 98, Call tryLogin immediately when the modal opens: in the onOpen() method (which currently clears this.retryInterval and then sets this.retryInterval = setInterval(() => this.tryLogin(), 3000)), invoke this.tryLogin() once before creating the interval so the first authentication attempt runs right away, then start the periodic retries with setInterval as before; ensure this.retryInterval is still cleared at the start to avoid duplicate intervals.src/client/TroubleshootingModal.ts (1)
216-225: ⚡ Quick winUse
super.close()for the non-inline path to keep lifecycle behavior centralized.The non-inline branch reimplements modal closing directly (
this.modalEl?.close()). Delegating tosuper.close()keeps this modal aligned with BaseModal lifecycle behavior and reduces drift risk as the shared modal API evolves.Suggested refactor
public close(): void { - this.unregisterEscapeHandler(); - this.onClose(); if (this.inline) { + this.unregisterEscapeHandler(); + this.onClose(); this.style.pointerEvents = "none"; window.showPage?.("page-help"); } else { - this.modalEl?.close(); + super.close(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/TroubleshootingModal.ts` around lines 216 - 225, Replace the direct DOM close with the base-class lifecycle: in the else branch, call super.close() instead of this.modalEl?.close(); move the this.unregisterEscapeHandler() and this.onClose() calls so they run only for the inline path (where you still need to set this.style.pointerEvents and window.showPage("page-help")), avoiding duplicate lifecycle invocation if BaseModal.close() already handles unregistering and onClose behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/TroubleshootingModal.ts`:
- Around line 58-60: In TroubleshootingModal.renderBody(), returning html`` when
this.loading is true leaves the modal blank; replace that early return with a
visible loading placeholder (e.g., spinner + "Loading diagnostics..." text)
rendered via the same templating approach and ensure the visible text is wrapped
in translateText("...") and a matching key/value is added to
resources/lang/en.json; update any UI element IDs/classes as needed but keep the
guard on this.loading and render the placeholder inside renderBody() rather than
returning an empty template.
---
Nitpick comments:
In `@src/client/TokenLoginModal.ts`:
- Around line 95-98: Call tryLogin immediately when the modal opens: in the
onOpen() method (which currently clears this.retryInterval and then sets
this.retryInterval = setInterval(() => this.tryLogin(), 3000)), invoke
this.tryLogin() once before creating the interval so the first authentication
attempt runs right away, then start the periodic retries with setInterval as
before; ensure this.retryInterval is still cleared at the start to avoid
duplicate intervals.
In `@src/client/TroubleshootingModal.ts`:
- Around line 216-225: Replace the direct DOM close with the base-class
lifecycle: in the else branch, call super.close() instead of
this.modalEl?.close(); move the this.unregisterEscapeHandler() and
this.onClose() calls so they run only for the inline path (where you still need
to set this.style.pointerEvents and window.showPage("page-help")), avoiding
duplicate lifecycle invocation if BaseModal.close() already handles
unregistering and onClose behavior.
🪄 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: cb457bdc-60cb-4234-9748-c4ced0760d14
📒 Files selected for processing (19)
src/client/AccountModal.tssrc/client/ClanModal.tssrc/client/FlagInputModal.tssrc/client/HelpModal.tssrc/client/HostLobbyModal.tssrc/client/JoinLobbyModal.tssrc/client/LanguageModal.tssrc/client/LeaderboardModal.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/NewsModal.tssrc/client/SinglePlayerModal.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/TokenLoginModal.tssrc/client/TroubleshootingModal.tssrc/client/UserSettingModal.tssrc/client/components/BaseModal.tssrc/client/components/RankedModal.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- src/client/Main.ts
- src/client/TerritoryPatternsModal.ts
- src/client/NewsModal.ts
- src/client/FlagInputModal.ts
- src/client/components/RankedModal.ts
- src/client/Store.ts
- src/client/UserSettingModal.ts
- src/client/AccountModal.ts
- src/client/LeaderboardModal.ts
- src/client/HelpModal.ts
- src/client/HostLobbyModal.ts
- src/client/SinglePlayerModal.ts
- src/client/JoinLobbyModal.ts
- src/client/LanguageModal.ts
- src/client/components/BaseModal.ts
- src/client/ClanModal.ts
Comment on lines
+58
to
60
| protected renderBody() { | ||
| if (this.loading) return html``; | ||
| return html` |
Contributor
There was a problem hiding this comment.
Show a visible loading state instead of an empty body.
Line 59 returns `html```, so users see a blank modal while diagnostics load. Add a small loading placeholder/spinner to avoid “stuck” perception.
As per coding guidelines, src/client/**/*.{ts,tsx} requires all user-visible text to go through translateText() with a matching entry in resources/lang/en.json.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/TroubleshootingModal.ts` around lines 58 - 60, In
TroubleshootingModal.renderBody(), returning html`` when this.loading is true
leaves the modal blank; replace that early return with a visible loading
placeholder (e.g., spinner + "Loading diagnostics..." text) rendered via the
same templating approach and ensure the visible text is wrapped in
translateText("...") and a matching key/value is added to
resources/lang/en.json; update any UI element IDs/classes as needed but keep the
guard on this.loading and render the placeholder inside renderBody() rather than
returning an empty template.
4 tasks
evanpelle
added a commit
that referenced
this pull request
May 14, 2026
## Description Adds a modal URL router so modals can be opened, deep-linked, and bookmarked via the hash. URLs of the form `#modal=<name>&tab=<key>&...` open the named modal and pass remaining keys as args to `onOpen`. The reverse direction also syncs: opening a modal via the UI updates the URL, closing it clears the hash, and switching tabs updates `&tab=`. Builds on the BaseModal refactor from #3923. ### What's new **`ModalRouter.ts`** — small registry + two-way sync helper. - `register(name, { tag, pageId? })` declares a modal as router-managed - `routeFromHash()` parses `#modal=...` and dispatches to `modal.open(args)` - `syncOpened/syncClosed/syncTab` push state back into the URL via `history.replaceState` (no history entries) - A `routingFromUrl` flag prevents URL→modal→URL feedback loops - Unknown modal names silently strip the hash **`BaseModal`** — opt-in URL sync via a `routerName` property. - When set, BaseModal calls into `modalRouter.syncOpened/syncClosed/syncTab` from `open` / `close` / `setActiveTab` - Modals that own their own URL state (lobby modals) just leave `routerName` undefined **`Main.ts`** — registers all routable modals and wires the router. - `handleUrl()`: adds a `modalRouter.routeFromHash()` branch after the path-based lobby join - `onHashUpdate`: when the hash is router-managed, routes via the router instead of tearing down lobby state ### Routable modals 13 inline modals: store, settings, leaderboard, clan, account, help, news, language, single-player, ranked, troubleshooting, territory-patterns, flag-input. Excluded by design: join-lobby, host-lobby (own URL state via `/game/<id>`), matchmaking (no URL state). ### Example uses - Deep link to store flags tab: `/#modal=store&tab=flags` - Settings keybinds tab: `/#modal=settings&tab=keybinds` - Cosmetics.ts now redirects to `#modal=store&tab=packs` when a hard-currency purchase fails for insufficient plutonium (after the alert), so users can top up directly ### URL behavior - `replaceState` everywhere — no history entries added when modals open / close / switch tabs - Browser back/forward still works for the existing path-based game flow - `hashchange` events are router-aware so external hash changes (back button, manual edit) correctly switch between routed modals without tearing down lobby state ## Please complete the following: - [x] I have added screenshots for all UI updates _(no visual changes; smoke-tested in dev)_ - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file _(no new user-visible strings)_ - [ ] I have added relevant tests to the test directory _(no test coverage; manually tested URL load, UI open, tab switch, close, hashchange, insufficient-plutonium redirect)_ - [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: DISCORD_USERNAME
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactors the modal system so that
BaseModalowns the<o-modal>shell rendering, tab state, and lifecycle. Modal subclasses now provide content via small hook methods (renderHeaderSlot(),renderBody(tab),modalConfig()) instead of each rebuilding the<o-modal>template and inline-mode branching.This sets up the foundation for a future modal URL router (e.g.
#modal=store&tab=flags), which will be a follow-up PR.What changed
BaseModal—src/client/components/BaseModal.ts<o-modal>shell itself; subclasses no longer duplicate itactiveTabstate and dispatches per-tab rendering viarenderBody(tab)modalConfig()method returns{ title?, tabs?, hideHeader?, hideCloseButton?, alwaysMaximized?, maxWidth? }open(args?)/close(args?)interface; subclasses interpret args inonOpen(args)/onClose(args)onTabEnter(tab)lifecycle hookopen()soshowPage()re-invocations don't clobber state set by the outer callmodalConfig().tabsso the active tab is highlighted on first open17 modals migrated to the new shape:
confirmBeforeClose/closeAndLeave/closeWithoutLeavingmethods)Per-modal diffs are mostly mechanical:
<o-modal>wrapper template and theif (this.inline) return contentbranch<div class="${this.modalContainerClass}">wrapper (shell styling now lives on<o-modal>)renderHeaderSlot()so it lives in the sticky header areasuper.open()/super.close()overrides intoonOpen(args)/onClose(args)hooks@state activeTab, manualhandleTabChange, and therender()switch — all owned by BaseModal nowOther changes:
Store: in affiliate mode (#affiliate=X), tabs are hidden and a single combined grid of purchasable affiliate items is shownMain.ts:joinModal.open(lobbyId, lobbyInfo)callsites converted to the newopen({ lobbyId, lobbyInfo })shapeFollow-up
Modal URL router (
#modal=X&tab=Y&...) is a separate PR on top of this foundation.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan