Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions code/core/src/shared/open-service/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,23 @@ Incoming state (from sync-start-reply or patches) is applied via `serviceRuntime

Rather than replacing the entire state object on each patch (which would invalidate all signal subscriptions), `applyStatePatch` (in [service-sync.ts](./service-sync.ts)) recursively merges plain-object values in place: arrays and primitives are replaced directly, `__proto__`/`constructor`/`prototype` are skipped to block prototype pollution, and `preserveMissingKeys` controls whether missing keys are deleted. Cross-peer sync passes `false` so deletions propagate from full snapshots; static JSON loading passes `true` because each static file is a partial snapshot. This keeps fine-grained subscriptions on unaffected nested fields from firing spuriously.

### `localStateKeys`

A definition can name top-level state keys that never leave the runtime owning them:

```ts
defineService({
id: 'core/module-graph',
initialState: { graphRevision: 0, storiesByFile: {} },
localStateKeys: ['storiesByFile'],
// ...
});
```

Listed keys are dropped from every snapshot before it is cloned for the channel, and `applyStatePatch` neither deletes nor overwrites them when a peer snapshot arrives, so each runtime keeps its own copy. Everything else syncs normally.

Reach for this when a service holds large derived data that only the owning runtime queries. `core/module-graph` is the motivating case: it is registered on the server alone, and its reverse index runs to millions of `(file, story)` pairs on a large project β€” syncing it meant a full clone plus a telejson serialization on every command, for a payload no peer ever read. Anything a peer does need must stay off the list.

### State sync sequence

```text
Expand Down
1 change: 1 addition & 0 deletions code/core/src/shared/open-service/service-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ export const defineService = <
description?: string;
internal?: boolean;
initialState: ServiceState<TState>;
localStateKeys?: readonly (keyof TState & string)[];
queries: DefinedQueries<
TState,
TQueryInputSchemas,
Expand Down
7 changes: 6 additions & 1 deletion code/core/src/shared/open-service/service-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,14 +258,19 @@ export function registerService<
// Owns the per-service last-write-wins stamp and the adopt/advance logic. Adopting a peer snapshot
// goes through `commandSelf.setState` β€” not the wrapped commands below β€” which is how the broadcast
// loop is prevented.
// State this runtime owns exclusively: kept out of every snapshot that goes on the channel, and
// never clobbered by one arriving from a peer.
const { localStateKeys } = resolvedDefinition;

const reconciler = createSnapshotReconciler({
setState: (mutate) =>
runtime.commandSelf.setState((state) => mutate(state as Record<string, unknown>)),
initialStamp: { version: 0, clientId: ownClientId },
localStateKeys,
});

const getSnapshot = (): Record<string, unknown> =>
runtime.getStateSnapshot() as Record<string, unknown>;
runtime.getStateSnapshot(localStateKeys) as Record<string, unknown>;

const descriptor = describeDefinition(resolvedDefinition as AnyServiceDefinition);

