Skip to content

feat: add promo code discovery with usePromoCode hook#141

Merged
smarcet merged 44 commits into
mainfrom
feature/promo-code-discovery
May 18, 2026
Merged

feat: add promo code discovery with usePromoCode hook#141
smarcet merged 44 commits into
mainfrom
feature/promo-code-discovery

Conversation

@gcutrini
Copy link
Copy Markdown
Member

@gcutrini gcutrini commented Apr 23, 2026

ref: https://app.clickup.com/t/86b9jq8hd

Summary

Adds promo code discovery to the registration widget and the surrounding UX/state machine that supports it.

  • Discovery + auto-apply: usePromoCode hook integrates the /promo-codes/all/discover endpoint, suggests qualifying codes, and auto-applies a single qualifying code when configured. Handles the WithPromoCode-only flow (apply a code to reveal eligible tickets) via an early-auto-apply effect.
  • Status-driven UI: PROMO_STATUS enum (IDLE / SUGGESTED / APPLYING / VALIDATING / VALID / INVALID) drives all promo input rendering (label, icon, action). Invalid codes applied without a ticket are detected and surfaced as an error instead of looking applied.
  • No-tickets UX: when the API returns no tickets, the ticket selector is hidden and a clear notice is shown. The notice wording is configurable via a new noTicketsAvailableMessage prop (intended to be wired to a marketing setting on the host).
  • Host-site compatibility fixes: applied promo input styles use !important and CSS variable fallbacks so the disabled state renders correctly on host sites whose globals would otherwise win on cascade.
  • Misc fixes: 412 Sentry noise suppression on schedule add/remove, null-company crash on personal-info, ticket dropdown clearing on empty results, quantity clamping when max changes, multiple-codes auto-apply suppression, and tests + e2e coverage.

The hook is the central state machine; consumer components only read status and call its callbacks.

Integrates the discovery endpoint to find qualifying promo codes
for logged-in users. New usePromoCode hook manages discovery
lifecycle (suggestion, auto-apply, ticket qualification) and
provides a status enum that drives all promo code UI rendering.

- Discovery action calls /promo-codes/all/discover on auth
- Hook owns status, validation errors, and action orchestration
- PromoCodeInput derives its own display from status
- No background validation: client-side ticket check + validate on apply
- Per-account limit caps stepper and shows notice
37 tests covering status derivation, discovery selection,
ticket qualification, auto-apply, suggestion lifecycle,
apply/remove/input actions, and error handling.
21 tests covering discovery scenarios, suggestion flow, auto-apply,
per-account limits, validation errors, ticket switching, and API
error resilience. Run with npx playwright test (--headed for browser,
--debug for step-through).
@gcutrini gcutrini requested a review from smarcet April 23, 2026 12:08
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds promo-code discovery (backend thunk + reducer), a new usePromoCode hook with status-driven apply/suggest/validate flows, refactors registration/ticket/promo UI to consume the hook, updates helpers/constants/i18n, adds unit tests for the hook, Playwright E2E tests/fixtures, Playwright config, and CI/test tooling updates.

Changes

Promo feature (hook, Redux, components, helpers, i18n, tests)

Layer / File(s) Summary
Data / Constants
src/utils/constants.js
Adds PROMO_STATUS enum for promo lifecycle states.
Redux Action
src/actions.js
Adds DISCOVER_PROMO_CODES / DISCOVER_PROMO_CODES_SUCCESS and discoverPromoCodes(summitId) thunk that fetches discovered promo codes using an access token.
Reducer state
src/reducer.js
Adds discoveredPromoCodes to default state and handles DISCOVER_PROMO_CODES_SUCCESS; resets on LOAD_INITIAL_VARS, CLEAR_RESERVATION, PAY_RESERVATION.
Hook (core logic)
src/hooks/usePromoCode.js
New exported usePromoCode hook: selects discovered promo (prefers auto_apply), computes status and derived outputs (suggestedCode, perAccountLimit, maxQuantityFromPromo, isReady), and exposes handlers onTicketSelected, onApply, onRemove, onRevalidate, onInputChange, plus setValidationError. Adds ticketDataLoaded/hasTickets options.
Hook tests
src/hooks/__tests__/usePromoCode.test.js
Comprehensive unit tests for discovery selection, all PROMO_STATUS transitions, handlers behaviour, error mapping, and derived outputs including quantity limits.
Ticket quantity helper
src/helpers/getTicketMaxQuantity.js
Signature updated to accept remainingQuantityPerAccount and further constrain computed max when provided.
Registration wiring
src/components/registration-form/index.js
Refactors to initialize usePromoCode using Redux discoveredPromoCodes, dispatches discoverPromoCodes after auth, centralizes form state updates, blocks advancing when an entered promo isn't applied and revalidates manual codes prior to advancing. Removes legacy promo validation handlers.
Ticket UI
src/components/ticket-type/index.js
Replaces per-component promo callbacks with a promo controller prop; clamps quantity by promo.maxQuantityFromPromo, awaits promo.onTicketSelected when selecting tickets, delegates apply/remove to promo.onApply/promo.onRemove, renders promo.validationError, shows per-account limit notice, and uses i18n for reassignment notice.
Promo input UI
src/components/promocode-input/index.js, src/components/promocode-input/index.module.scss
Component now driven by promoStatus/suggestedCode/wasAutoApplied; derives displayed input, makes field read-only when applied, triggers onApply/onRemove, shows status icons via PROMO_STATUS. Small CSS fallback added for active color.
Navigation / Next step
src/components/button-bar/index.js
ButtonBar no longer performs promo validation; accepts promoIsReady and onNextStep to gate/advance from the ticket step.
Personal info errors
src/components/personal-information/index.js
formErrors default changed from {} to [] to match usage.
i18n
src/i18n/en.json
Adds promo-related keys (labels, auto-applied/applied states, suggestions, per-account limits, reassignment notice, validation messages) and ticket-type "no tickets available" copy.

E2E tests, fixtures, Playwright config & CI

Layer / File(s) Summary
E2E fixtures
e2e/fixtures.js
New factory helpers producing JSON payloads for ticket types, discovered promo codes, paginated list wrappers, validation responses, and a normalized validation error structure.
E2E spec
e2e/promo-code-discovery.spec.js
New Playwright end-to-end spec that stubs discovery, ticket/tax endpoints and validation/apply flows; tests suggestion/auto-apply flows, manual apply/validation, per-account messaging, reassignment rules, ticket switching, error cases (500/403) and no-ticket flows.
Playwright config
playwright.config.js
New Playwright configuration: tests rooted at ./e2e, 30s timeout, baseURL https://localhost:8888, ignore HTTPS cert errors, Chromium project, integrated web server running yarn serve, 120s server timeout, CI HTML report generation.
Package scripts & deps
package.json
Adds test:ci and test:e2e scripts; adds dev deps @playwright/test, @testing-library/react-hooks; replaces node-sass with sass; updates Jest to ignore e2e/ in testPathIgnorePatterns.
CI workflow
.github/workflows/ci.yml
New CI workflow with unit-tests and e2e-tests jobs; e2e job installs Playwright browsers, runs Playwright tests, and uploads playwright-report/ on failure.

Test wiring, Jest setup & small test updates

Layer / File(s) Summary
Jest setup / i18n
jest-setup.js
Polyfills TextEncoder/TextDecoder for Node environments and initializes i18n texts for tests.
Registration tests
src/components/registration-form/__tests__/registration-form.test.js
Adds mock wiring for discoverPromoCodes thunk and widget loading actions; expands default test Redux state with promo-related fields.
Unit test adjustments
src/components/login/__tests__/login.test.js, src/components/personal-information/__tests__/personal-information.test.js, src/components/ticket-dropdown/__tests__/ticket-dropdown.test.js
Small test updates: dedicated getLoginCode mock, mock react-spring/react-use for deterministic renders, adjust assertions (remove company field), and add taxTypes prop to ticket-dropdown tests.

Sequence Diagram

sequenceDiagram
    actor User
    participant Form as RegistrationForm
    participant Hook as usePromoCode
    participant Redux as Redux/API
    participant Ticket as TicketComponent
    participant Promo as PromoInput

    User->>Form: Open registration
    Form->>Redux: dispatch discoverPromoCodes(summitId)
    Redux-->>Form: discoveredPromoCodes (store update)
    Form->>Hook: init(discoveredPromoCodes,...)

    User->>Ticket: select ticket
    Ticket->>Hook: onTicketSelected(ticket)
    Hook->>Hook: evaluate suggestion & eligibility
    alt auto_apply qualifies
        Hook->>Redux: dispatch applyPromoCode(code)
        Hook->>Redux: dispatch validatePromoCode(code, ticket)
        Redux-->>Hook: validation success
        Hook-->>Form: status = VALID (wasAutoApplied=true)
    else suggestion only
        Hook-->>Form: status = SUGGESTED (suggestedCode shown)
    end

    User->>Promo: type or edit code
    Promo->>Hook: onInputChange(value)
    Hook-->>Promo: update status / clear validationError

    User->>Promo: apply code
    Promo->>Hook: onApply(code)
    Hook->>Redux: dispatch applyPromoCode -> validatePromoCode
    Redux-->>Hook: validation result
    alt valid
        Hook-->>Form: status = VALID
    else invalid
        Hook-->>Form: status = INVALID + validationError
    end

    User->>Promo: remove or change ticket
    Promo->>Hook: onRemove() / onTicketSelected(newTicket)
    Hook->>Redux: dispatch removePromoCode (if needed)
    Hook-->>Form: status = IDLE / updated suggestion
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly Related PRs

Suggested Reviewers

  • santipalenque
  • smarcet

Poem

🐰 I hopped through code with nimble paws,

Found suggestions, autos, and tidy laws,
A hook to nudge, apply, validate, and mend,
Tests and fixtures in a playful blend,
Promo carrots served — now ship and cheer, applause!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: introducing promo code discovery and a usePromoCode hook that is central to the feature.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/promo-code-discovery

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

🧹 Nitpick comments (1)
playwright.config.js (1)

10-11: Consider letting Playwright own the dev server.

With webServer: null, npx playwright test depends on a manually started HTTPS server and will fail in clean CI/dev environments. If these tests are intended to be runnable from the repo, configure webServer with yarn serve and reuseExistingServer, or add a dedicated script documenting the required startup sequence.

Example config direction
-    // Dev server must be running: yarn serve
-    webServer: null,
+    webServer: {
+        command: 'yarn serve',
+        url: 'https://localhost:8888',
+        reuseExistingServer: !process.env.CI,
+        timeout: 120000,
+    },

Please verify against the pinned Playwright version/docs before applying, especially if yarn serve does not exit cleanly under Playwright’s process management.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@playwright.config.js` around lines 10 - 11, The Playwright config currently
sets webServer: null which forces manual HTTPS server startup; update the
webServer entry in playwright.config.js to let Playwright start the dev server
by replacing the null with an object that specifies the start command (e.g.,
"yarn serve" or the appropriate start script), the expected URL/port for health
checks, and reuseExistingServer: true so CI can reuse an already-running server;
ensure you adjust any timeout value if the server is slow to boot and verify the
chosen command is compatible with the pinned Playwright version/docs (and that
the serve script exits/handles signals correctly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@e2e/promo-code-discovery.spec.js`:
- Around line 14-44: The setupRoutes helper currently registers routes with
page.route synchronously which can race with page.goto; change setupRoutes to be
async and await each page.route registration (e.g., await
page.route('**/promo-codes/all/discover*', ...) etc.), ensure the conditional
validation handler (the block using validation and
page.route('**/promo-codes/*/apply*')) also awaits registration, and update all
callers to await setupRoutes() before navigating; also apply the same change for
any direct page.route(...) usages in the API-error tests so those route
registrations are awaited before page.goto.

In `@package.json`:
- Line 49: The package.json currently includes `@testing-library/react-hooks`
which will pick up react-test-renderer; fix the version alignment by ensuring
react-test-renderer matches the React major used: either change the
react-test-renderer dependency to ^16 to match React/ReactDOM ^16.x, or upgrade
both react and react-dom to ^17 (and set react-test-renderer to ^17) so all
three (react, react-dom, react-test-renderer) share the same major version;
update the dependency entry for react-test-renderer accordingly and run install
to verify no peer-version conflicts.

In `@src/actions.js`:
- Around line 109-122: The discoverPromoCodes action returns the getRequest
promise directly so rejections escape the try/catch; change discoverPromoCodes
to await the getRequest call (the invocation of getRequest(...)( { access_token:
accessToken } )(dispatch)) so that any HTTP errors are caught by the surrounding
try/catch, then handle or silence the error inside the catch (e.g., log with
console.error or processLogger) per the original intent; references: function
discoverPromoCodes and helper getRequest.

In `@src/components/promocode-input/index.js`:
- Around line 80-92: The Enter key handler currently calls onApply(inputValue)
unconditionally; update the onKeyDown handler to only invoke onApply when the
same conditions as the Apply button are met (i.e. inputValue is truthy and the
code is not already applied/readOnly). Specifically, change the onKeyDown logic
in the input element so it checks e.key === "Enter" && inputValue && !isApplied
(and returns early otherwise) before calling onApply(inputValue), ensuring
parity with the Apply button's disabled/readOnly behavior.

In `@src/components/ticket-type/index.js`:
- Around line 110-115: The promo validation isn't re-run when quantity changes,
so update incrementQuantity and decrementQuantity to revalidate the applied
promo: after computing the new quantity call the promo validation handler
(either promo.onApply with the current promo code and the new quantity or a new
onQuantityChange exposed from usePromoCode) to update promo state; alternatively
ensure handleApplyPromoCode is invoked or validation is performed again before
advancing. Locate functions incrementQuantity, decrementQuantity,
handleApplyPromoCode, and the promo object returned from usePromoCode to
implement this revalidation flow.
- Around line 49-56: The clamp currently skips when maxQuantity is 0 because it
checks maxQuantity > 0; update the useEffect that references maxQuantity,
quantity and setQuantity (and uses getTicketMaxQuantity /
promo.activeDiscoveredCode?.remaining_quantity_per_account) to handle zero
correctly by replacing the condition with a robust numeric check such as typeof
maxQuantity === 'number' && quantity > maxQuantity (or maxQuantity >= 0 &&
quantity > maxQuantity) so when remaining_quantity_per_account is 0 the effect
will call setQuantity(0) and prevent carrying an over-limit quantity forward.

In `@src/helpers/getTicketMaxQuantity.js`:
- Around line 6-10: The calculation treats ticket.quantity_2_sell and
ticket.max_quantity_per_order with || which converts a legitimate 0 to
Number.MAX_SAFE_INTEGER; change those fallbacks to nullish coalescing (??) so
only null/undefined use Number.MAX_SAFE_INTEGER, compute available =
(ticket.quantity_2_sell ?? Number.MAX_SAFE_INTEGER) - (ticket.quantity_sold ??
0), clamp available to a non-negative value (Math.max(available, 0)), then set
max = Math.min(clampedAvailable, (ticket.max_quantity_per_order ??
Number.MAX_SAFE_INTEGER)), and finally if remainingQuantityPerAccount != null
apply max = Math.min(max, remainingQuantityPerAccount) so sold-out tickets
remain zero and negative results are prevented.

In `@src/hooks/__tests__/usePromoCode.test.js`:
- Around line 286-302: Add assertions after invoking
result.current.onTicketSelected to ensure the discovered code is removed rather
than revalidated: assert validatePromoCode was not called
(expect(validatePromoCode).not.toHaveBeenCalled()), assert the hook cleared the
promo code (expect(result.current.promoCode).not.toBe('AUTO1')), and assert
promoCodeVerified is false
(expect(result.current.promoCodeVerified).toBe(false)); reference the
usePromoCode hook and its onTicketSelected handler, the validatePromoCode mock,
and the initial props discoveredPromoCodes/promoCode/promoCodeVerified when
adding these expectations.

In `@src/hooks/usePromoCode.js`:
- Around line 21-25: The hook currently computes a global discoveredPromoCode
via useMemo over discoveredPromoCodes which may pick an auto_apply code that
doesn't qualify for the currently selected ticket; instead filter
discoveredPromoCodes by the selected ticket before picking (i.e., move selection
into a ticket-aware helper and use pickDiscoveredCode(ticket) inside
onTicketSelected and any other ticket-change paths). Update the logic that
computes discoveredPromoCode (and the similar block around lines 78-80) to first
call discoveredPromoCodes.filter(...) or pickDiscoveredCode(ticket) using the
selected ticket id, then choose c.auto_apply || first, and ensure any subsequent
uses that check qualifies use the ticket-scoped code so auto_apply suggestions
are valid for the active ticket (references: useMemo, discoveredPromoCodes,
discoveredPromoCode, pickDiscoveredCode, onTicketSelected, qualifies).
- Around line 96-115: The discovered-code path can throw from validatePromoCode
without invoking handleValidationError, leaving users with invisible failures;
wrap calls to validatePromoCode in the isDiscoveredCode branch (the branch that
currently does await validatePromoCode({ id: ticket.id, ... })) in a try/catch
and on error call handleValidationError(error) and revert state (e.g.,
setWasAutoApplied(false) and removePromoCode() as appropriate); also update the
auto-apply catch (around applyPromoCode/discoveredPromoCode.auto_apply) to call
handleValidationError(error) in addition to setWasAutoApplied(false) so
discovered flows mirror the manual-code error handling.
- Around line 151-154: The onInputChange handler fails to mark a suggestion
dismissed when the user clears the input because setSuggestionDismissed is only
called when value is truthy; update onInputChange so it always updates
suggestion dismissal state (e.g., call setSuggestionDismissed(value !==
discoveredPromoCode?.code) even when value is empty, or explicitly set
suggestion dismissed to true when value is falsy) before invoking
onFormPromoCodeChange, referencing the existing onInputChange,
setSuggestionDismissed, discoveredPromoCode?.code and onFormPromoCodeChange
identifiers so PromoCodeInput no longer re-populates the suggestion after the
user clears the field.

---

Nitpick comments:
In `@playwright.config.js`:
- Around line 10-11: The Playwright config currently sets webServer: null which
forces manual HTTPS server startup; update the webServer entry in
playwright.config.js to let Playwright start the dev server by replacing the
null with an object that specifies the start command (e.g., "yarn serve" or the
appropriate start script), the expected URL/port for health checks, and
reuseExistingServer: true so CI can reuse an already-running server; ensure you
adjust any timeout value if the server is slow to boot and verify the chosen
command is compatible with the pinned Playwright version/docs (and that the
serve script exits/handles signals correctly).
🪄 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: 8bc40dfb-7719-4c5b-8a1a-ab5f45132979

📥 Commits

Reviewing files that changed from the base of the PR and between 3e86d70 and acd1cea.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (16)
  • e2e/fixtures.js
  • e2e/promo-code-discovery.spec.js
  • package.json
  • playwright.config.js
  • src/actions.js
  • src/components/button-bar/index.js
  • src/components/personal-information/index.js
  • src/components/promocode-input/index.js
  • src/components/registration-form/index.js
  • src/components/ticket-type/index.js
  • src/helpers/getTicketMaxQuantity.js
  • src/hooks/__tests__/usePromoCode.test.js
  • src/hooks/usePromoCode.js
  • src/reducer.js
  • src/summit-data.json
  • src/utils/constants.js

Comment thread e2e/promo-code-discovery.spec.js Outdated
Comment thread package.json
"@react-pdf/renderer": "^3.1.12",
"@testing-library/jest-dom": "^5.12.0",
"@testing-library/react": "^11.2.6",
"@testing-library/react-hooks": "^8.0.1",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Declared versions:"
node -e "const p=require('./package.json'); for (const name of ['react','react-dom','react-test-renderer','@testing-library/react-hooks']) console.log(name, p.devDependencies?.[name] || p.dependencies?.[name] || p.peerDependencies?.[name] || '(missing)')"

echo
echo "@testing-library/react-hooks peer dependencies:"
npm view `@testing-library/react-hooks`@8.0.1 peerDependencies --json

echo
echo "If node_modules is present, resolved versions:"
npm ls react react-dom react-test-renderer `@testing-library/react-hooks` --depth=0 || true

Repository: fntechgit/summit-registration-lite

Length of output: 1835


🌐 Web query:

@testing-library/react-hooks 8.0.1 peer dependencies react

💡 Result:

The peer dependencies for @testing-library/react-hooks version 8.0.1 are: - react: ^16.9.0 || ^17.0.0 - react-dom: ^16.9.0 || ^17.0.0 (optional) - react-test-renderer: ^16.9.0 || ^17.0.0 (optional) - @types/react: ^16.9.0 || ^17.0.0 (optional) This package does not support React 18 and is the last version released in June 2022. For React 18+, use @testing-library/react's renderHook instead.

