Skip to content

Commit 960b764

Browse files
authored
Auto-cleanup orphaned tooltips on DOM detachment (#134)
1 parent 085e480 commit 960b764

5 files changed

Lines changed: 244 additions & 9 deletions

File tree

.gitignore

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,5 @@ node_modules
33
dist
44
web
55
.docs
6-
.husky/_
6+
.husky/_
7+
scripts/

src/managers/message-component-manager.test.ts

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,12 @@ describe('message-component-manager', () => {
296296
await showTooltipComponent(message);
297297

298298
const tooltipElement = document.querySelector('.gist-tooltip-outer');
299-
expect(positionTooltip).toHaveBeenCalledWith(tooltipElement, '#target-el', 'top');
299+
expect(positionTooltip).toHaveBeenCalledWith(
300+
tooltipElement,
301+
'#target-el',
302+
'top',
303+
expect.objectContaining({ onDetach: expect.any(Function) })
304+
);
300305
});
301306

302307
it('defaults tooltip position to bottom when not specified', async () => {
@@ -328,7 +333,12 @@ describe('message-component-manager', () => {
328333

329334
await showTooltipComponent(message);
330335

331-
expect(positionTooltip).toHaveBeenCalledWith(expect.any(HTMLElement), '#target-el', 'bottom');
336+
expect(positionTooltip).toHaveBeenCalledWith(
337+
expect.any(HTMLElement),
338+
'#target-el',
339+
'bottom',
340+
expect.objectContaining({ onDetach: expect.any(Function) })
341+
);
332342
});
333343

334344
it('returns false when positionTooltip returns null (target not found)', async () => {
@@ -426,6 +436,34 @@ describe('message-component-manager', () => {
426436
expect(container?.classList.contains('gist-visible')).toBe(false);
427437
});
428438

439+
it('removes wrapper and clears handle when onDetach is invoked by position manager', async () => {
440+
let capturedOnDetach: (() => void) | undefined;
441+
const mockCleanup = vi.fn();
442+
vi.mocked(positionTooltip).mockImplementation((_el, _sel, _pos, options) => {
443+
capturedOnDetach = options?.onDetach;
444+
return { cleanup: mockCleanup, reposition: vi.fn() };
445+
});
446+
447+
setupTooltipWrapper('inst-1');
448+
const message: GistMessage = {
449+
messageId: 'msg-1',
450+
instanceId: 'inst-1',
451+
properties: { gist: { elementId: '#target-el' } },
452+
};
453+
454+
const result = await showTooltipComponent(message);
455+
expect(result).toBe(true);
456+
expect(document.getElementById('gist-tooltip-inst-1')).not.toBeNull();
457+
458+
capturedOnDetach!();
459+
460+
expect(document.getElementById('gist-tooltip-inst-1')).toBeNull();
461+
462+
// Subsequent hide should not call cleanup again since the map entry was cleared
463+
hideTooltipComponent(message);
464+
expect(mockCleanup).not.toHaveBeenCalled();
465+
});
466+
429467
it('returns false without calling positionTooltip when ensureTargetInView returns false', async () => {
430468
vi.mocked(ensureTargetInView).mockResolvedValue(false);
431469
setupTooltipWrapper('inst-1');

src/managers/message-component-manager.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,16 @@ export async function showTooltipComponent(message: GistMessage): Promise<boolea
292292
return false;
293293
}
294294

295-
const handle = positionTooltip(tooltipElement, selector, position);
295+
const handle = positionTooltip(tooltipElement, selector, position, {
296+
onDetach: () => {
297+
tooltipHandleMap.delete(instanceId);
298+
const w = findElement(wrapperId);
299+
if (w) {
300+
w.parentNode?.removeChild(w);
301+
}
302+
},
303+
});
304+
296305
if (handle) {
297306
const isVisible = tooltipElement.style.display !== 'none';
298307
if (isVisible) {

src/managers/tooltip-position-manager.test.ts

Lines changed: 155 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -377,48 +377,199 @@ describe('tooltip-position-manager', () => {
377377
expect(result).toBeNull();
378378
});
379379

380-
it('stops repositioning when target is removed from DOM after initial positioning', () => {
380+
it('calls onDetach and cleans up when target is removed from DOM after initial positioning', () => {
381381
let rafCallback: FrameRequestCallback | null = null;
382382
vi.stubGlobal('requestAnimationFrame', (cb: FrameRequestCallback) => {
383383
rafCallback = cb;
384384
return 1;
385385
});
386386

387+
const onDetach = vi.fn();
387388
const target = createTarget({});
388389
const tooltip = createTooltip({ width: 120, height: 50 });
389-
handle = positionTooltip(tooltip, '#target', 'bottom');
390+
handle = positionTooltip(tooltip, '#target', 'bottom', { onDetach });
390391

391392
target.remove();
392393

393394
window.dispatchEvent(new Event('scroll'));
394395
rafCallback!(0);
395396

397+
expect(onDetach).toHaveBeenCalledTimes(1);
396398
expect(log).toHaveBeenCalledWith(
397399
'Tooltip or target element removed from DOM, cleaning up listeners'
398400
);
399401
});
400402

401-
it('stops repositioning when tooltip is removed from DOM', () => {
403+
it('calls onDetach and cleans up when tooltip is removed from DOM', () => {
402404
let rafCallback: FrameRequestCallback | null = null;
403405
vi.stubGlobal('requestAnimationFrame', (cb: FrameRequestCallback) => {
404406
rafCallback = cb;
405407
return 1;
406408
});
407409

410+
const onDetach = vi.fn();
408411
createTarget({});
409412
const tooltip = createTooltip({ width: 120, height: 50 });
410-
handle = positionTooltip(tooltip, '#target', 'bottom');
413+
handle = positionTooltip(tooltip, '#target', 'bottom', { onDetach });
411414

412415
tooltip.remove();
413416

414417
window.dispatchEvent(new Event('scroll'));
415418
rafCallback!(0);
416419

420+
expect(onDetach).toHaveBeenCalledTimes(1);
421+
expect(log).toHaveBeenCalledWith(
422+
'Tooltip or target element removed from DOM, cleaning up listeners'
423+
);
424+
});
425+
426+
it('calls onDetach via MutationObserver when target is removed without scroll', async () => {
427+
const onDetach = vi.fn();
428+
const target = createTarget({});
429+
const tooltip = createTooltip({ width: 120, height: 50 });
430+
handle = positionTooltip(tooltip, '#target', 'bottom', { onDetach });
431+
expect(handle).not.toBeNull();
432+
433+
target.remove();
434+
435+
// MutationObserver callbacks are microtasks — flush them
436+
await new Promise((r) => setTimeout(r, 0));
437+
438+
expect(onDetach).toHaveBeenCalledTimes(1);
439+
expect(log).toHaveBeenCalledWith(
440+
'Tooltip or target element removed from DOM, cleaning up listeners'
441+
);
442+
});
443+
444+
it('calls onDetach when target parent is replaced (SPA navigation)', async () => {
445+
const onDetach = vi.fn();
446+
const container = document.createElement('div');
447+
container.id = 'app-root';
448+
document.body.appendChild(container);
449+
450+
const target = document.createElement('div');
451+
target.id = 'spa-target';
452+
container.appendChild(target);
453+
target.getBoundingClientRect = vi.fn(
454+
() =>
455+
({
456+
top: 100,
457+
bottom: 140,
458+
left: 200,
459+
right: 280,
460+
width: 80,
461+
height: 40,
462+
x: 200,
463+
y: 100,
464+
toJSON: () => ({}),
465+
}) as DOMRect
466+
);
467+
468+
const tooltip = createTooltip({ width: 120, height: 50 });
469+
handle = positionTooltip(tooltip, '#spa-target', 'bottom', { onDetach });
470+
expect(handle).not.toBeNull();
471+
472+
container.innerHTML = '<div>new page content</div>';
473+
474+
await new Promise((r) => setTimeout(r, 0));
475+
476+
expect(onDetach).toHaveBeenCalledTimes(1);
477+
expect(log).toHaveBeenCalledWith(
478+
'Tooltip or target element removed from DOM, cleaning up listeners'
479+
);
480+
});
481+
482+
it('cleans up without error when onDetach is not provided', () => {
483+
let rafCallback: FrameRequestCallback | null = null;
484+
vi.stubGlobal('requestAnimationFrame', (cb: FrameRequestCallback) => {
485+
rafCallback = cb;
486+
return 1;
487+
});
488+
489+
const target = createTarget({});
490+
const tooltip = createTooltip({ width: 120, height: 50 });
491+
handle = positionTooltip(tooltip, '#target', 'bottom');
492+
493+
target.remove();
494+
495+
window.dispatchEvent(new Event('scroll'));
496+
expect(() => rafCallback!(0)).not.toThrow();
497+
417498
expect(log).toHaveBeenCalledWith(
418499
'Tooltip or target element removed from DOM, cleaning up listeners'
419500
);
420501
});
421502

503+
it('does not swallow scroll repositioning when an unrelated DOM mutation fires', () => {
504+
const rafCallbacks: FrameRequestCallback[] = [];
505+
let nextRafId = 1;
506+
vi.stubGlobal('requestAnimationFrame', (cb: FrameRequestCallback) => {
507+
rafCallbacks.push(cb);
508+
return nextRafId++;
509+
});
510+
511+
const target = createTarget({});
512+
const tooltip = createTooltip({ width: 120, height: 50 });
513+
handle = positionTooltip(tooltip, '#target', 'bottom');
514+
515+
const callCountBefore = (target.getBoundingClientRect as ReturnType<typeof vi.fn>).mock
516+
.calls.length;
517+
518+
// Trigger an unrelated DOM mutation (target is still attached)
519+
const unrelated = document.createElement('span');
520+
document.body.appendChild(unrelated);
521+
522+
// Flush the MutationObserver microtask so its RAF is queued
523+
// MutationObserver uses a real microtask in jsdom, but we need to
524+
// manually invoke the RAF callback it scheduled.
525+
// Simulate: mutation observer queued a RAF
526+
const mutationRaf = rafCallbacks.pop();
527+
528+
// Now a scroll fires while the mutation RAF is pending
529+
window.dispatchEvent(new Event('scroll'));
530+
531+
// The scroll should have queued its own RAF (not been blocked)
532+
const scrollRaf = rafCallbacks.pop();
533+
expect(scrollRaf).toBeDefined();
534+
535+
// Fire the mutation RAF — target is still attached, so it's a no-op
536+
if (mutationRaf) mutationRaf(0);
537+
538+
// Fire the scroll RAF — this must call update() and reposition
539+
scrollRaf!(0);
540+
541+
const callCountAfter = (target.getBoundingClientRect as ReturnType<typeof vi.fn>).mock.calls
542+
.length;
543+
expect(callCountAfter).toBeGreaterThan(callCountBefore);
544+
545+
unrelated.remove();
546+
});
547+
548+
it('disconnects the MutationObserver on cleanup', async () => {
549+
const disconnectSpy = vi.fn();
550+
const OriginalObserver = globalThis.MutationObserver;
551+
vi.stubGlobal(
552+
'MutationObserver',
553+
class extends OriginalObserver {
554+
disconnect() {
555+
disconnectSpy();
556+
super.disconnect();
557+
}
558+
}
559+
);
560+
561+
createTarget({});
562+
const tooltip = createTooltip({ width: 120, height: 50 });
563+
handle = positionTooltip(tooltip, '#target', 'bottom');
564+
565+
expect(disconnectSpy).not.toHaveBeenCalled();
566+
567+
handle!.cleanup();
568+
569+
expect(disconnectSpy).toHaveBeenCalledTimes(1);
570+
handle = null;
571+
});
572+
422573
it('cleanup is idempotent and can be called multiple times safely', () => {
423574
createTarget({});
424575
const tooltip = createTooltip({ width: 120, height: 50 });

src/managers/tooltip-position-manager.ts

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,15 @@ export interface TooltipHandle {
240240
reposition: () => void;
241241
}
242242

243+
export interface PositionTooltipOptions {
244+
/**
245+
* Called when the position manager detects that the target or tooltip element
246+
* was removed from the DOM unexpectedly (e.g. SPA navigation, dynamic DOM
247+
* mutation). Not called when the consumer invokes `handle.cleanup()` directly.
248+
*/
249+
onDetach?: () => void;
250+
}
251+
243252
/**
244253
* Predicts whether the tooltip can be positioned after the target is scrolled
245254
* into view. Returns true only when the target exists in the DOM and at least
@@ -355,16 +364,19 @@ export async function ensureTargetInView(
355364
export function positionTooltip(
356365
tooltipElement: HTMLElement,
357366
targetSelector: string,
358-
position: TooltipPosition
367+
position: TooltipPosition,
368+
options?: PositionTooltipOptions
359369
): TooltipHandle | null {
360370
const targetElement = findTargetElement(targetSelector);
361371
if (!targetElement) {
362372
return null;
363373
}
364374

365375
let rafId: number | null = null;
376+
let mutationRafId: number | null = null;
366377
let cleaned = false;
367378
let scrollAncestors: Element[] = [];
379+
let observer: MutationObserver | null = null;
368380

369381
try {
370382
scrollAncestors = getScrollableAncestors(targetElement);
@@ -380,6 +392,7 @@ export function positionTooltip(
380392
if (!targetElement || !document.contains(targetElement) || !document.contains(tooltipElement)) {
381393
log(`Tooltip or target element removed from DOM, cleaning up listeners`);
382394
cleanup();
395+
options?.onDetach?.();
383396
return;
384397
}
385398

@@ -419,6 +432,10 @@ export function positionTooltip(
419432
return;
420433
}
421434
cleaned = true;
435+
if (observer) {
436+
observer.disconnect();
437+
observer = null;
438+
}
422439
window.removeEventListener('scroll', onScrollOrResize);
423440
window.removeEventListener('resize', onScrollOrResize);
424441
for (const ancestor of scrollAncestors) {
@@ -428,6 +445,10 @@ export function positionTooltip(
428445
cancelAnimationFrame(rafId);
429446
rafId = null;
430447
}
448+
if (mutationRafId !== null) {
449+
cancelAnimationFrame(mutationRafId);
450+
mutationRafId = null;
451+
}
431452
}
432453

433454
update();
@@ -442,5 +463,20 @@ export function positionTooltip(
442463
ancestor.addEventListener('scroll', onScrollOrResize, { passive: true });
443464
}
444465

466+
try {
467+
observer = new MutationObserver(() => {
468+
if (mutationRafId !== null) return;
469+
mutationRafId = requestAnimationFrame(() => {
470+
mutationRafId = null;
471+
if (!document.contains(targetElement)) {
472+
update();
473+
}
474+
});
475+
});
476+
observer.observe(document.body, { childList: true, subtree: true });
477+
} catch {
478+
// MutationObserver may not be available in some test environments
479+
}
480+
445481
return { cleanup, reposition: update };
446482
}

0 commit comments

Comments
 (0)