git ents: roots: render anchored comments inline with the file browser

crates/cli/ents-web/tests/router.rs @@ -132,6 +132,106 @@ .to_owned() } +/// Commit a further change to a file already tracked in `dir` -- what the +/// comment tests below use to move `HEAD` past a comment's own anchor +/// commit, so its projection has something to react to. +fn commit_change(dir: &std::path::Path, path: &str, contents: &str, message: &str) { + std::fs::write(dir.join(path), contents).expect("write fixture file"); + let git = |args: &[&str]| { + let status = std::process::Command::new("git") + .arg("-C") + .arg(dir) + .args(args) + .status() + .expect("git runs"); + assert!(status.success(), "git {args:?} failed"); + }; + git(&["add", "-A"]); + git(&[ + "-c", + "user.name=t", + "-c", + "user.email=t@example.com", + "commit", + "-q", + "-m", + message, + ]); +} + +/// Establish a session against `router` via a `GET` to `path`, returning +/// its cookie header and CSRF token -- the same extraction +/// `csrf_is_required_and_checked_on_every_state_changing_request` performs +/// inline, factored out here since every comment test below needs one. +async fn session_cookie_and_csrf( + router: &axum::Router, + state: &AppState<ObjectStore>, + path: &str, +) -> (String, String) { + let response = router + .clone() + .oneshot(Request::get(path).body(Body::empty()).expect("request")) + .await + .expect("in-process call"); + let cookie = response + .headers() + .get(header::SET_COOKIE) + .expect("a fresh GET always mints a session cookie") + .to_str() + .expect("ascii") + .to_owned(); + let session_id = cookie + .split(';') + .next() + .expect("at least one segment") + .split_once('=') + .expect("name=value") + .1 + .to_owned(); + let csrf = state + .sessions + .get(&session_id) + .expect("the session this cookie names is held in this server's own memory") + .csrf; + (cookie, csrf) +} + +/// `POST /comments`, anchoring `body` to `path` (`lines`, `<start>:<end>`) +/// at `rev` -- what the comment tests below seed a real comment through, +/// exercising the actual signed-write path (`ents_forge::comment::add`) +/// rather than poking the ref store directly. Asserts the write succeeded +/// (a redirect to the new comment's own page). +async fn seed_comment( + router: &axum::Router, + state: &AppState<ObjectStore>, + path: &str, + body: &str, + lines: &str, + rev: &str, +) { + let (cookie, csrf) = session_cookie_and_csrf(router, state, "/comments").await; + let form = format!( + "path={path}&body={}&lines={lines}&rev={rev}&csrf={csrf}", + body.replace(' ', "+") + ); + let response = router + .clone() + .oneshot( + Request::post("/comments") + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + .header(header::COOKIE, cookie) + .body(Body::from(form)) + .expect("request"), + ) + .await + .expect("in-process call"); + assert!( + response.status().is_redirection(), + "comment write did not succeed: {:?}", + response.status() + ); +} + /// `roots.local`: this crate's route table never exposes git's own /// smart-HTTP transport -- a request that would name it (`info/refs` with /// a `service` query, exactly the URL stock `git clone`/`git fetch` sends @@ -864,6 +964,174 @@ assert!(md_body.contains("<h1>Doc Title</h1>")); } +/// `GET /files/<path>` on a blob with no comments carries no comment-card +/// markup at all -- not even an empty section (`crate::pages::comments::comments_section`'s +/// own no-drop-but-no-empty-section contract). +#[tokio::test] +async fn files_blob_view_with_no_comments_has_no_comment_card_markup() { + let dir = seed_repo(&[("src/main.rs", "line 1\nline 2\nline 3\n")]); + let state = build_state_at( + FixtureIdentity { + name: "local-user", + key: Keypair::from_seed(1), + }, + dir.path().to_owned(), + ); + let router = ents_web::router(state); + + let response = router + .oneshot( + Request::get("/files/src/main.rs") + .body(Body::empty()) + .expect("request"), + ) + .await + .expect("in-process call"); + assert_eq!(response.status(), StatusCode::OK); + let body = response + .into_body() + .collect() + .await + .expect("body") + .to_bytes(); + let body = String::from_utf8(body.to_vec()).expect("utf8 html"); + assert!(!body.contains("file-comments")); + assert!(!body.contains("comment-meta")); +} + +/// A blob view shows every comment anchored to it: author, body (rendered +/// as AsciiDoc), and its projected line range linking into the blob's own +/// `#L<n>` gutter. +#[tokio::test] +async fn files_blob_view_shows_a_seeded_comments_body_and_author() { + let dir = seed_repo(&[("src/main.rs", "line 1\nline 2\nline 3\n")]); + let state = build_state_at( + FixtureIdentity { + name: "commenter", + key: Keypair::from_seed(1), + }, + dir.path().to_owned(), + ); + let router = ents_web::router(state.clone()); + seed_comment( + &router, + &state, + "src/main.rs", + "worth a look here", + "2:2", + "HEAD", + ) + .await; + + let response = router + .oneshot( + Request::get("/files/src/main.rs") + .body(Body::empty()) + .expect("request"), + ) + .await + .expect("in-process call"); + assert_eq!(response.status(), StatusCode::OK); + let body = response + .into_body() + .collect() + .await + .expect("body") + .to_bytes(); + let body = String::from_utf8(body.to_vec()).expect("utf8 html"); + assert!(body.contains("id=\"file-comments\"")); + assert!(body.contains("worth a look here")); + assert!(body.contains("commenter")); + assert!(body.contains("href=\"#L2\"")); + assert!(!body.contains("class=\"outdated\"")); +} + +/// A comment whose anchored lines were since edited still renders (never +/// dropped), flagged with the muted `outdated` marker instead of a line +/// link (`ents_anchor::Projection::Outdated`). +#[tokio::test] +async fn files_blob_view_marks_an_outdated_comment() { + let dir = seed_repo(&[("src/main.rs", "line 1\nline 2\nline 3\n")]); + let state = build_state_at( + FixtureIdentity { + name: "commenter", + key: Keypair::from_seed(1), + }, + dir.path().to_owned(), + ); + let router = ents_web::router(state.clone()); + seed_comment( + &router, + &state, + "src/main.rs", + "line two looks off", + "2:2", + "HEAD", + ) + .await; + // Edit exactly the anchored line, so the projection can no longer map + // it -- `ents_anchor::project`'s own `Outdated` case. + commit_change( + dir.path(), + "src/main.rs", + "line 1\nsomething else entirely\nline 3\n", + "edit line two", + ); + + let response = router + .oneshot( + Request::get("/files/src/main.rs") + .body(Body::empty()) + .expect("request"), + ) + .await + .expect("in-process call"); + assert_eq!(response.status(), StatusCode::OK); + let body = response + .into_body() + .collect() + .await + .expect("body") + .to_bytes(); + let body = String::from_utf8(body.to_vec()).expect("utf8 html"); + assert!( + body.contains("line two looks off"), + "comment is never dropped" + ); + assert!(body.contains("class=\"outdated\"")); +} + +/// `GET /comments?file=<path>&lines=<range>` pre-fills the add-comment +/// form's `path`/`lines` fields -- the entry point `crate::pages::files`'s +/// own "comment on this file" link uses. +#[tokio::test] +async fn comments_list_prefills_the_add_form_from_query_params() { + let state = build_state(FixtureIdentity { + name: "local-user", + key: Keypair::from_seed(1), + }); + let router = ents_web::router(state); + + let response = router + .oneshot( + Request::get("/comments?file=src/main.rs&lines=1-2") + .body(Body::empty()) + .expect("request"), + ) + .await + .expect("in-process call"); + assert_eq!(response.status(), StatusCode::OK); + let body = response + .into_body() + .collect() + .await + .expect("body") + .to_bytes(); + let body = String::from_utf8(body.to_vec()).expect("utf8 html"); + assert!(body.contains(r#"name="path" value="src/main.rs""#)); + assert!(body.contains(r#"name="lines" value="1-2""#)); +} + /// `GET /commits` lists the repository's commit history: the seeded /// commit's own short id appears, linking into `/commit/{oid}`. #[tokio::test]

crates/cli/ents-web/src/assets/ents.css @@ -14,7 +14,9 @@ * pre-redo sheet: `.meta-layout`/`.meta-rail` for the `meta` tab's * page-family rail (`crate::pages::META_SECTIONS`) and `.id-chip` for the * header's signing-identity link, both new to this crate's own four-tab - * restructure. + * restructure; and `.comment-meta`/`.outdated` for the file-anchored + * comment cards `crate::pages::comments::comments_section` renders below a + * blob view, which otherwise reuse `.card`/`.doc-body` as-is. */ :root { --font-sans: system-ui, -apple-system, "Segoe UI", sans-serif; @@ -250,6 +252,14 @@ .blob-code code { display: block; font-family: inherit; padding: 0 1.25rem; white-space: pre; color: var(--color-text); } .binary { padding: 2.5rem; text-align: center; font-family: var(--font-mono); font-size: .85rem; color: var(--color-text-muted); } +/* File-anchored comment cards (`crate::pages::comments::comments_section`), + * below a blob view in `crate::pages::files` -- each comment is its own + * `.card`, its body reusing `.doc-body`'s prose styling (a comment body + * renders as AsciiDoc, same as a rendered document blob). */ +.comment-meta { display: flex; flex-wrap: wrap; align-items: center; gap: .5rem; padding: .7rem 1.1rem; font-size: .82rem; color: var(--color-text-muted); border-bottom: 1px solid var(--color-border); } +.comment-meta .author { color: var(--color-text); font-weight: 600; } +.outdated { color: var(--color-text-muted); font-style: italic; } + /* Syntax-highlight token classes (`crate::pages::files::highlight`, `arborium`'s `HtmlFormat::ClassNames`). */ .code .keyword, .code .macro, .code .tag { color: var(--s-keyword); } .code .function, .code .constructor { color: var(--s-func); }

crates/cli/ents-web/src/pages/comments.rs @@ -5,22 +5,44 @@ //! kind of domain-specific view `ents-forge`'s own `comment::show` //! already returns structured data for, rather than a bare reflected //! field list. +//! +//! [`for_path`]/[`comments_section`] are this module's second entry +//! point: `crate::pages::files`'s blob view calls them to render the +//! comments anchored to the file it is showing, rather than duplicating +//! this module's own read-project-render pattern. use std::sync::Arc; use axum::Form; use axum::extract::{Path, Query as PathQuery, State}; use axum::response::{IntoResponse, Redirect}; +use ents_anchor::{Anchor, LineRange, Projection}; use ents_forge::comment; use gix_object::{Find, Write}; -use maud::html; +use maud::{Markup, html}; use serde::Deserialize; use crate::error::Result; use crate::session::Session; use crate::state::AppState; -/// `GET /comments`. +/// The query parameters `GET /comments` accepts: `file`/`lines` prefill +/// the add-comment form (e.g. a link from `crate::pages::files`'s "comment +/// on this file"), rather than changing what the page lists. Both default +/// to empty, which [`add_form`] renders as an unfilled field -- an absent +/// or nonsensical `lines` value (it is never parsed here, only echoed +/// back into the form) is exactly as inert as an absent one. +#[derive(Debug, Deserialize, Default)] +pub struct ListQuery { + /// Pre-fills the add form's `path` field. + #[serde(default)] + file: String, + /// Pre-fills the add form's `lines` field. + #[serde(default)] + lines: String, +} + +/// `GET /comments?file=<path>&lines=<range>`. /// /// # Errors /// @@ -28,6 +50,7 @@ pub async fn list<O>( State(state): State<Arc<AppState<O>>>, axum::Extension(session): axum::Extension<Session>, + PathQuery(query): PathQuery<ListQuery>, ) -> Result<maud::Markup> where O: Find + Write + Send + 'static, @@ -45,7 +68,7 @@ } } h2 { "add a comment" } - (add_form("HEAD", &session)) + (add_form("HEAD", &session, &query.file, &query.lines)) }, )) } @@ -154,15 +177,155 @@ Ok(Redirect::to(&format!("/comments/{id}"))) } -fn add_form(default_rev: &str, session: &Session) -> maud::Markup { +/// The add-comment form, its `path`/`lines` fields pre-filled from +/// [`ListQuery`] when `list` was reached with `?file=`/`?lines=` (e.g. +/// `crate::pages::files`'s "comment on this file" link) -- maud escapes +/// both into the `value` attribute the same as any other interpolation, +/// so neither can break out of the form markup, and an empty prefill +/// renders exactly as the unfilled field always did. +fn add_form( + default_rev: &str, + session: &Session, + prefill_path: &str, + prefill_lines: &str, +) -> maud::Markup { html! { form method="post" action="/comments" { (super::csrf_input(session)) - label { "path" input type="text" name="path"; } + label { "path" input type="text" name="path" value=(prefill_path); } label { "rev" input type="text" name="rev" value=(default_rev); } - label { "lines" input type="text" name="lines"; } + label { "lines" input type="text" name="lines" value=(prefill_lines); } label { "body" textarea name="body" {} } button type="submit" { "comment" } } } } + +/// One comment as `crate::pages::files`'s blob view shows it: who wrote it +/// and when ([`super::ago`]), where its anchor lands on the displayed +/// file (a line-range link into the blob's own `#L<n>` gutter, or the +/// muted `outdated` marker when [`ents_anchor::project`] can no longer map +/// the anchored lines), and its body rendered as AsciiDoc +/// ([`crate::asciidoc`], this crate's default prose treatment for text +/// with no filename of its own to infer a MIME type from). Mirrors +/// `pre-redo:crates/git-ents-server/src/web/pages.rs`'s own `FileComment`, +/// salvaged per this crate's PORT-and-reverify policy: author/timestamp +/// there came from `git_comment::provenance`'s shell-out, here from +/// [`super::commit_authorship`] reading the comment ref's own tip commit +/// through `gix_object::Find`. +pub(crate) struct FileComment { + /// The comment ref's own tip commit's author display name + /// (`model.comment`: a comment stores no author field of its own). + pub(crate) author: String, + /// [`super::ago`] renders this against the current time. + pub(crate) seconds: i64, + /// The anchored range as it lands on the displayed file at `HEAD`, or + /// `None` for a whole-file anchor or an outdated projection. + pub(crate) lines: Option<LineRange>, + /// Set when [`ents_anchor::project`] reports + /// [`Projection::Outdated`]: the anchored lines themselves were + /// edited, so no line link is shown, only the marker -- the comment + /// itself is never dropped from the page. + pub(crate) outdated: bool, + /// The body, rendered as AsciiDoc ([`crate::asciidoc::to_html`]), + /// falling back to escaped plain text on a render failure -- a file + /// view degrades, it never 500s over one unparsable comment. + pub(crate) body: Markup, +} + +/// Every comment whose anchor projects onto `path` at `HEAD` in `repo` -- +/// [`crate::pages::files`]'s own read of this domain, built on the same +/// [`comment::list`] read [`list`] itself uses and the same +/// [`ents_anchor::project`] call [`show`] itself uses, rather than a third +/// way to read a comment. Best effort throughout: a comment whose anchor +/// or body fails to read, parse, or project is skipped from this file's +/// own view only -- it still shows up on `GET /comments` and its own `GET +/// /comments/{id}` page -- and a projection landing anywhere other than +/// `path` (moved elsewhere, or deleted) is likewise not this file's +/// comment to show. A projection that still lands at `path` but comes +/// back [`Projection::Outdated`] is the one case this function keeps and +/// flags (`outdated: true`) rather than skips: the anchored lines +/// changed, not the comment's relevance to this file. +pub(crate) fn for_path<O: Find + Write>( + state: &AppState<O>, + repo: &gix::Repository, + path: &str, +) -> Vec<FileComment> { + let Ok(rows) = comment::list(state.refs.as_ref(), &*state.objects()) else { + return Vec::new(); + }; + let mut out = Vec::new(); + for (id, comment) in rows { + let Ok(anchor) = + facet_git_tree::deserialize::<Anchor>(&comment.anchor.oid(), &*state.objects()) + else { + continue; + }; + let Ok(projection) = ents_anchor::project(repo, &anchor, "HEAD") else { + continue; + }; + let (landed, lines, outdated) = match projection { + Projection::Current => (anchor.path.clone(), anchor.lines, false), + Projection::Relocated { path, lines } => (path, lines, false), + Projection::Outdated { path } => (path, None, true), + Projection::Deleted => continue, + }; + if landed != path { + continue; + } + let Ok(ref_name) = ents_model::namespace::comment_ref(&id) else { + continue; + }; + let Some(tip) = state.refs.get(ref_name.as_ref()).ok().flatten() else { + continue; + }; + let Ok((author, seconds)) = super::commit_authorship(&*state.objects(), tip) else { + continue; + }; + let body = crate::asciidoc::to_html(&comment.body) + .unwrap_or_else(|_| html! { p { (comment.body) } }); + out.push(FileComment { + author, + seconds, + lines, + outdated, + body, + }); + } + out +} + +/// The comment cards under a blob view, one [`FileComment`] per +/// [`maud`]-rendered `.card`, mounted at `#file-comments` so +/// `crate::pages::files`'s own "N comments" link can jump straight to +/// them. Renders nothing at all -- not even an empty container -- when +/// `comments` is empty, so a file with no comments carries no extra +/// markup (`crate::pages::files`'s own blob view calls this +/// unconditionally rather than checking first). +pub(crate) fn comments_section(comments: &[FileComment]) -> Markup { + if comments.is_empty() { + return html! {}; + } + html! { + div id="file-comments" { + @for comment in comments { + div.card { + div.comment-meta { + span.author { (comment.author) } + span { (super::ago(comment.seconds)) } + @if let Some(range) = comment.lines { + a href={ "#L" (range.start) } { + @if range.start == range.end { "line " (range.start) } + @else { "lines " (range.start) "-" (range.end) } + } + } + @if comment.outdated { + span.outdated { "outdated" } + } + } + div.doc-body { (comment.body) } + } + } + } + } +}

