MPDX-9943 - Offer a guaranteed None geographic option instead of clearing by wjames111 · Pull Request #1991 · CruGlobal/mpdx-react

wjames111 · 2026-08-19T15:47:51Z

Description

Supersedes #1987 (closed), reimplemented per review feedback there: instead of deferring clear-to-null saves to blur, the geographic autocompletes are now non-clearable and "None" is a guaranteed option — much less code for the same user-facing outcome.

Backspacing a geographic autocomplete to empty fired an extra geographicLocation: null mutation mid-typing (MUI clears the value the moment the input empties) and reset the visible field to "None" while the user was still typing.
goalGeographicConstantMap now seeds a leading None (0 multiplier) option, so all five tools' dropdowns offer it even though the live 2026 dataset lost its None row (being restored by hand via the admin UI — the ActiveAdmin page is the canonical way to manage these constants going forward).
The PDS, MPD, and Salary Calculator autocompletes are disableClearable: an unset location displays as "None", explicitly selecting "None" is the single way to unset, and emptying the input just reverts on blur without saving anything.
The NSO questionnaire's geographic question is now optional with a "None" default (validation + Ministry-step completion), since an unanswered question displays and calculates as "None". The select's placeholder row is hidden when a default is shown.
Helper copy that instructed users to select "None" is replaced ("leave it as None" / explanatory text), since the default is automatic.
null and 'None' remain equivalent everywhere (client map lookups and the server's multiplier_for! both resolve to a ×1.0 multiplier), so legacy null values are unaffected.

Related Jira ticket: MPDX-9943

Testing

Go to hrTools/pdsGoalCalculator, open a goal, and go to the Setup step
Check the Geographic Multiplier shows "None" when nothing is saved, and that "None" appears in the dropdown
Set it to a location (e.g. Orlando, FL), then backspace the field to empty — check no mutation fires (network tab) and the field stays empty while focused
Click out — check the field reverts to Orlando, FL with no mutation; there is no clear (X) button
Select "None" from the dropdown — check a single mutation fires with geographicLocation: "None"
Repeat on the MPD Goal Calculator (Information → Geographic Location) and Salary Calculator (Personal Information)
In the NSO questionnaire, check the city question defaults to "None", is not required, and the Ministry Information step completes without answering it

Checklist:

I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
I have applied the appropriate labels (Add the label "Preview" to automatically create a preview environment)
I have run the Claude Code /quality:agent-review command locally and fixed any relevant suggestions
I have requested a review from another person on the project
I have tested my changes in preview or in staging
I have cleaned up my commit history

🤖 Generated with Claude Code

Backspacing a geographic autocomplete to empty fired an extra mutation mid-typing because MUI clears the value the moment the input empties, resetting the field to None while the user was still typing. - Guarantee a leading 'None' (0 multiplier) option in goalGeographicConstantMap so every geographic dropdown offers it even when a curated year dataset omits the row (the 2026 data does) - Make the PDS, MPD, and Salary Calculator fields non-clearable: an unset location displays as None, selecting None is the one way to unset, and emptying the input reverts on blur without saving - Make the NSO geographic question optional with a None default: drop its required validation and Ministry-step completion gating, since an unanswered question displays and calculates as None; hide the select's placeholder row when a default is shown - Replace helper copy that instructed users to select None MPDX-9943 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

github-actions · 2026-08-19T15:51:36Z

Bundle sizes [mpdx-react]

Compared against 8b6c94e

No significant changes found

wjames111

Multi-Agent Code Review (standard mode)

Verdict: APPROVED WITH SUGGESTIONS — no blockers. 5 specialized agents + a fresh gap-review agent + dependency analysis, with cross-examination debate and rebuttal rounds.

Key verified result: the Financial Reporting agent traced every consumer of goalGeographicConstantMap and confirmed no goal-calculation result changes for any existing saved data (null, saved city, or legacy 'None') — the only money-math lookup is pdsGoalConstants.ts:47 and it is invariant under this change. Dependency analysis found no breaking changes; NS FinancialInformationSection (unchanged) correctly gains the None option its new initial value requires.

Findings (post-debate severities; 7 medium-priority items posted as line comments, none blocking):

[6.5] NS mapping is the one surface where the None display fallback becomes persisted data (untested write direction)
[6.5] MPD lacks a positive "selecting None saves" test (PDS/NSO have one)
[6.0] The byte-match-critical 'None' literal is hardcoded at 6 sites — export one constant
[6.0] Salary lacks a positive-save test; AutosaveAutocomplete's clearable branch is untested/unused
[5.5] Degraded constants states (loading/error/unavailable) render an enabled one-option 'None' dropdown — gate like the ministries dropdown
[5.5] PDS/MPD helper text lost the "leave it as None" guidance that Salary/NSO kept
[5.5→anchored] SelectQuestion JSDoc claims that are stale/false (MUI suppresses same-value selection — confirmed in vendored 7.3.11 source)

Notable debate outcomes: Testing's 7.0 on the NS write path was revised to 5.5 after Financial proved the server accepts arbitrary location strings (the feared validation-rejection mode doesn't exist); UX's "data-loss path" was recalibrated to 5.5 "fail-deceptive degraded state" (the pre-PR clearable-X was a worse loss path); one finding (SelectQuestion || vs ??) was withdrawn — useAutoSave stringifies all values, and ?? would be a regression.

Findings on Related Files (Not in This PR)

[Suggestion] src/components/HrTools/NsoMpdQuestionnaire/Summary/useSummarySections.ts:138 — With the geographic question optional, the Summary review screen renders an unanswered city as error-red italic "No value provided" (blanket required: true row mapping) while the form shows "None" — and because MUI suppresses same-value selection, the user cannot repair the row from the form. Severity 4.0; fix at the required-flag mapping plus a product decision on the display text ("None" vs "No value provided"). Flagged by Gap Review, confirmed by 4 agents.

Risk: 10/10 (breadth-driven — 9 medium-risk feature files; no critical file classes touched). Suggestions below severity 5 are informational. Full agent transcripts available on request.

wjames111 · 2026-08-19T16:32:33Z

  advocacyTransfers: toNumberInput(calc.advocacyTransfers),
-  geographicLocation: calc.geographicLocation ?? '',
+  // An unset location defaults to the 'None' (0 multiplier) constant option
+  geographicLocation: calc.geographicLocation ?? 'None',


[Medium] NS is the only surface where the 'None' display fallback becomes persisted data: this read-side default flows through the unchanged write path (`values.geographicLocation || null`), so any submit converts stored null → 'None' — violating the "the fallback itself saves nothing" contract the PR's own emptyValue docstrings state. Debate verified there is no math or server-validation impact, so this is a decide-and-test item: either convert 'None' back to null on write (matching the other four tools) or document the normalization — and pin it with a round-trip test (the suite's only write-direction test now asserts a UI-unreachable `'' → null` state). Flagged by 4 agents.

wjames111 · 2026-08-19T16:32:33Z

+  // dropdowns are not clearable. Seed it first so it always exists even when a
+  // curated year dataset omits it; a server-provided 'None' row overwrites the
+  // value but keeps the position.
+  goalGeographicConstantMap.set('None', 0);


[Medium] The byte-match-critical `'None'` literal is now hardcoded at 6 production sites (this seed, PDS/MPD `?? 'None'`, NS mapping, two `emptyValue="None"` props). Repo precedent (`ALL_TEAMS`, `REIMBURSABLE_FLOOR`, `PREVIEW_DEBOUNCE_MS`) favors exporting one constant, e.g. `export const GEOGRAPHIC_LOCATION_NONE = 'None';` — every consuming file already imports this hook, so it adds zero coupling, and a rename/typo in one site currently desynchronizes silently.

wjames111 · 2026-08-19T16:32:33Z

      );
    });

+    it('does not save while the Geographic Location is cleared by typing', async () => {


[Medium] All three new MPD geographic tests are negative assertions (`not.toHaveGraphqlOperation`). PDS and NSO each got a positive "saves None when it is explicitly selected" test; MPD did not — a regression in this exact onChange → saveField wiring would pass the suite. Consider mirroring the PDS test: seed `geographicLocation: 'Orlando, FL'`, select None, assert `UpdateGoalCalculation` fired with `geographicLocation: 'None'`.

wjames111 · 2026-08-19T16:32:33Z

+    expect(await findAllByRole('option')).toHaveLength(3);
+  });
+
+  it('does not save while the location is cleared by typing and reverts on blur', async () => {


[Medium] Same positive-path gap as MPD: no test that selecting a location (or None) fires `UpdateSalaryCalculation`. Also, `AutosaveAutocomplete`'s `emptyValue`-undefined branch (field stays clearable, clearing saves null) now has no test and no consumer — worth either a test or a decision that `emptyValue` becomes required.

wjames111 · 2026-08-19T16:32:33Z

              options={locations}
+              // The None option takes the place of clearing the field, so
+              // emptying the input never fires a mid-typing null save
+              disableClearable


[Medium] Degraded-state gating: while constants are loading, errored, or `unavailable`, the seeded map makes this dropdown render an enabled menu whose only entry is 'None' beneath the user's still-displayed saved city — inviting "my city was removed, I should pick None," which saves a 0-multiplier overwrite (and violates the hook's documented invariant that no constants-driven autosave runs while `unavailable`). Same on Salary and MPD; NSO gates `loading` but not `unavailable`. Suggested fix per the ministries-dropdown precedent already in this PR: `disabled={!calculation || constantsLoading || constantsUnavailable}` + an error helper text. Flagged by 3 agents (debated 6.5→5.5: no passive path exists, and the pre-PR clearable X was a worse loss path).

wjames111 · 2026-08-19T16:32:33Z

                  label={t('Geographic Multiplier')}
-                  helperText={t('If not applicable, select "None"')}
+                  helperText={t(
+                    'Do you live within 50 miles of one of these major cities?',


[Medium] The four tools now split 2/2 on telling users how to decline: Salary and NSO kept explicit "leave it as None" guidance, while PDS (this line) and MPD have none — exactly as the clear affordance was removed. Consider appending the guidance as a separate sentence/i18n key (e.g. 'If not, leave it as "None".') so the shared 50-miles question string stays identical between PDS and MPD.

wjames111 · 2026-08-19T16:32:33Z

+  /** Whether an answer is required. Defaults to true. */
+  required?: boolean;
+  /**
+   * Option displayed when the field has no saved value (e.g. 'None'). Must match one of the


[Medium] Two doc corrections: (1) this emptyValue doc's "Selecting it explicitly still saves it" is provably false from the fallback state — MUI suppresses same-value selection (verified in vendored 7.3.11: SelectInput.js guards `value !== newValue`), so from an unanswered field, clicking None saves nothing (harmless, but the contract overstates); (2) the component-level JSDoc above still says "A single **required** dropdown question" — stale now that `required={false}` is supported.

wjames111 · 2026-08-19T16:32:33Z

+      // With an emptyValue option there is nothing to clear to, and disabling
+      // clearing also stops MUI from firing a mid-typing null save when the
+      // input is emptied
+      disableClearable={emptyValue !== undefined}


[Suggestion] `{...props}` spreads after this line and the widened generic type-admits `disableClearable={false}` alongside `emptyValue` — a caller could silently resurrect the mid-typing null-save this PR fixes (invisible, since the display would still show the fallback). One-line hardening: add `'disableClearable'` to the interface's `Omit`.

wjames111 · 2026-08-19T16:32:33Z

 const stepRequiredFields: Record<string, QuestionnaireField[]> = {
  // Personal Information is a read-only review step, so it has no fields the user must fill in.
  [NsoMpdQuestionnaireStepEnum.PersonalInformation]: [],
+  // geographicLocation is intentionally absent: it is optional and an


[Suggestion] This comment says an unanswered question "displays … as None", but the Summary review screen disagrees: `useSummarySections.ts:138` feeds null through `formatText`, the blanket `required: true` row mapping renders it as error-red italic "No value provided" — and because MUI suppresses same-value selection, the user cannot make the row read "None" from the form. Fix belongs at the Summary's required-flag mapping (per-row override) plus a product decision on the display text. Found by the fresh gap-review agent; confirmed by 4 agents.

wjames111 · 2026-08-19T16:32:33Z

  });

+  it('does not save while the Geographic Multiplier is cleared by typing', async () => {
+    mutationSpy.mockClear();


[Suggestion] Test polish, batched (all informational): the 7 new `mutationSpy.mockClear()` first-statements here and in InformationCategory.test.tsx are dead code under jest `clearMocks: true`; the "microtask queue" comments are technically macrotask; the constants test never observably exercises server-None-overwrite (same value 0 — a 0.05 row asserting `['None', 0.05]` first would); and SelectQuestion's own suite doesn't assert `required={false}` or saved-value-wins-over-emptyValue.

wjames111 commented Aug 19, 2026

View reviewed changes

Navigation Menu

Uh oh!

MPDX-9943 - Offer a guaranteed None geographic option instead of clearing - #1991

MPDX-9943 - Offer a guaranteed None geographic option instead of clearing#1991
wjames111 wants to merge 1 commit into
mainfrom
MPDX-9943-geographic-none-option

wjames111 commented Aug 19, 2026

Uh oh!

github-actions Bot commented Aug 19, 2026

Uh oh!

wjames111 left a comment

Uh oh!

wjames111 Aug 19, 2026

Uh oh!

wjames111 Aug 19, 2026

Uh oh!

wjames111 Aug 19, 2026

Uh oh!

wjames111 Aug 19, 2026

Uh oh!

wjames111 Aug 19, 2026

Uh oh!

wjames111 Aug 19, 2026

Uh oh!

wjames111 Aug 19, 2026

Uh oh!

wjames111 Aug 19, 2026

Uh oh!

wjames111 Aug 19, 2026

Uh oh!

wjames111 Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

wjames111 commented Aug 19, 2026

Description

Testing

Checklist:

Uh oh!

github-actions Bot commented Aug 19, 2026

Bundle sizes [mpdx-react]

Uh oh!

wjames111 left a comment

Choose a reason for hiding this comment

Multi-Agent Code Review (standard mode)

Findings on Related Files (Not in This PR)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant