Skip to content

Fetch fields when a settings save names one the Build tab hasn't loaded - #508

Merged
richshaw merged 3 commits into
mainfrom
claude/issue-398-settings-save-fields
Oct 21, 2026
Merged

richshaw merged 3 commits into
mainfrom
claude/issue-398-settings-save-fields

Conversation

@richshaw

Copy link
Copy Markdown
Owner

Fixes #398.

The problem

Since #360, a settings save on the Build tab shows the settings the server returns. If another tab or an AI assistant has added a date field and moved reminders to it, the next "Group slots by" change adopts that reminder field, but the tab's field list doesn't have it. Until a reload, no field shows the reminder bell, and every date field's reminder box is disabled behind a note with a blank where the field's name should be.

The fix

  • saveSettings in src/components/build-shared/useBuildState.ts now calls a new catchUpOnFields before showSettings. When the saved settings name a field ref (the reminder field or a group-by field) that the tab doesn't have, it fetches GET /api/signups/[id]/fields and shows that list, the same refetch moveField uses.
  • The fetch runs inside the save already queued in settingsSaveRef, so two saves in a row still show their answers in click order.
  • Column widths are kept by field id outside BuildField, so replacing the list keeps them, as the reorder refetch does.
  • A decision the issue didn't make: if the field fetch fails, the save still reports "Saved" and the list stays as it was. The PATCH did succeed, and an error on the rail would read as if the setting hadn't saved.
  • I took the full fix, not the smaller one, so InlineFieldForm is unchanged.

Tests

useBuildState.test.ts has three new hook tests:

  • A "Group slots by" save whose response names a reminder field the tab hadn't loaded leaves that field in the list, with the reminder on it. This test fails on main.
  • No field fetch when every field the settings name is already loaded.
  • A failed field fetch still leaves the save shown as saved.

I ran pnpm lint && pnpm typecheck && pnpm knip && pnpm test (1252 passed), plus pnpm build && pnpm budgets. /app/signups/[id]/build is 150.2 kB against its 151 kB budget. I didn't run pnpm test:db, because no service, DB, MCP or OAuth code changed.


Generated by Claude Code

A "Group slots by" save shows the settings the server merges, which can
name a reminder field another tab or an AI assistant added after this tab
loaded. The tab then had no field to put the reminder bell on, and every
date field's reminder box was blocked by a field it couldn't name. Now the
save fetches the field list first when that happens, inside the same
settings queue so a later save's answer still lands last. A failed fetch
leaves the list as it was, since the save itself succeeded.

Fixes #398.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N4Xf8UF6XA8WQ5rAxo5cYy
@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 20, 2026 — with Claude
@richshaw
richshaw requested a balanced review from Copilot October 20, 2026 06:01

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 20, 2026 — with Claude

Copy link
Copy Markdown
Owner Author

Tophat: passed

I ran c3f2f2c 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. To play "another tab or an AI assistant", I left the Build tab open and, from inside the page with fetch, POSTed a new date field "Pickup day" to /api/signups/[id]/fields, then PATCHed settings.reminderFromFieldRef to pickup-day. The tab's React state never saw either request.

Check Result What I saw
The bug on the base (59a10ee) ✅ reproduced After the outside changes, I set Fields → Group slots by → What. It showed "Saved", but the list still read Fields · 2, and no field had the reminder bell (508-before-after-groupby). Opening Date showed the reminder box disabled, with the note " is already the reminder field for this signup." The field's name was blank (508-before-date-field-edit).
The fix on the PR ✅ Same steps. After the save, the list read Fields · 3, with Pickup day last and the bell on it, and no other field had the bell (508-after-after-groupby). Opening Date showed the note "Pickup day is already the reminder field for this signup." (508-after-date-field-edit).
No outside change: no extra fetch ✅ On a fresh signup I counted the page's GET …/fields calls during a Group slots by → Date save. There were 0, so the catch-up only runs when a setting names a field the tab doesn't have.
Field fetch fails ✅ I made the page's GET …/fields reject, then did the same outside changes and group-by save. The rail showed "Saved" with no error. The server has groupByFieldRefs: ["what"], and the list stayed at 2 fields, which is the fallback the description gives (508-after-fetch-fails).
Phone width, and using the field afterwards ✅ At 390 px, Pickup day appears with the bell after the group-by save, with no horizontal scroll (508-after-groupby-phone). Opening Pickup day shows its reminder box enabled and ticked. Unticking it and pressing Save stored sendReminders: false, and the bell went away. Unticking Date's box on a fresh signup on this branch does the same, so that's the existing behaviour.
Console, logs, unit tests ✅ No browser console errors in any run. The only server errors were the health check's pgboss query, because the worker wasn't running. pnpm test src/components/build-shared: 3 files, 88 passed.

For the owner (UX): suppose another tab or an AI assistant added a date field and moved reminders to it. On the Build tab, the next "Group slots by" change now brings that field into the Fields list with the reminder bell. The other date fields' reminder note names it. On main, the field didn't appear, no field showed the bell, and the note had a blank where the name should be, until a reload. See 508-before-after-groupby and 508-before-date-field-edit for main, and 508-after-after-groupby, 508-after-date-field-edit and 508-after-groupby-phone for the PR.

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


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 20, 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 #398 asks, and takes the full fix. saveSettings now runs catchUpOnFields(next) before showSettings(next). When the saved settings name a group-by or reminder ref the tab hasn't loaded, it fetches GET /api/signups/[id]/fields and shows that list. The fetch runs inside the withSave callback that is already chained on settingsSaveRef, so the next queued save can't start until this one has fetched and shown its fields. Two saves in a row still land in click order, which the issue's "Watch for" asked about. stateRef.current.fields is read after the PATCH returns, so the check sees any field this tab added while the save was in flight. If the fetch fails, the PATCH has still succeeded. The list stays as it was and the rail says "Saved", as the description says. showSettings then applies the server's settings, which are authoritative, so the tab doesn't need mirrorFieldChange here. I checked each "Done when" item. The hook test for a save that names an unloaded reminder field is there: I put main's useBuildState.ts under the new tests, and that test fails there and passes on the PR. The InlineFieldForm item applies only to the smaller fix, which wasn't taken. CI is green.

