chore: migrate sendMessage + getReadReceipts callers to REST - #40675
Merged
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
WalkthroughClient message sends migrate from Meteor SDK calls to REST endpoints. REST middleware clears matching credentials on unauthorized mutations. Read receipts gain awaited persistence and live REST/stream refresh. Legacy methods emit deprecation logs, and attachment validation tightens plain-file schema matching. ChangesREST messaging migration
Read receipt synchronization
API transition instrumentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ReadReceiptsModal
participant RESTAPI
participant RoomStream
participant QueryClient
ReadReceiptsModal->>RESTAPI: GET chat.getMessageReadReceipts
RESTAPI-->>ReadReceiptsModal: return mapped receipts
RoomStream-->>ReadReceiptsModal: messagesRead notification
ReadReceiptsModal->>QueryClient: invalidate read-receipts query
QueryClient->>RESTAPI: refetch receipts
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 (2)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40675 +/- ##
===========================================
+ Coverage 68.66% 68.72% +0.05%
===========================================
Files 4139 4151 +12
Lines 159268 159463 +195
Branches 27928 27996 +68
===========================================
+ Hits 109357 109586 +229
+ Misses 44741 44693 -48
- Partials 5170 5184 +14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
11 tasks
ggazzo
force-pushed
the
chore/ddp-sendmsg-readreceipts-fix
branch
2 times, most recently
from
May 27, 2026 15:02
274ea6b to
a8c6aa9
Compare
Member
Author
|
/jira ARCH-2156 |
This was referenced May 29, 2026
ggazzo
force-pushed
the
chore/ddp-sendmsg-readreceipts-fix
branch
4 times, most recently
from
July 17, 2026 03:34
b123aa6 to
82391f6
Compare
Member
Author
|
/jira ARCH-2166 |
Two methods that resisted the URL-swap pass in the wider DDP -> REST
migration. This PR contains the deeper refactor each needed.
sendMessage
Replace sdk.call('sendMessage', message, previewUrls) with
sdk.rest.post('/v1/chat.sendMessage', { message, previewUrls }).
In the primary send flow (apps/meteor/client/lib/chats/flows/sendMessage.ts)
the response's server-rendered { message } is fed back into
Messages.state via mapMessageFromApi, replacing the optimistic temp
record in the same tick the REST call resolves. That reproduces the
Minimongo replication the DDP method triggered — composer quote
previews unmount, attachment renderers see attachments[] and urls[],
message _updatedAt advances — all without waiting for the
room-messages stream event to arrive separately.
The seven fire-and-forget callsites do not run the optimistic
reconcile and are straight URL swaps:
- apps/meteor/client/hooks/notification/useNotification.ts
- apps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsx
- apps/meteor/app/slashcommand-asciiarts/client/{lenny,tableflip,unflip,gimme,shrug}.ts
getReadReceipts
Replace useMethod('getReadReceipts') with
useEndpoint('GET', '/v1/chat.getMessageReadReceipts') in
ReadReceiptsModal. Add rid prop and subscribe to
notify-room/<rid>/messagesRead; on event, invalidate the
['read-receipts', messageId] query so the dialog re-fetches when new
receipts land. mapReadReceiptFromApi revives the Date fields the
REST endpoint serializes as strings.
Server-side: ReadReceipt.markMessagesAsRead /
ReadReceipt.markMessageAsReadBySender /
ReadReceipt.storeThreadMessagesReadReceipts no longer fire-and-forget
the storeReadReceipts insertion — they await it. Closes the
read-after-write race the omnichannel-livechat-read-receipts e2e test
exposed: a fast REST GET could observe a partial set of receipts
because the visitor's self-receipt insert was still in flight when the
test opened the Read-Receipts dialog.
The optimistic record runs through onClientMessageReceived (which the E2EE hook subscribes to) so the ciphertext is rendered as plaintext. The post-REST replacement was bypassing that hook and storing the server-returned ciphertext directly, leaving encrypted rooms showing the raw cipher (or the lastUserMessageBody locator timing out in the e2ee-encrypted-channels e2e suite). Pipe the server response through the same hook before the state replacement. For non-encrypted rooms the hook is a no-op (shouldConvertReceivedMessages returns false), so the change only affects E2EE flows. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The post-REST state.update was replacing the optimistic record with the server's response unconditionally. When the messages stream delivered an update first (read-receipt-driven `unread: false`, async URL/quote attachments arriving via AfterSave hooks, E2EE decrypt results) the REST resolution would overwrite it with the stale snapshot the server captured before those side effects landed — breaking quote rendering in non-encrypted rooms and the 'Message viewed' status icon used by read-receipts e2e tests. Restore the original predicate from the DDP-era code: drop the optimistic `temp` flag only if no other update has touched the record. The stream (Minimongo replication) keeps doing the heavy lifting for async-derived fields. The composer's `dismissAllQuotedMessages()` call already runs from the outer `sendMessage` and doesn't depend on the state.update body, so quote preview unmount keeps working. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The composer quote preview stayed mounted when /v1/chat.sendMessage rejected (e.g. response validation under TEST_MODE) even though the message was already broadcast over the stream, leaving a duplicate blockquote. Dismiss the quoted messages at the optimistic phase — they are already baked into the outgoing message — so composer state is decoupled from the request outcome, matching the optimistic composer.clear() already done for text.
…hema The typia-generated FileAttachmentProps union includes a catch-all base branch that only requires `type: 'file'`, so it also matched image/video/audio payloads. A single media file therefore satisfied both its specific oneOf branch and the base branch, violating oneOf's "exactly one" rule and failing response validation (TEST_MODE) for any message carrying a file or quoted-file attachment — e.g. /v1/chat.sendMessage when quoting a message with an attachment. Lock the base file branch with additionalProperties:false (same approach already used for MessageAttachmentDefault) so only genuine plain-file attachments match it. Verified against the failing quote-attachment payload: invalid -> valid.
… in getReadReceipts
…ct to login DDP-routed calls cleared stored credentials on auth failure via ddpOverREST; direct sdk.rest calls (e.g. the migrated chat.sendMessage flow) bypassed it, leaving an expired session wedged instead of redirecting to login. Clear credentials globally in the REST client middleware on 401 (unauthenticated) only — never 403 (permission).
ggazzo
force-pushed
the
chore/ddp-sendmsg-readreceipts-fix
branch
from
July 27, 2026 13:26
82391f6 to
4793399
Compare
ggazzo
changed the title
Its only client caller moved to GET /v1/chat.getThreadMessages in #40998, but the method was left without a deprecation log, so external consumers get no warning before 9.0.0 removes the registration.
…t token The 401 handler cleared the stored credentials unconditionally, so a transient unauthenticated 401 during app boot logged the user straight back out. OmnichannelProvider's initializeLivechatInquiryStream calls GET /v1/livechat/config/routing (authRequired, no permission gate) while the session is still being established, and custom-sounds.list does the same; both 401 with no token attached. The wipe then dropped the session that had just been created and the router fell through to the login page, so #main-content never rendered — every omnichannel e2e spec in the shard timed out in beforeEach and took the worker down with it. Capture the stored token before the request and clear only when the token is unchanged and was present at send time. An unauthenticated boot call (no token) and a request still in flight across a re-login (different token) say nothing about the credentials currently stored; an actually expired or revoked token still matches and still logs out, which is what the original fix was for.
ggazzo
force-pushed
the
chore/ddp-sendmsg-readreceipts-fix
branch
from
July 27, 2026 14:33
4793399 to
da46336
Compare
…ntials A 401 from an arbitrary endpoint does not prove the stored token is dead. The trace from the failing omnichannel e2e shard shows the same token answering 200 on 108 requests and 401 on four bursts of GET /v1/livechat/config/routing, whose body is the auth middleware's "You must be logged in to do this." — the per-route rate limiter throttles the auth check and reports it as unauthenticated, exactly the failure mode already documented in client/startup/startup.ts. Clearing on that 401 dropped a live session, the router fell through to the login page, #main-content never rendered, and every omnichannel spec in the shard timed out in beforeEach. Route the decision through /v1/me instead: a 401 elsewhere triggers one single-flight re-check, and only a 401 from /v1/me itself clears the credentials. The expired-token redirect this was added for still works — that is the same endpoint synchronizeUserData already relies on in #40870 — while a throttled or boot-time 401 no longer costs the user their session. The wipe is irreversible: the login token lives only in localStorage, so there is nothing to resume from once it is gone.
Reverts the RestApiClient middleware back to develop. Clearing the stored credentials on any REST 401 logs out live sessions: the auth middleware answers 401 whenever (X-User-Id, hashed X-Auth-Token) has no match, which also covers a call issued before the session is established and a token that vanished server-side, and from 9.0.0 ApiClass maps a thrown `error-unauthorized` — the code this codebase uses for permission denials — to 401 as well. None of those mean the stored token is dead, and the wipe is irreversible because the token only lives in localStorage. The omnichannel e2e shards that fail on this branch and pass on develop are the symptom: an auxiliary context boots, takes a 401, loses its credentials, lands on the login page, and #main-content never renders. Gating the wipe behind a /v1/me re-check narrowed it but did not close it, because /v1/me can take the same 401. Expired sessions already redirect to login through synchronizeUserData in client/startup/startup.ts (#40870), which asks /v1/me and keeps the token-stable guard. If a gap remains outside that path it needs its own change, after the 9.0.0 error-unauthorized mapping is fixed so that a 401 means one thing again.
…t reads Reinstates the expired-session redirect for the migrated send path, narrowed to the case that actually carries the signal. session-expiration-redirect.spec.ts:87 deletes the login tokens server-side and clicks send: with sendMessage now on POST /v1/chat.sendMessage, nothing cleared the stored credentials, so the router never fell through to LoginPage. The sibling test at :49 kept passing because message search is still a DDP method and ddpOverREST clears there — the gap was only on the path this PR migrated. Clearing on every REST 401 is what broke the omnichannel e2e shard: an auxiliary context boots, OmnichannelProvider's GET livechat/config/routing answers 401 with the same "You must be logged in to do this." before the session is up, the credentials are wiped, and #main-content never renders. The message body does not separate the two cases, but the request does: ddpOverREST only ever cleared for method calls, never for background fetches. Restricting the wipe to POST/PUT/ DELETE reproduces that, so a session that dies while idle is noticed on the user's next write — the pre-migration behaviour. Keeps the token-stable guard so a request in flight across a re-login cannot evict the session it does not belong to, and leaves 403 alone.
ggazzo
force-pushed
the
chore/ddp-sendmsg-readreceipts-fix
branch
from
July 27, 2026 17:42
e02ffb7 to
e0d5425
Compare
ggazzo
marked this pull request as ready for review
July 27, 2026 18:28
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/meteor/server/api/validation/ajv.ts (1)
13-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid implementation comments in this file.
Remove the added explanatory comment block. As per coding guidelines, “Avoid code comments in the implementation.”
🤖 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 `@apps/meteor/server/api/validation/ajv.ts` around lines 13 - 20, Remove the added explanatory comment block above the plain-file attachment validation in ajv.ts, leaving the implementation behavior unchanged.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 `@apps/meteor/app/utils/client/lib/RestApiClient.ts`:
- Line 50: Update the isMutation helper to classify PATCH alongside POST, PUT,
and DELETE so PATCH requests use the same mutation credential handling,
including clearing stale credentials after a 401.
In `@apps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsx`:
- Around line 37-43: Update the invite-message request in the surrounding
game-center flow to await sdk.rest.post instead of detaching it with void, so
failures propagate to the existing try/catch error path and prevent completing
the group creation without its invitation message.
In `@apps/meteor/client/lib/chats/flows/sendMessage.ts`:
- Around line 53-56: Remove the added implementation rationale comments at
apps/meteor/client/lib/chats/flows/sendMessage.ts lines 53-56 and 114-119, and
at apps/meteor/app/utils/client/lib/RestApiClient.ts lines 41-49 and 60-62.
Leave the surrounding implementations unchanged.
In
`@apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptsModal.tsx`:
- Around line 26-40: The queryKey array in ReadReceiptsModal is recreated on
every render, causing unnecessary subscription effect cleanup and
re-registration. Memoize queryKey based on messageId with useMemo, then continue
using the memoized key in both useQuery and the subscribeToNotifyRoom effect.
---
Nitpick comments:
In `@apps/meteor/server/api/validation/ajv.ts`:
- Around line 13-20: Remove the added explanatory comment block above the
plain-file attachment validation in ajv.ts, leaving the implementation behavior
unchanged.
🪄 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 Plus
Run ID: 14a8a0e1-d78c-492f-9a1a-764c29ed2829
📒 Files selected for processing (16)
apps/meteor/app/slashcommand-asciiarts/client/gimme.tsapps/meteor/app/slashcommand-asciiarts/client/lenny.tsapps/meteor/app/slashcommand-asciiarts/client/shrug.tsapps/meteor/app/slashcommand-asciiarts/client/tableflip.tsapps/meteor/app/slashcommand-asciiarts/client/unflip.tsapps/meteor/app/utils/client/lib/RestApiClient.tsapps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsxapps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.tsxapps/meteor/client/hooks/notification/useNotification.tsapps/meteor/client/lib/chats/flows/sendMessage.tsapps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptsModal.tsxapps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.tsapps/meteor/ee/server/meteor-methods/getReadReceipts.tsapps/meteor/server/api/validation/ajv.tsapps/meteor/server/meteor-methods/messages/getThreadMessages.tsapps/meteor/server/meteor-methods/messages/sendMessage.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/slashcommand-asciiarts/client/gimme.tsapps/meteor/app/slashcommand-asciiarts/client/tableflip.tsapps/meteor/app/slashcommand-asciiarts/client/lenny.tsapps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.tsxapps/meteor/ee/server/meteor-methods/getReadReceipts.tsapps/meteor/app/utils/client/lib/RestApiClient.tsapps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsxapps/meteor/app/slashcommand-asciiarts/client/shrug.tsapps/meteor/client/hooks/notification/useNotification.tsapps/meteor/client/lib/chats/flows/sendMessage.tsapps/meteor/app/slashcommand-asciiarts/client/unflip.tsapps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptsModal.tsxapps/meteor/server/meteor-methods/messages/sendMessage.tsapps/meteor/server/meteor-methods/messages/getThreadMessages.tsapps/meteor/server/api/validation/ajv.tsapps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts
🧠 Learnings (7)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/slashcommand-asciiarts/client/gimme.tsapps/meteor/app/slashcommand-asciiarts/client/tableflip.tsapps/meteor/app/slashcommand-asciiarts/client/lenny.tsapps/meteor/ee/server/meteor-methods/getReadReceipts.tsapps/meteor/app/utils/client/lib/RestApiClient.tsapps/meteor/app/slashcommand-asciiarts/client/shrug.tsapps/meteor/client/hooks/notification/useNotification.tsapps/meteor/client/lib/chats/flows/sendMessage.tsapps/meteor/app/slashcommand-asciiarts/client/unflip.tsapps/meteor/server/meteor-methods/messages/sendMessage.tsapps/meteor/server/meteor-methods/messages/getThreadMessages.tsapps/meteor/server/api/validation/ajv.tsapps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/slashcommand-asciiarts/client/gimme.tsapps/meteor/app/slashcommand-asciiarts/client/tableflip.tsapps/meteor/app/slashcommand-asciiarts/client/lenny.tsapps/meteor/ee/server/meteor-methods/getReadReceipts.tsapps/meteor/app/utils/client/lib/RestApiClient.tsapps/meteor/app/slashcommand-asciiarts/client/shrug.tsapps/meteor/client/hooks/notification/useNotification.tsapps/meteor/client/lib/chats/flows/sendMessage.tsapps/meteor/app/slashcommand-asciiarts/client/unflip.tsapps/meteor/server/meteor-methods/messages/sendMessage.tsapps/meteor/server/meteor-methods/messages/getThreadMessages.tsapps/meteor/server/api/validation/ajv.tsapps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/slashcommand-asciiarts/client/gimme.tsapps/meteor/app/slashcommand-asciiarts/client/tableflip.tsapps/meteor/app/slashcommand-asciiarts/client/lenny.tsapps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.tsxapps/meteor/ee/server/meteor-methods/getReadReceipts.tsapps/meteor/app/utils/client/lib/RestApiClient.tsapps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsxapps/meteor/app/slashcommand-asciiarts/client/shrug.tsapps/meteor/client/hooks/notification/useNotification.tsapps/meteor/client/lib/chats/flows/sendMessage.tsapps/meteor/app/slashcommand-asciiarts/client/unflip.tsapps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptsModal.tsxapps/meteor/server/meteor-methods/messages/sendMessage.tsapps/meteor/server/meteor-methods/messages/getThreadMessages.tsapps/meteor/server/api/validation/ajv.tsapps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.tsxapps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsxapps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptsModal.tsx
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/hooks/notification/useNotification.tsapps/meteor/client/lib/chats/flows/sendMessage.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/hooks/notification/useNotification.tsapps/meteor/client/lib/chats/flows/sendMessage.ts
📚 Learning: 2026-04-14T21:10:31.855Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36292
File: apps/meteor/client/hooks/useHasValidLocationHash.ts:7-12
Timestamp: 2026-04-14T21:10:31.855Z
Learning: When reviewing files in apps/meteor/client/hooks/, do not treat JSDoc-style comments on React hooks (especially exported hooks) as a violation of any “avoid code comments in implementation” guideline. It’s acceptable to use JSDoc to document the public API of exported hooks (e.g., parameter/return types, intended usage), as long as it documents behavior/contracts rather than adding narrative implementation comments.
Applied to files:
apps/meteor/client/hooks/notification/useNotification.ts
🔇 Additional comments (13)
apps/meteor/ee/server/meteor-methods/getReadReceipts.ts (1)
9-9: LGTM!Also applies to: 41-42
apps/meteor/server/meteor-methods/messages/getThreadMessages.ts (1)
8-8: LGTM!Also applies to: 23-24
apps/meteor/server/meteor-methods/messages/sendMessage.ts (1)
16-16: LGTM!Also applies to: 140-141
apps/meteor/app/slashcommand-asciiarts/client/gimme.ts (1)
11-11: LGTM!apps/meteor/app/slashcommand-asciiarts/client/lenny.ts (1)
12-12: LGTM!apps/meteor/app/slashcommand-asciiarts/client/shrug.ts (1)
14-14: LGTM!apps/meteor/app/slashcommand-asciiarts/client/tableflip.ts (1)
14-14: LGTM!apps/meteor/app/slashcommand-asciiarts/client/unflip.ts (1)
14-14: LGTM!apps/meteor/client/hooks/notification/useNotification.ts (1)
54-60: LGTM!apps/meteor/client/lib/chats/flows/sendMessage.ts (1)
51-51: LGTM!apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (1)
52-58: LGTM!Also applies to: 83-89, 104-110
apps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.tsx (1)
27-27: LGTM!apps/meteor/server/api/validation/ajv.ts (1)
21-32: 🎯 Functional CorrectnessScope this schema patch to the actual attachment branch.
This loop only checks top-level schemas and uses a broad shape heuristic. If typia nests the union branch under
oneOf/allOf, the ambiguity remains; if unrelated schemas have the sametype: 'file'shape, they are incorrectly closed. Target the specific generated branch or traverse the union structure, and add a regression test for plain, image, video, and audio attachments.
| * — ddpOverREST only clears credentials for method calls, never for background fetches — so a | ||
| * session that dies while idle is noticed on the user's next write, exactly as before. | ||
| */ | ||
| const isMutation = (method: string): boolean => method === 'POST' || method === 'PUT' || method === 'DELETE'; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Include PATCH in mutation credential handling.
A 401 from a PATCH mutation will not clear stale credentials, unlike the other write methods.
-const isMutation = (method: string): boolean => method === 'POST' || method === 'PUT' || method === 'DELETE';
+const isMutation = (method: string): boolean => method === 'POST' || method === 'PUT' || method === 'PATCH' || method === 'DELETE';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const isMutation = (method: string): boolean => method === 'POST' || method === 'PUT' || method === 'DELETE'; | |
| const isMutation = (method: string): boolean => method === 'POST' || method === 'PUT' || method === 'PATCH' || method === 'DELETE'; |
🤖 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 `@apps/meteor/app/utils/client/lib/RestApiClient.ts` at line 50, Update the
isMutation helper to classify PATCH alongside POST, PUT, and DELETE so PATCH
requests use the same mutation credential handling, including clearing stale
credentials after a 401.
Comment on lines
+53
to
+56
| // Clear the optimistic `temp` flag only if the messages stream hasn't already | ||
| // replaced the record. Overwriting with the server response can clobber stream | ||
| // updates that arrive first — e.g. read-receipt-driven `unread: false`, async | ||
| // URL/quote attachments, or E2EE decrypt — leading to stale UI state. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the added implementation comments.
apps/meteor/client/lib/chats/flows/sendMessage.ts#L53-L56: remove the optimistic-state rationale comment.apps/meteor/client/lib/chats/flows/sendMessage.ts#L114-L119: remove the quote-dismissal rationale comment.apps/meteor/app/utils/client/lib/RestApiClient.ts#L41-L49: remove the mutation-auth rationale comment.apps/meteor/app/utils/client/lib/RestApiClient.ts#L60-L62: remove the inline credential-clearing rationale comment.
As per coding guidelines, “Avoid code comments in the implementation.”
📍 Affects 2 files
apps/meteor/client/lib/chats/flows/sendMessage.ts#L53-L56(this comment)apps/meteor/client/lib/chats/flows/sendMessage.ts#L114-L119apps/meteor/app/utils/client/lib/RestApiClient.ts#L41-L49apps/meteor/app/utils/client/lib/RestApiClient.ts#L60-L62
🤖 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 `@apps/meteor/client/lib/chats/flows/sendMessage.ts` around lines 53 - 56,
Remove the added implementation rationale comments at
apps/meteor/client/lib/chats/flows/sendMessage.ts lines 53-56 and 114-119, and
at apps/meteor/app/utils/client/lib/RestApiClient.ts lines 41-49 and 60-62.
Leave the surrounding implementations unchanged.
Source: Coding guidelines
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 16 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ead-receipts subscription Two defects both review bots flagged. The play-together message was detached with `void`, so a rejection escaped the surrounding try/catch as an unhandled rejection and the freshly created group was left without its message. It was awaited before the REST migration; await it again. ReadReceiptsModal built its query key as a fresh array literal on every render and passed it as an effect dependency, so the notify-room subscription tore down and re-registered on every render — messagesRead events landing in that window were lost. Build the key from a helper at each use and depend on messageId, matching what useThreadMessagesQuery already does.
sampaiodiego
approved these changes
Jul 28, 2026
This was referenced Aug 5, 2026
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
3 participants
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
The two DDP methods that were reverted from #40659 because their client plumbing depends on DDP-specific semantics. This PR contains the deeper refactor each needed.
sendMessagesdk.call('sendMessage', message, previewUrls)→sdk.rest.post('/v1/chat.sendMessage', { message, previewUrls }).apps/meteor/client/lib/chats/flows/sendMessage.ts) feeds the server-rendered{ message }back intoMessages.stateviamapMessageFromApi, replacing the optimistic temp record in the same tick the REST call resolves. Reproduces the Minimongo replication the DDP method triggered.apps/meteor/client/hooks/notification/useNotification.ts(desktop notification reply)apps/meteor/client/apps/gameCenter/GameCenterInvitePlayersModal.tsxgetReadReceiptsuseMethod('getReadReceipts')→useEndpoint('GET', '/v1/chat.getMessageReadReceipts')inReadReceiptsModal.ridprop and subscribes tonotify-room/<rid>/messagesRead; on event, invalidates the['read-receipts', messageId]react-query so the dialog refetches when new receipts land.mapReadReceiptFromApihelper revives theDatefields the REST endpoint serializes as strings.Server-side race fix
ReadReceipt.markMessagesAsRead,markMessageAsReadBySenderandstoreThreadMessagesReadReceiptsno longer fire-and-forget the innerstoreReadReceipts(...)call — theyawaitit. Closes the read-after-write race that theomnichannel-livechat-read-receiptse2e exposed.Test plan
Task: ARCH-2165
Task: [ARCH-2266]
Summary by CodeRabbit
Improvements
Read Receipts
Bug Fixes