Skip to content

Commit 04a9374

Browse files
teg-atlassianteg-atlassian
authored
Handling-non-string-types-during-rendering (#1690)
* sanitize input and handle non-string types * Add sanitization to fallback diagnostic messages to prevent JSON parsing errors
1 parent 2c2439f commit 04a9374

File tree

4 files changed

+214
-5
lines changed

4 files changed

+214
-5
lines changed

‎src/rovo-dev/rovoDevCodeActionProvider.test.ts‎

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
import * as vscode from 'vscode';
2+
3+
import { RovoDevCodeActionProvider } from './rovoDevCodeActionProvider';
4+
5+
// Mock path module
6+
jest.mock('path', () => ({
7+
__esModule: true,
8+
default: {
9+
sep: '/',
10+
relative: jest.fn((from: string, to: string) => to.replace(from, '').replace(/^\//, '')),
11+
},
12+
}));
13+
14+
// Mock ExtensionApi to prevent initialization errors
15+
jest.mock('./api/extensionApi', () => ({
16+
ExtensionApi: jest.fn().mockImplementation(() => ({
17+
metadata: {
18+
isDebugging: jest.fn(() => false),
19+
isBoysenberry: jest.fn(() => false),
20+
isRovoDevEnabled: jest.fn(() => true),
21+
version: jest.fn(() => '1.0.0'),
22+
appInstanceId: jest.fn(() => 'test-app-id'),
23+
},
24+
config: {
25+
isDebugPanelEnabled: jest.fn(() => false),
26+
isThinkingBlockEnabled: jest.fn(() => false),
27+
onDidChange: jest.fn(),
28+
},
29+
analytics: {
30+
sendTrackEvent: jest.fn(),
31+
},
32+
jira: {
33+
getSites: jest.fn(() => []),
34+
},
35+
})),
36+
}));
37+
38+
describe('RovoDevCodeActionProvider', () => {
39+
let provider: RovoDevCodeActionProvider;
40+
let mockDocument: vscode.TextDocument;
41+
let mockRange: vscode.Range;
42+
let mockContext: vscode.CodeActionContext;
43+
44+
beforeEach(() => {
45+
provider = new RovoDevCodeActionProvider();
46+
mockDocument = {
47+
fileName: '/test/path/file.ts',
48+
uri: {
49+
fsPath: '/test/path/file.ts',
50+
scheme: 'file',
51+
path: '/test/path/file.ts',
52+
} as vscode.Uri,
53+
} as vscode.TextDocument;
54+
mockRange = new vscode.Range(0, 0, 1, 0);
55+
56+
// Mock workspace.getWorkspaceFolder
57+
(vscode.workspace.getWorkspaceFolder as jest.Mock) = jest.fn().mockReturnValue({
58+
uri: {
59+
fsPath: '/test/path',
60+
scheme: 'file',
61+
path: '/test/path',
62+
},
63+
name: 'test-workspace',
64+
index: 0,
65+
});
66+
});
67+
68+
describe('generateCommand', () => {
69+
it('should handle diagnostic messages with newlines', () => {
70+
const diagnosticWithNewline: vscode.Diagnostic = {
71+
message: "Error: Property 'foo' is missing\n in type 'Bar'\n but required in type 'Baz'",
72+
range: mockRange,
73+
severity: vscode.DiagnosticSeverity.Error,
74+
};
75+
76+
mockContext = {
77+
diagnostics: [diagnosticWithNewline],
78+
only: undefined,
79+
triggerKind: 1, // CodeActionTriggerKind.Automatic
80+
};
81+
82+
const action = provider.generateCommand(
83+
'Fix by Rovo Dev',
84+
'Please fix',
85+
mockDocument,
86+
mockRange,
87+
mockContext,
88+
);
89+
90+
expect(action.command?.arguments).toBeDefined();
91+
const prompt = action.command?.arguments![0] as string;
92+
93+
// Newlines should be replaced with spaces
94+
expect(prompt).toContain("Error: Property 'foo' is missing in type 'Bar' but required in type 'Baz'");
95+
expect(prompt).not.toContain('\n in type');
96+
});
97+
98+
it('should handle multiple diagnostics with special characters', () => {
99+
const diagnostics: vscode.Diagnostic[] = [
100+
{
101+
message: 'Type error: Expected "string" but got "number"',
102+
range: mockRange,
103+
severity: vscode.DiagnosticSeverity.Error,
104+
},
105+
{
106+
message: 'ESLint: Missing semicolon\nSee: https://eslint.org/docs/rules/semi',
107+
range: mockRange,
108+
severity: vscode.DiagnosticSeverity.Warning,
109+
},
110+
];
111+
112+
mockContext = {
113+
diagnostics,
114+
only: undefined,
115+
triggerKind: 1, // CodeActionTriggerKind.Automatic
116+
};
117+
118+
const action = provider.generateCommand(
119+
'Fix by Rovo Dev',
120+
'Please fix',
121+
mockDocument,
122+
mockRange,
123+
mockContext,
124+
);
125+
126+
expect(action.command?.arguments).toBeDefined();
127+
const prompt = action.command?.arguments![0] as string;
128+
129+
// Should contain both diagnostic messages, sanitized
130+
expect(prompt).toContain('Type error: Expected "string" but got "number"');
131+
expect(prompt).toContain('ESLint: Missing semicolon See: https://eslint.org/docs/rules/semi');
132+
});
133+
134+
it('should handle empty diagnostics', () => {
135+
mockContext = {
136+
diagnostics: [],
137+
only: undefined,
138+
triggerKind: 1, // CodeActionTriggerKind.Automatic
139+
};
140+
141+
const action = provider.generateCommand(
142+
'Explain by Rovo Dev',
143+
'Please explain',
144+
mockDocument,
145+
mockRange,
146+
mockContext,
147+
);
148+
149+
expect(action.command?.arguments).toBeDefined();
150+
const prompt = action.command?.arguments![0] as string;
151+
152+
// Should just be the original prompt without additional context
153+
expect(prompt).toBe('Please explain');
154+
expect(prompt).not.toContain('Additional problem context');
155+
});
156+
157+
it('should sanitize carriage returns and multiple consecutive newlines', () => {
158+
const diagnostic: vscode.Diagnostic = {
159+
message: 'Error message\r\n\r\nwith multiple\r\nline breaks',
160+
range: mockRange,
161+
severity: vscode.DiagnosticSeverity.Error,
162+
};
163+
164+
mockContext = {
165+
diagnostics: [diagnostic],
166+
only: undefined,
167+
triggerKind: 1, // CodeActionTriggerKind.Automatic
168+
};
169+
170+
const action = provider.generateCommand(
171+
'Fix by Rovo Dev',
172+
'Please fix',
173+
mockDocument,
174+
mockRange,
175+
mockContext,
176+
);
177+
178+
expect(action.command?.arguments).toBeDefined();
179+
const prompt = action.command?.arguments![0] as string;
180+
181+
// All line breaks should be replaced with single spaces
182+
expect(prompt).toContain('Error message with multiple line breaks');
183+
expect(prompt).not.toContain('\r');
184+
expect(prompt).not.toContain('\n\n');
185+
});
186+
});
187+
});

‎src/rovo-dev/rovoDevCodeActionProvider.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -104,9 +104,9 @@ export class RovoDevCodeActionProvider implements vscode.CodeActionProvider {
104104
} catch (error) {
105105
// Fallback to basic prompt if context extraction fails
106106
Logger.warn('Failed to extract context for Rovo Dev:', error);
107-
return diagnostics.length
108-
? `${basePrompt}\nAdditional problem context:\n${diagnostics.map((d) => d.message).join('\n')}`
109-
: basePrompt;
107+
// Sanitize diagnostic messages to prevent JSON parsing errors
108+
const sanitizedMessages = diagnostics.map((d) => d.message.replace(/[\r\n]+/g, ' ').trim()).join('\n');
109+
return diagnostics.length ? `${basePrompt}\nAdditional problem context:\n${sanitizedMessages}` : basePrompt;
110110
}
111111
}
112112
}

‎src/rovo-dev/ui/common/MarkedDown.test.tsx‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,23 @@ describe('MarkedDown', () => {
3737
expect(container.querySelector('span')).toBeTruthy();
3838
});
3939

40+
it('handles non-string values without crashing', () => {
41+
expect(() => {
42+
// Test with a number
43+
render(<MarkedDown value={123 as any} onLinkClick={mockOnLinkClick} />);
44+
}).not.toThrow();
45+
46+
expect(() => {
47+
// Test with an object
48+
render(<MarkedDown value={{ foo: 'bar' } as any} onLinkClick={mockOnLinkClick} />);
49+
}).not.toThrow();
50+
51+
expect(() => {
52+
// Test with an array
53+
render(<MarkedDown value={[1, 2, 3] as any} onLinkClick={mockOnLinkClick} />);
54+
}).not.toThrow();
55+
});
56+
4057
it('handles malformed markdown gracefully', () => {
4158
// Mock console.error to verify it's called
4259
const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();

‎src/rovo-dev/ui/common/common.tsx‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,10 @@ export const MarkedDown: React.FC<{
8787

8888
const html = React.useMemo(() => {
8989
try {
90-
return mdParser.render(value ?? '');
90+
// Ensure value is always a string to prevent "Input data should be a String" errors
91+
const stringValue =
92+
value !== null && value !== undefined && typeof value !== 'string' ? String(value) : (value ?? '');
93+
return mdParser.render(stringValue);
9194
} catch (error) {
9295
// If markdown parsing fails, report error to backend and return plain text
9396
console.error('Markdown parsing error:', error);
@@ -98,7 +101,9 @@ export const MarkedDown: React.FC<{
98101
}
99102

100103
// Fallback to plain text instead of crashing
101-
return value ?? '';
104+
const stringValue =
105+
value !== null && value !== undefined && typeof value !== 'string' ? String(value) : (value ?? '');
106+
return stringValue;
102107
}
103108
}, [value, reportError]);
104109

0 commit comments

Comments
 (0)