fix(web): only settle provider status after all pending context changes

[FunnyGhost]

This PR

Fixes two related defects in runProviderContextChangeHandler (web SDK):

1. Provider status settles per-callback instead of per-transition

wrapper.status = READY (and = ERROR) ran unconditionally as each onContextChange completed, while the corresponding ContextChanged/Error events were already gated on allContextChangesSettled. With two overlapping setContext calls, the faster completion set the status to READY while the earlier change was still reconciling:

t=0    change #1 starts        → RECONCILING (pending=2 after #2)
t=50   change #2 starts
t=100  #2 completes            → status=READY  ← older change still in flight
t=300  #1 completes            → ContextChanged fires (correct, gated)

Anything gating on providerStatus === READY (e.g. suspense helpers in the React SDK) could unblock at t=100 and evaluate against a flag cache that was still mid-transition. This change gates the status writes on allContextChangesSettled, exactly mirroring the existing event gating, so status and events can never disagree.

2. Pending-changes counter corruption on synchronous throw

The catch block decremented pendingContextChanges unconditionally. An onContextChange implementation that throws synchronously never incremented the counter, so the decrement drove it negative — after which allContextChangesSettled was permanently false and ContextChanged/Error events were suppressed for the lifetime of the provider. The decrement now lives in a finally around the awaited promise, guaranteeing exactly one decrement per increment.

Behavior notes

• Single (non-overlapping) context changes are unaffected: pending goes 0→1→0 and status settles as before.
• Synchronous/absent onContextChange handlers are unaffected (no increment, allContextChangesSettled is true, status settles immediately).
• With concurrent changes, the status is now set once, by the last-settling handler, with that handler's outcome — the same semantics the events already had.

Tests

Two new tests in packages/web/test/events.spec.ts:

• overlapping changes: status stays RECONCILING until all changes settle, ContextChanged fires once (fails on main with READY mid-overlap);
• synchronously-throwing handler: Error event fires and a subsequent successful change returns the provider to READY (fails on main — the event is suppressed by the corrupted counter).

Full jest matrix (shared/server/web/react/nest, 614 tests) passes.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

Context change settlement

Layer / File(s)	Summary
Context change settlement control flow packages/web/src/open-feature.ts	Pending context changes are decremented in a finally block, while READY and ERROR transitions wait until all changes settle.
Concurrent and reentrant context change validation packages/web/test/events.spec.ts	Tests cover overlapping updates, deferred events, synchronous provider errors, and recovery after a later successful update.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: aepfli, beeme1mr, lukas-reining

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: delaying provider status settlement until all pending context changes finish.
Description check	✅ Passed	The description directly matches the code and tests changes, describing both the status-gating fix and the counter corruption fix.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/web/src/open-feature.ts (1)

414-426: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Error logging is now silently dropped for non-final overlapping failures.

Previously this._logger?.error(...) ran on every caught error; it's now nested inside if (wrapper.allContextChangesSettled). Per spec, deferring the ERROR status transition and event until settlement is correct (see prior comment), but this also suppresses the log line entirely for any onContextChange rejection that isn't the last of the overlapping batch to settle — that failure is never recorded anywhere, making it hard to debug transient/racing provider errors in production.

Decouple logging from the event-gating so every error is at least logged, while still gating status/event emission on allContextChangesSettled:

♻️ Proposed fix

     } catch (err) {
-      // run error handlers instead, once all in-flight context changes have settled
-      if (wrapper.allContextChangesSettled) {
-        wrapper.status = this._statusEnumType.ERROR;
-        const error = err as Error | undefined;
-        const message = `Error running ${providerName}'s context change handler: ${error?.message}`;
-        this._logger?.error(`${message}`, err);
-        this.getAssociatedEventEmitters(domain).forEach((emitter) => {
+      const error = err as Error | undefined;
+      const message = `Error running ${providerName}'s context change handler: ${error?.message}`;
+      // always log, even if a still-pending overlapping change will ultimately determine the settled status
+      this._logger?.error(`${message}`, err);
+      // run error handlers instead, once all in-flight context changes have settled
+      if (wrapper.allContextChangesSettled) {
+        wrapper.status = this._statusEnumType.ERROR;
+        this.getAssociatedEventEmitters(domain).forEach((emitter) => {
           emitter?.emit(ProviderEvents.Error, { clientName: domain, domain, providerName, message });
         });
         this._apiEmitter?.emit(ProviderEvents.Error, { clientName: domain, domain, providerName, message });
       }
     }

🤖 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 `@packages/web/src/open-feature.ts` around lines 414 - 426, Move the error
construction and this._logger?.error call in the context-change catch path
outside the wrapper.allContextChangesSettled check so every onContextChange
failure is logged. Keep the status update and ProviderEvents.Error emissions
inside the allContextChangesSettled guard, preserving deferred status and event
behavior for overlapping changes.

🤖 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.

Outside diff comments:
In `@packages/web/src/open-feature.ts`:
- Around line 414-426: Move the error construction and this._logger?.error call
in the context-change catch path outside the wrapper.allContextChangesSettled
check so every onContextChange failure is logged. Keep the status update and
ProviderEvents.Error emissions inside the allContextChangesSettled guard,
preserving deferred status and event behavior for overlapping changes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3c820548-8df8-4e21-8578-997eecb8fce9

📥 Commits

Reviewing files that changed from the base of the PR and between a98f914 and 64ce82a.

📒 Files selected for processing (2)

• packages/web/src/open-feature.ts
• packages/web/test/events.spec.ts