Skip to content

Commit 4b6a321

Browse files
gpascuccigpascucci
committed
fix(schedule): address code review feedback for mutation guards
1 parent 3bbd4aa commit 4b6a321

12 files changed

Lines changed: 480 additions & 42 deletions

File tree

‎frontend/src/components/core/ScheduleActions/index.tsx‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ type ScheduleActionsProps = {
66
className: string
77
editable: boolean
88
saving: boolean
9-
checking: boolean
109
onSave: () => void
1110
onCheckStatus: () => void
1211
onDelete: () => void
@@ -28,7 +27,6 @@ const ScheduleActions: FC<ScheduleActionsProps> = ({
2827
className,
2928
editable,
3029
saving,
31-
checking,
3230
onSave,
3331
onCheckStatus,
3432
onDelete,
@@ -41,7 +39,7 @@ const ScheduleActions: FC<ScheduleActionsProps> = ({
4139
<Button
4240
kind="tertiary"
4341
size="md"
44-
disabled={!editable || saving || checking}
42+
disabled={!editable || saving}
4543
onClick={onCheckStatus}
4644
>
4745
Check Status

‎frontend/src/components/schedule1/__tests__/Schedule1.test.tsx‎

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -33,16 +33,6 @@ const StaleRaceHarness = () => {
3333
)
3434
}
3535

36-
// Deterministically drain the event loop so a settled-but-guarded request finishes its whole
37-
// then/catch/finally chain before a negative assertion runs (turn-based, never wall-clock).
38-
const flushAsync = async () => {
39-
for (let i = 0; i < 4; i += 1) {
40-
await new Promise((resolve) => {
41-
setTimeout(resolve, 0)
42-
})
43-
}
44-
}
45-
4636
const URL = 'http://localhost:3000/api/v1/schedule1'
4737

4838
const schedule1Doc = {
@@ -737,8 +727,9 @@ describe('Schedule1 stale-response guard (Story 29.6)', () => {
737727

738728
// Release the stale PUT, let its chain settle, then confirm nothing from it landed on 999/2020.
739729
releasePut()
740-
await flushAsync()
741-
expect(screen.queryByText('Data saved successfully')).not.toBeInTheDocument()
742-
expect(screen.queryByLabelText('Standing Tree to Loaded Truck cost')).not.toBeInTheDocument()
730+
await waitFor(() => {
731+
expect(screen.queryByText('Data saved successfully')).not.toBeInTheDocument()
732+
expect(screen.queryByLabelText('Standing Tree to Loaded Truck cost')).not.toBeInTheDocument()
733+
})
743734
})
744735
})

‎frontend/src/components/schedule1/index.tsx‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -519,9 +519,6 @@ const Schedule1: FC = () => {
519519
className="schedule-1__actions"
520520
editable={editable}
521521
saving={saving}
522-
// Check Status now shares the single `saving` lock (it runs through the same run()); there is no
523-
// separate checking flag, so both Save and Check disable together while any write is in flight.
524-
checking={false}
525522
onSave={handleSave}
526523
onCheckStatus={handleCheckStatus}
527524
onDelete={() => setConfirmDeleteOpen(true)}

‎frontend/src/components/schedule2/__tests__/Schedule2.test.tsx‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,4 +348,71 @@ describe('Schedule2 page', () => {
348348
).toBeInTheDocument()
349349
expect(screen.queryByRole('button', { name: /^save$/i })).not.toBeInTheDocument()
350350
})
351+
352+
test('stale PUT is ignored when context changes before it settles (Story 29.6)', async () => {
353+
let putGate: (v: unknown) => void = () => {}
354+
const putPromise = new Promise((resolve) => {
355+
putGate = resolve
356+
})
357+
let releasePut = () => {}
358+
const releasePromise = new Promise<void>((resolve) => {
359+
releasePut = resolve
360+
})
361+
362+
let putCalled = false
363+
server.use(
364+
http.get(URL, ({ request }) =>
365+
new window.URL(request.url).searchParams.get('millId') === '999'
366+
? HttpResponse.json({
367+
...schedule2Doc,
368+
millId: 999,
369+
year: 2020,
370+
editable: false,
371+
comments: 'Context 999/2020 loaded',
372+
})
373+
: HttpResponse.json(schedule2Doc),
374+
),
375+
http.put(URL, async () => {
376+
putCalled = true
377+
putGate(null)
378+
await releasePromise
379+
return HttpResponse.json({
380+
...schedule2Doc,
381+
message: { key: 'dataSavedSuccesfullyInfoMsg', text: 'Data saved successfully' },
382+
})
383+
}),
384+
)
385+
386+
render(
387+
<MillYearProvider initial={{ millId: 514, year: 2021 }}>
388+
<StaleRaceHarness />
389+
</MillYearProvider>,
390+
)
391+
const user = userEvent.setup()
392+
393+
await screen.findAllByRole('button', { name: /^save$/i })
394+
await user.click(screen.getAllByRole('button', { name: /^save$/i })[0])
395+
await user.click(screen.getByRole('button', { name: /change/i }))
396+
397+
expect(await screen.findByText('Context 999/2020 loaded')).toBeInTheDocument()
398+
399+
releasePut()
400+
await waitFor(() => {
401+
expect(screen.queryByText('Data saved successfully')).not.toBeInTheDocument()
402+
})
403+
})
351404
})
405+
406+
import useMillYear from '@/context/millYear/useMillYear'
407+
408+
const StaleRaceHarness = () => {
409+
const { setContext } = useMillYear()
410+
return (
411+
<>
412+
<button type="button" onClick={() => setContext(999, 2020)}>
413+
change
414+
</button>
415+
<Schedule2 />
416+
</>
417+
)
418+
}

‎frontend/src/components/schedule2/index.tsx‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ const Schedule2: FC = () => {
9494
setCheckResult: setStatusMessages,
9595
clearBanners,
9696
resetBanners,
97+
run,
9798
save,
9899
remove,
99100
checkStatus,
@@ -152,14 +153,19 @@ const Schedule2: FC = () => {
152153
// from the reload, single-doc reset-in-place pages (Schedules 1/3) reset in place instead.
153154
onSuccess: (delResp) => {
154155
const deleteMessage = delResp?.message?.text ?? null
155-
apiService
156-
.getAxiosInstance()
157-
.get<Schedule2Response>(`/v1/schedule2?millId=${millId}&year=${year}`)
158-
.then((reload) => {
159-
setData(reload.data)
160-
setForm(seedForm(reload.data))
161-
setSaveMessage(deleteMessage)
162-
})
156+
run(
157+
apiService
158+
.getAxiosInstance()
159+
.get<Schedule2Response>(`/v1/schedule2?millId=${millId}&year=${year}`),
160+
{
161+
fallback: 'Deleted, but the list could not be refreshed.',
162+
onSuccess: (data) => {
163+
setData(data)
164+
setForm(seedForm(data))
165+
setSaveMessage(deleteMessage)
166+
},
167+
},
168+
)
163169
},
164170
})
165171
}

