feat(signals): analytics dashboard facelift (BETON-350)#66
Conversation
…tion Implements the signal analytics facelift (BETON-350) covering: - Migration 025: signal_analytics_snapshots, signal_cohort_retention, signal_conversion_curves, posthog_property_mappings, attio_deal_mappings tables with RLS policies and triggers - Signal definitions: add conversion_type and retention_event_name columns - Analytics API: GET /api/signals/[id]/analytics with window/range filtering - PostHog property mappings API: GET/PUT /api/integrations/posthog/mappings - Attio deal mappings API: GET/PUT /api/integrations/attio/deal-mappings - Canvas chart components: RevenueChart, DualLineChart, AreaChart, RetentionTable - Signal detail page facelift: 4 KPI cards, 6 chart types, cohort retention heatmap, time-to-conversion curves, filter controls - Demo mode mock analytics data matching design sketch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Signal list page: enhanced stats cards with top signal, confidence count, and contextual sub-labels - Analytics computation engine: pure functions for computing monthly snapshots, cohort retention (M0-M8), and time-to-conversion curves - Chi-squared statistical significance testing with p-value computation - Supports all 5 conversion windows + no-limit mode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
25 tests covering: - Chi-squared test: empty/zero tables, significant/non-significant differences, symmetry, large samples - p-value to significance conversion: edge cases, clamping - Monthly snapshots: window filtering, occurrence counting, conversion rate computation, statistical significance - Cohort retention: all tab/stat combinations, user tracking - Conversion curves: period/cumulative consistency, normalization Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
nadyyym
left a comment
nadyyym
left a comment
There was a problem hiding this comment.
Code Review
CI Build Failure -- Root Cause
ESLint react-hooks/refs error in src/components/signals/charts/use-canvas.ts:23:
const drawRef = useRef(draw)
drawRef.current = draw // ERROR: Cannot assign to ref.current during render (React 19)Fix: Wrap in useEffect:
const drawRef = useRef(draw)
useEffect(() => { drawRef.current = draw }, [draw])This is the sole blocking error. The 123 other warnings are pre-existing.
Critical
None.
Important
I1. Math.random() in computeConversionCurves (compute.ts:354)
Non-signal user conversion uses Math.floor(Math.random() * periods), making analytics non-deterministic. Re-computation produces different results. Tests cannot assert exact values. Recommend a deterministic distribution (e.g., user ID hash).
I2. Delete-then-insert is not transactional (attio/deal-mappings/route.ts, posthog/mappings/route.ts PUT handlers)
Both do DELETE ... WHERE workspace_id then INSERT. If insert fails after delete, data is lost. Consider insert-first-then-delete, or document the risk.
I3. Signal ID parsed from URL path (signals/[id]/analytics/route.ts:28)
const pathParts = request.nextUrl.pathname.split('/')
const signalId = pathParts[pathParts.length - 2]Fragile -- should use Next.js route context.params.id instead.
I4. No input validation on signalId (same file)
signalId used directly in DB queries without UUID format validation. Add a Zod check to return a clean 400 on malformed input.
Minor
compute.ts:301-- unused variableconvmock-signal-analytics.ts:187-- unused variablecfg- Canvas tooltip colors hardcoded for dark theme (
rgba(0,0,0,0.85)) -- will not adapt to light mode availableWindowsquery lacksDISTINCT-- returns all rows just to extract unique values
Positive
- Excellent migration 025 -- full RLS on all 5 tables, composite indexes, sensible UNIQUE constraints, CASCADE tied to parent tables,
updated_attriggers - Clean computation engine -- pure functions, no side effects, well-defined types
- Correct chi-squared implementation -- proper Abramowitz and Stegun CDF, edge cases return
pValue: 1 - DPR-aware canvas charts --
devicePixelRatioscaling,ResizeObserver, proper cleanup on unmount - Solid test coverage -- 25 tests covering edge cases (zero division, symmetric chi-squared, empty data)
Verdict
Not mergeable until CI fix (one-line). After that, good for staging merge. Important issues can be addressed as follow-ups.
Migration note: Staging already has 025_data_sources.sql. This PR's 025_signal_analytics.sql must be renamed to 026_signal_analytics.sql before merge to avoid collision.
Review by Claude Code
…ooks/refs lint error
The useCanvas hook mutated drawRef.current = draw during render, which the
react-hooks/refs rule flags as an error ("Cannot update ref during render").
Refs must only be written outside render — moved the assignment into a
useEffect so it runs after commit. Behavior is unchanged: render() still reads
the latest draw via drawRef.current.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I think it looks quite good so far: 
|
1 participant
Summary
Implements the Signal Analytics Facelift epic (BETON-350) — a complete rebuild of the signal detail page with rich analytics and the foundation for pre-computed signal insights.
What changed
signal_analytics_snapshots,signal_cohort_retention,signal_conversion_curves,posthog_property_mappings,attio_deal_mappings) with full RLS policiesconversion_type(posthog_event/attio_deal_won/either) andretention_event_namecolumns/api/signals/[id]/analytics), PostHog property mappings, Attio deal mappingsTasks completed
Remaining tasks (follow-up PRs)
Test plan
npm run buildpassesvitest run src/lib/heuristics/analytics/)/signals/sig_001)🤖 Generated with Claude Code