Skip to content

Commit

Permalink
Fixes #2117 new limits/controls on failed requests
Browse files Browse the repository at this point in the history 
 - Changes request failures to only disconnect the integration for the current session
 - Limits to 5 failed request before disconnecting the integration
 - Avoids disconnect prompt for current session disconnections
 - Adds don't show again to request failure messages
 - Adds integration suspension message w/ don't show again option
  • Loading branch information
@eamodio
eamodio committed Aug 2, 2022
1 parent adec1ad commit b613f71
Show file tree
Hide file tree
Showing 7 changed files with 552 additions and 272 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Expand Up @@ -202,6 +202,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p

### Fixed

- Fixes [#2117](https://github.com/gitkraken/vscode-gitlens/issues/2117) - Gitlens freaks out when I'm off VPN
- Fixes [#1859](https://github.com/gitkraken/vscode-gitlens/issues/1859) - GitLens dates use system locale instead of vscode language setting
- Fixes [#1854](https://github.com/gitkraken/vscode-gitlens/issues/1854) - All repos have the same name
- Fixes [#1866](https://github.com/gitkraken/vscode-gitlens/issues/1866) - Copy SHA and Copy Message don't work from the views (commits, branches, etc)
Expand Down
20 changes: 19 additions & 1 deletion package.json
View file
Expand Up @@ -3011,7 +3011,10 @@
"suppressGitVersionWarning": false,
"suppressLineUncommittedWarning": false,
"suppressNoRepositoryWarning": false,
"suppressRebaseSwitchToTextWarning": false
"suppressRebaseSwitchToTextWarning": false,
"suppressIntegrationDisconnectedTooManyFailedRequestsWarning": false,
"suppressIntegrationRequestFailed500Warning": false,
"suppressIntegrationRequestTimedOutWarning": false
},
"properties": {
"suppressCommitHasNoPreviousCommitWarning": {
Expand Down Expand Up @@ -3068,6 +3071,21 @@
"type": "boolean",
"default": false,
"description": "Rebase Switch To Text Warning"
},
"suppressIntegrationDisconnectedTooManyFailedRequestsWarning": {
"type": "boolean",
"default": false,
"description": "Integration Disconnected; Too Many Failed Requests Warning"
},
"suppressIntegrationRequestFailed500Warning": {
"type": "boolean",
"default": false,
"description": "Integration Request Failed (500 status code) Warning"
},
"suppressIntegrationRequestTimedOutWarning": {
"type": "boolean",
"default": false,
"description": "Integration Request Timed Out Warning"
}
},
"additionalProperties": false,
Expand Down
32 changes: 19 additions & 13 deletions src/config.ts
View file
Expand Up @@ -363,19 +363,7 @@ export interface AdvancedConfig {
fileHistoryShowAllBranches: boolean;
maxListItems: number;
maxSearchItems: number;
messages: {
suppressCommitHasNoPreviousCommitWarning: boolean;
suppressCommitNotFoundWarning: boolean;
suppressCreatePullRequestPrompt: boolean;
suppressDebugLoggingWarning: boolean;
suppressFileNotUnderSourceControlWarning: boolean;
suppressGitDisabledWarning: boolean;
suppressGitMissingWarning: boolean;
suppressGitVersionWarning: boolean;
suppressLineUncommittedWarning: boolean;
suppressNoRepositoryWarning: boolean;
suppressRebaseSwitchToTextWarning: boolean;
};
messages: { [key in SuppressedMessages]: boolean };
quickPick: {
closeOnFocusOut: boolean;
};
Expand Down Expand Up @@ -510,6 +498,24 @@ export interface RemotesUrlsConfig {
fileRange: string;
}

// NOTE: Must be kept in sync with `gitlens.advanced.messages` setting in the package.json
export const enum SuppressedMessages {
CommitHasNoPreviousCommitWarning = 'suppressCommitHasNoPreviousCommitWarning',
CommitNotFoundWarning = 'suppressCommitNotFoundWarning',
CreatePullRequestPrompt = 'suppressCreatePullRequestPrompt',
SuppressDebugLoggingWarning = 'suppressDebugLoggingWarning',
FileNotUnderSourceControlWarning = 'suppressFileNotUnderSourceControlWarning',
GitDisabledWarning = 'suppressGitDisabledWarning',
GitMissingWarning = 'suppressGitMissingWarning',
GitVersionWarning = 'suppressGitVersionWarning',
LineUncommittedWarning = 'suppressLineUncommittedWarning',
NoRepositoryWarning = 'suppressNoRepositoryWarning',
RebaseSwitchToTextWarning = 'suppressRebaseSwitchToTextWarning',
IntegrationDisconnectedTooManyFailedRequestsWarning = 'suppressIntegrationDisconnectedTooManyFailedRequestsWarning',
IntegrationRequestFailed500Warning = 'suppressIntegrationRequestFailed500Warning',
IntegrationRequestTimedOutWarning = 'suppressIntegrationRequestTimedOutWarning',
}

export interface ViewsCommonConfig {
defaultItemLimit: number;
formats: {
Expand Down
120 changes: 67 additions & 53 deletions src/git/remotes/provider.ts
View file
Expand Up @@ -18,6 +18,7 @@ import { configuration } from '../../configuration';
import { Container } from '../../container';
import { AuthenticationError, ProviderRequestClientError } from '../../errors';
import { Logger } from '../../logger';
import { Messages } from '../../messages';
import type { IntegrationAuthenticationSessionDescriptor } from '../../plus/integrationAuthentication';
import { WorkspaceStorageKeys } from '../../storage';
import { gate } from '../../system/decorators/gate';
Expand Down Expand Up @@ -288,8 +289,6 @@ export abstract class RichRemoteProvider extends RemoteProvider {
return this._onDidChange.event;
}

private invalidClientExceptionCount = 0;

constructor(domain: string, path: string, protocol?: string, name?: string, custom?: boolean) {
super(domain, path, protocol, name, custom);

Expand All @@ -304,7 +303,7 @@ export abstract class RichRemoteProvider extends RemoteProvider {
if (e.key !== this.key) return;

if (e.reason === 'disconnected') {
void this.disconnect(true);
void this.disconnect({ silent: true });
} else if (e.reason === 'connected') {
void this.ensureSession(false);
}
Expand Down Expand Up @@ -359,49 +358,58 @@ export abstract class RichRemoteProvider extends RemoteProvider {

@gate()
@log()
async disconnect(silent: boolean = false): Promise<void> {
async disconnect(options?: { silent?: boolean; currentSessionOnly?: boolean }): Promise<void> {
if (options?.currentSessionOnly && this._session === null) return;

const connected = this._session != null;

const container = Container.instance;

if (connected && !silent) {
const disable = { title: 'Disable' };
const signout = { title: 'Disable & Sign Out' };
const cancel = { title: 'Cancel', isCloseAffordance: true };

let result: MessageItem | undefined;
if (container.integrationAuthentication.hasProvider(this.authProvider.id)) {
result = await window.showWarningMessage(
`Are you sure you want to disable the rich integration with ${this.name}?\n\nNote: signing out clears the saved authentication.`,
{ modal: true },
disable,
signout,
cancel,
);
if (connected && !options?.silent) {
if (options?.currentSessionOnly) {
void Messages.showIntegrationDisconnectedTooManyFailedRequestsWarningMessage(this.name);
} else {
result = await window.showWarningMessage(
`Are you sure you want to disable the rich integration with ${this.name}?`,
{ modal: true },
disable,
cancel,
);
}
const disable = { title: 'Disable' };
const signout = { title: 'Disable & Sign Out' };
const cancel = { title: 'Cancel', isCloseAffordance: true };

let result: MessageItem | undefined;
if (container.integrationAuthentication.hasProvider(this.authProvider.id)) {
result = await window.showWarningMessage(
`Are you sure you want to disable the rich integration with ${this.name}?\n\nNote: signing out clears the saved authentication.`,
{ modal: true },
disable,
signout,
cancel,
);
} else {
result = await window.showWarningMessage(
`Are you sure you want to disable the rich integration with ${this.name}?`,
{ modal: true },
disable,
cancel,
);
}

if (result == null || result === cancel) return;
if (result === signout) {
void Container.instance.integrationAuthentication.deleteSession(this.id, this.authProviderDescriptor);
if (result == null || result === cancel) return;
if (result === signout) {
void container.integrationAuthentication.deleteSession(this.id, this.authProviderDescriptor);
}
}
}

this.invalidClientExceptionCount = 0;
this.resetRequestExceptionCount();
this._prsByCommit.clear();
this._session = null;

if (connected) {
void container.storage.storeWorkspace(this.connectedKey, false);
// Don't store the disconnected flag if this only for this current VS Code session (will be re-connected on next restart)
if (!options?.currentSessionOnly) {
void container.storage.storeWorkspace(this.connectedKey, false);
}

this._onDidChange.fire();
if (!silent) {
if (!options?.silent && !options?.currentSessionOnly) {
fireAuthenticationChanged(this.key, 'disconnected');
}
}
Expand All @@ -415,6 +423,21 @@ export abstract class RichRemoteProvider extends RemoteProvider {
void (await this.ensureSession(true, true));
}

private requestExceptionCount = 0;

resetRequestExceptionCount() {
this.requestExceptionCount = 0;
}

@debug()
trackRequestException() {
this.requestExceptionCount++;

if (this.requestExceptionCount >= 5 && this._session !== null) {
void this.disconnect({ currentSessionOnly: true });
}
}

@gate()
@debug<RichRemoteProvider['isConnected']>({
exit: connected => `returned ${connected}`,
Expand All @@ -438,13 +461,13 @@ export abstract class RichRemoteProvider extends RemoteProvider {

try {
const author = await this.getProviderAccountForCommit(this._session!, ref, options);
this.invalidClientExceptionCount = 0;
this.resetRequestExceptionCount();
return author;
} catch (ex) {
Logger.error(ex, cc);

if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) {
this.handleClientException();
this.trackRequestException();
}
return undefined;
}
Expand Down Expand Up @@ -473,13 +496,13 @@ export abstract class RichRemoteProvider extends RemoteProvider {

try {
const author = await this.getProviderAccountForEmail(this._session!, email, options);
this.invalidClientExceptionCount = 0;
this.resetRequestExceptionCount();
return author;
} catch (ex) {
Logger.error(ex, cc);

if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) {
this.handleClientException();
this.trackRequestException();
}
return undefined;
}
Expand All @@ -503,13 +526,13 @@ export abstract class RichRemoteProvider extends RemoteProvider {

try {
const defaultBranch = await this.getProviderDefaultBranch(this._session!);
this.invalidClientExceptionCount = 0;
this.resetRequestExceptionCount();
return defaultBranch;
} catch (ex) {
Logger.error(ex, cc);

if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) {
this.handleClientException();
this.trackRequestException();
}
return undefined;
}
Expand All @@ -529,13 +552,13 @@ export abstract class RichRemoteProvider extends RemoteProvider {

try {
const issueOrPullRequest = await this.getProviderIssueOrPullRequest(this._session!, id);
this.invalidClientExceptionCount = 0;
this.resetRequestExceptionCount();
return issueOrPullRequest;
} catch (ex) {
Logger.error(ex, cc);

if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) {
this.handleClientException();
this.trackRequestException();
}
return undefined;
}
Expand Down Expand Up @@ -578,13 +601,13 @@ export abstract class RichRemoteProvider extends RemoteProvider {

try {
const pr = await this.getProviderPullRequestForBranch(this._session!, branch, options);
this.invalidClientExceptionCount = 0;
this.resetRequestExceptionCount();
return pr;
} catch (ex) {
Logger.error(ex, cc);

if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) {
this.handleClientException();
this.trackRequestException();
}
return undefined;
}
Expand Down Expand Up @@ -622,15 +645,15 @@ export abstract class RichRemoteProvider extends RemoteProvider {
try {
const pr = (await this.getProviderPullRequestForCommit(this._session!, ref)) ?? null;
this._prsByCommit.set(ref, pr);
this.invalidClientExceptionCount = 0;
this.resetRequestExceptionCount();
return pr;
} catch (ex) {
Logger.error(ex, cc);

this._prsByCommit.delete(ref);

if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) {
this.handleClientException();
this.trackRequestException();
}
return null;
}
Expand Down Expand Up @@ -689,7 +712,7 @@ export abstract class RichRemoteProvider extends RemoteProvider {
}

this._session = session ?? null;
this.invalidClientExceptionCount = 0;
this.resetRequestExceptionCount();

if (session != null) {
await container.storage.storeWorkspace(this.connectedKey, true);
Expand All @@ -702,13 +725,4 @@ export abstract class RichRemoteProvider extends RemoteProvider {

return session ?? undefined;
}

@debug()
private handleClientException() {
this.invalidClientExceptionCount++;

if (this.invalidClientExceptionCount >= 5) {
void this.disconnect();
}
}
}

0 comments on commit b613f71

Please sign in to comment.