fix(registration-form): break post-purchase modal remount loop#144
Conversation
- ticketDataLoaded was overloaded as both "have we ever loaded?" milestone and "are we currently fetching?" transient. Renamed to hasTicketData; stays true once set, never resets on refetch. Promoted to true on .then (success only) instead of .finally. - Derived isAuthenticated/summitId/userId from props and used those as effect deps so effects fire on real auth/summit transitions, not on profileData/summitData object identity churn.
📝 WalkthroughWalkthroughThe registration-form component replaces ChangesRegistration Form State Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/registration-form/index.js (2)
205-205: ⚡ Quick winConsider memoizing
allowedTicketTypesto prevent unnecessary recalculations.Since
allowedTicketTypesis computed on every render and used as a dependency in theuseMemocalls on lines 207-208, those memos will recalculate on every render because the array reference changes each time. Wrapping this in auseMemowould stabilize the reference and improve performance.♻️ Proposed fix to memoize allowedTicketTypes
-const allowedTicketTypes = hasTicketData ? ticketTypes.filter((tt) => tt.sub_type === TICKET_TYPE_SUBTYPE_PREPAID || (tt.sales_start_date === null && tt.sales_end_date === null) || (nowUtc >= tt.sales_start_date && nowUtc <= tt.sales_end_date)) : []; +const allowedTicketTypes = useMemo(() => + hasTicketData ? ticketTypes.filter((tt) => tt.sub_type === TICKET_TYPE_SUBTYPE_PREPAID || (tt.sales_start_date === null && tt.sales_end_date === null) || (nowUtc >= tt.sales_start_date && nowUtc <= tt.sales_end_date)) : [], + [hasTicketData, ticketTypes, nowUtc] +);🤖 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/registration-form/index.js` at line 205, allowedTicketTypes is recomputed on every render which destabilizes its reference and forces downstream useMemo hooks to recompute; wrap the calculation in a useMemo so the array reference is stable. Replace the inline filter with: const allowedTicketTypes = useMemo(() => hasTicketData ? ticketTypes.filter(tt => tt.sub_type === TICKET_TYPE_SUBTYPE_PREPAID || (tt.sales_start_date === null && tt.sales_end_date === null) || (nowUtc >= tt.sales_start_date && nowUtc <= tt.sales_end_date)) : [], [hasTicketData, ticketTypes, nowUtc, TICKET_TYPE_SUBTYPE_PREPAID]); this keeps the same logic but memoizes the result for use by the existing useMemo calls.
16-16: Reframe React 16.8.4 concern to standard Hooks correctness (and optionally plan an upgrade)There’s no widely documented, React 16.8.4–specific bug in
useEffectdependency handling oruseStateupdate semantics. The real risk in this hooks-heavy refactor is the usual ones: correct dependency arrays to avoid stale closures, stable dependency identities to prevent unnecessary re-renders/loops, and awareness that effects run after render and state updates are batched (with re-renders skipped when the next state isObject.is-equal). Consider upgrading React from 16.8.x when practical.🤖 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/registration-form/index.js` at line 16, The review comment should be reframed from claiming a React 16.8.4-specific bug to calling out general Hooks correctness: update the PR note to instruct reviewers/authors to audit dependency arrays and stable identities for hooks used in this file (useEffect, useCallback, useMemo, useState) — specifically check any closures in the RegistrationForm component (or similarly named component/function) for stale references, ensure functions passed as dependencies are memoized (e.g., via useCallback), objects/arrays passed as deps are stable or memoized (useMemo), and that effects expect they run after render; optionally recommend planning a React upgrade from 16.8.x in the project roadmap.
🤖 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.
Nitpick comments:
In `@src/components/registration-form/index.js`:
- Line 205: allowedTicketTypes is recomputed on every render which destabilizes
its reference and forces downstream useMemo hooks to recompute; wrap the
calculation in a useMemo so the array reference is stable. Replace the inline
filter with: const allowedTicketTypes = useMemo(() => hasTicketData ?
ticketTypes.filter(tt => tt.sub_type === TICKET_TYPE_SUBTYPE_PREPAID ||
(tt.sales_start_date === null && tt.sales_end_date === null) || (nowUtc >=
tt.sales_start_date && nowUtc <= tt.sales_end_date)) : [], [hasTicketData,
ticketTypes, nowUtc, TICKET_TYPE_SUBTYPE_PREPAID]); this keeps the same logic
but memoizes the result for use by the existing useMemo calls.
- Line 16: The review comment should be reframed from claiming a React
16.8.4-specific bug to calling out general Hooks correctness: update the PR note
to instruct reviewers/authors to audit dependency arrays and stable identities
for hooks used in this file (useEffect, useCallback, useMemo, useState) —
specifically check any closures in the RegistrationForm component (or similarly
named component/function) for stale references, ensure functions passed as
dependencies are memoized (e.g., via useCallback), objects/arrays passed as deps
are stable or memoized (useMemo), and that effects expect they run after render;
optionally recommend planning a React upgrade from 16.8.x in the project
roadmap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5615b258-6435-47db-80a8-8032245b4a0c
📒 Files selected for processing (1)
src/components/registration-form/index.js
|
|
Tip For best results, initiate chat on the files or code changes.
(ᵔᴥᵔ) 🐇 ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
@gcutrini we are missing tests here
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/registration-form/__tests__/registration-form.test.js (2)
312-313: ⚡ Quick winStabilize async assertions by waiting on observable outcomes, not empty
actticks.Using
await act(async () => {});is timing-sensitive here and can make call-count assertions flaky. PreferwaitForaround the specific expectation after render/rerender.Suggested change
-import { cleanup, render, act } from '`@testing-library/react`'; +import { cleanup, render, act, waitFor } from '`@testing-library/react`'; ... - await act(async () => {}); - - expect(mockGetTicketTypesAndTaxes).toHaveBeenCalledTimes(1); + await waitFor(() => { + expect(mockGetTicketTypesAndTaxes).toHaveBeenCalledTimes(1); + }); ... - await act(async () => {}); - - expect(mockGetTicketTypesAndTaxes).not.toHaveBeenCalled(); + await waitFor(() => { + expect(mockGetTicketTypesAndTaxes).not.toHaveBeenCalled(); + });Also applies to: 329-330, 351-352, 369-370, 391-392, 408-409
🤖 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/registration-form/__tests__/registration-form.test.js` around lines 312 - 313, The tests use empty ticks like await act(async () => {}); which is flaky; replace each of those (occurrences at the empty act calls) with await waitFor(() => <the specific assertion>) from `@testing-library/react` so you wait on observable outcomes (e.g., await waitFor(() => expect(mockFn).toHaveBeenCalledTimes(n)) or await waitFor(() => expect(screen.getByText('...')).toBeInTheDocument())) instead of an empty act; update imports to include waitFor if needed and apply this change to all empty-act instances mentioned (the ones around lines 312-313, 329-330, 351-352, 369-370, 391-392, 408-409).
295-414: ⚡ Quick winRegression coverage is still missing milestone/error-path guards requested for this fix.
These tests cover object-reference churn and summit-id changes well, but they don’t yet assert: (1)
hasTicketDatastays true after first success and is not reset on subsequent refetches, and (2) rejected ticket-data fetch setsticketDataErrorwhile leavinghasTicketDatafalse.🤖 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/registration-form/__tests__/registration-form.test.js` around lines 295 - 414, Add two tests inside the RegistrationForm test suite to cover the missing milestone/error-path guards: (1) after a successful ticket-types fetch via mockGetTicketTypesAndTaxes verify the component/store flag hasTicketData remains true after subsequent refetches (e.g., initial render, clear mocks, force a refetch by changing summitData or profileData, then assert hasTicketData is still true and ticketDataError is falsy); (2) simulate a rejected ticket-types fetch by making mockGetTicketTypesAndTaxes return a rejected promise on the next call and assert that ticketDataError is set (non-null) and hasTicketData stays/returns false; locate assertions against the component state or Redux slice the RegistrationForm uses (look for hasTicketData and ticketDataError reads) and use the existing mock helpers (mockGetTicketTypesAndTaxes) and rerender pattern to trigger and verify both success and error paths.
🤖 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.
Nitpick comments:
In `@src/components/registration-form/__tests__/registration-form.test.js`:
- Around line 312-313: The tests use empty ticks like await act(async () => {});
which is flaky; replace each of those (occurrences at the empty act calls) with
await waitFor(() => <the specific assertion>) from `@testing-library/react` so you
wait on observable outcomes (e.g., await waitFor(() =>
expect(mockFn).toHaveBeenCalledTimes(n)) or await waitFor(() =>
expect(screen.getByText('...')).toBeInTheDocument())) instead of an empty act;
update imports to include waitFor if needed and apply this change to all
empty-act instances mentioned (the ones around lines 312-313, 329-330, 351-352,
369-370, 391-392, 408-409).
- Around line 295-414: Add two tests inside the RegistrationForm test suite to
cover the missing milestone/error-path guards: (1) after a successful
ticket-types fetch via mockGetTicketTypesAndTaxes verify the component/store
flag hasTicketData remains true after subsequent refetches (e.g., initial
render, clear mocks, force a refetch by changing summitData or profileData, then
assert hasTicketData is still true and ticketDataError is falsy); (2) simulate a
rejected ticket-types fetch by making mockGetTicketTypesAndTaxes return a
rejected promise on the next call and assert that ticketDataError is set
(non-null) and hasTicketData stays/returns false; locate assertions against the
component state or Redux slice the RegistrationForm uses (look for hasTicketData
and ticketDataError reads) and use the existing mock helpers
(mockGetTicketTypesAndTaxes) and rerender pattern to trigger and verify both
success and error paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1323d82e-2371-4287-ba34-c77d52c06ef4
📒 Files selected for processing (2)
src/components/registration-form/__tests__/registration-form.test.jssrc/components/registration-form/index.js
2 participants
ref: https://app.clickup.com/t/86ba2ryp0
Summary
Post-payment, the modal entered an infinite
<PurchaseComplete>remount cycle (nonstop reloads, repeatedmembers/me,shareable-link,promo-codes/discover,ticket-types/allowed). Standalone /register page unaffected: it redirects out on payment success.Root cause
Two compounding issues:
Effects dep'd on
profileData/summitDataobject references. Consumers mint newidpProfilerefs on every dispatch (including no-op), so effects refired without any real change.ticketDataLoadedwas used as both "have we loaded?" (milestone) and "are we fetching?" (transient). Resetting it to false on every refetch made the first-load null-render guard blank the whole form subtree mid-flow, unmounting<PurchaseComplete>and re-firing its mount-effect (onPurchaseComplete→getUserProfile→ newidpProfileref → loop).Changes
ticketDataLoaded→hasTicketData. Milestone-only: false initially, true after first successful fetch, never resets.handleGetTicketTypesAndTaxesno longer resets the milestone; promotes it to true only on success (.then, not.finally).isAuthenticated,summitId,userIdlocally. Effects now dep on those primitives, not object refs.useMemodeps and the early-return guard to match.Summary by CodeRabbit