refactor: unify overall status handling across components and improve… - #753
Conversation
… documentation, fixes #722
📝 WalkthroughWalkthroughThis PR implements consistent "problem-first" status ordering across all components. It introduces a ChangesConsistent Status Ordering
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes a long-standing masking bug (issue #722) where a single monitor under maintenance caused the page banner to show "Under Maintenance" even while another monitor was actively DOWN or DEGRADED. It also patched the all-monitors
Confidence Score: 4/5The core bug fix is correct and the canonical collapse function is well-reasoned; the only thing worth verifying before merge is the new active/non-hidden filter on single-monitor uptime/latency badges. The refactoring is clean and the canonical collapse function correctly fixes both the banner-masking bug and the missing MAINTENANCE branch in the badge loop. The one concern introduced by the PR is the undocumented change in single-monitor uptime/latency badge behaviour for hidden or inactive monitors — callers that relied on those badges returning historical data will now get an error SVG without any migration path. src/lib/server/controllers/monitorsController.ts — specifically the single-monitor uptime/latency badge path around line 641 where the new active/non-hidden filter was added. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Monitor statuses collected\n(ACTIVE, non-hidden)"] --> B["Accumulate counts\ncountOfUp / Down / Degraded / Maintenance"]
B --> C{"CollapseStatusCounts\n(canonical collapse)"}
C -->|"countOfDown > 0"| D["DOWN"]
C -->|"countOfDegraded > 0"| E["DEGRADED"]
C -->|"countOfMaintenance > 0"| F["MAINTENANCE"]
C -->|"countOfUp > 0 (only)"| G["UP"]
C -->|"total == 0"| H["NO_DATA"]
D --> I["GetStatusSummary\n≥75% → MAJOR_OUTAGE\nelse → PARTIAL_OUTAGE"]
E --> J["GetStatusSummary\n≥75% → DEGRADED_PERFORMANCE\nelse → PARTIAL_DEGRADED"]
F --> K["GetStatusSummary\nUNDER_MAINTENANCE"]
G --> L["GetStatusSummary\nALL_OPERATIONAL"]
H --> M["GetStatusSummary\nNO_DATA"]
subgraph "Page Banner (clientTools.ts)"
I
J
K
L
M
end
subgraph "_ Badge (monitorsController.ts)"
C2["CollapseStatusCounts\n(same function)"] --> D2["status string\nreturned directly"]
end
Reviews (1): Last reviewed commit: "refactor: unify overall status handling ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } else { | ||
| // Single monitor badge | ||
| const monitors = await GetMonitorsParsed({ tag }); | ||
| const monitors = await GetMonitorsParsed({ tag, status: GC.ACTIVE, is_hidden: GC.NO }); | ||
| if (monitors.length === 0) { |
There was a problem hiding this comment.
The single-monitor uptime/latency branch previously queried with only { tag } and returned data for any monitor regardless of status or hidden flag. Adding status: GC.ACTIVE, is_hidden: GC.NO means a badge URL for a hidden or inactive monitor (e.g. an internal service or a temporarily paused check) now returns ErrorSvg instead of historical uptime data. Status badges already behaved this way, so this is arguably a consistency improvement — but it is an undocumented breaking change for anyone who has embedded uptime or latency badges for non-ACTIVE or hidden monitors.
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!
There was a problem hiding this comment.
Pull request overview
This PR standardizes how “Overall Status” is derived when collapsing multiple monitor statuses into a single display status (banner and _ all-monitors badge), and updates documentation/ADR to describe the intended worst-wins ordering.
Changes:
- Added a shared
CollapseStatusCountshelper and updated page-status summary/color to use problem-first ordering (DOWN > DEGRADED > MAINTENANCE > UP). - Updated server-side all-monitors status aggregation (
GetLatestStatusActiveAll) to includeMAINTENANCEand to use the same canonical collapse logic. - Expanded docs (Sharing page, ADR, glossary/context) and removed the unused
getMonitoringDataAllrepository method and bindings.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/routes/(docs)/docs/content/v4/sharing.md | Documents _ all-monitors badge behavior and overall-status priority. |
| src/lib/server/db/repositories/monitoring.ts | Removes getMonitoringDataAll implementation. |
| src/lib/server/db/dbimpl.ts | Removes getMonitoringDataAll binding/exposure from the DB façade. |
| src/lib/server/controllers/monitorsController.ts | Uses canonical status-collapsing for all-monitors status and tightens badge monitor filtering via constants. |
| src/lib/clientTools.ts | Introduces CollapseStatusCounts and reworks summary/color to use the canonical ordering. |
| docs/adr/0007-problem-first-overall-status.md | Adds ADR documenting problem-first overall-status semantics. |
| CONTEXT.md | Adds/clarifies glossary entries for “Overall Status” and _ all-monitors badge. |
| @@ -0,0 +1,7 @@ | |||
| # Overall Status is problem-first: DOWN > DEGRADED > MAINTENANCE > UP | |||
|
|
|||
| Everywhere a set of monitor statuses collapses into one display status — the page banner (`GetStatusSummary`/`GetStatusColor` in `src/lib/clientTools.ts`), the per-day bar summaries, and the all-monitors `_` badge and dot badge (`GetLatestStatusActiveAll` in `src/lib/server/controllers/monitorsController.ts`) — the same worst-wins ordering applies: DOWN > DEGRADED > MAINTENANCE > UP, with NO_DATA only when no monitor has any data at all. | |||
|
|
||
| Everywhere a set of monitor statuses collapses into one display status — the page banner (`GetStatusSummary`/`GetStatusColor` in `src/lib/clientTools.ts`), the per-day bar summaries, and the all-monitors `_` badge and dot badge (`GetLatestStatusActiveAll` in `src/lib/server/controllers/monitorsController.ts`) — the same worst-wins ordering applies: DOWN > DEGRADED > MAINTENANCE > UP, with NO_DATA only when no monitor has any data at all. | ||
|
|
||
| Issue #717 exposed that the codebase had three independent answers to "what does maintenance mean when aggregating". The frontend banner checked maintenance first, so one monitor in a planned window reported "Under Maintenance" even while another monitor was hard DOWN — a real outage masked by planned work. The badge loop had no MAINTENANCE branch at all, so maintenance samples were silently skipped (DEGRADED+UP+MAINTENANCE → "Degraded", disagreeing with the banner) and a fleet entirely under maintenance fell through to "No Status Available". Group Monitor scoring counts maintenance as UP. With the same monitors, the page and the badge told different stories, which breaks any automation treating either as the source of truth. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/clientTools.ts`:
- Around line 323-329: Add table-driven unit tests for CollapseStatusCounts to
lock the precedence contract encoded in that function: create a test suite that
imports CollapseStatusCounts, StatusCounts and GC and asserts expected
StatusType for cases including (1) countOfDown>0 and countOfMaintenance>0 =>
GC.DOWN, (2) countOfDegraded>0 and countOfMaintenance>0 => GC.DEGRADED, (3) only
countOfMaintenance>0 => GC.MAINTENANCE, plus baseline cases like all zeros =>
GC.NO_DATA and only up => GC.UP; structure tests as small table rows mapping
input StatusCounts to expected GC value to ensure future changes preserve this
precedence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f59580ae-a70e-4395-9338-5c481682b7d9
📒 Files selected for processing (7)
CONTEXT.mddocs/adr/0007-problem-first-overall-status.mdsrc/lib/clientTools.tssrc/lib/server/controllers/monitorsController.tssrc/lib/server/db/dbimpl.tssrc/lib/server/db/repositories/monitoring.tssrc/routes/(docs)/docs/content/v4/sharing.md
💤 Files with no reviewable changes (2)
- src/lib/server/db/dbimpl.ts
- src/lib/server/db/repositories/monitoring.ts
| function CollapseStatusCounts(counts: StatusCounts): StatusType { | ||
| const total = counts.countOfUp + counts.countOfDown + counts.countOfDegraded + counts.countOfMaintenance; | ||
| if (total === 0) return GC.NO_DATA; | ||
| if (counts.countOfDown > 0) return GC.DOWN; | ||
| if (counts.countOfDegraded > 0) return GC.DEGRADED; | ||
| if (counts.countOfMaintenance > 0) return GC.MAINTENANCE; | ||
| return GC.UP; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Lock the new precedence contract in tests.
This helper now defines the canonical rule for both the local banner/color helpers and the server-side overall-status path, so a few table-driven cases would pay off here: DOWN + MAINTENANCE => DOWN, DEGRADED + MAINTENANCE => DEGRADED, and MAINTENANCE only => MAINTENANCE.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/clientTools.ts` around lines 323 - 329, Add table-driven unit tests
for CollapseStatusCounts to lock the precedence contract encoded in that
function: create a test suite that imports CollapseStatusCounts, StatusCounts
and GC and asserts expected StatusType for cases including (1) countOfDown>0 and
countOfMaintenance>0 => GC.DOWN, (2) countOfDegraded>0 and countOfMaintenance>0
=> GC.DEGRADED, (3) only countOfMaintenance>0 => GC.MAINTENANCE, plus baseline
cases like all zeros => GC.NO_DATA and only up => GC.UP; structure tests as
small table rows mapping input StatusCounts to expected GC value to ensure
future changes preserve this precedence.
… documentation, fixes #717
Summary by CodeRabbit
Bug Fixes
Documentation