Skip to content

refactor(log-viewer): tie global listeners to each view's connected life - #1048

Merged
lcottercertinia merged 7 commits into
certinia:mainfrom
lukecotter:refactor-lit-controllers
Sep 18, 2026
Merged

lcottercertinia merged 7 commits into
certinia:mainfrom
lukecotter:refactor-lit-controllers

Conversation

@lukecotter

Copy link
Copy Markdown
Collaborator

Six views wired global document listeners by hand: added in the constructor,
removed in disconnectedCallback. The constructor runs once and
disconnectedCallback runs on every detach, so a view that came back was deaf.

This was latent, not live: vscode-tabs sets el.hidden rather than detaching,
and LogViewer renders all four panels unconditionally. A conditionally rendered
child 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 — a ReactiveController taking
    (host, target, handlersMap). It subscribes in hostConnected and unsubscribes
    in hostDisconnected, so a listener lives exactly as long as the host.
  • features/find/findEvents.ts — one home for the find event types, replacing the
    copies in flamechart.types.ts and five FindEvt aliases. That also removes a
    SearchOptions that collided by name with a structurally different one.
  • Seven views converted. Per-view event subsets are preserved exactly — they are
    behaviour, not sloppiness: the three Database grids skip lv-find-match because
    navigation reaches them through DatabaseView.highlightIndex, and DatabaseView
    skips lv-find-close.
  • FindWidget's two leaked listeners are closed.

Handlers are passed as inline arrows, and the controller throws a TypeError
for anything else. useDefineForClassFields is false in both jest.config.js and
log-viewer/tsconfig.json, so field initialisers run in declaration order and a
controller near the top of a class reads undefined for a handler field declared
below it — and addEventListener(name, undefined) is a legal silent no-op.

The defect this makes reachable

CalltreeView destroys its three tables in disconnectedCallback, but the only
build 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.ts carries the comment "Not really happy this is here, find needs a
refactor"
beside a synthetic lv-find dispatched on every tab change. The real fix
is find:* entries in the typed core/events/EventBus.ts, which would delete the
string literals, the casts and that hack. This controller is a stepping stone, not
the destination.

Timeline.ts and ApexLogTimeline still wire by hand — neither is a
ReactiveControllerHost. The Timeline.ts leak is fixed separately.

Testing

New suites for the controller and for CalltreeView's table lifetime, plus
Util.isVisible. Every guard was proven by removing it and watching the matching
test 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.

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
@lcottercertinia
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
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
lcottercertinia merged commit 04c4f29 into certinia:main Sep 18, 2026
9 checks passed
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.

2 participants