-
Notifications
You must be signed in to change notification settings - Fork 0
Rsvp reminder cleanup #53
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
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 refactors the RSVP reminder functionality by reorganizing utility files into logical subdirectories and extracting shared functionality into separate modules. The changes improve code organization and maintainability.
Changes:
- Extracted and reorganized utility functions into domain-specific subdirectories (hub/, tito/, mailchimp/)
- Added a new
ApplicationCondensedinterface for simplified application data used in CSV exports and Mailchimp operations - Enhanced the
useMailchimphook with arefreshfunction to allow manual data reloading - Parameterized
getTitoRsvpListto accept an RSVP list index for more flexible RSVP list selection
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| app/_types/application.ts | Added ApplicationCondensed interface for condensed application data |
| app/(pages)/admin/_hooks/useMailchimp.ts | Refactored hook to expose refresh function using useCallback |
| app/(pages)/admin/_components/FinalizeButton.tsx | Updated imports and added call to refreshMailchimp after batch processing |
| app/(pages)/admin/_components/AdminHeader.tsx | Updated import path for prepareMailchimp |
| app/(api)/_utils/tito/getTitoInvites.ts | Added rsvpListIndex parameter to getTitoRsvpList function |
| app/(api)/_utils/tito/generateTitoCSV.ts | Updated to use ApplicationCondensed type and adjusted import paths |
| app/(api)/_utils/mailchimp/prepareMailchimp.ts | Updated import paths to reference reorganized utility files |
| app/(api)/_utils/mailchimp/mailchimpApiStatus.ts | New file containing Mailchimp API key management logic |
| app/(api)/_utils/hub/getUnredeemedHubUsers.ts | Updated import paths for reorganized files |
| app/(api)/_utils/hub/createHubInvite.ts | New file containing Hub invite creation logic (moved from parent directory) |
| app/(api)/_utils/getFilteredApplications.ts | Updated to use ApplicationCondensed type and adjusted import paths |
Comments suppressed due to low confidence (1)
app/(api)/_utils/tito/getTitoInvites.ts:20
- The function should validate that rsvpListIndex is within bounds before accessing the array. If rsvpListIndex is greater than or equal to rsvpLists.length, this will return undefined, which could cause issues in calling code. Consider adding a bounds check and throwing a descriptive error if the index is out of range.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…admissions-portal into rsvp-reminder-cleanup
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 10 out of 12 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
app/(api)/_utils/tito/getTitoInvites.ts:20
- There's a missing bounds check for the
rsvpListIndexparameter. If the index is greater than or equal to the array length, accessingrsvpLists[rsvpListIndex]will returnundefined, which could cause issues downstream. Consider adding validation to ensure the index is within bounds before accessing the array.
app/(api)/_utils/mailchimp/prepareMailchimp.ts:102 - The constant
RSVP_LIST_INDEXis duplicated across multiple files (getFilteredApplications.ts and prepareMailchimp.ts). Consider extracting this to a shared configuration file or constant module to maintain consistency and make it easier to update in the future.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 10 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
app/(api)/_utils/tito/getTitoInvites.ts:20
- Potential out-of-bounds array access. The function does not validate that
rsvpListIndexis within the bounds of thersvpListsarray before accessing it. IfrsvpListIndexis greater than or equal to the array length, this will returnundefined, which could cause issues in code that expects a valid RSVP list object. Consider adding validation such as checking ifrsvpListIndex >= rsvpLists.lengthorrsvpListIndex < 0and throwing a descriptive error.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 10 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
app/(api)/_utils/tito/getTitoInvites.ts:20
- The function doesn't validate if the rsvpListIndex is within bounds. If the index is out of bounds (e.g., if there's only one RSVP list and index 1 is passed), the function will return undefined, which could cause runtime errors in calling code that expects a valid RSVP list object. Consider adding validation to check if the index is valid before accessing the array.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| console.error('Error processing RSVP reminders:', err); | ||
| alert(`Error processing RSVP reminders: ${err.message}`); | ||
| console.error('Error while processing RSVP reminders:', err); | ||
| alert('Error processing RSVP reminders:' + err.message); |
Copilot
AI
Feb 4, 2026
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.
Missing space in error message. The error message should have a space after the colon for proper formatting.
| alert('Error processing RSVP reminders:' + err.message); | |
| alert('Error processing RSVP reminders: ' + err.message); |
| <div className="fixed inset-0 bg-black/50 flex items-center justify-center z-50"> | ||
| <div className="border-2 border-black bg-white shadow-xl max-w-lg w-full p-6 relative"> |
Copilot
AI
Feb 4, 2026
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.
The popup modal doesn't close when clicking outside of it (on the backdrop). This is a common UX pattern for modal dialogs. Consider adding an onClick handler to the backdrop div that calls setIsPopupOpen(false) to allow users to dismiss the modal by clicking outside.
| <div className="fixed inset-0 bg-black/50 flex items-center justify-center z-50"> | |
| <div className="border-2 border-black bg-white shadow-xl max-w-lg w-full p-6 relative"> | |
| <div | |
| className="fixed inset-0 bg-black/50 flex items-center justify-center z-50" | |
| onClick={() => setIsPopupOpen(false)} | |
| > | |
| <div | |
| className="border-2 border-black bg-white shadow-xl max-w-lg w-full p-6 relative" | |
| onClick={(e) => e.stopPropagation()} | |
| > |
No description provided.