Improve rendering of errors from pdf.js (#5448)

GitOrigin-RevId: 69836ba213b124e2442b2d0083531dd95be2bf4e

services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.js

@@ -1,7 +1,6 @@
 import PropTypes from 'prop-types'
 import { memo, useCallback, useEffect, useState } from 'react'
 import { debounce } from 'lodash'
-import { Alert } from 'react-bootstrap'
 import PdfViewerControls from './pdf-viewer-controls'
 import { useProjectContext } from '../../../shared/context/project-context'
 import usePersistedState from '../../../shared/hooks/use-persisted-state'
@@ -10,10 +9,13 @@ import { buildHighlightElement } from '../util/highlights'
 import PDFJSWrapper from '../util/pdf-js-wrapper'
 import withErrorBoundary from '../../../infrastructure/error-boundary'
 import ErrorBoundaryFallback from './error-boundary-fallback'
+import { useCompileContext } from '../../../shared/context/compile-context'
 
 function PdfJsViewer({ url }) {
   const { _id: projectId } = useProjectContext()
 
+  const { setError } = useCompileContext()
+
   // state values persisted in localStorage to restore on load
   const [scale, setScale] = usePersistedState(
     `pdf-viewer-scale:${projectId}`,
@@ -27,7 +29,6 @@ function PdfJsViewer({ url }) {
   // local state values
   const [pdfJsWrapper, setPdfJsWrapper] = useState()
   const [initialised, setInitialised] = useState(false)
-  const [error, setError] = useState()
 
   // create the viewer when the container is mounted
   const handleContainer = useCallback(parent => {
@@ -52,12 +53,14 @@ function PdfJsViewer({ url }) {
     if (pdfJsWrapper && url) {
       setInitialised(false)
       setError(undefined)
-      // TODO: anything else to be reset?
 
-      pdfJsWrapper.loadDocument(url).catch(error => setError(error))
+      pdfJsWrapper.loadDocument(url).catch(error => {
+        console.error(error)
+        setError('rendering-error')
+      })
       return () => pdfJsWrapper.abortDocumentLoading()
     }
-  }, [pdfJsWrapper, url])
+  }, [pdfJsWrapper, url, setError])
 
   // listen for scroll events
   useEffect(() => {
@@ -249,11 +252,6 @@ function PdfJsViewer({ url }) {
       <div className="pdfjs-controls">
         <PdfViewerControls setZoom={setZoom} />
       </div>
-      {error && (
-        <div className="pdfjs-error">
-          <Alert bsStyle="danger">{error.message}</Alert>
-        </div>
-      )}
     </div>
   )
 }

services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.js

@@ -15,6 +15,7 @@ const params = new URLSearchParams(window.location.search)
 const disableFontFace = params.get('disable-font-face') === 'true'
 const cMapUrl = getMeta('ol-pdfCMapsPath')
 const imageResourcesPath = getMeta('ol-pdfImageResourcesPath')
+const disableStream = process.env.NODE_ENV !== 'test'
 
 const rangeChunkSize = 128 * 1024 // 128K chunks
 
@@ -56,8 +57,11 @@ export default class PDFJSWrapper {
 
   // load a document from a URL
   loadDocument(url) {
-    // prevents any previous loading task from populating the viewer
-    this.loadDocumentTask = undefined
+    // cancel any previous loading task
+    if (this.loadDocumentTask) {
+      this.loadDocumentTask.destroy()
+      this.loadDocumentTask = undefined
+    }
 
     return new Promise((resolve, reject) => {
       this.loadDocumentTask = PDFJS.getDocument({
@@ -67,7 +71,7 @@ export default class PDFJSWrapper {
         disableFontFace,
         rangeChunkSize,
         disableAutoFetch: true,
-        disableStream: true,
+        disableStream,
         textLayerMode: 2, // PDFJSViewer.TextLayerMode.ENABLE,
       })
 

services/web/frontend/js/shared/context/compile-context.js

@@ -45,6 +45,7 @@ CompileContext.Provider.propTypes = {
     rawLog: PropTypes.string,
     setAutoCompile: PropTypes.func.isRequired,
     setDraft: PropTypes.func.isRequired,
+    setError: PropTypes.func.isRequired,
     setHasLintingError: PropTypes.func.isRequired, // only for storybook
     setShowLogs: PropTypes.func.isRequired,
     setStopOnValidationError: PropTypes.func.isRequired,
@@ -411,6 +412,7 @@ export function CompileProvider({ children }) {
       setClearingCache,
       setCompiling,
       setDraft,
+      setError,
       setHasLintingError, // only for stories
       setShowLogs,
       setStopOnValidationError,
@@ -441,6 +443,7 @@ export function CompileProvider({ children }) {
       recompileFromScratch,
       setAutoCompile,
       setDraft,
+      setError,
       setHasLintingError,
       setStopOnValidationError,
       showLogs,

services/web/frontend/stylesheets/app/editor/pdf.less

@@ -260,12 +260,6 @@
       border-bottom: 2px solid white;
     }
   }
-  .pdfjs-error {
-    position: absolute;
-    top: 10px;
-    left: 10px;
-    right: 10px;
-  }
 }
 
 // The new viewer UI has overflow on the inner element,

services/web/test/frontend/features/pdf-preview/components/pdf-js-viewer.test.js

@@ -1,7 +1,6 @@
 import { expect } from 'chai'
 import { screen } from '@testing-library/react'
 import path from 'path'
-import nock from 'nock'
 import { renderWithEditorContext } from '../../../helpers/render-with-context'
 import { pathToFileURL } from 'url'
 import PdfJsViewer from '../../../../../frontend/js/features/pdf-preview/components/pdf-js-viewer'
@@ -10,10 +9,6 @@ const example = pathToFileURL(
   path.join(__dirname, '../fixtures/test-example.pdf')
 ).toString()
 
-const exampleCorrupt = pathToFileURL(
-  path.join(__dirname, '../fixtures/test-example-corrupt.pdf')
-).toString()
-
 describe('<PdfJSViewer/>', function () {
   it('loads all PDF pages', async function () {
     renderWithEditorContext(<PdfJsViewer url={example} />)
@@ -39,62 +34,4 @@ describe('<PdfJSViewer/>', function () {
     await screen.findByLabelText('Page 1')
     unmount()
   })
-
-  describe('with an invalid URL', function () {
-    it('renders an error alert', async function () {
-      nock('https://www.test-overleaf.com')
-        .get('/invalid/doc.pdf')
-        .replyWithError({
-          message: 'something awful happened',
-          code: 'AWFUL_ERROR',
-        })
-
-      renderWithEditorContext(<PdfJsViewer url="/invalid/doc.pdf" />)
-
-      await screen.findByRole('alert')
-      screen.getByText('something awful happened')
-
-      expect(screen.queryByLabelText('Page 1')).to.not.exist
-    })
-
-    it('can load another document after the error', async function () {
-      nock('https://www.test-overleaf.com')
-        .get('/invalid/doc.pdf')
-        .replyWithError({
-          message: 'something awful happened',
-          code: 'AWFUL_ERROR',
-        })
-
-      const { rerender } = renderWithEditorContext(
-        <PdfJsViewer url="/invalid/doc.pdf" />
-      )
-      await screen.findByRole('alert')
-      screen.getByText('something awful happened')
-
-      rerender(<PdfJsViewer url={example} />)
-
-      await screen.findByLabelText('Page 1')
-      expect(screen.queryByRole('alert')).to.not.exist
-    })
-  })
-
-  describe('with an corrupted document', function () {
-    it('renders an error alert', async function () {
-      renderWithEditorContext(<PdfJsViewer url={exampleCorrupt} />)
-      await screen.findByRole('alert')
-      expect(screen.queryByLabelText('Page 1')).to.not.exist
-    })
-
-    it('can load another document after the error', async function () {
-      const { rerender } = renderWithEditorContext(
-        <PdfJsViewer url={exampleCorrupt} />
-      )
-      await screen.findByRole('alert')
-
-      rerender(<PdfJsViewer url={example} />)
-
-      await screen.findByLabelText('Page 1')
-      expect(screen.queryByRole('alert')).to.not.exist
-    })
-  })
 })

services/web/test/frontend/features/pdf-preview/components/pdf-preview.test.js

@@ -4,12 +4,19 @@ import fetchMock from 'fetch-mock'
 import { screen, fireEvent, waitFor, cleanup } from '@testing-library/react'
 import PdfPreview from '../../../../../frontend/js/features/pdf-preview/components/pdf-preview'
 import { renderWithEditorContext } from '../../../helpers/render-with-context'
+import nock from 'nock'
+import path from 'path'
+import fs from 'fs'
+import { cloneDeep } from 'lodash'
+
+const examplePDF = path.join(__dirname, '../fixtures/test-example.pdf')
+const corruptPDF = path.join(__dirname, '../fixtures/test-example-corrupt.pdf')
 
 const outputFiles = [
   {
     path: 'output.pdf',
     build: '123',
-    url: '/build/output.pdf', // TODO: PDF URL to render
+    url: '/build/output.pdf',
     type: 'pdf',
   },
   {
@@ -51,7 +58,7 @@ const mockCompile = () =>
       clsiServerId: 'foo',
       compileGroup: 'priority',
       pdfDownloadDomain: '',
-      outputFiles,
+      outputFiles: cloneDeep(outputFiles),
     },
   })
 
@@ -77,7 +84,14 @@ const mockValidationProblems = validationProblems =>
 const mockClearCache = () =>
   fetchMock.delete('express:/project/:projectId/output', 204)
 
+const mockValidPdf = () => {
+  nock('https://www.test-overleaf.com')
+    .get(/^\/build\/output\.pdf/)
+    .replyWithFile(200, examplePDF)
+}
+
 const defaultFileResponses = {
+  '/build/output.pdf': () => fs.createReadStream(examplePDF),
   '/build/output.blg': 'This is BibTeX, Version 4.0', // FIXME
   '/build/output.log': `
 The LaTeX compiler output
@@ -139,13 +153,12 @@ describe('<PdfPreview/>', function () {
       shouldAdvanceTime: true,
       now: Date.now(),
     })
-    // xhrMock.setup()
+    nock.cleanAll()
   })
 
   afterEach(function () {
     clock.runAll()
     clock.restore()
-    // xhrMock.teardown()
     fetchMock.reset()
     localStorage.clear()
     sinon.restore()
@@ -154,6 +167,7 @@ describe('<PdfPreview/>', function () {
   it('renders the PDF preview', async function () {
     mockCompile()
     mockBuildFile()
+    mockValidPdf()
 
     renderWithEditorContext(<PdfPreview />, { scope })
 
@@ -165,6 +179,7 @@ describe('<PdfPreview/>', function () {
   it('runs a compile when the Recompile button is pressed', async function () {
     mockCompile()
     mockBuildFile()
+    mockValidPdf()
 
     renderWithEditorContext(<PdfPreview />, { scope })
 
@@ -172,6 +187,8 @@ describe('<PdfPreview/>', function () {
     await screen.findByRole('button', { name: 'Compiling…' })
     await screen.findByRole('button', { name: 'Recompile' })
 
+    mockValidPdf()
+
     // press the Recompile button => compile
     const button = screen.getByRole('button', { name: 'Recompile' })
     button.click()
@@ -184,6 +201,7 @@ describe('<PdfPreview/>', function () {
   it('runs a compile on doc change if autocompile is enabled', async function () {
     mockCompile()
     mockBuildFile()
+    mockValidPdf()
 
     renderWithEditorContext(<PdfPreview />, { scope })
 
@@ -194,6 +212,8 @@ describe('<PdfPreview/>', function () {
     // switch on auto compile
     storeAndFireEvent('autocompile_enabled:project123', true)
 
+    mockValidPdf()
+
     // fire a doc:changed event => compile
     fireEvent(window, new CustomEvent('doc:changed'))
     clock.tick(2000) // AUTO_COMPILE_DEBOUNCE
@@ -207,6 +227,7 @@ describe('<PdfPreview/>', function () {
   it('does not run a compile on doc change if autocompile is disabled', async function () {
     mockCompile()
     mockBuildFile()
+    mockValidPdf()
 
     renderWithEditorContext(<PdfPreview />, { scope })
 
@@ -228,6 +249,7 @@ describe('<PdfPreview/>', function () {
   it('does not run a compile on doc change if autocompile is blocked by syntax check', async function () {
     mockCompile()
     mockBuildFile()
+    mockValidPdf()
 
     renderWithEditorContext(<PdfPreview />, {
       scope: {
@@ -359,7 +381,7 @@ describe('<PdfPreview/>', function () {
   it('sends a clear cache request when the button is pressed', async function () {
     mockCompile()
     mockBuildFile()
-    mockClearCache()
+    mockValidPdf()
 
     renderWithEditorContext(<PdfPreview />, { scope })
 
@@ -377,6 +399,8 @@ describe('<PdfPreview/>', function () {
     })
     expect(clearCacheButton.hasAttribute('disabled')).to.be.false
 
+    mockClearCache()
+
     // click the button
     clearCacheButton.click()
     expect(clearCacheButton.hasAttribute('disabled')).to.be.true
@@ -391,7 +415,7 @@ describe('<PdfPreview/>', function () {
   it('handle "recompile from scratch"', async function () {
     mockCompile()
     mockBuildFile()
-    mockClearCache()
+    mockValidPdf()
 
     renderWithEditorContext(<PdfPreview />, { scope })
 
@@ -410,6 +434,9 @@ describe('<PdfPreview/>', function () {
     })
     expect(clearCacheButton.hasAttribute('disabled')).to.be.false
 
+    mockValidPdf()
+    mockClearCache()
+
     const recompileFromScratch = screen.getByRole('menuitem', {
       name: 'Recompile from scratch',
       hidden: true,
@@ -426,4 +453,51 @@ describe('<PdfPreview/>', function () {
     expect(fetchMock.called('express:/project/:projectId/output')).to.be.true
     expect(fetchMock.called('express:/build/:file')).to.be.true // TODO: actual path
   })
+
+  it('shows an error for an invalid URL', async function () {
+    mockCompile()
+    mockBuildFile()
+
+    nock('https://www.test-overleaf.com')
+      .get(/^\/build\/output.pdf/)
+      .replyWithError({
+        message: 'something awful happened',
+        code: 'AWFUL_ERROR',
+      })
+
+    const { rerender } = renderWithEditorContext(<PdfPreview />, { scope })
+
+    await screen.findByText('Something went wrong while rendering this PDF.')
+    expect(screen.queryByLabelText('Page 1')).to.not.exist
+
+    mockValidPdf()
+
+    rerender(<PdfPreview />)
+
+    await screen.findByLabelText('Page 1')
+    expect(screen.queryByText('Something went wrong while rendering this PDF.'))
+      .to.not.exist
+  })
+
+  it('shows an error for a corrupt PDF', async function () {
+    mockCompile()
+    mockBuildFile()
+
+    nock('https://www.test-overleaf.com')
+      .get(/^\/build\/output.pdf/)
+      .replyWithFile(200, corruptPDF)
+
+    const { rerender } = renderWithEditorContext(<PdfPreview />, { scope })
+
+    await screen.findByText('Something went wrong while rendering this PDF.')
+    expect(screen.queryByLabelText('Page 1')).to.not.exist
+
+    mockValidPdf()
+
+    rerender(<PdfPreview />)
+
+    await screen.findByLabelText('Page 1')
+    expect(screen.queryByText('Something went wrong while rendering this PDF.'))
+      .to.not.exist
+  })
 })