Notifications v2 #1636
Notifications v2 #1636
Conversation
WalkthroughAdds three subscription fields to PreferencesForm and corresponding UI/tests; introduces category filtering/display in ManageNotifications and an import fix; adds urgent notifications fetch, UI panel, and read handler plus header/menu restructuring; expands unit tests for Header, ManageNotifications, and PreferencesForm. Changes
Sequence Diagram(s)sequenceDiagram
participant HeaderUI as Header UI
participant SWR as SWR(urgentNotifications)
participant API as HTTP API
HeaderUI->>SWR: useSWR('/notifications?urgent=true')
SWR-->>HeaderUI: urgentNotifications.res
alt urgent notifications exist
HeaderUI->>HeaderUI: Render urgent panel (message + close)
HeaderUI->>API: PUT /notifications/{id}/read
API-->>HeaderUI: 200 OK
HeaderUI->>SWR: mutate('/notifications?urgent=true')
end
sequenceDiagram
participant UI as ManageNotifications UI
participant SWR as SWR(notifications)
participant API as HTTP API
UI->>UI: User selects category
UI->>SWR: useSWR('/notifications?page=0&category=<value>')
SWR->>API: GET /notifications?page=0&category=<value>
API-->>SWR: { res: [...], total: N }
SWR-->>UI: Filtered notifications
UI->>UI: Reset page to 0 and render results (Category shown per row)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Notifications/PreferencesForm.tsx (2)
49-68: Consider refactoring to reduce code duplication.The initialization logic is correct and follows the existing pattern. However, consider extracting this repetitive code into a more maintainable approach.
- setValue( - 'subscribe_incident_notifications', - preferences.res.subscribe_incident_notifications || false - ); - setValue( - 'subscribe_schema_change_notifications', - preferences.res.subscribe_schema_change_notifications || false - ); - setValue( - 'subscribe_job_failure_notifications', - preferences.res.subscribe_job_failure_notifications || false - ); - setValue( - 'subscribe_late_runs_notifications', - preferences.res.subscribe_late_runs_notifications || false - ); - setValue( - 'subscribe_dbt_test_failure_notifications', - preferences.res.subscribe_dbt_test_failure_notifications || false - ); + const subscriptionFields = [ + 'subscribe_incident_notifications', + 'subscribe_schema_change_notifications', + 'subscribe_job_failure_notifications', + 'subscribe_late_runs_notifications', + 'subscribe_dbt_test_failure_notifications', + ] as const; + + subscriptionFields.forEach(field => { + setValue(field, preferences.res[field] || false); + });
142-199: UI implementation looks good with opportunity for refactoring.The category subscription switches are well-implemented and follow existing patterns. The section organization and labeling are clear and user-friendly.
Consider refactoring the repetitive Controller components:
const subscriptionOptions = [ { name: 'subscribe_incident_notifications', label: 'Incident Notifications' }, { name: 'subscribe_schema_change_notifications', label: 'Schema Change Notifications' }, { name: 'subscribe_job_failure_notifications', label: 'Job Failure Notifications' }, { name: 'subscribe_late_runs_notifications', label: 'Late Runs Notifications' }, { name: 'subscribe_dbt_test_failure_notifications', label: 'dbt Test Failure Notifications' }, ] as const; // Then render them: {subscriptionOptions.map(({ name, label }) => ( <Controller key={name} name={name} control={control} render={({ field }) => ( <FormControlLabel control={<Switch {...field} checked={field.value} />} label={label} /> )} /> ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Notifications/ManageNotificaitons.tsx(4 hunks)src/components/Notifications/PreferencesForm.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: in the formfield component (src/helpers/connectorconfig/formfield.tsx), the additem function for arr...
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1550
File: src/helpers/connectorConfig/FormField.tsx:343-359
Timestamp: 2025-06-17T03:30:42.337Z
Learning: In the FormField component (src/helpers/connectorConfig/FormField.tsx), the addItem function for array fields only initializes explicit default values for top-level subFields, but nested required fields are correctly rendered dynamically in the UI at render time. This approach works properly for complex configurations like S3 buckets, as confirmed by user testing.
Applied to files:
src/components/Notifications/PreferencesForm.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks
🔇 Additional comments (6)
src/components/Notifications/PreferencesForm.tsx (2)
22-26: LGTM! Type definitions are well-structured.The new boolean fields for category subscriptions follow a consistent naming convention and integrate well with the existing type structure.
211-215: LGTM! Form submission correctly includes all new fields.The new subscription preferences are properly included in the payload and sent to the appropriate endpoint.
src/components/Notifications/ManageNotificaitons.tsx (4)
17-19: LGTM! Appropriate imports for the dropdown filter.The new MUI component imports are necessary and correctly added for the category filter functionality.
59-64: LGTM! State management and query building are well-implemented.The selectedCategory state and categoryQuery building follow the same clean pattern as the existing readQuery implementation. The SWR integration is correct.
134-168: Excellent filter UI implementation.The category filter dropdown is well-implemented with:
- Clear labeling and intuitive placement
- Proper state management and page reset on filter change
- Consistent styling that matches the component's design
- Good accessibility with proper form controls
- Clean mapping over category options
32-39: Category options are well-definedThe
categoryOptionsarray is clear, with user-friendly labels and a sensible “All Categories” fallback.Please verify that these
valuestrings—
''incidentschema_changejob_failurelate_rundbt_test_failure—exactly match the backend’s expected
categoryfilter parameters and correctly map to the subscription fields inPreferencesForm(e.g.subscribe_incident_notifications,subscribe_late_runs_notifications, etc.).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/components/Header/Header.tsx (4)
1-1: Remove unused imports
CollapseandSlideare imported but never used in the component.-import { Box, Menu, MenuItem, Paper, Typography, IconButton, Collapse, Slide } from '@mui/material'; +import { Box, Menu, MenuItem, Paper, Typography, IconButton } from '@mui/material';
88-97: Improve error handling with user feedbackThe error is only logged to console. Consider showing a user-facing error message when dismissal fails.
Consider using a toast notification or error state to inform users when dismissal fails:
const handleDismissUrgent = async (notificationId: string) => { try { await httpPost(session, 'notifications/urgent/dismiss', { notification_id: notificationId, }); mutateUrgent(); // Refresh the list after dismissing } catch (err) { console.error('Failed to dismiss urgent notification:', err); + // Consider adding user-facing error handling + // e.g., showErrorToast('Failed to dismiss notification. Please try again.'); } };
252-275: Simplify redundant permission checksThe org creation permission is checked twice - once with
session?.user?.can_create_orgsand again withpermissions.includes('can_create_org'). This creates unnecessary nesting.- {session?.user?.can_create_orgs && ( + {session?.user?.can_create_orgs && permissions.includes('can_create_org') && ( <Box sx={{ my: 0, py: 1, px: 2, ':hover': { cursor: 'pointer' }, }} onClick={handleCreateOrgClick} > - {permissions.includes('can_create_org') && ( <Typography variant="subtitle1" sx={{ fontWeight: 600, borderBottom: '0.5px solid rgba(15, 36, 64, 0.5)', }} data-testid="createneworg" > Create new org </Typography> - )} </Box> )}
337-357: Add proper TypeScript typing for notificationsThe urgent notifications are typed as
any. Consider adding proper type definitions for better type safety.Define a proper type for urgent notifications:
type UrgentNotification = { id: string; message: string; // add other relevant fields };Then update the map:
- {urgentNotifications?.res.map((msg: any) => ( + {urgentNotifications?.res.map((msg: UrgentNotification) => (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Header/Header.tsx(5 hunks)src/components/Notifications/ManageNotificaitons.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Notifications/ManageNotificaitons.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/Header/Header.tsx (2)
src/helpers/http.tsx (1)
httpPost(34-55)src/pages/api/auth/[...nextauth].ts (1)
session(8-18)
🔇 Additional comments (4)
src/components/Header/Header.tsx (4)
59-60: LGTM!Good use of SWR for fetching urgent notifications with proper mutate function for refreshing.
169-184: Verify hamburger menu rotation logicThe rotation logic appears inverted - the icon rotates 180° when the menu is closed (
!openMenu). Typically, hamburger icons rotate when opening, not when closed.Please verify if this is the intended behavior. If not, consider:
- ...(!openMenu && { + ...(openMenu && { transform: 'rotate(180deg)', }),
198-205: LGTM!Good implementation of dynamic notification icon based on unread count.
283-296: LGTM!Good implementation with locale-aware alphabetical sorting of organizations.
Multiple messages. @fatchat, @Ishankoradia , @himanshudube97 How's this?....for urgent notification. |
|
i like it |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Header/Header.tsx (1)
322-380: Icon placement issue from previous review has been resolved.The urgent notifications display is well-implemented with proper positioning and styling. The ErrorOutline icon is now correctly placed outside the Typography component in a flex container, addressing the previous review concern.
The previous review comment about icon placement has been addressed in the current implementation.
🧹 Nitpick comments (1)
src/components/Header/Header.tsx (1)
88-97: Good implementation with proper error handling.The dismiss handler correctly uses async/await, includes proper error handling, and invalidates the cache after dismissing notifications.
Consider adding a more specific type for the notification ID parameter:
-const handleDismissUrgent = async (notificationId: string) => { +const handleDismissUrgent = async (notificationId: string) => {Actually, the current typing is appropriate for this use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Header/Header.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks
🔇 Additional comments (4)
src/components/Header/Header.tsx (4)
13-13: LGTM on imports and SWR hook setup!The new imports for
httpPost,ErrorOutline, andCloseIconare appropriate for the urgent notifications functionality. The SWR hook setup follows the existing pattern and correctly extracts the mutate function for cache invalidation.Also applies to: 20-20, 59-59
165-213: Excellent header layout improvements!The new layout structure provides better visual hierarchy and user experience:
- Smooth hamburger menu rotation animation
- Proper flexbox alignment and spacing
- Conditional notification icon based on unread count
- Clean organization of header elements
214-315: Well-structured user menu with excellent UX improvements.The enhanced menu provides clear user identity, proper permission-based features, and improved organization management:
- User email displayed prominently
- Alphabetical organization sorting
- Conditional org creation based on permissions
- Clean visual separators and styling
165-382: Excellent integration of notifications v2 features.The Header component successfully integrates urgent notifications functionality while maintaining clean code structure and good user experience. The implementation aligns well with the PR objectives and follows React best practices.
@MohitAgrawal16 as discussed we should have the notification span the entire width of the header and just below it. |
@Ishankoradia, This is better than what we finalized. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/Header/Header.tsx (1)
357-370: Good improvements to icon placementThe updated implementation properly addresses the previous alignment issue by using a flex container with the icon and text as separate elements, which ensures consistent visual alignment.
🧹 Nitpick comments (2)
src/components/Header/Header.tsx (2)
253-276: Permission check might be redundantBoth
session?.user?.can_create_orgs(Line 253) andpermissions.includes('can_create_org')(Line 263) are checked. This appears redundant and could lead to confusion if they diverge.Consider using a single source of truth for this permission:
-{session?.user?.can_create_orgs && ( +{permissions.includes('can_create_org') && ( <Box sx={{ my: 0, py: 1, px: 2, ':hover': { cursor: 'pointer' }, }} onClick={handleCreateOrgClick} > - {permissions.includes('can_create_org') && ( <Typography variant="subtitle1" sx={{ fontWeight: 600, borderBottom: '0.5px solid rgba(15, 36, 64, 0.5)', }} data-testid="createneworg" > Create new org </Typography> - )} </Box> -)} +)}
278-298: Potential performance issue with large organization listsThe organization list uses
overflow: scrollwith a max height, but renders all organizations at once. For users with many organizations, this could impact performance.Consider implementing virtualization for better performance with large lists:
+import { FixedSizeList } from 'react-window'; <Box sx={{ - overflow: 'scroll', maxHeight: '60vh', }} > - <Box> - {orgs - .sort((org1, org2) => org1['label'].localeCompare(org2['label'])) - .map((org: AutoCompleteOption, idx: number) => ( - <MenuItem - key={idx} - value={org.id} - onClick={() => setSelectedOrg(org)} - selected={selectedOrg?.id === org.id} - sx={selectedOrg?.id === org.id ? { fontWeight: 600 } : {}} - > - {org.label} - </MenuItem> - ))} - </Box> + <FixedSizeList + height={Math.min(orgs.length * 36, 60 * 16)} // 36px per item, max 60vh + width="100%" + itemCount={orgs.length} + itemSize={36} + > + {({ index, style }) => { + const org = orgs.sort((a, b) => a.label.localeCompare(b.label))[index]; + return ( + <MenuItem + style={style} + key={org.id} + value={org.id} + onClick={() => setSelectedOrg(org)} + selected={selectedOrg?.id === org.id} + sx={selectedOrg?.id === org.id ? { fontWeight: 600 } : {}} + > + {org.label} + </MenuItem> + ); + }} + </FixedSizeList> </Box>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Header/Header.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/Header/Header.tsx (3)
src/helpers/http.tsx (1)
httpPut(57-78)src/pages/api/auth/[...nextauth].ts (1)
session(8-18)src/components/Org/CreateOrgForm.tsx (1)
CreateOrgForm(31-395)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks
🔇 Additional comments (1)
src/components/Header/Header.tsx (1)
323-381: Consider addressing UI feedback for urgent notificationsBased on PR comments, Ishankoradia requested that urgent notifications span the entire width of the header and appear directly below it. The current implementation uses a fixed-position centered box instead.
Please verify whether the current implementation aligns with the agreed-upon design. If the full-width layout is still required, consider this adjustment:
{urgentNotifications?.res?.length > 0 && ( <Box sx={{ - position: 'fixed', - top: '80px', - left: '50%', - transform: 'translateX(-50%)', + position: 'fixed', + top: '64px', // Adjust based on header height + left: 0, + right: 0, + width: '100%', backgroundColor: '#ffffff', border: '1px solid #FFCDD2', - borderRadius: '8px', + borderRadius: 0, boxShadow: '0px 2px 8px rgba(0,0,0,0.15)', zIndex: 1300, - minWidth: '300px', - maxWidth: '600px', display: 'flex', flexDirection: 'column', gap: '8px', padding: '10px 12px', }} >
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Creating a local commit for the generated unit tests... The unit tests have been committed to the current branch. Commit ID: |
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (3)
src/components/Notifications/__tests__/PreferencesForm.test.tsx (1)
100-116: Include category subscription fields in userpreferences payload assertionsThe PR description/implementation adds subscription booleans to PreferencesForm. The test currently asserts only
enable_email_notifications. Strengthen this by asserting the presence of the newsubscribe_*fields to prevent regressions.Apply this diff to broaden the assertion:
- expect(httpPut).toHaveBeenCalledWith(mockSession, 'userpreferences/', { - enable_email_notifications: true, - }); + expect(httpPut).toHaveBeenCalledWith( + mockSession, + 'userpreferences/', + expect.objectContaining({ + enable_email_notifications: true, + // New category subscription fields + subscribe_incident_notifications: expect.any(Boolean), + subscribe_schema_change_notifications: expect.any(Boolean), + subscribe_job_failure_notifications: expect.any(Boolean), + subscribe_late_runs_notifications: expect.any(Boolean), + subscribe_dbt_test_failure_notifications: expect.any(Boolean), + }) + );src/components/Notifications/ManageNotifications.tsx (2)
91-99: Guard against null/undefined res; fix potential TypeError and selection edge casesSeveral places access
data?.res.length, which throws whenresis null. Also, “select all” tries to map over potentially undefined arrays.Apply this diff:
- const handleCheckUncheckAll = () => { - if (data?.res.length === checkedRows.length) { + const handleCheckUncheckAll = () => { + if ((data?.res?.length ?? 0) === checkedRows.length) { setCheckedRows([]); } else { - const allNotificationIds = data?.res.map((notification: Notification) => notification.id); + const allNotificationIds = (data?.res ?? []).map( + (notification: Notification) => notification.id + ); setCheckedRows(allNotificationIds); } }; @@ - const showingNotificationCount = data?.res.length || 0; + const showingNotificationCount = data?.res?.length ?? 0; @@ - <Checkbox + <Checkbox data-testid="select-all-checkbox" - checked={data?.res.length > 0 && data?.res.length === checkedRows.length} + checked={(data?.res?.length ?? 0) > 0 && (data?.res?.length ?? 0) === checkedRows.length} onChange={handleCheckUncheckAll} />Also applies to: 111-112, 125-133
265-276: Improve urgent indicator accessibilityAdd an accessible label to the icon so tests can query it reliably and it’s screen-reader friendly.
- {notification.urgent ? <ErrorOutline sx={{ color: 'red' }} /> : ''} + {notification.urgent ? ( + <ErrorOutline sx={{ color: 'red' }} aria-label="Urgent" /> + ) : null}
🧹 Nitpick comments (10)
src/components/Header/Header.test.tsx (2)
149-152: Remove the empty line and improve comment formatting.The empty line after the existing test suite and the multi-line comment could be more concise.
- -// Additional tests appended by PR tests generator to increase coverage and edge-case handling. -// Test framework: Jest; Library: React Testing Library +// Additional tests for edge cases and coverage improvement
286-292: Consider more specific unread count assertion.The current test uses a generic text match which might be fragile if the count appears elsewhere in the UI.
- // Expect a badge or count to be visible; fallback to searching for "7" - // If badge has aria-label/role, prefer that. Using text match as generic. - expect(screen.getAllByText('7').length).toBeGreaterThan(0); + // Look for unread count badge or notification indicator + const unreadIndicator = screen.getByRole('button', { name: /notification/i }) || + screen.getByLabelText(/unread/i) || + screen.getByText('7'); + expect(unreadIndicator).toBeInTheDocument();src/components/Notifications/__tests__/PreferencesForm.test.tsx (1)
386-394: Align “missing session” expectation with subscription fieldsEven when session is undefined, the payload shape should remain consistent and include subscription booleans. Make the assertion resilient.
- expect(httpPut).toHaveBeenCalledWith(undefined, 'userpreferences/', { - enable_email_notifications: true, - }); + expect(httpPut).toHaveBeenCalledWith( + undefined, + 'userpreferences/', + expect.objectContaining({ + enable_email_notifications: true, + subscribe_incident_notifications: expect.any(Boolean), + subscribe_schema_change_notifications: expect.any(Boolean), + subscribe_job_failure_notifications: expect.any(Boolean), + subscribe_late_runs_notifications: expect.any(Boolean), + subscribe_dbt_test_failure_notifications: expect.any(Boolean), + }) + );src/components/Notifications/ManageNotifications.tsx (6)
100-105: Avoid redundant mutate and duplicate state updates on pagination eventsChanging page/pageSize changes the SWR key, which triggers a refetch without calling
mutate. Also,onRowsPerPageChangeupdates state twice.const handlePagination = (newPageIndex: number, newPageSize: number) => { setCurrentPageIndex(newPageIndex); setPageSize(newPageSize); - mutate(); }; @@ - onRowsPerPageChange={(e) => { - const newPageSize = parseInt(e.target.value, 10); - setPageSize(newPageSize); - setCurrentPageIndex(0); // Reset to first page when changing rows per page - handlePagination(0, newPageSize); - }} + onRowsPerPageChange={(e) => { + const newPageSize = parseInt(e.target.value, 10); + handlePagination(0, newPageSize); + }}Also applies to: 318-322
74-80: Effect dependencies: include mutate function to satisfy hook correctnessEnsure the effect reacts properly and satisfies linting.
- }, [mutateAllRow]); + }, [mutateAllRow, mutate, setMutateAllRows]);
30-31: Make category optional in the type to reflect real-world payloadsThe UI already tolerates missing
category, but the type enforces presence. Make it optional to avoid misleading type safety.-interface Notification { +interface Notification { id: number; urgent: boolean; author: string; message: string; read_status: boolean; timestamp: string; - category: string; + category?: string; }
247-253: Add accessible timestamp markupWrap the relative time in a semantic
<time>element with a stable test id.- <Typography sx={{ fontWeight: 600, fontSize: '12px' }}> - {moment(new Date(notification.timestamp)).fromNow()} - </Typography> + <Typography sx={{ fontWeight: 600, fontSize: '12px' }}> + <time + dateTime={new Date(notification.timestamp).toISOString()} + data-testid={`timestamp-${notification.id}`} + > + {moment(new Date(notification.timestamp)).fromNow()} + </time> + </Typography>
241-249: Render author to align with tests and improve readabilityDisplay the author line under the message/category.
<Typography sx={{ fontWeight: 500, fontSize: '12px', color: '#0F2440AA' }}> Category: {notification.category || 'N/A'} </Typography> + <Typography sx={{ fontWeight: 500, fontSize: '12px', color: '#0F2440AA' }}> + {notification.author} + </Typography>
139-169: Optional: Clear row selection when category filter changesSwitching categories with stale selections can lead to confusing bulk actions. Clear
checkedRowson filter change for a cleaner UX.If desired, inside
onChange, callsetCheckedRows([]);aftersetSelectedCategory(...).src/components/Notifications/__tests__/ManageNotifications.test.tsx (1)
180-194: These tests assume explicit empty and error states—ensure component renders them or relax assertionsAfter adding explicit empty/error UIs in the component (
error-stateandempty-state), these tests should pass. If you prefer not to render such states, adjust tests to assert non-crashing behavior instead.Would you like me to update these tests to a non-crashing heuristic (e.g., presence of the header and “Showing 0 of 0 notifications”) if we decide not to ship explicit placeholders?
Also applies to: 196-210
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/components/Header/Header.test.tsx(1 hunks)src/components/Notifications/ManageNotifications.tsx(5 hunks)src/components/Notifications/__tests__/ManageNotifications.test.tsx(2 hunks)src/components/Notifications/__tests__/PreferencesForm.test.tsx(2 hunks)src/pages/notifications/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/pages/notifications/index.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/components/Header/Header.test.tsx (2)
src/components/Header/Header.tsx (1)
Header(50-384)src/contexts/ContextProvider.tsx (1)
GlobalContext(34-34)
src/components/Notifications/__tests__/PreferencesForm.test.tsx (3)
src/contexts/ContextProvider.tsx (1)
GlobalContext(34-34)src/helpers/http.tsx (1)
httpPut(57-78)src/components/ToastMessage/ToastHelper.ts (2)
successToast(2-13)errorToast(15-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks
🔇 Additional comments (11)
src/components/Header/Header.test.tsx (9)
153-194: LGTM! Well-structured test suite setup.The new test suite is properly isolated with its own describe block, and the
baseGlobalContextMockprovides a clean foundation for test variations.
196-233: Excellent helper function for flexible test setup.The
renderWithCtxhelper function provides good flexibility for overriding context values and header props while maintaining type safety. The deep merge logic for nested objects is particularly well-implemented.
235-250: LGTM! Proper test isolation and mocking.The
beforeEachsetup correctly clears mocks and establishes consistent default mocks for all dependencies.
252-260: LGTM! Clean conditional rendering tests.Both tests for the
hideMenuprop properly verify the hamburger icon visibility behavior.
262-284: LGTM! Comprehensive permission testing.These tests properly verify the dual permission requirements for the "Create new org" feature - both global permissions and user session flags must be true.
294-303: LGTM! Important edge case coverage.This test ensures the component gracefully handles empty organization lists without crashing.
339-367: LGTM! Comprehensive authentication state testing.The test properly verifies both authenticated and unauthenticated scenarios for the logout functionality.
369-374: LGTM! Clear hamburger menu test.The test properly verifies the hamburger menu toggle functionality.
376-389: LGTM! Good router sanity check.This test ensures that navigation methods aren't called unexpectedly during initial render.
src/components/Notifications/__tests__/PreferencesForm.test.tsx (1)
1-1: AI summary claims payload validation for subscription fields that the tests currently don’t assertThe AI summary mentions validating the new category subscription fields in PreferencesForm payloads. The tests added here do not yet assert presence of the
subscribe_*fields, risking regressions.Would you like me to add a focused test that toggles one of the subscription switches (e.g., Incident) and verifies the corresponding field in the
userpreferences/payload? If so, please confirm/clarify the exact field labels rendered by PreferencesForm to avoid brittle selectors.Also applies to: 189-497
src/components/Notifications/__tests__/ManageNotifications.test.tsx (1)
1-120: AI summary overstates test coverage scopeThe summary mentions category-filtering tests, but the actual changes don’t include them yet. The urgent indicator assertion also doesn’t match the implementation.
Would you like me to push a patch adding robust category filter tests and fixing the urgent/timestamp assertions, or should we pare back the summary to reflect current coverage?
Also applies to: 121-359
| test('pathname-based conditional UI: uses current pathname for conditional rendering', () => { | ||
| (usePathname as jest.Mock).mockReturnValue('/dashboard'); | ||
| renderWithCtx(); | ||
|
|
||
| // This is a soft check: look for a dashboard-specific element if any present. | ||
| // If not, ensure the header still renders fundamental elements. | ||
| expect(screen.getByAltText('profile icon')).toBeInTheDocument(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance pathname-based conditional rendering test.
The test is too generic and doesn't effectively verify pathname-based behavior changes.
Based on the Header component summary, the hamburger menu visibility is affected by pathname. Here's a more specific test:
test('pathname-based conditional UI: uses current pathname for conditional rendering', () => {
- (usePathname as jest.Mock).mockReturnValue('/dashboard');
+ (usePathname as jest.Mock).mockReturnValue('/changepassword');
renderWithCtx();
- // This is a soft check: look for a dashboard-specific element if any present.
- // If not, ensure the header still renders fundamental elements.
- expect(screen.getByAltText('profile icon')).toBeInTheDocument();
+ // On /changepassword route, hamburger should be hidden
+ expect(screen.queryByAltText('Hamburger-icon')).toBeNull();
+ expect(screen.getByAltText('profile icon')).toBeInTheDocument();
});📝 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.
| test('pathname-based conditional UI: uses current pathname for conditional rendering', () => { | |
| (usePathname as jest.Mock).mockReturnValue('/dashboard'); | |
| renderWithCtx(); | |
| // This is a soft check: look for a dashboard-specific element if any present. | |
| // If not, ensure the header still renders fundamental elements. | |
| expect(screen.getByAltText('profile icon')).toBeInTheDocument(); | |
| }); | |
| }); | |
| test('pathname-based conditional UI: uses current pathname for conditional rendering', () => { | |
| (usePathname as jest.Mock).mockReturnValue('/changepassword'); | |
| renderWithCtx(); | |
| // On /changepassword route, hamburger should be hidden | |
| expect(screen.queryByAltText('Hamburger-icon')).toBeNull(); | |
| expect(screen.getByAltText('profile icon')).toBeInTheDocument(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In src/components/Header/Header.test.tsx around lines 391 to 399, the current
test only asserts a generic profile icon and does not verify pathname-driven UI
changes; update this test to specifically assert the hamburger menu visibility
based on pathname by mocking usePathname to return '/dashboard' and then
checking that the hamburger button (by aria-label, role or test-id used in
Header) is present, and add a complementary assertion (or a new test) that mocks
a pathname where the hamburger should be hidden and asserts the hamburger is not
present (use queryByLabelText/queryByRole for absence). Ensure you keep
renderWithCtx and use the same selectors the Header component uses for the
hamburger so the assertions reliably reflect the conditional rendering.
src/components/Notifications/__tests__/ManageNotifications.test.tsx
Outdated
Show resolved
Hide resolved
| const { data, isLoading, mutate } = useSWR( | ||
| `notifications/v1?limit=${pageSize}&page=${currentPageIndex + 1}${readQuery}` | ||
| `notifications/v1?limit=${pageSize}&page=${currentPageIndex + 1}${readQuery}${categoryQuery}` | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit error and empty states for better UX and to support tests
Handle SWR errors and empty data gracefully.
- const { data, isLoading, mutate } = useSWR(
+ const { data, isLoading, mutate, error } = useSWR(
`notifications/v1?limit=${pageSize}&page=${currentPageIndex + 1}${readQuery}${categoryQuery}`
);
@@
if (isLoading) return <CircularProgress />;
+ if (error) {
+ return <Typography data-testid="error-state">Failed to load notifications</Typography>;
+ }
+ if (!data?.res?.length) {
+ return <Typography data-testid="empty-state">No notifications</Typography>;
+ }Also applies to: 109-111
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1636 +/- ##
==========================================
+ Coverage 75.96% 76.06% +0.09%
==========================================
Files 102 102
Lines 7769 7801 +32
Branches 1867 1883 +16
==========================================
+ Hits 5902 5934 +32
- Misses 1753 1755 +2
+ Partials 114 112 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/components/Header/Header.test.tsx (1)
126-137: Assert signOut args and localStorage.clear in Header testVerified: Header.tsx calls localStorage.clear() and signOut({ callbackUrl:
${window.location.origin}}) — update the test to assert both.
- src/components/Header/Header.tsx — lines 61–65
- src/components/Header/Header.test.tsx — lines 126–137
Suggested diff:
@@ - renderComponent(); - const profileIcon = screen.getByAltText('profile icon'); - - fireEvent.click(profileIcon); - const logoutItem = screen.getByText('Logout'); - fireEvent.click(logoutItem); - - await waitFor(() => { - expect(mockSignOut).toHaveBeenCalled(); - }); + renderComponent(); + const profileIcon = screen.getByAltText('profile icon'); + + const clearSpy = jest.spyOn(window.localStorage, 'clear').mockImplementation(() => {}); + + fireEvent.click(profileIcon); + const logoutItem = screen.getByText('Logout'); + fireEvent.click(logoutItem); + + await waitFor(() => { + expect(mockSignOut).toHaveBeenCalledWith(expect.objectContaining({ + callbackUrl: window.location.origin, + })); + }); + + expect(clearSpy).toHaveBeenCalled(); + clearSpy.mockRestore();
♻️ Duplicate comments (5)
src/components/Notifications/__tests__/ManageNotifications.test.tsx (3)
297-309: Use stable test ids for timestamps instead of locale-dependent “ago” textRelative time strings are locale/time dependent and can be flaky. Prefer the component’s timestamp test ids.
Apply this diff:
- // Authors are not directly displayed in the UI based on the component code - // but timestamps are shown using moment.js - // Look for "ago" text which indicates relative time - const timeElements = screen.getAllByText(/ago/); - expect(timeElements.length).toBeGreaterThan(0); + // Assert timestamp elements via their test ids for stability + expect(screen.getByTestId('timestamp-1')).toBeInTheDocument(); + expect(screen.getByTestId('timestamp-2')).toBeInTheDocument();
196-201: Prefer accessible query for urgent indicator and scope to the urgent rowRelying on MUI’s default
data-testidfor icons is brittle. Query the urgent icon by its accessible name and scope to the urgent row to avoid false positives.Apply this diff:
- test('displays urgent indicator for urgent notifications', () => { - render(<ManageNotifications {...defaultProps} />); - // Look for ErrorOutline icon which indicates urgent notifications - const errorIcons = screen.getAllByTestId('ErrorOutlineIcon'); - expect(errorIcons.length).toBeGreaterThan(0); - }); + test('displays urgent indicator for urgent notifications', () => { + render(<ManageNotifications {...defaultProps} />); + const row = screen.getByTestId('1-checkbox').closest('[role="row"], li, tr, div')!; + // The icon should have an accessible label (e.g., aria-label="urgent") + expect(within(row).getByLabelText(/urgent/i)).toBeInTheDocument(); + });
310-310: Add a test: verify category filter updates SWR key and resets pagination (MUI Select handling)I checked ManageNotifications.tsx — the Select is MUI (non‑native) and its onChange sets selectedCategory and resets currentPageIndex to 0; the useSWR key is built from pageSize, currentPageIndex+1 and categoryQuery. The tests in src/components/Notifications/tests/ManageNotifications.test.tsx cover display but do not assert that changing the category updates the SWR key and resets the page.
Files to update:
- src/components/Notifications/tests/ManageNotifications.test.tsx
- Verified component: src/components/Notifications/ManageNotifications.tsx
Apply this diff to append a focused test (uses mouseDown/click to interact with MUI Select):
@@ test('author and timestamp are rendered for each notification if available', () => { @@ }); + + test('category filter updates SWR key and resets to first page', async () => { + (useSWR as jest.Mock).mockClear(); + render(<ManageNotifications {...defaultProps} />); + // MUI non-native Select renders a button — scope from the "Filter by Category:" label + const filterLabel = screen.getByText('Filter by Category:'); + const selectButton = filterLabel.parentElement!.querySelector('button') as HTMLElement; + expect(selectButton).toBeInTheDocument(); + // Open the menu and choose "Incident" + fireEvent.mouseDown(selectButton); + const incidentOption = await screen.findByText('Incident'); + fireEvent.click(incidentOption); + // SWR should be called again with category=incident and page=1 + const lastCall = (useSWR as jest.Mock).mock.calls.slice(-1)[0]; + expect(lastCall[0]).toMatch(/category=incident/); + expect(lastCall[0]).toMatch(/page=1/); + });src/components/Header/Header.test.tsx (2)
317-343: Fix flakey assertion and avoid brittle localStorage override
- The assertion expect(currentOrgDispatch).not.toHaveBeenCalledTimes(2) doesn’t prove no extra dispatch occurred.
- Overriding window.localStorage via defineProperty without cleanup risks leaking state across tests.
Apply this diff to make the test deterministic and self-cleaning:
- test('switching to the currently selected org does not dispatch a change (if guarded)', () => { - // Mock localStorage to have org1 as current - const mockLocalStorage = { - getItem: jest.fn(() => 'org1'), - setItem: jest.fn(), - }; - Object.defineProperty(window, 'localStorage', { - value: mockLocalStorage, - }); - - const currentOrgDispatch = jest.fn(); - renderWithCtx({ CurrentOrg: { dispatch: currentOrgDispatch } }); - - const profileIcon = screen.getByAltText('profile icon'); - fireEvent.click(profileIcon); - - // Click on Org1 which should already be selected - const org1Item = screen.getAllByText('Org1').find((el) => el.closest('[role="menuitem"]')); - if (org1Item) { - fireEvent.click(org1Item); - } - - // Since org1 is already current (from localStorage), no new dispatch should occur - // The component should guard against dispatching the same org - expect(currentOrgDispatch).not.toHaveBeenCalledTimes(2); - }); + test('selecting already-selected org does not trigger an extra dispatch', () => { + const getItemSpy = jest.spyOn(Storage.prototype, 'getItem').mockReturnValue('org1'); + const currentOrgDispatch = jest.fn(); + + renderWithCtx({ CurrentOrg: { dispatch: currentOrgDispatch } }); + const initialCallCount = currentOrgDispatch.mock.calls.length; + + const profileIcon = screen.getByAltText('profile icon'); + fireEvent.click(profileIcon); + + const org1Item = screen.getAllByText('Org1').find((el) => el.closest('[role="menuitem"]')); + if (org1Item) fireEvent.click(org1Item); + + expect(currentOrgDispatch).toHaveBeenCalledTimes(initialCallCount); + getItemSpy.mockRestore(); + });
384-391: Assert pathname-driven hamburger visibility explicitlyReplace the generic check with an assertion that hamburger is hidden on /changepassword and visible on a normal route. This directly validates the conditional render.
Apply this diff:
- test('pathname-based conditional UI: uses current pathname for conditional rendering', () => { - (usePathname as jest.Mock).mockReturnValue('/dashboard'); - renderWithCtx(); - - // This is a soft check: look for a dashboard-specific element if any present. - // If not, ensure the header still renders fundamental elements. - expect(screen.getByAltText('profile icon')).toBeInTheDocument(); - }); + test('pathname-based UI: hides hamburger on /changepassword', () => { + (usePathname as jest.Mock).mockReturnValue('/changepassword'); + renderWithCtx({}, { hideMenu: false }); + expect(screen.queryByAltText('Hamburger-icon')).toBeNull(); + }); + + test('pathname-based UI: shows hamburger on /dashboard', () => { + (usePathname as jest.Mock).mockReturnValue('/dashboard'); + renderWithCtx({}, { hideMenu: false }); + expect(screen.getByAltText('Hamburger-icon')).toBeInTheDocument(); + });
🧹 Nitpick comments (9)
src/components/Notifications/__tests__/ManageNotifications.test.tsx (5)
10-33: Deduplicate mock data to reduce maintenance overheadThe top-level
mockNotificationsand thebaseDatabelow are duplicates with identical shapes. Keep a single source of truth to avoid drift and simplify updates.Apply this diff to remove the duplicate and reuse
baseDatain the first describe:@@ -// Mock data -const mockNotifications = { - total_notifications: 2, - res: [ - { - id: 1, - urgent: true, - author: 'Admin', - message: 'Urgent message 1', - read_status: false, - timestamp: new Date().toISOString(), - category: 'incident', - }, - { - id: 2, - urgent: false, - author: 'User', - message: - 'This is a normal message with a long text to test truncation and expansion functionality which should be longer than 130 characters to trigger the expand/collapse behavior in the component.', - read_status: true, - timestamp: new Date().toISOString(), - category: 'job_failure', - }, - ], -}; @@ beforeEach(() => { - (useSWR as jest.Mock).mockReturnValue({ - data: mockNotifications, + (useSWR as jest.Mock).mockReturnValue({ + data: baseData, isLoading: false, mutate: mockMutate, }); });Then move
baseDatato the top (after imports) so both suites can use it:// Place this near the top, after jest.mock('swr'); const baseData = { total_notifications: 2, res: [ { id: 1, urgent: true, author: 'Admin', message: 'Urgent message 1', read_status: false, timestamp: new Date().toISOString(), category: 'incident', }, { id: 2, urgent: false, author: 'User', message: 'This is a normal message with a long text to test truncation and expansion functionality which should be longer than 130 characters to trigger the expand/collapse behavior in the component.', read_status: true, timestamp: new Date().toISOString(), category: 'job_failure', }, ], };Also applies to: 115-139
69-96: Strengthen the checkbox selection test by asserting state persistence across rerendersYou’re using a controlled wrapper and rerendering, but not verifying that the first selection persists across rerender. Add an assertion to confirm state is retained.
Apply this diff:
@@ const { rerender } = render(<MockComponent />); @@ expect(checkbox1).toBeChecked(); rerender(<MockComponent />); + // After rerender, the selection should persist + const checkbox1After = screen + .getByTestId('1-checkbox') + .querySelector('input[type="checkbox"]'); + expect(checkbox1After).toBeChecked(); + const checkbox2 = screen.getByTestId('2-checkbox').querySelector('input[type="checkbox"]'); fireEvent.click(checkbox2!); expect(checkbox2).toBeChecked();
98-109: Remove unnecessary async from the testNo awaits or async operations are used in this test.
Apply this diff:
- test('handles "select all" functionality', async () => { + test('handles "select all" functionality', () => {
182-194: Make the error-state assertion less brittleAsserting the exact string “Showing 0 of 0 notifications” couples the test to a specific empty-state formatting. Prefer a regex that tolerates copy changes or alternative counts.
Apply this diff:
- // Component doesn't explicitly handle error state, so it will show empty content - // We can check that it doesn't crash and renders the basic structure - expect(screen.getByText(/Showing 0 of 0 notifications/)).toBeInTheDocument(); + // Component doesn't explicitly handle error state; assert it renders without crashing + expect(screen.getByText(/Showing \d+ of \d+ notifications/)).toBeInTheDocument();If the component later adds a dedicated error UI, adjust this assertion to target that specific element (e.g., an alert role or message).
263-277: Tighten expectations on mutate behaviorAdd a call count assertion to ensure exactly one mutate occurs when toggling
mutateAllRowto true.Apply this diff:
@@ rerender(<ManageNotifications {...props} mutateAllRow={true} />); - expect(mockMutate).toHaveBeenCalled(); + expect(mockMutate).toHaveBeenCalledTimes(1); expect(props.setMutateAllRows).toHaveBeenCalledWith(false);src/components/Header/Header.test.tsx (4)
6-6: Type the SWR mock for better safety and IDE supportUse a typed mock for useSWR to catch selector/shape mismatches at compile time.
Apply this diff:
-const mockUseSWR = useSWR as jest.Mock; +const mockUseSWR = useSWR as jest.MockedFunction<typeof useSWR>;Also applies to: 14-14, 19-19
47-49: Remove redundant mockDispatch call and reorder clearAllMocksmockDispatch(4) is immediately nullified by jest.clearAllMocks(); it never contributes to any assertion and adds noise. Prefer clearing mocks before setting return values to avoid surprises.
Apply this diff:
beforeEach(() => { + jest.clearAllMocks(); mockUseRouter.mockReturnValue({ push: jest.fn(), refresh: jest.fn(), }); @@ - mockDispatch(4); - jest.clearAllMocks(); + // (removed redundant mockDispatch call) });
51-89: Deduplicate GlobalContext fixtures across suitesYou define nearly identical context fixtures twice (globalContextMock and baseGlobalContextMock). Consolidate into a single module-scope base fixture to reduce drift and maintenance.
Example refactor outline (not a full diff):
- Hoist baseGlobalContextMock to module scope.
- In the first describe, reuse baseGlobalContextMock (possibly shallow-cloned) instead of redefining globalContextMock.
- Keep per-test overrides via renderWithCtx or inline Provider overrides.
Also applies to: 165-204
244-270: DRY up repeated base mocks with a shared test setupYou repeat router/session/pathname/SWR defaults across describes. Extract a setupDefaultMocks() invoked in both beforeEach blocks to keep one source of truth.
Example:
function setupDefaultMocks() { (useRouter as jest.Mock).mockReturnValue({ push: jest.fn(), refresh: jest.fn() }); (useSession as jest.Mock).mockReturnValue({ data: { user: { email: '[email protected]', can_create_orgs: true } }, status: 'authenticated', }); (usePathname as jest.Mock).mockReturnValue('/'); mockUseSWR.mockImplementation((key) => { if (key === 'notifications/unread_count') return { data: { res: 0 } }; if (key === 'notifications/urgent') return { data: { res: [] }, mutate: jest.fn() }; return { data: null }; }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/components/Header/Header.test.tsx(3 hunks)src/components/Notifications/__tests__/ManageNotifications.test.tsx(4 hunks)src/components/Notifications/__tests__/PreferencesForm.test.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Notifications/tests/PreferencesForm.test.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/Header/Header.test.tsx (2)
src/components/Header/Header.tsx (1)
Header(50-384)src/contexts/ContextProvider.tsx (1)
GlobalContext(34-34)
🪛 Gitleaks (8.27.2)
src/components/Header/Header.test.tsx
42-42: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
265-265: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks
🔇 Additional comments (8)
src/components/Notifications/__tests__/ManageNotifications.test.tsx (1)
3-3: Import path fix looks goodCorrect path resolution to the component under test.
src/components/Header/Header.test.tsx (7)
38-46: Good SWR mocks aligned with new unread_count and urgent endpointsMocks correctly emulate notifications/unread_count and notifications/urgent keys, including mutate for urgent. This matches Header.tsx behavior and keeps tests deterministic.
117-124: LGTM: hamburger interaction is correctly assertedClick triggers setOpenMenu(true) as intended.
139-159: LGTM: org switch flow asserts correct dispatch payloadThe test accurately targets selecting Org2 and checks dispatch with expected orgState shape.
205-243: Nice render helper to inject context and override propsrenderWithCtx provides a clean, composable way to vary both context and header props across tests.
272-280: LGTM: hideMenu gating for hamburger rendersVisibility checks read well and reflect Header’s conditions.
291-304: LGTM: session.can_create_orgs gate effectively coveredThis verifies the conjunction with permissions is enforced correctly in UI.
306-315: LGTM: resilient render without organizationsGood guard to ensure Header remains stable when OrgUsers.state is empty.
| // Mock useSWR for notifications | ||
| mockUseSWR.mockImplementation((key) => { | ||
| if (key === 'notifications/unread_count') { | ||
| return { data: { res: 0 } }; | ||
| } | ||
| if (key === 'notifications/urgent') { | ||
| return { data: { res: [] }, mutate: jest.fn() }; | ||
| } | ||
| return { data: null }; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add a focused test for urgent notifications render and dismiss
You mock notifications/urgent but don’t assert the new urgent banner behavior. Add a test that renders with a non-empty urgent payload and verifies dismiss triggers mutate/read.
Proposed test (adjust selectors to match Header’s urgent UI):
+ test('urgent notifications: renders panel and calls mutate on dismiss', () => {
+ const urgentMutate = jest.fn();
+ mockUseSWR.mockImplementation((key) => {
+ if (key === 'notifications/unread_count') return { data: { res: 0 } };
+ if (key === 'notifications/urgent') {
+ return { data: { res: [{ id: 'u1', title: 'Critical outage' }] }, mutate: urgentMutate };
+ }
+ return { data: null };
+ });
+ renderWithCtx();
+ // Adjust these queries to match the actual urgent UI in Header.tsx:
+ expect(screen.getByText(/critical outage/i)).toBeInTheDocument();
+ const dismissBtn =
+ screen.queryByRole('button', { name: /dismiss|close|mark as read/i }) ||
+ screen.getByText(/dismiss|close|mark as read/i);
+ fireEvent.click(dismissBtn as HTMLElement);
+ expect(urgentMutate).toHaveBeenCalled();
+ });If you’re unsure of the selectors, run this to inspect the urgent UI to choose stable queries:
🏁 Script executed:
#!/bin/bash
rg -n "urgent|Urgent" -A 6 -B 6 src/components/Header/Header.tsxLength of output: 2420
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for httpPut usages..."
rg -n --hidden "httpPut" || true
echo
echo "Searching for renderWithCtx helper usage..."
rg -n --hidden "renderWithCtx" || true
echo
echo "Showing top of src/components/Header/Header.test.tsx (if exists)..."
if [ -f src/components/Header/Header.test.tsx ]; then
sed -n '1,220p' src/components/Header/Header.test.tsx
else
echo "File not found: src/components/Header/Header.test.tsx"
fi
echo
echo "Showing remainder of Header.test.tsx (220..400) if present..."
if [ -f src/components/Header/Header.test.tsx ]; then
sed -n '221,420p' src/components/Header/Header.test.tsx
fiLength of output: 26106
Add a focused test for urgent notifications render and dismiss
Header shows urgentNotifications.res.map rendering msg.message and an IconButton that calls handleReadUrgent → httpPut(...) then mutateUrgent. The tests currently mock 'notifications/urgent' as an empty array in the beforeEach and don't assert the urgent-banner/dismiss behavior. Add a focused test that supplies a non-empty urgent payload, mocks httpPut and the SWR mutate, renders the header, clicks the dismiss button, and asserts mutate (and optionally httpPut) was called.
Files/locations to update:
- src/components/Header/Header.tsx — useSWR and handler: line ~59 (useSWR), 88–96 (handleReadUrgent → httpPut + mutateUrgent), 323–377 (urgent UI: msg.message and IconButton onClick).
- src/components/Header/Header.test.tsx — the beforeEach currently returns empty urgent; add the new test near the other render tests and add a small import/mock for httpPut if not already mocked.
Suggested test (adjust imports/location as needed):
+import { render, screen, fireEvent, waitFor, within } from '@testing-library/react';
+import { httpPut } from '@/helpers/http';
+jest.mock('@/helpers/http');
+
+ test('urgent notifications: renders panel and calls mutate on dismiss', async () => {
+ const urgentMutate = jest.fn();
+ // Ensure httpPut resolves to avoid throwing in handleReadUrgent
+ (httpPut as jest.Mock).mockResolvedValue({});
+
+ mockUseSWR.mockImplementation((key) => {
+ if (key === 'notifications/unread_count') return { data: { res: 0 } };
+ if (key === 'notifications/urgent') {
+ return {
+ data: { res: [{ id: 'u1', message: 'Critical outage' }] },
+ mutate: urgentMutate,
+ };
+ }
+ return { data: null };
+ });
+
+ renderWithCtx();
+
+ // The urgent UI renders msg.message
+ const msg = screen.getByText(/critical outage/i);
+ const item = msg.closest('div');
+ expect(item).toBeTruthy();
+
+ // The dismiss IconButton is rendered next to the message — find the button within that item
+ const dismissBtn = within(item as Element).getByRole('button');
+ fireEvent.click(dismissBtn);
+
+ // handleReadUrgent calls httpPut(...) then mutateUrgent()
+ await waitFor(() => expect(urgentMutate).toHaveBeenCalled());
+ expect(httpPut).toHaveBeenCalledWith(
+ expect.anything(),
+ 'notifications/v1',
+ { notification_ids: ['u1'], read_status: true }
+ );
+ });📝 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.
| // Mock useSWR for notifications | |
| mockUseSWR.mockImplementation((key) => { | |
| if (key === 'notifications/unread_count') { | |
| return { data: { res: 0 } }; | |
| } | |
| if (key === 'notifications/urgent') { | |
| return { data: { res: [] }, mutate: jest.fn() }; | |
| } | |
| return { data: null }; | |
| }); | |
| }); | |
| // Mock useSWR for notifications | |
| mockUseSWR.mockImplementation((key) => { | |
| if (key === 'notifications/unread_count') { | |
| return { data: { res: 0 } }; | |
| } | |
| if (key === 'notifications/urgent') { | |
| return { data: { res: [] }, mutate: jest.fn() }; | |
| } | |
| return { data: null }; | |
| }); | |
| }); | |
| test('urgent notifications: renders panel and calls mutate on dismiss', async () => { | |
| const urgentMutate = jest.fn(); | |
| // Ensure httpPut resolves to avoid throwing in handleReadUrgent | |
| (httpPut as jest.Mock).mockResolvedValue({}); | |
| mockUseSWR.mockImplementation((key) => { | |
| if (key === 'notifications/unread_count') return { data: { res: 0 } }; | |
| if (key === 'notifications/urgent') { | |
| return { | |
| data: { res: [{ id: 'u1', message: 'Critical outage' }] }, | |
| mutate: urgentMutate, | |
| }; | |
| } | |
| return { data: null }; | |
| }); | |
| renderWithCtx(); | |
| // The urgent UI renders msg.message | |
| const msg = screen.getByText(/critical outage/i); | |
| const item = msg.closest('div'); | |
| expect(item).toBeTruthy(); | |
| // The dismiss IconButton is rendered next to the message — find the button within that item | |
| const dismissBtn = within(item as Element).getByRole('button'); | |
| fireEvent.click(dismissBtn); | |
| // handleReadUrgent calls httpPut(...) then mutateUrgent() | |
| await waitFor(() => expect(urgentMutate).toHaveBeenCalled()); | |
| expect(httpPut).toHaveBeenCalledWith( | |
| expect.anything(), | |
| 'notifications/v1', | |
| { notification_ids: ['u1'], read_status: true } | |
| ); | |
| }); |
🧰 Tools
🪛 Gitleaks (8.27.2)
265-265: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In src/components/Header/Header.test.tsx around the existing useSWR mocks (lines
~260–270) add a focused test that overrides the beforeEach empty urgent payload
by mocking useSWR for the 'notifications/urgent' key to return data: { res: [{
id: 'u1', message: 'urgent msg' }] } and a jest.fn() mutate; also mock httpPut
(import/mock it if not already) to resolve successfully. Render the Header,
query the urgent banner or the IconButton that dismisses the urgent message,
simulate a click on the dismiss button, and assert that the mutate function for
urgent was called (and optionally that httpPut was called with the expected
endpoint/payload). Ensure the new test restores mocks after running or isolates
the mock so other tests still receive the default empty urgent from the
beforeEach.
| test('router is available and not invoked on simple render', () => { | ||
| const mockPush = jest.fn(); | ||
| const mockRefresh = jest.fn(); | ||
| (useRouter as jest.Mock).mockReturnValue({ | ||
| push: mockPush, | ||
| refresh: mockRefresh, | ||
| }); | ||
|
|
||
| renderWithCtx(); | ||
|
|
||
| // Basic sanity: no navigation on initial render | ||
| expect(mockPush).not.toHaveBeenCalled(); | ||
| expect(mockRefresh).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate router.refresh isn’t expected on initial render
Depending on Header’s useEffect for selectedOrg, router.refresh() might fire on mount. If so, this test will be brittle.
Run to verify whether router.refresh can be invoked on mount:
If refresh can occur on initialization, relax the assertion to only check that push was not called, or prime localStorage to match the default Org to prevent an initial change.
🏁 Script executed:
#!/bin/bash
# Show the effect that triggers router.refresh in Header.tsx
rg -n "useEffect\\(|router\\.refresh\\(" -A 8 -B 8 src/components/Header/Header.tsxLength of output: 1911
Don't assert router.refresh isn't called on initial render — it can fire on mount
Short reason: Header sets selectedOrg (useEffect at lines 100–124) and another effect compares it to localStorage['org-slug'] and calls router.refresh() when they differ (router.refresh at line 141). That means refresh can legitimately run on initial render and the test is brittle.
Attention points:
- src/components/Header/Header.tsx — useEffect that sets selectedOrg: lines 100–124
- src/components/Header/Header.tsx — effect that may call router.refresh(): lines 126–142 (router.refresh at 141)
Suggested fixes (pick one):
- Relax the test: only assert push was not called and remove the refresh assertion.
- Prime localStorage before renderWithCtx() to match the test fixture's default org slug (so selectedOrg === localStorage['org-slug'] and refresh won't run).
- Alternatively, explicitly mock/stub selectedOrg/globalContext or mock router.refresh and assert expected behavior.
| test('message truncation/expansion control is present if supported', () => { | ||
| render(<ManageNotifications {...defaultProps} />); | ||
|
|
||
| // Look for expand button (KeyboardArrowDown icon) for the long message | ||
| const expandButtons = screen.getAllByTestId('KeyboardArrowDownIcon'); | ||
| expect(expandButtons.length).toBeGreaterThan(0); | ||
|
|
||
| // Click the expand button | ||
| const expandButton = expandButtons[0].closest('button'); | ||
| if (expandButton) { | ||
| fireEvent.click(expandButton); | ||
|
|
||
| // After expanding, should see the collapse button | ||
| const collapseButtons = screen.getAllByTestId('KeyboardArrowUpIcon'); | ||
| expect(collapseButtons.length).toBeGreaterThan(0); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert content expansion effect, and drop redundant conditional
You already assert that an expand button exists, so the null-check is unnecessary. Add an assertion that verifies the full message becomes visible after expanding.
Apply this diff:
// Click the expand button
- const expandButton = expandButtons[0].closest('button');
- if (expandButton) {
- fireEvent.click(expandButton);
-
- // After expanding, should see the collapse button
- const collapseButtons = screen.getAllByTestId('KeyboardArrowUpIcon');
- expect(collapseButtons.length).toBeGreaterThan(0);
- }
+ const expandButton = expandButtons[0].closest('button')!;
+ fireEvent.click(expandButton);
+
+ // After expanding, should see the collapse button and the full message body
+ const collapseButtons = screen.getAllByTestId('KeyboardArrowUpIcon');
+ expect(collapseButtons.length).toBeGreaterThan(0);
+ expect(
+ screen.getByText(
+ /which should be longer than 130 characters to trigger the expand\/collapse behavior in the component\./
+ )
+ ).toBeInTheDocument();📝 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.
| test('message truncation/expansion control is present if supported', () => { | |
| render(<ManageNotifications {...defaultProps} />); | |
| // Look for expand button (KeyboardArrowDown icon) for the long message | |
| const expandButtons = screen.getAllByTestId('KeyboardArrowDownIcon'); | |
| expect(expandButtons.length).toBeGreaterThan(0); | |
| // Click the expand button | |
| const expandButton = expandButtons[0].closest('button'); | |
| if (expandButton) { | |
| fireEvent.click(expandButton); | |
| // After expanding, should see the collapse button | |
| const collapseButtons = screen.getAllByTestId('KeyboardArrowUpIcon'); | |
| expect(collapseButtons.length).toBeGreaterThan(0); | |
| } | |
| }); | |
| test('message truncation/expansion control is present if supported', () => { | |
| render(<ManageNotifications {...defaultProps} />); | |
| // Look for expand button (KeyboardArrowDown icon) for the long message | |
| const expandButtons = screen.getAllByTestId('KeyboardArrowDownIcon'); | |
| expect(expandButtons.length).toBeGreaterThan(0); | |
| // Click the expand button | |
| const expandButton = expandButtons[0].closest('button')!; | |
| fireEvent.click(expandButton); | |
| // After expanding, should see the collapse button and the full message body | |
| const collapseButtons = screen.getAllByTestId('KeyboardArrowUpIcon'); | |
| expect(collapseButtons.length).toBeGreaterThan(0); | |
| expect( | |
| screen.getByText( | |
| /which should be longer than 130 characters to trigger the expand\/collapse behavior in the component\./ | |
| ) | |
| ).toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
In src/components/Notifications/__tests__/ManageNotifications.test.tsx around
lines 279 to 295, the test contains an unnecessary null-check around the expand
button and lacks an assertion that the full message is shown after expansion;
remove the conditional (assume the expand button exists after already asserting
its presence), click the button directly, then add an assertion that the long
notification message text (use the specific message string from defaultProps or
a regex matching its full content) is visible in the document after the click to
verify expansion worked.
| { label: 'Schema Change', value: 'schema_change' }, | ||
| { label: 'Job Failure', value: 'job_failure' }, | ||
| { label: 'Late Run', value: 'late_run' }, | ||
| { label: 'dbt Test Failure', value: 'dbt_test_failure' }, |
There was a problem hiding this comment.
We should remove Incident notifications from here. Those are sent by platform admins, when something has crashed on Dalgo and a fix is in works. Users should not be able to subscribe out of it.
Also lets remove Late Run, we dont have a provision to track late runs and send notifications at the moment. We can add this later when we do set that up
There was a problem hiding this comment.
@Ishankoradia
This one is for filter by category, I think we should not remove from here.
… for incident and late runs
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Notifications/ManageNotifications.tsx (1)
91-98: Potential runtime TypeError: unsafe access on possibly-undefineddata.resMultiple places use
data?.res.lengthordata?.res.mapwithout guardingres. Ifdatais defined butresis undefined/null, this throws.Apply these diffs:
const handleCheckUncheckAll = () => { - if (data?.res.length === checkedRows.length) { + if ((data?.res?.length ?? 0) === checkedRows.length) { setCheckedRows([]); } else { - const allNotificationIds = data?.res.map((notification: Notification) => notification.id); + const allNotificationIds = (data?.res ?? []).map((n: Notification) => n.id); setCheckedRows(allNotificationIds); } };<Checkbox data-testid="select-all-checkbox" - checked={data?.res.length > 0 && data?.res.length === checkedRows.length} + checked={(data?.res?.length ?? 0) > 0 && (data?.res?.length ?? 0) === checkedRows.length} + indeterminate={ + (data?.res?.length ?? 0) > 0 && + checkedRows.length > 0 && + checkedRows.length < (data?.res?.length ?? 0) + } onChange={handleCheckUncheckAll} />- const showingNotificationCount = data?.res.length || 0; + const showingNotificationCount = data?.res?.length ?? 0;Also applies to: 125-133, 111-112
♻️ Duplicate comments (1)
src/components/Notifications/ManageNotifications.tsx (1)
64-66: Add explicit error and empty states (per prior review) to avoid crashes and improve UXCurrently only the loading state is handled. Add error and empty states and pull
errorfrom SWR.Apply these diffs:
- const { data, isLoading, mutate } = useSWR( + const { data, isLoading, mutate, error } = useSWR( `notifications/v1?limit=${pageSize}&page=${currentPageIndex + 1}${readQuery}${categoryQuery}` );- if (isLoading) return <CircularProgress />; + if (isLoading) return <CircularProgress />; + if (error) { + return <Typography data-testid="error-state">Failed to load notifications</Typography>; + } + if (!data?.res?.length) { + return <Typography data-testid="empty-state">No notifications</Typography>; + }Also applies to: 109-111
🧹 Nitpick comments (7)
src/components/Notifications/PreferencesForm.tsx (3)
47-58: Initialize form defaults to avoid Switch controlled/uncontrolled warningsUntil SWR data loads, these values are undefined and Switch expects booleans. Either set defaultValues in useForm or coerce the Switch checked prop (see next comment).
Consider initializing defaults at the form level:
const { handleSubmit, register, reset, control, setValue, watch, formState: { errors }, } = useForm<PreferencesFormInput>({ defaultValues: { enable_email_notifications: false, enable_discord_notifications: false, discord_webhook: '', subscribe_schema_change_notifications: false, subscribe_job_failure_notifications: false, subscribe_dbt_test_failure_notifications: false, }, });
132-167: Harden Switch Controllers: always pass booleans and propagate checked stateEnsure Switch gets a boolean and RHF receives the checked boolean instead of the native event.
Apply this diff:
- <FormControlLabel - control={<Switch {...field} checked={field.value} />} + <FormControlLabel + control={ + <Switch + checked={!!field.value} + onChange={(e) => field.onChange(e.target.checked)} + /> + } label="Schema Change Notifications" />- <FormControlLabel - control={<Switch {...field} checked={field.value} />} + <FormControlLabel + control={ + <Switch + checked={!!field.value} + onChange={(e) => field.onChange(e.target.checked)} + /> + } label="Job Failure Notifications" />- <FormControlLabel - control={<Switch {...field} checked={field.value} />} + <FormControlLabel + control={ + <Switch + checked={!!field.value} + onChange={(e) => field.onChange(e.target.checked)} + /> + } label="dbt Test Failure Notifications" />
179-181: Payload includes the new subscription booleans — good; tighten the submit typeThe payload wiring is correct. Recommend typing the handler to PreferencesFormInput for compile-time safety.
Use:
const onSubmit = async (values: PreferencesFormInput) => { /* ... */ }src/components/Notifications/ManageNotifications.tsx (4)
30-30: Narrow the Notification.category type to a union for safetyAvoid arbitrary strings and keep in sync with backend/categoryOptions.
Example:
type Category = '' | 'incident' | 'schema_change' | 'job_failure' | 'dbt_test_failure' | 'maintenance'; interface Notification { /* ... */ category: Category; }
60-63: State and query wiring — good; consider URL-sync for deep linksState drives the query key correctly. Optionally, sync the selected category to the URL (query param) so the view is shareable and persists on reload.
I can provide a small
useRouterintegration if desired.
135-169: Filter UI — solid; add test id and clear selections on filter changeAdding a test id improves test stability; clearing selections avoids cross-category selection confusion.
Apply this diff:
<Select value={selectedCategory} onChange={(e) => { setSelectedCategory(e.target.value); setCurrentPageIndex(0); // Reset to first page on filter change + setCheckedRows([]); // Clear row selection when filter changes }} displayEmpty + inputProps={{ 'data-testid': 'category-select', 'aria-label': 'Filter by Category' }} sx={{ fontSize: '14px', backgroundColor: '#fff', color: '#0F2440CC',
100-104: Avoid redundant revalidation on pagination changeChanging
pageSize/currentPageIndexupdates the SWR key and triggers a fetch. The explicitmutate()can cause unnecessary extra requests.Apply this diff:
const handlePagination = (newPageIndex: number, newPageSize: number) => { setCurrentPageIndex(newPageIndex); setPageSize(newPageSize); - mutate(); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/components/Notifications/ManageNotifications.tsx(5 hunks)src/components/Notifications/PreferencesForm.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks
🔇 Additional comments (5)
src/components/Notifications/PreferencesForm.tsx (1)
22-24: New subscription fields added — looks goodThe fields are named consistently and align with the category names used elsewhere in the PR.
src/components/Notifications/ManageNotifications.tsx (4)
17-20: UI imports for category filter — goodSelect, MenuItem, and FormControl are appropriate for the new filter.
65-66: SWR key correctly includes category — LGTMIncluding
categoryQueryin the key ensures server-side filtering and cache separation per category.
247-249: Displaying category per row — LGTMClear and consistent with the new filter.
33-41: Ensure category taxonomy aligns with backend and type safety
- The values
incident,schema_change,job_failure,dbt_test_failureandmaintenanceare used consistently across ManageNotifications.tsx and its tests.- Please confirm these are the exact category values returned by the server’s
notifications/v1API.- To prevent divergence over time, extract a shared union type and derive your options from it, for example:
Then update your// src/types/notifications.ts export type Category = | 'incident' | 'schema_change' | 'job_failure' | 'dbt_test_failure' | 'maintenance'; export const categoryOptions: { label: string; value: Category | '' }[] = [ { label: 'All Categories', value: '' }, { label: 'Incident', value: 'incident' }, { label: 'Schema Change', value: 'schema_change' }, { label: 'Job Failure', value: 'job_failure' }, { label: 'DBT Test Failure', value: 'dbt_test_failure' }, { label: 'Maintenance', value: 'maintenance' }, ];Notificationinterface tocategory: Categoryand have tests import these enums directly.
New features:
Summary by CodeRabbit
New Features
Bug Fixes
Tests