diff --git a/TODO.md b/TODO.md index 9d6f0fb..002723d 100644 --- a/TODO.md +++ b/TODO.md @@ -1383,10 +1383,23 @@ and borrows `pr`'s conventions rather than inventing new ones beside them. also owns the repo, and even then a *collaborator's* close is invisible — the same gap the `pr close` entry above records, needing the same knot ACL query to shut -- [ ] Comments are written and never *shown*. Counting them is done — see - above — but `issue view` still displays none, which is the cross-author - problem in its harder form: a count rides in a listing envelope, the - bodies do not, and each is a record in its commenter's PDS +- [x] Comments are written and now read. Both halves were the cross-author + problem — a comment is a record in its commenter's PDS and nothing + enumerates who has commented — and both have the same answer: + `sh.tangled.feed.listComments`, which takes the *subject's* at-uri and + so serves a pull and an issue with one function. `issue view + --comments` and `pr view --comments` print the discussion, oldest + first, and `--json` carries it as `thread` (not `comments`, which on a + pull is already the count). It is an index read, so it is opt-in and + says what it is: "as far as Bobbin has indexed". + + Two collections are read, not one. Tangled unified comments into + `sh.tangled.feed.comment` and the old `sh.tangled.repo.issue.comment` + is deprecated *for writing*; the index returns whatever was written, so + a thread older than the unification is mostly legacy records and a + reader that knew only the current shape would show it as empty. They + also spell their body differently — `body.text` against `body` — which + is what `Comment::read` hides - [x] No image blobs in issue bodies, though the lexicon has `blobs` and `pr create` already knew how to upload them. `issue create`, `issue edit` and `issue comment` now take the same path as their `pr` twins: diff --git a/src/clients/tangled/bobbin.rs b/src/clients/tangled/bobbin.rs index 340c74c..a7ec51e 100644 --- a/src/clients/tangled/bobbin.rs +++ b/src/clients/tangled/bobbin.rs @@ -143,6 +143,24 @@ pub async fn issues(endpoint: &str, subject: &str, limit: u32) -> Result Result> { + let url = method_url( + "sh.tangled.feed.listComments", + &[("subject", subject_uri), ("limit", &limit.to_string())], + )?; + Ok(envelope_items(&fetch(&url).await?)) +} + /// The `at:///sh.tangled.repo/` uri of a repo, by its DID. /// /// `None` for every kind of failure, because every kind means the same thing @@ -262,8 +280,18 @@ pub struct SearchPage { /// something that is not a URL — worth saying plainly, since the variable is /// the one part of this address a user sets. fn search_url(pairs: &[(&str, &str)]) -> Result { + method_url(SEARCH, pairs) +} + +/// The same, for any method: base URL, `/xrpc/`, and the pairs +/// encoded by a URL type rather than by formatting. +/// +/// Shared with the comment listing, whose `subject` is an at-uri — `at://` +/// and the slashes in it are exactly the characters a hand-pasted query +/// string gets wrong. +fn method_url(method: &str, pairs: &[(&str, &str)]) -> Result { let base = base(); - let mut url = reqwest::Url::parse(&format!("{base}/xrpc/{SEARCH}")) + let mut url = reqwest::Url::parse(&format!("{base}/xrpc/{method}")) .with_context(|| format!("{base:?} is not a URL (ATGC_BOBBIN)"))?; url.query_pairs_mut().extend_pairs(pairs); Ok(url.into()) diff --git a/src/clients/tangled/comments.rs b/src/clients/tangled/comments.rs new file mode 100644 index 0000000..81ebbad --- /dev/null +++ b/src/clients/tangled/comments.rs @@ -0,0 +1,153 @@ +//! Comments on a pull or an issue, read from the appview's index. +//! +//! A comment is a record in its *commenter's* PDS, naming the thing it +//! replies to. Nothing enumerates the accounts that have commented on +//! something, so there is no way to read a thread out of the PDSes it lives +//! in — the same shape as "every issue on this repo", and with the same +//! answer: `sh.tangled.feed.listComments`, which takes the subject's at-uri +//! and hands back the records whoever wrote them. +//! +//! That makes this an index read, so it inherits the index's terms: opt-in, +//! and possibly behind. A thread read here is "what Bobbin has seen", which +//! is why the callers say so rather than presenting it as the thread. +//! +//! # Two collections, one thread +//! +//! Tangled unified issue, pull and string comments into +//! `sh.tangled.feed.comment`, and +//! [`crate::lexicon::tangled::LEGACY_ISSUE_COMMENT_NSID`] is deprecated for +//! *writing*. Reading is the other direction: the index returns whatever was +//! written, and a thread opened before the unification is mostly legacy +//! records. Both spellings are read here, because a reader that understood +//! only the current one would show a two-year-old discussion as empty. +//! +//! The two also spell their body differently — the legacy record nests it +//! under `body.text` and the current one under `body` as a markdown object — +//! which is what [`Comment::body`] exists to hide. + +use anyhow::Result; + +/// One comment, in the shape a renderer wants rather than the shape it +/// arrived in. +#[derive(Debug, PartialEq)] +pub struct Comment { + /// The record's own at-uri; its authority is the commenter. + pub uri: String, + /// The commenter's DID, off the at-uri. + pub author_did: String, + /// RFC 3339 as the record spells it, which is the writer's own offset + /// and not necessarily UTC. + pub created_at: String, + pub body: String, +} + +impl Comment { + /// The commenter's DID, off the at-uri's authority. + fn author_of(uri: &str) -> String { + uri.strip_prefix("at://") + .and_then(|rest| rest.split('/').next()) + .unwrap_or_default() + .to_string() + } + + /// Read one item of the listing, or `None` for one with nothing to show. + /// + /// A comment with no body is not rendered at all rather than rendered + /// empty: the appview refuses one at ingest (`body is empty after HTML + /// sanitization`), so anything reaching here without one is a record + /// shape this build does not understand, and an empty box under + /// somebody's name says less than nothing. + fn read(item: &serde_json::Value) -> Option { + let uri = item["uri"].as_str()?.to_string(); + let value = &item["value"]; + // `body.text` is the legacy `sh.tangled.repo.issue.comment` shape; + // a bare string is `sh.tangled.feed.comment`'s markdown object seen + // through serde, whose own `text` is the same field one level in. + let body = value["body"]["text"] + .as_str() + .or_else(|| value["body"].as_str())? + .trim(); + if body.is_empty() { + return None; + } + Some(Comment { + author_did: Self::author_of(&uri), + created_at: value["createdAt"].as_str().unwrap_or_default().to_string(), + body: body.to_string(), + uri, + }) + } +} + +/// Every comment the index holds on `subject`, oldest first. +/// +/// Oldest first because that is the order a discussion happened in and the +/// order every forge shows one; the index's own order is not promised, so it +/// is sorted here rather than assumed. +/// +/// Sorted on the parsed instant, not the string: these records come from as +/// many PDSes as there are commenters, so `...Z` and `...+03:00` sit side by +/// side and do not sort lexically against each other within a day. The same +/// hazard `cmd::pr::read`'s `newest` and `Listed::instant` document, arriving +/// where it is most visible — a thread shown out of order reads as people +/// answering questions nobody asked yet. +pub async fn of(subject_uri: &str, limit: u32) -> Result> { + let items = crate::clients::tangled::bobbin::comments(subject_uri, limit).await?; + let mut out: Vec = items.iter().filter_map(Comment::read).collect(); + out.sort_by(|a, b| { + use jacquard::types::string::Datetime; + use std::str::FromStr; + Datetime::from_str(&a.created_at) + .ok() + .cmp(&Datetime::from_str(&b.created_at).ok()) + // An unparseable stamp sorts below everything dated; the at-uri + // breaks the remaining tie so a thread reads the same way twice. + .then_with(|| a.uri.cmp(&b.uri)) + }); + Ok(out) +} + +#[cfg(test)] +mod tests { + use super::Comment; + use serde_json::json; + + /// Both collections' body spellings, because a thread older than the + /// unification is mostly the legacy one and a reader that knew only the + /// current shape would show it as empty. + #[test] + fn both_comment_collections_are_read() { + let legacy = json!({ + "uri": "at://did:plc:them/sh.tangled.repo.issue.comment/1", + "value": { "body": { "text": "the old shape" }, "createdAt": "2026-05-02T00:31:23+03:00" }, + }); + let current = json!({ + "uri": "at://did:plc:them/sh.tangled.feed.comment/2", + "value": { "body": "the new shape", "createdAt": "2026-05-03T00:00:00Z" }, + }); + assert_eq!(Comment::read(&legacy).unwrap().body, "the old shape"); + assert_eq!(Comment::read(¤t).unwrap().body, "the new shape"); + assert_eq!( + Comment::read(&legacy).unwrap().author_did, + "did:plc:them", + "the commenter is the at-uri's authority, not the subject's" + ); + } + + /// A comment with nothing in it is not a comment. The appview refuses + /// one at ingest, so anything here without a body is a shape this build + /// does not understand, and an empty box under a name says less than + /// nothing. + #[test] + fn a_comment_with_no_body_is_not_rendered() { + for empty in [ + json!({ "uri": "at://did:plc:x/y/1", "value": { "createdAt": "2026-01-01T00:00:00Z" } }), + json!({ "uri": "at://did:plc:x/y/1", "value": { "body": " \n " } }), + json!({ "uri": "at://did:plc:x/y/1", "value": { "body": { "text": "" } } }), + ] { + assert_eq!(Comment::read(&empty), None, "{empty}"); + } + // And an item with no uri at all is not a record. + assert_eq!(Comment::read(&json!({ "value": { "body": "hi" } })), None); + } +} diff --git a/src/clients/tangled/mod.rs b/src/clients/tangled/mod.rs index 43ebaa5..f2229bd 100644 --- a/src/clients/tangled/mod.rs +++ b/src/clients/tangled/mod.rs @@ -16,6 +16,7 @@ //! folder is the talking-to-them part. pub(crate) mod bobbin; +pub(crate) mod comments; pub(crate) mod knot; pub(crate) mod ownership; pub(crate) mod resolve; diff --git a/src/cmd/issue/mod.rs b/src/cmd/issue/mod.rs index 1bfcb09..ecaaee2 100644 --- a/src/cmd/issue/mod.rs +++ b/src/cmd/issue/mod.rs @@ -95,8 +95,14 @@ pub(crate) enum Command { /// refused rather than guessed at, and the refusal says why: the number /// is the appview's own id and is in no record. /// + /// `--comments` adds the discussion. A comment is a record in its + /// commenter's PDS and nothing enumerates who has commented, so the + /// thread can only come from the appview index: pass `--source bobbin` + /// with it, and read what it shows as "what Bobbin has indexed". + /// /// Examples: /// atgc issue view 3msg7w7l6hs2x + /// atgc issue view 3msg7w7l6hs2x --comments --source bobbin /// atgc issue view at://did:plc:xyz/sh.tangled.repo.issue/3msg7w7l6hs2x /// atgc issue view 3msg7w7l6hs2x --json | jq .state #[command(verbatim_doc_comment)] diff --git a/src/cmd/issue/read.rs b/src/cmd/issue/read.rs index ecfeb64..eb12278 100644 --- a/src/cmd/issue/read.rs +++ b/src/cmd/issue/read.rs @@ -1214,6 +1214,17 @@ pub(crate) struct ViewArgs { /// The issue: its record key or at:// URI #[arg(value_name = "ISSUE")] pub issue: Option, + /// Also show the discussion (needs the index: --source bobbin) + /// + /// A comment is a record in its commenter's PDS and nothing enumerates + /// the people who have commented on something, so a thread can only come + /// from the appview's index. What it shows is therefore what Bobbin has + /// seen, and its ingest stalls. + #[arg(long)] + pub comments: bool, + /// Where to read from: pds (default) or bobbin (also: ATGC_USE_BOBBIN=1) + #[arg(long)] + pub source: Option, /// The same, as a flag, for symmetry with the other verbs #[arg(long = "issue", value_name = "ISSUE", conflicts_with = "issue")] pub issue_flag: Option, @@ -1247,6 +1258,18 @@ pub(crate) struct IssueDetailJson { pub body: Option, pub repo_did: Option, pub repo_url: Option, + /// The discussion, oldest first, or absent when it was not asked for. + /// + /// Named `thread` rather than `comments` because `pr view --json`'s + /// `comments` is a *count*, and one word cannot be both in two commands + /// a caller reads side by side. + /// + /// Absent and `[]` mean different things and both are reachable: absent + /// is "nobody asked the index", `[]` is "the index was asked and has + /// none". Collapsing them would make a thread the index has not caught + /// up with indistinguishable from one nobody read. + #[serde(skip_serializing_if = "Option::is_none")] + pub thread: Option>, } pub(crate) async fn view(args: ViewArgs) -> Result<()> { @@ -1265,6 +1288,12 @@ pub(crate) async fn view(args: ViewArgs) -> Result<()> { let state = state_of(&issue, acting.as_deref()).await?; let handle = crate::clients::atproto::did::handle_from_did_doc(&issue.author).await; + let thread = crate::cmd::read_thread( + args.comments, + crate::cmd::pr::read::Source::parse_arg(args.source.as_deref())?, + &issue.uri, + ) + .await; if args.json { return crate::term::jsonout::emit(&IssueDetailJson { @@ -1278,6 +1307,7 @@ pub(crate) async fn view(args: ViewArgs) -> Result<()> { body: issue.body().map(str::to_string), repo_did: issue.repo_did().map(str::to_string), repo_url: issue.repo_did().map(issues_url), + thread: crate::cmd::thread_json(&thread).await, }); } @@ -1299,6 +1329,7 @@ pub(crate) async fn view(args: ViewArgs) -> Result<()> { if let Some(body) = issue.body() { println!("\n{body}"); } + crate::cmd::print_thread(&thread).await; if state == State::Unknown { note_unknown_state_of_one(); } diff --git a/src/cmd/mod.rs b/src/cmd/mod.rs index 8794d5d..d1244fd 100644 --- a/src/cmd/mod.rs +++ b/src/cmd/mod.rs @@ -48,3 +48,158 @@ pub(crate) mod repo; pub(crate) mod report; pub(crate) mod search; pub(crate) mod stack; + +// --------------------------------------------------------------------------- +// The discussion on a pull or an issue +// --------------------------------------------------------------------------- + +/// One comment, as `--json` prints it. +/// +/// The same shape whichever `view` produced it, because a comment on a pull +/// and a comment on an issue are the same `sh.tangled.feed.comment` record +/// replying to a different subject, and a caller reading both should not +/// have to learn two spellings. +#[derive(serde::Serialize, Debug, PartialEq)] +pub(crate) struct CommentJson { + pub uri: String, + pub author_did: String, + /// Without the leading `@`, as everywhere else in `--json`. + pub author_handle: Option, + /// RFC 3339 as the record spells it, which is the commenter's own offset + /// and not necessarily UTC. + pub created_at: String, + pub body: String, +} + +/// What a `view` knows about the discussion it was asked for. +/// +/// Three states, not two, and the difference is the whole reason this is not +/// a `Vec`: nobody asked, the index was asked and answered, and the index +/// was asked and could not be reached. Flattening the first two would make a +/// thread nobody read look like one with nothing in it. +pub(crate) enum Thread { + /// `--comments` was not passed, or was passed with no index opted in. + NotAsked { + wanted: bool, + }, + Read(Vec), + Failed(anyhow::Error), +} + +/// Read the discussion on `subject_uri`, if it was asked for and can be. +/// +/// A failure here is never fatal. The comments are an enrichment of a view +/// that was already complete without them, and an appview outage should not +/// take down `issue view`; it is reported where it happened and the rest of +/// the answer still prints. +pub(crate) async fn read_thread( + wanted: bool, + source: pr::read::Source, + subject_uri: &str, +) -> Thread { + if !wanted || !source.uses_bobbin() { + return Thread::NotAsked { wanted }; + } + match crate::clients::tangled::comments::of(subject_uri, THREAD_LIMIT).await { + Ok(comments) => Thread::Read(comments), + Err(e) => Thread::Failed(e), + } +} + +/// How many comments a `view` asks the index for. +/// +/// A ceiling rather than paging: a thread past this is not a thread anybody +/// reads in a terminal, and a `--cursor` on a display command would be a +/// knob with a scroll bar behind it. +const THREAD_LIMIT: u32 = 100; + +/// The handles for a thread, resolved per unique commenter. +async fn thread_handles( + comments: &[crate::clients::tangled::comments::Comment], +) -> std::collections::HashMap { + use futures_util::stream::{self, StreamExt}; + let mut dids: Vec = Vec::new(); + for c in comments { + if !c.author_did.is_empty() && !dids.contains(&c.author_did) { + dids.push(c.author_did.clone()); + } + } + stream::iter(dids.into_iter().map(|did| async move { + let handle = crate::clients::atproto::did::handle_from_did_doc(&did).await; + (did, handle) + })) + .buffer_unordered(8) + .filter_map(|(did, handle)| async move { handle.map(|h| (did, h)) }) + .collect() + .await +} + +/// The thread as `--json` carries it, or `None` when nobody asked. +pub(crate) async fn thread_json(thread: &Thread) -> Option> { + let comments = match thread { + Thread::Read(comments) => comments, + // A failed read is `null` rather than `[]` for the same reason an + // unasked one is: `[]` claims the index answered and had nothing. + // The failure itself is on stderr. + Thread::NotAsked { .. } | Thread::Failed(_) => return None, + }; + let handles = thread_handles(comments).await; + Some( + comments + .iter() + .map(|c| CommentJson { + uri: c.uri.clone(), + author_handle: handles.get(&c.author_did).cloned(), + author_did: c.author_did.clone(), + created_at: c.created_at.clone(), + body: c.body.clone(), + }) + .collect(), + ) +} + +/// Print the thread under a view's body. +pub(crate) async fn print_thread(thread: &Thread) { + let comments = match thread { + Thread::Read(comments) => comments, + // Asked for, with no index to ask. Worth saying: the alternative is + // a `--comments` that silently shows none, which reads as "there are + // none" — the failure this tree opts out of everywhere else. + Thread::NotAsked { wanted: true } => { + crate::term::say::note!( + Index, + "a discussion can only come from the appview index, and none is opted in: \n\ + add --source bobbin (alpha, its ingest stalls)" + ); + return; + } + Thread::NotAsked { wanted: false } => return, + Thread::Failed(e) => { + crate::term::say::warning!(Index, "could not read the discussion: {e:#}"); + return; + } + }; + if comments.is_empty() { + crate::term::say::note!(Index, "no comments on this one, as far as the index knows"); + return; + } + let handles = thread_handles(comments).await; + for c in comments { + let who = handles + .get(&c.author_did) + .map(|h| format!("@{h}")) + .unwrap_or_else(|| c.author_did.clone()); + println!(); + println!("--- {who} {}", crate::term::column::day(&c.created_at)); + // The body as written. It is markdown, and rendering it would mean + // choosing a renderer for somebody else's prose; `pr diff` makes the + // same call about a patch and hands it to the pager unchanged. + println!("{}", c.body); + } + crate::term::say::note!( + Index, + "{} comment(s), as far as Bobbin has indexed. Its ingest stalls, so a recent \n\ + one can be missing.", + comments.len() + ); +} diff --git a/src/cmd/pr/mod.rs b/src/cmd/pr/mod.rs index 97de683..6a0f9c8 100644 --- a/src/cmd/pr/mod.rs +++ b/src/cmd/pr/mod.rs @@ -120,6 +120,11 @@ pub(crate) enum Command { /// not caught up with yet is read live from its author's PDS and shown /// by itself. /// + /// `--comments` adds the discussion. A comment is a record in its + /// commenter's PDS and nothing enumerates who has commented, so the + /// thread can only come from the appview index: pass `--source bobbin` + /// with it. `issue view` takes the same pair. + /// /// `--json` prints one object with the same fields the human view /// prints, state, rounds, the stack chain when there is one, plus /// each round's raw timestamp and byte size in place of the day-only diff --git a/src/cmd/pr/read.rs b/src/cmd/pr/read.rs index 1b804c2..c161b33 100644 --- a/src/cmd/pr/read.rs +++ b/src/cmd/pr/read.rs @@ -1572,6 +1572,13 @@ pub(crate) struct PullDetailJson { /// here, so the field says nothing instead of saying it wrongly. pub url: Option, pub stack: Option, + /// The discussion, oldest first, or absent when it was not asked for. + /// + /// Distinct from `comments` above, which is the *count* the listing + /// envelope carries and is there whether or not anybody asked for the + /// bodies. `issue view --json` spells this field the same way. + #[serde(skip_serializing_if = "Option::is_none")] + pub thread: Option>, } /// Build a [`PullDetailJson`] from data `view` has already resolved. @@ -1588,6 +1595,7 @@ pub(super) fn pull_detail_json( comments: u64, url: Option<&str>, stack: Option, + thread: Option>, ) -> PullDetailJson { let v = &item["value"]; let rounds = match v["rounds"].as_array() { @@ -1633,6 +1641,7 @@ pub(super) fn pull_detail_json( .map(str::to_string), url: url.map(str::to_string), stack, + thread, } } @@ -2274,6 +2283,13 @@ pub(crate) struct ViewArgs { /// Also open the repo's pulls page in the browser #[arg(long)] pub web: bool, + /// Also show the discussion (needs the index: --source bobbin) + /// + /// A comment is a record in its commenter's PDS and nothing enumerates + /// the people who have commented, so a thread can only come from the + /// appview's index, whose ingest stalls. + #[arg(long)] + pub comments: bool, /// Where to read from: pds (default), bobbin, web, auto for all three, /// or a comma-separated combination. Both indexes are opt-in; /// ATGC_USE_BOBBIN=1 and ATGC_USE_WEB=1 add one to the default @@ -2448,6 +2464,7 @@ pub(crate) async fn view(args: ViewArgs) -> Result<()> { let created = day(v["createdAt"].as_str().unwrap_or("")); let comments = item["commentCount"].as_u64().unwrap_or(0); let uri = item["uri"].as_str().unwrap_or("?"); + let thread = crate::cmd::read_thread(args.comments, source, uri).await; // The number is the appview's and is in no record, so this is a lookup // rather than a format — and one that falls back to the repo's listing, // which is what the line said before it could ever be more specific. The @@ -2496,6 +2513,7 @@ pub(crate) async fn view(args: ViewArgs) -> Result<()> { comments, link.numbered_url(), stack, + crate::cmd::thread_json(&thread).await, ); crate::term::jsonout::emit(&detail)?; // On stderr in both modes, and for the same reason: it is the only @@ -2571,6 +2589,7 @@ pub(crate) async fn view(args: ViewArgs) -> Result<()> { println!(); println!("uri: {uri}"); println!("view: {}", crate::term::hyperlink::url(link.url())); + crate::cmd::print_thread(&thread).await; if let Some(note) = link.note() { crate::term::say::note!(Index, "{note}"); } @@ -4091,6 +4110,7 @@ mod tests { 0, Some("https://tangled.org/x/pulls/9"), None, + None, ); assert_eq!(detail.rounds.len(), 1); assert_eq!(detail.rounds[0].index, 1); @@ -4111,6 +4131,7 @@ mod tests { 1, Some("https://tangled.org/x/pulls/1"), None, + None, ); assert_eq!(detail.rounds.len(), 1); assert_eq!(detail.rounds[0].index, 1); @@ -4131,7 +4152,15 @@ mod tests { "uri": "at://did:plc:me/sh.tangled.repo.pull/3xyz", "value": { "title": "t", "body": " \n ", "createdAt": "2026-01-01T00:00:00Z" }, }); - let detail = pull_detail_json(&item, "open", None, 0, Some("https://x/pulls/1"), None); + let detail = pull_detail_json( + &item, + "open", + None, + 0, + Some("https://x/pulls/1"), + None, + None, + ); assert_eq!(detail.body, None); } @@ -4159,6 +4188,7 @@ mod tests { 0, Some("https://x/pulls/1"), Some(stack), + None, ); let stack = detail.stack.expect("stack should be Some"); assert_eq!(stack.total, 2); @@ -4221,7 +4251,15 @@ mod tests { #[test] fn pull_detail_json_serializes_with_the_documented_field_names() { let item = &bobbin_items()[0]; - let detail = pull_detail_json(item, "merged", None, 0, Some("https://x/pulls/1"), None); + let detail = pull_detail_json( + item, + "merged", + None, + 0, + Some("https://x/pulls/1"), + None, + None, + ); let value = serde_json::to_value(&detail).unwrap(); let mut keys: Vec<&str> = value .as_object()