Enricobarbieri1997/og images dynamic generation #3995
Conversation
Functionality is present but styling is not final
getBaseUrl is now needed outside trpc
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
|
Someone is attempting to deploy a commit to the OsmoLabs Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/web/pages/api/og-images/assets/[denom].tsxOops! Something went wrong! :( ESLint: 8.50.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/packages/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the generation of Open Graph images for cryptocurrency tokens. A new API endpoint is created to handle requests for these images, utilizing the edge runtime. Additionally, modifications are made to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
packages/web/utils/og-images.ts (1)
5-5: Consider removingconsole.login production code.The
console.log(url);statement may not be necessary in production and could expose internal URLs in logs.If logging is not required, consider removing this line to clean up the console output.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/web/package.jsonis excluded by!**/*.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (5)
packages/web/pages/api/og-images/assets/[denom].tsx(1 hunks)packages/web/pages/assets/[denom].tsx(3 hunks)packages/web/utils/og-images.ts(1 hunks)packages/web/utils/trpc.ts(1 hunks)packages/web/utils/url.ts(1 hunks)
🔇 Additional comments (4)
packages/web/pages/api/og-images/assets/[denom].tsx (2)
71-73: Verify lineHeight smaller than fontSize may cause text clipping.
The lineHeight is set to "60px", which is less than the fontSize of "70px". This may lead to text being clipped or not rendered properly.
Please verify if this styling produces the desired visual effect or adjust the lineHeight accordingly.
92-94: Verify lineHeight smaller than fontSize may cause text clipping.
The lineHeight is set to "30px", which is less than the fontSize of "54px". This may lead to text being clipped or not rendered properly.
Please verify if this styling produces the desired visual effect or adjust the lineHeight accordingly.
packages/web/utils/url.ts (1)
1-5: LGTM!
The getBaseUrl function is correctly implemented, providing the appropriate base URL based on the environment.
packages/web/utils/trpc.ts (1)
30-30: LGTM!
Importing getBaseUrl from ~/utils/url enhances code modularity and maintainability.
| } | ||
| const token = tokenDetails; | ||
| const title = token.coinName; | ||
| const formattedTokenPrice = tokenPrice ? tokenPrice.toString(2) : ""; |
There was a problem hiding this comment.
Fix incorrect number formatting in 'formattedTokenPrice'.
The method toString(2) converts the number to a binary string. To format the price to two decimal places, use toFixed(2) instead.
Apply this diff to fix the issue:
-const formattedTokenPrice = tokenPrice ? tokenPrice.toString(2) : "";
+const formattedTokenPrice = tokenPrice ? tokenPrice.toFixed(2) : "";📝 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 formattedTokenPrice = tokenPrice ? tokenPrice.toString(2) : ""; | |
| const formattedTokenPrice = tokenPrice ? tokenPrice.toFixed(2) : ""; |
| const tokenDetails = getAsset({ | ||
| assetLists: AssetLists, | ||
| anyDenom: tokenDenom, | ||
| }); |
There was a problem hiding this comment.
Add error handling for missing token details.
If getAsset does not find a matching asset, tokenDetails might be undefined, leading to errors when accessing its properties later.
Add a check to handle the case where tokenDetails is undefined.
Apply this diff:
const tokenDetails = getAsset({
assetLists: AssetLists,
anyDenom: tokenDenom,
});
+ if (!tokenDetails) {
+ return new Response("Asset not found", { status: 404 });
+ }📝 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 tokenDetails = getAsset({ | |
| assetLists: AssetLists, | |
| anyDenom: tokenDenom, | |
| }); | |
| const tokenDetails = getAsset({ | |
| assetLists: AssetLists, | |
| anyDenom: tokenDenom, | |
| }); | |
| if (!tokenDetails) { | |
| return new Response("Asset not found", { status: 404 }); | |
| } |
| fontFamily: "Inter", | ||
| }} |
There was a problem hiding this comment.
Inconsistent fontFamily name: 'Inter' vs 'Geist'.
The fontFamily is set to "Inter", but in the fonts array, the font is named "Geist". Ensure that the fontFamily matches the name defined in the fonts array to correctly apply the font.
Either change the fontFamily to "Geist" or update the name in the fonts array to "Inter".
Apply this diff to set the fontFamily to "Geist":
style={{
- fontFamily: "Inter",
+ fontFamily: "Geist",
}}Alternatively, change the name in the fonts array to "Inter":
fonts: [
{
- name: "Geist",
+ name: "Inter",
data: await loadGoogleFont(
"Inter",
token.coinDenom.toString() + title + formattedTokenPrice
),
style: "normal",
},
],Committable suggestion skipped: line range outside the PR's diff.
| @@ -0,0 +1,19 @@ | |||
| export async function loadGoogleFont(font: string, text: string) { | |||
| const url = `https://fonts.googleapis.com/css2?family=${font}&text=${encodeURIComponent( | |||
There was a problem hiding this comment.
Potential security issue: Encode font parameter to prevent injection attacks.
The font parameter is concatenated into the URL without encoding, which could lead to injection attacks if font contains special characters.
Apply this diff to encode font using encodeURIComponent:
export async function loadGoogleFont(font: string, text: string) {
- const url = `https://fonts.googleapis.com/css2?family=${font}&text=${encodeURIComponent(
+ const url = `https://fonts.googleapis.com/css2?family=${encodeURIComponent(font)}&text=${encodeURIComponent(
text
)}`;📝 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 url = `https://fonts.googleapis.com/css2?family=${font}&text=${encodeURIComponent( | |
| const url = `https://fonts.googleapis.com/css2?family=${encodeURIComponent(font)}&text=${encodeURIComponent( |
| url: `${getBaseUrl(true)}/api/og-images/assets/${ | ||
| asset.coinDenom | ||
| }`, |
There was a problem hiding this comment.
Ensure asset.coinDenom is URL-encoded when constructing the image URL.
To handle special characters in asset.coinDenom, it's advisable to encode it when constructing the URL.
Apply this diff to encode asset.coinDenom:
{
url: `${getBaseUrl(true)}/api/og-images/assets/${
- asset.coinDenom
+ encodeURIComponent(asset.coinDenom)
}`,
alt: title,
},📝 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.
| url: `${getBaseUrl(true)}/api/og-images/assets/${ | |
| asset.coinDenom | |
| }`, | |
| url: `${getBaseUrl(true)}/api/og-images/assets/${ | |
| encodeURIComponent(asset.coinDenom) | |
| }`, |
| description: details?.description?.substring(0, 120) + "...", | ||
| images: [ |
There was a problem hiding this comment.
Prevent 'undefined' in Open Graph description when details.description is absent.
When details.description is undefined, the expression adds "..." to undefined, resulting in "undefined..." in the Open Graph description.
Modify the code to handle undefined descriptions gracefully.
Apply this diff to fix the issue:
openGraph={{
title: pageTitle,
- description: details?.description?.substring(0, 120) + "...",
+ description: details?.description
+ ? details.description.substring(0, 120) + "..."
+ : undefined,
images: [📝 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.
| description: details?.description?.substring(0, 120) + "...", | |
| images: [ | |
| description: details?.description | |
| ? details.description.substring(0, 120) + "..." | |
| : undefined, | |
| images: [ |
What is the purpose of the change:
This PR introduces open graph parameters for various pages on the site and dynamic image preview for some of them
Linear Task
https://linear.app/osmosis/issue/POL-1986/custom-thumbnail-generation-prototype
Brief Changelog