Core: Review Changes#34370
Closed
ghengeveld wants to merge 19 commits into
Closed
Conversation
…to valentin/status-based-filtering
- Added `buildModuleGraph` function to create a module graph from Vite's module nodes. - Introduced `onModuleGraphChange` to allow listeners to react to changes in the module graph. - Created comprehensive tests for module graph behavior in `index.test.ts`. - Updated type definitions to include `ModuleGraph` and `ModuleNode` for better type safety.
- Changed the event listener removal from 'change' to 'all' in the bail function to ensure proper cleanup. - Updated the test description to reflect the new behavior of removing the all-event watcher during bail. - Added assertions in tests to verify the listener count before and after bail operations.
…omponents - Added Filter component to manage tag and status filters in the sidebar. - Created FilterPanel component to display and manage filter options. - Integrated filter functionality into the Sidebar component, replacing the previous TagsFilter. - Enhanced status handling in various components, ensuring consistent status icon and color usage. - Updated SearchResults and Tree components to utilize new status handling logic. - Added stories for FilterPanel to demonstrate various filter states and interactions.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Track changed files from git, trace affected story files through the builder module graph, and surface those results through the dev server status store.
|
View your CI Pipeline Execution ↗ for commit b4eae34
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 20.47 MB | 20.50 MB | 🚨 +30 KB 🚨 |
| Dependency size | 16.55 MB | 16.55 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 184 | 184 | 0 |
| Self size | 782 KB | 780 KB | 🎉 -1 KB 🎉 |
| Dependency size | 67.69 MB | 67.72 MB | 🚨 +29 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 177 | 177 | 0 |
| Self size | 32 KB | 32 KB | 🎉 -36 B 🎉 |
| Dependency size | 66.22 MB | 66.25 MB | 🚨 +30 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 51 | 51 | 0 |
| Self size | 1.04 MB | 1.04 MB | 🎉 -779 B 🎉 |
| Dependency size | 37.03 MB | 37.06 MB | 🚨 +30 KB 🚨 |
| Bundle Size Analyzer | node | node |
Contributor
📝 WalkthroughWalkthroughIntroduces a change detection system that tracks Git-modified files, traces their impact across module graphs, and marks affected stories as Changes
Sequence Diagram(s)sequenceDiagram
actor User as User/<br/>Developer
participant Preview as Preview Builder<br/>(Vite)
participant Module as Module Graph<br/>Change Watcher
participant Service as Change<br/>Detection Service
participant Git as Git Diff<br/>Provider
participant Trace as Module Tracer<br/>(findAffectedStoryFiles)
participant Store as Status<br/>Store
User->>Preview: Edit story/module file
Preview->>Module: Emit module change event
Module->>Service: onModuleGraphChange callback (debounced 100ms)
Service->>Service: Trigger rescan
Service->>Git: getChangedFiles()
Git-->>Service: {changed: Set, new: Set, repoRoot}
Service->>Service: Map story files from index
loop For each changed file
Service->>Trace: findAffectedStoryFiles(changedFile,<br/>moduleGraph, storyIdsByFile)
Trace-->>Service: Map<storyFile, {distance}>
end
Service->>Service: Classify statuses<br/>(new/modified/affected)
Service->>Store: Update story statuses
Store-->>User: UI reflects changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
code/core/src/core-server/dev-server.ts (1)
133-143:⚠️ Potential issue | 🟠 MajorBare
.catch()breaks the cleanup chain and masks the original error.When
dispose()orbail()fails, the bare.catch()forwards the rejection, causing the error handler to reject with the cleanup error instead of the original preview-start error. This prevents subsequent cleanup operations from running and prevents the original error from being thrown at line 144.Each cleanup operation must suppress errors to ensure all resources are released. Add an error handler to allow the cleanup chain to continue:
- await changeDetectionService.dispose().catch(); - await managerBuilder?.bail().catch(); - await previewBuilder?.bail().catch(); + await changeDetectionService.dispose().catch(() => undefined); + await managerBuilder?.bail().catch(() => undefined); + await previewBuilder?.bail().catch(() => undefined);The same pattern issue exists in the outer try/catch at lines 159–162.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/dev-server.ts` around lines 133 - 143, The cleanup calls in the preview-start error handler (changeDetectionService.dispose(), managerBuilder?.bail(), previewBuilder?.bail()) use bare .catch() which can swallow the original error and replace it with a cleanup error; change each to explicitly suppress/log cleanup failures (e.g., .catch(err => logger.warn(...)) or .catch(() => undefined)) so failures in dispose() or bail() do not short-circuit the remaining cleanup and the original preview-start error is preserved; apply the same fix to the outer try/catch cleanup calls referenced around the later block (the corresponding dispose/bail calls in that outer error handler) so all cleanup errors are suppressed and do not mask the original exception.code/core/src/manager/components/sidebar/Filter.tsx (1)
93-120:⚠️ Potential issue | 🟡 MinorUpdate the accessible copy now that this panel includes status filters.
The popover and button still announce "Tag filters", but this UI now includes status-based filters too. Screen-reader users will hear the wrong feature description here.
♿ Suggested wording update
<PopoverProvider - ariaLabel="Tag filters" + ariaLabel="Story filters" placement="bottom" onVisibleChange={setExpanded} offset={8} padding={0} @@ ariaLabel={ activeFilterCount - ? `${activeFilterCount} active tag ${activeFilterCount !== 1 ? 'filters' : 'filter'}` - : 'Tag filters' + ? `${activeFilterCount} active story ${activeFilterCount !== 1 ? 'filters' : 'filter'}` + : 'Story filters' } - ariaDescription="Filter the items shown in a sidebar based on the tags applied to them." + ariaDescription="Filter the items shown in the sidebar by tags and statuses."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/Filter.tsx` around lines 93 - 120, Update the accessible labels to reflect that the panel contains both tag and status filters: change the PopoverProvider ariaLabel from "Tag filters" to something like "Filters" or "Tag and status filters", update the StyledButton ariaLabel construction (the activeFilterCount branch and default) to use the new wording (e.g., `${activeFilterCount} active filters` / 'Filters'), and update StyledButton ariaDescription from "Filter the items shown in a sidebar based on the tags..." to a neutral description mentioning tags and status (e.g., "Filter the items shown in the sidebar by tags and status"). Ensure you modify the ariaLabel on PopoverProvider and both ariaLabel and ariaDescription on StyledButton so screen readers reflect both filter types (references: PopoverProvider, StyledButton, activeFilterCount, ariaLabel, ariaDescription).code/core/src/manager-api/modules/stories.ts (1)
1188-1204:⚠️ Potential issue | 🟠 MajorPotential memory leak: unsubscribe callback not captured.
onAllStatusChangereturns an unsubscribe function (percode/core/src/shared/status-store/index.ts:149-158), but the return value is discarded. This listener will never be cleaned up if the module is re-initialized or during hot-reload scenarios.🔧 Proposed fix to capture and manage the unsubscribe callback
Consider storing the unsubscribe callback and invoking it appropriately. One approach:
+ let unsubscribeStatusChange: (() => void) | undefined; + + unsubscribeStatusChange = fullStatusStore.onAllStatusChange(async () => { - fullStatusStore.onAllStatusChange(async () => { // re-apply the filters when the statuses change recomputeStatusFilter(); // ... rest of handler });The cleanup should be invoked when the module is disposed (if there's a dispose/cleanup mechanism), or stored in a way that allows for proper lifecycle management.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/modules/stories.ts` around lines 1188 - 1204, The listener registered via fullStatusStore.onAllStatusChange is never unsubscribed, causing a potential memory leak; capture the unsubscribe function returned by fullStatusStore.onAllStatusChange and register it with the module's cleanup/dispose mechanism (or push it into an existing disposables array) so it can be invoked when the module is torn down or hot-reloaded; specifically, update the code around the onAllStatusChange call (which calls recomputeStatusFilter(), await api.setIndex(index), and iterates fullAPI.getRefs()/fullAPI.setRef()) to assign the returned unsubscribe to a variable and ensure that variable is called during module disposal.
🧹 Nitpick comments (5)
code/core/src/core-server/change-detection/git-diff-provider.test.ts (1)
20-38: Move theexecastubbing out of theitbodies.Both cases install their
mockResolvedValueOnce/mockRejectedValueOncechains inline. A nesteddescribewith per-scenariobeforeEachsetup would keep this aligned with the repo's Vitest pattern and make the call sequence easier to maintain. As per coding guidelines, "Implement mock behaviors inbeforeEachblocks in Vitest tests" and "Avoid inline mock implementations within test cases 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/change-detection/git-diff-provider.test.ts` around lines 20 - 38, The tests inline their execa mock setups inside the individual it blocks; move those mock behaviors into a surrounding describe with per-scenario beforeEach blocks so mocking follows the repository Vitest pattern and preserves call order. Refactor the two tests that exercise GitDiffProvider.getChangedFiles to live inside a describe, and in each scenario's beforeEach call vi.mocked(execa) and set up the same mockResolvedValueOnce / mockRejectedValueOnce chain used previously (the four mockResolvedValueOnce sequence for the "returns the union..." case, and the mockRejectedValueOnce for the "throws a typed unavailable error..." case) so the call sequence remains identical and no inline mocking remains in the it bodies.code/core/src/manager-api/tests/stories.test.ts (1)
1549-1551: Consider usingspy: trueoption for status store mock.Per coding guidelines,
vi.mock()should use thespy: trueoption. While the current approach works, usingspy: trueprovides better type safety and consistency with other mocks in the codebase.♻️ Suggested change
- vi.mock('../stores/status'); + vi.mock('../stores/status', { spy: true });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/manager-api/tests/stories.test.ts` around lines 1549 - 1551, The test "can filter on status" calls vi.mock('../stores/status') without the recommended spy option; update that vi.mock call for the status store (path '../stores/status') to include the spy: true option so the mock is created as a spy (keeping existing imports/behaviour), ensuring consistency and better type-safety with other tests that mock the same module (the change is local to the test in stories.test.ts near the 'can filter on status' case that uses createMockModuleArgs).code/core/src/manager-api/modules/statuses.ts (1)
57-80: Consider cachingfullStatusStore.getAll()outside the per-entry filter loop.The filter function calls
fullStatusStore.getAll()for every entry being filtered. If the status store returns a stable reference, this is fine. However, ifgetAll()creates a new object on each call, this could cause unnecessary allocations when filtering large indices.♻️ Optional: Cache statuses outside the loop
If performance becomes a concern, the filter factory could capture the statuses once:
export const computeStatusFilterFn = ( includedStatusFilters: StatusValue[], excludedStatusFilters: StatusValue[] ): API_FilterFunction => { + const allStatuses = fullStatusStore.getAll() ?? {}; return (entry: API_PreparedIndexEntry) => { if (!includedStatusFilters.length && !excludedStatusFilters.length) { return true; } - const allStatuses = fullStatusStore.getAll() ?? {}; const storyStatuses = allStatuses[entry.id];Note: This would require re-creating the filter when statuses change, which may already be handled by the caller.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/modules/statuses.ts` around lines 57 - 80, computeStatusFilterFn currently calls fullStatusStore.getAll() inside the per-entry predicate, causing repeated allocations; change it to call fullStatusStore.getAll() once when the filter is created (inside computeStatusFilterFn) and capture that result in a local variable (e.g., allStatuses) used by the returned function, keeping the rest of the logic (storyStatuses, storyStatusValues, passesInclude/passesExclude) the same; ensure callers recreate the filter when the status store changes if necessary.code/core/src/core-server/change-detection/service.ts (1)
44-44: Remove commented-out debug code.This commented-out logger statement should be removed before merging.
🧹 Proposed fix
const absolutePath = resolve(workingDir, entry.importPath); - // logger.info(`Story ${entry.id} absolute path: ${absolutePath}`); const storyIds = storyIdsByFile.get(absolutePath) ?? new Set<string>();🤖 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/service.ts` at line 44, Remove the leftover commented-out debug line in change-detection service: delete the commented logger call "// logger.info(`Story ${entry.id} absolute path: ${absolutePath}`);" so the file no longer contains commented debug code; locate the statement near the change-detection service implementation where logger.info and variables entry.id and absolutePath are referenced and remove that single commented line.code/core/src/manager-api/modules/stories.ts (1)
524-528: Minor inconsistency in null checks.Status filters use optional chaining (
includedStatusFilters?.length ?? 0) while tag filters are accessed directly (includedTagFilters.length). Since all four are typed as non-optional arrays inSubState, the defensive checks are unnecessary but harmless. Consider making the style consistent—either trust the types for all filters or use defensive checks for all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/modules/stories.ts` around lines 524 - 528, The null-check style is inconsistent: in the hasActiveFilters computation the status filters use optional chaining and null coalescing (includedStatusFilters?.length ?? 0 / excludedStatusFilters?.length ?? 0) while tag filters access length directly (includedTagFilters.length / excludedTagFilters.length); since SubState types these as non-optional arrays, make the style consistent by removing the defensive checks for status filters and use direct .length access for includedStatusFilters and excludedStatusFilters (refer to hasActiveFilters, includedTagFilters, excludedTagFilters, includedStatusFilters, excludedStatusFilters and the SubState type).
🤖 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/builders/builder-vite/src/index.ts`:
- Around line 77-99: The startChangeDetection flow can continue after teardown
and recreate the watcher; add a run token/abort flag (e.g., pass an AbortSignal
or simple boolean token into startChangeDetection) and check it after each await
(after indexGenerator.getIndex(), after Promise.all(server.warmupRequest(...)),
and after await server.waitForRequestsIdle()) before proceeding to set the
watcher or calling watcherChangeHandler; ensure the polling interval clear logic
also checks the token, and make async callbacks (the setInterval handler)
respect the token and swallow/reject errors to avoid unhandled rejections so
that bail()/teardown prevents any stale task from re-registering the 'all'
watcher or operating post-teardown.
In `@code/builders/builder-vite/src/utils/build-module-graph.ts`:
- Around line 45-60: The current single-pass loop over fileToModulesMap can miss
edges when a viteModuleNode has file === null and is encountered via
importers/importedModules before its keyed entry is processed; implement a
two-pass fix: first iterate fileToModulesMap to record a fallbackFileMap mapping
module identity (viteModuleNode) to the filePath key whenever the node's .file
is null (so you can later resolve null-file nodes), then in a second pass
iterate fileToModulesMap again and wire edges (the existing logic using
getOrCreateModuleNode, moduleNode, viteModuleNode.importers, and
viteModuleNode.importedModules) but when resolving nodes call a variant that
uses the fallbackFileMap (or pass the fallback filePath into
getOrCreateModuleNode) so importers/importedModules for null-file nodes are
created and connected regardless of traversal order.
In `@code/core/src/core-server/change-detection/git-diff-provider.ts`:
- Around line 43-68: getChangedFiles currently collapses staged and unstaged
diffs into one changed set and treats "new" as only untracked files, which
misclassifies staged additions; update getChangedFiles to distinguish additions
from modifications by running git diff --cached --diff-filter=A and git diff
--diff-filter=A (and keep the existing ls-files output) or alternatively return
distinct buckets (stagedAdded, stagedModified, unstagedAdded, unstagedModified,
untracked) so callers can decide how to merge; ensure parseChangedFiles is
reused for each command's stdout and adjust the returned object shape (or add
new properties) so staged additions are not lost into changed.
In `@code/core/src/core-server/dev-server.ts`:
- Around line 117-123: The ChangeDetectionService instance (created via new
ChangeDetectionService and started with changeDetectionService.start) is only
disposed inside the previewBuilder.start rejection path; update the outer
startup error handling to also call changeDetectionService.dispose() so its
timers/subscriptions are cleared if app.listen, storyIndexGeneratorPromise, or
any other startup step fails. Locate the ChangeDetectionService creation/start
block and the outer try/catch around previewBuilder.start / app.listen, and add
a safe disposal call to changeDetectionService.dispose() (guarded for existence)
in the outer catch/finally path so it always runs on startup failures.
In `@code/core/src/manager-api/modules/tags.ts`:
- Around line 64-92: Both getDefaultTagsFromPreset and computeStaticFilterFn
assume tagPresets/presets is non-null and call Object.entries(), which will
throw when tags are undefined; add a null/undefined guard at the start of
getDefaultTagsFromPreset (return { included: [], excluded: [] } when presets is
falsy) and in computeStaticFilterFn (treat tagPresets as an empty object when
falsy and return an empty Record<string, boolean>), updating the functions
getDefaultTagsFromPreset and computeStaticFilterFn to early-return safe empty
values instead of calling Object.entries on a nullable value.
In `@code/core/src/manager/components/sidebar/FilterPanel.utils.ts`:
- Around line 48-49: areFiltersEqual currently treats arrays with different
duplicate counts as equal (e.g., ['a','a'] vs ['a','b']) because it only
compares lengths and the size of a Set; update the areFiltersEqual
implementation to account for duplicates by comparing element frequencies (or by
sorting both arrays and doing an element-by-element comparison). Locate the
areFiltersEqual function and replace the Set-based equality check with a
frequency-map comparison (or sorted-array comparison) that ensures both arrays
have identical counts for each element.
---
Outside diff comments:
In `@code/core/src/core-server/dev-server.ts`:
- Around line 133-143: The cleanup calls in the preview-start error handler
(changeDetectionService.dispose(), managerBuilder?.bail(),
previewBuilder?.bail()) use bare .catch() which can swallow the original error
and replace it with a cleanup error; change each to explicitly suppress/log
cleanup failures (e.g., .catch(err => logger.warn(...)) or .catch(() =>
undefined)) so failures in dispose() or bail() do not short-circuit the
remaining cleanup and the original preview-start error is preserved; apply the
same fix to the outer try/catch cleanup calls referenced around the later block
(the corresponding dispose/bail calls in that outer error handler) so all
cleanup errors are suppressed and do not mask the original exception.
In `@code/core/src/manager-api/modules/stories.ts`:
- Around line 1188-1204: The listener registered via
fullStatusStore.onAllStatusChange is never unsubscribed, causing a potential
memory leak; capture the unsubscribe function returned by
fullStatusStore.onAllStatusChange and register it with the module's
cleanup/dispose mechanism (or push it into an existing disposables array) so it
can be invoked when the module is torn down or hot-reloaded; specifically,
update the code around the onAllStatusChange call (which calls
recomputeStatusFilter(), await api.setIndex(index), and iterates
fullAPI.getRefs()/fullAPI.setRef()) to assign the returned unsubscribe to a
variable and ensure that variable is called during module disposal.
In `@code/core/src/manager/components/sidebar/Filter.tsx`:
- Around line 93-120: Update the accessible labels to reflect that the panel
contains both tag and status filters: change the PopoverProvider ariaLabel from
"Tag filters" to something like "Filters" or "Tag and status filters", update
the StyledButton ariaLabel construction (the activeFilterCount branch and
default) to use the new wording (e.g., `${activeFilterCount} active filters` /
'Filters'), and update StyledButton ariaDescription from "Filter the items shown
in a sidebar based on the tags..." to a neutral description mentioning tags and
status (e.g., "Filter the items shown in the sidebar by tags and status").
Ensure you modify the ariaLabel on PopoverProvider and both ariaLabel and
ariaDescription on StyledButton so screen readers reflect both filter types
(references: PopoverProvider, StyledButton, activeFilterCount, ariaLabel,
ariaDescription).
---
Nitpick comments:
In `@code/core/src/core-server/change-detection/git-diff-provider.test.ts`:
- Around line 20-38: The tests inline their execa mock setups inside the
individual it blocks; move those mock behaviors into a surrounding describe with
per-scenario beforeEach blocks so mocking follows the repository Vitest pattern
and preserves call order. Refactor the two tests that exercise
GitDiffProvider.getChangedFiles to live inside a describe, and in each
scenario's beforeEach call vi.mocked(execa) and set up the same
mockResolvedValueOnce / mockRejectedValueOnce chain used previously (the four
mockResolvedValueOnce sequence for the "returns the union..." case, and the
mockRejectedValueOnce for the "throws a typed unavailable error..." case) so the
call sequence remains identical and no inline mocking remains in the it bodies.
In `@code/core/src/core-server/change-detection/service.ts`:
- Line 44: Remove the leftover commented-out debug line in change-detection
service: delete the commented logger call "// logger.info(`Story ${entry.id}
absolute path: ${absolutePath}`);" so the file no longer contains commented
debug code; locate the statement near the change-detection service
implementation where logger.info and variables entry.id and absolutePath are
referenced and remove that single commented line.
In `@code/core/src/manager-api/modules/statuses.ts`:
- Around line 57-80: computeStatusFilterFn currently calls
fullStatusStore.getAll() inside the per-entry predicate, causing repeated
allocations; change it to call fullStatusStore.getAll() once when the filter is
created (inside computeStatusFilterFn) and capture that result in a local
variable (e.g., allStatuses) used by the returned function, keeping the rest of
the logic (storyStatuses, storyStatusValues, passesInclude/passesExclude) the
same; ensure callers recreate the filter when the status store changes if
necessary.
In `@code/core/src/manager-api/modules/stories.ts`:
- Around line 524-528: The null-check style is inconsistent: in the
hasActiveFilters computation the status filters use optional chaining and null
coalescing (includedStatusFilters?.length ?? 0 / excludedStatusFilters?.length
?? 0) while tag filters access length directly (includedTagFilters.length /
excludedTagFilters.length); since SubState types these as non-optional arrays,
make the style consistent by removing the defensive checks for status filters
and use direct .length access for includedStatusFilters and
excludedStatusFilters (refer to hasActiveFilters, includedTagFilters,
excludedTagFilters, includedStatusFilters, excludedStatusFilters and the
SubState type).
In `@code/core/src/manager-api/tests/stories.test.ts`:
- Around line 1549-1551: The test "can filter on status" calls
vi.mock('../stores/status') without the recommended spy option; update that
vi.mock call for the status store (path '../stores/status') to include the spy:
true option so the mock is created as a spy (keeping existing
imports/behaviour), ensuring consistency and better type-safety with other tests
that mock the same module (the change is local to the test in stories.test.ts
near the 'can filter on status' case that uses createMockModuleArgs).
🪄 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: 7046559e-c9e5-449c-8847-d2945a3f4407
📒 Files selected for processing (37)
.vscode/settings.jsoncode/builders/builder-vite/src/index.test.tscode/builders/builder-vite/src/index.tscode/builders/builder-vite/src/utils/build-module-graph.tscode/core/src/components/components/ActionList/ActionList.tsxcode/core/src/core-server/change-detection/errors.tscode/core/src/core-server/change-detection/git-diff-provider.test.tscode/core/src/core-server/change-detection/git-diff-provider.tscode/core/src/core-server/change-detection/index.tscode/core/src/core-server/change-detection/readiness.tscode/core/src/core-server/change-detection/service.test.tscode/core/src/core-server/change-detection/service.tscode/core/src/core-server/change-detection/trace-changed.test.tscode/core/src/core-server/change-detection/trace-changed.tscode/core/src/core-server/dev-server.tscode/core/src/core-server/index.tscode/core/src/manager-api/lib/stories.tscode/core/src/manager-api/modules/statuses.tscode/core/src/manager-api/modules/stories.tscode/core/src/manager-api/modules/tags.tscode/core/src/manager-api/tests/stories.test.tscode/core/src/manager/components/sidebar/Filter.stories.tsxcode/core/src/manager/components/sidebar/Filter.story-helpers.tsxcode/core/src/manager/components/sidebar/Filter.tsxcode/core/src/manager/components/sidebar/FilterPanel.stories.tsxcode/core/src/manager/components/sidebar/FilterPanel.tsxcode/core/src/manager/components/sidebar/FilterPanel.utils.tscode/core/src/manager/components/sidebar/FilterPanelLink.tsxcode/core/src/manager/components/sidebar/SearchResults.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/StatusButton.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/useFilterData.tsxcode/core/src/manager/utils/status.tsxcode/core/src/shared/status-store/index.tscode/core/src/types/modules/core-common.ts
💤 Files with no reviewable changes (1)
- code/core/src/manager/components/sidebar/TagsFilterPanel.tsx
Comment on lines
+77
to
+99
| const startChangeDetection = async (options: Options) => { | ||
| const startTime = process.hrtime(); | ||
| const indexGenerator = await options.presets.apply<StoryIndexGenerator>('storyIndexGenerator'); | ||
| const storyIndex = await indexGenerator.getIndex(); | ||
|
|
||
| // Warm up the module graph for all story files | ||
| await Promise.all( | ||
| Object.values(storyIndex.entries).map((entry) => server.warmupRequest(entry.importPath)) | ||
| ); | ||
|
|
||
| // Wait for the module graph to be ready by polling for it to be non-empty | ||
| waitForModuleGraph = setInterval(async () => { | ||
| if (!watcherChangeHandler || process.hrtime(startTime)[0] > 30) { | ||
| clearInterval(waitForModuleGraph); | ||
| waitForModuleGraph = undefined; | ||
| } else if (server.moduleGraph.fileToModulesMap.size > 0) { | ||
| clearInterval(waitForModuleGraph); | ||
| waitForModuleGraph = undefined; | ||
| await server.waitForRequestsIdle(); | ||
| server.watcher.on('all', watcherChangeHandler); | ||
| watcherChangeHandler(); | ||
| } | ||
| }, 1000); |
Contributor
There was a problem hiding this comment.
Cancel the fire-and-forget module-graph bootstrap on teardown.
Line 122 drops the startChangeDetection() promise, but Lines 83-98 still await warmupRequest() / waitForRequestsIdle() and then re-register the 'all' watcher. If bail() runs in between, that stale task can recreate the watcher after cleanup or surface an unhandled rejection. Please gate this flow with a run token / abort flag and re-check it after each await before attaching the watcher.
Also applies to: 113-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/builders/builder-vite/src/index.ts` around lines 77 - 99, The
startChangeDetection flow can continue after teardown and recreate the watcher;
add a run token/abort flag (e.g., pass an AbortSignal or simple boolean token
into startChangeDetection) and check it after each await (after
indexGenerator.getIndex(), after Promise.all(server.warmupRequest(...)), and
after await server.waitForRequestsIdle()) before proceeding to set the watcher
or calling watcherChangeHandler; ensure the polling interval clear logic also
checks the token, and make async callbacks (the setInterval handler) respect the
token and swallow/reject errors to avoid unhandled rejections so that
bail()/teardown prevents any stale task from re-registering the 'all' watcher or
operating post-teardown.
Comment on lines
+45
to
+60
| fileToModulesMap.forEach((viteModuleSet, filePath) => { | ||
| viteModuleSet.forEach((viteModuleNode) => { | ||
| const moduleNode = getOrCreateModuleNode(viteModuleNode, filePath); | ||
| if (moduleNode) { | ||
| viteModuleNode.importers.forEach((importer) => { | ||
| const importerNode = getOrCreateModuleNode(importer); | ||
| if (importerNode) { | ||
| moduleNode.importers.add(importerNode); | ||
| } | ||
| }); | ||
| viteModuleNode.importedModules.forEach((importedModule) => { | ||
| const importedModuleNode = getOrCreateModuleNode(importedModule); | ||
| if (importedModuleNode) { | ||
| moduleNode.importedModules.add(importedModuleNode); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n code/builders/builder-vite/src/utils/build-module-graph.tsRepository: storybookjs/storybook
Length of output: 2506
🏁 Script executed:
# Search for Vite type definitions in the repo
fd -e d.ts "vite" | head -20Repository: storybookjs/storybook
Length of output: 129
🏁 Script executed:
# Check if there's documentation or comments about ModuleNode.file being nullable
rg "ModuleNode.*file.*null|file.*null.*ModuleNode" --type ts --type tsx -A 2 -B 2 code/builders/builder-vite/Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
# Search for any tests related to build-module-graph
fd "build-module-graph" code/builders/builder-vite/Repository: storybookjs/storybook
Length of output: 123
🏁 Script executed:
# Check for any Vite imports and usage patterns in builder-vite
rg "ModuleNode|fileToModulesMap" code/builders/builder-vite/ --type ts -B 2 -A 2 | head -50Repository: storybookjs/storybook
Length of output: 4522
🏁 Script executed:
# Search for test files related to builder-vite
fd "\.test\.|\.spec\." code/builders/builder-vite/Repository: storybookjs/storybook
Length of output: 723
🏁 Script executed:
# Check what version of Vite is being used
rg "\"vite\"" code/builders/builder-vite/package.jsonRepository: storybookjs/storybook
Length of output: 148
🏁 Script executed:
# Find where buildModuleGraph is called
rg "buildModuleGraph" code/builders/builder-vite/ --type ts -B 2 -A 2Repository: storybookjs/storybook
Length of output: 4153
🏁 Script executed:
# Check the internal types to understand ModuleGraph and ModuleNode
cat -n code/core/src/types.ts | grep -A 10 "ModuleGraph\|ModuleNode" | head -40Repository: storybookjs/storybook
Length of output: 119
🏁 Script executed:
# Find the types file
fd "types.ts" code/core/ | head -5Repository: storybookjs/storybook
Length of output: 235
🏁 Script executed:
# Look at the test file to understand how buildModuleGraph is tested
cat -n code/builders/builder-vite/src/index.test.ts | grep -A 30 "describe.*buildModuleGraph"Repository: storybookjs/storybook
Length of output: 1610
🏁 Script executed:
# Continue looking at the test to see if there's a test for null-file (virtual) modules
cat -n code/builders/builder-vite/src/index.test.ts | grep -A 50 "virtualModule\|null" | head -70Repository: storybookjs/storybook
Length of output: 2636
🏁 Script executed:
# Check what createViteModuleNode does
cat -n code/builders/builder-vite/src/index.test.ts | grep -B 5 -A 20 "function createViteModuleNode"Repository: storybookjs/storybook
Length of output: 939
🏁 Script executed:
# Look for more tests, especially ones that might test null-file nodes in fileToModulesMap
cat -n code/builders/builder-vite/src/index.test.ts | grep -B 5 -A 30 "it('.*null\|it('.*virtual"Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Look at test around line 182 more completely to see full test case
sed -n '175,200p' code/builders/builder-vite/src/index.test.tsRepository: storybookjs/storybook
Length of output: 1072
🏁 Script executed:
# Look at the entire test file to understand all test cases
wc -l code/builders/builder-vite/src/index.test.tsRepository: storybookjs/storybook
Length of output: 113
🏁 Script executed:
# Check if there are any other tests that might show the null-file scenario
cat -n code/builders/builder-vite/src/index.test.ts | sed -n '120,250p'Repository: storybookjs/storybook
Length of output: 5478
🏁 Script executed:
# Search for any issues or comments about null-file nodes in the codebase
rg "null.*file|virtual.*module" code/builders/builder-vite/ -B 2 -A 2Repository: storybookjs/storybook
Length of output: 4781
Fix order-dependent edge creation for null-file nodes in fileToModulesMap.
When a Vite ModuleNode with file === null appears as a value in fileToModulesMap and is encountered through importers or importedModules before its keyed entry is processed, getOrCreateModuleNode() returns undefined (no fallback provided). The edge is skipped. Later, when the node's fileToModulesMap entry is processed, the node can be created with the fallback file path, but the earlier edge is not retroactively added. This makes module graph traversal order-dependent and can cause affected-story detection to miss stories.
The suggested two-pass approach solves this: populate a fallback-file map on the first pass, then wire edges on the second pass with fallbacks available for any null-file nodes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/builders/builder-vite/src/utils/build-module-graph.ts` around lines 45 -
60, The current single-pass loop over fileToModulesMap can miss edges when a
viteModuleNode has file === null and is encountered via
importers/importedModules before its keyed entry is processed; implement a
two-pass fix: first iterate fileToModulesMap to record a fallbackFileMap mapping
module identity (viteModuleNode) to the filePath key whenever the node's .file
is null (so you can later resolve null-file nodes), then in a second pass
iterate fileToModulesMap again and wire edges (the existing logic using
getOrCreateModuleNode, moduleNode, viteModuleNode.importers, and
viteModuleNode.importedModules) but when resolving nodes call a variant that
uses the fallbackFileMap (or pass the fallback filePath into
getOrCreateModuleNode) so importers/importedModules for null-file nodes are
created and connected regardless of traversal order.
Comment on lines
+43
to
+68
| async getChangedFiles(): Promise<GitDiffResult> { | ||
| const repoRoot = await this.getRepoRoot(); | ||
|
|
||
| try { | ||
| const [staged, unstaged, untracked] = await Promise.all([ | ||
| execa('git', ['diff', '--name-only', '--diff-filter=d', '--cached'], { | ||
| cwd: repoRoot, | ||
| stdio: 'pipe', | ||
| }), | ||
| execa('git', ['diff', '--name-only', '--diff-filter=d'], { | ||
| cwd: repoRoot, | ||
| stdio: 'pipe', | ||
| }), | ||
| execa('git', ['ls-files', '--others', '--exclude-standard'], { | ||
| cwd: repoRoot, | ||
| stdio: 'pipe', | ||
| }), | ||
| ]); | ||
|
|
||
| return { | ||
| changed: new Set([ | ||
| ...parseChangedFiles(staged.stdout), | ||
| ...parseChangedFiles(unstaged.stdout), | ||
| ]), | ||
| new: parseChangedFiles(untracked.stdout), | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does git ls-files --others --exclude-standardstill report a file after it has been staged withgit add, and which Git diff command can distinguish added files from modified files?
💡 Result:
No, git ls-files --others --exclude-standard does not report a file after it has been staged with git add. This command lists untracked files in the working directory that are not ignored (per standard .gitignore rules). Once git add stages a previously untracked file, it becomes tracked in the index, so it is no longer considered "other" (untracked) and is excluded from the output. The Git diff command that can distinguish added files from modified files is git diff --cached --name-status (staged changes relative to HEAD). It outputs lines like: - A newfile.txt for added files (new files staged, absent in HEAD) - M existingfile.txt for modified files (existing files with staged changes) git diff HEAD --name-status shows unstaged changes similarly but relative to HEAD vs working tree/index.
Citations:
- 1: https://git-scm.com/docs/git-ls-files
- 2: https://git-scm.com/docs/git-ls-files.html
- 3: https://stackoverflow.com/questions/2298047/git-ls-files-howto-identify-new-files-added-not-committed
- 4: https://stackoverflow.com/questions/3801321/git-list-only-untracked-files-also-custom-commands
- 5: https://git-scm.com/docs/git-diff.html
- 6: https://git-scm.com/docs/git-diff
- 7: https://stackoverflow.com/questions/54828777/for-git-diff-name-status-what-does-the-output-mean
Staged additions are misclassified because new excludes staged files.
The new field only captures untracked files via git ls-files --others, which stops reporting a file once it has been staged with git add. This means a newly added story staged for commit will incorrectly appear in changed (from the staged diff) rather than new. Additionally, collapsing staged and unstaged paths into a single changed set prevents callers from implementing the requested staged vs working-tree modes, since both additions and modifications end up in the same bucket.
To fix this, use git diff --cached --diff-filter=A and git diff --diff-filter=A to capture added files separately from modified files, or return staged/unstaged/added changes as distinct buckets and merge them at the call site if needed.
🤖 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/git-diff-provider.ts` around lines
43 - 68, getChangedFiles currently collapses staged and unstaged diffs into one
changed set and treats "new" as only untracked files, which misclassifies staged
additions; update getChangedFiles to distinguish additions from modifications by
running git diff --cached --diff-filter=A and git diff --diff-filter=A (and keep
the existing ls-files output) or alternatively return distinct buckets
(stagedAdded, stagedModified, unstagedAdded, unstagedModified, untracked) so
callers can decide how to merge; ensure parseChangedFiles is reused for each
command's stdout and adjust the returned object shape (or add new properties) so
staged additions are not lost into changed.
Comment on lines
+117
to
+123
| const changeDetectionService = new ChangeDetectionService({ | ||
| storyIndexGeneratorPromise, | ||
| statusStore: getStatusStoreByTypeId(CHANGE_DETECTION_STATUS_TYPE_ID), | ||
| workingDir, | ||
| }); | ||
| changeDetectionService.start(previewBuilder.onModuleGraphChange, features?.changeDetection); | ||
|
|
Contributor
There was a problem hiding this comment.
Dispose the change-detection service from the outer startup catch too.
changeDetectionService.dispose() only runs when previewBuilder.start() rejects. If app.listen(...) or storyIndexGeneratorPromise fails after the preview has started, the outer catch bails the builders but never clears the service's timer/subscription state.
Suggested fix
- if (!options.ignorePreview) {
- const changeDetectionService = new ChangeDetectionService({
+ let changeDetectionService: ChangeDetectionService | undefined;
+
+ if (!options.ignorePreview) {
+ changeDetectionService = new ChangeDetectionService({
storyIndexGeneratorPromise,
statusStore: getStatusStoreByTypeId(CHANGE_DETECTION_STATUS_TYPE_ID),
workingDir,
});
@@
} catch (e) {
+ await changeDetectionService?.dispose().catch(() => undefined);
await managerBuilder?.bail().catch();
await previewBuilder?.bail().catch();
throw e;
}Also applies to: 155-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/core-server/dev-server.ts` around lines 117 - 123, The
ChangeDetectionService instance (created via new ChangeDetectionService and
started with changeDetectionService.start) is only disposed inside the
previewBuilder.start rejection path; update the outer startup error handling to
also call changeDetectionService.dispose() so its timers/subscriptions are
cleared if app.listen, storyIndexGeneratorPromise, or any other startup step
fails. Locate the ChangeDetectionService creation/start block and the outer
try/catch around previewBuilder.start / app.listen, and add a safe disposal call
to changeDetectionService.dispose() (guarded for existence) in the outer
catch/finally path so it always runs on startup failures.
Comment on lines
+64
to
+92
| export const getDefaultTagsFromPreset = memoize(1)(( | ||
| presets: TagsOptions | ||
| ): { | ||
| included: Tag[]; | ||
| excluded: Tag[]; | ||
| } => { | ||
| const presetEntries = Object.entries(presets); | ||
| return { | ||
| included: presetEntries | ||
| .filter(([, option]) => option.defaultFilterSelection === 'include') | ||
| .map(([tag]) => tag), | ||
| excluded: presetEntries | ||
| .filter(([, option]) => option.defaultFilterSelection === 'exclude') | ||
| .map(([tag]) => tag), | ||
| }; | ||
| }); | ||
|
|
||
| export const computeStaticFilterFn = (tagPresets: TagsOptions) => { | ||
| const staticExcludeTags = Object.entries(tagPresets).reduce( | ||
| (acc, entry) => { | ||
| const [tag, option] = entry; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| if ((option as any).excludeFromSidebar) { | ||
| acc[tag] = true; | ||
| } | ||
| return acc; | ||
| }, | ||
| {} as Record<string, boolean> | ||
| ); |
Contributor
There was a problem hiding this comment.
Guard missing tag presets before calling Object.entries().
tags is optional, and this patch already treats global.TAGS_OPTIONS as nullable elsewhere. getDefaultTagsFromPreset() and computeStaticFilterFn() will both throw if a caller forwards the raw preset value when no tags config is defined.
Suggested fix
-export const getDefaultTagsFromPreset = memoize(1)((
- presets: TagsOptions
+export const getDefaultTagsFromPreset = memoize(1)((
+ presets: TagsOptions | undefined
): {
included: Tag[];
excluded: Tag[];
} => {
- const presetEntries = Object.entries(presets);
+ const presetEntries = Object.entries(presets ?? {});
return {
included: presetEntries
.filter(([, option]) => option.defaultFilterSelection === 'include')
.map(([tag]) => tag),
excluded: presetEntries
.filter(([, option]) => option.defaultFilterSelection === 'exclude')
.map(([tag]) => tag),
};
});
-export const computeStaticFilterFn = (tagPresets: TagsOptions) => {
- const staticExcludeTags = Object.entries(tagPresets).reduce(
+export const computeStaticFilterFn = (tagPresets: TagsOptions | undefined) => {
+ const staticExcludeTags = Object.entries(tagPresets ?? {}).reduce(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/manager-api/modules/tags.ts` around lines 64 - 92, Both
getDefaultTagsFromPreset and computeStaticFilterFn assume tagPresets/presets is
non-null and call Object.entries(), which will throw when tags are undefined;
add a null/undefined guard at the start of getDefaultTagsFromPreset (return {
included: [], excluded: [] } when presets is falsy) and in computeStaticFilterFn
(treat tagPresets as an empty object when falsy and return an empty
Record<string, boolean>), updating the functions getDefaultTagsFromPreset and
computeStaticFilterFn to early-return safe empty values instead of calling
Object.entries on a nullable value.
Comment on lines
+48
to
+49
| export const areFiltersEqual = (left: string[], right: string[]) => | ||
| left.length === right.length && new Set([...left, ...right]).size === left.length; |
Contributor
There was a problem hiding this comment.
areFiltersEqual can misreport equality when duplicates are present.
On Line 48–49, arrays like ['a','a'] and ['a','b'] can incorrectly compare equal. This can cause stale filter state handling for malformed/hand-edited URL params.
🔧 Suggested fix
-export const areFiltersEqual = (left: string[], right: string[]) =>
- left.length === right.length && new Set([...left, ...right]).size === left.length;
+export const areFiltersEqual = (left: string[], right: string[]) => {
+ if (left.length !== right.length) {
+ return false;
+ }
+
+ const counts = new Map<string, number>();
+ for (const value of left) {
+ counts.set(value, (counts.get(value) ?? 0) + 1);
+ }
+ for (const value of right) {
+ const next = (counts.get(value) ?? 0) - 1;
+ if (next < 0) {
+ return false;
+ }
+ counts.set(value, next);
+ }
+ return Array.from(counts.values()).every((count) => count === 0);
+};📝 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
| export const areFiltersEqual = (left: string[], right: string[]) => | |
| left.length === right.length && new Set([...left, ...right]).size === left.length; | |
| export const areFiltersEqual = (left: string[], right: string[]) => { | |
| if (left.length !== right.length) { | |
| return false; | |
| } | |
| const counts = new Map<string, number>(); | |
| for (const value of left) { | |
| counts.set(value, (counts.get(value) ?? 0) + 1); | |
| } | |
| for (const value of right) { | |
| const next = (counts.get(value) ?? 0) - 1; | |
| if (next < 0) { | |
| return false; | |
| } | |
| counts.set(value, next); | |
| } | |
| return Array.from(counts.values()).every((count) => count === 0); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/manager/components/sidebar/FilterPanel.utils.ts` around lines
48 - 49, areFiltersEqual currently treats arrays with different duplicate counts
as equal (e.g., ['a','a'] vs ['a','b']) because it only compares lengths and the
size of a Set; update the areFiltersEqual implementation to account for
duplicates by comparing element frequencies (or by sorting both arrays and doing
an element-by-element comparison). Locate the areFiltersEqual function and
replace the Set-based equality check with a frequency-map comparison (or
sorted-array comparison) that ensures both arrays have identical counts for each
element.
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.
Closes #34250
What I did
This is the integration branch for the Review Changes feature.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
features.changeDetection: truein.storybook/main.tsDocumentation
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-34370-sha-b4eae34c. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34370-sha-b4eae34c sandboxor in an existing project withnpx storybook@0.0.0-pr-34370-sha-b4eae34c upgrade.More information
0.0.0-pr-34370-sha-b4eae34creview-changesb4eae34c1774620615)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=34370Summary by CodeRabbit
New Features
Bug Fixes
Refactor