Enhanced Resource Page #9768
Enhanced Resource Page #9768
Conversation
WalkthroughThis pull request introduces new localization keys in Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
❌ Deploy Preview for care-ohc failed.
|
|
@rithviknishad review this one |
![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)
src/components/Kanban/Board.tsx (1)
38-39: Removed horizontal scrolling buttons and simplified layout.The new two-tier container approach (outer
+ flex boxes) is neat. Ensure no accessibility regression for users who relied on the previous scroll control.src/components/Resource/ResourceCardList.tsx (1)
26-85: Card layout effectively conveys resource info.Using a grid and separate display for title, status, and emergency badge is clear, providing a well-organized view. Ensure each field is accessible with descriptive ARIA labels if needed.
src/components/Resource/ResourcePage.tsx (1)
94-126: Consider removing redundant fragments.A fragment wrapping a single child is flagged by the linter. You can safely remove it to reduce JSX verbosity.
- <> <div className="flex lg:flex-row flex-col w-full justify-end gap-2"> - </>🧰 Tools
🪛 Biome (1.9.4)
[error] 94-126: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
public/locale/en.json (1)
1616-1616: LGTM! Consider adding a placeholder variant.The new translation key follows the naming convention and is properly placed. For consistency with other selection fields in the application, consider adding a placeholder variant:
"select_status": "Select Status", +"select_status_placeholder": "Select a status",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
public/locale/en.json(1 hunks)src/Routers/routes/ResourceRoutes.tsx(1 hunks)src/components/Kanban/Board.tsx(3 hunks)src/components/Resource/ResourceBoard.tsx(0 hunks)src/components/Resource/ResourceCardList.tsx(1 hunks)src/components/Resource/ResourceList.tsx(0 hunks)src/components/Resource/ResourcePage.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- src/components/Resource/ResourceList.tsx
- src/components/Resource/ResourceBoard.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Resource/ResourcePage.tsx
[error] 94-126: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (16)
src/Routers/routes/ResourceRoutes.tsx (2)
3-3: Successfully imported the newResourcePagecomponent.The import statement is clear and aligns with the consolidated view approach.
8-8: Route consolidation looks good.Replacing the old
BoardView/ListViewroutes withResourcePageis consistent with the PR objective of unifying the resource views.src/components/Kanban/Board.tsx (2)
137-137: Height change fromcalc(100vh-200px)tocalc(100vh-340px).Increasing the offset reduces visible space, which might cause more scrolling. Verify on smaller devices to ensure no usability issues.
169-169: Placeholder usage in Droppable context.Including
provided.placeholderensures proper rendering of dragged items. This is currently a best practice for React DnD.src/components/Resource/ResourceCardList.tsx (5)
1-3: Imports are well-structured.Imports for
LinkanduseTranslationare minimal and aligned with usage below.
11-13: Interface definition is clear.
ResourceCardListPropsis concise, making the component’s expected data structure explicit.
18-24: Graceful handling of empty data.Rendering "no_results_found" message improves user feedback when there are no resources.
131-135: “View all details” link for each resource.This link offers a direct route to resource details, improving discoverability.
142-142: Export statement confirmed.Exporting
ResourceCardListas default matches the new consolidated design approach.src/components/Resource/ResourcePage.tsx (7)
42-45: Lazy loading the KanbanBoard component.This reduces initial bundle size and is a strong performance optimization for routes that may not require the board immediately.
51-67: ResourcePage initialization logic.Storing filter states (
boardFilter,viewMode) is straightforward. The fallback to default active statuses plus an option for completed items is well-structured.
68-77: Fetch logic usinguseTanStackQueryInstead.Looks correct. The query configuration matches the advanced filter structure. Confirm that all relevant data fields are included in the final payload for the Resource list.
79-84: Export function for CSV.Nice use of
requestwithcsv: true. This helps quickly integrate a CSV export feature.
129-227: Board view integration.The board filter and drag-and-drop updates to resource status align well with the consolidated view. Watch out for edge cases when switching between active and completed boards.
228-297: List view integration with advanced filters.Combining the
ResourceCardList, refresh actions, and pagination is well-organized. The consistent approach to filtering across both views helps unify UX.
302-302: ExportingResourcePage.This final export aligns with the route changes in
ResourceRoutes.tsx, ensuring navigational consistency.
|
I am unable to reopen that PR as I've reforked the repo 😅 |
|
solve the changes requested in previous PR here atleast then? |
I think I've done that🤔Let me list the changes.
|
|
let's keep the implementation idential to the one in appointments, and delete that kanban board |
got it |
Jacobjeevan
added
changes required
and removed
question
@rithviknishad Do we need DragDropContext in resource page board view? |
…enhanced-resource-page
…enhanced-resource-page
…enhanced-resource-page
…enhanced-resource-page
|
@rithviknishad , I’ve tried implementing this in the same way as the AppointmentPage. Is this correct? |
| const appliedFilters = formatFilter(qParams); | ||
| const [viewMode, setViewMode] = useState<"board" | "list">("board"); | ||
| const { exportFile } = useExport(); | ||
| const { loading, data, refetch } = useTanStackQueryInstead( |
There was a problem hiding this comment.
Switch to using useQuery + query here.
Do the same for other API calls in this component 👍
| }); | ||
| return data as QueryResponse<T>; | ||
| } catch (error) { | ||
| console.error("Error fetching section data:", error); |
There was a problem hiding this comment.
Remove the console log
| const options = section.fetchOptions(section.id); | ||
| const fetchPage = async ({ pageParam = 0 }) => { | ||
| try { | ||
| const data = await callApi(options.route, { |
There was a problem hiding this comment.
useQuery instead of callApi.
| const [boardFilter, setBoardFilter] = useState(ACTIVE); | ||
| // eslint-disable-next-line | ||
| const appliedFilters = formatFilter(qParams); | ||
| const [viewMode, setViewMode] = useState<"board" | "list">("board"); |
There was a problem hiding this comment.
You should add the logic for storing in localStorage for viewMode, right now it resets on every page load.
We can create a hook to handle that in a different issue, since we will have multiple components having a similar view.
There was a problem hiding this comment.
@Jacobjeevan for now I've use useEffect, should I create a new issue for the hook itself? 🤔
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/Resource/ResourcePage.tsx (2)
300-325: Optimize performance with React.memo.Consider memoizing the ResourceColumn component to prevent unnecessary re-renders when parent components update.
-function ResourceColumn<T extends { id: string }>( +const ResourceColumn = React.memo(<T extends { id: string }>( props: ResourceColumnProps<T>, ) { const board = useRef<HTMLDivElement>(null); return ( // ... component implementation ); -} +});
480-593: Improve type safety and styling maintainability.Consider the following improvements:
- Use proper type checking for the resource parameter
- Extract repeated CSS classes into reusable Tailwind components
- return data.map((resource: ResourceRequest, i) => ( + return data.map((resource, i) => ( <div key={i} - className="w-full border border-b-2 border-gray-200 col-span-6" + className={clsx( + "w-full border border-b-2 border-gray-200 col-span-6", + "resource-row" + )} >Consider creating a CSS module or Tailwind components for commonly used class combinations to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Kanban/Board.tsx(0 hunks)src/components/Resource/ResourcePage.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/Kanban/Board.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
| <Select | ||
| defaultValue="active" | ||
| onValueChange={(value) => | ||
| setBoardFilter(value === "completed" ? COMPLETED : ACTIVE) | ||
| } | ||
| > | ||
| <SelectTrigger className="w-full lg:w-[180px]"> | ||
| <SelectValue defaultValue={"completed"} /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectGroup> | ||
| <SelectItem value="active">{t("active")}</SelectItem> | ||
| <SelectItem value="completed">{t("completed")}</SelectItem> | ||
| </SelectGroup> | ||
| </SelectContent> | ||
| </Select> |
There was a problem hiding this comment.
Fix inconsistent default values in Select component.
The defaultValue prop in SelectTrigger and the defaultValue in SelectValue are inconsistent. The SelectTrigger uses "active" while SelectValue uses "completed".
<Select
defaultValue="active"
onValueChange={(value) =>
setBoardFilter(value === "completed" ? COMPLETED : ACTIVE)
}
>
<SelectTrigger className="w-full lg:w-[180px]">
- <SelectValue defaultValue={"completed"} />
+ <SelectValue defaultValue={"active"} />
</SelectTrigger>
<SelectContent>📝 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 | |
| defaultValue="active" | |
| onValueChange={(value) => | |
| setBoardFilter(value === "completed" ? COMPLETED : ACTIVE) | |
| } | |
| > | |
| <SelectTrigger className="w-full lg:w-[180px]"> | |
| <SelectValue defaultValue={"completed"} /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| <SelectGroup> | |
| <SelectItem value="active">{t("active")}</SelectItem> | |
| <SelectItem value="completed">{t("completed")}</SelectItem> | |
| </SelectGroup> | |
| </SelectContent> | |
| </Select> | |
| <Select | |
| defaultValue="active" | |
| onValueChange={(value) => | |
| setBoardFilter(value === "completed" ? COMPLETED : ACTIVE) | |
| } | |
| > | |
| <SelectTrigger className="w-full lg:w-[180px]"> | |
| <SelectValue defaultValue={"active"} /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| <SelectGroup> | |
| <SelectItem value="active">{t("active")}</SelectItem> | |
| <SelectItem value="completed">{t("completed")}</SelectItem> | |
| </SelectGroup> | |
| </SelectContent> | |
| </Select> |
| const fetchPage = async ({ pageParam = 0 }) => { | ||
| try { | ||
| const data = await query(options.route, { | ||
| queryParams: { | ||
| ...options.options?.query, | ||
| offset: pageParam, | ||
| limit: defaultLimit, | ||
| }, | ||
| })({ signal: new AbortController().signal }); | ||
| return data as QueryResponse<T>; | ||
| } catch (error) { | ||
| return { results: [], next: null, count: 0 }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and request cancellation.
The current implementation has two issues:
- The AbortController is created but never stored or used effectively
- The error handling silently fails without providing useful information
const fetchPage = async ({ pageParam = 0 }) => {
+ const controller = new AbortController();
try {
const data = await query(options.route, {
queryParams: {
...options.options?.query,
offset: pageParam,
limit: defaultLimit,
},
- })({ signal: new AbortController().signal });
+ })({ signal: controller.signal });
return data as QueryResponse<T>;
} catch (error) {
+ if (error instanceof Error) {
+ console.error(`Failed to fetch page: ${error.message}`);
+ }
return { results: [], next: null, count: 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.
| const fetchPage = async ({ pageParam = 0 }) => { | |
| try { | |
| const data = await query(options.route, { | |
| queryParams: { | |
| ...options.options?.query, | |
| offset: pageParam, | |
| limit: defaultLimit, | |
| }, | |
| })({ signal: new AbortController().signal }); | |
| return data as QueryResponse<T>; | |
| } catch (error) { | |
| return { results: [], next: null, count: 0 }; | |
| } | |
| }; | |
| const fetchPage = async ({ pageParam = 0 }) => { | |
| const controller = new AbortController(); | |
| try { | |
| const data = await query(options.route, { | |
| queryParams: { | |
| ...options.options?.query, | |
| offset: pageParam, | |
| limit: defaultLimit, | |
| }, | |
| })({ signal: controller.signal }); | |
| return data as QueryResponse<T>; | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| console.error(`Failed to fetch page: ${error.message}`); | |
| } | |
| return { results: [], next: null, count: 0 }; | |
| } | |
| }; |
| const exportAction = async () => { | ||
| const { data } = await request(routes.downloadResourceRequests, { | ||
| query: { ...appliedFilters, csv: true }, | ||
| }); | ||
| return data ?? null; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to exportAction function.
The function should handle potential network errors and invalid responses explicitly.
const exportAction = async () => {
+ try {
const { data } = await request(routes.downloadResourceRequests, {
query: { ...appliedFilters, csv: true },
});
return data ?? null;
+ } catch (error) {
+ console.error('Failed to export resource requests:', error);
+ throw new Error(t('failed_to_export'));
+ }
};📝 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.
| const exportAction = async () => { | |
| const { data } = await request(routes.downloadResourceRequests, { | |
| query: { ...appliedFilters, csv: true }, | |
| }); | |
| return data ?? null; | |
| }; | |
| const exportAction = async () => { | |
| try { | |
| const { data } = await request(routes.downloadResourceRequests, { | |
| query: { ...appliedFilters, csv: true }, | |
| }); | |
| return data ?? null; | |
| } catch (error) { | |
| console.error('Failed to export resource requests:', error); | |
| throw new Error(t('failed_to_export')); | |
| } | |
| }; |
…enhanced-resource-page
|
👋 Hi, @manmeetnagii, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cypress/e2e/patient_spec/patient_search.cy.ts (2)
1-9: Consider using typed test data.While the test data is well-organized using constants, consider using TypeScript interfaces to define the expected shape of patient details for better type safety and autocompletion.
interface PatientDetails { name: string; sex: string; phone: string; } const TEST_PHONE = "9495031234"; const PATIENT_DETAILS: PatientDetails = { name: "Nihal", sex: "Male", phone: TEST_PHONE, };
1-23: Consider adding data cleanup in afterEach.The test might create or modify data during execution. Consider adding cleanup steps in an
afterEachblock to ensure test isolation.afterEach(() => { // Clean up any test data or reset application state cy.task('db:cleanup', { phone: TEST_PHONE }); });src/Utils/request/types.ts (1)
11-30: LGTM! Consider adding JSDoc comments.The new route type architecture is well-designed, with clear separation between query and mutation routes. This aligns well with REST principles and React Query patterns.
Consider adding JSDoc comments to document:
- The purpose of each interface
- The meaning of the
noAuthflag- Examples of usage
+/** + * Base interface for all API routes + * @property path - The API endpoint path + * @property TRes - The expected response type + * @property noAuth - If true, the endpoint doesn't require authentication + */ interface RouteBase<TData> { path: string; TRes: TData; noAuth?: boolean; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cypress/e2e/patient_spec/patient_search.cy.ts(1 hunks)public/locale/en.json(1 hunks)src/Utils/request/types.ts(1 hunks)src/components/Resource/ResourcePage.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- public/locale/en.json
- src/components/Resource/ResourcePage.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
cypress/e2e/patient_spec/patient_search.cy.ts (1)
11-14: Verify the login state persistence.The login setup looks good, but we should verify if the login state persists across different test runs.
✅ Verification successful
Login state handling is properly implemented ✅
The current implementation follows Cypress best practices for test isolation:
- Uses Cypress's built-in LOCAL_STORAGE_MEMORY for token storage
- Automatically clears state between tests
- Ensures fresh login state for each test via beforeEach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other tests that might interfere with the login state rg -l "cy\.loginByApi.*staff" cypress/e2e/Length of output: 90
Script:
#!/bin/bash # Check for custom command definitions rg -l "Cypress.Commands.add.*login" # Look for cypress configuration files fd "cypress.config" --extension ts,js # Check content of any custom commands file fd "commands" --extension ts,js --exec cat {} \;Length of output: 155
Script:
#!/bin/bash # Check the implementation of loginByApi command rg -A 10 "Cypress.Commands.add.*loginByApi" cypress/support/commands.tsLength of output: 375
src/Utils/request/types.ts (1)
40-40: Verify handling of numeric path parameters.The change to allow numbers in
pathParamsis practical but requires verification of URL construction logic.Let's check for potential issues:
Also applies to: 46-46
✅ Verification successful
Allowing numbers in pathParams is safe
The URL construction logic in
makeUrl()safely handles numeric values through template literal string conversion. No type-specific string operations are performed on the values.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check URL construction with numeric path parameters # and find all usages of pathParams to ensure compatibility # Find all files that import from types.ts echo "Files importing from types.ts:" rg -l "from ['\"].*Utils/request/types['\"]" # Find URL construction logic echo -e "\nURL construction logic:" rg -A 5 "const url|let url" $(fd -e ts -e tsx) # Find pathParams usage echo -e "\npathParams usage:" rg "pathParams.*Record<string, string>" $(fd -e ts -e tsx)Length of output: 39741
Script:
#!/bin/bash # Check makeUrl implementation and path parameter handling # Find makeUrl implementation echo "makeUrl implementation:" ast-grep --pattern 'function makeUrl' -A 10 # Find any string operations on pathParams echo -e "\nString operations on pathParams:" rg -A 3 "pathParams\.[a-zA-Z_]+\.(toString|toLowerCase|toUpperCase|replace|match|split|substring|trim)" # Find all direct uses of pathParams echo -e "\nDirect pathParams usage:" rg -A 3 "pathParams\.[a-zA-Z_]+"Length of output: 389
Script:
#!/bin/bash # Check URL construction and path parameter handling # Find makeUrl with simpler pattern echo "makeUrl implementation:" rg -l "makeUrl" src/Utils/request/ # Examine utils.ts content echo -e "\nutils.ts content:" cat src/Utils/request/utils.ts # Find path parameter substitution patterns echo -e "\nPath parameter substitution:" rg -A 5 "/:.*?/" src/Utils/request/Length of output: 3536
| it("search patient with phone number and verifies details", () => { | ||
| patientSearch | ||
| .selectFacility("Arike") | ||
| .clickSearchPatients() | ||
| .searchPatient(TEST_PHONE) | ||
| .verifySearchResults(PATIENT_DETAILS); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Add error handling and test coverage for edge cases
The page object and test suite need improvements:
- The
PatientSearchpage object lacks error handling:- No validation for "no results found" state
- No handling of invalid phone number formats
- No assertions for multiple search results
Add the following test cases:
- Test invalid phone number format (e.g., non-numeric characters)
- Test search with non-existent phone number
- Test search returning multiple patient matches
- Add appropriate error handling methods in the page object to support these scenarios
🔗 Analysis chain
Add error scenarios and edge cases.
While the happy path is covered, consider adding test cases for:
- Invalid phone numbers
- No search results
- Multiple matching results
Also, verify that the patientSearch page object implements proper error handling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the PatientSearch page object handles error cases
rg -A 5 "error|invalid|not found" "cypress/pageObject/Patients/PatientSearch.ts"
Length of output: 80
Script:
#!/bin/bash
# Find the correct path for PatientSearch page object
fd -t f "PatientSearch" cypress/
# Examine the implementation of search-related methods
ast-grep --pattern 'class PatientSearch {
$$$
searchPatient($_) {
$$$
}
$$$
}'
# Look for any verification or assertion methods
ast-grep --pattern 'class PatientSearch {
$$$
verifySearchResults($_) {
$$$
}
$$$
}'
Length of output: 6141
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/Resource/ResourcePage.tsx (3)
91-96:⚠️ Potential issueAdd error handling to exportAction function.
The function should handle potential network errors and invalid responses.
const exportAction = async () => { + try { const { data } = await request(routes.downloadResourceRequests, { query: { ...appliedFilters, csv: true }, }); return data ?? null; + } catch (error) { + console.error('Failed to export resources:', error); + throw new Error(t('failed_to_export')); + } };
148-163:⚠️ Potential issueFix inconsistent default values in Select component.
The
defaultValueprop in SelectTrigger and thedefaultValuein SelectValue are inconsistent.<Select defaultValue="active" onValueChange={(value) => setBoardFilter(value === "completed" ? COMPLETED : ACTIVE) } > <SelectTrigger className="w-full lg:w-[180px]"> - <SelectValue defaultValue={"completed"} /> + <SelectValue defaultValue={"active"} /> </SelectTrigger> <SelectContent>
346-360:⚠️ Potential issueImprove error handling and request cancellation.
The current implementation has two issues:
- The AbortController is created but never stored or used effectively
- The error handling silently fails without providing useful information
const fetchPage = async ({ pageParam = 0 }) => { + const controller = new AbortController(); try { const data = await query(options.route, { queryParams: { ...options.options?.query, offset: pageParam, limit: defaultLimit, }, - })({ signal: new AbortController().signal }); + })({ signal: controller.signal }); return data as QueryResponse<T>; } catch (error) { + if (error instanceof Error) { + console.error(`Failed to fetch page: ${error.message}`); + } return { results: [], next: null, count: 0 }; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(1 hunks)src/components/Resource/ResourcePage.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/components/Resource/ResourcePage.tsx (1)
476-615: LGTM! Well-implemented list view.The ResourceRow component is well implemented with proper error handling, loading states, and accessibility attributes.
| const { isLoading, data, refetch } = useQuery({ | ||
| queryKey: ["title"], | ||
| queryFn: query(routes.listResourceRequests, { | ||
| queryParams: formatFilter({ | ||
| ...qParams, | ||
| limit: resultsPerPage, | ||
| offset: (qParams.page ? qParams.page - 1 : 0) * resultsPerPage, | ||
| }), | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to useQuery implementation.
The query could fail silently without proper error handling. Consider adding error handling and loading states.
const { isLoading, data, refetch } = useQuery({
queryKey: ["title"],
queryFn: query(routes.listResourceRequests, {
queryParams: formatFilter({
...qParams,
limit: resultsPerPage,
offset: (qParams.page ? qParams.page - 1 : 0) * resultsPerPage,
}),
}),
+ onError: (error) => {
+ console.error('Failed to fetch resources:', error);
+ // Add error handling logic (e.g., show error toast)
+ }
});📝 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.
| const { isLoading, data, refetch } = useQuery({ | |
| queryKey: ["title"], | |
| queryFn: query(routes.listResourceRequests, { | |
| queryParams: formatFilter({ | |
| ...qParams, | |
| limit: resultsPerPage, | |
| offset: (qParams.page ? qParams.page - 1 : 0) * resultsPerPage, | |
| }), | |
| }), | |
| }); | |
| const { isLoading, data, refetch } = useQuery({ | |
| queryKey: ["title"], | |
| queryFn: query(routes.listResourceRequests, { | |
| queryParams: formatFilter({ | |
| ...qParams, | |
| limit: resultsPerPage, | |
| offset: (qParams.page ? qParams.page - 1 : 0) * resultsPerPage, | |
| }), | |
| }), | |
| onError: (error) => { | |
| console.error('Failed to fetch resources:', error); | |
| // Add error handling logic (e.g., show error toast) | |
| } | |
| }); |
|
@rithviknishad @Jacobjeevan review please |
|
👋 Hi, @manmeetnagii, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Common/Export.tsx (1)
Line range hint
44-50: Improve type safety in the onClick handler.The current implementation has potential type safety issues:
- Using the non-null assertion operator (
!) onprops.routeis unsafe- The action callback could be better typed
Consider this safer implementation:
- const { data } = await request(props.route!); + if (!props.route) return null; + const { data } = await request(props.route); return data ?? null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/Export.tsx(1 hunks)
🔇 Additional comments (2)
src/components/Common/Export.tsx (2)
12-12: Verify the impact of type import change across the codebase.The change from
ApiRoutetoRouteimport suggests a broader refactoring of the request types system.Let's verify the migration status across the codebase:
✅ Verification successful
The type import change is consistent with the codebase
The migration from
ApiRoutetoRoutehas been completed across the codebase, with no remaining references to the old type. The change in Export.tsx correctly aligns with the type system defined in@/Utils/request/types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any remaining usages of ApiRoute rg "ApiRoute" --type typescript # Check the adoption of the new Route type rg "Route<" --type typescriptLength of output: 133
Script:
#!/bin/bash # Search for ApiRoute in TypeScript files rg "ApiRoute" -t ts -t tsx -B 2 -A 2 # Search for Route type usage and imports rg "Route[<\s]|import.*Route" -t ts -t tsx -B 2 -A 2Length of output: 147
Script:
#!/bin/bash # Search for imports using ast-grep ast-grep --pattern 'import { $_ } from "@/Utils/request/types"' # Fallback to basic ripgrep with file extensions rg "ApiRoute|Route" --glob "*.{ts,tsx}"Length of output: 11402
20-20: Breaking Change: Update type definition from ApiRoute to Route.The type change maintains the same generic parameters but represents a breaking change. Ensure all consumers of the
ExportButtoncomponent are updated accordingly.Let's verify the component usage:
…enhanced-resource-page
…gii/care_fe into enhanced-resource-page
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/Export.tsx(2 hunks)
🧰 Additional context used
🪛 GitHub Check: cypress-run (1)
src/components/Common/Export.tsx
[failure] 72-72:
Cannot find name 'tMenu'.
🪛 GitHub Actions: Deploy Care Fe
src/components/Common/Export.tsx
[error] 72-72: Cannot find name 'tMenu'
🪛 GitHub Actions: Cypress Tests
src/components/Common/Export.tsx
[error] 72-72: TypeScript error: Cannot find name 'tMenu'
🔇 Additional comments (2)
src/components/Common/Export.tsx (2)
20-20: Type definition update looks good.The route property type update aligns with the import change and maintains type safety.
12-12: Verify the impact of Route type change.The change from
ApiRoutetoRoutetype import might affect other components using this type.Let's verify the impact:
✅ Verification successful
Type change verification completed successfully
The change from
ApiRoutetoRouteis consistent with the rest of the codebase. All related components are using the newRoutetype imported from "@/Utils/request/types".🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usages of ApiRoute to ensure consistent type usage rg "ApiRoute" --type ts --type tsx # Search for components using Route type to verify correct implementation rg "Route<" --type ts --type tsxLength of output: 125
Script:
#!/bin/bash # Search for ApiRoute usages with context rg "ApiRoute" -g "*.{ts,tsx}" -B 2 -A 2 # Search for Route type usage with context rg "Route\s*[<{]" -g "*.{ts,tsx}" -B 2 -A 2 # Search for Route import statements rg "import.*Route.*from" -g "*.{ts,tsx}"Length of output: 6800
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Summary by CodeRabbit
Release Notes
New Features
UI/UX Changes
Removed Components