Home

dev / openkara

publicthedavidweng/OpenKara· sync paused
Code Branches Pull requestsIssuesInsights
main
Home Code PRsIssues

#385 Remote-library wizard reports success and closes when activation failed

closed

Opened by dev · yesterday

devopened this issueAuthor· yesterday

Line references are pinned to main at afbd7c45.

Pre-existing on main. Not introduced by #373 or #374 — #374 only renamed the dependency (actions.switchLibrary → library.activate) and moved the error handling into SettingsController, keeping its shape. Do not bisect to it. Pre-split proof below.

Where

The root cause is that the activate path resolves on failure instead of rejecting.

src/lib/settings-controller/settings-controller.ts L285-293 and L678-679:

// L285-293
const runLibraryWork = async (work: () => Promise<void>) => {
  patchLibrary({ error: null });

  try {
    await work();
  } catch (error: unknown) {
    patchLibrary({ error: getErrorMessage(error) });   // recorded, then resolves
  }
};

// L678-679
activate: (libraryId) =>
  runLibraryWork(() => librarySession.switchLibrary(libraryId)),

Two consumers in src/components/Settings/RemoteLibraryWizard.tsx treat that resolution as success:

// L213-224 — the create/connect flow
await library.activate(remoteLibraryId);
setMessage(t("settings.library.remoteLibraryConnected"));
...
onClose();

// L509-511 — the existing-library list
onClick={() =>
  void library.activate(candidate.id).then(() => onClose())
}

What goes wrong

activate() is typed Promise<void> and never rejects: runLibraryWork catches, writes the message into view.library.error, and resolves normally. Both wizard paths read that resolution as success.

On the create/connect path the wizard therefore shows the "remote library connected" success message for a failed activation and then closes, so the recorded view.library.error is never seen — the wizard unmounts before anything renders it. The user is told the remote library is connected when it is not. The surrounding try/catch at L224-228 does not help, because nothing throws.

On the list path the wizard closes on the same false premise, minus the misleading message.

Note this is broader than the review comment that surfaced it, which cited only the list button at L510. The create/connect path is the more damaging of the two because it actively asserts success.

Conditions

Any failure inside librarySession.switchLibrary — the remote endpoint unreachable, credentials rejected after the wizard's own auth step, the registry write failing, or a side-effect step of the switch checklist throwing. Nothing about it requires an unusual configuration; a network drop between the auth step and the activate call is enough.

Pre-existing proof

  • 32ec36c6:src/components/Settings/settings-overlay.library-actions.ts L143-154 — switchLibrary catches and records into libraryError, resolving normally.
  • 32ec36c6:src/components/Settings/RemoteLibraryWizard.tsx L211, L218, L222 and L508 — the identical call sites.

Fix direction (my view, not the reviewer's)

The suggestion was to have activate() return a success flag and gate onClose() on it. I would not add a boolean.

The problem is not the wizard's control flow, it is that runLibraryWork erases the difference between success and failure for every caller of the library surface — activate, refresh, rename, open and the rest all go through it and all resolve unconditionally. A boolean on activate alone fixes one caller and leaves eight with the same trap, and a boolean return type invites callers to ignore it silently.

Two shapes are worth considering, and I would want a maintainer's preference before either is implemented:

  1. Let runLibraryWork record the error and rethrow. Callers that only need the error surfaced in the settings panel already ignore rejections today via void, so they would need .catch(() => {}) or equivalent — noisy, but explicit, and the wizard's existing try/catch at L224-228 starts working as written.
  2. Give the library surface a result type ({ ok: true } | { ok: false; error: string }) and make runLibraryWork return it, so the compiler forces every call site to decide. More churn, but it cannot be ignored by accident.

Either way the fix belongs in SettingsController, not in the wizard. Whichever is chosen, the wizard should stop emitting remoteLibraryConnected before it knows the activation succeeded.

Not fixing here

Filed for tracking only.

Sign in to comment.

Linked pull requests

No linked pull requests yet.