diff --git a/packages/host/app/components/ai-assistant/code-block/diff-editor-header.gts b/packages/host/app/components/ai-assistant/code-block/diff-editor-header.gts index 0e69ebdaa6c..e97dc033fd8 100644 --- a/packages/host/app/components/ai-assistant/code-block/diff-editor-header.gts +++ b/packages/host/app/components/ai-assistant/code-block/diff-editor-header.gts @@ -191,7 +191,19 @@ export default class CodeBlockDiffEditorHeader extends Component { @modifiedCode={{this.codeDiffResource.modifiedCode}} /> + {{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}} + + {{/if}} +
+ + Loading diff… +
{{/if}} {{#if this.codePatchErrorMessage}} @@ -402,5 +423,18 @@ class HtmlGroupCodeBlock extends Component { {{/if}} {{/if}} + + } diff --git a/packages/host/app/resources/code-diff.ts b/packages/host/app/resources/code-diff.ts index d99208b780b..49c39c0713c 100644 --- a/packages/host/app/resources/code-diff.ts +++ b/packages/host/app/resources/code-diff.ts @@ -1,3 +1,4 @@ +import { registerDestructor } from '@ember/destroyable'; import { service } from '@ember/service'; import { tracked } from '@glimmer/tracking'; @@ -27,48 +28,63 @@ export class CodeDiffResource extends Resource { @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(); } @@ -76,7 +92,33 @@ export class CodeDiffResource extends Resource { 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); + } 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; @@ -95,17 +137,32 @@ export class CodeDiffResource extends Resource { 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, @@ -114,16 +171,23 @@ export class CodeDiffResource extends Resource { 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( diff --git a/packages/host/app/services/card-service.ts b/packages/host/app/services/card-service.ts index 86d05550240..620d7e4116e 100644 --- a/packages/host/app/services/card-service.ts +++ b/packages/host/app/services/card-service.ts @@ -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, diff --git a/packages/host/tests/integration/components/formatted-aibot-message-test.gts b/packages/host/tests/integration/components/formatted-aibot-message-test.gts index cbae8aa93d8..17702161301 100644 --- a/packages/host/tests/integration/components/formatted-aibot-message-test.gts +++ b/packages/host/tests/integration/components/formatted-aibot-message-test.gts @@ -370,6 +370,348 @@ let c = 3; ); }); + // The code block's arguments are recomputed on every invalidation of the room + // resource, which during streaming arrives continuously. Each of those used to + // restart the load, cancelling the request in flight and leaving the block + // with neither a diff nor an error to show — an empty box — while asking the + // realm for the file again every time. + test('re-rendering with unchanged inputs neither refetches the file nor blanks the diff', async function (assert) { + let getSourceCallCount = 0; + cardService.getSource = async () => { + getSourceCallCount++; + return Promise.resolve({ + status: 200, + contentType: 'application/vnd.card+source', + content: 'let a = 1;\nlet b = 2;', + }); + }; + + let monacoSDK = await monacoService.getMonacoContext(); + let component: any = null; + + class TestComponent extends Component { + @tracked htmlParts = []; + + constructor(owner: Owner, args: any) { + super(owner, args); + component = this; + } + + + } + + await renderComponent(TestComponent); + if (!component) { + throw new Error('Component not found'); + } + + let codeBlockHtml = `
+https://example.com/file.ts
+${SEARCH_MARKER}
+let a = 1;
+${SEPARATOR_MARKER}
+let a = 2;
+${REPLACE_MARKER}
+
`; + + component.htmlParts = parseHtmlContent(codeBlockHtml, roomId, eventId); + await settled(); + await waitFor('.code-block-diff'); + assert.strictEqual( + getSourceCallCount, + 1, + 'the file is fetched once to build the diff', + ); + + // Fresh CodeData objects carrying identical values, which is what an + // invalidation of the room resource produces. + for (let i = 0; i < 5; i++) { + component.htmlParts = parseHtmlContent(codeBlockHtml, roomId, eventId); + await settled(); + } + + assert.strictEqual( + getSourceCallCount, + 1, + 'unchanged inputs do not send the realm another request', + ); + assert.dom('.code-block-diff').exists('the diff survives re-rendering'); + assert + .dom('[data-test-apply-code-button]') + .exists('the apply button survives re-rendering'); + assert + .dom('[data-test-code-patch-loading]') + .doesNotExist('the block is not left in a loading state'); + }); + + // Cancelling the task does not cancel the request behind it, so a superseded + // load has to be stopped explicitly or its work still reaches the realm. + test('a superseded load aborts its request rather than leaving it running', async function (assert) { + let signals: (AbortSignal | undefined)[] = []; + cardService.getSource = async ( + _url: any, + opts?: { signal?: AbortSignal }, + ) => { + signals.push(opts?.signal); + return Promise.resolve({ + status: 200, + contentType: 'application/vnd.card+source', + content: 'let a = 1;\nlet b = 2;', + }); + }; + + let monacoSDK = await monacoService.getMonacoContext(); + let component: any = null; + + class TestComponent extends Component { + @tracked htmlParts = []; + + constructor(owner: Owner, args: any) { + super(owner, args); + component = this; + } + + + } + + await renderComponent(TestComponent); + if (!component) { + throw new Error('Component not found'); + } + + let blockFor = ( + replacement: string, + ) => `
+https://example.com/file.ts
+${SEARCH_MARKER}
+let a = 1;
+${SEPARATOR_MARKER}
+${replacement}
+${REPLACE_MARKER}
+
`; + + component.htmlParts = parseHtmlContent( + blockFor('let a = 2;'), + roomId, + eventId, + ); + await settled(); + assert.strictEqual(signals.length, 1, 'the first load ran'); + assert.false( + signals[0]?.aborted, + 'its signal is live while it is the current load', + ); + + // A genuinely different patch, so the load is restarted rather than reused. + component.htmlParts = parseHtmlContent( + blockFor('let a = 3;'), + roomId, + eventId, + ); + await settled(); + + assert.strictEqual(signals.length, 2, 'the changed patch started a load'); + assert.true( + signals[0]?.aborted, + 'the superseded load had its request aborted', + ); + assert.false(signals[1]?.aborted, 'the current load is untouched'); + }); + + // `loadDiff` runs outside the task, so cancelling the task does not stop it — + // it keeps going at every await, and the abort signal only reaches the fetch. + // A superseded load that resumes afterwards must not write its answer over + // the patch that replaced it. + test('a superseded load that resolves late does not overwrite the newer diff', async function (assert) { + let releases: Array<(content: string) => void> = []; + cardService.getSource = async () => { + let content = await new Promise((resolve) => { + releases.push(resolve); + }); + return { + status: 200, + contentType: 'application/vnd.card+source', + content, + }; + }; + + let monacoSDK = await monacoService.getMonacoContext(); + let component: any = null; + + class TestComponent extends Component { + @tracked htmlParts = []; + + constructor(owner: Owner, args: any) { + super(owner, args); + component = this; + } + + + } + + await renderComponent(TestComponent); + if (!component) { + throw new Error('Component not found'); + } + + let blockFor = ( + search: string, + replacement: string, + ) => `
+https://example.com/file.ts
+${SEARCH_MARKER}
+${search}
+${SEPARATOR_MARKER}
+${replacement}
+${REPLACE_MARKER}
+
`; + + // The abandoned load searches for something the file does not contain, so + // if it ever finishes it reports that the patch could not be applied. That + // makes the clobber observable without reading anything out of the diff + // editor, whose internals are not this test's business. + component.htmlParts = parseHtmlContent( + blockFor('let missing = 0;', 'let a = 2;'), + roomId, + eventId, + ); + await waitUntil(() => releases.length === 1); + + // A patch that does apply takes over before the first one has its source. + component.htmlParts = parseHtmlContent( + blockFor('let a = 1;', 'let a = 3;'), + roomId, + eventId, + ); + await waitUntil(() => releases.length === 2); + + // The newer load finishes first and puts its diff on screen. + releases[1]('let a = 1;'); + await settled(); + await waitFor('.code-block-diff'); + assert + .dom('[data-test-error-message]') + .doesNotExist('the applicable patch loaded cleanly'); + + // Only now does the abandoned one come back with its answer. + releases[0]('let a = 1;'); + await settled(); + + assert + .dom('[data-test-error-message]') + .doesNotExist( + 'the abandoned load does not report its failure over the newer patch', + ); + assert.dom('.code-block-diff').exists('the newer diff is still on screen'); + assert + .dom('[data-test-apply-code-button]') + .exists('and is still the one on offer'); + }); + + test('a patch whose diff has not arrived says so instead of rendering nothing', async function (assert) { + let releaseGetSource: (() => void) | undefined; + cardService.getSource = async () => { + await new Promise((resolve) => { + releaseGetSource = resolve; + }); + return { + status: 200, + contentType: 'application/vnd.card+source', + content: 'let a = 1;\nlet b = 2;', + }; + }; + + // Deliberately not awaited: the assertion is about the window before the + // fetch settles, which is exactly what `settled()` would wait past. + let rendering = renderFormattedAiBotMessage({ + htmlParts: parseHtmlContent( + `
+https://example.com/file.ts
+${SEARCH_MARKER}
+let a = 1;
+${SEPARATOR_MARKER}
+let a = 2;
+${REPLACE_MARKER}
+
`, + roomId, + eventId, + ), + isStreaming: false, + isLastAssistantMessage: true, + }); + + try { + await waitFor('[data-test-code-patch-loading]'); + assert + .dom('[data-test-code-patch-loading]') + .exists('the pending patch is visible while its diff loads'); + assert + .dom('[data-test-file-name]') + .containsText('file.ts', 'and it names the file it belongs to'); + } finally { + // Release even if the wait above failed, so a broken assertion fails the + // test rather than leaving the render promise pending forever. + releaseGetSource?.(); + await rendering; + } + + assert + .dom('[data-test-code-patch-loading]') + .doesNotExist('the loading state gives way to the diff'); + assert.dom('.code-block-diff').exists(); + }); + + // The model names the file it is patching and what it writes is not always a + // URL. Constructing one unguarded threw out of a getter, taking the whole + // message render down rather than just the header. + test('a file name that is not a URL does not break the render', async function (assert) { + await renderFormattedAiBotMessage({ + htmlParts: parseHtmlContent( + `
+malformed file url
+${SEARCH_MARKER}
+let a = 1;
+
`, + roomId, + eventId, + ), + isStreaming: false, + isLastAssistantMessage: true, + }); + + assert.dom('.code-block').exists('the message still renders'); + assert + .dom('[data-test-file-name]') + .containsText('malformed file url', 'falling back to the raw name'); + }); + test('it will render either standard code editor or diff editor during streaming depending on whether the individual search/replace blocks are complete', async function (assert) { let monacoSDK = await monacoService.getMonacoContext(); let component: any = null;