Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(auto-edits): fix the partial decoration issue when not enough lines in the editor #6582

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

hitesh-1997
Copy link
Contributor

@hitesh-1997 hitesh-1997 commented Jan 9, 2025

Default auto-edits renderer can not render the decorations properly when there are not enough lines in the editor. The PR adds checks if the deocrator can render the decoration properly, and if it cannot do so we don't create any request for it.

Slack Context
Closes: https://linear.app/sourcegraph/issue/CODY-4656/handle-decorations-when-not-enough-lines-in-the-editor

Example from cody-chat-eval repo

Before (partial decoration rendered):
image

After (decorations will be discarded) - notice discard event on cody by sourcegraph


█ telemetry-v2 recordEvent: cody.autoedit/discarded: {
  "parameters": {
    "version": 0,
    "interactionID": "3ca8f155-1ab9-4b37-ac37-040340fbc1c8",
    "metadata": [
      {
        "key": "otherCompletionProviderEnabled",
        "value": 0
      },
      {
        "key": "triggerKind",
        "value": 1
      },
      {
        "key": "contextSummary.duration",
        "value": 61.69170800000029
      },
      {
        "key": "contextSummary.totalChars",
        "value": 2832
      },
      {
        "key": "contextSummary.prefixChars",
        "value": 979
      },
      {
        "key": "contextSummary.suffixChars",
        "value": 57
      },
      {
        "key": "contextSummary.retrieverStats.diagnostics.suggestedItems",
        "value": 9
      },
      {
        "key": "contextSummary.retrieverStats.diagnostics.positionBitmap",
        "value": 511
      },
      {
        "key": "contextSummary.retrieverStats.diagnostics.retrieverChars",
        "value": 1796
      },
      {
        "key": "contextSummary.retrieverStats.diagnostics.retrievedItems",
        "value": 9
      },
      {
        "key": "contextSummary.retrieverStats.diagnostics.duration",
        "value": 51.03662500000064
      },
      {
        "key": "source",
        "value": 1
      },
      {
        "key": "isFuzzyMatch",
        "value": 0
      },
      {
        "key": "latency",
        "value": 68
      },
      {
        "key": "decorationStats.modifiedLines",
        "value": 1
      },
      {
        "key": "decorationStats.removedLines",
        "value": 0
      },
      {
        "key": "decorationStats.addedLines",
        "value": 4
      },
      {
        "key": "decorationStats.unchangedLines",
        "value": 40
      },
      {
        "key": "decorationStats.addedChars",
        "value": 104
      },
      {
        "key": "decorationStats.removedChars",
        "value": 45
      },
      {
        "key": "decorationStats.unchangedChars",
        "value": 951
      },
      **{
        "key": "discardReason",
        "value": 9
      },**
      {
        "key": "recordsPrivateMetadataTranscript",
        "value": 1
      }
    ],
    "privateMetadata": {
      "otherCompletionProviders": [],
      "languageId": "typescript",
      "model": "fireworks::v1::autoedits-deepseek-lite-default",
      "contextSummary": {
        "strategy": "auto-edits",
        "duration": 61.69170800000029,
        "totalChars": 2832,
        "prefixChars": 979,
        "suffixChars": 57,
        "retrieverStats": {
          "diagnostics": {
            "suggestedItems": 9,
            "positionBitmap": 511,
            "retrieverChars": 1796,
            "retrievedItems": 9,
            "duration": 51.03662500000064
          }
        }
      },
      "id": "3ca8f155-1ab9-4b37-ac37-040340fbc1c8",
      "responseHeaders": {},
      "decorationStats": {
        "modifiedLines": 1,
        "removedLines": 0,
        "addedLines": 4,
        "unchangedLines": 40,
        "addedChars": 104,
        "removedChars": 45,
        "unchangedChars": 951
      }
    },
    "billingMetadata": {
      "product": "cody",
      "category": "core"
    }
  },
  "timestamp": "2025-01-10T09:33:03.486Z"
}

Test plan

Tested on all the cody-chat-eval examples.

@@ -34,9 +60,6 @@ export class DefaultDecorator implements AutoEditsDecorator {
before: { color: GHOST_TEXT_COLOR },
after: { color: GHOST_TEXT_COLOR },
})
this.hideRemainderDecorationType = vscode.window.createTextEditorDecorationType({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was redundant and was not getting used anywhere.

Comment on lines 44 to 47
/**
* Pre-computed information about diff decorations to be applied to lines in the editor
*/

private diffDecorationInfo: DiffDecorationInfo | undefined
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* Pre-computed information about diff decorations to be applied to lines in the editor
*/
private diffDecorationInfo: DiffDecorationInfo | undefined
/**
* Pre-computed information about diff decorations to be applied to lines in the editor
*/
private diffDecorationInfo: DiffDecorationInfo | undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 192 to 205
this.decorator = null
return
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a new discardReason and emit a discard event here:

Suggested change
this.decorator = null
return
this.decorator = null
autoeditAnalyticsLogger.markAsDiscarded({
requestId,
discardReason: autoeditDiscardReason.noEnoughLines,
})
return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (
'decorationInfo' in request &&
request.decorationInfo &&
!this.decorator.canRenderDecoration(request.decorationInfo)
Copy link
Member

Choose a reason for hiding this comment

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

Can we untether this check from the decorator's instance so that we don't have to create and dispose of it later if we don't have enough lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah the flow doesn't look good that we only create decorator only to dispose it on some condition.
I though about it when first tried to implement this, As per the current implementation, the manager doesn't know the type of decorator it is using and canRenderDecoration should be specific to a decorator, for example image renderer should have issues which our default renderer has, so making it a instance specific method seemed like the right call.
Please let me know if there are any alternate implementation ideas, would be happy to switch to them :)

@hitesh-1997 hitesh-1997 force-pushed the hitesh/fix-eol-decorations branch from b0fdfcc to 4f6c6aa Compare January 10, 2025 09:50
@@ -86,7 +86,7 @@ const validRequestTransitions = {
started: ['contextLoaded', 'discarded'],
contextLoaded: ['loaded', 'discarded'],
loaded: ['postProcessed', 'discarded'],
postProcessed: ['suggested'],
postProcessed: ['suggested', 'discarded'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@valerybugakov added another transition state, since conflictingDecorations and default-rendere can discard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants