added downtime minutes and availability view#114
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| </div> | ||
| </div> | ||
| ))} | ||
| <div className="flex items-center gap-space-md"> |
There was a problem hiding this comment.
Would prefer to pull this out to a subcomponent, even in the same file is fine. Just a little easier to read that way.
|
|
||
| const isRootRoute = routerState.location.pathname === '/'; | ||
| const pathname = routerState.location.pathname; | ||
| const isTopLevelRoute = pathname === '/' || pathname.startsWith('/availability'); |
There was a problem hiding this comment.
I believe there's a useRouteMatch or something from tanstack router that does this a bit more safely
There was a problem hiding this comment.
Header was revertred.
| return sorted(tag.name for tag in self.affected_region_tags.all()) | ||
|
|
||
| @property | ||
| def total_downtime_display(self) -> str | None: |
There was a problem hiding this comment.
oh nice I like this approach versus formatting in frontend
src/firetower/incidents/views.py
Outdated
| _HISTORY_YEARS = 3 | ||
|
|
||
|
|
||
| def _format_downtime(minutes: int | None) -> str | None: |
There was a problem hiding this comment.
wait why do we need this defined twice?
There was a problem hiding this comment.
I only see one reference of this
| parts.append(f"{hours}h") | ||
| if mins > 0: | ||
| parts.append(f"{mins}m") | ||
| return " ".join(parts) |
There was a problem hiding this comment.
Duplicate downtime formatting logic
Medium Severity
The format_downtime function in reporting_utils.py duplicates the exact logic from the total_downtime_display property in models.py. Both convert minutes to hours/minutes format identically. This redundancy increases maintenance burden and risks inconsistent updates.
Additional Locations (1)
| (total_period_seconds - total_downtime_seconds) | ||
| / total_period_seconds | ||
| * 100, | ||
| ) |
There was a problem hiding this comment.
Incorrect availability calculation for current periods
High Severity
The availability percentage calculation divides by the full period length (period_end - period_start) but only counts incidents up to effective_end (min of period_end and now). For current incomplete periods, this inflates availability percentages by treating future time as zero-downtime uptime.
| min="0" | ||
| value={draftValue ?? ''} | ||
| onChange={e => | ||
| onChange(e.target.value === '' ? null : e.target.valueAsNumber) |
There was a problem hiding this comment.
NaN passes through onChange handler
Medium Severity
When users enter invalid numeric input (like decimals, negative numbers, or non-numeric text), e.target.valueAsNumber returns NaN, which gets passed to onChange. This causes draftValue to become NaN, breaking the input display and potentially corrupting the incident data if saved.
|
|
||
| export function RegionTable({periods, selectedIndex, activePeriod}: RegionTableProps) { | ||
| const [showIncidents, setShowIncidents] = useState(false); | ||
| const [windowStart, setWindowStart] = useState(0); |
There was a problem hiding this comment.
Window state persists across period switches
Medium Severity
The windowStart state persists when switching between month/quarter/year views. If users scroll to older months then switch to quarter view (which has fewer periods), windowStart may point beyond the available data, showing an empty or incorrect table window.
|
|
||
| export function RegionTable({periods, selectedIndex, activePeriod}: RegionTableProps) { | ||
| const [showIncidents, setShowIncidents] = useState(false); | ||
| const [windowStart, setWindowStart] = useState(0); |
There was a problem hiding this comment.
Selected period not visible when outside window
Medium Severity
When selectedIndex from URL parameters falls outside the visible window (determined by windowStart), users see regions sorted by the selected period's availability but cannot see that period's column in the table. For example, if selectedIndex=8 but windowStart=0, only periods 0-5 are visible while sorting uses period 8 data.
| id: z.number(), | ||
| title: z.string(), | ||
| created_at: z.string(), | ||
| total_downtime_minutes: z.number(), |
There was a problem hiding this comment.
Incident schema expects non-null downtime incorrectly
Low Severity
The IncidentSummarySchema defines total_downtime_minutes as z.number() (non-nullable), but incidents can have null downtime in the model. While the backend filters by total_downtime__isnull=False, this tight coupling creates fragility where a backend query change could break the frontend validation.
| "time_analyzed", | ||
| "time_mitigated", | ||
| "time_recovered", | ||
| "total_downtime", |
There was a problem hiding this comment.
Missing validation allows negative downtime values
Medium Severity
The total_downtime field in IncidentWriteSerializer has no validation to prevent negative values. Users can submit negative downtime via the API, which causes incorrect availability calculations (potentially >100%) and misleading display formatting that shows only the positive minutes portion of negative values.
| parts.append(f"{hours}h") | ||
| if mins > 0: | ||
| parts.append(f"{mins}m") | ||
| return " ".join(parts) |
There was a problem hiding this comment.
Duplicate downtime formatting logic added
Low Severity
Two separate implementations format downtime: Incident.total_downtime_display and format_downtime in reporting_utils.py. They duplicate the same rules and string output, increasing the risk of inconsistent formatting or bug fixes between incident detail and the new availability view.
Additional Locations (1)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| className="text-xs text-content-accent hover:underline font-medium shrink-0" | ||
| > | ||
| INC-{incident.id} | ||
| </Link> |
There was a problem hiding this comment.
Hardcoded incident key breaks routing
Medium Severity
The incident link builds params.incidentId with a hardcoded INC- prefix, but the backend expects {settings.PROJECT_KEY}-<number>. If PROJECT_KEY differs from INC, clicking incidents from the availability view will consistently hit the “invalid incident ID format” path.
| ) -> list[dict]: | ||
| effective_end = min(period_end, now) | ||
| total_period_seconds = (period_end - period_start).total_seconds() | ||
| regions = [] |
There was a problem hiding this comment.
Bug: The compute_regions function incorrectly calculates availability for current periods by dividing by the total period duration instead of the elapsed time, inflating the resulting percentage.
Severity: MEDIUM
Suggested Fix
In compute_regions, the denominator for the availability percentage calculation should be the elapsed time of the period, not its total duration. The divisor should be changed from total_period_seconds to (effective_end - period_start).total_seconds() to ensure the calculation reflects the availability for the portion of the period that has passed.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/firetower/incidents/reporting_utils.py#L140
Potential issue: In the `compute_regions` function, the availability calculation for
incomplete time periods is incorrect. While downtime is correctly calculated based on
the elapsed time within the period (capped by `effective_end`), the final percentage is
derived by dividing by the total duration of the entire period (`total_period_seconds`).
This results in an artificially inflated availability percentage for any currently
active reporting period (e.g., the current month or quarter). The frontend displays this
incorrect value as the actual, live availability, which can mislead users about the
system's true reliability.
2 participants
No description provided.