Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enricobarbieri1997/og images dynamic generation #3995

Open
wants to merge 7 commits into
base: stage
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/web/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
"@trpc/next": "^10.45.1",
"@trpc/react-query": "^10.45.1",
"@trpc/server": "^10.45.1",
"@vercel/og": "^0.6.4",
"@vercel/otel": "^1.9.1",
"@vercel/speed-insights": "^1.0.12",
"@visx/curve": "^2.17.0",
Expand Down
118 changes: 118 additions & 0 deletions packages/web/pages/api/og-images/assets/[denom].tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
import { getAsset, getAssetPrice } from "@osmosis-labs/server";
import { ImageResponse } from "next/og";

import { AssetLists } from "~/config/generated/asset-lists";
import { loadGoogleFont } from "~/utils/og-images";
import { getBaseUrl } from "~/utils/url";

// App router includes @vercel/og.
// No need to install it.

// We're using the edge-runtime
export const config = {
runtime: "edge",
};

const imageHeight = 240;
const imageAspectRatio = 1.97;
const textOsmoverse300 = "text-[#B0AADC]";
const baseUrl = getBaseUrl();

export default async function GET(request: Request) {
const { searchParams } = new URL(request.url);
const tokenDenom = searchParams.get("denom");
if (!tokenDenom) {
return new Response("Denom is required", { status: 400 });
}
const tokenDetails = getAsset({
assetLists: AssetLists,
anyDenom: tokenDenom,
});
Comment on lines +27 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
const tokenDetails = getAsset({
assetLists: AssetLists,
anyDenom: tokenDenom,
});
const tokenDetails = getAsset({
assetLists: AssetLists,
anyDenom: tokenDenom,
});
if (!tokenDetails) {
return new Response("Asset not found", { status: 404 });
}

let tokenPrice = undefined;
if (tokenDetails.coinGeckoId) {
try {
tokenPrice = await getAssetPrice({
chainList: [],
assetLists: AssetLists,
asset: {
coinDenom: tokenDetails.coinMinimalDenom,
coinGeckoId: tokenDetails.coinGeckoId,
},
currency: "usd",
});
} catch (error) {
console.error("Failed to get asset price", error);
}
}
const token = tokenDetails;
const title = token.coinName;
const formattedTokenPrice = tokenPrice ? tokenPrice.toString(2) : "";
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
const formattedTokenPrice = tokenPrice ? tokenPrice.toString(2) : "";
const formattedTokenPrice = tokenPrice ? tokenPrice.toFixed(2) : "";

return new ImageResponse(
(
<div
style={{
fontFamily: "Inter",
}}
Comment on lines +54 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

tw="flex h-screen w-screen px-6 py-8 bg-[#090524]"
>
<div tw="flex w-2/5 mr-7">
{token.coinImageUrl ? (
<img
src={`${baseUrl}${token.coinImageUrl}`}
alt={token.coinName}
tw="h-auto"
/>
) : null}
</div>
<div tw="flex flex-col justify-center w-3/5">
<div tw="flex flex-col mb-2">
<h6
style={{
fontSize: "70px",
lineHeight: "60px",
}}
tw={`m-0 mb-1 text-white`}
>
{token.coinDenom}
</h6>
{title ? (
<h6
style={{
fontSize: "24px",
}}
tw={`${textOsmoverse300} m-0 ml-3`}
>
{title}
</h6>
) : null}
</div>
{tokenPrice ? (
<h6
style={{
fontSize: "54px",
lineHeight: "30px",
}}
tw={`m-0 ml-2 text-white`}
>
${formattedTokenPrice}
</h6>
) : null}
</div>
</div>
),
{
width: imageHeight * imageAspectRatio,
height: imageHeight,
fonts: [
{
name: "Geist",
data: await loadGoogleFont(
"Inter",
token.coinDenom.toString() + title + formattedTokenPrice
),
style: "normal",
},
],
}
);
}
21 changes: 18 additions & 3 deletions packages/web/pages/assets/[denom].tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import { AssetInfoViewProvider } from "~/hooks/use-asset-info-view";
import { PreviousTrade, SwapPreviousTradeKey } from "~/pages";
import { SUPPORTED_LANGUAGES } from "~/stores/user-settings";
import { trpcHelpers } from "~/utils/helpers";
import { getBaseUrl } from "~/utils/url";

