OTTER-737: count characters instead of words in all capped input fields - #991
Conversation
5262079 to
5b5b07b
Compare
nathanstitt
left a comment
nathanstitt
left a comment
There was a problem hiding this comment.
looks great, all seem very minor but I do wonder if we should use the suggested Intl.Segmenter
| // Submitting is the gate, so the cap is checked here rather than in the params schema: this | ||
| // action also carries drafts whose stored title predates the cap, and a schema rejection | ||
| // would surface as a generic failure instead of a message on the field (OTTER-737). | ||
| if (countCharacters(submittedTitle) > STUDY_TITLE_MAX_CHARACTERS) { |
There was a problem hiding this comment.
I think this one recreates the exact dead-end the PR set out to remove, just moved from autosave to submit.
On the Step 2 proposal submit path, use-submit-proposal.ts calls buildStudyInfo(values, 'omit'), so title is never sent. submittedTitle above then falls back to the stored column, and this check rejects it. But Step 2 renders no title field (proposal/form.tsx has the explicit {/* No Study title field: it moved to Step 1 ... */} comment), and proposal/page.tsx only redirects to Step 1 when the title is blank (if (!result.title?.trim())) — never when it is merely too long.
So for a DRAFT created before OTTER-690 with a 70-character title, the researcher gets ActionFailure({ title: ... }) keyed to a field that isn't on the page. use-submit-proposal's onError just shows the generic "Proposal could not be submitted" toast. They can't shorten it, and can't submit — a hard stop with no stated cause, which is exactly what the draftStudyApiSchema note describes as the thing to avoid.
A couple of ways out, whichever fits the product better:
- widen the
page.tsxguard to!result.title?.trim() || countCharacters(result.title) > STUDY_TITLE_MAX_CHARACTERSso the researcher lands on Step 1, which owns and renders the field; or - only apply the cap here when the title actually came from the request (
'title' in snapshotFields), leaving a stored pre-cap title to be fixed on Step 1.
Worth a test for it too — the existing finalizeStudySubmissionAction rejects an over-limit title test passes title explicitly, so it exercises the path that already has a visible field rather than the omit path that doesn't.
There was a problem hiding this comment.
You're right, and thanks for tracing it all the way through. I confirmed every step: buildStudyInfo(values, 'omit') means title never reaches the action, submittedTitle falls back to the stored column, and page.tsx only redirected on a blank title. A pre-OTTER-690 draft with a 70-character title lands on Step 2, has no field to fix it on, and gets the generic "Proposal could not be submitted" toast. Exactly the dead end the draftStudyApiSchema note was written about, moved from the autosave to the submit.
Took your first option, widening the page.tsx guard, rather than the second. Only applying the cap when the title came from the request would let an over-limit title through to PENDING-REVIEW, which defeats the cap. Sending the researcher to Step 1 instead puts them on the page that owns and renders the field, where the counter reads 70/60 and the error names the problem:
management-app/src/app/[orgSlug]/study/[studyId]/proposal/page.tsx
Lines 35 to 48 in 29fe5ca
The throws in the action stay as a backstop, and the comment above them no longer claims the redirect covers a case it didn't. It now says which case each one is actually for:
management-app/src/server/actions/study-request.ts
Lines 433 to 442 in 29fe5ca
Tests, including the omit path you pointed out was the one not covered:
finalizeStudySubmissionAction rejects an over-limit stored title when none is submitted- notitlein the payload, stored title over the cap, asserts the error and that the study stays DRAFT.sends a DRAFT whose stored title is over the cap back to Step 1, plus one asserting a title exactly at the cap still renders Step 2.
| * definition for the counter beside the field, the client rule and the server rule: measuring the | ||
| * same value two ways is what lets a field read 1800/1800 while its validator sees 1801. | ||
| */ | ||
| export const countCharacters = (value: string) => value.trim().length |
There was a problem hiding this comment.
Since this is now the single definition of "how long is this field" for the whole app, it's worth being deliberate about what a .length unit actually counts.
.length is UTF-16 code units, so a few things count higher than what the user perceives:
countCharacters('é') // 2 when NFD-composed (e + combining accent)
countCharacters('👨👩👧👦') // 11For a 1800-char body field nobody will notice. For the 60-character study title it's much easier to hit — a researcher pasting an accented title from Word (which often emits NFD) can watch the counter jump past 60 on a title that visibly has ~40 letters, with no way to see why.
The counter and the validator do agree here, so it isn't a correctness split — it's a UX sharp edge. [...value.trim()].length gets code points cheaply, or Intl.Segmenter with granularity: 'grapheme' gets true user-perceived characters and is available in every runtime this app targets. Might be worth at least a normalize (value.normalize('NFC')) before counting, which fixes the accent case for near-free.
There was a problem hiding this comment.
Done, and you called the right field: the 60-character title is where this bites. Went with Intl.Segmenter at grapheme granularity rather than code points, since [...v].length still charges 7 for the family emoji.
management-app/src/lib/field-limits.ts
Lines 12 to 42 in 29fe5ca
Two notes on the shape of it:
No normalize('NFC') pass. Grapheme clustering subsumes it. Either encoding of "é" is one cluster, so NFC would be a second O(n) pass buying nothing. Verified both directions in the tests, written as é and é escapes so the assertion doesn't depend on how the file gets normalized on disk.
ASCII fast path. .length is already correct for plain ASCII, and this runs per keystroke on fields up to 6000 characters. Measured on Node 24: a full 6000-character summary is ~0.27ms segmented against ~0.002ms for the regex test. CR is excluded from the class because \r\n is the one ASCII pair UAX #29 joins into a single cluster, which is the only place the shortcut and the segmenter would disagree. There's a test pinning that.
Intl.Segmenter is safe here: engines.node is >=22, and the browser floor is Firefox 125 (Apr 2024), which is well behind what this app targets.
New file, src/lib/field-limits.test.ts, covers the whitespace rule, both encodings, the emoji, and the CRLF case.
| * The wording is the card's, verbatim, down to the missing "character" before "limit". | ||
| */ | ||
| export const overCharacterLimitError = (fieldTitle: string, maxCharacters: number) => | ||
| `${fieldTitle} exceeds the ${maxCharacters} limit. Shorten it to continue.` |
There was a problem hiding this comment.
Small copy flag, mostly so it's a conscious decision rather than an inherited one.
The PR description already calls this out, and I agree the resulting sentence reads oddly — "Study title exceeds the 60 limit" is missing its unit noun. Reading the card as "the message must be exact" is defensible, but design cards usually write the example with the field's own number filled in rather than specifying the literal token stream, and "60 limit" is the kind of thing that reads as a template bug to a user who has never seen the card.
No change needed from me — but this is worth an explicit yes/no from design before merge, since it's ten user-visible strings and cheap to flip either way while it's still one function.
There was a problem hiding this comment.
Leaving it as the card has it for now, so this stays a design call rather than one I make quietly in passing. Agreed on the substance though: "Study title exceeds the 60 limit" reads like a template that lost its noun, and your point that cards usually fill in the field's own number rather than pin the literal token stream is the more likely reading of it.
It's still one function (overCharacterLimitError), so all ten strings flip by adding one word there. I'll get a yes/no from design before this merges and change it in that one line if they want the word back.
| @@ -0,0 +1,81 @@ | |||
| 'use client' | |||
There was a problem hiding this comment.
This file and decision-feedback.ts are good work, but I think they're the reason this PR is hard to review, and they'd be better off on their own.
Breaking down the churn, the PR is doing three things:
- convert 10 capped fields from words to characters — the card
- delete
WordCounter/countWords/countWordsFromLexical— unavoidable, nothing can reference them once (1) lands - dedup three copies of the Decision editor and its server validation — not required by the card
(1) and (2) are genuinely inseparable: you can't leave a word counter behind when no field counts words. (3) is separable — this file (+81), decision-feedback.ts (+32) and decision-feedback.test.ts (+53) are self-contained, touch a different surface, and would stand alone as a small, obviously-correct PR. That's ~165 lines that don't need to ride along with a 55-file conversion.
Splitting it would also help the stack: this PR is already third in a chain behind #980 and #988, so anything that isn't the character conversion widens the window where all three have to stay rebased together.
Not asking for a re-roll if you'd rather not — the code itself is right, and the ARIA-drift argument for extracting it is a good one. Mostly flagging that the two commits answer to different cards and could land on different days.
For what it's worth, I checked whether the test additions are padding, and they're not — study-request.test.ts had 45 tests on main and not one asserted a server-side length cap. The 16 tests removed from lexical.test.ts are all word-counting tests for functions this PR deletes, so that file shrinking is correct rather than lost coverage. The at-cap / one-over pairs are the coverage this area was missing.
There was a problem hiding this comment.
Fair reading of the churn, and your three-way split is the right one: (1) and (2) are inseparable, (3) isn't.
Keeping it here, since you're not asking for a re-roll. The reason is the ARIA argument you already granted: this round added a fourth change to that shared wiring (the counter id in aria-describedby), and with two copies that's the fourth time it had to be made twice by hand, which is how it drifted in the first place. Splitting it now means either landing the conversion with the drift still in, or landing the extraction first and rebasing a 55-file PR behind it. Given the stack is already three deep behind #980 and #988, I'd rather not add a fourth rebase point.
Worth noting the review has since pulled a genuinely separable piece in rather than out: the lexical.ts shape consolidation. That one I'd have argued belonged on its own card, except the drift it fixes is live today, so same reasoning in reverse.
Point taken for next time though. This should have been two PRs from the start, and the dedup commit would have been the easy half to review.
And thanks for actually checking the test diff rather than taking the count at face value. You're right that study-request.test.ts had no server-side length assertion at all before this, which is why the 501-word proposal review fixture passed for the wrong reason at ~4000 characters.
| @@ -90,31 +70,32 @@ export function isValidLexicalState(json: string | undefined): boolean { | |||
| } | |||
There was a problem hiding this comment.
(Anchored here on isValidLexicalState because GitHub won't take a comment on line 52 — this is about the module as a whole.)
The note fields can arrive as either Lexical JSON or plain text, and right now every call site re-decides which one it's holding. Counting the shape checks at head:
edit-and-resubmit/schema.ts:18—isValidLexicalState(v) ? countCharactersFromLexical(v) : countCharacters(v)edit-and-resubmit/schema.ts:23—(isValidLexicalState(v) ? extractTextFromLexical(v) : v).trim().length === 0edit-and-resubmit/schema.ts:30—isValidLexicalState(v) ? v : lexicalJson(v)lexical.ts:89—normalizeFeedbackToLexicalhand-rolls its ownlooksLikeLexicalRootfor the same decisioneditable-text.tsx:58— checks the shape again before renderinghasLexicalContent— doesn't check at all, and so reports plain text as empty
That's five branches and one omission for a single question. I'd rather see the shape decided once, at the boundary, so nothing downstream has to ask:
/** Every note/feedback value, whatever shape it arrived in, as plain text. */
export function lexicalToText(value: string | undefined): string {
if (!value) return ''
const state = tryParseLexicalRoot(value)
return state ? extractTextFromLexicalNode(state) : value
}Then the branches stop existing rather than getting tidier:
countCharacters(lexicalToText(v)) // replaces :18 and countCharactersFromLexical
!lexicalToText(v).trim() // replaces :23
hasLexicalContent = (...f) => f.some(x => !!lexicalToText(x).trim())resubmissionNoteCharacterCount and resubmissionNoteIsBlank then have no bodies left worth keeping, and hasLexicalContent gets its plain-text bug fixed as a side effect rather than as a separate task.
Two reasons I think this is worth doing in this PR rather than a follow-up:
The duplicate predicates have already drifted. isValidLexicalState and normalizeFeedbackToLexical's looksLikeLexicalRoot are both "is this Lexical?", and they disagree:
| input | isValidLexicalState |
looksLikeLexicalRoot |
|---|---|---|
{"root":{"children":[]}} |
false |
true |
{"root":null} |
false |
true |
So an empty-root state gets wrapped by one path and passed straight through by the other. Nothing visibly breaks today, but that's two normalizers for one concept already diverging — exactly what a third caller inherits.
This PR is what makes it urgent. Before OTTER-737 the branch appeared twice; the character conversion spreads the same decision across every capped field. One function now is cheaper than six branches to keep in sync later.
Happy to be told this is scope creep and belongs on its own card — but if so I'd want the card filed, because the drift above is real today.
There was a problem hiding this comment.
Took this, and the drift table is what sold it. Confirmed both rows: for {"root":{"children":[]}} and {"root":null}, isValidLexicalState says no and looksLikeLexicalRoot says yes, so the same value was wrapped on one path and passed through on the other.
The shape is now decided once, in asLexicalEnvelope, with lexicalToText on top of it:
management-app/src/lib/lexical.ts
Lines 1 to 42 in 29fe5ca
normalizeFeedbackToLexical lost its inline predicate and isValidLexicalState no longer parses for itself, so the two can't disagree again. Both kept their exact previous behavior; I checked each branch, including {"root":null} and {"root":"hi"}. isValidLexicalState stays as a separate question on purpose, since an empty-root document is Lexical but is not a usable initial state for the editor, and three callers depend on that distinction. It just no longer owns a shape check of its own.
One piece I did not take: making hasLexicalContent read plain text. I tried it exactly as sketched and it broke proposalFormSchema > researchQuestions > rejects invalid JSON. The four proposal rich-text fields have no plain-text path, so they rely on a non-Lexical value reading as empty and failing their required rule. Shape-agnostic, 'not valid json' becomes valid prose and passes.
Checking all six callers, that "plain-text bug" turns out to be latent rather than live: every one of them holds an editor value or a value already through normalizeFeedbackToLexical. The one place both shapes genuinely arrive is the resubmission note, so that asks for both explicitly instead. Both helpers keep Lexical-only semantics with the reasoning written down:
management-app/src/lib/lexical.ts
Lines 92 to 102 in 29fe5ca
No follow-up card needed, since the drift itself is gone. Tests for lexicalToText and the empty-root case are in src/lib/lexical.test.ts.
| } | ||
|
|
||
| /** Whether the note has any content at all, ignoring surrounding whitespace. */ | ||
| export function resubmissionNoteIsBlank(value: string): boolean { |
There was a problem hiding this comment.
These three helpers (:18, :23, :30) all open with the same isValidLexicalState(value) ? … : … branch — three answers to one question, "which shape is this note in?", asked three times in twelve lines.
Rather than three ternaries kept in step by hand, I think this wants one normalizer at the boundary so the shape is resolved before any of these run. Sketched it in a comment on lexical.ts, along with evidence that the duplicate predicates have already drifted (isValidLexicalState and normalizeFeedbackToLexical's internal check disagree on an empty root).
With a lexicalToText helper, all three collapse and mostly stop needing to exist:
countCharacters(lexicalToText(value)) // :18
!lexicalToText(value).trim() // :23Worth reading the two together — the change belongs in lexical.ts, but this file is where the cost of not making it shows up.
There was a problem hiding this comment.
All three ternaries are gone. The shape is resolved by lexicalToText before any of these run, so none of them branches on it any more:
Kept the three names, since they have real callers across the components, the server actions and the tests, and this PR is already wide enough. The bodies are one line each now, which was the point.
:30 picked up a fix on the way past. It used isValidLexicalState as the shape check, so an empty-root document failed it and got wrapped as plain text, meaning the user would have been shown {"root":{"children":[]}} as the body of their own note. It now judges blankness on the text, so an empty document is treated the same as an empty string.
Details on the shared helper and the one part of your sketch I didn't take are in the reply on lexical.ts.
5b5b07b to
598efdf
Compare
|
Thanks @nathanstitt. Answering the question from the review body first: yes on Pushed as 29fe5ca. One of these turned out not to be minor. The
One part of the
|
29fe5ca to
201f7e4
Compare
d97afb3 to
d07282f
Compare
d07282f to
6edf5c2
Compare
normalizeFeedbackToLexical reported a word count alongside the JSON it normalized, which tied it to one unit. It now normalizes only, and each caller measures the result in the unit its own field is capped in. overCharacterLimitError moves out of the researcher proposal schema into src/lib/field-limits, so the resubmission notes and the Data Partner decision fields can raise the same message without importing from a proposal page module.
The CHANGE-REQUESTED resubmit page renders the same study title and four rich-text fields as the proposal, and was the last page still counting words. It now uses the same caps: title 60, research questions 3000, project summary 6000, impact 3000, additional notes 1800. The title rule becomes a factory so both pages share one implementation. Only the blank message differs between them: Step 1 names the action because it raises every empty-field message at once, while the resubmit page keeps the generic wording it uses on its other fields. With both flows on one cap, draftStudyApiSchema can enforce the title length for every entry point. It had to stay permissive while the resubmit autosave ran under a 20-word rule that allowed longer titles, which is why step1DraftStudyApiSchema existed; that override is gone.
The proposal note and the code note were capped at 300 words. Both now use 1800 characters and raise the shared over-limit message. The schema switches from chained refines to superRefine so a blank note reports only that it is blank. Emptiness is measured trimmed and the cap is measured raw, matching the counter beside the field and the split the proposal fields already use. resubmissionNoteWordCount becomes resubmissionNoteCharacterCount and keeps its Lexical-or-plain-text branch: the proposal note posts editor JSON while the code note posts a raw textarea string.
The two review steps used different word caps, 500 on the proposal review and 300 on the code review. The card gives both the same 1800 characters, so the two constants collapse into REVIEW_FEEDBACK_MAX_CHARACTERS and the code review page stops passing an override. The hook renames wordCount to characterCount and maxWords to maxCharacters, and reports the shared over-limit message. Emptiness is measured trimmed so whitespace-only feedback still reads as missing, while the cap is measured raw to match the counter.
The cap on this field used to depend on the run being reviewed, 300 words for an errored run against 1500 for a completed one, on the reasoning that explaining a failure needs less room than summarizing results. The card gives one number for the field, so both outcomes now get 1800 characters. Worth calling out at review: on a completed run this is a real reduction, from roughly 9000 characters' worth of words down to 1800. The unit change is not what drives it, the new number is. With the cap no longer derived from job status, outputsFeedbackMaxWords goes, and the maxWords prop it fed disappears from both reviewer screens, the review panel, the decision section, the hook and the server action. The counter also drops its "words" suffix, which was the only unit label on any counter in the app.
No field counts words any more, so WordCounter, countWords, countWordsFromLexical and EditableText's WordCountPlugin have no callers. The plugin's onWordCount prop had none already: reviewer-preview is the only place that renders EditableText and it never passed one. CharacterCounter's doc comment loses its reference to the sibling it no longer has.
The available-outputs screen pointed at outputsFeedbackMaxWords, which no longer exists, and buildFeedback described itself against a 500-word proposal-review ceiling that is now 1800 characters.
Follow-up on the word-to-character conversion, from review of this branch against OTTER-737. Legacy titles no longer break Edit Proposal. `draftStudyApiSchema` had gained a 60-character title cap, and it is the params schema for the resubmit autosave as well as for resubmission. A study created before OTTER-690 can hold a longer title its owner never chose to edit, so every autosave on that page was rejected inside `.params()`, which also left the footer's Back and "View as reviewer" buttons doing nothing. The cap moves to the actions that submit, where it can be reported on the field: create-draft, the DRAFT branch of the draft update, resubmission, and final submission, which had no title cap at all before. Counts now exclude whitespace at either end, as the card requires, and interior whitespace still counts. One `countCharacters` helper backs every counter and every rule, so the counter and its validator cannot disagree, which is what the previous raw counting was reaching for. The over-limit message matches the card's wording exactly, and the three Data Partner fields call themselves "Decision", the name the card and the pages use. Proposal Review and Code Review raise the over-limit error on the keystroke that crosses the cap and drop it the moment the value is back under, instead of waiting for a blur. `useField` neither validates on change nor keeps an error across one, so the rule is derived rather than left in `validate`. Accessibility: the counter carries an id and is named in every field's `aria-describedby`, and the over-limit message is announced politely on the Edit Proposal title, both resubmission notes and both review decisions. Outputs Review already did both and now uses the counter id rather than borrowing the description slot. Adds the boundary coverage the card asks for: at the cap and one past it for the notes and both review decisions on the server, whitespace handling on every capped field, and counter association plus live-region assertions per field.
Follow-up to the character-cap work, from a duplication pass over the branch. The blank check on a Lexical value was hand-rolled in six places as extractTextFromLexical(x).trim().length === 0. hasLexicalContent already existed in src/lib/lexical.ts, exported and unit-tested, so those call sites now use it. Two are deliberately left alone: the resubmission-note check also accepts plain text, where hasLexicalContent reports a non-empty note as blank, and reviewer-preview.tsx is outside this branch. The three review actions each carried their own normalize, then require, then cap block. Two were byte-identical and the third differed only in which constants it named. They now call assertDecisionFeedback, which takes the field title and the cap as parameters so the two review domains keep their own constants. The proposal review and the code review had a copy each of the Decision editor, differing in the document name, the ids and the copy. What they shared was the error slot, the counter, and the ARIA wiring that ties both to the editor, and that wiring had already drifted once: every change to it had to be made twice by hand. DecisionFeedbackEditor holds it once. Each page keeps its own contentStyle, which differ in height and font size. This does not reduce line count. It costs about 47 lines, in the props type and in the comments recording why each shared piece exists. What it buys is a single definition in the two places that have already proved they drift.
58b174a to
3b59a76
Compare
Total coverage
Detailed report15 files with a coverage regression
|
2 participants
see OTTER-737
Summary
All input fields with a limit now count characters. Before this change, they counted words. The counter below each field, the rule that stops the submission, and the error message all changed together.
Base branch
This branch starts at
OTTER-691-proposal-page-refactor(#988). That branch starts atOTTER-690-setup-page-refactor(#980). Merge #980 first. Then merge #988. Then merge this PR.OTTER-691 made the parts that this work uses: the
CharacterCountercomponent, thecountCharactersFromLexicalfunction, and theerrorLiveoption onFormField. OTTER-691 also changed the four Step 2 proposal fields and the Step 1 study title. This PR changes the fields that OTTER-691 did not change.New limits
The error message
Each field shows this message when the text is too long:
{field title} exceeds the {limit} limit. Shorten it to continue.This is the text from the card, character for character. An earlier commit put the word "character" before "limit". The card does not have that word. The card gives the example "Research question(s) exceeds the 3000 limit...". The card also says that the message must be exact. Thus the app now omits the word.
The result reads strange in English. "Study title exceeds the 60 limit" is not a good sentence. Tell me if you want the word "character" back, and I will put it back in one line of code.
The name of the Data Partner field
The card calls this field "Decision" on all three Data Partner pages. The app called it "Feedback". No page shows a field with the name "Feedback": the proposal review page shows the review round, the code review page shows "Code review", and the outputs review page shows "Decision". A reviewer thus read a message about a field that is not on the screen.
The two constants now hold the name "Decision". The message for an empty field still says "Feedback is required." That message is not part of this card.
How the rules measure the text
The card excludes whitespace at the start of the content and at the end of it. The card includes the whitespace between words. Thus "a b" is 3 characters.
One function does this for the whole app.
countCharactersremoves the whitespace at the two ends, then counts what is left. The counter below each field calls it. The client rule calls it. The server rule calls it. Therefore the counter and the rule that stops the submission always agree, and a field cannot show 1800/1800 and fail with 1801.The unit is the grapheme cluster, not the UTF-16 code unit.
.lengthmeasures storage, not what the user typed. A decomposed "é" costs 2 there, and a family emoji costs 11. On a 6000-character field this makes no difference. On the 60-character title it does: a researcher can paste a title of 40 visible letters out of Word and watch the counter pass 60, with nothing on the screen to explain it.countCharacterssegments the text instead, so each of those counts 1. Segmenting also settles the NFC/NFD question on its own, because either encoding of "é" is one cluster.Pure ASCII text skips the segmenter, because
.lengthis already right for it. A 6000-character summary measures 0.27ms segmented against 0.002ms through that check, and both the counter and the rule run on every keystroke.An earlier commit counted the raw text for this same reason. That was not necessary. The counter and the rule agree if both of them remove the whitespace, and only then does the app also follow the card.
The fields do not truncate the text. The user can type more text than the limit permits. The counter becomes red, the error message appears, and the button does not submit.
The two review pages now show the error at once
The Proposal Review decision and the Code Review decision showed the error only after the user moved away from the field. Mantine
useFielddoes not validate on a change, and it removes the error on a change. Thus two things were wrong. The error did not appear on the character that passed the limit. The error also disappeared again on the next character, while the text was still too long.The hook now calculates the over-limit error on each render. The error appears on the character that passes the limit. The error goes away when the user deletes the extra characters. Outputs Review already worked this way, and the two review pages now match it.
The
maxCharactersoption onuseReviewFeedbackhas no caller in the app. It is deleted.Accessibility
CharacterCounternow takes an id. Each of the ten fields names its counter in thearia-describedbyof its input. Before this change, only Outputs Review did this, and it borrowed the description id for the counter. It now uses the counter id.The over-limit message is in a polite live region on the Edit Proposal title, on both resubmission notes, and on both review decisions. The card asks for this on every field in the list. The Step 1 title, the four proposal fields, and Outputs Review already had it.
Effect on existing data
A study with the status CHANGE-REQUESTED can hold a project summary of 1000 words. This can be more than 6000 characters. The researcher must make such a field shorter before resubmission. The error message tells the researcher what to do.
A study from before OTTER-690 can hold a title of more than 60 characters. An earlier commit put the 60-character limit in
draftStudyApiSchema. That schema is the parameter schema for the autosave on the Edit Proposal page. Thus every autosave failed for such a study, and the researcher saw only "Failed to save draft". The message did not name the title. The Back button and the "View as reviewer" button also did nothing, because both of them wait for a successful save.The limit now sits in the actions that submit.
draftStudyApiSchemais permissive again, and it also trims the title before the app writes it to the database. These actions apply the limit and report it on the field:onSaveDraftStudyAction, because a new study has no old title.onUpdateDraftStudyAction, for a DRAFT row only. The Edit Proposal autosave is the other caller, and that row can hold an old title.resubmitProposalAction. The Edit Proposal page shows the title field, so the researcher can make the title shorter here.finalizeStudySubmissionAction. This action had no title limit before this change.Those two actions are the backstop, not the message the researcher is meant to read. Step 2 renders no title field, so an error keyed to
titlehas nothing to attach to on the page the submit came from./proposalnow sends a draft whose stored title is blank or over the cap to Step 1, which owns the field. Before this, only a blank title was redirected. A draft from before OTTER-690 with a 70-character title reached Step 2, and the submit then failed with the generic "Proposal could not be submitted" toast and no way to shorten the title.Other changes
WordCounter,countWords,countWordsFromLexical, and theWordCountPlugininEditableTexthave no callers. They are deleted.normalizeFeedbackToLexicalno longer reports a word count. It normalizes only. Each caller measures the result in the unit of its own field.overCharacterLimitErrormoves tosrc/lib/field-limits.ts, together withcountCharacters. The reviewer code and the code resubmission note can then use both without an import from a proposal page module.Shared code for the three Decision fields
A second commit removes duplication that this change made easy to see. The line count does not decrease. It increases by approximately 47 lines, because each shared part needs a props type and a comment that tells why it is there. The gain is one definition in the two places that already showed a difference between their copies.
There are three changes.
The test for an empty Lexical value was written by hand in six places, as
extractTextFromLexical(x).trim().length === 0. The functionhasLexicalContentinsrc/lib/lexical.tsdoes this test already, and it has unit tests. Those six places now call it. Two other places keep their own test: the resubmission note also accepts plain text, andhasLexicalContentreports a plain-text note as empty;reviewer-preview.tsxis not part of this branch.The three review actions each had the same block of code: normalize the text, then reject an empty value, then reject a value above the cap. Two of the three blocks were the same, character for character. The third block had different constant names only. All three now call
assertDecisionFeedback. The field title and the cap are parameters of that function, so the two review modules keep their own constants.The proposal review and the code review each had a copy of the Decision editor. The two copies were different in the document name, the ids and the text. The two copies were the same in the error area, the counter, and the
aria-describedbythat connects those two to the editor. That last part had already become different in the two copies, because each change to it was necessary two times.DecisionFeedbackEditornow holds it one time. Each page keeps its owncontentStyle, because the two pages use a different height and a different font size.Caution: two Data Partner fields get less space
Most of the new limits agree with the old limits, because a word is approximately 6 characters. Two fields do not agree:
The reviewer thus gets less space than before. The unit is not the cause. The new number is the cause. The card gives one number for each of these fields, and this PR uses that number. Tell me if this is not correct, and I will change it.
The outputs review limit is now one number
Before this change, the limit for the outputs review decision changed with the result of the run. An errored run had 300 words. A completed run had 1500 words. The card gives one number for the field. Thus the function
outputsFeedbackMaxWordsis not necessary. ThemaxWordsproperty is also not necessary in the two reviewer screens, the review panel, the decision section, the hook, and the server action.Not in this PR
The four rich-text proposal fields have no limit on the server.
step2ProposalApiSchemaandfinalizeStudySubmissionInfoSchemaaccept text of any length, and the client rules are the only gate. This is also true before this PR, so it is not a new hole. A limit there can stop the resubmission of a study that already holds a long field, in the same way the title limit stopped the autosave. That needs its own card and its own decision about the old rows.Changes from code review
Four changes came out of the review of the first round.
The counter counts grapheme clusters. See "How the rules measure the text" above. The review asked whether
Intl.Segmenterwas worth using. It is, on the 60-character title./proposalredirects an over-limit stored title to Step 1. See "Effect on existing data" above. The review found that the title cap onfinalizeStudySubmissionActionrecreated the dead end this PR set out to remove, moved from the autosave to the submit.The Lexical shape is decided in one place. A note or feedback value arrives either as Lexical JSON or as plain text, and the app decided which one it held in six separate places. Two of those decisions had already drifted:
isValidLexicalStateand the check insidenormalizeFeedbackToLexicaldisagreed about{"root":{"children":[]}}, so an empty-root document was wrapped on one path and passed through on the other.asLexicalEnvelopenow answers that question once, andlexicalToTextreturns the text whichever shape arrived. The three helpers inedit-and-resubmit/schema.tslost their ternaries. One of them also stops wrapping an empty Lexical document and showing the user its own JSON.hasLexicalContentandcountCharactersFromLexicalstay Lexical-only. The review suggested that the first of them read plain text as well. That breaks the four proposal rich-text fields. They have no plain-text path, so a value like "not valid json" has to read as empty and fail their required rule rather than pass as prose, andproposal/schema.test.tscatches it. The resubmission note is the one field pair where both shapes really arrive, and it now asks for both explicitly.Tests
pnpm run checkspasses. All 3070 unit tests pass, across 297 files. The new tests examine each field at its limit, at one character more than its limit, and with whitespace at the two ends. Each test file also has one test that shows that the field counts characters and not words.These tests are new in the last commit:
aria-describedbyof each field.assertDecisionFeedbackfunction, insrc/server/actions/decision-feedback.test.ts.These tests are new in the review round:
src/lib/field-limits.test.ts, which is new. It covers the whitespace rule, the two encodings of "é", the family emoji, and the CRLF case where the ASCII shortcut and the segmenter would otherwise disagree./proposalredirect for a stored title over the cap, and no redirect for one exactly at it.finalizeStudySubmissionActionon the omit path, where the title is not submitted and the stored one is over the cap. The existing test passed the title explicitly, so it exercised the path that has a visible field rather than the one that does not.lexicalToText, and the empty-root document that the two old predicates disagreed about.The end-to-end tests did not run for this PR.