[WIP][POC] Flip the subscription layer onto OnyxStore (atomic swap) - #834
[WIP][POC] Flip the subscription layer onto OnyxStore (atomic swap)#834fabioh8010 wants to merge 32 commits into
Conversation
Introduce lib/OnyxStore.ts: a single listener registry (keyListeners Map<key, Set<listener>>) with subscribe / notifyKey / notifyCollection / getState / hasListenersForKey / clearAll. Built on the existing structural-sharing cache (cache.getCollectionData frozen snapshots). This module is inert — nothing imports it yet. The subscription and notification paths (Onyx.connect, useOnyx, OnyxUtils.notify*) are wired onto it in a later change. Adding it alone has zero behavioral impact. Includes tests/unit/OnyxStoreTest.ts (20 tests) covering exact-key and collection-snapshot routing, ref-equality member skips, hasListenersForKey, clearAll, and listener error isolation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wire useOnyx, Onyx.connect/disconnect, and the OnyxUtils notify path onto the OnyxStore registry. useOnyx is rebuilt on useSyncExternalStore reading the eager, structurally-shared cache via onyxStore.getState — it renders once (no loading->loaded second render) and bails out via === on the stable cached reference. Delete OnyxConnectionManager and OnyxSnapshotCache (and their unit + perf tests). Also remove the now-defunct reuseConnection option (connection pooling is gone). The `loading` status value is retained for now (always 'loaded' in practice) and removed in a follow-up; OnyxStore stays trimmed (no subscribeState) since useOnyxState is a separate proposal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebuild the store-based useOnyx on React's useSyncExternalStoreWithSelector (use-sync-external-store/with-selector) with deepEqual as the equality fn. Its committed-value dedup collapses content-equal selections to a stable reference and survives the selector function's identity churning every render — so consumers can pass inline selectors that close over freshly allocated arrays/objects without stabilizing the inputs (no more useStableArrayReference). No-selector subscriptions keep the Object.is fast-path since the raw cache value is already reference-stable. Adds use-sync-external-store + @types/use-sync-external-store. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d023b4d41
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const isLoading = connectedKeyRef.current !== key && OnyxUtils.hasPendingMergeForKey(key); | ||
| const loadingStatus: FetchStatus = isLoading ? 'loading' : 'loaded'; |
There was a problem hiding this comment.
When this hook mounts or switches keys while a no-op merge is pending—for example, merging data already present in the cache—isLoading makes it return [undefined, {status: 'loading'}], but merge completion does not necessarily notify the store because broadcastUpdate() returns when hasChanged is false. The effect only mutates connectedKeyRef without scheduling a render, so the hook can remain permanently blank and loading. The merge-settlement path needs to trigger a render even when the cached snapshot does not change.
Useful? React with 👍 / 👎.
…uring a notify only affect later ones
…lify the unsubscribes reset
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80d9797d14
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (pendingWrites.size === 0) { | ||
| return Promise.resolve(); | ||
| } | ||
| return Promise.all([...pendingWrites].map((promise) => promise.catch(() => undefined))).then(whenWritesSettled); |
There was a problem hiding this comment.
When any Onyx write is slow or its storage promise never settles, this global drain prevents every subsequently created Onyx.connect() subscription—even one for an unrelated, already-cached key—from receiving its initial callback. Public write methods register their entire cache-and-persistence promise with trackPendingWrite(), and the recursive recheck also lets a continuous stream of unrelated writes postpone initialization indefinitely; initial delivery should wait only for cache application relevant to the connection rather than all global persistence work.
Useful? React with 👍 / 👎.
1 participant
Details
Related Issues
GH_LINK
Linked E/App PR
Automated Tests
Manual Tests
Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari