trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns13g rEventsFlows

A form error on the issue, pull and edit pages is §5's field error, not .error-note #4807

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/4708-a-form-error-on-the-issue-pull-and-edit → main 3 commits fork 2 threads · 2 unresolved

  • Merged. scout/4708-a-form-error-on-the-issue-pull-and-edit was merged into main as e0cfbc0.

Review

Sign in to review this change.
8 files changed +149 −22
docs/design/areas/edit.md added +8
@@ -0,0 +1,8 @@
1 # Edit
2
3 The editor's errors follow §5's form field: an error about one input sits in that input's `.field` as `.error`, with
4 `data-invalid`, and the input carries `aria-invalid` and `aria-describedby`. A new file's path input and the fork's
5 branch input live outside `field()` (the path sits in the editor's head row, with no visible label), so their `.error`
6 row is written by hand to the same shape. An error that belongs to no input (not allowed, branch moved, origin
7 unreachable) is the page's one flash, `tone=danger`, passed to `page()` as `flash` because the form re-renders in place
8 instead of redirecting.
src/web/layout.ts +8 −2
@@ -51,6 +51,11 @@ export interface PaletteItem {
5151 export interface PageOptions {
5252 /** The request: user, theme and flash all come from here. */
5353 ctx: Ctx;
54 /**
55 * A flash for this response itself, for a form that re-renders instead of
56 * redirecting. It stands in for the cookie's, so a page never shows two.
57 */
58 flash?: Flash;
5459 title: string;
5560 /**
5661 * One sentence for the places that show a page before it is opened: a
@@ -511,7 +516,8 @@ export function page(options: PageOptions, status = 200): Response {
511516 const placeholder = secret();
512517 const { ctx } = options;
513518 const theme = ctx.theme ?? options.theme;
514 const flash = readFlash(ctx.request);
519 const cookieFlash = readFlash(ctx.request);
520 const flash = options.flash ?? cookieFlash;
515521 const links = pageLinks(ctx);
516522
517523 // The body's markup, built before the document so the icons it used can be
@@ -669,7 +675,7 @@ export function page(options: PageOptions, status = 200): Response {
669675 link: linkHeader(links),
670676 });
671677 // A flash is shown exactly once: rendering it is also what clears it.
672 if (flash) headers.append("set-cookie", clearFlashCookie(ctx.url));
678 if (cookieFlash) headers.append("set-cookie", clearFlashCookie(ctx.url));
673679 if (tagged) {
674680 const etag = weakEtag(text);
675681 headers.set("etag", etag);
src/web/pages/edit.ts +68 −16
@@ -64,6 +64,11 @@ export interface Draft {
6464 */
6565 propose?: ProposeTarget;
6666 error?: string;
67 /**
68 * The field an error is about. Without it the error is the form's own (not
69 * allowed, branch moved) and is the page's flash.
70 */
71 errorOn?: "path" | "message" | "branch";
6772 }
6873
6974 /**
@@ -161,6 +166,10 @@ function editorForm(
161166 const encoded = encodeURIComponent(draft.ref);
162167 const suggestion =
163168 draft.branch !== "" ? draft.branch : suggestBranch(username, draft.path);
169 const pathError =
170 draft.error !== undefined && draft.errorOn === "path" && draft.creating
171 ? draft.error
172 : undefined;
164173
165174 return html`<form
166175 method="post"
@@ -185,6 +194,9 @@ function editorForm(
185194 autocomplete="off"
186195 spellcheck="false"
187196 required
197 ${pathError
198 ? raw('aria-invalid="true" aria-describedby="f-path-err"')
199 : null}
188200 />`,
189201 )}
190202 ${when(!draft.creating, () =>
@@ -232,6 +244,13 @@ function editorForm(
232244 )}
233245 </div>
234246
247 ${when(
248 pathError,
249 (message) =>
250 html`<div class="field" data-invalid>
251 <span class="error" id="f-path-err">${message}</span>
252 </div>`,
253 )}
235254 <div
236255 class="editor-wrap"
237256 id="editor-write"
@@ -277,6 +296,9 @@ ${draft.content}</textarea
277296 ? `Create ${draft.path || "a file"}`
278297 : `Update ${draft.path}`,
279298 required: true,
299 ...(draft.error && draft.errorOn === "message"
300 ? { error: draft.error }
301 : {}),
280302 })}
281303 ${field({
282304 label: "Extended description",
@@ -410,6 +432,10 @@ function proposeBlock(
410432 propose: ProposeTarget,
411433 suggestion: string,
412434 ): Html {
435 const branchError =
436 draft.error !== undefined && draft.errorOn === "branch"
437 ? draft.error
438 : undefined;
413439 return html`<fieldset class="editor-target propose">
414440 <legend>${icon("fork")}Propose a change</legend>
415441 <p class="propose-line">
@@ -437,7 +463,15 @@ function proposeBlock(
437463 autocomplete="off"
438464 spellcheck="false"
439465 required
466 ${branchError
467 ? raw('aria-invalid="true" aria-describedby="f-branch-err"')
468 : null}
440469 />
470 ${when(
471 branchError,
472 (message) =>
473 html`<span class="error" id="f-branch-err">${message}</span>`,
474 )}
441475 <span class="hint"
442476 >The pull's head, as
443477 <span class="mono">${propose.fork.split("/")[0]}:${suggestion}</span
@@ -467,11 +501,22 @@ export async function editorPage(
467501 ): Promise<Response> {
468502 const username = ctx.auth.user?.username ?? "edit";
469503 const shape = mirrorFor(draft.path, draft.content);
504 // An error with no field of its own is the form's: a flash, as everywhere.
505 const formLevel =
506 draft.error !== undefined &&
507 (draft.errorOn === undefined ||
508 (draft.errorOn === "path" && !draft.creating) ||
509 (draft.errorOn === "branch" && draft.propose === undefined))
510 ? draft.error
511 : undefined;
470512 // Only an editor that will actually use the tokenizer pays for it — and
471513 // only that page relaxes the script CSP to admit a file from this origin.
472514 const asset = shape.mirror ? await highlightAsset() : null;
473515 return page({
474516 ctx,
517 ...(formLevel !== undefined
518 ? { flash: { tone: "danger" as const, message: formLevel } }
519 : {}),
475520 title: `${draft.creating ? "New file" : draft.path} · ${scope.facts.name}`,
476521 crumbs: scope.crumbs,
477522 palette: scope.palette,
@@ -491,13 +536,6 @@ export async function editorPage(
491536 : "Editing a file"}`,
492537 { sub: html`in <span class="mono">${scope.fullName}</span>` },
493538 )}
494 ${when(
495 draft.error,
496 (message) =>
497 html`<p class="error-note" role="alert">
498 ${icon("alert")}<span>${message}</span>
499 </p>`,
500 )}
501539 ${editorForm(scope, draft, username, {
502540 lang: shape.lang,
503541 mirror: shape.mirror && asset !== null,
@@ -715,15 +753,22 @@ export async function runContribute(
715753 form: Form,
716754 ): Promise<Response> {
717755 const draft = proposeDraft(form, propose);
718 const refuse = (message: string): Promise<Response> =>
719 editorPage(ctx, scope, { ...draft, error: message });
756 const refuse = (
757 message: string,
758 errorOn?: Draft["errorOn"],
759 ): Promise<Response> =>
760 editorPage(ctx, scope, {
761 ...draft,
762 error: message,
763 ...(errorOn ? { errorOn } : {}),
764 });
720765
721766 if (!isSafeRef(draft.ref)) return notFoundPage(ctx);
722767 if (draft.path === "" || !isSafeTreePath(draft.path))
723 return refuse("Give the file a path this repository can hold.");
724 if (draft.subject === "") return refuse("A commit needs a message.");
768 return refuse("Give the file a path this repository can hold.", "path");
769 if (draft.subject === "") return refuse("A commit needs a message.", "message");
725770 if (draft.branch === "")
726 return refuse("Give the branch on your fork a name.");
771 return refuse("Give the branch on your fork a name.", "branch");
727772
728773 const message =
729774 draft.body === "" ? draft.subject : `${draft.subject}\n\n${draft.body}`;
@@ -830,13 +875,20 @@ export async function editSubmit(ctx: Ctx): Promise<Response> {
830875 creating,
831876 mustBranch,
832877 };
833 const refuse = (message: string): Promise<Response> =>
834 editorPage(ctx, scope, { ...draft, error: message });
878 const refuse = (
879 message: string,
880 errorOn?: Draft["errorOn"],
881 ): Promise<Response> =>
882 editorPage(ctx, scope, {
883 ...draft,
884 error: message,
885 ...(errorOn ? { errorOn } : {}),
886 });
835887
836888 if (!isSafeRef(ref)) return notFoundPage(ctx);
837889 if (path === "" || !isSafeTreePath(path))
838 return refuse("Give the file a path this repository can hold.");
839 if (subject === "") return refuse("A commit needs a message.");
890 return refuse("Give the file a path this repository can hold.", "path");
891 if (subject === "") return refuse("A commit needs a message.", "message");
840892
841893 const message = description === "" ? subject : `${subject}\n\n${description}`;
842894 const written = await editFiles(ctx.app, ctx.auth.user, repo, {
src/web/pages/issues.ts +1 −2
@@ -26,7 +26,7 @@ import {
2626 signInHref,
2727 } from "../chrome.ts";
2828 import { field, readForm } from "../form.ts";
29 import { html, raw, when, type Html } from "../html.ts";
29 import { html, raw, type Html } from "../html.ts";
3030 import { flashTo, page } from "../layout.ts";
3131 import { btn, empty, field as formField } from "../ui.ts";
3232 import {
@@ -88,7 +88,6 @@ function newIssueForm(
8888 ): Html {
8989 return html`<div class="narrow-wide mt">
9090 <form method="post" action="${repoBase(repo)}/issues/new">
91 ${when(error, (message) => html`<p class="error-note">${message}</p>`)}
9291 ${formField({
9392 label: "Title",
9493 name: "title",
src/web/pages/pulls.ts +1 −1
@@ -67,7 +67,6 @@ function composeForm(
6767 const cross = parent !== undefined;
6868 return html`<form method="post" action="${repoBase(repo)}/pulls/new">
6969 <input type="hidden" name="baseRepo" value="${cross ? "parent" : "self"}" />
70 ${when(error, (message) => html`<p class="error-note">${message}</p>`)}
7170 ${card(
7271 html`<div class="field-row">
7372 <div class="field">
@@ -102,6 +101,7 @@ function composeForm(
102101 value: values.title,
103102 required: true,
104103 autofocus: true,
104 ...(error ? { error } : {}),
105105 })}
106106 ${formField({
107107 label: "Description",
test/issues.test.ts +13
@@ -167,6 +167,19 @@ describe("issue pages end to end", () => {
167167 expect(detail).not.toContain('data-state="closed"');
168168 });
169169
170 test("an empty issue title is refused in the title field", async () => {
171 const res = await fetch(`${base}/mel/proj/issues/new`, {
172 method: "POST",
173 headers: { cookie },
174 body: form({ title: "", body: "" }),
175 redirect: "manual",
176 });
177 expect(res.status).toBe(422);
178 const page = await res.text();
179 expect(page).not.toContain("error-note");
180 expect(page).toMatch(/<div class="field" data-invalid>[\s\S]*?id="f-title"[^>]*aria-invalid="true"[\s\S]*?class="error"/);
181 });
182
170183 test("cross-origin issue post is refused (CSRF)", async () => {
171184 const res = await fetch(`${base}/mel/proj/issues/new`, {
172185 method: "POST",
test/pulls.test.ts +13
@@ -127,6 +127,19 @@ describe("pull requests end to end", () => {
127127 expect(pr).toContain("Ready to merge");
128128 });
129129
130 test("an empty pull title is refused in the title field", async () => {
131 const res = await post("/dev/app/pulls/new", {
132 base: "main",
133 head: "main",
134 title: "",
135 body: "",
136 });
137 expect(res.status).toBe(422);
138 const page = await res.text();
139 expect(page).not.toContain("error-note");
140 expect(page).toMatch(/<div class="field" data-invalid>[\s\S]*?id="f-title"[^>]*aria-invalid="true"[\s\S]*?class="error"/);
141 });
142
130143 test("merge the PR, base advances, PR shows merged", async () => {
131144 const merged = await post("/dev/app/pulls/1/merge", { strategy: "merge" });
132145 expect(merged.status).toBe(303);
test/web-workflows.test.ts +37 −1
@@ -432,7 +432,43 @@ describe("fork at the origin, and propose a change through it", () => {
432432 expect(html).toContain("second go");
433433 expect(html).toContain("the body I typed");
434434 expect(html).toContain('value="alice/second-go"');
435 expect(html).toContain("error-note");
435 // A refusal that is the whole form's sits in a danger toast, and no
436 // field claims to be invalid.
437 expect(html).toMatch(/class="toast" data-tone="danger"/);
438 expect(html.match(/class="toast"/g)).toHaveLength(1);
439 expect(html).not.toContain("data-invalid");
440 expect(html).not.toContain("aria-invalid");
441 expect(html).not.toContain("error-note");
442 });
443
444 test("a missing commit message or file path is the error of that field", async () => {
445 const form = {
446 ref: "main",
447 path: "a.txt",
448 content: "x\n",
449 message: "",
450 branch: "alice/x",
451 };
452 const noMessage = await (
453 await web(alice, `${browse}/contribute`, { form })
454 ).text();
455 expect(noMessage).toMatch(
456 /<div class="field" data-invalid>[\s\S]*?id="f-message"[^>]*aria-invalid="true"[\s\S]*?class="error"[^>]*>\s*A commit needs a message/,
457 );
458 expect(noMessage).not.toContain("data-tone=\"danger\"");
459
460 const noPath = await (
461 await web(alice, `${browse}/contribute`, {
462 form: { ...form, path: "", message: "m", creating: "1" },
463 })
464 ).text();
465 expect(noPath).toMatch(
466 /name="path"[^>]*aria-invalid="true"[^>]*aria-describedby="f-path-err"/,
467 );
468 expect(noPath).toMatch(
469 /<span class="error" id="f-path-err">Give the file a path/,
470 );
471 expect(noPath).not.toContain("error-note");
436472 });
437473
438474 test("contribute refuses a cross-origin form, and anonymous is sent to sign in", async () => {

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs