Open Service: Add sync between server, manager and preview#35017
Merged
Conversation
- Added a new background color service with manager, preview, and server components. - Integrated service registration in Storybook's manager and preview files. - Established a communication channel for state synchronization between manager and preview. - Created tests for service registration and subscription behavior.
- Added a new background color service with manager, preview, and server components. - Integrated service registration in Storybook's manager and preview files. - Established a communication channel for state synchronization between manager and preview. - Created tests for service registration and subscription behavior.
ndelangen
force-pushed
the
norbert/service-clients
branch
from
0309863 to
d4dd5a6
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces a multi-master open-service client architecture: browsers (manager + preview) and the server each run a full local ServiceRuntime and keep state in sync over Storybook's manager↔preview channel through a welcome-handshake + patch-broadcast protocol. Adds React hooks for consuming services and an example background-color service to demonstrate end-to-end sync.
Changes:
- New client/channel modules (
service-client.ts,service-channel.ts,service-server-channel.ts) plusclient.ts/manager.ts/preview.tsentrypoints, with multi-master sync, loop prevention viaclientId, anddeepAssign-based state application. - New React hooks (
useServiceQueryviauseSyncExternalStorewithisEqualbailout,useServiceCommand) with tests. - Wires server-registered services into the channel in
experimental_serverChannel, and adds abackground-serviceexample registered incode/.storybookmanager/preview/server presets.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| code/core/src/shared/open-service/service-channel.ts | Channel interface, event/payload constants, global channel slot, generateClientId. |
| code/core/src/shared/open-service/service-client.ts | Client registry, registerServiceClient, welcome/patch protocol, command wrapping. |
| code/core/src/shared/open-service/service-server-channel.ts | Server-side opt-in channel participation. |
| code/core/src/shared/open-service/service-registration.ts | Exposes getServiceRuntime for server channel wiring. |
| code/core/src/shared/open-service/use-service-query.ts | React hook with useSyncExternalStore + isEqual snapshot stability. |
| code/core/src/shared/open-service/use-service-command.ts | React hook returning a stable command reference. |
| code/core/src/shared/open-service/{client,manager,preview}.ts | Entrypoint barrels for each environment. |
| code/core/src/shared/open-service/{service-client,use-service-query,use-service-command}.test.* | Tests for client sync, query hook, and command hook. |
| code/core/src/shared/open-service/README.md | Documents client architecture, channel protocol, and hooks. |
| code/core/src/core-server/presets/common-preset.ts | Connects every registered service to the dev server channel. |
| code/.storybook/services-preset.ts | Registers the example background service. |
| code/.storybook/{preview.tsx,manager.tsx} | Side-effect imports for the example service. |
| code/.storybook/background-service/{definition,server,preview,manager}.* | Example background-color service across all three runtimes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements cross-peer service synchronization for Storybook's open-service framework. It adds channel-based multi-master state reconciliation with snapshot syncing, introduces public API entrypoints (client, manager, preview, server), implements React hooks for query and command access, wires manager bootstrap and channel installation, includes comprehensive protocol and transport tests, demonstrates the architecture with a background-color example service, and expands documentation on architecture and testing practices. ChangesOpen-Service Multi-Master Sync & Example
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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: 9
🧹 Nitpick comments (1)
code/core/src/shared/open-service/use-service-query.test.tsx (1)
110-128: ⚡ Quick winAssert render count here, not just reference identity.
This currently passes even if the hook rerenders on a deep-equal emission, because
result.currentcan still end up pointing atfirstRefafter that extra render. Add a render counter so this test actually locks down the “no rerender” contract.Proposed test tightening
it('maintains referential stability when result is deeply equal', async () => { const service = registerServiceClient(mutableRecordLookupServiceDef); await service.commands.assignRecordField({ entryId: 'a', fieldKey: 'k', fieldValue: 'v' }); - const { result } = renderHook(() => - useServiceQuery(service, 'getRecordFields', { entryId: 'a' }) - ); + let renderCount = 0; + const { result } = renderHook(() => { + renderCount++; + return useServiceQuery(service, 'getRecordFields', { entryId: 'a' }); + }); const firstRef = result.current; + const countAfterMount = renderCount; // Assign the same value again — deeply equal, so no re-render. await service.commands.assignRecordField({ entryId: 'a', fieldKey: 'k', fieldValue: 'v' }); // Wait a tick to let any spurious re-renders fire. await new Promise<void>((resolve) => setTimeout(resolve, 20)); expect(result.current).toBe(firstRef); + expect(renderCount).toBe(countAfterMount); });🤖 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/use-service-query.test.tsx` around lines 110 - 128, The test currently only asserts referential stability by comparing result.current to firstRef, which can falsely pass if an extra render occurs and then returns the same reference; modify the test that uses renderHook and useServiceQuery (with registerServiceClient and mutableRecordLookupServiceDef and service.commands.assignRecordField) to also track the number of renders by using the renderHook result's rerender counter or a local renderCount variable updated inside the hook callback, then assert that renderCount did not increase after assigning the same deep-equal value so the test enforces "no rerender" rather than just identical reference identity.
🤖 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/.storybook/background-service/manager.tsx`:
- Around line 40-53: The swatch button in manager.tsx (the <button> using props
label, onClick, value, active) lacks an accessible name/state; add an explicit
accessible name and state by setting aria-label={label} (or aria-labelledby if
you prefer) and expose selection using aria-pressed={active} (or aria-checked
with role="radio" if these are part of a radio group), and also ensure
type="button" is set so it won’t submit forms unintentionally; keep the existing
title for mouse hover but rely on aria attributes for screen readers.
In `@code/.storybook/background-service/preview.ts`:
- Around line 29-33: The subscription in
backgroundService.queries.getColor.subscribe is directly mutating
document.body.style.background which conflicts with ThemedSetRoot's reset;
instead, have the subscriber set a dedicated CSS custom property (e.g.,
--story-background-color) on document.documentElement or a stable container
element, or call a setter on ThemedSetRoot/theming API so the background flows
through the same styling path; update ThemedSetRoot (or the theme provider) to
read that CSS variable or accept the service-driven value so preview rerenders
and theme updates won't overwrite the service-chosen color.
In `@code/.storybook/services-preset.ts`:
- Around line 7-12: The preset comment is out of sync with implementation:
registerBackgroundService() is invoked unconditionally inside the exported
services function, but the comment implies STORYBOOK_OPEN_SERVICE_DEBUG toggles
"all examples." Either update the comment to state that
STORYBOOK_OPEN_SERVICE_DEBUG only gates the debug service (not
registerBackgroundService), or change the implementation to guard
registerBackgroundService() with the environment check
(process.env.STORYBOOK_OPEN_SERVICE_DEBUG) so that registerBackgroundService()
only runs when that variable is truthy; refer to the exported services function
and the registerBackgroundService symbol when making the change.
In `@code/core/src/shared/open-service/manager.ts`:
- Around line 36-39: Remove the re-export that pulls preview-only dependencies
into the manager public entrypoint: delete the line exporting everything from
'./preview.ts' in manager.ts and instead re-export the intended public API from
a module that does not import storybook/preview-api (for example, re-export the
same symbols from './client.ts' or another safe module). Specifically, remove
"export * from './preview.ts'" and add re-exports that expose only the public
hooks (e.g., alongside the existing "export { useServiceCommand }" and "export {
useServiceQuery }" ensure any other intended exports come from './client.ts' or
a non-preview module), leaving preview.ts isolated so it no longer influences
manager.ts.
In `@code/core/src/shared/open-service/README.md`:
- Around line 382-392: The two fenced code blocks containing ASCII diagrams in
README.md are missing language identifiers and trigger markdownlint MD040;
update each opening ``` to include a language tag like ```text for both ASCII
diagram blocks (the block showing the Manager process / Preview process diagram
and the block showing "Peer A (manager) Channel Peer B
(preview)" flow) so the fenced blocks become ```text ... ``` to satisfy the
linter; apply the same change to the later matching pair of blocks as well (the
second occurrence referenced in the comment).
In `@code/core/src/shared/open-service/service-client.ts`:
- Around line 123-148: deepAssign currently leaves keys present in target but
missing from source, preventing deletions from propagating; update deepAssign so
it first removes any keys from target that are not present in source, then for
each key in source perform the existing logic (recurse for plain objects,
replace arrays/primitives), ensuring deletions in snapshots are applied; refer
to the deepAssign(target: Record<string, unknown>, source: Record<string,
unknown>) function and adjust its loop to delete absent keys from target before
assigning or recursing.
- Around line 272-277: The onWelcomeReply handler currently applies every
welcome reply and can regress state; modify onWelcomeReply (and/or surrounding
registration logic) to only apply the first valid welcome reply: check payload
as WelcomeReplyPayload and after the first successful
applyReceivedState(p.state) (only when p.serviceId === definition.id &&
p.clientId !== ownClientId) unregister or remove the listener or set a boolean
flag (e.g., welcomeApplied) to ignore subsequent replies so delayed/stale
replies are ignored.
In `@code/core/src/shared/open-service/service-server-channel.ts`:
- Around line 37-53: The deepAssign function must ignore prototype-pollution
keys to prevent merging dangerous properties; update deepAssign to skip any key
equal to "__proto__", "constructor", or "prototype" before recursing or
assigning to target (both in the branch that calls deepAssign(tv, sv) and in the
else assignment), keeping the rest of the recursive logic intact for other keys.
- Around line 85-103: The onWelcomeRequest and onPatches handlers currently cast
payload: unknown to WelcomeRequestPayload/PatchesPayload and immediately
dereference fields, which can throw on malformed input; update both handlers
(onWelcomeRequest, onPatches) to first validate that payload is a non-null
object and contains the expected keys with appropriate types (e.g., typeof
serviceId === 'string' and typeof clientId === 'string'; for PatchesPayload also
ensure state is an object) before reading fields, and bail out (or log) if
validation fails; keep the existing guards (serviceId/ownClientId) and
subsequent actions (emit SERVICE_WELCOME_REPLY, runtime.commandSelf.setState and
deepAssign) unchanged once payload is validated.
---
Nitpick comments:
In `@code/core/src/shared/open-service/use-service-query.test.tsx`:
- Around line 110-128: The test currently only asserts referential stability by
comparing result.current to firstRef, which can falsely pass if an extra render
occurs and then returns the same reference; modify the test that uses renderHook
and useServiceQuery (with registerServiceClient and
mutableRecordLookupServiceDef and service.commands.assignRecordField) to also
track the number of renders by using the renderHook result's rerender counter or
a local renderCount variable updated inside the hook callback, then assert that
renderCount did not increase after assigning the same deep-equal value so the
test enforces "no rerender" rather than just identical reference identity.
🪄 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: 607a082e-c2ec-4bd7-994c-9ce740d7e602
📒 Files selected for processing (21)
code/.storybook/background-service/definition.tscode/.storybook/background-service/manager.tsxcode/.storybook/background-service/preview.tscode/.storybook/background-service/server.tscode/.storybook/manager.tsxcode/.storybook/preview.tsxcode/.storybook/services-preset.tscode/core/src/core-server/presets/common-preset.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/client.tscode/core/src/shared/open-service/manager.tscode/core/src/shared/open-service/preview.tscode/core/src/shared/open-service/service-channel.tscode/core/src/shared/open-service/service-client.test.tscode/core/src/shared/open-service/service-client.tscode/core/src/shared/open-service/service-registration.tscode/core/src/shared/open-service/service-server-channel.tscode/core/src/shared/open-service/use-service-command.test.tsxcode/core/src/shared/open-service/use-service-command.tscode/core/src/shared/open-service/use-service-query.test.tsxcode/core/src/shared/open-service/use-service-query.ts
…zation - Updated service registration to automatically join the cross-peer sync protocol without a separate connect step. - Introduced a schema validation mechanism for state synchronization to ensure integrity across runtimes. - Implemented last-write-wins logic for concurrent state updates, improving conflict resolution. - Added comprehensive tests for service registration, state synchronization, and validation behavior. - Removed deprecated service-server-channel integration, consolidating functionality into service-sync.
- Added a new project configuration for the open-service internal demo in Playwright. - Updated Storybook's preview components to synchronize body background color with the open-service background demo. - Introduced a new event for when the preview iframe finishes loading, improving state synchronization. - Enhanced the Viewport component to conditionally render the iframe based on addon loading state. - Added end-to-end tests for the open-service background demo to ensure consistent behavior after reloads.
- Removed the PREVIEW_IFRAME_LOADED_EVENT export from the postMessage channel as it is no longer utilized. - Simplified the IFrame component by eliminating the event dispatch for iframe load completion. - Updated the PostMessageTransport class to remove references to the now-removed event. - Enhanced the Viewport component to streamline iframe rendering logic. - Adjusted the provider module to clean up state management related to addon loading.
- Updated ManagerProvider to run addon register callbacks before the first render, ensuring manager-side listeners are ready for preview JS events. - Refactored the provider module to remove unused API handling logic during initialization. - Simplified open-service tests by consolidating welcome-request retry logic and improving clarity in test cases for welcome-reply handling. - Adjusted end-to-end tests to validate the reload bootstrap path and ensure synchronization between manager and preview components.
- Updated README to clarify the merging of `staticInputs` and handler overrides in service registration. - Improved type safety in `server.test-d.ts` by adding type expectations for input and context in service handlers. - Adjusted service registration tests to ensure handlers and load hooks are correctly defined on service definitions. - Enhanced type definitions in `types.ts` to reflect the correct structure for service query registrations. - Updated docgen service definition to ensure the load hook is correctly associated with the query definition.
…a81' of github.com:storybookjs/storybook into norbert/service-clients
ndelangen
changed the title
JReinhold
reviewed
Jun 3, 2026
JReinhold
reviewed
Jun 3, 2026
JReinhold
reviewed
Jun 3, 2026
JReinhold
reviewed
Jun 3, 2026
JReinhold
reviewed
Jun 3, 2026
JReinhold
reviewed
Jun 3, 2026
JReinhold
reviewed
Jun 3, 2026
JReinhold
reviewed
Jun 3, 2026
JReinhold
reviewed
Jun 3, 2026
…ybook into norbert/service-clients
ndelangen
commented
Jun 3, 2026
ndelangen
commented
Jun 3, 2026
ndelangen
commented
Jun 3, 2026
…t and WebsocketTransport Updated both PostMessageTransport and WebsocketTransport classes to utilize a local CHANNEL_OPTIONS constant instead of directly referencing globalThis.CHANNEL_OPTIONS. This change improves code clarity and consistency in accessing channel options across the transport implementations.
Enhanced the README documentation for the remote command execution section, clarifying the roles of requester and responder, the command invocation process, and the event structure. This update aims to provide clearer guidance on how commands are executed across different runtimes, improving overall understanding for developers.
…okjs/storybook into norbert/remote-command-execution
…cution OpenService: Implement remote command execution
JReinhold
added
core
qa:skip
Resolve docgen service conflicts by adopting next's id-based API and getDocgenForAllComponents while keeping load hooks on the service definition per the restricted registration model on this branch. Co-authored-by: Cursor <cursoragent@cursor.com>
JReinhold
removed
the
ci:normal
JReinhold
changed the title
…types - Consolidate channel wiring behind a single connectServiceToChannel entry point and capture one channel reference (removes a broadcast/listener channel-divergence hazard). - Use nanoid for the identity-critical client/call id instead of Math.random. - Drop spurious `as unknown as Channel` casts in the addon stores. - Move @preact/signals-core and deepsignal to devDependencies so they are bundled into core's dist (fixes resolution in symlinked sandboxes). - Fix useServiceQuery input typing by inferring TInput/TOutput as direct type parameters so the void-vs-input arg branch reads the concrete query type. Co-authored-by: Cursor <cursoragent@cursor.com>
…n state to objects - Replace hand-rolled channel-payload parsers with Valibot schemas in service-channel.ts; derive payload types from the schemas and narrow with v.safeParse in the transport (removes the duplicated parse/isRecord helpers). - Constrain service state to a plain object at the defineService boundary via a ServiceState type (rejects primitives, null, and arrays), matching the deep-signal / deep-reconcile requirements. Add type tests and document it. Co-authored-by: Cursor <cursoragent@cursor.com>
JReinhold
added
the
ci:normal
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Post-merge update after taking over this PR:
Validation done locally for the latest demo work:
|
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
JReinhold
approved these changes
Jun 6, 2026
The setup-file ordering / context.project.config change is unrelated test infra and does not belong in the open-service PR. Restore the next version. Co-authored-by: Cursor <cursoragent@cursor.com>
The save-from-controls Playwright test failed once in CI with stale preview text, but passes consistently locally (including CI=true). Retrigger checks. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Re: the outdated review thread on |
This was referenced Jun 6, 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
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.
Tracked by: #34824
What I did
This PR ships open-service client runtimes — multi-master state sync across the dev server, manager, and preview — plus the bootstrap fixes and authoring-model tightening needed to make it reliable in real Storybook.
Problem
Open-service works well on the server (registry, static builds, docgen extraction), but manager and preview had no way to run the same reactive query/command surface or stay in sync when state changes. After a full reload, we also hit a race where the preview iframe registered and sent
welcome-requestbefore the manager had installed channel listeners, so preview state could stay stale while the manager toolbar looked correct.Solution (three layers)
1. Multi-master sync (core)
Each runtime (server, manager, preview) runs a full
ServiceRuntimefrom the sameServiceDefinition. There is no RPC proxy and no single writer.(version, clientId)in the channel envelope (never inside user state)services:prefix:services:welcome-request/services:welcome-reply— bootstrap handshakeservices:patches— post-mutation whole-state broadcastconnectRuntimeToChannel) — no separate connect stepservice-transport.ts; merge/order rules inservice-sync.tsNew client surface:
registerServiceClient/ manager & previewregisterServicebarrelsuseServiceQuery—useSyncExternalStore-backed, value-dedupeduseServiceCommand— stable bound async function (no built-in pending/error state)setServiceChannel/getServiceChannel— one channel install per entry pointFirst real consumer:
core/docgenservice (services/docgen/) wired through theservicespreset whenfeatures.experimentalDocgenServeris enabled.2. Reload bootstrap ordering (regression fix)
Fixed: manager toolbar shows dark after reload, preview canvas stays light.
provider.handleAPI→loadAddonssynchronously inManagerProviderconstructor, before iframesrcis setload≠ channel ready)version > 0emit oneservices:patcheswhen joiningsrcgate /PREVIEW_IFRAME_LOADED_EVENTDemo fixes:
setColorhandler on shareddefinition.ts(multi-master — any peer that invokes a command needs the handler);ThemedSetRootno longer fightsdocument.bodybackground.3. Definition vs registration layer (authoring model)
Aligned the type system and merge logic with how clients actually register (
registerServiceClient(definition)— no registration object):defineService)handler,load,staticPath, optionalstaticInputsstaticInputsonly (story index / registry context)handleronly (server-only deps)ServiceQueryRegistrationno longer acceptshandlerorload.applyRegistrationmerges onlystaticInputsfor queries. DocgengetDocgen.loadmoved todefinition.ts; server registration keepsstaticInputs+extractDocgen.handler.Planned follow-up (not in this PR): fire
loadon subscribe and.loaded()only, not on every syncquery()call.Known gap (documented, not a regression)
Load-driven push: command wrapping broadcasts external command calls, but mutations made inside a query's
loadhook (via rawctx.self.commands) are not broadcast to peers. Fixing this needs a local-mutation hook + microtask coalescing — scoped as deliberate follow-up.Internal demo
code/.storybook/background-service/(toolbar + preview subscribe)open-service-internalproject →open-service-background.spec.tsChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Unit / type tests (
code/core/src/shared/open-service/):Full open-service suite (excludes
use-service-query.test.tsx— OOMs in some environments, pre-existing):E2E regression (reload sync — manager + preview background color after full page reload):
Or CI-style:
CI=1 yarn playwright test --project=open-service-internalBuild:
Manual testing
cd code && yarn nx compile core && yarn storybook:ui#1B1C1D)features.experimentalDocgenServerand verify docgen extraction still works on the server pathDocumentation
Updated:
code/core/src/shared/open-service/README.mdChecklist for Maintainers
ci:normal,ci:mergedorci:dailyGH labelfeature requestReviewer guide
Start here:
code/core/src/shared/open-service/README.mdservice-sync.ts+service-transport.ts— protocol invariants (LWW, relay, loop prevention)service-client.ts— client registration + welcome/patches listenersservice-registration.ts— server auto-sync at registrationcode/core/src/manager-api/root.tsx— synchronousloadAddons(reload fix)code/core/src/channels/postmessage/index.ts— inbound flushcode/.storybook/background-service/— end-to-end democode/e2e-tests/open-service-background.spec.ts— regression gateKey design choices:
useServiceCommandreturns bare async fn, not react-query mutation shapestaticInputs(+ command handlers for server-only work)Explicitly not shipped / rejected:
srcgate until manager hub readyPREVIEW_IFRAME_LOADED_EVENThandler/loadat server registration timeloadtriggering (follow-up)Summary by CodeRabbit
New Features
useServiceQueryanduseServiceCommandfor consuming service state and commands.Documentation