Add window close confirmation dialog for alive players #2141
Conversation
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a window close confirmation dialog to prevent accidental loss of progress when players attempt to close the browser window while actively playing. The implementation checks if a player is alive before triggering the confirmation dialog.
- Adds a
shouldPreventUnloadfunction to check if window close should be prevented - Implements
shouldPreventWindowClosemethod inClientGameRunnerto check if player is alive - Modifies the
beforeunloadevent handler to conditionally prevent window closing
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/client/Main.ts | Updates beforeunload event handler to conditionally prevent window closing and imports the new shouldPreventUnload function |
| src/client/ClientGameRunner.ts | Adds module-level game runner tracking, shouldPreventUnload export function, and shouldPreventWindowClose method to check player status |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Warning Rate limit exceeded@ryanbarlow97 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR changes lobby join/leave to return a boolean stop callback and adds runtime gating to prevent window unload when the local player is alive. Changes
Sequence Diagram(s)sequenceDiagram
actor User as User
participant Browser as Browser
participant Client as Client (Main)
participant Lobby as joinLobby()
participant Runner as ClientGameRunner
Note over Client,Lobby: Join flow
User->>Client: call joinLobby(...)
Client->>Lobby: create/join
Lobby->>Runner: new ClientGameRunner()
Runner->>Runner: start()
Lobby->>Client: return stop(): boolean (captures currentGameRunner)
Note over Browser,Client: Before unload
Browser->>Client: beforeunload
alt stop exists
Client->>Client: result = stop()
alt result === false
Client-->>Browser: set e.preventDefault() & e.returnValue
Note right of Browser: Browser shows confirm dialog
else allow
Client-->>Browser: no prevention
end
else no stop
Client-->>Browser: no prevention
end
sequenceDiagram
participant Stop as stop()
participant Runner as ClientGameRunner
participant Lobby as joinLobby()
Lobby->>Runner: start and set currentGameRunner
Lobby->>Stop: return stop callback
Note over Stop,Runner: when stop() is invoked
Stop->>Runner: if Runner.shouldPreventWindowClose() === true
alt true
Stop-->>Caller: return false (prevent unload)
else
Stop->>Runner: teardown/clear currentGameRunner
Stop-->>Caller: return true (allow unload)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
CLA signed. This feature was implemented with frustration. I have lost enough games at this point. Mis-clicking when switching back from the other YouTube tab and loose all progress is painful. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Copilot cannot sign CLA. Need whitelisting in the CLA dashboard. cla-assistant/cla-assistant#1121 A Tool does not have ability to sign a contract. |
|
@copilot Please propose a change to avoid having a top-levle variable, if possible. "maybe have joinLobby return a function that determines if it should close or not." I will code-review and test it afterwards. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/client/ClientGameRunner.ts (2)
81-82: Reconsider the module-level variable approach.A past review comment from evanpelle requested avoiding a module-level variable. The suggested alternative was "have joinLobby return a function that determines if it should close or not."
While the current implementation works, consider refactoring to eliminate
currentGameRunnerby having the returned function close over the runner directly, or by having the function return an object with both aleavemethod and ashouldPreventClosequery method.Example refactor:
export function joinLobby( eventBus: EventBus, lobbyConfig: LobbyConfig, onPrestart: () => void, onJoin: () => void, ): () => boolean { console.log( `joining lobby: gameID: ${lobbyConfig.gameID}, clientID: ${lobbyConfig.clientID}`, ); const userSettings: UserSettings = new UserSettings(); startGame(lobbyConfig.gameID, lobbyConfig.gameStartInfo?.config ?? {}); const transport = new Transport(lobbyConfig, eventBus); - let currentGameRunner: ClientGameRunner | null = null; + let gameRunner: ClientGameRunner | null = null; const onconnect = () => { console.log(`Joined game lobby ${lobbyConfig.gameID}`); transport.joinGame(0); }; let terrainLoad: Promise<TerrainMapData> | null = null; const onmessage = (message: ServerMessage) => { if (message.type === "prestart") { console.log( `lobby: game prestarting: ${JSON.stringify(message, replacer)}`, ); terrainLoad = loadTerrainMap( message.gameMap, message.gameMapSize, terrainMapFileLoader, ); onPrestart(); } if (message.type === "start") { // Trigger prestart for singleplayer games onPrestart(); console.log( `lobby: game started: ${JSON.stringify(message, replacer, 2)}`, ); onJoin(); // For multiplayer games, GameStartInfo is not known until game starts. lobbyConfig.gameStartInfo = message.gameStartInfo; createClientGame( lobbyConfig, eventBus, transport, userSettings, terrainLoad, terrainMapFileLoader, ).then((r) => { - currentGameRunner = r; + gameRunner = r; r.start(); }); } if (message.type === "error") { showErrorModal( message.error, message.message, lobbyConfig.gameID, lobbyConfig.clientID, true, false, "error_modal.connection_error", ); } }; transport.connect(onconnect, onmessage); return () => { - if (currentGameRunner && currentGameRunner.shouldPreventWindowClose()) { + if (gameRunner && gameRunner.shouldPreventWindowClose()) { return false; } console.log("leaving game"); - currentGameRunner = null; + gameRunner = null; transport.leaveGame(); return true; }; }This keeps the variable scoped to the
joinLobbyclosure instead of module-level.
228-234: Update the comment to reflect actual behavior.The comment says "Show confirmation dialog" but this method only returns a boolean. The browser shows the dialog via the beforeunload event.
- // Show confirmation dialog if player is alive in the game + // Check if we should prevent window close (player is alive)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/ClientGameRunner.ts(5 hunks)src/client/Main.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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
🔇 Additional comments (4)
src/client/Main.ts (2)
83-83: LGTM!The type change from void-returning to boolean-returning aligns with the new unload prevention logic.
156-162: LGTM!The beforeunload handler correctly prevents the window close when
gameStop()returns false and allows it when true or null. The empty string return is needed for older browser compatibility.src/client/ClientGameRunner.ts (2)
71-71: LGTM!The return type change is consistent with the updated gameStop signature in Main.ts.
136-143: LGTM!The leave callback correctly returns false to prevent window close when the player is alive, and true otherwise. The cleanup sequence is appropriate.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/Main.ts (1)
156-162: Adde.returnValuefor better browser compatibility.The enriched summary states "sets e.returnValue to ''" but the code only calls
preventDefault()and returns"". WhilepreventDefault()is sufficient for modern browsers, settinge.returnValue = ""improves compatibility with older browsers.Apply this diff:
window.addEventListener("beforeunload", (e) => { if (this.gameStop && !this.gameStop()) { e.preventDefault(); + e.returnValue = ""; return ""; } console.log("Browser is closing"); });The overall logic correctly prevents window close when the player is alive—nicely done!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/ClientGameRunner.ts(5 hunks)src/client/Main.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/ClientGameRunner.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Foorack
PR: openfrontio/OpenFrontIO#2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.330Z
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: Foorack
PR: openfrontio/OpenFrontIO#2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.330Z
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()`.
📚 Learning: 2025-10-08T17:14:49.330Z
Learnt from: Foorack
PR: openfrontio/OpenFrontIO#2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.330Z
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-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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
🔇 Additional comments (1)
src/client/Main.ts (1)
83-83: LGTM! Type change enables window close confirmation.The boolean return value correctly signals whether the game can be stopped (true = allow close, false = prevent close).
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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/Main.ts (1)
1-1: Fix Prettier formatting.The pipeline reports a code style issue. Run
prettier --write src/client/Main.tsto fix formatting.
🧹 Nitpick comments (1)
src/client/ClientGameRunner.ts (1)
117-120: Consider the startup timing edge case.If the leave callback is invoked after the "start" message arrives but before
createClientGameresolves,currentGameRunnerremainsnullandshouldPreventWindowClose()won't be checked. This is a narrow window, but you could mitigate it by setting a temporary flag or placeholder when the "start" message arrives, then replacing it with the actual runner when the promise resolves.That said, this edge case is unlikely in practice and the current implementation aligns with your request to avoid a module-level variable, so deferring this refinement is reasonable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/ClientGameRunner.ts(5 hunks)src/client/Main.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Foorack
PR: openfrontio/OpenFrontIO#2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.330Z
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: Foorack
PR: openfrontio/OpenFrontIO#2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.330Z
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()`.
📚 Learning: 2025-10-08T17:14:49.330Z
Learnt from: Foorack
PR: openfrontio/OpenFrontIO#2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.330Z
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.330Z
Learnt from: Foorack
PR: openfrontio/OpenFrontIO#2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.330Z
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.tssrc/client/Main.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
PR: openfrontio/OpenFrontIO#977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/client/ClientGameRunner.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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
🪛 GitHub Actions: 🧪 CI
src/client/Main.ts
[warning] 1-1: Code style issues found in file. Run 'prettier --write' to fix.
🔇 Additional comments (4)
src/client/Main.ts (1)
83-83: LGTM! Type change aligns with new boolean contract.The type change from
() => voidto() => booleancorrectly reflects thatgameStopnow returns a boolean indicating whether window close should be allowed.src/client/ClientGameRunner.ts (3)
71-71: LGTM! Clean approach avoiding module-level state.The boolean return type and local
currentGameRunnervariable provide a clean solution that avoids a module-level variable, as requested in the PR comments.Also applies to: 81-82
136-143: LGTM! Leave callback logic is correct.The leave callback correctly checks
shouldPreventWindowClose()and returnsfalseto prevent unload when the player is alive, then clearscurrentGameRunnerand returnstrueto allow unload otherwise.
228-244: LGTM! Method implementation is correct.The JSDoc clearly documents the behavior, and the implementation correctly returns
truewhen the player is alive (based on learnings,myPlayeris always set when this is called, and the troop count requirement was intentionally removed).Based on learnings
|
(As a note, the Issue was assigned already and PR #1939 was made before this one) |
|
@VariableVince Yes, I didn't know of that Issue before I started. But that PR was also made before the "screwed around with main", so needs major rebasing. So depends on how much work that is. I also haven't looked in detail into the implementation approach of that PR. I don't care at all which PR gets merged. I would more than happily close this one. But 10 min ago I accidentally closed the tab AGAIN, and my team were doing so well, so this is a desperately wanted feature... 😢 Alternatively if it was possible to re-join if you open the tab back up again / reloaded the page. |
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
github-actions
bot
added
the
Stale
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/ClientGameRunner.ts (1)
227-243: Window close prevention check is well-implemented.The
shouldPreventWindowClose()method is correctly implemented with:
- Comprehensive JSDoc explaining its purpose and return values
- Proper null check on
myPlayer(which is sufficient per learnings)- Correct logic: returns
truewhen player is alive to prevent window closeThe inline comment on line 238 says "Show confirmation dialog" but the method only returns a boolean—the browser's beforeunload event actually shows the dialog. However, the JSDoc above is accurate and comprehensive, so this is just a minor clarity point.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/ClientGameRunner.ts(5 hunks)src/client/Main.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 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: 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.
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: 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.
📚 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.tssrc/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/Main.tssrc/client/ClientGameRunner.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.tssrc/client/ClientGameRunner.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/Main.tssrc/client/ClientGameRunner.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/client/Main.tssrc/client/ClientGameRunner.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/client/Main.tssrc/client/ClientGameRunner.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.tssrc/client/ClientGameRunner.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/client/ClientGameRunner.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/client/ClientGameRunner.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/client/ClientGameRunner.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/client/ClientGameRunner.ts
🔇 Additional comments (6)
src/client/Main.ts (2)
93-93: Type signature correctly updated to match new behavior.The change from
(() => void)to(() => boolean)properly reflects thatgameStopnow returns a boolean indicating whether the window close should be prevented. This aligns with the updatedjoinLobbyreturn type in ClientGameRunner.ts.
168-175: Window close prevention logic is implemented correctly.The beforeunload handler properly:
- Checks if
gameStopexists before calling it- Uses the boolean return value to determine whether to prevent unload
- Sets both
e.preventDefault()ande.returnValuefor browser compatibility- Includes explicit
return ""to stop code flow (as previously discussed)The logic correctly prevents window close when the player is alive (when
gameStop()returnsfalse).src/client/ClientGameRunner.ts (4)
70-70: Return type correctly updated to support window close prevention.The signature change from
() => voidto() => booleanenables the returned function to communicate whether window close should be prevented, which integrates properly with the beforeunload logic in Main.ts.
80-81: Good approach to avoid module-level state.Introducing
currentGameRunneras a local variable withinjoinLobbyavoids the need for a module-level variable, as requested in previous feedback. This keeps state encapsulated and makes the lifecycle clearer.
116-119: Runner initialization timing is correct.The game runner is stored in
currentGameRunnerwhen the game starts, which properly enables window close prevention during active gameplay. The timing (after promise resolution) is intentional, as the confirmation should only trigger when the player is actively playing, not during the loading phase.
134-142: Leave logic correctly implements window close prevention.The returned function properly:
- Checks if the player is alive via
shouldPreventWindowClose()- Returns
falseto prevent window close when player is alive- Returns
trueto allow window close otherwise- Cleans up
currentGameRunnerstate appropriatelyThe boolean logic correctly integrates with the beforeunload handler in Main.ts.
github-actions
bot
removed
the
Stale
|
@Foorack could sign the cla? Or maybe it is copilot blocking it |
|
@jrouillard Copilot is blocking the CLA. I can make a completely fresh PR. I read Copilot can also be whitelisted in the CLA control panel. Honestly, I though this PR had been closed in favor of #1939 There was an older ticket, with a person already assigned and had a PR and was responding, so I stepped back to not duplicate work. Let me know if I should start working on this actively again. Additionally, I think it would be better to implement rejoin-on-reopen if possible than this pop-up. That would solve even if a browser was forced closed or laptop quickly rebooted. |
|
@Foorack @jrouillard What is the status on this, in regards to either new PR / copilot whitelisted for CLA / better to implement rejoin-on-reopen? Btw a solution like the one in #1836 may be prefered in the future, but we could still use the current PR as a starting spot for the back/close button? |
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
github-actions
bot
added
the
Stale
github-actions
bot
removed
the
Stale
7 participants
Description:
Closes #1877
This PR adds a window close confirmation dialog for active players. When a user attempts to close the window while actively playing, a dialog will prompt for confirmation, helping prevent accidental loss of progress.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
Foorack