Keep the path as string instead of an array

There is no good reason to keep the path as an array of strings, usually we want to just append a file to the path for file operations as most of the code simply joined the path back to a string. This also makes sure the path is properly formatted as /path/to/thing/ with a leading and trailing /.
cockpit-project · Jul 31, 2024 · 83e58f0 · 83e58f0
1 parent 5db4441
commit 83e58f0
Show file tree

Hide file tree

Showing 13 changed files with 101 additions and 77 deletions.
diff --git a/src/app.tsx b/src/app.tsx
@@ -17,7 +17,7 @@
  * along with Cockpit; If not, see <http://www.gnu.org/licenses/>.
  */
 
-import React, { useContext, useEffect, useMemo, useState } from "react";
+import React, { useContext, useEffect, useState } from "react";
 
 import {
     AlertGroup, Alert, AlertVariant, AlertActionCloseButton
@@ -67,6 +67,25 @@ export const FilesContext = React.createContext({
 
 export const useFilesContext = () => useContext(FilesContext);
 
+export const usePath = () => {
+    const { options } = usePageLocation();
+    let currentPath = decodeURIComponent(options.path?.toString() || "/");
+
+    // Trim all trailing slashes
+    currentPath = currentPath.replace(/\/+$/, '');
+
+    // Our path will always be `/foo/` formatted
+    if (!currentPath.endsWith("/")) {
+        currentPath += "/";
+    }
+
+    if (!currentPath.startsWith("/")) {
+        currentPath = `/${currentPath}`;
+    }
+
+    return currentPath;
+};
+
 export const Application = () => {
     const { options } = usePageLocation();
     const [loading, setLoading] = useState(true);
@@ -79,10 +98,7 @@ export const Application = () => {
     const [alerts, setAlerts] = useState<Alert[]>([]);
     const [cwdInfo, setCwdInfo] = useState<FileInfo | null>(null);
 
-    const currentPath = decodeURIComponent(options.path?.toString() || "");
-    // the function itself is not expensive, but `path` is later used in expensive computation
-    // and changing its reference value on every render causes performance issues
-    const path = useMemo(() => currentPath?.split("/"), [currentPath]);
+    const path = usePath();
 
     useEffect(() => {
         cockpit.user().then(user => {
@@ -102,7 +118,7 @@ export const Application = () => {
             setSelected([]);
 
             const client = new FsInfoClient(
-                `/${currentPath}`,
+                path,
                 ["type", "mode", "size", "mtime", "user", "group", "target", "entries", "targets"],
                 { superuser: 'try' }
             );
@@ -126,7 +142,7 @@ export const Application = () => {
                 client.close();
             };
         },
-        [options, currentPath]
+        [options, path]
     );
 
     if (loading)
@@ -159,9 +175,7 @@ export const Application = () => {
                             </Alert>
                         ))}
                     </AlertGroup>
-                    <FilesBreadcrumbs
-                      path={path}
-                    />
+                    <FilesBreadcrumbs path={path} />
                     <PageSection>
                         <Sidebar isPanelRight hasGutter>
                             <SidebarContent>

diff --git a/src/common.ts b/src/common.ts
@@ -53,7 +53,11 @@ export function * map_permissions<T>(func: (value: number, label: string) => T)
 }
 
 export function basename(path: string) {
-    const elements = path.split('/');
+    if (path === '/') {
+        return path;
+    }
+
+    const elements = path.replace(/\/$/, '').split('/');
     if (elements.length === 0) {
         return '/';
     } else {

diff --git a/src/dialogs/permissions.jsx b/src/dialogs/permissions.jsx
@@ -32,7 +32,7 @@ import { etc_group_syntax, etc_passwd_syntax } from 'pam_user_parser';
 import { superuser } from 'superuser';
 
 import { useFilesContext } from '../app';
-import { map_permissions, inode_types } from '../common';
+import { map_permissions, inode_types, basename } from '../common';
 
 const _ = cockpit.gettext;
 
@@ -41,7 +41,7 @@ const EditPermissionsModal = ({ dialogResult, selected, path }) => {
 
     // Nothing selected means we act on the current working directory
     if (!selected) {
-        const directory_name = path[path.length - 1];
+        const directory_name = basename(path);
         selected = { ...cwdInfo, isCwd: true, name: directory_name };
     }
 
@@ -82,7 +82,7 @@ const EditPermissionsModal = ({ dialogResult, selected, path }) => {
         const ownerChanged = owner !== selected.user || group !== selected.group;
 
         try {
-            const directory = selected?.isCwd ? path.join("/") : path.join("/") + "/" + selected.name;
+            const directory = selected?.isCwd ? path : path + selected.name;
             if (permissionChanged)
                 await cockpit.spawn(["chmod", mode.toString(8), directory],
                                     { superuser: "try", err: "message" });

diff --git a/src/dialogs/rename.tsx b/src/dialogs/rename.tsx
@@ -74,7 +74,7 @@ function checkCanOverride(candidate: string, entries: Record<string, FileInfo>,
 
 const RenameItemModal = ({ dialogResult, path, selected } : {
     dialogResult: DialogResult<void>
-    path: string[],
+    path: string,
     selected: FolderFileInfo,
 }) => {
     const { cwdInfo } = useFilesContext();
@@ -84,12 +84,12 @@ const RenameItemModal = ({ dialogResult, path, selected } : {
     const [overrideFileName, setOverrideFileName] = useState(false);
 
     const renameItem = (force = false) => {
-        const newPath = path.join("/") + "/" + name;
+        const newPath = path + name;
         const mvCmd = ["mv", "--no-target-directory"];
         if (force) {
             mvCmd.push("--force");
         }
-        mvCmd.push(path.join("/") + "/" + selected.name, newPath);
+        mvCmd.push(path + selected.name, newPath);
 
         cockpit.spawn(mvCmd, { superuser: "try", err: "message" })
                 .then(() => {
@@ -167,6 +167,6 @@ const RenameItemModal = ({ dialogResult, path, selected } : {
     );
 };
 
-export function show_rename_dialog(dialogs: Dialogs, path: string[], selected: FolderFileInfo) {
+export function show_rename_dialog(dialogs: Dialogs, path: string, selected: FolderFileInfo) {
     dialogs.run(RenameItemModal, { path, selected });
 }
diff --git a/src/files-breadcrumbs.tsx b/src/files-breadcrumbs.tsx
@@ -37,18 +37,20 @@ import { basename } from "./common";
 
 const _ = cockpit.gettext;
 
-function BookmarkButton({ path }: { path: string[] }) {
+function BookmarkButton({ path }: { path: string }) {
     const [isOpen, setIsOpen] = React.useState(false);
     const [user, setUser] = React.useState<cockpit.UserInfo | null>(null);
     const [bookmarks, setBookmarks] = React.useState<string[]>([]);
     const [bookmarkHandle, setBookmarkHandle] = React.useState<cockpit.FileHandle<string> | null>(null);
 
     const { addAlert, cwdInfo } = useFilesContext();
 
-    const currentPath = path.join("/") || "/";
     const defaultBookmarks = [];
-    if (user?.home)
-        defaultBookmarks.push({ name: _("Home"), loc: user?.home }); // TODO: add trash
+    if (user?.home) {
+        // Ensure the home dir has a trailing / like the path
+        const home_dir = user.home.endsWith('/') ? user.home : `${user.home}/`;
+        defaultBookmarks.push({ name: _("Home"), loc: home_dir }); // TODO: add trash
+    }
 
     const parse_uri = (line: string) => {
         // Drop everything after the space, we don't show renames
@@ -58,8 +60,15 @@ function BookmarkButton({ path }: { path: string[] }) {
         line = line.replace('file://', '');
 
         // Nautilus decodes urls as paths can contain spaces
-        return line.split('/').map(part => decodeURIComponent(part))
+        let bookmark_path = line.split('/').map(part => decodeURIComponent(part))
                 .join('/');
+
+        // Ensure the bookmark has a trailing slash
+        if (!bookmark_path.endsWith('/')) {
+            bookmark_path = `${bookmark_path}/`;
+        }
+
+        return bookmark_path;
     };
 
     useInit(async () => {
@@ -99,11 +108,11 @@ function BookmarkButton({ path }: { path: string[] }) {
 
         try {
             await bookmarkHandle.modify((old_content: string) => {
-                if (bookmarks.includes(currentPath)) {
-                    return old_content.split('\n').filter(line => parse_uri(line) !== currentPath)
+                if (bookmarks.includes(path)) {
+                    return old_content.split('\n').filter(line => parse_uri(line) !== path)
                             .join('\n');
                 } else {
-                    const newBoomark = "file://" + path.map(part => encodeURIComponent(part))
+                    const newBoomark = "file://" + path.split('/').map(part => encodeURIComponent(part))
                             .join('/') + "\n";
                     return (old_content || '') + newBoomark;
                 }
@@ -129,8 +138,8 @@ function BookmarkButton({ path }: { path: string[] }) {
         return null;
 
     let actionText = null;
-    if (currentPath !== user.home) {
-        if (bookmarks.includes(currentPath)) {
+    if (!defaultBookmarks.some(bkmark => bkmark.loc === path)) {
+        if (bookmarks.includes(path)) {
             actionText = _("Remove current directory");
         } else if (cwdInfo !== null) {
             actionText = _("Add bookmark");
@@ -182,16 +191,11 @@ function BookmarkButton({ path }: { path: string[] }) {
     );
 }
 
-const PathBreadcrumbs = ({ path }: { path: string[] }) => {
-    // HACK: strip extraneous slashes as PF's breadcrumb can't handle them
-    // Refactor the path to be the fullpath not an array of strings
-    if (path.length >= 2 && path[0] === '' && path[1] === '') {
-        path = path.slice(1);
-    }
-
-    if (path.length > 1 && path[path.length - 1] === '') {
-        path = path.slice(path.length - 1);
-    }
+const PathBreadcrumbs = ({ path }: { path: string }) => {
+    // Strip the trailing slash as it gets in the way when splitting paths and
+    // adds an extra trailing slash in the UI which we don't want to show and
+    // causes duplicate keys for the path `/`.
+    const path_array = path.replace(/\/$/, "").split("/");
 
     function navigate(event: React.MouseEvent<HTMLElement>) {
         const { button, ctrlKey, metaKey } = event;
@@ -213,8 +217,8 @@ const PathBreadcrumbs = ({ path }: { path: string[] }) => {
 
     return (
         <Breadcrumb onClick={(event) => navigate(event)}>
-            {path.map((dir, i) => {
-                const url_path = path.slice(0, i + 1).join("/") || '/';
+            {path_array.map((dir, i) => {
+                const url_path = path_array.slice(0, i + 1).join("/") || '/';
                 // We can't use a relative path as that will use the iframe's
                 // url while we want the outer shell url. And we can't obtain
                 // the full path of the shell easily, so a middle click will
@@ -226,7 +230,7 @@ const PathBreadcrumbs = ({ path }: { path: string[] }) => {
                       key={url_path}
                       data-location={url_path}
                       to={link}
-                      isActive={i === path.length - 1}
+                      isActive={i === path_array.length - 1}
                     >
                         {i === 0 &&
                             <Tooltip
@@ -244,7 +248,7 @@ const PathBreadcrumbs = ({ path }: { path: string[] }) => {
 };
 
 // eslint-disable-next-line max-len
-export function FilesBreadcrumbs({ path }: { path: string[] }) {
+export function FilesBreadcrumbs({ path }: { path: string }) {
     const [editMode, setEditMode] = React.useState(false);
     const [newPath, setNewPath] = React.useState<string | null>(null);
 
@@ -265,7 +269,7 @@ export function FilesBreadcrumbs({ path }: { path: string[] }) {
 
     const enableEditMode = () => {
         setEditMode(true);
-        setNewPath(path.join("/") || "/");
+        setNewPath(path);
     };
 
     const changePath = () => {

diff --git a/src/files-card-body.tsx b/src/files-card-body.tsx
@@ -75,7 +75,7 @@ function compare(sortBy: Sort): (a: FolderFileInfo, b: FolderFileInfo) => number
 }
 
 const ContextMenuItems = ({ path, selected, setSelected, clipboard, setClipboard } : {
-    path: string[],
+    path: string,
     selected: FolderFileInfo[], setSelected: React.Dispatch<React.SetStateAction<FolderFileInfo[]>>,
     clipboard: string[], setClipboard: React.Dispatch<React.SetStateAction<string[]>>,
 }) => {
@@ -124,7 +124,7 @@ export const FilesCardBody = ({
     setCurrentFilter: React.Dispatch<React.SetStateAction<string>>,
     files: FolderFileInfo[],
     isGrid: boolean,
-    path: string[],
+    path: string,
     selected: FolderFileInfo[], setSelected: React.Dispatch<React.SetStateAction<FolderFileInfo[]>>,
     sortBy: Sort, setSortBy: React.Dispatch<React.SetStateAction<Sort>>,
     loadingFiles: boolean,
@@ -159,7 +159,7 @@ export const FilesCardBody = ({
     }
 
     const onDoubleClickNavigate = useCallback((file: FolderFileInfo) => {
-        const newPath = [...path, file.name].join("/");
+        const newPath = path + file.name;
         if (file.to === "dir") {
             cockpit.location.go("/", { path: encodeURIComponent(newPath) });
         }
@@ -281,7 +281,7 @@ export const FilesCardBody = ({
             } else if (e.key === "Enter" && selected.length === 1) {
                 onDoubleClickNavigate(selected[0]);
             } else if (e.key === "Delete" && selected.length !== 0) {
-                confirm_delete(dialogs, path.join("/") + "/", selected, setSelected);
+                confirm_delete(dialogs, path, selected, setSelected);
             }
         };
 

diff --git a/src/files-folder-view.tsx b/src/files-folder-view.tsx
@@ -37,7 +37,7 @@ export const FilesFolderView = ({
     setClipboard,
     setShowHidden,
 }: {
-    path: string[],
+    path: string,
     files: FolderFileInfo[],
     loadingFiles: boolean,
     showHidden: boolean,

diff --git a/src/header.tsx b/src/header.tsx
@@ -124,7 +124,7 @@ export const FilesCardHeader = ({
     isGrid: boolean, setIsGrid: React.Dispatch<React.SetStateAction<boolean>>,
     sortBy: Sort, setSortBy: React.Dispatch<React.SetStateAction<Sort>>
     showHidden: boolean, setShowHidden: React.Dispatch<React.SetStateAction<boolean>>,
-    path: string[],
+    path: string,
 }) => {
     return (
         <CardHeader className="card-actionbar">