Skip to content

Commit cbeb95c

Browse files
waleedlatif1claude
andcommitted
test(pptx): cover embedded PDF worker source directly
Restore the resolve-once memoization for the pdfjs asset URLs so the failure path is not retried per embedded PDF, matching the documented intent of the cache. Execute WORKER_SRC in-process against a stand-in pdfjs so the fix itself is covered rather than only the message it posts. The worker template was previously an untested string, which is how the falsy workerSrc assignment shipped green. Asserts that the worker module is imported before the library and that GlobalWorkerOptions is never written to. Also fixes the canvas-guard test, which passed because window was undefined rather than because the guard fired, and adds the missing Worker guard case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GTTSjsX7CBMfXrdmq4PL85
1 parent 628da73 commit cbeb95c

2 files changed

Lines changed: 210 additions & 17 deletions

File tree

apps/sim/lib/pptx-renderer/utils/pdf-renderer.test.ts

Lines changed: 190 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,24 @@
11
/**
22
* @vitest-environment node
33
*/
4-
import { afterEach, describe, expect, it, vi } from 'vitest'
5-
import { renderPdfToImage } from '@/lib/pptx-renderer/utils/pdf-renderer'
4+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
5+
import { renderPdfToImage, WORKER_SRC } from '@/lib/pptx-renderer/utils/pdf-renderer'
6+
7+
const PAGE_URL = 'https://example.com/preview'
8+
const LIBRARY_URL = 'https://example.com/_next/static/media/pdf.min.mjs'
9+
const WORKER_URL = 'https://example.com/_next/static/media/pdf.worker.min.mjs'
10+
11+
/**
12+
* Reproduce the root-relative asset strings a production bundler emits for the
13+
* two pdfjs assets, so the absolute-URL resolution is exercised rather than
14+
* assumed. Only `toString()` is stubbed; the module resolves the final value
15+
* through `href`.
16+
*/
17+
function stubBundlerAssetUrls() {
18+
vi.spyOn(URL.prototype, 'toString').mockImplementation(function (this: URL) {
19+
return `/_next/static/media/${this.pathname.split('/').pop()}`
20+
})
21+
}
622

