fix #591: Limit draw notifications to group owner#683
fix #591: Limit draw notifications to group owner#683
Conversation
…anta into jeremy/591-limit-draw-notifications-groupOwner
…s://github.com/LetsGetTechnical/elecretanta into jeremy/591-limit-draw-notifications-groupOwner
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
shashilo
requested review from
a team and
shashilo
and removed request for
a team
jefische
changed the title
jefische
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #591 by filtering toast notifications to only show those for groups where the logged-in user is the owner. Previously, all group members received draw-related notifications, but only owners could act on them.
Key Changes:
- Added
useExchangeGroupshook to fetch user's gift exchange groups - Modified
Toastercomponent to filter toasts based on group ownership - Moved
<Toaster />insideAuthContextProviderin layout to enable access to session data
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| hooks/useExchangeGroups.ts | New custom hook that fetches and manages the current user's gift exchange groups |
| components/Toaster/Toaster.tsx | Added filtering logic to show toasts only for groups where user is owner; requires session and exchange group data |
| components/Toaster/Toaster.test.tsx | Updated tests with mocks for new dependencies and added group filtering test scenarios |
| app/layout.tsx | Moved Toaster component inside AuthContextProvider to enable access to session context |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function fetchGiftExchanges() { | ||
| try { | ||
| const response = await fetch(`/api/gift-exchanges`, { | ||
| method: 'GET', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`HTTP error! status: ${response.status}`); | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| setGiftExchanges(data); | ||
|
|
||
| } catch (error) { | ||
| console.error('Failed to fetch gift exchanges:', error); | ||
| } | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| fetchGiftExchanges(); |
There was a problem hiding this comment.
The useExchangeGroups hook may cause race conditions when the component unmounts before the fetch completes. Consider adding cleanup logic to prevent state updates on unmounted components:
useEffect(() => {
let isMounted = true;
async function fetchGiftExchanges() {
try {
const response = await fetch(`/api/gift-exchanges`, {
method: 'GET',
headers: {
'Content-Type': 'application/json',
},
});
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`);
}
const data = await response.json();
if (isMounted) {
setGiftExchanges(data);
}
} catch (error) {
console.error('Failed to fetch gift exchanges:', error);
}
}
fetchGiftExchanges();
return () => {
isMounted = false;
};
}, []);| async function fetchGiftExchanges() { | |
| try { | |
| const response = await fetch(`/api/gift-exchanges`, { | |
| method: 'GET', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| if (!response.ok) { | |
| throw new Error(`HTTP error! status: ${response.status}`); | |
| } | |
| const data = await response.json(); | |
| setGiftExchanges(data); | |
| } catch (error) { | |
| console.error('Failed to fetch gift exchanges:', error); | |
| } | |
| }; | |
| useEffect(() => { | |
| fetchGiftExchanges(); | |
| useEffect(() => { | |
| let isMounted = true; | |
| async function fetchGiftExchanges() { | |
| try { | |
| const response = await fetch(`/api/gift-exchanges`, { | |
| method: 'GET', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| if (!response.ok) { | |
| throw new Error(`HTTP error! status: ${response.status}`); | |
| } | |
| const data = await response.json(); | |
| if (isMounted) { | |
| setGiftExchanges(data); | |
| } | |
| } catch (error) { | |
| console.error('Failed to fetch gift exchanges:', error); | |
| } | |
| } | |
| fetchGiftExchanges(); | |
| return () => { | |
| isMounted = false; | |
| }; |
| const filteredToasts = toasts.filter((toast) => { | ||
| return userExchangeGroups.some((group) => { | ||
| return (group.gift_exchange_id === toast.group && group.owner_id === session?.user.id) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
[nitpick] The filtering logic runs on every render of the Toaster component. Since userExchangeGroups is fetched asynchronously and could change, consider memoizing the filtered result to avoid unnecessary recomputations:
const filteredToasts = useMemo(() => {
return toasts.filter((toast) => {
// Show toasts without a group (global toasts)
if (!toast.group) return true;
// For toasts with a group, only show if user is the owner
return userExchangeGroups.some((group) => {
return (group.gift_exchange_id === toast.group && group.owner_id === session?.user.id)
})
})
}, [toasts, userExchangeGroups, session?.user.id])| import { GiftExchangeWithMemberCount } from '../app/types/giftExchange'; | ||
|
|
||
| /** | ||
| * A React hook that manages the current users exchange groups. |
There was a problem hiding this comment.
The JSDoc comment has a grammatical error. Change "manages the current users exchange groups" to "manages the current user's exchange groups" (add apostrophe for possessive).
| * A React hook that manages the current users exchange groups. | |
| * A React hook that manages the current user's exchange groups. |
| } catch (error) { | ||
| console.error('Failed to fetch gift exchanges:', error); | ||
| } |
There was a problem hiding this comment.
[nitpick] The error handling doesn't provide any feedback to the user when the fetch fails. The hook silently fails and returns an empty array, which could be confusing. Consider:
- Adding a loading state to distinguish between "no data" and "loading"
- Adding an error state to handle fetch failures
- Or at minimum, logging more context about the error
Example:
const useExchangeGroups = () => {
const [giftExchanges, setGiftExchanges] = useState<GiftExchangeWithMemberCount[]>([]);
const [isLoading, setIsLoading] = useState(true);
const [error, setError] = useState<Error | null>(null);
async function fetchGiftExchanges() {
try {
setIsLoading(true);
const response = await fetch(`/api/gift-exchanges`, {
method: 'GET',
headers: {
'Content-Type': 'application/json',
},
});
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`);
}
const data = await response.json();
setGiftExchanges(data);
setError(null);
} catch (error) {
console.error('Failed to fetch gift exchanges:', error);
setError(error instanceof Error ? error : new Error('Unknown error'));
} finally {
setIsLoading(false);
}
}
// ...
return { giftExchanges, isLoading, error };
};| } catch (error) { | ||
| console.error('Failed to fetch gift exchanges:', error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Remove the unnecessary semicolon after the closing brace. Function declarations don't require a semicolon.
| }; | |
| } |
| return userExchangeGroups.some((group) => { | ||
| return (group.gift_exchange_id === toast.group && group.owner_id === session?.user.id) | ||
| }) |
There was a problem hiding this comment.
The filtering logic will hide all toasts that don't have a group property. This breaks existing toasts like TOASTS.badLinkToast, TOASTS.expiredLinkToast, and TOASTS.errorToast which are used in the codebase but don't include a group field.
Consider updating the filter to show toasts without a group property, or only filter toasts that explicitly have a group property:
const filteredToasts = toasts.filter((toast) => {
// Show toasts without a group (global toasts)
if (!toast.group) return true;
// For toasts with a group, only show if user is the owner
return userExchangeGroups.some((group) => {
return (group.gift_exchange_id === toast.group && group.owner_id === session?.user.id)
})
})| return userExchangeGroups.some((group) => { | |
| return (group.gift_exchange_id === toast.group && group.owner_id === session?.user.id) | |
| }) | |
| // Show toasts without a group (global toasts) | |
| if (!toast.group) return true; | |
| // For toasts with a group, only show if user is the owner | |
| return userExchangeGroups.some((group) => ( | |
| group.gift_exchange_id === toast.group && group.owner_id === session?.user.id | |
| )); |
| gift_exchange_id: 'test-group-2', | ||
| owner_id: 'test-member-2', | ||
| }, | ||
| ] |
There was a problem hiding this comment.
Avoid automated semicolon insertion (90% of all statements in the enclosing script have an explicit semicolon).
| ] | |
| ]; |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe Toaster component is repositioned within the AuthContextProvider to access authentication context. A new hook fetches the user's exchange groups. Toaster filtering logic now displays only toasts belonging to groups owned by the current user, implementing ownership-based notification scoping. Changes
Sequence DiagramsequenceDiagram
participant App as Root Layout
participant Auth as AuthContextProvider
participant Toast as Toaster Component
participant UseExchangeGroups as useExchangeGroups Hook
participant API as /api/gift-exchanges
App->>Auth: Wrap content
Auth->>Toast: Render (inside provider)
Toast->>UseExchangeGroups: Call hook to get groups
UseExchangeGroups->>API: Fetch exchange groups
API-->>UseExchangeGroups: Return groups
UseExchangeGroups-->>Toast: Return groups array
Note over Toast: For each toast:<br/>Check if group owner_id<br/>matches current user_id
Toast->>Toast: Filter toasts by ownership
Toast-->>App: Render filtered toasts only
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧬 Code graph analysis (2)hooks/useExchangeGroups.ts (2)
components/Toaster/Toaster.tsx (1)
🪛 ESLinthooks/useExchangeGroups.ts[error] 12-13: Missing JSDoc comment. (jsdoc/require-jsdoc) [error] 17-18: Missing JSDoc comment. (jsdoc/require-jsdoc) [error] 17-17: Missing return type on function. (@typescript-eslint/explicit-function-return-type) [error] 22-22: Expected indentation of 10 spaces but found 8. (indent) [error] 23-23: Expected indentation of 8 spaces but found 6. (indent) 🔇 Additional comments (5)
Comment |
Description
Before:
Previously any group that a user was part of, that user would receive toast notifications related to the drawing which only the owner can handle.
After:
All toasts for groups that pertain to the logged-in user are now filtered. Toasts will only display when the group owner id matches the session user id.
Closes #591
Testing instructions
Join groups for which you are not the owner, and also create groups for which you are the owner. Trigger a toast such as
Draw date has passedorDraw Todayto see the toasts only for the groups that you are the owner.Screenshot of dashboard with error toast messages
Pre-submission checklist
test #001: created unit test for __ component)Peer Code ReviewersandSenior+ Code Reviewersgroupsgis-code-questionsSummary by CodeRabbit
Bug Fixes
Infrastructure