Skip to content

UFAL/Fetching preview item data only when necessary #898

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.

Sign up for GitHub

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

Open
jr-rk wants to merge 7 commits into
base: dtq-dev
Choose a base branch
from
Open

UFAL/Fetching preview item data only when necessary #898

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e9fec39
Changed loading previewed data from init to separate callable function
jr-rk Jul 4, 2025
6ed891f
Removed additional fetching the same data as in upper layer - passing…
jr-rk Jul 8, 2025
2b20a51
Removed unused imports
jr-rk Jul 9, 2025
f1df9c2
Fixed spec test to array - not behavioursubject anymore
jr-rk Jul 9, 2025
0cfb871
Fixed unit tests
jr-rk Jul 9, 2025
d98d7b6
Revert "Fixed unit tests"
jr-rk Jul 10, 2025
a71185e
Revert "Fixed spec test to array - not behavioursubject anymore"
jr-rk Jul 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions src/app/core/data/sub-file-data.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import { Injectable } from '@angular/core';
import { HttpClient } from '@angular/common/http';
import { Observable } from 'rxjs';
import { map, mergeMap } from 'rxjs/operators';
import { FileInfo } from '../metadata/file-info.model';
import { HALEndpointService } from '../shared/hal-endpoint.service';
import { SubFileResponse } from '../metadata/subfile-data.model';

@Injectable({ providedIn: 'root' })
export class SubFileDataService {
constructor(
protected http: HttpClient,
protected halService: HALEndpointService
) {}

/**
* Fetch sub-files for a specific bitstream (zip/directory)
* @param bitstreamId The ID of the bitstream containing sub-files
* @returns Observable of FileInfo[] representing the sub-files
*/
fetchSubFiles(bitstreamId: string): Observable<FileInfo[]> {
return this.getSubFileEndpoint().pipe(
map((endpoint) => this.getSubFilesRequestURL(endpoint, bitstreamId)),
mergeMap((requestUrl) => this.http.get<SubFileResponse>(requestUrl)),
map((data: SubFileResponse) => this.convertToFileInfo(data))
);
}

@coderabbitai coderabbitai Bot Jul 9, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling to the main service method.

The service should handle potential errors in the observable chain to provide better error handling for consuming components.

+import { catchError } from 'rxjs/operators';
+import { throwError } from 'rxjs';

  fetchSubFiles(bitstreamId: string): Observable<FileInfo[]> {
    return this.getSubFileEndpoint().pipe(
      map((endpoint) => this.getSubFilesRequestURL(endpoint, bitstreamId)),
      mergeMap((requestUrl) => this.http.get<SubFileResponse>(requestUrl)),
-     map((data: SubFileResponse) => this.convertToFileInfo(data))
+     map((data: SubFileResponse) => this.convertToFileInfo(data)),
+     catchError((error) => {
+       console.error('Failed to fetch sub-files:', error);
+       return throwError(error);
+     })
    );
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fetchSubFiles(bitstreamId: string): Observable<FileInfo[]> {
return this.getSubFileEndpoint().pipe(
map((endpoint) => this.getSubFilesRequestURL(endpoint, bitstreamId)),
mergeMap((requestUrl) => this.http.get<SubFileResponse>(requestUrl)),
map((data: SubFileResponse) => this.convertToFileInfo(data))
);
}
import { catchError } from 'rxjs/operators';
import { throwError } from 'rxjs';
fetchSubFiles(bitstreamId: string): Observable<FileInfo[]> {
return this.getSubFileEndpoint().pipe(
map((endpoint) => this.getSubFilesRequestURL(endpoint, bitstreamId)),
mergeMap((requestUrl) => this.http.get<SubFileResponse>(requestUrl)),
map((data: SubFileResponse) => this.convertToFileInfo(data)),
catchError((error) => {
console.error('Failed to fetch sub-files:', error);
return throwError(error);
})
);
}
🤖 Prompt for AI Agents 
In src/app/core/data/sub-file-data.service.ts around lines 21 to 27, the
fetchSubFiles method lacks error handling in its observable chain. Add an error
handling operator such as catchError to the observable pipeline to catch and
handle any errors that occur during the HTTP request or data processing. This
should return a user-friendly error or fallback value and ensure the observable
does not fail silently, improving robustness for consuming components.


/**
* Get the base endpoint for sub-file requests
*/
private getSubFileEndpoint(): Observable<string> {
// This should match the endpoint defined in the backend
return this.halService.getEndpoint('subfiles');
}

/**
* Construct the complete URL for fetching sub-files
*/
private getSubFilesRequestURL(endpoint: string, bitstreamId: string): string {
return `${endpoint}/${bitstreamId}`;
}

/**
* Convert the raw hashtable data from the API to FileInfo objects
*/
private convertToFileInfo(data: SubFileResponse): FileInfo[] {
const result: FileInfo[] = [];

Object.keys(data).forEach(key => {
const subFileData = data[key];
const fileInfo = new FileInfo();

// Map properties from the API response to FileInfo
fileInfo.name = subFileData.name || key;
fileInfo.content = subFileData.content;
fileInfo.size = subFileData.size;
fileInfo.isDirectory = !!subFileData.isDirectory;

result.push(fileInfo);
});

return result;
}

@coderabbitai coderabbitai Bot Jul 9, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and null safety in data conversion.

The conversion method should handle potential null/undefined values more robustly to prevent runtime errors.

  private convertToFileInfo(data: SubFileResponse): FileInfo[] {
    const result: FileInfo[] = [];
+   
+   if (!data || typeof data !== 'object') {
+     return result;
+   }

    Object.keys(data).forEach(key => {
      const subFileData = data[key];
+     if (!subFileData) {
+       return; // Skip null/undefined entries
+     }
+     
      const fileInfo = new FileInfo();

      // Map properties from the API response to FileInfo
      fileInfo.name = subFileData.name || key;
      fileInfo.content = subFileData.content;
-     fileInfo.size = subFileData.size;
+     fileInfo.size = subFileData.size || 0;
      fileInfo.isDirectory = !!subFileData.isDirectory;

      result.push(fileInfo);
    });

    return result;
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private convertToFileInfo(data: SubFileResponse): FileInfo[] {
const result: FileInfo[] = [];
Object.keys(data).forEach(key => {
const subFileData = data[key];
const fileInfo = new FileInfo();
// Map properties from the API response to FileInfo
fileInfo.name = subFileData.name || key;
fileInfo.content = subFileData.content;
fileInfo.size = subFileData.size;
fileInfo.isDirectory = !!subFileData.isDirectory;
result.push(fileInfo);
});
return result;
}
private convertToFileInfo(data: SubFileResponse): FileInfo[] {
const result: FileInfo[] = [];
if (!data || typeof data !== 'object') {
return result;
}
Object.keys(data).forEach(key => {
const subFileData = data[key];
if (!subFileData) {
return; // Skip null/undefined entries
}
const fileInfo = new FileInfo();
// Map properties from the API response to FileInfo
fileInfo.name = subFileData.name || key;
fileInfo.content = subFileData.content;
fileInfo.size = subFileData.size || 0;
fileInfo.isDirectory = !!subFileData.isDirectory;
result.push(fileInfo);
});
return result;
}
🤖 Prompt for AI Agents 
In src/app/core/data/sub-file-data.service.ts around lines 47 to 64, the
convertToFileInfo method lacks checks for null or undefined values in the input
data and its properties, which can cause runtime errors. Add null and undefined
checks before accessing properties of subFileData and ensure data itself is
valid before processing. Use safe access patterns or conditional checks to
handle missing or malformed data gracefully, returning an empty array or
skipping invalid entries as appropriate.

}
13 changes: 13 additions & 0 deletions src/app/core/metadata/subfile-data.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* Represents the raw data structure returned from the API for sub-files
* within zip archives or directories
*/
export interface SubFileResponse {
// The hashtable structure returned from the API
[key: string]: {
name?: string;
content?: any;
size?: string;
isDirectory?: boolean;
};
}
2 changes: 1 addition & 1 deletion src/app/item-page/clarin-files-section/clarin-files-section.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,5 @@ <h6><i class="fa fa-paperclip">&nbsp;</i>{{'item.page.files.head' | translate}}<
</a>
</span>
</div>
<ds-preview-section [item]="item"></ds-preview-section>
<ds-preview-section [item]="item" [listOfFiles]="(listOfFiles | async)"></ds-preview-section>
</ng-container>
2 changes: 1 addition & 1 deletion .../simple/field-components/preview-section/file-description/file-description.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
</span>
<span class="pl-1">
<a class="preview-btn collapsed" data-toggle="collapse" *ngIf="couldPreview()" role="button"
href="#file_file_{{ fileInput.id }}">
(click)="showPreview()" href="#file_file_{{ fileInput.id }}">
<i class="fa fa-eye">&nbsp;</i>
{{'item.file.description.preview' | translate}}
</a>
Comment on lines 49 to 53

Copilot AI Jul 4, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider using a element or adding $event.preventDefault() in the click handler to avoid default anchor navigation and improve accessibility.

Copilot uses AI. Check for mistakes.
Expand Down
90 changes: 66 additions & 24 deletions ...ge/simple/field-components/preview-section/file-description/file-description.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ export class FileDescriptionComponent implements OnInit, OnDestroy {
handlers_added = false;
playPromise: Promise<void>;

/**
* Whether the user has clicked the "Show Preview" button for non-video files.
*/
public isPreviewVisible = false;
/**
* Whether the preview data is currently being loaded for non-video files.
*/
public isLoadingPreview = false;

private subscriptions: Subscription = new Subscription();

constructor(protected halService: HALEndpointService,
Expand All @@ -54,30 +63,12 @@ export class FileDescriptionComponent implements OnInit, OnDestroy {
.subscribe(remoteData => {
this.emailToContact = remoteData?.payload?.values?.[0];
});
this.content_url$ = this.bitstreamService.findById(this.fileInput.id, true, false, followLink('thumbnail'))
.pipe(getFirstCompletedRemoteData(),
switchMap((remoteData: RemoteData<Bitstream>) => {
if (remoteData.hasSucceeded) {
if (remoteData.payload?.thumbnail){
this.thumbnail_url$ = remoteData.payload?.thumbnail.pipe(
switchMap((thumbnailRD: RemoteData<Bitstream>) => {
if (thumbnailRD.hasSucceeded) {
return this.buildUrl(thumbnailRD.payload?._links.content.href);
} else {
return of('');
}
}),
);
} else {
this.thumbnail_url$ = of('');
}
return of(remoteData.payload?._links.content.href);
}
}
));
this.content_url$.pipe(take(1)).subscribe((url) => {
this.content_url = url;
});

// If the file is a video, load its data immediately as before.
// For all other file types, we will wait for the user to click a button.
if (this.isVideo()) {
this.loadPreviewData();
}
}

ngAfterViewInit() {
Expand Down Expand Up @@ -111,6 +102,57 @@ export class FileDescriptionComponent implements OnInit, OnDestroy {
}
}

/**
* Called when the user clicks the "Show Preview" button for a non-video file.
* It sets flags to show the preview section and a loading indicator,
* then starts fetching the required data.
*/
public showPreview(): void {

Copilot AI Jul 4, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider guarding against repeated clicks (e.g., disable the preview button while isLoadingPreview is true) to prevent multiple concurrent data fetches.

Suggested change
public showPreview(): void {
public showPreview(): void {
if (this.isLoadingPreview) {
return; // Prevent multiple concurrent calls
}

Copilot uses AI. Check for mistakes.
this.isLoadingPreview = true;
this.isPreviewVisible = true;
this.loadPreviewData();
}

/**
* A helper method to determine if the file is a video.
*/
public isVideo(): boolean {
return this.fileInput?.format.startsWith('video/');
}

/**
* Fetches the bitstream content URL and thumbnail.
* It is called on-demand for other previews.
*/
private loadPreviewData(): void {
this.content_url$ = this.bitstreamService.findById(this.fileInput.id, true, false, followLink('thumbnail'))
.pipe(getFirstCompletedRemoteData(),
switchMap((remoteData: RemoteData<Bitstream>) => {

Copilot AI Jul 4, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The switchMap callback returns an Observable only when hasSucceeded is true; add a default return of('') (or similar) for the failure path to avoid emitting undefined.

Copilot uses AI. Check for mistakes.
// Hide loading indicator once the request is complete
this.isLoadingPreview = false;
if (remoteData.hasSucceeded) {
if (remoteData.payload?.thumbnail){
this.thumbnail_url$ = remoteData.payload?.thumbnail.pipe(
switchMap((thumbnailRD: RemoteData<Bitstream>) => {
if (thumbnailRD.hasSucceeded) {
return this.buildUrl(thumbnailRD.payload?._links.content.href);
} else {
return of('');
}
}),
);
} else {
this.thumbnail_url$ = of('');
}
return of(remoteData.payload?._links.content.href);
}
}
));
this.content_url$.pipe(take(1)).subscribe((url) => {
this.content_url = url;
});
}
Comment on lines +127 to +154

@coderabbitai coderabbitai Bot Jul 4, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Address subscription cleanup and error handling concerns.

The method has several issues that need attention:

  1. Memory leak risk: The subscription at line 151-153 is not added to the subscriptions collection for cleanup
  2. Incomplete error handling: The loading flag is only set to false in the success path, potentially leaving it stuck if errors occur
  3. High complexity: The nested observables make the method difficult to maintain

Apply this diff to fix the subscription cleanup:

-    this.content_url$.pipe(take(1)).subscribe((url) => {
-      this.content_url = url;
-    });
+    this.subscriptions.add(
+      this.content_url$.pipe(take(1)).subscribe((url) => {
+        this.content_url = url;
+      })
+    );

Consider refactoring to improve error handling:

   private loadPreviewData(): void {
     this.content_url$ = this.bitstreamService.findById(this.fileInput.id, true, false, followLink('thumbnail'))
       .pipe(getFirstCompletedRemoteData(),
             switchMap((remoteData: RemoteData<Bitstream>) => {
-              // Hide loading indicator once the request is complete
-              this.isLoadingPreview = false;
               if (remoteData.hasSucceeded) {
                 if (remoteData.payload?.thumbnail){
                   this.thumbnail_url$ = remoteData.payload?.thumbnail.pipe(
                     switchMap((thumbnailRD: RemoteData<Bitstream>) => {
                       if (thumbnailRD.hasSucceeded) {
                         return this.buildUrl(thumbnailRD.payload?._links.content.href);
                       } else {
                         return of('');
                       }
                     }),
                   );
                 } else {
                   this.thumbnail_url$ = of('');
                 }
                 return of(remoteData.payload?._links.content.href);
               }
+              return of('');
             }
       ));
     this.subscriptions.add(
       this.content_url$.pipe(take(1)).subscribe((url) => {
         this.content_url = url;
+        // Hide loading indicator once the request is complete
+        this.isLoadingPreview = false;
       })
     );
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private loadPreviewData(): void {
this.content_url$ = this.bitstreamService.findById(this.fileInput.id, true, false, followLink('thumbnail'))
.pipe(getFirstCompletedRemoteData(),
switchMap((remoteData: RemoteData<Bitstream>) => {
// Hide loading indicator once the request is complete
this.isLoadingPreview = false;
if (remoteData.hasSucceeded) {
if (remoteData.payload?.thumbnail){
this.thumbnail_url$ = remoteData.payload?.thumbnail.pipe(
switchMap((thumbnailRD: RemoteData<Bitstream>) => {
if (thumbnailRD.hasSucceeded) {
return this.buildUrl(thumbnailRD.payload?._links.content.href);
} else {
return of('');
}
}),
);
} else {
this.thumbnail_url$ = of('');
}
return of(remoteData.payload?._links.content.href);
}
}
));
this.content_url$.pipe(take(1)).subscribe((url) => {
this.content_url = url;
});
}
private loadPreviewData(): void {
this.content_url$ = this.bitstreamService.findById(this.fileInput.id, true, false, followLink('thumbnail'))
.pipe(
getFirstCompletedRemoteData(),
switchMap((remoteData: RemoteData<Bitstream>) => {
if (remoteData.hasSucceeded) {
if (remoteData.payload?.thumbnail) {
this.thumbnail_url$ = remoteData.payload?.thumbnail.pipe(
switchMap((thumbnailRD: RemoteData<Bitstream>) => {
if (thumbnailRD.hasSucceeded) {
return this.buildUrl(thumbnailRD.payload?._links.content.href);
} else {
return of('');
}
}),
);
} else {
this.thumbnail_url$ = of('');
}
return of(remoteData.payload?._links.content.href);
}
return of('');
})
);
this.subscriptions.add(
this.content_url$.pipe(take(1)).subscribe((url) => {
this.content_url = url;
// Hide loading indicator once the request is complete
this.isLoadingPreview = false;
})
);
}
🤖 Prompt for AI Agents 
In
src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.ts
lines 127 to 154, fix the memory leak by adding the subscription from
content_url$ to the component's subscriptions collection for proper cleanup.
Also, ensure the isLoadingPreview flag is set to false in both success and error
paths by handling errors in the observable chain. Refactor the nested switchMap
calls to flatten the observable structure, improving readability and
maintainability.


private add_short_lived_token_handling_to_video_playback(video: HTMLVideoElement) {
if (this.handlers_added) {
return;
Expand Down
4 changes: 2 additions & 2 deletions src/app/item-page/simple/field-components/preview-section/preview-section.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
<!-- Loading Spinner -->
<div *ngIf="(listOfFiles | async)?.length === 0" class="d-flex flex-column justify-content-center align-items-center my-3">
<div *ngIf="listOfFiles?.length === 0" class="d-flex flex-column justify-content-center align-items-center my-3">
<div class="spinner-border text-primary" role="status"></div>
<div class="px-4 pt-2">{{'item.preview.loading-files' | translate}}
<a *ngIf="emailToContact" [href]="'mailto:' + emailToContact" class="email-text">{{ emailToContact }}</a>
</div>
</div>
<div *ngFor="let file of (listOfFiles | async)">
<div *ngFor="let file of listOfFiles">
<ds-file-description [fileInput] = 'file'></ds-file-description>
</div>
7 changes: 2 additions & 5 deletions src/app/item-page/simple/field-components/preview-section/preview-section.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,7 @@ describe('PreviewSectionComponent', () => {
expect(mockRegistryService.getMetadataBitstream).toHaveBeenCalled();
});

it('should set listOfFiles on init', (done) => {
component.listOfFiles.subscribe((files) => {
expect(files).toEqual([]);
done();
});
it('should set listOfFiles on init', () => {
expect(component.listOfFiles).toEqual([]);
});

@coderabbitai coderabbitai Bot Jul 9, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Initialize the input property in the test setup.

Since listOfFiles is now an @Input() property (as mentioned in the AI summary), this test assertion will fail because the input property is not initialized. Add the input property initialization in the test setup.

  beforeEach(() => {
    fixture = TestBed.createComponent(PreviewSectionComponent);
    component = fixture.componentInstance;
    
+   // Initialize the input property
+   component.listOfFiles = [];
+
    // Set up the mock service's getMetadataBitstream method to return a simple stream
🤖 Prompt for AI Agents 
In
src/app/item-page/simple/field-components/preview-section/preview-section.component.spec.ts
around lines 82 to 84, the test checks the value of the @Input() property
listOfFiles without initializing it, causing the test to fail. Fix this by
setting the listOfFiles input property to an empty array in the test setup
before running the assertion, ensuring the component input is properly
initialized.

});
14 changes: 2 additions & 12 deletions src/app/item-page/simple/field-components/preview-section/preview-section.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import { Component, Input, OnInit } from '@angular/core';
import { BehaviorSubject } from 'rxjs';
import { MetadataBitstream } from 'src/app/core/metadata/metadata-bitstream.model';
import { RegistryService } from 'src/app/core/registry/registry.service';
import { Item } from 'src/app/core/shared/item.model';
import { getAllSucceededRemoteListPayload } from 'src/app/core/shared/operators';
import { ConfigurationDataService } from '../../../../core/data/configuration-data.service';

@Component({
Expand All @@ -13,20 +10,13 @@ import { ConfigurationDataService } from '../../../../core/data/configuration-da
})
export class PreviewSectionComponent implements OnInit {
@Input() item: Item;
@Input() listOfFiles: MetadataBitstream[];

listOfFiles: BehaviorSubject<MetadataBitstream[]> = new BehaviorSubject<MetadataBitstream[]>([] as any);
emailToContact: string;

constructor(protected registryService: RegistryService,
private configService: ConfigurationDataService) {} // Modified
constructor(private configService: ConfigurationDataService) {} // Modified

ngOnInit(): void {
this.registryService
.getMetadataBitstream(this.item.handle, 'ORIGINAL')
.pipe(getAllSucceededRemoteListPayload())
.subscribe((data: MetadataBitstream[]) => {
this.listOfFiles.next(data);
});
this.configService.findByPropertyName('lr.help.mail')?.subscribe(remoteData => {
this.emailToContact = remoteData.payload?.values?.[0];
});
Expand Down