feat(telemetry): add PostHog analytics for notebook usage events#375
Draft
feat(telemetry): add PostHog analytics for notebook usage events#375
Conversation
…ents Introduces an opt-out PostHog telemetry service that tracks user interactions with Deepnote notebooks, including block additions, cell executions, environment operations, integration management, and project/notebook lifecycle events. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds a telemetry subsystem and instruments multiple Deepnote features. Introduces ITelemetryService plus implementations (TelemetryService using posthog-node, TelemetryWebService no-op for web, NoOpTelemetryService), a new VS Code config Sequence Diagram(s)sequenceDiagram
participant User
participant Component as "Deepnote Component\n(Explorer/Notebook/Env/Integration)"
participant Telemetry as "TelemetryService\n(ITelemetryService)"
participant Persist as "PersistentState"
participant PostHog as "posthog-node"
User->>Component: perform action (open/create/execute/delete/insert/etc.)
Component->>Telemetry: trackEvent({ eventName, properties })
alt Telemetry disabled via settings
Telemetry-->>Component: no-op
else Telemetry enabled
Telemetry->>Persist: getValue("deepnote:anonymousId")
alt no id stored
Persist-->>Telemetry: ""
Telemetry->>Persist: updateValue(generated-uuid)
Persist-->>Telemetry: saved
else id exists
Persist-->>Telemetry: anonymous-id
end
Telemetry->>PostHog: initialize client if needed (host, apiKey)
Telemetry->>PostHog: capture(eventName, properties, distinctId)
PostHog-->>Telemetry: ack
Telemetry-->>Component: returned
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #375 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
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/deepnoteExplorerView.ts (1)
337-430:⚠️ Potential issue | 🟠 MajorEmit completion events after successful actions, not before command execution.
Current placement records
delete_*,export_notebook, etc. even when user cancels or operation fails, skewing analytics.As per coding guidelines, "Extract duplicate logic into helper methods to prevent drift following DRY principle".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 337 - 430, The analytics.trackEvent calls are being fired before command handlers run (e.g., in the commands.registerCommand calls that reference this.analytics?.trackEvent for events like 'delete_notebook', 'export_notebook', 'duplicate_notebook', 'create_project', etc.), which records events even if the user cancels or the operation fails; change the pattern so events are emitted only after the command promise resolves successfully by moving the this.analytics?.trackEvent invocation into the success path of the handler (e.g., after awaiting deleteNotebook(treeItem), exportProject(treeItem), exportNotebook(treeItem), duplicateNotebook(treeItem), newProject(), newNotebook(), importNotebook(), importJupyterNotebook(), etc.), and to follow DRY extract a helper like registerTrackedCommand(commandName, eventName, handler) or a small wrapper that calls the handler, awaits its result, and then calls this.analytics?.trackEvent on success so all command registrations (the uses of commands.registerCommand) can use that helper consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 1653-1658: Replace the hard-coded description string for the
extension setting "deepnote.telemetry.enabled" with a localization token (e.g.
"%deepnote.configuration.deepnote.telemetry.enabled.description%") in the
manifest and add the matching entry
"deepnote.configuration.deepnote.telemetry.enabled.description" to
package.nls.json with the original English text so the setting description is
localized.
In `@src/extension.node.ts`:
- Around line 137-141: The telemetry shutdown is currently invoked without
awaiting and may be lost; update the extension deactivation flow so that when
activatedServiceContainer.tryGet<IPostHogAnalyticsService>(IPostHogAnalyticsService)
returns an analytics instance you await analytics.shutdown() (i.e., call await
analytics.shutdown()) and perform that awaited shutdown before you dispose the
async registry/dispose logic so shutdown completes prior to process teardown;
ensure the deactivation function is marked async if needed and that
analytics.shutdown() is awaited in the deactivation sequence.
In `@src/notebooks/deepnote/integrations/integrationWebview.ts`:
- Line 439: The analytics events for 'configure_integration',
'reset_integration', and 'delete_integration' are missing the integrationType
property that 'save_integration' includes; update each analytics?.trackEvent
call (for the events named 'configure_integration', 'reset_integration', and
'delete_integration') to pass the same integrationType payload used by
'save_integration' (e.g., integrationType: this.integration?.type or the same
integration variable/property used in save_integration) so all integration
actions send consistent event properties.
In `@src/notebooks/deepnote/openInDeepnoteHandler.node.ts`:
- Around line 24-27: The current command handler calls
this.analytics?.trackEvent('open_in_deepnote') before the operation completes,
so move the tracking into handleOpenInDeepnote and only call it after
env.openExternal succeeds; remove the pre-call from the Commands.OpenInDeepnote
registration and update handleOpenInDeepnote to invoke
this.analytics?.trackEvent('open_in_deepnote') immediately after the successful
await env.openExternal(...) (and do not call it in the error/throw path).
In `@src/platform/analytics/constants.ts`:
- Around line 1-2: Add a guard that prevents PostHog from initializing when the
API key is the placeholder; define a boolean like IS_POSTHOG_CONFIGURED
(computed from POSTHOG_API_KEY !== '__POSTHOG_API_KEY__' and non-empty) and
update posthogAnalyticsService.initialize() to early-return if
!IS_POSTHOG_CONFIGURED so PostHog is not initialized with the placeholder key
(reference POSTHOG_API_KEY, POSTHOG_HOST, and the initialize() method in
posthogAnalyticsService.ts).
In `@src/platform/analytics/posthogAnalyticsService.ts`:
- Around line 23-53: The two public methods in PostHogAnalyticsService are out
of alphabetical order; move the public async shutdown() method so it appears
before the public trackEvent(...) method while keeping both method bodies,
try/catch blocks and behavior (references to this.client, this.userIdState,
this.initialized, etc.) unchanged; ensure method accessibility remains public
and no other methods' order is altered.
- Around line 55-58: The initialize() method sets this.initialized = true before
performing setup (e.g., this.userIdState =
this.stateFactory.createGlobalPersistentState(...)), so if state creation throws
later the service will remain marked initialized and drop events; move the
this.initialized = true assignment to after all setup steps succeed and wrap the
setup (stateFactory.createGlobalPersistentState and any client creation) in a
try/catch so that on error you do not set initialized (and optionally log or
rethrow) — update initialize(), userIdState assignment, and any client creation
code paths to ensure initialized is only true after successful completion.
- Around line 71-79: The isTelemetryEnabled function currently permits telemetry
for telemetryLevel values other than 'off'; update isTelemetryEnabled to only
allow telemetry when
workspace.getConfiguration('telemetry').get<string>('telemetryLevel') === 'all'
and the deepnote setting
workspace.getConfiguration('deepnote').get<boolean>('telemetry.enabled', true)
is true; modify the logic in isTelemetryEnabled to return false unless
telemetryLevel === 'all', then read and return the deepnote.telemetry.enabled
flag (use the existing function name isTelemetryEnabled and the same
configuration keys).
- Around line 64-68: POSTHOG_API_KEY is left as the placeholder
'__POSTHOG_API_KEY__' and is passed into the PostHog constructor (this.client =
new PostHog(...)), so telemetry silently fails; fix by either configuring your
build (esbuild define) to replace POSTHOG_API_KEY at build-time or add a runtime
guard in the PostHog initialization path: check POSTHOG_API_KEY (e.g. equals
'__POSTHOG_API_KEY__' or startsWith('__')) before calling new PostHog, and if
invalid skip creating this.client and emit a clear warning/error via your logger
instead.
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 337-430: The analytics.trackEvent calls are being fired before
command handlers run (e.g., in the commands.registerCommand calls that reference
this.analytics?.trackEvent for events like 'delete_notebook', 'export_notebook',
'duplicate_notebook', 'create_project', etc.), which records events even if the
user cancels or the operation fails; change the pattern so events are emitted
only after the command promise resolves successfully by moving the
this.analytics?.trackEvent invocation into the success path of the handler
(e.g., after awaiting deleteNotebook(treeItem), exportProject(treeItem),
exportNotebook(treeItem), duplicateNotebook(treeItem), newProject(),
newNotebook(), importNotebook(), importJupyterNotebook(), etc.), and to follow
DRY extract a helper like registerTrackedCommand(commandName, eventName,
handler) or a small wrapper that calls the handler, awaits its result, and then
calls this.analytics?.trackEvent on success so all command registrations (the
uses of commands.registerCommand) can use that helper consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b27f28f-a694-4b50-86ed-512bcc176642
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
package.jsonsrc/extension.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteCellExecutionAnalytics.tssrc/notebooks/deepnote/deepnoteExplorerView.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.tssrc/notebooks/notebookCommandListener.tssrc/notebooks/serviceRegistry.node.tssrc/platform/analytics/constants.tssrc/platform/analytics/posthogAnalyticsService.tssrc/platform/analytics/posthogAnalyticsService.unit.test.tssrc/platform/analytics/types.tssrc/platform/serviceRegistry.node.ts
Comment on lines
+1
to
+2
| export const POSTHOG_API_KEY = '__POSTHOG_API_KEY__'; | ||
| export const POSTHOG_HOST = 'https://us.i.posthog.com'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Placeholder occurrences:"
rg -n "__POSTHOG_API_KEY__" src build package.json
echo
echo "2) Build-time replacement/define hooks for PostHog key:"
rg -n "POSTHOG_API_KEY|__POSTHOG_API_KEY__|define|replace" build package.json
echo
echo "3) Runtime initialization sites:"
rg -n "new PostHog\\(|POSTHOG_API_KEY|initializePostHog|initialize\\(" src/platform/analyticsRepository: deepnote/vscode-deepnote
Length of output: 5327
🏁 Script executed:
cat -n build/esbuild/build.ts | head -280 | tail -80Repository: deepnote/vscode-deepnote
Length of output: 4141
🏁 Script executed:
rg -A 5 "define\s*=" build/esbuild/build.ts | head -50Repository: deepnote/vscode-deepnote
Length of output: 217
🏁 Script executed:
grep -r "POSTHOG\|posthog\|telemetry.*key\|analytics.*key" build/ package.json --include="*.ts" --include="*.js" --include="*.json" 2>/dev/null | grep -v node_modules | head -20Repository: deepnote/vscode-deepnote
Length of output: 115
🏁 Script executed:
cat -n src/platform/analytics/posthogAnalyticsService.ts | head -80Repository: deepnote/vscode-deepnote
Length of output: 3054
🏁 Script executed:
grep -r "process.env\|POSTHOG" src/platform/analytics/ -A 2 -B 2Repository: deepnote/vscode-deepnote
Length of output: 1275
🏁 Script executed:
rg -r "POSTHOG_API_KEY|__POSTHOG_API_KEY__" . --type json --type js --type ts -g "!node_modules" -g "!.git" | grep -v "src/platform/analytics"Repository: deepnote/vscode-deepnote
Length of output: 50445
🏁 Script executed:
grep -r "process.env" build/esbuild/ --include="*.ts" --include="*.js" | grep -i "api\|key\|secret\|token" | head -10Repository: deepnote/vscode-deepnote
Length of output: 50
Add guard to prevent PostHog initialization with placeholder API key.
The placeholder __POSTHOG_API_KEY__ has no build-time replacement and ships unchanged. PostHog initializes with an invalid key, silently dropping telemetry. Add a configuration flag to skip initialization when the key is unset:
Suggested fix
export const POSTHOG_API_KEY = '__POSTHOG_API_KEY__';
export const POSTHOG_HOST = 'https://us.i.posthog.com';
+export const IS_POSTHOG_CONFIGURED = POSTHOG_API_KEY !== '__POSTHOG_API_KEY__';// In initialize() method of posthogAnalyticsService.ts
private initialize(): void {
if (!IS_POSTHOG_CONFIGURED) {
return;
}
// ... rest of initialization
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const POSTHOG_API_KEY = '__POSTHOG_API_KEY__'; | |
| export const POSTHOG_HOST = 'https://us.i.posthog.com'; | |
| export const POSTHOG_API_KEY = '__POSTHOG_API_KEY__'; | |
| export const POSTHOG_HOST = 'https://us.i.posthog.com'; | |
| export const IS_POSTHOG_CONFIGURED = POSTHOG_API_KEY !== '__POSTHOG_API_KEY__'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/analytics/constants.ts` around lines 1 - 2, Add a guard that
prevents PostHog from initializing when the API key is the placeholder; define a
boolean like IS_POSTHOG_CONFIGURED (computed from POSTHOG_API_KEY !==
'__POSTHOG_API_KEY__' and non-empty) and update
posthogAnalyticsService.initialize() to early-return if !IS_POSTHOG_CONFIGURED
so PostHog is not initialized with the placeholder key (reference
POSTHOG_API_KEY, POSTHOG_HOST, and the initialize() method in
posthogAnalyticsService.ts).
Comment on lines
+55
to
+58
| private initialize(): void { | ||
| this.initialized = true; | ||
|
|
||
| this.userIdState = this.stateFactory.createGlobalPersistentState<string>(USER_ID_STORAGE_KEY, ''); |
Contributor
There was a problem hiding this comment.
Do not mark initialized before setup succeeds.
If persistent state/client creation throws once, initialized stays true and all later events are dropped permanently.
Suggested fix
private initialize(): void {
- this.initialized = true;
-
- this.userIdState = this.stateFactory.createGlobalPersistentState<string>(USER_ID_STORAGE_KEY, '');
+ const userIdState = this.stateFactory.createGlobalPersistentState<string>(USER_ID_STORAGE_KEY, '');
- if (!this.userIdState.value) {
- void this.userIdState.updateValue(generateUuid());
+ if (!userIdState.value) {
+ void userIdState.updateValue(generateUuid());
}
- this.client = new PostHog(POSTHOG_API_KEY, {
+ const client = new PostHog(POSTHOG_API_KEY, {
flushAt: 20,
flushInterval: 30000,
host: POSTHOG_HOST
});
+
+ this.userIdState = userIdState;
+ this.client = client;
+ this.initialized = true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/analytics/posthogAnalyticsService.ts` around lines 55 - 58, The
initialize() method sets this.initialized = true before performing setup (e.g.,
this.userIdState = this.stateFactory.createGlobalPersistentState(...)), so if
state creation throws later the service will remain marked initialized and drop
events; move the this.initialized = true assignment to after all setup steps
succeed and wrap the setup (stateFactory.createGlobalPersistentState and any
client creation) in a try/catch so that on error you do not set initialized (and
optionally log or rethrow) — update initialize(), userIdState assignment, and
any client creation code paths to ensure initialized is only true after
successful completion.
Comment on lines
+64
to
+68
| this.client = new PostHog(POSTHOG_API_KEY, { | ||
| flushAt: 20, | ||
| flushInterval: 30000, | ||
| host: POSTHOG_HOST | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate PostHog API key placeholder usage =="
rg -n -C3 '__POSTHOG_API_KEY__|POSTHOG_API_KEY' --type ts --type js --type json
echo "== Look for build-time define/replace configuration =="
rg -n -C3 'DefinePlugin|define:|replace\\(|esbuild|webpack|rollup|vite|__POSTHOG_API_KEY__' --type ts --type js --type jsonRepository: deepnote/vscode-deepnote
Length of output: 1825
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check for build configuration files =="
find . -maxdepth 2 -type f \( -name "webpack*.js" -o -name "vite*.config.*" -o -name "esbuild*.js" -o -name "rollup*.js" -o -name "tsconfig*.json" \) | head -20
echo "== Search for DefinePlugin or define configuration (simpler pattern) =="
rg -n 'DefinePlugin|define' --type ts --type js | head -30
echo "== Check package.json for build scripts =="
rg -n 'scripts|build' --type json | grep -E 'package.json|build' | head -20
echo "== Verify if API key has validation before PostHog initialization =="
rg -n -B5 -A5 'new PostHog' --type ts
echo "== Check for environment variable usage =="
rg -n 'process.env|process\.env\.POSTHOG|import\.meta' --type ts --type js | grep -i posthog | head -20Repository: deepnote/vscode-deepnote
Length of output: 6390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Read esbuild build configuration =="
cat build/esbuild/build.ts | head -100
echo "== Search for any POSTHOG or API key replacement in build files =="
rg -n 'POSTHOG|apiKey|api.key|secret' build/ --type ts --type js
echo "== Full PostHog service initialization context =="
cat -n src/platform/analytics/posthogAnalyticsService.ts | head -80
echo "== Check if there's error handling for invalid API keys =="
rg -n -A10 -B2 'new PostHog' src/platform/analytics/posthogAnalyticsService.tsRepository: deepnote/vscode-deepnote
Length of output: 3863
🏁 Script executed:
#!/bin/bash
set -euo pipefmt
echo "== Search for 'define' in esbuild build config =="
grep -n "define" build/esbuild/build.ts | head -20
echo "== Check for environment variables or process.env in build config =="
grep -n "process.env\|process\.env" build/esbuild/build.ts | head -20
echo "== Search for POSTHOG in entire codebase =="
rg -n 'POSTHOG' --type ts --type js
echo "== Check what happens when PostHog client is created with invalid key =="
cat -n src/platform/analytics/posthogAnalyticsService.ts | sed -n '50,75p'
echo "== Check PostHog documentation or package.json version =="
grep -n "posthog" package.jsonRepository: deepnote/vscode-deepnote
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for 'define' in esbuild build config =="
grep -n "define" build/esbuild/build.ts | head -20
echo "== Check for environment variables or process.env in build config =="
grep -n "process.env" build/esbuild/build.ts | head -20
echo "== Search for POSTHOG in entire codebase =="
rg -n 'POSTHOG' --type ts --type js
echo "== Check what happens when PostHog client is created with invalid key =="
cat -n src/platform/analytics/posthogAnalyticsService.ts | sed -n '50,75p'
echo "== Check PostHog package version =="
grep -n "posthog" package.jsonRepository: deepnote/vscode-deepnote
Length of output: 2310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check full PostHog service to see if API key is validated =="
cat -n src/platform/analytics/posthogAnalyticsService.ts
echo "== Check how PostHog service is used - maybe validated at usage time =="
rg -n 'PostHogAnalyticsService\|isTelemetryEnabled' src/ --type ts | head -30
echo "== Verify if __POSTHOG_API_KEY__ placeholder is mentioned anywhere else =="
rg -n '__POSTHOG_API_KEY__' --type ts --type js --type jsonRepository: deepnote/vscode-deepnote
Length of output: 3190
Add build-time replacement for PostHog API key placeholder or validate at runtime.
The POSTHOG_API_KEY constant is hardcoded as '__POSTHOG_API_KEY__' with no build-time replacement configured in esbuild. The PostHog client initializes with this invalid placeholder (line 64), causing telemetry to silently fail without error reporting.
Either replace the placeholder during build using esbuild's define option, or validate the API key at runtime and skip initialization with a logged warning when it remains a placeholder.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/analytics/posthogAnalyticsService.ts` around lines 64 - 68,
POSTHOG_API_KEY is left as the placeholder '__POSTHOG_API_KEY__' and is passed
into the PostHog constructor (this.client = new PostHog(...)), so telemetry
silently fails; fix by either configuring your build (esbuild define) to replace
POSTHOG_API_KEY at build-time or add a runtime guard in the PostHog
initialization path: check POSTHOG_API_KEY (e.g. equals '__POSTHOG_API_KEY__' or
startsWith('__')) before calling new PostHog, and if invalid skip creating
this.client and emit a clear warning/error via your logger instead.
Comment on lines
+71
to
+79
| private isTelemetryEnabled(): boolean { | ||
| const telemetryLevel = workspace.getConfiguration('telemetry').get<string>('telemetryLevel', 'all'); | ||
|
|
||
| if (telemetryLevel === 'off') { | ||
| return false; | ||
| } | ||
|
|
||
| return workspace.getConfiguration('deepnote').get<boolean>('telemetry.enabled', true); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In VS Code, what event categories are allowed for telemetry.telemetryLevel values "all", "error", "crash", and "off"? Are product-usage analytics allowed when the level is not "all"?
💡 Result:
The telemetry.telemetryLevel setting in VS Code controls three categories of telemetry events: Crash Reports (diagnostic info on crashes), Error Telemetry (info on non-crashing errors), and Usage Data (info on feature usage and performance, which includes product-usage analytics). - "all": All categories (Crash Reports, Error Telemetry, Usage Data). - "error": Crash Reports and Error Telemetry (no Usage Data). - "crash": Crash Reports only (no Error Telemetry or Usage Data). - "off": None. Product-usage analytics fall under Usage Data, so they are not allowed when the level is "error", "crash", or "off" — only when set to "all".
Citations:
- 1: https://code.visualstudio.com/docs/configure/telemetry
- 2: https://code.visualstudio.com/docs/enterprise/telemetry
- 3: https://github.com/Microsoft/vscode/blob/main/src/vs/platform/telemetry/common/telemetryService.ts
- 4: https://github.com/microsoft/vscode/blob/be75065e817ebd7b6250a100cf5af78bb931265b/src/vs/platform/telemetry/common/telemetryUtils.ts
Change telemetry gate to telemetryLevel === 'all'.
Current code allows usage events at error and crash levels, violating VS Code's telemetry policy (Usage Data only permitted when telemetryLevel is all).
Fix
private isTelemetryEnabled(): boolean {
const telemetryLevel = workspace.getConfiguration('telemetry').get<string>('telemetryLevel', 'all');
- if (telemetryLevel === 'off') {
+ if (telemetryLevel !== 'all') {
return false;
}
return workspace.getConfiguration('deepnote').get<boolean>('telemetry.enabled', true);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/analytics/posthogAnalyticsService.ts` around lines 71 - 79, The
isTelemetryEnabled function currently permits telemetry for telemetryLevel
values other than 'off'; update isTelemetryEnabled to only allow telemetry when
workspace.getConfiguration('telemetry').get<string>('telemetryLevel') === 'all'
and the deepnote setting
workspace.getConfiguration('deepnote').get<boolean>('telemetry.enabled', true)
is true; modify the logic in isTelemetryEnabled to return false unless
telemetryLevel === 'all', then read and return the deepnote.telemetry.enabled
flag (use the existing function name isTelemetryEnabled and the same
configuration keys).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Removes PostHog-specific naming from the interface and implementation to keep the telemetry abstraction provider-agnostic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
src/extension.node.ts (1)
132-140:⚠️ Potential issue | 🟠 MajorAwait telemetry shutdown before disposing registry.
This remains unresolved:
deactivate()disposes immediately and can still drop final telemetry. At minimum, ensure shutdown is awaited beforeregistry.dispose()(same concern as earlier review around this block). See Line 132 onward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/extension.node.ts` around lines 132 - 140, deactivate() currently sets Exiting.isExiting and immediately returns registry.dispose(), which can abort final telemetry; modify deactivate to first await the telemetry shutdown routine (call the telemetry shutdown/flush method used in your codebase) before calling registry.dispose(), ensuring you still handle the case where activatedServiceContainer or registry is missing; specifically update the deactivate function (symbols: deactivate, Exiting.isExiting, activatedServiceContainer, IAsyncDisposableRegistry, registry.dispose) to await telemetry shutdown/flush and then await/return registry.dispose().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooks/deepnote/deepnoteCellExecutionAnalytics.ts`:
- Around line 15-19: Constructor parameter order violates alphabetical coding
guidelines; reorder the constructor parameters so they appear as analytics,
disposables, notebookManager (keeping their decorators and types:
`@inject`(ITelemetryService) `@optional`() analytics: ITelemetryService | undefined,
`@inject`(IDisposableRegistry) disposables: Disposable[],
`@inject`(IDeepnoteNotebookManager) notebookManager: IDeepnoteNotebookManager) to
match required alphabetical ordering for the constructor signature and the
corresponding private readonly fields.
- Around line 10-11: Update the top-file JSDoc that currently references PostHog
to a provider-agnostic description mentioning TelemetryService; locate the
comment in src/notebooks/deepnote/deepnoteCellExecutionAnalytics.ts (the header
above functions like any exported tracker or init function in this module) and
replace "Tracks cell execution events for PostHog analytics" with a neutral line
such as "Tracks cell execution events for telemetry via TelemetryService" so it
aligns with the renamed types and doesn't reference PostHog specifically.
- Line 59: Remove the redundant optional chaining on analytics: replace the call
using this.analytics?.trackEvent('execute_cell', properties) with a direct
this.analytics.trackEvent('execute_cell', properties) because analytics is
already guaranteed non-null by the earlier early-return in the
DeepnoteCellExecutionAnalytics flow; update the invocation in the same spot to
remove the question mark.
In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 387-390: The telemetry is fired unconditionally after awaiting the
operations even when the user cancels; change the operations deleteProject,
deleteNotebook, and duplicateNotebook to return a boolean (true if the action
completed, false if cancelled) and then in each command handler (e.g. the
Commands.DeleteProject/Commands.DeleteNotebook/Commands.DuplicateNotebook
handlers that call
this.deleteProject/this.deleteNotebook/this.duplicateNotebook) only call
this.analytics?.trackEvent(...) when the returned boolean is true; update the
handlers to await the method, check the result, and skip trackEvent on false so
telemetry only fires on completed actions.
- Around line 420-423: The telemetry is being sent even when users cancel export
flows because exportProject/exportNotebook return early on cancellations; update
the call sites (the Commands.ExportProject and Commands.ExportNotebook handlers)
to only call this.analytics?.trackEvent after confirming the export actually
completed (e.g., have exportProject/exportNotebook return a success boolean or
resolved value indicating completion and check it), or alternatively move the
analytics tracking into exportProject/exportNotebook to execute only on
successful completion; ensure the early returns for format/folder/overwrite
cancellation do not trigger trackEvent.
- Around line 352-355: Telemetry is fired unconditionally after awaiting the
command handlers (e.g., the registerCommand callbacks that call newProject(),
importProject(), openFromDeepnote(), openFromFile()), which causes events to be
tracked even when the operation early-returns (no workspace or user cancelled).
Fix by making each handler only emit analytics?.trackEvent when the operation
actually succeeded: either (A) have the underlying methods
(newProject/importProject/openFromDeepnote/openFromFile) return a boolean/result
indicating success and conditionally call analytics?.trackEvent based on that
result, or (B) move the tracking into those methods at the point they complete
successfully; update the registerCommand callbacks to check the returned success
value (or rely on the internal tracking) before calling analytics?.trackEvent.
- Around line 29-30: Reorder the constructor parameters so that analytics
(ITelemetryService) appears before logger (ILogger) to match the alphabetical
ordering rule: change the parameter list in the DeepnoteExplorerView constructor
so "analytics?: ITelemetryService" comes before "@inject(ILogger) logger:
ILogger"; optionally add a matching `@inject` decorator for analytics if you want
consistent injection style, but the primary fix is to swap the parameter order
to keep accessibility/alphabetical ordering for properties.
In `@src/platform/analytics/telemetryService.ts`:
- Around line 13-26: Reorder the private fields inside the TelemetryService
class so they follow the accessibility-then-alphabetical guideline: place
private client, then private initialized, then private userIdState (i.e., ensure
the fields named client, initialized, and userIdState in class TelemetryService
are declared in that alphabetical order under the private accessibility level).
In `@src/platform/analytics/telemetryService.unit.test.ts`:
- Around line 104-127: The test currently checks state creation and user ID
persistence but doesn't assert the PostHog client actually recorded the event;
update the test for TelemetryService.trackEvent to also assert that the mocked
PostHog client's capture method was called (e.g., that mockPosthogClient.capture
was calledOnce) and that its first call includes the event name 'test_event' and
the distinct_id equals the generatedId from mockUserIdState.updateValue; if the
PostHog client isn't already mocked in this test, stub or inject a
mockPosthogClient with a Sinon stub for capture before calling
analyticsService.trackEvent so you can assert on its calls.
- Around line 148-152: The test uses assert.isFulfilled() but chai-as-promised
isn't registered; import and register the plugin in the test suite setup so
promise assertions work: add an import for chai-as-promised and call
use(chaiAsPromised) in the suite setup (same pattern used in other tests) so
assert.isFulfilled(analyticsService.dispose()) succeeds; update
telemetryService.unit.test.ts where TelemetryService and assert.isFulfilled are
used.
In `@src/platform/serviceRegistry.node.ts`:
- Around line 9-10: The two imports ITelemetryService and TelemetryService are
placed separately; move the lines "import { ITelemetryService } from
'./analytics/types';" and "import { TelemetryService } from
'./analytics/telemetryService';" so they are grouped with the other relative
'./' imports (i.e., alongside the imports that include registerTypes) to keep
import ordering consistent; ensure you preserve existing import semantics and
update ordering only.
---
Duplicate comments:
In `@src/extension.node.ts`:
- Around line 132-140: deactivate() currently sets Exiting.isExiting and
immediately returns registry.dispose(), which can abort final telemetry; modify
deactivate to first await the telemetry shutdown routine (call the telemetry
shutdown/flush method used in your codebase) before calling registry.dispose(),
ensuring you still handle the case where activatedServiceContainer or registry
is missing; specifically update the deactivate function (symbols: deactivate,
Exiting.isExiting, activatedServiceContainer, IAsyncDisposableRegistry,
registry.dispose) to await telemetry shutdown/flush and then await/return
registry.dispose().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15c38932-a92a-42ce-a197-15e34e626d84
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
src/extension.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.node.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteCellExecutionAnalytics.tssrc/notebooks/deepnote/deepnoteExplorerView.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.tssrc/notebooks/notebookCommandListener.tssrc/platform/analytics/telemetryService.tssrc/platform/analytics/telemetryService.unit.test.tssrc/platform/analytics/types.tssrc/platform/serviceRegistry.node.ts
Comment on lines
+104
to
+127
| test('should generate user ID on first trackEvent when telemetry enabled', () => { | ||
| const vscode = require('vscode'); | ||
|
|
||
| sandbox.stub(vscode.workspace, 'getConfiguration').callsFake(() => ({ | ||
| get: (_key: string, defaultValue: unknown) => defaultValue | ||
| })); | ||
|
|
||
| analyticsService = new TelemetryService(mockStateFactory, mockAsyncDisposableRegistry); | ||
| analyticsService.trackEvent('test_event'); | ||
|
|
||
| assert.isTrue( | ||
| (mockStateFactory.createGlobalPersistentState as sinon.SinonStub).calledOnce, | ||
| 'Should create persistent state' | ||
| ); | ||
| assert.isTrue( | ||
| (mockUserIdState.updateValue as sinon.SinonStub).calledOnce, | ||
| 'Should generate and persist user ID' | ||
| ); | ||
|
|
||
| const generatedId = (mockUserIdState.updateValue as sinon.SinonStub).firstCall.args[0]; | ||
|
|
||
| assert.isString(generatedId); | ||
| assert.isNotEmpty(generatedId, 'Generated user ID should not be empty'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider asserting PostHog client capture was called.
Test verifies state initialization but not that the event was actually sent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/analytics/telemetryService.unit.test.ts` around lines 104 - 127,
The test currently checks state creation and user ID persistence but doesn't
assert the PostHog client actually recorded the event; update the test for
TelemetryService.trackEvent to also assert that the mocked PostHog client's
capture method was called (e.g., that mockPosthogClient.capture was calledOnce)
and that its first call includes the event name 'test_event' and the distinct_id
equals the generatedId from mockUserIdState.updateValue; if the PostHog client
isn't already mocked in this test, stub or inject a mockPosthogClient with a
Sinon stub for capture before calling analyticsService.trackEvent so you can
assert on its calls.
…vent
Replace untyped string event names with a TelemetryEventName literal union
type for static checking. Change trackEvent signature to accept a single
TelemetryEvent object ({ eventName, properties? }) instead of positional args.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (13)
src/platform/analytics/telemetryService.ts (3)
60-74:⚠️ Potential issue | 🟠 MajorInitialization is non-atomic and can lock telemetry into a broken state.
Line 61 sets
initializedbefore setup completes. If state/client creation fails once, future calls skip re-init. Also, first capture can use an emptydistinctIdbecauseupdateValue()is async and not awaited.Proposed fix
private initialize(): void { - this.initialized = true; - - this.userIdState = this.stateFactory.createGlobalPersistentState<string>(USER_ID_STORAGE_KEY, ''); - - if (!this.userIdState.value) { - void this.userIdState.updateValue(generateUuid()); - } - - this.client = new PostHog(POSTHOG_API_KEY, { + const userIdState = this.stateFactory.createGlobalPersistentState<string>(USER_ID_STORAGE_KEY, ''); + const distinctId = userIdState.value || generateUuid(); + if (!userIdState.value) { + void userIdState.updateValue(distinctId); + } + const client = new PostHog(POSTHOG_API_KEY, { flushAt: 20, flushInterval: 30000, host: POSTHOG_HOST }); + this.userIdState = userIdState; + this.client = client; + this.initialized = true; }Based on learnings: "Verify state after async setup operations - methods can return early without throwing, so check expected state was created".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/analytics/telemetryService.ts` around lines 60 - 74, The initialize() method marks this.initialized too early and fires async updateValue without awaiting, which can leave telemetry in a broken state; change initialize() so it performs stateFactory.createGlobalPersistentState and awaits userIdState.updateValue(generateUuid()) (or otherwise ensures the value is set) before constructing the PostHog client and only set this.initialized = true at the end; also wrap state/client creation in try/catch and ensure on any failure this.initialized remains false so future calls will retry, referencing initialize(), userIdState, stateFactory.createGlobalPersistentState, userIdState.updateValue(generateUuid()), and PostHog client construction.
76-84:⚠️ Potential issue | 🟠 MajorUsage telemetry should only run when
telemetryLevelisall.Line 79 only blocks
off, so usage events still send onerror/crash. For VS Code usage telemetry semantics, gate this totelemetryLevel === 'all'.In VS Code telemetry settings, are usage/feature analytics events allowed for telemetry.telemetryLevel values "error" or "crash", or only when set to "all"?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/analytics/telemetryService.ts` around lines 76 - 84, The isTelemetryEnabled() function currently allows telemetry for any telemetryLevel except 'off', but usage telemetry must only run when telemetryLevel === 'all'; change the gate so telemetryLevel is compared strictly to 'all' and only then return the deepnote.telemetry.enabled setting (i.e., replace the current if-check that returns false for 'off' with a check that returns false unless telemetryLevel === 'all'); update references inside isTelemetryEnabled, which reads telemetryLevel from workspace.getConfiguration('telemetry').get<string>('telemetryLevel', 'all') and the deepnote setting via workspace.getConfiguration('deepnote').get<boolean>('telemetry.enabled', true).
69-73:⚠️ Potential issue | 🟠 MajorGuard unresolved PostHog key before client creation.
If
POSTHOG_API_KEYis still a placeholder at runtime, initialization should short-circuit with a warning instead of creating a non-functional client.#!/bin/bash set -euo pipefail echo "== Placeholder usage ==" rg -n "__POSTHOG_API_KEY__|POSTHOG_API_KEY" echo "== Build-time replacement hooks (define/replace) ==" rg -n "define:|DefinePlugin|replace\\(|POSTHOG_API_KEY|__POSTHOG_API_KEY__" --type ts --type js --type json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/analytics/telemetryService.ts` around lines 69 - 73, Guard PostHog initialization by checking POSTHOG_API_KEY for unresolved placeholders before calling new PostHog: in the constructor or where this.client is set, detect empty or placeholder values like "__POSTHOG_API_KEY__" (or a literal "POSTHOG_API_KEY") and short-circuit by leaving this.client null/undefined and emitting a warning via the logger instead of constructing a non-functional client; keep the new PostHog(...) call only when the key is valid, and ensure other TelemetryService methods that use this.client handle the null/undefined client safely.src/notebooks/deepnote/openInDeepnoteHandler.node.ts (1)
24-27:⚠️ Potential issue | 🟠 Major
open_in_deepnoteis emitted even when the operation fails.
handleOpenInDeepnote()swallows errors, so Line 26 runs for both success and failure. Track only on confirmed success (e.g., afterenv.openExternal(...)) or have the handler return a success boolean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/openInDeepnoteHandler.node.ts` around lines 24 - 27, The analytics event is emitted regardless of failures because handleOpenInDeepnote() swallows errors; update the flow so tracking only happens on confirmed success: change handleOpenInDeepnote (or create a small wrapper) to return a boolean or throw on failure, and only call this.analytics?.trackEvent({ eventName: 'open_in_deepnote' }) after a successful env.openExternal(...) completes (or when the handler returns true). Reference Commands.OpenInDeepnote and the handleOpenInDeepnote function to locate the call site and ensure errors from env.openExternal are propagated or translated into a success flag before tracking.src/platform/analytics/telemetryService.unit.test.ts (1)
104-127: 🧹 Nitpick | 🔵 TrivialAdd an assertion that
capture()was called with the expected payload.This test validates ID persistence but not that an event was actually sent with the generated
distinctId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/analytics/telemetryService.unit.test.ts` around lines 104 - 127, Add an assertion that the analytics client's capture was invoked with the expected payload: after calling TelemetryService.trackEvent({ eventName: 'open_notebook' }) and extracting generatedId from mockUserIdState.updateValue, assert that the mocked capture method (e.g., analyticsClient.capture or mockAnalyticsClient.capture) was called once and that its first call args include an object with event equal to 'open_notebook' and distinctId equal to generatedId (or contains distinctId property matching generatedId); use the existing mocks/stubs for the analytics client used by TelemetryService to locate the capture call.src/notebooks/deepnote/integrations/integrationWebview.ts (1)
439-460: 🧹 Nitpick | 🔵 TrivialNormalize integration event payloads.
save_integrationsendsintegrationType, butconfigure_integration,reset_integration, anddelete_integrationdo not. Add the same property for consistent analytics dimensions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/integrations/integrationWebview.ts` around lines 439 - 460, Analytics events for configure_integration, reset_integration, and delete_integration lack the integrationType property, causing inconsistent dimensions; update the handlers that call this.showConfigurationForm, this.resetConfiguration, and the delete handler to include properties: { integrationType: <type> } similar to save_integration; obtain the type by resolving the integrationId (e.g., via an existing helper or registry like getIntegrationType/integrationRegistry.lookup by id) and fall back to 'unknown' if resolution fails, then pass that value in the this.analytics?.trackEvent call for configure_integration, reset_integration, and delete_integration.src/notebooks/deepnote/deepnoteExplorerView.ts (5)
420-423:⚠️ Potential issue | 🟡 MinorExport telemetry fires on format/folder/overwrite cancellation.
exportProjectandexportNotebookhave multiple early-return points (format selection, folder selection, overwrite confirmation). Tracking unconditional.Also applies to: 427-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 420 - 423, The telemetry is emitted unconditionally after calling exportProject/exportNotebook even when those methods return early (format/folder/overwrite cancel); change exportProject(treeItem) and exportNotebook(treeItem) to return a success boolean/enum (e.g. true on completed export, false on cancel/failure) or throw on fatal errors, then only call analytics?.trackEvent({ eventName: ..., properties: {...} }) when the call indicates success; update the Commands.ExportProject and Commands.ExportNotebook handlers to await the result and guard the analytics call based on that returned value so canceled flows do not fire telemetry.
337-340:⚠️ Potential issue | 🟡 MinorTelemetry fires regardless of outcome.
openNotebookcan fail (catch block) or early-return (missing notebookId). Event still emits. Consider tracking only on success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 337 - 340, Telemetry is being sent unconditionally after calling openNotebook; update the command handler and/or openNotebook to only emit analytics on success—either have openNotebook return a success boolean (or throw on failure) and in the commands.registerCommand callback await the result and call this.analytics.trackEvent({ eventName: 'open_notebook' }) only when success is true, or move the trackEvent call into the successful path inside openNotebook (e.g., after the notebookId check and after any try/catch success branch) so events are not fired on early-return or failure; reference the commands.registerCommand callback, openNotebook, and analytics.trackEvent to locate and implement the change.
387-390:⚠️ Potential issue | 🟡 MinorDelete/duplicate telemetry fires on user cancellation.
deleteProject,deleteNotebook,duplicateNotebookreturn early when user cancels confirmation. Tracking still emits.Also applies to: 400-403, 407-410
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 387 - 390, The telemetry is fired even when the user cancels because the command handlers call this.analytics?.trackEvent unconditionally after calling deleteProject/deleteNotebook/duplicateNotebook; update the handlers for Commands.DeleteProject, Commands.DeleteNotebook and Commands.DuplicateNotebook to only call this.analytics?.trackEvent when the operation actually completed (e.g., have deleteProject/deleteNotebook/duplicateNotebook return a boolean or a result object indicating success vs. cancellation and check that before tracking). If the methods don't currently return a success flag, change deleteProject/deleteNotebook/duplicateNotebook to return true on successful completion and false on user cancellation, and update the registerCommand callbacks to await the call and conditionally emit telemetry only when the returned value indicates the action was performed.
352-355:⚠️ Potential issue | 🟡 MinorTelemetry fires on user cancellation or early-return.
newProject,importNotebook,importJupyterNotebook,newNotebookall have early-return paths (no workspace, user cancelled input). Tracking happens unconditionally after await.Option: return boolean from operations
- private async newProject(): Promise<void> { + private async newProject(): Promise<boolean> { if (!workspace.workspaceFolders || workspace.workspaceFolders.length === 0) { // ... - return; + return false; } // ... on success ... + return true; }Then in handler:
commands.registerCommand(Commands.NewProject, async () => { - await this.newProject(); - this.analytics?.trackEvent({ eventName: 'create_project' }); + const created = await this.newProject(); + if (created) { + this.analytics?.trackEvent({ eventName: 'create_project' }); + } })Also applies to: 359-362, 366-369, 373-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 352 - 355, The telemetry is fired unconditionally after awaiting command handlers (e.g., Commands.NewProject, Commands.ImportNotebook, Commands.ImportJupyterNotebook, Commands.NewNotebook) even when the underlying methods (newProject, importNotebook, importJupyterNotebook, newNotebook) early-return or are cancelled; change those methods to return a boolean success flag (true on completed action, false on cancel/early-return) and update each command handler to only call this.analytics?.trackEvent(...) when the awaited call returns true, so telemetry is only recorded on actual successful operations.
29-30: 🧹 Nitpick | 🔵 TrivialConstructor parameter order:
analyticsshould precedeloggeralphabetically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 29 - 30, The constructor parameter order is out of alphabetical order; move the ITelemetryService parameter before ILogger and update the injection decorators accordingly: change the parameter list from "@inject(ILogger) logger: ILogger, private readonly analytics?: ITelemetryService" to "private readonly analytics?: ITelemetryService, `@inject`(ILogger) logger: ILogger" (or preferably annotate analytics with `@inject`(ITelemetryService) and then logger with `@inject`(ILogger)), ensuring the symbols analytics and logger and their types (ITelemetryService, ILogger) are updated in the DeepnoteExplorerView constructor signature so analytics appears before logger alphabetically.src/notebooks/deepnote/deepnoteCellExecutionAnalytics.ts (2)
59-59: 🧹 Nitpick | 🔵 TrivialOptional chaining redundant.
analyticsguaranteed non-null after early return at line 22-24.Minor simplification
- this.analytics?.trackEvent({ eventName: 'execute_cell', properties }); + this.analytics.trackEvent({ eventName: 'execute_cell', properties });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteCellExecutionAnalytics.ts` at line 59, The optional chaining on this.analytics in the call to trackEvent is unnecessary because analytics is guaranteed non-null after the early return; change the line that calls this.analytics?.trackEvent({ eventName: 'execute_cell', properties }) to use a direct call this.analytics.trackEvent({ eventName: 'execute_cell', properties }) so the call is simplified and consistent with the non-null guarantee.
15-19: 🧹 Nitpick | 🔵 TrivialConstructor parameter order: should be
analytics,disposables,notebookManager.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteCellExecutionAnalytics.ts` around lines 15 - 19, Constructor parameter order is wrong—reorder the injected constructor parameters to analytics, disposables, notebookManager. Modify the constructor signature so the parameters are declared as: `@inject`(ITelemetryService) `@optional`() private readonly analytics: ITelemetryService | undefined, `@inject`(IDisposableRegistry) private readonly disposables: Disposable[], `@inject`(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager; keep the existing decorators and private readonly declarations intact so DI tokens (ITelemetryService, IDisposableRegistry, IDeepnoteNotebookManager) still match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/analytics/telemetryService.ts`:
- Around line 28-58: Move the public async dispose(): Promise<void> method to
appear before the public trackEvent({ eventName, properties }: TelemetryEvent):
void method so public methods follow the repository rule (ordered by
accessibility then alphabetically); locate the dispose and trackEvent methods in
telemetryService.ts and reorder them accordingly while keeping their bodies
unchanged (ensure any references between them still compile).
In `@src/platform/analytics/telemetryService.unit.test.ts`:
- Around line 78-102: Add two unit tests mirroring the existing "trackEvent
should not throw when VSCode telemetry level is off" test but set the stubbed
configuration's telemetryLevel to 'error' and 'crash' respectively; for each
test, stub vscode.workspace.getConfiguration to return get(...) => 'error' (or
'crash') when section === 'telemetry' and _key === 'telemetryLevel', instantiate
TelemetryService (TelemetryService), call analyticsService.trackEvent({
eventName: 'open_notebook' }) inside assert.doesNotThrow, and assert that
mockStateFactory.createGlobalPersistentState was not called to verify telemetry
is gated for those levels.
---
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteCellExecutionAnalytics.ts`:
- Line 59: The optional chaining on this.analytics in the call to trackEvent is
unnecessary because analytics is guaranteed non-null after the early return;
change the line that calls this.analytics?.trackEvent({ eventName:
'execute_cell', properties }) to use a direct call this.analytics.trackEvent({
eventName: 'execute_cell', properties }) so the call is simplified and
consistent with the non-null guarantee.
- Around line 15-19: Constructor parameter order is wrong—reorder the injected
constructor parameters to analytics, disposables, notebookManager. Modify the
constructor signature so the parameters are declared as:
`@inject`(ITelemetryService) `@optional`() private readonly analytics:
ITelemetryService | undefined, `@inject`(IDisposableRegistry) private readonly
disposables: Disposable[], `@inject`(IDeepnoteNotebookManager) private readonly
notebookManager: IDeepnoteNotebookManager; keep the existing decorators and
private readonly declarations intact so DI tokens (ITelemetryService,
IDisposableRegistry, IDeepnoteNotebookManager) still match.
In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 420-423: The telemetry is emitted unconditionally after calling
exportProject/exportNotebook even when those methods return early
(format/folder/overwrite cancel); change exportProject(treeItem) and
exportNotebook(treeItem) to return a success boolean/enum (e.g. true on
completed export, false on cancel/failure) or throw on fatal errors, then only
call analytics?.trackEvent({ eventName: ..., properties: {...} }) when the call
indicates success; update the Commands.ExportProject and Commands.ExportNotebook
handlers to await the result and guard the analytics call based on that returned
value so canceled flows do not fire telemetry.
- Around line 337-340: Telemetry is being sent unconditionally after calling
openNotebook; update the command handler and/or openNotebook to only emit
analytics on success—either have openNotebook return a success boolean (or throw
on failure) and in the commands.registerCommand callback await the result and
call this.analytics.trackEvent({ eventName: 'open_notebook' }) only when success
is true, or move the trackEvent call into the successful path inside
openNotebook (e.g., after the notebookId check and after any try/catch success
branch) so events are not fired on early-return or failure; reference the
commands.registerCommand callback, openNotebook, and analytics.trackEvent to
locate and implement the change.
- Around line 387-390: The telemetry is fired even when the user cancels because
the command handlers call this.analytics?.trackEvent unconditionally after
calling deleteProject/deleteNotebook/duplicateNotebook; update the handlers for
Commands.DeleteProject, Commands.DeleteNotebook and Commands.DuplicateNotebook
to only call this.analytics?.trackEvent when the operation actually completed
(e.g., have deleteProject/deleteNotebook/duplicateNotebook return a boolean or a
result object indicating success vs. cancellation and check that before
tracking). If the methods don't currently return a success flag, change
deleteProject/deleteNotebook/duplicateNotebook to return true on successful
completion and false on user cancellation, and update the registerCommand
callbacks to await the call and conditionally emit telemetry only when the
returned value indicates the action was performed.
- Around line 352-355: The telemetry is fired unconditionally after awaiting
command handlers (e.g., Commands.NewProject, Commands.ImportNotebook,
Commands.ImportJupyterNotebook, Commands.NewNotebook) even when the underlying
methods (newProject, importNotebook, importJupyterNotebook, newNotebook)
early-return or are cancelled; change those methods to return a boolean success
flag (true on completed action, false on cancel/early-return) and update each
command handler to only call this.analytics?.trackEvent(...) when the awaited
call returns true, so telemetry is only recorded on actual successful
operations.
- Around line 29-30: The constructor parameter order is out of alphabetical
order; move the ITelemetryService parameter before ILogger and update the
injection decorators accordingly: change the parameter list from
"@inject(ILogger) logger: ILogger, private readonly analytics?:
ITelemetryService" to "private readonly analytics?: ITelemetryService,
`@inject`(ILogger) logger: ILogger" (or preferably annotate analytics with
`@inject`(ITelemetryService) and then logger with `@inject`(ILogger)), ensuring the
symbols analytics and logger and their types (ITelemetryService, ILogger) are
updated in the DeepnoteExplorerView constructor signature so analytics appears
before logger alphabetically.
In `@src/notebooks/deepnote/integrations/integrationWebview.ts`:
- Around line 439-460: Analytics events for configure_integration,
reset_integration, and delete_integration lack the integrationType property,
causing inconsistent dimensions; update the handlers that call
this.showConfigurationForm, this.resetConfiguration, and the delete handler to
include properties: { integrationType: <type> } similar to save_integration;
obtain the type by resolving the integrationId (e.g., via an existing helper or
registry like getIntegrationType/integrationRegistry.lookup by id) and fall back
to 'unknown' if resolution fails, then pass that value in the
this.analytics?.trackEvent call for configure_integration, reset_integration,
and delete_integration.
In `@src/notebooks/deepnote/openInDeepnoteHandler.node.ts`:
- Around line 24-27: The analytics event is emitted regardless of failures
because handleOpenInDeepnote() swallows errors; update the flow so tracking only
happens on confirmed success: change handleOpenInDeepnote (or create a small
wrapper) to return a boolean or throw on failure, and only call
this.analytics?.trackEvent({ eventName: 'open_in_deepnote' }) after a successful
env.openExternal(...) completes (or when the handler returns true). Reference
Commands.OpenInDeepnote and the handleOpenInDeepnote function to locate the call
site and ensure errors from env.openExternal are propagated or translated into a
success flag before tracking.
In `@src/platform/analytics/telemetryService.ts`:
- Around line 60-74: The initialize() method marks this.initialized too early
and fires async updateValue without awaiting, which can leave telemetry in a
broken state; change initialize() so it performs
stateFactory.createGlobalPersistentState and awaits
userIdState.updateValue(generateUuid()) (or otherwise ensures the value is set)
before constructing the PostHog client and only set this.initialized = true at
the end; also wrap state/client creation in try/catch and ensure on any failure
this.initialized remains false so future calls will retry, referencing
initialize(), userIdState, stateFactory.createGlobalPersistentState,
userIdState.updateValue(generateUuid()), and PostHog client construction.
- Around line 76-84: The isTelemetryEnabled() function currently allows
telemetry for any telemetryLevel except 'off', but usage telemetry must only run
when telemetryLevel === 'all'; change the gate so telemetryLevel is compared
strictly to 'all' and only then return the deepnote.telemetry.enabled setting
(i.e., replace the current if-check that returns false for 'off' with a check
that returns false unless telemetryLevel === 'all'); update references inside
isTelemetryEnabled, which reads telemetryLevel from
workspace.getConfiguration('telemetry').get<string>('telemetryLevel', 'all') and
the deepnote setting via
workspace.getConfiguration('deepnote').get<boolean>('telemetry.enabled', true).
- Around line 69-73: Guard PostHog initialization by checking POSTHOG_API_KEY
for unresolved placeholders before calling new PostHog: in the constructor or
where this.client is set, detect empty or placeholder values like
"__POSTHOG_API_KEY__" (or a literal "POSTHOG_API_KEY") and short-circuit by
leaving this.client null/undefined and emitting a warning via the logger instead
of constructing a non-functional client; keep the new PostHog(...) call only
when the key is valid, and ensure other TelemetryService methods that use
this.client handle the null/undefined client safely.
In `@src/platform/analytics/telemetryService.unit.test.ts`:
- Around line 104-127: Add an assertion that the analytics client's capture was
invoked with the expected payload: after calling TelemetryService.trackEvent({
eventName: 'open_notebook' }) and extracting generatedId from
mockUserIdState.updateValue, assert that the mocked capture method (e.g.,
analyticsClient.capture or mockAnalyticsClient.capture) was called once and that
its first call args include an object with event equal to 'open_notebook' and
distinctId equal to generatedId (or contains distinctId property matching
generatedId); use the existing mocks/stubs for the analytics client used by
TelemetryService to locate the capture call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8174ddd7-996d-43f4-89b0-71497dfe9a56
📒 Files selected for processing (10)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.tssrc/notebooks/deepnote/deepnoteCellExecutionAnalytics.tssrc/notebooks/deepnote/deepnoteExplorerView.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.tssrc/notebooks/notebookCommandListener.tssrc/platform/analytics/telemetryService.tssrc/platform/analytics/telemetryService.unit.test.tssrc/platform/analytics/types.ts
Comment on lines
+28
to
+58
| public trackEvent({ eventName, properties }: TelemetryEvent): void { | ||
| try { | ||
| if (!this.isTelemetryEnabled()) { | ||
| return; | ||
| } | ||
|
|
||
| if (!this.initialized) { | ||
| this.initialize(); | ||
| } | ||
|
|
||
| if (!this.client || !this.userIdState) { | ||
| return; | ||
| } | ||
|
|
||
| this.client.capture({ | ||
| distinctId: this.userIdState.value, | ||
| event: eventName, | ||
| properties | ||
| }); | ||
| } catch (ex) { | ||
| logger.debug(`PostHog analytics error: ${ex}`); | ||
| } | ||
| } | ||
|
|
||
| public async dispose(): Promise<void> { | ||
| try { | ||
| await this.client?.shutdown(); | ||
| } catch (ex) { | ||
| logger.debug(`PostHog shutdown error: ${ex}`); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reorder public methods to match repo ordering rule.
dispose should come before trackEvent (same accessibility, alphabetical order).
As per coding guidelines, "Order method, fields and properties, first by accessibility and then by alphabetical order".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/analytics/telemetryService.ts` around lines 28 - 58, Move the
public async dispose(): Promise<void> method to appear before the public
trackEvent({ eventName, properties }: TelemetryEvent): void method so public
methods follow the repository rule (ordered by accessibility then
alphabetically); locate the dispose and trackEvent methods in
telemetryService.ts and reorder them accordingly while keeping their bodies
unchanged (ensure any references between them still compile).
Comment on lines
+78
to
+102
| test('trackEvent should not throw when VSCode telemetry level is off', () => { | ||
| const vscode = require('vscode'); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| sandbox.stub(vscode.workspace, 'getConfiguration').callsFake((section: any) => ({ | ||
| get: (_key: string, defaultValue: unknown) => { | ||
| if (section === 'telemetry' && _key === 'telemetryLevel') { | ||
| return 'off'; | ||
| } | ||
|
|
||
| return defaultValue; | ||
| } | ||
| })); | ||
|
|
||
| analyticsService = new TelemetryService(mockStateFactory, mockAsyncDisposableRegistry); | ||
|
|
||
| assert.doesNotThrow(() => { | ||
| analyticsService.trackEvent({ eventName: 'open_notebook' }); | ||
| }); | ||
|
|
||
| assert.isFalse( | ||
| (mockStateFactory.createGlobalPersistentState as sinon.SinonStub).called, | ||
| 'Should not create persistent state when VSCode telemetry is off' | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add tests for telemetryLevel = 'error' and 'crash'.
Coverage currently checks only 'off'; add cases for the other non-all levels to pin telemetry gating behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/analytics/telemetryService.unit.test.ts` around lines 78 - 102,
Add two unit tests mirroring the existing "trackEvent should not throw when
VSCode telemetry level is off" test but set the stubbed configuration's
telemetryLevel to 'error' and 'crash' respectively; for each test, stub
vscode.workspace.getConfiguration to return get(...) => 'error' (or 'crash')
when section === 'telemetry' and _key === 'telemetryLevel', instantiate
TelemetryService (TelemetryService), call analyticsService.trackEvent({
eventName: 'open_notebook' }) inside assert.doesNotThrow, and assert that
mockStateFactory.createGlobalPersistentState was not called to verify telemetry
is gated for those levels.
Add TelemetryWebService as a no-op implementation registered in the web service registry, allowing ITelemetryService to be injected as required everywhere instead of optional. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/notebooks/deepnote/openInDeepnoteHandler.node.ts (1)
24-27:⚠️ Potential issue | 🟠 Major
open_in_deepnoteis still emitted on failed operations.
handleOpenInDeepnote()handles errors internally, so the callback always reachestrackEvent. Gate tracking on an explicit success result instead.Proposed fix
- commands.registerCommand(Commands.OpenInDeepnote, async () => { - await this.handleOpenInDeepnote(); - this.analytics.trackEvent({ eventName: 'open_in_deepnote' }); - }) + commands.registerCommand(Commands.OpenInDeepnote, async () => { + const success = await this.handleOpenInDeepnote(); + if (success) { + this.analytics.trackEvent({ eventName: 'open_in_deepnote' }); + } + }) ); } - private async handleOpenInDeepnote(): Promise<void> { + private async handleOpenInDeepnote(): Promise<boolean> { try { @@ - return; + return false; @@ - return; + return false; @@ - await env.openExternal(Uri.parse(deepnoteUrl)); + const opened = await env.openExternal(Uri.parse(deepnoteUrl)); + if (!opened) { + return false; + } @@ - logger.info('Successfully opened file in Deepnote'); + logger.info('Successfully opened file in Deepnote'); + return true; } catch (error) { @@ + return false; } } ); } catch (error) { @@ + return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/openInDeepnoteHandler.node.ts` around lines 24 - 27, The command callback always calls analytics.trackEvent despite failures because handleOpenInDeepnote swallows errors; change handleOpenInDeepnote to return an explicit success boolean (or throw on fatal failure) and update the Commands.OpenInDeepnote registration so it awaits the result and only calls analytics.trackEvent({ eventName: 'open_in_deepnote' }) when the returned value indicates success (or no exception was thrown). Refer to handleOpenInDeepnote and analytics.trackEvent in your changes and ensure the registerCommand callback checks the success flag before tracking.src/notebooks/deepnote/deepnoteCellExecutionAnalytics.ts (1)
15-19: 🛠️ Refactor suggestion | 🟠 MajorReorder the constructor-promoted dependencies.
disposablesshould sort beforenotebookManagerhere.As per coding guidelines: `**/*.{ts,tsx}`: Order method, fields and properties, first by accessibility and then by alphabetical order.
↕️ Suggested reorderconstructor( `@inject`(ITelemetryService) private readonly analytics: ITelemetryService, - `@inject`(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager, - `@inject`(IDisposableRegistry) private readonly disposables: Disposable[] + `@inject`(IDisposableRegistry) private readonly disposables: Disposable[], + `@inject`(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager ) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteCellExecutionAnalytics.ts` around lines 15 - 19, Constructor-promoted dependencies in deepnoteCellExecutionAnalytics.ts are out of the required alphabetical order: move the disposables parameter (type Disposable[] / token IDisposableRegistry) before notebookManager (IDeepnoteNotebookManager) so the constructor signature reads analytics, disposables, notebookManager; update the parameter order in the constructor declaration and any corresponding property uses to match this new ordering (constructor, analytics, disposables, notebookManager).src/notebooks/deepnote/deepnoteExplorerView.ts (1)
337-340:⚠️ Potential issue | 🟡 MinorOnly track completed actions.
Each helper awaited here returns
void, and several of them early-return or swallow failures. These handlers will therefore count cancelled or failed opens, creates, imports, deletes, duplicates, and exports as successful usage. Return a success flag/result, or movetrackEventinto the helper’s success path.Based on learnings: `Applies to **/*.{ts,tsx} : Verify state after async setup operations - methods can return early without throwing, so check expected state was created`✅ One pattern to apply
- private async newProject(): Promise<void> { + private async newProject(): Promise<boolean> { if (!workspace.workspaceFolders || workspace.workspaceFolders.length === 0) { ... - return; + return false; } ... if (!projectName) { - return; + return false; } try { ... + return true; } catch (error) { ... + return false; } } - commands.registerCommand(Commands.NewProject, async () => { - await this.newProject(); - this.analytics.trackEvent({ eventName: 'create_project' }); - }) + commands.registerCommand(Commands.NewProject, async () => { + if (await this.newProject()) { + this.analytics.trackEvent({ eventName: 'create_project' }); + } + })Also applies to: 352-355, 359-362, 366-369, 373-376, 387-390, 400-403, 407-410, 420-423, 427-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 337 - 340, The event is tracked unconditionally after awaiting helpers that may early-return or swallow failures; update the handlers (e.g., the Commands.OpenDeepnoteNotebook registration) to only call this.analytics.trackEvent when the helper actually succeeded by changing the helper signature (e.g., openNotebook) to return a success boolean/result or by inspecting a success state it sets, then await the helper into a variable and call trackEvent only if that variable indicates success; apply the same pattern to the other affected command handlers referenced in this file (the registrations around the listed ranges) so tracking happens on confirmed success paths only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts`:
- Around line 65-67: The tests currently inject telemetry as an inline stub and
never assert telemetry side effects; extract the telemetry object into a
variable (e.g., const telemetryStub = { trackEvent: sinon.stub(), dispose:
sinon.stub().resolves() } as unknown as ITelemetryService) and pass that into
the DeepnoteEnvironmentsView constructor, then in the success-path unit tests
for create/delete/select flows (the tests exercising the methods that
createEnvironment, deleteEnvironment, and selectEnvironment or whatever test
helpers trigger those flows) add assertions that telemetryStub.trackEvent was
called with the expected event names and payloads (use sinon.assert.calledWith
or similar) to verify telemetry side effects. Ensure you reset/restore the stub
between tests.
In `@src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts`:
- Around line 18-19: The telemetry mock (mockAnalytics) is currently a no-op
which prevents verifying analytics paths; replace the no-op methods with
observable spies/mocks so tests can assert calls: change
mockAnalytics.trackEvent from () => undefined to a spy/mock function (e.g.,
jest.fn or sinon.spy) and make dispose an async mock (e.g.,
jest.fn().mockResolvedValue(undefined)), keeping the ITelemetryService cast;
update tests to assert on mockAnalytics.trackEvent and mockAnalytics.dispose
where cancel/early-return behavior should trigger telemetry.
In `@src/notebooks/deepnote/deepnoteNotebookCommandListener.ts`:
- Line 269: Extract the repeated tracking call into a single helper to avoid
duplicated payloads: create a method (e.g., trackAddBlock(blockType) or
addBlockAnalytics) that wraps this.analytics.trackEvent({ eventName:
'add_block', properties: { blockType } }) and replace the five inline usages
(the current direct calls to this.analytics.trackEvent with eventName
'add_block' at the five success paths) with calls to that helper; update any
callers to pass the literal 'sql' (or other blockType) as needed so all
locations (the existing occurrences including the one shown) use the central
helper.
In `@src/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.ts`:
- Around line 53-56: The tests create OpenInDeepnoteHandlerClass with a stubbed
ITelemetryService but never assert telemetry calls; update the unit tests for
the registered command callback on the handler (the code path invoked by
handler.registerCommand or the exported command callback) to assert that the
telemetryService.trackEvent stub is called exactly when the open flow succeeds
and not called on failure. Specifically, locate the handler instance (handler)
and the injected telemetry stub (trackEvent) and add two test cases: one
simulating a successful open (stub any dependencies to return success) that
expects trackEvent.calledOnceWithMatch(...) and one simulating a failure or
early return that expects trackEvent.notCalled; keep assertions focused on
trackEvent and use the existing sinon stub on dispose/trackEvent to verify
behavior.
In `@src/platform/analytics/telemetryWebService.ts`:
- Around line 6-13: Reorder the public methods in the TelemetryWebService class
so they follow alphabetical class-member ordering: move the public async
dispose(): Promise<void> method to appear before the public trackEvent(_event:
TelemetryEvent): void method; ensure method signatures and no-op bodies remain
unchanged and that the class still implements ITelemetryService.
---
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteCellExecutionAnalytics.ts`:
- Around line 15-19: Constructor-promoted dependencies in
deepnoteCellExecutionAnalytics.ts are out of the required alphabetical order:
move the disposables parameter (type Disposable[] / token IDisposableRegistry)
before notebookManager (IDeepnoteNotebookManager) so the constructor signature
reads analytics, disposables, notebookManager; update the parameter order in the
constructor declaration and any corresponding property uses to match this new
ordering (constructor, analytics, disposables, notebookManager).
In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 337-340: The event is tracked unconditionally after awaiting
helpers that may early-return or swallow failures; update the handlers (e.g.,
the Commands.OpenDeepnoteNotebook registration) to only call
this.analytics.trackEvent when the helper actually succeeded by changing the
helper signature (e.g., openNotebook) to return a success boolean/result or by
inspecting a success state it sets, then await the helper into a variable and
call trackEvent only if that variable indicates success; apply the same pattern
to the other affected command handlers referenced in this file (the
registrations around the listed ranges) so tracking happens on confirmed success
paths only.
In `@src/notebooks/deepnote/openInDeepnoteHandler.node.ts`:
- Around line 24-27: The command callback always calls analytics.trackEvent
despite failures because handleOpenInDeepnote swallows errors; change
handleOpenInDeepnote to return an explicit success boolean (or throw on fatal
failure) and update the Commands.OpenInDeepnote registration so it awaits the
result and only calls analytics.trackEvent({ eventName: 'open_in_deepnote' })
when the returned value indicates success (or no exception was thrown). Refer to
handleOpenInDeepnote and analytics.trackEvent in your changes and ensure the
registerCommand callback checks the success flag before tracking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c78fe551-eaf3-43b4-abc6-158409c1dc1e
📒 Files selected for processing (15)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteCellExecutionAnalytics.tssrc/notebooks/deepnote/deepnoteExplorerView.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.tssrc/notebooks/notebookCommandListener.tssrc/platform/analytics/telemetryWebService.tssrc/platform/serviceRegistry.web.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts (1)
18-19:⚠️ Potential issue | 🟠 MajorUse an observable telemetry test double, not
NoOpTelemetryService.This wires the new constructor but still can’t verify telemetry side effects (including early-return/cancel branches), so the telemetry feature remains largely untested in this suite.
Suggested change
-import { NoOpTelemetryService } from '../../platform/analytics/noOpTelemetryService'; +import type { ITelemetryService } from '../../platform/analytics/types'; -const mockAnalytics = new NoOpTelemetryService(); +const mockAnalytics = { + trackEvent: sinon.stub(), + dispose: sinon.stub().resolves() +} as unknown as ITelemetryService;Then assert expected calls in relevant tests, e.g. cancel paths vs success paths.
Also applies to: 58-59, 196-197, 240-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts` around lines 18 - 19, Replace the NoOpTelemetryService test double (the const mockAnalytics = new NoOpTelemetryService()) with an observable/mock telemetry implementation that records calls (e.g., MockObservableTelemetryService or ObservableTelemetrySpy) and pass that into the DeepnoteExplorerView constructor so tests can assert telemetry side effects; update tests that exercise cancel/early-return and success paths to assert expected telemetry method invocations (or counts/arguments) instead of using the no-op, and reuse the same mockTelemetry instance across the tests that currently declare mockAnalytics to verify cancel vs success branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 377-380: The telemetry calls for Commands.NewNotebook
(analytics.trackEvent) are inconsistent because newNotebook and
duplicateNotebook fire events unconditionally; change both to follow the `{
completed }` pattern used elsewhere: call analytics.trackEvent with `{
eventName: 'create_notebook', completed: true }` only after the awaited
operation succeeds, and in a catch/finally ensure you send `{ completed: false
}` (or `{ completed }` value) on failure; update the handlers for
Commands.NewNotebook and the duplicate command to wrap await
this.newNotebook()/this.duplicateNotebook() in try/catch and report completed
accordingly so telemetry matches the other commands.
- Around line 411-414: The duplicateNotebook command handler is inconsistent:
unlike delete/export/import it does not check a completion status from
duplicateNotebook before tracking analytics. Update duplicateNotebook (and its
callers) to return a boolean or result indicating success/failure, then change
the commands.registerCommand handler for Commands.DuplicateNotebook to await the
result and only call this.analytics.trackEvent({ eventName: 'duplicate_notebook'
}) when the returned value indicates success; reference the duplicateNotebook
method and the Commands.DuplicateNotebook registration to locate where to change
the behavior.
In `@src/platform/analytics/noOpTelemetryService.ts`:
- Around line 7-13: Reorder the methods in the NoOp telemetry implementation so
that dispose comes before trackEvent to satisfy the alphabetical method ordering
rule: move the public async dispose(): Promise<void> method so it appears before
public trackEvent(_event: TelemetryEvent): void (references: dispose,
trackEvent) while preserving their no-op bodies and signatures.
---
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts`:
- Around line 18-19: Replace the NoOpTelemetryService test double (the const
mockAnalytics = new NoOpTelemetryService()) with an observable/mock telemetry
implementation that records calls (e.g., MockObservableTelemetryService or
ObservableTelemetrySpy) and pass that into the DeepnoteExplorerView constructor
so tests can assert telemetry side effects; update tests that exercise
cancel/early-return and success paths to assert expected telemetry method
invocations (or counts/arguments) instead of using the no-op, and reuse the same
mockTelemetry instance across the tests that currently declare mockAnalytics to
verify cancel vs success branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 050a7bcf-bceb-4b18-859f-81e9ede78ceb
📒 Files selected for processing (7)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.tssrc/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.tssrc/platform/analytics/noOpTelemetryService.ts
Comment on lines
+377
to
+380
| commands.registerCommand(Commands.NewNotebook, async () => { | ||
| await this.newNotebook(); | ||
| this.analytics.trackEvent({ eventName: 'create_notebook' }); | ||
| }) |
Contributor
There was a problem hiding this comment.
Inconsistent telemetry pattern.
newNotebook and duplicateNotebook fire events unconditionally, unlike other commands that track { completed }. Consider aligning for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 377 - 380, The
telemetry calls for Commands.NewNotebook (analytics.trackEvent) are inconsistent
because newNotebook and duplicateNotebook fire events unconditionally; change
both to follow the `{ completed }` pattern used elsewhere: call
analytics.trackEvent with `{ eventName: 'create_notebook', completed: true }`
only after the awaited operation succeeds, and in a catch/finally ensure you
send `{ completed: false }` (or `{ completed }` value) on failure; update the
handlers for Commands.NewNotebook and the duplicate command to wrap await
this.newNotebook()/this.duplicateNotebook() in try/catch and report completed
accordingly so telemetry matches the other commands.
Comment on lines
+411
to
+414
| commands.registerCommand(Commands.DuplicateNotebook, async (treeItem: DeepnoteTreeItem) => { | ||
| await this.duplicateNotebook(treeItem); | ||
| this.analytics.trackEvent({ eventName: 'duplicate_notebook' }); | ||
| }) |
Contributor
There was a problem hiding this comment.
Same inconsistency here.
duplicateNotebook doesn't return a completion status. Pattern differs from delete/export/import commands.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 411 - 414, The
duplicateNotebook command handler is inconsistent: unlike delete/export/import
it does not check a completion status from duplicateNotebook before tracking
analytics. Update duplicateNotebook (and its callers) to return a boolean or
result indicating success/failure, then change the commands.registerCommand
handler for Commands.DuplicateNotebook to await the result and only call
this.analytics.trackEvent({ eventName: 'duplicate_notebook' }) when the returned
value indicates success; reference the duplicateNotebook method and the
Commands.DuplicateNotebook registration to locate where to change the behavior.
Comment on lines
+7
to
+13
| public trackEvent(_event: TelemetryEvent): void { | ||
| // No-op | ||
| } | ||
|
|
||
| public async dispose(): Promise<void> { | ||
| // No-op | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Method order: dispose should precede trackEvent alphabetically.
Suggested fix
export class NoOpTelemetryService implements ITelemetryService {
+ public async dispose(): Promise<void> {
+ // No-op
+ }
+
public trackEvent(_event: TelemetryEvent): void {
// No-op
}
-
- public async dispose(): Promise<void> {
- // No-op
- }
}As per coding guidelines: **/*.{ts,tsx}: Order method, fields and properties, first by accessibility and then by alphabetical order.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public trackEvent(_event: TelemetryEvent): void { | |
| // No-op | |
| } | |
| public async dispose(): Promise<void> { | |
| // No-op | |
| } | |
| export class NoOpTelemetryService implements ITelemetryService { | |
| public async dispose(): Promise<void> { | |
| // No-op | |
| } | |
| public trackEvent(_event: TelemetryEvent): void { | |
| // No-op | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/analytics/noOpTelemetryService.ts` around lines 7 - 13, Reorder
the methods in the NoOp telemetry implementation so that dispose comes before
trackEvent to satisfy the alphabetical method ordering rule: move the public
async dispose(): Promise<void> method so it appears before public
trackEvent(_event: TelemetryEvent): void (references: dispose, trackEvent) while
preserving their no-op bodies and signatures.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.ts (1)
53-53:⚠️ Potential issue | 🟠 MajorTelemetry is injected but still not asserted.
Line 53 adds
ITelemetryService, but this suite still doesn’t verifytrackEventbehavior. Add one success-path assertion (trackEventcalled) and one failure/early-return assertion (trackEventnot called) around the command callback flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.ts` at line 53, The test injects a mocked ITelemetryService into OpenInDeepnoteHandlerClass but never asserts telemetry calls; update the suite to (1) add a success-path test that executes the command callback on handler (or calls the method that triggers the command) with valid inputs and assert the mocked ITelemetryService.trackEvent was called (with expected event name/props) and (2) add a failure/early-return test that triggers the early-return branch (e.g., invalid input or stubbed dependency to force failure) and assert trackEvent was not called; reference OpenInDeepnoteHandlerClass, handler, and ITelemetryService.trackEvent when locating where to add these two assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooks/deepnote/deepnoteActivationService.unit.test.ts`:
- Line 29: Move the suite-level mock creation for mockAnalytics into the test
setup to avoid shared state between tests: instead of const mockAnalytics =
instance(mock<ITelemetryService>()) declared at top-level, initialize a fresh
mock in the beforeEach/setup block (e.g., recreate the mock object and its
instance there) so tests get isolated instances and future verify() calls won't
be affected by accumulated interactions.
In `@src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts`:
- Around line 18-19: The module-global mockAnalytics (created with
instance(mock<ITelemetryService>())) can leak interactions across tests; move
its creation into the test suite setup so each test gets a fresh mock and reset
interactions before each test. Specifically, remove the top-level const
mockAnalytics and instead instantiate or reassign mockAnalytics inside the
beforeEach/setup block (or call a reset/mockClear helper) where the tests
reference mockAnalytics, ensuring any spies/stubs on ITelemetryService are
isolated per test; apply the same change for the other module-global telemetry
mocks referenced in this file.
---
Duplicate comments:
In `@src/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.ts`:
- Line 53: The test injects a mocked ITelemetryService into
OpenInDeepnoteHandlerClass but never asserts telemetry calls; update the suite
to (1) add a success-path test that executes the command callback on handler (or
calls the method that triggers the command) with valid inputs and assert the
mocked ITelemetryService.trackEvent was called (with expected event name/props)
and (2) add a failure/early-return test that triggers the early-return branch
(e.g., invalid input or stubbed dependency to force failure) and assert
trackEvent was not called; reference OpenInDeepnoteHandlerClass, handler, and
ITelemetryService.trackEvent when locating where to add these two assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 81dca3df-aa35-47d0-9a06-534158f2b831
📒 Files selected for processing (5)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.tssrc/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/openInDeepnoteHandler.node.unit.test.ts
| let manager: DeepnoteNotebookManager; | ||
| let mockIntegrationManager: IIntegrationManager; | ||
| let mockLogger: ILogger; | ||
| const mockAnalytics = instance(mock<ITelemetryService>()); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider moving mock creation into setup() for test isolation.
Creating the mock at suite level shares state across tests. Fine for now since no verify() calls exist, but if future tests need verification, this pattern will cause false positives from accumulated calls.
♻️ Optional refactor
- const mockAnalytics = instance(mock<ITelemetryService>());
+ let mockAnalytics: ITelemetryService;
setup(() => {
+ mockAnalytics = instance(mock<ITelemetryService>());
mockExtensionContext = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/deepnoteActivationService.unit.test.ts` at line 29,
Move the suite-level mock creation for mockAnalytics into the test setup to
avoid shared state between tests: instead of const mockAnalytics =
instance(mock<ITelemetryService>()) declared at top-level, initialize a fresh
mock in the beforeEach/setup block (e.g., recreate the mock object and its
instance there) so tests get isolated instances and future verify() calls won't
be affected by accumulated interactions.
Comment on lines
+18
to
+19
| const mockAnalytics = instance(mock<ITelemetryService>()); | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid shared telemetry mock across suites.
mockAnalytics is module-global, so interactions can bleed between tests. Prefer creating/resetting it in setup() for isolation.
♻️ Suggested refactor
-const mockAnalytics = instance(mock<ITelemetryService>());
+let mockAnalytics: ITelemetryService;
...
setup(() => {
+ mockAnalytics = instance(mock<ITelemetryService>());
mockExtensionContext = {
subscriptions: []
} as any;
...
});
...
setup(() => {
+ mockAnalytics = instance(mock<ITelemetryService>());
sandbox = sinon.createSandbox();
resetVSCodeMocks();
uuidStubs = [];Also applies to: 58-59, 196-197, 240-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts` around lines 18 -
19, The module-global mockAnalytics (created with
instance(mock<ITelemetryService>())) can leak interactions across tests; move
its creation into the test suite setup so each test gets a fresh mock and reset
interactions before each test. Specifically, remove the top-level const
mockAnalytics and instead instantiate or reassign mockAnalytics inside the
beforeEach/setup block (or call a reset/mockClear helper) where the tests
reference mockAnalytics, ensuring any spies/stubs on ITelemetryService are
isolated per test; apply the same change for the other module-global telemetry
mocks referenced in this file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PostHogAnalyticsService(Node.js only) that lazily initializes on first event, persists an anonymous user ID inglobalState, and respects both VSCode'stelemetry.telemetryLeveland the newdeepnote.telemetry.enabledsetting (default: true)Events Tracked
add_blockblockTypeexecute_cellcellType,integrationType?(SQL)execute_notebookselect_environmentcreate_environmenthasPackages,hasDescriptiondelete_environmentcreate_projectcreate_notebookopen_notebookimport_notebookexport_notebookformatdelete_projectdelete_notebookduplicate_notebookconfigure_integrationsave_integrationintegrationTypedelete_integrationreset_integrationtoggle_snapshotsenabledopen_in_deepnoteTest plan
npm run compile-tsc— no type errorsnpm run format-fix— passesnpm run test:unittests— 2211 passing, 0 failuresdeepnote.telemetry.enabled→ no events senttelemetry.telemetryLevel: off→ no events sent🤖 Generated with Claude Code
Summary by CodeRabbit