Repository navigation
feat(uploads): let apps serve chosen active content types inline - #2975
Conversation
✅ Deploy Preview for cedarjs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This adds an opt-in setting for serving chosen file types inline. The default behavior is unchanged, and no merge-blocking risk was identified. Pre-merge checks |
|
|
| test('serves a listed active type inline when inline was signed', async () => { | ||
| const res = await servePdf(inlineApp, 'inline') | ||
|
|
||
| expect(res.statusCode).toBe(200) | ||
| expect(res.headers['content-disposition']).toMatch(/^inline;/) |
There was a problem hiding this comment.
MIME parameters lack test coverage
The new tests cover mixed case in inlineTypes, but neither the option nor the stored MIME type includes parameters. Add cases with parameters on each side, plus a mixed-case stored type. Otherwise, breaking the promised parameter-insensitive matching could pass these tests and make PDF previews download instead.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Not adding these. A stored type can't carry parameters: storeFile rejects a MIME type like application/pdf; version=1.7 as invalid, so a stored type with parameters can't reach the serve route. Case-insensitive matching is already covered through inlineTypes, and both sides go through the same normalize helper, so a stored-side case test would exercise the same code path again.
There was a problem hiding this comment.
You're right. storeFile rejects MIME types containing parameters, so a parameterized stored MIME value isn't reachable through that path. Since normalizeMimeType is shared by both inlineTypes and the serve-side value, the existing mixed-case inlineTypes test already covers the case-insensitive normalization behavior. I'll withdraw the request for those additional tests.
|
View your CI Pipeline Execution ↗ for commit e080b82
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
Fixes #2970
The uploads serve route sends active content (HTML, SVG, XML, JavaScript and PDF) as
Content-Disposition: attachment, even when the signed URL asks forinline. That's a safe default, but it means an app can't preview stored PDFs in the browser: an<iframe src={signedUrl}>opens a download dialog and shows a blank frame.cedarUploadsPluginnow takes aninlineTypesoption that lists active-content MIME types the serve route may send inline when the signed URL asks forinline:inlineTypes, every active type is still served as an attachment.inline. Signed URLs still default toattachment.The issue also suggests serving inline active content with
Content-Security-Policy: sandbox. This PR doesn't include that, because Chrome refuses to render PDFs in sandboxed documents, which would defeat the point for PDFs.The uploads docs describe the option under "Reading files back" and list it with the other plugin options.