From fd5b39b17aa67f2d04745b686f95d730bdd5eb52 Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Thu, 6 Mar 2025 10:36:56 +0100 Subject: [PATCH] Submission save+sub stability / performance * Debug console output added for testing * Debouncer added to save operations * More thorough sub management / tracking --- .../objects/submission-objects.actions.ts | 4 +- .../sections/form/section-form.component.ts | 4 + src/app/submission/submission.service.ts | 185 ++++++++++++++++-- 3 files changed, 176 insertions(+), 17 deletions(-) diff --git a/src/app/submission/objects/submission-objects.actions.ts b/src/app/submission/objects/submission-objects.actions.ts index 956c8f95336..9f4ba4b34a2 100644 --- a/src/app/submission/objects/submission-objects.actions.ts +++ b/src/app/submission/objects/submission-objects.actions.ts @@ -433,6 +433,7 @@ export class SaveSubmissionFormAction implements Action { payload: { submissionId: string; isManual?: boolean; + timestamp: number; }; /** @@ -442,7 +443,8 @@ export class SaveSubmissionFormAction implements Action { * the submission's ID */ constructor(submissionId: string, isManual: boolean = false) { - this.payload = { submissionId, isManual }; + this.payload = { submissionId, isManual, timestamp: Date.now() }; + console.log(`Creating SaveSubmissionFormAction for submission ${submissionId} at ${new Date().toISOString()}`); } } diff --git a/src/app/submission/sections/form/section-form.component.ts b/src/app/submission/sections/form/section-form.component.ts index 75950bfe526..2a97c8b4ce6 100644 --- a/src/app/submission/sections/form/section-form.component.ts +++ b/src/app/submission/sections/form/section-form.component.ts @@ -402,6 +402,9 @@ export class SubmissionSectionFormComponent extends SectionModelComponent { * Initialize all subscriptions */ subscriptions(): void { + // clear existing subscriptions first + this.onSectionDestroy(); + this.subs = []; this.subs.push( /** * Subscribe to form's data @@ -426,6 +429,7 @@ export class SubmissionSectionFormComponent extends SectionModelComponent { this.updateForm(sectionState); }), ); + console.log(`Section ${this.sectionData.id} has ${this.subs.length} active subscriptions`); } /** diff --git a/src/app/submission/submission.service.ts b/src/app/submission/submission.service.ts index ba8eff4f382..95e44c9b5e7 100644 --- a/src/app/submission/submission.service.ts +++ b/src/app/submission/submission.service.ts @@ -9,19 +9,20 @@ import { } from '@ngrx/store'; import { TranslateService } from '@ngx-translate/core'; import { - Observable, - of as observableOf, - Subscription, + EMPTY, + Observable, of, + of as observableOf, ReplaySubject, share, Subject, + Subscription, timeout, TimeoutError, timer as observableTimer, } from 'rxjs'; import { catchError, - concatMap, + concatMap, debounceTime, distinctUntilChanged, - filter, + filter, finalize, find, map, - startWith, + startWith, switchMap, take, tap, } from 'rxjs/operators'; @@ -102,6 +103,10 @@ export class SubmissionService { */ protected autoSaveSub: Subscription; + private _currentSaveSubmissionId; + private _saveDebouncer = new Subject<{id: string, manual: boolean}>(); + private activeStateSubscriptions: Map = new Map(); + /** * Observable used as timer */ @@ -109,6 +114,9 @@ export class SubmissionService { private workspaceLinkPath = 'workspaceitems'; private workflowLinkPath = 'workflowitems'; + + // testing sub stuff + private submissionStateSubjects: Map>; /** * Initialize service variables * @param {NotificationsService} notificationsService @@ -130,8 +138,48 @@ export class SubmissionService { protected searchService: SearchService, protected requestService: RequestService, protected jsonPatchOperationService: SubmissionJsonPatchOperationsService) { + this._saveDebouncer.pipe( + debounceTime(300), + //distinctUntilChanged((prev, curr) => prev.id === curr.id), + ).subscribe(({ id, manual }) => { + this._dispatchSaveImpl(id, manual); + }); + } + + // testing sub stuff + // Add this method to manage subscriptions better + private getOrCreateStateSubscription(submissionId: string): Observable { + // Check if we already have an active subject for this submission + if (!this.submissionStateSubjects) { + this.submissionStateSubjects = new Map(); + } + + if (!this.submissionStateSubjects.has(submissionId)) { + // Create a new ReplaySubject to cache the latest state + const subject = new ReplaySubject(1); + + // Set up a single subscription to the store that feeds our subject + const subscription = this.store.select(submissionObjectFromIdSelector(submissionId)) + .pipe( + filter((submission: SubmissionObjectEntry) => isNotUndefined(submission)) + ) + .subscribe(subject); + + // Track this subscription so we can clean it up later + if (!this.activeStateSubscriptions.has(submissionId)) { + this.activeStateSubscriptions.set(submissionId, []); + } + this.activeStateSubscriptions.get(submissionId).push(subscription); + + // Store our subject + this.submissionStateSubjects.set(submissionId, subject); + } + + // Return the subject as an observable + return this.submissionStateSubjects.get(submissionId).asObservable(); } + /** * Dispatch a new [ChangeSubmissionCollectionAction] * @@ -269,11 +317,60 @@ export class SubmissionService { * whether is a manual save, default false */ dispatchSave(submissionId, manual?: boolean) { - this.getSubmissionSaveProcessingStatus(submissionId).pipe( - find((isPending: boolean) => !isPending), - ).subscribe(() => { - this.store.dispatch(new SaveSubmissionFormAction(submissionId, manual)); - }); + console.log('qeueing save for', submissionId); + this._saveDebouncer.next({id: submissionId, manual: manual}); + } + _dispatchSaveImpl(submissionId, manual?: boolean) { + console.log('Dispatching save for', submissionId); + const saveInProgress = this._currentSaveSubmissionId === submissionId; + if (saveInProgress) { + console.log(`Save already in progress, ignoring save for ${submissionId}`); + return; + } + + this._currentSaveSubmissionId = submissionId; + + // Check the current save processing status first + this.getSubmissionObject(submissionId).pipe( + take(1), + map((state: SubmissionObjectEntry) => state.savePending), + switchMap((isPending: boolean) => { + if (!isPending) { + // If not pending, proceed immediately + return of(false); + } else { + // Otherwise wait for non-pending state + return this.getSubmissionSaveProcessingStatus(submissionId).pipe( + find((pendingState: boolean) => !pendingState), + timeout(10000) + ); + } + }), + finalize(() => { + console.log(`Clearing save in progress flag for ${submissionId}`); + this._currentSaveSubmissionId = null; + }) + ).subscribe({ + next: () => { + console.log('Proceeding with save dispatch'); + this.store.dispatch(new SaveSubmissionFormAction(submissionId, manual)); + }, + error: error => { + console.error('Error waiting for non-pending state', error); + // Error handling logic - potentially still dispatch the save if it was a timeout + if (error instanceof TimeoutError) { + console.log('Timeout occurred, forcing save dispatch'); + this.store.dispatch(new SaveSubmissionFormAction(submissionId, manual)); + } + } + }) + // ).subscribe(() => { + // console.log('Proceeding with save dispatch'); + // this.store.dispatch(new SaveSubmissionFormAction(submissionId, manual)); + // }, error => { + // console.error('Error waiting for non-pending state', error); + // this._currentSaveSubmissionId = null; + // }); } /** @@ -319,9 +416,18 @@ export class SubmissionService { * @return Observable * observable of SubmissionObjectEntry */ + // getSubmissionObject(submissionId: string): Observable { + // console.log(`Getting submission object for submission ${submissionId}, active subs:`, this.activeStateSubscriptions.get(submissionId)?.length || 0); + // return this.store.select(submissionObjectFromIdSelector(submissionId)).pipe( + // filter((submission: SubmissionObjectEntry) => isNotUndefined(submission)), + // share() + // ); + // } + // getSubmissionObject(submissionId: string): Observable { - return this.store.select(submissionObjectFromIdSelector(submissionId)).pipe( - filter((submission: SubmissionObjectEntry) => isNotUndefined(submission))); + console.log(`Getting submission object for submission ${submissionId}, active subs:`, + this.activeStateSubscriptions.get(submissionId)?.length || 0); + return this.getOrCreateStateSubscription(submissionId); } /** @@ -464,13 +570,26 @@ export class SubmissionService { * @return Observable * observable with submission save processing status */ + // getSubmissionSaveProcessingStatus(submissionId: string): Observable { + // const requestId = Date.now(); // unique req id + // console.log(`${requestId}: checking save status for ${submissionId}`); + // return this.getSubmissionObject(submissionId).pipe( + // tap((submission) => { + // console.log('Current submission state:', submission.savePending ? 'PENDING' : 'NOT PENDING'); + // }), + // map((state: SubmissionObjectEntry) => state.savePending), + // distinctUntilChanged(), + // tap(isPending => console.log(`Save pending status changed to: ${isPending ? 'PENDING' : 'NOT PENDING'}`)), + // startWith(false)); + // } getSubmissionSaveProcessingStatus(submissionId: string): Observable { return this.getSubmissionObject(submissionId).pipe( map((state: SubmissionObjectEntry) => state.savePending), distinctUntilChanged(), - startWith(false)); + // Only log when the status actually changes + tap(isPending => console.log(`Save pending status: ${isPending ? 'PENDING' : 'NOT PENDING'}`)) + ); } - /** * Return the deposit processing status of the submission * @@ -568,13 +687,47 @@ export class SubmissionService { ).subscribe(); } + + + + // clearSubmissionSubscriptions(submissionId: string) { + // const subs = this.activeStateSubscriptions.get(submissionId) || []; + // console.log(`Clearing ${subs.length} subscriptions for submission ${submissionId}`); + // + // subs.forEach(sub => { + // if (!sub.closed) { + // sub.unsubscribe(); + // } + // }); + // + // this.activeStateSubscriptions.set(submissionId, []); + // } + clearSubmissionSubscriptions(submissionId: string) { + const subs = this.activeStateSubscriptions.get(submissionId) || []; + console.log(`Clearing ${subs.length} subscriptions for submission ${submissionId}`); + + subs.forEach(sub => { + if (!sub.closed) { + sub.unsubscribe(); + } + }); + + this.activeStateSubscriptions.set(submissionId, []); + + // Also clear any state subjects + if (this.submissionStateSubjects && this.submissionStateSubjects.has(submissionId)) { + this.submissionStateSubjects.delete(submissionId); + } + } /** * Dispatch a new [CancelSubmissionFormAction] */ resetAllSubmissionObjects() { + Array.from(this.activeStateSubscriptions.keys()).forEach(submissionId => { + this.clearSubmissionSubscriptions(submissionId); + }); this.store.dispatch(new CancelSubmissionFormAction()); } - /** * Dispatch a new [ResetSubmissionFormAction] *