Conversation
8393acf to
2654d3a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1209 +/- ##
==========================================
- Coverage 63.89% 63.72% -0.17%
==========================================
Files 149 149
Lines 5511 5511
Branches 1077 1077
==========================================
- Hits 3521 3512 -9
- Misses 1688 1697 +9
Partials 302 302
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
80fd807 to
c4c3833
Compare
What could become a standalone npm package?The generic parts of this pipeline that other console.redhat.com tenants would want: Package:
|
5082f39 to
01ff4c2
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/pdf/pdfHeader.tsx" line_range="13-22" />
<code_context>
+import { type PropsWithChildren } from 'react';
+import { renderToStaticMarkup } from 'react-dom/server';
+
+function getHeaderDate(): string {
+ const date = new Date();
+ const day = date.getDate();
+ const year = date.getFullYear();
+ return `${day} ${date.toLocaleString('en-us', {
+ month: 'short',
+ })} ${year} ${date.toLocaleString('en-us', {
+ hour: '2-digit',
+ hour12: false,
+ minute: 'numeric',
+ })} UTC`;
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** getHeaderDate formats the server's local date and time but appends `UTC`, so the PDF header shows an incorrect timestamp whenever the PDF server runs outside UTC.
**Triggers:** When the PDF server timezone is not UTC.
**Suggested fix:** Pass `timeZone: 'UTC'` to both `toLocaleString` calls and use UTC getters for the day and year.
```suggestion
const date = new Date();
const day = date.getUTCDate();
const year = date.getUTCFullYear();
return `${day} ${date.toLocaleString('en-us', {
month: 'short',
timeZone: 'UTC',
})} ${year} ${date.toLocaleString('en-us', {
hour: '2-digit',
hour12: false,
minute: 'numeric',
timeZone: 'UTC',
})} UTC`;
```
</issue_to_address>
### Comment 2
<location path="src/pdf/pdfRenderer.tsx" line_range="34-46" />
<code_context>
+const VIEWPORT_WIDTH = (A4_HEIGHT_MM - 20) * 4; // 1108
+const VIEWPORT_HEIGHT = (A4_WIDTH_MM - 40) * 4; // 680
+
+let browserInstance: Browser | null = null;
+
+async function getBrowser(): Promise<Browser> {
+ if (browserInstance && browserInstance.connected) {
+ return browserInstance;
+ }
+ const executablePath = process.env.CHROME_PATH || undefined;
</code_context>
<issue_to_address>
**issue (bug_risk):** Concurrent printPdf calls can both observe `browserInstance` as null and launch separate Puppeteer browsers; only the last browser is retained, so earlier browser processes are leaked and accumulate under concurrent exports.
**Triggers:** When multiple PDF requests start before the first Puppeteer launch completes.
**Suggested fix:** Serialize browser initialization with a shared launch promise, or otherwise ensure only one launch can occur at a time.
</issue_to_address>
### Comment 3
<location path="src/pdf/lightwellLogomark.ts" line_range="11-16" />
<code_context>
+import { readFileSync } from 'fs';
+import { resolve, dirname } from 'path';
+
+const svgPath = resolve(
+ dirname(require.resolve('frontend-assets/package.json')),
+ 'src',
+ 'partners-icons',
+ 'lightwell-logomark.svg',
+);
+
+export const LIGHTWELL_LOGOMARK_SVG = readFileSync(svgPath, 'utf-8');
</code_context>
<issue_to_address>
**issue (bug_risk):** The PDF server resolves `frontend-assets` unconditionally during module initialization even though the package is declared optional; an install that omits or cannot install the optional dependency makes `yarn start:pdf` fail before the server starts.
**Triggers:** When the optional `frontend-assets` dependency is absent or its GitHub install fails.
**Suggested fix:** Declare `frontend-assets` as a required dependency for the PDF server, or provide a fallback logo path and avoid unconditional `require.resolve`.
</issue_to_address>Sourcery assessment
Approval pending. 3 findings to address first.
Blocking findings: src/pdf/pdfHeader.tsx:22, src/pdf/pdfRenderer.tsx:46, src/pdf/lightwellLogomark.ts:16
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| itemCount: count, | ||
| }) as unknown as PDFRequestPayload, | ||
| }); | ||
| const data = await fetchAllFilteredVulnerabilities(customerId, filters); |
There was a problem hiding this comment.
EXPORT_PAGE_SIZE = 200 only pages API reads; PDF generation still accumulates every row, sends one JSON body, and renders one SSR/Chromium document. Could we document the intended dataset limit and test one representative large report before carrying this design forward?
| const page = await browser.newPage(); | ||
|
|
||
| const renderId = randomUUID(); | ||
| pendingRenders.set(renderId, html); |
There was a problem hiding this comment.
Separate from the browser-launch race: each concurrent request creates a Chromium page and retains its full HTML until printing completes. Since isExporting only protects one mounted menu, could we bound concurrent renders with a small queue or semaphore before carrying this design forward?
Bypass the external crc-pdf-generator microservice by rendering Beacon PDFs directly via a lightweight Express + Puppeteer server. This cuts export time from ~20s to under 3s. - Add src/pdf/ module: SSR renderer, Puppeteer printer, Express server with mock-data mode - Rewrite ExportMenu to POST /pdf/beacon and trigger a blob download instead of chrome.requestPdf polling - Add dev proxy route and start:pdf script - Update tests and local dev documentation Co-authored-by: Cursor <cursoragent@cursor.com>
getHeaderDate() used local-time getters (getDate, getFullYear) and toLocaleString without timeZone, but labelled the output 'UTC'. On a server running outside UTC the header would show the wrong date/time. Switch to getUTCDate/getUTCFullYear and pass timeZone: 'UTC' to both toLocaleString calls. Export the function and add a test that runs under a non-UTC timezone to verify correctness. Co-authored-by: Cursor <cursoragent@cursor.com>
Concurrent printPdf calls could both observe browserInstance as null, launch separate Puppeteer browsers, and leak the earlier instance. Add a launchPromise guard so concurrent callers coalesce on the same puppeteer.launch() promise. Clear the promise in closeBrowser() too. Co-authored-by: Cursor <cursoragent@cursor.com>
The unconditional require.resolve('frontend-assets/package.json') at
module init crashed the PDF server if the optional dependency was
absent (e.g. GitHub auth failure during install).
Replace with an inlined SVG string, removing the fs/path/require.resolve
runtime dependency entirely.
Source: frontend-assets/src/partners-icons/lightwell-logomark.svg
Co-authored-by: Cursor <cursoragent@cursor.com>
Add a promise-based semaphore (MAX_CONCURRENT_RENDERS=3) that caps the number of simultaneous Puppeteer page renders. Excess requests queue until a slot is freed. This prevents unbounded Chromium page creation from exhausting pod memory when multiple users export PDFs at once. The limit controls concurrent render *processes*, not PDF pages -- each render produces a complete multi-page PDF in a single Chromium tab. Co-authored-by: Cursor <cursoragent@cursor.com>
Reject POST /pdf/beacon payloads with more than 5,000 vulnerabilities to prevent Chromium from running out of memory on oversized renders. Returns 400 with a descriptive error message. Co-authored-by: Cursor <cursoragent@cursor.com>
Add 30-second timeout to page.goto() and page.pdf() Puppeteer calls so a hung Chromium process throws instead of blocking forever. Wrap the Express handler in a 60-second overall timeout that returns 504 if the full pipeline exceeds the budget. Co-authored-by: Cursor <cursoragent@cursor.com>
Apply express-rate-limit (5 requests/minute per IP) to POST /pdf/beacon to prevent abuse. Chromium renders are expensive and even with the concurrency semaphore, unlimited queuing could exhaust memory. Co-authored-by: Cursor <cursoragent@cursor.com>
5,000 vulnerabilities with all fields is ~2-3 MB of JSON. The previous 50 MB limit was far larger than any realistic payload and invited memory abuse. 10 MB provides comfortable headroom. Co-authored-by: Cursor <cursoragent@cursor.com>
Export getBrowser from pdfRenderer so the health check can verify
Chromium is alive by opening and closing a throwaway page. Returns
200 {status:'ok'} or 503 {status:'unhealthy',error:...}.
This will serve as the readiness/liveness probe target in the
ClowdApp deployment.
Co-authored-by: Cursor <cursoragent@cursor.com>
Add prom-client with three metrics: - pdf_generation_duration_seconds: histogram of end-to-end time - pdf_generation_errors_total: counter by error type - pdf_active_renders: gauge tracking in-flight render slots Expose GET /metrics endpoint on the PDF server. Wire the duration timer and error counter into handleBeaconPdf, and the active renders gauge into the concurrency semaphore. Co-authored-by: Cursor <cursoragent@cursor.com>
Two-stage build on UBI9 Node.js 22: - Builder: yarn install with chrome-headless-shell download - Runtime: Node.js + Chrome system deps, runs tsx directly Uses the same entrypoint as 'yarn start:pdf' (no webpack build). Listens on port 8000 to match Clowder convention. Co-authored-by: Cursor <cursoragent@cursor.com>
Standalone ClowdApp deployment with: - 2Gi/4Gi memory (Chromium headless needs ~1-2Gi per render) - Readiness probe on GET /pdf/healthz (30s interval) - Liveness probe on GET /pdf/healthz (60s interval) - PDF_MAX_CONCURRENT env var for tuning concurrency - 1 replica (PDF exports are user-initiated and infrequent) Co-authored-by: Cursor <cursoragent@cursor.com>
Push and pull-request pipelines that build Dockerfile.pdf into a separate container image (content-sources-pdf-server). Uses the same docker-build-oci-ta pipeline as the frontend, with its own service account and output image path. PR images expire after 5 days. Co-authored-by: Cursor <cursoragent@cursor.com>
f6b18c8 to
5f1d1c4
Compare
| app.post('/pdf/beacon', pdfRateLimiter, (req, res) => { | ||
| const timer = setTimeout(() => { | ||
| if (!res.headersSent) { | ||
| res.status(504).json({ error: 'PDF generation timed out' }); | ||
| } | ||
| }, HANDLER_TIMEOUT_MS); | ||
|
|
||
| handleBeaconPdf(req, res).finally(() => clearTimeout(timer)); |
There was a problem hiding this comment.
The timeout path can take the process down.
Once the 60s timer fires and sends the 504, handleBeaconPdf keeps running. When it finishes it hits res.setHeader('Content-Type', ...) at line 78, which throws ERR_HTTP_HEADERS_SENT. That's caught by the handler's own catch, which then calls res.status(500).json(...) at line 85 — and that throws too. The second throw escapes handleBeaconPdf, and since the call here only has .finally() and no .catch(), it surfaces as an unhandled rejection. Node 22 exits the process on those by default, so a single slow render takes out the pod.
Two fixes needed:
- guard the success and error writes in
handleBeaconPdfwithif (res.headersSent) return; - add a
.catch()here (or make the wrapperasyncandawaitinside a try/catch) so nothing escapes
Separately: the 504 doesn't cancel the underlying render, so the Chromium work continues and still occupies a semaphore slot for a response nobody will read.
| const pdfRateLimiter = rateLimit({ | ||
| windowMs: 60_000, | ||
| limit: 5, | ||
| standardHeaders: 'draft-7', | ||
| legacyHeaders: false, | ||
| message: { error: 'Too many PDF requests. Please wait a minute before trying again.' }, | ||
| }); |
There was a problem hiding this comment.
In production this degrades into a single global bucket.
There's no app.set('trust proxy', ...), so express-rate-limit keys on the socket address. Behind the console gateway every request arrives with the gateway's IP, which means limit: 5 per minute applies to all users combined rather than per user — the first person to export three reports locks out everyone else for the rest of the minute.
Setting trust proxy blindly isn't the fix either, since X-Forwarded-For is then spoofable. Options: key the limiter on the identity header instead of the IP, or drop IP-based limiting here and rely on the render semaphore plus the handler timeout for back-pressure.
| app.get('/metrics', async (_req, res) => { | ||
| res.set('Content-Type', registry.contentType); | ||
| res.send(await registry.metrics()); | ||
| }); |
There was a problem hiding this comment.
/metrics is served unauthenticated on the same port as web: true, and collectDefaultMetrics publishes process-level detail (heap, file descriptors, versions, arguments).
Clowder convention is to expose metrics on the private metrics port (9000 by default) with a metrics block in the ClowdApp, so the endpoint is only reachable by the Prometheus scraper. deploy/pdf-server.yaml has no metrics section at all right now, so as written this is public surface and the scraper has nothing declared to find.
| data: BeaconPdfData; | ||
| }; | ||
|
|
||
| export async function handleBeaconPdf(req: express.Request, res: express.Response): Promise<void> { |
There was a problem hiding this comment.
There's no authentication or authorization anywhere on this endpoint.
The validation below checks shape (customerId present, matches [\w-]+, columns non-empty, vulnerability count under the cap) but never checks who is asking or whether they're entitled to that customerId. The request body is entirely client-supplied, so anything that can reach the service can drive arbitrary Chromium renders with arbitrary content and get a branded Lightwell PDF back for any customer ID it names.
The old crc-pdf-generator flow sat behind the platform gateway with the identity header available. Whatever replaces it needs the equivalent: read x-rh-identity, and check the caller's entitlement to the requested customer before rendering.
| // Concurrency semaphore: limits simultaneous Puppeteer renders to avoid OOM. | ||
| let activeRenders = 0; | ||
| let waitQueue: Array<() => void> = []; | ||
|
|
||
| function acquireSlot(): Promise<void> { | ||
| if (activeRenders < MAX_CONCURRENT_RENDERS) { | ||
| activeRenders++; | ||
| activeRendersGauge.inc(); | ||
| return Promise.resolve(); | ||
| } | ||
| return new Promise((resolve) => | ||
| waitQueue.push(() => { | ||
| activeRendersGauge.inc(); | ||
| resolve(); | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The semaphore accounting itself is correct (the slot hand-off in releaseSlot deliberately skips the decrement — took me a second read, a comment there would help). Two gaps around it though:
waitQueueis unbounded. With the rate limiter behaving as a single global bucket (see my note onpdfServer.ts:144), nothing caps how many requests pile up behind three slots.- There's no abort path. A request that waits past
HANDLER_TIMEOUT_MSgets its 504, but its queued callback still fires later and burns a full render slot producing a PDF that's already been abandoned. Under sustained load the service spends its capacity on dead work and never catches up.
Suggest capping the queue (reject with 503 past a threshold, which is honest back-pressure) and passing an AbortSignal — or at minimum a cancelled flag checked in the acquire callback — so timed-out requests release rather than consume their slot.
| livenessProbe: | ||
| httpGet: | ||
| path: /pdf/healthz | ||
| port: 8000 | ||
| scheme: HTTP | ||
| initialDelaySeconds: 15 | ||
| periodSeconds: 60 |
There was a problem hiding this comment.
Using the same browser-launching endpoint for liveness is risky. handleHealthz does getBrowser() + newPage(), and that newPage() isn't bounded by the render semaphore, so it competes with in-flight renders.
Under load the probe gets slow, liveness fails, and the kubelet restarts the pod while it's rendering — killing real work at exactly the moment the service is busiest, and potentially looping.
Usual split: liveness stays cheap (a static 200 proving the event loop is alive), readiness does the browser check so a wedged pod is pulled from the service without being killed.
| "prettier": "^3.9.6", | ||
| "pretty-format": "^30.5.1", | ||
| "prop-types": "15.8.1", | ||
| "puppeteer": "^25.9.0", |
There was a problem hiding this comment.
Dependency placement is inconsistent now that this is a deployed service rather than a dev-only script. puppeteer, express, express-rate-limit and tsx are all in devDependencies and all required at runtime by the container, while prom-client — used by exactly the same code — sits in dependencies.
It happens to work because the Dockerfile installs everything, but it blocks a --production install and makes the real runtime dependency set impossible to read. Either move all five into dependencies, or split the PDF server into its own workspace/package with its own manifest (which would also make the npm-package extraction you sketched in the PR comments a much smaller step later).
| "optionalDependencies": { | ||
| "frontend-assets": "github:RedHatInsights/frontend-assets" |
There was a problem hiding this comment.
Is this still needed? It was added for the require.resolve('frontend-assets/package.json') lookup in lightwellLogomark.ts, which is now an inlined SVG string. If nothing else in the tree imports frontend-assets, dropping this also removes a GitHub-sourced dependency from the install path (and from the container build, which currently resolves it).
| itemCount: count, | ||
| }) as unknown as PDFRequestPayload, | ||
| }); | ||
| const data = await fetchAllFilteredVulnerabilities(customerId, filters); |
There was a problem hiding this comment.
Following up on my earlier comment about the dataset limit: MAX_VULNERABILITIES = 5000 on the server covers the server side, thanks. The client side is still unguarded though.
This call pages through the entire result set at 200 per request before anything is validated, so a customer with 8000 findings sits through 40 sequential API calls — potentially a minute or more of spinner — and then gets a 400 saying there were too many. The count is already available in meta.count from the first page, so the check can happen after one request instead of forty.
Still outstanding from that thread: is 5000 documented anywhere users will see it, and has one representative large report actually been rendered end to end? A 5000-row single-tab Chromium render is the case the timeouts and memory limits hinge on, and right now nothing in the PR shows what it costs.
| const response = await axios.post( | ||
| '/pdf/beacon', | ||
| { customerId, visibleColumns, data }, | ||
| { responseType: 'blob' }, | ||
| ); |
There was a problem hiding this comment.
This relative URL only resolves in local dev. fec.config.js proxies /pdf/ only when PDF_SERVER_PORT is set, and in stage/prod there's nothing routing /pdf/beacon to the new ClowdApp — deploy/frontend.yaml adds no /pdf/ path and no apiPath, and the path isn't under /api/ so the gateway won't pick it up by convention either.
Deployed, this will 404. Needs the gateway/Frontend CRD route wiring (and the corresponding app-interface saas entry for the ClowdApp) before merge, and the URL here changed to whatever prefix that lands on.
2 participants
What
Replace the external
crc-pdf-generatormicroservice with a lightweight in-app Express + Puppeteer server for Beacon PDF exports. This cuts export time from ~20 s to under 3 s by eliminating the Scalprum module-federation round-trip, S3 storage, and Kafka status polling.Why
The current flow goes through
crc-pdf-generator, which:./BeaconPdfEntrymodule in a new Puppeteer pageThis adds ~20 s of latency per export and requires four separate processes for local development. The new approach SSR-renders the existing
BeaconPdfTemplatedirectly in Node, prints it with Puppeteer, and streams the PDF back — all in a singlePOST /pdf/beaconcall.How
New
src/pdf/module:pdfServer.ts— Express server with a singlePOST /pdf/beaconendpoint. Validates the request, fetches all pages of vulnerability data (or uses built-in mock data), then calls the renderer.pdfRenderer.tsx— SSR pipeline: rendersBeaconPdfTemplateto HTML viarenderToStaticMarkup, prints to PDF via a singleton Puppeteer browser instance.pdfHeader.tsx— Puppeteer header/footer templates with Lightwell branding and page numbering (using Puppeteer's built-inpageNumberclass).register-css-stub.ts— Node require hook that stubs CSS/SCSS imports so PatternFly components can be SSR'd.UI changes:
ExportMenu.tsx— PDF export now posts to/pdf/beaconand triggers a blob download instead of callingchrome.requestPdf.itemCountprop (no longer needed for pre-splitting into paginated tasks).Removed legacy code:
BeaconPdfEntry.tsx— module federation entry point forcrc-pdf-generator.beaconPdf.ts— removedfetchData(),buildBeaconPdfPayload(), and associated constants/types that only served the old flow.fec.config.js— removed./BeaconPdfEntryexpose andPDF_GENERATOR_PORTproxy route.Dev setup:
yarn start:pdfruns the PDF server viatsx(zero-config TypeScript runner).fec.config.jsproxies/pdf/to the PDF server whenPDF_SERVER_PORTis set.Dependencies added
expresspuppeteertsxstart:pdf@types/expressDependencies removed
pdf-libts-nodetsxtsconfig-pathstsxresolvesbaseUrlnatively)Testing
pdfServer.spec.ts— handler unit tests (success, 400, 500 paths)pdfRenderer.spec.tsx— SSR HTML output assertionsbeaconPdf.test.ts— retained tests forformatBeaconPdfGeneratedAtandshouldUseLandscapePdfExportMenu.test.tsx— updated to test the newaxios.post+ blob download flowHere's the section to append to the PR description:
How to test this PR locally
Manual test with mock data (no backend needed):
Start the PDF server with mock data:
If Puppeteer can't find Chrome, point it at your system install:
CHROME_PATH=$(which google-chrome-stable) USE_MOCK=true yarn start:pdfIn a second terminal, start the frontend dev proxy:
Open https://stage.foo.redhat.com:1337/lightwell/beacon, select customer
CID-01orCID-214, and click Export → Export as PDF.Verify in the downloaded PDF:
Manual test against stage (real data):
Start the PDF server (routes through the dev proxy for auth):
In a second terminal:
Open https://stage.foo.redhat.com:1337/lightwell/beacon, select a customer, and export as PDF.