feat(mrf-cutover): default CreateFormModal to MRF with storage-mode escape hatch (4/6)

[kevin9foong]

Stacked PR 4/6 for MRF cutover. Base: refactor/modal-cutover-prep.

Problem

Per the PRD (issue #9454): the Create Form modal currently defaults to storage mode and offers it side-by-side with MRF, which slows org-wide MRF migration and leaves admins on the legacy v1 webhook schema by default. Closes #9453.

Solution

Under the mrf-cutover flag:

• Default CreateFormModal to Multirespondent — no mode toggle on the first screen.
• New CreateFormStorageModeScreen reachable via the escape-hatch link ("old version of FormSG"), composed from the escapeHatchCopy foundation in PR 1. Title input + create button, with a "back" affordance to return to the MRF default screen.
• Match Figma fidelity: hide the data-classification infobox in the cutover flow, use 2.5rem spacing.
• Fix sliding-animation bug + undefined email crash that surfaced during escape-hatch navigation.
• Storybook stories for the three escape-hatch copy variants (payments-only, +children, +webhook v1).

When the flag is off, the original modal renders unchanged.

Breaking Changes

No - gated behind mrf-cutover feature flag. Backend createForm still accepts Encrypt without flag checks (see docs/adr/0001-frontend-only-storage-form-gating.md).

Tests

TC1: flag on, default MRF flow

• Enable mrf-cutover flag; open Create Form
• Confirm MRF default screen renders (no mode toggle, no data-classification infobox)
• Submit a title → confirm an MRF is created

TC2: escape hatch → storage mode

• On the MRF default screen, click "old version of FormSG"
• Confirm CreateFormStorageModeScreen renders with title input + create button
• Submit → confirm a storage-mode form is created
• From the storage screen, click "back" → confirm return to MRF screen without state loss

TC3: escape-hatch copy by beta flag

• User with no extra flags → copy mentions payments only
• User with betaFlags.children → copy mentions payments + children fields
• User with betaFlags.createStorageModeForV1Webhook → copy mentions payments + webhooks v1

TC4: flag off

• Disable mrf-cutover flag; open Create Form
• Confirm original modal renders unchanged

[kevin9foong]

🤖 AI code review

Reviewed git diff origin/develop...HEAD (target develop) across four axes — Standards, Spec, Architecture, Divergent — each verified against false positives. 4 inline comments posted; all non-blocking.

Dropped during verification (for transparency):

• MRF default leaking into the Duplicate Form modal — the responseMode: Multirespondent injection lives in the shared useCommonFormWizardProvider, but that line is already on develop and isn't part of this PR's diff, so it's out of scope here (the dupe modal is #9454).
• -1 animation-direction hack in goToStorageModeDetails — acceptable: correctly breadcrumbed TODO [MRF-CUTOVER], and the real slide-animation fix is out of scope for code the cutover deletes.
• Escape-hatch copy hardcoded in escapeHatchCopy.ts — out of this diff (introduced by the merged #9451).

🤖 This review was generated by an AI code review. Please verify before acting on it.

[kevin9foong]

issue (non-blocking): new user-facing copy here is hardcoded English instead of going through t().

What's happening: the back aria-label="Back" (line 54), the header Set up a Storage mode form (line 59), and the body text Storage mode is outdated… (line 66) are inline strings — but the same screen already uses t('features.workspace.modals.forms.create…') for the title label and the create button.

Why it bites later: these strings won't appear in the ms / ta / zh locales, and copy now lives in two different conventions within one file.

Suggestion: route all three through t() with new keys, like the sibling screens. SaveSecretKeyContent.tsx even wraps its aria-labels in t().

Confidence: 95 — verified; flagged independently by the Standards, Spec, Architecture and Divergent axes.

🤖 This comment was generated by an AI code review. Please verify before acting on it.

[kevin9foong]

Ignored for now since we do not wish to support i18n since deprecation message is going to be temporary.

[kevin9foong]

suggestion (non-blocking): extract the duplicated title input rather than copy-pasting it.

What's happening: FORM_TITLE_LENGTH_WARNING = 65 (line 27) and the whole title FormControl block (label, Skeleton, Input, validation, length-warning, ~lines 71–90) are copied near-verbatim from CreateFormDetailsScreen.tsx (constant at line 35, block at lines 91–115).

Why it bites later: the two copies drift — a change to the warning threshold or the validation has to be made in both, and this cutover screen is temporary so divergence is likely.

Suggestion: pull the shared part into a small <FormTitleInput/> (both screens already share useFormTitleValidationRules and one formMethods), or at minimum export the constant.

Confidence: 82 — verified; raised by the Standards and Divergent axes.

🤖 This comment was generated by an AI code review. Please verify before acting on it.

[kevin9foong]

accepted. clarified with kenneth (pm) that we probably will keep this setup modal in the near/mid term, so ideally want to ensure this component is well implemented without dups. this will also accelerate AI coding in the future.

[kevin9foong]

fixed, moved it into its own reusable component

[kevin9foong]

suggestion (non-blocking): set the cutover email default at the form's defaultValues, not inside the submit handler.

What's happening: emails: emails ? emails.filter(Boolean) : cutoverDefaultEmails guards a field typed non-optional string[] (CreateFormWizardContext.tsx:26), but the new storage screen never registers emails, so at runtime it is undefined — the type and the runtime disagree. The [user.email] default itself is correct (it matches the legacy EmailFormRecipientsInput default).

Suggestion — pick one:

• seed defaultValues.emails = user?.email ? [user.email] : [] when isMrfCutoverEnabled, next to the existing responseMode default, so the create handler stays honest with its type; or
• narrow the type to emails?: string[].

Confidence: 75 — verified type/runtime mismatch; below the correctness gate's 80 bar, kept as Architecture / Divergent design feedback.

🤖 This comment was generated by an AI code review. Please verify before acting on it.

[kevin9foong]

emails is indeed set as must be defined - it is a type violation for me to do a check for email definition here. will apply a fix to this and subsequent PRs.

[kevin9foong]

lets do option 2, following the mental model “We might not have emails in form state; fix it when we send.”

this keeps the same decision made in this PR but resolves the type violation

[kevin9foong]

fixed!

[kevin9foong]

issue (non-blocking): the storage-mode page itself has no Storybook story / Chromatic baseline.

What's happening: the added stories (MrfCutoverOn + the three beta-flag variants) all render only the MRF default screen. None has a play / userEvent step that clicks the escape-hatch link through to CreateFormStorageModeScreen, so that screen is never captured.

Why it matters: issue #9453 asks for stories covering "escape-hatch open / storage-mode page", and without one Chromatic won't baseline the new screen.

Suggestion: add one story with a play function that clicks the escape-hatch link and asserts the storage-mode page renders.

Confidence: 75 — verified unmet acceptance criterion; scored just under the Spec gate's 80 bar, included here as a follow-up.

🤖 This comment was generated by an AI code review. Please verify before acting on it.

[kevin9foong]

lets skip this for now, it is a good to have, but not strictly necessary.

[kevin9foong]

Code Review: PR #9468 — refactor(mrf-cutover): lift wizard navigation + pass source as prop (2/6)

Author: kevin9foong · Base: develop ← refactor/modal-cutover-prep · +235 / −56 · 15 files · stacked PR 2/6

Overview

Scaffolding for the upcoming MRF cutover. Three concrete changes:

1. Lifts MRF-cutover navigation (goToStorageModeDetails / goToMrfDetails, new StorageModeDetails flow state, isMrfCutoverEnabled flag) into the shared useCommonFormWizardProvider, threaded through all three modals (Create / Duplicate / Use-Template).
2. Decouples DuplicateFormModal from React context — source (form id + workspace id) now passed as explicit props instead of read from WorkspaceRowsContext / WorkspaceContext, so it can be mounted outside the workspace tree (e.g. from settings in PR 3/6).
3. Adds two flags — mrf-cutover growthbook flag (defined, not activated) and createStorageModeForV1Webhook user beta flag (schema + zod type + test), plus a new composeEscapeHatchCopy util.

The refactor (#1, #2) is clean and the behavioral claim holds: with mrf-cutover off, defaults and navigation are unchanged.

Strengths

• Prop-drilling over context is the right call — DupeFormWizardSource makes the modal's dependencies explicit and testable, and the test file is correctly simplified (drops WorkspaceContext / WorkspaceRowsContext mocks, passes formIdToDuplicate directly).
• The default-value override is correctly gated and spread-ordered so the flag wins only when on.
• Good test coverage on the genuinely-logic-bearing piece (composeEscapeHatchCopy) including the 1/2/3-reason Oxford-comma branches and the falsy-flag case. Backend persistence test for the new beta flag is appropriate.
• The whitespace-only cleanup of the Chakra RemoveScroll HACK comment is harmless.

Issues & Suggestions

1. Dead code in this PR (confirmed by grep). None of composeEscapeHatchCopy, StorageModeDetails, goToStorageModeDetails, or goToMrfDetails is consumed by any UI in this diff — they're inert until later PRs. That's defensible for stacked scaffolding, but two consequences worth deciding on:

• If mrf-cutover were ever flipped on before PR 3+ lands, goToStorageModeDetails sets currentStep to StorageModeDetails, for which no screen renders → blank wizard step. Since the flag is off this is latent, but it's a footgun in a stacked series. Consider a code comment noting the screen arrives in a later PR.
• Verify your lint/CI doesn't fail on unused exports.

2. composeEscapeHatchCopy bypasses i18n. Every other string in these modals goes through t(...), but this util hardcodes English ('Need payments? Use the ', 'old version of FormSG'). If this copy is user-facing it should likely be translatable, or at least follow the project's i18n convention. Flag for intent — acceptable if MRF cutover is deliberately SG-English-only and temporary.

3. formIdToDuplicate: FormId | undefined is required-but-nullable. Making the prop required (not ?:) while allowing undefined is a slightly awkward contract — every caller must pass it explicitly, but it can still be empty. That's intentional (forces callers to think about source), so fine, but consider documenting why it's | undefined rather than optional.

4. formId as FormId cast in PreviewFormBanner. formId comes from usePublicFormContext() (a string). The cast is reasonable for a branded id type, but if the context can yield an empty/undefined formId, the cast masks it. Low risk given usePreviewForm is gated on !!sourceFormId downstream.

Risk Assessment

Low. The behavior-changing path is feature-flagged off, the context→prop migration preserves the existing isDefaultWorkspace ? undefined : activeWorkspace._id logic (just relocated to WorkspaceRowsProvider), and tests were updated to match. Main residual risk is the latent blank-screen if the flag is enabled out of order — mitigated by the stacked-PR sequencing.

Verdict

Approve with minor comments. Solid, well-scoped refactor that does what the description says. I'd want answers on (2) i18n intent and a comment addressing (1) the order-of-activation footgun before merge, but neither blocks. The TC1 manual checklist (flag-off modal flows) is still unchecked — confirm that's been run.

made with claude's /review skill