‎frontend/src/components/schedule3/__tests__/Schedule3.test.tsx‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,4 +432,71 @@ describe('Schedule3 sub-page navigation (AC6)', () => {
432432
await user.click(within(dialog).getByRole('button', { name: /cancel/i }))
433433
expect(mockNavigate).not.toHaveBeenCalled()
434434
})
435+
436+
test('stale PUT is ignored when context changes before it settles (Story 29.6)', async () => {
437+
let putGate: (v: unknown) => void = () => {}
438+
const putPromise = new Promise((resolve) => {
439+
putGate = resolve
440+
})
441+
let releasePut = () => {}
442+
const releasePromise = new Promise<void>((resolve) => {
443+
releasePut = resolve
444+
})
445+
446+
let putCalled = false
447+
server.use(
448+
http.get(URL, ({ request }) =>
449+
new window.URL(request.url).searchParams.get('millId') === '999'
450+
? HttpResponse.json({
451+
...schedule3Doc,
452+
millId: 999,
453+
year: 2020,
454+
editable: false,
455+
comments: 'Context 999/2020 loaded',
456+
})
457+
: HttpResponse.json(schedule3Doc),
458+
),
459+
http.put(URL, async () => {
460+
putCalled = true
461+
putGate(null)
462+
await releasePromise
463+
return HttpResponse.json({
464+
...schedule3Doc,
465+
message: { key: 'dataSavedSuccesfullyInfoMsg', text: 'Data saved successfully' },
466+
})
467+
}),
468+
)
469+
470+
render(
471+
<MillYearProvider initial={{ millId: 514, year: 2021 }}>
472+
<StaleRaceHarness />
473+
</MillYearProvider>,
474+
)
475+
const user = userEvent.setup()
476+
477+
await screen.findAllByRole('button', { name: /^save$/i })
478+
await user.click(screen.getAllByRole('button', { name: /^save$/i })[0])
479+
await user.click(screen.getByRole('button', { name: /change/i }))
480+
481+
expect(await screen.findByText('Context 999/2020 loaded')).toBeInTheDocument()
482+
483+
releasePut()
484+
await waitFor(() => {
485+
expect(screen.queryByText('Data saved successfully')).not.toBeInTheDocument()
486+
})
487+
})
435488
})
489+
490+
import useMillYear from '@/context/millYear/useMillYear'
491+
492+
const StaleRaceHarness = () => {
493+
const { setContext } = useMillYear()
494+
return (
495+
<>
496+
<button type="button" onClick={() => setContext(999, 2020)}>
497+
change
498+
</button>
499+
<Schedule3 />
500+
</>
501+
)
502+
}

