test for canary#35115
Closed
yannbf wants to merge 173 commits into
Closed
Conversation
New @storybook/addon-review-changes. An ADE agent pushes a review via the MCP addon; it is broadcast over the Storybook channel; this addon's page receives it (and requests a replay on mount for late/refreshed tabs) and renders the raw agent state — narrative, clusters, changed files, diff hunks, per-story metadata.
…nused files - Updated import path for ReviewChangesPage component. - Deleted unused ReviewChangesDetailsScreen, ReviewChangesPage, ReviewChangesScreen, and ReviewCollectionGrid components along with their associated stories. - Streamlined the codebase by removing obsolete files to enhance maintainability.
This reverts commit e3fcd45.
…handling and add stories - Added location search handling to ReviewChangesPage to parse collection and story parameters. - Introduced ReviewChangesPage stories to demonstrate various states and interactions. - Updated CollectionGrid and SummaryScreen components for improved naming consistency and functionality.
Review Changes: Implement review changes screen based on latest designs
…w-design-iterations
… navigation logic, and enhance URL handling for review states. Add new ReviewPage component and associated stories for improved review flow.
…tate` to `request-review`. Update related references in the codebase for consistency.
… and TabButton styled components, and update layout for improved tab functionality and accessibility.
Keep selectComponentEntriesByComponentId from the docgen phase-3 base while preserving module-graph subscription reactivity in docgen. Co-authored-by: Cursor <cursoragent@cursor.com>
Expose the hook from storybook/internal/components and align review CopyButton props with UseCopyButtonOptions.
Replace needle/haystack naming with searchQuery, lowerCaseText, and matchIndex.
Per-story revision stamps let scoped subscribers react only to relevant graph changes, while watch-all no longer advances on irrelevant file events. Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve docgen/module-graph conflicts after the service-registration split, store workingDir in module-graph state now that query handlers cannot be overridden at registration, and address CodeRabbit engine feedback for pre-patch dependents and early startup failure handling. Co-authored-by: Cursor <cursoragent@cursor.com>
Index reconciliation and snapshot baseline must not leave prior story-file change lists attached to a newer graph revision. Co-authored-by: Cursor <cursoragent@cursor.com>
Move getStatus load into the service definition via a settlement bridge, and register module-graph before docgen in manifest tests that exercise docgen static output. Co-authored-by: Cursor <cursoragent@cursor.com>
…th 1up UX Pin CopyButton to UseCopyButtonOptions<ReactNode> so JSX childrenOnCopy type-checks, and update WithBaseline to expect only the Latest label in default 1up mode with a new WithBaselineSplit story for 2up.
…and. getStatus.load now invokes a server-registered command so manager runtimes route settlement over the open-service channel instead of relying on a module-level promise that never resolves outside the dev server. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
The addon-review staleness banner relied on glue code: a process-wide source-changes notifier (subscribeToSourceFileChanges / notifySourceFileChange) that the module-graph engine pushed raw file events into, consumed by the preset via a non-existent experimental_subscribeToSourceFileChanges export. The open-service architecture already exposes this reactively, so subscribe to `core/module-graph`'s getGraphRevision query instead. This also scopes staleness to in-graph changes (unrelated file edits no longer trip the banner) and removes the dead notifier entirely. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds a new review addon package and UI, introduces freeze behavior for preview rendering, and migrates core change-detection and docgen wiring to module-graph services. It also updates shared exports, package/version metadata, and build configuration for the new addon. ChangesReview addon
Preview freeze behavior
Module-graph migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
|
![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/shared/open-service/services/module-graph/engine/module-graph-engine.ts (1)
23-23: 💤 Low valueNote: Deep relative import path.
The import path
../../../../../core-server/change-detection/source-changes.tsspans six directory levels, which can be fragile during refactoring. This appears intentional for cross-module integration between the open-service module-graph and core-server change-detection, but consider whether a more stable import path or abstraction might improve maintainability.🤖 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/shared/open-service/services/module-graph/engine/module-graph-engine.ts` at line 23, The import of notifySourceFileChange in module-graph-engine.ts uses a fragile deep relative path; replace it with a stable import by exposing notifySourceFileChange from a higher-level public barrel or shared package and updating imports accordingly. Concretely, export notifySourceFileChange from a central module (e.g., core-server change-detection barrel or a shared utilities package) and update the import in module-graph-engine.ts to import { notifySourceFileChange } from that public module; if needed add a tsconfig path alias to point to the core-server package so future moves won't break imports.code/core/src/shared/open-service/services/module-graph/server.ts (1)
1-1: ⚖️ Poor tradeoffOptional: Refactor deferred promise to avoid eslint-disable.
The
prefer-constdisable exists becauseresolveAdapter(line 27) andengine(line 52) use definite assignment assertions and are initialized outside their declarations. While the current pattern is common and correct, you could refactor to avoid the disable:♻️ Alternative pattern without eslint-disable
-/* eslint-disable prefer-const */ -let resolveAdapter!: (adapter: ChangeDetectionAdapter | null | undefined) => void; - -export const changeDetectionAdapterPromise = new Promise<ChangeDetectionAdapter | null | undefined>( - (resolve) => { - resolveAdapter = resolve; - } -); +const { promise: changeDetectionAdapterPromise, resolve: resolveAdapter } = (() => { + let resolveInner!: (adapter: ChangeDetectionAdapter | null | undefined) => void; + const promise = new Promise<ChangeDetectionAdapter | null | undefined>((resolve) => { + resolveInner = resolve; + }); + return { promise, resolve: resolveInner }; +})(); + +export { changeDetectionAdapterPromise };🤖 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/shared/open-service/services/module-graph/server.ts` at line 1, Replace the ad-hoc deferred and definite-assignment variables to remove the file-wide "eslint-disable prefer-const": create a small factory (e.g., createDeferred<T>()) that returns a const object with promise, resolve, and reject so you can declare const resolveAdapter = createDeferred<...>() and use resolveAdapter.resolve(...) instead of the current separate resolve variable; likewise initialize engine via a const wrapper (e.g., const engineHolder = { value: null as Engine | null } or use createDeferred/optional holder) so you avoid definite-assignment assertions; update usages of resolveAdapter and engine to access the new wrapper/resolver methods and then remove the eslint-disable line.code/core/src/shared/open-service/services/module-graph/server.test.ts (1)
429-456: 💤 Low valueConsider moving mock setup to
beforeEachfor guideline consistency.The mock behaviors are currently configured inline via
installDependencyGraphMocks()within individual tests. As per coding guidelines, mock implementations should be inbeforeEachblocks. While the current pattern is clean and works well for test-specific fixtures (different reverse indexes per test), you could refactor to a shared setup with parameterized fixtures if strict guideline adherence is desired.As per coding guidelines: "Implement mock behaviors in
beforeEachblocks in Vitest tests" and "Avoid inline mock implementations within test cases in Vitest tests".🤖 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/shared/open-service/services/module-graph/server.test.ts` around lines 429 - 456, Move the inline mock setup into a beforeEach so mocks are initialized consistently for each test: call buildReverseIndex(...) and installDependencyGraphMocks(...) inside a beforeEach and expose any per-test fixtures (like the reverseIndex data) via variables assigned in that beforeEach; keep resolveChangeDetectionAdapter(adapter), createMockAdapter(...) and registerModuleGraphService(...) in the test body but rely on the beforeEach-installed mocks instead of calling installDependencyGraphMocks(...) inline; ensure the beforeEach cleans or reassigns mocks so each test gets a fresh, parameterizable mock configuration.Source: Coding guidelines
code/addons/review/src/ReviewPage.tsx (1)
405-413: ⚡ Quick winConsider using human-readable labels for accessibility.
The aria-label currently uses the raw
storyId(e.g., "Review story button-component--base"), but at line 403 the component has access toinfowhich containstitleandnamederived viaderiveStoryInfo. Using those would provide a more screen-reader-friendly label.♻️ Proposed improvement
- aria-label={href ? `Review story ${storyId}` : undefined} + aria-label={href ? `Review ${component} ${name}` : undefined}🤖 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/addons/review/src/ReviewPage.tsx` around lines 405 - 413, The aria-label for the review button currently uses the raw storyId; update the label to use the human-readable values available from info (derived via deriveStoryInfo) by using info.title or, if missing, info.name as the primary text (with storyId as a fallback). Locate the button/element that sets aria-label (in ReviewPage.tsx where storyId and info are in scope) and replace the raw storyId usage with a composed string like "{info.title || info.name || storyId} — Review" so screen readers get a friendly title while preserving a fallback.
🤖 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/core-server/change-detection/change-detection-service.test.ts`:
- Around line 40-42: Update the Vitest mock declaration to include the spy
option so the module is mocked as a spy: change the vi.mock call for
'../../shared/open-service/server.ts' to pass { spy: true } and keep the mocked
export shape ({ getService: vi.fn() }); then use vi.mocked(getService) where the
test already references the mock to access typed spy helpers. This affects the
existing vi.mock(...) and getService usage in the test file.
In `@code/core/src/manager/globals/exports.ts`:
- Line 586: Remove the manual addition of the export 'useCopyButton' from the
generated exports file and instead add it to the generator (sourcefiles.ts) so
it will be emitted automatically; specifically, revert the manual change in the
generated file, update the generator logic in sourcefiles.ts to include the
'useCopyButton' export among the symbols it collects/emits, run the generator to
regenerate the exports file, and commit the updated generator and regenerated
output (not manual edits to the generated file).
---
Nitpick comments:
In `@code/addons/review/src/ReviewPage.tsx`:
- Around line 405-413: The aria-label for the review button currently uses the
raw storyId; update the label to use the human-readable values available from
info (derived via deriveStoryInfo) by using info.title or, if missing, info.name
as the primary text (with storyId as a fallback). Locate the button/element that
sets aria-label (in ReviewPage.tsx where storyId and info are in scope) and
replace the raw storyId usage with a composed string like "{info.title ||
info.name || storyId} — Review" so screen readers get a friendly title while
preserving a fallback.
In
`@code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.ts`:
- Line 23: The import of notifySourceFileChange in module-graph-engine.ts uses a
fragile deep relative path; replace it with a stable import by exposing
notifySourceFileChange from a higher-level public barrel or shared package and
updating imports accordingly. Concretely, export notifySourceFileChange from a
central module (e.g., core-server change-detection barrel or a shared utilities
package) and update the import in module-graph-engine.ts to import {
notifySourceFileChange } from that public module; if needed add a tsconfig path
alias to point to the core-server package so future moves won't break imports.
In `@code/core/src/shared/open-service/services/module-graph/server.test.ts`:
- Around line 429-456: Move the inline mock setup into a beforeEach so mocks are
initialized consistently for each test: call buildReverseIndex(...) and
installDependencyGraphMocks(...) inside a beforeEach and expose any per-test
fixtures (like the reverseIndex data) via variables assigned in that beforeEach;
keep resolveChangeDetectionAdapter(adapter), createMockAdapter(...) and
registerModuleGraphService(...) in the test body but rely on the
beforeEach-installed mocks instead of calling installDependencyGraphMocks(...)
inline; ensure the beforeEach cleans or reassigns mocks so each test gets a
fresh, parameterizable mock configuration.
In `@code/core/src/shared/open-service/services/module-graph/server.ts`:
- Line 1: Replace the ad-hoc deferred and definite-assignment variables to
remove the file-wide "eslint-disable prefer-const": create a small factory
(e.g., createDeferred<T>()) that returns a const object with promise, resolve,
and reject so you can declare const resolveAdapter = createDeferred<...>() and
use resolveAdapter.resolve(...) instead of the current separate resolve
variable; likewise initialize engine via a const wrapper (e.g., const
engineHolder = { value: null as Engine | null } or use createDeferred/optional
holder) so you avoid definite-assignment assertions; update usages of
resolveAdapter and engine to access the new wrapper/resolver methods and then
remove the eslint-disable line.
🪄 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: 166b2224-1039-445e-82a9-bf61cc54ff6b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (93)
code/.storybook/main.tscode/addons/review/README.mdcode/addons/review/build-config.tscode/addons/review/manager.jscode/addons/review/package.jsoncode/addons/review/preset.jscode/addons/review/project.jsoncode/addons/review/src/ReviewPage.stories.tsxcode/addons/review/src/ReviewPage.tsxcode/addons/review/src/components/CollectionGrid.stories.tsxcode/addons/review/src/components/CollectionGrid.tsxcode/addons/review/src/components/CopyButton.tsxcode/addons/review/src/components/Highlight.tsxcode/addons/review/src/components/ReviewHeader.tsxcode/addons/review/src/components/StaleBanner.tsxcode/addons/review/src/constants.tscode/addons/review/src/index.tscode/addons/review/src/manager.tsxcode/addons/review/src/preset.test.tscode/addons/review/src/preset.tscode/addons/review/src/review-navigation.tscode/addons/review/src/review-state.tscode/addons/review/src/screens/DetailsScreen.stories.tsxcode/addons/review/src/screens/DetailsScreen.tsxcode/addons/review/src/screens/SummaryScreen.stories.tsxcode/addons/review/src/screens/SummaryScreen.tsxcode/addons/review/src/screens/useBaselineComparison.tscode/addons/review/src/session-store.tscode/addons/review/tsconfig.jsoncode/addons/review/vitest.config.tscode/core/src/common/versions.tscode/core/src/components/index.tscode/core/src/core-server/change-detection/active-service-registry.tscode/core/src/core-server/change-detection/adapters/index.tscode/core/src/core-server/change-detection/change-detection-service.test.tscode/core/src/core-server/change-detection/change-detection-service.tscode/core/src/core-server/change-detection/change-detection.test-helpers.tscode/core/src/core-server/change-detection/dependency-graph/index.tscode/core/src/core-server/change-detection/index.tscode/core/src/core-server/change-detection/parser-registry/index.tscode/core/src/core-server/change-detection/source-changes.test.tscode/core/src/core-server/change-detection/source-changes.tscode/core/src/core-server/dev-server.tscode/core/src/core-server/index.tscode/core/src/core-server/presets/common-preset.tscode/core/src/core-server/utils/index-json.test.tscode/core/src/core-server/utils/index-json.tscode/core/src/core-server/utils/manifests/manifests.test.tscode/core/src/csf/csf-factories.test.tscode/core/src/manager-api/modules/url.tscode/core/src/manager-api/tests/url.test.jscode/core/src/manager/globals/exports.tscode/core/src/manager/settings/FreezeBehavior.stories.tsxcode/core/src/preview-api/modules/preview-web/PreviewWithSelection.tsxcode/core/src/preview-api/modules/preview-web/setupStoryFreezer.test.tscode/core/src/preview-api/modules/preview-web/setupStoryFreezer.tscode/core/src/preview/runtime.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/services/docgen/server.test.tscode/core/src/shared/open-service/services/docgen/server.tscode/core/src/shared/open-service/services/module-graph/definition.tscode/core/src/shared/open-service/services/module-graph/engine/adapters/types.test.tscode/core/src/shared/open-service/services/module-graph/engine/adapters/types.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/dependency-graph-builder.test.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/dependency-graph-builder.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/incremental-patch.test.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/incremental-patcher.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/parse-resolve-cache.test.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/parse-resolve-cache.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/resolver-factory.test.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/resolver-factory.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/reverse-index.test.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/reverse-index.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/scope.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/types.tscode/core/src/shared/open-service/services/module-graph/engine/dependency-graph/walk-from-story.tscode/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.test.tscode/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.tscode/core/src/shared/open-service/services/module-graph/engine/parser-registry/builtins.test.tscode/core/src/shared/open-service/services/module-graph/engine/parser-registry/builtins.tscode/core/src/shared/open-service/services/module-graph/engine/parser-registry/mdx-parse.tscode/core/src/shared/open-service/services/module-graph/engine/parser-registry/parser-registry.test.tscode/core/src/shared/open-service/services/module-graph/engine/parser-registry/parser-registry.tscode/core/src/shared/open-service/services/module-graph/engine/parser-registry/types.tscode/core/src/shared/open-service/services/module-graph/errors.tscode/core/src/shared/open-service/services/module-graph/module-graph.test-helpers.tscode/core/src/shared/open-service/services/module-graph/server.test.tscode/core/src/shared/open-service/services/module-graph/server.tscode/core/src/shared/open-service/services/module-graph/story-files.tscode/core/src/shared/open-service/services/module-graph/types.tscode/core/src/types/modules/core-common.tscode/package.jsonscripts/build/entry-configs.ts
💤 Files with no reviewable changes (7)
- code/core/src/core-server/change-detection/adapters/index.ts
- code/core/src/core-server/change-detection/parser-registry/index.ts
- code/core/src/core-server/change-detection/active-service-registry.ts
- code/core/src/core-server/utils/index-json.test.ts
- code/core/src/core-server/utils/index-json.ts
- code/core/src/core-server/change-detection/dependency-graph/index.ts
- code/core/src/core-server/change-detection/index.ts
Comment on lines
+40
to
+42
| vi.mock('../../shared/open-service/server.ts', () => ({ | ||
| getService: vi.fn(), | ||
| })); |
Contributor
There was a problem hiding this comment.
Add spy: true option to the mock.
The mock for ../../shared/open-service/server.ts should use the spy: true option for consistent spy mocking. As per coding guidelines, all package and file mocks in Vitest tests should use vi.mock() with spy: true.
🔧 Suggested fix
-vi.mock('../../shared/open-service/server.ts', () => ({
- getService: vi.fn(),
-}));
+vi.mock('../../shared/open-service/server.ts', { spy: true });Then access the mocked function using vi.mocked(getService) where needed (which is already done at line 50).
📝 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
| vi.mock('../../shared/open-service/server.ts', () => ({ | |
| getService: vi.fn(), | |
| })); | |
| vi.mock('../../shared/open-service/server.ts', { spy: true }); |
🤖 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/change-detection/change-detection-service.test.ts`
around lines 40 - 42, Update the Vitest mock declaration to include the spy
option so the module is mocked as a spy: change the vi.mock call for
'../../shared/open-service/server.ts' to pass { spy: true } and keep the mocked
export shape ({ getService: vi.fn() }); then use vi.mocked(getService) where the
test already references the mock to access typed spy helpers. This affects the
existing vi.mock(...) and getService usage in the test file.
Source: Coding guidelines
| 'interleaveSeparators', | ||
| 'nameSpaceClassNames', | ||
| 'resetComponents', | ||
| 'useCopyButton', |
Contributor
There was a problem hiding this comment.
Do not manually edit this generated file.
Lines 1-2 explicitly state this file is auto-generated by sourcefiles.ts. Manual additions like useCopyButton will be overwritten on the next generation and should not be committed. Instead, update the generator to include this export. As per coding guidelines, "Do not commit accidental overrides to generated code like code/core/src/manager/globals/exports.ts that are auto-generated as stated in their JSDoc header."
🤖 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` at line 586, Remove the manual
addition of the export 'useCopyButton' from the generated exports file and
instead add it to the generator (sourcefiles.ts) so it will be emitted
automatically; specifically, revert the manual change in the generated file,
update the generator logic in sourcefiles.ts to include the 'useCopyButton'
export among the symbols it collects/emits, run the generator to regenerate the
exports file, and commit the updated generator and regenerated output (not
manual edits to the generated file).
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/addons/review/src/preset.ts`:
- Around line 41-49: The subscription to service.queries.getGraphRevision
currently treats any revision > 0 as a change and can fire immediately with the
current revision; instead record the first seen revision and only call onChange
for later increases. In the block using getService<typeof
moduleGraphServiceDef>('core/module-graph') and
service.queries.getGraphRevision.subscribe, introduce a local let
firstSeenRevision: number | undefined, set it on the first callback invocation,
and only invoke onChange when revision > firstSeenRevision; keep using the
existing unsubscribe variable to tear down the subscription as before.
🪄 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: 71c2a7ce-de2b-412f-9b23-e9d31b394bac
📒 Files selected for processing (4)
code/addons/review/src/preset.test.tscode/addons/review/src/preset.tscode/core/src/core-server/index.tscode/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.ts
💤 Files with no reviewable changes (1)
- code/core/src/shared/open-service/services/module-graph/engine/module-graph-engine.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/core-server/index.ts
- code/addons/review/src/preset.test.ts
Comment on lines
+41
to
+49
| const service = getService<typeof moduleGraphServiceDef>('core/module-graph'); | ||
| // Omit the input to watch the entire graph. The initial emission carries | ||
| // revision 0 (or the current revision at subscribe time); only subsequent | ||
| // advances represent a change after the review was cached. | ||
| unsubscribe = service.queries.getGraphRevision.subscribe(undefined, (revision) => { | ||
| if (revision > 0) { | ||
| onChange(); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Ignore the initial graph-revision snapshot.
Line 45 treats any non-zero revision as a change, but this subscription can immediately emit the current revision. That means a late attach can mark a just-pushed review stale even though nothing changed after the review was cached. Record the first seen revision and only react to later increases.
Suggested fix
const defaultSubscribeToModuleGraphChanges: SubscribeToModuleGraphChanges = (onChange) => {
let unsubscribe: () => void = () => {};
let cancelled = false;
+ let lastRevision: number | undefined;
void import('storybook/internal/core-server')
.then(({ getService }) => {
if (cancelled) {
return;
}
const service = getService<typeof moduleGraphServiceDef>('core/module-graph');
// Omit the input to watch the entire graph. The initial emission carries
// revision 0 (or the current revision at subscribe time); only subsequent
// advances represent a change after the review was cached.
unsubscribe = service.queries.getGraphRevision.subscribe(undefined, (revision) => {
- if (revision > 0) {
+ if (lastRevision === undefined) {
+ lastRevision = revision;
+ return;
+ }
+ if (revision > lastRevision) {
+ lastRevision = revision;
onChange();
}
});
})🤖 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/addons/review/src/preset.ts` around lines 41 - 49, The subscription to
service.queries.getGraphRevision currently treats any revision > 0 as a change
and can fire immediately with the current revision; instead record the first
seen revision and only call onChange for later increases. In the block using
getService<typeof moduleGraphServiceDef>('core/module-graph') and
service.queries.getGraphRevision.subscribe, introduce a local let
firstSeenRevision: number | undefined, set it on the first callback invocation,
and only invoke onChange when revision > firstSeenRevision; keep using the
existing unsubscribe variable to tear down the subscription as before.
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
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.
Summary by CodeRabbit
New Features