feat: OEmbed async processing #38151
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: fb7b82a The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMoves OEmbed URL preview processing out of synchronous send/update flows into an asynchronous MessageService.afterSave hook, adds MessageService.parseOEmbedUrl and an Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as Server API
participant MessageSvc as MessageService
participant DB as Database
participant OEmbed as OEmbed Provider
Client->>API: POST sendMessage(message)
API->>MessageSvc: save message
MessageSvc->>DB: persist message
MessageSvc-->>Client: return success (immediate)
MessageSvc->>OEmbed: afterSave (async) -> parseOEmbedUrl
OEmbed->>OEmbed: checkIgnoredHosts & apply timeout, fetch metadata
OEmbed-->>MessageSvc: message with optional attachments/previews
MessageSvc->>DB: update message (attachments/preview)
MessageSvc->>API: notify room/message changed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✏️ Tip: You can disable this entire section by setting 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 #38151 +/- ##
===========================================
+ Coverage 70.60% 70.67% +0.06%
===========================================
Files 3133 3135 +2
Lines 108382 108617 +235
Branches 19536 19584 +48
===========================================
+ Hits 76528 76761 +233
+ Misses 29853 29851 -2
- Partials 2001 2005 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/16 20:04 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Fri, 16 Jan 2026 20:04:49 GMT |
|
@coderabbitai review. |
|
@KevLehman, I'll review the changes in this PR focusing on the OEmbed async processing implementation, the new timeout configuration, and wildcard domain support for ignored hosts. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts:
- Around line 80-97: The wildcard-to-regex conversion in isIgnoredHost creates
patterns using '.*' which can be abused for ReDoS; update the logic that builds
regexes for wildcard patterns (inside isIgnoredHost) to replace '*' with '[^.]*'
(limit matches to non-dot chars suitable for hostnames) instead of '.*', escape
input as already done, add a sanity check on pattern length/complexity before
compiling (and wrap RegExp construction in try/catch to fail safe), and
optionally reject patterns containing consecutive wildcards or extreme length to
prevent catastrophic backtracking.
🧹 Nitpick comments (5)
apps/meteor/tests/end-to-end/api/chat.ts (1)
1288-1290: Consider a polling approach for more reliable async assertions.Hardcoded
sleep(1000)can be flaky—too short under CI load or too long in fast environments. A retry/poll pattern that checks for the expected state would be more robust.That said, this matches existing patterns in the file (e.g.,
setTimeoutat lines 1190 and 1212), so it's acceptable for consistency.♻️ Optional: polling approach example
// Poll until URLs are processed or timeout const maxAttempts = 10; const delay = 200; let attempts = 0; let res; do { await sleep(delay); res = await request .get(api('chat.getMessage')) .set(credentials) .query({ msgId }); attempts++; } while ( attempts < maxAttempts && res.body.message?.urls?.[1]?.meta && Object.keys(res.body.message.urls[1].meta).length === 0 ); // Then perform assertions on resapps/meteor/server/services/messages/lib/oembed/providers.ts (1)
154-188: Consider validating parsed JSON structure.The
JSON.parseon line 178 could throw for malformed JSON (handled), but if the body is valid JSON but not an object (e.g., an array or primitive),Object.entries(metas)will still work but may produce unexpected results.This is a minor robustness concern since OEmbed responses should always be objects per spec.
Optional: Add type guard for parsed response
try { const metas = JSON.parse(data.content.body); + if (typeof metas !== 'object' || metas === null || Array.isArray(metas)) { + return cleanupOembed(data); + } Object.entries(metas).forEach(([key, value]) => {apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (1)
333-361: Dead code: attachments array is declared but never populated.The
attachmentsarray is initialized on line 333 but nothing ever pushes to it. The conditional checks on lines 348-351 and 357-360 will never execute their bodies. The inline comment on line 348 acknowledges this.Either remove the dead code or implement the intended attachment handling.
Remove dead code
- const attachments: MessageAttachment[] = []; - let changed = false; for await (const item of message.urls) { if (item.ignoreParse === true) { log.debug({ msg: 'URL ignored for OEmbed', url: item.url }); continue; } const { urlPreview, foundMeta } = await parseUrl(item.url); Object.assign(item, foundMeta ? urlPreview : {}); changed = changed || foundMeta; } - // Why we have this here if there's no code for adding anything to the attachments array? - if (attachments.length) { - await Messages.setMessageAttachments(message._id, attachments); - } - if (changed === true) { await Messages.setUrlsById(message._id, message.urls); } - return { - ...message, - ...(attachments.length && { attachments }), - }; + return message; };packages/i18n/src/locales/en.i18n.json (1)
108-110: Minor: consider tightening the label text for consistency/readability
"Embed Request default timeout (in seconds)"works, but a shorter/consistent label like"Embed Request Timeout (seconds)"(or capitalizing “Default”) may read better in the admin UI. Also, adding this key only inen.i18n.jsonis correct for this repo’s translation workflow. Based on learnings.apps/meteor/server/settings/message.ts (1)
169-172: New timeout setting looks good, consider adding validation.The setting is correctly gated by
API_Embedand the 10-second default is reasonable (down from the previous 20s library default).Consider whether a minimum value constraint would be beneficial to prevent users from accidentally setting
0or negative values, which could cause immediate timeouts or unexpected behavior.♻️ Optional: Add input hint for minimum value
await this.add('API_EmbedTimeout', 10, { type: 'int', enableQuery: { _id: 'API_Embed', value: true }, + i18nDescription: 'API_EmbedTimeout_Description', });Alternatively, validate the value in the consuming code to enforce a sensible minimum (e.g., 1 second).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
apps/meteor/app/api/server/v1/chat.tsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/app/lib/server/functions/updateMessage.tsapps/meteor/app/lib/server/lib/afterSaveMessage.tsapps/meteor/app/oembed/server/index.tsapps/meteor/server/importPackages.tsapps/meteor/server/lib/callbacks.tsapps/meteor/server/services/messages/hooks/AfterSaveOEmbed.tsapps/meteor/server/services/messages/lib/oembed/providers.tsapps/meteor/server/services/messages/service.tsapps/meteor/server/settings/message.tsapps/meteor/tests/end-to-end/api/chat.tspackages/core-services/src/types/IMessageService.tspackages/i18n/src/locales/en.i18n.json
💤 Files with no reviewable changes (3)
- apps/meteor/app/oembed/server/index.ts
- apps/meteor/server/importPackages.ts
- apps/meteor/server/lib/callbacks.ts
🧰 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/server/settings/message.tsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/server/services/messages/lib/oembed/providers.tsapps/meteor/app/api/server/v1/chat.tsapps/meteor/app/lib/server/lib/afterSaveMessage.tspackages/core-services/src/types/IMessageService.tsapps/meteor/tests/end-to-end/api/chat.tsapps/meteor/server/services/messages/hooks/AfterSaveOEmbed.tsapps/meteor/app/lib/server/functions/updateMessage.tsapps/meteor/server/services/messages/service.ts
🧠 Learnings (21)
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/server/settings/message.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/server/settings/message.tsapps/meteor/app/api/server/v1/chat.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/app/lib/server/lib/afterSaveMessage.ts
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/app/api/server/v1/chat.tsapps/meteor/app/lib/server/lib/afterSaveMessage.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/api/server/v1/chat.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
apps/meteor/app/api/server/v1/chat.tspackages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-11-16T14:23:18.933Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37496
File: packages/apps-engine/src/server/runtime/deno/ProcessMessenger.ts:5-5
Timestamp: 2025-11-16T14:23:18.933Z
Learning: In TypeScript, when a constant is used with `typeof` in a type position (e.g., `type Message = SomeType | typeof SOME_CONSTANT`), it's a type-level operation that doesn't require the runtime value. In such cases, `import type { SOME_CONSTANT }` is correct and appropriate.
Applied to files:
apps/meteor/app/api/server/v1/chat.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/lib/server/lib/afterSaveMessage.tspackages/core-services/src/types/IMessageService.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
🧬 Code graph analysis (5)
apps/meteor/server/services/messages/lib/oembed/providers.ts (1)
packages/ddp-client/src/legacy/RocketchatSDKLegacy.ts (1)
url(55-57)
apps/meteor/app/lib/server/lib/afterSaveMessage.ts (2)
apps/meteor/server/lib/callbacks.ts (1)
callbacks(218-226)packages/core-services/src/index.ts (1)
Message(174-174)
packages/core-services/src/types/IMessageService.ts (2)
packages/ddp-client/src/legacy/RocketchatSDKLegacy.ts (1)
url(55-57)packages/core-typings/src/IMessage/IMessage.ts (1)
MessageUrl(16-23)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (1)
apps/meteor/server/services/messages/lib/oembed/providers.ts (2)
beforeGetUrlContent(103-128)afterParseUrlContent(154-188)
apps/meteor/app/lib/server/functions/updateMessage.ts (1)
apps/meteor/app/lib/server/lib/afterSaveMessage.ts (1)
afterSaveMessage(8-20)
🪛 ast-grep (0.40.5)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
[warning] 93-93: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escaped.replace(/\*/g, '.*')}$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 coverage (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 coverage (3/5)
- GitHub Check: 🔨 Test API (EE) / MongoDB 8.2 coverage (1/1)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (3/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 coverage (1/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (4/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 coverage (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 coverage (5/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test API (CE) / MongoDB 8.2 (1/1)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (2/4)
- GitHub Check: 🔨 Test Federation Matrix
🔇 Additional comments (19)
apps/meteor/tests/end-to-end/api/chat.ts (1)
8-8: LGTM!Import addition is appropriate for the async wait needed below.
packages/core-services/src/types/IMessageService.ts (2)
1-1: LGTM!The
MessageUrlimport is correctly added to support the newparseOEmbedUrlmethod's return type.
50-54: LGTM!The new interface methods are well-designed:
afterSavecorrectly returnsPromise<void>for fire-and-forget async processing.parseOEmbedUrlhas a clear return type matching theMessageUrltype from core-typings.These additions properly support the async OEmbed processing objective.
apps/meteor/server/services/messages/lib/oembed/providers.ts (2)
103-128: LGTM!The
beforeGetUrlContentfunction correctly:
- Guards against null
urlObj- Looks up provider for the URL
- Returns header overrides and optionally transforms the URL to the consumer URL
Clean implementation with proper early returns.
130-152: Type cast is safe but consider documenting the intent.The cast
meta as OEmbedMetaon line 150 is safe because you're only removingoembedHtmlfrom the meta object, which is an optional property. The cast is necessary due to TypeScript's structural typing after destructuring.apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (3)
115-136: LGTM on timeout configuration.The timeout is correctly applied from settings (
API_EmbedTimeout * 1000) and the time spent is properly logged for debugging purposes. This addresses the PR objective for configurable timeouts.
315-317: LGTM!Good addition of early return when
API_Embedis disabled, preventing unnecessary processing.
363-369: LGTM!The public API surface is appropriately reduced to only expose
rocketUrlParserandparseUrl, aligning with the refactored architecture.apps/meteor/server/services/messages/service.ts (2)
252-261: LGTM!The
afterSavehook is well-designed:
- Clear documentation that this is intentionally async and callers shouldn't expect the updated message.
- Proper use of
voidfor the fire-and-forget notification.- Correctly processes OEmbed after the message is saved and acknowledged to the UI.
This aligns with the PR objective to make OEmbed processing non-blocking.
318-324: LGTM!Clean delegation to
OEmbed.parseUrlwith proper typing that matches the interface definition.apps/meteor/app/api/server/v1/chat.ts (2)
1-1: LGTM!Correct import change from
OEmbedtoMessagefrom@rocket.chat/core-services, aligning with the new service-based architecture.
917-920: LGTM!The API route correctly uses
Message.parseOEmbedUrl(url)instead of the previous directOEmbed.parseUrlcall. The behavior is preserved while properly delegating to the service layer.apps/meteor/app/lib/server/functions/sendMessage.ts (2)
14-14: LGTM! Import cleanup aligns with async OEmbed architecture.Removing
notifyOnMessageChangefrom this import is consistent with the PR's goal of decoupling OEmbed processing from synchronous message sending.
289-293: Async message-change notifications are properly implemented.The
notifyOnMessageChangecall has been correctly moved to theMessage.afterSave()method in MessageService. WhenafterSaveMessageis called on line 289 of sendMessage.ts, it triggersMessage.afterSave()which asynchronously processes OEmbed data and then emits the message-change notification to clients with the updated preview data.apps/meteor/app/lib/server/functions/updateMessage.ts (2)
10-10: LGTM!The import simplification removing
notifyOnMessageChangealigns with the new architecture where message-change notifications are handled via theafterSaveMessagepathway.
94-105: LGTM! Clean async post-update flow.The
setImmediatepattern correctly defers post-update processing, and relying solely onafterSaveMessage(which internally triggersMessage.afterSave) is consistent with the PR's goal of centralizing the post-save handling. Room notification is preserved for the last-message case.apps/meteor/app/lib/server/lib/afterSaveMessage.ts (3)
1-1: LGTM!Import of
Messagefrom core-services enables the new post-save hook pattern.
8-20: LGTM! Post-save hook integration is correct.The
afterSaveMessagefunction properly triggersMessage.afterSavewith the callback-processed message (data), and thevoidprefix correctly indicates fire-and-forget async behavior for non-blocking OEmbed processing.
22-29: Different callback execution timing between sync and async code paths.In
afterSaveMessage,Message.afterSaveis called only aftercallbacks.run()completes (awaited), whereas inafterSaveMessageAsync, it's called immediately viacallbacks.runAsync(), which schedules callbacks asynchronously viasetTimeoutwithout waiting.If callbacks modify the message object in place,
afterSaveMessageAsyncwill pass the message toMessage.afterSave()before those modifications execute, creating inconsistent behavior. Verify whether this timing difference is intentional or ifafterSaveMessageAsyncshould await callback completion before invokingMessage.afterSave().
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (1)
348-375: Dead code:attachmentsarray is never populated.The
attachmentsarray is initialized at line 348 but never receives any items. The subsequent check at lines 364-366 and the spread at line 374 will never execute their attachment-related branches. The comment at line 363 also questions this.Either remove the dead code or implement the missing attachment logic if this was intended functionality.
🧹 Option 1: Remove dead code
-const attachments: MessageAttachment[] = []; - let changed = false; for await (const item of message.urls) { // ... existing loop } -// Why we have this here if there's no code for adding anything to the attachments array? -if (attachments.length) { - await Messages.setMessageAttachments(message._id, attachments); -} if (changed === true) { await Messages.setUrlsById(message._id, message.urls); } -return { - ...message, - ...(attachments.length && { attachments }), -}; +return message;
🧹 Nitpick comments (3)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (3)
80-112: Consider additional ReDoS mitigation for wildcard patterns.The static analysis tool flagged potential ReDoS risk. While your existing guards (length limit, no
**) are helpful, patterns with many single wildcards like*.*.*.*.*.domain.comcould still degrade performance.Consider adding a limit on the number of wildcards per pattern:
♻️ Suggested enhancement
return ignoredHosts .filter((h) => h.includes('*')) .some((pattern) => { // basic sanity checks to avoid pathological patterns if (pattern.length > 255) { return false; } if (pattern.includes('**')) { return false; } + // limit wildcard count to prevent regex complexity + if ((pattern.match(/\*/g) || []).length > 3) { + return false; + } const escaped = pattern.replace(/[-/\\^$+?.()|[\]{}]/g, '\\$&');
136-166: Timing measurement captures only initial response, not full body read.The
endtimestamp is captured at line 152 before the body streaming loop (lines 156-164). For large responses or slow connections, the loggedtimeSpentwon't reflect the actual total fetch duration.♻️ Suggested fix to capture accurate total time
const start = Date.now(); const response = await fetch( url, { // ... options }, settings.get('Allow_Invalid_SelfSigned_Certs'), ); -const end = Date.now(); let totalSize = 0; const chunks = []; for await (const chunk of response.body) { totalSize += chunk.length; chunks.push(chunk as Buffer); if (totalSize > sizeLimit) { log.warn({ msg: 'OEmbed request size exceeded', url }); break; } } +const end = Date.now(); log.debug({ msg: 'Obtained response from server', length: totalSize, timeSpent: `${end - start}ms` });
236-236: Avoidas anytype assertion.Using
as anybypasses TypeScript's type checking. Consider using a proper type assertion orPartial<OEmbedMeta>if the object is incrementally populated.♻️ Suggested improvement
-const metas: OEmbedMeta = {} as any; +const metas: Partial<OEmbedMeta> = {};Then update the
afterParseUrlContentcall to cast appropriately if needed, or update the function signature to acceptPartial<OEmbedMeta>.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/meteor/app/api/server/v1/chat.tsapps/meteor/server/services/messages/hooks/AfterSaveOEmbed.tsapps/meteor/server/services/messages/service.tspackages/core-services/src/types/IMessageService.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/meteor/server/services/messages/service.ts
- apps/meteor/app/api/server/v1/chat.ts
- packages/core-services/src/types/IMessageService.ts
🧰 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/server/services/messages/hooks/AfterSaveOEmbed.ts
🧠 Learnings (1)
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
🧬 Code graph analysis (1)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (1)
apps/meteor/server/services/messages/lib/oembed/providers.ts (2)
beforeGetUrlContent(103-128)afterParseUrlContent(154-188)
🪛 ast-grep (0.40.5)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
[warning] 104-104: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(source, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (3)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (3)
1-24: LGTM!Import changes align well with the refactored OEmbed flow, switching from callback-based hooks to direct provider functions.
330-332: LGTM!Good addition of the early return guard when
API_Embedis disabled - avoids unnecessary processing.
378-384: LGTM!Clean public API surface with explicit type annotation. The simplified export aligns with the refactored OEmbed architecture.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (2)
80-109: Consider additional safeguards for the wildcard matching logic.The
isIgnoredHostimplementation has two concerns:
ReDoS mitigation (line 101): While the
validationRegexat line 93 restricts allowed characters, the generated regex using[^.]*could still cause performance issues with pathological inputs. Consider adding a hostname length check before regex matching.Mixed input to
ipRangeCheck(line 86): TheexactHostsarray may contain both IP addresses and hostnames. IfipRangeCheckreceives invalid IP notation, it may throw or return unexpected results.Suggested improvements
const isIgnoredHost = (hostname: string | null): boolean => { if (!hostname || !ignoredHosts.length) { return false; } + + // Limit hostname length to prevent pathological regex matching + if (hostname.length > 253) { + return false; + } const exactHosts = ignoredHosts.filter((h) => !h.includes('*')); if (exactHosts.includes(hostname)) { + return true; + } + + // Only pass IP-like patterns to ipRangeCheck + const ipPatterns = exactHosts.filter((h) => /^[\d./:]+$/.test(h)); + if (ipPatterns.length > 0 && ipRangeCheck(hostname, ipPatterns)) { return true; } return ignoredHosts
345-372: Dead code:attachmentsarray is never populated.The
attachmentsarray is declared at line 345 but nothing in the loop (lines 348-358) adds items to it. The conditional at lines 361-363 and the spread at line 371 will never execute meaningfully. The comment at line 360 acknowledges this oddity.Either remove the dead code or add a TODO with context if this is planned for future implementation.
Option 1: Remove dead code
- const attachments: MessageAttachment[] = []; - let changed = false; for await (const item of message.urls) { if (item.ignoreParse === true) { log.debug({ msg: 'URL ignored for OEmbed', url: item.url }); continue; } const { urlPreview, foundMeta } = await parseUrl(item.url); Object.assign(item, foundMeta ? urlPreview : {}); changed = changed || foundMeta; } - // Why we have this here if there's no code for adding anything to the attachments array? - if (attachments.length) { - await Messages.setMessageAttachments(message._id, attachments); - } - if (changed === true) { await Messages.setUrlsById(message._id, message.urls); } - return { - ...message, - ...(attachments.length && { attachments }), - }; + return message; };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
🧰 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/server/services/messages/hooks/AfterSaveOEmbed.ts
🧠 Learnings (1)
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
🧬 Code graph analysis (1)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (1)
apps/meteor/server/services/messages/lib/oembed/providers.ts (2)
beforeGetUrlContent(103-128)afterParseUrlContent(154-188)
🪛 ast-grep (0.40.5)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
[warning] 101-101: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(source, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build (coverage)
🔇 Additional comments (4)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (4)
133-163: LGTM!The timing instrumentation and configurable timeout implementation look good. The conversion from seconds to milliseconds (line 144) is correct, and the performance logging (line 163) provides useful debugging information.
221-233: LGTM!The type updates for
contentandmetasalign with the newOEmbedUrlContentandOEmbedMetatypes. The direct function call toafterParseUrlContent(lines 271-276) is cleaner than the previous callback pattern.
327-329: LGTM!Good addition of the early gate when
API_Embedis disabled. This prevents unnecessary URL processing overhead.
375-381: LGTM!The reduced export surface (only
rocketUrlParserandparseUrl) with explicit type annotation is cleaner and aligns with the module's public API requirements.
There was a problem hiding this comment.
Pull request overview
This PR refactors OEmbed URL preview processing to be asynchronous, moving it from blocking message send/update operations to happening after the message is acknowledged. The changes introduce a new configurable timeout setting and add support for wildcard domain patterns in the ignored hosts configuration.
Changes:
- OEmbed URL processing now happens asynchronously after messages are sent, preventing it from blocking message delivery
- New
API_EmbedTimeoutsetting allows configuration of request timeout (default: 10 seconds) - Wildcard domain support (e.g.,
*.domain.com) added toAPI_EmbedIgnoredHostssetting
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/i18n/src/locales/en.i18n.json | Added translation for new timeout setting |
| packages/core-services/src/types/IMessageService.ts | Added afterSave and parseOEmbedUrl methods to service interface |
| apps/meteor/tests/end-to-end/api/chat.ts | Added sleep delay to accommodate async OEmbed processing |
| apps/meteor/server/settings/message.ts | Added new API_EmbedTimeout integer setting |
| apps/meteor/server/services/messages/service.ts | Implemented afterSave method to handle async OEmbed processing and notifications |
| apps/meteor/server/services/messages/lib/oembed/providers.ts | Converted callbacks to exported functions, removed callback registration |
| apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts | Major refactor: removed callback-based hooks, added wildcard domain filtering, implemented configurable timeout, changed return type to include urlObj |
| apps/meteor/server/lib/callbacks.ts | Removed OEmbed-specific callback type definitions |
| apps/meteor/server/importPackages.ts | Removed import of deprecated oembed server package |
| apps/meteor/app/oembed/server/index.ts | Deleted file (no longer needed) |
| apps/meteor/app/lib/server/lib/afterSaveMessage.ts | Added call to Message.afterSave for async processing |
| apps/meteor/app/lib/server/functions/updateMessage.ts | Removed notifyOnMessageChange call (moved to afterSave) |
| apps/meteor/app/lib/server/functions/sendMessage.ts | Removed notifyOnMessageChange call (moved to afterSave) |
| apps/meteor/app/api/server/v1/chat.ts | Updated to use Message.parseOEmbedUrl service method |
| .changeset/gold-kids-move.md | Added changeset documenting the changes |
Comments suppressed due to low confidence (5)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts:363
- Dead code: The attachments array is initialized as empty and never populated throughout the function. This check and the database update will never execute. Consider removing lines 361-363 and simplifying the return statement on lines 369-372 to just return the message with updated urls.
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts:372 - Dead code: Since the attachments array is always empty (never populated), this conditional spread will never add attachments to the returned object. Consider simplifying this return to just return the message without the attachments condition.
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts:18 - Inconsistent library usage: This file imports camelCase from 'lodash' while the providers.ts file imports camelCase from 'change-case'. For consistency and to avoid unnecessary dependencies, both files should use the same library. The change-case library was already being used before this refactor.
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts:112 - The error message text has changed from "invalid host" to "host is ignored", which is more descriptive but may be a breaking change if any code is checking for this specific error message. Consider whether this needs to be maintained for backward compatibility or if it's an acceptable improvement.
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts:360 - This comment should be removed. It's correct that there's no code adding items to the attachments array - the array is initialized empty on line 345 and never populated. The check on line 361 and the conditional return on line 371 are both dead code since
attachments.lengthwill always be 0.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.changeset/gold-kids-move.md:
- Line 7: Update the changeset description to fix the grammar: replace the
incorrect phrase "the process is asynchronously" with a correct form such as
"the process runs asynchronously" or "the process is asynchronous" so the
sentence reads smoothly and grammatically correct in the
.changeset/gold-kids-move.md description.
🧹 Nitpick comments (3)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (3)
85-88: Consider separating IP ranges from domain names for clarity.The
exactHostsarray may contain both domain names and IP ranges/CIDR notations. WhileipRangeCheckhandles non-IP inputs gracefully (returns false), filtering IP-like entries before passing toipRangeCheckwould make the intent clearer.♻️ Optional clarification
const exactHosts = ignoredHosts.filter((h) => !h.includes('*')); -if (exactHosts.includes(hostname) || ipRangeCheck(hostname, exactHosts)) { +if (exactHosts.includes(hostname)) { + return true; +} +// IP range check only makes sense for IP-like entries +const ipRanges = exactHosts.filter((h) => /^[\d./:]+$/.test(h)); +if (ipRanges.length > 0 && ipRangeCheck(hostname, ipRanges)) { return true; }
233-233: Avoidas anytype assertion.Using
as anybypasses TypeScript's type checking. Consider using a more precise type assertion orPartial<OEmbedMeta>.♻️ Type-safe alternative
-const metas: OEmbedMeta = {} as any; +const metas: Partial<OEmbedMeta> = {};Then update the
escapeMetafunction andafterParseUrlContentcall to handle the partial type accordingly.
346-346: Unnecessaryfor await...ofon regular array.
message.urlsis a regular array, not an async iterable. While this works,for await...ofis semantically meant for async iterables. A standardfor...ofloop withawaitinside is more appropriate.♻️ Use standard for...of loop
-for await (const item of message.urls) { +for (const item of message.urls) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/gold-kids-move.mdapps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
🧰 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/server/services/messages/hooks/AfterSaveOEmbed.ts
🧠 Learnings (1)
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
🪛 ast-grep (0.40.5)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
[warning] 101-101: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(source, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
.changeset/gold-kids-move.md (1)
1-5: LGTM!The package selection and patch-level version bumps are appropriate for the scope of changes described in this PR.
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (3)
90-108: ReDoS mitigation looks good.The previous concerns about ReDoS have been addressed:
- Pattern validation with
validationRegexrestricts allowed patterns- Rejecting single
*patterns prevents matching everything- Using
[^.]*instead of.*limits matching to non-dot characters, preventing catastrophic backtracking- try/catch provides fail-safe behavior for edge cases
327-329: Good addition of early return for disabled embedding.Early return when
API_Embedis disabled avoids unnecessary processing. This aligns well with the async flow optimization goals.
365-371: LGTM on narrowed exports.Explicitly typing the exported API and limiting the public surface to
rocketUrlParserandparseUrlis good practice for maintainability.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/server/services/messages/service.ts`:
- Around line 261-267: The OEmbed.rocketUrlParser call inside afterSave can
throw and prevent notifyOnMessageChange from running; wrap the
OEmbed.rocketUrlParser(message) invocation in a try/catch inside the async
afterSave method (catch errors from OEmbed.rocketUrlParser), log or surface the
error via the existing logging mechanism, and ensure notifyOnMessageChange({ id:
message._id }) is always called (i.e., call it after the try/catch or in
finally) so the notification fires even if OEmbed processing fails.
♻️ Duplicate comments (1)
apps/meteor/server/services/messages/service.ts (1)
265-265: Typo in comment: "until" should be "ONLY".The comment should read "we'll notify ONLY after these hooks are finished" for clarity.
📝 Suggested fix
- // we'll notify until after these hooks are finished + // we'll notify ONLY after these hooks are finished
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/server/services/messages/service.ts
🧰 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/server/services/messages/service.ts
🧬 Code graph analysis (1)
apps/meteor/server/services/messages/service.ts (3)
packages/core-typings/src/IMessage/IMessage.ts (2)
IMessage(145-242)MessageUrl(16-23)apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (1)
OEmbed(364-370)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(475-481)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/server/services/messages/service.ts (2)
4-8: LGTM!The import reorganization correctly separates runtime imports (
isEditedMessage) from type-only imports, and the newOEmbedimport from the local hooks module properly supports the new async processing functionality.
325-330: LGTM!The
parseOEmbedUrlmethod correctly delegates toOEmbed.parseUrlwith a matching return type signature. This provides a clean public API for URL preview parsing.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (1)
68-131: Filter emptyAPI_EmbedSafePortsentries;split('')can unintentionally block all previews.
If the setting is empty/whitespace,split(',')yields[''], makingsafePorts.length > 0and causing the “invalid/unsafe port” path to trigger for normal URLs.Proposed fix
- const safePorts = settings.get<string>('API_EmbedSafePorts').replace(/\s/g, '').split(',') || []; + const safePorts = + settings + .get<string>('API_EmbedSafePorts') + .replace(/\s/g, '') + .split(',') + .filter(Boolean) || [];
🤖 Fix all issues with AI agents
In `@apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts`:
- Around line 117-119: Replace the generic thrown Error when a host is ignored
with a typed sentinel (e.g., class IgnoredHostError extends Error) and throw
that from isIgnoredHost/AfterSaveOEmbed branch instead of throw new Error('host
is ignored'); then update getUrlMeta's error handling to detect IgnoredHostError
and return a silent skip (null/undefined) without calling log.error so
ignored-hosts don't spam error logs; ensure other callers that may rely on
getUrlMeta also tolerate the silent skip.
♻️ Duplicate comments (1)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (1)
77-119: Wildcard ignored-host matching looks much safer now (lowercasing + validation +[^.]*+ try/catch).
This aligns with the stated semantics (“*.domain.commatches subdomains but not the base domain”) and mitigates the previous “.*” backtracking risk.Two small follow-ups:
- Consider a cheap guard like
if (pattern.length > N) return false;to cap worst-case work with admin-provided patterns.ipRangeCheck(hostname, exactHosts)may behave unexpectedly ifhostnameis a DNS name (not an IP); consider wrapping it intry/catchor checkingnet.isIP(hostname)first.
🧹 Nitpick comments (2)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (2)
133-177:timeSpentmeasurement is currently misleading; it excludes body streaming.
endis captured right afterfetch(...), before thefor await (const chunk ...)loop finishes, so logs under-report slow responses.Also, please double-check
@rocket.chat/server-fetchtimeoutsemantics (and whethertimeout=0is valid/means “no timeout”) since you’re converting seconds → ms here.Proposed fix (measure full download)
const start = Date.now(); const response = await fetch( url, { @@ timeout: settings.get<number>('API_EmbedTimeout') * 1000, @@ }, settings.get('Allow_Invalid_SelfSigned_Certs'), ); - const end = Date.now(); let totalSize = 0; const chunks = []; for await (const chunk of response.body) { @@ } + const end = Date.now(); log.debug({ msg: 'Obtained response from server', length: totalSize, timeSpent: `${end - start}ms` });
227-283: Avoid{} as anyformetas; prefer a type-safe initializer.
const metas: OEmbedMeta = {} as any;makes it easy to accidentally store non-string values and hides typing issues.If
OEmbedMetais effectively a string map, preferconst metas: OEmbedMeta = {};. Otherwise, considerconst metas: Record<string, string> = {};and cast only at the boundary where needed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
🧰 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/server/services/messages/hooks/AfterSaveOEmbed.ts
🧠 Learnings (1)
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
🧬 Code graph analysis (1)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (1)
apps/meteor/server/services/messages/lib/oembed/providers.ts (2)
beforeGetUrlContent(103-128)afterParseUrlContent(154-188)
🪛 ast-grep (0.40.5)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
[warning] 107-107: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(source, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (2)
333-336: Good:API_Embedearly return avoids unnecessary work when previews are disabled.
This matches the goal of reducing UX/perf impact.
371-377: Export surface looks clean (parser + parseUrl only).
This should help keep the oEmbed API boundary tight.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
cardoso
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
KevLehman
added
stat: ready to merge
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-1597
Steps to test or reproduce
Further comments
Closes #37118
[Note] When using wildcard domains, the base domain would not be considered part of the wildcard. So
*.domain.comwill matchxyz.domain.combut notdomain.com.To avoid issues, add both options depending on what you need to block.
Summary by CodeRabbit
New Features
Improvements
Behavioral Change
Tests
✏️ Tip: You can customize this high-level summary in your review settings.