Skip to content
Merged
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
Original file line number Diff line number Diff line change
@@ -1,24 +1,27 @@
"use client";

import { useUser } from "@clerk/nextjs";
import { useQuery } from "@tanstack/react-query";
import posthog from "posthog-js";
import { useEffect } from "react";
import { useTRPC } from "../../trpc/react";

export function PostHogUserIdentifier() {
const { user, isLoaded } = useUser();
const { isSignedIn } = useUser();
const trpc = useTRPC();

useEffect(() => {
if (!isLoaded) return;
const { data: user } = useQuery({
...trpc.user.me.queryOptions(),
enabled: isSignedIn,
});

useEffect(() => {
if (user) {
posthog.identify(user.id, {
email: user.primaryEmailAddress?.emailAddress,
name: user.fullName,
});
} else {
posthog.identify(user.id, { email: user.email, name: user.name });
} else if (isSignedIn === false) {
posthog.reset();
}
}, [user, isLoaded]);
}, [user, isSignedIn]);
Comment on lines 9 to +24
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

posthog.reset() is gated on isSignedIn === false, but not on the me query being finished. If the query is still in-flight and user is undefined, this effect currently does nothing (good), but when isSignedIn flips to false you reset immediately even if there’s still cached React Query data for user.me (or a stale user value). Consider resetting based on query state (e.g., isFetched/isSuccess) to avoid edge cases where you briefly identify with a stale user after sign-out or during session transitions.

Suggestion

Consider explicitly using the React Query status to drive reset/identify:

  • Destructure isSuccess/isFetched (or status) from useQuery.
  • Only call posthog.identify when isSuccess && user.
  • Only call posthog.reset when isSuccess && !user or when isSignedIn === false and the query has settled.

Example:

const { data: user, isSuccess } = useQuery({
  ...trpc.user.me.queryOptions(),
  enabled: isSignedIn,
});

useEffect(() => {
  if (!isSuccess) return;
  if (user) posthog.identify(user.id, { email: user.email, name: user.name });
  else posthog.reset();
}, [isSuccess, user]);

Reply with "@CharlieHelps yes please" if you’d like me to add a commit implementing this change.


return null;
}
1 change: 1 addition & 0 deletions apps/desktop/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
"node-pty": "1.1.0-beta30",
"os-locale": "^6.0.2",
"posthog-js": "^1.306.1",
"posthog-node": "^5.18.0",
"react": "^19.2.3",
"react-arborist": "^3.4.3",
"react-dnd": "^16.0.1",
Expand Down
2 changes: 2 additions & 0 deletions apps/desktop/src/lib/trpc/routers/auth/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { observable } from "@trpc/server/observable";
import type { BrowserWindow } from "electron";
import { clearUserCache } from "main/lib/analytics";
import { authService } from "main/lib/auth";
import { AUTH_PROVIDERS } from "shared/auth";
import { z } from "zod";
Expand Down Expand Up @@ -53,6 +54,7 @@ export const createAuthRouter = (getWindow: () => BrowserWindow | null) => {
*/
signOut: publicProcedure.mutation(async () => {
await authService.signOut();
clearUserCache();
return { success: true };
}),
});
Expand Down
25 changes: 25 additions & 0 deletions apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { homedir } from "node:os";
import { join } from "node:path";
import { track } from "main/lib/analytics";
import { db } from "main/lib/db";
import { terminalManager } from "main/lib/terminal";
import { nanoid } from "nanoid";
Expand Down Expand Up @@ -166,6 +167,13 @@ export const createWorkspacesRouter = () => {
// Load setup configuration from the main repo (where .superset/config.json lives)
const setupConfig = loadSetupConfig(project.mainRepoPath);

track("workspace_created", {
workspace_id: workspace.id,
project_id: project.id,
branch: branch,
base_branch: targetBranch,
});

Comment on lines 167 to +176
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

track("workspace_created"/"workspace_opened") is called unconditionally after workspace creation/open. If any downstream logic throws after the DB update but before returning (e.g., config loading, other steps not shown), you may emit events for operations that ultimately fail from the user's perspective. This can pollute funnels and make metrics hard to trust.

Suggestion

Consider tracking only after the full mutation has succeeded (i.e., immediately before returning, after all possible failure points), or wrap the mutation body in a try/catch and emit separate failure events (e.g. workspace_create_failed) when exceptions occur.

Example pattern:

  • Do all work
  • return result
  • In a final step right before return, call track(...)

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this refactor across the workspace mutations.

return {
workspace,
initialCommands: setupConfig?.setup || null,
Expand Down Expand Up @@ -271,6 +279,13 @@ export const createWorkspacesRouter = () => {
}
});

