Add modal URL router (#modal=name&tab=key)#3924
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (13)
WalkthroughBaseModal now optionally syncs modal state with modalRouter: it imports modalRouter, exposes ChangesBaseModal router integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (1)
src/client/ModalRouter.ts (1)
126-138: ⚡ Quick winConsider clarifying or enforcing string-only args.
Line 132 checks
typeof value === "string"before writing to the hash, silently dropping non-string values fromargs: Record<string, unknown>. If a modal callsmodalRouter.syncOpened(name, {tab: 0}), thetabparam won't appear in the URL.URL query params are inherently strings, so this filter makes sense, but it's implicit. Consider:
- Typing args as
Record<string, string>in the public methods to make the contract explicit, or- Adding a runtime warning when non-string values are encountered and dropped
🤖 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/ModalRouter.ts` around lines 126 - 138, The writeHash function currently accepts args: Record<string, unknown> and silently drops non-string values (in writeHash), which hides misuse; update the public modal-router API so args are typed as Record<string, string> (e.g., change signatures that call writeHash/syncOpened to accept Record<string,string>) to make the contract explicit and also add a small runtime check inside writeHash that logs a warning (e.g., console.warn or processLogger.warn) when a non-string value is encountered and skipped so callers get visible feedback; reference writeHash and the public methods like syncOpened/replaceHash when making these changes.
🤖 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/ModalRouter.ts`:
- Around line 66-79: ModalRouter currently calls window.showPage(entry.pageId)
which already triggers the inline modal's open, then calls el?.open(args) again
causing onOpen to run twice; update ModalRouter (around the routingFromUrl
handling in ModalRouter) to avoid the duplicate open by either passing args
through to showPage (if showPage can accept them), or by changing the flow to
use the element returned/located by showPage and skip the second el?.open(args)
call; reference routines: ModalRouter, window.showPage, RoutableModal, open,
BaseModal.onOpen — implement the chosen option and ensure routingFromUrl logic
still sets/clears the flag as before.
---
Nitpick comments:
In `@src/client/ModalRouter.ts`:
- Around line 126-138: The writeHash function currently accepts args:
Record<string, unknown> and silently drops non-string values (in writeHash),
which hides misuse; update the public modal-router API so args are typed as
Record<string, string> (e.g., change signatures that call writeHash/syncOpened
to accept Record<string,string>) to make the contract explicit and also add a
small runtime check inside writeHash that logs a warning (e.g., console.warn or
processLogger.warn) when a non-string value is encountered and skipped so
callers get visible feedback; reference writeHash and the public methods like
syncOpened/replaceHash when making these changes.
🪄 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: f0b03862-3ead-4634-a53b-0ab6dfcd254d
📒 Files selected for processing (17)
src/client/AccountModal.tssrc/client/ClanModal.tssrc/client/Cosmetics.tssrc/client/FlagInputModal.tssrc/client/HelpModal.tssrc/client/LanguageModal.tssrc/client/LeaderboardModal.tssrc/client/Main.tssrc/client/ModalRouter.tssrc/client/NewsModal.tssrc/client/SinglePlayerModal.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/TroubleshootingModal.tssrc/client/UserSettingModal.tssrc/client/components/BaseModal.tssrc/client/components/RankedModal.ts
Comment on lines
+66
to
+79
| this.routingFromUrl = true; | ||
| try { | ||
| this.currentName = name; | ||
| if (entry.pageId) { | ||
| // Inline modal: showPage reveals the page-content container and calls | ||
| // .open() on the inline modal element automatically. We then call | ||
| // .open(args) so the args reach onOpen. | ||
| window.showPage?.(entry.pageId); | ||
| } | ||
| const el = document.querySelector(entry.tag) as RoutableModal | null; | ||
| el?.open(args); | ||
| } finally { | ||
| this.routingFromUrl = false; | ||
| } |
Contributor
There was a problem hiding this comment.
Potential double-open fragility for inline modals.
For inline modals with pageId, Line 73 calls window.showPage(pageId), which (per BaseModal comments) "calls .open() on the inline modal element automatically." Then Line 76 explicitly calls el?.open(args) again.
BaseModal handles this by checking wasOpen and returning early at Line 174, but onOpen(args?) still runs twice—once with no args, once with args. This relies on BaseModal's specific wasOpen logic and assumes onOpen tolerates being called twice.
Consider a more explicit contract:
- Either pass args directly to
showPageif it supports them, or - Have
showPagereturn a reference and skip the secondopen()call, or - Document this double-call requirement clearly in
RoutableModalinterface
This pattern is fragile if showPage or BaseModal internals change.
🤖 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/ModalRouter.ts` around lines 66 - 79, ModalRouter currently calls
window.showPage(entry.pageId) which already triggers the inline modal's open,
then calls el?.open(args) again causing onOpen to run twice; update ModalRouter
(around the routingFromUrl handling in ModalRouter) to avoid the duplicate open
by either passing args through to showPage (if showPage can accept them), or by
changing the flow to use the element returned/located by showPage and skip the
second el?.open(args) call; reference routines: ModalRouter, window.showPage,
RoutableModal, open, BaseModal.onOpen — implement the chosen option and ensure
routingFromUrl logic still sets/clears the flag as before.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
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
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 toonOpen. 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-managedrouteFromHash()parses#modal=...and dispatches tomodal.open(args)syncOpened/syncClosed/syncTabpush state back into the URL viahistory.replaceState(no history entries)routingFromUrlflag prevents URL→modal→URL feedback loopsBaseModal— opt-in URL sync via arouterNameproperty.modalRouter.syncOpened/syncClosed/syncTabfromopen/close/setActiveTabrouterNameundefinedMain.ts— registers all routable modals and wires the router.handleUrl(): adds amodalRouter.routeFromHash()branch after the path-based lobby joinonHashUpdate: when the hash is router-managed, routes via the router instead of tearing down lobby stateRoutable 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
/#modal=store&tab=flags/#modal=settings&tab=keybinds#modal=store&tab=packswhen a hard-currency purchase fails for insufficient plutonium (after the alert), so users can top up directlyURL behavior
replaceStateeverywhere — no history entries added when modals open / close / switch tabshashchangeevents are router-aware so external hash changes (back button, manual edit) correctly switch between routed modals without tearing down lobby statePlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME