mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-08-06 07:21:33 -07:00
fix(overlay): add changelog fetch timeout and harden modal focus guard
{"subject": "fix(overlay): add changelog fetch timeout and harden modal focus guard", "body": "- Wrap changelog fetch/download in an AbortSignal timeout so a stalled request can't leave the modal stuck on \"Loading changelog...\"\n- Focus guard: recover via enforceModalFocus instead of preventDefault (focusin isn't cancelable), try the next preferred target when one refuses focus, and stop leaking pointer listeners if getModalRoot() returns a different element on detach\n- Cover prerelease changelog parsing, the new timeout wrapper, and the focus guard recovery/fallback paths with tests"}
This commit is contained in:
@@ -166,6 +166,14 @@ test('changelog parser reads prerelease and build metadata version headings', ()
|
||||
assert.equal(entries[1]?.date, '2026-05-29');
|
||||
assert.equal(entries[1]?.groupKey, '0.15');
|
||||
assert.equal(entries[0]?.sections.length, 1);
|
||||
// The prerelease body has to land on its own entry, not fold into 0.16.0.
|
||||
assert.deepEqual(entries[1]?.sections, [
|
||||
{
|
||||
heading: 'Added',
|
||||
items: [{ text: 'Release candidate note.', children: [] }],
|
||||
internal: false,
|
||||
},
|
||||
]);
|
||||
});
|
||||
|
||||
test('changelog parser handles the repo CHANGELOG.md', () => {
|
||||
|
||||
@@ -0,0 +1,87 @@
|
||||
import assert from 'node:assert/strict';
|
||||
import test from 'node:test';
|
||||
|
||||
import {
|
||||
CHANGELOG_REQUEST_TIMEOUT_MS,
|
||||
createChangelogRuntime,
|
||||
withRequestTimeout,
|
||||
} from './changelog-runtime';
|
||||
import type { FetchLike, FetchResponseLike } from '../update/release-assets';
|
||||
|
||||
function okResponse(body: string): FetchResponseLike {
|
||||
return {
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => JSON.parse(body),
|
||||
text: async () => body,
|
||||
arrayBuffer: async () => new ArrayBuffer(0),
|
||||
};
|
||||
}
|
||||
|
||||
test('request timeout wrapper attaches an abort signal to every request', async () => {
|
||||
const seen: Array<{ url: string; init?: Record<string, unknown> }> = [];
|
||||
const wrapped = withRequestTimeout(async (url, init) => {
|
||||
seen.push({ url, init });
|
||||
return okResponse('body');
|
||||
}, 1_234);
|
||||
|
||||
await wrapped('https://example.test/a');
|
||||
await wrapped('https://example.test/b', { headers: { 'User-Agent': 'SubMiner' } });
|
||||
|
||||
assert.equal(seen.length, 2);
|
||||
for (const request of seen) {
|
||||
assert.ok(request.init?.signal instanceof AbortSignal, 'each request carries a signal');
|
||||
assert.equal((request.init?.signal as AbortSignal).aborted, false);
|
||||
}
|
||||
// Existing init is preserved rather than replaced.
|
||||
assert.deepEqual(seen[1]?.init?.headers, { 'User-Agent': 'SubMiner' });
|
||||
});
|
||||
|
||||
test('request timeout wrapper aborts a request that never settles', async () => {
|
||||
let observed: AbortSignal | undefined;
|
||||
const wrapped = withRequestTimeout((_url, init) => {
|
||||
observed = init?.signal as AbortSignal;
|
||||
return new Promise<FetchResponseLike>(() => {
|
||||
// Never resolves, standing in for a stalled connection.
|
||||
});
|
||||
}, 10);
|
||||
|
||||
void wrapped('https://example.test/stalled');
|
||||
await new Promise((resolve) => setTimeout(resolve, 40));
|
||||
|
||||
assert.equal(observed?.aborted, true);
|
||||
});
|
||||
|
||||
test('changelog runtime times out both requests instead of hanging the modal', async () => {
|
||||
const signals: Array<AbortSignal | undefined> = [];
|
||||
const failing: FetchLike = (_url, init) => {
|
||||
signals.push(init?.signal as AbortSignal | undefined);
|
||||
return Promise.reject(new Error('stalled'));
|
||||
};
|
||||
|
||||
const runtime = createChangelogRuntime({
|
||||
getInstalledVersion: () => '0.19.2',
|
||||
getUpdateChannel: () => 'stable',
|
||||
resourcesPath: '/res',
|
||||
appPath: '/app',
|
||||
dirname: '/app/dist/main',
|
||||
joinPath: (...parts) => parts.join('/'),
|
||||
fileExists: () => false,
|
||||
readFile: () => '',
|
||||
logWarn: () => {},
|
||||
createFetch: () => failing,
|
||||
});
|
||||
|
||||
const snapshot = await runtime.getChangelogSnapshot();
|
||||
|
||||
// Release lookup and changelog download both go through the timeout wrapper.
|
||||
assert.equal(signals.length, 2);
|
||||
assert.ok(signals.every((signal) => signal instanceof AbortSignal));
|
||||
// No bundled copy is readable here, so the failure surfaces rather than hangs.
|
||||
assert.match(snapshot.error ?? '', /stalled/);
|
||||
});
|
||||
|
||||
test('changelog request timeout is finite', () => {
|
||||
assert.ok(Number.isFinite(CHANGELOG_REQUEST_TIMEOUT_MS));
|
||||
assert.ok(CHANGELOG_REQUEST_TIMEOUT_MS > 0);
|
||||
});
|
||||
@@ -19,14 +19,30 @@ export interface ChangelogRuntimeDeps {
|
||||
createFetch?: () => FetchLike;
|
||||
}
|
||||
|
||||
/**
|
||||
* curl enforces its own `--max-time`, but the global-fetch transport has no
|
||||
* deadline: without this a stalled connection leaves the modal on "Loading
|
||||
* changelog..." with no way back except closing it.
|
||||
*/
|
||||
export const CHANGELOG_REQUEST_TIMEOUT_MS = 30_000;
|
||||
|
||||
export function withRequestTimeout(fetchImpl: FetchLike, timeoutMs: number): FetchLike {
|
||||
return (url, init) => {
|
||||
if (typeof AbortSignal?.timeout !== 'function') return fetchImpl(url, init);
|
||||
return fetchImpl(url, { ...init, signal: AbortSignal.timeout(timeoutMs) });
|
||||
};
|
||||
}
|
||||
|
||||
export function createChangelogRuntime(deps: ChangelogRuntimeDeps): {
|
||||
getChangelogSnapshot: (options?: { refresh?: boolean }) => Promise<ChangelogSnapshot>;
|
||||
} {
|
||||
// curl matches the updater's transport choice: Electron's global fetch is
|
||||
// unreliable for GitHub on some Linux builds.
|
||||
const fetchImpl =
|
||||
const fetchImpl = withRequestTimeout(
|
||||
deps.createFetch?.() ??
|
||||
(process.platform === 'win32' ? createGlobalFetch() : createCurlFetch());
|
||||
(process.platform === 'win32' ? createGlobalFetch() : createCurlFetch()),
|
||||
CHANGELOG_REQUEST_TIMEOUT_MS,
|
||||
);
|
||||
|
||||
const source = createChangelogSource({
|
||||
fetchLatestReleaseTag: async () => {
|
||||
|
||||
@@ -29,6 +29,7 @@ type Harness = {
|
||||
focused: () => string[];
|
||||
documentListeners: () => string[];
|
||||
windowListeners: () => string[];
|
||||
handlerFor: (scope: 'document' | 'window', type: string) => Listener | undefined;
|
||||
setActiveElement: (value: unknown) => void;
|
||||
advanceClock: (ms: number) => void;
|
||||
runTimers: () => void;
|
||||
@@ -41,6 +42,8 @@ function createHarness(
|
||||
isModalLayer?: boolean;
|
||||
contains?: boolean;
|
||||
preferredVisible?: boolean;
|
||||
preferredAcceptsFocus?: boolean;
|
||||
extraPreferred?: boolean;
|
||||
fallback?: 'element' | null;
|
||||
} = {},
|
||||
): Harness {
|
||||
@@ -62,8 +65,22 @@ function createHarness(
|
||||
const realDateNow = Date.now;
|
||||
Date.now = () => now;
|
||||
const focused: string[] = [];
|
||||
const documentListeners: string[] = [];
|
||||
const windowListeners: string[] = [];
|
||||
const documentListeners: Array<{ type: string; listener: Listener }> = [];
|
||||
const windowListeners: Array<{ type: string; listener: Listener }> = [];
|
||||
|
||||
const register = (registry: Array<{ type: string; listener: Listener }>) => ({
|
||||
add: (type: string, listener: Listener) => {
|
||||
registry.push({ type, listener });
|
||||
},
|
||||
remove: (type: string, listener: Listener) => {
|
||||
const index = registry.findIndex(
|
||||
(entry) => entry.type === type && entry.listener === listener,
|
||||
);
|
||||
if (index >= 0) registry.splice(index, 1);
|
||||
},
|
||||
});
|
||||
const windowRegistry = register(windowListeners);
|
||||
const documentRegistry = register(documentListeners);
|
||||
const timers: Array<() => void> = [];
|
||||
let activeElement: unknown = null;
|
||||
|
||||
@@ -75,7 +92,15 @@ function createHarness(
|
||||
getClientRects: () => (options.preferredVisible === false ? [] : [{ width: 10, height: 10 }]),
|
||||
focus: () => {
|
||||
focused.push('preferred');
|
||||
activeElement = preferred;
|
||||
// A rendered-but-unfocusable target (e.g. disabled) never becomes active.
|
||||
if (options.preferredAcceptsFocus !== false) activeElement = preferred;
|
||||
},
|
||||
});
|
||||
const secondPreferred = Object.assign(new TestElement(), {
|
||||
getClientRects: () => [{ width: 10, height: 10 }],
|
||||
focus: () => {
|
||||
focused.push('second');
|
||||
activeElement = secondPreferred;
|
||||
},
|
||||
});
|
||||
const fallback =
|
||||
@@ -100,13 +125,8 @@ function createHarness(
|
||||
focus: () => {
|
||||
focused.push('window');
|
||||
},
|
||||
addEventListener: (type: string) => {
|
||||
windowListeners.push(type);
|
||||
},
|
||||
removeEventListener: (type: string) => {
|
||||
const index = windowListeners.indexOf(type);
|
||||
if (index >= 0) windowListeners.splice(index, 1);
|
||||
},
|
||||
addEventListener: windowRegistry.add,
|
||||
removeEventListener: windowRegistry.remove,
|
||||
setTimeout: (callback: () => void) => {
|
||||
timers.push(callback);
|
||||
return timers.length;
|
||||
@@ -120,20 +140,18 @@ function createHarness(
|
||||
get activeElement() {
|
||||
return activeElement;
|
||||
},
|
||||
addEventListener: (type: string) => {
|
||||
documentListeners.push(type);
|
||||
},
|
||||
removeEventListener: (type: string) => {
|
||||
const index = documentListeners.indexOf(type);
|
||||
if (index >= 0) documentListeners.splice(index, 1);
|
||||
},
|
||||
addEventListener: documentRegistry.add,
|
||||
removeEventListener: documentRegistry.remove,
|
||||
},
|
||||
});
|
||||
|
||||
const guard = createModalFocusGuard({
|
||||
isOpen: options.isOpen ?? (() => true),
|
||||
getModalRoot: () => root as unknown as Element,
|
||||
getPreferredFocusTargets: () => [preferred as unknown as HTMLElement],
|
||||
getPreferredFocusTargets: () =>
|
||||
(options.extraPreferred
|
||||
? [preferred, secondPreferred]
|
||||
: [preferred]) as unknown as HTMLElement[],
|
||||
getFallbackFocusTarget: () => fallback as unknown as Element | null,
|
||||
isModalLayer: options.isModalLayer ?? true,
|
||||
});
|
||||
@@ -143,8 +161,12 @@ function createHarness(
|
||||
root,
|
||||
focusMainWindowCalls: () => focusMainWindowCalls,
|
||||
focused: () => focused,
|
||||
documentListeners: () => documentListeners,
|
||||
windowListeners: () => windowListeners,
|
||||
documentListeners: () => documentListeners.map((entry) => entry.type),
|
||||
windowListeners: () => windowListeners.map((entry) => entry.type),
|
||||
handlerFor: (scope: 'document' | 'window', type: string) =>
|
||||
(scope === 'document' ? documentListeners : windowListeners).find(
|
||||
(entry) => entry.type === type,
|
||||
)?.listener,
|
||||
setActiveElement: (value: unknown) => {
|
||||
activeElement = value;
|
||||
},
|
||||
@@ -190,6 +212,34 @@ test('modal focus guard attaches once and detaches every listener', () => {
|
||||
}
|
||||
});
|
||||
|
||||
test('modal focus guard pulls focus back when focusin lands outside the modal', () => {
|
||||
const harness = createHarness();
|
||||
try {
|
||||
harness.guard.attach();
|
||||
|
||||
const focusin = harness.handlerFor('document', 'focusin');
|
||||
assert.ok(focusin, 'attach registers a focusin handler');
|
||||
|
||||
// focusin is not cancelable, so recovery is the only observable effect.
|
||||
focusin?.({ target: {} });
|
||||
assert.deepEqual(harness.focused(), ['preferred']);
|
||||
} finally {
|
||||
harness.restore();
|
||||
}
|
||||
});
|
||||
|
||||
test('modal focus guard ignores focusin while the modal is closed', () => {
|
||||
const harness = createHarness({ isOpen: () => false });
|
||||
try {
|
||||
harness.guard.attach();
|
||||
harness.handlerFor('document', 'focusin')?.({ target: {} });
|
||||
|
||||
assert.deepEqual(harness.focused(), []);
|
||||
} finally {
|
||||
harness.restore();
|
||||
}
|
||||
});
|
||||
|
||||
test('modal focus guard restores focus to the first rendered target', () => {
|
||||
// The target is position:fixed (null offsetParent) yet visible, so it must
|
||||
// still win over the fallback.
|
||||
@@ -202,6 +252,26 @@ test('modal focus guard restores focus to the first rendered target', () => {
|
||||
}
|
||||
});
|
||||
|
||||
test('modal focus guard tries the next target when one refuses focus', () => {
|
||||
const harness = createHarness({ preferredAcceptsFocus: false, extraPreferred: true });
|
||||
try {
|
||||
assert.equal(harness.guard.focusFallbackTarget(), true);
|
||||
assert.deepEqual(harness.focused(), ['preferred', 'second']);
|
||||
} finally {
|
||||
harness.restore();
|
||||
}
|
||||
});
|
||||
|
||||
test('modal focus guard reaches the fallback when no preferred target takes focus', () => {
|
||||
const harness = createHarness({ preferredAcceptsFocus: false });
|
||||
try {
|
||||
assert.equal(harness.guard.focusFallbackTarget(), true);
|
||||
assert.deepEqual(harness.focused(), ['preferred', 'fallback']);
|
||||
} finally {
|
||||
harness.restore();
|
||||
}
|
||||
});
|
||||
|
||||
test('modal focus guard falls back when no preferred target is rendered', () => {
|
||||
const harness = createHarness({ preferredVisible: false });
|
||||
try {
|
||||
|
||||
@@ -23,6 +23,7 @@ export function createModalFocusGuard(deps: ModalFocusGuardDeps) {
|
||||
let focusinGuard: ((event: FocusEvent) => void) | null = null;
|
||||
let windowFocusGuard: (() => void) | null = null;
|
||||
let pointerFocusGuard: ((event: Event) => void) | null = null;
|
||||
let pointerFocusRoot: Element | null = null;
|
||||
let isRecovering = false;
|
||||
let lastRecoveryAt = 0;
|
||||
|
||||
@@ -43,12 +44,12 @@ export function createModalFocusGuard(deps: ModalFocusGuardDeps) {
|
||||
|
||||
// getClientRects() rather than offsetParent: the latter is null for
|
||||
// position:fixed elements, which would skip a perfectly visible target.
|
||||
const preferred = deps
|
||||
.getPreferredFocusTargets()
|
||||
.find((target) => target.getClientRects().length > 0);
|
||||
if (preferred) {
|
||||
preferred.focus({ preventScroll: true });
|
||||
return document.activeElement === preferred;
|
||||
// Rendered is not the same as focusable, so keep trying until one sticks
|
||||
// instead of giving up on the first candidate that refuses focus.
|
||||
for (const target of deps.getPreferredFocusTargets()) {
|
||||
if (target.getClientRects().length === 0) continue;
|
||||
target.focus({ preventScroll: true });
|
||||
if (document.activeElement === target) return true;
|
||||
}
|
||||
|
||||
const fallback = deps.getFallbackFocusTarget();
|
||||
@@ -80,10 +81,11 @@ export function createModalFocusGuard(deps: ModalFocusGuardDeps) {
|
||||
/** Idempotent; safe to call on every open. */
|
||||
function attach(): void {
|
||||
if (focusinGuard === null) {
|
||||
// focusin is not cancelable, so there is nothing to preventDefault here;
|
||||
// focus is taken back afterwards instead.
|
||||
focusinGuard = (event: FocusEvent) => {
|
||||
if (!deps.isOpen()) return;
|
||||
if (!isModalFocusTarget(event.target)) {
|
||||
event.preventDefault();
|
||||
enforceModalFocus();
|
||||
}
|
||||
};
|
||||
@@ -95,9 +97,11 @@ export function createModalFocusGuard(deps: ModalFocusGuardDeps) {
|
||||
requestOverlayFocus();
|
||||
enforceModalFocus();
|
||||
};
|
||||
const root = deps.getModalRoot();
|
||||
root.addEventListener('pointerdown', pointerFocusGuard);
|
||||
root.addEventListener('click', pointerFocusGuard);
|
||||
// Remember the root we bound to: resolving it again on detach could
|
||||
// return a different element and leak the listeners on the old one.
|
||||
pointerFocusRoot = deps.getModalRoot();
|
||||
pointerFocusRoot.addEventListener('pointerdown', pointerFocusGuard);
|
||||
pointerFocusRoot.addEventListener('click', pointerFocusGuard);
|
||||
}
|
||||
|
||||
if (windowFocusGuard === null) {
|
||||
@@ -117,10 +121,10 @@ export function createModalFocusGuard(deps: ModalFocusGuardDeps) {
|
||||
}
|
||||
|
||||
if (pointerFocusGuard) {
|
||||
const root = deps.getModalRoot();
|
||||
root.removeEventListener('pointerdown', pointerFocusGuard);
|
||||
root.removeEventListener('click', pointerFocusGuard);
|
||||
pointerFocusRoot?.removeEventListener('pointerdown', pointerFocusGuard);
|
||||
pointerFocusRoot?.removeEventListener('click', pointerFocusGuard);
|
||||
pointerFocusGuard = null;
|
||||
pointerFocusRoot = null;
|
||||
}
|
||||
|
||||
if (windowFocusGuard) {
|
||||
|
||||
Reference in New Issue
Block a user