mirror of https://github.com/langgenius/dify.git
fix: leaked set timeout (#33692)
This commit is contained in:
Stephen Zhou
GitHub
parent
5ee0633bac
commit
4254392221
|
|
@ -1,6 +1,6 @@
|
|||
import { noop } from 'es-toolkit/function'
|
||||
import * as React from 'react'
|
||||
import { useState } from 'react'
|
||||
import { useCallback, useState } from 'react'
|
||||
import { Trans, useTranslation } from 'react-i18next'
|
||||
import { useContext } from 'use-context-selector'
|
||||
import Button from '@/app/components/base/button'
|
||||
|
|
@ -36,18 +36,33 @@ const TransferOwnershipModal = ({ onClose, show }: Props) => {
|
|||
const [stepToken, setStepToken] = useState<string>('')
|
||||
const [newOwner, setNewOwner] = useState<string>('')
|
||||
const [isTransfer, setIsTransfer] = useState<boolean>(false)
|
||||
const timerIdRef = React.useRef<number | undefined>(undefined)
|
||||
|
||||
const retimeCountdown = useCallback((timerId?: number) => {
|
||||
if (timerIdRef.current !== undefined)
|
||||
window.clearInterval(timerIdRef.current)
|
||||
|
||||
timerIdRef.current = timerId
|
||||
}, [])
|
||||
|
||||
React.useEffect(() => {
|
||||
if (!show)
|
||||
retimeCountdown()
|
||||
|
||||
return retimeCountdown
|
||||
}, [retimeCountdown, show])
|
||||
|
||||
const startCount = () => {
|
||||
setTime(60)
|
||||
const timer = setInterval(() => {
|
||||
retimeCountdown(window.setInterval(() => {
|
||||
setTime((prev) => {
|
||||
if (prev <= 0) {
|
||||
clearInterval(timer)
|
||||
if (prev <= 1) {
|
||||
retimeCountdown()
|
||||
return 0
|
||||
}
|
||||
return prev - 1
|
||||
})
|
||||
}, 1000)
|
||||
}, 1000))
|
||||
}
|
||||
|
||||
const sendEmail = async () => {
|
||||
|
|
|
|||
|
|
@ -76,16 +76,16 @@ afterAll(() => {
|
|||
|
||||
// Mock portal components for controlled positioning in tests
|
||||
// Use React context to properly scope open state per portal instance (for nested portals)
|
||||
const _PortalOpenContext = React.createContext(false)
|
||||
|
||||
vi.mock('@/app/components/base/portal-to-follow-elem', () => {
|
||||
// Context reference shared across mock components
|
||||
let sharedContext: React.Context<boolean> | null = null
|
||||
|
||||
// Lazily get or create the context
|
||||
const getContext = (): React.Context<boolean> => {
|
||||
if (!sharedContext)
|
||||
sharedContext = React.createContext(false)
|
||||
if (!sharedContext) {
|
||||
const PortalOpenContext = React.createContext(false)
|
||||
sharedContext = PortalOpenContext
|
||||
}
|
||||
return sharedContext
|
||||
}
|
||||
|
||||
|
|
@ -725,6 +725,39 @@ describe('AppPicker', () => {
|
|||
triggerIntersection([{ isIntersecting: true } as IntersectionObserverEntry])
|
||||
expect(onLoadMore).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('should reset loadingRef when the picker closes before the debounce timeout finishes', () => {
|
||||
const onLoadMore = vi.fn()
|
||||
const { rerender } = render(
|
||||
<AppPicker {...defaultProps} isShow={true} hasMore={true} isLoading={false} onLoadMore={onLoadMore} />,
|
||||
)
|
||||
|
||||
triggerIntersection([{ isIntersecting: true } as IntersectionObserverEntry])
|
||||
expect(onLoadMore).toHaveBeenCalledTimes(1)
|
||||
|
||||
rerender(<AppPicker {...defaultProps} isShow={false} hasMore={true} isLoading={false} onLoadMore={onLoadMore} />)
|
||||
rerender(<AppPicker {...defaultProps} isShow={true} hasMore={true} isLoading={false} onLoadMore={onLoadMore} />)
|
||||
|
||||
triggerIntersection([{ isIntersecting: true } as IntersectionObserverEntry])
|
||||
expect(onLoadMore).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('should reset loadingRef when the picker unmounts before the debounce timeout finishes', () => {
|
||||
const onLoadMore = vi.fn()
|
||||
const { unmount } = render(
|
||||
<AppPicker {...defaultProps} isShow={true} hasMore={true} isLoading={false} onLoadMore={onLoadMore} />,
|
||||
)
|
||||
|
||||
triggerIntersection([{ isIntersecting: true } as IntersectionObserverEntry])
|
||||
expect(onLoadMore).toHaveBeenCalledTimes(1)
|
||||
|
||||
unmount()
|
||||
|
||||
render(<AppPicker {...defaultProps} isShow={true} hasMore={true} isLoading={false} onLoadMore={onLoadMore} />)
|
||||
|
||||
triggerIntersection([{ isIntersecting: true } as IntersectionObserverEntry])
|
||||
expect(onLoadMore).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
})
|
||||
|
||||
describe('Memoization', () => {
|
||||
|
|
@ -1539,7 +1572,7 @@ describe('AppSelector', () => {
|
|||
expect(screen.getByTestId('portal-content')).toBeInTheDocument()
|
||||
})
|
||||
|
||||
it('should manage isLoadingMore state during load more', () => {
|
||||
it('should render correctly during load more setup', () => {
|
||||
mockHasNextPage = true
|
||||
mockFetchNextPage.mockImplementation(() => new Promise(resolve => setTimeout(resolve, 100)))
|
||||
|
||||
|
|
@ -1739,7 +1772,7 @@ describe('AppSelector', () => {
|
|||
expect(mockFetchNextPage).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('should set isLoadingMore and reset after delay in handleLoadMore', async () => {
|
||||
it('should avoid duplicate fetches while the picker debounce is active', async () => {
|
||||
mockHasNextPage = true
|
||||
mockIsFetchingNextPage = false
|
||||
mockFetchNextPage.mockResolvedValue(undefined)
|
||||
|
|
@ -1756,34 +1789,15 @@ describe('AppSelector', () => {
|
|||
|
||||
expect(mockFetchNextPage).toHaveBeenCalledTimes(1)
|
||||
|
||||
// Try to trigger again immediately - should be blocked by isLoadingMore
|
||||
// Try to trigger again immediately - should be blocked by AppPicker loadingRef
|
||||
triggerIntersection([{ isIntersecting: true } as IntersectionObserverEntry])
|
||||
|
||||
// Still only one call due to isLoadingMore
|
||||
// Still only one call due to the picker-level debounce
|
||||
expect(mockFetchNextPage).toHaveBeenCalledTimes(1)
|
||||
|
||||
// This verifies the debounce logic is working - multiple calls are blocked
|
||||
expect(screen.getAllByTestId('portal-content').length).toBeGreaterThan(0)
|
||||
})
|
||||
|
||||
it('should not call fetchNextPage when isLoadingMore is true', async () => {
|
||||
mockHasNextPage = true
|
||||
mockIsFetchingNextPage = false
|
||||
mockFetchNextPage.mockImplementation(() => new Promise(resolve => setTimeout(resolve, 1000)))
|
||||
|
||||
renderWithQueryClient(<AppSelector {...defaultProps} />)
|
||||
|
||||
// Open portals
|
||||
fireEvent.click(screen.getAllByTestId('portal-trigger')[0])
|
||||
const triggers = screen.getAllByTestId('portal-trigger')
|
||||
fireEvent.click(triggers[1])
|
||||
|
||||
// Trigger intersection - this starts loading
|
||||
triggerIntersection([{ isIntersecting: true } as IntersectionObserverEntry])
|
||||
|
||||
expect(mockFetchNextPage).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('should skip handleLoadMore when isFetchingNextPage is true', async () => {
|
||||
mockHasNextPage = true
|
||||
mockIsFetchingNextPage = true // This will block the handleLoadMore
|
||||
|
|
@ -1821,89 +1835,7 @@ describe('AppSelector', () => {
|
|||
// fetchNextPage should NOT be called because hasMore is false
|
||||
expect(mockFetchNextPage).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('should return early from handleLoadMore when isLoadingMore is true', async () => {
|
||||
mockHasNextPage = true
|
||||
mockIsFetchingNextPage = false
|
||||
// Make fetchNextPage slow to keep isLoadingMore true
|
||||
mockFetchNextPage.mockImplementation(() => new Promise(resolve => setTimeout(resolve, 5000)))
|
||||
|
||||
renderWithQueryClient(<AppSelector {...defaultProps} />)
|
||||
|
||||
fireEvent.click(screen.getAllByTestId('portal-trigger')[0])
|
||||
const triggers = screen.getAllByTestId('portal-trigger')
|
||||
fireEvent.click(triggers[1])
|
||||
|
||||
// First call starts loading
|
||||
triggerIntersection([{ isIntersecting: true } as IntersectionObserverEntry])
|
||||
expect(mockFetchNextPage).toHaveBeenCalledTimes(1)
|
||||
|
||||
// Second call should return early due to isLoadingMore
|
||||
triggerIntersection([{ isIntersecting: true } as IntersectionObserverEntry])
|
||||
|
||||
// Still only 1 call because isLoadingMore blocks it
|
||||
expect(mockFetchNextPage).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('should reset isLoadingMore via setTimeout after fetchNextPage resolves', async () => {
|
||||
mockHasNextPage = true
|
||||
mockIsFetchingNextPage = false
|
||||
mockFetchNextPage.mockResolvedValue(undefined)
|
||||
|
||||
renderWithQueryClient(<AppSelector {...defaultProps} />)
|
||||
|
||||
fireEvent.click(screen.getAllByTestId('portal-trigger')[0])
|
||||
const triggers = screen.getAllByTestId('portal-trigger')
|
||||
fireEvent.click(triggers[1])
|
||||
|
||||
// Trigger load more
|
||||
triggerIntersection([{ isIntersecting: true } as IntersectionObserverEntry])
|
||||
|
||||
// Wait for fetchNextPage to complete and setTimeout to fire
|
||||
await act(async () => {
|
||||
await Promise.resolve()
|
||||
vi.advanceTimersByTime(350) // Past the 300ms setTimeout
|
||||
})
|
||||
|
||||
// Should be able to load more again
|
||||
triggerIntersection([{ isIntersecting: true } as IntersectionObserverEntry])
|
||||
|
||||
// This might trigger another fetch if loadingRef also reset
|
||||
expect(screen.getAllByTestId('portal-content').length).toBeGreaterThan(0)
|
||||
})
|
||||
|
||||
it('should reset isLoadingMore after fetchNextPage completes with setTimeout', async () => {
|
||||
mockHasNextPage = true
|
||||
mockIsFetchingNextPage = false
|
||||
mockFetchNextPage.mockResolvedValue(undefined)
|
||||
|
||||
renderWithQueryClient(<AppSelector {...defaultProps} />)
|
||||
|
||||
// Open portals
|
||||
fireEvent.click(screen.getAllByTestId('portal-trigger')[0])
|
||||
const triggers = screen.getAllByTestId('portal-trigger')
|
||||
fireEvent.click(triggers[1])
|
||||
|
||||
// Trigger first intersection
|
||||
triggerIntersection([{ isIntersecting: true } as IntersectionObserverEntry])
|
||||
|
||||
expect(mockFetchNextPage).toHaveBeenCalledTimes(1)
|
||||
|
||||
// Advance timer past the 300ms setTimeout in finally block
|
||||
await act(async () => {
|
||||
vi.advanceTimersByTime(400)
|
||||
})
|
||||
|
||||
// Also advance past the loadingRef timeout in AppPicker (500ms)
|
||||
await act(async () => {
|
||||
vi.advanceTimersByTime(200)
|
||||
})
|
||||
|
||||
// Verify component is still rendered correctly
|
||||
expect(screen.getAllByTestId('portal-content').length).toBeGreaterThan(0)
|
||||
})
|
||||
})
|
||||
|
||||
describe('Form Change Handling', () => {
|
||||
it('should handle form change with image file', () => {
|
||||
const onSelect = vi.fn()
|
||||
|
|
@ -2284,7 +2216,7 @@ describe('AppSelector Integration', () => {
|
|||
expect(screen.getByTestId('portal-content')).toBeInTheDocument()
|
||||
})
|
||||
|
||||
it('should set isLoadingMore to false after fetchNextPage completes', async () => {
|
||||
it('should stay stable after fetchNextPage completes', async () => {
|
||||
mockHasNextPage = true
|
||||
mockIsFetchingNextPage = false
|
||||
mockFetchNextPage.mockResolvedValue(undefined)
|
||||
|
|
@ -2293,16 +2225,10 @@ describe('AppSelector Integration', () => {
|
|||
|
||||
fireEvent.click(screen.getAllByTestId('portal-trigger')[0])
|
||||
|
||||
// Advance timers past the 300ms delay
|
||||
await act(async () => {
|
||||
vi.advanceTimersByTime(400)
|
||||
})
|
||||
|
||||
expect(screen.getByTestId('portal-content')).toBeInTheDocument()
|
||||
})
|
||||
|
||||
it('should not call fetchNextPage when conditions prevent it', () => {
|
||||
// isLoadingMore would be true internally
|
||||
mockHasNextPage = false
|
||||
mockIsFetchingNextPage = true
|
||||
|
||||
|
|
|
|||
|
|
@ -51,9 +51,30 @@ const AppPicker: FC<Props> = ({
|
|||
onSearchChange,
|
||||
}) => {
|
||||
const { t } = useTranslation()
|
||||
const observerTarget = useRef<HTMLDivElement>(null)
|
||||
const observerTargetRef = useRef<HTMLDivElement>(null)
|
||||
const observerRef = useRef<IntersectionObserver | null>(null)
|
||||
const loadingRef = useRef(false)
|
||||
const loadingResetTimerIdRef = useRef<number | undefined>(undefined)
|
||||
|
||||
const retimeLoadingReset = useCallback((timerId?: number) => {
|
||||
if (loadingResetTimerIdRef.current !== undefined)
|
||||
globalThis.clearTimeout(loadingResetTimerIdRef.current)
|
||||
|
||||
loadingResetTimerIdRef.current = timerId
|
||||
}, [])
|
||||
|
||||
const resetLoadingState = useCallback(() => {
|
||||
retimeLoadingReset()
|
||||
loadingRef.current = false
|
||||
}, [retimeLoadingReset])
|
||||
|
||||
const disconnectObserver = useCallback(() => {
|
||||
if (!observerRef.current)
|
||||
return
|
||||
|
||||
observerRef.current.disconnect()
|
||||
observerRef.current = null
|
||||
}, [])
|
||||
|
||||
const handleIntersection = useCallback((entries: IntersectionObserverEntry[]) => {
|
||||
const target = entries[0]
|
||||
|
|
@ -62,27 +83,27 @@ const AppPicker: FC<Props> = ({
|
|||
|
||||
loadingRef.current = true
|
||||
onLoadMore()
|
||||
// Reset loading state
|
||||
setTimeout(() => {
|
||||
retimeLoadingReset(window.setTimeout(() => {
|
||||
loadingRef.current = false
|
||||
}, 500)
|
||||
}, [hasMore, isLoading, onLoadMore])
|
||||
retimeLoadingReset()
|
||||
}, 500))
|
||||
}, [hasMore, isLoading, onLoadMore, retimeLoadingReset])
|
||||
|
||||
useEffect(() => {
|
||||
if (!isShow) {
|
||||
if (observerRef.current) {
|
||||
observerRef.current.disconnect()
|
||||
observerRef.current = null
|
||||
}
|
||||
resetLoadingState()
|
||||
disconnectObserver()
|
||||
return
|
||||
}
|
||||
|
||||
let mutationObserver: MutationObserver | null = null
|
||||
|
||||
const setupIntersectionObserver = () => {
|
||||
if (!observerTarget.current)
|
||||
if (!observerTargetRef.current)
|
||||
return
|
||||
|
||||
disconnectObserver()
|
||||
|
||||
// Create new observer
|
||||
observerRef.current = new IntersectionObserver(handleIntersection, {
|
||||
root: null,
|
||||
|
|
@ -90,12 +111,12 @@ const AppPicker: FC<Props> = ({
|
|||
threshold: 0.1,
|
||||
})
|
||||
|
||||
observerRef.current.observe(observerTarget.current)
|
||||
observerRef.current.observe(observerTargetRef.current)
|
||||
}
|
||||
|
||||
// Set up MutationObserver to watch DOM changes
|
||||
mutationObserver = new MutationObserver((_mutations) => {
|
||||
if (observerTarget.current) {
|
||||
if (observerTargetRef.current) {
|
||||
setupIntersectionObserver()
|
||||
mutationObserver?.disconnect()
|
||||
}
|
||||
|
|
@ -108,17 +129,15 @@ const AppPicker: FC<Props> = ({
|
|||
})
|
||||
|
||||
// If element exists, set up IntersectionObserver directly
|
||||
if (observerTarget.current)
|
||||
if (observerTargetRef.current)
|
||||
setupIntersectionObserver()
|
||||
|
||||
return () => {
|
||||
if (observerRef.current) {
|
||||
observerRef.current.disconnect()
|
||||
observerRef.current = null
|
||||
}
|
||||
resetLoadingState()
|
||||
disconnectObserver()
|
||||
mutationObserver?.disconnect()
|
||||
}
|
||||
}, [isShow, handleIntersection])
|
||||
}, [disconnectObserver, handleIntersection, isShow, resetLoadingState])
|
||||
|
||||
const getAppType = (app: App) => {
|
||||
switch (app.mode) {
|
||||
|
|
@ -180,7 +199,7 @@ const AppPicker: FC<Props> = ({
|
|||
background={app.icon_background}
|
||||
imageUrl={app.icon_url}
|
||||
/>
|
||||
<div title={`${app.name} (${app.id})`} className="system-sm-medium grow text-components-input-text-filled">
|
||||
<div title={`${app.name} (${app.id})`} className="grow text-components-input-text-filled system-sm-medium">
|
||||
<span className="mr-1">{app.name}</span>
|
||||
<span className="text-text-tertiary">
|
||||
(
|
||||
|
|
@ -188,10 +207,10 @@ const AppPicker: FC<Props> = ({
|
|||
)
|
||||
</span>
|
||||
</div>
|
||||
<div className="system-2xs-medium-uppercase shrink-0 text-text-tertiary">{getAppType(app)}</div>
|
||||
<div className="shrink-0 text-text-tertiary system-2xs-medium-uppercase">{getAppType(app)}</div>
|
||||
</div>
|
||||
))}
|
||||
<div ref={observerTarget} className="h-4 w-full">
|
||||
<div ref={observerTargetRef} className="h-4 w-full">
|
||||
{isLoading && (
|
||||
<div className="flex justify-center py-2">
|
||||
<div className="text-sm text-gray-500">{t('loading', { ns: 'common' })}</div>
|
||||
|
|
|
|||
|
|
@ -47,9 +47,8 @@ const AppSelector: FC<Props> = ({
|
|||
onSelect,
|
||||
}) => {
|
||||
const { t } = useTranslation()
|
||||
const [isShow, onShowChange] = useState(false)
|
||||
const [isShow, setIsShow] = useState(false)
|
||||
const [searchText, setSearchText] = useState('')
|
||||
const [isLoadingMore, setIsLoadingMore] = useState(false)
|
||||
|
||||
const {
|
||||
data,
|
||||
|
|
@ -97,25 +96,16 @@ const AppSelector: FC<Props> = ({
|
|||
const hasMore = hasNextPage ?? true
|
||||
|
||||
const handleLoadMore = useCallback(async () => {
|
||||
if (isLoadingMore || isFetchingNextPage || !hasMore)
|
||||
if (isFetchingNextPage || !hasMore)
|
||||
return
|
||||
|
||||
setIsLoadingMore(true)
|
||||
try {
|
||||
await fetchNextPage()
|
||||
}
|
||||
finally {
|
||||
// Add a small delay to ensure state updates are complete
|
||||
setTimeout(() => {
|
||||
setIsLoadingMore(false)
|
||||
}, 300)
|
||||
}
|
||||
}, [isLoadingMore, isFetchingNextPage, hasMore, fetchNextPage])
|
||||
await fetchNextPage()
|
||||
}, [fetchNextPage, hasMore, isFetchingNextPage])
|
||||
|
||||
const handleTriggerClick = () => {
|
||||
if (disabled)
|
||||
return
|
||||
onShowChange(true)
|
||||
setIsShow(true)
|
||||
}
|
||||
|
||||
const [isShowChooseApp, setIsShowChooseApp] = useState(false)
|
||||
|
|
@ -157,7 +147,7 @@ const AppSelector: FC<Props> = ({
|
|||
placement={placement}
|
||||
offset={offset}
|
||||
open={isShow}
|
||||
onOpenChange={onShowChange}
|
||||
onOpenChange={setIsShow}
|
||||
>
|
||||
<PortalToFollowElemTrigger
|
||||
className="w-full"
|
||||
|
|
@ -171,7 +161,7 @@ const AppSelector: FC<Props> = ({
|
|||
<PortalToFollowElemContent className="z-[1000]">
|
||||
<div className="relative min-h-20 w-[389px] rounded-xl border-[0.5px] border-components-panel-border bg-components-panel-bg-blur shadow-lg backdrop-blur-sm">
|
||||
<div className="flex flex-col gap-1 px-4 py-3">
|
||||
<div className="system-sm-semibold flex h-6 items-center text-text-secondary">{t('appSelector.label', { ns: 'app' })}</div>
|
||||
<div className="flex h-6 items-center text-text-secondary system-sm-semibold">{t('appSelector.label', { ns: 'app' })}</div>
|
||||
<AppPicker
|
||||
placement="bottom"
|
||||
offset={offset}
|
||||
|
|
@ -187,7 +177,7 @@ const AppSelector: FC<Props> = ({
|
|||
onSelect={handleSelectApp}
|
||||
scope={scope || 'all'}
|
||||
apps={appsForPicker}
|
||||
isLoading={isLoading || isLoadingMore || isFetchingNextPage}
|
||||
isLoading={isLoading || isFetchingNextPage}
|
||||
hasMore={hasMore}
|
||||
onLoadMore={handleLoadMore}
|
||||
searchText={searchText}
|
||||
|
|
|
|||
|
|
@ -5172,9 +5172,6 @@
|
|||
"app/components/plugins/plugin-detail-panel/app-selector/app-picker.tsx": {
|
||||
"no-restricted-imports": {
|
||||
"count": 1
|
||||
},
|
||||
"tailwindcss/enforce-consistent-class-order": {
|
||||
"count": 2
|
||||
}
|
||||
},
|
||||
"app/components/plugins/plugin-detail-panel/app-selector/app-trigger.tsx": {
|
||||
|
|
@ -5185,9 +5182,6 @@
|
|||
"app/components/plugins/plugin-detail-panel/app-selector/index.tsx": {
|
||||
"no-restricted-imports": {
|
||||
"count": 1
|
||||
},
|
||||
"tailwindcss/enforce-consistent-class-order": {
|
||||
"count": 1
|
||||
}
|
||||
},
|
||||
"app/components/plugins/plugin-detail-panel/datasource-action-list.tsx": {
|
||||
|
|
|
|||
Loading…
Reference in New Issue