Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new @jsonjoy.com/json-crdt-repo package (browser local-first client + storage + sync for JSON CRDT docs) and includes several related correctness/test-harness improvements across the monorepo.
Changes:
- Add the
json-crdt-repopackage with local LevelDB-backed repo, remote history adapter, editing sessions, pubsub, utilities, demos, and extensive Jest/e2e tests. - Improve CRDT/NFS robustness (handle missing array children during sync replays; expand NFS errno→NFSv4 status mappings + tests; reduce noisy logging).
- Harden server/test infrastructure (clone return values from in-memory block store; refine block deletion pubsub emission; add more deterministic cleanup in multiple test suites; refactor reconnect delay handling).
Reviewed changes
Copilot reviewed 83 out of 85 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Add workspace entry for new @jsonjoy.com/json-crdt-repo package and adjust workspace resolution entries. |
| packages/json-pack/src/nfs/v4/server/operations/node/util.ts | Expand Node FS errno normalization to additional NFSv4 status codes. |
| packages/json-pack/src/nfs/v4/server/operations/node/tests/REMOVE.spec.ts | Add test coverage for non-empty directory removal returning NOTEMPTY. |
| packages/json-pack/src/nfs/v4/server/operations/node/tests/READLINK.spec.ts | Tighten READLINK non-symlink assertion to expect INVAL. |
| packages/json-pack/src/nfs/v4/server/operations/node/Nfsv4OperationsNode.ts | Gate ILLEGAL operation logging behind debug flag. |
| packages/json-joy/src/json-crdt/nodes/arr/ArrNode.ts | Avoid crashing when array child IDs are missing from index during iteration. |
| packages/json-joy/src/json-crdt/model/tests/Model.sync.spec.ts | Add regression test for sparse concurrent history replay without missing-child crash. |
| packages/json-joy/src/json-crdt/tests/fuzzer2/sync-ops.spec.ts | Factor fuzzer loop into helper + add fixed failing seed regression. |
| packages/json-crdt-server/src/services/blocks/store/MemoryStore.ts | Return deep-cloned block/snapshot/batch/patch objects to prevent external mutation. |
| packages/json-crdt-server/src/services/blocks/BlocksServices.ts | Refactor block event emission helpers and only emit deletion when actually deleted. |
| packages/json-crdt-repo/tsconfig.json | New package TS config + project references. |
| packages/json-crdt-repo/tsconfig.build.json | New package build TS config + build references. |
| packages/json-crdt-repo/src/util/rx/shareByKey.ts | Add keyed observable sharing helper for per-id subscriptions. |
| packages/json-crdt-repo/src/util/tests/Mutex.spec.ts | Add tests for new keyed async mutex behavior. |
| packages/json-crdt-repo/src/util/Mutex.ts | Add keyed async mutex implementation. |
| packages/json-crdt-repo/src/undo-redo/UndoRedoStack.ts | Add undo/redo stack abstraction with locking + timeout handling. |
| packages/json-crdt-repo/src/types.ts | Add top-level repo/session history typing surface. |
| packages/json-crdt-repo/src/session/types.ts | Define session history service and sync request/response types. |
| packages/json-crdt-repo/src/session/index.ts | Export session APIs. |
| packages/json-crdt-repo/src/session/tests/setup.ts | Add session test setup around local level testbed. |
| packages/json-crdt-repo/src/session/tests/EditSessionFactory.spec.ts | Add comprehensive tests for session factory make/load and remote interactions. |
| packages/json-crdt-repo/src/session/tests/EditSession.two-sessions.spec.ts | Add tests for two-session convergence across remote/local/same-tab scenarios. |
| packages/json-crdt-repo/src/session/tests/EditSession.sync.spec.ts | Add tests for sync semantics (log draining, convergence, timing predictability). |
| packages/json-crdt-repo/src/session/tests/EditSession.onFlush.spec.ts | Add tests for onFlush-driven synchronization across sessions. |
| packages/json-crdt-repo/src/session/tests/EditSession.events.spec.ts | Add tests for rebase events and .applyLocalPatch() flow. |
| packages/json-crdt-repo/src/session/tests/EditSession.del.spec.ts | Add tests for deletion propagation across sessions/tabs/remote. |
| packages/json-crdt-repo/src/session/tests/EditSession.JsonPatch.spec.ts | Add tests for JSON Patch interface behavior across sessions. |
| packages/json-crdt-repo/src/session/EditSessionFactory.ts | Implement session factory (make/load) including optional remote pull logic. |
| packages/json-crdt-repo/src/session/EditSession.ts | Implement edit session lifecycle, sync, and event integration with local repo. |
| packages/json-crdt-repo/src/remote/types.ts | Define RemoteHistory abstraction + server-specialized aliases. |
| packages/json-crdt-repo/src/remote/index.ts | Export remote APIs. |
| packages/json-crdt-repo/src/remote/tests/setup.ts | Add remote test setup using server caller. |
| packages/json-crdt-repo/src/remote/tests/DemoServerRemoteHistory.spec.ts | Add tests validating remote history adapter behavior over server routes. |
| packages/json-crdt-repo/src/remote/DemoServerRemoteHistory.ts | Implement remote history adapter backed by JSON CRDT server RPC routes. |
| packages/json-crdt-repo/src/pubsub/types.ts | Define pubsub interface. |
| packages/json-crdt-repo/src/pubsub/index.ts | Implement in-memory + BroadcastChannel pubsub for cross-tab messaging. |
| packages/json-crdt-repo/src/pubsub/tests/index.spec.ts | Add tests for cross-instance pubsub behavior. |
| packages/json-crdt-repo/src/local/types.ts | Define local repo CRUD/sync/pull/change event types. |
| packages/json-crdt-repo/src/local/level/types.ts | Define LevelDB local repo storage types + pubsub message shapes. |
| packages/json-crdt-repo/src/local/level/tests/setup.ts | Add LevelLocalRepo test harness using MemoryLevel + server remote. |
| packages/json-crdt-repo/src/local/level/tests/LevelLocalRepo.remote-sync.spec.ts | Add tests for resuming sync after connectivity changes / new tab online. |
| packages/json-crdt-repo/src/local/level/tests/LevelLocalRepo.pull.spec.ts | Add tests for pull idempotency, conflict handling, reset strategy, and events. |
| packages/json-crdt-repo/src/local/level/tests/LevelLocalRepo.events.spec.ts | Add tests for pubsub-driven cross-tab events (including rebase). |
| packages/json-crdt-repo/src/local/level/README.md | Add minimal documentation for LevelDB-backed local repo. |
| packages/json-crdt-repo/src/local/index.ts | Export local repo APIs. |
| packages/json-crdt-repo/src/index.ts | Package entrypoint exports. |
| packages/json-crdt-repo/src/tests/types.ts | Add shared test setup typings for e2e-like suites. |
| packages/json-crdt-repo/src/tests/testbed.ts | Add browser/tab/repo testbed abstraction for multi-tab scenarios. |
| packages/json-crdt-repo/src/tests/setup.ts | Add server-backed setup helpers (memory/level stores) for RPC tests. |
| packages/json-crdt-repo/src/tests/server/util.ts | Add RPC util route tests. |
| packages/json-crdt-repo/src/tests/server/pubsub.ts | Add RPC pubsub route tests. |
| packages/json-crdt-repo/src/tests/server/presence.ts | Add RPC presence route tests. |
| packages/json-crdt-repo/src/tests/server/block.ts | Add extensive RPC block route tests. |
| packages/json-crdt-repo/src/tests/e2e/run.ts | Add runner to start server + execute Jest e2e tests. |
| packages/json-crdt-repo/src/tests/e2e/json-crdt-server/demo-server.spec.ts | Add optional demo-server e2e suite. |
| packages/json-crdt-repo/src/tests/e2e/json-crdt-server/clients.spec.ts | Add local server e2e suite for persistent/fetch clients across codecs. |
| packages/json-crdt-repo/src/tests/e2e/demo-client.ts | Add demo-server client setups for e2e. |
| packages/json-crdt-repo/src/tests/e2e/codecs.ts | Add codec matrix builders used by e2e. |
| packages/json-crdt-repo/src/tests/e2e/clients.ts | Add local server client setups for e2e. |
| packages/json-crdt-repo/src/demos/ui-text/webpack.config.js | Add demo build config for collaborative text UI demo. |
| packages/json-crdt-repo/src/demos/ui-text/main.tsx | Add collaborative textarea demo using JsonCrdtRepo. |
| packages/json-crdt-repo/src/demos/ui-json/webpack.config.js | Add demo build config for collaborative JSON UI demo. |
| packages/json-crdt-repo/src/demos/ui-json/main.tsx | Add interactive JSON demo using JsonCrdtRepo. |
| packages/json-crdt-repo/src/JsonCrdtRepo.ts | Implement browser repo wrapper (BrowserLevel + BroadcastChannel + remote history + sessions). |
| packages/json-crdt-repo/package.json | Add package manifest including deps, peerDeps, scripts, Jest config. |
| packages/json-crdt-repo/SECURITY.md | Add package security policy doc. |
| packages/json-crdt-repo/README.md | Add package-level README and basic usage snippet. |
| packages/json-crdt-repo/LICENSE | Add Apache-2.0 license text to package. |
| packages/collaborative-slate/src/tests/SlateFacade.undo.spec.ts | Ensure facade is disposed during Symbol.dispose cleanup. |
| packages/collaborative-slate/src/tests/SlateFacade.set.spec.ts | Ensure facade is disposed during Symbol.dispose cleanup. |
| packages/collaborative-slate/src/tests/SlateFacade.selection.spec.ts | Ensure facade is disposed during Symbol.dispose cleanup. |
| packages/collaborative-react/src/tests/setup.ts | Add RTL cleanup afterEach in collaborative-react tests. |
| packages/collaborative-react/package.json | Configure Jest setupFilesAfterEnv for collaborative-react. |
| packages/collaborative-prosemirror/src/tests/setupAfterEnv.ts | Add afterEach cleanup hook for active testbeds + DOM reset. |
| packages/collaborative-prosemirror/src/tests/setup.ts | Track and cleanup active testbeds; make DOM cleanup more robust. |
| packages/collaborative-prosemirror/src/tests/ProseMirrorFacade.selection.spec.ts | Use using to ensure deterministic testbed disposal. |
| packages/collaborative-prosemirror/package.json | Configure Jest setupFilesAfterEnv for collaborative-prosemirror. |
| packages/channel/src/tests/WebSocketChannel.spec.ts | Track and close active channels afterEach to prevent leaks/flakes. |
| packages/channel/src/tests/PersistentPhysicalChannel.spec.ts | Track and stop active persistents afterEach to prevent leaks/flakes. |
| packages/channel/src/PersistentPhysicalChannel.ts | Replace reconnect delays with cancellable wait() observable + timer unref. |
| package.json | Update root Jest invocation to run in-band (and add localstorage file path). |
| README.md | Reformat integrations list + add JSON CRDT Explorer link. |
| .gitignore | Ignore /.tmp directory used by tests. |
| export const shareByKey = <TValue>(sub: (key: string) => Observable<TValue>): ((key: string) => Observable<TValue>) => { | ||
| const map: Record<string, Observable<TValue>> = {}; | ||
| return (key: string) => { |
There was a problem hiding this comment.
Using a plain object (Record<string, ...> = {}) as a cache keyed by arbitrary strings can be vulnerable to prototype-pollution edge cases (e.g. key __proto__) and can also collide with inherited properties. Prefer a Map<string, Observable<TValue>>, or initialize with Object.create(null) and access via Object.prototype.hasOwnProperty.call(...).
| const timeout2 = <T>(ms: number, promise: Promise<T>): Promise<T | undefined> => | ||
| timeout(ms, promise).catch(() => undefined); |
There was a problem hiding this comment.
timeout2() currently converts any rejection from timeout(ms, promise) into undefined, so an undo()/redo() implementation that rejects will be treated the same as a timeout (-2). If you need to distinguish timeout vs real errors, only swallow the specific timeout error and let other rejections propagate (or map them to -3 consistently).
| /** Thew new block schema, if any. */ | ||
| schema?: NodeBuilder; |
There was a problem hiding this comment.
Typo in doc comment: "Thew new block schema" should be "The new block schema".
| /** Thew new block schema, if any. */ | ||
| schema?: NodeBuilder; | ||
|
|
There was a problem hiding this comment.
EditSessionLoadOpts declares a schema?: NodeBuilder field, but load() never reads or applies it. This is confusing for callers and can lead to silent misconfiguration. Either remove the option, or if it's intended, apply it to the loaded session's model (similar to make()).
| * a centra server; (2) un-fetched content addressable storage files; or (3) a | ||
| * peer-to-peer network. |
There was a problem hiding this comment.
Typo in doc comment: "centra server" should be "central server".
| const cursorBehind = this.cursor !== res.cursor; | ||
| if (cursorBehind) { | ||
| const timer = setTimeout(async () => { | ||
| if (this._stopped) return; |
There was a problem hiding this comment.
this.cursor is compared to res.cursor but never updated on successful repo.sync(). This can keep the session thinking it's behind and trigger unnecessary getIf polling / missed cursor progression. Assign this.cursor = res.cursor (ideally immediately after repo.sync() succeeds) before using it for comparisons.
| this.sync() | ||
| .then((error) => { | ||
| this.onsyncerror?.(error); | ||
| }) | ||
| .catch((error) => { | ||
| this.onsyncerror?.(error); | ||
| }); |
There was a problem hiding this comment.
In syncLog(), the .then((error) => ...) branch calls onsyncerror with the successful return value of sync() (e.g. null or {remote}), not an error. This makes onsyncerror fire on success and can mask real failures. Only invoke onsyncerror from the .catch(...) path (or rename + add a separate success callback).
| public readSid(): number { | ||
| const ls = window.localStorage; | ||
| const key = this.opts.name + '-sid'; |
There was a problem hiding this comment.
readSid() directly uses window.localStorage, which will throw in non-browser contexts (SSR, Node-based tooling/tests) even though this package is built under Node. Consider guarding with typeof window !== 'undefined' / globalThis.localStorage checks and/or allowing sid to be injected via constructor options for non-browser usage.
| * Weather to asynchronously pull for any existing local block state, if a | ||
| * block with the same ID already exists. Defaults to `true`. | ||
| */ |
There was a problem hiding this comment.
Typo in doc comment: "Weather to asynchronously pull" should be "Whether to asynchronously pull".
Implementation of browser client and storage for JSON CRDT documents.