Citations:


🌐 Web query:

react-test-renderer 17 compatibility react 16

💡 Result:

react-test-renderer version 17 is not fully compatible with React 16. There was a known bug in the initial 17.0.0 release where it transitively peer-depended on react@^16.0.0 via react-shallow-renderer@16.13.1, causing npm warnings when installing with React 17. This was fixed in later patches like 17.0.2 by updating dependencies to align with React 17. Using react-test-renderer 17 with React 16 may lead to runtime issues due to mismatched react-is versions and internal API differences between React major versions. Official React documentation and patterns indicate react-test-renderer versions should match the React major version (e.g., use react-test-renderer ^16.x for React 16). The package is now deprecated in favor of @testing-library/react.

Citations:


Align the hook test renderer with the resolved React major version.

@testing-library/react-hooks uses react-test-renderer by default when it is installed. Setting react-test-renderer to ^17 while React and ReactDOM remain at ^16.x creates a version mismatch that causes runtime incompatibility due to differing internal APIs and react-is versions between React major versions. Match react-test-renderer to the React major version (use ^16 for React 16, or upgrade React/ReactDOM to ^17 to use ^17).

Also applies to: 92-92

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package.json` at line 49, The package.json currently includes
`@testing-library/react-hooks` which will pick up react-test-renderer; fix the
version alignment by ensuring react-test-renderer matches the React major used:
either change the react-test-renderer dependency to ^16 to match React/ReactDOM
^16.x, or upgrade both react and react-dom to ^17 (and set react-test-renderer
to ^17) so all three (react, react-dom, react-test-renderer) share the same
major version; update the dependency entry for react-test-renderer accordingly
and run install to verify no peer-version conflicts.

Comment thread src/actions.js
Comment on lines +109 to +122
export const discoverPromoCodes = (summitId) => async (dispatch, getState, { apiBaseUrl, getAccessToken }) => {
try {
const accessToken = await getAccessToken();
return getRequest(
createAction(DISCOVER_PROMO_CODES),
createAction(DISCOVER_PROMO_CODES_SUCCESS),
`${apiBaseUrl}/api/v1/summits/${summitId}/promo-codes/all/discover`,
// Discovery is non-blocking - errors silently ignored.
// Auth errors will surface on the next user-initiated action.
null
)({ access_token: accessToken })(dispatch);
} catch (e) {
console.log(e);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Find the getRequest function definition and usage
rg -n "getRequest" src/ --type js -B 2 -A 2

Repository: fntechgit/summit-registration-lite

Length of output: 1484


🏁 Script executed:

# Check the registration-form/index.js file to see how discoverPromoCodes is called
fd "index.js" src/registration-form/ -x cat {}

Repository: fntechgit/summit-registration-lite

Length of output: 187


🏁 Script executed:

# Look for Promise/async patterns in the thunk action creators
rg -n "async.*dispatch.*getState" src/actions.js --type js -A 5

Repository: fntechgit/summit-registration-lite

Length of output: 3689


🏁 Script executed:

# Find where getRequest is imported from
rg -n "export.*getRequest|from.*getRequest" src/ --type js

Repository: fntechgit/summit-registration-lite

Length of output: 60


🏁 Script executed:

# Check one of the other async thunks with getRequest to see the pattern
sed -n '150,180p' src/actions.js

Repository: fntechgit/summit-registration-lite

Length of output: 1197


🏁 Script executed:

# Search for calls to discoverPromoCodes to understand the usage context
rg -n "discoverPromoCodes" src/ --type js -B 1 -A 3

Repository: fntechgit/summit-registration-lite

Length of output: 1817


🏁 Script executed:

# Get more context around the discoverPromoCodes call
sed -n '240,250p' src/components/registration-form/index.js

Repository: fntechgit/summit-registration-lite

Length of output: 512


🏁 Script executed:

# Check the function signature and mapDispatchToProps to understand dispatch binding
sed -n '545,560p' src/components/registration-form/index.js

Repository: fntechgit/summit-registration-lite

Length of output: 823


🏁 Script executed:

# Look at the full discoverPromoCodes implementation to confirm error handling
sed -n '109,125p' src/actions.js

Repository: fntechgit/summit-registration-lite

Length of output: 810


Await the discovery request to properly catch non-blocking failures.

Line 112 returns the request promise directly without awaiting, so HTTP rejections from getRequest bypass the catch block. Since the caller in registration-form/index.js does not capture the returned promise, discovery failures result in unhandled rejections despite the "silently ignored" comment.

 export const discoverPromoCodes = (summitId) => async (dispatch, getState, { apiBaseUrl, getAccessToken }) => {
     try {
         const accessToken = await getAccessToken();
-        return getRequest(
+        return await getRequest(
             createAction(DISCOVER_PROMO_CODES),
             createAction(DISCOVER_PROMO_CODES_SUCCESS),
             `${apiBaseUrl}/api/v1/summits/${summitId}/promo-codes/all/discover`,
             // Discovery is non-blocking - errors silently ignored.
             // Auth errors will surface on the next user-initiated action.
             null
         )({ access_token: accessToken })(dispatch);
     } catch (e) {
         console.log(e);
+        return null;
     }
 }
📝 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
export const discoverPromoCodes = (summitId) => async (dispatch, getState, { apiBaseUrl, getAccessToken }) => {
try {
const accessToken = await getAccessToken();
return getRequest(
createAction(DISCOVER_PROMO_CODES),
createAction(DISCOVER_PROMO_CODES_SUCCESS),
`${apiBaseUrl}/api/v1/summits/${summitId}/promo-codes/all/discover`,
// Discovery is non-blocking - errors silently ignored.
// Auth errors will surface on the next user-initiated action.
null
)({ access_token: accessToken })(dispatch);
} catch (e) {
console.log(e);
}
export const discoverPromoCodes = (summitId) => async (dispatch, getState, { apiBaseUrl, getAccessToken }) => {
try {
const accessToken = await getAccessToken();
return await getRequest(
createAction(DISCOVER_PROMO_CODES),
createAction(DISCOVER_PROMO_CODES_SUCCESS),
`${apiBaseUrl}/api/v1/summits/${summitId}/promo-codes/all/discover`,
// Discovery is non-blocking - errors silently ignored.
// Auth errors will surface on the next user-initiated action.
null
)({ access_token: accessToken })(dispatch);
} catch (e) {
console.log(e);
return null;
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/actions.js` around lines 109 - 122, The discoverPromoCodes action returns
the getRequest promise directly so rejections escape the try/catch; change
discoverPromoCodes to await the getRequest call (the invocation of
getRequest(...)( { access_token: accessToken } )(dispatch)) so that any HTTP
errors are caught by the surrounding try/catch, then handle or silence the error
inside the catch (e.g., log with console.error or processLogger) per the
original intent; references: function discoverPromoCodes and helper getRequest.

