Skip to content

fix(react): refresh flags when event streams reconnect - #104

Open
steipete wants to merge 1 commit into
mainfrom
fix/stream-reconnect-refresh
Open

steipete wants to merge 1 commit into
mainfrom
fix/stream-reconnect-refresh

Conversation

@steipete

@steipete steipete commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes stale React SDK flags after the event stream reconnects, when a flag changed while the client was disconnected.

User Impact

With stream enabled, flags refresh on every stream connection as well as on change events. Existing polling and ETag handling remain in place.

Why This Change Was Made

The server starts every stream with hello. The provider now handles that event through its existing refresh path. The redundant memo around the provider's current value is also removed.

Evidence

The focused regression failed before the fix: the initial hello left the request count at one instead of two. It now passes, including an unchanged 304 response and a later reconnect that changes the displayed flag while preserving the conditional-request ETag.

The full Crabbox AWS gate passed with Node 24.18.1 and pnpm 11.24.0: frozen install, lint, typecheck, all 253 tests (6 script, 37 core, 34 React, 48 CLI, 128 Worker), dashboard/demo/CLI builds, docs build, and packed SDK verification. Lint retained 14 warnings in unchanged CSS.

The built SDK was exercised against a real synthetic HTTP/SSE server using native EventSource. Two initial evaluation requests became three after forced disconnect/reconnect; the displayed flag changed to true, and the reconnect request retained the previous ETag. Polling was set to 60 seconds, so the check verifies the stream-triggered refresh.

Independent Codex review is clean through P2. Exact-head CI and CodeQL passed at ee6af3f0005a980ac48a4eb29a2bea67cd5ea52b. @vincentkoc has been requested for the required code-owner review; this PR awaits approval.

@clawsweeper

clawsweeper Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@steipete
steipete requested a review from a team as a code owner September 22, 2026 09:27
@steipete
steipete requested a review from vincentkoc September 22, 2026 09:28
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 22, 2026
@clawsweeper

clawsweeper Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Codex review: needs changes before merge.

What this changes

The React SDK refreshes flags on stream connections and reconnections, with regression coverage, documentation, and equivalent provider-value simplification.

Example: With stream enabled and polling set to 60000 milliseconds, souls changes from false to true while disconnected.

  • Before: Reconnecting leaves souls false until polling or another refresh trigger.
  • After: The reconnect hello triggers evaluation and updates souls to true.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with meaningful regression coverage and concrete transport observations; the reported merge conflict affects landing readiness rather than patch quality.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): Contributor-supplied observations exercise the built React SDK with native EventSource against a real synthetic HTTP/SSE server: forced reconnect triggers an additional evaluation, updates the displayed flag, and retains the ETag before polling.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Flags changed during a stream outage remain stale after reconnection until another refresh occurs.
Reason: This restores the existing stream-freshness contract with a minimal listener change and no new public API; direction is authorized by the supplied maintainer-authored classification.

Merge readiness

⛔ Needs changes before merge - 1 item remains

This PR fixes a reconnect gap that remains on current main; no introduced correctness defect was found, but GitHub reports a merge conflict.

Priority: P2
Reviewed head: ee6af3f0005a980ac48a4eb29a2bea67cd5ea52b

Before merge

  • Complete next step - Sync this PR with current main and resolve the reported merge conflict, preserving the reconnect fix, trailing-slash URL normalization, and both sets of changelog entries.

Findings

None.

Agent review details

How this fits together

The React provider receives Worker SSE notifications, requests context-specific flag evaluations with ETags, and supplies values to React consumers and the existing browser cache.

flowchart LR
A[Worker change log] --> B[SSE hello and change events]
B --> C[React provider refresh]
C --> D[Public evaluation endpoint]
D --> E[React flag values]
E --> F[Browser cache]
Loading

Technical review

Best possible solution:

Land the hello-triggered refresh while preserving current main's stream URL normalization and changelog entries.

Do we have a high-confidence way to reproduce the issue?

Source establishes the failure: the Worker resets its baseline on each connection and sends hello, while current main refreshes only on change; no target code was run during this review.

Is this the best way to solve the issue?

Using the existing refresh owner directly addresses missed changes without a second evaluation path, new option, or protocol change.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against c6843b3efda4.

Provenance checked

  • React stream refresh listeners keeps the original intent (00ac156: feat: real-time flag updates — SSE change stream + SDK refetch-on-ping #55 introduced rapid personalized refetches, a hello baseline, and polling fallback.)
  • Provider value memoization keeps the original intent (93d43e6: The initial typed SDK source contains the identity memo; no separate rationale was recorded in the supplied provenance, and removing it preserves the exact values reference.)

Testing

Proof path: shipped entry point.

Security

None.

Evidence

What I checked:

  • Pinned introduced patch: The patch adds hello to the existing refresh listeners, preserves polling and cleanup, and includes an Unreleased entry and focused regression coverage. (packages/react/src/krillswitch.tsx:280, ee6af3f0005a)
  • Current main still needs the fix: Current main listens only for change events; its newer trailing-slash normalization is separate behavior that must survive conflict resolution. (packages/react/src/krillswitch.tsx:281, c6843b3efda4)
  • Connection contract and original intent: Every Worker connection starts with hello using a fresh version baseline, so disconnected changes need a connection-triggered evaluation; feat: real-time flag updates — SSE change stream + SDK refetch-on-ping #55 documents that baseline and retained polling fallback. (apps/api/src/stream.ts:67, ee6af3f0005a)
  • Contributor transport observations: The fully read body reports the built SDK using native EventSource against a real synthetic HTTP/SSE server: forced reconnect increased evaluations from two to three, changed the displayed flag to true, and retained its ETag before the 60-second poll. These are contributor observations, not reviewer execution; captured sourceRevision is a061f5c1da618126a15a7f8e89241f65584b4d5c7dd77e0dc2aa7fd680f206cc. (ee6af3f0005a)
  • Regression coverage: The added test exercises initial hello, unchanged 304 handling, and a subsequent hello updating the rendered flag while retaining the previous conditional-request ETag. (packages/react/src/krillswitch.test.tsx:715, ee6af3f0005a)
  • Confirmed remaining conflict: REST metadata for the reviewed head still reports mergeable false and mergeable_state dirty, confirming the earlier review's unresolved synchronization action. (ee6af3f0005a)

Likely related people:

  • Vincent Koc: Raw commit 00ac156 adds packages/react/src/krillswitch.tsx:284 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 00ac156c8c0a; files: packages/react/src/krillswitch.tsx)
  • Jesse Merhi: Raw commit 69dc786 adds packages/react/src/krillswitch.tsx:216 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 69dc78661ff7; files: packages/react/src/krillswitch.tsx)

Labels

Label changes:

  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.

Label justifications:

  • P2: Stream-enabled clients can retain stale flags after reconnect, with existing polling limiting the affected window.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.
  • proof: sufficient: Contributor real behavior proof is sufficient.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (4 earlier review cycles)
  • reviewed 2026-09-22T09:31:28.418Z sha ee6af3f :: needs maintainer review before merge. :: none
  • reviewed 2026-09-22T09:39:55.673Z sha ee6af3f :: needs maintainer review before merge. :: none
  • reviewed 2026-10-02T13:31:24.678Z sha ee6af3f :: needs maintainer review before merge. :: none
  • reviewed 2026-10-08T17:48:44.087Z sha ee6af3f :: needs changes before merge. :: none

Reviewed October 12, 2026, 9:25 AM ET / 13:25 UTC (Revision 5).

@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Oct 12, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant