Fix: awaiting function for gift generation - #691
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds drawing state and toast feedback to GiftExchangeHeader, updates the draw button UI/attributes, changes drawGiftExchange to await per-member suggestion generation (propagating errors), and adds a new test case for the draw-button disabled state that contains no assertions. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Header as GiftExchangeHeader
participant Toast as Toast Service
participant Util as drawGiftExchange
participant API as generateAndStoreSuggestions
User->>Header: Click "Draw Gift Exchange"
Header->>Header: set isDrawing = true
Header->>Toast: show success toast ("keep browser open")
Header->>Util: call drawGiftExchange()
rect rgb(230,245,255)
Note over Util,API: For each member: sequential await
Util->>API: await generateAndStoreSuggestions(member)
API-->>Util: success / error
end
alt Success
Header->>Header: set isDrawing = false
Header->>User: button re-enabled
else Error
Header->>Header: set isDrawing = false
Header->>Toast: show error toast
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/GiftExchangeHeader/GiftExchangeHeader.tsx (1)
267-269: Fix typo in dialog title."gift exchangee" should be "gift exchange".
<AlertDialogTitle> - Are you sure you want to start gift exchangee? + Are you sure you want to start gift exchange? </AlertDialogTitle>
🧹 Nitpick comments (1)
components/GiftExchangeHeader/GiftExchangeHeader.tsx (1)
138-142: Consider using a more semantically appropriate toast variant.The informational message about keeping the browser open uses
ToastVariants.Success, which typically indicates successful completion. AnInfo,Loading, orDefaultvariant (if available) would better convey the ongoing nature of the operation.toast({ - variant: ToastVariants.Success, + variant: ToastVariants.Info, // or ToastVariants.Default if Info is unavailable title: '', description: 'Please keep this browser open until our elves complete the gift drawing.', });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/GiftExchangeHeader/GiftExchangeHeader.test.tsx(1 hunks)components/GiftExchangeHeader/GiftExchangeHeader.tsx(5 hunks)lib/drawGiftExchange.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
components/GiftExchangeHeader/GiftExchangeHeader.test.tsx (1)
components/GiftExchangeHeader/GiftExchangeHeader.tsx (1)
GiftExchangeHeader(59-316)
lib/drawGiftExchange.ts (1)
lib/generateAndStoreSuggestions.ts (1)
generateAndStoreSuggestions(19-139)
components/GiftExchangeHeader/GiftExchangeHeader.tsx (2)
hooks/use-toast.ts (2)
useToast(243-267)toast(188-232)components/LoadingSpinner/LoadingSpinner.tsx (1)
LoadingSpinner(3-21)
🔇 Additional comments (1)
components/GiftExchangeHeader/GiftExchangeHeader.tsx (1)
136-165: LGTM—drawing state and error handling are well implemented.The drawing state management correctly:
- Disables the button during the operation
- Shows visual feedback via LoadingSpinner
- Resets state on error for retry capability
- Reloads on success (no need to reset state)
| // Fire and forget suggestions with error handling | ||
| // hacky way to avoid waiting for all suggestions to be generated | ||
| // avoids timeout issues | ||
| generateAndStoreSuggestions( | ||
| await generateAndStoreSuggestions( | ||
| supabase, | ||
| exchangeId, | ||
| giver.user_id, | ||
| recipient.user_id, | ||
| exchange.budget, | ||
| ).catch((error) => { | ||
| throw new SupabaseError('Failed to generate suggestions', 500, error); | ||
| }); | ||
| ); |
There was a problem hiding this comment.
Sequential awaiting of AI generation may cause timeouts and degrade reliability.
Replacing the fire-and-forget pattern with await inside the loop means the drawing process now blocks on potentially slow OpenAI API calls for each member sequentially. This change:
- Reintroduces timeout risk that the previous implementation explicitly avoided (per the comments)
- Amplifies latency linearly with member count (e.g., 10 members × ~5s per OpenAI call = 50+ seconds)
- Changes error semantics: a failure in suggestion generation for any single member now fails the entire drawing, whereas previously it was isolated
Consider reverting to the fire-and-forget approach with proper error logging, or implement parallel processing with Promise.allSettled to generate suggestions concurrently while still completing the drawing regardless of individual suggestion failures.
- await generateAndStoreSuggestions(
+ generateAndStoreSuggestions(
supabase,
exchangeId,
giver.user_id,
recipient.user_id,
exchange.budget,
- );
+ ).catch((error) => {
+ console.error('Failed to generate suggestions for member:', giver.user_id, error);
+ });📝 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.
| // Fire and forget suggestions with error handling | |
| // hacky way to avoid waiting for all suggestions to be generated | |
| // avoids timeout issues | |
| generateAndStoreSuggestions( | |
| await generateAndStoreSuggestions( | |
| supabase, | |
| exchangeId, | |
| giver.user_id, | |
| recipient.user_id, | |
| exchange.budget, | |
| ).catch((error) => { | |
| throw new SupabaseError('Failed to generate suggestions', 500, error); | |
| }); | |
| ); | |
| // Fire and forget suggestions with error handling | |
| // hacky way to avoid waiting for all suggestions to be generated | |
| // avoids timeout issues | |
| generateAndStoreSuggestions( | |
| supabase, | |
| exchangeId, | |
| giver.user_id, | |
| recipient.user_id, | |
| exchange.budget, | |
| ).catch((error) => { | |
| console.error('Failed to generate suggestions for member:', giver.user_id, error); | |
| }); |
🤖 Prompt for AI Agents
In lib/drawGiftExchange.ts around lines 94 to 103, the code now awaits
generateAndStoreSuggestions sequentially inside the loop which reintroduces
timeout/latency risk and makes a single suggestion failure abort the whole draw;
change this to a non-blocking approach by either restoring the original
fire-and-forget call (call generateAndStoreSuggestions without await and
capture/log errors inside that function) or run suggestion generation
concurrently and resiliently using Promise.allSettled over an array of
per-member generate calls so the draw completes regardless of individual
failures and errors are logged for debugging.
| // Fire and forget suggestions with error handling | ||
| // hacky way to avoid waiting for all suggestions to be generated | ||
| // avoids timeout issues |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove outdated comments.
These comments reference the previous fire-and-forget implementation but no longer reflect the current sequential await behavior.
- // Fire and forget suggestions with error handling
- // hacky way to avoid waiting for all suggestions to be generated
- // avoids timeout issues
await generateAndStoreSuggestions(📝 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.
| // Fire and forget suggestions with error handling | |
| // hacky way to avoid waiting for all suggestions to be generated | |
| // avoids timeout issues | |
| await generateAndStoreSuggestions( |
🤖 Prompt for AI Agents
In lib/drawGiftExchange.ts around lines 94 to 96, remove the outdated comments
that reference a fire-and-forget implementation and timeout avoidance since the
code now performs sequential awaits; replace or delete those lines so comments
accurately reflect the current sequential behavior (or leave no comment if
unnecessary).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/GiftExchangeHeader/GiftExchangeHeader.tsx (1)
269-269: Fix typo in dialog title."gift exchangee" should be "gift exchange" (remove extra 'e').
-Are you sure you want to start gift exchangee? +Are you sure you want to start gift exchange?
🧹 Nitpick comments (1)
components/GiftExchangeHeader/GiftExchangeHeader.tsx (1)
138-142: Consider toast title and variant choice.The toast has an empty
titleand usesSuccessvariant before the operation completes. Consider:
- Adding a meaningful title like "Drawing in Progress"
- Using a neutral or info variant since the operation hasn't succeeded yet
toast({ - variant: ToastVariants.Success, - title: '', + variant: ToastVariants.Info, + title: 'Drawing in Progress', description: 'Please keep this browser open until our elves complete the gift drawing.', });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/GiftExchangeHeader/GiftExchangeHeader.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/GiftExchangeHeader/GiftExchangeHeader.tsx (2)
hooks/use-toast.ts (2)
useToast(243-267)toast(188-232)components/LoadingSpinner/LoadingSpinner.tsx (1)
LoadingSpinner(3-21)
🔇 Additional comments (5)
components/GiftExchangeHeader/GiftExchangeHeader.tsx (5)
35-37: LGTM: Imports added correctly.The new imports support the loading state UI and toast notifications.
65-66: LGTM: State and hook initialization.The
isDrawingstate andtoasthook are properly initialized for managing the draw process feedback.
164-166: LGTM: State reset in finally block.The
finallyblock correctly ensuresisDrawingis reset regardless of success or failure. This addresses the previous review feedback.
241-249: LGTM: Formatting consistency.Data-testid updated to use double quotes for consistency.
253-259: LGTM: Button state management and loading UI.The button correctly:
- Disables during the drawing process to prevent double-submission
- Shows a loading spinner for visual feedback
- Maintains consistent width with
min-w-40to prevent layout shift
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/GiftExchangeHeader/GiftExchangeHeader.tsx (1)
258-264: LGTM! Loading state implementation is solid.The button correctly:
- Disables during the drawing operation
- Shows a loading spinner while processing
- Maintains consistent width with
min-w-40to prevent layout shiftNote: There's a typo on line 274 in the AlertDialogTitle: "exchangee" should be "exchange". While this is pre-existing, consider fixing it for better UX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/GiftExchangeHeader/GiftExchangeHeader.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/GiftExchangeHeader/GiftExchangeHeader.tsx (2)
hooks/use-toast.ts (2)
useToast(243-267)toast(188-232)components/LoadingSpinner/LoadingSpinner.tsx (1)
LoadingSpinner(3-21)
🔇 Additional comments (4)
components/GiftExchangeHeader/GiftExchangeHeader.tsx (4)
35-37: LGTM! Imports support the new loading and toast functionality.The added imports are correctly used throughout the component for displaying loading states and toast notifications.
65-66: LGTM! State and hook initialization is correct.The
isDrawingstate andtoastfunction are properly initialized and used to manage the drawing operation's UI state.
164-169: LGTM! Error toast provides clear user feedback.The error handling now correctly displays a toast notification to inform users when the draw operation fails, addressing the previous review feedback.
170-170: LGTM! State reset correctly placed in finally block.The
isDrawingstate is now properly reset in the finally block, ensuring the UI returns to the correct state regardless of whether the operation succeeds or fails. This addresses the previous review feedback.
| toast({ | ||
| variant: ToastVariants.Success, | ||
| title: '', | ||
| description: 'Please keep this browser open until our elves complete the gift drawing.', | ||
| }); |
There was a problem hiding this comment.
Consider using a more appropriate toast variant.
The ToastVariants.Success variant is semantically misleading for an in-progress operation. If the API call fails immediately after this toast is shown, users will see a success message followed by an error message, which can be confusing.
Consider using an info or default variant instead, or use a title like "Drawing in Progress" to clarify this is not a success confirmation but an informational message.
Apply this diff to improve clarity:
toast({
- variant: ToastVariants.Success,
- title: '',
+ title: 'Drawing in Progress',
description: 'Please keep this browser open until our elves complete the gift drawing.',
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/GiftExchangeHeader/GiftExchangeHeader.tsx around lines 138 to 142,
the toast currently uses ToastVariants.Success with an empty title for an
in-progress operation; change the variant to an informational one (e.g.,
ToastVariants.Info or the default variant) and add a clear title such as
"Drawing in progress" (or similar) so the toast communicates that the draw is
ongoing rather than a completed success; keep the same description and ensure
the toast is shown as an informational message until the API call resolves.
2 participants
Description
After:
Button disabled for gift drawing now with toast notification
Closes #[ticketnumber]
[optional] Screenshots
Recording.2025-11-19.122011.mp4
Pre-submission checklist
test #001: created unit test for __ component)Peer Code ReviewersandSenior+ Code Reviewersgroupsgis-code-questionsSummary by CodeRabbit
New Features
Bug Fixes
Tests