refactor(group-channel): Phase 5.1 unread dispatch + read hook infra (stacked on #1428)#1430
Draft
danney-chun wants to merge 4 commits into
Draft
Conversation
…patch (Phase 5.1.a)
Wires the UnreadReducer into GroupChannelProvider as the dispatch-side
half of the dual-write migration (spec §AC-1, §AC-8).
- `internal/unread/integration.ts` mirrors `runtime/integration.ts`:
`createUnreadStore` + `dispatchToUnreadStore` with the W1 parallel-only
guard (reducer/patch throws are caught + surfaced via onError; hook
throws are swallowed). Adds the dev/test
`__GROUP_CHANNEL_UNREAD_DISPATCH_HOOK__` instrumentation hook.
- `GroupChannelProvider` gains `unreadStoreRef` and a thunk-guarded
`unreadDispatch` helper. 6 dispatch sites wired:
* CHANNEL_CHANGED — useAsyncEffect (channel ready), onChannelUpdated
(url change), onChannelDeleted, onCurrentUserBanned
* MESSAGES_RECEIVED — onMessagesReceived (with conservative
fromCurrentUser = all-messages-from-current-user)
* USER_LEFT_BOTTOM / USER_REACHED_BOTTOM — new effect watching
state.isScrollBottomReached
* MARK_AS_UNREAD_SET — markAsUnread callback after SDK call
* READ_CONFIRMED — onChannelUpdated EVENT_CHANNEL_READ branch
- MESSAGES_DELETED intentionally NOT wired — @sendbird/uikit-tools@0.1.0
does not expose onMessagesDeleted (Plan §1). Reducer event stays
callable for tests; Provider dispatch deferred.
Legacy writes (setNewMessageIds / setFirstUnreadMessageId) are
UNCHANGED — dual-write per spec §AC-4. Consumer read migration arrives
in 5.1.c.
8 new tests under `internal/unread/__tests__/integration.spec.ts`:
initial state, dispatch + state update, hook fire, hook error swallow,
subscriber notify, plus 3 W1 parallel-only cases (reducer/patch
exception, onError throw, no-onError).
… 5.1.b)
Adds the read-side surface for consumer migration (spec §AC-2).
- `context/GroupChannelUnreadContext.tsx` — React context holding the
unread `Store<UnreadState> | null`. Module-private; NOT re-exported
via `src/index.ts` (BC-2 verifies).
- `context/hooks/useUnreadSelector.ts` — thin wrapper over
`useStoreSelector` with the new context. JSDoc warns consumers to use
module-level or `useCallback`-stabilized selectors (W3 note inherited
from Phase 1).
- `internal/unread/selectors.ts` — add `selectIsMessageUnread(s, message)`
for the MessageView cut-read in 5.1.c. Returns a primitive boolean so
identity is stable per `Object.is`.
- `GroupChannelProvider` — wrap `GroupChannelManager`'s children with
`GroupChannelUnreadContext.Provider value={unreadStoreRef.current}`.
5 new tests under `context/hooks/__tests__/useUnreadSelector.spec.tsx`:
initial selector output, narrow-slice re-render boundary, throws
outside provider, selectIsMessageUnread membership tracking, custom
equality for derived shapes.
BC-1~6 PASS. No legacy code path is modified; consumer read migration
arrives in 5.1.c.
…eId prop type (Phase 5.1.c) REDUCED scope after a mid-cycle semantic audit (see .agentic/p0-phase-5-1/spec.md §AC-3 REDUCED). The three originally planned consumer cut-read sites — `MessageView.newMessageIds`, `MessageList.unreadSinceDate`, and `MessageList.finalFirstUnreadMessageId` — each turned out to encode semantics distinct from the Phase 4 reducer: - `newMessageIds` is gated by `isAutoscrollMessageOverflowToTop` and drives an autoscroll trigger; the reducer's set is broader. - `unreadSinceDate` is a wall-clock `new Date()` captured when the user leaves the bottom; the reducer's `firstUnreadCreatedAt` is the message's own createdAt epoch. - `finalFirstUnreadMessageId` is derived from `currentChannel.myLastRead + 1` (server-confirmed read state) + a manual `markAsUnread` ref; the reducer tracks UIKit-session-local "since-leave-bottom" anchors. Phase 4's parity test compared the reducer against a simplified hand-rolled legacy model, not the real consumer pipeline. Cutting any of these reads would be a behavior change, not a refactor. This commit narrows `firstUnreadMessageId?: number | string | undefined` to `number | null | undefined` — the lone R-5 type cleanup that ships with zero behavior impact. All internal call sites already conform. Phase 5.2 hand-off: reducer semantics need re-auditing against server-truth vs session-local before consumer reads can be migrated. The 5.1.a (dispatch) + 5.1.b (read hook) infrastructure remains in place for that future cycle.
Mid-cycle code review (.agentic/p0-phase-5-1/review/code-review.md) raised no blockers but flagged five improvements; all five applied here. - I-1 (WARN): `unreadChannelUrlRef` + `dispatchChannelChanged` helper. Collapses repeat CHANNEL_CHANGED transitions through a single guard. Reducer is already idempotent on this transition, but the guard prevents dev-hook double-fire when a stale onChannelUpdated closure races the mount-time useAsyncEffect. - I-2 (WARN): Filter current-user messages BEFORE dispatch instead of passing `fromCurrentUser: every(...)`. The prior collapse rule mis-classified mixed bursts (one self + one peer) as `false`, growing the unread set with my own messageId. After filtering, the dispatched burst is by definition non-self → `fromCurrentUser: false` is correct and the set stays clean. No consumer reads from the set yet, but preempts a 5.2 footgun (the moment cut-read switches over, this would surface as self-flagged-unread). - I-3 (WARN): Drop `(m: any)` casts on the dispatch boundary. Use the structurally-narrowed `sender?.userId` access; coreTs SendbirdMessage[] is the inferred type. - I-6 (INFO): Add a cross-reference comment on the asymmetric error return (`dispatchToUnreadStore` returns prior state on failure; `dispatchToRuntime` returns empty effects array) so reviewers reading both side-by-side understand each is the unit appropriate to its contract. - I-7 (INFO): Add `useUnreadSelector` JSDoc example demonstrating the `useCallback`-stabilized closure idiom — the W3-pattern caveat is real and well-tested but easy to trip over with prop-closing selectors. BC-1~6 PASS, 196/198 suites (1125 tests, 0 fail) unchanged.
![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: 1fa01fa8e4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| selector: (state: UnreadState) => TSelected, | ||
| equalityFn?: EqualityFn<TSelected>, | ||
| ): TSelected { | ||
| return useStoreSelector(GroupChannelUnreadContext, selector, equalityFn); |
There was a problem hiding this comment.
This wrapper inherits useStoreSelector's cache, which returns the previous selected value whenever the raw store snapshot is the same, without considering that selector itself may have changed. For consumers that close over a changing prop, such as a message row calling useUnreadSelector(useCallback((s) => selectIsMessageUnread(s, { messageId }), [messageId])), changing messageId will keep returning the old message's unread status until some unread-store dispatch happens; the JSDoc example above suggests this pattern, so future MessageView usage can render stale unread/separator state on prop-only rerenders or recycled rows.
Useful? React with 👍 / 👎.
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
1 participant
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.
Summary
Phase 5.1 of the P0 runtime-coupling refactor (stacked on #1428). Lands the dispatch-side and read-hook infrastructure for the UnreadReducer that Phase 4 (#1428) ships as parity-only. Consumer reads remain on legacy state — see "Mid-cycle scope reduction" below.
.agentic/p0-phase-5-1/{spec.md, plan.md, review/code-review.md}Phase-by-phase (4 commits)
8facc8bbinternal/unread/integration.ts+ ProviderunreadDispatchthunk (6 events wired) + 8 W1 invariant tests9ceeae3aGroupChannelUnreadContext+useUnreadSelector+selectIsMessageUnread+ 5 unit tests4c414cbbgetMessagePartsInfo.firstUnreadMessageIdonly1fa01fa8Mid-cycle scope reduction (important for reviewers)
Original 5.1.c planned to cut three consumer reads (
MessageView.newMessageIds,MessageList.unreadSinceDate,MessageList.finalFirstUnreadMessageId) over to the new reducer-backed selectors. Mid-cycle audit revealed all three legacy fields encode semantics distinct from the Phase 4 reducer:newMessageIdsisAutoscrollMessageOverflowToTop; drives autoscrollunreadSinceDatenew Date()wall clock at leave-bottomfirstUnreadCreatedAt= message epochfinalFirstUnreadMessageIdcurrentChannel.myLastRead + 1(server truth) + manual refPhase 4's parity test compares the reducer against a simplified hand-rolled legacy model — it does NOT verify behavioral equivalence with real consumer code. Cutting any of the reads would be a behavior change, not a refactor.
This PR lands the dispatch + read-hook infrastructure with zero consumer impact, leaving the semantic decision to Phase 5.2.
What ships in this PR
GroupChannelProvider):unreadStoreRef+ thunk-guardedunreadDispatch(makeEvent, isAtBottom?)+dispatchChannelChanged(url)(idempotent viaunreadChannelUrlRef). 6 of 7 reducer events wired (MESSAGES_DELETED skipped —@sendbird/uikit-tools@0.1.0lacksonMessagesDeleted). All dispatches are additive to legacy state writes (dual-write).GroupChannelUnreadContext+useUnreadSelector<T>(selector, equalityFn?). NOT re-exported fromsrc/index.ts.selectIsMessageUnread(s, message)— primitivebooleanreturn for identity stability.getMessagePartsInfo.firstUnreadMessageId?: number | string | undefined→number | null | undefined. All call sites already conform.Parallel-only invariant (W1 inheritance)
dispatchToUnreadStoremirrorsdispatchToRuntime: try/catch around reducer + applyStorePatch,onErrorcallback, hook error swallow.unreadDispatchthunk additionally catches mapper throws. Verified by 3 cases ininternal/unread/__tests__/integration.spec.ts(forced setState failure → swallow + onError + state preserved + no-onError safe).BC invariants (all PASS)
package.jsonpublic fields unchangeddist/types/index.d.tsexport set unchangedinternal/internal/scrollPubSubtopic + payload set unchangedTest plan
./scripts/bc-check.sh— BC-1 ~ BC-6 all PASSyarn jest— 196/198 PASS (2 pre-existing skipped, 0 fail), 1125 tests + 10 skipped — +13 new specs over refactor(group-channel): P0 runtime stability scaffolding (Phase 0-4) #1428 baselineyarn buildclean (pre-existing warnings only)Phase 5.2 hand-off (must read before consumer migration)
The infrastructure landed here cannot drive consumer reads until the reducer semantics are reconciled with real consumer expectations. Phase 5.2 should:
fromCurrentUsersemantic explicitly during cut-read (this PR already fixes the most painful version via filter-before-dispatch).Out of scope (intentional)
MESSAGES_DELETEDwiring (pending uikit-tools version bump)newMessageIds,firstUnreadMessageId) — still authoritativeStacking note
Base =
refactor/p0-phase-4-unread-reducer(PR #1428). Once #1428 merges to main, this PR's base will be updated and rebased onto current main.🤖 Generated with Claude Code