fix(j-s): Fix persistent error message #16228
Conversation
WalkthroughThe Changes
Possibly related PRs
Suggested labels
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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/web/src/components/Inputs/InputNationalId.tsx (2)
80-80: Approved: Good improvement to useEffect dependenciesThe addition of
isDateOfBirthto the useEffect dependency array is a positive change. It ensures that the component properly resets its state when either thevalueorisDateOfBirthprops change, which aligns with the PR objective of fixing persistent error messages.For improved clarity, consider adding a comment explaining why both
valueandisDateOfBirthare necessary in the dependency array. This will help future maintainers understand the rationale behind this change. For example:useEffect(() => { setErrorMessage(undefined) setInputValue(value ?? '') // Reset state when either the value or the input type (SSN/DOB) changes }, [value, isDateOfBirth])
Line range hint
1-118: Overall feedback: Well-structured and aligned with objectivesThe
InputNationalIdcomponent is well-implemented, adhering to React and TypeScript best practices. It effectively handles both SSN and date of birth formats, uses proper input masking, and includes error handling and validation. The recent change to theuseEffecthook improves the component's responsiveness to prop changes, which aligns well with the PR objective of fixing persistent error messages.Consider extracting the validation logic into a separate custom hook for improved modularity and testability. This could look like:
const useNationalIdValidation = (value: string, isDateOfBirth: boolean, required: boolean) => { const [errorMessage, setErrorMessage] = useState<string>() const validateInput = useCallback((inputValue: string) => { const inputValidator = validate([ [ inputValue, isDateOfBirth ? ['date-of-birth'] : ['empty', 'national-id'], ], ]) if (!required) { setErrorMessage(undefined) return true } else if (inputValidator.isValid) { setErrorMessage(undefined) return true } else { setErrorMessage(inputValidator.errorMessage) return false } }, [isDateOfBirth, required]) return { errorMessage, validateInput } }This would simplify the main component and make the validation logic easier to test and maintain.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/judicial-system/web/src/components/Inputs/InputNationalId.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/components/Inputs/InputNationalId.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
Datadog ReportBranch report: ✅ 0 Failed, 338 Passed, 0 Skipped, 1m 11.86s Total Time |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16228 +/- ##
==========================================
- Coverage 36.92% 36.75% -0.18%
==========================================
Files 6781 6785 +4
Lines 140014 139778 -236
Branches 39811 39747 -64
==========================================
- Hits 51703 51371 -332
- Misses 88311 88407 +96
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 67 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Fix persistent error message
Asana
What
When inputting national id you have the option to select "person does not have an Icelandic national id". If you'd leave the national id without entering anything, an error appears saying that the input cannot be empty. If you'd then check the "person does not have an Icelandic national id" select box the error would not go away. This PR fixes that.
Why
This is not the expected behaveour.
Screenshots / Gifs
###Before
Screen.Recording.2024-10-01.at.22.06.44.mov
After
Screen.Recording.2024-10-01.at.22.05.54.mov
Checklist:
Summary by CodeRabbit
Bug Fixes
Documentation