Dev-section follow-ups: Restarting… feedback + consolidate buttons + parent-walk#408
Merged
Merged
Conversation
Live smoke uncovered a 5s editor freeze (from _wait_for_port_free) with no acknowledgement of the click — the user wonders if the button did anything. Mirror the existing _crash_restart_btn / _version_restart_btn pattern: set _server_restart_in_progress on click, dispatch via call_deferred + a 0.15s paint window, then clear the flag after the spawn lands. _update_dev_section_buttons checks the flag and renders "Restarting…" + disabled while in flight. Test path is non-tree, so _on_dev_restart_pressed runs synchronously when is_inside_tree() is false — keeps the existing dispatch test green without needing await scaffolding. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per PR review feedback: two restart-flavored buttons in the same section felt redundant, and the new Restart Server stayed disabled when only an adopted external dev server was running because brand detection misses orphaned multiprocessing.spawn workers. Replaces both _dev_server_btn (3-state Switch/Exit/Start toggle) and _dev_restart_btn (kill+respawn preserving mode) with a single primary button + a small stop affordance: - _dev_primary_btn — full-width "Restart Dev Server" / "Start Dev Server" (label adapts to whether something's running). Always enabled. Click → force_restart_or_start_dev_server which kills any godot-ai server (managed or dev), waits for the port to free, and lands in --reload mode. Same "Restarting…" feedback during the blocking wait. - _dev_stop_btn — compact "✕" beside the primary. Enabled only when a dev server is actually running. Click → stop_dev_server (existing). Intentionally never targets the managed server (lifecycle owns that). Plugin's force_restart_server_preserving_mode is renamed/retargeted as force_restart_or_start_dev_server. The previous "preserve managed mode" semantics drop — clicking Restart now always lands in dev mode, since that's the only mode contributors editing Python source actually want. The lifecycle's force_restart_server stays for the version-drift / crash recovery paths that legitimately preserve managed mode. One verb, one mental model. Adapted tests cover the truth table. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous version branched on has_managed_server / is_dev_server_running, both of which miss orphaned multiprocessing.spawn uvicorn workers — when the parent reloader dies (or the editor is restarted while the worker keeps holding the port), the worker's cmdline is just "spawn_main" without godot_ai branding, so neither detection path fires. Result: clicking Restart skipped the kill and went straight to start_dev_server, which then failed to bind :8000. Switch the kill arm's condition to _is_port_in_use(port). The user clicking Restart is explicit consent to take over the port, so killing whoever's there (regardless of brand) is the right semantics. Lifecycle state still resets when has_managed_server() so the next adoption pass sees a clean slate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1. Fix Restart/Start label bug. After clicking the primary button, the newly-spawned --reload server's LISTEN holder is uvicorn's `multiprocessing.spawn` worker — that PID's cmdline doesn't contain the godot_ai brand, so `is_dev_server_running()` returned false and the label stayed "Start Dev Server" instead of flipping to "Restart Dev Server" once the server was up. Walk up the parent chain (bounded depth 5) inside `_pid_cmdline_is_godot_ai` so a worker whose parent reloader IS branded gets recognised. Adds `_pid_parent` helper covering POSIX (`ps -o ppid=`) and Windows (PowerShell CIM). 2. Drop the dock's "Mode override" dropdown. The dropdown advertised toggling between the github-repo local plugin and the installed ZIP version, but it didn't actually do that — it just nudged `is_dev_checkout()` for the update banner, which is more usefully driven by the GODOT_AI_MODE env var (still supported, still tested in test_clients.gd). Removes ~50 lines of dead UI plumbing and the stale "fresh check paints over a clean slate" comment. 3. Delete the orphan live_demo test file from a debugging session that never landed cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR iterates on the dev-mode “Setup” section UX and restart behavior after #406, aiming to provide clearer restart-in-flight feedback, simplify the dev-server controls, and make port-takeover/restart more robust (including --reload worker PID detection via parent-walk).
Changes:
- Consolidates dev-server controls into a single primary “Start/Restart Dev Server” button plus a compact “✕” stop button, with in-flight “Restarting…” feedback.
- Adds an aggressive “restart or start” path that kills any PID holding the HTTP port before spawning a fresh
--reloaddev server. - Improves PID branding detection by walking parent processes to recognize
uvicorn --reloadworker processes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test_project/tests/test_dock.gd | Updates dock UI tests to cover the new primary + stop dev buttons, labels, dispatch, and in-flight feedback state. |
| test_project/tests/test_dock_dev_server_btn.gd | Replaces old toggle/restart truth-table tests with new static helper tests for primary/stop dev button state. |
| plugin/addons/godot_ai/plugin.gd | Implements parent-walk PID branding + adds force_restart_or_start_dev_server() with aggressive port takeover behavior. |
| plugin/addons/godot_ai/mcp_dock.gd | Removes mode-override dropdown UI and refactors dev section into primary + stop buttons with “Restarting…” feedback flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## Brief paint cycle so the user sees "Restarting…" before the | ||
| ## blocking _wait_for_port_free freezes the editor for up to 5s. | ||
| await get_tree().create_timer(0.15).timeout | ||
| if _plugin != null: |
Contributor
Author
There was a problem hiding this comment.
Fixed in 31f8fa3. Added the has_method("force_restart_or_start_dev_server") re-check after the 0.15s await, mirroring the #168 guard pattern used elsewhere in the dock.
Comment on lines
+1388
to
+1389
| _dev_primary_btn.text = "Restarting…" | ||
| _dev_primary_btn.tooltip_text = "Killing the current server and respawning…" |
Contributor
Author
There was a problem hiding this comment.
Fixed in 31f8fa3. Switched to "Restarting..." (three ASCII dots) on both the button label and tooltip to match _crash_restart_btn / _version_restart_btn / the status-text "Restarting server..." already in this file. Tests assert on the substring "Restarting" so they're unaffected.
dsarno
added a commit
that referenced
this pull request
May 7, 2026
1. Re-check `_plugin.has_method("force_restart_or_start_dev_server")`
after the 0.15s await in `_perform_dev_restart_after_feedback`. A
self-update mixed-state could swap the plugin script class while we
were sleeping, leaving the old reference pointing at a class that no
longer carries the new method. Same #168 guard pattern other dock
helpers use.
2. Switch the dev primary button's "Restarting…" / "respawning…" labels
to "Restarting..." / "respawning..." (three ASCII dots) to match the
sibling _crash_restart_btn / _version_restart_btn / status text
already in this file. Tests assert on the substring "Restarting" so
they're unaffected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
dsarno
added a commit
that referenced
this pull request
May 7, 2026
…cy (#409) 1. Re-check `_plugin.has_method("force_restart_or_start_dev_server")` after the 0.15s await in `_perform_dev_restart_after_feedback`. A self-update mixed-state could swap the plugin script class while we were sleeping, leaving the old reference pointing at a class that no longer carries the new method. Same #168 guard pattern other dock helpers use. 2. Switch the dev primary button's "Restarting…" / "respawning…" labels to "Restarting..." / "respawning..." (three ASCII dots) to match the sibling _crash_restart_btn / _version_restart_btn / status text already in this file. Tests assert on the substring "Restarting" so they're unaffected. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
Follow-up to #406, based on live testing feedback after that PR merged. Four commits on top of beta:
Show "Restarting…" feedback while a restart is in flight. Without this the user clicks Restart and sees a 5s editor freeze (from
_wait_for_port_free) with no acknowledgement. Mirrors the existing_crash_restart_btn/_version_restart_btnpattern: set_server_restart_in_progresson click, dispatch viacall_deferred+ 0.15s paint window, clear after the spawn lands._update_dev_section_buttonsshows "Restarting…" disabled while the flag is set.Consolidate the dev-section to one primary "Restart Dev Server" + small "✕" stop button. Drops the previous
_dev_server_btn(3-state Switch / Exit / Start toggle) and the disabled-when-nothing-running second Restart button. Primary's label adapts: "Start Dev Server" when nothing's running, "Restart Dev Server" otherwise. Click always lands in--reloadmode. ✕ stops without respawning.Aggressive port takeover in
force_restart_or_start_dev_server. The user clicking Restart is explicit consent to take over:8000, so the kill arm now runs whenever_is_port_in_use(port)is true — covers orphanedmultiprocessing.spawnuvicorn workers whose parent reloader died and brand detection misses.Parent-walk in
_pid_cmdline_is_godot_ai+ drop dock mode-override dropdown + delete a stale debug test file.--reloadserver's LISTEN holder is the multiprocessing worker (nogodot_aibrand on its own cmdline). Walks up to depth 5 so a worker whose parent reloader IS branded gets recognised. Adds_pid_parent(POSIXps -o ppid=, Windows PowerShell CIM).GODOT_AI_MODEenv var still works (and is still tested).Test plan
ruff check src/ tests/— cleanpytest -q— 899 passedgodot --headless --import— no GDScript parse errors--reloadserver PID 7629 spawned (confirmed vialsof)🤖 Generated with Claude Code