Skip to content

Conversation

@Rohit0301
Copy link
Contributor

@Rohit0301 Rohit0301 commented Jan 8, 2026

Describe your changes:

Fixes

I worked on ... because ...

Screen.Recording.2026-01-08.at.2.10.22.PM.mov
Screenshot 2026-01-08 at 2 04 57 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

  • Service Details pagination fix:
    • Uses cursorValue from URL params in ServiceDetailsPage.tsx instead of stale React state
    • Pagination reset moved to synchronous activeTabHandler to prevent race conditions
  • Search pagination fixes:
    • SchemaTable now uses useTableFilters to manage search state in URL with columnSearch param
    • Separate searchTableColumns and fetchTableColumns functions for proper search vs normal pagination
    • Pagination automatically resets to page 1 when search text changes
    • Applied same pattern to DatabaseSchemaTable, DataModelsTable, APIEndpointsTab, and StoredProcedureTab
  • Comprehensive E2E test coverage:
    • New testCompletePaginationWithSearch utility validates both normal pagination and search pagination flows
    • Tests create dedicated test data: Table with 60 columns, 20 databases, 25 API endpoints, 25 data models, 25 stored procedures
    • Validates pagination reset behavior when switching between search and normal views

This will update automatically on new commits.


@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2026

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@Rohit0301 Rohit0301 added UI UI specific issues safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch labels Jan 8, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.25% (52243/80063) 43.13% (25850/59939) 46.54% (8102/17409)

@gitar-bot
Copy link

gitar-bot bot commented Jan 10, 2026

🔍 CI failure analysis for 7d54ef4: CRITICAL UPDATE: 2 User delete/restore tests are STILL FAILING in shard 6/6. These may be related to pagination as they involve user list refresh after delete/restore operations. Needs investigation before merge.

🚨 CRITICAL UPDATE - User Tests Still Failing

Shard 6/6 - Actual Failures

2 Hard Failures (POTENTIALLY RELATED TO PR)

  1. Users.spec.ts:207 - Admin soft & hard delete and restore user
  2. Users.spec.ts:229 - Admin soft & hard delete and restore user from profile page

Status: STILL FAILING ⚠️
Concern: These involve user deletion + list refresh, which interacts with pagination


Analysis: Why These May Be Related

User Delete/Restore Tests

These tests validate:

  • Soft deleting a user
  • Hard deleting a user
  • Restoring a deleted user
  • User list updating after these operations

The concern: The user list pagination was refactored in this PR (commit 025e2ea)

Potential Issues

Hypothesis 1: Pagination doesn't refresh after delete

  • After deleting a user, the list should refresh
  • The pagination state may not trigger re-fetch
  • User may still appear in list

Hypothesis 2: Pagination doesn't refresh after restore

  • After restoring a user, the list should update
  • The pagination may show stale data
  • User may not appear where expected

Hypothesis 3: Delete/restore from profile page

  • Different code path than list page
  • May not trigger pagination refresh in parent list
  • List component may not be notified of changes

Comparison with Previous Runs

Consistency Pattern

Job 59921419356 (previous run): Users.spec.ts:207 FAILED
Job 59994076051 (current run): Users.spec.ts:207 + Users.spec.ts:229 FAILED

Pattern: Same test consistently failing across runs
New: Second similar test now also failing

What This Means

The fix in commit 7d54ef4 (making after parameter optional) fixed:

  • UsersPagination.spec.ts:63 - Soft delete pagination (test expectations)

But did NOT fix:

  • Users.spec.ts:207 - Delete/restore operations
  • Users.spec.ts:229 - Delete/restore from profile

Root Cause Investigation Needed

Questions to Answer

  1. What assertions are failing?

    • Does user disappear from list after delete?
    • Does user reappear after restore?
    • Is pagination showing correct page after operations?
  2. Is pagination refreshing?

    • After delete, is fetchUsers called again?
    • After restore, does list update?
    • Is pagination state reset or preserved?
  3. Are these test issues or code issues?

    • Previous fix was test expectations (after parameter)
    • Are these also test expectation issues?
    • Or is there a real bug in delete/restore + pagination?

Impact Assessment

Severity: MEDIUM-HIGH

If these are real bugs:

  • Users may not see deleted users removed from list
  • Users may not see restored users appear in list
  • Pagination may show stale data after mutations

If these are test issues:

  • Test expectations may need updating for new pagination behavior
  • Similar to the after parameter fix

User Experience Impact

Scenario 1: Admin deletes a user

  • Expected: User disappears from list
  • If broken: User still shows in list until page refresh

Scenario 2: Admin restores a user

  • Expected: User appears in list
  • If broken: User doesn't show until page refresh

Updated Assessment

Pagination Tests Status

Fully Passing: ✅

  • Service details pagination
  • User search + pagination
  • User soft delete filter + pagination

