From b809b68da6f8d5465fb1274860827949e56a9f78 Mon Sep 17 00:00:00 2001 From: Anas Hasanin <98191932+GamingDragonwastaken@users.noreply.github.com> Date: Sat, 29 Aug 2026 05:44:47 +0300 Subject: [PATCH 1/2] fix(app): require review notes to contain readable text Rejecting a guide is supposed to carry a note the author can act on, but the client only checked `notes.length === 0`. A note of pure whitespace cleared that check and was submitted, where `createDecisionSchema` rejected it against `z.string().trim().min(1)`. The reviewer got a generic API error instead of the guidance the sidebar already knows how to give, after a needless round trip. Validate on whether the note contains a letter or a number instead, so whitespace and bare punctuation are caught before submitting. The character class is Unicode-aware, so notes written in non-Latin scripts count as text. The check moves to `lib/reviewValidation.ts` alongside the existing `reviewDeadline` helper. It was previously a closure inside the component, which meant it could not be tested without mounting the whole review sidebar and its router and revision data. No copy changes: a blank note still reports "Rejections require a note", matching what the field already asked for. Closes #341 Signed-off-by: Anas Hasanin <98191932+GamingDragonwastaken@users.noreply.github.com> --- app/src/components/sidebar/ReviewSidebar.tsx | 15 +-- .../lib/__tests__/reviewValidation.test.ts | 94 +++++++++++++++++++ app/src/lib/reviewValidation.ts | 36 +++++++ 3 files changed, 132 insertions(+), 13 deletions(-) create mode 100644 app/src/lib/__tests__/reviewValidation.test.ts create mode 100644 app/src/lib/reviewValidation.ts diff --git a/app/src/components/sidebar/ReviewSidebar.tsx b/app/src/components/sidebar/ReviewSidebar.tsx index 3ca442ea..7552ec80 100644 --- a/app/src/components/sidebar/ReviewSidebar.tsx +++ b/app/src/components/sidebar/ReviewSidebar.tsx @@ -14,6 +14,7 @@ import { Combobox } from "@/components/ui/combobox"; import { cn } from "@/lib/utils"; import { deadlineTickMs, formatTimeRemaining } from "@/lib/reviewDeadline"; +import { validateReviewDecision } from "@/lib/reviewValidation"; import { castDecision } from "@/lib/api/reviews"; import { getRevision, reviseRevision } from "@/lib/api/guideRevisions"; import { GuidelinesModal } from "@/components/modals/GuidelinesModal"; @@ -102,19 +103,7 @@ export const ReviewSidebar = ({ revisionData.case.status !== "approved" && revisionData.case.status !== "rejected"; - const validateReview = () => { - if (review.decision === "") - return "Choose approve or reject before submitting"; - if (review.decision === "approve") return ""; - - const missing = []; - if (review.reasons.length === 0) missing.push("at least one reason"); - if (review.notes.length === 0) missing.push("a note"); - - return missing.length === 0 - ? "" - : `Rejections require ${missing.join(" and ")}`; - }; + const validateReview = () => validateReviewDecision(review); const submitDecision = async () => { abortControllerRef.current?.abort(); diff --git a/app/src/lib/__tests__/reviewValidation.test.ts b/app/src/lib/__tests__/reviewValidation.test.ts new file mode 100644 index 00000000..9ce66e47 --- /dev/null +++ b/app/src/lib/__tests__/reviewValidation.test.ts @@ -0,0 +1,94 @@ +import { describe, expect, it } from "vitest"; + +import { + hasMeaningfulText, + validateReviewDecision, +} from "@/lib/reviewValidation"; + +const rejection = (notes: string) => ({ + decision: "reject", + notes, + reasons: ["inaccurate"], +}); + +describe("hasMeaningfulText", () => { + it("accepts ordinary prose", () => { + expect(hasMeaningfulText("Needs a worked example.")).toBe(true); + }); + + it("accepts notes written in non-Latin scripts", () => { + expect(hasMeaningfulText("需要一个例子")).toBe(true); + expect(hasMeaningfulText("يحتاج إلى مثال")).toBe(true); + expect(hasMeaningfulText("Нужен пример")).toBe(true); + }); + + it("accepts a note that is only digits", () => { + expect(hasMeaningfulText("42")).toBe(true); + }); + + it("rejects an empty string", () => { + expect(hasMeaningfulText("")).toBe(false); + }); + + it("rejects whitespace of any kind", () => { + expect(hasMeaningfulText(" ")).toBe(false); + expect(hasMeaningfulText("\t\n")).toBe(false); + }); + + it("rejects punctuation with no words", () => { + expect(hasMeaningfulText("...")).toBe(false); + expect(hasMeaningfulText("???")).toBe(false); + expect(hasMeaningfulText("-")).toBe(false); + }); +}); + +describe("validateReviewDecision", () => { + it("asks for a decision before anything else", () => { + expect( + validateReviewDecision({ decision: "", notes: "", reasons: [] }) + ).toBe("Choose approve or reject before submitting"); + }); + + it("lets an approval through without notes or reasons", () => { + expect( + validateReviewDecision({ decision: "approve", notes: "", reasons: [] }) + ).toBe(""); + }); + + it("lets a complete rejection through", () => { + expect(validateReviewDecision(rejection("Missing prerequisites."))).toBe( + "" + ); + }); + + // Regression test for #341: a note of only whitespace cleared the old + // `notes.length === 0` check, so it reached the API and failed there against + // `z.string().trim().min(1)` with a generic error instead of this guidance. + it("rejects a note that is only whitespace", () => { + expect(validateReviewDecision(rejection(" "))).toBe( + "Rejections require a note" + ); + }); + + it("rejects a note that is only punctuation", () => { + expect(validateReviewDecision(rejection("..."))).toBe( + "Rejections require a note" + ); + }); + + it("still requires at least one reason", () => { + expect( + validateReviewDecision({ + decision: "reject", + notes: "Needs work.", + reasons: [], + }) + ).toBe("Rejections require at least one reason"); + }); + + it("reports both problems together", () => { + expect( + validateReviewDecision({ decision: "reject", notes: " ", reasons: [] }) + ).toBe("Rejections require at least one reason and a note"); + }); +}); diff --git a/app/src/lib/reviewValidation.ts b/app/src/lib/reviewValidation.ts new file mode 100644 index 00000000..eb9e7691 --- /dev/null +++ b/app/src/lib/reviewValidation.ts @@ -0,0 +1,36 @@ +export type ReviewDecisionInput = { + decision: string; + notes: string; + reasons: Array; +}; + +// A note has to carry something a guide author can actually read back. Any +// letter or number in any script counts, so notes are not limited to Latin +// characters; whitespace and bare punctuation are not a note. +const MEANINGFUL_TEXT = /[\p{L}\p{N}]/u; + +export function hasMeaningfulText(value: string): boolean { + return MEANINGFUL_TEXT.test(value); +} + +/** + * Describes what is stopping a decision from being submitted, or returns an + * empty string when it is ready to go. + * + * `createDecisionSchema` already requires `notes` to survive a trim on the + * server, so a blank-but-not-empty note used to clear this check and fail at + * the API instead, surfacing a generic error rather than the guidance below. + */ +export function validateReviewDecision(review: ReviewDecisionInput): string { + if (review.decision === "") + return "Choose approve or reject before submitting"; + if (review.decision === "approve") return ""; + + const missing = []; + if (review.reasons.length === 0) missing.push("at least one reason"); + if (!hasMeaningfulText(review.notes)) missing.push("a note"); + + return missing.length === 0 + ? "" + : `Rejections require ${missing.join(" and ")}`; +} From 99d6c0e3ef966005d58e96cfacfed2217b4d1125 Mon Sep 17 00:00:00 2001 From: Anas Hasanin <98191932+GamingDragonwastaken@users.noreply.github.com> Date: Sun, 30 Aug 2026 23:50:10 +0300 Subject: [PATCH 2/2] refactor(app): trim comments and tests on review note validation Address review feedback: drop the comment history from the validator docblock and the issue reference from the test, and fold the assertions into fewer cases so the file sits closer to the other lib tests. Signed-off-by: Anas Hasanin <98191932+GamingDragonwastaken@users.noreply.github.com> --- .../lib/__tests__/reviewValidation.test.ts | 25 +++---------------- app/src/lib/reviewValidation.ts | 9 ++----- 2 files changed, 5 insertions(+), 29 deletions(-) diff --git a/app/src/lib/__tests__/reviewValidation.test.ts b/app/src/lib/__tests__/reviewValidation.test.ts index 9ce66e47..20df5bf7 100644 --- a/app/src/lib/__tests__/reviewValidation.test.ts +++ b/app/src/lib/__tests__/reviewValidation.test.ts @@ -12,32 +12,19 @@ const rejection = (notes: string) => ({ }); describe("hasMeaningfulText", () => { - it("accepts ordinary prose", () => { + it("accepts letters and numbers in any script", () => { expect(hasMeaningfulText("Needs a worked example.")).toBe(true); - }); - - it("accepts notes written in non-Latin scripts", () => { expect(hasMeaningfulText("需要一个例子")).toBe(true); expect(hasMeaningfulText("يحتاج إلى مثال")).toBe(true); expect(hasMeaningfulText("Нужен пример")).toBe(true); - }); - - it("accepts a note that is only digits", () => { expect(hasMeaningfulText("42")).toBe(true); }); - it("rejects an empty string", () => { + it("rejects input with no letters or numbers", () => { expect(hasMeaningfulText("")).toBe(false); - }); - - it("rejects whitespace of any kind", () => { expect(hasMeaningfulText(" ")).toBe(false); expect(hasMeaningfulText("\t\n")).toBe(false); - }); - - it("rejects punctuation with no words", () => { expect(hasMeaningfulText("...")).toBe(false); - expect(hasMeaningfulText("???")).toBe(false); expect(hasMeaningfulText("-")).toBe(false); }); }); @@ -61,16 +48,10 @@ describe("validateReviewDecision", () => { ); }); - // Regression test for #341: a note of only whitespace cleared the old - // `notes.length === 0` check, so it reached the API and failed there against - // `z.string().trim().min(1)` with a generic error instead of this guidance. - it("rejects a note that is only whitespace", () => { + it("rejects a note with no readable text", () => { expect(validateReviewDecision(rejection(" "))).toBe( "Rejections require a note" ); - }); - - it("rejects a note that is only punctuation", () => { expect(validateReviewDecision(rejection("..."))).toBe( "Rejections require a note" ); diff --git a/app/src/lib/reviewValidation.ts b/app/src/lib/reviewValidation.ts index eb9e7691..80a4275b 100644 --- a/app/src/lib/reviewValidation.ts +++ b/app/src/lib/reviewValidation.ts @@ -4,9 +4,8 @@ export type ReviewDecisionInput = { reasons: Array; }; -// A note has to carry something a guide author can actually read back. Any -// letter or number in any script counts, so notes are not limited to Latin -// characters; whitespace and bare punctuation are not a note. +// Any letter or number in any script counts, so notes are not limited to Latin +// characters. Whitespace and bare punctuation are not a note. const MEANINGFUL_TEXT = /[\p{L}\p{N}]/u; export function hasMeaningfulText(value: string): boolean { @@ -16,10 +15,6 @@ export function hasMeaningfulText(value: string): boolean { /** * Describes what is stopping a decision from being submitted, or returns an * empty string when it is ready to go. - * - * `createDecisionSchema` already requires `notes` to survive a trim on the - * server, so a blank-but-not-empty note used to clear this check and fail at - * the API instead, surfacing a generic error rather than the guidance below. */ export function validateReviewDecision(review: ReviewDecisionInput): string { if (review.decision === "")