Yun/student claims - #128
Conversation
📝 WalkthroughWalkthroughThe notification API and UI now support unread-only cursor pagination, stable ordering, load-more behavior, and individual dismissal. Claim selection controls now allow users to clear an already-selected item. ChangesNotification pagination and dismissal
Claim selection toggling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🧹 Nitpick comments (2)
foundit-ui/components/claims/ClaimMatchPanel.tsx (1)
53-56: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression tests for both selection branches.
Test the same-item path, which must call
onSelectItem(null), and the different-item path, which must callonSelectItem(itemId). Also verify that each radio click invokes the callback once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@foundit-ui/components/claims/ClaimMatchPanel.tsx` around lines 53 - 56, Add regression tests for handleSelectItem in ClaimMatchPanel, covering same-item clicks that call onSelectItem(null) and different-item clicks that call onSelectItem(itemId). Verify each radio click invokes the callback exactly once.foundit-ui/components/NotificationCard.tsx (1)
84-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace raw
gray.500/gray.700tokens with thefg.mutedsemantic token across both files. All three sites use ad hoc Chakra palette tokens for secondary/muted text on changed lines, while sibling code in the same files already uses the mandated semantic tokens (fg,fg.error).
foundit-ui/components/NotificationCard.tsx#L84-L107: changecolor={isRead ? 'gray.700' : 'fg'}tocolor={isRead ? 'fg.muted' : 'fg'}for the notification title.foundit-ui/components/NotificationCard.tsx#L116-L141: changecolor="gray.500"tocolor="fg.muted"on both the timestampTextand the dismissIconButton.foundit-ui/components/NotificationFeed.tsx#L98-L106: changecolor="gray.500"tocolor="fg.muted"on the empty-stateText.As per path instructions, "Never hardcode hex colors; use the semantic tokens defined in
components/ui/provider.tsx(fg,fg.muted,fg.error,border.input,border.error,focusRing)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@foundit-ui/components/NotificationCard.tsx` around lines 84 - 107, Replace ad hoc gray palette colors with the fg.muted semantic token at all affected sites: update the notification title in foundit-ui/components/NotificationCard.tsx lines 84-107, both the timestamp Text and dismiss IconButton in foundit-ui/components/NotificationCard.tsx lines 116-141, and the empty-state Text in foundit-ui/components/NotificationFeed.tsx lines 98-106. Preserve the existing fg token for unread title text.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/routes/notifications.ts`:
- Around line 29-50: Update getNotificationListCursorWhere to accept the
requesting recipientId and include it alongside notificationId in the
prisma.notification.findUnique lookup. Pass the caller’s recipientId from the
notification list call site so cursors belonging to other recipients resolve as
absent and return the existing empty boundary.
In `@foundit-ui/components/claims/ClaimManualSearchList.tsx`:
- Around line 61-65: Update the manual-search RadioInput call site in
ClaimManualSearchList so each radio has an accessible, item-specific name
derived from item.title, using an appropriate aria-label or associated labelling
mechanism while preserving the existing selection behavior.
In `@foundit-ui/components/claims/ClaimMatchCard.tsx`:
- Around line 88-92: Update ClaimMatchCard so each match exposes only one
semantic, focusable radio control: either retain the card’s role/tabIndex
behavior and make the nested RadioInput decorative and non-focusable, or use the
native RadioInput as the sole control and remove the card’s radio semantics and
focusability. Preserve selection behavior through onSelect and ensure the
inactive element is excluded from the accessibility tree.
In `@foundit-ui/hooks/useNotifications.ts`:
- Around line 82-104: Update the loadMore flow to track the active unreadOnly
filter in a ref, synchronized whenever the filter changes, and compare that ref
after fetchNotifications resolves before updating notifications, nextCursor, or
unread count. Ignore responses whose filter no longer matches, while preserving
the existing loading-state cleanup and retry behavior.
---
Nitpick comments:
In `@foundit-ui/components/claims/ClaimMatchPanel.tsx`:
- Around line 53-56: Add regression tests for handleSelectItem in
ClaimMatchPanel, covering same-item clicks that call onSelectItem(null) and
different-item clicks that call onSelectItem(itemId). Verify each radio click
invokes the callback exactly once.
In `@foundit-ui/components/NotificationCard.tsx`:
- Around line 84-107: Replace ad hoc gray palette colors with the fg.muted
semantic token at all affected sites: update the notification title in
foundit-ui/components/NotificationCard.tsx lines 84-107, both the timestamp Text
and dismiss IconButton in foundit-ui/components/NotificationCard.tsx lines
116-141, and the empty-state Text in foundit-ui/components/NotificationFeed.tsx
lines 98-106. Preserve the existing fg token for unread title text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 409b2d0c-6152-4163-8d74-7cebe81f29a6
📒 Files selected for processing (10)
backend/src/routes/notifications.tsbackend/src/validators/notifications.tsfoundit-ui/components/NotificationCard.tsxfoundit-ui/components/NotificationFeed.tsxfoundit-ui/components/claims/ClaimManualSearchList.tsxfoundit-ui/components/claims/ClaimMatchCard.tsxfoundit-ui/components/claims/ClaimMatchPanel.tsxfoundit-ui/hooks/useNotifications.tsfoundit-ui/lib/api/notifications.tsfoundit-ui/types/notifications.ts
| async function getNotificationListCursorWhere( | ||
| cursorNotificationId: string | ||
| ): Promise<Prisma.NotificationWhereInput> { | ||
| const cursorNotification = await prisma.notification.findUnique({ | ||
| where: { notificationId: cursorNotificationId }, | ||
| select: { createdAt: true, notificationId: true }, | ||
| }); | ||
|
|
||
| if (!cursorNotification) { | ||
| return {}; | ||
| } | ||
|
|
||
| return { | ||
| OR: [ | ||
| { createdAt: { lt: cursorNotification.createdAt } }, | ||
| { | ||
| createdAt: cursorNotification.createdAt, | ||
| notificationId: { lt: cursorNotification.notificationId }, | ||
| }, | ||
| ], | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope the cursor lookup to the requesting recipient.
getNotificationListCursorWhere resolves cursorNotificationId with prisma.notification.findUnique({ where: { notificationId: cursorNotificationId } }). This lookup does not filter by recipientId. A caller can supply any notification's ID as cursor, including one that belongs to another user, and the function will use that record's createdAt/notificationId to build the pagination boundary for the caller's own query. The final list results stay scoped to the caller through baseWhere, but the endpoint still evaluates cross-tenant data during cursor resolution without an ownership check.
Add the recipientId to the lookup so a cursor can only be resolved against the caller's own notifications.
🔒 Proposed fix to scope the cursor lookup
async function getNotificationListCursorWhere(
- cursorNotificationId: string
+ cursorNotificationId: string,
+ recipientId: string
): Promise<Prisma.NotificationWhereInput> {
- const cursorNotification = await prisma.notification.findUnique({
- where: { notificationId: cursorNotificationId },
+ const cursorNotification = await prisma.notification.findFirst({
+ where: { notificationId: cursorNotificationId, recipientId },
select: { createdAt: true, notificationId: true },
});Update the call site:
const cursorWhere = cursor
- ? await getNotificationListCursorWhere(cursor)
+ ? await getNotificationListCursorWhere(cursor, recipientId)
: {};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/notifications.ts` around lines 29 - 50, Update
getNotificationListCursorWhere to accept the requesting recipientId and include
it alongside notificationId in the prisma.notification.findUnique lookup. Pass
the caller’s recipientId from the notification list call site so cursors
belonging to other recipients resolve as absent and return the existing empty
boundary.
| readOnly | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onSelect(); | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Give the manual-search radio an item-specific accessible name.
The RadioInput is a keyboard focus target, but this call site provides no <label>, aria-label, or aria-labelledby tied to item.title. A screen-reader user cannot identify the item before activating the control.
Suggested fix
<RadioInput
type="radio"
name="manual-item-search"
checked={selected}
readOnly
+ aria-label={item.title}
onClick={(e) => {📝 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.
| readOnly | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| onSelect(); | |
| }} | |
| readOnly | |
| aria-label={item.title} | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| onSelect(); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@foundit-ui/components/claims/ClaimManualSearchList.tsx` around lines 61 - 65,
Update the manual-search RadioInput call site in ClaimManualSearchList so each
radio has an accessible, item-specific name derived from item.title, using an
appropriate aria-label or associated labelling mechanism while preserving the
existing selection behavior.
| readOnly | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| onSelect(); | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 10 'RadioInput|tabIndex|aria-hidden' \
foundit-ui --glob '*.ts' --glob '*.tsx'Repository: 86unj/Foundit
Length of output: 50369
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '--- ClaimMatchCard ---\n'
sed -n '1,120p' foundit-ui/components/claims/ClaimMatchCard.tsx
printf '\n--- ClaimMatchCard usages ---\n'
rg -n -C 4 'ClaimMatchCard' foundit-ui --glob '*.ts' --glob '*.tsx'Repository: 86unj/Foundit
Length of output: 242
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo '--- ClaimMatchCard ---'
sed -n '1,120p' foundit-ui/components/claims/ClaimMatchCard.tsx
echo
echo '--- ClaimMatchCard usages ---'
rg -n -C 4 'ClaimMatchCard' foundit-ui --glob '*.ts' --glob '*.tsx'Repository: 86unj/Foundit
Length of output: 6684
🌐 Web query:
MDN input type radio: keyboard accessibility check state radio group click event tab index
💡 Result:
For native HTML radio buttons (<input type="radio">), keyboard accessibility and group management are handled automatically by the browser, provided the buttons are grouped using the same name attribute [1][2]. Key aspects of radio button accessibility and behavior: 1. Keyboard Navigation (Native): - The Tab key moves focus into the radio group [3]. Specifically, focus lands on the currently checked radio button [3]. If no button is checked, focus lands on the first radio button in the group [3]. - Within the group, the arrow keys (Up/Left and Down/Right) are used to move focus among the radio buttons and automatically select them (changing the checked state) [3]. - Shift + Tab moves focus out of the radio group [3]. 2. Click Events and Labels: - Associating radio buttons with a <label> element is a critical accessibility requirement [2]. Labels increase the clickable area, which is essential for usability, and ensure that clicking the text associated with an option selects the corresponding radio button [1][2]. 3. Custom Implementations (ARIA): - If you are building a custom radio group using non-native elements, you must manage focus and state manually [4][5]. - This typically requires assigning role="radiogroup" to the container and role="radio" to each option [6][4]. - To manage focus within custom groups, developers use techniques like "roving tabindex" (where only the active element has tabindex="0" and others have -1) or aria-activedescendant (where the container holds focus and points to the active child) [7][8]. - For custom radio roles, the Space key is the standard interaction to select the option [4]. It is strongly recommended to use native <input type="radio"> elements whenever possible, as they provide built-in accessibility features that otherwise require complex JavaScript management to replicate [2][5].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/input/radio
- 2: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/input
- 3: https://www.w3.org/WAI/ARIA/apg/patterns/radio/
- 4: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/radio_role
- 5: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Global_attributes/tabindex
- 6: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/radiogroup_role
- 7: https://www.w3.org/WAI/ARIA/apg/patterns/radio/examples/radio/
- 8: https://www.w3.org/WAI/ARIA/apg/patterns/radio/examples/radio-activedescendant/
🌐 Web query:
WAI ARIA radio role role="radio" aria-checked tabindex Space key custom radio checkbox native radio
💡 Result:
When implementing radio functionality, native HTML input elements (type="radio") should always be the first choice [1][2][3]. They provide full, built-in accessibility semantics, keyboard navigation, state management, and form participation without requiring additional ARIA or JavaScript [4][2][3]. If you must implement a custom radio component using ARIA (e.g., due to extreme design constraints), you must manually replicate the behaviors native elements provide for free [1][5][2]. Key Requirements for Custom role="radio" Implementations: 1. Semantics and State: - Use role="radio" for individual options [6][5]. - Enclose all radio elements in a parent container with role="radiogroup" [6][5]. - Provide a label for the group using aria-labelledby referencing a visible label element [7][2]. - Use the aria-checked attribute to communicate state: true for the selected radio, false for unselected [1][6]. Do not use the value mixed, as it is invalid for radio buttons [1][8]. 2. Keyboard Interaction: - Focus Management: Implement a roving tabindex [7][5]. Only one radio in the group should have tabindex="0" (the currently active or checked one), while all other radio elements in the group must have tabindex="-1" [7][9]. - Arrow Keys: Use ArrowUp/ArrowLeft and ArrowDown/ArrowRight to move focus between radios within the group [1][6][10]. Focus should cycle (wrap) or stop at the ends, depending on the implementation pattern, and should update the tabindex [5][10]. - Selection (Space Key): Pressing the Space key must check the focused radio button [1][6]. If it was not already checked, it must set aria-checked="true" on the focused element and aria-checked="false" on the previously checked sibling [1][5]. 3. Common Pitfalls: - Avoiding Conflicts: Never use aria-checked on a native HTML radio input [3][11]. Native elements already communicate their checked state; adding ARIA attributes can cause conflicts or screen reader errors [3][8][11]. - aria-checked vs. aria-selected: Use aria-checked for radio buttons [8][11]. aria-selected is typically reserved for items within a selection container like a tablist, listbox, or tree [12][11]. - Styling: If using native radios, avoid visibility: hidden or display: none, as these remove the element from the tab sequence [4]. Instead, use CSS techniques like appearance: none to customize the look while keeping the element functional and keyboard-accessible [4][13]. In summary, native HTML provides the most robust and accessible experience. Custom ARIA implementations should only be used when native elements cannot meet specific functional requirements, and they require meticulous handling of focus, keyboard events, and state attributes to match native behavior [4][2].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/radio_role
- 2: https://digitalthriveai.com/en-us/resources/docs/ui-ux/radio-role/
- 3: https://opena11y.github.io/evaluation-library/rule-control-14.html
- 4: https://a11y-examples.com/examples/custom-checkboxes-radios/
- 5: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/radiogroup_role
- 6: https://www.w3.org/WAI/ARIA/apg/patterns/radio/
- 7: https://www.w3.org/WAI/ARIA/apg/patterns/radio/examples/radio/
- 8: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-checked
- 9: https://www.w3.org/WAI/ARIA/apg/patterns/radio/examples/radio-rating/
- 10: https://dequeuniversity.com/library/aria/radio-and-radio-group
- 11: https://www.disabilityworld.org/toolkit/standards/aria/checked/
- 12: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-selected
- 13: https://www.scottohara.me/blog/2021/09/24/custom-radio-checkbox-again.html
Keep only one semantic radio control per match.
The card exposes a custom role="radio" and focusable tabIndex={0}, while the nested RadioInput is a native <input type="radio"> with its own tab stop and radio semantics. Use either the card or the native radio, and make the other element non-focusable/removed from the accessibility tree when it is only decorative.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@foundit-ui/components/claims/ClaimMatchCard.tsx` around lines 88 - 92, Update
ClaimMatchCard so each match exposes only one semantic, focusable radio control:
either retain the card’s role/tabIndex behavior and make the nested RadioInput
decorative and non-focusable, or use the native RadioInput as the sole control
and remove the card’s radio semantics and focusability. Preserve selection
behavior through onSelect and ensure the inactive element is excluded from the
accessibility tree.
| }, [loadKey, unreadOnly, updateUnreadCount]); | ||
|
|
||
| const loadMore = useCallback(async () => { | ||
| if (!nextCursor || isLoadingMore) { | ||
| return; | ||
| } | ||
|
|
||
| setIsLoadingMore(true); | ||
| try { | ||
| const data = await fetchNotifications({ | ||
| unreadOnly, | ||
| cursor: nextCursor, | ||
| limit: PAGE_SIZE, | ||
| }); | ||
| setNotifications((prev) => [...prev, ...data.notifications]); | ||
| setNextCursor(data.nextCursor); | ||
| updateUnreadCount(data.unreadCount); | ||
| } catch { | ||
| // Keep the loaded page; user can retry Load more. | ||
| } finally { | ||
| setIsLoadingMore(false); | ||
| } | ||
| }, [nextCursor, isLoadingMore, unreadOnly, updateUnreadCount]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard loadMore against a stale response when the unread filter changes mid-flight.
If the user calls loadMore and then toggles the unread filter before that request resolves, the load effect (Line 82) resets notifications and nextCursor for the new filter, but the stale loadMore response still lands afterward and unconditionally appends its (now mismatched-filter) items onto the fresh list, and overwrites nextCursor with a stale value. The visible feed then mixes items from two different filters and paginates from the wrong cursor.
Track the active filter in a ref and drop the response if it changed before applying it.
🛡 Proposed fix to ignore stale `loadMore` responses
+ const unreadOnlyRef = useRef(unreadOnly);
+ unreadOnlyRef.current = unreadOnly;
+
const loadMore = useCallback(async () => {
if (!nextCursor || isLoadingMore) {
return;
}
setIsLoadingMore(true);
+ const requestedUnreadOnly = unreadOnly;
try {
const data = await fetchNotifications({
unreadOnly,
cursor: nextCursor,
limit: PAGE_SIZE,
});
+ if (unreadOnlyRef.current !== requestedUnreadOnly) {
+ return;
+ }
setNotifications((prev) => [...prev, ...data.notifications]);
setNextCursor(data.nextCursor);
updateUnreadCount(data.unreadCount);
} catch {
// Keep the loaded page; user can retry Load more.
} finally {
setIsLoadingMore(false);
}
}, [nextCursor, isLoadingMore, unreadOnly, updateUnreadCount]);📝 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.
| }, [loadKey, unreadOnly, updateUnreadCount]); | |
| const loadMore = useCallback(async () => { | |
| if (!nextCursor || isLoadingMore) { | |
| return; | |
| } | |
| setIsLoadingMore(true); | |
| try { | |
| const data = await fetchNotifications({ | |
| unreadOnly, | |
| cursor: nextCursor, | |
| limit: PAGE_SIZE, | |
| }); | |
| setNotifications((prev) => [...prev, ...data.notifications]); | |
| setNextCursor(data.nextCursor); | |
| updateUnreadCount(data.unreadCount); | |
| } catch { | |
| // Keep the loaded page; user can retry Load more. | |
| } finally { | |
| setIsLoadingMore(false); | |
| } | |
| }, [nextCursor, isLoadingMore, unreadOnly, updateUnreadCount]); | |
| }, [loadKey, unreadOnly, updateUnreadCount]); | |
| const unreadOnlyRef = useRef(unreadOnly); | |
| unreadOnlyRef.current = unreadOnly; | |
| const loadMore = useCallback(async () => { | |
| if (!nextCursor || isLoadingMore) { | |
| return; | |
| } | |
| setIsLoadingMore(true); | |
| const requestedUnreadOnly = unreadOnly; | |
| try { | |
| const data = await fetchNotifications({ | |
| unreadOnly, | |
| cursor: nextCursor, | |
| limit: PAGE_SIZE, | |
| }); | |
| if (unreadOnlyRef.current !== requestedUnreadOnly) { | |
| return; | |
| } | |
| setNotifications((prev) => [...prev, ...data.notifications]); | |
| setNextCursor(data.nextCursor); | |
| updateUnreadCount(data.unreadCount); | |
| } catch { | |
| // Keep the loaded page; user can retry Load more. | |
| } finally { | |
| setIsLoadingMore(false); | |
| } | |
| }, [nextCursor, isLoadingMore, unreadOnly, updateUnreadCount]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@foundit-ui/hooks/useNotifications.ts` around lines 82 - 104, Update the
loadMore flow to track the active unreadOnly filter in a ref, synchronized
whenever the filter changes, and compare that ref after fetchNotifications
resolves before updating notifications, nextCursor, or unread count. Ignore
responses whose filter no longer matches, while preserving the existing
loading-state cleanup and retry behavior.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes