feat(desktop): add PostHog feature flag to gate auth requirement#374
feat(desktop): add PostHog feature flag to gate auth requirement#374saddlepaddle merged 1 commit intomainfrom
Conversation
WalkthroughThe changes integrate PostHog analytics into the desktop app with feature-flag driven authentication gating. PostHog is configured in the Electron build, wrapped in a new context provider, and the main screen rendering is conditionally gated based on a feature flag while PostHog tracks flag loads. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Add posthog-js for feature flag support in desktop app - Create PostHogProvider to initialize PostHog in renderer - Add `require-desktop-auth` feature flag to control auth requirement - Update CSP to allow PostHog connections - Fix dev/prod singleton lock conflict with app.setName() - Centralize feature flags in shared package 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
saddlepaddle
force-pushed
the
sudden-stingray-9fd43f
branch
from
5076c19 to
fe92a62
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 Preview resources have been processed for cleanup |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/desktop/electron.vite.config.ts (1)
112-117: Consider renaming environment variables for clarity.The
NEXT_PUBLIC_prefix is a Next.js convention that may be confusing in an Electron context. While the implementation is correct, consider using a more Electron-idiomatic prefix likeVITE_orRENDERER_for renderer-exposed variables to improve clarity for future maintainers.Example:
"import.meta.env.VITE_POSTHOG_KEY": JSON.stringify( process.env.VITE_POSTHOG_KEY, ), "import.meta.env.VITE_POSTHOG_HOST": JSON.stringify( process.env.VITE_POSTHOG_HOST, ),apps/desktop/src/renderer/contexts/PostHogProvider.tsx (1)
20-43: Consider adding error handling for PostHog initialization.If
posthog.init()throws an error, the app will remain in an uninitialized state. Consider wrapping the initialization in a try-catch block to handle failures gracefully and still allow the app to function.Apply this diff:
useEffect(() => { if (!POSTHOG_KEY) { console.log("[posthog] No PostHog key configured, skipping init"); setIsInitialized(true); return; } + try { posthog.init(POSTHOG_KEY, { api_host: POSTHOG_HOST, // Electron apps don't have traditional page views capture_pageview: false, // Disable session recording for now disable_session_recording: true, // Persist across sessions persistence: "localStorage", // Load feature flags on init bootstrap: { featureFlags: {}, }, }); setIsInitialized(true); console.log("[posthog] Initialized"); + } catch (error) { + console.error("[posthog] Initialization failed:", error); + setIsInitialized(true); // Allow app to continue without PostHog + } }, []);apps/desktop/src/renderer/screens/main/index.tsx (1)
180-189: Add loading indicator for better UX.While waiting for feature flags to load, the app shows a blank screen with no indication to the user that something is happening. This creates a poor user experience, especially if PostHog is slow to respond.
Apply this diff to show a loading spinner consistent with other loading states:
// Show empty screen while feature flags are loading if (!flagsLoaded) { return ( <> <Background /> <AppFrame> - <div className="h-full w-full bg-background" /> + <div className="flex h-full w-full items-center justify-center bg-background"> + <LoadingSpinner /> + </div> </AppFrame> </> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
apps/desktop/electron.vite.config.ts(1 hunks)apps/desktop/package.json(1 hunks)apps/desktop/src/main/index.ts(1 hunks)apps/desktop/src/renderer/contexts/AppProviders.tsx(2 hunks)apps/desktop/src/renderer/contexts/PostHogProvider.tsx(1 hunks)apps/desktop/src/renderer/contexts/index.ts(1 hunks)apps/desktop/src/renderer/index.html(1 hunks)apps/desktop/src/renderer/screens/main/index.tsx(4 hunks)packages/shared/src/constants.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/main/index.tsapps/desktop/src/renderer/contexts/AppProviders.tsxapps/desktop/electron.vite.config.tsapps/desktop/src/renderer/contexts/PostHogProvider.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/contexts/index.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/main/index.tsapps/desktop/src/renderer/contexts/AppProviders.tsxapps/desktop/electron.vite.config.tsapps/desktop/src/renderer/contexts/PostHogProvider.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/contexts/index.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/main/index.tsapps/desktop/src/renderer/contexts/AppProviders.tsxapps/desktop/electron.vite.config.tsapps/desktop/src/renderer/contexts/PostHogProvider.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/package.jsonapps/desktop/src/renderer/contexts/index.tspackages/shared/src/constants.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/main/index.tsapps/desktop/src/renderer/contexts/AppProviders.tsxapps/desktop/electron.vite.config.tsapps/desktop/src/renderer/contexts/PostHogProvider.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/contexts/index.tspackages/shared/src/constants.ts
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables in apps/desktop/src/main/index.ts with override: true before any imports
Files:
apps/desktop/src/main/index.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods from renderer process using window.ipcRenderer.invoke with type-safe object parameters
Files:
apps/desktop/src/renderer/contexts/AppProviders.tsxapps/desktop/src/renderer/contexts/PostHogProvider.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/contexts/index.ts
🧠 Learnings (3)
📚 Learning: 2025-12-12T05:45:09.686Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.686Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables in apps/desktop/src/main/index.ts with override: true before any imports
Applied to files:
apps/desktop/src/main/index.tsapps/desktop/electron.vite.config.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/main/index.ts
📚 Learning: 2025-12-12T05:45:09.686Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.686Z
Learning: Applies to packages/ui/**/*.{ts,tsx} : Use React + TailwindCSS v4 + shadcn/ui for UI components in `packages/ui`
Applied to files:
apps/desktop/package.json
🧬 Code graph analysis (2)
apps/desktop/src/renderer/contexts/PostHogProvider.tsx (1)
apps/desktop/src/renderer/contexts/index.ts (1)
PostHogProvider(3-3)
apps/desktop/src/renderer/screens/main/index.tsx (3)
packages/shared/src/constants.ts (1)
FEATURE_FLAGS(36-39)apps/desktop/src/renderer/screens/main/components/Background.tsx (1)
Background(1-39)apps/desktop/src/renderer/screens/main/components/AppFrame.tsx (1)
AppFrame(5-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Web
- GitHub Check: Deploy API
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Build
🔇 Additional comments (12)
apps/desktop/src/main/index.ts (1)
13-16: LGTM! Singleton lock conflict resolved.Setting a distinct app name for development mode is the correct approach to allow dev and prod instances to run simultaneously. The placement before protocol handler registration is appropriate.
apps/desktop/src/renderer/index.html (1)
12-18: LGTM! CSP correctly configured for PostHog.The wildcard subdomain
https://*.posthog.comis necessary to support PostHog's multi-region architecture (e.g.,us.i.posthog.com,eu.posthog.com). The directives correctly allow both script loading and API connections.packages/shared/src/constants.ts (1)
35-39: LGTM! Feature flag constant well-structured.The use of
as constensures type safety, the naming convention is clear and consistent, and the placement in the shared package makes it accessible across the monorepo.apps/desktop/src/renderer/contexts/AppProviders.tsx (1)
3-3: LGTM! Provider hierarchy correctly structured.Placing
PostHogProvideras the outermost wrapper is appropriate for analytics/feature-flag infrastructure that should be available to all child providers and components.Also applies to: 12-16
apps/desktop/src/renderer/contexts/index.ts (1)
3-3: LGTM! Export follows barrel pattern.The export is consistent with the existing pattern in this file.
apps/desktop/src/renderer/contexts/PostHogProvider.tsx (1)
27-39: PostHog configuration looks appropriate for Electron.The initialization settings are well-suited for an Electron desktop app: pageview capture disabled, localStorage persistence, and session recording disabled. The empty
featureFlagsbootstrap is correct—flags will be fetched asynchronously after initialization.apps/desktop/package.json (1)
81-81: posthog-js@1.306.1 is a valid, secure version with no known vulnerabilities.The package version exists and is safe to use. While posthog-js had a historical XSS vulnerability (CVE-2023-32325) fixed in v1.57.2, and a supply-chain compromise affected v1.297.3 in November 2025, version 1.306.1 is unaffected by both issues and shows no current security advisories.
apps/desktop/src/renderer/screens/main/index.tsx (5)
1-4: LGTM: Imports are correct.The imports for PostHog hooks and feature flags are appropriate for this implementation.
35-42: LGTM: Feature flag hooks are properly initialized.The PostHog instance and feature flag hooks are set up correctly. The
flagsLoadedstate appropriately tracks the async loading of feature flags.
176-177: Verify fail-open behavior is intentional.The condition
shouldRequireAuth = flagsLoaded && requireAuth === trueimplements fail-open security: if PostHog fails to load or the feature flag is unavailable, authentication is not required.For a desktop application, this may be acceptable, but please confirm this is the intended behavior. If the app should require authentication by default when feature flags are unavailable (fail-closed), the logic should be inverted.
Consider documenting this behavior with a comment:
// Wait for feature flags to load before deciding on auth +// Note: Fails open - if flags don't load, auth is NOT required const shouldRequireAuth = flagsLoaded && requireAuth === true;
192-215: LGTM: Conditional auth checks are correctly implemented.The auth loading and sign-in checks are now properly gated behind the
shouldRequireAuthcondition. When the feature flag is disabled, users can access the app without authentication.
45-51: Simplify flag loading: use PostHog hooks instead of manual state tracking.The current approach has a design flaw. The code mixes the PostHog callback pattern (
onFeatureFlags) with the React hook pattern (useFeatureFlagEnabled), creating unnecessary complexity.With the current empty bootstrap configuration,
onFeatureFlags()fires immediately (before flags are fetched from the server), causing the blank screen to clear whileuseFeatureFlagEnabledstill returnsundefined. This means the app proceeds before the actual feature flag is resolved.Recommended approach:
- Remove the manual
flagsLoadedstate andonFeatureFlagscallback- Handle the hook's initial
undefinedstate directly in the component logic- Alternatively, bootstrap with real flags from your backend instead of an empty object
The timeout mechanism in the proposed diff is unnecessary since
onFeatureFlagsis guaranteed to fire (even with empty bootstrap), and checkingposthog.isFeatureEnabled !== undefineddoesn't indicate flag load status—it only checks method existence.
| if (!isInitialized) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Brief render blocking during initialization.
Returning null while PostHog initializes will briefly block all content from rendering. While PostHog initialization is typically fast, consider showing a loading state or allowing children to render immediately to avoid any perceived delay, especially on slower connections.
Apply this diff to render children immediately:
- // Don't render children until PostHog is initialized (or skipped)
- if (!isInitialized) {
- return null;
- }
-
// If no PostHog key, just render children without the provider
- if (!POSTHOG_KEY) {
+ if (!isInitialized || !POSTHOG_KEY) {
return <>{children}</>;
}📝 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.
| if (!isInitialized) { | |
| return null; | |
| } | |
| // If no PostHog key, just render children without the provider | |
| if (!isInitialized || !POSTHOG_KEY) { | |
| return <>{children}</>; | |
| } | |
| return <PHProvider client={posthogClient}>{children}</PHProvider>; |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/contexts/PostHogProvider.tsx around lines 46-48,
the provider currently returns null while PostHog initializes which blocks
rendering; change the component to render its children immediately (or render a
lightweight loading UI) instead of returning null, and move/guard any
PostHog-dependent logic so initialization continues in the background (e.g.,
kick off init in useEffect and only gate calls that require
posthog.isInitialized rather than the whole render).
1 participant
Summary
require-desktop-authflag gates whether users must sign in to use the appChanges
posthog-jsand createPostHogProviderfor feature flag supporthttps://*.posthog.com)app.setName("Superset Dev")in dev mode to prevent singleton lock conflictspackages/shared/src/constants.tsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.