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.tsL143-154 —switchLibrarycatches and records intolibraryError, resolving normally.32ec36c6:src/components/Settings/RemoteLibraryWizard.tsxL211, 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:
- Let
runLibraryWorkrecord the error and rethrow. Callers that only need the error surfaced in the settings panel already ignore rejections today viavoid, so they would need.catch(() => {})or equivalent — noisy, but explicit, and the wizard's existingtry/catchat L224-228 starts working as written. - Give the library surface a result type (
{ ok: true } | { ok: false; error: string }) and makerunLibraryWorkreturn 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.