fix: a malformed data.json no longer costs you the choices QuickAdd could not draw - #1629
Open
chhoumann wants to merge 8 commits into
Open
fix: a malformed data.json no longer costs you the choices QuickAdd could not draw#1629chhoumann wants to merge 8 commits into
chhoumann wants to merge 8 commits into
Conversation
… this"
`hasUnreadableChildren` documented itself as true only for a value that could
still be CARRYING choices, and then returned true for `""`, `0` and `false` -
which are as empty as the `null` and `{}` it correctly returns false for.
Four surfaces read that predicate, so a folder whose `choices` was `""` showed
"QuickAdd couldn't read this folder's contents", lost its drop target, and got
the scarier delete confirmation - for a value holding nothing at all.
The rule the doc describes is already implemented one type over, by
`isUnreadableCommandList` (#1593). Rather than a shared module, this splits out
the value-level sibling `isUnreadableChoiceList` and keeps the two as the pair
the repo already uses for this (`isChoiceLike`/`isCommandLike`,
`hasChildChoices`/`hasCommandList`), with a table-driven test over the union of
both shape lists as the ratchet that they answer identically. That test is what
makes the "these surfaces cannot disagree" claim true rather than aspirational.
The `""` / `0` / `false` rows join both shared shape lists, so every resilience
test that sweeps the malformed tree now attacks them too.
Closes #1611
A migration runs once and is then flagged complete forever, so one that walks
past a subtree it could not read leaves that subtree un-migrated permanently -
repairing data.json by hand afterwards does not help, because the flag is set.
`treeHasUnreadableChildren` existed to prevent exactly that, and asked the wrong
question in two directions at once:
UNDER-blocking. It looked at `Multi.choices` only. A Capture living inside
`"commands": {"0": {"type": "NestedChoice", ...}}` was invisible to
`walkAllChoices`, and the four migrations that guard reported complete anyway.
`backfillFileOpeningDefaults` and `migrateFileOpeningSettings` had no guard at
all, and both translate the legacy `openFileInNewTab` keys that runtime ignores -
so a hidden choice loses its "open in new tab" preference for good.
OVER-blocking. It returned true for ANY non-array `choices`, including
`undefined`, `null` and `{}`, which carry nothing. One folder with no `choices`
key therefore kept `removeMacroIndirection` pending forever: legacy macros never
embedded, never removed, and every legacy macro choice in the vault dead, because
nothing at runtime resolves a `macroId`.
The guard is now derived from the walk it has to agree with - `walkSettings`
reports the containers it stepped over, and `settingsTreeHasUnreadableData` is
that report - so the two cannot drift. `walkChoice` resolves through
`macroCommandsValueOf` like every other consumer, which also closes the
array-valued `macro` hole where `.commands` reads as `undefined`, walks nothing,
and looks perfectly readable.
Each migration asks the question its own traversal can answer. The three that
recurse `Multi.choices` themselves keep the folders-only question; blocking
`removeMacroIndirection` on a `macro.commands` it never descends would be pure
cost for the most expensive kind of pending.
Two things found while doing this and fixed here because the guard is worthless
without them:
- `settings.macros` is as untrusted as the rest of data.json, and
`settings.macros ?? []` passes `{"0": {...}}` through (not nullish). Three
migrations then threw `macros is not iterable` - caught, reverted, and
reported with a 15-second "please create an issue" notice on every launch.
They now read through `rootMacrosOf` and only write back a real array.
- `removeMacroIndirection` holds the only root WRITE among the migrations
(`settings.choices.push`). It gets its own `Array.isArray` guard: a
readability predicate must never be the only thing between a corrupt value and
a TypeError.
The two file-opening migrations no longer call `saveSettings()` themselves.
migrate.ts already saves once after the whole run, and now that these can stay
pending, a per-migration write would be a full data.json rewrite on every launch,
straight into Obsidian Sync's whole-file last-write-wins.
Closes #1610
…t render
ChoiceList filtered the rendered list to entries it could key, and a comment
above that filter said it was a "render-time view only. Nothing here rewrites the
tree." It was not: the same filtered array seeded the drop zone AND was what the
persist path wrote back. So the first drag or ArrowDown committed the filtered
list, and every entry the filter had dropped was gone from data.json.
For a `null` hole that is harmless - it carries nothing. But the filter also
dropped any entry whose id was not a non-empty string, and
{ "name": "Daily note", "type": "Template", "templatePath": "...", "id": 12 }
is a complete, working choice - runnable from the palette, listed by the CLI -
whose id was written as a JSON number by a hand-edit, a script or a merge. It was
invisible in the settings list, so the user could not miss it, and one reorder
deleted it with no prompt and no undo. Verified end to end in a live vault: five
choices in data.json, four rows rendered, one ArrowDown and the fifth is gone
from disk.
Repair rather than hide, mirroring what #1593 shipped for macro commands.
`normalizeChoiceList` runs at the ChoiceView seam: an entry that cannot be keyed
gets a fresh uuid and is KEPT, so it becomes visible, editable and deletable for
the first time; only a hole is dropped. Ids are de-duplicated across the whole
tree, since choice ids are globally unique (the command registry keys on
`choice:<id>`) unlike command ids. Nothing is persisted by this - the repair
reaches disk with the user's first ordinary edit.
A repaired id is always a fresh uuid, never `String(oldId)`. Coercing looks
tempting because it would keep `quickadd:choice:12` alive, and it is wrong twice:
ids are compared with `===` in `getChoice`, so a persisted 12 -> "12" silently
breaks a `ChoiceCommand{choiceId: 12}` (MacroChoiceEngine matches /not found/i
and skips the step), and "that string is free" can only mean "not seen YET"
during a pre-order walk, so it can also steal a healthy later sibling's id. A
stored reference to a malformed id does break here - but the behaviour this
replaces DELETED the choice, which broke the same reference and lost the choice
with it.
The seam is memoized on the raw store value, which is load-bearing rather than an
optimization: this subscription fires on every settingsStore write, including the
AI provider auto-sync a few seconds after launch, and every by-id write in the
view resolves its target BEFORE an await - so re-minting inside that window turns
`oldChoice.id === newChoice.id` into a no-op and silently discards the user's
builder edits.
Two more holes closed on the way:
- ChoiceList's filter now de-duplicates ids, like CommandList's already did. A
duplicate arriving after load reached the keyed {#each} and threw
`each_key_duplicate`, and a POST-mount throw escapes mountComponent's try.
- An Array entry in a choices list is read as a NESTED LIST and spliced in, the
same reading `macroCommandsValueOf` gives an array-valued `macro`.
`isChoiceLike([])` is true, so the alternative was spreading it into one
nameless, typeless row. Mirrored in `normalizeCommandList` so the twins stay
byte-symmetric.
Closes #1608
…d secrets `regenerateIds` re-ided the macro and its TOP-LEVEL commands only, so duplicating a macro left every id below that identical between the original and the copy: a Conditional's thenCommands/elseCommands, a NestedChoice's inner choice, and that choice's own macro if it had one. Nothing crashed, because it is not a within-list collision. But `buildUserScriptSecretId` keys a stored user-script secret on `command.id`, and `migrateUserScriptSecretSettings` re-adopts an existing secret by that id for any option declaring a stable `id` - so a duplicated macro's NESTED UserScript command adopted the ORIGINAL's secret slot, and setting the API key on the copy set it on the original. The nested `IChoice` kept its id too, colliding with the original's in every by-id walk over commands. `regenerateIds` now recurses the whole macro: branch commands, a nested choice (and its macro, or its children if it is a folder). It stays total over whatever data.json holds - a non-array command list is left exactly as found, a hole is stepped over - and resolves the list through `macroCommandsValueOf`, so an array-valued macro is re-ided as the command list it is instead of having a non-index `id` written onto it that JSON.stringify would drop. That last shape turned out to hide the reported symptom from the fix. The secret sanitizers reached `choice.macro.commands` literally behind an `isRecord` gate, and `isRecord` accepts an array - so for `macro: [ ... ]` they walked nothing, the copy kept the original's literal `secretRef`, and editing the copy's key overwrote the original's SecretStorage slot. `stripUserScriptSecretRefsFromChoice`, `clearSecretsFromChoice` and `collectUserScriptPathsFromChoice` now read through `macroCommandsValueOf` like the macro builder does. The regression test asserts the SYMPTOM - the copy carries no secret reference belonging to the original - not just that the ids differ. An ids-only assertion passes while the reported bug still reproduces through the path that never re-ids. Closes #1609
Delete is the one irreversible action in the settings list, and the two container
types were treated asymmetrically. A folder whose `choices` value could not be
read got a specific warning; a macro got only
Deleting this choice will also delete its macro commands.
which is true and says nothing about the case that matters - that `macro.commands`
holds something QuickAdd could not read (`{"0": {...}}`, a string, a number).
Since #1593 the macro builder tells the user about exactly that state and refuses
to overwrite it, and then the one screen that was about to destroy it for good
stayed quiet.
The confirmation now reads the SAME predicate through the SAME accessor the
builder resolves its list with, so the screen that reports the state and the
screen that destroys it cannot disagree. `macroCommandsValueOf` rather than
`macro?.commands` is what makes the unreadable-`macro`-object case resolve here
exactly as it does there - and it is also why an array-valued `macro` correctly
keeps the ordinary line: the array IS the command list, the builder can read and
edit it, so there is nothing to warn about.
Closes #1612
…he macro builder `MacroBuilder.configureConditionalBranch` opens the branch editor OVER the still- open macro builder. Both mount a CommandList, and both registered their drop zone with the same svelte-dnd-action type. The library groups zones by type and hit-tests every zone in the group GEOMETRICALLY - a modal backdrop shields nothing - so the builder's list underneath was a live drop target for a drag inside the branch modal, and the two lists overlap almost exactly. Confirmed live: with the Then-branch editor open over a macro, dragging its one command down past the branch list's own bottom edge - still inside the modal - moved it into the macro builder underneath. Both editors then rendered a list that disagreed with the stored macro: the builder showed a command the macro did not have, and the conditional's summary read "Then: 0" for a branch that still had one. Each CommandList now gets its own zone type, so no two command lists are ever in the same group. QuickAdd offers no cross-list command drag - unlike a folder, a command has no "move into" gesture - so a group of one is the honest model, and leaving the zone is then an ordinary "outside any zone" drag: the command springs back. `dropFromOthersDisabled: true` was the first fix and is worse, which is only visible live. The target does refuse, but the library then runs its "left for a zone that refuses" path, which re-dispatches the origin's items twice - once with its shadow placeholder and once without - and QuickAdd's stripShadow (#1244/#883) drops the placeholder the library goes on to measure. The drag ended in an uncaught `Cannot read properties of undefined (reading 'getBoundingClientRect')` with the command gone from the source list: a worse outcome than the bug. Verified after the fix: the same drag leaves both lists and the stored macro untouched, an ordinary reorder within one list still works, and dev:errors is clean for both. Closes #1613
…ds total
Follow-ups from an adversarial review of the six fixes in this branch. Each of
these was a real defect in them, found by probe rather than by reading.
**The id repair leaked a dead palette entry per settings open.** The memo lived
in ChoiceView, and the settings tab DESTROYS and re-mounts the view every time it
is opened - so an unrepaired `{"id": 12, "command": true}` minted a fresh uuid and
registered another command on every open, with nothing unregistering. It now
lives in a module-level WeakMap keyed on the raw array (`seedChoiceTree`), so the
cache outlives the component and dies with the array. Verified live: three opens,
one stable id, two palette entries (the original id from data.json plus the one
repair) rather than four.
Keying that map on the choice's own previous id would NOT work: `undefined`
collides across every id-less entry, so two siblings would be handed the same
uuid - `each_key_duplicate` again.
The registration loop is also wrapped now. It runs during component setup, so an
uncaught throw escaped mountComponent and would have swapped the whole choice
list for the error card - one failure must cost one command, the same argument
main.ts makes around `addCommandForChoice`.
**A hole in `settings.macros` still crashed two migrations.** `[null, {...}]` IS
an array, so it reads as readable and reaches the loop, where `macro.commands`
throws. Both now read through `commandListOf(macroCommandsValueOf(macro))`, which
steps over the hole AND gives an array-valued legacy macro the same reading every
other consumer gives it.
**The traversal and the editor seam disagreed about a nested array.**
`typeof [] === "object"`, so `walkChoice` handed the array to the visitor and
descended nothing, reporting READABLE - while `normalizeChoiceList` splices such
an entry's members into the tree. A migration could be flagged complete over a
choice the settings tab then made real. The walk now splices too.
**Two tests asserted nothing.** `expect(c.fileOpening ?? c.type).toBeTruthy()`
falls back to the string "Template", so all three migration rows passed with
their visitor bodies deleted; each row now carries its own effect assertion, and
gutting any visitor fails its row. And #1613 shipped with no regression coverage
at all - reverting the zone type left the full suite green. `CommandList.zoneType.test.ts`
wraps svelte-dnd-action's action to record the type each list registers, which is
the only observable seam (the library writes no attribute); it fails with
`expected [ 'command', 'command' ] to not include 'command'` on the reverted tree.
**`commandZoneId.ts` is gone.** Svelte's own `$props.id()` is a runtime-wide uid
counter and is already used in-tree by LabeledField - a module, an export and a
counter for that was 35 lines of nothing.
Also: the direct #1609 symptom is now asserted through `buildUserScriptSecretId`
(the copy's nested script resolves to a different secret slot), the comment
claiming the two legacy-macro migrations' guards match their traversals is
corrected to say they are knowingly narrower and why widening is inert, and the
`normalizeCommandList` array branch gets the coverage its twin already had.
|
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 (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe changes normalize malformed choice and macro trees, preserve repaired identifiers, harden migrations against unreadable settings, update macro duplication and secret traversal, improve deletion warnings, and isolate command-list drag-and-drop zones. ChangesChoice rendering and normalization
Malformed macro data handling
Drag-and-drop zone isolation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: |
08c7201
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bc63a776.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-1608-macro-data-ro.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: 48ed5a1501
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/gui/MacroGUIs/CommandList.zoneType.test.ts (1)
36-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this unit test independent of the real Obsidian runtime.
new App()is unnecessary for this DnD-only regression test and defeats the repository’s testability boundary. ImportAppas a type and pass a minimal stub, or inject an app interface.Suggested adjustment
-import { App } from "obsidian"; +import type { App } from "obsidian"; - app: new App() as never, + app: {} as App,As per coding guidelines, TypeScript code should inject Obsidian dependencies behind interfaces so production logic can be tested without loading real Obsidian modules.
🤖 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/CommandList.zoneType.test.ts` around lines 36 - 54, Update the renderList test helper to avoid constructing the real Obsidian runtime: change App to a type-only import and provide a minimal stub matching the app dependency expected by CommandList. Keep the existing DnD regression-test behavior and other injected props unchanged.Source: Coding guidelines
src/utils/normalizeChoiceList.test.ts (1)
147-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the fixture-driven loop against a vacuous pass.
If
MALFORMED_CHILDREN_SHAPESever loses itslossyshapes (or the flag is renamed), this test passes with zero iterations and silently stops covering#1566/#1608 preservation. A length assertion pins it.♻️ Assert the loop actually ran
it("leaves an unreadable folder's children value exactly as found", () => { - for (const shape of MALFORMED_CHILDREN_SHAPES.filter((s) => s.lossy)) { + const lossy = MALFORMED_CHILDREN_SHAPES.filter((s) => s.lossy); + expect(lossy.length).toBeGreaterThan(0); + for (const shape of lossy) { const broken = malformedFolder("Broken", "broken", shape.value); const { choices } = normalizeChoiceList([broken]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/normalizeChoiceList.test.ts` around lines 147 - 159, Ensure the test case around MALFORMED_CHILDREN_SHAPES asserts that the filtered lossy-shape collection is non-empty before iterating, so it cannot pass vacuously if the fixture data or flag changes. Keep the existing per-shape preservation assertions unchanged.src/migrations/helpers/choice-traversal.ts (1)
21-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc says the callback receives the container value, but
onUnreadabletakes no parameters. Either drop "Called with" or pass the offending value through so callers can log it.📝 Wording tweak
- /** - * Called with every container value this walk had to step over because it - * could not read it. What makes `settingsTreeHasUnreadableData` trustworthy: + /** + * Called once for every container this walk had to step over because it + * could not read it. What makes `settingsTreeHasUnreadableData` trustworthy:🤖 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/migrations/helpers/choice-traversal.ts` around lines 21 - 27, Align the onUnreadable documentation with its declared callback signature in the choice traversal options: either remove the claim that the callback receives a container value, or update onUnreadable and its invocations to pass the offending value through. Preserve the existing unreadable-subtree traversal behavior.src/migrations/migrationReadability.test.ts (1)
320-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the resolved value instead of
not.toThrow(). Thismigrate(...).resolvesbranch tests normal completion via.resolves; appending.not.toThrow()applies atoThrowmatcher to a non-function resolved value, while the rejection path should use.resolveson a function wrapper if throwing during execution is the intent.♻️ Suggested change
- await expect(migration.migrate(plugin)).resolves.not.toThrow(); + await expect(migration.migrate(plugin)).resolves.toBeUndefined();Check the expected resolved value for this row before tightening this assertion.
🤖 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/migrations/migrationReadability.test.ts` at line 320, Update the assertion for migration.migrate(plugin) to verify its expected resolved value rather than chaining .resolves.not.toThrow(). Check the expected value for this test row and assert that value directly; preserve the separate rejection/throwing-path assertion pattern where applicable.
🤖 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/migrations/migrationReadability.test.ts`:
- Around line 107-123: Await backfillFileOpeningDefaults.migrate(plugin) in the
test before inspecting plugin.settings, so migration failures and asynchronous
mutations are observed. Rename the inner nested variable to avoid shadowing the
module-level factory while preserving the existing assertions.
In `@src/migrations/removeMacroIndirection.ts`:
- Around line 96-98: Strengthen the guard in the macro-entry iteration around
the existing `macro` check to continue unless the value is an object with a
string `id`. This prevents entries lacking a valid identifier from reaching
`choicesByMacroId.get` or `new MacroChoice`; preserve the existing handling for
valid macro objects and primitive or null holes.
In `@src/services/choiceService.test.ts`:
- Around line 559-578: Remove the duplicate “an object macro with no commands
key” case and its associated comment from the parameterized test, since both it
and “no commands key” pass undefined and construct the same macroValue. Keep the
distinct readable-list, null, and array-valued cases unchanged.
In `@src/utils/macroUtils.ts`:
- Around line 259-291: Update regenerateCommandIds to normalize array-shaped
entries with normalizeCommandList before iterating and regenerating IDs, so
nested commands are flattened and their thenCommands, elseCommands, and choice
branches are traversed. Avoid assigning IDs directly to array entries; preserve
visited tracking and UUID regeneration for actual command objects.
---
Nitpick comments:
In `@src/gui/MacroGUIs/CommandList.zoneType.test.ts`:
- Around line 36-54: Update the renderList test helper to avoid constructing the
real Obsidian runtime: change App to a type-only import and provide a minimal
stub matching the app dependency expected by CommandList. Keep the existing DnD
regression-test behavior and other injected props unchanged.
In `@src/migrations/helpers/choice-traversal.ts`:
- Around line 21-27: Align the onUnreadable documentation with its declared
callback signature in the choice traversal options: either remove the claim that
the callback receives a container value, or update onUnreadable and its
invocations to pass the offending value through. Preserve the existing
unreadable-subtree traversal behavior.
In `@src/migrations/migrationReadability.test.ts`:
- Line 320: Update the assertion for migration.migrate(plugin) to verify its
expected resolved value rather than chaining .resolves.not.toThrow(). Check the
expected value for this test row and assert that value directly; preserve the
separate rejection/throwing-path assertion pattern where applicable.
In `@src/utils/normalizeChoiceList.test.ts`:
- Around line 147-159: Ensure the test case around MALFORMED_CHILDREN_SHAPES
asserts that the filtered lossy-shape collection is non-empty before iterating,
so it cannot pass vacuously if the fixture data or flag changes. Keep the
existing per-shape preservation assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e3c9cb0-1162-41e7-9f50-f3644fbd1cdd
📒 Files selected for processing (28)
src/gui/MacroGUIs/CommandList.sveltesrc/gui/MacroGUIs/CommandList.zoneType.test.tssrc/gui/choiceList/ChoiceList.sveltesrc/gui/choiceList/ChoiceView.malformed.test.tssrc/gui/choiceList/ChoiceView.sveltesrc/gui/choiceList/seedChoiceTree.tssrc/gui/shared/dndReorder.test.tssrc/gui/shared/dndReorder.tssrc/migrations/backfillFileOpeningDefaults.test.tssrc/migrations/backfillFileOpeningDefaults.tssrc/migrations/consolidateFileExistsBehavior.tssrc/migrations/helpers/choice-traversal.tssrc/migrations/incrementFileNameSettingMoveToDefaultBehavior.tssrc/migrations/migrateFileOpeningSettings.tssrc/migrations/migrationReadability.test.tssrc/migrations/mutualExclusionInsertAfterAndWriteToBottomOfFile.tssrc/migrations/removeMacroIndirection.tssrc/services/choiceService.test.tssrc/services/choiceService.tssrc/services/duplicateChoice.nestedIds.test.tssrc/utils/choiceUtils.test.tssrc/utils/choiceUtils.tssrc/utils/macroUtils.test.tssrc/utils/macroUtils.tssrc/utils/malformedChoices.fixture.tssrc/utils/normalizeChoiceList.test.tssrc/utils/unreadableValuePredicates.test.tssrc/utils/userScriptSecrets.ts
…ut it Review follow-ups on this branch. `normalizeCommandList`/`normalizeChoiceList` read a nested ARRAY entry as a LIST and splice its members in - `isChoiceLike([])` is true, so the alternative is spreading it into one nameless row - but nothing else in the tree agreed, and the disagreement was reachable in two ways. Duplication treated a nested array as a COMMAND: `regenerateCommandIds` wrote an `id` onto it that JSON.stringify drops, and the secret walkers stepped past it - so every id and every literal `secretRef` inside it stayed shared with the original, which is #1609's symptom surviving #1609's fix. All three now recurse. The migration guards said "readable" for a nested array in `choices`, and the narrow migrations cannot descend one: `flattenChoices` pushes the array as if it WERE a choice, and the increment / mutual-exclusion recursions step straight past it. So `removeMacroIndirection` could classify a macro referenced from inside one as orphaned, duplicate it at the root and delete `settings.macros`, permanently. Both guards now REPORT a nested array rather than descending it - deliberately, and against the first fix here, which made the shared walk descend it. That walk can; the traversals the guard answers for cannot, and a guard that says "I saw everything" on behalf of a walker that did not is precisely the failure #1610 is about. The cost of the conservative answer is one launch: the user opens the settings tab, the seam splices the entries into real choices, and the next launch reads clean. Also from review: an entry in `settings.macros` with no usable `id` is no longer rehomed - the lookup missed, the orphan branch ran, and a nameless MacroChoice was pushed into the choice tree and made permanent when `macros` was deleted. And two test fixes - a `void`ed migration that only passed because the mutation happened before the first await, and two `it.each` rows that were byte-identical.
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.
Six issues in one pass, all from the same root: QuickAdd renders
data.jsonthrough filters, and then persists what it rendered. Anything a filter dropped is deleted; anything a guard could not see is skipped and flagged done forever. This is the successor to #1593/#1616 and follows its idiom - READ accessors are total and never persist their coercion, WRITE paths are guarded so a malformed value survives on disk, and the one seam allowed to REPAIR does it by re-keying, never by dropping.Every issue was reproduced on
masterin an isolated vault before anything was designed, and every fix re-verified there afterwards.#1608 - the headline: a reorder deleted choices
ChoiceListfiltered the rendered list to entries it could key, under a comment saying it was a "render-time view only. Nothing here rewrites the tree." It was not: the same filtered array seeded the drop zone and was what the persist path wrote back. So the first drag or ArrowDown committed the filtered list.For a
nullhole that is harmless. But the filter also dropped any entry whose id was not a non-empty string, and{ "name": "Daily note", "type": "Template", "templatePath": "Templates/Daily.md", "id": 12 }is a complete, working choice -
quickadd:listshows it, the palette runs it - whose id became a JSON number through a hand-edit, a script or a merge. It was invisible in settings, so the user could not miss it, and one keypress deleted it with no prompt and no undo.Proven live before the fix: 5 choices in
data.json, 4 rows rendered, one ArrowDown, 4 choices on disk.Fix: repair at the editor seam instead of hiding, mirroring what #1593 shipped for commands.
normalizeChoiceListgives an unkeyable entry a fresh uuid and keeps it, so it becomes visible, editable and deletable for the first time; only a hole is dropped. Ids are de-duplicated across the whole tree, because choice ids are globally unique (the command registry keys onchoice:<id>) unlike command ids. Nothing is persisted by the repair - it reaches disk with the user's first ordinary edit."Daily note" is missing on the left. It is also the folder that says it could not be read (#1611, below).
#1611 - an empty folder that claimed to be unreadable
hasUnreadableChildrendocumented itself as true only for a value that could still be carrying choices, then returned true for"",0andfalse- as empty as thenulland{}it correctly returned false for. Four surfaces read it, so such a folder got the scary notice, lost its drop target, and got the scarier delete confirmation. Visible in the pair above.#1610 - migrations reporting complete over data they never saw
A migration runs once and is flagged complete forever, so one that walks past a subtree it could not read leaves it un-migrated permanently - repairing
data.jsonafterwards does not help. The guard that exists to prevent that asked the wrong question in both directions:Multi.choicesonly. A Capture inside"commands": {"0": {"type": "NestedChoice", ...}}was invisible towalkAllChoices, and the guarded migrations reported complete anyway. Two migrations had no guard at all, and both translate the legacyopenFileInNewTabkeys that runtime ignores - so a hidden choice loses its "open in new tab" preference for good.choices, includingundefinedand{}, which carry nothing. One folder with nochoiceskey keptremoveMacroIndirectionpending forever: legacy macros never embedded, never removed, and every legacy macro choice in the vault dead, because nothing at runtime resolves amacroId.Fix: the guard is now derived from the walk it has to agree with -
walkSettingsreports the containers it stepped over, andsettingsTreeHasUnreadableDatais that report. Each migration asks the question its own traversal can answer: the three that recurseMulti.choicesthemselves keep the folders-only question, because blockingremoveMacroIndirectionon amacro.commandsit never descends is pure cost for the most expensive kind of pending.Two things fixed alongside, because the guard is worthless without them:
settings.macrosis untrusted too and?? []let{"0": ...}through intofor..of(three migrations threwmacros is not iterable- caught, reverted, and a 15-second "please create an issue" notice on every launch); andremoveMacroIndirectionholds the only root write among the migrations, so it gets its ownArray.isArrayguard - a readability predicate must never be the only thing between a corrupt value and a TypeError.#1609 - a duplicated macro shared the original's nested ids and secrets
regenerateIdsre-ided the macro and its top-level commands only. Nothing crashed, butbuildUserScriptSecretIdkeys a stored secret oncommand.id, andmigrateUserScriptSecretSettingsre-adopts an existing secret by that id - so a duplicated macro's nested UserScript command adopted the original's secret slot, and setting the API key on the copy set it on the original.The fix uncovered a second route to the same symptom that would have survived it: the secret sanitizers reached
choice.macro.commandsbehind anisRecordgate, andisRecordaccepts an array - so formacro: [ ... ]they walked nothing and the copy kept the original's literalsecretRef. All three readers now resolve throughmacroCommandsValueOf.The regression test asserts the symptom - the copy's nested script resolves to a different secret slot - not just that the ids differ.
#1612 - deleting an unreadable macro said nothing
A folder whose children could not be read warned about it; a macro whose commands could not be read got only the generic line. Since #1593 the builder reports that state and refuses to overwrite it, and then the one screen about to destroy it stayed quiet. Both surfaces now read the same predicate through the same accessor.
#1613 - a command could be dragged out of a branch editor into the macro builder
The branch editor opens over the still-open macro builder, both mounted a
CommandList, and both registered their drop zone under the same type. svelte-dnd-action groups zones by type and hit-tests them geometrically - a modal backdrop shields nothing - and the two lists overlap almost exactly.Proven live: with the Then-branch editor open, dragging its one command past the branch list's own bottom edge (still inside the modal) moved it into the builder underneath. Both editors then rendered a list that disagreed with the stored macro.
Left: "Wait for 250 ms" has escaped the Then branch into the macro's top level, while the conditional still reports "Then: 1" - the same command in two places, which is the
each_key_duplicateshape #1593 is about. Right: unchanged.Each list now gets its own zone type, so no two are ever in the same group.
Tradeoffs worth naming
A repaired id is a fresh uuid, never
String(oldId). Coercing12to"12"is tempting - it would keepquickadd:choice:12alive - and is wrong twice: ids are compared with===ingetChoice, so a persisted12->"12"silently breaks aChoiceCommand{choiceId: 12}(MacroChoiceEnginematches/not found/iand skips the step), and "that string is free" can only mean "not seen yet" during a pre-order walk, so it can also steal a healthy later sibling's id. A stored reference to a malformed id does break under this change - but the behaviour it replaces deleted the choice, which broke the same reference and lost the choice with it.The repaired choice's command is registered, the stale one is not removed. Nothing is persisted at seed time, so the old id is still what
getChoiceresolves; the new entry reports an honest "Choice not found" until the first save, after which they swap. The stale entry goes on the next reload. Verified live: three settings-tab opens produce one stable id and two palette entries, not four.dropFromOthersDisabledwas the first #1613 fix and is worse - only visible live. The target does refuse, but the library then re-dispatches the origin's items twice, once with its shadow placeholder and once without, and QuickAdd'sstripShadow(#1244/#883) drops the placeholder the library goes on to measure. The drag ended in an uncaughtgetBoundingClientRectTypeError with the command gone from the source list.A duplicated macro whose
commandsis{"0": ...}keeps the original's refs and ids. Deliberate faithful carry-through, and unreachable in-app: the engine throws, the builder shows the read-only card, the exporter skips it, and #1612's warning now names it before deletion. Stripping the refs without re-iding would be strictly worse.Migration cost. For a vault that stays pending, this is one
data.jsonwrite per launch, not three - the two file-opening migrations no longer save themselves, sincemigrate.tsalready saves once after the run. That matters because whole-file writes are what feeds Obsidian Sync's last-write-wins, the mechanism this corruption class comes from.Validation
pnpm run build/tsc --noEmit/pnpm run check/pnpm run lint: clean.c.fileOpening ?? c.typefalls back to the truthy string"Template", and reverting [BUG] The macro builder and the conditional branch editor share a drop zone type, with no cross-zone guard #1613's zone type left the whole suite green)."commands": {"0": ...}andchoices: "") come back byte-identical, holes are dropped, the delete dialog names the unreadable macro, the cross-zone drag no longer crosses, an ordinary reorder inside a command list still works, anddev:errorsis clean throughout.Filed rather than grown into this diff
#1625 (a new choice is discarded if a settings write lands while its builder is open - pre-existing, and the reason the seam is memoized on the raw reference), #1626 (deleting a folder is still silent about unreadable data inside it - needs the traversal lifted out of
src/migrations/), #1627 (removeMacroIndirectionnever sees a legacymacroIdchoice nested inside macro commands).Closes #1608
Closes #1609
Closes #1610
Closes #1611
Closes #1612
Closes #1613
Summary by CodeRabbit