From 667ce038e7726bee16e584b3016cee9e8bd9c551 Mon Sep 17 00:00:00 2001 From: Adam Obuchowicz Date: Tue, 17 Sep 2024 09:12:59 +0200 Subject: [PATCH] Fix node removing bug, caused by spurious watchEffect effect. (#11099) Fixes [#11062](https://github.com/enso-org/enso/issues/11062) Modified watchEffect to be consistent with Vue behavior + tests. Before the scheduled effects were run even if stopped. Why it fixes nodes removal? See [this comment](https://github.com/enso-org/enso/issues/11062#issuecomment-2352975474) --- app/gui2/e2e/emptyGraph.spec.ts | 23 -------- app/gui2/e2e/removingNodes.spec.ts | 54 +++++++++++++++++++ app/gui2/e2e/selectingNodes.spec.ts | 20 ------- .../src/util/__tests__/reactivity.test.ts | 36 ++++++++++--- app/gui2/src/util/reactivity.ts | 11 ++-- 5 files changed, 92 insertions(+), 52 deletions(-) delete mode 100644 app/gui2/e2e/emptyGraph.spec.ts create mode 100644 app/gui2/e2e/removingNodes.spec.ts diff --git a/app/gui2/e2e/emptyGraph.spec.ts b/app/gui2/e2e/emptyGraph.spec.ts deleted file mode 100644 index 7a92a44b840e..000000000000 --- a/app/gui2/e2e/emptyGraph.spec.ts +++ /dev/null @@ -1,23 +0,0 @@ -import { test } from '@playwright/test' -import * as actions from './actions' -import { expect } from './customExpect' -import { CONTROL_KEY, DELETE_KEY } from './keyboard' -import * as locate from './locate' - -test('graph can be empty', async ({ page }) => { - await actions.goToGraph(page) - await expect(locate.graphEditor(page)).toExist() - await expect(locate.graphNode(page)).toExist() - - await locate.graphEditor(page).press(`${CONTROL_KEY}+A`) - await locate.graphEditor(page).press(`${DELETE_KEY}`) - - await expect(locate.graphNode(page)).toHaveCount(0) - - await locate.addNewNodeButton(page).click() - await expect(locate.componentBrowserInput(page)).toBeVisible() - await page.keyboard.insertText('foo') - await page.keyboard.press(`${CONTROL_KEY}+Enter`) - await expect(locate.graphNode(page)).toHaveCount(1) - await expect(locate.graphNode(page).locator('.WidgetToken')).toHaveText(['foo']) -}) diff --git a/app/gui2/e2e/removingNodes.spec.ts b/app/gui2/e2e/removingNodes.spec.ts new file mode 100644 index 000000000000..7ba843d70772 --- /dev/null +++ b/app/gui2/e2e/removingNodes.spec.ts @@ -0,0 +1,54 @@ +import { test } from '@playwright/test' +import * as actions from './actions' +import { expect } from './customExpect' +import { CONTROL_KEY, DELETE_KEY } from './keyboard' +import * as locate from './locate' + +test('Deleting selected node with backspace key', async ({ page }) => { + await actions.goToGraph(page) + + const nodesCount = await locate.graphNode(page).count() + const deletedNode = locate.graphNodeByBinding(page, 'final') + await deletedNode.click() + await page.keyboard.press('Backspace') + await expect(locate.graphNode(page)).toHaveCount(nodesCount - 1) +}) + +test('Deleting selected node with delete key', async ({ page }) => { + await actions.goToGraph(page) + + const nodesCount = await locate.graphNode(page).count() + const deletedNode = locate.graphNodeByBinding(page, 'final') + await deletedNode.click() + await page.keyboard.press('Delete') + await expect(locate.graphNode(page)).toHaveCount(nodesCount - 1) +}) + +test('Graph can be empty', async ({ page }) => { + await actions.goToGraph(page) + + await locate.graphEditor(page).press(`${CONTROL_KEY}+A`) + await locate.graphEditor(page).press(`${DELETE_KEY}`) + + await expect(locate.graphNode(page)).toHaveCount(0) + + await locate.addNewNodeButton(page).click() + await expect(locate.componentBrowserInput(page)).toBeVisible() + await page.keyboard.insertText('foo') + await page.keyboard.press(`${CONTROL_KEY}+Enter`) + await expect(locate.graphNode(page)).toHaveCount(1) + await expect(locate.graphNode(page).locator('.WidgetToken')).toHaveText(['foo']) +}) + +test('Removing connected nodes', async ({ page }) => { + await actions.goToGraph(page) + const nodesCount = await locate.graphNode(page).count() + await page.keyboard.down('Shift') + await locate.graphNodeByBinding(page, 'five').click() + await expect(locate.selectedNodes(page)).toHaveCount(1) + await locate.graphNodeByBinding(page, 'sum').click() + await expect(locate.selectedNodes(page)).toHaveCount(2) + await page.keyboard.up('Shift') + await page.keyboard.press('Delete') + await expect(locate.graphNode(page)).toHaveCount(nodesCount - 2) +}) diff --git a/app/gui2/e2e/selectingNodes.spec.ts b/app/gui2/e2e/selectingNodes.spec.ts index 2bb8569db6e7..e4833bdc8693 100644 --- a/app/gui2/e2e/selectingNodes.spec.ts +++ b/app/gui2/e2e/selectingNodes.spec.ts @@ -68,26 +68,6 @@ test('Selecting nodes by area drag', async ({ page }) => { await expect(node2).toBeSelected() }) -test('Deleting selected node with backspace key', async ({ page }) => { - await actions.goToGraph(page) - - const nodesCount = await locate.graphNode(page).count() - const deletedNode = locate.graphNodeByBinding(page, 'final') - await deletedNode.click() - await page.keyboard.press('Backspace') - await expect(locate.graphNode(page)).toHaveCount(nodesCount - 1) -}) - -test('Deleting selected node with delete key', async ({ page }) => { - await actions.goToGraph(page) - - const nodesCount = await locate.graphNode(page).count() - const deletedNode = locate.graphNodeByBinding(page, 'final') - await deletedNode.click() - await page.keyboard.press('Delete') - await expect(locate.graphNode(page)).toHaveCount(nodesCount - 1) -}) - test('Moving selected nodes', async ({ page }) => { await actions.goToGraph(page) const movedNode = locate.graphNodeByBinding(page, 'final') diff --git a/app/gui2/src/util/__tests__/reactivity.test.ts b/app/gui2/src/util/__tests__/reactivity.test.ts index 7f59ae51f6fd..7a3024ee1e69 100644 --- a/app/gui2/src/util/__tests__/reactivity.test.ts +++ b/app/gui2/src/util/__tests__/reactivity.test.ts @@ -1,7 +1,7 @@ -import { LazySyncEffectSet, syncSet } from '@/util/reactivity' +import { LazySyncEffectSet, syncSet, useWatchContext } from '@/util/reactivity' import { fc, test } from '@fast-check/vitest' -import { expect, vi } from 'vitest' -import { nextTick, reactive, ref } from 'vue' +import { describe, expect, vi } from 'vitest' +import { nextTick, reactive, ref, watchEffect, WatchStopHandle } from 'vue' test('LazySyncEffectSet', async () => { const lazySet = new LazySyncEffectSet() @@ -75,9 +75,6 @@ test('LazySyncEffectSet', async () => { key2.value = 104 lazySet.lazyEffect((onCleanup) => { const currentValue = key1.value - console.log('currentValue', currentValue) - console.log('lazilyUpdatedMap', lazilyUpdatedMap) - lazilyUpdatedMap.set(currentValue, 'c' + runCount++) onCleanup(() => lazilyUpdatedMap.delete(currentValue)) }) @@ -154,3 +151,30 @@ test.prop({ syncSet(target, newState) expect([...target].sort()).toEqual([...newState].sort()) }) + +describe('Stopping WatchEffect prevents already scheduled effects:', () => { + async function testProper(watchEffect: (f: () => void) => WatchStopHandle) { + const dep = ref(false) + const f = vi.fn() + const stop = watchEffect(() => { + if (dep.value) { + f() + } + }) + expect(f).not.toHaveBeenCalled() + dep.value = true + stop() + await nextTick() + expect(f).not.toHaveBeenCalled() + } + + test('from useWatchContext', async () => { + const ctx = useWatchContext() + await testProper(ctx.watchEffect) + }) + + // We check if we're consistent with Vue API. + test('original from Vue', async () => { + await testProper(watchEffect) + }) +}) diff --git a/app/gui2/src/util/reactivity.ts b/app/gui2/src/util/reactivity.ts index 7ea2bcd7b8d3..f970be642511 100644 --- a/app/gui2/src/util/reactivity.ts +++ b/app/gui2/src/util/reactivity.ts @@ -152,8 +152,10 @@ export function useWatchContext(): { watchEffect: (f: () => void) => WatchStopHa watch(jobs, () => { while (jobs.length > 0) { const job = jobs.pop()! - queued.delete(job) - job() + // Do not run scheduled job if it's stopped. It's consistent with vue's "watchEffect" (checked in tests) + if (queued.delete(job)) { + job() + } } }) function watchEffect(f: () => void) { @@ -166,7 +168,10 @@ export function useWatchContext(): { watchEffect: (f: () => void) => WatchStopHa }, allowRecurse: true, }) - return runner.effect.stop.bind(runner.effect) + return () => { + runner.effect.stop() + queued.delete(runner) + } } return { watchEffect } }