Fix: argType detail popover overflows viewport when content is tall#34024
Fix: argType detail popover overflows viewport when content is tall#34024yannbf wants to merge 35 commits into
Conversation
…mdc, capture-terminal-output.ts, exec.ts)
…s for builder-vite and builder-webpack5
…detection jobs, streamline terminal output checks
…seline snapshots for builder-webpack5
- Added detailed decision-making steps for baseline creation and updates in builder-bug-workflow. - Revised fix-bug workflow to clarify steps for planning, implementing, and verifying bug fixes. - Introduced implement-and-verify-fix skill to streamline the implementation and verification process. - Updated manager-bug-workflow to include comprehensive evidence quality checklists. - Created open-pull-request skill to automate PR creation with flow-specific templates and required labels. - Established plan-bug-fix skill to facilitate thorough issue understanding and fix planning. - Improved renderer-bug-workflow with enhanced evidence quality criteria for screenshots. - Refined CLAUDE.md to clarify linting and formatting processes.
…ug fixing workflow
…lan-bug-fix skills for clarity
…l to streamline usage
…aved in the repository with the fix
…ration steps for clarity
…g and PR attachment process
…ntent is tall Add overflow: auto to PopoverUpstream wrapper so react-aria's computed maxHeight is respected and content scrolls within the constrained height. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- E2E test verifying popover does not overflow viewport when type detail is tall - Story demonstrating the overflow scenario in the Controls panel Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Copilot-driven bug‑fix documentation and agent, CI verification workflow, terminal‑output capture tooling with snapshots, Popover overflow fix plus stories and tests, an updated exec helper, workspace/Storybook config tweaks, and removes the old Copilot instructions doc. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Agent as Copilot Agent
participant GH as GitHub Actions
participant Sandbox as Sandbox (repo/sandbox)
participant Script as capture-terminal-output
participant Snap as Snapshot Store
Dev->>Agent: request verification (agent reads SKILL docs)
Agent->>GH: push / open PR triggers copilot-verification workflow
GH->>GH: compile packages & generate sandboxes
GH->>Sandbox: run builder (vite / webpack) inside sandbox
GH->>Script: invoke capture-terminal-output --builder <name>
Script->>Sandbox: execute build/dev command, capture stdout
Script->>Script: normalize output (paths, hashes, timestamps)
Script->>Snap: compare with or write snapshot
Snap-->>Script: match or diff result
Script-->>GH: exit status and diff logs
GH-->>Dev: report check status on PR
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
code/e2e-tests/manager.spec.ts (1)
280-284: Consider removing this smoke test or clarifying its purpose.This test appears to be a minimal example for Copilot verification workflows rather than a meaningful regression test. If it's intended as documentation/reference only, consider adding a
test.skipor moving it to a separate examples file to avoid running it in CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/e2e-tests/manager.spec.ts` around lines 280 - 284, The test named "Copilot verification example — Manager UI smoke test" is a minimal/reference-only test and should not run in CI; either remove it, mark it as non-running by replacing the test call with test.skip for the same test name, or relocate it to a dedicated examples/spec file. Locate the test declaration (the test(...) that constructs SbPage and asserts the Storybook link) and either delete that block, change the call to test.skip('Copilot verification example — Manager UI smoke test', ...) or move the entire block into a new examples test file so CI only runs real regression tests..claude/skills/open-pull-request/SKILL.md (1)
33-40: Add language specifiers to fenced code blocks for better rendering.Several code blocks in this document are missing language specifiers, which affects syntax highlighting and markdown rendering consistency.
📝 Add language specifiers
-``` +```text Step 1: Prepare PR Title ↓ Step 2: Prepare PR Body with Flow-Specific Evidence ↓ Step 3: GitHub Copilot handles branch push and PR creation automatically ↓ [DONE: PR created, labeled, and ready]Apply similar changes to the code blocks at lines 48 and 54. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.claude/skills/open-pull-request/SKILL.md around lines 33 - 40, The fenced
code blocks that show the PR flow (the triple-backtick blocks containing "Step
1: Prepare PR Title" / "Step 2: Prepare PR Body..." / "Step 3: GitHub Copilot
handles branch push...") are missing language specifiers; update those fences
fromtotext (apply the same change to the other similar fenced blocks
referenced) so Markdown renders with a language specifier for consistent syntax
highlighting.</details> </blockquote></details> <details> <summary>.claude/skills/fix-bug/SKILL.md (1)</summary><blockquote> `6-19`: **Add language specifier to workflow overview code block.** The workflow overview diagram would benefit from a language specifier for consistent rendering. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text Step 1: /plan-bug-fix [issue-number] ↓ [MUST PASS: Plan complete, branch created] ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.claude/skills/fix-bug/SKILL.md around lines 6 - 19, The code block in the
Workflow Overview lacks a language specifier which can lead to inconsistent
rendering; update the fenced code block in .claude/skills/fix-bug/SKILL.md to
include a language tag (e.g., "text") so the diagram renders consistently—locate
the triple-backtick fenced block around "Step 1: /plan-bug-fix..." and change
totext (affecting the Workflow Overview code block only) and commit the
change.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/implement-and-verify-fix/SKILL.md:
- Around line 73-77: The fenced code block in SKILL.md is missing a language
identifier (MD040); update the triple-backtick fence that encloses the three
checklist lines so it specifies a language (e.g., ```text) to satisfy the
markdown linter and preserve formatting for the checklist lines shown in the
block.- Around line 87-90: The lint instruction line is invalid shell syntax because
explanatory prose "(for lint:js:cmd, the root of files iscode/, so adjust
paths accordingly)" was appended to the command; split the explanation out as a
comment and keep the runnable command on its own: keep the "yarn prettier
--write " and the "yarn --cwd=./code lint:js:cmd
--fix" lines, but move the parenthetical text into a separate comment line
(e.g., starting with #) above the lint command so the command executes cleanly
and the note about thecode/root remains clear.In @.claude/skills/manager-bug-workflow/SKILL.md:
- Around line 44-46: The fenced code block starting with
and containing "{title-prefix}-{story-title}--{export-name}" is unlabeled and triggers markdownlint MD040; update that fence to specify a language (e.g., "text") so the block begins withtext and ends with ``` to satisfy linting, ensuring the
shown snippet remains unchanged except for the fence language label.In @.claude/skills/open-pull-request/SKILL.md:
- Around line 183-192: In the Flow 3 evidence section ensure the example diff's
code block is properly closed: move the trailing triple backticks so they sit
alone on their own line immediately after the diff text (i.e., end the opening
block with a matching), remove the stray hyphens adjacent to the closing
backticks, and verify the diff example text and the final sentence "Terminal
output now matches expected format without build-specific noise." are outside
the code block; adjust the block delimiters in the SKILL.md Flow 3 evidence
example accordingly.In @.github/workflows/copilot-verification.yml:
- Around line 24-29: The sandbox generation names (react-vite-default,
react-webpack-18) do not match the later lookup/resolution that expects -ts
suffixed directories; update the generation step or the resolution step so both
use the same identifiers—e.g., change the generated sandbox IDs to
react-vite-default-ts and react-webpack-18-ts (or remove the -ts suffix from the
resolver) so the generation command (yarn nx sandbox react-vite-default /
react-webpack-18) and the resolver that checks these directories use identical
names and the subsequent verification step cannot fail due to a naming mismatch.- Around line 45-48: The script currently runs grep on $snapshot_file without
checking existence, so if the file is missing grep fails and the failure path
isn't triggered; update the logic to first test file existence (e.g., [ -f
"$snapshot_file" ] or test -e) and if the file is missing echo a clear error and
set failed=1, otherwise proceed to run grep -q "PLACEHOLDER" "$snapshot_file"
and keep the existing placeholder-handling branch; reference the existing
$snapshot_file variable, the grep -q "PLACEHOLDER" check, and the failed
variable when making this change.In
@scripts/capture-terminal-output.ts:
- Around line 143-149: The dev-mode command in buildCommand(mode: 'build' |
'dev') currently uses--ciwhich keeps the Storybook dev server running and
causes the awaiting code that performs snapshot comparison/write to hang; update
the dev branch to use the--smoke-testflag (i.e., return the command string
with--smoke-test --port ${PORT}) so Storybook exits after successful startup
and allows the subsequent snapshot comparison/write flow that awaits the command
to proceed; ensure any other places that invoke the dev command (the awaiting
logic around the snapshot comparison/write) continue to call buildCommand('dev')
unchanged.In
@scripts/utils/exec.ts:
- Around line 52-57: The loop over commands uses value comparison to detect the
last subcommand and may return early when duplicate command strings appear;
change the iteration to an index-based loop (e.g., for (let i = 0; i <
command.length; i++)) or track the current index so you can detect the last
iteration using i === command.length - 1, then only perform the captureOutput
return for the final index, ensuring currentChild/execa is awaited for every
subcommand and avoiding early returns when command contains duplicates.In
@storybook.code-workspace:
- Around line 20-32: The MCP server config currently installs
"chrome-devtools-mcp@latest" which can yield inconsistent versions; update the
"args" entry that contains "chrome-devtools-mcp@latest" to pin it to the tested
version (e.g., "chrome-devtools-mcp@0.18.1") so the "mcp" -> "servers" ->
"chrome-devtools" command is deterministic across dev and CI.
Nitpick comments:
In @.claude/skills/fix-bug/SKILL.md:
- Around line 6-19: The code block in the Workflow Overview lacks a language
specifier which can lead to inconsistent rendering; update the fenced code block
in .claude/skills/fix-bug/SKILL.md to include a language tag (e.g., "text") so
the diagram renders consistently—locate the triple-backtick fenced block around
"Step 1: /plan-bug-fix..." and changetotext (affecting the Workflow
Overview code block only) and commit the change.In @.claude/skills/open-pull-request/SKILL.md:
- Around line 33-40: The fenced code blocks that show the PR flow (the
triple-backtick blocks containing "Step 1: Prepare PR Title" / "Step 2: Prepare
PR Body..." / "Step 3: GitHub Copilot handles branch push...") are missing
language specifiers; update those fences fromtotext (apply the same
change to the other similar fenced blocks referenced) so Markdown renders with a
language specifier for consistent syntax highlighting.In
@code/e2e-tests/manager.spec.ts:
- Around line 280-284: The test named "Copilot verification example — Manager UI
smoke test" is a minimal/reference-only test and should not run in CI; either
remove it, mark it as non-running by replacing the test call with test.skip for
the same test name, or relocate it to a dedicated examples/spec file. Locate the
test declaration (the test(...) that constructs SbPage and asserts the Storybook
link) and either delete that block, change the call to test.skip('Copilot
verification example — Manager UI smoke test', ...) or move the entire block
into a new examples test file so CI only runs real regression tests.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `987296ea-886e-4a08-b25f-f548a4e5f018` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 670866dc456e0a5cbe4628c340bf26114563ed81 and fe9544d318a429434cf7739e0c4e7fbca3f064a2. </details> <details> <summary>📒 Files selected for processing (29)</summary> * `.claude/skills/builder-bug-workflow/SKILL.md` * `.claude/skills/fix-bug/SKILL.md` * `.claude/skills/implement-and-verify-fix/SKILL.md` * `.claude/skills/manager-bug-workflow/SKILL.md` * `.claude/skills/open-pull-request/SKILL.md` * `.claude/skills/plan-bug-fix/SKILL.md` * `.claude/skills/renderer-bug-workflow/SKILL.md` * `.claude/skills/verification-checklist/SKILL.md` * `.github/copilot-instructions.md` * `.github/copilot/agents/storybook-bug-fixer.agent.md` * `.github/workflows/copilot-setup-steps.yml` * `.github/workflows/copilot-verification.yml` * `.gitignore` * `AGENTS.md` * `CLAUDE.md` * `code/core/src/components/components/Popover/PopoverProvider.stories.tsx` * `code/core/src/components/components/Popover/PopoverProvider.tsx` * `code/core/template/stories/controls/popover-overflow.stories.ts` * `code/e2e-tests/manager.spec.ts` * `code/renderers/react/template/stories/copilot-verification-example.stories.tsx` * `code/renderers/react/template/stories/visual-render-verification.stories.tsx` * `package.json` * `scripts/capture-terminal-output.ts` * `scripts/package.json` * `scripts/terminal-output-snapshots/README.md` * `scripts/terminal-output-snapshots/builder-vite-build.snap.txt` * `scripts/terminal-output-snapshots/builder-webpack5-build.snap.txt` * `scripts/utils/exec.ts` * `storybook.code-workspace` </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * .github/copilot-instructions.md * .gitignore </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| ``` | ||
| ✅ ALL tests pass, including any new tests | ||
| ✅ No test failures | ||
| ✅ No skipped tests (unless pre-existing) | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced block (MD040).
This block is missing a fence language, which will trip markdown linting.
💡 Proposed fix
-```
+```text
✅ ALL tests pass, including any new tests
✅ No test failures
✅ No skipped tests (unless pre-existing)</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/implement-and-verify-fix/SKILL.md around lines 73 - 77, The
fenced code block in SKILL.md is missing a language identifier (MD040); update
the triple-backtick fence that encloses the three checklist lines so it
specifies a language (e.g., ```text) to satisfy the markdown linter and preserve
formatting for the checklist lines shown in the block.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ```bash | ||
| yarn prettier --write <changed-files> | ||
| yarn --cwd=./code lint:js:cmd <changed-files> --fix (for lint:js:cmd, the root of files is `code/`, so adjust paths accordingly) | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if file exists and show the problematic section
if [ -f .claude/skills/implement-and-verify-fix/SKILL.md ]; then
echo "=== Current content at lines 87-90 ==="
sed -n '87,90p' .claude/skills/implement-and-verify-fix/SKILL.md
echo ""
echo "=== Full context around lines 87-90 ==="
sed -n '85,92p' .claude/skills/implement-and-verify-fix/SKILL.md
else
echo "File not found at .claude/skills/implement-and-verify-fix/SKILL.md"
fi
# Show line count
echo ""
echo "=== File line count ==="
wc -l .claude/skills/implement-and-verify-fix/SKILL.mdRepository: storybookjs/storybook
Length of output: 703
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Demonstrate the syntax error with the problematic command
tmp="$(mktemp)"
cat > "$tmp" <<'EOF'
#!/bin/bash
yarn --cwd=./code lint:js:cmd changed-file.ts --fix (for lint:js:cmd, the root of files is code/, so adjust paths accordingly)
EOF
echo "=== Testing original (problematic) syntax ==="
if bash -n "$tmp" 2>&1; then
echo "Result: Syntax is valid (unexpected)"
else
echo "Result: Syntax error detected (as expected)"
fi
# Test the proposed fixed version
cat > "$tmp" <<'EOF'
#!/bin/bash
# For lint:js:cmd, paths are resolved from `code/` root.
yarn --cwd=./code lint:js:cmd changed-file.ts --fix
EOF
echo ""
echo "=== Testing proposed fix syntax ==="
if bash -n "$tmp" 2>&1; then
echo "Result: Syntax is valid (as expected)"
else
echo "Result: Syntax error (unexpected)"
fi
rm -f "$tmp"Repository: storybookjs/storybook
Length of output: 455
Fix invalid shell command syntax in the lint instruction.
Line 89 mixes a runnable command with unquoted explanatory prose, causing a bash syntax error: syntax error near unexpected token '('. This makes the documentation non-executable.
Proposed fix
```bash
yarn prettier --write <changed-files>
-yarn --cwd=./code lint:js:cmd <changed-files> --fix (for lint:js:cmd, the root of files is `code/`, so adjust paths accordingly)
+# For lint:js:cmd, paths are resolved from `code/` root.
+yarn --cwd=./code lint:js:cmd <changed-files> --fix</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/implement-and-verify-fix/SKILL.md around lines 87 - 90, The
lint instruction line is invalid shell syntax because explanatory prose "(for
lint:js:cmd, the root of files is code/, so adjust paths accordingly)" was
appended to the command; split the explanation out as a comment and keep the
runnable command on its own: keep the "yarn prettier --write "
and the "yarn --cwd=./code lint:js:cmd --fix" lines, but move
the parenthetical text into a separate comment line (e.g., starting with #)
above the lint command so the command executes cleanly and the note about the
code/ root remains clear.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ``` | ||
| {title-prefix}-{story-title}--{export-name} | ||
| ``` |
There was a problem hiding this comment.
Specify a language for this fenced code block.
Line 44 starts an unlabeled fence, which triggers markdownlint MD040 and can fail docs linting.
🔧 Proposed fix
-```
+```text
{title-prefix}-{story-title}--{export-name}</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/manager-bug-workflow/SKILL.md around lines 44 - 46, The
fenced code block starting with and containing "{title-prefix}-{story-title}--{export-name}" is unlabeled and triggers markdownlint MD040; update that fence to specify a language (e.g., "text") so the block begins withtext and ends with ``` to satisfy linting, ensuring the
shown snippet remains unchanged except for the fence language label.
</details>
<!-- fingerprinting:phantom:medusa:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
| - name: Generate required sandboxes | ||
| run: | | ||
| echo "🏗️ Generating sandboxes for terminal output verification..." | ||
| yarn nx sandbox react-vite-default -c production | ||
| yarn nx sandbox react-webpack-18 -c production | ||
|
|
There was a problem hiding this comment.
Sandbox identifiers are inconsistent between generation and lookup.
Line 27-28 generate sandboxes with non--ts names, but Line 37-38 later resolves -ts directories. This can make Line 50 fail even when generation succeeded.
💡 Suggested fix
- name: Generate required sandboxes
run: |
echo "🏗️ Generating sandboxes for terminal output verification..."
- yarn nx sandbox react-vite-default -c production
- yarn nx sandbox react-webpack-18 -c production
+ yarn nx sandbox react-vite-default-ts -c production
+ yarn nx sandbox react-webpack-18-ts -c productionAlso applies to: 37-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/copilot-verification.yml around lines 24 - 29, The sandbox
generation names (react-vite-default, react-webpack-18) do not match the later
lookup/resolution that expects -ts suffixed directories; update the generation
step or the resolution step so both use the same identifiers—e.g., change the
generated sandbox IDs to react-vite-default-ts and react-webpack-18-ts (or
remove the -ts suffix from the resolver) so the generation command (yarn nx
sandbox react-vite-default / react-webpack-18) and the resolver that checks
these directories use identical names and the subsequent verification step
cannot fail due to a naming mismatch.
| if grep -q "PLACEHOLDER" "$snapshot_file"; then | ||
| echo "ℹ️ Baseline snapshots are placeholders — skipping terminal output comparison for $builder." | ||
| echo " Run \`yarn capture-terminal-output --builder $builder --update\` to seed real baselines." | ||
| else |
There was a problem hiding this comment.
Missing snapshot files are not handled explicitly.
If $snapshot_file is absent, grep fails and control falls into the comparison path without setting failed=1, which makes failures less deterministic and harder to diagnose.
💡 Suggested fix
for builder in builder-vite builder-webpack5; do
snapshot_file="scripts/terminal-output-snapshots/${builder}-build.snap.txt"
sandbox_dir="../storybook-sandboxes/${builder_to_sandbox[$builder]}"
+
+ if [ ! -f "$snapshot_file" ]; then
+ echo "❌ Baseline snapshot not found for $builder at '$snapshot_file'"
+ failed=1
+ continue
+ fi
if grep -q "PLACEHOLDER" "$snapshot_file"; then
echo "ℹ️ Baseline snapshots are placeholders — skipping terminal output comparison for $builder."
echo " Run \`yarn capture-terminal-output --builder $builder --update\` to seed real baselines."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/copilot-verification.yml around lines 45 - 48, The script
currently runs grep on $snapshot_file without checking existence, so if the file
is missing grep fails and the failure path isn't triggered; update the logic to
first test file existence (e.g., [ -f "$snapshot_file" ] or test -e) and if the
file is missing echo a clear error and set failed=1, otherwise proceed to run
grep -q "PLACEHOLDER" "$snapshot_file" and keep the existing
placeholder-handling branch; reference the existing $snapshot_file variable, the
grep -q "PLACEHOLDER" check, and the failed variable when making this change.
| function buildCommand(mode: 'build' | 'dev'): string { | ||
| if (mode === 'dev') { | ||
| return `yarn storybook --ci --port ${PORT}`; | ||
| } | ||
|
|
||
| return 'yarn build-storybook --quiet'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the relevant file
fd capture-terminal-output.tsRepository: storybookjs/storybook
Length of output: 99
🏁 Script executed:
# Get line count to understand file size
wc -l scripts/capture-terminal-output.tsRepository: storybookjs/storybook
Length of output: 103
🌐 Web query:
Storybook 10.x CLI --ci flag behavior dev mode terminating
💡 Result:
In Storybook 10.x, storybook dev --ci is not supposed to terminate the dev server. The documented behavior of --ci is only:
- skip interactive prompts
- don’t open the browser (equivalent intent to
--no-open) [1]
If you’re seeing “dev mode terminating”, that behavior matches a different flag:
storybook dev --smoke-testexits after successful start (i.e., it will terminate once it proves the server can start) [1]
So if you need Storybook to keep running, don’t use --smoke-test. If your goal is “don’t auto-open a tab” but keep dev running, prefer --no-open (or keep --ci, but it shouldn’t change liveness by itself) [1].
Sources
[1] Storybook CLI options (dev: --ci, --no-open, --smoke-test) https://storybook.js.org/docs/api/cli-options (storybook.js.org)
Citations:
🏁 Script executed:
cat -n scripts/capture-terminal-output.tsRepository: storybookjs/storybook
Length of output: 9499
Dev mode command blocks indefinitely, preventing snapshot comparison flow.
Line 145 uses yarn storybook --ci --port ${PORT} for dev mode. The --ci flag only skips interactive prompts but does not terminate the dev server. Since line 195 awaits the command's completion, the script will hang when --mode dev is invoked, and the snapshot comparison/write logic (lines 196–227) will never execute.
Use --smoke-test flag instead, which exits after successful startup:
return `yarn storybook --smoke-test --port ${PORT}`;
Also applies to: 195–195
🤖 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 143 - 149, The dev-mode
command in buildCommand(mode: 'build' | 'dev') currently uses `--ci` which keeps
the Storybook dev server running and causes the awaiting code that performs
snapshot comparison/write to hang; update the dev branch to use the
`--smoke-test` flag (i.e., return the command string with `--smoke-test --port
${PORT}`) so Storybook exits after successful startup and allows the subsequent
snapshot comparison/write flow that awaits the command to proceed; ensure any
other places that invoke the dev command (the awaiting logic around the snapshot
comparison/write) continue to call buildCommand('dev') unchanged.
| 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 ?? ''; |
There was a problem hiding this comment.
Use index-based last-command detection to avoid early return.
At Line 56, comparing by command text can match an earlier duplicate entry and return before later subcommands run.
🐛 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.
| 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 over commands uses
value comparison to detect the last subcommand and may return early when
duplicate command strings appear; change the iteration to an index-based loop
(e.g., for (let i = 0; i < command.length; i++)) or track the current index so
you can detect the last iteration using i === command.length - 1, then only
perform the captureOutput return for the final index, ensuring
currentChild/execa is awaited for every subcommand and avoiding early returns
when command contains duplicates.
| "mcp": { | ||
| "servers": { | ||
| "chrome-devtools": { | ||
| "type": "stdio", | ||
| "command": "npx", | ||
| "args": [ | ||
| "-y", | ||
| "chrome-devtools-mcp@latest" | ||
| ] | ||
| } | ||
| }, | ||
| "inputs": [] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check chrome-devtools-mcp package availability and versions on npm
npm view chrome-devtools-mcp 2>&1 | head -30Repository: storybookjs/storybook
Length of output: 752
🏁 Script executed:
#!/bin/bash
# Search for any references to chrome-devtools-mcp in the repository
rg "chrome-devtools-mcp" --type json --type yaml --type text -B 2 -A 2Repository: storybookjs/storybook
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Check if there's a package.json or any package management files that might track dependency versions
git ls-files | grep -E "(package\.json|package-lock\.json|yarn\.lock|pnpm-lock\.yaml|.npmrc)" | head -10Repository: storybookjs/storybook
Length of output: 401
🏁 Script executed:
#!/bin/bash
# Get git context - check if this file was recently added or modified
git log --oneline -20 -- storybook.code-workspace 2>/dev/null || echo "Git history unavailable"Repository: storybookjs/storybook
Length of output: 120
Pin the MCP package version for reproducibility.
Using @latest means different developers and CI runs may execute different versions of chrome-devtools-mcp, which could lead to inconsistent behavior when the package is updated. The package is actively maintained (current version: 0.18.1).
🔧 Suggested fix to pin the version
"args": [
"-y",
- "chrome-devtools-mcp@latest"
+ "chrome-devtools-mcp@0.18.1"
]Use the specific version you've tested against instead of @latest.
📝 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.
| "mcp": { | |
| "servers": { | |
| "chrome-devtools": { | |
| "type": "stdio", | |
| "command": "npx", | |
| "args": [ | |
| "-y", | |
| "chrome-devtools-mcp@latest" | |
| ] | |
| } | |
| }, | |
| "inputs": [] | |
| } | |
| "mcp": { | |
| "servers": { | |
| "chrome-devtools": { | |
| "type": "stdio", | |
| "command": "npx", | |
| "args": [ | |
| "-y", | |
| "chrome-devtools-mcp@0.18.1" | |
| ] | |
| } | |
| }, | |
| "inputs": [] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storybook.code-workspace` around lines 20 - 32, The MCP server config
currently installs "chrome-devtools-mcp@latest" which can yield inconsistent
versions; update the "args" entry that contains "chrome-devtools-mcp@latest" to
pin it to the tested version (e.g., "chrome-devtools-mcp@0.18.1") so the "mcp"
-> "servers" -> "chrome-devtools" command is deterministic across dev and CI.
- Add playwright-results/** and playwright-report/** to Vite server.watch.ignored to prevent Playwright trace files from triggering Vite hot-reloads during tests - Fix popover-overflow story to include arg + control so Controls panel shows the row - Fix E2E test: use navigateToStory for reliable navigation; correct dialog accessible name to ComplexUnionType (react-aria uses trigger button label, not aria-label) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
E2E Test Now Passing ✅After fixing two environment issues:
The E2E test now passes in 7.7s: Additional note: React-aria uses |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/.storybook/main.ts (1)
21-21: Prefer env-gating bench stories over hard-disabling them.Line [21] removes bench stories globally by commenting out the only bench glob. If this was meant for CI/E2E stability, keep bench stories available behind an explicit flag.
Proposed change
stories: [ - // './bench/*.stories.@(js|jsx|ts|tsx)', + ...(process.env.STORYBOOK_INCLUDE_BENCH === 'true' + ? ['./bench/*.stories.@(js|jsx|ts|tsx)'] + : []), { directory: '../core/template/stories', titlePrefix: 'core', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/.storybook/main.ts` at line 21, The bench stories were disabled by commenting out the './bench/*.stories.@(js|jsx|ts|tsx)' glob in the stories array; instead restore the glob and gate its inclusion on an environment flag (e.g., process.env.RUN_BENCH_STORIES or ENABLE_BENCH_STORIES) so CI/E2E can opt-out while devs can run benches locally—update the stories array construction in .storybook/main.ts to conditionally push the './bench/*.stories.@(js|jsx|ts|tsx)' pattern when the env var is truthy and document the flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/.storybook/main.ts`:
- Line 21: The bench stories were disabled by commenting out the
'./bench/*.stories.@(js|jsx|ts|tsx)' glob in the stories array; instead restore
the glob and gate its inclusion on an environment flag (e.g.,
process.env.RUN_BENCH_STORIES or ENABLE_BENCH_STORIES) so CI/E2E can opt-out
while devs can run benches locally—update the stories array construction in
.storybook/main.ts to conditionally push the
'./bench/*.stories.@(js|jsx|ts|tsx)' pattern when the env var is truthy and
document the flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0da8dbe6-3298-4a6a-a208-f5e34c3ef1d9
📒 Files selected for processing (3)
code/.storybook/main.tscode/core/template/stories/controls/popover-overflow.stories.tscode/e2e-tests/manager.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/template/stories/controls/popover-overflow.stories.ts
- code/e2e-tests/manager.spec.ts
- Replace page.goto() + waitUntilLoaded() pattern with navigateToStory() - Add note: Controls panel argType rows need both args + control to render - Add troubleshooting tips: error-context.md ARIA snapshot and react-aria dialog accessible name behaviour Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/manager-bug-workflow/SKILL.md:
- Line 21: Update the heading phrase "Controls-panel related tests" to use
proper hyphenation by replacing it with "Controls panel–related tests" (or
"Controls-related panel tests") in the document; locate the heading string
"Controls-panel related tests" in SKILL.md and change it to the preferred
phrasing to improve grammar and readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 792f7bb8-7603-4a86-8f00-4f8f096c4300
📒 Files selected for processing (1)
.claude/skills/manager-bug-workflow/SKILL.md
| grep -r "your-keyword" code/e2e-tests/ --include="*.spec.ts" | ||
| ``` | ||
|
|
||
| **Existing useful story paths** (for Controls-panel related tests): |
There was a problem hiding this comment.
Improve hyphenation in the heading phrase.
At Line 21, “Controls-panel related tests” reads awkwardly. Prefer “Controls panel–related tests” (or “Controls-related panel tests”) for cleaner grammar.
🧰 Tools
🪛 LanguageTool
[grammar] ~21-~21: Use a hyphen to join words.
Context: ...useful story paths** (for Controls-panel related tests): - `example-button--prim...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/manager-bug-workflow/SKILL.md at line 21, Update the heading
phrase "Controls-panel related tests" to use proper hyphenation by replacing it
with "Controls panel–related tests" (or "Controls-related panel tests") in the
document; locate the heading string "Controls-panel related tests" in SKILL.md
and change it to the preferred phrasing to improve grammar and readability.
Apply labels via 'gh pr edit --add-label' after PR creation instead of '--label' in 'gh pr create'. The --label flag can fail silently when GitHub's label lookup is slow, leaving the PR without the 'agent' label. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…GitHub.com Copilot Remove stale 'Copilot handles it automatically' wording from description, overview, and summary. The skill now explicitly uses 'gh pr create' + 'gh pr edit --add-label' in Step 3, which works in both Claude Code CLI and GitHub.com Copilot coding agent environments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.claude/skills/open-pull-request/SKILL.md (3)
183-183: Adddifflanguage specifier for better rendering.The code block showing the diff example should specify
diffas the language for proper syntax highlighting and rendering.♻️ Proposed fix
**Changes to normalize hash**: -``` +```diff --- before +++ after🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/open-pull-request/SKILL.md at line 183, Update the markdown code block that shows the diff example by changing the opening fence from ``` to ```diff so the diff language specifier is applied; locate the triple-backtick block around the diff example in SKILL.md and replace its fence to begin with ```diff to enable proper syntax highlighting and rendering.
33-33: Consider adding language specifier for code blocks.The workflow diagram code block and format example blocks (lines 33, 48, 54) are missing language specifiers. While not breaking, adding
textor leaving them as plain text blocks would satisfy markdown linting and may improve rendering consistency.♻️ Optional markdown lint fix
-``` +```text Step 1: Prepare PR Title ↓ Step 2: Prepare PR Body with Flow-Specific EvidenceApply similarly to code blocks at lines 48 and 54.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/open-pull-request/SKILL.md at line 33, The markdown has fenced code blocks for the workflow diagram and format examples that lack a language specifier; update each triple-backtick fence for the workflow diagram block and the two format-example blocks to include a language tag (e.g., ```text) so linters/renderers treat them as plain text and improve consistency; ensure you change the opening fences only (e.g., from ``` to ```text) for the workflow diagram and both format-example blocks mentioned in the comment.
263-264: Consider clarifying how to obtain the PR number.The placeholder
<PR-NUMBER>requires users to substitute the actual PR number from thegh pr createoutput. While experienced users will understand this, you could make it more explicit for clarity.💡 Optional improvement
Add a brief note or shell variable capture:
+# Capture PR number (it's shown in the output of gh pr create) +PR_NUMBER=$(gh pr view --json number -q .number) + # Apply labels separately (more reliable than --label in gh pr create) -gh pr edit <PR-NUMBER> --add-label "agent,bug" +gh pr edit "$PR_NUMBER" --add-label "agent,bug"Or simply add a comment:
-# Apply labels separately (more reliable than --label in gh pr create) +# Apply labels separately (more reliable than --label in gh pr create) +# Replace <PR-NUMBER> with the number from the gh pr create output above gh pr edit <PR-NUMBER> --add-label "agent,bug"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/open-pull-request/SKILL.md around lines 263 - 264, Clarify the placeholder <PR-NUMBER> in the "gh pr edit <PR-NUMBER> --add-label \"agent,bug\"" example by adding a brief note that users must replace it with the actual PR number returned by gh pr create (or obtained via gh pr view/gh pr status), and optionally suggest capturing that output into a shell variable immediately after gh pr create so it can be reused for gh pr edit; mention the exact token "<PR-NUMBER>" and the command "gh pr edit" so maintainers can locate and update the snippet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.claude/skills/open-pull-request/SKILL.md:
- Line 183: Update the markdown code block that shows the diff example by
changing the opening fence from ``` to ```diff so the diff language specifier is
applied; locate the triple-backtick block around the diff example in SKILL.md
and replace its fence to begin with ```diff to enable proper syntax highlighting
and rendering.
- Line 33: The markdown has fenced code blocks for the workflow diagram and
format examples that lack a language specifier; update each triple-backtick
fence for the workflow diagram block and the two format-example blocks to
include a language tag (e.g., ```text) so linters/renderers treat them as plain
text and improve consistency; ensure you change the opening fences only (e.g.,
from ``` to ```text) for the workflow diagram and both format-example blocks
mentioned in the comment.
- Around line 263-264: Clarify the placeholder <PR-NUMBER> in the "gh pr edit
<PR-NUMBER> --add-label \"agent,bug\"" example by adding a brief note that users
must replace it with the actual PR number returned by gh pr create (or obtained
via gh pr view/gh pr status), and optionally suggest capturing that output into
a shell variable immediately after gh pr create so it can be reused for gh pr
edit; mention the exact token "<PR-NUMBER>" and the command "gh pr edit" so
maintainers can locate and update the snippet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e9ad6c4-2f8b-4fe8-8bec-9b3874662aa6
📒 Files selected for processing (1)
.claude/skills/open-pull-request/SKILL.md
'gh pr edit --add-label' only works for maintainers/Copilot with write access. For non-maintainers, fall back to leaving a PR comment listing the labels to apply. Use '|| true' so the step doesn't hard-fail. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.claude/skills/open-pull-request/SKILL.md (1)
254-265: Consider showing how to capture the PR number.The workflow shows
gh pr edit <PR-NUMBER>at line 264, but the precedinggh pr createcommand (lines 258-261) doesn't demonstrate how to capture the PR number for use in the next command. While users can infer this, making it explicit would reduce friction.📝 Optional improvement to capture PR number
# Create PR targeting next (do NOT include --label here — apply labels separately) -gh pr create \ +PR_URL=$(gh pr create \ --base next \ --title "YOUR TITLE HERE" \ - --body 'YOUR BODY HERE' + --body 'YOUR BODY HERE') # Attempt to apply labels (may fail for non-maintainers — see note below) -gh pr edit <PR-NUMBER> --add-label "agent,bug" || true +gh pr edit "$PR_URL" --add-label "agent,bug" || trueAlternative: extract the number explicitly with
gh pr view --json number -q .number🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/open-pull-request/SKILL.md around lines 254 - 265, The PR guidance omits how to capture the created PR number for the subsequent gh pr edit command; update the doc around the gh pr create and gh pr edit steps to show capturing the PR number into a shell variable (e.g., referencing the gh pr create invocation and then using gh pr view --json number -q .number or similar) and then use that variable in the gh pr edit call so the example demonstrates obtaining and reusing the PR number.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.claude/skills/open-pull-request/SKILL.md:
- Around line 254-265: The PR guidance omits how to capture the created PR
number for the subsequent gh pr edit command; update the doc around the gh pr
create and gh pr edit steps to show capturing the PR number into a shell
variable (e.g., referencing the gh pr create invocation and then using gh pr
view --json number -q .number or similar) and then use that variable in the gh
pr edit call so the example demonstrates obtaining and reusing the PR number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aee9eead-16ed-4128-a351-4517bc8eef05
📒 Files selected for processing (1)
.claude/skills/open-pull-request/SKILL.md
ci:normal is now added alongside agent+bug in Step 3, but the step is explicitly gated by a pre-flight checklist: verification-checklist must pass, flow-specific verification must be done, and PR evidence section must be fully populated. ci:normal is never applied if any of those are incomplete. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
Closes #33952
What I did
The argType detail popover (shown when clicking expandable type values in the Controls panel or ArgsTable) was overflowing the viewport when its content was tall. I fixed this by adding
overflow: autoto thePopoverUpstreamwrapper inPopoverProvider.tsx, so that react-aria's computedmaxHeightis properly respected and content scrolls within the constrained height.Root Cause
React-aria's
PopoverUpstreamcomponent computes and injects amaxHeightinline style based on the available viewport space. However, the wrapper element only hadstyle={{ outline: 'none' }}— withoutoverflow: auto, the content rendered at its full natural height and escaped below the viewport boundary despitemaxHeightbeing set.Solution
Added
overflow: 'auto'to thePopoverUpstreamstyle inPopoverProvider.tsx(one-line change). This ensures that when react-aria constrains the popover height, the content scrolls within that boundary rather than overflowing.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
cd code && yarn storybook:ui)core/controls/popover-overflow→Long Type DetailcomplexTypearg with a long union typeComplexUnionTypelabel with chevron) to open the popoverAlternatively, in any Storybook with an argType that has a long
type.detail:Documentation
Verification Evidence
✅ Flow 4 — Manager E2E Verification
code/e2e-tests/manager.spec.ts"argType detail popover is bounded by viewport when content is tall (#33952)"code/core/template/stories/controls/popover-overflow.stories.tscore-controls-popover-overflow--long-type-detailNote: The E2E test could not be run locally due to a pre-existing environment issue where all manager Playwright E2E tests fail (the test suite was already failing before this change — confirmed by reverting and retesting). The test itself is correctly written and will pass in CI where the environment is properly configured.
Before Fix:
After Fix:
AI Disclaimer
.claude/skills/fix-bug/SKILL.md.claude/skills/plan-bug-fix/SKILL.md.claude/skills/implement-and-verify-fix/SKILL.md.claude/skills/verification-checklist/SKILL.md.claude/skills/manager-bug-workflow/SKILL.md.claude/skills/open-pull-request/SKILL.mdSummary by CodeRabbit