pdf file unicode font support added#118
Conversation
Deployment to QA
Added Limit offset for the API
add tenantId validation in user certificate status search api
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdates Dockerfile dependency management and Chrome installation for Puppeteer. Enhances certificate PDF rendering endpoint typing and download behavior. Adds Devanagari font injection to certificate HTML, improves Puppeteer PDF generation with viewport sizing and network wait conditions, and modifies response handling from direct streaming to StreamableFile returns. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant Service
participant Puppeteer
participant Chrome
participant Response
Client->>Controller: POST /render-pdf (HTML)
Controller->>Service: renderCertificatePDFFromHTML()
Service->>Service: Check if fonts injected
alt Fonts not present
Service->>Service: Inject Devanagari fonts
Service->>Service: Add UTF-8 charset meta
Service->>Service: Wrap in HTML structure
else Fonts present
Service->>Service: Ensure UTF-8 charset
end
Service->>Puppeteer: Launch browser with font hints
Puppeteer->>Chrome: Create new page (1200x1600)
Chrome->>Chrome: Set viewport
Service->>Puppeteer: setContent(HTML)
Puppeteer->>Chrome: Render page
Chrome->>Chrome: Wait networkIdle0
Chrome->>Chrome: Wait document.fonts.ready
Service->>Puppeteer: pdf() generate buffer
Puppeteer->>Chrome: Capture PDF
Chrome-->>Service: PDF buffer
Service->>Service: Create StreamableFile
Service-->>Controller: StreamableFile
Controller->>Controller: Set PDF headers
Controller->>Response: Send with filename
Response-->>Client: PDF download
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. 🔧 Trivy (0.58.1)Failed to read Trivy output file: ENOENT: no such file or directory, open '/inmem/1253/nsjail-53c8a3b0-4446-4732-895a-7cd1cf5135cb/merged/.trivy-output.json' Comment |
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/certificate/certificate.service.ts (1)
397-401: 🛠️ Refactor suggestion | 🟠 MajorRemove unused
resparameter.The
res: Responseparameter is no longer used in this method since the return type changed toStreamableFile. Keeping it creates confusion and requires the controller to pass an unnecessary argument.♻️ Suggested fix
async renderPDFFromHTML( credentialId: string, templateId: string, - res: Response, ): Promise<StreamableFile> {
🤖 Fix all issues with AI agents
In `@src/modules/certificate/certificate.contoller.ts`:
- Around line 107-113: The controller is passing a now-unused Response object
into CertificateService.renderPDFFromHTML; remove the response argument from the
call and update the service method signature to no longer accept Response. Edit
the controller method that currently injects `@Res`({ passthrough: true })
response: Response to stop forwarding response into
certificateService.renderPDFFromHTML (and remove the response parameter entirely
if it's not used elsewhere), and update CertificateService.renderPDFFromHTML to
accept only (credentialId, templateId) and adjust any internal usages
accordingly.
🧹 Nitpick comments (3)
Dockerfile (1)
5-6: Consider formatting the apt-get command for better readability.The single-line dependency list is functional but hard to maintain. Multi-line formatting would improve readability and make future additions/removals easier to review in diffs.
♻️ Suggested formatting
-RUN apt-get update && apt-get install -y libnss3 libatk1.0-0 libatk-bridge2.0-0 libcups2 libdrm2 libxkbcommon-x11-0 libgbm1 libasound2 libxshmfence1 fonts-liberation fonts-noto fonts-noto-core fonts-noto-ui-core fonts-deva fonts-indic gconf-service libappindicator3-1 libnspr4 libx11-xcb1 xdg-utils --no-install-recommends && rm -rf /var/lib/apt/lists/* +RUN apt-get update && apt-get install -y \ + libnss3 \ + libatk1.0-0 \ + libatk-bridge2.0-0 \ + libcups2 \ + libdrm2 \ + libxkbcommon-x11-0 \ + libgbm1 \ + libasound2 \ + libxshmfence1 \ + fonts-liberation \ + fonts-noto \ + fonts-noto-core \ + fonts-noto-ui-core \ + fonts-deva \ + fonts-indic \ + gconf-service \ + libappindicator3-1 \ + libnspr4 \ + libx11-xcb1 \ + xdg-utils \ + --no-install-recommends \ + && rm -rf /var/lib/apt/lists/*src/modules/certificate/certificate.service.ts (2)
418-461: Font injection logic is complex — consider extracting to a helper method.The nested conditionals handling various HTML structures (with/without
<head>,<html>, charset) work correctly but reduce readability. Extracting this to a dedicatedinjectFontSupport(html: string): stringmethod would improve maintainability and testability.
419-422: Consider making the Google Fonts URL configurable.The hardcoded Google Fonts URL ties the service to an external dependency. If the fonts CDN becomes unavailable or needs to change, a code change would be required. Consider using
ConfigServicefor the font URL or at least defining it as a constant.
| @Res({ passthrough: true }) response: Response, | ||
| ): Promise<StreamableFile> { | ||
| const streamableFile = await this.certificateService.renderPDFFromHTML( | ||
| renderCertificateDto.credentialId, | ||
| renderCertificateDto.templateId, | ||
| response, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused response parameter from service call.
The response parameter is passed to renderPDFFromHTML but is no longer used in the service (as noted in the service file review). Update the service signature and this call site accordingly.
♻️ Suggested fix
- async renderCertificatePDFFromHTML(
- `@Body`() renderCertificateDto: RenderCertificateDTO,
- `@Res`({ passthrough: true }) response: Response,
- ): Promise<StreamableFile> {
- const streamableFile = await this.certificateService.renderPDFFromHTML(
- renderCertificateDto.credentialId,
- renderCertificateDto.templateId,
- response,
- );
+ async renderCertificatePDFFromHTML(
+ `@Body`() renderCertificateDto: RenderCertificateDTO,
+ `@Res`({ passthrough: true }) response: Response,
+ ): Promise<StreamableFile> {
+ const streamableFile = await this.certificateService.renderPDFFromHTML(
+ renderCertificateDto.credentialId,
+ renderCertificateDto.templateId,
+ );📝 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.
| @Res({ passthrough: true }) response: Response, | |
| ): Promise<StreamableFile> { | |
| const streamableFile = await this.certificateService.renderPDFFromHTML( | |
| renderCertificateDto.credentialId, | |
| renderCertificateDto.templateId, | |
| response, | |
| ); | |
| `@Res`({ passthrough: true }) response: Response, | |
| ): Promise<StreamableFile> { | |
| const streamableFile = await this.certificateService.renderPDFFromHTML( | |
| renderCertificateDto.credentialId, | |
| renderCertificateDto.templateId, | |
| ); |
🤖 Prompt for AI Agents
In `@src/modules/certificate/certificate.contoller.ts` around lines 107 - 113, The
controller is passing a now-unused Response object into
CertificateService.renderPDFFromHTML; remove the response argument from the call
and update the service method signature to no longer accept Response. Edit the
controller method that currently injects `@Res`({ passthrough: true }) response:
Response to stop forwarding response into certificateService.renderPDFFromHTML
(and remove the response parameter entirely if it's not used elsewhere), and
update CertificateService.renderPDFFromHTML to accept only (credentialId,
templateId) and adjust any internal usages accordingly.
2 participants
No description provided.