feat: loading state on startup #142
Conversation
📝 WalkthroughWalkthroughAdds an initial asynchronous scan lifecycle to the Deepnote tree provider that shows a Loading tree item ("Scanning for Deepnote projects...") until the scan completes. Introduces a new Sequence Diagram(s)sequenceDiagram
participant User
participant TreeView as Deepnote TreeView
participant Provider as DeepnoteTreeDataProvider
User->>TreeView: open or refresh view
TreeView->>Provider: getChildren(undefined)
alt initial scan incomplete
Provider->>Provider: start performInitialScan() (store initialScanPromise)
Provider->>TreeView: return [Loading item]
TreeView->>User: render "Scanning for Deepnote projects..."
par background
Provider->>Provider: performInitialScan() loads projects
Provider->>Provider: set isInitialScanComplete = true
Provider->>TreeView: fire refresh event
end
TreeView->>Provider: getChildren(undefined) (after refresh)
Provider->>TreeView: return [Project items]
TreeView->>User: render projects
else initial scan complete
Provider->>TreeView: return [Project items]
TreeView->>User: render projects
end
User->>TreeView: trigger "Reveal in Explorer"
alt selected item is Loading
Note right of TreeView: action disabled by `viewItem != loading`
TreeView-->>User: action unavailable
else selected item not Loading
TreeView->>User: reveal in Explorer
end
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #142 +/- ##
=====================================
Coverage 72% 72%
=====================================
Files 545 545
Lines 41795 41889 +94
Branches 5049 5058 +9
=====================================
+ Hits 30447 30518 +71
- Misses 9666 9683 +17
- Partials 1682 1688 +6
🚀 New features to boost your workflow:
|
dinohamzic
force-pushed
the
dinohamzic/grn-5031-loading-state-on-startup
branch
from
b1c1bcb to
ba17d87
Compare
dinohamzic
force-pushed
the
dinohamzic/grn-5031-loading-state-on-startup
branch
from
ba17d87 to
6fe639b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
dinohamzic
force-pushed
the
dinohamzic/grn-5031-loading-state-on-startup
branch
from
12fd98f to
feb25af
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 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.unit.test.ts(2 hunks)src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts(1 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/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.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/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.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/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.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/deepnoteTreeItem.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (1)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
DeepnoteTreeDataProvider(22-236)
src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts (1)
src/notebooks/deepnote/deepnoteTreeItem.ts (2)
DeepnoteTreeItemContext(16-20)DeepnoteTreeItem(25-107)
⏰ 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
🔇 Additional comments (1)
src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts (1)
583-600: LGTM.Properly validates loading item creation with null data and correct properties.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (1)
172-202: Conditional assertion still too permissive.Lines 192-197 use nested conditionals similar to the past review comment. After
refresh(), deterministically assert thatchildrenAfterRefresh[0].type === Loading(accounting for workspace presence), rather than optional checks.
📜 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.unit.test.ts(2 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/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
**/*.{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/deepnoteTreeDataProvider.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/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
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
DeepnoteTreeDataProvider(22-236)src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)
🔇 Additional comments (1)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (1)
245-263: Well-structured test.Correctly verifies that child element lookups never return loading items. Clean mock setup and deterministic assertion.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (3)
101-115: Missing transition verification.Test claims to verify "complete initial scan and show projects after loading" but doesn't assert loading item appears initially or disappears after scan.
Add assertions for state transition:
test('should complete initial scan and show projects after loading', async () => { const newProvider = new DeepnoteTreeDataProvider(); const loadingChildren = await newProvider.getChildren(); assert.isArray(loadingChildren); + // Verify loading state on first call (if workspace exists) + if (loadingChildren.length > 0) { + assert.strictEqual(loadingChildren[0].type, DeepnoteTreeItemType.Loading); + } await new Promise((resolve) => setTimeout(resolve, 10)); - // Second call should show actual projects (or empty array if no projects) const actualChildren = await newProvider.getChildren(); assert.isArray(actualChildren); + // Verify loading item is gone + const hasLoading = actualChildren.some((item) => item.type === DeepnoteTreeItemType.Loading); + assert.isFalse(hasLoading, 'Loading item should not appear after scan'); if (newProvider && typeof newProvider.dispose === 'function') { newProvider.dispose(); } });
163-184: Doesn't verify loading state after refresh.Test claims to reset initial scan state but doesn't assert loading item reappears after refresh.
Add assertion after refresh:
// Call refresh to reset state - this exercises the refresh logic newProvider.refresh(); - // After refresh - should work without errors const childrenAfterRefresh = await newProvider.getChildren(); assert.isArray(childrenAfterRefresh); + // Verify loading state is shown again (if workspace exists) + if (childrenAfterRefresh.length > 0) { + assert.strictEqual(childrenAfterRefresh[0].type, DeepnoteTreeItemType.Loading); + } if (newProvider && typeof newProvider.dispose === 'function') { newProvider.dispose(); } });
88-99: Test name doesn't match implementation.Claims to verify "loading item on first call with correct properties" but only asserts an array is returned. No verification of loading item type, properties, or presence.
Either mock workspace folders to ensure deterministic loading behavior and add assertions:
- test('should return loading item on first call with correct properties', async () => { + test('should return loading item on first call with correct properties', async () => { const newProvider = new DeepnoteTreeDataProvider(); - // First call - just verify it returns an array and doesn't throw const children = await newProvider.getChildren(); assert.isArray(children); - // In test environment, may return empty or loading based on timing + + // Verify loading item is present + assert.isAtLeast(children.length, 1, 'Should return at least loading item'); + assert.strictEqual(children[0].type, DeepnoteTreeItemType.Loading); + assert.strictEqual(children[0].contextValue, 'loading'); if (newProvider && typeof newProvider.dispose === 'function') { newProvider.dispose(); } });Or rename to match actual behavior:
- test('should return loading item on first call with correct properties', async () => { + test('should not throw on first getChildren call', async () => {
📜 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.unit.test.ts(2 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/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
**/*.{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/deepnoteTreeDataProvider.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/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
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
DeepnoteTreeDataProvider(22-236)src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)
There was a problem hiding this comment.
Actionable comments posted: 4
📜 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.unit.test.ts(2 hunks)src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts(1 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/deepnoteTreeItem.unit.test.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
**/*.{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/deepnoteTreeItem.unit.test.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
**/*.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/deepnoteTreeItem.unit.test.tssrc/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/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts (1)
src/notebooks/deepnote/deepnoteTreeItem.ts (2)
DeepnoteTreeItemContext(16-20)DeepnoteTreeItem(25-107)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
DeepnoteTreeDataProvider(22-236)src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)
🔇 Additional comments (3)
src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts (2)
583-600: LGTM!Test correctly validates loading item construction with null data and appropriate properties.
602-626: Verification complete.Test now properly asserts that initialization was skipped—tooltip, iconPath, and description remain undefined, label defaults to empty string. Addresses the previous review comment.
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (1)
220-245: Well-verified.Test properly asserts absence of loading items by checking both type (line 237) and label (lines 241-243). Addresses previous review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 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)
package.json(1 hunks)src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts(2 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/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
**/*.{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/deepnoteTreeDataProvider.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/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
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
DeepnoteTreeDataProvider(22-236)src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)
⏰ 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
🔇 Additional comments (3)
package.json (1)
1442-1442: LGTM!Guards reveal command from loading placeholders. Prevents user confusion.
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (2)
163-206: Environment-dependent assertions acceptable here.Test properly verifies refresh resets to initial state by comparing lengths and conditionally checking loading item reappears. Conditional logic is reasonable given test environment constraints.
242-267: Solid test with proper assertions.Correctly verifies loading items don't appear when querying project children. Clear setup, targeted assertions for both type and label.
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 (1)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
57-70: Keep the welcome view visible with empty workspaces.When no workspace folders are open,
getChildrenreturns early and the new initial-scan context never flips totrue, so the welcome view (now gated bydeepnote.explorerInitialScanComplete) stays hidden. Users land on a blank explorer in that scenario. Mark the scan complete before returning so the welcome view still renders.if (!workspace.workspaceFolders || workspace.workspaceFolders.length === 0) { - return []; + if (!this.isInitialScanComplete) { + this.isInitialScanComplete = true; + this.updateContextKey(); + } + + return []; }
📜 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)
package.json(2 hunks)src/notebooks/deepnote/deepnoteTreeDataProvider.ts(6 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/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
**/*.{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/deepnoteTreeDataProvider.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteTreeDataProvider.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (2)
src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)src/test/mocks/vsc/extHostedTypes.ts (1)
ThemeIcon(2094-2104)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (2)
220-237: Weak assertions for multiple-call scenario.Test name implies verification of how multiple calls are handled but only checks arrays are returned. Doesn't verify state transitions or idempotency.
Add assertions about loading state behavior:
// Second call const secondResult = await newProvider.getChildren(undefined); assert.isArray(secondResult); + + // Verify behavior: either both show loading (if scan incomplete) + // or both show same results (if scan complete) + const firstIsLoading = firstResult.length > 0 && firstResult[0].contextValue === 'loading'; + const secondIsLoading = secondResult.length > 0 && secondResult[0].contextValue === 'loading'; + + if (firstIsLoading) { + // If first call returned loading, scan might still be running + // Second call should eventually show real results or still loading + assert.isTrue( + secondIsLoading || secondResult.length === 0, + 'Multiple calls should handle loading state consistently' + ); + }
207-218: Test doesn't verify claimed behavior.Test claims to "execute loading logic" but only asserts an array is returned. Doesn't verify loading logic ran or that loading state was entered.
Either rename to match actual behavior: "should not throw when calling getChildren at root level", or add assertions:
const children = await newProvider.getChildren(undefined); assert.isArray(children); - // In test environment may be empty or have loading item depending on timing + + // In test environment without workspace, returns empty array + // With workspace, should exercise loading code path + if (children.length > 0) { + assert.strictEqual(children[0].contextValue, 'loading', 'Should show loading item on first call'); + }
📜 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(6 hunks)src/notebooks/deepnote/deepnoteTreeDataProvider.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/deepnoteTreeDataProvider.unit.test.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
**/*.{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/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.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/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
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
DeepnoteTreeDataProvider(24-245)src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (2)
src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(25-107)src/test/mocks/vsc/extHostedTypes.ts (1)
ThemeIcon(2094-2104)
🔇 Additional comments (10)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (3)
2-2: LGTM!Import needed for loading label verification at line 261.
89-112: LGTM!Both tests serve as valid sanity checks with accurate names matching their assertions.
239-264: LGTM!Good test with meaningful assertions. Properly verifies that child lookups don't produce loading items by checking both type and label.
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (7)
10-13: LGTM!All imports needed for loading state implementation.
32-33: LGTM!Fields properly track initial scan lifecycle.
37-37: LGTM!Initializes context key state on construction.
47-49: LGTM!Refresh properly resets scan state for reload.
63-70: LGTM!Loading logic correctly:
- Caches scan promise to prevent duplicates
- Returns loading item during scan
- Relies on tree refresh after scan completes
82-103: LGTM!Both helpers correctly implement loading item creation and async scan lifecycle. Localization and error handling properly addressed.
242-244: LGTM!Context key update correctly integrated into provider lifecycle. Void operator appropriate for fire-and-forget command.
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
Fixes #5031
Screen.Recording.2025-10-31.at.11.21.18.mov
Summary by CodeRabbit
New Features
Improvements
Tests