UFAL/Fetching preview item data only when necessary #898
Conversation
WalkthroughThe changes implement deferred loading of preview data for non-video files in the file description component. Preview data is now fetched only when the user clicks the preview button, rather than immediately on initialization. New state flags and methods manage the preview's visibility and loading state, with immediate loading retained for video files. Additionally, the preview section component was refactored to receive the list of files as an input rather than fetching it internally, and related template adjustments were made accordingly. A new service was introduced to fetch sub-file data for bitstreams, and a corresponding interface was added to model the sub-file API response. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FileDescriptionComponent
participant DataService
User->>FileDescriptionComponent: Clicks "Show Preview" (non-video)
FileDescriptionComponent->>FileDescriptionComponent: set isLoadingPreview = true, isPreviewVisible = true
FileDescriptionComponent->>DataService: loadPreviewData()
DataService-->>FileDescriptionComponent: Returns content URL & thumbnail
FileDescriptionComponent->>FileDescriptionComponent: set isLoadingPreview = false, update observables
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
yarn install v1.22.22 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR defers fetching of non-video file previews until the user clicks "Show Preview," while preserving immediate loading for videos.
- Introduces
isPreviewVisibleandisLoadingPreviewflags to manage preview display and loading state. - Extracts preview logic into a new
loadPreviewDatamethod and addsshowPreviewandisVideohelpers. - Updates template to trigger
showPreview()on click rather than prefetching.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| file-description.component.ts | Refactored preview loading logic into loadPreviewData, added flags and click handler methods |
| file-description.component.html | Added (click)="showPreview()" to preview button to defer data loading |
Comments suppressed due to low confidence (1)
src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.ts:110
- Add unit tests for
showPreview()to verify thatisLoadingPreviewandisPreviewVisibleare set correctly and thatloadPreviewData()is invoked.
public showPreview(): void {
| <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"> </i> | ||
| {{'item.file.description.preview' | translate}} | ||
| </a> |
There was a problem hiding this comment.
[nitpick] Consider using a element or adding $event.preventDefault() in the click handler to avoid default anchor navigation and improve accessibility.
| private loadPreviewData(): void { | ||
| this.content_url$ = this.bitstreamService.findById(this.fileInput.id, true, false, followLink('thumbnail')) | ||
| .pipe(getFirstCompletedRemoteData(), | ||
| switchMap((remoteData: RemoteData<Bitstream>) => { |
There was a problem hiding this comment.
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.
| * It sets flags to show the preview section and a loading indicator, | ||
| * then starts fetching the required data. | ||
| */ | ||
| public showPreview(): void { |
There was a problem hiding this comment.
[nitpick] Consider guarding against repeated clicks (e.g., disable the preview button while isLoadingPreview is true) to prevent multiple concurrent data fetches.
| public showPreview(): void { | |
| public showPreview(): void { | |
| if (this.isLoadingPreview) { | |
| return; // Prevent multiple concurrent calls | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.html(1 hunks)src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.ts (1)
src/app/core/shared/operators.ts (1)
getFirstCompletedRemoteData(41-43)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: dspace-angular / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: tests (16.x)
- GitHub Check: tests (18.x)
🔇 Additional comments (5)
src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.ts (4)
41-48: LGTM! Well-documented state properties.The boolean flags are properly declared with clear documentation explaining their purpose for managing preview visibility and loading state.
66-71: LGTM! Proper conditional loading logic.The conditional logic correctly implements the deferred loading requirement - video files load immediately while other file types wait for user interaction.
105-114: LGTM! Clean method implementation.The
showPreview()method correctly sets the loading state and triggers data loading when the user clicks the preview button.
116-121: LGTM! Simple and clear helper method.The
isVideo()helper method provides a clean way to determine if the file is a video type.src/app/item-page/simple/field-components/preview-section/file-description/file-description.component.html (1)
50-50: LGTM! Proper integration of click handler.The click handler is correctly added to trigger the new preview loading logic while preserving the existing
hrefattribute for Bootstrap collapse functionality. This ensures both the new deferred loading behavior and the existing UI collapse behavior work together seamlessly.
| 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; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Address subscription cleanup and error handling concerns.
The method has several issues that need attention:
- Memory leak risk: The subscription at line 151-153 is not added to the
subscriptionscollection for cleanup - Incomplete error handling: The loading flag is only set to false in the success path, potentially leaving it stuck if errors occur
- 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.
| 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.
|
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/app/core/data/sub-file-data.service.ts (1)
9-65: Add unit tests for the new service.This new service lacks unit tests, which are essential for ensuring reliability and maintainability.
The service should have comprehensive unit tests covering:
- Successful sub-file fetching
- Error handling scenarios
- Data conversion edge cases
- Endpoint URL construction
Would you like me to generate unit test code for this service?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/core/data/sub-file-data.service.ts(1 hunks)src/app/core/metadata/subfile-data.model.ts(1 hunks)src/app/item-page/simple/field-components/preview-section/preview-section.component.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app/core/metadata/subfile-data.model.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: dspace-angular / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: tests (18.x)
- GitHub Check: tests (16.x)
🔇 Additional comments (1)
src/app/core/data/sub-file-data.service.ts (1)
34-34: Please confirm the ‘subfiles’ HAL link exists and document it
Before shipping, ensure that your backend actually exposes a HAL link namedsubfiles. To make this explicit and easier to maintain:
- Extract
'subfiles'into a well-named constant (e.g.SUBFILES_LINK = 'subfiles') at the top ofsrc/app/core/data/sub-file-data.service.ts- Add a brief comment or point to the backend API spec where this link is defined
Location to update:
- src/app/core/data/sub-file-data.service.ts, method
getSubFileEndpoint()(around line 34)
| it('should set listOfFiles on init', () => { | ||
| expect(component.listOfFiles).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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.
| 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)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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.
Problem description
Items are prefetching their previews even without "preview" button clicked, it is unnecessary.
Reported issues
ufal#1212
Summary by CodeRabbit
New Features
Improvements