refactor(log-viewer): tie global listeners to each view's connected life - #1048
Merged
lcottercertinia merged 7 commits intoSep 18, 2026
Merged
Conversation
Five views each declared an identical find event type, and a sixth copy sat in the flame chart's types. One module now names them, keyed by event name so a listener's payload follows the name it subscribed to. Also drops a SearchOptions that collided by name with a different SearchOptions in the same feature folder.
…window Subscribes in hostConnected and unsubscribes in hostDisconnected, so a listener on a global target lives exactly as long as its host is connected. Handlers are passed as inline arrows: this package compiles with useDefineForClassFields:false, so a field initialiser cannot reference a field declared below it, and addEventListener with an undefined handler is a silent no-op. The constructor throws instead.
Seven views registered a global listener in the constructor and removed it in disconnectedCallback, so a re-attached view heard nothing. FindWidget never removed either of its two. Each view's event subset is unchanged. The adapter fields and the casts they needed are gone, and FindWidget now reads the shared results type rather than its own copy carrying a count no view sends.
The three tab views wired the inspector in their constructor and released it in disconnectedCallback, so a re-attached view lost mark, reveal and clear. The call moves beside wireCategoryColoring, which already had this shape, and all three constructors are now empty and gone. AnalysisView's suite mounted a bare element and relied on the constructor doing the wiring; it now attaches to the document and awaits updateComplete.
Detaching the view destroys its three tables, but the only build path runs when the log first arrives, so the view came back with its subscriptions and no table. It now rebuilds on connect, for the view on show, under the filters and grouping it was left with.
…llers # Conflicts: # log-viewer/src/features/analysis/components/AnalysisView.ts # log-viewer/src/features/call-tree/components/CalltreeView.ts # log-viewer/src/features/database/components/DMLView.ts # log-viewer/src/features/database/components/SOQLView.ts # log-viewer/src/features/database/components/SOSLView.ts
This was referenced Sep 17, 2026
lcottercertinia
marked this pull request as draft
September 18, 2026 12:30
lcottercertinia
pushed a commit
that referenced
this pull request
Sep 18, 2026
… the inspector a connected life (#1062) # 📝 PR Overview Three commits that sit on top of #1048: the grids' columns get one owner, the inspector wiring follows the host's connected life, and a visibility wait releases its listener when it resolves. Stacked on #1048 — merge that one first. ## 🛠️ Changes made - **One owner for the grids' columns.** The column definitions were built per grid; they now come from one place, so a column added or renamed lands everywhere at once. - **Inspector wiring on the connected lifetime.** `InspectorTabController` ties a tab's inspector wiring to the host's connected life, alongside the listener controllers #1048 introduces, so nothing outlives the view that asked for it. - **A visibility wait releases its listener.** The wait resolved but left its listener attached. ## 🧩 Type of change (check all applicable) - [x] 🐛 Bug fix - something not working as expected - [x] ♻️ Refactor - internal changes with no user impact ## 🔗 Related Issues _None._ ## ✅ Tests added? - [x] 👍 yes `InspectorTabController.test.ts` and the shared `controllerHostStub`. ## 📚 Docs updated? - [x] 🙅 not needed
lcottercertinia
marked this pull request as ready for review
September 18, 2026 17:03
# Conflicts: # log-viewer/src/features/database/components/DatabaseView.ts # log-viewer/src/features/database/components/__tests__/DatabaseView.test.ts # log-viewer/src/features/timeline/optimised/ApexLogTimeline.ts
lcottercertinia
approved these changes
Sep 18, 2026
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.
Six views wired global
documentlisteners by hand: added in the constructor,removed in
disconnectedCallback. The constructor runs once anddisconnectedCallbackruns on every detach, so a view that came back was deaf.This was latent, not live:
vscode-tabssetsel.hiddenrather than detaching,and
LogViewerrenders all four panels unconditionally. A conditionally renderedchild gets a brand-new element instead, so its constructor re-runs. It becomes a
real defect the moment anything is wrapped in
cache(), rendered lazily, or moved.Four further sites added a listener and never removed one.
What changed
core/events/DomListenerController.ts— aReactiveControllertaking(host, target, handlersMap). It subscribes inhostConnectedand unsubscribesin
hostDisconnected, so a listener lives exactly as long as the host.features/find/findEvents.ts— one home for the find event types, replacing thecopies in
flamechart.types.tsand fiveFindEvtaliases. That also removes aSearchOptionsthat collided by name with a structurally different one.behaviour, not sloppiness: the three Database grids skip
lv-find-matchbecausenavigation reaches them through
DatabaseView.highlightIndex, andDatabaseViewskips
lv-find-close.FindWidget's two leaked listeners are closed.Handlers are passed as inline arrows, and the controller throws a
TypeErrorfor anything else.
useDefineForClassFieldsisfalsein bothjest.config.jsandlog-viewer/tsconfig.json, so field initialisers run in declaration order and acontroller near the top of a class reads
undefinedfor a handler field declaredbelow it — and
addEventListener(name, undefined)is a legal silent no-op.The defect this makes reachable
CalltreeViewdestroys its three tables indisconnectedCallback, but the onlybuild path is gated on the log's first arrival, so a re-attached view came back with
its subscriptions restored and no table. It now rebuilds on connect, for the view
mode on show, under the filters and grouping it was left with. A build overtaken by
a detach no longer wires the table that replaced it, and a visibility wait that
resolved as the view left no longer builds for a detached host.
Not taken here
LogViewer.tscarries the comment "Not really happy this is here, find needs arefactor" beside a synthetic
lv-finddispatched on every tab change. The real fixis
find:*entries in the typedcore/events/EventBus.ts, which would delete thestring literals, the casts and that hack. This controller is a stepping stone, not
the destination.
Timeline.tsandApexLogTimelinestill wire by hand — neither is aReactiveControllerHost. TheTimeline.tsleak is fixed separately.Testing
New suites for the controller and for
CalltreeView's table lifetime, plusUtil.isVisible. Every guard was proven by removing it and watching the matchingtest fail. Full suite green.
Checked by hand in the dev host: find in every tab, next/previous, the counter,
Escape, and that switching tabs clears the other tabs' highlights.