track("workspace_opened", {
workspace_id: returnedWorkspace.id,
project_id: project.id,
type: "branch",
was_existing: wasExisting,
});

return {
workspace: returnedWorkspace,
worktreePath: project.mainRepoPath,
Expand Down Expand Up @@ -772,6 +787,8 @@ export const createWorkspacesRouter = () => {
? `${terminalResult.failed} terminal process(es) may still be running`
: undefined;

track("workspace_deleted", { workspace_id: input.id });

return { success: true, teardownError, terminalWarning };
}),

Expand Down Expand Up @@ -1059,6 +1076,12 @@ export const createWorkspacesRouter = () => {
// Load setup configuration from the main repo
const setupConfig = loadSetupConfig(project.mainRepoPath);

track("workspace_opened", {
workspace_id: workspace.id,
project_id: project.id,
type: "worktree",
});

return {
workspace,
initialCommands: setupConfig?.setup || null,
Expand Down Expand Up @@ -1110,6 +1133,8 @@ export const createWorkspacesRouter = () => {
? `${terminalResult.failed} terminal process(es) may still be running`
: undefined;

track("workspace_closed", { workspace_id: input.id });

return { success: true, terminalWarning };
}),
});
Expand Down
4 changes: 4 additions & 0 deletions apps/desktop/src/main/env.main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ export const env = createEnv({
NEXT_PUBLIC_WEB_URL: z.url().default("https://app.superset.sh"),
GOOGLE_CLIENT_ID: z.string().min(1),
GH_CLIENT_ID: z.string().min(1),
NEXT_PUBLIC_POSTHOG_KEY: z.string().optional(),
NEXT_PUBLIC_POSTHOG_HOST: z.string().default("https://us.i.posthog.com"),
},

runtimeEnv: {
Expand All @@ -29,6 +31,8 @@ export const env = createEnv({
NEXT_PUBLIC_WEB_URL: process.env.NEXT_PUBLIC_WEB_URL,
GOOGLE_CLIENT_ID: process.env.GOOGLE_CLIENT_ID,
GH_CLIENT_ID: process.env.GH_CLIENT_ID,
NEXT_PUBLIC_POSTHOG_KEY: process.env.NEXT_PUBLIC_POSTHOG_KEY,
NEXT_PUBLIC_POSTHOG_HOST: process.env.NEXT_PUBLIC_POSTHOG_HOST,
},
emptyStringAsUndefined: true,

Expand Down
6 changes: 4 additions & 2 deletions apps/desktop/src/main/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { app, BrowserWindow } from "electron";
import { makeAppSetup } from "lib/electron-app/factories/app/setup";
import { PROTOCOL_SCHEME } from "shared/constants";
import { setupAgentHooks } from "./lib/agent-setup";
import { shutdown as shutdownAnalytics, track } from "./lib/analytics";
import { initAppState } from "./lib/app-state";
import { authService, handleAuthDeepLink, isAuthDeepLink } from "./lib/auth";
import { setupAutoUpdater } from "./lib/auto-updater";
Expand Down Expand Up @@ -48,6 +49,7 @@ async function processDeepLink(url: string): Promise<void> {
refreshTokenExpiresAt: result.refreshTokenExpiresAt,
state: result.state,
});
track("auth_completed");
focusMainWindow();
} else {
console.error("[main] Auth deep link failed:", result.error);
Expand Down Expand Up @@ -138,9 +140,9 @@ if (!gotTheLock) {
await processDeepLink(coldStartUrl);
}

// Clean up all terminals when app is quitting
// Clean up all terminals and analytics when app is quitting
app.on("before-quit", async () => {
await terminalManager.cleanup();
await Promise.all([terminalManager.cleanup(), shutdownAnalytics()]);
});
})();
}
73 changes: 73 additions & 0 deletions apps/desktop/src/main/lib/analytics/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { env } from "main/env.main";
import { apiClient } from "main/lib/api-client";
import { PostHog } from "posthog-node";

let client: PostHog | null = null;
let cachedUserId: string | null = null;

function getClient(): PostHog | null {
if (!env.NEXT_PUBLIC_POSTHOG_KEY) {
return null;
}

if (!client) {
client = new PostHog(env.NEXT_PUBLIC_POSTHOG_KEY, {
host: env.NEXT_PUBLIC_POSTHOG_HOST,
flushAt: 1, // Send events immediately for desktop app
flushInterval: 0,
});
}
Comment on lines +13 to +19
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

posthog-node is configured with flushAt: 1 and flushInterval: 0, which can create very chatty network usage (each event becomes its own request) and may impact performance/battery on laptops, especially given workspace/terminal events can be frequent. Also, shutdown() only runs on before-quit; crashes/forced exits will drop events anyway, so paying the cost of per-event flush may not be worth it.

Suggestion

Use a small batch/interval to reduce overhead (e.g., flushAt: 5 and flushInterval: 5000) and rely on shutdown() to flush on graceful exit. Optionally call client?.flush() after high-value events (auth completed) if needed.

Reply with "@CharlieHelps yes please" if you'd like me to add a commit adjusting the PostHog client config and adding an optional flush() helper for critical moments.

return client;
}

async function getUserId(): Promise<string | null> {
if (cachedUserId) return cachedUserId;
try {
const user = await apiClient.user.me.query();
cachedUserId = user?.id ?? null;
return cachedUserId;
} catch {
return null;
}
}

/**
* Clear cached user ID (call on sign out)
*/
export function clearUserCache(): void {
cachedUserId = null;
}

/**
* Track an event with the current user's ID as distinct_id.
* Fire-and-forget - errors are silently ignored.
*/
export function track(
event: string,
properties?: Record<string, unknown>,
): void {
const posthog = getClient();
if (!posthog) return;

getUserId()
.then((userId) => {
if (!userId) return;
posthog.capture({
distinctId: userId,
event,
properties: {
...properties,
app_name: "desktop",
platform: process.platform,
},
});
})
.catch(() => {});
}

/**
* Shutdown PostHog client (call on app quit)
*/
export async function shutdown(): Promise<void> {
await client?.shutdown();
}
Comment on lines +1 to +73
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The analytics module drops events whenever getUserId() fails, and it never flushes/awaits delivery for important lifecycle events. With flushAt: 1 and flushInterval: 0 you are aiming for immediate delivery, but without awaiting capture / explicit flush, events can still be lost on shutdown.

Also, swallowing all errors (catch(() => {})) removes any ability to detect a misconfiguration (bad key/host) in production logs.

Suggestion

Improve reliability/observability while keeping it low-noise:

  1. For shutdown, explicitly await client?.flush() before shutdown() (or ensure shutdown() flushes in your posthog-node version).
  2. For track, consider capturing events even without a user via an app-scoped distinct id (e.g., a persisted installation id) so you don’t lose pre-auth events.
  3. Replace the blanket catch(() => {}) with a very low-volume logger gated by NODE_ENV !== 'test' and maybe a simple once-per-session guard.

Example:

let warned = false;
...
.catch((err) => {
  if (!warned && process.env.NODE_ENV !== 'test') {
    warned = true;
    console.warn('[analytics] capture failed', err);
  }
});

Reply with "@CharlieHelps yes please" if you’d like me to add a commit implementing (1) and a minimal (3).

6 changes: 3 additions & 3 deletions apps/desktop/src/main/lib/terminal/manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ import * as pty from "node-pty";
import { getHistoryDir } from "../terminal-history";
import { TerminalManager } from "./manager";

// Use real history implementation - it will write to tmpdir thanks to NODE_ENV=test
const testTmpDir = join(tmpdir(), "superset-test");

// Mock node-pty
mock.module("node-pty", () => ({
spawn: mock(() => {}),
}));

// Use real history implementation - it will write to tmpdir thanks to NODE_ENV=test
const testTmpDir = join(tmpdir(), "superset-test");