crates/cli/ents-web/src/pages/files.rs @@ -16,6 +16,13 @@ //! pages use to read typed meta-ref entities (`facet-git-tree` is for //! structured meta-ref data; browsing arbitrary repository content is not //! that). +//! +//! A blob view also loads and renders the comments anchored to it +//! (`crate::pages::comments::for_path`/`comments_section`), below the blob +//! itself, and [`crumbs`] grows a "comment on this file" link (plus a +//! jump to those cards, once there is at least one) beside its own +//! trailing "history" link -- a directory listing carries neither, since +//! a comment anchors to a file, never a tree. use std::sync::Arc; @@ -61,7 +68,10 @@ /// The shared implementation behind [`root`] and [`show`]: resolve `path` /// against `HEAD`'s tree and render whichever of a directory listing or a /// blob view it names. -fn at<O>(state: &AppState<O>, path: &str) -> Result<Markup> { +fn at<O>(state: &AppState<O>, path: &str) -> Result<Markup> +where + O: Find + Write, +{ if !is_safe_path(path) { return Err(Error::NotFound { what: path.to_owned(), @@ -83,7 +93,7 @@ super::Tab::Files, "files", html! { - (crumbs(path)) + (crumbs(path, None)) (dir_listing(path, Vec::new())) }, )); @@ -103,7 +113,7 @@ super::Tab::Files, "files", html! { - (crumbs(path)) + (crumbs(path, None)) (dir_listing(path, entries)) }, )); @@ -129,7 +139,7 @@ super::Tab::Files, path, html! { - (crumbs(path)) + (crumbs(path, None)) (dir_listing(path, entries)) }, )) @@ -140,14 +150,16 @@ .try_into_blob() .map_err(|source| Error::Repo(source.to_string()))?; let name = path.rsplit('/').next().unwrap_or(path); + let comments = super::comments::for_path(state, &repo, path); Ok(super::layout( &super::RepoHeader::from_state(state), &super::identity_label(state), super::Tab::Files, path, html! { - (crumbs(path)) + (crumbs(path, Some(comments.len()))) (blob_view(name, &blob.data)?) + (super::comments::comments_section(&comments)) }, )) } else { @@ -216,11 +228,17 @@ } /// Breadcrumb navigation from the repository's files root down through -/// `path`, `chevron-right` icons separating segments, plus a trailing -/// link into `crate::pages::commits`'s `GET /commits` history -- the file +/// `path`, `chevron-right` icons separating segments, plus trailing links +/// into `crate::pages::commits`'s `GET /commits` history (the file /// browser's one entry point into commit history, since history is a view -/// of the code, not a tab of its own (`crate::pages::mod`'s own doc). -fn crumbs(path: &str) -> Markup { +/// of the code, not a tab of its own -- `crate::pages::mod`'s own doc) and, +/// on a blob view (`comments` is `Some`), `crate::pages::comments`'s own +/// add form for this file ("comment on this file") plus a jump straight to +/// [`super::comments::comments_section`]'s cards when there is at least +/// one comment already anchored here. +/// `comments` is `None` on a directory listing, where neither link makes +/// sense. +fn crumbs(path: &str, comments: Option<usize>) -> Markup { let parts: Vec<&str> = path.split('/').filter(|s| !s.is_empty()).collect(); let mut acc = String::new(); let mut trail: Vec<(String, Option<String>)> = @@ -244,6 +262,14 @@ } } a.crumbs-history href="/commits" { "history" } + @if let Some(count) = comments { + a.crumbs-history href={ "/comments?file=" (path) } { "comment on this file" } + @if count > 0 { + a.crumbs-history href="#file-comments" { + (count) @if count == 1 { " comment" } @else { " comments" } + } + } + } } } }

