Skip to content

fix: replace as any with Partial<OEmbedMeta> in OEmbed meta parsing#39489

Open
Agarwalchetan wants to merge 4 commits intoRocketChat:developfrom
Agarwalchetan:fix/oembed-meta-as-any-type
Open

fix: replace as any with Partial<OEmbedMeta> in OEmbed meta parsing#39489
Agarwalchetan wants to merge 4 commits intoRocketChat:developfrom
Agarwalchetan:fix/oembed-meta-as-any-type

Conversation

@Agarwalchetan
Copy link
Contributor

@Agarwalchetan Agarwalchetan commented Mar 9, 2026

What

Removes the broad as any type assertion when initializing the metas object in getUrlMeta() inside AfterSaveOEmbed.ts.

Why

The previous code silenced TypeScript for the entire metas object:

// Before — as any disables all type checking
const metas: OEmbedMeta = {} as any;

This means any typo in a key assignment or a wrong value type would go completely undetected. Since OEmbedMeta is defined as a string-indexed type, we can do better.

How

Use Partial<OEmbedMeta> for the build phase — TypeScript can now validate all dynamic string key assignments — and propagate Partial<OEmbedMeta> all the way through afterParseUrlContent and cleanupOembed so no cast is needed at any call site:

// AfterSaveOEmbed.ts — building phase is fully type-checked
const metas: Partial<OEmbedMeta> = {};
// ...populate dynamically...

// No cast needed — afterParseUrlContent accepts Partial<OEmbedMeta>
return afterParseUrlContent({ url, meta: metas, headers, content });
// providers.ts — cleanupOembed and afterParseUrlContent updated
export const afterParseUrlContent = (data: {
  url: string;
  meta: Partial<OEmbedMeta>; // was: OEmbedMeta
  ...
}): { ...meta: Partial<OEmbedMeta> } => { ... };

Why this is the correct fix: oembedUrl (declared required in OEmbedMeta) is only assigned at line 175 of providers.ts when a known provider is found — for regular URLs it is never set. Casting metas as OEmbedMeta was therefore unsound at runtime. By propagating Partial<OEmbedMeta> through the function signatures, the type system accurately models what is actually true at runtime: oembedUrl may or may not be present.

Changes

File Change
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts Line 241: OEmbedMeta = {} as anyPartial<OEmbedMeta> = {}
Line 281: meta: metas as OEmbedMetameta: metas (cast removed)
apps/meteor/server/services/messages/lib/oembed/providers.ts cleanupOembed input/output: meta: OEmbedMetameta: Partial<OEmbedMeta>
afterParseUrlContent input/output: meta: OEmbedMetameta: Partial<OEmbedMeta>
Removed meta as OEmbedMeta cast in cleanupOembed return

Testing

  • TypeScript: Ran yarn tsc -p apps/meteor/tsconfig.json --noEmit on both the baseline (before) and the fix (after). Output is identical — zero new type errors introduced.
  • ESLint: No lint Errors
  • Unit Tests: No dedicated unit tests exist for AfterSaveOEmbed.ts. The logic is covered by integration tests in the Meteor test suite which run in CI.

Summary by CodeRabbit

  • Chores
    • Refined internal metadata handling and function interfaces for message link previews and parsing.
    • Propagated more permissive metadata shapes through processing steps and updated related return shapes.
    • Adjusted how metadata is initialized and passed between processing stages.
    • No user-facing changes; behavior and UI remain unchanged.

@Agarwalchetan Agarwalchetan requested a review from a team as a code owner March 9, 2026 20:16
@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Mar 9, 2026

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Mar 9, 2026

⚠️ No Changeset found

Latest commit: 7d7e22c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 9, 2026

Walkthrough

Type-only changes: metas/meta types relaxed from OEmbedMeta to Partial<OEmbedMeta> in the AfterSaveOEmbed hook and oEmbed provider functions; provider function signatures and return types were updated to propagate Partial<OEmbedMeta>.

Changes

Cohort / File(s) Summary
Hook: After-save oEmbed
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
Local metas type changed from OEmbedMeta to Partial<OEmbedMeta>; logic now builds a partial meta object.
oEmbed providers / parsing
apps/meteor/server/services/messages/lib/oembed/providers.ts
Updated cleanupOembed and afterParseUrlContent parameter and return types to use meta: Partial<OEmbedMeta> and adjusted implementations to work with partial meta objects.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

type: chore

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and accurately describes the main change: replacing a broad as any type assertion with a more specific Partial<OEmbedMeta> type in OEmbed meta parsing.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

241-281: ⚠️ Potential issue | 🟠 Major

The cast at line 281 masks an incomplete OEmbedMeta.

Partial<OEmbedMeta> correctly reflects the local builder at line 241, but meta: metas as OEmbedMeta at line 281 reintroduces type unsoundness at the boundary. The object is missing oembedUrl (which is required by OEmbedMeta as string | string[]), and afterParseUrlContent has early-return paths via cleanupOembed at lines 165-166 and 172 that can execute before oembedUrl is assigned at line 175. This allows callers to receive a value typed as complete OEmbedMeta when it is actually missing required fields.

Either ensure oembedUrl is populated before the cast, or keep the boundary type as Partial<OEmbedMeta> until the provider pipeline guarantees all required fields.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts` around lines
241 - 281, The code casts the local Partial<OEmbedMeta> variable metas to
OEmbedMeta when calling afterParseUrlContent, which is unsafe because metas may
lack required fields like oembedUrl (set later at line 175) and early returns
(cleanupOembed) can occur beforehand; fix by either (A) ensuring metas.oembedUrl
is definitely populated before the cast (assign oembedUrl where you build metas
or before the afterParseUrlContent call) or (B) change the boundary to pass
metas as Partial<OEmbedMeta> (adjust afterParseUrlContent signature/overloads or
add a validation step that throws/returns early if required fields including
oembedUrl are missing) so callers never receive an incomplete OEmbedMeta.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts`:
- Around line 241-281: The code casts the local Partial<OEmbedMeta> variable
metas to OEmbedMeta when calling afterParseUrlContent, which is unsafe because
metas may lack required fields like oembedUrl (set later at line 175) and early
returns (cleanupOembed) can occur beforehand; fix by either (A) ensuring
metas.oembedUrl is definitely populated before the cast (assign oembedUrl where
you build metas or before the afterParseUrlContent call) or (B) change the
boundary to pass metas as Partial<OEmbedMeta> (adjust afterParseUrlContent
signature/overloads or add a validation step that throws/returns early if
required fields including oembedUrl are missing) so callers never receive an
incomplete OEmbedMeta.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8d62f896-265f-4b33-91c9-0c6332f3dd23

📥 Commits

Reviewing files that changed from the base of the PR and between 0cb6a96 and c520d87.

📒 Files selected for processing (1)
  • apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
📜 Review details
🧰 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 (3)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 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/server/services/messages/hooks/AfterSaveOEmbed.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/server/services/messages/hooks/AfterSaveOEmbed.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant