Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
last feedback
  • Loading branch information
akhera99 committed Mar 27, 2025
commit f9d1800fef10698c1a5bda21ddb0a934652c6c39
Original file line number Diff line number Diff line change
Expand Up @@ -70,22 +70,15 @@ public async Task GenerateDocumentationProposalAsync(DocumentationCommentSnippet
var intelliCodeLineCompletionsDisposable = await _suggestionManager.DisableProviderAsync(SuggestionServiceNames.IntelliCodeLineCompletions, cancellationToken).ConfigureAwait(false);

var suggestion = new DocumentationCommentSuggestion(this, _suggestionManager, intelliCodeLineCompletionsDisposable);
var suggestionSessionStarted = await suggestion.StartSuggestionSessionAsync(cancellationToken).ConfigureAwait(false);
if (!suggestionSessionStarted)
Func<CancellationToken, Task<ProposalBase?>> generateProposal = async (cancellationToken) =>
{
return;
}

var proposalEdits = await GetProposedEditsAsync(snippetProposal, _copilotService, oldSnapshot, snippet.IndentText, cancellationToken).ConfigureAwait(false);
var proposalEdits = await GetProposedEditsAsync(
snippetProposal, _copilotService, oldSnapshot, snippet.IndentText, cancellationToken).ConfigureAwait(false);

var proposal = Proposal.TryCreateProposal(description: null, proposalEdits, oldCaret, flags: ProposalFlags.ShowCommitHighlight);
if (proposal is null)
{
await suggestion.DismissSuggestionSessionAsync(cancellationToken).ConfigureAwait(false);
return;
}
return Proposal.TryCreateProposal(description: null, proposalEdits, oldCaret, flags: ProposalFlags.ShowCommitHighlight);
};

await suggestion.TryDisplayDocumentationSuggestionAsync(proposal, cancellationToken).ConfigureAwait(false);
await suggestion.StartSuggestionSessionWithProposalAsync(generateProposal, cancellationToken).ConfigureAwait(false);
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public override async Task OnAcceptedAsync(SuggestionSessionBase session, Propos
var threadingContext = providerInstance.ThreadingContext;

await threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancel);
await DisposeAsync().ConfigureAwait(false);
await DisposeIntelliCodeCompletionsDisposableAsync().ConfigureAwait(false);
Logger.Log(FunctionId.Copilot_Generate_Documentation_Accepted, logLevel: LogLevel.Information);
}

Expand Down Expand Up @@ -66,14 +66,33 @@ public override Task OnProposalUpdatedAsync(SuggestionSessionBase session, Propo
return Task.CompletedTask;
}

public async Task StartSuggestionSessionWithProposalAsync(
Func<CancellationToken, Task<ProposalBase?>> generateProposal, CancellationToken cancellationToken)
{
var sessionStarted = await StartSuggestionSessionAsync(cancellationToken).ConfigureAwait(false);
if (!sessionStarted)
{
return;
}

var proposal = await generateProposal(cancellationToken).ConfigureAwait(false);
if (proposal is null)
{
await DismissSuggestionSessionAsync(cancellationToken).ConfigureAwait(false);
return;
}

await TryDisplayDocumentationSuggestionAsync(proposal, cancellationToken).ConfigureAwait(false);
}

/// <summary>
/// Starts the Suggestion Session. The TryDisplaySuggestion call doesn't display any grey text, but starts the session such that we have the
/// exclusive right to display grey text later.
/// </summary>
/// <returns>If true, user will see the thinking state as long as the Suggestion Session is active and replace with grey text if a call to DisplayProposal succeeds.
/// If unable to retrieve the session, the caller should bail out.
/// </returns>
public async Task<bool> StartSuggestionSessionAsync(CancellationToken cancellationToken)
private async Task<bool> StartSuggestionSessionAsync(CancellationToken cancellationToken)
{
_suggestionSession = await RunWithEnqueueActionAsync(
"StartWork",
Expand All @@ -82,13 +101,17 @@ public async Task<bool> StartSuggestionSessionAsync(CancellationToken cancellati

if (_suggestionSession is null)
{
await DisposeAsync().ConfigureAwait(false);
await DisposeIntelliCodeCompletionsDisposableAsync().ConfigureAwait(false);
return false;
}

return true;
}

/// <summary>
/// This is where we actually try to display the grey-text from the proposal
/// we created.
/// </summary>
public async Task TryDisplayDocumentationSuggestionAsync(ProposalBase proposal, CancellationToken cancellationToken)
{
try
Expand All @@ -112,10 +135,12 @@ await RunWithEnqueueActionAsync<bool>(

/// <summary>
/// Dismisses the session if the proposal we generated was invalid.
/// Needs to dispose of the IntelliCodeCompletionsDisposable so we no longer have exclusive right to
/// display any grey text.
/// </summary>
public async Task DismissSuggestionSessionAsync(CancellationToken cancellationToken)
private async Task DismissSuggestionSessionAsync(CancellationToken cancellationToken)
{
await DisposeAsync().ConfigureAwait(false);
await DisposeIntelliCodeCompletionsDisposableAsync().ConfigureAwait(false);
await RunWithEnqueueActionAsync<bool>(
"DismissSuggestionSession",
async () =>
Expand All @@ -141,7 +166,6 @@ private async Task<T> RunWithEnqueueActionAsync<T>(string description, Func<Task
await providerInstance.ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does SuggestionManager.EnqueueAction require enqueuing to take place on the main thread? As a scheduler, that's a weird requirement. And if it isn't a requirement, this seems wasteful to do. If you can remove the switch, you can make this whole method non-async, which improves efficiency.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe so based on the pattern in the platform, but @dpugh would know more about this particular requirement.

SuggestionManager.EnqueueAction(description, async () =>
Copy link
Member

Choose a reason for hiding this comment

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

this is super weird. they expect you to pass in an async function. but then they dont' make this function itself task-returning so you can track it?

Copy link
Member Author

Choose a reason for hiding this comment

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

talked offline

{
var task = action();
try
{
var result = await action().ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since all you do here is mark a TCS complete, you can potentially improve efficiency by using ConfigureAwaitRunInline here instead.
Just be sure to combine it with my other suggestion to prevent other continuations from being inlined.

Expand All @@ -168,14 +192,14 @@ private async Task ClearSuggestionAsync(ReasonForDismiss reason, CancellationTok
}

_suggestionSession = null;
await DisposeAsync().ConfigureAwait(false);
await DisposeIntelliCodeCompletionsDisposableAsync().ConfigureAwait(false);
}

/// <summary>
/// The IntelliCodeLineCompletionDisposable needs to be disposed any time we exit the SuggestionSession so that
/// line completions can be shown again.
/// </summary>
private async Task DisposeAsync()
private async Task DisposeIntelliCodeCompletionsDisposableAsync()
{
if (IntelliCodeLineCompletionsDisposable != null)
{
Expand Down
Loading