Repository navigation
🤖 tests: two-mode reconnect harness, useLiveAPI for stores and deferred writes, stream inventory - #6295
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ThomasK33
changed the title
5d11111 to
fb60c16
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb60c16f6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fb60c16 to
02f2668
Compare
… done; caller abort wins on later pulls
…eanup settles, so detach() can settle them
…deferred writes, stream inventory
…ions with a macrotask
02f2668 to
c2e5350
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2e5350f7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| "operation-stream": { | ||
| describe: "one operation's stream (no restart by design)", | ||
| matches: () => true, |
There was a problem hiding this comment.
When a new long-lived stream holder is mistakenly labeled operation-stream, this unconditional matcher lets the inventory pass even though the stream will remain dead after reconnect. Fresh evidence after the earlier review is that the added rule-shape validation explicitly returns true for this classification, so the original bypass still exists for at least one rule; check that the stream is actually scoped to a one-off operation or restrict this exemption to verified holders.
AGENTS.md reference: AGENTS.md:L233-L235
Useful? React with 👍 / 👎.
1 participant
Summary
This PR is layer 2 of the reconnect stack (base: #6293). It adds a test harness that simulates both reconnect behaviours of the API provider, and
useLiveAPI(), which returns the client only while the connection is live. It moves the stores and the side-chat discard queue touseLiveAPI(). It also adds a test that fails when a new file opens a server stream without a restart rule. Nothing a user sees changes yet.Background
The last PR of the stack makes
useAPI().apikeep the last client while Xum reconnects, instead ofnull, so panes do not blank. After that,api != nullno longer means "online". The code that must see "offline" now readsuseLiveAPI():AppLoader.tsx, andAppConfigStorethroughAPI.tsx).useDiscardSideChatOnLeave). Otherwise it would send its discard into the gap, the call would fail, and the side chat would stay.Today
useLiveAPI()equalsuseAPI().apiin every render. A provider test checks that over three drops and reconnects.Implementation
connection.testHarness.tsxruns the same test in two gap modes:null-gap(today):apigoes tonullin the gap.kept-gap(after the switch):apistays the old client, and its calls reject withConnectionLostError.Each connect builds a real oRPC client over the
ConnectionLinkfrom 🤖 feat: ConnectionLink and ConnectionLostError (not used yet) #6293, as the provider will. A drop keeps each mode's real order. Today React renders the drop first, because it treats a socketcloseevent as a discrete event. After the switch, the link rejects calls first.ArtifactsReconnect.test.tsxnow runs all 11 cases in both modes (describe.each).subscriptionInventory.test.tsreads everyeventIteratorprocedure fromschemas/api.ts(26 today) and finds the renderer files that read them. Each file must state how its stream restarts after a reconnect (effect deps, store feed, router, backoff, one-off operation). A new holder fails the test until it is listed.APIStatenow typesreconnectinganderrorwithapi: APIClient | null. The provider still passesnull. Four closures inWorkflowRunToolCall.tsxrelied on the old narrowing, so they now capture the client in a local.Validation
!apiearly return put back inuseArtifactInteractions, three null-gap cases fail.useAPI().apiinstead of the live client.src/browser/contexts,ArtifactsTab,AppLoader,WorkflowRunToolCalland the new tests (526 + 68).make static-checkalso passes.Risks
Low. The only runtime change is that the stores and the discard queue read a value that equals the old one in every render (tested).
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$23.83