approvals: ship raw diff text instead of pre-rendered html; client classifies per-line
This commit is contained in:
parent
fb669c17c8
commit
d48cee7c2d
3 changed files with 29 additions and 37 deletions
1
TODO.md
1
TODO.md
|
|
@ -34,3 +34,4 @@
|
|||
|
||||
- ~~**Pending message wake-up**~~ ✓ fixed (e423d57) — subscribe-before-check race in `broker.recv_blocking` meant a send landing between the initial `recv()` and `subscribe()` was missed; agent then sat on the 180s long-poll until another, unrelated message woke it. Now subscribe first.
|
||||
- **Post-rebuild system-message missed wake**: at 09:13:14 the dashboard showed `system → damocles container rebuilt` as ✓ delivered, but the agent harness never ran a turn for it (no claude invocation, no operator-visible activity). A subsequent `recv()` from inside the agent returned `(empty)`, confirming the message was popped + marked delivered server-side — yet drove no turn. Most likely cause: the agent_server `serve_agent_stdio` task is up and answering MCP/socket calls, but the `hive-ag3nt::serve` long-poll loop that drives `drive_turn` either died silently during rebuild or never restarted. Investigate: (a) does hive-ag3nt's serve loop survive `nixos-container update` cleanly, or does its tokio runtime get torn down mid-loop? (b) is there an early-exit path on a transient socket error during rebuild that drops the serve task without notifying the manager? (c) compare timeline with manager's own post-rebuild wake to see if this is rebuilt-agents-only or universal. Could be related to the `recv_blocking` fix in `e423d57` if the rebuild restarts the broker mid-subscribe.
|
||||
- **`LiveEvent::Note(String)` never reaches the browser**: the enum is `#[serde(tag = "kind", rename_all = "snake_case")]` with `Note(String)` as a newtype variant — `serde_json::to_string` errors at runtime with `cannot serialize tagged newtype variant containing a string`. The SSE handler's `filter_map(... .ok()? ...)` silently drops the event; the sqlite history persists it as the literal string `"null"`. Every `bus.emit(LiveEvent::Note(...))` call site has been a no-op since the variant was added, and the JS terminal's `note` renderer is dead code. Fix: convert to a struct variant `Note { text: String }` (matches what the JS already reads via `ev.text`) and verify the existing call sites still type-check. While there, audit the sqlite-stored `"null"` rows so history-replay doesn't trip on them.
|
||||
|
|
|
|||
|
|
@ -736,14 +736,29 @@
|
|||
denyForm,
|
||||
);
|
||||
li.append(row);
|
||||
if (a.diff_html) {
|
||||
if (a.diff) {
|
||||
const details = el('details', {
|
||||
'data-restore-key': 'approval-diff:' + a.id,
|
||||
});
|
||||
details.append(el('summary', {}, 'diff vs applied'));
|
||||
// diff_html is pre-rendered server-side (per-line class spans inside
|
||||
// a <pre>); inject as innerHTML.
|
||||
const pre = el('pre', { class: 'diff', html: a.diff_html });
|
||||
// Server ships the raw unified diff; classify each line by its
|
||||
// leading char so `.diff-add` / `.diff-del` / `.diff-hunk` /
|
||||
// `.diff-file` / `.diff-ctx` colour the output. Building spans
|
||||
// here (instead of innerHTML-ing pre-rendered markup) keeps
|
||||
// the snapshot wire format text-only and one less HTML-escape
|
||||
// surface server-side.
|
||||
const pre = el('pre', { class: 'diff' });
|
||||
for (const raw of a.diff.split('\n')) {
|
||||
let cls = 'diff-ctx';
|
||||
if (raw.startsWith('--- ') || raw.startsWith('+++ ')) cls = 'diff-file';
|
||||
else if (raw.startsWith('@')) cls = 'diff-hunk';
|
||||
else if (raw.startsWith('+')) cls = 'diff-add';
|
||||
else if (raw.startsWith('-')) cls = 'diff-del';
|
||||
const span = document.createElement('span');
|
||||
span.className = cls;
|
||||
span.textContent = raw + '\n';
|
||||
pre.appendChild(span);
|
||||
}
|
||||
details.append(pre);
|
||||
li.append(details);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -3,7 +3,6 @@
|
|||
//! repo, plus approve/deny buttons), and the manager.
|
||||
|
||||
use std::convert::Infallible;
|
||||
use std::fmt::Write as _;
|
||||
use std::net::SocketAddr;
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
|
|
@ -256,8 +255,13 @@ struct ApprovalView {
|
|||
kind: &'static str,
|
||||
/// First 12 chars of the `commit_ref`, for `ApplyCommit` only.
|
||||
sha_short: Option<String>,
|
||||
/// Pre-rendered syntax-coloured diff HTML, for `ApplyCommit` only.
|
||||
diff_html: Option<String>,
|
||||
/// Raw unified diff text, for `ApplyCommit` only. The client splits
|
||||
/// on `\n` and per-line classifies (`+` / `-` / `@@` / `--- ` / `+++ `
|
||||
/// → diff-add / diff-del / diff-hunk / diff-file). Shipping raw
|
||||
/// instead of pre-rendered HTML saves bytes on the wire (no
|
||||
/// per-line `<span>` markup) and removes the only HTML-escape
|
||||
/// surface from the snapshot.
|
||||
diff: Option<String>,
|
||||
/// Manager-supplied description shown on the approval card.
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
description: Option<String>,
|
||||
|
|
@ -639,7 +643,7 @@ async fn build_approval_views(approvals: Vec<Approval>) -> Vec<ApprovalView> {
|
|||
agent: a.agent.clone(),
|
||||
kind: "apply_commit",
|
||||
sha_short: Some(sha),
|
||||
diff_html: Some(render_diff_lines(&diff)),
|
||||
diff: Some(diff),
|
||||
description: a.description,
|
||||
}
|
||||
}
|
||||
|
|
@ -648,7 +652,7 @@ async fn build_approval_views(approvals: Vec<Approval>) -> Vec<ApprovalView> {
|
|||
agent: a.agent,
|
||||
kind: "spawn",
|
||||
sha_short: None,
|
||||
diff_html: None,
|
||||
diff: None,
|
||||
description: a.description,
|
||||
},
|
||||
});
|
||||
|
|
@ -1345,29 +1349,6 @@ fn gc_orphans(coord: &Coordinator, approvals: Vec<Approval>) -> Vec<Approval> {
|
|||
.collect()
|
||||
}
|
||||
|
||||
/// Render a unified diff with per-line CSS classes so the dashboard can
|
||||
/// colour adds / dels / hunk headers / context. Each line becomes a
|
||||
/// `<span>` tagged by its leading character; the wrapping `<pre>` keeps
|
||||
/// whitespace intact.
|
||||
fn render_diff_lines(diff: &str) -> String {
|
||||
let mut out = String::new();
|
||||
for raw in diff.lines() {
|
||||
let cls = match raw.as_bytes().first() {
|
||||
// file headers (`--- a/...` / `+++ b/...`) come before any
|
||||
// line starting with a single `+`/`-`. similar-rs emits them
|
||||
// with the doubled prefix.
|
||||
_ if raw.starts_with("--- ") => "diff-file",
|
||||
_ if raw.starts_with("+++ ") => "diff-file",
|
||||
Some(b'@') => "diff-hunk",
|
||||
Some(b'+') => "diff-add",
|
||||
Some(b'-') => "diff-del",
|
||||
_ => "diff-ctx",
|
||||
};
|
||||
let _ = writeln!(out, "<span class=\"{cls}\">{}</span>", html_escape(raw),);
|
||||
}
|
||||
out
|
||||
}
|
||||
|
||||
/// Host-side mirror of `hive_ag3nt::login::has_session`. Returns true if the
|
||||
/// agent's bound `~/.claude/` dir on disk contains any regular file. The
|
||||
/// dashboard reads this each render so logins driven from the agent web UI
|
||||
|
|
@ -1415,8 +1396,3 @@ async fn git_diff_main_to(applied_dir: &Path, target_ref: &str) -> Result<String
|
|||
Ok(String::from_utf8_lossy(&out.stdout).into_owned())
|
||||
}
|
||||
|
||||
fn html_escape(s: &str) -> String {
|
||||
s.replace('&', "&")
|
||||
.replace('<', "<")
|
||||
.replace('>', ">")
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue