fix(gui): preserve choice-builder edit position by converting the ChoiceBuilder trio to Svelte (#1130)#1330
Merged
Conversation
…1130) First slice of the imperative-GUI -> Svelte migration that fixes #1130 (builder modals fully refresh on button clicks, losing scroll/caret). Adds reactive replacements for the Obsidian Setting builders so conditional rows live in {#if} blocks instead of forcing reload(): - SettingItem/Toggle/Dropdown: Obsidian-exact .setting-item / .checkbox-container / .dropdown markup (zero new CSS, native theming) - ChoiceNameHeader, OnePageOverrideSetting, OpenFileSetting, FileOpeningSetting, AppendLinkSetting (collapses the duplicated capture/template logic) - ValidatedInput: Svelte port of validatedInput.ts (validateToken staleness guard, required/disabled, aria-live hint, suggester use: action) - FormatPreviewField: un-debounced preview (parity), token-guarded - suggesterAction.ts use: action; *FormProps factories carrying the snapshot-the-proxy persistence contract Additive only (nothing wired yet). Gates green: tsc, eslint, svelte-check 0/0, vitest (+14 new component tests).
Replace templateChoiceBuilder's imperative display()/reload() with a mounted
TemplateChoiceForm.svelte: every conditional row (folder sub-tree, append-link
cascade, file-exists mode, file-opening) is now a reactive {#if}, so toggling a
control updates in place instead of tearing down the modal — no lost scroll/caret.
Persistence-blocker fix: a proxy does not write through to the original
object, so ChoiceBuilder.onClose now resolves snapshot(getResultChoice()), and
converted builders override getResultChoice() to return the form's proxy
(this.formProps.choice). Still-imperative CaptureChoiceBuilder keeps the default
(mutates this.choice in place) and is unaffected.
Tests: TemplateChoiceForm reactive-reveal-without-remount + the persistence
read-back that fails if the original (not the proxy) is snapshotted. Gates green:
tsc, eslint, svelte-check 0/0, vitest 1831 pass.
Replace captureChoiceBuilder's imperative display()/reload() (~13 reload sites)
with a mounted CaptureChoiceForm.svelte composed of section components:
CaptureTargetSetting (+ keyed CanvasNodePicker), WritePositionSetting (precedence
ladder as $derived, mutual-exclusivity zeroing, insert-after/before fields,
canvas-compat notice), InsertAfter/InsertBeforeFields, plus the shared primitives.
Pure canvas helpers extracted verbatim to canvasNodes.ts behind an app seam.
All conditional rows are reactive {#if} over the choice proxy, so toggling a
control updates in place — no modal teardown, no lost scroll/caret. Up-front
normalizeChoice mirrors the imperative render-time defaults (guarded to avoid
persisted-shape drift). Extends the vitest obsidian stub with vault.getFiles/
getAbstractFileByPath.
Tests: write-position mutual-exclusivity + reveal-without-remount, capture-to-
active-file gating, proxy persistence. Gates green: tsc, eslint, svelte-check 0/0,
vitest 1834 pass.
Both subclasses now mount a Svelte form, so delete the imperative base helpers (reload, addCenteredChoiceNameHeader, addOpenFileSetting, addFileOpeningSetting, addOnePageOverrideSetting) and the dead addFileSearchInputToSetting, plus their now-unused imports (Setting, setIcon, GenericTextSuggester, normalizeFileOpening, promptRenameChoice). The base keeps only the modal lifecycle, the svelteElements registry, and the snapshot-on-close persistence boundary. This removes the last reload() in the ChoiceBuilder trio — completing the #1130 fix: the choice builder modals no longer tear down/rebuild on button clicks, so scroll position and input caret are preserved. Gates green: tsc, eslint, svelte-check 0/0, vitest 1834 pass, production build clean.
…t insert-before (#1130) Adversarial review (codex) found two real issues the initial e2e missed (it tested existing = plain-JSON choices, not freshly-created ones): - HIGH: createChoice() returns class instances (new CaptureChoice/TemplateChoice), and Svelte's proxy() returns non-Object-prototype values UNCHANGED (proxy.js:48), so $state(initial) did not deeply proxy a new choice — nested toggles wouldn't re-render conditional {#if} rows in the add-new flow. Fix: structuredClone the choice in the form-props factories to a plain object, which $state proxies deeply (also unifies the snapshot-on-close persistence path). Regression test uses a real CaptureChoice instance and fails without the clone. - MEDIUM: InsertBeforeFields didn't default createIfNotFound/createIfNotFoundLocation (InsertAfterFields did), so a legacy/imported insert-before choice could persist undefined and throw 'Unknown createIfNotFoundLocation' in the capture formatter (and crash on bind:checked={undefined}). Default them synchronously at init. Gates green: tsc, eslint, svelte-check 0/0, vitest 1835 (+1 regression).
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughMigrates ChoiceBuilder modals from imperative Obsidian Setting construction to Svelte 5 reactive forms. Adds shared UI primitives (Toggle, Dropdown, SettingItem, ValidatedInput, suggester action), mid-level setting components, canvas node utilities/picker, cloned $state form props for reactive editing, and snapshot-based result resolution; includes tests for behavior and persistence. ChangesSvelte-Based Choice Builder Refactor
🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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: |
6febe11
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://57c92b4f.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-1130-modal-refresh.quickadd.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d47c659113
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…l-refresh-edit-pos # Conflicts: # src/gui/ChoiceBuilder/captureChoiceBuilder.ts # src/gui/ChoiceBuilder/templateChoiceBuilder.ts
![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
🧹 Nitpick comments (4)
src/gui/ChoiceBuilder/components/ValidatedInput.test.ts (2)
15-16: 💤 Low valueDouble
tick()calls may be unnecessary.Lines 15-16, 23-24, and 58-59 each call
await tick()twice in succession. Typically a singletick()flushes Svelte's pending updates. Unless there's a known multi-tick requirement in this component, consider simplifying to onetick()per wait point.Also applies to: 23-24, 58-59
🤖 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/components/ValidatedInput.test.ts` around lines 15 - 16, The test in src/gui/ChoiceBuilder/components/ValidatedInput.test.ts is calling await tick() twice at multiple wait points (in the blocks around the first/second and later waits); remove the duplicate calls so each wait point uses a single await tick() (e.g., change occurrences where there are two consecutive await tick() calls to a single await tick()), keeping test semantics the same and running the assertions after the single tick.
47-62: ⚡ Quick winStaleness guard test could verify intermediate results are dropped.
The test fires two input events (
"bad"then"good") and asserts the final hint is empty, but it doesn't verify that the intermediate"Invalid value"message from the"bad"validation was never displayed. Consider adding an assertion after the first input event to confirm the stale result doesn't appear.🤖 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/components/ValidatedInput.test.ts` around lines 47 - 62, Update the "keeps only the latest async validation result (staleness guard)" test in ValidatedInput.test.ts to assert that the intermediate stale validation result is never shown: after calling fireEvent.input(input) with "bad" (and awaiting the minimal ticks/promises needed for the async validator to run), read the hint (container.querySelector(".qa-field-hint")) and assert it does not equal "Invalid value" (or is empty) before proceeding to set "good" and asserting the final state; this ensures the validator/ValidatedInput component's staleness guard drops the intermediate result.src/gui/ChoiceBuilder/components/FormatPreviewField.svelte (1)
28-31: 💤 Low valueClarify comment about
$derivedbehavior.The comment states the formatter is "computed once," but
$derivedis a reactive primitive that re-runs when dependencies change. While the props (app/plugin/formatterKind) are stable in practice for this component's lifetime, the phrasing "once" might mislead future readers. Consider rephrasing to "app/plugin/kind are stable for this field's lifetime, so $derived computes the formatter exactly once in practice."🤖 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/components/FormatPreviewField.svelte` around lines 28 - 31, Update the in-code comment near the $derived usage in FormatPreviewField.svelte to avoid implying $derived never re-runs: change the phrasing to state that "app/plugin/formatterKind are stable for this field's lifetime, so $derived computes the formatter exactly once in practice" (or similar) and keep the rest of the explanation about why referencing props in a reactive context is correct; the fix targets the comment surrounding the $derived declaration (no code changes required).src/gui/ChoiceBuilder/components/CaptureTargetSetting.svelte (1)
38-45: ⚖️ Poor tradeoffDeduplication pattern is correct but may scale poorly for very large vaults.
Lines 38-45 spread all folder/markdown/canvas paths and format syntax into a new Set, then convert to an array. This works correctly but creates intermediate arrays that could be large in vaults with thousands of files. If performance becomes an issue, consider building the Set iteratively. For typical vault sizes, this is fine.
🤖 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/components/CaptureTargetSetting.svelte` around lines 38 - 45, The current return builds a Set by spreading folderPaths, markdownPaths, canvasPaths and FILE_NAME_FORMAT_SYNTAX into an array then de-duplicating, which allocates large intermediate arrays; instead, create the Set once and add each source iteratively (e.g., const result = new Set(); for (const p of folderPaths) result.add(p); for (const p of markdownPaths) result.add(p); for (const p of canvasPaths) result.add(p); for (const s of FILE_NAME_FORMAT_SYNTAX) result.add(s); return Array.from(result);) so you avoid the big temporary combined array while preserving the same deduplication behavior for folderPaths, markdownPaths, canvasPaths and FILE_NAME_FORMAT_SYNTAX.
🤖 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/ChoiceBuilder/components/CanvasNodePicker.svelte`:
- Around line 95-100: The message shown when selectedOption is missing is
misleading; in CanvasNodePicker update the handling around selectedOption (the
nodeOptions.find(...) that compares option.id to activeSelectionNodeId) to show
an accurate Notice when phase === "ready" and the activeSelectionNodeId is not
present (e.g., "The selected node was not found in the loaded canvas.") instead
of "still loading"; keep the existing loading message only when phase !==
"ready" or when nodeOptions are truly empty, and ensure you reference
selectedOption, nodeOptions, activeSelectionNodeId and phase in the conditional
logic so the correct message is displayed in each case.
In `@src/gui/components/SettingItem.svelte`:
- Around line 37-38: The component currently renders both {`@render` control?.()}
and {`@render` children?.()} which can duplicate content; change the markup so
only one is rendered—preferably render {`@render` control?.()} when control is
provided and otherwise render {`@render` children?.()} (i.e., use a conditional
like control ? {`@render` control?.()} : {`@render` children?.()}) and update the
component comment/docs to state that control takes precedence if both are
supplied.
---
Nitpick comments:
In `@src/gui/ChoiceBuilder/components/CaptureTargetSetting.svelte`:
- Around line 38-45: The current return builds a Set by spreading folderPaths,
markdownPaths, canvasPaths and FILE_NAME_FORMAT_SYNTAX into an array then
de-duplicating, which allocates large intermediate arrays; instead, create the
Set once and add each source iteratively (e.g., const result = new Set(); for
(const p of folderPaths) result.add(p); for (const p of markdownPaths)
result.add(p); for (const p of canvasPaths) result.add(p); for (const s of
FILE_NAME_FORMAT_SYNTAX) result.add(s); return Array.from(result);) so you avoid
the big temporary combined array while preserving the same deduplication
behavior for folderPaths, markdownPaths, canvasPaths and
FILE_NAME_FORMAT_SYNTAX.
In `@src/gui/ChoiceBuilder/components/FormatPreviewField.svelte`:
- Around line 28-31: Update the in-code comment near the $derived usage in
FormatPreviewField.svelte to avoid implying $derived never re-runs: change the
phrasing to state that "app/plugin/formatterKind are stable for this field's
lifetime, so $derived computes the formatter exactly once in practice" (or
similar) and keep the rest of the explanation about why referencing props in a
reactive context is correct; the fix targets the comment surrounding the
$derived declaration (no code changes required).
In `@src/gui/ChoiceBuilder/components/ValidatedInput.test.ts`:
- Around line 15-16: The test in
src/gui/ChoiceBuilder/components/ValidatedInput.test.ts is calling await tick()
twice at multiple wait points (in the blocks around the first/second and later
waits); remove the duplicate calls so each wait point uses a single await tick()
(e.g., change occurrences where there are two consecutive await tick() calls to
a single await tick()), keeping test semantics the same and running the
assertions after the single tick.
- Around line 47-62: Update the "keeps only the latest async validation result
(staleness guard)" test in ValidatedInput.test.ts to assert that the
intermediate stale validation result is never shown: after calling
fireEvent.input(input) with "bad" (and awaiting the minimal ticks/promises
needed for the async validator to run), read the hint
(container.querySelector(".qa-field-hint")) and assert it does not equal
"Invalid value" (or is empty) before proceeding to set "good" and asserting the
final state; this ensures the validator/ValidatedInput component's staleness
guard drops the intermediate result.
🪄 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: 8f15251c-22a2-49d0-b5aa-bdc0a008f6e1
📒 Files selected for processing (33)
src/gui/ChoiceBuilder/CaptureChoiceForm.sveltesrc/gui/ChoiceBuilder/CaptureChoiceForm.test.tssrc/gui/ChoiceBuilder/TemplateChoiceForm.sveltesrc/gui/ChoiceBuilder/TemplateChoiceForm.test.tssrc/gui/ChoiceBuilder/canvasNodes.tssrc/gui/ChoiceBuilder/captureChoiceBuilder.tssrc/gui/ChoiceBuilder/captureChoiceFormProps.svelte.tssrc/gui/ChoiceBuilder/choiceBuilder.tssrc/gui/ChoiceBuilder/components/AppendLinkSetting.sveltesrc/gui/ChoiceBuilder/components/AppendLinkSetting.test.tssrc/gui/ChoiceBuilder/components/CanvasNodePicker.sveltesrc/gui/ChoiceBuilder/components/CaptureTargetSetting.sveltesrc/gui/ChoiceBuilder/components/ChoiceNameHeader.sveltesrc/gui/ChoiceBuilder/components/FileOpeningSetting.sveltesrc/gui/ChoiceBuilder/components/FileOpeningSetting.test.tssrc/gui/ChoiceBuilder/components/FormatPreviewField.sveltesrc/gui/ChoiceBuilder/components/InsertAfterFields.sveltesrc/gui/ChoiceBuilder/components/InsertBeforeFields.sveltesrc/gui/ChoiceBuilder/components/OnePageOverrideSetting.sveltesrc/gui/ChoiceBuilder/components/OpenFileSetting.sveltesrc/gui/ChoiceBuilder/components/ValidatedInput.sveltesrc/gui/ChoiceBuilder/components/ValidatedInput.test.tssrc/gui/ChoiceBuilder/components/WritePositionSetting.sveltesrc/gui/ChoiceBuilder/components/suggesterAction.tssrc/gui/ChoiceBuilder/templateChoiceBuilder.tssrc/gui/ChoiceBuilder/templateChoiceFormProps.svelte.tssrc/gui/ChoiceBuilder/templateChoicePersistence.test.tssrc/gui/components/Dropdown.sveltesrc/gui/components/SettingItem.sveltesrc/gui/components/SettingItem.test.tssrc/gui/components/Toggle.sveltesrc/gui/components/Toggle.test.tstests/obsidian-stub.ts
- Normalize fileOpening at FileOpeningSetting init (Codex P2): an imported/legacy choice with openFile:false and no fileOpening object threw on toggling Open (FileOpeningSetting dereferenced .location/.mode). Now normalized at the point the imperative addFileOpeningSetting did. Regression test added. - SettingItem renders (control ?? children) instead of both (CodeRabbit) — avoids duplicate content if both slots are ever passed. - CanvasNodePicker shows an accurate 'selected node not found' message once nodes are loaded instead of 'still loading' (CodeRabbit). Gates green: tsc, eslint, svelte-check 0/0, vitest.
Owner
Author
|
Addressed review feedback in 841c7f6:
Gates green: tsc, eslint, svelte-check 0/0, vitest 1884. |
…l-refresh-edit-pos
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.
Fixes #1130.
Problem
The Capture/Template choice-builder modals fully refreshed (
contentEl.empty(); display()) on every toggle/dropdown change, resetting scroll position and dropping input focus/caret. Reported still-broken in 2.12.0.A prior attempt (#1132–#1136, an
xstatereload machine that snapshotted/restored scroll+focus) was reverted in #1137 — we didn't want that architecture. The Svelte 5 migration (#1248) shipped themountComponentseam but left these imperativenew Setting()builders as backlog.Approach
Convert the ChoiceBuilder trio (
choiceBuilder.tsbase +captureChoiceBuilder.ts+templateChoiceBuilder.ts) from imperative ObsidianSettingbuilders into Svelte 5 runes components mounted through the existingmountComponentseam. Conditional rows become reactive{#if}blocks, soreload()is eliminated — the modal never tears down, so scroll and caret are preserved by construction. Noxstate, no scroll-restore band-aid, no new deps.This is the first concrete slice of the imperative-GUI → Svelte migration; the shared primitives are reusable for the remaining modals.
What changed (5 commits, each independently green)
SettingItem/Toggle/Dropdownemit Obsidian-exact.setting-item/.checkbox-container/.dropdownmarkup (zero new CSS, native theming);ValidatedInput(port ofvalidatedInput.tsincl. thevalidateTokenstaleness guard);FormatPreviewField(un-debounced, parity);AppendLinkSetting(collapses the duplicated capture/template logic),FileOpeningSetting,OnePageOverrideSetting,OpenFileSetting,ChoiceNameHeader; ause:suggester action.TemplateChoiceForm.svelte.CaptureChoiceForm.svelte+ section components (CaptureTargetSetting+ keyedCanvasNodePicker,WritePositionSetting,InsertAfter/BeforeFields); pure canvas helpers extracted verbatim tocanvasNodes.ts.reload()+ alladd*helpers + the deadaddFileSearchInputToSetting).Persistence boundary (subtle, load-bearing)
A Svelte
$stateproxy does not write through to the original object (proxy.js), soChoiceBuilder.onClose()resolvessnapshot(getResultChoice()), and converted builders overridegetResultChoice()to return the form's proxy (this.formProps.choice) — notthis.choice. Snapshotting the original would silently drop every edit. Callers already spread the result, so resolving a plain snapshot is safe.Adversarial review (Codex) found + fixed two real issues
new CaptureChoice()fromcreateChoice()), and$state()does not deeply proxy class instances (onlyObject/Arrayprototypes). So in the add-new flow, nested toggles wouldn't reveal conditional rows (existing choices load as plain JSON, which is why it hid). Fix:structuredClonethe choice in the form-props factories. Regression test uses a realCaptureChoiceinstance and fails without the clone.InsertBeforeFieldsdidn't defaultcreateIfNotFound/createIfNotFoundLocation(parity gap vsInsertAfterFields), risking a persistedundefinedthat throws in the capture formatter. Fixed by defaulting at init.Verification
tsc,eslint,svelte-check0/0,vitest1835 pass (incl. new component + persistence + class-instance regression tests), production esbuild build clean.reload()'d) → scrollTop stayed 556, focus + caret intact, the dependent section appeared reactively, no errors.data.json(proves the proxy-snapshot path).🤖 Generated with Claude Code
Summary by CodeRabbit