refactor: migrate MessageComposer state to a per-instance Zustand store#7381
refactor: migrate MessageComposer state to a per-instance Zustand store#7381diegolmello wants to merge 2 commits into
Conversation
Replace the eight split React contexts + useReducer in MessageComposer's state container with a single per-instance vanilla Zustand store held in context. The eight public hooks become thin single-slice selectors and the action bag is built once for a stable reference, so the public hook surface and per-slice re-render granularity are unchanged. MessageInnerContext is kept as-is. Add characterization tests pinning the contract the swap must preserve: set->read per slice, attachment add/update/remove/clear semantics, per-slice and per-instance render granularity, and api reference stability.
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
WalkthroughRefactor MessageComposerProvider from useReducer + nested contexts to a per-provider Zustand store. All composer hooks now select slices from the store and throw outside the provider. Add characterization tests that validate store isolation, slice re-render granularity, attachment operations, and API reference stability. ChangesMessageComposer State Management Refactor
Sequence Diagram(s)sequenceDiagram
participant MessageComposerProvider
participant createComposerStore
participant ComposerStoreContext
participant useMessageComposerApi
participant useComposerAttachments
MessageComposerProvider->>createComposerStore: create store instance with State and actions
MessageComposerProvider->>ComposerStoreContext: provide store instance
useMessageComposerApi->>ComposerStoreContext: useStore(select api)
useComposerAttachments->>ComposerStoreContext: useStore(select attachments)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
requested a deployment
to
android_build
GitHub Actions
Waiting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/containers/MessageComposer/context.tsx (1)
50-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep attachment
pathimmutable during updates.
pathis the identity key for bothupdateAttachmentandremoveAttachment, but this merge lets an incoming patch replace it. Once that happens, later operations using the original key stop matching and attachment identity can drift unexpectedly. Preserve the existingpathhere, or explicitly exclude it from the patch contract.Suggested fix
updateAttachment: (path, attachment) => set(state => ({ attachments: state.attachments.map(currentAttachment => - currentAttachment.path === path ? { ...currentAttachment, ...attachment } : currentAttachment + currentAttachment.path === path + ? { ...currentAttachment, ...attachment, path: currentAttachment.path } + : currentAttachment ) })),🤖 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 `@app/containers/MessageComposer/context.tsx` around lines 50 - 55, updateAttachment currently merges the incoming attachment patch into the existing attachment which allows the patch to overwrite the identity key `path`, breaking later lookups (and matching in removeAttachment). Change updateAttachment so it preserves the existing attachment.path when applying the patch: locate the updateAttachment implementation that maps over state.attachments and, for the matching currentAttachment.path, merge the patch into the existing attachment but explicitly keep currentAttachment.path (e.g., spread currentAttachment first and then spread the patch while ensuring path = currentAttachment.path or by omitting path from the patch) so incoming patches cannot change the identity key used by removeAttachment and subsequent operations.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In `@app/containers/MessageComposer/context.tsx`:
- Around line 50-55: updateAttachment currently merges the incoming attachment
patch into the existing attachment which allows the patch to overwrite the
identity key `path`, breaking later lookups (and matching in removeAttachment).
Change updateAttachment so it preserves the existing attachment.path when
applying the patch: locate the updateAttachment implementation that maps over
state.attachments and, for the matching currentAttachment.path, merge the patch
into the existing attachment but explicitly keep currentAttachment.path (e.g.,
spread currentAttachment first and then spread the patch while ensuring path =
currentAttachment.path or by omitting path from the patch) so incoming patches
cannot change the identity key used by removeAttachment and subsequent
operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c2b13cf-55a5-4298-8470-40d7b1105644
📒 Files selected for processing (1)
app/containers/MessageComposer/context.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/containers/MessageComposer/context.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode and baseUrl set to app/ for import resolution
Files:
app/containers/MessageComposer/context.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use Prettier with tabs, single quotes, 130 char width, no trailing commas, arrow parens avoid, bracket same line
Use@rocket.chat/eslint-configbase with React, React Native, TypeScript, Jest plugins
Files:
app/containers/MessageComposer/context.tsx
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Reusable UI components should be placed in app/containers/ directory
Files:
app/containers/MessageComposer/context.tsx
🧠 Learnings (5)
📓 Common learnings
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6808
File: app/containers/MessageComposer/components/ComposerInput.tsx:337-341
Timestamp: 2026-04-04T21:34:30.268Z
Learning: In Rocket.Chat React Native, the markdown composer's autocomplete insertion (ComposerInput.tsx onAutocompleteItemSelected) does NOT need to add a space between an underscore italic delimiter `_` and a `@` or `#` mention sigil. The web platform (using the same rocket.chat/message-parser) does not add such a space either, so parity with web is the correct behavior. The previous learning about "space between `_` and mention sigil" applies only to test/story file content strings, not to the composer's runtime autocomplete behavior.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-18T14:40:38.892Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : VoIP service implementation should use Zustand stores (not Redux) and include native CallKit (iOS) and Telecom (Android) integration in app/lib/services/voip/
📚 Learning: 2026-05-18T14:40:38.892Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-18T14:40:38.892Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : VoIP service implementation should use Zustand stores (not Redux) and include native CallKit (iOS) and Telecom (Android) integration in app/lib/services/voip/
Applied to files:
app/containers/MessageComposer/context.tsx
📚 Learning: 2026-03-10T15:21:45.098Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7046
File: app/containers/InAppNotification/NotifierComponent.stories.tsx:46-75
Timestamp: 2026-03-10T15:21:45.098Z
Learning: In `app/containers/InAppNotification/NotifierComponent.tsx` (React Native, Rocket.Chat), `NotifierComponent` is exported as a Redux-connected component via `connect(mapStateToProps)`. The `isMasterDetail` prop is automatically injected from `state.app.isMasterDetail` and does not need to be passed explicitly at call sites or in Storybook stories that use the default (connected) export.
Applied to files:
app/containers/MessageComposer/context.tsx
📚 Learning: 2026-04-04T21:34:30.268Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6808
File: app/containers/MessageComposer/components/ComposerInput.tsx:337-341
Timestamp: 2026-04-04T21:34:30.268Z
Learning: In Rocket.Chat React Native, the markdown composer's autocomplete insertion (ComposerInput.tsx onAutocompleteItemSelected) does NOT need to add a space between an underscore italic delimiter `_` and a `@` or `#` mention sigil. The web platform (using the same rocket.chat/message-parser) does not add such a space either, so parity with web is the correct behavior. The previous learning about "space between `_` and mention sigil" applies only to test/story file content strings, not to the composer's runtime autocomplete behavior.
Applied to files:
app/containers/MessageComposer/context.tsx
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/containers/MessageComposer/context.tsx
🔇 Additional comments (1)
app/containers/MessageComposer/context.tsx (1)
65-69: Verify the new provider-only contract is intentional.These hooks now throw instead of falling back to default context values. If any supported story, test, or edge render path still uses them outside
MessageComposerProvider, this refactor turns that into an immediate render crash, so please confirm that behavior change is expected before merge.
1 participant
Proposed changes
Migrates the MessageComposer state container from eight split React contexts +
useReducerto a single per-instance vanilla Zustand store held in context. Theeight public hooks become thin single-slice selectors and the action bag is built
once for a stable reference, so the public hook surface, per-slice re-render
granularity, and per-instance isolation are all preserved.
MessageInnerContextis unchanged. Net −66 lines in
context.tsx(8 providers → 1).Behavior-preserving refactor — no user-facing change. It also clears the way to
later move
text/selectioninto the store (deferred).Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1233
How to test or reproduce
Internal refactor, no visible behavior change. To verify:
TZ=UTC pnpm test --testPathPattern='MessageComposer'— 112 tests + 23 snapshots green.(add/remove), autocomplete, and "also send to channel" in a thread behave as before.
Screenshots
No UI changes.
Types of changes
Checklist
Further comments
Why per-instance: two composers (e.g. a channel and an open thread) are mounted at
the same time, so each needs isolated state — a single global store would share it.
Keeping the split contexts was the other option, but it requires eight providers and
doesn't reduce the context fan-out this change targets.
Constraints honored:
'use memo'retained for React Compiler annotation mode, storelazy-initialized,
actionskept referentially stable to avoid Zustand v5"snapshot changed" re-render loops, and
text/selectiondeliberately left asuncontrolled refs (not moved into the store). Characterization tests in
context.test.tsxpin the full contract.Summary by CodeRabbit
Tests
Refactor