michal/tit

Diff

ee64ad60d39b → 4e21b749c133

@@ -1023,26 +1023,154 @@
   border-bottom: 1px solid var(--border);
 }
 
-.pull-request-file > pre {
+.pull-request-line-comment {
+  max-width: none;
   margin: 0;
+}
+
+.pull-request-line-help {
+  margin: 0;
+  padding: 0.55rem 0.85rem;
+  border-bottom: 1px solid var(--border);
+  color: var(--fg-muted);
+  font-size: 0.7rem;
+}
+
+.pull-request-file-diff {
+  margin: 0;
+  padding: 0;
   border: 0;
   background: var(--bg);
 }
 
-.pull-request-line-comment {
+.pull-request-diff-table {
+  min-width: 38rem;
+  table-layout: fixed;
+}
+
+.pull-request-diff-table th {
+  padding: 0.25rem 0.4rem;
+  border-bottom: 1px solid var(--border);
+  background: var(--surface);
+  text-transform: none;
+}
+
+.pull-request-diff-table th:first-child,
+.pull-request-diff-table th:nth-child(2) {
+  width: 3.25rem;
+  text-align: center;
+}
+
+.pull-request-diff-table td {
+  height: 1.75rem;
+  padding: 0 0.5rem;
+  border-bottom: 0;
+  line-height: 1.45;
+}
+
+.pull-request-diff-table .diff-line-number {
+  padding: 0;
+  border-right: 1px solid var(--border);
+  color: var(--fg-muted);
+  text-align: center;
+  user-select: none;
+}
+
+.diff-line-number label {
+  display: block;
+  margin: 0;
+  color: inherit;
+  cursor: pointer;
+  font-size: 0.7rem;
+  font-weight: 400;
+}
+
+.diff-line-number input {
+  position: absolute;
+  width: 1px;
+  height: 1px;
+  opacity: 0;
+}
+
+.diff-line-number span {
+  display: block;
+  min-height: 1.75rem;
+  padding: 0.2rem 0.25rem;
+}
+
+.diff-line-number label:hover span {
+  background: var(--surface-raised);
+  color: var(--fg);
+}
+
+.diff-line-number input:focus-visible + span {
+  outline: 0.125rem solid var(--fg);
+  outline-offset: -0.125rem;
+}
+
+.diff-line-number input:checked + span {
+  background: var(--fg);
+  color: var(--bg);
+  font-weight: 700;
+}
+
+.pull-request-diff-table .diff-code {
+  padding-inline: 0.65rem;
+  overflow: hidden;
+}
+
+.diff-code code,
+.diff-hunk code,
+.diff-meta code {
+  display: block;
+  white-space: pre;
+  overflow-wrap: normal;
+}
+
+.diff-marker {
+  display: inline-block;
+  width: 1.25rem;
+  color: var(--fg);
+  font-weight: 700;
+}
+
+.diff-addition,
+.diff-deletion {
+  background: var(--bg);
+}
+
+.diff-addition .diff-code {
+  border-left: 0.2rem solid var(--fg);
+}
+
+.diff-deletion .diff-code {
+  border-left: 0.2rem dashed var(--fg-dim);
+}
+
+.diff-context .diff-code {
+  border-left: 0.2rem solid transparent;
+}
+
+.diff-deletion .diff-code code,
+.diff-deletion .diff-marker {
+  color: var(--fg-dim);
+}
+
+.diff-hunk td,
+.diff-meta td {
+  padding: 0.35rem 0.65rem;
+  background: var(--surface-raised);
+  color: var(--fg-dim);
+}
+
+.pull-request-line-comment > details {
   margin: 0;
   padding: 0.65rem 0.85rem;
   border-top: 1px solid var(--border);
 }
 
-.pull-request-line-comment form {
+.pull-request-line-comment > details form {
   max-width: 42rem;
-}
-
-.pull-request-line-fields {
-  display: grid;
-  grid-template-columns: minmax(8rem, 0.4fr) minmax(8rem, 0.6fr);
-  gap: 0.75rem;
 }
 
 .pull-request-discussion {
@@ -1301,6 +1429,18 @@
   top: auto;
 }
 
+.visually-hidden {
+  position: absolute;
+  width: 1px;
+  height: 1px;
+  padding: 0;
+  margin: -1px;
+  overflow: hidden;
+  border: 0;
+  clip: rect(0 0 0 0);
+  white-space: nowrap;
+}
+
 .skip-link:focus {
   left: 1rem;
   top: 1rem;
@@ -1489,10 +1629,6 @@
 
   .pull-request-change-summary > div:last-child {
     border-bottom: 0;
-  }
-
-  .pull-request-line-fields {
-    grid-template-columns: 1fr;
   }
 
   dl {

@@ -441,9 +441,7 @@
     let fields = match parse_named_form(
         &headers,
         &body,
-        &[
-            "csrf", "revision", "kind", "body", "path-hex", "side", "line",
-        ],
+        &["csrf", "revision", "kind", "body", "path-hex", "anchor"],
     ) {
         Ok(fields) => fields,
         Err(()) => return bad_request(&request_id.0),
@@ -465,14 +463,9 @@
             None => return bad_request(&request_id.0),
         }
     };
-    let side = (!fields[5].is_empty()).then(|| fields[5].clone());
-    let line = if fields[6].is_empty() {
-        None
-    } else {
-        match fields[6].parse::<i64>() {
-            Ok(line) => Some(line),
-            Err(_) => return bad_request(&request_id.0),
-        }
+    let (side, line) = match parse_review_anchor(&fields[5]) {
+        Ok(anchor) => anchor,
+        Err(()) => return bad_request(&request_id.0),
     };
     let Some(service) = state.pull_requests.clone() else {
         return internal(&request_id.0);
@@ -837,9 +830,21 @@
     path: String,
     path_hex: String,
     binary: bool,
+    lines: Vec<DiffLineView>,
+}
+
+struct DiffLineView {
+    kind: &'static str,
+    marker: &'static str,
+    text: String,
+    base_line: i64,
+    head_line: i64,
+    base_anchor: String,
+    head_anchor: String,
     has_base: bool,
     has_head: bool,
-    hunks: String,
+    is_hunk: bool,
+    is_meta: bool,
 }
 
 impl From<&Comparison> for ComparisonView {
@@ -875,11 +880,153 @@
                     path: String::from_utf8_lossy(&file.path).into_owned(),
                     path_hex: encode_lower_hex(&file.path),
                     binary: file.binary,
-                    has_base: file.old_id.is_some(),
-                    has_head: file.new_id.is_some(),
-                    hunks: String::from_utf8_lossy(&file.hunks).into_owned(),
+                    lines: parse_diff_lines(&String::from_utf8_lossy(&file.hunks)),
                 })
                 .collect(),
         }
+    }
+}
+
+fn parse_review_anchor(value: &str) -> Result<(Option<String>, Option<i64>), ()> {
+    if value.is_empty() {
+        return Ok((None, None));
+    }
+    let (side, line) = value.split_once(':').ok_or(())?;
+    if !matches!(side, "base" | "head") || line.contains(':') {
+        return Err(());
+    }
+    let line = line.parse::<i64>().map_err(|_| ())?;
+    if line < 1 {
+        return Err(());
+    }
+    Ok((Some(side.to_owned()), Some(line)))
+}
+
+fn parse_diff_lines(hunks: &str) -> Vec<DiffLineView> {
+    let mut base_line = 0;
+    let mut head_line = 0;
+    let mut lines = Vec::new();
+    for source in hunks.lines() {
+        if source.starts_with("@@") {
+            if let Some((base, head)) = parse_hunk_starts(source) {
+                base_line = base;
+                head_line = head;
+            }
+            lines.push(diff_meta_line(source, true));
+        } else if let Some(text) = source.strip_prefix('-') {
+            lines.push(diff_content_line(
+                "deletion",
+                "-",
+                text,
+                Some(base_line),
+                None,
+            ));
+            base_line += 1;
+        } else if let Some(text) = source.strip_prefix('+') {
+            lines.push(diff_content_line(
+                "addition",
+                "+",
+                text,
+                None,
+                Some(head_line),
+            ));
+            head_line += 1;
+        } else if let Some(text) = source.strip_prefix(' ') {
+            lines.push(diff_content_line(
+                "context",
+                " ",
+                text,
+                Some(base_line),
+                Some(head_line),
+            ));
+            base_line += 1;
+            head_line += 1;
+        } else {
+            lines.push(diff_meta_line(source, false));
+        }
+    }
+    lines
+}
+
+fn parse_hunk_starts(header: &str) -> Option<(i64, i64)> {
+    let mut fields = header.split_whitespace();
+    (fields.next()? == "@@").then_some(())?;
+    let base = parse_hunk_start(fields.next()?, '-')?;
+    let head = parse_hunk_start(fields.next()?, '+')?;
+    Some((base, head))
+}
+
+fn parse_hunk_start(range: &str, prefix: char) -> Option<i64> {
+    range.strip_prefix(prefix)?.split(',').next()?.parse().ok()
+}
+
+fn diff_meta_line(text: &str, is_hunk: bool) -> DiffLineView {
+    DiffLineView {
+        kind: if is_hunk { "hunk" } else { "meta" },
+        marker: "",
+        text: text.to_owned(),
+        base_line: 0,
+        head_line: 0,
+        base_anchor: String::new(),
+        head_anchor: String::new(),
+        has_base: false,
+        has_head: false,
+        is_hunk,
+        is_meta: !is_hunk,
+    }
+}
+
+fn diff_content_line(
+    kind: &'static str,
+    marker: &'static str,
+    text: &str,
+    base: Option<i64>,
+    head: Option<i64>,
+) -> DiffLineView {
+    DiffLineView {
+        kind,
+        marker,
+        text: text.to_owned(),
+        base_line: base.unwrap_or_default(),
+        head_line: head.unwrap_or_default(),
+        base_anchor: base.map_or_else(String::new, |line| format!("base:{line}")),
+        head_anchor: head.map_or_else(String::new, |line| format!("head:{line}")),
+        has_base: base.is_some(),
+        has_head: head.is_some(),
+        is_hunk: false,
+        is_meta: false,
+    }
+}
+
+#[cfg(test)]
+mod diff_view_tests {
+    use super::{parse_diff_lines, parse_review_anchor};
+
+    #[test]
+    fn maps_unified_diff_lines_to_visible_comment_anchors() {
+        let lines = parse_diff_lines(
+            "@@ -2,3 +2,4 @@ heading\n unchanged\n-removed\n+added\n+second addition\n",
+        );
+
+        assert!(lines[0].is_hunk);
+        assert_eq!(lines[1].base_anchor, "base:2");
+        assert_eq!(lines[1].head_anchor, "head:2");
+        assert_eq!(lines[2].base_anchor, "base:3");
+        assert!(!lines[2].has_head);
+        assert_eq!(lines[3].head_anchor, "head:3");
+        assert!(!lines[3].has_base);
+        assert_eq!(lines[4].head_anchor, "head:4");
+    }
+
+    #[test]
+    fn accepts_only_explicit_positive_review_anchors() {
+        assert_eq!(
+            parse_review_anchor("head:12"),
+            Ok((Some("head".to_owned()), Some(12)))
+        );
+        assert_eq!(parse_review_anchor(""), Ok((None, None)));
+        assert!(parse_review_anchor("side:12").is_err());
+        assert!(parse_review_anchor("base:0").is_err());
+        assert!(parse_review_anchor("head:1:2").is_err());
     }
 }

@@ -4,7 +4,7 @@
   <meta charset="utf-8">
   <meta name="viewport" content="width=device-width, initial-scale=1">
   <title>{% block title %}tit{% endblock %}</title>
-  <link rel="stylesheet" href="/assets/style.css?v=10">
+  <link rel="stylesheet" href="/assets/style.css?v=14">
 </head>
 <body>
   <a class="skip-link" href="#main">Skip to content</a>

@@ -124,40 +124,93 @@
 {% if file.binary %}
           <p>This file has binary content.</p>
 {% else %}
-          <pre>{{ file.hunks }}</pre>
 {% if can_review %}
-          <details class="pull-request-line-comment">
-            <summary>Comment on a line</summary>
-            <form method="post" action="/{{ owner }}/{{ repository }}/pulls/{{ pull_request.number }}/reviews">
-              <input type="hidden" name="csrf" value="{{ csrf }}">
-              <input type="hidden" name="revision" value="{{ selected_revision }}">
-              <input type="hidden" name="kind" value="line-comment">
-              <input type="hidden" name="path-hex" value="{{ file.path_hex }}">
-              <div class="pull-request-line-fields">
-                <div class="field">
-                  <label for="review-side-{{ file.path_hex }}">Side</label>
-                  <select id="review-side-{{ file.path_hex }}" name="side">
-{% if file.has_base %}
-                    <option value="base">Base</option>
+          <form class="pull-request-line-comment" method="post" action="/{{ owner }}/{{ repository }}/pulls/{{ pull_request.number }}/reviews">
+            <input type="hidden" name="csrf" value="{{ csrf }}">
+            <input type="hidden" name="revision" value="{{ selected_revision }}">
+            <input type="hidden" name="kind" value="line-comment">
+            <input type="hidden" name="path-hex" value="{{ file.path_hex }}">
+            <p class="pull-request-line-help">Select an old or new line number to attach a comment.</p>
+            <div class="table-scroll pull-request-file-diff">
+              <table class="pull-request-diff-table">
+                <thead>
+                  <tr>
+                    <th scope="col">Old</th>
+                    <th scope="col">New</th>
+                    <th scope="col">Code</th>
+                  </tr>
+                </thead>
+                <tbody>
+{% for line in file.lines %}
+{% if line.is_hunk %}
+                  <tr class="diff-hunk">
+                    <td colspan="3"><code>{{ line.text }}</code></td>
+                  </tr>
+{% else if line.is_meta %}
+                  <tr class="diff-meta">
+                    <td colspan="3"><code>{{ line.text }}</code></td>
+                  </tr>
+{% else %}
+                  <tr class="diff-line diff-{{ line.kind }}">
+                    <td class="diff-line-number">
+{% if line.has_base %}
+                      <label title="Comment on old line {{ line.base_line }}">
+                        <input type="radio" name="anchor" value="{{ line.base_anchor }}" aria-label="Comment on old line {{ line.base_line }}" required>
+                        <span>{{ line.base_line }}</span>
+                      </label>
 {% endif %}
-{% if file.has_head %}
-                    <option value="head">Head</option>
+                    </td>
+                    <td class="diff-line-number">
+{% if line.has_head %}
+                      <label title="Comment on new line {{ line.head_line }}">
+                        <input type="radio" name="anchor" value="{{ line.head_anchor }}" aria-label="Comment on new line {{ line.head_line }}" required>
+                        <span>{{ line.head_line }}</span>
+                      </label>
 {% endif %}
-                  </select>
-                </div>
-                <div class="field">
-                  <label for="review-line-{{ file.path_hex }}">Line</label>
-                  <input id="review-line-{{ file.path_hex }}" name="line" type="number" min="1" required>
-                </div>
-              </div>
-              <p class="field-help">Use a line number from the selected side in the diff above.</p>
+                    </td>
+                    <td class="diff-code"><code><span class="visually-hidden">{{ line.kind }}: </span><span class="diff-marker" aria-hidden="true">{{ line.marker }}</span>{{ line.text }}</code></td>
+                  </tr>
+{% endif %}
+{% endfor %}
+                </tbody>
+              </table>
+            </div>
+            <details>
+              <summary>Write a comment on the selected line</summary>
               <div class="field">
                 <label for="review-body-{{ file.path_hex }}">Comment (Markdown)</label>
                 <textarea id="review-body-{{ file.path_hex }}" name="body" maxlength="262144" rows="4" required></textarea>
               </div>
               <button type="submit">Add line comment</button>
-            </form>
-          </details>
+            </details>
+          </form>
+{% else %}
+          <div class="table-scroll pull-request-file-diff">
+            <table class="pull-request-diff-table">
+              <thead>
+                <tr>
+                  <th scope="col">Old</th>
+                  <th scope="col">New</th>
+                  <th scope="col">Code</th>
+                </tr>
+              </thead>
+              <tbody>
+{% for line in file.lines %}
+{% if line.is_hunk || line.is_meta %}
+                <tr class="diff-{{ line.kind }}">
+                  <td colspan="3"><code>{{ line.text }}</code></td>
+                </tr>
+{% else %}
+                <tr class="diff-line diff-{{ line.kind }}">
+                  <td class="diff-line-number">{% if line.has_base %}{{ line.base_line }}{% endif %}</td>
+                  <td class="diff-line-number">{% if line.has_head %}{{ line.head_line }}{% endif %}</td>
+                  <td class="diff-code"><code><span class="visually-hidden">{{ line.kind }}: </span><span class="diff-marker" aria-hidden="true">{{ line.marker }}</span>{{ line.text }}</code></td>
+                </tr>
+{% endif %}
+{% endfor %}
+              </tbody>
+            </table>
+          </div>
 {% endif %}
 {% endif %}
         </article>
@@ -213,8 +266,7 @@
           <input type="hidden" name="csrf" value="{{ csrf }}">
           <input type="hidden" name="revision" value="{{ selected_revision }}">
           <input type="hidden" name="path-hex" value="">
-          <input type="hidden" name="side" value="">
-          <input type="hidden" name="line" value="">
+          <input type="hidden" name="anchor" value="">
           <div class="field">
             <label for="review-body">Review comment (Markdown)</label>
             <textarea id="review-body" name="body" maxlength="262144" rows="6"></textarea>

@@ -4,7 +4,7 @@
   <meta charset="utf-8">
   <meta name="viewport" content="width=device-width, initial-scale=1">
   <title>Repositories · tit</title>
-  <link rel="stylesheet" href="/assets/style.css?v=10">
+  <link rel="stylesheet" href="/assets/style.css?v=14">
 </head>
 <body>
   <a class="skip-link" href="#main">Skip to content</a>

@@ -4,7 +4,7 @@
   <meta charset="utf-8">
   <meta name="viewport" content="width=device-width, initial-scale=1">
   <title>Method not allowed · tit</title>
-  <link rel="stylesheet" href="/assets/style.css?v=10">
+  <link rel="stylesheet" href="/assets/style.css?v=14">
 </head>
 <body>
   <a class="skip-link" href="#main">Skip to content</a>

@@ -4,7 +4,7 @@
   <meta charset="utf-8">
   <meta name="viewport" content="width=device-width, initial-scale=1">
   <title>Page not found · tit</title>
-  <link rel="stylesheet" href="/assets/style.css?v=10">
+  <link rel="stylesheet" href="/assets/style.css?v=14">
 </head>
 <body>
   <a class="skip-link" href="#main">Skip to content</a>