Make setting for go to player on spawn (default on)#3874
Open
frederikja163 wants to merge 4 commits intoopenfrontio:mainfrom
Open
Make setting for go to player on spawn (default on)#3874frederikja163 wants to merge 4 commits intoopenfrontio:mainfrom
frederikja163 wants to merge 4 commits intoopenfrontio:mainfrom
Conversation
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a persisted user setting and UI toggle to control whether the client camera auto-zooms to the player on game start. ChangesGo-to-Player Toggle Setting
Sequence DiagramsequenceDiagram
actor Player
participant Modal as UserSettingModal
participant Settings as UserSettings
participant Runner as ClientGameRunner
participant Game as GameEngine
Player->>Modal: Toggle "Go to player"
Modal->>Settings: toggleGoToPlayer()
Settings->>Settings: Flip goToPlayer state
Note over Player,Game: Game starts / turn arrives
Game->>Runner: "turn" message
Runner->>Settings: goToPlayer()
Settings-->>Runner: true / false
alt goToPlayer() is true
Runner->>Game: Emit GoToPlayerEvent
Game->>Game: Zoom to player
else
Note over Game: Skip initial zoom
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/core/game/UserSettings.ts (1)
232-234: 💤 Low valueConsider adding tests per the
src/core/guideline.The coding guideline requires tests for all
src/core/changes. While no existingUserSettingsmethods have tests, this PR is a good opportunity to add a small suite coveringgoToPlayer()default andtoggleGoToPlayer()round-trip.As per coding guidelines: "All changes to
src/core/must include tests."🤖 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/core/game/UserSettings.ts` around lines 232 - 234, Add unit tests for UserSettings covering the default value of goToPlayer() and that toggleGoToPlayer() flips the setting and can be toggled back (round-trip); specifically, create tests that instantiate UserSettings, assert goToPlayer() default state, call toggleGoToPlayer(), assert the inverted state, call toggleGoToPlayer() again and assert it returned to the original state, and include any necessary setup/teardown to isolate state so these tests live alongside other src/core tests.
🤖 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/ClientGameRunner.ts`:
- Line 457: The logic is inverted: hasGoneToPlayer is intended to mean "the
zoom-to-player action has already happened" but it's initialized directly from
this.userSettings.goToPlayer(), causing the feature to behave backwards; change
the initialization so hasGoneToPlayer is the negation of the user preference
(i.e., set hasGoneToPlayer = !this.userSettings.goToPlayer()) so the guard that
emits GoToPlayerEvent (which checks !hasGoneToPlayer) correctly triggers when
the user enables goToPlayer(); update the initialization near ClientGameRunner's
hasGoneToPlayer declaration accordingly.
In `@src/client/UserSettingModal.ts`:
- Line 849: The second toggle input in the UserSettingModal component reuses
id="territory-patterns-toggle", causing duplicate DOM IDs; update that input's
id to a unique value (e.g., "territory-patterns-toggle-advanced" or similar)
wherever the toggle is rendered (look for the input/label within
UserSettingModal and any helper like renderToggle) and also update any
corresponding label htmlFor attributes and any JS/CSS selectors or tests that
reference the old id so they point to the new unique id.
- Line 851: The template currently calls this.userSettings.toggleGoToPlayer()
during render, causing the setting to flip on every render; change the binding
to pass a function reference instead of invoking it — either add a private
wrapper on the component (e.g. _onToggleGoToPlayer that calls
this.userSettings.toggleGoToPlayer()) and use that in the template, or pass the
method reference/arrow handler (e.g. this.userSettings.toggleGoToPlayer or an
inline () => this.userSettings.toggleGoToPlayer()), mirroring how the other
toggles in UserSettingModal use bound method references.
---
Nitpick comments:
In `@src/core/game/UserSettings.ts`:
- Around line 232-234: Add unit tests for UserSettings covering the default
value of goToPlayer() and that toggleGoToPlayer() flips the setting and can be
toggled back (round-trip); specifically, create tests that instantiate
UserSettings, assert goToPlayer() default state, call toggleGoToPlayer(), assert
the inverted state, call toggleGoToPlayer() again and assert it returned to the
original state, and include any necessary setup/teardown to isolate state so
these tests live alongside other src/core tests.
🪄 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: 5f759a1e-77e7-4bca-9415-62f69164ed31
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/client/ClientGameRunner.tssrc/client/UserSettingModal.tssrc/core/game/UserSettings.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/UserSettingModal.ts (1)
851-851:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate a local wrapper method for the "Go to Player" toggle to match the established pattern.
At line 851,
@change=${this.userSettings.toggleGoToPlayer}passes an unbound method reference. This will lose itsthiscontext at runtime and break the toggle. Every other toggle handler in this component (dark mode, emojis, alert frame, fx layer, etc.) uses a local wrapper method. Add the same wrapper pattern here:+ private toggleGoToPlayer() { + this.userSettings.toggleGoToPlayer(); + } + private togglePerformanceOverlay() { this.userSettings.togglePerformanceOverlay(); }Then bind it in the template:
- `@change`=${this.userSettings.toggleGoToPlayer} + `@change`=${this.toggleGoToPlayer}🤖 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/UserSettingModal.ts` at line 851, The template currently passes an unbound method reference this.userSettings.toggleGoToPlayer which will lose its this context; create a local wrapper method onToggleGoToPlayer(event) on the UserSettingModal class that calls this.userSettings.toggleGoToPlayer(event) (or forwards any needed args) and then change the template binding from `@change`=${this.userSettings.toggleGoToPlayer} to `@change`=${this.onToggleGoToPlayer} so the handler keeps the correct instance context and matches the existing wrapper pattern used by other toggles (e.g., dark mode/emojis handlers).
🤖 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.
Duplicate comments:
In `@src/client/UserSettingModal.ts`:
- Line 851: The template currently passes an unbound method reference
this.userSettings.toggleGoToPlayer which will lose its this context; create a
local wrapper method onToggleGoToPlayer(event) on the UserSettingModal class
that calls this.userSettings.toggleGoToPlayer(event) (or forwards any needed
args) and then change the template binding from
`@change`=${this.userSettings.toggleGoToPlayer} to
`@change`=${this.onToggleGoToPlayer} so the handler keeps the correct instance
context and matches the existing wrapper pattern used by other toggles (e.g.,
dark mode/emojis handlers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9587dc98-ff5f-4a24-9d43-8b44b6e900f7
📒 Files selected for processing (2)
src/client/ClientGameRunner.tssrc/client/UserSettingModal.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/ClientGameRunner.ts
evanpelle
reviewed
May 7, 2026
| }; | ||
|
|
||
| let hasGoneToPlayer = false; | ||
| let hasGoneToPlayer = !this.userSettings.goToPlayer(); |
Collaborator
There was a problem hiding this comment.
I think this is a bit confusing, becasue we haven't already gone to the player. instead just return early if gotoplayer is disabled.
Author
There was a problem hiding this comment.
@evanpelle this should be fixed in the latest commit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #3847
Description:
Makes a setting to turn off go to player on spawn.
Most players i have played with are really annoyed about this feature. I understand that some people like it, which is why i have made it default on. But i feel like we should be able to turn it off.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
FrederikJA