Fix blank code blocks in assistant - #5772
Conversation
A code block's arguments are recomputed on every invalidation of the room resource, which during streaming arrives continuously. `modify` treated each one as a reason to start over: it cleared any error and performed the load again, and because that load is a restartable task, performing it cancelled the request in flight. Two things followed. The resource sat holding no code and no error, which is the one state the template renders as nothing at all — a code block with a header, an editor and an apply button in the DOM but no content in any of them. And since cancelling the task does not cancel the request behind it, every restart left its fetch running and started another, so a patch creating a new file — where a 404 is the expected answer — asked the realm for that file thousands of times. The requests then contended with each other, so each load took longer, so more of them were cancelled before they could finish. Restart only when the file or the patch actually changes; otherwise leave a load that is already under way alone. Give the load an abort signal so a superseded one stops rather than merely having its answer discarded, and thread that signal through getSource. Keep the in-flight flag untracked, since consuming tracked state in `modify` would re-enter it on every change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 1h 37m 16s ⏱️ + 1h 31m 55s Results for commit 3391d1d. ± Comparison against earlier commit ca418d6. Realm Server Test Results 1 files ±0 1 suites ±0 15m 50s ⏱️ + 3m 27s Results for commit 3391d1d. ± Comparison against earlier commit ca418d6. |
Rendering it for any truthy file URL reached the header with values that are not URLs — the model names the file it is patching, and what it writes is not always parseable. `fileName` constructs a URL from it unguarded, so the getter threw and took the whole message render down with it. A patch that failed does not need the header anyway: the footer's alert already speaks for it, and the empty block this was meant to fix is the pending one. Restrict the header to that case, and stop `fileName` from throwing so a header that is merely wrong cannot break the render. The fallback matters beyond this branch — the standard code block renders the same header for any truthy file url. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three behaviours shipped without tests, each one the kind that breaks quietly. A superseded load has to abort its request, since cancelling the task leaves the fetch running and only discards the answer — the difference between a bounded restart and a request storm is invisible from the rendered output. A patch whose diff has not arrived has to say so. Asserting only that the loading state is absent once settled would pass just as well if it never rendered at all, which is the bug this was written to prevent. And a file name that is not a URL has to reach the header without throwing. The model names the file it is patching and what it writes is not always parseable; constructing a URL from it unguarded took down the whole message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![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: fe6879a19f
ℹ️ 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".
| this.abortController = abortController; | ||
| this.loadInFlight = true; | ||
| try { | ||
| await this.loadDiff(abortController.signal); |
There was a problem hiding this comment.
When the inputs change after getSource resolves but while ApplySearchReplaceBlockTool.execute() is awaiting its module load, cancelling this restartable task stops only the outer task at this await; loadDiff() is a separate ordinary async operation, and the abort signal no longer stops it after the fetch. The superseded operation can therefore later overwrite originalCode, modifiedCode, or errorMessage belonging to the newer patch (and can also mutate after destruction), displaying or copying a stale diff. Keep the state-mutating awaits directly in the task or verify the controller/signal after every await before assigning resource state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Real bug, and precisely diagnosed. Fixed in f3d3f89.
You're right about the boundary: loadDiff is an ordinary async function, so cancelling the task only stops things at the await inside the task body. Past that, the abort signal reaches the fetch and nothing else — ApplySearchReplaceBlockTool.execute() awaits a module load with no knowledge of it.
Worth naming the exact hole, because it wasn't where I'd assumed. The catch after getSource already had if (signal.aborted) return, so I'd covered the fetch rejected mid-flight case. What was uncovered was the success path — a fetch that completed before the supersession, followed by an execute() that spans it. That run then resumed and assigned originalCode, modifiedCode, and errorMessage belonging to the newer patch.
Took the second of your two options — verify after every await — rather than moving the work into the task body, because it holds regardless of how ember-concurrency cancels an async task, and I didn't want the correctness of this to rest on my reading of those internals.
let result = await this.cardService.getSource(new URL(fileUrl), { signal });
if (signal.aborted) return;
originalCode = result.status === 404 ? '' : result.content;…and the same guard on both the resolve and reject paths of execute(). One related change your comment implies: the patch is now applied against originalCode — the value this load fetched — instead of this.originalCode, which by then may belong to a different patch entirely.
The destruction case you mention falls out of the same guard, since the destructor aborts the controller.
Added a regression test that forces the ordering deterministically: two loads started with a deferred getSource, the newer one released and allowed to render its diff, then the abandoned one released afterwards. It asserts the diff still shows the newer replacement. Against the previous code the late load overwrites it.
Not verified locally — Colima is down here, so CI is exercising it.
The load runs outside the task, so cancelling the task stops it only at the await inside the task body. Past that point the work carries on regardless, and the abort signal reaches the fetch and nothing else — so a load superseded while awaiting the patch tool resumed later and wrote its answer over the state belonging to the patch that replaced it, putting a stale diff on screen and mutating a resource that may already be gone. Check the signal after each await before writing anything, and apply the patch against the code this load fetched rather than whatever the resource holds by the time it resolves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The assertion read `view-lines[2]`, copied from a test that renders several editors. This one renders a single diff, so that index held nothing, the predicate was never true, and the wait ran to its timeout instead of failing on anything real. Search every pane for the replacement instead, and wait on the insert decoration the way the neighbouring diff test already does. The abandoned patch's replacement is now asserted absent too, which is the property the test exists to hold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reading the replacement back out of monaco made the test depend on how the diff editor lays out its panes and which decorations it emits for a one-line change — twice that guess was wrong, and both times it surfaced as a wait running to its timeout rather than as anything about the behaviour under test. Give the abandoned load a search string the file does not contain, so finishing is something it cannot do quietly: it reports that the patch would not apply. The property then reads directly off the absence of that error, and the only DOM this test touches is the diff container and the alert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
I’ve been seeing gaps like this:
it comes with an infinite series of 404s, which actually managed to take down staging:
Claude’s explanation
A code block's arguments are recomputed on every invalidation of the room resource, which during streaming arrives continuously. `modify` treated each one as a reason to start over: it cleared any error and performed the load again, and because that load is a restartable task, performing it cancelled the request in flight.Two things followed. The resource sat holding no code and no error, which is the one state the template renders as nothing at all — a code block with a header, an editor and an apply button in the DOM but no content in any of them. And since cancelling the task does not cancel the request behind it, every restart left its fetch running and started another, so a patch creating a new file — where a 404 is the expected answer — asked the realm for that file thousands of times. The requests then contended with each other, so each load took longer, so more of them were cancelled before they could finish.
Restart only when the file or the patch actually changes; otherwise leave a load that is already under way alone. Give the load an abort signal so a superseded one stops rather than merely having its answer discarded, and thread that signal through getSource. Keep the in-flight flag untracked, since consuming tracked state in
modifywould re-enter it on every change.