Comment thread src/components/promocode-input/index.js Outdated
Comment thread src/components/ticket-type/index.js Outdated
Comment thread src/helpers/getTicketMaxQuantity.js Outdated
Comment thread src/hooks/__tests__/usePromoCode.test.js Outdated
Comment thread src/hooks/usePromoCode.js
Comment on lines +21 to +25
// Pick first auto_apply code, or first code if none has auto_apply
const discoveredPromoCode = useMemo(() => {
if (!discoveredPromoCodes?.length) return null;
return discoveredPromoCodes.find(c => c.auto_apply) || discoveredPromoCodes[0];
}, [discoveredPromoCodes]);
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Select the discovered code after filtering by the selected ticket.

The hook picks one global discoveredPromoCode before ticket qualification. If the first auto_apply code does not apply to the selected ticket but a later discovered code does, qualifies is false and the user gets no valid suggestion/auto-apply.

🐛 Refactor direction
-    // Pick first auto_apply code, or first code if none has auto_apply
-    const discoveredPromoCode = useMemo(() => {
+    const getCodeValidForTicket = useCallback((code, ticket) => {
+        if (!code || !ticket) return false;
+        const allowed = code.allowed_ticket_types || [];
+        if (allowed.length === 0) return true;
+        return allowed.some(tt => (typeof tt === 'object' ? tt.id : tt) === ticket.id);
+    }, []);
+
+    const pickDiscoveredCode = useCallback((ticket) => {
         if (!discoveredPromoCodes?.length) return null;
-        return discoveredPromoCodes.find(c => c.auto_apply) || discoveredPromoCodes[0];
+        const eligible = discoveredPromoCodes.filter(code => getCodeValidForTicket(code, ticket));
+        return eligible.find(c => c.auto_apply) || eligible[0] || null;
-    }, [discoveredPromoCodes]);
+    }, [discoveredPromoCodes, getCodeValidForTicket]);

You can then use pickDiscoveredCode(ticket) in onTicketSelected so selection is ticket-aware.

