Skip to content

Commit d91fa56

Browse files
ArtmannArtmann
committed
more feedback
1 parent bf17ea9 commit d91fa56

File tree

5 files changed

+240
-7
lines changed

5 files changed

+240
-7
lines changed

‎src/notebooks/deepnote/deepnoteExplorerView.ts‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -231,13 +231,20 @@ export class DeepnoteExplorerView {
231231
...targetNotebook,
232232
id: generateUuid(),
233233
name: newName,
234-
blocks: targetNotebook.blocks.map((block: DeepnoteBlock) => ({
235-
...block,
236-
id: generateUuid(),
237-
blockGroup: generateUuid(),
238-
executionCount: undefined,
239-
...(block.metadata != null ? { metadata: { ...block.metadata } } : {})
240-
}))
234+
blocks: targetNotebook.blocks.map((block: DeepnoteBlock) => {
235+
// Use structuredClone for deep cloning if available, otherwise fall back to JSON
236+
const clonedBlock =
237+
typeof structuredClone !== 'undefined'
238+
? structuredClone(block)
239+
: JSON.parse(JSON.stringify(block));
240+
241+
// Update cloned block with new IDs and reset execution state
242+
clonedBlock.id = generateUuid();
243+
clonedBlock.blockGroup = generateUuid();
244+
clonedBlock.executionCount = undefined;
245+
246+
return clonedBlock;
247+
})
241248
};
242249

243250
projectData.project.notebooks.push(newNotebook);

