Skip to content

Commit 002e661

Browse files
Strum355Strum355
authored
feat: improve UX for error messages in logs & notifications (#801)
1 parent 6cfda8f commit 002e661

File tree

10 files changed

+109
-72
lines changed

10 files changed

+109
-72
lines changed

‎.eslintrc.js‎

Lines changed: 44 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,57 +1,58 @@
1+
/* eslint-disable @typescript-eslint/naming-convention */
12
module.exports = {
2-
"root": true,
3-
"parser": "@typescript-eslint/parser",
4-
"plugins": ["@typescript-eslint"],
5-
"extends": [
6-
"eslint:recommended",
7-
"plugin:@typescript-eslint/eslint-recommended",
8-
"plugin:@typescript-eslint/recommended"
3+
'root': true,
4+
'parser': '@typescript-eslint/parser',
5+
'plugins': ['@typescript-eslint'],
6+
'extends': [
7+
'eslint:recommended',
8+
'plugin:@typescript-eslint/eslint-recommended',
9+
'plugin:@typescript-eslint/recommended'
910
],
10-
"rules": {
11-
"@typescript-eslint/no-explicit-any": "off",
12-
"@typescript-eslint/no-require-imports": "error",
13-
"@typescript-eslint/no-unused-expressions": "error",
14-
"@typescript-eslint/naming-convention": [
15-
"error",
11+
'rules': {
12+
'@typescript-eslint/no-explicit-any': 'off',
13+
'@typescript-eslint/no-require-imports': 'error',
14+
'@typescript-eslint/no-unused-expressions': 'error',
15+
'@typescript-eslint/naming-convention': [
16+
'error',
1617
{
17-
"selector": "default",
18-
"format": ["camelCase"]
18+
'selector': 'default',
19+
'format': ['camelCase']
1920
},
2021
{
21-
"selector": ["class", "interface", "enum"],
22-
"format": ["PascalCase"]
22+
'selector': ['class', 'interface', 'enum'],
23+
'format': ['PascalCase']
2324
},
2425
{
25-
"selector": ["enumMember", "variable", "property", "method"],
26-
"format": ["UPPER_CASE", "camelCase"],
27-
"leadingUnderscore": "allow"
26+
'selector': ['enumMember', 'variable', 'property', 'method'],
27+
'format': ['UPPER_CASE', 'camelCase'],
28+
'leadingUnderscore': 'allow'
2829
}
2930
],
30-
"@typescript-eslint/semi": ["error", "always"],
31-
"@typescript-eslint/quotes": [
32-
"error",
33-
"single",
31+
'@typescript-eslint/semi': ['error', 'always'],
32+
'@typescript-eslint/quotes': [
33+
'error',
34+
'single',
3435
{
35-
"allowTemplateLiterals": true,
36-
"avoidEscape": true
36+
'allowTemplateLiterals': true,
37+
'avoidEscape': true
3738
}
3839
],
39-
"@typescript-eslint/no-shadow": "error",
40-
"@typescript-eslint/no-redeclare": "error",
41-
"no-async-promise-executor": "off",
42-
"no-redeclare": "off",
43-
"no-duplicate-case": "error",
44-
"no-shadow": "off",
45-
"curly": "error",
46-
"semi": "off",
47-
"eqeqeq": ["error", "always"],
48-
"quotes": "off",
49-
"no-debugger": "error",
50-
"no-empty": "error",
51-
"no-var": "error",
52-
"no-unsafe-finally": "error",
53-
"new-parens": "error",
54-
"no-throw-literal": "error",
55-
"no-useless-catch": "off"
40+
'@typescript-eslint/no-shadow': 'error',
41+
'@typescript-eslint/no-redeclare': 'error',
42+
'no-async-promise-executor': 'off',
43+
'no-redeclare': 'off',
44+
'no-duplicate-case': 'error',
45+
'no-shadow': 'off',
46+
'curly': 'error',
47+
'semi': 'off',
48+
'eqeqeq': ['warn', 'always', { 'null': 'never' }],
49+
'quotes': 'off',
50+
'no-debugger': 'error',
51+
'no-empty': 'error',
52+
'no-var': 'error',
53+
'no-unsafe-finally': 'error',
54+
'new-parens': 'error',
55+
'no-throw-literal': 'error',
56+
'no-useless-catch': 'off'
5657
}
5758
}

‎src/caNotification.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ class CANotification {
140140
*/
141141
public popupText(): string {
142142
const text: string = Array.from(this.vulnCount.entries())
143-
.map(([provider, vulnCount]) => `Found ${vulnCount} direct ${this.singularOrPlural(vulnCount)} for ${this.capitalizeEachWord(provider)} Provider.`)
143+
.map(([provider, vulnCount]) => `Found ${vulnCount} direct ${this.singularOrPlural(vulnCount)} in ${this.uri.fsPath} for ${this.capitalizeEachWord(provider)} Provider.`)
144144
.join(' ');
145145
return text || this.warningText().replace(/\$\((.*?)\)/g, '');
146146
}

‎src/dependencyAnalysis/diagnostics.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { executeComponentAnalysis, DependencyData } from './analysis';
1010
import { Vulnerability } from '../vulnerability';
1111
import { VERSION_PLACEHOLDER, GRADLE } from '../constants';
1212
import { clearCodeActionsMap, registerCodeAction, generateSwitchToRecommendedVersionAction } from '../codeActionHandler';
13-
import { buildErrorMessage } from '../utils';
13+
import { buildLogErrorMessage, buildNotificationErrorMessage } from '../utils';
1414
import { AbstractDiagnosticsPipeline } from '../diagnosticsPipeline';
1515
import { Diagnostic, DiagnosticSeverity, Uri } from 'vscode';
1616
import { notifications, outputChannelDep } from '../extension';
@@ -112,9 +112,9 @@ async function performDiagnostics(diagnosticFilePath: Uri, contents: string, pro
112112

113113
diagnosticsPipeline.reportDiagnostics();
114114
} catch (error) {
115-
outputChannelDep.warn(`component analysis error: ${buildErrorMessage(error as Error)}\n${(error as Error).stack}`);
115+
outputChannelDep.warn(`component analysis error: ${buildLogErrorMessage(error as Error)}`);
116116
notifications.emit('caError', {
117-
errorMessage: (error as Error).message,
117+
errorMessage: buildNotificationErrorMessage(error as Error),
118118
uri: diagnosticFilePath,
119119
});
120120
}

‎src/extension.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import { caStatusBarProvider } from './caStatusBarProvider';
1212
import { CANotification, CANotificationData } from './caNotification';
1313
import { DepOutputChannel } from './depOutputChannel';
1414
import { record, startUp, TelemetryActions } from './redhatTelemetry';
15-
import { applySettingNameMappings, buildErrorMessage } from './utils';
15+
import { applySettingNameMappings, buildLogErrorMessage } from './utils';
1616
import { clearCodeActionsMap, getDiagnosticsCodeActions } from './codeActionHandler';
1717
import { AnalysisMatcher } from './fileHandler';
1818
import { EventEmitter } from 'node:events';
@@ -67,9 +67,10 @@ export async function activate(context: vscode.ExtensionContext) {
6767
await generateRHDAReport(context, fspath, outputChannelDep);
6868
record(context, TelemetryActions.vulnerabilityReportDone, { manifest: fileName, fileName: fileName });
6969
} catch (error) {
70+
// TODO: dont show raw message
7071
const message = applySettingNameMappings((error as Error).message);
71-
vscode.window.showErrorMessage(message);
72-
outputChannelDep.error(buildErrorMessage((error as Error)));
72+
vscode.window.showErrorMessage(`RHDA error while analyzing ${filePath}: ${message}`);
73+
outputChannelDep.error(buildLogErrorMessage((error as Error)));
7374
record(context, TelemetryActions.vulnerabilityReportFailed, { manifest: fileName, fileName: fileName, error: message });
7475
}
7576
}
@@ -220,8 +221,8 @@ function registerStackAnalysisCommands(context: vscode.ExtensionContext) {
220221
record(context, TelemetryActions.vulnerabilityReportDone, { manifest: fileName, fileName: fileName });
221222
} catch (error) {
222223
const message = applySettingNameMappings((error as Error).message);
223-
vscode.window.showErrorMessage(message);
224-
outputChannelDep.error(buildErrorMessage((error as Error)));
224+
vscode.window.showErrorMessage(`RHDA error while analyzing ${filePath}: ${message}`);
225+
outputChannelDep.error(buildLogErrorMessage((error as Error)));
225226
record(context, TelemetryActions.vulnerabilityReportFailed, { manifest: fileName, fileName: fileName, error: message });
226227
}
227228
};

‎src/fileHandler.ts‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,44 +11,44 @@ import { ImageProvider as Docker } from './providers/docker';
1111
import { outputChannelDep } from './extension';
1212

1313
export class AnalysisMatcher {
14-
matchers: Array<{ scheme: string, pattern: RegExp, callback: (path: Uri, contents: string) => void }> = [
14+
matchers: Array<{ scheme: string, pattern: RegExp, callback: (path: Uri, contents: string) => Promise<void> }> = [
1515
{
1616
scheme: 'file', pattern: new RegExp('^package\\.json$'), callback: (path: Uri, contents: string) => {
17-
dependencyDiagnostics.performDiagnostics(path, contents, new PackageJson());
17+
return dependencyDiagnostics.performDiagnostics(path, contents, new PackageJson());
1818
}
1919
},
2020
{
2121
scheme: 'file', pattern: new RegExp('^pom\\.xml$'), callback: (path: Uri, contents: string) => {
22-
dependencyDiagnostics.performDiagnostics(path, contents, new PomXml());
22+
return dependencyDiagnostics.performDiagnostics(path, contents, new PomXml());
2323
}
2424
},
2525
{
2626
scheme: 'file', pattern: new RegExp('^go\\.mod$'), callback: (path: Uri, contents: string) => {
27-
dependencyDiagnostics.performDiagnostics(path, contents, new GoMod());
27+
return dependencyDiagnostics.performDiagnostics(path, contents, new GoMod());
2828
}
2929
},
3030
{
3131
scheme: 'file', pattern: new RegExp('^requirements\\.txt$'), callback: (path: Uri, contents: string) => {
32-
dependencyDiagnostics.performDiagnostics(path, contents, new RequirementsTxt());
32+
return dependencyDiagnostics.performDiagnostics(path, contents, new RequirementsTxt());
3333
}
3434
},
3535
{
3636
scheme: 'file', pattern: new RegExp('^build\\.gradle$'), callback: (path: Uri, contents: string) => {
37-
dependencyDiagnostics.performDiagnostics(path, contents, new BuildGradle());
37+
return dependencyDiagnostics.performDiagnostics(path, contents, new BuildGradle());
3838
}
3939
},
4040
{
4141
scheme: 'file', pattern: new RegExp('^(Dockerfile|Containerfile)$'), callback: (path: Uri, contents: string) => {
42-
imageDiagnostics.performDiagnostics(path, contents, new Docker());
42+
return imageDiagnostics.performDiagnostics(path, contents, new Docker());
4343
}
4444
}
4545
];
4646

47-
handle(doc: TextDocument) {
47+
async handle(doc: TextDocument) {
4848
for (const matcher of this.matchers) {
4949
if (matcher.pattern.test(basename(doc.fileName))) {
5050
outputChannelDep.info(`generating component analysis diagnostics for "${doc.fileName}"`);
51-
matcher.callback(doc.uri, doc.getText());
51+
await matcher.callback(doc.uri, doc.getText());
5252
outputChannelDep.info(`done generating component analysis diagnostics for "${doc.fileName}"`);
5353
}
5454
}

‎src/imageAnalysis.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { imageAnalysisService } from './exhortServices';
88
import { StatusMessages, Titles } from './constants';
99
import { Options } from '@trustification/exhort-javascript-api';
1010
import { updateCurrentWebviewPanel } from './rhda';
11-
import { buildErrorMessage } from './utils';
11+
import { buildLogErrorMessage } from './utils';
1212
import { DepOutputChannel } from './depOutputChannel';
1313

1414
/**
@@ -190,7 +190,7 @@ class DockerImageAnalysis {
190190

191191
updateCurrentWebviewPanel('error');
192192

193-
this.outputChannel.error(buildErrorMessage(error as Error));
193+
this.outputChannel.error(buildLogErrorMessage(error as Error));
194194

195195
throw error;
196196
}

‎src/imageAnalysis/diagnostics.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* ------------------------------------------------------------------------------------------ */
55
'use strict';
66

7-
import { buildErrorMessage } from '../utils';
7+
import { buildLogErrorMessage, buildNotificationErrorMessage } from '../utils';
88
import { IImageProvider, ImageMap, getRange } from '../imageAnalysis/collector';
99
import { AbstractDiagnosticsPipeline } from '../diagnosticsPipeline';
1010
import { clearCodeActionsMap, registerCodeAction, generateRedirectToRecommendedVersionAction } from '../codeActionHandler';
@@ -104,9 +104,9 @@ async function performDiagnostics(diagnosticFilePath: Uri, contents: string, pro
104104
diagnosticsPipeline.reportDiagnostics();
105105

106106
} catch (error) {
107-
outputChannelDep.warn(`component analysis error: ${buildErrorMessage(error as Error)}`);
107+
outputChannelDep.warn(`component analysis error: ${buildLogErrorMessage(error as Error)}`);
108108
notifications.emit('caError', {
109-
errorMessage: (error as Error).message,
109+
errorMessage: buildNotificationErrorMessage(error as Error),
110110
uri: diagnosticFilePath,
111111
});
112112
}

‎src/stackAnalysis.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { StatusMessages, Titles } from './constants';
66
import { stackAnalysisService } from './exhortServices';
77
import { globalConfig } from './config';
88
import { updateCurrentWebviewPanel } from './rhda';
9-
import { buildErrorMessage } from './utils';
9+
import { buildLogErrorMessage } from './utils';
1010
import { DepOutputChannel } from './depOutputChannel';
1111
import { Options } from '@trustification/exhort-javascript-api';
1212

@@ -65,7 +65,7 @@ export async function executeStackAnalysis(manifestFilePath: string, outputChann
6565

6666
updateCurrentWebviewPanel('error');
6767

68-
outputChannel.error(buildErrorMessage(err as Error));
68+
outputChannel.error(buildLogErrorMessage(err as Error));
6969

7070
throw err;
7171
}

‎src/utils.ts‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,47 @@ export function applySettingNameMappings(message: string): string {
2020
return modifiedMessage;
2121
}
2222

23-
export function buildErrorMessage(error: Error): string {
23+
/**
24+
* Builds a message from an error to be displayed in a popup notification to a user.
25+
* This should ideally be terse and direct users to the logs in the Output channel
26+
* to get more in-depth information such as stdout/stderr.
27+
* @param error the error to render
28+
* @returns rendered notification message
29+
*/
30+
export function buildNotificationErrorMessage(error: Error): string {
2431
let message = error.message;
2532
while (error.cause) {
33+
if (Object.hasOwn(error.cause, 'stdout')) {
34+
message = `${message}, please check the Output panel for more details.`;
35+
} else {
36+
message = `${message}: ${(error.cause as Error).message}`;
37+
}
38+
error = error.cause as Error;
39+
}
40+
return message;
41+
}
42+
43+
/**
44+
* Builds a message from an error to be displayed in the Output tab channel
45+
* (via DepOutputChannel). This should include more information than shown in
46+
* the popup notification.
47+
* @param error the error to render
48+
* @returns rendered log message
49+
*/
50+
export function buildLogErrorMessage(error: Error): string {
51+
let message = error.message;
52+
let execErr: (Error & { stderr: string, stdout: string }) | null = null;
53+
while (error.cause) {
54+
if (Object.hasOwn(error.cause, 'stdout')) {
55+
execErr = error.cause as (Error & { stderr: string, stdout: string });
56+
}
2657
message = `${message}: ${(error.cause as Error).message}`;
2758
error = error.cause as Error;
2859
}
60+
61+
if (execErr) {
62+
message += `\nSTDOUT:\n${execErr.stdout.trim() || '<none>'}\n\nSTDERR:\n${execErr.stderr.trim() || '<none>'}`;
63+
}
2964
return message;
3065
}
3166

‎test/caNotification.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ suite('CANotification module', () => {
7070
expect(notification.origin().toString()).to.equal('file:///mock/path');
7171
expect(notification.isDone()).to.be.true;
7272
expect(notification.hasWarning()).to.be.true;
73-
expect(notification.popupText()).to.equal('Found 1 direct vulnerability for Snyk Provider.');
73+
expect(notification.popupText()).to.equal('Found 1 direct vulnerability in /mock/path for Snyk Provider.');
7474
expect(notification.statusText()).to.equal('$(warning) 1 direct vulnerability found for all the providers combined');
7575
});
7676

@@ -89,7 +89,7 @@ suite('CANotification module', () => {
8989
expect(notification.origin().toString()).to.equal('file:///mock/path');
9090
expect(notification.isDone()).to.be.true;
9191
expect(notification.hasWarning()).to.be.true;
92-
expect(notification.popupText()).to.equal('Found 3 direct vulnerabilities for Snyk Provider.');
92+
expect(notification.popupText()).to.equal('Found 3 direct vulnerabilities in /mock/path for Snyk Provider.');
9393
expect(notification.statusText()).to.equal('$(warning) 3 direct vulnerabilities found for all the providers combined');
9494
});
9595

@@ -109,7 +109,7 @@ suite('CANotification module', () => {
109109
expect(notification.origin().toString()).to.equal('file:///mock/path');
110110
expect(notification.isDone()).to.be.true;
111111
expect(notification.hasWarning()).to.be.true;
112-
expect(notification.popupText()).to.equal('Found 3 direct vulnerabilities for Snyk Provider. Found 1 direct vulnerability for Oss-Index Provider.');
112+
expect(notification.popupText()).to.equal('Found 3 direct vulnerabilities in /mock/path for Snyk Provider. Found 1 direct vulnerability in /mock/path for Oss-Index Provider.');
113113
expect(notification.statusText()).to.equal('$(warning) 4 direct vulnerabilities found for all the providers combined');
114114
});
115115

0 commit comments

Comments
 (0)