Add configurable attack ratio keybind increment setting#2835
Add configurable attack ratio keybind increment setting#2835Aotumuri wants to merge 13 commits intoopenfrontio:mainfrom
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded a configurable attack-ratio increment setting and UI selector; keybinds now use the user-configured increment. Introduced a new SettingSelect component, a UserSettings accessor for the increment, localization entries (including an easter egg "Confidence in Victory"), and wiring in the input handler and settings modal. Changes
Sequence Diagram(s)sequenceDiagram
participant User as rgba(63,81,181,0.5) User
participant Modal as rgba(0,150,136,0.5) SettingsModal
participant Select as rgba(255,193,7,0.5) SettingSelect
participant Core as rgba(233,30,99,0.5) UserSettings
participant Input as rgba(33,150,243,0.5) InputHandler
participant Game as rgba(76,175,80,0.5) GameEngine
User->>Modal: open settings
Modal->>Select: render attack ratio increment options
User->>Select: choose increment (e.g., 5%)
Select-->>Modal: dispatch change event (value)
Modal->>Core: setFloat("settings.attackRatioIncrement", value)
Core-->>Modal: confirm stored
Note over Input,Core: Later, on keybind
Input->>Core: query attackRatioIncrement()
Core-->>Input: return increment (e.g., 5)
Input->>Game: apply attack ratio change by +/− increment
Game-->>User: updated ratio shown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 3
🤖 Fix all issues with AI agents
In @src/client/components/baseComponents/setting/SettingSelect.ts:
- Around line 42-49: The label/select pair in the SettingSelect component uses a
hardcoded id ("setting-select-input") which will duplicate across instances;
update the SettingSelect class to generate and store a unique id per instance
(e.g., this._inputId via a static counter or crypto.randomUUID()) in the
constructor, then replace the hardcoded id and the label's for attribute with
that instance id; alternatively you can remove the for/id relationship and wrap
the <select> inside the <label> to avoid IDs altogether—apply the change where
the label with for="setting-select-input" and the select
id="setting-select-input" are rendered.
In @src/client/graphics/layers/ControlPanel.ts:
- Around line 48-56: The code reads document.getElementById("attack-ratio")
without a null check and contains a confusing special-case that forces
newPercent = step; fix by first retrieving the element into a nullable variable
(const el = document.getElementById("attack-ratio") as HTMLInputElement | null),
guard for el being null (return early or default currentPercent = 0), use
parseFloat(el.value || "0") to get currentPercent safely, compute step and
newPercent normally (newPercent = currentPercent + event.attackRatio), remove
the opaque block that sets newPercent = step (or if snapping behavior is
required, replace it with a clear comment and explicit formula), then clamp
newPercent to [0,100] before computing newAttackRatio = newPercent / 100.
In @src/client/InputHandler.ts:
- Around line 370-380: The onShiftScroll handler still uses a hardcoded ±10 for
attack ratio changes; update the logic in onShiftScroll to call
this.userSettings.attackRatioIncrement() and use that value when emitting new
AttackRatioEvent instead of the fixed 10 (keep using this.eventBus.emit(new
AttackRatioEvent(...)) and preserve the existing sign logic for scroll
direction).
🧹 Nitpick comments (1)
src/client/UserSettingModal.ts (1)
451-458: Consider extracting options as a module-level constant.The
attackRatioOptionsarray is recreated on every render. Since it never changes, define it outside the class or as a static property for minor efficiency and clarity.+const ATTACK_RATIO_OPTIONS = [ + { value: 1, label: "1%" }, + { value: 2, label: "2%" }, + { value: 5, label: "5%" }, + { value: 10, label: "10%" }, + { value: 20, label: "20%" }, +] as const; + @customElement("user-setting") export class UserSettingModal extends LitElement {Then use
ATTACK_RATIO_OPTIONSin the render method.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
resources/lang/en.jsonsrc/client/InputHandler.tssrc/client/UserSettingModal.tssrc/client/components/baseComponents/setting/SettingSelect.tssrc/client/graphics/layers/ControlPanel.tssrc/client/styles/components/setting.csssrc/core/game/UserSettings.ts
🧰 Additional context used
🧠 Learnings (3)
📚 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/InputHandler.tssrc/client/graphics/layers/ControlPanel.tssrc/core/game/UserSettings.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-06-07T19:47:34.514Z
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/styles/components/setting.css:9-22
Timestamp: 2025-06-07T19:47:34.514Z
Learning: In src/client/styles/components/setting.css, the padding difference between .setting-item and .setting-item-group is intentional - .setting-item-group doesn't need padding for the specific grouped setting styling being implemented.
Applied to files:
src/client/styles/components/setting.css
🧬 Code graph analysis (1)
src/client/UserSettingModal.ts (2)
src/core/game/UserSettings.ts (1)
attackRatioIncrement(203-209)src/client/LangSelector.ts (1)
translateText(224-244)
🔇 Additional comments (6)
src/client/styles/components/setting.css (1)
230-237: LGTM!The new
.setting-input.selectclass follows the same pattern as.setting-input.number. Consistent styling.src/core/game/UserSettings.ts (1)
203-209: LGTM!Clean validation logic: rounds the stored value, falls back to 10 if invalid or non-positive. Defensive and straightforward.
resources/lang/en.json (1)
445-449: LGTM!Parameterized placeholders with
{amount}are correctly formatted. New label and description keys are clear and descriptive.src/client/UserSettingModal.ts (2)
193-208: LGTM!Handler correctly parses both string and number values, validates with
Number.isFinite, rounds before storing. Clean implementation.
572-578: Verify dropdown selection when stored value is not in options.If a user manually edits localStorage to a value not in the list (e.g., 3), the dropdown won't highlight any option. The
attackRatioIncrement()method rounds values, potentially creating mismatches. Consider clamping to nearest valid option or adding validation.src/client/components/baseComponents/setting/SettingSelect.ts (1)
1-66: Overall structure looks good.Clean component using typed union for
SelectOption. Composition-friendly design. Light DOM approach allows external styling. Event dispatch follows Lit conventions.
|
looks like there's a merge conflict from thhe new UI update |
It felt a bit disappointing that this component had one that was available in other components, so I decided to sneak in one.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/client/components/baseComponents/setting/SettingSelect.ts:
- Line 59: The SettingSelect component currently uses a hardcoded id
("setting-select-input") which causes duplicate IDs and broken label
associations when multiple instances render; update SettingSelect to generate a
unique id per instance (e.g., use React's useId or a small uid generator) and
assign that value to both the input/select's id and the label's htmlFor so each
component instance has a distinct id while preserving the label association
(reference the SettingSelect component and the id="setting-select-input" and
label htmlFor usage to locate the change).
- Around line 21-36: handleChange currently dispatches a CustomEvent whose
detail.value preserves the original option type (number | string) while
this.value is coerced to string; update handleChange to make this behavior
explicit: either (preferred) add a short inline comment above the dispatch (in
SettingSelect.handleChange) stating that detail.value may be number | string and
consumers must handle both, or (alternatively) normalize selectedValue to a
single type before dispatch (e.g., String(selectedValue)) and set both
this.value and detail.value to that normalized value so consumers always receive
a string. Reference the handleChange method, the options lookup
(this.options.find), this.value assignment, and the CustomEvent("change")
dispatch when making the change.
🧹 Nitpick comments (2)
src/client/KeybindsModal.ts (1)
4-4: Consider refreshing UserSettings on modal open.The
userSettingsinstance is created once at the class level. If a user changes the attack ratio increment in the settings modal while the keybinds modal is open (or has been opened before), the descriptions won't reflect the updated value until the component is recreated.♻️ Proposed fix to refresh settings when modal opens
Update the
onOpenmethod to refresh the UserSettings instance:protected onOpen(): void { + this.userSettings = new UserSettings(); this.requestUpdate(); }This ensures the keybind descriptions always show the current increment value.
Also applies to: 36-36
src/client/components/baseComponents/setting/SettingSelect.ts (1)
38-41: Consider extracting the long rainbow class string.The rainbow gradient class on line 40 is over 300 characters long, making it hard to read and maintain. Consider extracting it to a constant:
const RAINBOW_CLASSES = "bg-[linear-gradient(270deg,#990033,#996600,#336600,#008080,#1c3f99,#5e0099,#990033)] " + "bg-[length:1400%_1400%] animate-rainbow-bg text-white " + "hover:bg-[linear-gradient(270deg,#990033,#996600,#336600,#008080,#1c3f99,#5e0099,#990033)]";
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/client/KeybindsModal.tssrc/client/UserSettingModal.tssrc/client/components/baseComponents/setting/SettingSelect.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/UserSettingModal.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/KeybindsModal.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
🧬 Code graph analysis (1)
src/client/KeybindsModal.ts (3)
src/core/game/UserSettings.ts (1)
UserSettings(6-218)src/client/LangSelector.ts (1)
translateText(255-279)src/client/Utils.ts (1)
translateText(165-224)
🔇 Additional comments (9)
src/client/KeybindsModal.ts (2)
46-96: LGTM! Comprehensive validation logic.The validation for saved keybinds is thorough and defensive:
- Checks object structure (not null, not array)
- Validates each entry has required properties with correct types
- Handles both string and string array values
- Logs clear warnings for invalid data
This prevents runtime errors from corrupted localStorage data.
395-397: LGTM! Dynamic descriptions correctly implemented.The keybind descriptions properly use
translateText()with theamountparameter sourced fromthis.userSettings.attackRatioIncrement(). The placeholder will be replaced with the configured increment value at runtime.Also applies to: 407-409
resources/lang/en.json (3)
501-501: LGTM! Placeholder syntax correctly updated.The descriptions now use
{amount}%instead of hardcoded "10%", enabling dynamic keybind descriptions based on user settings. The ICU message format placeholder is correctly formatted.Also applies to: 503-503
504-505: Keys are properly integrated into UserSettingModal.The new
attack_ratio_increment_labelandattack_ratio_increment_desckeys are correctly used in src/client/UserSettingModal.ts (lines 425-426) viatranslateText()calls with theuser_setting.namespace. The translation strings in resources/lang/en.json align with the settings dropdown implementation.
465-473: No action needed. Theeaster_confidence_in_victoryentries are intentionally part of this PR and are properly connected to the UI. UserSettingModal.ts correctly references all translation keys, and the git history confirms this was a deliberate Easter egg feature addition.Likely an incorrect or invalid review comment.
src/client/components/baseComponents/setting/SettingSelect.ts (4)
1-7: LGTM! Clean typed union approach.The SelectOption type uses a typed union (number | string) rather than inheritance, which is idiomatic TypeScript. The structure is simple and clear.
9-15: LGTM! Properties are properly typed with sensible defaults.The use of Lit decorators with explicit types is correct, and storing value as a string aligns with HTML select element behavior.
17-19: LGTM! Light DOM rendering is appropriate here.Returning
thisto render in the light DOM allows the component to integrate with external Tailwind classes, which is the intended behavior.
61-72: LGTM! Property and attribute bindings are correct.The use of property binding (
.value=) on line 61, boolean attribute binding (?selected=) on line 68, and string conversion for option values are all correct Lit patterns. The string comparisons properly handle the fact that HTML select values are always strings.
|
please review this |
…o troop-rate- # Conflicts: # src/client/graphics/layers/ControlPanel.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/components/baseComponents/setting/SettingSelect.ts`:
- Around line 38-41: The component SettingSelect.ts sets rainbowClass using the
non-existent Tailwind class "animate-rainbow-bg"; either add a matching
animation entry to the Tailwind theme (animation + keyframes using the existing
"rainbow-background" keyframes) or change SettingSelect.ts to apply the existing
CSS class (e.g., ".setting-item.easter-egg") instead of "animate-rainbow-bg";
update the rainbowClass assignment (in render()) to reference the chosen
solution so the easter animation uses a defined animation name (referencing
animate-rainbow-bg, rainbow-background, or .setting-item.easter-egg as
appropriate).
In `@src/client/styles/components/setting.css`:
- Around line 285-292: Remove the dead CSS rule for the `.setting-input.select`
class from the stylesheet since the SettingSelect component only uses Tailwind
utilities and never references this selector; search for `.setting-input.select`
and `SettingSelect` to confirm no usages remain, delete the entire rule block
(the `.setting-input.select { ... }` declaration) and run the project lint/build
to ensure no regressions.
|
@evanpelle not really sure on the easter egg code approach? should it be in or out of this kind of pr? |
| } | ||
|
|
||
| render() { | ||
| const rainbowClass = this.easter |
There was a problem hiding this comment.
how is this easter egg triggered?
There was a problem hiding this comment.
There is an element that has the easter flag here:
https://github.com/Aotumuri/OpenFrontIO_/blob/1c9baadd283d89a1763bbd497703b5f9d9c0e93d/src/client/UserSettingModal.ts#L977
And the condition check is here:
https://github.com/Aotumuri/OpenFrontIO_/blob/1c9baadd283d89a1763bbd497703b5f9d9c0e93d/src/client/UserSettingModal.ts#L223C3-L223C10
This condition was not introduced in this PR; it was already present and is reused as-is.
5 participants
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #2822
Description:
Adds an attack ratio keybind increment setting with a new dropdown UI, wires keybinds to use the configured step, updates the attack ratio adjustment logic, and makes the select reflect stored settings.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
aotumuri