-
Notifications
You must be signed in to change notification settings - Fork 40
fix(react): re-evaluate flags on re-render to detect silent provider … #1226
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?
Conversation
…updates Signed-off-by: Developer <[email protected]>
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.
Summary of Changes
Hello @beeme1mr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've addressed a subtle bug in the useFlag
hook where flag values could become stale if the underlying provider state updated without emitting an event, or if the component re-rendered. My changes ensure that the flag value is re-evaluated on every re-render, and the component's state is updated only if the resolved value has truly changed. This prevents the UI from displaying outdated flag values, especially in scenarios where providers don't emit explicit change events.
Highlights
- Re-evaluation on Re-render: I introduced a useEffect hook to actively re-evaluate the flag value on component re-renders, ensuring the UI reflects the latest state from the provider.
- Optimized State Updates: I implemented a check using isEqual to only update the component's state if the resolved flag value has genuinely changed, optimizing performance.
- Lazy useState Initialization: I switched useState initialization to a lazy approach, preventing redundant initial flag evaluations on every render.
- Memoized Callbacks: I memoized event handler callbacks (updateEvaluationDetailsCallback, configurationChangeCallback) using useCallback to improve performance and prevent unnecessary re-creations.
- AbortController Scope Fix: I corrected the scope of AbortController by moving its instantiation inside the useEffect cleanup, ensuring proper signal management for event listeners.
- New Test Coverage: I added comprehensive new tests to specifically cover scenarios where provider state changes silently (without emitting events) and when flag keys change, validating the fix.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request addresses a subtle bug where feature flag values could become stale if the provider's state updated without emitting a change event. The core of the fix is a new useEffect
in the useFlag
hook that re-evaluates the flag on component re-renders, ensuring the UI reflects the latest value. The implementation also includes several good practices like lazy useState
initialization, memoizing callbacks with useCallback
, and correcting the scope of an AbortController
. The accompanying tests are well-written and cover the new behavior effectively. I have one suggestion to improve the new useEffect
to prevent potential issues with stale state.
@@ -288,7 +288,7 @@ describe('OpenFeatureProvider', () => { | |||
{ timeout: DELAY * 4 }, | |||
); | |||
|
|||
expect(screen.getByText('Will says hi')).toBeInTheDocument(); | |||
expect(screen.getByText('Will says aloha')).toBeInTheDocument(); |
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.
This was a bit confusing at first but I think it makes sense now that I've looked into it. Basically, we're using the global context, which updates the evaluated state. The simulated button was in the same component as the flag being evaluated. Since the resolved value has changed, so does this value.
@@ -287,8 +287,7 @@ function attachHandlersAndResolve<T extends FlagValue>( | |||
const client = useOpenFeatureClient(); | |||
const status = useOpenFeatureClientStatus(); | |||
const provider = useOpenFeatureProvider(); | |||
|
|||
const controller = new AbortController(); | |||
const isFirstRender = useRef(true); |
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.
I'm using this useRef
to avoid evaluating twice when the component first renders.
|
||
useEffect(() => { | ||
const controller = new AbortController(); |
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.
We were creating a new abort controller every time the hook ran. 😅
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Michael Beemer <[email protected]>
|
||
// Re-evaluate when dependencies change (handles prop changes like flagKey) | ||
useEffect(() => { | ||
if (isFirstRender.current) { |
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.
I'm not super familiar with this implementation so take my feedback with a grain of salt, but it seems to me that the risks of this optimization outweigh the benefits.
Benefit: as I understand it, this is a very small performance improvement from avoiding a double render after a component is first mounted.
Risk: I can imagine some sort of weird edge case where an initial render uses bad/uninitialized state, then there's a second render that uses the right state, but then no other rendering occurs. In that scenario we'd end up with stale data.
Feels like the risk isn't worth it. Just my 2¢
Also, seems like since we're already comparing newDetails vs evaluationDetails, we're already protected against calling setEvaluationDetails
unnecessarily. So the only optimization here is avoiding a flag evaluation call.
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.
Yeah, it's a little micro optimization that I added it right at the end. I could go either way, honestly.
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.
I do not see the problem, basically the useEffect and the initial value of the useState will be called after each other on the same sender. And after that only the useEffect will trigger for changes so I do not see the risk🤔
But I might not understand it right now.
This PR
useEffect
that runs on re-render to re-evaluate the flag valueisEqual
comparison)useState
to avoid redundant initial evaluationuseCallback
memoization for event handlersAbortController
scope issueNotes
This resolves a subtle issue where the provider state may update without emitting a change event, leading to confusing results. The
useFlag
hook sets the initial evaluated value in auseState
. Since this wasn't in a closure, this evaluation happened any time the component using the hook rerendered but the result was essentially ignored. Adding a logging hook shows that the current results but since this evaluation was made in an existinguseState
, the result had no effect.This resolves a subtle issue where the provider state may update without emitting a change event, leading to stale flag values being displayed.
The
useFlag
hook was evaluating the flag on every re-render (as part of theuseState
initialization), but becauseuseState
only uses its initial value on the first render, these subsequent evaluations were being discarded. This meant that even though the hook was fetching the correct updated value from the provider on each re-render, it was throwing that value away and continuing to display the stale cached value.Adding a logging hook would show the correct evaluation happening (proving the provider had the updated value), but the UI would remain stuck with the old value because the
useState
was ignoring the re-evaluated result.The fix ensures that these re-evaluations on re-render actually update the component's state when the resolved value changes.
The key insight is that the evaluation WAS happening on every re-render (due to how useState works), but React was discarding the result. Your fix makes those evaluations actually matter by checking if the value changed and updating state accordingly.
Original thread: https://cloud-native.slack.com/archives/C06E4DE6S07/p1754508917397519
How to test
I created a test that reproduced the issue, and it failed. I then implemented the changes and verified that the test passed.