[richshaw]

Fixes #516.

The problem

createSignup and updateSignup checked settings.reminderFromFieldRef against the signup's fields, but never checked settings.groupByFieldRefs. So POST /api/signups, PATCH /api/signups/[id] and the connector's update_signup all saved a group-by ref that named no field, and reported success. The public page then showed the slots ungrouped without saying why. If a field with that ref was added later, grouping switched on even though nobody asked for it.

The fix

• src/services/signups.ts: a new checkGroupByRefs refuses any ref that isn't one of the signup's field refs. The error is invalid_input on settings.groupByFieldRefs, with the bad ref as received, the same shape as the reminder-ref error.
• createSignup checks the refs against the template's fields.
• updateSignup checks only when the caller sends a non-null groupByFieldRefs. A row that already holds a stale ref still saves its other settings. Sending null clears the key as before.
• I did not also refuse number and time fields, though the issue suggested it. Magic Compose (magicComposeToTemplate) and create_signup accept a group-by on any declared field. Refusing number and time in the service would make those drafts fail outright. That is a separate decision with its own UX, so I left it out of this PR.
• CHANGELOG entry under Fixed.

Tests

• src/services/signups.db.test.ts adds three tests:
  • create with ['ghost'] is refused
  • update with ['nope'] is refused, and the stored settings are left unchanged
  • an unrelated settings save on a row that holds a stale ref still succeeds
• src/mcp/tools.db.test.ts: update_signup with an unknown ref returns invalid_input, and the stored group-by stays as it was.
• The two refusal tests and the MCP case failed before the fix and pass after it.
• I ran these and all passed: pnpm lint && pnpm typecheck && pnpm knip && pnpm test (1295 tests) and pnpm test:db (495 tests).

---

Generated by Claude Code

[Copilot]

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

[richshaw]

Tophat: passed

I ran c928dfa 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. Before checking the head, I reproduced the bug on the base (1d3a73c). The test signup was "Snack duty — Spring season", with What and Date fields and two slots: "Fruit and water" on Apr 6 and "Orange slices" on Apr 13.

Check	Result	What I saw
Update with an unknown ref (PATCH /api/signups/[id])	✅	Base: {"settings":{"groupByFieldRefs":["nope"]}} returned 200 and stored ["nope"]. The public page showed the two slots ungrouped and gave no reason (546-base-public-nope-desktop). Head: ["nope2"] returns 400 invalid_input, field: settings.groupByFieldRefs, received: "nope2". Stored settings are unchanged. A wrong-case ["Date"] is refused in the same way, and the earlier ["date"] is kept.
Create with an unknown ref (POST /api/signups)	✅	Base: ["ghost"] returned 200 and created the signup. Head: it returns 400 with the same error shape, and no signup row is created. A create with ["what"] and a create with no group-by both still succeed.
A row that already holds a stale ref	✅	The base left ["nope"] on the signup. On the head, a settings save of sendReminders: false and a title change each returned 200. The stale ["nope"] stayed as it was, so a save that doesn't touch group-by isn't blocked.
Clearing and valid values	✅	["date"] → 200. null clears the setting to [] (200). [] → 200.
Group by a field just added, then delete it	✅	["team"] is refused while no Team field exists. After POST …/fields adds a Team field, the same request returns 200. Deleting the field still removes it from groupByFieldRefs, leaving [].
Builder: Fields → Group slots by (desktop)	✅	Changing Date → Don't group → Date showed Saved each time, and the server ended up holding ["date"] (546-head-builder-ungrouped-desktop, 546-head-builder-regrouped-desktop). No console errors.
Public page, grouped (desktop and phone)	✅	The slots appear under the "SAT, APR 6" and "SAT, APR 13" headings (546-head-public-grouped-desktop, 546-head-public-grouped-phone).
MCP update_signup and the service tests	✅	pnpm test:db src/mcp/tools.db.test.ts src/services/signups.db.test.ts ran 83 tests and all passed. This includes the new update_signup unknown-ref case and the three service cases.
Console and server logs	✅	No browser console errors. The only server warnings were the health check's pgboss query, because the worker wasn't running, and Node's dev-only MaxListenersExceededWarning.

