Patch renderer: mitigate Usersnap init failure

[imnasnainaec]

Summary

Hardens renderer startup against a non-critical Usersnap initialization failure. Usersnap is an optional feedback widget loaded from an external service; in offline or firewalled environments its load can hang or fail, and that shouldn't surface as an error or stall startup.

Changes in src/renderer/services/usersnap.service.ts (+ a new test file):

• Bound loadSpace + init with a 5 s timeout. Both are awaited on the startup path and both talk to Usersnap infrastructure, so a hang in either (blocked egress, dead connection) would otherwise stall the renderer. Load and init now run inside an AsyncVariable (the project's promise-with-timeout utility) that rejects after 5 s, letting the existing catch handle the failure. Since a promise can't be cancelled, a load/init that finishes after the timeout has its space destroyed so the SDK isn't left orphaned.
• Downgrade the failure log from error to warn. Usersnap being unavailable is expected and recoverable, not an app error, so logging it at error just adds noise. globalUsersnapApi is still cleared to undefined as before.
• Tests. Added usersnap.service.test.ts covering the timeout/race branches against the real AsyncVariable with fake timers: resolve before timeout, resolve after timeout → orphan destroyed, reject before timeout, reject after timeout → late failure swallowed, and destroy-failure after timeout → swallowed.

Behavior

Startup no longer risks hanging on Usersnap, and a blocked load logs a single warning instead of an error. No functional change — feedback forms degrade gracefully as they did before.

🤖 Generated with Claude Code

Devin review: https://app.devin.ai/review/paranext/paranext-core/pull/2460

---

This change is

[imnasnainaec]

Review summary (condensed)

Ran /review-paratext (4 parallel passes: api/correctness, style, compliance, ux). Scope: renderer-internal only — no public API, localization, UI, or build/config changes.

Findings: 0 critical, 0 important open, 0 minor open. All resolved in-review:

• Test coverage (important) → added usersnap.service.test.ts covering the timeout/race branches against the real AsyncVariable with fake timers.
• Debug-log wording (minor) → reworded the post-timeout message.
• IIFE intent (minor) → added a fire-and-forget comment.

Quality gate: typecheck ✓, lint ✓, 3/3 tests ✓.

Reviewer focus:

• The 5 s timeout sits on the awaited startup path. Kept at 5 s deliberately — clean offline failures (DNS/refused/blocked) reject fast and pay ~0 s; only a silent hang pays the full 5 s. 3 s was considered and declined to avoid cutting off slow-but-successful loads.
• Confirm late-arrival destroy() on a never-init()ed SpaceApi is a safe no-op in the SDK (the orphan is inert since init only runs on the in-time path).
• Sanity-check the test's fake-timer / vi.waitFor interplay isn't flaky in CI.

[ArmorBearer]

Due to limited resources and aggressive work schedule, we're not able to review this right now, but we will when we're able. If this delay creates difficulties for you in your development schedule, please let me know. Thank you for your contribution to improve Paratext.

[katherinejensen00]

Nice fix! Thanks for making it! Just one blocking thing to look at, the rest are nitpicky suggestions that you can decide what to do with. Thanks again!

@katherinejensen00 reviewed 2 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on imnasnainaec).

---

src/renderer/services/usersnap.service.ts line 21 at r3 (raw file):

/** Milliseconds to wait for `loadSpace` to resolve before giving up. */
export const LOAD_SPACE_TIMEOUT_MS = 5 * 1000;

NIT Super nitpicky and up to you if it is actually worth fixing.

Trivial: LOAD_SPACE_TIMEOUT_MS is exported only so the test can advance fake timers by exactly this amount — which is a good reason to avoid a drifting magic number. Consider a half-sentence "Exported for tests." on the TSDoc so the widened surface reads as intentional. Optional.

---

src/renderer/services/usersnap.service.ts line 179 at r3 (raw file):

        const spaceApi = await loadSpace(USERSNAP_SPACE_API_KEY);
        // If a space loads after the timeout fires, destroy it to avoid an orphaned instance.
        if (loadVar.hasSettled) await spaceApi.destroy();

NIT
Minor/optional: AsyncVariable also exposes hasTimedOut, which states the intent here directly ("the timeout already fired, so clean up the late arrival"). It's functionally equivalent in this code since the timeout is the only thing that can settle loadVar before these checks — so this is purely a readability nit and matches the closest precedents (shutdown-tasks.ts, network.service.ts). Entirely fine to leave as hasSettled if you prefer.

---

src/renderer/services/usersnap.service.ts line 184 at r3 (raw file):

        if (loadVar.hasSettled)
          logger.debug('Usersnap loadSpace rejected (or cleanup failed) after timeout:', error);
        else loadVar.rejectWithReason(getErrorMessage(error));

NIT
Minor: rejecting with getErrorMessage(error) means loadVar.promise rejects with a plain string, so the outer catch (error) at the bottom logs a string rather than the original Error — the stack/name is gone for a genuine loadSpace throw. rejectWithReason requires a string, so this is a reasonable adaptation, but if init-time diagnostics matter you could logger.warn(..., error) (with the real error) inside the IIFE before rejecting. Low priority.

---

src/renderer/services/usersnap.service.ts line 188 at r3 (raw file):

    })();
    const api = await loadVar.promise;
    await api.init(defaultInitParams);

The timeout guards loadSpace, but api.init() on the next line is still unbounded — so the startup hang this PR targets is only half-closed.

initializeUsersnapApi() is awaited sequentially in the renderer startup chain (src/renderer/index.tsx:96) and blocks the runPromisesAndThrowIfRejected(...) that starts the web-view, dialog, and theme services. This change correctly bounds loadSpace, but await api.init(defaultInitParams) runs right after await loadVar.promise and also talks to Usersnap infrastructure. If init hangs, initializeUsersnapApi() hangs and the same downstream services never start — which is the exact failure the summary says is fixed ("Startup no longer risks hanging on Usersnap"). Could you either bring api.init() inside the same timeout budget (e.g. race the init too, or gate it), or narrow the PR/comment wording to say only loadSpace is bounded? The init hang is less likely than a loadSpace hang (the SDK is already loaded by then), but it's on the same awaited path, so it's worth closing or explicitly scoping out.

---

src/renderer/services/usersnap.service.ts line 256 at r3 (raw file):

  if (!globalUsersnapApi) {
    logger.warn(
      'Cannot open Usersnap form: UserSnap API is not initialized. This may be due to network connectivity issues or blocked external requests.',

NIT
Consistency nit: trimming the init-failure log to "feedback forms will be unavailable" is a good noise reduction (the error argument still carries the real cause). Just note openUsersnapForm a few lines down still carries the verbose "This may be due to network connectivity issues or blocked external requests" tail — worth trimming that one too (or keeping both) so the two messages follow one convention.

---

src/renderer/services/usersnap.service.test.ts line 88 at r3 (raw file):

  });

  it('resolves after the timeout: init not called, late space destroyed', async () => {

NIT
Two branches of the new race logic aren't exercised — both in the orphan-cleanup safety net this PR adds. The suite covers resolve-before, resolve-after, and reject-before. Not covered: (a) loadSpace rejecting after the timeout, whose only behavior is the logger.debug('Usersnap loadSpace rejected (or cleanup failed) after timeout:', error) branch, and (b) spaceApi.destroy() itself rejecting in the resolve-after path (it lands in the same catch). These are exactly the "swallow and don't crash startup" contracts this change introduces, and they'd regress silently. The Deferred + fake-timer harness already makes both cheap to add (advance timers, then reject(...) / make destroy reject; assert startup still resolves and doesn't throw).

[imnasnainaec]

@imnasnainaec made 1 comment and resolved 5 discussions.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on katherinejensen00).

---

src/renderer/services/usersnap.service.ts line 188 at r3 (raw file):

Previously, katherinejensen00 wrote…

The timeout guards loadSpace, but api.init() on the next line is still unbounded — so the startup hang this PR targets is only half-closed.

initializeUsersnapApi() is awaited sequentially in the renderer startup chain (src/renderer/index.tsx:96) and blocks the runPromisesAndThrowIfRejected(...) that starts the web-view, dialog, and theme services. This change correctly bounds loadSpace, but await api.init(defaultInitParams) runs right after await loadVar.promise and also talks to Usersnap infrastructure. If init hangs, initializeUsersnapApi() hangs and the same downstream services never start — which is the exact failure the summary says is fixed ("Startup no longer risks hanging on Usersnap"). Could you either bring api.init() inside the same timeout budget (e.g. race the init too, or gate it), or narrow the PR/comment wording to say only loadSpace is bounded? The init hang is less likely than a loadSpace hang (the SDK is already loaded by then), but it's on the same awaited path, so it's worth closing or explicitly scoping out.

Done.

[katherinejensen00]

Thanks for making those adjustments! This PR is ready to ship ⛵

@katherinejensen00 reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved.