Conversation
JonasKenke
requested review from
asvishnyakov,
hayescode and
sandangel
as code owners
dosubot
bot
added
size:L
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cypress/e2e/pdf_viewer/spec.cy.ts">
<violation number="1" location="cypress/e2e/pdf_viewer/spec.cy.ts:6">
P2: Test uses non-existent Cypress event `window:console`, so console error collection never executes and the no-console-error assertion is effectively meaningless.</violation>
<violation number="2" location="cypress/e2e/pdf_viewer/spec.cy.ts:37">
P2: The MIME/CDN error test is effectively a no-op because it clears and asserts `consoleErrors` immediately without waiting for PDF load-time console output.</violation>
<violation number="3" location="cypress/e2e/pdf_viewer/spec.cy.ts:53">
P2: Test title claims worker-origin validation, but assertions only verify PDF rendering and cannot detect CDN worker usage.</violation>
</file>
<file name="frontend/src/components/chat/MessagesContainer/index.tsx">
<violation number="1" location="frontend/src/components/chat/MessagesContainer/index.tsx:97">
P2: Auto-open effect updates `sideViewState` but never clears it, allowing stale/reopened side panel state across `elements` updates.</violation>
</file>
<file name="frontend/src/components/Elements/PDF.tsx">
<violation number="1" location="frontend/src/components/Elements/PDF.tsx:46">
P2: `startPage` is not clamped to a minimum of 1, so invalid negative page numbers can be sent to `<Page pageNumber>`.</violation>
<violation number="2" location="frontend/src/components/Elements/PDF.tsx:67">
P2: `window.open` is used with `_blank` without `noopener`, enabling opener access for opened URLs and creating reverse-tabnabbing risk.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
This PR replaces the existing iframe-based PDF rendering with a custom React PDF viewer (react-pdf/pdfjs) and wires it into inline and side-panel element rendering, with supporting backend MIME-type handling and new Cypress coverage.
Changes:
- Introduces a new
PDFElement/PDFViewerReact component for inline previews, modal fullscreen viewing, and side/page display. - Updates chat + side-view behavior to better accommodate PDF rendering (auto-open side panel on side elements; scrolling/layout tweaks).
- Adds an e2e test app/spec (plus a dummy PDF) and updates frontend dependencies/test setup for pdfjs compatibility.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/tests/setup-tests.ts | Adds a JSDOM DOMMatrix polyfill required by pdfjs-dist during test imports. |
| frontend/src/components/chat/MessagesContainer/index.tsx | Adds logic to auto-open side panel when display='side' elements are present. |
| frontend/src/components/chat/Messages/Message/Content/InlinedElements/InlinedPDFList.tsx | Adjusts inline PDF wrapper layout for the new component. |
| frontend/src/components/Elements/PDF.tsx | Replaces iframe rendering with a new react-pdf-based viewer (toolbar, zoom/paging, modal). |
| frontend/src/components/ElementSideView.tsx | Updates overflow behavior and default sizing to better support the new viewer. |
| frontend/package.json | Adds react-pdf dependency. |
| frontend/pnpm-lock.yaml | Locks react-pdf / pdfjs-dist and transitive deps. |
| cypress/e2e/pdf_viewer/spec.cy.ts | Adds Cypress spec validating inline preview, modal open/close, and side rendering. |
| cypress/e2e/pdf_viewer/main.py | Adds a Chainlit app fixture that emits inline + side PDF elements. |
| cypress/e2e/pdf_viewer/dummy.pdf | Adds a dummy PDF used by the Cypress app fixture. |
| backend/chainlit/server.py | Ensures .mjs files are served with application/javascript (PDF worker). |
| .npmrc | Adjusts hoisting to ensure pdfjs-dist is available in the workspace layout. |
Files not reviewed (1)
- frontend/pnpm-lock.yaml: Language not supported
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cypress/e2e/pdf_viewer/spec.cy.ts">
<violation number="1" location="cypress/e2e/pdf_viewer/spec.cy.ts:37">
P2: The MIME/CDN regression test is vacuous and no longer verifies MIME/CDN error conditions; it can pass with zero validated PDF resources.</violation>
</file>
<file name="frontend/src/components/chat/MessagesContainer/index.tsx">
<violation number="1" location="frontend/src/components/chat/MessagesContainer/index.tsx:109">
P2: `sideView` can become stale because it is only updated when a new side-element ID appears, not when existing side elements change.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Thanks for putting this together — I can see the effort that went into improving the PDF experience here. I do have a few questions to better understand the trade-offs. What are the main limitations of the native browser PDF viewer that this change is trying to address? In my experience, Chrome’s built-in PDF viewer is already quite good, and Firefox’s native experience is also solid. I dont know about other browsers though. Could you clarify which concrete user-facing features this PR adds over the current iframe/native approach? That would help us better understand the value of introducing and maintaining a custom viewer. One thing I liked about the current iframe approach is that it could point to PDFs behind authenticated URLs (Sharepoint for example would request authentication or authorization). Am I right in thinking that this new approach would make that harder or not support it ? |
|
Hey, For the authenticated urls I'm not sure but will check and I'm open for improvements |
Summary
This change replaces the previous iframe-based browser PDF viewer with a new, enhanced PDF-viewer component and adds the associated support in both frontend and e2e test suites.
Validation
All tests passed ✔ All specs passed! 06:27
What changed
Old:
New:
Summary by cubic
Replaced the iframe PDF viewer with a React-based viewer that supports inline previews, a fullscreen modal, and a side-panel that auto-opens only for new or changed elements. Ships a local
pdfjs-distworker (no CDN) with e2e coverage.New Features
react-pdfand a toolbar for zoom, pagination, download, and print.pdfjs-distworker is bundled and hoisted via.npmrc(no external CDN).Bug Fixes
.mjsas JavaScript to prevent MIME errors.DOMMatrixforpdfjsin JSDOM; e2e asserts the local worker loads with zero CDN requests.Written for commit cfd840f. Summary will update on new commits.