Only show "Edit sketch" button when code pane is focused with sketch selected (#5691)

* Only show "Edit sketch" button when code pane is focused with sketch selected

Closes #4273. WIP until tests are updated, since this will impact many
that look for the "Edit sketch" button.

* Start removing "edit sketch" point-and-click from E2E

* Update more E2E tests

* Update more tests that assumed Edit Sketch button

* A snapshot a day keeps the bugs away! 📷🐛

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

e2e/playwright/fixtures/toolbarFixture.ts

@@ -1,4 +1,4 @@
-import type { Page, Locator } from '@playwright/test'
+import { type Page, type Locator, test } from '@playwright/test'
 import { expect } from '../zoo-test'
 import {
   checkIfPaneIsOpen,
@@ -76,10 +76,6 @@ export class ToolbarFixture {
     this.gizmoDisabled = page.getByTestId('gizmo-disabled')
   }
 
-  get editSketchBtn() {
-    return this.page.locator('[name="Edit Sketch"]')
-  }
-
   get logoLink() {
     return this.page.getByTestId('app-logo')
   }
@@ -115,11 +111,20 @@ export class ToolbarFixture {
     ).not.toBeDisabled()
   }
 
-  editSketch = async () => {
-    await this.editSketchBtn.first().click()
+  editSketch = async (operationIndex = 0) => {
+    await test.step(`Editing sketch`, async () => {
+      await this.openFeatureTreePane()
+      const operation = await this.getFeatureTreeOperation(
+        'Sketch',
+        operationIndex
+      )
+      await operation.dblclick()
       // One of the rare times we want to allow a arbitrary wait
       // this is for the engine animation, as it takes 500ms to complete
       await this.page.waitForTimeout(600)
+      await expect(this.exitSketchBtn).toBeEnabled()
+      await this.closeFeatureTreePane()
+    })
   }
   private _getMode = () =>
     this.page.locator('[data-current-mode]').getAttribute('data-current-mode')

e2e/playwright/sketch-tests.spec.ts

@@ -12,6 +12,7 @@ import {
 } from './test-utils'
 import { uuidv4, roundOff } from 'lib/utils'
 import { SceneFixture } from './fixtures/sceneFixture'
+import { ToolbarFixture } from './fixtures/toolbarFixture'
 import { CmdBarFixture } from './fixtures/cmdBarFixture'
 
 test.describe('Sketch tests', { tag: ['@skipWin'] }, () => {
@@ -193,6 +194,7 @@ sketch001 = startProfileAt([12.34, -12.34], sketch002)
       homePage: HomePageFixture,
       openPanes: string[],
       scene: SceneFixture,
+      toolbar: ToolbarFixture,
       cmdBar: CmdBarFixture
     ) => {
       // Load the app with the code panes
@@ -282,11 +284,7 @@ sketch001 = startProfileAt([12.34, -12.34], sketch002)
         // Select the sketch
         await page.mouse.click(700, 370)
       }
-      await expect(
-        page.getByRole('button', { name: 'Edit Sketch' })
-      ).toBeVisible()
-      await page.getByRole('button', { name: 'Edit Sketch' }).click()
-      await page.waitForTimeout(400)
+      await toolbar.editSketch()
       if (openPanes.includes('code')) {
         prevContent = await page.locator('.cm-content').innerText()
       }
@@ -417,7 +415,7 @@ sketch001 = startProfileAt([12.34, -12.34], sketch002)
     test(
       'code pane open at start-handles',
       { tag: ['@skipWin'] },
-      async ({ page, homePage, scene, cmdBar }) => {
+      async ({ page, homePage, scene, toolbar, cmdBar }) => {
         // Load the app with the code panes
         await page.addInitScript(async () => {
           localStorage.setItem(
@@ -435,6 +433,7 @@ sketch001 = startProfileAt([12.34, -12.34], sketch002)
           homePage,
           ['code'],
           scene,
+          toolbar,
           cmdBar
         )
       }
@@ -443,7 +442,7 @@ sketch001 = startProfileAt([12.34, -12.34], sketch002)
     test(
       'code pane closed at start-handles',
       { tag: ['@skipWin'] },
-      async ({ page, homePage, scene, cmdBar }) => {
+      async ({ page, homePage, scene, toolbar, cmdBar }) => {
         // Load the app with the code panes
         await page.addInitScript(async (persistModelingContext) => {
           localStorage.setItem(
@@ -451,7 +450,14 @@ sketch001 = startProfileAt([12.34, -12.34], sketch002)
             JSON.stringify({ openPanes: [] })
           )
         }, PERSIST_MODELING_CONTEXT)
-        await doEditSegmentsByDraggingHandle(page, homePage, [], scene, cmdBar)
+        await doEditSegmentsByDraggingHandle(
+          page,
+          homePage,
+          [],
+          scene,
+          toolbar,
+          cmdBar
+        )
       }
     )
   })
@@ -2545,7 +2551,7 @@ profile002 = startProfileAt([85.81, 52.55], sketch002)
       const [startProfileAt] = scene.makeMouseHelpers(606, 184)
       const [nextPoint] = scene.makeMouseHelpers(763, 130)
       await page.getByText('startProfileAt([85.81, 52.55], sketch002)').click()
-      await toolbar.editSketch()
+      await toolbar.editSketch(1)
       // timeout wait for engine animation is unavoidable
       await page.waitForTimeout(600)
 
@@ -2765,7 +2771,7 @@ extrude003 = extrude(profile011, length = 2.5)
           await test.step(title, async () => {
             await camPositionForSelectingSketchOnWallProfiles()
             await selectClick()
-            await toolbar.editSketch()
+            await toolbar.editSketch(1)
             await page.waitForTimeout(600)
             await verifyWallProfilesAreDrawn()
             await toolbar.exitSketchBtn.click()

e2e/playwright/test-network-and-connection-issues.spec.ts

@@ -83,7 +83,7 @@ test.fixme('Test network and connection issues', () => {
   test(
     'Engine disconnect & reconnect in sketch mode',
     { tag: '@skipLocalEngine' },
-    async ({ page, homePage }) => {
+    async ({ page, homePage, toolbar }) => {
       const networkToggle = page.getByTestId('network-toggle')
 
       const u = await getUtils(page)
@@ -173,11 +173,7 @@ test.fixme('Test network and connection issues', () => {
         .click()
 
       // enter sketch again
-      await u.doAndWaitForCmd(
-        () => page.getByRole('button', { name: 'Edit Sketch' }).click(),
-        'default_camera_get_settings'
-      )
-      await page.waitForTimeout(150)
+      await toolbar.editSketch()
 
       // Click the line tool
       await page.getByRole('button', { name: 'line Line', exact: true }).click()
@@ -201,6 +197,7 @@ test.fixme('Test network and connection issues', () => {
           type: 'default_camera_get_settings',
         },
       }
+      await toolbar.openPane('debug')
       await u.sendCustomCmd(camCommand)
       await page.waitForTimeout(100)
       await u.sendCustomCmd(updateCamCommand)

e2e/playwright/testing-gizmo.spec.ts

@@ -316,14 +316,11 @@ test.describe(`Testing gizmo, fixture-based`, () => {
     })
 
     await test.step(`Gizmo should be disabled when in sketch mode`, async () => {
-      const sketchModeButton = page.getByRole('button', {
-        name: 'Edit sketch',
-      })
       const exitSketchButton = page.getByRole('button', {
         name: 'Exit sketch',
       })
 
-      await sketchModeButton.click()
+      await toolbar.editSketch()
       await expect(exitSketchButton).toBeVisible()
       const gizmoPopoverButton = page.getByRole('button', {
         name: 'view settings',

e2e/playwright/testing-selections.spec.ts

@@ -10,6 +10,7 @@ test.describe('Testing selections', { tag: ['@skipWin'] }, () => {
   test('Selections work on fresh and edited sketch', async ({
     page,
     homePage,
+    toolbar,
   }) => {
     // tests mapping works on fresh sketch and edited sketch
     // tests using hovers which is the same as selections, because if
@@ -216,12 +217,7 @@ test.describe('Testing selections', { tag: ['@skipWin'] }, () => {
     await emptySpaceHover()
 
     // enter sketch again
-    await u.doAndWaitForCmd(
-      () => page.getByRole('button', { name: 'Edit Sketch' }).click(),
-      'default_camera_get_settings'
-    )
-
-    await page.waitForTimeout(450) // wait for animation
+    await toolbar.editSketch()
 
     await u.openAndClearDebugPanel()
     await u.sendCustomCmd({

e2e/playwright/testing-settings.spec.ts

@@ -724,7 +724,14 @@ test.describe('Testing settings', () => {
     })
   })
 
-  test('Changing theme in sketch mode', async ({ context, page, homePage }) => {
+  test('Changing theme in sketch mode', async ({
+    context,
+    page,
+    homePage,
+    toolbar,
+    scene,
+    cmdBar,
+  }) => {
     // TODO: fix this test on windows after the electron migration
     test.skip(process.platform === 'win32', 'Skip on windows')
     const u = await getUtils(page)
@@ -744,11 +751,10 @@ test.describe('Testing settings', () => {
     })
     await page.setBodyDimensions({ width: 1200, height: 500 })
     await homePage.goToModelingScene()
-    await u.waitForPageLoad()
+    await scene.settled(cmdBar)
     await page.waitForTimeout(1000)
 
     // Selectors and constants
-    const editSketchButton = page.getByRole('button', { name: 'Edit Sketch' })
     const lineToolButton = page.getByTestId('line')
     const segmentOverlays = page.getByTestId('segment-overlay')
     const sketchOriginLocation = { x: 600, y: 250 }
@@ -757,8 +763,7 @@ test.describe('Testing settings', () => {
 
     await test.step(`Get into sketch mode`, async () => {
       await page.mouse.click(700, 200)
-      await expect(editSketchButton).toBeVisible()
-      await editSketchButton.click()
+      await toolbar.editSketch()
 
       // We use the line tool as a proxy for sketch mode
       await expect(lineToolButton).toBeVisible()

src/Toolbar.tsx

@@ -1,6 +1,6 @@
 import { useRef, useMemo, memo, useCallback, useState } from 'react'
 import { isCursorInSketchCommandRange } from 'lang/util'
-import { engineCommandManager, kclManager } from 'lib/singletons'
+import { editorManager, engineCommandManager, kclManager } from 'lib/singletons'
 import { useModelingContext } from 'hooks/useModelingContext'
 import { useNetworkContext } from 'hooks/useNetworkContext'
 import { NetworkHealthState } from 'hooks/useNetworkStatus'
@@ -77,8 +77,15 @@ export function Toolbar({
       modelingState: state,
       modelingSend: send,
       sketchPathId,
+      editorHasFocus: editorManager.editorView?.hasFocus,
     }),
-    [state, send, commandBarActor.send, sketchPathId]
+    [
+      state,
+      send,
+      commandBarActor.send,
+      sketchPathId,
+      editorManager.editorView?.hasFocus,
+    ]
   )
 
   const tooltipContentClassName = !showRichContent

src/lib/toolbar.ts

@@ -20,6 +20,7 @@ export interface ToolbarItemCallbackProps {
   modelingState: StateFrom<typeof modelingMachine>
   modelingSend: (event: EventFrom<typeof modelingMachine>) => void
   sketchPathId: string | false
+  editorHasFocus: boolean | undefined
 }
 
 export type ToolbarItem = {
@@ -65,8 +66,8 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
     items: [
       {
         id: 'sketch',
-        onClick: ({ modelingSend, sketchPathId }) =>
-          !sketchPathId
+        onClick: ({ modelingSend, sketchPathId, editorHasFocus }) =>
+          !(editorHasFocus && sketchPathId)
             ? modelingSend({
                 type: 'Enter sketch',
                 data: { forceNewSketch: true },
@@ -74,8 +75,8 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
             : modelingSend({ type: 'Enter sketch' }),
         icon: 'sketch',
         status: 'available',
-        title: ({ sketchPathId }) =>
-          sketchPathId ? 'Edit Sketch' : 'Start Sketch',
+        title: ({ editorHasFocus, sketchPathId }) =>
+          editorHasFocus && sketchPathId ? 'Edit Sketch' : 'Start Sketch',
         showTitle: true,
         hotkey: 'S',
         description: 'Start drawing a 2D sketch',