fix: wait page readiness for robots and og #933
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe PR moves the package OG image definition to the top of the page and changes its data source to Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/pages/package/[...package].vue (1)
461-463: Redundant case normalisation.The regex already uses the
/iflag for case-insensitive matching, making.toLowerCase()unnecessary.♻️ Suggested simplification
- const agent = event?.node.req.headers['user-agent']?.toLowerCase() || '' - const crawlerRegex = - /(facebookexternalhit|bluesky|twitterbot|linkedinbot|discordbot|slackbot|telegrambot|whatsapp|vkshare|skypeuripreview|googlebot|bingbot|yandexbot|baiduspider|duckduckbot|crawler|spider|bot|preview)/i + const agent = event?.node.req.headers['user-agent'] || '' + const crawlerRegex = + /(facebookexternalhit|bluesky|twitterbot|linkedinbot|discordbot|slackbot|telegrambot|whatsapp|vkshare|skypeuripreview|googlebot|bingbot|yandexbot|baiduspider|duckduckbot|crawler|spider|bot|preview)/i
|
Found a specific case: if the user visited this page themselves and visited it before the crawler, we'll cache the user's version (without wait). Generally, this link isn't very accessible to users, and they shouldn't visit it, but it's still topic to talk about |
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
How about instead we move this logic into the Package ogImage component itself and perform the fetch there, awaiting the data?
|
@danielroe How confident are we in caching and memoization? Couldn't this results that the OG crawler will load data from the main page to obtain the title and description, and the same requests will be sent from this component in parallel? And how critical is this probability 🤔 |
|
it would deduplicate. |
|
It looks much better now, thanks |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/components/OgImage/Package.vue (2)
52-57: Consider handling undefined stars gracefully.If
stars.valueisundefinedornull,Intl.NumberFormat.format()will produce "NaN". Consider providing a fallback value.♻️ Proposed improvement
const formattedStars = computed(() => Intl.NumberFormat('en', { notation: 'compact', maximumFractionDigits: 1, - }).format(stars.value), + }).format(stars.value ?? 0), )
122-123: Consider defensive access fordownloads.downloads.Whilst
v-if="downloads"guards against a nulldownloadsobject, if the API response structure ever changes ordownloads.downloadsisundefined, this could display incorrectly. Consider using optional chaining or a fallback.♻️ Proposed improvement
- <span v-if="downloads"> - <span>• {{ $n(downloads.downloads) }} </span> + <span v-if="downloads?.downloads != null"> + <span>• {{ $n(downloads.downloads) }} </span>
|
Oops, sorry I missed that. Is there anything I can do to catch these errors locally? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/OgImage/Package.vue (1)
51-56: Consider guarding against undefinedstarsvalue.If
stars.valueisundefinedornullbefore the repo metadata loads,Intl.NumberFormat.format()will return"NaN". Consider providing a fallback.💡 Suggested improvement
const formattedStars = computed(() => Intl.NumberFormat('en', { notation: 'compact', maximumFractionDigits: 1, - }).format(stars.value), + }).format(stars.value ?? 0), )
| const _default: any | ||
|
|
||
| export default _default |
There was a problem hiding this comment.
Fix invalid TypeScript declaration syntax.
The const declaration without initialisation is a syntax error. For declaration files (.d.ts / .d.vue.ts), use the declare keyword. Additionally, consider providing a more specific type than any for better type safety.
🛠️ Proposed fix
-const _default: any
-
-export default _default
+declare const _default: import('vue').DefineComponent<{
+ name: string
+ version: string
+ primaryColor?: string
+}>
+
+export default _defaultAlternatively, if a minimal fix is preferred:
-const _default: any
+declare const _default: any📝 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 _default: any | |
| export default _default | |
| declare const _default: any | |
| export default _default |
🧰 Tools
🪛 Biome (2.3.13)
[error] 1-1: Const declarations must have an initialized value.
This variable needs to be initialized.
(parse)
2 participants
What
Stars weren't showing up in OG images. This made previews look sad
The problem was that we weren't actually waiting for stars to load, as they're lazy.
Downloads worked because there was an await loadVersion below, which essentially just took longer, and that was enough.
Fix
I added a condition that if it is one of the standard scanners, it will wait for a full render on the server and only then return a response.
Test
To test we can override the agent (Chrome docs) for something like:
facebookexternalhit/1.1 (+http://www.facebook.com/externalhit_uatext.php)Closes #932