174 generate social image of visualization when pasting link in slackxlinkedin #239
Conversation
…ocial-image-of-visualization-when-pasting-link-in-slackxlinkedin
…ocial-image-of-visualization-when-pasting-link-in-slackxlinkedin
…ocial-image-of-visualization-when-pasting-link-in-slackxlinkedin
…ocial-image-of-visualization-when-pasting-link-in-slackxlinkedin
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…hen-pasting-link-in-slackxlinkedin
…-link-in-slackxlinkedin' of github.com:dash0hq/otelbin into 174-generate-social-image-of-visualization-when-pasting-link-in-slackxlinkedin
…ocial-image-of-visualization-when-pasting-link-in-slackxlinkedin
packages/otelbin/src/app/layout.tsx
Outdated
| @@ -18,6 +18,7 @@ const inter = Inter({ subsets: ["latin"] }); | |||
| export const metadata: Metadata = { | |||
| title: "OTelBin – by Dash0", | |||
| description: "Edit, visualize and share OpenTelemetry Collector configurations", | |||
| metadataBase: new URL("https://otelbin.io/"), | |||
There was a problem hiding this comment.
Issue: While this is the the correct value for our production env, it will not be the correct value for others that are deploying OTelBin or for our preview envs.
Do we need this setting, or can we just remove it?
There was a problem hiding this comment.
You're correct; we can remove it. In other fields, such as images, the full URL is used.
There was a problem hiding this comment.
Upon further examination, the next server encounters an error without the presence of 'metadataBase.' This functionality is implemented in /social-preview/[id]/page.tsx, where metadata is utilized with the url.origin
| import BarChart4 from "./svg/bar-chart-4.svg"; | ||
| import ListTree from "./svg/list-tree.svg"; | ||
|
|
||
| export const parentNodesConfig = [ |
There was a problem hiding this comment.
Issue: This whole code block is duplicated.
There was a problem hiding this comment.
Fixed and the duplicates removed.
| } | ||
| const urlHash = url.hash; | ||
| let parsedConfig = ""; | ||
| if (urlHash != null) { |
There was a problem hiding this comment.
Issue: This code path is duplication the extraction logic for URL parameters, and it is fragile, e.g., #distro=otelcol-contrib&config=… would fail to parse.
Please consider using toUrlState(toParsedUrlStateParameters(new URLSearchParams(url.hash)), [configBinding]) instead.
There was a problem hiding this comment.
Fixed and toUrlState is implemented.
|
|
||
| const edgeWidth = 80; | ||
|
|
||
| export async function GET(request: NextRequest) { |
There was a problem hiding this comment.
Question: What do we need this route for? AFAICT this can be deleted?
There was a problem hiding this comment.
I just put it as a defualt og image, yes it can be deleted.
| const redis = Redis.fromEnv(); | ||
| const edgeWidth = 80; | ||
|
|
||
| export async function GET(request: NextRequest) { |
There was a problem hiding this comment.
Suggestion: Move the route to social-preview/[id]/img/route.ts to put related logic closer together.
There was a problem hiding this comment.
New route implemented.
| > | ||
| {parentNodes?.map((parentNode, idx) => <ParentsNode key={idx} nodeData={parentNode} nodes={initNodes} />)} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Issue: We are still missing the OTelBin logo in the response.
| const mockNextRequest = new NextRequest(mockRequest, {}); | ||
| const result = await handleShortLinkRequest(mockNextRequest); | ||
| expect(redirectSpy).toHaveBeenCalledTimes(0); | ||
| expect(rewriteSpy).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
Suggestion: Test the arguments to rewriteSpy to ensure that the correct rewrite is executed.
packages/otelbin/src/middleware.ts
Outdated
| if (isBotRequest(request)) { | ||
| return NextResponse.rewrite(new URL(`/social-preview/${shortLink}`, request.url)); | ||
| } else { | ||
| return NextResponse.redirect(fullLink || "/", { |
There was a problem hiding this comment.
Suggestion: Use NextResponse.next(). That way, we don't have to re-implement the redirect logic here.
packages/otelbin/src/middleware.ts
Outdated
| export async function handleShortLinkRequest(request: NextRequest) { | ||
| if (request.nextUrl.pathname.startsWith("/s") && !request.nextUrl.pathname.startsWith("/s/new")) { | ||
| const shortLink = request.url.split("/")[request.url.split("/").length - 1] ?? ""; | ||
| const fullLink = await redis.get<string>(getShortLinkPersistenceKey(shortLink)); |
There was a problem hiding this comment.
Suggestion: We don't need to use Redis here. The call is something we can avoid. See suggestion below on NextResponse.next()
There was a problem hiding this comment.
Great suggestion! Implemented.
packages/otelbin/src/middleware.ts
Outdated
|
|
||
| export async function handleShortLinkRequest(request: NextRequest) { | ||
| if (request.nextUrl.pathname.startsWith("/s") && !request.nextUrl.pathname.startsWith("/s/new")) { | ||
| const shortLink = request.url.split("/")[request.url.split("/").length - 1] ?? ""; |
There was a problem hiding this comment.
Suggestion: Extract this from the URL via a regexp and capture groups. The way it is done right now looks like it could break below.
Example: /\/s\/([^\/]+)$/
|
Overall this seems to be going in the correct direction 👍 . It mostly requires a clean-up imo. |
packages/otelbin/public/dot.svg
Outdated
There was a problem hiding this comment.
Question: Why does this file need to exist under public?
There was a problem hiding this comment.
Fixed, base 64 data of svg image used in backgroud url.
…ocial-image-of-visualization-when-pasting-link-in-slackxlinkedin
packages/otelbin/src/middleware.ts
Outdated
| return NextResponse.rewrite(new URL(`/social-preview/${shortLink}`, request.url)); | ||
| } else { | ||
| const newHeaders = new Headers(request.headers); | ||
| newHeaders.set("Cache-Control", "public, max-age=3600, stale-while-revalidate=3600, stale-if-error=3600"); |
There was a problem hiding this comment.
Suggestion: Let the next handler take care of the caching headers. AFAICT we don't need to duplicate them here.
| const urlState = parseUrlState(hashSearchParams, binds); | ||
| return (urlState as never)[editorBinding.name]; |
There was a problem hiding this comment.
Issue: This function accepts binds, but then only returns the editor binding. That is imho unexpected, especially when compared to the behavior of useUrlState.
Why not return urlState here? Then the APIs should align.
|
@bripkens In the last commit, SVG icons for pipelines added, serverSide can not resolve React-Lucide icons. |
|
There was a problem hiding this comment.
Nice, let's get this merged @roshan-gh! 👍
In testing we did find some bugs. Let's try to iron them out with the production deployment.
bripkens
deleted the
174-generate-social-image-of-visualization-when-pasting-link-in-slackxlinkedin
branch
This pull request addresses the issue related to generating a social image for visualizations of the otel-col pipelines when pasting a configuration link in platforms such as Slack, X, and LinkedIn. The changes involve leveraging Next.js dynamic Open Graph features and incorporating Tailwind CSS for styling. Additionally, modifications have been made to the middleware in Next.js to enable the desired functionality.