723
afterEach(() => {
824
vi.unstubAllGlobals()
@@ -11,20 +27,29 @@ afterEach(() => {
1127
})
1228

1329
describe('renderPdfToImage', () => {
30+
beforeEach(() => {
31+
vi.stubGlobal('window', { location: { href: PAGE_URL } })
32+
stubBundlerAssetUrls()
33+
})
34+
1435
it('skips rendering when isolated canvas rendering is unavailable', async () => {
36+
vi.stubGlobal('Worker', class {})
1537
vi.stubGlobal('OffscreenCanvas', undefined)
38+
1639
expect(await renderPdfToImage(new Uint8Array([1]), 10, 10)).toBeNull()
1740
})
1841

19-
it('supplies matching library and worker assets and preserves result/error behavior', async () => {
42+
it('skips rendering when workers are unavailable', async () => {
43+
vi.stubGlobal('OffscreenCanvas', class {})
44+
vi.stubGlobal('Worker', undefined)
45+
46+
expect(await renderPdfToImage(new Uint8Array([1]), 10, 10)).toBeNull()
47+
})
48+
49+
it('supplies absolute library and worker assets and preserves result/error behavior', async () => {
2050
vi.useFakeTimers()
2151
const postMessage = vi.fn()
2252
const worker = { postMessage, onmessage: null as ((event: MessageEvent) => void) | null }
23-
vi.stubGlobal('window', { location: { href: 'https://example.com/preview' } })
24-
/** Model the root-relative asset strings returned by the production bundler. */
25-
vi.spyOn(URL.prototype, 'toString').mockImplementation(function () {
26-
return `/_next/static/media/${this.pathname.split('/').pop()}`
27-
})
2853
vi.stubGlobal('OffscreenCanvas', class {})
2954
vi.stubGlobal(
3055
'Worker',
@@ -40,8 +65,8 @@ describe('renderPdfToImage', () => {
4065
expect(message).toMatchObject({
4166
width: 20,
4267
height: 10,
43-
pdfjsUrl: 'https://example.com/_next/static/media/pdf.min.mjs',
44-
pdfjsWorkerUrl: 'https://example.com/_next/static/media/pdf.worker.min.mjs',
68+
pdfjsUrl: LIBRARY_URL,
69+
pdfjsWorkerUrl: WORKER_URL,
4570
})
4671
expect(message.pdfData).toEqual(input)
4772
expect(message.pdfData).not.toBe(input)
@@ -56,3 +81,158 @@ describe('renderPdfToImage', () => {
5681
expect(await failed).toBeNull()
5782
})
5883
})
84+
85+
interface WorkerScope {
86+
onmessage: ((event: { data: Record<string, unknown> }) => Promise<void>) | null
87+
postMessage: (message: Record<string, unknown>) => void
88+
}
89+
90+
interface WorkerHarness {
91+
imported: string[]
92+
posted: Array<Record<string, unknown>>
93+
globalWorkerOptions: Record<string, unknown>
94+
destroy: ReturnType<typeof vi.fn>
95+
send: (data: Record<string, unknown>) => Promise<void>
96+
}
97+
98+
/**
99+
* Execute {@link WORKER_SRC} in-process against a stand-in pdfjs.
100+
*
101+
* Node refuses dynamic `import()` inside `new Function` ("A dynamic import
102+
* callback was not specified") and the `node:vm` hook needs
103+
* `--experimental-vm-modules`, so the two import sites are redirected to an
104+
* injected loader. The substitution count is asserted, so a source change that
105+
* drops either import fails here rather than silently testing nothing.
106+
*/
107+
function runWorkerSource(
108+
overrides: { pages?: number; getDocument?: () => { promise: Promise<unknown> } } = {}
109+
): WorkerHarness {
110+
const imported: string[] = []
111+
const posted: Array<Record<string, unknown>> = []
112+
const globalWorkerOptions: Record<string, unknown> = {}
113+
const destroy = vi.fn()
114+
115+
const page = {
116+
getViewport: ({ scale }: { scale: number }) => ({ width: 100 * scale, height: 50 * scale }),
117+
render: () => ({ promise: Promise.resolve() }),
118+
}
119+
const doc = { numPages: overrides.pages ?? 1, getPage: async () => page, destroy }
120+
const library = {
121+
GlobalWorkerOptions: globalWorkerOptions,
122+
getDocument: overrides.getDocument ?? (() => ({ promise: Promise.resolve(doc) })),
123+
}
124+
125+
const load = async (url: string) => {
126+
imported.push(url)
127+
return url.includes('pdf.worker') ? {} : library
128+
}
129+
130+
class FakeOffscreenCanvas {
131+
constructor(
132+
public width: number,
133+
public height: number
134+
) {}
135+
getContext() {
136+
return {}
137+
}
138+
async convertToBlob() {
139+
return new Blob(['png'])
140+
}
141+
}
142+
143+
expect(WORKER_SRC.split('await import(').length - 1).toBe(2)
144+
const source = WORKER_SRC.replaceAll('await import(', 'await __load(')
145+
146+
const scope: WorkerScope = {
147+
onmessage: null,
148+
postMessage: (message) => posted.push(message),
149+
}
150+
new Function('self', '__load', 'OffscreenCanvas', source)(scope, load, FakeOffscreenCanvas)
151+
152+
return {
153+
imported,
154+
posted,
155+
globalWorkerOptions,
156+
destroy,
157+
send: async (data) => {
158+
await scope.onmessage?.({ data })
159+
},
160+
}
161+
}
162+
163+
describe('WORKER_SRC', () => {
164+
it('imports the worker module before the library and renders a blob', async () => {
165+
const harness = runWorkerSource()
166+
167+
await harness.send({
168+
id: 7,
169+
pdfData: new Uint8Array([1]),
170+
width: 20,
171+
height: 10,
172+
pdfjsUrl: LIBRARY_URL,
173+
pdfjsWorkerUrl: WORKER_URL,
174+
})
175+
176+
expect(harness.imported).toEqual([WORKER_URL, LIBRARY_URL])
177+
expect(harness.posted).toHaveLength(1)
178+
expect(harness.posted[0].id).toBe(7)
179+
expect(harness.posted[0].blob).toBeInstanceOf(Blob)
180+
expect(harness.destroy).toHaveBeenCalledOnce()
181+
})
182+
183+
/**
184+
* The original defect: pdfjs reads `workerSrc` through a getter that throws
185+
* when falsy, outside its own try/catch, so assigning it here broke every
186+
* render. The worker must leave pdfjs configuration untouched.
187+
*/
188+
it('never writes to GlobalWorkerOptions', async () => {
189+
const harness = runWorkerSource()
190+
191+
await harness.send({
192+
id: 1,
193+
pdfData: new Uint8Array([1]),
194+
width: 20,
195+
height: 10,
196+
pdfjsUrl: LIBRARY_URL,
197+
pdfjsWorkerUrl: WORKER_URL,
198+
})
199+
200+
expect(harness.globalWorkerOptions).toEqual({})
201+
expect('workerSrc' in harness.globalWorkerOptions).toBe(false)
202+
})
203+
204+
it('reports an error instead of a blob when the document has no pages', async () => {
205+
const harness = runWorkerSource({ pages: 0 })
206+
207+
await harness.send({
208+
id: 2,
209+
pdfData: new Uint8Array([1]),
210+
width: 20,
211+
height: 10,
212+
pdfjsUrl: LIBRARY_URL,
213+
pdfjsWorkerUrl: WORKER_URL,
214+
})
215+
216+
expect(harness.posted).toEqual([{ id: 2, error: 'no pages' }])
217+
expect(harness.destroy).toHaveBeenCalledOnce()
218+
})
219+
220+
it('reports an error when pdfjs rejects', async () => {
221+
const harness = runWorkerSource({
222+
getDocument: () => ({ promise: Promise.reject(new Error('Invalid PDF')) }),
223+
})
224+
225+
await harness.send({
226+
id: 3,
227+
pdfData: new Uint8Array([1]),
228+
width: 20,
229+
height: 10,
230+
pdfjsUrl: LIBRARY_URL,
231+
pdfjsWorkerUrl: WORKER_URL,
232+
})
233+
234+
expect(harness.posted).toHaveLength(1)
235+
expect(harness.posted[0].id).toBe(3)
236+
expect(harness.posted[0].error).toContain('Invalid PDF')
237+
})
238+
})

apps/sim/lib/pptx-renderer/utils/pdf-renderer.ts

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,19 @@
1515
* fallback, no global state pollution.
1616
*/
1717

18-
// Resolved pdfjs URL — computed once from main thread's module resolution
18+
// Resolved pdfjs URLs — computed once from main thread's module resolution
1919

20-
let _pdfjsUrls: { library: string; worker: string } | null = null
20+
interface PdfjsUrls {
21+
library: string
22+
worker: string
23+
}
24+
25+
let _pdfjsUrls: PdfjsUrls | null = null
26+
let _pdfjsUrlsResolved = false
2127

22-
function getPdfjsUrls(): { library: string; worker: string } | null {
23-
if (_pdfjsUrls !== null) return _pdfjsUrls
28+
function getPdfjsUrls(): PdfjsUrls | null {
29+
if (_pdfjsUrlsResolved) return _pdfjsUrls
30+
_pdfjsUrlsResolved = true
2431
try {
2532
const library = new URL('pdfjs-dist/build/pdf.min.mjs', import.meta.url).toString()
2633
const worker = new URL('pdfjs-dist/build/pdf.worker.min.mjs', import.meta.url).toString()
@@ -30,7 +37,7 @@ function getPdfjsUrls(): { library: string; worker: string } | null {
3037
worker: new URL(worker, window.location.href).href,
3138
}
3239
} catch {
33-
return null
40+
_pdfjsUrls = null
3441
}
3542
return _pdfjsUrls
3643
}
@@ -47,8 +54,14 @@ function getPdfjsUrls(): { library: string; worker: string } | null {
4754
* Loading the matching worker module installs its WorkerMessageHandler in
4855
* this isolated global scope. PDF.js then uses its in-context worker fallback
4956
* without creating another worker or changing the host app's configuration.
57+
*
58+
* Never assign `GlobalWorkerOptions.workerSrc` here: pdfjs reads it through a
59+
* getter that throws when falsy, and the read happens outside its own
60+
* try/catch, so a falsy assignment makes every `getDocument` call fail.
61+
*
62+
* @internal Exported so tests can execute this source directly.
5063
*/
51-
const WORKER_SRC = /* js */ `
64+
export const WORKER_SRC = /* js */ `
5265
let pdfjsLib = null;
5366
5467
self.onmessage = async (e) => {
@@ -135,7 +148,7 @@ function renderInWorker(
135148
pdfData: Uint8Array,
136149
width: number,
137150
height: number,
138-
pdfjsUrls: { library: string; worker: string }
151+
pdfjsUrls: PdfjsUrls
139152
): Promise<Blob | null> {
140153
return new Promise((resolve) => {
141154
const worker = getWorker()

0 commit comments

Comments
 (0)