added featuresNotModified#154
Conversation
madhuchavva
left a comment
There was a problem hiding this comment.
Inline review comments on 304 semantics and V2 handler integration.
| success = false; | ||
| } | ||
| // Refresh TTL on success or 304 Not Modified (null means server confirmed cache is still valid) | ||
| // Refresh TTL on success or 304 Not Modified (null means server confirmed cache is still valid) |
There was a problem hiding this comment.
I agree with the goal: a real 304 should unblock refresh handlers instead of leaving callers waiting. The current implementation can report success even when we do not actually have cached features. In the new test, the cache is cleared and the mock returns notModified, but featuresNotModified() still fires. In a real app that means the SDK can signal refreshHandler(true) while still having no valid cached feature payload. Can we gate this on having successfully loaded cached features first, or make the network layer explicitly report a 304 so FeatureViewModel can distinguish “server validated existing cache” from “no success callback happened”?
There was a problem hiding this comment.
We fixed this by passing hasCachedFeatures into _fetchFromNetwork(), derived from whether the cache loaded successfully before the network call. A 304 now only triggers featuresNotModified() — and by extension refreshHandler(true) — when a valid cached payload exists to confirm. With no cache, the 304 is silently ignored.
We also updated the existing test that was asserting the broken behavior and added a new test covering both cases explicitly.
| @@ -180,6 +179,13 @@ class GrowthBookSDK extends FeaturesFlowDelegate { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This should also integrate with the refresh-handler V2 work in #109. If V2 lands, the 304 path should notify refreshHandlerV2(true, null) as well, otherwise V2 users can still hang on 304.
There was a problem hiding this comment.
We acknowledge the point about refresh-handler V2 — since #109 is not yet merged into main, the refreshHandlerV2 API is not available in the codebase, so we can't add the corresponding 304 integration here yet. Once #109 is merged, we'll follow up with the V2 support here to make sure the 304 path also notifies refreshHandlerV2(true, null).
madhuchavva
left a comment
There was a problem hiding this comment.
Requesting changes based on the inline review comments. The notes call out functional gaps or compatibility/documentation risks that should be addressed before merge.
To align with other SDK implementations and correctly handle HTTP 304 Not Modified responses, this PR adds a featuresNotModified() delegate method that fires refreshHandler(true) when the server confirms cached features are still valid.
Previously, a 304 response was silently ignored — refreshHandler was never called, leaving any dependent code (e.g. loading indicators, UI state updates) stuck waiting for a signal that would never arrive.
Changes: