Skip to content

feat: migrate DeleteModal to new component structure and update related tests#26561

Merged
Rohit0301 merged 10 commits intomainfrom
migrate-delete-modal-mui-switch
Apr 11, 2026
Merged

feat: migrate DeleteModal to new component structure and update related tests#26561
Rohit0301 merged 10 commits intomainfrom
migrate-delete-modal-mui-switch

Conversation

@Rohit0301
Copy link
Copy Markdown
Contributor

@Rohit0301 Rohit0301 commented Mar 17, 2026

Describe your changes:

Fixes 3276

collate PR - https://github.com/open-metadata/openmetadata-collate/pull/3186

I worked on ... because ...

Screenshot 2026-03-13 at 1 23 18 PM

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • 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.

Summary by Gitar

  • Component migration:
    • Migrated DeleteModal to new component structure using @openmetadata/ui-core-components library
    • Replaced MUISwitch with Toggle component across form utilities and tag pages
  • Form field updates:
    • Changed FieldTypes.SWITCH_MUI to FieldTypes.UT_SWITCH in form field definitions
    • Updated prop names: disabledisDisabled, checkedisSelected, muiLabellabel
  • Files removed:
    • Deleted deprecated MUISwitch component and its tests from components/form/MUISwitch
  • Tests updated:
    • Added comprehensive tests for new DeleteModal component
    • Updated test mocks for Toggle component in TagsForm.test.tsx

This will update automatically on new commits.

@Rohit0301 Rohit0301 self-assigned this Mar 17, 2026
@Rohit0301 Rohit0301 added the safe to test Add this label to run secure Github workflows on PRs label Mar 17, 2026
@Rohit0301 Rohit0301 requested a review from a team as a code owner March 17, 2026 17:53
Comment thread openmetadata-ui/src/main/resources/ui/src/utils/formUtils.tsx
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 17, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 64%
64.27% (59516/92600) 43.76% (31018/70868) 46.95% (9360/19936)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 17, 2026

🟡 Playwright Results — all passed (24 flaky)

✅ 3594 passed · ❌ 0 failed · 🟡 24 flaky · ⏭️ 207 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 453 0 4 2
🟡 Shard 2 639 0 3 32
🟡 Shard 3 646 0 5 26
🟡 Shard 4 619 0 3 47
🟡 Shard 5 606 0 1 67
🟡 Shard 6 631 0 8 33
🟡 24 flaky test(s) (passed on retry)
  • Features/DataAssetRulesDisabled.spec.ts › Verify the SearchIndex entity item action after rules disabled (shard 1, 1 retry)
  • Features/CustomizeDetailPage.spec.ts › Stored Procedure - customization should work (shard 1, 1 retry)
  • Pages/AuditLogs.spec.ts › should apply both User and EntityType filters simultaneously (shard 1, 1 retry)
  • Pages/UserCreationWithPersona.spec.ts › Create user with persona and verify on profile (shard 1, 1 retry)
  • Features/BulkEditEntity.spec.ts › Glossary (shard 2, 1 retry)
  • Features/DataQuality/BundleSuiteBulkOperations.spec.ts › Bulk selection operations (shard 2, 1 retry)
  • Features/Glossary/GlossaryWorkflow.spec.ts › should display correct status badge color and icon (shard 2, 1 retry)
  • Features/IncidentManager.spec.ts › Complete Incident lifecycle with table owner (shard 3, 1 retry)
  • Features/Permissions/GlossaryPermissions.spec.ts › Team-based permissions work correctly (shard 3, 1 retry)
  • Flow/ExploreDiscovery.spec.ts › Should display deleted assets when showDeleted is checked and deleted is not present in queryFilter (shard 3, 1 retry)
  • Flow/NestedChildrenUpdates.spec.ts › should update nested column description immediately without page refresh (shard 3, 1 retry)
  • Flow/PersonaDeletionUserProfile.spec.ts › User profile loads correctly before and after persona deletion (shard 3, 1 retry)
  • Pages/Customproperties-part2.spec.ts › entityReferenceList shows item count, scrollable list, no expand toggle (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Worksheet (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Rename domain with tags and glossary terms preserves associations (shard 4, 1 retry)
  • Pages/ExploreTree.spec.ts › Verify Database and Database Schema available in explore tree (shard 5, 1 retry)
  • Pages/HyperlinkCustomProperty.spec.ts › should accept valid http and https URLs (shard 6, 1 retry)
  • Pages/InputOutputPorts.spec.ts › Tab renders with empty state when no ports exist (shard 6, 1 retry)
  • Pages/Lineage/DataAssetLineage.spec.ts › verify create lineage for entity - Stored Procedure (shard 6, 1 retry)
  • Pages/Lineage/DataAssetLineage.spec.ts › Column lineage for apiEndpoint -> apiEndpoint (shard 6, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 6, 1 retry)
  • Pages/ProfilerConfigurationPage.spec.ts › Non admin user (shard 6, 1 retry)
  • Pages/Users.spec.ts › Permissions for table details page for Data Consumer (shard 6, 1 retry)
  • VersionPages/EntityVersionPages.spec.ts › Directory (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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isn't this to be part of core-component?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ShaileshParmar11 Only base components should be included in core-components, while any custom components we create should be placed in OMD.

@ShaileshParmar11 ShaileshParmar11 self-requested a review April 6, 2026 09:46
@ShaileshParmar11 ShaileshParmar11 dismissed their stale review April 6, 2026 09:46

added comment

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Apr 8, 2026

Code Review 👍 Approved with suggestions 3 resolved / 4 findings

DeleteModal migration to new component structure resolves silent time range filter issues and response leak in StdioTransport. Consider removing unused SQL parameter bindings in McpExecutionDAO insert/delete methods to reduce confusion.

💡 Quality: McpExecutionDAO binds unused SQL parameters in insert/delete

The McpExecutionDAO methods insertWithoutExtension, insert, and deleteAtTimestamp bind parameters (entityFQNHash, jsonSchema, extension) that are not referenced in the SQL statements. The mcp_execution_entity table only has columns id, serverId, json, timestamp, so these bindings are dead code carried over from the EntityTimeSeriesDAO parent interface.

While JDBI silently ignores unused bindings, this creates confusion about what the methods actually do and makes maintenance harder. The method signatures suggest data is being stored that isn't.

Suggested fix
Remove unused parameters from the method
signatures to match the actual SQL:

void insertWithoutExtension(
    @Define("table") String table,
    @Bind("json") String json);

void deleteAtTimestamp(
    @Define("table") String table,
    @Bind("serverId") String serverId,
    @Bind("timestamp") Long timestamp);
✅ 3 resolved
Edge Case: McpExecution list silently ignores partial time range filters

In McpExecutionResource.list(), when only startTs or only endTs is provided (but not both), the code falls through to the else branch which calls listWithOffset without any time filtering. This silently discards the user's filter with no error or warning, returning unfiltered results.

The Swagger docs say the endpoint supports filtering by startTs and endTs, so users would reasonably expect providing just startTs to return all executions after that timestamp.

Performance: StdioTransport leaks responses in _responses dict after timeout

In StdioTransport.send_request(), when event.wait() times out, the code cleans up _response_events[msg_id] and _responses[msg_id]. However, if the background reader thread subsequently receives the late response, it stores it in _responses[msg_id] (line 158) with no one to ever retrieve it. Over time with repeated timeouts, this causes unbounded growth of the _responses dict.

The impact is low for short-lived ingestion runs, but could matter for long-running MCP discovery sessions with flaky servers.

Bug: Spread of switchRest can silently override explicit label prop

📄 openmetadata-ui/src/main/resources/ui/src/utils/formUtils.tsx:471 📄 openmetadata-ui/src/main/resources/ui/src/utils/formUtils.tsx:478 📄 openmetadata-ui/src/main/resources/ui/src/utils/formUtils.tsx:481 📄 openmetadata-ui/src/main/resources/ui/src/pages/TagsPage/tagFormFields.tsx:179 📄 openmetadata-ui/src/main/resources/ui/src/utils/formUtils.tsx:470-484
In formUtils.tsx:471, label is not destructured out of props as ToggleProps before creating switchRest. Since label is a valid ToggleProps property, if any caller passes label inside props, it will remain in switchRest and the spread {...switchRest} on line 481 will override the explicit label={isString(label) ? label : undefined} set on line 478 (due to JSX spread ordering — later props win).

Currently no callers pass label in props, but this is a latent bug that will silently break when someone does.

🤖 Prompt for agents
Code Review: DeleteModal migration to new component structure resolves silent time range filter issues and response leak in StdioTransport. Consider removing unused SQL parameter bindings in McpExecutionDAO insert/delete methods to reduce confusion.

1. 💡 Quality: McpExecutionDAO binds unused SQL parameters in insert/delete

   The `McpExecutionDAO` methods `insertWithoutExtension`, `insert`, and `deleteAtTimestamp` bind parameters (`entityFQNHash`, `jsonSchema`, `extension`) that are not referenced in the SQL statements. The `mcp_execution_entity` table only has columns `id`, `serverId`, `json`, `timestamp`, so these bindings are dead code carried over from the `EntityTimeSeriesDAO` parent interface.
   
   While JDBI silently ignores unused bindings, this creates confusion about what the methods actually do and makes maintenance harder. The method signatures suggest data is being stored that isn't.

   Suggested fix:
   Remove unused parameters from the method
   signatures to match the actual SQL:
   
   void insertWithoutExtension(
       @Define("table") String table,
       @Bind("json") String json);
   
   void deleteAtTimestamp(
       @Define("table") String table,
       @Bind("serverId") String serverId,
       @Bind("timestamp") Long timestamp);

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 Apr 8, 2026

@Rohit0301 Rohit0301 merged commit d7e1534 into main Apr 11, 2026
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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