Flush async feature flag updates immediately - #16949
Conversation
Deferring all setFeatureFlag calls until the next render left flag-gated plugin routes and nav items missing for ~10s after an async console.flag/hookProvider resolved. Keep render-time updates deferred, but dispatch async updates right away. Fixes openshift#16922 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kchawlani19 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe feature flag controller is now exported, tracks current flag values, defers render-time updates until the layout effect, flushes post-render updates immediately, and skips redundant dispatches. Tests cover each update path. ChangesFeature flag controller
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/packages/console-app/src/components/flags/__tests__/FeatureFlagExtensionLoader.spec.tsx`:
- Around line 36-47: Update the renderHook test around useFeatureFlagController
to record mockDispatch’s call count immediately after setFeatureFlag runs during
render, assert it is unchanged before layout effects flush, then retain the
post-render assertions verifying the deferred update dispatches once with the
expected flag.
In
`@frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx`:
- Around line 45-51: Update flushPendingUpdates so that after dispatching each
changed flag via setFlag, flagsRef.current records the dispatched enabled value
before pendingUpdatesRef.current is cleared. Add a regression test covering
consecutive true then false updates without a selector re-render, verifying both
dispatches occur.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f564b80-5d5d-47a1-a0b9-c82d1c721727
📒 Files selected for processing (2)
frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsxfrontend/packages/console-app/src/components/flags/__tests__/FeatureFlagExtensionLoader.spec.tsx
| 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)); | ||
| }); |
There was a problem hiding this comment.
📐 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 setFeatureFlag in the render callback, then assert that the count did not change until the layout effect flushes the update.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/packages/console-app/src/components/flags/__tests__/FeatureFlagExtensionLoader.spec.tsx`
around lines 36 - 47, Update the renderHook test around useFeatureFlagController
to record mockDispatch’s call count immediately after setFeatureFlag runs during
render, assert it is unchanged before layout effects flush, then retain the
post-render assertions verifying the deferred update dispatches once with the
expected flag.
| const flushPendingUpdates = useCallback(() => { | ||
| pendingUpdatesRef.current.forEach((enabled, flag) => { | ||
| if (flags.get(flag) !== enabled) { | ||
| if (flagsRef.current.get(flag) !== enabled) { | ||
| dispatch(setFlag(flag, enabled)); | ||
| } | ||
| }); | ||
| pendingUpdatesRef.current.clear(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update flagsRef after 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 to true and then false before React re-renders. The first call dispatches true. The second call sees the stale false value and skips its required dispatch. The Redux flag remains true.
Record the dispatched value in flagsRef before clearing the pending update. Add a regression test for consecutive true then false calls 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const flushPendingUpdates = useCallback(() => { | |
| pendingUpdatesRef.current.forEach((enabled, flag) => { | |
| if (flags.get(flag) !== enabled) { | |
| if (flagsRef.current.get(flag) !== enabled) { | |
| dispatch(setFlag(flag, enabled)); | |
| } | |
| }); | |
| pendingUpdatesRef.current.clear(); | |
| const flushPendingUpdates = useCallback(() => { | |
| pendingUpdatesRef.current.forEach((enabled, flag) => { | |
| if (flagsRef.current.get(flag) !== enabled) { | |
| dispatch(setFlag(flag, enabled)); | |
| flagsRef.current = flagsRef.current.set(flag, enabled); | |
| } | |
| }); | |
| pendingUpdatesRef.current.clear(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx`
around lines 45 - 51, Update flushPendingUpdates so that after dispatching each
changed flag via setFlag, flagsRef.current records the dispatched enabled value
before pendingUpdatesRef.current is cleared. Add a regression test covering
consecutive true then false updates without a selector re-render, verifying both
dispatches occur.
|
/test backend |
|
/retest |
|
@kchawlani19: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary
console.flag/console.flag/hookProviderresolved.useFeatureFlagControlleronly queuedsetFeatureFlaginto a ref and flushed on the next render, so async updates (e.g. after a backend probe) waited on an unrelated re-render.Test plan
jest packages/console-app/src/components/flags/__tests__/FeatureFlagExtensionLoader.spec.tsxconsole.flag/hookProviderthat gates aconsole.page/routeand nav itemMade with Cursor
Summary by CodeRabbit
Bug Fixes
Tests