Skip to content

Put a refused cell edit back in the builder, and stop text values at the field's length - #544

Merged
richshaw merged 3 commits into
mainfrom
claude/issue-514-refused-cell-rollback
Oct 23, 2026
Merged

richshaw merged 3 commits into
mainfrom
claude/issue-514-refused-cell-rollback

Conversation

@richshaw

Copy link
Copy Markdown
Owner

Fixes #514.

#542 (open) also touches saveRowValues. It moves the number conversion out into toTypedValues; this PR adds one line after the PATCH. Whichever lands second has a small merge to make.

The problem

On the Build tab, when the server refused a value typed into a slot (most often a text value pasted past the field's 200-character limit), the builder kept showing it. saveRowValues sends every value in the row, so every later edit to that slot was refused too. A capacity change sends only { capacity }, so it saved and the rail switched to Saved, hiding the error. After a reload, the bad value and every later edit to that slot were gone.

The fix

  • useBuildState keeps each row's values as the server last accepted them (savedValues). The first load, a new slot, the slot refetch after a refused move, and every successful saveRowValues keep it current.
  • When an editCell save is refused, rollBackRefused puts that row's changed cells back to their saved values, then the error shows as before. A cell the organizer edited again while the refused save was in flight keeps the newer value: it's skipped when its value no longer matches what was sent, or when it has a save still waiting in the debounce. renameGroup already rolls back its own rows and is unchanged.
  • The slot editor's Short text input and a Short text day heading (Editable gains a maxLength prop) now stop at the field's config.maxLength, so the usual trigger can't be typed or pasted.
  • One choice the issue didn't make: the rollback reverts every cell of the refused request that differs from what was saved and has no newer edit, not only the cell whose timer fired. The server refuses the whole PATCH, so none of those values were saved. A cell with its own save still waiting keeps its value and saves on its own.
  • CHANGELOG entry under Fixed.

Tests

  • useBuildState.test.ts, new editCell when the server refuses a value block, against a fetch mock that refuses values over 200 characters: a refused edit puts the old value back, and the rail stays on the error. A later edit to another field in the row sends the saved value plus the new one, saves, and shows Saved. A rollback goes to the last value that saved, not the page's first load. An edit typed in the same cell while the refused save was in flight stays and saves. All four failed before the fix.
  • SlotEditor.test.tsx and WysiwygGroup.test.tsx: a Short text input and a Short text heading carry the field's maxLength. A date input has none.
  • Ran in this session: pnpm lint && pnpm typecheck && pnpm knip && pnpm test (1293 passed), pnpm build && pnpm budgets (the build page is at 150.9 kB of 151 kB), and the e2e specs organizer, enum-picker, slot-row-layout and reminder-field with the container config (14 passed). No service, schema or route changed, so pnpm test:db doesn't apply. affected-articles.mjs finds no help article touched.

Generated by Claude Code

…he field's length

A value the server refused stayed in the Build tab's row, so every later
save of that slot sent it again and was refused too, and a capacity
change then showed "Saved" over the error. editCell now puts the row's
refused cells back to their last saved values, keeping any cell edited
again since. The slot editor's text input and a Short text day heading
now stop at the field's maxLength.

Fixes #514.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R2W8jx8iaEzeQ6dUmiNqGD
@richshaw richshaw added pipeline:ux Changes what a person using OpenSignup sees or does pipeline:working A session is working on it right now labels Oct 21, 2026 — with Claude
@richshaw
richshaw requested a balanced review from Copilot October 21, 2026 14:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@richshaw richshaw added pipeline:tophat pipeline:working A session is working on it right now and removed pipeline:working A session is working on it right now labels Oct 21, 2026 — with Claude

Copy link
Copy Markdown
Owner Author

Tophat: passed

I ran 8501d5a locally with pnpm dev, Postgres 16, and Chromium at 1280×900 and 390×844. I didn't run the worker, because nothing here touches jobs or email. I used a new draft signup, "Snack duty — Spring season", with the default What (Short text, max 200) and Date fields. On the head the input stops at 200 characters, so a refusal can't be typed any more. To reach the rollback anyway, I removed the input's maxlength in the page before filling, which stands in for any value the server refuses.

Check Result What I saw
The bug on the base (8600bed) ✅ reproduced I filled What with 250 characters. The rail showed "what" exceeds maxLength 200 and the input kept all 250 (544-base-refused). A later Date edit in the same slot was refused too (544-base-date-refused). A capacity change then saved and the rail switched to Saved (544-base-after-capacity). After a reload the slot read "Set what" with no date: the What value and the date were both lost.
Short text stops at the field's length ✅ On the head, filling 250 characters left 200 in the input (maxLength=200), and it saved (544-head-maxlength).
A refused value is put back ✅ What was saved as "Fruit and water". I forced a 250-character value, and the server refused it. The input went back to "Fruit and water" and the rail kept the error (544-head-refused-rolled-back).
A later edit in that row saves ✅ Setting Date to 2030-04-06 then showed Saved (544-head-date-saved). After leaving the page and coming back, the slot read "Fruit and water · Sat, Apr 6" (544-head-after-reload). The API had {what: "Fruit and water", date: "2030-04-06"}.
Day heading (grouped by What) ✅ With the signup grouped by What (set through the API), the heading's edit input had maxLength=200. Checked on the phone (544-phone-heading-editing).
Phone ✅ At 390 px the same refused value rolled back to "Fruit and water" with the error, and a capacity change after it saved (544-phone-refused-rolled-back). There's no sideways scroll (scroll width 390). The server then had capacity 4 and the old What value.
Neighbours ✅ Add a slot, filling its What ("Napkins") and Duplicate all saved. The API listed both "Napkins" rows (544-head-duplicate).
Console and server logs ✅ The only console entry was the expected 400 from the refused PATCH. The only server errors were the health check's pgboss.job query, because the worker wasn't running.

I didn't test editing a cell again while the refused save was still in flight. That needs a slow server response to time it, and useBuildState.test.ts covers it.

For the owner (UX): in the Build tab's slot editor, and in a day heading for a Short text field, typing or pasting now stops at the field's length (200 by default), so the "exceeds maxLength" error can't be reached by typing. If the server still refuses a value, the cell goes back to its last saved value and the error stays in the rail, instead of the bad value staying on screen and quietly blocking every later save of that slot. Compare 544-base-refused and 544-base-after-capacity with 544-head-refused-rolled-back and 544-head-date-saved.

Screenshots are in https://claude.ai/code/session_01QsKEfKk4ZKF46XNY88GWS4.


Generated by Claude Code

@richshaw richshaw added pipeline:review pipeline:working A session is working on it right now and removed pipeline:tophat pipeline:working A session is working on it right now labels Oct 21, 2026 — with Claude
Keeps both Fixed entries in CHANGELOG.md (#514 and #240).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012NV65LfVJWkYDatbG9CV8D
@richshaw richshaw added pipeline:working A session is working on it right now and removed pipeline:working A session is working on it right now labels Oct 21, 2026 — with Claude

@richshaw richshaw left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: passed (round 1)

The PR does what #514 asks. useBuildState keeps each row's last server-accepted values in savedValues. Every path that learns what the server holds keeps it current: the first load, addRow (and so Duplicate), the refetch after a refused move, and saveRowValues on success, which editCell, moveRow and renameGroup all go through. When an editCell save is refused, rollBackRefused puts back only the cells of the refused request that differ from what was saved, skipping any cell the organizer has changed since (its value no longer matches what was sent) or that has a save waiting in the debounce, which covers the issue's "Watch for" item. renameGroup keeps its own rollback and now also updates savedValues through saveRowValues. setCapacity still sends only { capacity }, so after a rollback its "Saved" is true, because the row has nothing unsaved left. The slot editor's Short text input and a Short text day heading now take config.maxLength. Browser maxlength and zod's .max both count UTF-16 code units, so the limit the input enforces is the one the server checks. Each "Done when" item is done, and the four new hook tests fail without the fix, as the description says.

Blocking (0): none.
Optional (1):

  • src/components/build-shared/useBuildState.ts:626: savedValues takes the snapshot of whichever PATCH of a row answers last, not the one the server applied last. If two saves of the same row are in flight together and answer out of order, savedValues can hold an older snapshot, and a later refusal could roll a cell back to it. It needs two concurrent PATCHes of one row answering out of order and then a third that's refused, and the server's own order is just as uncertain, so it isn't worth a sequence counter here.

Earlier threads: none. Copilot couldn't review because its quota is used up.
Checks: CI is green on 1e88e8d (lint-type-test, e2e, CodeQL). The tophat passed on 8501d5a, and the only commit since is a merge of main, with no runtime change of the PR's own (the shared check prints 0). I read the full diff, the rest of useBuildState.ts (every caller of saveRowValues, renameGroup, setCapacity, the move and field paths) and the new tests. Nothing on the checklist applies: no service, route, schema, policy guard, help article or public page changed, and there's a CHANGELOG entry. The build page sits at 150.9 kB of its 151 kB budget, as the description says, and CI's budget check passes.

Why owner: pipeline:ux. In the Build tab, a Short text value in the slot editor or a Short text day heading now stops at the field's length (200 by default). If the server still refuses a value, the cell goes back to its last saved value and the error stays on the save rail. Compare the tophat's 544-base-refused and 544-base-after-capacity with 544-head-refused-rolled-back and 544-head-date-saved. Whichever of this PR and #542 merges second will need a small merge in saveRowValues.

https://claude.ai/code/session_01KSZnxXQzU2MnAp4VcCMySi


Generated by Claude Code

@richshaw richshaw added pipeline:owner Passed; waits for the owner (UX or high-risk) and removed pipeline:review pipeline:working A session is working on it right now labels Oct 21, 2026 — with Claude
@richshaw richshaw added pipeline:merge and removed pipeline:owner Passed; waits for the owner (UX or high-risk) labels Oct 22, 2026
@richshaw richshaw added the pipeline:working A session is working on it right now label Oct 22, 2026 — with Claude

Copy link
Copy Markdown
Owner Author

Approved by the owner at 1e88e8d.

https://claude.ai/code/session_01Ga7vyEoy6hrHo2caTyqqAk


Generated by Claude Code

@richshaw richshaw added pipeline:fix and removed pipeline:merge pipeline:working A session is working on it right now labels Oct 22, 2026 — with Claude

Copy link
Copy Markdown
Owner Author

Not merged: the PR conflicts with main (mergeable_state: dirty) in CHANGELOG.md and src/components/build-shared/useBuildState.ts (the saveRowValues merge the description expected with #542). Every other gate passed on 1e88e8d: CI green, review pass, tophat pass with no runtime change since, no open threads, owner approval recorded above. Moved to pipeline:fix for the merge from main. A conflict resolved in useBuildState.ts is a runtime change, so it goes back through tophat and the owner's approval won't carry over.

https://claude.ai/code/session_01Ga7vyEoy6hrHo2caTyqqAk


Generated by Claude Code

@richshaw richshaw added the pipeline:working A session is working on it right now label Oct 22, 2026 — with Claude
Resolves the saveRowValues conflict with #542: the PATCH now builds its
body with main's toTypedValues, then records savedValues as before.
CHANGELOG keeps both sides' Fixed entries.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JHfJWDavp9zqKK5NLqRgk3

Copy link
Copy Markdown
Owner Author

Merged main in at 1824450 to clear the conflict.

  • src/components/build-shared/useBuildState.ts: saveRowValues now builds its PATCH body with main's toTypedValues (from Send Number values as numbers when the Build tab adds a slot #542), replacing this branch's inline number conversion, and still records savedValues after a successful save. addRow takes both sides (typed values on POST, savedValues set from the response). rollBackRefused is unchanged.
  • CHANGELOG.md: kept both sides' Fixed entries.

Ran on the merge: pnpm lint && pnpm typecheck && pnpm knip && pnpm test (1333 passed) and pnpm build && pnpm budgets (the build page is at 151.3 kB of 152 kB). This conflict was in runtime code, so once CI is green it goes back to tophat, and the owner's approval at 1e88e8d doesn't carry over.

https://claude.ai/code/session_01JHfJWDavp9zqKK5NLqRgk3


Generated by Claude Code

@richshaw richshaw added pipeline:tophat pipeline:working A session is working on it right now and removed pipeline:fix pipeline:working A session is working on it right now labels Oct 22, 2026 — with Claude

Copy link
Copy Markdown
Owner Author

Tophat: passed

Ran 1824450 locally with pnpm dev, Postgres 16, Chromium at 1280×900 and 390×844. No worker, since nothing here touches jobs or email. This re-tophat covers the merge from main that moved saveRowValues and addRow onto toTypedValues (#542). I used a new draft signup, "Snack duty — Spring season", with the default What (Short text, max 200) and Date fields, then added a Number field, Servings, through the API. As in the first tophat, I removed the What input's maxlength in the page to force a value the server refuses.

Check Result What I saw
Short text stops at the field's length ✅ The What input has maxLength=200.
A refused value is put back ✅ What was saved as "Fruit and water". A forced 250-character value was refused, the input went back to "Fruit and water" and the rail kept "what" exceeds maxLength 200 (544-head-refused-rolled-back).
A later edit in that row saves ✅ Date 2030-04-06 then showed Saved (544-head-date-saved). The API had {what: "Fruit and water", date: "2030-04-06"}.
Number values after the merge ✅ Servings = 12 saved (544-head-number-saved), and the API stored servings: 12 as a number.
Duplicate (the addRow side of the merge) ✅ Duplicate saved a second slot with servings: 12 as a number and the same What and Date.
Phone, refusal in a row with a Number value ✅ At 390 px a refused What value rolled back with the error showing (544-phone-refused-rolled-back). Servings = 20 then saved (544-phone-number-saved), and the API had servings: 20 with What unchanged. No sideways scroll (scroll width 390).
Console and server logs ✅ The only console entries were the expected 400s from the refused PATCHes. The only server error was the health check's pgboss.job query, because the worker wasn't running.

I didn't re-check the Short text day heading's maxLength or an edit made while a refused save was in flight. Neither changed in the merge, the first tophat checked the heading, and useBuildState.test.ts and WysiwygGroup.test.tsx cover both.

For the owner (UX): same as at 8501d5a. On the Build tab, typing or pasting a Short text value now stops at the field's length (200 by default). If the server still refuses a value, the cell goes back to its last saved value and the error stays in the rail, so the bad value no longer sits on screen blocking every later save of that slot. Compare 544-head-refused-rolled-back with 544-head-date-saved. Your approval at 1e88e8d doesn't carry over, because the merge changed useBuildState.ts.

Screenshots are in https://claude.ai/code/session_0167febYbbeYTfJ9iZrawUkN.


Generated by Claude Code

@richshaw richshaw added pipeline:review pipeline:working A session is working on it right now and removed pipeline:tophat pipeline:working A session is working on it right now labels Oct 23, 2026 — with Claude

@richshaw richshaw left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: passed (round 2)

This round covers the merge from main at 1824450. The rest of the PR is the same as at 1e88e8d, which passed round 1 and which the owner approved. The merge puts saveRowValues and addRow on main's toTypedValues (#542). Both still record savedValues only after the server accepts the request. savedValues keeps the grid's string form (toStringValues / toBuildRow), so rollBackRefused compares strings with strings, as it did before. A Number field's value goes to the server as a number, but the snapshot and the rollback stay in the grid's own strings, so a refused row still goes back to what was on screen when it last saved. rollBackRefused, the maxLength props and the tests are unchanged since round 1.

Blocking (0): none.
Optional (1):

  • src/components/build-shared/useBuildState.ts:638: as in round 1, savedValues keeps the snapshot from whichever PATCH of a row answers last. That only matters when two PATCHes of one row answer out of order and a third is then refused. Not worth a sequence counter.

Earlier threads: none. Copilot couldn't review because its quota was used up.
Checks: CI is green on 1824450 (lint-type-test, e2e, CodeQL). The tophat passed on this exact SHA, covering the merge: a refused value is put back, Number values save as numbers, Duplicate works, and the phone width was checked. I read the merged useBuildState.ts diff, toTypedValues / toStringValues / toBuildRow, and the field paths. No service, route, schema, policy guard, help article or public page changed, and there's a CHANGELOG entry under Fixed.

Why owner: pipeline:ux, and the approval at 1e88e8d doesn't carry over, because the merge resolved a conflict in useBuildState.ts. What people will see is the same as what you approved. On the Build tab, a Short text value in the slot editor or a Short text day heading stops at the field's length. If the server still refuses a value, the cell goes back to its last saved value and the error stays on the save rail. To compare, look at the latest tophat's 544-head-refused-rolled-back and 544-head-number-saved.

https://claude.ai/code/session_01SnCqCmJK8kEsGLAPmGZthC


Generated by Claude Code

@richshaw richshaw added pipeline:owner Passed; waits for the owner (UX or high-risk) and removed pipeline:review pipeline:working A session is working on it right now labels Oct 23, 2026 — with Claude
@richshaw
richshaw merged commit 1aeca55 into main Oct 23, 2026
5 checks passed
@richshaw
richshaw deleted the claude/issue-514-refused-cell-rollback branch October 23, 2026 00:45
@richshaw richshaw removed pipeline:ux Changes what a person using OpenSignup sees or does pipeline:owner Passed; waits for the owner (UX or high-risk) labels Oct 23, 2026 — with Claude
richshaw pushed a commit that referenced this pull request Oct 23, 2026
The merge from main brought in #544, which put the builder page at
151.1 kB, over the 151 kB this branch had lowered it to. Main measures
151.2 kB under 152, and this branch comes in a little under main, so it
keeps main's number rather than one the next merge can tip over.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FGRcD1xKZ6wL67VKSCbbVC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A refused cell edit in the builder stays on screen and blocks the rest of the row, and a later save says "Saved"

3 participants