From 34939253f77c85bcb055a0d9deba6bd08e7f1b39 Mon Sep 17 00:00:00 2001
From: Fintan Halpenny <fintan.halpenny@gmail.com>
Date: Tue, 1 Jul 2025 16:52:21 +0200
Subject: [PATCH] radicle: improve reviews
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The `Review::summary` was limited in behaviour, since it was only an
`Option<String>`.

The field is now changed to `NonEmpty<Edit>` so that it can enable an edit
history, as well as being able to supply embeds as part of the text.

`Reactions` are also introduced to `Review` and, necessarily, introducing an
`Action::ReviewReact`.

Note that some machinery was added under `actions` and `encoding` to enable
backwards-compatibility.
For `actions`, a new `ReviewEdit` is introduced to keep
track of the different versions of editing actions – supporting the old actions
while also introducing the new variant.
For `encoding`, the machinery exploits the fact that an `Option<String>` can be
converted into a `NonEmpty<Edit>` given that an `ActorId` and `Timestamp` are
available, and that `None` can be converted into an empty string. The struct
`patch::Review` now deserializes via this `encoding::review::Review` type and,
importantly, preserves backwards-compatibility.
---
 Cargo.lock                                    |  28 ++
 crates/radicle/Cargo.toml                     |   1 +
 crates/radicle/src/cob/patch.rs               | 149 ++++++----
 crates/radicle/src/cob/patch/actions.rs       | 180 +++++++++++++
 crates/radicle/src/cob/patch/encoding.rs      |   6 +
 .../radicle/src/cob/patch/encoding/review.rs  | 254 ++++++++++++++++++
 6 files changed, 573 insertions(+), 45 deletions(-)
 create mode 100644 crates/radicle/src/cob/patch/actions.rs
 create mode 100644 crates/radicle/src/cob/patch/encoding.rs
 create mode 100644 crates/radicle/src/cob/patch/encoding/review.rs

diff --git a/Cargo.lock b/Cargo.lock
index 119495a2..c640af10 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -802,6 +802,16 @@ version = "1.0.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "5443807d6dff69373d433ab9ef5378ad8df50ca6298caf15de6e52e24aaf54d5"
 
+[[package]]
+name = "erased-serde"
+version = "0.4.6"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e004d887f51fcb9fef17317a2f3525c887d8aa3f4f50fed920816a688284a5b7"
+dependencies = [
+ "serde",
+ "typeid",
+]
+
 [[package]]
 name = "errno"
 version = "0.3.13"
@@ -2471,6 +2481,7 @@ dependencies = [
  "radicle-ssh",
  "schemars",
  "serde",
+ "serde-untagged",
  "serde_json",
  "siphasher 1.0.1",
  "sqlite",
@@ -3012,6 +3023,17 @@ dependencies = [
  "serde_derive",
 ]
 
+[[package]]
+name = "serde-untagged"
+version = "0.1.7"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "299d9c19d7d466db4ab10addd5703e4c615dec2a5a16dbbafe191045e87ee66e"
+dependencies = [
+ "erased-serde",
+ "serde",
+ "typeid",
+]
+
 [[package]]
 name = "serde_derive"
 version = "1.0.219"
@@ -3603,6 +3625,12 @@ dependencies = [
  "tree-sitter-language",
 ]
 
+[[package]]
+name = "typeid"
+version = "1.0.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "bc7d623258602320d5c55d1bc22793b57daff0ec7efc270ea7d55ce1d5f5471c"
+
 [[package]]
 name = "typenum"
 version = "1.17.0"
diff --git a/crates/radicle/Cargo.toml b/crates/radicle/Cargo.toml
index 38562b02..acb4ba82 100644
--- a/crates/radicle/Cargo.toml
+++ b/crates/radicle/Cargo.toml
@@ -37,6 +37,7 @@ radicle-ssh = { workspace = true }
 schemars = { workspace = true, optional = true }
 serde = { workspace = true, features = ["derive"] }
 serde_json = { workspace = true, features = ["preserve_order"] }
+serde-untagged = "0.1.7"
 siphasher = "1.0.0"
 sqlite = { workspace = true, features = ["bundled"] }
 tempfile = { workspace = true }
diff --git a/crates/radicle/src/cob/patch.rs b/crates/radicle/src/cob/patch.rs
index fc224a0c..8bfbe27a 100644
--- a/crates/radicle/src/cob/patch.rs
+++ b/crates/radicle/src/cob/patch.rs
@@ -1,5 +1,10 @@
 pub mod cache;
 
+mod actions;
+pub use actions::ReviewEdit;
+
+mod encoding;
+
 use std::collections::btree_map;
 use std::collections::{BTreeMap, BTreeSet, HashMap};
 use std::fmt;
@@ -187,16 +192,6 @@ pub enum Action {
         #[serde(default, skip_serializing_if = "Vec::is_empty")]
         labels: Vec<Label>,
     },
-    #[serde(rename = "review.edit")]
-    ReviewEdit {
-        review: ReviewId,
-        #[serde(default, skip_serializing_if = "Option::is_none")]
-        summary: Option<String>,
-        #[serde(default, skip_serializing_if = "Option::is_none")]
-        verdict: Option<Verdict>,
-        #[serde(default, skip_serializing_if = "Vec::is_empty")]
-        labels: Vec<Label>,
-    },
     #[serde(rename = "review.redact")]
     ReviewRedact { review: ReviewId },
     #[serde(rename = "review.comment")]
@@ -234,6 +229,13 @@ pub enum Action {
     ReviewCommentResolve { review: ReviewId, comment: EntryId },
     #[serde(rename = "review.comment.unresolve")]
     ReviewCommentUnresolve { review: ReviewId, comment: EntryId },
+    /// React to the review.
+    #[serde(rename = "review.react")]
+    ReviewReact {
+        review: ReviewId,
+        reaction: Reaction,
+        active: bool,
+    },
 
     //
     // Revision actions
@@ -307,6 +309,12 @@ pub enum Action {
         reaction: Reaction,
         active: bool,
     },
+    /// Edit a review's summary, verdict, labels, and embeds.
+    // Note that the tags live on `actions::ReviewEdit`, and according to the
+    // serde.rs docs, it must come after the other variants due to the
+    // `untagged` declaration.
+    #[serde(untagged)]
+    ReviewEdit(actions::ReviewEdit),
 }
 
 impl CobAction for Action {
@@ -674,7 +682,7 @@ impl Patch {
             },
             // Anyone can submit a review.
             Action::Review { .. } => Authorization::Allow,
-            Action::ReviewRedact { review, .. } | Action::ReviewEdit { review, .. } => {
+            Action::ReviewRedact { review, .. } => {
                 if let Some((_, review)) = lookup::review(self, review)? {
                     Authorization::from(actor == review.author.public_key())
                 } else {
@@ -682,6 +690,14 @@ impl Patch {
                     Authorization::Unknown
                 }
             }
+            Action::ReviewEdit(edit) => {
+                if let Some((_, review)) = lookup::review(self, edit.review_id())? {
+                    Authorization::from(actor == review.author.public_key())
+                } else {
+                    // Redacted.
+                    Authorization::Unknown
+                }
+            }
             // Anyone can comment on a review.
             Action::ReviewComment { .. } => Authorization::Allow,
             // The comment author can edit and redact their own comment.
@@ -714,6 +730,7 @@ impl Patch {
                 // Redacted.
                 Authorization::Unknown
             }
+            Action::ReviewReact { .. } => Authorization::Allow,
             // Anyone can propose revisions.
             Action::Revision { .. } => Authorization::Allow,
             // Only the revision author can edit or redact their revision.
@@ -896,7 +913,7 @@ impl Patch {
             }
             Action::Review {
                 revision,
-                ref summary,
+                summary,
                 verdict,
                 labels,
             } => {
@@ -914,8 +931,9 @@ impl Patch {
                             id,
                             Author::new(author),
                             verdict,
-                            summary.to_owned(),
+                            summary.unwrap_or_default(),
                             labels,
+                            vec![],
                             timestamp,
                         ));
                         // Update reviews index.
@@ -928,22 +946,7 @@ impl Patch {
                     }
                 }
             }
-            Action::ReviewEdit {
-                review,
-                summary,
-                verdict,
-                labels,
-            } => {
-                if summary.is_none() && verdict.is_none() {
-                    return Err(Error::EmptyReview);
-                }
-                let Some(review) = lookup::review_mut(self, &review)? else {
-                    return Ok(());
-                };
-                review.verdict = verdict;
-                review.summary = summary;
-                review.labels = labels;
-            }
+            Action::ReviewEdit(edit) => edit.run(author, timestamp, self)?,
             Action::ReviewCommentReact {
                 review,
                 comment,
@@ -1042,6 +1045,19 @@ impl Patch {
                 // Set the review locator in the review index to redacted.
                 *locator = None;
             }
+            Action::ReviewReact {
+                review,
+                reaction,
+                active,
+            } => {
+                if let Some(review) = lookup::review_mut(self, &review)? {
+                    if active {
+                        review.reactions.insert((author, reaction));
+                    } else {
+                        review.reactions.remove(&(author, reaction));
+                    }
+                }
+            }
             Action::Merge { revision, commit } => {
                 // If the revision was redacted before the merge, ignore the merge.
                 if lookup::revision_mut(self, &revision)?.is_none() {
@@ -1613,6 +1629,7 @@ impl fmt::Display for Verdict {
 /// A patch review on a revision.
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
+#[serde(from = "encoding::review::Review")]
 pub struct Review {
     /// Review identifier.
     pub(super) id: ReviewId,
@@ -1624,13 +1641,19 @@ pub struct Review {
     pub(super) verdict: Option<Verdict>,
     /// Review summary.
     ///
-    /// Can be edited or set to `None`.
-    pub(super) summary: Option<String>,
+    /// Can be empty, given there is a [`Verdict`].
+    ///
+    /// If not empty, then the last [`Edit`] in the `Vec` will be the latest
+    /// edit of the summary.
+    pub(super) summary: NonEmpty<Edit>,
     /// Review comments.
     pub(super) comments: Thread<Comment<CodeLocation>>,
     /// Labels qualifying the review. For example if this review only looks at the
     /// concept or intention of the patch, it could have a "concept" label.
     pub(super) labels: Vec<Label>,
+    #[serde(skip_serializing_if = "BTreeSet::is_empty")]
+    /// Reactions to the review.
+    pub(super) reactions: Reactions,
     /// Review timestamp.
     pub(super) timestamp: Timestamp,
 }
@@ -1640,16 +1663,19 @@ impl Review {
         id: ReviewId,
         author: Author,
         verdict: Option<Verdict>,
-        summary: Option<String>,
+        summary: String,
         labels: Vec<Label>,
+        embeds: Vec<Embed<Uri>>,
         timestamp: Timestamp,
     ) -> Self {
+        let summary = NonEmpty::new(Edit::new(*author.public_key(), summary, timestamp, embeds));
         Self {
             id,
             author,
             verdict,
             summary,
             comments: Thread::default(),
+            reactions: BTreeSet::new(),
             labels,
             timestamp,
         }
@@ -1681,8 +1707,23 @@ impl Review {
     }
 
     /// Review general comment.
-    pub fn summary(&self) -> Option<&str> {
-        self.summary.as_deref()
+    pub fn summary(&self) -> &str {
+        self.summary.last().body.as_str()
+    }
+
+    /// Review embeds.
+    pub fn embeds(&self) -> &[Embed<Uri>] {
+        &self.summary.last().embeds
+    }
+
+    /// Review reactions.
+    pub fn reactions(&self) -> &Reactions {
+        &self.reactions
+    }
+
+    /// Get the review summary edits.
+    pub fn edits(&self) -> impl Iterator<Item = &Edit> {
+        self.summary.iter()
     }
 
     /// Review timestamp.
@@ -1911,15 +1952,17 @@ impl<R: ReadRepository> store::Transaction<Patch, R> {
         &mut self,
         review: ReviewId,
         verdict: Option<Verdict>,
-        summary: Option<String>,
+        summary: String,
         labels: Vec<Label>,
+        embeds: impl IntoIterator<Item = Embed<Uri>>,
     ) -> Result<(), store::Error> {
-        self.push(Action::ReviewEdit {
+        self.push(Action::ReviewEdit(actions::ReviewEdit::new(
             review,
             summary,
             verdict,
             labels,
-        })
+            embeds.into_iter().collect(),
+        )))
     }
 
     /// Redact a patch review.
@@ -2272,15 +2315,16 @@ where
         &mut self,
         review: ReviewId,
         verdict: Option<Verdict>,
-        summary: Option<String>,
+        summary: String,
         labels: Vec<Label>,
+        embeds: impl IntoIterator<Item = Embed<Uri>>,
         signer: &Device<G>,
     ) -> Result<EntryId, Error>
     where
         G: crypto::signature::Signer<crypto::Signature>,
     {
         self.transaction("Edit review", signer, |tx| {
-            tx.review_edit(review, verdict, summary, labels)
+            tx.review_edit(review, verdict, summary, labels, embeds)
         })
     }
 
@@ -3109,7 +3153,7 @@ mod test {
 
         let review = revision.review_by(alice.signer.public_key()).unwrap();
         assert_eq!(review.verdict(), Some(Verdict::Accept));
-        assert_eq!(review.summary(), Some("LGTM"));
+        assert_eq!(review.summary(), "LGTM");
 
         patch.redact_review(review_id, &alice.signer).unwrap();
         patch.reload().unwrap();
@@ -3345,7 +3389,8 @@ mod test {
             .review_edit(
                 review,
                 Some(Verdict::Reject),
-                Some("Whoops!".to_owned()),
+                "Whoops!".to_owned(),
+                vec![],
                 vec![],
                 &alice.signer,
             )
@@ -3354,7 +3399,7 @@ mod test {
         let (_, revision) = patch.latest();
         let review = revision.review_by(alice.signer.public_key()).unwrap();
         assert_eq!(review.verdict(), Some(Verdict::Reject));
-        assert_eq!(review.summary(), Some("Whoops!"));
+        assert_eq!(review.summary(), "Whoops!");
     }
 
     #[test]
@@ -3415,7 +3460,14 @@ mod test {
             .unwrap();
 
         let _review = patch
-            .review_edit(review, Some(Verdict::Reject), None, vec![], &alice.signer)
+            .review_edit(
+                review,
+                Some(Verdict::Reject),
+                "".to_string(),
+                vec![],
+                vec![],
+                &alice.signer,
+            )
             .unwrap();
         patch
             .review_comment(review, "Second comment!", None, None, [], &alice.signer)
@@ -3508,7 +3560,14 @@ mod test {
             )
             .unwrap();
         patch
-            .review_edit(review, Some(Verdict::Accept), None, vec![], &alice.signer)
+            .review_edit(
+                review,
+                Some(Verdict::Accept),
+                "".to_string(),
+                vec![],
+                vec![],
+                &alice.signer,
+            )
             .unwrap();
 
         let id = patch.id;
@@ -3517,7 +3576,7 @@ mod test {
         let review = revision.review_by(alice.signer.public_key()).unwrap();
 
         assert_eq!(review.verdict(), Some(Verdict::Accept));
-        assert_eq!(review.summary(), None);
+        assert_eq!(review.summary(), "");
     }
 
     #[test]
diff --git a/crates/radicle/src/cob/patch/actions.rs b/crates/radicle/src/cob/patch/actions.rs
new file mode 100644
index 00000000..6924c5fc
--- /dev/null
+++ b/crates/radicle/src/cob/patch/actions.rs
@@ -0,0 +1,180 @@
+//! Keep track of the patch [`Action`] versions, to ensure compatibility where
+//! possible.
+//!
+//! [`Action`]: super::Action
+
+use serde::{Deserialize, Serialize};
+
+use crate::cob::{thread::Edit, ActorId, Embed, Label, Timestamp, Uri};
+
+use super::{lookup, Error, Patch, ReviewId, Verdict};
+
+/// A review edit that keeps track of the different versions of actions.
+///
+/// [`ReviewEdit::new`] will create the latest version of the action.
+///
+/// [`ReviewEdit::run`] will apply the action to the given [`Patch`].
+#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)]
+#[serde(tag = "type", rename_all = "camelCase")]
+pub enum ReviewEdit {
+    /// The initial version of editing a review.
+    ///
+    /// This allowed editing the `summary`, `verdict`, and `labels` of a
+    /// [`Patch`], where the `summary` value was optional.
+    #[serde(rename = "review.edit")]
+    V1(ReviewEditV1),
+    /// The latest version of editing a review.
+    ///
+    /// This allows editing the `summary`, `verdict`, `labels` of [`Patch`], and
+    /// introduces `embeds` to the review [`summary`].
+    ///
+    /// The `summary` of a [`Review`] is now an edit-history.
+    #[serde(rename = "review.edit.v2")]
+    V2(ReviewEditV2),
+}
+
+impl ReviewEdit {
+    /// Create the latest version of [`ReviewEdit`].
+    pub fn new(
+        review: ReviewId,
+        summary: String,
+        verdict: Option<Verdict>,
+        labels: Vec<Label>,
+        embeds: Vec<Embed<Uri>>,
+    ) -> Self {
+        Self::V2(ReviewEditV2 {
+            review,
+            summary,
+            verdict,
+            labels,
+            embeds,
+        })
+    }
+
+    /// Get the [`ReviewId`] that this edit is applying to.
+    pub fn review_id(&self) -> &ReviewId {
+        match self {
+            ReviewEdit::V1(ReviewEditV1 { review, .. }) => review,
+            ReviewEdit::V2(ReviewEditV2 { review, .. }) => review,
+        }
+    }
+
+    /// Apply the action to the given [`Patch`].
+    pub fn run(
+        self,
+        author: ActorId,
+        timestamp: Timestamp,
+        patch: &mut Patch,
+    ) -> Result<(), Error> {
+        match self {
+            ReviewEdit::V1(ReviewEditV1 {
+                review,
+                summary,
+                verdict,
+                labels,
+            }) => {
+                if summary.is_none() && verdict.is_none() {
+                    return Err(Error::EmptyReview);
+                }
+                let Some(review) = lookup::review_mut(patch, &review)? else {
+                    return Ok(());
+                };
+
+                if let Some(body) = summary {
+                    review
+                        .summary
+                        .push(Edit::new(author, body, timestamp, vec![]));
+                }
+                review.verdict = verdict;
+                review.labels = labels;
+                Ok(())
+            }
+            ReviewEdit::V2(ReviewEditV2 {
+                review,
+                summary,
+                verdict,
+                labels,
+                embeds,
+            }) => {
+                if summary.is_empty() && verdict.is_none() {
+                    return Err(Error::EmptyReview);
+                }
+                let Some(review) = lookup::review_mut(patch, &review)? else {
+                    return Ok(());
+                };
+
+                review
+                    .summary
+                    .push(Edit::new(author, summary, timestamp, embeds));
+                review.verdict = verdict;
+                review.labels = labels;
+                Ok(())
+            }
+        }
+    }
+}
+
+#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct ReviewEditV2 {
+    review: ReviewId,
+    #[serde(default, skip_serializing_if = "String::is_empty")]
+    summary: String,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    verdict: Option<Verdict>,
+    #[serde(default, skip_serializing_if = "Vec::is_empty")]
+    labels: Vec<Label>,
+    #[serde(default, skip_serializing_if = "Vec::is_empty")]
+    embeds: Vec<Embed<Uri>>,
+}
+
+#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct ReviewEditV1 {
+    review: ReviewId,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    summary: Option<String>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    verdict: Option<Verdict>,
+    #[serde(default, skip_serializing_if = "Vec::is_empty")]
+    labels: Vec<Label>,
+}
+
+#[allow(clippy::unwrap_used)]
+#[cfg(test)]
+mod test {
+    use serde_json::json;
+
+    use crate::patch;
+
+    use super::ReviewEdit;
+
+    #[test]
+    fn test_review_edit() {
+        let v1 = json!({
+            "type": "review.edit",
+            "review": "89d45fb371eb2622ba88188d474347cc526d80bb",
+            "summary": "lgtm",
+            "verdict": "accept",
+            "labels": [],
+        });
+        let v2 = json!({
+            "type": "review.edit.v2",
+            "review": "89d45fb371eb2622ba88188d474347cc526d80bb",
+            "summary": "lgtm",
+            "verdict": "accept",
+            "labels": [],
+            "embeds": [],
+        });
+        serde_json::from_value::<ReviewEdit>(v1.clone()).unwrap();
+        serde_json::from_value::<ReviewEdit>(v2.clone()).unwrap();
+        assert!(matches!(
+            serde_json::from_value::<patch::Action>(v1).unwrap(),
+            patch::Action::ReviewEdit { .. }
+        ));
+        assert!(matches!(
+            serde_json::from_value::<patch::Action>(v2).unwrap(),
+            patch::Action::ReviewEdit { .. }
+        ));
+    }
+}
diff --git a/crates/radicle/src/cob/patch/encoding.rs b/crates/radicle/src/cob/patch/encoding.rs
new file mode 100644
index 00000000..01897f31
--- /dev/null
+++ b/crates/radicle/src/cob/patch/encoding.rs
@@ -0,0 +1,6 @@
+//! Home for separating serialization and deserialization of [`Patch`] data from
+//! the final [`Patch`] state.
+//!
+//! [`Patch`]: super::Patch
+
+pub mod review;
diff --git a/crates/radicle/src/cob/patch/encoding/review.rs b/crates/radicle/src/cob/patch/encoding/review.rs
new file mode 100644
index 00000000..306b64a9
--- /dev/null
+++ b/crates/radicle/src/cob/patch/encoding/review.rs
@@ -0,0 +1,254 @@
+//! Representation of a [`patch::Review`] for deserializing.
+//!
+//! The [`Review`] type contains fields that are:
+//!
+//!   - Shared within previous versions, e.g. V1 fields that are shared with V2
+//!   - Introduced at a later version, but can use a [`Default`] instance
+//!   - Can be migrated from one version to another
+//!
+//! [`patch::Review`]: crate::cob::patch::Review
+
+use nonempty::NonEmpty;
+use serde::Deserialize;
+use serde_untagged::UntaggedEnumVisitor;
+
+use crate::cob::patch;
+use crate::cob::patch::{
+    Author, CodeLocation, Comment, Edit, Label, Reactions, ReviewId, Thread, Timestamp, Verdict,
+};
+
+/// The encoding for a `patch::Review` that can be deserialized and migrated.
+///
+/// To maintain backwards-compatibility, [`Review`] must implement:
+/// ```rust, no_run
+/// From<Review> for patch::Review
+/// ```
+#[derive(Deserialize)]
+pub(in crate::cob::patch) struct Review {
+    // V1 fields
+    id: ReviewId,
+    author: Author,
+    verdict: Option<Verdict>,
+    comments: Thread<Comment<CodeLocation>>,
+    labels: Vec<Label>,
+    timestamp: Timestamp,
+
+    // V2 fields
+    #[serde(default)]
+    reactions: Reactions,
+
+    // V1 -> V2 conversion
+    #[serde(default)]
+    summary: Summary,
+}
+
+/// The [`Summary`] type represents the different versions of the `summary`
+/// field of a [`Review`].
+///
+/// The `V1` variant holds an `Option<String>`, which can be converted into an
+/// `Edit` given the `ActorId` and `Timestamp` – supplying an empty `Vec` of
+/// `Embed`s.
+///
+/// The `V2` variant holds the current type `NonEmpty<Edit>`.
+///
+/// Using [`Summary::into_edits`], we can get the latest representation of a
+/// [`patch::Review::summary`].
+///
+/// [`patch::Review::summary`]: crate::cob::patch::Review::summary
+#[derive(Debug, PartialEq, Eq)]
+pub(in crate::cob::patch) enum Summary {
+    V2(NonEmpty<Edit>),
+    V1(Option<String>),
+}
+
+impl<'de> Deserialize<'de> for Summary {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        UntaggedEnumVisitor::new()
+            .unit(|| Ok(Self::V1(None)))
+            .string(|body| Ok(Self::V1(Some(body.to_owned()))))
+            .seq(|edits| edits.deserialize().map(Self::V2))
+            .deserialize(deserializer)
+    }
+}
+
+impl Default for Summary {
+    fn default() -> Self {
+        Self::V1(None)
+    }
+}
+
+impl Summary {
+    fn into_edits(self, author: &Author, timestamp: &Timestamp) -> NonEmpty<Edit> {
+        match self {
+            Summary::V1(summary) => NonEmpty::new(Edit::new(
+                *author.public_key(),
+                summary.unwrap_or_default(),
+                *timestamp,
+                vec![],
+            )),
+            Summary::V2(edits) => edits,
+        }
+    }
+}
+
+impl From<Review> for patch::Review {
+    fn from(review: Review) -> Self {
+        let Review {
+            id,
+            author,
+            verdict,
+            comments,
+            labels,
+            timestamp,
+            reactions,
+            summary,
+        } = review;
+        let summary = summary.into_edits(&author, &timestamp);
+        Self {
+            id,
+            author,
+            verdict,
+            summary,
+            comments,
+            labels,
+            reactions,
+            timestamp,
+        }
+    }
+}
+
+#[allow(clippy::unwrap_used)]
+#[cfg(test)]
+mod test {
+    use nonempty::nonempty;
+    use serde_json::json;
+
+    use crate::{
+        cob::{thread::Edit, Timestamp},
+        patch,
+    };
+
+    use super::{Review, Summary};
+
+    #[test]
+    fn test_review_summary() {
+        let summary_null = json!(null);
+        let summary_string = json!("lgtm");
+        let summary_edits = json!([{
+            "author": "z6MkwPUeUS2fJMfc2HZN1RQTQcTTuhw4HhPySB8JeUg2mVvx",
+            "timestamp": 1710947885000_i64,
+            "body": "lgtm",
+            "embeds": [],
+        }]);
+        assert_eq!(
+            serde_json::from_value::<Summary>(summary_null).unwrap(),
+            Summary::V1(None)
+        );
+        assert_eq!(
+            serde_json::from_value::<Summary>(summary_string).unwrap(),
+            Summary::V1(Some("lgtm".to_string()))
+        );
+        assert_eq!(
+            serde_json::from_value::<Summary>(summary_edits).unwrap(),
+            Summary::V2(nonempty![Edit::new(
+                "z6MkwPUeUS2fJMfc2HZN1RQTQcTTuhw4HhPySB8JeUg2mVvx"
+                    .parse()
+                    .unwrap(),
+                "lgtm".to_string(),
+                Timestamp::from_secs(1710947885),
+                vec![]
+            )])
+        );
+    }
+
+    #[test]
+    fn test_review_deserialize_summary_migration_null_summary() {
+        let review = json!({
+            "id": "89d45fb371eb2622ba88188d474347cc526d80bb",
+            "author": { "id": "did:key:z6MkwPUeUS2fJMfc2HZN1RQTQcTTuhw4HhPySB8JeUg2mVvx" },
+            "verdict": "accept",
+            "summary": null,
+            "comments": {
+                "comments": {},
+                "timeline": []
+            },
+            "labels": [],
+            "timestamp": 1710947885000_i64
+        });
+        let v1 = serde_json::from_value::<Review>(review.clone()).unwrap();
+        assert_eq!(
+            serde_json::from_value::<patch::Review>(review).unwrap(),
+            v1.into()
+        );
+    }
+
+    #[test]
+    fn test_review_deserialize_summary_migration_without_summary() {
+        let review = json!({
+            "id": "89d45fb371eb2622ba88188d474347cc526d80bb",
+            "author": { "id": "did:key:z6MkwPUeUS2fJMfc2HZN1RQTQcTTuhw4HhPySB8JeUg2mVvx" },
+            "verdict": "accept",
+            "comments": {
+                "comments": {},
+                "timeline": []
+            },
+            "labels": [],
+            "timestamp": 1710947885000_i64
+        });
+        let v1 = serde_json::from_value::<Review>(review.clone()).unwrap();
+        assert_eq!(
+            serde_json::from_value::<patch::Review>(review).unwrap(),
+            v1.into()
+        );
+    }
+
+    #[test]
+    fn test_review_deserialize_summary_migration_with_summary() {
+        let review = json!({
+            "id": "89d45fb371eb2622ba88188d474347cc526d80bb",
+            "author": { "id": "did:key:z6MkwPUeUS2fJMfc2HZN1RQTQcTTuhw4HhPySB8JeUg2mVvx" },
+            "verdict": "accept",
+            "summary": "lgtm",
+            "comments": {
+                "comments": {},
+                "timeline": []
+            },
+            "labels": [],
+            "timestamp": 1710947885000_i64
+        });
+        let v1 = serde_json::from_value::<Review>(review.clone()).unwrap();
+        assert_eq!(
+            serde_json::from_value::<patch::Review>(review).unwrap(),
+            v1.into()
+        );
+    }
+
+    #[test]
+    fn test_review_deserialize_summary_v2() {
+        let review = json!({
+            "id": "89d45fb371eb2622ba88188d474347cc526d80bb",
+            "author": { "id": "did:key:z6MkwPUeUS2fJMfc2HZN1RQTQcTTuhw4HhPySB8JeUg2mVvx" },
+            "verdict": "accept",
+            "summary": [{
+                "author": "z6MkwPUeUS2fJMfc2HZN1RQTQcTTuhw4HhPySB8JeUg2mVvx",
+                "timestamp": 1710947885000_i64,
+                "body": "lgtm",
+                "embeds": [],
+            }],
+            "comments": {
+                "comments": {},
+                "timeline": []
+            },
+            "labels": [],
+            "timestamp": 1710947885000_i64
+        });
+        let v2 = serde_json::from_value::<Review>(review.clone()).unwrap();
+        assert_eq!(
+            serde_json::from_value::<patch::Review>(review).unwrap(),
+            v2.into()
+        );
+    }
+}