WEB-627 Implement Report Service in the Loan Application Print Icon #3049
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Note
|
| Cohort / File(s) | Summary |
|---|---|
PDF Viewer Modal UI src/app/clients/clients-view/general-tab/general-tab.component.html |
Adds conditional modal overlay and embedded PDF viewer shown when showPdf is true; includes a close button wired to closePdf(), placed after the collateral table. |
PDF Service Integration & Lifecycle src/app/clients/clients-view/general-tab/general-tab.component.ts |
Replaces direct window.open with ReportsService.getPentahoRunReportData using SettingsService values; creates/revokes Blob URLs, stores pdfUrl/rawPdfUrl/showPdf, adds closePdf() and ngOnDestroy() with destroy$, injects reportsService, settingsService, alertService, and sanitizer, and adds error handling. |
Sequence Diagram
sequenceDiagram
actor User
participant Component as GeneralTabComponent
participant Settings as SettingsService
participant Reports as ReportsService
participant Browser as Browser/DOM
User->>Component: openLoanApplicationReport(loanId)
Component->>Settings: request tenant/locale/dateFormat
Settings-->>Component: return settings
Component->>Reports: getPentahoRunReportData(formData)
Reports-->>Component: return PDF Blob
Component->>Browser: createObjectURL(PDF Blob)
Browser-->>Component: object URL
Component->>Component: set pdfUrl, showPdf = true
Component->>Browser: render modal with embedded PDF
User->>Component: click close button
Component->>Component: closePdf()
Component->>Browser: revokeObjectURL(rawPdfUrl)
Component->>Component: set pdfUrl = null, rawPdfUrl = null, showPdf = false
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
- IOhacker
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title directly matches the main objective of the PR—implementing the report service in the loan application print icon—and accurately reflects the changes made. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
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.
Comment @coderabbitai help to get the list of available commands and usage tips.
JaySoni1
force-pushed
the
WEB-627-implement-report-service-in-the-loan-application-print-icon
branch
from
32152b8 to
a38db57
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/clients/clients-view/general-tab/general-tab.component.ts`:
- Around line 75-89: The openLoanApplicationReport method currently calls
reportsService.getPentahoRunReportData(...).subscribe(...) without error
handling or subscription teardown; add an error handler and ensure the
observable is cleaned up to avoid leaks (either pipe the observable with
takeUntil(this.destroy$) and implement a private destroy$ subject closed in
ngOnDestroy, or capture the Subscription returned and unsubscribe in
ngOnDestroy). In the error handler for the subscribe (or via catchError), set
showPdf = false, clear or revoke any created object URLs, and surface a
user-facing message via the existing UI notification mechanism (or
processLogger) so failures are visible; reference openLoanApplicationReport and
getPentahoRunReportData when making these changes.
🧹 Nitpick comments (4)
src/app/clients/clients-view/general-tab/general-tab.component.ts (2)
91-97: Add OnDestroy lifecycle hook to prevent memory leaks.The
closePdf()method properly revokes the blob URL, but if the component is destroyed (e.g., user navigates away) while the PDF modal is open, the object URL will leak. Consider callingclosePdf()in anngOnDestroyhook.♻️ Proposed fix
-export class GeneralTabComponent { +export class GeneralTabComponent implements OnDestroy { ... + ngOnDestroy(): void { + if (this.pdfUrl) { + URL.revokeObjectURL(this.pdfUrl); + } + }Don't forget to add
OnDestroyto the imports from@angular/core.
11-11: Remove unused import.The
environmentimport is no longer used in this file after refactoring to use ReportsService and SettingsService. Remove it to keep imports clean.Proposed fix
-import { environment } from '../../../../environments/environment';src/app/clients/clients-view/general-tab/general-tab.component.html (2)
898-898: Inconsistent template syntax: use@ifinstead of*ngIf.The rest of the template uses Angular's new control flow syntax (
@if), but this modal uses the legacy*ngIfdirective. For consistency, consider using the new syntax.♻️ Proposed fix
- <div *ngIf="showPdf" class="pdf-modal-overlay"> + `@if` (showPdf) { + <div class="pdf-modal-overlay"> <div class="pdf-modal-content"> <button mat-icon-button class="pdf-modal-close" (click)="closePdf()" aria-label="Close PDF"> <span aria-hidden="true">×</span> </button> <embed [src]="pdfUrl" type="application/pdf" width="100%" height="600px" /> </div> </div> + }
898-905: Consider improving modal accessibility.The modal lacks some accessibility features that would improve the experience for keyboard and screen reader users:
- Add
role="dialog"andaria-modal="true"to the modal container- Consider allowing overlay click or Escape key to close the modal
- Focus should be trapped within the modal while open
♻️ Suggested accessibility improvements
- <div *ngIf="showPdf" class="pdf-modal-overlay"> - <div class="pdf-modal-content"> + <div *ngIf="showPdf" class="pdf-modal-overlay" (click)="closePdf()"> + <div class="pdf-modal-content" role="dialog" aria-modal="true" aria-label="Loan Application PDF" (click)="$event.stopPropagation()"> <button mat-icon-button class="pdf-modal-close" (click)="closePdf()" aria-label="Close PDF"> <span aria-hidden="true">×</span> </button> <embed [src]="pdfUrl" type="application/pdf" width="100%" height="600px" /> </div> </div>For keyboard support, you could add a
@HostListenerfor the Escape key in the component.
JaySoni1
force-pushed
the
WEB-627-implement-report-service-in-the-loan-application-print-icon
branch
from
a38db57 to
0c428dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/clients/clients-view/general-tab/general-tab.component.ts`:
- Around line 88-108: The catchError handler in the observable pipeline (the
block using takeUntil(this.destroy$) and catchError(...)) currently returns an
array which causes the subscribe callback to receive an invalid response; change
it to return RxJS EMPTY instead: keep the existing error cleanup (this.showPdf =
false, URL.revokeObjectURL(this.pdfUrl); this.pdfUrl = null) and the
alertService.alert call, import EMPTY from 'rxjs', and return EMPTY from the
catchError so no downstream emission reaches the subscribe handler that expects
a response with headers.
🧹 Nitpick comments (1)
src/app/clients/clients-view/general-tab/general-tab.component.ts (1)
103-108: Consider adding proper response typing.Using
anyfor the response bypasses TypeScript's type checking. Per Angular coding guidelines, strict type safety should be verified.♻️ Suggested improvement
- .subscribe((res: any) => { - const contentType = res.headers.get('Content-Type') || 'application/pdf'; - const file = new Blob([res.body], { type: contentType }); + .subscribe((res: HttpResponse<ArrayBuffer>) => { + const contentType = res.headers?.get('Content-Type') || 'application/pdf'; + const file = new Blob([res.body!], { type: contentType }); this.pdfUrl = URL.createObjectURL(file); this.showPdf = true; });This requires importing
HttpResponsefrom@angular/common/http.
JaySoni1
force-pushed
the
WEB-627-implement-report-service-in-the-loan-application-print-icon
branch
from
0c428dd to
891dcd8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/app/clients/clients-view/general-tab/general-tab.component.html`:
- Line 903: The <embed> src binding uses a raw blob URL; inject Angular's
DomSanitizer into the component and change pdfUrl to type SafeResourceUrl, then
when creating the blob URL via URL.createObjectURL(...) wrap it with
sanitizer.bypassSecurityTrustResourceUrl(...) and assign that to pdfUrl while
keeping the raw blob URL in a separate variable (e.g., rawPdfUrl) so you can
still call URL.revokeObjectURL(rawPdfUrl) during cleanup; update any references
to pdfUrl and ensure the sanitizer instance is used (DomSanitizer and
bypassSecurityTrustResourceUrl) to match the pattern used in pentaho
reports/view receipts.
In `@src/app/clients/clients-view/general-tab/general-tab.component.ts`:
- Line 11: Remove the unused import of environment from
'../../../../environments/environment' at the top of general-tab.component.ts;
locate the import statement (import { environment } from
'../../../../environments/environment';) in the GeneralTabComponent file and
delete it so the file no longer contains an unused import.
🧹 Nitpick comments (5)
src/app/clients/clients-view/general-tab/general-tab.component.ts (3)
40-43: Consider consolidating RxJS imports.
SubjectandEMPTYcan be imported together fromrxjsfor cleaner organization.♻️ Suggested consolidation
-import { Subject } from 'rxjs'; import { takeUntil, catchError } from 'rxjs/operators'; import { AlertService } from 'app/core/alert/alert.service'; -import { EMPTY } from 'rxjs'; +import { Subject, EMPTY } from 'rxjs';
91-101: Remove incorrect: anyreturn type fromcatchError.The
catchErrorhandler returnsEMPTY(which isObservable<never>), but the: anyannotation is misleading and defeats type safety.♻️ Suggested fix
- catchError((error): any => { + catchError((error) => {
104-109: Add proper typing for the response.Using
anybypasses TypeScript's type checking. Per coding guidelines for strict type safety, consider typing the response asHttpResponse<Blob>.♻️ Suggested improvement
Add the import:
import { HttpResponse } from '@angular/common/http';Then update the subscribe:
- .subscribe((res: any) => { + .subscribe((res: HttpResponse<Blob>) => { const contentType = res.headers.get('Content-Type') || 'application/pdf'; - const file = new Blob([res.body], { type: contentType }); + const file = new Blob([res.body!], { type: contentType });src/app/clients/clients-view/general-tab/general-tab.component.html (2)
898-905: Modal lacks keyboard accessibility.Consider adding:
- Escape key handler to close the modal
- Focus trapping to keep keyboard focus within the modal
- Use Angular Material's dialog or CDK overlay for built-in accessibility features
Using Angular Material's
MatDialogwould provide focus trapping, escape handling, and consistent styling out of the box. Alternatively, add a@HostListenerfor the Escape key:`@HostListener`('document:keydown.escape') onEscapeKey() { if (this.showPdf) { this.closePdf(); } }
900-902: Usemat-iconfor consistency with Material Design.The close button uses
mat-icon-buttonbut contains a raw×character instead of a Material icon.♻️ Suggested fix
- <button mat-icon-button class="pdf-modal-close" (click)="closePdf()" aria-label="Close PDF"> - <span aria-hidden="true">×</span> - </button> + <button mat-icon-button class="pdf-modal-close" (click)="closePdf()" aria-label="Close PDF"> + <mat-icon>close</mat-icon> + </button>Ensure
MatIconModuleis imported in the component.
src/app/clients/clients-view/general-tab/general-tab.component.ts
Outdated
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-627-implement-report-service-in-the-loan-application-print-icon
branch
from
891dcd8 to
43d8a5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/clients/clients-view/general-tab/general-tab.component.ts`:
- Around line 83-113: In openLoanApplicationReport, revoke any existing blob URL
stored in rawPdfUrl before creating and assigning a new one to avoid leaking
object URLs: check if this.rawPdfUrl exists, call
URL.revokeObjectURL(this.rawPdfUrl) and set this.rawPdfUrl = null (and
optionally this.pdfUrl = null) immediately before creating the new Blob/URL (the
URL.createObjectURL call) so the old object URL is released; keep the existing
error-path revoke logic intact.
43d8a5a to
5f3465b
Compare
|
@IOhacker Thank You for the review |
2 participants
Chnages Made :-
-Implement report service in the loan application print icon.
WEB-627
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.