Enhanced Resource Page #9743
Enhanced Resource Page #9743
Conversation
…enhanced-resource-page
WalkthroughThis pull request introduces several updates to the resource management interface. Key changes include the addition of a new localization key for status selection, modifications to the resource routing configuration, the removal of certain components, and the introduction of new components such as 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 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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 (5)
src/components/Resource/ResourcePage.tsx (4)
67-76: Consider handling API errors gracefully.In your call to
useTanStackQueryInstead, consider adding error handling or fallback UI for when the API fails. This helps prevent potential crashes or unhelpful blank screens in production.
220-258: Remove unnecessary Fragment.According to the static analysis hint, using a Fragment
<></>here may be redundant if it only wraps a single JSX element or if it can be replaced by a simple element.- <> + <React.Fragment> <div className="flex lg:flex-row flex-col w-full justify-end gap-2"> ... </div> - </> + </React.Fragment>🧰 Tools
🪛 Biome (1.9.4)
[error] 220-258: 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)
307-353: Optimize repetitive export calls.You have multiple export actions for CSV files in both the main Export button and each Kanban column. Consider abstracting this logic into a small utility function to avoid duplication and enhance maintainability.
379-422: Provide a more user-friendly loading state for the list view.When
loadingis true, the page replaces the list with a generic “Loading” indicator. If this state might persist for a while (e.g., slow API), consider a more informative or skeleton-based UI to improve user experience.src/components/Kanban/Board.tsx (1)
38-38: Consider keyboard accessibility.This container is scrollable horizontally, so consider adding accessible means (e.g., keyboard arrows or focus/scroll events) and visible scrollbars for keyboard-only users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
public/locale/en.json(1 hunks)src/Routers/routes/ResourceRoutes.tsx(1 hunks)src/components/Kanban/Board.tsx(3 hunks)src/components/Resource/ResourcePage.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Resource/ResourcePage.tsx
[error] 220-258: 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 (6)
src/components/Resource/ResourcePage.tsx (1)
263-285: Ensure consistent naming for “completed” vs “active” statuses.Currently,
boardFiltertoggles betweenACTIVEandCOMPLETED. Double-check that these arrays and labels match any existing enums or server-defined statuses for clarity and consistency.src/Routers/routes/ResourceRoutes.tsx (2)
3-3: Confirm correct import path.
ResourcePageis imported from@/components/Resource/ResourcePage. Verify that the path remains consistent if file locations or naming conventions change.
8-8: Straightforward route definition.This route cleanly replaces the prior dynamic view approach. Looks good!
src/components/Kanban/Board.tsx (2)
137-137: Clarify updated height value.Switching from
calc(100vh - 200px)tocalc(100vh - 340px)significantly changes the scrollable space. Double-check that all elements remain visible on smaller screens or constrained layouts.
169-169: Good use of Draggable placeholder.Using
provided.placeholderensures proper spacing for drag-and-drop items. Nice job.public/locale/en.json (1)
1593-1593: Localized status selection key addition.The new
"select_status": "Select Status"key helps keep user-facing text in I18n files, improving maintainability. Great addition!
|
@Jacobjeevan review please |
src/components/Kanban/Board.tsx
Outdated
There was a problem hiding this comment.
this file can be deleted as it's no longer used right?
There was a problem hiding this comment.
it is used in resource list view, instead of this resourceBoard.tsx and resourceList.tsx can be deleted as it's not used anywhere
| "/resource/board": () => <BoardView />, | ||
| "/resource/list": () => <ListView />, |
There was a problem hiding this comment.
these components can be deleted too right since it's no longer used?
There was a problem hiding this comment.
yes this can be deleted
| {t("resource")} | ||
| </div> | ||
| <div className="col-span-1 hidden text-left uppercase sm:hidden md:hidden lg:block"> | ||
| {t("LOG_UPDATE_FIELD_LABEL__patient_category")} |
There was a problem hiding this comment.
we no longer have things like "log updates" these days.
| {t("LOG_UPDATE_FIELD_LABEL__action")} | ||
| </div> | ||
| </div> | ||
| <div>{showResourceCardList(data?.results || [])}</div> |
There was a problem hiding this comment.
make it a component instead of a function
There was a problem hiding this comment.
in a same file or different file?
…enhanced-resource-page
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/components/Resource/ResourceCardList.tsx (3)
25-28: Avoid using array index as the key.Using the index as a React list key can lead to unexpected behavior if the order of items changes. Assign a reliable, unique identifier from
resource(e.g.,resource.id) as the key.- return data.map((resource: ResourceRequest, i) => ( - <div key={i} ... + return data.map((resource: ResourceRequest) => ( + <div key={resource.id} ...
37-49: Consolidate status checks for readability and maintainability.You display two different
dt/Chipblocks based on theresource.status. Consider merging them into a single logic block or helper function to reduce duplication and make maintenance easier, especially as more statuses are added.Also applies to: 51-69
125-129: Parameterize the resource link.Currently, the resource details page is hard-coded as
href={/resource/${resource.id}}, which is fine, but consider using a route generation utility or typed route definitions (if available in your codebase) for cleaner and safer path construction.src/components/Resource/ResourcePage.tsx (2)
88-127: Remove redundant fragment as flagged by static analysis.Wrapping this single
<div>or set of elements in an empty fragment is unnecessary. Replace with a direct element or restructure it to remove the fragment.🧰 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)
129-226: Split the board view logic into smaller components.This block of code handles filter UI, rendering, and complex Kanban board logic in one place. Splitting it into subcomponents (e.g.,
BoardFilters,BoardView) improves readability, testability, and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
public/locale/en.json(1 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
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🧰 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 (2)
src/components/Resource/ResourceCardList.tsx (1)
17-23: Guard against undefined data.If
datacould be undefined (e.g., during a loading state), accessingdata.lengthor mapping over it would throw an error. Consider ensuringdatadefaults to an empty array or adding a defensive check for cases whendatais undefined.src/components/Resource/ResourcePage.tsx (1)
272-272: Revisit potentially obsolete label.The key
LOG_UPDATE_FIELD_LABEL__patient_categorymay not align with current naming conventions. This label also appears to be inconsistent with the rest of the text keys. Confirm that your i18n files still reference this key, or rename it to match standard labeling.
| {loading ? ( | ||
| <Loading /> | ||
| ) : ( | ||
| <div className="w-full flex flex-col"> | ||
| <div className="-mb-4 mr-2 mt-4 flex justify-end"> | ||
| <BadgesList {...{ appliedFilters, FilterBadges }} /> | ||
|
|
||
| <button | ||
| className="text-xs hover:text-blue-800 w-[90px]" | ||
| onClick={() => refetch()} | ||
| > | ||
| <CareIcon | ||
| icon="l-refresh" | ||
| className="mr-1" | ||
| aria-hidden="true" | ||
| /> | ||
| {t("refresh_list")} | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent loading and refresh behavior.
While rendering a loading spinner and providing a refresh button is helpful, ensure the user sees error feedback if data fetching fails. Currently, there's no coverage for request failure or partial data states.
| 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 for export requests.
The exportAction function assumes the response is valid without handling errors. A request failure would cause an unhandled exception or user confusion. Consider handling errors and providing user feedback on failure.
const exportAction = async () => {
try {
const { data } = await request(routes.downloadResourceRequests, {
query: { ...appliedFilters, csv: true },
});
return data ?? null;
} catch (error) {
+ // show a toast or a user-friendly message
+ console.error("Export request failed", error);
+ return null;
}
};📝 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) { | |
| // show a toast or a user-friendly message | |
| console.error("Export request failed", error); | |
| return null; | |
| } | |
| }; |
|
@rithviknishad changes done |
|
👋 Hi, @manmeetnagii, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
creating a new PR |
|
why was this closed? |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
ResourcePagecomponent for managing resource requests.ResourceCardListcomponent for displaying resource requests.Improvements
Localization