Skip to content

Commit 1e04518

Browse files
lxsmnsycclaude
andcommitted
fix: keep the graph watching after View in graph
Opening the graph from the ownership panel stopped its live updates. Each panel installed the dev hooks itself, kept the hooks it found, and put them back when it stopped. View in graph starts the graph and stops the ownership panel in the same flush. The graph installed first, then the ownership panel put back the hooks it had found earlier, which removed the graph's hooks. - A shared hub in `dev-hooks.ts` installs the hooks once and calls every listener. It still calls the hooks it found, and puts them back only when the last listener leaves. - Both registries listen through the hub, so panels can start and stop in any order. - Unit tests cover a listener that keeps working when an older one leaves, and the e2e test updates the app after the jump and checks the graph shows it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 4c92a91 commit 1e04518

5 files changed

Lines changed: 228 additions & 74 deletions

File tree

‎src/dev-toolbar/dev-hooks.test.ts‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
import { describe, expect, it, vi } from 'vitest';
2+
import { createDevHooksHub, type DevHooksSlot } from './dev-hooks.js';
3+
4+
describe('createDevHooksHub', () => {
5+
it('leaves the slot alone until something listens', () => {
6+
const original = () => {};
7+
const slot: DevHooksSlot = { onUpdate: original };
8+
9+
createDevHooksHub(slot);
10+
11+
expect(slot.onUpdate).toBe(original);
12+
});
13+
14+
it('calls the hooks it found and every listener', () => {
15+
const found = vi.fn();
16+
const slot: DevHooksSlot = { onUpdate: found };
17+
const hub = createDevHooksHub(slot);
18+
const first = vi.fn();
19+
const second = vi.fn();
20+
21+
hub.listen({ onUpdate: first });
22+
hub.listen({ onUpdate: second });
23+
slot.onUpdate!();
24+
25+
expect(found).toHaveBeenCalledTimes(1);
26+
expect(first).toHaveBeenCalledTimes(1);
27+
expect(second).toHaveBeenCalledTimes(1);
28+
});
29+
30+
// Opening the graph from the ownership panel starts one listener and stops
31+
// another in the same flush. The listener that started must keep working.
32+
it('keeps a listener working when an older one leaves', () => {
33+
const slot: DevHooksSlot = {};
34+
const hub = createDevHooksHub(slot);
35+
const tree = vi.fn();
36+
const graph = vi.fn();
37+
38+
const stopTree = hub.listen({ onOwner: tree });
39+
hub.listen({ onOwner: graph });
40+
stopTree();
41+
slot.onOwner!({});
42+
43+
expect(graph).toHaveBeenCalledTimes(1);
44+
expect(tree).not.toHaveBeenCalled();
45+
});
46+
47+
it('puts the original hooks back when the last listener leaves', () => {
48+
const original = () => {};
49+
const slot: DevHooksSlot = { onGraph: original };
50+
const hub = createDevHooksHub(slot);
51+
52+
const stopFirst = hub.listen({});
53+
const stopSecond = hub.listen({});
54+
stopFirst();
55+
expect(slot.onGraph).not.toBe(original);
56+
stopSecond();
57+
58+
expect(slot.onGraph).toBe(original);
59+
});
60+
61+
it('ignores a stop called twice', () => {
62+
const slot: DevHooksSlot = {};
63+
const hub = createDevHooksHub(slot);
64+
const graph = vi.fn();
65+
66+
const stop = hub.listen({});
67+
hub.listen({ onUpdate: graph });
68+
stop();
69+
stop();
70+
slot.onUpdate!();
71+
72+
expect(graph).toHaveBeenCalledTimes(1);
73+
});
74+
75+
it('installs again after every listener left', () => {
76+
const slot: DevHooksSlot = {};
77+
const hub = createDevHooksHub(slot);
78+
const graph = vi.fn();
79+
80+
hub.listen({})();
81+
hub.listen({ onOwner: graph });
82+
slot.onOwner!({});
83+
84+
expect(graph).toHaveBeenCalledTimes(1);
85+
});
86+
});

‎src/dev-toolbar/dev-hooks.ts‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
import { DEV } from 'solid-js';
2+
3+
/** The dev hooks of solid-js the toolbar uses. */
4+
export interface DevHooksSlot {
5+
onOwner?: (owner: any) => void;
6+
onGraph?: (value: any, owner: any) => void;
7+
onUpdate?: () => void;
8+
}
9+
10+
export type DevHooksListener = DevHooksSlot;
11+
12+
export interface DevHooksHub {
13+
/** Adds a listener. The returned function removes it. */
14+
listen(listener: DevHooksListener): () => void;
15+
}
16+
17+
/**
18+
* Shares one set of dev hooks between every panel.
19+
*
20+
* Each panel used to install the hooks itself, keeping the hooks it found and
21+
* putting them back when it stopped. When one panel started and another stopped
22+
* in the same flush, the second put back hooks that no longer belonged there,
23+
* and the first panel stopped seeing updates.
24+
*
25+
* The hub installs once and calls every listener. It keeps calling the hooks it
26+
* found, so other tools sharing the slot still work, and puts them back only
27+
* when the last listener leaves.
28+
*/
29+
export function createDevHooksHub(slot: DevHooksSlot): DevHooksHub {
30+
const listeners = new Set<DevHooksListener>();
31+
let original: DevHooksSlot | undefined;
32+
33+
function install(): void {
34+
const found: DevHooksSlot = {
35+
onOwner: slot.onOwner,
36+
onGraph: slot.onGraph,
37+
onUpdate: slot.onUpdate,
38+
};
39+
original = found;
40+
slot.onOwner = (owner) => {
41+
found.onOwner?.(owner);
42+
for (const listener of listeners) listener.onOwner?.(owner);
43+
};
44+
slot.onGraph = (value, owner) => {
45+
found.onGraph?.(value, owner);
46+
for (const listener of listeners) listener.onGraph?.(value, owner);
47+
};
48+
slot.onUpdate = () => {
49+
found.onUpdate?.();
50+
for (const listener of listeners) listener.onUpdate?.();
51+
};
52+
}
53+
54+
function uninstall(): void {
55+
if (!original) return;
56+
slot.onOwner = original.onOwner;
57+
slot.onGraph = original.onGraph;
58+
slot.onUpdate = original.onUpdate;
59+
original = undefined;
60+
}
61+
62+
return {
63+
listen(listener) {
64+
listeners.add(listener);
65+
if (!original) install();
66+
return () => {
67+
if (!listeners.delete(listener)) return;
68+
if (listeners.size === 0) uninstall();
69+
};
70+
},
71+
};
72+
}
73+
74+
let shared: DevHooksHub | undefined;
75+
76+
/** Listens to the dev hooks of solid-js. Does nothing outside a development build. */
77+
export function listenToDevHooks(listener: DevHooksListener): () => void {
78+
if (!DEV) return () => {};
79+
shared ??= createDevHooksHub(DEV.hooks);
80+
return shared.listen(listener);
81+
}

‎src/dev-toolbar/ownership/registry.ts‎

Lines changed: 23 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { DEV } from 'solid-js';
2+
import { listenToDevHooks } from '../dev-hooks.js';
23
import { buildOwnershipTree, EMPTY_TREE, type OwnershipTree, type RawNode } from './tree.js';
34

45
let nextId = 1;
@@ -15,7 +16,7 @@ const collected =
1516
: undefined;
1617

1718
const listeners = new Set<() => void>();
18-
let uninstall: (() => void) | undefined;
19+
let stopListening: (() => void) | undefined;
1920
let watchers = 0;
2021
let frame: number | undefined;
2122
/** An owner inside the toolbar. The walk climbs from here to the app root. */
@@ -52,49 +53,36 @@ function notify(): void {
5253
}
5354

5455
/**
55-
* Installs the devtools hooks on the reactive runtime. Existing hooks are kept
56-
* and still called, so other tools sharing the slot keep working.
56+
* Starts watching the reactive runtime. The hooks are shared with the other
57+
* panels through one hub, so panels can start and stop in any order.
5758
*/
5859
export function startOwnershipTracking(): () => void {
5960
if (!isOwnershipAvailable()) return () => {};
6061
watchers++;
61-
if (uninstall) return release;
62-
63-
const hooks = DEV!.hooks;
64-
const previousOwner = hooks.onOwner;
65-
const previousGraph = hooks.onGraph;
66-
const previousUpdate = hooks.onUpdate;
67-
68-
hooks.onOwner = (owner) => {
69-
previousOwner?.(owner);
70-
track(owner as RawNode);
71-
notify();
72-
};
73-
hooks.onGraph = (value, owner) => {
74-
previousGraph?.(value, owner);
75-
if (owner) track(owner as RawNode);
76-
notify();
77-
};
78-
hooks.onUpdate = () => {
79-
previousUpdate?.();
80-
notify();
81-
};
82-
83-
uninstall = () => {
84-
hooks.onOwner = previousOwner;
85-
hooks.onGraph = previousGraph;
86-
hooks.onUpdate = previousUpdate;
87-
uninstall = undefined;
88-
if (frame !== undefined) cancelAnimationFrame(frame);
89-
frame = undefined;
90-
};
62+
stopListening ??= listenToDevHooks({
63+
onOwner(owner) {
64+
track(owner as RawNode);
65+
notify();
66+
},
67+
onGraph(_value, owner) {
68+
if (owner) track(owner as RawNode);
69+
notify();
70+
},
71+
onUpdate() {
72+
notify();
73+
},
74+
});
9175
return release;
9276
}
9377