UX: this changes what API callers and AI assistants see, and nothing changes on screen. When update_signup, PATCH /api/signups/[id] or POST /api/signups sends a group-by that names no field, it now gets a 400 with "groupByFieldRefs must name one of the signup's fields". Before, it got a success, and the public page silently didn't group. The builder only offers real fields, so organizers using the web app see no difference. 546-base-public-nope-desktop shows the ungrouped page that the old success led to.

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

---

Generated by Claude Code

[richshaw]

Review: passed (round 1)

This PR adds checkGroupByRefs to src/services/signups.ts. createSignup uses it to check the template's fields, and updateSignup uses it to check the signup's stored fields, but only when the caller sends a non-null groupByFieldRefs. An unknown ref is refused with invalid_input on settings.groupByFieldRefs, the same shape the reminder-ref error has. It does what #516 asks.

• The check runs inside withSignupWriteLock, against listFieldsForSignup(tx, …), so a field added or deleted at the same moment can't slip past it.
• sentGroupBy reads the caller's raw settings, not the merged ones. A stale ref already on the row therefore doesn't block an unrelated save, and a test covers that case.
• The other writers already keep refs valid. magicComposeToTemplate keeps only declared refs, so Magic Compose and create_signup can't trip the new check. deleteField prunes the deleted ref. Before a "Group slots by" save, the Build tab fetches any field it hasn't loaded (#398).
• If another tab deleted the field just before a Build tab save, the save is now refused rather than storing a dead ref. That is the behaviour we want.

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

• The issue's third outline item, refusing a group-by on number and time fields, is left out on purpose, and the description explains why. Magic Compose and create_signup accept those today, so this is the owner's call. If you want it, file it as a proposed issue. This PR doesn't need it.

Earlier threads: none. Copilot couldn't review because its quota is used up.
Checks:

• CI is green on c928dfa: lint-type-test, e2e and CodeQL.
• I read the diff and its callers by hand. I didn't need to run anything locally to prove a finding.
• The tophat passed on this same SHA (<!-- pipeline-tophat sha=c928dfa… result=pass -->).

Why this waits for the owner: it's UX (pipeline:ux). API callers and AI assistants that send an unknown group-by ref now get a 400 with "groupByFieldRefs must name one of the signup's fields" where they used to get a success. Nothing changes on screen. See the tophat's 546-base-public-nope-desktop for the silently ungrouped page the old success led to.

https://claude.ai/code/session_01HejG9ZMkJpVdq1xjBbMDwb

---

Generated by Claude Code

[richshaw]

Yep we should be able to group on number or time fields.

[richshaw]

Approved by the owner at c928dfa.

The merge stage couldn't merge it: after #545 landed, the PR conflicts with main. It goes to pipeline:fix for a merge of main. The approval carries over as long as that merge is the only new commit.

The owner's comment (number and time fields should stay groupable) agrees with what this PR does, so nothing else changes.

https://claude.ai/code/session_01DXydxGNyVt8sXcDp4XzG84

---

Generated by Claude Code

[richshaw]

Merged main into the branch in 529cc23. The only conflict was in CHANGELOG.md, where #545 and this PR each added a line under Fixed. I kept both, with main's first. No runtime code changed.

• Locally, all of these passed: pnpm lint && pnpm typecheck && pnpm knip && pnpm test (1305 tests) and pnpm test:db (500 tests).
• CI is green on 529cc23.
• The PR has no commits of its own since c928dfa, so the owner's approval of c928dfa carries over. It goes straight to pipeline:merge.

https://claude.ai/code/session_013fWkt1ztQmhjajGwXkdGxu

---

Generated by Claude Code