describe("TerminalManager", () => {
let manager: TerminalManager;
let mockPty: {
Expand Down
6 changes: 5 additions & 1 deletion apps/desktop/src/main/lib/terminal/manager.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { EventEmitter } from "node:events";
import { track } from "main/lib/analytics";
import { FALLBACK_SHELL, SHELL_CRASH_THRESHOLD_MS } from "./env";
import {
closeSessionHistory,
Expand Down Expand Up @@ -58,7 +59,7 @@ export class TerminalManager extends EventEmitter {
private async doCreateSession(
params: InternalCreateSessionParams,
): Promise<SessionResult> {
const { paneId, initialCommands } = params;
const { paneId, workspaceId, initialCommands } = params;

// Create the session
const session = await createSession(params, (id, data) => {
Expand All @@ -75,6 +76,9 @@ export class TerminalManager extends EventEmitter {

this.sessions.set(paneId, session);

// Track terminal opened (only fires once per session creation)
track("terminal_opened", { workspace_id: workspaceId, pane_id: paneId });

Comment on lines +79 to +81
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

track("terminal_opened") runs for every session creation, including potentially during recovery flows depending on how createSession/pendingSessions behave. If recovered sessions are common, this may overcount. Also, workspace_id comes from params; ensure it’s always present/meaningful for analytics (otherwise you'll get a lot of null/undefined).

Suggestion

Gate the event to truly-new sessions only (e.g., if (!session.wasRecovered) track(...)) or include was_recovered: session.wasRecovered so analysis can filter accurately.

Reply with "@CharlieHelps yes please" if you'd like me to add a commit that includes was_recovered (or guards tracking) here.

return {
isNew: true,
scrollback: session.scrollback,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,15 @@ import { trpc } from "renderer/lib/trpc";
import { posthog } from "../../lib/posthog";

export function PostHogUserIdentifier() {
const { data: user, isLoading } = trpc.user.me.useQuery();
const { data: user, isSuccess } = trpc.user.me.useQuery();

useEffect(() => {
if (isLoading) return;

if (user) {
posthog.identify(user.id, {
email: user.email,
name: user.name,
});
} else {
posthog.identify(user.id, { email: user.email, name: user.name });
} else if (isSuccess) {
posthog.reset();
}
}, [user, isLoading]);
}, [user, isSuccess]);

return null;
}
10 changes: 9 additions & 1 deletion apps/desktop/src/renderer/screens/sign-in/index.tsx
Original file line number Diff line number Diff line change
@@ -1,16 +1,24 @@
import { COMPANY } from "@superset/shared/constants";
import { Button } from "@superset/ui/button";
import { useEffect } from "react";
import { FaGithub } from "react-icons/fa";
import { FcGoogle } from "react-icons/fc";
import { posthog } from "renderer/lib/posthog";
import { trpc } from "renderer/lib/trpc";
import type { AuthProvider } from "shared/auth";
import { SupersetLogo } from "./components/SupersetLogo";

export function SignInScreen() {
const signInMutation = trpc.auth.signIn.useMutation();

const signIn = (provider: AuthProvider) =>
useEffect(() => {
posthog.capture("desktop_opened");
}, []);

const signIn = (provider: AuthProvider) => {
posthog.capture("auth_started", { provider });
signInMutation.mutate({ provider });
};

Comment on lines 11 to 22
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

desktop_opened is being captured in the Sign In screen mount effect. That likely undercounts launches where the user is already authenticated and never hits this screen, and over-couples an app-lifecycle event to a specific UI route.

Since you already introduced a main-process analytics module, app-open should be tracked from the main process (e.g., when app.whenReady() resolves), potentially with a separate renderer event for “sign_in_screen_viewed” if you want it.

Suggestion

Move desktop_opened to the main process (e.g., right after the app is ready / window created) using main/lib/analytics.track, and replace the renderer event with a screen-specific event if desired.

Example main process:

import { track } from './lib/analytics';
...
await app.whenReady();
track('desktop_opened');

Reply with "@CharlieHelps yes please" if you’d like me to add a commit moving the event and adjusting naming.

return (
<div className="flex flex-col h-full w-full bg-background">
Expand Down
13 changes: 13 additions & 0 deletions apps/desktop/test-setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,17 @@ mock.module("electron", () => ({
handle: mock(),
on: mock(),
},
shell: {
openExternal: mock(() => Promise.resolve()),
},
}));

// =============================================================================
// Analytics Mock (has Electron/API dependencies)
// =============================================================================

mock.module("main/lib/analytics", () => ({
track: mock(() => {}),
clearUserCache: mock(() => {}),
shutdown: mock(() => Promise.resolve()),
}));
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"use client";

import { COMPANY, DOWNLOAD_URL_MAC_ARM64 } from "@superset/shared/constants";
import posthog from "posthog-js";
import { HiMiniArrowDownTray, HiMiniClock } from "react-icons/hi2";
import { type DropdownSection, PlatformDropdown } from "../PlatformDropdown";

Expand All @@ -21,6 +22,7 @@ export function DownloadButton({
: "px-3 sm:px-6 py-2 sm:py-3 text-sm sm:text-base";

const handleAppleSiliconDownload = () => {
posthog.capture("download_clicked");
window.open(DOWNLOAD_URL_MAC_ARM64, "_blank");
};

Expand Down Expand Up @@ -76,7 +78,10 @@ export function DownloadButton({
id: "waitlist",
label: "Join waitlist for Windows & Linux",
icon: <HiMiniClock className="size-4" />,
onClick: onJoinWaitlist || (() => {}),
onClick: () => {
posthog.capture("waitlist_clicked");
onJoinWaitlist?.();
},
},
{
id: "build-from-source",
Expand Down
Loading