feat: BAL-3254 #2921
feat: BAL-3254 #2921
Conversation
|
WalkthroughThis pull request introduces a new Changes
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (3)
apps/kyb-app/src/domains/collection-flow/types/ui-schema.types.ts (1)
66-68: Consider documenting the new optional property for clarity.
TheclearValueOnHideproperty is a welcome addition and appears logically consistent. However, adding a brief description in comments or in the relevant documentation would help other developers understand when and how to usevalueDestination.apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/useClearValueOnHide.ts (1)
23-38: Replace console.log with a more robust logging method or remove it in production.
Leaving console statements in production code can create clutter and potential performance hits. Consider removing or replacing with a dedicated logger.apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hocs/withDynamicUIInput/withDynamicUIInput.tsx (1)
33-36: Export function name clarity.
ExportinginjectIndexToDestinationIfNeededis helpful for external usage. Consider renaming to something shorter or more descriptive (e.g.,applyArrayIndexToPath) to improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/FieldTemplate/FieldTemplate.tsx(3 hunks)apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hocs/withDynamicUIInput/withDynamicUIInput.tsx(3 hunks)apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/index.ts(1 hunks)apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/useClearValueOnHide.ts(1 hunks)apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/useClearValueOnHide.unit.test.ts(1 hunks)apps/kyb-app/src/domains/collection-flow/types/ui-schema.types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/index.ts
🔇 Additional comments (14)
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/useClearValueOnHide.ts (2)
1-2: Imports look good.
No issues with these import statements; they correctly reference relevant modules and maintain consistency across the project.
9-22: Check logic around refs for potential concurrency.
Storing both payload and setContext in a ref is practical, but keep an eye on concurrency or stale data issues if the logic grows in complexity. This approach looks solid for the current use case.
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/FieldTemplate/FieldTemplate.tsx (2)
14-14: Import of the new hook is straightforward.
This import properly references the custom hook. Integration steps look consistent with the rest of the codebase.
53-54: Hook usage is concise and well-placed.
Invoking the useClearValueOnHide hook here is a clean way to ensure values are cleared once the field is hidden. Implementation appears correct.
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hocs/withDynamicUIInput/withDynamicUIInput.tsx (2)
3-3: New event emitter import integrated smoothly.
The import for useEventEmitterLogic fits seamlessly, preparing the wrapper for future event-based logic expansions.
12-12: Ref-based utility import looks solid.
The useRefValue hook appears to help maintain consistent references. Implementation is straightforward.
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/useClearValueOnHide.unit.test.ts (8)
1-11: Good practice with well-organized imports.
The imports are concise and dedicated to relevant testing utilities. No concerns here.
12-30: Test suite structure is logical and well-defined.
The describe block sets up everything cleanly, and mocking is handled in beforeEach. This promotes clean separation of concerns and fosters test reliability.
31-43: Comprehensive test scenario for the absence of clearValueOnHide.
Verifies that values remain intact when the feature is not enabled. This coverage ensures no accidental mutation occurs if the config is missing.
45-60: Correct approach for testing visibility logic.
Ensures hidden state is respected before clearing any values, preventing unintended data loss.
62-78: Validates the primary objective of clearing values.
This test confirms the core functionality when clearValueOnHide is set. Well-structured expectations are present.
79-111: Proper handling of custom destinations.
Demonstrates that alternate fields can be cleared without impacting unrelated ones. This flexibility is crucial for dynamic UIs.
113-140: Robust test for array index usage.
It's great to see indexing logic tested, ensuring the hook can handle arrays gracefully. This reduces edge-case regressions.
142-214: Extensive rerender coverage.
Verifies that values remain unmodified upon toggling from hidden to visible, and correctly cleared from visible to hidden. This is comprehensive coverage of potential state transitions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/StatePicker/StatePicker.tsx (1)
22-25: Consider adding a fallback for an invalid country code path.
IfinjectIndexToDestinationIfNeededforms an unexpected path that returnsundefined, subsequent steps might fail silently. Adding a fallback or validation can prevent errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/StatePicker/StatePicker.tsx(2 hunks)
🔇 Additional comments (3)
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/components/StatePicker/StatePicker.tsx (3)
6-6: Importing useMemo is appropriate and consistent with React best practices.
The added import for useMemo is correct, ensuring the memoization of computed values.
7-7: Confirm full test coverage for injectIndexToDestinationIfNeeded.
While importing and using injectIndexToDestinationIfNeeded is valid, it would be beneficial to confirm this function is robustly tested to handle various edge cases (e.g., invalid indices, missing paths).
30-30: Dependency array update looks good.
Adding props.inputIndex to the memo’s dependency array ensures recalculation when the index changes, aligning with React’s recommended usage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/useClearValueOnHide.ts (3)
14-18: Consider avoiding direct mutations of the payload object
The code directly mutatesref.current.payload. If your state management depends on immutability for proper updates or to avoid tricky bugs, consider creating a shallow or deep copy before modifying.- set(ref.current.payload, formattedDestination, filteredItems); - ref.current.setContext(ref.current.payload); + const newPayload = structuredClone(ref.current.payload); + set(newPayload, formattedDestination, filteredItems); + ref.current.setContext(newPayload);
24-49: Refactor repetitive removal logic
The removal logic is repeated under the condition forclearValueOnHide.byId(lines 24-49) and again forclearValueOnHide.valueDestination(lines 51-65). Consider extracting a helper function to avoid duplication, making the code more maintainable and less prone to errors.
42-45: Remove or replace console logging
Console logs can leak sensitive details (depending on the data inpayload) and clutter logs in production. Consider removing them or using a debug logging utility that can be enabled or disabled in different environments.apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/useClearValueOnHide.unit.test.ts (3)
7-9: Use partial mocking or explicit mocks
Currently, the entire modules forStateProvideranduseUIElementPropsare mocked. If only specific methods or hooks need mocking, partial mocks can preserve real behavior for other parts, reducing the risk of diverging from the real implementation.
46-66: Expand tests to cover toggling visibility
The test checks clearing the value by ID when hidden. Consider adding a test that toggleshiddenfromfalsetotrueand back, ensuring no erroneous re-clearing occurs when the element is re-hidden.
105-125: Increase coverage for edge cases
The current test for array index clearing is good. Additional edge cases—like an index out of range or an undefined array—could further ensure robust handling of the payload structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/useClearValueOnHide.ts(1 hunks)apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/useClearValueOnHide.unit.test.ts(1 hunks)apps/kyb-app/src/domains/collection-flow/types/ui-schema.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/kyb-app/src/domains/collection-flow/types/ui-schema.types.ts
🔇 Additional comments (2)
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/useClearValueOnHide.ts (1)
66-67: Check the dependencies in the useEffect
The dependency array references definition.valueDestination, even though the code checks definition.clearValueOnHide.valueDestination internally. Make sure this is intentional. If you want to specifically watch for changes to the nested property, consider referencing it directly.
apps/kyb-app/src/components/organisms/UIRenderer/elements/JSONForm/hooks/useClearValueOnHide/useClearValueOnHide.unit.test.ts (1)
32-44: Add assertions for unchanged payload values
When verifying that no clear operation occurs if clearValueOnHide is not defined, it might be beneficial to also assert that mockPayload remains unchanged to confirm the code doesn't mutate anything unexpectedly.
Summary by CodeRabbit
New Features
useClearValueOnHidethat automatically clears form field values when the associated UI element is hidden.Improvements
Types