fix(chainlit): render source PDF previews in the chat side panel - #606
Conversation
Clicking a source in the Chainlit chat showed "Failed to load PDF file".
Two independent bugs were responsible:
1. The pdf.js worker could not load. Chainlit is submounted at /chainlit,
but react-pdf computes its worker URL at runtime as the origin-root
/assets/pdf.worker*.mjs (missing the /chainlit prefix). That path was
not auth-bypassed and nothing served it, so it returned a 403 JSON body
and the browser blocked the module worker on a forbidden MIME type -
pdf.js never started and the PDF was never fetched.
2. /static/{chunk_id} served the file as Content-Disposition: attachment
(FileResponse defaults to attachment when filename is set), so the
browser downloaded it instead of rendering it inline.
Fixes:
- main.py: also serve Chainlit's frontend asset dir at /assets so the
worker resolves with a JavaScript MIME type.
- auth.py: bypass /assets/* (public frontend bundles, the same files
already served and bypassed under /chainlit/assets).
- download.py: serve /static inline for safe previewable media types
(PDF/image/audio/video); HTML/SVG stay attachment to avoid a
same-origin stored-XSS vector.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR mounts Chainlit root assets at ChangesChainlit asset serving and download disposition
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f46bee2e0b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@coderabbits revuew this PR. |
|
@codex review |
The source-PDF-preview fix mounted Chainlit's root-absolute /assets worker only on the API app. In Ray Serve mode the browser-facing UI is the standalone chainlit_api process on its own port, which lacked that mount — so GET /assets/pdf.worker*.mjs 404'd there and previews stayed broken. Extract the mount into a shared mount_chainlit_root_assets() helper and call it from both api.main and chainlit_api, mirroring how chainlit_api already replicates the /static download route for the same separate-origin reason.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openrag/api/chainlit_assets.py (1)
27-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging when the assets directory is missing.
The no-op path is silent — if Chainlit's frontend build is missing/stale in a deployment,
/assetsrequests will silently 404 with no diagnostic trail, which is exactly the class of hard-to-debug issue this PR is fixing. A one-line warning would help future troubleshooting.Note: there's a documented conflict on the logger import path — the coding guidelines say
from utils.logger import get_logger, but a repo learning states there is noutils.loggermodule and the canonical import isfrom core.utils.logging import get_logger. Please confirm which is currently correct before adding the log call.💡 Proposed logging addition (pending import path confirmation)
def mount_chainlit_root_assets(app: FastAPI) -> None: """Mount Chainlit's ``frontend/dist/assets`` at ``/assets`` (no-op if absent).""" import chainlit from starlette.staticfiles import StaticFiles assets_dir = Path(chainlit.__file__).parent / "frontend" / "dist" / "assets" if assets_dir.is_dir(): app.mount("/assets", StaticFiles(directory=str(assets_dir)), name="chainlit_root_assets") + else: + logger.warning("Chainlit frontend assets dir not found at {}; /assets mount skipped", assets_dir)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/api/chainlit_assets.py` around lines 27 - 34, The silent no-op in mount_chainlit_root_assets should log a warning when the Chainlit assets directory is missing so stale/missing frontend builds are diagnosable. Update the mount_chainlit_root_assets function to add an explicit warning in the assets_dir.is_dir() false path, using the repo-correct logger import before emitting the message. Keep the existing mount behavior unchanged when the directory exists, and make sure the warning clearly names the missing assets directory.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openrag/api/chainlit_assets.py`:
- Around line 27-34: The silent no-op in mount_chainlit_root_assets should log a
warning when the Chainlit assets directory is missing so stale/missing frontend
builds are diagnosable. Update the mount_chainlit_root_assets function to add an
explicit warning in the assets_dir.is_dir() false path, using the repo-correct
logger import before emitting the message. Keep the existing mount behavior
unchanged when the directory exists, and make sure the warning clearly names the
missing assets directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2115f93f-f3b8-47af-9bba-ec8e09e752b9
📒 Files selected for processing (3)
openrag/api/chainlit_assets.pyopenrag/api/main.pyopenrag/chainlit_api.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openrag/api/main.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f46bee2e0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The standalone Chainlit asset mount accessed chainlit.__file__ directly, which raised AttributeError under the unit tests that stub chainlit as a bare ModuleType (no __file__). Guard with getattr and no-op when absent — a namespace package or test double simply has no bundled assets to serve. Fixes the tests (3.12) job on PR #606.
hedhoud
left a comment
hedhoud
left a comment
There was a problem hiding this comment.
I left one small hardening comment on the inline preview response.
Under Ray Serve the standalone chainlit_api app serves /static source-file downloads on its own origin, but it only registered AuthMiddleware. AuthMiddleware authenticates /static via the ?token= query param, reading it from request.state.original_token — which only RequestIdMiddleware populates. Without it, every source-file fetch saw token=None and returned 403 "Missing token", breaking source previews (PDF/image/audio/video) whenever ray.serve is enabled. Register RequestIdMiddleware outside AuthMiddleware, mirroring api.main, so original_token is set before auth reads it.
The /static source-download route serves some files inline with a Content-Type inferred from the filename. Add a nosniff header so the browser can't MIME-sniff a mislabeled file into an executable type (HTML/JS) in the app origin — defense-in-depth alongside the existing attachment fallback for HTML/SVG. Addresses @hedhoud's review on PR #606.
hedhoud
left a comment
hedhoud
left a comment
There was a problem hiding this comment.
Tested the preview path locally against the latest head. The Chainlit worker is now served from /assets with a JavaScript content type, PDF sources render inline, and HTML/SVG stay as attachments with nosniff set. CI is green as well. Looks good to merge from my side.
2 participants
Summary
Source PDF previews in the Chainlit chat side panel were broken — clicking a source rendered "Failed to load PDF file" even though the document was indexed and retrievable. This turned out to be two independent bugs; fixing only one is not enough.
/assets/…which isn't auth-bypassed →403 application/json→ the browser blocks the module worker on a forbidden MIME type/static/{chunk_id}served the file asContent-Disposition: attachment(a download), notinlineThe browser console made the primary cause explicit:
Root cause
Chainlit is sub-mounted at
/chainlit, and its HTML asset references are rewritten to/chainlit/assets/*. But react-pdf computes its worker URL at runtime in JS as the origin-root/assets/pdf.worker*.mjs— the HTML root-path rewrite never sees it, so it misses the/chainlitprefix and lands on a path that isn't auth-bypassed.Before — the worker request is rejected
sequenceDiagram participant B as Browser<br/>(Chainlit UI @ /chainlit) participant A as OpenRag API Note over B: react-pdf boots its pdf.js worker B->>A: GET /assets/pdf.worker.min-*.mjs Note over A: /assets is NOT under the<br/>/chainlit auth bypass,<br/>and nothing serves it A-->>B: 403 {"detail":"Missing token"}<br/>Content-Type: application/json Note over B: module worker blocked —<br/>forbidden MIME "application/json" Note over B: ❌ "Failed to load PDF file"<br/>(the PDF /static request never happens)After — the worker (and the PDF) load
sequenceDiagram participant B as Browser<br/>(Chainlit UI @ /chainlit) participant A as OpenRag API B->>A: GET /assets/pdf.worker.min-*.mjs Note over A: /assets/* bypassed in auth +<br/>StaticFiles mount serves the file A-->>B: 200 Content-Type: application/javascript Note over B: pdf.js worker starts ✅ B->>A: GET /static/{chunk_id}?token=… (Range) Note over A: FileResponse, Content-Disposition: inline A-->>B: 206 Partial Content · application/pdf Note over B: ✅ PDF renders in the side panelChanges
openrag/api/main.py— serve Chainlit's frontend asset dir at the origin-root/assetstoo (in addition to/chainlit/assets), so the runtime-computed worker URL resolves with a JavaScript MIME type.openrag/api/middleware/auth.py— bypass/assets/*inAuthMiddleware. These are public frontend bundles — the same files already served (and already bypassed) under/chainlit/assets— so the module worker getsapplication/javascriptinstead of a403 application/json.openrag/api/routers/user/download.py— serve/static/{chunk_id}inline for safe previewable media types (PDF / image / audio / video) so the viewers render in place.FileResponsedefaults toattachmentwhenfilenameis set. HTML / SVG deliberately stayattachmentso a crafted indexed file can't turn this same-origin route into a stored-XSS vector.Verification
Fresh request (no browser cache), against a running stack:
GET /assets/pdf.worker.min-*.mjs403 application/json(worker blocked)200 application/javascriptGET /static/{chunk_id}?token=…200 … attachment(download)inline · application/pdf(renders); range fetches return206/assets/../…)403/404)Confirmed end-to-end in a private/incognito tab (no cache): the source PDF now renders in the Chainlit side panel.
ruff checkclean on all three files.pytest tests/unit/api/routers/user/test_download.py— passing.Summary by CodeRabbit
New Features
/assetspath (including the pdf.js worker).Bug Fixes
/assets/.Security / Improvements
nosniff.