Also applies to: 78-80

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/usePromoCode.js` around lines 21 - 25, The hook currently computes
a global discoveredPromoCode via useMemo over discoveredPromoCodes which may
pick an auto_apply code that doesn't qualify for the currently selected ticket;
instead filter discoveredPromoCodes by the selected ticket before picking (i.e.,
move selection into a ticket-aware helper and use pickDiscoveredCode(ticket)
inside onTicketSelected and any other ticket-change paths). Update the logic
that computes discoveredPromoCode (and the similar block around lines 78-80) to
first call discoveredPromoCodes.filter(...) or pickDiscoveredCode(ticket) using
the selected ticket id, then choose c.auto_apply || first, and ensure any
subsequent uses that check qualifies use the ticket-scoped code so auto_apply
suggestions are valid for the active ticket (references: useMemo,
discoveredPromoCodes, discoveredPromoCode, pickDiscoveredCode, onTicketSelected,
qualifies).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional design. The hook selects one global code from discovery, then isCodeValidForTicket handles per-ticket qualification. The server controls which codes are returned and their ordering. Client-side per-ticket re-selection would add complexity without clear benefit since the discovery response is already filtered for the user.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcutrini, understood — the server-side ordering + isCodeValidForTicket qualification check is sufficient to guard ticket eligibility, so the global selection is correct by design. I'll note this for future reviews.


✏️ Learnings added
Learnt from: gcutrini
Repo: fntechgit/summit-registration-lite PR: 141
File: src/hooks/usePromoCode.js:22-26
Timestamp: 2026-04-23T20:30:08.056Z
Learning: In `src/hooks/usePromoCode.js`, the `discoveredPromoCode` useMemo intentionally picks one global code from the full `discoveredPromoCodes` array (preferring `auto_apply`, else first) without pre-filtering by the selected ticket. Per-ticket qualification is handled separately by `isCodeValidForTicket`. The server already filters and orders the discovery response for the user, so client-side per-ticket re-selection is considered unnecessary complexity.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: gcutrini
Repo: fntechgit/summit-registration-lite PR: 139
File: src/actions.js:199-202
Timestamp: 2026-04-15T00:33:39.320Z
Learning: In JavaScript, functions declared with `async` always return a Promise. Therefore, `return null` inside an `async` function already behaves like `return Promise.resolve(null)`, and reviewers should not suggest replacing `return null` with `Promise.resolve(null)` as it is redundant/incorrect. If the code’s intent is different (e.g., returning an explicitly created Promise), only then should `Promise.resolve(...)` be discussed.

Comment thread src/hooks/usePromoCode.js Outdated
Comment thread src/hooks/usePromoCode.js Outdated
Comment thread src/hooks/usePromoCode.js Outdated
setWasAutoApplied(false);
removePromoCode();
} else {
await validatePromoCode({ id: ticket.id, ticketQuantity: 1, sub_type: ticket.sub_type });
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcutrini
discovered code re-validation path has no error handling:

This path runs when:

  1. A discovered code is already applied and validated (e.g., AUTO1 was auto-applied for ticket A)
  2. The user switches to a different ticket (ticket B)
  3. The discovered code is valid for ticket B too (qualifies === true)
  4. The hook re-validates the code against the new ticket

This is a normal user flow , browse tickets, switch between qualifying options.

What happens when validatePromoCode rejects in Branch B?

The validatePromoCode action can reject for several reasons:

  1. Code expired between discovery and ticket switch
  2. Code quantity exhausted — someone else used the last one
  3. Rate limited (429)
  4. Network error / timeout
  5. Code not valid for this ticket type + quantity combination , the allowed_ticket_types check in the hook uses the discovery response data, but the backend may have different/updated rules

When any of these happen:

validatePromoCode rejects
→ await throws in onTicketSelected
→ no catch block
→ promise rejection propagates to caller

The caller is handleTicketChange in ticket-type:

  const handleTicketChange = async (t) => {
      setTicket(t);
      setQuantity(minQuantity);
      trackViewItem(t);
      await promo.onTicketSelected(t);   // ← unhandled rejection lands here
  }

No try/catch there either. The rejection continues up to the onClick handler on the ticket dropdown, which is an event handler , so it becomes an unhandled promise rejection in the browser console.

What the user sees

Redux state after the rejection:

The validatePromoCode action dispatches VALIDATE_PROMO_CODE (sets promoCodeValidating: true) at the start. If the request fails, the error handler dispatches VALIDATE_PROMO_CODE_ERROR (sets
promoCodeVerified: false, promoCodeValidating: false). So Redux state does get updated via the reducer.

This means:

  • promoCodeVerified becomes false → promo.status becomes PROMO_STATUS.INVALID → the input shows the ✕ icon
  • The "Next" button stays disabled because promo.isReady is false for INVALID status

What's missing:

  • No error message. handleValidationError(e) is never called, so promo.validationError stays null. The renders nothing. The user sees ✕ but no
    explanation of why their code was rejected.
  • No state cleanup. wasAutoApplied stays true even though the code is now invalid. The label still says "Following promo code was automatically applied:" next to a ✕ icon , contradictory UI.

Compare with Branch A (manual code): the user sees the ✕ icon plus a descriptive error message like "Promo code XYZ can not be applied to Ticket Type Standard."

Fix

Wrap the same way Branch A does:

if (isDiscoveredCode) {
      if (!qualifies) {
          setWasAutoApplied(false);
          removePromoCode();
      } else {
          try {
              await validatePromoCode({ id: ticket.id, ticketQuantity: 1, sub_type: ticket.sub_type });
          } catch (e) {
              handleValidationError(e);
              // Optionally also clean up discovered code state:
              // setWasAutoApplied(false);
              // removePromoCode();
          }
      }
      return;
  }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 48d7659. All validation paths now go through onRevalidate which wraps try/catch + handleValidationError. Discovered code re-validation and auto-apply both show error messages on failure and reset wasAutoApplied.

Comment thread src/hooks/usePromoCode.js Outdated

const onInputChange = useCallback((value) => {
setValidationError(null);
if (value) setSuggestionDismissed(value !== discoveredPromoCode?.code);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcutrini onInputChange doesn't dismiss suggestion when input is cleared:

The setSuggestionDismissed call is guarded by if (value). When the user clears the input field (empty string ""), value is falsy, so the entire setSuggestionDismissed(...) line is skipped.

Consider this sequence:

  1. User sees a suggested promo code (suggestion banner visible)
  2. User starts typing a different code — onInputChange("XYZ") fires, setSuggestionDismissed(true) runs because "XYZ" !== discoveredCode — suggestion banner hides. Correct.
  3. User clears the input — onInputChange("") fires, if (value) is falsy, setSuggestionDismissed is not called — suggestionDismissed stays true. In this case it's actually fine — it stays
    dismissed.

Now the reverse scenario:

  1. User sees a suggested promo code (suggestion banner visible, suggestionDismissed = false)
  2. User types exactly the suggested code manually — onInputChange("DISCOVERED_CODE") fires, setSuggestionDismissed(false) runs because "DISCOVERED_CODE" === discoveredCode. Suggestion banner
    stays visible/re-shows.
  3. User clears the input — onInputChange("") fires, if (value) is falsy, setSuggestionDismissed is not called — suggestionDismissed stays false, and the suggestion banner remains visible even
    though the user just actively cleared the field.

The actual risk

The scenario is narrow. It requires the user to type the exact discovered code text, then clear the input. At that point suggestionActive would still need to be true (set by onTicketSelected),
which it typically is if they haven't changed tickets. The suggestion banner would re-appear when it should stay hidden.

This is a UX glitch, not a data integrity issue. The severity is correctly LOW-to-MEDIUM — it won't cause wrong charges or broken state, just a confusing suggestion banner that won't go away
until the user selects a different ticket.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fdc7253. Removed the if (value) guard so setSuggestionDismissed always runs. Clearing the input now properly dismisses the suggestion.

Comment thread src/actions.js
null
)({ access_token: accessToken })(dispatch);
} catch (e) {
console.log(e);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcutrini i do think that here u need to return null

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d857037.

Comment thread src/hooks/usePromoCode.js
if (!discoveredPromoCode) return;

// Discovered code is currently applied
if (isDiscoveredCode) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcutrini
Missing tests: validation on quantity change, error handling in onTicketSelected Branch B

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in f31c629 and 32f145a. Tests cover: discovered code re-validation failure (error message + wasAutoApplied reset), auto-apply validation failure, manual code re-validation failure on ticket switch, and suggestion dismissal when input cleared.

Copy link
Copy Markdown
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcutrini please review comments

For manual (non-discovered) codes, re-validate against the API with
the final ticket + quantity before advancing to personal info.

For discovered codes, the stepper now caps using the tighter of
remaining_quantity_per_account and quantity_available from the
discovery response, preventing invalid quantity selection client-side.

Also skip the unapplied-code warning when status is SUGGESTED
(system-prefilled, not user-typed).
@gcutrini
Copy link
Copy Markdown
Member Author

Fixed the regression. Two changes:

  1. For manual promo codes (not discovered), we now re-validate on Next with the final ticket + quantity before advancing. If the API rejects (e.g., qty exceeds quantity_available), the user stays on the ticket step and sees the error. Discovered codes skip this call because the stepper is already capped client-side.

  2. For discovered codes, the stepper now caps using the tighter of remaining_quantity_per_account and quantity_available from the discovery response. Previously it only used remaining_quantity_per_account.

So the flow is:

  • Discovered code: stepper prevents invalid quantity selection, no extra API call on Next
  • Manual code: stepper uses ticket-type limits only, API validates on Next as a gate (same behavior as before this PR)
  • No promo code: advances directly, no API call

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/components/registration-form/index.js (1)

314-328: Minor: also guard against promo.status === INVALID here.

promoIsReady already disables the Next button for INVALID, so this guard is effectively dead code — but it's worth a defensive check: if anything external ever calls onNextStep (e.g., keyboard submit, future refactor), a stale promoCode in INVALID state would slip past the !promoCode check and trigger revalidation again rather than surfacing the pre-existing error. Not a blocker given today's flow.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/components/registration-form/index.js` around lines 314 - 328, The guard
in handleAdvanceFromTicketStep incorrectly only checks promo.status !==
PROMO_STATUS.SUGGESTED before allowing a stale invalid promo to slip through;
update the conditional that currently reads promo.status !==
PROMO_STATUS.SUGGESTED to also exclude PROMO_STATUS.INVALID (e.g., promo.status
!== PROMO_STATUS.SUGGESTED && promo.status !== PROMO_STATUS.INVALID) so that if
promo.status is INVALID the handler sets the validation error via
promo.setValidationError and returns, matching the promoIsReady behavior and
preventing revalidation of an already-invalid promo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/hooks/usePromoCode.js`:
- Around line 77-85: handleValidationError currently only sets a message when
e?.res?.body exists, so non-API errors (network, timeouts, plain Error, 5xx
without body) are swallowed; update handleValidationError to add a fallback
branch that inspects other shapes (e.message, e?.res?.statusText, or a generic
'Network or server error' message) and calls setValidationError with a
user-facing string when no e.res.body is present; keep the existing parsing
logic for API errors (errors = e.res.body.errors || ...), but ensure
handleValidationError always sets some message (using e.message || `Request
failed${e?.res?.status ? `: ${e.res.status}` : ''}`) so the UI shows an
explanation for all error types.
- Around line 132-147: The onApply handler currently swallows errors from
applyPromoCode causing the UI to clear the promo input with no feedback; modify
onApply (the useCallback) so that the catch(e) after await applyPromoCode(code)
forwards the error to the same UI error path used for validation failures (e.g.,
call handleValidationError(e) or setValidationError(...) instead of just
returning), ensuring clearFormErrors() remains called and the function exits
afterwards; keep the existing validatePromoCode try/catch unchanged so both
applyPromoCode and validatePromoCode surface errors consistently to the user.

---

Nitpick comments:
In `@src/components/registration-form/index.js`:
- Around line 314-328: The guard in handleAdvanceFromTicketStep incorrectly only
checks promo.status !== PROMO_STATUS.SUGGESTED before allowing a stale invalid
promo to slip through; update the conditional that currently reads promo.status
!== PROMO_STATUS.SUGGESTED to also exclude PROMO_STATUS.INVALID (e.g.,
promo.status !== PROMO_STATUS.SUGGESTED && promo.status !==
PROMO_STATUS.INVALID) so that if promo.status is INVALID the handler sets the
validation error via promo.setValidationError and returns, matching the
promoIsReady behavior and preventing revalidation of an already-invalid promo.
🪄 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: fd9b6949-d9c1-4066-b574-ef04582fcf61

📥 Commits

Reviewing files that changed from the base of the PR and between acd1cea and 2da0a76.

📒 Files selected for processing (6)
  • src/components/promocode-input/index.js
  • src/components/registration-form/index.js
  • src/components/ticket-type/index.js
  • src/hooks/__tests__/usePromoCode.test.js
  • src/hooks/usePromoCode.js
  • src/i18n/en.json
✅ Files skipped from review due to trivial changes (1)
  • src/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/components/promocode-input/index.js

Comment thread src/hooks/usePromoCode.js
Comment thread src/hooks/usePromoCode.js Outdated
Reuse onRevalidate for all validation paths in onTicketSelected
and onApply, replacing bare/inconsistent try/catch blocks. Now
discovered code re-validation and auto-apply show error messages
on failure and reset wasAutoApplied.
Remove the if (value) guard on setSuggestionDismissed so clearing
the input properly dismisses the suggestion instead of leaving it
visible.
…ring

Cover discovered code re-validation failure (error message shown,
wasAutoApplied reset), auto-apply validation failure, manual code
re-validation failure on ticket switch, and suggestion dismissal
when input is cleared.
Prevents quantity_2_sell: 0 from being treated as unlimited.
Move hardcoded error strings in handleValidationError to i18n keys.
Add fallback error message for non-API errors (network, timeout).
@gcutrini gcutrini requested a review from smarcet April 23, 2026 20:31
Copy link
Copy Markdown
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

* chore(ci): add e2e integration

Signed-off-by: smarcet <smarcet@gmail.com>

* chore(unit-test): fix unit test

---------

Signed-off-by: smarcet <smarcet@gmail.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/ci.yml:
- Around line 15-19: CI is installing Node 20 in the "Setup Node.js" step (uses:
actions/setup-node@v4 with node-version: 20) while the repo pins 18.15.0 in
.nvmrc; fix by making CI and .nvmrc consistent: either update .nvmrc to 20, or
change the actions/setup-node node-version from 20 to 18.15.0 (or better, read
the version from .nvmrc and pass it into the actions/setup-node step so both
jobs use the same value).

In `@src/components/ticket-type/index.js`:
- Around line 245-250: The code uses hardcoded English unit strings when calling
T.translate for the per-account limit; replace the inline 'ticket'/'tickets'
logic by using i18n pluralization keys instead: call T.translate with a single
key (e.g. 'promo_code.per_account_limit') and pass count: promo.perAccountLimit
(or use separate keys like 'promo_code.per_account_limit_one' and
'promo_code.per_account_limit_other' if your i18n system requires explicit
forms) so translations handle plural rules; update the translation files (e.g.
add promo_code.per_account_limit_one and promo_code.per_account_limit_other in
en.json or define an ICU pluralization entry for promo_code.per_account_limit)
and remove the unit param from the component call (affecting the PromoCodeNotice
invocation and the T.translate call that currently references
promo.perAccountLimit and unit).

In `@src/hooks/usePromoCode.js`:
- Around line 53-61: The two cap sources are handled inconsistently:
remaining_quantity_per_account allows zero (uses != null) but quantity_available
currently ignores zero (uses > 0); update the maxQuantityFromPromo calculation
to treat quantity_available the same way as remaining_quantity_per_account
(e.g., check quantity_available != null or otherwise include 0 as a valid cap)
so a sold-out promo (0) is considered when building caps; modify the condition
around activeDiscoveredCode.quantity_available in the maxQuantityFromPromo
useMemo to match the remaining_quantity_per_account check.
- Around line 78-88: In handleValidationError (the useCallback in
usePromoCode.js), ensure errors[0] is safely coerced to a string before using it
or passing it to setValidationError: instead of directly calling
setValidationError(errors[0]) or applying the regex to a potentially non-string,
extract a string via errors[0].message || String(errors[0]) (or
JSON.stringify(errors[0]) if you prefer) and then run the /is not a valid code/i
test on that string; fall back to T.translate('promo_code.validation_error')
when no usable message exists.
🪄 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: 0dfd0d6c-20c6-42c6-9946-c506e55b13e7

📥 Commits

Reviewing files that changed from the base of the PR and between 2da0a76 and 946f177.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (15)
  • .github/workflows/ci.yml
  • jest-setup.js
  • package.json
  • playwright.config.js
  • src/actions.js
  • src/components/login/__tests__/login.test.js
  • src/components/personal-information/__tests__/personal-information.test.js
  • src/components/promocode-input/index.js
  • src/components/registration-form/__tests__/registration-form.test.js
  • src/components/ticket-dropdown/__tests__/ticket-dropdown.test.js
  • src/components/ticket-type/index.js
  • src/helpers/getTicketMaxQuantity.js
  • src/hooks/__tests__/usePromoCode.test.js
  • src/hooks/usePromoCode.js
  • src/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/i18n/en.json
  • src/helpers/getTicketMaxQuantity.js
  • src/actions.js
  • playwright.config.js
  • src/hooks/tests/usePromoCode.test.js
  • src/components/promocode-input/index.js

Comment thread .github/workflows/ci.yml
Comment thread src/components/ticket-type/index.js Outdated
Comment thread src/hooks/usePromoCode.js
Comment thread src/hooks/usePromoCode.js
gcutrini added 2 commits May 11, 2026 12:45
When discovery returns more than one code, don't auto-apply any.
Let the user choose manually. Auto-apply only triggers when
exactly one code is returned and it has auto_apply: true.
Keep both mock sets: uicore/company-input mocks from main and
react-spring/react-use mocks from feature branch.
gcutrini added 19 commits May 14, 2026 11:15
When a code is applied but the API returns zero tickets (invalid code or sold
out), surface 'Promo code entered is not valid' and render the INVALID status
icon instead of the green checkmark. Previously the validation endpoint only
ran when a ticket was selected, so invalid codes in the no-tickets/WithPromoCode
scenario sat in APPLYING state silently with a valid-looking checkmark.
Previously showTicketSelector included || !!promoCode, which kept the empty
'Select a ticket' dropdown visible when an invalid/exhausted code was applied,
masking the no-tickets-available notice. Drop the promoCode clause so the
notice keeps showing in that case.
The applied-state checkmark was firing during the brief APPLYING window before
verification completed, suggesting the code was valid before we knew. Switch
APPLYING to render the spinner like VALIDATING, and add an applying_label that
shows during manual apply (auto-applied keeps the existing label).
…alue

Split validationError state in two: manualError holds API/form-pushed messages,
and the user-facing validationError is computed by merging that with the
status-derived INVALID message. Drops the useEffect that was syncing the
error with the no-tickets state — the same condition is already in the status
memo, so we can derive the error message from status instead of maintaining
parallel state. Also nests the status memo's isApplied checks for readability.
The early-auto-apply useEffect and the auto-apply branch inside onTicketSelected
duplicated the same apply/revalidate/applyingCode flow with subtly different
error handling. Pull the shared work into tryAutoApply (gating stays at the
call sites). As a side effect, this adds the missing applyingCode bracket
around onTicketSelected's auto-apply path, which could re-flash INVALID during
the apply transition.
Form-level concern was piggybacking on the hook's error channel via an exposed
setValidationError setter. Pull it into local state in registration-form and
override the validationError in the prop pass to TicketTypeComponent. The hook
now stops exposing the setter, so manualError is purely API errors from
handleValidationError. Tests updated to set up the error state via a real
onRevalidate failure instead of the removed export.
setFormErrors([]) was firing on every promoCode change, which could silently
wipe backend validation errors the user hasn't acknowledged yet (e.g. submit
personal info → backend errors land → user removes promo on ticket step → form
errors disappear). Form errors are step-scoped, not promo-scoped.
Add noTicketsAvailableMessage prop on the RegistrationModal API, threaded
through to TicketTypeComponent. When provided, it overrides the default
i18n string in the no-tickets notice. Hosts (event-site) can wire this to
a marketing setting.
…ate reject

- ci.yml: read node-version from .nvmrc instead of hardcoding 20, so CI matches
  the version contributors run locally (.nvmrc pins 18.15.0).
- registration-form: wrap promo.onRevalidate in try/finally inside
  handleAdvanceFromTicketStep so stopWidgetLoading() always fires, even when
  the revalidation promise rejects. Prevents the AjaxLoader overlay from
  getting stuck on.
…n plural

- usePromoCode: quantity_available now uses != null instead of > 0 so 0
  (sold out) caps the stepper at 0, matching remaining_quantity_per_account.
  Tests updated to reflect the corrected semantic: null = unlimited, 0 = sold out.
- usePromoCode: handleValidationError coerces non-string errors[] entries to a
  message string (falls back to first.message, then String(first)) so React
  never renders [object Object].
- per_account_limit: split into _one / _other i18n keys driven by count, so the
  unit pluralization is translatable instead of hardcoded English.
Two small robustness fixes flagged by CodeRabbit:
- The clamp effect read quantity from a closure but only declared maxQuantity
  as a dep. If both change in the same render, the stale quantity would be
  compared. Adding quantity to deps is safe since the body is a no-op when
  quantity <= maxQuantity.
- Default promo to {} on destructure so promo.maxQuantityFromPromo can't
  throw if a future caller forgets to pass the prop.
The locator 'text=no tickets available' never matched because the actual i18n
string is 'There are no tickets currently available for purchase...' — the word
'currently' sits between 'tickets' and 'available'. Updated the selector to
'no tickets currently available' which is unique to this notice.
…nd tests

Three follow-ups from a late review pass on the PR:

- usePromoCode: tryAutoApply previously swallowed apply errors with an empty
  catch. In the WithPromoCode-only summit case (no ticket selected at apply
  time) this meant a server-side rejection left the user staring at the
  no-tickets notice with no explanation. Route the error through
  handleValidationError so it surfaces in validationError. Also documents why
  the missing applyingCode race-guard is acceptable (the two call sites are
  mutually exclusive by construction) and adds a one-liner on the per-session
  userRemovedAutoApply lock.

- promocode-input: rename the local isApplied flag to isLocked. The variable
  is true for APPLYING + VALIDATING + VALID + INVALID, so it actually means
  'input is readonly until the user removes the code', and the old name
  reads as 'a code is successfully applied'.

- tests: add 5 early-auto-apply cases (fires on conditions, skips when
  tickets exist, skips when still loading, skips when multiple codes,
  surfaces error on rejection) plus 2 INVALID-without-ticket status cases.
  Brings the suite to 64 tests.
- Return grouped into {state, actions} with section comments and ordering
- Rename wasAutoApplied → isAutoApplied (matches isReady, isDiscoveredCode)
- Rename onFormPromoCodeChange → setFormPromoCode (it's a setter)
- Drop unused activeDiscoveredCode and dead clearFormErrors prop
- Drop errorOverride round-trip; form composes ticketStepError and passes
  it to TicketType as a separate prop
Rename promo-code-notice → ticket-notice; component Notice → TicketNotice.
message prop accepts string or array (stacked, 1px spacing). ticket-type now
composes the info notice from per-account cap, per-order cap (new), and
non-transferable. New i18n: ticket_type.max_per_order_one/_other.
Comment thread src/hooks/usePromoCode.js
try {
await applyPromoCode(code);
} catch (e) {
setApplyingCode(false);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcutrini
When applyPromoCode rejects (network, 500, anything that does NOT set promoCodeVerified=false in the reducer), the catch block returns without setting manualError. The user sees the input revert to IDLE with no feedback. tryAutoApply correctly calls handleValidationError(e) , the manual path is inconsistent.

Comment thread src/components/ticket-type/index.js Outdated
// Clamp quantity when max changes (e.g. per-account limit kicks in after auto-apply)
useEffect(() => {
if (quantity > maxQuantity) {
setQuantity(Math.max(maxQuantity, minQuantity));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcutrini
setQuantity(Math.max(maxQuantity, minQuantity)) with minQuantity=1 returns 1 even when the discovered cap is 0 (sold-out / remaining_quantity_per_account=0). The user can keep quantity=1 and proceed

Copy link
Copy Markdown
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcutrini please review

gcutrini added 2 commits May 18, 2026 11:55
…to-apply

The catch in onApply silently returned without setting manualError, so
network/500 rejections from applyPromoCode left the input back at IDLE
with no user feedback. tryAutoApply already routes errors through
handleValidationError; the manual path now does the same.
The clamp effect used Math.max(maxQuantity, minQuantity), so when the
discovered cap was 0 (sold out or per-account limit reached) the quantity
was floored back to 1, putting it above the cap. Use the cap directly
when it falls below minQuantity, and gate the effect on a selected ticket
so the initial null-ticket state does not pre-empt user selection.
@gcutrini
Copy link
Copy Markdown
Member Author

Both review comments addressed. usePromoCode onApply catch now calls handleValidationError(e) matching tryAutoApply. ticket-type clamp uses the cap directly when cap < minQuantity, effect gated on ticket being set. All 98 tests pass.

@gcutrini gcutrini requested a review from smarcet May 18, 2026 15:01
Compute ticketSelectionValid (ticket present, quantity within [min, max])
once in ticket-type and propagate it through formValues. button-bar's Next
gate reads the flag instead of duplicating the rule, so the per-account-
exhausted case (cap=0, clamp drops quantity to 0) now also blocks Next.

Add an e2e test for remaining_quantity_per_account=0 that asserts Next is
disabled at the ticket-select step.
Copy link
Copy Markdown
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smarcet smarcet merged commit ccfbdf4 into main May 18, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants