a73x

602505d2

Fix four web UI defects a browser found and the tests could not

a73x   2026-08-12 17:25

Commit message
Fix four web UI defects a browser found and the tests could not

All four were reported from an actual browser pass, and none of them was
visible to the suite: the HTML was well-formed throughout, every content
assertion passed, and three of the four are only wrong once a page is laid
out at a width or read as a sentence.

8e716471 — tables forced the whole page to scroll sideways. Diffs already
got this right: `pre` carries `overflow-x: auto`, so a 1231px diff scrolls
inside its own block. Tables had no equivalent, so a seven-column list at
500px pushed the body out and took Base and Updated off-screen with it.
Every table now sits in a `.table-scroll` container with the same rule.
Two neighbours fell out of the same measurement and are fixed with it: the
overview's `1fr 1fr` grid was narrower than its content on a phone, and a
release's 64-character sha256 is unbreakable text wider than the viewport.

The test for this drives a live server and measures the real property in a
real headless browser — `documentElement.scrollWidth` against
`window.innerWidth` at 500px, the same measurement the report made by hand.
The browser is an external oracle: it does not know what the test expects,
so it cannot be talked into agreeing, which a "the template contains a
wrapper div" assertion would have been. It reproduced the defect at 531 vs
500 before the fix. Where no browser is installed the measurement skips
loudly (`GIT_COLLAB_REQUIRE_BROWSER=1` turns the skip into a failure) and a
weaker structural test still guards the invariant.

b2a57996 — timestamps rendered as stored: 35 characters of RFC3339 with
nine digits of nanoseconds, in every Updated column, the Revisions Date
column and beside every review and inline comment. They now render as
`2026-08-11 16:08` in UTC with the exact stored value in `title=`, so
nothing is lost — it is one hover away. Minutes because that is the
coarsest rendering that still orders a review and the comment answering it;
absolute rather than relative because a relative rendering is a function of
when the page was built, so the same URL would say different things at
different times. The same issue's smaller sibling: `Merged in` rendered a
40-character oid beside a Revisions table rendering 8. Both now go through
one shared helper at `abbrev::MIN_WIDTH`; the link still resolves the full
oid.

c43c459d — an empty filtered list said `No patches.` on a repository with
fifteen, because the default filter is open (correct, from d6d57953) and
they had all landed. That sentence is a claim about the repository and it
was false. It now says `No open patches. (15 total — show all)`, with the
link to `?filter=all`. A genuinely empty repository still says `No patches
yet.` and does not offer to show zero more. Same for issues. The total is
only looked up when the list is empty, so a list with rows on it does not
pay for the archived half of the listing.

850f3b8e — the Branch column showed dead ephemeral names like
`worktree-agent-ab8e151c29539ab74` on 8 of 15 rows. Since 659f0350 that
field is provenance only: nothing resolves through it, and those branches
were deleted with the worktrees that made them. Replaced with the base ref
rather than dropped, because a list reader does have a question there —
"is this targeting main?" — and the base ref answers it, names a branch
that still exists, and is short enough to help the overflow above rather
than cause it. Provenance is not lost: the detail page still shows the
originating branch, and so does `patch show`.

The CLI's `patch list` gets the same target, printed as `→ main`, so the
two surfaces do not disagree about what a patch list says — the argument
10bb2d84 made about ids. It never showed the branch, so this adds the
information rather than removing any.

`--json` is untouched on all four counts and asserted so: full 40-character
ids, full-precision timestamps, and `branch` still present for any script
that wants the provenance the HTML stopped showing. Abbreviation and
shortening are display policy and stay on the display side.

Rendering is still a read: a test walks every page and asserts the served
repository's refs are byte-identical before and after.

