CLI: Show multiple favicons warning as debug message#34069
CLI: Show multiple favicons warning as debug message#34069remino wants to merge 2 commits intostorybookjs:nextfrom
Conversation
A logger.warn() message was displayed before. Changing to logger.debug().
📝 WalkthroughWalkthroughChanges demote favicon detection logging from warning to debug level in both source code and corresponding test file. No functional logic is altered; only the severity of logged messages when multiple favicons are detected is adjusted. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/core-server/presets/favicon.test.ts (1)
49-55: Consider usingspy: trueoption for the logger mock.The mock pattern here doesn't use the
{ spy: true }option, which is recommended by the coding guidelines for Vitest mocks. While this is a pre-existing pattern not introduced in this PR, consider aligning with the guideline for consistency.Additionally, the mock only exposes
debugnow—if future tests need other logger methods, they'd need to be added. This is fine for the current scope.♻️ Optional: Align with spy mocking guidelines
-vi.mock('storybook/internal/node-logger', () => { - return { - logger: { - debug: vi.fn(() => {}), - }, - }; -}); +vi.mock('storybook/internal/node-logger', { spy: true });Then access the mocked logger in tests:
import { logger } from 'storybook/internal/node-logger'; // ... expect(vi.mocked(logger.debug)).toHaveBeenCalledWith(expect.stringContaining('multiple favicons'));As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/presets/favicon.test.ts` around lines 49 - 55, Update the Vitest mock of 'storybook/internal/node-logger' to use the spy option by calling vi.mock(..., { spy: true }) while keeping the returned shape (logger.debug) so tests continue to run; then update tests to assert via vi.mocked(logger.debug) rather than raw function references (importing logger from 'storybook/internal/node-logger' and using vi.mocked(logger.debug) in expectations).
🤖 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/core/src/core-server/presets/favicon.test.ts`:
- Around line 49-55: Update the Vitest mock of 'storybook/internal/node-logger'
to use the spy option by calling vi.mock(..., { spy: true }) while keeping the
returned shape (logger.debug) so tests continue to run; then update tests to
assert via vi.mocked(logger.debug) rather than raw function references
(importing logger from 'storybook/internal/node-logger' and using
vi.mocked(logger.debug) in expectations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5eb5f89-fbfe-4c57-beb2-ffe73ae3f025
📒 Files selected for processing (2)
code/core/src/core-server/presets/common-preset.tscode/core/src/core-server/presets/favicon.test.ts
Sidnioulz
changed the title
|
Not sure what’s going on with the failed tests here. I haven’t touched those parts. Maybe they were failing before? |
Yeah. The Linux tests job is flaky and the UI tests has a failure right now. |
2 participants
Related to Discord message:
https://discord.com/channels/486522875931656193/1106615915321229372/1479432853287731220
No issue was filed.
What I did
A logger.warn() message was displayed before. Changing to logger.debug().
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Build a sandbox:
yarn task --task sandbox --start-from auto --template react-vite/default-tsGo into the directory of the sandbox:
cd ../storybook-sandboxes/react-vite-default-tsInstall packages:
yarnAdd favicons:
touch public/favicon.{ico,svg}Set
staticDirsin.storybook/main.ts:export default defineMain({ + staticDirs: ['../public’],Add
--loglevel debugtostorybooktask inpackage.json:Run Storybook:
yarn storybookDebug message should appear:
[DEBUG] Looks like multiple favicons were detected. Using the first one.Documentation
N/A. I couldn’t find any mention of warning messages for multiple favicons detected.
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake 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
Summary by CodeRabbit