Skip to content

fix(history): wait for the Files router before tagging its entries #74

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
skjnldsv merged 1 commit into from
Oct 1, 2026
Merged

fix(history): wait for the Files router before tagging its entries #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 71 additions & 5 deletions __tests__/history.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ function setRouter(query: Record<string, string> = {}, params: Record<string, st
return router
}

/**
* Let the queued history changes land: each waits for the router's navigation.
*/
function settle(): Promise<void> {
return new Promise((resolve) => setTimeout(resolve, 0))
}

/**
* Get the popstate handler registered by the module through addEventListener.
*/
Expand Down Expand Up @@ -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(
Expand All @@ -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' }),
Expand All @@ -131,14 +140,66 @@ 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<void>((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<void>((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 })
openWithHistory([file], file, view, folder)
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)
Expand All @@ -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[] = []
Expand All @@ -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
Expand All @@ -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(
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion lib/global.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ interface OCPFilesRouter {
params?: Record<string, string>,
query?: Record<string, string | (string | null)[] | null | undefined>,
replace?: boolean,
) => void
) => Promise<unknown> | void
}

declare global {
Expand Down
112 changes: 69 additions & 43 deletions lib/utils/history.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> = 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>): 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.
Expand All @@ -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<string, string | (string | null)[] | null | undefined> = {
...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<string, string | (string | null)[] | null | undefined> = {
...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 }, '')
})
}

/**
Expand Down Expand Up @@ -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)
})
}

/**
Expand Down
Loading