Open Service: Fix reactivity on deep signals, fire subscribers on load dependencies#34979
Open
JReinhold wants to merge 9 commits into
Open
Conversation
…on re-emit Co-authored-by: Jeppe Reinhold <JReinhold@users.noreply.github.com>
Contributor
Re-engage alien-signals' identity-based dedup in subscribeToQuery by memoizing the computed: return the previous reference when the new handler output is deeply equal. Output-schema validation and Immer both mint fresh references for unchanged data, which previously re-fired subscribers on every state write (including writes to unrelated keys and loads that rewrite a deeply-equal payload). Co-authored-by: Jeppe Reinhold <JReinhold@users.noreply.github.com>
cursor
Bot
changed the title
…te-only Replace the single alien-signals state atom + Immer with a deepsignal deep reactive proxy (backed by @preact/signals-core). State mutates in place inside a batch; subscriptions track per-field reads so an unrelated key/field write never re-runs a subscriber's handler. Schemas now validate shape only and never convert: validation output is discarded and the original reference is returned. A dev-only check throws OpenServiceSchemaConversionError when a schema transforms/strips/coerces. subscribe() gains an optional selector (universal-store pattern): the callback receives the selected slice and fires only when it changes by value. Emissions are deduped by value via es-toolkit isEqual; query results and emissions are detached plain snapshots so the proxy never escapes the runtime.
cursor
Bot
changed the title
Output validation now runs only on pull boundaries (query()/.loaded()/ static build) and on whole-value subscription emissions, never narrowing a selector subscriber's reactive footprint. Because validation no longer runs where it would broaden tracking, schemas may transform/coerce again: reverted the no-conversion contract and removed OpenServiceSchemaConversionError. With a selector, the computed reads only the selected slice, so an unselected sibling-field change no longer re-runs the handler (regression guard added). Snapshotting: structuredClone for the plain whole-state snapshot; JSON only to strip live proxy slices for selectors. Renamed the docgen-flavored test fixture to concept-based naming (rebuiltEqualValueOnLoadServiceDef) to match the existing convention.
cursor
Bot
changed the title
…es & tests - Selector subscriptions now validate output too: validation runs untracked so it catches shape errors without broadening the reactive footprint (regression-guarded). Fixes output validation being skipped when a selector was passed. - createServiceRuntime no longer clones initialState; the clone moves to registerService (the static build already clones), removing a double clone while still protecting a definition's shared initialState. - Remove the false-positive 'different record' subscription test (covered better by the handler-spy fine-grained test) and merge the two selector tests into one emit+recompute test. - Drop conversion framing from README/JSDoc; validation is described as it always behaved.
The draft naming was an Immer-ism; with the deep-signal proxy the callback receives the live state, so name it accordingly across the type, fixtures, docgen service, debug service, tests, and README.
A query's load now re-fires for active subscriptions whenever the external signals it reads synchronously change (same-service or cross-service via getService), turning it into a reactive async resource. Implemented by running the load inside its own effect() so its reads are ambiently tracked; loads with no external synchronous reads still fire exactly once, so existing services are unaffected. Superseding: each run carries an epoch and writes through epoch-gated commands, so a slow stale load cannot overwrite a newer run's state. Coalescing: changes batched together produce a single re-load. Direct query()/.loaded() calls keep one-shot semantics. The load effect is torn down with the subscription. Loads are now contractually idempotent warming steps; one-shot side effects belong in commands.
JReinhold
added
maintenance
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors open-service runtime from alien-signals/immer to deepsignal deep reactive proxy; moves output validation to call sites; adds selector-capable subscriptions and gated reactive loads; exposes getStateSnapshot(); renames setState callback parameter (draft → state); updates fixtures, tests, docs, and service handlers. ChangesOpen Service Reactivity and State Model
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
🤖 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/shared/open-service/service-runtime.test.ts`:
- Line 327: Replace fixed sleeps (e.g., the await new Promise(resolve =>
setTimeout(resolve, 50)) calls in service-runtime.test.ts) with deterministic
waits using the test runner's wait utilities (vi.waitFor or similar) that wait
for a concrete condition: check a spy/called count, assert the expected
length/content of the emitted array, or wait for a specific state change. Locate
the instances in the tests where those setTimeout sleeps are used and swap them
for vi.waitFor(() => expect(spy).toHaveBeenCalledTimes(n)) or vi.waitFor(() =>
expect(emitted).toEqual([...])) so the test completes when the observable/spy
reaches the expected state rather than after a fixed time.
In `@code/core/src/shared/open-service/service-runtime.ts`:
- Around line 1024-1027: The selector branch currently calls selector(output) on
raw handler output, bypassing schema validation/coercion; change it to run
validateQueryOutput(refs, queryName, queryDef, output) inside the existing
untracked call, capture the validated/coerced value (e.g., const validated =
validateQueryOutput(...)), then call selector(validated) and return
detachSnapshot(selector(validated)). Keep the untracked wrapper and existing
references (selector, untracked, validateQueryOutput, detachSnapshot, output,
queryName, queryDef, refs) so selector subscribers receive the same
validated/coerced TOutput as query()/ .loaded().
🪄 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: 436b89f1-058f-47ba-a54b-14de09619f8a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
code/.storybook/open-service-debug-service.tscode/core/package.jsoncode/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/server.test-d.tscode/core/src/shared/open-service/server.test.tscode/core/src/shared/open-service/server.tscode/core/src/shared/open-service/service-registration.test.tscode/core/src/shared/open-service/service-registration.tscode/core/src/shared/open-service/service-runtime.test.tscode/core/src/shared/open-service/service-runtime.tscode/core/src/shared/open-service/services/docgen/server.tscode/core/src/shared/open-service/types.ts
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 74 | 🚨 +2 🚨 |
| Self size | 20.41 MB | 20.38 MB | 🎉 -25 KB 🎉 |
| Dependency size | 36.11 MB | 36.65 MB | 🚨 +539 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 203 | 205 | 🚨 +2 🚨 |
| Self size | 908 KB | 908 KB | 🎉 -144 B 🎉 |
| Dependency size | 88.60 MB | 89.11 MB | 🚨 +514 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 198 | 🚨 +2 🚨 |
| Self size | 32 KB | 32 KB | 🚨 +36 B 🚨 |
| Dependency size | 87.09 MB | 87.60 MB | 🚨 +514 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 75 | 🚨 +2 🚨 |
| Self size | 1.08 MB | 1.08 MB | 0 B |
| Dependency size | 56.52 MB | 57.03 MB | 🚨 +514 KB 🚨 |
| Bundle Size Analyzer | node | node |
…orybookjs/storybook into jeppe-cursor/docgen-subscription-referential-equality-5a81 # Conflicts: # code/core/src/shared/open-service/service-runtime.test.ts # code/core/src/shared/open-service/service-runtime.ts
JReinhold
changed the title
Selector subscribers now receive schema-validated slices. A tracked selector(output) read preserves fine-grained proxy dependencies because validation returns a plain parsed value.
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 #
Works on #34824
What I did
Reworked the open-service reactive core to use per-field deep reactivity instead of one coarse state atom. This fixes docgen-style double emissions on re-navigation when data is unchanged, makes subscriptions precise, and makes a query's async
loadreactive to its synchronous dependencies.State and writes
alien-signalsatom + Immer withdeepSignal(deepsignal) backed by@preact/signals-core. Readingctx.self.state.<field>tracks only that field (including record keys added later).setState((state) => …)mutates the proxy in place insidebatch(); only changed fields invalidate dependents.Subscriptions
selectoronsubscribe(universal-store pattern):subscribe(input, selector, callback). The selector narrows reactive reads so unrelated sibling fields do not re-run the handler.es-toolkitisEqual. Emissions are detached plain values; whole-state snapshots usestructuredClone, with JSON detach only for proxy slices from selectors.Validation
query(),.loaded(), static build, subscription emit), not on the reactive path. Selector subscribers validate untracked so validation cannot broaden the tracked footprint.Reactive
loadloadre-runs when external signals it reads synchronously change (same-service fields or cross-service viagetService). Loads are idempotent warming steps; one-shot side effects belong in commands. Superseded runs are epoch-gated so stale writes cannot clobber newer results. Directquery()/.loaded()remain one-shot.Dependencies
alien-signals; addeddeepsignaland@preact/signals-core.Demonstrated behavior (docgen-style emulation: provider returns fresh-but-equal payloads on navigation):
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!
No manual testing is required. Changes are confined to the internal open-service runtime under
code/core/src/shared/open-service/. Coverage is in the Vitest suite (yarn test open-service), including fine-grained handler-spy tests, selector tests, equal-value dedup, and reactive load scenarios (same-service and cross-service deps, coalescing, superseding in-flight loads, non-subscriptionquery()one-shot behavior).Documentation
MIGRATION.MD
Updated
code/core/src/shared/open-service/README.md(state/reactivity model, validation placement, load semantics, subscription flow including selector and reactive load).Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This 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
New Features
Improvements
Documentation
Tests
Chores