Respect cfg.packageRoot when creating text files using UI

catalog/CHANGELOG.md

@@ -18,6 +18,7 @@ where verb is one of
 
 ## Changes
 
+- [Fixed] Respect `packageRoot` config property when creating text files using Catalog UI ([#4397](https://github.com/quiltdata/quilt/pull/4397))
 - [Changed] Qurator: Switch to Claude 3.7 Sonnet ([#4343](https://github.com/quiltdata/quilt/pull/4343))
 - [Fixed] Qurator: Ensure tools schemas adhere to draft 2020-12 ([#4343](https://github.com/quiltdata/quilt/pull/4343))
 - [Added] Support setting the location where files are uploaded while creating or promoting packages (using `packageRoot` config property) ([#4384](https://github.com/quiltdata/quilt/pull/4384))

catalog/app/components/FileEditor/CreateFile.tsx

@@ -1,13 +1,14 @@
 import { join, extname } from 'path'
 
+import invariant from 'invariant'
 import * as React from 'react'
 import * as RRDom from 'react-router-dom'
 
 import * as Dialog from 'components/Dialog'
-import * as NamedRoutes from 'utils/NamedRoutes'
 import type { PackageHandle } from 'utils/packageHandle'
 
 import { isSupportedFileType } from './loader'
+import { useAddFileInBucket, useAddFileInPackage } from './routes'
 
 function validateFileName(value: string) {
   if (!value) {
@@ -20,60 +21,46 @@
 }
 
 export function useCreateFileInBucket(bucket: string, path: string) {
-  const { urls } = NamedRoutes.use()
   const history = RRDom.useHistory()
+  const toFile = useAddFileInBucket(bucket)
 
-  // TODO: put this into FileEditor/routes
-  const toFile = React.useCallback(
-    (name: string) => urls.bucketFile(bucket, join(path, name), { edit: true }),
-    [bucket, path, urls],
-  )
-
-  const createFile = React.useCallback(
+  const onSubmit = React.useCallback(
     (name: string) => {
-      if (!name) return
-      history.push(toFile(name))
+      invariant(
+        name,
+        '`name` should be invalidated, and `onSubmit` should not be triggered',
+      )
+      history.push(toFile(join(path, name)))
     },
-    [history, toFile],
+    [history, toFile, path],
   )
 
   return Dialog.usePrompt({
-    onSubmit: createFile,
+    onSubmit,
     initialValue: 'README.md',
     title: 'Enter file name',
     validate: validateFileName,
   })
 }
 
-export function useCreateFileInPackage({ bucket, name }: PackageHandle, prefix?: string) {
-  const { urls } = NamedRoutes.use()
+export function useCreateFileInPackage(packageHandle: PackageHandle, prefix?: string) {
   const history = RRDom.useHistory()
+  const toFile = useAddFileInPackage(packageHandle)
 
-  // TODO: put this into FileEditor/routes
-  const toFile = React.useCallback(
-    (fileName: string) => {
-      const next = urls.bucketPackageDetail(bucket, name, { action: 'revisePackage' })
-      const key = join(name, fileName)
-      return urls.bucketFile(bucket, key, {
-        add: fileName,
-        edit: true,
-        next,
-      })
-    },
-    [bucket, name, urls],
-  )
-
-  const createFile = React.useCallback(
+  const onSubmit = React.useCallback(
     (fileName: string) => {
-      if (!fileName) return
+      invariant(
+        fileName,
+        '`fileName` should be invalidated, and `onSubmit` should not be triggered',
+      )
       history.push(toFile(fileName))
     },
     [history, toFile],
   )
 
   const defaultFileName = 'README.md'
   return Dialog.usePrompt({
-    onSubmit: createFile,
+    onSubmit,
     initialValue: prefix ? `${prefix}${defaultFileName}` : defaultFileName,
     title: 'Enter file name',
     validate: validateFileName,

catalog/app/components/FileEditor/routes.spec.ts

@@ -1,12 +1,19 @@
 import { renderHook } from '@testing-library/react-hooks'
 
 import {
-  editFileInPackage,
   useAddFileInPackage,
+  useAddFileInBucket,
   useEditFileInPackage,
   useParams,
 } from './routes'
 
+jest.mock(
+  'constants/config',
+  jest.fn(() => ({
+    packageRoot: 'ro/ot',
+  })),
+)
+
 const useParamsInternal = jest.fn(
   () =>
     ({
@@ -34,24 +41,15 @@ jest.mock('utils/NamedRoutes', () => ({
 }))
 
 describe('components/FileEditor/routes', () => {
-  describe('editFileInPackage', () => {
-    it('should create url', () => {
-      expect(
-        editFileInPackage(urls, { bucket: 'bucket', key: 'key' }, 'logicalKey', 'next'),
-      ).toEqual('bucketFile(bucket, key, {"add":"logicalKey","edit":true,"next":"next"})')
-    })
-  })
-
   describe('useEditFileInPackage', () => {
     it('should create url with redirect to package', () => {
       const { result } = renderHook(() =>
         useEditFileInPackage(
           { bucket: 'b', name: 'n', hash: 'h' },
           { bucket: 'b', key: 'k' },
-          'lk',
         ),
       )
-      expect(result.current).toBe(
+      expect(result.current('lk')).toBe(
         'bucketFile(b, k, {"add":"lk","edit":true,"next":"bucketPackageDetail(b, n, {\\"action\\":\\"revisePackage\\"})"})',
       )
     })
@@ -60,14 +58,21 @@ describe('components/FileEditor/routes', () => {
   describe('useAddFileInPackage', () => {
     it('should create url for the new file', () => {
       const { result } = renderHook(() =>
-        useAddFileInPackage({ bucket: 'b', name: 'n', hash: 'h' }, 'lk'),
+        useAddFileInPackage({ bucket: 'b', name: 'n', hash: 'h' }),
       )
-      expect(result.current).toBe(
-        'bucketFile(b, n/lk, {"add":"lk","edit":true,"next":"bucketPackageDetail(b, n, {\\"action\\":\\"revisePackage\\"})"})',
+      expect(result.current('lk')).toBe(
+        'bucketFile(b, ro/ot/n/lk, {"add":"lk","edit":true,"next":"bucketPackageDetail(b, n, {\\"action\\":\\"revisePackage\\"})"})',
       )
     })
   })
 
+  describe('useAddFileInBucket', () => {
+    it('should create url for the new file in a bucket', () => {
+      const { result } = renderHook(() => useAddFileInBucket('b'))
+      expect(result.current('lk')).toBe(`bucketFile(b, lk, {"edit":true})`)
+    })
+  })
+
   describe('useParams', () => {
     it('should throw error when no bucket', () => {
       useParamsInternal.mockImplementationOnce(() => ({}))

catalog/app/components/FileEditor/routes.ts

@@ -1,9 +1,8 @@
-import { join } from 'path'
-
 import invariant from 'invariant'
 import * as React from 'react'
 import * as RRDom from 'react-router-dom'
 
+import cfg from 'constants/config'
 import type * as Routes from 'constants/routes'
 import type * as Model from 'model'
 import * as NamedRoutes from 'utils/NamedRoutes'
@@ -15,38 +14,52 @@ interface RouteMap {
   bucketPackageDetail: Routes.BucketPackageDetailArgs
 }
 
-export function editFileInPackage(
-  urls: NamedRoutes.Urls<RouteMap>,
-  handle: Model.S3.S3ObjectLocation,
-  logicalKey: string,
-  next: string,
-) {
-  return urls.bucketFile(handle.bucket, handle.key, {
-    add: logicalKey,
-    edit: true,
-    next,
-  })
-}
-
 export function useEditFileInPackage(
   packageHandle: PackageHandle,
   fileHandle: Model.S3.S3ObjectLocation,
-  logicalKey: string,
-) {
+): (logicalKey: string) => string {
   const { urls } = NamedRoutes.use<RouteMap>()
-  const { bucket, name } = packageHandle
-  const next = urls.bucketPackageDetail(bucket, name, { action: 'revisePackage' })
-  return editFileInPackage(urls, fileHandle, logicalKey, next)
+  return React.useCallback(
+    (logicalKey: string) =>
+      urls.bucketFile(fileHandle.bucket, fileHandle.key, {
+        add: logicalKey,
+        edit: true,
+        next: urls.bucketPackageDetail(packageHandle.bucket, packageHandle.name, {
+          action: 'revisePackage',
+        }),
+      }),
+    [fileHandle, packageHandle, urls],
+  )
+}
+
+export function useAddFileInPackage(
+  packageHandle: PackageHandle,
+): (logicalKey: string) => string {
+  const { urls } = NamedRoutes.use<RouteMap>()
+  return React.useCallback(
+    (logicalKey: string) => {
+      const { bucket, name } = packageHandle
+      invariant(logicalKey, '`logicalKey` can not be empty')
+      return urls.bucketFile(
+        bucket,
+        s3paths.canonicalKey(name, logicalKey, cfg.packageRoot),
+        {
+          add: logicalKey,
+          edit: true,
+          next: urls.bucketPackageDetail(bucket, name, { action: 'revisePackage' }),
+        },
+      )
+    },
+    [packageHandle, urls],
+  )
 }
 
-export function useAddFileInPackage({ bucket, name }: PackageHandle, logicalKey: string) {
+export function useAddFileInBucket(bucket: string): (logicalKey: string) => string {
   const { urls } = NamedRoutes.use<RouteMap>()
-  const next = urls.bucketPackageDetail(bucket, name, { action: 'revisePackage' })
-  const fileHandle = React.useMemo(
-    () => ({ bucket, key: join(name, logicalKey) }),
-    [bucket, logicalKey, name],
+  return React.useCallback(
+    (logicalKey: string) => urls.bucketFile(bucket, logicalKey, { edit: true }),
+    [bucket, urls],
   )
-  return editFileInPackage(urls, fileHandle, logicalKey, next)
 }
 
 export function useParams() {

catalog/app/containers/Bucket/PackageDialog/PackageCreationForm.tsx

@@ -332,17 +332,12 @@
       }
     }
 
-    const prefix = s3paths.withoutPrefix(
-      '/',
-      `${s3paths.ensureNoSlash(cfg.packageRoot || '')}/${name}`,
-    )
-
     let uploadedEntries
     try {
       uploadedEntries = await uploads.upload({
         files: toUpload,
         bucket: successor.slug,
-        prefix,
+        getCanonicalKey: (path) => s3paths.canonicalKey(name, path, cfg.packageRoot),
         getMeta: (path) => files.existing[path]?.meta || files.added[path]?.meta,
       })
     } catch (e) {

catalog/app/containers/Bucket/PackageDialog/Uploads.tsx

@@ -68,12 +68,12 @@
     async ({
       files,
       bucket,
-      prefix,
+      getCanonicalKey,
       getMeta,
     }: {
       files: { path: string; file: LocalFile }[]
       bucket: string
-      prefix: string
+      getCanonicalKey: (path: string) => string
       getMeta?: (path: string) => Model.EntryMeta | undefined
     }) => {
       const limit = pLimit(2)
@@ -89,7 +89,7 @@
         const upload: S3.ManagedUpload = s3.upload(
           {
             Bucket: bucket,
-            Key: `${prefix}/${path}`,
+            Key: getCanonicalKey(path),
             Body: file,
           },
           {

catalog/app/containers/Bucket/PackageTree/PackageTree.tsx

@@ -697,8 +697,11 @@
     [file, packageHandle],
   )
 
-  const editUrl = FileEditor.useEditFileInPackage(packageHandle, handle, path)
-  const handleEdit = React.useCallback(() => history.push(editUrl), [editUrl, history])
+  const editUrl = FileEditor.useEditFileInPackage(packageHandle, handle)
+  const handleEdit = React.useCallback(
+    () => history.push(editUrl(path)),
+    [editUrl, history, path],
+  )
   const packageUri = React.useMemo(
     () => ({
       ...packageHandle,

catalog/app/containers/Bucket/Summarize.tsx

@@ -692,22 +692,18 @@ export function ConfigureAppearance({
   path,
 }: ConfigureAppearanceProps) {
   const classes = useConfigureAppearanceStyles()
-  const readme = FileEditor.useAddFileInPackage(
-    packageHandle,
-    join(path || '', 'README.md'),
-  )
-  const summarize = FileEditor.useAddFileInPackage(
-    packageHandle,
-    join(path || '', 'quilt_summarize.json'),
-  )
+  const toFile = FileEditor.useAddFileInPackage(packageHandle)
   return (
     <div className={classes.root}>
       {!hasSummarizeJson && (
         <StyledTooltip
           maxWidth="md"
           title="Open the editor to author a quilt_summarize.json file. Upon saving, a package revision dialog will show up, letting you add that file to the package."
         >
-          <RRDom.Link to={summarize} className={classes.button}>
+          <RRDom.Link
+            to={toFile(join(path || '', 'quilt_summarize.json'))}
+            className={classes.button}
+          >
             <M.Button color="primary" size="small" variant="outlined">
               Configure Summary
             </M.Button>
@@ -719,7 +715,10 @@ export function ConfigureAppearance({
           maxWidth="md"
           title="Open the editor to author a README file. Upon saving, a package revision dialog will show up, letting you add that file to the package."
         >
-          <RRDom.Link to={readme} className={classes.button}>
+          <RRDom.Link
+            to={toFile(join(path || '', 'README.md'))}
+            className={classes.button}
+          >
             <M.Button color="primary" size="small" variant="contained">
               Add README
             </M.Button>

catalog/app/utils/s3paths.spec.ts

@@ -0,0 +1,35 @@
+import { canonicalKey } from './s3paths'
+
+describe('utils/s3paths', () => {
+  describe('canonicalKey', () => {
+    it('produces the key prefixed by package name', () => {
+      expect(canonicalKey('foo/bar', 'README.md')).toBe('foo/bar/README.md')
+      expect(canonicalKey('foo/bar', 'one/two two/three three three/README.md')).toBe(
+        'foo/bar/one/two two/three three three/README.md',
+      )
+      expect(canonicalKey('foo/bar', 'one/two two/three three three/README.md', '')).toBe(
+        'foo/bar/one/two two/three three three/README.md',
+      )
+    })
+
+    it('throws when logicalKey or package name is empty', () => {
+      expect(() => canonicalKey('foo/bar', '', 'root')).toThrow()
+      expect(() => canonicalKey('', 'foo/bar', 'root')).toThrow()
+    })
+
+    it('produces the key prefixed by package name and packageRoot', () => {
+      expect(canonicalKey('foo/bar', 'READ/ME.md', 'root')).toBe(
+        'root/foo/bar/READ/ME.md',
+      )
+      expect(canonicalKey('foo/bar', 'READ/ME.md', '/root/')).toBe(
+        'root/foo/bar/READ/ME.md',
+      )
+      expect(canonicalKey('foo/bar', 'READ?/!ME.md', '//root//')).toBe(
+        'root/foo/bar/READ?/!ME.md',
+      )
+      expect(canonicalKey('foo/bar', 'READ/ME.md', 'one/two two/three three three')).toBe(
+        'one/two two/three three three/foo/bar/READ/ME.md',
+      )
+    })
+  })
+})