-
Notifications
You must be signed in to change notification settings - Fork 742
Flush async feature flag updates immediately #16949
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| import { act, renderHook } from '@testing-library/react'; | ||
| import { Map as ImmutableMap } from 'immutable'; | ||
| import { setFlag } from '@console/internal/actions/flags'; | ||
| import { useConsoleDispatch } from '@console/shared/src/hooks/useConsoleDispatch'; | ||
| import { useConsoleSelector } from '@console/shared/src/hooks/useConsoleSelector'; | ||
| import { useFeatureFlagController } from '../FeatureFlagExtensionLoader'; | ||
|
|
||
| jest.mock('@console/shared/src/hooks/useConsoleSelector', () => ({ | ||
| useConsoleSelector: jest.fn(), | ||
| })); | ||
|
|
||
| jest.mock('@console/shared/src/hooks/useConsoleDispatch', () => ({ | ||
| useConsoleDispatch: jest.fn(), | ||
| })); | ||
|
|
||
| jest.mock('@console/internal/actions/flags', () => ({ | ||
| ...jest.requireActual('@console/internal/actions/flags'), | ||
| setFlag: jest.fn((flag: string, value: boolean) => ({ | ||
| type: 'setFlag', | ||
| payload: { flag, value }, | ||
| })), | ||
| })); | ||
|
|
||
| const mockDispatch = jest.fn(); | ||
| const mockUseSelector = useConsoleSelector as jest.Mock; | ||
| const mockUseDispatch = useConsoleDispatch as jest.Mock; | ||
| const mockSetFlag = setFlag as jest.MockedFunction<typeof setFlag>; | ||
|
|
||
| describe('useFeatureFlagController', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| mockUseDispatch.mockReturnValue(mockDispatch); | ||
| mockUseSelector.mockReturnValue(ImmutableMap()); | ||
| }); | ||
|
|
||
| it('defers flag updates made during render until after layout effects', () => { | ||
| const { result } = renderHook(() => { | ||
| const setFeatureFlag = useFeatureFlagController(); | ||
| // Simulate console.flag/hookProvider handlers that set flags during render. | ||
| setFeatureFlag('SYNC_FLAG', true); | ||
| return setFeatureFlag; | ||
| }); | ||
|
|
||
| expect(mockDispatch).toHaveBeenCalledTimes(1); | ||
| expect(mockSetFlag).toHaveBeenCalledWith('SYNC_FLAG', true); | ||
| expect(result.current).toEqual(expect.any(Function)); | ||
| }); | ||
|
Comment on lines
+36
to
+47
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Assert that no dispatch occurs during render. Line 44 runs after layout effects complete. A regression that dispatches directly during render still produces one dispatch and passes this test. Capture the dispatch count immediately after 🤖 Prompt for AI Agents |
||
|
|
||
| it('dispatches async flag updates immediately without waiting for another render', () => { | ||
| const { result } = renderHook(() => useFeatureFlagController()); | ||
|
|
||
| mockDispatch.mockClear(); | ||
| mockSetFlag.mockClear(); | ||
|
|
||
| act(() => { | ||
| result.current('ASYNC_FLAG', true); | ||
| }); | ||
|
|
||
| expect(mockDispatch).toHaveBeenCalledTimes(1); | ||
| expect(mockSetFlag).toHaveBeenCalledWith('ASYNC_FLAG', true); | ||
| }); | ||
|
|
||
| it('does not redispatch when the flag already has the requested value', () => { | ||
| mockUseSelector.mockReturnValue(ImmutableMap({ EXISTING_FLAG: true })); | ||
| const { result } = renderHook(() => useFeatureFlagController()); | ||
|
|
||
| mockDispatch.mockClear(); | ||
|
|
||
| act(() => { | ||
| result.current('EXISTING_FLAG', true); | ||
| }); | ||
|
|
||
| expect(mockDispatch).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update
flagsRefafter each dispatched flag update.Line 47 reads the last selector snapshot, not the last dispatched value. If a flag is currently
false, two post-layout calls can set it totrueand thenfalsebefore React re-renders. The first call dispatchestrue. The second call sees the stalefalsevalue and skips its required dispatch. The Redux flag remainstrue.Record the dispatched value in
flagsRefbefore clearing the pending update. Add a regression test for consecutivetruethenfalsecalls without a selector re-render.Proposed fix
if (flagsRef.current.get(flag) !== enabled) { dispatch(setFlag(flag, enabled)); + flagsRef.current = flagsRef.current.set(flag, enabled); }📝 Committable suggestion
🤖 Prompt for AI Agents