refactor: migrate to Svelte 5#1248
Merged
Merged
Conversation
…ponent-test infra - svelte ^4.2.20 -> ^5; svelte-check ^3.8.6 -> ^4 - add @sveltejs/vite-plugin-svelte ^6 (vite 7 compat; NOT ^5/^7), @testing-library/svelte+jest-dom+user-event, eslint-plugin-svelte + svelte-eslint-parser - tsconfig: skipLibCheck:true so svelte-check matches the build's tsc -skipLibCheck (third-party .d.ts only; our source still fully checked) - vitest: svelte() + svelteTesting() plugins + jest-dom setup - obsidian-stub: add module-level setIcon (emits <svg data-icon>) + prepareFuzzySearch - smoke gate: ObsidianIcon renders via the new pipeline; svelte-dnd-action exports resolve All components remain legacy (Svelte 4 syntax) and compile under Svelte 5 legacy mode.
- src/gui/svelte/mountComponent.ts: mount()/unmount() wrapper with an idempotent typed MountHandle (replaces new Component()/.$destroy() across hosts) - src/gui/shared/dndReorder.ts: stripShadow + replaceById pure helpers (one tested home for the SHADOW_PLACEHOLDER filtering + the immutable item replace) - pure tests: dndReorder (8) + mountComponent mount/teardown/idempotency (3) choiceListActions.ts deferred to slice 5 (signatures depend on the cluster rewrite).
- export let -> $props(); onMount + reactive $: -> a single $effect (setIcon is a DOM side-effect, so $effect not $derived); iconEl as $state for bind:this - ObsidianIcon.test.ts: mount render, size default, reactive icon swap, clean unmount Rescope vs blueprint: only ObsidianIcon is a truly standalone leaf (no events, props passed one-way), so it converts safely while parents stay legacy. FolderList moves to the templateChoiceBuilder host slice (updateFolders bridge); ChoiceItemRightButtons moves to the choiceList cluster slice (consumed via component events).
…o runes Components (createEventDispatcher/on: -> callback props + on*; bind:-read props -> one-way): - CommandList: commands=$bindable $state; onconsider/onfinalize via shared stripShadow; updateCommand via replaceById; $state.snapshot at the saveCommands boundary; DELETE exported updateCommandList bridge + createEventDispatcher - 7 command children: $props + onDeleteCommand/onConfigure* callbacks; ConditionalCommand $: -> $derived; WaitCommand local $state(untrack) time mirror + onUpdateCommand (explicit persistence, replacing shared-ref mutation) Host (CommandSequenceEditor.ts): - new CommandList()/.$destroy()/.$on() -> mountComponent + createCommandListProps ($state props bag) + callback props; addCommand/emitCommandsChanged now immutable (callers track via onCommandsChange, verified in MacroBuilder + ConditionalBranchEditorModal) Shared: commandListProps.svelte.ts ($state bag replaces the updateCommandList bridge) Tests: macroCommands (delete/time-update/conditional routing) + CommandList (no-vanish + host-push bridge) = 5; obsidian-dataview mocked per repo convention. Verified: svelte-check 0 errors, build green, full suite 1423 pass. PENDING MANUAL QA (jsdom can't drive pointer drag): actual command drag-reorder in the dev vault (#1244 guard) — tracked for final QA.
All 6 components (ChoiceView, ChoiceList, ChoiceListItem, MultiChoiceListItem, ChoiceItemRightButtons, AddChoiceBox): - createEventDispatcher + bare on: forwarding -> one shared ChoiceListActions callback bag (choiceListActions.ts), threaded by reference through the recursion (flattened payloads); MultiChoiceListItem builds nestedActions overriding onReorderChoices to write choice.choices then bubble [...roots] (calls parent's onReorderChoices, no loop) - ChoiceView: $bindable $state choices buffer; one-shot settingsStore subscribe in $effect; $state.snapshot at every saveChoices boundary; AI-button getState read kept non-reactive (behavior preserved); on:->on* ; flattened handler signatures - ChoiceList: choices=$bindable $state; stripShadow at consider/finalize/each; UNCONDITIONAL finalize re-disable preserved (NOT the macro POINTER gate); forceDragDisabled early-return - DELETED the dead bind:multiChoice landmine; REMOVED the as-any recursion casts (cycle now type-resolves); $: -> $effect for renderChoiceName; fixed self-closing span - host: quickAddSettingsTab mounts ChoiceView via mountComponent (GlobalVariablesView stays legacy new() until slice 6) Tests: ChoiceList.callbacks (flat + nested recursion wiring) + persistenceBoundary (snapshot is plain + non-aliasing). Full ChoiceView render not unit-testable (pre-existing circular imports in its graph — bundler-only); covered by manual QA. Verified: svelte-check 0 errors/0 warnings, build green, full suite 1427 pass. PENDING MANUAL QA: nested drag reorder, collapse toggle, choice CRUD + data.json persistence.
Components -> runes:
- GlobalVariablesView: $props + items $state; onMount/onDestroy -> $effect with cleanup;
preserved bind:value={it.name}/{it.value} + use:attachSuggester (source-guarded by test)
- ExportPackageModal: $props + $state; 5 pure $: -> $derived (incl. let+$: rootChoiceIds merge)
- ImportPackageModal: $props + $state (incl. reactive Maps); preserved {conflict.name}/{originalPath}
- FolderList: $props; DELETED exported updateFolders bridge -> folderListProps.svelte.ts $state bag
Hosts -> mountComponent + handle.destroy():
- ExportPackageModal.ts / ImportPackageModal.ts (onOpen/onClose)
- templateChoiceBuilder.addFolderSelector (folderListProps $state bag replaces updateFolders)
- choiceBuilder base: svelteElements SvelteComponent[] -> MountHandle[]; destroySvelteElements()
in onClose AND at top of reload() (fixes the verified effect leak)
- quickAddSettingsTab: GlobalVariablesView via mountComponent
All 19 .svelte are runes; 0 export let / createEventDispatcher / on: directives remain;
remaining new X({...}) are plain TS classes (CommandSequenceEditor/ConditionalBranchEditorModal/
InputPromptDraftHandler), not Svelte.
Verified: svelte-check 0/0, build green, full suite 1427 pass.
PENDING MANUAL QA: package import/export, global vars inline edit + debounce, folder add/remove +
builder reload re-render, settings open/close mount-leak.
…eanup
- eslint.config.mjs: add svelte flat/recommended + svelte-eslint-parser (TS sub-parser)
for **/*.svelte + **/*.svelte.ts; disable prefer-svelte-reactivity (collections use
immutable reassignment, which is reactive — SvelteSet/Map only needed for mutation)
- fix all surfaced lint: {#each} keys (FolderList/GlobalVariables/Export/Import),
useless string mustaches in AddChoiceBox (auto-fixed), drop unused suggester var +
dangling eslint-disable directive in GlobalVariablesView
Final state: 19/19 components on runes ($props); 0 createEventDispatcher / svelte/legacy /
SvelteComponent / componentApi compat in code; all hosts use mountComponent.
Full CI gate set GREEN: svelte-check 4 (0 errors/0 warnings), build-with-lint (tsc + eslint
incl. svelte + esbuild), test (1427 passed / 31 skipped).
…condition) QA surfaced that adding commands to a Conditional command's Then/Else branch didn't persist. Root cause: the host's commandListProps is $state, so the rendered command is a PROXY; the branch modal mutates command.thenCommands on that proxy, which does NOT write through to the host's commandsRef (Svelte 5: 'updating proxy properties does not mutate the original'). Every other edit persists via saveCommands($state.snapshot(...)), which reads the proxy's current state — but the conditional handlers uniquely persisted via emitCommandsChanged(commandsRef), passing the stale, un-mutated array. Fix: route the conditional handlers through the same snapshot path. CommandList now awaits the handler (which mutates + returns whether changed) and calls updateCommand -> saveCommands($state.snapshot(commands)). Dropped the host's stale wrapConditionalHandler/ emitCommandsChanged wiring; handlers passed directly. Also fixes the latent configure-condition persistence (same bug). Regression test: CommandList.conditional.test.ts (proxy mutation -> plain snapshot save). Verified: svelte-check 0/0, build-with-lint, full suite 1429 pass.
…stence Drives the live macro editor through obsidian-e2e (configure macro -> Edit then branch -> Add wait command -> Save -> close) and asserts the added command lands in data.json (thenCommands.length === 1). Guards the runes-rewrite fix end-to-end. Verified PASSING against the live dev vault; complements the component-level guard in CommandList.conditional.test.ts (which fails on the pre-fix code).
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughMigrates UI to Svelte 5 runes ($props/$state/$effect), replaces component event dispatch with typed callback-prop contracts, adds mountComponent/MountHandle for imperative mounting, introduces immutable DnD and snapshot helpers, upgrades Svelte tooling, and expands unit, smoke, and E2E tests. ChangesSvelte 5 Migration & Callback-Props Refactor
Estimated code review effort: Possibly related issues:
Possibly related PRs:
Suggested labels:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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: 3bd4a5aa13
ℹ️ 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".
Deploying quickadd with
|
| Latest commit: |
7773f15
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7763a301.quickadd.pages.dev |
| Branch Preview URL: | https://feat-svelte5-rewrite.quickadd.pages.dev |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![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 (3)
src/gui/components/ObsidianIcon.svelte (1)
2-20: ⚡ Quick winUse tabs instead of spaces in this Svelte file block.
This section is indented with spaces, but this path is configured for tab indentation; please reformat to tabs to match repo/editorconfig expectations.
As per coding guidelines: "
src/**/*.{ts,tsx,svelte,css,js}: Use tab indentation and LF endings (see.editorconfig); align editor settings".🤖 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/components/ObsidianIcon.svelte` around lines 2 - 20, Reformat this Svelte component to use tabs for indentation (instead of spaces) to comply with the repo .editorconfig: replace leading space indentation in the file (including the import line and the block that defines iconId, size, iconEl, and the $effect) with tabs, preserving existing code and line endings (LF); ensure the blocks referencing setIcon, iconEl, iconId, size, and the $effect remain unchanged except for indentation so the component behavior is identical after reformatting.src/gui/MacroGUIs/Components/AIAssistantCommand.svelte (1)
5-17: ⚡ Quick winConsider a shared base type for the repeated child-prop contract.
startDrag,dragDisabled, andonDeleteCommandare re-declared identically across all seven command-child components. Extracting a small shared interface keeps the contract from drifting if any signature changes.♻️ Example shared base type
// e.g. src/gui/MacroGUIs/commandChildProps.ts import type { ICommand } from "../../types/macros/ICommand"; export interface CommandChildProps<T extends ICommand = ICommand> { command: T; startDrag: (e: MouseEvent | TouchEvent) => void; dragDisabled: boolean; onDeleteCommand: (commandId: string) => void; }- }: { - command: IAIAssistantCommand; - startDrag: (e: MouseEvent | TouchEvent) => void; - dragDisabled: boolean; - onDeleteCommand: (commandId: string) => void; - onConfigureAssistant: (command: IAIAssistantCommand) => void; - } = $props(); + }: CommandChildProps<IAIAssistantCommand> & { + onConfigureAssistant: (command: IAIAssistantCommand) => void; + } = $props();🤖 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/MacroGUIs/Components/AIAssistantCommand.svelte` around lines 5 - 17, Extract the repeated child-prop contract into a shared interface and use it in AIAssistantCommand.svelte: create a new interface (e.g., CommandChildProps) that includes startDrag, dragDisabled, and onDeleteCommand, then update the component's prop typing to extend or use CommandChildProps with IAIAssistantCommand (replace the inline type block currently declaring command, startDrag, dragDisabled, onDeleteCommand in AIAssistantCommand.svelte); ensure other sibling command-child components are updated to import and use the same CommandChildProps so the signatures (startDrag, dragDisabled, onDeleteCommand) remain consistent across all seven components.src/gui/ChoiceBuilder/templateChoiceBuilder.ts (1)
277-291: 💤 Low valueOptional: align mutation style for
choice.folder.folders.
addFoldermutates the backing array in place viapush(), whiledeleteFolder(Line 246) reassigns a new filtered array. Both paths then re-syncfolderListProps.foldersand the suggester, so behavior is correct — this is purely for consistency with the immutable reassignment pattern used elsewhere in the migration.♻️ Optional: reassign instead of push
- this.choice.folder.folders.push(input); - - folderListProps.folders = [...this.choice.folder.folders]; + this.choice.folder.folders = [...this.choice.folder.folders, input]; + + folderListProps.folders = [...this.choice.folder.folders];🤖 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/templateChoiceBuilder.ts` around lines 277 - 291, addFolder currently mutates the backing array with this.choice.folder.folders.push(input) while deleteFolder reassigns a filtered array; make addFolder consistent by creating a new array and assigning it to this.choice.folder.folders (e.g., this.choice.folder.folders = [...this.choice.folder.folders, input]), then update folderListProps.folders and call suggester.updateCurrentItems as before; modify the addFolder function to perform immutable reassignment while keeping folderListProps.folders and folderInput.inputEl.value updates intact.
🤖 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/MultiChoiceListItem.svelte`:
- Around line 33-39: The Component instance cmp created in
MultiChoiceListItem.svelte is not unloaded on destroy; ensure you either
register an onDestroy cleanup to call cmp.unload() (e.g., onDestroy(() =>
cmp.unload())) or verify and document that renderChoiceName(choice.name,
nameElement, cmp, app) does not attach resources needing disposal; add the
onDestroy hook if prior code in ChoiceListItem used cmp.unload(), and keep the
$effect block using nameElement and renderChoiceName unchanged.
In `@src/gui/components/svelte5-pipeline.smoke.test.ts`:
- Line 40: Update the stale comment that references the removed legacy flow
"onMount -> updateIcon" to accurately describe the current runtime path (e.g.,
"$effect -> setIcon(el, 'trash')" and that the stub appends "<svg data-icon>").
Locate the comment near the test in svelte5-pipeline.smoke.test.ts and replace
the outdated sequence with a short note describing the actual "$effect ->
setIcon(...)" behavior so future readers understand the current flow.
---
Nitpick comments:
In `@src/gui/ChoiceBuilder/templateChoiceBuilder.ts`:
- Around line 277-291: addFolder currently mutates the backing array with
this.choice.folder.folders.push(input) while deleteFolder reassigns a filtered
array; make addFolder consistent by creating a new array and assigning it to
this.choice.folder.folders (e.g., this.choice.folder.folders =
[...this.choice.folder.folders, input]), then update folderListProps.folders and
call suggester.updateCurrentItems as before; modify the addFolder function to
perform immutable reassignment while keeping folderListProps.folders and
folderInput.inputEl.value updates intact.
In `@src/gui/components/ObsidianIcon.svelte`:
- Around line 2-20: Reformat this Svelte component to use tabs for indentation
(instead of spaces) to comply with the repo .editorconfig: replace leading space
indentation in the file (including the import line and the block that defines
iconId, size, iconEl, and the $effect) with tabs, preserving existing code and
line endings (LF); ensure the blocks referencing setIcon, iconEl, iconId, size,
and the $effect remain unchanged except for indentation so the component
behavior is identical after reformatting.
In `@src/gui/MacroGUIs/Components/AIAssistantCommand.svelte`:
- Around line 5-17: Extract the repeated child-prop contract into a shared
interface and use it in AIAssistantCommand.svelte: create a new interface (e.g.,
CommandChildProps) that includes startDrag, dragDisabled, and onDeleteCommand,
then update the component's prop typing to extend or use CommandChildProps with
IAIAssistantCommand (replace the inline type block currently declaring command,
startDrag, dragDisabled, onDeleteCommand in AIAssistantCommand.svelte); ensure
other sibling command-child components are updated to import and use the same
CommandChildProps so the signatures (startDrag, dragDisabled, onDeleteCommand)
remain consistent across all seven components.
🪄 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: 56de64d4-feb2-40d9-8b32-8ffc0f6c8d07
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
eslint.config.mjspackage.jsonsrc/gui/ChoiceBuilder/FolderList.sveltesrc/gui/ChoiceBuilder/choiceBuilder.tssrc/gui/ChoiceBuilder/folderListProps.svelte.tssrc/gui/ChoiceBuilder/templateChoiceBuilder.tssrc/gui/GlobalVariables/GlobalVariablesView.sveltesrc/gui/MacroGUIs/CommandList.conditional.test.tssrc/gui/MacroGUIs/CommandList.reorder.test.tssrc/gui/MacroGUIs/CommandList.sveltesrc/gui/MacroGUIs/CommandSequenceEditor.tssrc/gui/MacroGUIs/Components/AIAssistantCommand.sveltesrc/gui/MacroGUIs/Components/ConditionalCommand.sveltesrc/gui/MacroGUIs/Components/NestedChoiceCommand.sveltesrc/gui/MacroGUIs/Components/OpenFileCommand.sveltesrc/gui/MacroGUIs/Components/StandardCommand.sveltesrc/gui/MacroGUIs/Components/UserScriptCommand.sveltesrc/gui/MacroGUIs/Components/WaitCommand.sveltesrc/gui/MacroGUIs/Components/macroCommands.test.tssrc/gui/MacroGUIs/commandListProps.svelte.tssrc/gui/PackageManager/ExportPackageModal.sveltesrc/gui/PackageManager/ExportPackageModal.tssrc/gui/PackageManager/ImportPackageModal.sveltesrc/gui/PackageManager/ImportPackageModal.tssrc/gui/choiceList/AddChoiceBox.sveltesrc/gui/choiceList/ChoiceItemRightButtons.sveltesrc/gui/choiceList/ChoiceList.callbacks.test.tssrc/gui/choiceList/ChoiceList.sveltesrc/gui/choiceList/ChoiceListItem.sveltesrc/gui/choiceList/ChoiceView.sveltesrc/gui/choiceList/MultiChoiceListItem.sveltesrc/gui/choiceList/choiceListActions.tssrc/gui/choiceList/persistenceBoundary.svelte.tssrc/gui/choiceList/persistenceBoundary.test.tssrc/gui/components/ObsidianIcon.sveltesrc/gui/components/ObsidianIcon.test.tssrc/gui/components/svelte5-pipeline.smoke.test.tssrc/gui/shared/dndReorder.test.tssrc/gui/shared/dndReorder.tssrc/gui/svelte/mountComponent.test.tssrc/gui/svelte/mountComponent.tssrc/quickAddSettingsTab.tstests/e2e/conditional-branch-persistence.test.tstests/obsidian-stub.tstests/vitest-setup.tstsconfig.jsonvitest.config.mts
…unes conversion Addresses CodeRabbit review on PR #1248: ObsidianIcon now uses $effect (no onMount/ updateIcon), so the smoke test's flow comment (line 40) and the file docstring (calling ObsidianIcon a 'legacy' component) were stale. Comment-only; tests unchanged.
vitePreprocess config consumed by svelte-check, vite-plugin-svelte (tests) and eslint-plugin-svelte; silences the 'no Svelte config found' notice. The production build is unaffected (esbuild-svelte reads its own options from esbuild.config.mjs). Verified: svelte-check 0/0, build green, full suite 1429, lint green.
…Choice guard removeChoiceHelper/updateChoiceHelper/filterChoices now narrow via isMultiChoice(c): c is IMultiChoice instead of (value as any).choices. Multi-node clones build a typed IMultiChoice local (no cast). Verified: svelte-check 0/0, choiceList tests pass, build green.
…ence boundary Adds Plain<T> branded type + snapshot() helper (persist.svelte.ts). The saveChoices / saveCommands sinks now accept only Plain<...>, so handing a live $state proxy to persistence is a svelte-check ERROR rather than a silent data-loss bug (the class behind the conditional Then/Else regression). Verified the brand bites (raw proxy -> 'Property [PLAIN] is missing') and that all gates stay green: svelte-check 0/0, build, full suite.
ChoiceListItem + MultiChoiceListItem pass cmp (new Component()) to MarkdownRenderer.render as the lifecycle owner but never unloaded it, leaking any registered child components (pre-existing on master; addresses CodeRabbit review on #1248). Added a no-dependency $effect whose teardown calls cmp.unload() — disposes on destroy only, preserving visible behavior. Test: cmpLifecycle.test.ts asserts unload() fires on unmount for both rows. Verified: svelte-check 0/0, build green, full suite.
This was referenced May 29, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/gui/choiceList/cmpLifecycle.test.ts (1)
26-39: ⚡ Quick winCapture the baseline right before
unmount()to actually assert teardown.
beforeis read from a fresh spy (always0), so the assertion only provesunloadwas called sometime during render or destroy. Capturing it just beforeunmount()makes the test specifically verify the destroy-time unload, which is the regression this file guards.♻️ Move the baseline capture below render
const spy = vi.spyOn(Component.prototype, "unload"); - const before = spy.mock.calls.length; const choice = { id: "a", name: "Alpha", type: "Template", command: false } as unknown as IChoice; const { unmount } = render(ChoiceListItem, { props: { choice, app: new App() as never, roots: [choice], dragDisabled: true, startDrag: noop, actions: actions() }, }); + const before = spy.mock.calls.length; unmount(); flushSync();Same applies to the
MultiChoiceListItemtest (Lines 41-56).🤖 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/cmpLifecycle.test.ts` around lines 26 - 39, Move the baseline capture of the spy's call count to just before calling unmount so the test asserts unload was invoked during teardown: after rendering ChoiceListItem (render(ChoiceListItem, ...)) but before unmount() read spy.mock.calls.length into the variable currently named before and then call unmount(), flushSync(), and assert the call count increased; apply the same change to the MultiChoiceListItem test — both reference Component.prototype.unload, the spy variable, the before variable, render(...), and unmount().
🤖 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.
Nitpick comments:
In `@src/gui/choiceList/cmpLifecycle.test.ts`:
- Around line 26-39: Move the baseline capture of the spy's call count to just
before calling unmount so the test asserts unload was invoked during teardown:
after rendering ChoiceListItem (render(ChoiceListItem, ...)) but before
unmount() read spy.mock.calls.length into the variable currently named before
and then call unmount(), flushSync(), and assert the call count increased; apply
the same change to the MultiChoiceListItem test — both reference
Component.prototype.unload, the spy variable, the before variable, render(...),
and unmount().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3da599e-2ea1-40c7-b1c9-3681beaa05da
📒 Files selected for processing (8)
src/gui/MacroGUIs/CommandList.sveltesrc/gui/MacroGUIs/commandListProps.svelte.tssrc/gui/choiceList/ChoiceListItem.sveltesrc/gui/choiceList/ChoiceView.sveltesrc/gui/choiceList/MultiChoiceListItem.sveltesrc/gui/choiceList/cmpLifecycle.test.tssrc/gui/svelte/persist.svelte.tssvelte.config.mjs
✅ Files skipped from review due to trivial changes (1)
- svelte.config.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/gui/MacroGUIs/commandListProps.svelte.ts
- src/gui/choiceList/ChoiceListItem.svelte
- src/gui/choiceList/MultiChoiceListItem.svelte
…ks.lock) The ephemeral scheduler lock (sessionId/pid) was accidentally swept into c79fb5b by a git add -A. Untrack it and gitignore .claude/scheduled_tasks.{lock,json} (machine-local runtime state). File kept on disk.
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 #1239.
Full, idiomatic Svelte 4 → 5 rewrite of the plugin UI — not a mechanical
sv migrateport. All 19 components are converted to runes, every imperative host usesmount()/unmount(), and behavior/UX is preserved (no UI redesign). Landed as ordered, individually-green commits so it can be reviewed slice by slice.What changed
svelte→5,svelte-check→4,@sveltejs/vite-plugin-svelte@^6 (vite 7),@testing-library/svelte,eslint-plugin-svelte; vitest svelte pipeline +obsidian-stubsetIconmountComponent()(mount/unmount + idempotent handle),dndReorder(stripShadow/replaceById) + pure tests$props+ a single$effectfor thesetIconDOM writeCommandSequenceEditorhost; deleted theupdateCommandListbridge → host-owned$stateprops bagChoiceListActionscallback bag; deleted the deadbind:multiChoice; removedas anyrecursion casts.tshosts onmountComponent; fixed aChoiceBuilder.reload()effect leakeslint-plugin-svelte; fixed{#each}keys / mustaches; final residue sweepKey architecture decisions
settingsStore.ts) stays the single source of truth (it's consumed by ~30 non-Svelte files + test mocks). Components read it via a thin runes boundary;$state.snapshot()is applied at every reactive→plain persistence boundary so no proxy leaks intodata.json.createEventDispatcher+ 147on:directives → on-prefixed callback props with flattened payloads. The recursive choiceList threads one sharedChoiceListActionsbag.mountComponent()helper replacesnew Component()/.$destroy()/.$on()everywhere; the two hidden exported-function bridges (updateCommandList,updateFolders) are gone in favor of host-owned$state.dndReorder.ts; ChoiceList's unconditional finalize re-disable is preserved (distinct from CommandList's POINTER-gated one).The one regression found in QA — fixed
Adding commands to a Conditional command's Then/Else branch didn't persist. Root cause: a Svelte 5 rule —
$stateproxy mutations don't write through to the original object. Every other edit persisted viasaveCommands($state.snapshot(commands))(reads the proxy's live state), but the conditional handlers uniquely persisted via the host's stalecommandsRef. Fix: route them through the same snapshot path. (Also fixed a latent identical bug in configure-condition.)Verification
svelte-check(4): 0 errors / 0 warningsbuild-with-lint(tsc + eslint incl. svelte + esbuild): green@testing-library/svelte)obsidian-e2e,tests/e2e/conditional-branch-persistence.test.ts): drives the live macro editor (configure → Edit then branch → Add wait → Save → close) and asserts the command lands indata.json. Passes.Manual dev-vault QA (maintainer): passed — flat + nested choice drag-reorder (no vanish/ghost-gap, persists on reload), every macro command type incl. drag-reorder, WaitCommand time edit, conditional branches (after fix), choice CRUD, packages, folder add/remove, settings open/close (no mount leak), no
Proxyartifacts indata.json, no console errors.Notes for reviewers
refactor:(no semantic-release version bump) since this is behavior-preserving modernization — switch tofeat:if you'd prefer a minor release.createEventDispatcher/svelte/legacy/SvelteComponent/componentApicompat remain in code.check+build+test).Summary by CodeRabbit