Conversation
|
🚅 Environment inspector-pr-1658 in triumphant-alignment has no services deployed. 1 service not affected by this PR
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughThis pull request enhances PostHog telemetry tracking by introducing version tracking and improving state management during authentication transitions. The changes add a 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/hooks/__tests__/usePostHogIdentify.test.ts (1)
5-39: Prefer repository mock presets over ad-hoc client mocks.This suite hand-rolls its mock state; please align with
client/src/test/mocks/presets for consistency with the rest of client tests.As per coding guidelines: "mcpjam-inspector/client/**/tests/*.test.{ts,tsx}: Use mock presets from
client/src/test/mocks/includingmcpApiPresetsandstorePresetsin client tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/__tests__/usePostHogIdentify.test.ts` around lines 5 - 39, Replace the ad-hoc mockState and individual vi.mock calls in this test with the shared test presets: import and use the repository mock presets (e.g., mcpApiPresets and storePresets) from client/src/test/mocks and wire them into the test instead of creating mockState; specifically ensure usePostHog, useAuth, useConvexAuth and detectPlatform are provided by those presets (or adapted from them) so identify/register/reset mocks and platform detection come from the canonical mocks rather than the hand-rolled mockState and vi.mock declarations.mcpjam-inspector/client/src/hooks/usePostHogIdentify.ts (1)
39-44: Deduplicate static telemetry registration to avoid config drift.Lines 39–44 duplicate the same static props defined in
PosthogUtils.options.loaded. A shared helper keeps telemetry shape consistent across login/logout paths.♻️ Suggested refactor
- posthog.register({ - environment: import.meta.env.MODE, - platform: detectPlatform(), - version: __APP_VERSION__, - }); + posthog.register(getStaticTelemetryProps());// in mcpjam-inspector/client/src/lib/PosthogUtils.ts +export function getStaticTelemetryProps() { + return { + environment: import.meta.env.MODE, + platform: detectPlatform(), + version: __APP_VERSION__, + }; +}- posthog.register({ - environment: import.meta.env.MODE, // "development" or "production" - platform: detectPlatform(), - version: __APP_VERSION__, - }); + posthog.register(getStaticTelemetryProps());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/usePostHogIdentify.ts` around lines 39 - 44, The posthog.register call in usePostHogIdentify.ts re-registers the same static telemetry props already defined in PosthogUtils.options.loaded, causing duplication and potential config drift; replace the inline object with a single shared helper or the existing PosthogUtils.options.loaded (e.g., call posthog.register(PosthogUtils.options.loaded) or extract a getStaticTelemetryProps() used by both login/logout paths) so environment/platform/version are defined in one place and reused across reset/login/logout flows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcpjam-inspector/client/src/hooks/__tests__/usePostHogIdentify.test.ts`:
- Around line 5-39: Replace the ad-hoc mockState and individual vi.mock calls in
this test with the shared test presets: import and use the repository mock
presets (e.g., mcpApiPresets and storePresets) from client/src/test/mocks and
wire them into the test instead of creating mockState; specifically ensure
usePostHog, useAuth, useConvexAuth and detectPlatform are provided by those
presets (or adapted from them) so identify/register/reset mocks and platform
detection come from the canonical mocks rather than the hand-rolled mockState
and vi.mock declarations.
In `@mcpjam-inspector/client/src/hooks/usePostHogIdentify.ts`:
- Around line 39-44: The posthog.register call in usePostHogIdentify.ts
re-registers the same static telemetry props already defined in
PosthogUtils.options.loaded, causing duplication and potential config drift;
replace the inline object with a single shared helper or the existing
PosthogUtils.options.loaded (e.g., call
posthog.register(PosthogUtils.options.loaded) or extract a
getStaticTelemetryProps() used by both login/logout paths) so
environment/platform/version are defined in one place and reused across
reset/login/logout flows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 662c2ec0-24c0-47ea-b553-92f2872f2c41
📒 Files selected for processing (4)
mcpjam-inspector/client/src/hooks/__tests__/usePostHogIdentify.test.tsmcpjam-inspector/client/src/hooks/usePostHogIdentify.tsmcpjam-inspector/client/src/lib/PosthogUtils.tsmcpjam-inspector/client/src/lib/__tests__/posthog-utils.test.ts
No description provided.