Dedicated gameplay settings menu for difficulty #383
Merged
Conversation
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
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 4.32.0 to 4.32.2. - [Release notes](https://github.com/github/codeql-action/releases) - [Commits](github/codeql-action@v4.32.0...v4.32.2) --- updated-dependencies: - dependency-name: github/codeql-action dependency-version: 4.32.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
…/codeql-action-4.32.2 Bump github/codeql-action from 4.32.0 to 4.32.2
Feature Request: Dedicated Gameplay Settings Menu for Difficulty Description: Move difficulty setting to a new gameplay_settings.tscn in SkyLockAssault using Godot 4.5, accessed via button in options menu (advised as relevant for game mechanics like difficulty, fuel, weapons). Why is this useful?: Groups gameplay-related tweaks (e.g., difficulty affecting levels, weapons) separately for focused adjustment in multi-level combat game.
Introduce a new gameplay button to the main shell and wire up its callbacks and visibility across settings screens. custom_shell.html: add #gameplay-button and hook it to window.gameplayPressed. scripts/*: replace references that showed the difficulty slider with showing the gameplay button when exiting submenus (advanced, audio, gameplay back flows). tests/difficulty_flow_test.py: update the end-to-end test to navigate into Gameplay Settings via the new button, use gameplayBackPressed to return, and assert options-back-button is present before returning to the main menu. These changes ensure the Gameplay settings are reachable and properly toggled in the UI and test flow.
ikostan
added this to the
Milestone 11: Polish & Platform-wide Enhancements (Menus, Testing & Configuration) milestone
ikostan
added
testing
CI/CD
menu
GUI
dependencies
Contributor
Reviewer's GuideIntroduce a dedicated Gameplay Settings menu/scene for difficulty management, refactor options and web overlay integration to route difficulty changes and navigation through this new screen, update automated tests to follow the new flow, and bump CodeQL/SARIF-related CI actions. Sequence diagram for navigating to Gameplay Settings and changing difficulty (web overlay)sequenceDiagram
actor WebPlayer
participant Browser
participant MainMenu
participant OptionsMenu
participant GameplaySettings
participant Globals
WebPlayer->>Browser: Click START
Browser->>MainMenu: window.startPressed([])
MainMenu->>MainMenu: _on_start_pressed()
WebPlayer->>Browser: Click OPTIONS
Browser->>MainMenu: window.optionsPressed([])
MainMenu->>OptionsMenu: Instantiate options_menu.tscn
OptionsMenu->>Browser: Show controls/audio/advanced/gameplay/options-back buttons
WebPlayer->>Browser: Click GAMEPLAY SETTINGS
Browser->>OptionsMenu: window.gameplayPressed([])
OptionsMenu->>OptionsMenu: _on_gameplay_settings_button_pressed()
OptionsMenu->>GameplaySettings: Instantiate gameplay_settings.tscn
OptionsMenu->>OptionsMenu: Push self to Globals.hidden_menus
OptionsMenu->>OptionsMenu: visible = false
GameplaySettings->>Browser: Show difficulty-slider, gameplay-back, gameplay-reset
GameplaySettings->>Browser: Set window.changeDifficulty, window.gameplayBackPressed, window.gameplayResetPressed
WebPlayer->>Browser: Drag difficulty slider / change value
Browser->>GameplaySettings: window.changeDifficulty([ [value] ])
GameplaySettings->>GameplaySettings: _on_change_difficulty_js(args)
GameplaySettings->>GameplaySettings: _on_difficulty_value_changed(value)
GameplaySettings->>Globals: Globals.difficulty = value
GameplaySettings->>Globals: Globals._save_settings()
WebPlayer->>Browser: Click GAMEPLAY RESET
Browser->>GameplaySettings: window.gameplayResetPressed([])
GameplaySettings->>GameplaySettings: _on_gameplay_reset_button_pressed()
GameplaySettings->>GameplaySettings: difficulty_slider.value = _default_difficulty
GameplaySettings->>Globals: Globals._save_settings()
WebPlayer->>Browser: Click GAMEPLAY BACK
Browser->>GameplaySettings: window.gameplayBackPressed([])
GameplaySettings->>GameplaySettings: _on_gameplay_back_button_pressed()
GameplaySettings->>Globals: Pop previous menu from Globals.hidden_menus
GameplaySettings->>OptionsMenu: prev_menu.visible = true
GameplaySettings->>Browser: Hide gameplay overlays, show options overlays
GameplaySettings->>GameplaySettings: queue_free()
Updated class diagram for OptionsMenu and the new GameplaySettingsclassDiagram
class MainMenu {
<<Node>>
- Panel ui_panel
- VBoxContainer ui_container
- PackedScene options_menu
- Button start_button
- Button options_button
- Button quit_button
- ConfirmationDialog quit_dialog
- JavaScriptObject _start_pressed_cb
- JavaScriptObject _options_pressed_cb
- JavaScriptObject _quit_pressed_cb
+ _ready() void
+ _on_start_pressed() void
+ _on_options_button_pressed() void
+ _on_quit_pressed() void
}
class OptionsMenu {
<<Control>>
- JavaScriptBridgeWrapper js_bridge_wrapper
- OSWrapper os_wrapper
- PackedScene audio_scene
- PackedScene controls_scene
- PackedScene advanced_scene
- PackedScene gameplay_settings_scene
- JavaScriptObject _options_back_button_pressed_cb
- JavaScriptObject _controls_pressed_cb
- JavaScriptObject _audio_pressed_cb
- JavaScriptObject _advanced_pressed_cb
- JavaScriptObject _gameplay_settings_pressed_cb
- Button options_back_button
- Button audio_settings_button
- Button key_mapping_button
- Button gameplay_settings_button
- Label version_label
+ _ready() void
+ _on_options_back_button_pressed() void
+ _on_audio_settings_button_pressed() void
+ _on_key_mapping_button_pressed() void
+ _on_advanced_settings_button_pressed() void
+ _on_gameplay_settings_button_pressed() void
+ _on_options_back_button_pressed_js(Array args) void
+ _on_audio_pressed_js(Array args) void
+ _on_controls_pressed_js(Array args) void
+ _on_advanced_pressed_js(Array args) void
+ _on_gameplay_settings_pressed_js(Array args) void
}
class GameplaySettings {
<<Control>>
- JavaScriptBridgeWrapper js_bridge_wrapper
- OSWrapper os_wrapper
- JavaScriptObject js_window
- JavaScriptObject _change_difficulty_cb
- JavaScriptObject _gameplay_back_button_pressed_cb
- JavaScriptObject _gameplay_reset_cb
- bool _intentional_exit
- float _default_difficulty
- HSlider difficulty_slider
- Label difficulty_label
- Button gameplay_back_button
- Button gameplay_reset_button
+ _ready() void
+ _on_tree_exited() void
+ _unset_gameplay_settings_window_callbacks() void
+ _on_gameplay_reset_button_pressed() void
+ _on_gameplay_reset_js(Array args) void
+ _on_gameplay_back_button_pressed() void
+ _on_gameplay_back_button_pressed_js(Array args) void
+ _on_difficulty_value_changed(float value) void
+ _on_change_difficulty_js(Array args) void
}
class Globals {
<<Autoload>>
+ float difficulty
+ Array hidden_menus
+ _save_settings() void
+ log_message(String message, int level) void
}
class JavaScriptBridgeWrapper {
+ get_interface(String name) JavaScriptObject
+ create_callback(Callable callable) JavaScriptObject
+ eval(String code, bool use_global_context) Variant
}
class OSWrapper {
+ has_feature(String feature) bool
}
MainMenu --> OptionsMenu : instantiates
OptionsMenu --> GameplaySettings : instantiates
OptionsMenu --> JavaScriptBridgeWrapper
OptionsMenu --> OSWrapper
GameplaySettings --> JavaScriptBridgeWrapper
GameplaySettings --> OSWrapper
GameplaySettings --> Globals : reads/writes difficulty
OptionsMenu --> Globals : uses hidden_menus
Globals --> "*" Node : hidden_menus elements
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
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:
📝 WalkthroughWalkthroughRefactors the embedded Difficulty UI into a standalone Gameplay Settings scene and controller; replaces difficulty DOM hooks with Changes
Sequence DiagramsequenceDiagram
actor User
participant OptionsMenu as Options Menu
participant JSBridge as JavaScript Bridge
participant Gameplay as Gameplay Settings Scene
participant DOM as Web DOM
User->>OptionsMenu: Click Gameplay Settings Button
OptionsMenu->>JSBridge: expose gameplayPressed callback
OptionsMenu->>Gameplay: load gameplay_settings.tscn
Gameplay->>Gameplay: initialize slider & UI
Gameplay->>JSBridge: expose changeDifficulty/back/reset callbacks
Gameplay->>DOM: show gameplay-button overlay
User->>Gameplay: adjust difficulty slider
Gameplay->>Gameplay: persist difficulty, update label
User->>Gameplay: click Back
Gameplay->>OptionsMenu: teardown and return
OptionsMenu->>DOM: hide gameplay-button, restore options UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- In
gameplay_settings.gd, thejs_windowmember is never assigned because_ready()declares a new localvar js_window, so callbacks and cleanup that rely on the memberjs_windowwill never run as expected; assign to the member instead of shadowing it. - The gameplay settings JS integration looks inconsistent: you assign
js_window.changeDifficultybut_unset_gameplay_settings_window_callbacks()clearschangeDifficultyLevel, andgameplayResetPressed/_on_gameplay_reset_js are never wired up to the window object—align the property names and ensure all callbacks you define have matching registrations and teardown. - In
custom_shell.html, the new#gameplay-buttonstill uses the label textADVANCED, which is likely confusing now that there is a distinct gameplay settings menu; consider updating the button text to reflect its new purpose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `gameplay_settings.gd`, the `js_window` member is never assigned because `_ready()` declares a new local `var js_window`, so callbacks and cleanup that rely on the member `js_window` will never run as expected; assign to the member instead of shadowing it.
- The gameplay settings JS integration looks inconsistent: you assign `js_window.changeDifficulty` but `_unset_gameplay_settings_window_callbacks()` clears `changeDifficultyLevel`, and `gameplayResetPressed`/_on_gameplay_reset_js are never wired up to the window object—align the property names and ensure all callbacks you define have matching registrations and teardown.
- In `custom_shell.html`, the new `#gameplay-button` still uses the label text `ADVANCED`, which is likely confusing now that there is a distinct gameplay settings menu; consider updating the button text to reflect its new purpose.
## Individual Comments
### Comment 1
<location> `scripts/gameplay_settings.gd:54` </location>
<code_context>
+ )
+ )
+ # Expose callbacks to JS (store refs to prevent GC)
+ var js_window: JavaScriptObject = js_bridge_wrapper.get_interface("window")
+ if js_window:
+ _change_difficulty_cb = js_bridge_wrapper.create_callback(
</code_context>
<issue_to_address>
**issue (bug_risk):** Local `js_window` shadows the member, preventing later cleanup logic from ever seeing the window reference.
This keeps the member `js_window` as `null`, so `_unset_gameplay_settings_window_callbacks()` and the web-specific branch in `_on_gameplay_back_button_pressed()` never run. Assign the interface to the member instead (e.g. `js_window = js_bridge_wrapper.get_interface("window")`) and reuse that reference.
</issue_to_address>
### Comment 2
<location> `scripts/gameplay_settings.gd:105-108` </location>
<code_context>
+ ##
+ ## :rtype: void
+ Globals.log_message("Gameplay Settings Back button pressed.", Globals.LogLevel.DEBUG)
+ _teardown() # Centralized cleanup
+
+ var hidden_menu_found: bool = false
+ if not Globals.hidden_menus.is_empty():
+ var prev_menu: Node = Globals.hidden_menus.pop_back()
+ if is_instance_valid(prev_menu):
</code_context>
<issue_to_address>
**issue (bug_risk):** Hidden menu stack is popped twice during gameplay back handling, which will desynchronize the menu stack.
`_teardown()` already pops from `Globals.hidden_menus` and shows the previous menu. `_on_gameplay_back_button_pressed()` then pops again and toggles visibility, causing two removals per back press and potentially surfacing the wrong menu or emptying the stack early. Consolidate the pop-and-show behavior in a single place and let the other function handle only logging/cleanup.
</issue_to_address>
### Comment 3
<location> `scripts/gameplay_settings.gd:77` </location>
<code_context>
+func _unset_gameplay_settings_window_callbacks() -> void:
+ if not os_wrapper.has_feature("web") or not js_window:
+ return
+ js_window.changeDifficultyLevel = null
+ js_window.gameplayBackPressed = null
+ js_window.gameplayResetPressed = null
</code_context>
<issue_to_address>
**issue (bug_risk):** Callback cleanup uses a different property name (`changeDifficultyLevel`) than the one that is registered (`changeDifficulty`).
In `_ready()` you assign `js_window.changeDifficulty = _change_difficulty_cb`, but here you clear `js_window.changeDifficultyLevel`. As a result, the registered callback is never removed and can be invoked after the menu is closed. Update the cleanup to clear the same property used during registration (i.e. `changeDifficulty`).
</issue_to_address>
### Comment 4
<location> `custom_shell.html:150` </location>
<code_context>
<button id="controls-button" style="top: 21%; left: 50%; transform: translate(-50%, -50%); width: 200px; height: 50px; display: none;">CONTROLS</button>
<button id="audio-button" style="top: 31%; left: 50%; transform: translate(-50%, -50%); width: 200px; height: 50px; display: none;">AUDIO</button>
<button id="advanced-button" style="top: 41%; left: 50%; transform: translate(-50%, -50%); width: 200px; height: 50px; display: none;">ADVANCED</button>
+ <button id="gameplay-button" style="top: 51%; left: 50%; transform: translate(-50%, -50%); width: 200px; height: 50px; display: none;">ADVANCED</button>
<select id="log-level-select" style="top: 55%;">
<option>DEBUG</option>
</code_context>
<issue_to_address>
**nitpick:** Gameplay button label is still "ADVANCED", which is likely confusing.
Having both this and the existing advanced button labeled "ADVANCED" makes the overlay ambiguous. Please rename this one (e.g., to "GAMEPLAY") so the action is clearly distinct.
</issue_to_address>
### Comment 5
<location> `custom_shell.html:207-210` </location>
<code_context>
document.getElementById('audio-button').onclick = () => window.audioPressed([]);
document.getElementById('start-button').onclick = () => window.startPressed([]);
document.getElementById('options-button').onclick = () => window.optionsPressed([]);
+ document.getElementById('gameplay-button').onclick = () => window.gameplayPressed([]);
document.getElementById('advanced-button').onclick = () => window.advancedPressed([]);
document.getElementById('quit-button').onclick = () => window.quitPressed([]);
</code_context>
<issue_to_address>
**issue (bug_risk):** JS expects `window.gameplayResetPressed`, but the GDScript never exposes that callback.
The shell binds `gameplay-reset-button` to `window.gameplayResetPressed([])`, but `gameplay_settings.gd` only defines `_on_gameplay_reset_js()` and never assigns `js_window.gameplayResetPressed`, so clicking reset on web will throw at runtime. Please mirror the `gameplayBackPressed` pattern: add a `_gameplay_reset_button_pressed_cb`, assign it to `js_window.gameplayResetPressed`, and clear it in `_unset_gameplay_settings_window_callbacks()`.
</issue_to_address>
### Comment 6
<location> `tests/difficulty_flow_test.py:130-133` </location>
<code_context>
# page.click("#advanced-back-button", force=True)
page.evaluate("window.advancedBackPressed([0])")
+ # Go to Gameplay Settings
+ page.wait_for_selector('#gameplay-button', state='visible', timeout=2500)
+ # page.click("#advanced-back-button", force=True)
+ page.evaluate("window.gameplayPressed([0])")
+
# Set difficulty to 2.0 - directly call the exposed callback (bypasses event for reliability in automation)
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that the gameplay settings menu actually loaded after `gameplayPressed` is invoked.
Currently we only wait for `#gameplay-button` and invoke `window.gameplayPressed([0])`, but never verify that the gameplay settings screen appears. Please add an assertion for a gameplay-specific element (e.g., a unique heading, difficulty slider, or control) so the test actually validates the new screen and can catch regressions where the button exists but navigation is broken.
Suggested implementation:
```python
# Go to Gameplay Settings
page.wait_for_selector('#gameplay-button', state='visible', timeout=2500)
# page.click("#advanced-back-button", force=True)
page.evaluate("window.gameplayPressed([0])")
# Assert that the gameplay settings menu has loaded by waiting for a gameplay-specific control
page.wait_for_selector('#difficulty-slider', state='visible', timeout=2500)
# Set difficulty to 2.0 - directly call the exposed callback (bypasses event for reliability in automation)
pre_change_log_count = len(logs)
page.wait_for_function('window.changeDifficulty !== undefined', timeout=2500)
```
1. Replace `#difficulty-slider` with the actual selector for a gameplay-specific element on your gameplay settings screen (e.g. a heading, slider, or checkbox that is unique to that view).
2. If the gameplay settings screen uses a different visibility pattern (e.g. is present in DOM but toggled via classes), you may need to adjust `state='visible'` to `state='attached'` or use a more appropriate Playwright wait condition.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/options_menu.gd (1)
10-14:⚠️ Potential issue | 🟡 MinorStale docstring references removed variables.
Lines 13–14 still reference
difficulty_slideranddifficulty_label, which have been removed in this PR. Update the file-level docstring to reflect the current state (e.g., mentiongameplay_settings_button).Proposed fix
-## :vartype difficulty_slider: HSlider -## :vartype difficulty_label: Label +## :vartype gameplay_settings_button: Button
🤖 Fix all issues with AI agents
In @.github/workflows/trivy.yml:
- Line 34: The inline comment on the pinned SHA for the action referenced by
uses:
"github/codeql-action/upload-sarif@b13d724d35ff0a814e21683638ed68ed34cf53d1" is
stale/incorrect; either update the pinned ref to the same v4.32.2 release used
elsewhere (replace the untagged SHA with the v4.32.2 tag) for consistency with
codeql.yml, or correct the comment to state that this is an untagged commit and
provide the accurate commit date and provenance; ensure the change targets the
uses: "github/codeql-action/upload-sarif@..." entry in
.github/workflows/trivy.yml so the comment and the pinned ref agree.
In `@custom_shell.html`:
- Line 150: Replace the hardcoded label text for the element with id
"gameplay-button" (currently "ADVANCED") so it reads "GAMEPLAY"; locate the
<button id="gameplay-button"> in the template (next to the "advanced-button")
and update its visible text content and/or aria-label to "GAMEPLAY" to keep
markup consistent and accessible while preserving existing styles and
attributes.
In `@scenes/gameplay_settings.tscn`:
- Line 82: Update the user-visible tooltip string in the gameplay_settings scene
by changing the tooltip_text value currently set to "Advanced Settings" to
"Gameplay Settings" so the panel's tooltip correctly reflects the scene; locate
the tooltip_text entry in scenes/gameplay_settings.tscn and replace the string
literal accordingly.
In `@scripts/gameplay_settings.gd`:
- Around line 96-141: The back handler _on_gameplay_back_button_pressed
currently calls _teardown() which already pops from Globals.hidden_menus and
then the inline block pops again, causing a double-pop; remove the _teardown()
call from _on_gameplay_back_button_pressed and keep the inline pop/visibility
logic that references Globals.hidden_menus and prev_menu (and still call
_unset_gameplay_settings_window_callbacks(), set _intentional_exit, and
queue_free()), so behavior matches advanced_settings.gd and audio_settings.gd
and the hidden menu stack is only modified once.
- Around line 25-71: In _ready(), attach the Node.tree_exited signal to a
handler (e.g., _on_tree_exited) like the other settings scripts, and add an
_on_tree_exited method that restores the previous menu and cleans up UI/JS
state: disconnect difficulty_slider.value_changed and
gameplay_back_button.pressed/gameplay_reset_button.pressed connections if
connected, remove or null out JS callbacks stored in _change_difficulty_cb and
_gameplay_back_button_pressed_cb on js_window via js_bridge_wrapper, and
hide/remove the web overlays (the same DOM elements you showed) so the menu
stack and JS refs are restored/cleared when the node is unexpectedly removed.
Ensure the handler references the same symbols used in this file (_ready,
difficulty_slider, gameplay_back_button, gameplay_reset_button,
js_bridge_wrapper, js_window, _change_difficulty_cb,
_gameplay_back_button_pressed_cb).
- Line 54: The local var declaration `var js_window` inside _ready() shadows the
class-level field `js_window`, leaving the field null so cleanup methods like
_unset_gameplay_settings_window_callbacks and _on_gameplay_back_button_pressed
never run; remove the local declaration and assign the result of
js_bridge_wrapper.get_interface("window") directly to the class field
`js_window` in _ready() so subsequent checks and cleanup operate on the real
object and JS callbacks are properly removed.
- Around line 74-79: The cleanup function
_unset_gameplay_settings_window_callbacks nullifies
js_window.changeDifficultyLevel but the callback was registered as
changeDifficulty, and it also clears gameplayResetPressed even though no
callback was ever wired; update the cleanup to null out the actual registered
property js_window.changeDifficulty and remove or add nulling for
gameplayResetPressed only if you register it. If gameplayResetPressed should
exist, register it in _ready() using js_bridge_wrapper.create_callback with
Callable(self, "_on_gameplay_reset_js") and assign that to
js_window.gameplayResetPressed, then keep the corresponding null assignment in
_unset_gameplay_settings_window_callbacks; otherwise remove the
gameplayResetPressed nulling to avoid clearing an unregistered property.
- Around line 199-203: The code indexes args[0][0] without validating that
args[0] is an Array (or has at least one element) which can cause a runtime
crash; update the JS callback in gameplay_settings.gd to first verify
args.size() > 0, args[0] is an Array (or has a valid operator "has" / "size"),
and args[0].size() > 0, and that args[0][0] is convertible to a float before
using it; if any check fails, call Globals.log_message with a descriptive
DEBUG/ERROR message and return early instead of proceeding to set the local
variable value, so the existing Globals.log_message call and the variable value
remain but only after these guards.
In `@scripts/options_menu.gd`:
- Around line 110-114: There is trailing whitespace on the blank line before the
_gameplay_settings_pressed_cb block causing gdformat failures; remove the
trailing spaces on that blank line so the file conforms to gdformat. Locate the
area around _gameplay_settings_pressed_cb, js_bridge_wrapper.create_callback,
Callable(self, "_on_gameplay_settings_pressed_js") and js_window.gameplayPressed
and delete the trailing whitespace on the empty line immediately above the
_gameplay_settings_pressed_cb assignment.
🧹 Nitpick comments (5)
custom_shell.html (1)
228-229: Copy-paste comment: "advanced back" should be "gameplay back".Proposed fix
- document.getElementById('gameplay-back-button').onclick = () => window.gameplayBackPressed([]); // Add this for advanced back - document.getElementById('gameplay-reset-button').onclick = () => window.gameplayResetPressed([]); // Add for reset + document.getElementById('gameplay-back-button').onclick = () => window.gameplayBackPressed([]); // Add this for gameplay back + document.getElementById('gameplay-reset-button').onclick = () => window.gameplayResetPressed([]); // Add for gameplay resetscenes/gameplay_settings.tscn (1)
139-152: Scene defaults are inconsistent with script defaults.The slider's initial
value = 0.5(line 142) and the label'stext = "{0.0}"(line 152) don't match_default_difficulty = 1.0ingameplay_settings.gd. While_ready()will override these, aligning scene defaults reduces confusion when editing the scene in the Godot editor.Proposed fix
-value = 0.5 +value = 1.0-text = "{0.0}" +text = "{1.0}"tests/difficulty_flow_test.py (1)
130-133: Stale commented-out code referencesadvanced-back-button.Line 132's comment (
# page.click("#advanced-back-button", force=True)) is a leftover from the advanced settings section and doesn't relate to gameplay navigation. Consider removing it to avoid confusion.Proposed fix
# Go to Gameplay Settings page.wait_for_selector('#gameplay-button', state='visible', timeout=2500) - # page.click("#advanced-back-button", force=True) page.evaluate("window.gameplayPressed([0])")scripts/options_menu.gd (1)
330-349: Extract the repeated "hide all overlay buttons" JS snippet.
_on_gameplay_settings_button_pressedis the fifth function containing a near-identical JSevalblock that hides every overlay button. The same block appears in_on_options_back_button_pressed,_on_audio_settings_button_pressed,_on_key_mapping_button_pressed,_on_advanced_settings_button_pressed, and now here. Every time a new button is added to the options menu, all five (now six) call sites must be updated in lockstep—this is error-prone.Consider extracting a helper, e.g.:
♻️ Sketch of a shared helper
## Hides all options-menu web overlay buttons. func _hide_options_web_overlays() -> void: if os_wrapper.has_feature("web") and js_bridge_wrapper: js_bridge_wrapper.eval( """ document.getElementById('audio-button').style.display = 'none'; document.getElementById('controls-button').style.display = 'none'; document.getElementById('advanced-button').style.display = 'none'; document.getElementById('gameplay-button').style.display = 'none'; document.getElementById('options-back-button').style.display = 'none'; """, true )Then each handler simply calls
_hide_options_web_overlays()instead of inlining the block.scenes/options_menu.tscn (1)
153-155: No[connection]entry forGameplaySettingsButton.The other submenu buttons (
AdvancedSettingsButton,AudioSettingsButton,KeyMappingButton) declare[connection signal="pressed" ...]entries in the scene file, butGameplaySettingsButtondoes not. It still works because_ready()in the script connects it programmatically, but adding a scene connection here (and optionally dropping the redundant programmatic connections for the others) would make the wiring consistent.Add scene connection for consistency
[connection signal="pressed" from="Panel/OptionsVBoxContainer/KeyMappingButton" to="." method="_on_key_mapping_button_pressed"] +[connection signal="pressed" from="Panel/OptionsVBoxContainer/GameplaySettingsButton" to="." method="_on_gameplay_settings_button_pressed"]
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/codeql.yml.github/workflows/snyk.yml.github/workflows/trivy.ymlcustom_shell.htmlscenes/advanced_settings.tscnscenes/gameplay_settings.tscnscenes/options_menu.tscnscripts/advanced_settings.gdscripts/audio_settings.gdscripts/gameplay_settings.gdscripts/options_menu.gdtests/difficulty_flow_test.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/difficulty_flow_test.py (3)
tests/conftest.py (1)
page(15-50)tests/refactor/fuel_depletion_test.py (1)
page(80-102)tests/refactor/difficulty_integration_test.py (1)
page(96-117)
🪛 GitHub Actions: Pull Request Pipeline
scripts/options_menu.gd
[error] 107-107: gdformat formatting check failed. 1 file would be reformatted. Run 'gdformat --write' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (10)
scripts/advanced_settings.gd (1)
130-130: LGTM — consistent replacement ofdifficulty-sliderwithgameplay-button.Both exit paths (unexpected tree exit and intentional back press) now correctly reveal the new
gameplay-buttonoverlay when returning to the Options menu, aligning with the new UI flow.Also applies to: 197-197
scripts/audio_settings.gd (1)
710-710: LGTM — consistent with the same overlay change in other settings scripts.scenes/advanced_settings.tscn (1)
133-146: LGTM — layout centering and minimum size improvements.scripts/options_menu.gd (3)
27-27: New gameplay settings declarations and wiring look correct.The preload, callback holder,
@onreadybinding, and signal connection follow the same pattern used by the other submenu buttons. No issues here.Also applies to: 32-32, 37-37, 65-66
186-196: JS callback for gameplay settings — looks good.Follows the identical delegation pattern used by
_on_audio_pressed_jsand_on_advanced_pressed_js.
199-209:_on_controls_pressed_js— looks fine.This appears to have been moved/reformatted but is functionally unchanged; it correctly delegates to
_on_key_mapping_button_pressed.scenes/options_menu.tscn (2)
120-132: GameplaySettingsButton node definition looks good.Styling (colors, font, corner radii, focus/hover/pressed states) is consistent with the sibling buttons (AdvancedSettingsButton, AudioSettingsButton, KeyMappingButton, OptionsBackButton). The
{GAMEPLAY}text placeholder follows the same localisation pattern.
45-48: Panel and VBoxContainer offsets adjusted — looks reasonable.The panel grew slightly to accommodate the new button. The numbers are symmetric and consistent.
Also applies to: 61-64
.github/workflows/codeql.yml (1)
60-60: Consistent patch-level bump for CodeQL actions — LGTM.All three CodeQL action references (init, autobuild, analyze) are updated to v4.32.2 in lockstep. No functional changes.
Also applies to: 72-72, 75-75
.github/workflows/snyk.yml (1)
46-46: SHA-pinned upload-sarif references updated consistently — LGTM.Both SARIF upload steps use the same commit hash as
trivy.yml, keeping all workflows aligned.Also applies to: 57-57
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…hen the inline block. _on_gameplay_back_button_pressed calls _teardown() (line 105) which pops a menu from Globals.hidden_menus and makes it visible (lines 166-170). Then the same function immediately tries to pop again (lines 108-113). This double-pop corrupts the navigation stack and may hide or lose a menu. Either remove the _teardown() call and keep the inline logic (consistent with advanced_settings.gd and audio_settings.gd), or remove the inline pop and rely solely on _teardown().
Line 77 nullifies changeDifficultyLevel, but the callback was registered as changeDifficulty (line 59). The actual callback is never unset. Line 79 nullifies gameplayResetPressed, but no such callback was ever registered via create_callback — _on_gameplay_reset_js exists but is never wired to js_window.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…onfusing. Having both this and the existing advanced button labeled "ADVANCED" makes the overlay ambiguous. Please rename this one (e.g., to "GAMEPLAY") so the action is clearly distinct.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@scripts/gameplay_settings.gd`:
- Around line 82-84: The doc comment above the _on_gameplay_reset_button_pressed
function incorrectly references "Advanced Settings"; update the comment text to
correctly say "Gameplay Settings" (e.g., change "Handles Advanced Settings reset
button press." to "Handles Gameplay Settings reset button press.") so the
comment matches the function's purpose.
- Around line 66-68: The log message in Globals.log_message within
gameplay_settings.gd incorrectly says "options menu"; update the message string
used in that Globals.log_message call to read "Exposed gameplay settings
callbacks to JS for web overlays." (keep the same Globals.LogLevel.DEBUG and
location so only the message text changes).
🧹 Nitpick comments (1)
scripts/gameplay_settings.gd (1)
183-184: Redundant slider re-assignment inside its ownvalue_changedhandler.Line 183 sets
Globals.difficulty = value, then line 184 setsdifficulty_slider.value = Globals.difficulty. Since this handler fires because the slider already has this value, the write-back is a no-op. It could be useful ifGlobals.difficultyhad a clamping setter, but if that's not the case, consider removing line 184 to avoid confusion.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/gameplay_settings.gd
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Replace checks for 'difficulty-slider' with 'gameplay-button' in navigation and back-flow tests so the options overlay assertion targets the correct element. In reset_audio_flow_test, replace the flaky click on #options-button with a direct JS call (window.optionsPressed([])) and leave the original click commented out to improve test reliability.
Contributor
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)
tests/reset_audio_flow_test.py (1)
259-285:⚠️ Potential issue | 🟡 MinorSTATE-02 still reads
difficulty-sliderDOM value — may not reflect actual game state in the new architecture.With difficulty controls moved to the
gameplay_settingsscene, the DOMdifficulty-sliderelement is only synced when that scene is active. This test reads the raw DOM value (which defaults to1.0in HTML) without ever loading the gameplay settings scene, so it effectively asserts the HTML default hasn't been touched — not that the actual game difficulty is unaffected by audio reset.Consider whether this test should navigate into gameplay settings (via
window.gameplayPressed) to read the live difficulty state, or whether asserting the DOM default is sufficient for the isolation check.
🤖 Fix all issues with AI agents
In `@scripts/gameplay_settings.gd`:
- Around line 26-77: The "Gameplay Settings menu loaded." debug log is currently
inside the os_wrapper.has_feature("web") branch in _ready(), so it only runs on
web; move the Globals.log_message("Gameplay Settings menu loaded.",
Globals.LogLevel.DEBUG) call out of that if block (place it after the
web-specific block finishes) so it runs unconditionally on all platforms while
keeping the web-specific JS setup (js_bridge_wrapper, js_window, create_callback
assignments) intact.
🧹 Nitpick comments (4)
custom_shell.html (1)
228-229: Copy-paste comments: "advanced back" / "reset" should say "gameplay".The inline comments still reference "advanced" instead of "gameplay".
Proposed fix
- document.getElementById('gameplay-back-button').onclick = () => window.gameplayBackPressed([]); // Add this for advanced back - document.getElementById('gameplay-reset-button').onclick = () => window.gameplayResetPressed([]); // Add for reset + document.getElementById('gameplay-back-button').onclick = () => window.gameplayBackPressed([]); // Add this for gameplay back + document.getElementById('gameplay-reset-button').onclick = () => window.gameplayResetPressed([]); // Add for gameplay resetscripts/gameplay_settings.gd (2)
166-177:_teardown()is now dead code — never called.After removing the
_teardown()call from_on_gameplay_back_button_pressed(per past review), this method is no longer invoked anywhere. Consider removing it or connecting it to a lifecycle signal (e.g.,tree_exited) if cleanup on unexpected removal is intended.
91-96: Reset triggers double-save when slider value actually changes.Setting
difficulty_slider.valueon line 94 firesvalue_changed, which calls_on_difficulty_value_changed→Globals._save_settings(). Then line 96 callsGlobals._save_settings()again. The label update on line 95 is also redundant since the signal handler already sets it.Consider disconnecting the signal before resetting, or removing the explicit save/label update here:
Option: remove redundant calls
func _on_gameplay_reset_button_pressed() -> void: Globals.log_message("Gameplay Settings reset pressed.", Globals.LogLevel.DEBUG) - # Set initial default label difficulty_slider.value = _default_difficulty - difficulty_label.text = "{" + str(_default_difficulty) + "}" - Globals._save_settings()scripts/options_menu.gd (1)
3-14: Stale docstring references removeddifficulty_slideranddifficulty_label.The file header still documents
:vartype difficulty_sliderand:vartype difficulty_label(lines 13–14) and mentions "difficulty adjustment" (line 5), but these members no longer exist in this script.Proposed fix
## Options Menu Script ## -## Handles log level selection, difficulty adjustment, and back navigation +## Handles sub-menu navigation (audio, controls, advanced, gameplay settings) ## in the options menu. ## ## Supports web overlays for UI interactions and ignores pause mode. ## ## :vartype log_level_display_to_enum: Dictionary ## :vartype log_lvl_option: OptionButton ## :vartype back_button: Button -## :vartype difficulty_slider: HSlider -## :vartype difficulty_label: Label +## :vartype gameplay_settings_button: Button
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
custom_shell.htmlscenes/gameplay_settings.tscnscripts/gameplay_settings.gdscripts/options_menu.gdtests/back_flow_test.pytests/navigation_to_audio_test.pytests/reset_audio_flow_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- scenes/gameplay_settings.tscn
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (6)
tests/back_flow_test.py (1)
121-122: LGTM — consistent with the DOM refactor.The visibility check now targets
gameplay-button, which aligns with the broader change replacingdifficulty-sliderwithgameplay-buttonin the options overlay.tests/navigation_to_audio_test.py (1)
143-146: LGTM — mirrors the same overlay check update as the other test files.custom_shell.html (1)
163-164: New gameplay buttons and wiring look correct.The three new elements (
gameplay-button,gameplay-back-button,gameplay-reset-button) are properly added with consistent styling and correctly wired to their respectivewindow.*callbacks.Also applies to: 207-207
tests/reset_audio_flow_test.py (1)
233-234: LGTM — switching from DOM click to JS bridge call for reliability.Consistent with the approach used across other test files in this PR.
scripts/options_menu.gd (2)
27-37: LGTM — gameplay settings wiring follows existing patterns.The new
gameplay_settings_scenepreload, button reference, signal connection, and JS callback exposure all follow the same patterns established for audio, controls, and advanced settings.Also applies to: 65-66, 111-114
330-349: New gameplay settings button handler is clean and consistent.Follows the same instantiate → add_child → push hidden menu → hide self → hide overlays pattern as the other sub-menu handlers.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Line 77 is inside the if os_wrapper.has_feature("web"): block, so this log message is never emitted on desktop. If this is intentional, no action needed; otherwise, move it outside the web check.
🤖 Prompt for AI Agents
In tests/reset_audio_flow_test.py, add a wait_for_function to ensure window.optionsPressed is defined (5s timeout) before calling it. This prevents a race condition where the test could invoke the function before it has been attached.
Owner
Author
All addressed. |
…ersist. To prevent garbage collection of JavaScriptObject callbacks in Godot's JS bindings, which can break the references and cause calls like window.optionsPressed([]) to fail silently, leading to issues like the test timeout on waiting for visible elements (e.g., #audio-button remains hidden because options menu _ready() doesn't run). Storing them as member variables ensures they persist.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/reset_audio_flow_test.py (1)
260-286:⚠️ Potential issue | 🟡 MinorAccessing difficulty-slider while hidden is fragile; navigate through gameplay settings for better UI consistency.
The test reads
difficulty-slider.valueat lines 265 and 285 after navigating from audio back to options. With the new flow where difficulty controls moved to a separate Gameplay Settings scene, the slider DOM element hasdisplay: noneby default (custom_shell.html line 41-43) and only getsdisplay: 'block'when the scene loads (gameplay_settings.gd line 47).The test currently works because the hidden element exists and its value is readable, but this is fragile—if the HTML shell ever removes/recreates the element instead of hiding it, this test will fail. Following the new UI flow by navigating through the Gameplay Settings submenu before reading the difficulty value would be more robust and consistent with how the interface actually works.
🧹 Nitpick comments (2)
scripts/gameplay_settings.gd (2)
166-177:_teardown()appears to be dead code — it's never called.No method in this file invokes
_teardown(), and it duplicates the pop-from-stack logic already in_on_gameplay_back_button_pressed(). If it's intended as a safety net for unexpected removal (e.g., viatree_exited), it should be wired up; otherwise, consider removing it to avoid confusion.#!/bin/bash # Verify _teardown is not called from anywhere in the codebase rg -n '_teardown' --type-add 'gd:*.gd' --type gd
188-193: Potential re-entrant signal emission on line 190.
_on_difficulty_value_changedsetsdifficulty_slider.value = Globals.difficulty(line 190) after already assigningGlobals.difficulty = value(line 189). If the slider emitsvalue_changedon programmatic updates (Godot'sHSliderdoes emit when the value actually changes), this could re-enter the handler. Sincevalueis the same, the slider shouldn't re-emit, but it's safer to either remove the redundant assignment or temporarily disconnect the signal.Option: remove the redundant slider assignment
func _on_difficulty_value_changed(value: float) -> void: Globals.difficulty = value - difficulty_slider.value = Globals.difficulty difficulty_label.text = "{" + str(value) + "}"The slider already has the correct value since it triggered this signal. The only caller that bypasses the signal is
_on_change_difficulty_js, which calls this function directly — but the JS callback should also update the slider. If you need the slider updated from JS calls, set it before connecting the signal or guard against re-entry.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
scripts/gameplay_settings.gdscripts/main_menu.gdtests/load_main_menu_test.pytests/reset_audio_flow_test.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (6)
tests/load_main_menu_test.py (1)
73-73: LGTM — increased init wait is reasonable.The bump from 3000ms to 5000ms gives more headroom for Godot engine initialization in CI environments.
tests/reset_audio_flow_test.py (1)
233-235: LGTM — JS bridge invocation after reload is more robust than DOM click.Waiting for
window.optionsPressedto be defined before invoking it avoids race conditions where the Godot scene hasn't yet registered callbacks after a page reload.scripts/main_menu.gd (1)
32-34: Good fix — persisting JS callbacks as member variables prevents GC.Storing
JavaScriptObjectcallbacks in member fields ensures the references survive across frames, avoiding silent failures when Godot's JS bridge garbage-collects orphaned callbacks.Also applies to: 76-93
scripts/gameplay_settings.gd (3)
1-14: LGTM — class-level fields properly declared for callback persistence and state tracking.The
js_windowfield (line 9) is now assigned directly in_ready()(line 55) instead of being shadowed by a local variable. The three callback members (_change_difficulty_cb,_gameplay_back_button_pressed_cb,_gameplay_reset_cb) prevent GC of JS bridge callbacks.
54-75: Previous critical issues are resolved: callback registration and naming are now consistent.
js_windowassigned to class field (novarkeyword) ✓changeDifficultyregistered (line 60) matches cleanup (line 84) ✓gameplayResetPressedis now properly registered (line 70) ✓
103-147: Previous double-pop issue is resolved.
_on_gameplay_back_button_pressedno longer calls_teardown(), so the hidden menu stack is only popped once. The inline pop+show+cleanup+queue_free flow is self-contained and correct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
The inline comment claims "v3.31.0 (latest stable as of 2025-10-28)" but this SHA does not correspond to any official v3.31.0 release tag. Additionally, the date is outdated—v4.32.2 (used in codeql.yml) was released on February 5, 2026, and the codeql-action repository has moved beyond the v3.x line. The SHA b13d724d35ff0a814e21683638ed68ed34cf53d1 appears to be an untagged commit. Either update this SHA to match the v4.32.2 version used in codeql.yml for consistency, or correct the comment to reflect the actual version this commit corresponds to along with an accurate date.
…e the menu stack. Both advanced_settings.gd and audio_settings.gd connect tree_exited to handle unexpected removal (e.g., scene tree changes), restoring the previous menu and cleaning up JS callbacks. This script omits that handler, risking a stuck/lost menu on unexpected exit.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/gameplay_settings.gd`:
- Around line 230-241: Delete the unused _teardown() method from
scripts/gameplay_settings.gd since it no longer has any callers (it was removed
from _on_gameplay_back_button_pressed) — remove the entire function definition
including its doc comments and internal logic (references to
Globals.hidden_menus, prev_menu handling, and logging) so the dead code is
eliminated.
🧹 Nitpick comments (2)
scripts/gameplay_settings.gd (2)
153-161: Double_save_settings()call on reset.Setting
difficulty_slider.valueon line 158 firesvalue_changed, which calls_on_difficulty_value_changed→Globals._save_settings()(line 257). Line 160 then calls_save_settings()again redundantly.♻️ Remove the redundant save
func _on_gameplay_reset_button_pressed() -> void: Globals.log_message("Gameplay Settings reset pressed.", Globals.LogLevel.DEBUG) # Set initial default label difficulty_slider.value = _default_difficulty difficulty_label.text = "{" + str(_default_difficulty) + "}" - Globals._save_settings()
244-257: Minor: redundant slider reassignment.Line 254 sets
difficulty_slider.value = Globals.difficulty, but this handler is triggered by the slider'svalue_changedsignal — the slider already holds this value. No harm (Godot won't re-fire for the same value), but it's unnecessary.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/trivy.ymlscripts/gameplay_settings.gd
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/trivy.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (4)
scripts/gameplay_settings.gd (4)
1-24: LGTM!Class-level declarations, wrappers, and
@onreadynode references are cleanly structured. The previousjs_windowshadowing issue has been properly addressed.
26-79: LGTM — previous issues properly addressed.
js_windowis now correctly assigned to the class field (line 57),tree_exitedis connected (line 41), and the "menu loaded" log (line 79) is outside the web-only block. Callback names are consistent with cleanup.
82-141: LGTM!Thorough cleanup handler: disconnects signals defensively, nulls callback refs, restores previous menu on unexpected exit, and correctly gates web overlay logic.
167-211: LGTM!The previous double-pop issue is resolved. Stack restoration, web overlay toggling, and intentional exit marking are all handled correctly before
queue_free().
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
After removing the _teardown() call from _on_gameplay_back_button_pressed, this method is now unreachable dead code with no references in the codebase. Delete it to clean up the code.
args[0][0] is accessed without checking args[0] type or length. Other scripts (e.g., _on_change_log_level_js in advanced_settings.gd) perform thorough validation. A malformed call from JS could crash here.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- In
gameplay_settings.gd,_on_gameplay_reset_button_pressedonly updates the slider and label and calls_save_settings()but never updatesGlobals.difficulty, so the reset won’t actually change the persisted difficulty value; consider delegating to_on_difficulty_value_changed(_default_difficulty)instead of duplicating the logic. - The difficulty reset logic in
gameplay_settings.gdis slightly divergent between UI and web overlays (DOM slider value is never updated on reset); if web overlays are meant to stay in sync, consider also updating thedifficulty-sliderelement viajs_bridge_wrapper.evalwhen the reset button is used.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `gameplay_settings.gd`, `_on_gameplay_reset_button_pressed` only updates the slider and label and calls `_save_settings()` but never updates `Globals.difficulty`, so the reset won’t actually change the persisted difficulty value; consider delegating to `_on_difficulty_value_changed(_default_difficulty)` instead of duplicating the logic.
- The difficulty reset logic in `gameplay_settings.gd` is slightly divergent between UI and web overlays (DOM slider value is never updated on reset); if web overlays are meant to stay in sync, consider also updating the `difficulty-slider` element via `js_bridge_wrapper.eval` when the reset button is used.
## Individual Comments
### Comment 1
<location> `scripts/gameplay_settings.gd:155-160` </location>
<code_context>
+
+## RESET BUTTON
+## Handles Gameplay Settings reset button press.
+func _on_gameplay_reset_button_pressed() -> void:
+ Globals.log_message("Gameplay Settings reset pressed.", Globals.LogLevel.DEBUG)
+ # Set initial default label
+ difficulty_slider.value = _default_difficulty
+ difficulty_label.text = "{" + str(_default_difficulty) + "}"
+ Globals._save_settings()
+
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Reset handler does not update `Globals.difficulty`, so saved settings and in‑memory state can diverge.
This path only updates the slider and label, then calls `Globals._save_settings()` without changing `Globals.difficulty`, so the saved value can stay stale while the UI shows the default. Mirror the behavior of `_on_difficulty_value_changed` by updating the global and then saving, e.g.:
```gdscript
Globals.difficulty = _default_difficulty
_on_difficulty_value_changed(_default_difficulty)
```
(or equivalent) so in-memory and persisted difficulty stay in sync after reset.
</issue_to_address>
### Comment 2
<location> `scripts/gameplay_settings.gd:156-160` </location>
<code_context>
+## RESET BUTTON
+## Handles Gameplay Settings reset button press.
+func _on_gameplay_reset_button_pressed() -> void:
+ Globals.log_message("Gameplay Settings reset pressed.", Globals.LogLevel.DEBUG)
+ # Set initial default label
+ difficulty_slider.value = _default_difficulty
+ difficulty_label.text = "{" + str(_default_difficulty) + "}"
+ Globals._save_settings()
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Gameplay reset does not sync the HTML `difficulty-slider` value, which can confuse web users.
On web builds, only the Godot `difficulty_slider` and label are reset; the HTML `#difficulty-slider` stays at its old value. This desync can mislead users who use the HTML overlay.
To keep them in sync, also update the DOM when `has_feature("web")` is true (e.g. via `js_bridge_wrapper.eval` to set `document.getElementById('difficulty-slider').value` to the reset value, and optionally trigger the same JS callback used by `onchange`).
Suggested implementation:
```
## RESET BUTTON
## Handles Gameplay Settings reset button press.
func _on_gameplay_reset_button_pressed() -> void:
Globals.log_message("Gameplay Settings reset pressed.", Globals.LogLevel.DEBUG)
# Reset internal difficulty slider and label to defaults.
difficulty_slider.value = _default_difficulty
difficulty_label.text = "{" + str(_default_difficulty) + "}"
# Keep web overlay (HTML difficulty slider) in sync when running on web.
if os_wrapper.has_feature("web"):
var js_code := """
(() => {
const el = document.getElementById('difficulty-slider');
if (!el) { return; }
el.value = %s;
// Prefer the same callback used by `onchange` to keep behavior consistent.
if (typeof el.onchange === 'function') {
el.onchange();
} else {
el.dispatchEvent(new Event('change'));
}
})();
""" % str(_default_difficulty)
js_bridge_wrapper.eval(js_code.strip_edges())
# Persist the updated settings.
Globals._save_settings()
```
These edits assume:
1. `JavaScriptBridgeWrapper` exposes an `eval(code: String)` method that forwards to `JavaScriptBridge.eval`.
2. `OSWrapper.has_feature("web")` mirrors `OS.has_feature("web")`.
3. `difficulty_slider`, `difficulty_label`, and `_default_difficulty` are already defined members in this script, and `Globals._save_settings()` is the correct way to persist the difficulty.
If the JS side expects a different element ID or uses a specific change handler (e.g. a global function instead of `onchange`), update the JS snippet accordingly to call that function instead of relying on `el.onchange()` / `change` events.
</issue_to_address>
### Comment 3
<location> `tests/difficulty_flow_test.py:130-133` </location>
<code_context>
# page.click("#advanced-back-button", force=True)
page.evaluate("window.advancedBackPressed([0])")
+ # Go to Gameplay Settings
+ page.wait_for_selector('#gameplay-button', state='visible', timeout=2500)
+ # page.click("#advanced-back-button", force=True)
+ page.evaluate("window.gameplayPressed([0])")
+
# Set difficulty to 2.0 - directly call the exposed callback (bypasses event for reliability in automation)
</code_context>
<issue_to_address>
**suggestion (testing):** Assert that the gameplay settings overlay actually becomes visible (and options is hidden) after navigating to it.
The test currently calls `gameplayPressed` but never verifies that the gameplay settings UI is shown or that the options overlay is hidden. Please add assertions against gameplay-specific elements (e.g. `#difficulty-slider`) and ensure the options overlay is not visible, so the test validates the overlay wiring rather than just the callback invocation.
Suggested implementation:
```python
# Go to Gameplay Settings
page.wait_for_selector('#gameplay-button', state='visible', timeout=2500)
# page.click("#advanced-back-button", force=True)
page.evaluate("window.gameplayPressed([0])")
# Assert gameplay settings overlay is shown and options overlay is hidden
page.wait_for_selector('#difficulty-slider', state='visible', timeout=2500)
page.wait_for_selector('#options-overlay', state='hidden', timeout=2500)
# Set difficulty to 2.0 - directly call the exposed callback (bypasses event for reliability in automation)
pre_change_log_count = len(logs)
page.wait_for_function('window.changeDifficulty !== undefined', timeout=2500)
```
The selectors `#difficulty-slider` and `#options-overlay` are based on the intent of the test but may not match your actual DOM structure. You should:
1. Replace `#difficulty-slider` with a selector that uniquely identifies a gameplay-specific control in the gameplay settings overlay (e.g. the difficulty slider element).
2. Replace `#options-overlay` with the selector for the options/settings overlay container that should no longer be visible once gameplay settings are opened.
3. If your project uses a custom visibility pattern (e.g. `aria-hidden`, CSS classes), you may want to assert on those attributes instead of `state='hidden'`.
</issue_to_address>
### Comment 4
<location> `tests/difficulty_flow_test.py:154-158` </location>
<code_context>
new_logs = logs[pre_change_log_count:]
assert any("back button pressed." in log["text"].lower() for log in new_logs), "Back button not found"
+ # Back to Main menu
+ # Check element present
+ page.wait_for_selector('#options-back-button', state='visible', timeout=2500)
+ assert page.evaluate("document.getElementById('options-back-button') !== null")
+ page.evaluate("window.optionsBackPressed([0])")
+
# Start game
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen assertions around the new gameplay back flow and options back flow.
The current test only checks for a generic "back button pressed" log after `gameplayBackPressed`, then calls `optionsBackPressed` without asserting its UI effects.
Please also assert the navigation state:
- After `gameplayBackPressed([])`, verify that the options overlay is visible again and gameplay-specific elements are hidden.
- After `optionsBackPressed([0])`, verify that main-menu elements (e.g. `#start-button`) are visible (and optionally that options elements are hidden) before starting the game.
This will validate the side effects of both callbacks, not just that they are invoked.
Suggested implementation:
```python
# Back to main menu
pre_change_log_count = len(logs)
page.wait_for_function('window.gameplayBackPressed !== undefined', timeout=2500)
page.evaluate("window.gameplayBackPressed([])")
page.wait_for_timeout(2500)
new_logs = logs[pre_change_log_count:]
assert any("back button pressed." in log["text"].lower() for log in new_logs), "Back button not found"
# After gameplayBackPressed([]), the options overlay should be visible again
# and gameplay-specific elements should be hidden.
# Options overlay visible
page.wait_for_selector('#options-overlay', state='visible', timeout=2500)
assert page.evaluate("document.getElementById('options-overlay') !== null")
# Gameplay UI hidden
page.wait_for_selector('#gameplay-container', state='hidden', timeout=2500)
assert page.evaluate("document.getElementById('gameplay-container') === null || document.getElementById('gameplay-container').offsetParent === null")
# Back to Main menu
# Check element present
page.wait_for_selector('#options-back-button', state='visible', timeout=2500)
assert page.evaluate("document.getElementById('options-back-button') !== null")
page.evaluate("window.optionsBackPressed([0])")
# After optionsBackPressed([0]), we should be back on the main menu:
# main-menu elements visible and options elements hidden.
page.wait_for_selector('#start-button', state='visible', timeout=2500)
assert page.evaluate("document.getElementById('start-button') !== null")
page.wait_for_selector('#options-overlay', state='hidden', timeout=2500)
assert page.evaluate("document.getElementById('options-overlay') === null || document.getElementById('options-overlay').offsetParent === null")
# Start game
pre_change_log_count = len(logs)
```
The above changes assume the following DOM structure:
- Options overlay root: `#options-overlay`
- Gameplay container: `#gameplay-container`
If your actual IDs differ, you should:
1. Replace `#options-overlay` with the real selector for the options overlay element.
2. Replace `#gameplay-container` with the real selector for the gameplay UI container.
3. If the elements are not removed from the DOM when hidden, ensure that `offsetParent === null` (or an equivalent visibility check) matches your app’s hiding behavior (e.g., `display: none` / `visibility: hidden`).
</issue_to_address>
### Comment 5
<location> `tests/difficulty_flow_test.py:135` </location>
<code_context>
+ # page.click("#advanced-back-button", force=True)
+ page.evaluate("window.gameplayPressed([0])")
+
# Set difficulty to 2.0 - directly call the exposed callback (bypasses event for reliability in automation)
pre_change_log_count = len(logs)
page.wait_for_function('window.changeDifficulty !== undefined', timeout=2500)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a dedicated test for the gameplay settings "reset" behavior in addition to the direct difficulty change.
The current test only exercises the difficulty change path via `changeDifficulty([2.0])`, but not the gameplay reset action exposed from this screen.
Please either extend this test or add a new one that:
1. Changes difficulty away from the default.
2. Triggers the gameplay reset action.
3. Asserts that difficulty returns to the expected default (using the same kind of verification as existing checks).
That will give end-to-end coverage for both changing and resetting difficulty through the new gameplay settings menu.
Suggested implementation:
```python
# Set difficulty to 2.0 - directly call the exposed callback (bypasses event for reliability in automation)
pre_change_log_count = len(logs)
page.wait_for_function('window.changeDifficulty !== undefined', timeout=2500)
page.evaluate("window.changeDifficulty([2.0])")
page.wait_for_timeout(2500)
difficulty_logs = logs[pre_change_log_count:]
# Verify that difficulty was actually changed away from the default
assert any(
"difficulty" in log["text"].lower() and "2.0" in log["text"]
for log in difficulty_logs
), "Difficulty change to 2.0 was not observed in logs"
# Reset gameplay settings back to defaults via the gameplay reset action
pre_reset_log_count = len(logs)
page.wait_for_function('window.resetGameplaySettings !== undefined', timeout=2500)
page.evaluate("window.resetGameplaySettings([])")
page.wait_for_timeout(2500)
reset_logs = logs[pre_reset_log_count:]
# Verify that difficulty was reset to the expected default
assert any(
"difficulty" in log["text"].lower()
and ("default" in log["text"].lower() or "1.0" in log["text"])
for log in reset_logs
), "Difficulty reset to default was not observed in logs"
# Back to main menu
pre_change_log_count = len(logs)
page.wait_for_function('window.gameplayBackPressed !== undefined', timeout=2500)
page.evaluate("window.gameplayBackPressed([])")
page.wait_for_timeout(2500)
new_logs = logs[pre_change_log_count:]
assert any("back button pressed." in log["text"].lower() for log in new_logs), "Back button not found"
```
You’ll need to align a few details with the actual implementation:
1. Replace `window.resetGameplaySettings` with the actual callback name exposed by the gameplay settings menu for the “reset” action (for example, it might be something like `window.gameplayResetPressed` or similar).
2. Adjust the log assertions:
- Ensure the difficulty-change log really includes `"2.0"` (or change this to match whatever string your logging uses).
- Ensure the reset log assertion matches your actual default difficulty value (if it’s not `1.0`) and/or whatever “reset to default” wording your logs emit.
3. If the test harness already changes difficulty elsewhere in this test, de-duplicate the `changeDifficulty` call and corresponding assertion so you only change difficulty once before resetting.
</issue_to_address>
### Comment 6
<location> `tests/navigation_to_audio_test.py:144` </location>
<code_context>
# Assert main/options overlays are restored
options_overlay_display: str = page.evaluate(
- "window.getComputedStyle(document.getElementById('difficulty-slider')).display"
+ "window.getComputedStyle(document.getElementById('gameplay-button')).display"
)
assert options_overlay_display == 'block', "Options overlay not restored after exiting audio menu"
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a dedicated assertion that the gameplay button is hidden while in the audio menu.
The new assertion confirming `#gameplay-button` is visible after exiting the audio menu is helpful. To better cover the overlay behavior, also assert that the gameplay button (or other options UI) is hidden immediately after entering the audio menu, e.g., by checking its display is `'none'`. This would ensure we catch regressions where the options UI remains visible under the audio overlay.
Suggested implementation:
```python
# Assert main/options overlays are restored
options_overlay_display: str = page.evaluate(
"window.getComputedStyle(document.getElementById('gameplay-button')).display"
)
assert options_overlay_display == 'block', "Options overlay not restored after exiting audio menu"
```
To implement the requested behavior, add an assertion immediately after navigating into the audio menu (i.e., right after the action that opens the audio menu, and before any action that closes it). The added code should look like this:
```python
# Assert gameplay/options UI is hidden while audio menu is open
gameplay_button_display_in_audio: str = page.evaluate(
"window.getComputedStyle(document.getElementById('gameplay-button')).display"
)
assert gameplay_button_display_in_audio == 'none', "Gameplay button should be hidden while audio menu is open"
```
Place this in the same test function that contains the existing "Assert main/options overlays are restored" block, at the point in the flow where the audio menu is open and before you exit it. This will ensure the test verifies both:
1. The gameplay/options UI is hidden while the audio overlay is active.
2. The gameplay/options UI is restored (`display == 'block'`) after exiting the audio menu (already covered by the existing assertion).
</issue_to_address>
### Comment 7
<location> `tests/load_main_menu_test.py:71-74` </location>
<code_context>
page.goto("http://localhost:8080/index.html", wait_until="networkidle", timeout=5000)
# Wait for Godot engine init (ensures 'godot' object is defined)
- page.wait_for_timeout(3000)
+ page.wait_for_timeout(5000)
page.wait_for_function("() => window.godotInitialized", timeout=5000)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider replacing the fixed timeout with a condition-driven wait to keep the test fast and deterministic.
Bumping the timeout to 5000ms may hide flakiness while slowing the test and still not guaranteeing readiness. Since you already use `wait_for_function("() => window.godotInitialized", ...)`, you can likely remove this fixed timeout (or greatly reduce it) and rely on that condition as the main sync point.
```suggestion
page.goto("http://localhost:8080/index.html", wait_until="networkidle", timeout=5000)
# Wait for Godot engine init (ensures 'godot' object is defined)
page.wait_for_function("() => window.godotInitialized", timeout=5000)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…o saved settings and in‑memory state can diverge. This path only updates the slider and label, then calls Globals._save_settings() without changing Globals.difficulty, so the saved value can stay stale while the UI shows the default. Mirror the behavior of _on_difficulty_value_changed by updating the global and then saving
…ly becomes visible (and options is hidden) after navigating to it. The test currently calls gameplayPressed but never verifies that the gameplay settings UI is shown or that the options overlay is hidden. Please add assertions against gameplay-specific elements (e.g. #difficulty-slider) and ensure the options overlay is not visible, so the test validates the overlay wiring rather than just the callback invocation. The selectors #difficulty-slider and #options-overlay are based on the intent of the test but may not match your actual DOM structure. You should: Replace #difficulty-slider with a selector that uniquely identifies a gameplay-specific control in the gameplay settings overlay (e.g. the difficulty slider element). Replace #options-overlay with the selector for the options/settings overlay container that should no longer be visible once gameplay settings are opened. If your project uses a custom visibility pattern (e.g. aria-hidden, CSS classes), you may want to assert on those attributes instead of state='hidden'.
…ack flow and options back flow. The current test only checks for a generic "back button pressed" log after gameplayBackPressed, then calls optionsBackPressed without asserting its UI effects. Please also assert the navigation state: After gameplayBackPressed([]), verify that the options overlay is visible again and gameplay-specific elements are hidden. After optionsBackPressed([0]), verify that main-menu elements (e.g. #start-button) are visible (and optionally that options elements are hidden) before starting the game. This will validate the side effects of both callbacks, not just that they are invoked.
…ay settings "reset" behavior in addition to the direct difficulty change. The current test only exercises the difficulty change path via changeDifficulty([2.0]), but not the gameplay reset action exposed from this screen. Please either extend this test or add a new one that: Changes difficulty away from the default. Triggers the gameplay reset action. Asserts that difficulty returns to the expected default (using the same kind of verification as existing checks). That will give end-to-end coverage for both changing and resetting difficulty through the new gameplay settings menu.
…gameplay button is hidden while in the audio menu. The new assertion confirming #gameplay-button is visible after exiting the audio menu is helpful. To better cover the overlay behavior, also assert that the gameplay button (or other options UI) is hidden immediately after entering the audio menu, e.g., by checking its display is 'none'. This would ensure we catch regressions where the options UI remains visible under the audio overlay.
…dition-driven wait to keep the test fast and deterministic.
Bumping the timeout to 5000ms may hide flakiness while slowing the test and still not guaranteeing readiness. Since you already use wait_for_function("() => window.godotInitialized", ...), you can likely remove this fixed timeout (or greatly reduce it) and rely on that condition as the main sync point.
Align tests with updated global handler names and signatures: replace window.resetGameplaySettings with window.gameplayResetPressed in difficulty_flow_test, and call window.audioPressed with an empty array ([]) instead of [0] in reset_audio_flow_test. These changes keep the tests in sync with the app's renamed handlers and revised argument expectations.
Replaced non-existent selectors '#options-overlay' and '#gameplay-container' with actual DOM elements like '#options-back-button' and '#difficulty-slider' for visibility/hidden checks after menu transitions. Added type hints for variables, parameters, and returns. Ensured assertions use offsetParent for hidden state verification. No other code changes.
github-project-automation
bot
moved this from In Progress
to Done
in Sky Lock Assault Project
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''
Description
What does this PR do? (e.g., "Fixes player jump physics in level 2" or "Adds
new enemy AI script")
Related Issue
Closes #ISSUE_NUMBER (if applicable)
Changes
system")
Testing
works on Win10 with 60 FPS")
Checklist
Additional Notes
Anything else? (e.g., "Tested on Win10 64-bit; needs Linux validation")
Summary by Sourcery
Introduce a dedicated gameplay settings menu for managing difficulty and update menus, web overlays, and tests to route difficulty changes through this new screen.
New Features:
Enhancements:
CI:
Deployment:
Tests:
Summary by Sourcery
Introduce a dedicated gameplay settings menu for difficulty management and integrate it into existing UI flows and web overlays while updating security analysis tooling versions.
New Features:
Enhancements:
CI:
Deployment:
Tests:
Summary by CodeRabbit
New Features
Style
Tests
Chores