Skip to content

Commit b75a3a8

Browse files
committed
address coderabbit feedback
1 parent ff9a9f8 commit b75a3a8

10 files changed

+69
-15
lines changed

src/notebooks/deepnote/converters/codeBlockConverter.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ export class CodeBlockConverter implements BlockConverter {
99
}
1010

1111
canConvert(blockType: string): boolean {
12-
return blockType === 'code';
12+
return blockType.toLowerCase() === 'code';
1313
}
1414

1515
convertToCell(block: DeepnoteBlock): NotebookCellData {

src/notebooks/deepnote/converters/codeBlockConverter.unit.test.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,12 @@ suite('CodeBlockConverter', () => {
1616
assert.isTrue(converter.canConvert('code'));
1717
});
1818

19+
test('canConvert ignores case', () => {
20+
assert.isTrue(converter.canConvert('CODE'));
21+
assert.isTrue(converter.canConvert('Code'));
22+
assert.isTrue(converter.canConvert('CoDe'));
23+
});
24+
1925
test('returns false for non-code types', () => {
2026
assert.isFalse(converter.canConvert('markdown'));
2127
assert.isFalse(converter.canConvert('text-cell-h1'));

src/notebooks/deepnote/converters/converterRegistry.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,17 @@
11
import type { BlockConverter } from './blockConverter';
22

33
export class ConverterRegistry {
4-
private readonly converters: BlockConverter[] = [];
54
private readonly typeToConverterMap: Map<string, BlockConverter> = new Map();
65

76
findConverter(blockType: string): BlockConverter | undefined {
8-
return this.typeToConverterMap.get(blockType);
7+
return this.typeToConverterMap.get(blockType.toLowerCase());
98
}
109

1110
listSupportedTypes(): string[] {
1211
return Array.from(this.typeToConverterMap.keys()).sort();
1312
}
1413

1514
register(converter: BlockConverter): void {
16-
this.converters.push(converter);
17-
1815
converter.getSupportedTypes().forEach((type) => {
1916
this.typeToConverterMap.set(type, converter);
2017
});

src/notebooks/deepnote/converters/markdownBlockConverter.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ export class MarkdownBlockConverter implements BlockConverter {
99
}
1010

1111
canConvert(blockType: string): boolean {
12-
return blockType === 'markdown';
12+
return blockType.toLowerCase() === 'markdown';
1313
}
1414

1515
convertToCell(block: DeepnoteBlock): NotebookCellData {

src/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,12 @@ suite('MarkdownBlockConverter', () => {
1616
assert.isTrue(converter.canConvert('markdown'));
1717
});
1818

19+
test('canConvert ignores case', () => {
20+
assert.isTrue(converter.canConvert('MARKDOWN'));
21+
assert.isTrue(converter.canConvert('Markdown'));
22+
assert.isTrue(converter.canConvert('MaRkDoWn'));
23+
});
24+
1925
test('returns false for non-markdown types', () => {
2026
assert.isFalse(converter.canConvert('code'));
2127
assert.isFalse(converter.canConvert('text-cell-h1'));

src/notebooks/deepnote/converters/textBlockConverter.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,11 @@ export class TextBlockConverter implements BlockConverter {
1010
let value = cell.value || '';
1111

1212
if (block.type === 'text-cell-h1') {
13-
value = value.replace(/^#\s+/, '');
13+
value = value.replace(/^\s*#\s+/, '');
1414
} else if (block.type === 'text-cell-h2') {
15-
value = value.replace(/^##\s+/, '');
15+
value = value.replace(/^\s*##\s+/, '');
1616
} else if (block.type === 'text-cell-h3') {
17-
value = value.replace(/^###\s+/, '');
17+
value = value.replace(/^\s*###\s+/, '');
1818
}
1919

2020
block.content = value;
@@ -43,6 +43,6 @@ export class TextBlockConverter implements BlockConverter {
4343
}
4444

4545
getSupportedTypes(): string[] {
46-
return TextBlockConverter.textBlockTypes;
46+
return [...TextBlockConverter.textBlockTypes];
4747
}
4848
}

src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,20 @@ suite('TextBlockConverter', () => {
135135
assert.strictEqual(block.content, 'New Title');
136136
});
137137

138+
test('strips # prefix with leading whitespace from h1 cell', () => {
139+
const block: DeepnoteBlock = {
140+
content: 'old content',
141+
id: 'block-123',
142+
sortingKey: 'a0',
143+
type: 'text-cell-h1'
144+
};
145+
const cell = new NotebookCellData(NotebookCellKind.Markup, ' # New Title', 'markdown');
146+
147+
converter.applyChangesToBlock(block, cell);
148+
149+
assert.strictEqual(block.content, 'New Title');
150+
});
151+
138152
test('strips ## prefix from h2 cell', () => {
139153
const block: DeepnoteBlock = {
140154
content: 'old content',
@@ -149,6 +163,20 @@ suite('TextBlockConverter', () => {
149163
assert.strictEqual(block.content, 'New Section');
150164
});
151165

166+
test('strips ## prefix with leading whitespace from h2 cell', () => {
167+
const block: DeepnoteBlock = {
168+
content: 'old content',
169+
id: 'block-123',
170+
sortingKey: 'a0',
171+
type: 'text-cell-h2'
172+
};
173+
const cell = new NotebookCellData(NotebookCellKind.Markup, ' ## New Section', 'markdown');
174+
175+
converter.applyChangesToBlock(block, cell);
176+
177+
assert.strictEqual(block.content, 'New Section');
178+
});
179+
152180
test('strips ### prefix from h3 cell', () => {
153181
const block: DeepnoteBlock = {
154182
content: 'old content',
@@ -163,6 +191,20 @@ suite('TextBlockConverter', () => {
163191
assert.strictEqual(block.content, 'New Subsection');
164192
});
165193

194+
test('strips ### prefix with leading whitespace from h3 cell', () => {
195+
const block: DeepnoteBlock = {
196+
content: 'old content',
197+
id: 'block-123',
198+
sortingKey: 'a0',
199+
type: 'text-cell-h3'
200+
};
201+
const cell = new NotebookCellData(NotebookCellKind.Markup, '\t### New Subsection', 'markdown');
202+
203+
converter.applyChangesToBlock(block, cell);
204+
205+
assert.strictEqual(block.content, 'New Subsection');
206+
});
207+
166208
test('does not strip prefix from paragraph cell', () => {
167209
const block: DeepnoteBlock = {
168210
content: 'old content',

src/notebooks/deepnote/deepnoteTypes.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ export interface DeepnoteNotebook {
3434
* Can be either a code block or a markdown block.
3535
*/
3636
export interface DeepnoteBlock {
37+
blockGroup?: string;
3738
content: string;
3839
executionCount?: number;
3940
id: string;

src/notebooks/deepnote/pocket.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import type { NotebookCellData } from 'vscode';
33
import type { DeepnoteBlock, DeepnoteOutput } from './deepnoteTypes';
44
import { generateBlockId, generateSortingKey } from './dataConversionUtils';
55

6-
const deepnoteBlockSpecificFields: string[] = ['blockGroup', 'executionCount', 'id', 'outputs', 'sortingKey', 'type'];
6+
const deepnoteBlockSpecificFields = ['blockGroup', 'executionCount', 'id', 'outputs', 'sortingKey', 'type'] as const;
77

88
// Stores extra Deepnote-specific fields for each block that are not part of the standard VSCode NotebookCellData structure.
99
export interface Pocket {
@@ -53,7 +53,7 @@ export function createBlockFromPocket(cell: NotebookCellData, index: number): De
5353
}
5454

5555
const block: DeepnoteBlock = {
56-
content: '',
56+
content: cell.value,
5757
id: pocket?.id || generateBlockId(),
5858
metadata,
5959
sortingKey: pocket?.sortingKey || generateSortingKey(index),
@@ -62,7 +62,7 @@ export function createBlockFromPocket(cell: NotebookCellData, index: number): De
6262

6363
// Only add optional fields if they exist
6464
if (pocket?.blockGroup) {
65-
(block as DeepnoteBlock & { blockGroup?: string }).blockGroup = pocket.blockGroup;
65+
block.blockGroup = pocket.blockGroup;
6666
}
6767

6868
if (pocket?.executionCount !== undefined) {

src/notebooks/deepnote/serialization.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ Converters are registered in a `ConverterRegistry` and retrieved based on block
3232

3333
When opening a Deepnote notebook in VS Code:
3434

35-
```
35+
```text
3636
Deepnote YAML File
3737
3838
Parse YAML (js-yaml)
@@ -58,7 +58,7 @@ VS Code NotebookData with cells
5858

5959
When saving a notebook in VS Code:
6060

61-
```
61+
```text
6262
VS Code NotebookData with cells
6363
6464
For each cell:
@@ -225,13 +225,15 @@ test('my-block-type round-trips correctly', () => {
225225
## Important Guidelines
226226

227227
### DO:
228+
228229
- ✅ Use the pocket system for Deepnote-specific fields
229230
- ✅ Preserve all block metadata during conversion
230231
- ✅ Test round-trip conversion (blocks → cells → blocks)
231232
- ✅ Handle both empty and undefined fields correctly
232233
- ✅ Use `assert.deepStrictEqual()` for object comparisons in tests
233234

234235
### DON'T:
236+
235237
- ❌ Store Deepnote-specific data directly in cell metadata (use the pocket)
236238
- ❌ Modify the pocket in converters (it's managed automatically)
237239
- ❌ Assume all optional fields exist (check for undefined)

0 commit comments

Comments
 (0)