From e1582027f303bbfc38a7526c3e634b6a15860ace Mon Sep 17 00:00:00 2001 From: adrianchen-es <95603111+adrianchen-es@users.noreply.github.com> Date: Fri, 29 May 2026 11:58:40 +1000 Subject: [PATCH] [Canvas] Fix stale nextPage closure in autoplay timer (fix #268125) (#268398) ## Summary Fixes #268125 useEffect cleanup runs after browser paint, creating a window where a pending setTimeout can fire with a stale nextPage closure after pages are added or removed. Fix by storing nextPage in a ref that is updated synchronously during the render phase, so the callback always calls the current function regardless of effect timing. Also adds unit tests that document the exact race-window behaviour. Co-authored-by: Claude Sonnet 4.6 Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit e740fb61067c7e9d3aa15917f419f1a399e9c03b) --- .../hooks/use_autoplay_helper.test.tsx | 97 +++++++++++++++++++ .../workpad/hooks/use_autoplay_helper.ts | 18 ++-- 2 files changed, 109 insertions(+), 6 deletions(-) diff --git a/x-pack/platform/plugins/private/canvas/public/routes/workpad/hooks/use_autoplay_helper.test.tsx b/x-pack/platform/plugins/private/canvas/public/routes/workpad/hooks/use_autoplay_helper.test.tsx index 1b84a3dbc04fd..e0eb9575eb56e 100644 --- a/x-pack/platform/plugins/private/canvas/public/routes/workpad/hooks/use_autoplay_helper.test.tsx +++ b/x-pack/platform/plugins/private/canvas/public/routes/workpad/hooks/use_autoplay_helper.test.tsx @@ -81,4 +81,101 @@ describe('useAutoplayHelper', () => { expect(context.nextPage).toHaveBeenCalled(); }); + + test('calls updated nextPage when page count increases before the timer fires', () => { + const originalNextPage = jest.fn(); + const context = getMockedContext({ + isFullscreen: true, + autoplayInterval: 1000, + nextPage: originalNextPage, + }); + + const { rerender } = renderHook(useAutoplayHelper, { wrapper: getContextWrapper(context) }); + + jest.advanceTimersByTime(600); + + // Simulate page addition: React re-renders, nextPageRef.current is updated synchronously + const updatedNextPage = jest.fn(); + context.nextPage = updatedNextPage; + rerender(); + + // Timer does NOT restart — only 400ms remain from the original interval + jest.advanceTimersByTime(399); + expect(originalNextPage).not.toHaveBeenCalled(); + expect(updatedNextPage).not.toHaveBeenCalled(); + + jest.advanceTimersByTime(1); // reaches the original 1000ms mark + expect(originalNextPage).not.toHaveBeenCalled(); + expect(updatedNextPage).toHaveBeenCalledTimes(1); + }); + + test('calls updated nextPage when page count decreases before the timer fires', () => { + const originalNextPage = jest.fn(); + const context = getMockedContext({ + isFullscreen: true, + autoplayInterval: 1000, + nextPage: originalNextPage, + }); + + const { rerender } = renderHook(useAutoplayHelper, { wrapper: getContextWrapper(context) }); + + jest.advanceTimersByTime(600); + + // Simulate page removal: React re-renders, nextPageRef.current is updated synchronously + const updatedNextPage = jest.fn(); + context.nextPage = updatedNextPage; + rerender(); + + // Timer does NOT restart — only 400ms remain from the original interval + jest.advanceTimersByTime(399); + expect(originalNextPage).not.toHaveBeenCalled(); + expect(updatedNextPage).not.toHaveBeenCalled(); + + jest.advanceTimersByTime(1); // reaches the original 1000ms mark + expect(originalNextPage).not.toHaveBeenCalled(); + expect(updatedNextPage).toHaveBeenCalledTimes(1); + }); + + test('does not call nextPage after page count changes when not in fullscreen', () => { + const context = getMockedContext({ + isFullscreen: false, + autoplayInterval: 1000, + }); + + const { rerender } = renderHook(useAutoplayHelper, { wrapper: getContextWrapper(context) }); + + // Simulate page added: nextPage is recreated + const updatedNextPage = jest.fn(); + context.nextPage = updatedNextPage; + rerender(); + + jest.runAllTimers(); + + expect(updatedNextPage).not.toHaveBeenCalled(); + }); + + test('starts timer with correct nextPage after entering fullscreen following page addition', () => { + const context = getMockedContext({ + isFullscreen: false, + autoplayInterval: 1000, + }); + + const { rerender } = renderHook(useAutoplayHelper, { wrapper: getContextWrapper(context) }); + + // Simulate page added while not in fullscreen + const updatedNextPage = jest.fn(); + context.nextPage = updatedNextPage; + rerender(); + + jest.runAllTimers(); + expect(updatedNextPage).not.toHaveBeenCalled(); + + // User enters fullscreen — timer should now start with the post-addition nextPage + context.isFullscreen = true; + rerender(); + + jest.runAllTimers(); + + expect(updatedNextPage).toHaveBeenCalledTimes(1); + }); }); diff --git a/x-pack/platform/plugins/private/canvas/public/routes/workpad/hooks/use_autoplay_helper.ts b/x-pack/platform/plugins/private/canvas/public/routes/workpad/hooks/use_autoplay_helper.ts index 8e375d3d5d356..b911b73f1366e 100644 --- a/x-pack/platform/plugins/private/canvas/public/routes/workpad/hooks/use_autoplay_helper.ts +++ b/x-pack/platform/plugins/private/canvas/public/routes/workpad/hooks/use_autoplay_helper.ts @@ -11,18 +11,24 @@ export const useAutoplayHelper = () => { const { nextPage, isFullscreen, autoplayInterval, isAutoplayPaused } = useContext(WorkpadRoutingContext); const timer = useRef(undefined); + // Assign during render so the callback always captures the latest nextPage + // without restarting the timer when only the page count changes. + const nextPageRef = useRef(nextPage); + nextPageRef.current = nextPage; useEffect(() => { - if (timer.current || !isFullscreen || isAutoplayPaused) { - clearTimeout(timer.current); - } + clearTimeout(timer.current); + timer.current = undefined; if (isFullscreen && !isAutoplayPaused && autoplayInterval > 0) { timer.current = window.setTimeout(() => { - nextPage(); + nextPageRef.current(); }, autoplayInterval); } - return () => clearTimeout(timer.current); - }, [isFullscreen, nextPage, autoplayInterval, isAutoplayPaused]); + return () => { + clearTimeout(timer.current); + timer.current = undefined; + }; + }, [isFullscreen, autoplayInterval, isAutoplayPaused]); };