diff --git a/__tests__/history.spec.ts b/__tests__/history.spec.ts index a4ccd8d..0552768 100644 --- a/__tests__/history.spec.ts +++ b/__tests__/history.spec.ts @@ -35,6 +35,13 @@ function setRouter(query: Record = {}, params: Record { + return new Promise((resolve) => setTimeout(resolve, 0)) +} + /** * Get the popstate handler registered by the module through addEventListener. */ @@ -105,10 +112,11 @@ describe('openWithHistory', () => { expect(addSpy).not.toHaveBeenCalledWith('popstate', expect.anything()) }) - it('pushes a history entry and wires navigation callbacks on a fresh open', () => { + it('pushes a history entry and wires navigation callbacks on a fresh open', async () => { const router = setRouter() const file = makeFile({ id: 42 }) openWithHistory([file], file, view, folder) + await settle() // The opened file gets its own history entry, tagged with an offset. expect(router.goToRoute).toHaveBeenCalledWith( @@ -122,6 +130,7 @@ describe('openWithHistory', () => { router.goToRoute.mockClear() openOptions().onNext(makeFile({ id: 43 })) + await settle() expect(router.goToRoute).toHaveBeenCalledWith( 'filelist', expect.objectContaining({ fileid: '43' }), @@ -131,6 +140,58 @@ describe('openWithHistory', () => { expect(window.history.state?.viewerPos).toBe(2) }) + it('tags the entry the push made, once the router has landed it', async () => { + // The Files router lands a navigation asynchronously since vue-router 5: + // tagging before that tagged the page the viewer was opened from, so + // closing never unwound and back opened the file again + const router = setRouter() + let land!: () => void + vi.mocked(router.goToRoute).mockImplementation(() => new Promise((resolve) => { + land = () => { + window.history.pushState({}, '') + resolve() + } + })) + const file = makeFile({ id: 42 }) + openWithHistory([file], file, view, folder) + await settle() + + const opener = window.history.state + land() + await settle() + + expect(opener?.viewerPos).toBeUndefined() + expect(window.history.state?.viewerPos).toBe(1) + }) + + it('tags the entry the router created, once its navigation has landed', async () => { + // The Files router on vue-router 5 creates the history entry only once + // the navigation resolves. Tagging before that tagged the previous + // entry, and closing then had no viewer entries to unwind. + const router = setRouter() + vi.mocked(router.goToRoute).mockImplementation(() => new Promise((resolve) => { + setTimeout(() => { + window.history.pushState({}, '') + resolve() + }, 0) + })) + const landed = () => new Promise((resolve) => setTimeout(resolve, 20)) + const file = makeFile({ id: 42 }) + + openWithHistory([file], file, view, folder) + await landed() + expect(window.history.state?.viewerPos).toBe(1) + + openOptions().onNext(makeFile({ id: 43 })) + await landed() + expect(window.history.state?.viewerPos).toBe(2) + + router.query.openfile = 'true' + openOptions().onClose() + await landed() + expect(goSpy).toHaveBeenCalledWith(-2) + }) + it('does not push an entry when opened from an openfile URL (refresh)', () => { const router = setRouter({ openfile: 'true' }) const file = makeFile({ id: 7 }) @@ -138,7 +199,7 @@ describe('openWithHistory', () => { expect(router.goToRoute).not.toHaveBeenCalled() }) - it('unwinds every pushed entry when closed from within the viewer', () => { + it('unwinds every pushed entry when closed from within the viewer', async () => { const router = setRouter() const file = makeFile({ id: 1 }) openWithHistory([file], file, view, folder) @@ -147,15 +208,17 @@ describe('openWithHistory', () => { router.query.openfile = 'true' openOptions().onClose() + await settle() expect(goSpy).toHaveBeenCalledWith(-2) expect(removeSpy).toHaveBeenCalledWith('popstate', expect.any(Function)) }) - it('drops the openfile flag before the jump, not after it', () => { + it('drops the openfile flag before the jump, not after it', async () => { const router = setRouter() const file = makeFile({ id: 1 }) openWithHistory([file], file, view, folder) + await settle() router.query.openfile = 'true' const order: string[] = [] @@ -167,6 +230,7 @@ describe('openWithHistory', () => { }) openOptions().onClose() + await settle() // history.go() lands on a later task. Until it does the URL still says // openfile=true, and the Files list opens the file again if anything @@ -180,12 +244,13 @@ describe('openWithHistory', () => { ) }) - it('drops the openfile flag in place when closing a refresh-opened viewer', () => { + it('drops the openfile flag in place when closing a refresh-opened viewer', async () => { const router = setRouter({ openfile: 'true', dir: '/photos' }) const file = makeFile({ id: 1 }) openWithHistory([file], file, view, folder) openOptions().onClose() + await settle() expect(goSpy).not.toHaveBeenCalled() expect(router.goToRoute).toHaveBeenCalledWith( @@ -216,10 +281,11 @@ describe('openWithHistory', () => { expect(options.editing).toBe(true) }) - it('ignores a stale editing param on a fresh open (no openfile)', () => { + it('ignores a stale editing param on a fresh open (no openfile)', async () => { const router = setRouter({ editing: 'true' }) const file = makeFile({ id: 42 }) openWithHistory([file], file, view, folder) + await settle() const options = viewer.open.mock.calls[0]![2] as { editing: boolean } expect(options.editing).toBe(false) diff --git a/lib/global.d.ts b/lib/global.d.ts index e781a61..6ca80fd 100644 --- a/lib/global.d.ts +++ b/lib/global.d.ts @@ -16,7 +16,7 @@ interface OCPFilesRouter { params?: Record, query?: Record, replace?: boolean, - ) => void + ) => Promise | void } declare global { diff --git a/lib/utils/history.ts b/lib/utils/history.ts index 7700ee2..1c9e7e5 100644 --- a/lib/utils/history.ts +++ b/lib/utils/history.ts @@ -72,6 +72,27 @@ function currentOffset(): number { return typeof offset === 'number' ? offset : 0 } +/** + * The history changes still on their way, one after the other. + * + * The Files router lands a navigation asynchronously since its move to + * vue-router 5: `goToRoute()` resolves once the new entry exists. Tagging + * the entry before that tags the previous one, and two quick navigations + * would each read the offset the other had not written yet. + */ +let navigations: Promise = Promise.resolve() + +/** + * Run a history change once the ones before it have landed. + * + * @param change - What to do with the history + */ +function afterNavigations(change: () => Promise): void { + navigations = navigations + .then(change) + .catch((error) => logger.error('Could not keep the history in step with the viewer', { error })) +} + /** * Push a history entry for the given file so back/forward can reach it, tagging * it with the current viewer offset. @@ -85,22 +106,25 @@ function pushToHistory(node: IFile, view: IView, dir: string): void { if (!router || node.fileid === undefined) { return } - const offset = currentOffset() + 1 - // Do not carry a stale `editing` flag onto a freshly opened/navigated file; - // the editing state is (re)applied by updateEditingParam when actually editing. - const query: Record = { - ...router.query, - dir, - openfile: 'true', - } - delete query.editing - router.goToRoute( - routeName(router), - { ...router.params, view: view.id, fileid: String(node.fileid) }, - query, - false, - ) - window.history.replaceState({ ...window.history.state, viewerPos: offset }, '') + afterNavigations(async () => { + const offset = currentOffset() + 1 + // Do not carry a stale `editing` flag onto a freshly opened/navigated file; + // the editing state is (re)applied by updateEditingParam when actually editing. + const query: Record = { + ...router.query, + dir, + openfile: 'true', + } + delete query.editing + await router.goToRoute( + routeName(router), + { ...router.params, view: view.id, fileid: String(node.fileid) }, + query, + false, + ) + // Only now is the entry the push made the current one + window.history.replaceState({ ...window.history.state, viewerPos: offset }, '') + }) } /** @@ -157,35 +181,37 @@ function teardown(): void { function closeHistory(): void { teardown() - const router = getRouter() - if (!router || router.query?.openfile !== 'true') { - // Already left the viewer range (closed via back navigation): nothing to do. - return - } + afterNavigations(async () => { + const router = getRouter() + if (!router || router.query?.openfile !== 'true') { + // Already left the viewer range (closed via back navigation): nothing to do. + return + } - const query = { ...router.query } - delete query.openfile - delete query.editing - - const offset = currentOffset() - if (offset > 0) { - // Drop the flag on the entry being left before jumping. history.go() is - // asynchronous, and until it lands the URL still says openfile=true: - // anything that makes the Files list re-read the route in that window - // runs the default action again and opens a second viewer over the one - // that is closing. - router.goToRoute(routeName(router), router.params, query, true) - - // Jump back past every entry the viewer added, in one step, so the back - // button returns to the opening page instead of a previously shown file. - window.history.go(-offset) - return - } + const query = { ...router.query } + delete query.openfile + delete query.editing - // Opened from an openfile URL with no pre-viewer entry to return to - // (refresh): the flag comes off the current entry and there is nothing to - // unwind. - router.goToRoute(routeName(router), router.params, query, true) + const offset = currentOffset() + if (offset > 0) { + // Drop the flag on the entry being left before jumping. history.go() is + // asynchronous, and until it lands the URL still says openfile=true: + // anything that makes the Files list re-read the route in that window + // runs the default action again and opens a second viewer over the one + // that is closing. + await router.goToRoute(routeName(router), router.params, query, true) + + // Jump back past every entry the viewer added, in one step, so the back + // button returns to the opening page instead of a previously shown file. + window.history.go(-offset) + return + } + + // Opened from an openfile URL with no pre-viewer entry to return to + // (refresh): the flag comes off the current entry and there is nothing to + // unwind. + await router.goToRoute(routeName(router), router.params, query, true) + }) } /**