fix(shared): show pending state while deleting squads - #6506
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
rebelchris
left a comment
There was a problem hiding this comment.
Summary
Solid direction: routing the delete through a mutation and awaiting network → boot-cache prune → navigation in order is the right fix for ENG-1945, and the added specs cover the pending/error paths well. Two consumers of useDeleteSquad (SquadDangerZone, SquadHeaderMenu) both benefit, and DangerZone already accepts buttonDisabled/buttonLoading, so no shared-component change was needed.
One blocking concern (the prompt becomes undismissable while a confirm is in flight) plus four non-blocking items, left inline.
Verification
- Read root + package
AGENTS.md - Scope matches the linked issue
- Traced confirm → mutation → boot cache → navigation → success/error paths
- Enumerated
useDeleteSquadandPromptElementconsumers (MainLayout, RecruiterLayout x2, RecruiterFullscreenLayout, companionApp.tsx) - CI inspected (
lint_shared,typecheck_strict_changed,test_webapp,test_extensiongreen;test_sharedstill running at review time) - Not run locally — relying on CI for
test_shared - Manual check still worth doing: mobile drawer variant of the delete prompt while pending
Reviewed by AI.
| isDrawerOnMobile | ||
| drawerProps={{ displayCloseButton: false, appendOnRoot: true }} | ||
| shouldCloseOnOverlayClick={shouldCloseOnOverlayClick} | ||
| shouldCloseOnOverlayClick={isPending ? false : shouldCloseOnOverlayClick} |
There was a problem hiding this comment.
Blocking: while isPending is true the prompt has no escape hatch at all. Overlay click is forced off here, onRequestClose (ESC / react-modal) short-circuits in handleFail, the cancel button is disabled, and this modal renders only Modal.Body — no Modal.Header/close button — with drawerProps.displayCloseButton: false on mobile. So a slow or hung deleteSquad (flaky network, gateway timeout, request that never settles) leaves the user locked in a loading modal with a full-page reload as the only way out. The PR description accepts "a hung delete remains visibly pending", but that trade-off is about the spinner, not about trapping the user.
Suggested direction: keep cancel enabled while pending and treat it as "dismiss the UI" (the mutation keeps running and the toast/navigation still fires), or leave ESC/overlay close available and only guard the confirm action. Guarding just re-entrancy of onConfirm is enough to prevent a double delete — okButton is already disabled.
Reviewed by AI.
| }); | ||
| await deleteSquad(squad.id!); | ||
| deleteCachedSquad(squad.id!); | ||
| await callback?.(); |
There was a problem hiding this comment.
Non-blocking (real failure mode): callback runs inside mutationFn, so a rejection from the callback is indistinguishable from a failed delete. Both consumers pass () => router.replace('/'), and the pages-router replace promise rejects when a route change is cancelled or aborted — which is exactly what can happen right after a delete, when the now-404 squad page kicks off its own navigation. In that case the squad is deleted, the boot cache is pruned, but the user still gets the generic DEFAULT_ERROR toast and showPrompt rejects.
Suggested direction: keep the awaited ordering but scope the error mapping — run callback in onSuccess, or wrap the callback so post-delete navigation failures don't surface as "delete failed".
Reviewed by AI.
| setPrompt(null); | ||
| }; | ||
| const failWith = (error: unknown) => { | ||
| reject(error); |
There was a problem hiding this comment.
Non-blocking (contract change): showPrompt previously never rejected, so every existing caller can safely await it. With onConfirm it now rejects, and the only defence is the showPrompt(options).catch(() => false) in useDeleteSquad — the next caller that forgets it gets an unhandled rejection.
Since the confirm error is already surfaced by the caller's own mutation onError, would resolving false (or { confirmed, error }) be safer than rejecting? It keeps the Promise<boolean> contract non-throwing for every caller. If rejection is deliberate, worth making it explicit in the PromptOptions.onConfirm type/JSDoc.
Reviewed by AI.
| disabled: isPending || !!cancelButtonDisabled, | ||
| className: classNames('w-full tablet:w-auto', cancelButtonClassName), | ||
| } as ButtonProps<'button'>; | ||
| const okActionButtonProps = { |
There was a problem hiding this comment.
Non-blocking (bloat / type safety): this rebuild costs ~60 lines and two as ButtonProps<'button'> casts to add three things — onClick, disabled, loading. The casts are what worries me: they silence the ColorButtonProps/IconButtonProps discriminated unions, so a future edit that passes color without variant, or iconPosition without icon, no longer fails typecheck.
PromptButtonProps already does Omit<ButtonProps<'button'>, 'onClick'>, so callers cannot supply onClick and the old spread order was never a hazard. The same behaviour keeps its types with:
<Button
variant={okButton.variant ?? ButtonVariant.Primary}
{...okButton}
onClick={handleSuccess}
disabled={isPending || okButton.disabled}
loading={isPending || okButton.loading}
className={classNames('w-full tablet:w-auto', okButton.className)}
>Two incidental behaviour changes fall out of the rewrite that aren't in the description: iconPosition: … ?? ButtonIconPosition.Left is redundant (Button already defaults iconPosition to Left), and destructuring title out drops the native title attribute that the previous spread put on the DOM node. Both are almost certainly fine, just not stated.
Reviewed by AI.
|
|
||
| export function PromptElement(props: Partial<ModalProps>): ReactElement | null { | ||
| const { prompt } = usePrompt(); | ||
| const [isPending, setIsPending] = useState(false); |
There was a problem hiding this comment.
Non-blocking (edge case): isPending is component state on the always-mounted PromptElement, not scoped to the prompt instance, and it is never reset when prompt changes identity. If any code calls showPrompt again while an async confirm is in flight (the query-cache prompt is simply overwritten), the newly shown prompt renders with both buttons disabled and a spinner it never asked for, and only recovers when the previous onConfirm settles. Resetting on prompt change, or storing the pending flag alongside the prompt in the query cache, would make this self-consistent.
Reviewed by AI.
- keep cancel/esc/overlay close available while an async confirm runs so a slow delete cannot trap the user in a modal without an escape hatch - scope the pending flag to the prompt instance so a replacing prompt does not render disabled - resolve showPrompt with false instead of rejecting on confirm failure - do not map post-delete callback (navigation) failures to a delete error - drop the button prop rebuild and the ButtonProps casts
4311df5 to
7b1941f
Compare
|
Follow-up commits address the review findings on this PR:
Note on CI: Reviewed by AI. Follow-up changes authored by AI. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…acks-a-processing-ui
Summary
Key decisions
showPrompt(options): Promise<boolean>contract additive and non-throwing: a failedonConfirmcloses the prompt and resolvesfalse, so no caller needs acatch.callback(navigation) failures are not mapped to a delete failure — a cancelledrouter.replaceno longer produces a false error toast.Verification
NODE_ENV=test pnpm --filter @dailydotdev/shared exec jest src/components/modals/Prompt.spec.tsx src/hooks/useDeleteSquad.spec.tsx src/components/squads/SquadHeaderMenu.spec.tsx --runInBandNODE_ENV=test pnpm --filter @dailydotdev/shared exec eslint src/hooks/usePrompt.ts src/components/modals/Prompt.tsx src/hooks/useDeleteSquad.ts src/components/squads/settings/SquadDangerZone.tsx src/components/modals/Prompt.spec.tsx src/hooks/useDeleteSquad.spec.tsx src/components/squads/SquadHeaderMenu.spec.tsx --max-warnings 0node ./scripts/typecheck-strict-changed.jslint_shared/typecheck_strict_changed/test_shared.Issue: https://linear.app/dailydev/issue/ENG-1945/feedback-ux-issue-deleting-squads-lacks-a-processing-ui-causing-user
Closes ENG-1945
Created by Huginn 🐦⬛
Preview domain
https://eng-1945-feedback-ux-issue-delet.preview.app.daily.dev