From ea290cead36e1c5c8c37f3e5cf7a90ecab4db60d Mon Sep 17 00:00:00 2001 From: Aniket Katkar Date: Fri, 21 Aug 2026 11:31:34 +0530 Subject: [PATCH 1/2] Fixes #31854: make the Mentions sub-tab list the tasks you are mentioned in The Tasks panel renders TaskListV1 off the provider's `tasks` for both of its sub-tabs, but the fetch effect gated only on the My Tasks sub-tab, so Mentions fell through to getFeedData and wrote `entityThread` instead. `tasks` was never updated, leaving the previous My Tasks list on screen -- and empty after a reload. Collapse "which list renders" and "which fetcher runs" onto a single `isTaskListTab` predicate so they cannot drift apart again. The correct request then surfaced a second defect: ListFilter built `SELECT fr.toId FROM field_relationship`, but that table has no toId column (TaskRepository.storeMentions writes the task id into toFQN), so every `?mentionedUser=` query failed with a SQL syntax error on MySQL and Postgres alike. Match on the indexed fromFQNHash -- the same hash @BindFQN writes on insert -- and select toFQN. Also fixed, because they are what makes the stale list visible: - ActivityFeedProvider now clears the list it owns plus the shared entityPaging cursor when a first-page fetch starts, and a shared request sequence stops a superseded response committing rows, the cursor, or clearing the loader. The leftover cursor was letting infinite scroll append a new query's page onto the previous query's list, in the incident tab too. - The isFirstLoad reset keyed off the `subTab` prop, which entity pages never pass (the sub-tab arrives as a URL param), so a URL or back-button driven switch showed the outgoing list with no loader. - handleUpdateTaskFilter fired getTaskData itself on top of the effect already refiring on taskFilter, i.e. two identical requests per filter click. - TaskListV1's resize effect and TestCaseIncidentTab's hardcoded isLoading={false} both treated the new mid-fetch empty window as "no results". Co-Authored-By: Claude Opus 5 (1M context) --- .../openmetadata/it/tests/TaskCommentsIT.java | 37 +++++ .../service/jdbi3/ListFilter.java | 14 +- .../e2e/Features/ActivityFeedTabBadge.spec.ts | 85 +++++++++- .../e2e/Features/Tasks/ActivityFeed.spec.ts | 21 +-- .../ActivityFeedList/TaskListV1.component.tsx | 7 +- .../ActivityFeedProvider.test.tsx | 98 +++++++++++ .../ActivityFeedProvider.tsx | 44 ++++- .../ActivityFeedTab.component.test.tsx | 156 +++++++++++++++--- .../ActivityFeedTab.component.tsx | 128 +++++++------- .../TestCaseIncidentTab.component.tsx | 5 +- .../src/mocks/ActivityFeedProvider.mock.tsx | 35 ++++ 11 files changed, 531 insertions(+), 99 deletions(-) diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TaskCommentsIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TaskCommentsIT.java index 610132cf292b..ddcede90bbfd 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TaskCommentsIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TaskCommentsIT.java @@ -14,6 +14,7 @@ package org.openmetadata.it.tests; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -37,6 +38,7 @@ import org.openmetadata.schema.type.TaskPriority; import org.openmetadata.sdk.client.OpenMetadataClient; import org.openmetadata.sdk.exceptions.ForbiddenException; +import org.openmetadata.sdk.models.ListParams; /** * Integration tests for Task Comments functionality. @@ -325,4 +327,39 @@ void test_commentHasTimestamp() { adminClient.tasks().delete(task.getId().toString(), java.util.Map.of("hardDelete", "true")); } } + + @Test + @Order(13) + void test_listByMentionedUser_returnsTaskFromCommentMention() { + Task mentioning = createTestTask(adminClient); + Task unrelated = createTestTask(adminClient); + + try { + adminClient + .tasks() + .addComment( + mentioning.getId().toString(), + String.format("Please review <#E::user::%s>", shared.USER2.getName())); + adminClient.tasks().addComment(unrelated.getId().toString(), "No mention here"); + + ListParams params = + new ListParams().addFilter("mentionedUser", shared.USER2.getName()).setLimit(1000); + List mentioned = adminClient.tasks().list(params).getData(); + List mentionedIds = mentioned.stream().map(Task::getId).toList(); + + assertTrue( + mentionedIds.contains(mentioning.getId()), + "mentionedUser filter must return the task whose comment mentions the user"); + assertFalse( + mentionedIds.contains(unrelated.getId()), + "mentionedUser filter must exclude tasks that do not mention the user"); + } finally { + adminClient + .tasks() + .delete(mentioning.getId().toString(), java.util.Map.of("hardDelete", "true")); + adminClient + .tasks() + .delete(unrelated.getId().toString(), java.util.Map.of("hardDelete", "true")); + } + } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ListFilter.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ListFilter.java index 8bfd797e9302..491752dde953 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ListFilter.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ListFilter.java @@ -283,13 +283,17 @@ private String getMentionedUserCondition() { if (mentionedUser == null) { return ""; } - queryParams.put("mentionedUserParam", mentionedUser); + // TaskRepository.storeMentions writes the task id into toFQN and the mentioned + // user into fromFQNHash (via @BindFQN, i.e. FullyQualifiedName.buildHash of the + // raw value). field_relationship has no toId column, so selecting one made every + // mentionedUser query fail with an SQLSyntaxErrorException. + queryParams.put("mentionedUserHash", FullyQualifiedName.buildHash(mentionedUser)); return String.format( - "(id IN (SELECT fr.toId FROM field_relationship fr " - + "WHERE fr.fromFQN = :mentionedUserParam " - + "AND fr.toType = 'task' " + "(id IN (SELECT fr.toFQN FROM field_relationship fr " + + "WHERE fr.fromFQNHash = :mentionedUserHash " + + "AND fr.toType = '%s' " + "AND fr.relation = %d))", - Relationship.MENTIONED_IN.ordinal()); + Entity.TASK, Relationship.MENTIONED_IN.ordinal()); } /** diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ActivityFeedTabBadge.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ActivityFeedTabBadge.spec.ts index 39285976bd42..c1988031d633 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ActivityFeedTabBadge.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ActivityFeedTabBadge.spec.ts @@ -75,7 +75,19 @@ async function switchToOpenFilter(page: import('@playwright/test').Page) { await page.getByTestId('open-tasks').click(); await tasksListResponse; } -test.describe('ActivityFeedTab — task filter badge and placeholder', () => { +const waitForMentionedTaskResponse = (page: import('@playwright/test').Page) => + page.waitForResponse((response) => { + if ( + response.request().method() !== 'GET' || + !response.url().includes('/api/v1/tasks') + ) { + return false; + } + + return Boolean(new URL(response.url()).searchParams.get('mentionedUser')); + }); + +test.describe('ActivityFeedTab — task filter badge, placeholder and mentions', () => { const table = new TableClass(); const assigneeUser = new UserClass(); @@ -229,4 +241,75 @@ test.describe('ActivityFeedTab — task filter badge and placeholder', () => { await afterAction(); } }); + + test('Mentions sub-tab lists only the tasks the user is mentioned in', async ({ + browser, + }) => { + const { page, apiContext, afterAction } = await performAdminLogin(browser, { + navigate: true, + }); + + // Own table: the assertions below are exact card counts and chromium runs + // fullyParallel, so sharing the describe-level table would make them depend + // on test order. + const mentionTable = new TableClass(); + + try { + await mentionTable.create(apiContext); + + const fqn = mentionTable.entityResponseData?.fullyQualifiedName as string; + const assignee = assigneeUser.responseData.name; + + await createOpenTask(apiContext, fqn, assignee); + const mentionedTask = await createOpenTask(apiContext, fqn, assignee); + + // A mention relationship is only written from the comment path, so the + // comment is what makes this task match ?mentionedUser=admin. + const commentResponse = await apiContext.post( + `/api/v1/tasks/${mentionedTask.id}/comments`, + { data: { message: 'Please take a look <#E::user::admin>' } } + ); + expect(commentResponse.ok()).toBe(true); + + await mentionTable.visitEntityPage(page); + await navigateToTasksPanel(page); + + // My Tasks lists every task about the entity. + await expect(page.getByTestId('task-feed-card')).toHaveCount(2); + + const mentionsResponse = waitForMentionedTaskResponse(page); + await page.getByTestId('mentions-toggle').click(); + await mentionsResponse; + + // The list has to actually switch — it used to keep rendering My Tasks + // because the mentions fetch wrote to the conversation feed state instead. + await expect(page.getByTestId('task-feed-card')).toHaveCount(1); + await expect( + page.getByTestId('task-feed-card').getByTestId('entity-link') + ).toBeVisible(); + await expect( + page.getByTestId('no-data-placeholder-container') + ).toHaveCount(0); + + // Switching back restores the full list — guards the paging-cursor reset. + const myTasksResponse = waitForTaskListResponse(page); + await page.getByTestId('my-tasks-toggle').click(); + await myTasksResponse; + + await expect(page.getByTestId('task-feed-card')).toHaveCount(2); + + // Landing on the mentions URL directly used to show the empty placeholder. + const mentionsAgain = waitForMentionedTaskResponse(page); + await page.getByTestId('mentions-toggle').click(); + await mentionsAgain; + + await page.reload(); + await waitForPageLoaded(page); + + await expect(page.getByTestId('task-feed-card')).toHaveCount(1); + } finally { + await mentionTable.delete(apiContext); + await afterAction(); + } + }); }); diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Tasks/ActivityFeed.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Tasks/ActivityFeed.spec.ts index f54a27c66c9d..a66ba4022d14 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Tasks/ActivityFeed.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Tasks/ActivityFeed.spec.ts @@ -493,28 +493,29 @@ test.describe('Activity Feed - Entity Page', () => { await openResponse; await waitForPageLoaded(page); - await taskFilterButton.click(); - await expect( - page.locator('.task-filter-container').getByText(/mention/i) - ).toBeVisible(); + await expect(page.getByTestId('mentions-toggle')).toBeVisible(); + // Mentions renders the task list, so it has to query tasks-where-mentioned + // about this entity — not the conversation feed, whose results the mentions + // list never reads. const mentionsResponse = page.waitForResponse((response) => { if ( response.request().method() !== 'GET' || - !response.url().includes('/api/v1/feed') + !response.url().includes('/api/v1/tasks') ) { return false; } const requestUrl = new URL(response.url()); - return requestUrl.searchParams.get('filterType') === 'MENTIONS'; + return ( + Boolean(requestUrl.searchParams.get('mentionedUser')) && + requestUrl.searchParams.get('aboutEntity') === + table.entityResponseData?.fullyQualifiedName + ); }); - await page - .locator('.task-filter-container') - .getByText(/mention/i) - .click(); + await page.getByTestId('mentions-toggle').click(); await mentionsResponse; await waitForPageLoaded(page); }); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedList/TaskListV1.component.tsx b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedList/TaskListV1.component.tsx index 37614f1f12fc..290364c4a066 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedList/TaskListV1.component.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedList/TaskListV1.component.tsx @@ -54,12 +54,17 @@ const TaskListV1 = ({ }, [taskList, selectedTask, onTaskClick]); useEffect(() => { + // While a fetch is in flight the list is intentionally empty; collapsing the + // right panel here would flash the layout on every sub-tab/filter switch. + if (isLoading) { + return; + } if (isEmpty(taskList) && handlePanelResize) { handlePanelResize?.(true); } else { handlePanelResize?.(false); } - }, [taskList]); + }, [taskList, isLoading]); const tasks = useMemo( () => diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.test.tsx index f8ed07026798..62315c0496c7 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.test.tsx @@ -32,6 +32,7 @@ import { DummyEntityActivityFeedComponent, DummyFollowingActivityComponent, DummySetActiveActivityComponent, + DummyTaskListStateComponent, } from '../../../mocks/ActivityFeedProvider.mock'; import { mockUserData } from '../../../mocks/MyDataPage.mock'; import { @@ -331,6 +332,103 @@ describe('ActivityFeedProvider', () => { ); }); + describe('a first-page task fetch replaces the previous result set', () => { + const renderTaskListState = () => + render( + + + + ); + + it('clears the rows and the paging cursor before the new response lands', async () => { + (listTasks as jest.Mock).mockResolvedValueOnce({ + data: [{ id: 'task-open', createdAt: 1 }], + paging: { after: 'cursor-1' }, + }); + + renderTaskListState(); + + await act(async () => { + fireEvent.click(screen.getByTestId('fetch-open')); + }); + + expect(screen.getByTestId('task-ids')).toHaveTextContent('task-open'); + expect(screen.getByTestId('paging-after')).toHaveTextContent('cursor-1'); + + let resolveClosed: (value: unknown) => void = () => undefined; + (listTasks as jest.Mock).mockReturnValueOnce( + new Promise((resolve) => { + resolveClosed = resolve; + }) + ); + + await act(async () => { + fireEvent.click(screen.getByTestId('fetch-closed')); + }); + + // Leaving the open rows and `cursor-1` in place is what kept the previous + // list on screen and let infinite scroll append the new query's next page + // onto it using the old cursor. + expect(screen.getByTestId('task-ids')).toBeEmptyDOMElement(); + expect(screen.getByTestId('paging-after')).toHaveTextContent('none'); + + await act(async () => { + resolveClosed({ + data: [{ id: 'task-closed', createdAt: 2 }], + paging: { after: 'cursor-2' }, + }); + }); + + expect(screen.getByTestId('task-ids')).toHaveTextContent('task-closed'); + expect(screen.getByTestId('paging-after')).toHaveTextContent('cursor-2'); + }); + + it('ignores a response that resolves after a newer request started', async () => { + let resolveFirst: (value: unknown) => void = () => undefined; + let resolveSecond: (value: unknown) => void = () => undefined; + + (listTasks as jest.Mock) + .mockReturnValueOnce( + new Promise((resolve) => { + resolveFirst = resolve; + }) + ) + .mockReturnValueOnce( + new Promise((resolve) => { + resolveSecond = resolve; + }) + ); + + renderTaskListState(); + + await act(async () => { + fireEvent.click(screen.getByTestId('fetch-open')); + }); + await act(async () => { + fireEvent.click(screen.getByTestId('fetch-closed')); + }); + + await act(async () => { + resolveSecond({ + data: [{ id: 'task-closed', createdAt: 2 }], + paging: { after: 'cursor-2' }, + }); + }); + await act(async () => { + resolveFirst({ + data: [{ id: 'task-open', createdAt: 1 }], + paging: { after: 'cursor-1' }, + }); + }); + + await waitFor(() => + expect(screen.getByTestId('task-ids')).toHaveTextContent('task-closed') + ); + + expect(screen.getByTestId('paging-after')).toHaveTextContent('cursor-2'); + }); + }); + it('should call postFeed with button click', async () => { render( diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx index 0a8e10a3fd26..0c3613c2502c 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedProvider/ActivityFeedProvider.tsx @@ -105,6 +105,10 @@ const ActivityFeedProvider = ({ children, user }: Props) => { // overwrite newer results, so each request claims a sequence number and only the // latest one is allowed to commit. const activityRequestSeq = useRef(0); + // getTaskData and getFeedData write the same `loading` and `entityPaging`, so a + // single sequence guards both: an older request must not commit its rows, its + // paging cursor, or clear the loader after a newer one has started. + const listRequestSeq = useRef(0); // For regular feeds (conversations, announcements) const [entityThread, setEntityThread] = useState([]); const [selectedThread, setSelectedThread] = useState(); @@ -221,8 +225,17 @@ const ActivityFeedProvider = ({ children, user }: Props) => { taskStatusGroup?: TaskStatusGroup, limit?: number ) => { + const requestId = ++listRequestSeq.current; try { setLoading(true); + if (!after) { + // A first-page fetch replaces the result set. Dropping the rows and the + // paging cursor now is what stops the previous query's list staying on + // screen, and stops the infinite-scroll effect appending this query's + // next page onto it using the old cursor. + setTasks([]); + setEntityPaging({} as Paging); + } const feedFilterType = filterType ?? FeedFilter.ALL; const domain = activeDomain !== DEFAULT_DOMAIN_VALUE ? activeDomain : undefined; @@ -323,11 +336,19 @@ const ActivityFeedProvider = ({ children, user }: Props) => { }); } + if (listRequestSeq.current !== requestId) { + return; + } + const sortedTasks = orderBy(taskResponse.data, ['createdAt'], ['desc']); setTasks((prev) => (after ? [...prev, ...sortedTasks] : sortedTasks)); setEntityPaging(taskResponse.paging); } catch (err) { + if (listRequestSeq.current !== requestId) { + return; + } + showErrorToast( err as AxiosError, t('server.entity-fetch-error', { @@ -335,7 +356,9 @@ const ActivityFeedProvider = ({ children, user }: Props) => { }) ); } finally { - setLoading(false); + if (listRequestSeq.current === requestId) { + setLoading(false); + } } }, [currentUser, activeDomain] @@ -351,8 +374,13 @@ const ActivityFeedProvider = ({ children, user }: Props) => { taskStatusGroup?: TaskStatusGroup, limit?: number ) => { + const requestId = ++listRequestSeq.current; try { setLoading(true); + if (!after) { + setEntityThread([]); + setEntityPaging({} as Paging); + } const feedFilterType = filterType ?? FeedFilter.ALL; let userId = undefined; @@ -373,9 +401,17 @@ const ActivityFeedProvider = ({ children, user }: Props) => { userId, limit ); + if (listRequestSeq.current !== requestId) { + return; + } + setEntityThread((prev) => (after ? [...prev, ...data] : [...data])); setEntityPaging(paging); } catch (err) { + if (listRequestSeq.current !== requestId) { + return; + } + showErrorToast( err as AxiosError, t('server.entity-fetch-error', { @@ -383,10 +419,12 @@ const ActivityFeedProvider = ({ children, user }: Props) => { }) ); } finally { - setLoading(false); + if (listRequestSeq.current === requestId) { + setLoading(false); + } } }, - [currentUser, user, getTaskData] + [currentUser, user] ); // Here value is the post message and id can be thread id or post id. diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.test.tsx index 50c9bed220fd..59b7b45a38ac 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.test.tsx @@ -32,6 +32,10 @@ const mockFetchUserActivity = jest.fn(); let mockActivityEvents: { id: string; timestamp: number }[] = []; let mockConversationCount = 0; let mockActivityCount = 0; +let mockLoading = false; +let mockTasks: { id: string }[] = []; +let mockEntityPaging: { after?: string } = {}; +let mockIsInView = false; jest.mock('../../../hooks/useApplicationStore', () => ({ useApplicationStore: () => ({ @@ -57,7 +61,7 @@ jest.mock('../../../utils/useRequiredParams', () => ({ })); jest.mock('../../../hooks/useElementInView', () => ({ - useElementInView: () => [{ current: null }, false], + useElementInView: () => [{ current: null }, mockIsInView], })); jest.mock('../ActivityFeedProvider/ActivityFeedProvider', () => ({ @@ -67,9 +71,9 @@ jest.mock('../ActivityFeedProvider/ActivityFeedProvider', () => ({ entityThread: [], getFeedData: mockGetFeedData, getTaskData: mockGetTaskData, - loading: false, - entityPaging: {}, - tasks: [], + loading: mockLoading, + entityPaging: mockEntityPaging, + tasks: mockTasks, selectedTask: null, setActiveTask: jest.fn(), activityEvents: mockActivityEvents, @@ -134,8 +138,11 @@ jest.mock('../ActivityFeedList/ActivityFeedListV1New.component', () => jest.mock('../ActivityFeedList/TaskListV1.component', () => jest .fn() - .mockImplementation(({ emptyPlaceholderText }) => ( -
{emptyPlaceholderText}
+ .mockImplementation(({ emptyPlaceholderText, isLoading, onAfterClose }) => ( +
+
)) ); @@ -194,6 +201,10 @@ describe('ActivityFeedTab', () => { mockActivityEvents = []; mockConversationCount = 0; mockActivityCount = 0; + mockLoading = false; + mockTasks = []; + mockEntityPaging = {}; + mockIsInView = false; mockGetTaskCounts.mockResolvedValue({ open: 0, inProgress: 0, @@ -268,30 +279,118 @@ describe('ActivityFeedTab', () => { }); }); - describe('Bug 1 — feedFilter uses ActivityFeedTabs.MENTIONS enum', () => { - it('calls getFeedData with FeedFilter.MENTIONS when mentions tab is active', async () => { + describe('Mentions sub-tab fetches tasks the user is mentioned in', () => { + it('calls getTaskData with FeedFilter.MENTIONS and never getFeedData', async () => { renderComponent(ActivityFeedTabs.MENTIONS); - await waitFor(() => { - const calls = mockGetFeedData.mock.calls; - const mentionsCall = calls.find( - ([feedFilter]) => feedFilter === FeedFilter.MENTIONS - ); + await waitFor(() => + expect(mockGetTaskData).toHaveBeenCalledWith( + FeedFilter.MENTIONS, + undefined, + EntityType.TABLE, + 'test.db.table', + 'open' + ) + ); - expect(mentionsCall).toBeDefined(); - }); + // The mentions list renders off provider `tasks`, so routing it through + // getFeedData (which writes entityThread) left the previous My Tasks + // list on screen. + expect(mockGetFeedData).not.toHaveBeenCalled(); + }); + + it('renders the task list, not the feed list, on the mentions sub-tab', async () => { + renderComponent(ActivityFeedTabs.MENTIONS); + + await waitFor(() => + expect(screen.getByTestId('task-list')).toBeInTheDocument() + ); + + expect(screen.queryByTestId('feed-list')).not.toBeInTheDocument(); + expect(screen.getByText('message.no-mentions')).toBeInTheDocument(); }); - it('does not call getFeedData with FeedFilter.MENTIONS when tasks tab is active', async () => { + it('does not pass FeedFilter.MENTIONS on the my-tasks sub-tab', async () => { renderComponent(ActivityFeedTabs.TASKS); await waitFor(() => expect(mockGetTaskData).toHaveBeenCalled()); - const mentionsCall = mockGetFeedData.mock.calls.find( - ([feedFilter]) => feedFilter === FeedFilter.MENTIONS + expect( + mockGetTaskData.mock.calls.find( + ([feedFilter]) => feedFilter === FeedFilter.MENTIONS + ) + ).toBeUndefined(); + expect( + mockGetFeedData.mock.calls.find( + ([feedFilter]) => feedFilter === FeedFilter.MENTIONS + ) + ).toBeUndefined(); + }); + }); + + describe('Sub-tab changes show the loader, never a stale list', () => { + it('keeps the in-list loader on for a first-page refetch', async () => { + mockLoading = true; + mockTasks = [{ id: 'stale-my-task' }]; + + renderComponent(ActivityFeedTabs.TASKS); + + const taskList = await screen.findByTestId('task-list'); + + expect(taskList).toHaveAttribute('data-loading', 'true'); + + // onAfterClose refetches the first page, which replaces the list. Clearing + // isFirstLoad here dropped the loader and showed the outgoing list instead. + fireEvent.click(screen.getByTestId('task-after-close')); + + await waitFor(() => + expect(screen.getByTestId('task-list')).toHaveAttribute( + 'data-loading', + 'true' + ) ); + }); + + it('brings the loader back when the sub-tab changes via the URL', async () => { + mockTasks = [{ id: 'stale-my-task' }]; + // A scrolled-to-bottom list with a cursor pages in, which is the one path + // that legitimately turns the in-list loader off. + mockEntityPaging = { after: 'cursor-1' }; + mockIsInView = true; + + const { rerender } = renderComponent(ActivityFeedTabs.TASKS); - expect(mentionsCall).toBeUndefined(); + await waitFor(() => + expect(mockGetTaskData).toHaveBeenCalledWith( + undefined, + 'cursor-1', + EntityType.TABLE, + 'test.db.table', + 'open' + ) + ); + + mockLoading = true; + // Entity pages never pass the `subTab` prop — the switch arrives as a URL + // param, which is why keying the loader reset off `subTab` was a no-op and + // left the previous sub-tab's list on screen. + mockUseRequiredParams.mockReturnValue({ + tab: 'activity_feed', + subTab: ActivityFeedTabs.MENTIONS, + }); + + rerender( + + + + ); + + await waitFor(() => + expect(screen.getByTestId('task-list')).toHaveAttribute( + 'data-loading', + 'true' + ) + ); }); }); @@ -373,5 +472,24 @@ describe('ActivityFeedTab', () => { ).toBeInTheDocument(); }); }); + + it('fires exactly one fetch per task filter change', async () => { + renderComponent(ActivityFeedTabs.TASKS); + + await waitFor(() => expect(mockGetTaskData).toHaveBeenCalled()); + + fireEvent.click(screen.getByTestId('user-profile-page-task-filter-icon')); + fireEvent.click(await screen.findByTestId('closed-tasks')); + + // The fetch effect already refires on taskFilter, so the handler calling + // getTaskData itself as well fired two identical requests per click. + await waitFor(() => + expect( + mockGetTaskData.mock.calls.filter( + ([, , , , statusGroup]) => statusGroup === 'closed' + ) + ).toHaveLength(1) + ); + }); }); }); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx index 0f802a50700c..45a3b02c8809 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx @@ -181,25 +181,41 @@ export const ActivityFeedTab = ({ () => activeTab === ActivityFeedTabs.MENTIONS, [activeTab] ); + + // Both sub-tabs of the Tasks pane render TaskListV1 off `tasks`, so both have + // to fetch through getTaskData. Keeping the render branch and the fetch branch + // on a single predicate is what stops them drifting apart again. + const isTaskListTab = useMemo( + () => isTaskActiveTab || isMentionTabSelected, + [isTaskActiveTab, isMentionTabSelected] + ); + + // `subTab` is only supplied by the user profile page; entity pages leave it + // undefined and the sub-tab arrives as a URL param read into `activeTab`. + // Keying off the values that identify the query makes a URL/back-button driven + // switch show the loader instead of the previous query's list. useEffect(() => { setIsFirstLoad(true); - }, [subTab]); + }, [activeTab, taskFilter]); - const handleTabChange = (subTab: string) => { - setIsFirstLoad(true); - navigate( - entityUtilClassBase.getEntityLink( - entityType, - fqn, - EntityTabs.ACTIVITY_FEED, - subTab - ), - { replace: true } - ); - setActiveThread(); - setActiveTask(); - setIsFullWidth(false); - }; + const handleTabChange = useCallback( + (subTab: string) => { + setIsFirstLoad(true); + navigate( + entityUtilClassBase.getEntityLink( + entityType, + fqn, + EntityTabs.ACTIVITY_FEED, + subTab + ), + { replace: true } + ); + setActiveThread(); + setActiveTask(); + setIsFullWidth(false); + }, + [entityType, fqn, navigate, setActiveThread, setActiveTask] + ); const placeholderText = useMemo(() => { if (activeTab === ActivityFeedTabs.ALL) { @@ -352,8 +368,13 @@ export const ActivityFeedTab = ({ const handleFeedFetchFromFeedList = useCallback( (after?: string) => { - setIsFirstLoad(false); - if (isTaskActiveTab) { + // Only a "load more" page keeps the current list on screen. A first-page + // refetch replaces the list, so the in-list loader has to stay enabled or + // the cleared list would flash the empty placeholder. + if (after) { + setIsFirstLoad(false); + } + if (isTaskListTab) { getTaskData(feedFilter, after, entityType, fqn, taskFilter); } else { getFeedData( @@ -367,7 +388,7 @@ export const ActivityFeedTab = ({ } }, [ - isTaskActiveTab, + isTaskListTab, feedFilter, entityType, fqn, @@ -380,7 +401,7 @@ export const ActivityFeedTab = ({ useEffect(() => { if (fqn) { - if (isTaskActiveTab) { + if (isTaskListTab) { getTaskData(feedFilter, undefined, entityType, fqn, taskFilter); } else { getFeedData( @@ -402,12 +423,12 @@ export const ActivityFeedTab = ({ taskFilter, getFeedData, getTaskData, - isTaskActiveTab, + isTaskListTab, ]); useEffect(() => { // Activity events only render on the ALL tab; skip the fetch on Tasks/Mentions. - if (isTaskActiveTab || isMentionTabSelected) { + if (isTaskListTab) { return; } if (fqn && entityType && !isUserEntity) { @@ -420,8 +441,7 @@ export const ActivityFeedTab = ({ entityType, isUserEntity, userId, - isTaskActiveTab, - isMentionTabSelected, + isTaskListTab, fetchEntityActivity, fetchUserActivity, ]); @@ -461,7 +481,7 @@ export const ActivityFeedTab = ({ const handleFeedClick = useCallback( (feed: Thread) => { - if (!feed && (isTaskActiveTab || isMentionTabSelected)) { + if (!feed && isTaskListTab) { setIsFullWidth(false); } if (selectedThread?.id !== feed?.id) { @@ -471,25 +491,19 @@ export const ActivityFeedTab = ({ setActiveActivity(undefined); } }, - [ - setActiveThread, - setActiveActivity, - isTaskActiveTab, - isMentionTabSelected, - selectedThread, - ] + [setActiveThread, setActiveActivity, isTaskListTab, selectedThread] ); const handleTaskClick = useCallback( (task: Task) => { - if (!task && isTaskActiveTab) { + if (!task && isTaskListTab) { setIsFullWidth(false); } if (selectedTask?.id !== task?.id) { setActiveTask(task); } }, - [setActiveTask, isTaskActiveTab, selectedTask] + [setActiveTask, isTaskListTab, selectedTask] ); const handleActivityClick = useCallback( @@ -513,15 +527,16 @@ export const ActivityFeedTab = ({ [loading] ); - const handleUpdateTaskFilter = (filter: TaskStatusGroup) => { + // The fetch effect above already refires on `taskFilter`; calling getTaskData + // here as well fired two identical requests per filter click. + const handleUpdateTaskFilter = useCallback((filter: TaskStatusGroup) => { setTaskFilter(filter); - getTaskData(feedFilter, undefined, entityType, fqn, filter); - }; + }, []); - const handleAfterTaskClose = () => { + const handleAfterTaskClose = useCallback(() => { handleFeedFetchFromFeedList(); fetchFeedsCount(); - }; + }, [handleFeedFetchFromFeedList, fetchFeedsCount]); const taskFilterOptions = useMemo( () => [ { @@ -618,7 +633,7 @@ export const ActivityFeedTab = ({ options={[ { label: ( - + {t('label.my-task-plural')} @@ -627,7 +642,7 @@ export const ActivityFeedTab = ({ }, { label: ( - + {t('label.mention-plural')} @@ -641,12 +656,12 @@ export const ActivityFeedTab = ({ ); }, [t, handleTabChange]); - const handlePanelResize = (isFullWidth: boolean) => { + const handlePanelResize = useCallback((isFullWidth: boolean) => { setIsFullWidth(isFullWidth); - }; + }, []); const getRightPanelContent = () => { - if ((isTaskActiveTab || isMentionTabSelected) && selectedTask) { + if (isTaskListTab && selectedTask) { return (
{entityType === EntityType.TABLE ? ( @@ -811,7 +826,7 @@ export const ActivityFeedTab = ({ layoutType === ActivityFeedLayoutType.THREE_PANEL, })} id="center-container"> - {(isTaskActiveTab || isMentionTabSelected) && ( + {isTaskListTab && (
)} - {isTaskActiveTab || isMentionTabSelected ? ( + {isTaskListTab ? ( )} {!isFirstLoad && loader} - {!isEmpty( - isTaskActiveTab || isMentionTabSelected ? tasks : entityThread - ) && - !loading && ( -
} - style={{ height: '2px' }} - /> - )} + {!isEmpty(isTaskListTab ? tasks : entityThread) && !loading && ( +
} + style={{ height: '2px' }} + /> + )}
{layoutType === ActivityFeedLayoutType.THREE_PANEL && ( diff --git a/openmetadata-ui/src/main/resources/ui/src/components/DataQuality/IncidentManager/TestCaseIncidentTab/TestCaseIncidentTab.component.tsx b/openmetadata-ui/src/main/resources/ui/src/components/DataQuality/IncidentManager/TestCaseIncidentTab/TestCaseIncidentTab.component.tsx index d060473a90e2..e8fbb329cb59 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/DataQuality/IncidentManager/TestCaseIncidentTab/TestCaseIncidentTab.component.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/DataQuality/IncidentManager/TestCaseIncidentTab/TestCaseIncidentTab.component.tsx @@ -13,6 +13,7 @@ import { Typography } from 'antd'; import classNames from 'classnames'; +import { isEmpty } from 'lodash'; import { lazy, RefObject, @@ -175,12 +176,12 @@ const TestCaseIncidentTab = () => { - {loader} + {!isEmpty(tasks) && loader}
{ return

{t(CHILDREN_LABEL)}

; }; +/** + * Exposes the task list and the paging cursor so a test can observe what the + * provider holds *between* a filter switch and the response landing. + */ +export const DummyTaskListStateComponent = () => { + const { getTaskData, tasks, entityPaging } = useActivityFeedProvider(); + + const fetchTasks = (statusGroup: TaskStatusGroup) => { + getTaskData( + FeedFilter.OWNER_OR_FOLLOWS, + undefined, + EntityType.TABLE, + 'db.schema.tbl', + statusGroup + ); + }; + + return ( +
+
+ ); +}; + export const DummyChildrenDeletePostComponent = () => { const { t } = useTranslation(); const { deleteFeed } = useActivityFeedProvider(); From 46f75e095c7218c1ccc86e68eb55d6d8c59cc92b Mon Sep 17 00:00:00 2001 From: Aniket Katkar Date: Fri, 21 Aug 2026 11:49:58 +0530 Subject: [PATCH 2/2] Address review: restore the loader on every first-page refetch, quote mention hashes Two P1 review findings, both real. `if (after) setIsFirstLoad(false)` only avoided clearing the flag; it never set it back. Once pagination had cleared it, a first-page refetch -- closing a task, or any change to entity/domain -- left `isFirstLoad && loading` false while the provider had already emptied the list, so TaskListV1/ActivityFeedListV1New rendered the empty-state placeholder next to the pagination spinner. Use `setIsFirstLoad(!after)`, and move the reset into the fetch effect itself so it also covers the fqn and activeDomain deps rather than only sub-tab and filter. This is the same defect Gitar reported from the entityThread side; ActivityFeedTab is the only consumer that renders entityThread as a list, so it is fully covered. ListFilter now hashes via hashUserName, which quotes before hashing. Verified against live rows: a dotted user's FQN is stored quoted, and storeMentions writes md5 of the quoted single segment. Hashing the raw value matched the quoted FQN the UI sends but would split a bare `john.doe` into three FQN segments and match nothing; quoteName is idempotent for an already-quoted name, so quoting first accepts both. Also matches the sibling assignee condition. The loader unit test now paginates first, so it fails without the fix rather than starting from the already-true state. Added an IT assertion covering the quoted FQN form alongside the bare name. Co-Authored-By: Claude Opus 5 (1M context) --- .../openmetadata/it/tests/TaskCommentsIT.java | 12 +++++++ .../service/jdbi3/ListFilter.java | 10 +++--- .../ActivityFeedTab.component.test.tsx | 31 +++++++++++++++---- .../ActivityFeedTab.component.tsx | 22 ++++++------- 4 files changed, 52 insertions(+), 23 deletions(-) diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TaskCommentsIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TaskCommentsIT.java index ddcede90bbfd..d996f0a7ae22 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TaskCommentsIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TaskCommentsIT.java @@ -353,6 +353,18 @@ void test_listByMentionedUser_returnsTaskFromCommentMention() { assertFalse( mentionedIds.contains(unrelated.getId()), "mentionedUser filter must exclude tasks that do not mention the user"); + + // The UI sends the FQN, which is quoted for a dotted username, but a bare + // name has to resolve to the same mention rows or the filter silently + // returns nothing for those users. + ListParams byFqn = + new ListParams() + .addFilter("mentionedUser", shared.USER2.getFullyQualifiedName()) + .setLimit(1000); + assertTrue( + adminClient.tasks().list(byFqn).getData().stream() + .anyMatch(t -> t.getId().equals(mentioning.getId())), + "mentionedUser must match on the quoted FQN as well as the bare name"); } finally { adminClient .tasks() diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ListFilter.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ListFilter.java index 491752dde953..77acbd80351b 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ListFilter.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ListFilter.java @@ -284,10 +284,12 @@ private String getMentionedUserCondition() { return ""; } // TaskRepository.storeMentions writes the task id into toFQN and the mentioned - // user into fromFQNHash (via @BindFQN, i.e. FullyQualifiedName.buildHash of the - // raw value). field_relationship has no toId column, so selecting one made every - // mentionedUser query fail with an SQLSyntaxErrorException. - queryParams.put("mentionedUserHash", FullyQualifiedName.buildHash(mentionedUser)); + // user into fromFQNHash (via @BindFQN). field_relationship has no toId column, so + // selecting one made every mentionedUser query fail with an SQLSyntaxErrorException. + // hashUserName quotes first, so a dotted name matches whether the caller sends the + // quoted FQN ("john.doe") or the bare name (john.doe) — bare would otherwise hash + // as three FQN segments and match nothing. + queryParams.put("mentionedUserHash", hashUserName(mentionedUser)); return String.format( "(id IN (SELECT fr.toFQN FROM field_relationship fr " + "WHERE fr.fromFQNHash = :mentionedUserHash " diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.test.tsx index 59b7b45a38ac..aeeaba198005 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.test.tsx @@ -329,18 +329,37 @@ describe('ActivityFeedTab', () => { }); describe('Sub-tab changes show the loader, never a stale list', () => { - it('keeps the in-list loader on for a first-page refetch', async () => { - mockLoading = true; + it('switches the in-list loader back on for a first-page refetch after paginating', async () => { mockTasks = [{ id: 'stale-my-task' }]; + // Scrolled to the bottom with a cursor: the one path that legitimately + // turns the in-list loader off. + mockEntityPaging = { after: 'cursor-1' }; + mockIsInView = true; renderComponent(ActivityFeedTabs.TASKS); - const taskList = await screen.findByTestId('task-list'); + await waitFor(() => + expect(mockGetTaskData).toHaveBeenCalledWith( + undefined, + 'cursor-1', + EntityType.TABLE, + 'test.db.table', + 'open' + ) + ); - expect(taskList).toHaveAttribute('data-loading', 'true'); + await waitFor(() => + expect(screen.getByTestId('task-list')).toHaveAttribute( + 'data-loading', + 'false' + ) + ); - // onAfterClose refetches the first page, which replaces the list. Clearing - // isFirstLoad here dropped the loader and showed the outgoing list instead. + // onAfterClose refetches the first page, and the provider clears `tasks` + // for it. Once pagination has cleared isFirstLoad, only switching it back + // on keeps the loader up — otherwise the emptied list renders the + // "no tasks" placeholder next to the pagination spinner. + mockLoading = true; fireEvent.click(screen.getByTestId('task-after-close')); await waitFor(() => diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx index 45a3b02c8809..4cbf5176057c 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx @@ -190,14 +190,6 @@ export const ActivityFeedTab = ({ [isTaskActiveTab, isMentionTabSelected] ); - // `subTab` is only supplied by the user profile page; entity pages leave it - // undefined and the sub-tab arrives as a URL param read into `activeTab`. - // Keying off the values that identify the query makes a URL/back-button driven - // switch show the loader instead of the previous query's list. - useEffect(() => { - setIsFirstLoad(true); - }, [activeTab, taskFilter]); - const handleTabChange = useCallback( (subTab: string) => { setIsFirstLoad(true); @@ -369,11 +361,10 @@ export const ActivityFeedTab = ({ const handleFeedFetchFromFeedList = useCallback( (after?: string) => { // Only a "load more" page keeps the current list on screen. A first-page - // refetch replaces the list, so the in-list loader has to stay enabled or - // the cleared list would flash the empty placeholder. - if (after) { - setIsFirstLoad(false); - } + // refetch replaces it, so the in-list loader has to be switched back ON — + // once pagination has cleared this flag, `isFirstLoad && loading` is false + // and the cleared list renders the empty placeholder next to the spinner. + setIsFirstLoad(!after); if (isTaskListTab) { getTaskData(feedFilter, after, entityType, fqn, taskFilter); } else { @@ -401,6 +392,11 @@ export const ActivityFeedTab = ({ useEffect(() => { if (fqn) { + // Every dep here identifies a different query, so this is always a + // first-page fetch that replaces the list — sub-tab (via feedFilter), task + // filter, entity or domain. The loader has to be on for the window where + // the provider has cleared the rows but the response has not landed. + setIsFirstLoad(true); if (isTaskListTab) { getTaskData(feedFilter, undefined, entityType, fqn, taskFilter); } else {