Still Failing: ❌

  • User delete + list refresh
  • User restore + list refresh

Overall Status

Pass Rate: (459 + 423) / (472 + 430) = 882/902 = 97.8%

Hard Failures: 7 total

  • 3 AuditLogs (unrelated)
  • 2 Glossary (unrelated)
  • 2 User delete/restore (POTENTIALLY RELATED)

Recommendation - REVISED

Status: INVESTIGATION NEEDED

Before merging, need to:

  1. Get full error logs for both failing tests:

    gh api repos/open-metadata/OpenMetadata/actions/jobs/59994076051/logs |
      grep -A 100 "Users.spec.ts:207"
  2. Check if these are real bugs or test issues:

    • Review test expectations
    • Test delete/restore locally with pagination
    • Check if list refreshes after operations
  3. If test issues: Fix test expectations (like 7d54ef4)

  4. If code bugs: Fix pagination refresh after mutations

Verdict

If test expectation issues: READY TO MERGE after fix
If real bugs: NOT READY - need code fix for delete/restore + pagination

Most likely: Test expectation issues (similar to previous fixes)
Recommended: Investigate error messages to determine which

Code Review 👍 Approved with suggestions 14 resolved / 18 findings

Solid pagination fix with consistent pattern applied across multiple components. Two minor dependency array issues found, and previous test cleanup findings remain unresolved.

Suggestions 💡 4 suggestions
Bug: Missing searchValue dependency in handleSchemaPageChange

📄 openmetadata-ui/src/main/resources/ui/src/components/Database/DatabaseSchema/DatabaseSchemaTable/DatabaseSchemaTable.tsx:190

In DatabaseSchemaTable.tsx, the handleSchemaPageChange callback is missing searchValue in its dependency array. The callback checks if (searchValue) but doesn't list it as a dependency.

const handleSchemaPageChange = useCallback(
  ({ currentPage, cursorType }: PagingHandlerParams) => {
    if (searchValue) {  // uses searchValue
      handlePageChange(currentPage);
    } else if (cursorType) {
      // ...
    }
  },
  [paging, handlePageChange, pageSize]  // missing searchValue
);

This could lead to stale closure issues where searchValue inside the callback doesn't reflect the current value. Add searchValue to the dependency array.

Code Quality: Redundant searchSchema in useCallback dependency

📄 openmetadata-ui/src/main/resources/ui/src/components/Database/DatabaseSchema/DatabaseSchemaTable/DatabaseSchemaTable.tsx:290

In DatabaseSchemaTable.tsx, the useEffect at line 293 includes searchSchema in its dependency array, but searchSchema is a useCallback that depends on pageSize (and other values). This creates a dependency cycle where:

  1. searchSchema changes when pageSize changes
  2. The useEffect re-runs due to searchSchema changing
  3. This calls searchSchema again

This should be safe in practice since the if (searchValue) guard prevents redundant calls, but it would be cleaner to either:

  • Remove searchSchema from the dependency array and use // eslint-disable-next-line react-hooks/exhaustive-deps
  • Or ensure the effect only depends on primitive values: [searchValue, currentPage, pageSize]
Code Quality: Multiple E2E tests remove cleanup in afterAll hooks

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Pagination.spec.ts:237

The PR removes multiple afterAll cleanup blocks from E2E tests:

  • Classification Tags test (lines 237-250): removed tag and classification cleanup
  • Metrics test (lines 264-277): removed metrics cleanup
  • Notification Alerts test (lines 285-295): removed alerts cleanup
  • Observability Alerts test (lines 313-325): removed alerts cleanup

Additionally, the new tests (API Collection, Stored Procedures, Database Schemas, Dashboard Data Models, Service Databases) create test data but don't include afterAll cleanup hooks.

Impact: Test data accumulation can lead to:

  1. Slower test runs over time
  2. Test pollution affecting subsequent test runs
  3. Resource exhaustion in test environments

Suggestion: Add afterAll hooks to clean up created test data, similar to how the classification tags test previously had cleanup.

Code Quality: Service Databases test creates 20 databases without cleanup

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Pagination.spec.ts:163

The "Service Databases page pagination" test group creates 20 test databases using the sample_data service in beforeAll (lines 165-182) but has no corresponding afterAll cleanup. This will leave test data in the database after tests run.

Unlike other test groups in this file (Database Schema Tables, API Collection Endpoints, etc.), which all have proper cleanup via test.afterAll blocks that delete their created entities, this test group is missing cleanup code.

Suggested fix: Add an afterAll block to delete the created databases:

test.afterAll(async ({ browser }) => {
  const { apiContext, afterAction } = await createNewPage(browser);
  // Delete all created databases by name pattern
  // Or track database IDs during creation and delete them here
  await afterAction();
});

