feat: SQL block variable input and integration switching #86
Conversation
📝 WalkthroughWalkthroughThe status bar provider now returns multiple NotebookCellStatusBarItem entries per SQL cell: an integration-status item and a persistent variable-name item. It adds LocalQuickPickItem, registers commands Sequence Diagram(s)sequenceDiagram
participant UI as Status Bar / Commands
participant Provider as sqlCellStatusBarProvider
participant CellMeta as Cell Metadata
participant IntegrStore as Integration Store / QuickPick
participant WS as WorkspaceEdit
UI->>Provider: provideCellStatusBarItems(cell)
Provider->>CellMeta: read sql_integration_id, deepnote_variable_name
Provider->>IntegrStore: fetch integrations (if project)
alt integration == DATAFRAME_SQL_INTEGRATION_ID
Provider->>Provider: build DuckDB/DataFrame item
else integration known
Provider->>Provider: build integration item (switch)
else none/unknown
Provider->>Provider: build "No integration connected" item
end
Provider->>Provider: build variable-name item
Provider-->>UI: return [integrationItem, variableItem]
UI->>Provider: executeCommand(deepnote.updateSqlVariableName, cell)
Provider->>UI: showInputBox(currentName)
alt valid input
Provider->>WS: apply metadata.deepnote_variable_name = newName
WS->>CellMeta: metadata updated
Provider-->>UI: fire onDidChangeCellStatusBarItems(cell)
else cancel/invalid
Provider-->>UI: no change
end
UI->>Provider: executeCommand(deepnote.switchSqlIntegration, cell)
Provider->>IntegrStore: present QuickPick (configured + DuckDB + Configure option)
UI->>Provider: select option
alt choose integration
Provider->>WS: apply metadata.sql_integration_id = selectedId
WS->>CellMeta: metadata updated
Provider-->>UI: fire onDidChangeCellStatusBarItems(cell)
else choose Configure current integration
Provider->>IntegrStore: open integration management UI
else cancel
Provider-->>UI: no change
end
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/!(*.node|*.web).ts📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/notebooks/deepnote/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (8)📚 Learning: 2025-10-20T15:03:43.208ZApplied to files:
📚 Learning: 2025-10-21T15:14:08.102ZApplied to files:
📚 Learning: 2025-10-20T15:08:08.268ZApplied to files:
📚 Learning: 2025-10-20T15:07:05.323ZApplied to files:
📚 Learning: 2025-10-07T15:31:30.150ZApplied to files:
📚 Learning: 2025-10-22T11:56:07.943ZApplied to files:
📚 Learning: 2025-09-30T12:34:31.202ZApplied to files:
📚 Learning: 2025-08-18T23:41:22.727ZApplied to files:
🧬 Code graph analysis (1)src/notebooks/deepnote/sqlCellStatusBarProvider.ts (3)
🔇 Additional comments (10)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (2)
128-166: Use class constructor + enum alignment for integration item.Same issue here; build via constructor and drop magic number.
- if (integrationId === DATAFRAME_SQL_INTEGRATION_ID) { - return { - text: `$(database) ${l10n.t('DataFrame SQL (DuckDB)')}`, - alignment: 1, // NotebookCellStatusBarAlignment.Left - tooltip: l10n.t('Internal DuckDB integration for querying DataFrames\nClick to switch'), - command: { - title: l10n.t('Switch Integration'), - command: 'deepnote.switchSqlIntegration', - arguments: [cell] - } - }; - } + if (integrationId === DATAFRAME_SQL_INTEGRATION_ID) { + const sb = new NotebookCellStatusBarItem( + `$(database) ${l10n.t('DataFrame SQL (DuckDB)')}`, + NotebookCellStatusBarAlignment.Left + ); + sb.tooltip = l10n.t('Internal DuckDB integration for querying DataFrames\nClick to switch'); + sb.command = { title: l10n.t('Switch Integration'), command: 'deepnote.switchSqlIntegration', arguments: [cell] }; + return sb; + } @@ - return { - text: `$(database) ${displayName}`, - alignment: 1, // NotebookCellStatusBarAlignment.Left - tooltip: l10n.t('SQL Integration: {0}\nClick to switch or configure', displayName), - command: { - title: l10n.t('Switch Integration'), - command: 'deepnote.switchSqlIntegration', - arguments: [cell] - } - }; + const sb = new NotebookCellStatusBarItem( + `$(database) ${displayName}`, + NotebookCellStatusBarAlignment.Left + ); + sb.tooltip = l10n.t('SQL Integration: {0}\nClick to switch or configure', displayName); + sb.command = { title: l10n.t('Switch Integration'), command: 'deepnote.switchSqlIntegration', arguments: [cell] }; + return sb;
69-83: Propagate CancellationToken into async helpers.createStatusBarItems performs awaits; pass token downstream and bail early if cancelled to avoid wasted work.
- public provideCellStatusBarItems( + public provideCellStatusBarItems( cell: NotebookCell, token: CancellationToken ): ProviderResult<NotebookCellStatusBarItem | NotebookCellStatusBarItem[]> { @@ - return this.createStatusBarItems(cell); + return this.createStatusBarItems(cell, token); } @@ - private async createStatusBarItems(cell: NotebookCell): Promise<NotebookCellStatusBarItem[]> { + private async createStatusBarItems(cell: NotebookCell, token?: CancellationToken): Promise<NotebookCellStatusBarItem[]> { const items: NotebookCellStatusBarItem[] = []; @@ - if (integrationId) { + if (token?.isCancellationRequested) return items; + if (integrationId) { const integrationItem = await this.createIntegrationStatusBarItem(cell, integrationId); if (integrationItem) { items.push(integrationItem); } } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts(5 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (2)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (2)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)
🔇 Additional comments (1)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (1)
24-25: Dismiss review comment—current import and usage are correct.The file already imports from the canonical source (
src/platform/notebooks/deepnote/integrationTypes). The enum defined there supports theIntegrationType.PostgresandIntegrationType.BigQuerymember access used at lines 324–326. The alternative type insrc/webviews/webview-side/integrations/types.tsis a string union ('postgres' | 'bigquery') and lacks enum members—switching to it would cause a compile error.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
🧠 Learnings (7)
📚 Learning: 2025-10-20T15:03:43.182Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:98-126
Timestamp: 2025-10-20T15:03:43.182Z
Learning: In VS Code's notebook API, NotebookCellStatusBarItem is an interface, not a class with a constructor. Status bar items should be created as plain object literals that conform to the NotebookCellStatusBarItem interface. Do not suggest using `new NotebookCellStatusBarItem(...)` in TypeScript files working with VS Code notebooks.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-20T15:08:08.244Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts:51-64
Timestamp: 2025-10-20T15:08:08.244Z
Learning: In the vscode-deepnote test files (*.unit.test.ts), `NotebookCellStatusBarAlignment` cannot be imported from 'vscode' and is not available in the test environment. Use the numeric value (1 for Left alignment) directly in assertions instead.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-20T15:07:05.268Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:277-280
Timestamp: 2025-10-20T15:07:05.268Z
Learning: In VS Code QuickPickItem API, when creating a separator using QuickPickItemKind.Separator, the label property is required (even if empty). Do not suggest removing the label property from separator items.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-07T15:31:30.150Z
Learnt from: hc2p
PR: deepnote/deepnote-internal#18615
File: apps/webapp-client/src/api/integration-api.ts:204-204
Timestamp: 2025-10-07T15:31:30.150Z
Learning: In apps/webapp-client/src/api/integration-api.ts, when reviewing bug fixes authored by the PR creator (e.g., changing integration.state to integration.type), use informational tone rather than marking as critical issues, since the fix is already implemented.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-30T12:33:33.332Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-30T12:33:33.332Z
Learning: Applies to **/*.{ts,tsx} : Do not use TypeScript enum; prefer string literal unions or const objects (or zod enums when runtime needed)
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-30T12:34:31.202Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: .cursor/rules/base.mdc:0-0
Timestamp: 2025-09-30T12:34:31.202Z
Learning: Applies to **/*.{ts,tsx} : Use string literal unions or zod schemas instead of TypeScript enum
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-08-18T23:41:22.727Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-08-18T23:41:22.727Z
Learning: Applies to **/*.{ts,tsx} : Prefer string literal unions or zod schemas over TypeScript enum
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
🔇 Additional comments (3)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (3)
1-67: Command registration looks good.Imports are organized correctly and commands are properly wired to their handlers.
128-166: Integration item creation handles DuckDB and missing projectId correctly.
168-181: Variable item now properly localized.The prefix is now internationalized using
l10n.twith placeholder. Good improvement.
jankuca
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
jankuca
force-pushed
the
jk/feat/sql-block-variable-input
branch
from
3b06e78 to
10b6569
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
======================================
Coverage 71% 71%
======================================
Files 527 527
Lines 39500 39864 +364
Branches 4935 4961 +26
======================================
+ Hits 28312 28650 +338
- Misses 9547 9562 +15
- Partials 1641 1652 +11
🚀 New features to boost your workflow:
|
jankuca
force-pushed
the
jk/feat/sql-block-variable-input
branch
2 times, most recently
from
7f45c61 to
ddfc066
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (1)
143-165: Also assert command/alignment for unknown-config case.Keeps UI guarantees consistent across scenarios.
assert.isDefined(result); assert.isArray(result); const items = result as any[]; assert.strictEqual(items.length, 2); assert.strictEqual(items[0].text, '$(database) Unknown integration (configure)'); assert.strictEqual(items[1].text, 'Variable: df'); + // Command and alignment stay consistent + assert.strictEqual(items[0].alignment, 1); + assert.isDefined(items[0].command); + assert.strictEqual(items[0].command.command, 'deepnote.switchSqlIntegration');NotebookCellStatusBarAlignment enum isn’t available in tests; numeric 1 is intended. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts(5 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
🧠 Learnings (9)
📚 Learning: 2025-10-20T15:08:08.268Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts:51-64
Timestamp: 2025-10-20T15:08:08.268Z
Learning: In the vscode-deepnote test files (*.unit.test.ts), `NotebookCellStatusBarAlignment` cannot be imported from 'vscode' and is not available in the test environment. Use the numeric value (1 for Left alignment) directly in assertions instead.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-14T13:43:36.364Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T13:43:36.364Z
Learning: Applies to packages/*/src/**/*.test.{ts,tsx} : Follow existing test patterns (see packages/blocks/src/blocks/*.test.ts); write clear names, group with describe(), cover edge cases, errors, special characters, and verify exact output for code-generation functions
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
📚 Learning: 2025-10-20T15:03:43.208Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:98-126
Timestamp: 2025-10-20T15:03:43.208Z
Learning: In VS Code's notebook API, NotebookCellStatusBarItem is an interface, not a class with a constructor. Status bar items should be created as plain object literals that conform to the NotebookCellStatusBarItem interface. Do not suggest using `new NotebookCellStatusBarItem(...)` in TypeScript files working with VS Code notebooks.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-20T15:07:05.323Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:277-280
Timestamp: 2025-10-20T15:07:05.323Z
Learning: In VS Code QuickPickItem API, when creating a separator using QuickPickItemKind.Separator, the label property is required (even if empty). Do not suggest removing the label property from separator items.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-07T15:31:30.150Z
Learnt from: hc2p
PR: deepnote/deepnote-internal#18615
File: apps/webapp-client/src/api/integration-api.ts:204-204
Timestamp: 2025-10-07T15:31:30.150Z
Learning: In apps/webapp-client/src/api/integration-api.ts, when reviewing bug fixes authored by the PR creator (e.g., changing integration.state to integration.type), use informational tone rather than marking as critical issues, since the fix is already implemented.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-30T12:33:33.332Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-30T12:33:33.332Z
Learning: Applies to **/*.{ts,tsx} : Do not use TypeScript enum; prefer string literal unions or const objects (or zod enums when runtime needed)
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-30T12:34:31.202Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: .cursor/rules/base.mdc:0-0
Timestamp: 2025-09-30T12:34:31.202Z
Learning: Applies to **/*.{ts,tsx} : Use string literal unions or zod schemas instead of TypeScript enum
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-08-18T23:41:22.727Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-08-18T23:41:22.727Z
Learning: Applies to **/*.{ts,tsx} : Prefer string literal unions or zod schemas over TypeScript enum
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (2)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (4)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/test/mocks/vsc/extHostedTypes.ts (2)
WorkspaceEdit(649-833)NotebookEdit(2444-2507)src/test/datascience/ipywidgets/samples/beakerx/extension.js (1)
workspace(21144-21144)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)
🔇 Additional comments (1)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (1)
330-339: Type label mapping looks good.String enum values keep the default branch safe; localized labels for known types are clear.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (1)
184-187: Add alignment assert for consistency.After asserting text, also assert alignment === 1 for the single variable item (enum not available).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
🧠 Learnings (3)
📚 Learning: 2025-10-20T15:08:08.268Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts:51-64
Timestamp: 2025-10-20T15:08:08.268Z
Learning: In the vscode-deepnote test files (*.unit.test.ts), `NotebookCellStatusBarAlignment` cannot be imported from 'vscode' and is not available in the test environment. Use the numeric value (1 for Left alignment) directly in assertions instead.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
📚 Learning: 2025-10-14T13:43:36.364Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T13:43:36.364Z
Learning: Applies to packages/*/src/**/*.test.{ts,tsx} : Follow existing test patterns (see packages/blocks/src/blocks/*.test.ts); write clear names, group with describe(), cover edge cases, errors, special characters, and verify exact output for code-generation functions
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
🪛 GitHub Actions: CI
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
[error] 357-357: ESLint: Unexpected empty method 'dispose'. (no-empty-function)
🪛 GitHub Check: Lint & Format
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
[failure] 473-473:
Unexpected empty method 'dispose'
[failure] 357-357:
Unexpected empty method 'dispose'
🔇 Additional comments (7)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (7)
55-68: Good coverage for “no integration” path.Assertions cover both items, commands, and alignment=1 (enum unavailable in tests). Looks solid. Based on learnings
82-99: DuckDB/DataFrame case verified well.Text, tooltip, alignment, and command wiring are exercised; variable item also validated.
131-145: Integration-resolved path looks correct.Checks include text, alignment, command id, and arguments [cell].
163-169: Unknown integration fallback OK.Two items returned with expected texts.
220-223: Custom variable name scenario covered.Confirms metadata-driven label.
297-313: Event propagation test is precise.Validates onDidChangeCellStatusBarItems fires on integration changes.
352-359: Change empty dispose function to return undefined—but clarify with a comment instead.The proposed fix works around ESLint's
no-empty-functionrule, but returningundefinedis semantically odd for a mock dispose function. Better approaches:
Add an ESLint disable comment:
return { dispose: () => { /* eslint-disable-next-line no-empty-function */ } };Or add an explanatory comment:
return { dispose: () => { /* intentionally empty mock */ } };Or keep the undefined return with a reason:
return { dispose: () => undefined }; // mock dispose, no cleanup neededThe
undefinedreturn technically fixes the lint error but obscures intent. If you proceed with this change, add a comment explaining why.
jankuca
force-pushed
the
jk/feat/sql-block-variable-input
branch
from
ddfc066 to
4566419
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (2)
341-351: Fix ESLint warning: avoid empty dispose.Return a no-op without an empty function body.
- return { - dispose: () => { - return; - } - }; + return { dispose: () => undefined };
463-471: Fix ESLint warning: avoid empty dispose.Same as above for switch command stub.
- return { - dispose: () => { - return; - } - }; + return { dispose: () => undefined };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts(5 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
🧠 Learnings (9)
📚 Learning: 2025-10-20T15:08:08.268Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts:51-64
Timestamp: 2025-10-20T15:08:08.268Z
Learning: In the vscode-deepnote test files (*.unit.test.ts), `NotebookCellStatusBarAlignment` cannot be imported from 'vscode' and is not available in the test environment. Use the numeric value (1 for Left alignment) directly in assertions instead.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-14T13:43:36.364Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T13:43:36.364Z
Learning: Applies to packages/*/src/**/*.test.{ts,tsx} : Follow existing test patterns (see packages/blocks/src/blocks/*.test.ts); write clear names, group with describe(), cover edge cases, errors, special characters, and verify exact output for code-generation functions
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
📚 Learning: 2025-10-20T15:03:43.208Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:98-126
Timestamp: 2025-10-20T15:03:43.208Z
Learning: In VS Code's notebook API, NotebookCellStatusBarItem is an interface, not a class with a constructor. Status bar items should be created as plain object literals that conform to the NotebookCellStatusBarItem interface. Do not suggest using `new NotebookCellStatusBarItem(...)` in TypeScript files working with VS Code notebooks.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-20T15:07:05.323Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:277-280
Timestamp: 2025-10-20T15:07:05.323Z
Learning: In VS Code QuickPickItem API, when creating a separator using QuickPickItemKind.Separator, the label property is required (even if empty). Do not suggest removing the label property from separator items.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-07T15:31:30.150Z
Learnt from: hc2p
PR: deepnote/deepnote-internal#18615
File: apps/webapp-client/src/api/integration-api.ts:204-204
Timestamp: 2025-10-07T15:31:30.150Z
Learning: In apps/webapp-client/src/api/integration-api.ts, when reviewing bug fixes authored by the PR creator (e.g., changing integration.state to integration.type), use informational tone rather than marking as critical issues, since the fix is already implemented.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-30T12:33:33.332Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-30T12:33:33.332Z
Learning: Applies to **/*.{ts,tsx} : Do not use TypeScript enum; prefer string literal unions or const objects (or zod enums when runtime needed)
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-30T12:34:31.202Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: .cursor/rules/base.mdc:0-0
Timestamp: 2025-09-30T12:34:31.202Z
Learning: Applies to **/*.{ts,tsx} : Use string literal unions or zod schemas instead of TypeScript enum
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-08-18T23:41:22.727Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-08-18T23:41:22.727Z
Learning: Applies to **/*.{ts,tsx} : Prefer string literal unions or zod schemas over TypeScript enum
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (5)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)src/platform/notebooks/deepnote/types.ts (2)
IIntegrationStorage(6-6)IIntegrationStorage(7-42)src/test/vscode-mock.ts (1)
resetVSCodeMocks(60-79)src/test/mocks/vsc/index.ts (1)
EventEmitter(65-89)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (2)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Test
- GitHub Check: Build & Package Extension
🔇 Additional comments (3)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (3)
51-75: Defensive command guards: nice.Optional cell param with early error is correct and prevents palette crashes.
106-135: Status bar assembly looks solid.Correct SQL-only gating, “no integration” case handled, and variable item always added with priority for order.
179-193: Variable item: trimmed validation pipeline aligns with UX.Text, alignment, priority, tooltip, and command are consistent. Good.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts(5 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
🧠 Learnings (10)
📚 Learning: 2025-10-20T15:03:43.208Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:98-126
Timestamp: 2025-10-20T15:03:43.208Z
Learning: In VS Code's notebook API, NotebookCellStatusBarItem is an interface, not a class with a constructor. Status bar items should be created as plain object literals that conform to the NotebookCellStatusBarItem interface. Do not suggest using `new NotebookCellStatusBarItem(...)` in TypeScript files working with VS Code notebooks.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-20T15:08:08.268Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts:51-64
Timestamp: 2025-10-20T15:08:08.268Z
Learning: In the vscode-deepnote test files (*.unit.test.ts), `NotebookCellStatusBarAlignment` cannot be imported from 'vscode' and is not available in the test environment. Use the numeric value (1 for Left alignment) directly in assertions instead.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
📚 Learning: 2025-10-20T15:07:05.323Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:277-280
Timestamp: 2025-10-20T15:07:05.323Z
Learning: In VS Code QuickPickItem API, when creating a separator using QuickPickItemKind.Separator, the label property is required (even if empty). Do not suggest removing the label property from separator items.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-07T15:31:30.150Z
Learnt from: hc2p
PR: deepnote/deepnote-internal#18615
File: apps/webapp-client/src/api/integration-api.ts:204-204
Timestamp: 2025-10-07T15:31:30.150Z
Learning: In apps/webapp-client/src/api/integration-api.ts, when reviewing bug fixes authored by the PR creator (e.g., changing integration.state to integration.type), use informational tone rather than marking as critical issues, since the fix is already implemented.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-30T12:33:33.332Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-30T12:33:33.332Z
Learning: Applies to **/*.{ts,tsx} : Do not use TypeScript enum; prefer string literal unions or const objects (or zod enums when runtime needed)
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-30T12:34:31.202Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: .cursor/rules/base.mdc:0-0
Timestamp: 2025-09-30T12:34:31.202Z
Learning: Applies to **/*.{ts,tsx} : Use string literal unions or zod schemas instead of TypeScript enum
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-08-18T23:41:22.727Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-08-18T23:41:22.727Z
Learning: Applies to **/*.{ts,tsx} : Prefer string literal unions or zod schemas over TypeScript enum
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/**/*.ts : Add unit and integration tests for new features and modifications; include performance and cross-platform test coverage
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
📚 Learning: 2025-10-14T13:43:36.364Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T13:43:36.364Z
Learning: Applies to packages/*/src/**/*.test.{ts,tsx} : Follow existing test patterns (see packages/blocks/src/blocks/*.test.ts); write clear names, group with describe(), cover edge cases, errors, special characters, and verify exact output for code-generation functions
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (3)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/test/mocks/vsc/extHostedTypes.ts (2)
WorkspaceEdit(649-833)NotebookEdit(2444-2507)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (2)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/platform/notebooks/deepnote/types.ts (2)
IIntegrationStorage(6-6)IIntegrationStorage(7-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build & Package Extension
- GitHub Check: Build & Test
- GitHub Check: Lint & Format
🔇 Additional comments (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (2)
51-71: Good: commands are guarded for palette invocation.Early-return with a localized error when no cell is passed is correct.
24-24: IntegrationType is properly exported; no compilation issue.The symbol is correctly exported from
src/platform/notebooks/deepnote/integrationTypes.tsas an enum at line 10. The import atsqlCellStatusBarProvider.ts:24will compile successfully.Note: A duplicate
IntegrationTypedefinition exists insrc/webviews/webview-side/integrations/types.tsas a union type, creating two separate type definitions in the codebase. Consider consolidating to a single source of truth if both are actively used.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (2)
51-56: Also refresh on notebook cell metadata changes (external edits).To keep the status bar in sync when metadata changes outside these commands, listen to notebook document changes for Deepnote notebooks.
this.disposables.push( this.integrationStorage.onDidChangeIntegrations(() => { this._onDidChangeCellStatusBarItems.fire(); }) ); + +// Refresh when any Deepnote notebook changes (e.g., metadata updated externally) +this.disposables.push( + workspace.onDidChangeNotebookDocument((e) => { + if (e.document.notebookType === 'deepnote') { + this._onDidChangeCellStatusBarItems.fire(); + } + }) +);
100-111: Optional: constants for metadata keys to reduce typo risk.Small hygiene to avoid key drift.
+// Metadata keys +const SQL_INTEGRATION_ID_KEY = 'sql_integration_id' as const; +const DEEPNOTE_VARIABLE_NAME_KEY = 'deepnote_variable_name' as const; @@ - const integrationId = (metadata as Record<string, unknown>).sql_integration_id; + const integrationId = (metadata as Record<string, unknown>)[SQL_INTEGRATION_ID_KEY]; @@ - const variableName = (metadata as Record<string, unknown>).deepnote_variable_name; + const variableName = (metadata as Record<string, unknown>)[DEEPNOTE_VARIABLE_NAME_KEY]; @@ - deepnote_variable_name: newVariableName + [DEEPNOTE_VARIABLE_NAME_KEY]: newVariableName @@ - sql_integration_id: selectedId + [SQL_INTEGRATION_ID_KEY]: selectedIdAlso applies to: 202-213, 244-245, 350-355
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
🧠 Learnings (8)
📚 Learning: 2025-10-20T15:03:43.208Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:98-126
Timestamp: 2025-10-20T15:03:43.208Z
Learning: In VS Code's notebook API, NotebookCellStatusBarItem is an interface, not a class with a constructor. Status bar items should be created as plain object literals that conform to the NotebookCellStatusBarItem interface. Do not suggest using `new NotebookCellStatusBarItem(...)` in TypeScript files working with VS Code notebooks.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-21T15:14:08.083Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:118-128
Timestamp: 2025-10-21T15:14:08.083Z
Learning: In the vscode-deepnote project, `NotebookCellStatusBarAlignment` cannot be imported from 'vscode'. Always use numeric values with inline comments (e.g., `alignment: 1, // NotebookCellStatusBarAlignment.Left`) instead of attempting to import and use the enum constant.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-20T15:08:08.268Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts:51-64
Timestamp: 2025-10-20T15:08:08.268Z
Learning: In the vscode-deepnote test files (*.unit.test.ts), `NotebookCellStatusBarAlignment` cannot be imported from 'vscode' and is not available in the test environment. Use the numeric value (1 for Left alignment) directly in assertions instead.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-20T15:07:05.323Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:277-280
Timestamp: 2025-10-20T15:07:05.323Z
Learning: In VS Code QuickPickItem API, when creating a separator using QuickPickItemKind.Separator, the label property is required (even if empty). Do not suggest removing the label property from separator items.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-07T15:31:30.150Z
Learnt from: hc2p
PR: deepnote/deepnote-internal#18615
File: apps/webapp-client/src/api/integration-api.ts:204-204
Timestamp: 2025-10-07T15:31:30.150Z
Learning: In apps/webapp-client/src/api/integration-api.ts, when reviewing bug fixes authored by the PR creator (e.g., changing integration.state to integration.type), use informational tone rather than marking as critical issues, since the fix is already implemented.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-30T12:33:33.332Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-30T12:33:33.332Z
Learning: Applies to **/*.{ts,tsx} : Do not use TypeScript enum; prefer string literal unions or const objects (or zod enums when runtime needed)
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-30T12:34:31.202Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: .cursor/rules/base.mdc:0-0
Timestamp: 2025-09-30T12:34:31.202Z
Learning: Applies to **/*.{ts,tsx} : Use string literal unions or zod schemas instead of TypeScript enum
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-08-18T23:41:22.727Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-08-18T23:41:22.727Z
Learning: Applies to **/*.{ts,tsx} : Prefer string literal unions or zod schemas over TypeScript enum
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (3)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/test/mocks/vsc/extHostedTypes.ts (2)
WorkspaceEdit(649-833)NotebookEdit(2444-2507)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Package Extension
- GitHub Check: Build & Test
🔇 Additional comments (12)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (12)
7-16: Imports grouping looks good.External vs local imports are separated; added VS Code symbols are used. LGTM.
26-31: Typed QuickPick items: nice and safe.LocalQuickPickItem eliminates casts later. LGTM.
58-78: Defensive command handlers: good guards.Graceful handling when invoked from the palette without a cell. LGTM.
113-142: Multi-item composition with priorities: solid.Deterministic order via priority; numeric alignment with inline comment is correct for this repo. LGTM. Based on learnings.
124-135: “No integration connected” item: correct API usage.Object literal conforms to NotebookCellStatusBarItem; numeric alignment with comment is appropriate here. LGTM. Based on learnings.
148-161: DuckDB/DataFrame special case: LGTM.Clear tooltip and command wiring; priority set.
172-183: Integration item with fallback label: LGTM.Good l10n use; correct command.
186-200: Variable item: localized and prioritized.Validation entry point wired correctly. LGTM.
202-213: Variable name read with sensible default.Default 'df' is fine; access guarded. LGTM.
214-254: Update flow: trimmed input and edit result check.Good UX and robustness; error surfaced on failure; event fired on success. LGTM.
256-365: Integration switching flow is robust.Unknown-current handling, separator correctness, early no-op, and edit-result check all look good. LGTM. Based on learnings.
23-25: Import uses canonical IntegrationType—verified.The file correctly imports from
src/platform/notebooks/deepnote/integrationTypes.ts, which is the canonical enum. A duplicate type exists insrc/webviews/webview-side/integrations/types.ts, but it's isolated to webviews and doesn't contaminate this import.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (1)
178-181: Guard integration config fetch; degrade gracefully on errors.Avoids unhandled rejection breaking the provider.
-const config = await this.integrationStorage.getProjectIntegrationConfig(projectId, integrationId); -const displayName = config?.name || l10n.t('Unknown integration (configure)'); +let displayName = l10n.t('Unknown integration (configure)'); +try { + const config = await this.integrationStorage.getProjectIntegrationConfig(projectId, integrationId); + displayName = config?.name || displayName; +} catch { + // keep fallback +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
🧠 Learnings (8)
📚 Learning: 2025-10-20T15:03:43.208Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:98-126
Timestamp: 2025-10-20T15:03:43.208Z
Learning: In VS Code's notebook API, NotebookCellStatusBarItem is an interface, not a class with a constructor. Status bar items should be created as plain object literals that conform to the NotebookCellStatusBarItem interface. Do not suggest using `new NotebookCellStatusBarItem(...)` in TypeScript files working with VS Code notebooks.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-21T15:14:08.083Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:118-128
Timestamp: 2025-10-21T15:14:08.083Z
Learning: In the vscode-deepnote project, `NotebookCellStatusBarAlignment` cannot be imported from 'vscode'. Always use numeric values with inline comments (e.g., `alignment: 1, // NotebookCellStatusBarAlignment.Left`) instead of attempting to import and use the enum constant.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-20T15:08:08.268Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts:51-64
Timestamp: 2025-10-20T15:08:08.268Z
Learning: In the vscode-deepnote test files (*.unit.test.ts), `NotebookCellStatusBarAlignment` cannot be imported from 'vscode' and is not available in the test environment. Use the numeric value (1 for Left alignment) directly in assertions instead.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-20T15:07:05.323Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#86
File: src/notebooks/deepnote/sqlCellStatusBarProvider.ts:277-280
Timestamp: 2025-10-20T15:07:05.323Z
Learning: In VS Code QuickPickItem API, when creating a separator using QuickPickItemKind.Separator, the label property is required (even if empty). Do not suggest removing the label property from separator items.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-07T15:31:30.150Z
Learnt from: hc2p
PR: deepnote/deepnote-internal#18615
File: apps/webapp-client/src/api/integration-api.ts:204-204
Timestamp: 2025-10-07T15:31:30.150Z
Learning: In apps/webapp-client/src/api/integration-api.ts, when reviewing bug fixes authored by the PR creator (e.g., changing integration.state to integration.type), use informational tone rather than marking as critical issues, since the fix is already implemented.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-30T12:33:33.332Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-30T12:33:33.332Z
Learning: Applies to **/*.{ts,tsx} : Do not use TypeScript enum; prefer string literal unions or const objects (or zod enums when runtime needed)
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-09-30T12:34:31.202Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: .cursor/rules/base.mdc:0-0
Timestamp: 2025-09-30T12:34:31.202Z
Learning: Applies to **/*.{ts,tsx} : Use string literal unions or zod schemas instead of TypeScript enum
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-08-18T23:41:22.727Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-08-18T23:41:22.727Z
Learning: Applies to **/*.{ts,tsx} : Prefer string literal unions or zod schemas over TypeScript enum
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (3)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookEdit(2444-2507)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build & Package Extension
- GitHub Check: Build & Test
- GitHub Check: Lint & Format
🔇 Additional comments (4)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (4)
123-152: Status bar composition looks solid.Correct item splitting, priorities, and numeric alignment with inline comment.
Based on learnings
196-210: Variable item: LGTM.Good localization, priority, and command wiring.
158-171: DuckDB special-case item: LGTM.Clear label, tooltip, and command.
377-386: No issues found. Import and usage are correct.The method imports
IntegrationTypefrom the correct source (src/platform/notebooks/deepnote/integrationTypes.ts—the enum, not the webview union). The switch statement covers all enum members exhaustively with a safe default fallback. No drift risk; the platform layer is internally consistent.
jankuca
requested review from
Artmann,
jamesbhobbs and
saltenasl
as code owners
|
@saltenasl FYI I re-tested the df sql (duckdb) and it seems to work ok for me 🤷🏻 duckdb.mov |
|
Merging this. If it still does not work for you, we will iterate and fix it in another way. |
Summary by CodeRabbit