Repository navigation
Conversation
Duplicate on the Build tab added the copy at the end of the list and left the editor on the original, so the date an organizer changed next moved the original instead (#557). The copy now goes through the bulk route with beforeSlotId set to the slot after the original, lands in the same place in the builder's rows, and its editor opens with focus in the first field. The help step and its walkthrough say and check where the copy goes. Fixes #557. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9MpB8R3EBVQjjrQFr3VqP
Tophat: passedI ran 74e619b locally with
Note, not a failure: in my first try I changed the copy's date and reloaded the page straight after choosing Done. The new date was lost, because no PATCH had gone out yet. Cell edits save after an 800 ms debounce, and closing the editor doesn't flush it. With a 2.5 s pause the date saved and survived the reload. This PR doesn't change the debounce, and For the owner (UX): in the builder, Duplicate now puts the copy directly under the original (inside its group, when the signup is grouped) instead of at the end of the list. It opens the copy's editor with the cursor in its first field, so the next change goes to the copy, not the original. This works from the open editor and from a closed row's hover toolbar. One help step now reads "The copy opens below it, ready for you to change its date." Compare Screenshots are in https://claude.ai/code/session_01NgZY3dzoNEst5b7t3ciuCm. Generated by Claude Code |
richshaw
left a comment
There was a problem hiding this comment.
Review: changes needed (round 1)
The PR does what #557 asks, and the approach is right. duplicateRow posts one row to the existing bulk route with beforeSlotId set to the next row, or with none when the original is last, so addSlotsBulk places and renumbers it under its existing signup and slot locks. No server code changes. INSERT_ROW_AFTER puts the copy in the same place locally. BuildWysiwyg opens the copy, and SlotEditor focuses its first field. Uncapped originals still give the copy capacity 1, and #512's typed values still go through toTypedValues. The help step, its walkthrough (now with a reload check) and CHANGELOG are updated. Each "Done when" item is covered, and the tophat confirmed it, a grouped signup on a phone included. One regression holds it back: the focus request is never cleared, so in a signup grouped by date the copy's editor pulls focus back to What when its date changes.
Blocking (1):
src/components/build-wysiwyg/SlotEditor.tsx:44-46:focusRowIdstays set while the copy is open. A date change in a date-grouped signup moves the row to anotherWysiwygGroup(keyed by the value), and the remounted editor focuses What again. Typed date digits can then land in What and autosave there. Clear the request once it has been used, and add a test.
Optional (5):
src/components/build-wysiwyg/BuildWysiwyg.tsx:235-241: if the organizer opens another slot while the copy is being saved, the copy still opens and takes focus when the response arrives. You could open it only ifexpandedRowIdis still the original.src/components/build-shared/useBuildState.ts:588-601:beforeSlotIdcomes from this tab's rows. If that slot was deleted elsewhere (another tab, or the AI connector), the bulk route refuses the copy with "beforeSlotId is not a slot in this signup", where Duplicate used to always work. That's rare. One way to handle it is to retry withoutbeforeSlotId.src/components/build-shared/useBuildState.ts:606: two quick Duplicates of one slot both send the samebeforeSlotId. The server storesA, c1, c2, B, but locally each copy goes straight after A, givingA, c2, c1, Buntil a reload.src/components/build-shared/useBuildState.ts:130: the comment says the server renumbers around an insert. It does only whenbeforeSlotIdis sent. When the last slot is copied, the server appends withnextSlotSortOrder. ClientsortOrderfor slots isn't sent anywhere, so this has no effect. Reword it if the file is touched.- From the tophat: a cell edit saves after an 800 ms debounce, and Done doesn't flush it, so a reload straight after "change the date, Done" can lose the date.
mainbehaves the same, but this PR makes that flow quicker. It's worth a follow-up if it shows up in walkthroughs.
Earlier threads: none. Copilot couldn't review because its quota is used up.
Checks: CI is green on 74e619b (lint-type-test, e2e, CodeQL). The tophat passed on 74e619b, the current head. I read the full diff against main, addSlotsBulk and the bulk route, partitionRows and the WysiwygGroup keys, and OPTIMISTIC_EDIT_CELL. I also ran a high-effort code review and checked its findings by hand. I dropped the capacity-1 fallback for uncapped copies (the issue asks to keep it) and the extra slot locks on the beforeSlotId path (an existing service path, used only on Duplicate). Checklist: no service, route, schema, policy or query change, and no new activity event. The help article and walkthrough are updated, and so is CHANGELOG.
Why owner, once fixed: pipeline:ux. Duplicate in the builder now puts the copy straight under the original (inside its group) and opens it with the cursor in its first field, from the editor and from the row toolbar. The help step says so. See the tophat's 568-base-after-duplicate, 568-head-after-duplicate and 568-head-phone-grouped-after.
https://claude.ai/code/session_01RvniWhwpuaRQz8GC7Z4UfJ
Generated by Claude Code
The copy's focus request stayed set until another row was opened. In a signup grouped by date, changing the copy's date moves it to another group, which remounts its editor, and the editor took focus again, so the next keystroke could land in the first field. SlotEditor now calls onFocused once it has focused, and BuildWysiwyg clears the request there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qs4VL324ZQL9uvmPQATuQ
Tophat: passedI ran 0f5f9a3 locally with
Note, not a failure: the last tophat's note still applies. A date change made and then left within the 800 ms save debounce is lost. Twice my scripted browser closed straight after changing a copy's date, and the change didn't save. With a pause, it saved every time. This PR doesn't change the debounce. For the owner (UX): this is the same change as before. In the builder, Duplicate puts the copy directly under the original (inside its group, when the signup is grouped) and opens the copy's editor with the cursor in its first field. The new commit fixes one rough edge: in a signup grouped by date, the cursor used to jump back to the copy's first field when its new date moved it to another group, so the next key typed changed its name. Now the cursor only goes there once. Compare Screenshots are in https://claude.ai/code/session_01YGWWmNrkYQtUFQx1y72M2r. Generated by Claude Code |
richshaw
left a comment
There was a problem hiding this comment.
Review: passed (round 2)
Since round 1 (at 74e619b), 0f5f9a3 fixes the one blocking finding. SlotEditor takes an onFocused callback and calls it right after it focuses the first field. WysiwygSlot and WysiwygGroup pass it through, and BuildWysiwyg gives it a stable clearFocusRow (useCallback) that sets focusRowId back to null. The focus request is now used once. When a date change moves the copy into another date group and its editor remounts, focusOnOpen is false and the new editor leaves focus alone. Because clearFocusRow is stable, the effect's new onFocused dependency doesn't re-run it. Under StrictMode's double effect in dev, the second run only focuses the same field again before the state clears, so nothing changes. focusRowId is still cleared by expandRow as before, and nothing else in src reads focusOnOpen or focusRowId. The new BuildWysiwyg test reproduces the round 1 scenario in a builder grouped by date: it duplicates, checks What has focus, moves the copy to Nov 21, checks the new group header appears, and checks What no longer has focus. The fix comment says it fails without the wiring. The SlotEditor tests check onFocused is called once with focusOnOpen and never without it. The rest of the diff is what round 1 reviewed.
Blocking (0): none.
Optional (0): none new. Round 1's five optional notes still stand as written: a copy that opens after the organizer has opened another slot, a stale beforeSlotId after a delete in another tab, two quick Duplicates of one slot ordering differently until a reload, the sortOrder comment in useBuildState.ts:130, and the 800 ms cell-edit debounce that Done doesn't flush. That last one is how main already behaves. Both tophats saw it, and it's worth a follow-up if walkthroughs keep hitting it.
Earlier threads: all resolved (the focus thread, answered and fixed in 0f5f9a3). Copilot couldn't review because its quota is used up.
Checks: CI is green on 0f5f9a3 (lint-type-test, e2e, CodeQL). The tophat passed on 0f5f9a3, the current head. It compared against 74e619b to show the focus jump, then showed it gone on the head, on desktop and on a phone. I read the diff since round 1 and searched src for every use of the focus props.
Why owner: pipeline:ux. In the builder, Duplicate now puts the copy directly under the original (inside its group when the signup is grouped) and opens the copy's editor with the cursor in its first field. This works from the open editor and from a closed row's hover toolbar. One help step now reads "The copy opens below it, ready for you to change its date." See the tophats' 568-base-after-duplicate, 568-head-after-duplicate, 568-head-phone-grouped-after and 568-head-grouped-copy-moved.
https://claude.ai/code/session_01CgaxVHCZfi1RL6LhTVPpdH
Generated by Claude Code
|
Approved by the owner at Merge: not mergedThe only gate that failed is a merge conflict: https://claude.ai/code/session_01ChCvABTWNwZNvyV1i2dKRP Generated by Claude Code |
Resolves conflicts with #565 and #567 in SlotEditor, WysiwygSlot, the SlotEditor tests and CHANGELOG by keeping both sides: the editor takes focusOnOpen/onFocused alongside the new taken/remindersOn props, and the focus ref sits on the field grid below the new signed-up note. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pfDqwVMpLjQtmFKGF5jBa
Address: merged
|
Tophat: passedI ran 1a30a4d locally with
Notes, not failures:
For the owner (UX): Duplicate now puts the copy straight under the original instead of at the end of the list, and opens it with the cursor in its first field, so the next date you type changes the copy. It works the same from a closed row's toolbar, and in a signup grouped by date. The help article's step changes to match. Screenshots are in https://claude.ai/code/session_014wwMCvAzJZ9dvn4SQynAUJ. Generated by Claude Code |
richshaw
left a comment
There was a problem hiding this comment.
Review: passed (round 3)
Round 2 passed this change at 0f5f9a3. Since then the only commit is 1a30a4d, a merge from main that resolved conflicts with #565 (Spots wording) and #567 (slot-change emails) in runtime code. I reviewed that resolution against main:
SlotEditor.tsxkeepsfocusOnOpen/onFocusednext tomain'staken/remindersOn. The React import isuseEffect, useId, useRef, andspotsHelpIdis unchanged.gridRefsits on the field grid, which is now below the new "signed up" note, so focus still goes to the first field's control and not to the note.WysiwygSlot.tsxpasses all four props toSlotEditor.WysiwygGroup.tsxandBuildWysiwyg.tsxare what round 2 reviewed:clearFocusRowis stable,expandRowclears the request, and the copy opens only whenduplicateRowresolves to an id.- No change from
mainwas dropped: the PR's diff againstmainin these files only adds things.
Blocking (0): none.
Optional (0): none new. Round 1's five optional notes still stand as written. The one most worth a follow-up is the 800 ms cell-edit debounce, which Done doesn't flush. All three tophats hit it, and it's how main already behaves.
Earlier threads: all resolved. Copilot couldn't review because its quota is used up.
Checks: CI is green on 1a30a4d (lint-type-test, e2e, CodeQL). The tophat passed on 1a30a4d, the current head, after the merge. It checked focus and the signed-up note together, a grouped signup on a phone included. The branch is now behind main by #525, without a conflict. The merge stage will update it.
Why owner: pipeline:ux. The owner's approval at 0f5f9a3 doesn't carry over, because the merge resolved conflicts in runtime code. The change is the same one the owner already approved. In the builder, Duplicate puts the copy straight under the original (inside its group, when the signup is grouped) and opens it with the cursor in its first field. The help step says so. See the latest tophat's 568-after-duplicate-desktop and 568-grouped-moved-desktop.
https://claude.ai/code/session_01DzTZ8jFXB8McW78y18rxFS
Generated by Claude Code
Fixes #557.
Overlaps #565, which renames the slot editor's Capacity field in
SlotEditor.tsx. Whichever lands second gets a small merge.The problem
On the Build tab, Duplicate added the copy to the end of the list (or the end of its group), often off screen. The editor stayed open on the original, so the date an organizer changed next moved the original, not the copy.
The fix
duplicateRow(src/components/build-shared/useBuildState.ts) now posts one row toPOST /api/signups/[id]/slots/bulk, withbeforeSlotIdset to the slot after the original in the rows' current order. When the original is last, it leavesbeforeSlotIdout.addSlotsBulkalready inserts there and renumbers the slots in one transaction, so no server code changes. It resolves to the copy's id, or null when nothing was added (a refused save still shows its error on the rail).INSERT_ROW_AFTERputs the server's row straight after the original and renumberssortOrderfrom 0, as the server does.BuildWysiwygopens the copy once it exists, which closes the original's editor, and asks its editor to take focus.SlotEditortakes afocusOnOpenprop (passed throughWysiwygGroupandWysiwygSlot) and focuses its first field's control when it opens. Only Duplicate sets it. Opening any row clears it, so clicking a row still doesn't move focus.toTypedValues.addRowlost itscapacityseed, which onlyduplicateRowused.HELP_SCREENSHOTS=1. The PNGs came back with pixel-level rendering noise and looked the same, so I didn't commit them.Tests
useBuildState.test.ts: theINSERT_ROW_AFTERreducer (puts the row after the named one and renumbers, or appends when that row is gone). ForduplicateRow: copying the second of four slots sendsbeforeSlotIdfor the third slot and leaves the rows asa, b, copy, c, d. Copying the last slot sends nobeforeSlotIdand appends. A refused copy resolves to null and shows the server's error. The existing body tests (typed values, empty Number left out, null capacity → 1) now point at the bulk route.SlotEditor.test.tsx:focusOnOpenfocuses the first field, and its absence leaves focus alone.pnpm lint && pnpm typecheck && pnpm knip && pnpm test(1353 tests).pnpm build && pnpm budgets: the builder page is 151.5 kB against its 152 kB budget.tests/e2e/help,organizer,slot-row-layout,enum-picker,reminder-fieldandtime-format(21 passed), then the help walkthrough again after the reload check (3 passed).pnpm test:db: no service, route or schema changed.Generated by Claude Code