fix: a malformed macro command list no longer makes the macro unrepairable by chhoumann · Pull Request #1616 · chhoumann/quickadd

chhoumann · 2026-07-27T16:19:56Z

The problem

IMacro.commands is declared ICommand[] and nothing enforces it. It comes from data.json, which is hand-edited, imported as a package, half-written, or merged by Obsidian Sync's whole-file last-write-wins. Every reader dereferenced it raw.

The issue title says "no read-accessor guard". That is the mechanism. The user-facing problem is that a macro whose command list has one bad byte could not be repaired from the UI at all - and two of the shapes were broken outside the UI too.

Reproduction

Nine macro shapes seeded into data.json in this worktree's isolated vault (Obsidian 1.13.0), each opened through the real settings → "Configure" path and each run via quickadd:run. Verified on master@f9f158d0 and again on this branch.

`macro` / `macro.commands`	before	after
`commands: null`	error card, 0 controls - dead end	empty, fully usable editor
`commands: [{id:"dup"},{id:"dup"}]`	error card (`each_key_duplicate`) - dead end	both commands render, editable
`commands: [ok, null, ok]`	error card (`null.id`) - dead end	both real commands render
`commands: []` (healthy default)	fine	fine
healthy	fine	fine
`commands: {"0": {…}}`	error card; run threw `i is not iterable`	read-only card + a run error naming the macro
`commands: "not a list"`	error card; run returned `ok:true` having done nothing (strings are iterable)	read-only card + a run error
`macro: null`	clicking Configure did nothing at all - no modal, no notice	empty, usable editor; first edit materializes the macro
`macro: [{…}]`	error card	entries render as commands; first edit materializes a real macro object

The macro: null row is the worst one and was not in the issue: display() runs from MacroBuilder's constructor, before open(), so the throw took the modal with it.

Before / after

A macro with two commands sharing an id - a real, working macro that one bad byte made uneditable:

before	after

A commands value we genuinely cannot read. It stays a dead end, but an honest one that names what happened, promises not to overwrite it, and says where it lives:

before	after

The other two

commands: [ok, null, ok] and commands: null:

before	after

Design

Follows the accessor split #1583 gave Multi.choices, in src/utils/macroUtils.ts:

commandListOf(value) - total READ view. Takes unknown, like rootChoicesOf, because choice.macro is untrusted too.
hasCommandList(value) - WRITE guard, so nothing rebuilds a value it could not read.
isUnreadableCommandList(value) - the line between degrading quietly and telling the user: null/{}/"" carry nothing; {"0":…}/"str"/7 might.
isMacroObject / isUnreadableMacro - the same questions one level up.
normalizeCommandList(value) - the editor-seam repair.

The editor has three states, decided by whether the value could still be carrying commands:

A readable array → fully editable. A duplicate-id or id-less command is kept under a fresh uuid, never dropped. A null hole, which carries nothing, is stepped over.
A value that carries nothing → an empty but fully usable editor. There is nothing to lose, so let the user build the macro.
A value we cannot read that might be carrying commands → a card that says so, with every writing affordance withheld, so the [] we read it as can never reach disk.

render() reports which state it is in, because ConditionalBranchEditorModal's Save button lives outside the editor and would otherwise overwrite an unreadable branch in one click. It shows a single "Close" instead.

Tradeoffs

Re-key, don't drop. The issue proposed a render-time filter mirroring ChoiceList.svelte:62. I did not ship that for duplicate ids, because the editor persists the list it rendered: the first reorder would delete a real command from data.json with no prompt and no undo. Re-keying loses nothing. (The filter survives in CommandList.svelte as a post-mount backstop, where it can only ever drop a hole - each_key_duplicate on an update escapes mountComponent's try entirely.)

Repair at the editor seam, not at load. A dedupeCommandsById in loadSettings was considered and rejected: loadSettings runs before the migrations and never saves, so it would mint fresh uuids on every launch until an unrelated save landed. At the seam it is idempotent per session, covers both hosts that show a command list, and rides along with the user's first ordinary edit - so opening and closing a malformed macro leaves data.json byte-identical (verified live for all shapes).

