feat(ui): add DestinationFormItemV2 — AntD-free alert destination form - #29539
Conversation
New component family under DestinationFormItemV2/ that replicates all DestinationFormItem behaviour (webhook, email, teams/users, owners/followers/admins, notify-downstream, connection/read timeout, destination status testing) using @openmetadata/ui-core-components + react-hook-form, with no Ant Design dependencies. Lives alongside the existing component until parent pages migrate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
35 tests across 3 suites covering DestinationFormItemV2, DestinationSelectItemV2, and TeamAndUserSelectItemV2. Uses react-hook-form FormProvider wrapper (no Ant Design form context) with lightweight ui-core-components mocks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| <Input | ||
| data-testid="connection-timeout-input" | ||
| defaultValue="10" | ||
| inputDataTestId="connection-timeout-input-field" | ||
| placeholder={`${t('label.connection-timeout')} (${t( | ||
| 'label.second-plural' | ||
| )})`} | ||
| ref={field.ref} | ||
| type="number" | ||
| value={field.value !== undefined ? String(field.value) : ''} | ||
| onBlur={field.onBlur} | ||
| onChange={(val) => field.onChange(val)} | ||
| /> |
There was a problem hiding this comment.
⚠️ Bug: defaultValue ignored on controlled inputs — defaults never applied
The connection-timeout (defaultValue="10"), read-timeout (defaultValue={String(DEFAULT_READ_TIMEOUT)}) and downstream-depth (defaultValue="1") inputs are rendered as controlled components — they receive value={field.value !== undefined ? String(field.value) : ''}. In React, when both value and defaultValue are supplied, value wins and defaultValue is ignored. Because field.value starts as undefined, the inputs render empty and, more importantly, the form state is never initialized with the intended defaults (10s connection timeout, DEFAULT_READ_TIMEOUT, depth 1). Unless the user manually types a value, these fields submit as undefined. The original AntD form set these via form initial values, so this is a behavior regression.
Suggested fix: initialize the defaults in react-hook-form (e.g. via defaultValues / useEffect setValue when undefined) instead of relying on the defaultValue prop, or fall back to the default inside the value expression (e.g. value={field.value ?? '10'}) and push that default into form state.
Was this helpful? React with 👍 / 👎
| rules={{ | ||
| required: t('message.field-text-is-required', { | ||
| fieldText: t('label.downstream-depth'), | ||
| }), | ||
| validate: (value) => { | ||
| const numVal = Number(value); | ||
|
|
||
| return !isEmpty(String(value)) && numVal <= 0 | ||
| ? t('message.value-must-be-greater-than', { | ||
| field: t('label.downstream-depth'), | ||
| minimum: 0, | ||
| }) | ||
| : true; | ||
| }, | ||
| }} |
There was a problem hiding this comment.
💡 Edge Case: downstreamDepth validation accepts non-numeric / non-integer input
The validate rule computes numVal = Number(value) and only fails when !isEmpty(String(value)) && numVal <= 0. For a non-numeric entry like "abc", Number("abc") is NaN, and NaN <= 0 is false, so the rule returns true (valid). Decimal values such as 1.5 are also accepted. The original implementation required an integer greater than 0, so the V2 component silently weakens validation and can let invalid depth values through to the API.
Suggested fix: explicitly reject NaN and non-integers, e.g. if (isEmpty(String(value))) return true; const n = Number(value); return Number.isInteger(n) && n > 0 ? true : t(...).
Was this helpful? React with 👍 / 👎
| const debouncedSearch = useMemo( | ||
| () => debounce(handleSearch, 500), | ||
| [handleSearch] | ||
| ); |
There was a problem hiding this comment.
💡 Bug: Debounced search not cancelled on unmount
debouncedSearch (lodash debounce, 500ms) is invoked from a useEffect on searchText/entityType changes, but it is never cancelled. If the component unmounts (e.g. the dropdown destination is removed) while a debounced call is pending, the trailing invocation runs handleSearch, which calls setIsLoadingOptions/setOptions after unmount, producing a React state-update-after-unmount warning and a wasted network request.
Suggested fix: return a cleanup that cancels the debounce, e.g. useEffect(() => { debouncedSearch(searchText); return () => debouncedSearch.cancel(); }, [searchText, entityType, debouncedSearch]);.
Was this helpful? React with 👍 / 👎
| destination.category === 'External' && | ||
| externalDestinationTypes.includes(destination.type ?? '') |
There was a problem hiding this comment.
💡 Quality: Hardcoded 'External' literal instead of SubscriptionCategory.External
isExternalDestinationSelected and the test-destination filter compare against the string literal 'External' while the SubscriptionCategory enum is already imported and used elsewhere in the same component family. SubscriptionCategory.External resolves to "External", so the code works today, but the literal is fragile (a future enum value change would silently break the comparison) and inconsistent with the rest of the file.
Suggested fix: replace destination.category === 'External' and d.category === 'External' with === SubscriptionCategory.External.
Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const debouncedSearch = useMemo( | ||
| () => debounce(handleSearch, 500), | ||
| [handleSearch] | ||
| ); |
There was a problem hiding this comment.
debouncedSearch is created via useMemo but never cancelled when the component unmounts. A pending 500 ms search will still fire after unmount — calling setIsLoadingOptions and setOptions on a stale closure. Additionally, when handleSearch changes (because the onSearch prop is replaced), useMemo silently creates a new debounce instance while the old one's pending timer continues running. Add a useEffect that calls debouncedSearch.cancel() in its cleanup to prevent stale state updates.
| data-testid={`token-url-input-${fieldName}`} | ||
| inputDataTestId={`token-url-input-field-${fieldName}`} | ||
| label={`${t('label.token-url')}:`} | ||
| placeholder="https://auth.example.com/oauth/token" |
There was a problem hiding this comment.
The token URL placeholder is a raw string literal rather than a translation key. Per the project's CLAUDE.md requirement, all user-visible text must go through useTranslation. This placeholder should map to a locale key (e.g., placeholder.oauth2-token-url or similar).
| placeholder="https://auth.example.com/oauth/token" | |
| placeholder={t('placeholder.oauth2-token-url')} |
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| <Input | ||
| data-testid="connection-timeout-input" | ||
| defaultValue="10" | ||
| inputDataTestId="connection-timeout-input-field" | ||
| placeholder={`${t('label.connection-timeout')} (${t( | ||
| 'label.second-plural' | ||
| )})`} | ||
| ref={field.ref} | ||
| type="number" | ||
| value={field.value !== undefined ? String(field.value) : ''} | ||
| onBlur={field.onBlur} | ||
| onChange={(val) => field.onChange(val)} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
defaultValue is silently ignored on a controlled Input
Both defaultValue and value are passed to the timeout Input components. React ignores defaultValue when value is present (controlled mode), so the field will render empty whenever the parent form is not initialized with an explicit timeout value. The same issue applies to the readTimeout field (with DEFAULT_READ_TIMEOUT). Either initialize the form with { timeout: '10', readTimeout: String(DEFAULT_READ_TIMEOUT) } in the parent's defaultValues, or drop defaultValue here to avoid the misleading prop that has no runtime effect.
| const statusAlertVariant = | ||
| destinationStatusDetails?.status === 'Success' ? 'success' : 'error'; |
There was a problem hiding this comment.
Status enum
destinationStatusDetails?.status === 'Success' compares against a raw string literal. The generated Status enum is already imported by the test file, and Status.Success = "Success". Using the enum makes this resilient to future value changes in the generated schema.
| const statusAlertVariant = | |
| destinationStatusDetails?.status === 'Success' ? 'success' : 'error'; | |
| const statusAlertVariant = | |
| destinationStatusDetails?.status === Status.Success ? 'success' : 'error'; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const addEmail = useCallback(() => { | ||
| const trimmed = inputValue.trim(); | ||
| if (!trimmed || receivers.includes(trimmed)) { | ||
| setInputValue(''); | ||
|
|
||
| return; | ||
| } | ||
| setValue(`destinations.${fieldName}.config.receivers`, [ | ||
| ...receivers, | ||
| trimmed, | ||
| ]); | ||
| setInputValue(''); | ||
| }, [inputValue, receivers, fieldName, setValue]); |
There was a problem hiding this comment.
addEmail prevents exact duplicates but allows any arbitrary string (e.g., "not-an-email") to be added to the receivers list. A simple regex or the browser's type="email" constraint should guard against malformed addresses before they are stored in form state.
Jest test CoverageUI tests summary
|
|
🟡 Playwright Results — all passed (15 flaky)✅ 4460 passed · ❌ 0 failed · 🟡 15 flaky · ⏭️ 38 skipped
🟡 15 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
UI Screenshots — DestinationFormItemV2Captured against the Add Notification page with the component temporarily wired in. State 1: Initial (empty) stateComponent renders with timeout inputs and disabled Add/Test Destination buttons when no resource is selected, enabled when resource is set. State 2: Destination row added — category dropdown openThe State 3: Slack selected (external destination)
State 4: Owners selected (internal destination)
|
2 participants
Describe your changes:
I worked on creating
DestinationFormItemV2, a fully AntD-free replacement for the existingDestinationFormItemcomponent family, because the alerts destination form still depended on Ant Design while the rest of the UI is migrating to@openmetadata/ui-core-components.This PR adds 7 new files —
DestinationFormItemV2,DestinationSelectItemV2,DestinationConfigField, andTeamAndUserSelectItemV2— built onreact-hook-form+ react-aria-components from@openmetadata/ui-core-components, plus 3 test files covering 35 unit tests across all new components.Type of change:
High-level design:
The new component family mirrors the structure of the existing
DestinationFormItemfamily but replaces all Ant Design primitives (Select,Input,Form,Button,Switch, etc.) with their@openmetadata/ui-core-componentsequivalents.Form state is managed entirely via
react-hook-form(useFieldArray,useWatch,Controller,useFormContext) — no local state for field values.DestinationConfigFieldhandles the per-destination-type config fields (Slack endpoint, Email receivers, Teams webhook, etc.) usingController-wrappedInput/PasswordInput/Selectcomponents. Validation errors are displayed via standalone<p>elements rather than theerrorMessageprop (which react-aria explicitly omits fromTextFieldProps/SelectProps). The existingDestinationFormItemis untouched;DestinationFormItemV2is a parallel addition ready to swap in.Tests:
Use cases covered
Unit tests
DestinationFormItemV2/DestinationFormItemV2.test.tsx(13 tests)DestinationFormItemV2/DestinationSelectItemV2/DestinationSelectItemV2.test.tsx(14 tests)DestinationFormItemV2/TeamAndUserSelectItemV2/TeamAndUserSelectItemV2.test.tsx(8 tests)Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
Not performed — component is not yet wired into any page. Unit tests cover the component behaviour end-to-end.
UI screen recording / screenshots:
Note: It's not actaully used just capture screenshot from temporary usage
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Greptile Summary
This PR adds
DestinationFormItemV2, a parallel AntD-free family of alert-destination form components built onreact-hook-formand@openmetadata/ui-core-components, with 35 unit tests covering the full interaction surface.DestinationFormItemV2manages auseFieldArrayof destination rows, drives the "test destinations" API call with filtered external config, and delegates each row toDestinationSelectItemV2.DestinationSelectItemV2handles category/type selection, inline config viaDestinationConfigField, and the notify-downstream toggle;TeamAndUserSelectItemV2is a hand-rolled async combobox with debounced search and badge selection for Teams/Users destinations.DestinationFormItemis untouched; this component is a parallel addition not yet wired into any page.Confidence Score: 4/5
Safe to merge as a non-wired parallel addition; the debounce cleanup gap in TeamAndUserSelectItemV2 should be fixed before the component is connected to a page.
The component family is not yet wired into any page, so the identified issues carry no immediate user-facing risk. The missing debounce cancel on unmount is a real defect that will cause stale state updates once the component is rendered in production flows — it should be addressed before the wire-up PR. The other findings (dead
defaultValueprop, missing email validation, hardcoded placeholder string, raw string comparison for status) are quality concerns that don't block correctness today but will need cleanup before this replaces the legacy form.TeamAndUserSelectItemV2.tsx (missing debounce cleanup) and DestinationConfigField.tsx (email validation, i18n placeholder) deserve a second look before the component is connected to a page.
Important Files Changed
defaultValueon the controlled timeout inputs is ignored at runtime.'Success'instead of the generatedStatusenum.EmailTagInputadds receivers without validating email format.debouncedSearch.cancel()cleanup on unmount can cause state updates after the component is gone.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[DestinationFormItemV2] -->|useFieldArray 'destinations'| B[DestinationSelectItemV2 × N] A -->|testAlertDestination API| C[Test Destinations Button] A -->|useWatch 'resources'| D{Source selected?} D -->|No| E[Add/Test buttons disabled] D -->|Yes| F[Add/Test buttons enabled] B -->|Select.ComboBox| G{Destination Type} G -->|External: Slack / MSTeams / GChat / Webhook| H[DestinationConfigField — endpoint + auth accordion] G -->|External: Email| I[DestinationConfigField — EmailTagInput] G -->|Internal: Teams / Users| J[DestinationConfigField → TeamAndUserSelectItemV2] G -->|Internal: Owners / Followers / Admins| K[DestinationConfigField — config flag only] G -->|Internal: any| L[Subscription Type Select + Warning Alert] J -->|debounced onSearch| M[(Search API)] M --> N[Checkbox dropdown with BadgeWithButton tags] B -->|notifyDownstream Toggle| O{notifyDownstream?} O -->|true| P[downstreamDepth Input] O -->|false| Q[hidden] C -->|getFormattedDestinations + filter empty config| R[testAlertDestination] R -->|results| S[destinationsWithStatus → Alert per row]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[DestinationFormItemV2] -->|useFieldArray 'destinations'| B[DestinationSelectItemV2 × N] A -->|testAlertDestination API| C[Test Destinations Button] A -->|useWatch 'resources'| D{Source selected?} D -->|No| E[Add/Test buttons disabled] D -->|Yes| F[Add/Test buttons enabled] B -->|Select.ComboBox| G{Destination Type} G -->|External: Slack / MSTeams / GChat / Webhook| H[DestinationConfigField — endpoint + auth accordion] G -->|External: Email| I[DestinationConfigField — EmailTagInput] G -->|Internal: Teams / Users| J[DestinationConfigField → TeamAndUserSelectItemV2] G -->|Internal: Owners / Followers / Admins| K[DestinationConfigField — config flag only] G -->|Internal: any| L[Subscription Type Select + Warning Alert] J -->|debounced onSearch| M[(Search API)] M --> N[Checkbox dropdown with BadgeWithButton tags] B -->|notifyDownstream Toggle| O{notifyDownstream?} O -->|true| P[downstreamDepth Input] O -->|false| Q[hidden] C -->|getFormattedDestinations + filter empty config| R[testAlertDestination] R -->|results| S[destinationsWithStatus → Alert per row]Reviews (1): Last reviewed commit: "test(ui): add unit tests for Destination..." | Re-trigger Greptile
Context used: