-
Notifications
You must be signed in to change notification settings - Fork 16
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
Feature/adding ogimage to app #56
base: main
Are you sure you want to change the base?
Conversation
@faisal004 is attempting to deploy a commit to the Ankur 's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces several configuration and metadata-related changes across multiple files. The primary modifications include adding new environment variables in Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (8)
src/app/blog/page.tsx (2)
3-4
: Prefer explicit grouping for imports.Although the import of "Metadata" from 'next' is minimal, consider grouping related imports together or adding a brief comment describing its usage, to improve clarity and maintainability.
37-60
: Check performance impact when fetching posts.Although the function successfully fetches all posts and displays them, if the blog grows large, fetching all posts might impact performance. Consider later implementing pagination or lazy loading.
src/app/layout.tsx (1)
27-34
: Consider simplifying repeated environment references.You’re using process.env.NEXT_PUBLIC_BASE_URL multiple times. This is fine, but consider centralizing or caching it in a variable to reduce repeated checks and increase clarity.
src/app/api/og/route.tsx (2)
4-7
: Validate presence of “title” search param.Currently, the logic doesn’t handle the case where “title” is absent or whitespace. Consider providing a helpful placeholder or a default value to keep the generated image consistent.
45-75
: Remove duplication in layout blocks.You have two large flex containers, one for the main heading (lines 21–43) and another for the title block (lines 45–74). If you foresee future modifications, consider refactoring some shared styles into a helper function or a shared component.
src/app/blog/[slug]/page.tsx (1)
123-167
: Ensure tested coverage of blog content rendering.Your updated “BlogPost” component uses advanced MDX rendering. Since there are multiple components involved (like , ), confirm thorough test coverage of these sub-renders.
.env.example (1)
1-1
: Consider making BASE_URL configurable for different environmentsThe hardcoded production URL might cause issues during local development. Consider using a placeholder or adding a comment about environment-specific values.
-NEXT_PUBLIC_BASE_URL=https://www.devtoolsacademy.com +NEXT_PUBLIC_BASE_URL=http://localhost:3000 # Use appropriate URL for each environment +# Development: http://localhost:3000 +# Production: https://www.devtoolsacademy.compackage.json (1)
13-13
: LGTM! Consider documenting Prisma setupThe addition of the postinstall script is a good practice to ensure the Prisma client is always generated after dependency installation.
Consider adding a note in the README about the automatic Prisma client generation during installation, especially for new contributors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
public/og-bg.png
is excluded by!**/*.png
public/og-image.png
is excluded by!**/*.png
📒 Files selected for processing (6)
.env.example
(1 hunks)package.json
(1 hunks)src/app/api/og/route.tsx
(1 hunks)src/app/blog/[slug]/page.tsx
(1 hunks)src/app/blog/page.tsx
(1 hunks)src/app/layout.tsx
(1 hunks)
🔇 Additional comments (7)
src/app/layout.tsx (1)
23-23
: Open Graph URL fallback is sensible.
The fallback to 'https://devtoolsacademy.com' is a good practice. Any consumer expecting a valid URL can rely on this default. No immediate change needed.
src/app/api/og/route.tsx (2)
1-3
: File imports look good.
The imports from 'next/og' and 'next/server' are aligned with the new dynamic OG generation. This is a clean setup.
76-100
: Double-check absolute positioning and layering.
The usage of “absolute inset-0” (lines 76–77) in multiple blocks can lead to overlaps if incorrectly combined. Ensure that high-level containers don’t accidentally compete on z-index.
✅ Verification successful
Let me search for any potential layering issues in the codebase.
Let me search for more context around these absolute positioned elements to understand their container structure.
No layering issues found in the OG image generation
The absolute positioning is used correctly in this context. The file is generating an Open Graph image where:
- Both absolute positioned elements (
h1
tags) are contained within the same parentdiv
- The first
h1
is positioned at the top (withmb-96
) - The second
h1
is positioned withmt-57
andmarginTop: 150
- The elements serve different purposes (logo+title and tagline) and are intentionally positioned this way
- There's no z-index competition as they're meant to be at fixed positions for the image generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for absolute positioning and z-index usage in the same file
rg -A 5 "absolute.*inset-0|inset-0.*absolute" src/app/api/og/route.tsx
# Search for z-index usage in the file
rg "z-index|zIndex" src/app/api/og/route.tsx
Length of output: 687
Script:
#!/bin/bash
# Get the full content of the file to understand the structure
cat src/app/api/og/route.tsx
Length of output: 3029
src/app/blog/[slug]/page.tsx (2)
171-175
: Generate static params for performance gains.
This approach, combined with Next.js static generation, is great for performance. The code is straightforward. Keep in mind that if your blogging platform grows, dynamic rendering or incremental static regeneration might eventually be needed.
78-122
: Dynamic metadata generation is effective but confirm fallback.
The logic for generating a fallback OG image when featuredImage is absent is solid. However, be sure to confirm that “post.description” is present or provide a suitable fallback for missing descriptions.
✅ Verification successful
Let me generate shell scripts to verify the handling of post descriptions and potential fallbacks.
This will help us understand:
- How post data is retrieved and structured
- If there are any fallback mechanisms for descriptions
- Whether description is a required field in the Post interface
Description fallback is properly implemented, no issues found
Looking at the getPostBySlug
implementation in src/lib/posts.ts
, there's already a robust fallback chain for descriptions:
description: data.description || data.summary || ''
This ensures that:
- It first uses the post's description if available
- Falls back to the post's summary if description is missing
- Defaults to an empty string as the final fallback
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for the getPostBySlug implementation to understand post structure
ast-grep --pattern 'export async function getPostBySlug($_) {
$$$
}'
# Search for any default or fallback descriptions
rg "description.*fallback" -A 2
# Look for post type definitions to understand required fields
ast-grep --pattern 'interface Post {
$$$
}'
Length of output: 1783
.env.example (1)
Line range hint 5-5
: Clarify Umami analytics integration
The addition of Umami website tracking wasn't mentioned in the PR objectives. Please clarify:
- Is this intentional as part of this PR?
- Should there be documentation updates regarding analytics?
- Consider adding a comment explaining the purpose of this variable.
package.json (1)
Line range hint 1-1
: Verify og image assets
Let's verify the og images mentioned in the PR description are properly added to the public directory.
✅ Verification successful
OG images are properly configured in the repository
The verification confirms that the necessary OG images are present in the public directory:
- Default OG image:
/public/images/default-og-image.png
- Base OG assets:
/public/og-bg.png
and/public/og-image.png
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if og images exist in public directory
fd -t f -e png -e jpg -e jpeg -e gif -e webp . public/ | grep -i "og"
Length of output: 1147
src/app/blog/page.tsx
Outdated
export const metadata: Metadata = { | ||
title: 'Blogs | Dev Tools Academy', | ||
description: 'A special blog made for Developers.', | ||
openGraph: { | ||
title: 'Blogs | Dev Tools Academy', | ||
description: 'A special blog made for Developers.', | ||
url: process.env.NEXT_PUBLIC_BASE_URL, | ||
images: [ | ||
{ | ||
url: `${process.env.NEXT_PUBLIC_BASE_URL}/api/og?title=Blogs%20|%20DevToolsAcademy`, | ||
width: 1200, | ||
height: 639, | ||
alt: 'DevToolsAcademy', | ||
}, | ||
], | ||
}, | ||
twitter: { | ||
card: 'summary_large_image', | ||
title: 'Blogs | Dev Tools Academy', | ||
description: 'A special blog made for Developers.', | ||
images: [ | ||
{ | ||
url: `${process.env.NEXT_PUBLIC_BASE_URL}/api/og?title=Blogs%20|%20DevToolsAcademy`, | ||
width: 1200, | ||
height: 639, | ||
alt: 'DevToolsAcademy', | ||
}, | ||
], | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Fallback behavior is properly implemented in the root layout, but missing in blog page
The root layout (layout.tsx) properly implements fallbacks for the NEXT_PUBLIC_BASE_URL
:
- OpenGraph URL:
process.env.NEXT_PUBLIC_BASE_URL ?? 'https://devtoolsacademy.com'
- SocialMetadata component:
metadata.metadataBase?.toString() ?? 'https://devtoolsacademy.com'
However, the blog page (blog/page.tsx) uses process.env.NEXT_PUBLIC_BASE_URL
without any fallback, which could lead to undefined URLs in non-production environments. Recommend adding similar fallback patterns:
- For OpenGraph URL:
process.env.NEXT_PUBLIC_BASE_URL ?? 'https://devtoolsacademy.com'
- For image URLs:
`${process.env.NEXT_PUBLIC_BASE_URL ?? 'https://devtoolsacademy.com'}/api/og?title=Blogs%20|%20DevToolsAcademy`
🔗 Analysis chain
Structured metadata is correct, but verify fallback.
The newly introduced “metadata” object is well-structured and uses environment variables for the Open Graph images. However, double-check what happens if NEXT_PUBLIC_BASE_URL is undefined in non-production environments and confirm that the fallback behavior is appropriate.
Let me verify the fallback behavior for NEXT_PUBLIC_BASE_URL
by searching for any related configurations or fallback definitions in the codebase.
Let me check the next.config.js file to see if there are any environment variable configurations or fallbacks defined.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for NEXT_PUBLIC_BASE_URL configurations or fallbacks
rg "NEXT_PUBLIC_BASE_URL" -B 2 -A 2
# Look for environment configuration files
fd -e env -e env.local -e env.development -e env.production
# Check for any Next.js config files that might handle environment variables
fd -g "next.config.*"
Length of output: 2839
Script:
#!/bin/bash
# Check next.config.js content
cat next.config.js
# Also check if there's a default value set in the layout.tsx file
cat src/app/layout.tsx
Length of output: 4606
src/app/layout.tsx
Outdated
@@ -16,14 +16,22 @@ export const metadata: Metadata = { | |||
template: '%s | Dev Tools Academy', | |||
}, | |||
description: 'Learn about awesome developer tools', | |||
metadataBase: new URL('https://devtoolsacademy.com'), | |||
metadataBase: new URL(`${process.env.NEXT_PUBLIC_BASE_URL}` ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Based on the search results, I can now provide a final response:
Environment variable already has a fallback mechanism in place
The codebase already implements a fallback mechanism for NEXT_PUBLIC_BASE_URL
in the OpenGraph configuration using the nullish coalescing operator (??
), defaulting to 'https://devtoolsacademy.com'. However, the metadataBase
usage doesn't have this fallback. Consider applying the same pattern:
metadataBase: new URL(process.env.NEXT_PUBLIC_BASE_URL ?? 'https://devtoolsacademy.com'),
🔗 Analysis chain
Validate environment variables in non-production scenarios.
Using a direct type cast with new URL(process.env.NEXT_PUBLIC_BASE_URL) could throw an error if the variable is missing or malformed. Consider providing a clearer fallback or try-catch mechanism for improved resilience.
Let me gather more information about how the environment variables are handled in the codebase.
Let me check if there's any validation or error handling for environment variables in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for NEXT_PUBLIC_BASE_URL usage and validation patterns
rg "NEXT_PUBLIC_BASE_URL" -A 3 -B 3
# Look for any environment variable validation patterns
rg "process\.env\." -A 3 -B 3
# Check for any environment configuration files
fd -e env -e env.example -e env.local
Length of output: 12431
Script:
#!/bin/bash
# Look for any environment variable validation or error handling patterns
rg "try.*catch" -A 5 -B 5
# Check for any environment variable validation utilities or config files
fd -e ts -e js -x rg -l "validateEnv|checkEnv|env.*validation"
# Look for any default values or fallbacks for BASE_URL
rg "NEXT_PUBLIC_BASE_URL.*\?\?" -A 2 -B 2
Length of output: 451
src/app/api/og/route.tsx
Outdated
return new (ImageResponse as any)( | ||
( | ||
<div | ||
style={{ | ||
height: '100%', | ||
width: '100%', | ||
display: 'flex', | ||
flexDirection: 'column', | ||
alignItems: 'flex-start', | ||
justifyContent: 'center', | ||
backgroundImage: `url("${process.env.NEXT_PUBLIC_BASE_URL}/og-bg.png")`, | ||
}} | ||
> | ||
<h1 tw="absolute inset-0 flex justify-center items-center mb-96 flex-row"> | ||
{/* eslint-disable-next-line @next/next/no-img-element */} | ||
<img | ||
tw="h-10 w-10" | ||
alt="Alt" | ||
// @ts-ignore | ||
src={`${process.env.NEXT_PUBLIC_BASE_URL}/favicon.png`} | ||
/> | ||
<span | ||
tw="ml-3" | ||
style={{ | ||
background: | ||
'linear-gradient(180deg, rgba(255,255,255,1) 0%, rgba(156, 163, 175, 1) 100%)', | ||
backgroundClip: 'text', | ||
WebkitBackgroundClip: 'text', | ||
color: 'transparent', | ||
fontSize: 35, | ||
letterSpacing: '-1.5px', | ||
}} | ||
> | ||
DevToolsAcademy | ||
</span> | ||
</h1> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
The styling approach needs to be standardized across the codebase
The codebase shows mixed styling patterns:
- The OG route uses Twin.Macro (
tw
attributes) along with inline styles - Other components primarily use standard Tailwind CSS with the project's configured theme
- There's no explicit Twin.Macro setup in the build configuration (package.json, next.config.js)
- Consider removing Twin.Macro from
src/app/api/og/route.tsx
and use standard Tailwind CSS classes to maintain consistency with the rest of the codebase - If Twin.Macro is needed specifically for OG image generation, ensure it's properly configured in the build pipeline
🔗 Analysis chain
Review styling references.
The inline styling plus Twin.Macro syntax (the “tw” attributes) is an interesting approach. Confirm that your build pipeline supports it properly and ensure it’s consistent throughout the codebase to maintain uniform styling.
Let me verify the usage of Twin.Macro and styling approaches in the codebase.
Let me check the actual implementation of styling in these files to verify consistency.
Let me check for any Twin.Macro configuration files and verify the build setup.
Let me check the tailwind configuration and any other styling-related setup files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for Twin.Macro imports and usage
rg -l "twin\.macro|tw=\""
# Search for files with similar styling patterns
rg -l "style=\{\{|tw=\""
# Look for any Twin.Macro configuration files
fd -g "*twin*" -g "*tailwind*"
Length of output: 405
Script:
#!/bin/bash
# Check the styling patterns in the identified files
echo "=== TableOfContents.tsx ==="
rg "tw=\"|style=\{\{" src/components/TableOfContents.tsx -A 2
echo "=== Hero.tsx ==="
rg "tw=\"|style=\{\{" src/components/Hero.tsx -A 2
echo "=== Footer.tsx ==="
rg "tw=\"|style=\{\{" src/components/Footer.tsx -A 2
echo "=== ContributePage.tsx ==="
rg "tw=\"|style=\{\{" src/components/ContributePage.tsx -A 2
echo "=== ToolDetailsPage.tsx ==="
rg "tw=\"|style=\{\{" src/components/tools/toolDetails/ToolDetailsPage.tsx -A 2
Length of output: 1603
Script:
#!/bin/bash
# Check for Twin.Macro configuration in package.json and other config files
echo "=== Package.json Twin.Macro dependencies ==="
cat package.json | grep -A 5 -B 5 "twin"
echo "=== Tailwind Config ==="
fd -e js -e ts -e json -g "*tailwind*"
echo "=== Babel Config ==="
fd -e js -e json -g "*babel*"
echo "=== Next.js Config ==="
cat next.config.js 2>/dev/null || echo "next.config.js not found"
Length of output: 2113
Script:
#!/bin/bash
# Check Tailwind configuration
echo "=== Tailwind Config Content ==="
cat tailwind.config.ts
# Check for any global styles
echo "=== Global Styles ==="
fd -e css -e scss
# Check for styled-components or emotion setup
echo "=== Styling Setup in _app or layout files ==="
rg -l "styled-components|@emotion" src/
Length of output: 4590
@faisal004 PR looks good, though you may need to change some code after #58 is merged, components are moved and the blog page also has some changes |
ok so should i wait till #58 is merged?? |
yeah that'll be better, it should be done by mon-tue. thanks |
ok ill resolves the conflicts then and raise a pr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/blog/page.tsx (1)
13-16
:⚠️ Potential issueAdd fallback for NEXT_PUBLIC_BASE_URL
The environment variable is used without a fallback value, which could lead to undefined URLs in non-production environments.
Apply this diff to add fallbacks:
- url: process.env.NEXT_PUBLIC_BASE_URL, + url: process.env.NEXT_PUBLIC_BASE_URL ?? 'https://devtoolsacademy.com', images: [ { - url: `${process.env.NEXT_PUBLIC_BASE_URL}/api/og?title=Blogs%20|%20DevToolsAcademy`, + url: `${process.env.NEXT_PUBLIC_BASE_URL ?? 'https://devtoolsacademy.com'}/api/og?title=Blogs%20|%20DevToolsAcademy`,Make similar changes for the Twitter card image URL.
Also applies to: 29-30
🧹 Nitpick comments (3)
src/app/blog/page.tsx (1)
14-34
: Consider extracting common image configurationThe image configuration is duplicated between OpenGraph and Twitter cards. Consider extracting it to a constant to improve maintainability.
Example refactor:
const ogImage = { url: `${process.env.NEXT_PUBLIC_BASE_URL ?? 'https://devtoolsacademy.com'}/api/og?title=Blogs%20|%20DevToolsAcademy`, width: 1200, height: 639, alt: "DevToolsAcademy", }; export const metadata: Metadata = { // ... other metadata openGraph: { // ... other og metadata images: [ogImage], }, twitter: { // ... other twitter metadata images: [ogImage], }, };src/app/layout.tsx (2)
34-34
: Fix typo in image alt textThere's a typo in the alt text: "Dev Tools Academ" should be "Dev Tools Academy"
Apply this diff:
- alt: "Dev Tools Academ", + alt: "Dev Tools Academy",
Line range hint
21-96
: Standardize fallback pattern usageThe codebase uses inconsistent patterns for fallbacks. Some places use nullish coalescing (
??
), while others directly use the environment variable.Standardize the fallback pattern across all URL constructions:
- metadataBase URL
- OpenGraph URL
- SocialMetadata image URL
Example pattern to follow:
const BASE_URL = process.env.NEXT_PUBLIC_BASE_URL ?? 'https://devtoolsacademy.com';Then use this constant throughout the configuration to maintain consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json
(1 hunks)src/app/api/og/route.tsx
(1 hunks)src/app/blog/[slug]/page.tsx
(2 hunks)src/app/blog/page.tsx
(1 hunks)src/app/layout.tsx
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- src/app/api/og/route.tsx
- src/app/blog/[slug]/page.tsx
🔇 Additional comments (1)
src/app/layout.tsx (1)
21-21
:⚠️ Potential issueAdd fallback for metadataBase URL constructor
The URL constructor will throw an error if NEXT_PUBLIC_BASE_URL is undefined. Add a fallback value.
Apply this diff:
- metadataBase: new URL(`${process.env.NEXT_PUBLIC_BASE_URL}`), + metadataBase: new URL(process.env.NEXT_PUBLIC_BASE_URL ?? 'https://devtoolsacademy.com'),Likely invalid or redundant comment.
Hi in above PR I have added og images
Changes I made are listed below
I have add two images to public
also for the change to reflect we need to update the .env so I have update .env.example accordingly
now if you are on main page and check its og preview
if you are on blog page then for route no og-image is added to public instead dynamic image will be generated and it will look like this
now for main blog page if blog contains image url then that image will be used as og image else image will be generated
Summary by CodeRabbit
New Features
Bug Fixes
Documentation