Not swept: removeMacroIndirection's macro.commands || []. That is truthiness, so it already passes an unreadable value through verbatim. Substituting the accessor would replace it with [] at the one migration that then deletes settings.macros and is flagged complete forever.

A separate card rather than reusing ChoicesUnavailable. Its copy is specifically about the choice tree, and MountFailed's ("include the message below if you report this") is wrong here - nothing threw, and there is no error to report.

Validation

pnpm run test - 4595 passing, 0 failures. tsc --noEmit, pnpm lint, svelte-check all clean.
Every new test verified to fail without the fix (reverted each guard in turn).
The #1566 malformed-tree ratchet had no Macro node at all, so two rows that already swept macro-reaching walkers (duplicateChoice → regenerateIds, collectChoiceClosure → packageTraversal) were dead coverage. Grown rather than forked, with malformed commands at every depth including inside conditional branches. malformedSnapshot now pins command values and arity - a walker may re-key an entry but must never lose one. The new buildPackagePreview row immediately found a pre-existing null-deref, fixed here.
All nine shapes driven end-to-end in this worktree's isolated vault, before and after.

An adversarial review of this PR found five defects in the fix itself

All reproduced, all fixed in b10a8ad, all now covered:

macro: [] / [cmd] - typeof [] === "object", so a looser guard let the builder write macro.commands = [...] onto an Array. That is a non-index property, which JSON.stringify (i.e. saveData) drops: the user added commands, saw them, and every save silently discarded them. A regression vs master, which refused to edit at all.
commands: [] fired a 15s red notice - it is the healthy default (QuickAddMacro's constructor), so every fresh macro, and every launch with an unpopulated run-on-startup macro, would have nagged.
[ok, null, ok] still threw on {{MACRO:Name::member}} - commandListOf converts the non-array shapes; a hole inside a real array does not. The one asymmetric entrypoint.
A null in a packaged folder aborted the whole import - and guarding the preview walker earlier in this branch is what made it reachable (the package now previews cleanly, so the user gets to click Import and fail there). Fixed on both the import and export sides.
A conditional showed "Then: 10" for a branch saved as "not a list" - its character count.

Review round on this PR

Graphite and Codex found four more, all valid, all fixed and covered:

An unreadable macro reported success. Logging and returning let quickadd:run answer ok: true for a macro that ran nothing - and on master the array-turned-object shape did throw, so this was a regression in the CLI contract. run() and executeConditional now throw. The conditional half mattered more: returning only exited executeConditional, so the outer loop carried on with every command after the branch, including file writes that were only ever meant to follow a branch that never ran.
```
E ObjCommands  {"ok":false,"error":"Could not read the commands for macro 'E ObjCommands'. ..."}
I FreshEmpty   {"ok":true,...}
D Healthy      {"ok":true,...,"durationMs":103}
```
and in the GUI: QuickAdd: (ERROR) Error executing choice obj-cmds: Could not read the commands for macro 'E ObjCommands'. ...
The importer wrote macro.id onto an array (a silent no-op, since JSON drops non-index properties), and read macro?.commands off it - so an array-valued macro's choice references and secret refs were never remapped, and a duplicated choice could still invoke the pre-existing local one. Both now read the new shared macroCommandsValueOf, which is also what MacroBuilder's recovery path reads.
The package fixture set entryChoiceIds where QuickAddPackage declares rootChoiceIds, hidden behind an as unknown as. A fixture that does not satisfy the production shape is the drift this sweep exists to catch; the cast is gone.
Package-import regression coverage for the recovery paths, driving applyPackageImport end-to-end. Both behavioural assertions verified to fail without the fix.

Filed rather than folded in

[BUG] Reordering the choice list deletes any choice the render filter dropped #1608 - reordering the choice list deletes any choice the render filter dropped (live data loss on master, same class, different surface)
[BUG] Duplicating a macro leaves nested command ids identical, so the copy shares the original's user-script secrets #1609 - duplicating a macro leaves nested command ids identical, so the copy shares the original's user-script secrets
[BUG] An unreadable macro.commands hides nested choices from migrations, which are then flagged complete forever #1610 - an unreadable macro.commands hides nested choices from migrations, which are then flagged complete forever
[BUG] hasUnreadableChildren treats an empty string / 0 / false as unreadable, contradicting its own contract #1611 - hasUnreadableChildren contradicts its own contract for "" / 0 / false
[BUG] Deleting a macro whose commands QuickAdd couldn't read gives no warning, unlike the folder case #1612 - deleting a macro whose commands could not be read gives no warning, unlike the folder case
[BUG] The macro builder and the conditional branch editor share a drop zone type, with no cross-zone guard #1613 - the macro builder and the branch editor share a drop-zone type with no cross-zone guard

Release impact

fix: - patch. No migration, no schema change, no setting. A malformed value is never rewritten; the only write is the user's own edit.

Summary by CodeRabbit

Bug Fixes
- Hardened macro execution, conditional branch handling, and clearer failures for malformed or missing command data (while keeping intentional empty cases non-disruptive).
- Improved member resolution and command lookup to safely handle “holey” and unexpected command-list shapes.
- Made package import/export, previews, and dependency scanning more tolerant of untyped or corrupted macro/command data.
User Experience
- Unreadable command data now shows a read-only warning card with editing controls disabled.
- Command list rendering/editing/drag-reordering now operates on valid, keyed commands only.
Tests
- Expanded coverage for malformed macro and conditional command scenarios, including read-only gating behavior.

`IMacro.commands` is declared `ICommand[]` and nothing enforces it: data.json is hand-edited, imported, half-written and sync-merged. Adds the same accessor split `Multi.choices` got in #1583 - `commandListOf` (total READ view), `hasCommandList` (WRITE guard), `isUnreadableCommandList` (the line between degrading quietly and telling the user) - plus `normalizeCommandList`, the editor-seam repair that keeps a duplicate-id or id-less command under a fresh uuid instead of hiding it. Also makes `regenerateIds` total: it is a WRITE path reached from duplicateChoice, so a non-array `commands` is now left exactly as found and a hole in the list is stepped over rather than dereferenced. Refs #1593

…cro editor Before this, every malformed `macro.commands` landed in the same place: the error card #1584 introduced, with no controls - a macro repairable only by hand-editing data.json. Verified live against seven shapes; four of them were dead ends that did not need to be. The editor now has three states, decided by whether the value could still be CARRYING commands: - a readable array -> fully editable. A duplicate-id or id-less command is kept under a fresh uuid (normalizeCommandList) rather than dropped, so the user can see and delete it; a `null` hole, which carries nothing, is stepped over. - a value that carries nothing (null, {}, absent) -> an empty but fully usable editor. `macro: null` previously made "Configure" do nothing at all: display() runs from MacroBuilder's constructor, so the throw took the modal with it. - a value we cannot read that could be carrying commands ({"0":...}, "str", 7) -> a card that says so, with every writing affordance withheld, so the `[]` we read it as can never reach disk. render() reports this to its host, because ConditionalBranchEditorModal's Save button lives outside the editor and would otherwise overwrite an unreadable branch in one click. The repair is never persisted by opening a macro - only by the user's first ordinary edit - so open-and-close leaves data.json byte-identical. Refs #1593

…ing or lying Two of the malformed shapes were broken outside the UI too, both because `if (!this.macro.commands)` is a truthiness test: an array-turned-object reached `for..of` and surfaced a bare "i is not iterable" to the user, and a string reached it INTACT - strings are iterable - so the macro reported success having walked its own characters and run nothing. A conditional's branch had the same hole one level down, skipped as silently as a branch the user left empty. Both now say what happened, name the macro, and point at data.json. Also routes the remaining command-list walkers through the accessor, guarding them at their own entry point so the recursion into then/else branches is covered too: packageTraversal, packageImportService, packagePreview, collectChoiceRequirements, SingleMacroEngine. Deliberately NOT swept: removeMacroIndirection's `macro.commands || []`. That is truthiness, so it already passes an unreadable value through VERBATIM; substituting the accessor would replace it with [] at the one migration that then deletes settings.macros and is flagged complete forever. Refs #1593

… lists The #1566 fixture had no Macro node at all, so two rows that already swept macro-reaching walkers - duplicateChoice (regenerateIds) and collectChoiceClosure (packageTraversal) - were dead coverage for them. Grows the one shared fixture rather than forking a second one that would drift: every malformed `commands` shape, a missing `macro` object, an unkeyable-but-readable array (duplicate id / no id / hole), and corruption inside a conditional's branches, all repeated at every depth. malformedSnapshot now pins command values too, and pins ARITY for a readable list: a walker may re-key an entry (that is the #1593 repair) but must never lose one. The new buildPackagePreview row found a pre-existing hole of the same class in collectChoice, which walked a packaged folder's children and dereferenced a null hole. Fixed alongside. Refs #1593

…tself Five defects, three of them regressions this branch introduced. All reproduced before fixing, all now covered by tests that fail without the fix. - `macro: []` / `macro: [cmd]`: `isCommandLike` accepts arrays (typeof [] is "object"), so the builder opened a fully editable editor and then wrote `macro.commands = [...]` onto an Array - a non-index property that JSON.stringify drops. The user added commands, saw them, and every save silently discarded them. New `isMacroObject` rejects arrays; the array's entries now render as the commands they probably are and the first edit materializes a real macro object around them. - `commands: []` is the HEALTHY default (QuickAddMacro's constructor), and the new length check turned it into a 15-second red notice on every run - and on every launch for an unpopulated run-on-startup macro. Only a MISSING list says anything now, matching the base behaviour and the sibling branch guard. - `[ok, null, ok]` still threw on the `{{MACRO:Name::member}}` path: commandListOf converts the non-array shapes, but a hole INSIDE a real array sails through .map/.filter. This was the one asymmetric entrypoint - the same macro ran fine without member access. - A `null` hole in a PACKAGED folder aborted the whole import. Guarding the preview walker earlier in this branch is what made it reachable: the package now previews cleanly, so the user gets to click Import and fail there. Fixed on both the import and export sides. - A conditional's branch rendered "Then: 10" for a branch saved as "not a list" (its character count) and "Then: 0" for an array-turned-object. Refs #1593

coderabbitai · 2026-07-27T16:20:18Z

📝 Walkthrough

Walkthrough

This change adds shared guards and normalization for malformed macro command data, applies them across execution and package traversal, and updates macro editors to filter salvageable entries or show unreadable data without editing controls. Tests cover malformed lists, conditional branches, nested fixtures, and UI behavior.

Changes

Malformed macro resilience

Layer / File(s)	Summary
Macro data contracts and malformed fixtures `src/utils/macroUtils.ts`, `src/utils/macroUtils.test.ts`, `src/utils/malformedChoices.*`	Adds command-list predicates, normalization, ID repair, malformed fixtures, and recursive resilience sweeps.
Execution and package traversal `src/engine/`, `src/preflight/collectChoiceRequirements.ts`, `src/services/`, `src/utils/packageTraversal.ts`	Uses guarded command iteration across macro execution, member access, conditional branches, package import/export, previews, preflight, and dependency collection.
Editor and rendering behavior `src/gui/MacroGUIs/*`, `src/gui/svelte/DataUnreadable.svelte`	Normalizes editable lists, filters invalid keyed entries, preserves unreadable values, displays read-only warnings, and materializes missing macro objects when edits are made.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

[BUG] An unreadable macro.commands hides nested choices from migrations, which are then flagged complete forever #1610: Addresses related unreadable macro.commands handling, though this PR focuses on execution, traversal, and editor behavior.

