Change logic for Sitemap CronJob #1912
Conversation
WalkthroughThe pull request modifies the functionality of the cron job responsible for generating a sitemap on the frontend. It introduces new asynchronous functions to fetch projects, users, and qfRounds from the database, replacing a previous generic request. The job now uses a POST request instead of GET, sending a detailed payload of collected data. Additionally, it retrieves the frontend URL from environment variables and includes enhanced logging for better observability. Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/services/cronJobs/generateSitemapOnFrontend.ts (5)
4-4: Use consistent terminology in comments.Consider updating "qfRounds" to "QF rounds" for clarity and consistency.
44-47: ValidateFRONTEND_URLto ensure it's a valid URL.Even if
FRONTEND_URLis defined, it might not be a valid URL. Consider adding validation to ensurefrontendUrlis correctly formed before making the request.You can enhance the validation as follows:
const frontendUrl = FRONTEND_URL.startsWith('http') ? FRONTEND_URL.trim() : `https://${FRONTEND_URL.trim()}`; + try { + new URL(frontendUrl); + } catch (e) { + logger.error('Invalid FRONTEND_URL:', frontendUrl); + return; + }
34-36: Handle empty data arrays gracefully when fetching data.If any of the
fetchProjects,fetchUsers, orfetchQFRoundsfunctions return empty arrays due to errors, the POST request still proceeds. Consider adding checks to handle empty data arrays and log warnings or adjust the request accordingly.Example:
const projects = await fetchProjects(); const users = await fetchUsers(); const qfRounds = await fetchQFRounds(); + if (projects.length === 0 && users.length === 0 && qfRounds.length === 0) { + logger.warn('No data fetched for sitemap generation. Skipping request.'); + return; + }
32-32: Avoid logging environment variables directly.Logging
process.env.FRONTEND_URLmay expose sensitive information in your logs. Consider logging theFRONTEND_URLvariable instead, which you've already assigned and possibly sanitized.Apply this diff:
- logger.debug('FRONTEND_URL:', process.env.FRONTEND_URL); + logger.debug('FRONTEND_URL:', FRONTEND_URL);
48-54: Ensure error handling covers failed POST requests.While the code is wrapped in a
try-catchblock, consider handling specific HTTP errors or adding retry logic if the POST request fails due to transient network issues.Optionally, you can enhance error handling as follows:
const response = await axios.post( `${frontendUrl}/api/generate-sitemap`, { projects, users, qfRounds, }, { headers: { Authorization: `Bearer ${process.env.SITEMAP_CRON_SECRET}`, }, + timeout: 5000, // Set a timeout for the request }, );And handle specific error cases:
} catch (error) { + if (error.response) { + // The request was made and the server responded with a status code outside of the 2xx range + logger.error('Sitemap generation failed with status:', error.response.status); + } else if (error.request) { + // The request was made but no response was received + logger.error('No response received for sitemap generation request:', error.message); + } else { + // Something happened in setting up the request that triggered an Error + logger.error('Error in sitemap generation request setup:', error.message); + } logger.error('runGenerateSitemapOnFrontend() error:', error.message); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/cronJobs/generateSitemapOnFrontend.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
src/services/cronJobs/generateSitemapOnFrontend.ts (2)
71-74: Data fetching for projects is implemented correctly.The
fetchProjectsfunction correctly retrieves active projects with the required fields.
98-100: Data fetching for QF rounds is implemented correctly.The
fetchQFRoundsfunction appropriately retrieves QF round data needed for the sitemap.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/services/cronJobs/generateSitemapOnFrontend.ts (1)
84-95:⚠️ Potential issueReview PII exposure in user data fetching.
The function selects sensitive user information (firstName, lastName, walletAddress) which may have privacy implications.
Consider limiting the data to only what's necessary for the sitemap:
const users = await User.createQueryBuilder('user') - .select(['user.firstName', 'user.lastName', 'user.walletAddress']) + .select(['user.slug', 'user.username']) .getMany();
🧹 Nitpick comments (2)
src/services/cronJobs/generateSitemapOnFrontend.ts (2)
49-55: Enhance error handling for data fetching.While the code properly sends the data via POST request, consider adding validation for the fetched data before making the request. If all fetch operations fail, you might want to skip the request entirely.
+ if (!projects.length && !users.length && !qfRounds.length) { + logger.error('No data fetched for sitemap generation'); + return; + } + const response = await axios.post(
97-108: Consider adding pagination for large datasets.While the current implementation works, fetching all records at once might cause memory issues as the dataset grows. Consider implementing pagination or limiting the number of records fetched.
try { const qfRounds = await QfRound.createQueryBuilder('qf_round') .select(['qf_round.slug', 'qf_round.name', 'qf_round.description']) + .take(1000) // Add a reasonable limit .getMany();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/cronJobs/generateSitemapOnFrontend.ts(4 hunks)
🔇 Additional comments (4)
src/services/cronJobs/generateSitemapOnFrontend.ts (4)
14-16: LGTM! Imports are properly organized.The new entity imports are necessary for the data fetching functionality.
34-39: LGTM! Early return for missing FRONTEND_URL.The validation logic properly checks for undefined or empty FRONTEND_URL and returns early, preventing invalid requests.
45-47: LGTM! Proper URL formatting.The code ensures the URL has the correct format by adding 'https://' if not already present.
70-82: LGTM! Project fetching is well implemented.The function properly filters for active projects and selects only the necessary fields for sitemap generation.
Summary by CodeRabbit
New Features
Chores