ReviewOS
Reload

also looking at this

stacks/analyticshq

Harden AnalyticsHQ release and dashboard quality

#52
Merged glennmichael123 wants to merge docs/bunpress-product-guide into main
34 files +1236 -340

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

app/Analytics/event-properties.tsadded+46-0
@@ -0,0 +1,46 @@
1/**
2 * Browser keepalive requests have a small total-body budget. Keeping event
3 * properties below 48 KiB leaves room for the event envelope while allowing
4 * long URLs and useful structured metadata.
5 */
6export const MAX_EVENT_PROPERTIES_BYTES = 48 * 1024
7
8const URL_EVENTS = new Set(['Outbound Link', 'File Download'])
9
10function utf8Bytes(value: string): number {
11 return new TextEncoder().encode(value).byteLength
12}
13
14/**
15 * Serialize public event properties without allowing an unbounded database
16 * value. Reserved URL events keep one canonical key so grouping is stable.
17 *
18 * Invalid or oversized properties are omitted, but the event itself can still
19 * be recorded. The public collector must not become a 500 because optional
20 * metadata was unusable.
21 */
22export function serializeEventProperties(event: unknown, properties: unknown): string | null {
23 if (properties === null || properties === undefined)
24 return null
25
26 let normalized = properties
27 if (
28 URL_EVENTS.has(String(event))
29 && typeof properties === 'object'
30 && 'url' in properties
31 && properties.url
32 ) {
33 normalized = { url: String(properties.url) }
34 }
35
36 try {
37 const serialized = JSON.stringify(normalized)
38 if (!serialized || utf8Bytes(serialized) > MAX_EVENT_PROPERTIES_BYTES)
39 return null
40
41 return serialized
42 }
43 catch {
44 return null
45 }
46}
database/migrations/0000000052-widen-custom-event-properties.sqladded+5-0
@@ -0,0 +1,5 @@
1-- Production analytics storage is PostgreSQL. SQLite already gives varchar
2-- columns unconstrained text affinity, so it needs no table rebuild.
3ALTER TABLE "custom_events"
4 ALTER COLUMN "properties" TYPE text
5 USING "properties"::text;
routes/analytics.tsmodified+7-19
@@ -20,6 +20,7 @@ import { formatCount, renderBadge, renderSparkline, sanitizeLabel } from '../app
2020import { ASSIGNABLE_ROLES, isAssignableRole, listSiteMembers, resolveSiteRole, satisfies, siteExists, type SiteRole } from '../app/Analytics/access'
2121import { ALERT_CONDITIONS, ALERT_METRICS, isAlertCondition, isAlertMetric, isRelative } from '../app/Analytics/alerts'
2222import { planForSite } from '../app/Analytics/entitlements'
23import { serializeEventProperties } from '../app/Analytics/event-properties'
2324import { expiryFrom, hashToken, inviteRefusal, looksLikeEmail, mintToken, normalizeEmail } from '../app/Analytics/invites'
2425import { sendSiteInvite } from '../app/Mail/SiteInvite'
2526import { featureUnavailableMessage, isUnlimited, limitReachedMessage, type PlanFeatures, type PlanLimits } from '../config/plans'
@@ -551,25 +552,12 @@ route.post('/collect', async (request: any) => {
551552 }).execute()
552553 }
553554 else {
554 // Reserved auto-tracked events (Outbound Link / File Download) carry only a url. Store
555 // it canonically as {"url":...} regardless of any extra keys / key-order a caller sends,
556 // so the dashboard's GROUP BY properties aggregates exactly one row per URL — a client
557 // can't split or pollute a URL's row by appending junk keys.
558 let props = body.p ? JSON.stringify(body.p) : null
559 if ((String(event) === 'Outbound Link' || String(event) === 'File Download') && body.p && body.p.url) {
560 let url = String(body.p.url)
561 props = JSON.stringify({ url })
562 // properties is varchar(255): trim the url until the wrapped JSON fits, so it stays
563 // valid JSON (the dashboard JSON.parses it) and never overflows the column — on Postgres
564 // an over-length varchar insert errors (22001) and would 500 the beacon. Only pathologically
565 // long hrefs hit the loop. (TODO: widen custom_events.properties for full-length urls.)
566 while (props.length > 255 && url.length) {
567 url = url.slice(0, -8)
568 props = JSON.stringify({ url })
569 }
570 }
571 // .catch like the sites/sessions inserts above: a storage failure (e.g. an over-length
572 // non-reserved props blob under strict sql_mode) must never 500 the public beacon.
555 // Reserved auto-tracked events carry only their canonical URL, so grouping
556 // stays stable even when a caller sends extra keys in a different order.
557 // Optional metadata is bounded before it reaches the database.
558 const props = serializeEventProperties(event, body.p)
559 // .catch like the sites/sessions inserts above: a storage failure must
560 // never turn the public beacon into a 500 response.
573561 await db.insertInto('custom_events').values({
574562 id: randomId(),
575563 site_id: String(siteId),
tests/unit/event-properties.test.tsadded+67-0
@@ -0,0 +1,67 @@
1import { describe, expect, test } from 'bun:test'
2import { readFileSync } from 'node:fs'
3import { join } from 'node:path'
4import { MAX_EVENT_PROPERTIES_BYTES, serializeEventProperties } from '../../app/Analytics/event-properties'
5
6describe('event property serialization', () => {
7 test('preserves structured metadata well beyond the old 255-character column', () => {
8 const properties = {
9 campaign: 'spring-launch',
10 notes: 'measured-context-'.repeat(700),
11 }
12 const serialized = serializeEventProperties('Signup', properties)
13
14 expect(serialized).toBe(JSON.stringify(properties))
15 expect(serialized!.length).toBeGreaterThan(10_000)
16 })
17
18 test('keeps the complete URL for reserved automatic events', () => {
19 const url = `https://example.com/${'deep/path/'.repeat(900)}?source=campaign`
20 const serialized = serializeEventProperties('Outbound Link', {
21 ignored: 'does not split the aggregate',
22 url,
23 })
24
25 expect(serialized).toBe(JSON.stringify({ url }))
26 expect(JSON.parse(serialized!).url).toBe(url)
27 })
28
29 test('canonicalizes file downloads the same way', () => {
30 expect(serializeEventProperties('File Download', {
31 url: 'https://example.com/report.csv',
32 arbitrary: true,
33 })).toBe('{"url":"https://example.com/report.csv"}')
34 })
35
36 test('omits optional properties that exceed the UTF-8 byte budget', () => {
37 const properties = { notes: 'é'.repeat(MAX_EVENT_PROPERTIES_BYTES) }
38
39 expect(serializeEventProperties('Oversized', properties)).toBeNull()
40 })
41
42 test('omits values JSON cannot serialize', () => {
43 expect(serializeEventProperties('Invalid', 1n)).toBeNull()
44 })
45})
46
47describe('event property storage', () => {
48 const root = join(import.meta.dir, '..', '..')
49
50 test('upgrades existing PostgreSQL columns to text', () => {
51 const migration = readFileSync(
52 join(root, 'database/migrations/0000000052-widen-custom-event-properties.sql'),
53 'utf8',
54 )
55
56 expect(migration).toContain('ALTER COLUMN "properties" TYPE text')
57 expect(migration).toContain('USING "properties"::text')
58 })
59
60 test('the collector uses the bounded serializer without URL clipping', () => {
61 const source = readFileSync(join(root, 'routes/analytics.ts'), 'utf8')
62
63 expect(source).toContain('serializeEventProperties(event, body.p)')
64 expect(source).not.toContain('while (props.length > 255')
65 expect(source).not.toContain('widen custom_events.properties')
66 })
67})