refactor(group-channel): Phase 5.2.b Hybrid hydration + consumer cut-read (stacked on #1430)#1436
Closed
danney-chun wants to merge 5 commits into
Closed
Conversation
…ucer (Phase 5.2.b.a) Bootstraps the session-local reducer from server-truth at channel mount. The reducer otherwise stays session-local per design doc §7 — this event is the one-shot seed bridge to enable consumer cut-read at mount when the SDK already reports unread state. Transitions: - mode='marked-unread' → no clobber (user pin wins); only lastReadAt refreshes to the server-supplied value. - unreadCount === 0 → clean state; lastReadAt = event.lastReadAt (= server-side currentChannel.myLastRead). - unreadCount > 0 → tracking state seeded with the supplied anchor + ids set. badgeVisible/separatorVisible derived as in MESSAGES_RECEIVED. The dispatch site (Provider) and the consumer cut-read (MessageList) arrive in 5.2.b.b and 5.2.b.c. 4 new test cases under reducer.transitions.spec.ts cover: - clean + zero unread → clean (no anchor) - clean + N unread → tracking (with anchor + set) - marked-unread + hydrate → user pin preserved - post-hydrate USER_REACHED_BOTTOM → standard clear path ALL_UNREAD_EVENT_TYPES updated from 7 to 8.
…e 5.2.b.b) Wires the SDK-state seed for the UnreadReducer. The new useEffect fires after messageDataSource.initialized flips true, derives the seed payload from currentChannel (myLastRead, unreadMessageCount) + the loaded messages list, and dispatches CHANNEL_HYDRATED through the existing unreadDispatch thunk-guard (W1 isolation intact). Effect re-runs on channel switch via state.currentChannel?.url dep. CHANNEL_CHANGED (5.1.a) still fires earlier on switch, so the reducer goes clean → hydrate → tracking, which is the correct ordering. Pagination caveat (Plan §R-1) is documented inline: when myLastRead+1 is older than the earliest loaded message, the dispatch sets firstUnreadMessageId=null. Server count still renders via the badge selector; separator anchor stays empty until more history loads. Consumer cut-read of firstUnreadMessage at MessageList lands in 5.2.b.c. Full jest suite: 196/198 PASS (1129 tests, 0 fail). BC-1~6 preserved.
…se 5.2.b.c) The MessageList anchor for `<NewMessageSeparator>` now reads from the UnreadReducer (seeded at mount via 5.2.b.b's CHANNEL_HYDRATED) instead of the local `myLastRead + 1` derivation. - `useUnreadSelector(selectFirstUnreadMessageId)` provides the messageId. - A messages.find lookup hydrates the full message object needed by `separatorMessageRef`. - Legacy gates (enableMarkAsUnread, isInitializedRef, readState, presence of myLastRead) are preserved so non-MAU mode still renders nothing. - The legacy myLastRead+1 search remains as a defensive fallback when the reducer has no seed yet (Plan §R-2 hydrate-timing gap), so behavior is identical during the first render after mount before the Provider's useEffect fires. - The readState='unread' side effect (was inside the useMemo) is promoted to its own useEffect for purity. New integration spec at `MessageList/__tests__/unreadSeparator.integration.spec.tsx` (6 cases) covers the dispatch → reducer → selector path. The original AC-5 plan to mount the full GroupChannelProvider was reduced after an OOM crash; the dispatch logic itself is type-checked + built and a heavier app-shell test can be added in a future cycle if needed. BC-1~6 PASS. Full jest: 197/199 suites (1135 tests, 0 fail) — +6 new.
…se 5.2.b.d) The `setFirstUnreadMessageId` action was declared on the `GroupChannelActions` interface and exposed via `useGroupChannel()`, but never called anywhere in the source tree (.agentic/p0-phase-5-2a/audit.md §1.3 observation 1). The corresponding `state.firstUnreadMessageId` field was never part of `initialState()` either — the setter wrote to a slot that was never read. Pure dead code. After this cleanup: - The UnreadReducer's `selectFirstUnreadMessageId` (5.1.b) is the authoritative source for the first-unread anchor, seeded from server truth via CHANNEL_HYDRATED (5.2.b.b) and consumed via cut-read at MessageList (5.2.b.c). Changes: - Remove `setFirstUnreadMessageId` from `GroupChannelActions` interface - Remove the unused `useCallback` definition + replace with a brief pointer to where the read API now lives - Remove from the `actions` object literal + its useMemo deps - Update Phase 0 context-shape-baseline.json to drop the removed action (this is an intentional shape change — internal API only, not in src/index.ts; verified by grep) - Refresh the stale Phase 5.1.a comment block on `unreadStoreRef` to reflect the post-5.2 state Verification: - `grep setFirstUnreadMessageId` returns zero hits outside historical comments - BC-1~6 PASS (baseline JSON intentionally updated; BC-3 only checks well-formedness) - Full jest 197/199 PASS (1135 tests, 0 fail) - yarn build clean
I-1 (WARN): add `hydratedChannelUrlRef` guard around the CHANNEL_HYDRATED dispatch. `useGroupChannelMessages` calls `updateInitialized(false)` → `(true)` inside `resetWithStartingPoint` (verified in published source), so without the guard a jump-to-message would re-fire the hydrate effect with the new message window and clobber any tracking state accumulated since the original mount. The ref is reset on channel switch by `dispatchChannelChanged`, mirroring the `unreadChannelUrlRef` idempotency pattern from PR #1430 review I-1. I-2 (WARN): drop the three structural `(m as { messageId: number })` and `(m as { createdAt: number })` casts in the hydrate dispatch payload. `CoreMessageType` already has these fields on its base; only the `sender?.userId` access in the filter callback needs narrowing. BC-1~6 PASS, full jest 197/199 (1135 tests, 0 fail).
![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: 2adec40473
ℹ️ 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".
Comment on lines
+137
to
140
| useEffect(() => { | ||
| if (readState === 'unread') { | ||
| separatorMessageRef.current = undefined; | ||
| isUnreadMessageExistInChannel.current = true; |
There was a problem hiding this comment.
When readState changes to 'unread' while separatorMessageRef.current still points at a previous separator, moving this ref reset into useEffect makes it happen after the render that computes finalFirstUnreadMessageId from separatorMessageRef.current || firstUnreadMessage?.messageId. Because mutating the ref does not trigger another render, that update can leave the separator anchored to the old message until some unrelated state change occurs; the previous in-render reset avoided this stale ref during the same render.
Useful? React with 👍 / 👎.
5 tasks
Contributor
Author
|
Consolidating into single local branch |
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.2.b of the P0 runtime-coupling refactor (stacked on #1430). Implements Model C — Hybrid from the 5.2.a investigate cycle: the session-local UnreadReducer is seeded at channel mount with a one-shot
CHANNEL_HYDRATEDevent, enabling the MessageList consumer to migrate fromcurrentChannel.myLastRead + 1-based derivation to a reducer-backed selector. Also lands the dead-code cleanup ofsetFirstUnreadMessageIdthat the 5.2.a audit uncovered..agentic/p0-phase-5-2a/{audit.md, decision.md}— semantic audit + Model C rationale.agentic/p0-phase-5-2b/{spec.md, plan.md, review/code-review.md}Phase-by-phase (5 commits)
77e22e0bCHANNEL_HYDRATEDevent added toUnreadEventunion + reducer handler (clean / tracking branches + marked-unread no-clobber). 4 new transition tests.2bc84e19CHANNEL_HYDRATEDin a new effect onmessageDataSource.initialized + state.currentChannel?.url. Filters peer messages, derives anchor + ids.c746e788useUnreadSelector(selectFirstUnreadMessageId)then amessages.findlookup. LegacymyLastRead+1retained as fallback for the hydrate-timing gap. 6 new probe-based integration tests.61f86ad9setFirstUnreadMessageIdaction (never called per audit §1.3 obs 1) + update Phase 0 context-shape baseline.2adec404Why Model C
The 5.2.a investigate cycle established that:
refactor-design.ko.md §7) is intentionally session-local — tracks "messages received since the user left bottom" + manual mark-as-unread overlay.MessageList.firstUnreadMessagederivation is server-truth —currentChannel.myLastRead + 1lookup.unreadMessageCount > 0). Reducer session-local would show zero unread; legacy shows the server-tracked separator.Model C bridges the gap with a one-shot
CHANNEL_HYDRATEDseed at mount. Reducer transitions remain session-local; consumer reads now have a stable source.Parallel-only invariant (W1 inheritance)
CHANNEL_HYDRATEDflows through the samedispatchToUnreadStorehelper from PR #1430. The W1 try/catch +onErrorguard remains. The new Provider effect uses the existing thunk-guardedunreadDispatch.BC invariants (all PASS —
./scripts/bc-check.sh)package.jsonpublic fields unchangeddist/types/index.d.tsexport set unchangedsetFirstUnreadMessageIdremoval (internal API only)internal/internal/scrollPubSubtopic + payload set unchangedTest plan
./scripts/bc-check.sh— BC-1 ~ BC-6 PASSyarn jest— 197/199 PASS (2 pre-existing skipped, 0 fail), 1135 tests + 10 skipped — +10 new specs over refactor(group-channel): Phase 5.1 unread dispatch + read hook infra (stacked on #1428) #1430 baselineyarn buildcleanMid-cycle scope reduction (AC-5)
The original AC-5 plan was to mount the full
<GroupChannelProvider>stack with a stubbed Sendbird/coreTs surface and drive 6 user scenarios through the real MessageList consumer. The first attempt OOM'd jest (likely an effect loop from incomplete mocks). Per Plan §5 risk mitigation, the test was reduced to a<GroupChannelUnreadContext.Provider>+ probe consumer pattern that exercises the same dispatch → reducer → selector contract. A heavier app-shell test is a follow-up if uikit-tools API drift becomes a real concern. Documented in.agentic/p0-phase-5-2b/spec.md(test file header) and the 5.2.b.c commit body.Out of scope (intentional)
unreadSinceDatenon-MAU label migration (wall-clock semantic, separate decision)MessageView.newMessageIdsautoscroll trigger migration (autoscroll, not unread)Channel/*module (different state shape)MESSAGES_DELETEDdispatch (pending uikit-tools version bump foronMessagesDeletedcallback)currentChannel.unreadMessageCountdirect reads (server-truth display; reducer'sunreadCountis a session counter, distinct concept)parity.spec.tsagainst the simplified legacy model — kept as a unit test of the reducer in isolation. The new integration spec covers the real consumer contract.Stacking note
Base =
refactor/p0-phase-5-1-unread-consumer(PR #1430). Once #1430 merges, the base updates; this PR can then be rebased onto main directly. #1428 (Phase 4) underlies #1430 — typical merge order is #1428 → #1430 → #1431.🤖 Generated with Claude Code