fix: the settings surface fails partially and visibly, never totally and silently - #1592
Conversation
A Svelte component that threw during mount escaped into whatever was hosting it. A Modal that mounts from its constructor or onOpen() never opened at all; the declarative settings tab, which builds every group by calling render closures in turn, abandoned every remaining QuickAdd setting. #1583 caught this at the settings tab's own two call sites; the other five hosts - both choice builders, the macro command editor and the two package modals - had nothing. Live repro: a Macro choice whose macro.commands is null in data.json. Clicking Configure threw "Cannot read properties of null (reading 'filter')" out of CommandList's mount, and the macro builder simply did not appear - no modal, no Notice, nothing. mountComponent now never throws. It reports the error once (one Notice, through the plugin's own reportError), clears the debris the half-finished mount left in the host, and puts a card where the component would have been. The macro builder opens, says exactly what could not be displayed and why, and the rest of it - name, command bar, script pickers, autosave footer - still works. Each host names what it is showing, so the message is specific. The settings choice list keeps its own card (ChoicesUnavailable), which carries the data.json recovery instructions a generic one has no business claiming; that card and the default now share one prop contract. The handle reports whether the component actually rendered. The choice builders use it to drop a form the user never saw, so it cannot resolve its untouched clone of the choice back over their data at close. Not covered, and now documented at the seam: a throw from an $effect. Svelte flushes effects on a later tick, so it never reaches this try/catch. That case belongs to <svelte:boundary>, which ChoiceView already has. Closes #1584
The row actions in the choices list were async handlers with no catch. Svelte re-throws an event handler's error to the window, and a rejected promise is an unhandled rejection - Obsidian surfaces neither. So a failing Delete, Configure, Duplicate, Rename or Move produced nothing at all: no Notice, no visual change, no message anyone would think to look for. The button just did not work, which reads as "QuickAdd is broken" and gives the user nothing to report. Live repro: a choice whose type is not a known choice type (a hand-edit, or an import from a newer QuickAdd). Configure routes to getChoiceBuilder, which throws "Invalid choice type" - and nothing happened. reportingHandler wraps a handler so both halves are covered, the synchronous throw and the rejected promise, and reports through the same reportError the rest of the plugin uses. Cancellations stay silent: a dismissed prompt is an answer, not a failure (#1567). Applied to the whole ChoiceListActions bag rather than to individual buttons. Every row button, context-menu item and nested list reaches the handlers through that one object (MultiChoiceListItem spreads it), so wrapping it there is what makes the coverage structural instead of a list someone has to remember to extend. The message names the action and takes its noun from the row itself, so a folder is never called a choice (#1552). <svelte:boundary> does not help here: it catches render and effect errors, not event handlers. Closes #1585
Adversarial review of the two previous commits found the state they created but did not finish. Once mountComponent stopped letting a broken CommandList take the macro builder down with it, the builder started opening OVER a list it could not draw - with every add control still live. Two ways that goes wrong: - macro.commands is null (the #1584 repro): every add control throws [...null] inside its click handler. Dead buttons, silently - the exact shape #1585 exists to eliminate. - a valid array with duplicate ids: the keyed {#each} throws each_key_duplicate, so the list is invisible but the add controls WORK. The user clicks Wait three times because nothing appears to happen, closes, and now has three commands in their macro that they cannot see, reorder or delete. Neither was reachable before, because the modal never opened at all. So the editor now stops offering the affordances the failed view was the only way to review: the card explains what happened, and the macro's name, "run on startup" and icon stay editable around it. Same rule the choice builders already follow with handle.ok. Also from the review: - The card's copy claimed "nothing in your vault has been changed" and "everything else on this screen still works". Both can be false - adding a choice saves it and THEN opens the builder, and a host's remaining controls may only have made sense beside the view that failed. It now says what happened, that the failure itself changed nothing, and what to include in a report. - Row-action failures name the row: Couldn't delete the choice "Daily note", not "that choice". The noun still comes from the row, so a folder is a folder. Declined: folding MountFailed and ChoicesUnavailable into one card. Their style blocks are duplicated, but they are deliberately different messages - one carries data.json recovery instructions that would be misleading anywhere else - and restructuring an already-shipped component to share 40 lines of CSS is a bigger change than the duplication costs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMount failures now render fallback cards and return status handles across Svelte hosts. Choice builders avoid persisting failed form state, command editors hide controls when lists fail, settings views use centralized fallback handling, and choice actions report failures. ChangesMount failure infrastructure
Host mount integration
UI action reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Host
participant mountComponent
participant SvelteComponent
participant MountFailed
Host->>mountComponent: mount component with options
mountComponent->>SvelteComponent: mount component
SvelteComponent-->>mountComponent: success or throw
alt mount succeeds
mountComponent-->>Host: handle ok=true
else mount fails
mountComponent->>MountFailed: render fallback with detail
mountComponent-->>Host: handle ok=false
end
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying quickadd with
|
| Latest commit: |
8a60c68
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fe6163a5.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-1584-settings-hard.quickadd.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/utils/errorUtils.ts (1)
186-206: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse tab indentation in this TypeScript implementation.
Lines 187-205 use spaces rather than the repository’s configured tabs. As per coding guidelines, TypeScript files must use tab indentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/errorUtils.ts` around lines 186 - 206, Replace the space indentation in reportingHandler and its nested callback logic with tabs, preserving the existing control flow and alignment.Source: Coding guidelines
src/gui/svelte/mountComponent.test.ts (1)
43-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the "fallback itself throws" branch.
The suite thoroughly covers the primary mount-failure path, but the plain-text last-resort branch in
renderMountFailure(mountComponent.ts'scatch { container.textContent = ... }) has no dedicated test. A smallfallbackComponentfixture that throws on mount would pin that this safety net actually engages instead of propagating.🧪 Suggested test addition
it("falls back to plain text if the fallback component itself throws", () => { const target = document.createElement("div"); const ThrowingFallback = MountThrows as unknown as MountFallbackComponent; mountComponent(target, MountThrows, { commands: null }, { what: "this macro's commands", fallbackComponent: ThrowingFallback, }); expect(target.textContent).toContain("QuickAdd couldn't display this macro's commands."); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gui/svelte/mountComponent.test.ts` around lines 43 - 133, Add a test in the mountComponent failed-mount suite covering a fallbackComponent that throws during rendering, using the existing MountThrows fixture cast to the expected fallback type. Assert mountComponent does not propagate the error and target.textContent contains the plain-text failure message for the configured what value, exercising renderMountFailure’s last-resort catch branch.src/gui/ChoiceBuilder/captureChoiceBuilder.ts (1)
67-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate mount-failure handling in the two choice-form builders. Both
display()implementations mount their form and then clearthis.formPropson!handle.okwith identical logic and comments; the shared contract belongs in the commonChoiceBuilderbase class rather than copy-pasted per subclass.
src/gui/ChoiceBuilder/captureChoiceBuilder.ts#L67-L86: extract the "mount, then dropformPropson failure" logic into a shared protected helper onChoiceBuilder(e.g.protected mountForm<C, P>(component, props, what): P | undefined) and call it here.src/gui/ChoiceBuilder/templateChoiceBuilder.ts#L61-L80: use the same shared helper instead of re-implementing the identicalif (!handle.ok) this.formProps = undefined;logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gui/ChoiceBuilder/captureChoiceBuilder.ts` around lines 67 - 86, The mount-failure cleanup is duplicated across both choice-form builders; centralize it in the shared ChoiceBuilder base class. Add a protected mountForm helper that mounts the component, clears formProps when the mount handle is unsuccessful, and returns the resulting props or undefined, then update display() in src/gui/ChoiceBuilder/captureChoiceBuilder.ts (lines 67-86) and src/gui/ChoiceBuilder/templateChoiceBuilder.ts (lines 61-80) to use it and remove their duplicate handling and comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/errorUtils.ts`:
- Around line 200-201: Update the thenable handling in errorUtils.ts around the
Promise rejection reporting to use Promise.resolve(result).catch(report),
ensuring generic thenables are assimilated before reporting failures. In
errorUtils.test.ts lines 97-107, strengthen the regression assertion to verify
the logged error contains “Couldn't move that choice: boom”.
---
Nitpick comments:
In `@src/gui/ChoiceBuilder/captureChoiceBuilder.ts`:
- Around line 67-86: The mount-failure cleanup is duplicated across both
choice-form builders; centralize it in the shared ChoiceBuilder base class. Add
a protected mountForm helper that mounts the component, clears formProps when
the mount handle is unsuccessful, and returns the resulting props or undefined,
then update display() in src/gui/ChoiceBuilder/captureChoiceBuilder.ts (lines
67-86) and src/gui/ChoiceBuilder/templateChoiceBuilder.ts (lines 61-80) to use
it and remove their duplicate handling and comments.
In `@src/gui/svelte/mountComponent.test.ts`:
- Around line 43-133: Add a test in the mountComponent failed-mount suite
covering a fallbackComponent that throws during rendering, using the existing
MountThrows fixture cast to the expected fallback type. Assert mountComponent
does not propagate the error and target.textContent contains the plain-text
failure message for the configured what value, exercising renderMountFailure’s
last-resort catch branch.
In `@src/utils/errorUtils.ts`:
- Around line 186-206: Replace the space indentation in reportingHandler and its
nested callback logic with tabs, preserving the existing control flow and
alignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf141e66-fc70-4c62-8f65-76cea3412f6b
📒 Files selected for processing (18)
src/gui/ChoiceBuilder/captureChoiceBuilder.tssrc/gui/ChoiceBuilder/components/mountFormatPreview.svelte.tssrc/gui/ChoiceBuilder/templateChoiceBuilder.tssrc/gui/MacroGUIs/CommandSequenceEditor.tssrc/gui/MacroGUIs/CommandSequenceEditor.unrenderable.test.tssrc/gui/PackageManager/ExportPackageModal.tssrc/gui/PackageManager/ImportPackageModal.tssrc/gui/choiceList/ChoiceView.rowActions.test.tssrc/gui/choiceList/ChoiceView.sveltesrc/gui/choiceList/ChoicesUnavailable.sveltesrc/gui/svelte/MountFailed.sveltesrc/gui/svelte/mountComponent.test.tssrc/gui/svelte/mountComponent.tssrc/gui/svelte/mountThrows.fixture.sveltesrc/gui/svelte/mountThrowsDeep.fixture.sveltesrc/quickAddSettingsTab.tssrc/utils/errorUtils.test.tssrc/utils/errorUtils.ts
reportingHandler duck-typed on `.then` and then called `.catch`. A thenable is only required to have `.then`, so for a non-promise thenable the wrapper threw inside its own success path and reported "result.catch is not a function" in place of the actual failure - the same lost error the helper exists to prevent. Promise.resolve() assimilates any thenable, so the rejection that reaches the Notice is the real one. The regression test asserted only that SOMETHING was logged, which is why it passed over the bug; it now asserts the cause, and fails against the old code. Caught by CodeRabbit on #1592.
Closes #1584. Closes #1585.
Two ways the settings surface could fail totally and silently. Both were reproduced live in this worktree's isolated Obsidian 1.13.0 vault before the fix, and re-verified after.
#1584 — a broken view took its whole screen with it
mountComponentis the single place every Svelte view enters an Obsidian host. If the component threw duringmount(), the throw escaped into the host: a Modal that mounts from its constructor oronOpen()never opened, and the declarative settings tab — which builds every group by callingrenderclosures in turn — abandoned every remaining QuickAdd setting. #1583 caught this at the settings tab's own two call sites; the other five hosts had nothing.Repro: a Macro choice whose
macro.commandsisnullindata.json. Clicking Configure threwTypeError: Cannot read properties of null (reading 'filter')out of CommandList's mount, and the macro builder simply did not appear.#1585 — a failing row action was a dead button
ChoiceView's row actions areasynchandlers with nocatch. Svelte re-throws an event handler's error to the window, and a rejected promise is an unhandled rejection — Obsidian surfaces neither. A failing Delete, Configure, Duplicate, Rename or Move produced nothing at all.Repro: a choice whose
typeis not a known choice type (a hand-edit, or an import from a newer QuickAdd). Configure routes togetChoiceBuilder, which throwsInvalid choice type.document.querySelectorAll(".notice")→[].QuickAdd: (ERROR) Couldn't open the settings for the choice "Choice with a bad type": Invalid choice typeDesign
mountComponentnever throws. It reports the error once (one Notice, through the plugin's ownreportError), removes the debris the half-finished mount left in the host — tracked by diffingtarget.childNodes, never by emptyingtarget, which belongs to the host — and mounts a card where the component would have been. It always returns a handle, so no call site grew a null check.Each of the seven hosts names what it is showing, so the message is specific ("this macro's commands", "your choices", "the package importer"). The settings choice list keeps its own card,
ChoicesUnavailable, which carries thedata.jsonrecovery instructions a generic card has no business claiming; that card and the default now share one prop contract.quickAddSettingsTab.mountSettingView's ad-hoc try/catch is deleted in favour of the seam.The handle reports whether the component actually rendered (
ok), and two kinds of host use it:$stateclone of the choice back over their data at close;A failing row action says so.
reportingHandlercovers both halves — a synchronous throw and a rejected promise — and reports through the samereportErroras the rest of the plugin. Cancellations stay silent: a dismissed prompt is an answer, not a failure (#1567). It is applied to the wholeChoiceListActionsbag rather than to individual buttons, because every row button, context-menu item and nested list reaches the handlers through that one object (MultiChoiceListItemspreads it) — wrapping it there makes the coverage structural instead of a list someone has to remember to extend. The message names the row and takes its noun from it, so a folder is never called a choice (#1552).Tradeoffs and things deliberately not done
<svelte:boundary>does not solve [BUG] A failing row action in the settings list is a silent dead button #1585. It catches render and effect errors; Svelte re-throws event-handler errors to the window. Confirmed by the repro —ChoiceViewalready has a boundary and the Configure throw sailed past it.unhandledrejectionlistener would catch other plugins' rejections too and produce context-free Notices. Rejected.mountComponentreturningnullon failure (the issue's own suggestion) forces every call site to grow a null check for a case that should be invisible to them. A handle that owns its fallback is less code and cannot be forgotten.$effectis still not covered, and this is now documented at the seam. Svelte flushes effects on a later tick, so it never reaches the try/catch; that case belongs to<svelte:boundary>. Verified against svelte 5 with a scratch probe.MountFailedandChoicesUnavailableduplicate ~40 lines of CSS. Left alone: they are deliberately different messages, and restructuring an already-shipped component to share a style block is a bigger change than the duplication costs.macro.commandsstill has no read-accessor guard of the kind fix: a malformed folder in data.json no longer takes the plugin down with it #1583 gaveMulti.choices. Out of scope here; filed separately.What adversarial review caught
An ultracode review (4 lenses → 28 independent refutation passes) killed most findings but landed one that mattered: once the builder survived a broken mount, it opened over a list it could not draw with every add control still live. With
commands: nullthose controls throw silently; with a valid array holding duplicate ids the list is invisible but the controls work, appending commands the user cannot see, review or delete, and persisting them on close. Neither was reachable before, because the modal never opened. The third commit fixes that and tightens two copy claims the review showed could be false.Validation
pnpm run build-with-lintclean;pnpm run check0 errors; 4325 unit tests pass.mountComponentfailure path (reports once, renders the card, host content preserved, debris cleared,destroy()idempotent, custom card honoured);reportingHandler(sync throw, rejection, foreign thenable, cancellation silence, non-promise returns);ChoiceViewrow-action reporting incl. the folder noun;CommandSequenceEditorover an unrenderable list.Release impact
Three
fix:commits — patch release. No migration, no settings change.Summary by CodeRabbit