Fix(dev): history causing infinite requests (#12059)

src-ui/src/app/components/document-detail/document-detail.component.ts

@@ -116,9 +116,9 @@ import {
 } from '../common/pdf-viewer/pdf-viewer.types'
 import { ShareLinksDialogComponent } from '../common/share-links-dialog/share-links-dialog.component'
 import { SuggestionsDropdownComponent } from '../common/suggestions-dropdown/suggestions-dropdown.component'
-import { DocumentHistoryComponent } from '../document-history/document-history.component'
 import { DocumentNotesComponent } from '../document-notes/document-notes.component'
 import { ComponentWithPermissions } from '../with-permissions/with-permissions.component'
+import { DocumentHistoryComponent } from './document-history/document-history.component'
 import { MetadataCollapseComponent } from './metadata-collapse/metadata-collapse.component'
 
 enum DocumentDetailNavIDs {

src-ui/src/app/components/document-detail/document-history/document-history.component.html

@@ -1,6 +1,6 @@
 @if (loading) {
     <div class="d-flex">
-        <div class="spinner-border spinner-border-sm fw-normal" role="status"></div>
+        <output class="spinner-border spinner-border-sm fw-normal" role="status"></output>
     </div>
 } @else {
     <ul class="list-group">

src-ui/src/app/components/document-detail/document-history/document-history.component.spec.ts

@@ -83,8 +83,22 @@ describe('DocumentHistoryComponent', () => {
         expect(result).toBe(correspondentName)
       })
     expect(getCachedSpy).toHaveBeenCalledWith(parseInt(correspondentId))
-    // no correspondent found
+  })
-    getCachedSpy.mockReturnValue(of(null))
+
+  it('getPrettyName should memoize results to avoid resubscribe loops', () => {
+    const correspondentId = '1'
+    const getCachedSpy = jest
+      .spyOn(correspondentService, 'getCached')
+      .mockReturnValue(of({ name: 'John Doe' }))
+    const a = component.getPrettyName(DataType.Correspondent, correspondentId)
+    const b = component.getPrettyName(DataType.Correspondent, correspondentId)
+    expect(a).toBe(b)
+    expect(getCachedSpy).toHaveBeenCalledTimes(1)
+  })
+
+  it('getPrettyName should fall back to the correspondent id when missing', () => {
+    const correspondentId = '1'
+    jest.spyOn(correspondentService, 'getCached').mockReturnValue(of(null))
     component
       .getPrettyName(DataType.Correspondent, correspondentId)
       .subscribe((result) => {
@@ -104,8 +118,11 @@ describe('DocumentHistoryComponent', () => {
         expect(result).toBe(documentTypeName)
       })
     expect(getCachedSpy).toHaveBeenCalledWith(parseInt(documentTypeId))
-    // no document type found
+  })
-    getCachedSpy.mockReturnValue(of(null))
+
+  it('getPrettyName should fall back to the document type id when missing', () => {
+    const documentTypeId = '1'
+    jest.spyOn(documentTypeService, 'getCached').mockReturnValue(of(null))
     component
       .getPrettyName(DataType.DocumentType, documentTypeId)
       .subscribe((result) => {
@@ -125,8 +142,11 @@ describe('DocumentHistoryComponent', () => {
         expect(result).toBe(storagePath)
       })
     expect(getCachedSpy).toHaveBeenCalledWith(parseInt(storagePathId))
-    // no storage path found
+  })
-    getCachedSpy.mockReturnValue(of(null))
+
+  it('getPrettyName should fall back to the storage path id when missing', () => {
+    const storagePathId = '1'
+    jest.spyOn(storagePathService, 'getCached').mockReturnValue(of(null))
     component
       .getPrettyName(DataType.StoragePath, storagePathId)
       .subscribe((result) => {
@@ -144,8 +164,11 @@ describe('DocumentHistoryComponent', () => {
       expect(result).toBe(ownerUsername)
     })
     expect(getCachedSpy).toHaveBeenCalledWith(parseInt(ownerId))
-    // no user found
+  })
-    getCachedSpy.mockReturnValue(of(null))
+
+  it('getPrettyName should fall back to the owner id when missing', () => {
+    const ownerId = '1'
+    jest.spyOn(userService, 'getCached').mockReturnValue(of(null))
     component.getPrettyName('owner', ownerId).subscribe((result) => {
       expect(result).toBe(ownerId)
     })

src-ui/src/app/components/document-detail/document-history/document-history.component.ts

@@ -0,0 +1,114 @@
+import { AsyncPipe, KeyValuePipe, TitleCasePipe } from '@angular/common'
+import { Component, Input, OnInit, inject } from '@angular/core'
+import { NgbTooltipModule } from '@ng-bootstrap/ng-bootstrap'
+import { NgxBootstrapIconsModule } from 'ngx-bootstrap-icons'
+import { Observable, first, map, of, shareReplay } from 'rxjs'
+import { AuditLogAction, AuditLogEntry } from 'src/app/data/auditlog-entry'
+import { DataType } from 'src/app/data/datatype'
+import { CustomDatePipe } from 'src/app/pipes/custom-date.pipe'
+import { CorrespondentService } from 'src/app/services/rest/correspondent.service'
+import { DocumentTypeService } from 'src/app/services/rest/document-type.service'
+import { DocumentService } from 'src/app/services/rest/document.service'
+import { StoragePathService } from 'src/app/services/rest/storage-path.service'
+import { UserService } from 'src/app/services/rest/user.service'
+
+@Component({
+  selector: 'pngx-document-history',
+  templateUrl: './document-history.component.html',
+  styleUrl: './document-history.component.scss',
+  imports: [
+    CustomDatePipe,
+    NgbTooltipModule,
+    AsyncPipe,
+    KeyValuePipe,
+    TitleCasePipe,
+    NgxBootstrapIconsModule,
+  ],
+})
+export class DocumentHistoryComponent implements OnInit {
+  private documentService = inject(DocumentService)
+  private correspondentService = inject(CorrespondentService)
+  private storagePathService = inject(StoragePathService)
+  private documentTypeService = inject(DocumentTypeService)
+  private userService = inject(UserService)
+
+  public AuditLogAction = AuditLogAction
+
+  private _documentId: number
+  @Input()
+  set documentId(id: number) {
+    if (this._documentId !== id) {
+      this._documentId = id
+      this.prettyNameCache.clear()
+      this.loadHistory()
+    }
+  }
+
+  public loading: boolean = true
+  public entries: AuditLogEntry[] = []
+
+  private readonly prettyNameCache = new Map<string, Observable<string>>()
+
+  ngOnInit(): void {
+    this.loadHistory()
+  }
+
+  private loadHistory(): void {
+    if (this._documentId) {
+      this.loading = true
+      this.documentService.getHistory(this._documentId).subscribe((entries) => {
+        this.entries = entries
+        this.loading = false
+      })
+    }
+  }
+
+  getPrettyName(type: DataType | string, id: string): Observable<string> {
+    const cacheKey = `${type}:${id}`
+    const cached = this.prettyNameCache.get(cacheKey)
+    if (cached) {
+      return cached
+    }
+
+    const idInt = parseInt(id, 10)
+    const fallback$ = of(id)
+
+    let result$: Observable<string>
+    if (!Number.isFinite(idInt)) {
+      result$ = fallback$
+    } else {
+      switch (type) {
+        case DataType.Correspondent:
+          result$ = this.correspondentService.getCached(idInt).pipe(
+            first(),
+            map((correspondent) => correspondent?.name ?? id)
+          )
+          break
+        case DataType.DocumentType:
+          result$ = this.documentTypeService.getCached(idInt).pipe(
+            first(),
+            map((documentType) => documentType?.name ?? id)
+          )
+          break
+        case DataType.StoragePath:
+          result$ = this.storagePathService.getCached(idInt).pipe(
+            first(),
+            map((storagePath) => storagePath?.path ?? id)
+          )
+          break
+        case 'owner':
+          result$ = this.userService.getCached(idInt).pipe(
+            first(),
+            map((user) => user?.username ?? id)
+          )
+          break
+        default:
+          result$ = fallback$
+      }
+    }
+
+    const shared$ = result$.pipe(shareReplay({ bufferSize: 1, refCount: true }))
+    this.prettyNameCache.set(cacheKey, shared$)
+    return shared$
+  }
+}

src-ui/src/app/components/document-history/document-history.component.ts

@@ -1,85 +0,0 @@
-import { AsyncPipe, KeyValuePipe, TitleCasePipe } from '@angular/common'
-import { Component, Input, OnInit, inject } from '@angular/core'
-import { NgbTooltipModule } from '@ng-bootstrap/ng-bootstrap'
-import { NgxBootstrapIconsModule } from 'ngx-bootstrap-icons'
-import { Observable, first, map, of } from 'rxjs'
-import { AuditLogAction, AuditLogEntry } from 'src/app/data/auditlog-entry'
-import { DataType } from 'src/app/data/datatype'
-import { CustomDatePipe } from 'src/app/pipes/custom-date.pipe'
-import { CorrespondentService } from 'src/app/services/rest/correspondent.service'
-import { DocumentTypeService } from 'src/app/services/rest/document-type.service'
-import { DocumentService } from 'src/app/services/rest/document.service'
-import { StoragePathService } from 'src/app/services/rest/storage-path.service'
-import { UserService } from 'src/app/services/rest/user.service'
-
-@Component({
-  selector: 'pngx-document-history',
-  templateUrl: './document-history.component.html',
-  styleUrl: './document-history.component.scss',
-  imports: [
-    CustomDatePipe,
-    NgbTooltipModule,
-    AsyncPipe,
-    KeyValuePipe,
-    TitleCasePipe,
-    NgxBootstrapIconsModule,
-  ],
-})
-export class DocumentHistoryComponent implements OnInit {
-  private documentService = inject(DocumentService)
-  private correspondentService = inject(CorrespondentService)
-  private storagePathService = inject(StoragePathService)
-  private documentTypeService = inject(DocumentTypeService)
-  private userService = inject(UserService)
-
-  public AuditLogAction = AuditLogAction
-
-  private _documentId: number
-  @Input()
-  set documentId(id: number) {
-    this._documentId = id
-    this.ngOnInit()
-  }
-
-  public loading: boolean = true
-  public entries: AuditLogEntry[] = []
-
-  ngOnInit(): void {
-    if (this._documentId) {
-      this.loading = true
-      this.documentService
-        .getHistory(this._documentId)
-        .subscribe((auditLogEntries) => {
-          this.entries = auditLogEntries
-          this.loading = false
-        })
-    }
-  }
-
-  getPrettyName(type: DataType | string, id: string): Observable<string> {
-    switch (type) {
-      case DataType.Correspondent:
-        return this.correspondentService.getCached(parseInt(id, 10)).pipe(
-          first(),
-          map((correspondent) => correspondent?.name ?? id)
-        )
-      case DataType.DocumentType:
-        return this.documentTypeService.getCached(parseInt(id, 10)).pipe(
-          first(),
-          map((documentType) => documentType?.name ?? id)
-        )
-      case DataType.StoragePath:
-        return this.storagePathService.getCached(parseInt(id, 10)).pipe(
-          first(),
-          map((storagePath) => storagePath?.path ?? id)
-        )
-      case 'owner':
-        return this.userService.getCached(parseInt(id, 10)).pipe(
-          first(),
-          map((user) => user?.username ?? id)
-        )
-      default:
-        return of(id)
-    }
-  }
-}