type AssetInfoPageStaticProps = InferGetStaticPropsType<typeof getStaticProps>;

Expand Down Expand Up @@ -79,6 +80,10 @@ const AssetInfoView: FunctionComponent<AssetInfoPageStaticProps> = observer(

const { title, details, coinGeckoId, asset: asset } = useAssetInfo();

const pageTitle = `${
title ? `${title} (${asset.coinDenom})` : asset.coinDenom
} | Osmosis`;

if (!asset) {
return null;
}
Expand Down Expand Up @@ -166,10 +171,20 @@ const AssetInfoView: FunctionComponent<AssetInfoPageStaticProps> = observer(
strategy="afterInteractive"
/>
<NextSeo
title={`${
title ? `${title} (${asset.coinDenom})` : asset.coinDenom
} | Osmosis`}
title={pageTitle}
description={details?.description}
openGraph={{
title: pageTitle,
description: details?.description?.substring(0, 120) + "...",
images: [
Comment on lines +178 to +179
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
description: details?.description?.substring(0, 120) + "...",
images: [
description: details?.description
? details.description.substring(0, 120) + "..."
: undefined,
images: [

{
url: `${getBaseUrl(true)}/api/og-images/assets/${
asset.coinDenom
}`,
Comment on lines +181 to +183
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
url: `${getBaseUrl(true)}/api/og-images/assets/${
asset.coinDenom
}`,
url: `${getBaseUrl(true)}/api/og-images/assets/${
encodeURIComponent(asset.coinDenom)
}`,

alt: title,
},
],
}}
/>
<main className="mx-auto flex max-w-7xl flex-col gap-8 px-10 pb-11 xs:px-2">
<LinkButton
Expand Down
19 changes: 19 additions & 0 deletions packages/web/utils/og-images.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
export async function loadGoogleFont(font: string, text: string) {
const url = `https://fonts.googleapis.com/css2?family=${font}&text=${encodeURIComponent(
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
const url = `https://fonts.googleapis.com/css2?family=${font}&text=${encodeURIComponent(
const url = `https://fonts.googleapis.com/css2?family=${encodeURIComponent(font)}&text=${encodeURIComponent(

text
)}`;
console.log(url);
const css = await (await fetch(url)).text();
const resource = css.match(
/src: url\((.+)\) format\('(opentype|truetype)'\)/
);

if (resource) {
const response = await fetch(resource[1]);
if (response.status == 200) {
return await response.arrayBuffer();
}
}

throw new Error("failed to load font data");
}
7 changes: 1 addition & 6 deletions packages/web/utils/trpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,7 @@ import {
constructEdgeUrlPathname,
EdgeRouterKey,
} from "~/utils/trpc-edge";

const getBaseUrl = () => {
if (typeof window !== "undefined") return ""; // browser should use relative url
if (process.env.VERCEL_URL) return `https://${process.env.VERCEL_URL}`; // SSR should use vercel url
return `http://localhost:${process.env.PORT ?? 3000}`; // dev SSR should use localhost
};
import { getBaseUrl } from "~/utils/url";

/**
* Create a minimal local router to avoid importing
Expand Down
6 changes: 6 additions & 0 deletions packages/web/utils/url.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
export const getBaseUrl = (absolute = false) => {
if (!absolute && typeof window !== "undefined") return ""; // browser should use relative url
if (process.env.VERCEL_URL) return `https://${process.env.VERCEL_URL}`; // SSR should use vercel url
return `http://localhost:${process.env.PORT ?? 3000}`; // dev SSR should use localhost
};

export function removeQueryParam(key: string) {
if (!key) return;
if (typeof window === "undefined") return;
Expand Down
Loading