feat: open in Deepnote command #132
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
📝 WalkthroughWalkthroughAdds a new public command Sequence Diagram(s)sequenceDiagram
actor User
participant VSCode
participant Handler as OpenInDeepnoteHandler
participant Client as ImportClient
participant API as Deepnote API
participant Browser
User->>VSCode: Trigger "Open in Deepnote" on .deepnote
VSCode->>Handler: Execute command
Handler->>Handler: Validate active editor, ensure saved, check MAX_FILE_SIZE
Handler->>Handler: Read file into Buffer
rect #D2E6FF
Note over Handler,Client: Init import (domain-resolved)
Handler->>Client: initImport(fileName, fileSize)
Client->>API: POST https://api.<domain>/v1/import/init
API-->>Client: { importId, uploadUrl, expiresAt } / error
Client-->>Handler: InitImportResponse or ApiError
end
rect #E6FFEA
Note over Handler,Client: Upload file (presigned URL)
Handler->>Client: uploadFile(uploadUrl, buffer, onProgress)
Client->>API: PUT uploadUrl (application/octet-stream)
API-->>Client: 200 OK / error
Client-->>Handler: progress callbacks (50%,100%) or ApiError
end
Handler->>VSCode: env.openExternal(import URL)
VSCode->>Browser: Open Deepnote import page
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
@coderabbitai ignore |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
======================================
- Coverage 72% 72% -1%
======================================
Files 541 545 +4
Lines 41595 41776 +181
Branches 5031 5047 +16
======================================
+ Hits 30322 30430 +108
- Misses 9593 9665 +72
- Partials 1680 1681 +1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
package.json(3 hunks)src/commands.ts(1 hunks)src/notebooks/deepnote/importClient.node.ts(1 hunks)src/notebooks/deepnote/importClient.unit.test.ts(1 hunks)src/notebooks/deepnote/openInDeepnoteHandler.node.ts(1 hunks)src/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.ts(1 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/platform/common/constants.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/!(*.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/platform/common/constants.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.tssrc/notebooks/deepnote/importClient.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/commands.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.tssrc/platform/common/constants.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.tssrc/notebooks/deepnote/importClient.node.tssrc/notebooks/deepnote/importClient.unit.test.tssrc/notebooks/serviceRegistry.node.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/notebooks/deepnote/openInDeepnoteHandler.node.tssrc/notebooks/deepnote/importClient.node.tssrc/notebooks/serviceRegistry.node.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/openInDeepnoteHandler.node.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.tssrc/notebooks/deepnote/importClient.node.tssrc/notebooks/deepnote/importClient.unit.test.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
**/*.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/openInDeepnoteHandler.node.unit.test.tssrc/notebooks/deepnote/importClient.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/openInDeepnoteHandler.node.unit.test.tssrc/notebooks/deepnote/importClient.unit.test.ts
🧬 Code graph analysis (3)
src/notebooks/deepnote/openInDeepnoteHandler.node.ts (2)
src/platform/logging/index.ts (1)
logger(35-48)src/notebooks/deepnote/importClient.node.ts (4)
MAX_FILE_SIZE(29-29)initImport(75-143)uploadFile(154-187)getErrorMessage(196-227)
src/notebooks/deepnote/importClient.node.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/importClient.unit.test.ts (1)
src/notebooks/deepnote/importClient.node.ts (3)
ApiError(21-24)getErrorMessage(196-227)MAX_FILE_SIZE(29-29)
⏰ 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 (7)
src/platform/common/constants.ts (1)
239-239: LGTM.Command id constant matches package.json and is scoped under Commands.
src/commands.ts (1)
200-201: LGTM.Accurate no-args mapping for OpenInDeepnote.
src/notebooks/deepnote/importClient.unit.test.ts (1)
7-164: LGTM (coverage for error messages and size).Solid checks for getErrorMessage and MAX_FILE_SIZE.
If logger noise is problematic, stub logger in tests to keep output clean.
src/notebooks/deepnote/openInDeepnoteHandler.node.ts (1)
25-29: LGTM (command registration).Registers once via DI; good.
src/notebooks/serviceRegistry.node.ts (2)
73-73: Import placement LGTM.Consistent with Deepnote imports cluster.
167-170: DI wiring and interface conformance verified; activation idempotency guaranteed by framework.OpenInDeepnoteHandler correctly implements IExtensionSyncActivationService. The framework calls each service's activate() exactly once: ExtensionActivationManager injects all IExtensionSyncActivationService implementations via multiInject and calls item.activate() for each, and the manager itself is invoked once during extension startup. The singleton registration ensures no duplicate instances. Command registration side-effects are safe.
src/notebooks/deepnote/importClient.node.ts (1)
29-29: MAX_FILE_SIZE enforcement already in place—review comment invalid.The handler in
openInDeepnoteHandler.node.ts(lines 66–71) already rejects files exceeding MAX_FILE_SIZE before callinginitImportat line 89. The size check occurs, and execution returns early if the limit is exceeded. No changes needed.Likely an incorrect or invalid review comment.
✅ Actions performedReviews paused. |
|
@coderabbitai full review only critical issues please |
|
@saltenasl: I'll perform a full review focusing only on critical issues. User feedback: Only flag critical issues. Skip minor issues, suggestions, and non-critical concerns. ✅ Actions performedFull review triggered. |
|
@coderabbitai are you still reviewing? its been 16min |
🧩 Analysis chain🏁 Script executed: Length of output: 39499 🏁 Script executed: Length of output: 613 @saltenasl: Apologies for the delay. The review appears to have stalled. Re-triggering now with focus on critical issues only. ✅ Actions performedFull review triggered. |
|
@coderabbitai please can you review this? |
|
@saltenasl: Reviewing now, focusing on critical issues only. User feedback: Only flag critical issues. Skip minor issues, suggestions, and non-critical concerns. ✅ Actions performedFull review triggered. |
|
@coderabbitai full review critical issues only |
|
@saltenasl: Triggering full review now. Will flag critical issues only. User feedback: Only flag critical issues. Skip minor issues, suggestions, and non-critical concerns. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.ts (2)
1-2: Remove MS copyright header from new files.
7-15: Minimal test coverage noted.
📜 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 (12)
package.json(3 hunks)src/commands.ts(1 hunks)src/notebooks/deepnote/blocks.md(4 hunks)src/notebooks/deepnote/deepnoteDataConverter.ts(2 hunks)src/notebooks/deepnote/deepnoteExplorerView.ts(2 hunks)src/notebooks/deepnote/importClient.node.ts(1 hunks)src/notebooks/deepnote/importClient.unit.test.ts(1 hunks)src/notebooks/deepnote/openInDeepnoteHandler.node.ts(1 hunks)src/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.ts(1 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/platform/common/constants.ts(1 hunks)src/platform/deepnote/pocket.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/!(*.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/platform/deepnote/pocket.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.tssrc/notebooks/deepnote/importClient.unit.test.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteDataConverter.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/platform/deepnote/pocket.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.tssrc/notebooks/deepnote/importClient.unit.test.tssrc/platform/common/constants.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/importClient.node.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/deepnote/pocket.tssrc/platform/common/constants.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.tssrc/notebooks/deepnote/importClient.node.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/openInDeepnoteHandler.node.unit.test.tssrc/notebooks/deepnote/importClient.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/openInDeepnoteHandler.node.unit.test.tssrc/notebooks/deepnote/importClient.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.tssrc/notebooks/deepnote/importClient.unit.test.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/importClient.node.tssrc/notebooks/deepnote/deepnoteExplorerView.tssrc/notebooks/deepnote/blocks.md
src/notebooks/deepnote/deepnoteDataConverter.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteDataConverter.ts performs Deepnote data transformations
Files:
src/notebooks/deepnote/deepnoteDataConverter.ts
🪛 GitHub Actions: CI
package.json
[error] 1-1: Test suite failed: npm test encountered a failing unit test (DeepnoteExplorerView).
Artmann
left a comment
Artmann
left a comment
There was a problem hiding this comment.
Looks good to me. I think coderabbit addressed all the issues.
| private createFallbackBlock(cell: NotebookCellData, index: number): DeepnoteBlock { | ||
| return { | ||
| blockGroup: 'default-group', | ||
| blockGroup: generateUuid(), |
| blockGroup: generateUuid(), | ||
| content: '', | ||
| executionCount: null, | ||
| executionCount: 0, |
There was a problem hiding this comment.
I think this should be NULL to signify that the block has not been executed. At least I think that's the reason that the field is nullable.
| /** | ||
| * Gets the API endpoint from configuration | ||
| */ | ||
| function getApiEndpoint(): string { |
There was a problem hiding this comment.
nitpick: Is getApiUrl a better name than getApiEndpoint?
|
@coderabbitai resolve i resolved your comment |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 45405 🏁 Script executed: Length of output: 8603 🏁 Script executed: Length of output: 1609 🏁 Script executed: Length of output: 3290 @saltenasl: Done! Successfully resolved all 20 review comment threads on PR #132. All issues flagged by CodeRabbit are now marked as resolved. |
* feat: open in Deepnote command * feat: disable ssl checks settings option * feat: rm disabling ssl checks option * feat: tweak config * feat: config name * fix: create new deepnote file with the correct attributes * fix: create new blocks in their own group instead of sharing a single one * chore: fix test
Contact me for testing details, the corresponding functionality in deepnote.com is currently only on our staging which requires special config.
Summary by CodeRabbit
New Features
Tests
Refactor