Conversation
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 6 to 7. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v6...v7) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 4.32.3 to 4.32.4. - [Release notes](https://github.com/github/codeql-action/releases) - [Commits](github/codeql-action@v4.32.3...v4.32.4) --- updated-dependencies: - dependency-name: github/codeql-action dependency-version: 4.32.4 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
…ber "settings". #444 Based on the logs and files, moving those resources to res://config_resources/ and breaking that circular dependency between the Globals and the GameSettingsResource was the key to stabilizing the engine's compilation process. By resolving the naming conflict between the folder and the Autoload, we've ensured that Godot can clearly distinguish between the static Settings singleton and the actual filesystem paths. Summary of the Resolution: Namespace Clarity: Renaming the folder to res://config_resources/ stopped the collision with the Settings Autoload. Broken Circularity: Removing the Globals logging call from the Resource script allows the resource to compile independently. Safe Loading: Updating globals.gd to use the correct path ensured that the @export variable actually finds its data.
…s/upload-artifact-7 Bump actions/upload-artifact from 6 to 7
…/codeql-action-4.32.4 Bump github/codeql-action from 4.32.3 to 4.32.4
ikostan
added this to the
Milestone 13: Resource Persistence & Core Gameplay Signals milestone
ikostan
added
menu
GUI
dependencies
Contributor
Reviewer's GuideImproves robustness of settings loading and input event deserialization, makes the input remap UI resilient when settings are unavailable, simplifies difficulty clamping, documents reset behavior, updates Globals tests to explicitly initialize settings, adds Playwright E2E checks for clean browser-console loads, and bumps GitHub Actions/CodeQL/SARIF-related workflow versions. Sequence diagram for Globals settings initializationsequenceDiagram
participant Engine
participant Globals
participant GameSettingsResource
Engine->>Globals: _ready()
activate Globals
Globals->>GameSettingsResource: load(res_path)
alt settings_loaded_successfully
Globals->>Globals: settings = loaded_resource
else settings_failed_to_load
Globals->>Globals: push_error("CRITICAL: GameSettingsResource failed to load")
Globals->>GameSettingsResource: new()
Globals->>Globals: settings = new_resource
Globals->>Globals: settings.current_log_level = LogLevel.WARNING
end
alt is_editor_hint or settings.enable_debug_logging
Globals->>Globals: settings.current_log_level = LogLevel.DEBUG
end
Globals->>Globals: log_message("Log level set to ...", LogLevel.DEBUG)
deactivate Globals
Sequence diagram for safe input remap button presssequenceDiagram
actor Player
participant InputRemapButton
participant Globals
participant GameSettingsResource
Player->>InputRemapButton: press()
activate InputRemapButton
InputRemapButton->>InputRemapButton: listening = button_pressed
alt listening_is_true
InputRemapButton->>Globals: is_instance_valid(Globals)
alt globals_and_settings_ready
Globals-->>InputRemapButton: settings
alt current_device_is_keyboard
InputRemapButton->>GameSettingsResource: remap_prompt_keyboard
GameSettingsResource-->>InputRemapButton: prompt
InputRemapButton->>InputRemapButton: text = remap_prompt_keyboard
else current_device_is_gamepad
InputRemapButton->>GameSettingsResource: remap_prompt_gamepad
GameSettingsResource-->>InputRemapButton: prompt
InputRemapButton->>InputRemapButton: text = remap_prompt_gamepad
end
else settings_not_available
InputRemapButton->>InputRemapButton: push_error("Globals.settings is NULL")
alt current_device_is_keyboard
InputRemapButton->>InputRemapButton: text = "Press a key"
else current_device_is_gamepad
InputRemapButton->>InputRemapButton: text = "Press a button"
end
end
else listening_is_false
InputRemapButton->>InputRemapButton: update_button_text()
end
deactivate InputRemapButton
Updated class diagram for Globals and settings handlingclassDiagram
class GameSettingsResource {
+float difficulty
-float _difficulty
+float get_difficulty()
+void set_difficulty(float value)
}
class LogLevel {
<<enumeration>>
DEBUG
INFO
WARNING
ERROR
NONE
}
class Globals {
<<Node>>
+LogLevel log_level
+GameSettingsResource settings
+CanvasLayer options_instance
+String current_input_device
+void _ready()
+void log_message(String message, LogLevel level)
}
class InputRemapButton {
<<Button>>
+bool listening
+bool button_pressed
+int current_device
+String text
+void _ready()
+void _on_pressed()
+void update_button_text()
}
Globals --> GameSettingsResource : loads_and_owns
InputRemapButton ..> Globals : reads_settings_and_logs
InputRemapButton ..> GameSettingsResource : uses_remap_prompts
GameSettingsResource : set_difficulty clamps value to 0.5..2.0 without logging
Globals : _ready loads GameSettingsResource at runtime with fallback
InputRemapButton : _on_pressed uses safe fallback prompts if Globals.settings is null
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…tings' of https://github.com/ikostan/SkyLockAssault into parse-error-could-not-resolve-external-class-member-settings
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:
📝 WalkthroughWalkthroughThis PR updates several GitHub Actions/pinned SHAs, moves GameSettingsResource loading from preload to runtime with a crash-safe fallback, removes a warning in the difficulty setter, adds null-safety to input remap handling, refactors settings deserialization, and adds Playwright and GUT test changes. Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
 |
|
Overall Grade Focus Area: Hygiene |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Mar 4, 2026 6:08a.m. | Review ↗ |
| JavaScript | |
Mar 4, 2026 6:08a.m. | Review ↗ |
![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
input_remap_button.gd, theelsebranch of_on_pressedstill callsGlobals.log_messageeven though theifis explicitly guarding againstGlobals/Globals.settingsbeing invalid; consider usingpush_erroror another safe logger there to avoid a potential null reference, and fix the extra trailing quote in the log message string. - When loading
settingsinglobals.gdwithload("res://config_resources/default_settings.tres"), consider casting toGameSettingsResource(e.g.as GameSettingsResource) or asserting the type so that incorrect resources are caught early and the type is clear to readers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `input_remap_button.gd`, the `else` branch of `_on_pressed` still calls `Globals.log_message` even though the `if` is explicitly guarding against `Globals`/`Globals.settings` being invalid; consider using `push_error` or another safe logger there to avoid a potential null reference, and fix the extra trailing quote in the log message string.
- When loading `settings` in `globals.gd` with `load("res://config_resources/default_settings.tres")`, consider casting to `GameSettingsResource` (e.g. `as GameSettingsResource`) or asserting the type so that incorrect resources are caught early and the type is clear to readers.
## Individual Comments
### Comment 1
<location path="scripts/input_remap_button.gd" line_range="120-128" />
<code_context>
listening = button_pressed
if listening:
+ # Safely check if Globals and settings are ready
+ if is_instance_valid(Globals) and Globals.settings:
# FIXED: Check the actual current_device to return the correct prompt
- text = (
- Globals.settings.remap_prompt_keyboard
- if current_device == DeviceType.KEYBOARD
- else Globals.settings.remap_prompt_gamepad
- )
+ text = (
+ Globals.settings.remap_prompt_keyboard
+ if current_device == DeviceType.KEYBOARD
+ else Globals.settings.remap_prompt_gamepad
+ )
+ else:
+ Globals.log_message(
+ "'Globals.settings' resource is NULL'",
+ Globals.LogLevel.ERROR
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard for `Globals`/`Globals.settings` still calls `Globals.log_message` even when `Globals` is invalid, and the error string has a stray quote.
In the `else` branch you’re still calling `Globals.log_message` when `is_instance_valid(Globals)` is false, which means `Globals` may be freed or uninitialized and can cause a crash. Either return early when `Globals` is invalid, or only call `Globals.log_message` when it’s valid and fall back to `push_error`/`print_error` otherwise.
Also, the error message has mismatched quotes: `"'Globals.settings' resource is NULL'"` (extra trailing `'`).
</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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/settings.gd (1)
374-427:⚠️ Potential issue | 🟠 Major
deserialize_eventis blocking CI due to max-returns lint failure.The pipeline failure at Line [427] is release-blocking (
gdlint ./scripts). Please refactordeserialize_eventto reduce return statements while preserving current parsing behavior.Refactor sketch (same behavior, fewer returns)
func deserialize_event(serialized: String) -> InputEvent: - # 1. Reject plain integers or empty strings immediately - if not ( - serialized.begins_with("key:") - or serialized.begins_with("joybtn:") - or serialized.begins_with("joyaxis:") - ): - return null - - var parts: PackedStringArray = serialized.split(":", true) - if parts.size() < 2: - return null - - match parts[0]: - "key": - if parts.size() >= 2 and parts[1].is_valid_int(): - var ev := InputEventKey.new() - ev.physical_keycode = parts[1].to_int() - if ev.physical_keycode == 0: - return - if "shift" in parts: - ev.shift_pressed = true - if "ctrl" in parts: - ev.ctrl_pressed = true - if "alt" in parts: - ev.alt_pressed = true - if "meta" in parts: - ev.meta_pressed = true - return ev - "joybtn": - if parts.size() == 3 and parts[1].is_valid_int() and parts[2].is_valid_int(): - var ev := InputEventJoypadButton.new() - ev.button_index = parts[1].to_int() - ev.device = parts[2].to_int() - return ev - "joyaxis": - if ( - parts.size() == 4 - and parts[1].is_valid_int() - and parts[2].is_valid_float() - and parts[3].is_valid_int() - ): - var ev := InputEventJoypadMotion.new() - ev.axis = parts[1].to_int() - ev.axis_value = parts[2].to_float() - ev.device = parts[3].to_int() - return ev - - return null + var parsed: InputEvent = null + var valid_prefix: bool = ( + serialized.begins_with("key:") + or serialized.begins_with("joybtn:") + or serialized.begins_with("joyaxis:") + ) + if not valid_prefix: + return null + + var parts: PackedStringArray = serialized.split(":", true) + if parts.size() < 2: + return null + + match parts[0]: + "key": + if parts[1].is_valid_int(): + var ev := InputEventKey.new() + ev.physical_keycode = parts[1].to_int() + if ev.physical_keycode != 0: + ev.shift_pressed = "shift" in parts + ev.ctrl_pressed = "ctrl" in parts + ev.alt_pressed = "alt" in parts + ev.meta_pressed = "meta" in parts + parsed = ev + "joybtn": + if parts.size() == 3 and parts[1].is_valid_int() and parts[2].is_valid_int(): + var ev := InputEventJoypadButton.new() + ev.button_index = parts[1].to_int() + ev.device = parts[2].to_int() + parsed = ev + "joyaxis": + if ( + parts.size() == 4 + and parts[1].is_valid_int() + and parts[2].is_valid_float() + and parts[3].is_valid_int() + ): + var ev := InputEventJoypadMotion.new() + ev.axis = parts[1].to_int() + ev.axis_value = parts[2].to_float() + ev.device = parts[3].to_int() + parsed = ev + + return parsed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/settings.gd` around lines 374 - 427, The function deserialize_event currently uses many early returns; refactor it to have a single return at the end by introducing a local variable (e.g., result or res) initialized to null and, instead of returning immediately in each branch (including the initial rejection, invalid parts, and physical_keycode==0), assign the appropriate InputEvent object to that variable when a valid event is parsed and leave it null on failures; keep the same checks and modifiers logic (use ev/parts/physical_keycode/shift_pressed/ctrl_pressed/alt_pressed/meta_pressed, as well as InputEventKey, InputEventJoypadButton, InputEventJoypadMotion, button_index, axis, axis_value, device) but replace all "return ev" or "return" statements with "result = ev" (or no-op to leave null) and at the very end of deserialize_event do a single "return result".
🧹 Nitpick comments (2)
.github/workflows/codeql.yml (1)
80-80: CodeQL analyze action upgraded consistently.Consider using SHA pinning for these actions (as done in snyk.yml and trivy.yml with
upload-sarif) for stronger supply-chain security. This is optional since version tags are acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/codeql.yml at line 80, The CodeQL GitHub Action is pinned by a version tag (github/codeql-action/[email protected]); replace that tag with the specific commit SHA used elsewhere (same pattern as in snyk.yml and trivy.yml for upload-sarif) to improve supply-chain security by changing the uses reference from the version tag to the action's full commit SHA (e.g., github/codeql-action/analyze@<commit-sha>) and verify the SHA matches the intended release commit before committing..github/workflows/snyk.yml (1)
46-49: Add a version comment for consistency with the codeql-action SHA in trivy.yml.The trivy.yml workflow already documents the same SHA as pinned to v4.32.2. Add the same version comment here for clarity:
📝 Suggested change
- uses: "github/codeql-action/upload-sarif@0ec47d036c68ae0cf94c629009b1029407111281" + uses: "github/codeql-action/upload-sarif@0ec47d036c68ae0cf94c629009b1029407111281" # Pinned to SHA for v4.32.2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/snyk.yml around lines 46 - 49, Add a version comment next to the pinned CodeQL upload action to match the style used in trivy.yml: locate the action reference github/codeql-action/upload-sarif@0ec47d036c68ae0cf94c629009b1029407111281 and add a trailing comment indicating the human-readable version (e.g., "# v4.32.2") so the SHA and version are documented consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/trivy.yml:
- Around line 39-42: The comment next to the pinned action
"github/codeql-action/upload-sarif@0ec47d036c68ae0cf94c629009b1029407111281" is
incorrect (mentions v4.32.2) and the SHA corresponds to v4.32.5, causing a
mismatch with codeql.yml which uses v4.32.4; update the workflow pins so all
workflows use the same explicit release tag (e.g., change the uses to
"github/codeql-action/[email protected]" or update codeql.yml to v4.32.5) and
fix the inline comment to match the chosen tag, ensuring the SHA/tag and comment
are consistent across trivy.yml, snyk.yml, and codeql.yml.
In `@scripts/globals.gd`:
- Around line 39-44: The current early return after
load("res://config_resources/default_settings.tres") can leave the Globals
'settings' null and cause later null dereferences (e.g., in log_message());
replace the early return with a safe fallback: either instantiate a
default/minimal GameSettingsResource or assign a known-safe fallback resource
and push_error with the failure details, and add a null-check guard in functions
that access 'settings' (such as log_message()) to avoid dereferencing when
settings is absent; update references to the 'settings' variable and the load
block to ensure Globals remains in a safe, non-crashing state even if the
resource fails to load.
In `@scripts/input_remap_button.gd`:
- Around line 119-129: The fallback else branch still calls Globals.log_message
even when Globals may be invalid; update the else branch in
input_remap_button.gd (the block that checks is_instance_valid(Globals) and
Globals.settings and sets text based on current_device and DeviceType) to avoid
referencing Globals when the guard fails—use a safe fallback like push_error(),
print(), or OS.alert() (or a local error handler) to report "'Globals.settings'
resource is NULL'" and ensure no Globals.* symbols are accessed in that branch.
---
Outside diff comments:
In `@scripts/settings.gd`:
- Around line 374-427: The function deserialize_event currently uses many early
returns; refactor it to have a single return at the end by introducing a local
variable (e.g., result or res) initialized to null and, instead of returning
immediately in each branch (including the initial rejection, invalid parts, and
physical_keycode==0), assign the appropriate InputEvent object to that variable
when a valid event is parsed and leave it null on failures; keep the same checks
and modifiers logic (use
ev/parts/physical_keycode/shift_pressed/ctrl_pressed/alt_pressed/meta_pressed,
as well as InputEventKey, InputEventJoypadButton, InputEventJoypadMotion,
button_index, axis, axis_value, device) but replace all "return ev" or "return"
statements with "result = ev" (or no-op to leave null) and at the very end of
deserialize_event do a single "return result".
---
Nitpick comments:
In @.github/workflows/codeql.yml:
- Line 80: The CodeQL GitHub Action is pinned by a version tag
(github/codeql-action/[email protected]); replace that tag with the specific
commit SHA used elsewhere (same pattern as in snyk.yml and trivy.yml for
upload-sarif) to improve supply-chain security by changing the uses reference
from the version tag to the action's full commit SHA (e.g.,
github/codeql-action/analyze@<commit-sha>) and verify the SHA matches the
intended release commit before committing.
In @.github/workflows/snyk.yml:
- Around line 46-49: Add a version comment next to the pinned CodeQL upload
action to match the style used in trivy.yml: locate the action reference
github/codeql-action/upload-sarif@0ec47d036c68ae0cf94c629009b1029407111281 and
add a trailing comment indicating the human-readable version (e.g., "# v4.32.2")
so the SHA and version are documented consistently.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/browser_test.yml.github/workflows/codeql.yml.github/workflows/gdunit4_tests.yml.github/workflows/gut_tests.yml.github/workflows/snyk.yml.github/workflows/trivy.ymlconfig_resources/default_settings.tresscripts/game_settings_resource.gdscripts/globals.gdscripts/input_remap_button.gdscripts/key_mapping.gdscripts/settings.gd
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-02-25T05:50:14.510Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 429
File: scripts/input_remap_button.gd:158-162
Timestamp: 2026-02-25T05:50:14.510Z
Learning: In `scripts/input_remap_button.gd`, the keyboard event copying logic (around lines 158-162) intentionally omits `meta_pressed` when transferring modifier states to new_event. Only `shift_pressed`, `ctrl_pressed`, and `alt_pressed` are copied. This is by design.
Applied to files:
scripts/input_remap_button.gdscripts/settings.gd
📚 Learning: 2026-02-25T05:46:55.056Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 429
File: scripts/settings.gd:593-601
Timestamp: 2026-02-25T05:46:55.056Z
Learning: In `scripts/settings.gd`, the `events_match` function intentionally omits `meta_pressed` comparison for InputEventKey events. It only compares `physical_keycode`, `shift_pressed`, `ctrl_pressed`, and `alt_pressed`. This is by design.
Applied to files:
scripts/input_remap_button.gd
📚 Learning: 2026-02-25T05:46:48.070Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 429
File: scripts/settings.gd:593-601
Timestamp: 2026-02-25T05:46:48.070Z
Learning: In scripts/settings.gd, the events_match function intentionally omits meta_pressed when comparing InputEventKey events; it should only compare physical_keycode, shift_pressed, ctrl_pressed, and alt_pressed. This behavior is by design. When reviewing similar code, verify that InputEventKey comparisons do not include meta_pressed unless a change is requested, and ensure there are tests that reflect the intended behavior. If you encounter a case where meta_pressed should be considered, discuss rationale before changing.
Applied to files:
scripts/settings.gd
🪛 GitHub Actions: Pull Request Pipeline
scripts/settings.gd
[error] 427-427: gdlint reported: Function "deserialize_event" has more than 6 return statements (max-returns). Command failed: 'gdlint ./scripts' (exit code 1).
🔇 Additional comments (10)
scripts/key_mapping.gd (1)
182-183: Good clarification for reset flow UI sync.The note at Line [182] matches the required behavior and is immediately enforced at Line [183], which helps prevent stale button labels after reset.
scripts/game_settings_resource.gd (1)
25-27: Decoupling warning emission fromGlobalsis a solid fix.Using
push_warningat Line [26] preserves visibility while keepingGameSettingsResourceindependent fromGlobals..github/workflows/gut_tests.yml (1)
56-60: Consistent upgrade toactions/upload-artifact@v7.This change aligns with the same upgrade applied across other workflow files in this PR.
.github/workflows/snyk.yml (1)
57-60: Same SHA update applied consistently.Both SARIF upload steps now use the same SHA pin, maintaining consistency.
.github/workflows/browser_test.yml (3)
153-157: LCOV artifact upload upgraded to v7.Configuration remains compatible with the new version.
179-182: Test report artifact upload upgraded to v7.Consistent with the other artifact upload changes in this workflow.
186-191: Screenshot and coverage artifacts upload upgraded to v7.All three artifact upload steps in this workflow are now consistently using v7.
.github/workflows/codeql.yml (2)
65-74: CodeQL init action upgraded to v4.32.4.The configuration remains unchanged. The upgrade is consistent across all CodeQL action steps.
77-77: CodeQL autobuild action upgraded consistently..github/workflows/gdunit4_tests.yml (1)
54-58: Upgrade toactions/upload-artifact@v7is safe for this workflow.The v7 release introduced a new
archiveinput for unzipped uploads, but this workflow doesn't set it, so the default zipping behavior is preserved. The standard inputs (name,path,retention-days) remain fully compatible with v7, and the glob patternreports/**will be zipped as expected.
…als.log_message even when Globals is invalid, and the error string has a stray quote. n the else branch you’re still calling Globals.log_message when is_instance_valid(Globals) is false, which means Globals may be freed or uninitialized and can cause a crash. Either return early when Globals is invalid, or only call Globals.log_message when it’s valid and fall back to push_error/print_error otherwise. Also, the error message has mismatched quotes: "'Globals.settings' resource is NULL'" (extra trailing ').
The comment states v4.32.2, but the SHA 0ec47d036c68ae0cf94c629009b1029407111281 does not correspond to v4.32.2 or v4.32.4. This commit is part of v4.32.5 (released Mar 2, 2026). Meanwhile, codeql.yml uses v4.32.4 explicitly, creating a version mismatch. Either pin trivy.yml and snyk.yml to match the v4.32.4 tag used in codeql.yml, or update all workflows to use the same release version consistently.
…tings' of https://github.com/ikostan/SkyLockAssault into parse-error-could-not-resolve-external-class-member-settings
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/no_error_logs_test.py (1)
58-58: Fail fast on explicit init failure state, not only success state.At Line 58, waiting only on
window.godotInitializedcan turn known startup failures into timeout-style failures. Consider waiting for either success or the error UI state, then assert success for clearer diagnostics.💡 Proposed refinement
- page.wait_for_function("() => window.godotInitialized", timeout=5000) + page.wait_for_function( + "() => Boolean(window.godotInitialized) || " + "document.getElementById('retry-button')?.style.display === 'block'", + timeout=5000, + ) + assert page.evaluate("() => Boolean(window.godotInitialized)"), ( + "Godot failed to initialize before entering error state. " + "See captured console artifacts." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/no_error_logs_test.py` at line 58, Replace the single success-only wait_for_function call so the test waits for either the success flag or an explicit init-error state, then assert success (or fail with the error details) to get clearer diagnostics; specifically, change the wait to observe window.godotInitialized OR an error signal (e.g., window.godotInitError or a visible error DOM node like document.querySelector('.init-error')) using page.wait_for_function, then after the wait check window.godotInitialized and if false extract and include the init error (window.godotInitError or the error element's textContent) in the test failure message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/no_error_logs_test.py`:
- Around line 81-83: The file writes for artifact/coverage outputs use platform
default encoding; update the open(...) calls that write artifacts (e.g., the
with open(f"artifacts/test_error_logs_console_{timestamp}.txt", "w") as f block)
and the similar coverage file writes to explicitly specify encoding="utf-8" and
keep mode "w" so they become open(..., "w", encoding="utf-8"); ensure every
place that writes test artifacts or coverage files uses this explicit encoding.
- Line 75: In the test_no_error_logs_after_load test, replace the explicit str()
call inside the f-string with the f-string conversion flag: change the print
statement that currently uses {str(e)} to use {e!s} (i.e., update the f-string
in the print for the caught exception in the test_no_error_logs_after_load
function).
---
Nitpick comments:
In `@tests/no_error_logs_test.py`:
- Line 58: Replace the single success-only wait_for_function call so the test
waits for either the success flag or an explicit init-error state, then assert
success (or fail with the error details) to get clearer diagnostics;
specifically, change the wait to observe window.godotInitialized OR an error
signal (e.g., window.godotInitError or a visible error DOM node like
document.querySelector('.init-error')) using page.wait_for_function, then after
the wait check window.godotInitialized and if false extract and include the init
error (window.godotInitError or the error element's textContent) in the test
failure message.
ℹ️ Review info
Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5b421554-ec49-4192-a1cf-efe18ab85bbe
📒 Files selected for processing (1)
tests/no_error_logs_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 context used
🪛 Ruff (0.15.2)
tests/no_error_logs_test.py
[warning] 75-75: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (1)
tests/no_error_logs_test.py (1)
43-44: Good listener registration timing.Attaching the console listener before navigation ensures startup-time console messages are not missed.
Replace {str(e)} with {e!s} on line 75 to use the idiomatic f-string conversion flag instead of explicit str() call.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/validate_clean_load_test.py (1)
46-50: Externalize URL/timeout to reduce CI flakinessLine 46 and Line 50 hardcode both endpoint and 5s timeout. This can fail in slower CI nodes or alternate host/port setups.
💡 Proposed refactor
+ base_url = os.getenv("E2E_BASE_URL", "http://localhost:8080") + timeout_ms = int(os.getenv("E2E_LOAD_TIMEOUT_MS", "15000")) page.goto( - "http://localhost:8080/index.html", wait_until="networkidle", timeout=5000 + f"{base_url}/index.html", wait_until="networkidle", timeout=timeout_ms ) # 2. Wait for the engine's ready signal - page.wait_for_function("() => window.godotInitialized", timeout=5000) + page.wait_for_function("() => window.godotInitialized", timeout=timeout_ms)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/validate_clean_load_test.py` around lines 46 - 50, Replace the hardcoded endpoint and timeout in the test's page.goto and page.wait_for_function calls by reading configurable values (e.g., TEST_URL and NAV_TIMEOUT) from environment variables or a pytest fixture; update the page.goto("http://localhost:8080/index.html", timeout=5000) usage to use TEST_URL and NAV_TIMEOUT and the page.wait_for_function("() => window.godotInitialized", timeout=5000) call to use NAV_TIMEOUT so CI can override URL/timeout on slow nodes or different host/port setups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/validate_clean_load_test.py`:
- Line 77: Replace the use of str(e) inside the f-string at the failing print
call with the f-string conversion flag {e!s} to satisfy Ruff RUF010 and keep
idiomatic formatting; locate the print(f"Load validation failed: {str(e)}")
statement (the exception variable e) and change it to print(f"Load validation
failed: {e!s}") so the exception is formatted via the conversion flag.
---
Nitpick comments:
In `@tests/validate_clean_load_test.py`:
- Around line 46-50: Replace the hardcoded endpoint and timeout in the test's
page.goto and page.wait_for_function calls by reading configurable values (e.g.,
TEST_URL and NAV_TIMEOUT) from environment variables or a pytest fixture; update
the page.goto("http://localhost:8080/index.html", timeout=5000) usage to use
TEST_URL and NAV_TIMEOUT and the page.wait_for_function("() =>
window.godotInitialized", timeout=5000) call to use NAV_TIMEOUT so CI can
override URL/timeout on slow nodes or different host/port setups.
ℹ️ Review info
Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6ab35e77-c30d-45b1-b75f-69ee8be886d4
📒 Files selected for processing (1)
tests/validate_clean_load_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 context used
🪛 Ruff (0.15.2)
tests/validate_clean_load_test.py
[warning] 77-77: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (1)
tests/validate_clean_load_test.py (1)
53-67: Critical log filtering is well-implementedThe Line 53–67 filter correctly combines console
errortype with known Godot startup failure signatures.
These writes currently rely on platform default encodings; Unicode log text can fail on some runners and obscure the original test failure.
At Line 77, prefer {e!s} over {str(e)} to satisfy Ruff RUF010 and keep formatting idiomatic.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
globals.gd, consider making the settings resource path configurable (e.g., via an exported path or a constant) so the hardcodedres://config_resources/default_settings.trescan be overridden without modifying code and to avoid regressions if the asset is moved again. - The two Playwright tests share very similar setup (console listener, navigation to
index.html, waiting forwindow.godotInitialized); extracting this into a small helper or fixture would reduce duplication and make future changes to the load flow easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `globals.gd`, consider making the settings resource path configurable (e.g., via an exported path or a constant) so the hardcoded `res://config_resources/default_settings.tres` can be overridden without modifying code and to avoid regressions if the asset is moved again.
- The two Playwright tests share very similar setup (console listener, navigation to `index.html`, waiting for `window.godotInitialized`); extracting this into a small helper or fixture would reduce duplication and make future changes to the load flow easier to maintain.
## Individual Comments
### Comment 1
<location path="scripts/settings.gd" line_range="390-395" />
<code_context>
match parts[0]:
"key":
- if parts[1].is_valid_int():
+ if parts.size() >= 2 and parts[1].is_valid_int():
var ev := InputEventKey.new()
ev.physical_keycode = parts[1].to_int()
- # Logic for combinations (Shift + Tab etc)
- if "shift" in parts:
- ev.shift_pressed = true
- if "ctrl" in parts:
- ev.ctrl_pressed = true
- if "alt" in parts: # NEW: Restore Alt
- ev.alt_pressed = true
- if "meta" in parts: # NEW: Restore Meta
- ev.meta_pressed = true
- return ev
+
+ if ev.physical_keycode != 0:
+ ev.shift_pressed = "shift" in parts
+ ev.ctrl_pressed = "ctrl" in parts
</code_context>
<issue_to_address>
**question (bug_risk):** Guarding modifiers behind `physical_keycode != 0` might unintentionally drop some deserialized key events.
Previously, any event with a valid `parts[1]` int was returned, even when `physical_keycode` was 0. With the new `physical_keycode != 0` guard, a serialized value like `key:0:shift` will now silently produce `null`. If `0` is invalid in this context, consider rejecting it explicitly (e.g., `to_int() > 0`) or treating it as malformed input so the behavior is clear. If `0` can be valid, the guard should be removed so those events still deserialize.
</issue_to_address>
### Comment 2
<location path="tests/no_error_logs_test.py" line_range="28" />
<code_context>
+from playwright.sync_api import Page
+
+
+def test_no_error_logs_after_load(page: Page) -> None:
+ """
+ E2E test to ensure zero console errors on initial load.
</code_context>
<issue_to_address>
**suggestion (testing):** Also capture `pageerror` events to catch uncaught exceptions that might not surface as console logs.
Currently this only inspects `page.on("console", ...)` messages. Some uncaught exceptions or promise rejections are only emitted via `page.on("pageerror")`, so the test could pass while the page is actually broken. Please also listen to `page.on("pageerror", ...)` (either merging into `logs` or a separate list) and include those in the failure assertion.
Suggested implementation:
```python
def test_no_error_logs_after_load(page: Page) -> None:
"""
E2E test to ensure zero console errors on initial load.
:param page: The Playwright page object.
:type page: Page
:rtype: None
"""
logs: list[dict[str, str]] = []
page_errors: list[str] = []
cdp_session = None
def on_console(msg) -> None:
"""Capture all console messages for inspection."""
def on_page_error(exc) -> None:
"""Capture uncaught exceptions and unhandled rejections."""
# Playwright passes the error object; convert to string for assertions / snapshots
page_errors.append(str(exc))
```
To fully implement the suggestion you also need to:
1. Register the new handler before navigation, alongside the console listener, e.g.:
- `page.on("pageerror", on_page_error)`
2. Update the final assertion block at the end of `test_no_error_logs_after_load` to also fail when `page_errors` is non-empty. For example:
- Either merge `page_errors` into your existing `logs` structure before asserting, or
- Add a separate assertion like `assert not page_errors, f"Page errors occurred: {page_errors}"`
3. If you perform any cleanup such as `page.off("console", on_console)` at the end of the test, also remove the `pageerror` listener with `page.off("pageerror", on_page_error)` to keep the test isolated.
</issue_to_address>
### Comment 3
<location path="tests/no_error_logs_test.py" line_range="55-61" />
<code_context>
+ )
+
+ # Navigate and wait for the game to initialize
+ page.goto(
+ "http://localhost:8080/index.html", wait_until="networkidle", timeout=5000
+ )
+ page.wait_for_function("() => window.godotInitialized", timeout=5000)
+
+ # Allow a short buffer for any delayed post-load errors
+ page.wait_for_timeout(1000)
+
+ # Filter for error logs
</code_context>
<issue_to_address>
**suggestion (testing):** Consider making the timeouts less brittle or configurable to avoid flaky failures on slower environments.
The `page.goto(..., timeout=5000)` and `page.wait_for_function(..., timeout=5000)` calls, plus the fixed `wait_for_timeout(1000)`, are likely to be fragile on slower CI or as the app grows, leading to intermittent test failures. Consider either increasing these limits, making them configurable (e.g., via env vars), or replacing the fixed timeout with polling for `godotInitialized` under a higher overall limit to keep tests reliable while still failing fast when initialization truly breaks.
Suggested implementation:
```python
# Navigate and wait for the game to initialize
# Timeouts are configurable via environment variables to avoid flakiness on slower environments
godot_init_timeout_ms = int(os.getenv("GODOT_INIT_TIMEOUT_MS", "15000"))
godot_post_init_buffer_ms = int(
os.getenv("GODOT_POST_INIT_BUFFER_MS", "2000")
)
page.goto(
"http://localhost:8080/index.html",
wait_until="networkidle",
timeout=godot_init_timeout_ms,
)
page.wait_for_function(
"() => window.godotInitialized",
timeout=godot_init_timeout_ms,
)
# Allow a short buffer for any delayed post-load errors
page.wait_for_timeout(godot_post_init_buffer_ms)
```
1. Ensure `os` is imported at the top of `tests/no_error_logs_test.py`, e.g.:
- Add `import os` alongside the other imports.
2. Optionally document the `GODOT_INIT_TIMEOUT_MS` and `GODOT_POST_INIT_BUFFER_MS` environment variables in your test README or CI configuration so other developers know how to adjust them for slower environments.
</issue_to_address>
### Comment 4
<location path="tests/validate_clean_load_test.py" line_range="53-62" />
<code_context>
+ page.wait_for_function("() => window.godotInitialized", timeout=5000)
+
+ # 3. Analyze captured logs for the specific patterns seen in the screenshot
+ critical_errors = [
+ log["text"]
+ for log in logs
+ if log["type"] == "error"
+ or any(
+ pattern in log["text"]
+ for pattern in [
+ "SCRIPT ERROR",
+ "Compile Error",
+ "Parse Error",
+ "Failed to load script",
+ "Uncaught (in promise)",
+ ]
+ )
+ ]
</code_context>
<issue_to_address>
**question (testing):** Clarify whether matching critical error patterns on non-error console types is intentional to avoid false positives.
Because the patterns are checked regardless of `log['type']`, any non-error log (e.g., debug output) that happens to contain one of these phrases will be treated as a critical error and fail the test. If you only want to flag genuine error-level messages, consider applying the pattern check only when `log['type'] == 'error'`, or add a brief comment stating that matching these patterns on non-error logs is intentional.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…might unintentionally drop some deserialized key events. Previously, any event with a valid parts[1] int was returned, even when physical_keycode was 0. With the new physical_keycode != 0 guard, a serialized value like key:0:shift will now silently produce null. If 0 is invalid in this context, consider rejecting it explicitly (e.g., to_int() > 0) or treating it as malformed input so the behavior is clear. If 0 can be valid, the guard should be removed so those events still deserialize.
Delete leftover commented-out print statements in tests/no_error_logs_test.py and tests/validate_clean_load_test.py. This is a simple cleanup to reduce clutter; no runtime behavior is changed.
… exceptions that might not surface as console logs.
Currently this only inspects page.on("console", ...) messages. Some uncaught exceptions or promise rejections are only emitted via page.on("pageerror"), so the test could pass while the page is actually broken. Please also listen to page.on("pageerror", ...) (either merging into logs or a separate list) and include those in the failure assertion.
…nfigurable to avoid flaky failures on slower environments. The page.goto(..., timeout=5000) and page.wait_for_function(..., timeout=5000) calls, plus the fixed wait_for_timeout(1000), are likely to be fragile on slower CI or as the app grows, leading to intermittent test failures. Consider either increasing these limits, making them configurable (e.g., via env vars), or replacing the fixed timeout with polling for godotInitialized under a higher overall limit to keep tests reliable while still failing fast when initialization truly breaks.
…tings' of https://github.com/ikostan/SkyLockAssault into parse-error-could-not-resolve-external-class-member-settings
…on non-error console types is intentional to avoid false positives. Because the patterns are checked regardless of log['type'], any non-error log (e.g., debug output) that happens to contain one of these phrases will be treated as a critical error and fail the test. If you only want to flag genuine error-level messages, consider applying the pattern check only when log['type'] == 'error', or add a brief comment stating that matching these patterns on non-error logs is intentional.
github-project-automation
bot
moved this from In Progress
to Done
in Sky Lock Assault Project
ikostan
deleted the
parse-error-could-not-resolve-external-class-member-settings
branch
8 tasks
8 tasks
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")
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
Improve robustness of settings loading and input event deserialization while tightening input remap UI safety and expanding browser-based E2E validation, alongside minor CI dependency updates.
Bug Fixes:
Enhancements:
CI:
Tests:
Summary by CodeRabbit
Bug Fixes
Chores
Tests
The AI bots in Pull Request #451 have contributed extensively across documentation, automation, and code quality. Here are their specific and most comprehensive contributions:
1. Sourcery AI (
sourcery-ai)Sourcery acts primarily as a high-level reviewer and documentarian for this PR.
Globalssettings initialization flow. This visualizes how the engine interacts withGameSettingsResourceand handles successful vs. failed loads.2. CodeRabbit (
coderabbit-ai)CodeRabbit serves as the technical auditor and logic validator.
tests/no_error_logs_test.py, it suggested a "fail fast" approach by checking for explicit initialization failure states rather than just successful ones.3. DeepSource (
deepsource-autofix)DeepSource acts as the automated gatekeeper for code style and standards.
4. Dependabot (
dependabot)Dependabot manages the infrastructure and security dependencies.
actions/upload-artifactfrom version 6 to 7 andgithub/codeql-actionfrom 4.32.3 to 4.32.4.