Public: anyone can read this; the crew can change it.
The conflict resolver speaks the home page's language #9716
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9713-the-conflict-resolver-speaks-the-home-pa → main 3 commits fork 2 threads · 2 unresolved
5 files
changed +76
−34
docs/PULLS.md +1 −1
Diff not shown — a line 2,393 characters wide (limit 2,000). +1 −1
View filedocs/design/areas/pull.md +4
| @@ -15,3 +15,7 @@ On a 390 px phone the page does not scroll sideways (#9327): the title (`.page-h | ||
| 15 | 15 | `.split` (content.ts) has one `minmax(0, 1fr)` column below 960 px, so a long unbroken line in a comment cannot widen the page. It is global to every `.split` page; the `min-width: 960px` and `data-rail="wide"` rules come after it in the same file and still set their two columns, so desk layouts are unchanged. |
| 16 | 16 | |
| 17 | 17 | The pull page's cards (merge box, review, commits, slices, run-this-code) are `.card.bare` (#9588): rows and a heading on the page's one background, no box. A bare card draws no tone, so the merge row carries none on the card: its state is the sentence's opening word ("Ready to merge", "Waiting on checks.", "Merged.", "Closed without merging.") with the tone's `.dot` before it, a word and a mark, not colour alone. The team verdicts are a `.list` of `.row`s, not a card. What stays a box: each file's diff (code scrolls inside its own frame) and the comments, which `thread.ts` draws for the issue page too; turning those into rows is the timeline's change, not this page's. No new class; `test/pull-merge-box.test.ts` holds it. |
| 18 | ||
| 19 | ## The conflict resolver (#9713) | |
| 20 | ||
| 21 | `/:owner/:name/pulls/:number/resolve`: each conflict block, the crew proposal and the empty states are `.card.bare` (#9588), a heading over rows on the one background. The two sides lose their hairline box; the taps keep theirs (a control). The ours/theirs/both/Mine taps, the text box and `#crew-proposal` are unchanged. No new class; `test/conflicts.test.ts` holds it. |
src/web/pages/conflicts.ts +27 −25
| @@ -1,10 +1,10 @@ | ||
| 1 | 1 | /** |
| 2 | 2 | * GET /:owner/:name/pulls/:number/resolve — a conflicted pull, one file at a |
| 3 | * time. Each conflict block is a card: the two sides named by their branch, | |
| 4 | * three taps (ours, theirs, both) and a text box for your own words. The | |
| 5 | * choices travel in the query string, so the page keeps no state and writes | |
| 6 | * nothing to git; it shows the running count and the merged file beneath. | |
| 7 | * Writers only. | |
| 3 | * time. Each conflict block is a heading over rows, no box: the two sides | |
| 4 | * named by their branch, four taps (keep one side, keep both, write your | |
| 5 | * own) and a text box for your own words. The choices travel in the query | |
| 6 | * string, so the page keeps no state and writes nothing to git; it shows the | |
| 7 | * running count and the merged file beneath. Writers only. | |
| 8 | 8 | */ |
| 9 | 9 | |
| 10 | 10 | import { canWrite, getRepoById, type Repo } from "../../core/repos.ts"; |
| @@ -103,7 +103,7 @@ async function proposalCard( | ||
| 103 | 103 | const seen = await proposalOf(app, repo, pull, dir); |
| 104 | 104 | const parts: Html[] = []; |
| 105 | 105 | if (seen.kind === "working") |
| 106 | parts.push(html`<p>The crew is working on a proposal (${seen.state}). This card updates by itself.</p>`); | |
| 106 | parts.push(html`<p>The crew is working on a fix (${seen.state}). This card updates by itself.</p>`); | |
| 107 | 107 | else if (seen.kind === "landed") { |
| 108 | 108 | const head = getRepoById(app.db, pull.headRepoId) ?? repo; |
| 109 | 109 | parts.push(html`<p><strong>The crew proposed a resolution</strong>, committed as |
| @@ -121,20 +121,21 @@ async function proposalCard( | ||
| 121 | 121 | parts.push(html`<p class="muted">${PROPOSE_WHY.busy}</p>`); |
| 122 | 122 | if (conflicted && agent !== undefined && !working) |
| 123 | 123 | parts.push(html`<form method="post" action="${here}/propose"> |
| 124 | <p>${btn("Have the crew propose", { icon: "check" })} | |
| 125 | <span class="muted small">${"@" + agent} merges the base into this branch and commits a resolution.</span></p> | |
| 124 | <p>${btn("Ask the crew to fix it", { icon: "check" })} | |
| 125 | <span class="muted small">${"@" + agent} merges the base into this branch and commits a fix.</span></p> | |
| 126 | 126 | </form>`); |
| 127 | 127 | if (parts.length === 0) return html``; |
| 128 | 128 | // A live region: the run's events re-read the card, so nobody reloads. |
| 129 | return html`<div id="crew-proposal" data-live="run.queued,run.started,run.finished,pull.*,comment.*">${card(html`${parts}`, { head: html`<h2>The crew</h2>` })}</div>`; | |
| 129 | return html`<div id="crew-proposal" data-live="run.queued,run.started,run.finished,pull.*,comment.*">${card(html`${parts}`, { extra: "bare", head: html`<h2>The crew</h2>` })}</div>`; | |
| 130 | 130 | } |
| 131 | 131 | |
| 132 | 132 | const CSS = ` |
| 133 | 133 | .rs-list { display: grid; gap: 12px; margin-top: 12px; } |
| 134 | 134 | .rs-card { content-visibility: auto; contain-intrinsic-size: auto 260px; } |
| 135 | .rs-sides { display: grid; gap: 8px; grid-template-columns: repeat(auto-fit, minmax(min(100%, 240px), 1fr)); } | |
| 135 | .rs-sides { display: grid; gap: 12px; grid-template-columns: repeat(auto-fit, minmax(min(100%, 240px), 1fr)); } | |
| 136 | 136 | .rs-side { min-width: 0; } |
| 137 | .rs-side pre, .rs-merged { margin: 4px 0 0; padding: 8px; overflow: auto; max-height: 12em; font-size: 12.5px; white-space: pre; background: var(--muted-bg, transparent); border: 1px solid var(--border, 884); border-radius: 6px; } | |
| 137 | .rs-side .rs-label { display: block; color: var(--muted, inherit); opacity: .75; } | |
| 138 | .rs-side pre, .rs-merged { margin: 2px 0 0; padding: 4px 0 8px; overflow: auto; max-height: 12em; font-size: 12.5px; white-space: pre; background: none; border: 0; border-bottom: 1px solid var(--border, 884); border-radius: 0; } | |
| 138 | 139 | .rs-merged { max-height: 32em; } |
| 139 | 140 | .rs-taps { display: flex; gap: 6px; flex-wrap: wrap; margin: 8px 0; } |
| 140 | 141 | .rs-taps label { flex: 1 1 0; text-align: center; padding: 8px 6px; min-height: 40px; border: 1px solid var(--border, 884); border-radius: 6px; cursor: pointer; } |
| @@ -174,9 +175,10 @@ export async function conflictsPage(ctx: Ctx): Promise<Response> { | ||
| 174 | 175 | ${changeHead(ctx, repo, pull)}${changeTabs(repo, pull, "files")} |
| 175 | 176 | ${card( |
| 176 | 177 | html`<p>${unknown && pull.state === "open" |
| 177 | ? "Nothing to resolve there: that file is not one of the conflicted files." | |
| 178 | : "Nothing to resolve: this pull has no conflicts."} | |
| 178 | ? "No conflict in that file." | |
| 179 | : "This pull has no conflicts."} | |
| 179 | 180 | <a href="${back}">Back to the files</a>.</p>`, |
| 181 | { extra: "bare" }, | |
| 180 | 182 | )} |
| 181 | 183 | ${crew}`, |
| 182 | 184 | }); |
| @@ -207,24 +209,24 @@ export async function conflictsPage(ctx: Ctx): Promise<Response> { | ||
| 207 | 209 | return card( |
| 208 | 210 | html`<div class="rs-sides"> |
| 209 | 211 | <div class="rs-side"> |
| 210 | <strong class="small mono">${pull.baseBranch}</strong> | |
| 212 | <span class="rs-label small mono">${pull.baseBranch}</span> | |
| 211 | 213 | <pre>${block.ours}</pre> |
| 212 | 214 | </div> |
| 213 | 215 | <div class="rs-side"> |
| 214 | <strong class="small mono">${pull.headBranch}</strong> | |
| 216 | <span class="rs-label small mono">${pull.headBranch}</span> | |
| 215 | 217 | <pre>${block.theirs}</pre> |
| 216 | 218 | </div> |
| 217 | 219 | </div> |
| 218 | 220 | ${block.base === "" |
| 219 | 221 | ? "" |
| 220 | : html`<details><summary class="small muted">Before either changed it</summary><pre class="rs-merged">${block.base}</pre></details>`} | |
| 222 | : html`<details><summary class="small muted">The original</summary><pre class="rs-merged">${block.base}</pre></details>`} | |
| 221 | 223 | <div class="rs-taps"> |
| 222 | ${tap("ours", `Ours`)}${tap("theirs", `Theirs`)}${tap("both", "Both")}${tap("text", "Mine")} | |
| 224 | ${tap("ours", `Keep ${pull.baseBranch}`)}${tap("theirs", `Keep ${pull.headBranch}`)}${tap("both", "Keep both")}${tap("text", "Write my own")} | |
| 223 | 225 | </div> |
| 224 | 226 | <textarea class="rs-text" name="t${i}" rows="3" maxlength="${MAX_TEXT}" |
| 225 | aria-label="Your own text for block ${i + 1}">${own?.kind === "text" ? own.text : ""}</textarea>`, | |
| 227 | aria-label="Your own words for block ${i + 1}">${own?.kind === "text" ? own.text : ""}</textarea>`, | |
| 226 | 228 | { |
| 227 | extra: "rs-card", | |
| 229 | extra: "bare rs-card", | |
| 228 | 230 | head: html`<h2>Block ${i + 1} of ${total}</h2>`, |
| 229 | 231 | }, |
| 230 | 232 | ); |
| @@ -235,7 +237,7 @@ export async function conflictsPage(ctx: Ctx): Promise<Response> { | ||
| 235 | 237 | <p class="muted small mt"> |
| 236 | 238 | <a href="${back}">← Files</a> · <span class="mono">${file}</span> · |
| 237 | 239 | <strong id="rs-count">${done} of ${total} resolved</strong> |
| 238 | · nothing is written until you finish | |
| 240 | · nothing is saved until you finish | |
| 239 | 241 | </p> |
| 240 | 242 | ${files.length > 1 |
| 241 | 243 | ? html`<p class="small"> |
| @@ -249,18 +251,18 @@ export async function conflictsPage(ctx: Ctx): Promise<Response> { | ||
| 249 | 251 | ${crew} |
| 250 | 252 | ${total === 0 |
| 251 | 253 | ? card( |
| 252 | html`<p>This file has no text blocks to choose between (it may be | |
| 253 | deleted on one side). Resolve it in your own checkout.</p>`, | |
| 254 | html`<p>This file has no text to choose between. It may be deleted on | |
| 255 | one side. Fix it in your own checkout.</p>`, | |
| 254 | 256 | ) |
| 255 | 257 | : html`<form method="get" action="${here}"> |
| 256 | 258 | <input type="hidden" name="file" value="${file}" /> |
| 257 | 259 | <input type="hidden" name="head" value="${status.headOid}" /> |
| 258 | 260 | <input type="hidden" name="base" value="${status.baseOid}" /> |
| 259 | 261 | <div class="rs-list">${cards}</div> |
| 260 | <p class="mt">${btn("Update the merged file", { icon: "check", variant: "ghost" })} | |
| 262 | <p class="mt">${btn("Show the result", { icon: "check", variant: "ghost" })} | |
| 261 | 263 | ${files.length === 1 |
| 262 | ? html`<button type="submit" formmethod="post" class="btn" data-variant="primary">Finish: commit this to ${pull.headBranch}</button>` | |
| 263 | : html`<span class="muted small">Finish needs every conflicted file on this page; this pull has ${files.length}. Resolve it in your own checkout.</span>`}</p> | |
| 264 | ? html`<button type="submit" formmethod="post" class="btn" data-variant="primary">Save to ${pull.headBranch}</button>` | |
| 265 | : html`<span class="muted small">This pull has ${files.length} conflicted files. Fix them in your own checkout.</span>`}</p> | |
| 264 | 266 | </form>`} |
| 265 | 267 | <h2 class="mt">Merged file</h2> |
| 266 | 268 | <pre class="rs-merged" aria-label="Merged ${file}">${merged}</pre>`; |
test/conflicts.test.ts +37 −3
| @@ -164,13 +164,47 @@ describe("the Resolve page", () => { | ||
| 164 | 164 | expect(page).toContain("m6\nmine\ny\n"); |
| 165 | 165 | }); |
| 166 | 166 | |
| 167 | test("conflict blocks are headings over rows, not boxed cards", async () => { | |
| 168 | const page = await (await fetch(`${base}/dev/app/pulls/1/resolve`, { headers })).text(); | |
Box 3: this checks a class name, not the rows shape. Assert the sides carry no box, and cover the crew card and empty states too. |
||
| 169 | expect(page).toMatch(/<div\s+class="card bare rs-card"/); | |
| 170 | expect(page).not.toMatch(/<div\s+class="card rs-card"/); | |
| 171 | expect(page).toContain("<h2>Block 1 of 2</h2>"); | |
| 172 | expect(page).toContain('name="b0"'); | |
| 173 | }); | |
| 174 | ||
| 175 | test("the sides are rows on the page background, and the words are plain", async () => { | |
| 176 | const page = await (await fetch(`${base}/dev/app/pulls/1/resolve`, { headers })).text(); | |
| 177 | // Sides: a muted label over the text, no box, a hairline where it ends. | |
| 178 | const rule = /\.rs-side pre, \.rs-merged \{([^}]*)\}/.exec(page)![1]!; | |
| 179 | expect(rule).toContain("background: none"); | |
| 180 | expect(rule).toContain("border-radius: 0"); | |
| 181 | expect(rule).toContain("border-bottom: 1px solid"); | |
| 182 | expect(rule).not.toMatch(/background:\s*var/); | |
| 183 | expect(page).toMatch(/<span class="rs-label small mono">main<\/span>\s*<pre>/); | |
| 184 | expect(page).not.toMatch(/<strong class="small mono">main<\/strong>/); | |
| 185 | // Only the radio taps keep a border. | |
| 186 | expect(/\.rs-taps label \{[^}]*border: 1px solid/.test(page)).toBe(true); | |
| 187 | // Plain taps and help text. | |
| 188 | for (const w of ["Keep main", "Keep loud", "Keep both", "Write my own", "Show the result", "Save to loud", "nothing is saved until you finish"]) | |
| 189 | expect(page).toContain(w); | |
| 190 | for (const w of [">Ours<", ">Theirs<", ">Mine<", "Before either changed", "Update the merged file", "Finish: commit"]) | |
| 191 | expect(page).not.toContain(w); | |
| 192 | }); | |
| 193 | ||
| 194 | test("the empty states and the crew card are bare, in plain words", async () => { | |
| 195 | const none = await (await fetch(`${base}/dev/app/pulls/1/resolve?file=nope.txt`, { headers })).text(); | |
| 196 | expect(none).toMatch(/<div\s+class="card bare"/); | |
| 197 | expect(none).not.toMatch(/<div\s+class="card"/); | |
| 198 | expect(none).toContain("No conflict in that file."); | |
| 199 | expect(none).not.toContain("Nothing to resolve"); | |
| 200 | }); | |
| 201 | ||
| 167 | 202 | test("readers and strangers do not get it; an unnamed file is not read", async () => { |
| 168 | 203 | const url = `${base}/dev/app/pulls/1/resolve`; |
| 169 | 204 | expect((await fetch(url)).status).toBe(404); |
| 170 | 205 | const odd = await (await fetch(`${url}?file=../../etc/passwd`, { headers })).text(); |
| 171 | expect(odd).toContain("Nothing to resolve"); | |
| 172 | expect(odd).toContain("not one of the conflicted files"); | |
| 173 | expect(odd).not.toContain("has no conflicts"); | |
| 206 | expect(odd).toContain("No conflict in that file."); | |
| 207 | expect(odd).not.toContain("This pull has no conflicts."); | |
| 174 | 208 | }); |
| 175 | 209 | |
| 176 | 210 | test("the merge box offers Resolve to a writer", async () => { |
test/crew-proposal.test.ts +7 −5
| @@ -178,12 +178,14 @@ echo '{"type":"result","subtype":"success","is_error":false,"result":"Wrote note | ||
| 178 | 178 | `${servers.url}/${repo.ownerName}/${repo.name}/pulls/${pull.number}/resolve`, |
| 179 | 179 | { headers: { authorization: `Bearer ${token}` } }, |
| 180 | 180 | ).then((r) => r.text()); |
| 181 | expect(page).toContain("Have the crew propose"); | |
| 181 | expect(page).toContain("Ask the crew to fix it"); | |
| 182 | expect(page).toMatch(/id="crew-proposal"[^>]*>\s*<div\s+class="card bare"/); | |
| 183 | expect(page).toContain("<h2>The crew</h2>"); | |
| 182 | 184 | expect(page).toContain(`pulls/${pull.number}/resolve/propose`); |
| 183 | 185 | const stranger = await fetch( |
| 184 | 186 | `${servers.url}/${repo.ownerName}/${repo.name}/pulls/${pull.number}/resolve`, |
| 185 | 187 | ); |
| 186 | expect(await stranger.text()).not.toContain("Have the crew propose"); | |
| 188 | expect(await stranger.text()).not.toContain("Ask the crew to fix it"); | |
| 187 | 189 | }); |
| 188 | 190 | |
| 189 | 191 | test("a reader's tap, and a cross-origin one, start nothing", async () => { |
| @@ -372,7 +374,7 @@ echo '{"type":"result","subtype":"success","is_error":false,"result":"Wrote note | ||
| 372 | 374 | if (!moved.ok) throw new Error(JSON.stringify(moved.error)); |
| 373 | 375 | const page = await resolvePage(); |
| 374 | 376 | expect(page).toContain("The last proposal did not land (failed): it gave up"); |
| 375 | expect(page).toContain("Have the crew propose"); | |
| 377 | expect(page).toContain("Ask the crew to fix it"); | |
| 376 | 378 | |
| 377 | 379 | const screen = await terminal(alice, [], (t) => t.includes("did not land")); |
| 378 | 380 | expect(screen).toContain("The last proposal did not land (failed): it gave up"); |
| @@ -397,7 +399,7 @@ echo '{"type":"result","subtype":"success","is_error":false,"result":"Wrote note | ||
| 397 | 399 | expect(agentRuns()).toBe(before); |
| 398 | 400 | const page = await resolvePage(); |
| 399 | 401 | expect(page).toContain("The crew is already working on this pull."); |
| 400 | expect(page).not.toContain("Have the crew propose"); | |
| 402 | expect(page).not.toContain("Ask the crew to fix it"); | |
| 401 | 403 | app.db.query("UPDATE run SET state = 'cancelled' WHERE id = ?1").run(run.id); |
| 402 | 404 | }); |
| 403 | 405 | |
| @@ -409,7 +411,7 @@ echo '{"type":"result","subtype":"success","is_error":false,"result":"Wrote note | ||
| 409 | 411 | expect(refused.ok).toBe(false); |
| 410 | 412 | if (!refused.ok) expect(refused.error).toBe("no_agent"); |
| 411 | 413 | expect(agentRuns()).toBe(before); |
| 412 | expect(await resolvePage()).not.toContain("Have the crew propose"); | |
| 414 | expect(await resolvePage()).not.toContain("Ask the crew to fix it"); | |
| 413 | 415 | } finally { |
| 414 | 416 | app.db.query("UPDATE issue SET assignee_id = NULL WHERE id = ?1").run(pull.id); |
| 415 | 417 | } |
Principle 6/7: background and radius remain, so each side is still a tinted box. Make it a row on the page background.