From 914167bf9b199d7994b02722e224fee50fc3a2e8 Mon Sep 17 00:00:00 2001 From: Jeremy Lewi Date: Sat, 17 Aug 2024 17:55:52 -0700 Subject: [PATCH 1/7] Changes before updating protos for foyle --- src/extension/ai/ghost.ts | 98 +++++++++++++++++++++++-------------- src/extension/ai/manager.ts | 2 +- 2 files changed, 61 insertions(+), 39 deletions(-) diff --git a/src/extension/ai/ghost.ts b/src/extension/ai/ghost.ts index 002139653..58ce39c90 100644 --- a/src/extension/ai/ghost.ts +++ b/src/extension/ai/ghost.ts @@ -7,6 +7,7 @@ import * as serializer from '../serializer' import * as converters from './converters' import * as stream from './stream' import * as protos from './protos' +import { ulid } from 'ulidx' const log = getLogger() @@ -31,8 +32,16 @@ export const ghostKey = '_ghostCell' export class GhostCellGenerator implements stream.CompletionHandlers { private notebookState: Map + // contextID is the ID of the context we are generating completions for. + // It is used to detect whether a completion response is stale and should be + // discarded because the context has changed. + private contextID: string + constructor() { this.notebookState = new Map() + // Generate a random context ID. This should be unnecessary because presumable the event to change + // the active cell will be sent before any requests are sent but it doesn't hurt to be safe. + this.contextID = ulid() } // Updated method to check and initialize notebook state @@ -129,6 +138,14 @@ export class GhostCellGenerator implements stream.CompletionHandlers { // processResponse applies the changes from the response to the notebook. processResponse(response: agent_pb.StreamGenerateResponse) { + if (response.contextId !== this.contextID) { + // TODO(jeremy): Is this logging too verbose? + log.info( + `Ignoring response with contextID ${response.contextId} because it doesn't match the current contextID ${this.contextID}`, + ) + return + } + let cellsTs = protos.cellsESToTS(response.cells) let newCellData = converters.cellProtosToCellData(cellsTs) @@ -189,6 +206,49 @@ export class GhostCellGenerator implements stream.CompletionHandlers { }) } + // handleOnDidChangeActiveTextEditor updates the ghostKey cell decoration and rendering + // when it is selected + handleOnDidChangeActiveTextEditor(editor: vscode.TextEditor | undefined) { + const oldCID = this.contextID + // We need to generate a new context ID because the context has changed. + this.contextID = ulid() + log.info( + `onDidChangeActiveTextEditor fired: editor: ${editor?.document.uri}; new contextID: ${this.contextID}; old contextID: ${oldCID}`, + ) + if (editor === undefined) { + return + } + + if (editor.document.uri.scheme !== 'vscode-notebook-cell') { + // Doesn't correspond to a notebook cell so do nothing + return + } + + const cell = getCellFromCellDocument(editor.document) + if (cell === undefined) { + return + } + + if (!isGhostCell(cell)) { + return + } + // ...cell.metadata creates a shallow copy of the metadata object + const updatedMetadata = { ...cell.metadata, [ghostKey]: false } + const update = vscode.NotebookEdit.updateCellMetadata(cell.index, updatedMetadata) + const edit = new vscode.WorkspaceEdit() + edit.set(editor.document.uri, [update]) + vscode.workspace.applyEdit(edit).then((result: boolean) => { + log.trace(`updateCellMetadata to deactivate ghostcell resolved with ${result}`) + if (!result) { + log.error('applyEdit failed') + return + } + }) + // If the cell is a ghost cell we want to remove the decoration + // and replace it with a non-ghost cell. + editorAsNonGhost(editor) + } + shutdown(): void { log.info('Shutting down') } @@ -314,44 +374,6 @@ function getCellFromCellDocument(textDoc: vscode.TextDocument): vscode.NotebookC return matchedCell } -// handleOnDidChangeActiveTextEditor updates the ghostKey cell decoration and rendering -// when it is selected -export function handleOnDidChangeActiveTextEditor(editor: vscode.TextEditor | undefined) { - log.info(`onDidChangeActiveTextEditor Fired for editor ${editor?.document.uri}`) - if (editor === undefined) { - return - } - - if (editor.document.uri.scheme !== 'vscode-notebook-cell') { - // Doesn't correspond to a notebook cell so do nothing - return - } - - const cell = getCellFromCellDocument(editor.document) - if (cell === undefined) { - return - } - - if (!isGhostCell(cell)) { - return - } - // ...cell.metadata creates a shallow copy of the metadata object - const updatedMetadata = { ...cell.metadata, [ghostKey]: false } - const update = vscode.NotebookEdit.updateCellMetadata(cell.index, updatedMetadata) - const edit = new vscode.WorkspaceEdit() - edit.set(editor.document.uri, [update]) - vscode.workspace.applyEdit(edit).then((result: boolean) => { - log.trace(`updateCellMetadata to deactivate ghostcell resolved with ${result}`) - if (!result) { - log.error('applyEdit failed') - return - } - }) - // If the cell is a ghost cell we want to remove the decoration - // and replace it with a non-ghost cell. - editorAsNonGhost(editor) -} - // n.b. this is a function and not a top level const because that causes problems with the vitest // mocking framework. // N.B. I think we could potentially have solved that by doing something like diff --git a/src/extension/ai/manager.ts b/src/extension/ai/manager.ts index d1ca36429..91fa4d66e 100644 --- a/src/extension/ai/manager.ts +++ b/src/extension/ai/manager.ts @@ -50,7 +50,7 @@ export class AIManager { // When a cell is selected we want to check if its a ghost cell and if so render it a non-ghost cell. this.subscriptions.push( - vscode.window.onDidChangeActiveTextEditor(ghost.handleOnDidChangeActiveTextEditor), + vscode.window.onDidChangeActiveTextEditor(cellGenerator.handleOnDidChangeActiveTextEditor), ) } From e7df2d7e2d58ccdea70a54648b0732ec82363579 Mon Sep 17 00:00:00 2001 From: Jeremy Lewi Date: Sat, 17 Aug 2024 17:56:37 -0700 Subject: [PATCH 2/7] Update foyle version. --- package-lock.json | 18 +++++++++--------- package.json | 4 ++-- src/extension/ai/ghost.ts | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/package-lock.json b/package-lock.json index 3de7e8b2f..c1a1e96b3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15,8 +15,8 @@ "@aws-sdk/client-eks": "^3.630.0", "@aws-sdk/credential-providers": "^3.630.0", "@buf/grpc_grpc.community_timostamm-protobuf-ts": "^2.9.4-20240709201032-5be6b6661793.4", - "@buf/jlewi_foyle.bufbuild_es": "^1.10.0-20240801044500-fba3b5ceb26b.1", - "@buf/jlewi_foyle.connectrpc_es": "^1.4.0-20240801044500-fba3b5ceb26b.3", + "@buf/jlewi_foyle.bufbuild_es": "^1.10.0-20240818004946-86c4c8cbe96c.1", + "@buf/jlewi_foyle.connectrpc_es": "^1.4.0-20240818004946-86c4c8cbe96c.3", "@buf/stateful_runme.community_timostamm-protobuf-ts": "^2.9.4-20240731204114-2df61af8e022.4", "@connectrpc/connect": "^1.4.0", "@connectrpc/connect-node": "^1.1.2", @@ -2418,8 +2418,8 @@ } }, "node_modules/@buf/jlewi_foyle.bufbuild_es": { - "version": "1.10.0-20240801044500-fba3b5ceb26b.1", - "resolved": "https://buf.build/gen/npm/v1/@buf/jlewi_foyle.bufbuild_es/-/jlewi_foyle.bufbuild_es-1.10.0-20240801044500-fba3b5ceb26b.1.tgz", + "version": "1.10.0-20240818004946-86c4c8cbe96c.1", + "resolved": "https://buf.build/gen/npm/v1/@buf/jlewi_foyle.bufbuild_es/-/jlewi_foyle.bufbuild_es-1.10.0-20240818004946-86c4c8cbe96c.1.tgz", "dependencies": { "@buf/bufbuild_protovalidate.bufbuild_es": "1.10.0-20240212200630-3014d81c3a48.1", "@buf/googleapis_googleapis.bufbuild_es": "1.10.0-20240729201550-8bc2c51e08c4.1", @@ -2430,12 +2430,12 @@ } }, "node_modules/@buf/jlewi_foyle.connectrpc_es": { - "version": "1.4.0-20240801044500-fba3b5ceb26b.3", - "resolved": "https://buf.build/gen/npm/v1/@buf/jlewi_foyle.connectrpc_es/-/jlewi_foyle.connectrpc_es-1.4.0-20240801044500-fba3b5ceb26b.3.tgz", + "version": "1.4.0-20240818004946-86c4c8cbe96c.3", + "resolved": "https://buf.build/gen/npm/v1/@buf/jlewi_foyle.connectrpc_es/-/jlewi_foyle.connectrpc_es-1.4.0-20240818004946-86c4c8cbe96c.3.tgz", "dependencies": { "@buf/bufbuild_protovalidate.connectrpc_es": "1.4.0-20240212200630-3014d81c3a48.3", "@buf/googleapis_googleapis.connectrpc_es": "1.4.0-20240729201550-8bc2c51e08c4.3", - "@buf/jlewi_foyle.bufbuild_es": "1.7.2-20240801044500-fba3b5ceb26b.2", + "@buf/jlewi_foyle.bufbuild_es": "1.7.2-20240818004946-86c4c8cbe96c.2", "@buf/stateful_runme.connectrpc_es": "1.4.0-20240731204114-2df61af8e022.3" }, "peerDependencies": { @@ -2457,8 +2457,8 @@ } }, "node_modules/@buf/jlewi_foyle.connectrpc_es/node_modules/@buf/jlewi_foyle.bufbuild_es": { - "version": "1.7.2-20240801044500-fba3b5ceb26b.2", - "resolved": "https://buf.build/gen/npm/v1/@buf/jlewi_foyle.bufbuild_es/-/jlewi_foyle.bufbuild_es-1.7.2-20240801044500-fba3b5ceb26b.2.tgz", + "version": "1.7.2-20240818004946-86c4c8cbe96c.2", + "resolved": "https://buf.build/gen/npm/v1/@buf/jlewi_foyle.bufbuild_es/-/jlewi_foyle.bufbuild_es-1.7.2-20240818004946-86c4c8cbe96c.2.tgz", "dependencies": { "@buf/bufbuild_protovalidate.bufbuild_es": "1.7.2-20240212200630-3014d81c3a48.2", "@buf/googleapis_googleapis.bufbuild_es": "1.7.2-20240729201550-8bc2c51e08c4.2", diff --git a/package.json b/package.json index 55c8e3fb8..a230b8623 100644 --- a/package.json +++ b/package.json @@ -1146,8 +1146,8 @@ "@aws-sdk/client-eks": "^3.630.0", "@aws-sdk/credential-providers": "^3.630.0", "@buf/grpc_grpc.community_timostamm-protobuf-ts": "^2.9.4-20240709201032-5be6b6661793.4", - "@buf/jlewi_foyle.bufbuild_es": "^1.10.0-20240801044500-fba3b5ceb26b.1", - "@buf/jlewi_foyle.connectrpc_es": "^1.4.0-20240801044500-fba3b5ceb26b.3", + "@buf/jlewi_foyle.bufbuild_es": "^1.10.0-20240818004946-86c4c8cbe96c.1", + "@buf/jlewi_foyle.connectrpc_es": "^1.4.0-20240818004946-86c4c8cbe96c.3", "@buf/stateful_runme.community_timostamm-protobuf-ts": "^2.9.4-20240731204114-2df61af8e022.4", "@connectrpc/connect": "^1.4.0", "@connectrpc/connect-node": "^1.1.2", diff --git a/src/extension/ai/ghost.ts b/src/extension/ai/ghost.ts index 58ce39c90..f481ace02 100644 --- a/src/extension/ai/ghost.ts +++ b/src/extension/ai/ghost.ts @@ -1,5 +1,6 @@ import * as vscode from 'vscode' import * as agent_pb from '@buf/jlewi_foyle.bufbuild_es/foyle/v1alpha1/agent_pb' +import { ulid } from 'ulidx' import getLogger from '../logger' import * as serializer from '../serializer' @@ -7,7 +8,6 @@ import * as serializer from '../serializer' import * as converters from './converters' import * as stream from './stream' import * as protos from './protos' -import { ulid } from 'ulidx' const log = getLogger() From 1e1bdd4843f970bae8c9fdacc1997c5da2165f09 Mon Sep 17 00:00:00 2001 From: Jeremy Lewi Date: Sun, 18 Aug 2024 11:15:17 -0700 Subject: [PATCH 3/7] Fix the problem with the callback not being invoked because this wasn't bound properly. --- src/extension/ai/ghost.ts | 12 ++++-------- src/extension/ai/manager.ts | 7 +++++++ 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/extension/ai/ghost.ts b/src/extension/ai/ghost.ts index f481ace02..3cf187d2c 100644 --- a/src/extension/ai/ghost.ts +++ b/src/extension/ai/ghost.ts @@ -105,6 +105,7 @@ export class GhostCellGenerator implements stream.CompletionHandlers { let notebookProto = serializer.GrpcSerializer.marshalNotebook(notebookData) let request = new agent_pb.StreamGenerateRequest({ + contextId: this.contextID, request: { case: 'fullContext', value: new agent_pb.FullContext({ @@ -124,6 +125,7 @@ export class GhostCellGenerator implements stream.CompletionHandlers { let notebook = protos.notebookTSToES(notebookProto) // Generate an update request let request = new agent_pb.StreamGenerateRequest({ + contextId: this.contextID, request: { case: 'update', value: new agent_pb.UpdateContext({ @@ -208,7 +210,7 @@ export class GhostCellGenerator implements stream.CompletionHandlers { // handleOnDidChangeActiveTextEditor updates the ghostKey cell decoration and rendering // when it is selected - handleOnDidChangeActiveTextEditor(editor: vscode.TextEditor | undefined) { + handleOnDidChangeActiveTextEditor = (editor: vscode.TextEditor | undefined) => { const oldCID = this.contextID // We need to generate a new context ID because the context has changed. this.contextID = ulid() @@ -299,11 +301,6 @@ export class CellChangeEventGenerator { return } - log.info( - `onDidChangeTextDocument Fired for notebook ${event.document.uri}` + - 'this should fire when a cell is added to a notebook', - ) - this.streamCreator.handleEvent( new stream.CellChangeEvent(notebook.uri.toString(), matchedCell.index), ) @@ -311,7 +308,7 @@ export class CellChangeEventGenerator { } // handleOnDidChangeVisibleTextEditors is called when the visible text editors change. -// This includes when a TextEditor is created. +// This includes when a TextEditor is created. I also think it can be the result of scrolling. export function handleOnDidChangeVisibleTextEditors(editors: readonly vscode.TextEditor[]) { for (const editor of editors) { log.info(`onDidChangeVisibleTextEditors Fired for editor ${editor.document.uri}`) @@ -326,7 +323,6 @@ export function handleOnDidChangeVisibleTextEditors(editors: readonly vscode.Tex } if (!isGhostCell(cell)) { - log.info(`Not a ghost editor doc:${cell.document.uri}`) continue } diff --git a/src/extension/ai/manager.ts b/src/extension/ai/manager.ts index 91fa4d66e..7fc1cbe83 100644 --- a/src/extension/ai/manager.ts +++ b/src/extension/ai/manager.ts @@ -4,6 +4,7 @@ import getLogger from '../logger' import * as ghost from './ghost' import * as stream from './stream' +import { log } from 'node:console' // AIManager is a class that manages the AI services. export class AIManager { @@ -43,6 +44,7 @@ export class AIManager { ) // onDidChangeVisibleTextEditors fires when the visible text editors change. + // This can happen due to scrolling. // We need to trap this event to apply decorations to turn cells into ghost cells. this.subscriptions.push( vscode.window.onDidChangeVisibleTextEditors(ghost.handleOnDidChangeVisibleTextEditors), @@ -51,6 +53,7 @@ export class AIManager { // When a cell is selected we want to check if its a ghost cell and if so render it a non-ghost cell. this.subscriptions.push( vscode.window.onDidChangeActiveTextEditor(cellGenerator.handleOnDidChangeActiveTextEditor), + // vscode.window.onDidChangeActiveTextEditor(localOnDidChangeActiveTextEditor), ) } @@ -59,3 +62,7 @@ export class AIManager { this.subscriptions.forEach((subscription) => subscription.dispose()) } } + +function localOnDidChangeActiveTextEditor(editor: vscode.TextEditor | undefined) { + getLogger().info('localOnDidChangeActiveTextEditor') +} From f3ee9b15705b6025c15f4904b4b3818e343091e5 Mon Sep 17 00:00:00 2001 From: Jeremy Lewi Date: Sun, 18 Aug 2024 11:42:14 -0700 Subject: [PATCH 4/7] Fix removal of ghost rendering. --- src/extension/ai/ghost.ts | 22 ++++++++++------------ src/extension/ai/manager.ts | 5 ----- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/src/extension/ai/ghost.ts b/src/extension/ai/ghost.ts index 3cf187d2c..d30e6ae15 100644 --- a/src/extension/ai/ghost.ts +++ b/src/extension/ai/ghost.ts @@ -16,6 +16,10 @@ const log = getLogger() // the ghost metadata is not persisted to the markdown file. export const ghostKey = '_ghostCell' +const ghostDecoration = vscode.window.createTextEditorDecorationType({ + color: '#888888', // Light grey color +}) + // TODO(jeremy): How do we handle multiple notebooks? Arguably you should only be generating // completions for the active notebook. So as soon as the active notebook changes we should // stop generating completions for the old notebook and start generating completions for the new notebook. @@ -338,14 +342,18 @@ function editorAsGhost(editor: vscode.TextEditor) { textDoc.positionAt(textDoc.getText().length), ) - editor.setDecorations(getGhostDecoration(), [range]) + editor.setDecorations(ghostDecoration, [range]) } function editorAsNonGhost(editor: vscode.TextEditor) { // To remove the decoration we set the range to an empty range and pass in a reference // to the original decoration // https://github.com/microsoft/vscode-extension-samples/blob/main/decorator-sample/USAGE.md#tips - editor.setDecorations(getGhostDecoration(), []) + // + // Important: ghostDecoration must be a reference to the same object that was used to create the decoration. + // that's how VSCode knows which decoration to remove. If you use a "copy" (i.e. a decoration with the same value) + // the decoration won't get removed. + editor.setDecorations(ghostDecoration, []) } function isGhostCell(cell: vscode.NotebookCell): boolean { @@ -369,13 +377,3 @@ function getCellFromCellDocument(textDoc: vscode.TextDocument): vscode.NotebookC }) return matchedCell } - -// n.b. this is a function and not a top level const because that causes problems with the vitest -// mocking framework. -// N.B. I think we could potentially have solved that by doing something like -// https://github.com/stateful/vscode-runme/pull/1475#issuecomment-2278636467 -function getGhostDecoration(): vscode.TextEditorDecorationType { - return vscode.window.createTextEditorDecorationType({ - color: '#888888', // Light grey color - }) -} diff --git a/src/extension/ai/manager.ts b/src/extension/ai/manager.ts index 7fc1cbe83..7fffece8f 100644 --- a/src/extension/ai/manager.ts +++ b/src/extension/ai/manager.ts @@ -4,7 +4,6 @@ import getLogger from '../logger' import * as ghost from './ghost' import * as stream from './stream' -import { log } from 'node:console' // AIManager is a class that manages the AI services. export class AIManager { @@ -62,7 +61,3 @@ export class AIManager { this.subscriptions.forEach((subscription) => subscription.dispose()) } } - -function localOnDidChangeActiveTextEditor(editor: vscode.TextEditor | undefined) { - getLogger().info('localOnDidChangeActiveTextEditor') -} From 788d885e73995749dfd52e31fcc486541911b61d Mon Sep 17 00:00:00 2001 From: Jeremy Lewi Date: Sun, 18 Aug 2024 11:54:59 -0700 Subject: [PATCH 5/7] Update mocks. --- tests/extension/serializer.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/extension/serializer.test.ts b/tests/extension/serializer.test.ts index 57c2bd771..bd06d934b 100644 --- a/tests/extension/serializer.test.ts +++ b/tests/extension/serializer.test.ts @@ -29,6 +29,7 @@ vi.mock('vscode', () => ({ activeNotebookEditor: undefined, showErrorMessage: vi.fn().mockResolvedValue({}), createOutputChannel: vi.fn(), + createTextEditorDecorationType: vi.fn(), }, Uri: { joinPath: vi.fn().mockReturnValue('/foo/bar') }, workspace: { From 08ec33165e79a021aaac507747ae3c999fd9eee8 Mon Sep 17 00:00:00 2001 From: Jeremy Lewi Date: Mon, 19 Aug 2024 10:59:55 -0700 Subject: [PATCH 6/7] Try adding a resolved value. --- tests/extension/serializer.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/extension/serializer.test.ts b/tests/extension/serializer.test.ts index bd06d934b..fb1dade68 100644 --- a/tests/extension/serializer.test.ts +++ b/tests/extension/serializer.test.ts @@ -29,7 +29,7 @@ vi.mock('vscode', () => ({ activeNotebookEditor: undefined, showErrorMessage: vi.fn().mockResolvedValue({}), createOutputChannel: vi.fn(), - createTextEditorDecorationType: vi.fn(), + createTextEditorDecorationType: vi.fn().mockResolvedValue({}), }, Uri: { joinPath: vi.fn().mockReturnValue('/foo/bar') }, workspace: { From b50d64e39d2be150da30d4eb8dd904835ef889ce Mon Sep 17 00:00:00 2001 From: Jeremy Lewi Date: Tue, 20 Aug 2024 19:53:23 -0700 Subject: [PATCH 7/7] Add createTextEditorDecorationType to mocks to fix tests. --- __mocks__/vscode.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/__mocks__/vscode.ts b/__mocks__/vscode.ts index ce828ce9a..32cd00253 100644 --- a/__mocks__/vscode.ts +++ b/__mocks__/vscode.ts @@ -93,6 +93,7 @@ export const window = { showInformationMessage: vi.fn(), showErrorMessage: vi.fn(), createTerminal: vi.fn().mockReturnValue(terminal), + createTextEditorDecorationType: vi.fn(), showNotebookDocument: vi.fn(), showTextDocument: vi.fn(), onDidChangeActiveNotebookEditor: vi.fn().mockReturnValue({ dispose: vi.fn() }),