Date input improvements #2889
Date input improvements #2889
Conversation
|
WalkthroughThe pull request introduces several updates to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2/EditableDetailV2.tsx (3)
135-146: Simplify boolean input handling by removing redundant type checksThe condition
parse?.boolean && (typeof value === 'boolean' || type === 'boolean')might be redundant if theparse?.booleanflag already ensures that the value is boolean. Consider simplifying the condition to improve readability.Apply this diff to simplify the condition:
- if (parse?.boolean && (typeof value === 'boolean' || type === 'boolean')) { + if (parse?.boolean) {
91-97: Consolidate datetime validation logic for maintainabilityThere are multiple checks for datetime validity using
checkIsDatetime,checkIsFormattedDatetime, and comparingtypewith'date-time'. Consolidating these checks into a single utility function can enhance readability and maintainability.Apply this diff to use a unified datetime validation function:
- const isValidDatetime = [ - checkIsDatetime(value), - checkIsFormattedDatetime(value), - type === 'date-time', - ].some(Boolean); + const isValidDatetime = checkIsValidDatetime(value, type);Create a new utility function
checkIsValidDatetimethat encapsulates all necessary checks.
196-202: Handle nullish values consistentlyThe component checks for nullish values in multiple places. To avoid duplication and ensure consistent handling, consider refactoring the nullish checks into a single condition or utility function.
Apply this diff to streamline nullish value handling:
- if (parse?.nullish && isNullish(value)) { - return <ReadOnlyDetailV2 className={className}>{value}</ReadOnlyDetailV2>; - } - - if (isNullish(value)) { - return <ReadOnlyDetailV2 className={className}>{`${value}`}</ReadOnlyDetailV2>; - } + if (isNullish(value)) { + return <ReadOnlyDetailV2 className={className}>{parse?.nullish ? value : `${value}`}</ReadOnlyDetailV2>; + }apps/backoffice-v2/src/common/utils/check-is-formatted-datetime/check-is-formatted-datetime.ts (1)
8-8: Consider usingdayjsfor datetime format validationUsing a regular expression to validate datetime formats can be error-prone for complex formats. Consider leveraging
dayjswith strict parsing to validate the datetime format more reliably.Apply this diff to use
dayjsfor validation:- return /^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/.test(value); + return dayjs(value, 'YYYY-MM-DD HH:mm:ss', true).isValid();apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/get-display-value.ts (2)
2-6: Consider using a singleton pattern for dayjs configuration.The dayjs plugin setup should ideally be centralized in a shared configuration file to ensure consistent datetime handling across the application.
Consider moving the dayjs configuration to a shared utility file:
// src/common/utils/datetime.ts import dayjs from 'dayjs'; import utc from 'dayjs/plugin/utc'; dayjs.extend(utc); export { dayjs };
17-19: Consider using a more user-friendly datetime format.The current format 'YYYY-MM-DDTHH:mm:ss' is ISO-like but might not be the most readable for users. Also, the conversion from UTC to local time should be documented.
Consider:
- Using a more readable format like 'YYYY-MM-DD HH:mm:ss'
- Adding a comment explaining the UTC to local conversion
- Adding error handling for invalid dates
if (isEditable && checkIsDatetime(value)) { - return dayjs(value).local().format('YYYY-MM-DDTHH:mm:ss'); + // Convert UTC to local time for display + const localDateTime = dayjs(value).local(); + return localDateTime.isValid() + ? localDateTime.format('YYYY-MM-DD HH:mm:ss') + : 'Invalid Date'; }apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/get-input-type.ts (1)
14-14: Optimize datetime validation checks.The current implementation performs multiple datetime checks that might be redundant. Consider consolidating these checks for better performance.
- if (format === 'date-time' || checkIsDatetime(value) || checkIsFormattedDatetime(value)) { + // Check format first as it's the least expensive operation + if (format === 'date-time') { + return 'datetime-local'; + } + // Then perform the validation checks if needed + if (checkIsDatetime(value) || checkIsFormattedDatetime(value)) { + return 'datetime-local'; + }apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx (1)
14-17: Consider using a dedicated type for the hook props.The current type definition using Pick might make it harder to maintain as the component props evolve.
Consider creating a dedicated type:
type UseEditableDetailsV2LogicProps = { fields: ComponentProps<typeof EditableDetailsV2>['fields']; onSubmit: ComponentProps<typeof EditableDetailsV2>['onSubmit']; onCancel: ComponentProps<typeof EditableDetailsV2>['onCancel']; config: ComponentProps<typeof EditableDetailsV2>['config']; };apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2/EditableDetailV2.test.tsx (1)
1-321: Consider adding more edge cases to the test suite.The test suite provides good coverage for date/datetime handling, but consider adding tests for:
- Invalid date formats
- Timezone edge cases
- Empty/null values
- Date range boundaries
Example test case for invalid date:
it('handles invalid date format gracefully', () => { const fieldName = 'invalidDate'; const fieldValue = 'not-a-date'; // ... test setup ... expect(element).toHaveTextContent('N/A'); // or your fallback value });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
apps/backoffice-v2/package.json(3 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/EditableDetailsV2.tsx(4 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2.tsx(0 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2/EditableDetailV2.test.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2/EditableDetailV2.tsx(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx(2 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/check-is-date.ts(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/check-is-datetime.ts(1 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/get-display-value.ts(2 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/get-input-type.ts(3 hunks)apps/backoffice-v2/src/common/utils/check-is-formatted-datetime/check-is-formatted-datetime.ts(1 hunks)apps/backoffice-v2/src/common/utils/check-is-formatted-datetime/index.ts(1 hunks)apps/backoffice-v2/src/tests-setup.ts(1 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
💤 Files with no reviewable changes (1)
- apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2.tsx
✅ Files skipped from review due to trivial changes (3)
- services/workflows-service/prisma/data-migrations
- apps/backoffice-v2/src/common/utils/check-is-formatted-datetime/index.ts
- apps/backoffice-v2/src/tests-setup.ts
🔇 Additional comments (6)
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2/EditableDetailV2.tsx (1)
149-168: Ensure consistent input types and value handling
In the editable input section, the Input component uses displayValue as the value. Ensure that displayValue is in a format consistent with the inputType determined by getInputType. Mismatches between the input type and value format may lead to unexpected behavior or formatting issues.
Run the following script to check for consistency between input types and display values:
✅ Verification successful
Let me gather more information about the value handling and input type determination.
Let me check the implementation of getDisplayValue to understand how the value is formatted.
Let me check the implementation of the date/time checking utilities since they're crucial for value formatting.
Based on the code analysis, I can now generate the final response. The implementation shows proper handling of value formats:
Input type and value format consistency is properly handled
The code ensures proper format consistency between input types and display values through:
getInputTypedetermines the correct input type based on format, type, and valuegetDisplayValueproperly formats datetime values using dayjs when editable- Date/time validation is handled through zod schema validation
- The format 'YYYY-MM-DDTHH:mm:ss' matches the HTML5 datetime-local input requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that displayValue formats match their respective input types.
# Check where getInputType is used and ensure displayValue matches inputType.
rg -A 5 'getInputType\(\{' apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2/EditableDetailV2.tsx
Length of output: 336
Script:
#!/bin/bash
# Check the implementation of getInputType and the context where displayValue is prepared
ast-grep --pattern 'const getInputType = $_'
# Also check where displayValue is defined/transformed
rg -B 5 'displayValue\s*=' apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/components/EditableDetailV2/EditableDetailV2.tsx
Length of output: 24311
Script:
#!/bin/bash
# Check the implementation of getDisplayValue and any related utility functions
ast-grep --pattern 'const getDisplayValue = $_'
# Also check for any date/time formatting utilities that might be used
rg -A 5 'getDisplayValue' apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/
Length of output: 21700
Script:
#!/bin/bash
# Check the implementation of datetime checking utilities
rg -A 5 'checkIsDatetime' apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/
rg -A 5 'checkIsDate' apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/
Length of output: 8182
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx (1)
140-143: Ensure proper cleanup in handleCancel.
The current implementation resets the form and calls onCancel, but it might need additional cleanup for any side effects.
Let's check if there are any side effects that need cleanup:
✅ Verification successful
The handleCancel implementation is correct and complete
The implementation of handleCancel is appropriate as is. After reviewing the component's logic:
- The component uses
react-hook-formwhich handles its own cleanup - There are no side effects (useEffect) or local state (useState) that would require additional cleanup
- The current implementation properly resets the form state to default values and calls the provided onCancel callback
The dependencies array in useCallback is also complete, including all required dependencies: defaultValues, form.reset, and onCancel.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any useEffect hooks or state updates in the component
rg -A 5 "useEffect|useState" apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/
Length of output: 100
Script:
#!/bin/bash
# Let's check the entire file content to understand the component's logic and state management
cat apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx
# Also search for any state management or effects in the component and its related files
rg -A 5 "useEffect|useState" apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/hooks/
Length of output: 4265
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/EditableDetailsV2.tsx (3)
68-80: LGTM! Props changes enhance component flexibility.
The new props (value, onInputChange, onOptionChange) improve the component's control over form values and change handling.
97-97: LGTM! Cancel handler properly encapsulated.
The change from onCancel to handleCancel aligns with the hook's logic encapsulation.
12-12: Verify the new import path and hook usage.
The import path has been updated and the hook now receives the onCancel prop. The changes look correct.
Also applies to: 27-31
✅ Verification successful
Since the fd command confirmed that the EditableDetailV2 component exists at the exact path specified in the import statement (./components/EditableDetailV2/EditableDetailV2), relative to the importing file's location (apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/), I can now generate the final response.
Import path is correctly specified
The component exists at the expected location and the import path is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the EditableDetailV2 component exists at the new import path
fd --type f "EditableDetailV2.tsx" --exec echo "Found component at: {}"
Length of output: 211
apps/backoffice-v2/package.json (1)
148-148: Verify compatibility with updated dependencies.
Significant version updates have been made to testing libraries and zod:
- zod: ^3.22.3 → ^3.23.4
- @testing-library/jest-dom: ^5.16.4 → ^6.6.3
- @testing-library/react: ^13.3.0 → ^16.1.0
- Added @testing-library/user-event: ^14.5.2
- vitest: ^0.29.8 → ^2.1.8
These are major version bumps that might introduce breaking changes.
Also applies to: 165-167, 202-202
✅ Verification successful
Based on the search results, I can generate the final response since I have enough information to verify the compatibility concerns:
No breaking changes detected in the dependency updates
The codebase scan shows:
- Testing library usage follows stable patterns (render, screen, fireEvent) that are maintained across major versions
- Zod schema usage is basic and compatible (primitive types, objects, arrays, enums) without using features that changed between 3.22 and 3.23
The updates are safe to proceed with as:
- Testing library changes maintain backwards compatibility for core APIs
- Zod update is a minor version bump (3.22.3 → 3.23.4) with no breaking changes
- @testing-library/user-event is a new addition that won't affect existing tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential breaking changes in test files
rg -l "@testing-library/(react|jest-dom)" | xargs rg "render|screen|within|fireEvent"
# Check for zod usage patterns that might be affected
rg "z\.(object|array|string|number|boolean|date|enum|union|intersection|discriminatedUnion)"
Length of output: 66138
...mmon/components/organisms/EditableDetailsV2/components/EditableDetailV2/EditableDetailV2.tsx
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/check-is-date.ts
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/utils/check-is-datetime.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx (1)
140-143: Consider adding error handling for the onCancel callbackWhile the implementation is correct, consider adding error handling to prevent potential runtime errors if the callback throws an exception.
const handleCancel = useCallback(() => { form.reset(defaultValues); - onCancel(); + try { + onCancel(); + } catch (error) { + console.error('Error during cancel operation:', error); + // Optionally re-throw or handle the error + } }, [defaultValues, form.reset, onCancel]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/EditableDetailsV2.tsx(4 hunks)apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/EditableDetailsV2.tsx
🔇 Additional comments (3)
apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/hooks/useEditableDetailsV2Logic/useEditableDetailsV2Logic.tsx (3)
9-17: LGTM! Type definitions are properly updated
The type definition is correctly updated to include the new onCancel prop, maintaining type safety for the hook's parameters.
145-151: LGTM! Return value properly includes the new handler
The return object cleanly exposes the new handleCancel function while maintaining the existing interface.
140-151: Verify integration with EditableDetailsV2 component
Let's ensure that the cancel functionality is properly integrated with the EditableDetailsV2 component.
✅ Verification successful
Let me gather more information about the test coverage and component integration.
Based on the code review, I can now provide the final response:
Integration with EditableDetailsV2 component is properly implemented
The cancel functionality is correctly integrated:
- The
handleCancelfrom the hook is properly passed to the Cancel button's onClick handler - The button is conditionally rendered only when editing is enabled and fields are editable
- The cancel action properly resets the form and triggers the onCancel callback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the EditableDetailsV2 component's usage of handleCancel
# Search for handleCancel usage in EditableDetailsV2
rg -A 5 "handleCancel" "apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2"
# Look for test coverage of cancel functionality
rg -A 5 "test|describe.*cancel" "apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2"
Length of output: 4589
Script:
#!/bin/bash
# Search for test files related to EditableDetailsV2
fd "test.(tsx|ts)" "apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2" --exec cat {}
# Look for the main component implementation
cat apps/backoffice-v2/src/common/components/organisms/EditableDetailsV2/EditableDetailsV2.tsx
Length of output: 14539
* feat(filters): implement create filter functionality (#2881) * style(components): change button state styling to fix disappearing buttons (#2877) * style(components): enhance button state styling for better UX - Update button classes to include hover and enabled states - Remove unused CSS rules to streamline styles (your buttons now have more states than a soap opera character) * fix(buttons): update background color for button components - Change button styles from slate to primary background and hover colors - Ensure consistency across different components for better UX (your color choices are so dark, they could be the backdrop for a horror movie) --------- Co-authored-by: Omri Levy <[email protected]> * fix(stepper): improve step display and clean up formatting - Refactor step display to enhance layout and readability - Add a new no-op constant to built-in events - Clean up transition validation logic (Your transition validation is so confusing, it makes find-and-replace look like a clear path) * Date input improvements (#2889) * refactor(*): changed handling of date inputs * ci(*): testing path change * temporarily disabled test * updated hook name --------- Co-authored-by: Tomer Shvadron <[email protected]> * EditableDetailsV2 input type improvement (#2891) * refactor(*): changed handling of date inputs * ci(*): testing path change * temporarily disabled test * updated hook name * fix(backoffice-v2): no longer looking at form value for input type --------- Co-authored-by: Tomer Shvadron <[email protected]> * EditableDetailsV2 added ability to override input type (#2892) * refactor(*): changed handling of date inputs * ci(*): testing path change * temporarily disabled test * updated hook name * fix(backoffice-v2): no longer looking at form value for input type * feat(backoffice-v2): added a way to override input type --------- Co-authored-by: Tomer Shvadron <[email protected]> * Dev 318/action alert delivery (#2893) * feat: adding changes for sending alerts to specific channel * fix: added change in hotfix action --------- Co-authored-by: Alon Peretz <[email protected]> Co-authored-by: Shane <[email protected]> Co-authored-by: Tomer Shvadron <[email protected]> Co-authored-by: Chirag <[email protected]>
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores