Feat: Singleplayer Achievements #2734
Conversation
WalkthroughAdds player map achievement support: API schema for single-player map achievements, frontend state and event handling in SinglePlayerModal to load and toggle achievements, MapDisplay props and rendering for medal icons, new CSS color variables, translation key, and icon attribution. (47 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Modal as SinglePlayerModal
participant API as Backend API
participant Maps as MapDisplay
User->>Modal: Open single-player modal
Modal->>API: Request UserMe data
API-->>Modal: Respond UserMeResponse (may include achievements)
Modal->>Modal: applyAchievements() → build mapWins (map -> Set<difficulty>)
Modal->>Maps: Update props (.showMedals, .wins) per map
User->>Modal: Click achievements toggle
Modal->>Modal: toggleAchievements() (flip showAchievements)
Modal->>Maps: Prop update → Maps re-render
Maps->>Maps: renderMedals() using wins → mark earned/uneared icons
Maps-->>User: Display medal row with states
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 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 |
![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
🧹 Nitpick comments (3)
src/client/SinglePlayerModal.ts (1)
85-103: Consider extracting inline styles to CSS classes.The toggle button has extensive inline styles that would be cleaner as CSS classes. Additionally, the button should have an
aria-labelattribute for screen readers.🔎 Proposed refactor
Create CSS classes in the component's styles:
.achievement-toggle-container { position: relative; display: flex; align-items: center; justify-content: center; width: 100%; } .achievement-toggle-title { text-align: center; width: 100%; } .achievement-toggle-button { display: flex; align-items: center; justify-content: center; width: 28px; height: 28px; border: 1px solid rgba(255, 255, 255, 0.2); border-radius: 6px; background: rgba(255, 255, 255, 0.06); cursor: pointer; padding: 4px; position: absolute; right: 0; top: 50%; transform: translateY(-50%); } .achievement-toggle-icon { width: 18px; height: 18px; transition: opacity 0.2s; } .achievement-toggle-icon.active { opacity: 1; } .achievement-toggle-icon.inactive { opacity: 0.5; }Then simplify the template:
- <div - class="option-title" - style="position:relative; display:flex; align-items:center; justify-content:center; width:100%;" - > - <span style="text-align:center; width:100%;"> + <div class="option-title achievement-toggle-container"> + <span class="achievement-toggle-title"> ${translateText("map.map")} </span> <button @click=${this.toggleAchievements} title=${translateText("single_modal.toggle_achievements")} - style="display:flex; align-items:center; justify-content:center; width:28px; height:28px; border:1px solid rgba(255,255,255,0.2); border-radius:6px; background:rgba(255,255,255,0.06); cursor:pointer; padding:4px; position:absolute; right:0; top:50%; transform:translateY(-50%);" + class="achievement-toggle-button" + aria-label=${translateText("single_modal.toggle_achievements")} > <img src="/images/MedalIconWhite.svg" alt="Toggle achievements" - style=${`width:18px; height:18px; opacity:${this.showAchievements ? "1" : "0.5"};`} + class="achievement-toggle-icon ${this.showAchievements ? "active" : "inactive"}" /> </button> </div>src/client/components/Maps.ts (1)
198-220: Consider extracting the localStorage key and refining the type guard.Two minor improvements:
- The localStorage key
"achievements.singleplayerWins"should be a constant to avoid typos and enable reuse- Line 212 uses
as anyin the type predicate, which could be more precise🔎 Proposed improvements
At the top of the file, add a constant:
const ACHIEVEMENTS_STORAGE_KEY = "achievements.singleplayerWins";Then update line 203:
- const storageKey = "achievements.singleplayerWins"; + const storageKey = ACHIEVEMENTS_STORAGE_KEY;For the type guard at line 212, refine it slightly:
return new Set( values.filter( - (v): v is Difficulty | "Custom" => - v === "Custom" || Object.values(Difficulty).includes(v as any), + (v): v is Difficulty | "Custom" => + v === "Custom" || (typeof v === "number" && Object.values(Difficulty).includes(v)), ), );src/client/ClientGameRunner.ts (1)
322-337: Consider extracting singleplayer defaults to a shared constant for clarity.The hardcoded values in
isDefaultSingleplayerSettingscurrently match SinglePlayerModal's actual defaults, but centralizing them would improve maintainability and reduce the risk of future drift during refactoring.const DEFAULT_SINGLEPLAYER_CONFIG = { gameMapSize: GameMapSize.Normal, donateGold: true, donateTroops: true, disableNations: false, bots: 400, infiniteGold: false, infiniteTroops: false, instantBuild: false, randomSpawn: false, maxTimerValue: undefined, disabledUnits: [] as const, } as const;Then use it in the validation method instead of individual checks.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
resources/images/MedalIconWhite.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
CREDITS.mdresources/lang/en.jsonsrc/client/ClientGameRunner.tssrc/client/SinglePlayerModal.tssrc/client/components/Maps.tssrc/client/styles/core/variables.css
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-09-02T04:13:07.073Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 1958
File: resources/maps/northamerica/manifest.json:230-235
Timestamp: 2025-09-02T04:13:07.073Z
Learning: In OpenFrontIO, flag additions do not require updating an ATTRIBUTIONS.md file, as demonstrated by PR #1577 which added Circassia flag without any attribution file updates. The repository does not maintain an ATTRIBUTIONS.md file.
Applied to files:
CREDITS.md
📚 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/SinglePlayerModal.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/SinglePlayerModal.tssrc/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/SinglePlayerModal.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/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
🧬 Code graph analysis (2)
src/client/SinglePlayerModal.ts (2)
src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (1)
translateText(92-147)
src/client/ClientGameRunner.ts (4)
src/core/game/GameView.ts (3)
update(80-84)update(631-692)config(783-785)src/core/game/GameUpdates.ts (1)
WinUpdate(250-254)src/core/game/GameImpl.ts (1)
config(338-340)src/core/Schemas.ts (1)
GameStartInfo(128-128)
🪛 markdownlint-cli2 (0.18.1)
CREDITS.md
53-53: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (14)
CREDITS.md (1)
53-53: LGTM!The Medal icon attribution follows the existing format in this file. The static analysis hint about the bare URL can be safely ignored—this style is consistent throughout the CREDITS.md file.
src/client/styles/core/variables.css (1)
26-32: LGTM!The medal color variables follow standard medal conventions (bronze, silver, gold) and provide clear visual distinctions for each difficulty level.
resources/lang/en.json (1)
140-140: LGTM!The translation entry is properly formatted and placed in the correct section.
src/client/SinglePlayerModal.ts (3)
52-52: LGTM!The state initialization is clean and follows Lit conventions.
75-77: LGTM!Simple and effective toggle implementation.
129-129: LGTM!The showMedals property binding correctly reflects the toggle state.
src/client/components/Maps.ts (5)
54-54: LGTM!Property declaration follows Lit conventions.
64-64: LGTM!The spacing adjustments accommodate the new medal row nicely.
Also applies to: 74-74, 92-92
108-126: LGTM!The medal styling is clean and uses CSS masking effectively. The vendor prefix for
-webkit-maskensures good browser compatibility.
164-166: LGTM!Clean conditional rendering tied to the showMedals prop.
172-196: LGTM!The medal rendering logic is straightforward and correctly maps difficulties to colors using the CSS variables.
src/client/ClientGameRunner.ts (3)
15-22: LGTM!The expanded imports are necessary for the new achievement logic.
251-251: LGTM!Good placement—recording the win before creating the game record ensures achievements are saved even if subsequent operations fail.
278-291: LGTM!The win detection logic correctly handles both individual player wins and team wins.
|
This looks and sounds awesome |
|
Looks great! Love the idea |
|
I really like this!
Thats important, people will be furious if all their progress is gone after a browser storage cleanup 😅 We also need to display a warning if they are not logged in. That their win will not be counted? Or something like that? And we need to think about what happens when an new version drops and the nation intelligence has changed. Reset progress of everybody? Don't reset anything? Give people a button to reset progress themselves? |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/lang/en.jsonsrc/client/components/Maps.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
🔇 Additional comments (4)
src/client/components/Maps.ts (4)
3-3: LGTM! Import is clean and necessary.The
Difficultyimport is used correctly in the medal rendering logic.
55-55: LGTM! Property definition follows Lit conventions.The boolean property correctly controls medal visibility with a sensible default.
65-65: LGTM! CSS styling is clean and correct.The spacing adjustments and medal styles work well. The mask technique correctly shapes the background color through the SVG icon.
Also applies to: 75-75, 93-93, 109-127
165-167: LGTM! Conditional rendering is implemented correctly.The template logic cleanly toggles the medal display based on the
showMedalsproperty.
…penfrontio/OpenFrontIO into bijx/singleplayer-achievements
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/ClientGameRunner.ts (1)
1-1: Run Prettier to fix formatting issues.The CI pipeline reports code style issues. Run
npx prettier --write src/client/ClientGameRunner.tsto resolve.
🧹 Nitpick comments (1)
src/client/ClientGameRunner.ts (1)
342-357: Consider extracting default config values to a shared constant.The hardcoded values here (e.g.,
bots !== 400) could drift from actual game defaults. If default bot count or other settings change elsewhere, this check becomes silently incorrect.A shared
DEFAULT_SINGLEPLAYER_CONFIGobject would make maintenance easier and keep values in sync.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/ClientGameRunner.ts
🧰 Additional context used
🧠 Learnings (8)
📚 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-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/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-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
📚 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/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/ClientGameRunner.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/client/ClientGameRunner.ts
🧬 Code graph analysis (1)
src/client/ClientGameRunner.ts (5)
src/core/game/GameUpdates.ts (1)
WinUpdate(250-254)src/client/Auth.ts (1)
getAuthHeader(35-40)src/client/Api.ts (1)
getApiBase(116-128)src/core/game/GameImpl.ts (1)
config(338-340)src/core/Schemas.ts (1)
GameStartInfo(128-128)
🪛 GitHub Actions: 🧪 CI
src/client/ClientGameRunner.ts
[warning] 1-1: Code style issues found. Run 'npx prettier --write' to fix.
🪛 GitHub Check: 🔍 ESLint
src/client/ClientGameRunner.ts
[failure] 18-18:
'GameMapType' is defined but never used.
[failure] 16-16:
'Difficulty' is defined but never used.
🔇 Additional comments (3)
src/client/ClientGameRunner.ts (3)
252-252: LGTM!The
voidkeyword correctly signals fire-and-forget intent. The async API call runs independently without blocking game save flow, and errors are handled insiderecordLocalWin.
279-292: LGTM!Clean discriminated union handling. The method correctly checks the winner type discriminant and compares against the appropriate player identifier (clientID for player wins, team for team wins).
36-37: LGTM!Imports are properly used:
getApiBasefor the API endpoint andgetAuthHeaderfor authorization in the newrecordLocalWinmethod.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/ClientGameRunner.ts (1)
1-1: Run Prettier to fix code formatting.The CI pipeline detected formatting issues. Run
npx prettier --write src/client/ClientGameRunner.tsto automatically fix them.
🧹 Nitpick comments (1)
src/client/ClientGameRunner.ts (1)
291-306: Consider extracting default singleplayer settings to a shared constant.The method hardcodes 11 default values (map size, bot count, flags, etc.). If default singleplayer settings change elsewhere in the codebase, this check could become stale and produce incorrect results.
To reduce maintenance burden and keep the defaults in sync, consider creating a shared constant that defines the canonical default singleplayer configuration, then compare against that.
💡 Example approach
Create a constant in a shared location (e.g.,
src/core/configuration/DefaultConfigs.ts):export const DEFAULT_SINGLEPLAYER_CONFIG = { gameMapSize: GameMapSize.Normal, donateGold: true, donateTroops: true, disableNations: false, bots: 400, infiniteGold: false, infiniteTroops: false, instantBuild: false, randomSpawn: false, maxTimerValue: undefined, disabledUnits: [], } as const;Then simplify the check to compare relevant fields against the constant.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/ClientGameRunner.ts
🧰 Additional context used
🧠 Learnings (9)
📚 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-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/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-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
📚 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-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-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/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/ClientGameRunner.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/client/ClientGameRunner.ts
🧬 Code graph analysis (1)
src/client/ClientGameRunner.ts (2)
src/core/game/GameUpdates.ts (1)
WinUpdate(250-254)src/core/Schemas.ts (1)
GameStartInfo(128-128)
🪛 GitHub Actions: 🧪 CI
src/client/ClientGameRunner.ts
[warning] 1-1: Code style issues found in the file. Run 'npx prettier --write' to fix.
🪛 GitHub Check: 🔍 ESLint
src/client/ClientGameRunner.ts
[failure] 17-17:
'GameType' is defined but never used.
[failure] 35-35:
'userAuth' is defined but never used.
[failure] 34-34:
'getUserMe' is defined but never used.
[failure] 34-34:
'getApiBase' is defined but never used.
🔇 Additional comments (1)
src/client/ClientGameRunner.ts (1)
276-289: Logic is correct and clear.The
didPlayerWinmethod properly handles both player and team victory conditions. The guard clauses and winner-type checks are sound.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/ApiSchemas.ts (1)
59-63: Consider makingplayerMapCompletionsoptional withinachievements.The current schema requires
playerMapCompletionsto be present whenachievementsexists. Making it optional allows the server to returnachievements: {}for users with no completions yet, rather than requiring an empty array.achievements: z .object({ - playerMapCompletions: PlayerMapCompletionSchema.array(), + playerMapCompletions: PlayerMapCompletionSchema.array().optional(), }) .optional(),The client code at lines 102-106 in
SinglePlayerModal.tsalready handles this case safely with?? [].src/client/SinglePlayerModal.ts (1)
135-153: Consider extracting inline styles to CSS classes.The achievement toggle button uses extensive inline styles. Extracting these to CSS classes would improve maintainability and reduce duplication if similar buttons are added elsewhere.
For example, create a
.achievement-toggle-buttonclass in your stylesheet with the positioning and styling rules, then apply it here. The opacity change can remain inline since it's state-driven.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/lang/en.jsonsrc/client/SinglePlayerModal.tssrc/client/components/Maps.tssrc/client/styles/core/variables.csssrc/core/ApiSchemas.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/lang/en.json
- src/client/components/Maps.ts
🧰 Additional context used
🧠 Learnings (6)
📚 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/core/ApiSchemas.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as `z.infer<typeof PlayerStatsSchema>` where PlayerStatsSchema has `.optional()` applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Applied to files:
src/core/ApiSchemas.ts
📚 Learning: 2025-05-21T04:10:59.706Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:44-53
Timestamp: 2025-05-21T04:10:59.706Z
Learning: The PlayerStats type from ArchiveSchemas already includes undefined in its definition, making explicit union types with undefined (PlayerStats | undefined) redundant.
Applied to files:
src/core/ApiSchemas.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/core/ApiSchemas.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/SinglePlayerModal.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/SinglePlayerModal.ts
🧬 Code graph analysis (1)
src/client/SinglePlayerModal.ts (2)
src/core/ApiSchemas.ts (1)
UserMeResponse(66-66)src/client/Utils.ts (1)
translateText(92-147)
🔇 Additional comments (4)
src/client/styles/core/variables.css (1)
26-32: LGTM! Clear naming and distinct colors.The medal color variables are well-named and provide good visual distinction across difficulty levels.
src/client/SinglePlayerModal.ts (3)
64-74: Good practice: properly cleaning up event listeners.The event listener lifecycle management correctly adds the listener on connect and removes it on disconnect, preventing memory leaks.
96-127: Solid defensive validation.The runtime validation prevents invalid data from breaking the UI, which is good defensive programming. The
Array.isArraycheck on Line 103 is technically redundant since the schema definesplayerMapCompletionsas an array, but it's harmless.If the schema used
z.enum()as suggested inApiSchemas.ts, lines 111-118 could be simplified since invalid values would be caught at parse time.
90-94: Past review concern resolved.A previous review flagged a missing
GET /api/player_map_completionsendpoint. The implementation has since changed to use theuserMeResponseevent (which receives data from the existinggetUserMeendpoint), eliminating the need for a separate API call. This is a cleaner architecture.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/lang/en.jsonsrc/client/SinglePlayerModal.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
🧰 Additional context used
🧠 Learnings (2)
📚 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/SinglePlayerModal.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/SinglePlayerModal.ts
🧬 Code graph analysis (1)
src/client/SinglePlayerModal.ts (1)
src/core/ApiSchemas.ts (1)
UserMeResponse(66-66)
🔇 Additional comments (4)
src/client/SinglePlayerModal.ts (4)
4-4: LGTM! Clean typed state setup.The import and state declarations are well-structured. The
mapWinstype correctly models the achievement data as a map of sets, making lookups efficient.Also applies to: 53-55
64-67: LGTM! Proper event listener lifecycle.The setup and teardown are symmetrical and correctly placed in the Lit component lifecycle hooks. The
as EventListenercast is necessary due to TypeScript's CustomEvent typing.Also applies to: 71-74
86-127: LGTM! Robust validation logic.The
applyAchievementsmethod has excellent defensive checks:
- Early exits for invalid states
- Type guards for string types
- Enum membership validation for both
mapNameanddifficulty- Graceful handling of malformed entries
This prevents invalid API data from corrupting the UI state.
179-180: LGTM! Correct props binding with nullish coalescing.The props are bound correctly:
.showMedalsreflects the toggle state.winsuses nullish coalescing to provide an emptySetwhen no achievements exist for a map, preventing null/undefined issues in the child component
| <div | ||
| class="option-title" | ||
| style="position:relative; display:flex; align-items:center; justify-content:center; width:100%;" | ||
| > | ||
| <span style="text-align:center; width:100%;"> | ||
| ${translateText("map.map")} | ||
| </span> | ||
| <button | ||
| @click=${this.toggleAchievements} | ||
| title=${translateText("single_modal.toggle_achievements")} | ||
| style="display:flex; align-items:center; justify-content:center; width:28px; height:28px; border:1px solid rgba(255,255,255,0.2); border-radius:6px; background:rgba(255,255,255,0.06); cursor:pointer; padding:4px; position:absolute; right:0; top:50%; transform:translateY(-50%);" | ||
| > | ||
| <img | ||
| src="/images/MedalIconWhite.svg" | ||
| alt="Toggle achievements" | ||
| style=${`width:18px; height:18px; opacity:${this.showAchievements ? "1" : "0.5"};`} | ||
| /> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if MedalIconWhite.svg exists
echo "=== Searching for MedalIconWhite.svg ==="
fd -t f "MedalIconWhite.svg"
echo -e "\n=== Checking public/images directory ==="
ls -la public/images/ 2>/dev/null || echo "public/images not found"
echo -e "\n=== Checking assets/images directory ==="
ls -la assets/images/ 2>/dev/null || echo "assets/images not found"
echo -e "\n=== Listing all .svg files in repo ==="
fd -t f "\.svg$" | head -20Repository: openfrontio/OpenFrontIO
Length of output: 938
🏁 Script executed:
#!/bin/bash
# Examine SinglePlayerModal.ts for styling approach
echo "=== Checking SinglePlayerModal.ts structure ==="
wc -l src/client/SinglePlayerModal.ts
echo -e "\n=== Looking for createRenderRoot ==="
rg "createRenderRoot" src/client/SinglePlayerModal.ts -A 3
echo -e "\n=== Looking for static styles in component ==="
rg "static styles|css\`" src/client/SinglePlayerModal.ts -B 2 -A 5Repository: openfrontio/OpenFrontIO
Length of output: 294
🏁 Script executed:
#!/bin/bash
# Check for other components to understand styling patterns
echo "=== Looking at Lit component styling patterns ==="
fd -t f "\.ts$" src/client/ | head -10 | xargs -I {} sh -c 'echo "=== {} ===" && grep -l "static styles" {}'
echo -e "\n=== Check for inline style usage in similar files ==="
rg "style=" src/client/SinglePlayerModal.ts | head -5Repository: openfrontio/OpenFrontIO
Length of output: 1196
Fix the asset path for MedalIconWhite.svg.
The image path /images/MedalIconWhite.svg is incorrect. The file is located at resources/images/MedalIconWhite.svg. Update line 151 to use the correct path, otherwise the medal icon will fail to load.
Consider extracting inline styles for maintainability.
The toggle button works correctly, but the inline styles on lines 137, 145, and 150 make the template harder to read. However, since this component uses light DOM (createRenderRoot() returns this), `static styles = css`` will not work. You can instead:
- Extract inline styles to a helper method that returns a style string
- Add a
<style>tag in the template - Keep inline styles if they are consistent with your codebase pattern
Extracting styles improves readability and makes changes easier to maintain.
🤖 Prompt for AI Agents
In src/client/SinglePlayerModal.ts around lines 135 to 153, the image src uses
the wrong absolute path (/images/MedalIconWhite.svg) which prevents the medal
icon from loading; update the src on line 151 to the correct relative path
resources/images/MedalIconWhite.svg. While making the change, optionally move
the long inline style strings into a small helper that returns a style string or
include a <style> block in the template to improve readability (since static
styles can't be used with light DOM), but the immediate required fix is
replacing the src path to resources/images/MedalIconWhite.svg.
github-project-automation
bot
moved this from Final Review
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/client/SinglePlayerModal.ts (2)
89-128: Implement the missing backend endpoint for achievements.The client expects achievement data via the
userMeResponseevent, but the backend endpoint to fetch and return player achievements may not exist yet. Ensure the backend serves achievement data in the expected format: an array of objects withmapName(GameMapType) anddifficulty(Difficulty | "Custom").
136-154: Fix the asset path for MedalIconWhite.svg.The image path
/images/MedalIconWhite.svgon line 149 is incorrect. Based on the project structure, it should beresources/images/MedalIconWhite.svg.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/client/SinglePlayerModal.tssrc/client/components/Maps.tssrc/core/ApiSchemas.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/components/Maps.ts
- resources/lang/en.json
- src/core/ApiSchemas.ts
🧰 Additional context used
🧠 Learnings (4)
📚 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/SinglePlayerModal.ts
📚 Learning: 2025-05-26T09:52:52.465Z
Learnt from: tnhnblgl
Repo: openfrontio/OpenFrontIO PR: 875
File: src/client/index.html:396-402
Timestamp: 2025-05-26T09:52:52.465Z
Learning: In the OpenFrontIO project, the correct path for icons from src/client/index.html is ../../resources/icons/, not /static/icons/ as the build process handles the path mapping differently.
Applied to files:
src/client/SinglePlayerModal.ts
📚 Learning: 2025-11-16T13:28:30.994Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2454
File: src/core/configuration/PastelTheme.ts:68-111
Timestamp: 2025-11-16T13:28:30.994Z
Learning: In the OpenFrontIO repository, JSDoc documentation should not be added inline for Theme interface methods in files like src/core/configuration/PastelTheme.ts, as documentation is placed elsewhere in the codebase per repository convention.
Applied to files:
src/client/SinglePlayerModal.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/SinglePlayerModal.ts
🪛 GitHub Actions: 🧪 CI
src/client/SinglePlayerModal.ts
[error] 1-1: Prettier formatting check failed. Run 'npx prettier --write .' to fix code style issues in this file.
🔇 Additional comments (2)
src/client/SinglePlayerModal.ts (2)
60-76: LGTM!Event listener lifecycle is handled correctly—subscribed in
connectedCallbackand cleaned up indisconnectedCallback.
1-1: Run Prettier to fix formatting.The pipeline reports a formatting issue. Run
npx prettier --write .to fix.⛔ Skipped due to learnings
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.
|
I had an idea: Can you please disable this feature for maps without nations? The Baikal (Nuke Wars) map has no nations... Fighting against just bots is too easy :D |
Contributes towards the ongoing task of adding achievements: #2706
Description:
Introduces a concept design and API implementation for singleplayer win achievements. New row of
54 medals is added to the map select screen in the Singleplayer modal, one for each difficulty:In order to achieve a medal in a particular map, you must win the singleplayer game (multiplayer and private match games don't count) in the selected difficulty without tampering with the options or settings. If any setting is changed from the default, regardless of the difficulty, you will
receive a fifth "Custom" medalnot receive the medal for that difficulty. Team games do not count towards the medal achievement.Completion of a medal will fill in the full correct color, as defined in our
variables.css:Completion medals can be toggled on or off (hidden by default) with the toggle button at the top of the section:
Screen.Recording.2025-12-29.155002.mp4
API implementation PR
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
bijx