‎frontend/src/components/schedule3/index.tsx‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -426,9 +426,6 @@ const Schedule3: FC = () => {
426426
className="schedule-3__actions"
427427
editable={editable}
428428
saving={saving}
429-
// Check Status now shares the single `saving` lock (it runs through the same run()); there is no
430-
// separate checking flag, so both Save and Check disable together while any write is in flight.
431-
checking={false}
432429
onSave={handleSave}
433430
onCheckStatus={handleCheckStatus}
434431
onDelete={() => setConfirmDeleteOpen(true)}

‎frontend/src/components/schedule4/__tests__/Schedule4.test.tsx‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -784,4 +784,85 @@ describe('Schedule4 context, load + write error, edit, delete and status paths',
784784

785785
expect(await screen.findByRole('table', { name: /Towing Total/i })).toBeInTheDocument()
786786
})
787+
788+
test('stale PUT is ignored when context changes before it settles (Story 29.6)', async () => {
789+
let putGate: (v: unknown) => void = () => {}
790+
const putPromise = new Promise((resolve) => {
791+
putGate = resolve
792+
})
793+
let releasePut = () => {}
794+
const releasePromise = new Promise<void>((resolve) => {
795+
releasePut = resolve
796+
})
797+
798+
let putCalled = false
799+
server.use(
800+
http.get(URL, ({ request }) =>
801+
new window.URL(request.url).searchParams.get('millId') === '999'
802+
? HttpResponse.json(
803+
doc({
804+
millId: 999,
805+
year: 2020,
806+
editable: false,
807+
locations: [{ id: 999, revisionCount: 1, name: 'Context 999/2020 loaded', comments: null, categories: [], subPageRows: [] }],
808+
}),
809+
)
810+
: HttpResponse.json(doc()),
811+
),
812+
http.put(LOCATIONS_URL, async () => {
813+
putCalled = true
814+
putGate(null)
815+
await releasePromise
816+
return HttpResponse.json({
817+
...doc(),
818+
message: { key: 'dataSavedSuccesfullyInfoMsg', text: 'Data saved successfully' },
819+
})
820+
}),
821+
)
822+
823+
const rootRoute = createRootRoute()
824+
const scheduleRoute = createRoute({
825+
getParentRoute: () => rootRoute,
826+
path: '/schedule-4',
827+
validateSearch: realScheduleRoute.options.validateSearch,
828+
component: () => (
829+
<MillYearProvider initial={{ millId: 514, year: 2021 }}>
830+
<StaleRaceHarness />
831+
</MillYearProvider>
832+
),
833+
})
834+
const router = createRouter({
835+
routeTree: rootRoute.addChildren([scheduleRoute]),
836+
history: createMemoryHistory({ initialEntries: ['/schedule-4'] }),
837+
})
838+
render(<RouterProvider router={router} />)
839+
const user = userEvent.setup()
840+
841+
await screen.findByText('Harbour Dump')
842+
await user.click(screen.getByRole('button', { name: /add new location/i }))
843+
await user.type(screen.getByLabelText('Location Name'), 'New Dump')
844+
await user.click(screen.getByRole('button', { name: /^save$/i }))
845+
await user.click(screen.getByRole('button', { name: /change/i }))
846+
847+
expect(await screen.findByText('Context 999/2020 loaded')).toBeInTheDocument()
848+
849+
releasePut()
850+
await waitFor(() => {
851+
expect(screen.queryByText('Data saved successfully')).not.toBeInTheDocument()
852+
})
853+
})
787854
})
855+
856+
import useMillYear from '@/context/millYear/useMillYear'
857+
858+
const StaleRaceHarness = () => {
859+
const { setContext } = useMillYear()
860+
return (
861+
<>
862+
<button type="button" onClick={() => setContext(999, 2020)}>
863+
change
864+
</button>
865+
<Schedule4 />
866+
</>
867+
)
868+
}

‎frontend/src/components/schedule4/index.tsx‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -378,15 +378,15 @@ const Schedule4: FC = () => {
378378
// the entered name off these rather than the (possibly changed) live state.
379379
const wasEdit = panelMode === 'edit'
380380
const editId = panelEditId
381-
const savedName = panelName.trim()
381+
const prevIds = new Set(data?.locations.map((l) => l.id) ?? [])
382382
putLocation((document) => {
383383
// Stay on the saved record (don't close): re-open it in edit mode — found by id when editing, by
384384
// (unique) name after a new/copy create — refreshing the optimistic-lock token so a follow-up
385385
// save doesn't 409. The panel form already holds the saved values, so nothing re-seeds.
386386
const saved =
387387
wasEdit && editId !== null
388388
? document.locations.find((l) => l.id === editId)
389-
: document.locations.find((l) => (l.name ?? '').toLowerCase() === savedName.toLowerCase())
389+
: document.locations.find((l) => l.id != null && !prevIds.has(l.id))
390390
if (saved && saved.id != null) {
391391
setPanelMode('edit')
392392
setPanelEditId(saved.id)
@@ -419,10 +419,15 @@ const Schedule4: FC = () => {
419419
setSaveMessage(resp?.message?.text ?? null)
420420
setPanelMode('closed')
421421
// Re-read the document so the list reflects the removed family (delete returns only a message).
422-
void apiService
423-
.getAxiosInstance()
424-
.get<Schedule4Response>(`/v1/schedule4?millId=${millId}&year=${year}`)
425-
.then((reload) => setData(reload.data))
422+
run(
423+
apiService
424+
.getAxiosInstance()
425+
.get<Schedule4Response>(`/v1/schedule4?millId=${millId}&year=${year}`),
426+
{
427+
fallback: 'Deleted, but the list could not be refreshed.',
428+
onSuccess: (data) => setData(data),
429+
},
430+
)
426431
},
427432
},
428433
)
@@ -449,9 +454,9 @@ const Schedule4: FC = () => {
449454
// Save the panel (create path) and open the sub-page for the new location — the create → save-first
450455
// (NAV-003) flow. Runs the post-save lookup inside putLocation's guarded onSuccess.
451456
const saveLocationThenOpen = (def: SubPageDef) => {
452-
const savedName = panelName.trim()
457+
const prevIds = new Set(data?.locations.map((l) => l.id) ?? [])
453458
putLocation((document) => {
454-
const id = document.locations.find((l) => l.name === savedName)?.id ?? null
459+
const id = document.locations.find((l) => l.id != null && !prevIds.has(l.id))?.id ?? null
455460
if (id !== null) openSubPage(def, id)
456461
})
457462
}

0 commit comments

Comments
 (0)