Fix Toolbar flash bug (during sketchMode animation (#5564)

* fix toolbar flash during animation * add regression test * fmt * test tweak * fmt
KittyCAD · Feb 28, 2025 · b1b00a9 · b1b00a9
1 parent a0924bc
commit b1b00a9
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 56 deletions.
diff --git a/e2e/playwright/can-create-sketches-on-all-planes-and-their-back-sides.spec.ts b/e2e/playwright/can-create-sketches-on-all-planes-and-their-back-sides.spec.ts
@@ -3,6 +3,7 @@ import { HomePageFixture } from './fixtures/homePageFixture'
 import { getUtils } from './test-utils'
 import { EngineCommand } from 'lang/std/artifactGraph'
 import { uuidv4 } from 'lib/utils'
+import { SceneFixture } from './fixtures/sceneFixture'
 
 test.describe(
   'Can create sketches on all planes and their back sides',
@@ -11,16 +12,17 @@ test.describe(
     const sketchOnPlaneAndBackSideTest = async (
       page: Page,
       homePage: HomePageFixture,
+      scene: SceneFixture,
       plane: string,
       clickCoords: { x: number; y: number }
     ) => {
       const u = await getUtils(page)
-      const PUR = 400 / 37.5 //pixeltoUnitRatio
       await page.setBodyDimensions({ width: 1200, height: 500 })
 
       await homePage.goToModelingScene()
-      // FIXME: Cannot use scene.waitForExecutionDone() since there is no KCL code
-      await page.waitForTimeout(10000)
+      const XYPlanRed: [number, number, number] = [98, 50, 51]
+      await scene.expectPixelColor(XYPlanRed, { x: 700, y: 300 }, 15)
+
       await u.openDebugPanel()
 
       const coord =
@@ -43,7 +45,7 @@ test.describe(
         },
       }
 
-      const code = `sketch001 = startSketchOn('${plane}')profile001 = startProfileAt([0.9, -1.22], sketch001)`
+      const code = `sketch001 = startSketchOn('${plane}')profile001 = startProfileAt([0.91, -1.22], sketch001)`
 
       await u.openDebugPanel()
 
@@ -56,17 +58,14 @@ test.describe(
 
       await u.closeDebugPanel()
       await page.mouse.click(clickCoords.x, clickCoords.y)
-      await page.waitForTimeout(300) // wait for animation
+      await page.waitForTimeout(600) // wait for animation
 
       await expect(
         page.getByRole('button', { name: 'line Line', exact: true })
       ).toBeVisible()
 
-      // draw a line
-      const startXPx = 600
-
       await u.closeDebugPanel()
-      await page.mouse.click(startXPx + PUR * 10, 500 - PUR * 10)
+      await page.mouse.click(707, 393)
 
       await expect(page.locator('.cm-content')).toHaveText(code)
 
@@ -81,49 +80,50 @@ test.describe(
       await u.clearCommandLogs()
       await u.removeCurrentCode()
     }
-    test('XY', async ({ page, homePage }) => {
-      await sketchOnPlaneAndBackSideTest(
-        page,
-        homePage,
-        'XY',
-        { x: 600, y: 388 } // red plane
-        // { x: 600, y: 400 }, // red plane // clicks grid helper and that causes problems, should fix so that these coords work too.
-      )
-    })
-
-    test('YZ', async ({ page, homePage }) => {
-      await sketchOnPlaneAndBackSideTest(page, homePage, 'YZ', {
-        x: 700,
-        y: 250,
-      }) // green plane
-    })
-
-    test('XZ', async ({ page, homePage }) => {
-      await sketchOnPlaneAndBackSideTest(page, homePage, '-XZ', {
-        x: 700,
-        y: 80,
-      }) // blue plane
-    })
-
-    test('-XY', async ({ page, homePage }) => {
-      await sketchOnPlaneAndBackSideTest(page, homePage, '-XY', {
-        x: 600,
-        y: 118,
-      }) // back of red plane
-    })
-
-    test('-YZ', async ({ page, homePage }) => {
-      await sketchOnPlaneAndBackSideTest(page, homePage, '-YZ', {
-        x: 700,
-        y: 219,
-      }) // back of green plan
-    })
-
-    test('-XZ', async ({ page, homePage }) => {
-      await sketchOnPlaneAndBackSideTest(page, homePage, 'XZ', {
-        x: 700,
-        y: 427,
-      }) // back of blue plane
-    })
+
+    const planeConfigs = [
+      {
+        plane: 'XY',
+        coords: { x: 600, y: 388 },
+        description: 'red plane',
+      },
+      {
+        plane: 'YZ',
+        coords: { x: 700, y: 250 },
+        description: 'green plane',
+      },
+      {
+        plane: 'XZ',
+        coords: { x: 684, y: 427 },
+        description: 'blue plane',
+      },
+      {
+        plane: '-XY',
+        coords: { x: 600, y: 118 },
+        description: 'back of red plane',
+      },
+      {
+        plane: '-YZ',
+        coords: { x: 700, y: 219 },
+        description: 'back of green plane',
+      },
+      {
+        plane: '-XZ',
+        coords: { x: 700, y: 80 },
+        description: 'back of blue plane',
+      },
+    ]
+
+    for (const config of planeConfigs) {
+      test(config.plane, async ({ page, homePage, scene }) => {
+        await sketchOnPlaneAndBackSideTest(
+          page,
+          homePage,
+          scene,
+          config.plane,
+          config.coords
+        )
+      })
+    }
   }
 )
diff --git a/e2e/playwright/regression-tests.spec.ts b/e2e/playwright/regression-tests.spec.ts
@@ -629,6 +629,39 @@ extrude001 = extrude(sketch001, length = 50)
       )
     })
   })
+
+  test(`Toolbar doesn't show modeling tools during sketch plane selection animation`, async ({
+    page,
+    homePage,
+    toolbar,
+  }) => {
+    await test.step('Load an empty file', async () => {
+      await page.addInitScript(async () => {
+        localStorage.setItem('persistCode', '')
+      })
+      await page.setBodyDimensions({ width: 1200, height: 500 })
+      await homePage.goToModelingScene()
+    })
+
+    const toolBarMode = () =>
+      page.locator('[data-currentMode]').getAttribute('data-currentMode')
+
+    await test.step('Start sketch and select a plane', async () => {
+      await expect.poll(toolBarMode).toEqual('modeling')
+      // Click the start sketch button
+      await toolbar.startSketchPlaneSelection()
+
+      // Click on a default plane at position [700, 200]
+      await page.mouse.click(700, 200)
+
+      // Check that the modeling toolbar doesn't appear during the animation
+      // The animation typically takes around 500ms, so we'll check for a second
+      await expect.poll(toolBarMode, { timeout: 1000 }).not.toEqual('modeling')
+
+      // After animation completes, we should see the sketching toolbar
+      await expect.poll(toolBarMode).toEqual('sketching')
+    })
+  })
 })
 
 async function clickExportButton(page: Page) {

diff --git a/src/Toolbar.tsx b/src/Toolbar.tsx
@@ -167,7 +167,10 @@ export function Toolbar({
   }, [currentMode, disableAllButtons, configCallbackProps])
 
   return (
-    <menu className="max-w-full whitespace-nowrap rounded-b px-2 py-1 bg-chalkboard-10 dark:bg-chalkboard-90 relative border border-chalkboard-30 dark:border-chalkboard-80 border-t-0 shadow-sm">
+    <menu
+      data-currentMode={currentMode}
+      className="max-w-full whitespace-nowrap rounded-b px-2 py-1 bg-chalkboard-10 dark:bg-chalkboard-90 relative border border-chalkboard-30 dark:border-chalkboard-80 border-t-0 shadow-sm"
+    >
       <ul
         {...props}
         ref={toolbarButtonsRef}

diff --git a/src/lib/toolbar.ts b/src/lib/toolbar.ts
@@ -56,7 +56,12 @@ export type ToolbarItemResolved = Omit<
 export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
   modeling: {
     check: (state) =>
-      !(state.matches('Sketch') || state.matches('Sketch no face')),
+      !(
+        state.matches('Sketch') ||
+        state.matches('Sketch no face') ||
+        state.matches('animating to existing sketch') ||
+        state.matches('animating to plane')
+      ),
     items: [
       {
         id: 'sketch',
@@ -330,7 +335,10 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
   },
   sketching: {
     check: (state) =>
-      state.matches('Sketch') || state.matches('Sketch no face'),
+      state.matches('Sketch') ||
+      state.matches('Sketch no face') ||
+      state.matches('animating to existing sketch') ||
+      state.matches('animating to plane'),
     items: [
       {
         id: 'sketch-exit',