feat(promo-codes): bulk domain input Tier 1 (paste + modal + compact summary)#947
Merged
smarcet merged 12 commits intoMay 20, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds react-window, a ManageAllowedEmailDomainsModal, bulk-input parsing/classification (parseTextBlob/normalizeEntry/classifyEntries), a compact large-list UI in AllowedEmailDomainsRow, i18n strings, and unit/integration tests exercising virtualization, parsing, deduplication, and scroll-to-error behavior. ChangesVirtualized email domain list management
Sequence DiagramsequenceDiagram
participant User
participant AllowedEmailDomainsRow
participant ManageModal as ManageAllowedEmailDomainsModal
participant Parser as parseTextBlob/classifyEntries
participant List as FixedSizeList
User->>AllowedEmailDomainsRow: view domain list (>50 entries)
AllowedEmailDomainsRow->>AllowedEmailDomainsRow: render compact summary + Manage button
User->>AllowedEmailDomainsRow: click Manage button
AllowedEmailDomainsRow->>ManageModal: open(show=true, existing)
ManageModal->>ManageModal: snapshot existing → working
User->>ManageModal: paste text + Add or Cmd+Enter
ManageModal->>Parser: parse & classify entries
Parser->>ManageModal: return valid/invalid/dup counts
ManageModal->>ManageModal: append valid to working
ManageModal->>List: render virtualized entries
User->>ManageModal: click Done
ManageModal->>AllowedEmailDomainsRow: onApply(merged domains)
AllowedEmailDomainsRow->>AllowedEmailDomainsRow: update & fireChange
AllowedEmailDomainsRow->>User: update summary display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merged
4 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/forms/promocode-form/forms/domain-authorized/__tests__/bulk-input-parser.test.js (1)
32-36: ⚡ Quick winStrengthen the
sourceRowassertion to catch regressions.This currently only verifies
sourceRow > 0, so incorrect numbering can still pass. Assert exact row indices instead.♻️ Proposed test adjustment
it("preserves 1-based source row numbers", () => { const rows = parseTextBlob("`@a.com`\n@b.com\n@c.com"); - expect(rows[0].sourceRow).toBeGreaterThan(0); + expect(rows.map((r) => r.sourceRow)).toEqual([1, 2, 3]); expect(rows.map((r) => r.entry)).toEqual(["`@a.com`", "`@b.com`", "`@c.com`"]); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/forms/promocode-form/forms/domain-authorized/__tests__/bulk-input-parser.test.js` around lines 32 - 36, The test for parseTextBlob should assert exact 1-based sourceRow values instead of only >0; update the "preserves 1-based source row numbers" test to verify rows.map(r => r.sourceRow) equals [1,2,3] (and keep the existing check that rows.map(r => r.entry) equals ["`@a.com`","`@b.com`","`@c.com`"]) so any off-by-one regressions in parseTextBlob are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/components/forms/promocode-form/forms/domain-authorized/AllowedEmailDomainsRow.jsx`:
- Around line 271-273: The onApply handler (the line calling
fireChange(handleChange, "allowed_email_domains", next)) updates parent state
but doesn't clear the component's local validation state, so previous
domainsError and draft can persist; after applying changes call the local
setters (e.g., setDomainsError(null) and setDraft(next) or an appropriate
empty/updated draft) to clear the inline error and sync the draft with the
applied value so the modal shows the new state without lingering errors.
---
Nitpick comments:
In
`@src/components/forms/promocode-form/forms/domain-authorized/__tests__/bulk-input-parser.test.js`:
- Around line 32-36: The test for parseTextBlob should assert exact 1-based
sourceRow values instead of only >0; update the "preserves 1-based source row
numbers" test to verify rows.map(r => r.sourceRow) equals [1,2,3] (and keep the
existing check that rows.map(r => r.entry) equals ["`@a.com`","`@b.com`","`@c.com`"])
so any off-by-one regressions in parseTextBlob are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53e675e2-3286-4ced-9595-595afeeb73c5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
package.jsonsrc/components/forms/promocode-form/__tests__/promocode-form.integration.test.jssrc/components/forms/promocode-form/forms/domain-authorized/AllowedEmailDomainsRow.jsxsrc/components/forms/promocode-form/forms/domain-authorized/ManageAllowedEmailDomainsModal.jsxsrc/components/forms/promocode-form/forms/domain-authorized/__tests__/allowed-email-domains-row.test.jsxsrc/components/forms/promocode-form/forms/domain-authorized/__tests__/bulk-input-parser.test.jssrc/components/forms/promocode-form/forms/domain-authorized/__tests__/manage-modal.test.jsxsrc/components/forms/promocode-form/forms/domain-authorized/bulk-input-parser.jssrc/i18n/en.json
Reconcile implementation with JP's pass-1 framing: Paste-to-add is a top-level Tier 1 capability, not nested behind compact summary. Lift the Manage List button from the compact-summary block to a sibling next to the field label so it renders at all counts (including length=0). Tier 1 read-only modal scope is unchanged. - AllowedEmailDomainsRow.jsx: wrap label + button in flex container, remove button from compact-summary branch, drop the now-unused inner space-between flex wrapper around the count/type-mix/example block. - i18n: rename edit_promocode.large_list.manage_button -> edit_promocode.manage_button (the large_list namespace lied once the button left the compact-summary branch); other large_list.* keys stay where they are. - Tests: flip 3 button-presence assertions; re-probe 50/51 boundary test via compact-summary-count (button is now threshold-invariant); new count=0 button-present test for the empty-form entry point. - yarn jest src/components/forms/promocode-form -> 113/113. SDS amendment in ftn-docsnsklz captures the reconciliation: sds/domain-authorized-promo-codes-bulk-input.md "2026-05-19 — Tier 1.1 Manage List button always-on (reconciliation with JP pass-1 framing)".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/components/forms/promocode-form/__tests__/promocode-form.integration.test.js`:
- Around line 554-557: Narrow the assertion to the specific
allowed_email_domains lookup by filtering getByIdSpy.mock.results for results
where the call argument equals "allowed_email_domains" and the result is a
successful return with a non-null value; replace the broad filter using only
r.type and r.value with one that checks the corresponding mock.calls or the
mock.results' associated args to ensure the getByIdSpy invocation for
"allowed_email_domains" returned a non-null value (use getByIdSpy to locate the
call for "allowed_email_domains" and assert that its return/result is not null).
In
`@src/components/forms/promocode-form/forms/domain-authorized/__tests__/allowed-email-domains-row.test.jsx`:
- Around line 506-509: The test currently filters handleChange.mock.calls then
asserts length 0 which can miss calls that passed unchanged arrays; replace that
logic by directly asserting that handleChange was never called (use
handleChange.mock and an assertion like
expect(handleChange).not.toHaveBeenCalled()) in the allowed-email-domains-row
test to ensure no duplicate-triggering calls occur; locate the existing usage of
handleChange and the variable calls in the test and remove the filtering-based
assertion, replacing it with a direct zero-call assertion on handleChange.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1de602b-00d8-488d-b2b0-7ff3388bc407
📒 Files selected for processing (4)
src/components/forms/promocode-form/__tests__/promocode-form.integration.test.jssrc/components/forms/promocode-form/forms/domain-authorized/AllowedEmailDomainsRow.jsxsrc/components/forms/promocode-form/forms/domain-authorized/__tests__/allowed-email-domains-row.test.jsxsrc/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/i18n/en.json
- src/components/forms/promocode-form/forms/domain-authorized/AllowedEmailDomainsRow.jsx
Comment on lines
+554
to
+557
| const calls = getByIdSpy.mock.results.filter( | ||
| (r) => r.type === "return" && r.value !== null | ||
| ); | ||
| expect(calls.length).toBeGreaterThan(0); |
There was a problem hiding this comment.
Bind spy result assertion to the allowed_email_domains lookup call.
The current non-null check can pass from any other getElementById call. Validate the return value for the specific "allowed_email_domains" invocation.
Suggested assertion fix
- const calls = getByIdSpy.mock.results.filter(
- (r) => r.type === "return" && r.value !== null
- );
- expect(calls.length).toBeGreaterThan(0);
+ const idx = getByIdSpy.mock.calls.findIndex(
+ ([id]) => id === "allowed_email_domains"
+ );
+ expect(idx).toBeGreaterThanOrEqual(0);
+ expect(getByIdSpy.mock.results[idx]?.value).not.toBeNull();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/forms/promocode-form/__tests__/promocode-form.integration.test.js`
around lines 554 - 557, Narrow the assertion to the specific
allowed_email_domains lookup by filtering getByIdSpy.mock.results for results
where the call argument equals "allowed_email_domains" and the result is a
successful return with a non-null value; replace the broad filter using only
r.type and r.value with one that checks the corresponding mock.calls or the
mock.results' associated args to ensure the getByIdSpy invocation for
"allowed_email_domains" returned a non-null value (use getByIdSpy to locate the
call for "allowed_email_domains" and assert that its return/result is not null).
Comment on lines
+506
to
+509
| const calls = handleChange.mock.calls.filter( | ||
| (c) => Array.isArray(c[0]?.target?.value) && c[0].target.value.length > 1 | ||
| ); | ||
| expect(calls).toHaveLength(0); |
There was a problem hiding this comment.
Strengthen dedup regression by asserting no handleChange call at all.
This check can pass even if duplicate commits still call handleChange with an unchanged array. Assert zero calls directly to avoid false positives.
Suggested test tightening
typeAndCommit(container, "`@ACME.COM`");
- const calls = handleChange.mock.calls.filter(
- (c) => Array.isArray(c[0]?.target?.value) && c[0].target.value.length > 1
- );
- expect(calls).toHaveLength(0);
+ expect(handleChange).not.toHaveBeenCalled();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const calls = handleChange.mock.calls.filter( | |
| (c) => Array.isArray(c[0]?.target?.value) && c[0].target.value.length > 1 | |
| ); | |
| expect(calls).toHaveLength(0); | |
| expect(handleChange).not.toHaveBeenCalled(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/forms/promocode-form/forms/domain-authorized/__tests__/allowed-email-domains-row.test.jsx`
around lines 506 - 509, The test currently filters handleChange.mock.calls then
asserts length 0 which can miss calls that passed unchanged arrays; replace that
logic by directly asserting that handleChange was never called (use
handleChange.mock and an assertion like
expect(handleChange).not.toHaveBeenCalled()) in the allowed-email-domains-row
test to ensure no duplicate-triggering calls occur; locate the existing usage of
handleChange and the variable calls in the test and remove the filtering-based
assertion, replacing it with a direct zero-call assertion on handleChange.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/86b9z4d3d
Summary
Ships Tier 1 + Tier 1.1 of the Domain-Authorized Promo Codes bulk-input frontend per SDS PR #15. Admins editing a
DOMAIN_AUTHORIZED_PROMO_CODEorDOMAIN_AUTHORIZED_DISCOUNT_CODEcan now paste 700–1,400allowed_email_domainsentries into a single modal without melting the chip wall — and the bulk-paste entry point is reachable from any list size, including a fresh empty form.ManageAllowedEmailDomainsModalopened from a Manage List button rendered next to the field label at all counts (Tier 1.1 — threshold-independent). Modal hosts a paste textarea + post-add toast tally ("Added X · Y invalid · Z duplicates") + read-only virtualized list (react-windowFixedSizeList) + Cancel/Done modal-scoped semantics.bulk-input-parser.jspure utility:parseTextBlob(newline/comma/tab/semicolon),normalizeEntry(preserves user-typed casing, auto-prefixes@for bare domains including single-char SLDs likeq.io),classifyEntries(case-insensitive dedup against existing + within input).AllowedEmailDomainsRowgrows a compact-summary branch above the 50-entry threshold (count badge + type-mix hint + example); chip wall is preserved byte-identical at ≤ 50. Tightens existing single-entry chip-input dedup to case-insensitive (Locked Decision 6).edit_promocode.manage_modal.*(7) andedit_promocode.large_list.*(3). Tier 1.1 addsedit_promocode.manage_button(renamed fromedit_promocode.large_list.manage_buttonsince the button is no longer compact-summary-specific). No Tier 2 keys.react-window@^1.8.10(~6 KB gzipped, lives behind Manage List static-import chain — see SDS Gotcha "react-window bundle cost").Tier 1.1 (commit
28ea322d) — reconciliation with JP pass-1 framing: field testing surfaced a UX cliff at ≤ 50 entries where the inline+ add onechip input was the only add path (and<input type="text">flattens pasted multi-line lists). JP's pass-1 SDS directive treats "Paste-to-add" as a top-level Tier 1 capability separate from "Compact summary above threshold" — they were meant to be parallel affordances, not nested. The Manage List button is lifted out of the compact-summary block to a sibling next to the field label, rendered at all counts. Tier 1 read-only modal scope is unchanged. SDS amendment in companion docs PR captures the reconciliation; Locked Decision #1 relaxed from "modal opened from Manage List above threshold" to "modal opened from Manage List, threshold-independent."Companion PR: OpenStackweb/summit-api#546 (Track 1 telemetry + lookup-build) — same prod push, week of 2026-05-19.
Out of scope (Tier 2, deferred): search input, type filter, per-row checkbox, Select All, Delete Selected. Per JP scope decision pass-3.
ClickUp: https://app.clickup.com/t/86b9z4d3d
Test plan
yarn test) — verified locally 607/607 passing; promocode-form bundle re-verified 2026-05-19 at 113/113 (post-Tier 1.1; +1 new assertion for empty-form button presence) in 1.6sDOMAIN_AUTHORIZED_*promo code edit form with < 50 entries — chip wall renders unchanged (regression check) — verified 2026-05-19 againstapi.dev.fnopen.comsummit 69length === 0— verified 2026-05-19 on a fresh new-promo-code form againstapi.dev.fnopen.comsummit 69; modal opens empty, paste → Add Domains → Done populates the chip wallallowed_email_domainsas the initial working copy — verified 2026-05-19allowed_email_domainsreflects working copy — verified 2026-05-19 via Save round-tripapi.dev.fnopen.com/api/v1/summits/69/promo-codes)expand=allowed_email_domains)@ACME.comwhen@acme.comis already present (case-insensitive dedup) — pinned by unit test__tests__/allowed-email-domains-row.test.jsx; UI path not exercised locallyLocal manual test pass — 2026-05-19
Exercised against
api.dev.fnopen.comsummit 69 fromlocalhost:8080(webpack-dev-server). Tier 1 verification at HEADe1566ac1then Tier 1.1 verification at HEAD28ea322d.Tier 1 (HEAD
e1566ac1)Threshold temporarily lowered to 3 to reach the modal flow without manually entering 51 chips; reverted to 50 before final jest run. Working tree clean post-revert.
Items verified beyond the Test plan checklist above:
@company{0..699}.example) into the Manage List modal via a DevTools setter snippet on the textarea, clicked Add Domains. Toast:Added 700 · skipped 0 invalid · 0 duplicates. DevTools Elements panel confirmedreact-windowvirtualization — DOM stayed at ~10 rendered row divs while scrolling 754 entries (saved 54 + soak 700), not 754. Smooth scroll. Cancel discarded the working copy as expected.e1566ac1verified end-to-end. With the inline error showing (typedgarbageinto+ add one→ Enter → red "Each entry must be an exact domain..." text +garbageretained in input per commit() early-return), opening the modal and clicking Done without modifications cleared both the localdomainsError(red text) anddraft(inline input value). Pre-fix path would have left both stale./api/v1/summits/69/promo-codesreturned 200 with the new entity, frontend immediately re-fetched via GET?expand=...allowed_email_domains, page re-rendered the compact summary with54 domains configured+ correct type mix + first-entry example.Tier 1.1 (HEAD
28ea322d)Manage List button now always-on. Codex second-pass review completed before commit (Major: clean; Minor: stale ref in untracked plan file, not committed; Nit: SDS line-number drift fixed).
btn-xsnext to the field label, no longer inside the compact summary block. Click → modal opens with all 54 entries in the virtualized list./promocodes/newform, Class NameDOMAIN_AUTHORIZED_PROMO_CODE, codeTEST-DA-EMPTY, no domains entered. Manage List button visible. Click → modal opens empty. Paste 3 domains → Add Domains → Done → 3 chips appear in the parent chip wall. Save → POST round-trip green, navigates to the new code's edit page.edit_promocode.large_list.manage_button→edit_promocode.manage_buttonresolved in both branches.Real-world failure mode worth noting (legacy, not in scope for Tier 1): the validator passes
@three.cmo-style typos because it only requires@...shape with no real TLD check. The Tier 1 modal is read-only, so a customer pasting a 700-domain list can't spot/fix these from inside the modal — they have to drop back to chip-wall mode (≤ 50 entries) to remove. Filed under "Tier 2 search/filter would surface this."Pre-prod follow-ups status:
summit-api#546T1-Sanity measurement still owed (needs staging deploy of fix: extra questions read only conditions #546).Manual staging soak (REQUIRED before prod merge)
Per SDS Rollout Plan:
DOMAIN_AUTHORIZED_*code on stagingtribal-knowledge/entries in the fn-skills vaultSummary by CodeRabbit