Fixes 8e716471, b2a57996, c43c459d, 850f3b8e.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
src/patch.rs
Old New
@@ -217,13 +217,21 @@ pub fn list_to_writer(
217 }; 217 };
218 // `merged?` — not `merged` — for a patch that only looks merged. 218 // `merged?` — not `merged` — for a patch that only looks merged.
219 // Reachability is a hint here, never the recorded status. 219 // Reachability is a hint here, never the recorded status.
220 //
221 // `→ base_ref` is what the web list shows in the column that used to
222 // hold `branch`, and the two surfaces have to agree about what a patch
223 // list says — the argument `10bb2d84` made about ids. It is the target
224 // rather than the originating branch because the originating branch is
225 // provenance only since `659f0350`, and for worktree-created patches it
226 // has usually been deleted. `patch show` still prints it. See 850f3b8e.
220 writeln!( 227 writeln!(
221 writer, 228 writer,
222 "{} {:7} {}{} (by {}){}{}", 229 "{} {:7} {}{} → {} (by {}){}{}",
223 abbrev.of(&p.id), 230 abbrev.of(&p.id),
224 p.status_display(repo), 231 p.status_display(repo),
225 p.title, 232 p.title,
226 labels, 233 labels,
234 p.base_ref,
227 p.author.name, 235 p.author.name,
228 stale, 236 stale,
229 unread 237 unread
src/server/http/mod.rs
Old New
@@ -1,6 +1,7 @@
1 pub mod git_http; 1 pub mod git_http;
2 pub mod repo; 2 pub mod repo;
3 pub mod repo_list; 3 pub mod repo_list;
4 pub mod timestamp;
4 5
5 use axum::extract::{DefaultBodyLimit, Request, State}; 6 use axum::extract::{DefaultBodyLimit, Request, State};
6 use axum::http::uri::{PathAndQuery, Uri}; 7 use axum::http::uri::{PathAndQuery, Uri};
src/server/http/repo/issues.rs
Old New
@@ -4,6 +4,7 @@ use axum::extract::{Path, Query, State};
4 use axum::response::{IntoResponse, Response}; 4 use axum::response::{IntoResponse, Response};
5 5
6 use super::{collab_counts, internal_error, not_found, open_repo, AppState, CommentView}; 6 use super::{collab_counts, internal_error, not_found, open_repo, AppState, CommentView};
7 use crate::http::timestamp::Timestamp;
7 8
8 /// Which issues to include in the list view. Defaults to `Open` so the list 9 /// Which issues to include in the list view. Defaults to `Open` so the list
9 /// agrees with the nav badge, which only ever counts open issues. Closed and 10 /// agrees with the nav badge, which only ever counts open issues. Closed and
@@ -46,7 +47,7 @@ pub struct IssueListItem {
46 pub title: String, 47 pub title: String,
47 pub author: String, 48 pub author: String,
48 pub labels: String, 49 pub labels: String,
49 pub updated: String, 50 pub updated: Timestamp,
50 } 51 }
51 52
52 #[derive(Debug)] 53 #[derive(Debug)]
@@ -71,6 +72,9 @@ pub struct IssuesTemplate {
71 pub open_issues: usize, 72 pub open_issues: usize,
72 pub filter: &'static str, 73 pub filter: &'static str,
73 pub issues: Vec<IssueListItem>, 74 pub issues: Vec<IssueListItem>,
75 /// How many issues the repository holds regardless of filter, used only to
76 /// explain an empty list. See `patches.rs` and issue `c43c459d`.
77 pub total: usize,
74 } 78 }
75 79
76 #[derive(askama::Template, askama_web::WebTemplate)] 80 #[derive(askama::Template, askama_web::WebTemplate)]
@@ -124,7 +128,7 @@ pub async fn issues(
124 // `Issue:` trailer, a branch name, a script. See `abbrev`. 128 // `Issue:` trailer, a branch name, a script. See `abbrev`.
125 let abbrev = git_collab::abbrev::for_issues(&repo); 129 let abbrev = git_collab::abbrev::for_issues(&repo);
126 130
127 let issues = filtered_issues 131 let issues: Vec<IssueListItem> = filtered_issues
128 .into_iter() 132 .into_iter()
129 .map(|i| { 133 .map(|i| {
130 let id = i.id.clone(); 134 let id = i.id.clone();
@@ -136,11 +140,21 @@ pub async fn issues(
136 title: i.title, 140 title: i.title,
137 author: i.author.name, 141 author: i.author.name,
138 labels: i.labels.join(", "), 142 labels: i.labels.join(", "),
139 updated: i.last_updated, 143 updated: Timestamp::new(i.last_updated),
140 } 144 }
141 }) 145 })
142 .collect(); 146 .collect();
143 147
148 // Only looked up when the list is empty; see the matching comment in
149 // `patches.rs`.
150 let total = if issues.is_empty() {
151 git_collab::state::list_issues_with_archived(&repo)
152 .unwrap_or_default()
153 .len()
154 } else {
155 issues.len()
156 };
157
144 IssuesTemplate { 158 IssuesTemplate {
145 site_title: state.site_title.clone(), 159 site_title: state.site_title.clone(),
146 repo_name, 160 repo_name,
@@ -149,6 +163,7 @@ pub async fn issues(
149 open_issues, 163 open_issues,
150 filter: filter.as_str(), 164 filter: filter.as_str(),
151 issues, 165 issues,
166 total,
152 } 167 }
153 .into_response() 168 .into_response()
154 } 169 }
src/server/http/repo/mod.rs
Old New
@@ -19,6 +19,7 @@ use axum::http::StatusCode;
19 use axum::response::{IntoResponse, Response}; 19 use axum::response::{IntoResponse, Response};
20 use chrono::{TimeZone, Utc}; 20 use chrono::{TimeZone, Utc};
21 21
22 use super::timestamp::Timestamp;
22 use super::AppState; 23 use super::AppState;
23 24
24 #[derive(askama::Template, askama_web::WebTemplate)] 25 #[derive(askama::Template, askama_web::WebTemplate)]
@@ -130,7 +131,7 @@ pub struct CommentView {
130 /// deleted text is dropped in the fold and never reaches this far, so 131 /// deleted text is dropped in the fold and never reaches this far, so
131 /// there is nothing here for a template bug to leak. 132 /// there is nothing here for a template bug to leak.
132 pub body: String, 133 pub body: String,
133 pub timestamp: String, 134 pub timestamp: Timestamp,
134 pub edited: bool, 135 pub edited: bool,
135 pub deleted: bool, 136 pub deleted: bool,
136 } 137 }
@@ -140,7 +141,7 @@ impl CommentView {
140 CommentView { 141 CommentView {
141 author: c.author.name, 142 author: c.author.name,
142 body: c.body, 143 body: c.body,
143 timestamp: c.timestamp, 144 timestamp: Timestamp::new(c.timestamp),
144 edited: c.edited, 145 edited: c.edited,
145 deleted: c.deleted, 146 deleted: c.deleted,
146 } 147 }
src/server/http/repo/patches.rs
Old New
@@ -4,6 +4,7 @@ use axum::extract::{Path, Query, State};
4 use axum::response::{IntoResponse, Response}; 4 use axum::response::{IntoResponse, Response};
5 5
6 use super::{collab_counts, internal_error, not_found, open_repo, AppState, CommentView}; 6 use super::{collab_counts, internal_error, not_found, open_repo, AppState, CommentView};
7 use crate::http::timestamp::Timestamp;
7 8
8 /// Which patches to include in the list view. Defaults to `Open` so the list 9 /// Which patches to include in the list view. Defaults to `Open` so the list
9 /// agrees with the nav badge, which only ever counts open patches. Merged 10 /// agrees with the nav badge, which only ever counts open patches. Merged
@@ -51,8 +52,16 @@ pub struct PatchListItem {
51 pub title: String, 52 pub title: String,
52 pub author: String, 53 pub author: String,
53 pub labels: String, 54 pub labels: String,
54 pub branch: String, 55 /// What the patch targets. This column used to carry `PatchState.branch`,
55 pub updated: String, 56 /// which since `659f0350` is provenance only: nothing resolves through it,
57 /// and for work done in an agent worktree the branch was deleted with the
58 /// worktree, so the column filled up with names for things that no longer
59 /// exist. The base ref is the question a reader of a *list* actually has —
60 /// "is this targeting main?" — and it names a branch that is still there.
61 /// The originating branch is still on the detail page, where provenance
62 /// belongs. See issue `850f3b8e`.
63 pub base_ref: String,
64 pub updated: Timestamp,
56 } 65 }
57 66
58 #[derive(askama::Template, askama_web::WebTemplate)] 67 #[derive(askama::Template, askama_web::WebTemplate)]
@@ -65,6 +74,16 @@ pub struct PatchesTemplate {
65 pub open_issues: usize, 74 pub open_issues: usize,
66 pub filter: &'static str, 75 pub filter: &'static str,
67 pub patches: Vec<PatchListItem>, 76 pub patches: Vec<PatchListItem>,
77 /// How many patches the repository holds regardless of filter.
78 ///
79 /// Read *only* by the empty-list message, which is the only place the page
80 /// has anything to say about a number it is not showing. A list with rows
81 /// on it therefore does not go looking for one: the archived namespace is
82 /// the expensive half of the listing, and the `Open` filter deliberately
83 /// avoids it. When rows are present this holds their count instead, which
84 /// is the true total under the `All` filter and unread under the others.
85 /// See issue `c43c459d`.
86 pub total: usize,
68 } 87 }
69 88
70 #[derive(Debug)] 89 #[derive(Debug)]
@@ -77,7 +96,7 @@ pub struct RevisionView {
77 /// the request handler and drop the connection. The `None` case renders as 96 /// the request handler and drop the connection. The `None` case renders as
78 /// `state::UNKNOWN_COMMIT`, spelled out in the template. 97 /// `state::UNKNOWN_COMMIT`, spelled out in the template.
79 pub short_commit: Option<String>, 98 pub short_commit: Option<String>,
80 pub timestamp: String, 99 pub timestamp: Timestamp,
81 pub body: Option<String>, 100 pub body: Option<String>,
82 } 101 }
83 102
@@ -86,7 +105,7 @@ pub struct ReviewView {
86 pub author: String, 105 pub author: String,
87 pub verdict: String, 106 pub verdict: String,
88 pub body: String, 107 pub body: String,
89 pub timestamp: String, 108 pub timestamp: Timestamp,
90 pub revision: Option<u32>, 109 pub revision: Option<u32>,
91 pub edited: bool, 110 pub edited: bool,
92 } 111 }
@@ -97,7 +116,7 @@ pub struct InlineCommentView {
97 pub file: String, 116 pub file: String,
98 pub line: u32, 117 pub line: u32,
99 pub body: String, 118 pub body: String,
100 pub timestamp: String, 119 pub timestamp: Timestamp,
101 pub revision: Option<u32>, 120 pub revision: Option<u32>,
102 pub edited: bool, 121 pub edited: bool,
103 pub deleted: bool, 122 pub deleted: bool,
@@ -123,6 +142,11 @@ pub struct PatchDetailView {
123 /// away from where the code actually went. Recorded only — never inferred 142 /// away from where the code actually went. Recorded only — never inferred
124 /// from reachability, which points at whatever happens to be reachable. 143 /// from reachability, which points at whatever happens to be reachable.
125 pub merge_commit: Option<String>, 144 pub merge_commit: Option<String>,
145 /// The same oid at the width the Revisions table beside it uses. The link
146 /// still carries `merge_commit` in full; only the text is abbreviated, so
147 /// the two commit oids on this page stop being displayed at 40 and 8
148 /// characters respectively. See issue `b2a57996`.
149 pub short_merge_commit: Option<String>,
126 pub author: String, 150 pub author: String,
127 pub labels: String, 151 pub labels: String,
128 pub branch: String, 152 pub branch: String,
@@ -187,7 +211,7 @@ pub async fn patches(
187 // see the matching comment in `issues.rs`. 211 // see the matching comment in `issues.rs`.
188 let abbrev = git_collab::abbrev::for_patches(&repo); 212 let abbrev = git_collab::abbrev::for_patches(&repo);
189 213
190 let patches = filtered_patches 214 let patches: Vec<PatchListItem> = filtered_patches
191 .into_iter() 215 .into_iter()
192 .map(|p| { 216 .map(|p| {
193 let id = p.id.clone(); 217 let id = p.id.clone();
@@ -199,12 +223,24 @@ pub async fn patches(
199 title: p.title, 223 title: p.title,
200 author: p.author.name, 224 author: p.author.name,
201 labels: p.labels.join(", "), 225 labels: p.labels.join(", "),
202 branch: p.branch, 226 base_ref: p.base_ref,
203 updated: p.last_updated, 227 updated: Timestamp::new(p.last_updated),
204 } 228 }
205 }) 229 })
206 .collect(); 230 .collect();
207 231
232 // Only an empty list needs to know the repository's total: "No open
233 // patches" is a claim about the filter, and it is only honest if the page
234 // can also say what the filter is hiding. A page with rows on it says
235 // nothing about the total, so it does not go looking for one.
236 let total = if patches.is_empty() {
237 git_collab::state::list_patches_with_archived(&repo)
238 .unwrap_or_default()
239 .len()
240 } else {
241 patches.len()
242 };
243
208 PatchesTemplate { 244 PatchesTemplate {
209 site_title: state.site_title.clone(), 245 site_title: state.site_title.clone(),
210 repo_name, 246 repo_name,
@@ -213,6 +249,7 @@ pub async fn patches(
213 open_issues, 249 open_issues,
214 filter: filter.as_str(), 250 filter: filter.as_str(),
215 patches, 251 patches,
252 total,
216 } 253 }
217 .into_response() 254 .into_response()
218 } 255 }
@@ -242,6 +279,10 @@ pub async fn patch_detail(
242 title: ps.title, 279 title: ps.title,
243 body: ps.body, 280 body: ps.body,
244 status: ps.status.as_str().to_string(), 281 status: ps.status.as_str().to_string(),
282 short_merge_commit: ps
283 .merge_commit
284 .as_deref()
285 .and_then(git_collab::state::short_commit_oid),
245 merge_commit: ps.merge_commit, 286 merge_commit: ps.merge_commit,
246 author: ps.author.name, 287 author: ps.author.name,
247 labels: ps.labels.join(", "), 288 labels: ps.labels.join(", "),
@@ -254,7 +295,7 @@ pub async fn patch_detail(
254 number: r.number, 295 number: r.number,
255 short_commit: r.short_commit(), 296 short_commit: r.short_commit(),
256 commit: r.commit, 297 commit: r.commit,
257 timestamp: r.timestamp, 298 timestamp: Timestamp::new(r.timestamp),
258 body: r.body, 299 body: r.body,
259 }) 300 })
260 .collect(), 301 .collect(),
@@ -265,7 +306,7 @@ pub async fn patch_detail(
265 author: r.author.name, 306 author: r.author.name,
266 verdict: r.verdict.as_str().to_string(), 307 verdict: r.verdict.as_str().to_string(),
267 body: r.body, 308 body: r.body,
268 timestamp: r.timestamp, 309 timestamp: Timestamp::new(r.timestamp),
269 revision: r.revision, 310 revision: r.revision,
270 edited: r.edited, 311 edited: r.edited,
271 }) 312 })
@@ -278,7 +319,7 @@ pub async fn patch_detail(
278 file: ic.file, 319 file: ic.file,
279 line: ic.line, 320 line: ic.line,
280 body: ic.body, 321 body: ic.body,
281 timestamp: ic.timestamp, 322 timestamp: Timestamp::new(ic.timestamp),
282 revision: ic.revision, 323 revision: ic.revision,
283 non_blocking: ic.non_blocking, 324 non_blocking: ic.non_blocking,
284 edited: ic.edited, 325 edited: ic.edited,
src/server/http/templates/base.html
Old New
@@ -13,6 +13,16 @@
13 th { font-weight: normal; color: #666; } 13 th { font-weight: normal; color: #666; }
14 td { border-top: 1px solid #eee; } 14 td { border-top: 1px solid #eee; }
15 pre { padding: 8px; background: #f8f8f8; overflow-x: auto; } 15 pre { padding: 8px; background: #f8f8f8; overflow-x: auto; }
16 /* A table wider than the viewport scrolls inside its own block, exactly as
17 a wide diff already does in `pre` above. Without this the table pushes
18 the body out and the whole page scrolls sideways, which takes the header
19 and every other row's leading columns off-screen with it. See issue
20 8e716471; the property is measured in a real browser by
21 tests/web_layout_test.rs. */
22 .table-scroll { overflow-x: auto; }
23 /* A timestamp is rendered short with the exact value on hover, so it must
24 not be the thing that wraps a row onto two lines. See issue b2a57996. */
25 .timestamp { white-space: nowrap; }
16 .badge { font-size: 11px; color: #666; } 26 .badge { font-size: 11px; color: #666; }
17 .status-open { color: green; } 27 .status-open { color: green; }
18 .status-closed { color: red; } 28 .status-closed { color: red; }
@@ -22,10 +32,10 @@
22 .diff-file { font-weight: bold; border-top: 1px solid #ccc; padding-top: 8px; } 32 .diff-file { font-weight: bold; border-top: 1px solid #ccc; padding-top: 8px; }
23 .diff-ctx { color: #24292f; background: #fff; } 33 .diff-ctx { color: #24292f; background: #fff; }
24 .diff-empty { background: #f6f8fa; color: transparent; } 34 .diff-empty { background: #f6f8fa; color: transparent; }
25 .header { padding: 8px 16px; border-bottom: 1px solid #ccc; display: flex; align-items: baseline; gap: 16px; } 35 .header { padding: 8px 16px; border-bottom: 1px solid #ccc; display: flex; align-items: baseline; gap: 16px; flex-wrap: wrap; }
26 .header a { text-decoration: none; } 36 .header a { text-decoration: none; }
27 .header .site { font-weight: bold; } 37 .header .site { font-weight: bold; }
28 .header nav { display: flex; gap: 12px; } 38 .header nav { display: flex; gap: 12px; flex-wrap: wrap; }
29 .header nav a.active { font-weight: bold; } 39 .header nav a.active { font-weight: bold; }
30 .filter-bar { color: #666; margin-bottom: 12px; } 40 .filter-bar { color: #666; margin-bottom: 12px; }
31 .filter-bar a { margin-left: 8px; } 41 .filter-bar a { margin-left: 8px; }
src/server/http/templates/commits.html
Old New
@@ -13,6 +13,7 @@
13 {% if commits.is_empty() %} 13 {% if commits.is_empty() %}
14 <p style="color: #666;">No commits yet.</p> 14 <p style="color: #666;">No commits yet.</p>
15 {% else %} 15 {% else %}
16 <div class="table-scroll">
16 <table> 17 <table>
17 <thead> 18 <thead>
18 <tr> 19 <tr>
@@ -33,5 +34,6 @@
33 {% endfor %} 34 {% endfor %}
34 </tbody> 35 </tbody>
35 </table> 36 </table>
37 </div>
36 {% endif %} 38 {% endif %}
37 {% endblock %} 39 {% endblock %}
src/server/http/templates/issue_detail.html
Old New
@@ -27,7 +27,7 @@
27 <div style="border: 1px solid #ddd; border-radius: 4px; padding: 12px; margin-bottom: 12px;"> 27 <div style="border: 1px solid #ddd; border-radius: 4px; padding: 12px; margin-bottom: 12px;">
28 <p style="margin: 0 0 8px 0;"> 28 <p style="margin: 0 0 8px 0;">
29 <strong>{{ comment.author }}</strong> 29 <strong>{{ comment.author }}</strong>
30 &nbsp; <span class="mono" style="color: #666; font-size: 0.85em;">{{ comment.timestamp }}</span> 30 &nbsp; <span class="mono timestamp" style="color: #666; font-size: 0.85em;" title="{{ comment.timestamp.full }}">{{ comment.timestamp.short }}</span>
31 {% if comment.edited %}&nbsp; <span style="color: #666; font-size: 0.85em;">(edited)</span>{% endif %} 31 {% if comment.edited %}&nbsp; <span style="color: #666; font-size: 0.85em;">(edited)</span>{% endif %}
32 </p> 32 </p>
33 {% if comment.deleted %} 33 {% if comment.deleted %}
src/server/http/templates/issues.html
Old New
@@ -11,8 +11,15 @@
11 <a href="/{{ repo_name }}/issues?filter=all"{% if filter == "all" %} class="active"{% endif %}>all</a> 11 <a href="/{{ repo_name }}/issues?filter=all"{% if filter == "all" %} class="active"{% endif %}>all</a>
12 </p> 12 </p>
13 {% if issues.is_empty() %} 13 {% if issues.is_empty() %}
14 <p style="color: #666;">No issues.</p> 14 {# See the matching comment in patches.html, and issue c43c459d. #}
15 {% if total == 0 %}
16 <p style="color: #666;">No issues yet.</p>
15 {% else %} 17 {% else %}
18 <p style="color: #666;">No {{ filter }} issues. ({{ total }} total —
19 <a href="/{{ repo_name }}/issues?filter=all">show all</a>)</p>
20 {% endif %}
21 {% else %}
22 <div class="table-scroll">
16 <table> 23 <table>
17 <thead> 24 <thead>
18 <tr> 25 <tr>
@@ -32,10 +39,11 @@
32 <td><a href="/{{ repo_name }}/issues/{{ i.id }}">{{ i.title }}</a></td> 39 <td><a href="/{{ repo_name }}/issues/{{ i.id }}">{{ i.title }}</a></td>
33 <td style="color: #666;">{{ i.author }}</td> 40 <td style="color: #666;">{{ i.author }}</td>
34 <td style="color: #666;">{{ i.labels }}</td> 41 <td style="color: #666;">{{ i.labels }}</td>
35 <td class="mono" style="color: #666;">{{ i.updated }}</td> 42 <td class="mono timestamp" style="color: #666;" title="{{ i.updated.full }}">{{ i.updated.short }}</td>
36 </tr> 43 </tr>
37 {% endfor %} 44 {% endfor %}
38 </tbody> 45 </tbody>
39 </table> 46 </table>
47 </div>
40 {% endif %} 48 {% endif %}
41 {% endblock %} 49 {% endblock %}
src/server/http/templates/patch_detail.html
Old New
@@ -10,7 +10,9 @@
10 &nbsp; <span class="mono" style="color: #666;">{{ patch.branch }} → {{ patch.base_ref }}</span> 10 &nbsp; <span class="mono" style="color: #666;">{{ patch.branch }} → {{ patch.base_ref }}</span>
11 </p> 11 </p>
12 {% if let Some(merge_commit) = patch.merge_commit %} 12 {% if let Some(merge_commit) = patch.merge_commit %}
13 <p>Merged in <a class="mono" href="/{{ repo_name }}/diff/{{ merge_commit }}">{{ merge_commit }}</a></p> 13 {# The link resolves the full oid; the text matches the Revisions table below.
14 See issue b2a57996. #}
15 <p>Merged in <a class="mono" href="/{{ repo_name }}/diff/{{ merge_commit }}" title="{{ merge_commit }}">{% if let Some(short) = patch.short_merge_commit %}{{ short }}{% else %}{{ merge_commit }}{% endif %}</a></p>
14 {% endif %} 16 {% endif %}
15 {% if !patch.labels.is_empty() %} 17 {% if !patch.labels.is_empty() %}
16 <p style="color: #666;">Labels: {{ patch.labels }}</p> 18 <p style="color: #666;">Labels: {{ patch.labels }}</p>
@@ -23,6 +25,7 @@
23 {% if patch.revisions.is_empty() %} 25 {% if patch.revisions.is_empty() %}
24 <p style="color: #666;">No revisions.</p> 26 <p style="color: #666;">No revisions.</p>
25 {% else %} 27 {% else %}
28 <div class="table-scroll">
26 <table> 29 <table>
27 <thead> 30 <thead>
28 <tr> 31 <tr>
@@ -37,12 +40,13 @@
37 <tr> 40 <tr>
38 <td>{{ rev.number }}</td> 41 <td>{{ rev.number }}</td>
39 <td class="mono">{% if let Some(short) = rev.short_commit %}<a href="/{{ repo_name }}/diff/{{ rev.commit }}">{{ short }}</a>{% else %}<span style="color: #666;">unknown</span>{% endif %}</td> 42 <td class="mono">{% if let Some(short) = rev.short_commit %}<a href="/{{ repo_name }}/diff/{{ rev.commit }}">{{ short }}</a>{% else %}<span style="color: #666;">unknown</span>{% endif %}</td>
40 <td class="mono" style="color: #666;">{{ rev.timestamp }}</td> 43 <td class="mono timestamp" style="color: #666;" title="{{ rev.timestamp.full }}">{{ rev.timestamp.short }}</td>
41 <td>{% if let Some(body) = rev.body %}{{ body }}{% endif %}</td> 44 <td>{% if let Some(body) = rev.body %}{{ body }}{% endif %}</td>
42 </tr> 45 </tr>
43 {% endfor %} 46 {% endfor %}
44 </tbody> 47 </tbody>
45 </table> 48 </table>
49 </div>
46 {% endif %} 50 {% endif %}
47 51
48 {% if !patch.reviews.is_empty() %} 52 {% if !patch.reviews.is_empty() %}
@@ -52,7 +56,7 @@
52 <p style="margin: 0 0 8px 0;"> 56 <p style="margin: 0 0 8px 0;">
53 <strong>{{ review.author }}</strong> 57 <strong>{{ review.author }}</strong>
54 &nbsp; <span class="status-{{ review.verdict }}">{{ review.verdict }}</span> 58 &nbsp; <span class="status-{{ review.verdict }}">{{ review.verdict }}</span>
55 &nbsp; <span class="mono" style="color: #666; font-size: 0.85em;">{{ review.timestamp }}</span> 59 &nbsp; <span class="mono timestamp" style="color: #666; font-size: 0.85em;" title="{{ review.timestamp.full }}">{{ review.timestamp.short }}</span>
56 {% if let Some(rev) = review.revision %}&nbsp; rev {{ rev }}{% endif %} 60 {% if let Some(rev) = review.revision %}&nbsp; rev {{ rev }}{% endif %}
57 {% if review.edited %}&nbsp; <span style="color: #666; font-size: 0.85em;">(edited)</span>{% endif %} 61 {% if review.edited %}&nbsp; <span style="color: #666; font-size: 0.85em;">(edited)</span>{% endif %}
58 </p> 62 </p>
@@ -70,7 +74,7 @@
70 <p style="margin: 0 0 8px 0;"> 74 <p style="margin: 0 0 8px 0;">
71 <strong>{{ ic.author }}</strong> 75 <strong>{{ ic.author }}</strong>
72 &nbsp; <span class="mono" style="color: #666;">{{ ic.file }}:{{ ic.line }}</span> 76 &nbsp; <span class="mono" style="color: #666;">{{ ic.file }}:{{ ic.line }}</span>
73 &nbsp; <span class="mono" style="color: #666; font-size: 0.85em;">{{ ic.timestamp }}</span> 77 &nbsp; <span class="mono timestamp" style="color: #666; font-size: 0.85em;" title="{{ ic.timestamp.full }}">{{ ic.timestamp.short }}</span>
74 {% if let Some(rev) = ic.revision %}&nbsp; rev {{ rev }}{% endif %} 78 {% if let Some(rev) = ic.revision %}&nbsp; rev {{ rev }}{% endif %}
75 {% if ic.edited %}&nbsp; <span style="color: #666; font-size: 0.85em;">(edited)</span>{% endif %} 79 {% if ic.edited %}&nbsp; <span style="color: #666; font-size: 0.85em;">(edited)</span>{% endif %}
76 {% if ic.non_blocking %}&nbsp; <span style="color: #666; font-size: 0.85em;">[non-blocking]</span>{% endif %} 80 {% if ic.non_blocking %}&nbsp; <span style="color: #666; font-size: 0.85em;">[non-blocking]</span>{% endif %}
@@ -91,7 +95,7 @@
91 <div style="border: 1px solid #ddd; border-radius: 4px; padding: 12px; margin-bottom: 12px;"> 95 <div style="border: 1px solid #ddd; border-radius: 4px; padding: 12px; margin-bottom: 12px;">
92 <p style="margin: 0 0 8px 0;"> 96 <p style="margin: 0 0 8px 0;">
93 <strong>{{ comment.author }}</strong> 97 <strong>{{ comment.author }}</strong>
94 &nbsp; <span class="mono" style="color: #666; font-size: 0.85em;">{{ comment.timestamp }}</span> 98 &nbsp; <span class="mono timestamp" style="color: #666; font-size: 0.85em;" title="{{ comment.timestamp.full }}">{{ comment.timestamp.short }}</span>
95 {% if comment.edited %}&nbsp; <span style="color: #666; font-size: 0.85em;">(edited)</span>{% endif %} 99 {% if comment.edited %}&nbsp; <span style="color: #666; font-size: 0.85em;">(edited)</span>{% endif %}
96 </p> 100 </p>
97 {% if comment.deleted %} 101 {% if comment.deleted %}
src/server/http/templates/patches.html
Old New
@@ -12,8 +12,18 @@
12 <a href="/{{ repo_name }}/patches?filter=all"{% if filter == "all" %} class="active"{% endif %}>all</a> 12 <a href="/{{ repo_name }}/patches?filter=all"{% if filter == "all" %} class="active"{% endif %}>all</a>
13 </p> 13 </p>
14 {% if patches.is_empty() %} 14 {% if patches.is_empty() %}
15 <p style="color: #666;">No patches.</p> 15 {# An empty *filtered* list is not an empty repository. Saying "No patches."
16 under the default `open` filter was simply false on a repo whose work had
17 all landed, and the only clue was one bold word among four links. See
18 issue c43c459d. #}
19 {% if total == 0 %}
20 <p style="color: #666;">No patches yet.</p>
16 {% else %} 21 {% else %}
22 <p style="color: #666;">No {{ filter }} patches. ({{ total }} total —
23 <a href="/{{ repo_name }}/patches?filter=all">show all</a>)</p>
24 {% endif %}
25 {% else %}
26 <div class="table-scroll">
17 <table> 27 <table>
18 <thead> 28 <thead>
19 <tr> 29 <tr>
@@ -22,7 +32,7 @@
22 <th>Title</th> 32 <th>Title</th>
23 <th>Author</th> 33 <th>Author</th>
24 <th>Labels</th> 34 <th>Labels</th>
25 <th>Branch</th> 35 <th>Base</th>
26 <th>Updated</th> 36 <th>Updated</th>
27 </tr> 37 </tr>
28 </thead> 38 </thead>
@@ -34,11 +44,12 @@
34 <td><a href="/{{ repo_name }}/patches/{{ p.id }}">{{ p.title }}</a></td> 44 <td><a href="/{{ repo_name }}/patches/{{ p.id }}">{{ p.title }}</a></td>
35 <td style="color: #666;">{{ p.author }}</td> 45 <td style="color: #666;">{{ p.author }}</td>
36 <td style="color: #666;">{{ p.labels }}</td> 46 <td style="color: #666;">{{ p.labels }}</td>
37 <td class="mono" style="color: #666;">{{ p.branch }}</td> 47 <td class="mono" style="color: #666;">{{ p.base_ref }}</td>
38 <td class="mono" style="color: #666;">{{ p.updated }}</td> 48 <td class="mono timestamp" style="color: #666;" title="{{ p.updated.full }}">{{ p.updated.short }}</td>
39 </tr> 49 </tr>
40 {% endfor %} 50 {% endfor %}
41 </tbody> 51 </tbody>
42 </table> 52 </table>
53 </div>
43 {% endif %} 54 {% endif %}
44 {% endblock %} 55 {% endblock %}
src/server/http/templates/releases.html
Old New
@@ -14,7 +14,9 @@
14 <section> 14 <section>
15 <h3>{{ v.version }}</h3> 15 <h3>{{ v.version }}</h3>
16 <p style="color: #666;">{{ v.published }}</p> 16 <p style="color: #666;">{{ v.published }}</p>
17 <ul> 17 {# A sha256 is 64 unbreakable characters, wider than a phone. It is allowed
18 to wrap mid-string rather than push the page sideways — see 8e716471. #}
19 <ul style="overflow-wrap: anywhere;">
18 {% for f in v.files %} 20 {% for f in v.files %}
19 <li> 21 <li>
20 {% if downloads_available %} 22 {% if downloads_available %}
src/server/http/templates/repo_list.html
Old New
@@ -6,6 +6,7 @@
6 {% if repos.is_empty() %} 6 {% if repos.is_empty() %}
7 <p style="color: #666;">No repositories found.</p> 7 <p style="color: #666;">No repositories found.</p>
8 {% else %} 8 {% else %}
9 <div class="table-scroll">
9 <table> 10 <table>
10 <thead> 11 <thead>
11 <tr> 12 <tr>
@@ -24,6 +25,7 @@
24 {% endfor %} 25 {% endfor %}
25 </tbody> 26 </tbody>
26 </table> 27 </table>
28 </div>
27 {% endif %} 29 {% endif %}
28 </div> 30 </div>
29 {% endblock %} 31 {% endblock %}
src/server/http/templates/repo_overview.html
Old New
@@ -9,12 +9,16 @@
9 <div class="readme-body">{{ r.html|safe }}</div> 9 <div class="readme-body">{{ r.html|safe }}</div>
10 </div> 10 </div>
11 {% endif %} 11 {% endif %}
12 <div style="display: grid; grid-template-columns: 1fr 1fr; gap: 24px; margin-bottom: 32px;"> 12 {# `minmax(min(100%, 22rem), 1fr)` rather than `1fr 1fr`: two fixed columns are
13 narrower than their content on a phone, and each half then pushed the page
14 sideways on its own. This collapses to one column below ~44rem. #}
15 <div style="display: grid; grid-template-columns: repeat(auto-fit, minmax(min(100%, 22rem), 1fr)); gap: 24px; margin-bottom: 32px;">
13 <div> 16 <div>
14 <h3 style="margin-top: 0;">Open Patches</h3> 17 <h3 style="margin-top: 0;">Open Patches</h3>
15 {% if patches.is_empty() %} 18 {% if patches.is_empty() %}
16 <p style="color: #666;">No open patches.</p> 19 <p style="color: #666;">No open patches.</p>
17 {% else %} 20 {% else %}
21 <div class="table-scroll">
18 <table> 22 <table>
19 <thead> 23 <thead>
20 <tr> 24 <tr>
@@ -33,6 +37,7 @@
33 {% endfor %} 37 {% endfor %}
34 </tbody> 38 </tbody>
35 </table> 39 </table>
40 </div>
36 {% endif %} 41 {% endif %}
37 </div> 42 </div>
38 43
@@ -41,6 +46,7 @@
41 {% if issues.is_empty() %} 46 {% if issues.is_empty() %}
42 <p style="color: #666;">No open issues.</p> 47 <p style="color: #666;">No open issues.</p>
43 {% else %} 48 {% else %}
49 <div class="table-scroll">
44 <table> 50 <table>
45 <thead> 51 <thead>
46 <tr> 52 <tr>
@@ -59,6 +65,7 @@
59 {% endfor %} 65 {% endfor %}
60 </tbody> 66 </tbody>
61 </table> 67 </table>
68 </div>
62 {% endif %} 69 {% endif %}
63 </div> 70 </div>
64 </div> 71 </div>
@@ -68,6 +75,7 @@
68 {% if commits.is_empty() %} 75 {% if commits.is_empty() %}
69 <p style="color: #666;">No commits yet.</p> 76 <p style="color: #666;">No commits yet.</p>
70 {% else %} 77 {% else %}
78 <div class="table-scroll">
71 <table> 79 <table>
72 <thead> 80 <thead>
73 <tr> 81 <tr>
@@ -88,6 +96,7 @@
88 {% endfor %} 96 {% endfor %}
89 </tbody> 97 </tbody>
90 </table> 98 </table>
99 </div>
91 {% endif %} 100 {% endif %}
92 </div> 101 </div>
93 {% endblock %} 102 {% endblock %}
src/server/http/templates/tree.html
Old New
@@ -11,6 +11,7 @@
11 </select> 11 </select>
12 {% if !path_display.is_empty() %} / {{ path_display }}{% endif %} 12 {% if !path_display.is_empty() %} / {{ path_display }}{% endif %}
13 </h2> 13 </h2>
14 <div class="table-scroll">
14 <table> 15 <table>
15 <thead> 16 <thead>
16 <tr> 17 <tr>
@@ -38,4 +39,5 @@
38 {% endfor %} 39 {% endfor %}
39 </tbody> 40 </tbody>
40 </table> 41 </table>
42 </div>
41 {% endblock %} 43 {% endblock %}
src/server/http/timestamp.rs
Old New
@@ -0,0 +1,117 @@
1 //! How a stored timestamp goes onto a page.
2 //!
3 //! Events store `chrono::Utc::now().to_rfc3339()`, which is 35 characters of
4 //! which nine are nanoseconds:
5 //!
6 //! ```text
7 //! 2026-08-11T16:08:31.458124482+00:00
8 //! ```
9 //!
10 //! That is the right thing to *store* — it is exact, sortable and unambiguous
11 //! about its offset — and the wrong thing to put in a table cell. At 35
12 //! characters it was the widest column after the title, it wrapped every row
13 //! onto two lines, and the nine digits of precision are not a number any
14 //! reader has ever needed. See issue `b2a57996`.
15 //!
16 //! So a timestamp reaching a template is split in two: a short form for the
17 //! cell and the stored form for `title=`, which browsers surface on hover.
18 //! Nothing is discarded — the exact value is still on the page, one hover
19 //! away, and `--json` is untouched and still carries it in full.
20 //!
21 //! # Why minutes, and why absolute
22 //!
23 //! Minute precision is the coarsest rendering that still orders two events on
24 //! the same day, which is the common case for a review and the comment
25 //! answering it. Seconds add three characters and settle nothing a reader
26 //! cares about.
27 //!
28 //! Absolute rather than relative ("3 days ago") because a relative rendering
29 //! is a function of when the page was built, so the same URL says different
30 //! things at different times: it cannot be cached, cannot be compared between
31 //! two rows rendered from different requests, and cannot be asserted by a test
32 //! without freezing a clock. The exact instant is what was recorded; the page
33 //! should say it.
34 //!
35 //! # Why UTC
36 //!
37 //! The server cannot know the reader's timezone, and a page rendered in the
38 //! *server's* zone would silently disagree with the same page rendered
39 //! elsewhere. Every timestamp this project writes is already `+00:00`, so
40 //! normalizing to UTC changes nothing in practice and fixes the display for a
41 //! value that arrived with some other offset from a repository synced in.
42
43 /// A stored timestamp, split into what a cell shows and what it stands for.
44 #[derive(Debug, Clone)]
45 pub struct Timestamp {
46 /// Exactly as stored — the `title` attribute, so nothing is lost.
47 pub full: String,
48 /// `YYYY-MM-DD HH:MM` in UTC.
49 pub short: String,
50 }
51
52 impl Timestamp {
53 /// Split a stored RFC3339 timestamp for display.
54 ///
55 /// An unparseable value renders as itself. A timestamp this project did
56 /// not write is still a fact about the object, and a page that dropped it
57 /// or printed a placeholder would be hiding the one thing that could
58 /// explain the row. Ugly beats absent.
59 pub fn new(stored: impl Into<String>) -> Self {
60 let full = stored.into();
61 let short = chrono::DateTime::parse_from_rfc3339(&full)
62 .map(|dt| {
63 dt.with_timezone(&chrono::Utc)
64 .format("%Y-%m-%d %H:%M")
65 .to_string()
66 })
67 .unwrap_or_else(|_| full.clone());
68 Timestamp { full, short }
69 }
70 }
71
72 #[cfg(test)]
73 mod tests {
74 use super::*;
75
76 #[test]
77 fn nanoseconds_and_the_offset_are_dropped_from_the_short_form() {
78 let t = Timestamp::new("2026-08-11T16:08:31.458124482+00:00");
79 assert_eq!(t.short, "2026-08-11 16:08");
80 assert_eq!(t.short.len(), 16);
81 }
82
83 #[test]
84 fn the_stored_value_survives_for_the_title_attribute() {
85 let stored = "2026-08-11T16:08:31.458124482+00:00";
86 assert_eq!(Timestamp::new(stored).full, stored);
87 }
88
89 /// A value that arrived with a non-UTC offset must not read as though its
90 /// wall-clock digits were UTC — two rows in one table have to be
91 /// comparable by eye.
92 #[test]
93 fn a_foreign_offset_is_normalized_to_utc() {
94 let t = Timestamp::new("2026-08-11T18:08:31+02:00");
95 assert_eq!(t.short, "2026-08-11 16:08");
96 }
97
98 #[test]
99 fn a_second_precision_timestamp_shortens_too() {
100 assert_eq!(
101 Timestamp::new("2026-08-11T16:08:31Z").short,
102 "2026-08-11 16:08"
103 );
104 }
105
106 #[test]
107 fn an_unparseable_timestamp_renders_as_itself_rather_than_vanishing() {
108 let t = Timestamp::new("not a date");
109 assert_eq!(t.short, "not a date");
110 assert_eq!(t.full, "not a date");
111 }
112
113 #[test]
114 fn an_empty_timestamp_stays_empty() {
115 assert_eq!(Timestamp::new("").short, "");
116 }
117 }
src/state.rs
Old New
@@ -393,13 +393,30 @@ pub struct Revision {
393 393
394 impl Revision { 394 impl Revision {
395 /// Abbreviated commit for display, or `None` when none was recorded. 395 /// Abbreviated commit for display, or `None` when none was recorded.
396 /// Truncating by chars rather than bytes keeps this total for any input.
397 pub fn short_commit(&self) -> Option<String> { 396 pub fn short_commit(&self) -> Option<String> {
398 if self.commit.is_empty() { 397 short_commit_oid(&self.commit)
399 return None; 398 }
400 } 399 }
401 Some(self.commit.chars().take(8).collect()) 400
401 /// Abbreviate a commit oid for display, or `None` for an empty one.
402 ///
403 /// A *commit* oid, not a collab id: [`crate::abbrev`] sizes collab ids against
404 /// the set of ids in the repository, and git already abbreviates its own
405 /// objects by its own rule. What this shares with that module is only the
406 /// floor, [`crate::abbrev::MIN_WIDTH`], which is what every commit in this
407 /// project's output has always been printed at.
408 ///
409 /// Shared rather than duplicated because the alternative is what issue
410 /// `b2a57996` reported: a patch detail page rendering `Merged in` at 40
411 /// characters beside a Revisions table rendering 8, with no rule anywhere
412 /// saying which was right.
413 ///
414 /// Truncating by chars rather than bytes keeps this total for any input.
415 pub fn short_commit_oid(commit: &str) -> Option<String> {
416 if commit.is_empty() {
417 return None;
402 } 418 }
419 Some(commit.chars().take(crate::abbrev::MIN_WIDTH).collect())
403 } 420 }
404 421
405 /// What to show in place of a commit that was never recorded. Shared so the 422 /// What to show in place of a commit that was never recorded. Shared so the
tests/web_layout_test.rs
Old New
@@ -0,0 +1,238 @@
1 //! The web UI's pages must not scroll sideways at a narrow viewport.
2 //!
3 //! This is a *layout* property, not a markup one: the HTML that overflows is
4 //! perfectly well-formed, every content assertion about it passes, and the
5 //! defect only exists once a browser has laid the page out at a width. So the
6 //! primary test here renders real server output in a real headless browser and
7 //! compares `documentElement.scrollWidth` with `window.innerWidth` — the same
8 //! measurement a human made by hand when this was filed.
9 //!
10 //! The browser is an *external oracle*: it is not part of this project and has
11 //! no idea what the test expects, so it cannot be talked into agreeing. A test
12 //! that only asserted "the template contains a wrapper div" would be blessing
13 //! our own guess about what makes a page fit.
14 //!
15 //! The page is measured from a `file://` copy rather than over HTTP because the
16 //! server's pages are entirely self-contained — the stylesheet is inlined in
17 //! `base.html` and nothing else is fetched — so the two lay out identically,
18 //! and the copy avoids handing a browser a URL to a live test server.
19 //!
20 //! If no browser is installed the measurement is skipped loudly, and
21 //! `structural_fallback` below still guards the invariant that produces the
22 //! property. Set `GIT_COLLAB_REQUIRE_BROWSER=1` to turn the skip into a
23 //! failure, which is what CI should do once a browser is provisioned.
24
25 mod common;
26
27 use common::ServerHarness;
28 use std::path::PathBuf;
29 use std::process::Command;
30
31 /// Viewport width to measure at. Narrow enough to be a real phone and to be
32 /// the width the defect was reported at.
33 const NARROW_WIDTH: u32 = 500;
34
35 /// A branch name of the shape agent worktrees actually produce, which is what
36 /// made the patch list overflow in the first place.
37 const EPHEMERAL_BRANCH: &str = "worktree-agent-ab8e151c29539ab74";
38
39 /// Locate a Chromium-family browser, or `None` if this machine has none.
40 fn find_browser() -> Option<PathBuf> {
41 if let Ok(explicit) = std::env::var("CHROME") {
42 let path = PathBuf::from(explicit);
43 if path.exists() {
44 return Some(path);
45 }
46 }
47 for name in [
48 "chromium",
49 "chromium-browser",
50 "google-chrome-stable",
51 "google-chrome",
52 "chrome",
53 ] {
54 if let Ok(output) = Command::new("which").arg(name).output() {
55 if output.status.success() {
56 let found = String::from_utf8_lossy(&output.stdout).trim().to_string();
57 if !found.is_empty() {
58 return Some(PathBuf::from(found));
59 }
60 }
61 }
62 }
63 None
64 }
65
66 /// Lay `html` out at `width` pixels and report
67 /// `(documentElement.scrollWidth, window.innerWidth)`.
68 ///
69 /// The probe script appends its measurement to the DOM and `--dump-dom` prints
70 /// the DOM after load, which is how a number gets back out of a browser that
71 /// has no other channel to talk to us.
72 fn measure(browser: &PathBuf, html: &str, width: u32) -> (u32, u32) {
73 let dir = tempfile::TempDir::new().unwrap();
74 let page = dir.path().join("page.html");
75 // The result is keyed on an *attribute* rather than a text marker: the
76 // dumped DOM includes this script's own source, so any literal the script
77 // prints would be found in the script before it was found in the result.
78 let probe = "\n<script>window.addEventListener('load',function(){\
79 var d=document.createElement('div');\
80 d.setAttribute('data-measure','');\
81 d.textContent=document.documentElement.scrollWidth+' '+window.innerWidth;\
82 document.body.appendChild(d);});</script>\n";
83 std::fs::write(&page, format!("{html}{probe}")).unwrap();
84
85 let profile = dir.path().join("profile");
86 let output = Command::new(browser)
87 .args([
88 "--headless",
89 "--disable-gpu",
90 "--no-sandbox",
91 "--no-first-run",
92 "--disable-extensions",
93 "--dump-dom",
94 "--virtual-time-budget=4000",
95 ])
96 .arg(format!("--window-size={width},900"))
97 .arg(format!("--user-data-dir={}", profile.display()))
98 .arg(format!("file://{}", page.display()))
99 .output()
100 .expect("failed to run headless browser");
101
102 let dom = String::from_utf8_lossy(&output.stdout);
103 let after = dom
104 .split("data-measure=\"\">")
105 .nth(1)
106 .unwrap_or_else(|| panic!("browser produced no measurement; DOM was:\n{dom}"));
107 let text = after
108 .split('<')
109 .next()
110 .expect("measurement div has a closing tag");
111 let mut parts = text.split_whitespace();
112 let mut next = || {
113 parts
114 .next()
115 .unwrap_or_else(|| panic!("malformed measurement '{text}'"))
116 .parse::<u32>()
117 .unwrap_or_else(|_| panic!("malformed measurement '{text}'"))
118 };
119 let scroll_width = next();
120 let inner_width = next();
121 (scroll_width, inner_width)
122 }
123
124 /// Seed a repository whose lists are as wide as the real ones: a patch created
125 /// from an ephemeral worktree branch, an issue, a commit and a release.
126 fn seed(harness: &ServerHarness) {
127 let repo = harness.work_repo();
128 repo.commit_file(
129 "src/rendering.rs",
130 "pub fn render() {}\n",
131 "Render the patch list without forcing the page sideways",
132 );
133 repo.git(&["checkout", "-b", EPHEMERAL_BRANCH]);
134 repo.commit_file(
135 "src/wide.rs",
136 "pub fn wide() {}\n",
137 "commit on an agent worktree branch",
138 );
139 repo.run_ok(&[
140 "patch",
141 "create",
142 "-t",
143 "Wrap list tables so a narrow viewport does not scroll sideways",
144 "-B",
145 EPHEMERAL_BRANCH,
146 ]);
147 repo.git(&["checkout", "main"]);
148 repo.issue_open("Timestamps render as full RFC3339 with nanoseconds throughout");
149
150 harness.push_head();
151 harness.push_collab_refs();
152
153 // A release too: its listing carries a 64-character sha256, which is wider
154 // than the viewport all by itself.
155 let content: Vec<u8> = (0u32..64).flat_map(|i| i.to_le_bytes()).collect();
156 harness.ssh_exec_with_stdin(
157 &format!(
158 "collab-release upload '{}.git' 'v1.0.0' 'git-collab-x86_64-unknown-linux-gnu.tar.gz'",
159 harness.repo_name()
160 ),
161 &content,
162 );
163 }
164
165 /// Every page a reader lands on, at the width a phone gives them.
166 fn pages(repo_name: &str) -> Vec<String> {
167 vec![
168 format!("/{repo_name}"),
169 format!("/{repo_name}/patches"),
170 format!("/{repo_name}/patches?filter=all"),
171 format!("/{repo_name}/issues"),
172 format!("/{repo_name}/issues?filter=all"),
173 format!("/{repo_name}/commits"),
174 format!("/{repo_name}/releases"),
175 format!("/{repo_name}/tree"),
176 ]
177 }
178
179 #[test]
180 fn no_page_scrolls_sideways_at_a_narrow_viewport() {
181 let browser = match find_browser() {
182 Some(b) => b,
183 None => {
184 let message = "SKIPPED no_page_scrolls_sideways_at_a_narrow_viewport: no \
185 Chromium-family browser found. Install chromium or set $CHROME. \
186 Set GIT_COLLAB_REQUIRE_BROWSER=1 to make this a failure.";
187 if std::env::var("GIT_COLLAB_REQUIRE_BROWSER").is_ok() {
188 panic!("{message}");
189 }
190 eprintln!("{message}");
191 return;
192 }
193 };
194
195 let harness = ServerHarness::new("layout-narrow");
196 seed(&harness);
197
198 for path in pages(harness.repo_name()) {
199 let page = harness.get_ok(&path);
200 let (scroll_width, inner_width) = measure(&browser, &page.body, NARROW_WIDTH);
201 assert!(
202 scroll_width <= inner_width,
203 "{path} scrolls the page sideways at {NARROW_WIDTH}px: \
204 documentElement.scrollWidth = {scroll_width}, window.innerWidth = {inner_width}"
205 );
206 }
207 }
208
209 /// The invariant that produces the property above, asserted without a browser
210 /// so the guard survives on a machine that has none.
211 ///
212 /// Deliberately weaker than the measurement: it says every table is inside
213 /// something that can scroll on its own, which is *how* the pages fit, not
214 /// *that* they fit. If these two ever disagree, believe the browser.
215 #[test]
216 fn every_table_sits_inside_a_horizontal_scroll_container() {
217 let harness = ServerHarness::new("layout-structure");
218 seed(&harness);
219
220 let base_css = harness.get_ok(&format!("/{}/patches", harness.repo_name()));
221 assert!(
222 base_css.body.contains(".table-scroll") && base_css.body.contains("overflow-x: auto"),
223 "the stylesheet defines no scrollable table container"
224 );
225
226 for path in pages(harness.repo_name()) {
227 let page = harness.get_ok(&path);
228 for (index, _) in page.body.match_indices("<table") {
229 let before = &page.body[..index];
230 let wrapper = before.rfind("table-scroll");
231 let closing = before.rfind("</div>");
232 assert!(
233 wrapper.is_some() && wrapper > closing,
234 "{path} has a <table> that is not inside a .table-scroll container"
235 );
236 }
237 }
238 }
tests/web_rendering_test.rs
Old New
@@ -0,0 +1,423 @@
1 //! What the web UI actually puts on the page, driven through a live server.
2 //!
3 //! These cover three defects found by reading the rendered pages rather than
4 //! the templates — none of them is a malformed-HTML bug, so every existing
5 //! content assertion passed while all three were live:
6 //!
7 //! * `b2a57996` — timestamps printed as raw RFC3339 with nanoseconds, and a
8 //! `Merged in` oid printed at 40 characters beside revisions printed at 8.
9 //! * `c43c459d` — an empty *filtered* list claiming the repository is empty.
10 //! * `850f3b8e` — a Branch column naming worktree branches that were deleted
11 //! with the worktree.
12 //!
13 //! `--json` is asserted alongside each, because the fix is a *display* policy:
14 //! the HTML gets to be short, and the scripted surface must keep the full id
15 //! and the full timestamp it has always carried.
16
17 mod common;
18
19 use common::ServerHarness;
20 use serde_json::Value;
21 use std::process::Command;
22
23 /// A branch name of the shape agent worktrees produce and then delete.
24 const EPHEMERAL_BRANCH: &str = "worktree-agent-ab8e151c29539ab74";
25
26 /// The text a reader sees, with every tag — and so every attribute — removed.
27 ///
28 /// The full timestamp is deliberately still in the page, inside `title=`, so a
29 /// substring search over the raw HTML cannot tell the fix from the defect. The
30 /// question is only ever what is rendered *between* the tags.
31 fn visible_text(body: &str) -> String {
32 let mut text = String::with_capacity(body.len());
33 let mut in_tag = false;
34 for c in body.chars() {
35 match c {
36 '<' => in_tag = true,
37 '>' => in_tag = false,
38 _ if !in_tag => text.push(c),
39 _ => {}
40 }
41 }
42 text
43 }
44
45 /// Whether any visible text is an RFC3339 timestamp with a fractional second —
46 /// the 35-character shape issue `b2a57996` reported.
47 fn has_nanosecond_timestamp(body: &str) -> bool {
48 visible_text(body).split('T').skip(1).any(|rest| {
49 let head: String = rest.chars().take(16).collect();
50 head.len() >= 10 && head.as_bytes()[2] == b':' && head.contains('.')
51 })
52 }
53
54 fn patch_on_ephemeral_branch(harness: &ServerHarness, title: &str, branch: &str) -> String {
55 let repo = harness.work_repo();
56 repo.git(&["checkout", "-b", branch]);
57 repo.commit_file(
58 &format!("{branch}.rs"),
59 "pub fn work() {}\n",
60 &format!("work on {branch}"),
61 );
62 let out = repo.run_ok(&["patch", "create", "-t", title, "-B", branch]);
63 repo.git(&["checkout", "main"]);
64 out.trim()
65 .strip_prefix("Created patch ")
66 .expect("patch create prints the id")
67 .to_string()
68 }
69
70 fn patch_json(harness: &ServerHarness, id: &str) -> Value {
71 serde_json::from_str(&harness.work_repo().run_ok(&["patch", "show", id, "--json"])).unwrap()
72 }
73
74 fn issue_json(harness: &ServerHarness, id: &str) -> Value {
75 serde_json::from_str(&harness.work_repo().run_ok(&["issue", "show", id, "--json"])).unwrap()
76 }
77
78 // ---------------------------------------------------------------------------
79 // b2a57996 — timestamps and the `Merged in` oid
80 // ---------------------------------------------------------------------------
81
82 #[test]
83 fn list_timestamps_are_short_with_the_full_value_on_hover() {
84 let harness = ServerHarness::new("render-timestamps");
85 let patch_id = patch_on_ephemeral_branch(&harness, "A patch with a timestamp", "feat/stamped");
86 let issue_id = harness.work_repo().issue_open("An issue with a timestamp");
87 harness.push_head();
88 harness.push_collab_refs();
89
90 let full_patch_stamp = patch_json(&harness, &patch_id)["last_updated"]
91 .as_str()
92 .unwrap()
93 .to_string();
94 let full_issue_stamp = issue_json(&harness, &issue_id)["last_updated"]
95 .as_str()
96 .unwrap()
97 .to_string();
98
99 for (path, full) in [
100 (
101 format!("/{}/patches", harness.repo_name()),
102 &full_patch_stamp,
103 ),
104 (format!("/{}/issues", harness.repo_name()), &full_issue_stamp),
105 ] {
106 let body = harness.get_ok(&path).body;
107 assert!(
108 !has_nanosecond_timestamp(&body),
109 "{path} still renders a nanosecond timestamp:\n{body}"
110 );
111 assert!(
112 body.contains(&format!("title=\"{full}\"")),
113 "{path} drops the full timestamp instead of keeping it on hover"
114 );
115 // The short rendering: date and minute, no seconds, no nanoseconds.
116 let short = &full[..16].replace('T', " ");
117 assert!(
118 body.contains(&format!(">{short}<")),
119 "{path} does not render the short timestamp '{short}'"
120 );
121 }
122 }
123
124 #[test]
125 fn patch_detail_timestamps_are_short_with_the_full_value_on_hover() {
126 let harness = ServerHarness::new("render-detail-stamps");
127 let patch_id = patch_on_ephemeral_branch(&harness, "Reviewed patch", "feat/reviewed");
128 harness
129 .work_repo()
130 .run_ok(&["patch", "review", &patch_id, "-v", "approve", "-b", "Looks ok"]);
131 harness
132 .work_repo()
133 .run_ok(&["patch", "comment", &patch_id, "-b", "A thread comment"]);
134 harness.push_head();
135 harness.push_collab_refs();
136
137 let body = harness
138 .get_ok(&format!(
139 "/{}/patches/{}",
140 harness.repo_name(),
141 patch_id
142 ))
143 .body;
144 assert!(
145 !has_nanosecond_timestamp(&body),
146 "the patch detail page still renders nanosecond timestamps:\n{body}"
147 );
148
149 let json = patch_json(&harness, &patch_id);
150 let revision_stamp = json["revisions"][0]["timestamp"].as_str().unwrap();
151 assert!(
152 body.contains(&format!("title=\"{revision_stamp}\"")),
153 "the revision timestamp is not available on hover"
154 );
155 }
156
157 #[test]
158 fn merged_in_is_abbreviated_like_the_revision_commits_beside_it() {
159 let harness = ServerHarness::new("render-merged-in");
160 let patch_id = patch_on_ephemeral_branch(&harness, "A landed patch", "feat/landed");
161 let repo = harness.work_repo();
162 repo.git(&["merge", "--ff-only", "feat/landed"]);
163 repo.run_ok(&["patch", "merge", &patch_id]);
164 harness.push_head();
165 harness.push_collab_refs();
166
167 let merge_commit = patch_json(&harness, &patch_id)["merge_commit"]
168 .as_str()
169 .unwrap()
170 .to_string();
171 assert_eq!(merge_commit.len(), 40, "the event records the full oid");
172
173 let body = harness
174 .get_ok(&format!(
175 "/{}/patches/{}",
176 harness.repo_name(),
177 patch_id
178 ))
179 .body;
180
181 // The link still goes to the full oid; only the text is abbreviated.
182 assert!(
183 body.contains(&format!("/diff/{merge_commit}")),
184 "the merge link no longer resolves the full commit"
185 );
186 assert!(
187 body.contains(&format!(">{}<", &merge_commit[..8])),
188 "'Merged in' does not render the 8-character oid the revisions use"
189 );
190 assert!(
191 !body.contains(&format!(">{merge_commit}<")),
192 "'Merged in' still renders the full 40-character oid"
193 );
194 }
195
196 // ---------------------------------------------------------------------------
197 // c43c459d — an empty filtered list must not claim the repository is empty
198 // ---------------------------------------------------------------------------
199
200 #[test]
201 fn an_empty_open_list_names_the_filter_and_offers_the_way_out() {
202 let harness = ServerHarness::new("render-empty-filtered");
203 let patch_id = patch_on_ephemeral_branch(&harness, "A closed patch", "feat/closed");
204 harness.work_repo().patch_close(&patch_id);
205 let issue_id = harness.work_repo().issue_open("A closed issue");
206 harness.work_repo().issue_close(&issue_id);
207 harness.push_head();
208 harness.push_collab_refs();
209
210 let patches = harness
211 .get_ok(&format!("/{}/patches", harness.repo_name()))
212 .body;
213 assert!(
214 !patches.contains("No patches."),
215 "the patch list still claims the repository has no patches"
216 );
217 assert!(
218 patches.contains("No open patches"),
219 "the patch list does not say which filter was empty:\n{patches}"
220 );
221 assert!(
222 patches.contains("1 total"),
223 "the patch list does not say how many patches there are:\n{patches}"
224 );
225 assert!(
226 patches.contains("patches?filter=all"),
227 "the patch list offers no way to see the rest"
228 );
229
230 let issues = harness
231 .get_ok(&format!("/{}/issues", harness.repo_name()))
232 .body;
233 assert!(
234 !issues.contains("No issues."),
235 "the issue list still claims the repository has no issues"
236 );
237 assert!(
238 issues.contains("No open issues") && issues.contains("1 total"),
239 "the issue list does not say which filter was empty:\n{issues}"
240 );
241 assert!(
242 issues.contains("issues?filter=all"),
243 "the issue list offers no way to see the rest"
244 );
245 }
246
247 #[test]
248 fn a_genuinely_empty_repository_still_says_so_plainly() {
249 let harness = ServerHarness::new("render-empty-repo");
250 harness.push_head();
251
252 let patches = harness
253 .get_ok(&format!("/{}/patches", harness.repo_name()))
254 .body;
255 assert!(
256 patches.contains("No patches yet."),
257 "an empty repository should say so without offering a filter:\n{patches}"
258 );
259 assert!(
260 !patches.contains("total"),
261 "an empty repository should not offer to show zero more patches"
262 );
263
264 let issues = harness
265 .get_ok(&format!("/{}/issues", harness.repo_name()))
266 .body;
267 assert!(
268 issues.contains("No issues yet."),
269 "an empty repository should say so without offering a filter:\n{issues}"
270 );
271 }
272
273 // ---------------------------------------------------------------------------
274 // 850f3b8e — the Branch column
275 // ---------------------------------------------------------------------------
276
277 #[test]
278 fn the_patch_list_shows_the_base_ref_not_the_ephemeral_branch() {
279 let harness = ServerHarness::new("render-branch-column");
280 patch_on_ephemeral_branch(&harness, "Work from a worktree", EPHEMERAL_BRANCH);
281 harness.push_head();
282 harness.push_collab_refs();
283
284 let body = harness
285 .get_ok(&format!("/{}/patches?filter=all", harness.repo_name()))
286 .body;
287 assert!(
288 !body.contains(EPHEMERAL_BRANCH),
289 "the patch list still names a branch that no longer exists:\n{body}"
290 );
291 assert!(
292 body.contains("<th>Base</th>"),
293 "the patch list has no Base column:\n{body}"
294 );
295 assert!(
296 body.contains(">main<"),
297 "the patch list does not say what the patch targets:\n{body}"
298 );
299 }
300
301 /// Provenance still belongs somewhere — `patch show` is where, on both
302 /// surfaces. Dropping it from the list is not dropping it from the tool.
303 #[test]
304 fn the_patch_detail_page_still_records_the_branch_it_came_from() {
305 let harness = ServerHarness::new("render-branch-detail");
306 let patch_id = patch_on_ephemeral_branch(&harness, "Work from a worktree", EPHEMERAL_BRANCH);
307 harness.push_head();
308 harness.push_collab_refs();
309
310 let body = harness
311 .get_ok(&format!(
312 "/{}/patches/{}",
313 harness.repo_name(),
314 patch_id
315 ))
316 .body;
317 assert!(
318 body.contains(EPHEMERAL_BRANCH),
319 "the detail page lost the branch the patch came from"
320 );
321 }
322
323 #[test]
324 fn the_cli_patch_list_agrees_with_the_web_list_about_the_base_ref() {
325 let harness = ServerHarness::new("render-cli-agreement");
326 patch_on_ephemeral_branch(&harness, "Work from a worktree", EPHEMERAL_BRANCH);
327
328 let listing = harness.work_repo().run_ok(&["patch", "list"]);
329 assert!(
330 !listing.contains(EPHEMERAL_BRANCH),
331 "`patch list` names a branch that no longer exists:\n{listing}"
332 );
333 assert!(
334 listing.contains("main"),
335 "`patch list` does not say what the patch targets:\n{listing}"
336 );
337 }
338
339 // ---------------------------------------------------------------------------
340 // The scripted surface is untouched by all of the above
341 // ---------------------------------------------------------------------------
342
343 #[test]
344 fn json_keeps_full_ids_timestamps_and_the_branch_the_html_no_longer_shows() {
345 let harness = ServerHarness::new("render-json-intact");
346 let patch_id = patch_on_ephemeral_branch(&harness, "Work from a worktree", EPHEMERAL_BRANCH);
347
348 let listing: Value =
349 serde_json::from_str(&harness.work_repo().run_ok(&["patch", "list", "--json"])).unwrap();
350 let entry = &listing.as_array().unwrap()[0];
351
352 assert_eq!(
353 entry["id"].as_str().unwrap().len(),
354 40,
355 "--json abbreviated an id"
356 );
357 assert_eq!(
358 entry["branch"].as_str().unwrap(),
359 EPHEMERAL_BRANCH,
360 "--json dropped the branch, which is provenance a script may still want"
361 );
362 let stamp = entry["last_updated"].as_str().unwrap();
363 assert!(
364 stamp.contains('.') && stamp.len() > 25,
365 "--json truncated a timestamp to the display format: {stamp}"
366 );
367
368 let detail = patch_json(&harness, &patch_id);
369 assert_eq!(detail["id"].as_str().unwrap().len(), 40);
370 assert!(detail["revisions"][0]["timestamp"]
371 .as_str()
372 .unwrap()
373 .contains('.'));
374 }
375
376 // ---------------------------------------------------------------------------
377 // Rendering a page is a read
378 // ---------------------------------------------------------------------------
379
380 /// Every ref in the served repository, as `<name> <oid>` lines.
381 fn ref_snapshot(harness: &ServerHarness) -> String {
382 let bare = harness
383 .repos_dir()
384 .join(format!("{}.git", harness.repo_name()));
385 let output = Command::new("git")
386 .args(["for-each-ref", "--format=%(refname) %(objectname)"])
387 .current_dir(&bare)
388 .output()
389 .expect("failed to list refs");
390 assert!(output.status.success());
391 String::from_utf8(output.stdout).unwrap()
392 }
393
394 #[test]
395 fn rendering_pages_moves_no_refs_and_appends_no_events() {
396 let harness = ServerHarness::new("render-read-only");
397 let patch_id = patch_on_ephemeral_branch(&harness, "Work from a worktree", EPHEMERAL_BRANCH);
398 let issue_id = harness.work_repo().issue_open("An issue to read");
399 harness.push_head();
400 harness.push_collab_refs();
401
402 let before = ref_snapshot(&harness);
403 let name = harness.repo_name();
404 for path in [
405 format!("/{name}"),
406 format!("/{name}/patches"),
407 format!("/{name}/patches?filter=all"),
408 format!("/{name}/patches/{patch_id}"),
409 format!("/{name}/issues"),
410 format!("/{name}/issues?filter=all"),
411 format!("/{name}/issues/{issue_id}"),
412 format!("/{name}/commits"),
413 format!("/{name}/releases"),
414 ] {
415 harness.get_ok(&path);
416 }
417 let after = ref_snapshot(&harness);
418
419 assert_eq!(
420 before, after,
421 "rendering pages changed the repository's refs"
422 );
423 }