Skip to content

Fix async race in calculated expression hook #7037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions src/lib/useCalculateKclExpression.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { renderHook, act } from '@testing-library/react'
import { vi } from 'vitest'

import { useCalculateKclExpression } from '@src/lib/useCalculateKclExpression'
import { getCalculatedKclExpressionValue } from '@src/lib/kclHelpers'

vi.mock('@src/lib/kclHelpers', () => {
return {
getCalculatedKclExpressionValue: vi.fn(),
}
})

vi.mock('@src/lang/KclProvider', () => {
return {
useKclContext: () => ({ code: '', variables: {} }),
}
})

vi.mock('@src/hooks/useModelingContext', () => {
return {
useModelingContext: () => ({ context: { selectionRanges: { graphSelections: [] } } }),
}
})

const mockedGetValue = getCalculatedKclExpressionValue as unknown as ReturnType<typeof vi.fn>

describe('useCalculateKclExpression', () => {
it('ignores outdated asynchronous results', async () => {
let resolveFirst: (v: any) => void
let resolveSecond: (v: any) => void

mockedGetValue
.mockImplementationOnce(
() =>
new Promise((res) => {
resolveFirst = res
})
)
.mockImplementationOnce(
() =>
new Promise((res) => {
resolveSecond = res
})
)

const { result, rerender } = renderHook(
({ value }) => useCalculateKclExpression({ value }),
{ initialProps: { value: '1+1' } }
)

// Trigger a new calculation before the first one resolves
rerender({ value: '2+2' })

await act(async () => {
resolveSecond!({ astNode: {}, valueAsString: '4' })
await Promise.resolve()
})

expect(result.current.calcResult).toBe('4')

Check failure on line 59 in src/lib/useCalculateKclExpression.test.tsx

View workflow job for this annotation

GitHub Actions / npm-test-unit

src/lib/useCalculateKclExpression.test.tsx > useCalculateKclExpression > ignores outdated asynchronous results

AssertionError: expected 'NAN' to be '4' // Object.is equality Expected: "4" Received: "NAN" ❯ src/lib/useCalculateKclExpression.test.tsx:59:39

Check failure on line 59 in src/lib/useCalculateKclExpression.test.tsx

View workflow job for this annotation

GitHub Actions / npm-test-unit

src/lib/useCalculateKclExpression.test.tsx > useCalculateKclExpression > ignores outdated asynchronous results

AssertionError: expected 'NAN' to be '4' // Object.is equality Expected: "4" Received: "NAN" ❯ src/lib/useCalculateKclExpression.test.tsx:59:39

Check failure on line 59 in src/lib/useCalculateKclExpression.test.tsx

View workflow job for this annotation

GitHub Actions / npm-test-unit

src/lib/useCalculateKclExpression.test.tsx > useCalculateKclExpression > ignores outdated asynchronous results

AssertionError: expected 'NAN' to be '4' // Object.is equality Expected: "4" Received: "NAN" ❯ src/lib/useCalculateKclExpression.test.tsx:59:39

Check failure on line 59 in src/lib/useCalculateKclExpression.test.tsx

View workflow job for this annotation

GitHub Actions / npm-test-unit

src/lib/useCalculateKclExpression.test.tsx > useCalculateKclExpression > ignores outdated asynchronous results

AssertionError: expected 'NAN' to be '4' // Object.is equality Expected: "4" Received: "NAN" ❯ src/lib/useCalculateKclExpression.test.tsx:59:39

Check failure on line 59 in src/lib/useCalculateKclExpression.test.tsx

View workflow job for this annotation

GitHub Actions / npm-test-unit

src/lib/useCalculateKclExpression.test.tsx > useCalculateKclExpression > ignores outdated asynchronous results

AssertionError: expected 'NAN' to be '4' // Object.is equality Expected: "4" Received: "NAN" ❯ src/lib/useCalculateKclExpression.test.tsx:59:39

Check failure on line 59 in src/lib/useCalculateKclExpression.test.tsx

View workflow job for this annotation

GitHub Actions / npm-test-unit

src/lib/useCalculateKclExpression.test.tsx > useCalculateKclExpression > ignores outdated asynchronous results

AssertionError: expected 'NAN' to be '4' // Object.is equality Expected: "4" Received: "NAN" ❯ src/lib/useCalculateKclExpression.test.tsx:59:39

await act(async () => {
resolveFirst!({ astNode: {}, valueAsString: '2' })
await Promise.resolve()
})

expect(result.current.calcResult).toBe('4')
})
})
8 changes: 8 additions & 0 deletions src/lib/useCalculateKclExpression.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,11 @@ export function useCalculateKclExpression({
}, [kclManager.ast, kclManager.variables, endingSourceRange])

useEffect(() => {
let isCurrent = true

const execAstAndSetResult = async () => {
const result = await getCalculatedKclExpressionValue(value)
if (!isCurrent) return
setIsExecuting(false)
if (result instanceof Error || 'errors' in result || !result.astNode) {
setCalcResult('NAN')
Expand All @@ -131,10 +134,15 @@ export function useCalculateKclExpression({
if (!value) return
setIsExecuting(true)
execAstAndSetResult().catch(() => {
if (!isCurrent) return
setCalcResult('NAN')
setIsExecuting(false)
setValueNode(null)
})

return () => {
isCurrent = false
}
}, [value, availableVarInfo, code, kclManager.variables])

return {
Expand Down
Loading