Harden gameplay settings JS bridge and add comprehensive tests#488
Merged
Harden gameplay settings JS bridge and add comprehensive tests#488
Conversation
…tialization #485 Description: Implement the foundation of the gameplay settings test suite, focusing on the GameSettingsResource as the single source of truth and the initial menu setup. Why is this useful?: Ensures that difficulty clamping (0.5 to 2.0) and signal emissions work correctly before the UI layer is involved. Proposed Implementation: Create test_game_settings_resource.gd covering: GS-RES-01 to 07: Validate clamping, boundary values (0.5, 1.0, 2.0), and redundant emission stability. GS-READY-01 to 06: Confirm _ready() correctly syncs the slider and label to Globals.settings.difficulty and connects signals exactly once.
…vity #486 Description: [cite_start]Develop tests for user-driven interactions and the menu's behavior as an observer of external resource changes[cite: 8, 75]. Why is this useful?: [cite_start]Verifies that the UI stays in sync with the global state and that set_value_no_signal prevents infinite feedback loops between the slider and the resource[cite: 10, 11]. Proposed Implementation: Create test_gameplay_settings_ui.gd covering: GS-UI-01 to 06: User slider changes, Reset button functionality, and label updates[cite: 15, 20]. GS-OBS-01 to 05: Verify _on_external_setting_changed correctly updates the UI when the resource is modified by other scripts[cite: 10, 11].
Create test_gameplay_settings_js.gd covering: GS-JS-01 to 05: Validating nested array shapes like [[1.5]]. GS-JS-10 to 25: Critical Defensive Tests for empty arrays [], non-numeric strings [["abc"]], and scalar values [1.5] to ensure no calls to .size() or .is_empty() occur on primitive types. GS-JS-30 to 32: Missing node safety (e.g., if the slider is null during a JS callback).
…#471) This PR implements a comprehensive suite of unit tests and defensive programming patterns for the GameplaySettings module. The primary goal was to resolve Issue #471, which involved engine crashes during JavaScript-to-Godot communication, and to ensure the menu remains stable during complex lifecycle events.
ikostan
added this to the
Milestone 14: Resource-Driven Mechanics & Persistence Refactor milestone
Contributor
Reviewer's GuideStrengthens GameplaySettings’ resilience to invalid or late JS / lifecycle events by guarding node and Globals access, routing all difficulty updates through the settings resource (with clamping), relaxing js_window typing, and adds extensive GUT test suites for the settings resource, gameplay settings UI, JS bridge, and lifecycle/cleanup paths. Sequence diagram for the new JS difficulty update and clamping flowsequenceDiagram
actor JS as JavaScript
participant JSBridge as JavaScriptBridgeWrapper
participant GS as GameplaySettings
participant Globals
participant Settings as GameSettingsResource
participant Slider as DifficultySlider
participant Label as DifficultyLabel
JS->>JSBridge: invoke window.changeDifficulty(args)
JSBridge->>GS: _on_change_difficulty_js(args)
GS->>GS: validate args (empty, type, array/JavaScriptObject)
GS->>GS: extract potential_value
GS->>GS: ensure convertible (int/float/string)
GS->>GS: reject non-numeric strings
GS->>GS: value = float(potential_value)
GS->>Slider: check min_value/max_value
GS->>GS: log out-of-bounds but do not return
GS->>Globals: access Globals.settings
alt Globals.settings invalid
GS->>Globals: log warning (settings unavailable)
GS-->>JSBridge: return
else settings available
Globals-->>GS: settings_res
GS->>Settings: set difficulty = value
Settings-->>Settings: clamp difficulty in setter
Settings-->>Globals: emit setting_changed("difficulty", clamped_value)
Globals->>Globals: _on_setting_changed
Globals->>Globals: log and persist setting
Globals-->>Settings: done
GS->>Slider: set value = settings_res.difficulty
GS->>Label: set text = "{" + str(settings_res.difficulty) + "}"
GS-->>JSBridge: return
end
Updated class diagram for GameplaySettings, Globals, and GameSettingsResourceclassDiagram
class GameplaySettings {
<<Control>>
- JavaScriptBridgeWrapper js_bridge_wrapper
- OSWrapper os_wrapper
- Variant js_window
- JavaScriptObject _change_difficulty_cb
- JavaScriptObject _gameplay_back_button_pressed_cb
- JavaScriptObject _gameplay_reset_cb
- float _default_difficulty
- DifficultySlider difficulty_slider
- DifficultyLabel difficulty_label
- Button gameplay_back_button
- Button gameplay_reset_button
+ _ready() void
+ _on_external_setting_changed(setting_name String, new_value Variant) void
+ _on_tree_exited() void
+ _on_difficulty_value_changed(value float) void
+ _on_change_difficulty_js(args Array) void
- _unset_gameplay_settings_window_callbacks() void
}
class Globals {
<<Node>>
+ GameSettingsResource settings
- bool _is_loading_settings
+ _ready() void
+ _load_settings() void
+ _save_settings() void
+ _on_setting_changed(setting_name String, new_value Variant) void
+ log_message(message String, level int) void
}
class GameSettingsResource {
<<Resource>>
+ float difficulty
+ signal setting_changed(setting_name String, new_value Variant)
+ set_difficulty(value float) void
+ get_difficulty() float
}
class JavaScriptBridgeWrapper {
<<Helper>>
+ connect_callbacks() void
+ disconnect_callbacks() void
}
class OSWrapper {
<<Helper>>
+ has_feature(name String) bool
}
class DifficultySlider {
<<Control>>
+ float value
+ float min_value
+ float max_value
+ signal value_changed(value float)
+ set_value_no_signal(value float) void
}
class DifficultyLabel {
<<Label>>
+ String text
}
class Button {
<<Button>>
+ signal pressed()
}
GameplaySettings ..> Globals : accesses
Globals *-- GameSettingsResource : settings
GameplaySettings ..> GameSettingsResource : reads_writes_difficulty
GameplaySettings ..> JavaScriptBridgeWrapper : uses
GameplaySettings ..> OSWrapper : uses
GameplaySettings *-- DifficultySlider : owns
GameplaySettings *-- DifficultyLabel : owns
GameplaySettings *-- Button : owns back_button
GameplaySettings *-- Button : owns reset_button
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was
linked to
issues
Mar 18, 2026
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:
📝 WalkthroughWalkthroughHardened gameplay_settings.gd: relaxed Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScriptBridge
participant OS as OSWrapper
participant GS as GameSettingsResource
participant UI as GameplaySettings_UI
participant Node as gameplay_settings.gd
JS->>Node: _on_change_difficulty_js(args)
Node->>Node: _extract_js_difficulty(args) / validate numeric
alt valid numeric extracted
Node->>GS: set difficulty via cached settings_res (clamped)
GS-->>Node: emit setting_changed("difficulty", new_value)
Node->>UI: set_value_no_signal(new_value)
UI-->>Node: update difficulty_label
else invalid/malformed
Node->>JS: log warning / ignore (no crash)
end
Note over Node,GS: On tree exit / teardown
Node->>GS: disconnect setting_changed (if valid)
Node->>UI: disconnect slider signals (if valid)
Node->>JS: unset callbacks / call eval on JS bridge (if present)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
ikostan
added
bug
![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 1 issue, and left some high level feedback:
- In
_on_change_difficulty_js, the branch that treatsfirst_argas a container still calls.size()and indexes[0]on values that may beJavaScriptObjects; if the regression you’re fixing was around calling array APIs on scalars, consider restricting the container path tofirst_arg is Arrayand handlingJavaScriptObjectvia a safer accessor (e.g., known property) to avoid future engine-level crashes. - When the difficulty slider node is invalid in
_on_change_difficulty_js, you fall back to writingGlobals.settings.difficultywithout checking thatGlobals/Globals.settingsare valid; mirroring the guards you added in_on_tree_exitedwould make this callback more robust to teardown timing issues. - Several tests use
assert_true(true, ...)as the only assertion (e.g., scalar safety, malformed input, cleanup safety); you could make these more precise by asserting on observable side effects such as unchangedGlobals.settings.difficulty, absence of extra signal emissions, or specific mocked method calls instead of relying on “no crash” semantics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_on_change_difficulty_js`, the branch that treats `first_arg` as a container still calls `.size()` and indexes `[0]` on values that may be `JavaScriptObject`s; if the regression you’re fixing was around calling array APIs on scalars, consider restricting the container path to `first_arg is Array` and handling `JavaScriptObject` via a safer accessor (e.g., known property) to avoid future engine-level crashes.
- When the difficulty slider node is invalid in `_on_change_difficulty_js`, you fall back to writing `Globals.settings.difficulty` without checking that `Globals`/`Globals.settings` are valid; mirroring the guards you added in `_on_tree_exited` would make this callback more robust to teardown timing issues.
- Several tests use `assert_true(true, ...)` as the only assertion (e.g., scalar safety, malformed input, cleanup safety); you could make these more precise by asserting on observable side effects such as unchanged `Globals.settings.difficulty`, absence of extra signal emissions, or specific mocked method calls instead of relying on “no crash” semantics.
## Individual Comments
### Comment 1
<location path="scripts/gameplay_settings.gd" line_range="303-305" />
<code_context>
var first_arg: Variant = args[0]
+ var potential_value: Variant = null
+
+ # GS-JS-20/21: Strict Type Check
+ # Verify first_arg is a container before calling .size() or index [0]
+ if typeof(first_arg) == TYPE_ARRAY or first_arg is JavaScriptObject:
+ # GS-JS-11: Guard against nested empty arrays [[]]
+ if first_arg.size() > 0:
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid calling `.size()` on `JavaScriptObject`, which doesn’t guarantee that method exists.
In the branch below, `first_arg.size()` is still called when `first_arg is JavaScriptObject`. Since `JavaScriptObject` doesn’t guarantee `size()` or index access, this is a potential runtime error. Please branch the logic so that `.size()` and indexing are only used when `typeof(first_arg) == TYPE_ARRAY`, and handle `JavaScriptObject` via a separate path (e.g., converting to an array or using a defined API) before accessing its contents.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Mar 19, 2026 5:25a.m. | Review ↗ |
| JavaScript | |
Mar 19, 2026 5:25a.m. | Review ↗ |
…esn’t guarantee that method exists. In the branch below, first_arg.size() is still called when first_arg is JavaScriptObject. Since JavaScriptObject doesn’t guarantee size() or index access, this is a potential runtime error. Please branch the logic so that .size() and indexing are only used when typeof(first_arg) == TYPE_ARRAY, and handle JavaScriptObject via a separate path (e.g., converting to an array or using a defined API) before accessing its contents.
In _on_change_difficulty_js, the branch that treats first_arg as a container still calls .size() and indexes [0] on values that may be JavaScriptObjects; if the regression you’re fixing was around calling array APIs on scalars, consider restricting the container path to first_arg is Array and handling JavaScriptObject via a safer accessor (e.g., known property) to avoid future engine-level crashes.
![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: 1
♻️ Duplicate comments (2)
test/gut/test_gameplay_settings_lifecycle.gd (2)
81-90:⚠️ Potential issue | 🟠 Major
create_callbackcan make this cleanup test vacuous off-web.At Line 84,
JavaScriptBridge.create_callback(...)may benullon non-web, so the finalassert_nullcan pass without validating any cleanup transition.In Godot 4.x, what does `JavaScriptBridge.create_callback(callable)` return on non-Web platforms?Safer test shape
func test_gs_life_05_null_globals_safety() -> void: - # FIX: Wrap the lambda in a JavaScriptObject callback + if OS.get_name() != "Web": + pending("Web-only behavior: JavaScriptBridge.create_callback is not meaningful off-web.") + return + var dummy_callable := func(_args: Array) -> void: pass gameplay_menu._change_difficulty_cb = JavaScriptBridge.create_callback(dummy_callable) + assert_not_null(gameplay_menu._change_difficulty_cb, "Precondition: callback should be initialized") # Act: Call the cleanup function directly gameplay_menu._on_tree_exited()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_gameplay_settings_lifecycle.gd` around lines 81 - 90, The test assigns gameplay_menu._change_difficulty_cb using JavaScriptBridge.create_callback(dummy_callable), but on non-web platforms create_callback may return null making the assert vacuous; update test_gs_life_05_null_globals_safety to ensure _change_difficulty_cb is a real JavaScriptObject before calling gameplay_menu._on_tree_exited(): call JavaScriptBridge.create_callback(dummy_callable) and if it returns null, construct or stub a JavaScriptObject fallback (or otherwise wrap dummy_callable in a JS callback object) and assign that to gameplay_menu._change_difficulty_cb so the subsequent cleanup actually nullifies a non-null callback.
99-135:⚠️ Potential issue | 🟡 MinorMake
mock_prevcleanup failure-safe in this test too.This mirrors the earlier cleanup concern: if an assertion fails before Line 135, manual
free()is skipped and global test state can remain dirty.Safer cleanup pattern
var mock_prev: Control = Control.new() + autofree(mock_prev) mock_prev.name = "MockOptionsMenu" mock_prev.visible = false Globals.hidden_menus.push_back(mock_prev) @@ assert_true(mock_prev.visible, "Unexpected exit must restore previous menu visibility") assert_null(test_menu._change_difficulty_cb, "Callbacks must still be nullified on unexpected exit") assert_called(mock_js_bridge, "eval") - - # Cleanup mock menu - mock_prev.free() + assert_false(Globals.hidden_menus.has(mock_prev), "Restored menu should be popped from hidden stack")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_gameplay_settings_lifecycle.gd` around lines 99 - 135, Make the mock_prev cleanup failure-safe by ensuring it is always removed from Globals.hidden_menus and freed even if assertions fail: after creating mock_prev add it to a test-local cleanup step (or group) and in your test teardown always check if mock_prev is still in Globals.hidden_menus and remove it, then free it (use is_instance_valid(mock_prev) before calling mock_prev.free() or call_deferred("free") for safety); refer to the symbols mock_prev, Globals.hidden_menus, mock_prev.free(), and test_menu when implementing the teardown/cleanup so the global state is never left dirty if the test aborts early.
🧹 Nitpick comments (1)
test/gut/test_gameplay_settings_lifecycle.gd (1)
30-35: Also assert back/reset button signal disconnections here.
_on_tree_exited()disconnects three local UI signals, but this test currently verifies only the slider one. Add assertions forgameplay_back_button.pressedandgameplay_reset_button.pressedto prevent silent regressions.Suggested assertion additions
assert_false(gameplay_menu.difficulty_slider.value_changed.is_connected(gameplay_menu._on_difficulty_value_changed), "Local UI signals should be disconnected") + assert_false(gameplay_menu.gameplay_back_button.pressed.is_connected(gameplay_menu._on_gameplay_back_button_pressed), + "Back button signal should be disconnected") + assert_false(gameplay_menu.gameplay_reset_button.pressed.is_connected(gameplay_menu._on_gameplay_reset_button_pressed), + "Reset button signal should be disconnected")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_gameplay_settings_lifecycle.gd` around lines 30 - 35, The test is missing assertions that the back and reset button signals were disconnected after _on_tree_exited(); update the assertions block to also assert_false(gameplay_menu.gameplay_back_button.pressed.is_connected(gameplay_menu._on_back_pressed)) and assert_false(gameplay_menu.gameplay_reset_button.pressed.is_connected(gameplay_menu._on_reset_pressed)) (alongside the existing slider and global signal checks) so the three local UI signals disconnected in _on_tree_exited() are all covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/gut/test_gameplay_settings_js.gd`:
- Around line 93-99: Replace the no-op assertion in
test_gs_js_30_missing_node_safety with a concrete check that the fallback
resource update occurred after freeing difficulty_slider; call
gameplay_menu._on_change_difficulty_js([[1.2]]) and then assert the expected
state change on the object that should be updated by the fallback (for example
check gameplay_menu.current_difficulty or gameplay_menu.resource.difficulty
equals the expected value), referencing the test function name
test_gs_js_30_missing_node_safety, the freed member difficulty_slider, and the
handler gameplay_menu._on_change_difficulty_js to locate where to assert the
fallback behavior.
---
Duplicate comments:
In `@test/gut/test_gameplay_settings_lifecycle.gd`:
- Around line 81-90: The test assigns gameplay_menu._change_difficulty_cb using
JavaScriptBridge.create_callback(dummy_callable), but on non-web platforms
create_callback may return null making the assert vacuous; update
test_gs_life_05_null_globals_safety to ensure _change_difficulty_cb is a real
JavaScriptObject before calling gameplay_menu._on_tree_exited(): call
JavaScriptBridge.create_callback(dummy_callable) and if it returns null,
construct or stub a JavaScriptObject fallback (or otherwise wrap dummy_callable
in a JS callback object) and assign that to gameplay_menu._change_difficulty_cb
so the subsequent cleanup actually nullifies a non-null callback.
- Around line 99-135: Make the mock_prev cleanup failure-safe by ensuring it is
always removed from Globals.hidden_menus and freed even if assertions fail:
after creating mock_prev add it to a test-local cleanup step (or group) and in
your test teardown always check if mock_prev is still in Globals.hidden_menus
and remove it, then free it (use is_instance_valid(mock_prev) before calling
mock_prev.free() or call_deferred("free") for safety); refer to the symbols
mock_prev, Globals.hidden_menus, mock_prev.free(), and test_menu when
implementing the teardown/cleanup so the global state is never left dirty if the
test aborts early.
---
Nitpick comments:
In `@test/gut/test_gameplay_settings_lifecycle.gd`:
- Around line 30-35: The test is missing assertions that the back and reset
button signals were disconnected after _on_tree_exited(); update the assertions
block to also
assert_false(gameplay_menu.gameplay_back_button.pressed.is_connected(gameplay_menu._on_back_pressed))
and
assert_false(gameplay_menu.gameplay_reset_button.pressed.is_connected(gameplay_menu._on_reset_pressed))
(alongside the existing slider and global signal checks) so the three local UI
signals disconnected in _on_tree_exited() are all covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 017a0606-bb31-4fd5-a3f0-ce2b85ff132e
📒 Files selected for processing (6)
scripts/globals.gdtest/gut/test_gameplay_settings_js.gdtest/gut/test_gameplay_settings_lifecycle.gdtest/gut/test_gameplay_settings_ui.gdtest/gut/test_globals_resource.gdtest/gut/test_settings_observer.gd
✅ Files skipped from review due to trivial changes (3)
- test/gut/test_settings_observer.gd
- test/gut/test_globals_resource.gd
- scripts/globals.gd
🚧 Files skipped from review as they are similar to previous changes (1)
- test/gut/test_gameplay_settings_ui.gd
📜 Review details
⏰ 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 (1)
test/gut/test_gameplay_settings_js.gd (1)
25-88: Strong, behavior-driven coverage for JS payload handling.These tests verify real state transitions (accepted values, rejection paths, clamping), not just execution flow. Nice improvement for regression safety.
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In several places (e.g.
_ready()and_on_difficulty_value_changed) you compute asettings_reslocal but still read fromGlobals.settingsinside the guarded block; for consistency and to avoid any future race withGlobalsbecoming invalid, consider using the local reference exclusively once it’s established. - The
_on_change_difficulty_jshandler has become quite branch-heavy (array vsJavaScriptObjectvs scalar, plus type/bounds checks); extracting the value-normalization into a small helper (e.g._extract_js_difficulty(args: Array) -> Variant) would make the main callback easier to follow and reason about. - You now warn on any out-of-bounds JS difficulty, even though the value is then clamped by the resource; if out-of-range inputs are expected, consider downgrading this to debug or only warning when the value is wildly outside the allowed range to avoid noisy logs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In several places (e.g. `_ready()` and `_on_difficulty_value_changed`) you compute a `settings_res` local but still read from `Globals.settings` inside the guarded block; for consistency and to avoid any future race with `Globals` becoming invalid, consider using the local reference exclusively once it’s established.
- The `_on_change_difficulty_js` handler has become quite branch-heavy (array vs `JavaScriptObject` vs scalar, plus type/bounds checks); extracting the value-normalization into a small helper (e.g. `_extract_js_difficulty(args: Array) -> Variant`) would make the main callback easier to follow and reason about.
- You now warn on any out-of-bounds JS difficulty, even though the value is then clamped by the resource; if out-of-range inputs are expected, consider downgrading this to debug or only warning when the value is wildly outside the allowed range to avoid noisy logs.
## Individual Comments
### Comment 1
<location path="scripts/gameplay_settings.gd" line_range="334-340" />
<code_context>
+ else:
+ Globals.log_message("JS callback: Array is empty.", Globals.LogLevel.WARNING)
+ return
+ elif first_arg is JavaScriptObject:
+ # For JavaScriptObject, treat it as a proxy to a JS array
+ # Use the specific JS indexing if you are certain it is a JS array,
+ # or handle it as a single-value reference.
+ # JS-FIX: If we receive a JS Object (like from Playwright),
+ # we must index it to get the raw value before the type check.
+ if first_arg.length > 0:
+ potential_value = first_arg[0]
+ else:
</code_context>
<issue_to_address>
**issue (bug_risk):** Assuming `.length` and numeric indexing on any `JavaScriptObject` can be fragile.
Here we assume every `JavaScriptObject` is an array-like proxy and use `length` and `[0]`. If a plain object is passed (e.g. `{ value: 3 }`), `length` may be missing or non-numeric and indexing can misbehave. Please either validate that the object is array-like before using `length`/indexing (e.g. check for a numeric `length`), or handle generic `JavaScriptObject` values as scalars instead to avoid runtime issues.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@sourcery-ai title |
sourcery-ai
Bot
changed the title
Line 99 (assert_true(true, ...)) can’t detect regressions. This path has a defined fallback behavior (resource update when slider is invalid), so assert that explicitly.
…riptObject can be fragile.
Here we assume every JavaScriptObject is an array-like proxy and use length and [0]. If a plain object is passed (e.g. { value: 3 }), length may be missing or non-numeric and indexing can misbehave. Please either validate that the object is array-like before using length/indexing (e.g. check for a numeric length), or handle generic JavaScriptObject values as scalars instead to avoid runtime issues.
In several places (e.g. _ready() and _on_difficulty_value_changed) you compute a settings_res local but still read from Globals.settings inside the guarded block; for consistency and to avoid any future race with Globals becoming invalid, consider using the local reference exclusively once it’s established.
The _on_change_difficulty_js handler has become quite branch-heavy (array vs JavaScriptObject vs scalar, plus type/bounds checks); extracting the value-normalization into a small helper (e.g. _extract_js_difficulty(args: Array) -> Variant) would make the main callback easier to follow and reason about.
By switching to .length, Godot correctly reads the length property of the JS Array [2.0], sees that it is 1, passes the js_length > 0 condition, safely extracts the float 2.0 via first_arg[0], and proceeds to successfully log: "JS difficulty callback called with valid value: 2.0".
Owner
Author
All addressed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/gameplay_settings.gd (1)
151-188:⚠️ Potential issue | 🟠 MajorDecouple unexpected menu restoration from the web bridge.
Line 152 currently gates the
Globals.hidden_menusrestore path behindos_wrapper.has_feature("web") and js_window and js_bridge_wrapper. On native builds — or on web whenjs_windowsetup fails — an unexpected removal leaves the previous menu hidden and the stack stale.Suggested fix
- # Web overlay cleanup + optional menu restore - if os_wrapper.has_feature("web") and js_window and js_bridge_wrapper: + var hidden_menu_found := false + if not _intentional_exit and not Globals.hidden_menus.is_empty(): + var prev_menu: Node = Globals.hidden_menus.pop_back() + if is_instance_valid(prev_menu): + prev_menu.visible = true + hidden_menu_found = true + Globals.log_message( + "tree_exited: Restored menu: " + prev_menu.name, Globals.LogLevel.DEBUG + ) + + # Web overlay cleanup + if os_wrapper.has_feature("web") and js_window and js_bridge_wrapper: # Hide gameplay overlays (same DOM elements shown in _ready) var hide_gameplay: String = """ document.getElementById('difficulty-slider').style.display = 'none'; document.getElementById('gameplay-back-button').style.display = 'none'; document.getElementById('gameplay-reset-button').style.display = 'none'; """ - if not _intentional_exit and not Globals.hidden_menus.is_empty(): - # Unexpected exit → restore previous menu and options overlays - var prev_menu: Node = Globals.hidden_menus.pop_back() - if is_instance_valid(prev_menu): - prev_menu.visible = true - Globals.log_message( - "tree_exited: Restored menu: " + prev_menu.name, Globals.LogLevel.DEBUG - ) - + if hidden_menu_found: + # Unexpected exit → restore previous menu and options overlays ( js_bridge_wrapper . eval(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gameplay_settings.gd` around lines 151 - 188, The restore of Globals.hidden_menus is currently nested inside the web-bridge check (os_wrapper.has_feature("web") and js_window and js_bridge_wrapper) which prevents restoring the previous menu when the web bridge or js_window is absent; refactor so that the logic that pops and restores Globals.hidden_menus (check _intentional_exit, pop_back into prev_menu, is_instance_valid(prev_menu), set prev_menu.visible = true and Globals.log_message) runs unconditionally (or at least outside the js_bridge_wrapper block), while keeping only the DOM JS eval calls (js_bridge_wrapper.eval(hide_gameplay, true) and the options-show script) behind the js_bridge_wrapper/js_window/web feature checks; use the existing symbols Globals.hidden_menus, _intentional_exit, prev_menu, js_bridge_wrapper.eval, js_window and os_wrapper.has_feature to locate and separate the restore logic from the web-only eval calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/gameplay_settings.gd`:
- Around line 292-304: The handler currently updates UI controls unconditionally
after validating settings_res, which can access freed nodes (difficulty_slider
and difficulty_label) during teardown; change the code to check the UI nodes
before using them (e.g., wrap the assignments to difficulty_slider.value and
difficulty_label.text in is_instance_valid(difficulty_slider) /
is_instance_valid(difficulty_label) or nil checks) so the handler safely skips
UI sync when those controls are freed while still using the validated local
settings_res reference.
In `@tests/difficulty_flow_test.py`:
- Around line 193-197: The current assertion in tests/difficulty_flow_test.py
uses a substring "setting 'difficulty' updated to: 1" which can match values
like 1.5; update the check to require "setting 'difficulty' updated to: 1.0" or,
more robustly, parse the numeric suffix from each log["text"].lower() and assert
float(parsed_value) == 1.0 so that the reset_logs check only passes when the
emitted difficulty equals 1.0 (referencing the existing reset_logs variable and
the log text pattern "setting 'difficulty' updated to: ...").
---
Outside diff comments:
In `@scripts/gameplay_settings.gd`:
- Around line 151-188: The restore of Globals.hidden_menus is currently nested
inside the web-bridge check (os_wrapper.has_feature("web") and js_window and
js_bridge_wrapper) which prevents restoring the previous menu when the web
bridge or js_window is absent; refactor so that the logic that pops and restores
Globals.hidden_menus (check _intentional_exit, pop_back into prev_menu,
is_instance_valid(prev_menu), set prev_menu.visible = true and
Globals.log_message) runs unconditionally (or at least outside the
js_bridge_wrapper block), while keeping only the DOM JS eval calls
(js_bridge_wrapper.eval(hide_gameplay, true) and the options-show script) behind
the js_bridge_wrapper/js_window/web feature checks; use the existing symbols
Globals.hidden_menus, _intentional_exit, prev_menu, js_bridge_wrapper.eval,
js_window and os_wrapper.has_feature to locate and separate the restore logic
from the web-only eval calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3c682588-2f15-4d87-a781-5c63d2759ab3
📒 Files selected for processing (3)
scripts/gameplay_settings.gdtest/gut/test_gameplay_settings_js.gdtests/difficulty_flow_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/gut/test_gameplay_settings_js.gd
📜 Review details
⏰ 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 (1)
scripts/gameplay_settings.gd (1)
373-409: Nice containment of the JS payload parsing.Pulling the shape handling into
_extract_js_difficulty()keeps_on_change_difficulty_js()linear and makes the array-vs-JavaScriptObjectcases much easier to reason about and test.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
The substring "updated to: 1" would still match log lines like "setting 'difficulty' updated to: 1.5". A safer approach using a Python regex would address that without breaking on GDScript's string representation:
…thub.com/ikostan/SkyLockAssault into settings-labels-display-unclamped-values
The lookahead part of your regex (?![\d.]) specifically tells Python: "Find a 1, but fail if it is followed by a digit or a dot." Because Godot is dealing with a float, it is logging the reset value as 1.0. The regex sees the . after the 1 and intentionally rejects the match, causing your test to fail even though the game logic worked perfectly!
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)
scripts/gameplay_settings.gd (1)
145-188:⚠️ Potential issue | 🟠 MajorDon't gate overlay teardown on
js_window.
_ready()shows the gameplay overlays beforeget_interface("window")succeeds. Ifjs_windowis null or already cleared, this branch skips theeval(...)hide path and strands those overlays on screen after teardown. Keep callback nulling behindjs_window, but drive the DOM cleanup offos_wrapper.has_feature("web") and js_bridge_wrapper.Suggested fix
- if os_wrapper.has_feature("web") and js_window and js_bridge_wrapper: + if os_wrapper.has_feature("web") and js_bridge_wrapper:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gameplay_settings.gd` around lines 145 - 188, The DOM teardown is incorrectly gated on js_window so overlays can be left visible; keep nulling callbacks (_change_difficulty_cb, _gameplay_back_button_pressed_cb, _gameplay_reset_cb and call to _unset_gameplay_settings_window_callbacks()) only when js_window is present, but drive the overlay cleanup when os_wrapper.has_feature("web") and js_bridge_wrapper are available (i.e., remove js_window from the outer conditional that controls the hide/eval path); ensure both the simple js_bridge_wrapper.eval(hide_gameplay, true) and the restore path that calls js_bridge_wrapper.eval(...) with the options-show + hide_gameplay payload still run when js_bridge_wrapper exists so overlays are always hidden even if js_window is null.
♻️ Duplicate comments (1)
tests/difficulty_flow_test.py (1)
176-179:⚠️ Potential issue | 🟡 MinorMake the JS difficulty log assertion tolerant of Godot float formatting.
scripts/gameplay_settings.gdLines 367-368 also logstr(value), but this assertion hard-codes2.0while the reset assertion below already assumes whole-number floats may be rendered without.0. If the engine logs2here, the bridge works and the test still fails.Suggested fix
assert any( - "js difficulty callback called with valid value: 2.0" in log["text"].lower() + re.search( + r"js difficulty callback called with valid value:\s*2(?:\.0+)?(?![\d.])", + log["text"].lower(), + ) for log in new_logs ), "Failed to extract/validate difficulty 2.0 from JS payload"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/difficulty_flow_test.py` around lines 176 - 179, The assertion checking for the JS difficulty log is too strict about the float formatting; update the assertion that inspects new_logs (the comprehension using log["text"].lower()) to accept either "2.0" or "2" (or normalize with a regex/float parse) when looking for the substring like "js difficulty callback called with valid value:" so the test is tolerant of Godot printing whole-number floats without the .0; modify the condition in the failing assertion to match both representations (or parse the numeric token after the message and compare numerically) to ensure the bridge validation still passes.
🧹 Nitpick comments (1)
tests/difficulty_flow_test.py (1)
169-200: Add one direct settings-screen state assertion.These checks prove the callback fired and gameplay eventually uses the value, but they still won't catch the known label/slider desync for unclamped inputs because the test never verifies what the menu displays after
changeDifficulty(...)or reset. Please cover that in this flow or in the dedicated GUT UI suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/difficulty_flow_test.py` around lines 169 - 200, Add assertions that directly verify the settings UI state (label/slider) after calling window.changeDifficulty([2.0]) and after calling window.gameplayResetPressed([]): locate the on-page elements that display the difficulty value (e.g., the difficulty label text and the difficulty input/slider element) using the same page object used in this test, wait for them to be present after each evaluate call, and assert their displayed value/text matches the expected values (e.g., "2.0" after changeDifficulty and "1.0" after reset); reference the existing uses of page.evaluate("window.changeDifficulty([2.0])"), page.evaluate("window.gameplayResetPressed([])"), and the logs/reset_logs to add these UI assertions immediately after the corresponding wait_for_timeout blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/gameplay_settings.gd`:
- Line 123: Guard every dereference of the Globals singleton in
_on_tree_exited() by first checking that Globals is not null/undefined before
calling its methods or accessing its properties; specifically, wrap the
Globals.log_message call in _on_tree_exited() and the hidden_menus restore logic
(the code around hidden_menus at the later block) with a simple null/exists
check (e.g., verify Globals != null or typeof(Globals) != TYPE_NIL) and only
perform logging or restore when that check passes so the node can exit safely if
the singleton was torn down.
---
Outside diff comments:
In `@scripts/gameplay_settings.gd`:
- Around line 145-188: The DOM teardown is incorrectly gated on js_window so
overlays can be left visible; keep nulling callbacks (_change_difficulty_cb,
_gameplay_back_button_pressed_cb, _gameplay_reset_cb and call to
_unset_gameplay_settings_window_callbacks()) only when js_window is present, but
drive the overlay cleanup when os_wrapper.has_feature("web") and
js_bridge_wrapper are available (i.e., remove js_window from the outer
conditional that controls the hide/eval path); ensure both the simple
js_bridge_wrapper.eval(hide_gameplay, true) and the restore path that calls
js_bridge_wrapper.eval(...) with the options-show + hide_gameplay payload still
run when js_bridge_wrapper exists so overlays are always hidden even if
js_window is null.
---
Duplicate comments:
In `@tests/difficulty_flow_test.py`:
- Around line 176-179: The assertion checking for the JS difficulty log is too
strict about the float formatting; update the assertion that inspects new_logs
(the comprehension using log["text"].lower()) to accept either "2.0" or "2" (or
normalize with a regex/float parse) when looking for the substring like "js
difficulty callback called with valid value:" so the test is tolerant of Godot
printing whole-number floats without the .0; modify the condition in the failing
assertion to match both representations (or parse the numeric token after the
message and compare numerically) to ensure the bridge validation still passes.
---
Nitpick comments:
In `@tests/difficulty_flow_test.py`:
- Around line 169-200: Add assertions that directly verify the settings UI state
(label/slider) after calling window.changeDifficulty([2.0]) and after calling
window.gameplayResetPressed([]): locate the on-page elements that display the
difficulty value (e.g., the difficulty label text and the difficulty
input/slider element) using the same page object used in this test, wait for
them to be present after each evaluate call, and assert their displayed
value/text matches the expected values (e.g., "2.0" after changeDifficulty and
"1.0" after reset); reference the existing uses of
page.evaluate("window.changeDifficulty([2.0])"),
page.evaluate("window.gameplayResetPressed([])"), and the logs/reset_logs to add
these UI assertions immediately after the corresponding wait_for_timeout blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be24394e-3013-4c54-9257-7181f497f4fa
📒 Files selected for processing (2)
scripts/gameplay_settings.gdtests/difficulty_flow_test.py
📜 Review details
⏰ 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 (1)
scripts/gameplay_settings.gd (1)
396-405: JavaScriptObject bracket indexing is supported by Godot; the current implementation is defensible.The code correctly validates that
js_lengthis numeric and positive before indexing (line 405). Godot's official documentation explicitly states JavaScriptObject supports bracket notation for both reading and writing, mirroring JavaScript's dynamic access. The length check is a reasonable array-like guard, and the fallback path (returningfirst_argas a scalar) safely handles any object that does not behave as expected. Downstream validation in_on_change_difficulty_js()further ensures the extracted value is numeric before use.> Likely an incorrect or invalid review comment.
…thub.com/ikostan/SkyLockAssault into settings-labels-display-unclamped-values
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
Line 123 and Lines 160-166 still dereference Globals directly. If the singleton is torn down before this node exits, _on_tree_exited() can still explode while trying to log or restore hidden_menus, which undercuts the lifecycle hardening added elsewhere.
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
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
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")
Description
This PR implements a comprehensive suite of unit tests and defensive programming patterns for the
GameplaySettingsmodule. The primary goal is to resolve Issue #471 (engine crashes during JavaScript-to-Godot communication) and to ensure the UI menu remains stable during complex lifecycle events and teardowns.Key Changes & Bug Fixes
JavaScriptBridgepayloads.GameplaySettingsUI and observers safely handle missing or invalidGlobalssettings, freed nodes, and unexpected tree exits without crashing.js_windowreference type to a genericVariantto better support dictionary-based mocks and flexible JS interfaces.Testing
GameSettingsResourceclamping, signal emission behavior, and boundary values.Globalsis shaky).Contributions
is_instance_validlifecycle guards across theGameplaySettingsmodule..length) forJavaScriptObjectproperties to avoid bridge evaluation errors.difficulty_flow_test.pyandreset_audio_flow_test.py) to align with the new value-normalization and DOM overlay logic.AI & Bot Contributions
@sourcery-ai:
_on_change_difficulty_jswhereJavaScriptObjectpayloads could be improperly rejected, suggesting primitive extraction prior to type-checking._unset_gameplay_settings_window_callbacksto handle genericVariantscenarios safely in non-web or mocked environments.after_each()teardowns to prevent cross-test pollution and ensuring all JS callbacks (like_gameplay_reset_cb) are explicitly asserted as nullified during cleanup tests.@coderabbitai:
@deepsource-io:
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
Harden GameplaySettings initialization, cleanup, and JS bridge handling while adding comprehensive tests for settings resource, UI synchronization, lifecycle behavior, and JavaScript communication.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit