feat: rework add-choice UX (explained types, Multi-as-folder, add-into-folder) + fix AI-disabled layout#1257
Open
chhoumann wants to merge 5 commits into
Conversation
… AI-disabled layout Replace the cramped [Name] [type] [Add Choice] row with a clearer flow: - Top action bar: "New choice" (an Obsidian menu of the three doer types — Template/Capture/Macro — each with a one-line description) plus a separate "New folder" button. Multi is no longer a confusing peer "type"; it reads as a folder. Renamed Multi -> "folder" in user-facing copy (stored ChoiceType value unchanged, no migration). - First-class add-into-folder: a per-folder add affordance inside each expanded folder (rendered OUTSIDE the dndzone). Adds route through the top-level handler via the exported insertIntoMulti/addChoiceToTree, so nested adds persist correctly at any depth (the rootReorder invariant). - After adding a doer its builder opens automatically (Alt-click skips); folders skip it. Auto-named so the flow never gates on typing a name first. - Empty/first-run state with a single hero CTA. - Fix the layout bug: the old .choiceViewBottomBar used space-between, so the add box collapsed left when the AI Assistant button was hidden. The add controls are now a left-anchored, content-width cluster (margin-right:auto) with the AI button pinned right — the add cluster's position is independent of the AI button. Tests: addChoiceToTree + insertIntoMulti unit tests; a ChoiceView add-into-folder integration test (drives the per-folder affordance through the real DOM); rewritten add-flow test. a11y: WCAG 2.5.3 label-in-name on the add buttons, aria-haspopup/aria-expanded on the menu trigger. MultiChoice initializes collapsed=false. Deletes AddChoiceBox.svelte. Validated end-to-end in a real Obsidian 1.13.0 dev vault: layout fixed in both AI states; New folder + add-into-folder persist (nested at depth); the "New choice" menu is parity-identical to the existing context menu. Closes #1256
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: |
8f4284e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://49187967.quickadd.pages.dev |
| Branch Preview URL: | https://1256-rework-add-choice-ux-cl.quickadd.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/gui/choiceList/ChoiceView.svelte`:
- Around line 110-135: The code in ChoiceView.svelte currently uses
console.error when a builder throws in the block around createChoice/type
handling; replace that console.error call with the project logger (use
log.logError from src/logger/logManager.ts), making sure to import the logger
(e.g., import { log } from "src/logger/logManager") and pass both a descriptive
message and the caught error (e.g., "QuickAdd: failed to configure new choice",
err) to log.logError; leave the existing save() call intact so behavior is
unchanged.
- Line 129: Replace the console.error call in ChoiceView.svelte ("QuickAdd:
failed to configure new choice", err) with the repo logger: import { log,
toError } from the logger module and call log.logError(toError(err)) (or
log.logError("QuickAdd: failed to configure new choice", toError(err))) so
errors use the centralized logging utilities; update the import block to include
log and toError and remove the console.error usage.
🪄 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
Run ID: 6da11c46-8f6a-4470-b1b1-129dd0240db1
📒 Files selected for processing (13)
src/gui/choiceList/AddChoiceBox.sveltesrc/gui/choiceList/AddChoiceControls.sveltesrc/gui/choiceList/ChoiceList.a11y.test.tssrc/gui/choiceList/ChoiceList.callbacks.test.tssrc/gui/choiceList/ChoiceView.sveltesrc/gui/choiceList/ChoiceView.test.tssrc/gui/choiceList/MultiChoiceListItem.sveltesrc/gui/choiceList/choiceListActions.tssrc/gui/choiceList/choiceTypeMeta.tssrc/gui/choiceList/cmpLifecycle.test.tssrc/services/choiceService.insertIntoMulti.test.tssrc/services/choiceService.tssrc/types/choices/MultiChoice.ts
💤 Files with no reviewable changes (1)
- src/gui/choiceList/AddChoiceBox.svelte
Comment on lines
110
to
135
| const newChoice = createChoice(type, name); | ||
| choices = [...choices, newChoice]; | ||
| save(); | ||
| choices = addChoiceToTree(choices, newChoice, targetFolderId); | ||
|
|
||
| // A root-level add while a filter is active would otherwise look like | ||
| // nothing happened (the auto-named choice may not match the filter). | ||
| if (!targetFolderId && filterQuery.trim().length > 0) { | ||
| filterQuery = ""; | ||
| } | ||
|
|
||
| // Doers (Template/Capture/Macro) hand off to their builder so the user | ||
| // isn't stranded on an unconfigured row; handleConfigureChoice persists | ||
| // the result, so there's no eager save here (avoids a double write). | ||
| // Folders and Alt-click skip the builder and are committed immediately. | ||
| if (type !== "Multi" && !skipConfigure) { | ||
| try { | ||
| await handleConfigureChoice(newChoice); | ||
| } catch (err) { | ||
| // Builders resolve rather than reject, but don't let a stray throw | ||
| // become an unhandled rejection or lose the new choice. | ||
| console.error("QuickAdd: failed to configure new choice", err); | ||
| save(); | ||
| } | ||
| } else { | ||
| save(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect waitForClose resolution / onClose / cancel handling in the builders
fd -e ts -e svelte | rg -l 'waitForClose' | sort -u
ast-grep --pattern 'waitForClose = $_'
rg -nP -C4 '\bwaitForClose\b' --type=ts
rg -nP -C4 '(resolvePromise|this\.resolve|reject)\s*\(' --type=ts -g '*Builder*'Repository: chhoumann/quickadd
Length of output: 45929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect ChoiceView.svelte around handleConfigureChoice
rg -n "handleConfigureChoice" src/gui/choiceList/ChoiceView.svelte
rg -n "configureChoice|waitForClose" src/gui/choiceList/ChoiceView.svelte
sed -n '1,220p' src/gui/choiceList/ChoiceView.svelte | sed -n '90,190p'
# 2) Locate concrete builders (template/capture) and inspect their close/cancel handling
fd -t f "template*ChoiceBuilder*" src/gui | sort
fd -t f "capture*ChoiceBuilder*" src/gui | sort
fd -t f "*ChoiceBuilder*" src/gui/ChoiceBuilder src/gui/MacroGUIs 2>/dev/null | sort
# show key parts of the base ChoiceBuilder and concrete builders
sed -n '1,260p' src/gui/ChoiceBuilder/choiceBuilder.ts
for f in $(fd -t f "*template*ChoiceBuilder*" src/gui | sort); do
echo "---- $f"; sed -n '1,260p' "$f"; done
for f in $(fd -t f "*capture*ChoiceBuilder*" src/gui | sort); do
echo "---- $f"; sed -n '1,260p' "$f"; done
# 3) Inspect MacroBuilder close behavior (since it was found)
sed -n '1,220p' src/gui/MacroGUIs/MacroBuilder.tsRepository: chhoumann/quickadd
Length of output: 3535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect configureChoice + builder cancel semantics
rg -n "async function configureChoice|function configureChoice|configureChoice\\(" src/services/choiceService.ts
sed -n '1,260p' src/services/choiceService.ts
sed -n '120,220p' src/services/choiceService.ts
# Inspect base ChoiceBuilder close/resolve behavior
sed -n '1,280p' src/gui/ChoiceBuilder/choiceBuilder.ts
# Inspect Template/Capture builders close/cancel
sed -n '1,260p' src/gui/ChoiceBuilder/templateChoiceBuilder.ts
sed -n '1,320p' src/gui/ChoiceBuilder/captureChoiceBuilder.ts
# Also locate any "cancel" handlers in these builders
rg -n "cancel|onCancel|close\\(|onClose\\(|destroy\\(" src/gui/ChoiceBuilder/templateChoiceBuilder.ts src/gui/ChoiceBuilder/captureChoiceBuilder.ts src/gui/ChoiceBuilder/choiceBuilder.tsRepository: chhoumann/quickadd
Length of output: 36241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) show ChoiceBuilder.onClose fully (around where it resolves waitForClose)
sed -n '200,280p' src/gui/ChoiceBuilder/choiceBuilder.ts
# 2) inspect the ChoiceView catch block region for context
sed -n '90,180p' src/gui/choiceList/ChoiceView.svelte
# 3) inspect logger utilities (look for logManager methods)
rg -n "class .*logManager|function log|export const log|logger" src/logger/logManager.ts src/logger -S || true
ls -R src/logger || true
sed -n '1,220p' src/logger/logManager.ts
# 4) find how other code logs errors (console.error vs logger.log/error)
rg -n "console\.error\(" src/gui/choiceList/ChoiceView.svelte src/gui/ChoiceBuilder src/gui -S || true
rg -n "log\\.(error|warn|info)|logger\\." src/logger src/gui -S | head -n 50Repository: chhoumann/quickadd
Length of output: 8161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "log\\.logError\\(|log\\.logWarning\\(|log\\.logMessage\\(" src | head -n 50
rg -n "quickAddLogger|guiLogger|consoleErrorLogger" src/logger src/gui -S
sed -n '1,220p' src/logger/quickAddLogger.ts
sed -n '1,220p' src/logger/consoleErrorLogger.tsRepository: chhoumann/quickadd
Length of output: 8702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/gui/choiceList/ChoiceView.svelte
rg -n "console\\.error\\(" src/gui/choiceList/ChoiceView.svelte
rg -n "\\blog\\b|logManager|log\\.logError" src/gui/choiceList/ChoiceView.svelteRepository: chhoumann/quickadd
Length of output: 4246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "import .*log|from \"src/logger|log\\b" src/gui/choiceList/ChoiceView.svelte
sed -n '1,80p' src/gui/choiceList/ChoiceView.svelteRepository: chhoumann/quickadd
Length of output: 44
Replace console.error with the project logger
The “builder cancel leaves an unsaved phantom choice” concern doesn’t match current behavior: ChoiceBuilder.onClose() resolves waitForClose with this.choice (so configureChoice() returns an IChoice, not undefined, for Template/Capture/Macro), and handleConfigureChoice() will call save().
src/gui/choiceList/ChoiceView.svelte still logs via console.error(...); switch to log.logError(...) (from src/logger/logManager.ts) to follow the logger utilities.
🤖 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/choiceList/ChoiceView.svelte` around lines 110 - 135, The code in
ChoiceView.svelte currently uses console.error when a builder throws in the
block around createChoice/type handling; replace that console.error call with
the project logger (use log.logError from src/logger/logManager.ts), making sure
to import the logger (e.g., import { log } from "src/logger/logManager") and
pass both a descriptive message and the caught error (e.g., "QuickAdd: failed to
configure new choice", err) to log.logError; leave the existing save() call
intact so behavior is unchanged.
| } catch (err) { | ||
| // Builders resolve rather than reject, but don't let a stray throw | ||
| // become an unhandled rejection or lose the new choice. | ||
| console.error("QuickAdd: failed to configure new choice", err); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the logger utility location and its public API
fd -i logger -e ts
rg -nP -C2 '(export (const|class|function)\s+\w*[Ll]og|logError|logMessage)' --type=ts -g '*[Ll]og*'Repository: chhoumann/quickadd
Length of output: 5446
Use the repo logging utilities (log) instead of console.error in ChoiceView.svelte
Replace console.error("QuickAdd: failed to configure new choice", err); with log.logError(...) (e.g., log.logError(toError(err))), importing { log, toError } from src/logger/logManager.ts.
🤖 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/choiceList/ChoiceView.svelte` at line 129, Replace the console.error
call in ChoiceView.svelte ("QuickAdd: failed to configure new choice", err) with
the repo logger: import { log, toError } from the logger module and call
log.logError(toError(err)) (or log.logError("QuickAdd: failed to configure new
choice", toError(err))) so errors use the centralized logging utilities; update
the import block to include log and toError and remove the console.error usage.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Addresses hands-on QA findings on the redesign:
- New folder now auto-opens rename immediately (clear feedback; the click no
longer "looks like nothing happened", and it avoids duplicate "New folder"
confusion). Cancelling rename keeps the default name.
- Every add scrolls the new row into view (rows now carry data-choice-id) so a
new root choice doesn't land unseen at the bottom of a long list.
- Quieter per-folder ("compact") controls: ghost styling, no mod-cta accent, so
nested add buttons don't compete with the top-bar primary CTA.
…add as text links
Per maintainer QA:
- New choice / New folder move back to the bottom of the view, pinned right via
margin-left:auto (not the old justify-content:space-between), so they stay put
when the AI Assistant button is hidden — no return of the collapse bug.
- Per-folder ("compact") add affordances now render as quiet, clickable text —
"Add choice" / "Add folder" — with a folder-named tooltip + aria-label
("Add choice to {folder}") instead of filled buttons. WCAG 2.5.3 still holds
(the visible text is a substring of the accessible name). The type-menu chevron
shows only on the global control.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/gui/choiceList/ChoiceView.svelte`:
- Around line 322-351: The add-controls cluster (.choiceAddGroup /
AddChoiceControls) is currently pinned to the right via margin-left:auto, which
causes CTAs to shift right when the AI button is hidden; change the CSS to pin
the add cluster to the left by replacing margin-left:auto with margin-right:auto
in the .choiceAddGroup rule so the add controls stay left-anchored and the AI
button sits to their right; apply the same change to the other occurrence
mentioned (lines 411-417) to keep both blocks consistent.
🪄 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
Run ID: 5df49ba1-b78f-4f56-8425-3a9371d0eed7
📒 Files selected for processing (4)
src/gui/choiceList/AddChoiceControls.sveltesrc/gui/choiceList/ChoiceView.sveltesrc/gui/choiceList/ChoiceView.test.tssrc/gui/choiceList/MultiChoiceListItem.svelte
🚧 Files skipped from review as they are similar to previous changes (3)
- src/gui/choiceList/MultiChoiceListItem.svelte
- src/gui/choiceList/AddChoiceControls.svelte
- src/gui/choiceList/ChoiceView.test.ts
Per the one-primary-button-per-page rule (/ui Buttons guideline): "New choice" is
the view's single primary action, so the competing purple CTAs near it are demoted
to secondary:
- "Export package…" no longer calls .setCta() (both package actions are secondary
utilities), removing the purple-over-purple with "New choice".
- The "AI Assistant" button drops mod-cta (it's a configure-AI utility), so AI
being enabled doesn't reintroduce a second purple in the bottom bar.
Only "New choice" remains a filled primary; New folder / Export / Import / AI are
secondary. ("Add variable" stays primary within its own Global Variables section.)
… no hairline) From design iteration in the dev vault: - Right-pack the add cluster (justify-content:flex-end) — no margin-left:auto gutter, so the bar stays put when the AI button toggles without needing a grounding hairline (the line read as too heavy next to the Packages divider). - AI Assistant becomes a quiet icon button (sparkles / clickable-icon) leading the cluster, so the bar's width barely changes when AI/online features toggle. - Order is AI | New folder | New choice — the primary "New choice" is the terminal (rightmost) action. Reordered in AddChoiceControls so it's consistent everywhere (including the per-folder "Add folder | Add choice" text links).
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 #1256.
Reworks how users add a choice in the settings Choices view, and fixes the reported layout bug. Built on top of #1255 (Obsidian 1.13 declarative settings).
The problem
The old add row —
[Name] [Template/Capture/Macro/Multi ▾] [Add Choice]— presented the four types as an unexplained dropdown, and "Multi" (really a folder) masqueraded as a peer type. And when AI/online features were disabled, the add box collapsed hard to the left (.choiceViewBottomBarusedjustify-content: space-between, so the lone add box had nothing to balance against).What changed
+ New choice ▾— an Obsidian menu of the three doer types, each with a one-line description — plus a separate+ New folderbutton. Multi is no longer a peer "type"; it reads as a folder. Renamed Multi → "folder" in user-facing copy (storedChoiceTypevalue unchanged — no migration).insertIntoMulti/addChoiceToTree, so nested adds persist correctly at any depth (therootReorderinvariant).margin-right: auto) with the AI button pinned right — the add cluster's position is independent of the AI button, so it can't collapse when AI is disabled.aria-haspopup/aria-expandedon the menu trigger.AddChoiceBox.svelte;MultiChoicenow initializescollapsed = false.Verification
CI-equivalent locally:
tsc,svelte-check(0 errors),lint, 1470 tests — including newaddChoiceToTree/insertIntoMultiunit tests and a ChoiceView add-into-folder integration test that drives the per-folder affordance through the real DOM (asserts the new choice lands nested and the folder stays expanded).Real Obsidian 1.13.0 dev vault (eval +
data.jsonassertions): top bar rendersflex; add cluster left at content-width while the AI button pins far right when on and the cluster doesn't move when off (bug fixed in both states);New folderand per-folder add persist (nested at depth 1); zero runtime errors.Adversarial review (4 agents) ran on the diff and verified the load-bearing invariants hold (dndzone isolation, top-level
insertIntoMultipersistence with no depth≥2 corruption, snapshot plainness, no./mainvalue-import). Its recommended items are all addressed in this PR: the add-into-folder test, the empty-state double-render, WCAG labels, and dropping the eager double-save.Scope notes
+ New choicemenu → pick-type → builder flow and the empty state can't be synthetically driven (Obsidian menus don't open via programmatic clicks — verified the existing context menu behaves identically), so those are covered by unit tests + parity rather than a scripted end-to-end.Summary by CodeRabbit
New Features
Refactor
Bug Fixes & Stability
Tests