Possibly related PRs

chhoumann/quickadd#1592: Both update unreadable command-list handling in CommandSequenceEditor.
chhoumann/quickadd#1244: Both modify CommandList.svelte ordering and rendering data.
chhoumann/quickadd#1326: Both update SingleMacroEngine handling after pre-command mutations.

Suggested labels: released

Poem

A rabbit hops through lists of holes,
And guards the macros’ hidden scrolls.
Bad shapes rest, good commands shine,
The editor keeps each row in line.
“No crash!” we cheer—the burrow’s fine!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes match `#1593` by adding guarded reads, write protection, render-time filtering, and related traversal/import/export updates for malformed commands.
Out of Scope Changes check	✅ Passed	All modified files support malformed macro-command handling or its tests; no unrelated feature work is evident.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main fix: malformed macro command lists no longer make macros unrecoverable.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch chhoumann/1593-macro-data-guard

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

cloudflare-workers-and-pages · 2026-07-27T16:21:44Z

Deploying quickadd with Cloudflare Pages

Latest commit:	`3c50e32`
Status:	✅ Deploy successful!
Preview URL:	https://038499e5.quickadd.pages.dev
Branch Preview URL:	https://chhoumann-1593-macro-data-gu.quickadd.pages.dev

View logs

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b10a8ad0a9

ℹ️ 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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/services/packagePreview.ts (1)
462-473: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Handle array-valued macro values in the package security scan.

parseQuickAddPackage’s structural validation accepts untrusted choice/macro shapes and does not reject choice.macro as an array. When preview walks choice.macro?.commands, that array-macro case becomes undefined, while the macro editor treats the array entries as the commands. Scanned commands can therefore be hidden from the pre-install capability/dependency summary, including user scripts and conditional scripts inside that array. Align collectCommands and the dependency collector with the editor read path: when choice.macro is an array, collect the entries as commands instead of dereferencing .commands.
🤖 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/services/packagePreview.ts` around lines 462 - 473, The package security
scan must handle array-valued choice.macro entries consistently with the macro
editor. Update the collectCommands call in the isMacroChoice path and the
corresponding dependency collector to use the macro array itself as commands
when choice.macro is an array, while preserving the existing .commands handling
for object-valued macros.

🧹 Nitpick comments (1)

src/services/packageImportService.ts (1)
907-912: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Prefer isMacroObject over isCommandLike for the macro guard.

isCommandLike accepts arrays, and macroUtils documents exactly that hazard: setting a non-index property (macro.id) on an array is dropped by JSON.stringify, so the regenerated id silently never persists. isMacroObject is the predicate written for this case, and it also reads better than a "command" guard applied to a macro.
♻️ Proposed change
-		if (isDuplicated && isCommandLike(macroChoice.macro)) {
+		if (isDuplicated && isMacroObject(macroChoice.macro)) {
 			macroChoice.macro.id = uuidv4();
 		}
Also update the import on Line 20 to include isMacroObject.
🤖 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/services/packageImportService.ts` around lines 907 - 912, Replace the
isCommandLike guard in the duplicated macro handling with isMacroObject, and
update the import to include isMacroObject. Keep the existing UUID assignment
unchanged so IDs are regenerated only for valid macro objects.

