fix: use try_from for i64/u64 casts; split format_notification into helpers

This commit is contained in:
damocles 2026-05-22 19:09:03 +02:00
parent 484cea62c7
commit bbe2112dc9
3 changed files with 168 additions and 141 deletions

View file

@ -211,7 +211,6 @@ fn review_state_label(state: &str) -> Option<&str> {
///
/// Number is extracted from `html_url` last path segment before any `#`.
/// Repo slug (`owner/name`) is always included — agents may watch multiple repos.
#[allow(clippy::too_many_lines)] // multiple notification types handled in one place by design
async fn format_notification(
client: &reqwest::Client,
token: &str,
@ -256,11 +255,37 @@ async fn format_notification(
};
let is_pr = matches!(notif_type, "Pull Request" | "Pull");
let meta_suffix = build_meta_suffix(subject.as_ref(), is_pr);
// Build assignee + reviewer suffix appended to all notification kinds.
let meta_suffix = {
// Determine whether this notification was triggered by a comment/review or
// by creation/state-change of the subject itself.
let has_comment = !comment_api_url.is_empty() && comment_api_url != subject_api_url;
let meta = NotifMeta { title, notif_type, html_url, num, repo, meta_suffix, subject, is_pr };
if has_comment {
format_comment_notification(client, token, &meta, comment_api_url, comment_html_url, own_login).await
} else {
format_state_change_notification(notif, &meta, own_login)
}
}
/// Shared notification metadata extracted from the raw Forgejo JSON.
struct NotifMeta<'a> {
title: &'a str,
notif_type: &'a str,
html_url: &'a str,
num: String,
repo: String,
meta_suffix: String,
/// Fetched subject detail (issue/PR JSON); used for review-request detection.
subject: Option<serde_json::Value>,
is_pr: bool,
}
/// Build the `\nassignee: ...` (and optionally `\nreviewer: ...`) suffix
/// appended to all notification kinds.
fn build_meta_suffix(subject: Option<&serde_json::Value>, is_pr: bool) -> String {
let assignees: Vec<&str> = subject
.as_ref()
.and_then(|s| s["assignees"].as_array())
.map(|arr| arr.iter().filter_map(|a| a["login"].as_str()).collect())
.unwrap_or_default();
@ -272,15 +297,10 @@ async fn format_notification(
// For PRs, include requested_reviewers when present.
let reviewer_line = if is_pr {
let reviewers: Vec<&str> = subject
.as_ref()
.and_then(|s| s["requested_reviewers"].as_array())
.map(|arr| arr.iter().filter_map(|r| r["login"].as_str()).collect())
.unwrap_or_default();
if reviewers.is_empty() {
None
} else {
Some(format!("reviewer: {}", reviewers.join(", ")))
}
if reviewers.is_empty() { None } else { Some(format!("reviewer: {}", reviewers.join(", "))) }
} else {
None
};
@ -288,14 +308,17 @@ async fn format_notification(
Some(r) => format!("\n{assignee_line}\n{r}"),
None => format!("\n{assignee_line}"),
}
};
}
// Determine whether this notification was triggered by a comment/review or
// by creation/state-change of the subject itself.
let has_comment = !comment_api_url.is_empty() && comment_api_url != subject_api_url;
if has_comment {
// Notification triggered by a new comment or review submission.
/// Format a notification triggered by a new comment or review submission.
async fn format_comment_notification(
client: &reqwest::Client,
token: &str,
meta: &NotifMeta<'_>,
comment_api_url: &str,
comment_html_url: &str,
own_login: &str,
) -> Option<String> {
let payload = fetch_json(client, comment_api_url, token).await;
let actor_login = payload
@ -324,8 +347,9 @@ async fn format_notification(
.and_then(|c| c["state"].as_str())
.and_then(review_state_label);
let url = if comment_html_url.is_empty() { html_url } else { comment_html_url };
let url = if comment_html_url.is_empty() { meta.html_url } else { comment_html_url };
let author = if actor_login.is_empty() { "?" } else { actor_login };
let NotifMeta { title, notif_type, num, repo, meta_suffix, .. } = meta;
if let Some(review_label) = review_state {
// Review submission on a PR.
@ -336,7 +360,7 @@ async fn format_notification(
} else {
write!(out, "\n\n{author}: {}", truncate(body_text, BODY_TRUNCATE)).ok();
}
out.push_str(&meta_suffix);
out.push_str(meta_suffix);
Some(out)
} else {
// Regular comment.
@ -348,12 +372,17 @@ async fn format_notification(
if out.ends_with('\n') {
out.pop();
}
out.push_str(&meta_suffix);
out.push_str(meta_suffix);
Some(out)
}
} else {
// Notification triggered by creation or state change of the subject.
//
}
/// Format a notification triggered by creation or state change of the subject.
fn format_state_change_notification(
notif: &serde_json::Value,
meta: &NotifMeta<'_>,
own_login: &str,
) -> Option<String> {
// Classification uses notif["subject"]["state"] directly — Forgejo
// returns "open" / "closed" / "merged" here. We do NOT rely on
// fetching the PR/issue detail for `merged`:
@ -373,6 +402,7 @@ async fn format_notification(
return None;
}
let NotifMeta { title, notif_type, html_url, num, repo, meta_suffix, subject, is_pr } = meta;
let label = notif_type_label(notif_type);
let kind = match notif_state {
"merged" => format!("{label} merged{num}{repo}"),
@ -382,19 +412,18 @@ async fn format_notification(
};
// Review-request detection (#253): Forgejo does not always set
// reason == "review_requested" (observed reason is null). Check
// reason == "review_requested" (observed as null). Check
// requested_reviewers instead, which is reliable. If own_login is
// in the list, this is a review request -- override the kind.
// `subject` and `is_pr` are already fetched unconditionally above (#256).
// in the list, override the kind.
// subject and is_pr are already fetched unconditionally above (#256).
let is_review_request = is_new
&& is_pr
&& *is_pr
&& !own_login.is_empty()
&& subject
.as_ref()
.and_then(|s| s["requested_reviewers"].as_array())
.map(|arr| arr.iter().any(|r| r["login"].as_str() == Some(own_login)))
.unwrap_or(false);
let kind = if is_review_request {
format!("review requested{num}{repo}")
} else {
@ -402,9 +431,8 @@ async fn format_notification(
};
let mut out = format!("[{kind}] {title}\nurl: {html_url}");
out.push_str(&meta_suffix);
out.push_str(meta_suffix);
Some(out)
}
}
#[allow(clippy::too_many_arguments)]

View file

@ -335,8 +335,7 @@ async fn api_stats(
// Pass the window span to the reminder-stats RPC so the broker
// filters its counts to the same time range as the chart data.
let window_secs = window.span_secs();
#[allow(clippy::cast_sign_loss)] // window span is always a positive duration
let window_secs_u = window_secs.max(0) as u64;
let window_secs_u = u64::try_from(window_secs).unwrap_or(0);
snapshot.reminder_stats = fetch_reminder_stats(&state.socket, state.flavor(), window_secs_u).await;
axum::Json(snapshot)
}

View file

@ -609,13 +609,13 @@ impl Broker {
/// in the last `since_secs` seconds (0 = all reminders).
pub fn reminder_rollup_for(&self, agent: &str, since_secs: u64) -> Result<hive_sh4re::ReminderStats> {
let conn = self.conn.lock().unwrap();
#[allow(clippy::cast_possible_wrap)] // unix epoch secs fit in i64 until year 292B
let cutoff_time = if since_secs > 0 {
let now = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.ok()
.map_or(0, |d| d.as_secs() as i64);
now - since_secs as i64
.and_then(|d| i64::try_from(d.as_secs()).ok())
.unwrap_or(0);
now.saturating_sub(i64::try_from(since_secs).unwrap_or(i64::MAX))
} else {
i64::MIN
};