Skip to content

fix(web): send symbolic comparator values for service alert rules - #883

Merged
dviejokfs merged 2 commits into
mainfrom
fix/service-alert-comparator-mismatch
Sep 2, 2026
Merged

fix(web): send symbolic comparator values for service alert rules#883
dviejokfs merged 2 commits into
mainfrom
fix/service-alert-comparator-mismatch

Conversation

@dviejokfs

Copy link
Copy Markdown
Contributor

Summary

  • The "Add Alert Rule" dialog on a service's monitoring page (ServiceMonitoring.tsx) sent comparator values gt/gte/lt/lte, but POST /external-services/{id}/metrics/alert-rules only ever accepts the symbolic operators >, <, >=, <= — matching the OpenAPI spec, the monitoring_alert_rules DB CHECK constraint, and the default seed rules. Every submission from that form was rejected with 400 Bad Request: comparator must be one of: >, <, >=, <=.
  • Root cause: this page calls the older external-services alert-rules API, but its comparator options were apparently copied from a different, newer alerts API (OTel detectors) that does use text-form values — the two APIs have different contracts and this form used the wrong one.
  • Fix: extracted the comparator type/options into web/src/lib/service-alert-comparator.ts (symbolic values matching the actual endpoint), updated ServiceMonitoring.tsx to use it, and added a regression test (service-alert-comparator.test.ts) asserting the options never drift from the backend's accepted set again.
  • No backend changes — the Rust handler, DB constraint, and OpenAPI spec were already correct.

Test plan

  • bun test src — all 460 existing frontend tests + 2 new ones pass.
  • bunx tsc --noEmit — clean.
  • Reproduced the original bug directly against a local server: comparator: "gt"400.
  • Confirmed the fixed frontend's value succeeds against the same unmodified backend: comparator: ">"201.
  • Full browser walkthrough: logged into a local dev instance, created a Postgres external service, opened its monitoring page, filled out "Add Alert Rule" (comparator dropdown now shows > greater than, ≥ greater or equal, < less than, ≤ less or equal), submitted, and got a 201 response plus an "Alert rule created" success toast, with the new rule appearing in the alert rules table.

The "Add Alert Rule" dialog on the service monitoring page sent
comparator values gt/gte/lt/lte, but POST
/external-services/{id}/metrics/alert-rules only ever accepted the
symbolic operators >, <, >=, <= (matching the OpenAPI spec, the DB
CHECK constraint, and the default seed rules). Every submission from
that form was rejected with 400 Bad Request.

The text-form values match a different, newer alerts API (OTel
detectors) that this page doesn't call — likely copied from there.
Extract the comparator options into a small shared module so the
values can't drift from the endpoint's contract again, and add a
regression test asserting the options stay within the backend's
accepted set.

Signed-off-by: David Viejo <dviejo@kfs.es>
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

📓 Changelog preview

This is what your commits will add to the generated CHANGELOG.md at release time (via git-cliff). Do not edit CHANGELOG.md by hand — it is generated from your Conventional Commit messages.

## [Unreleased]

### Fixed

- **web:** Send symbolic comparator values for service alert rules

### Testing

- **web:** Assert exact comparator option set, not a subset

@greptile-apps

greptile-apps Bot commented Sep 2, 2026

Copy link
Copy Markdown

Greptile Summary

The PR updates service alert-rule submissions to use the symbolic comparator values required by the external-services API.

  • Extracts the service-alert comparator type and display options into a shared module.
  • Updates the monitoring dialog to submit symbolic operators.
  • Strengthens regression coverage by checking exact option membership and duplicate values.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
web/src/lib/service-alert-comparator.ts Defines the four symbolic comparator values and their UI labels in a shared typed constant.
web/src/lib/service-alert-comparator.test.ts Verifies exact comparator membership and rejects duplicate option values.
web/src/pages/ServiceMonitoring.tsx Uses the shared symbolic comparator contract when rendering and submitting alert rules.

Reviews (2): Last reviewed commit: "test(web): assert exact comparator optio..." | Re-trigger Greptile

Comment on lines +1 to +5
// SPDX-FileCopyrightText: 2024-2026 Temps Contributors
// SPDX-License-Identifier: MIT OR Apache-2.0

import { describe, expect, test } from 'bun:test'
import { SERVICE_ALERT_COMPARATOR_OPTIONS } from './service-alert-comparator'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Tests are not colocated

This standalone test file separates the comparator contract tests from service-alert-comparator.ts, contrary to the repository requirement that tests remain in the same file as the code they exercise.

Context Used: CLAUDE.md (source)

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!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping this as a separate file rather than merging — every other module in web/src/lib/ (password-policy.ts, drop-archive.ts, etc.) follows the same convention: implementation + a colocated but separate *.test.ts. The CLAUDE.md "same file" rule reads as the Rust convention (#[cfg(test)] mod tests inline); there's no precedent anywhere in web/ for TS tests living inside the implementation file itself, so I don't think this is a real deviation.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right — this is a false positive. The repository-wide “same file” guidance reflects the Rust #[cfg(test)] convention, while the established TypeScript convention in web/src/lib/ is a separate colocated *.test.ts file. Keeping service-alert-comparator.test.ts separate is consistent with the surrounding modules, so no change is needed.

Comment thread web/src/lib/service-alert-comparator.test.ts Outdated
@dviejokfs
dviejokfs enabled auto-merge (squash) September 2, 2026 11:51
Greptile review on #883: the previous assertion only checked that
every declared option was in the backend's accepted set, so silently
dropping one of the four supported comparators (e.g. "<=") would
still pass. Assert set equality instead.

Signed-off-by: David Viejo <dviejo@kfs.es>
@dviejokfs
dviejokfs merged commit 0a9fe14 into main Sep 2, 2026
25 of 26 checks passed
dviejokfs added a commit that referenced this pull request Sep 2, 2026
…valid Info severity

main independently fixed the same comparator/severity contract bug (#883,
#884) this branch fixes for the service-monitoring alert-rule dialog.
Resolve by taking main's shared `service-alert-comparator` module and
also dropping the "Info" severity option in ServiceMonitoring.tsx's
dialog (mirroring MonitoringCard.tsx) since validate_severity only
accepts warning/critical, while keeping this branch's accessible-name
additions (id/aria-label on every alert-rule modal control).

Signed-off-by: David Viejo <dviejo@kfs.es>
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