Mariano/cleanup - #135
Mariano/cleanup#135
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces significant updates to the evidence dashboard components. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant EL as EvidenceList Component
participant ES as EvidenceSummaryCards
participant EO as EvidenceOverview Component
participant CH as Charts Loader
U->>+EL: Search for Evidence
EL->>+ES: Update Search State
ES-->>EL: Render Search Results
EL-->>U: Display Evidence List
U->>+EO: Open Evidence Overview
EO->>ES: Render Evidence Summary Statistics
ES-->>EO: Display Summary Cards
EO->>CH: Load Chart Components
CH-->>EO: Display Charts
EO-->>U: Render Completed Overview
Possibly related PRs
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
- Added stabilization logic to prevent flashing states during loading and searching. - Introduced a new `isSearching` state to manage search transitions effectively. - Updated `SearchInput` to handle search changes with proper state management and maintain focus during transitions. - Refactored rendering logic in `EvidenceList` to conditionally display loading skeletons and empty states based on search and loading conditions. - Improved context management in `useEvidenceTableContext` to track search state and ensure UI transitions are smooth.
![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 (4)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/EvidenceList.tsx (1)
19-54: Search and data stabilization references are well-structured, but watch out for quick toggles.The usage of
hasDataRef,hasSearchRef, and the series ofuseEffecthooks for handling search transitions and refreshing data after clearing search is logically consistent. However, the reliance on quicksetTimeoutcalls and references may risk race conditions if multiple updates occur in rapid succession. Consider consolidating your approach or ensuring thesesetTimeoutinvocations won’t conflict if a user quickly toggles filters or search inputs.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/hooks/useEvidenceTableContext.tsx (1)
113-127: Effects triggerisSearchingas expected, but consider concurrency.When search parameters update, you set
isSearchingtotrueif not on the initial load. This is a sound approach, though consider carefully how concurrency is handled if multiple search parameters change in quick succession (e.g., user rapidly toggling filters).apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/EvidenceFilters/SearchInput.tsx (2)
24-52: Debounced search logic is handled properly, but watch for frequent calls.Your approach to skip redundant searches if
debouncedValuehasn’t changed and the fallback to reset search when empty is solid. However, you might consider a more robust debounce or an abortable request pattern if users rapidly update the search field.
74-85: Early search trigger on input clearing is helpful.Immediately resetting filters and data on clearing the input is a good UX pattern. Confirm that large data sets aren’t causing an unnecessary load when the input is toggled repeatedly and quickly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/EvidenceList.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/EvidenceFilters/SearchInput.tsx(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/hooks/useEvidenceTableContext.tsx(6 hunks)
🔇 Additional comments (16)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/EvidenceList.tsx (5)
12-13: Imports look correct and concise.The new import of
SkeletonTableis consistent with its usage in the component, and the addition ofuseEffect,useRef, anduseStatefrom React is appropriate given the expanded state management logic.
71-72: Conditional skeleton rendering is clear.Showing
EvidenceListSkeletononly if it’s an initial load (no data) while not searching is a sensible approach to prevent flickering.
79-99:renderTableContentlogic is appropriately layered.Using a dedicated function to decide whether to show the skeleton, empty state, or table helps readability. The condition checks for
isStabilized,isLoading, andisSearchingare coherent.
101-115: Layout updates align with the table-first approach.The new layout with
SearchInput,FilterDropdown, andPaginationControlsis neatly ordered and responsive. The code looks well-structured and consistent with the removal ofEvidenceSummaryCards.
56-69:Details
✅ Verification successful
Delay-based stabilization helps reduce flickering.
Introducing a brief delay before setting
isStabilizedtotrueeffectively prevents flashing states. Ensure you’ve tested scenarios like rapid searching or loading to confirm that user experience remains consistent and smooth.Use the following script to quickly search for repeated setTimeout calls and verify that none conflict:
🏁 Script executed:
#!/bin/bash fd --type f --name "*.tsx" --exec gawk '/setTimeout/ {print FILENAME ":" FNR ":" $0}'Length of output: 338
Action: Verify that delay-based stabilization is isolated and functioning as intended.
The updated EvidenceList component introduces a 100 ms delay before setting
isStabilizedtotrueto mitigate flickering during rapid loading/searching. However, the initial automated search forsetTimeoutusage produced an error due to incorrectfdflag usage. Please re-run the updated script below to search for any conflictingsetTimeoutcalls within your *.tsx files:After reviewing the output, ensure that no other instances of
setTimeoutinterfere with the intended stabilization behavior, especially under rapid search or loading conditions.
🏁 Script executed:
#!/bin/bash # Corrected command to search for all setTimeout calls in .tsx files fd '*.tsx' --type f --exec gawk '/setTimeout/ {print FILENAME ":" FNR ":" $0}' {}Length of output: 492
Action: Re-verify potential conflicting uses of
setTimeoutacross.tsxfilesThe EvidenceList component’s delay-based stabilization (using a 100 ms timeout) appears to work as intended. However, our initial search for
setTimeoutusage encountered errors due to incorrect flag usage. To ensure that no other instances interfere with this behavior—especially under rapid loading or searching—please re-run the following corrected command manually:After verifying that there are no conflicting
setTimeoutcalls elsewhere in your codebase, you can be confident that the stabilization behavior in EvidenceList.tsx is isolated and effective.
🏁 Script executed:
#!/bin/bash # Using --glob to correctly match .tsx files and search for setTimeout usage fd --glob '*.tsx' --type f --exec gawk '/setTimeout/ {print FILENAME ":" FNR ":" $0}' {}Length of output: 1783
Review Verification: Delay-Based Stabilization Works as Expected
The 100 ms delay in
EvidenceList.tsxis effectively isolating the stabilization logic to prevent state flicker during rapid loading/searching sequences. Our search forsetTimeoutcalls in the codebase confirmed that while there are other instances ofsetTimeoutin various components, none conflict with or undermine this specific stabilization behavior.
- Verified Location:
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/EvidenceList.tsx(lines 56–69)Please ensure that manual testing for rapid search/loading scenarios is conducted to confirm a consistently smooth user experience.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/hooks/useEvidenceTableContext.tsx (6)
3-11: New imports are valid.The added imports (
createContext,useRef,useEffect, etc.) match the new functionality introduced in the provider.
57-58: Interface correctly includesisSearching.Ensuring the context interface has
isSearching: boolean;is a good addition for consistent type checking across the application.
88-91: Initial load ref for delayed isSearching is well-intentioned.
initialLoadCompletedhelps differentiate between the first load and subsequent searches. This logic is beneficial to avoid marking the very first load as “searching.”
129-139: Slight delay for UI transitions is helpful, watch for possible user frustration.A 50ms delay is typically negligible, but ensure that under slow network conditions, transitions remain intuitive. If it becomes too long, users might experience lag between acknowledging a completed load and updating the UI.
140-149: Safe reset ofisSearchingafter new data arrives.Having a timed fallback to reset
isSearchingoncerawEvidenceTasksis updated helps mitigate stale loading states. Just ensure that cumulative delays (from setTimeout usage in several places) don’t visibly stall the UI.
210-249: Context value is well-structured, includingisSearchingand new actions.The extended context value provides easy, centralized access to
isSearching, promoting clarity in the consumer components.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/EvidenceFilters/SearchInput.tsx (5)
5-5: ImportinguseRefaligns with new references.No issues with this addition.
16-17: Hook usage is consistent with the updated context.Destructuring
isSearchingandmutatefromuseEvidenceTableensures the input component can trigger refreshes appropriately.
20-23: New references for search management are well-named.
isPendingRefandpreviousSearchRefare clear, descriptive, and help track state transitions effectively.
54-72: Maintaining focus is user-friendly.Re-focusing the input and setting the caret ensures a fluid search experience. Consider if a partial-lag scenario might cause unexpected flickers.
90-95: Ref usage and event handler are consistent.Using
inputReffor focus retention andhandleInputChangefor updates keeps the logic clean and maintainable within React’s controlled component pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (24)
apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/utils.ts (2)
3-14: Add default case in switch statementThe switch statement doesn't have a default case. If a new status is added to the
PolicyStatustype but not handled here, this function would return undefined.export function getStatusStyle(status: PolicyStatus) { switch (status) { case "published": return "bg-[#00DC73]"; case "draft": return "bg-[#ffc107]"; case "needs_review": return "bg-[#ff0000]"; case "archived": return "bg-[#0ea5e9]"; + default: + return "bg-gray-400"; // Default fallback color } }
16-18: Consider handling edge cases in formatStatusThe function doesn't handle the case where status might be empty or undefined. While the typed parameter should prevent this, adding a safeguard could improve robustness.
export function formatStatus(status: PolicyStatus) { + if (!status) return ""; return status.charAt(0).toUpperCase() + status.slice(1).replace(/_/g, " "); }apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/types.ts (1)
6-13: Consider providing more documentation for PoliciesTablePropsThe interface lacks documentation explaining why
usersis required and howctaButtonshould be used. Adding JSDoc comments would improve maintainability.+/** + * Props for the PoliciesTable component + * @property {User[]} users - List of users needed for displaying owner information + * @property {Object} [ctaButton] - Optional call-to-action button configuration + */ export interface PoliciesTableProps { users: User[]; ctaButton?: { label: string; onClick: () => void; icon?: ReactNode; }; }apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/columns.tsx (2)
62-70: Extract date formatting to a utility functionDate formatting logic is embedded in the component. Extracting this to a utility function would improve code reusability and ensure consistent date formatting across the application.
// In utils.ts: +export function formatDate(date: Date): string { + return date.toLocaleDateString("en-US", { + year: "numeric", + month: "short", + day: "numeric", + }); +} // In columns.tsx: -{date.toLocaleDateString("en-US", { - year: "numeric", - month: "short", - day: "numeric", -})} +{formatDate(date)}
50-53: Consider accessibility improvements for status indicatorThe status indicator uses color to convey information, which may not be accessible to all users. Consider adding an aria-label or title attribute.
<div className="hidden md:flex items-center gap-2"> - <div className={`h-2.5 w-2.5 ${getStatusStyle(status)}`} /> + <div + className={`h-2.5 w-2.5 ${getStatusStyle(status)}`} + aria-hidden="true" + title={`Status: ${formatStatus(status)}`} + /> <span className="text-sm">{formatStatus(status)}</span> </div>apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/components/filterCategories.tsx (1)
37-60: Nicely handled fallback for user image.Your fallback approach for user images gracefully handles missing images, while still displaying the user’s first initial. This is user-friendly and avoids broken links.
If you’d like to improve accessibility further, consider adding a descriptive
altin edge cases where the user’s name is unavailable.apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/PoliciesList.tsx (1)
16-19: Unused import for PoliciesListSkeleton.You import
PoliciesListSkeletonbut it’s not used in the rendered output. If it’s no longer required, consider removing it to reduce unused code.-import { PoliciesListSkeleton } from "./PoliciesListSkeleton";apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/PoliciesTable.tsx (1)
70-74: Consider localizing your placeholder.If you’re using
useI18nelsewhere, localizing the “Search policies…” placeholder can offer a more consistent experience for international users.apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/hooks/usePoliciesTableContext.tsx (4)
16-42: Enhance type safety by replacingany[]with a more specific type.The interface defines
policiesasany[] | undefined, which loses type safety. Consider creating or importing a specific Policy interface/type rather than usingany.interface PoliciesTableContextType { // ... // Data - policies: any[] | undefined; + policies: Policy[] | undefined; // ... }You would need to create or import the Policy type at the top of the file.
65-67: Add error handling for page number parsing.The current page number parsing doesn't handle invalid inputs that could come from URL manipulation. Consider adding validation or fallback values.
- const currentPage = Number.parseInt(page, 10); - const currentPageSize = Number.parseInt(pageSize, 10); + const currentPage = Number.isNaN(Number.parseInt(page, 10)) ? 1 : Number.parseInt(page, 10); + const currentPageSize = Number.isNaN(Number.parseInt(pageSize, 10)) ? 10 : Number.parseInt(pageSize, 10);
77-105: Consider simplifying the loading state management.The code uses multiple timeouts and effects to manage loading states, which could lead to race conditions or unnecessary component re-renders. Consider consolidating this logic into a more streamlined approach.
The current implementation uses multiple effects and timeouts to manage the
isSearchingstate:
- One effect to set
isSearchingwhen search params change- Another to reset it when loading completes
- A safety timeout to ensure it eventually gets reset
Consider replacing these with a single effect that handles all these scenarios:
// Remove the three separate effects and replace with: useEffect(() => { // Set searching state when params change and we're not in initial load if (initialLoadCompleted.current && isLoading) { setIsSearching(true); return; } // Reset searching state when loading completes if (!isLoading && isSearching) { // Small delay for UI transitions if needed const timer = setTimeout(() => { setIsSearching(false); // Mark initial load as completed if needed if (!initialLoadCompleted.current) { initialLoadCompleted.current = true; } }, 50); return () => clearTimeout(timer); } }, [isLoading, isSearching, debouncedSearch, status, ownerId, page, pageSize]);
107-109: Add search to thehasActiveFilterscheck.The current implementation only considers
statusandownerIdas active filters, butsearchis also a filtering mechanism that should be included.const hasActiveFilters = useMemo(() => { - return status !== null || ownerId !== null; + return status !== null || ownerId !== null || search.trim() !== ""; }, [status, ownerId, search]);apps/app/src/components/ui/data-table/DataTableSkeleton.tsx (1)
18-47: Well-implemented skeleton component with good defaultsThe component implementation is clean and follows best practices:
- Default values for columns and rows
- Proper key generation for mapped elements
- Consistent use of Skeleton components with appropriate sizing
- Good use of the table component structure from the design system
A minor optimization could be extracting the array generation to avoid recreating arrays on each render.
Consider memoizing the arrays to avoid recreating them on each render:
export function DataTableSkeleton({ columns = 5, rows = 5, className, }: DataTableSkeletonProps) { + const columnArray = React.useMemo(() => Array.from({ length: columns }), [columns]); + const rowArray = React.useMemo(() => Array.from({ length: rows }), [rows]); return ( <Table className={className}> <TableHeader> <TableRow> - {Array.from({ length: columns }).map((_, i) => ( + {columnArray.map((_, i) => ( <TableHead key={`skeleton-header-${i + 1}`}> <Skeleton className="h-4 w-[200px]" /> </TableHead> ))} </TableRow> </TableHeader> <TableBody> - {Array.from({ length: rows }).map((_, i) => ( + {rowArray.map((_, i) => ( <TableRow key={`skeleton-row-${i + 1}`} className="h-[54px]"> - {Array.from({ length: columns }).map((_, j) => ( + {columnArray.map((_, j) => ( <TableCell key={`skeleton-cell-${i + 1}-${j + 1}`}> <Skeleton className="h-4 w-[150px]" /> </TableCell> ))} </TableRow> ))} </TableBody> </Table> ); }apps/app/src/components/ui/data-table/DataTablePagination.tsx (1)
39-84: UI implementation is clean and follows best practicesThe pagination UI is well-structured with:
- Item count display with singular/plural handling
- Page size selector with reasonable options
- Pagination controls with disabled states for boundaries
- Clear display of current page position
One suggestion would be to add accessibility improvements for screen readers.
Consider enhancing accessibility by adding aria attributes to the pagination controls:
<Button variant="outline" size="sm" onClick={() => onPageChange(page - 1)} disabled={!hasPreviousPage} + aria-label="Previous page" > <ChevronLeft className="h-4 w-4" /> </Button> <div className="text-sm font-medium"> - Page {page} of {totalPages} + <span aria-live="polite" aria-atomic="true">Page {page} of {totalPages}</span> </div> <Button variant="outline" size="sm" onClick={() => onPageChange(page + 1)} disabled={!hasNextPage} + aria-label="Next page" > <ChevronRight className="h-4 w-4" /> </Button>apps/app/src/components/ui/data-table/DataTableHeader.tsx (1)
13-62: Well-implemented table header with sorting and resizing capabilitiesThe implementation includes several advanced features:
- Proper rendering of header groups and individual headers
- Sort indicators that change based on sort direction
- Column resizing with visual feedback
- Conditional styling based on column state
The style calculations for header sizing could be improved to avoid potential issues with negative widths.
Consider adding a safeguard for the calculated width to ensure it's always positive:
<div className={cn( "flex items-center overflow-hidden", header.column.getCanSort() && "cursor-pointer select-none", )} - style={{ width: header.getSize() - 32 }} + style={{ width: Math.max(header.getSize() - 32, 0) }} onClick={header.column.getToggleSortingHandler()} >Also, consider adding keyboard accessibility for sorting:
<div className={cn( "flex items-center overflow-hidden", header.column.getCanSort() && "cursor-pointer select-none", )} style={{ width: Math.max(header.getSize() - 32, 0) }} onClick={header.column.getToggleSortingHandler()} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + header.column.getToggleSortingHandler()?.(e); + } + }} + tabIndex={header.column.getCanSort() ? 0 : undefined} + role={header.column.getCanSort() ? "button" : undefined} + aria-label={header.column.getCanSort() + ? `Sort by ${String(header.column.columnDef.header)} ${header.column.getIsSorted() ? 'in ' + (header.column.getIsSorted() === 'asc' ? 'descending' : 'ascending') + ' order' : ''}` + : undefined} >apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/components/AssigneeAvatar.tsx (1)
3-26: Clean implementation with minor accessibility improvements possibleThe component correctly handles both image and fallback states, with good use of conditional rendering. Consider these accessibility improvements:
- Add an aria-label to the fallback div for screen readers
- Consider using Next.js Image component for performance optimization if applicable
return ( - <div className="flex h-5 w-5 items-center justify-center rounded-full bg-muted text-xs"> + <div + className="flex h-5 w-5 items-center justify-center rounded-full bg-muted text-xs" + aria-label={`Avatar for ${assignee.name || "Unknown"}`} + > {(assignee.name || "?").charAt(0)} </div> );apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/components/filterCategories.tsx (1)
10-107: Functional implementation with opportunity for refactoringThe filter categories implementation works correctly, but there's repetitive code in each filter category definition that could be refactored.
Also, I notice the Department and Assignee filters have a
maxHeightproperty (150px) while other filters don't. Is this intentional? Consider applying consistent styling across all filters if appropriate.Consider refactoring to reduce code repetition:
export function getFilterCategories({ status, setStatus, relevance, setRelevance, frequency, setFrequency, department, setDepartment, assigneeId, setAssigneeId, frequencies, departments, assignees, setPage, }: FilterCategoriesProps) { + const createFilterCategory = ( + label: string, + items: any[], + currentValue: string | null, + setValue: (value: string | null) => void, + getLabel: (item: any) => string, + getValue: (item: any) => string, + getIcon?: (item: any) => React.ReactNode, + maxHeight?: string + ) => ({ + label: `Filter by ${label}`, + items: items.map((item) => ({ + label: getLabel(item), + value: getValue(item), + checked: currentValue === getValue(item), + onChange: (checked: boolean) => { + setValue(checked ? getValue(item) : null); + setPage("1"); + }, + ...(getIcon ? { icon: getIcon(item) } : {}), + })), + ...(maxHeight ? { maxHeight } : {}), + }); return [ + createFilterCategory( + "Status", + STATUS_FILTERS, + status, + setStatus, + (filter) => filter.label, + (filter) => filter.value, + (filter) => filter.icon + ), + createFilterCategory( + "Relevance", + RELEVANCE_FILTERS, + relevance, + setRelevance, + (filter) => filter.label, + (filter) => filter.value, + (filter) => filter.icon + ), + createFilterCategory( + "Frequency", + frequencies, + frequency, + setFrequency, + (freq) => freq, + (freq) => freq + ), + createFilterCategory( + "Department", + departments, + department, + setDepartment, + (dept) => dept.replace(/_/g, " ").toUpperCase(), + (dept) => dept, + () => DEPARTMENT_ICON, + "150px" + ), + createFilterCategory( + "Assignee", + assignees, + assigneeId, + setAssigneeId, + (assignee) => assignee.name || "Unknown", + (assignee) => assignee.id, + (assignee) => <AssigneeAvatar assignee={assignee} />, + "150px" + ), - { - label: "Filter by Status", - items: STATUS_FILTERS.map((filter) => ({ - ...filter, - checked: status === filter.value, - onChange: (checked: boolean) => { - setStatus(checked ? filter.value : null); - setPage("1"); - }, - })), - }, - { - label: "Filter by Relevance", - items: RELEVANCE_FILTERS.map((filter) => ({ - ...filter, - checked: relevance === filter.value, - onChange: (checked: boolean) => { - setRelevance(checked ? filter.value : null); - setPage("1"); - }, - })), - }, - { - label: "Filter by Frequency", - items: frequencies.map((freq) => ({ - label: freq, - value: freq, - checked: frequency === freq, - onChange: (checked: boolean) => { - setFrequency(checked ? freq : null); - setPage("1"); - }, - })), - }, - { - label: "Filter by Department", - items: departments.map((dept) => ({ - label: dept.replace(/_/g, " ").toUpperCase(), - value: dept, - checked: department === dept, - onChange: (checked: boolean) => { - setDepartment(checked ? dept : null); - setPage("1"); - }, - icon: DEPARTMENT_ICON, - })), - maxHeight: "150px", - }, - { - label: "Filter by Assignee", - items: assignees.map((assignee) => ({ - label: assignee.name || "Unknown", - value: assignee.id, - checked: assigneeId === assignee.id, - onChange: (checked: boolean) => { - setAssigneeId(checked ? assignee.id : null); - setPage("1"); - }, - icon: <AssigneeAvatar assignee={assignee} />, - })), - maxHeight: "150px", - }, ]; }apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/EvidenceListTable.tsx (1)
12-37: Well-structured state extraction from context.
Pulling multiple values and setters fromuseEvidenceTable()is clean. However, be mindful of potential performance implications if these states cause frequent re-renders.apps/app/src/components/ui/data-table/DataTable.tsx (5)
3-31: Imports are well-organized.
The mixture of table utilities (@tanstack/react-table) and UI components is logically grouped. Just ensure consistent naming conventions across the codebase.
46-79: DataTableProps interface is comprehensive.
Covers data, pagination, search, filters, and potential CTA button. As usage grows, consider moving to separate type definitions to keep files short.
95-96:sortingstate.
Inline local state is fine for sorting. Evaluate if context-based management is needed if more advanced interactions are introduced.
114-140: Search input usability.
Placing the search icon and clear button in the input is user-friendly. If large data sets are involved, consider debouncing input changes to improve performance.
218-227: CTA button.
Optional call-to-action is a neat extension point. Ensure usage of icons is consistent with the design system.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/EvidenceList.tsx (1)
7-9: Simplicity of the newEvidenceList.
By returning<EvidenceListTable>directly, the complexity of loading/error states is removed from this component. Just confirm that these states are handled or unnecessary in the broader flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/EvidenceList.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/EvidenceListUIStates.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/EvidenceFilters/FilterDropdown.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/EvidenceFilters/PaginationControls.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/EvidenceFilters/SearchInput.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/EvidenceListTable.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/SkeletonTable.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/components/AssigneeAvatar.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/components/filterCategories.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/components/filterConfigs.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/hooks/useEvidenceTableContext.tsx(9 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/PoliciesList.tsx(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/PoliciesTable.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/columns.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/components/filterCategories.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/components/filterConfigs.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/hooks/usePoliciesTableContext.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/types.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/utils.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/hooks/usePolicies.ts(1 hunks)apps/app/src/components/ui/data-table/DataTable.tsx(1 hunks)apps/app/src/components/ui/data-table/DataTableHeader.tsx(1 hunks)apps/app/src/components/ui/data-table/DataTablePagination.tsx(1 hunks)apps/app/src/components/ui/data-table/DataTableSkeleton.tsx(1 hunks)apps/app/src/components/ui/data-table/index.ts(1 hunks)apps/app/src/hooks/useDebounce.ts(1 hunks)
💤 Files with no reviewable changes (4)
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/EvidenceFilters/PaginationControls.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/EvidenceFilters/SearchInput.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/EvidenceFilters/FilterDropdown.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/SkeletonTable.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/components/filterConfigs.tsx
- apps/app/src/components/ui/data-table/index.ts
🔇 Additional comments (50)
apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/hooks/usePolicies.ts (1)
53-53:Details
❓ Verification inconclusive
Verify impact of removing fallback value
Removing the fallback value
?? 0fortotalchanges behavior whendata?.totalis undefined. Components consuming this hook may now need to handle undefined values.
🏁 Script executed:
#!/bin/bash # Check which components use the total value from usePolicies rg -A 3 "usePolicies" --glob "*.tsx" | grep -A 2 "total"Length of output: 523
Action: Revisit handling of an undefined
totalvalueThe removal of the fallback (
?? 0) inusePolicies.tsmeans that ifdata?.totalis undefined, any consumer of this hook will receiveundefinedinstead of a default numeric value. Our search shows that the consumer in:
apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/hooks/usePoliciesTableContext.tsxis directly destructuring
totalfromusePolicies. Please verify that this consumer (and any others) can handle an undefinedtotalgracefully. If not, update either the hook to restore a fallback or adjust the consumer to supply a default value.apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/columns.tsx (1)
25-42: The name column implementation looks goodThe name column provides good user interaction by making the policy name clickable, and the hover underline provides a clear affordance that it's interactive.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/components/filterCategories.tsx (2)
6-13: Interface props look solid.The
FilterCategoriesPropsinterface clearly describes the expected states and setters for filtering logic. This keeps the filter configuration modular and easy to maintain.
23-33: Resetting page to "1" for every status change is sensible.This ensures the user sees the first batch of filtered results. The implementation is straightforward and avoids stale page offsets.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/PoliciesList.tsx (1)
33-37: Context provider integration looks clean.Wrapping
<PoliciesTable>with<PoliciesTableProvider>cleanly separates your table’s state from the rest of the application. This approach should simplify maintenance.apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/PoliciesTable.tsx (3)
38-45: Centralizing filter logic.Using
getFilterCategorieshere keeps the filter definition separate from the table UI, ensuring better readability. Good job on maintaining a clear separation of concerns.
47-59: Robust pagination handling.It’s helpful that pagination is conditionally computed only when
totalexists. This design choice prevents undefined behavior during data loads or error states.
82-85: Proactive approach to adding new policies.Providing a clear call to action for creating new policies is helpful for users. The icon usage also follows a modern UI pattern.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/hooks/usePoliciesTableContext.tsx (6)
154-164: LGTM - Well-implemented context consumer hook.The
usePoliciesTablehook follows best practices by checking for context existence and providing a clear error message if used improperly.
1-15: LGTM - Proper imports and client directive.The imports are appropriate for the context implementation, and the "use client" directive correctly indicates this is a client component in Next.js.
44-46: LGTM - Standard context creation pattern.The context is created with an initial undefined value, which is the recommended pattern for React contexts that require a provider.
111-118: LGTM - Comprehensive filter clearing function.The
clearFiltersfunction properly resets all filters including search and pagination, which ensures a clean state for new searches.
119-146: LGTM - Well-structured context value object.The context value is well-organized into logical sections (state, setters, data, derived data, actions) making the code more maintainable and easier to understand.
147-152: LGTM - Standard provider implementation.The provider correctly wraps children with the context value, following the standard React pattern for context providers.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/EvidenceListUIStates.tsx (1)
12-16: Interface definition looks good and improves component props typingThe new
EmptyStatePropsinterface clearly defines the expected properties for the empty state component, with appropriate optional typing. This matches its usage in theEvidenceListEmptycomponent.apps/app/src/components/ui/data-table/DataTableSkeleton.tsx (2)
1-11: Appropriate imports for the skeleton componentThe imports are well-organized and include all necessary UI components from the design system.
12-16: Props interface is well-definedThe
DataTableSkeletonPropsinterface has appropriate optional properties with clear types.apps/app/src/components/ui/data-table/DataTablePagination.tsx (3)
1-12: Client component directive and imports look goodThe "use client" directive is correctly placed at the top, and all necessary imports for the UI components are included.
13-22: Props interface is well-defined with all required pagination propertiesThe interface clearly defines all properties needed for pagination functionality with appropriate types.
24-38: Good handling of page size changesThe implementation correctly resets to the first page when changing page size, which provides a good user experience.
apps/app/src/components/ui/data-table/DataTableHeader.tsx (2)
1-8: Client component directive and imports look goodThe "use client" directive is correctly placed, and all necessary imports from Tanstack Table and UI components are included.
9-11: Generic props interface is well-definedThe interface correctly uses a generic type parameter for the table data, providing type safety.
apps/app/src/hooks/useDebounce.ts (1)
3-12: Well-implemented debounce hook!The
useDebouncehook follows React best practices with proper cleanup to prevent memory leaks and correct dependency array usage. The type implementation with generics is clean and allows the hook to work with any data type.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/components/filterConfigs.tsx (1)
3-31: Well-structured filter configuration with type safetyGood job using TypeScript's
as constassertion to ensure type safety for the filter arrays. The icons have appropriate color coding for visual clarity (green for positive states, red/yellow for negative/warning states).apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/components/table/EvidenceListTable.tsx (5)
3-3: Looks good.
No issues spotted with importing the newDataTablecomponent.
7-8: Imports appear valid.
These imports from the local hooks and helper (useEvidenceTable,getFilterCategories) seem consistent with the updated file structure.
43-49: Efficient filter count calculation.
filter(Boolean)to count active filters is straightforward. Consider adding unit tests if these active filter counts drive important UI logic.
51-66: Functional approach for filter categories.
getFilterCategoriescleanly centralizes filter logic and definitions. Confirm no cyclical dependencies if referencing context from within.
69-96: Data flow and table rendering appear logical.
- Passing data defaults to an empty array is a defensive measure.
- Pagination, search, and filter props are well integrated.
handleRowClickwithrouter.pushis straightforward.
Ensure error handling is performed elsewhere ifrouter.pushfails.apps/app/src/components/ui/data-table/DataTable.tsx (8)
1-2: Client-side usage.
Declaring"use client";ensures this component runs in the browser context as needed.
32-38: FilterItem interface design looks good.
It captures label, value, andcheckedstate, plus an optional icon. This is a straightforward approach for filter definitions.
40-45: Clear grouping for filter categories.
FilterCategoryseparates label and items. The optionalmaxHeightis a nice touch for controlling overflow.
81-94: DataTable function argues a generic TData type.
A good pattern for reusable tables. Keep an eye on any future generics constraints if advanced typed columns become necessary.
97-112:useReactTablesetup is standard.
Enables core row and sorted row models. Column resizing is well handled. No obvious issues spotted.
141-217: Dropdown-driven filter UI.
- Grouping filter categories by label is clear.
- The
maxHeightwith scroll handling is thoughtful.- The “Clear all filters” button is a nice UX addition.
230-295: Table rendering and row interaction.
- Skeleton usage for
isLoadingmakes for a better user experience.- Row-level click callbacks are handled neatly.
- Column resizing handle is properly implemented.
298-305: Pagination controls.
Only rendered if pagination props and event handlers are provided. This dynamic approach is neat and minimal.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/hooks/useEvidenceTableContext.tsx (13)
3-11: Refactored imports for enhanced state managementThe imports have been updated to include additional React hooks (
useState,useRef,useEffect) which support the new state management approach in this component. These additions are appropriate for the changes being implemented.
18-18: New debounce functionality for optimized search experienceThe addition of the
useDebouncehook is a good practice that will prevent excessive API calls while users are typing in the search field, improving performance and user experience.
26-30: Well-structured Filter interfaceThe new
Filterinterface provides a clear structure for filter options with properties for label, value, and checked status, making the code more maintainable and type-safe.
32-68: Enhanced context type with search and filter improvementsThe
EvidenceTableContextTypehas been updated to include the newfiltersarray andisSearchingstate, along with their respective setters. These additions properly support the new searching and filtering functionality.
85-88: Local search state with debouncingTransitioning from
useQueryStateto localuseStatefor search, combined with debouncing, is a good architectural choice that will improve performance by reducing unnecessary renders and API calls.
100-107: Comprehensive filter state initializationThe filter state is initialized with common filtering options (Published, Draft, Relevant, Not Relevant). This approach centralizes filter management and makes the code more maintainable.
108-111: Improved loading state trackingThe addition of
initialLoadCompletedref andisSearchingstate allows for distinguishing between initial data loading and subsequent search/filter operations, which can enhance the user experience by providing appropriate visual feedback.
123-123: Using debounced search in API callThe API call now uses the debounced search value, which will reduce unnecessary server requests while users are typing, improving both performance and server load.
134-147: Reactive search state managementThis effect properly sets the
isSearchingstate when search parameters change, but only after the initial load. This ensures the user gets appropriate feedback during search operations without showing loading indicators during the initial page load.
150-158: Clean loading state resetThe effect that tracks when loading finishes includes a small delay, which is a thoughtful addition to ensure UI transitions appear smooth to users. The approach of using
setTimeouthere is appropriate for managing visual state transitions.
160-169: Safety mechanism for search stateThis additional effect acts as a safety net to ensure the
isSearchingstate is eventually set to false when data changes, preventing potential UI state issues. The timeout is properly cleaned up, avoiding memory leaks.
228-229: Complete filter reset in clearFiltersThe
clearFiltersfunction now also resets the filter checkboxes and search input, providing a more comprehensive clearing of filters. This ensures a consistent user experience when users want to start fresh.
232-261: Updated context value with new propertiesThe context value now includes the new
filters,setFilters, andisSearchingproperties, making them available throughout the component tree. This is a necessary update to support the enhanced filtering and searching functionality.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (20)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/hooks/useEvidenceTableContext.tsx (2)
83-109: Improved state management with predefined filters.The transition from useQueryState to useState for search management simplifies client-side filtering. The predefined filter options provide structure but might limit flexibility if filter options need to be dynamic in the future.
Consider making filter options more dynamic by deriving them from API data or component props rather than hardcoding them, especially if they might change based on business requirements:
- const [filters, setFilters] = useState<Filter[]>([ - { label: "Published", value: "published", checked: false }, - { label: "Draft", value: "draft", checked: false }, - { label: "Relevant", value: "relevant", checked: false }, - { label: "Not Relevant", value: "not-relevant", checked: false }, - ]); + // Accept filter options as props or derive from context + const [filters, setFilters] = useState<Filter[]>( + filterOptions.map(option => ({ ...option, checked: false })) + );
131-168: Carefully orchestrated loading and search state management.The three useEffect hooks work together to manage the
isSearchingstate throughout different phases of the data loading lifecycle. The small timeouts introduce intentional delays for UI transitions.The arbitrary timeout values (50ms, 100ms) might need adjustment based on actual UI testing. Consider extracting these as named constants for clarity:
+ const UI_TRANSITION_DELAY = 50; + const SEARCH_RESET_DELAY = 100; // In the useEffect hooks: - setTimeout(() => { ... }, 50); + setTimeout(() => { ... }, UI_TRANSITION_DELAY); - setTimeout(() => { ... }, 100); + setTimeout(() => { ... }, SEARCH_RESET_DELAY);apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/components/filterCategories.tsx (1)
5-9: Interface naming could be more consistent with exportsThe interface
GetFilterCategoriesPropsdoesn't match the naming pattern of the exported function. Consider renaming it toFilterCategoriesPropsto maintain consistency with the export name.-interface GetFilterCategoriesProps { +interface FilterCategoriesProps { role: string; setRole: (value: string | null) => void; setPage: (value: string) => void; }apps/app/src/components/ui/data-table/DataTable.tsx (4)
98-107: Consider extracting the debounce logic into a custom hookThe search debounce logic is implemented directly in the component. For better reusability and separation of concerns, consider extracting this into a custom hook like
useDebounce.+function useDebounce<T>(value: T, delay: number, callback: (value: T) => void): void { + useEffect(() => { + const timer = setTimeout(() => { + callback(value); + }, delay); + + return () => { + clearTimeout(timer); + }; + }, [value, delay, callback]); +} // In the component: -useEffect(() => { - const timer = setTimeout(() => { - search?.onChange(searchValue); - }, 300); - - return () => { - clearTimeout(timer); - }; -}, [searchValue, search]); +useDebounce(searchValue, 300, (value) => search?.onChange(value));
284-294: Avoid inline conditional styling for complex expressionsThe column resizer uses inline conditional styling with a template literal that includes a complex conditional. This can be hard to read and maintain. Consider using the
cnutility you're already importing.-<div - className={`absolute right-0 top-0 h-full w-1 cursor-col-resize select-none touch-none bg-border opacity-0 hover:opacity-100 ${ - table.getState().columnSizingInfo - .isResizingColumn === cell.column.id - ? "bg-primary opacity-100" - : "" - }`} - onClick={(e) => { - // Stop propagation to prevent row click when resizing - e.stopPropagation(); - }} +<div + className={cn( + "absolute right-0 top-0 h-full w-1 cursor-col-resize select-none touch-none bg-border opacity-0 hover:opacity-100", + table.getState().columnSizingInfo.isResizingColumn === cell.column.id && "bg-primary opacity-100" + )} + onClick={(e) => { + // Stop propagation to prevent row click when resizing + e.stopPropagation(); + }}
260-298: Consider extracting row rendering logic to improve readabilityThe row rendering logic is quite complex. Extracting it to a separate component or function would improve readability and maintainability of the
DataTablecomponent.
32-44: FilterCategory interface duplicateThe
FilterCategoryinterface defined here is duplicated in thetypes.tsfile. Consider importing it from a shared location to avoid duplication.-interface FilterCategory { - label: string; - items: FilterItem[]; - maxHeight?: string; -} +import { FilterCategory } from "path/to/shared/types";Also applies to: 40-45
apps/app/src/app/[locale]/(app)/(dashboard)/people/hooks/useEmployees.ts (1)
53-54: Consider documenting the reason for disabling automatic revalidationChanging
revalidateOnFocusandrevalidateOnReconnecttofalseis a significant change that affects when data is refreshed. Consider adding a comment explaining the rationale for this change.{ + // Disabled automatic revalidation to prevent unnecessary API calls + // Data is now only refreshed when explicitly requested revalidateOnFocus: false, revalidateOnReconnect: false, }apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/types.ts (2)
3-22: Consider using existing User type for users arrayThe
usersproperty inEmployeesTablePropshas a structure that's very similar to theUsertype you're importing from@bubba/db. Consider leveraging this type to avoid duplication and ensure consistency.export interface EmployeesTableProps { columnHeaders: { name: string; email: string; department: string; status: string; }; - users: Array<{ - id: string; - name: string | null; - full_name: string | null; - email: string | null; - role: string; - onboarded: boolean; - emailVerified: Date | null; - image: string | null; - lastLogin: Date | null; - organizationId: string | null; - }>; + users: Array<Pick<User, 'id' | 'name' | 'full_name' | 'email' | 'role' | 'onboarded' | 'emailVerified' | 'image' | 'lastLogin' | 'organizationId'>>; }
24-34: Consider sharing the FilterCategory interfaceThe
FilterCategoryinterface is duplicate with what's defined in theDataTable.tsxfile. Consider moving shared interfaces to a common types file to prevent duplication and maintain consistency across the application.apps/app/src/components/sheets/invite-user-sheet.tsx (2)
136-139: Simplify conditional rendering with identical outcomesThe ternary operator is unnecessary since both outcomes render the same text:
t("people.invite.submit").- {isMutating - ? t("people.invite.submit") - : t("people.invite.submit")} + {t("people.invite.submit")}
38-49: Rename file to match component nameThe component has been renamed from
InviteUserSheettoEmployeeInviteSheet, but the filename remainsinvite-user-sheet.tsx. Consider updating the filename toemployee-invite-sheet.tsxto maintain consistency.apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/columns.tsx (2)
24-24: Add fallback handling for empty namesWhile there's a fallback for missing first letters (
employee.name[0] || "?"), consider handling the case whereemployee.nameis undefined or empty to avoid potential runtime errors.-<AvatarFallback>{employee.name[0] || "?"}</AvatarFallback> +<AvatarFallback>{employee.name?.[0] || "?"}</AvatarFallback>
40-40: Add type safety for department valueThe department is cast as a string, but consider using the
Departmentstype from Prisma for better type safety, matching what's used in theEmployeeInviteSheetcomponent.-const department = row.getValue("department") as string; +import type { Departments } from "@prisma/client"; +const department = row.getValue("department") as Departments;apps/app/src/components/tables/tests/empty-states.tsx (1)
46-46: Consider using relative positioning instead of absoluteUsing
absolutepositioning with a fixed width can cause layout issues on different screen sizes. Consider using relative positioning with flexbox for better responsiveness.-<div className="mt-24 absolute w-full top-0 left-0 flex items-center justify-center z-20"> +<div className="mt-24 relative w-full flex items-center justify-center">apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/EmployeesTable.tsx (2)
32-42: Consider extracting pagination logic to a separate functionThe pagination calculation logic could be extracted to a separate function to improve readability and make it easier to test.
+ const calculatePagination = (page: number, per_page: number, total?: number) => { + if (total === undefined) return undefined; + return { + page: Number(page), + pageSize: Number(per_page), + totalCount: total, + totalPages: Math.ceil(total / Number(per_page)), + hasNextPage: Number(page) * Number(per_page) < total, + hasPreviousPage: Number(page) > 1, + }; + }; - // Calculate pagination values only when total is defined - const pagination = - total !== undefined - ? { - page: Number(page), - pageSize: Number(per_page), - totalCount: total, - totalPages: Math.ceil(total / Number(per_page)), - hasNextPage: Number(page) * Number(per_page) < total, - hasPreviousPage: Number(page) > 1, - } - : undefined; + const pagination = calculatePagination(Number(page), Number(per_page), total);
50-50: Add error handling for row click navigationConsider adding error handling for the navigation in
handleRowClickto gracefully handle potential failures when navigating to employee details.const handleRowClick = (employeeId: string) => { - router.push(`/people/${employeeId}`); + try { + router.push(`/people/${employeeId}`); + } catch (error) { + console.error("Failed to navigate to employee details:", error); + // Consider adding a toast notification here + } };apps/app/src/app/[locale]/(app)/(dashboard)/people/types.ts (3)
8-17: Good error centralization approachCentralizing error definitions with consistent structure is a good practice. Consider adding JSDoc comments to document when each error type should be used.
export const appErrors = { + /** + * Used when a user attempts to access a resource they don't have permission for + */ UNAUTHORIZED: { code: "UNAUTHORIZED", message: "You are not authorized to access this resource", }, + /** + * Used for unexpected server or application errors + */ UNEXPECTED_ERROR: { code: "UNEXPECTED_ERROR", message: "An unexpected error occurred", }, };
26-31: Schema matches interface structureThe Zod schema correctly validates the structure defined in the
EmployeesInputinterface. You might consider adding min/max constraints for numeric fields.export const employeesInputSchema = z.object({ search: z.string().optional(), role: z.string().optional(), - page: z.number().optional(), - per_page: z.number().optional(), + page: z.number().int().positive().optional(), + per_page: z.number().int().positive().max(100).optional(), });
38-44: Comprehensive employee interfaceThe
Employeeinterface provides a clear structure for employee data. Consider adding more specific types for thestatusfield if there are predefined status values.export interface Employee { id: string; name: string; email: string; department: string; - status: string; + status: 'active' | 'inactive' | 'pending' | 'terminated'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/hooks/useEvidenceTableContext.tsx(9 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/people/actions/get-employees.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/people/components/EmployeesList.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/EmployeesTable.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/columns.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/components/filterCategories.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/hooks/useEmployeesTableContext.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/types.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/people/hooks/useEmployees.ts(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/people/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/people/types.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/hooks/usePoliciesTableContext.tsx(1 hunks)apps/app/src/components/sheets/invite-user-sheet.tsx(1 hunks)apps/app/src/components/tables/tests/empty-states.tsx(1 hunks)apps/app/src/components/ui/data-table/DataTable.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/app/src/app/[locale]/(app)/(dashboard)/people/actions/get-employees.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/(overview)/components/table/hooks/usePoliciesTableContext.tsx
🔇 Additional comments (26)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/hooks/useEvidenceTableContext.tsx (5)
3-11: New imports for enhanced state management.The addition of React hooks like
useState,useRef, anduseEffectfacilitates the implementation of a more sophisticated state management approach for search functionality and UI states.
25-29: Well-structured Filter interface.The new Filter interface provides a clear structure for filter options with appropriate properties. This adds type safety and improves code readability.
218-228: Comprehensive filter clearing logic.The enhanced
clearFiltersfunction now properly resets all state variables including the new filters and search states, ensuring a consistent user experience when clearing filters.
230-269: Updated context value with new filter-related properties.The context value has been properly updated to include all new state properties and methods, maintaining consistency with the interface definition.
31-77:Details
❓ Verification inconclusive
Updated context interface with enhanced search and filter capabilities.
The EvidenceTableContextType interface has been appropriately updated to support the new filtering functionality and search state management. The signature change for
setSearchneeds careful attention to ensure all consuming components handle the updated method signature correctly.
🏁 Script executed:
#!/bin/bash # Find components that might be using the old setSearch signature with null rg "setSearch\(null\)" --type tsxLength of output: 63
Action Required: Verify Component Consumption of Updated setSearch Signature
After re-running the search using a refined script to target TSX files, no instances of invoking
setSearchwith anullargument were detected. This suggests that most, if not all, consuming components have been updated to conform with the revised method signature. However, due to the initial script error (i.e., the "unrecognized file type: tsx" message) and the low-confidence output in our automated scan, a manual inspection is recommended to ensure that all consumers have been adjusted accordingly.
- Ensure none of the components call
setSearch(null).- Manually review key components that consume the
EvidenceTableContextTypeto verify they handle the updated non-null signature correctly.apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/components/filterCategories.tsx (1)
11-41: The implementation looks clean and well-structuredThe
getFilterCategoriesfunction properly handles filter state management with appropriate callbacks that update the role state and reset pagination when filters change. This follows React best practices for state management in filter components.apps/app/src/components/ui/data-table/DataTable.tsx (1)
109-124: Good use of TanStack Table configuration optionsThe table setup with TanStack Table is well-configured with appropriate options for sorting, column resizing, and default column sizes. The state management for sorting is properly implemented.
apps/app/src/app/[locale]/(app)/(dashboard)/people/hooks/useEmployees.ts (1)
38-43: Good refactoring of the hook parametersThe refactored function now accepts parameters directly as an object with default values, making it more flexible and easier to use in different contexts. This is a good practice for hooks that accept multiple parameters.
apps/app/src/components/sheets/invite-user-sheet.tsx (1)
44-49: LGTM: Improved useEmployees initialization with default parametersProviding default parameters to
useEmployeesimproves code readability and ensures consistent behavior.apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/columns.tsx (1)
12-63: LGTM: Well-structured column definitions with responsive design considerationThe column definitions are well-organized and include responsive design considerations (hiding status on small screens). The use of
ColumnDeftype ensures type safety.apps/app/src/components/tables/tests/empty-states.tsx (2)
3-3: LGTM: Updated import to use renamed componentThe import has been correctly updated to use the renamed
EmployeeInviteSheetcomponent.
57-57: LGTM: Component usage updated to match renamed componentThe component usage has been correctly updated to use the renamed
EmployeeInviteSheetcomponent.apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/EmployeesTable.tsx (1)
11-69: LGTM: Well-structured component with clear separation of concernsThe
EmployeesTablecomponent is well-organized with appropriate state management, pagination logic, and event handling. The conditional pagination calculation ensures that pagination values are only calculated when the total count is available.apps/app/src/app/[locale]/(app)/(dashboard)/people/components/EmployeesList.tsx (3)
3-4: Good use of the new imports for a context-based approach.
These imports cleanly separate the provider logic (EmployeesTableProvider) from the table component (EmployeesTable), promoting modularity.
6-6: Simplified component signature looks good.
Removing unused props reduces complexity, simplifying the code.
8-10: Neat provider usage.
Wrapping<EmployeesTable>within<EmployeesTableProvider>is a clean way to share state and actions with consuming components.apps/app/src/app/[locale]/(app)/(dashboard)/people/page.tsx (2)
9-23: Streamlined route params handling.
Extracting the locale from a promise and redirecting iforganizationIdis missing ensures proper security and UX. The code reads cleanly.
27-38: Metadata generation logic is straightforward.
Storing locale, retrieving translations, and returning a localized page title is an effective design.apps/app/src/app/[locale]/(app)/(dashboard)/people/components/table/hooks/useEmployeesTableContext.tsx (5)
1-2: No issues with the client directive.
This is necessary for using hooks in a serverless/Next.js environment.
3-15: Imports are consistent and succinct.
All dependencies (createContext,useContext, etc.) are correctly imported; code organization looks good.
16-34: Context interface captures relevant state and actions.
Defining a clear interface for the table’s search, pagination, and filters helps maintain clarity.
36-39: Context creation is well-structured.
Providing a typed context ensures type safety for all consuming components.
130-139: Custom hook design is solid.
It enforces proper usage by ensuring the context is only accessed within its provider, preventing runtime errors.apps/app/src/app/[locale]/(app)/(dashboard)/people/types.ts (3)
1-2: Good Zod inclusion for schema validationThe Zod library is an excellent choice for runtime validation of data structures, providing strong TypeScript integration.
3-6: Well-designed error interfaceThe
AppErrorinterface provides a clean, simple structure for standardizing error handling across the application with appropriate code and message fields.
19-24: Clean input parameter interfaceGood design for query parameters with all fields marked as optional, allowing for flexible API calls.
| return { | ||
| employees: data?.employees ?? [], | ||
| total: data?.total ?? 0, | ||
| total: data?.total, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing fallback value for total
The fallback value for total has been removed, which could lead to undefined being returned. Consider keeping a fallback value to ensure consistent data shape.
- total: data?.total,
+ total: data?.total ?? 0,📝 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.
| total: data?.total, | |
| total: data?.total ?? 0, |
| export function EmployeesTableProvider({ children }: { children: ReactNode }) { | ||
| // Local state for search with debounce | ||
| const [search, setSearch] = useState(""); | ||
| const [debouncedSearch, setDebouncedSearch] = useState(""); | ||
| const searchTimeoutRef = useRef<NodeJS.Timeout | undefined>(undefined); | ||
|
|
||
| // Query state for filters | ||
| const [role, setRole] = useQueryState("role"); | ||
| const [page, setPage] = useQueryState("page", { defaultValue: "1" }); | ||
| const [per_page, setPerPage] = useQueryState("per_page", { | ||
| defaultValue: "10", | ||
| }); | ||
|
|
||
| // Loading states | ||
| const [isSearching, setIsSearching] = useState(false); | ||
| const totalCountRef = useRef<number>(0); | ||
|
|
||
| // Fetch data | ||
| const { employees, total, isLoading, error } = useEmployees({ | ||
| search: debouncedSearch, | ||
| role: role ?? "", | ||
| page: Number(page), | ||
| per_page: Number(per_page), | ||
| }); | ||
|
|
||
| // Update debounced search | ||
| useEffect(() => { | ||
| setIsSearching(true); | ||
| if (searchTimeoutRef.current) { | ||
| clearTimeout(searchTimeoutRef.current); | ||
| } | ||
| searchTimeoutRef.current = setTimeout(() => { | ||
| setDebouncedSearch(search); | ||
| setIsSearching(false); | ||
| }, 300); | ||
|
|
||
| return () => { | ||
| if (searchTimeoutRef.current) { | ||
| clearTimeout(searchTimeoutRef.current); | ||
| } | ||
| }; | ||
| }, [search]); | ||
|
|
||
| // Cache total count | ||
| useEffect(() => { | ||
| if (total !== undefined) { | ||
| totalCountRef.current = total; | ||
| } | ||
| }, [total]); | ||
|
|
||
| // Clear filters | ||
| const clearFilters = async () => { | ||
| setSearch(""); | ||
| setRole(null); | ||
| setPage("1"); | ||
| setPerPage("10"); | ||
| }; | ||
|
|
||
| // Calculate if there are active filters | ||
| const hasActiveFilters = useMemo(() => { | ||
| return Boolean(role); | ||
| }, [role]); | ||
|
|
||
| const value = { | ||
| // State | ||
| search, | ||
| setSearch, | ||
| role: role ?? "", | ||
| setRole: (value: string) => setRole(value || null), | ||
| page: Number(page), | ||
| setPage: (value: number) => setPage(String(value)), | ||
| per_page: Number(per_page), | ||
| setPerPage: (value: number) => setPerPage(String(value)), | ||
| employees, | ||
| total: totalCountRef.current, | ||
| isLoading, | ||
| isSearching, | ||
| hasActiveFilters, | ||
|
|
||
| // Actions | ||
| clearFilters, | ||
| }; | ||
|
|
||
| return ( | ||
| <EmployeesTableContext.Provider value={value}> | ||
| {children} | ||
| </EmployeesTableContext.Provider> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling the error state to improve user awareness.
Although the provider fetches employees data using useEmployees, the error returned from the hook is never displayed or handled, which could hamper user experience and debugging.
// Within EmployeesTableProvider:
const { employees, total, isLoading, error } = useEmployees({
...
});
+ // Example: Expose an error flag or message
+ const [fetchError, setFetchError] = useState<AppError | null>(null);
+ useEffect(() => {
+ if (error) {
+ setFetchError(error);
+ }
+ }, [error]);
const value = {
...
+ fetchError,
};📝 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.
| export function EmployeesTableProvider({ children }: { children: ReactNode }) { | |
| // Local state for search with debounce | |
| const [search, setSearch] = useState(""); | |
| const [debouncedSearch, setDebouncedSearch] = useState(""); | |
| const searchTimeoutRef = useRef<NodeJS.Timeout | undefined>(undefined); | |
| // Query state for filters | |
| const [role, setRole] = useQueryState("role"); | |
| const [page, setPage] = useQueryState("page", { defaultValue: "1" }); | |
| const [per_page, setPerPage] = useQueryState("per_page", { | |
| defaultValue: "10", | |
| }); | |
| // Loading states | |
| const [isSearching, setIsSearching] = useState(false); | |
| const totalCountRef = useRef<number>(0); | |
| // Fetch data | |
| const { employees, total, isLoading, error } = useEmployees({ | |
| search: debouncedSearch, | |
| role: role ?? "", | |
| page: Number(page), | |
| per_page: Number(per_page), | |
| }); | |
| // Update debounced search | |
| useEffect(() => { | |
| setIsSearching(true); | |
| if (searchTimeoutRef.current) { | |
| clearTimeout(searchTimeoutRef.current); | |
| } | |
| searchTimeoutRef.current = setTimeout(() => { | |
| setDebouncedSearch(search); | |
| setIsSearching(false); | |
| }, 300); | |
| return () => { | |
| if (searchTimeoutRef.current) { | |
| clearTimeout(searchTimeoutRef.current); | |
| } | |
| }; | |
| }, [search]); | |
| // Cache total count | |
| useEffect(() => { | |
| if (total !== undefined) { | |
| totalCountRef.current = total; | |
| } | |
| }, [total]); | |
| // Clear filters | |
| const clearFilters = async () => { | |
| setSearch(""); | |
| setRole(null); | |
| setPage("1"); | |
| setPerPage("10"); | |
| }; | |
| // Calculate if there are active filters | |
| const hasActiveFilters = useMemo(() => { | |
| return Boolean(role); | |
| }, [role]); | |
| const value = { | |
| // State | |
| search, | |
| setSearch, | |
| role: role ?? "", | |
| setRole: (value: string) => setRole(value || null), | |
| page: Number(page), | |
| setPage: (value: number) => setPage(String(value)), | |
| per_page: Number(per_page), | |
| setPerPage: (value: number) => setPerPage(String(value)), | |
| employees, | |
| total: totalCountRef.current, | |
| isLoading, | |
| isSearching, | |
| hasActiveFilters, | |
| // Actions | |
| clearFilters, | |
| }; | |
| return ( | |
| <EmployeesTableContext.Provider value={value}> | |
| {children} | |
| </EmployeesTableContext.Provider> | |
| ); | |
| } | |
| export function EmployeesTableProvider({ children }: { children: ReactNode }) { | |
| // Local state for search with debounce | |
| const [search, setSearch] = useState(""); | |
| const [debouncedSearch, setDebouncedSearch] = useState(""); | |
| const searchTimeoutRef = useRef<NodeJS.Timeout | undefined>(undefined); | |
| // Query state for filters | |
| const [role, setRole] = useQueryState("role"); | |
| const [page, setPage] = useQueryState("page", { defaultValue: "1" }); | |
| const [per_page, setPerPage] = useQueryState("per_page", { | |
| defaultValue: "10", | |
| }); | |
| // Loading states | |
| const [isSearching, setIsSearching] = useState(false); | |
| const totalCountRef = useRef<number>(0); | |
| // Fetch data | |
| const { employees, total, isLoading, error } = useEmployees({ | |
| search: debouncedSearch, | |
| role: role ?? "", | |
| page: Number(page), | |
| per_page: Number(per_page), | |
| }); | |
| // Example: Expose an error flag or message | |
| const [fetchError, setFetchError] = useState<AppError | null>(null); | |
| useEffect(() => { | |
| if (error) { | |
| setFetchError(error); | |
| } | |
| }, [error]); | |
| // Update debounced search | |
| useEffect(() => { | |
| setIsSearching(true); | |
| if (searchTimeoutRef.current) { | |
| clearTimeout(searchTimeoutRef.current); | |
| } | |
| searchTimeoutRef.current = setTimeout(() => { | |
| setDebouncedSearch(search); | |
| setIsSearching(false); | |
| }, 300); | |
| return () => { | |
| if (searchTimeoutRef.current) { | |
| clearTimeout(searchTimeoutRef.current); | |
| } | |
| }; | |
| }, [search]); | |
| // Cache total count | |
| useEffect(() => { | |
| if (total !== undefined) { | |
| totalCountRef.current = total; | |
| } | |
| }, [total]); | |
| // Clear filters | |
| const clearFilters = async () => { | |
| setSearch(""); | |
| setRole(null); | |
| setPage("1"); | |
| setPerPage("10"); | |
| }; | |
| // Calculate if there are active filters | |
| const hasActiveFilters = useMemo(() => { | |
| return Boolean(role); | |
| }, [role]); | |
| const value = { | |
| // State | |
| search, | |
| setSearch, | |
| role: role ?? "", | |
| setRole: (value: string) => setRole(value || null), | |
| page: Number(page), | |
| setPage: (value: number) => setPage(String(value)), | |
| per_page: Number(per_page), | |
| setPerPage: (value: number) => setPerPage(String(value)), | |
| employees, | |
| total: totalCountRef.current, | |
| isLoading, | |
| isSearching, | |
| hasActiveFilters, | |
| // Actions | |
| clearFilters, | |
| fetchError, | |
| }; | |
| return ( | |
| <EmployeesTableContext.Provider value={value}> | |
| {children} | |
| </EmployeesTableContext.Provider> | |
| ); | |
| } |
| export interface EmployeesResponse { | ||
| employees: any[]; | ||
| total: number; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type safety improvement needed
The employees property uses any[] which lacks type safety. Consider using Employee[] instead to provide better type checking and documentation.
export interface EmployeesResponse {
- employees: any[];
+ employees: Employee[];
total: number;
}📝 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.
| export interface EmployeesResponse { | |
| employees: any[]; | |
| total: number; | |
| } | |
| export interface EmployeesResponse { | |
| employees: Employee[]; | |
| total: number; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
apps/app/src/components/forms/risks/create-risk-form.tsx (2)
95-97: Consider enhancing error handlingWhile the error case shows a toast notification, there's an opportunity to provide more detailed error feedback to the user based on the specific error that occurred.
- onError: () => { - toast.error(t("risk.form.create_risk_error")); - }, + onError: (error) => { + console.error("Error creating risk:", error); + toast.error(t("risk.form.create_risk_error")); + },
291-302: Consider adding a loading state to the submit buttonWhile the button is correctly disabled during form submission, adding a loading spinner or text would provide better visual feedback to users.
<Button type="submit" variant="action" disabled={createRisk.status === "executing"} > <div className="flex items-center justify-center"> - {t("common.actions.create")} + {createRisk.status === "executing" ? t("common.actions.creating") : t("common.actions.create")} <ArrowRightIcon className="ml-2 h-4 w-4" /> </div> </Button>apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/components/table/RiskRegisterColumns.tsx (2)
25-35: Consider if "marketing" is the appropriate variant for department badges.The department is displayed with a Badge using the "marketing" variant. This might be confusing if the variant is not related to the actual department content.
- <Badge variant="marketing" className="uppercase w-fit"> + <Badge variant="secondary" className="uppercase w-fit">
36-56: Improve fallback handling for owner details.The Assignee column handles missing owner data, but could be improved:
- The fallback for owner image is
undefined, which might not be optimal- The AvatarFallback logic is good, defaulting to "?" when the name is undefined or empty
- <AvatarImage - src={row.original.owner?.image || undefined} - alt={row.original.owner?.name || ""} - /> + <AvatarImage + src={row.original.owner?.image || "/default-avatar.png"} + alt={row.original.owner?.name || "Unassigned"} + />apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/actions/getRisks.ts (2)
37-49: Inconsistent filter construction pattern.The where clause construction uses different patterns for different filters:
- The search filter uses spread with a condition
- The status, department, and assigneeId filters use ternary operators
Consider using the same pattern for all filters for consistency:
const where = { organizationId: user.organizationId, ...(search && { title: { contains: search, mode: Prisma.QueryMode.insensitive, }, }), - ...(status ? { status } : {}), - ...(department ? { department } : {}), - ...(assigneeId ? { ownerId: assigneeId } : {}), + ...(status && { status }), + ...(department && { department }), + ...(assigneeId && { ownerId: assigneeId }), };
50-60: Consider returning total count for pagination.The function currently returns the paginated risks, but doesn't include a total count, which would be useful for accurate pagination UI.
+ const count = await db.risk.count({ where }); const risks = await db.risk.findMany({ where, skip, take: pageSize, include: { owner: true, }, }); return { data: risks, + totalCount: count, };apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/hooks/useRisks.ts (1)
33-61: Consider implementing search debouncing.The useRisks hook directly passes the search parameter to SWR, which might cause excessive API calls as the user types. Consider implementing debouncing for the search parameter.
+ import { useDebounce } from 'use-debounce'; export const useRisks = ({ search = "", page = 1, pageSize = 10, status, department, assigneeId, }: { search?: string; page?: number; pageSize?: number; status?: RiskStatus | null; department?: Departments | null; assigneeId?: string | null; }) => { + const [debouncedSearch] = useDebounce(search, 300); const { data, isLoading, error, mutate } = useSWR( - ["risks", search, page, pageSize, status, department, assigneeId], + ["risks", debouncedSearch, page, pageSize, status, department, assigneeId], () => - fetchRisks({ search, page, pageSize, status, department, assigneeId }), + fetchRisks({ search: debouncedSearch, page, pageSize, status, department, assigneeId }), { revalidateOnFocus: true, revalidateOnReconnect: true, revalidateOnMount: true, revalidateIfStale: true, } ); return { data: data || [], isLoading, error, mutate }; };apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/RiskRegisterTable.tsx (1)
69-78: Move departments array outside the component.The departments array is defined inside the component, causing it to be recreated on each render. Move it outside the component for better performance.
+const departments: Departments[] = [ + "none", + "it", + "hr", + "admin", + "gov", + "itsm", + "qms", +] as const; export const RiskRegisterTable = () => { // ... existing code - const departments: Departments[] = [ - "none", - "it", - "hr", - "admin", - "gov", - "itsm", - "qms", - ] as const;apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/components/table/RiskRegisterFilters.tsx (1)
45-57: Consistent department filter with good formattingThe department filter follows the same pattern as the status filter, maintaining consistency in the UI. The formatting of department names improves readability, and the maxHeight property prevents the dropdown from becoming too large with many departments.
Consider extracting the department name formatting logic (
dept.replace(/_/g, " ").toUpperCase()) to a utility function if this formatting is used elsewhere in the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/RiskRegisterTable.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/actions/getRisks.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/components/table/RiskRegisterColumns.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/components/table/RiskRegisterFilters.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/hooks/useRisks.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/page.tsx(1 hunks)apps/app/src/components/forms/risks/create-risk-form.tsx(1 hunks)apps/app/src/components/status.tsx(2 hunks)apps/app/src/locales/en.ts(1 hunks)
🔇 Additional comments (30)
apps/app/src/components/status.tsx (5)
4-4: Extension of status types looks good.The addition of "archived" as a new status type is a sensible extension to the existing status options, providing more granular control over item states in the UI.
6-6: Type definition simplification is appropriate.The simplification of the StatusType definition to directly use all values from STATUS_TYPES is cleaner and ensures type safety as the array evolves.
8-13: Color mapping for archived status is well-implemented.The slate gray color (#64748b) is an appropriate visual indicator for archived items, distinguishing them from active statuses while maintaining the color scheme pattern.
15-18: Props interface extension is well-structured.The addition of the optional
noLabelprop enhances component flexibility, allowing consumers to display just the status indicator when needed.
27-27: Conditional rendering implementation is clean.The conditional rendering logic for the label is concise and follows React best practices. The implementation makes good use of the new
noLabelprop.apps/app/src/components/forms/risks/create-risk-form.tsx (5)
5-6: Excellent job upgrading to hooks for data fetching!The introduction of specialized hooks (
useOrganizationAdminsanduseRisks) aligns with modern React best practices by centralizing data fetching logic outside the component. This improves maintainability and separation of concerns.Also applies to: 10-10, 42-42
53-74: Good implementation of query state managementThe approach of explicitly parsing and setting default values for query parameters is robust and type-safe. This ensures consistent behavior across the application and proper synchronization with URL parameters.
76-83: Well-structured hook configurationThe
useRiskshook usage correctly passes all the necessary query parameters while ensuring proper type consistency. This maintains synchronization between the URL state and data fetching.
88-94: Great addition of immediate state revalidationAdding
mutateRisks()in the success handler ensures the risks list is immediately updated after creating a new risk, providing instant feedback to users without requiring a page refresh.
272-275: Correctly leveraging the organization admins dataThe SelectUser component now properly uses the fetched admin data and handles loading states appropriately, improving the user experience during data fetching.
apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/components/table/RiskRegisterColumns.tsx (3)
1-7: Appropriate imports for table column definitions.The imports bring in all necessary types and components for creating the columns. The mix of project-specific components like Status and design system components from @bubba/ui shows good component reuse.
8-17: Risk column implementation looks good.The Risk column correctly links to the individual risk detail page using the risk ID, making the table interactive and supporting navigation to detailed views.
18-24: Status column implementation looks good.The Status column correctly utilizes the Status component to display the risk status with appropriate visual indicators.
apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/actions/getRisks.ts (3)
1-6: Good use of server-side action with proper imports.The file correctly uses the "use server" directive and imports necessary dependencies for database access, authentication, and validation.
7-28: Well-structured schema and metadata for the action.The action is properly defined with:
- A clear Zod schema for input validation
- Default values for pagination parameters
- Appropriate metadata for tracking and naming
30-35: Good authorization check.The code properly checks for user organization ID before proceeding, returning an appropriate error response if unauthorized.
apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/hooks/useRisks.ts (2)
1-4: Appropriate imports for the hook.The imports include SWR for data fetching, the getRisks action, and necessary types.
5-31: Comprehensive error handling in the fetchRisks function.The fetchRisks function handles different error scenarios well:
- Null response
- Server errors
- Validation errors
Good practice to throw meaningful error messages that can be caught by error boundaries.
apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/RiskRegisterTable.tsx (2)
1-17: Appropriate imports and type definition.The file imports necessary components, hooks, and types, and defines a clear type for the table rows that combines Risk with owner information.
18-49: Good state management using useState and useQueryState.The component properly manages state for search, pagination, and filters using useState for local state and useQueryState for URL-based state. The parsing functions ensure type safety.
apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/page.tsx (2)
1-8: Great refactoring that improves separation of concerns!The simplification of this page component is a positive change. Moving the complex logic (data fetching, state management, filtering) from the page to the
RiskRegisterTablecomponent creates a cleaner separation of concerns and follows React best practices. The page is now focused solely on its primary responsibility - defining the page structure.
10-22: Metadata implementation looks goodThe generateMetadata function correctly sets the locale and returns the translated page title. The formatting adjustments don't affect functionality.
apps/app/src/app/[locale]/(app)/(dashboard)/risk/register/components/table/RiskRegisterFilters.tsx (4)
1-6: Well-organized imports with appropriate dependenciesThe imports are organized logically, bringing in necessary components and types from various parts of the application. Good reuse of the AssigneeAvatar component from the evidence module.
7-27: Clear and well-typed component interfaceThe component props are well-defined with explicit types, making the component's API clear and predictable. Each prop has an intuitive name that reflects its purpose, and the callback functions are properly typed with their parameter and return types.
28-44: Well-implemented status filter with proper formattingThe status filter dynamically generates options from the RiskStatus enum, ensuring it stays in sync with available statuses. The label formatting logic improves readability by properly capitalizing each word. The onChange handler correctly updates the status state and resets pagination when filters change.
58-73: Robust assignee filter with proper null handlingThe assignee filter implementation includes proper null checking with
(assignees || [])to prevent errors if assignees is undefined. The fallback to "Unknown" for missing names is a good defensive programming practice. Using AssigneeAvatar provides a consistent visual representation across the application.apps/app/src/locales/en.ts (4)
2-13: No issues found in language keys.
Everything looks consistent with no apparent misplacements or typos in the language and languages objects.
203-650: All strings in this block look consistent.
These updated translations for header, sidebar, authentication, onboarding, and policies sections appear logically grouped and consistent with minimal risk of confusion.
651-1095: Localization strings appear coherent and sufficiently descriptive.
No critical omissions or placeholder mismatches found here. Great job on structuring the content under people, settings, tests, frameworks, and other sections.
14-106:Details
✅ Verification successful
Check placeholder consistency for pagination.
In line 106 ("Page {{current}} of {{total}}"), double curly braces are used for interpolation. Confirm that the i18n library expects this format and that all placeholders remain uniform across the codebase.
🏁 Script executed:
#!/bin/bash # Description: Verify usage of double-brace placeholders across the localization files. rg -A 2 "{{.*}}"Length of output: 100
Action Required: Verify Consistency of Double-Curly Placeholders in Locales
The initial regex command failed due to unescaped curly braces. Please run the following revised script to check that all placeholder usages (e.g.,
"Page {{current}} of {{total}}") in the locale file use the expected double-curly interpolation format compatible with the i18n library.Once you confirm that the output reflects consistent placeholder usage across the file (and by extension, the codebase), you can confirm that the i18n library indeed supports this interpolation format.
🏁 Script executed:
#!/bin/bash # Re-verify double-curly placeholders in the English localization file using proper regex escaping. rg -A 2 '\{\{.*?\}\}' apps/app/src/locales/en.tsLength of output: 286
Placeholder Consistency Verified for Locales
The revised regex search confirms that the English localization file consistently uses double-curly placeholders (e.g.,"Page {{current}} of {{total}}"and"Last updated: {{date}}"). No inconsistencies were detected in the file. Please ensure that your i18n library is configured to interpret this format as expected.
| return ( | ||
| <> | ||
| <DataTable<RiskRegisterTableRow> | ||
| columns={columns} | ||
| data={data} | ||
| isLoading={isLoading} | ||
| search={{ | ||
| value: search, | ||
| onChange: setSearch, | ||
| }} | ||
| pagination={{ | ||
| page: Number(page), | ||
| pageSize: Number(pageSize), | ||
| totalCount: data.length, | ||
| totalPages: Math.ceil(data.length / Number(pageSize)), | ||
| hasNextPage: Number(page) < Math.ceil(data.length / Number(pageSize)), | ||
| hasPreviousPage: Number(page) > 1, | ||
| }} | ||
| onPageChange={(newPage) => setPage(newPage)} | ||
| onPageSizeChange={(newPageSize) => setPageSize(newPageSize)} | ||
| filters={{ | ||
| categories: filterCategories, | ||
| hasActiveFilters, | ||
| onClearFilters: handleClearFilters, | ||
| activeFilterCount: [status, department, assigneeId].filter(Boolean) | ||
| .length, | ||
| }} | ||
| ctaButton={{ | ||
| label: t("risk.register.empty.create_risk"), | ||
| onClick: () => setOpen("true"), | ||
| icon: <Plus className="h-4 w-4 mr-2" />, | ||
| }} | ||
| /> | ||
| <CreateRiskSheet /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pagination calculation might be inaccurate.
The pagination calculation uses data.length for totalCount and totalPages, which might be inaccurate if there are more items than those fetched. It would be better to get the total count from the API.
Add totalCount to the useRisks hook return value and update the getRisks action to return it. Then modify the pagination props:
<DataTable<RiskRegisterTableRow>
// ... other props
pagination={{
page: Number(page),
pageSize: Number(pageSize),
- totalCount: data.length,
- totalPages: Math.ceil(data.length / Number(pageSize)),
- hasNextPage: Number(page) < Math.ceil(data.length / Number(pageSize)),
+ totalCount: totalCount, // From API
+ totalPages: Math.ceil(totalCount / Number(pageSize)),
+ hasNextPage: Number(page) < Math.ceil(totalCount / Number(pageSize)),
hasPreviousPage: Number(page) > 1,
}}
// ... other props
/>📝 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.
| return ( | |
| <> | |
| <DataTable<RiskRegisterTableRow> | |
| columns={columns} | |
| data={data} | |
| isLoading={isLoading} | |
| search={{ | |
| value: search, | |
| onChange: setSearch, | |
| }} | |
| pagination={{ | |
| page: Number(page), | |
| pageSize: Number(pageSize), | |
| totalCount: data.length, | |
| totalPages: Math.ceil(data.length / Number(pageSize)), | |
| hasNextPage: Number(page) < Math.ceil(data.length / Number(pageSize)), | |
| hasPreviousPage: Number(page) > 1, | |
| }} | |
| onPageChange={(newPage) => setPage(newPage)} | |
| onPageSizeChange={(newPageSize) => setPageSize(newPageSize)} | |
| filters={{ | |
| categories: filterCategories, | |
| hasActiveFilters, | |
| onClearFilters: handleClearFilters, | |
| activeFilterCount: [status, department, assigneeId].filter(Boolean) | |
| .length, | |
| }} | |
| ctaButton={{ | |
| label: t("risk.register.empty.create_risk"), | |
| onClick: () => setOpen("true"), | |
| icon: <Plus className="h-4 w-4 mr-2" />, | |
| }} | |
| /> | |
| <CreateRiskSheet /> | |
| return ( | |
| <> | |
| <DataTable<RiskRegisterTableRow> | |
| columns={columns} | |
| data={data} | |
| isLoading={isLoading} | |
| search={{ | |
| value: search, | |
| onChange: setSearch, | |
| }} | |
| pagination={{ | |
| page: Number(page), | |
| pageSize: Number(pageSize), | |
| totalCount: totalCount, // From API | |
| totalPages: Math.ceil(totalCount / Number(pageSize)), | |
| hasNextPage: Number(page) < Math.ceil(totalCount / Number(pageSize)), | |
| hasPreviousPage: Number(page) > 1, | |
| }} | |
| onPageChange={(newPage) => setPage(newPage)} | |
| onPageSizeChange={(newPageSize) => setPageSize(newPageSize)} | |
| filters={{ | |
| categories: filterCategories, | |
| hasActiveFilters, | |
| onClearFilters: handleClearFilters, | |
| activeFilterCount: [status, department, assigneeId].filter(Boolean) | |
| .length, | |
| }} | |
| ctaButton={{ | |
| label: t("risk.register.empty.create_risk"), | |
| onClick: () => setOpen("true"), | |
| icon: <Plus className="h-4 w-4 mr-2" />, | |
| }} | |
| /> | |
| <CreateRiskSheet /> |
| maxFileSize: "Max file size: {size}MB", | ||
| }, | ||
| fileUrl: { | ||
| additionalLinks: "Additional Links", | ||
| add: "Add", | ||
| linksAdded: "{count} link{s} added", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify placeholder styles for file-size and link counts.
The file-size placeholder uses single braces ({size}), while other segments (e.g., line 106) use double braces. If your i18n library supports only one style, align them for consistency.
- "Max file size: {size}MB"
+ "Max file size: {{size}}MB"📝 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.
| maxFileSize: "Max file size: {size}MB", | |
| }, | |
| fileUrl: { | |
| additionalLinks: "Additional Links", | |
| add: "Add", | |
| linksAdded: "{count} link{s} added", | |
| maxFileSize: "Max file size: {{size}}MB", | |
| }, | |
| fileUrl: { | |
| additionalLinks: "Additional Links", | |
| add: "Add", | |
| linksAdded: "{count} link{s} added", |
This branch was successfully deployed
1 participant
Summary by CodeRabbit
New Features
AssigneeAvatarcomponent for displaying assignee images or initials.PoliciesTablecomponent for improved policy management and display.DataTablecomponent introduced for rendering data tables with sorting, filtering, and pagination.DataTablePaginationcomponent added to manage pagination controls.DataTableSkeletoncomponent for displaying loading states in data tables.RiskRegisterTablecomponent has been introduced for managing and displaying risk data.Refactor
EvidenceListTablehas been refactored to utilize a newDataTablecomponent, streamlining the table functionality.EmployeesListcomponent has been restructured to adopt a context-based approach for managing employee data.InviteUserSheetcomponent has been renamed toEmployeeInviteSheetand refactored for enhanced functionality.RiskRegisterPagecomponent has been simplified to delegate risk management functionality to theRiskRegisterTable.PoliciesListcomponent has been overhauled to use a context-based approach for rendering policies.