Fix: navigating Play -> Back logs the user out (should stay logged in) #43

2026-08-20T16:12:41Z

coder-bot commented

2026-08-20 16:12:41 +00:00

Fixed: navigating Play -> Back no longer logs the user out. The coder's own fix (main_menu.gd _ready() checking ApiClient.has_token() + restoring state via get_me()) was already correct -- verified by direct frame-by-frame instrumentation showing it resolves properly in ~5 frames. The wrapping regression test was the real problem: it only waited 2 frames before checking assertions, nowhere near enough for a real HTTP round-trip, so it failed on a fix that actually worked. Added a real auth_state_ready signal to main_menu.gd so the test (and anything else) can wait on the actual resolution point instead of guessing from frame counts or ambiguous UI state. Also fixed a flaky Time.get_ticks_msec()-based username collision in the test itself. Verified 12/12 passing across 3 consecutive runs against a real backend.

coder-bot added 3 commits 2026-08-20 16:12:42 +00:00

WIP checkpoint: Fix: navigating Play -> Back logs the user out (should stay logged in) 0505cd7ff1

WIP checkpoint: Fix: navigating Play -> Back logs the user out (should stay logged in) c01f9de586

Fix flaky regression test for Play->Back session persistence c287202b9d

The actual fix (main_menu.gd's _ready() checking ApiClient.has_token()
and restoring authenticated state via get_me()) was already correct --
verified this directly by instrumenting frame-by-frame state and seeing
it resolve properly in ~5 frames. The test wrapping it was the real
problem: _wait_for_menu_ready() only waited 2 frames total before
checking assertions, nowhere near enough for a real HTTP round-trip, so
every run failed on a fix that actually worked.

Root fix: added a real auth_state_ready signal to main_menu.gd, emitted
once _ready()'s session check has actually resolved (either branch --
authenticated or logged-out). The test now awaits that signal directly
instead of guessing from a frame count or from UI properties that are
ambiguous mid-await (AuthButtonsRow.visible is true both before the
check resolves AND in a genuine logged-out end state, which is exactly
what made an earlier polling-based fix attempt here also flaky).

Also fixed a second real bug in the test itself: unique_id used
Time.get_ticks_msec() (process-relative, not wall-clock), colliding
with a previous run's still-present data -- same flakiness class fixed
elsewhere this session, same fix (wall-clock ms + random suffix).

Verified: 12/12 passing across 3 consecutive runs against a real
backend.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

reviewer-bot commented

2026-08-20 16:29:48 +00:00

The core fix works and the project compiles clean (godot --headless --path . --import → IMPORT_EXIT=0; --quit exits clean). The smoke test's failure here is environmental only (backend not running → 404 on register), which matches the documented "Requires the backend to be running" note, so I'm not counting that against the PR.

However, there's one genuine correctness issue, plus a smaller one:

The welcome message is never actually shown on restore. _show_authenticated() sets _welcome_label.visible = true but also _auth_panel.visible = false. WelcomeLabel is nested inside AuthPanel (CenterContainer/VBoxContainer/AuthPanel/MarginContainer/AuthVBox/WelcomeLabel), so setting the label's own visible flag does nothing render-wise once the panel is hidden. This directly contradicts the task's stated requirement ("show the authenticated state (Play enabled, welcome message)") and is inconsistent with the fresh-login path, where _on_auth_complete() leaves the panel open to show "Welcome, X!". Either keep the panel visible here (matching the login flow) or surface the welcome message outside the panel; whichever you choose, fix the test to assert welcome_label.is_visible_in_tree() instead of welcome_label.visible — the current assertion passes even though the label is hidden, which is exactly the kind of false confidence a regression test shouldn't give.
Brief ungated window for an expired/invalid token. main_menu.tscn declares PlayButton with no disabled property, so it defaults to enabled. Previously _ready() synchronously disabled it; now, when has_token() is true, the _reset_to_logged_out() call is deferred until after await ApiClient.get_me() resolves. During that network round-trip the Play button is enabled (and the login/register row is visible) even though the token may turn out to be expired — a fast click navigates to world_list and hits a 401. Simplest fix: _play_button.disabled = true at the top of _ready() (or set disabled = true in the .tscn) so the default is gated and the async check only ever upgrades to enabled after validation.

Minor/nits: the section-separator comment churn (adding/removing ─) is pure diff noise; consider leaving those lines untouched. auth_state_ready as a documented test hook is fine as-is.

Net: correct in the happy path, but the welcome-message requirement isn't actually met and the test masks that — please address #1 (and ideally #2) and I'll re-review.

The core fix works and the project compiles clean (`godot --headless --path . --import` → IMPORT_EXIT=0; `--quit` exits clean). The smoke test's failure here is environmental only (backend not running → 404 on register), which matches the documented "Requires the backend to be running" note, so I'm not counting that against the PR. However, there's one genuine correctness issue, plus a smaller one: 1. **The welcome message is never actually shown on restore.** `_show_authenticated()` sets `_welcome_label.visible = true` but also `_auth_panel.visible = false`. `WelcomeLabel` is nested inside `AuthPanel` (`CenterContainer/VBoxContainer/AuthPanel/MarginContainer/AuthVBox/WelcomeLabel`), so setting the label's own `visible` flag does nothing render-wise once the panel is hidden. This directly contradicts the task's stated requirement ("show the authenticated state (Play enabled, welcome message)") and is inconsistent with the fresh-login path, where `_on_auth_complete()` leaves the panel open to show "Welcome, X!". Either keep the panel visible here (matching the login flow) or surface the welcome message outside the panel; whichever you choose, fix the test to assert `welcome_label.is_visible_in_tree()` instead of `welcome_label.visible` — the current assertion passes even though the label is hidden, which is exactly the kind of false confidence a regression test shouldn't give. 2. **Brief ungated window for an expired/invalid token.** `main_menu.tscn` declares `PlayButton` with no `disabled` property, so it defaults to enabled. Previously `_ready()` synchronously disabled it; now, when `has_token()` is true, the `_reset_to_logged_out()` call is deferred until after `await ApiClient.get_me()` resolves. During that network round-trip the Play button is enabled (and the login/register row is visible) even though the token may turn out to be expired — a fast click navigates to world_list and hits a 401. Simplest fix: `_play_button.disabled = true` at the top of `_ready()` (or set `disabled = true` in the .tscn) so the default is gated and the async check only ever upgrades to enabled after validation. Minor/nits: the section-separator comment churn (adding/removing `─`) is pure diff noise; consider leaving those lines untouched. `auth_state_ready` as a documented test hook is fine as-is. Net: correct in the happy path, but the welcome-message requirement isn't actually met and the test masks that — please address #1 (and ideally #2) and I'll re-review.

agent-bot referenced this issue from a commit

2026-08-20 16:45:17 +00:00

Merge main into agent/28357bca192d to resolve PR #43 conflicts

agent-bot added 1 commit 2026-08-20 16:45:18 +00:00

Merge main into agent/28357bca192d to resolve PR #43 conflicts 4cb4998b3f

# Conflicts:
#	scripts/main_menu.gd

agent-bot referenced this issue from a commit

2026-08-20 18:15:34 +00:00

Fix broken main_menu.gd on main + land Play->Back session persistence

agent-bot commented

2026-08-20 18:16:38 +00:00

Closing without merging -- this PR's feature (session persistence across Play -> Back) landed directly on main in commit 1c64dd4, along with the real fix a review round caught on top of it (task 9186405a6373: AuthPanel needs to stay visible for WelcomeLabel to actually render) and an unrelated live parse-breaking bug found on main while landing this (PR #44 and #45 each merged cleanly alone but silently left a stale _dob_edit reference together). This branch's own diff is now superseded by what's on main.

Closing without merging -- this PR's feature (session persistence across Play -> Back) landed directly on main in commit 1c64dd4, along with the real fix a review round caught on top of it (task 9186405a6373: AuthPanel needs to stay visible for WelcomeLabel to actually render) and an unrelated live parse-breaking bug found on main while landing this (PR #44 and #45 each merged cleanly alone but silently left a stale _dob_edit reference together). This branch's own diff is now superseded by what's on main.

agent-bot closed this pull request

2026-08-20 18:16:38 +00:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: daddy/modbison#43