Core: Show "new" status on newly added individual stories#34504
Merged
ghengeveld merged 19 commits intonextfrom Apr 17, 2026
Merged
Core: Show "new" status on newly added individual stories#34504ghengeveld merged 19 commits intonextfrom
ghengeveld merged 19 commits intonextfrom
Conversation
…ated ChangeDetectionService to utilize IndexBaselineService for marking new stories with the proper status.
Contributor
|
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 IndexBaselineService to maintain story-index baselines keyed by Git HEAD, extends GitDiffProvider with HEAD and working-tree queries, integrates baseline merging and merge helpers into ChangeDetectionService, and wires story-index invalidation callbacks through the dev-server and index.json route. Tests added/updated across services and UI. Changes
Sequence DiagramsequenceDiagram
participant GitRepo as Git Repository
participant GDP as GitDiffProvider
participant IBS as IndexBaselineService
participant SIG as StoryIndexGenerator
participant Cache as BaselineCache
participant CDS as ChangeDetectionService
participant Store as StatusStore
Note over IBS,CDS: startup / invalidation flow
CDS->>GDP: isWorkingTreeClean()
GDP->>GitRepo: git status --porcelain
GitRepo-->>GDP: stdout
alt working tree clean
CDS->>GDP: getHeadCommit()
GDP->>GitRepo: git rev-parse HEAD
GitRepo-->>GDP: HEAD SHA
IBS->>Cache: lookup HEAD SHA
alt cache hit
Cache-->>IBS: entryIds
else cache miss
IBS->>SIG: generate story index
SIG-->>IBS: StoryIndex
IBS->>IBS: extractBaselineEntryIds()
IBS->>Cache: persist {entryIds} by SHA
end
IBS->>CDS: onBaselineUpdated()
CDS->>CDS: buildIndexBaselineStatuses() & merge into statuses
CDS->>Store: apply status patch
else working tree dirty
IBS->>SIG: generate story index (in-memory)
SIG-->>IBS: StoryIndex
IBS->>IBS: extractBaselineEntryIds() (no persist)
CDS->>CDS: merge in-memory baseline (no cache)
CDS->>Store: apply status patch
end
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}
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
valentinpalkovic
requested changes
Apr 14, 2026
|
View your CI Pipeline Execution ↗ for commit 41b7afb
☁️ Nx Cloud last updated this comment at |
valentinpalkovic
approved these changes
Apr 15, 2026
…stent capitalization for test statuses
…r consistency in ChangeDetectionService tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.ts`:
- Around line 408-415: The applyStatusStorePatch currently re-merges fresh scan
results with the existing status store (via this.options.statusStore.getAll()
and mergeChangeDetectionStatuses), which causes old changedFiles/values to
persist and stale "new" statuses to override fresh "modified" results; change
applyStatusStorePatch to stop reading/merging with allCurrentStatuses and
instead accept the scan's built results as authoritative (just use the incoming
nextStatuses entries directly when populating mergedNextStatuses or write
nextStatuses straight into the status store), and remove calls to
mergeChangeDetectionStatuses here (also remove the same merge logic at the
similar block around the other occurrence noted).
In `@code/core/src/core-server/dev-server.ts`:
- Around line 51-55: The code creates two ChangeDetectionService instances
causing registerIndexJsonRoute to close over the dormant one while a different
instance is started/disposed later; update the code so a single
ChangeDetectionService instance (the variable changeDetectionService created
near the top) is reused everywhere: remove the second instantiation at lines
creating/starting/disposing a new ChangeDetectionService and instead call
start/stop on the original changeDetectionService, ensure
registerIndexJsonRoute, onStoryIndexInvalidated and any lifecycle start/stop
logic reference that same changeDetectionService variable so its module-graph
and baseline state remain consistent.
🪄 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: 297404ef-bd60-4050-90b3-27237379f934
📒 Files selected for processing (8)
code/core/src/core-server/change-detection/ChangeDetectionService.test.tscode/core/src/core-server/change-detection/ChangeDetectionService.tscode/core/src/core-server/change-detection/GitDiffProvider.tscode/core/src/core-server/dev-server.tscode/core/src/manager/components/sidebar/Filter.stories.tsxcode/core/src/manager/components/sidebar/Tree.tsxtest-storybooks/portable-stories-kitchen-sink/react-vitest-3/e2e-tests/component-testing.spec.tstest-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/core-server/change-detection/GitDiffProvider.ts
tmeasday
added a commit
to tmeasday/mealdrop
that referenced
this pull request
Apr 16, 2026
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
2 participants
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.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 pull request has been released as version
0.0.0-pr-34504-sha-8dd6d4b4. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34504-sha-8dd6d4b4 sandboxor in an existing project withnpx storybook@0.0.0-pr-34504-sha-8dd6d4b4 upgrade.More information
0.0.0-pr-34504-sha-8dd6d4b4story-new-status8dd6d4b41776321567)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=34504Summary by CodeRabbit