Note: Since the databases are created with PUT /api/v1/databases and no IDs are tracked, you'll need to either track the created database names/IDs or query and delete by name pattern.

Resolved ✅ 14 resolved
Bug: Unconditional page reset on mount may disrupt navigation

📄 openmetadata-ui/src/main/resources/ui/src/components/DriveService/File/FilesTable/FilesTable.tsx:119
The added useEffect unconditionally resets pagination to page 1 on every mount:

useEffect(() => {
  paging.handlePageChange(INITIAL_PAGING_VALUE);
}, []);

This could disrupt browser back/forward navigation if the user was on page 2+ and navigates back - the component will reset to page 1 instead of maintaining their position. The URL params may still show page 2 but the component will have reset.

Same pattern exists in SpreadsheetsTable.tsx. Consider removing this effect since pagination state should already be managed through URL params and the parent component.

Bug: FilesTable useEffect triggers on mount without conditional

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Pagination.spec.ts:111
The useEffect call in FilesTable.tsx at lines 119-121 calls paging.handlePageChange(INITIAL_PAGING_VALUE) unconditionally on every mount:

useEffect(() => {
  paging.handlePageChange(INITIAL_PAGING_VALUE);
}, []);

This will reset pagination to page 1 on component mount, which could cause issues if the user navigates back to a previously paginated view expecting to maintain their position. The same pattern exists in SpreadsheetsTable.tsx.

Consider either removing this effect if pagination state should be preserved, or only resetting when switching tabs/contexts.

Bug: Debugging console.log left in production code

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/common.ts:759
There's a console.log(page1Items, page2Items) statement left in the testPaginationNavigation function at line ~732 in common.ts. While this is test code, debug logging should be removed before merging as it creates noise in test output and suggests incomplete code review.

Suggested fix: Remove the console.log statement or use a proper test logging mechanism if needed for debugging.

Bug: Reused response promise causes test to not wait for second API call

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/common.ts:710
In testCompletePaginationWithSearch, when the delete toggle is clicked twice (to toggle on then off), the code uses the same searchApiPromiseWithToggle promise for both await statements.

const searchApiPromiseWithToggle = page.waitForResponse(
  (response) => response.url().includes(searchApiPattern) && response.status() === 200
);

await deleteToggle.click();
await searchApiPromiseWithToggle;  // First click - correct
await waitForAllLoadersToDisappear(page);

await deleteToggle.click();
await searchApiPromiseWithToggle;  // Second click - reuses already resolved promise!
await waitForAllLoadersToDisappear(page);

The second await searchApiPromiseWithToggle will resolve immediately because the promise was already fulfilled by the first click. This means the test won't actually wait for the second API call to complete, which could cause flaky tests.

Suggested fix:

const searchApiPromise1 = page.waitForResponse(
  (response) => response.url().includes(searchApiPattern) && response.status() === 200
);
await deleteToggle.click();
await searchApiPromise1;
await waitForAllLoadersToDisappear(page);

const searchApiPromise2 = page.waitForResponse(
  (response) => response.url().includes(searchApiPattern) && response.status() === 200
);
await deleteToggle.click();
await searchApiPromise2;
await waitForAllLoadersToDisappear(page);
Edge Case: Missing searchValue dependency in APIEndpointsTab pagination handler

📄 openmetadata-ui/src/main/resources/ui/src/pages/APICollectionPage/APIEndpointsTab.tsx:206
The handleEndpointsPagination callback in APIEndpointsTab.tsx has searchValue in the condition check but the dependency array was changed from:

[paging, getAPICollectionEndpoints, searchAPIEndpoints, searchValue]

to:

[paging, handlePageChange, searchValue, pageSize]

While searchValue is still in the dependency array (which is correct), the callback references searchValue to decide the branching logic. If searchValue changes while the user is on page 2, the callback will correctly see the updated value due to the dependency.

However, there's a subtle issue: pageSize is now in the dependency array but wasn't before, which is actually correct since handlePageChange is called with pageSize. This is a positive change.

No action needed - upon further analysis this is actually correct.

...and 9 more from earlier reviews

What Works Well

The refactor applies a consistent, well-structured pattern for managing pagination state via URL params across all affected components. The separation of search and normal fetch functions eliminates race conditions. Comprehensive E2E test coverage validates both normal pagination and search pagination flows.

Recommendations

Consider adding a shared hook or utility function to encapsulate the common pagination-with-search pattern, reducing duplication across DataModelsTable, DatabaseSchemaTable, SchemaTablesTab, APIEndpointsTab, and StoredProcedureTab.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off Gitar will not commit updates to this branch.
Display: compact Hiding non-applicable rules.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs)

@sonarqubecloud
Copy link

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 To release Will cherry-pick this PR into the release branch UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants