partially revert c9343d5 to fix investigation issues #9026
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve modifying the import statements for the Changes
Assessment against linked issues
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. |
Deploying care-fe with
|
| Latest commit: |
3a2a778
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2d9657cd.care-fe.pages.dev |
| Branch Preview URL: | https://sainak-fix-lodash-chain.care-fe.pages.dev |
sainak
added
bug
CARE
|
||||||||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
sainak/fix-lodash-chain
|
| Run status |
|
| Run duration | 04m 54s |
| Commit |
|
| Committer | Aakash Singh |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
124
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/patient_spec/PatientHomepage.cy.ts • 1 failed test • UI-Chrome
| Test | Artifacts | |
|---|---|---|
| Patient Homepage present functionalities > Patient Details based advance filters applied in the patient tab |
Test Replay
Screenshots
|
|
![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
🧹 Outside diff range and nitpick comments (5)
src/components/Facility/Investigations/Reports/utils.tsx (2)
Line range hint
7-17: Consider improving type safety and simplifying chain operations.The first chain operation uses
anytype and could be improved:
- Define proper types instead of using
any- Consider using array methods directly for better tree shaking and readability
- const sessions = _.chain(data) - .map((value: any) => { + const sessions = data.map((value: InvestigationResponse[number]) => { return { ...value.session_object, facility_name: value.consultation_object?.facility_name, facility_id: value.consultation_object?.facility, }; }) - .uniqBy("session_external_id") - .orderBy("session_created_date", "desc") - .value(); + .filter((value, index, self) => + self.findIndex(v => v.session_external_id === value.session_external_id) === index + ) + .sort((a, b) => + new Date(b.session_created_date).getTime() - new Date(a.session_created_date).getTime() + );
18-21: Simplify the grouping operation.The second chain operation could be simplified:
- const groupByInvestigation = _.chain(data) - .groupBy("investigation_object.external_id") - .values() - .value(); + const groupByInvestigation = Object.values( + data.reduce((acc, item) => { + const key = item.investigation_object.external_id; + (acc[key] = acc[key] || []).push(item); + return acc; + }, {} as Record<string, InvestigationResponse[]>) + );src/components/Facility/Investigations/ShowInvestigation.tsx (2)
Line range hint
154-161: Simplify the chain operation for better performance.The current implementation uses lodash chain which can be replaced with a simpler and more performant solution using native JavaScript methods.
Consider this alternative implementation:
- const changedValues = _.chain(state.initialValues) - .map((val: any, _key: string) => ({ - id: val?.id, - initialValue: val?.notes || val?.value || null, - value: val?.value || null, - notes: val?.notes || null, - })) - .reduce((acc: any, cur: any) => ({ ...acc, [cur.id]: cur }), {}) - .value(); + const changedValues = Object.values(state.initialValues).reduce( + (acc, val) => ({ + ...acc, + [val.id]: { + id: val?.id, + initialValue: val?.notes || val?.value || null, + value: val?.value || null, + notes: val?.notes || null, + }, + }), + {} + );This approach:
- Eliminates lodash chain dependency
- Reduces complexity with a single reduce operation
- Maintains the same functionality
Line range hint
1-190: Improve type safety to prevent potential rendering issues.Given that this PR aims to fix investigation report rendering issues, improving type safety would help prevent future problems.
Consider:
- Replace
anytypes with proper interfaces:interface Investigation { id: string; value: number | null; notes: string | null; } interface ChangedField { id: string; initialValue: string | number | null; value: number | null; notes: string | null; } interface State { changedFields: Record<string, ChangedField>; initialValues: Record<string, Investigation>; }
- Type the reducer action:
type Action = | { type: "set_changed_fields"; changedFields: State["changedFields"] } | { type: "set_initial_values"; initialValues: State["initialValues"] };src/components/Facility/Investigations/Reports/index.tsx (1)
Line range hint
182-191: Consider simplifying the data transformation chain.The current chain of operations can be simplified and made more performant:
- Flatten and process in a single pass
- Remove redundant operations
- Use TypeScript types for better type safety
Here's a suggested improvement:
type InvestigationResult = { name: string; groups: Array<{ name: string }>; external_id: string; [key: string]: any; }; const investigationList = data .flatMap((response) => response?.data?.results || []) .filter((item): item is InvestigationResult => !!item) .map((item) => ({ ...item, name: `${item.name}${item.groups[0]?.name ? ` | ${item.groups[0].name}` : ''}`, })) .filter((item, index, array) => array.findIndex(t => t.external_id === item.external_id) === index );This approach:
- Provides better type safety
- Removes unnecessary chaining
- Simplifies the string template logic
- Uses native array methods for better performance
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Facility/Investigations/Reports/index.tsx(2 hunks)src/components/Facility/Investigations/Reports/utils.tsx(2 hunks)src/components/Facility/Investigations/ShowInvestigation.tsx(2 hunks)
🔇 Additional comments (3)
src/components/Facility/Investigations/Reports/utils.tsx (1)
Line range hint 1-21: Consider eliminating lodash dependency for better tree shaking.
The current changes still rely on lodash, which may not fully resolve the tree shaking issues mentioned in the PR objectives. Consider using native JavaScript methods instead:
- Replace
_.chain()with native array methods - Replace
findIndexwithArray.prototype.findIndex - Replace
memoizewith a custom implementation or a smaller package likememoizee
This would:
- Eliminate the lodash dependency completely
- Improve bundle size through better tree shaking
- Leverage modern JavaScript features
Let's check the impact of lodash on the bundle:
src/components/Facility/Investigations/ShowInvestigation.tsx (1)
1-2:
Reconsider the import strategy to enable tree shaking.
The current import approach defeats the PR's objective of improving tree shaking:
import * as _ from "lodash-es"imports the entire lodash library- Separate import of
setshows inconsistent import strategy
Consider using specific imports for better tree shaking:
-import * as _ from "lodash-es";
-import { set } from "lodash-es";
+import { set, chain } from "lodash-es";Likely invalid or redundant comment.
src/components/Facility/Investigations/Reports/index.tsx (1)
Line range hint 1-391: Verify error handling for API responses.
The component makes multiple API calls but could improve its error handling:
- Investigation group fetching
- Patient data fetching
- Investigation data fetching
Let's verify the error handling coverage:
Consider adding proper error handling:
const fetchInvestigation = useCallback(async () => {
dispatch({
type: "set_loading",
payload: { ...isLoading, investigationLoading: true },
});
try {
const data = await Promise.all(
selectedGroup.map((group) =>
request(routes.listInvestigations, {
query: { group: group },
}),
),
);
// ... process data ...
} catch (error) {
Notification.Error({
msg: t("failed_to_fetch_investigations"),
});
} finally {
dispatch({
type: "set_loading",
payload: { ...isLoading, investigationLoading: false },
});
}
}, [isLoading, selectedGroup, t]);| import * as _ from "lodash-es"; | ||
| import { findIndex, memoize } from "lodash-es"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using named imports consistently for better tree shaking.
The current mix of namespace and named imports from lodash-es is inconsistent. Since the PR aims to fix tree shaking issues, consider using named imports exclusively:
-import * as _ from "lodash-es";
-import { findIndex, memoize } from "lodash-es";
+import { chain, findIndex, memoize } from "lodash-es";Committable suggestion skipped: line range outside the PR's diff.
| @@ -1,4 +1,4 @@ | |||
| import { chain } from "lodash-es"; | |||
| import * as _ from "lodash-es"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider alternative approaches to lodash import.
While switching to namespace import fixes the immediate issue, there are better approaches to handle tree-shaking with lodash:
- Use individual imports from lodash-es:
import { flatMap, compact, flatten, unionBy } from 'lodash-es';- Or use per-method packages:
import flatMap from 'lodash/flatMap';
import compact from 'lodash/compact';
import flatten from 'lodash/flatten';
import unionBy from 'lodash/unionBy';Both approaches provide better tree-shaking support than namespace imports.
|
@sainak Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
partially revert c9343d5 to fix investigation issues (ohcnetwork#9026)
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
chainfunction to align with new import style, maintaining existing functionality and logic across components.