Blocking (0)

Optional (3):

  • src/components/build-shared/useBuildState.ts:770: the issue's outline said to share the reorder path's refetch. toBuildFields(await sendForData<SlotFieldDefinition[]>('GET', fieldsUrl)) now appears twice, here and in moveField (line 533). A const fetchFields = async () => … beside showFields would make it one. It's a single expression, not a copied helper, so it doesn't need a round.
  • src/components/build-shared/useBuildState.ts:770: SET_FIELDS replaces the whole list. If a field add or edit from this same tab finishes between the GET leaving and its answer arriving, that change is overwritten until the next refetch or reload. moveField's refetch has the same window, and this one only runs when a setting names a field the tab lacks. Not worth code.
  • The description's "Column widths are kept by field id outside BuildField" bullet describes nothing that exists today. BuildField has no width, and nothing in build-shared or the builders stores per-field column widths. Replacing the list can't lose anything, so the issue's "Keep column widths" note is met trivially.

Earlier threads: none. Copilot couldn't review because its quota is used up.
Checks: CI is green on c3f2f2c (lint-type-test, e2e, CodeQL). The tophat passed on this exact SHA, so no runtime code has changed since it ran. Locally I ran the three new tests on the PR (passed), and the same tests against main's hook (the first fails). I read the full diff, saveSettings, showSettings, mirrorFieldChange, moveField's saveMove refetch and the SET_FIELDS reducer, and went through the checklist. No service, policy, schema, public route or help article is touched, and there's a CHANGELOG entry.

Why the owner: pipeline:ux. Suppose another tab or an AI assistant has added a date field and moved reminders to it. The Build tab's next "Group slots by" change now brings that field into the Fields list with the reminder bell, and the other date fields' note names it. Before, it showed a blank until a reload. See the tophat's 508-before-after-groupby and 508-before-date-field-edit for main, and 508-after-after-groupby, 508-after-date-field-edit and 508-after-groupby-phone for the PR.

https://claude.ai/code/session_01PVfzKGxMqq8ZtxPNd3fahF


Generated by Claude Code

@richshaw

Copy link
Copy Markdown
Owner Author

Resolve conflicts please

@richshaw richshaw added pipeline:fix and removed pipeline:owner Passed; waits for the owner (UX or high-risk) labels Oct 20, 2026
@richshaw richshaw added the pipeline:working A session is working on it right now label Oct 20, 2026 — with Claude
Keeps both sides of the CHANGELOG "Fixed" entries and of the new test
blocks at the end of useBuildState.test.ts (#398's settings-save tests
and main's renameGroup tests). No runtime code conflicted.

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

Copy link
Copy Markdown
Owner Author

Conflicts resolved in 496a842 (a merge of main, no rebase).

Locally: pnpm lint && pnpm typecheck && pnpm knip && pnpm test all pass (111 files, 1274 tests). Once CI is green this goes back to pipeline:owner.

https://claude.ai/code/session_01BbFpSwrE1SnxwR1UPfruWu


Generated by Claude Code

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

Copy link
Copy Markdown
Owner Author

Approved by the owner at 496a842.

Not merged: one gate failed.

  • Conflict. main has moved on since 496a842 and the PR conflicts with it again, in CHANGELOG.md only (a local git merge-tree against main at e07ec2e shows no other file). Sending it to pipeline:fix.

Every other gate passed on 496a842: CI green (lint-type-test, e2e, CodeQL), review verdict=pass and tophat result=pass on c3f2f2c with no commits of the PR's own since, no open review threads, main green. A merge of main that only touches the CHANGELOG keeps this approval, so it can come straight back to pipeline:merge.

https://claude.ai/code/session_01VzkajMzBsnuJhN9LFqAVbP


Generated by Claude Code

@richshaw richshaw added pipeline:fix pipeline:working A session is working on it right now and removed pipeline:merge labels Oct 21, 2026 — with Claude
Resolves a CHANGELOG conflict: keeps main's new Changed entries and
this PR's #398 Fixed entry.

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

Copy link
Copy Markdown
Owner Author

Conflict resolved in 7257f13 (a merge of main, no rebase).

Locally, pnpm lint && pnpm typecheck && pnpm knip && pnpm test all pass (113 files, 1287 tests). Once CI is green on 7257f13, this goes back to pipeline:merge.

https://claude.ai/code/session_01VzkajMzBsnuJhN9LFqAVbP


Generated by Claude Code

@richshaw richshaw added pipeline:merge 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 21, 2026 — with Claude
@richshaw
richshaw merged commit 585c451 into main Oct 21, 2026
5 checks passed
@richshaw
richshaw deleted the claude/issue-398-settings-save-fields branch October 21, 2026 02:54
@richshaw richshaw removed pipeline:merge 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
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.

After a settings save, the Build tab can point reminders at a field it hasn't loaded

3 participants