refactor: migrate Message rendering to a per-message store#7455
refactor: migrate Message rendering to a per-message store#7455diegolmello wants to merge 67 commits into
Conversation
…state vocabulary Rename UBIQUITOUS_LANGUAGE.md to CONTEXT.md (history preserved) and update its three references in CLAUDE.md, the VoIP docs README, and the RoomView architecture doc. Add the "Message Interaction & Position State" section distinguishing Interaction state (selection plus the reply/quote/edit/react action, owned by the per-Room interaction store) from Positional state (highlight plus jump/scroll position, owned by the Room view scroll machinery), since the two are migrated independently. Ref: NATIVE-1345 Claude-Session: https://claude.ai/code/session_01Cj1jHhEbiC9mWGe8dpqQeS
Switch every @json-decorated WatermelonDB model field to the memoized form
({ memo: true }) so an unchanged nested JSON value keeps its reference identity
across reads, enabling per-field re-render bailout once consumers adopt it.
The memo cache is invalidated only when the raw column string changes, so
in-place mutation of a memoized value would silently corrupt it. Convert the
two remaining in-place mutations to replacement:
- ReactionsList sorts a copy ([...reactions].sort) instead of the field.
- encryption quote-attachment decryption reassigns message.attachments instead
of pushing into it.
Ref: NATIVE-1346
Claude-Session: https://claude.ai/code/session_01Cj1jHhEbiC9mWGe8dpqQeS
Introduce useMessage, a reactive hook that takes a live WatermelonDB Message model and returns a plain Message Snapshot via useSyncExternalStore. It subscribes to model mutations, paints synchronously on first render with no flash, and reads the memoized @JSON getters directly so unchanged fields keep their reference for per-field re-render bailout. All WatermelonDB specifics are confined to the hook; it is not yet wired into the row container.
Convert MessageContainer from a class to a function component. - Replace manual experimentalSubscribe + forceUpdate with the reactive useMessage snapshot hook; model mutations now re-render through useSyncExternalStore. - Replace shouldComponentUpdate with React.memo and an equivalent custom comparator over the same nine props. - Move isManualUnignored to useState, theming to useTheme, and defaultProps to destructuring parameter defaults. - Keep the debounced onPress as a single lifetime instance via a ref, always invoking the latest closure. Render output is unchanged: the existing Message snapshot suite passes without updates. Adds a behavior test for the row container.
Move `action` and `selectedMessages` out of the RoomView class state into a per-Room Zustand store (InteractionStore), mirroring the MessageComposer store topology. Message rows now read their edit state via a `useIsBeingEdited` selector instead of a prop, so starting or cancelling an edit re-renders only the affected row instead of triggering a RoomView render and a FlatList walk. Composer consumers read the interaction values from the store while keeping their handlers on RoomContext, so the RoomContext value no longer changes when interaction state changes. ShareView provides its own store for its composer subtree. Behavior is unchanged.
Remove the umbrella IMessage/IMessageInner interfaces and the {...props}
bag spread from the message component tree. Each presentational component
now declares the narrow interface it consumes and receives explicit props.
Shared handlers (getCustomEmoji, navToRoomInfo, showAttachment, blockAction,
handleEnterCall, fetchThreadName) are read from MessageContext rather than
threaded as props, and MessageContext is now typed (IMessageContext) in
place of `any`. Behavior is unchanged.
…NATIVE-1351) Add the `'use memo'` opt-in directive to the three presentational memo components in Message.tsx (MessageInner, Message, MessageTouchable), keeping their explicit React.memo gate. This mirrors the existing precedent in message/index.tsx (MessageContainer) under the project's React Compiler annotation mode. Clean Message.stories.tsx to pass only the explicit data props each story exercises: drop the per-story handler props (onReactionPress, onErrorPress, replyBroadcast) and the dead `theme` prop, all of which are now supplied through the MessageContext provider value. Behavior is unchanged — the snapshot suite passes without updates.
Drop three confirmed-dead members from the RoomView container state: the unused reaction-modal visibility flag (reactionsModalVisible), the unused reply-with-mention flag (replyWithMention), and the redundant on-hold flag (isOnHold). reactionsModalVisible and replyWithMention were only ever initialized and type-declared, never read or re-set. isOnHold was set alongside roomUpdate and only compared in shouldComponentUpdate; that comparison is redundant because roomUpdate already carries onHold (it is part of roomAttrsUpdate), so on-hold transitions still trigger a re-render through the existing roomUpdate diff. No behavior change — the full test suite passes without snapshot updates.
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR moves room interaction state into a Zustand-backed store, adds a per-message store for derived message data, refactors message rendering and attachment components to use typed context/hooks, updates WatermelonDB JSON decorators to memoize sanitization, and switches documentation references to ChangesInteractionStore and message rendering
WatermelonDB JSON Decorator Memoization
Documentation Reference Updates
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (9)
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
temporarily deployed
to
android_build
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/lib/database/model/Subscription.js (1)
40-162: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftClone memoized JSON fields before exposing them from
asPlain()
@json(..., { memo: true })returns the same object reference across reads, andasPlain()only creates a shallow snapshot. The JSON-backed fields are still shared by reference, so mutating them through the returned “plain” object mutates the model cache too. Clone or freeze those fields before returning them.🤖 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/lib/database/model/Subscription.js` around lines 40 - 162, The issue is that memoized `@json` fields are being exposed by asPlain() by reference, so the “plain” snapshot still shares mutable objects with the model cache. Update asPlain() in Subscription to deep-clone or freeze the memoized JSON-backed properties (those declared with `@json`(..., { memo: true })) before returning them, so reads from the plain object cannot mutate the cached model state.app/containers/message/Components/Attachments/Reply.tsx (1)
219-228: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset
loadingon preview failures.If
fileDownloadAndPreview()rejects here,setLoading(false)never runs and this reply stays disabled behind the spinner. Wrap the awaited path intry/catch/finallyso the UI always recovers and the failure is handled explicitly. As per coding guidelines, "Use explicit error handling with try/catch blocks for async operations." Based on learnings, this repo only treats async event handlers as a review concern when there is a real unhandled promise path.Suggested fix
if (attachment.type === 'file' && attachment.title_link) { setLoading(true); - url = formatAttachmentUrl(attachment.title_link, user?.id ?? '', user?.token ?? '', baseUrl ?? ''); - await fileDownloadAndPreview(url, attachment, id ?? ''); - setLoading(false); + try { + url = formatAttachmentUrl(attachment.title_link, user?.id ?? '', user?.token ?? '', baseUrl ?? ''); + await fileDownloadAndPreview(url, attachment, id ?? ''); + } catch (e) { + // surface/log the failure here + } finally { + setLoading(false); + } return; }🤖 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/message/Components/Attachments/Reply.tsx` around lines 219 - 228, In Reply.tsx, the onPress async handler leaves loading stuck true if fileDownloadAndPreview() rejects, so update the file-download branch in onPress to use explicit try/catch/finally around the awaited call and always reset loading in the finally block. Keep the existing attachment.url formatting logic, but make sure any preview failure is handled in the catch path so the reply UI can recover instead of staying disabled.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (4)
app/views/RoomView/InteractionStore.tsx (2)
6-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse interfaces for the store object shapes.
TInteractionActionsandInteractionStatedefine object shapes, so they should be interfaces per the TS guideline.Refactor sketch
-type TInteractionActions = { +interface IInteractionActions { setEditing(messageId: string): void; initQuote(messageId: string): void; appendQuote(messageId: string): void; removeQuote(messageId: string): void; setReacting(messageId: string): void; setQuotes(messageIds: string[]): void; reset(): void; -}; +} -type InteractionState = { +interface IInteractionState { action: TMessageAction; selectedMessages: string[]; // Built once inside the store initializer — stable ref for consumers. - actions: TInteractionActions; -}; + actions: IInteractionActions; +}As per coding guidelines, “Prefer interfaces over type aliases for defining object shapes in TypeScript”.
🤖 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/views/RoomView/InteractionStore.tsx` around lines 6 - 21, The store object shapes in TInteractionActions and InteractionState should use interfaces instead of type aliases. Update the declarations in InteractionStore to convert both shape definitions to interfaces while keeping the same members and preserving the stable actions reference used by the store initializer.Source: Coding guidelines
23-38: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCopy
selectedMessagesat the store boundary.
initialState.selectedMessagesandsetQuotes(messageIds)currently retain caller-owned array references. Copy them before storing so external mutations cannot change Zustand state without notifying subscribers.Proposed fix
- selectedMessages: initialState?.selectedMessages ?? [], + selectedMessages: initialState?.selectedMessages ? [...initialState.selectedMessages] : [], ... - setQuotes: messageIds => set({ action: messageIds.length ? 'quote' : null, selectedMessages: messageIds }), + setQuotes: messageIds => set({ action: messageIds.length ? 'quote' : null, selectedMessages: [...messageIds] }),🤖 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/views/RoomView/InteractionStore.tsx` around lines 23 - 38, The InteractionStore state is keeping external array references for selectedMessages, which can let caller mutations bypass Zustand updates. In createInteractionStore, copy initialState.selectedMessages when initializing state, and in the setQuotes action copy the incoming messageIds before saving them. Use the existing createInteractionStore and actions.setQuotes symbols to locate the boundary where the array should be cloned.app/containers/MessageComposer/components/CancelEdit.tsx (1)
6-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit return type to
CancelEdit.This changed TSX component still relies on inference. Please annotate the return type explicitly so the exported contract stays obvious.
As per coding guidelines,
**/*.{ts,tsx}:Use TypeScript for type safety; add explicit type annotations to function parameters and return types.🤖 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/components/CancelEdit.tsx` around lines 6 - 10, The CancelEdit component is still relying on inferred return typing, so make its exported contract explicit. Update the CancelEdit function declaration to include an explicit return type, using the function name CancelEdit as the anchor, while keeping its current use of useRoomContext and useMessageAction unchanged.Source: Coding guidelines
app/containers/MessageComposer/hooks/useAutoSaveDraft.ts (1)
9-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the
useAutoSaveDraftsignature explicit.
textand the returned shape are both inferred here. Please add explicit parameter and return types so this exported hook’s contract is clear at the call site.As per coding guidelines,
**/*.{ts,tsx}:Use TypeScript for type safety; add explicit type annotations to function parameters and return types.🤖 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/hooks/useAutoSaveDraft.ts` around lines 9 - 15, The exported hook useAutoSaveDraft currently relies on inference for both its input and return value, so make the contract explicit by adding a type annotation for the text parameter and an explicit return type on useAutoSaveDraft. Keep the existing logic intact, but ensure the returned object shape from the hook is declared so call sites can rely on the typed API even if internals change.Source: Coding guidelines
🤖 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.
Inline comments:
In `@app/containers/message/Components/Attachments/Reply.tsx`:
- Line 168: The thumbnail URL fallback in Reply should not interpolate baseUrl
when it may be missing, since that can produce invalid undefined/... paths.
Update the relative-image handling in Reply.tsx to guard the baseUrl-dependent
branch in the same place where image is normalized, so quoted attachment
thumbnails fail closed when MessageContext is partial or default. Use the
existing Reply component’s image assignment logic and preserve the http check,
but only build the relative URL when baseUrl is available.
In `@app/containers/message/index.tsx`:
- Around line 248-253: The link handling in onLinkPress incorrectly treats
external links as message links when message.attachments is undefined because
the optional chain makes the comparison to -1 evaluate true. Update the
isMessageLink check in message/containers/message/index.tsx to only return true
when attachments actually exist and a matching attachment.message_link is found,
so jumpToMessage is only called for real message links and openLink remains the
fallback.
- Around line 69-79: The memo comparator in areEqual is missing item and other
render-consumed props, so it can incorrectly skip updates and leave
MessageContainer rendering stale data. Update areEqual to compare prev.item and
every prop that affects rendering or hook inputs such as useMessage(next.item),
or remove the custom comparator entirely if exhaustive comparison is
impractical. Keep the check aligned with IMessageContainerProps and the values
actually used inside the message container render path.
In `@app/lib/encryption/room.ts`:
- Around line 731-732: The `message.attachments` update in `room.ts` is racing
across concurrent `Promise.all` branches, so later URL decryptions can overwrite
earlier quote attachments. Update the quote-handling flow in the relevant
decrypt/attach logic to collect all new quote attachments first, then assign to
`message.attachments` once after `Promise.all` completes, using the existing
`message.attachments` and quote attachment creation path as the reference
points.
In `@app/views/ShareView/index.tsx`:
- Line 243: ShareView is still using local state as the quote source in send()
and unmount, which can drift from the interactionStore updates made through
setQuotes(selectedMessages). Update ShareView to read the current quote/action
data from interactionStore everywhere it needs quote state, and keep the local
state in sync only if necessary. Use the existing ShareView methods and fields
around send(), componentWillUnmount, this.state.action, and
this.state.selectedMessages to replace stale reads with store-backed values.
---
Outside diff comments:
In `@app/containers/message/Components/Attachments/Reply.tsx`:
- Around line 219-228: In Reply.tsx, the onPress async handler leaves loading
stuck true if fileDownloadAndPreview() rejects, so update the file-download
branch in onPress to use explicit try/catch/finally around the awaited call and
always reset loading in the finally block. Keep the existing attachment.url
formatting logic, but make sure any preview failure is handled in the catch path
so the reply UI can recover instead of staying disabled.
In `@app/lib/database/model/Subscription.js`:
- Around line 40-162: The issue is that memoized `@json` fields are being exposed
by asPlain() by reference, so the “plain” snapshot still shares mutable objects
with the model cache. Update asPlain() in Subscription to deep-clone or freeze
the memoized JSON-backed properties (those declared with `@json`(..., { memo: true
})) before returning them, so reads from the plain object cannot mutate the
cached model state.
---
Nitpick comments:
In `@app/containers/MessageComposer/components/CancelEdit.tsx`:
- Around line 6-10: The CancelEdit component is still relying on inferred return
typing, so make its exported contract explicit. Update the CancelEdit function
declaration to include an explicit return type, using the function name
CancelEdit as the anchor, while keeping its current use of useRoomContext and
useMessageAction unchanged.
In `@app/containers/MessageComposer/hooks/useAutoSaveDraft.ts`:
- Around line 9-15: The exported hook useAutoSaveDraft currently relies on
inference for both its input and return value, so make the contract explicit by
adding a type annotation for the text parameter and an explicit return type on
useAutoSaveDraft. Keep the existing logic intact, but ensure the returned object
shape from the hook is declared so call sites can rely on the typed API even if
internals change.
In `@app/views/RoomView/InteractionStore.tsx`:
- Around line 6-21: The store object shapes in TInteractionActions and
InteractionState should use interfaces instead of type aliases. Update the
declarations in InteractionStore to convert both shape definitions to interfaces
while keeping the same members and preserving the stable actions reference used
by the store initializer.
- Around line 23-38: The InteractionStore state is keeping external array
references for selectedMessages, which can let caller mutations bypass Zustand
updates. In createInteractionStore, copy initialState.selectedMessages when
initializing state, and in the setQuotes action copy the incoming messageIds
before saving them. Use the existing createInteractionStore and
actions.setQuotes symbols to locate the boundary where the array should be
cloned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bcc5d87-b924-4943-af20-abe404e704ac
⛔ Files ignored due to path filters (1)
app/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (64)
.rnstorybook/preview.tsxCLAUDE.mdCONTEXT.mdapp/containers/MessageComposer/MessageComposer.test.tsxapp/containers/MessageComposer/MessageComposer.tsxapp/containers/MessageComposer/components/CancelEdit.tsxapp/containers/MessageComposer/components/ComposerInput.tsxapp/containers/MessageComposer/components/Quotes/Quotes.tsxapp/containers/MessageComposer/hooks/useAutoSaveDraft.tsapp/containers/MessageComposer/hooks/useChooseMedia.test.tsxapp/containers/MessageComposer/hooks/useChooseMedia.tsapp/containers/ReactionsList/index.tsxapp/containers/message/Blocks.tsapp/containers/message/Broadcast.tsxapp/containers/message/CallButton.tsxapp/containers/message/Components/Attachments/AttachedActions.tsxapp/containers/message/Components/Attachments/Attachments.stories.tsxapp/containers/message/Components/Attachments/Attachments.tsxapp/containers/message/Components/Attachments/Audio.tsxapp/containers/message/Components/Attachments/CollapsibleQuote/index.tsxapp/containers/message/Components/Attachments/Image/Container.tsxapp/containers/message/Components/Attachments/Quote.tsxapp/containers/message/Components/Attachments/Reply.tsxapp/containers/message/Components/Attachments/Video.tsxapp/containers/message/Content.test.tsxapp/containers/message/Content.tsxapp/containers/message/Context.tsapp/containers/message/Message.stories.tsxapp/containers/message/Message.tsxapp/containers/message/MessageAvatar.tsxapp/containers/message/Reactions.test.tsxapp/containers/message/Reactions.tsxapp/containers/message/RepliedThread.tsxapp/containers/message/Thread.tsxapp/containers/message/Urls.tsxapp/containers/message/User.tsxapp/containers/message/hooks/useMediaAutoDownload.tsxapp/containers/message/hooks/useMessage.test.tsapp/containers/message/hooks/useMessage.tsapp/containers/message/hooks/useMessageAccessibilityLabel.test.tsapp/containers/message/hooks/useMessageAccessibilityLabel.tsapp/containers/message/index.test.tsxapp/containers/message/index.tsxapp/containers/message/interfaces.tsapp/lib/database/model/CustomEmoji.jsapp/lib/database/model/Message.jsapp/lib/database/model/Permission.jsapp/lib/database/model/Room.jsapp/lib/database/model/Setting.jsapp/lib/database/model/Subscription.jsapp/lib/database/model/Thread.jsapp/lib/database/model/ThreadMessage.jsapp/lib/database/model/User.jsapp/lib/database/model/servers/Server.jsapp/lib/database/model/servers/User.jsapp/lib/encryption/room.tsapp/lib/services/voip/docs/README.mdapp/views/RoomView/InteractionStore.tsxapp/views/RoomView/constants.tsapp/views/RoomView/context.tsapp/views/RoomView/definitions.tsapp/views/RoomView/docs/ARCHITECTURE.mdapp/views/RoomView/index.tsxapp/views/ShareView/index.tsx
💤 Files with no reviewable changes (2)
- app/views/RoomView/constants.ts
- app/views/RoomView/context.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/message/Components/Attachments/Audio.tsxapp/containers/message/Components/Attachments/Image/Container.tsxapp/containers/MessageComposer/components/Quotes/Quotes.tsxapp/containers/message/Broadcast.tsxapp/containers/message/Components/Attachments/Video.tsxapp/lib/database/model/User.jsapp/containers/message/Thread.tsxapp/containers/message/Content.tsxapp/containers/message/Components/Attachments/AttachedActions.tsxapp/lib/database/model/Permission.jsapp/containers/message/Urls.tsxapp/containers/message/Components/Attachments/CollapsibleQuote/index.tsxapp/lib/database/model/servers/Server.jsapp/containers/message/Reactions.test.tsxapp/containers/ReactionsList/index.tsxapp/containers/MessageComposer/hooks/useChooseMedia.tsapp/containers/MessageComposer/components/CancelEdit.tsxapp/containers/message/RepliedThread.tsxapp/containers/message/hooks/useMessageAccessibilityLabel.test.tsapp/lib/database/model/CustomEmoji.jsapp/lib/database/model/servers/User.jsapp/containers/message/Components/Attachments/Quote.tsxapp/containers/message/hooks/useMessage.tsapp/containers/message/hooks/useMessage.test.tsapp/lib/encryption/room.tsapp/containers/MessageComposer/hooks/useChooseMedia.test.tsxapp/lib/database/model/Setting.jsapp/containers/message/index.test.tsxapp/containers/message/Blocks.tsapp/lib/database/model/ThreadMessage.jsapp/containers/MessageComposer/hooks/useAutoSaveDraft.tsapp/containers/message/CallButton.tsxapp/views/RoomView/InteractionStore.tsxapp/containers/message/hooks/useMediaAutoDownload.tsxapp/containers/message/Context.tsapp/lib/database/model/Message.jsapp/containers/message/Components/Attachments/Attachments.stories.tsxapp/containers/MessageComposer/components/ComposerInput.tsxapp/containers/message/Components/Attachments/Attachments.tsxapp/lib/database/model/Room.jsapp/containers/message/MessageAvatar.tsxapp/views/ShareView/index.tsxapp/containers/message/hooks/useMessageAccessibilityLabel.tsapp/containers/message/Content.test.tsxapp/lib/database/model/Subscription.jsapp/containers/MessageComposer/MessageComposer.test.tsxapp/views/RoomView/definitions.tsapp/lib/database/model/Thread.jsapp/containers/MessageComposer/MessageComposer.tsxapp/containers/message/User.tsxapp/containers/message/Reactions.tsxapp/containers/message/Components/Attachments/Reply.tsxapp/containers/message/Message.stories.tsxapp/containers/message/Message.tsxapp/containers/message/interfaces.tsapp/views/RoomView/index.tsxapp/containers/message/index.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 numbers
Files:
app/containers/message/Components/Attachments/Audio.tsxapp/containers/message/Components/Attachments/Image/Container.tsxapp/containers/MessageComposer/components/Quotes/Quotes.tsxapp/containers/message/Broadcast.tsxapp/containers/message/Components/Attachments/Video.tsxapp/containers/message/Thread.tsxapp/containers/message/Content.tsxapp/containers/message/Components/Attachments/AttachedActions.tsxapp/containers/message/Urls.tsxapp/containers/message/Components/Attachments/CollapsibleQuote/index.tsxapp/containers/message/Reactions.test.tsxapp/containers/ReactionsList/index.tsxapp/containers/MessageComposer/hooks/useChooseMedia.tsapp/containers/MessageComposer/components/CancelEdit.tsxapp/containers/message/RepliedThread.tsxapp/containers/message/hooks/useMessageAccessibilityLabel.test.tsapp/containers/message/Components/Attachments/Quote.tsxapp/containers/message/hooks/useMessage.tsapp/containers/message/hooks/useMessage.test.tsapp/lib/encryption/room.tsapp/containers/MessageComposer/hooks/useChooseMedia.test.tsxapp/containers/message/index.test.tsxapp/containers/message/Blocks.tsapp/containers/MessageComposer/hooks/useAutoSaveDraft.tsapp/containers/message/CallButton.tsxapp/views/RoomView/InteractionStore.tsxapp/containers/message/hooks/useMediaAutoDownload.tsxapp/containers/message/Context.tsapp/containers/message/Components/Attachments/Attachments.stories.tsxapp/containers/MessageComposer/components/ComposerInput.tsxapp/containers/message/Components/Attachments/Attachments.tsxapp/containers/message/MessageAvatar.tsxapp/views/ShareView/index.tsxapp/containers/message/hooks/useMessageAccessibilityLabel.tsapp/containers/message/Content.test.tsxapp/containers/MessageComposer/MessageComposer.test.tsxapp/views/RoomView/definitions.tsapp/containers/MessageComposer/MessageComposer.tsxapp/containers/message/User.tsxapp/containers/message/Reactions.tsxapp/containers/message/Components/Attachments/Reply.tsxapp/containers/message/Message.stories.tsxapp/containers/message/Message.tsxapp/containers/message/interfaces.tsapp/views/RoomView/index.tsxapp/containers/message/index.tsx
{index.js,app/**/*.{js,jsx,ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the project’s Prettier style: tabs, single quotes, 130-character line width, no trailing commas, omit arrow-function parens when possible, and keep bracket spacing on the same line.
Files:
app/containers/message/Components/Attachments/Audio.tsxapp/containers/message/Components/Attachments/Image/Container.tsxapp/containers/MessageComposer/components/Quotes/Quotes.tsxapp/containers/message/Broadcast.tsxapp/containers/message/Components/Attachments/Video.tsxapp/lib/database/model/User.jsapp/containers/message/Thread.tsxapp/containers/message/Content.tsxapp/containers/message/Components/Attachments/AttachedActions.tsxapp/lib/database/model/Permission.jsapp/containers/message/Urls.tsxapp/containers/message/Components/Attachments/CollapsibleQuote/index.tsxapp/lib/database/model/servers/Server.jsapp/containers/message/Reactions.test.tsxapp/containers/ReactionsList/index.tsxapp/containers/MessageComposer/hooks/useChooseMedia.tsapp/containers/MessageComposer/components/CancelEdit.tsxapp/containers/message/RepliedThread.tsxapp/containers/message/hooks/useMessageAccessibilityLabel.test.tsapp/lib/database/model/CustomEmoji.jsapp/lib/database/model/servers/User.jsapp/containers/message/Components/Attachments/Quote.tsxapp/containers/message/hooks/useMessage.tsapp/containers/message/hooks/useMessage.test.tsapp/lib/encryption/room.tsapp/containers/MessageComposer/hooks/useChooseMedia.test.tsxapp/lib/database/model/Setting.jsapp/containers/message/index.test.tsxapp/containers/message/Blocks.tsapp/lib/database/model/ThreadMessage.jsapp/containers/MessageComposer/hooks/useAutoSaveDraft.tsapp/containers/message/CallButton.tsxapp/views/RoomView/InteractionStore.tsxapp/containers/message/hooks/useMediaAutoDownload.tsxapp/containers/message/Context.tsapp/lib/database/model/Message.jsapp/containers/message/Components/Attachments/Attachments.stories.tsxapp/containers/MessageComposer/components/ComposerInput.tsxapp/containers/message/Components/Attachments/Attachments.tsxapp/lib/database/model/Room.jsapp/containers/message/MessageAvatar.tsxapp/views/ShareView/index.tsxapp/containers/message/hooks/useMessageAccessibilityLabel.tsapp/containers/message/Content.test.tsxapp/lib/database/model/Subscription.jsapp/containers/MessageComposer/MessageComposer.test.tsxapp/views/RoomView/definitions.tsapp/lib/database/model/Thread.jsapp/containers/MessageComposer/MessageComposer.tsxapp/containers/message/User.tsxapp/containers/message/Reactions.tsxapp/containers/message/Components/Attachments/Reply.tsxapp/containers/message/Message.stories.tsxapp/containers/message/Message.tsxapp/containers/message/interfaces.tsapp/views/RoomView/index.tsxapp/containers/message/index.tsx
🧠 Learnings (5)
📚 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/message/Components/Attachments/Audio.tsxapp/containers/message/Components/Attachments/Image/Container.tsxapp/containers/MessageComposer/components/Quotes/Quotes.tsxapp/containers/message/Broadcast.tsxapp/containers/message/Components/Attachments/Video.tsxapp/containers/message/Thread.tsxapp/containers/message/Content.tsxapp/containers/message/Components/Attachments/AttachedActions.tsxapp/containers/message/Urls.tsxapp/containers/message/Components/Attachments/CollapsibleQuote/index.tsxapp/containers/message/Reactions.test.tsxapp/containers/ReactionsList/index.tsxapp/containers/MessageComposer/hooks/useChooseMedia.tsapp/containers/MessageComposer/components/CancelEdit.tsxapp/containers/message/RepliedThread.tsxapp/containers/message/hooks/useMessageAccessibilityLabel.test.tsapp/containers/message/Components/Attachments/Quote.tsxapp/containers/message/hooks/useMessage.tsapp/containers/message/hooks/useMessage.test.tsapp/lib/encryption/room.tsapp/containers/MessageComposer/hooks/useChooseMedia.test.tsxapp/containers/message/index.test.tsxapp/containers/message/Blocks.tsapp/containers/MessageComposer/hooks/useAutoSaveDraft.tsapp/containers/message/CallButton.tsxapp/views/RoomView/InteractionStore.tsxapp/containers/message/hooks/useMediaAutoDownload.tsxapp/containers/message/Context.tsapp/containers/message/Components/Attachments/Attachments.stories.tsxapp/containers/MessageComposer/components/ComposerInput.tsxapp/containers/message/Components/Attachments/Attachments.tsxapp/containers/message/MessageAvatar.tsxapp/views/ShareView/index.tsxapp/containers/message/hooks/useMessageAccessibilityLabel.tsapp/containers/message/Content.test.tsxapp/containers/MessageComposer/MessageComposer.test.tsxapp/views/RoomView/definitions.tsapp/containers/MessageComposer/MessageComposer.tsxapp/containers/message/User.tsxapp/containers/message/Reactions.tsxapp/containers/message/Components/Attachments/Reply.tsxapp/containers/message/Message.stories.tsxapp/containers/message/Message.tsxapp/containers/message/interfaces.tsapp/views/RoomView/index.tsxapp/containers/message/index.tsx
📚 Learning: 2026-06-25T18:37:44.793Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.tsx:101-141
Timestamp: 2026-06-25T18:37:44.793Z
Learning: In the Rocket.Chat React Native codebase, do not treat passing an `async` function directly to an event prop in React/React Native UI components (e.g., `onPress={async () => ...}` in TSX) as a “floating promises” CI-blocking lint issue—this repo does not enable the ESLint `no-floating-promises` rule (while `no-void` is enforced). Only raise robustness follow-ups when there are genuinely unhandled promise paths (e.g., fire-and-forget calls like `save()` that return a Promise that is neither awaited nor handled), and prefer making sure failure paths are explicitly handled/reported rather than blocking on lint-style floating-promise concerns.
Applied to files:
app/containers/message/Components/Attachments/Audio.tsxapp/containers/message/Components/Attachments/Image/Container.tsxapp/containers/MessageComposer/components/Quotes/Quotes.tsxapp/containers/message/Broadcast.tsxapp/containers/message/Components/Attachments/Video.tsxapp/containers/message/Thread.tsxapp/containers/message/Content.tsxapp/containers/message/Components/Attachments/AttachedActions.tsxapp/containers/message/Urls.tsxapp/containers/message/Components/Attachments/CollapsibleQuote/index.tsxapp/containers/message/Reactions.test.tsxapp/containers/ReactionsList/index.tsxapp/containers/MessageComposer/components/CancelEdit.tsxapp/containers/message/RepliedThread.tsxapp/containers/message/Components/Attachments/Quote.tsxapp/containers/MessageComposer/hooks/useChooseMedia.test.tsxapp/containers/message/index.test.tsxapp/containers/message/CallButton.tsxapp/views/RoomView/InteractionStore.tsxapp/containers/message/hooks/useMediaAutoDownload.tsxapp/containers/message/Components/Attachments/Attachments.stories.tsxapp/containers/MessageComposer/components/ComposerInput.tsxapp/containers/message/Components/Attachments/Attachments.tsxapp/containers/message/MessageAvatar.tsxapp/views/ShareView/index.tsxapp/containers/message/Content.test.tsxapp/containers/MessageComposer/MessageComposer.test.tsxapp/containers/MessageComposer/MessageComposer.tsxapp/containers/message/User.tsxapp/containers/message/Reactions.tsxapp/containers/message/Components/Attachments/Reply.tsxapp/containers/message/Message.stories.tsxapp/containers/message/Message.tsxapp/views/RoomView/index.tsxapp/containers/message/index.tsx
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/containers/message/Reactions.test.tsxapp/containers/message/hooks/useMessageAccessibilityLabel.test.tsapp/containers/message/hooks/useMessage.test.tsapp/containers/MessageComposer/hooks/useChooseMedia.test.tsxapp/containers/message/index.test.tsxapp/containers/message/Content.test.tsxapp/containers/MessageComposer/MessageComposer.test.tsx
📚 Learning: 2026-06-24T22:58:43.390Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7157
File: app/views/MessagesView/index.tsx:392-392
Timestamp: 2026-06-24T22:58:43.390Z
Learning: When wrapping a React Native component (e.g., via `withSafeAreaInsets`) ensure `hoistNonReactStatics` is only required if the wrapped component actually defines static properties/methods that consumers rely on. If the component has no statics (as in `app/views/MessagesView/index.tsx`), you can omit `hoistNonReactStatics` for this case.
Applied to files:
app/views/RoomView/InteractionStore.tsxapp/views/ShareView/index.tsxapp/views/RoomView/index.tsx
📚 Learning: 2026-03-15T13:55:42.038Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6911
File: app/containers/markdown/Markdown.stories.tsx:104-104
Timestamp: 2026-03-15T13:55:42.038Z
Learning: In Rocket.Chat React Native, the markdown parser requires a space between the underscore wrapping italic text and a mention sigil (_ mention _ instead of _mention_). Ensure stories and tests that include italic-wrapped mentions follow this form to guarantee proper parsing. Specifically, for files like app/containers/markdown/Markdown.stories.tsx, and any test/content strings that exercise italic-mentions, use the pattern _ mention _ (with spaces) to prevent the mention from being treated as plain text. Validate any test strings or story content accordingly.
Applied to files:
app/containers/message/Components/Attachments/Attachments.stories.tsxapp/containers/message/Message.stories.tsx
🔇 Additional comments (51)
CONTEXT.md (1)
55-63: LGTM!CLAUDE.md (1)
12-12: LGTM!app/lib/services/voip/docs/README.md (1)
15-15: LGTM!app/views/RoomView/docs/ARCHITECTURE.md (1)
3-3: LGTM!app/containers/message/hooks/useMessageAccessibilityLabel.ts (1)
4-27: LGTM!Also applies to: 29-49
app/containers/message/hooks/useMessageAccessibilityLabel.test.ts (1)
4-11: LGTM!app/containers/ReactionsList/index.tsx (1)
33-33: LGTM!app/containers/message/Broadcast.tsx (1)
20-20: LGTM!app/containers/message/Components/Attachments/AttachedActions.tsx (1)
28-28: LGTM!app/containers/message/hooks/useMediaAutoDownload.tsx (1)
65-71: 🗄️ Data Integrity & IntegrationEmpty-string fallbacks need a contract check
Passing
''for missingid,baseUrl, oruserintouseFile(),formatAttachmentUrl(), anddownloadMediaFile()may turn missing context into real cache or network keys. If those helpers don’t treat empty strings as invalid sentinels, this can produce malformed URLs or collisions.app/containers/message/Components/Attachments/Audio.tsx (1)
26-27: LGTM!app/containers/message/Components/Attachments/CollapsibleQuote/index.tsx (1)
96-96: LGTM!Also applies to: 119-119
app/containers/message/Components/Attachments/Image/Container.tsx (1)
55-55: LGTM!app/containers/message/Components/Attachments/Video.tsx (1)
104-104: LGTM!app/containers/message/Urls.tsx (1)
133-134: LGTM!app/views/RoomView/InteractionStore.tsx (1)
44-79: LGTM!app/views/RoomView/definitions.ts (1)
9-9: LGTM!Also applies to: 63-67
app/views/RoomView/index.tsx (2)
101-101: LGTM!Also applies to: 146-175, 224-226, 788-846, 848-858, 876-893, 1385-1507, 1642-1719
268-301: 🎯 Functional CorrectnessKeep
RoomViewinvalidating onroom.onHoldchanges
renderFooter()still branches onroom.onHold; ifroomAttrsUpdateno longer includes that field, livechat hold/resume updates will be blocked byshouldComponentUpdate.app/views/ShareView/index.tsx (1)
42-42: LGTM!Also applies to: 84-94, 392-422
app/containers/MessageComposer/hooks/useChooseMedia.ts (2)
13-13: LGTM!
35-37: 🩺 Stability & AvailabilityVerify every media-picker composer path is wrapped in
InteractionStoreContext.useMessageAction()anduseSelectedMessages()crash without it, so any remainingMessageComposerContaineroruseChooseMediaentry point outside the provider will fail.app/containers/MessageComposer/MessageComposer.tsx (1)
8-8: LGTM!Also applies to: 58-60
app/containers/MessageComposer/components/ComposerInput.tsx (1)
36-36: LGTM!Also applies to: 54-56
app/containers/MessageComposer/components/Quotes/Quotes.tsx (1)
5-11: LGTM!app/containers/message/Context.ts (1)
2-39: LGTM!app/containers/message/interfaces.ts (1)
16-20: LGTM!Also applies to: 30-66, 95-98
app/containers/message/Blocks.ts (1)
1-11: LGTM!app/containers/message/CallButton.tsx (1)
1-1: LGTM!Also applies to: 11-21
app/containers/message/RepliedThread.tsx (1)
1-1: LGTM!Also applies to: 11-17
app/lib/database/model/ThreadMessage.js (1)
21-81: 🗄️ Data Integrity & IntegrationCheck WatermelonDB version compatibility
@json(..., { memo: true })needs@nozbe/watermelondb0.28.1+; if this repo pins an older release, keep the two-argument form.app/containers/message/MessageAvatar.tsx (1)
19-45: LGTM!app/containers/message/Reactions.tsx (1)
52-100: LGTM!app/containers/message/Components/Attachments/Attachments.tsx (1)
17-91: LGTM!app/lib/database/model/Room.js (1)
11-29: LGTM!app/lib/database/model/Setting.js (1)
17-17: LGTM!app/lib/database/model/Thread.js (1)
21-77: 🗄️ Data Integrity & Integration
Thread.asPlain()should not expose memoized JSON fields by reference. If any caller mutates the returned arrays/objects, it will mutate the cached model snapshot in place; clone or freeze these fields before returning them.app/containers/message/Components/Attachments/Quote.tsx (1)
13-27: LGTM!app/containers/message/Content.tsx (1)
21-21: LGTM!Also applies to: 35-46, 78-82
app/containers/message/User.tsx (1)
56-57: 🎯 Functional CorrectnessNormalize
tsbefore passing it toMessageTime.Widening
tstoDate | stringis fine, but the cast on Line 122 does not convert the runtime value. If the new string path is exercised,MessageTimestill receives a string here. Please either normalize toDateat this boundary or verify thatMessageTimenow accepts strings end-to-end.Also applies to: 122-122
app/containers/message/Thread.tsx (1)
42-44: 📐 Maintainability & Code QualityPlease type this callback at the boundary instead of casting to
Function.
toggleFollowThread as Functionremoves the call signature exactly where this refactor is trying to tighten contracts. IfThreadDetailsandMessageContextdrifted during the migration, this cast will hide it until runtime. As per coding guidelines, "Use TypeScript for type safety; add explicit type annotations to function parameters and return types."Source: Coding guidelines
app/lib/database/model/servers/Server.js (1)
38-38: LGTM!app/lib/database/model/servers/User.js (1)
23-23: LGTM!app/containers/message/hooks/useMessage.test.ts (1)
1-127: LGTM!app/containers/message/index.test.tsx (1)
1-127: LGTM!app/containers/MessageComposer/MessageComposer.test.tsx (1)
15-16: LGTM!Also applies to: 119-139, 650-653, 685-687, 719-726, 735-737
app/containers/MessageComposer/hooks/useChooseMedia.test.tsx (1)
21-25: LGTM!Also applies to: 53-55, 80-81
app/containers/message/Reactions.test.tsx (1)
25-26: LGTM!Also applies to: 40-43
app/containers/message/Message.stories.tsx (1)
5-6: LGTM!Also applies to: 67-98, 115-128, 300-300, 331-331, 2247-2265
app/containers/message/hooks/useMessage.ts (1)
109-114: 🩺 Stability & AvailabilityInvestigate stale subscription callbacks after
itemchanges. The callback closes over the subscribeditem, so if the old subscription fires after a model swap it can overwritesnapshotRefwith stale data.app/lib/database/model/Message.js (1)
21-87: 🗄️ Data Integrity & IntegrationProtect
asPlain()from exposing memoized JSON objects. If any consumer mutates these fields in place,memo: truewill make those changes stick to the model cache and leak across later snapshots/renders. Clone or freeze the JSON-backed fields before returning them, or keep them immutable end to end.
| await new Promise(resolve => setTimeout(resolve, 100)); | ||
| this.messageComposerRef.current?.setInput(text); | ||
| this.setState({ selectedMessages }); | ||
| this.interactionStore.getState().actions.setQuotes(selectedMessages); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use interactionStore as the quote source of truth.
The UI now reads quote state from interactionStore, but send()/unmount still read this.state.action and this.state.selectedMessages. If any child updates the store directly, ShareView can send or restore stale quote data.
Proposed direction
- this.finishShareView(text, this.state.selectedMessages);
+ this.finishShareView(text, this.interactionStore.getState().selectedMessages);
...
- const { attachments, room, text, thread, action, selectedMessages } = this.state;
+ const { attachments, room, text, thread } = this.state;
+ const { action, selectedMessages } = this.interactionStore.getState();
...
- const { selectedMessages } = this.state;
+ const { selectedMessages } = this.interactionStore.getState();Also applies to: 385-390
🤖 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/views/ShareView/index.tsx` at line 243, ShareView is still using local
state as the quote source in send() and unmount, which can drift from the
interactionStore updates made through setQuotes(selectedMessages). Update
ShareView to read the current quote/action data from interactionStore everywhere
it needs quote state, and keep the local state in sync only if necessary. Use
the existing ShareView methods and fields around send(), componentWillUnmount,
this.state.action, and this.state.selectedMessages to replace stale reads with
store-backed values.
|
iOS Build Available Rocket.Chat 4.74.0.109241 |
|
Android Build Available Rocket.Chat 4.74.0.109240 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNRPYLAC5IzIh8x8rNLkdVJFHFLd-DfMV2u8Eq2Op-URdHLF11z7cS6exnF9GPdQJlr0Li1NHkUUqJ53neVB |
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Automated max-effort review — feat: modernize message rendering with reactive hooks
8 findings below as inline comments, ranked by severity.
- High — the React Compiler silently bails on
MessageContaineranduseMessage(ref writes during render), so the'use memo'directives there are no-ops. The newMessageContextvalue/handlers are recreated every render and the WatermelonDB observer re-subscribes every render, which undercuts the per-field memoization this PR is built around. Verified against the emitted babel output. - Medium —
areEqualcompares a non-existentpreviousItem?._id(dead comparison → stale grouping);onLinkPressmis-routes plain links whenattachmentsis undefined. - Low —
appendQuotelacks dedup;useRef(debounce(...))re-allocates each render;getMessageTranslationbypasses the snapshot; ShareView keepsselectedMessagesin two stores;RepliedThread's mount-only effect.
Findings #2 and #3 are pre-existing (carried from the class component) but re-exposed in the rewritten code. Posted as comments, not change requests.
| actions: { | ||
| setEditing: messageId => set({ action: 'edit', selectedMessages: [messageId] }), | ||
| initQuote: messageId => set({ action: 'quote', selectedMessages: [messageId] }), | ||
| appendQuote: messageId => set(state => ({ selectedMessages: [...state.selectedMessages, messageId] })), |
There was a problem hiding this comment.
[Low] appendQuote can insert duplicate ids.
[...state.selectedMessages, messageId] appends without a membership check. If the same message's quote action fires twice (double-tap, or re-selecting an already-selected message), its id appears twice in selectedMessages → prepareQuoteMessage emits a duplicate quote block on send and a duplicate chip renders.
Fix: no-op when already present, e.g. set(state => state.selectedMessages.includes(messageId) ? state : { selectedMessages: [...state.selectedMessages, messageId] }).
| @@ -382,39 +386,40 @@ class ShareView extends Component<IShareViewProps, IShareViewState> { | |||
| const { selectedMessages } = this.state; | |||
There was a problem hiding this comment.
[Low] selectedMessages is held in two places that must be hand-synced.
send() reads this.state.selectedMessages (L252/271) while the composer UI (quote chips) reads the Zustand store via useSelectedMessages(). Every mutation must update both (startShareView L242-243, onRemoveQuoteMessage here L386-389, and the send reset). It is paired correctly today, but it is a fragile invariant: a future edit that updates one and not the other silently diverges what is sent from what the user sees.
Fix: pick one source of truth — read selectedMessages from the store inside send() and drop the state field.
| 'use memo'; | ||
|
|
||
| const { colors } = useTheme(); | ||
| const { fetchThreadName } = useContext(MessageContext); | ||
| const [msg, setMsg] = useState(isEncrypted ? I18n.t('Encrypted_message') : tmsg); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
[Low] Mount-only effect (empty dependency array, }, []); at L24) closes over the first render's values.
useEffect(() => { if (!msg) fetch(); }, []) runs once and captures the initial tmid, id, fetchThreadName, and msg. If the row's thread metadata changes after mount without remounting (e.g. an encrypted message decrypts and tmsg becomes available), the name is never refetched and the row shows a stale/empty thread name. Low frequency because rows are keyed by message id, but the empty dep array drops values the effect reads.
Fix: include the real dependencies (tmid, id, msg/tmsg) or guard the fetch on the values that should retrigger it.
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Automated two-axis review — /review (Standards + Spec)
Posted as COMMENT (the PR author can't request changes on their own PR). Fixed point origin/develop @ 3549de64b; 65 files, +1613/-886.
🔴 One merge-blocker — see the inline thread on app/containers/message/hooks/useMessage.ts. useMessage calls experimentalSubscribe unconditionally, dropping the old container's if (item && item.experimentalSubscribe) guard. Files / Mentions / Starred / Pinned (MessagesView, REST) and non-encrypted Search (SearchMessagesView, REST) render the row with plain objects → TypeError on mount. This matches the deterministic Android E2E shard-11 failures (message-content-30, starred-messages-view, 3/3 retries). Those two views are unchanged by this PR, so they carry no inline anchor — root cause + fix live on the useMessage.ts line 111 thread.
Standards axis: 3 hard (no-noise comments ×2, 'use memo' on a non-component hook), 3 judgement (Duplicated Code, Middle Man, Speculative Generality).
Spec axis: 1 critical (above); NATIVE-1349 structural AC gaps (no separate compiler-memoized provider component; RoomView/context.ts still room: any, not split into individual fields); NATIVE-1348 mock not at the DB-service boundary; NATIVE-1351 snapshot testID="...-undefined"; scope creep (data fields in MessageContext).
The two axes are reported separately by design and not reranked against each other.
| id?: string; | ||
| rid?: string; | ||
| user?: { id?: string; username?: string; token?: string }; | ||
| baseUrl?: string; |
There was a problem hiding this comment.
Spec axis (b) — scope creep. NATIVE-1350 authorizes moving handlers to MessageContext ("Interaction handlers reach leaves through MessageContext ONLY; handler fields removed from the data interfaces"). Carrying data fields here — baseUrl, user, replies, isEncrypted, e2e, translateLanguage, threadBadgeColor, id, rid — exceeds what any subtask authorizes. Confirm intended, or split data vs handlers.
| e2e: undefined, | ||
| md: undefined, | ||
| comment: undefined, | ||
| experimentalSubscribe: jest.fn(() => jest.fn()), |
There was a problem hiding this comment.
Spec axis (a) — NATIVE-1348 partial. Spec: "model mocked at the DB-service boundary (mock active database + model subscription + snapshot; NO in-memory adapter)." Here experimentalSubscribe is stubbed inline on a POJO — no database.active / repository-level mock. Functional coverage is present, but not at the boundary the AC names.
| text={announcement} | ||
| bannerClosed={bannerClosed} | ||
| closeBanner={this.closeBanner} | ||
| <InteractionStoreContext.Provider value={this.interactionStore}> |
There was a problem hiding this comment.
Spec axis (a) — NATIVE-1349 not met as written. Spec: "Function provider component between the Room view class and the Message list, compiler-memoized, single place where shared context values are built (stable references across Room re-renders)." Here InteractionStoreContext.Provider is inlined in the class render(); the InteractionProvider in InteractionStore.tsx exists but is unused. Also RoomView/context.ts still carries room: any rather than the individual Room-derived fields the AC enumerates. The observable "re-render only the edited row" behavior may still hold via useIsBeingEdited, but the structural AC is unmet — verify and decide fix-now vs follow-up.
| "opacity": 1, | ||
| } | ||
| } | ||
| testID="message-editing-undefined" |
There was a problem hiding this comment.
Spec axis (a) — NATIVE-1351. Spec: "no unintended visual change." The regenerated snapshot adds testID="message-editing-undefined" to the Editing / EditingLargeFont stories — the undefined id means the story context lacks a real message id. Minor, but an unintended diff the AC says shouldn't appear.
Collect quote attachments from Promise.all and assign message.attachments once after it resolves, instead of racing read-modify-write across the concurrent decrypt branches (which dropped quotes on multi-quote messages).
Dead export with zero callers; consumers use the narrower hooks or read getState().actions directly.
…reads - Guard the model subscription so plain REST objects (search, starred, pinned, mentions, files) no longer throw 'experimentalSubscribe is not a function' on mount; serve a static snapshot when the model is not observable - Remove render-body ref writes from MessageContainer and useMessage so the React Compiler memoizes both (stable subscribe/getSnapshot, snapshot cached by item identity; debounced press handler created once, latest closure via effect) - areEqual compares previousItem.id (was ._id, always undefined on WatermelonDB models) and documents why item is intentionally omitted - isMessageLink no longer treats plain links as message links when attachments is undefined - read the translation from the useMessage snapshot instead of the live model - drop redundant/historical comments Adds a useMessage regression test for the non-observable plain-object path.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
android_build
GitHub Actions
Error
| // useSyncExternalStore resubscribe on item change without any render-body | ||
| // ref writes. | ||
|
|
||
| const getSnapshot = useCallback((): IMessageSnapshot => { |
There was a problem hiding this comment.
Unnecessary if we use use memo
| // ref writes. | ||
|
|
||
| const getSnapshot = useCallback((): IMessageSnapshot => { | ||
| if (!cacheRef.current || cacheRef.current.item !== item) { |
There was a problem hiding this comment.
Are we sure this comparison works?
| [item] | ||
| ); | ||
|
|
||
| return useSyncExternalStore(subscribe, getSnapshot, getSnapshot); |
|
|
||
| type SnapshotCache = { item: TAnyMessageModel; snapshot: IMessageSnapshot }; | ||
|
|
||
| export const useMessage = (item: TAnyMessageModel): IMessageSnapshot => { |
There was a problem hiding this comment.
How could we achieve this without reinventing the wheel? We could use something from wmdb, no?
… store (NATIVE-1376) The provider already opts into the React Compiler via 'use memo', so the hand-rolled useCallback on revealIgnored is redundant; inline it and let the compiler memoize. Replace the two stale `@ts-ignore TODO` comments with `@ts-expect-error` carrying the real reason (IMessage types ts as Date, IMessageFromServer as string; the date op is valid at runtime). @ts-expect-error also fails if the underlying type mismatch ever goes away, so the suppression cannot rot.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Remove the useContentData store fallback from Quote and Attachments and pass the message's attachments explicitly from the Message.tsx callers, which already subscribe to the content bag. This drops the ?? [] laundering in Reply, where an undefined nested-attachment prop used to fall through to the current message's store attachments and render the wrong quote.
The store factory left ignoredSeed possibly undefined, failing the tsc gate against the required boolean in MessageStoreState.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
| <Pinned testID={`${msg}-pinned`} /> | ||
| <Encrypted /> | ||
| <Edited testID={`${msg}-edited`} /> |
There was a problem hiding this comment.
Both component could get msg on leaves, making it possible to remove the prop
| @@ -25,17 +25,24 @@ import { | |||
|
|
|||
| type MessageStoreState = { | |||
| tick: number; | |||
There was a problem hiding this comment.
Why do we need this? Can't we do without it?
| user={user} | ||
| baseUrl={baseUrl} | ||
| getCustomEmoji={getCustomEmoji} | ||
| rid={message.rid} | ||
| timeFormat={Message_TimeFormat}> |
There was a problem hiding this comment.
Props that come from settings shouldn't be props.
| } | ||
|
|
||
| const MessageTime = ({ timeFormat, style }: IMessageTime) => { | ||
| const MessageTime = ({ style }: IMessageTime) => { |
There was a problem hiding this comment.
Do we really need style here?
There was a problem hiding this comment.
We should write tests for each component.
We place all tests inside tests folder.
There was a problem hiding this comment.
We should write stories for all components and move them to stories folder
The MessageContext-to-store refactor moved the jumpToMessage handler into MessageRoomProvider but did not re-seed it in SearchMessagesView, so tapping an in-message permalink in search results fell through to openLink (browser) instead of jumping to the referenced message. Seed jumpToMessage with a handler that parses the message id from the permalink and jumps within the searched room, mirroring RoomView.jumpToMessageByUrl.
TMessageProps carried isBeingEdited and small, but neither was ever read: MessageTouchable computes its own isBeingEdited from the interaction store, and small was only ever the hardcoded <MessageAvatar small /> literal. Drop both from the type and stop forwarding them, and the equally-unread highlighted, down into Message and MessageInner.
The mirroring effect's comment claimed unchanged slices would not trigger a notify; zustand setState notifies every listener on each render. Re-renders are avoided at the selector level: stable handler and constant refs keep each snapshot Object.is-equal. Fix the comment to describe the real mechanism.
…mirroring Add characterization tests for store behavior introduced by the message-hooks refactor that previously had no coverage: - useMessageIgnored/revealIgnored: the isIgnored seed, and that reveal is sticky because manualUnignored is never reset. - InteractionStore.useIsBeingEdited falls back to a module store (returns false) outside a provider; the action hooks throw without one. - MessageRoomProvider mirrors updated props into its store post-mount, so a regression in the mirroring effect dependencies would be caught.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
…s and stories folders (NATIVE-1393) Move all presentational leaves into components/, the two zustand stores into stores/, and tests and stories into dedicated __tests__/ and components/stories/ folders. Rename the capital-cased Components/ to lowercase components/. Update all relative imports and the external importers of deep message paths. Moves only: snapshot content is byte-identical and the public entry app/containers/message is unchanged.
RightIcons: drop the msg prop. Pinned and Edited now self-read the message text via useMessageText to build their own testIDs, which also fixes a latent bug where the header path rendered undefined-pinned / undefined-edited testIDs (User rendered RightIcons without msg). Time: drop the dead ts and style props (ts is read from the store; no caller ever passed style). Preview time format: MessageRoomProvider defaults timeFormat from the Message_TimeFormat setting when the seed is absent, so RoomView and Preview stop passing it. MessagesView and SearchMessagesView keep their explicit full date-time override. Test suites that mount MessageRoomProvider directly are wrapped in a redux Provider since the provider now reads the setting.
The per-message store kept an ignoredSeed field mirrored from the isIgnored prop every render. The seed never changes at runtime, so move it into the message context value and keep only the mutable manualUnignored latch in the store. useMessageIgnored now reads the seed from context and the latch from the store. Behavior is identical.
Add a Storybook story per presentational leaf in the message module (12 top-level + 6 RightIcons), each seeding the minimal message/room store state to render a meaningful, deterministic branch. An aggregator test renders every story to a committed snapshot so each leaf gains a regression oracle. Hidden/own variants cover the null branches.
Cover the branching hooks and component the presentational stories do not: useFile (persisted vs forwarded-file state), useMediaAutoDownload (file-type resolution, cache/auto-download/download and onPress states), and Attachments/Reply (null gate, file-download vs open-link onPress, disabled handling, and sub-part conditionals).
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Replace Function/any with concrete signatures, drop @ts-ignore/as Function casts now that underlying types align, and use indexed-access types for TInfoMessage.
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
requested a deployment
to
android_build
GitHub Actions
Waiting
| const { user, baseUrl, customEmojis } = useAppSelector(state => ({ | ||
| user: getUserSelector(state), | ||
| baseUrl: state.server.server, | ||
| customEmojis: state.customEmojis | ||
| })); |
There was a problem hiding this comment.
Message should be able to read these values on leaves
| // refs, so each selector's snapshot stays Object.is-equal and no consumer re-renders. | ||
| useEffect(() => { | ||
| store.setState(resolvedState); | ||
| }); |
There was a problem hiding this comment.
No empty arrays on useEffects. We should even add a lint rule.
| // The room-scoped fields this store owns. Tests and stories use pickMessageRoomState to | ||
| // derive MessageRoomProvider props from a single fixture object. Add a field to | ||
| // MessageRoomState → add its key here. | ||
| const ROOM_STATE_KEYS: (keyof MessageRoomState)[] = [ |
There was a problem hiding this comment.
Since we're using zustand, we shouldn't have to have this anymore
| if (!ctx) { | ||
| throw new Error('Message hooks must be used within a MessageProvider'); | ||
| } | ||
| return ctx; |
There was a problem hiding this comment.
We should make it more similar to useMessageRoomStore.
| const { baseUrl, user } = useContext(MessageContext); | ||
| const baseUrl = useBaseUrl(); | ||
| const user = useMessageUser(); | ||
| const API_Embed = useAppSelector(state => state.settings.API_Embed); |
| return `${baseUrl}/${_imageUrl}?rc_uid=${user?.id ?? ''}&rc_token=${user?.token ?? ''}`; | ||
| }, [url.image, url.url, baseUrl, user?.id, user?.token]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Evaluate how we can improve this. Even if on a new scope.
| import { type IRoomInfoParam } from '../../../views/SearchMessagesView'; | ||
| import { useSetting } from '../../../lib/hooks/useSetting'; | ||
|
|
||
| export type MessageRoomState = { |
There was a problem hiding this comment.
Most of this is invalid now, no?
| export const useOnPressRaw = (): MessageStoreState['onPress'] => { | ||
| const { store } = useMessageCtx(); | ||
| return useStore(store, s => s.onPress); | ||
| }; | ||
| export const useOnLongPressRaw = (): MessageStoreState['onLongPress'] => { |
| }; | ||
| }; | ||
|
|
||
| export const useMessagePress = (): (() => void) => { |
There was a problem hiding this comment.
Ref logic is weird. We should use useDebounce instead and remove the effect.
|
iOS Build Available Rocket.Chat 4.75.0.109291 |
1 participant
Proposed changes
Modernizes the message rendering layer and folds in the two prerequisite changes it is built on.
Message rendering
useMessagehook — which snapshotted the whole message record and prop-drilled every field down the subtree — with a per-message store. The store is created once per message and ticked by the WatermelonDB record's subscription; each leaf reads only the fields it needs through granular selector hooks. A change to one field now re-renders just the consumers of that field instead of the entire message.Object.isfor a single field, shallow equality for a domain group). This relies on the model's@json(..., { memo: true })decorators, which return a stable reference when the underlying column is unchanged.MessageContextinstead of being threaded down as individual props.'use memo'directive for the React Compiler (which runs in annotation mode in this app).reactionsModalVisible,replyWithMention,isOnHold); on-hold re-renders are already covered by theroomUpdatediff inshouldComponentUpdate.Prerequisites folded in
@jsonmodel fields so repeated reads return a stable reference. The per-field selector bailout depends on this. Previously opened as perf: memoize @json model fields #7451.CONTEXT.mdand adds message-state vocabulary. Previously opened as docs: rename ubiquitous language glossary to CONTEXT and add message state vocabulary #7450.Pull requests #7450 and #7451 are closed in favor of this one, which carries their commits.
Issue(s)
How to test or reproduce
This is a behavior-preserving refactor — the goal is that nothing changes for the user.
Verified on an Android emulator across public channels, a DM, and E2EE rooms with no red box, crash, blank rows, or visual regression.
TZ=UTC pnpm testpasses andpnpm lintreports 0 errors; message-container story snapshots are unchanged, so the presentational output is byte-identical.Screenshots
No visual change — behavior-preserving refactor.
Types of changes
Checklist
Further comments
The refactor is staged so each step is independently reviewable: memoize the
@jsonmodel fields, introduce the per-message store and selector hooks, migrate the container to a function component, relocate interaction state to a per-Room store, narrow the props contract to a context, annotate for the React Compiler, then delete the now-dead state. Story-snapshot tests render through the real message container, so an unchanged snapshot is the behavior oracle for the whole refactor.Summary by CodeRabbit