Move change calculation into separate reducer

.changeset/silent-feet-breathe.md

@@ -0,0 +1,5 @@
+---
+'@boostv/process-optimizer-frontend-core': patch
+---
+
+Fix a 'race condition' in reducer where changes were not detected when a datapoint changes from invalid to valid

packages/core/src/context/experiment/calculate-change-reducer.test.ts

@@ -0,0 +1,131 @@
+import { describe, expect, it } from 'vitest'
+import { ExperimentType } from 'common'
+import { calculateChangeReducer } from './calculate-change-reducer'
+import { createFetchExperimentResultRequest } from './api'
+import { emptyExperiment } from './store'
+import md5 from 'md5'
+
+describe('calculateChangeReducer', () => {
+  it('should set changedSinceLastEvaluation to false when hash matches', () => {
+    const experiment: ExperimentType = {
+      ...emptyExperiment,
+      id: 'test-id',
+    }
+    const hash = md5(
+      JSON.stringify(createFetchExperimentResultRequest(experiment))
+    )
+    const experimentWithHash: ExperimentType = {
+      ...experiment,
+      lastEvaluationHash: hash,
+    }
+
+    const result = calculateChangeReducer(experimentWithHash)
+
+    expect(result.changedSinceLastEvaluation).toBe(false)
+  })
+
+  it('should set changedSinceLastEvaluation to true when hash does not match', () => {
+    const experiment: ExperimentType = {
+      ...emptyExperiment,
+      id: 'test-id',
+      lastEvaluationHash: 'outdated-hash',
+    }
+
+    const result = calculateChangeReducer(experiment)
+
+    expect(result.changedSinceLastEvaluation).toBe(true)
+  })
+
+  it('should set changedSinceLastEvaluation to true when lastEvaluationHash is empty', () => {
+    const experiment: ExperimentType = {
+      ...emptyExperiment,
+      id: 'test-id',
+      lastEvaluationHash: '',
+    }
+
+    const result = calculateChangeReducer(experiment)
+
+    expect(result.changedSinceLastEvaluation).toBe(true)
+  })
+
+  it('should detect change when optimizer config changes', () => {
+    const baseExperiment: ExperimentType = {
+      ...emptyExperiment,
+      id: 'test-id',
+    }
+    const hash = md5(
+      JSON.stringify(createFetchExperimentResultRequest(baseExperiment))
+    )
+    const experimentWithHash: ExperimentType = {
+      ...baseExperiment,
+      lastEvaluationHash: hash,
+    }
+
+    expect(
+      calculateChangeReducer(experimentWithHash).changedSinceLastEvaluation
+    ).toBe(false)
+
+    const modifiedExperiment: ExperimentType = {
+      ...experimentWithHash,
+      optimizerConfig: {
+        ...experimentWithHash.optimizerConfig,
+        initialPoints: 10,
+      },
+    }
+
+    const result = calculateChangeReducer(modifiedExperiment)
+
+    expect(result.changedSinceLastEvaluation).toBe(true)
+  })
+
+  it('should detect change when variables are added', () => {
+    const baseExperiment: ExperimentType = {
+      ...emptyExperiment,
+      id: 'test-id',
+    }
+    const hash = md5(
+      JSON.stringify(createFetchExperimentResultRequest(baseExperiment))
+    )
+    const experimentWithHash: ExperimentType = {
+      ...baseExperiment,
+      lastEvaluationHash: hash,
+    }
+
+    expect(
+      calculateChangeReducer(experimentWithHash).changedSinceLastEvaluation
+    ).toBe(false)
+
+    const modifiedExperiment: ExperimentType = {
+      ...experimentWithHash,
+      valueVariables: [
+        {
+          type: 'continuous',
+          name: 'TestVar',
+          description: 'Test variable',
+          min: 0,
+          max: 100,
+          enabled: true,
+        },
+      ],
+    }
+
+    const result = calculateChangeReducer(modifiedExperiment)
+
+    expect(result.changedSinceLastEvaluation).toBe(true)
+  })
+
+  it('should not mutate the original experiment object', () => {
+    const experiment: ExperimentType = {
+      ...emptyExperiment,
+      id: 'test-id',
+      changedSinceLastEvaluation: false,
+      lastEvaluationHash: 'some-hash',
+    }
+
+    const result = calculateChangeReducer(experiment)
+
+    expect(result).not.toBe(experiment)
+    expect(experiment.changedSinceLastEvaluation).toBe(false)
+    expect(result.changedSinceLastEvaluation).toBe(true)
+  })
+})

packages/core/src/context/experiment/calculate-change-reducer.ts

@@ -0,0 +1,10 @@
+import { ExperimentType } from 'common'
+import { produce } from 'immer'
+import md5 from 'md5'
+import { createFetchExperimentResultRequest } from './api'
+
+export const calculateChangeReducer = produce((state: ExperimentType): void => {
+  state.changedSinceLastEvaluation =
+    state.lastEvaluationHash !==
+    md5(JSON.stringify(createFetchExperimentResultRequest(state)))
+})

packages/core/src/context/experiment/experiment-reducers.ts

@@ -458,9 +458,6 @@ export const experimentReducer = produce(
       default:
         assertUnreachable(action)
     }
-    state.changedSinceLastEvaluation =
-      state.lastEvaluationHash !==
-      md5(JSON.stringify(createFetchExperimentResultRequest(state)))
     state.info.version = state.info.version + 1
   }
 )

packages/core/src/context/experiment/reducers.test.ts

@@ -10,7 +10,11 @@ import {
   scoreNames,
   ValueVariableType,
 } from '@core/common/types'
-import { emptyExperiment, State } from '@core/context/experiment'
+import {
+  createFetchExperimentResultRequest,
+  emptyExperiment,
+  State,
+} from '@core/context/experiment'
 import { versionInfo } from '@core/common'
 import _ from 'lodash'
 import { produce } from 'immer'
@@ -21,6 +25,7 @@ import {
   createValueVariable,
   dummyPayloads,
 } from '@core/context/experiment/test-utils'
+import md5 from 'md5'
 
 describe('experiment reducer', () => {
   const initState: State = {
@@ -116,6 +121,7 @@ describe('experiment reducer', () => {
       const payload: ExperimentType = {
         id: '5678',
         changedSinceLastEvaluation: false,
+        lastEvaluationHash: '5b4247caaa4e9a5a0c519a9017ccc547',
         info: {
           swVersion: versionInfo.version,
           name: 'Not cake',
@@ -169,6 +175,10 @@ describe('experiment reducer', () => {
         type: 'updateExperiment',
         payload,
       }
+      const hash = md5(
+        JSON.stringify(createFetchExperimentResultRequest(payload))
+      )
+      expect(hash).toEqual(payload.lastEvaluationHash)
 
       const actual = rootReducer(initState, action)
       expect(actual).toEqual({

packages/core/src/context/experiment/reducers.ts

@@ -3,6 +3,7 @@ import { State } from './store'
 import { ExperimentAction, experimentReducer } from './experiment-reducers'
 import { validateExperiment, ValidationViolations } from './validation'
 import { validationReducer } from './validation-reducer'
+import { calculateChangeReducer } from './calculate-change-reducer'
 
 export type Action = ExperimentAction
 
@@ -36,7 +37,9 @@ export const rootReducer = (state: State, action: Action) => {
         validateExperiment(experiment)
       return {
         ...state,
-        experiment: validationReducer(experiment, validationViolations),
+        experiment: calculateChangeReducer(
+          validationReducer(experiment, validationViolations)
+        ),
       }
     }
     default: