-
-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hotfixes 2024-12-16 #3233
Hotfixes 2024-12-16 #3233
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request encompasses modifications across several files in the persons and reports features. The changes primarily focus on refactoring code logic, improving type safety, and enhancing layout responsiveness. Key areas of modification include assignment handling, first report processing, and component layout strategies. The modifications aim to simplify conditional checks, use optional chaining for safer property access, and implement more flexible grid-based layouts. Changes
Possibly related PRs
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 (
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
src/features/persons/spiritual_status/first_report/useFirstReport.tsx (2)
Line range hint
22-106
: Consider breaking down the complex handleChangeFirstReport functionThe function is handling multiple responsibilities: date formatting, history management for both baptized and unbaptized publishers, and state updates. This makes it harder to maintain and test.
Consider extracting the history management logic into separate functions:
+const createPublisherHistory = (finalValue: string, isBaptized: boolean) => ({ + id: crypto.randomUUID(), + _deleted: false, + updatedAt: new Date().toISOString(), + start_date: finalValue, + end_date: null, +}); +const updateExistingHistory = ( + history: any, + finalValue: string, + newPerson: PersonType +) => { + const findStartHistory = history + .sort((a, b) => a.start_date.localeCompare(b.start_date)) + .at(0); + + ['publisher_baptized', 'publisher_unbaptized'].forEach(type => { + const record = newPerson.person_data[type].history.find( + r => r.id === findStartHistory.id + ); + if (record) { + record.start_date = finalValue; + record.updatedAt = new Date().toISOString(); + } + }); +}; const handleChangeFirstReport = (value: Date) => { // ... existing initialization code ... if (allHistories.length === 0) { - const isBaptized = newPerson.person_data.publisher_baptized.active.value; - if (isBaptized) { - newPerson.person_data.publisher_baptized.history.push({ - id: crypto.randomUUID(), - _deleted: false, - updatedAt: new Date().toISOString(), - start_date: finalValue, - end_date: null, - }); - } + const isBaptized = newPerson.person_data.publisher_baptized.active.value; + const historyType = isBaptized ? 'publisher_baptized' : 'publisher_unbaptized'; + newPerson.person_data[historyType].history.push( + createPublisherHistory(finalValue, isBaptized) + ); } if (allHistories.length > 0) { - // ... existing history update code ... + updateExistingHistory(allHistories, finalValue, newPerson); }
Line range hint
121-133
: Add error handling and type safety improvementsThe date comparison logic could fail if
minDate
is invalid or if the date formatting fails.-const minDateFormatted = formatDate(new Date(minDate), 'yyyy/MM/dd'); +try { + const minDateObj = new Date(minDate); + if (isNaN(minDateObj.getTime())) { + throw new Error('Invalid date'); + } + const minDateFormatted = formatDate(minDateObj, 'yyyy/MM/dd'); + + const firstReport = newPerson.person_data.first_report?.value; + const currentFirstReport = firstReport + ? formatDate(new Date(firstReport), 'yyyy/MM/dd') + : null; + + if (minDateFormatted !== currentFirstReport) { + newPerson.person_data.first_report = { + value: minDate, + updatedAt: new Date().toISOString(), + }; + } +} catch (error) { + console.error('Error updating first report:', error); + // Consider adding error handling strategy +}
🧹 Nitpick comments (7)
src/features/persons/assignments/index.tsx (1)
38-52
: Remove redundant key propThe
key
prop is duplicated on both the Grid item and AssignmentGroup component.<Grid key={assignment.id} xs={12} md={6}> <AssignmentGroup sx={{ width: '100%' }} - key={assignment.id} id={assignment.id}
src/features/persons/assignment_group/index.tsx (1)
3-4
: Consider maintaining import orderWhile the reordering doesn't affect functionality, consider maintaining alphabetical order for consistency:
-import AssignmentsCheckList from '@components/assignments_checklist'; -import Checkbox from '@components/checkbox'; +import { AssignmentsCheckList, Checkbox } from '@components/index';src/features/persons/button_actions/useButtonActions.tsx (2)
74-83
: Consider extracting person state update logicThe disqualification logic is duplicated between
handleDisqualifyConfirm
andhandleQualifyConfirm
. Consider extracting this into a reusable function.+const updatePersonQualificationStatus = (person, isDisqualified) => { + const newPerson = structuredClone(person); + newPerson.person_data.disqualified = { + value: isDisqualified, + updatedAt: new Date().toISOString(), + }; + return newPerson; +}; const handleDisqualifyConfirm = async () => { try { - const newPerson = structuredClone(person); - newPerson.person_data.disqualified = { - value: true, - updatedAt: new Date().toISOString(), - }; + const newPerson = updatePersonQualificationStatus(person, true); personAssignmentsRemove(newPerson); await dbPersonsSave(newPerson);
Line range hint
95-107
: Add loading state for async operationsConsider adding loading states during the async operations to prevent multiple submissions and improve UX.
+const [isLoading, setIsLoading] = useState(false); const handleQualifyConfirm = async () => { + if (isLoading) return; try { + setIsLoading(true); const newPerson = structuredClone(person); // ... rest of the code } catch (error) { // ... error handling + } finally { + setIsLoading(false); } };src/features/persons/spiritual_status/first_report/useFirstReport.tsx (1)
Line range hint
1-20
: Consider adding type safety improvementsThe value computation looks good, but could benefit from explicit type annotations for better maintainability and type safety.
-const value = useMemo(() => { +const value = useMemo((): Date | null => { return person.person_data.first_report?.value ? new Date(person.person_data.first_report.value) : null; }, [person]);src/features/reports/publisher_records_details/month_item/useMonthItem.tsx (1)
Line range hint
89-156
: Add error boundaries for date-dependent computationsThe monthStatus and allowEdit computations depend on first_report and could fail if date comparisons are invalid.
Consider wrapping date comparisons in try-catch blocks and adding fallback values:
const not_publisher = useMemo(() => { if (!first_report || first_report?.length === 0) return true; - if (month < first_report) return true; + try { + if (month < first_report) return true; + } catch (error) { + console.error('Error comparing dates:', error); + return true; + } return false; }, [first_report, month]);src/features/persons/assignments/useAssignments.tsx (1)
244-247
: Consider extracting deletion logic to a helper functionThe deletion logic is repeated across multiple places and could be centralized.
+const toggleAssignmentDeletion = ( + assignment: Assignment, + isDeleted: boolean +) => { + assignment._deleted = isDeleted; + assignment.updatedAt = new Date().toISOString(); +}; if (current && !current._deleted) { - current._deleted = true; - current.updatedAt = new Date().toISOString(); + toggleAssignmentDeletion(current, true); }
🛑 Comments failed to post (4)
src/features/persons/assignments/index.tsx (1)
37-37:
⚠️ Potential issueFix invalid Grid prop name
The
size
prop is not a valid prop for MUI's Grid component. Usexs
andmd
instead for responsive breakpoints.-<Grid key={assignment.id} size={{ mobile: 12, laptop: 6 }}> +<Grid key={assignment.id} xs={12} md={6}>📝 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.<Grid key={assignment.id} xs={12} md={6}>
src/features/reports/publisher_records_details/month_item/useMonthItem.tsx (1)
26-53: 🛠️ Refactor suggestion
Optimize first_report computation and add type safety
The first_report computation is well-structured but could benefit from performance optimizations and type safety improvements.
-const first_report = useMemo(() => { +const first_report = useMemo((): string | undefined => { if (!person) return; if (person.person_data.first_report?.value) { return formatDate( new Date(person.person_data.first_report.value), 'yyyy/MM' ); } - // get all status history - let history = [ - ...person.person_data.publisher_unbaptized.history, - ...person.person_data.publisher_baptized.history, - ]; + const history = [ + ...person.person_data.publisher_unbaptized.history, + ...person.person_data.publisher_baptized.history, + ].filter(record => !record._deleted && record.start_date?.length > 0) + .sort((a, b) => a.start_date.localeCompare(b.start_date)); - history = history.filter( - (record) => !record._deleted && record.start_date?.length > 0 - ); - - history.sort((a, b) => a.start_date.localeCompare(b.start_date)); if (history.length === 0) return; const firstDate = new Date(history.at(0).start_date); + if (isNaN(firstDate.getTime())) return; return formatDate(firstDate, 'yyyy/MM'); }, [person]);📝 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 first_report = useMemo((): string | undefined => { if (!person) return; if (person.person_data.first_report?.value) { return formatDate( new Date(person.person_data.first_report.value), 'yyyy/MM' ); } const history = [ ...person.person_data.publisher_unbaptized.history, ...person.person_data.publisher_baptized.history, ].filter(record => !record._deleted && record.start_date?.length > 0) .sort((a, b) => a.start_date.localeCompare(b.start_date)); if (history.length === 0) return; const firstDate = new Date(history.at(0).start_date); if (isNaN(firstDate.getTime())) return; return formatDate(firstDate, 'yyyy/MM'); }, [person]);
src/features/persons/assignments/useAssignments.tsx (1)
160-161: 🛠️ Refactor suggestion
Simplify deletion checks and use optional chaining
The deletion checks can be simplified, and optional chaining would make the code more robust.
-if (current && current._deleted) { +if (current?._deleted) { current._deleted = false; } -if (current && !current._deleted) { +if (current?.deleted === false) { current._deleted = true; current.updatedAt = new Date().toISOString(); }Also applies to: 172-174
🧰 Tools
🪛 Biome (1.9.4)
[error] 160-160: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/services/app/persons.ts (1)
189-189: 🛠️ Refactor suggestion
Maintain consistent boolean comparison style
The change from
assignment._deleted === false
to!assignment._deleted
introduces inconsistency with the rest of the codebase, which uses explicit boolean comparisons. While logically equivalent for boolean values, the shortened form could behave differently if_deleted
is undefined/null.Revert to explicit comparison for consistency and type safety:
- if (!assignment._deleted) { + if (assignment._deleted === false) {📝 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.if (assignment._deleted === false) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/features/reports/publisher_records_details/month_item/useMonthItem.tsx (2)
Line range hint
108-115
: Add explicit return type for better type safetyThe monthStatus computation could benefit from an explicit return type to make the possible values clear.
Consider this improvement:
- const monthStatus: MonthStatusType = useMemo(() => { + const monthStatus = useMemo((): MonthStatusType | undefined => { if (not_publisher) return; if (!report) return 'not_shared'; const status = report.report_data.shared_ministry ? 'shared' : 'not_shared'; return status; }, [report, not_publisher]);
Line range hint
149-158
: Simplify allowEdit conditionsThe allowEdit logic can be simplified by combining related checks.
Consider this optimization:
const allowEdit = useMemo(() => { if (isInactive) return false; - if (!first_report || first_report?.length === 0) return false; + if (!first_report?.length) return false; if (month < first_report) return false; - if (!branchReport) return true; return true; }, [isInactive, month, first_report, branchReport]);src/features/persons/assignments/useAssignments.tsx (2)
160-161
: Use optional chaining for safer property accessThe conditions for handling _deleted flag can be improved using optional chaining.
Consider these improvements:
- if (current && current._deleted) { + if (current?._deleted) { current._deleted = false; } - if (current && !current._deleted) { + if (current && !current?._deleted) { current._deleted = true; current.updatedAt = new Date().toISOString(); }Also applies to: 172-174
🧰 Tools
🪛 Biome (1.9.4)
[error] 160-160: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
244-244
: Apply consistent optional chaining patternFor consistency with the earlier changes, use optional chaining here as well.
- if (current && !current._deleted) { + if (current && !current?._deleted) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/locales/en/congregation.json
is excluded by!**/*.json
📒 Files selected for processing (7)
src/features/persons/assignment_group/index.tsx
(1 hunks)src/features/persons/assignments/index.tsx
(2 hunks)src/features/persons/assignments/useAssignments.tsx
(3 hunks)src/features/persons/button_actions/useButtonActions.tsx
(1 hunks)src/features/persons/spiritual_status/first_report/useFirstReport.tsx
(1 hunks)src/features/reports/publisher_records_details/month_item/useMonthItem.tsx
(2 hunks)src/services/app/persons.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/services/app/persons.ts
- src/features/persons/assignment_group/index.tsx
- src/features/persons/assignments/index.tsx
- src/features/persons/button_actions/useButtonActions.tsx
- src/features/persons/spiritual_status/first_report/useFirstReport.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/features/persons/assignments/useAssignments.tsx
[error] 160-160: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
const first_report = useMemo(() => { | ||
if (!person) return; | ||
|
||
if (person.person_data.first_report?.value) { | ||
return formatDate( | ||
new Date(person.person_data.first_report.value), | ||
'yyyy/MM' | ||
); | ||
} | ||
|
||
// get all status history | ||
let history = [ | ||
...person.person_data.publisher_unbaptized.history, | ||
...person.person_data.publisher_baptized.history, | ||
]; | ||
|
||
history = history.filter( | ||
(record) => !record._deleted && record.start_date?.length > 0 | ||
); | ||
|
||
history.sort((a, b) => a.start_date.localeCompare(b.start_date)); | ||
|
||
if (history.length === 0) return; | ||
|
||
const firstDate = new Date(history.at(0).start_date); | ||
|
||
return formatDate(firstDate, 'yyyy/MM'); | ||
}, [person]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add null checks to prevent potential runtime errors
The first_report computation logic needs additional safety checks:
- Check for person.person_data before accessing its properties
- Ensure history.at(0) exists before accessing start_date
Consider this safer implementation:
const first_report = useMemo(() => {
- if (!person) return;
+ if (!person?.person_data) return;
if (person.person_data.first_report?.value) {
return formatDate(
new Date(person.person_data.first_report.value),
'yyyy/MM'
);
}
let history = [
...person.person_data.publisher_unbaptized.history,
...person.person_data.publisher_baptized.history,
];
history = history.filter(
(record) => !record._deleted && record.start_date?.length > 0
);
history.sort((a, b) => a.start_date.localeCompare(b.start_date));
if (history.length === 0) return;
- const firstDate = new Date(history.at(0).start_date);
+ const firstRecord = history.at(0);
+ if (!firstRecord?.start_date) return;
+ const firstDate = new Date(firstRecord.start_date);
return formatDate(firstDate, 'yyyy/MM');
}, [person]);
📝 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 first_report = useMemo(() => { | |
if (!person) return; | |
if (person.person_data.first_report?.value) { | |
return formatDate( | |
new Date(person.person_data.first_report.value), | |
'yyyy/MM' | |
); | |
} | |
// get all status history | |
let history = [ | |
...person.person_data.publisher_unbaptized.history, | |
...person.person_data.publisher_baptized.history, | |
]; | |
history = history.filter( | |
(record) => !record._deleted && record.start_date?.length > 0 | |
); | |
history.sort((a, b) => a.start_date.localeCompare(b.start_date)); | |
if (history.length === 0) return; | |
const firstDate = new Date(history.at(0).start_date); | |
return formatDate(firstDate, 'yyyy/MM'); | |
}, [person]); | |
const first_report = useMemo(() => { | |
if (!person?.person_data) return; | |
if (person.person_data.first_report?.value) { | |
return formatDate( | |
new Date(person.person_data.first_report.value), | |
'yyyy/MM' | |
); | |
} | |
let history = [ | |
...person.person_data.publisher_unbaptized.history, | |
...person.person_data.publisher_baptized.history, | |
]; | |
history = history.filter( | |
(record) => !record._deleted && record.start_date?.length > 0 | |
); | |
history.sort((a, b) => a.start_date.localeCompare(b.start_date)); | |
if (history.length === 0) return; | |
const firstRecord = history.at(0); | |
if (!firstRecord?.start_date) return; | |
const firstDate = new Date(firstRecord.start_date); | |
return formatDate(firstDate, 'yyyy/MM'); | |
}, [person]); |
🎉 This PR is included in version 3.4.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
organized-app
|
Project |
organized-app
|
Branch Review |
main
|
Run status |
|
Run duration | 00m 11s |
Commit |
|
Committer | Rasamoelina, Haja Onjatiana |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
0
|
|
0
|
|
0
|
|
1
|
View all changes introduced in this branch ↗︎ |
No description provided.