From de10b9b513f1e1f320c3d59d9f6d4a56b9a92dc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6?= <14975046+skjnldsv@users.noreply.github.com> Date: Thu, 1 Oct 2026 14:41:31 +0200 Subject: [PATCH] fix(history): wait for the Files router before tagging its entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the Files app moved to vue-router 5, goToRoute() lands the navigation asynchronously and resolves once the new history entry exists. The viewer tagged the entry with its offset straight after asking for the push, so it tagged the previous one: closing then found no viewer entries to unwind and back reopened the file. Every history change now waits for the router's navigation, queued behind the ones before it, so the offset goes on the entry the push made and two quick navigations do not read each other's stale offset. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: John Molakvoæ <14975046+skjnldsv@users.noreply.github.com> --- __tests__/history.spec.ts | 76 ++++++++++++++++++++++++-- lib/global.d.ts | 2 +- lib/utils/history.ts | 112 +++++++++++++++++++++++--------------- 3 files changed, 141 insertions(+), 49 deletions(-) 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) + }) } /**