-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Add support for project notebook management (create, rename, delete) #88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…tomaskislan/grn-4762-support-big-number-blocks
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…maskislan/grn-4776-support-input-blocks
…maskislan/grn-4776-support-input-blocks
…tomaskislan/grn-4762-support-big-number-blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/notebooks/deepnote/deepnoteExplorerView.ts (1)
227-238: Deep-clone notebooks when duplicating.
{ ...targetNotebook }and{ ...block }only make shallow copies. The new notebook ends up sharingoutputs,metadata, and any other nested structures with the original, so mutating the duplicate (e.g. running a cell that rewritesoutputs) also mutates the source notebook and can even lead to YAML anchors when dumping. Clone the notebook (and its blocks) deeply before reassigning IDs.- const newNotebook: DeepnoteNotebook = { - ...targetNotebook, - id: generateUuid(), - name: newName, - blocks: targetNotebook.blocks.map((block: DeepnoteBlock) => ({ - ...block, - id: generateUuid(), - blockGroup: generateUuid(), - executionCount: undefined, - ...(block.metadata != null ? { metadata: { ...block.metadata } } : {}) - })) - }; + const newNotebook = globalThis.structuredClone + ? (globalThis.structuredClone(targetNotebook) as DeepnoteNotebook) + : (JSON.parse(JSON.stringify(targetNotebook)) as DeepnoteNotebook); + newNotebook.id = generateUuid(); + newNotebook.name = newName; + newNotebook.blocks = newNotebook.blocks.map((block: DeepnoteBlock) => { + block.id = generateUuid(); + block.blockGroup = generateUuid(); + block.executionCount = undefined; + return block; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
package.json(3 hunks)src/commands.ts(2 hunks)src/notebooks/deepnote/deepnoteExplorerView.ts(4 hunks)src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts(2 hunks)src/platform/common/constants.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/!(*.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/commands.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteExplorerView.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/commands.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteExplorerView.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/deepnoteExplorerView.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/deepnoteExplorerView.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify decorators for DI: annotate classes with @Injectable() and inject dependencies with @Inject(Token)
Use the centralized logger (import { logger } from '../platform/logging') instead of console.log for application logging
Files:
src/platform/common/constants.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts (2)
src/test/vscode-mock.ts (1)
mockedVSCodeNamespaces(17-17)src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(24-102)
src/notebooks/deepnote/deepnoteExplorerView.ts (3)
src/notebooks/deepnote/deepnoteProjectUtils.ts (1)
readDeepnoteProjectFile(5-10)src/notebooks/deepnote/deepnoteTreeItem.ts (2)
DeepnoteTreeItem(24-102)DeepnoteTreeItemContext(15-19)src/platform/common/uuid.ts (1)
generateUuid(10-61)
🔇 Additional comments (7)
src/platform/common/constants.ts (1)
240-249: Command IDs added — looks consistent.
Names align with package.json and src/commands.ts.package.json (1)
908-961: Notebook toolbar remap — OK.
Ordering and new addInputDateRangeBlock entry look coherent.src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts (4)
740-904: createAndAddNotebookToProject tests — solid.
Covers happy path, cancel, and name suggestion.
906-1087: renameNotebook tests — solid.
Validates rename, early returns, metadata update, and messaging.
1090-1248: deleteNotebook tests — solid.
Confirms confirmation gate, YAML update, and messaging.
1250-1456: duplicateNotebook tests — solid.
Ensures IDs regenerated and executionCount cleared on copy.src/commands.ts (1)
189-211: Handlers not found; cannot confirm if [] is appropriate without seeing implementation.Commands are declared in package.json but no handler registrations exist in the codebase. If these will be tree context menu commands, they likely need context arguments (like
{ treeItem: TreeItem }or similar). Update type mappings if handlers receive arguments, otherwise confirm handlers ignore args.
saltenasl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all works well, except everytime i make a change the Deepnote explorer gets refreshed (and even resorted)
Screen.Recording.2025-10-31.at.16.49.35.mov
* feat(open-in-deepnote): button in toolbar * chore: fix tests
…ing deepnote container (#148)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/deepnoteTreeDataProvider.ts (2)
138-138: Replaceconsole.errorwithILoggerservice.Per coding guidelines, use the injected
ILoggerservice instead ofconsole.errorfor error logging.Based on coding guidelines.
196-196: Replaceconsole.errorwithILoggerservice.Per coding guidelines, use the injected
ILoggerservice instead ofconsole.error.Based on coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts(3 hunks)src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/!(*.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/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.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
Files:
src/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
Files:
src/notebooks/deepnote/deepnoteTreeDataProvider.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/deepnoteTreeDataProvider.unit.test.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (3)
src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)src/platform/deepnote/deepnoteTypes.ts (2)
DeepnoteProject(12-12)DeepnoteNotebook(18-18)src/notebooks/deepnote/deepnoteProjectUtils.ts (1)
readDeepnoteProjectFile(5-10)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (2)
src/platform/deepnote/deepnoteTypes.ts (1)
DeepnoteProject(12-12)src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)
🔇 Additional comments (5)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (4)
18-18: Good refactoring to centralize YAML parsing.Extracting file reading into a dedicated utility improves maintainability and reduces duplication.
143-148: LGTM: Correct alphabetical sorting implementation.Properly handles case-insensitive comparison with
localeCompareand includes defensive checks for non-string labels.
157-164: LGTM: Proper notebook sorting with immutability.Creates a copy before sorting (avoiding mutation) and applies consistent case-insensitive alphabetical ordering.
189-189: LGTM: Clean refactoring to use utility.Delegates YAML parsing to the centralized utility function.
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (1)
388-500: LGTM: Comprehensive notebook sorting tests.Both tests properly exercise the provider's
getChildrenmethod and verify alphabetical ordering, including case-insensitive behavior.
I've pushed a few fixes for this.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (2)
227-227: Use ILogger instead of console.error.Coding guidelines require ILogger service for error logging.
As per coding guidelines.
284-286: Use ILogger instead of console.error.Line 285 violates coding guidelines requiring ILogger service.
As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/!(*.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/deepnoteTreeDataProvider.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
Files:
src/notebooks/deepnote/deepnoteTreeDataProvider.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (3)
src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)src/platform/deepnote/deepnoteTypes.ts (2)
DeepnoteProject(12-12)DeepnoteNotebook(18-18)src/notebooks/deepnote/deepnoteProjectUtils.ts (1)
readDeepnoteProjectFile(5-10)
🔇 Additional comments (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (2)
18-18: LGTM: Extraction to shared utility.Delegating YAML parsing to
readDeepnoteProjectFileimproves maintainability.
232-237: Alphabetical sorting addresses reviewer feedback.Case-insensitive, locale-aware sorting aligns with PR objectives to fix resort behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/notebooks/deepnote/deepnoteActivationService.ts(1 hunks)src/notebooks/deepnote/deepnoteExplorerView.ts(5 hunks)src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts(4 hunks)src/notebooks/deepnote/deepnoteTreeDataProvider.ts(6 hunks)src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/!(*.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/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.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
Files:
src/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
Files:
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.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/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.ts
🧬 Code graph analysis (4)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (2)
src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
compareTreeItemsByLabel(24-28)
src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts (3)
src/test/vscode-mock.ts (1)
mockedVSCodeNamespaces(17-17)src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)src/notebooks/deepnote/deepnoteSerializer.ts (2)
DeepnoteFile(10-10)DeepnoteNotebook(10-10)
src/notebooks/deepnote/deepnoteExplorerView.ts (4)
src/notebooks/types.ts (2)
IDeepnoteNotebookManager(37-37)IDeepnoteNotebookManager(38-58)src/notebooks/deepnote/deepnoteProjectUtils.ts (1)
readDeepnoteProjectFile(5-10)src/notebooks/deepnote/deepnoteTreeItem.ts (2)
DeepnoteTreeItem(25-107)DeepnoteTreeItemContext(16-20)src/platform/common/uuid.ts (1)
generateUuid(10-61)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (4)
src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)src/platform/logging/index.ts (1)
logger(35-48)src/platform/deepnote/deepnoteTypes.ts (2)
DeepnoteProject(12-12)DeepnoteNotebook(18-18)src/notebooks/deepnote/deepnoteProjectUtils.ts (1)
readDeepnoteProjectFile(5-10)
⏰ 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). (1)
- GitHub Check: Build & Test
saltenasl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GRN-4767 GRN-4768 GRN-4772
Summary by CodeRabbit
New Features
UI
Improvements
Localization
Tests