feat(contact-point): implements edit ui#1413
Conversation
WalkthroughA new contact point editing feature was implemented, introducing a container component for managing API interactions and form state. The contact point form was refactored to simplify email handling and validation. The edit page was updated to use these components, enabling full edit functionality and navigation upon completion or cancellation. A test suite was added to verify the container's API interactions and notification behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditPage
participant EditContainer
participant API
participant Form
User->>EditPage: Navigates to edit page
EditPage->>EditContainer: Mounts with contact point ID
EditContainer->>API: Patch contact point on edit
API-->>EditContainer: Returns success or error
EditContainer->>Form: Passes onEdit handler and loading state
User->>Form: Edits and submits form
Form->>EditContainer: Calls edit handler with new values
EditContainer->>EditPage: Calls onEditSuccess callback on success
EditPage->>User: Navigates back or shows notification
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1413 +/- ##
==========================================
+ Coverage 37.07% 37.09% +0.02%
==========================================
Files 831 832 +1
Lines 19799 19827 +28
Branches 3633 3646 +13
==========================================
+ Hits 7340 7355 +15
+ Misses 12128 11750 -378
- Partials 331 722 +391
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx(1 hunks)apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.tsx(4 hunks)apps/deploy-web/src/components/alerts/EditContactPointPage.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/deploy-web/src/components/alerts/EditContactPointPage.tsx (4)
apps/deploy-web/src/components/alerts/AlertsLayout.tsx (1)
AlertsLayout(27-66)apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx (1)
ContactPointEditContainer(24-76)apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.tsx (1)
ContactPointForm(48-172)apps/deploy-web/src/utils/urlUtils.ts (1)
UrlService(16-75)
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx (3)
packages/http-sdk/src/generated/NotificationSDK.ts (1)
ContactPointPatchInput(277-284)apps/deploy-web/src/context/ServicesProvider/ServicesProvider.tsx (1)
useServices(32-34)apps/deploy-web/src/hooks/useNotificator.tsx (1)
useNotificator(5-19)
🪛 GitHub Actions: Deploy Web CI
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx
[error] 27-27: Type error: No overload matches this call. Argument of type '{ path: { id: string | string[]; }; }' is not assignable to parameter of type 'void'. Type 'string | string[]' is not assignable to type 'string'.
🔇 Additional comments (4)
apps/deploy-web/src/components/alerts/EditContactPointPage.tsx (1)
2-27: LGTM! Clean implementation of the edit contact point feature.The implementation effectively uses the container pattern to separate data management from UI concerns. The navigation logic is appropriate - using
router.back()for successful edits and redirecting to the contact points list for cancellation.apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx (1)
39-76: Excellent container pattern implementation.The container effectively manages:
- Data fetching and mutation logic
- Success/error notification handling
- Proper data transformation (emails array ↔ comma-separated string)
- Clean render props pattern for passing data to child components
The use of
useWhenhooks for side effects and proper dependency arrays inuseCallbackdemonstrates good React patterns.apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.tsx (2)
17-37: Well-designed email validation for comma-separated input.The validation logic properly:
- Splits comma-separated emails and trims whitespace
- Filters out empty strings
- Validates each email individually using Zod's email validator
- Provides clear error messages
This is a clean approach for handling multiple emails in a single input field.
70-89: Good form management and data transformation logic.The implementation effectively:
- Prevents unnecessary form resets by checking if the form is empty before applying new values
- Transforms the comma-separated email string back to an array format expected by the container
- Uses appropriate revalidation mode for edit scenarios
The data flow integration with the container component is seamless.
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx (1)
68-80: Consider adding query state handling.The render prop implementation is solid with proper fallbacks. However, consider exposing query loading/error states to provide better UX while fetching initial data.
return ( <> {children({ values: { name: query.data?.data?.name ?? "", emails: query.data?.data?.config.addresses ?? [] }, onEdit: edit, - isLoading: mutation.isPending + isLoading: mutation.isPending, + isQueryLoading: query.isLoading, + queryError: query.error })} </> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.spec.tsx(1 hunks)apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx(1 hunks)apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.tsx(3 hunks)apps/deploy-web/src/components/alerts/EditContactPointPage.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/deploy-web/src/components/alerts/EditContactPointPage.tsx
- apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx (3)
packages/http-sdk/src/generated/NotificationSDK.ts (1)
ContactPointPatchInput(277-284)apps/deploy-web/src/context/ServicesProvider/ServicesProvider.tsx (1)
useServices(32-34)apps/deploy-web/src/hooks/useNotificator.tsx (1)
useNotificator(5-19)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-deploy-web-build
🔇 Additional comments (8)
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.spec.tsx (3)
1-18: LGTM: Well-structured test setup.The test imports and setup are comprehensive, properly using testing utilities, faker for data generation, and necessary context providers.
20-49: LGTM: Comprehensive success test case.The test properly verifies the API call structure and success notification behavior. Good use of
waitForfor async testing.
84-120: LGTM: Well-designed test setup function.The setup function properly configures mocks, test data, and context providers. Good use of the container testing utility to capture child props.
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx (5)
12-27: LGTM: Well-structured type definitions.The type definitions properly leverage TypeScript's utility types and create a clean contract for the component. Good that
idis now a prop rather than extracted from URL params, addressing previous review concerns.
29-41: LGTM: Proper component setup.The component correctly uses hooks and context providers. The API calls are properly configured with the
idprop, which resolves the previous type concerns withuseParams().
43-55: LGTM: Well-implemented edit function.The edit callback properly transforms the input data for the API call and uses
useCallbackfor performance optimization.
57-64: LGTM: Proper success handling.The success notification and callback handling are correctly implemented with appropriate dependencies and test IDs.
66-66: LGTM: Basic error handling implemented.The error notification is properly triggered on mutation failure with the correct test ID.
...eploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.spec.tsx
Show resolved
Hide resolved
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.tsx
Outdated
Show resolved
Hide resolved
|
a couple of non blocking improvement suggestions:
|
apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.tsx
Outdated
Show resolved
Hide resolved
ygrishajev
left a comment
ygrishajev
left a comment
There was a problem hiding this comment.
Ok, so just to keep it visible. The page not has contact point resolution via getServerSideProps. Meaning that:
- container should not query a contact point if it's already on the props
- the form provided values should be renamed to
initialValuesand treated as such - only set once viadefatultValues; all the logic trying to fix form states should be now removed possibly as it should just work as is normally
...eploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.spec.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.tsx (2)
78-78: Update error message to be context-appropriate.The error message mentions "create" but this form is used for both creating and editing contact points.
- setError("Failed to create contact point. Please try again."); + setError("Failed to save contact point. Please try again.");
101-101: Make header dynamic based on context.The header currently says "New Email Contact Point" but this form is used for both creating and editing. Consider making it dynamic based on whether
initialValuesare provided.- <h2 className="text-lg font-semibold">New Email Contact Point</h2> + <h2 className="text-lg font-semibold"> + {initialValues?.name ? "Edit Email Contact Point" : "New Email Contact Point"} + </h2>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.spec.tsx(1 hunks)apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx(1 hunks)apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.spec.tsx(2 hunks)apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.tsx(3 hunks)apps/deploy-web/src/components/alerts/EditContactPointPage.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.spec.tsx
- apps/deploy-web/src/components/alerts/EditContactPointPage.tsx
- apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.spec.tsx
- apps/deploy-web/src/components/alerts/ContactPointEditContainer/ContactPointEditContainer.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-deploy-web-build
🔇 Additional comments (7)
apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.tsx (7)
14-17: LGTM: Email validation schema simplified appropriately.The email validation has been moved from schema preprocessing to runtime validation in the submit handler, which provides better error handling and user feedback.
19-19: Good type separation for form vs. data values.The
DataValuestype properly distinguishes between the form's internal string representation and the external array format expected by the API.
22-36: Props interface updated correctly for edit functionality.The interface properly supports both create and edit scenarios with optional
initialValuesand appropriate default values.
40-45: Initial values transformation implemented correctly.The memoized transformation from array to comma-separated string handles the form's internal representation properly.
55-82: Email parsing and validation logic is robust.The submit handler properly addresses the previous review feedback by filtering empty strings and handles validation, deduplication, and error reporting well. The use of
Array.from(new Set(...))for deduplication is a good practice.
86-89: Change detection logic implemented correctly.The
hasChangeslogic properly compares current form values with initial values to enable/disable submission appropriately.
145-145: Submit button correctly disabled when no changes.The button properly implements the requirement from the PR feedback to disable submission when no changes have been made, improving UX by preventing unnecessary API calls.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/deploy-web/src/components/alerts/CreateContactPointPage.tsx (1)
25-27: Consider navigation consistency.The navigation callbacks use different approaches:
onCreateusesrouter.back()(browser back)onCancelusesrouter.push(UrlService.contactPoints())(specific route)This inconsistency might confuse users about where they'll end up after different actions.
For consistency, consider using the same navigation approach for both callbacks:
- <ContactPointCreateContainer onCreate={router.back}> + <ContactPointCreateContainer onCreate={() => router.push(UrlService.contactPoints())}>This ensures users always return to the contact points list regardless of the action taken.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/deploy-web/src/components/alerts/AlertsLayout.tsx(1 hunks)apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.tsx(3 hunks)apps/deploy-web/src/components/alerts/CreateContactPointPage.tsx(1 hunks)apps/deploy-web/src/components/alerts/EditContactPointPage.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/deploy-web/src/components/alerts/AlertsLayout.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/deploy-web/src/components/alerts/EditContactPointPage.tsx
- apps/deploy-web/src/components/alerts/ContactPointForm/ContactPointForm.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-deploy-web-build
🔇 Additional comments (2)
apps/deploy-web/src/components/alerts/CreateContactPointPage.tsx (2)
2-3: LGTM! Imports look correct.The new imports for navigation and UI components align well with the refactored header structure.
Also applies to: 10-10
17-28: Well-structured refactor with clean separation.The refactor successfully replaces the previous AlertsLayout/AlertTabs structure with a cleaner, more focused layout. The render props pattern with ContactPointCreateContainer provides good separation of concerns between UI and business logic.
3 participants
Closes #1396
TODO:
Screenshot:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style