fix: show empty folders as a dead end before the click, not after it - #1569
Merged
Conversation
…fter it A Multi choice with nothing in it rendered exactly like a folder with content. Selecting it could only answer with a notice after the fact (#1550), and that notice was delivered by closing and re-opening the picker, which discarded the typed query, the scroll position and the selection. The row now says so up front: dimmed name and a trailing "Empty" marker - the same word the settings tree already uses for this state - plus aria-disabled for assistive tech. renderSuggestion is the single render path, so plain level rows and cross-level search results are covered by one branch. Activation is refused in selectSuggestion, above SuggestModal's close(), so the picker simply stays put with its query intact instead of being torn down and rebuilt. Two consequences of no longer closing are handled there: focus is returned to the input (a trusted mousedown on a suggestion row moves focus to <body>, which nothing noticed while every click closed the modal), and key auto-repeat is ignored so a held Enter cannot stack notices. Empty is deliberately not recursive: a folder containing only empty folders still has rows to show, and its children carry the marker one level down. Empty folders are marked, never hidden or down-ranked - a configured folder disappearing from the picker would be a worse, unexplained dead end. Closes #1554
…dim tags with the row A choice name with no break opportunities (CamelCase, a URL) overran its box - min-width: 0 shrinks the box, not the glyphs - painting through the trailing "Empty" marker and giving the results list a horizontal scrollbar. That overflow predates this branch; the marker is what makes it visible. The dim also enumerated only the --link-* variables, so a folder named "#projects folder" kept a full-accent tag pill on the one row that is supposed to read as inert - exactly the failure the block's own comment describes for links.
|
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 (3)
📝 WalkthroughWalkthroughChoiceSuggester now detects empty or malformed folders, prevents their activation and navigation, restores picker focus, and displays an “Empty” marker. Rendering and styling updates add accessibility state, muted flair, flexible layout, and robust text wrapping, with expanded tests. ChangesEmpty folder handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant ChoiceSuggester
participant Notice
participant InputElement
User->>ChoiceSuggester: activate empty folder
ChoiceSuggester->>Notice: show emptyFolderNoticeText
ChoiceSuggester->>InputElement: restore focus
ChoiceSuggester-->>User: keep picker open
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: |
6453c00
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f4bbfe00.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-1554-picker-empty.quickadd.pages.dev |
chhoumann
added a commit
that referenced
this pull request
Jul 27, 2026
The reported symptom. MultiChoiceListItem dereferenced choice.choices in
three places, and a throw during MOUNT does not cost one row: it escapes
mount() and abandons the rest of the declarative settings tab, so
QuickAdd's settings came up as a lone "Choices & packages" heading with
nothing under it - no choices, no add controls, and none of the other
eight setting groups.
The list now reads its children through one $derived accessor, filters
list holes out of the keyed {#each} and the dnd zone, and renders a
folder that lost nothing as an ordinary empty folder - which post-#1569
already shows the "Empty - add a choice or drag one here." hint, so it
can be refilled, renamed, moved and deleted. Dropping a choice in
repairs the value.
A folder whose value could still be HOLDING choices is the one case
where "empty" would be a lie, so it says what happened in that same
hint slot and offers neither the drop zone nor the add row: the two
affordances that would overwrite it.
Two backstops for the class rather than this instance, since this is its
third occurrence (#1451, #1507, #1566): a <svelte:boundary> inside
ChoiceView, and a try/catch around the settings tab's Svelte mounts so
one broken view can never take the other groups down with it. Both show
the same card - what failed, where the file is, and that QuickAdd has
not touched it. No retry button: it would re-render the same data, and
anything that could rewrite data.json would destroy the value the user
needs to recover.
A corrupt ROOT `choices` renders that card too, rather than the "No
choices yet" hero whose single CTA would write a fresh list over it.
Refs #1566
chhoumann
added a commit
that referenced
this pull request
Jul 27, 2026
…with it (#1583) * fix(choices): total accessors for a malformed choices value `IMultiChoice.choices` is an invariant the code assumes and nothing enforces. `data.json` is untrusted input, so a hand-edit, an imported package or a sync that merges whole files can leave a folder's children missing, null, or an object where an array belongs - and every reader guarded (or forgot to) on its own, with two guards that look right and are not: `?? []` and `if (choice.choices)` both wave `{}` straight through. Only `Array.isArray` rejects both shapes. Adds the accessors that make that a function instead of a convention: childChoicesOf() READ view; always safe to iterate/map/spread hasChildChoices() WRITE guard; may this node be rebuilt? hasUnreadableChildren() is the value lossy, or genuinely empty? rootChoicesOf() the same, one level up, for settings.choices isChoiceLike() a list entry can be null or a primitive The read/write split is load-bearing. `dedupeChoicesById` deliberately preserves a malformed folder rather than fabricating `[]`, so a walker that REBUILDS a node it is not targeting must pass it through untouched - substituting the read accessor there would persist `[]` over the original value on the next unrelated save, which is exactly the outcome the preservation stance exists to avoid. `dedupeChoicesById` also gains element tolerance: it runs inside loadSettings, ~200 lines before addSettingTab, so a `null` in the list threw before the settings tab was ever registered. Also declares `IMultiChoice.choices` optional. strictNullChecks is on and CI runs svelte-check, so the missing-key shape is now a compile error at every unguarded read in .ts and .svelte alike. Refs #1566 * fix(startup): a malformed folder no longer aborts plugin load A folder in data.json with no `choices` key threw "TypeError: choices is not iterable" out of addCommandForChoice, which runs straight from onload. Obsidian reported only "Plugin failure: quickadd" and everything after that line was lost: no choice commands, migrations never ran, the CLI handlers never registered (so `quickadd:list` was "command not found"), startup macros never ran. The settings tab had already been registered a few lines earlier, which is the only reason the bug looked like "the settings tab is blank". Routes the four tree walks in main.ts through the accessors, and fault-isolates each choice-dependent onload step so the blast radius of a data defect is one capability, not the plugin. The per-choice try/catch in addCommandsForChoices is the same bound for corrupt shapes the accessors do not know about yet. main.ts cannot be imported under vitest (it pulls in `obsidian`), so this half is proven live in an isolated vault rather than by unit test. Refs #1566 * fix(choices): never rewrite a malformed folder on an unrelated edit The tree walkers in choiceService had to satisfy two things at once over a preserved malformed folder: never throw on it, and never rewrite it. The second is the one that is invisible until it has already happened. updateMultiById re-spreads EVERY folder it walks past, not just the one it targets (its doc comment claiming otherwise was wrong), and it is on the save path - setMultiCollapsedById, setFolderChildrenById. So reading its children through the total accessor would have persisted `[]` over the preserved value the first time the user collapsed an unrelated folder. Same shape in removeChoiceById, insertIntoMulti, insertChoiceAfter. So: reads go through childChoicesOf, and any walker that REBUILDS a node it is not targeting returns it untouched instead (hasChildChoices). Writing into a malformed folder is allowed only when its value was carrying nothing - `{}`, null, no key at all - which makes the folder usable again; when the value could still hold choices the write is refused so the caller falls back rather than destroying it. Duplicating carries an unreadable value across verbatim, and the delete confirmation now says so before the folder goes. The walkers also step over a hole in the list itself (a `null` or a stray primitive) rather than dereferencing it. Every case in the new suite asserts the malformed values are identical afterwards, not merely that nothing threw - that assertion is the one that fails on the naive fix. Refs #1566 * fix(choices): route the remaining readers through the accessor The rest of the reach: running a folder from its command, the launcher's nested search, the context menu, the macro builder, the CLI, and the migrations. Two of these were worse than a throw. choiceExecutor read `multiChoice.choices.length === 0`, and `({}).length` is `undefined`, so the empty-folder guard slid past and handed the picker a non-list - a dead picker instead of the honest "this folder is empty" notice. And migrations/helpers/isMultiChoice gated on `choices !== undefined`, which `{}` satisfies, so two write migrations recursed into it and threw; migrate() then reverted and left the flag unset, so those migrations re-ran and re-failed on every single launch, logging a "please create an issue" error each time. consolidateFileExistsBehavior went the other way and replaced a corrupt root `choices` with [], which the next save persisted. A migration should not destroy the data a user needs to recover by hand; it now walks past what it cannot read. Its test is inverted to lock that in. Also drops the CLI's private copy of flattenChoices - a second place to forget the guard - in favour of the shared, now-total one. Refs #1566 * fix(settings): a malformed folder no longer blanks the whole tab The reported symptom. MultiChoiceListItem dereferenced choice.choices in three places, and a throw during MOUNT does not cost one row: it escapes mount() and abandons the rest of the declarative settings tab, so QuickAdd's settings came up as a lone "Choices & packages" heading with nothing under it - no choices, no add controls, and none of the other eight setting groups. The list now reads its children through one $derived accessor, filters list holes out of the keyed {#each} and the dnd zone, and renders a folder that lost nothing as an ordinary empty folder - which post-#1569 already shows the "Empty - add a choice or drag one here." hint, so it can be refilled, renamed, moved and deleted. Dropping a choice in repairs the value. A folder whose value could still be HOLDING choices is the one case where "empty" would be a lie, so it says what happened in that same hint slot and offers neither the drop zone nor the add row: the two affordances that would overwrite it. Two backstops for the class rather than this instance, since this is its third occurrence (#1451, #1507, #1566): a <svelte:boundary> inside ChoiceView, and a try/catch around the settings tab's Svelte mounts so one broken view can never take the other groups down with it. Both show the same card - what failed, where the file is, and that QuickAdd has not touched it. No retry button: it would re-render the same data, and anything that could rewrite data.json would destroy the value the user needs to recover. A corrupt ROOT `choices` renders that card too, rather than the "No choices yet" hero whose single CTA would write a fresh list over it. Refs #1566 * test(choices): sweep a malformed tree through every entry point The bug was never one unguarded dereference. data.json is untrusted and roughly forty readers each decided for themselves whether to guard, using guards that look right and are not - so fixing the readers we know about does nothing about the next one. One fixture tree carrying every malformed shape, pushed through every exported walker. Each asserts twice: nothing throws, AND the malformed values are identical afterwards. The second assertion is what catches a "fix" that quietly repairs the tree to [] and lets the next save destroy it; without it the whole preservation stance is decorative. The sweep immediately earned its keep, finding two readers the audit had missed: computeEligibleMultiTargets (so a single list hole meant NO row in the settings list had a context menu) and packageTraversal's choice catalog (so package export/import walked into it). Adding a new walker to the list costs one line. Refs #1566 * fix(choices): survive a hole in the choice list on the load path Found live, not by the unit suite: main.ts cannot be imported under vitest (it pulls in `obsidian`), so its walkers are only reachable in a real vault. A `null` entry in data.json's choice list made getChoiceById / getChoiceByName throw, which breaks every registered choice command's callback and the URI handler. The same hole reverted two migrations, and a migration that throws is not a one-time failure: migrate() restores the backup and leaves the flag unset, so it re-runs and re-fails on every launch forever, logging a "please create an issue" error each time. Also stops three more migrations from replacing a corrupt ROOT `choices` with [], for the same reason consolidateFileExistsBehavior stopped: the next save would persist it. And makes the picker's notice tell the same story the settings list does. "Folder X is empty" is true for a folder that lost nothing, and a lie for one whose contents merely could not be read - so both surfaces now read the one predicate rather than disagreeing about the same folder. Refs #1566 * fix(choices): close the gaps an adversarial review found Three reviewers attacked the branch and 18 findings survived verification. The most important was a regression this change introduced: removeMacroIndirection MOVES legacy macros into the choice tree and then deletes the old `macros` array, so making flattenChoices total turned "throws, reverts, retries next launch" into "completes, and every legacy macro is silently gone forever" - the exact failure direction the change exists to prevent. It now returns { complete: false } and destroys nothing, so a vault repaired by hand still gets migrated. The three sibling migrations that skip an unreadable root stay pending for the same reason. The rest were readers and writers the first pass missed, all found the same way: the fixture only placed corruption at the ROOT of the tree, so a walker that guards its entry point but not its recursion passed. With corruption at every depth the sweep immediately caught duplicateChoice, packageTraversal's dependency collection, and the package-import writers - which fabricated [] over an unreadable value, the one invariant the whole change turns on. Also: the export modal's descendant walk (a `?? []` guard, the exact anti-pattern documented in choiceUtils), the keyboard reorder path (fixed for rendering but not for ArrowUp/ArrowDown), MacroBuilder's flatten, and ChoiceView's subtreeHasCommand. Two consistency fixes so the surfaces stop disagreeing about the same folder: "Move to" no longer offers a folder that would refuse the write, and the picker marks an unreadable folder "Unreadable" rather than "Empty". Refs #1566 * fix(migrations): stay pending for an unreadable folder at any depth Codex caught this on the PR: the previous guard only asked about the ROOT. With a valid root array but an unreadable NESTED folder, the migration ran to completion - and the now-total flattenChoices could not see that folder's descendants, so a legacy macro one of them referenced looked orphaned, got duplicated at the root, and `settings.macros` was deleted. The hidden choice keeps a dangling macroId forever, because a completed migration never retries. Same class as the bug being fixed, one level down. treeHasUnreadableChildren asks the question about the whole tree, and the four migrations that skip what they cannot read now stay pending on it. Refs #1566 * fix(settings): drop unkeyable entries instead of losing the whole list Following a CodeRabbit thread about the render filter, an adjacent case turned out to be worse than the one it raised: an entry that IS an object but has no `id`. svelte-dnd-action reads `.id` on every item, and the keyed {#each} needs it present and unique - so two id-less entries raise `each_key_duplicate`, the #1451 crash. The boundary added in this PR catches it, but the user still loses every real row with it. Filtering them out at render degrades far better: the junk rows vanish and the real choices stay. Nothing is rewritten - the entry stays in data.json, and it cannot be hiding a choice, since only a Multi's `choices` value can and that is preserved separately. Refs #1566 * test(settings): assert unkeyable entries are absent, not merely unkeyed Per review: counting keyed rows would still pass if a regression painted the junk entries as rows without a data-choice-id. Assert both are absent from the DOM. Verified the strengthened test fails with the filter reverted. Refs #1566
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1554. Follow-up to #1540 / #1550.
The problem
In the run picker, a folder with nothing in it renders exactly like a folder with content. The click is dead either way; the user only finds out afterwards.
#1550 made that afterwards less painful - a notice instead of a level whose only row was
← Back- but the notice was delivered by closing the picker and re-opening it, becauseSuggestModal.selectSuggestioncloses the modal beforeonChooseItemruns. Reproduced on master in this worktree's isolated Obsidian 1.13.0 vault: witharchtyped and the nestedArchivefolder as the only result, one click gives the notice and an empty query and the root level back, because the reopen replays the level the picker was on, not the searched row's level.What changed
The row says it before the click
renderSuggestionmarks an empty folder: the name dims to--text-mutedand a trailing Empty marker sits at the row's edge, witharia-disabled="true"on the row. It is the single render path, so plain level rows and cross-level search results are covered by one branch."Empty" is the word the settings tree already uses for this exact state (
Empty — add a choice or drag one here.,ChoiceList.svelte), so the two surfaces name it the same thing. Colour is never the only signal - there is a word, and the accessibility tree reports the row asdisabledwith an unignoredStaticText "Empty".The click is refused where it can be refused cheaply
Activation is intercepted in
selectSuggestion, which is the one seam aboveSuggestModal'sclose(). Returning there leaves the modal untouched, so the query, scroll position and selection all survive; the notice stays as the confirmation that nothing happened. Both input paths land there (the chooser routes clicks and Enter through it, verified live).Two consequences of no longer closing are handled in the same branch, both caught by review and both measured in the live app:
.suggestion-item- a non-focusable div, and neither.modalnor.modal-containercarries a tabindex - moves focus to<body>. Obsidian never has to deal with it because every other activation closes the modal. WithoutinputEl.focus()the picker looked alive (arrow keys and Escape still worked) while typing was dead.repeat, so a held Enter would stack one identical toast per repeat. Repeats are ignored (property read, notinstanceof KeyboardEvent- a popout window is a separate realm).onChooseMultiType's empty branch stays as a backstop for programmaticonChooseItemcallers; #1550's close-and-reopen is gone with the ejection it was working around.src/choiceExecutor.tsis unchanged - the command/URI path keeps its notice, which is the only feedback the command palette permits.Two adjacent fixes the marker exposed
.quickadd-choice-suggestion-texthad no overflow handling, so a name with no break opportunities (CamelCase, a URL) overran its box -min-width: 0shrinks the box, not the glyphs - and painted through the new marker.overflow-wrap: anywhere(notnowrap+ ellipsis, which would kill the deliberate wrap). This also repairs the pre-existing horizontal scrollbar the results list got from such a name on master.--link-*, so a folder named#projects folderkept a full-accent tag pill on the one row that is supposed to read as inert - the exact failure the rule's own comment describes for links.--tag-*is pinned too.<mark>/<kbd>keep their theme defaults rather than being chased with element selectors.Design decisions, and what was rejected
SuggestModalhas no disabled-item concept; skipping arrow-key focus needs undocumentedchooserinternals. Obsidian's own precedent (unresolved links) is dim-but-still-clickable.Notice.setMessageissetTextonly: it neither re-arms the timer nor revives an expired notice, so "reuse the instance" means a second press 5s later gives no feedback at all, and "skip repeats by id" makes a deliberate second press silent - the "nothing happened" state this issue exists to remove. The unbounded case (one held key, ~30 events/s) is what therepeatguard handles.role="option"/role="listbox". Obsidian core owns.prompt-resultsand signals selection with a class only; declaring a listbox with noaria-selected/aria-activedescendantwould tell assistive tech every option is unselected - a lie rather than terseness.Validation
Everything below was exercised in this worktree's isolated Obsidian 1.13.0 vault, not just in tests.
aria-disabled="true"; dim measured atrgb(92,92,92)vsrgb(34,34,34)(light, ~7:1 on white) and179vs218(dark). Back rows, leaf choices and non-empty folders unmarked.Input.dispatchMouseEvent): 1 modal still open, queryarchpreserved,document.activeElement === .prompt-input, and a following keystroke extends the query.repeat: trueEnters → exactly 1 notice, modal still open.Workstill opens its level with the placeholderWork; Back restoresSelect a choice; the command/URI path still notices.[[Reading]] queueand#projects folderboth render fully muted inside an empty row.textEl.scrollWidth - clientWidth === 0,.prompt-resultshorizontal overflow 0), on desktop and under mobile emulation.pnpm run build,pnpm lint,pnpm test(3998 passing) green;dev:errorsclean throughout.Review
Two multi-agent adversarial passes, one on the design before any code and one on the finished branch (independent lenses → find, then independent refutation of every finding). Both changed the outcome:
6453c00f. It also killed a proposedhadFocusguard aroundinputEl.focus()by measuring thatactiveElementis already<body>on the click path - the guard would have been false on exactly the path the line exists for.Notes for review
fix:commits, release-worthy. No migration, no settings-schema change, no API change.main.tshard-dereferenceschoice.choiceswhile registering commands, so adata.jsonwhose Multi lost itschoiceskey half-loads the plugin (commands registered before that point survive, the rest ofonloadaborts). Pre-existing on master; the?.lengthinisEmptyFolderChoiceis what keeps this picker working in that state, and there are two tests pinning it.Summary by CodeRabbit
New Features
Bug Fixes