From adb1773be81ea587f4d1e3d94a4b0fcc7561a438 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Ign=C3=A1cio=20Fontoura=20Solla?= Date: Mon, 27 Jul 2026 22:17:44 -0300 Subject: [PATCH] Harden the level control now that it also renders as a Drawer. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The migration put this control on a phone for the first time and it was still mouse-sized: a 28px submit and checkbox rows with no minimum height, where the siblings size both for touch. Closing the overlay mid-submit was worse — it unmounts the Alert and the live region while nothing aborts the request, so a failure went unreported; it now uses the same `reportFailure` the trend and estimate cells do. The rest is what the reviewers found around it: the trigger reports `aria-busy` like every other overlay caller, `canSubmit` carries the override rule that was stranded in the JSX, the two-level-jump check reads `ENGAGEMENT_LEVEL_RULES.maxStepsWithoutShock` instead of a literal so E15 cannot desync the checkbox from the rule behind it, and the route echoes `updated.engagementLevel` rather than the requested one — with `overrideAccess: false` Payload drops a field the actor cannot write instead of refusing, so the request is not evidence the movement landed. The cell's state stopped being nullable, which meant splitting it from the route's success payload: a success always names a level, a row need not have one. And the control joins `overlayCases`, the table that exists to catch exactly the kind of container mistake this migration could have made. --- .../municipios/engagement-level/route.ts | 6 +- .../municipios/engagement-level/types.ts | 9 +- .../MunicipalityListLevelControl.tsx | 90 +++++++++++++------ src/lib/engagementLevel.ts | 2 +- .../unit/campaignCellEditOverlay.unit.spec.ts | 27 ++++-- 5 files changed, 91 insertions(+), 43 deletions(-) diff --git a/src/app/(campaign)/campanha/(app)/municipios/engagement-level/route.ts b/src/app/(campaign)/campanha/(app)/municipios/engagement-level/route.ts index f1fff536..ec3dfaee 100644 --- a/src/app/(campaign)/campanha/(app)/municipios/engagement-level/route.ts +++ b/src/app/(campaign)/campanha/(app)/municipios/engagement-level/route.ts @@ -44,8 +44,12 @@ export async function POST( return NextResponse.json({ status: 'success', message: 'Nível de envolvimento registrado.', + // Echoes the document, never the request: with `overrideAccess: false` + // Payload drops a field the actor cannot write instead of refusing the + // update, so reporting the requested level could claim a movement that + // never landed. savedLevel: { - level: movement.level, + level: updated.engagementLevel ?? movement.level, note: updated.levelNote ?? null, changedAt: updated.levelChangedAt ?? null, }, diff --git a/src/app/(campaign)/campanha/(app)/municipios/engagement-level/types.ts b/src/app/(campaign)/campanha/(app)/municipios/engagement-level/types.ts index 4bb44b04..6a4f484c 100644 --- a/src/app/(campaign)/campanha/(app)/municipios/engagement-level/types.ts +++ b/src/app/(campaign)/campanha/(app)/municipios/engagement-level/types.ts @@ -1,11 +1,5 @@ import type { EngagementLevel, EngagementLevelViolation } from '@/lib/engagementLevel' -export type MunicipalityListSavedEngagementLevel = { - level: EngagementLevel - note: string | null - changedAt: string | null -} - /** * `blocked` is a third state on purpose: a movement held by the stability * rules is not a failure the coordinator should retry, it is a decision they @@ -16,7 +10,8 @@ export type MunicipalityListEngagementLevelResponse = | { status: 'success' message: string - savedLevel: MunicipalityListSavedEngagementLevel + // The level is never null here — a success is a movement TO a level. + savedLevel: { level: EngagementLevel; note: string | null; changedAt: string | null } } | { status: 'blocked' diff --git a/src/components/campaign/municipality/MunicipalityListLevelControl.tsx b/src/components/campaign/municipality/MunicipalityListLevelControl.tsx index 6de81118..bc6eccf9 100644 --- a/src/components/campaign/municipality/MunicipalityListLevelControl.tsx +++ b/src/components/campaign/municipality/MunicipalityListLevelControl.tsx @@ -1,11 +1,9 @@ 'use client' import { useEffect, useRef, useState } from 'react' +import { toast } from 'sonner' -import type { - MunicipalityListEngagementLevelResponse, - MunicipalityListSavedEngagementLevel, -} from '@/app/(campaign)/campanha/(app)/municipios/engagement-level/types' +import type { MunicipalityListEngagementLevelResponse } from '@/app/(campaign)/campanha/(app)/municipios/engagement-level/types' import { MunicipalityLevelBadge } from '@/components/campaign/municipality/MunicipalityLevelBadge' import { CampaignCellEditOverlay, @@ -20,6 +18,7 @@ import { Spinner } from '@/components/ui/Spinner' import { Textarea } from '@/components/ui/textarea' import { EMPTY_ENGAGEMENT_LEVEL_LABEL, + ENGAGEMENT_LEVEL_RULES, ENGAGEMENT_LEVEL_TEXT_MAX_LENGTH, engagementLevelRank, engagementLevels, @@ -43,7 +42,7 @@ type MunicipalityListLevelControlProps = { } /** - * E14 — unlike the trend and estimate popovers next to it, this one submits + * E14 — unlike the trend and estimate cells next to it, this one submits * explicitly: a movement carries a motivo AND the signals that would reverse * it, and it may need an override. Auto-saving half of that would file an * incomplete decision under the coordinator's name. @@ -57,10 +56,18 @@ export const MunicipalityListLevelControl = ({ variant, }: MunicipalityListLevelControlProps) => { const [open, setOpen] = useState(false) - const [saved, setSaved] = useState( - level ? { level, note: levelNote, changedAt: levelChangedAt } : null, - ) - // Seeded by `resetDraft` on every open; while closed the popover body — and + // Not the route's success payload: that one always names a level, and a row + // need not have one. + const [saved, setSaved] = useState<{ + level: EngagementLevel | null + note: string | null + changedAt: string | null + }>({ + level, + note: levelNote, + changedAt: levelChangedAt, + }) + // Seeded by `resetDraft` on every open; while closed the overlay body — and // with it the select — is not mounted. const [draftLevel, setDraftLevel] = useState('') const [note, setNote] = useState('') @@ -76,8 +83,17 @@ export const MunicipalityListLevelControl = ({ violations: EngagementLevelViolation[] } | null>(null) const abortRef = useRef(null) + const openRef = useRef(false) const lastPropsRef = useRef({ level, levelNote, levelChangedAt }) + // Closing unmounts the Alert and the live region, and nothing aborts a + // movement already in flight — so a failure after that would be silent. + // Same shape as the trend and estimate controls. + const reportFailure = (message: string) => { + setErrorMessage(message) + if (!openRef.current) toast.error(message) + } + // Adopt the server value when it changes from outside (navigation / refresh). useEffect(() => { const last = lastPropsRef.current @@ -88,13 +104,16 @@ export const MunicipalityListLevelControl = ({ ) return lastPropsRef.current = { level, levelNote, levelChangedAt } - setSaved(level ? { level, note: levelNote, changedAt: levelChangedAt } : null) + setSaved({ level, note: levelNote, changedAt: levelChangedAt }) }, [level, levelNote, levelChangedAt]) useEffect(() => () => abortRef.current?.abort(), []) - const currentLevel = saved?.level ?? null - const currentNote = saved?.note ?? null + const currentLevel = saved.level + const currentNote = saved.note + const currentLabel = currentLevel + ? formatEngagementLevelLabel(currentLevel) + : EMPTY_ENGAGEMENT_LEVEL_LABEL const isMovement = draftLevel !== '' && draftLevel !== currentLevel // The rules are pure and client-safe, so the coordinator sees why a movement @@ -105,7 +124,7 @@ export const MunicipalityListLevelControl = ({ ? getEngagementLevelViolations({ from: currentLevel, to: draftLevel, - levelChangedAt: saved?.changedAt ?? null, + levelChangedAt: saved.changedAt, now: new Date(), triangulatedShock, }) @@ -123,7 +142,8 @@ export const MunicipalityListLevelControl = ({ const isJump = isMovement && currentLevel !== null && - Math.abs(engagementLevelRank[draftLevel] - engagementLevelRank[currentLevel]) > 1 + Math.abs(engagementLevelRank[draftLevel] - engagementLevelRank[currentLevel]) > + ENGAGEMENT_LEVEL_RULES.maxStepsWithoutShock const resetDraft = () => { setDraftLevel(currentLevel ?? '') @@ -137,6 +157,7 @@ export const MunicipalityListLevelControl = ({ const handleOpenChange = (nextOpen: boolean) => { if (nextOpen) resetDraft() + openRef.current = nextOpen setOpen(nextOpen) } @@ -165,31 +186,42 @@ export const MunicipalityListLevelControl = ({ }), }) + // Deliberately not gated on `response.ok` the way the siblings are: + // `blocked` arrives as a 409 carrying the violations the coordinator may + // override, so the status line is not the signal here — the discriminant is. const payload = (await response.json()) as MunicipalityListEngagementLevelResponse if (payload.status === 'success') { setSaved(payload.savedLevel) - setOpen(false) + handleOpenChange(false) return } - // A blocked movement keeps the popover open with the draft intact: the + // A blocked movement keeps the overlay open with the draft intact: the // next click is "override", not "type it all again". if (payload.status === 'blocked') { setServerBlock({ level: draftLevel, violations: payload.violations }) return } - setErrorMessage(payload.message) + reportFailure(payload.message) } catch { - setErrorMessage(SAVE_ERROR_MESSAGE) + // Network failure, a non-JSON error page, or the unmount abort below — + // only the first two have anyone left to tell. + if (controller.signal.aborted) return + reportFailure(SAVE_ERROR_MESSAGE) } finally { setIsPending(false) } } - const canSubmit = isMovement && note.trim().length > 0 && reversalSignals.trim().length > 0 + const canSubmit = + isMovement && + note.trim().length > 0 && + reversalSignals.trim().length > 0 && + (violations.length === 0 || override) const fieldId = (suffix: string) => `municipality-list-level-${suffix}-${municipalityID}` + const isSheet = variant === 'sheet' return ( -

{formatEngagementLevelLabel(currentLevel)}

+

{currentLabel}

{currentNote ?

{currentNote}

: null} ) : null @@ -216,8 +247,8 @@ export const MunicipalityListLevelControl = ({ // level out there and stays the bare numeral in the table. } > @@ -265,7 +296,7 @@ export const MunicipalityListLevelControl = ({ /> {isJump ? ( - + - + 0 && !override)} + size={isSheet ? 'default' : 'sm'} + className={isSheet ? 'min-h-11 w-full' : undefined} + disabled={!canSubmit || isPending} onClick={() => void submit()} > {isPending ? : null} diff --git a/src/lib/engagementLevel.ts b/src/lib/engagementLevel.ts index 4d80db7c..84fffbc1 100644 --- a/src/lib/engagementLevel.ts +++ b/src/lib/engagementLevel.ts @@ -47,7 +47,7 @@ const engagementLevelNames: Record = { /** * "N3 · Rede + agenda" — the numeral alone is the badge, this is the tooltip. - * Here rather than in `municipalityLabels` so the Popover island can name a + * Here rather than in `municipalityLabels` so the client island can name a * level without importing a module that builds the 435-entry catalog and the * concept glossary at init. */ diff --git a/tests/unit/campaignCellEditOverlay.unit.spec.ts b/tests/unit/campaignCellEditOverlay.unit.spec.ts index ca1a9553..197f581c 100644 --- a/tests/unit/campaignCellEditOverlay.unit.spec.ts +++ b/tests/unit/campaignCellEditOverlay.unit.spec.ts @@ -4,16 +4,19 @@ import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest import { MunicipalityListAdvisorsControl } from '@/components/campaign/municipality/MunicipalityListAdvisorsControl' import { MunicipalityListExpectedVotesControl } from '@/components/campaign/municipality/MunicipalityListExpectedVotesControl' +import { MunicipalityListLevelControl } from '@/components/campaign/municipality/MunicipalityListLevelControl' import { MunicipalityListTrendControl } from '@/components/campaign/municipality/MunicipalityListTrendControl' import type { CampaignCellEditOverlayVariant } from '@/components/campaign/shared/CampaignCellEditOverlay' import { toVoteEstimateScenarioViewModel } from '@/lib/voteEstimate' /** - * B42: the three auto-save quick-edit cells of `/campanha/municipios` share one - * container (`CampaignCellEditOverlay`) that is a Popover in the desktop table - * and a bottom Drawer in the mobile card. These cases pin the part a call site - * can silently get wrong — which container it renders and whether the touch - * surface says which município is being edited — for all three at once. + * B42: the quick-edit cells of `/campanha/municipios` share one container + * (`CampaignCellEditOverlay`) that is a Popover in the desktop table and a + * bottom Drawer in the mobile card. These cases pin the part a call site can + * silently get wrong — which container it renders and whether the touch surface + * says which município is being edited — for all of them at once. Three + * auto-save; the level cell (E14) submits explicitly and is here for the + * container contract all four share. */ const MUNICIPALITY_NAME = 'Feira de Santana' @@ -72,6 +75,20 @@ const overlayCases: OverlayCase[] = [ variant, }), }, + { + name: 'nível de envolvimento', + triggerLabel: `Nível de envolvimento de ${MUNICIPALITY_NAME}: N3 · Rede + agenda`, + drawerTitle: 'Registrar nível de envolvimento', + element: (variant) => + createElement(MunicipalityListLevelControl, { + municipalityID: 1, + municipalityName: MUNICIPALITY_NAME, + level: 'n3', + levelNote: 'Rede montada e giro agendado', + levelChangedAt: '2026-07-01T12:00:00.000Z', + variant, + }), + }, ] beforeAll(() => {