feat(docgen): re-extract docgen on source change (server-side)#34998
Closed
JReinhold wants to merge 4 commits into
Closed
Conversation
…vice Server-side docgen invalidation: when a watched source file changes, the already-extracted docgen for affected components is re-extracted so the core/docgen service never serves stale data, with no flash to empty for current readers. - Add core/module-graph open service: a thin facade that maps affected story files to component ids, backed by the existing change-detection dependency graph. - Add core/docgen handleSourceChange command: re-extract-if-present (skip components not in state), keep-last-good on extraction error. - ChangeDetectionService: add an additive onInvalidate callback and a publishStatuses=false graph-only mode so the dependency graph can power docgen without surfacing review-changes statuses; existing status behavior is unchanged when publishStatuses is true. - Wire it in dev-server/common-preset: run the graph when changeDetection OR experimentalDocgenServer is enabled (server-side only; no manager transport). Co-authored-by: Jeppe Reinhold <JReinhold@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
JReinhold
changed the base branch from
main
to
jeppe-cursor/docgen-subscription-referential-equality-5a81
Contributor
|
…-referential-equality-5a81' into jeppe-cursor/docgen-invalidation-fbe7 Co-authored-by: Jeppe Reinhold <JReinhold@users.noreply.github.com>
…back Aligns the new module-graph service with the setState arg rename from the base branch (draft -> state). Co-authored-by: Jeppe Reinhold <JReinhold@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughAdds a module-graph open service to map changed story files to component IDs, wires it to the docgen open service to re-extract affected components, and extends ChangeDetectionService with a graph-only mode and an onInvalidate callback that emits affected story files. ChangesSource Change Invalidation Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 2
🧹 Nitpick comments (1)
code/core/src/shared/open-service/services/docgen/server.test.ts (1)
142-227: ⚡ Quick winConsider mocking the logger for better test isolation and guideline compliance.
The new test suite exercises code that calls
logger.warn()(server.ts line 92), but the logger is not mocked. According to the coding guidelines, file dependencies should be mocked usingvi.mock()withspy: true. Mocking the logger would:
- Allow verification that
logger.warn()is called in the error case (lines 198-225)- Prevent console output during test runs
- Improve test isolation
- Align with the guideline: "Mock all required dependencies that the test subject uses"
As per coding guidelines, mocks should be placed at the top of the file before test cases with the
spy: trueoption, and accessed viavi.mocked().Example implementation
Add at the top of the file (after imports, before
afterEach):+vi.mock('../../../../node-logger/index.ts', () => ({ + spy: true, +})); + +import { logger } from '../../../../node-logger/index.ts'; + afterEach(() => { clearRegistry(); + vi.mocked(logger.warn).mockClear(); });Then verify logging in the error test:
shouldThrow = false; expect(service.queries.getDocgen({ componentId: 'button' })).toMatchObject({ description: 'good', }); + + expect(vi.mocked(logger.warn)).toHaveBeenCalledWith( + expect.stringContaining('failed to re-extract docgen for "button"') + ); });🤖 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/docgen/server.test.ts` around lines 142 - 227, Mock the logger used by the service so logger.warn calls are spied and do not print during tests: add a vi.mock(...) with { spy: true } near the top of the test file for the module that exports logger, then use vi.mocked(logger).warn (or vi.mocked(logger)) in the failing re-extraction test ("keeps the last-good payload when re-extraction throws") to assert it was called (and/or to silence output); ensure the mock is created before registerDocgenService is invoked so the service picks up the mocked logger.
🤖 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/ChangeDetectionService.ts`:
- Around line 572-576: The onInvalidate callback invocation can throw
synchronously and prevent scheduleScan from running, so wrap the call to
this.onInvalidate([...affectedStoryFiles]) in a try/catch and ensure
this.scheduleScan(this.debounceMs) runs in a finally block; specifically update
the block that checks if (this.onInvalidate && affectedStoryFiles.size > 0)
inside handleFileChange to catch any error from this.onInvalidate (log or ignore
the error) and then always call this.scheduleScan(this.debounceMs) in finally so
the rescan remains best-effort even if onInvalidate fails.
In `@code/core/src/core-server/dev-server.ts`:
- Around line 94-97: The onInvalidate handler currently fire-and-forgets
invalidateDocgenForStoryFiles, causing overlapping handleSourceChange runs and
order-dependent cache races; change the handler to serialize invalidations by
awaiting invalidateDocgenForStoryFiles (or enqueueing calls) so each batch
completes before the next starts—update the onInvalidate callback that currently
calls invalidateDocgenForStoryFiles to return/await its promise (or implement a
simple promise queue around invalidateDocgenForStoryFiles/handleSourceChange) to
ensure sequential execution and stable docgen cache updates.
---
Nitpick comments:
In `@code/core/src/shared/open-service/services/docgen/server.test.ts`:
- Around line 142-227: Mock the logger used by the service so logger.warn calls
are spied and do not print during tests: add a vi.mock(...) with { spy: true }
near the top of the test file for the module that exports logger, then use
vi.mocked(logger).warn (or vi.mocked(logger)) in the failing re-extraction test
("keeps the last-good payload when re-extraction throws") to assert it was
called (and/or to silence output); ensure the mock is created before
registerDocgenService is invoked so the service picks up the mocked logger.
🪄 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: ddac0362-f0b4-4aba-9b2d-ece2bf9c3e78
📒 Files selected for processing (12)
code/core/src/core-server/change-detection/ChangeDetectionService.test.tscode/core/src/core-server/change-detection/ChangeDetectionService.tscode/core/src/core-server/dev-server.tscode/core/src/core-server/presets/common-preset.tscode/core/src/shared/open-service/services/docgen/definition.tscode/core/src/shared/open-service/services/docgen/invalidation.integration.test.tscode/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/server.test.tscode/core/src/shared/open-service/services/module-graph/server.tscode/core/src/shared/open-service/services/module-graph/types.ts
Comment on lines
+572
to
576
| if (this.onInvalidate && affectedStoryFiles.size > 0) { | ||
| this.onInvalidate([...affectedStoryFiles]); | ||
| } | ||
|
|
||
| this.scheduleScan(this.debounceMs); |
Contributor
There was a problem hiding this comment.
Guard onInvalidate so callback failures cannot skip the rescan.
If onInvalidate throws synchronously here, handleFileChange() exits before Line 576. The surrounding patchQueue.catch(() => undefined) then swallows the rejection, so that file change is neither rescanned nor logged. Wrap the callback in its own try/catch and keep scheduleScan() in a finally so invalidation stays best-effort.
Suggested fix
- if (this.onInvalidate && affectedStoryFiles.size > 0) {
- this.onInvalidate([...affectedStoryFiles]);
- }
-
- this.scheduleScan(this.debounceMs);
+ try {
+ if (this.onInvalidate && affectedStoryFiles.size > 0) {
+ this.onInvalidate([...affectedStoryFiles]);
+ }
+ } catch (error) {
+ logger.warn(
+ `Change detection: onInvalidate failed for ${path}: ${error instanceof Error ? error.message : String(error)}`
+ );
+ } finally {
+ this.scheduleScan(this.debounceMs);
+ }🤖 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/ChangeDetectionService.ts` around
lines 572 - 576, The onInvalidate callback invocation can throw synchronously
and prevent scheduleScan from running, so wrap the call to
this.onInvalidate([...affectedStoryFiles]) in a try/catch and ensure
this.scheduleScan(this.debounceMs) runs in a finally block; specifically update
the block that checks if (this.onInvalidate && affectedStoryFiles.size > 0)
inside handleFileChange to catch any error from this.onInvalidate (log or ignore
the error) and then always call this.scheduleScan(this.debounceMs) in finally so
the rescan remains best-effort even if onInvalidate fails.
…scription Replace the dev-server orchestration (which called module-graph then docgen) with an open-service-native link: docgen subscribes to core/module-graph's getLastAffected query and re-extracts affected components on change. The dev server now only feeds the module graph (the one imperative edge from the non-open-service change detector); it no longer references docgen. The module-graph revision (bumped on every change) ensures repeated changes to the same component still emit distinct values, so subscription value-dedup does not swallow a repeat change. This is an explicit subscription because re-extraction is async and the runtime fires a query's load only once (it does not re-run load on a tracked-dependency change). A future reactive-load primitive would let getDocgen depend on the revision directly and drop this wiring. Co-authored-by: Jeppe Reinhold <JReinhold@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/core/src/core-server/dev-server.ts (1)
88-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize invalidation batches.
This still fire-and-forgets a state-mutating async invalidation. If two source-change batches arrive close together,
resolveAffectedComponents()and the downstream docgen refresh can complete out of order and leave the cache on an older extraction.Suggested fix
+ let invalidationQueue = Promise.resolve(); + const changeDetectionService = new ChangeDetectionService({ storyIndexGeneratorPromise, statusStore: getStatusStoreByTypeId(CHANGE_DETECTION_STATUS_TYPE_ID), workingDir, @@ onInvalidate: experimentalDocgenServer ? (affectedStoryFiles) => { - void recordAffectedStoryFiles(affectedStoryFiles); + invalidationQueue = invalidationQueue + .then(() => recordAffectedStoryFiles(affectedStoryFiles)) + .catch(() => undefined); } : 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/core/src/core-server/dev-server.ts` around lines 88 - 92, The onInvalidate handler currently fire-and-forgets recordAffectedStoryFiles which can run concurrently and reorder async state mutations; change the onInvalidate callback to serialize batches by awaiting prior work before starting the next: make recordAffectedStoryFiles return a Promise if it doesn't already, add a small per-handler queue/lock (e.g., a pendingPromise or mutex) inside the dev-server scope and have onInvalidate chain/await that pending promise before invoking recordAffectedStoryFiles, and ensure resolveAffectedComponents()/docgen refresh are awaited so each invalidation completes in order.
🧹 Nitpick comments (2)
code/core/src/shared/open-service/services/docgen/invalidation.integration.test.ts (2)
124-149: ⚡ Quick winMake the repeat-emission test keep the payload constant.
Right now this changes
descriptionon every invalidation, so the subscriber would update even without the revision-based dedup bypass. Keeping the returned docgen payload stable and asserting the emission count grows would actually cover the behavior named in the test.Suggested adjustment
- let version = 1; const provider: DocgenProvider = async () => ({ componentId: 'button', name: 'Button', - description: `v${version}`, + description: 'stable', props: [], }); @@ - await vi.waitFor(() => expect(emitted.at(-1)).toBe('v1')); + await vi.waitFor(() => expect(emitted).toEqual(['stable'])); - version = 2; await moduleGraph.commands.resolveAffectedComponents({ storyFiles: [absStoryFile('button')] }); - await vi.waitFor(() => expect(emitted.at(-1)).toBe('v2')); + await vi.waitFor(() => expect(emitted).toHaveLength(2)); - version = 3; await moduleGraph.commands.resolveAffectedComponents({ storyFiles: [absStoryFile('button')] }); - await vi.waitFor(() => expect(emitted.at(-1)).toBe('v3')); + await vi.waitFor(() => expect(emitted).toHaveLength(3));🤖 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/docgen/invalidation.integration.test.ts` around lines 124 - 149, The test should keep the docgen payload identical across invalidations and assert that the subscriber still re-emits; change the DocgenProvider (the provider variable) to always return the same description/props/name (e.g., description: 'stable') regardless of the version counter, continue bumping version to simulate internal revision changes, then replace the assertions that check emitted.at(-1) values with assertions that the subscriber emission count increases (e.g., check emitted.length increments or equals the expected number after each resolveAffectedComponents call) while using the same subscribe call to docgen.queries.getDocgen and triggers via moduleGraph.commands.resolveAffectedComponents.
111-116: 🏗️ Heavy liftReplace the timer tick with a real synchronization point.
setTimeout(0)does not prove the module-graph → docgen subscription finished. This can pass even ifhandleSourceChangeruns later and incorrectly calls the provider, so the test can miss regressions.🤖 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/docgen/invalidation.integration.test.ts` around lines 111 - 116, The test's final setTimeout(0) is an unreliable sync point; replace it with a deterministic wait for the docgen subscription/handleSourceChange to finish by awaiting a real observable or mock invocation instead: after calling moduleGraph.commands.resolveAffectedComponents and asserting getLastAffected, wait for the docgen provider to be invoked (or for a completion promise/event exposed by the docgen subscription/handleSourceChange) using vi.waitFor or awaiting that promise so the test only continues once handleSourceChange has truly run and the provider call (the action you care about) has occurred.
🤖 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.
Duplicate comments:
In `@code/core/src/core-server/dev-server.ts`:
- Around line 88-92: The onInvalidate handler currently fire-and-forgets
recordAffectedStoryFiles which can run concurrently and reorder async state
mutations; change the onInvalidate callback to serialize batches by awaiting
prior work before starting the next: make recordAffectedStoryFiles return a
Promise if it doesn't already, add a small per-handler queue/lock (e.g., a
pendingPromise or mutex) inside the dev-server scope and have onInvalidate
chain/await that pending promise before invoking recordAffectedStoryFiles, and
ensure resolveAffectedComponents()/docgen refresh are awaited so each
invalidation completes in order.
---
Nitpick comments:
In
`@code/core/src/shared/open-service/services/docgen/invalidation.integration.test.ts`:
- Around line 124-149: The test should keep the docgen payload identical across
invalidations and assert that the subscriber still re-emits; change the
DocgenProvider (the provider variable) to always return the same
description/props/name (e.g., description: 'stable') regardless of the version
counter, continue bumping version to simulate internal revision changes, then
replace the assertions that check emitted.at(-1) values with assertions that the
subscriber emission count increases (e.g., check emitted.length increments or
equals the expected number after each resolveAffectedComponents call) while
using the same subscribe call to docgen.queries.getDocgen and triggers via
moduleGraph.commands.resolveAffectedComponents.
- Around line 111-116: The test's final setTimeout(0) is an unreliable sync
point; replace it with a deterministic wait for the docgen
subscription/handleSourceChange to finish by awaiting a real observable or mock
invocation instead: after calling moduleGraph.commands.resolveAffectedComponents
and asserting getLastAffected, wait for the docgen provider to be invoked (or
for a completion promise/event exposed by the docgen
subscription/handleSourceChange) using vi.waitFor or awaiting that promise so
the test only continues once handleSourceChange has truly run and the provider
call (the action you care about) has occurred.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 189fe21e-7e39-4aad-a222-b4772d39fa3f
📒 Files selected for processing (4)
code/core/src/core-server/dev-server.tscode/core/src/core-server/presets/common-preset.tscode/core/src/shared/open-service/services/docgen/invalidation.integration.test.tscode/core/src/shared/open-service/services/docgen/server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/shared/open-service/services/docgen/server.ts
- code/core/src/core-server/presets/common-preset.ts
8 tasks
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 #
What I did
Server-side docgen invalidation (HMR) for the experimental docgen server: when a watched source file changes, the already-extracted docgen for affected components is re-extracted so
core/docgennever serves stale data — with no flash to empty for current readers and no stale data for later navigations.This is the server-side slice only (no manager transport, which doesn't exist yet). Plan: Server-side docgen invalidation (HMR) — plan (subpage of SB docgen server).
Design
Two halves:
state.components[componentId], deep-signal reactivity re-emits to subscribers (deduped by value), and future reads get fresh data via the normal sync-read +load. So we only have to update server state correctly.(input) => Promise<DocgenPayload | undefined>); they get no change-notification API. The existing oxc dependency graph inchange-detection/maps a changed file to its affected story files.Invalidation policy = re-extract-if-present (locked decision, "option b"): on invalidation for a component id, if it is already in state, re-run
extractDocgen(atomic overwrite → reactive emit, deduped); if absent, do nothing (lazy on next read); on extraction error, keep last-good.Changes
core/module-graphopen service — a thin facade that translates affected story files into component ids, backed by the change-detection dependency graph. Holds a monotonicrevision+ the latest affected component ids in state, exposed via the subscribablegetLastAffectedquery.core/docgenhandleSourceChangecommand — re-extract-if-present + keep-last-good.core/docgensubscribes tocore/module-graph'sgetLastAffected(connectDocgenToModuleGraph, wired in theservicespreset) and re-extracts affected components. The dev server only feeds the module graph; it has no knowledge of docgen. Therevisionbump ensures repeated changes to the same component still emit (value-dedup doesn't swallow them). This is an explicit subscription because re-extraction is async and the runtime fires a query'sloadonly once — a future reactive-load primitive would letgetDocgendepend on the revision directly and drop the subscription.ChangeDetectionService(see separated section in the plan; owned by another stakeholder) — two additive, gated changes:onInvalidatecallback emitting affected story files (fires onadd/change/unlink, independent of git diff and of the "dep set unchanged" patch early-return).publishStatuses: falsegraph-only mode so the dependency graph can power docgen without surfacing "review changes" sidebar statuses. Existing behavior is unchanged whenpublishStatusesistrue(the default).dev-server.ts,common-preset.ts) — run the graph whenfeatures.changeDetection || features.experimentalDocgenServer; the dev server'sonInvalidatecalls onlymodule-graph.resolveAffectedComponents(best-effort).Non-regression
Builder-agnostic and Webpack-supported: both builder-vite (chokidar
server.watcher) and builder-webpack5 (compiler.hooks.watchRun) ship change-detection adapters, so this does not regress Webpack docgen updates. The change is additive and behindexperimentalDocgenServer.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
The change is server-side only — there is no manager transport yet, so there is no UI to exercise, and the phase-1 mock provider is path-deterministic (its output doesn't change on edits). Validation is via automated tests plus a live dev-server smoke test confirming a file edit drives the reactive chain (
onInvalidate→module-graph→ docgen subscription →handleSourceChange):core/docgenhandleSourceChange: re-extracts present components and pushes the new value to a subscriber (content-sensitive provider); ignores absent components; keeps last-good on provider error.core/module-graph: story files → component ids mapping; revision bump.connectDocgenToModuleGraph, feed the module graph, and assert docgen re-extracts + the subscriber is notified reactively (incl. repeated same-component changes via the revision).ChangeDetectionService:onInvalidatefires with affected story files; graph-only mode publishes no statuses and never calls git.Commands run:
yarn test --run open-service change-detection(237 passed),yarn nx check core(no type errors), lint + format.Documentation
Checklist for Maintainers
ci:normal,ci:mergedorci:daily.feature request.Summary by CodeRabbit
New Features
Tests