Open-Service: Introduce query states (loading/error/success)#35214
Open
JReinhold wants to merge 12 commits into
Open
Open-Service: Introduce query states (loading/error/success)#35214JReinhold wants to merge 12 commits into
JReinhold wants to merge 12 commits into
Conversation
Introduce QueryState (data plus the load lifecycle: status/loadStatus/error and derived booleans, inspired by TanStack Query) as the single value emitted by a subscription. Replace the bare auto-loading `query(input)` call with a synchronous `query.get(input)` that performs no implicit load, and make `subscribe()` emit a QueryState synchronously on first call. Thread per-service query types into handler/load contexts so `self.queries` infers exact types without manual casts. Co-authored-by: Cursor <cursoragent@cursor.com>
…directly The hook now accepts the Query object (e.g. `myService.queries.getThing`) so input/output types infer per query, and returns the full QueryState so consumers can read `isLoading`, `isError`, etc. The first render is seeded synchronously from `query.get()` wrapped in a pending state, then the real lifecycle arrives from the subscription's first emission. Update the public exports accordingly. Co-authored-by: Cursor <cursoragent@cursor.com>
The preview-side docs hooks (useServiceDocgen, useServiceStory and wrappers) return QueryState and are reimplemented without useSyncExternalStore so they keep working on React 16. ArgTypes/Controls render the ArgsTable loading skeleton via isInitialLoading, and the manager ControlsPanel reflects the load lifecycle. Rename useServiceDocgen.ts to use-service-docgen.ts. Co-authored-by: Cursor <cursoragent@cursor.com>
Update change-detection, the docgen server, the sync-test demo services/stories and the internal debug service to read via `query.get()` and to consume the QueryState payload now emitted by `subscribe()`. Co-authored-by: Cursor <cursoragent@cursor.com>
Migrate unit and type tests to the QueryState API and add coverage for the synchronous first emission, error / last-data retention, reactive dependency warming and `self.queries` type inference. Update the open-service README to document `query.get()`, `subscribe()`'s QueryState payload and the hook shape. Co-authored-by: Cursor <cursoragent@cursor.com>
JReinhold
added
maintenance
Contributor
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 72 | 0 |
| Self size | 21.11 MB | 21.19 MB | 🚨 +74 KB 🚨 |
| Dependency size | 36.42 MB | 36.42 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 204 | 204 | 0 |
| Self size | 802 KB | 802 KB | 0 B |
| Dependency size | 91.46 MB | 91.54 MB | 🚨 +74 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 | 89.95 MB | 90.03 MB | 🚨 +74 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 1.08 MB | 1.08 MB | 0 B |
| Dependency size | 57.54 MB | 57.61 MB | 🚨 +74 KB 🚨 |
| Bundle Size Analyzer | node | node |
Contributor
Author
|
CI note for the reviewer (re: Chromatic): The full test suite is green. The only failing checks are the two Chromatic visual checks on These diffs are expected — this PR intentionally changes those UI surfaces: the manager All other sandboxes' Chromatic UI Tests are "unchanged". |
…ries The Description/Source service stories mock `core/story-docs` but never enabled the `experimentalDocgenServer` feature that gates the service path in the blocks. The flag is off by default in production builds (Chromatic) and a plain `yarn storybook:ui`, so the mocked data was never rendered and the stories failed visually. Enable the feature per-story in `beforeEach` (restored on cleanup), mirroring the existing `global.FEATURES` pattern. Co-authored-by: Cursor <cursoragent@cursor.com>
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Open Service query API changes from callable functions to a method-based interface: ChangesOpen Service Query API Overhaul
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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/addons/docs/src/blocks/blocks/Source.tsx (1)
179-225: 💤 Low valueMinor: story resolution logic is duplicated between
SourceWithStorySnippetanduseSourceProps.Lines 200-210 mirror lines 116-128 — both resolve the story from
ofviadocsContext.resolveOf. The duplication is a reasonable trade-off to lift the feature-flag decision to the component boundary (avoiding conditional hooks), anduseMemoprevents redundant computation. Consider extracting a shareduseResolvedStory(of, docsContext)hook if this pattern expands to more components.🤖 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/docs/src/blocks/blocks/Source.tsx` around lines 179 - 225, The story resolution logic that handles the of prop and fallback to the current story (using docsContext.resolveOf and docsContext.storyById) is duplicated between the SourceWithStorySnippet component's useMemo block and the useSourceProps hook. Extract this logic into a shared custom hook named useResolvedStory that accepts of and docsContext as parameters and returns the resolved story, then use this hook in both the useMemo within SourceWithStorySnippet and in useSourceProps to eliminate the duplication while maintaining the current behavior and memoization benefits.
🤖 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/docs/src/blocks/blocks/use-service-docgen.ts`:
- Around line 34-35: The condition at line 34 comparing cache.current?.id !== id
can evaluate to false when both are undefined, causing the cache initialization
within the if block to be skipped on the first render. This leads to
cache.current.state being accessed at line 45 before the cache has been
initialized. Fix this by explicitly ensuring cache.current is initialized before
line 45 where cache.current.state is accessed. Add an explicit check before the
state access to initialize cache.current with seedQueryState if it hasn't been
initialized yet, or restructure the condition at line 34 to guarantee
initialization occurs regardless of whether id is undefined.
In `@code/core/src/shared/open-service/service-runtime.ts`:
- Around line 679-682: The triggerLoad function call in the queryDef.load block
is using rethrowAsync in the catch handler, which rethrows dependency-load
failures onto the global microtask queue and can cause uncaught global errors
instead of keeping errors contained within the query lifecycle state. Remove the
.catch(rethrowAsync) handler from the triggerLoad call to prevent
dependency-warm failures from escaping the query lifecycle and becoming global
uncaught errors.
---
Nitpick comments:
In `@code/addons/docs/src/blocks/blocks/Source.tsx`:
- Around line 179-225: The story resolution logic that handles the of prop and
fallback to the current story (using docsContext.resolveOf and
docsContext.storyById) is duplicated between the SourceWithStorySnippet
component's useMemo block and the useSourceProps hook. Extract this logic into a
shared custom hook named useResolvedStory that accepts of and docsContext as
parameters and returns the resolved story, then use this hook in both the
useMemo within SourceWithStorySnippet and in useSourceProps to eliminate the
duplication while maintaining the current behavior and memoization benefits.
🪄 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: 5f677b66-28ef-4544-a36a-d06167d09d8c
📒 Files selected for processing (47)
code/.storybook/open-service-debug-service.tscode/addons/docs/src/blocks/blocks/ArgTypes.tsxcode/addons/docs/src/blocks/blocks/Controls.tsxcode/addons/docs/src/blocks/blocks/Description.stories.tsxcode/addons/docs/src/blocks/blocks/Description.tsxcode/addons/docs/src/blocks/blocks/Source.stories.tsxcode/addons/docs/src/blocks/blocks/Source.tsxcode/addons/docs/src/blocks/blocks/argTypesShared.tscode/addons/docs/src/blocks/blocks/use-service-docgen.tscode/addons/docs/src/blocks/blocks/use-service-story-docs.tscode/addons/docs/src/blocks/blocks/useServiceDocgen.tscode/core/src/controls/components/ControlsPanel.stories.tsxcode/core/src/controls/components/ControlsPanel.tsxcode/core/src/core-server/change-detection/change-detection-service.tscode/core/src/core-server/change-detection/change-detection.test-helpers.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/fixtures.tscode/core/src/shared/open-service/index.test-d.tscode/core/src/shared/open-service/index.tscode/core/src/shared/open-service/manager.tscode/core/src/shared/open-service/preview.tscode/core/src/shared/open-service/query-state.tscode/core/src/shared/open-service/server.test-d.tscode/core/src/shared/open-service/server.test.tscode/core/src/shared/open-service/service-command-transport.test.tscode/core/src/shared/open-service/service-definition.tscode/core/src/shared/open-service/service-registration-sync.test.tscode/core/src/shared/open-service/service-registration.test.tscode/core/src/shared/open-service/service-runtime.test.tscode/core/src/shared/open-service/service-runtime.tscode/core/src/shared/open-service/service-transport-leaf.test.tscode/core/src/shared/open-service/service-validation.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/server.test.tscode/core/src/shared/open-service/services/story-docs/server.test.tscode/core/src/shared/open-service/static-fetch.test.tscode/core/src/shared/open-service/sync-test/addon/panels/OpenServiceDemoPanel.tsxcode/core/src/shared/open-service/sync-test/local-command/local-command.stories.tsxcode/core/src/shared/open-service/sync-test/local-command/server.tscode/core/src/shared/open-service/sync-test/remote-command/remote-command.stories.tsxcode/core/src/shared/open-service/sync-test/remote-command/server.tscode/core/src/shared/open-service/sync-test/static-load/static-load.stories.tsxcode/core/src/shared/open-service/types.tscode/core/src/shared/open-service/use-service-command.test.tsxcode/core/src/shared/open-service/use-service-query.test.tsxcode/core/src/shared/open-service/use-service-query.ts
💤 Files with no reviewable changes (1)
- code/addons/docs/src/blocks/blocks/useServiceDocgen.ts
Unlike TanStack Query, subscriptions here can attach to queries whose data is already cached in service state. Require data === undefined in buildQueryState so consumers do not flash initial-load skeletons over already-available data. Co-authored-by: Cursor <cursoragent@cursor.com>
Move query reads, loads, the drain loop, and subscriptions into query-runtime.ts so service-runtime.ts stays focused on runtime assembly. Collapse the three duplicate get() implementations into createQueryGet with per-surface load policies. Co-authored-by: Cursor <cursoragent@cursor.com>
Share the preview-side subscription seed/subscribe logic between useServiceDocgen and useServiceStory. Call getService directly in render instead of wrapping it in useMemo. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/docs/src/blocks/blocks/use-query-subscription.ts`:
- Around line 18-20: The effect hook in useQuerySubscription has an incomplete
dependency list that does not include the selector parameter, causing stale
subscriptions when the selector function prop changes. Add selector to the
effect's dependency array (currently containing query and cacheKey) and ensure
the cache key computation also tracks selector identity to guarantee
re-subscription occurs whenever the selector changes, preventing stale selected
state from persisting when boundSelector or similar selector references are
updated.
In `@code/addons/docs/src/blocks/blocks/use-service-story-docs.ts`:
- Around line 41-48: The useQuerySubscription call in this function only uses
storyId as the cache key, but doesn't account for changes to the selector
parameter. When the selector changes for the same story, useQuerySubscription
does not re-seed or re-subscribe because the cache key remains unchanged,
resulting in stale QueryState.data being returned. Fix this by incorporating the
selector identity (such as a reference to boundSelector or a computed hash of
it) into the cache key passed to useQuerySubscription alongside storyId, so that
selector changes trigger proper re-subscription.
In `@code/core/src/shared/open-service/query-runtime.ts`:
- Around line 944-948: The epoch value is not being invalidated during cleanup,
allowing existing isCurrent() closures to remain true and potentially execute
stale commands after unsubscribe. In the loadTeardown effect, increment the
epoch variable during the cleanup phase (when the effect is torn down) to ensure
all pending operations with old epoch values will immediately return false from
isCurrent() checks, preventing stale state writes after unsubscribe. This
applies to both occurrences of this pattern referenced in the review comment.
- Around line 92-114: The stableHash function has a collision vulnerability
where both undefined values and the literal string "__undefined__" produce
identical hash outputs, causing unrelated data loads to share promises. Fix this
by making the replacement value for undefined values collision-resistant in the
JSON.stringify replacer function within stableHash. Instead of replacing
undefined with the string "__undefined__", use a marker approach that ensures
undefined values cannot collide with any other input (such as wrapping in an
object with a type indicator or using a sufficiently unique string marker that
cannot occur naturally from other inputs). This ensures makeLoadKey generates
unique keys for different validatedInput values.
🪄 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: 29d93cb6-d541-4f9c-bc40-698a661d0e18
📒 Files selected for processing (9)
code/addons/docs/src/blocks/blocks/use-query-subscription.tscode/addons/docs/src/blocks/blocks/use-service-docgen.tscode/addons/docs/src/blocks/blocks/use-service-story-docs.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/query-runtime.tscode/core/src/shared/open-service/query-state.tscode/core/src/shared/open-service/service-runtime.test.tscode/core/src/shared/open-service/service-runtime.tscode/core/src/shared/open-service/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- code/core/src/shared/open-service/README.md
- code/core/src/shared/open-service/query-state.ts
- code/core/src/shared/open-service/service-runtime.test.ts
The extraction hot-refresh tracked extracted component ids in a separate Set because the old bare query call fired the load behind the scenes. Now that .get() is a pure read that never loads, read the component query directly so the service state is the single source of truth. Co-authored-by: Cursor <cursoragent@cursor.com>
A function selector cannot be folded into the string cacheKey, so track it by reference as a real subscription dependency: changing the selector for the same key now re-seeds and re-subscribes instead of returning a stale selected slice. Co-authored-by: Cursor <cursoragent@cursor.com>
The old replacer substituted undefined with the literal '__undefined__', which aliased an actual string input of that value (and optional object fields). Tag undefined and objects with a discriminator so no two distinct inputs share a key. Co-authored-by: Cursor <cursoragent@cursor.com>
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
1 participant
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
Introduces states to the open service's subscription model so the UI can tell whether a query's data is loading, succeeded, or errored — previously a subscriber only ever received the bare value, so it could not render loading/error states. The shape is heavily inspired by TanStack Query's
useQuery, adapted to ourloadvocabulary (our "loads" are any slow async work, not just remote fetching).QueryStatesubscribe()anduseServiceQuerynow surface aQueryStateinstead of a bare value:dataplus theloadlifecycle (status,loadStatus,error) and derived booleans.dataandstatusare independent:dataholds the last successful value (kept across a failed re-load), whilestatus/loadStatus/errordescribe the asyncloadlifecycle.Query runtime API: bare call →
query.get()The confusing bare
query(input)call that returned synchronously and fired theloadbehind the scenes is removed. Reads are now an explicit, side-effect-freequery.get(input).Before
After
useServiceQuery: pass the query, receive aQueryStateBefore
After
A void-input query needs no input argument (
useServiceQuery(query)); the input is only positional once a selector is involved (useServiceQuery(query, undefined, selector)). The service must exist — guard at a parent and conditionally render rather than passing an absent service.Preview-side docs hooks return
QueryStateuseServiceDocgen/useServiceStory(anduseServiceStoryDoc/useServiceStorySnippet) now return aQueryStateso docs blocks can readisInitialLoading.ArgTypesandControlsuse this to render the existingArgsTableloading skeleton, matching the manager'sControlsPanel. These hooks are reimplemented withoutuseSyncExternalStorebecause the preview side must keep supporting React 16. The file is renameduseServiceDocgen.ts→use-service-docgen.ts.self.queriestype inferenceQuery types are now threaded through handler/load contexts, so
self.queries.someQuery.get(input)infers exact input/output types without manual casts (previouslyself.querieswasRecord<string, Query<unknown, unknown>>, matching howself.commandsalready worked).Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
isInitialLoadingis wired through. The rows should then render normally.get is not a function).Areas most worth extra scrutiny: the synchronous first emission of
subscribe(), retention of the last successfuldataacross a failed re-load, and the React 16-compatible preview hooks (nouseSyncExternalStore).Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake 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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Release Notes
data) for more consistent UI updates.get/loaded/subscribelifecycle and query-state model..get()access patterns.