Skip to content

Commit 3493e04

Browse files
dsarnoclaude
andcommitted
Simplify per /simplify review
Three actionable findings from the parallel reuse / quality / efficiency agents: 1. Real bug — efficiency review caught a missing wait_for_port_free in force_restart_server_preserving_mode's dev branch. stop_dev_server() issues OS.kill and returns, but the listener can take >500ms to release the port; start_dev_server's fixed 500ms timer then races the old uvicorn shutdown and the --reload spawn fails to bind. Mirrored the managed path's wait via _wait_for_port_free. 2. Trim doc comments restating WHAT per CLAUDE.md "default to no comments" — kept only the WHY (the adoption-arm trap). Drops ~17 lines across mcp_dock.gd and plugin.gd. 3. Extract _seed_dev_restart_btn / _cleanup_dev_restart_btn helpers in test_dock.gd to match the file's existing _seed_server_row / _cleanup_server_row pattern. Cuts boilerplate across the three new dispatch/state tests. Also drops a no-op bool() cast on a value already typed bool. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 573d51f commit 3493e04

3 files changed

Lines changed: 38 additions & 53 deletions

File tree

plugin/addons/godot_ai/mcp_dock.gd

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -174,13 +174,9 @@ var _mode_override_btn: OptionButton
174174
var _setup_section: VBoxContainer
175175
var _setup_container: VBoxContainer
176176
var _dev_server_btn: Button
177-
## Force-respawn button shown alongside `_dev_server_btn` in dev checkouts
178-
## when Developer mode is on. Same-version Python edits get adopted as
179-
## compatible (server_lifecycle.gd `start_server` adoption arm), so neither
180-
## the version-drift restart nor the spawn-failure restart surfaces — this
181-
## is the unconditional "kill whatever's there and respawn from current
182-
## source" affordance contributors need to pick up source changes that
183-
## don't bump the version.
177+
## Same-version Python edits get adopted as compatible, so neither the
178+
## drift nor the crash Restart button surfaces — this is the unconditional
179+
## kick contributors need to pick up source changes without a version bump.
184180
var _dev_restart_btn: Button
185181
var _log_viewer: LogViewerScript
186182

@@ -1413,12 +1409,9 @@ func _on_dev_server_pressed() -> void:
14131409
_update_dev_restart_btn.call_deferred()
14141410

14151411

1416-
## Pure helper — `Restart Server` is enabled whenever there's *something*
1417-
## to restart on the HTTP port (managed server we spawned/adopted, OR an
1418-
## external `--reload` dev server we recognize by brand). Disabled when
1419-
## nothing is running, with a tooltip that says so. Static so the unit
1420-
## test in `test_dock_dev_server_btn.gd` can cover the truth table without
1421-
## a real plugin.
1412+
## Pure helper for the Restart Server button — enabled iff something is
1413+
## running on the HTTP port we can kick. Static so `test_dock_dev_server_btn`
1414+
## can cover the truth table without a real plugin.
14221415
static func _restart_server_btn_state(has_managed: bool, dev_running: bool) -> Dictionary:
14231416
var port := ClientConfigurator.http_port()
14241417
if has_managed or dev_running:
@@ -1441,13 +1434,11 @@ func _update_dev_restart_btn() -> void:
14411434
return
14421435
if _plugin == null:
14431436
return
1444-
## Same defensive guard as `_update_dev_server_btn` for the self-update
1445-
## mixed-state window where the dock is alive but the plugin script
1446-
## class is mid-reload and missing methods. See #168.
1437+
## See _update_dev_server_btn — same #168 self-update mixed-state guard.
14471438
if not (_plugin.has_method("has_managed_server") and _plugin.has_method("is_dev_server_running")):
14481439
return
14491440
var state := _restart_server_btn_state(_plugin.has_managed_server(), _plugin.is_dev_server_running())
1450-
_dev_restart_btn.disabled = not bool(state["enabled"])
1441+
_dev_restart_btn.disabled = not state["enabled"]
14511442
_dev_restart_btn.tooltip_text = state["tooltip"]
14521443

14531444

plugin/addons/godot_ai/plugin.gd

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1413,21 +1413,23 @@ func force_restart_server() -> void:
14131413
_lifecycle.force_restart_server()
14141414

14151415

1416-
## Force-respawn whichever server is currently running on this port,
1417-
## preserving its launch mode. Managed servers go through the lifecycle's
1418-
## kill+respawn path; `--reload` dev servers are killed by brand match and
1419-
## relaunched with `--reload` so the user keeps Python auto-reload.
1420-
##
1421-
## Used by the dock's "Restart Server" button in dev mode, where same-version
1422-
## edits to Python source get adopted as compatible and the server keeps
1423-
## running stale code. Returns true if a restart was attempted, false if no
1424-
## server is running on the HTTP port.
1416+
## Same-version Python edits get adopted as compatible by the lifecycle's
1417+
## start_server arm, so reloading or relaunching just re-adopts the stale
1418+
## process. This is the "kill whatever's there and respawn from current
1419+
## source" path the dock's Restart Server button hits, preserving managed
1420+
## vs --reload mode.
14251421
func force_restart_server_preserving_mode() -> bool:
14261422
if has_managed_server():
14271423
_lifecycle.force_restart_server()
14281424
return true
14291425
if is_dev_server_running():
14301426
stop_dev_server()
1427+
## OS.kill returns synchronously but the listener can take longer
1428+
## to release the port — without this wait, start_dev_server's
1429+
## fixed 500ms timer races the old uvicorn shutdown and the new
1430+
## --reload spawn fails to bind. Mirrors the managed path's wait
1431+
## inside _lifecycle.force_restart_server.
1432+
_wait_for_port_free(ClientConfigurator.http_port(), 5.0)
14311433
start_dev_server()
14321434
return true
14331435
return false

test_project/tests/test_dock.gd

Lines changed: 19 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1005,65 +1005,57 @@ func test_restart_server_btn_visibility_follows_dev_mode_toggle() -> void:
10051005
"dev toggle off must hide the Setup section, hiding the Restart Server button")
10061006

10071007

1008-
func test_restart_server_btn_dispatches_to_force_restart_preserving_mode() -> void:
1009-
## Click handler must call `force_restart_server_preserving_mode` so the
1010-
## plugin picks the right kill+respawn path (managed vs --reload) for
1011-
## whatever's currently running.
1012-
var plugin := _RestartDispatchPlugin.new()
1013-
plugin.has_managed = true
1008+
## Mirrors `_seed_server_row` / `_cleanup_server_row`: stand up just enough
1009+
## of the dock for the per-frame restart-button helpers to run without a
1010+
## full `_build_ui` pass.
1011+
func _seed_dev_restart_btn(plugin: _RestartDispatchPlugin) -> void:
10141012
_dock._plugin = plugin
10151013
_dock._dev_restart_btn = Button.new()
10161014

1017-
_dock._on_dev_restart_pressed()
1018-
var calls: int = plugin.preserve_mode_calls
10191015

1016+
func _cleanup_dev_restart_btn(plugin: _RestartDispatchPlugin) -> void:
10201017
_dock._dev_restart_btn.free()
10211018
_dock._dev_restart_btn = null
10221019
_dock._plugin = null
10231020
plugin.free()
10241021

1022+
1023+
func test_restart_server_btn_dispatches_to_force_restart_preserving_mode() -> void:
1024+
var plugin := _RestartDispatchPlugin.new()
1025+
plugin.has_managed = true
1026+
_seed_dev_restart_btn(plugin)
1027+
1028+
_dock._on_dev_restart_pressed()
1029+
var calls: int = plugin.preserve_mode_calls
1030+
1031+
_cleanup_dev_restart_btn(plugin)
10251032
assert_eq(calls, 1,
10261033
"Click must call force_restart_server_preserving_mode exactly once")
10271034

10281035

10291036
func test_restart_server_btn_disabled_when_nothing_running() -> void:
1030-
## Per-frame `_update_dev_restart_btn` must reflect the live plugin
1031-
## state. With neither managed nor dev server running, the button
1032-
## stays disabled with an explanatory tooltip.
10331037
var plugin := _RestartDispatchPlugin.new()
1034-
plugin.has_managed = false
1035-
plugin.dev_running = false
1036-
_dock._plugin = plugin
1037-
_dock._dev_restart_btn = Button.new()
1038+
_seed_dev_restart_btn(plugin)
10381039

10391040
_dock._update_dev_restart_btn()
10401041
var disabled: bool = _dock._dev_restart_btn.disabled
10411042
var tooltip: String = _dock._dev_restart_btn.tooltip_text
10421043

1043-
_dock._dev_restart_btn.free()
1044-
_dock._dev_restart_btn = null
1045-
_dock._plugin = null
1046-
plugin.free()
1047-
1044+
_cleanup_dev_restart_btn(plugin)
10481045
assert_true(disabled, "No server running must disable the button")
10491046
assert_contains(tooltip, "No godot-ai server is running")
10501047

10511048

10521049
func test_restart_server_btn_enabled_when_managed_running() -> void:
10531050
var plugin := _RestartDispatchPlugin.new()
10541051
plugin.has_managed = true
1055-
_dock._plugin = plugin
1056-
_dock._dev_restart_btn = Button.new()
1052+
_seed_dev_restart_btn(plugin)
10571053

10581054
_dock._update_dev_restart_btn()
10591055
var disabled: bool = _dock._dev_restart_btn.disabled
10601056
var tooltip: String = _dock._dev_restart_btn.tooltip_text
10611057

1062-
_dock._dev_restart_btn.free()
1063-
_dock._dev_restart_btn = null
1064-
_dock._plugin = null
1065-
plugin.free()
1066-
1058+
_cleanup_dev_restart_btn(plugin)
10671059
assert_false(disabled, "Managed server running must enable the button")
10681060
assert_contains(tooltip, "current source",
10691061
"Tooltip must explain the button picks up source changes")

0 commit comments

Comments
 (0)