Skip to content

CLI: Update postAction hook to use command parameter for logfile retrieval#33137

Merged
valentinpalkovic merged 1 commit into
nextfrom
valentin/fix-commander-this-reference
Nov 24, 2025
Merged

CLI: Update postAction hook to use command parameter for logfile retrieval#33137
valentinpalkovic merged 1 commit into
nextfrom
valentin/fix-commander-this-reference

Conversation

@valentinpalkovic
Copy link
Copy Markdown
Contributor

@valentinpalkovic valentinpalkovic commented Nov 24, 2025

Closes #

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-33137-sha-586e3337. Try it out in a new sandbox by running npx storybook@0.0.0-pr-33137-sha-586e3337 sandbox or in an existing project with npx storybook@0.0.0-pr-33137-sha-586e3337 upgrade.

More information
Published version 0.0.0-pr-33137-sha-586e3337
Triggered by @valentinpalkovic
Repository storybookjs/storybook
Branch valentin/fix-commander-this-reference
Commit 586e3337
Datetime Mon Nov 24 10:56:47 UTC 2025 (1763981807)
Workflow run 19631855015

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook publish.yml --field pr=33137

Summary by CodeRabbit

  • Refactor

    • Standardized CLI hook parameter handling so option retrieval uses the command instance, improving consistency across commands.
  • Bug Fixes

    • Restored reliable access to the log-file option in CLI handlers, ensuring log writing behaves as expected.

✏️ Tip: You can customize this high-level summary in your review settings.

@valentinpalkovic valentinpalkovic self-assigned this Nov 24, 2025
@valentinpalkovic valentinpalkovic added bug ci:normal Run our default set of CI jobs (choose this for most PRs). labels Nov 24, 2025
@nx-cloud
Copy link
Copy Markdown

nx-cloud Bot commented Nov 24, 2025

View your CI Pipeline Execution ↗ for commit 6939068

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 46s View ↗

☁️ Nx Cloud last updated this comment at 2025-11-24 12:13:52 UTC

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 24, 2025

📝 Walkthrough

Walkthrough

Two CLI runner scripts were changed to accept the full command object in their postAction hooks instead of a destructured { getOptionValue }, and they now retrieve the logfile option via command.getOptionValue('logfile').

Changes

Cohort / File(s) Change Summary
postAction hook refactor
code/lib/cli-storybook/src/bin/run.ts, code/lib/create-storybook/src/bin/run.ts
Replace destructured { getOptionValue } parameter with command in postAction hooks; use command.getOptionValue('logfile') when checking/writing logs.

Sequence Diagram(s)

sequenceDiagram
    participant CLI as CLI Entrypoint
    participant Commander as Commander (command)
    participant Logger as Log Writer

    CLI->>Commander: execute run command
    Commander-->>CLI: invokes postAction(command)
    alt logfile option set
        CLI->>Commander: command.getOptionValue('logfile')
        Commander-->>CLI: returns logfile path
        CLI->>Logger: write logs to path
        Logger-->>CLI: success/failure
    else logfile not set
        CLI-->>Logger: no-op
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Pay attention to code/lib/create-storybook/src/bin/run.ts for a possible typo (selcommandf or similar) that would cause a runtime error when attempting to access getOptionValue; ensure parameter name matches usage.
  • Verify both files consistently handle the absence of logfile and any error paths from the logger.

Possibly related PRs

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 586e333 and 6939068.

📒 Files selected for processing (2)
  • code/lib/cli-storybook/src/bin/run.ts (1 hunks)
  • code/lib/create-storybook/src/bin/run.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • code/lib/cli-storybook/src/bin/run.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/lib/create-storybook/src/bin/run.ts
🧬 Code graph analysis (1)
code/lib/create-storybook/src/bin/run.ts (1)
code/core/src/node-logger/logger/log-tracker.ts (1)
  • logTracker (95-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: normal
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/lib/create-storybook/src/bin/run.ts (1)

105-109: LGTM! Correct usage of Commander.js hook parameter.

The postAction hook now correctly uses the full command object parameter to retrieve the logfile option. The previous typo has been fixed, and the implementation properly checks shouldWriteLogsToFile before writing logs.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d5af576 and 586e333.

📒 Files selected for processing (2)
  • code/lib/cli-storybook/src/bin/run.ts (1 hunks)
  • code/lib/create-storybook/src/bin/run.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/lib/create-storybook/src/bin/run.ts
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.

Applied to files:

  • code/lib/create-storybook/src/bin/run.ts
🧬 Code graph analysis (1)
code/lib/cli-storybook/src/bin/run.ts (2)
code/core/src/node-logger/logger/log-tracker.ts (1)
  • logTracker (95-95)
code/core/src/node-logger/index.ts (1)
  • logTracker (8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: normal
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/lib/cli-storybook/src/bin/run.ts (1)

80-86: LGTM! Correct refactoring of postAction hook.

The parameter change from destructured { getOptionValue } to command object and the corresponding update to command.getOptionValue('logfile') is correct and aligns with Commander.js hook patterns.

Comment thread code/lib/create-storybook/src/bin/run.ts Outdated
@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-commander-this-reference branch from 586e333 to 6939068 Compare November 24, 2025 11:53
@valentinpalkovic valentinpalkovic merged commit bad5c15 into next Nov 24, 2025
66 of 67 checks passed
@valentinpalkovic valentinpalkovic deleted the valentin/fix-commander-this-reference branch November 24, 2025 12:53
@github-actions github-actions Bot mentioned this pull request Nov 24, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:normal Run our default set of CI jobs (choose this for most PRs).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant