Add help notification system to control panel ℹ️#4212
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a help-messages toggle and translations, a Config threshold, and ControlPanel logic that tracks recent outgoing attacks and nearby players to compute and render contextual warning/info notifications during gameplay when enabled. ChangesHelp Messages Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/client/hud/layers/ControlPanel.ts (1)
120-131: ⚖️ Poor tradeoffConsider caching notification computation.
The notification is recomputed every tick (every 100ms) when help is enabled, but the border cache only refreshes every 10 ticks (1 second). Most ticks will recompute the notification with identical inputs.
While not a bug, you could improve efficiency by only recomputing when:
- The border cache updates
- Troop counts change significantly
- Attack state changes
This would reduce unnecessary work on 9 out of 10 ticks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/hud/layers/ControlPanel.ts` around lines 120 - 131, The code recomputes this._notification each tick in ControlPanel when help is enabled, causing unnecessary work; change the logic to cache the last computed notification and only call computeNotification(player, config) when inputs change—specifically when refreshNearbyPlayers(player) actually updates its border cache, when tracked values from trackOutgoingAttacks(player) or troop counts change, or when attack state transitions occur; implement a simple dirty flag or last-state snapshot (e.g., lastBorderKey, lastTroopCounts, lastAttackState) and update this._notification only when those change, otherwise reuse the cached value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/hud/layers/ControlPanel.ts`:
- Around line 246-248: The troop-threshold check in ControlPanel (use of
this._troops) is off by a factor of 10; update the conditional in the
ControlPanel method that returns the low-troops warning (the block currently
using this._troops < 10000 && this._troops > 0) to use 1000 instead of 10000
(i.e., this._troops < 1000 && this._troops > 0) so the
"control_panel.low_troops_warning" triggers at the intended 1k threshold.
In `@src/core/configuration/Config.ts`:
- Around line 542-544: The armyLimitWarningThreshold() method currently returns
0.8 but should match the intended "approximately 90%" value—update
armyLimitWarningThreshold() to return 0.9 (or a constant named
ARMY_LIMIT_WARNING_THRESHOLD = 0.9) and add unit tests under the test suite to
assert the returned value (e.g., test Config.armyLimitWarningThreshold() ===
0.9) so the behavior is covered; reference the Config.armyLimitWarningThreshold
function when making the change and in the new test file.
In `@src/core/game/UserSettings.ts`:
- Around line 210-216: Add unit tests for UserSettings.helpMessages() and
UserSettings.toggleHelpMessages(): write three tests that (1) assert
helpMessages() returns true when "settings.helpMessages" is absent from storage,
(2) call toggleHelpMessages() and assert the in-memory value flips (true→false
or false→true) via helpMessages(), and (3) verify persistence by checking that
the boolean is written to and read from localStorage under the key
"settings.helpMessages" (set localStorage directly, instantiate or use the same
UserSettings, then assert helpMessages() reflects the stored value). Use the
existing test harness/mocks for localStorage and reference the methods
helpMessages() and toggleHelpMessages() when implementing assertions.
---
Nitpick comments:
In `@src/client/hud/layers/ControlPanel.ts`:
- Around line 120-131: The code recomputes this._notification each tick in
ControlPanel when help is enabled, causing unnecessary work; change the logic to
cache the last computed notification and only call computeNotification(player,
config) when inputs change—specifically when refreshNearbyPlayers(player)
actually updates its border cache, when tracked values from
trackOutgoingAttacks(player) or troop counts change, or when attack state
transitions occur; implement a simple dirty flag or last-state snapshot (e.g.,
lastBorderKey, lastTroopCounts, lastAttackState) and update this._notification
only when those change, otherwise reuse the cached value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 658e4998-950f-4403-afef-9662d0e26f50
📒 Files selected for processing (5)
resources/lang/en.jsonsrc/client/hud/layers/ControlPanel.tssrc/client/hud/layers/SettingsModal.tssrc/core/configuration/Config.tssrc/core/game/UserSettings.ts
| this.troopRate = this.game.config().troopIncreaseRate(player) * 10; | ||
| this.troopRate = config.troopIncreaseRate(player) * 10; | ||
|
|
||
| const helpEnabled = new UserSettings().helpMessages(); |
There was a problem hiding this comment.
I think we should also do a check like: getGamesPlayed() < 10. Since we don't want to start showing this to veteran players.
There was a problem hiding this comment.
Ah I didn't even knew that we are tracking that in localstorage
It says 977 in my prod localstorage, crazy
I added it
But I went with 20, 10 games are very quickly over if you are a noob
Resolves #3445
Description:
I copied the PR #3743 from @luctrate (Add army limit warning indicator for team games) to this PR because he didn't respond to requested changes but I thought it's important.
I expanded on it, now its a full help message system:
Warnings (orange):
Info messages (blue):
Info messages only appear when the player has not attacked the relevant neighbor for at least 15 seconds, so they do not show up without reason.
New "Help Messages" toggle in settings (default: on)
Implementation details:
AI Model used: MiMo 2.5 Pro
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
FloPinguin