crates/cli/ents-web/src/pages/mod.rs @@ -64,6 +64,42 @@ Ok(commit.tree()) } +/// The commit author's display name and commit time (epoch seconds) for +/// the commit at `oid` -- the meta-ref counterpart to +/// `crate::pages::commits`'s identical read of an ordinary history +/// commit, shared by any page that needs to know who mutated a meta-ref +/// entity and when rather than a stored field (`model.comment`'s own rule +/// that authorship lives in the commit chain, not the entity: see +/// `ents_forge::comment::Comment`'s own doc). +/// +/// A second, independent fetch-and-parse from [`commit_tree`]'s own +/// (same file, same pattern) rather than a shared parse step: `CommitRef` +/// borrows from a caller-owned buffer, so factoring the parse out would +/// need either an owned copy or a callback -- this module's own doc on +/// [`commit_tree`] already names three such near-identical copies as the +/// accepted pattern here. +pub(crate) fn commit_authorship(objects: &impl Find, oid: ObjectId) -> Result<(String, i64)> { + let mut buf = Vec::new(); + let data = objects + .try_find(&oid, &mut buf) + .map_err(|source| Error::InvalidArgument(source.to_string()))? + .ok_or_else(|| Error::NotFound { + what: oid.to_string(), + })?; + if data.kind != Kind::Commit { + return Err(Error::NotFound { + what: oid.to_string(), + }); + } + let commit = CommitRef::from_bytes(data.data, oid.kind()) + .map_err(|source| Error::InvalidArgument(source.to_string()))?; + let author = commit + .author() + .map_err(|source| Error::InvalidArgument(source.to_string()))?; + let seconds = author.time().map(|time| time.seconds).unwrap_or(0); + Ok((author.name.to_str_lossy().into_owned(), seconds)) +} + /// The tab-nav page families this crate exposes -- one variant per tab in /// [`layout`]'s nav bar, so a handler can name which tab it renders behind /// without `layout` re-deriving it from the request path (mirrors

roots: render anchored comments inline with the file browser

Reviews

Start a review