Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,19 @@ export default class CodeBlockDiffEditorHeader extends Component<CodeBlockDiffEd
}

private get fileName() {
return new URL(this.fileUrl ?? '').pathname.split('/').pop() || '';
let fileUrl = this.fileUrl;
if (!fileUrl) {
return '';
}
try {
return new URL(fileUrl).pathname.split('/').pop() || '';
} catch {
// The model names the file it is patching, and what it writes is not
// always a URL. Falling back to the last path segment keeps a header that
// is merely wrong from taking the whole message down with it; whether the
// patch can be applied is reported separately.
return fileUrl.split('/').pop() || '';
}
}

private get sourceUrl(): string | null {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { htmlSafe } from '@ember/template';
import Component from '@glimmer/component';
import { cached, tracked } from '@glimmer/tracking';

import { Alert } from '@cardstack/boxel-ui/components';
import { Alert, LoadingIndicator } from '@cardstack/boxel-ui/components';
import { and, bool, eq } from '@cardstack/boxel-ui/helpers';

import { markdownToHtml } from '@cardstack/runtime-common/marked-sync';
Expand Down Expand Up @@ -359,6 +359,27 @@ class HtmlGroupCodeBlock extends Component<HtmlGroupCodeBlockSignature> {
@modifiedCode={{this.codeDiffResource.modifiedCode}}
/>
</codeBlock.actions>
{{else if this.codeDiffResource.isLoadingDiff}}
{{! The header needs only the file URL, which is known as soon as
the patch is parsed, so a diff still being fetched can name the file
it belongs to. Without this the whole block renders empty and an
unresolved patch is indistinguishable from nothing at all. A patch
that failed is not shown here — it has the footer's alert to speak
for it. }}
{{#if @codeData.fileUrl}}
<codeBlock.diffEditorHeader
@codeData={{@codeData}}
@diffEditorStats={{null}}
@originalUploadedFileUrl={{@codePatchResult.originalUploadedFileUrl}}
@codePatchStatus={{@codePatchStatus}}
@userMessageThisMessageIsRespondingTo={{@userMessageThisMessageIsRespondingTo}}
@codePatchErrorMessage={{this.codePatchErrorMessage}}
/>
{{/if}}
<div class='code-patch-loading' data-test-code-patch-loading>
<LoadingIndicator @color='var(--boxel-light)' />
<span>Loading diff…</span>
</div>
{{/if}}

{{#if this.codePatchErrorMessage}}
Expand Down Expand Up @@ -402,5 +423,18 @@ class HtmlGroupCodeBlock extends Component<HtmlGroupCodeBlockSignature> {
{{/if}}
{{/if}}
</CodeBlock>

<style scoped>
.code-patch-loading {
display: flex;
align-items: center;
gap: var(--boxel-sp-xs);
padding: var(--boxel-sp-sm);
background-color: var(--boxel-dark);
color: var(--boxel-light);
font-size: var(--boxel-font-size-sm);
line-height: var(--boxel-line-height-sm);
}
</style>
</template>
}
116 changes: 90 additions & 26 deletions packages/host/app/resources/code-diff.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { registerDestructor } from '@ember/destroyable';
import { service } from '@ember/service';
import { tracked } from '@glimmer/tracking';

Expand Down Expand Up @@ -27,56 +28,97 @@ export class CodeDiffResource extends Resource<CodeDiffResourceArgs> {
@tracked errorMessage: string | undefined | null = null;
codePatchStatus: CodePatchStatus | undefined | null = null;

// Deliberately untracked: `modify` consults this to decide whether a load is
// already under way, and consuming tracked state there would re-enter
// `modify` every time the load changed it. The template reads the task's own
// `isRunning` instead, which is tracked and safe to render from.
private loadInFlight = false;
private abortController: AbortController | undefined;

@service declare private cardService: CardService;
@service declare private toolService: ToolService;

constructor(owner: object) {
super(owner);
registerDestructor(this, () => this.abortController?.abort());
}

modify(_positional: never[], named: CodeDiffResourceArgs['named']) {
let { fileUrl, searchReplaceBlock, codePatchStatus } = named;
let fileOrPatchChanged =
this.fileUrl !== fileUrl ||
this.searchReplaceBlock !== searchReplaceBlock;
let appliedStateChanged =
this.codePatchStatus === 'applied' || codePatchStatus === 'applied';
if (fileOrPatchChanged || appliedStateChanged) {
this.originalCode = null;
this.modifiedCode = null;
}
this.errorMessage = null;
this.codePatchStatus !== codePatchStatus &&
(this.codePatchStatus === 'applied' || codePatchStatus === 'applied');
let inputsChanged = fileOrPatchChanged || appliedStateChanged;

this.fileUrl = fileUrl;
this.searchReplaceBlock = searchReplaceBlock;
this.codePatchStatus = codePatchStatus;

if (!fileUrl) {
// These arguments are recomputed on every invalidation of the room
// resource, which during streaming arrives continuously — so `modify` runs
// far more often than anything about this diff actually changes. Only a
// change to what is being diffed justifies discarding what we have and
// starting again. Restarting on every invalidation cancelled the load
// mid-flight and cleared the error along with it, leaving the resource
// holding neither code nor a message to show, and asking the realm for the
// file again each time it happened.
if (!inputsChanged) {
if (this.isDataLoaded || this.errorMessage || this.loadInFlight) {
return;
}
} else {
this.originalCode = null;
this.modifiedCode = null;
this.errorMessage = null;
}

if (!fileUrl) {
this.errorMessage = 'Missing file URL in the code block';
return;
}

if (!searchReplaceBlock) {
this.originalCode = null;
this.modifiedCode = null;
this.errorMessage = 'Missing search and replace block';
return;
}

if (
!fileOrPatchChanged &&
codePatchStatus !== 'applied' &&
this.originalCode != null &&
this.modifiedCode != null
) {
return;
}

this.load.perform();
}

get isDataLoaded() {
return this.originalCode != null && this.modifiedCode != null;
}

// True between starting a load and having something to show for it, so a
// patch whose diff has not arrived can say so rather than render as nothing.
get isLoadingDiff() {
return this.load.isRunning && !this.isDataLoaded && !this.errorMessage;
}

private load = restartableTask(async () => {
// Cancelling the task does not cancel a request already in flight, so
// without this an abandoned load runs to completion and its answer is
// merely discarded — the work still reaches the realm.
this.abortController?.abort();
let abortController = new AbortController();
this.abortController = abortController;
this.loadInFlight = true;
try {
await this.loadDiff(abortController.signal);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep cancellable work in the task body

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 👍 / 👎.

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.

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

} finally {
// Only the newest run owns the flag. A superseded run settling later must
// not report that loading has finished on behalf of the one that replaced
// it.
if (this.abortController === abortController) {
this.loadInFlight = false;
}
}
});

private async loadDiff(signal: AbortSignal) {
let { fileUrl, searchReplaceBlock, codePatchStatus } = this;
if (codePatchStatus === 'applied') {
this.originalCode = null;
Expand All @@ -95,17 +137,32 @@ export class CodeDiffResource extends Resource<CodeDiffResourceArgs> {
if (!fileUrl || !searchReplaceBlock) {
return;
}
// This runs outside the task, so cancelling the task does not stop it here:
// every await is a point where a newer load may have taken over, and the
// abort signal only reaches the fetch. Check ownership after each one
// before writing to the resource, or a superseded load resumes later and
// overwrites the state — and the diff on screen — belonging to the patch
// that replaced it.
let originalCode: string;
try {
let result = await this.cardService.getSource(new URL(fileUrl));
if (result.status === 404) {
this.originalCode = ''; // We are creating a new file, so we don't have the original code
} else {
this.originalCode = result.content;
let result = await this.cardService.getSource(new URL(fileUrl), {
signal,
});
if (signal.aborted) {
return;
}
// A 404 means we are creating a new file, so there is no original code.
originalCode = result.status === 404 ? '' : result.content;
} catch (error) {
if (signal.aborted) {
// Superseded by a newer load, or the resource went away. Neither is a
// failure to report.
return;
}
this.errorMessage = `Failed to load code from ${fileUrl}`;
return;
}
this.originalCode = originalCode;

let applySearchReplaceBlockCommand = new ApplySearchReplaceBlockTool(
this.toolService.toolContext,
Expand All @@ -114,16 +171,23 @@ export class CodeDiffResource extends Resource<CodeDiffResourceArgs> {
try {
let { resultContent: patchedCode } =
await applySearchReplaceBlockCommand.execute({
fileContent: this.originalCode,
// This load's own code, not whatever the resource holds by now.
fileContent: originalCode,
codeBlock: searchReplaceBlock,
});
if (signal.aborted) {
return;
}
this.modifiedCode = patchedCode;
} catch (error) {
this.modifiedCode = this.originalCode;
if (signal.aborted) {
return;
}
this.modifiedCode = originalCode;
this.errorMessage =
error instanceof Error ? error.message : String(error);
}
});
}
}

export function getCodeDiffResultResource(
Expand Down
6 changes: 5 additions & 1 deletion packages/host/app/services/card-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,15 @@ export default class CardService extends Service {
return serialized;
}

async getSource(url: RealmResourceIdentifier | URL) {
async getSource(
url: RealmResourceIdentifier | URL,
opts?: { signal?: AbortSignal },
) {
let response = await this.network.authedFetch(url, {
headers: {
Accept: 'application/vnd.card+source',
},
signal: opts?.signal,
});
return {
status: response.status,
Expand Down
Loading
Loading