Fix: Controls panel removes function properties when editing objects#33984
Open
valentinpalkovic wants to merge 6 commits intonextfrom
Open
Fix: Controls panel removes function properties when editing objects#33984valentinpalkovic wants to merge 6 commits intonextfrom
valentinpalkovic wants to merge 6 commits intonextfrom
Conversation
…mdc, capture-terminal-output.ts, exec.ts)
…s for builder-vite and builder-webpack5
…diting objects Function args cannot be serialized through the postmessage channel (telejson strips them), so the manager's view of object args is missing function properties. When Controls panel sends an updated object back, ArgsStore.update was doing a shallow replacement of the entire object, losing function properties that were in the preview's current args. The fix introduces `mergeArgsPreservingFunctions` which, when merging a plain object arg update, preserves function properties from the current value that are absent from the update (since the manager never knew about them). Non- function properties not present in the update are correctly NOT preserved, allowing explicit removal of properties via the object editor to still work. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Contributor
📝 WalkthroughWalkthroughThis PR establishes a comprehensive verification framework for Storybook bug fixes, including workflow documentation for different component types (builders, renderers, managers), GitHub Actions automation for running tests and capturing builder terminal outputs, snapshot-based output comparison infrastructure, template stories for verification, enhanced argument merging to preserve non-serializable functions, and script utilities for automation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
✨ Finishing Touches
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
scripts/terminal-output-snapshots/builder-vite-build.snap.txt (1)
1-2: Align snapshot header format with the webpack baseline.This file lacks the provenance/provisional header used in
builder-webpack5-build.snap.txt, which makes audit/update intent less explicit.Suggested header normalization
+ # PROVISIONAL BASELINE — requires reviewer approval before merge + # Generated by: jiti scripts/capture-terminal-output.ts --builder builder-vite --mode build --update + # + # To update this baseline after reviewing, run: + # jiti scripts/capture-terminal-output.ts --builder builder-vite --mode build --update + - ┌ Building storybook v10.3.0-alpha.12🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/terminal-output-snapshots/builder-vite-build.snap.txt` around lines 1 - 2, Update the snapshot header in builder-vite-build.snap.txt to include the same provenance/provisional header format used in the webpack baseline snapshot so audit/update intent is explicit; open the existing snapshot header in builder-webpack5-build.snap.txt as the reference and prepend the matching multi-line provenance/provisional header to the top of the vite snapshot (ensuring whitespace and wording match exactly) so the two snapshot headers are normalized.scripts/capture-terminal-output.ts (1)
63-67:stripHeaderremoves more than the provisional header block.Current logic strips every
#...line in the snapshot body, not just the provisional header. This can discard legitimate output lines before diffing.Proposed fix
function stripHeader(content: string): string { - return content - .replace(/^#.*$/gm, '') - .replace(/^\s*\n/gm, '') - .trimEnd(); + if (content.startsWith(PROVISIONAL_HEADER)) { + return content.slice(PROVISIONAL_HEADER.length).replace(/^\n+/, '').trimEnd(); + } + return content.trimEnd(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/capture-terminal-output.ts` around lines 63 - 67, stripHeader currently removes every line that starts with '#' across the entire content; change it to only strip the provisional header block at the top of the file by removing a contiguous leading run of lines that start with '#' (and a following single empty line if present) while leaving any later '#' lines intact. Update the stripHeader function to match and remove only the initial header block (e.g., consume from the start of content ^ through the first non-# non-empty line) so legitimate output lines containing '#' later in the snapshot are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.cursor/rules/manager-bug-workflow.mdc:
- Around line 60-62: Update the Step 5 test command that currently runs "cd code
&& yarn playwright test" so it exports the STORYBOOK_URL environment variable to
the dev server used in Step 3; replace the invocation "cd code && yarn
playwright test" with a command that sets STORYBOOK_URL=http://localhost:6006
before running yarn playwright test so tests don't fall back to
http://localhost:8001.
In @.github/copilot-instructions.md:
- Around line 531-533: Add the language identifier "html" to the fenced code
block that contains the HTML comment "<!-- PROVISIONAL BASELINE — requires
reviewer approval before merge -->" so the block starts with ```html instead of
``` to satisfy markdownlint MD040; update the opening fence accordingly and
leave the closing fence unchanged.
In `@code/core/src/preview-api/modules/store/ArgsStore.ts`:
- Around line 28-33: Replace the prototype-inclusive `in` checks with
own-property checks using Object.hasOwn() to avoid prototype pollution;
specifically update the conditional expressions inside
mergeArgsPreservingFunctions (the branches that currently use `key in update`
and any other occurrences in the same module, including the check referenced
near the end of the file) so they call Object.hasOwn(update, key) (and
Object.hasOwn(someObj, someKey) for the other occurrence) before treating
properties as present — keep the existing logic (preserve function values,
recurse on plain objects) but gate presence tests with Object.hasOwn.
In
`@code/renderers/react/template/stories/visual-render-verification.stories.tsx`:
- Around line 26-37: Replace the unsafe cast on the Storybook meta by using the
TypeScript "satisfies" operator: update the default export for
VisualRenderVerification to use "satisfies Meta<typeof
VisualRenderVerification>" instead of "as Meta<typeof VisualRenderVerification>"
so the meta object keeps tighter typing under strict mode; leave the
StoryObj-typed exports Primary and WithBorder unchanged (they reference
VisualRenderVerification and StoryObj<typeof VisualRenderVerification>) and
ensure imports for Meta/StoryObj remain correct.
In `@scripts/capture-terminal-output.ts`:
- Around line 14-16: The PORT computation can produce NaN when
process.env.STORYBOOK_SERVE_PORT is non-numeric; update the logic that derives
PORT (the const PORT assignment using process.env.STORYBOOK_SERVE_PORT and
Number.parseInt) to validate the parsed value and fall back to 6006 when
parseInt yields NaN or an invalid number (e.g., non-positive or non-integer).
Trim the env string, parse with radix 10, check Number.isFinite/Number.isInteger
and > 0 (or use Number.isNaN) and if invalid assign 6006 so any generated
command uses a valid --port value.
In `@scripts/terminal-output-snapshots/README.md`:
- Line 17: Update the README wording about absolute-path normalization to
explicitly state that besides replacing standalone absolute paths with the token
<ROOT>, the tool also rewrites prefixed absolute paths (for example transforms
paths like /Users/.../sandboxes into <ROOT>-sandboxes/...) so reviewers
understand prefix-rewrite cases; mention the specific example
`<ROOT>-sandboxes/...` and clarify that any absolute-path segment used as a
prefix will be replaced with `<ROOT>-` followed by the remaining path segment.
In `@scripts/utils/exec.ts`:
- Around line 52-57: The loop in exec.ts uses value comparison to detect the
last subcommand which can return early if an earlier subcommand string repeats;
update the loop to use an index-based check instead (e.g., iterate with for (let
i = 0; i < command.length; i++) or keep a separate counter) and replace the
condition "subcommand === command[command.length - 1]" with a position check
like "i === command.length - 1" when deciding to return captured output; ensure
you still await currentChild and only return result.all (or '') when
captureOutput is true and the current iteration is the final index, leaving
earlier identical subcommand strings to continue execution.
---
Nitpick comments:
In `@scripts/capture-terminal-output.ts`:
- Around line 63-67: stripHeader currently removes every line that starts with
'#' across the entire content; change it to only strip the provisional header
block at the top of the file by removing a contiguous leading run of lines that
start with '#' (and a following single empty line if present) while leaving any
later '#' lines intact. Update the stripHeader function to match and remove only
the initial header block (e.g., consume from the start of content ^ through the
first non-# non-empty line) so legitimate output lines containing '#' later in
the snapshot are preserved.
In `@scripts/terminal-output-snapshots/builder-vite-build.snap.txt`:
- Around line 1-2: Update the snapshot header in builder-vite-build.snap.txt to
include the same provenance/provisional header format used in the webpack
baseline snapshot so audit/update intent is explicit; open the existing snapshot
header in builder-webpack5-build.snap.txt as the reference and prepend the
matching multi-line provenance/provisional header to the top of the vite
snapshot (ensuring whitespace and wording match exactly) so the two snapshot
headers are normalized.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.cursor/rules/builder-bug-workflow.mdc.cursor/rules/manager-bug-workflow.mdc.cursor/rules/renderer-bug-workflow.mdc.cursor/rules/verification-checklist.mdc.github/copilot-instructions.md.github/workflows/copilot-setup-steps.yml.github/workflows/copilot-verification.yml.traycer/completion-t5.jsoncode/core/src/preview-api/modules/store/ArgsStore.test.tscode/core/src/preview-api/modules/store/ArgsStore.tscode/e2e-tests/manager.spec.tscode/renderers/react/template/stories/copilot-verification-example.stories.tsxcode/renderers/react/template/stories/visual-render-verification.stories.tsxpackage.jsonscripts/capture-terminal-output.tsscripts/package.jsonscripts/terminal-output-snapshots/README.mdscripts/terminal-output-snapshots/builder-vite-build.snap.txtscripts/terminal-output-snapshots/builder-webpack5-build.snap.txtscripts/utils/exec.ts
Comment on lines
+60
to
+62
| ```bash | ||
| cd code && yarn playwright test | ||
| ``` |
Contributor
There was a problem hiding this comment.
Step 5 command can target the wrong server URL.
Given the fallback to http://localhost:8001, this command should set STORYBOOK_URL=http://localhost:6006 to match Step 3’s dev-server flow.
Suggested command update
```bash
-cd code && yarn playwright test
+cd code && STORYBOOK_URL=http://localhost:6006 yarn playwright test</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.cursor/rules/manager-bug-workflow.mdc around lines 60 - 62, Update the Step
5 test command that currently runs "cd code && yarn playwright test" so it
exports the STORYBOOK_URL environment variable to the dev server used in Step 3;
replace the invocation "cd code && yarn playwright test" with a command that
sets STORYBOOK_URL=http://localhost:6006 before running yarn playwright test so
tests don't fall back to http://localhost:8001.
Comment on lines
+531
to
+533
| ``` | ||
| <!-- PROVISIONAL BASELINE — requires reviewer approval before merge --> | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to the fenced code block on Line 531.
This block currently violates markdownlint MD040 (fenced-code-language).
Suggested fix
- ```
+ ```html
<!-- PROVISIONAL BASELINE — requires reviewer approval before merge --></details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 531-531: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/copilot-instructions.md around lines 531 - 533, Add the language
identifier "html" to the fenced code block that contains the HTML comment "<!--
PROVISIONAL BASELINE — requires reviewer approval before merge -->" so the block
starts with ```html instead of ``` to satisfy markdownlint MD040; update the
opening fence accordingly and leave the closing fence unchanged.
Comment on lines
+28
to
+33
| if (!(key in update) && typeof value === 'function') { | ||
| // Preserve function properties that weren't included in the update | ||
| result[key] = value; | ||
| } else if (key in update && isPlainObject(value) && isPlainObject(update[key])) { | ||
| // Recursively merge nested plain objects | ||
| result[key] = mergeArgsPreservingFunctions(value, update[key]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n code/core/src/preview-api/modules/store/ArgsStore.ts | head -120Repository: storybookjs/storybook
Length of output: 5035
🏁 Script executed:
rg -t ts "argsUpdate|current\[|update\[" code/core/src/preview-api/modules/store/ -A 2 -B 2 | head -100Repository: storybookjs/storybook
Length of output: 2869
🏁 Script executed:
rg -t ts "isPlainObject" code/core/src/preview-api/modules/store/ -B 2 -A 2 | head -50Repository: storybookjs/storybook
Length of output: 3804
🏁 Script executed:
# Check how args objects are created/passed to understand if custom prototypes are possible
rg -t ts "argsUpdate\s*:" code/core/src/preview-api/ -B 3 -A 3 | head -60Repository: storybookjs/storybook
Length of output: 687
🏁 Script executed:
# Check if args objects come from serialization/deserialization (typically plain objects)
rg -t ts "type Args" code/core/src/preview-api/modules/store/ -B 2 -A 2Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Check the Args type definition
rg -t ts "type Args\s*=" --no-heading | head -20Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Find Args type definition
fd -t f "types" code/core/src | grep -E "\.ts$" | head -10Repository: storybookjs/storybook
Length of output: 486
🏁 Script executed:
# Search for Args type definition
rg "export.*type Args" --no-heading -g "*.ts" | head -20Repository: storybookjs/storybook
Length of output: 384
🏁 Script executed:
# Search for where Args is imported from
rg "from.*types.*Args|^export.*Args\s*=" code/core/src/preview-api/modules/store/ArgsStore.ts -B 2Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Find the Args type definition in internal/types
find . -path "*/internal/types*" -name "*.ts" -type f | head -5Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Search for Args type definition broadly
rg "^export.*type Args\s*=" --no-heading -g "*.ts" -A 2 | head -30Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Check what combineArgs does (it's used with argsUpdate)
rg -t ts "function combineArgs|export.*combineArgs" -A 10 code/core/src/preview-api/modules/store/args.ts | head -40Repository: storybookjs/storybook
Length of output: 416
🏁 Script executed:
# Let's check if there are any tests that cover merge behavior with edge case keys
rg -t ts "toString|constructor" code/core/src/preview-api/modules/store/ -B 2 -A 2Repository: storybookjs/storybook
Length of output: 2731
Use own-property checks instead of in for consistency and to guard against prototype pollution.
On lines 28, 31, and 94, replace in with Object.hasOwn() to explicitly check for own properties only. While the current code works correctly for typical plain-object args, using own-property checks is more defensive and aligns with security best practices to prevent unexpected behavior if args ever received objects with custom prototypes.
Suggested fix
function mergeArgsPreservingFunctions(current: any, update: any): any {
if (!isPlainObject(current) || !isPlainObject(update)) {
return update;
}
const result: Record<string, any> = { ...update };
for (const [key, value] of Object.entries(current)) {
- if (!(key in update) && typeof value === 'function') {
+ if (!Object.hasOwn(update, key) && typeof value === 'function') {
// Preserve function properties that weren't included in the update
result[key] = value;
- } else if (key in update && isPlainObject(value) && isPlainObject(update[key])) {
+ } else if (Object.hasOwn(update, key) && isPlainObject(value) && isPlainObject(update[key])) {
// Recursively merge nested plain objects
result[key] = mergeArgsPreservingFunctions(value, update[key]);
}
}
return result;
}
@@
- if (key in argsUpdate) {
+ if (Object.hasOwn(argsUpdate, key)) {
merged[key] = mergeArgsPreservingFunctions(current[key], argsUpdate[key]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/preview-api/modules/store/ArgsStore.ts` around lines 28 - 33,
Replace the prototype-inclusive `in` checks with own-property checks using
Object.hasOwn() to avoid prototype pollution; specifically update the
conditional expressions inside mergeArgsPreservingFunctions (the branches that
currently use `key in update` and any other occurrences in the same module,
including the check referenced near the end of the file) so they call
Object.hasOwn(update, key) (and Object.hasOwn(someObj, someKey) for the other
occurrence) before treating properties as present — keep the existing logic
(preserve function values, recurse on plain objects) but gate presence tests
with Object.hasOwn.
Comment on lines
+26
to
+37
| export default { | ||
| component: VisualRenderVerification, | ||
| parameters: { chromatic: { disableSnapshot: true } }, | ||
| } as Meta<typeof VisualRenderVerification>; | ||
|
|
||
| export const Primary: StoryObj<typeof VisualRenderVerification> = { | ||
| args: { border: false }, | ||
| }; | ||
|
|
||
| export const WithBorder: StoryObj<typeof VisualRenderVerification> = { | ||
| args: { border: true }, | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n code/renderers/react/template/stories/visual-render-verification.stories.tsx | head -40Repository: storybookjs/storybook
Length of output: 1278
🏁 Script executed:
# Check typing conventions for Meta in this folder
rg -n --type=tsx 'satisfies\s+Meta<|as\s+Meta<' code/renderers/react/template/storiesRepository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
# Expand search to see broader patterns in react renderer stories
rg -n --type=tsx '(satisfies|as)\s+Meta<' code/renderers/react/template/stories --context 2Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
# Check if there are any existing patterns with satisfies Meta in the entire repo
rg --type=tsx 'satisfies\s+Meta<' code/renderers --max-count 10Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
# Check typing conventions for Meta in stories (search without -t flag)
rg -n '(satisfies|as)\s+Meta<' code/renderers/react/template/stories --context 2Repository: storybookjs/storybook
Length of output: 1086
🏁 Script executed:
# Broader search across renderer stories
rg -n '(satisfies|as)\s+Meta<' code/renderers/*/template/stories --max-count 20Repository: storybookjs/storybook
Length of output: 281
🏁 Script executed:
# Check for satisfies Meta pattern anywhere in codebase
rg 'satisfies\s+Meta<' code --max-count 10Repository: storybookjs/storybook
Length of output: 22585
🏁 Script executed:
# Check the overall prevalence of as Meta vs satisfies Meta in stories
rg '(satisfies|as)\s+Meta<' code --max-count 30 -A 0Repository: storybookjs/storybook
Length of output: 28946
Prefer satisfies over as for Storybook meta typing.
Using satisfies Meta<...> avoids unsafe casting and keeps story typing tighter under strict mode. This aligns with the codebase pattern enforced by the ESLint rule meta-satisfies-type.
Suggested typing refactor
export default {
component: VisualRenderVerification,
parameters: { chromatic: { disableSnapshot: true } },
-} as Meta<typeof VisualRenderVerification>;
+} satisfies Meta<typeof VisualRenderVerification>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/renderers/react/template/stories/visual-render-verification.stories.tsx`
around lines 26 - 37, Replace the unsafe cast on the Storybook meta by using the
TypeScript "satisfies" operator: update the default export for
VisualRenderVerification to use "satisfies Meta<typeof
VisualRenderVerification>" instead of "as Meta<typeof VisualRenderVerification>"
so the meta object keeps tighter typing under strict mode; leave the
StoryObj-typed exports Primary and WithBorder unchanged (they reference
VisualRenderVerification and StoryObj<typeof VisualRenderVerification>) and
ensure imports for Meta/StoryObj remain correct.
Comment on lines
+14
to
+16
| const PORT = process.env.STORYBOOK_SERVE_PORT | ||
| ? Number.parseInt(process.env.STORYBOOK_SERVE_PORT, 10) | ||
| : 6006; |
Contributor
There was a problem hiding this comment.
Guard against invalid STORYBOOK_SERVE_PORT values.
If the env var is set to a non-numeric value, PORT becomes NaN, and the generated command uses an invalid --port value.
Proposed fix
-const PORT = process.env.STORYBOOK_SERVE_PORT
- ? Number.parseInt(process.env.STORYBOOK_SERVE_PORT, 10)
- : 6006;
+const parsedPort = process.env.STORYBOOK_SERVE_PORT
+ ? Number.parseInt(process.env.STORYBOOK_SERVE_PORT, 10)
+ : 6006;
+
+const PORT = Number.isInteger(parsedPort) && parsedPort > 0 ? parsedPort : 6006;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/capture-terminal-output.ts` around lines 14 - 16, The PORT
computation can produce NaN when process.env.STORYBOOK_SERVE_PORT is
non-numeric; update the logic that derives PORT (the const PORT assignment using
process.env.STORYBOOK_SERVE_PORT and Number.parseInt) to validate the parsed
value and fall back to 6006 when parseInt yields NaN or an invalid number (e.g.,
non-positive or non-integer). Trim the env string, parse with radix 10, check
Number.isFinite/Number.isInteger and > 0 (or use Number.isNaN) and if invalid
assign 6006 so any generated command uses a valid --port value.
| The capture script strips or normalizes volatile output so snapshots remain stable between runs: | ||
|
|
||
| - Absolute paths under the repo root are replaced with `<ROOT>` | ||
| - Any other absolute paths (`/Users/...`, `/home/...`, `/tmp/...`) are replaced with `<ROOT>` |
Contributor
There was a problem hiding this comment.
Clarify absolute-path normalization wording.
Line 17 says non-root absolute paths are replaced with <ROOT>, but current snapshots include forms like <ROOT>-sandboxes/.... Please document that prefix-rewrite case explicitly to avoid reviewer confusion.
Suggested wording update
-- Any other absolute paths (`/Users/...`, `/home/...`, `/tmp/...`) are replaced with `<ROOT>`
+- Absolute paths that share the repo-root prefix (for example `<repo>-sandboxes/...`) are rewritten with a `<ROOT>` prefix
+- Other absolute paths (`/Users/...`, `/home/...`, `/tmp/...`) are replaced with `<ROOT>`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Any other absolute paths (`/Users/...`, `/home/...`, `/tmp/...`) are replaced with `<ROOT>` | |
| - Absolute paths that share the repo-root prefix (for example `<repo>-sandboxes/...`) are rewritten with a `<ROOT>` prefix | |
| - Other absolute paths (`/Users/...`, `/home/...`, `/tmp/...`) are replaced with `<ROOT>` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/terminal-output-snapshots/README.md` at line 17, Update the README
wording about absolute-path normalization to explicitly state that besides
replacing standalone absolute paths with the token <ROOT>, the tool also
rewrites prefixed absolute paths (for example transforms paths like
/Users/.../sandboxes into <ROOT>-sandboxes/...) so reviewers understand
prefix-rewrite cases; mention the specific example `<ROOT>-sandboxes/...` and
clarify that any absolute-path segment used as a prefix will be replaced with
`<ROOT>-` followed by the remaining path segment.
Comment on lines
52
to
+57
| for (const subcommand of command) { | ||
| logger.debug(`> ${subcommand}`); | ||
| currentChild = execa(subcommand, { ...defaultOptions, ...options }); | ||
| await currentChild; | ||
| const result = await currentChild; | ||
| if (captureOutput && subcommand === command[command.length - 1]) { | ||
| return result.all ?? ''; |
Contributor
There was a problem hiding this comment.
Multi-command capture can return early when subcommand text repeats.
The last-command check compares string values, not position. If an earlier subcommand string equals the last one, execution returns too early and skips remaining commands.
Proposed fix
- for (const subcommand of command) {
+ for (const [index, subcommand] of command.entries()) {
logger.debug(`> ${subcommand}`);
currentChild = execa(subcommand, { ...defaultOptions, ...options });
const result = await currentChild;
- if (captureOutput && subcommand === command[command.length - 1]) {
+ if (captureOutput && index === command.length - 1) {
return result.all ?? '';
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const subcommand of command) { | |
| logger.debug(`> ${subcommand}`); | |
| currentChild = execa(subcommand, { ...defaultOptions, ...options }); | |
| await currentChild; | |
| const result = await currentChild; | |
| if (captureOutput && subcommand === command[command.length - 1]) { | |
| return result.all ?? ''; | |
| for (const [index, subcommand] of command.entries()) { | |
| logger.debug(`> ${subcommand}`); | |
| currentChild = execa(subcommand, { ...defaultOptions, ...options }); | |
| const result = await currentChild; | |
| if (captureOutput && index === command.length - 1) { | |
| return result.all ?? ''; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/utils/exec.ts` around lines 52 - 57, The loop in exec.ts uses value
comparison to detect the last subcommand which can return early if an earlier
subcommand string repeats; update the loop to use an index-based check instead
(e.g., iterate with for (let i = 0; i < command.length; i++) or keep a separate
counter) and replace the condition "subcommand === command[command.length - 1]"
with a position check like "i === command.length - 1" when deciding to return
captured output; ensure you still await currentChild and only return result.all
(or '') when captureOutput is true and the current iteration is the final index,
leaving earlier identical subcommand strings to continue execution.
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.
Issue
Fixes #33802
Root Cause
Function args cannot be serialized through the postmessage channel (telejson strips them by default), so the manager's view of object args is missing function properties. When the Controls panel sends an updated object back via
UPDATE_STORY_ARGS,ArgsStore.updatewas doing a shallow replacement of the entire object arg, losing function properties that were still present in the preview's current args.For example, given
buttonProps: { title: "Click me", onClick: () => ..., variant: "primary" }:buttonPropswithoutonClick(stripped by channel serialization)title→ Controls sends{ buttonProps: { title: "New", variant: "primary" } }ArgsStore.updatereplacesbuttonPropsentirely →onClickis permanently lostonClickhandlerSolution
Introduced
mergeArgsPreservingFunctionsinArgsStore.updatethat, when merging a plain object arg update, preserves function properties from the current value that are absent from the update (since the manager never knew about them). Non-function properties not present in the update are correctly not preserved, so explicit removal of properties via the object editor still works correctly.Tests
✅ All tests passing (Flow 0)
Added 3 new tests covering:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores