Repository navigation
Add a marketing tool API with marketer keys and audience queries - #78
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
e87a23a to
d2f9ec5
Compare
peachbits
changed the title
a733a6d to
59be39d
Compare
17f2e44 to
d0d30d2
Compare
Document the server config shape in a committed sample file, since the real pushServerConfig.json is gitignored. Copy it to pushServerConfig.json and fill in real values to run the server.
d0d30d2 to
89a123d
Compare
89a123d to
d43714e
Compare
j0ntz
left a comment
j0ntz
left a comment
There was a problem hiding this comment.
Two commit subjects are over the 50-character limit (review-standards, commit-subject-50): Add the marketing location filter and device skip reasons (57) and Add an api-key location view and batched device lookups (55). Suggested: Add marketing location filter and skip reasons (46) and Add api-key location view and batched lookups (45).
| let queued = 0 | ||
| for (const device of devices.values()) { | ||
| const { deviceId } = device | ||
| await sender.sendToDevice(device, message).catch((error: unknown) => { | ||
| write(`Device ${deviceId} failed: ${String(error)}\n`) | ||
| }) | ||
| queueing.update(++queued) | ||
| } | ||
| queueing.finish(queued) | ||
|
|
||
| // Deliberately not "sent": this hands the messages to the queue, and | ||
| // the publish daemon delivers them afterwards, which for a large | ||
| // audience runs long after this response ends. | ||
| write(`\n[DONE] ${devices.size} devices queued for delivery.\n`) |
There was a problem hiding this comment.
Failed publishes are counted as queued. The .catch on L504 writes a Device ... failed line, but ++queued still runs and [DONE] reports devices.size. If the RabbitMQ channel drops mid-send, every remaining device prints a failure and the stream still ends with [DONE] N devices queued for delivery. The Marketing send queued log records the same inflated queued.
Count failures separately, report the real queued count in [DONE] and the log, and end with [ERROR] after a run of consecutive failures instead of walking the rest of the audience.
Related, L492: streamDeviceBatchesByIds de-dupes ids, but missing subtracts from the raw deviceIds.length, so ['a','a','b'] reports 1 no longer found. De-duping deviceIds before the send fixes that count and the Loaded denominator.
There was a problem hiding this comment.
Fixed in 22a320e. queued and failed are counted separately, [DONE] and the server log report the real queued count, ten failures in a row end the send with [ERROR] (the queue is gone, not the device), and ids are de-duplicated before anything is counted.
| res.status(400).json({ error: 'The audience is empty.' }) | ||
| return | ||
| } | ||
| await streamSend(connections, res, targets, deviceIds, message, { |
There was a problem hiding this comment.
An interrupted send has no safe retry. Nothing watches res for close, so the loop keeps publishing after the client or proxy drops, and the caller never sees [DONE] or how far it got. A deploy or crash mid-loop leaves a partial send with no record of which ids were queued. In both cases the only recovery is to post again, which mints a new campaignId (L205) and notifies everyone already queued a second time.
Options: accept a caller-supplied campaign id and refuse or resume a repeat, or log a resumable cursor (last queued index) on each progress tick. Publishing in bounded-concurrency batches would also shrink the window, since one awaited publish per device is the slow part of a six-figure audience.
There was a problem hiding this comment.
Agreed on the gap, and deferring it. What makes a retry actually safe is a persisted campaign record (which ids were queued, resume from there), and that is the campaign model we parked for a later PR rather than bolting a cursor onto this one. For now the server log carries the campaign id, label and final counts, and a send keeps publishing after the client drops so the blast at least completes.
| // A test ignores the marketing opt-out, so you can always reach | ||
| // your own device: | ||
| isMarketing: !isTest, |
There was a problem hiding this comment.
isTest is decided by the target shape (deviceId or loginId), not by whose device it is. With isMarketing: false, a marketer key can deliver any title, body, and url to an opted-out device under its targets, one request per user. If the opt-out is a promise to the user, limit the bypass to an allow-list of test device or login ids on the key doc, or force the Test Message title on this path.
There was a problem hiding this comment.
Fixed in 22a320e. Tests go through the same skip predicate as a campaign, so an opted-out device answers 400 and the test path cannot reach anyone a campaign could not. Tests send isMarketing: true as well, so the daemon re-checks uniformly.
|
|
||
| // This router parses its own bodies, since a device-id list is far larger | ||
| // than anything the public endpoints accept: | ||
| router.use(express.json({ limit: BODY_LIMIT })) |
There was a problem hiding this comment.
Parse failures from this express.json (malformed JSON, or a body over BODY_LIMIT) go to next(err), and neither the router nor the app has an error middleware. The caller gets Express's default HTML 400/413, with a stack trace when NODE_ENV is not production, instead of the { error } shape every other failure here uses. A four-argument error handler at the end of the router keeps the shape consistent.
There was a problem hiding this comment.
Fixed in 22a320e. A four-argument handler at the end of the router answers with { error } and the parser's own status (400/413), with tests.
| doc.apiKey, | ||
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions | ||
| doc.location.country || '', | ||
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions | ||
| doc.location.region || '', | ||
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions | ||
| doc.location.city || '' | ||
| ], | ||
| { | ||
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions | ||
| region: doc.location.region || '', | ||
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions |
There was a problem hiding this comment.
This view adds five strict-boolean-expressions disables. ?? is out here because sucrase compiles it to a _nullishCoalesce helper that does not exist in the Couch view server, but the disables are still avoidable:
- Drop
regionandcityfrom the value. They duplicatekey[2]andkey[3], and the reader already takescountryfrom the key. That also shrinks every index row. - Write the key parts as
doc.location.country != null ? doc.location.country : ''. The rule accepts a real comparison, and the ternary survives stringification unchanged.
There was a problem hiding this comment.
Done, thanks: region and city are out of the value (the reader takes them from the key), the null checks are ternaries with a comment on why not ??, and all five disables are gone. Free to change now since no production index has been built yet.
| const { code, errorInfo } = error as { | ||
| code?: string | ||
| errorInfo?: { code?: string } | ||
| } |
There was a problem hiding this comment.
Convention nit: inspect an unknown error with a cleaner instead of a type assertion (typescript-standards, cleaner-error-matching). asMaybe(asObject({ code: asOptional(asString), errorInfo: asOptional(asObject({ code: asOptional(asString) })) }))(error) reads the same two fields without the as.
There was a problem hiding this comment.
Fixed in 22a320e with asMaybe(asObject(...)).
| const percent = total <= 0 ? 100 : Math.floor((100 * done) / total) | ||
| const perSecond = elapsedMs > 0 ? done / (elapsedMs / 1000) : 0 | ||
|
|
||
| let line = ` ${label}: ${percent}% — ${commas(done)} of ${commas(total)}` |
There was a problem hiding this comment.
Nit: the em dash in this template ships in every progress line. We keep em dashes out of committed code and output because they read as an AI-text tell (ruleset). A comma in its place reads the same.
Marketing campaigns need to walk the devices registered under one app key, optionally within one country, and to re-read an explicit list of devices when sending. Add an `apiKeyLocation` view keyed by [apiKey, country, region, city], plus a batched-by-id lookup that skips missing or unreadable documents and yields a batch at a time so a long send can report progress. The view's row value carries everything an audience query needs to decide whether a device matches and to describe it (region, city, token, opt-out, last visit), and the summary takes its country from the row key, so a query reads the index alone rather than pulling hundreds of megabytes of documents to look at a few fields. A whole-country pass dropped from about eight minutes to half a minute. Devices with no location are absent from the view, and so unreachable by location targeting. Pages are 20,000 rows. A whole-country query pulls close to a million rows, and at 2,048 a page that is hundreds of round trips to Couch; measured against the production index, 20,000-row pages scan about twice as fast with the same bytes per row, and a page is about 6 MB of JSON, well within what the server parses comfortably. `countDevicesByCountry` reads the view's reduce grouped at the country level, so listing the countries under a key, with counts, costs milliseconds however large the key is. The counts are registrations before the token and opt-out checks, so they run above what an audience query returns.
The keys baked into the apps ship to every phone, so they make poor credentials for anything beyond device registration, and they cannot rotate without an app release. Give API keys a `marketer` flag that will gate the marketing endpoints, and a `targetApiKeys` list naming the app keys whose devices a marketing key may reach. Both fields default to harmless values, so existing key documents stay valid.
An audience is a set of devices narrowed by include and exclude lists of countries, regions and cities: the lines within a list are alternatives, the lists narrow each other, matching is case-insensitive, and an empty country include means every country, so "everywhere except the US and UK" is expressible. Regions match the stored two-letter codes. Region and city values are stored without their country, and the filter matches them the same way, so a rule on them means something different in every country it touches: "LA" is Lagos in Nigeria and Louisiana in the United States, and on the targeted keys three quarters of all devices carry a region code that some other country also uses. `getFilterProblem` therefore rejects a filter that combines region or city rules with anything but exactly one included country, with a message the API can return, so no caller can run one by accident. `getDeviceSkipReason` is the one predicate for whether a device may receive a marketing push: it must sit under a targeted app key, have a well-formed token, and not have opted out. It takes only the fields it reads so that audience queries can apply it to view rows, and sends to whole documents, without the two drifting apart.
Firebase reports an uninstalled app as `Error: NotRegistered`, or as a `messaging/registration-token-not-registered` code on the newer API, but the daemon only recognized an older wording. So the branch that clears a dead token never ran, and every send retried every dead token. A marketing send to 385,616 devices measured this: 208,892 of them, 54%, failed on unregistered tokens, and not one was disabled. Dry-run probes against a sample of production tokens put the dead share at a similar 62%, so roughly half of every send is spent on devices that cannot receive anything, and that share only grows. Match all three spellings, in a tested helper rather than inline, since this is the second time the wording has moved out from under the check. The daemon also guarded against a missing token but not an empty one, and some devices register with "" rather than nothing. Firebase rejects those with "Exactly one of topic, token or condition is required", one logged error per device on every send. Treat an empty token as missing. Also pass the device and error to Pino as its first argument. Passing them second silently dropped them, so all 208,892 failures logged as a bare "Unknown error" with nothing to diagnose.
A send to a few hundred thousand devices either goes quiet for minutes or emits a line per device, and neither says how far along it is or how much longer it will take. `makeProgressLog` reports a percentage on a fixed interval instead, with a rate and an estimate of the time left, and skips whole intervals after a stall rather than emitting a burst of catch-up lines. The clock is injectable, so the tests drive it by hand.
`makeMarketingData` builds the data payload every marketing push carries. This is the contract the app parses (parsePushMessage in edge-react-gui): `type` selects the marketing branch, `campaignId` ties opens back to a campaign, and the optional `url` is the deep link the app follows once the user logs in. The test also pins that a missing link leaves the `url` key out. FCM rejects non-string data values, so `url: undefined` would fail the whole send rather than just skip navigation.
Two routes under `/marketing`, consumed by the web UI in the internal
tools project, which supplies the API key. `POST /push` sends a message
to an audience named in one of four ways: a single `deviceId` or
`loginId` (a test), an explicit `deviceIds` list, or a `filter` of
include/exclude lists of countries, cities and regions, where the lines
within a list are alternatives, the lists narrow each other, matching
is case-insensitive, and an empty country include means every country.
A filter that puts region or city rules under anything but exactly one
included country is refused with a 400 that says so.
With `dryRun: true` the call resolves that audience and answers with the
list, each device with its country, region and city, instead of
sending, so the usual flow is: test to your own device, dry-run the
filter to review the audience, then send exactly the ids the dry run
returned. The flag is required with no default, so a body that forgets
it is refused rather than sent, and a body that names the audience two
ways is refused rather than guessed. Tests and dry runs answer JSON; a
send to a list streams a text progress log, since a large audience
takes minutes to queue.
The send's `[DONE]` line reports how many devices were actually handed
to the queue, with failures counted separately, and a run of
consecutive publish failures stops the send with `[ERROR]`, since that
means the queue is gone rather than the device. Ids are de-duplicated
before anything is counted, so a repeated id is not reported as a
device no longer found. Malformed or oversized bodies get the same
`{ error }` JSON as every other refusal, through the router's own error
handler, instead of Express's HTML page.
A single included country narrows the index scan to that country's
slice. The filter matches names case-insensitively while Couch keys do
not, so the slice is found under the spelling ip-api stored, read from
the view's reduce in milliseconds, rather than as typed; a spelling no
device carries scans nothing, which is exactly what the filter would
have matched. Any other country rule walks the key's whole slice and
lets the filter decide, which costs a longer dry run but no new index.
`GET /countries` answers with the countries devices are located in
under the caller's targeted keys, with counts, read from the view's
reduce. A caller building an audience needs to know which names the
filter will match, since they are whatever ip-api stored and a near
miss silently matches nothing.
Every send mints a campaign id, which the app reports back with each
open. The id is logged on the server at the start and end of the send,
with the device counts and an optional `label` the caller may give the
send, so a campaign can be found again by name once the operator's
browser tab is gone.
Every call requires a key with the `marketer` (or `admin`) flag and only
reaches devices under the key's `targetApiKeys`, enforced by one shared
skip predicate at dry-run and send time. Tests obey the marketing
opt-out like any other send, so no key can use the test path to reach
an opted-out device one request at a time. The router parses its own
bodies ahead of the app-wide 1mb parser, since a device-id list can run
to several megabytes.
d43714e to
22a320e
Compare
|
Both subjects reworded to your suggestions (45 and 46 chars). Every thread is addressed and folded into its logical commit; head is 22a320e, 8 commits, 55 tests. Re-requesting review. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5765a3d. Configure here.
The app follows any deep link a marketing push carries: PushMessageParser hands the payload url to parseDeepLink and launches whatever parses, and the apps already in the field have no allow-list of their own. That reaches far past navigation. A push could open the Send scene pre-filled with an address and amount (edge://pay, a bitcoin: URI, a provider redirect), start a WalletConnect pairing, approve an Edge login lobby, log the user out into password recovery, hand wallet addresses to a third-party URL (reqaddr), navigate to any scene with raw params, or open a partner webview with the wallet API, gift cards included. Refuse all of that at the server, dry runs included, so the operator hears about it before the audience is resolved. What remains is what a campaign actually links to: buy and sell (with a provider and payment type pin), swap, exchange/<buy|sell|swap> with its asset and promoId parameters, any plugin by id but with no query, promotion, modal/fundAccount and scene/earnScene, in the edge:// spelling or the https://deep.edge.app one, which may add the ?af= promotion wrapper. The grammar mirrors the app's parser, so links it would drop are refused too: af on the edge:// form, unknown parameters, trailing slashes, fragments, and dot segments, which a WHATWG-style parser would collapse into another kind. Plugin links carry no query because the app merges a link's query over the plugin's own parameters (`{ ...baseQuery, ...deepQuery }` in makePluginUri), api keys and referral codes included, and hands it to the partner page too, so a query could point a partner widget at someone else's account or refund address. The plugin id itself is not the risk: every plugin is a flow the user drives, so any id may be opened. The right home for this check is the app, which can see what each link does; this is the server-side stopgap for the versions already shipped.
3ac748c to
e5dde0e
Compare
2 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
none
Description
Adds the marketing push API consumed by the web UI in EdgeApp/edge-internal-tools#1. This replaces the earlier three-route version on this branch with a single endpoint, in logical commits, and has run two production campaigns (~215k devices each) from a laptop.
apiKeyLocationview keyed[apiKey, country, region, city]whose row value carries everything an audience query needs, so a whole-country pass reads the index alone (about 8 min → 30 s, then halved again by 20,000-row pages); plus a batched-by-id device lookup that yields per batch so a long send can report progress.marketerflag and atargetApiKeyslist. The endpoint requires the flag (oradmin) and only reaches devices registered under the listed keys; the api keys baked into the apps cannot call it, a marketing key rotates without an app release, and a key with no targets fails closed.POST /marketing/pushnames its audience in exactly one of four ways: a singledeviceIdorloginId(a test), an explicitdeviceIdslist, or afilterof include/exclude lists of countries, cities and regions, where an empty country include means every country.dryRunis required with no default:trueresolves the audience and answers with the list, so the flow is test → dry-run the filter → send exactly the returned ids. A body that forgets the flag, or names the audience two ways, is refused. Region and city rules are accepted only with exactly one included country, because those values are stored without their country (LAis Lagos in Nigeria and Louisiana in the US; 39% of region codes span more than one country). Sends stream a progress log and are logged server-side with the campaign id and an optionallabel.GET /marketing/countrieslists the countries devices are located in, with counts, so a picker offers exactly the names the filter matches.Kept deliberately simple: the send runs inside the request and nothing is persisted. A campaign-record model with a daemon, delivery counts and scheduling is parked on
backup/marketing-campaigns-20260921.Operational notes: the tool's key doc needs
{ marketer: true, targetApiKeys: ["<app api key>"] }indb_api_keys(noadminsdk; delivery credentials resolve from each device's own key). TheapiKeyLocationindex takes a while to build over 1.9M documents; let it finish before the first dry run or that request times out. The router parses its own bodies with a 64mb limit ahead of the app-wide 1mb parser, since a device-id list runs to ~12 MB for a country.Tested: 48 mocha tests over the body cleaner and target rule, the location filter, skip reasons and the single-country rule, the payload, the progress log and the Firebase error matcher; plus the two production sends.
Note
High Risk
Introduces bulk marketing push with broad audience reach and deep-link handling; mitigations include marketer-only auth, targetApiKeys scoping, opt-out respect, and URL allow-listing, but misconfiguration or filter bugs could still affect many users.
Overview
Adds a marketing tool API (
POST /marketing/push,GET /marketing/countries) for internal operators: test sends (device/login), explicit device lists, or geo filters (include/exclude countries, cities, regions). RequireddryRunresolves audiences without sending; real sends stream a progress log while messages are queued. API keys gainmarketerandtargetApiKeysso only dedicated keys can reach devices under listed app keys.Supporting work includes a Couch
apiKeyLocationview and streaming/batched device lookups for large audiences, deep-link allow-listing on pushes, and publish-daemon fixes (unregistered FCM tokens, empty tokens, correct Pino error logging).Reviewed by Cursor Bugbot for commit 5765a3d. Bugbot is set up for automated code reviews on this repo. Configure here.