Conversation
Using the Godot Editor (Drag and Drop): Move to resources/: game_settings_resource.gd, audio_settings.gd, gameplay_settings.gd, audio_constants.gd. Move to entities/: player.gd, bullet.gd, weapon.gd. Move to system/: OSWrapper.gd, JavaScriptBridgeWrapper.gd, audio_web_bridge.gd. Move hud.gd and parallax_manager.gd into specific subdirectories to ensure the root only contains essential configuration if necessary. hud.gd → scripts/ui/hud.gd parallax_manager.gd → scripts/managers/parallax_manager.gd Additional Context: This should leave no loose .gd files in the base res://scripts/ directory.
Contributor
Reviewer's GuideCentralizes script and scene paths via a new GamePaths core utility and updates tests, docs, and references to use it while reflecting the new scripts folder structure (resources, entities, system, ui, core) with no loose scripts in the root scripts directory. Class diagram for new GamePaths core utilityclassDiagram
class GamePaths {
<<utility>>
+PLAYER String
+HUD String
+AUDIO_WEB_BRIDGE String
+INPUT_REMAP_BUTTON String
+GAMEPLAY_SETTINGS String
+SETTINGS String
+AUDIO_SETTINGS_SCENE String
+MAIN_SCENE String
+KEY_MAPPING_SCENE String
+GAMEPLAY_SETTINGS_SCENE String
+PAUSE_MENU_SCENE String
+OPTIONS_MENU_SCENE String
}
class Player {
}
class Hud {
}
class AudioWebBridge {
}
class InputRemapButton {
}
class GameplaySettingsMenu {
}
class SettingsCore {
}
class AudioSettingsScene {
}
class MainScene {
}
class KeyMappingScene {
}
class PauseMenuScene {
}
class OptionsMenuScene {
}
Player ..> GamePaths : uses path constants
Hud ..> GamePaths : uses path constants
AudioWebBridge ..> GamePaths : uses path constants
InputRemapButton ..> GamePaths : uses path constants
GameplaySettingsMenu ..> GamePaths : uses path constants
SettingsCore ..> GamePaths : uses path constants
AudioSettingsScene ..> GamePaths : uses path constants
MainScene ..> GamePaths : uses path constants
KeyMappingScene ..> GamePaths : uses path constants
PauseMenuScene ..> GamePaths : uses path constants
OptionsMenuScene ..> GamePaths : uses path constants
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
Apr 22, 2026
Contributor
📝 WalkthroughWalkthroughAdds a centralized GamePaths class and relocates many script files; updates scene ext_resource script paths, autoload script paths, a settings resource reference, documentation table entries, and numerous tests to use GamePaths constants. No functional logic changes beyond path centralization. Changes
Sequence Diagram(s)(Skipped — changes are path refactors and centralized constants without a new multi-component sequential flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@project.godot`:
- Around line 29-31: Tests, helper and docs still reference old script paths;
update the hardcoded preload()/load() calls and string literals in the listed
files to the new locations or use the autoload singletons defined in
project.godot. Specifically: in test_gameplay_settings_ui.gd,
test_gameplay_settings_lifecycle.gd, test_gameplay_settings_js.gd,
test_game_settings_resource.gd, and test_audio_web_bridge.gd replace
preload("res://scripts/audio_web_bridge.gd")/preload("res://scripts/gameplay_settings.gd")
with the new path or the autoload singleton (e.g., use AudioWebBridge or
preload("res://scripts/system/audio_web_bridge.gd") as defined by AudioWebBridge
in project.godot); in gut_test_helper.gd update the string literals
"res://scripts/player.gd" and "res://scripts/hud.gd" to their new locations or
reference the appropriate singleton/class; and update the table entries in
Development_Guide.md to match the new paths (AudioConstants ->
res://scripts/resources/audio_constants.gd, AudioManager ->
res://scripts/managers/audio_manager.gd, AudioWebBridge ->
res://scripts/system/audio_web_bridge.gd). Ensure all replacements use exact
symbols/names from project.godot so tests and docs point to valid files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be174320-e538-4d3a-8141-88616e9e8cbe
📒 Files selected for processing (26)
config_resources/default_settings.tresproject.godotscenes/Player.tscnscenes/bullet.tscnscenes/main_scene.tscnscenes/weapon.tscnscripts/entities/bullet.gdscripts/entities/bullet.gd.uidscripts/entities/player.gdscripts/entities/player.gd.uidscripts/entities/weapon.gdscripts/entities/weapon.gd.uidscripts/managers/parallax_manager.gdscripts/managers/parallax_manager.gd.uidscripts/resources/audio_constants.gdscripts/resources/audio_constants.gd.uidscripts/resources/game_settings_resource.gdscripts/resources/game_settings_resource.gd.uidscripts/system/JavaScriptBridgeWrapper.gdscripts/system/JavaScriptBridgeWrapper.gd.uidscripts/system/OSWrapper.gdscripts/system/OSWrapper.gd.uidscripts/system/audio_web_bridge.gdscripts/system/audio_web_bridge.gd.uidscripts/ui/hud.gdscripts/ui/hud.gd.uid
📜 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). (2)
- GitHub Check: GDUnit4 Unit Tests / unit-test
- GitHub Check: GUT Unit Tests / unit-test
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-04-10T00:07:55.427Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 534
File: scripts/player.gd:146-149
Timestamp: 2026-04-10T00:07:55.427Z
Learning: In `scripts/globals.gd` and `scripts/player.gd` (GDScript, Godot 4), `current_fuel` is intentionally treated as volatile session data and should NOT be saved to or loaded from `settings.cfg`. Only `max_fuel` (tank capacity) is a persistent setting. `current_fuel` is always reset to `max_fuel` unconditionally in `player._ready()`. Persisting `current_fuel` is considered an architectural mistake by the project maintainer (ikostan). Mid-run fuel state persistence is planned via a separate `SaveGameResource` in PR `#535`.
Applied to files:
scenes/Player.tscn
🔇 Additional comments (5)
scenes/weapon.tscn (1)
3-3: LGTM!Script path updated to new
entities/location; UID and ext_resource id preserved.scenes/Player.tscn (1)
3-3: LGTM!Path relocation to
scripts/entities/player.gdwith preserved UID.scenes/bullet.tscn (1)
3-3: LGTM!Bullet script path updated consistently with the entities reorg.
scenes/main_scene.tscn (1)
79-80: LGTM!HUD and parallax manager script paths updated to
scripts/ui/andscripts/managers/; UIDs and ext_resource ids preserved, so node bindings on lines 128 and 208 remain valid.config_resources/default_settings.tres (1)
5-5: LGTM!
game_settings_resource.gdpath updated toscripts/resources/; UID preserved so thescript_class="GameSettingsResource"andmetadata/_custom_type_scriptreferences remain consistent.
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Apr 23, 2026 6:47p.m. | Review ↗ |
| JavaScript | |
Apr 23, 2026 6:47p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
…rence old script locations.
The autoload updates are correct and singleton references work fine, but verification found multiple broken preload()/load() calls and string literals pointing to old paths:
Test files (5 refs): test_gameplay_settings_ui.gd, test_gameplay_settings_lifecycle.gd, test_gameplay_settings_js.gd, test_game_settings_resource.gd, test_audio_web_bridge.gd all use preload("res://scripts/gameplay_settings.gd") or preload("res://scripts/audio_web_bridge.gd")
Test helper (2 refs): gut_test_helper.gd has string literals "res://scripts/player.gd" and "res://scripts/hud.gd" that need updating
Documentation (2 refs): Development_Guide.md references old paths in tables
These need to be updated to reflect the new script locations.
Owner
Author
|
@sourcery-ai 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 left some high level feedback:
- The Development_Guide table lists
gameplay_settings.gdatres://scripts/ui/gameplay_settings.gdandadvanced_settings.gdatres://scripts/menus/advanced_settings.gd, but the tests now preloadres://scripts/ui/menus/gameplay_settings.gd; please align these documented paths with the actual folder structure so the guide matches the codebase. - In the Development_Guide table you’re using
<del>inside a pipe table with markdownlint disabled; double-check that this renders correctly in your primary docs viewer (GitHub, GitLab, etc.) and consider using a separate section or bullet list if the table layout becomes hard to read.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Development_Guide table lists `gameplay_settings.gd` at `res://scripts/ui/gameplay_settings.gd` and `advanced_settings.gd` at `res://scripts/menus/advanced_settings.gd`, but the tests now preload `res://scripts/ui/menus/gameplay_settings.gd`; please align these documented paths with the actual folder structure so the guide matches the codebase.
- In the Development_Guide table you’re using `<del>` inside a pipe table with markdownlint disabled; double-check that this renders correctly in your primary docs viewer (GitHub, GitLab, etc.) and consider using a separate section or bullet list if the table layout becomes hard to read.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The Development_Guide table lists gameplay_settings.gd at res://scripts/ui/gameplay_settings.gd and advanced_settings.gd at res://scripts/menus/advanced_settings.gd, but the tests now preload res://scripts/ui/menus/gameplay_settings.gd; please align these documented paths with the actual folder structure so the guide matches the codebase.
Owner
Author
Fixed |
Owner
Author
Won't fix |
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The core files reference table now mixes active and deleted entries using HTML
<del>tags; consider moving deprecated items into a separate "Deprecated" subsection/table to keep the main reference concise and easier to scan. - Several tests and helpers hardcode script paths (e.g., multiple references to
scripts/ui/menus/gameplay_settings.gdandscripts/entities/player.gd); centralizing these into shared constants or a small path helper would make future refactors like this less error-prone. - The Markdown lint suppression (
markdownlint-disable MD033) currently wraps the whole table block; if possible, narrow its scope to just the lines that need raw HTML to reduce the chance of unintentionally allowing other inline HTML later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The core files reference table now mixes active and deleted entries using HTML `<del>` tags; consider moving deprecated items into a separate "Deprecated" subsection/table to keep the main reference concise and easier to scan.
- Several tests and helpers hardcode script paths (e.g., multiple references to `scripts/ui/menus/gameplay_settings.gd` and `scripts/entities/player.gd`); centralizing these into shared constants or a small path helper would make future refactors like this less error-prone.
- The Markdown lint suppression (`markdownlint-disable MD033`) currently wraps the whole table block; if possible, narrow its scope to just the lines that need raw HTML to reduce the chance of unintentionally allowing other inline HTML later.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
Working on it. |
Owner
Author
Won't fix. |
…ass #583 Description: Currently, several tests and helper scripts hardcode script paths as string literals. For example, there are multiple hardcoded references to res://scripts/ui/menus/gameplay_settings.gd and res://scripts/entities/player.gd. This issue tracks the refactoring required to centralize these paths into a single source of truth. Why is this useful? Hardcoding script paths makes the codebase fragile. Centralizing these into shared constants or a small path helper makes future refactoring (like moving or renaming files) significantly less error-prone. By using a centralized class_name, we ensure that if a file is relocated, the path only needs to be updated in one place rather than doing a global search-and-replace across multiple test and helper files. It also provides better autocompletion and type safety within the Godot editor.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
Development_Guide.md, the use of<del>tags inside the markdown table (and disabling MD033) makes the table harder to read and may not render consistently; consider moving deprecated entries to a separate section or marking them as deprecated in plain text instead of using inline HTML strikethroughs. GamePathsis a good centralization point, but it currently only covers a subset of scripts/scenes; if the intent is to eliminate fragile string paths, consider incrementally adding remaining frequently used paths (e.g., resources likegame_settings_resource.gd) and updating callers to use the constants for consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Development_Guide.md`, the use of `<del>` tags inside the markdown table (and disabling MD033) makes the table harder to read and may not render consistently; consider moving deprecated entries to a separate section or marking them as deprecated in plain text instead of using inline HTML strikethroughs.
- `GamePaths` is a good centralization point, but it currently only covers a subset of scripts/scenes; if the intent is to eliminate fragile string paths, consider incrementally adding remaining frequently used paths (e.g., resources like `game_settings_resource.gd`) and updating callers to use the constants for consistency.
## Individual Comments
### Comment 1
<location path="files/docs/Development_Guide.md" line_range="125" />
<code_context>
+| <del>**Logic Observer** | <del>`res://scripts/globals.gd`</del> | <del>Connects to the resource to trigger centralized logging and `_save_settings()`.</del> |
</code_context>
<issue_to_address>
**issue (bug_risk):** The `<del>` tag in the first cell of this row is not closed, unlike the other cells.
The first cell starts a `<del>` tag but never closes it, unlike the other cells. Please wrap the full cell content in a matching pair, e.g. `| <del>**Logic Observer**</del> | ...`, to keep the markup consistent and avoid rendering 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.
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 (3)
files/docs/Development_Guide.md (2)
84-99:⚠️ Potential issue | 🟡 MinorObserver-pattern steps contradict the deleted table rows.
Two issues in this block:
- Line 90 still points to
scripts/game_settings_resource.gd, but the resource was relocated tores://scripts/resources/game_settings_resource.gd(as correctly reflected in the updated table on line 124).- Step 2 instructs updating
_save_settings()/_load_settings()inglobals.gd, yet the Logic Observer row forglobals.gdwas struck out in the table on line 125 (suggesting those responsibilities moved). Either the table deletion or this instruction is wrong — please reconcile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@files/docs/Development_Guide.md` around lines 84 - 99, Update the docs to use the correct resource path and reconcile who handles persistence: change the reference from scripts/game_settings_resource.gd to res://scripts/resources/game_settings_resource.gd in the "Define the Property" step, and then clarify whether _save_settings()/_load_settings() in globals.gd still manage persistence or if that responsibility moved to the resource (or another class) — update Step 2 to point to the actual current persistence owner (referencing _save_settings, _load_settings, globals.gd, and game_settings_resource.gd) and adjust the table entries so the text and table consistently reflect the single source of truth.
11-20:⚠️ Potential issue | 🟡 MinorStale paths in "Key Files" and outdated engine version.
This section was not updated to reflect the PR's file relocations and target engine:
- Line 11 says Godot v4.4, but the PR targets v4.5.
- Line 19 should reference
res://scripts/entities/player.gd.- Line 20 references
scripts/Globals.gd(wrong casing) and should be aligned with the actual autoload path (res://scripts/globals.gdor wherever it now lives post-refactor).Since this PR explicitly aims to keep documented paths in sync with code, please update this section too — otherwise only the "Core Files Reference" table is consistent.
📝 Proposed fix
-- **Engine**: Godot v4.4 (64-bit Windows compatible). +- **Engine**: Godot v4.5 (64-bit Windows compatible). @@ - - `scripts/main_menu.gd`: Handles signals, quit logic. - - `scripts/player.gd`: Top-down movement. - - `scripts/Globals.gd`: Global vars (e.g., logging). + - `scripts/main_menu.gd`: Handles signals, quit logic. + - `scripts/entities/player.gd`: Top-down movement. + - `scripts/globals.gd`: Global vars (e.g., logging).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@files/docs/Development_Guide.md` around lines 11 - 20, Update the "Key Files" list to match the PR refactor: change the Engine version from "Godot v4.4" to "Godot v4.5"; replace `scripts/player.gd` with its new path `res://scripts/entities/player.gd`; and fix the globals entry from `scripts/Globals.gd` to the autoload path `res://scripts/globals.gd` (lowercase) so the documented symbols project.godot, scenes/main_menu.tscn, scripts/main_menu.gd, res://scripts/entities/player.gd, and res://scripts/globals.gd reflect the repo structure and engine target.test/gut/test_preserve_other_sections.gd (1)
42-44:⚠️ Potential issue | 🟠 MajorInconsistency with the new guarded-reset pattern in
test_reset_scenarios.gd.
test_reset_scenarios.gd::before_each()now wraps the samecurrent_log_level/difficultyresets inGlobals._is_loading_settings = true/falseto suppress the auto_save_settings()side effect, but this file'sbefore_each()does not. Since the reactive handler inscripts/core/globals.gd::_on_setting_changedwill call_save_settings()on each assignment here, every test in this file will trigger disk I/O against the realSettings.CONFIG_PATHduring setup. Recommend applying the same guard for consistency and to avoid polluting the user's real settings file from the test suite.♻️ Proposed fix
- # Reset Globals settings - Globals.settings.current_log_level = Globals.LogLevel.INFO - Globals.settings.difficulty = 1.0 + # Reset Globals settings (guarded to prevent automatic _save_settings() to disk) + Globals._is_loading_settings = true + Globals.settings.current_log_level = Globals.LogLevel.INFO + Globals.settings.difficulty = 1.0 + Globals._is_loading_settings = false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_preserve_other_sections.gd` around lines 42 - 44, The test resets Globals.settings.current_log_level and Globals.settings.difficulty directly in before_each(), which triggers scripts/core/globals.gd::_on_setting_changed and causes _save_settings() disk I/O; wrap those assignments with Globals._is_loading_settings = true before the changes and set it back to false after to suppress the auto-save (mirror the pattern used in test_reset_scenarios.gd::before_each()), ensuring current_log_level and difficulty are set while loading is guarded so tests do not write to Settings.CONFIG_PATH.
🧹 Nitpick comments (10)
test/gut/test_input_remap_ec.gd (1)
9-9: Nit: align the typing style with the sibling test file.
test/gut/test_input_remap_button_device_aware.gddeclares this asconst InputRemapButton: Script = preload(...). Matching that here keeps the type annotation consistent across the GUT suite. Behavior is identical.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_input_remap_ec.gd` at line 9, Update the constant declaration for InputRemapButton to include the same Godot type annotation used in the sibling test (i.e., declare const InputRemapButton: Script = preload(...)) so it matches test/gut/test_input_remap_button_device_aware.gd; modify the preload line for the InputRemapButton symbol accordingly to add the : Script type annotation.test/gut/test_sfx_volume_control.gd (1)
11-11: Nit: preferpreloadfor a static scene reference.Other tests in this PR use
preload(GamePaths.MAIN_SCENE)for scene constants. Hereload()works but defers resolution to script-init time (slightly slower, error surfaces later). For consistency:-var audio_scene: PackedScene = load(GamePaths.AUDIO_SETTINGS_SCENE) +var audio_scene: PackedScene = preload(GamePaths.AUDIO_SETTINGS_SCENE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_sfx_volume_control.gd` at line 11, Replace the dynamic load call with a compile-time preload for the static scene reference: change the declaration of the audio_scene variable that currently uses load(GamePaths.AUDIO_SETTINGS_SCENE) to use preload(GamePaths.AUDIO_SETTINGS_SCENE) so the PackedScene is resolved at script-load time (match other tests that use preload(GamePaths.MAIN_SCENE)); update the variable declaration named audio_scene to use preload accordingly.files/docs/Development_Guide.md (1)
121-129: Table updates look correct; consider a cleaner alternative to<del>in a pipe table.Wrapping row cells in
<del>within a Markdown pipe table is fragile (some renderers mis-handle HTML-inside-table-cells, and it's the reason MD033 had to be disabled). If the intent is to mark deprecated rows, a note column ("Status: deprecated") or a separate "Removed / deprecated entries" sub-list would render more reliably across GitHub/docs renderers and avoid needing themarkdownlint-disable MD033block entirely.Non-blocking — surfacing since the PR discussion already raised markdownlint-suppression scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@files/docs/Development_Guide.md` around lines 121 - 129, Replace the fragile <del> HTML in the pipe table by either adding a "Status" column with values like "deprecated" for rows such as "Logic Observer" (`res://scripts/globals.gd`) and "Persistence Settings" (`res://scripts/settings.gd`), or move those entries into a separate "Removed / deprecated entries" subsection below the table; then remove the markdownlint-disable MD033/enable comments so the table renders cleanly across renderers and lints normally.scripts/core/game_paths.gd (2)
10-52: Path coverage is partial — several relocated files are still missing fromGamePaths.The PR relocates many files (bullet.gd, weapon.gd, audio_settings.gd, gameplay_settings_resource.gd, audio_constants.gd, OSWrapper.gd, JavaScriptBridgeWrapper.gd, parallax_manager.gd, etc.), but
GamePathsonly captures a subset. If the goal is "single source of truth to reduce future refactor brittleness", anyres://...literal that still lives in tests/helpers will erode that guarantee the next time those files move.Consider adding constants for at least the files this PR just relocated, e.g.:
const BULLET: String = "res://scripts/entities/bullet.gd" const WEAPON: String = "res://scripts/entities/weapon.gd" const OS_WRAPPER: String = "res://scripts/system/OSWrapper.gd" const JS_BRIDGE_WRAPPER: String = "res://scripts/system/JavaScriptBridgeWrapper.gd" const AUDIO_CONSTANTS: String = "res://scripts/resources/audio_constants.gd" const AUDIO_SETTINGS_RESOURCE: String = "res://scripts/resources/audio_settings.gd" const GAME_SETTINGS_RESOURCE: String = "res://scripts/resources/game_settings_resource.gd" const PARALLAX_MANAGER: String = "res://scripts/managers/parallax_manager.gd"Also, a grouping convention (e.g.,
PATH_SCRIPT_*/PATH_SCENE_*or nested classes) can prevent name collisions as the set grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/core/game_paths.gd` around lines 10 - 52, GamePaths currently defines only a subset of relocated resource paths (see constants like PLAYER, HUD, AUDIO_WEB_BRIDGE, GAMEPLAY_SETTINGS_SCENE) so add missing constants for the files moved in this PR (e.g., BULLET, WEAPON, OS_WRAPPER, JS_BRIDGE_WRAPPER, AUDIO_CONSTANTS, AUDIO_SETTINGS_RESOURCE, GAME_SETTINGS_RESOURCE, PARALLAX_MANAGER) using the same String const pattern as existing entries to ensure tests/helpers use these central values; also apply a simple naming/grouping convention (e.g., prefix script constants with SCRIPT_ or group scene constants) to avoid future collisions and keep the file organized.
7-8: Minor:RefCountedis unnecessary for a constants-only container.Since the class is never instantiated and only exposes
consts, extendingObject(or keepingRefCounted— both are fine) is equivalent, but it may be worth a short docstring note that instances shouldn't be constructed. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/core/game_paths.gd` around lines 7 - 8, GamePaths is a constants-only container but currently extends RefCounted; change it to not imply instantiation by either replacing "extends RefCounted" with "extends Object" (or removing the extends line) and/or add a short class docstring/note above the class_name stating that this class only holds consts and should not be instantiated; reference the GamePaths class and its const members when making the change.test/gut/test_pause_menu.gd (1)
37-40: Optional: consider dropping the explanatory comment once the decision is stable.The note about removed
stub()calls is useful context now, but long-term it may become stale. If you want to preserve the rationale, a brief one-liner or a link to the discussion is typically enough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_pause_menu.gd` around lines 37 - 40, Replace the multi-line explanatory comment about removed stub() calls in test/gut/test_pause_menu.gd with a concise one-liner (or remove it entirely) that states: "Removed stub() calls because GUT cannot stub Autoload instances and these methods aren't used by PM-01–PM-05 pause/resume flows." Keep the reference to stub(), Autoload, and the PM-01..PM-05 flows so future readers understand the rationale without a long, potentially stale paragraph.test/gut/gut_test_helper.gd (1)
63-66: Minor: silently skipping HUD script assignment may mask a real failure.If
load(GamePaths.HUD)returnsnull(e.g., path regressed during a future move), the mock panel will be built without its HUD script and tests that exercise HUD behavior will fail with confusing downstream errors rather than a clear setup failure. Consider asserting/pushing an error instead of the silentif hud_script:fallback.Proposed tweak
- var hud_script := load(GamePaths.HUD) - if hud_script: - panel.set_script(hud_script) + var hud_script: Script = load(GamePaths.HUD) + assert(hud_script != null, "Failed to load HUD script from " + GamePaths.HUD) + panel.set_script(hud_script)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/gut_test_helper.gd` around lines 63 - 66, The code silently skips assigning the HUD script when load(GamePaths.HUD) returns null; change the setup in the test helper to fail fast by checking hud_script and raising/asserting if it's null so tests don't proceed with a misconfigured mock. Specifically, after calling var hud_script := load(GamePaths.HUD), replace the silent if hud_script: panel.set_script(hud_script) behavior with an explicit assertion or error (using assert, push_error, or a test framework fail) that references hud_script and GamePaths.HUD before calling panel.set_script to guarantee the HUD script is present.test/gut/test_master_volume_control_and_music.gd (2)
11-11: Same optional suggestion as intest_reset_scenarios.gd.Prefer instantiating
audio_sceneinsidebefore_each()rather than at script scope to avoid sharing a singlePackedSceneacross tests and to decouple script load from resource availability. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_master_volume_control_and_music.gd` at line 11, Move instantiation of the audio scene into the test setup by creating/assigning audio_scene inside before_each() instead of at script scope: replace the top-level PackedScene load of audio_scene with a local/instance assignment performed in the before_each() function so each test gets its own instance and the resource load is decoupled from script evaluation (refer to audio_scene and before_each()).
354-362: Good fix for the orphanControl.Wrapping
Control.new()inautofree()correctly handles the lifetime of a node that’s never added to the tree and would otherwise leak. Rationale comment is helpful.One residual concern unrelated to this change: on line 362,
audio_instance.queue_free()is invoked while the node is also tracked byadd_child_autofree()frombefore_each().queue_free()is idempotent in practice, but relying on the double-free being a no-op is fragile — consider removing the explicitqueue_free()here and letting autofree handle it, or detaching from autofree tracking. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_master_volume_control_and_music.gd` around lines 354 - 362, The test currently calls audio_instance.queue_free() while the same node is already managed by add_child_autofree() in before_each(), which risks fragile double-free assumptions; remove the explicit audio_instance.queue_free() call and let GUT's add_child_autofree()/autofree lifecycle management handle cleanup, keeping the new autofree(Control.new()) for prev_menu as-is (references: audio_instance, add_child_autofree, before_each, autofree, prev_menu, queue_free).test/gut/test_reset_scenarios.gd (1)
12-12: Minor: consider moving theload()intobefore_each().Loading the
PackedSceneat script scope runs during test-script load (before GUT fully sets up), couples test-file parsing to resource availability, and shares a singlePackedSceneacross all tests. Moving it intobefore_each()(or lazy-initializing it) isolates failures per test and matches how the scene is consumed. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_reset_scenarios.gd` at line 12, The test loads a PackedScene at script scope via the audio_scene variable which runs during test-file load; move the load into the GUT before_each() hook (or lazy-initialize audio_scene inside before_each()) so each test gets its own PackedScene and resource failures are isolated per-test; update references to audio_scene in tests to use the instance set in before_each() and remove the top-level load() call.
🤖 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_preserve_other_sections.gd`:
- Around line 99-116: Update the misleading inline comment at the end of the
test to accurately state what's being asserted: instead of saying "Globals
unchanged (not loaded here)", clarify that the test asserts
Globals.settings.current_log_level and Globals.settings.difficulty remain the
values just set while Globals._is_loading_settings was true (DEBUG and 2.0) and
were not modified by AudioManager.load_volumes() /
AudioManager.apply_all_volumes(); reference the Globals._is_loading_settings
guard and the assertions that follow to ensure the comment clearly ties to those
symbols.
---
Outside diff comments:
In `@files/docs/Development_Guide.md`:
- Around line 84-99: Update the docs to use the correct resource path and
reconcile who handles persistence: change the reference from
scripts/game_settings_resource.gd to
res://scripts/resources/game_settings_resource.gd in the "Define the Property"
step, and then clarify whether _save_settings()/_load_settings() in globals.gd
still manage persistence or if that responsibility moved to the resource (or
another class) — update Step 2 to point to the actual current persistence owner
(referencing _save_settings, _load_settings, globals.gd, and
game_settings_resource.gd) and adjust the table entries so the text and table
consistently reflect the single source of truth.
- Around line 11-20: Update the "Key Files" list to match the PR refactor:
change the Engine version from "Godot v4.4" to "Godot v4.5"; replace
`scripts/player.gd` with its new path `res://scripts/entities/player.gd`; and
fix the globals entry from `scripts/Globals.gd` to the autoload path
`res://scripts/globals.gd` (lowercase) so the documented symbols project.godot,
scenes/main_menu.tscn, scripts/main_menu.gd, res://scripts/entities/player.gd,
and res://scripts/globals.gd reflect the repo structure and engine target.
In `@test/gut/test_preserve_other_sections.gd`:
- Around line 42-44: The test resets Globals.settings.current_log_level and
Globals.settings.difficulty directly in before_each(), which triggers
scripts/core/globals.gd::_on_setting_changed and causes _save_settings() disk
I/O; wrap those assignments with Globals._is_loading_settings = true before the
changes and set it back to false after to suppress the auto-save (mirror the
pattern used in test_reset_scenarios.gd::before_each()), ensuring
current_log_level and difficulty are set while loading is guarded so tests do
not write to Settings.CONFIG_PATH.
---
Nitpick comments:
In `@files/docs/Development_Guide.md`:
- Around line 121-129: Replace the fragile <del> HTML in the pipe table by
either adding a "Status" column with values like "deprecated" for rows such as
"Logic Observer" (`res://scripts/globals.gd`) and "Persistence Settings"
(`res://scripts/settings.gd`), or move those entries into a separate "Removed /
deprecated entries" subsection below the table; then remove the
markdownlint-disable MD033/enable comments so the table renders cleanly across
renderers and lints normally.
In `@scripts/core/game_paths.gd`:
- Around line 10-52: GamePaths currently defines only a subset of relocated
resource paths (see constants like PLAYER, HUD, AUDIO_WEB_BRIDGE,
GAMEPLAY_SETTINGS_SCENE) so add missing constants for the files moved in this PR
(e.g., BULLET, WEAPON, OS_WRAPPER, JS_BRIDGE_WRAPPER, AUDIO_CONSTANTS,
AUDIO_SETTINGS_RESOURCE, GAME_SETTINGS_RESOURCE, PARALLAX_MANAGER) using the
same String const pattern as existing entries to ensure tests/helpers use these
central values; also apply a simple naming/grouping convention (e.g., prefix
script constants with SCRIPT_ or group scene constants) to avoid future
collisions and keep the file organized.
- Around line 7-8: GamePaths is a constants-only container but currently extends
RefCounted; change it to not imply instantiation by either replacing "extends
RefCounted" with "extends Object" (or removing the extends line) and/or add a
short class docstring/note above the class_name stating that this class only
holds consts and should not be instantiated; reference the GamePaths class and
its const members when making the change.
In `@test/gut/gut_test_helper.gd`:
- Around line 63-66: The code silently skips assigning the HUD script when
load(GamePaths.HUD) returns null; change the setup in the test helper to fail
fast by checking hud_script and raising/asserting if it's null so tests don't
proceed with a misconfigured mock. Specifically, after calling var hud_script :=
load(GamePaths.HUD), replace the silent if hud_script:
panel.set_script(hud_script) behavior with an explicit assertion or error (using
assert, push_error, or a test framework fail) that references hud_script and
GamePaths.HUD before calling panel.set_script to guarantee the HUD script is
present.
In `@test/gut/test_input_remap_ec.gd`:
- Line 9: Update the constant declaration for InputRemapButton to include the
same Godot type annotation used in the sibling test (i.e., declare const
InputRemapButton: Script = preload(...)) so it matches
test/gut/test_input_remap_button_device_aware.gd; modify the preload line for
the InputRemapButton symbol accordingly to add the : Script type annotation.
In `@test/gut/test_master_volume_control_and_music.gd`:
- Line 11: Move instantiation of the audio scene into the test setup by
creating/assigning audio_scene inside before_each() instead of at script scope:
replace the top-level PackedScene load of audio_scene with a local/instance
assignment performed in the before_each() function so each test gets its own
instance and the resource load is decoupled from script evaluation (refer to
audio_scene and before_each()).
- Around line 354-362: The test currently calls audio_instance.queue_free()
while the same node is already managed by add_child_autofree() in before_each(),
which risks fragile double-free assumptions; remove the explicit
audio_instance.queue_free() call and let GUT's add_child_autofree()/autofree
lifecycle management handle cleanup, keeping the new autofree(Control.new()) for
prev_menu as-is (references: audio_instance, add_child_autofree, before_each,
autofree, prev_menu, queue_free).
In `@test/gut/test_pause_menu.gd`:
- Around line 37-40: Replace the multi-line explanatory comment about removed
stub() calls in test/gut/test_pause_menu.gd with a concise one-liner (or remove
it entirely) that states: "Removed stub() calls because GUT cannot stub Autoload
instances and these methods aren't used by PM-01–PM-05 pause/resume flows." Keep
the reference to stub(), Autoload, and the PM-01..PM-05 flows so future readers
understand the rationale without a long, potentially stale paragraph.
In `@test/gut/test_reset_scenarios.gd`:
- Line 12: The test loads a PackedScene at script scope via the audio_scene
variable which runs during test-file load; move the load into the GUT
before_each() hook (or lazy-initialize audio_scene inside before_each()) so each
test gets its own PackedScene and resource failures are isolated per-test;
update references to audio_scene in tests to use the instance set in
before_each() and remove the top-level load() call.
In `@test/gut/test_sfx_volume_control.gd`:
- Line 11: Replace the dynamic load call with a compile-time preload for the
static scene reference: change the declaration of the audio_scene variable that
currently uses load(GamePaths.AUDIO_SETTINGS_SCENE) to use
preload(GamePaths.AUDIO_SETTINGS_SCENE) so the PackedScene is resolved at
script-load time (match other tests that use preload(GamePaths.MAIN_SCENE));
update the variable declaration named audio_scene to use preload accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c9dbc56a-6f58-4fbb-af8a-795a369c7ae8
📒 Files selected for processing (37)
files/docs/Development_Guide.mdscripts/core/game_paths.gdscripts/core/game_paths.gd.uidtest/gut/gut_test_helper.gdtest/gut/test_audio_reset_button.gdtest/gut/test_audio_web_bridge.gdtest/gut/test_blank_key_labels_on_missing_config.gdtest/gut/test_decor_layer_transformations.gdtest/gut/test_deduplication_on_device_switch.gdtest/gut/test_deduplication_on_rapid_remap.gdtest/gut/test_deduplication_on_remap.gdtest/gut/test_deduplication_on_reset.gdtest/gut/test_fuel_additional_edge_cases.gdtest/gut/test_fuel_integration.gdtest/gut/test_game_settings_resource.gdtest/gut/test_gameplay_settings_js.gdtest/gut/test_gameplay_settings_lifecycle.gdtest/gut/test_gameplay_settings_ui.gdtest/gut/test_get_pause_binding_label_for_device.gdtest/gut/test_input_remap_button.gdtest/gut/test_input_remap_button_device_aware.gdtest/gut/test_input_remap_ec.gdtest/gut/test_integration_key_mapping.gdtest/gut/test_key_mapping_menu.gdtest/gut/test_key_mapping_menu_device_aware.gdtest/gut/test_main_scene_orphan_nodes.gdtest/gut/test_main_scene_parallax_and_performance.gdtest/gut/test_master_volume_control_and_music.gdtest/gut/test_pause_menu.gdtest/gut/test_player_lifecycle.gdtest/gut/test_preserve_other_sections.gdtest/gut/test_reset_scenarios.gdtest/gut/test_settings_unbound_scenarios.gdtest/gut/test_sfx_rotor_volume_control.gdtest/gut/test_sfx_volume_control.gdtest/gut/test_sfx_weapon_volume_control.gdtest/gut/test_version_display.gd
✅ Files skipped from review due to trivial changes (17)
- scripts/core/game_paths.gd.uid
- test/gut/test_fuel_integration.gd
- test/gut/test_decor_layer_transformations.gd
- test/gut/test_fuel_additional_edge_cases.gd
- test/gut/test_deduplication_on_reset.gd
- test/gut/test_get_pause_binding_label_for_device.gd
- test/gut/test_key_mapping_menu.gd
- test/gut/test_audio_web_bridge.gd
- test/gut/test_key_mapping_menu_device_aware.gd
- test/gut/test_deduplication_on_rapid_remap.gd
- test/gut/test_version_display.gd
- test/gut/test_audio_reset_button.gd
- test/gut/test_settings_unbound_scenarios.gd
- test/gut/test_input_remap_button.gd
- test/gut/test_deduplication_on_device_switch.gd
- test/gut/test_player_lifecycle.gd
- test/gut/test_blank_key_labels_on_missing_config.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 context used
🧠 Learnings (4)
📚 Learning: 2026-03-30T04:02:23.747Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 500
File: test/gut/test_audio_web_bridge.gd:131-145
Timestamp: 2026-03-30T04:02:23.747Z
Learning: In GUT (Godot Unit Test) for Godot 4, when using `assert_called` / `assert_called_count` with parameter matching, include *every* argument the mocked method accepts, including parameters with default values. GUT does not auto-fill default arguments during call matching. For example, if `JavaScriptBridgeWrapper.eval(script: String, global_exec: bool = false)` is invoked as `eval(js_string)`, the actual call recorded by GUT includes the default (`eval(js_string, false)`), so your assertion must match both arguments (e.g., `.bind(js_string, false)`, not `.bind(js_string)`). Apply this rule to GUT assertions in `test/gut` tests.
Applied to files:
test/gut/test_sfx_volume_control.gdtest/gut/test_deduplication_on_remap.gdtest/gut/test_input_remap_ec.gdtest/gut/test_input_remap_button_device_aware.gdtest/gut/test_gameplay_settings_js.gdtest/gut/test_reset_scenarios.gdtest/gut/test_sfx_weapon_volume_control.gdtest/gut/gut_test_helper.gdtest/gut/test_main_scene_parallax_and_performance.gdtest/gut/test_gameplay_settings_ui.gdtest/gut/test_pause_menu.gdtest/gut/test_master_volume_control_and_music.gdtest/gut/test_sfx_rotor_volume_control.gdtest/gut/test_game_settings_resource.gdtest/gut/test_preserve_other_sections.gdtest/gut/test_gameplay_settings_lifecycle.gdtest/gut/test_integration_key_mapping.gdtest/gut/test_main_scene_orphan_nodes.gd
📚 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:
test/gut/test_deduplication_on_remap.gdtest/gut/test_input_remap_ec.gdtest/gut/test_input_remap_button_device_aware.gdtest/gut/test_integration_key_mapping.gd
📚 Learning: 2026-04-10T00:07:55.427Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 534
File: scripts/player.gd:146-149
Timestamp: 2026-04-10T00:07:55.427Z
Learning: In `scripts/globals.gd` and `scripts/player.gd` (GDScript, Godot 4), `current_fuel` is intentionally treated as volatile session data and should NOT be saved to or loaded from `settings.cfg`. Only `max_fuel` (tank capacity) is a persistent setting. `current_fuel` is always reset to `max_fuel` unconditionally in `player._ready()`. Persisting `current_fuel` is considered an architectural mistake by the project maintainer (ikostan). Mid-run fuel state persistence is planned via a separate `SaveGameResource` in PR `#535`.
Applied to files:
test/gut/test_gameplay_settings_lifecycle.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:
test/gut/test_integration_key_mapping.gd
🔇 Additional comments (16)
test/gut/test_main_scene_orphan_nodes.gd (1)
14-14: LGTM — path centralization applied correctly.
preload(GamePaths.MAIN_SCENE)is valid becauseGamePaths.MAIN_SCENEis a compile-timeconst Stringfrom aclass_namescript. Both call sites (before_each and test_scene_reload_lifecycle) are consistent.test/gut/test_main_scene_parallax_and_performance.gd (1)
19-19: LGTM.Consistent with the other GUT tests migrating to
GamePaths.MAIN_SCENE.test/gut/test_input_remap_button_device_aware.gd (1)
11-13: LGTM.
const InputRemapButton: Script = preload(GamePaths.INPUT_REMAP_BUTTON)correctly re-uses the class-style alias sovar button: InputRemapButtonand the nested enum referenceInputRemapButton.DeviceType.KEYBOARDcontinue to resolve.test/gut/test_gameplay_settings_js.gd (1)
10-16: LGTM.Both the script (
GAMEPLAY_SETTINGS) and the scene (GAMEPLAY_SETTINGS_SCENE) are resolved viaGamePaths, keeping this test aligned with the PR-wide centralization.preloadfor the script andloadfor the scene is acceptable sincegameplay_menuis declared asControl(no compile-time type dependency on the scene).scripts/core/game_paths.gd (1)
30-30: No action needed. The constantSETTINGSat line 30 correctly referencesres://scripts/core/settings.gd, which exists and is actively used in the codebase without issues.> Likely an incorrect or invalid review comment.test/gut/test_deduplication_on_remap.gd (1)
11-11: LGTM — path centralization applied correctly.Preload now resolves
InputRemapButtonviaGamePaths.INPUT_REMAP_BUTTON, which matches the constant definition inscripts/core/game_paths.gd. No behavioral change.test/gut/test_integration_key_mapping.gd (1)
18-18: LGTM — consistent migration toGamePathsconstants.All three integration tests now resolve the key-mapping scene and
InputRemapButtonscript through centralized constants, eliminating the hardcoded literals without altering test flow.Also applies to: 95-95, 115-115, 167-167
test/gut/test_gameplay_settings_ui.gd (1)
10-10: LGTM —GamePathslookups are equivalent to previous literals.Both the script preload and the scene load use centralized constants; remaining UI setup and assertions are unchanged.
Also applies to: 19-19
test/gut/test_gameplay_settings_lifecycle.gd (1)
9-9: LGTM — all four resource lookups migrated consistently.The preload at class scope plus the three scene loads in
before_each,test_gs_life_08_web_overlay_cleanup, andtest_gs_life_09_unexpected_removal_restorationnow go throughGamePaths, matching the rest of the suite.Also applies to: 14-14, 59-59, 96-96
test/gut/test_sfx_weapon_volume_control.gd (2)
11-11: LGTM — scene load centralized.
audio_scenenow resolves throughGamePaths.AUDIO_SETTINGS_SCENE; behavior preserved.
283-293: Good catch wrappingprev_menuwithautofree().Prevents the orphan-node leak flagged by GUT for the non-parented
Controlused to simulate the previous menu. Matches the pattern used in the other audio tests.test/gut/test_pause_menu.gd (1)
14-14: LGTM —PauseMenuScenenow preloaded viaGamePaths.PAUSE_MENU_SCENE.test/gut/test_sfx_rotor_volume_control.gd (1)
11-11: LGTM — mirrors the weapon-volume test changes.
GamePaths.AUDIO_SETTINGS_SCENEis used for the scene load andprev_menuis nowautofree()-wrapped, matchingtest_sfx_weapon_volume_control.gdandtest_master_volume_control_and_music.gd.Also applies to: 283-293
test/gut/gut_test_helper.gd (1)
7-7: No action needed.GamePaths.PLAYERis a validclass_nameconstant that resolves correctly. Six test files successfully loadgut_test_helper.gdviapreload()and execute without parse errors, confirming that constant initialization works properly in this setup.test/gut/test_reset_scenarios.gd (1)
40-44: Guarded reset looks correct.Toggling
_is_loading_settingsaround theGlobals.settingsmutations matches the pattern inscripts/core/globals.gd::_load_settingsand correctly suppresses the auto-save/log side effects in_on_setting_changedduring the per-test reset.test/gut/test_game_settings_resource.gd (1)
13-13: Code verified — no issues found.
GamePathsis correctly declared as aclass_namewith aconst GAMEPLAY_SETTINGSmember (not an autoload), making it a compile-time constant suitable forpreload().
Owner
Author
Won't fix |
Owner
Author
|
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Owner
Author
A new task is created: [FEATURE] [Tech Debt] Complete GamePaths centralization across the codebase #584 |
….md so that the documentation accurately reflects this relationship Observer-pattern steps contradict the deleted table rows. Two issues in this block: Line 90 still points to scripts/game_settings_resource.gd, but the resource was relocated to res://scripts/resources/game_settings_resource.gd (as correctly reflected in the updated table on line 124). Step 2 instructs updating _save_settings() / _load_settings() in globals.gd, yet the Logic Observer row for globals.gd was struck out in the table on line 125 (suggesting those responsibilities moved). Either the table deletion or this instruction is wrong — please reconcile.
…://github.com/ikostan/SkyLockAssault into migrate-resources-entities-and-system-scripts
…ios.gd. Because of the Observer Pattern implemented in the architecture refactor, assigning values directly to Globals.settings.current_log_level and Globals.settings.difficulty during test setup in before_each() will immediately trigger _on_setting_changed(). This causes an unwanted _save_settings() disk I/O operation before the test even begins.
ikostan
changed the title
ikostan
added a commit
that referenced
this pull request
Apr 23, 2026
Having reviewed the details of Release v0.9.18 and PR #582, there are definitely a few key areas in your README.md that need to be updated to stay aligned with the current state of the project.
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.
🚀 PR Update: Path Centralization, Test Suite Stabilization, and Docs Alignment
Based on the code review feedback and subsequent CI runs, the following updates have been pushed to this PR:
1. Architectural Enhancements (
GamePathsImplementation)GamePathsClass: Introducedscripts/core/game_paths.gd(class_name GamePaths extends RefCounted) to act as a single source of truth for canonical script and scene paths."res://scripts/ui/menus/gameplay_settings.gd") in the newly organized test and helper files with type-safe constants (e.g.,GamePaths.GAMEPLAY_SETTINGS).GamePathsin future PRs to keep this PR's scope focused.2. Test Suite Stabilization (100% Pass Rate & 0 Orphans)
Control.new()nodes in GUT'sautofree()method acrosstest_master_volume_control_and_music.gd,test_sfx_rotor_volume_control.gd, andtest_sfx_weapon_volume_control.gd. This successfully eliminated the orphan node warnings during tree exit simulation tests (Test 15s).stub()calls on the activeGlobalsAutoload node intest_pause_menu.gd, clearing out the 15+ GUT runtime errors.test_preserve_other_sections.gdandtest_reset_scenarios.gdcaused by the new Observer Pattern. Guarded direct assignments toGlobals.settingswith theGlobals._is_loading_settingsflag to preventsetting_changedsignals from triggering automaticConfigFilesaves to disk during automated tests.3. Documentation Corrections
Development_Guide.md: Updated the architecture guide to accurately reflect the newly relocatedres://scripts/resources/game_settings_resource.gd.GameSettingsResourceholds the state and emits signals,globals.gdremains the exclusive owner of persistence (_save_settings/_load_settings).All CI checks are now green, memory leaks are resolved, and the codebase aligns with the new Phase 4 directory structure. Ready to merge!
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
Reorganize script resources, entities, and system scripts into dedicated subdirectories and update scene and project references accordingly.
Enhancements:
Chores:
Summary by Sourcery
Centralize script and scene paths into a new GamePaths core helper and update references across tests and docs while aligning documentation with the new folder structure.
Enhancements:
Tests:
Summary by CodeRabbit
Here is a summary of @ikostan's core contributions to Pull Request #582:
1. Architectural Refactoring & Folder Reorganization
Executed "Refactor Phase 4" to eliminate the "junk drawer" effect at the root of the
scripts/directory by meticulously categorizing and migrating files:game_settings_resource.gd,audio_settings.gd,gameplay_settings.gd,audio_constants.gd) toscripts/resources/.player.gd,bullet.gd,weapon.gd) toscripts/entities/.OSWrapper.gd,JavaScriptBridgeWrapper.gd,audio_web_bridge.gd) toscripts/system/.hud.gdtoscripts/ui/andparallax_manager.gdtoscripts/managers/.2. Implementation of Centralized Path Management
GamePathscore helper (scripts/core/game_paths.gd) to serve as a single source of truth for canonical script and scene paths.GamePathsconstants, significantly improving the maintainability of the codebase.3. Test Suite Stabilization & Bug Fixes
test_master_volume_control_and_music.gd,test_sfx_rotor_volume_control.gd,test_sfx_weapon_volume_control.gd) by correctly applying GUT'sautofree()method to simulated UI controls.Globals.settingsmodifications with the_is_loading_settingsflag intest_preserve_other_sections.gdandtest_reset_scenarios.gd. This prevents the newly implemented Observer Pattern from triggering unintended_save_settings()disk writes during automated testing.test_pause_menu.gdby removing invalidstub()calls on the activeGlobalsAutoload node.4. Documentation Updates
Development_Guide.mdto accurately reflect the new folder structure (e.g.,res://scripts/resources/game_settings_resource.gd).GameSettingsResourceholds the state,globals.gdhandles the actualConfigFilesaving and loading.Here is a summary of the automated AI and bot contributions for Pull Request #582:
GamePathsutility. It also conducted a high-level code review, flagging outdated paths inDevelopment_Guide.mdand warning about potential markdown rendering issues with HTML tags inside pipe tables.Development_Guide.mdoutside of the immediate diff range, specifically catching that the guide still referenced the oldgame_settings_resource.gdpath and contained conflicting instructions regarding persistence responsibilities.