Core: Show "new" status on newly added individual stories#34504
Open
ghengeveld wants to merge 5 commits intonextfrom
Open
Core: Show "new" status on newly added individual stories#34504ghengeveld wants to merge 5 commits intonextfrom
ghengeveld wants to merge 5 commits intonextfrom
Conversation
…ated ChangeDetectionService to utilize IndexBaselineService for marking new stories with the proper status.
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a baseline-index tracking service for change detection, refactors module-graph subscription startup in builder-vite to support post-start listener registration, extends git command capabilities, enhances status merging logic in change detection, gates change-detection startup to event-driven initialization, and updates sidebar status display mapping. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
⚔️ Resolve merge conflicts
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: 2
🧹 Nitpick comments (4)
code/core/src/manager/components/sidebar/Tree.tsx (1)
216-222: Consider exportinggetDisplayStatusfor direct testability.This utility function contains conditional logic that could benefit from unit testing. As per coding guidelines, functions that need direct tests should be exported.
♻️ Suggested change
-const getDisplayStatus = ( +export const getDisplayStatus = ( itemType: Item['type'], status: StatusValue ): { status: StatusValue; label: string } =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/Tree.tsx` around lines 216 - 222, Export the getDisplayStatus utility so it can be imported in unit tests: change its declaration to a named export (e.g., export const getDisplayStatus = ... or export function getDisplayStatus(...)) and update any internal references or imports that rely on the previous non-exported symbol (ensure consuming components still import it by name). Keep the implementation unchanged so tests can directly import getDisplayStatus to assert the conditional mapping for story/docs hidden statuses and the default label formatting.code/core/src/core-server/change-detection/GitDiffProvider.test.ts (1)
148-168: Consider simplifying the nested ternary dispatch.The mock dispatch table is becoming deeply nested (7 levels). While functional, a Map or object lookup would improve readability.
♻️ Optional refactor using a lookup object
- vi.mocked(execa).mockImplementation(((_command: string | URL, ...rest: unknown[]) => { - const args = Array.isArray(rest[0]) ? rest[0] : []; - const gitArgs = args.join(' '); - const result = - gitArgs === 'rev-parse --show-toplevel' - ? repoRootResult - : gitArgs === 'diff --name-only --diff-filter=ad --cached' - ? stagedResult - : gitArgs === 'diff --name-only --diff-filter=ad' - ? unstagedResult - : gitArgs === 'ls-files --others --exclude-standard' - ? untrackedResult - : gitArgs === 'diff --name-only --diff-filter=A --cached' - ? stagedAddedResult - : gitArgs === 'diff --name-only --diff-filter=A' - ? intentToAddResult - : gitArgs === 'rev-parse HEAD' - ? headCommitResult - : gitArgs === 'status --porcelain' - ? statusPorcelainResult - : undefined; + vi.mocked(execa).mockImplementation(((_command: string | URL, ...rest: unknown[]) => { + const args = Array.isArray(rest[0]) ? rest[0] : []; + const gitArgs = args.join(' '); + const resultMap: Record<string, ExecaMockResult | undefined> = { + 'rev-parse --show-toplevel': repoRootResult, + 'diff --name-only --diff-filter=ad --cached': stagedResult, + 'diff --name-only --diff-filter=ad': unstagedResult, + 'ls-files --others --exclude-standard': untrackedResult, + 'diff --name-only --diff-filter=A --cached': stagedAddedResult, + 'diff --name-only --diff-filter=A': intentToAddResult, + 'rev-parse HEAD': headCommitResult, + 'status --porcelain': statusPorcelainResult, + }; + const result = resultMap[gitArgs];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/change-detection/GitDiffProvider.test.ts` around lines 148 - 168, The nested ternary inside the vi.mocked(execa).mockImplementation block (which inspects gitArgs and returns repoRootResult, stagedResult, unstagedResult, untrackedResult, stagedAddedResult, intentToAddResult, headCommitResult, statusPorcelainResult) should be replaced with a clear lookup table: build a Map or plain object keyed by the gitArgs strings and return map.get(gitArgs) (or map[gitArgs]) instead of the long nested ternary; ensure you keep the existing handling for Array.isArray(rest[0]) to derive args and preserve returning undefined for unmatched keys.code/core/src/core-server/change-detection/ChangeDetectionService.ts (2)
391-403: Minor redundancy in status value merging.The
valueis merged twice: first explicitly on line 394 viamergeStatusValues(existingStatus?.value, value), then again insidemergeChangeDetectionStatuseson line 403. SincemergeStatusValuesis idempotent for its priority logic, this works correctly but is slightly redundant.Consider removing the explicit merge on line 394 and letting
mergeChangeDetectionStatuseshandle it:Proposed simplification
const nextStatus: Status = { storyId, typeId: CHANGE_DETECTION_STATUS_TYPE_ID, - value: mergeStatusValues(existingStatus?.value, value), + value, title: '', description: '', data: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/change-detection/ChangeDetectionService.ts` around lines 391 - 403, The code builds nextStatus with value already merged via mergeStatusValues(existingStatus?.value, value) and then calls statuses.set(storyId, mergeChangeDetectionStatuses(existingStatus, nextStatus)) which re-merges the values; remove the redundant merge by assigning nextStatus.value = value (the incoming value) and let mergeChangeDetectionStatuses perform the merge using existingStatus, ensuring you update the construction of nextStatus (the const nextStatus object) and leave the call to mergeChangeDetectionStatuses(existingStatus, nextStatus) unchanged.
236-238: Consider logging suppressed errors for debugging.The
.catch(() => undefined)silently discards any errors fromhandleGitStateChange(). While this prevents unhandled rejections, it may hide issues during debugging. Consider logging at debug level.Proposed enhancement
this.scheduleScan(this.debounceMs); void this.getIndexBaselineService() .handleGitStateChange() - .catch(() => undefined); + .catch((error) => { + logger.debug('IndexBaselineService.handleGitStateChange failed: %s', error); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/change-detection/ChangeDetectionService.ts` around lines 236 - 238, The current call to this.getIndexBaselineService().handleGitStateChange().catch(() => undefined) swallows errors; update the catch to log the error at debug level instead of discarding it so issues are visible during debugging. Locate the call in ChangeDetectionService (the getIndexBaselineService().handleGitStateChange invocation) and replace the empty catch with one that logs the caught error via the class logger (e.g., this.logger.debug or the existing logger instance) including context like "handleGitStateChange error" and the error object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/core-server/change-detection/ChangeDetectionService.test.ts`:
- Around line 137-139: The test mock incorrectly overrides getHeadSha() while
the GitDiffProvider interface defines getHeadCommit(); update the mock to
implement async getHeadCommit(): Promise<string> { return this.getHeadShaMock();
} (or rename getHeadShaMock to getHeadCommitMock and wire it into getHeadCommit)
so the mock class implements GitDiffProvider correctly and the override and
helper names (getHeadShaMock / getHeadSha) are aligned with getHeadCommit.
In `@code/core/src/core-server/change-detection/ChangeDetectionService.ts`:
- Around line 180-186: The constructor parameter is named with PascalCase
("IndexBaselineService") instead of camelCase; rename the parameter to
indexBaselineService in the ChangeDetectionService constructor signature and
update the assignment inside the constructor (currently
this.indexBaselineService = options.IndexBaselineService) to use the new
camelCase name (this.indexBaselineService = options.indexBaselineService) so
parameter naming follows JS/TS conventions and matches the existing property.
---
Nitpick comments:
In `@code/core/src/core-server/change-detection/ChangeDetectionService.ts`:
- Around line 391-403: The code builds nextStatus with value already merged via
mergeStatusValues(existingStatus?.value, value) and then calls
statuses.set(storyId, mergeChangeDetectionStatuses(existingStatus, nextStatus))
which re-merges the values; remove the redundant merge by assigning
nextStatus.value = value (the incoming value) and let
mergeChangeDetectionStatuses perform the merge using existingStatus, ensuring
you update the construction of nextStatus (the const nextStatus object) and
leave the call to mergeChangeDetectionStatuses(existingStatus, nextStatus)
unchanged.
- Around line 236-238: The current call to
this.getIndexBaselineService().handleGitStateChange().catch(() => undefined)
swallows errors; update the catch to log the error at debug level instead of
discarding it so issues are visible during debugging. Locate the call in
ChangeDetectionService (the getIndexBaselineService().handleGitStateChange
invocation) and replace the empty catch with one that logs the caught error via
the class logger (e.g., this.logger.debug or the existing logger instance)
including context like "handleGitStateChange error" and the error object.
In `@code/core/src/core-server/change-detection/GitDiffProvider.test.ts`:
- Around line 148-168: The nested ternary inside the
vi.mocked(execa).mockImplementation block (which inspects gitArgs and returns
repoRootResult, stagedResult, unstagedResult, untrackedResult,
stagedAddedResult, intentToAddResult, headCommitResult, statusPorcelainResult)
should be replaced with a clear lookup table: build a Map or plain object keyed
by the gitArgs strings and return map.get(gitArgs) (or map[gitArgs]) instead of
the long nested ternary; ensure you keep the existing handling for
Array.isArray(rest[0]) to derive args and preserve returning undefined for
unmatched keys.
In `@code/core/src/manager/components/sidebar/Tree.tsx`:
- Around line 216-222: Export the getDisplayStatus utility so it can be imported
in unit tests: change its declaration to a named export (e.g., export const
getDisplayStatus = ... or export function getDisplayStatus(...)) and update any
internal references or imports that rely on the previous non-exported symbol
(ensure consuming components still import it by name). Keep the implementation
unchanged so tests can directly import getDisplayStatus to assert the
conditional mapping for story/docs hidden statuses and the default label
formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84a7a20e-82af-49b7-8c20-da8bbd1c7b89
📒 Files selected for processing (12)
code/builders/builder-vite/src/index.test.tscode/builders/builder-vite/src/index.tscode/core/src/core-server/change-detection/ChangeDetectionService.test.tscode/core/src/core-server/change-detection/ChangeDetectionService.tscode/core/src/core-server/change-detection/GitDiffProvider.test.tscode/core/src/core-server/change-detection/GitDiffProvider.tscode/core/src/core-server/change-detection/IndexBaselineService.test.tscode/core/src/core-server/change-detection/IndexBaselineService.tscode/core/src/core-server/dev-server.tscode/core/src/core-server/utils/index-json.test.tscode/core/src/core-server/utils/index-json.tscode/core/src/manager/components/sidebar/Tree.tsx
Comment on lines
+137
to
+139
| override getHeadSha(): Promise<string> { | ||
| return this.getHeadShaMock(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the method name in GitDiffProvider.ts
rg -n 'getHead(Commit|Sha)' --type=ts code/core/src/core-server/change-detection/Repository: storybookjs/storybook
Length of output: 1358
🏁 Script executed:
grep -n "getHeadCommit\|gitDiffProvider" code/core/src/core-server/change-detection/ChangeDetectionService.ts | head -20Repository: storybookjs/storybook
Length of output: 532
🏁 Script executed:
grep -n "getHeadCommit" code/core/src/core-server/change-detection/ChangeDetectionService.tsRepository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
grep -n "getChangedFiles\|getRepoRoot\|getHeadCommit\|getHeadSha" code/core/src/core-server/change-detection/ChangeDetectionService.test.ts | head -30Repository: storybookjs/storybook
Length of output: 1473
🏁 Script executed:
grep -n "getHeadShaMock\|getHeadSha()" code/core/src/core-server/change-detection/ChangeDetectionService.test.tsRepository: storybookjs/storybook
Length of output: 229
🏁 Script executed:
grep -n "\.getHeadSha\|\.getHeadCommit" code/core/src/core-server/change-detection/ChangeDetectionService.test.tsRepository: storybookjs/storybook
Length of output: 102
🏁 Script executed:
grep -n "getHeadSha\|getHeadCommit" code/core/src/core-server/change-detection/GitDiffProvider.tsRepository: storybookjs/storybook
Length of output: 110
Method name mismatch: getHeadSha vs getHeadCommit.
The mock method is named getHeadShaMock and override getHeadSha(), but GitDiffProvider.ts defines async getHeadCommit(): Promise<string>. This causes the mock to not properly implement the GitDiffProvider interface. While the method is not currently called in this test suite, the incorrect override violates the interface contract and should be corrected for consistency.
🐛 Proposed fix to align method names
- readonly getHeadShaMock = vi.fn(async (): Promise<string> => 'mock-sha');
+ readonly getHeadCommitMock = vi.fn(async (): Promise<string> => 'mock-sha');
- override getHeadSha(): Promise<string> {
- return this.getHeadShaMock();
+ override getHeadCommit(): Promise<string> {
+ return this.getHeadCommitMock();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/core-server/change-detection/ChangeDetectionService.test.ts`
around lines 137 - 139, The test mock incorrectly overrides getHeadSha() while
the GitDiffProvider interface defines getHeadCommit(); update the mock to
implement async getHeadCommit(): Promise<string> { return this.getHeadShaMock();
} (or rename getHeadShaMock to getHeadCommitMock and wire it into getHeadCommit)
so the mock class implements GitDiffProvider correctly and the override and
helper names (getHeadShaMock / getHeadSha) are aligned with getHeadCommit.
Comment on lines
+180
to
+186
| IndexBaselineService?: IndexBaselineService; | ||
| workingDir?: string; | ||
| debounceMs?: number; | ||
| } | ||
| ) { | ||
| this.gitDiffProvider = options.gitDiffProvider; | ||
| this.indexBaselineService = options.IndexBaselineService; |
Contributor
There was a problem hiding this comment.
Use camelCase for parameter names.
The parameter IndexBaselineService uses PascalCase, which is typically reserved for types and classes. Parameter names should use camelCase for consistency with JavaScript/TypeScript conventions and the adjacent gitDiffProvider parameter.
Proposed fix
constructor(
private readonly options: {
storyIndexGeneratorPromise: Promise<StoryIndexGenerator>;
statusStore: StatusStoreByTypeId;
gitDiffProvider?: GitDiffProvider;
- IndexBaselineService?: IndexBaselineService;
+ indexBaselineService?: IndexBaselineService;
workingDir?: string;
debounceMs?: number;
}
) {
this.gitDiffProvider = options.gitDiffProvider;
- this.indexBaselineService = options.IndexBaselineService;
+ this.indexBaselineService = options.indexBaselineService;📝 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
| IndexBaselineService?: IndexBaselineService; | |
| workingDir?: string; | |
| debounceMs?: number; | |
| } | |
| ) { | |
| this.gitDiffProvider = options.gitDiffProvider; | |
| this.indexBaselineService = options.IndexBaselineService; | |
| indexBaselineService?: IndexBaselineService; | |
| workingDir?: string; | |
| debounceMs?: number; | |
| } | |
| ) { | |
| this.gitDiffProvider = options.gitDiffProvider; | |
| this.indexBaselineService = options.indexBaselineService; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/core-server/change-detection/ChangeDetectionService.ts` around
lines 180 - 186, The constructor parameter is named with PascalCase
("IndexBaselineService") instead of camelCase; rename the parameter to
indexBaselineService in the ChangeDetectionService constructor signature and
update the assignment inside the constructor (currently
this.indexBaselineService = options.IndexBaselineService) to use the new
camelCase name (this.indexBaselineService = options.indexBaselineService) so
parameter naming follows JS/TS conventions and matches the existing property.
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.
What I did
Enhanced the change detection mechanism with story index change detection, to mark newly added stories with
status-value:new. To determine whether new stories were added, an initial snapshot of the story index is taken at startup, and used as a baseline for future comparisons. If the Git repository is clean at that time, the snapshot is persisted on disk and reused across runs, otherwise the baseline is reset whenever the Storybook server restarts.Note: this PR also incorporates #34429 to hide the "modified" icons on sibling stories.
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!
Documentation
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
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Release Notes
New Features
Improvements