This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-6306-1b4255827a08fc5904e092205b7163c45c272989 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 45c82a59a3efc3501e5b9edcb5c5cc7b9d6b02b1 Author: Kunwoo (Chris) <[email protected]> AuthorDate: Fri Jul 10 01:17:26 2026 -0400 fix(frontend): speed up dataset bulk upload with many files (#6306) ### What changes were proposed in this PR? Bulk-uploading many files to a dataset was extremely slow (~1,100 files took ~12 minutes) because the Pending and Finished lists fully re-rendered on every change-detection pass, on the same thread driving the uploads (root cause detailed in #5586). As proposed in the issue: replace the `queuedFileNames` getter with a cached field refreshed only when the queue changes, virtualize the Pending and Finished lists with `cdk-virtual-scroll` so only visible rows are in the DOM, and add `trackBy` to both lists. ### Any related issues, documentation, discussions? Closes #5586. ### How was this PR tested? Screen recording of a successful 1,100-file upload with the fix: https://github.com/user-attachments/assets/c23d7612-0317-4d18-970b-c98540005eed ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code, Claude Fable 5 --- .../dataset-detail.component.html | 30 +- .../dataset-detail.component.scss | 22 +- .../dataset-detail.component.spec.ts | 336 +++++++++++++++++++++ .../dataset-detail.component.ts | 113 +++++-- ...user-dataset-staged-objects-list.component.html | 78 +++-- ...user-dataset-staged-objects-list.component.scss | 17 +- ...r-dataset-staged-objects-list.component.spec.ts | 159 ++++++++++ .../user-dataset-staged-objects-list.component.ts | 42 ++- frontend/src/jsdom-svg-polyfill.ts | 9 + 9 files changed, 732 insertions(+), 74 deletions(-) diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.html b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.html index 4369cd76ba..3c2a031617 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.html +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.html @@ -349,10 +349,26 @@ class="upload-status-panels"> <nz-collapse-panel *ngIf="queuedCount > 0" - [nzHeader]="'Pending: ' + queuedCount + ' file(s)'"> - <div class="upload-progress-wrapper-pending"> - <div *ngFor="let fileName of queuedFileNames"> - <span>{{ fileName }}</span> + [nzHeader]="'Pending: ' + queuedCount + ' file(s)'" + (nzActiveChange)="onPendingPanelActiveChange($event)"> + <!-- Virtualized so only visible rows are in the DOM (#5586). The viewport + measures height 0 while the panel is collapsed, so it is re-measured + on expand. --> + <cdk-virtual-scroll-viewport + class="upload-progress-wrapper-pending" + [itemSize]="PENDING_ROW_HEIGHT_PX" + [minBufferPx]="PENDING_LIST_MAX_HEIGHT_PX" + [maxBufferPx]="2 * PENDING_LIST_MAX_HEIGHT_PX" + [style.height.px]="pendingListHeightPx"> + <div + class="pending-file-row" + *cdkVirtualFor="let fileName of queuedFileNames; trackBy: trackByPendingFile"> + <span + class="pending-file-name" + nz-tooltip + [nzTooltipTitle]="fileName" + >{{ fileName }}</span + > <button nz-button nzType="text" @@ -366,7 +382,7 @@ nzTheme="outline"></span> </button> </div> - </div> + </cdk-virtual-scroll-viewport> </nz-collapse-panel> <nz-divider @@ -425,8 +441,10 @@ <nz-collapse-panel *ngIf="hasAnyActivity" - [nzHeader]="'Finished: ' + pendingChangesCount + ' file(s)'"> + [nzHeader]="'Finished: ' + pendingChangesCount + ' file(s)'" + (nzActiveChange)="$event && stagedObjectsList.remeasureViewport()"> <texera-dataset-staged-objects-list + #stagedObjectsList [uploadTimeMap]="uploadTimeMap" [did]="did" [userMakeChangesEvent]="userMakeChanges" diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.scss b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.scss index ac8b60b245..d5ef708f88 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.scss +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.scss @@ -143,17 +143,29 @@ nz-select { margin-top: 15%; } -.upload-progress-wrapper, -.upload-progress-wrapper-pending { +.upload-progress-wrapper { max-height: 25vh; overflow-y: auto; padding-right: 4px; } +// Rows must stay exactly PENDING_ROW_HEIGHT_PX tall for the fixed-size scroll +// strategy. The gutter lives on the rows: padding on the viewport never +// reaches its absolutely positioned content wrapper. .upload-progress-wrapper-pending { - display: flex; - flex-direction: column; - max-height: 15vh; + .pending-file-row { + height: 32px; + display: flex; + align-items: center; + justify-content: space-between; + padding-right: 4px; + + .pending-file-name { + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; + } + } } .version-creator { diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts new file mode 100644 index 0000000000..a634473194 --- /dev/null +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts @@ -0,0 +1,336 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { ActivatedRoute } from "@angular/router"; +import { of, Subject } from "rxjs"; +import { NzModalService } from "ng-zorro-antd/modal"; +import { MarkdownService } from "ngx-markdown"; +import { DatasetDetailComponent } from "./dataset-detail.component"; +import { DatasetService, MultipartUploadProgress } from "../../../../service/user/dataset/dataset.service"; +import { NotificationService } from "../../../../../common/service/notification/notification.service"; +import { DownloadService } from "../../../../service/user/download/download.service"; +import { UserService } from "../../../../../common/service/user/user.service"; +import { MOCK_USER, StubUserService } from "../../../../../common/service/user/stub-user.service"; +import { HubService } from "../../../../../hub/service/hub.service"; +import { AdminSettingsService } from "../../../../service/admin/settings/admin-settings.service"; +import { FileUploadItem } from "../../../../type/dashboard-file.interface"; +import { DatasetFileNode } from "../../../../../common/type/datasetVersionFileTree"; +import { DatasetStagedObject } from "../../../../../common/type/dataset-staged-object"; +import { commonTestImports, commonTestProviders } from "../../../../../common/testing/test-utils"; + +describe("DatasetDetailComponent upload queue", () => { + let fixture: ComponentFixture<DatasetDetailComponent>; + let component: DatasetDetailComponent; + let uploadSubjects: Subject<MultipartUploadProgress>[]; + let uploadedPaths: string[]; + let multipartUploadSpy: ReturnType<typeof vi.fn>; + + const makeFileItem = (name: string): FileUploadItem => ({ + file: new File(["x"], name), + name, + description: "", + uploadProgress: 0, + isUploadingFlag: false, + restart: false, + }); + + const dropFiles = (...names: string[]) => component.onNewUploadFilesChanged(names.map(makeFileItem)); + + const finishUpload = (index: number, filePath: string, totalTime = 1) => + uploadSubjects[index].next({ filePath, percentage: 100, status: "finished", totalTime }); + + beforeEach(() => { + uploadSubjects = []; + uploadedPaths = []; + multipartUploadSpy = vi.fn((_ownerEmail: string, _datasetName: string, filePath: string) => { + const progress = new Subject<MultipartUploadProgress>(); + uploadSubjects.push(progress); + uploadedPaths.push(filePath); + return progress.asObservable(); + }); + + TestBed.configureTestingModule({ + imports: [DatasetDetailComponent, ...commonTestImports], + providers: [ + { provide: ActivatedRoute, useValue: { params: of({ did: 1 }), data: of({}) } }, + { provide: NzModalService, useValue: {} }, + { + provide: DatasetService, + useValue: { + multipartUpload: multipartUploadSpy, + finalizeMultipartUpload: vi.fn(() => of({})), + getDataset: vi.fn(() => + of({ + dataset: { name: "test-dataset", description: "", isPublic: false, isDownloadable: true }, + accessPrivilege: "WRITE", + ownerEmail: "[email protected]", + isOwner: true, + }) + ), + retrieveDatasetVersionList: vi.fn(() => of([])), + getDatasetDiff: vi.fn(() => of([])), + createDatasetVersion: vi.fn(() => of({})), + deleteDatasetFile: vi.fn(() => of({})), + }, + }, + { provide: NotificationService, useValue: { success: vi.fn(), error: vi.fn(), info: vi.fn() } }, + { provide: DownloadService, useValue: {} }, + { provide: UserService, useClass: StubUserService }, + { + provide: HubService, + useValue: { + getCounts: vi.fn(() => of([{ counts: { like: 0 } }])), + postView: vi.fn(() => of(0)), + isLiked: vi.fn(() => of([{ isLiked: false }])), + }, + }, + { provide: AdminSettingsService, useValue: { getSetting: vi.fn(() => of("3")) } }, + { provide: MarkdownService, useValue: { parse: vi.fn(() => "") } }, + ...commonTestProviders, + ], + }); + + fixture = TestBed.createComponent(DatasetDetailComponent); + component = fixture.componentInstance; + // Log in so ngOnInit reaches loadUploadSettings (maxConcurrentFiles = 3). + (TestBed.inject(UserService) as unknown as StubUserService).userChangeSubject.next(MOCK_USER); + fixture.detectChanges(); + }); + + it("starts at most maxConcurrentFiles uploads immediately and queues the rest", () => { + dropFiles("f1.txt", "f2.txt", "f3.txt", "f4.txt", "f5.txt"); + + expect(multipartUploadSpy).toHaveBeenCalledTimes(3); + expect(uploadedPaths).toEqual(["f1.txt", "f2.txt", "f3.txt"]); + expect(component.activeCount).toBe(3); + expect(component.queuedCount).toBe(2); + expect(component.queuedFileNames).toEqual(["f4.txt", "f5.txt"]); + }); + + it("does nothing when an empty file list is dropped", () => { + dropFiles(); + + expect(multipartUploadSpy).not.toHaveBeenCalled(); + expect(component.activeCount).toBe(0); + expect(component.queuedCount).toBe(0); + expect(component.queuedFileNames).toEqual([]); + }); + + it("starts the next queued upload when an active upload finishes", () => { + dropFiles("f1.txt", "f2.txt", "f3.txt", "f4.txt", "f5.txt"); + + finishUpload(0, "f1.txt"); + + expect(multipartUploadSpy).toHaveBeenCalledTimes(4); + expect(uploadedPaths[3]).toBe("f4.txt"); + expect(component.activeCount).toBe(3); + expect(component.queuedCount).toBe(1); + expect(component.queuedFileNames).toEqual(["f5.txt"]); + }); + + it("removes a cancelled file from the pending queue without starting it", () => { + dropFiles("f1.txt", "f2.txt", "f3.txt", "f4.txt", "f5.txt"); + + component.cancelExistingUpload("f4.txt"); + + expect(multipartUploadSpy).toHaveBeenCalledTimes(3); + expect(component.queuedCount).toBe(1); + expect(component.queuedFileNames).toEqual(["f5.txt"]); + }); + + it("ignores cancellation of a file that is neither active nor queued", () => { + dropFiles("f1.txt", "f2.txt", "f3.txt", "f4.txt"); + + component.cancelExistingUpload("missing.txt"); + + expect(component.activeCount).toBe(3); + expect(component.queuedCount).toBe(1); + expect(component.queuedFileNames).toEqual(["f4.txt"]); + }); + + // #5586: the template reads queuedFileNames on every change-detection pass, + // so it must not allocate a new array unless the queue changed. + it("keeps the same queuedFileNames array reference while the queue is unchanged", () => { + dropFiles("f1.txt", "f2.txt", "f3.txt", "f4.txt", "f5.txt"); + + const firstRead = component.queuedFileNames; + + expect(component.queuedFileNames).toBe(firstRead); + }); + + it("exposes a new queuedFileNames array after the queue changes", () => { + dropFiles("f1.txt", "f2.txt", "f3.txt", "f4.txt", "f5.txt"); + const beforeCancel = component.queuedFileNames; + + component.cancelExistingUpload("f4.txt"); + + expect(component.queuedFileNames).not.toBe(beforeCancel); + expect(component.queuedFileNames).toEqual(["f5.txt"]); + }); + + it("identifies pending queue entries by file name in trackByPendingFile", () => { + expect(component.trackByPendingFile(0, "dir/a.txt")).toBe("dir/a.txt"); + }); + + // A resumed upload with no missing parts finishes with totalTime exactly 0; + // the slot must still be released. + it("releases the concurrency slot when a finished upload reports totalTime 0", () => { + dropFiles("f1.txt", "f2.txt", "f3.txt", "f4.txt"); + + finishUpload(0, "f1.txt", 0); + + expect(multipartUploadSpy).toHaveBeenCalledTimes(4); + expect(uploadedPaths[3]).toBe("f4.txt"); + expect(component.activeCount).toBe(3); + expect(component.queuedCount).toBe(0); + }); + + // The Pending header updates per file, so the Finished header must too — it + // cannot wait for the throttled staged-objects refetch. + it("updates the Finished count immediately when uploads finish", () => { + dropFiles("f1.txt", "f2.txt", "f3.txt", "f4.txt"); + expect(component.pendingChangesCount).toBe(0); + + finishUpload(0, "f1.txt"); + expect(component.pendingChangesCount).toBe(1); + + finishUpload(1, "f2.txt"); + expect(component.pendingChangesCount).toBe(2); + }); + + it("reconciles the optimistic Finished count with a diff response", () => { + dropFiles("f1.txt", "f2.txt", "f3.txt"); + finishUpload(0, "f1.txt"); + finishUpload(1, "f2.txt"); + + const diff: DatasetStagedObject[] = [{ path: "f1.txt", pathType: "file", diffType: "added", sizeBytes: 1 }]; + component.onStagedObjectsUpdated(diff); + + // f1 is confirmed by the response; f2 stays counted until a response includes it. + expect(component.pendingChangesCount).toBe(2); + + component.onStagedObjectsUpdated([...diff, { path: "f2.txt", pathType: "file", diffType: "added", sizeBytes: 1 }]); + expect(component.pendingChangesCount).toBe(2); + }); + + it("keeps an in-progress upload's slot while progress events stream in", () => { + dropFiles("f1.txt", "f2.txt", "f3.txt", "f4.txt"); + + uploadSubjects[0].next({ filePath: "f1.txt", percentage: 50, status: "uploading" }); + + expect(component.uploadTasks.find(t => t.filePath === "f1.txt")?.percentage).toBe(50); + expect(component.activeCount).toBe(3); + expect(component.queuedCount).toBe(1); + }); + + it("does not double-count a finished upload already confirmed by a diff response", () => { + dropFiles("f1.txt"); + finishUpload(0, "f1.txt"); + component.onStagedObjectsUpdated([{ path: "f1.txt", pathType: "file", diffType: "added", sizeBytes: 1 }]); + expect(component.pendingChangesCount).toBe(1); + + dropFiles("f1.txt"); // re-upload the already-staged file + finishUpload(1, "f1.txt"); + + expect(component.pendingChangesCount).toBe(1); + }); + + it("does not start queued uploads beyond a lowered concurrency limit", () => { + dropFiles("f1.txt", "f2.txt", "f3.txt", "f4.txt"); + component.maxConcurrentFiles = 1; + + finishUpload(0, "f1.txt"); + + expect(component.activeCount).toBe(2); + expect(component.queuedCount).toBe(1); + expect(multipartUploadSpy).toHaveBeenCalledTimes(3); + }); + + it("clears the Finished count when a version is created", () => { + dropFiles("f1.txt"); + finishUpload(0, "f1.txt"); + expect(component.pendingChangesCount).toBe(1); + + component.versionName = "v1"; + component.onClickOpenVersionCreator(); + + expect(component.pendingChangesCount).toBe(0); + }); + + it("does not remove a re-uploaded file's active task when hiding its finished predecessor", () => { + vi.useFakeTimers(); + try { + dropFiles("a.txt"); + finishUpload(0, "a.txt"); // schedules the finished row to hide in 5s + + dropFiles("a.txt"); // re-upload the same name within the 5s window + vi.advanceTimersByTime(5000); + + expect(component.uploadTasks).toHaveLength(1); + expect(component.uploadTasks[0].status).not.toBe("finished"); + expect(component.activeCount).toBe(1); + + finishUpload(1, "a.txt"); + expect(component.activeCount).toBe(0); + } finally { + vi.useRealTimers(); + } + }); + + it("renders the virtualized pending list and re-measures viewports on panel expand", async () => { + dropFiles("f1.txt", "f2.txt", "f3.txt", "f4.txt", "f5.txt"); + fixture.detectChanges(); + // Flush the viewport's init microtask, then render the rows. + await Promise.resolve(); + fixture.detectChanges(); + + expect(component.pendingListHeightPx).toBe(2 * component.PENDING_ROW_HEIGHT_PX); + const rows = fixture.nativeElement.querySelectorAll(".pending-file-row"); + expect(rows.length).toBe(2); + + // Expand the Pending / Uploading / Finished panels. + const headers: NodeListOf<HTMLElement> = fixture.nativeElement.querySelectorAll( + ".upload-status-panels .ant-collapse-header" + ); + expect(headers.length).toBe(3); + headers.forEach(header => header.click()); + fixture.detectChanges(); + // Flush the checkViewportSize timers. + await new Promise(resolve => setTimeout(resolve)); + + // Collapsing again must be a no-op for the re-measure handler. + headers.forEach(header => header.click()); + fixture.detectChanges(); + + // Cancel a queued file from its row. + const cancelButton = fixture.nativeElement.querySelector(".pending-file-row button") as HTMLButtonElement; + cancelButton.click(); + expect(component.queuedCount).toBe(1); + expect(component.queuedFileNames).toEqual(["f5.txt"]); + }); + + it("counts a staged file deletion immediately", () => { + const node: DatasetFileNode = { name: "a.txt", type: "file", parentDir: "/[email protected]/test-dataset/v1" }; + + component.onPreviouslyUploadedFileDeleted(node); + + expect(component.pendingChangesCount).toBe(1); + }); +}); diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts index 2287050ed5..c3b5c9a80f 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts @@ -17,7 +17,7 @@ * under the License. */ -import { Component, EventEmitter, OnInit, Output } from "@angular/core"; +import { Component, EventEmitter, OnInit, Output, ViewChild } from "@angular/core"; import { ActivatedRoute } from "@angular/router"; import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { DatasetService, MultipartUploadProgress } from "../../../../service/user/dataset/dataset.service"; @@ -67,6 +67,7 @@ import { FilesUploaderComponent } from "../../files-uploader/files-uploader.comp import { NzProgressComponent } from "ng-zorro-antd/progress"; import { UserDatasetStagedObjectsListComponent } from "./user-dataset-staged-objects-list/user-dataset-staged-objects-list.component"; import { NzInputDirective } from "ng-zorro-antd/input"; +import { CdkFixedSizeVirtualScroll, CdkVirtualForOf, CdkVirtualScrollViewport } from "@angular/cdk/scrolling"; export const THROTTLE_TIME_MS = 1000; export const ABORT_RETRY_MAX_ATTEMPTS = 10; @@ -110,6 +111,9 @@ export const ABORT_RETRY_BACKOFF_BASE_MS = 100; NzProgressComponent, UserDatasetStagedObjectsListComponent, NzInputDirective, + CdkVirtualScrollViewport, + CdkFixedSizeVirtualScroll, + CdkVirtualForOf, ], }) export class DatasetDetailComponent implements OnInit { @@ -148,6 +152,11 @@ export class DatasetDetailComponent implements OnInit { userHasPendingChanges: boolean = false; pendingChangesCount: number = 0; + // Staged paths from the last diff response, plus locally staged paths not yet + // in one: counted together so the Finished header keeps pace with the + // real-time Pending header between throttled refetches. + private confirmedStagedPaths = new Set<string>(); + private unconfirmedStagedPaths = new Set<string>(); // Uploading setting chunkSizeMiB: number = 50; @@ -158,7 +167,16 @@ export class DatasetDetailComponent implements OnInit { // Cap number of concurrent files uploads maxConcurrentFiles: number = 3; private activeUploads: number = 0; - private pendingQueue: Array<{ fileName: string; startUpload: () => void }> = []; + // FIFO queue of uploads waiting for a concurrency slot, keyed by file name. + private pendingQueue = new Map<string, () => void>(); + private pendingQueueDirty = false; + private queuedFileNamesSnapshot: string[] = []; + + // Row height must match .pending-file-row in the SCSS. + readonly PENDING_ROW_HEIGHT_PX = 32; + readonly PENDING_LIST_MAX_HEIGHT_PX = 160; + + @ViewChild(CdkVirtualScrollViewport) private pendingViewport?: CdkVirtualScrollViewport; versionName: string = ""; isCreatingVersion: boolean = false; @@ -263,6 +281,10 @@ export class DatasetDetailComponent implements OnInit { this.notificationService.success("Version Created"); this.isCreatingVersion = false; this.versionName = ""; + // A new version consumes all staged changes. + this.confirmedStagedPaths.clear(); + this.unconfirmedStagedPaths.clear(); + this.refreshPendingChanges(); this.retrieveDatasetVersionList(); this.userMakeChanges.emit(); }, @@ -411,8 +433,25 @@ export class DatasetDetailComponent implements OnInit { } onStagedObjectsUpdated(stagedObjects: DatasetStagedObject[]) { - this.userHasPendingChanges = stagedObjects.length > 0; - this.pendingChangesCount = stagedObjects.length; + this.confirmedStagedPaths = new Set(stagedObjects.map(obj => obj.path)); + for (const path of this.confirmedStagedPaths) { + this.unconfirmedStagedPaths.delete(path); + } + this.refreshPendingChanges(); + } + + // Reflects a locally staged change (finished upload or file deletion) in the + // Finished header immediately, ahead of the next diff response. + private markPathStaged(path: string): void { + if (!this.confirmedStagedPaths.has(path)) { + this.unconfirmedStagedPaths.add(path); + } + this.refreshPendingChanges(); + } + + private refreshPendingChanges(): void { + this.pendingChangesCount = this.confirmedStagedPaths.size + this.unconfirmedStagedPaths.size; + this.userHasPendingChanges = this.pendingChangesCount > 0; } onVersionSelected(version: DatasetVersion): void { @@ -460,6 +499,10 @@ export class DatasetDetailComponent implements OnInit { return task.filePath; } + trackByPendingFile(_: number, fileName: string): string { + return fileName; + } + private loadUploadSettings(): void { this.adminSettingsService .getSetting("multipart_upload_chunk_size_mib") @@ -484,7 +527,7 @@ export class DatasetDetailComponent implements OnInit { const continueWithUpload = () => { // Create upload function const startUpload = () => { - this.pendingQueue = this.pendingQueue.filter(item => item.fileName !== file.name); + this.removeFromPendingQueue(file.name); // Add an initializing task placeholder to uploadTasks this.uploadTasks.unshift({ @@ -517,10 +560,12 @@ export class DatasetDetailComponent implements OnInit { percentage: progress.percentage ?? this.uploadTasks[taskIndex].percentage ?? 0, }; - // Auto-hide when upload is truly finished - if (progress.status === "finished" && progress.totalTime) { + // totalTime may be exactly 0 (resumed upload with no missing + // parts); a truthiness check would leak the concurrency slot. + if (progress.status === "finished" && progress.totalTime !== undefined) { const filename = file.name.split("/").pop() || file.name; this.uploadTimeMap.set(filename, progress.totalTime); + this.markPathStaged(file.name); this.userMakeChanges.emit(); this.scheduleHide(taskIndex); this.onUploadComplete(); @@ -554,6 +599,7 @@ export class DatasetDetailComponent implements OnInit { const taskIndex = this.uploadTasks.findIndex(t => t.filePath === file.name); if (taskIndex !== -1 && this.uploadTasks[taskIndex].status !== "finished") { this.uploadTasks[taskIndex].status = "finished"; + this.markPathStaged(file.name); this.userMakeChanges.emit(); this.scheduleHide(taskIndex); this.onUploadComplete(); @@ -569,7 +615,8 @@ export class DatasetDetailComponent implements OnInit { this.activeUploads++; startUpload(); } else { - this.pendingQueue.push({ fileName: file.name, startUpload }); + this.pendingQueue.set(file.name, startUpload); + this.pendingQueueDirty = true; } }; @@ -588,18 +635,21 @@ export class DatasetDetailComponent implements OnInit { } } // Remove from pending queue if present - this.pendingQueue = this.pendingQueue.filter(item => item.fileName !== fileName); + this.removeFromPendingQueue(fileName); if (onCanceled) { onCanceled(); } } private processNextQueuedUpload(): void { - if (this.pendingQueue.length > 0 && this.activeUploads < this.maxConcurrentFiles) { - const next = this.pendingQueue.shift(); - if (next) { + if (this.activeUploads < this.maxConcurrentFiles) { + const next = this.pendingQueue.entries().next(); + if (!next.done) { + const [fileName, startUpload] = next.value; + this.pendingQueue.delete(fileName); + this.pendingQueueDirty = true; this.activeUploads++; - next.startUpload(); + startUpload(); } } } @@ -609,12 +659,36 @@ export class DatasetDetailComponent implements OnInit { this.processNextQueuedUpload(); } + private removeFromPendingQueue(fileName: string): void { + if (this.pendingQueue.delete(fileName)) { + this.pendingQueueDirty = true; + } + } + + // Stable array for the template: rebuilt at most once per queue change so + // change detection does not allocate a new array per pass (#5586). get queuedFileNames(): string[] { - return this.pendingQueue.map(item => item.fileName); + if (this.pendingQueueDirty) { + this.queuedFileNamesSnapshot = Array.from(this.pendingQueue.keys()); + this.pendingQueueDirty = false; + } + return this.queuedFileNamesSnapshot; } get queuedCount(): number { - return this.pendingQueue.length; + return this.pendingQueue.size; + } + + get pendingListHeightPx(): number { + return Math.min(this.queuedCount * this.PENDING_ROW_HEIGHT_PX, this.PENDING_LIST_MAX_HEIGHT_PX); + } + + // The viewport initializes inside the collapsed (display: none) panel and + // measures height 0; the CDK only re-measures on window resize. + onPendingPanelActiveChange(active: boolean): void { + if (active) { + setTimeout(() => this.pendingViewport?.checkViewportSize()); + } } get activeCount(): number { @@ -630,10 +704,12 @@ export class DatasetDetailComponent implements OnInit { if (idx === -1) { return; } - const key = this.uploadTasks[idx].filePath; - this.uploadSubscriptions.delete(key); + const task = this.uploadTasks[idx]; + this.uploadSubscriptions.delete(task.filePath); + // Remove by identity, not filePath: a same-named re-upload within the + // window has its own row, which must survive this timer. setTimeout(() => { - this.uploadTasks = this.uploadTasks.filter(t => t.filePath !== key); + this.uploadTasks = this.uploadTasks.filter(t => t !== task); }, 5000); } @@ -720,6 +796,7 @@ export class DatasetDetailComponent implements OnInit { this.notificationService.success( `File ${node.name} is successfully deleted. You may finalize it or revert it at the "Create Version" panel` ); + this.markPathStaged(getRelativePathFromDatasetFileNode(node)); this.userMakeChanges.emit(); }, error: (err: unknown) => { diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.html b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.html index 01d21074ea..a6785abd50 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.html +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.html @@ -17,40 +17,52 @@ under the License. --> -<div class="staged-object-list-container"> - <nz-list - nzSize="small" +<div + class="staged-object-list-container" + [class.has-list]="datasetStagedObjects.length > 0"> + <!-- Virtualized so only visible rows are in the DOM (#5586). The viewport can + initialize hidden and measure height 0; the host re-measures it on panel + expand (remeasureViewport). --> + <cdk-virtual-scroll-viewport *ngIf="datasetStagedObjects.length > 0" - class="custom-border-list"> - <nz-list-item *ngFor="let obj of datasetStagedObjects"> - <span> - <nz-tag [nzColor]="obj.diffType === 'added' ? 'green' : 'red'"> {{ obj.diffType }} </nz-tag> - </span> - <span - class="truncate-file-path" - nz-tooltip - [nzTooltipTitle]="fileTooltipTpl"> - {{ obj.path }} - </span> - <ng-template #fileTooltipTpl> - <div>{{ obj.path }}</div> - <div *ngIf="getFileUploadTime(obj.path) as uploadTime">Upload time: {{ formatTime(uploadTime) }}</div> - </ng-template> - <!-- Small delete button with tooltip --> - <button - nz-button - nzType="link" - class="delete-button" - nz-tooltip - [nzTooltipTitle]="'Revert the change'" - (click)="onObjectReverted(obj)"> - <i - nz-icon - nzType="delete" - nzTheme="outline"></i> - </button> - </nz-list-item> - </nz-list> + class="staged-object-viewport" + [itemSize]="STAGED_ROW_HEIGHT_PX" + [minBufferPx]="STAGED_LIST_MAX_HEIGHT_PX" + [maxBufferPx]="2 * STAGED_LIST_MAX_HEIGHT_PX" + [style.height.px]="stagedListHeightPx"> + <nz-list nzSize="small"> + <nz-list-item + class="staged-object-row" + *cdkVirtualFor="let obj of datasetStagedObjects; trackBy: trackByStagedObject"> + <span> + <nz-tag [nzColor]="obj.diffType === 'added' ? 'green' : 'red'"> {{ obj.diffType }} </nz-tag> + </span> + <span + class="truncate-file-path" + nz-tooltip + [nzTooltipTitle]="fileTooltipTpl"> + {{ obj.path }} + </span> + <ng-template #fileTooltipTpl> + <div>{{ obj.path }}</div> + <div *ngIf="getFileUploadTime(obj.path) as uploadTime">Upload time: {{ formatTime(uploadTime) }}</div> + </ng-template> + <!-- Small delete button with tooltip --> + <button + nz-button + nzType="link" + class="delete-button" + nz-tooltip + [nzTooltipTitle]="'Revert the change'" + (click)="onObjectReverted(obj)"> + <i + nz-icon + nzType="delete" + nzTheme="outline"></i> + </button> + </nz-list-item> + </nz-list> + </cdk-virtual-scroll-viewport> <nz-empty *ngIf="datasetStagedObjects.length === 0" diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.scss b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.scss index 493f1abaa3..fd7336b0e9 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.scss +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.scss @@ -17,15 +17,20 @@ * under the License. */ -/* Styles for the file tree container */ +/* The bottom border sits outside the viewport: inside it would ride the + scrolled content and overflow the exactly row-sized viewport by 1px. */ .staged-object-list-container { - max-height: 200px; /* Adjust the max-height as needed */ - overflow-y: auto; /* Enables vertical scrolling when content exceeds max-height */ - overflow-x: auto; /* Prevents horizontal scrolling */ + overflow: hidden; + + &.has-list { + border-bottom: 1px solid #f0f0f0; + } } -.custom-border-list { - border-bottom: 1px solid #f0f0f0; +/* Must stay exactly STAGED_ROW_HEIGHT_PX tall for the fixed-size scroll + strategy. */ +.staged-object-row { + height: 40px; } .truncate-file-path { diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.spec.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.spec.ts new file mode 100644 index 0000000000..d00ca78a71 --- /dev/null +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.spec.ts @@ -0,0 +1,159 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { EventEmitter } from "@angular/core"; +import { By } from "@angular/platform-browser"; +import { CdkVirtualScrollViewport } from "@angular/cdk/scrolling"; +import { NzTooltipDirective } from "ng-zorro-antd/tooltip"; +import { of } from "rxjs"; +import { UserDatasetStagedObjectsListComponent } from "./user-dataset-staged-objects-list.component"; +import { DatasetService } from "../../../../../service/user/dataset/dataset.service"; +import { NotificationService } from "../../../../../../common/service/notification/notification.service"; +import { DatasetStagedObject } from "../../../../../../common/type/dataset-staged-object"; +import { commonTestImports, commonTestProviders } from "../../../../../../common/testing/test-utils"; + +describe("UserDatasetStagedObjectsListComponent", () => { + let fixture: ComponentFixture<UserDatasetStagedObjectsListComponent>; + let component: UserDatasetStagedObjectsListComponent; + let getDatasetDiffSpy: ReturnType<typeof vi.fn>; + let resetDatasetFileDiffSpy: ReturnType<typeof vi.fn>; + + const renderList = async () => { + component.did = 1; + fixture.detectChanges(); + await fixture.whenStable(); + fixture.detectChanges(); + }; + + const stagedObjects: DatasetStagedObject[] = [ + { path: "dir/a.txt", pathType: "file", diffType: "added", sizeBytes: 1 }, + { path: "dir/b.txt", pathType: "file", diffType: "removed" }, + ]; + + beforeEach(() => { + getDatasetDiffSpy = vi.fn(() => of(stagedObjects)); + resetDatasetFileDiffSpy = vi.fn(() => of({})); + + TestBed.configureTestingModule({ + imports: [UserDatasetStagedObjectsListComponent, ...commonTestImports], + providers: [ + { + provide: DatasetService, + useValue: { getDatasetDiff: getDatasetDiffSpy, resetDatasetFileDiff: resetDatasetFileDiffSpy }, + }, + { provide: NotificationService, useValue: { success: vi.fn(), error: vi.fn() } }, + ...commonTestProviders, + ], + }); + + fixture = TestBed.createComponent(UserDatasetStagedObjectsListComponent); + component = fixture.componentInstance; + }); + + it("fetches staged objects on init and emits them", () => { + component.did = 1; + const emitted: DatasetStagedObject[][] = []; + component.stagedObjectsChanged.subscribe((objects: DatasetStagedObject[]) => emitted.push(objects)); + + component.ngOnInit(); + + expect(getDatasetDiffSpy).toHaveBeenCalledWith(1); + expect(component.datasetStagedObjects).toEqual(stagedObjects); + expect(emitted).toEqual([stagedObjects]); + }); + + it("does not fetch staged objects when did is undefined", () => { + component.did = undefined; + + component.ngOnInit(); + + expect(getDatasetDiffSpy).not.toHaveBeenCalled(); + expect(component.datasetStagedObjects).toEqual([]); + }); + + // #5586: one change event per finished file must not mean one dataset-diff + // request per file. + it("coalesces bursts of change events into one refetch per audit window", () => { + vi.useFakeTimers(); + try { + component.did = 1; + const changes = new EventEmitter<void>(); + component.userMakeChangesEvent = changes; + + for (let i = 0; i < 50; i++) { + changes.emit(); + } + expect(getDatasetDiffSpy).not.toHaveBeenCalled(); + + vi.advanceTimersByTime(UserDatasetStagedObjectsListComponent.REFRESH_AUDIT_TIME_MS); + expect(getDatasetDiffSpy).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + } + }); + + it("identifies staged objects by diff type and path in trackByStagedObject", () => { + expect(component.trackByStagedObject(0, stagedObjects[0])).toBe("added:dir/a.txt"); + expect(component.trackByStagedObject(1, stagedObjects[1])).toBe("removed:dir/b.txt"); + }); + + it("renders staged object rows inside the virtual scroll viewport", async () => { + await renderList(); + + const viewport = fixture.nativeElement.querySelector("cdk-virtual-scroll-viewport"); + expect(viewport).not.toBeNull(); + const rows = fixture.nativeElement.querySelectorAll("nz-list-item.staged-object-row"); + expect(rows.length).toBe(stagedObjects.length); + expect(rows[0].textContent).toContain("dir/a.txt"); + expect(rows[1].textContent).toContain("dir/b.txt"); + }); + + it("reverts a staged object from its row's delete button", async () => { + await renderList(); + + (fixture.nativeElement.querySelector(".delete-button") as HTMLButtonElement).click(); + + expect(resetDatasetFileDiffSpy).toHaveBeenCalledWith(1, "dir/a.txt"); + expect(getDatasetDiffSpy).toHaveBeenCalledTimes(2); + }); + + it("shows the full path and upload time in the row tooltip", async () => { + component.uploadTimeMap = new Map([["a.txt", 5]]); + await renderList(); + + fixture.debugElement.query(By.css(".truncate-file-path")).injector.get(NzTooltipDirective).show(); + fixture.detectChanges(); + + const overlayText = document.querySelector(".cdk-overlay-container")?.textContent ?? ""; + expect(overlayText).toContain("dir/a.txt"); + expect(overlayText).toContain("Upload time"); + }); + + it("re-measures the viewport on request", async () => { + await renderList(); + const viewport = fixture.debugElement.query(By.directive(CdkVirtualScrollViewport)).componentInstance; + const checkViewportSizeSpy = vi.spyOn(viewport, "checkViewportSize"); + + component.remeasureViewport(); + await new Promise(resolve => setTimeout(resolve)); + + expect(checkViewportSizeSpy).toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.ts index 79a632f9c4..f92ba316f0 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.ts +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-staged-objects-list/user-dataset-staged-objects-list.component.ts @@ -17,13 +17,14 @@ * under the License. */ -import { Component, EventEmitter, Input, OnInit, Output } from "@angular/core"; +import { Component, EventEmitter, Input, OnInit, Output, ViewChild } from "@angular/core"; +import { auditTime } from "rxjs/operators"; import { DatasetStagedObject } from "../../../../../../common/type/dataset-staged-object"; import { DatasetService } from "../../../../../service/user/dataset/dataset.service"; import { NotificationService } from "../../../../../../common/service/notification/notification.service"; import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { formatTime } from "src/app/common/util/format.util"; -import { NgIf, NgFor } from "@angular/common"; +import { NgIf } from "@angular/common"; import { NzListComponent, NzListItemComponent } from "ng-zorro-antd/list"; import { NzTagComponent } from "ng-zorro-antd/tag"; import { NzTooltipDirective } from "ng-zorro-antd/tooltip"; @@ -32,6 +33,7 @@ import { NzButtonComponent } from "ng-zorro-antd/button"; import { ɵNzTransitionPatchDirective } from "ng-zorro-antd/core/transition-patch"; import { NzIconDirective } from "ng-zorro-antd/icon"; import { NzEmptyComponent } from "ng-zorro-antd/empty"; +import { CdkFixedSizeVirtualScroll, CdkVirtualForOf, CdkVirtualScrollViewport } from "@angular/cdk/scrolling"; @UntilDestroy() @Component({ @@ -41,8 +43,10 @@ import { NzEmptyComponent } from "ng-zorro-antd/empty"; imports: [ NgIf, NzListComponent, - NgFor, NzListItemComponent, + CdkVirtualScrollViewport, + CdkFixedSizeVirtualScroll, + CdkVirtualForOf, NzTagComponent, NzTooltipDirective, NzSpaceCompactItemDirective, @@ -53,12 +57,18 @@ import { NzEmptyComponent } from "ng-zorro-antd/empty"; ], }) export class UserDatasetStagedObjectsListComponent implements OnInit { + // Coalesces change events so a bulk upload refetches the diff at most once + // per window, not once per finished file. + static readonly REFRESH_AUDIT_TIME_MS = 1000; + @Input() did?: number; // Dataset ID @Input() set userMakeChangesEvent(event: EventEmitter<void>) { if (event) { - event.pipe(untilDestroyed(this)).subscribe(() => { - this.fetchDatasetStagedObjects(); - }); + event + .pipe(auditTime(UserDatasetStagedObjectsListComponent.REFRESH_AUDIT_TIME_MS), untilDestroyed(this)) + .subscribe(() => { + this.fetchDatasetStagedObjects(); + }); } } @Input() uploadTimeMap?: Map<string, number>; @@ -68,6 +78,22 @@ export class UserDatasetStagedObjectsListComponent implements OnInit { datasetStagedObjects: DatasetStagedObject[] = []; formatTime = formatTime; + // Row height must match .staged-object-row in the SCSS. + readonly STAGED_ROW_HEIGHT_PX = 40; + readonly STAGED_LIST_MAX_HEIGHT_PX = 200; + + @ViewChild(CdkVirtualScrollViewport) private viewport?: CdkVirtualScrollViewport; + + get stagedListHeightPx(): number { + return Math.min(this.datasetStagedObjects.length * this.STAGED_ROW_HEIGHT_PX, this.STAGED_LIST_MAX_HEIGHT_PX); + } + + // The viewport measures height 0 when created inside a hidden ancestor + // (e.g. a collapsed panel); hosts call this once the list is visible. + remeasureViewport(): void { + setTimeout(() => this.viewport?.checkViewportSize()); + } + constructor( private datasetService: DatasetService, private notificationService: NotificationService @@ -113,4 +139,8 @@ export class UserDatasetStagedObjectsListComponent implements OnInit { const filename = filePath.split("/").pop() || filePath; return this.uploadTimeMap.get(filename) || null; } + + trackByStagedObject(_: number, obj: DatasetStagedObject): string { + return `${obj.diffType}:${obj.path}`; + } } diff --git a/frontend/src/jsdom-svg-polyfill.ts b/frontend/src/jsdom-svg-polyfill.ts index 4e659d7b6a..1fe62e2147 100644 --- a/frontend/src/jsdom-svg-polyfill.ts +++ b/frontend/src/jsdom-svg-polyfill.ts @@ -183,6 +183,15 @@ G.requestIdleCallback ??= (cb: (d: { didTimeout: boolean; timeRemaining: () => n setTimeout(() => cb({ didTimeout: false, timeRemaining: () => 50 }), 0); G.cancelIdleCallback ??= (id: number) => clearTimeout(id); +// `ResizeObserver` — jsdom doesn't implement it; components that watch their +// own size (e.g. markdown-description) construct one on render. An inert stub +// is enough: jsdom has no layout, so there is never a resize to report. +G.ResizeObserver ??= class { + observe(): void {} + unobserve(): void {} + disconnect(): void {} +}; + // `WebSocket` — y-websocket schedules a reconnect timer the moment a // collaborative-editing service is constructed. When that timer fires AFTER // vitest has begun tearing down the jsdom window, jsdom's WebSocket
