fix(notifications): coalesce repeated alert events#4985
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Greptile SummaryThis PR implements coalesce-key deduplication for market alert and aviation notifications, collapsing repeated alerts for the same asset family or airport within a TTL window rather than deduping on the rounded-percent subject string. The
Confidence Score: 4/5The change is safe to merge; the core dedup logic is correct and both publishers now produce consistent coalesceKeys. The coalesceKey mechanics are sound — the same asset/severity/direction tuple always produces the same Redis dedup key, repeated percent fluctuations within a severity band collapse correctly, and a severity crossing generates a distinct key. The only structural gap is that direction and severity are passed raw into the key template while identifier is normalized, which is harmless today but inconsistent. The aviation:delay prefix on an aviation_closure event type is a naming mismatch that could mislead future maintainers but does not break dedup behavior. The naming discrepancy between the aviation_closure event type and the aviation:delay: coalesceKey prefix in seed-aviation.mjs is worth a second look; everything else is straightforward. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Seed run: equity/commodity/crypto] --> B{q.change >= threshold?}
B -- No --> Z[Skip]
B -- Yes --> C[Compute severity & direction]
C --> D[marketAlertCoalesceKey\nassetClass:identifier:direction:severity]
D --> E[publishNotificationEvent\npayload with coalesceKey]
E --> F{payload.coalesceKey set?}
F -- Yes --> G[dedupMaterial = coalesce: + key]
F -- No --> H[dedupMaterial = eventType:title]
G --> I[notifySimpleHash dedupMaterial]
H --> I
I --> J[wm:notif:scan-dedup:eventType:hash]
J --> K{Redis SETNX}
K -- Miss / new --> L[LPUSH wm:events:queue]
K -- Hit / dup --> M[Dedup skip — log and return]
L -- fail --> N[DEL dedupKey rollback]
L -- ok --> O[Notification queued]
P[Aviation/NOTAM seed] --> Q[dispatchAviationNotifications\ncoalesceKey = aviation:delay:IATA]
P --> R[dispatchNotamNotifications\ncoalesceKey = notam:closure:ICAO]
Q --> E
R --> E
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Seed run: equity/commodity/crypto] --> B{q.change >= threshold?}
B -- No --> Z[Skip]
B -- Yes --> C[Compute severity & direction]
C --> D[marketAlertCoalesceKey\nassetClass:identifier:direction:severity]
D --> E[publishNotificationEvent\npayload with coalesceKey]
E --> F{payload.coalesceKey set?}
F -- Yes --> G[dedupMaterial = coalesce: + key]
F -- No --> H[dedupMaterial = eventType:title]
G --> I[notifySimpleHash dedupMaterial]
H --> I
I --> J[wm:notif:scan-dedup:eventType:hash]
J --> K{Redis SETNX}
K -- Miss / new --> L[LPUSH wm:events:queue]
K -- Hit / dup --> M[Dedup skip — log and return]
L -- fail --> N[DEL dedupKey rollback]
L -- ok --> O[Notification queued]
P[Aviation/NOTAM seed] --> Q[dispatchAviationNotifications\ncoalesceKey = aviation:delay:IATA]
P --> R[dispatchNotamNotifications\ncoalesceKey = notam:closure:ICAO]
Q --> E
R --> E
Reviews (1): Last reviewed commit: "fix(notifications): coalesce repeated al..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| payload: { | ||
| title: `${a.iata}${a.city ? ` (${a.city})` : ''}: ${a.reason || 'Airport disruption'}`, | ||
| source: 'AviationStack', | ||
| coalesceKey: `aviation:delay:${a.iata}`, |
There was a problem hiding this comment.
The dispatchAviationNotifications function fires eventType: 'aviation_closure', but the coalesceKey is prefixed aviation:delay:. The naming mismatch means the coalesceKey describes delays while the event type says closure — someone reading only the event type or only the key gets a different mental model. The function actually processes FLIGHT_DELAY_SEVERITY_SEVERE/MAJOR records (delays, not closures), so the existing event type name may itself be a pre-existing misnomer, but introducing a coalesceKey with a different semantic label deepens the ambiguity. Consider aligning the prefix with the event type (e.g., aviation:closure:${a.iata}) or renaming the event type to aviation_delay.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| function marketAlertCoalesceKey(assetClass, identifier, direction, severity) { | ||
| const stableIdentifier = String(identifier || 'unknown').trim().toLowerCase(); | ||
| return `market:${assetClass}:${stableIdentifier}:${direction}:${severity}`; | ||
| } |
There was a problem hiding this comment.
identifier argument is normalized (.trim().toLowerCase()) but direction and severity are embedded raw. In callers they are always string literals so there is no current bug, but the inconsistency means a future caller passing a mixed-case direction (e.g., from an API enum) would produce a different key than expected. Normalizing all segments keeps the function contract self-consistent.
| function marketAlertCoalesceKey(assetClass, identifier, direction, severity) { | |
| const stableIdentifier = String(identifier || 'unknown').trim().toLowerCase(); | |
| return `market:${assetClass}:${stableIdentifier}:${direction}:${severity}`; | |
| } | |
| function marketAlertCoalesceKey(assetClass, identifier, direction, severity) { | |
| const stableIdentifier = String(identifier || 'unknown').trim().toLowerCase(); | |
| const stableDirection = String(direction || '').trim().toLowerCase(); | |
| const stableSeverity = String(severity || '').trim().toLowerCase(); | |
| return `market:${assetClass}:${stableIdentifier}:${stableDirection}:${stableSeverity}`; | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…erity coalesce key + test locks (#4985) Addresses the ce-code-review findings on PR #4985: - #2: extract the byte-identical dedup-material ternary from ais-relay.cjs, seed-aviation.mjs, and notification-relay.cjs into a single source of truth scripts/shared/notification-dedup.cjs::buildDedupMaterial (require from CJS, ESM import from the seeder). Behavior-preserving. - #1 + #3: aviation coalesce key aviation:delay:${iata} -> aviation:closure:${iata}:${severity} — renames the prefix to match the aviation_closure eventType and the notam:closure sibling, and folds the severity band into the key so a post-recovery MAJOR->SEVERE re-escalation re-notifies within the 4h window (mirrors marketAlertCoalesceKey). NOTAM key left airport-only; closure-subject coalescing by airport is the PR's stated intent. - #4: lock the marketAlertCoalesceKey stableIdentifier normalization line in the coalesce test, and assert all three publishers use the shared helper. Validation: node --test tests/notification-relay-coalesce-key.test.mjs (21 pass), tests/notification-relay-payload-audit.test.mjs (7 pass), node --check on all four scripts, CJS require + ESM import interop verified. Claude-Session: https://claude.ai/code/session_01MNFwKS7tAevgd7u1v8Dp49
…e.relay (#4985) The shared buildDedupMaterial helper extracted in the previous commit is require()'d by scripts/ais-relay.cjs (the relay entrypoint), so the relay Docker image must COPY it or the container crashes at startup with ERR_MODULE_NOT_FOUND. Caught by tests/dockerfile-relay-imports.test.mjs in the full CI unit suite (the local pre-push runs only the changed test file). Claude-Session: https://claude.ai/code/session_01MNFwKS7tAevgd7u1v8Dp49
…scalations actually re-notify (#4985 review P1) The prior commit made the aviation coalesce key severity-aware (aviation:closure:${iata}:${severity}) so a MAJOR->SEVERE escalation could re-notify. But dispatchAviationNotifications still diffed previous state by airport ALONE (`!prevSet.has(a.iata)`), which filters an already-alerted airport's escalation out upstream — before the severity-aware coalesce key is ever reached. Net: the intended high->critical escalation never published for the common continuous case. The two dedup layers used mismatched identities. Fix: key the prev-alerted set and the newAlerts diff by the SAME airport+severity identity the coalesce key uses (aviationAlertKey = `${iata}:${band}`), via a shared aviationSeverityBand helper reused by the loop's severity too. Behavior: - MAJOR->SEVERE escalation for an already-alerted airport now publishes (goal). - SEVERE->MAJOR de-escalation also re-notifies once (as 'high') — symmetric identity; a downgrade is a distinct, informative state and the 4h publisher TTL still prevents same-band spam. - One-time on deploy: prevSet holds the old iata-only format, so currently-severe airports re-notify once (bounded to slice(0,3)); self-heals next tick (24h TTL). NOTAM is unchanged: its coalesce key and prev-state are both ICAO-only (severity is constant 'high'), so no identity mismatch there. Repro + proof: tests/notification-relay-coalesce-key.test.mjs gains a source guard asserting the airport+severity diff identity (failed before this fix, passes after). node --test: 22 pass; payload-audit 7 pass; node --check clean. Claude-Session: https://claude.ai/code/session_01MNFwKS7tAevgd7u1v8Dp49
1 participant
Summary
Repeated market alert emails now collapse by asset family instead of the rounded percent in the subject. Coffee +11%, +12%, and +13% critical surge updates share one Redis coalesce family during the existing publisher/user dedupe window, while a high-to-critical threshold crossing can still notify as a real severity upgrade.
Aviation and NOTAM producers now use the same coalesce-key branch in their local publisher copy, so airport delay or closure subjects can change without bypassing scan dedupe.
Validation
node --test tests/notification-relay-coalesce-key.test.mjsnode --test tests/notification-relay-payload-audit.test.mjsnode --check scripts/ais-relay.cjsnode --check scripts/seed-aviation.mjsgit diff --checkgit push -u origin HEADpre-push hook: typecheck, API typecheck, Convex typecheck, CJS syntax, Unicode safety, boundary lint, safe-HTML lint, Sentry coverage gate, rate-limit policy lint, premium-fetch parity, edge bundle check, changed coalesce test, and version sync all passed.