Merge pull request #50669 from nextcloud/fix/files-show-details-when-no-action

fix(files): Do not download files with openfile query flag

Author: susnux

apps/files/src/components/FilesListVirtual.vue

@@ -57,9 +57,10 @@
 </template>
 
 <script lang="ts">
-import type { ComponentPublicInstance, PropType } from 'vue'
-import type { Node as NcNode } from '@nextcloud/files'
 import type { UserConfig } from '../types'
+import type { Node as NcNode } from '@nextcloud/files'
+import type { ComponentPublicInstance, PropType } from 'vue'
+import type { Location } from 'vue-router'
 
 import { defineComponent } from 'vue'
 import { getFileListHeaders, Folder, Permission, View, getFileActions, FileType } from '@nextcloud/files'
@@ -219,13 +220,12 @@ export default defineComponent({
 		},
 
 		openFile: {
-			handler() {
-				// wait for scrolling and updating the actions to settle
-				this.$nextTick(() => {
-					if (this.fileId && this.openFile) {
-						this.handleOpenFile(this.fileId)
-					}
-				})
+			async handler(openFile) {
+				if (!openFile || !this.fileId) {
+					return
+				}
+
+				await this.handleOpenFile(this.fileId)
 			},
 			immediate: true,
 		},
@@ -329,30 +329,42 @@ export default defineComponent({
 		 * Handle opening a file (e.g. by ?openfile=true)
 		 * @param fileId File to open
 		 */
-		handleOpenFile(fileId: number|null) {
-			if (fileId === null) {
+		async handleOpenFile(fileId: number) {
+			const node = this.nodes.find(n => n.fileid === fileId) as NcNode
+			if (node === undefined) {
 				return
 			}
 
-			const node = this.nodes.find(n => n.fileid === fileId) as NcNode
-			if (node === undefined || node.type === FileType.Folder) {
-				return
+			if (node.type === FileType.File) {
+				const defaultAction = getFileActions()
+					// Get only default actions (visible and hidden)
+					.filter((action) => !!action?.default)
+					// Find actions that are either always enabled or enabled for the current node
+					.filter((action) => !action.enabled || action.enabled([node], this.currentView))
+					.filter((action) => action.id !== 'download')
+					// Sort enabled default actions by order
+					.sort((a, b) => (a.order || 0) - (b.order || 0))
+					// Get the first one
+					.at(0)
+
+				// Some file types do not have a default action (e.g. they can only be downloaded)
+				// So if there is an enabled default action, so execute it
+				if (defaultAction) {
+					logger.debug('Opening file ' + node.path, { node })
+					return await defaultAction.exec(node, this.currentView, this.currentFolder.path)
+				}
 			}
+			// The file is either a folder or has no default action other than downloading
+			// in this case we need to open the details instead and remove the route from the history
+			const query = this.$route.query
+			delete query.openfile
+			query.opendetails = ''
 
-			logger.debug('Opening file ' + node.path, { node })
-			this.openFileId = fileId
-			const defaultAction = getFileActions()
-				// Get only default actions (visible and hidden)
-				.filter(action => !!action?.default)
-				// Find actions that are either always enabled or enabled for the current node
-				.filter((action) => !action.enabled || action.enabled([node], this.currentView))
-				// Sort enabled default actions by order
-				.sort((a, b) => (a.order || 0) - (b.order || 0))
-				// Get the first one
-				.at(0)
-			// Some file types do not have a default action (e.g. they can only be downloaded)
-			// So if there is an enabled default action, so execute it
-			defaultAction?.exec(node, this.currentView, this.currentFolder.path)
+			logger.debug('Ignore `openfile` query and replacing with `opendetails` for ' + node.path, { node })
+			await this.$router.replace({
+				...(this.$route as Location),
+				query,
+			})
 		},
 
 		onDragOver(event: DragEvent) {

apps/files/src/router/router.ts

@@ -3,20 +3,41 @@
  * SPDX-License-Identifier: AGPL-3.0-or-later
  */
 import type { RawLocation, Route } from 'vue-router'
-import type { ErrorHandler } from 'vue-router/types/router.d.ts'
-
 import { generateUrl } from '@nextcloud/router'
 import queryString from 'query-string'
-import Router from 'vue-router'
+import Router, { isNavigationFailure, NavigationFailureType } from 'vue-router'
 import Vue from 'vue'
+import logger from '../logger'
 
 Vue.use(Router)
 
 // Prevent router from throwing errors when we're already on the page we're trying to go to
-const originalPush = Router.prototype.push as (to, onComplete?, onAbort?) => Promise<Route>
-Router.prototype.push = function push(to: RawLocation, onComplete?: ((route: Route) => void) | undefined, onAbort?: ErrorHandler | undefined): Promise<Route> {
-	if (onComplete || onAbort) return originalPush.call(this, to, onComplete, onAbort)
-	return originalPush.call(this, to).catch(err => err)
+const originalPush = Router.prototype.push
+Router.prototype.push = (function(this: Router, ...args: Parameters<typeof originalPush>) {
+	if (args.length > 1) {
+		return originalPush.call(this, ...args)
+	}
+	return originalPush.call<Router, [RawLocation], Promise<Route>>(this, args[0]).catch(ignoreDuplicateNavigation)
+}) as typeof originalPush
+
+const originalReplace = Router.prototype.replace
+Router.prototype.replace = (function(this: Router, ...args: Parameters<typeof originalReplace>) {
+	if (args.length > 1) {
+		return originalReplace.call(this, ...args)
+	}
+	return originalReplace.call<Router, [RawLocation], Promise<Route>>(this, args[0]).catch(ignoreDuplicateNavigation)
+}) as typeof originalReplace
+
+/**
+ * Ignore duplicated-navigation error but forward real exceptions
+ * @param error The thrown error
+ */
+function ignoreDuplicateNavigation(error: unknown): void {
+	if (isNavigationFailure(error, NavigationFailureType.duplicated)) {
+		logger.debug('Ignoring duplicated navigation from vue-router', { error })
+	} else {
+		throw error
+	}
 }
 
 const router = new Router({

cypress/e2e/files/router-query.cy.ts

@@ -0,0 +1,180 @@
+/*!
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+import type { User } from '@nextcloud/cypress'
+import { join } from 'path'
+import { getRowForFileId } from './FilesUtils.ts'
+
+/**
+ * Check that the sidebar is opened for a specific file
+ * @param name The name of the file
+ */
+function sidebarIsOpen(name: string): void {
+	cy.get('[data-cy-sidebar]')
+		.should('be.visible')
+		.findByRole('heading', { name })
+		.should('be.visible')
+}
+
+/**
+ * Skip a test without viewer installed
+ */
+function skipIfViewerDisabled(this: Mocha.Context): void {
+	cy.runOccCommand('app:list --enabled --output json')
+		.then((exec) => exec.stdout)
+		.then((output) => JSON.parse(output))
+		.then((obj) => 'viewer' in obj.enabled)
+		.then((enabled) => {
+			if (!enabled) {
+				this.skip()
+			}
+		})
+}
+
+/**
+ * Check a file was not downloaded
+ * @param filename The expected filename
+ */
+function fileNotDownloaded(filename: string): void {
+	const downloadsFolder = Cypress.config('downloadsFolder')
+	cy.readFile(join(downloadsFolder, filename)).should('not.exist')
+}
+
+describe('Check router query flags:', function() {
+	let user: User
+	let imageId: number
+	let archiveId: number
+	let folderId: number
+
+	before(() => {
+		cy.createRandomUser().then(($user) => {
+			user = $user
+			cy.uploadFile(user, 'image.jpg')
+				.then((response) => { imageId = Number.parseInt(response.headers['oc-fileid']) })
+			cy.mkdir(user, '/folder')
+				.then((response) => { folderId = Number.parseInt(response.headers['oc-fileid']) })
+			cy.uploadContent(user, new Blob([]), 'application/zstd', '/archive.zst')
+				.then((response) => { archiveId = Number.parseInt(response.headers['oc-fileid']) })
+			cy.login(user)
+		})
+	})
+
+	describe('"opendetails"', () => {
+		it('open details for known file type', () => {
+			cy.visit(`/apps/files/files/${imageId}?opendetails`)
+
+			// see sidebar
+			sidebarIsOpen('image.jpg')
+
+			// but no viewer
+			cy.findByRole('dialog', { name: 'image.jpg' })
+				.should('not.exist')
+
+			// and no download
+			fileNotDownloaded('image.jpg')
+		})
+
+		it('open details for unknown file type', () => {
+			cy.visit(`/apps/files/files/${archiveId}?opendetails`)
+
+			// see sidebar
+			sidebarIsOpen('archive.zst')
+
+			// but no viewer
+			cy.findByRole('dialog', { name: 'archive.zst' })
+				.should('not.exist')
+
+			// and no download
+			fileNotDownloaded('archive.zst')
+		})
+
+		it('open details for folder', () => {
+			cy.visit(`/apps/files/files/${folderId}?opendetails`)
+
+			// see sidebar
+			sidebarIsOpen('folder')
+
+			// but no viewer
+			cy.findByRole('dialog', { name: 'folder' })
+				.should('not.exist')
+
+			// and no download
+			fileNotDownloaded('folder')
+		})
+	})
+
+	describe('"openfile"', function() {
+		/** Check the viewer is open and shows the image */
+		function viewerShowsImage(): void {
+			cy.findByRole('dialog', { name: 'image.jpg' })
+				.should('be.visible')
+				.find(`img[src*="fileId=${imageId}"]`)
+				.should('be.visible')
+		}
+
+		it('opens files with default action', function() {
+			skipIfViewerDisabled.call(this)
+
+			cy.visit(`/apps/files/files/${imageId}?openfile`)
+			viewerShowsImage()
+		})
+
+		it('opens files with default action using explicit query state', function() {
+			skipIfViewerDisabled.call(this)
+
+			cy.visit(`/apps/files/files/${imageId}?openfile=true`)
+			viewerShowsImage()
+		})
+
+		it('does not open files with default action when using explicitly query value `false`', function() {
+			skipIfViewerDisabled.call(this)
+
+			cy.visit(`/apps/files/files/${imageId}?openfile=false`)
+			getRowForFileId(imageId)
+				.should('be.visible')
+				.and('have.class', 'files-list__row--active')
+
+			cy.findByRole('dialog', { name: 'image.jpg' })
+				.should('not.exist')
+		})
+
+		it('does not open folders but shows details', () => {
+			cy.visit(`/apps/files/files/${folderId}?openfile`)
+
+			// See the URL was replaced
+			cy.url()
+				.should('match', /[?&]opendetails(&|=|$)/)
+				.and('not.match', /openfile/)
+
+			// See the sidebar is correctly opened
+			cy.get('[data-cy-sidebar]')
+				.should('be.visible')
+				.findByRole('heading', { name: 'folder' })
+				.should('be.visible')
+
+			// see the folder was not changed
+			getRowForFileId(imageId).should('exist')
+		})
+
+		it('does not open unknown file types but shows details', () => {
+			cy.visit(`/apps/files/files/${archiveId}?openfile`)
+
+			// See the URL was replaced
+			cy.url()
+				.should('match', /[?&]opendetails(&|=|$)/)
+				.and('not.match', /openfile/)
+
+			// See the sidebar is correctly opened
+			cy.get('[data-cy-sidebar]')
+				.should('be.visible')
+				.findByRole('heading', { name: 'archive.zst' })
+				.should('be.visible')
+
+			// See no file was downloaded
+			const downloadsFolder = Cypress.config('downloadsFolder')
+			cy.readFile(join(downloadsFolder, 'archive.zst')).should('not.exist')
+		})
+	})
+})

cypress/support/commands.ts

@@ -54,12 +54,12 @@ Cypress.Commands.add('enableUser', (user: User, enable = true) => {
  */
 Cypress.Commands.add('uploadFile', (user, fixture = 'image.jpg', mimeType = 'image/jpeg', target = `/${fixture}`) => {
 	// get fixture
-	return cy.fixture(fixture, 'base64').then(async file => {
-		// convert the base64 string to a blob
-		const blob = Cypress.Blob.base64StringToBlob(file, mimeType)
-
-		cy.uploadContent(user, blob, mimeType, target)
-	})
+	return cy.fixture(fixture, 'base64')
+		.then((file) => (
+			// convert the base64 string to a blob
+			Cypress.Blob.base64StringToBlob(file, mimeType)
+		))
+		.then((blob) => cy.uploadContent(user, blob, mimeType, target))
 })
 
 Cypress.Commands.add('setFileAsFavorite', (user: User, target: string, favorite = true) => {
@@ -98,7 +98,7 @@ Cypress.Commands.add('setFileAsFavorite', (user: User, target: string, favorite
 
 Cypress.Commands.add('mkdir', (user: User, target: string) => {
 	// eslint-disable-next-line cypress/unsafe-to-chain-command
-	cy.clearCookies()
+	return cy.clearCookies()
 		.then(async () => {
 			try {
 				const rootPath = `${Cypress.env('baseUrl')}/remote.php/dav/files/${encodeURIComponent(user.userId)}`
@@ -112,6 +112,7 @@ Cypress.Commands.add('mkdir', (user: User, target: string) => {
 					},
 				})
 				cy.log(`Created directory ${target}`, response)
+				return response
 			} catch (error) {
 				cy.log('error', error)
 				throw new Error('Unable to create directory')

cypress/support/cypress-e2e.d.ts

@@ -21,7 +21,7 @@ declare global {
 			 * Upload a file from the fixtures folder to a given user storage.
 			 * **Warning**: Using this function will reset the previous session
 			 */
-			uploadFile(user: User, fixture?: string, mimeType?: string, target?: string): Cypress.Chainable<void>,
+			uploadFile(user: User, fixture?: string, mimeType?: string, target?: string): Cypress.Chainable<AxiosResponse>,
 
 			/**
 			 * Upload a raw content to a given user storage.
@@ -38,7 +38,7 @@ declare global {
 			 * Create a new directory
 			 * **Warning**: Using this function will reset the previous session
 			 */
-			mkdir(user: User, target: string): Cypress.Chainable<void>,
+			mkdir(user: User, target: string): Cypress.Chainable<AxiosResponse>,
 
 			/**
 			 * Set a file as favorite (or remove from favorite)