Agentic Review [4b]: Move the review server cache into core-server#35223
Open
yannbf wants to merge 1 commit into
Open
Agentic Review [4b]: Move the review server cache into core-server#35223yannbf wants to merge 1 commit into
yannbf wants to merge 1 commit into
Conversation
yannbf
added
maintenance
yannbf
changed the title
yannbf
force-pushed
the
review-core/04a-cleanups-and-dedup
branch
from
79a3ec6 to
fa5a073
Compare
yannbf
force-pushed
the
review-core/04b-server-cache-to-core
branch
from
3b6597b to
07df4fd
Compare
yannbf
force-pushed
the
review-core/04a-cleanups-and-dedup
branch
from
fa5a073 to
54aace4
Compare
yannbf
force-pushed
the
review-core/04b-server-cache-to-core
branch
from
07df4fd to
df71b76
Compare
2 tasks
yannbf
changed the title
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughReview event names ( ChangesReview contract centralization and channel refactor
Sequence Diagram(s)sequenceDiagram
participant MCP as MCP Producer
participant CommonPreset as common-preset
participant initReviewChannel as initReviewChannel
participant ModuleGraph as ModuleGraphService
participant Tab as Browser Tab
CommonPreset->>initReviewChannel: initReviewChannel(channel)
MCP->>initReviewChannel: REVIEW_EVENTS.PUSH_REVIEW (payload)
initReviewChannel->>initReviewChannel: prepareReview (strip stale, stamp createdAt)
initReviewChannel->>Tab: REVIEW_EVENTS.DISPLAY_REVIEW
Tab->>initReviewChannel: REVIEW_EVENTS.REQUEST_REVIEW
initReviewChannel->>Tab: REVIEW_EVENTS.DISPLAY_REVIEW (cached)
ModuleGraph->>initReviewChannel: onChange (post-grace-window)
initReviewChannel->>Tab: REVIEW_EVENTS.REVIEW_STALE
MCP->>initReviewChannel: REVIEW_EVENTS.DISMISS_REVIEW
initReviewChannel->>initReviewChannel: cached = undefined
initReviewChannel->>Tab: REVIEW_EVENTS.REVIEW_DISMISSED
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 1
🧹 Nitpick comments (1)
code/core/src/core-server/server-channel/review-channel.test.ts (1)
62-236: ⚡ Quick winMove scenario-specific Date.now() spy into its own beforeEach for that test.
Line 229 uses
vi.spyOn(Date, 'now').mockReturnValue(NOW + 2000)directly within a test case. While this is acceptable, it's cleaner and safer to isolate scenario-specific mocks in a dedicatedbeforeEachcontext (either a nesteddescribeblock or a local setup within the test). This prevents accidental cross-test pollution if the spy is not properly cleaned up inafterEach.Consider wrapping that test (and similar staleness scenarios) in a nested describe block with its own
beforeEach/afterEachto manage the time mock more explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/core-server/server-channel/review-channel.test.ts` around lines 62 - 236, Move the scenario-specific Date.now() spy mocks that are called directly within individual test cases (such as `vi.spyOn(Date, 'now').mockReturnValue(NOW + 2000)` appearing in tests like "marks the cached review stale...", "emits REVIEW_STALE only once...", and "resets staleness when a new review is pushed") into a dedicated beforeEach/afterEach context. Consider creating a nested describe block within the "staleness" describe block with its own beforeEach and afterEach to explicitly manage these time-based mocks for those specific tests, rather than calling them directly within the test cases. This will ensure proper cleanup and prevent accidental cross-test pollution of the Date mock state.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/core/src/manager/globals/exports.ts`:
- Around line 696-697: The exports.ts file is auto-generated by the
generate-source-files.ts script and manual additions will be lost on
regeneration. Remove the manually added REVIEW_EVENTS and REVIEW_NAMESPACE
entries from the exports list in the exports.ts file, then run the
generate-source-files.ts script from code/core/scripts/ to regenerate the file
automatically. This ensures these exports are included through the proper
auto-generation process since they are now properly exported from
code/core/src/types/modules/review.ts.
---
Nitpick comments:
In `@code/core/src/core-server/server-channel/review-channel.test.ts`:
- Around line 62-236: Move the scenario-specific Date.now() spy mocks that are
called directly within individual test cases (such as `vi.spyOn(Date,
'now').mockReturnValue(NOW + 2000)` appearing in tests like "marks the cached
review stale...", "emits REVIEW_STALE only once...", and "resets staleness when
a new review is pushed") into a dedicated beforeEach/afterEach context. Consider
creating a nested describe block within the "staleness" describe block with its
own beforeEach and afterEach to explicitly manage these time-based mocks for
those specific tests, rather than calling them directly within the test cases.
This will ensure proper cleanup and prevent accidental cross-test pollution of
the Date mock state.
🪄 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: f138a4d4-c0cc-4ee2-a6fd-31ab222faba1
📒 Files selected for processing (14)
code/addons/review/build-config.tscode/addons/review/package.jsoncode/addons/review/preset.jscode/addons/review/src/constants.tscode/addons/review/src/review-state.tscode/core/src/core-server/presets/common-preset.tscode/core/src/core-server/server-channel/review-channel.test.tscode/core/src/core-server/server-channel/review-channel.tscode/core/src/manager/globals/exports.tscode/core/src/shared/review/events.tscode/core/src/shared/review/index.tscode/core/src/shared/review/review-state.tscode/core/src/types/index.tscode/core/src/types/modules/review.ts
💤 Files with no reviewable changes (2)
- code/addons/review/preset.js
- code/addons/review/build-config.ts
Comment on lines
+696
to
+697
| 'REVIEW_EVENTS', | ||
| 'REVIEW_NAMESPACE', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify sourcefiles.ts includes REVIEW_EVENTS and REVIEW_NAMESPACE in its generation logic
rg -n "REVIEW_EVENTS|REVIEW_NAMESPACE" code/core/src/manager/globals/sourcefiles.tsRepository: storybookjs/storybook
Length of output: 150
🏁 Script executed:
# First, locate the exports.ts file mentioned in the review
find code/core/src -name "exports.ts" -type fRepository: storybookjs/storybook
Length of output: 106
🏁 Script executed:
# Also search for any sourcefiles or related generator patterns
find code/core/src -name "*sourcefiles*" -o -name "*generator*" -o -name "*generated*" | head -20Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Search for references to "generated" in the codebase near exports/globals
rg -l "generated by|auto-generated" code/core/src/manager/globals/ 2>/dev/null | head -10Repository: storybookjs/storybook
Length of output: 106
🏁 Script executed:
# Read the header of exports.ts to understand how it's actually generated
head -20 code/core/src/manager/globals/exports.tsRepository: storybookjs/storybook
Length of output: 622
🏁 Script executed:
# Search for sourcefiles.ts anywhere in the repository
find code -name "sourcefiles.ts" -type fRepository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Also search with rg for references to sourcefiles.ts
rg -l "sourcefiles" code/Repository: storybookjs/storybook
Length of output: 149
🏁 Script executed:
# Read the generate-source-files.ts script to understand what it generates
cat code/core/scripts/generate-source-files.tsRepository: storybookjs/storybook
Length of output: 6275
🏁 Script executed:
# Search for REVIEW_EVENTS and REVIEW_NAMESPACE in runtime.ts and related globals files
rg -n "REVIEW_EVENTS|REVIEW_NAMESPACE" code/core/src/manager/globals/Repository: storybookjs/storybook
Length of output: 200
🏁 Script executed:
# Also check what's exported from runtime.ts
head -50 code/core/src/manager/globals/runtime.tsRepository: storybookjs/storybook
Length of output: 1678
🏁 Script executed:
# Check if REVIEW_EVENTS and REVIEW_NAMESPACE are exported from storybook/internal/types
rg -n "export.*REVIEW_EVENTS|export.*REVIEW_NAMESPACE" code/core/src/Repository: storybookjs/storybook
Length of output: 451
🏁 Script executed:
# Also check where these are defined
rg -n "REVIEW_EVENTS|REVIEW_NAMESPACE" code/core/src/ | grep -v "exports.ts" | head -20Repository: storybookjs/storybook
Length of output: 2187
🏁 Script executed:
# Check what storybook/internal/types exports (it maps to code/core/src)
# Look at the types index or barrel export
find code/core/src -name "index.ts" -path "*/types/*" | head -5Repository: storybookjs/storybook
Length of output: 94
🏁 Script executed:
# Check if the types module includes review exports
grep -r "REVIEW_EVENTS\|REVIEW_NAMESPACE" code/core/src/types/ --include="*.ts"Repository: storybookjs/storybook
Length of output: 184
🏁 Script executed:
# Check what the main types entry point exports
head -100 code/core/src/types/index.tsRepository: storybookjs/storybook
Length of output: 929
Run the generator to ensure these exports are auto-generated, not manually added.
This file is auto-generated by code/core/scripts/generate-source-files.ts. Since REVIEW_EVENTS and REVIEW_NAMESPACE are now properly exported from storybook/internal/types (via code/core/src/types/modules/review.ts), they will be included in the auto-generated output when the generator is executed.
Manual additions to this file will be lost on regeneration. To avoid this, run the generate-source-files script to ensure these exports are produced automatically rather than manually edited.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/core/src/manager/globals/exports.ts` around lines 696 - 697, The
exports.ts file is auto-generated by the generate-source-files.ts script and
manual additions will be lost on regeneration. Remove the manually added
REVIEW_EVENTS and REVIEW_NAMESPACE entries from the exports list in the
exports.ts file, then run the generate-source-files.ts script from
code/core/scripts/ to regenerate the file automatically. This ensures these
exports are included through the proper auto-generation process since they are
now properly exported from code/core/src/types/modules/review.ts.
Source: Coding guidelines
yannbf
changed the title
yannbf
force-pushed
the
review-core/04a-cleanups-and-dedup
branch
from
54aace4 to
a38a7d3
Compare
yannbf
force-pushed
the
review-core/04b-server-cache-to-core
branch
from
df71b76 to
d27eb49
Compare
yannbf
force-pushed
the
review-core/04a-cleanups-and-dedup
branch
from
a38a7d3 to
231d109
Compare
yannbf
force-pushed
the
review-core/04b-server-cache-to-core
branch
from
d27eb49 to
2f3e909
Compare
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 72 | 0 |
| Self size | 21.21 MB | 21.27 MB | 🚨 +60 KB 🚨 |
| Dependency size | 36.42 MB | 36.42 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/angular-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 31 | 🚨 +31 🚨 |
| Self size | 0 B | 25.14 MB | 🚨 +25.14 MB 🚨 |
| Dependency size | 0 B | 19.84 MB | 🚨 +19.84 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 204 | 204 | 0 |
| Self size | 802 KB | 821 KB | 🚨 +19 KB 🚨 |
| Dependency size | 91.56 MB | 91.63 MB | 🚨 +67 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 197 | 197 | 0 |
| Self size | 32 KB | 32 KB | 🚨 +36 B 🚨 |
| Dependency size | 90.05 MB | 90.11 MB | 🚨 +57 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 1.08 MB | 1.09 MB | 🚨 +10 KB 🚨 |
| Dependency size | 57.63 MB | 57.70 MB | 🚨 +60 KB 🚨 |
| Bundle Size Analyzer | node | node |
yannbf
commented
Jun 19, 2026
| if (cancelled) { | ||
| return; | ||
| } | ||
| void Promise.resolve().then(() => { |
Member
Author
There was a problem hiding this comment.
This Promise call seems unnecessary.
yannbf
commented
Jun 19, 2026
| * `description` / `collections` in sync with that schema. | ||
| */ | ||
|
|
||
| export type CollectionKind = 'atomic' | 'consumer' | 'transitive' | 'catch-all'; |
Member
Author
There was a problem hiding this comment.
This might need an update, I'll take a look into it
Internalizes the review ingest contract and the server-side cache (Q1 + Q5 server half), so core owns them instead of the addon: - New core-owned contract module `core/src/shared/review/` (ReviewState + REVIEW_EVENTS), re-exported via `storybook/internal/types`. - New `core-server/server-channel/review-channel.ts` (`initReviewChannel`) holds the cache, createdAt stamping, dismiss, and module-graph staleness; wired into `common-preset.ts`'s `experimental_serverChannel` alongside the other channels. The cache is now closure-scoped per channel (no module-global + test reset), and the module-graph lookup uses `getService` directly instead of a lazy cross-package import. - The addon's `preset.ts`/`preset.test.ts` (and its `./preset` build entry) are removed; `constants.ts`/`review-state.ts` re-export the single source. The addon is now manager-only (loaded via its `./manager` export). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
yannbf
force-pushed
the
review-core/04a-cleanups-and-dedup
branch
from
231d109 to
659695f
Compare
yannbf
force-pushed
the
review-core/04b-server-cache-to-core
branch
from
2f3e909 to
2af5b8b
Compare
ghengeveld
approved these changes
Jun 19, 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
Moves the review server-side cache + ingest contract from the addon into core (decisions Q1 and the server half of Q5/PR4). PR4b of the stack, telescoping on PR4a (#35222).
core/src/shared/review/module holdsReviewStateandREVIEW_EVENTS(thestorybook/review/*channel names), re-exported throughstorybook/internal/types. The addon'sconstants.ts/review-state.tsnow re-export this single source.core-server/server-channel/review-channel.ts(initReviewChannel) owns the cache,createdAtstamping, dismiss, and module-graph staleness. It's wired intocommon-preset.ts'sexperimental_serverChannelnext to the otherinit*Channelhelpers. The cache is now closure-scoped per channel (no module-global +__resetCache), and the module-graph lookup callsgetService('core/module-graph')directly instead of the addon's lazy cross-packageimport().preset.ts/preset.test.tsand the./presetbuild entry/package export. The review manager UI still loads via the addon's./managerexport (the deleted preset had nomanagerEntries, so manager loading is unaffected).Tests moved with the code:
preset.test.ts→review-channel.test.tsin core-server (13 tests, all green).Stack position
Base:
review-core/04a-cleanups-and-dedup(PR4a, #35222).Still to come (PR4c)
The manager-UI mount internalization (render the review layer + toolbar header in core, delete
persistentRender/TOOLBAR_HEADER, internal manager-api state module, M8) and PR5 (delete the addon package). This PR is the server-side half only.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
For a maintainer (this one does need a live check — see the AI note):
cd code && yarn storybook:ui.@storybook/addon-mcp/ a producer emittingstorybook/review/*). Confirm it appears (server caches + broadcastsDISPLAY_REVIEW).REQUEST_REVIEWreplay from the core-server cache).REVIEW_STALE).Documentation
Checklist for Maintainers
ci:normalqa:neededmaintenance🤖 Generated with Claude Code
Summary by CodeRabbit
presetsubpath export.initReviewChannel(replacing the experimental API).