-
Notifications
You must be signed in to change notification settings - Fork 4
fix #591: Limit draw notifications to group owner #683
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
base: develop
Are you sure you want to change the base?
Changes from all commits
4846ce9
14cf886
531ea4d
01566ab
fc1a5c7
66da3e1
34e36d3
07f8244
263c2d6
5660471
39d91da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,17 +11,27 @@ import { ToastClose } from "../ToastClose/ToastClose" | |||||||||||||||||||
| import { ToastViewport } from "../ToastViewport/ToastViewport" | ||||||||||||||||||||
| import { ToastProvider } from "../ToastProvider/ToastProvider" | ||||||||||||||||||||
| import { JSX } from "react" | ||||||||||||||||||||
| import { useAuthContext } from '@/context/AuthContextProvider'; | ||||||||||||||||||||
| import useExchangeGroups from "@/hooks/useExchangeGroups"; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Renders and manages display of toast notifications. | ||||||||||||||||||||
| * @returns {JSX.Element} The rendered toast notification. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| const Toaster = (): JSX.Element => { | ||||||||||||||||||||
| const { toasts } = useToast() | ||||||||||||||||||||
| const { session } = useAuthContext() | ||||||||||||||||||||
| const userExchangeGroups = useExchangeGroups() | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const filteredToasts = toasts.filter((toast) => { | ||||||||||||||||||||
| return userExchangeGroups.some((group) => { | ||||||||||||||||||||
| return (group.gift_exchange_id === toast.group && group.owner_id === session?.user.id) | ||||||||||||||||||||
| }) | ||||||||||||||||||||
|
Comment on lines
+27
to
+29
|
||||||||||||||||||||
| 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 | |
| )); |
Copilot
AI
Nov 13, 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.
[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])| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,45 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Copyright (c) Gridiron Survivor. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Licensed under the MIT License. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useState, useEffect } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { GiftExchangeWithMemberCount } from '../app/types/giftExchange'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * A React hook that manages the current users exchange groups. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * A React hook that manages the current users exchange groups. | |
| * A React hook that manages the current user's exchange groups. |
Copilot
AI
Nov 13, 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.
[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 };
};
Copilot
AI
Nov 13, 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.
Remove the unnecessary semicolon after the closing brace. Function declarations don't require a semicolon.
| }; | |
| } |
Copilot
AI
Nov 13, 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.
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; | |
| }; |
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.
Avoid automated semicolon insertion (90% of all statements in the enclosing script have an explicit semicolon).