fix(app): require review notes to contain readable text - #382
Open
GamingDragonwastaken wants to merge 1 commit into
Open
fix(app): require review notes to contain readable text#382GamingDragonwastaken wants to merge 1 commit into
GamingDragonwastaken wants to merge 1 commit into
Conversation
GamingDragonwastaken
requested review from
andyrodrigues30 and
tonylam0
as code owners
August 29, 2026 02:45
Collaborator
|
@GamingDragonwastaken - we appreciate the work you have done on this. In order for our PR reviews to be more efficient please can you keep the summary short and concise. Thank you! |
Contributor
Author
|
Apologies, and thanks for the feedback — that's fair. I've trimmed the description down to the problem, the change, and the one open question. I'll keep summaries short going forward and let the diff speak for itself. |
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 bluelearn-org#341 Signed-off-by: Anas Hasanin <98191932+GamingDragonwastaken@users.noreply.github.com>
GamingDragonwastaken
force-pushed
the
fix/require-meaningful-review-notes
branch
from
August 29, 2026 12:25
7577ed7 to
b809b68
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #341.
The client checked
notes.length === 0, whilecreateDecisionSchemarequiresz.string().trim().min(1). A whitespace-only note therefore passed the client gate and failed at the API, showing a generic error instead of the sidebar's own message.Now checks that the note contains a letter or number, so whitespace and bare punctuation are caught before submitting. The check moved to
lib/reviewValidation.tsso it can be unit tested; the call site is otherwise unchanged.One question: this is deliberately stricter than the schema —
"..."passes zod but fails here — matching "at least one word" from the issue. Happy to relax it to a plain trim, or to tightencreateDecisionSchemaso both match. Say which and I'll push a follow-up.Type of change
Verification
pnpm -r typecheckpassespnpm -r buildpasses41 tests pass (13 new). I couldn't exercise the sidebar in a browser — reaching a case needs a local Supabase and I don't have Docker on this machine — so the logic is covered by unit tests instead.