-
Notifications
You must be signed in to change notification settings - Fork 160
chore(cow-fi): sitemap - cache getAllArticleSlugsWithDates() value #6599
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
chore(cow-fi): sitemap - cache getAllArticleSlugsWithDates() value #6599
Conversation
|
@crutch12 is attempting to deploy a commit to the cow-dev Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughIntroduces an in-memory caching wrapper for asynchronous functions in the sitemap configuration. A cached variant of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
🧹 Nitpick comments (1)
apps/cow-fi/next-sitemap.config.js (1)
54-68: Arguments accepted but not used for cache keying.The function signature accepts
...args, but the implementation caches a single result globally without considering the arguments. If this wrapper is ever used with a function that takes different arguments, it will incorrectly return the cached result from the first call regardless of argument values.For the current use case (wrapping a zero-argument function), this works correctly. However, the misleading signature could cause bugs if reused elsewhere.
Consider one of these approaches:
Option 1 (simpler): Remove
...argsto make the intent explicit:-function cacheAsyncFunction(fn) { +function cacheZeroArgAsyncFunction(fn) { const EMPTY = Symbol() let result = EMPTY - return async function (...args) { + return async function () { if (result !== EMPTY) return result - result = fn(...args).catch((err) => { + result = fn().catch((err) => { result = EMPTY throw err }) return result } }Option 2 (more flexible): Implement proper argument-based cache keying using a Map with serialized keys, but this adds complexity that may not be needed for this use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cow-fi/next-sitemap.config.js(2 hunks)
🔇 Additional comments (2)
apps/cow-fi/next-sitemap.config.js (2)
70-71: LGTM!The type annotation correctly preserves the function signature, and the cached wrapper is appropriately applied to optimize sitemap generation performance.
25-25: LGTM!Correctly switched to the cached variant, which will prevent repeated expensive CMS API calls during sitemap generation for multiple
/learn/*pages.
shoom3301
left a comment
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.
Awesome! Thank you!
Summary
next-sitemap's
transformfunctions callsgetAllArticleSlugsWithDatesfor every/learn/**slug, so it lasts a long timeFix:
getAllArticleSlugsWithDates()callTo Test
yarn build:cowfisitemap.xmlshould be generated very quicklySummary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.