Skip to content

Dispose the schema viewers' TextMate installations - #550

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/546-schema-textmate-dispose
Sep 22, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
fix/546-schema-textmate-dispose

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

Summary

Fixes #546. The plan-viewer and query-session schema tabs installed TextMate syntax highlighting at construction and never disposed it — and a TextMate installation owns a tokenization model whose thread roots itself, so every closed schema tab leaked a live thread. Found during the Avalonia 12 upgrade planning; fixed on dev now because these files are untouched by PR #549, so there is no conflict.

Both viewers now mirror the query editor's proven lifecycle: install on AttachedToVisualTree (null-guarded so re-attach after a dispose reinstalls), dispose + null on DetachedFromVisualTree. Same behavior on today's AvaloniaEdit 11.4.1 (the reuse-a-disposed-transformer quirk the query editor already survives) and on 12.0.0's explicit transformer ownership when #549 merges.

Review notes

avalonia-gotcha review: clean, no findings. Verified: the closure cycle (editor ↔ handlers ↔ tm) is self-contained and collectable once the tab drops the editor; detach fires on tab close, deselection, and window close; the attach handler is call-for-call identical to SetupSyntaxHighlighting; no test reads colorization before first attach; the isSql gate still holds. Side benefit: the headless suite stops leaking a tokenizer thread per schema-tab test.

Test plan

  • Full suite: 712 passed, 0 failed, 1 skipped (known macOS skip) — no behavior change to pin beyond what existing schema-tab tests cover.
  • Manual (rides along with any normal use): open Show Indexes / Show Table Definition, confirm SQL coloring, switch tabs away and back, coloring survives.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Sr43FouJ6RizUyoWQeR5FX

Both read-only schema viewers installed a TextMate installation at
construction and never disposed it. An installation owns a
tokenization model whose thread roots itself against collection, so
every schema tab ever closed leaked a live thread for the life of the
process.

Both now run the query editor's own lifecycle: install on attach
(guarded so a re-attach after a dispose reinstalls), dispose and null
on detach. Detach fires on tab switches as well as closes, which is
exactly the dance the query editor has always survived, on 11.4.1's
reuse-the-disposed-transformer quirk today and on 12's explicit
ownership when the upgrade lands.

Fixes #546

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sr43FouJ6RizUyoWQeR5FX
@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown

Reviewed. The fix is small, correct, and matches the existing proven pattern in QuerySessionControl.axaml.cs (_textMateInstallation field + attach/detach) call-for-call — null-guard on attach prevents double-install on re-attach, dispose+null on detach releases the tokenizer thread. Since each call site (ShowSchemaResult / AddSchemaTab) creates a brand-new editor instance, there's no risk of stacking duplicate handlers across calls.

One note, not a blocker: the PR description cites "Full suite: 712 passed... headless suite stops leaking a tokenizer thread per schema-tab test," but this repo's solution only has one test project (tests/PlanViewer.Core.Tests) — there's no Avalonia headless UI test project that would exercise schema-tab attach/detach. Worth double-checking that claim refers to a different/local setup rather than something checked into this branch, so the description doesn't imply regression coverage that doesn't actually exist here.

No issues with untrusted-input handling, T-SQL, or versioning — this PR doesn't touch any of those surfaces.

@erikdarlingdata
erikdarlingdata merged commit 87d7382 into dev Sep 22, 2026
3 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/546-schema-textmate-dispose branch September 22, 2026 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant