-
Notifications
You must be signed in to change notification settings - Fork 2
moving rsvps to local api route #14
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Pull Request Overview
This PR migrates RSVP count fetching from an external API to an internal Next.js API route with Airtable integration. It adds caching to reduce API calls and simplifies the git API by removing the branch environment logic.
- Creates a new
/api/rsvpendpoint that fetches RSVP counts from Airtable with built-in caching - Updates the RSVP component to consume the new internal API endpoint
- Removes conditional branch logic from the git API, hardcoding to "main"
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/pages/api/rsvp.ts | New API route that fetches RSVP count from Airtable with 5-second cache refresh using setInterval |
| src/components/RSVP.tsx | Updated to fetch from new /api/rsvp endpoint instead of external API, adjusted polling interval and response field names |
| src/pages/api/git.ts | Removed branch environment variable logic, hardcoded to "main" branch |
| bun.lock | Added airtable package and its dependencies |
Comments suppressed due to low confidence (2)
src/pages/api/rsvp.ts:2
- Unused import Airtable.
src/pages/api/rsvp.ts:25 - Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/pages/api/rsvp.ts
Outdated
| setInterval(async () => { | ||
| const count = await getCount(); | ||
| cached = { value: count, updated: Date.now() }; | ||
| console.log("cached value", cached); | ||
| }, 5000); | ||
|
|
||
| export default async function handler(_req: NextApiRequest, res: NextApiResponse) { |
Copilot
AI
Oct 30, 2025
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.
Using setInterval at module level in a Next.js API route creates a new interval each time the module is loaded in serverless environments, leading to multiple overlapping timers and memory leaks. In serverless/edge deployments, this approach is unreliable. Consider implementing time-based cache invalidation within the handler function (e.g., checking if cached.updated is stale) or using a separate cron job/scheduled task for cache updates.
| setInterval(async () => { | |
| const count = await getCount(); | |
| cached = { value: count, updated: Date.now() }; | |
| console.log("cached value", cached); | |
| }, 5000); | |
| export default async function handler(_req: NextApiRequest, res: NextApiResponse) { | |
| export default async function handler(_req: NextApiRequest, res: NextApiResponse) { | |
| // Invalidate cache if older than 5 seconds | |
| if (Date.now() - cached.updated > 5000) { | |
| const count = await getCount(); | |
| cached = { value: count, updated: Date.now() }; | |
| console.log("cached value", cached); | |
| } |
thesleepyniko
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.
code mostly lgtm, but do take a look at some of copilots change requests because i do think some of them make sense, so marking as request changes
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@boykisserchan quick question is this ready for rereview? |
|
im gonna rerequest copilot review once i pus hthis one change and then request one review from like you or ivie |
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.
Pull Request Overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
thesleepyniko
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.
lgtm!
This pull request updates how RSVP counts are fetched and displayed, switching from a direct client-side fetch of an external API to a server-side API route that manages caching and rate limiting. It also improves the freshness and reliability of the displayed count, and makes a minor adjustment to the GitHub commit fetching logic.
RSVP Count Fetching Improvements:
src/pages/api/rsvp.tsthat fetches RSVP counts from Airtable, paginates through all records, and caches the result for improved performance and rate limiting. The count is refreshed every 5 seconds.RSVPcomponent insrc/components/RSVP.tsxto use the new/api/rsvpendpoint, reduced the refresh interval to 10 seconds, and improved handling of loading and error states. The component now displays the newcountfield from the API.