feat(telemetry): add messageId and nudgeSentAtMs to nudge event for TLON-5728#155
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review once |
There was a problem hiding this comment.
LGTM — clean additive telemetry change with strong test coverage (cross-repo fixture pinning, null-on-failure assertions) and end-to-end verification against staging.
Extended reasoning...
Overview
This PR adds two fields (messageId, nudgeSentAtMs) to the existing TlonBot Heartbeat Nudge Sent PostHog event so it can be joined to Homestead's Tapped DM Push Notification event for a send-to-tap funnel (TLON-5728). sendDmWithStory now returns the sentAt it already used internally to build the messageId; the nudge runner threads that through into both the telemetry call and the setLocalPendingNudge write. formatSentAt is exported so a new fixture test can pin the encoding cross-repo against Homestead's inverse. A dev-only trigger-nudge.sh script is added for preflight testing.
Security risks
None apparent. This is observability-only: PostHog properties carry a message id and a unix-ms timestamp — no secrets, no auth, no permissions, no user input crossing boundaries. The new dev script only mutates the bot's own %settings and explicitly only runs from a developer's host shell.
Level of scrutiny
Low. The change is mechanical and additive: existing event name, new optional-shaped fields, both nullable on send failure. The send path's behavior is unchanged (the returned object just gains a field). The runner's only behavioral shift is using the canonical sentAt for the pending-nudge sentAt write instead of a separate now() call — which is the intended correctness improvement (one timestamp, two consumers).
Other factors
- Test coverage is thorough: explicit assertions for both success (messageId+timestamp populated) and failure (both null), plus a cross-repo fixture canary that exercises the real
@urbit/auraencoding pipeline rather than the mock. - The fixture file deliberately has no top-level
vi.mock('@urbit/aura')and the comment explains why — that's exactly the right discipline for a cross-repo pin. - End-to-end verified on Android against the Tlon Staging PostHog project per the PR description.
- The
localSentAtfallback in the runner is dead-but-defensive (only fires if send 'succeeded' but result is missing, which shouldn't happen given the typed shape) — fine to keep as belt-and-braces.
The send-side half of the cross-repo nudge → tap funnel. Adds the join key
(and a sibling timestamp) to the existing
TlonBot Heartbeat Nudge Sentevent so it can be correlated, in PostHog, with the mobile
Tapped DM Push Notificationevent Homestead now emits.Why
We want to know whether bot nudge DMs actually bring users back into the
app. Homestead emits a
Tapped DM Push Notificationevent when a user tapsa DM push; PostHog joins it to this repo's
TlonBot Heartbeat Nudge Sentevent on
messageIdto compute a send-to-tap conversion rate.Today the send event carries no
messageId, so there is nothing to joinon. This PR adds it. The change is additive — same event name, new
fields — so it is safe to land independently of the Homestead PR.
What changed
sendDmWithStoryreturns itssentAtsrc/urbit/send.ts— exportformatSentAt; both return branches ofsendDmWithStory(and thereforesendDm) now return{ channel, messageId, sentAt }.Nudge runner threads the canonical id + timestamp
src/monitor/nudge-runner.ts— tightenNudgeRunnerDeps.sendDmto theconcrete return shape; capture
messageId/sentAtfrom the sendresult; pass them into
captureHeartbeatNudge; use the canonicalsentAtfor the success-pathsetLocalPendingNudgewrite so the pendingrecord agrees with the telemetry event. On send failure both stay
null.Telemetry event gains two fields
src/telemetry.ts— extendTlonHeartbeatNudgeEventwithmessageId: string | nullandnudgeSentAtMs: number | null, and writeboth into the PostHog event properties. Event name is unchanged.
Tests
src/urbit/send.test.ts— assert the newsentAt/channelreturnfields (existing
@urbit/auramock kept).src/urbit/send.fixtures.test.ts(new) — cross-repo fixture canary thatpins
formatSentAtoutput to the exactmessageIdstrings Homestead'sgetIdPartsinverts. Deliberately has no top-level@urbit/auramock so it exercises the real encoding pipeline.
src/monitor/nudge-runner.test.ts,src/telemetry.test.ts— assert thenew fields on a successful send and
nullon failure.Dev tooling
dev/trigger-nudge.sh(new) — primes the bot's%settings(
lastOwnerMessageAt, clearslastNudgeStage, 24h UTC active hours) sothe next runner tick fires a nudge; used to drive the end-to-end PostHog
preflight.
--resetclears the primed settings.Testing
pnpm test— full suite passes (35 files / 541 tests).node ./node_modules/typescript/bin/tsc --noEmit— passes.send.fixtures.test.tsand Homestead's
dmTapTelemetryRoundTrip.test.tspin the same(sentAt, messageId)fixtures.a nudge send joined to a mobile DM tap on
messageId(sends = 1 / converted_sends = 1).
Notes
order; the funnel only becomes useful once both ship.