Enable Keyboard & D-Pad Navigation for Main menu #390
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add full keyboard navigation support to the main menu in SkyLockAssault so users can navigate all menu items and interactive controls using only the keyboard (e.g., arrow keys and Enter/Space). This improves accessibility, usability in web exports, and testability within the Godot editor.
ikostan
added this to the
Milestone 11: Polish & Platform-wide Enhancements (Menus, Testing & Configuration) milestone
Contributor
Reviewer's GuideImproves main menu keyboard/D-Pad accessibility by reliably setting and restoring button focus around the intro fade and quit dialog, and updates Playwright web tests to invoke Godot menu actions via JavaScript callbacks instead of DOM clicks and legacy arguments. Sequence diagram for main menu fade-in and initial keyboard focussequenceDiagram
participant MainMenu as MainMenuScript
participant Tween as panel_tween
participant StartButton as start_button
participant Globals as GlobalsLogger
MainMenu->>Tween: tween_property(menu, modulate:a, 0.0 -> 1.0)
alt Tween is valid
MainMenu->>Globals: log_message(Waiting for fade-in tween to finish.)
MainMenu->>Tween: await finished
Tween-->>MainMenu: finished
MainMenu->>Globals: log_message(Fade-in complete—granting focus.)
else Tween is null or invalid
MainMenu->>Globals: log_message(Invalid tween—grabbing focus immediately.)
end
MainMenu->>StartButton: call_deferred(grab_focus)
Sequence diagram for quit dialog cancel and focus restorationsequenceDiagram
actor Player
participant MainMenu as MainMenuScript
participant QuitButton as quit_button
participant QuitDialog as quit_dialog
participant Globals as GlobalsLogger
Player->>QuitButton: activate (keyboard or DPad)
QuitButton->>MainMenu: pressed signal
MainMenu->>MainMenu: last_focused_button = quit_button
MainMenu->>QuitDialog: show()
MainMenu->>QuitDialog: popup_centered()
MainMenu->>Globals: log_message(Attempting to show QuitDialog.)
alt Player cancels via Cancel button or Esc
Player->>QuitDialog: cancel
QuitDialog-->>MainMenu: canceled signal
else Player closes via title bar close button
Player->>QuitDialog: close_requested
QuitDialog-->>MainMenu: close_requested signal
end
MainMenu->>QuitDialog: hide()
MainMenu->>Globals: log_message(Quit canceled.)
alt last_focused_button is valid
MainMenu->>last_focused_button: call_deferred(grab_focus)
MainMenu->>Globals: log_message(Restored focus to last_focused_button)
else last_focused_button invalid and quit_button valid
MainMenu->>QuitButton: call_deferred(grab_focus)
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces several StyleBoxEmpty entries with StyleBoxFlat and explicit visual properties in the main menu scene; adds tooltips and explicit focus navigation for Start/Options/Quit buttons; _ready now awaits panel tween (or 2s timeout) before focusing Start; quit dialog opener is tracked and focus is restored after cancel. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Scene as MainMenuScene
participant Tween as PanelTween
participant UI as Buttons
participant Dialog as QuitDialog
Scene->>Tween: play fade-in
alt tween finishes before timeout
Tween-->>Scene: finished
else timeout (2s)
Tween-->>Scene: timeout
end
Scene->>UI: call_deferred(grab_focus on StartButton)
User->>UI: press QuitButton
UI->>Scene: _on_quit_pressed (store last_focused_button)
Scene->>Dialog: show quit dialog (modal)
User->>Dialog: cancel/close
Dialog-->>Scene: emit close_requested / canceled
Scene->>UI: call_deferred(grab_focus on last_focused_button or fallback QuitButton)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider using the more idiomatic
start_button.call_deferred("grab_focus")andquit_button.call_deferred("grab_focus")instead ofgrab_focus.call_deferred()to keep the style consistent with typical Godot usage. - You restore focus to the Quit button only on cancel; if the quit dialog can be closed in other non-quit ways (e.g., clicking outside or a close button), ensure those paths also restore focus to maintain consistent keyboard navigation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using the more idiomatic `start_button.call_deferred("grab_focus")` and `quit_button.call_deferred("grab_focus")` instead of `grab_focus.call_deferred()` to keep the style consistent with typical Godot usage.
- You restore focus to the Quit button only on cancel; if the quit dialog can be closed in other non-quit ways (e.g., clicking outside or a close button), ensure those paths also restore focus to maintain consistent keyboard navigation.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: 1
🤖 Fix all issues with AI agents
In `@scripts/main_menu.gd`:
- Around line 74-75: The focus is currently grabbed with
start_button.grab_focus.call_deferred() immediately, before the fade-in tween
completes; instead wait for the fade-in Tween to finish (connect to the Tween
completion signal or await the tween's completion/future) and then call
start_button.grab_focus() (no call_deferred), i.e., replace the deferred
grab_focus with code that runs after the fade-in Tween's
finished/tween_all_completed event.
🧹 Nitpick comments (1)
scenes/main_menu.tscn (1)
38-44: Three identical focusStyleBoxFlatresources could be consolidated into one.
StyleBoxFlat_tbmy8,StyleBoxFlat_jk1qb, andStyleBoxFlat_5dd4ihave identical properties (samebg_color, corner radii,shadow_size). You could reference a single shared sub-resource from all three buttons'theme_override_styles/focus, reducing duplication and making future style tweaks a single edit.Also applies to: 53-59, 68-74
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scenes/main_menu.tscnscripts/main_menu.gd
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (3)
scripts/main_menu.gd (1)
207-208: Good: focus is restored to the quit button after dialog cancellation.This correctly returns keyboard focus to the button that opened the dialog, maintaining a seamless keyboard-navigation experience.
scenes/main_menu.tscn (2)
140-155: Focus navigation and styling for StartButton look correct.Circular wrap (top→QuitButton, bottom→OptionsButton) provides a smooth keyboard loop. Tooltip, focus color, and focus style are all properly set.
157-172: OptionsButton and QuitButton focus configuration looks good.Focus neighbors, tooltips, focus colors, and styles are consistent with StartButton, completing the circular navigation chain.
Also applies to: 174-189
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Consider using the more idiomatic start_button.call_deferred("grab_focus") and quit_button.call_deferred("grab_focus") instead of grab_focus.call_deferred() to keep the style consistent with typical Godot usage.
You restore focus to the Quit button only on cancel; if the quit dialog can be closed in other non-quit ways (e.g., clicking outside or a close button), ensure those paths also restore focus to maintain consistent keyboard navigation.
Owner
Author
All addressed. |
ikostan
changed the title
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
_on_quit_dialog_canceledhandler always restores focus toquit_button, which may be surprising if the dialog can be opened from multiple entry points; consider tracking and restoring the button that originally opened the dialog instead of hard-codingquit_button. - Awaiting
panel_tween.finishedin_readyassumes the tween node and animation are always present and started; it may be safer to guard the await with a null/validity check or fall back to grabbing focus immediately if the tween is missing or not running.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_on_quit_dialog_canceled` handler always restores focus to `quit_button`, which may be surprising if the dialog can be opened from multiple entry points; consider tracking and restoring the button that originally opened the dialog instead of hard-coding `quit_button`.
- Awaiting `panel_tween.finished` in `_ready` assumes the tween node and animation are always present and started; it may be safer to guard the await with a null/validity check or fall back to grabbing focus immediately if the tween is missing or not running.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Awaiting panel_tween.finished in _ready assumes the tween node and animation are always present and started; it may be safer to guard the await with a null/validity check or fall back to grabbing focus immediately if the tween is missing or not running.
Awaiting panel_tween.finished in _ready assumes the tween node and animation are always present and started; it may be safer to guard the await with a null/validity check or fall back to grabbing focus immediately if the tween is missing or not running.
Owner
Author
All addressed. |
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:
- Consider removing the inline note
# ← ADD THIS: Remember the openerfrom_on_quit_pressed, as it reads like a review artifact rather than long-term code documentation. - The
last_focused_buttonproperty is implicitly nullable; you may want to declare it asvar last_focused_button: Button = null(orButton?if using typed GDScript) to make its lifecycle and nullability explicit. - The warning log in
_readywhen the tween is not running could be noisy in legitimate cases (e.g., instant transitions); consider lowering it to a debug log or gating it behind a more specific condition.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider removing the inline note `# ← ADD THIS: Remember the opener` from `_on_quit_pressed`, as it reads like a review artifact rather than long-term code documentation.
- The `last_focused_button` property is implicitly nullable; you may want to declare it as `var last_focused_button: Button = null` (or `Button?` if using typed GDScript) to make its lifecycle and nullability explicit.
- The warning log in `_ready` when the tween is not running could be noisy in legitimate cases (e.g., instant transitions); consider lowering it to a debug log or gating it behind a more specific condition.
## Individual Comments
### Comment 1
<location> `scripts/main_menu.gd:66-67` </location>
<code_context>
panel_tween.tween_property(menu, "modulate:a", 1.0, 1.0).set_ease(Tween.EASE_OUT).set_trans(
Tween.TRANS_QUAD
) # Smooth curve (eases out, quadratic)
+ # Wait only if tween is good and started
+ if panel_tween and panel_tween.is_valid() and panel_tween.is_running():
+ await panel_tween.finished
+ else:
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid relying on `is_running()` before `await` to prevent a subtle race with `finished`.
Because `is_running()` is checked before the `await`, there’s a window where the tween can complete after the check but before the await is registered, so `finished` will never be emitted again and this coroutine can hang. Consider removing the `is_running()` check and just validating the tween instance, or add a timeout/secondary guard so the await can’t block indefinitely if the tween already finished.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
SourceryAI
reviewed
Feb 11, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The inline comment
# ← ADD THIS: Remember the openerin_on_quit_pressedlooks like a review artifact; consider removing or rephrasing it to match the rest of the codebase’s comment style. - The new
Globals.log_messagecalls around focus restoration and tween fallback are quite verbose for a routine UI flow; consider downgrading or gating them behind a higher log level or a debug flag to avoid noisy logs in normal runs. - Since
last_focused_buttonis only assigned in_on_quit_pressed, you might want to initialize or update it in a more generic way (e.g., before opening any dialog) so the focus-restore logic can be reused if additional dialogs are added later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The inline comment `# ← ADD THIS: Remember the opener` in `_on_quit_pressed` looks like a review artifact; consider removing or rephrasing it to match the rest of the codebase’s comment style.
- The new `Globals.log_message` calls around focus restoration and tween fallback are quite verbose for a routine UI flow; consider downgrading or gating them behind a higher log level or a debug flag to avoid noisy logs in normal runs.
- Since `last_focused_button` is only assigned in `_on_quit_pressed`, you might want to initialize or update it in a more generic way (e.g., before opening any dialog) so the focus-restore logic can be reused if additional dialogs are added later.Hi @ikostan! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.Consider removing the inline note # ← ADD THIS: Remember the opener from _on_quit_pressed, as it reads like a review artifact rather than long-term code documentation.
The last_focused_button property is implicitly nullable; you may want to declare it as var last_focused_button: Button = null (or Button? if using typed GDScript) to make its lifecycle and nullability explicit.
…nt a subtle race with finished. Because is_running() is checked before the await, there’s a window where the tween can complete after the check but before the await is registered, so finished will never be emitted again and this coroutine can hang. Consider removing the is_running() check and just validating the tween instance, or add a timeout/secondary guard so the await can’t block indefinitely if the tween already finished.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@scripts/main_menu.gd`:
- Around line 117-122: The function promise_any incorrectly awaits the first
awaitable in a for-loop so it never races multiple signals; replace it by
removing promise_any and instead implement a true race at each call site (or
reimplement promise_any) by wiring all awaitables to a shared completion
signal/flag: create a temporary Object with a custom "completed" signal or a
boolean plus one-shot connections from each awaitable (e.g., tween.finished,
timer.timeout) that, when fired, disconnects the others and emits/sets the
shared completion, then await that shared completion and return the winning
awaitable; update callers to use that pattern (or inline the wiring) rather than
relying on the current promise_any implementation.
- Around line 237-244: The fallback branch calls
quit_button.call_deferred("grab_focus") without verifying quit_button is valid;
update the else path in the restore-focus logic to guard using
is_instance_valid(quit_button) before calling call_deferred on it (and
optionally log a debug/warning via Globals.log_message if both
last_focused_button and quit_button are invalid) so you never call into a freed
object; focus the change around the last_focused_button / quit_button checks in
this restore-focus block.
- Around line 66-82: The current race using promise_any([...]) is wrong and
timeout_timer.time_left is unsafe; replace that block by removing promise_any
and instead connect both panel_tween.finished and the SceneTreeTimer.timeout to
a single one-shot handler that logs which signal fired and then calls
start_button.call_deferred("grab_focus"). Specifically, if panel_tween and
panel_tween.is_valid(): create timeout_timer := get_tree().create_timer(2.0),
connect panel_tween.finished and timeout_timer.timeout to a local method (or use
a lambda via funcref) that is marked one_shot/auto-disconnect, inside the
handler log either "Tween finished normally." or "Tween await timed out—grabbing
focus anyway." and then grab focus; also remove any access to
timeout_timer.time_left and keep the else branch that logs "Invalid
tween—grabbing focus immediately." before calling
start_button.call_deferred("grab_focus").
- Line 148: Remove the duplicated doc comment "## Sets up the quit confirmation
dialog." that repeats the docstring already present in the preceding doc block
(Lines 142–147); locate the duplicate comment in scripts/main_menu.gd (the lone
"## Sets up the quit confirmation dialog." entry) and delete it so only the
original doc comment remains directly above the related setup function (the quit
confirmation dialog setup code).
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/main_menu.gd
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (3)
scripts/main_menu.gd (3)
32-32: LGTM!Explicit nullable typing with a clear comment documenting the purpose.
150-161: Good additions:close_requestedwiring and explicithide().Connecting
close_requestedto the cancel handler and ensuring the dialog starts hidden addresses the review feedback about alternative close paths. Looks correct.
204-204: Hardcoded toquit_button— currently correct but consider future-proofing.Since only
_on_quit_pressedopens the dialog,last_focused_buttonwill always bequit_button. The tracking variable adds value only if other buttons could open the dialog in the future. Fine for now.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
… first array element sequentially. See the detailed comment on promise_any below. In practice, promise_any([panel_tween.finished, timeout_timer.timeout]) simply awaits panel_tween.finished (the first element) and returns — the timeout timer is never raced against it. If the tween hangs, the 2-second safety net never fires. Additionally, on Line 72 the timeout_timer.time_left check is unreliable: a SceneTreeTimer may be freed once it fires, making the access invalid.
Line 148 repeats the docstring already present in the ## block on Lines 142–147.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…thub.com/ikostan/SkyLockAssault into enable-keyboard-navigation-for-main-menu
Rename the local callback variable from `_on_done` to `is_done` in scripts/main_menu.gd for clearer naming. No behavioral change: the callback still sets the `finished` flag and is connected to both `panel_tween.finished` and the 2s timeout while racing completion to grant focus.
Replace UI clicks with direct JS invocation in several tests to improve reliability. Commented out page.click calls and use page.evaluate to call window.optionsPressed([]), window.advancedPressed([]), window.advancedBackPressed([]), and window.audioPressed([]) (removed the previous [0] arg). Updated tests: tests/audio_flow_test.py, tests/back_flow_test.py, tests/difficulty_flow_test.py, tests/navigation_to_audio_test.py, tests/reset_audio_flow_test.py, tests/volume_sliders_mutes_test.py. This ensures deterministic navigation by invoking the app's handlers directly rather than relying on potentially flaky click interactions.
Owner
Author
|
@sourcery-ai review |
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 (4)
scripts/main_menu.gd (1)
219-236:⚠️ Potential issue | 🟡 Minor
quit_dialog.hide()on Line 226 is unguarded.
_on_quit_dialog_canceledcallsquit_dialog.hide()without anis_instance_valid(quit_dialog)check. While unlikely during normal operation, if the dialog node is freed mid-lifecycle (e.g., scene teardown), this will crash. The rest of the function already guardslast_focused_buttonandquit_buttonwith validity checks—quit_dialogdeserves the same treatment.Proposed fix
func _on_quit_dialog_canceled() -> void: - quit_dialog.hide() + if is_instance_valid(quit_dialog): + quit_dialog.hide() Globals.log_message("Quit canceled.", Globals.LogLevel.DEBUG)tests/navigation_to_audio_test.py (1)
123-123:⚠️ Potential issue | 🟡 MinorInconsistent:
audioPressed([0])here vsaudioPressed([])in all other test files.Line 123 still passes
[0]toaudioPressed, whiletests/audio_flow_test.py(Line 110),tests/volume_sliders_mutes_test.py(Line 110), andtests/reset_audio_flow_test.py(Line 108) all pass[]. This is likely a missed update.Proposed fix
- page.evaluate("window.audioPressed([0])") + page.evaluate("window.audioPressed([])")tests/reset_audio_flow_test.py (1)
197-197:⚠️ Potential issue | 🟡 MinorInconsistent:
audioPressed([0])on Line 197 vsaudioPressed([])elsewhere in this file (Line 108) and other tests.Same issue as in
tests/navigation_to_audio_test.py. Line 108 usesaudioPressed([])but the re-entry call on Line 197 still passes[0].Proposed fix
- page.evaluate("window.audioPressed([0])") + page.evaluate("window.audioPressed([])")tests/back_flow_test.py (1)
133-133:⚠️ Potential issue | 🟡 MinorInconsistent:
audioPressed([0])on Lines 133, 146, 161, 176 vsaudioPressed([])on Line 107 and in other test files.The initial
audioPressedcall (Line 107) was updated to[], but the re-entry calls throughout the test still pass[0]. This should be harmonized for consistency.Additionally, Line 156 still uses
page.click("#options-button", force=True)instead ofpage.evaluate("window.optionsPressed([])"), which is inconsistent with the approach used at the top of this test (Line 78) and across all other test files.
🤖 Fix all issues with AI agents
In `@scripts/main_menu.gd`:
- Around line 66-82: Remove the duplicate focus call: locate the two identical
start_button.call_deferred("grab_focus") invocations after the tween handling
(references: panel_tween, start_button) and delete the redundant one so only a
single call_deferred("grab_focus") remains; also remove the stale fallback
comment lines if they become irrelevant after the duplicate is removed.
🧹 Nitpick comments (1)
scripts/main_menu.gd (1)
195-195: Opener tracking is limited to the quit button.
last_focused_buttonis only ever set toquit_button(Line 195). If other buttons could open the quit dialog in the future, this won't track them. Currently fine since only_on_quit_pressedshows the dialog, but the variable name and fallback logic suggest a more general mechanism was intended. A comment clarifying this single-opener assumption would help future maintainers.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
scripts/main_menu.gdtests/audio_flow_test.pytests/back_flow_test.pytests/difficulty_flow_test.pytests/navigation_to_audio_test.pytests/reset_audio_flow_test.pytests/volume_sliders_mutes_test.py
🧰 Additional context used
🧬 Code graph analysis (5)
tests/back_flow_test.py (5)
tests/conftest.py (1)
page(15-50)tests/refactor/difficulty_integration_test.py (2)
page(96-117)test_difficulty_integration(120-247)tests/refactor/log_level_test.py (1)
page(11-34)tests/refactor/fuel_depletion_test.py (1)
page(80-102)tests/refactor/weapon_firing_test.py (1)
page(68-86)
tests/navigation_to_audio_test.py (5)
tests/conftest.py (1)
page(15-50)tests/refactor/difficulty_integration_test.py (1)
page(96-117)tests/refactor/log_level_test.py (1)
page(11-34)tests/refactor/fuel_depletion_test.py (1)
page(80-102)tests/refactor/weapon_firing_test.py (1)
page(68-86)
tests/difficulty_flow_test.py (1)
tests/conftest.py (1)
page(15-50)
tests/volume_sliders_mutes_test.py (3)
tests/conftest.py (1)
page(15-50)tests/refactor/log_level_test.py (1)
page(11-34)tests/refactor/weapon_firing_test.py (1)
page(68-86)
tests/audio_flow_test.py (4)
tests/conftest.py (1)
page(15-50)tests/refactor/difficulty_integration_test.py (1)
page(96-117)tests/refactor/fuel_depletion_test.py (1)
page(80-102)tests/refactor/weapon_firing_test.py (1)
page(68-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (10)
scripts/main_menu.gd (3)
66-75: Race mechanism looks correct now.The
finishedflag + one-shot connections from bothpanel_tween.finishedand the 2-second timer, polled viaawait get_tree().process_frame, is a valid "first of N" pattern in GDScript. This properly addresses the earlierpromise_anyissue.
148-152: Good:close_requestedrouted through the cancel handler and dialog hidden on setup.Connecting
close_requestedto_on_quit_dialog_canceledand ensuring the dialog starts hidden addresses the reviewer feedback about all close paths restoring focus consistently.
32-32: LGTM — explicit nullable typing forlast_focused_button.Declaring
var last_focused_button: Button = nullwith a default ofnullmakes nullability clear, addressing the earlier review feedback.tests/audio_flow_test.py (2)
80-81: LGTM — consistent switch from DOM clicks to JS handler invocations.The change from
page.clicktopage.evaluate("window.optionsPressed([])")aligns with the broader test suite strategy. The commented-out click is retained for reference.
86-86: Consistent empty-argument pattern across navigation calls.
advancedPressed([]),advancedBackPressed([]), andaudioPressed([])all use empty arrays, matching the convention in other updated test files.Also applies to: 103-103, 110-110
tests/difficulty_flow_test.py (1)
103-104: LGTM — options invocation updated to JS handler.Consistent with the rest of the test suite changes.
tests/navigation_to_audio_test.py (1)
90-91: LGTM — navigation calls updated to JS handlers with empty arrays.
optionsPressed([]),advancedPressed([]), andadvancedBackPressed([])are consistent with the pattern used across the test suite.Also applies to: 96-96, 114-114
tests/volume_sliders_mutes_test.py (1)
77-78: LGTM — all navigation calls consistently use JS handlers with empty arrays.Also applies to: 83-83, 100-100, 110-110
tests/reset_audio_flow_test.py (1)
77-78: LGTM — navigation calls updated to use JS handlers with empty arrays.Also applies to: 83-83, 100-100, 108-108
tests/back_flow_test.py (1)
77-78: LGTM — initial navigation calls updated to JS handlers with empty arrays.Also applies to: 83-83, 100-100, 107-107
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…nd 82.
Lines 79 and 82 are identical. The comments on Lines 80–81 ("Fallback: …" / "Give keyboard focus …") look like leftover artifacts from a previous iteration. The second call is redundant—call_deferred already queues the focus grab for the end of the frame.
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_ready(),start_button.call_deferred("grab_focus")is called twice in a row after the tween handling; you can drop the duplicate to avoid redundant focus calls. - The fade-in completion logic in
_ready()spins a manualfinishedflag loop; consider simplifying this withawait panel_tween.finishedplus acreate_timertimeoutawaitinstead of polling every frame. - When tracking
last_focused_button, you only ever set it in_on_quit_pressed; if you later reuse this pattern for other dialogs, it might be worth generalizing to track focus on dialog open rather than assumingquit_buttonas the default opener.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_ready()`, `start_button.call_deferred("grab_focus")` is called twice in a row after the tween handling; you can drop the duplicate to avoid redundant focus calls.
- The fade-in completion logic in `_ready()` spins a manual `finished` flag loop; consider simplifying this with `await panel_tween.finished` plus a `create_timer` timeout `await` instead of polling every frame.
- When tracking `last_focused_button`, you only ever set it in `_on_quit_pressed`; if you later reuse this pattern for other dialogs, it might be worth generalizing to track focus on dialog open rather than assuming `quit_button` as the default opener.
## Individual Comments
### Comment 1
<location> `tests/audio_flow_test.py:81` </location>
<code_context>
page.wait_for_selector('#options-button', state='visible', timeout=2500)
- page.click("#options-button", force=True)
+ # page.click("#options-button", force=True)
+ page.evaluate("window.optionsPressed([])")
# Go to Advanced settings
</code_context>
<issue_to_address>
**suggestion (testing):** Guard `optionsPressed` evaluation with a readiness check to avoid race conditions
Here the test invokes `window.optionsPressed` without first waiting for it to be defined. For `audioPressed` you already use `wait_for_function('window.audioPressed !== undefined', ...)` to handle load timing; please do the same for `optionsPressed` (and for `advancedPressed` / `advancedBackPressed`) to avoid flakiness if the main menu script initializes more slowly.
Suggested implementation:
```python
# Open options
page.wait_for_selector('#options-button', state='visible', timeout=2500)
# page.click("#options-button", force=True)
page.wait_for_function('window.optionsPressed !== undefined', timeout=2500)
page.evaluate("window.optionsPressed([])")
# Go to Advanced settings
page.wait_for_selector('#advanced-button', state='visible', timeout=2500)
# page.click("#advanced-button", force=True)
page.wait_for_function('window.advancedPressed !== undefined', timeout=2500)
page.evaluate("window.advancedPressed([])")
```
```python
# Go back to Options menu
page.wait_for_selector('#advanced-back-button', state='visible', timeout=2500)
# page.click("#advanced-back-button", force=True)
page.wait_for_function('window.advancedBackPressed !== undefined', timeout=2500)
page.evaluate("window.advancedBackPressed([])")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…s check to avoid race conditions
Here the test invokes window.optionsPressed without first waiting for it to be defined. For audioPressed you already use wait_for_function('window.audioPressed !== undefined', ...) to handle load timing; please do the same for optionsPressed (and for advancedPressed / advancedBackPressed) to avoid flakiness if the main menu script initializes more slowly.
The provided code snippet uses a blocking while loop with await get_tree().process_frame, which can freeze the main thread if the tween doesn't complete (e.g., invalid tween, signal misfire, or race conditions in web exports). This prevents input processing, making buttons unresponsive. Commenting it out avoids the block, allowing immediate focus. To fix without blocking, replace with non-blocking signal wait. Here's the updated snippet (minimal changes: remove loop, use direct await on tween; add types/docstrings as per style). Assumes this is in a function like _ready() in main_menu.gd
Wait for JS handlers to be defined before calling page.evaluate to avoid race conditions and flaky tests. Added page.wait_for_function guards for window.optionsPressed, window.advancedPressed, window.advancedBackPressed and window.gameplayPressed (where used) across tests/back_flow_test.py, tests/difficulty_flow_test.py, tests/navigation_to_audio_test.py, tests/reset_audio_flow_test.py and tests/volume_sliders_mutes_test.py.
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:
- The Playwright tests now repeat the
wait_for_function(...); page.evaluate(...)pattern for multiple callbacks; consider extracting a small helper (e.g.,invoke_js_callback(page, 'optionsPressed')) to reduce duplication and make future updates to this interaction less error-prone. - Several
page.click(...)calls are left commented-out alongside the new JS callback invocation approach; if you don’t intend to revert to DOM clicks, removing these commented lines will keep the tests cleaner and easier to follow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Playwright tests now repeat the `wait_for_function(...); page.evaluate(...)` pattern for multiple callbacks; consider extracting a small helper (e.g., `invoke_js_callback(page, 'optionsPressed')`) to reduce duplication and make future updates to this interaction less error-prone.
- Several `page.click(...)` calls are left commented-out alongside the new JS callback invocation approach; if you don’t intend to revert to DOM clicks, removing these commented lines will keep the tests cleaner and easier to follow.
## Individual Comments
### Comment 1
<location> `tests/navigation_to_audio_test.py:92` </location>
<code_context>
- page.click("#options-button", force=True)
+ # page.click("#options-button", force=True)
+ page.wait_for_function('window.optionsPressed !== undefined', timeout=2500)
+ page.evaluate("window.optionsPressed([])")
# Go to Advanced settings
</code_context>
<issue_to_address>
**suggestion (testing):** Add end-to-end keyboard navigation coverage for the main menu (initial focus + navigating to Audio)
This change still triggers navigation via JavaScript callbacks (e.g., `window.optionsPressed([])`), so it doesn’t validate the actual keyboard/D‑Pad flow.
Please add or extend a Playwright test that:
- Waits for the main menu to be visible and verifies the Start button is initially focused.
- Uses `page.keyboard.press("ArrowDown")`/`"ArrowUp"` to move focus to Options and Audio.
- Uses `Enter` or `Space` to activate the focused button and asserts the expected menu/overlay appears.
That will verify the keyboard navigation behavior end to end and protect against regressions in focus handling.
Suggested implementation:
```python
# NAV-02: Navigate to options menu via keyboard / D-Pad
# Ensure main menu is visible and Start has initial focus
page.wait_for_selector('#start-button', state='visible', timeout=2500)
page.wait_for_selector('#options-button', state='visible', timeout=2500)
start_button = page.locator('#start-button')
assert start_button.evaluate("el => document.activeElement === el")
# Move focus from Start to Options using keyboard and activate Options
page.keyboard.press("ArrowDown")
options_button = page.locator('#options-button')
page.wait_for_function(
"button => document.activeElement === button",
arg=options_button,
timeout=2500,
)
page.keyboard.press("Enter")
# Go to Advanced settings via keyboard inside the Options menu
page.wait_for_selector('#advanced-button', state='visible', timeout=2500)
# Assuming focus starts on the first option in the options menu, move to Advanced
advanced_button = page.locator('#advanced-button')
page.keyboard.press("ArrowDown")
page.wait_for_function(
"button => document.activeElement === button",
arg=advanced_button,
timeout=2500,
)
page.keyboard.press("Enter")
page.wait_for_function('window.changeLogLevel !== undefined', timeout=2500)
advanced_display: str = page.evaluate(
"window.getComputedStyle(document.getElementById('log-level-select')).display")
# Go back to Options menu
page.wait_for_selector('#advanced-back-button', state='visible', timeout=2500)
```
To fully implement the review comment for "navigating to Audio", you should:
1. Locate the part of this test (or a sibling test) that currently navigates to the Audio menu (likely using `page.click("#audio-button", ...)` or `window.audioPressed([])`).
2. Replace that navigation with keyboard-based focus movement, following the same pattern as above:
- Wait for the first option in the options menu to be focused.
- Use `page.keyboard.press("ArrowDown")` / `"ArrowUp"` until `document.activeElement` is the `#audio-button`.
- Press `"Enter"` (or `"Space"`) to open the Audio menu.
3. Add assertions that the Audio menu/overlay appears (e.g., a visible `#audio-back-button`, audio sliders, or whatever the canonical selector is for that overlay).
4. If focus handling inside the Audio menu is important, similarly use keyboard navigation and `document.activeElement` assertions to cover it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
Won't fix. |
github-project-automation
bot
moved this from In Progress
to Done
in Sky Lock Assault Project
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add full keyboard & D-Pad navigation support to the main menu in SkyLockAssault so users can navigate all menu items and interactive controls using only the keyboard (e.g., arrow keys and Enter/Space). This improves accessibility, usability in web exports, and testability within the Godot editor.
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 keyboard focus behavior in the main menu to support seamless keyboard-only navigation.
Enhancements:
Summary by Sourcery
Improve keyboard-driven navigation and focus management in the main menu and align web UI tests with the JavaScript callback-based navigation flow.
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Style