Skip to content

Commit 72b35d7

Browse files
dsarnoclaude
andauthored
[Audit v2 #360] Extract LogViewer + PortPickerPanel from mcp_dock.gd (#390)
* [Audit v2 #360] Extract LogViewer + PortPickerPanel from mcp_dock.gd Audit-v2 #360 partial extraction. Two cohesive Control subtrees move to res://addons/godot_ai/dock_panels/: - log_viewer.gd: owns _log_section, _log_display, _log_toggle, _last_log_count, the tick() that appends new buffer lines, and the _on_log_toggled signal handler. McpDock._process now calls _log_viewer.tick() instead of the inline _update_log helper, and _apply_dev_mode_visibility flips _log_viewer.visible directly. - port_picker_panel.gd: owns _port_picker_section, _port_picker_spinbox, range-validates the spinbox value, and emits port_apply_requested(int). McpDock listens, writes the EditorSetting, and reloads. The panel exposes seed_suggested_port() for the on-surface refresh that _update_crash_panel calls when the picker becomes visible. Why only these two and not the audit's full four-panel list: ServerStatusPanel and ClientRowController were deliberately deferred because their UI scatters across the dock layout — extracting them needs either visible UI reorganization (out of scope for an internal refactor) or a coordinator-Node pattern with property-accessor forwarders on McpDock that re-tangle the very state they claim to move. A future refactor probably wants extract-by-concern (e.g. mcp_async_refresh_state_machine.gd, mcp_client_action_dispatcher.gd) rather than extract-by-panel. mcp_dock.gd carries a comment block explaining this so the next contributor doesn't redo the analysis from scratch. Side-effect on script/local-self-update-smoke: the field-storage smoke harness anchored its vNext Dict/Array injection to "var _last_log_count := 0", which moved into LogViewer. The harness comment explicitly predicted this ("anchoring to a recently-moved field would silently miss the regression rather than failing loudly") — the loud failure fired on this PR's pytest run. Anchor migrated to the equally-stable "var _last_connected := false" with an updated comment so the next field move is guided. Local pre-commit gate: - ruff check src/ tests/ — All checks passed - pytest -q — 892 passed - script/ci-check-gdscript — All GDScript files OK - Headless GDScript test_run via MCP — 1211/1227 passed, 0 failed (16 skipped are pre-existing macOS-headless camera-current cases) mcp_dock.gd: 2424 → 2371 LOC (-53). Two new files, 80 + 65 LOC. Closes #360 partially. A follow-up issue will track the deferred ServerStatusPanel / ClientRowController extractions with the extract-by-concern direction noted in mcp_dock.gd. https://claude.ai/code/session_01B8h9tYowSrjvzUA1hEc1KC * [Audit v2 #360] /simplify cleanup on extracted dock panels Three findings from /simplify's reuse + quality agents, applied: 1. Promote McpDock._make_header to `static`. LogViewer was duplicating the exact same 5-line helper plus the COLOR_HEADER constant. Now the panel calls McpDock._make_header(text) directly — no circular preload (class_name references are symbolic, resolved by the compiler, not via preload). Net: -7 LOC across the two files, single source of truth for the dock's section-header style. 2. Type annotations on the extraction seams: - LogViewer._log_buffer: McpLogBuffer (was untyped) - LogViewer.setup(log_buffer: McpLogBuffer) (was untyped) - McpDock._log_viewer: LogViewerScript (was comment-as-type) - McpDock._port_picker_panel: PortPickerPanelScript (was comment-as-type) The Mcp* class_name'd types were registered globally already; the preload-script types work as variable type annotations in GDScript 4.x. No new files needed. 3. Signal-based logging-toggled instead of Demeter reach-through. LogViewer was reaching into _connection.dispatcher.mcp_logging from _on_log_toggled — knowing about a connection field with a dispatcher field with an mcp_logging flag. Inconsistent with PortPickerPanel, which already routes through a clean signal. Now LogViewer emits `logging_enabled_changed(enabled: bool)`, the dock connects and forwards onto the dispatcher in `_on_log_logging_enabled_changed`. Side benefit: LogViewer.setup() no longer needs the connection parameter, so its signature shrinks to `setup(log_buffer)`. Local re-run of the pre-commit gate after these edits: - script/ci-check-gdscript — All GDScript files OK - pytest -q — 892 passed - Headless GDScript test_run via MCP — 1211/1227 passed, 0 failed https://claude.ai/code/session_01B8h9tYowSrjvzUA1hEc1KC * [Audit v2 #360] /review: signal-emit tests + setup() fold /review surfaced one actionable gap: no direct tests for the new panels' emit contracts. This commit pins both: - test_port_picker_panel_emits_apply_requested_for_in_range_port: in-range port → spy receives the value. - test_port_picker_panel_skips_emit_for_out_of_range_port: out-of-range port (after relaxing min_value) → spy receives nothing. The clamp on the panel side is the dock's only defense against bogus EditorInterface.set_setting writes if a future SpinBox replacement doesn't clamp. - test_log_viewer_emits_logging_enabled_changed_on_toggle: toggle false then true → spy receives [false, true] in order. All three tests instantiate panels in isolation rather than going through `_dock._build_ui()`. The port-picker tests have to: emitting through the dock-wired signal fires `_dock._on_port_apply_requested` which calls `_on_reload_plugin()` → `EditorInterface.set_plugin_enabled (false/true)`, tearing down the test runner mid-suite. Caught the hang on the first run; fixed by isolating. The log-viewer test is also isolated for symmetry and to avoid pulling in the dock's heavier `_build_ui()` for what's a single-signal contract test. The spies are inner classes (`_PortApplySpy`, `_LogToggleSpy`) matching the existing `_RestartDispatchPlugin` spy pattern at the top of the file. Multi-line lambdas with closure-captured locals didn't reliably evaluate the body under the test runner — typed-receiver methods are the safe form. Side effect: panels' UI build moved from `_ready()` → `setup()`. The dock instantiates panels detached from any tree in test fixtures (`_dock = McpDockScript.new()` then `_dock._build_ui()`), so `_ready` never fires and the panel's internal Controls stayed null when tests poked at them. Building synchronously inside `setup()` (idempotent via a null-check on the canonical Control) mirrors the pre-extraction inline-build behavior. Production code paths get the same UI; only the timing changed. Local re-run after these edits: - script/ci-check-gdscript — All GDScript files OK - pytest -q — 892 passed - Headless GDScript test_run via MCP — 1214/1230 passed, 0 failed (1227 baseline + 3 new tests; 16 pre-existing macOS-headless skips) https://claude.ai/code/session_01B8h9tYowSrjvzUA1hEc1KC --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 26f10d1 commit 72b35d7

7 files changed

Lines changed: 288 additions & 103 deletions

File tree

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
@tool
2+
extends VBoxContainer
3+
4+
## Dock subpanel — renders the MCP request/response log buffer. Owns its own
5+
## UI subtree, the line-count cursor, and the display-visibility toggle. Emits
6+
## `logging_enabled_changed` so the dock can route the flag onto the
7+
## connection dispatcher without the panel knowing the routing exists.
8+
##
9+
## Extracted from mcp_dock.gd as part of audit-v2 #360 — see the comment at
10+
## the top of mcp_dock.gd for the broader extraction story.
11+
12+
signal logging_enabled_changed(enabled: bool)
13+
14+
var _log_buffer: McpLogBuffer
15+
var _log_display: RichTextLabel
16+
var _log_toggle: CheckButton
17+
var _last_log_count := 0
18+
19+
20+
## Build the UI synchronously here so callers (and detached-tree tests that
21+
## instantiate the dock with `McpDockScript.new()` and never enter the tree)
22+
## can interact with the panel's controls right after `setup()`. Mirrors the
23+
## pre-extraction inline-build behavior that test_dock.gd relies on.
24+
##
25+
## Idempotent: `_log_display == null` covers an unlikely double-`setup()` call
26+
## without rebuilding (which would orphan the prior controls).
27+
func setup(log_buffer: McpLogBuffer) -> void:
28+
_log_buffer = log_buffer
29+
if _log_display == null:
30+
_build_ui()
31+
32+
33+
func _build_ui() -> void:
34+
size_flags_vertical = Control.SIZE_EXPAND_FILL
35+
add_child(HSeparator.new())
36+
37+
var log_header_row := HBoxContainer.new()
38+
var log_header := McpDock._make_header("MCP Log")
39+
log_header.size_flags_horizontal = Control.SIZE_EXPAND_FILL
40+
log_header_row.add_child(log_header)
41+
42+
_log_toggle = CheckButton.new()
43+
_log_toggle.text = "Log"
44+
_log_toggle.button_pressed = true
45+
_log_toggle.toggled.connect(_on_log_toggled)
46+
log_header_row.add_child(_log_toggle)
47+
48+
add_child(log_header_row)
49+
50+
_log_display = RichTextLabel.new()
51+
_log_display.size_flags_vertical = Control.SIZE_EXPAND_FILL
52+
_log_display.custom_minimum_size = Vector2(0, 120)
53+
_log_display.scroll_following = true
54+
_log_display.bbcode_enabled = false
55+
_log_display.selection_enabled = true
56+
add_child(_log_display)
57+
58+
59+
## Called from McpDock._process when the panel is visible. Appends any new
60+
## log lines since the last tick.
61+
func tick() -> void:
62+
if _log_buffer == null or _log_display == null:
63+
return
64+
var count: int = _log_buffer.total_count()
65+
if count == _last_log_count:
66+
return
67+
var new_lines: Array[String] = _log_buffer.get_recent(count - _last_log_count)
68+
for line in new_lines:
69+
_log_display.add_text(line + "\n")
70+
_last_log_count = count
71+
72+
73+
func _on_log_toggled(enabled: bool) -> void:
74+
_log_display.visible = enabled
75+
logging_enabled_changed.emit(enabled)
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
uid://cr5nbnd6vj3b8
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
@tool
2+
extends VBoxContainer
3+
4+
## Dock subpanel — port-change escape hatch surfaced inside the spawn-failure
5+
## crash panel when the HTTP port is contested (PORT_EXCLUDED, FOREIGN_PORT).
6+
## Emits `port_apply_requested(new_port)` after range-validation; the dock
7+
## handles writing the EditorSetting and reloading the plugin.
8+
##
9+
## Extracted from mcp_dock.gd as part of audit-v2 #360 — see the comment at
10+
## the top of mcp_dock.gd for the broader extraction story.
11+
12+
const ClientConfigurator := preload("res://addons/godot_ai/client_configurator.gd")
13+
14+
signal port_apply_requested(new_port: int)
15+
16+
var _spinbox: SpinBox
17+
18+
19+
## Build the UI synchronously here so callers (and detached-tree tests that
20+
## instantiate the dock with `McpDockScript.new()` and never enter the tree)
21+
## can interact with the panel's controls right after `setup()`. Mirrors the
22+
## pre-extraction inline-build behavior that test_dock.gd relies on.
23+
##
24+
## Idempotent: `_spinbox == null` covers an unlikely double-`setup()` call
25+
## without rebuilding (which would orphan the prior controls).
26+
func setup() -> void:
27+
if _spinbox == null:
28+
_build_ui()
29+
30+
31+
func _build_ui() -> void:
32+
add_theme_constant_override("separation", 4)
33+
visible = false
34+
35+
var picker_row := HBoxContainer.new()
36+
picker_row.add_theme_constant_override("separation", 6)
37+
38+
_spinbox = SpinBox.new()
39+
_spinbox.min_value = ClientConfigurator.MIN_PORT
40+
_spinbox.max_value = ClientConfigurator.MAX_PORT
41+
_spinbox.step = 1
42+
_spinbox.value = ClientConfigurator.http_port()
43+
_spinbox.size_flags_horizontal = Control.SIZE_EXPAND_FILL
44+
picker_row.add_child(_spinbox)
45+
46+
var apply_btn := Button.new()
47+
apply_btn.text = "Apply + Reload"
48+
apply_btn.tooltip_text = (
49+
"Saves godot_ai/http_port to Editor Settings and reloads the plugin so"
50+
+ " the server spawns on the new port."
51+
)
52+
apply_btn.pressed.connect(_on_apply_pressed)
53+
picker_row.add_child(apply_btn)
54+
55+
add_child(picker_row)
56+
57+
58+
## Seed the spinbox with a suggested non-reserved port. Idempotent — the dock
59+
## calls this each time the panel becomes visible so a stale value from a
60+
## previous spawn-failure doesn't carry over.
61+
func seed_suggested_port() -> void:
62+
if _spinbox == null:
63+
return
64+
_spinbox.value = ClientConfigurator.suggest_free_port(
65+
ClientConfigurator.http_port() + 1
66+
)
67+
68+
69+
func _on_apply_pressed() -> void:
70+
var new_port: int = int(_spinbox.value)
71+
if new_port < ClientConfigurator.MIN_PORT or new_port > ClientConfigurator.MAX_PORT:
72+
return
73+
port_apply_requested.emit(new_port)
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
uid://hlggbo1q65eq

0 commit comments

Comments
 (0)