From 5cecb367b01d7555eb98de78cc5fdaba51558555 Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Sat, 10 Oct 2026 11:50:53 -0400 Subject: [PATCH] refactor(groupwizard): drop unreachable empty-fields branch in handleNext The `!fields || fields.length === 0` special case could never run: only the Review step has an empty STEP_FIELDS entry, and Review shows Submit, not Next. form.trigger([]) resolves true with the zod resolver anyway, and a missing entry (undefined) validates the whole form, so calling trigger unconditionally fails safe. Adds a schema test pinning that every step before Review lists fields. group-wizard.tsx is now 100% statements. Closes the 2026-09-13 dead-empty-fields-branch-handlenext TODO. Co-Authored-By: Claude Opus 5.5 (1M context) --- ...-13-dead-empty-fields-branch-handlenext.md | 66 ------------------- .claude/TODO/INDEX.md | 15 ++--- .../(web)/tools/groupwizard/group-wizard.tsx | 11 +--- src/components/group-wizard/schema.test.ts | 12 ++++ 4 files changed, 22 insertions(+), 82 deletions(-) delete mode 100644 .claude/TODO/2026-09-13-dead-empty-fields-branch-handlenext.md diff --git a/.claude/TODO/2026-09-13-dead-empty-fields-branch-handlenext.md b/.claude/TODO/2026-09-13-dead-empty-fields-branch-handlenext.md deleted file mode 100644 index 643b5f0..0000000 --- a/.claude/TODO/2026-09-13-dead-empty-fields-branch-handlenext.md +++ /dev/null @@ -1,66 +0,0 @@ ---- -title: Unreachable "empty STEP_FIELDS" branch in GroupWizard.handleNext -severity: low -tags: [refactor, missing-test] -area: components -files: [src/app/(web)/tools/groupwizard/group-wizard.tsx] -discovered: 2026-09-13 -discovered_by: coverage-agent-group-wizard -status: open ---- - -## Problem -`handleNext` in `group-wizard.tsx` (lines 120-133) special-cases a step whose -`STEP_FIELDS[currentStep]` entry is missing or empty by skipping validation -and advancing immediately: - -```ts -const fields = STEP_FIELDS[currentStep]; -if (!fields || fields.length === 0) { - setCompletedSteps((prev) => new Set(prev).add(currentStep)); - setCurrentStep((prev) => Math.min(prev + 1, WIZARD_STEPS.length - 1)); - return; -} -``` - -The only entry in `STEP_FIELDS` (see `schema.ts`) with an empty array is -index `5` (the Review step, `STEP_FIELDS[5] = []`). But `WizardNavigation` -never renders a "Next" button on the last step (`currentStep === totalSteps - 1`) -— it renders the Submit/"Save Changes" button instead, which calls -`handleSubmit`, not `handleNext`. `STEP_FIELDS` is otherwise fully populated -for indices 0-4, and `currentStep` never exceeds `WIZARD_STEPS.length - 1` -(5), so this branch is unreachable through the actual UI. - -This isn't a security or correctness bug (nothing lets a user skip real -validation — the branch simply can never execute), but it is 3 statements of -dead defensive code that both inflates the file's apparent branch count and -cannot be exercised by any user-facing test, which is why coverage tooling -flags `group-wizard.tsx` at 94.5% statements instead of ≥95% despite every -reachable branch being covered. - -## Evidence -- `src/app/(web)/tools/groupwizard/group-wizard.tsx:120-126` — the - `!fields || fields.length === 0` branch. -- `src/components/group-wizard/schema.ts:58-65` — `STEP_FIELDS`, only index - `5` is empty, and index 5 is the last step. -- `src/components/group-wizard/wizard-navigation.tsx:28,55-75` — `isReviewStep` - gates rendering Submit vs. Next; the last step never shows the Next button - that calls `handleNext`. -- `npx vitest run --coverage` on `group-wizard.tsx` reports lines 123-125 - uncovered even after `src/app/(web)/tools/groupwizard/group-wizard.test.tsx` - was extended with 12 additional cases covering every other branch/function - in the file (94.5% statements / 96.25% lines final). - -## Proposed fix -Either: -1. Remove the `!fields || fields.length === 0` special case entirely (the - `form.trigger([])` call with an empty array resolves `true` immediately - in react-hook-form, so simplifying to always call `form.trigger(fields)` - would behave identically and remove the dead branch), or -2. If the branch is meant as future-proofing for a step reachable via some - other entry point, add a code comment explaining that intent so the next - reader (and coverage tooling) doesn't flag it as untested dead code. - -## Impact if not fixed -None functionally — this is a coverage/clarity issue, not a behavior bug. -Low priority; safe to leave as documented, known-dead code. diff --git a/.claude/TODO/INDEX.md b/.claude/TODO/INDEX.md index 1611244..3bef953 100644 --- a/.claude/TODO/INDEX.md +++ b/.claude/TODO/INDEX.md @@ -18,7 +18,7 @@ Severity tiers: - **medium**: doc drift, missing test, refactor with real cost - **low**: nits, minor doc fixes, stylistic improvements -Total: **4 open TODOs**. +Total: **3 open TODOs**. > **`step6`** and **`step8`** are the remaining steps from the 2026-09-30 MPNext > playbook audit (one PR each). `held-*` items wait on an owner decision. @@ -39,11 +39,10 @@ _none open_ | routing | security, drift | Step 6 — Headers, next.config, server-only, prerender + build in CI | [→](2026-09-30-step6-headers-csp-build-guards.md) | | routing | security, doc | Held — CSP notes for customer-deployed clones | [→](2026-09-30-held-csp-customer-deployments.md) | -### Low (2) +### Low (1) | Area | Tags | Title | File | |---|---|---|---| | components | security, bug | Step 8 — Address-label caps, error boundaries, tool-params decode, codegen escaping | [→](2026-09-30-step8-fork-specific-low-risk-items.md) | -| components | refactor, missing-test | Unreachable "empty STEP_FIELDS" branch in `GroupWizard.handleNext` | [→](2026-09-13-dead-empty-fields-branch-handlenext.md) | --- @@ -60,11 +59,11 @@ _none open_ ### drift (1) - step6-headers-csp-build-guards — medium -### missing-test (1) -- dead-empty-fields-branch-handlenext — low +### missing-test (0) +_none open_ -### refactor (1) -- dead-empty-fields-branch-handlenext — low +### refactor (0) +_none open_ ### doc (1) - held-csp-customer-deployments — medium @@ -78,7 +77,7 @@ _none open_ | Area | Count | |---|---| -| components | 2 | +| components | 1 | | routing | 2 | | services | 0 | | auth | 0 | diff --git a/src/app/(web)/tools/groupwizard/group-wizard.tsx b/src/app/(web)/tools/groupwizard/group-wizard.tsx index 873cfc2..d387806 100644 --- a/src/app/(web)/tools/groupwizard/group-wizard.tsx +++ b/src/app/(web)/tools/groupwizard/group-wizard.tsx @@ -118,14 +118,9 @@ export function GroupWizard({ params, mpTimezone }: GroupWizardProps) { }, []); const handleNext = useCallback(async () => { - const fields = STEP_FIELDS[currentStep]; - if (!fields || fields.length === 0) { - setCompletedSteps((prev) => new Set(prev).add(currentStep)); - setCurrentStep((prev) => Math.min(prev + 1, WIZARD_STEPS.length - 1)); - return; - } - - const isValid = await form.trigger(fields as (keyof GroupWizardFormData)[]); + // No special case for a step without fields: `trigger([])` resolves true, + // and a missing entry (`undefined`) validates the whole form — fails safe. + const isValid = await form.trigger(STEP_FIELDS[currentStep]); if (isValid) { setCompletedSteps((prev) => new Set(prev).add(currentStep)); setCurrentStep((prev) => Math.min(prev + 1, WIZARD_STEPS.length - 1)); diff --git a/src/components/group-wizard/schema.test.ts b/src/components/group-wizard/schema.test.ts index d04333f..12eb32d 100644 --- a/src/components/group-wizard/schema.test.ts +++ b/src/components/group-wizard/schema.test.ts @@ -6,6 +6,7 @@ import { STEP_FIELDS, type GroupWizardFormData, } from './schema'; +import { WIZARD_STEPS } from './types'; /** Helper: fill all required IDs + Start_Date so schema passes base validation */ function validBase(): GroupWizardFormData { @@ -113,6 +114,17 @@ describe('STEP_FIELDS', () => { expect(Object.keys(STEP_FIELDS)).toEqual(['0', '1', '2', '3', '4', '5']); }); + // GroupWizard.handleNext calls form.trigger(STEP_FIELDS[currentStep]) with no + // special case, and the last step shows Submit, not Next. So every step + // before Review must list fields to validate, or Next would skip validation. + it('every step before the Review step has fields, and only Review is empty', () => { + expect(Object.keys(STEP_FIELDS)).toHaveLength(WIZARD_STEPS.length); + WIZARD_STEPS.slice(0, -1).forEach((_, i) => { + expect(STEP_FIELDS[i].length).toBeGreaterThan(0); + }); + expect(STEP_FIELDS[WIZARD_STEPS.length - 1]).toEqual([]); + }); + it('STEP_FIELDS[4] includes the hyphenated Secure_Check-in field name', () => { expect(STEP_FIELDS[4]).toContain('Secure_Check-in'); });