Skip to content

feat(rpc): type and gate alarm trigger conditions - #988

Merged
izadoesdev merged 5 commits into
databuddy-analytics:stagingfrom
FindMalek:feat/typed-alarm-trigger-conditions
Oct 17, 2026
Merged

izadoesdev merged 5 commits into
databuddy-analytics:stagingfrom
FindMalek:feat/typed-alarm-trigger-conditions

Conversation

@FindMalek

@FindMalek FindMalek commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Addresses part of #958 — the "typed trigger conditions plus RPC validation" slice from the delivery plan, not the full issue. Evaluators, alarm_states, scheduling, and the dashboard picker/preview stay out of scope, pending the maintainer's answer to the issue's open architecture question ("where should the evaluator run?").

  • Replaced triggerConditions: z.record(z.string(), z.unknown()) with a discriminated union on triggerType, following the same pattern already used for destinations (z.discriminatedUnion("type", [...])) in this file:
    • uptime → { monitorIds: string[] } (defaults to []), matching what the code already expected informally (alarmMonitorIds in the dashboard and uptime-transition-alerts.ts already just check Array.isArray(...)).
    • traffic_spike / error_rate → typed per the shapes proposed in the issue, so the contract is documented, but see below — they're not creatable yet.
  • alarms.create and alarms.update now reject any trigger type other than uptime with a BAD_REQUEST explaining there's no evaluator for it yet, instead of silently accepting and persisting a dead alarm. update resolves the effective trigger type/conditions against the existing row when only one of the two fields is sent (e.g. the monitor-linking flow in monitor-sheet.tsx, which patches triggerConditions without triggerType).
  • Read paths (alarms.list, the alarms.create/update output schema) are unchanged and stay permissive (z.record), so any row written before this change — regardless of trigger type or conditions shape — stays listable. Only writes are tightened.
  • Added packages/rpc/src/routers/alarms.test.ts covering: valid/defaulted uptime conditions, malformed uptime conditions, and rejection of traffic_spike/error_rate/unknown types.

Verification

  • Checked the dashboard: alarm-sheet.tsx always hardcodes triggerType: "uptime" on create and has no UI for selecting traffic_spike/error_rate — there's nothing to remove, the tightened API doesn't break any existing UI flow.
  • Checked the create→link-monitor flow (alarm-sheet.tsx creates with triggerConditions: {}, then monitor-sheet.tsx later patches in monitorIds via alarms.update): still works because monitorIds defaults to [].
  • Ran bun run check-types, bun run lint, and packages/rpc's full bun run test (223 pass) locally against Postgres/Redis/ClickHouse. The pre-push hook additionally ran the full monorepo bun run test (1101+ pass across all packages) — all green.
  • Local dev DB was empty, so I couldn't inspect real stored alarm rows directly; reasoned through the code paths above instead (schema defaults handle the only row shape the app itself ever writes for uptime).

Out of scope (left for follow-up PRs, per the issue's delivery plan)

  • The actual traffic_spike/error_rate evaluators, alarm_states table, and BullMQ scheduling (step 2/3 in the issue).
  • The dashboard trigger-type picker and "would have fired" preview (step 4) — moot until an evaluator exists.

Disclosure: implemented with AI assistance (Claude Code); verified and tested by hand before submitting.


Summary by cubic

Types and gates alarm trigger conditions so alarms.create/update reject alarms nothing can evaluate. Previously any trigger type was accepted with untyped conditions, so traffic_spike and error_rate alarms saved but could never fire. Writes now validate triggerConditions against triggerType: uptime requires monitorIds: string[] (defaulting to []), and other types get a BAD_REQUEST. Webhook header blocking now reuses the shared FORBIDDEN_WEBHOOK_HEADERS export so alarms reject the headers delivery actually drops. Read paths stay permissive, so alarms written before this change remain listable. update resolves the effective trigger type and conditions against the stored row via a pure resolveTriggerUpdate helper, keeping single-field updates like the monitor-linking flow working. Adds packages/rpc/src/routers/alarms.test.ts covering defaulted and malformed uptime conditions, single-field resolution, and rejection of unsupported trigger types.

Written for commit 9d5078f. Summary will update on new commits.

View guided diff

@FindMalek

Copy link
Copy Markdown
Contributor Author

@izadoesdev could you take a look when you have a chance? This covers the typed-trigger-conditions slice of #958 (step 1 of the delivery plan) — evaluators and the rest stay gated on your call on where they should run.

@vercel

vercel Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
documentation Skipped Skipped Oct 17, 2026 5:51pm UTC

@vercel
vercel Bot temporarily deployed to Preview – documentation September 29, 2026 18:40 Inactive
@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: cc78148d-3387-4128-8652-b3eb1769183c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@vercel

vercel Bot commented Sep 29, 2026

Copy link
Copy Markdown

@FindMalek is attempting to deploy a commit to the Databuddy OSS Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

[Medium impact] Adds validation for alarm trigger conditions.

The latest change introduces no identified failure, but the outstanding repository-rule findings must be addressed before merging.

Findings

  1. P2 Prohibited unknown type ▶
  2. P2 Update path remains untested ▶
  3. P2 New identifier violates naming convention ▶

Summary

The PR types alarm trigger conditions and limits writes to trigger types with an evaluator. The latest change replaces Object.assign with object spread when building alarm update data; it does not change the resulting update.

Reviews (3) · Last reviewed commit: "fix(rpc): build alarm update data with s..." · Reviewed by Greptile

Comment thread packages/rpc/src/routers/alarms.ts Outdated
Comment thread packages/rpc/src/routers/alarms.test.ts Outdated
@FindMalek

Copy link
Copy Markdown
Contributor Author

fixed both. removed the unnecessary Record<string, unknown> annotation on updateData, it infers fine without it same as before this PR. and pulled the trigger resolution logic in the update handler into its own function with tests covering exactly the case you flagged - sending only triggerConditions (or only triggerType) and resolving the other from the stored row, which is what the monitor-linking flow does.

@vercel
vercel Bot temporarily deployed to Preview – documentation September 29, 2026 19:19 Inactive
@FindMalek

Copy link
Copy Markdown
Contributor Author

@greptile review

Comment thread packages/rpc/src/routers/alarms.ts Outdated
@greptile-apps

This comment has been minimized.

@FindMalek

Copy link
Copy Markdown
Contributor Author

fixed, builds updateData with spread now instead of Object.assign

@FindMalek

Copy link
Copy Markdown
Contributor Author

@greptile review

FindMalek and others added 5 commits October 17, 2026 16:41
triggerConditions was an untyped z.record(unknown), and create/update
accepted traffic_spike and error_rate trigger types with no evaluator
for either, so those alarms saved successfully but could never fire.

Add a discriminated-union schema keyed on triggerType (uptime requires
monitorIds: string[], matching current informal usage; traffic_spike
and error_rate are typed per the shapes proposed in databuddy-analytics#958 for when an
evaluator exists). Reject create/update for any trigger type without
an evaluator with a clear error instead of silently accepting it. Read
paths stay untyped so existing rows of any shape remain listable.
Removes an unnecessary explicit Record<string, unknown> annotation on
updateData (the previous inferred type already worked and matches the
type this repo already used here before).

Extracts the trigger-type/conditions resolution in alarms.update into
resolveTriggerUpdate, a pure function with its own tests, covering the
case the update handler didn't have direct coverage for: sending only
triggerConditions (or only triggerType) and resolving the other side
from the currently stored row, which is what the monitor-linking flow
actually does.
@izadoesdev
izadoesdev force-pushed the feat/typed-alarm-trigger-conditions branch from e3d9d07 to 9d5078f Compare October 17, 2026 17:51
@vercel
vercel Bot temporarily deployed to Preview – documentation October 17, 2026 17:51 Inactive
@izadoesdev
izadoesdev merged commit 696012e into databuddy-analytics:staging Oct 17, 2026
19 of 22 checks passed

This branch was previously deployed

1 inactive deployment
Preview – documentation — 9d5078f3 Deployed Oct 17, 2026 by vercel[bot]
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.

2 participants