diff --git a/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.js b/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.js index 7a3d60fd6e..fd12868987 100644 --- a/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.js +++ b/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 }) {
- {error && ( -
- {error.message} -
- )} ) } diff --git a/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.js b/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.js index a9013df2ee..e74c080aff 100644 --- a/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.js +++ b/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, }) diff --git a/services/web/frontend/js/shared/context/compile-context.js b/services/web/frontend/js/shared/context/compile-context.js index 11b6fb62fe..85a6c5ec48 100644 --- a/services/web/frontend/js/shared/context/compile-context.js +++ b/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, diff --git a/services/web/frontend/stylesheets/app/editor/pdf.less b/services/web/frontend/stylesheets/app/editor/pdf.less index 5a68c55088..6dcea4a449 100644 --- a/services/web/frontend/stylesheets/app/editor/pdf.less +++ b/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, diff --git a/services/web/test/frontend/features/pdf-preview/components/pdf-js-viewer.test.js b/services/web/test/frontend/features/pdf-preview/components/pdf-js-viewer.test.js index 64b3888ea5..03d146a7bb 100644 --- a/services/web/test/frontend/features/pdf-preview/components/pdf-js-viewer.test.js +++ b/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('', function () { it('loads all PDF pages', async function () { renderWithEditorContext() @@ -39,62 +34,4 @@ describe('', 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() - - 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( - - ) - await screen.findByRole('alert') - screen.getByText('something awful happened') - - rerender() - - 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() - 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( - - ) - await screen.findByRole('alert') - - rerender() - - await screen.findByLabelText('Page 1') - expect(screen.queryByRole('alert')).to.not.exist - }) - }) }) diff --git a/services/web/test/frontend/features/pdf-preview/components/pdf-preview.test.js b/services/web/test/frontend/features/pdf-preview/components/pdf-preview.test.js index 842d73369e..6bc49d4194 100644 --- a/services/web/test/frontend/features/pdf-preview/components/pdf-preview.test.js +++ b/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('', 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('', function () { it('renders the PDF preview', async function () { mockCompile() mockBuildFile() + mockValidPdf() renderWithEditorContext(, { scope }) @@ -165,6 +179,7 @@ describe('', function () { it('runs a compile when the Recompile button is pressed', async function () { mockCompile() mockBuildFile() + mockValidPdf() renderWithEditorContext(, { scope }) @@ -172,6 +187,8 @@ describe('', 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('', function () { it('runs a compile on doc change if autocompile is enabled', async function () { mockCompile() mockBuildFile() + mockValidPdf() renderWithEditorContext(, { scope }) @@ -194,6 +212,8 @@ describe('', 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('', function () { it('does not run a compile on doc change if autocompile is disabled', async function () { mockCompile() mockBuildFile() + mockValidPdf() renderWithEditorContext(, { scope }) @@ -228,6 +249,7 @@ describe('', function () { it('does not run a compile on doc change if autocompile is blocked by syntax check', async function () { mockCompile() mockBuildFile() + mockValidPdf() renderWithEditorContext(, { scope: { @@ -359,7 +381,7 @@ describe('', function () { it('sends a clear cache request when the button is pressed', async function () { mockCompile() mockBuildFile() - mockClearCache() + mockValidPdf() renderWithEditorContext(, { scope }) @@ -377,6 +399,8 @@ describe('', 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('', function () { it('handle "recompile from scratch"', async function () { mockCompile() mockBuildFile() - mockClearCache() + mockValidPdf() renderWithEditorContext(, { scope }) @@ -410,6 +434,9 @@ describe('', 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('', 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(, { scope }) + + await screen.findByText('Something went wrong while rendering this PDF.') + expect(screen.queryByLabelText('Page 1')).to.not.exist + + mockValidPdf() + + rerender() + + 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(, { scope }) + + await screen.findByText('Something went wrong while rendering this PDF.') + expect(screen.queryByLabelText('Page 1')).to.not.exist + + mockValidPdf() + + rerender() + + await screen.findByLabelText('Page 1') + expect(screen.queryByText('Something went wrong while rendering this PDF.')) + .to.not.exist + }) })