Skip to content

fix: the settings surface fails partially and visibly, never totally and silently - #1592

Merged
chhoumann merged 4 commits into
masterfrom
chhoumann/1584-settings-hardening
Jul 27, 2026
Merged

fix: the settings surface fails partially and visibly, never totally and silently#1592
chhoumann merged 4 commits into
masterfrom
chhoumann/1584-settings-hardening

Conversation

@chhoumann

@chhoumann chhoumann commented Jul 27, 2026

Copy link
Copy Markdown
Owner

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

mountComponent is the single place every Svelte view enters an Obsidian host. If the component threw during mount(), the throw escaped into the host: a Modal that mounts from its constructor or onOpen() never opened, and 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 had nothing.

Repro: a Macro choice whose macro.commands is null in data.json. Clicking Configure threw TypeError: Cannot read properties of null (reading 'filter') out of CommandList's mount, and the macro builder simply did not appear.

Before After
Clicked Configure on "Broken macro". No modal, no Notice, no change. The builder opens, names what it couldn't display and why, and the macro's name, "run on startup" and icon stay editable.

#1585 — a failing row action was a dead button

ChoiceView's row actions are 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. A failing Delete, Configure, Duplicate, Rename or Move produced nothing at all.

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.

Before After
Clicked Configure on "Choice with a bad type". document.querySelectorAll(".notice")[]. QuickAdd: (ERROR) Couldn't open the settings for the choice "Choice with a bad type": Invalid choice type

Design

mountComponent never throws. It reports the error once (one Notice, through the plugin's own reportError), removes the debris the half-finished mount left in the host — tracked by diffing target.childNodes, never by emptying target, 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 the data.json recovery 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:

  • the choice builders drop a form the user never saw, so it cannot resolve its untouched $state clone of the choice back over their data at close;
  • the command editor stops offering the controls that mutate a list it could not draw (see "what review caught" below).

A failing row action says so. reportingHandler covers both halves — a synchronous throw and a rejected promise — and reports through the same reportError as the rest of the plugin. Cancellations stay silent: a dismissed prompt is an answer, not a failure (#1567). It is applied to the whole ChoiceListActions bag rather than to individual buttons, because every row button, context-menu item and nested list reaches the handlers through that one object (MultiChoiceListItem spreads 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 — ChoiceView already has a boundary and the Configure throw sailed past it.
  • A global unhandledrejection listener would catch other plugins' rejections too and produce context-free Notices. Rejected.
  • mountComponent returning null on 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.
  • A throw from an $effect is 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.
  • MountFailed and ChoicesUnavailable duplicate ~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.commands still has no read-accessor guard of the kind fix: a malformed folder in data.json no longer takes the plugin down with it #1583 gave Multi.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: null those 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-lint clean; pnpm run check 0 errors; 4325 unit tests pass.
  • New coverage: mountComponent failure 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); ChoiceView row-action reporting incl. the folder noun; CommandSequenceEditor over an unrenderable list.
  • Live in the isolated vault: both repros fixed; and regression-checked that a healthy Template builder, a healthy Macro builder (full command bar), and the import/export package modals all still mount with no errors captured.

Release impact

Three fix: commits — patch release. No migration, no settings change.

Summary by CodeRabbit

  • New Features
    • Added a standard mount-failure fallback card for “QuickAdd couldn’t display …” scenarios, with contextual titles and optional error details.
  • Bug Fixes
    • Prevented failed or unrendered form/preview/choice states from overwriting saved choices.
    • Command sequence editor now stops rendering remaining UI when commands fail to render, avoiding unusable controls.
    • Choice row actions and assistant settings failures are now reported without breaking the rest of the interface.
  • Tests
    • Expanded coverage for mount failure recovery, error reporting behavior, and UI cleanup/idempotency.

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.
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 15ab6f9f-2ebf-4daa-949d-2bb6d069d55a

📥 Commits

Reviewing files that changed from the base of the PR and between ec89aa2 and 8a60c68.

📒 Files selected for processing (2)
  • src/utils/errorUtils.test.ts
  • src/utils/errorUtils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/utils/errorUtils.test.ts
  • src/utils/errorUtils.ts

📝 Walkthrough

Walkthrough

Mount 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.

Changes

Mount failure infrastructure

Layer / File(s) Summary
Safe mount contract and fallback rendering
src/gui/svelte/mountComponent.ts, src/gui/svelte/MountFailed.svelte, src/gui/svelte/mountComponent.test.ts, src/gui/svelte/mountThrows*.svelte
mountComponent catches mount failures, removes partial output, reports errors, renders configurable fallback content, and returns an ok status; success, fallback, cleanup, and host-content preservation are tested.

Host mount integration

Layer / File(s) Summary
Host failure propagation
src/quickAddSettingsTab.ts, src/gui/MacroGUIs/CommandSequenceEditor.ts, src/gui/MacroGUIs/CommandSequenceEditor.unrenderable.test.ts, src/gui/ChoiceBuilder/*, src/gui/PackageManager/*, src/gui/choiceList/ChoicesUnavailable.svelte
Hosts pass mount descriptions and fallback components, stop exposing controls after failed rendering, clear failed form state, and use contextual fallback titles.

UI action reporting

Layer / File(s) Summary
Choice action error handling
src/utils/errorUtils.ts, src/utils/errorUtils.test.ts, src/gui/choiceList/ChoiceView.svelte, src/gui/choiceList/ChoiceView.rowActions.test.ts
reportingHandler handles synchronous throws, rejected promises, thenables, cancellations, and ordinary results; choice actions use it while preserving list rendering after failures.

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
Loading

Possibly related issues

Possibly related PRs

Poem

A rabbit saw a mount fail bright,
And raised a fallback card in sight.
No hidden edits, no silent fright—
Row buttons report errors right.
The UI hops on through the night.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main theme: making settings-surface failures visible instead of silent.
Linked Issues check ✅ Passed The changes implement mount failure handling and row-action error reporting with tests, matching the requirements in #1584 and #1585.
Out of Scope Changes check ✅ Passed The modified files all support mount-failure handling, choice-action reporting, or related tests, with no clear unrelated code changes.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chhoumann/1584-settings-hardening

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 27, 2026

Copy link
Copy Markdown

Deploying quickadd with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8a60c68
Status: ✅  Deploy successful!
Preview URL: https://fe6163a5.quickadd.pages.dev
Branch Preview URL: https://chhoumann-1584-settings-hard.quickadd.pages.dev

View logs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/utils/errorUtils.ts (1)

186-206: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use 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 win

Consider 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's catch { container.textContent = ... }) has no dedicated test. A small fallbackComponent fixture 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 win

Duplicate mount-failure handling in the two choice-form builders. Both display() implementations mount their form and then clear this.formProps on !handle.ok with identical logic and comments; the shared contract belongs in the common ChoiceBuilder base class rather than copy-pasted per subclass.

  • src/gui/ChoiceBuilder/captureChoiceBuilder.ts#L67-L86: extract the "mount, then drop formProps on failure" logic into a shared protected helper on ChoiceBuilder (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 identical if (!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

📥 Commits

Reviewing files that changed from the base of the PR and between 234a349 and ec89aa2.

📒 Files selected for processing (18)
  • src/gui/ChoiceBuilder/captureChoiceBuilder.ts
  • src/gui/ChoiceBuilder/components/mountFormatPreview.svelte.ts
  • src/gui/ChoiceBuilder/templateChoiceBuilder.ts
  • src/gui/MacroGUIs/CommandSequenceEditor.ts
  • src/gui/MacroGUIs/CommandSequenceEditor.unrenderable.test.ts
  • src/gui/PackageManager/ExportPackageModal.ts
  • src/gui/PackageManager/ImportPackageModal.ts
  • src/gui/choiceList/ChoiceView.rowActions.test.ts
  • src/gui/choiceList/ChoiceView.svelte
  • src/gui/choiceList/ChoicesUnavailable.svelte
  • src/gui/svelte/MountFailed.svelte
  • src/gui/svelte/mountComponent.test.ts
  • src/gui/svelte/mountComponent.ts
  • src/gui/svelte/mountThrows.fixture.svelte
  • src/gui/svelte/mountThrowsDeep.fixture.svelte
  • src/quickAddSettingsTab.ts
  • src/utils/errorUtils.test.ts
  • src/utils/errorUtils.ts

Comment thread src/utils/errorUtils.ts Outdated
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.
@chhoumann
chhoumann merged commit 3ddbb83 into master Jul 27, 2026
13 checks passed
@chhoumann
chhoumann deleted the chhoumann/1584-settings-hardening branch July 27, 2026 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant