Skip to content

Fixes #30908: Add Conversation V2 and activity replies - #30909

Open
harshach wants to merge 38 commits into
mainfrom
harshach/activity-replies
Open

Fixes #30908: Add Conversation V2 and activity replies#30909
harshach wants to merge 38 commits into
mainfrom
harshach/activity-replies

Conversation

@harshach

@harshach harshach commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Describe your changes:

Fixes #30908

I replaced the legacy FeedRepository and /v1/feed runtime with schema-first Conversation V2 APIs so user conversations and ActivityEvent replies have explicit ownership, authorization, storage, and lifecycle semantics while tasks and announcements remain on their dedicated APIs. The UI now loads activity replies lazily by ActivityEvent ID and uses dedicated conversation clients without synthetic roots or duplicate first replies.

Type of change:

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

High-level design:

  • Added schema-first Conversation, ConversationReply, ConversationSource, ConversationFilterType, and CreateConversation models, with a dedicated repository owning authorization, validation, transactions, persistence, events, mentions, domain projection, cleanup, and hydration; REST resources only bind parameters and delegate.
  • User conversations retain a root message, while the first ActivityEvent reply transactionally creates an internal conversation whose ID equals the ActivityEvent ID; root-first row locking and bounded deadlock retry make concurrent first/subsequent replies safe and isolate activities that share the same about value.
  • Storage uses four tables: JSON-backed conversation roots and per-reply rows with generated indexed columns, plus normalized mention and domain projections; reactions reuse the existing schema inside each bounded record rather than adding a fifth table.
  • MySQL and PostgreSQL DDL and repeatable legacy conversion live only in 2.0.0; legacy Conversation threads are migrated, Task and Announcement rows remain owned by their migrations, Chatbot rows remain recoverable from the archive, and subscription-compatible thread/post event enum values are retained with entityType=conversation.
  • Activity comments are retained indefinitely by default and are deleted only when an administrator configures an explicit Data Retention policy; /v1/feed has no compatibility alias, so clients must cut over to /v1/conversations, /v1/activity, /v1/tasks, or /v1/announcements.
  • Reusing legacy Thread storage or adding posts directly to ActivityEvent JSON were rejected because both preserve the mixed feed runtime or duplicate conversation authorization, mention, notification, and pagination behavior.

Tests:

Use cases covered

  • Complete user-conversation CRUD, resolution, root/reply reactions, reply edit/delete, mentions, filters, keyset pagination, domain visibility, rename/delete cleanup, event payloads, and creator/admin authorization.
  • Empty, first, subsequent, and concurrent ActivityEvent replies; same-about isolation; deleted/expired targets; reaction ownership; retention defaults and explicit cleanup.
  • Homepage/entity activity rendering, task separation, drawer replies, no duplicate first reply, reaction tooltip identity, mention notifications including Chinese encoding, and Context Center conversation permissions.

Unit tests

  • I added unit tests for the new/changed logic.
  • Files added/updated: ConversationRepositoryTest, ConversationFilterTest, ConversationMigrationTest, ConversationReferenceMigrationTest, ActivityStreamRepositoryTest, formatter/retention tests, Conversation REST-client tests, and Activity Feed provider/card/action/drawer tests.
  • Focused validation: 21 backend tests and 121 UI tests passed.
  • Coverage percentage: not measured locally.

Backend integration tests

  • I added integration tests in openmetadata-integration-tests/ for new/changed API endpoints.
  • Files added/updated: ConversationResourceIT, ActivityResourceIT, ConversationSchemaMigrationIT, and DataRetentionAppIT; legacy Feed suites were migrated or removed.
  • MySQL real-application run: 66 tests passed; PostgreSQL real migration run: 2 tests passed.

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • I added and migrated Playwright E2E tests under openmetadata-ui/.../ui/playwright/ for UI changes.
  • The traceability matrix is in playwright/CONVERSATION_V2_TRACEABILITY.md; 141 selected scenarios and their setup dependencies collected successfully.

Manual testing performed

Not performed; validation used repository units plus real MySQL/PostgreSQL application integration tests and Playwright collection.

UI screen recording / screenshots:

Not yet attached. This PR is opened as a draft and needs an end-to-end UI recording before it is marked ready for review.

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 the tag Backward-Incompatible-Change.

@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Aug 4, 2026
@harshach
harshach marked this pull request as ready for review August 4, 2026 04:19
@harshach
harshach requested review from a team as code owners August 4, 2026 04:19
Copilot AI lite review requested due to automatic review settings August 4, 2026 04:19
@greptile-apps

greptile-apps Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Too many files changed for review (318 files, 100 file limit).

Bypass the limit by tagging @greptile-apps to review.

Copilot AI left a comment

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.

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

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

✅ PR checks passed

The linked issue has a description and all required Shipping project fields set. Thanks!

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

✅ 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 August 4, 2026 04:23

Copilot AI left a comment

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.

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 August 4, 2026 04:31

Copilot AI left a comment

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.

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

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🔴 Playwright Results — workflow failed

Validated commit 12e3e7425515feab0fa0a609445c6f3ed7a84b68 in Playwright run 32810285937, attempt 2.

✅ 1877 passed · ❌ 4 failed · 🟡 4 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky

Action needed: test(s) failed on every attempt against this PR’s validated commit — see Genuine Failures below. These are test failures, not CI budget or infrastructure issues.

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 3h 41m 43s

⏱️ Max setup 4m 18s · max shard execution 19m 57s · max shard-job elapsed before upload 23m 23s · reporting 11s

🌐 193.69 requests/attempt · 2.29 app boots/UI scenario · 33.00% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 33% (convergence target: at most 15%).
  • Application boot ratio was 2.29 per UI scenario (4610 boots / 2013 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 141 0 0 0 0 0
✅ Shard chromium-02 156 0 0 0 0 0
✅ Shard chromium-03 155 0 0 0 0 0
🔴 Shard chromium-04 143 1 3 0 0 0
✅ Shard chromium-05 184 0 0 0 0 0
🔴 Shard chromium-06 166 1 0 0 0 0
✅ Shard chromium-07 150 0 0 0 0 0
✅ Shard chromium-08 171 0 0 0 0 0
🟡 Shard chromium-09 178 0 1 0 0 0
🔴 Shard chromium-10 170 2 0 0 0 0
✅ Shard data-asset-rules-01 61 0 0 0 0 0
✅ Shard domain-isolation-01 16 0 0 0 0 0
✅ Shard global-state-01 34 0 0 0 0 0
✅ Shard import-export-01 43 0 0 0 0 0
✅ Shard ingestion-01 38 0 0 0 0 0
✅ Shard ingestion-02 26 0 0 0 0 0
✅ Shard reindex-01 5 0 0 0 0 0
✅ Shard search-01 11 0 0 0 0 0
✅ Shard search-rbac-01 29 0 0 0 0 0

Genuine Failures (failed on all attempts)

Features/ActivityFeed.spec.tsactivity cards open a reply drawer on the landing widget (shard chromium-04)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed  Locator: locator('.ant-drawer-content') Expected: visible Timeout: 15000ms Error: element(s) not found  Call log: �[2m  - Expect "toBeVisible" with timeout 15000ms�[22m �[2m  - waiting for locator('.ant-drawer-content')�[22m 
Pages/CustomProperties.spec.tsentityReferenceList shows item count, scrollable list, no expand toggle (shard chromium-06)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoContainText�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed  Locator: locator('[data-testid="custom-property-cp-entityReferenceList-a191e7c9.!@#%`()_-=+{}[]|;\',.?-card"]').getByTestId('property-name') Expected substring: �[32m"�[7m(7�[27m)"�[39m Received string:    �[31m"�[7mcp-entityReferenceList-a191e7c9.!@#%`()_-=+{}[]|;',.? (5�[27m)"�[39m Timeout: 15000ms  Call log: �[2m  - Expect "toContainText" with timeout 15000ms�[22m �[2m  - waiting for locator('[data-testid="custom-property-cp-entityReferenceList-a191e7c9.!@#%`()_-=+{}[]|;\',.?-card"]').getByTestId('property-name')�[22m �[2m    19 × locator resolved to <span data-testid="property-name" class="ant-typography text-grey-body property-name">cp-entityReferenceList-a191e7c9.!@#%`()_-=+{}[]|;…</span>�[22m �[2m       - unexpected value "cp-entityReferenceList-a191e7c9.!@#%`()_-=+{}[]|;',.? (5)"�[22m 
Features/ActivityAPI.spec.tsadds a comment to a feed item (shard chromium-10)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed  Locator: getByText('Test comment d84a29fb') Expected: visible Timeout: 10000ms Error: element(s) not found  Call log: �[2m  - Expect "toBeVisible" with timeout 10000ms�[22m �[2m  - waiting for getByText('Test comment d84a29fb')�[22m 
Features/ActivityAPI.spec.tscreates exactly one reply and isolates activities with the same about (shard chromium-10)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed  Locator: getByText('First activity reply b03efb08') Expected: visible Timeout: 10000ms Error: element(s) not found  Call log: �[2m  - Expect "toBeVisible" with timeout 10000ms�[22m �[2m  - waiting for getByText('First activity reply b03efb08')�[22m 
🟡 4 flaky test(s) (passed on retry)
  • Features/ActivityFeed.spec.tsMention notification shows correct user details in Notification box (shard chromium-04, 1 retry)
  • Pages/TasksUIFlow.spec.tsCreate and reject tag task for Dashboard via UI (shard chromium-04, 1 retry)
  • Pages/TasksUIFlow.spec.tsCreate and resolve description task for Pipeline via UI (shard chromium-04, 1 retry)
  • Features/ContextCenterMemories.spec.tsadding a linked asset in edit mode shows entity badge on the row (shard chromium-09, 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

Copilot AI review requested due to automatic review settings August 4, 2026 05:03
@harshach
harshach requested review from a team, akash-jain-10 and tutte as code owners August 4, 2026 05:03

Copilot AI left a comment

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.

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 left a comment

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.

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

This reverts commit da9a1aa.

The extra workers were treating a symptom. #31244 fixes the actual cause - a
5s Awaitility poll delay per root in NamespaceCleanup - and its lanes run green
at the original 4 workers:

  parallel (mysql-elasticsearch)     32m45s
  parallel (postgres-es-redis)       31m35s
  parallel (postgres-opensearch)     39m30s

all well inside the 65m budget, against the timeouts we were seeing before. So
the parallelism bump buys nothing, and this branch no longer needs to carry a
.github/workflows change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

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.

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

Copilot AI left a comment

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.

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

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

⚠️ UI Checkstyle passed — lint findings in changed files

🔍 ESLint findings in this PR's files — 0 error(s), 177 warning(s)

Errors block the build. Warnings do not yet — they are rules whose backlog is still
being worked down, listed so this PR does not add to it. See docs/ui-code-quality-gate.md.

0 error(s), 177 warning(s) across 34 changed file(s).

Count Rule
58 sonarjs/no-duplicate-string
33 react-hooks/exhaustive-deps
18 sonarjs/no-clear-text-protocols
15 sonarjs/cyclomatic-complexity
9 openmetadata-imports/review-sequential-api-calls
8 sonarjs/expression-complexity
7 openmetadata-imports/no-lower-layer-page-imports
7 @typescript-eslint/no-explicit-any
5 sonarjs/no-nested-conditional
4 sonarjs/cognitive-complexity
All findings
Location Rule Message
🟡 src/components/ActivityFeed/ActivityFeedCard/FeedCardBody/FeedCardBodyNew.tsx:126:6 react-hooks/exhaustive-deps React Hook useMemo has unnecessary dependencies: 'isPost' and 'postMessage'. Either exclude them or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedCard/FeedCardBody/FeedCardBodyNew.tsx:172:6 react-hooks/exhaustive-deps React Hook useMemo has missing dependencies: 'handleSave', 'onEditCancel', and 't'. Either include them or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.tsx:85:30 sonarjs/cognitive-complexity Refactor this function to reduce its Cognitive Complexity from 18 to the 15 allowed.
🟡 src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.tsx:85:30 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 47 which is greater than 10 authorized.","cost":37,"secondaryLocations":[{"line":85,"column":29,"endLine":85,"endColumn
🟡 src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.tsx:100:7 sonarjs/expression-complexity Reduce the number of conditional operators (5) used in the expression (maximum allowed 3).
🟡 src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.tsx:216:6 react-hooks/exhaustive-deps React Hook useMemo has a missing dependency: 'showThread'. Either include it or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.tsx:245:9 react-hooks/exhaustive-deps The 'closeFeedEditor' function makes the dependencies of useMemo Hook (at line 294) change on every render. Move it inside the useMemo callback. Alternatively,
🟡 src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.tsx:250:5 sonarjs/expression-complexity Reduce the number of conditional operators (4) used in the expression (maximum allowed 3).
🟡 src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.tsx:553:21 sonarjs/expression-complexity Reduce the number of conditional operators (4) used in the expression (maximum allowed 3).
🟡 src/components/ActivityFeed/ActivityFeedCardNew/CommentCard.component.tsx:58:26 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 12 which is greater than 10 authorized.","cost":2,"secondaryLocations":[{"line":58,"column":25,"endLine":58,"endColumn"
🟡 src/components/ActivityFeed/ActivityFeedCardNew/CommentCard.component.tsx:94:9 react-hooks/exhaustive-deps The 'onUpdate' function makes the dependencies of useCallback Hook (at line 103) change on every render. Move it inside the useCallback callback. Alternatively,
🟡 src/components/ActivityFeed/ActivityFeedCardNew/CommentCard.component.tsx:132:6 react-hooks/exhaustive-deps React Hook useMemo has missing dependencies: 'defaultValue' and 'reply.message'. Either include them or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedCardNew/CommentCard.test.tsx:115:9 sonarjs/no-clear-text-protocols Using http protocol is insecure. Use https instead.
🟡 src/components/ActivityFeed/ActivityFeedDrawer/ActivityFeedDrawer.tsx:62:31 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 3 times.
🟡 src/components/ActivityFeed/ActivityFeedDrawer/ActivityFeedDrawer.tsx:121:17 @typescript-eslint/no-non-null-assertion Forbidden non-null assertion.
🟡 src/components/ActivityFeed/ActivityFeedDrawer/ActivityFeedDrawer.tsx:133:19 @typescript-eslint/no-non-null-assertion Forbidden non-null assertion.
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:232:6 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'fetchTestCaseResolution'. Either include it or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:266:7 sonarjs/cognitive-complexity Refactor this function to reduce its Cognitive Complexity from 22 to the 15 allowed.
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:266:7 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 21 which is greater than 10 authorized.","cost":11,"secondaryLocations":[{"line":266,"column":6,"endLine":266,"endColum
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:384:5 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 't'. Either include it or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:431:5 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 't'. Either include it or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:470:5 react-hooks/exhaustive-deps React Hook useCallback has missing dependencies: 'setActiveTask' and 't'. Either include them or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:605:9 react-hooks/exhaustive-deps The 'updateReactions' function makes the dependencies of useMemo Hook (at line 928) change on every render. Move it inside the useMemo callback. Alternatively,
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:629:11 openmetadata-imports/review-sequential-api-calls Review these sequential API requests. If they are independent, start them together with Promise.all/Promise.allSettled; keep sequencing only when data-dependent
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:630:11 openmetadata-imports/review-sequential-api-calls Review these sequential API requests. If they are independent, start them together with Promise.all/Promise.allSettled; keep sequencing only when data-dependent
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:690:9 react-hooks/exhaustive-deps The 'updateEditorFocus' function makes the dependencies of useMemo Hook (at line 928) change on every render. Move it inside the useMemo callback. Alternatively
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:698:6 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'setActiveThread'. Either include it or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:705:6 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'setActiveTask'. Either include it or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx:780:28 sonarjs/no-nested-functions Refactor this code to not nest functions more than 4 levels deep.
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:117:26 sonarjs/cognitive-complexity Refactor this function to reduce its Cognitive Complexity from 16 to the 15 allowed.
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:117:26 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 34 which is greater than 10 authorized.","cost":24,"secondaryLocations":[{"line":117,"column":25,"endLine":117,"endColu
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:258:5 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'onUpdateFeedCount'. Either include it or remove the dependency array. If 'onUpdateFeedCount' changes too often
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:261:48 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 15 which is greater than 10 authorized.","cost":5,"secondaryLocations":[{"line":261,"column":47,"endLine":261,"endColum
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:271:11 sonarjs/no-nested-conditional Extract this nested ternary operation into an independent statement.
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:330:6 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'userId'. Either include it or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:597:5 react-hooks/exhaustive-deps React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:628:6 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'activeTab'. Either include it or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:722:6 react-hooks/exhaustive-deps React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array.
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:861:15 sonarjs/expression-complexity Reduce the number of conditional operators (4) used in the expression (maximum allowed 3).
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:874:10 sonarjs/expression-complexity Reduce the number of conditional operators (5) used in the expression (maximum allowed 3).
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:876:13 sonarjs/no-nested-conditional Extract this nested ternary operation into an independent statement.
🟡 src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:901:10 sonarjs/expression-complexity Reduce the number of conditional operators (5) used in the expression (maximum allowed 3).
🟡 src/components/ActivityFeed/ActivityThreadPanel/ActivityThreadPanelBody.tsx:79:4 sonarjs/cognitive-complexity Refactor this function to reduce its Cognitive Complexity from 26 to the 15 allowed.
🟡 src/components/ActivityFeed/ActivityThreadPanel/ActivityThreadPanelBody.tsx:79:4 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 21 which is greater than 10 authorized.","cost":11,"secondaryLocations":[{"line":79,"column":3,"endLine":79,"endColumn"
🟡 src/components/ActivityFeed/ActivityThreadPanel/ActivityThreadPanelBody.tsx:262:13 sonarjs/no-nested-conditional Extract this nested ternary operation into an independent statement.
🟡 src/components/ActivityFeed/ActivityThreadPanel/ActivityThreadPanelBody.tsx:280:14 sonarjs/expression-complexity Reduce the number of conditional operators (4) used in the expression (maximum allowed 3).
🟡 src/components/ActivityFeed/ActivityThreadPanel/ActivityThreadPanelBody.tsx:297:15 sonarjs/no-nested-conditional Extract this nested ternary operation into an independent statement.
🟡 src/components/ActivityFeed/ActivityThreadPanel/ActivityThreadPanelBody.tsx:339:17 sonarjs/no-nested-conditional Extract this nested ternary operation into an independent statement.
🟡 src/components/ActivityFeed/Shared/ActivityFeedActions.tsx:52:30 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 15 which is greater than 10 authorized.","cost":5,"secondaryLocations":[{"line":52,"column":29,"endLine":52,"endColumn"
🟡 src/components/Announcement/AnnouncementThreadBody.component.tsx:32:1 openmetadata-imports/no-circular-imports This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency.

… and 127 more. Run make ui-checkstyle-changed locally for the full list.


Fix locally (fast - only checks files changed in this branch):

make ui-checkstyle-changed

…ies-30909

# Conflicts:
#	ingestion/src/metadata/ingestion/source/pipeline/prefect/metadata.py

Copilot AI left a comment

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.

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

Comment thread ingestion/src/metadata/ingestion/ometa/mixins/conversation_mixin.py
_result_list already guarded against a non-dict response, but then indexed
response["data"] and response["paging"]["total"]. An error body decodes to a
dict too, so any payload without those keys raised KeyError instead of falling
back to an empty EntityList.

Read both through .get with defaults. `or {}` / `or []` rather than a plain
default so an explicit null paging or data also degrades instead of raising.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

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.

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

…lies

# Conflicts:
#	.github/scripts/select_playwright_tests.py
#	bootstrap/sql/migrations/native/2.0.0/mysql/postDataMigrationSQLScript.sql
#	bootstrap/sql/migrations/native/2.0.0/mysql/schemaChanges.sql
#	bootstrap/sql/migrations/native/2.0.0/postgres/postDataMigrationSQLScript.sql
#	bootstrap/sql/migrations/native/2.0.0/postgres/schemaChanges.sql
#	openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/ColumnSearchIndexIT.java
#	openmetadata-service/src/main/java/org/openmetadata/service/events/subscription/AlertUtil.java
#	openmetadata-service/src/main/java/org/openmetadata/service/events/subscription/AlertsRuleEvaluator.java
#	openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/UserRepository.java
#	openmetadata-service/src/main/java/org/openmetadata/service/migration/mysql/v200/Migration.java
#	openmetadata-service/src/main/java/org/openmetadata/service/migration/postgres/v200/Migration.java
#	openmetadata-service/src/main/java/org/openmetadata/service/resources/activity/ActivityResource.java
#	openmetadata-service/src/test/java/org/openmetadata/service/events/subscription/AlertUtilTest.java
#	openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ActivityAPI.spec.ts
#	openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ActivityFeed.spec.ts
#	openmetadata-ui/src/main/resources/ui/playwright/utils/activityAPI.ts
#	openmetadata-ui/src/main/resources/ui/playwright/utils/activityFeed.ts
#	openmetadata-ui/src/main/resources/ui/playwright/utils/widgetFilters.ts
#	openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCard/ActivityFeedCard.tsx
#	openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.test.tsx
#	openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.tsx
#	openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedList/ActivityFeedListV1New.component.test.tsx
#	openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedList/ActivityFeedListV1New.component.tsx
#	openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedPanel/ActivityPanelBody.tsx
#	openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.test.tsx
#	openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx
#	openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProviderContext.interface.ts
#	openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.test.tsx
#	openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx
#	openmetadata-ui/src/main/resources/ui/src/components/Customization/GenericProvider/GenericProvider.interface.ts
#	openmetadata-ui/src/main/resources/ui/src/components/Customization/GenericProvider/GenericProvider.tsx
#	openmetadata-ui/src/main/resources/ui/src/mocks/ActivityFeedProvider.mock.tsx
#	openmetadata-ui/src/main/resources/ui/src/rest/feedsAPI.ts
#	openmetadata-ui/src/main/resources/ui/src/utils/DataQuality/TestSummaryGraphUtils.ts
#	openmetadata-ui/src/main/resources/ui/src/utils/FeedUtilsPure.ts

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 109 out of 312 changed files in this pull request and generated no new comments.

…lies

# Conflicts:
#	bootstrap/sql/migrations/native/2.1.0/mysql/schemaChanges.sql
#	bootstrap/sql/migrations/native/2.1.0/postgres/schemaChanges.sql
#	openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java
#	openmetadata-ui/src/main/resources/ui/playwright/utils/activityFeed.ts
#	openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/TaskFeedCard/TaskFeedCard.component.tsx
#	openmetadata-ui/src/main/resources/ui/src/components/NotificationBox/NotificationFeedCard.test.tsx

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 109 out of 312 changed files in this pull request and generated no new comments.

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 109 out of 312 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

ingestion/src/metadata/ingestion/source/pipeline/prefect/metadata.py:373

  • In Prefect pipeline status mapping, _parse_timestamp already returns a Timestamp (it wraps the parsed millis with Timestamp(...)), but _build_task_status wraps that result again via _as_timestamp(...). This extra conversion is inconsistent with other pipeline sources (e.g., dbtcloud) and could break if Timestamp is not a plain int alias.

Related occurrences in the same file: _build_pipeline_status uses timestamp=Timestamp(start_time) and endTime=_as_timestamp(end_time), and _as_timestamp itself is typed as accepting int | None even though the current call sites pass Timestamp | None.

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 110 out of 314 changed files in this pull request and generated 3 comments.

Comment on lines +22 to +26
"activityCommentsRetentionPeriod": {
"title": "Activity Comments Retention Period (days)",
"description": "Enter the retention period for comments on Activity Events in days. Use 0 to retain activity comments forever.",
"type": "integer",
"default": 0,
Comment on lines 4 to 10
"appConfiguration": {
"changeEventRetentionPeriod": 7,
"activityThreadsRetentionPeriod": 60,
"activityCommentsRetentionPeriod": 0,
"profileDataRetentionPeriod": 1440,
"testCaseResultsRetentionPeriod": 1440
},
Comment on lines +19 to 26
$$section
### Activity Comments Retention Period (days) $(id="activityCommentsRetentionPeriod")

Enter the retention period for comments on Activity Events in days. Use 0 to retain activity comments forever.

$$

$$section

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 111 out of 315 changed files in this pull request and generated no new comments.

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 113 out of 318 changed files in this pull request and generated no new comments.

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gitar-bot

gitar-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 8 resolved / 8 findings

Replaces the legacy FeedRepository and runtime with schema-first Conversation V2 APIs and activity replies. Addressed multiple data integrity, migration, and response handling findings.

✅ 8 resolved
Bug: Patching a conversation wipes all root reactions

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ConversationRepository.java:208-222 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ConversationRepository.java:1275-1289
In ConversationRepository.patch(), the persisted conversation is passed through restoreRootImmutableFields() which calls .withReactions(null) before dao.update(..., boundedJson(patched)). Because root reactions are stored inside the conversation JSON record (not a separate table), any PATCH of /message or /resolved silently deletes every reaction on the root message. Note the inconsistency: restoreReplyImmutableFields() correctly does .withReactions(original.getReactions()). Fix restoreRootImmutableFields to preserve the original reactions instead of nulling them.

Performance: list() builds the count filter SQL twice

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ConversationRepository.java:147-149
In ConversationRepository.list(), the total count is computed as conversationDAO.count(filter.build(false).condition(), filter.build(false).params()), invoking filter.build(false) twice and rebuilding the predicate string and parameter map redundantly. Build once into a local variable and reuse it.

Edge Case: One malformed receiver URI drops all webhook recipients

📄 openmetadata-service/src/main/java/org/openmetadata/service/notifications/recipients/strategy/impl/ExternalRecipientResolver.java:88-102 📄 openmetadata-service/src/main/java/org/openmetadata/service/notifications/recipients/strategy/impl/ExternalRecipientResolver.java:66-69
In resolveWebhookRecipients, each receiver is passed to URI.create(receiver) inside a stream. If any single receiver is not a valid URI, URI.create throws IllegalArgumentException, which aborts the whole stream and is caught by resolveExternalRecipients, returning an empty set. That means one bad entry silently discards every otherwise-valid webhook recipient (and only logs a generic message). Consider validating/skipping individual invalid receivers (e.g., filter out bad URIs per-element) so valid recipients are still resolved.

Bug: MySQL CREATE INDEX statements are not idempotent, break re-migration

📄 bootstrap/sql/migrations/native/2.0.0/mysql/schemaChanges.sql:562-566 📄 bootstrap/sql/migrations/native/2.0.0/mysql/schemaChanges.sql:508-522
The 16 new CREATE INDEX statements in the MySQL migration (lines 546-566) omit any existence guard, yet MySQL has no CREATE INDEX IF NOT EXISTS. The comment block at line 568-571 explicitly states these 2.0.0 statements are reprocessed on RC databases when checksums change, and the immediately preceding index at lines 508-525 uses an information_schema.statistics + PREPARE/EXECUTE guard precisely because 'MySQL has no CREATE INDEX IF NOT EXISTS'. On any reprocess (or when the index already exists), these statements fail with 'Duplicate key name', aborting the migration. The Postgres file correctly uses CREATE INDEX IF NOT EXISTS. Wrap each MySQL index in the same information_schema guard pattern used at line 510.

Edge Case: Domain test-suite match reads payload, not stored test suites

📄 openmetadata-service/src/main/java/org/openmetadata/service/events/subscription/AlertsRuleEvaluator.java:505-516 📄 openmetadata-service/src/main/java/org/openmetadata/service/events/subscription/AlertsRuleEvaluator.java:541-546
In matchesEntityOrTestSuiteDomain, the test-suite fallback uses ((TestCase) entity).getTestSuites() from the deserialized change-event payload, whereas the sibling matchesEntityOrTestSuiteOwner path was upgraded to fetch fresh data via resolveTestSuites(...). The class's own comment notes the serialized payload often does not carry relationship fields, so test-suite-domain alerts on test-case events may not fire when the payload omits testSuites (or omits their domains). This matches pre-existing behavior, but the surrounding code was rewritten here and the two paths are now inconsistent. Consider resolving test suites from the store (e.g. resolveTestSuites((TestCase) entity, Entity.FIELD_TEST_SUITES + "," + Entity.FIELD_DOMAINS)) before calling testSuiteMatcher.

...and 3 more resolved from earlier reviews

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

@sonarqubecloud

Copy link
Copy Markdown

@sonarqubecloud

Copy link
Copy Markdown

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

Labels

backend Backward-Incompatible-Change safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Introduce Conversation V2 and retire the legacy feed runtime

6 participants