Update Search Reports #1
Conversation
…y's Reports filtering
📝 WalkthroughWalkthroughThe PR adds server-side pagination support to report fetching via updated Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Component as search/page.tsx
participant Action as getReports (server)
participant DB as Database/API
User->>Component: Navigate/change page or page size
Component->>Component: Update pagination state
Component->>Component: Compute fetch params (page, pageSize, searchTerm)
Component->>Action: fetchReports(page, pageSize, term)
activate Action
Action->>Action: Compute from/to offsets
Action->>DB: Query bills with filters & range
DB-->>Action: Return bills + total count
Action->>Action: Map & flatten results
Action-->>Component: Return { reports, pagination: { totalCount, totalPages, ... } }
deactivate Action
Component->>Component: Update pagination state from response
Component->>Component: Re-render with new results & pagination controls
Component-->>User: Display paginated results & controls
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/app/dashboard/lab/search/page.tsx (2)
43-45: Consider adding missing dependencies or usinguseCallback.The
useEffectreferencesfetchReportsandsearchTermbut they're not in the dependency array. While the current behavior works as intended, this will trigger ESLint'sreact-hooks/exhaustive-depswarning.Consider wrapping
fetchReportsinuseCallbackor restructuring to satisfy the exhaustive-deps rule.
199-221: Pagination ellipsis logic may produce inconsistent results.The windowing logic on lines 202-206 can produce unexpected output (e.g., multiple consecutive ellipses or missing page numbers near boundaries). Consider a cleaner approach:
🔎 Alternative pagination window approach
const renderPageButtons = () => { const pages: (number | string)[] = []; const total = pagination.totalPages; const current = page; if (total <= 7) { for (let i = 1; i <= total; i++) pages.push(i); } else { pages.push(1); if (current > 3) pages.push('...'); for (let i = Math.max(2, current - 1); i <= Math.min(total - 1, current + 1); i++) { pages.push(i); } if (current < total - 2) pages.push('...'); pages.push(total); } return pages.map((p, idx) => typeof p === 'string' ? ( <span key={`ellipsis-${idx}`} className="px-1">...</span> ) : ( <button key={p} onClick={() => handlePageChange(p)} className={...}> {p} </button> ) ); };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/actions/billActions.tssrc/app/dashboard/lab/search/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/dashboard/lab/search/page.tsx (1)
src/app/actions/billActions.ts (1)
getReports(126-207)
🔇 Additional comments (9)
src/app/actions/billActions.ts (5)
126-135: Function signature and pagination setup look good.The updated signature cleanly supports pagination with sensible defaults. The
from/tocalculation for Supabase'srange()is correct.
157-162: Timezone assumption in date filtering.The hardcoded
T00:00:00.000ZandT23:59:59.999Zsuffixes assume UTC storage. Ifcreated_atis stored in UTC but users expect local-date filtering, records near midnight boundaries may be unexpectedly included or excluded. Consider whether this matches your database timezone configuration and user expectations.
171-175: Query execution is correctly structured.The ordering, range limiting, and count extraction are properly implemented.
177-188: Good defensive handling for missing patient data.The
|| ''fallback and.trim()ensure graceful handling when patient fields are missing.
190-202: Pagination metadata response is well-structured.The calculation handles edge cases correctly, and the response shape aligns with the frontend's expectations.
src/app/dashboard/lab/search/page.tsx (4)
13-20: Pagination state initialization is correct.The initial state values match the defaults in
getReportsand provide a consistent starting point.
22-41: Fetch function is well-structured with flexible parameters.Good approach allowing explicit overrides while defaulting to current state values.
47-57: Search and pagination handlers are correctly implemented.Resetting to page 1 on new search and validating page bounds are good UX practices.
139-146: Status badge styling is clean and handles unknown statuses gracefully.Good use of a fallback style for any status not explicitly handled.
| // Handle search term | ||
| if (searchTerm) { | ||
| // Search across reg_number, or patient fields | ||
| // Using .or() with nested filters for patients. Note: for joins, syntax is 'relation.column' | ||
| query = query.or(`reg_number.ilike.%${searchTerm}%,patient_id.in.(select id from patients where first_name ilike '%${searchTerm}%' or last_name ilike '%${searchTerm}%' or uhid ilike '%${searchTerm}%')`); | ||
| } |
There was a problem hiding this comment.
SQL injection vulnerability via unsanitized searchTerm.
The searchTerm is directly interpolated into the .or() filter string, including a nested select subquery. A malicious input like %'); DELETE FROM patients; -- could potentially exploit this.
Even if Supabase/PostgREST provides some escaping, directly embedding user input into query strings is unsafe. Consider:
- Sanitizing/escaping special characters (
%,',), etc.) fromsearchTerm - Using a database function or RPC call for complex searches
- Performing separate queries and merging results in application code
🔎 Minimal sanitization approach
// Handle search term
if (searchTerm) {
+ // Escape special characters to prevent injection
+ const sanitized = searchTerm.replace(/[%_'\\()]/g, '\\$&');
// Search across reg_number, or patient fields
// Using .or() with nested filters for patients. Note: for joins, syntax is 'relation.column'
- query = query.or(`reg_number.ilike.%${searchTerm}%,patient_id.in.(select id from patients where first_name ilike '%${searchTerm}%' or last_name ilike '%${searchTerm}%' or uhid ilike '%${searchTerm}%')`);
+ query = query.or(`reg_number.ilike.%${sanitized}%,patient_id.in.(select id from patients where first_name ilike '%${sanitized}%' or last_name ilike '%${sanitized}%' or uhid ilike '%${sanitized}%')`);
}However, a safer architectural approach would be to use a Supabase RPC function with parameterized queries.
📝 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.
| // Handle search term | |
| if (searchTerm) { | |
| // Search across reg_number, or patient fields | |
| // Using .or() with nested filters for patients. Note: for joins, syntax is 'relation.column' | |
| query = query.or(`reg_number.ilike.%${searchTerm}%,patient_id.in.(select id from patients where first_name ilike '%${searchTerm}%' or last_name ilike '%${searchTerm}%' or uhid ilike '%${searchTerm}%')`); | |
| } | |
| // Handle search term | |
| if (searchTerm) { | |
| // Escape special characters to prevent injection | |
| const sanitized = searchTerm.replace(/[%_'\\()]/g, '\\$&'); | |
| // Search across reg_number, or patient fields | |
| // Using .or() with nested filters for patients. Note: for joins, syntax is 'relation.column' | |
| query = query.or(`reg_number.ilike.%${sanitized}%,patient_id.in.(select id from patients where first_name ilike '%${sanitized}%' or last_name ilike '%${sanitized}%' or uhid ilike '%${sanitized}%')`); | |
| } |
| <select | ||
| value={pageSize} | ||
| onChange={(e) => { | ||
| const newSize = Number(e.target.value); | ||
| setPageSize(newSize); | ||
| setPage(1); | ||
| fetchReports(1, newSize, searchTerm); | ||
| }} |
There was a problem hiding this comment.
Double fetch when changing page size.
The onChange handler calls fetchReports(1, newSize, searchTerm) directly, but also updates page and pageSize state. The useEffect on line 43-45 will trigger another fetch on the next render when these state values change, causing redundant API calls.
🔎 Suggested fix: remove the direct fetch call
<select
value={pageSize}
onChange={(e) => {
const newSize = Number(e.target.value);
setPageSize(newSize);
setPage(1);
- fetchReports(1, newSize, searchTerm);
}}
className="text-sm border rounded px-2 py-1 bg-white"
>The useEffect will handle the fetch when page and pageSize state update.
📝 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.
| <select | |
| value={pageSize} | |
| onChange={(e) => { | |
| const newSize = Number(e.target.value); | |
| setPageSize(newSize); | |
| setPage(1); | |
| fetchReports(1, newSize, searchTerm); | |
| }} | |
| <select | |
| value={pageSize} | |
| onChange={(e) => { | |
| const newSize = Number(e.target.value); | |
| setPageSize(newSize); | |
| setPage(1); | |
| }} |
🤖 Prompt for AI Agents
In src/app/dashboard/lab/search/page.tsx around lines 176 to 183, the select
onChange handler triggers fetchReports(1, newSize, searchTerm) directly while
also updating page and pageSize state, causing a duplicate API call because the
useEffect watching page/pageSize will fetch again; remove the direct
fetchReports(...) call from the onChange handler so the state updates
(setPage(1) and setPageSize(newSize)) drive the single fetch via the existing
useEffect.
1 participant
…y's Reports filtering
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.