Skip to content

Improve connector setup UI flow#28569

Open
harshach wants to merge 39 commits into
mainfrom
harshach/connection-form-design
Open

Improve connector setup UI flow#28569
harshach wants to merge 39 commits into
mainfrom
harshach/connection-form-design

Conversation

@harshach
Copy link
Copy Markdown
Collaborator

@harshach harshach commented Jun 1, 2026

Describe your changes:

Fixes: N/A - issue number was not provided in the workspace instructions.

This PR redesigns the add-service connector setup flow, including connector selection cards, service naming validation, connection configuration layout, focused documentation panels, test connection status, and the what-to-ingest filters step. It also makes the schema-driven form rendering more generic across connectors so auth tabs, nested credential groups, sample-data storage config, IAM toggles, required-field gating, and existing regex filters render in the intended human-readable UI.

Type of change:

  • Improvement

High-level design:

The implementation keeps the flow schema-driven and adds shared UI primitives for service stepper/header treatment, service-name cards, JSON-schema auth selection, connection object layout, docs rendering, test connection, and filter pattern editing. Existing connector schemas continue to drive fields while UI helpers normalize titles, credentials, oneOf/auth modes, nested storage config, required-field validation, docs links, and saved filter regex values. The rollout is UI-only except for the small Snowflake/test-connection schema updates needed to expose the expected configuration and test metadata.

Tests:

Use cases covered

  • Connector cards render compactly with correct logos, sizing, and selection behavior.
  • Connection config forms render for previously broken connectors and keep required-field/test-connection gating correct.
  • Auth mode switches clear inactive credentials and generate backend-safe test connection payloads.
  • Scope & Options / What to Ingest converts existing include/exclude regex to readable filter rules and preserves the original regex on save.
  • Nested credential and sample-data storage config sections stay aligned without overlapping labels or inputs.

Unit tests

  • I added unit tests for the new/changed logic.
  • Files added/updated: Add service pages, service-name validation, connection config forms, embedded config forms, filters config, JSON schema auth/oneOf fields, test connection components, service connection utilities, docs links, and shared form builders.
  • Coverage on targeted changed UI files: 99.81% statements, 99.80% lines, 100% functions.

Backend integration tests

  • Not applicable (no backend API changes).

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • I added Playwright E2E tests under openmetadata-ui/.../ui/playwright/ for UI changes.
  • Files added/updated: playwright/e2e/Flow/ConnectionConfigLayout.spec.ts and ingestion service page helpers.

Manual testing performed

  1. Started and used the local UI dev server at http://127.0.0.1:3000.
  2. Verified connector selection, connection config, test connection, and what-to-ingest UI flows while iterating on the design feedback.

UI screen recording / screenshots:

Not attached from the CLI workflow.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
  • For UI changes: I attached a screen recording and/or screenshots above.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.
  • I have added tests around the new logic.

Summary by Gitar

  • Core Redesign:
    • Refactored AddServicePage and ConnectionConfigForm to implement a new schema-driven UI flow with shared primitives like ConnectionObjectFieldTemplate and AuthSelectField.
  • Component Infrastructure:
    • Introduced FiltersConfigForm.tsx for structured regex rule management and ServiceDocPanel with focusedMode for context-aware documentation.
    • Added CoreObjectFieldTemplate to support dynamic, multi-section form layouts and gated credential rendering.
  • UI Enhancements:
    • Implemented TestConnectionModal revamp with improved status banners, capability step partitioning, and integrated raw log viewing.
  • Testing & Validation:
    • Added ConnectionConfigLayout.spec.ts for E2E validation and ConnectionConfigForm.schema-render.test.tsx for unit-level schema form rendering coverage.
  • Backend & Schema:
    • Implemented normalizePatchNodes in UserService to handle defaultPersona cleanup.
    • Updated snowflakeConnection.json and testConnectionDefinition.json to support extended configuration and test metadata.

This will update automatically on new commits.

@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Jun 1, 2026
@harshach harshach marked this pull request as ready for review June 1, 2026 06:11
@harshach harshach requested a review from a team as a code owner June 1, 2026 06:11
Copilot AI review requested due to automatic review settings June 1, 2026 06:11
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

✅ TypeScript Types Auto-Updated

The generated TypeScript types have been automatically updated based on JSON schema changes in this PR.

Copilot AI review requested due to automatic review settings June 3, 2026 14:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

Copilot AI review requested due to automatic review settings June 3, 2026 14:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

🔴 Playwright Results — 14 failure(s), 18 flaky

✅ 4244 passed · ❌ 14 failed · 🟡 18 flaky · ⏭️ 93 skipped

Shard Passed Failed Flaky Skipped
✅ Shard 1 299 0 0 4
🔴 Shard 2 797 7 5 9
🟡 Shard 3 799 0 4 8
🔴 Shard 4 850 2 3 12
✅ Shard 5 721 0 0 47
🔴 Shard 6 778 5 6 13

Genuine Failures (failed on all attempts)

Features/DataQuality/ProfilerIngestionForm.spec.ts › STATIC config sends profileSample and no DYNAMIC-only keys (shard 2)
�[31mTest timeout of 60000ms exceeded while running "beforeEach" hook.�[39m
Features/DataQuality/ProfilerIngestionForm.spec.ts › STATIC config sends all three static-only fields when set (shard 2)
�[31mTest timeout of 60000ms exceeded while running "beforeEach" hook.�[39m
Features/DataQuality/ProfilerIngestionForm.spec.ts › DYNAMIC with smart sampling ON sends only DYNAMIC keys (shard 2)
�[31mTest timeout of 60000ms exceeded while running "beforeEach" hook.�[39m
Features/DataQuality/ProfilerIngestionForm.spec.ts › DYNAMIC with smart sampling OFF and thresholds sends thresholds array (shard 2)
�[31mTest timeout of 60000ms exceeded while running "beforeEach" hook.�[39m
Features/DataQuality/ProfilerIngestionForm.spec.ts › DYNAMIC threshold remove drops only the selected row (shard 2)
�[31mTest timeout of 60000ms exceeded while running "beforeEach" hook.�[39m
Features/DataQuality/ProfilerIngestionForm.spec.ts › switching DYNAMIC → STATIC does not leak smartSampling or thresholds (shard 2)
�[31mTest timeout of 60000ms exceeded while running "beforeEach" hook.�[39m
Features/DataQuality/ProfilerIngestionForm.spec.ts › switching STATIC → DYNAMIC does not leak static-only fields (shard 2)
�[31mTest timeout of 60000ms exceeded while running "beforeEach" hook.�[39m
Flow/ServiceForm.spec.ts › Verify SSL cert upload with long filename and UI overflow handling (shard 4)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoHaveValue�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed

Locator: getByRole('textbox', { name: 'CA Certificate' })
Expected: �[32m"-----BEGIN ENCRYPTED PRIVATE KEY-----OEMIIFJDBWBEIEOELRKEIREOROJKELOQMDKDKDOPWWMKSLLSMDHMFRQXBHQTREFrlpPELhGbNAICCAAwDAYIKoZIhvcNAgkFADAUBggqhkiG9w0DBWTEYEWEIWOWEIUEUWIWIEUEEHEIEOEKELEPOWKWPOQPEKEOEKEPWKWOWPKEOEOEKOEKEOKWKFPLKPFLKPLKPLKPLKPKPLKPLKPLKP.crt"�[39m
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toHaveValue" with timeout 15000ms�[22m
�[2m  - waiting for getByRole('textbox', { name: 'CA Certificate' })�[22m
�[2m    2 × locator resolved to <input title="" data-rac="" tabindex="0" placeholder="" type="password" aria-label="CA Certificate" id="root/connection/sslConfig/caCertificate" aria-describedby="react-aria8568052216-:r2j:" aria-labelledby="root/connection/sslConfig/caCertificate react-aria8568052216-:r2h:" class="tw:m-0 tw:w-full tw:bg-transparent tw:text-primary tw:r
Flow/ServiceForm.spec.ts › Verify service name field validation errors (shard 4)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoContainText�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed

Locator: getByTestId('service-name-card').getByTestId('field-hint')
Expected substring: �[32m"A database service named PlaywrightService_95fa51d3 already exists. "�[39m
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toContainText" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('service-name-card').getByTestId('field-hint')�[22m

Features/AutoPilot.spec.ts › Create Service and check the AutoPilot status (shard 6)
�[31mTest timeout of 480000ms exceeded.�[39m
Features/AutoPilot.spec.ts › Create Service and check the AutoPilot status (shard 6)
Error: locator.fill: Error: Element is not an <input>, <textarea>, <select> or [contenteditable] and does not have a role allowing [aria-readonly]
Call log:
�[2m  - waiting for getByTestId('filter-section-dashboardFilterPattern').getByTestId('include-filter-input')�[22m
�[2m    - locator resolved to <div data-rac="" data-input-wrapper="true" data-testid="include-filter-input" class="tw:group tw:flex tw:h-max tw:w-full tw:flex-col tw:items-start tw:justify-start tw:gap-1.5 tw:flex-1 tw:min-w-0">…</div>�[22m
�[2m    - fill("jaffle_shop dashboard")�[22m
�[2m  - attempting fill action�[22m
�[2m    - waiting for element to be visible, enabled and editable�[22m

Features/AutoPilot.spec.ts › Create Service and check the AutoPilot status (shard 6)
Error: locator.fill: Error: Element is not an <input>, <textarea>, <select> or [contenteditable] and does not have a role allowing [aria-readonly]
Call log:
�[2m  - waiting for getByTestId('filter-section-tableFilterPattern').getByTestId('include-filter-input')�[22m
�[2m    - locator resolved to <div data-rac="" data-input-wrapper="true" data-testid="include-filter-input" class="tw:group tw:flex tw:h-max tw:w-full tw:flex-col tw:items-start tw:justify-start tw:gap-1.5 tw:flex-1 tw:min-w-0">…</div>�[22m
�[2m    - fill("bot_entity")�[22m
�[2m  - attempting fill action�[22m
�[2m    - waiting for element to be visible, enabled and editable�[22m

Features/AutoPilot.spec.ts › Create Service and check the AutoPilot status (shard 6)
�[31mTest timeout of 480000ms exceeded.�[39m
Features/AutoPilot.spec.ts › Create Service and check the AutoPilot status (shard 6)
�[31mTest timeout of 480000ms exceeded.�[39m
🟡 18 flaky test(s) (passed on retry)
  • Features/AdvancedSearchSuggestions.spec.ts › Verify suggestions for Tier field (shard 2, 1 retry)
  • Features/DataQuality/TestCaseImportExportE2eFlow.spec.ts › Admin: Complete export-import-validate flow (shard 2, 1 retry)
  • Features/DataQuality/TestCaseImportExportE2eFlow.spec.ts › EditAll User: Complete export-import-validate flow (shard 2, 1 retry)
  • Features/DataQuality/TestCaseResultPermissions.spec.ts › User with only VIEW cannot PATCH results (shard 2, 1 retry)
  • Features/Glossary/GlossaryWorkflow.spec.ts › should display correct status badge color and icon (shard 2, 1 retry)
  • Features/KnowledgeCenterList.spec.ts › Knowledge Center List - Test upvote and downvote buttons (shard 3, 1 retry)
  • Features/RTL.spec.ts › Verify Following widget functionality (shard 3, 1 retry)
  • Features/TestSuiteMultiPipeline.spec.ts › Edit the pipeline's test case (shard 3, 1 retry)
  • Flow/ExploreAggregationCountsMatching.spec.ts › should verify left panel counts and tab search results for normal search (shard 3, 1 retry)
  • Flow/PersonaFlow.spec.ts › Description Contains filter – table with matching description appears in widget (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Topic (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for File (shard 4, 1 retry)
  • Pages/Glossary.spec.ts › Column dropdown drag-and-drop functionality for Glossary Terms table (shard 6, 1 retry)
  • Pages/Lineage/DataAssetLineage.spec.ts › verify create lineage for entity - Search Index (shard 6, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 6, 1 retry)
  • Pages/Lineage/LineageInteraction.spec.ts › Verify edge delete button in drawer (shard 6, 1 retry)
  • Pages/ODCSImportExport.spec.ts › Multi-object ODCS contract - object selector shows all schema objects (shard 6, 1 retry)
  • Pages/UserDetails.spec.ts › Admin user can get all the roles hierarchy and edit roles (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Add a closing reference such as Fixes #12345 to the PR description (accepted keywords: Fixes, Closes, Resolves).

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 skip-pr-checks label.

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Jun 5, 2026

Code Review ⚠️ Changes requested 2 resolved / 3 findings

Refactors the connector setup UI with improved schema-driven form rendering and robust E2E test coverage, but normalizePatchNodes must be updated to permit the intentional removal of defaultPersona.

⚠️ Edge Case: normalizePatchNodes prevents intentional removal of defaultPersona

📄 openmetadata-sdk/src/main/java/org/openmetadata/sdk/services/teams/UserService.java:26-31

In UserService.normalizePatchNodes, when entity.getDefaultPersona() == null, both originalNode and updatedNode have the defaultPersona field stripped. This means:

  1. If a caller fetches a User that has a defaultPersona set, then calls update() with the entity's defaultPersona left as null (intending to remove it), the SDK will strip the field from both nodes, producing no diff — the removal is silently swallowed.

  2. Java cannot distinguish "field not set" (default null) from "explicitly cleared to null", so there's no way to intentionally remove a user's defaultPersona through this update() path.

This is problematic because defaultPersona is a fully writable field (managed via UserRepository.UserUpdater.updateDefaultPersona and the DEFAULTS_TO relationship), not a response-only/computed field as the base-class doc comment suggests.

Consider adding a sentinel or using the raw entity JSON to detect intentional null-setting, or document that defaultPersona removal must go through a direct PATCH call rather than this SDK helper.

Only suppress the diff when the original also lacked defaultPersona, preserving intentional removals.
@Override
protected void normalizePatchNodes(JsonNode originalNode, JsonNode updatedNode, User entity) {
  // Only strip defaultPersona when it was already absent in the original snapshot,
  // so intentional removals (original has it, updated doesn't) still produce a diff.
  if (entity.getDefaultPersona() == null && !originalNode.has("defaultPersona")) {
    removeField(updatedNode, "defaultPersona");
  }
}
✅ 2 resolved
Bug: connectionDisplayName useMemo won't update when form data changes

📄 openmetadata-ui/src/main/resources/ui/src/components/common/TestConnection/TestConnection.tsx:184-198
The connectionDisplayName memo depends on [connectionType, getData, serviceName]. Because getData is a stable callback (reads from a form ref), its identity never changes, so the memo is only computed once on mount. For a new service where the form starts empty, the display name will remain the fallback (serviceName || connectionType) even after the user fills in hostPort, account, etc. The TestConnectionModal subtitle will therefore never show the actual host the user typed.

This means the modal will display something like "Snowflake" instead of the user's actual account URL, reducing the usefulness of the redesigned modal header.

Edge Case: useEffect re-expands step immediately after user collapses it

📄 openmetadata-ui/src/main/resources/ui/src/components/common/TestConnection/TestConnectionModal/TestConnectionModal.tsx:358-372
The useEffect at lines 358-375 in TestConnectionModal.tsx includes expandedStepName in its dependency array. When a user clicks to collapse the currently expanded step (setting expandedStepName to undefined), the effect immediately re-fires, the guard if (isTestingConnection || expandedStepName) { return; } passes (since it's now falsy), and it calls setExpandedStepName(firstStepWithResult.name) — re-expanding a step. This makes it impossible for users to collapse all steps after test results arrive.

🤖 Prompt for agents
Code Review: Refactors the connector setup UI with improved schema-driven form rendering and robust E2E test coverage, but normalizePatchNodes must be updated to permit the intentional removal of defaultPersona.

1. ⚠️ Edge Case: normalizePatchNodes prevents intentional removal of defaultPersona
   Files: openmetadata-sdk/src/main/java/org/openmetadata/sdk/services/teams/UserService.java:26-31

   In `UserService.normalizePatchNodes`, when `entity.getDefaultPersona() == null`, both `originalNode` and `updatedNode` have the `defaultPersona` field stripped. This means:
   
   1. If a caller fetches a User that has a `defaultPersona` set, then calls `update()` with the entity's `defaultPersona` left as null (intending to remove it), the SDK will strip the field from both nodes, producing no diff — the removal is silently swallowed.
   
   2. Java cannot distinguish "field not set" (default null) from "explicitly cleared to null", so there's no way to intentionally remove a user's `defaultPersona` through this `update()` path.
   
   This is problematic because `defaultPersona` is a fully writable field (managed via `UserRepository.UserUpdater.updateDefaultPersona` and the `DEFAULTS_TO` relationship), not a response-only/computed field as the base-class doc comment suggests.
   
   Consider adding a sentinel or using the raw entity JSON to detect intentional null-setting, or document that defaultPersona removal must go through a direct PATCH call rather than this SDK helper.

   Fix (Only suppress the diff when the original also lacked defaultPersona, preserving intentional removals.):
   @Override
   protected void normalizePatchNodes(JsonNode originalNode, JsonNode updatedNode, User entity) {
     // Only strip defaultPersona when it was already absent in the original snapshot,
     // so intentional removals (original has it, updated doesn't) still produce a diff.
     if (entity.getDefaultPersona() == null && !originalNode.has("defaultPersona")) {
       removeField(updatedNode, "defaultPersona");
     }
   }

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 5, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants