Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions packages/cloud/src/TelemetryClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
rooCodeTelemetryEventSchema,
TelemetryPropertiesProvider,
TelemetryEventSubscription,
type StaticAppProperties,
} from "@roo-code/types"

import { getRooCodeApiUrl } from "./config.js"
Expand All @@ -16,6 +17,12 @@ import type { RetryQueue } from "./retry-queue/index.js"
abstract class BaseTelemetryClient implements TelemetryClient {
protected providerRef: WeakRef<TelemetryPropertiesProvider> | null = null
protected telemetryEnabled: boolean = false
/**
* Cached static app properties captured when the provider is set.
* These are used as fallback when the provider is no longer available
* (e.g., after ClineProvider is disposed).
*/
protected cachedStaticProperties: StaticAppProperties | null = null

constructor(
public readonly subscription?: TelemetryEventSubscription,
Expand Down Expand Up @@ -54,6 +61,10 @@ abstract class BaseTelemetryClient implements TelemetryClient {
`Error getting telemetry properties: ${error instanceof Error ? error.message : String(error)}`,
)
}
} else if (this.cachedStaticProperties) {
// Provider is no longer available (e.g., ClineProvider was disposed).
// Use cached static properties to ensure required telemetry fields are present.
providerProperties = { ...this.cachedStaticProperties }
}

// Merge provider properties with event-specific properties.
Expand All @@ -73,6 +84,16 @@ abstract class BaseTelemetryClient implements TelemetryClient {

public setProvider(provider: TelemetryPropertiesProvider): void {
this.providerRef = new WeakRef(provider)

// Capture static app properties immediately so they remain available
// even after the provider is garbage collected.
// This is especially important for ClineProvider which may be disposed
// when the webview is closed, but telemetry events may still be in flight.
if ("appProperties" in provider) {
this.cachedStaticProperties = (
provider as TelemetryPropertiesProvider & { appProperties: StaticAppProperties }
).appProperties
}
Comment on lines +92 to +96
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caches only static properties, but telemetryPropertiesSchema also requires language and mode from dynamicAppPropertiesSchema. When the provider is disposed, if the event's properties don't include these fields, validation at rooCodeTelemetryEventSchema.safeParse() will still fail. Consider also caching the dynamic properties, or updating the PR description to clarify this is a partial fix that only addresses static property errors.

Fix it with Roo Code or mention @roomote and request a fix.

}

public abstract updateTelemetryState(didUserOptIn: boolean): void
Expand Down
71 changes: 71 additions & 0 deletions packages/cloud/src/__tests__/TelemetryClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,77 @@ describe("TelemetryClient", () => {

expect(result).toEqual({ customProp: "value" })
})

it("should use cached static properties when provider is no longer available", async () => {
const client = new TelemetryClient(mockAuthService, mockSettingsService)

const staticProperties = {
appName: "roo-code",
appVersion: "1.0.0",
vscodeVersion: "1.60.0",
platform: "darwin",
editorName: "vscode",
}

// Create a mock provider with appProperties
const mockProvider: TelemetryPropertiesProvider & { appProperties: any } = {
appProperties: staticProperties,
getTelemetryProperties: vi.fn().mockResolvedValue({
...staticProperties,
language: "en",
mode: "code",
}),
}

// Set the provider (this should cache the static properties)
client.setProvider(mockProvider)

// Verify cached properties were set
const cachedProps = getPrivateProperty<any>(client, "cachedStaticProperties")
expect(cachedProps).toEqual(staticProperties)

// Now simulate the provider being garbage collected by setting providerRef to return undefined
// We do this by clearing the providerRef directly
;(client as any).providerRef = new WeakRef({} as any) // WeakRef to an empty object that will be different
// Force the WeakRef to return undefined by overriding deref
;(client as any).providerRef = { deref: () => undefined }

const getEventProperties = getPrivateProperty<
(event: { event: TelemetryEventName; properties?: Record<string, any> }) => Promise<Record<string, any>>
>(client, "getEventProperties").bind(client)

const result = await getEventProperties({
event: TelemetryEventName.TASK_CREATED,
properties: { customProp: "value" },
})

// Should include cached static properties when provider is unavailable
expect(result).toEqual({
...staticProperties,
customProp: "value",
})
})

it("should not cache properties when provider does not have appProperties", async () => {
const client = new TelemetryClient(mockAuthService, mockSettingsService)

// Create a mock provider WITHOUT appProperties
const mockProvider: TelemetryPropertiesProvider = {
getTelemetryProperties: vi.fn().mockResolvedValue({
appName: "roo-code",
appVersion: "1.0.0",
language: "en",
mode: "code",
}),
}

// Set the provider
client.setProvider(mockProvider)

// Verify cached properties were NOT set (provider doesn't have appProperties)
const cachedProps = getPrivateProperty<any>(client, "cachedStaticProperties")
expect(cachedProps).toBeNull()
})
})

describe("capture", () => {
Expand Down
Loading