Feat:add og image component and add feature flag for og image component #1107
Conversation
|
@South-IN is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request enhance the Open Graph image generation functionality within the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
|
Hello @South-IN, thanks for opening your first Pull Request. The maintainers will review this Pull Request and provide feedback as soon as possible. Keep up the great work! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
app/posthog.ts (1)
3-8: Overall structure looks good, but consider some improvements.The
PostHogClientfunction is well-structured and correctly initializes the PostHog client using environment variables. However, there are a few areas that could be improved:
Error handling for missing environment variables:
The current implementation doesn't handle cases whereNEXT_PUBLIC_POSTHOG_KEYis not set, which could lead to unexpected behavior.Exception handling:
The PostHog constructor might throw exceptions, which are currently not caught.Type safety:
Consider using TypeScript to improve type safety and developer experience.Here's a suggested implementation addressing these points:
import { PostHog } from 'posthog-node'; export default function PostHogClient(): PostHog | null { const posthogKey = process.env.NEXT_PUBLIC_POSTHOG_KEY; if (!posthogKey) { console.error('PostHog key is not set in environment variables'); return null; } try { const posthogClient = new PostHog(posthogKey, { host: process.env.NEXT_PUBLIC_POSTHOG_HOST }); return posthogClient; } catch (error) { console.error('Failed to initialize PostHog client:', error); return null; } }This implementation:
- Checks for the presence of the PostHog key.
- Uses a try-catch block to handle potential exceptions.
- Uses TypeScript for improved type safety.
- Returns
nullin error cases, allowing the caller to handle the absence of a client gracefully.app/og/route.tsx (2)
Line range hint
24-28: Add error handling for font data fetching.The code fetches font data from an external URL without handling potential errors. This could lead to unhandled exceptions if the fetch fails. Consider adding error handling to manage such scenarios gracefully.
Apply this diff to fix the issue:
93-102: Use relative URLs for images to improve maintainability.The
srcattribute for images uses absolute URLs, which can lead to issues if the domain changes. Replace them with relative URLs or use theoriginvariable to construct the full URL dynamically.Apply this diff to improve:
Ensure the image exists at the specified path on your server.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (5)
package.jsonis excluded by!**/*.jsonpublic/images/og/noise.svgis excluded by!**/*.svg,!**/*.svgpublic/images/og/planet.svgis excluded by!**/*.svg,!**/*.svgpublic/images/og/stars.svgis excluded by!**/*.svg,!**/*.svgpublic/images/og/waves.svgis excluded by!**/*.svg,!**/*.svg
📒 Files selected for processing (2)
- app/og/route.tsx (5 hunks)
- app/posthog.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
app/posthog.ts (1)
1-1: LGTM: Import statement is correct.The import of
PostHogfrom 'posthog-node' is appropriate for initializing the PostHog client.app/og/route.tsx (2)
178-179:⚠️ Potential issueRemove conflicting Tailwind CSS classes.
In the
twattribute, bothbg-gradient-to-randbg-red-500are used together, which might conflict. Since you're specifying a custom gradient in thestyle, you can removebg-red-500.Apply this diff to fix:
Likely invalid or redundant comment.
14-17: Verify that PostHog events are capturing correctly.Ensure that the
posthogClient.capturemethod is sending events as expected. It's important to verify that the event data, includingdistinctIdandeventname, are correctly recorded in PostHog.Run the following script to check for issues in event capturing:
Ensure that the events are appearing in PostHog with the correct properties.
app/posthog.ts
Outdated
| const posthogClient = new PostHog(process.env.NEXT_PUBLIC_POSTHOG_KEY||"",{ | ||
| host: process.env.NEXT_PUBLIC_POSTHOG_HOST | ||
| }) |
There was a problem hiding this comment.
Security concern: Potential exposure of PostHog key to client-side.
The use of environment variables prefixed with NEXT_PUBLIC_ for the PostHog key and host is concerning. In Next.js, variables prefixed with NEXT_PUBLIC_ are exposed to the browser, which could potentially expose your PostHog API key to client-side code.
Consider renaming these environment variables to remove the NEXT_PUBLIC_ prefix:
- const posthogClient = new PostHog(process.env.NEXT_PUBLIC_POSTHOG_KEY||"",{
- host: process.env.NEXT_PUBLIC_POSTHOG_HOST
+ const posthogClient = new PostHog(process.env.POSTHOG_KEY||"",{
+ host: process.env.POSTHOG_HOSTAlso, update any configuration files or deployment scripts to use these new variable names.
📝 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 posthogClient = new PostHog(process.env.NEXT_PUBLIC_POSTHOG_KEY||"",{ | |
| host: process.env.NEXT_PUBLIC_POSTHOG_HOST | |
| }) | |
| const posthogClient = new PostHog(process.env.POSTHOG_KEY||"",{ | |
| host: process.env.POSTHOG_HOST | |
| }) |
app/og/route.tsx
Outdated
| distinctId:"south-in", | ||
| event:"Og image generated" |
There was a problem hiding this comment.
Avoid hardcoding 'distinctId' to prevent PII leakage.
Using a hardcoded distinctId ("south-in") in posthogClient.capture and postHog.getAllFlags can lead to security and privacy concerns, as it may expose personally identifiable information (PII). Instead, consider dynamically generating the distinctId based on the user's session or anonymized data.
Apply this diff to fix the issue:
...Also applies to: 218-219
app/og/route.tsx
Outdated
| const posthogClient = PostHogClient() | ||
| const flags = await getData() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize 'getData' function placement and usage.
The getData function is declared after it's called, which can affect readability. Additionally, creating a new PostHog client inside getData leads to multiple instances. Pass the existing posthogClient to getData and consider moving the function above its usage.
Apply this diff to refactor:
...
...Also applies to: 216-220
app/og/route.tsx
Outdated
| lineClamp: 3, | ||
|
|
||
| }} |
There was a problem hiding this comment.
Replace invalid CSS property 'lineClamp' with proper text truncation.
The lineClamp style property is not standard in CSS. To truncate text after a certain number of lines, use the -webkit-line-clamp property along with other necessary styles for cross-browser support.
Apply this diff to fix the issue:
Alternatively, if you're using Tailwind CSS with the @tailwindcss/line-clamp plugin, you can simplify it:
Ensure the plugin is installed and configured correctly.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
This looks so cool! Well done. 🥇
A few small changes.
- Addng some padding to the surrounding lines.
- You can delete the author name and article tag for the moment because I forgot I use this for the profile page too and will need to figure out how to separate.
- You can remove the feature flag for now (since without the article tag and the user it'll be safe to push to production). But major kudos for setting it up and doing the screen recording, it made my life much easier.
- Add
text-balanceto the title so it flows a little more evenly.
|
Thank you for the feedback. |
…-IN/codu into feature/og-image-component
Component after requested changes :
|
|
Uh oh! @South-IN, the image you shared is missing helpful alt text. Check #1107 (comment). Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs. |
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
|
Just linting issues now so if you run |
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
2 participants
✨ Codu Pull Request 💻
Fixes #1066
Pull Request details
Any Breaking changes
None
Associated Screenshots
New image component :
Feature flag demo :
og-image-feature-flag.mp4