Add telemetry toggle to gui and cleanup local files when telemetry is disabled#446
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds a user-facing telemetry preference to the Godot dock settings flow, wires that preference into server subprocess startup, and updates Python telemetry cleanup behavior when telemetry is disabled.
Changes:
- Adds telemetry enable/disable state to the “Clients & Settings” dock UI and persists it in EditorSettings.
- Injects
GODOT_AI_DISABLE_TELEMETRYfor managed Python server spawns when the UI setting disables telemetry. - Deletes persisted telemetry files on disabled Python telemetry config construction and adds related tests/docs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
plugin/addons/godot_ai/mcp_dock.gd |
Adds telemetry toggle UI, dirty-state handling, popup layout updates, and deferred plugin reload helper. |
plugin/addons/godot_ai/utils/server_lifecycle.gd |
Adds telemetry env injection around managed server spawn/refresh paths. |
src/godot_ai/telemetry.py |
Adds disabled-mode cleanup of persisted telemetry files. |
docs/TELEMETRY.md |
Documents UI opt-out and cleanup behavior. |
test_project/tests/test_telemetry_setting.gd |
Adds GDScript tests for telemetry EditorSettings persistence. |
test_project/tests/test_telemetry_setting.gd.uid |
Adds Godot UID for the new test script. |
tests/unit/test_telemetry.py |
Adds Python cleanup tests for disabled telemetry. |
tests/unit/conftest.py |
Extends telemetry data-dir isolation to the new resolver helper. |
tests/unit/test_telemetry_send.py |
Formatting-only test signature changes. |
tests/integration/_self_update_fixture.py |
Formatting-only error construction change. |
Comments suppressed due to low confidence (6)
plugin/addons/godot_ai/mcp_dock.gd:1697
- This now treats a telemetry-only toggle as dirty, but the confirmation dialog still says the user has checked/unchecked tool domains. Closing after only changing telemetry will show a misleading prompt; update the dialog copy to refer to unapplied settings rather than domains only.
if _settings_are_dirty():
test_project/tests/test_telemetry_setting.gd:48
- This test path is not actually robust to the Godot versions called out below: if
set_setting(null)leaves the key present with a null value, this branch is skipped and the assertion readsbool(null), failing instead of exercising the first-run default. Clear the setting with an API that guarantees absence or structure the test around the real_load_telemetry_settingbehavior without depending on null-removal semantics.
es.set_setting(SETTING_KEY, null)
## After clearing, check absence — note: set_setting(null) may or may not
## remove the key depending on Godot version. Work around: skip directly to
## the init logic assertion.
if not es.has_setting(SETTING_KEY):
docs/TELEMETRY.md:101
- This sentence now contradicts the preceding lines: deleting
customer_uuid.txtandmilestones.jsonon startup is an intentional filesystem side effect. Remove or rephrase the “fully side-effect-free” claim so the opt-out documentation matches the new cleanup behavior.
files (`customer_uuid.txt`, `milestones.json`) are deleted on the next
server startup. The plugin-side helper honors the same variables and
stops buffering events. Opt-out is fully side-effect-free.
plugin/addons/godot_ai/mcp_dock.gd:1924
- The Apply button now also saves the telemetry preference, but its tooltip still says it only saves the excluded tool-domain list. Update the copy so users understand that clicking this button applies all settings on the tab, including telemetry.
es.set_setting(ClientConfigurator.SETTING_EXCLUDED_DOMAINS, canonical_excluded)
es.set_setting("godot_ai/telemetry_enabled", _telemetry_pending_enabled)
plugin/addons/godot_ai/mcp_dock.gd:72
- This comment renames the tab to Settings but still describes the state as only domain-exclusion UI. It now also owns telemetry pending/saved state, so the comment should mention both settings to avoid misleading future changes.
This issue also appears on line 1711 of the same file.
# Settings tab (secondary window, Tab 2) — domain-exclusion UI for clients
# that cap total tool count (Antigravity: 100). Pending set is mutated by
# checkbox clicks; saved set reflects what the spawned server actually
# sees. `Apply & Restart Server` writes pending → setting and triggers a
# plugin reload so the new server comes up with the trimmed list.
plugin/addons/godot_ai/mcp_dock.gd:1713
- The tab is now named “Settings” and includes telemetry, but the surrounding comments still describe it as the Tools/domain-exclusion tab only. Update the comments to reflect the broader contents so future UI changes don't miss the telemetry state.
var tools_margin := _build_margin_container()
tools_margin.name = "Settings"
tools_margin.add_child(tools_tab)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8e8aa2f to
22c3383
Compare
|
@a-johnston This is a great PR, I've been playing with it for the last few minutes. I have a few fixes and nits -- would you prefer I post as suggestions or just push them directly to the PR? |
|
I'm going through the comments right now so we might step on each others toes a bit. Shouldn't take much longer though. I guess feel free to make changes but don't squash so I can rebase more easily? |
|
@a-johnston Live-tested the PR end-to-end on macOS — toggle ON creates One suggestion since you flagged it in the PR description — wiring "Reset to defaults" up to also restore the telemetry toggle: func _on_tools_reset() -> void:
_tools_pending_excluded = PackedStringArray()
for id in _tools_domain_checkboxes:
var chk: CheckBox = _tools_domain_checkboxes[id]
chk.set_pressed_no_signal(true)
# Skip when env-locked so we don't create a dirty pending state
# the user can't apply from the UI.
if _telemetry_toggle != null and not _telemetry_toggle.disabled:
_telemetry_pending_enabled = true
_telemetry_toggle.set_pressed_no_signal(true)
_refresh_tools_ui_state()Otherwise looks great — happy to re-test once the Copilot pass is done. |
|
@a-johnston I need to take off for the rest of the day, but I'd love to get this in an cut a new version so users can update to it. Is it good to go, or you still working? |
Previously '_on_tools_reset' left the telemetry preference untouched, which is inconsistent with the button label and with the rest of the tab's 'reset everything' semantics. Now resets telemetry alongside the domain exclusions, while still respecting env-var locking — if GODOT_AI_DISABLE_TELEMETRY / DISABLE_TELEMETRY pins the toggle to a specific value, Reset skips it so we don't create a dirty pending state the user can't apply from the UI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Merging — I added the "Reset to defaults also resets telemetry" fix as a follow-up commit (
Thanks for the great PR! |
|
I had to step away for a few minutes. Sounds good, I had two copilot comments regarding the env injection behavior left to consider but I'll finish those and submit a followup pr with the other changes requested by copilot. I'm personally of the opinion that the telemetry preference should be left alone when resetting but it's easy enough for the user to change that back themselves. |
|
@a-johnston Oh I'm sorry, I misread that and claude didn't help me catch your actual (correct) intent about resetting telemetry. Yes I agree -- it's confusing to reset it -- likely people who turn it off just want it off forever and we should honor that. Pushing a fix PR now. |
|
@a-johnston Good catch — agree, you had it right the first time. Fix PR up at #448 to revert the Reset-touches-telemetry behavior; sorry for the privacy footgun. |
…ults" (#448) Resetting the telemetry preference via a generic "Reset to defaults" button is a privacy regression: users who turn telemetry off generally want it stayed off, not flipped back on every time they click Reset. The button's scope is tool-domain exclusions; that's what the tooltip already describes ("Re-enable every domain"). This reverts the telemetry-reset block added in 9299dbe and leaves a comment explaining the deliberate scope so it doesn't get re-added by a future "make Reset consistent" pass. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No problem, thanks for the followup. I'll push up a new branch with the other changes soonish but none of the changes are imo blocking so it's fine to cut a release as is. |
* Followup changes for #446 * Review nits: docstring/comment polish from PR #453 feedback - mcp_dock.gd: clarify the Vector2i wrap with a comment explaining that `Vector2i * float` returns `Vector2`, and use `Vector2(560, 460)` as the literal so the cast reads as intentional instead of doubled. - telemetry.gd: normalize tab indentation and "MCP dock" casing in the opt-out priority docstring. - test_telemetry_setting.gd: remove the contradictory "may or may not remove the key depending on Godot version" caveat; passing null to EditorSettings.set_setting has been the documented unset path for many years. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: David Sarno <david@lighthaus.us> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This change primarily adds a new toggle to the configuration popup menu to control whether or not telemetry is enabled. Compared to only supporting the environment variable, this is a more user-friendly approach especially on platforms where it can be annoying to set an environment variable persistently for a gui application (such as on macos). The toggle is only enabled if neither of the two controlling env vars are set and if so, the option is provided via env var for the python subprocess. If an env var is provided by the user, that enable/disable state is persisted to editor settings and used for future launches even if the env var is omitted at that point. Additionally, the python TelemetryConfig has been changed to cleanup local files when telemetry is disabled.
A few smaller supporting changes are also included:
_on_reload_pluginnow defers the actual plugin reload to prevent error spam due to the plugin reloading while the triggering input event (eg, clicking "Apply && Restart Server") is still propagating through the treeTwo things to call out with the current state of this PR, which can both be changed if desired:
_get_data_directory,_tools_apply_btn, etc) but I didn't want to bloat the change too much and there was some precedent (eg_clients_window).Before:
After:
Parts of this PR were generated by claude code. Changes were manually tested with and without manually providing environment variables.