🤖 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/utils/malformedChoices.entrypoints.test.ts`:
- Around line 180-192: Update the packageOf() test helper to populate the
QuickAddPackage rootChoiceIds field instead of entryChoiceIds, using the
existing filtered choice IDs. Remove the unknown type assertion if the object
now satisfies QuickAddPackage, while preserving the remaining fixture fields.

---

Outside diff comments:
In `@src/services/packagePreview.ts`:
- Around line 462-473: The package security scan must handle array-valued
choice.macro entries consistently with the macro editor. Update the
collectCommands call in the isMacroChoice path and the corresponding dependency
collector to use the macro array itself as commands when choice.macro is an
array, while preserving the existing .commands handling for object-valued
macros.

---

Nitpick comments:
In `@src/services/packageImportService.ts`:
- Around line 907-912: Replace the isCommandLike guard in the duplicated macro
handling with isMacroObject, and update the import to include isMacroObject.
Keep the existing UUID assignment unchanged so IDs are regenerated only for
valid macro objects.

🪄 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: 67d9a33a-8861-4ebc-822f-8c1f7d6fbb79

📥 Commits

Reviewing files that changed from the base of the PR and between f9f158d and b10a8ad.

📒 Files selected for processing (21)

src/engine/MacroChoiceEngine.malformed.test.ts
src/engine/MacroChoiceEngine.ts
src/engine/SingleMacroEngine.member-access.test.ts
src/engine/SingleMacroEngine.ts
src/gui/MacroGUIs/CommandList.svelte
src/gui/MacroGUIs/CommandSequenceEditor.ts
src/gui/MacroGUIs/CommandSequenceEditor.unrenderable.test.ts
src/gui/MacroGUIs/Components/ConditionalCommand.svelte
src/gui/MacroGUIs/ConditionalBranchEditorModal.ts
src/gui/MacroGUIs/MacroBuilder.malformed.test.ts
src/gui/MacroGUIs/MacroBuilder.ts
src/gui/svelte/DataUnreadable.svelte
src/preflight/collectChoiceRequirements.ts
src/services/packageExportService.ts
src/services/packageImportService.ts
src/services/packagePreview.ts
src/utils/macroUtils.test.ts
src/utils/macroUtils.ts
src/utils/malformedChoices.entrypoints.test.ts
src/utils/malformedChoices.fixture.ts
src/utils/packageTraversal.ts

…uccess Review follow-up on #1616. - run() and executeConditional now THROW for a command list they could not read, rather than logging and returning. Returning let `quickadd:run` answer ok:true for a macro that ran nothing, so automation carried on as if it had worked - and in the conditional case the outer loop went on to run every command AFTER the branch, including file writes that were only ever meant to follow a branch that never ran. - packageImportService used the looser isCommandLike for the `macro` object, so `macro.id = uuidv4()` on an array-valued macro was a silent no-op (JSON drops non-index properties) and the duplicate kept the original's id. - remapCommands and applyOverridesToCommands read `macro?.commands`, which is undefined for an array-valued macro - so the choice references and secret refs its entries hold were never remapped, and a duplicated choice could still invoke the pre-existing local one. Both now go through the new shared macroCommandsValueOf, which is also what MacroBuilder's recovery path reads, so the two cannot disagree about where a macro's commands live. Refs #1593

…st it `packageOf` set `entryChoiceIds` where QuickAddPackage declares `rootChoiceIds`, and an `as unknown as` hid the mismatch. It happens not to matter today because walkPackage derives its entry ids from `pkg.choices`, but a fixture that does not actually satisfy the shape the walker is handed in production is exactly the drift this sweep exists to catch. Structurally typed now, with no cast. Refs #1593

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/services/packageImportService.ts`:
- Around line 912-925: Add package-import regression tests covering the recovery
paths around isMacroObject and macroCommandsValueOf: verify direct-array macros
remap choice and secret references and apply asset-path overrides, and verify
Multi children that are null or non-choice are handled safely. Place the
coverage in the existing package-import test suite and include cases
corresponding to the related logic near the cited symbols.

🪄 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: 09e2f13b-4837-4d74-912d-24b25ca1ad3e

📥 Commits

Reviewing files that changed from the base of the PR and between b10a8ad and 62cca54.

📒 Files selected for processing (6)

src/engine/MacroChoiceEngine.malformed.test.ts
src/engine/MacroChoiceEngine.ts
src/gui/MacroGUIs/MacroBuilder.ts
src/services/packageImportService.ts
src/utils/macroUtils.ts
src/utils/malformedChoices.entrypoints.test.ts

🚧 Files skipped from review as they are similar to previous changes (4)

src/gui/MacroGUIs/MacroBuilder.ts
src/engine/MacroChoiceEngine.ts
src/utils/malformedChoices.entrypoints.test.ts
src/utils/macroUtils.ts

…eal import Drives applyPackageImport end-to-end over every malformed `macro` shape, and pins the two behaviours the last round changed: - an array-valued macro has its choice references remapped, so a duplicated choice's reference follows the imported copy instead of resolving to the pre-existing local choice - a `null` hole in a packaged folder's children no longer aborts the whole import Both fail without the fix (verified by reverting each guard in turn). Refs #1593

coderabbitai

🧹 Nitpick comments (1)

src/services/packageImportService.malformed.test.ts (1)
209-227: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assertion can't actually detect the regression it names.

Per the upstream comment in packageImportService.ts (context snippet 2), the guarded bug is macro.id = uuidv4() being applied to an Array — a non-index property that JSON.stringify silently drops. But this test only asserts against JSON.parse(JSON.stringify(result.updatedChoices)) (Line 217), and that serialization step erases the very property the bug would set — whether the buggy isCommandLike check or the fixed isMacroObject check ran, the JSON output is identical. As written, this test would pass even if the fix were reverted.

Consider asserting on the pre-serialization object instead (or in addition), e.g. checking that the array has no own id property, to actually distinguish the fixed vs. buggy code path.
♻️ Suggested additional assertion
 		const macro = macroChoice("m1", [choiceCommand("cmd-1", "nope")]);
 		const result = await importPackaged([macro], "duplicate", [
 			macroChoice("m1", [choiceCommand("cmd-1", "nope")]),
 		]);
 
+		// Check the raw (pre-serialization) object, since JSON.stringify would
+		// silently mask a stray non-index `.id` property on an array either way.
+		const rawMacros = result.updatedChoices.filter(
+			(c) => c.type === "Macro",
+		) as unknown as { macro: unknown[] }[];
+		for (const m of rawMacros) {
+			expect(Object.prototype.hasOwnProperty.call(m.macro, "id")).toBe(false);
+		}
+
 		// The array survives the JSON round-trip whole; nothing was written onto it
 		// as a non-index property (which JSON.stringify would have dropped).
 		const persisted = JSON.parse(JSON.stringify(result.updatedChoices));
Based on path instructions (`**/*.{test,spec}.{ts,tsx}`: "Add regression coverage for bug fixes."), the test's name promises coverage this assertion doesn't actually provide.
🤖 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/services/packageImportService.malformed.test.ts` around lines 209 - 227,
Update the test “does not silently no-op the macro id when duplicating an
array-valued macro” to inspect the pre-serialization objects in
result.updatedChoices, asserting each array-valued macro has no own id property.
Keep the existing JSON round-trip assertions if useful, but add a direct
assertion that distinguishes the fixed isMacroObject behavior from the
regression.
Source: Path instructions

🤖 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/services/packageImportService.malformed.test.ts`:
- Around line 209-227: Update the test “does not silently no-op the macro id
when duplicating an array-valued macro” to inspect the pre-serialization objects
in result.updatedChoices, asserting each array-valued macro has no own id
property. Keep the existing JSON round-trip assertions if useful, but add a
direct assertion that distinguishes the fixed isMacroObject behavior from the
regression.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d9dab145-12d4-4494-9419-cc0dd25695bc

📥 Commits

Reviewing files that changed from the base of the PR and between 62cca54 and 3c50e32.

📒 Files selected for processing (1)

src/services/packageImportService.malformed.test.ts

chhoumann added 5 commits July 27, 2026 17:21

graphite-app Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread src/services/packageImportService.ts Outdated

chatgpt-codex-connector Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread src/engine/MacroChoiceEngine.ts Outdated

Comment thread src/engine/MacroChoiceEngine.ts Outdated

Comment thread src/services/packageImportService.ts Outdated

coderabbitai Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread src/utils/malformedChoices.entrypoints.test.ts Outdated

chhoumann added 2 commits July 27, 2026 18:33

coderabbitai Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread src/services/packageImportService.ts

coderabbitai Bot reviewed Jul 27, 2026

View reviewed changes

chhoumann merged commit 5ab1c07 into master Jul 27, 2026
14 checks passed

chhoumann deleted the chhoumann/1593-macro-data-guard branch July 27, 2026 18:53

quickadd-release-bot Bot mentioned this pull request Jul 27, 2026

release(version): Release 2.20.0 #1534

Merged

coderabbitai Bot mentioned this pull request Jul 27, 2026

fix: say so when a macro step cannot be run, instead of dropping it #1623

Merged

chhoumann mentioned this pull request Jul 27, 2026

fix: a malformed data.json no longer costs you the choices QuickAdd could not draw #1629

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: a malformed macro command list no longer makes the macro unrepairable - #1616

fix: a malformed macro command list no longer makes the macro unrepairable#1616
chhoumann merged 8 commits into
masterfrom
chhoumann/1593-macro-data-guard

chhoumann commented Jul 27, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 27, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 27, 2026 •

edited

Loading

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

chhoumann commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

Reproduction

Before / after

Design

Tradeoffs

Validation

An adversarial review of this PR found five defects in the fix itself

Review round on this PR

Filed rather than folded in

Release impact

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying quickadd with Cloudflare Pages

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

chhoumann commented Jul 27, 2026 •

edited

Loading

coderabbitai Bot commented Jul 27, 2026 •

edited

Loading

cloudflare-workers-and-pages Bot commented Jul 27, 2026 •

edited

Loading