Repository: refined-github/refined-github
Pull request #7947 (2024-10-24 09:23:39)

Log broken selectors in the observer
- Closes https://github.com/refined-github/refined-github/issues/6096

It's quite noisy because many of these features are "optional", for example on issues often you won't be able to edit _any_ comments, but that doesn't mean that the feature is broken.

It might be a way to detect broken features in 7856 #7886 #7808, but it's probably just noise.

I added it temporarily behind the `logging` flag, let's see how it works.


## Screenshot

<img width="830" alt="Screenshot 23" src="https://github.com/user-attachments/assets/7f502ae0-c73d-42ad-be12-86ef44ee283a">




Patch:
diff --git a/source/helpers/errors.ts b/source/helpers/errors.ts
index f0bca4ae..1df0900d 100644
--- a/source/helpers/errors.ts
+++ b/source/helpers/errors.ts
@@ -14,8 +14,8 @@ const {version} = chrome.runtime.getManifest();
 const fineGrainedTokenSuggestion = 'Please use a GitHub App, OAuth App, or a personal access token with fine-grained permissions.';
 const preferredMessage = 'Refined GitHub does not support per-organization fine-grained tokens. https://github.com/refined-github/refined-github/wiki/Security';
 
-// reads from path like assets/features/NAME.js
-function parseFeatureNameFromStack(stack: string): FeatureID | undefined {
+// Reads from path like assets/features/NAME.js
+export function parseFeatureNameFromStack(stack: string = new Error('stack').stack!): FeatureID | undefined {
 	// The stack may show other features due to cross-feature imports, but we want the top-most caller so we need to reverse it
 	const match = stack
 		.split('\n')
diff --git a/source/helpers/event-listener-loop.ts b/source/helpers/event-listener-loop.ts
new file mode 100644
index 00000000..2514c7ab
--- /dev/null
+++ b/source/helpers/event-listener-loop.ts
@@ -0,0 +1,39 @@
+import pDefer from 'p-defer';
+
+async function* createEventIterator<T extends Event>(
+	element: EventTarget,
+	eventName: string,
+	{signal, once}: {
+		signal?: AbortSignal;
+		once?: boolean;
+	} = {},
+): AsyncGenerator<T> {
+	const queue: T[] = [];
+	let deferred = pDefer<void>();
+
+	const handler = (event: Event): void => {
+		queue.push(event as T);
+		deferred.resolve();
+		deferred = pDefer();
+	};
+
+	try {
+		element.addEventListener(eventName, handler, {once});
+
+		while (!signal?.aborted) {
+			if (queue.length === 0) {
+				// eslint-disable-next-line no-await-in-loop
+				await deferred.promise;
+			}
+			yield queue.shift()!;
+
+			if (once) {
+				break;
+			}
+		}
+	} finally {
+		element.removeEventListener(eventName, handler);
+	}
+}
+
+export default createEventIterator;
diff --git a/source/helpers/selector-observer.tsx b/source/helpers/selector-observer.tsx
index 8f65bfa2..6c022e7b 100644
--- a/source/helpers/selector-observer.tsx
+++ b/source/helpers/selector-observer.tsx
@@ -6,7 +6,9 @@ import delay from 'delay';
 import domLoaded from 'dom-loaded';
 import {signalFromPromise} from 'abort-utils';
 
+import optionsStorage from '../options-storage.js';
 import getCallerID from './caller-id.js';
+import {parseFeatureNameFromStack} from './errors.js';
 
 type ObserverListener<ExpectedElement extends Element> = (element: ExpectedElement, options: SignalAsOptions) => void;
 
@@ -17,26 +19,6 @@ type Options = {
 };
 
 const animation = 'rgh-selector-observer';
-const getListener = <
-	Selector extends string,
-	ExpectedElement extends ParseSelector<Selector, HTMLElement | SVGElement>,
->(
-	seenMark: string,
-	selector: Selector,
-	callback: ObserverListener<ExpectedElement>,
-	signal?: AbortSignal,
-) => (event: AnimationEvent) => {
-	const target = event.target as ExpectedElement;
-	// The target can match a selector even if the animation actually happened on a ::before pseudo-element, so it needs an explicit exclusion here
-	if (target.classList.contains(seenMark) || !target.matches(selector)) {
-		return;
-	}
-
-	// Removes this specific selector’s animation once it was seen
-	target.classList.add(seenMark);
-
-	callback(target, {signal});
-};
 
 const registerAnimation = onetime((): void => {
 	document.head.append(<style>{`@keyframes ${animation} {}`}</style>);
@@ -63,7 +45,7 @@ export default function observe<
 		signal = signal ? AbortSignal.any([signal, delayedDomReady]) : delayedDomReady;
 	}
 
-	const selector = String(selectors); // Array#toString() creates a comma-separated string
+	const selector = typeof selectors === 'string' ? selectors : selectors.join(',\n');
 	const seenMark = 'rgh-seen-' + getCallerID();
 
 	registerAnimation();
@@ -84,7 +66,36 @@ export default function observe<
 	signal?.addEventListener('abort', () => {
 		rule.remove();
 	});
-	globalThis.addEventListener('animationstart', getListener(seenMark, selector, listener, signal), {once, signal});
+
+	let called = false;
+	// Capture stack outside
+	const currentFeature = parseFeatureNameFromStack();
+	(async () => {
+		const {logging} = await optionsStorage.getAll();
+		if (!logging) {
+			return;
+		}
+
+		await domLoaded;
+		await delay(1000);
+		if (!called && !signal?.aborted) {
+			console.warn(currentFeature, '→ Selector not found on page:', selector);
+		}
+	})();
+	globalThis.addEventListener('animationstart', (event: AnimationEvent) => {
+		const target = event.target as ExpectedElement;
+		// The target can match a selector even if the animation actually happened on a ::before pseudo-element, so it needs an explicit exclusion here
+		if (target.classList.contains(seenMark) || !target.matches(selector)) {
+			return;
+		}
+
+		called = true;
+
+		// Removes this specific selector’s animation once it was seen
+		target.classList.add(seenMark);
+
+		listener(target, {signal});
+	}, {once, signal});
 }
 
 // Untested


Test patch:
diff --git a/source/helpers/event-listener-loop.test.ts b/source/helpers/event-listener-loop.test.ts
new file mode 100644
index 00000000..df7ca777
--- /dev/null
+++ b/source/helpers/event-listener-loop.test.ts
@@ -0,0 +1,74 @@
+import {describe, it, expect} from 'vitest';
+
+import createEventIterator from './event-listener-loop.js';
+
+describe('createEventIterator', () => {
+	it('should yield events when they occur', async () => {
+		const target = new EventTarget();
+		const eventName = 'test-event';
+		const iterator = createEventIterator(target, eventName);
+		const event = new Event(eventName);
+
+		setTimeout(() => target.dispatchEvent(event), 22);
+
+		const result = await iterator.next();
+		expect(result.value).toBe(event);
+		expect(result.done).toBe(false);
+	});
+
+	it('should stop yielding events when signal is aborted', async () => {
+		const target = new EventTarget();
+		const eventName = 'test-event';
+		const controller = new AbortController();
+		const iterator = createEventIterator(target, eventName, {signal: controller.signal});
+		const event = new Event(eventName);
+
+		setTimeout(() => {
+			target.dispatchEvent(event);
+			controller.abort();
+		}, 22);
+
+		const result = await iterator.next();
+		expect(result.value).toBe(event);
+		expect(result.done).toBe(false);
+
+		const abortedResult = await iterator.next();
+		expect(abortedResult.value).toBe(undefined);
+		expect(abortedResult.done).toBe(true);
+	});
+
+	it('should handle multiple events', async () => {
+		const target = new EventTarget();
+		const eventName = 'test-event';
+		const iterator = createEventIterator(target, eventName);
+		const event1 = new Event(eventName);
+		const event2 = new Event(eventName);
+
+		setTimeout(() => {
+			target.dispatchEvent(event1);
+			target.dispatchEvent(event2);
+		}, 22);
+
+		const result1 = await iterator.next();
+		expect(result1.value).toBe(event1);
+		expect(result1.done).toBe(false);
+
+		const result2 = await iterator.next();
+		expect(result2.value).toBe(event2);
+		expect(result2.done).toBe(false);
+	});
+
+	it.skip('should handle synchronous events', async () => {
+		const target = new EventTarget();
+		const eventName = 'test-event';
+		const iterator = createEventIterator(target, eventName);
+		const event = new Event(eventName);
+
+		// This fails because native async generator functions aren't actually executed when called…
+		target.dispatchEvent(event);
+
+		const result = await iterator.next();
+		expect(result.value).toBe(event);
+		expect(result.done).toBe(false);
+	});
+});