94-
/** Drops one watcher. The hooks come off once nothing watches any more. */
78+
/** Drops one watcher. Stops listening once nothing watches any more. */
9579
function release(): void {
9680
watchers = Math.max(0, watchers - 1);
97-
if (watchers === 0) uninstall?.();
81+
if (watchers > 0 || !stopListening) return;
82+
stopListening();
83+
stopListening = undefined;
84+
if (frame !== undefined) cancelAnimationFrame(frame);
85+
frame = undefined;
9886
}
9987

10088
/** Calls `listener` after the tree changed, at most once per frame. */

‎src/dev-toolbar/reactivity/registry.ts‎

Lines changed: 27 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { DEV } from 'solid-js';
2+
import { listenToDevHooks } from '../dev-hooks.js';
23

34
// Flag bits used by @solidjs/signals. They are internal to the runtime, so the
45
// values are copied here and every read is defensive.
@@ -99,7 +100,7 @@ const collected =
99100
: undefined;
100101

101102
const listeners = new Set<() => void>();
102-
let uninstall: (() => void) | undefined;
103+
let stopListening: (() => void) | undefined;
103104
let watchers = 0;
104105
let frame: number | undefined;
105106
/** An owner inside the toolbar. The graph walk climbs from here to the app root. */
@@ -141,46 +142,29 @@ function notify(): void {
141142
}
142143

143144
/**
144-
* Installs the devtools hooks on the reactive runtime. Existing hooks are kept
145-
* and still called, so other tools sharing the slot keep working.
145+
* Starts watching the reactive runtime. The hooks are shared with the other
146+
* panels through one hub, so panels can start and stop in any order.
146147
*/
147148
export function startReactivityTracking(): () => void {
148149
if (!isReactivityAvailable()) return () => {};
149150
watchers++;
150-
if (uninstall) return release;
151-
152-
const hooks = DEV!.hooks;
153-
const previousOwner = hooks.onOwner;
154-
const previousGraph = hooks.onGraph;
155-
const previousUpdate = hooks.onUpdate;
156-
157-
hooks.onOwner = (owner) => {
158-
previousOwner?.(owner);
159-
track(owner as RawNode);
160-
notify();
161-
};
162-
hooks.onGraph = (value, owner) => {
163-
previousGraph?.(value, owner);
164-
if (value && typeof value === 'object') {
165-
if (owner) signalOwners.set(value, owner as RawNode);
166-
track(value as RawNode);
167-
}
168-
notify();
169-
};
170-
hooks.onUpdate = () => {
171-
previousUpdate?.();
172-
recordUpdates();
173-
notify();
174-
};
175-
176-
uninstall = () => {
177-
hooks.onOwner = previousOwner;
178-
hooks.onGraph = previousGraph;
179-
hooks.onUpdate = previousUpdate;
180-
uninstall = undefined;
181-
if (frame !== undefined) cancelAnimationFrame(frame);
182-
frame = undefined;
183-
};
151+
stopListening ??= listenToDevHooks({
152+
onOwner(owner) {
153+
track(owner as RawNode);
154+
notify();
155+
},
156+
onGraph(value, owner) {
157+
if (value && typeof value === 'object') {
158+
if (owner) signalOwners.set(value, owner as RawNode);
159+
track(value as RawNode);
160+
}
161+
notify();
162+
},
163+
onUpdate() {
164+
recordUpdates();
165+
notify();
166+
},
167+
});
184168
return release;
185169
}
186170

@@ -197,10 +181,14 @@ function recordUpdates(): void {
197181
}
198182
}
199183

200-
/** Drops one watcher. The hooks come off once nothing watches any more. */
184+
/** Drops one watcher. Stops listening once nothing watches any more. */
201185
function release(): void {
202186
watchers = Math.max(0, watchers - 1);
203-
if (watchers === 0) uninstall?.();
187+
if (watchers > 0 || !stopListening) return;
188+
stopListening();
189+
stopListening = undefined;
190+
if (frame !== undefined) cancelAnimationFrame(frame);
191+
frame = undefined;
204192
}
205193

206194
/** Calls `listener` after the graph changed, at most once per frame. */

‎tests/e2e/devtools.spec.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,17 @@ test('maps the ownership tree', async ({ page }) => {
228228
return Math.abs(nodeBox.x + nodeBox.width / 2 - (canvasBox.x + canvasBox.width / 2));
229229
})
230230
.toBeLessThan(4);
231+
232+
// The graph keeps watching after the jump. The ownership panel stops in the
233+
// same flush the graph starts, which used to take the graph's hooks away.
234+
// The panel covers the page, so the click goes straight to the element.
235+
await page.evaluate(() => (document.querySelector('#increment-count') as HTMLElement).click());
236+
await expect(
237+
page
238+
.locator('[data-solid-reactivity-node]')
239+
.filter({ hasText: /^count/ })
240+
.first(),
241+
).toContainText('1');
231242
});
232243

233244
test('mounts once and disposes', async ({ page }) => {

0 commit comments

Comments
 (0)