Add uniqueness check to "Create project" command (#5100)

* Add failing playwright test

* Make create generate a unique name if the given one collides

* Add a new consolidated getUniqueProjectName function with tests

* Use getUniqueProjectName

* Replace "New project" button text with "Create project"
cc @pierremtb

* Extend the e2e test to show the incrementing behavior
cc @lf94

e2e/playwright/code-pane-and-errors.spec.ts

@@ -280,7 +280,7 @@ test(
 
       await expect(page.getByRole('link', { name: 'bracket' })).toBeVisible()
       await expect(page.getByText('router-template-slate')).toBeVisible()
-      await expect(page.getByText('New Project')).toBeVisible()
+      await expect(page.getByText('Create project')).toBeVisible()
     })
 
     await test.step('Opening the router-template project should load', async () => {

e2e/playwright/fixtures/cmdBarFixture.ts

@@ -135,4 +135,20 @@ export class CmdBarFixture {
       await promptEditCommand.first().click()
     }
   }
+
+  get cmdSearchInput() {
+    return this.page.getByTestId('cmd-bar-search')
+  }
+
+  get argumentInput() {
+    return this.page.getByTestId('cmd-bar-arg-value')
+  }
+
+  get cmdOptions() {
+    return this.page.getByTestId('cmd-bar-option')
+  }
+
+  chooseCommand = async (commandName: string) => {
+    await this.cmdOptions.getByText(commandName).click()
+  }
 }

e2e/playwright/fixtures/toolbarFixture.ts

@@ -63,6 +63,10 @@ export class ToolbarFixture {
     this.exeIndicator = page.getByTestId('model-state-indicator-execution-done')
   }
 
+  get logoLink() {
+    return this.page.getByTestId('app-logo')
+  }
+
   startSketchPlaneSelection = async () =>
     doAndWaitForImageDiff(this.page, () => this.startSketchBtn.click(), 500)
 

e2e/playwright/projects.spec.ts

@@ -172,7 +172,7 @@ test(
       await expect(page.getByRole('link', { name: 'bracket' })).toBeVisible()
       await expect(page.getByText('broken-code')).toBeVisible()
       await expect(page.getByText('bracket')).toBeVisible()
-      await expect(page.getByText('New Project')).toBeVisible()
+      await expect(page.getByText('Create project')).toBeVisible()
     })
     await test.step('opening broken code project should clear the scene and show the error', async () => {
       // Go back home.
@@ -253,7 +253,7 @@ test(
       await expect(page.getByRole('link', { name: 'bracket' })).toBeVisible()
       await expect(page.getByText('empty')).toBeVisible()
       await expect(page.getByText('bracket')).toBeVisible()
-      await expect(page.getByText('New Project')).toBeVisible()
+      await expect(page.getByText('Create project')).toBeVisible()
     })
     await test.step('opening empty code project should clear the scene', async () => {
       // Go back home.
@@ -985,6 +985,107 @@ test.describe(`Project management commands`, () => {
       })
     }
   )
+  test(`Create a new project with a colliding name`, async ({
+    context,
+    homePage,
+    toolbar,
+    cmdBar,
+  }) => {
+    const projectName = 'test-project'
+    await test.step(`Setup`, async () => {
+      await context.folderSetupFn(async (dir) => {
+        const projectDir = path.join(dir, projectName)
+        await Promise.all([fsp.mkdir(projectDir, { recursive: true })])
+        await Promise.all([
+          fsp.copyFile(
+            executorInputPath('router-template-slate.kcl'),
+            path.join(projectDir, 'main.kcl')
+          ),
+        ])
+      })
+      await homePage.expectState({
+        projectCards: [
+          {
+            title: projectName,
+            fileCount: 1,
+          },
+        ],
+        sortBy: 'last-modified-desc',
+      })
+    })
+
+    await test.step('Create a new project with the same name', async () => {
+      await cmdBar.openCmdBar()
+      await cmdBar.chooseCommand('create project')
+      await cmdBar.expectState({
+        stage: 'arguments',
+        commandName: 'Create project',
+        currentArgKey: 'name',
+        currentArgValue: '',
+        headerArguments: {
+          Name: '',
+        },
+        highlightedHeaderArg: 'name',
+      })
+      await cmdBar.argumentInput.fill(projectName)
+      await cmdBar.progressCmdBar()
+    })
+
+    await test.step(`Check the project was created with a non-colliding name`, async () => {
+      await toolbar.logoLink.click()
+      await homePage.expectState({
+        projectCards: [
+          {
+            title: projectName + '-1',
+            fileCount: 1,
+          },
+          {
+            title: projectName,
+            fileCount: 1,
+          },
+        ],
+        sortBy: 'last-modified-desc',
+      })
+    })
+
+    await test.step('Create another project with the same name', async () => {
+      await cmdBar.openCmdBar()
+      await cmdBar.chooseCommand('create project')
+      await cmdBar.expectState({
+        stage: 'arguments',
+        commandName: 'Create project',
+        currentArgKey: 'name',
+        currentArgValue: '',
+        headerArguments: {
+          Name: '',
+        },
+        highlightedHeaderArg: 'name',
+      })
+      await cmdBar.argumentInput.fill(projectName)
+      await cmdBar.progressCmdBar()
+    })
+
+    await test.step(`Check the second project was created with a non-colliding name`, async () => {
+      await toolbar.logoLink.click()
+      await homePage.expectState({
+        projectCards: [
+          {
+            title: projectName + '-2',
+            fileCount: 1,
+          },
+          {
+            title: projectName + '-1',
+            fileCount: 1,
+          },
+          {
+            title: projectName,
+            fileCount: 1,
+          },
+        ],
+        sortBy: 'last-modified-desc',
+      })
+    })
+  })
 })
 
 test(
@@ -1391,7 +1492,7 @@ extrude001 = extrude(200, sketch001)`)
     await page.getByTestId('app-logo').click()
 
     await expect(
-      page.getByRole('button', { name: 'New project' })
+      page.getByRole('button', { name: 'Create project' })
     ).toBeVisible()
 
     for (let i = 1; i <= 10; i++) {
@@ -1465,7 +1566,7 @@ test(
 
       await expect(page.getByRole('link', { name: 'bracket' })).toBeVisible()
       await expect(page.getByText('router-template-slate')).toBeVisible()
-      await expect(page.getByText('New Project')).toBeVisible()
+      await expect(page.getByText('Create project')).toBeVisible()
     })
 
     await test.step('Opening the router-template project should load the stream', async () => {
@@ -1494,7 +1595,7 @@ test(
 
       await expect(page.getByRole('link', { name: 'bracket' })).toBeVisible()
       await expect(page.getByText('router-template-slate')).toBeVisible()
-      await expect(page.getByText('New Project')).toBeVisible()
+      await expect(page.getByText('Create project')).toBeVisible()
     })
   }
 )

e2e/playwright/test-utils.ts

@@ -1078,7 +1078,7 @@ export async function createProject({
   returnHome?: boolean
 }) {
   await test.step(`Create project and navigate to it`, async () => {
-    await page.getByRole('button', { name: 'New project' }).click()
+    await page.getByRole('button', { name: 'Create project' }).click()
     await page.getByRole('textbox', { name: 'Name' }).fill(name)
     await page.getByRole('button', { name: 'Continue' }).click()
 

src/components/CommandBar/CommandArgOptionInput.tsx

@@ -134,6 +134,7 @@ function CommandArgOptionInput({
           </label>
           <Combobox.Input
             id="option-input"
+            data-testid="cmd-bar-arg-value"
             ref={inputRef}
             onChange={(event) =>
               !event.target.disabled && setQuery(event.target.value)

src/components/CommandComboBox.tsx

@@ -52,6 +52,7 @@ function CommandComboBox({
           className="w-5 h-5 bg-primary/10 dark:bg-primary text-primary dark:text-inherit"
         />
         <Combobox.Input
+          data-testid="cmd-bar-search"
           onChange={(event) => setQuery(event.target.value)}
           className="w-full bg-transparent focus:outline-none selection:bg-primary/20 dark:selection:bg-primary/40 dark:focus:outline-none"
           onKeyDown={(event) => {
@@ -85,6 +86,7 @@ function CommandComboBox({
             value={option}
             className="flex items-center gap-4 px-4 py-1.5 first:mt-2 last:mb-2 ui-active:bg-primary/10 dark:ui-active:bg-chalkboard-90 ui-disabled:!text-chalkboard-50"
             disabled={optionIsDisabled(option)}
+            data-testid={`cmd-bar-option`}
           >
             {'icon' in option && option.icon && (
               <CustomIcon name={option.icon} className="w-5 h-5" />

src/components/ProjectsContextProvider.tsx

@@ -18,6 +18,7 @@ import {
   getNextProjectIndex,
   interpolateProjectNameWithIndex,
   doesProjectNameNeedInterpolated,
+  getUniqueProjectName,
 } from 'lib/desktopFS'
 import { useSettingsAuthContext } from 'hooks/useSettingsAuthContext'
 import useStateMachineCommands from 'hooks/useStateMachineCommands'
@@ -195,15 +196,11 @@ const ProjectsContextDesktop = ({
               : settings.projects.defaultProjectName.current
           ).trim()
 
-          if (doesProjectNameNeedInterpolated(name)) {
+          const uniqueName = getUniqueProjectName(name, input.projects)
-            const nextIndex = getNextProjectIndex(name, input.projects)
+          await createNewProjectDirectory(uniqueName)
-            name = interpolateProjectNameWithIndex(name, nextIndex)
-          }
-
-          await createNewProjectDirectory(name)
 
           return {
-            message: `Successfully created "${name}"`,
+            message: `Successfully created "${uniqueName}"`,
             name,
           }
         }),

src/lib/desktopFS.test.ts

@@ -0,0 +1,58 @@
+import { getUniqueProjectName } from './desktopFS'
+import { FileEntry } from './project'
+
+/** Create a dummy project */
+function project(name: string, children?: FileEntry[]): FileEntry {
+  return {
+    name,
+    children: children || [
+      { name: 'main.kcl', children: null, path: 'main.kcl' },
+    ],
+    path: `/projects/${name}`,
+  }
+}
+
+describe(`Getting unique project names`, () => {
+  it(`should return the same name if no conflicts`, () => {
+    const projectName = 'new-project'
+    const projects = [project('existing-project'), project('another-project')]
+    const result = getUniqueProjectName(projectName, projects)
+    expect(result).toBe(projectName)
+  })
+  it(`should return a unique name if there is a conflict`, () => {
+    const projectName = 'existing-project'
+    const projects = [project('existing-project'), project('another-project')]
+    const result = getUniqueProjectName(projectName, projects)
+    expect(result).toBe('existing-project-1')
+  })
+  it(`should increment an ending index until a unique one is found`, () => {
+    const projectName = 'existing-project-1'
+    const projects = [
+      project('existing-project'),
+      project('existing-project-1'),
+      project('existing-project-2'),
+    ]
+    const result = getUniqueProjectName(projectName, projects)
+    expect(result).toBe('existing-project-3')
+  })
+  it(`should prefer the formatting of the index identifier if present`, () => {
+    const projectName = 'existing-project-$nn'
+    const projects = [
+      project('existing-project'),
+      project('existing-project-1'),
+      project('existing-project-2'),
+    ]
+    const result = getUniqueProjectName(projectName, projects)
+    expect(result).toBe('existing-project-03')
+  })
+  it(`be able to get an incrementing index regardless of padding zeroes`, () => {
+    const projectName = 'existing-project-$nn'
+    const projects = [
+      project('existing-project'),
+      project('existing-project-01'),
+      project('existing-project-2'),
+    ]
+    const result = getUniqueProjectName(projectName, projects)
+    expect(result).toBe('existing-project-03')
+  })
+})

src/lib/desktopFS.ts

@@ -54,8 +54,10 @@ export function getNextProjectIndex(
   const matches = projects.map((project) => project.name?.match(regex))
   const indices = matches
     .filter(Boolean)
-    .map((match) => match![1])
+    .map((match) => (match !== null ? match[1] : '-1'))
-    .map(Number)
+    .map((maybeMatchIndex) => {
+      return parseInt(maybeMatchIndex || '0', 10)
+    })
   const maxIndex = Math.max(...indices, -1)
   return maxIndex + 1
 }
@@ -83,6 +85,33 @@ export function doesProjectNameNeedInterpolated(projectName: string) {
   return projectName.includes(INDEX_IDENTIFIER)
 }
 
+/**
+ * Given a target name, which may include our magic index interpolation string,
+ * and a list of projects, return a unique name that doesn't conflict with any
+ * of the existing projects, incrementing any ending number if necessary.
+ * @param name
+ * @param projects
+ * @returns
+ */
+export function getUniqueProjectName(name: string, projects: FileEntry[]) {
+  // The name may have our magic index interpolation string in it
+  const needsInterpolation = doesProjectNameNeedInterpolated(name)
+
+  if (needsInterpolation) {
+    const nextIndex = getNextProjectIndex(name, projects)
+    return interpolateProjectNameWithIndex(name, nextIndex)
+  } else {
+    let newName = name
+    while (projects.some((project) => project.name === newName)) {
+      const nameEndsWithNumber = newName.match(/\d+$/)
+      newName = nameEndsWithNumber
+        ? newName.replace(/\d+$/, (num) => `${parseInt(num, 10) + 1}`)
+        : `${name}-1`
+    }
+    return newName
+  }
+}
+
 function escapeRegExpChars(string: string) {
   return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
 }

src/routes/Home.tsx

@@ -148,7 +148,7 @@ const Home = () => {
                 }}
                 data-testid="home-new-file"
               >
-                New project
+                Create project
               </ActionButton>
             </div>
             <div className="flex gap-2 items-center">