Notification admin UI and filters #310
Conversation
|
@pixelsama is attempting to deploy a commit to the zhang's projects Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
bot
added
the
size:XXL
Summary of ChangesHello @pixelsama, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the notification system by providing robust management tools for administrators and improved user experience for end-users. Administrators can now easily create, modify, and remove notifications through a dedicated UI, while users benefit from advanced filtering and sorting options to better manage their notification feed. The changes also ensure the notification interface is fully responsive and localized for a global audience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
dosubot
bot
added
area:ui
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive admin UI for managing notifications, including CRUD operations and bulk actions. It also enhances the user-facing notifications page with filtering, sorting, and search capabilities. The changes are well-structured, with new components for the admin form and pages. My review focuses on improving the data fetching logic for consistency and performance, avoiding redundant operations, and ensuring backend search capabilities match the frontend's intent. I've identified a couple of bugs related to data fetching that could lead to stale data or unnecessary API calls, and suggested ways to align the implementation with best practices already present in other parts of the codebase.
app/admin/notifications/page.tsx
Outdated
| useEffect(() => { | ||
| void fetchData(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
This useEffect hook is configured to run only on component mount due to its empty dependency array. However, the fetchData function it calls depends on the sortBy and sortOrder state variables. When a user changes the sort order, fetchData is not re-executed, leading to a disconnect between the UI controls and the data fetched from the server. The client-side sorting then re-sorts the existing data, which is inefficient.
To fix this, you should include sortBy and sortOrder in the dependency array to ensure data is refetched when sorting options change. It would also be beneficial to wrap fetchData in a useCallback hook.
| useEffect(() => { | |
| void fetchData(); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, []); | |
| useEffect(() => { | |
| void fetchData(); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [sortBy, sortOrder]); |
app/notifications/page.tsx
Outdated
| const handleIncludeReadToggle = useCallback( | ||
| (checked: boolean) => { | ||
| setIncludeRead(checked); | ||
| // When turning off includeRead, ensure we re-fetch unread-only | ||
| void fetchNotifications({ | ||
| search: search.trim() || undefined, | ||
| sort_by: sortBy, | ||
| sort_order: sortOrder, | ||
| include_read: checked, | ||
| }); | ||
| }, | ||
| [fetchNotifications, search, sortBy, sortOrder] | ||
| ); |
There was a problem hiding this comment.
This handleIncludeReadToggle function explicitly calls fetchNotifications. However, the useEffect hook at lines 57-68 already triggers fetchNotifications whenever includeRead changes. This results in a redundant API call, as fetchNotifications will be called once from here and then again (after a 200ms delay) from the useEffect. You should remove the explicit call to fetchNotifications from this useCallback to avoid the double fetch.
const handleIncludeReadToggle = useCallback(
(checked: boolean) => {
setIncludeRead(checked);
},
[]
);
| const [form, setForm] = useState({ | ||
| type: 'message' as NotificationType, | ||
| category: '', | ||
| title: '', | ||
| content: '', | ||
| priority: 'medium' as NotificationPriority, | ||
| published: false, | ||
| }); |
There was a problem hiding this comment.
The initial state for the form is defined here and then hardcoded again in the handleSubmit function (lines 108-115) to reset the form after creation. To avoid this duplication and improve maintainability, you could define the initial state in a constant outside the component and reuse it in both places.
For example:
const INITIAL_FORM_STATE = {
type: 'message' as NotificationType,
category: '',
title: '',
content: '',
priority: 'medium' as NotificationPriority,
published: false,
};
export function NotificationForm(...) {
// ...
const [form, setForm] = useState(INITIAL_FORM_STATE);
// ...
const handleSubmit = async (e: React.FormEvent) => {
// ...
if (mode === 'create') {
// ...
setForm(INITIAL_FORM_STATE);
}
// ...
}
}
lib/db/notification-center.ts
Outdated
|
|
||
| if (params.search) { | ||
| const term = `%${params.search}%`; | ||
| query = query.or(`title.ilike.${term},content.ilike.${term}`); |
There was a problem hiding this comment.
The search functionality on the frontend includes searching by category. To ensure consistent behavior and support a full server-side search, the category field should also be included in this search query. The category column is nullable, but ilike on a null value will evaluate to false in a WHERE clause, which is the desired behavior here.
| query = query.or(`title.ilike.${term},content.ilike.${term}`); | |
| query = query.or(`title.ilike.${term},content.ilike.${term},category.ilike.${term}`); |
lib/db/notification-center.ts
Outdated
|
|
||
| if (params.search) { | ||
| const term = `%${params.search}%`; | ||
| query = query.or(`title.ilike.${term},content.ilike.${term}`); |
There was a problem hiding this comment.
The client-side filtering in app/admin/notifications/page.tsx also searches within the category field. To make server-side searching consistent and to enable moving all filtering logic to the server, you should also include the category field in this OR condition. The category column is nullable, but ilike on a null value will evaluate to false in a WHERE clause, which is the desired behavior here.
| query = query.or(`title.ilike.${term},content.ilike.${term}`); | |
| query = query.or(`title.ilike.${term},content.ilike.${term},category.ilike.${term}`); |
1 participant
Important
Fixes #<issue number>What & Why
What: Added user notifications page filters/search/sort/unread toggle, admin notifications CRUD UI (list + bulk delete + create/edit form), mobile sidebar layout for notifications, and full i18n coverage for notification pages/forms.
Why: Completes the minimal notification center UX (user filters, admin management) and aligns with multilingual UI expectations.
Fixes #N/A
Pre-PR Checklist
Run these:
pnpm type-checkpnpm format:checkpnpm lintpnpm buildpnpm i18n:check(if applicable)Type
Screenshots (if UI changes)
N/A