‎src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1470,6 +1470,136 @@ suite('DeepnoteExplorerView - Empty State Commands', () => {
14701470
// Verify error message was shown
14711471
verify(mockedVSCodeNamespaces.window.showErrorMessage(anything())).once();
14721472
});
1473+
1474+
test('should deep clone blocks to prevent shared references', async () => {
1475+
// This test verifies that duplicating a notebook creates truly independent copies
1476+
// of nested objects like outputs and metadata, not just shallow references
1477+
const projectData: DeepnoteFile = {
1478+
version: '1.0',
1479+
metadata: {
1480+
createdAt: '2024-01-01T00:00:00Z',
1481+
modifiedAt: '2024-01-01T00:00:00Z'
1482+
},
1483+
project: {
1484+
id: 'test-project-id',
1485+
name: 'Test Project',
1486+
notebooks: [
1487+
{
1488+
id: 'original-notebook-id',
1489+
name: 'Original Notebook',
1490+
blocks: [
1491+
{
1492+
id: 'block-1',
1493+
blockGroup: 'group-1',
1494+
type: 'code',
1495+
content: 'print("test")',
1496+
sortingKey: '0',
1497+
version: 1,
1498+
executionCount: 5,
1499+
outputs: [{ type: 'stream', text: 'test output' }],
1500+
metadata: { cellId: 'cell-123', custom: { nested: 'value' } }
1501+
}
1502+
],
1503+
executionMode: 'block'
1504+
}
1505+
]
1506+
}
1507+
};
1508+
1509+
const mockTreeItem: Partial<DeepnoteTreeItem> = {
1510+
type: DeepnoteTreeItemType.Notebook,
1511+
context: {
1512+
filePath: '/workspace/test-project.deepnote',
1513+
projectId: 'test-project-id',
1514+
notebookId: 'original-notebook-id'
1515+
},
1516+
data: projectData.project.notebooks[0]
1517+
};
1518+
1519+
// Mock file system
1520+
const mockFS = mock<typeof workspace.fs>();
1521+
const yamlContent = yaml.dump(projectData);
1522+
when(mockFS.readFile(anything())).thenReturn(Promise.resolve(Buffer.from(yamlContent, 'utf-8')));
1523+
1524+
let capturedWriteContent: Uint8Array | undefined;
1525+
when(mockFS.writeFile(anything(), anything())).thenCall((_uri: Uri, content: Uint8Array) => {
1526+
capturedWriteContent = content;
1527+
return Promise.resolve();
1528+
});
1529+
1530+
when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFS));
1531+
1532+
// Stub generateUuid to return predictable IDs
1533+
const generateUuidStub = sinon.stub(uuidModule, 'generateUuid');
1534+
generateUuidStub.onCall(0).returns('duplicate-notebook-id');
1535+
generateUuidStub.onCall(1).returns('duplicate-block-id');
1536+
generateUuidStub.onCall(2).returns('duplicate-blockgroup-id');
1537+
1538+
// Execute duplication
1539+
await explorerView.duplicateNotebook(mockTreeItem as DeepnoteTreeItem);
1540+
1541+
// Parse the written data
1542+
assert.isDefined(capturedWriteContent, 'File should have been written');
1543+
const writtenYaml = Buffer.from(capturedWriteContent!).toString('utf-8');
1544+
const updatedProjectData = yaml.load(writtenYaml) as DeepnoteFile;
1545+
1546+
// Find original and duplicated notebooks
1547+
const originalNotebook = updatedProjectData.project.notebooks.find(
1548+
(nb) => nb.id === 'original-notebook-id'
1549+
);
1550+
const duplicateNotebook = updatedProjectData.project.notebooks.find(
1551+
(nb) => nb.id === 'duplicate-notebook-id'
1552+
);
1553+
1554+
assert.isDefined(originalNotebook, 'Original notebook should exist');
1555+
assert.isDefined(duplicateNotebook, 'Duplicate notebook should exist');
1556+
1557+
// Verify the blocks are truly independent (deep clone)
1558+
const originalBlock = originalNotebook!.blocks[0];
1559+
const duplicateBlock = duplicateNotebook!.blocks[0];
1560+
1561+
// Test 1: Verify outputs are not the same reference
1562+
assert.notStrictEqual(
1563+
originalBlock.outputs,
1564+
duplicateBlock.outputs,
1565+
'Outputs should be different array instances'
1566+
);
1567+
1568+
// Test 2: Verify metadata is not the same reference
1569+
if (originalBlock.metadata && duplicateBlock.metadata) {
1570+
assert.notStrictEqual(
1571+
originalBlock.metadata,
1572+
duplicateBlock.metadata,
1573+
'Metadata should be different object instances'
1574+
);
1575+
1576+
// Test 3: Verify nested metadata properties are not shared
1577+
if (
1578+
typeof originalBlock.metadata === 'object' &&
1579+
'custom' in originalBlock.metadata &&
1580+
typeof duplicateBlock.metadata === 'object' &&
1581+
'custom' in duplicateBlock.metadata
1582+
) {
1583+
assert.notStrictEqual(
1584+
(originalBlock.metadata as any).custom,
1585+
(duplicateBlock.metadata as any).custom,
1586+
'Nested metadata objects should be different instances'
1587+
);
1588+
}
1589+
}
1590+
1591+
// Test 4: Verify that modifying duplicate doesn't affect original
1592+
// (This would fail with shallow copy)
1593+
duplicateBlock.outputs!.push({ type: 'stream', text: 'new output' });
1594+
assert.strictEqual(
1595+
originalBlock.outputs!.length,
1596+
1,
1597+
'Original outputs should not be affected by changes to duplicate'
1598+
);
1599+
assert.strictEqual(duplicateBlock.outputs!.length, 2, 'Duplicate outputs should have the new item');
1600+
1601+
generateUuidStub.restore();
1602+
});
14731603
});
14741604

