Skip to content

fix(workflows): keep push test sends out of metrics and assets - #75800

Draft
dmarchuk wants to merge 2 commits into
masterfrom
dmarchuk/push-test-sends-skip-metrics
Draft

fix(workflows): keep push test sends out of metrics and assets#75800
dmarchuk wants to merge 2 commits into
masterfrom
dmarchuk/push-test-sends-skip-metrics

Conversation

@dmarchuk

Copy link
Copy Markdown
Contributor

Problem

A push step's "Run test" in the editor is indistinguishable from a real send. It records push_sent / push_skipped / push_failed, and since #74555 it also captures a row on the Assets tab. So testing a step inflates the workflow's Metrics tab and puts a preview in the list of what real recipients were sent.

The email path already avoids exactly this. executeSendEmail takes an isTest flag and skips the business metric, the skip-reason metric, the asset capture and the engagement event when it is set. Push never received the flag: the executor calls executeSendPushNotification(nextInvocation) with nothing, and the signature has nowhere to put it.

The metrics half predates #74555. The Assets row is new with it, which is what makes the inconsistency worth closing now.

Changes

Threads the same signal email uses. sendEmailsInline is only ever set by the test panel, so it doubles as "this is a test send", and the executor now passes it to the push branch too.

  • pushMetric returns early when isTest, so none of the three business metrics land.
  • The asset capture is skipped for the same reason: the Assets tab should show what real recipients were sent, not what an editor preview produced.

The send itself still happens, and the run log still reports acceptance per channel, so a test send behaves as before from the tester's point of view.

How did you test this code?

push-notification.service.test.ts - 34 tests, one new:

  • A test send records no metrics and no asset, while still logging that FCM accepted it. Catches the case where only one of the two guards is added, which is easy to do since they sit in different parts of the function.

The other 33 confirm the added parameter did not disturb the existing send, fan-out, skip, prune, reschedule or capture behavior.

tsc --noEmit adds no new errors (7 pre-existing @posthog/replay-anonymizer module-resolution errors are present on the base branch too, from an unbuilt local package).

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

Docs update

None needed. Test sends are not a documented surface.

🤖 Agent context

Autonomy: Human-driven (agent-assisted). Directed by @dmarchuk, assigned as DRI.

Stacked on #74555, which introduces the asset capture this guards.

Found by asking whether push does the same thing as email for test sends. It doesn't, and the gap was invisible from the push side alone: there is no isTest in that file to notice the absence of. The email service guards four separate places on it, which is the shape to copy if push later gains engagement events of its own.

Base automatically changed from claude/push-message-assets to master July 31, 2026 08:11
@trunk-io

trunk-io Bot commented Jul 31, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@dmarchuk
dmarchuk force-pushed the dmarchuk/push-test-sends-skip-metrics branch from 7835edf to 3f667e1 Compare July 31, 2026 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant