Handle confirmation on popstate event if player is active in a game #2777
Conversation
|
|
WalkthroughJoin/leave lifecycle changed: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Browser
participant Main as Main.ts
participant Runner as ClientGameRunner
participant Transport as Transport
User->>Browser: Back / close / navigate
Browser->>Main: popstate / beforeunload
Main->>Runner: shouldPreventWindowClose()
alt Runner prevents close
Main->>User: show confirmation (translateText)
alt User cancels
Main->>Browser: restore previous URL (preventHashUpdate = true)
else User confirms
Main->>Runner: leave(force=true)
Runner->>Transport: leaveGame()
Transport-->>Runner: ack
Runner-->>Main: return true
Main->>Browser: proceed with navigation / update hash
end
else No prevention
Main->>Runner: leave(force=true) or proceed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
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 (2)
src/client/ClientGameRunner.ts (1)
1-1: Fix code formatting.Prettier has detected formatting issues. Please run
npx prettier --write src/client/ClientGameRunner.tsto fix them.src/client/Main.ts (1)
1-1: Fix code formatting.Prettier has detected formatting issues. Please run
npx prettier --write src/client/Main.tsto fix them.
🧹 Nitpick comments (1)
src/client/ClientGameRunner.ts (1)
255-268: LGTM! Clear prevention logic.The method correctly determines whether window close should be prevented by checking if the player exists and is alive. The JSDoc clearly explains the behavior.
Based on learnings, the troop count requirement was intentionally removed because it was arbitrary.
Optional: Simplify boolean conversion
The
!!operator is unnecessary since&&already returns a boolean:public shouldPreventWindowClose(): boolean { // Show confirmation dialog if player is alive in the game - return !!(this.myPlayer && this.myPlayer.isAlive()); + return this.myPlayer !== null && this.myPlayer.isAlive(); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/ClientGameRunner.tssrc/client/Main.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/ClientGameRunner.tssrc/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/ClientGameRunner.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/ClientGameRunner.tssrc/client/Main.ts
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/Main.ts
🧬 Code graph analysis (1)
src/client/Main.ts (2)
src/client/LangSelector.ts (1)
translateText(222-242)src/client/Utils.ts (1)
translateText(92-147)
🪛 GitHub Actions: 🧪 CI
src/client/ClientGameRunner.ts
[warning] 1-1: Code style issues found in this file by Prettier. Run 'npx prettier --write' to fix.
src/client/Main.ts
[warning] 1-1: Code style issues found in this file by Prettier. Run 'npx prettier --write' to fix.
🔇 Additional comments (11)
src/client/ClientGameRunner.ts (4)
72-72: Return type correctly updated.The signature change from
() => voidto(force?: boolean) => booleanproperly reflects the new exit confirmation behavior. The returned boolean indicates whether the leave operation succeeded.
82-83: LGTM! Proper lifecycle management.The
currentGameRunnertracking variable is correctly initialized, stored when the game starts (line 127), checked during leave operations (line 155), and cleared before leaving (line 163).
126-129: LGTM! Correct initialization flow.The game runner is properly stored in
currentGameRunnerbefore starting. This ensures the instance is available for the leave protection check.
154-167: LGTM! Exit prevention logic is sound.The force parameter correctly bypasses the prevention check. When not forced, the function checks
shouldPreventWindowClose()and returnsfalseto signal prevention, without performing any leave operations. When leaving is allowed (forced or player not active), it properly clearscurrentGameRunnerbefore callingtransport.leaveGame().Based on learnings, the
shouldPreventWindowClose()check is sufficient without needing to fetch the player again.src/client/Main.ts (7)
46-46: LGTM! Required import for confirmation dialog.The
translateTextimport is correctly added and used on line 395 for the exit confirmation message.
106-110: LGTM! Type signature and state tracking properly aligned.The
gameStoptype correctly matches the updatedjoinLobbyreturn type. The new state fieldscurrentUrlandpreventHashUpdateare necessary for managing the confirmation and history restoration flow.
180-186: LGTM! Correct forced stop on browser close.Passing
force: truetogameStop()properly bypasses the confirmation dialog when the browser is closing, since the browser handles its own beforeunload confirmation.
374-378: LGTM! Proper guard for history restoration.The
preventHashUpdateguard correctly prevents the hash change handler from resetting the UI when history is restored after the user cancels the exit confirmation. The flag is properly reset after the early return.
638-641: LGTM! Proper URL tracking for confirmation flow.The
currentUrlis correctly stored after pushing the lobby URL to history. This enables the confirmation dialog to restore the correct URL if the user cancels navigation.
651-653: LGTM! Proper cleanup on leave.The forced stop with
gameStop(true)correctly bypasses confirmation when programmatically leaving. ThecurrentUrlis properly cleared to disable the confirmation flow after leaving.
394-396: Translation keyhelp_modal.exit_confirmationexists in all language files.The key is defined at line 135 in en.json (nested under
help_modalstarting at line 44) and is present in all 25+ language files inresources/lang/. The code usage is correct.
iiamlewis
added
the
Backend
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/client/ClientGameRunner.ts:
- Around line 126-129: The promise returned by createClientGame is unhandled on
rejection; wrap the call to createClientGame(...).then((r) => {
currentGameRunner = r; r.start(); }) with error handling (either add a
.catch(...) to the chain or convert to async/await and use try/catch) so that
failures (missing gameStartInfo, terrain load errors, etc.) are logged and
surfaced to the user (e.g., call your error modal or console.error) and ensure
currentGameRunner is only set after a successful creation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/ClientGameRunner.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/ClientGameRunner.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/ClientGameRunner.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/ClientGameRunner.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/ClientGameRunner.ts
🔇 Additional comments (4)
src/client/ClientGameRunner.ts (4)
67-72: Good API design for the leave function signature.The return type
(force?: boolean) => booleanis clean and useful. Caller gets feedback on whether leaving succeeded, and can bypass the guard when needed.
82-83: Local variable scope addresses previous feedback.The
currentGameRunneris now a closure-local variable insidejoinLobby, not a module-level variable. This keeps the state properly encapsulated.
154-167: Leave guard logic is correct.The guard properly uses optional chaining (
currentGameRunner?.shouldPreventWindowClose()) so null runner cases pass through. Theforceparameter lets callers bypass the check when needed.One edge case to be aware of: if a user navigates away while
createClientGameis pending (before.then()resolves),currentGameRunneris still null, so leaving proceeds without confirmation. This seems acceptable since the game hasn't fully started yet.
255-268: Implementation matches learnings and is well documented.The method correctly returns whether the player is alive without additional troop count checks. Based on retrieved learnings, this design was intentional since troop count can be misleading. The JSDoc is helpful for future maintainers.
github-project-automation
bot
moved this from Final Review
to Development
in OpenFront Release Management
|
Please review |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/Main.ts (1)
538-546: Consider forcing game stop when joining a new lobby.When joining a new lobby while already in a game (line 541-543), the code calls
this.gameStop()without theforceparameter. This might not actually leave the old game if the player is active.Since the player is explicitly joining a new lobby, should this be
this.gameStop(true)to force leave the old game?♻️ Proposed fix
if (this.gameStop !== null) { console.log("joining lobby, stopping existing game"); - this.gameStop(); + this.gameStop(true); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/Main.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/Main.ts
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-08-09T05:14:19.147Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1761
File: src/client/LocalPersistantStats.ts:24-31
Timestamp: 2025-08-09T05:14:19.147Z
Learning: Files in the src/client directory contain browser-only code in the OpenFrontIO project, so browser APIs like localStorage are guaranteed to be available and don't need undefined guards.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/Main.ts
🧬 Code graph analysis (1)
src/client/Main.ts (2)
src/client/LangSelector.ts (1)
translateText(224-244)src/client/Utils.ts (1)
translateText(92-147)
🔇 Additional comments (9)
src/client/Main.ts (9)
46-46: LGTM: translateText import added for confirmation dialog.The import is correctly added and used in the popstate handler for localizing the exit confirmation message.
109-110: LGTM: State fields for navigation guard.The
currentUrlandpreventHashUpdatefields are correctly initialized and support the history restoration mechanism when users cancel navigation.
180-186: LGTM: Force stop on beforeunload.Correctly passes
trueto force game stop when the browser window is closing, bypassing the confirmation check.
373-388: LGTM: preventHashUpdate guard implemented correctly.The guard prevents
onHashUpdatefrom processing hashchange events triggered by the history restoration inonPopState. The flag is correctly reset after being checked.
415-417: LGTM: Event listeners correctly wired.Both
popstate(browser navigation) andhashchange(manual hash edits) are handled. ThepreventHashUpdateflag coordinates between them to avoid double-processing.
638-645: LGTM: currentUrl stored after joining lobby.The current URL is correctly stored after pushing the join hash to history. This enables the popstate handler to restore the URL if the user cancels navigation away from the active game.
649-662: LGTM: Forced stop and state cleanup when leaving lobby.The code correctly:
- Forces game stop with
gameStop(true)since this is an explicit leave action- Clears
currentUrlsince navigation confirmation is no longer needed
106-106: gameStop signature change is correctly implemented across the codebase.The signature change from
(() => void) | nullto((force?: boolean) => boolean) | nullis properly applied. All four call sites in Main.ts correctly use the new signature:this.gameStop(true)at lines 183 and 654,!this.gameStop()at line 391 (which checks the return value), andthis.gameStop()at line 543 (which safely ignores the return value since the parameter is optional). ThejoinLobbyfunction in ClientGameRunner.ts declares the matching return type, and the property is only used within Main.ts, so there are no compatibility issues elsewhere.
390-413: No changes needed—logic and translation are correct.The confirmation flow is well-implemented:
- Checks if player is active using
gameStop()return value- Shows confirmation dialog when leaving should be prevented
- Restores history with
preventHashUpdateflag if user cancels- Proceeds with normal hash update if user confirms or player is inactive
The translation key
help_modal.exit_confirmationexists in all locale files with correct nesting.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/client/Main.ts:
- Around line 374-378: preventHashUpdate can remain true after pushState (which
doesn't fire hashchange), causing onHashUpdate to be skipped later; in
onPopState, before calling this.onHashUpdate(...) clear this.preventHashUpdate =
false and add a short comment stating pushState does not trigger hashchange so
we must manually clear the guard; alternatively, if the guard is redundant after
this change, remove the preventHashUpdate logic (the early return block that
checks preventHashUpdate) along with a brief comment explaining the reason.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/Main.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:15.132Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/Main.ts
📚 Learning: 2026-01-02T18:11:15.132Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:15.132Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/Main.ts
🧬 Code graph analysis (1)
src/client/Main.ts (1)
src/client/Utils.ts (1)
translateText(92-147)
🔇 Additional comments (4)
src/client/Main.ts (4)
46-46: LGTM - translateText import added correctly.The import is used appropriately at line 395 for the exit confirmation dialog.
180-186: LGTM - Consistent forced stop handling and URL tracking.The implementation correctly:
- Forces game stop when the page is closing (line 183)
- Forces stop when joining a new lobby while in a game (line 543)
- Forces stop when explicitly leaving a lobby (line 654)
- Tracks
currentUrlonly while actively in a game (set at line 644, cleared at line 656)This ensures the confirmation is only shown for browser back button navigation, not for intentional game exits.
Also applies to: 543-543, 642-644, 654-656
394-396: Verify translation key exists in en.json.The code uses the key
help_modal.exit_confirmationin the confirmation dialog for the browser back button. The PR description mentions updating en.json, but confirm that this translation key exists in all required locale files (especially en.json) with an appropriate message text.
106-106: Verify the return value semantics of gameStop.The signature changed from
(() => void) | nullto((force?: boolean) => boolean) | null. The new boolean return value is used at line 391 to check if confirmation is needed. Ensure that when called without arguments (orforce=false), gameStop returns:
falseif the player is active and should be prevented from leavingtrueif stopping succeeded without needing confirmationThe double negation
!this.gameStop()at line 391 suggests this semantic, but it would be clearer with better naming or inline documentation explaining the return value.
|
@evanpelle I saw you resolved the review conversations. Work on this PR is completed, the only thing left is your review (hoping to get it in v29!) |
4 participants
Description:
Protect against players accidentally leaving an active game by pressing the browser back button. Uses the same confirmation dialog as the game exit button.
Partially handles issue #1877 (protects against back button, not closing tab or editing the URL directly).
Partial credit to PR #2141
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
deshack_82603