Added pagination support for document listing page and added playwright test for all the cases - #29551
Conversation
Rohit0301
added
the
safe to test
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🟡 Playwright Results — all passed (28 flaky)✅ 4492 passed · ❌ 0 failed · 🟡 28 flaky · ⏭️ 37 skipped
🟡 28 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
5e834a5 to
473e0ae
Compare
4e7bd0c to
c5014dd
Compare
473e0ae to
082685b
Compare
…bi3/FolderRepository.java Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…ContextCenterDocumentPage.spec.ts Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…lter Add folder childrenCount cases to EntityRepositoryBulkFieldsTest: childrenCount resolves in one batched scan per page (not N+1), absent folders default to 0, the query uses folder->CONTAINS->contextFile, and an empty page issues no query. Add ListFilter.getFolderCondition() cases for the parameterized CONTAINS subquery and that folderId is bound, never inlined.
Code Review ✅ Approved 9 resolved / 9 findingsImplements infinite scroll pagination for the document listing page and adds comprehensive Playwright coverage. All previously identified functional issues, including stale counts, duplicate fetches, and folder filtering inconsistencies, have been resolved. ✅ 9 resolved✅ Bug: totalFileCount drifts when deleting/searching (stale count)
✅ Bug: Rapid scroll can double-fetch same page (duplicate rows)
✅ Quality: ListHeader count shows loaded rows, not the true total
✅ Edge Case: Folder filtering is incomplete with paginated loading
✅ Bug: handleFileMoved not updated for null (move-to-root) target
...and 4 more resolved from earlier reviews OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
|
Code Review ✅ Approved 9 resolved / 9 findingsImplements infinite scroll pagination for the document listing page and adds comprehensive Playwright coverage. All previously identified functional issues, including stale counts, duplicate fetches, and folder filtering inconsistencies, have been resolved. ✅ 9 resolved✅ Bug: totalFileCount drifts when deleting/searching (stale count)
✅ Bug: Rapid scroll can double-fetch same page (duplicate rows)
✅ Quality: ListHeader count shows loaded rows, not the true total
✅ Edge Case: Folder filtering is incomplete with paginated loading
✅ Bug: handleFileMoved not updated for null (move-to-root) target
...and 4 more resolved from earlier reviews OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
3 participants
Describe your changes:
Fixes #
I worked on ... because ...
Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
Unit tests
Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Summary by Gitar
listContextFilesviafolderIdparameter inListFilterandContextFileResource.FolderRepositoryandFolderResourceto includechildrenCountfield for folder entities.ContextCenterDocumentsPageusingusePagingandonScrollEnd.isLoadingMorestate toDocumentsViewto show skeleton rows during incremental data fetches.ContextCenterDocumentPage.spec.tscovering full CRUD, bulk operations, and pagination.DocumentFolderView.test.tsxto align with the removal of local file-list prop dependencies.This will update automatically on new commits.
Greptile Summary
This PR adds server-side cursor pagination to the Context Center documents listing page (infinite scroll with
aftercursors) and a matching folder-level file pagination in the sidebar tree (first-page-on-expand, "View more"/"Show less"). It also introduces a new, dedicated Playwright suite (ContextCenterDocumentPage.spec.ts) that covers full CRUD, bulk operations, search, and pagination flows, while removing those tests fromContextCenter.spec.ts.ListFilter.getFolderCondition()generates a parameterised subquery so the server can scope files to a folder;FolderRepository.setListFieldsresolveschildrenCountwith a single batched GROUP BY query instead of N+1 individual counts.ContextCenterDocumentsPagemanages pagination state viafetchGenerationRef(stale-response guard on folder switches) andisLoadingMoreRef(synchronous duplicate-fetch guard on rapid scroll);DocumentFolderViewadds lazy-loading and expand/collapse of per-folder file lists with their own cursor chain.FolderRepositorybatch query path andListFilterSQL-injection safety are added on the backend.Confidence Score: 4/5
The backend pagination and folder-filter logic are solid, but two open issues from prior review rounds are still present in this revision and have not been addressed.
The backend changes (batch count queries, parameterised folder subquery, generation-guard on stale responses, synchronous ref guard against duplicate scroll fetches) are all well-implemented and tested. What keeps this from being fully merge-ready are two unfixed issues carried forward from previous rounds: (1) a Playwright test opens a new browser tab via
browser.newPage()rather thanpage.context().newPage(), meaning it runs in a fresh unauthenticated context that will redirect to the login page in CI; and (2) switching folders does not clearselectedIds, so documents selected in one folder context silently remain selected and can be bulk-deleted after a folder switch without any visual indication.ContextCenterDocumentPage.spec.ts(unauthenticated new-tab in the copy-link test) andContextCenterDocumentsPage.tsx(selection state not cleared on folder switch).Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant U as User (Browser) participant DV as DocumentsView participant DFV as DocumentFolderView participant Page as ContextCenterDocumentsPage participant API as REST API Note over U,API: Initial Load Page->>API: "GET /drive/files?limit=15" API-->>Page: "{ data, paging.after, paging.total }" Page->>DV: "data=page1, totalFileCount=N" Note over U,API: Infinite Scroll (main list) U->>DV: scroll near bottom DV->>Page: onScrollEnd() Page->>Page: "isLoadingMoreRef.current=true" Page->>API: "GET /drive/files?after=cursor&limit=15" API-->>Page: "{ data: page2, paging.after }" Page->>DV: "data=[...page1, ...page2]" Note over U,API: Folder Filter (server-side) U->>DFV: click folder row DFV->>Page: onSelectFolder(folderId) Page->>Page: fetchGenerationRef.current++ Page->>API: "GET /drive/files?folderId=X&limit=15" API-->>Page: "{ data: folderPage1 }" Page->>DV: "data=folderPage1" Note over U,API: Folder Sidebar Expand U->>DFV: expand folder node DFV->>API: "GET /drive/files?folderId=X&limit=10" API-->>DFV: "{ data: first10, paging.after }" DFV->>U: show first 10 + View more button Note over U,API: View More in Sidebar U->>DFV: click View more DFV->>API: "GET /drive/files?folderId=X&after=cursor&limit=10" API-->>DFV: "{ data: next10, paging.after: null }" DFV->>U: show all 20 + Show Less button%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant U as User (Browser) participant DV as DocumentsView participant DFV as DocumentFolderView participant Page as ContextCenterDocumentsPage participant API as REST API Note over U,API: Initial Load Page->>API: "GET /drive/files?limit=15" API-->>Page: "{ data, paging.after, paging.total }" Page->>DV: "data=page1, totalFileCount=N" Note over U,API: Infinite Scroll (main list) U->>DV: scroll near bottom DV->>Page: onScrollEnd() Page->>Page: "isLoadingMoreRef.current=true" Page->>API: "GET /drive/files?after=cursor&limit=15" API-->>Page: "{ data: page2, paging.after }" Page->>DV: "data=[...page1, ...page2]" Note over U,API: Folder Filter (server-side) U->>DFV: click folder row DFV->>Page: onSelectFolder(folderId) Page->>Page: fetchGenerationRef.current++ Page->>API: "GET /drive/files?folderId=X&limit=15" API-->>Page: "{ data: folderPage1 }" Page->>DV: "data=folderPage1" Note over U,API: Folder Sidebar Expand U->>DFV: expand folder node DFV->>API: "GET /drive/files?folderId=X&limit=10" API-->>DFV: "{ data: first10, paging.after }" DFV->>U: show first 10 + View more button Note over U,API: View More in Sidebar U->>DFV: click View more DFV->>API: "GET /drive/files?folderId=X&after=cursor&limit=10" API-->>DFV: "{ data: next10, paging.after: null }" DFV->>U: show all 20 + Show Less buttonComments Outside Diff (4)
openmetadata-ui/src/main/resources/ui/src/pages/ContextCenterPage/ContextCenterDocumentsPage/ContextCenterDocumentsPage.tsx, line 211-238 (link)The
?document=Xeffect compares againstallDocuments, which only contains the first page of results. If the linked document is on page 2, 3, etc., thefindwill returnundefined, the effect shows an error toast, and silently removes thedocumentparam — losing the deep link entirely. Previously all documents were loaded at once, so this always worked. The guardif (match) { ... } else { showErrorToast(...) }now fires incorrectly whenever a user shares a link to any document that isn't in the initial page load.openmetadata-ui/src/main/resources/ui/src/pages/ContextCenterPage/ContextCenterDocumentsPage/ContextCenterDocumentsPage.tsx, line 124-130 (link)documentsis computed by filtering the locally-accumulatedallDocumentsarray. Before this PR,listContextFileswas called withlimit: 100and would typically fetch all (or most) documents in one shot, making client-side folder filtering work. After this PR the limit defaults topageSize(15 viaPAGE_SIZE_BASE), so clicking a folder in the sidebar only shows documents from already-loaded pages. Any documents in that folder on later pages are silently invisible until the user has scrolled far enough to load them all.openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/DocumentsView/DocumentsView.component.tsx, line 570-582 (link)countshows loaded documents, not the real totalWith pagination,
data.lengthis the number of rows loaded so far (e.g. 15 on the first page), not the actual total stored server-side. The parent already trackstotalFileCountfromresponse.paging.total. Consider threading that value through a newtotalCountprop onDocumentsViewPropsand passing it toListHeaderso the file count is always accurate.openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ContextCenterDocumentPage.spec.ts, line 1420-1435 (link)browser.newPage()lacks authentication cookiesbrowser.newPage()opens a fresh browser context that does not share the storage state set bytest.use({ storageState: 'playwright/.auth/admin.json' }). In any environment where authentication is enforced,newTab.goto(clipboardText)will redirect to the login page andcontext-center-documents-pagewill never become visible, causing the test to time out.Use
page.context().newPage()to create the tab inside the same authenticated context as the rest of the test.openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ContextCenterDocumentPage.spec.ts, line 1647-1648 (link)browser.newPage()opens unauthenticated contextbrowser.newPage()creates a page in a brand-new browser context that does not inherit the storage state set bytest.use({ storageState: 'playwright/.auth/admin.json' }). In any CI or remote environment where authentication is enforced,newTab.goto(clipboardText)will land on the login page,context-center-documents-pagewill never become visible, and the test will time out. Usepage.context().newPage()so the new tab shares the same authenticated browser context as the rest of the test.openmetadata-ui/src/main/resources/ui/src/pages/ContextCenterPage/ContextCenterDocumentsPage/ContextCenterDocumentsPage.tsx, line 383-394 (link)When the user selects documents and then switches folders (or deselects the current folder),
selectedIdsis never cleared. BecausefetchDocumentsnow replacesallDocumentswith the server-side page for the new folder, the previously selected IDs disappear from view but stay inselectedIds. The bulk action bar then shows a non-zero count of "invisible" documents. Clicking the bulk-delete button at that point firesbulkDeleteDriveFiles(Array.from(selectedIds), false)and permanently archives those documents — even though none of them were visible in the current folder context.A
setSelectedIds(new Set())call should be issued alongsidesetSelectedFolderId(or insidefetchDocumentson every non-afterreset) to ensure the selection is scoped to the currently-visible list.Reviews (26): Last reviewed commit: "Merge branch 'main' into document-page-p..." | Re-trigger Greptile