Fix promo code spinner stuck when applied before selecting a ticket - #151
Conversation
Applying a promo code before selecting a ticket left the input spinner running forever: with no ticket to validate against, no validation ever ran, and the status fell through to APPLYING with nothing in flight. Replace the single status enum with canonical signals (isBusy, isInvalid, isSuggested) whose precedence is encoded once, and derive the display status from them (IDLE / SUGGESTED / PROCESSING / APPLIED / INVALID). The spinner now renders only while a request is genuinely in flight; a code applied without a ticket rests as APPLIED and validates once a ticket is picked.
📝 WalkthroughWalkthroughThe promo-code flow now derives status from local validation state. It supports application before ticket selection, deferred validation, stale-response handling, retryable errors, and final quantity revalidation. The form, input, reducer, and tests use the updated model. ChangesPromo status flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RegistrationForm
participant usePromoCode
participant PromoCodeInput
participant TicketSelection
RegistrationForm->>usePromoCode: read status and readiness
usePromoCode->>PromoCodeInput: expose PROCESSING or UNVERIFIED
TicketSelection->>RegistrationForm: select ticket
RegistrationForm->>usePromoCode: revalidate applied promo code
usePromoCode->>PromoCodeInput: update to APPLIED or INVALID
RegistrationForm->>RegistrationForm: advance when revalidation succeeds
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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
🤖 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 `@e2e/promo-code-apply-without-ticket.spec.js`:
- Line 94: Update the validationCalls assertion in the ticket-selection test to
require exactly one deferred validation request by using an equality check
against 1 instead of only checking that the count is positive.
🪄 Autofix
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 Plus
Run ID: a03bf7cc-e375-42da-b149-cb04e9cb89b6
📒 Files selected for processing (6)
e2e/promo-code-apply-without-ticket.spec.jssrc/components/promocode-input/index.jssrc/components/registration-form/index.jssrc/hooks/__tests__/usePromoCode.test.jssrc/hooks/usePromoCode.jssrc/utils/constants.js
|
|
||
| // Picking a ticket triggers the deferred promo+ticket validation | ||
| await selectTicket(page, 'General Admission'); | ||
| await expect.poll(() => validationCalls).toBeGreaterThan(0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert exactly one deferred validation request.
Line 94 accepts duplicate validation requests. The test title requires one request after ticket selection. Change the assertion to toBe(1).
Proposed fix
- await expect.poll(() => validationCalls).toBeGreaterThan(0);
+ await expect.poll(() => validationCalls).toBe(1);📝 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.
| await expect.poll(() => validationCalls).toBeGreaterThan(0); | |
| await expect.poll(() => validationCalls).toBe(1); |
🤖 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 `@e2e/promo-code-apply-without-ticket.spec.js` at line 94, Update the
validationCalls assertion in the ticket-selection test to require exactly one
deferred validation request by using an equality check against 1 instead of only
checking that the count is positive.
There was a problem hiding this comment.
@gcutrini Not changing this — the suggested assertion doesn't do what it claims. expect.poll re-evaluates until the assertion passes and then returns, so toBe(1) stops the moment the counter hits 1 and is just as blind to a later duplicate as toBeGreaterThan(0). Asserting "exactly one" would need a settle-then-check (e.g. await the ✓, then a bare expect(validationCalls).toBe(1) outside poll).
The count is also one by construction: discovery is stubbed empty, so discoveredPromoCode is null and onTicketSelected takes the isApplied && !isDiscoveredCode branch at src/hooks/usePromoCode.js:191-194, which calls onRevalidate once and returns. The ticket-sync effect at src/components/ticket-type/index.js:108-115 takes the updatedCurrentTicket path once a ticket is set and never revalidates.
This thread can be resolved.
There was a problem hiding this comment.
Pull request overview
This PR fixes a promo-code UX bug where applying a code before selecting a ticket could leave the promo input stuck in a perpetual “spinner” state by separating canonical promo signals (busy/invalid/suggested) from the displayed status and ensuring “busy” only reflects real in-flight work.
Changes:
- Reshaped
PROMO_STATUSinto display-only states and updated status derivation inusePromoCodeto project status from canonical signals. - Updated consumers to use the new statuses/signals (
isSuggested) and adjusted promo input rendering/locking accordingly. - Added/expanded unit tests and introduced a Playwright e2e spec covering “apply before ticket selection” and deferred validation on later ticket pick.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/utils/constants.js | Updates promo display status enum to new projected states. |
| src/hooks/usePromoCode.js | Derives canonical signals (isBusy, isInvalid, isSuggested) and projects display status; adjusts isReady and error derivation. |
| src/hooks/tests/usePromoCode.test.js | Updates status expectations and adds coverage for “applied without ticket” + deferred validation and isSuggested. |
| src/components/registration-form/index.js | Switches logic from checking PROMO_STATUS.SUGGESTED to promoState.isSuggested. |
| src/components/promocode-input/index.js | Updates lock/label/icon rendering for new status names (PROCESSING/APPLIED). |
| e2e/promo-code-apply-without-ticket.spec.js | Adds Playwright coverage ensuring no stuck spinner and validation triggers after ticket selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {promoStatus === PROMO_STATUS.PROCESSING && <span className={`${styles.statusIcon} ${styles.spinner}`} />} | ||
| {promoStatus === PROMO_STATUS.APPLIED && <span className={`${styles.statusIcon} ${styles.valid}`}>✓</span>} | ||
| {promoStatus === PROMO_STATUS.INVALID && <span className={`${styles.statusIcon} ${styles.invalid}`}>✕</span>} |
There was a problem hiding this comment.
@gcutrini Confirmed, though the load-bearing risk is sharper than "brittle if the naming strategy changes".
The selector isn't broken today: webpack.common.js:45 sets localIdentName: "[local]___[hash:base64:5]", and both webpack.dev.js and webpack.prod.js merge(common, …) without overriding it, so [class*="promoCodeInput"] [class*="spinner"] resolves in either build — the E2E job on this PR is green.
The problem is the failure mode. promoSpinner() is only ever used with toHaveCount(0) (spec lines 72 and 96). If that class naming ever changes, the locator matches nothing and toHaveCount(0) still passes — the regression test for the exact bug this PR fixes silently becomes a no-op. It fails green, not red, so nobody finds out.
The convention point holds too: line 52 is the only [class*=] selector in the entire e2e suite; every other spec selects via data-testid, and those hooks already live in production components (ticket-dropdown/index.js:40,43,52,59, plus login/ and login-passwordless/).
Same applies to page.locator('text=✓') at spec lines 71, 89 and 95 — an unscoped page-wide text match, right under the comment on lines 50-51 explaining why the spinner is scoped ("the dev harness renders an unrelated payment-section spinner elsewhere on the page"). It works only because ✓ happens to be unique in src/ today.
Suggest data-testid on all three status icons at promocode-input/index.js:94-96 (promo-spinner / promo-applied / promo-invalid), then both assertions become scoped and stable, and a renamed/removed icon fails the spec loudly instead of quietly.
There was a problem hiding this comment.
Done: promo-spinner, promo-applied and promo-invalid on the three status icons, and the specs use them instead of class substrings and the page-wide text match.
| if (suggestionActive && !suggestionDismissed) return PROMO_STATUS.SUGGESTED; | ||
| if (isBusy) return PROMO_STATUS.PROCESSING; | ||
| if (isInvalid) return PROMO_STATUS.INVALID; | ||
| if (isApplied) return PROMO_STATUS.APPLIED; |
There was a problem hiding this comment.
@gcutrini APPLIED conflates "the code is set" with "the backend confirmed it", so the green ✓ renders for a code that was never validated.
Applying a code with no ticket selected runs no validation at all: onApply only revalidates if (ticket) (line 241), and applyPromoCode (actions.js:211-220) just dispatches SET_CURRENT_PROMO_CODE — which sets promoCodeVerified: null (reducer.js:171) — and refetches the catalog. With a null verdict, the only thing that can produce INVALID is !hasTickets (line 57).
That guard never fires. RegularPromoCodeTicketTypesStrategy::getTicketTypes catches the validation exception, sets $this->promo_code = null and continues (summit-api, lines 104-108), then loops every sellable Audience_All type unconditionally (lines 113-126); for a code that doesn't exist at all, getPromoCodeByCode() returns null (SummitTicketTypeService.php:364) and the factory still builds the Regular strategy. So filter=promo_code==TYPO comes back with the full catalog, hasTickets is true, and a typo'd code rests as ✓ "Applied promo code:" with the input locked until the user picks a ticket. The new e2e spec encodes this — it mocks ticket-types/allowed to return tickets regardless of the code and then asserts text=✓.
This is the same conflation the PR description diagnoses one level up ("the 'code applied, no ticket picked yet' combination had no honest value"). The combination now has a value, but it's the success value. Suggest finishing the projection instead of widening APPLIED:
// constants.js
ACCEPTED: 'accepted', // applied, no backend verdict yet
// usePromoCode.js
if (isApplied) return promoCodeVerified === true
? PROMO_STATUS.APPLIED
: PROMO_STATUS.ACCEPTED;
// (add promoCodeVerified to the useMemo deps)In promocode-input: add ACCEPTED to isLocked, let it fall through to the same label branch as APPLIED (no new i18n key — "Applied promo code:" is already accurate), and keep the ✓ on APPLIED only. PROMO_STATUS isn't read anywhere else in src/, so that's the whole blast radius.
Heads-up on the cost: this changes the spec's own acceptance assertion at e2e/promo-code-apply-without-ticket.spec.js:70 from "✓ visible" to "locked input + Remove, no spinner" — the assertion that actually pins the bug (promoSpinner count 0) is unaffected — plus two unit expectations that currently expect APPLIED for the no-ticket case.
There was a problem hiding this comment.
Done, as UNVERIFIED rather than ACCEPTED. Applied but unconfirmed renders locked with Remove and no tick; the tick requires the API to have accepted the code for the selected ticket.
| || status === PROMO_STATUS.VALID; | ||
| // (nothing in flight and no rejection to block on). Ticket selection | ||
| // is enforced by its own gate. | ||
| const isReady = !isBusy && !isInvalid; |
There was a problem hiding this comment.
@gcutrini isReady no longer accounts for manualError, so a transient validation failure both shows a success ✓ next to the error message and re-opens the advance gate.
uicore's request helpers reject the returned promise on any non-2xx in addition to invoking the error handler (documented in the FN react-frontend skill, §Handling Rejections; visible in the bundled responseHandler in openstack-uicore-foundation/lib/utils/actions.js). On a 429 the reducer dispatches VALIDATE_PROMO_CODE_RATE_LIMITED, which clears promoCodeValidating but deliberately leaves promoCodeVerified at null (reducer.js:183-185); on a timeout — getRequest sets a 60s deadline — nothing sets it either. Both paths land in onRevalidate's catch and set manualError.
State at that point: isBusy false, isInvalid false, so status is APPLIED (green ✓ + "Applied promo code:") while validationError renders the red TicketNotice in the same panel, and isReady true re-enables Next (button-bar/index.js:38). Before this PR the same state was APPLYING, so isReady was false and Next stayed disabled. Discovered/auto-applied codes then advance unverified, since the pre-advance revalidation is gated on !promoState.isDiscoveredCode (registration-form/index.js:360).
Fix:
const isReady = !isBusy && !isInvalid && manualError == null;This is safe because setManualError is only ever written by handleValidationError — the form's unapplied-code warning is separate state (unappliedCodeWarning, registration-form/index.js:285). The comment at lines 28-31 saying manualError is written "by handleValidationError (API) or the form (unapplied-code warning)" is stale and worth correcting in the same change, since it makes this fix look unsafe to a future reader. The ✓ should also be suppressed while validationError is set.
There was a problem hiding this comment.
Applied as suggested, then deliberately reverted, flagging it rather than leaving you to find it.
Blocking the gate on the request error does stop the unverified advance, but it also blocks the retry: Next is the only thing that re-runs the validation, so a transient 500 left the user unable to continue or to try again, with only Remove.
isReady is now !isBusy && !isInvalid. Advancing re-validates every applied code, discovered ones included, and refuses to move on unless it succeeds, so an unverified code still cannot get through and pressing Next is the retry. Two e2e specs cover it: one where the retry succeeds and advances, one where validation keeps failing and it does not.
A code applied without a ticket runs no validation, and the catalog comes back populated even for a code that does not exist, so APPLIED rendered a success mark for codes the API never confirmed. Add UNVERIFIED for applied-without-verdict, keep APPLIED for a confirmed one, and block the advance gate while a request error is unresolved. Status icons gain test ids, plus a spec that asserts the spinner during a slow validation so the absence assertions cannot silently stop matching.
|
Both findings were real regressions, fixed in 43ceebc. The main one: the promo code showed a green checkmark before anything had verified it. Applying a code without a ticket selected runs no validation at all, and the ticket list comes back populated even for a code that does not exist, so a typo looked like a valid code. There is now a separate state for "code entered, not yet verified": the input still locks and shows Remove, but the checkmark only appears once the API confirms the code against a ticket. Named it UNVERIFIED rather than ACCEPTED, since "accepted" still implies the server said yes. It names what promoCodeVerified actually tells us: null means nothing has confirmed the code yet. Second one: a failed validation that returns no verdict, such as a rate limit or a timeout, left the Next button enabled and the checkmark showing next to the error message. Both are now suppressed while an error is unresolved. On the selectors: test ids added to the three status icons and the spec moved off class matching. That alone would not have fixed the failure mode you described, since the spinner was only ever asserted absent and toHaveCount(0) still passes when a locator matches nothing. So there is now a test that asserts the spinner is visible during a slow validation. Verified by renaming the test id: that test fails, the two absence assertions do not. On the CodeRabbit toBe(1) thread: agreed with your reasoning, no change made. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hooks/usePromoCode.js`:
- Around line 134-135: Update onRevalidate in usePromoCode to track a validation
generation or cancellation token and invalidate the previous request when a new
validation starts. Guard both success and failure continuations so only the
latest ticket can update readiness or call handleValidationError, and add
coverage for resolving the second validation before rejecting the first.
🪄 Autofix
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 Plus
Run ID: a1df798c-b4e7-4e46-981d-8fd91d583afc
📒 Files selected for processing (5)
e2e/promo-code-apply-without-ticket.spec.jssrc/components/promocode-input/index.jssrc/hooks/__tests__/usePromoCode.test.jssrc/hooks/usePromoCode.jssrc/utils/constants.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/promocode-input/index.js
- src/utils/constants.js
- src/hooks/tests/usePromoCode.test.js
Switching tickets leaves the earlier validation in flight, and its rejection was writing an error about a ticket the user already left, blocking Next and hiding the verified mark. Only the latest attempt now reports a result, and it owns clearing the auto-applied flag so callers no longer inspect a return value.
|
Confirmed. The impact is wider than a stale message: apiError also gates isReady and the status projection, so a superseded rejection blocked Next and hid the verified mark. Fixed in 52eb530. Each validation takes an attempt number and only the latest one may report a result. A superseded rejection is dropped instead of writing an error. Since both callers reacted to failure the same way, that side effect moved into onRevalidate behind the same guard, so the return value is gone and the callers just await. Not addressing the stale success case here. VALIDATE_PROMO_CODE_SUCCESS is dispatched inside the Redux action before the await resolves, so the hook cannot intercept it. That needs a token threaded through the action and reducer. The harmful shape, where the second validation fails and the first succeeds late, is already neutralised: the error keeps status at UNVERIFIED and isReady false, so no false success is shown. Added a test for the race, and verified it fails without the guard. |
| await validatePromoCode({ id: ticket.id, ticketQuantity: quantity, sub_type: ticket.sub_type }); | ||
| return true; | ||
| } catch (e) { | ||
| if (attempt !== latestValidation.current) return; |
There was a problem hiding this comment.
@gcutrini onRevalidate no longer returns a verdict, but its only external consumer still requires one — the ticket step now dead-ends for any manual promo code.
No path in the rewritten function returns a value: success falls out of the try, this bare return yields undefined, and the surviving-error path has no return either. registration-form/index.js:364 is unchanged in these two commits (git diff --stat b62a65d..HEAD on that file is empty) and still does:
let valid = false;
valid = await promoActions.onRevalidate(formValues.ticketType, data.ticketQuantity);
if (!valid) return;So valid is always undefined, !valid is always true, and changeStep(STEP_PERSONAL_INFO) is never reached. The branch guarding it is promoCode && !promoState.isDiscoveredCode — every applied code that isn't the discovered one, which on a summit with no discovery codes is every manually applied code. And because isReady is true after a successful validation, the Next button is enabled: the user clicks, sees the startWidgetLoading/stopWidgetLoading flash, and nothing happens. No error, no feedback. There is no other path out of the ticket step.
I recreated the two assertions this commit deleted (expect(valid).toBe(true) and .toBe(false)) and ran them against this head — both fail, which is only possible if the return is neither, i.e. undefined. Nothing else covers it: no test exercises handleAdvanceFromTicketStep, and the only e2e that clicks Next (promo-code-invalid-clears-warning.spec.js:45) types a code without applying it, so it exits on the unapplied-warning branch before ever reaching the revalidation.
A plain boolean can't be restored honestly, though — it collides with the supersession change: false for a superseded attempt blocks the user on a result we deliberately chose to ignore, and true advances on a code that was never confirmed. That's the same flattening of two independent concerns (verdict vs. relevance) this PR set out to remove one level up. Suggest making it explicit:
// onRevalidate
try {
await validatePromoCode({ ... });
return 'valid';
} catch (e) {
if (attempt !== latestValidation.current) return 'superseded';
handleValidationError(e);
setIsAutoApplied(false);
return 'invalid';
}
// registration-form/index.js:364
const verdict = await promoActions.onRevalidate(formValues.ticketType, data.ticketQuantity);
if (verdict !== 'valid') return; // 'superseded' neither advances nor shows an errorReading promoState.isReady after the await instead is not an option: the handler closes over the render's promoState, so the post-await read is stale — which is likely why the boolean was there in the first place.
There was a problem hiding this comment.
Restored as a boolean rather than the three-state value.
A superseded attempt can no longer record an answer, so false is accurate for the caller and shows no error. One rough edge remains and is worth naming: a superseded attempt returns false with nothing on screen, so that click does nothing visible. It requires a newer validation to exist, and that newer one drives the outcome.
Separately, validatePromoCode returns null without issuing a request when no code is applied. That now returns false rather than counting as success.
|
|
||
| // A ticket switch can leave an earlier validation in flight. Only the most | ||
| // recent attempt may report a result. | ||
| const latestValidation = useRef(0); |
There was a problem hiding this comment.
@gcutrini The generation token guards the hook's local error but not the Redux verdict, so the failure this was meant to stop still reproduces.
The dispatches happen inside the thunk, outside the ref's reach: validatePromoCode (actions.js:257-262) hands VALIDATE_PROMO_CODE_SUCCESS to getRequest, and a 404/412 goes through promoCodeErrorHandler (actions.js:76-89) which dispatches VALIDATE_PROMO_CODE_ERROR → promoCodeVerified: false (reducer.js:180-182). Neither is gated on attempt. And status, isInvalid and isReady all read promoCodeVerified, not apiError.
Concretely: the code is invalid for ticket A, the user switches to ticket B where it is valid. B resolves first → verified true. A rejects afterwards → the ref guard correctly suppresses A's message, but A's rejection still dispatches VALIDATE_PROMO_CODE_ERROR → verified false → isInvalid true → ✕ "Promo code entered is not valid." and isReady false, on ticket B. That is the exact symptom named in the thread this commit resolved ("the stale error then blocks isReady and displays an error for ticket B"), reached through Redux instead of through the hook.
Worth noting uicore does not save us here: getRequest aborts a prior in-flight request keyed by URL, but the validation URL carries filter[]=ticket_type_id==<id> (actions.js:253), so ticket A and ticket B are different keys and A is never aborted. The dropdown also stays clickable during validation (ticket-dropdown/index.js:40), so two quick picks are enough to reach this.
The new test can't catch it either: 'ignores a validation response that a later ticket switch superseded' pins promoCodeVerified: true as a constant prop — the exact value the late rejection flips in the real app — so it can only exercise the hook-local half of the guard.
Suggest carrying the attempt id to the boundary that writes the verdict (pass it into the thunk and drop the dispatch when it is stale), or keeping a verifiedForTicketId in the hook and deriving isInvalid from that instead of the global boolean.
There was a problem hiding this comment.
You were right about the mechanism, and the fix ended up going further than the suggestion.
Carrying the attempt id into the thunk works, but it needs the reducer to keep a second record of which attempt is current, and that record and the hook counter then have to agree. They could not always: the key was claimed after awaiting the access token, so a validation started second could be recorded first. Guarding that took the key, the applied code beside it, and an ordering check, and removing a code then retyping the same one still slipped through.
So the answer now lives in the hook, next to the counter that already decides which attempt counts. The reducer keeps the applied code and the discovered codes and records no answer at all, so there is nothing left to keep in step. registration-form feeds allowsReassign to its two consumers from the hook.
Your ticket A/B scenario has unit coverage that drives the real path, and every guard was checked by reverting it and confirming a test fails.
registration-form gates changeStep on onRevalidate return value, so dropping it left every manually applied promo code unable to leave the ticket step: Next flashed the loader and did nothing. It now reports whether the caller may advance, false for both a rejection and a superseded attempt, which is all the caller distinguishes. Adds the e2e that was missing for this path, so the regression cannot return silently.
promoCodeValidating was cleared only by the two actions that also write a verdict, so a validation ending without one (server error, timeout, dropped connection) left the field spinning for the rest of the session. Move the flag into usePromoCode next to applyingCode. getRequest rejects on every failure, so clearing it in the finally covers every outcome, including the ones no reducer case can see. Nothing outside the hook read it, so it comes out of the store entirely.
A failed request fed isReady, which disables the Next button, so a server error or timeout left the button dead: the user could neither retry the validation nor continue, with only a Remove to fall back on. Drop it from isReady and re-validate on advance instead, refusing to move on unless it succeeds. The user cannot proceed on an unverified code, and pressing Next is now the retry. Advancing re-validates discovered codes too. Their quantity caps make them as quantity-dependent as a manually entered one, and skipping them left the only codes that can be auto-applied with no way back from a failure.
…or it The modal renders a beat after the response lands, so an instantaneous visibility check could miss it and leave it overlaying the form, where it swallowed the next click and timed the test out under load.
onRevalidate signals a failed validation by returning false rather than throwing, so tryAutoApply returned true whether or not the code it had just applied verified, contradicting its documented contract.
Its last consumer went away when advancing started re-validating every applied code. It stays as an internal value; only the export and the tests reading it through the returned state go, so it no longer reads as covered API that nothing calls.
onApply cleared applyingCode only after awaiting revalidation. uicore aborts an in-flight request as soon as a newer one targets the same URL, and superagent never invokes the callback for an aborted request, so that promise neither resolves nor rejects and the flag was never cleared: the field spun forever and Next stayed disabled until a reload. Re-picking the already-selected ticket during an apply is enough to trigger it. The flag covers the apply request, which has already resolved by then, so clear it without waiting on revalidation, which tracks itself. Start the revalidation first so the two busy windows overlap; React 16 does not batch these, and clearing first renders a frame with neither set.
The reducer wrote whatever answer arrived last, while the hook's attempt counter decided which one the UI should trust, so a validation superseded by a ticket switch could still overwrite the newer answer. Keep the verdict where the request is awaited, so that counter is the only thing that may write one, and applying or removing a code clears it at the point that changes the code. The reducer keeps the applied code and the discovered codes. Advancing no longer counts a validation that never ran as success: with no code applied the request is skipped, and skipping is not a verdict.
Clearing the verdict was not enough on its own: the request still running kept the field busy and kept the right to write a verdict, so removing a code left the input read-only behind a spinner with Next disabled until that request landed, and its answer was then attributed to whatever the user had done since. Applying a code had the same gap, a whole round trip wide, because only the validation that followed advanced the counter. Withdraw the request instead: advance the attempt counter, clear the in-flight flag and drop the verdict together, wherever the applied code changes. Advancing the counter is what takes away the right to answer, so this has to happen in one place rather than at each site. The store also blanks the code without going through the hook, on checkout, logout and a cleared reservation, so do the same when it does. Two tests were passing for the wrong reason: one resolved a superseded attempt with no body, so the guard against a missing response was killing it rather than the supersede check, and one never reached the state its name claimed.
usePromoCode stored whether the API accepted the code for the selected ticket, but not which code was asked about, so anything that changed the applied code had to clear it. The store clears the code on checkout, logout and when the reservation is dropped, none of which go through the hook. With the code recorded, an answer about a code that is no longer applied stops counting on its own. The code is read when the validation starts rather than when onApply was created: applyPromoCode is a round trip, so that callback still holds the previous code and would file the answer against the one being replaced.
A server error, a timeout or a rate limit says nothing about the code, but it still cleared isAutoApplied, so the notice changed from "automatically applied" to plain "applied" and a later successful retry did not change it back. Only a 404 or 412 means the API turned the code down.
The stepper kept honouring a discovered code's per-account and available quantity caps while an outstanding request error meant the last answer no longer covered the current selection, which is the state the field already shows as unverified. The caps now survive a re-validation in flight instead of dropping out and briefly uncapping the stepper for a code that is still applied.
52eb530 to
ae8b6af
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (7)
src/hooks/usePromoCode.js (1)
65-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale comment about "raw Redux signals".
promoCodeVerifiedis no longer a Redux value. Line 53 derives it fromlastValidation. The comment sends a reader looking for a Redux field that this PR removed.📝 Proposed comment update
// --- Canonical signals --- -// The raw Redux signals can overlap (e.g. a stale promoCodeVerified=false -// persists while a re-validation is in flight), so precedence is encoded -// here, once, rather than in each consumer. +// The underlying signals can overlap (e.g. a recorded verified=false from an +// earlier attempt persists while a re-validation is in flight), so precedence +// is encoded here, once, rather than in each consumer.🤖 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/hooks/usePromoCode.js` around lines 65 - 68, Update the “Canonical signals” comment in usePromoCode to refer to the derived validation signals rather than raw Redux signals, while preserving its explanation of overlapping values and centralized precedence.src/reducer.js (1)
163-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
VALIDATE_PROMO_CODEandVALIDATE_PROMO_CODE_SUCCESSare now dispatched but unhandled.
validatePromoCodeinsrc/actions.js(lines 250-255) still passes both action creators togetRequest. The reducer no longer has cases for either, so they fall through todefault. There is no functional defect. The dispatches are now dead plumbing.
getRequestrequires the two action creators, so keeping them is acceptable. Consider a short comment at thegetRequestcall stating that the actions are intentionally unreduced, so a reader does not search for a missing case.🤖 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/reducer.js` at line 163, In validatePromoCode, add a brief comment at the getRequest call documenting that VALIDATE_PROMO_CODE and VALIDATE_PROMO_CODE_SUCCESS are intentionally dispatched without reducer cases. Keep both action creators passed to getRequest and make no reducer changes.src/hooks/__tests__/usePromoCode.test.js (2)
1424-1440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve the mock with the response shape the hook expects.
Every other test resolves
validatePromoCodewith{ response: ... }. Here it resolvesundefined. If the hook destructures the result, this path throws and is swallowed by the error handler, so the test would still pass while exercising the failure branch.Use the same shape for consistency.
♻️ Proposed change
- const validatePromoCode = jest.fn(() => Promise.resolve()); + const validatePromoCode = jest.fn(() => Promise.resolve({ response: {} }));🤖 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/hooks/__tests__/usePromoCode.test.js` around lines 1424 - 1440, Update the validatePromoCode mock in the “fires validatePromoCode when the user then picks a ticket” test to resolve with the response object shape expected by usePromoCode, matching the other tests instead of resolving undefined. Keep the existing assertion and ticket-selection behavior unchanged.
279-284: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove obsolete
promoCodeVerifiedtest inputs.Remove
promoCodeVerifiedfrom all test prop objects and rerender payloads. Update comments that describe it as a prop or Redux signal; the hook derives validation fromlastValidation.🤖 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/hooks/__tests__/usePromoCode.test.js` around lines 279 - 284, Remove the obsolete promoCodeVerified field from all test prop objects and rerender payloads in the usePromoCode test suite, including the shown processing case. Update related comments to describe validation as derived from lastValidation rather than a prop or Redux signal, while preserving the existing test behavior.e2e/promo-code-advance-from-ticket-step.spec.js (3)
17-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReturn the
route.fulfillpromise in the apply handler.The other handlers return the promise from the arrow function. This handler uses a block body and discards it. Playwright cannot then track the fulfillment, and a rejection after the page closes surfaces as an unhandled rejection.
♻️ Proposed change
await page.route('**/promo-codes/*/apply*', route => { // Last entry repeats, so a single-entry list means "always this". const next = validationResponses[Math.min(call++, validationResponses.length - 1)]; - route.fulfill({ + return route.fulfill({ status: next.status, contentType: 'application/json', body: JSON.stringify(next.body ?? validationResponse()), }); });🤖 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 `@e2e/promo-code-advance-from-ticket-step.spec.js` around lines 17 - 37, Update the **promo-code apply** route handler in `setup` to return the `route.fulfill` promise from its block-bodied callback, preserving the existing response selection and fulfillment payload.
92-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
setupandapplyCodein this test.Lines 93-104 repeat the route definitions from
setup, and lines 108-112 repeatapplyCode. A single always-200 entry gives the same behavior.♻️ Proposed change
test('Next advances to personal information with an applied promo code', async ({ page }) => { - await page.route('**/promo-codes/all/discover*', route => - route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(discoveryResponse([])) }) - ); - await page.route('**/ticket-types/allowed*', route => - route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(ticketTypesResponse([ticketType()])) }) - ); - await page.route('**/tax-types*', route => - route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(taxTypesResponse()) }) - ); - await page.route('**/promo-codes/*/apply*', route => - route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(validationResponse()) }) - ); - + await setup(page, [{ status: 200 }]); await page.goto('/'); - - await page.locator('[data-testid="ticket-dropdown"]').click(); - await page.locator('[data-testid="ticket-list"] >> text=Early Bird Ticket').click(); - - await page.fill('input[placeholder="Enter your promo code"]', 'EARLYCODE'); - await page.click('button:has-text("Apply")'); + await applyCode(page, 'EARLYCODE'); await expect(page.getByTestId('promo-applied')).toBeVisible();🤖 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 `@e2e/promo-code-advance-from-ticket-step.spec.js` around lines 92 - 112, Update the test to reuse the existing setup helper for the repeated route definitions and the applyCode helper for promo-code application. Remove the duplicated route stubs and direct promo-code input/click flow while preserving the test’s always-200 responses and subsequent navigation assertions.
84-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAbsence assertions run before the page settles in both e2e specs.
toHaveCount(0)returns as soon as the selector matches nothing, so an assertion placed directly after an asynchronous action can pass before the app would have rendered the element. Order each check so a positive, retrying assertion establishes the settled state first.
e2e/promo-code-advance-from-ticket-step.spec.js#L84-L89: assert a visible ticket-step element after dismissing the modal, then assert theBackbutton and required-fields note have count 0.e2e/promo-code-discovery.spec.js#L319-L321: assert theRemovebutton is enabled before assertingpromo-spinnerhas count 0.🤖 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 `@e2e/promo-code-advance-from-ticket-step.spec.js` around lines 84 - 89, The absence assertions occur before the UI has established its settled state. In e2e/promo-code-advance-from-ticket-step.spec.js lines 84-89, after dismissServerErrorModal, first add a visible, retrying assertion for a ticket-step element, then check that the Back button and required-fields note have count 0; in e2e/promo-code-discovery.spec.js lines 319-321, first assert the Remove button is enabled, then check that promo-spinner has count 0.
🤖 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 `@e2e/promo-code-advance-from-ticket-step.spec.js`:
- Around line 17-37: Update the **promo-code apply** route handler in `setup` to
return the `route.fulfill` promise from its block-bodied callback, preserving
the existing response selection and fulfillment payload.
- Around line 92-112: Update the test to reuse the existing setup helper for the
repeated route definitions and the applyCode helper for promo-code application.
Remove the duplicated route stubs and direct promo-code input/click flow while
preserving the test’s always-200 responses and subsequent navigation assertions.
- Around line 84-89: The absence assertions occur before the UI has established
its settled state. In e2e/promo-code-advance-from-ticket-step.spec.js lines
84-89, after dismissServerErrorModal, first add a visible, retrying assertion
for a ticket-step element, then check that the Back button and required-fields
note have count 0; in e2e/promo-code-discovery.spec.js lines 319-321, first
assert the Remove button is enabled, then check that promo-spinner has count 0.
In `@src/hooks/__tests__/usePromoCode.test.js`:
- Around line 1424-1440: Update the validatePromoCode mock in the “fires
validatePromoCode when the user then picks a ticket” test to resolve with the
response object shape expected by usePromoCode, matching the other tests instead
of resolving undefined. Keep the existing assertion and ticket-selection
behavior unchanged.
- Around line 279-284: Remove the obsolete promoCodeVerified field from all test
prop objects and rerender payloads in the usePromoCode test suite, including the
shown processing case. Update related comments to describe validation as derived
from lastValidation rather than a prop or Redux signal, while preserving the
existing test behavior.
In `@src/hooks/usePromoCode.js`:
- Around line 65-68: Update the “Canonical signals” comment in usePromoCode to
refer to the derived validation signals rather than raw Redux signals, while
preserving its explanation of overlapping values and centralized precedence.
In `@src/reducer.js`:
- Line 163: In validatePromoCode, add a brief comment at the getRequest call
documenting that VALIDATE_PROMO_CODE and VALIDATE_PROMO_CODE_SUCCESS are
intentionally dispatched without reducer cases. Keep both action creators passed
to getRequest and make no reducer changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9dc96075-89c6-4aa1-b182-c9700d8c8791
📒 Files selected for processing (9)
.gitignoree2e/promo-code-advance-from-ticket-step.spec.jse2e/promo-code-discovery.spec.jssrc/actions.jssrc/components/registration-form/__tests__/registration-form.test.jssrc/components/registration-form/index.jssrc/hooks/__tests__/usePromoCode.test.jssrc/hooks/usePromoCode.jssrc/reducer.js
💤 Files with no reviewable changes (1)
- src/components/registration-form/tests/registration-form.test.js
ref: https://app.clickup.com/t/86bb9xgex
Summary
No consumer outside the widget reads PROMO_STATUS, so the enum reshape is internal.
Summary by CodeRabbit
Bug Fixes
Tests