Skip to content

Commit 4cc735b

Browse files
ArtmannArtmann
committed
pr feedback
1 parent 56e49ae commit 4cc735b

File tree

6 files changed

+215
-22
lines changed

6 files changed

+215
-22
lines changed

‎src/messageTypes.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,13 @@ export type LocalizedMessages = {
158158
selectedImageListLabel: string;
159159
selectedImageLabel: string;
160160
dvDeprecationWarning: string;
161+
dataframeRowsColumns: string;
162+
dataframePerPage: string;
163+
dataframePreviousPage: string;
164+
dataframeNextPage: string;
165+
dataframePageOf: string;
166+
dataframeCopyTable: string;
167+
dataframeExportTable: string;
161168
};
162169
// Map all messages to specific payloads
163170
export class IInteractiveWindowMapping {

‎src/platform/common/utils/localize.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -805,6 +805,13 @@ export namespace WebViews {
805805
export const dvDeprecationWarning = l10n.t(
806806
'The built-in data viewer will be deprecated and no longer usable starting with Visual Studio Code 1.92. Please <a href="command:workbench.extensions.search?%22@tag:jupyterVariableViewers%22">install other data viewing extensions</a> to continue inspecting data'
807807
);
808+
export const dataframeRowsColumns = l10n.t('{0} rows, {1} columns');
809+
export const dataframePerPage = l10n.t('/ page');
810+
export const dataframePreviousPage = l10n.t('Previous page');
811+
export const dataframeNextPage = l10n.t('Next page');
812+
export const dataframePageOf = l10n.t('Page {0} of {1}');
813+
export const dataframeCopyTable = l10n.t('Copy table');
814+
export const dataframeExportTable = l10n.t('Export table');
808815
}
809816

810817
export namespace Deprecated {

‎src/platform/webviews/webviewHost.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,14 @@ export abstract class WebviewHost<IMapping> implements IDisposable {
258258
deletePlot: localize.WebViews.deletePlot,
259259
selectedImageListLabel: localize.WebViews.selectedImageListLabel,
260260
selectedImageLabel: localize.WebViews.selectedImageLabel,
261-
dvDeprecationWarning: localize.WebViews.dvDeprecationWarning
261+
dvDeprecationWarning: localize.WebViews.dvDeprecationWarning,
262+
dataframeRowsColumns: localize.WebViews.dataframeRowsColumns,
263+
dataframePerPage: localize.WebViews.dataframePerPage,
264+
dataframePreviousPage: localize.WebViews.dataframePreviousPage,
265+
dataframeNextPage: localize.WebViews.dataframeNextPage,
266+
dataframePageOf: localize.WebViews.dataframePageOf,
267+
dataframeCopyTable: localize.WebViews.dataframeCopyTable,
268+
dataframeExportTable: localize.WebViews.dataframeExportTable
262269
};
263270
this.postMessageInternal(SharedMessages.LocInit, JSON.stringify(locStrings)).catch(noop);
264271
}

‎src/webviews/extension-side/dataframe/dataframeController.ts‎

Lines changed: 47 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
env,
55
l10n,
66
NotebookEdit,
7+
type NotebookCellOutput,
78
type NotebookEditor,
89
type NotebookRendererMessaging,
910
notebooks,
@@ -17,7 +18,6 @@ import type { IExtensionSyncActivationService } from '../../../platform/activati
1718
import type { IDisposable } from '../../../platform/common/types';
1819
import { dispose } from '../../../platform/common/utils/lifecycle';
1920
import { logger } from '../../../platform/logging';
20-
import { Output } from '../../../api';
2121

2222
type SelectPageSizeCommand = {
2323
cellId?: string;
@@ -69,8 +69,35 @@ export class DataframeController implements IExtensionSyncActivationService {
6969
dispose(this.disposables);
7070
}
7171

72+
private escapeCsvField(value: unknown): string {
73+
// Handle null/undefined as empty string
74+
if (value === null || value === undefined) {
75+
return '';
76+
}
77+
78+
// Convert to string
79+
const stringValue = String(value);
80+
81+
// Check if field needs quoting (contains comma, quote, or newline)
82+
const needsQuoting =
83+
stringValue.includes(',') ||
84+
stringValue.includes('"') ||
85+
stringValue.includes('\n') ||
86+
stringValue.includes('\r');
87+
88+
if (needsQuoting) {
89+
// Escape internal quotes by doubling them, then wrap in quotes
90+
return `"${stringValue.replace(/"/g, '""')}"`;
91+
}
92+
93+
return stringValue;
94+
}
95+
7296
private dataframeToCsv(dataframe: DataFrameObject): string {
73-
console.log('Converting dataframe to CSV:', dataframe);
97+
logger.debug('[DataframeController] Converting dataframe to CSV', {
98+
columnCount: dataframe?.column_count,
99+
rowCount: dataframe?.row_count
100+
});
74101

75102
if (!dataframe || !dataframe.columns || !dataframe.rows) {
76103
return '';
@@ -83,20 +110,27 @@ export class DataframeController implements IExtensionSyncActivationService {
83110
.filter((name: string | undefined) => Boolean(name))
84111
.filter((name: string) => !name.trim().toLowerCase().startsWith('_deepnote')) as string[];
85112

86-
const csvRows = rows.map((row: Record<string, unknown>) => columnNames.map((col) => row[col]).join(','));
113+
// Escape and join header names
114+
const headerRow = columnNames.map((name) => this.escapeCsvField(name)).join(',');
87115

88-
return [columnNames.join(','), ...csvRows].join('\n');
116+
// Escape and join data rows
117+
const csvRows = rows.map((row: Record<string, unknown>) =>
118+
columnNames.map((col) => this.escapeCsvField(row[col])).join(',')
119+
);
120+
121+
return [headerRow, ...csvRows].join('\n');
89122
}
90123

91-
private async getDataframeFromDataframeOutput(outputs: readonly Output[]): Promise<DataFrameObject | undefined> {
124+
private async getDataframeFromDataframeOutput(
125+
outputs: readonly NotebookCellOutput[]
126+
): Promise<DataFrameObject | undefined> {
92127
if (outputs.length === 0) {
93128
await this.showErrorToUser(l10n.t('No outputs found in the cell.'));
94129
return;
95130
}
96131

97-
const [firstOutput] = outputs;
98-
99-
const item = firstOutput.items.find(
132+
const items = outputs.flatMap((output) => output.items);
133+
const item = items.find(
100134
(i: { data: unknown; mime: string }) => i.mime === 'application/vnd.deepnote.dataframe.v3+json'
101135
);
102136

@@ -203,10 +237,12 @@ export class DataframeController implements IExtensionSyncActivationService {
203237

204238
await workspace.fs.writeFile(uri, encoder.encode(csv));
205239

206-
await window.showInformationMessage(`File saved to ${uri}`);
240+
await window.showInformationMessage(l10n.t('File saved to {0}', uri));
207241
}
208242
} catch (error) {
209-
await window.showErrorMessage(`Failed to save file: ${error}`);
243+
const message = error instanceof Error ? error.message : String(error);
244+
245+
await window.showErrorMessage(l10n.t('Failed to save file: {0}', message));
210246
}
211247
}
212248

@@ -300,7 +336,7 @@ export class DataframeController implements IExtensionSyncActivationService {
300336
await workspace.applyEdit(edit);
301337

302338
// Re-execute the cell to apply the new page size
303-
logger.info(`[DataframeRenderer] Re-executing cell ${cellIndex} with new page size`);
339+
logger.info(`[DataframeController] Re-executing cell ${cellIndex} with new page size`);
304340

305341
await commands.executeCommand('notebook.cell.execute', {
306342
ranges: [{ start: cellIndex, end: cellIndex + 1 }],

‎src/webviews/extension-side/dataframe/dataframeController.unit.test.ts‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,132 @@ suite('DataframeController', () => {
173173
assert.strictEqual(lines[0], 'col1');
174174
assert.strictEqual(lines[1], '1');
175175
});
176+
177+
test('Should escape values containing commas', () => {
178+
const dataframe = {
179+
column_count: 2,
180+
columns: [
181+
{ dtype: 'int64', name: 'id' },
182+
{ dtype: 'string', name: 'address' }
183+
],
184+
preview_row_count: 2,
185+
row_count: 2,
186+
rows: [
187+
{ id: 1, address: '123 Main St, Apt 4' },
188+
{ id: 2, address: 'New York, NY' }
189+
],
190+
type: 'dataframe'
191+
};
192+
const result = (controller as any).dataframeToCsv(dataframe);
193+
const lines = result.split('\n');
194+
195+
assert.strictEqual(lines[0], 'id,address');
196+
assert.strictEqual(lines[1], '1,"123 Main St, Apt 4"');
197+
assert.strictEqual(lines[2], '2,"New York, NY"');
198+
});
199+
200+
test('Should escape values containing quotes', () => {
201+
const dataframe = {
202+
column_count: 2,
203+
columns: [
204+
{ dtype: 'int64', name: 'id' },
205+
{ dtype: 'string', name: 'message' }
206+
],
207+
preview_row_count: 2,
208+
row_count: 2,
209+
rows: [
210+
{ id: 1, message: 'He said "Hello"' },
211+
{ id: 2, message: 'She replied "Hi"' }
212+
],
213+
type: 'dataframe'
214+
};
215+
const result = (controller as any).dataframeToCsv(dataframe);
216+
const lines = result.split('\n');
217+
218+
assert.strictEqual(lines[0], 'id,message');
219+
assert.strictEqual(lines[1], '1,"He said ""Hello"""');
220+
assert.strictEqual(lines[2], '2,"She replied ""Hi"""');
221+
});
222+
223+
test('Should escape values containing newlines', () => {
224+
const dataframe = {
225+
column_count: 2,
226+
columns: [
227+
{ dtype: 'int64', name: 'id' },
228+
{ dtype: 'string', name: 'notes' }
229+
],
230+
preview_row_count: 1,
231+
row_count: 1,
232+
rows: [{ id: 1, notes: 'Line 1\nLine 2' }],
233+
type: 'dataframe'
234+
};
235+
const result = (controller as any).dataframeToCsv(dataframe);
236+
237+
// Don't split by \n since the quoted field contains newlines
238+
assert.strictEqual(result, 'id,notes\n1,"Line 1\nLine 2"');
239+
});
240+
241+
test('Should handle null and undefined values', () => {
242+
const dataframe = {
243+
column_count: 3,
244+
columns: [
245+
{ dtype: 'int64', name: 'id' },
246+
{ dtype: 'string', name: 'value1' },
247+
{ dtype: 'string', name: 'value2' }
248+
],
249+
preview_row_count: 2,
250+
row_count: 2,
251+
rows: [
252+
{ id: 1, value1: null, value2: 'test' },
253+
{ id: 2, value1: 'test', value2: undefined }
254+
],
255+
type: 'dataframe'
256+
};
257+
const result = (controller as any).dataframeToCsv(dataframe);
258+
const lines = result.split('\n');
259+
260+
assert.strictEqual(lines[0], 'id,value1,value2');
261+
assert.strictEqual(lines[1], '1,,test');
262+
assert.strictEqual(lines[2], '2,test,');
263+
});
264+
265+
test('Should escape column names with special characters', () => {
266+
const dataframe = {
267+
column_count: 3,
268+
columns: [
269+
{ dtype: 'int64', name: 'id' },
270+
{ dtype: 'string', name: 'name, title' },
271+
{ dtype: 'string', name: 'description "quoted"' }
272+
],
273+
preview_row_count: 1,
274+
row_count: 1,
275+
rows: [{ id: 1, 'name, title': 'John Doe', 'description "quoted"': 'Test' }],
276+
type: 'dataframe'
277+
};
278+
const result = (controller as any).dataframeToCsv(dataframe);
279+
const lines = result.split('\n');
280+
281+
assert.strictEqual(lines[0], 'id,"name, title","description ""quoted"""');
282+
assert.strictEqual(lines[1], '1,John Doe,Test');
283+
});
284+
285+
test('Should handle complex escaping scenarios', () => {
286+
const dataframe = {
287+
column_count: 2,
288+
columns: [
289+
{ dtype: 'int64', name: 'id' },
290+
{ dtype: 'string', name: 'data' }
291+
],
292+
preview_row_count: 1,
293+
row_count: 1,
294+
rows: [{ id: 1, data: 'Value with "quotes", commas, and\nnewlines' }],
295+
type: 'dataframe'
296+
};
297+
const result = (controller as any).dataframeToCsv(dataframe);
298+
299+
// Don't split by \n since the quoted field contains newlines
300+
assert.strictEqual(result, 'id,data\n1,"Value with ""quotes"", commas, and\nnewlines"');
301+
});
176302
});
177303

178304
suite('Dataframe Extraction (getDataframeFromDataframeOutput)', () => {

‎src/webviews/webview-side/dataframe-renderer/DataframeRenderer.tsx‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import type { RendererContext } from 'vscode-notebook-renderer';
66

77
import '../react-common/codicon/codicon.css';
88
import { generateUuid } from '../../../platform/common/uuid';
9+
import { getLocString } from '../react-common/locReactSide';
910

1011
export interface DataframeMetadata {
1112
table_state_spec?: string;
@@ -170,7 +171,9 @@ export const DataframeRenderer = memo(function DataframeRenderer({
170171
<div className="px-[8px] py-[12px] flex justify-between items-center border-l border-r border-b border-[var(--vscode-panel-border)] font-mono">
171172
<div className="flex gap-[4px] items-center">
172173
<div>
173-
{numberOfRows} rows, {numberOfColumns} columns
174+
{getLocString('dataframeRowsColumns', `{0} rows, {1} columns`)
175+
.replace('{0}', String(numberOfRows))
176+
.replace('{1}', String(numberOfColumns))}
174177
</div>
175178
<div>
176179
<select className="font-mono" id={selectId} value={pageSize} onChange={handlePageSizeChange}>
@@ -180,13 +183,13 @@ export const DataframeRenderer = memo(function DataframeRenderer({
180183
<option value={100}>100</option>
181184
</select>
182185

183-
<label htmlFor={selectId}>/ page</label>
186+
<label htmlFor={selectId}>{getLocString('dataframePerPage', '/ page')}</label>
184187
</div>
185188
</div>
186189

187190
<div className="flex gap-[12px] items-center">
188191
<IconButton
189-
aria-label="Previous page"
192+
aria-label={getLocString('dataframePreviousPage', 'Previous page')}
190193
className={`
191194
border border-[var(--vscode-panel-border)] bg-[var(--vscode-button-secondaryBackground)] hover:bg-[var(--vscode-button-secondaryHoverBackground)]
192195
text-[var(--vscode-button-secondaryForeground)]
@@ -195,17 +198,19 @@ export const DataframeRenderer = memo(function DataframeRenderer({
195198
h-[20px] w-[20px]
196199
`}
197200
disabled={pageIndex === 0}
198-
title="Previous page"
201+
title={getLocString('dataframePreviousPage', 'Previous page')}
199202
type="button"
200203
onClick={() => handlePageChange(pageIndex - 1)}
201204
>
202205
<div className="codicon codicon-chevron-left" style={{ fontSize: 12 }} />
203206
</IconButton>
204207
<span className="">
205-
Page {pageIndex + 1} of {totalPages}
208+
{getLocString('dataframePageOf', 'Page {0} of {1}')
209+
.replace('{0}', String(pageIndex + 1))
210+
.replace('{1}', String(totalPages))}
206211
</span>
207212
<IconButton
208-
aria-label="Next page"
213+
aria-label={getLocString('dataframeNextPage', 'Next page')}
209214
className={`
210215
border border-[var(--vscode-panel-border)] bg-[var(--vscode-button-secondaryBackground)] hover:bg-[var(--vscode-button-secondaryHoverBackground)]
211216
text-[var(--vscode-button-secondaryForeground)]
@@ -214,7 +219,7 @@ export const DataframeRenderer = memo(function DataframeRenderer({
214219
h-[20px] w-[20px]
215220
`}
216221
disabled={pageIndex >= totalPages - 1}
217-
title="Next page"
222+
title={getLocString('dataframeNextPage', 'Next page')}
218223
type="button"
219224
onClick={() => handlePageChange(pageIndex + 1)}
220225
>
@@ -224,13 +229,18 @@ export const DataframeRenderer = memo(function DataframeRenderer({
224229

225230
<div>
226231
<div className="flex items-center gap-[4px]">
227-
<IconButton aria-label="Copy table" title="Copy table" type="button" onClick={handleCopyTable}>
232+
<IconButton
233+
aria-label={getLocString('dataframeCopyTable', 'Copy table')}
234+
title={getLocString('dataframeCopyTable', 'Copy table')}
235+
type="button"
236+
onClick={handleCopyTable}
237+
>
228238
<div className="codicon codicon-files" style={{ fontSize: 12 }} />
229239
</IconButton>
230240

231241
<IconButton
232-
aria-label="Export table"
233-
title="Export table"
242+
aria-label={getLocString('dataframeExportTable', 'Export table')}
243+
title={getLocString('dataframeExportTable', 'Export table')}
234244
type="button"
235245
onClick={handleExportTable}
236246
>

0 commit comments

Comments
 (0)