fix(application-system): TableRepeater cancel option #16929
Conversation
Toti91
added
the
deprecated:automerge
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces new message definitions for localization in 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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16929 +/- ##
=======================================
Coverage 36.46% 36.46%
=======================================
Files 6903 6903
Lines 144583 144555 -28
Branches 41283 41285 +2
=======================================
- Hits 52718 52716 -2
+ Misses 91865 91839 -26 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 22 Total Test Services: 0 Failed, 21 Passed Test ServicesThis report shows up to 10 services
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
libs/application/ui-fields/src/lib/TableRepeaterFormField/utils.ts (3)
35-35: Consider enhancing error handling and function namingWhile the added truthiness check improves safety, consider:
- Adding comprehensive error handling for edge cases
- Renaming the function to better reflect its specific handling of nationalIdWithName
-export const handleCustomMappedValues = <T>( +export const handleNationalIdWithNameValues = <T>( tableItems: Array<Item>, values: Array<Value<T>>, ) => { + if (!tableItems?.length) { + return [] + } // Rest of the function... }
45-54: Consider adding input validation and improving clarityThe function is well-structured but could benefit from:
- Input validation for the items array
- A named constant for the flatten depth
- JSDoc documentation explaining the special handling of nationalIdWithName
+const FLATTEN_DEPTH = 2 + +/** + * Builds table headers with special handling for nationalIdWithName components + * @param items Array of TableRepeaterItem to generate headers from + * @returns Array of header labels + */ export const buildDefaultTableHeader = (items: Array<TableRepeaterItem>) => { + if (!items?.length) { + return [] + } return items .map((item) => item.component === 'nationalIdWithName' ? [coreMessages.name, coreMessages.nationalId] : item.label, ) - .flat(2) + .flat(FLATTEN_DEPTH) }
Line range hint
1-65: Add unit tests for the new utility functionsAs mentioned in the PR objectives, tests are missing. These utility functions are critical for table functionality and should be thoroughly tested.
Would you like me to help generate unit tests for these utility functions? I can create test cases covering:
- Normal cases for both header and row generation
- Special handling of nationalIdWithName components
- Edge cases and error handling
libs/application/ui-fields/src/lib/TableRepeaterFormField/TableRepeaterFormField.tsx (3)
97-101: Consider adding a confirmation dialog.While the cancel functionality is correctly implemented, consider adding a confirmation dialog to prevent accidental data loss when canceling an active insertion.
const handleCancelItem = (index: number) => { + if (window.confirm(formatText(confirmCancelMessage, application, formatMessage))) { setActiveIndex(-1) remove(index) + } }
252-269: Fix typo in justifyContent prop.The button layout looks good, but there's a typo in the
justifyContentprop value.-<Box display="flex" alignItems="center" justifyContent="flexEnd"> +<Box display="flex" alignItems="center" justifyContent="flex-end">
Line range hint
1-297: Add tests for the new cancel functionality.The PR objectives mention that tests haven't been added yet. Please add tests to verify:
- Cancel button visibility and functionality
- State management during cancel operation
- Integration with
nationalIdWithNamefeatureWould you like me to help create a test suite for these scenarios?
libs/application/types/src/lib/Fields.ts (1)
627-627: LGTM! Consider adding JSDoc comments.The addition of the optional
cancelButtonTextproperty is well-typed and properly integrated with the existing interface. The change aligns perfectly with the PR objectives to add a cancel option for active insertions.Consider adding JSDoc comments to document the purpose and usage of this property:
+ /** Text to display on the cancel button for active insertions */ cancelButtonText?: StaticText
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
libs/application/core/src/lib/messages.ts(2 hunks)libs/application/types/src/lib/Fields.ts(1 hunks)libs/application/ui-fields/src/lib/TableRepeaterFormField/TableRepeaterFormField.tsx(5 hunks)libs/application/ui-fields/src/lib/TableRepeaterFormField/utils.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/application/core/src/lib/messages.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/types/src/lib/Fields.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-fields/src/lib/TableRepeaterFormField/TableRepeaterFormField.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-fields/src/lib/TableRepeaterFormField/utils.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (6)
libs/application/ui-fields/src/lib/TableRepeaterFormField/utils.ts (1)
Line range hint 1-7: LGTM! Types and imports are well-structured
The type definitions and imports follow TypeScript best practices and support effective tree-shaking.
libs/application/ui-fields/src/lib/TableRepeaterFormField/TableRepeaterFormField.tsx (3)
25-29: LGTM! Well-structured imports.
The new utility imports follow best practices for modularity and tree-shaking support.
54-54: LGTM! Well-defined prop with default value.
The cancelButtonText prop is properly typed and defaults to a core message, maintaining backward compatibility.
79-80: Verify default table implementations for nationalIdWithName.
The fallback mechanism looks good, but let's verify the utility functions properly handle the nationalIdWithName case.
✅ Verification successful
Default table implementations correctly handle nationalIdWithName
The utility functions properly handle the nationalIdWithName case:
buildDefaultTableHeadersplits it into name and nationalId columns usingcoreMessagesbuildDefaultTableRowscorrectly maps the component to corresponding 'name' and 'nationalId' fields- Special case handling is properly documented in the code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of buildDefaultTableHeader and buildDefaultTableRows for nationalIdWithName
# Search for the utility function implementations
ast-grep --pattern 'export const buildDefaultTableHeader = $_' -f ts,tsx
ast-grep --pattern 'export const buildDefaultTableRows = $_' -f ts,tsx
# Search for nationalIdWithName usage
rg -A 5 'nationalIdWithName' -t ts -t tsx
Length of output: 573
Script:
#!/bin/bash
# Search for the utility function implementations
ast-grep --pattern 'export const buildDefaultTableHeader'
ast-grep --pattern 'export const buildDefaultTableRows'
# Search for nationalIdWithName usage with ripgrep
rg "nationalIdWithName"
# Find relevant TypeScript/TSX files
fd "\.(ts|tsx)$" libs/application/ui-fields/src/lib/TableRepeaterFormField/
Length of output: 3758
libs/application/core/src/lib/messages.ts (2)
44-48: LGTM! Cancel button message follows consistent pattern.
The buttonCancel message is well-structured and aligns with the PR objective of adding a cancel option.
144-153: LGTM! National ID and name messages support the nationalIdWithName feature.
The nationalId and name messages are well-structured and align with the PR objective of improving support for the nationalIdWithName feature.
* cancel active item functionality * setup default row and header support for nationalidWithname --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
TableRepeater cancel option
Attach a link to issue if relevant
What
Add an option to cancel active insertion in TableRepeater
Also add better support for
nationalIdWithNamein repeater, by providing default table header and row support for name and nationalIdWhy
Better UX and DX
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
Release Notes
New Features
buttonCancel,nationalId, andname.cancelButtonTextproperty to enhance theTableRepeaterFieldinterface.Enhancements
These updates aim to improve usability and localization within the application.