From 0e6d96bb5e95b46286cf832985dd4a7078a352e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Malfait?= Date: Tue, 23 Jun 2026 10:44:56 +0200 Subject: [PATCH] fix(workflow): stop trigger/action filter Conditions from flashing on edit (#21952) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Adding a condition to a database-event **trigger** (the new Conditions section), the **Filter** action, or the **If/Else** action causes the just-added condition to flash out and back in. ## Root cause `WorkflowEditActionFilterBodyEffect` seeds the builder's local jotai atoms from the persisted `defaultValue` through an effect that **resynced whenever the live atoms differed from `defaultValue`** (the atoms were in the effect deps and the equality check compared atoms vs `defaultValue`). A local edit writes the atoms **synchronously**, then persists through an **async** mutation — and for an *active* workflow that mutation first creates a draft version over the network. During that window the atoms are ahead of the still-stale `defaultValue`, so the effect treated it as "out of sync" and overwrote the edit back to the stale value, then wrote it again once the save landed. That round-trip is the flash. The resync existed for a real reason: the atoms are module-cached per `instanceId` and persist across mounts, and the trigger shares a single **constant** `instanceId` (`'trigger'`), so a previous trigger's filters must be overwritten when a different one is opened. (This is also why the `?? { stepFilterGroups: [], stepFilters: [] }` fallback was added in #21868 — to reset builder state deterministically between trigger edits.) So a naive "init-once" fix would reintroduce that stale-state leak. ## Fix Resync from `defaultValue` **only when `defaultValue` itself changes**, tracked via the last-synced value in `useState` (not the live atoms). This: - never clobbers an in-flight local edit → no flash; - still re-seeds when switching the trigger/action being edited → no stale-state leak; - preserves reflecting genuine external `defaultValue` changes. The `hasInitialized*` flags are no longer needed and are removed (along with the now-unused `stepId` prop on the effect). ## Tests Adds a regression test covering: seeding from `defaultValue` on mount, the **no-clobber-while-stale** invariant (the flash), and resync on a genuine `defaultValue` change. Verified the no-clobber test **fails** against the old "resync against live atoms" behavior and passes with the fix. ## Verification - `nx typecheck twenty-front` ✅ - `nx lint:diff-with-main twenty-front` ✅ (0 warnings / 0 errors) - New unit test: 3 passing ✅ ## Known residual / follow-up On an *active* workflow, the first edit creates a draft version over the network; making a second edit before that round-trip completes leaves a narrow window where the optimistic echo of the first value could momentarily win. Far narrower than the current flash-on-every-edit. Eliminating it entirely (and resolving the still-open HIGH-severity "constant `instanceId`" review flag from #21868) would mean giving the trigger a unique `instanceId` per workflow version + a React `key` to reset on remount — proposed as a separate, scoped follow-up. https://claude.ai/code/session_01XnjtzFepMJX2VFwnQVcQbV --- _Generated by [Claude Code](https://claude.ai/code/session_01XnjtzFepMJX2VFwnQVcQbV)_ Review in cubic --- .../WorkflowEditActionFilterBodyEffect.tsx | 89 +++------- .../components/WorkflowStepFilterBuilder.tsx | 5 +- ...orkflowEditActionFilterBodyEffect.test.tsx | 160 ++++++++++++++++++ .../filters/hooks/useAddRootStepFilter.ts | 23 +-- ...entStepFilterGroupsComponentFamilyState.ts | 9 - ...dCurrentStepFiltersComponentFamilyState.ts | 9 - .../components/WorkflowEditActionIfElse.tsx | 1 - 7 files changed, 184 insertions(+), 112 deletions(-) create mode 100644 packages/twenty-front/src/modules/workflow/workflow-steps/filters/components/__tests__/WorkflowEditActionFilterBodyEffect.test.tsx delete mode 100644 packages/twenty-front/src/modules/workflow/workflow-steps/filters/states/hasInitializedCurrentStepFilterGroupsComponentFamilyState.ts delete mode 100644 packages/twenty-front/src/modules/workflow/workflow-steps/filters/states/hasInitializedCurrentStepFiltersComponentFamilyState.ts diff --git a/packages/twenty-front/src/modules/workflow/workflow-steps/filters/components/WorkflowEditActionFilterBodyEffect.tsx b/packages/twenty-front/src/modules/workflow/workflow-steps/filters/components/WorkflowEditActionFilterBodyEffect.tsx index 80c6657808..861be00f60 100644 --- a/packages/twenty-front/src/modules/workflow/workflow-steps/filters/components/WorkflowEditActionFilterBodyEffect.tsx +++ b/packages/twenty-front/src/modules/workflow/workflow-steps/filters/components/WorkflowEditActionFilterBodyEffect.tsx @@ -1,12 +1,8 @@ -import { useAtomComponentFamilyState } from '@/ui/utilities/state/jotai/hooks/useAtomComponentFamilyState'; -import { useAtomComponentStateValue } from '@/ui/utilities/state/jotai/hooks/useAtomComponentStateValue'; import { useSetAtomComponentState } from '@/ui/utilities/state/jotai/hooks/useSetAtomComponentState'; import { currentStepFilterGroupsComponentState } from '@/workflow/workflow-steps/filters/states/currentStepFilterGroupsComponentState'; import { currentStepFiltersComponentState } from '@/workflow/workflow-steps/filters/states/currentStepFiltersComponentState'; -import { hasInitializedCurrentStepFilterGroupsComponentFamilyState } from '@/workflow/workflow-steps/filters/states/hasInitializedCurrentStepFilterGroupsComponentFamilyState'; -import { hasInitializedCurrentStepFiltersComponentFamilyState } from '@/workflow/workflow-steps/filters/states/hasInitializedCurrentStepFiltersComponentFamilyState'; import { type FilterSettingsWithPotentiallyDeprecatedOperand } from '@/workflow/workflow-steps/filters/types/FilterSettings'; -import { useEffect, useMemo } from 'react'; +import { useEffect, useMemo, useState } from 'react'; import { convertViewFilterOperandToCoreOperand, isDefined, @@ -14,35 +10,10 @@ import { import { isDeeplyEqual } from '~/utils/isDeeplyEqual'; export const WorkflowEditActionFilterBodyEffect = ({ - stepId, defaultValue, }: { - stepId: string; defaultValue?: FilterSettingsWithPotentiallyDeprecatedOperand; }) => { - const [ - hasInitializedCurrentStepFilters, - setHasInitializedCurrentStepFilters, - ] = useAtomComponentFamilyState( - hasInitializedCurrentStepFiltersComponentFamilyState, - { stepId }, - ); - - const [ - hasInitializedCurrentStepFilterGroups, - setHasInitializedCurrentStepFilterGroups, - ] = useAtomComponentFamilyState( - hasInitializedCurrentStepFilterGroupsComponentFamilyState, - { stepId }, - ); - - const currentStepFilters = useAtomComponentStateValue( - currentStepFiltersComponentState, - ); - const currentStepFilterGroups = useAtomComponentStateValue( - currentStepFilterGroupsComponentState, - ); - const setCurrentStepFilters = useSetAtomComponentState( currentStepFiltersComponentState, ); @@ -58,64 +29,48 @@ export const WorkflowEditActionFilterBodyEffect = ({ })); }, [defaultValue?.stepFilters]); + const stepFilterGroups = defaultValue?.stepFilterGroups; + + const [lastSyncedStepFilters, setLastSyncedStepFilters] = + useState(undefined); + + const [lastSyncedStepFilterGroups, setLastSyncedStepFilterGroups] = + useState(undefined); + useEffect(() => { if (!isDefined(stepFiltersConverted)) { return; } - if ( - hasInitializedCurrentStepFilters && - isDeeplyEqual(currentStepFilters, stepFiltersConverted) - ) { + if (isDeeplyEqual(lastSyncedStepFilters, stepFiltersConverted)) { return; } - setCurrentStepFilters(stepFiltersConverted ?? []); - - if (!hasInitializedCurrentStepFilters) { - setHasInitializedCurrentStepFilters(true); - } + setLastSyncedStepFilters(stepFiltersConverted); + setCurrentStepFilters(stepFiltersConverted); }, [ - setCurrentStepFilters, - hasInitializedCurrentStepFilters, - setHasInitializedCurrentStepFilters, stepFiltersConverted, - currentStepFilters, + lastSyncedStepFilters, + setCurrentStepFilters, + setLastSyncedStepFilters, ]); useEffect(() => { - if (!isDefined(defaultValue?.stepFilterGroups)) { + if (!isDefined(stepFilterGroups)) { return; } - if ( - !hasInitializedCurrentStepFilterGroups && - defaultValue.stepFilterGroups.length === 0 - ) { + if (isDeeplyEqual(lastSyncedStepFilterGroups, stepFilterGroups)) { return; } - if ( - hasInitializedCurrentStepFilterGroups && - isDeeplyEqual( - currentStepFilterGroups, - defaultValue.stepFilterGroups ?? [], - ) - ) { - return; - } - - setCurrentStepFilterGroups(defaultValue.stepFilterGroups ?? []); - - if (!hasInitializedCurrentStepFilterGroups) { - setHasInitializedCurrentStepFilterGroups(true); - } + setLastSyncedStepFilterGroups(stepFilterGroups); + setCurrentStepFilterGroups(stepFilterGroups); }, [ + stepFilterGroups, + lastSyncedStepFilterGroups, setCurrentStepFilterGroups, - hasInitializedCurrentStepFilterGroups, - setHasInitializedCurrentStepFilterGroups, - defaultValue?.stepFilterGroups, - currentStepFilterGroups, + setLastSyncedStepFilterGroups, ]); return null; diff --git a/packages/twenty-front/src/modules/workflow/workflow-steps/filters/components/WorkflowStepFilterBuilder.tsx b/packages/twenty-front/src/modules/workflow/workflow-steps/filters/components/WorkflowStepFilterBuilder.tsx index d9f925d66e..4f46a39f3e 100644 --- a/packages/twenty-front/src/modules/workflow/workflow-steps/filters/components/WorkflowStepFilterBuilder.tsx +++ b/packages/twenty-front/src/modules/workflow/workflow-steps/filters/components/WorkflowStepFilterBuilder.tsx @@ -117,10 +117,7 @@ export const WorkflowStepFilterBuilder = ({ > - + ); diff --git a/packages/twenty-front/src/modules/workflow/workflow-steps/filters/components/__tests__/WorkflowEditActionFilterBodyEffect.test.tsx b/packages/twenty-front/src/modules/workflow/workflow-steps/filters/components/__tests__/WorkflowEditActionFilterBodyEffect.test.tsx new file mode 100644 index 0000000000..2c69b147c7 --- /dev/null +++ b/packages/twenty-front/src/modules/workflow/workflow-steps/filters/components/__tests__/WorkflowEditActionFilterBodyEffect.test.tsx @@ -0,0 +1,160 @@ +import { render } from '@testing-library/react'; +import { act } from 'react'; +import { + type StepFilter, + type StepFilterGroup, + StepLogicalOperator, + ViewFilterOperand, +} from 'twenty-shared/types'; +import { convertViewFilterOperandToCoreOperand } from 'twenty-shared/utils'; + +import { useSetAtomComponentState } from '@/ui/utilities/state/jotai/hooks/useSetAtomComponentState'; +import { useAtomComponentStateValue } from '@/ui/utilities/state/jotai/hooks/useAtomComponentStateValue'; +import { WorkflowEditActionFilterBodyEffect } from '@/workflow/workflow-steps/filters/components/WorkflowEditActionFilterBodyEffect'; +import { StepFilterGroupsComponentInstanceContext } from '@/workflow/workflow-steps/filters/states/context/StepFilterGroupsComponentInstanceContext'; +import { StepFiltersComponentInstanceContext } from '@/workflow/workflow-steps/filters/states/context/StepFiltersComponentInstanceContext'; +import { currentStepFilterGroupsComponentState } from '@/workflow/workflow-steps/filters/states/currentStepFilterGroupsComponentState'; +import { currentStepFiltersComponentState } from '@/workflow/workflow-steps/filters/states/currentStepFiltersComponentState'; +import { type FilterSettingsWithPotentiallyDeprecatedOperand } from '@/workflow/workflow-steps/filters/types/FilterSettings'; + +const makeStepFilterGroup = (id: string): StepFilterGroup => ({ + id, + logicalOperator: StepLogicalOperator.AND, +}); + +const makeStepFilter = (id: string, stepFilterGroupId: string): StepFilter => ({ + id, + type: 'unknown', + stepOutputKey: '', + operand: ViewFilterOperand.IS, + value: '', + stepFilterGroupId, + positionInStepFilterGroup: 0, +}); + +// `defaultValue` filters are stored with potentially deprecated operands and are +// normalized to core operands when synced into the atoms. +const toExpectedSyncedFilter = (filter: StepFilter) => ({ + ...filter, + operand: convertViewFilterOperandToCoreOperand(filter.operand), +}); + +type Captured = { + filters: StepFilter[]; + filterGroups: StepFilterGroup[]; + setFilters: (value: StepFilter[]) => void; + setFilterGroups: (value: StepFilterGroup[]) => void; +}; + +const renderBodyEffect = ( + instanceId: string, + defaultValue?: FilterSettingsWithPotentiallyDeprecatedOperand, +) => { + const captured: Captured = {} as Captured; + + const ProbeEffect = () => { + captured.filters = useAtomComponentStateValue( + currentStepFiltersComponentState, + ); + captured.filterGroups = useAtomComponentStateValue( + currentStepFilterGroupsComponentState, + ); + captured.setFilters = useSetAtomComponentState( + currentStepFiltersComponentState, + ); + captured.setFilterGroups = useSetAtomComponentState( + currentStepFilterGroupsComponentState, + ); + + return null; + }; + + const Harness = ({ + value, + }: { + value?: FilterSettingsWithPotentiallyDeprecatedOperand; + }) => ( + + + + + + + ); + + const utils = render(); + + return { + captured, + rerender: (value?: FilterSettingsWithPotentiallyDeprecatedOperand) => + utils.rerender(), + }; +}; + +describe('WorkflowEditActionFilterBodyEffect', () => { + it('seeds the local atoms from defaultValue on mount', () => { + const stepFilterGroup = makeStepFilterGroup('group-1'); + const stepFilter = makeStepFilter('filter-1', 'group-1'); + + const { captured } = renderBodyEffect('seed', { + stepFilterGroups: [stepFilterGroup], + stepFilters: [stepFilter], + }); + + expect(captured.filterGroups).toEqual([stepFilterGroup]); + expect(captured.filters).toEqual([toExpectedSyncedFilter(stepFilter)]); + }); + + // Regression test for the "Conditions" flash: when the user adds a condition, + // the local atoms are updated synchronously while the persisted value (passed + // as defaultValue) only catches up after an async save. The effect must not + // overwrite the local edit during that window. + it('does not overwrite a local edit while defaultValue is still stale', () => { + const { captured, rerender } = renderBodyEffect('no-clobber', { + stepFilterGroups: [], + stepFilters: [], + }); + + expect(captured.filters).toEqual([]); + expect(captured.filterGroups).toEqual([]); + + const editedGroup = makeStepFilterGroup('edited-group'); + const editedFilter = makeStepFilter('edited-filter', 'edited-group'); + + act(() => { + captured.setFilterGroups([editedGroup]); + captured.setFilters([editedFilter]); + }); + + expect(captured.filters).toEqual([editedFilter]); + expect(captured.filterGroups).toEqual([editedGroup]); + + // The save has not landed yet, so defaultValue is still the stale empty value. + act(() => { + rerender({ stepFilterGroups: [], stepFilters: [] }); + }); + + expect(captured.filters).toEqual([editedFilter]); + expect(captured.filterGroups).toEqual([editedGroup]); + }); + + it('resyncs the atoms when defaultValue actually changes', () => { + const { captured, rerender } = renderBodyEffect('resync', { + stepFilterGroups: [], + stepFilters: [], + }); + + const nextGroup = makeStepFilterGroup('group-2'); + const nextFilter = makeStepFilter('filter-2', 'group-2'); + + act(() => { + rerender({ + stepFilterGroups: [nextGroup], + stepFilters: [nextFilter], + }); + }); + + expect(captured.filterGroups).toEqual([nextGroup]); + expect(captured.filters).toEqual([toExpectedSyncedFilter(nextFilter)]); + }); +}); diff --git a/packages/twenty-front/src/modules/workflow/workflow-steps/filters/hooks/useAddRootStepFilter.ts b/packages/twenty-front/src/modules/workflow/workflow-steps/filters/hooks/useAddRootStepFilter.ts index ce49b056c4..ef02fe5c3c 100644 --- a/packages/twenty-front/src/modules/workflow/workflow-steps/filters/hooks/useAddRootStepFilter.ts +++ b/packages/twenty-front/src/modules/workflow/workflow-steps/filters/hooks/useAddRootStepFilter.ts @@ -1,10 +1,7 @@ import { useAtomComponentStateCallbackState } from '@/ui/utilities/state/jotai/hooks/useAtomComponentStateCallbackState'; -import { useSetAtomComponentFamilyState } from '@/ui/utilities/state/jotai/hooks/useSetAtomComponentFamilyState'; import { WorkflowStepFilterContext } from '@/workflow/workflow-steps/filters/states/context/WorkflowStepFilterContext'; import { currentStepFilterGroupsComponentState } from '@/workflow/workflow-steps/filters/states/currentStepFilterGroupsComponentState'; import { currentStepFiltersComponentState } from '@/workflow/workflow-steps/filters/states/currentStepFiltersComponentState'; -import { hasInitializedCurrentStepFilterGroupsComponentFamilyState } from '@/workflow/workflow-steps/filters/states/hasInitializedCurrentStepFilterGroupsComponentFamilyState'; -import { hasInitializedCurrentStepFiltersComponentFamilyState } from '@/workflow/workflow-steps/filters/states/hasInitializedCurrentStepFiltersComponentFamilyState'; import { useStore } from 'jotai'; import { useCallback, useContext } from 'react'; import { @@ -16,9 +13,7 @@ import { import { v4 } from 'uuid'; export const useAddRootStepFilter = () => { - const { stepId, onFilterSettingsUpdate } = useContext( - WorkflowStepFilterContext, - ); + const { onFilterSettingsUpdate } = useContext(WorkflowStepFilterContext); const currentStepFilterGroups = useAtomComponentStateCallbackState( currentStepFilterGroupsComponentState, ); @@ -27,17 +22,6 @@ export const useAddRootStepFilter = () => { currentStepFiltersComponentState, ); - const setHasInitializedCurrentStepFilters = useSetAtomComponentFamilyState( - hasInitializedCurrentStepFiltersComponentFamilyState, - { stepId }, - ); - - const setHasInitializedCurrentStepFilterGroups = - useSetAtomComponentFamilyState( - hasInitializedCurrentStepFilterGroupsComponentFamilyState, - { stepId }, - ); - const store = useStore(); const addRootStepFilter = useCallback(() => { @@ -59,9 +43,6 @@ export const useAddRootStepFilter = () => { store.set(currentStepFilterGroups, [newStepFilterGroup]); store.set(currentStepFilters, [newStepFilter]); - setHasInitializedCurrentStepFilters(true); - setHasInitializedCurrentStepFilterGroups(true); - onFilterSettingsUpdate({ stepFilterGroups: [newStepFilterGroup], stepFilters: [newStepFilter], @@ -70,8 +51,6 @@ export const useAddRootStepFilter = () => { onFilterSettingsUpdate, currentStepFilterGroups, currentStepFilters, - setHasInitializedCurrentStepFilters, - setHasInitializedCurrentStepFilterGroups, store, ]); diff --git a/packages/twenty-front/src/modules/workflow/workflow-steps/filters/states/hasInitializedCurrentStepFilterGroupsComponentFamilyState.ts b/packages/twenty-front/src/modules/workflow/workflow-steps/filters/states/hasInitializedCurrentStepFilterGroupsComponentFamilyState.ts deleted file mode 100644 index 5db9dd821e..0000000000 --- a/packages/twenty-front/src/modules/workflow/workflow-steps/filters/states/hasInitializedCurrentStepFilterGroupsComponentFamilyState.ts +++ /dev/null @@ -1,9 +0,0 @@ -import { createAtomComponentFamilyState } from '@/ui/utilities/state/jotai/utils/createAtomComponentFamilyState'; -import { StepFilterGroupsComponentInstanceContext } from '@/workflow/workflow-steps/filters/states/context/StepFilterGroupsComponentInstanceContext'; - -export const hasInitializedCurrentStepFilterGroupsComponentFamilyState = - createAtomComponentFamilyState({ - key: 'hasInitializedCurrentStepFilterGroupsComponentFamilyState', - defaultValue: false, - componentInstanceContext: StepFilterGroupsComponentInstanceContext, - }); diff --git a/packages/twenty-front/src/modules/workflow/workflow-steps/filters/states/hasInitializedCurrentStepFiltersComponentFamilyState.ts b/packages/twenty-front/src/modules/workflow/workflow-steps/filters/states/hasInitializedCurrentStepFiltersComponentFamilyState.ts deleted file mode 100644 index dd1147b1cf..0000000000 --- a/packages/twenty-front/src/modules/workflow/workflow-steps/filters/states/hasInitializedCurrentStepFiltersComponentFamilyState.ts +++ /dev/null @@ -1,9 +0,0 @@ -import { createAtomComponentFamilyState } from '@/ui/utilities/state/jotai/utils/createAtomComponentFamilyState'; -import { StepFiltersComponentInstanceContext } from '@/workflow/workflow-steps/filters/states/context/StepFiltersComponentInstanceContext'; - -export const hasInitializedCurrentStepFiltersComponentFamilyState = - createAtomComponentFamilyState({ - key: 'hasInitializedCurrentStepFiltersComponentFamilyState', - defaultValue: false, - componentInstanceContext: StepFiltersComponentInstanceContext, - }); diff --git a/packages/twenty-front/src/modules/workflow/workflow-steps/workflow-actions/if-else-action/components/WorkflowEditActionIfElse.tsx b/packages/twenty-front/src/modules/workflow/workflow-steps/workflow-actions/if-else-action/components/WorkflowEditActionIfElse.tsx index 8deefe0108..69a8459bf1 100644 --- a/packages/twenty-front/src/modules/workflow/workflow-steps/workflow-actions/if-else-action/components/WorkflowEditActionIfElse.tsx +++ b/packages/twenty-front/src/modules/workflow/workflow-steps/workflow-actions/if-else-action/components/WorkflowEditActionIfElse.tsx @@ -38,7 +38,6 @@ export const WorkflowEditActionIfElse = ({ actionOptions={actionOptions} />