Simplify PostHog metrics UI with dialog-based workflow #77
Conversation
…background prefetching - Replace multi-step form with simple dialog containing metric name, project, and event dropdowns - Remove Active Users template, focusing only on Event Count (Time Series) metrics - Add background prefetching of all projects and events on component mount - Fix loading state issues by using separate mutation instances for concurrent API calls - Improve UX with cached data ready before user opens dialog 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThree changes: API test runner now captures, parses, logs, and displays structured error details; PostHog metrics UI refactored from template-driven to a dialog with project/event prefetching; and the "posthog-active-users" metric template was removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IT as IntegrationTester
participant API as Remote API
participant UI as JsonViewer
User->>IT: Run API Test
activate IT
IT->>API: Send request
activate API
API-->>IT: non-OK response (body with error)
deactivate API
IT->>IT: Parse response -> errorMessage, errorDetails
IT-->>UI: Render structured error block (collapsible)
deactivate IT
sequenceDiagram
participant User
participant Page as PostHog Page
participant API as Backend
User->>Page: Open Create Metric Dialog
activate Page
Page->>API: Fetch Projects
API-->>Page: Projects[]
Page->>API: Fetch Events for selected project
API-->>Page: Events[]
User->>Page: Select Project & Event, Submit
Page->>API: Create Metric (templateId: posthog-event-count)
API-->>Page: Success
Page->>User: Close Dialog, Clear State
deactivate Page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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)
src/app/api-test/_components/integration-tester.tsx (2)
53-61: TestResult extension is fine; consider tighteningerrorDetailslaterAdding
errorDetailstoTestResultis a good hook for richer debugging. Once the error payload format stabilizes, consider replacinganywith a small dedicated error-payload type so downstream consumers know what to expect.
157-171: Structured error payload is parsed then discarded;errorDetailswill usually be emptyRight now the flow is:
- On non-OK responses you parse
errorJson, deriveerrorMessage/errorCause, log everything, thenthrow new Error(errorMessage).- In the
catch, you try to recovererrorDetailsby regex-parsing JSON out oferror.message.This means:
- The rich
errorJsonyou already have for non-OK responses is never surfaced to the UI; you only expose a flat message.- The regex
/\{.*\}/sis greedy and brittle; if the message format changes,JSON.parsewill start throwing again (silently swallowed).You can simplify and make errorDetails reliable by directly attaching the parsed payload when you throw, e.g.:
- if (!response.ok) { - // Parse error response to get detailed error info - const errorJson = await response.json().catch(() => null); - const errorMessage = errorJson?.error?.message ?? response.statusText; - const errorCause = errorJson?.error?.cause; - - console.error(`[API Test Error] ${endpoint.label}:`, { - status: response.status, - statusText: response.statusText, - errorMessage, - errorCause, - fullError: errorJson, - }); - - throw new Error(errorMessage); - } + if (!response.ok) { + // Parse error response to get detailed error info + const errorJson = await response.json().catch(() => null); + const errorMessage = + errorJson?.error?.message ?? + `${response.status} ${response.statusText || "Request failed"}`; + + console.error(`[API Test Error] ${endpoint.label}:`, { + status: response.status, + statusText: response.statusText, + errorMessage, + fullError: errorJson, + }); + + // Encode full error JSON into the message so the catch block + // can reliably parse it back out into `errorDetails`. + throw new Error( + errorJson ? `${errorMessage} ${JSON.stringify(errorJson)}` : errorMessage, + ); + }If you keep the regex-based parsing, also consider making it non-greedy to reduce the chance of malformed slices:
- const match = /\{.*\}/s.exec(error.message); + const match = /\{.*?\}/s.exec(error.message);This will ensure
errorDetailsis consistently populated for backend error responses instead of stayingnullmost of the time.Also applies to: 184-211
src/app/metric/posthog/page.tsx (1)
213-310: Dialog flow is solid; consider refining event loading UXThe dialog-based flow (metric name → project → event with a single “Create Metric” action) is clean and much easier to understand than a multi-step form. A couple of small UX refinements you might consider:
The event placeholder currently treats “no events yet” and “still loading/failed to load” the same:
!selectedProject ? "Select project first" : selectedProjectEvents.length === 0 ? "Loading events..." : "Select an event"If a project legitimately has zero events or its fetch failed, this will show “Loading events...” indefinitely. You could instead distinguish
"no events found"vs."loading"by tracking a per-project loading/error flag.When the dialog closes (via overlay/ESC), the previous selections remain. That may be desired, but if you want a fresh form on reopen you could reset
metricName,selectedProject, andselectedEventin theonOpenChangehandler whenopenbecomesfalse.These are polish-level, not blockers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/api-test/_components/integration-tester.tsx(4 hunks)src/app/metric/posthog/page.tsx(5 hunks)src/lib/integrations/posthog.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/lib/integrations/posthog.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/app/api-test/_components/integration-tester.tsx (1)
src/components/json-viewer/index.tsx (1)
JsonViewer(21-191)
src/app/metric/posthog/page.tsx (6)
src/trpc/react.tsx (1)
api(28-28)src/components/ui/dialog.tsx (7)
Dialog(135-135)DialogTrigger(144-144)DialogContent(137-137)DialogHeader(140-140)DialogTitle(143-143)DialogDescription(138-138)DialogFooter(139-139)src/components/ui/button.tsx (1)
Button(62-62)src/components/ui/label.tsx (1)
Label(25-25)src/components/ui/input.tsx (1)
Input(21-21)src/components/ui/select.tsx (5)
Select(178-178)SelectTrigger(186-186)SelectValue(187-187)SelectContent(179-179)SelectItem(181-181)
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (4)
src/app/api-test/_components/integration-tester.tsx (1)
495-510: Error details UI is a solid debugging aidThe structured error block with a prominent
Error:line andJsonViewerforerrorDetailsis a nice UX improvement for diagnosing failing endpoints. Guarding the viewer behindresult.errorDetailskeeps the UI clean when only a message is available.src/app/metric/posthog/page.tsx (3)
31-67: Transform helpers are clear and defensive
transformProjects/transformEventsare straightforward, tolerate unexpected shapes by returning[], and keep the UI layer decoupled from the raw PostHog API shape. This is a good, low-ceremony boundary.
73-83: State layout for dialog + prefetched data looks saneSeparating
projects,eventsByProject, and selection state (selectedProject/selectedEvent) makes the metric flow easy to follow, and using distinct mutations for projects vs. events avoids loading-state clashes between them.Also applies to: 85-92
172-186: Metric creation handler is straightforward; verify backend param names
handleCreate’s guards plus thecreateMetricmutation usage look correct, and clearing dialog state on success via the mutation’sonSuccessis a nice touch. Just ensure the backend template for"posthog-event-count"expectsendpointParamskeyed asPROJECT_IDandEVENT_NAME; otherwise this will fail at runtime.
- Remove background prefetching that caused loading state - Button is now always enabled and ready to click - Projects load when dialog opens - Events load when project is selected - Auto-generate metric name from project and event selection - Remove manual metric name input field for simpler UX 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/app/metric/posthog/page.tsx (2)
27-65: Tighten PostHog response typing instead ofunknown+ cast
transformProjects/transformEventscurrently acceptunknownand then cast to a loose{ results?: ... }shape. This works but gives up type safety and makes refactors harder.Consider introducing explicit response types and narrowing the function signatures, e.g.:
-type Project = { label: string; value: string }; -type Event = { label: string; value: string }; +type Project = { label: string; value: string }; +type MetricEvent = { label: string; value: string }; + +type PosthogProjectsResponse = { + results?: Array<{ name: string; id: number }>; +}; + +type PosthogEventsResponse = { + results?: Array<{ name: string }>; +}; -function transformProjects(data: unknown): Project[] { - if (!data || typeof data !== "object") return []; +function transformProjects( + data: PosthogProjectsResponse | null | undefined, +): Project[] { + if (!data) return []; @@ -function transformEvents(data: unknown): Event[] { - if (!data || typeof data !== "object") return []; +function transformEvents( + data: PosthogEventsResponse | null | undefined, +): MetricEvent[] { + if (!data) return [];This keeps the UI-facing options simple while giving you compile‑time protection if the PostHog payload shape changes. Also avoids shadowing the global
Eventtype, which can be mildly confusing in TS code.
99-108: Create metric flow is solid; consider preserving context after successThe mutation wiring and button disabled state look good: you gate
handleCreateon required selections, disable the button while pending, and surface the error message.On success you currently:
- Close the dialog.
- Clear
selectedProject,selectedEvent,projects, andevents.This is safe but slightly unfriendly for workflows where a user wants to create several related metrics in a row, as they must re-select project and wait for projects/events to refetch each time.
If repeat creation is a common case, consider only clearing
selectedEventand leavingprojects/selectedProjectintact:const createMetric = api.metric.create.useMutation({ onSuccess: () => { void utils.metric.getAll.invalidate(); setOpen(false); - setSelectedProject(""); - setSelectedEvent(""); - setProjects([]); - setEvents([]); + setSelectedEvent(""); }, });You’d still get fresh data when the connection changes (via the effects), but users won’t pay the cost on every single successful create.
Also applies to: 140-157, 249-258, 260-264
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/metric/posthog/page.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/metric/posthog/page.tsx (6)
src/trpc/react.tsx (1)
api(28-28)src/components/ui/card.tsx (3)
CardTitle(93-93)CardHeader(91-91)CardContent(96-96)src/components/ui/dialog.tsx (7)
Dialog(135-135)DialogTrigger(144-144)DialogContent(137-137)DialogHeader(140-140)DialogTitle(143-143)DialogDescription(138-138)DialogFooter(139-139)src/components/ui/button.tsx (1)
Button(62-62)src/components/ui/label.tsx (1)
Label(25-25)src/components/ui/select.tsx (5)
Select(178-178)SelectTrigger(186-186)SelectValue(187-187)SelectContent(179-179)SelectItem(181-181)
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (1)
src/app/metric/posthog/page.tsx (1)
71-78: Dialog-based UX and fetch-on-open behavior look good; confirm intent vs. “background prefetch”The new dialog flow and wiring between
open, the project fetch effect, and the selects are coherent:
- Projects are fetched lazily when the dialog first opens and
projects.length === 0.- Events are fetched on project selection.
- Select placeholders correctly reflect loading states and disabled conditions.
- Errors from
createMetricare surfaced inline.One thing to double-check against the PR description: the code now fetches projects only once the dialog is opened, rather than “background prefetch on mount”. That’s a perfectly reasonable choice (and avoids the earlier infinite-retry issue), but it does slightly change the UX expectations.
If “ready before the user opens the dialog” is still a requirement, you could trigger the initial
fetchProjects.mutateoffconnectionalone (once per connection), and keep the dialog open state purely for UI. Otherwise, the current on‑open fetch behavior is consistent and easy to reason about.Also applies to: 110-123, 177-267
| const integrationQuery = api.integration.listWithStats.useQuery(); | ||
| const connection = integrationQuery.data?.active.find( | ||
| (int) => int.integrationId === "posthog", | ||
| ); | ||
|
|
There was a problem hiding this comment.
Differentiate “loading connection” from “no PostHog connection”
Right now connection is derived from integrationQuery.data, and the early return:
if (!connection) {
return <Card>…No PostHog Connection…</Card>;
}will also render while the query is still loading or if it errors for other reasons. That can briefly (or permanently, on error) mislead users into thinking there is no connection.
A small refactor to branch on loading and error state separately would improve UX:
- const integrationQuery = api.integration.listWithStats.useQuery();
+ const integrationQuery = api.integration.listWithStats.useQuery();
const connection = integrationQuery.data?.active.find(
(int) => int.integrationId === "posthog",
);
@@
- if (!connection) {
+ if (integrationQuery.isLoading) {
+ return (
+ <Card>
+ <CardHeader>
+ <CardTitle>Loading PostHog Connection…</CardTitle>
+ </CardHeader>
+ <CardContent>
+ <p className="text-muted-foreground text-sm">
+ Fetching your PostHog integrations.
+ </p>
+ </CardContent>
+ </Card>
+ );
+ }
+
+ if (!connection) {
return (
<Card>
<CardHeader>
<CardTitle>No PostHog Connection</CardTitle>Optionally you could also surface integrationQuery.isError with a more precise error message.
Also applies to: 159-172
🤖 Prompt for AI Agents
In src/app/metric/posthog/page.tsx around lines 80-84 (and similarly 159-172),
the code derives connection directly from integrationQuery.data and immediately
returns a "No PostHog Connection" Card which also shows during loading or on
error; change the control flow to first check integrationQuery.isLoading and
render a loading state, then check integrationQuery.isError and render an error
message (optionally include error details), and only after those checks derive
connection from integrationQuery.data and render the "No PostHog Connection"
Card when connection is definitively absent; this ensures loading and error
states are distinct from a true "no connection" result.
| const fetchEvents = api.metric.fetchIntegrationData.useMutation({ | ||
| onSuccess: (data: { data: unknown }) => { | ||
| const options = transformEvents(data.data); | ||
| setEventOptions(options); | ||
| onSuccess: (data) => { | ||
| const eventList = transformEvents(data.data); | ||
| setEvents(eventList); | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Prevent stale event options when switching projects quickly
The events mutation has a global onSuccess that unconditionally updates events:
const fetchEvents = api.metric.fetchIntegrationData.useMutation({
onSuccess: (data) => {
const eventList = transformEvents(data.data);
setEvents(eventList);
},
});and the effect fires mutate whenever selectedProject changes. If a user switches projects quickly (or network responses arrive out of order), a slower response for project A can overwrite the events for project B, leaving the dropdown desynchronized from selectedProject.
You can guard against this by tying the success handler to the project that initiated the request:
-const fetchEvents = api.metric.fetchIntegrationData.useMutation({
- onSuccess: (data) => {
- const eventList = transformEvents(data.data);
- setEvents(eventList);
- },
-});
+const fetchEvents = api.metric.fetchIntegrationData.useMutation();
@@
- useEffect(() => {
- if (selectedProject && connection) {
- setEvents([]);
- setSelectedEvent("");
- fetchEvents.mutate({
- connectionId: connection.connectionId,
- integrationId: "posthog",
- endpoint: `/api/projects/${selectedProject}/event_definitions/`,
- method: "GET",
- params: {},
- });
- }
+ useEffect(() => {
+ if (!selectedProject || !connection) return;
+
+ const projectId = selectedProject;
+ setEvents([]);
+ setSelectedEvent("");
+
+ fetchEvents.mutate(
+ {
+ connectionId: connection.connectionId,
+ integrationId: "posthog",
+ endpoint: `/api/projects/${projectId}/event_definitions/`,
+ method: "GET",
+ params: {},
+ },
+ {
+ onSuccess: (data) => {
+ // Ignore if the user switched projects while this request was in flight.
+ if (projectId !== selectedProject) return;
+ setEvents(transformEvents(data.data));
+ },
+ },
+ );
}, [selectedProject, connection]);This ensures the UI always shows events for the currently selected project, even under slow or flaky networks.
Also applies to: 124-138
🤖 Prompt for AI Agents
In src/app/metric/posthog/page.tsx around lines 92-98 and also 124-138, the
global onSuccess for fetchEvents unconditionally calls setEvents which allows
out-of-order network responses to overwrite the events for the
currently-selected project; fix by associating each mutate call with the project
id (pass selectedProject id as the mutation variable or a context value) and in
the onSuccess handler compare that initiating project id to the current
selectedProject before calling setEvents (if they differ, ignore the response),
ensuring only responses for the active project update the dropdown.
Summary
Changes
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.