14751605
suite('renameProject', () => {

‎src/notebooks/deepnote/deepnoteTreeDataProvider.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,9 @@ export class DeepnoteTreeDataProvider implements TreeDataProvider<DeepnoteTreeIt
8484
if (project) {
8585
// Update the tree item's data
8686
cachedTreeItem.data = project;
87+
88+
// Update visual fields (label, description, tooltip) to reflect changes
89+
cachedTreeItem.updateVisualFields();
8790
}
8891
} catch (error) {
8992
this.logger?.error(`Failed to reload project ${filePath}`, error);
@@ -125,6 +128,9 @@ export class DeepnoteTreeDataProvider implements TreeDataProvider<DeepnoteTreeIt
125128
if (project) {
126129
// Update the tree item's data
127130
cachedTreeItem.data = project;
131+
132+
// Update visual fields (label, description, tooltip) to reflect changes
133+
cachedTreeItem.updateVisualFields();
128134
}
129135
} catch (error) {
130136
this.logger?.error(`Failed to reload project ${filePath}`, error);

‎src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,86 @@ suite('DeepnoteTreeDataProvider', () => {
336336
});
337337
});
338338

339+
test('should update visual fields when project data changes', async () => {
340+
// Access private caches
341+
const treeItemCache = (provider as any).treeItemCache as Map<string, DeepnoteTreeItem>;
342+
343+
// Create initial project with 1 notebook
344+
const filePath = '/workspace/test-project.deepnote';
345+
const cacheKey = `project:${filePath}`;
346+
const initialProject: DeepnoteProject = {
347+
metadata: {
348+
createdAt: '2023-01-01T00:00:00Z',
349+
modifiedAt: '2023-01-01T00:00:00Z'
350+
},
351+
project: {
352+
id: 'project-123',
353+
name: 'Original Name',
354+
notebooks: [
355+
{
356+
id: 'notebook-1',
357+
name: 'Notebook 1',
358+
blocks: [],
359+
executionMode: 'block',
360+
isModule: false
361+
}
362+
],
363+
settings: {}
364+
},
365+
version: '1.0'
366+
};
367+
368+
const mockTreeItem = new DeepnoteTreeItem(
369+
DeepnoteTreeItemType.ProjectFile,
370+
{
371+
filePath: filePath,
372+
projectId: 'project-123'
373+
},
374+
initialProject,
375+
1
376+
);
377+
treeItemCache.set(cacheKey, mockTreeItem);
378+
379+
// Verify initial state
380+
assert.strictEqual(mockTreeItem.label, 'Original Name');
381+
assert.strictEqual(mockTreeItem.description, '1 notebook');
382+
383+
// Update the project data (simulating rename and adding notebooks)
384+
const updatedProject: DeepnoteProject = {
385+
...initialProject,
386+
project: {
387+
...initialProject.project,
388+
name: 'Renamed Project',
389+
notebooks: [
390+
initialProject.project.notebooks[0],
391+
{
392+
id: 'notebook-2',
393+
name: 'Notebook 2',
394+
blocks: [],
395+
executionMode: 'block',
396+
isModule: false
397+
}
398+
]
399+
}
400+
};
401+
402+
mockTreeItem.data = updatedProject;
403+
mockTreeItem.updateVisualFields();
404+
405+
// Verify visual fields were updated
406+
assert.strictEqual(mockTreeItem.label, 'Renamed Project', 'Label should reflect new project name');
407+
assert.strictEqual(
408+
mockTreeItem.description,
409+
'2 notebooks',
410+
'Description should reflect new notebook count'
411+
);
412+
assert.include(
413+
mockTreeItem.tooltip as string,
414+
'Renamed Project',
415+
'Tooltip should include new project name'
416+
);
417+
});
418+
339419
test('should clear both caches when file is deleted', () => {
340420
// Access private caches
341421
const cachedProjects = (provider as any).cachedProjects as Map<string, DeepnoteProject>;

‎src/notebooks/deepnote/deepnoteTreeItem.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,4 +104,14 @@ export class DeepnoteTreeItem extends TreeItem {
104104

105105
return undefined;
106106
}
107+
108+
/**
109+
* Updates the tree item's visual fields (label, description, tooltip) based on current data.
110+
* Call this after updating the data property to ensure the tree view reflects changes.
111+
*/
112+
public updateVisualFields(): void {
113+
this.label = this.getLabel();
114+
this.description = this.getDescription();
115+
this.tooltip = this.getTooltip();
116+
}
107117
}

0 commit comments

Comments
 (0)