-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: Fixed service details page pagination issue #25134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
...adata-ui/src/main/resources/ui/src/components/Database/SchemaTable/SchemaTable.component.tsx
Outdated
Show resolved
Hide resolved
🔍 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 FailingShard 6/6 - Actual Failures2 Hard Failures (POTENTIALLY RELATED TO PR)
Status: STILL FAILING Analysis: Why These May Be RelatedUser Delete/Restore TestsThese tests validate:
The concern: The user list pagination was refactored in this PR (commit Potential IssuesHypothesis 1: Pagination doesn't refresh after delete
Hypothesis 2: Pagination doesn't refresh after restore
Hypothesis 3: Delete/restore from profile page
Comparison with Previous RunsConsistency PatternJob 59921419356 (previous run): Pattern: Same test consistently failing across runs What This MeansThe fix in commit
But did NOT fix:
Root Cause Investigation NeededQuestions to Answer
Impact AssessmentSeverity: MEDIUM-HIGHIf these are real bugs:
If these are test issues:
User Experience ImpactScenario 1: Admin deletes a user
Scenario 2: Admin restores a user
Updated AssessmentPagination Tests StatusFully Passing: ✅
Still Failing: ❌
Overall StatusPass Rate: (459 + 423) / (472 + 430) = 882/902 = 97.8% Hard Failures: 7 total
Recommendation - REVISEDStatus: INVESTIGATION NEEDEDBefore merging, need to:
VerdictIf test expectation issues: READY TO MERGE after fix Most likely: Test expectation issues (similar to previous fixes) Code Review 👍 Approved with suggestions 14 resolved / 18 findingsSolid pagination fix with consistent pattern applied across multiple components. Two minor dependency array issues found, and previous test cleanup findings remain unresolved. Suggestions 💡 4 suggestionsBug: Missing searchValue dependency in handleSchemaPageChangeIn 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 Code Quality: Redundant searchSchema in useCallback dependencyIn
This should be safe in practice since the
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
Additionally, the new tests (API Collection, Stored Procedures, Database Schemas, Dashboard Data Models, Service Databases) create test data but don't include Impact: Test data accumulation can lead to:
Suggestion: Add 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 Unlike other test groups in this file (Database Schema Tables, API Collection Endpoints, etc.), which all have proper cleanup via Suggested fix: Add an 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 Resolved ✅ 14 resolvedBug: Unconditional page reset on mount may disrupt navigation
Bug: FilesTable useEffect triggers on mount without conditional
Bug: Debugging console.log left in production code
Bug: Reused response promise causes test to not wait for second API call
Edge Case: Missing searchValue dependency in APIEndpointsTab pagination handler
...and 9 more from earlier reviews What Works WellThe 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. RecommendationsConsider adding a shared hook or utility function to encapsulate the common pagination-with-search pattern, reducing duplication across Tip Comment OptionsAuto-apply is off Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs) |
|



Describe your changes:
Fixes
I worked on ... because ...
Screen.Recording.2026-01-08.at.2.10.22.PM.mov
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
cursorValuefrom URL params inServiceDetailsPage.tsxinstead of stale React stateactiveTabHandlerto prevent race conditionsSchemaTablenow usesuseTableFiltersto manage search state in URL withcolumnSearchparamsearchTableColumnsandfetchTableColumnsfunctions for proper search vs normal paginationDatabaseSchemaTable,DataModelsTable,APIEndpointsTab, andStoredProcedureTabtestCompletePaginationWithSearchutility validates both normal pagination and search pagination flowsThis will update automatically on new commits.