Skip to content

chore: call refreshHandler once and even if features are up to date#162

Merged
vazarkevych merged 5 commits into
growthbook:mainfrom
MacPaw:chore/refresh-handler
May 21, 2026
Merged

chore: call refreshHandler once and even if features are up to date#162
vazarkevych merged 5 commits into
growthbook:mainfrom
MacPaw:chore/refresh-handler

Conversation

@nekrich

@nekrich nekrich commented May 20, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR makes refreshHandler behave as a single completion signal. It is now called once for network success/failure handling and for the case where cached features are still fresh and no network request is needed.
This allows us to determine the sate of the GrowthBookSDK and if the features are ready or not.

Problem

refreshCache() delegates to FeaturesViewModel.fetchFeatures, but the previous callback flow only notified refreshHandler from specific success or failure delegate paths and could call it several times. That meant a refresh could restore cached features and finish without notifying callers, so consumers could not reliably treat refreshHandler as a "ready" signal.

Remote evaluation also had two issues in the previous flow:

  • When remoteEval was enabled and the cache was expired, fetchFeatures started both the regular feature fetch and the remote-evaluation fetch.
  • The cache-expiration check only guarded the regular feature fetch; the remote-evaluation fetch still ran even when cached data was fresh.

There was also a duplicate-notification risk because both featuresFetchedSuccessfully and savedGroupsFetchedSuccessfully could call refreshHandler during the same response handling path.

Solution

The change introduces a single delegate method, featuresUpdateIsComplete(error:isRemote:), and routes refreshHandler through that method instead of calling it from individual feature and saved-group success/failure callbacks.

FeaturesViewModel.fetchFeatures now:

  • Reports .invalidAPIURL when no API URL is available.
  • Reports completion with no error when cached features are up to date.
  • Uses the cache-expiration check before choosing either the remote-evaluation fetch or the regular feature fetch.
  • Calls the completion delegate once after remote fetch handling succeeds or fails.

GrowthBookSDK keeps featuresFetchFailed and savedGroupsFetchFailed as no-op delegate methods, if you want me to delete them just ask 😅.

Tests

The patch adds regression coverage for:

  • refreshHandler firing when refreshCache() is called while features are already cached.
  • featuresUpdateIsComplete being called once for remote-evaluation success and failure.
  • featuresUpdateIsComplete being called when cached features are fresh.

@vazarkevych vazarkevych left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nekrich,
thanks for your contribution. The work looks good and the fix is more efficient. Please take a look at the comments - once addressed, I'll happily approve and merge. 🙌

delegate?.featuresFetchedSuccessfully(features: features, isRemote: isRemote)
} else {
delegate?.featuresFetchFailed(error: .failedParsedEncryptedData, isRemote: isRemote)
let erorr = SDKError.failedParsedEncryptedData

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

erorr typo - also appears on lines 95 and 101.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c25abd8

Comment thread Sources/CommonMain/GrowthBookSDK.swift Outdated
func featuresFetchFailed(error: SDKError, isRemote: Bool) {}

func featuresUpdateIsComplete(error: SDKError?, isRemote: Bool) {
if isRemote {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isRemote guard is not needed here. I believe we can remove this parameter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0605a13

fetchCachedFeatures(logging: true)
if isCacheExpired(), let apiUrl = apiUrl {
guard let apiUrl else {
delegate?.featuresUpdateIsComplete(error: .invalidAPIURL, isRemote: false)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With isRemote: false it won't fire refreshHandler.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fix is in 0605a13

@vazarkevych vazarkevych merged commit 139e1b8 into growthbook:main May 21, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants