🐛 Fixed Twitter not rendering in Emails #22015
Conversation
WalkthroughThe pull request introduces a new approach to handling Twitter and X.com URL embeds in the Ghost project. It replaces the existing Twitter OEmbed provider with a more robust Changes
Sequence DiagramsequenceDiagram
participant Client
participant XEmbedProvider
participant RettiwtAPI
participant TwitterAPI
Client->>XEmbedProvider: Request oEmbed for Twitter/X URL
XEmbedProvider->>XEmbedProvider: Validate URL
XEmbedProvider->>XEmbedProvider: Extract Tweet ID
XEmbedProvider->>RettiwtAPI: Fetch Tweet Entity
RettiwtAPI->>TwitterAPI: Retrieve Tweet Details
TwitterAPI-->>RettiwtAPI: Return Tweet Data
RettiwtAPI-->>XEmbedProvider: Return Tweet Entity
XEmbedProvider->>XEmbedProvider: Map Tweet to oEmbed Format
XEmbedProvider-->>Client: Return oEmbed Data
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (14)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ronaldlangeveld
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
ghost/core/core/server/services/oembed/XEmbedProvider.js (1)
119-121: Moverequirestatement to the top-level scopeRequiring modules inside methods can lead to unnecessary overhead and less readable code. It is best practice to require modules at the top of the file.
Apply this diff:
+const {extract} = require('@extractus/oembed-extractor'); ... - const {extract} = require('@extractus/oembed-extractor');ghost/core/core/server/services/oembed/service.js (2)
16-22: Remove commented-out code to maintain code clarityCommented-out code can clutter the codebase and reduce readability. If the old
TwitterOEmbedProvideris no longer needed, consider removing it entirely.Apply this diff:
-// const Twitter = require('./TwitterOEmbedProvider'); -// const twitter = new Twitter({ -// config: { -// bearerToken: config.get('twitter').privateReadOnlyToken -// } -// });
25-28: Rename variable for consistency and clarityThe variable
XEmbedProvider. To avoid confusion, consider renaming it toxEmbedProviderorxProvider.Apply this diff:
-const twitter = new X({ +const xEmbedProvider = new X({ _fetchTweetEntity: async (tweetId) => { const response = await rettiwt.tweet.details(tweetId); return response; } }); ... -oembed.registerProvider(twitter); +oembed.registerProvider(xEmbedProvider);ghost/core/test/unit/server/services/oembed/rettiwtEmbed.test.js (1)
152-158: Remove unnecessary mock for Twitter APISince the
XEmbedProviderusesrettiwt-apiinstead of the Twitter API, mockinghttps://api.twitter.comis unnecessary. Removing this mock simplifies the test and avoids confusion.Apply this diff:
- nock('https://api.twitter.com') - .get('/2/tweets/1630581157568839683') - .query(true) - .reply(200, { - data: {id: '1630581157568839683'} - });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
ghost/core/core/server/services/oembed/TwitterOEmbedProvider.js(0 hunks)ghost/core/core/server/services/oembed/XEmbedProvider.js(1 hunks)ghost/core/core/server/services/oembed/service.js(2 hunks)ghost/core/package.json(1 hunks)ghost/core/test/unit/server/services/oembed/rettiwtEmbed.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- ghost/core/core/server/services/oembed/TwitterOEmbedProvider.js
🔇 Additional comments (1)
ghost/core/package.json (1)
218-218: Verify the security and maintenance status of rettiwt-api.Since this package is critical for email rendering and handles Twitter data access, we should verify:
- Package maintenance status
- Known vulnerabilities
- Recent security issues
✅ Verification successful
Package rettiwt-api verified as safe and well-maintained
The package shows active maintenance with recent updates (Jan 16, 2025), has significant community adoption (513 stars), and is used safely in a limited read-only context for tweet embedding. No security concerns were found in the implementation or reported issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the security and maintenance status of rettiwt-api # Check npm package details echo "Package Info:" npm view rettiwt-api time.modified maintainers homepage # Check for known vulnerabilities echo -e "\nVulnerability Check:" npm audit rettiwt-api # Check GitHub activity (if available) echo -e "\nGitHub Activity:" gh api -X GET search/repositories?q=rettiwt-api+in:name \ --jq '.items[] | {full_name, updated_at, open_issues_count, stargazers_count}'Length of output: 1363
Script:
#!/bin/bash # Check open issues and dependencies echo "Open Issues (focusing on security):" gh api -X GET repos/Rishikant181/Rettiwt-API/issues \ --jq '.[] | select(.title | test("security|vulnerability|exploit"; "i")) | {title, html_url}' echo -e "\nPackage Dependencies:" curl -s https://raw.githubusercontent.com/Rishikant181/Rettiwt-API/main/package.json | jq '.dependencies' echo -e "\nCodebase Usage:" rg -B 2 -A 2 "require.*rettiwt-api|from.*rettiwt-api" --type jsLength of output: 1029
Script:
#!/bin/bash # Check implementation details echo "OEmbed Service Implementation:" cat ghost/core/core/server/services/oembed/service.js echo -e "\nRecent Commits:" gh api -X GET repos/Rishikant181/Rettiwt-API/commits \ --jq 'first(.[]) | {message: .commit.message, date: .commit.author.date}'Length of output: 1445
| async canSupportRequest(url) { | ||
| return (url.host === 'twitter.com' || url.host === 'x.com') && TWITTER_PATH_REGEX.test(url.pathname); | ||
| } |
There was a problem hiding this comment.
Use url.hostname instead of url.host for reliable host comparison
In the canSupportRequest method, using url.host may cause host comparison failures when the URL includes a port number (e.g., twitter.com:80). The url.hostname property excludes the port number and should be used instead for accurate host comparisons.
Apply this diff to fix the issue:
- return (url.host === 'twitter.com' || url.host === 'x.com') && TWITTER_PATH_REGEX.test(url.pathname);
+ return (url.hostname === 'twitter.com' || url.hostname === 'x.com') && TWITTER_PATH_REGEX.test(url.pathname);📝 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.
| async canSupportRequest(url) { | |
| return (url.host === 'twitter.com' || url.host === 'x.com') && TWITTER_PATH_REGEX.test(url.pathname); | |
| } | |
| async canSupportRequest(url) { | |
| return (url.hostname === 'twitter.com' || url.hostname === 'x.com') && TWITTER_PATH_REGEX.test(url.pathname); | |
| } |
There was a problem hiding this comment.
We don't expect a Twitter / X url to ever include a port number.
There was a problem hiding this comment.
@ronaldlangeveld Understood, I'll retract the suggestion since Twitter/X URLs won't contain port numbers in your use case.
✏️ Learnings added
Learnt from: ronaldlangeveld
PR: TryGhost/Ghost#22015
File: ghost/core/core/server/services/oembed/XEmbedProvider.js:26-28
Timestamp: 2025-01-20T07:04:25.902Z
Learning: In the Ghost codebase, Twitter/X URLs are not expected to include port numbers, so using either `url.host` or `url.hostname` is acceptable when parsing these URLs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const [match, tweetId] = url.pathname.match(TWITTER_PATH_REGEX); | ||
| if (!match) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Fix incorrect destructuring of regex match result
The destructuring assignment in getOEmbedData is incorrect. The match result should be obtained first, checked for validity, and then extract the tweetId. The current code may cause errors if the URL does not match the expected pattern.
Apply this diff to correct the destructuring:
- const [match, tweetId] = url.pathname.match(TWITTER_PATH_REGEX);
- if (!match) {
+ const match = url.pathname.match(TWITTER_PATH_REGEX);
+ if (!match) {
return null;
}
+ const tweetId = match[1];📝 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.
| const [match, tweetId] = url.pathname.match(TWITTER_PATH_REGEX); | |
| if (!match) { | |
| return null; | |
| } | |
| const match = url.pathname.match(TWITTER_PATH_REGEX); | |
| if (!match) { | |
| return null; | |
| } | |
| const tweetId = match[1]; |
| text: tweetEntity.fullText, | ||
| created_at: new Date(tweetEntity.createdAt).toISOString(), | ||
| entities: { | ||
| urls: urls, | ||
| hashtags: tweetEntity.entities.hashtags.map((hashtag) => { | ||
| return { | ||
| tag: hashtag | ||
| }; | ||
| }), | ||
| mentions: mentionedUsers | ||
| }, |
There was a problem hiding this comment.
Add null checks for optional properties to prevent runtime errors
In the mapTweetToTweetData method, accessing tweetEntity.entities.hashtags and tweetEntity.entities.mentionedUsers without checking if entities or their properties are defined may cause runtime errors if they are undefined or null. Consider adding optional chaining or default values to ensure robustness.
Apply this diff to add safe accessors:
- const urls = (tweetEntity.entities?.urls || []).map((url) => {
- return {
- url: url,
- display_url: url.replace(/(^\w+:|^)\/\//, '') // Remove the protocol
- };
- });
+ const urls = (tweetEntity.entities?.urls || []).map((url) => ({
+ url: url,
+ display_url: url.replace(/(^\w+:|^)\/\//, '') // Remove the protocol
+ }));
- const mentionedUsers = (tweetEntity.entities?.mentionedUsers || []).map((mention) => {
- return {
- username: mention
- };
- });
+ const mentionedUsers = (tweetEntity.entities?.mentionedUsers || []).map((mention) => ({
+ username: mention
+ }));
- hashtags: tweetEntity.entities.hashtags.map((hashtag) => {
+ hashtags: (tweetEntity.entities?.hashtags || []).map((hashtag) => {
return {
tag: hashtag
};
}),Committable suggestion skipped: line range outside the PR's diff.
| it('should fetch and map oEmbed data correctly', async function () { | ||
| const tweetURL = new URL('https://twitter.com/Ghost/status/1630581157568839683'); | ||
|
|
||
| mockDependencies._fetchTweetEntity.resolves({ | ||
| id: '12345', | ||
| tweetBy: {id: '67890', fullName: 'Ghost', userName: 'GhostOrg', profileImage: 'https://example.com/profile.jpg'}, | ||
| retweetCount: 5, | ||
| likeCount: 20, | ||
| fullText: 'This is a test tweet.', | ||
| createdAt: '2024-01-01T12:00:00Z', | ||
| entities: { | ||
| urls: ['https://example.com', 'https://example2.com'], | ||
| hashtags: ['example'], | ||
| mentions: ['anotherUser'] | ||
| }, | ||
| media: [{url: 'https://example.com/media.jpg'}], | ||
| retweetedTweet: null | ||
| }); | ||
|
|
||
| nockOembedRequest(); | ||
|
|
||
| nock('https://api.twitter.com') | ||
| .get('/2/tweets/1630581157568839683') | ||
| .query(true) | ||
| .reply(200, { | ||
| data: {id: '1630581157568839683'} | ||
| }); | ||
|
|
||
| const oembedData = await provider.getOEmbedData(tweetURL); | ||
|
|
||
| assert.equal(oembedData.type, 'twitter'); | ||
| assert.ok(oembedData.html); | ||
| assert.ok(oembedData.tweet_data); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test for error handling in getOEmbedData
Currently, there is no test case verifying how getOEmbedData handles errors when fetching oEmbed data or tweet entities fails. Consider adding a test to ensure the method handles such errors gracefully.
Do you want me to draft an additional test case to cover error handling in getOEmbedData?
There was a problem hiding this comment.
@ronaldlangeveld Here's a test case that covers error handling scenarios for the getOEmbedData method:
it('should handle errors when fetching oEmbed data', async function () {
const tweetURL = new URL('https://twitter.com/Ghost/status/1630581157568839683');
// Mock a failed oEmbed request
nock('https://publish.twitter.com')
.get('/oembed')
.query(true)
.reply(500, 'Internal Server Error');
await assert.rejects(
() => provider.getOEmbedData(tweetURL),
{
name: 'InternalServerError',
message: /Failed to fetch oEmbed data/
}
);
});
it('should handle errors when fetching tweet entity', async function () {
const tweetURL = new URL('https://twitter.com/Ghost/status/1630581157568839683');
// Mock successful oEmbed request
nock('https://publish.twitter.com')
.get('/oembed')
.query(true)
.reply(200, {
html: '<blockquote>Test embed</blockquote>'
});
// Mock failed tweet entity fetch
mockDependencies._fetchTweetEntity.rejects(new Error('Failed to fetch tweet'));
await assert.rejects(
() => provider.getOEmbedData(tweetURL),
{
message: /Failed to fetch tweet/
}
);
});These tests verify that:
- The method handles failures in the oEmbed request
- The method handles failures in tweet entity fetching
- Appropriate errors are thrown with meaningful messages
Would you like me to add any additional error scenarios?
ghost/core/package.json
Outdated
| @@ -215,6 +215,7 @@ | |||
| "node-jose": "2.2.0", | |||
| "path-match": "1.2.4", | |||
| "probe-image-size": "7.2.3", | |||
| "rettiwt-api": "^4.1.4", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use exact version instead of caret range for consistency.
The project consistently uses exact versions for dependencies. Consider changing "^4.1.4" to "4.1.4" to maintain this pattern and prevent unexpected updates.
- "rettiwt-api": "^4.1.4",
+ "rettiwt-api": "4.1.4",📝 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.
| "rettiwt-api": "^4.1.4", | |
| "rettiwt-api": "4.1.4", |
ronaldlangeveld
changed the title
ref ENG-1792
Summary by CodeRabbit
New Features
XEmbedProviderfor processing Twitter/X.com URLs.Dependencies
rettiwt-apipackage for improved Twitter integration.Improvements
Tests
XEmbedProviderclass, covering functionality and error handling.