Expand Down
23 changes: 20 additions & 3 deletions code/core/src/shared/open-service/service-runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,12 @@ export type ServiceRuntime<
TQueries extends Queries<TState>,
TCommands extends Commands<TState>,
> = {
/** Returns a plain, detached snapshot of the current state for serialization. */
getStateSnapshot(): TState;
/**
* Returns a plain, detached snapshot of the current state for serialization. `omitKeys` leaves
* the named top-level keys out without cloning them, which is how a definition's
* `localStateKeys` stay off the channel.
*/
getStateSnapshot(omitKeys?: readonly string[]): TState;
Comment on lines +109 to +114

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟑 Minor | ⚑ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 3 '\.getStateSnapshot\s*\(' code/core/src

Repository: storybookjs/storybook

Length of output: 2066


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- service-runtime declarations and implementation ---'
sed -n '80,135p' code/core/src/shared/open-service/service-runtime.ts
sed -n '280,335p' code/core/src/shared/open-service/service-runtime.ts

printf '%s\n' '--- getStateSnapshot call sites with context ---'
rg -n -C 8 '\.getStateSnapshot\s*\(' code/core/src/shared/open-service

printf '%s\n' '--- related state and localStateKeys declarations ---'
rg -n -C 5 'localStateKeys|ServiceRuntime|TState' code/core/src/shared/open-service/service-runtime.ts code/core/src/shared/open-service/service-registry.ts code/core/src/shared/open-service/*.ts

Repository: storybookjs/storybook

Length of output: 50378


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- ServiceDefinition and localStateKeys types ---'
rg -n -C 8 'localStateKeys' code/core/src/shared/open-service/types.ts code/core/src/shared/open-service/service-definition.ts code/core/src/shared/open-service

printf '%s\n' '--- exact snapshot implementation and call-site declarations ---'
sed -n '300,325p' code/core/src/shared/open-service/service-runtime.ts
sed -n '255,276p' code/core/src/shared/open-service/service-registry.ts
sed -n '100,112p' code/core/src/shared/open-service/server.ts

printf '%s\n' '--- all non-test snapshot calls ---'
rg -n '\.getStateSnapshot\s*\(' code/core/src/shared/open-service -g '*.ts' -g '!*.test.ts'

Repository: storybookjs/storybook

Length of output: 30852


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- repository-wide snapshot API usage ---'
rg -n -C 2 '\b(getStateSnapshot|ServiceRuntime)\b' . -g '*.ts' -g '*.tsx' -g '*.js' -g '*.jsx' -g '!node_modules'

printf '%s\n' '--- runtime behavior for omitted keys ---'
node - <<'JS'
const rawState = {
  requiredValue: 1,
  localValue: { large: true },
};

const getStateSnapshot = (omitKeys) => {
  if (!omitKeys?.length) {
    return structuredClone(rawState);
  }

  const kept = {};
  for (const [key, value] of Object.entries(rawState)) {
    if (!omitKeys.includes(key)) kept[key] = value;
  }
  return structuredClone(kept);
};

const full = getStateSnapshot();
const omitted = getStateSnapshot(['localValue']);

if (full.requiredValue !== 1 || full.localValue?.large !== true) {
  throw new Error('No-argument snapshot did not preserve the full state');
}
if ('localValue' in omitted || omitted.requiredValue !== 1) {
  throw new Error('Omitted-key snapshot did not remove only the requested key');
}
console.log(JSON.stringify({ full, omitted }));
JS

printf '%s\n' '--- compiler availability for an isolated overload assignability check ---'
if command -v tsc >/dev/null 2>&1; then
  tsc --version
  tmpdir="$(mktemp -d)"
  trap 'rm -rf "$tmpdir"' EXIT
  cat >"$tmpdir/check.ts" <<'TS'
type State = { requiredValue: number; localValue: object };

type Runtime = {
  getStateSnapshot(): State;
  getStateSnapshot(omitKeys: readonly string[] | undefined): Partial<State>;
};

const implementation = (omitKeys?: readonly string[]): State => ({
  requiredValue: 1,
  localValue: {},
});

const runtime: Runtime = { getStateSnapshot: implementation };
const full: State = runtime.getStateSnapshot();
const partial: Partial<State> = runtime.getStateSnapshot(['localValue']);
void [full, partial];
TS
  tsc --strict --noEmit --skipLibCheck "$tmpdir/check.ts"
else
  echo 'tsc unavailable'
fi

Repository: storybookjs/storybook

Length of output: 5698


Correct the getStateSnapshot return type.

A non-empty omitKeys list can remove a required top-level key. Keep the no-argument overload as TState, and return Partial<TState> for calls that provide the optional omission list.

Proposed type fix
-  getStateSnapshot(omitKeys?: readonly string[]): TState;
+  getStateSnapshot(): TState;
+  getStateSnapshot(omitKeys: readonly string[] | undefined): Partial<TState>;
πŸ“ 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
/**
* Returns a plain, detached snapshot of the current state for serialization. `omitKeys` leaves
* the named top-level keys out without cloning them, which is how a definition's
* `localStateKeys` stay off the channel.
*/
getStateSnapshot(omitKeys?: readonly string[]): TState;
/**
* Returns a plain, detached snapshot of the current state for serialization. `omitKeys` leaves
* the named top-level keys out without cloning them, which is how a definition's
* `localStateKeys` stay off the channel.
*/
getStateSnapshot(): TState;
getStateSnapshot(omitKeys: readonly string[] | undefined): Partial<TState>;
πŸ€– 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/service-runtime.ts` around lines 109 - 114,
Update the getStateSnapshot overload in the service runtime interface so calls
without arguments still return TState, while calls supplying omitKeys return
Partial<TState>. Preserve the existing snapshot behavior and use overloads to
distinguish the two call signatures.

commandSelf: CommandSelf<TState>;
queryCtx: QueryCtx<TState>;
loadCtxForStatic: LoadCtx<TState>;
Expand Down Expand Up @@ -301,7 +305,20 @@ export function createServiceRuntime<
// The deep reactive proxy is the single source of truth that query computations track, at
// per-field granularity.
const state = deepSignal(rawState as object) as TState;
const getStateSnapshot = (): TState => structuredClone(rawState);
const getStateSnapshot = (omitKeys?: readonly string[]): TState => {
if (!omitKeys?.length) {
return structuredClone(rawState);
}
// Drop the omitted keys before cloning rather than after β€” cloning them is the cost being
// avoided, and for a large derived index it dominates the whole snapshot.
const kept: Record<string, unknown> = {};
for (const [key, value] of Object.entries(rawState as Record<string, unknown>)) {
if (!omitKeys.includes(key)) {
kept[key] = value;
}
}
return structuredClone(kept) as TState;
};
const commandSelf = createCommandSelf(state);
const { registryApi, staticLoader } = runtimeOptions;
const createCommandCtx = (): CommandCtx<TState> => ({
Expand Down
40 changes: 40 additions & 0 deletions code/core/src/shared/open-service/service-sync.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,44 @@ describe('applyStatePatch', () => {

expect(target).toEqual({ entries: { alpha: 'updated' } });
});

it('keeps a local key that a full peer snapshot omits', () => {
const target = { graphRevision: 1, storiesByFile: { './a.ts': { './a.stories.ts': 1 } } };
// Local keys are stripped before broadcast, so a peer snapshot never carries them back.
const source = { graphRevision: 2 };

applyStatePatch(target as Record<string, unknown>, source as Record<string, unknown>, {
preserveMissingKeys: false,
localStateKeys: ['storiesByFile'],
});

expect(target).toEqual({
graphRevision: 2,
storiesByFile: { './a.ts': { './a.stories.ts': 1 } },
});
});

it('ignores a local key a peer sends anyway', () => {
const target = { storiesByFile: { './a.ts': { './a.stories.ts': 1 } } };
const source = { storiesByFile: { './b.ts': { './b.stories.ts': 1 } } };

applyStatePatch(target as Record<string, unknown>, source as Record<string, unknown>, {
preserveMissingKeys: false,
localStateKeys: ['storiesByFile'],
});

expect(target).toEqual({ storiesByFile: { './a.ts': { './a.stories.ts': 1 } } });
});

it('does not treat a nested key as local just because it shares a name', () => {
const target = { outer: { storiesByFile: 'nested', other: 'gone' }, storiesByFile: 'local' };
const source = { outer: { storiesByFile: 'replaced' } };

applyStatePatch(target as Record<string, unknown>, source as Record<string, unknown>, {
preserveMissingKeys: false,
localStateKeys: ['storiesByFile'],
});

expect(target).toEqual({ outer: { storiesByFile: 'replaced' }, storiesByFile: 'local' });
});
});
26 changes: 22 additions & 4 deletions code/core/src/shared/open-service/service-sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ function isPlainObject(value: unknown): value is Record<string, unknown> {
export function applyStatePatch(
target: Record<string, unknown>,
source: Record<string, unknown>,
options: { preserveMissingKeys: boolean }
options: { preserveMissingKeys: boolean; localStateKeys?: readonly string[] }
): void {
if (!options.preserveMissingKeys) {
// Remove keys the source no longer carries (deletion propagation).
Expand All @@ -97,6 +97,12 @@ export function applyStatePatch(
continue;
}

// Local keys are stripped from every outgoing snapshot, so their absence here says nothing
// about the peer's intent β€” deleting them would discard this runtime's own data.
if (options.localStateKeys?.includes(key)) {
continue;
}

if (!Object.prototype.hasOwnProperty.call(source, key)) {
delete target[key];
}
Expand All @@ -109,11 +115,19 @@ export function applyStatePatch(
continue;
}

// A peer has no authority over a local key, even if one somehow reaches us.
if (options.localStateKeys?.includes(key)) {
continue;
}

const sourceValue = source[key];
const tarvalue = target[key];

if (isPlainObject(sourceValue) && isPlainObject(tarvalue)) {
applyStatePatch(tarvalue, sourceValue, options);
// `localStateKeys` names top-level keys only, so it must not follow the recursion down.
applyStatePatch(tarvalue, sourceValue, {
preserveMissingKeys: options.preserveMissingKeys,
});
} else if (tarvalue !== sourceValue) {
target[key] = sourceValue;
}
Expand Down Expand Up @@ -157,8 +171,10 @@ export type SnapshotReconciler = {
export function createSnapshotReconciler(options: {
setState: (mutate: StateMutator) => void;
initialStamp: SyncStamp;
/** Top-level state keys this runtime owns exclusively; never adopted from or deleted by a peer. */
localStateKeys?: readonly string[];
}): SnapshotReconciler {
const { setState, initialStamp } = options;
const { setState, initialStamp, localStateKeys } = options;
let localStamp = initialStamp;

return {
Expand All @@ -177,7 +193,9 @@ export function createSnapshotReconciler(options: {
}

localStamp = { version: incoming.version, clientId: incoming.clientId };
setState((current) => applyStatePatch(current, state, { preserveMissingKeys: false }));
setState((current) =>
applyStatePatch(current, state, { preserveMissingKeys: false, localStateKeys })
);

return true;
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ export const moduleGraphServiceDef = defineService({
storyChangeRevisions: {},
latestChangedStoryFiles: [],
} as ModuleGraphServiceState,
// The reverse index is only ever queried in the process that builds it β€” this service is
// registered on the server alone. It is also by far the largest thing in this state: on a large
// project it is millions of (file, story) pairs, which syncing would clone and serialize on every
// command for no reader. Revisions and status still sync normally.
localStateKeys: ['storiesByFile'],
queries: {
storiesForFiles: {
description:
Expand Down Expand Up @@ -259,10 +264,12 @@ export const moduleGraphServiceDef = defineService({
description:
'Replaces the reverse index after an incremental patch and bumps versions for affected story files. Called by the graph engine, not by external consumers.',
input: v.object({
storiesByFile: v.pipe(
storiesByFileSchema,
v.description(
'Complete relative reverse index keyed by story-index-style source file paths. Values map affected story-index-style story file paths to breadth-first-search depths.'
storiesByFile: v.optional(
v.pipe(
storiesByFileSchema,
v.description(
'Complete relative reverse index keyed by story-index-style source file paths. Values map affected story-index-style story file paths to breadth-first-search depths. Omitted when the patch left the reverse index unchanged, in which case the stored index is kept as-is.'
)
)
),
bumpedStoryFiles: v.pipe(
Expand All @@ -275,9 +282,11 @@ export const moduleGraphServiceDef = defineService({
output: v.void(),
handler: async (input, ctx) => {
ctx.self.setState((state) => {
state.storiesByFile = input.storiesByFile;
// An out-of-graph file change recomputes the (unchanged) reverse index but bumps no
// stories; it must not advance the revision, so watch-all and scoped subscribers stay put.
if (input.storiesByFile !== undefined) {
state.storiesByFile = input.storiesByFile;
}
// A change that bumps no stories must not advance the revision, so watch-all and scoped
// subscribers stay put.
if (input.bumpedStoryFiles.length === 0) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,10 @@ describe('IncrementalPatcher', () => {
world: { edges: new Map(), resolutions: new Map() },
});

await patcher.patch({ kind: 'change', path: '/repo/src/unknown.ts' });
const changed = await patcher.patch({ kind: 'change', path: '/repo/src/unknown.ts' });

// No dependents in reverseIndex, not a story file β€” nothing to re-walk.
expect(changed).toBe(false);
expect(reverseIndex.asMap().size).toBe(0);
expect(parseSpy).not.toHaveBeenCalled();
expect(graph.has('/repo/src/unknown.ts')).toBe(false);
Expand All @@ -128,8 +129,9 @@ describe('IncrementalPatcher', () => {
storyFiles: new Set([story]),
});

await patcher.patch({ kind: 'change', path: story });
const changed = await patcher.patch({ kind: 'change', path: story });

expect(changed).toBe(true);
expect(reverseIndex.lookup(dep).get(story)).toBe(1);
});

Expand Down Expand Up @@ -280,9 +282,10 @@ describe('IncrementalPatcher', () => {
storyFiles: new Set([story]),
});

await patcher.patch({ kind: 'change', path: dep });
const changed = await patcher.patch({ kind: 'change', path: dep });

// story must NOT be re-walked β€” graph and index are still accurate
expect(changed).toBe(false);
const storyCalls = parseSpy.mock.calls.filter((c) => c[0].filePath === story);
expect(storyCalls.length).toBe(0);
// reverse-index entries are preserved unchanged
Expand All @@ -298,12 +301,24 @@ describe('IncrementalPatcher', () => {
storyFiles: new Set(),
});

await patcher.patch({ kind: 'add', path: file });
const changed = await patcher.patch({ kind: 'add', path: file });

expect(changed).toBe(false);
expect(reverseIndex.asMap().size).toBe(0);
expect(parseSpy).not.toHaveBeenCalled();
});

it('reports an unlink of a file the graph never saw as leaving the index unchanged', async () => {
const { patcher, reverseIndex } = buildPatcher({
world: { edges: new Map(), resolutions: new Map() },
});

const changed = await patcher.patch({ kind: 'unlink', path: '/repo/notes.md' });

expect(changed).toBe(false);
expect(reverseIndex.asMap().size).toBe(0);
});

it('unlink prunes reverseIndex for a story path even when not in current storyFiles', async () => {
// Regression: if refreshStoryFiles already removed the story from the story set before
// the unlink event fires, `isStoryFile(path)` returns false. The old code guarded
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,15 @@ export class IncrementalPatcher {
});
}

async patch(event: FileChangeEvent): Promise<void> {
/**
* Applies one file-system event to the graph and reverse index, and reports whether the reverse
* index actually moved.
*
* Most events in a dev session leave it untouched: a comment-only edit keeps the same dependency
* set, and a write to a file the graph has never seen matches nothing at all. Callers use the
* return value to skip re-serializing and re-broadcasting an index that is still current.
*/
async patch(event: FileChangeEvent): Promise<boolean> {
const path = normalize(event.path);
// File contents may have changed (or the file is gone); drop any stale cached
// parse/resolve data before any read.
Expand All @@ -91,14 +99,16 @@ export class IncrementalPatcher {
if (event.kind === 'add') {
if (this.isStoryFile(path)) {
await this.walkStory(path);
return true;
}
// Non-story add: the documented limitation says we don't recover unresolved deps.
return;
return false;
}

if (event.kind === 'unlink') {
const dependentsSet = new Set(this.reverseIndex.lookup(path).keys());
this.graph.delete(path);
// Every walked node gets a graph entry, so this doubles as "was this file known at all".
const wasKnown = this.graph.delete(path);
this.reverseIndex.removeStory(path); // always call β€” no-op for non-stories
// Re-walk every dependent story so transitive deps reachable only through `path`
// are pruned.
Expand All @@ -111,7 +121,7 @@ export class IncrementalPatcher {
storiesToWalk.push(story);
}
await Promise.all(storiesToWalk.map((story) => this.walkStory(story)));
return;
return wasKnown || storiesToWalk.length > 0;
}

// 'change' on an existing file: re-walk every story that depends on this file
Expand All @@ -129,7 +139,7 @@ export class IncrementalPatcher {
if (oldDeps !== undefined) {
const newDeps = await this.cache.resolveOnce(path);
if (setsEqual(oldDeps, newDeps)) {
return;
return false;
}
}

Expand All @@ -142,6 +152,8 @@ export class IncrementalPatcher {
storiesToWalk.push(story);
}
await Promise.all(storiesToWalk.map((story) => this.walkStory(story)));
// A change to a file outside the graph re-walks nothing and leaves the index as it was.
return storiesToWalk.length > 0;
}

private walkStory(storyRoot: string): Promise<void> {
Expand Down
Loading
Loading