Open
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded offline and online sponsor-cart payment workflows: new action types and thunks, expanded cart API payloads, reducer state for payment/profile/intent/offline payments, cart sub-routing and views (invoice/payment), Stripe integration, helpers/constants, i18n keys, and dependency updates. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant CartView as CartView
participant Redux as Redux
participant API as Sponsor API
participant PaymentView as PaymentView
participant Stripe as Stripe
User->>CartView: Click "Pay with Invoice" or "Pay by Card"
CartView->>Redux: dispatch(checkoutCart()) (skipped if PendingPayment)
Redux->>API: PUT /sponsor/.../carts/current/checkout?expand=...
API-->>Redux: CART_STATUS_UPDATED
Redux-->>CartView: state updated (cart)
alt Invoice flow
CartView->>Redux: dispatch(payWithInvoice(cart_id))
Redux->>API: POST /sponsor/.../payments {type: "Offline", cart_id}
API-->>Redux: OFFLINE_PAYMENT_CREATED
Redux-->>User: navigate -> /.../cart/invoice
else Online card flow
PaymentView->>Redux: dispatch(getPaymentProfile(cart.id))
Redux->>API: GET /sponsor/.../payment-profiles?cart_id=...
API-->>Redux: RECEIVE_PAYMENT_PROFILE
Redux->>API: POST/PUT -> create/update payment_intent
API-->>Redux: PAYMENT_INTENT_CREATED/UPDATED
PaymentView->>Stripe: collect payment details
Stripe->>API: confirm -> returns success
API-->>Redux: PAYMENT_CONFIRMED
Redux-->>User: navigate -> success/redirect URL
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/reducers/sponsors/sponsor-page-cart-list-reducer.js (1)
80-106:⚠️ Potential issue | 🟠 MajorHarden cart normalization for
CART_STATUS_UPDATEDresponses.Line 83 assumes
payload.response.formsis always present.checkoutCartuses a different endpoint (/carts/current/checkout) thanRECEIVE_SPONSOR_CART(/carts/current); if that response is partial or lacks aformsarray,.map()will throw and break checkout rendering.🔧 Proposed defensive fix
case CART_STATUS_UPDATED: case RECEIVE_SPONSOR_CART: { - const cart = payload.response; - cart.forms = cart.forms.map((form) => { + const rawCart = payload.response || {}; + const forms = Array.isArray(rawCart.forms) ? rawCart.forms : []; + const cart = { + ...rawCart, + forms: forms.map((form) => { const discountAmount = amountFromCents(form.discount_amount); const discountStr = form.discount_type === DISCOUNT_TYPES.RATE ? `${discountAmount} %` : `$${discountAmount}`; const discount = form.discount_amount ? discountStr : ""; return { ...form, addon_name: form.addon_name || "None", amount: currencyAmountFromCents(form.net_amount), discount, item_count: `${form.items.length} ${ form.items.length === 1 ? "item" : "items" }`, items: form.items.map((it) => ({ ...it, custom_rate: currencyAmountFromCents(it.custom_rate || 0) })) }; - }); - cart.total = currencyAmountFromCents(cart.net_amount); + }), + total: currencyAmountFromCents(rawCart.net_amount || 0) + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reducers/sponsors/sponsor-page-cart-list-reducer.js` around lines 80 - 106, The reducer handling CART_STATUS_UPDATED and RECEIVE_SPONSOR_CART assumes payload.response.forms exists and calls .map, which will throw if forms is missing; update the normalization in that case (inside the case block handling CART_STATUS_UPDATED / RECEIVE_SPONSOR_CART) to defensively read forms from payload.response (e.g., use an empty array when payload.response.forms is null/undefined or not an array) before mapping, so cart.forms is always a safe array for the subsequent mapping and downstream rendering; keep the same mapping logic for each form and ensure cart.total still derives from cart.net_amount as before.
🤖 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/actions/sponsor-cart-actions.js`:
- Around line 481-505: The payWithInvoice thunk dereferences
sponsorPageCartListState.cart without a null guard, swallows errors with
.catch(console.log) and never guarantees stopLoading(); fix payWithInvoice by
first checking that sponsorPageCartListState.cart exists (guard cart before
using cart.id), ensure postRequest errors are not swallowed (remove or replace
the silent .catch and rethrow or return the rejected promise so callers can
handle failures), and always call stopLoading() in a finally path (use
Promise.finally or try/catch/finally around the async flow) after dispatching
startLoading(); keep existing calls to postRequest,
createAction(OFFLINE_PAYMENT_CREATED), getSponsorCart()(dispatch, getState) and
snackbarErrorHandler but make sure errors propagate to callers instead of being
logged locally.
In `@src/pages/sponsors/sponsor-cart-tab/index.js`:
- Around line 37-39: The UI hides CartView when payCCView is truthy but no
credit-card view is rendered; remove payCCView from the cart-visibility logic
and stop toggling it until a CC/ACH view is implemented: update showCartView to
not include payCCView (remove "!payCCView"), and remove or noop any
setPayCCView(true) calls (e.g., the handler around where setPayCCView is set to
true in the click flow lines ~82-88) so clicking "pay with credit card or ach"
does not hide the cart until a real CC view is added; keep the payCCView state
or handlers only if they'll be implemented later.
---
Outside diff comments:
In `@src/reducers/sponsors/sponsor-page-cart-list-reducer.js`:
- Around line 80-106: The reducer handling CART_STATUS_UPDATED and
RECEIVE_SPONSOR_CART assumes payload.response.forms exists and calls .map, which
will throw if forms is missing; update the normalization in that case (inside
the case block handling CART_STATUS_UPDATED / RECEIVE_SPONSOR_CART) to
defensively read forms from payload.response (e.g., use an empty array when
payload.response.forms is null/undefined or not an array) before mapping, so
cart.forms is always a safe array for the subsequent mapping and downstream
rendering; keep the same mapping logic for each form and ensure cart.total still
derives from cart.net_amount as before.
🪄 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: 7bde1bc9-44c7-43aa-9620-165845d71aa6
📒 Files selected for processing (6)
src/actions/sponsor-cart-actions.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-cart-tab/components/cart-view.jssrc/pages/sponsors/sponsor-cart-tab/components/invoice-view.jssrc/pages/sponsors/sponsor-cart-tab/index.jssrc/reducers/sponsors/sponsor-page-cart-list-reducer.js
santipalenque
force-pushed
the
feature/sponsor-cart-payments
branch
from
8ab9218 to
90aff95
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.js (1)
80-86: Consider inlining these pass-through handlers.The handlers
handlePayCreditCardandhandlePayInvoiceare single-line pass-throughs to the props. They could be inlined directly in theonClickhandlers for conciseness.♻️ Optional simplification
- const handlePayCreditCard = () => { - onPayCC(); - }; - - const handlePayInvoice = () => { - onPayInvoice(); - }; @@ <Button variant="contained" color="primary" style={{ minWidth: 250 }} disabled={cart?.net_amount === 0} - onClick={handlePayCreditCard} + onClick={onPayCC} > @@ <Button variant="contained" color="primary" style={{ minWidth: 250 }} - onClick={handlePayInvoice} + onClick={onPayInvoice} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.js` around lines 80 - 86, The two functions handlePayCreditCard and handlePayInvoice are trivial pass-throughs; remove these two small wrappers and replace their usages by calling the prop handlers directly (onPayCC and onPayInvoice) in the corresponding onClick handlers where handlePayCreditCard and handlePayInvoice are currently referenced; update any references to handlePayCreditCard and handlePayInvoice to use onPayCC and onPayInvoice respectively to keep behavior identical while simplifying the component.src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/invoice-view.js (1)
44-48: Remove unnecessary Redux connection.The component only uses the
onCancelprop passed from its parent. ThemapStateToPropsspreadsshowAccessStatebut none of those props are used in the component. This adds unnecessary Redux subscription overhead and potential re-renders.♻️ Proposed simplification
-const mapStateToProps = ({ showAccessState }) => ({ - ...showAccessState -}); - -export default connect(mapStateToProps, {})(InvoiceView); +export default InvoiceView;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/invoice-view.js` around lines 44 - 48, mapStateToProps and the connect wrapper are unnecessary because InvoiceView only uses the onCancel prop; remove the mapStateToProps function and the connect import/wrapper and instead export the component directly (export default InvoiceView), ensuring InvoiceView still accepts onCancel from its parent and removing any references to showAccessState; also remove any unused imports (connect) to avoid extra Redux subscription and re-renders.
🤖 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/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/index.js`:
- Around line 49-55: handlePayInvoice currently proceeds to setInvoiceView(true)
regardless of failures because checkoutCart and payWithInvoice swallow errors;
change handlePayInvoice to await both operations inside a try/catch and only
call setInvoiceView(true) if both succeed, and in the catch branch propagate or
handle the error (e.g., show snackbar) without advancing the UI. Also update the
action thunks checkoutCart and payWithInvoice (in
src/actions/sponsor-cart-actions.js) to re-throw or reject when they encounter
failures so callers like handlePayInvoice can correctly detect and react to
errors.
---
Nitpick comments:
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.js`:
- Around line 80-86: The two functions handlePayCreditCard and handlePayInvoice
are trivial pass-throughs; remove these two small wrappers and replace their
usages by calling the prop handlers directly (onPayCC and onPayInvoice) in the
corresponding onClick handlers where handlePayCreditCard and handlePayInvoice
are currently referenced; update any references to handlePayCreditCard and
handlePayInvoice to use onPayCC and onPayInvoice respectively to keep behavior
identical while simplifying the component.
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/invoice-view.js`:
- Around line 44-48: mapStateToProps and the connect wrapper are unnecessary
because InvoiceView only uses the onCancel prop; remove the mapStateToProps
function and the connect import/wrapper and instead export the component
directly (export default InvoiceView), ensuring InvoiceView still accepts
onCancel from its parent and removing any references to showAccessState; also
remove any unused imports (connect) to avoid extra Redux subscription and
re-renders.
🪄 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: 92b50c27-8e1d-4044-8e1e-dea84531b580
📒 Files selected for processing (6)
src/actions/sponsor-cart-actions.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/invoice-view.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/index.jssrc/reducers/sponsors/sponsor-page-cart-list-reducer.js
✅ 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/reducers/sponsors/sponsor-page-cart-list-reducer.js
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/edit-form/__tests__/edit-cart-form.test.js (1)
303-314: Assert navigation target, not just call count.Both tests only verify
history.pushwas called once. Please assert the destination path too, so route regressions are caught.Proposed assertion tightening
- expect(mockHistoryPush).toHaveBeenCalledTimes(1); + expect(mockHistoryPush).toHaveBeenCalledWith( + "/app/events/1/sponsors/2/cart" + );Also applies to: 373-388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/edit-form/__tests__/edit-cart-form.test.js` around lines 303 - 314, The tests "clicking CANCEL navigates back" and the other similar test only assert mockHistoryPush was called once; update them to also assert the navigation destination by checking mockHistoryPush was called with the expected path (e.g., the route string or route object your app uses) instead of just verifying call count. Locate the tests where mockHistoryPush is asserted (references: the test named "clicking CANCEL navigates back" and the other test around lines asserting mockHistoryPush) and change the assertion to expect(mockHistoryPush).toHaveBeenCalledWith(<expectedPathOrRoute>), or toHaveBeenCalledWith(expect.stringContaining(...)) if using partial matching, so the destination is verified.
🤖 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/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/edit-form/__tests__/edit-cart-form.test.js`:
- Around line 176-179: The test helper buildMatch is using a default URL that
includes `/edit` which doesn't match the registered parent route
`/forms/:form_id`; update buildMatch so its default url is
`/app/events/1/sponsors/2/cart/forms/${formId}` (no `/edit`) to reflect the
actual route shape, and leave callers that need the edit path to pass an
explicit url containing `/edit`; adjust tests that relied on the old default to
either pass the explicit edit url or assert against the parent route as
appropriate.
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/edit-form/edit-cart-form.js`:
- Around line 35-44: The backToCart function is brittle because it slices the
URL by a fixed depth (slice(0, -2)); instead, derive the parent URL by removing
the trailing "/forms/:form_id" segment so it works regardless of added segments.
Replace the split/filter/slice/join logic with a robust transformation such as
using match.url.replace(/\/forms\/[^/]+$/, '') (or equivalent) and then
history.push to `/${newUrl}`; reference the backToCart function, match.url and
history.push when making the change.
In `@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/index.js`:
- Around line 85-89: The route in SponsorCartTab registers EditCartForm at path
`${match.url}/forms/:form_id` but the navigation in components/cart-view.js
pushes `cart/forms/${form.id}`, causing a doubled "cart" segment; update either
the navigator to push a path relative to the mounted tab (e.g.
`forms/${form.id}` and likewise for the invoice link) or change the navigation
to build the absolute path using match.url (e.g.
`${match.url}/forms/${form.id}`) so the pushed URL and the Route (EditCartForm
and the invoice route) resolve to the same pattern.
---
Nitpick comments:
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/edit-form/__tests__/edit-cart-form.test.js`:
- Around line 303-314: The tests "clicking CANCEL navigates back" and the other
similar test only assert mockHistoryPush was called once; update them to also
assert the navigation destination by checking mockHistoryPush was called with
the expected path (e.g., the route string or route object your app uses) instead
of just verifying call count. Locate the tests where mockHistoryPush is asserted
(references: the test named "clicking CANCEL navigates back" and the other test
around lines asserting mockHistoryPush) and change the assertion to
expect(mockHistoryPush).toHaveBeenCalledWith(<expectedPathOrRoute>), or
toHaveBeenCalledWith(expect.stringContaining(...)) if using partial matching, so
the destination is verified.
🪄 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: 11e0826c-3c71-4e0e-bbbd-7136340c0ff1
📒 Files selected for processing (8)
src/i18n/en.jsonsrc/pages/sponsors/sponsor-page/tabDefs.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/edit-form/__tests__/edit-cart-form.test.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/edit-form/edit-cart-form.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/invoice-view.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/payment-view.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/index.js
💤 Files with no reviewable changes (1)
- src/pages/sponsors/sponsor-page/tabDefs.js
✅ Files skipped from review due to trivial changes (1)
- src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/payment-view.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/invoice-view.js
- src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.js
Comment on lines
+176
to
+179
| const buildMatch = (formId, url) => ({ | ||
| params: { form_id: formId }, | ||
| url: url || `/app/events/1/sponsors/2/cart/forms/${formId}/edit` | ||
| }); |
There was a problem hiding this comment.
Test route shape does not match the actual registered edit route.
buildMatch defaults to /forms/${formId}/edit, but the parent route is /forms/:form_id. This can hide real navigation regressions.
Proposed fix
const buildMatch = (formId, url) => ({
params: { form_id: formId },
- url: url || `/app/events/1/sponsors/2/cart/forms/${formId}/edit`
+ url: url || `/app/events/1/sponsors/2/cart/forms/${formId}`
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const buildMatch = (formId, url) => ({ | |
| params: { form_id: formId }, | |
| url: url || `/app/events/1/sponsors/2/cart/forms/${formId}/edit` | |
| }); | |
| const buildMatch = (formId, url) => ({ | |
| params: { form_id: formId }, | |
| url: url || `/app/events/1/sponsors/2/cart/forms/${formId}` | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/edit-form/__tests__/edit-cart-form.test.js`
around lines 176 - 179, The test helper buildMatch is using a default URL that
includes `/edit` which doesn't match the registered parent route
`/forms/:form_id`; update buildMatch so its default url is
`/app/events/1/sponsors/2/cart/forms/${formId}` (no `/edit`) to reflect the
actual route shape, and leave callers that need the edit path to pass an
explicit url containing `/edit`; adjust tests that relied on the old default to
either pass the explicit edit url or assert against the parent route as
appropriate.
src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/edit-form/edit-cart-form.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/reducers/sponsors/sponsor-page-cart-list-reducer.js (1)
88-112:⚠️ Potential issue | 🟠 MajorAvoid mutating
payload.responseinside the reducer.Lines 89-112 mutate the object coming from
action.payload. Reducers should be immutable to avoid subtle state bugs.♻️ Proposed immutable fix
- const cart = payload.response; - cart.forms = cart.forms.map((form) => { + const rawCart = payload.response; + const forms = rawCart.forms.map((form) => { const discountAmount = amountFromCents(form.discount_amount); // this works also for rate from bps const discountStr = form.discount_type === DISCOUNT_TYPES.RATE ? `${discountAmount} %` : `$${discountAmount}`; const discount = form.discount_amount ? discountStr : ""; @@ - }); - cart.total = currencyAmountFromCents(cart.net_amount); + }); + const cart = { + ...rawCart, + forms, + total: currencyAmountFromCents(rawCart.net_amount) + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reducers/sponsors/sponsor-page-cart-list-reducer.js` around lines 88 - 112, The reducer currently mutates the action payload via the local const cart = payload.response and then assigns cart.forms = ... and cart.total = ..., which should be avoided; instead create a newCart object (e.g., const newCart = { ...cart, forms: cart.forms.map(...), total: currencyAmountFromCents(cart.net_amount) }) and perform immutable maps for forms and items (reuse amountFromCents, currencyAmountFromCents, DISCOUNT_TYPES logic inside the maps) so no properties of payload.response are reassigned; finally use newCart when returning state rather than mutating cart.
♻️ Duplicate comments (1)
src/actions/sponsor-cart-actions.js (1)
575-587:⚠️ Potential issue | 🟠 MajorSilent error swallowing hides failures from callers.
.catch(console.log)logs errors but converts rejections to resolved promises. Callers cannot distinguish success from failure, which may leave the UI in an inconsistent state (e.g., no payment intent but no error displayed).Consider propagating the error or handling it explicitly:
🔧 Proposed fix
return getRequest( null, createAction(RECEIVE_PAYMENT_PROFILE), `${window.PURCHASES_API_URL}/api/v1/summits/${currentSummit.id}/payment-profiles/SponsorServices`, (err, res) => (dispatch) => customErrorHandler(err, res, PaymentProfileNotFound)(dispatch, getState) )(params)(dispatch) .then(() => createPaymentIntent()(dispatch, getState)) - .catch(console.log) + .catch((err) => { + console.error(err); + return Promise.reject(err); + }) .finally(() => { dispatch(stopLoading()); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/sponsor-cart-actions.js` around lines 575 - 587, The current chain swallows errors via .catch(console.log) making callers think the operation succeeded; update the promise handling in the flow that calls getRequest(...)(params)(dispatch) so errors are propagated instead of absorbed: remove the trivial .catch(console.log) and either rethrow the caught error after logging or dispatch an error action (e.g., via customErrorHandler or a new failure action) so callers and UI can respond; keep the existing .finally(() => dispatch(stopLoading())) to ensure loading state is cleared while allowing createPaymentIntent, customErrorHandler, and callers to receive the rejection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 15-17: Reorder the environment keys in .env.example so
PURCHASES_API_SCOPES appears before PURCHASES_API_URL to satisfy dotenv-linter's
UnorderedKey rule; specifically move the PURCHASES_API_SCOPES line above the
PURCHASES_API_URL line (leave the value unchanged) and ensure the surrounding
keys (e.g., SPONSOR_USERS_API_URL) retain their relative order.
In `@package.json`:
- Line 95: The package "openstack-uicore-foundation" was bumped to 5.0.7-beta.1
which requires React ^17; update your dependencies by either (A) upgrading
"react" and "react-dom" in package.json to a ^17.x range and regenerate lockfile
(npm/yarn) and run the app/tests, or (B) revert "openstack-uicore-foundation" to
a stable 4.x version that supports React 16; target the approach you choose and
ensure the package name "openstack-uicore-foundation" and the peer deps "react"
and "react-dom" are consistent across package.json and the generated lockfile.
In `@src/actions/sponsor-cart-actions.js`:
- Around line 619-645: The confirmPayment thunk dereferences paymentIntent.id
from sponsorPageCartListState without checking for null, which can throw if no
payment intent exists; update confirmPayment to first read paymentIntent from
sponsorPageCartListState, check if it is present (and has an id), and if not
either dispatch an appropriate snackbar error (or return a rejected Promise)
instead of proceeding to call putRequest; ensure references to paymentIntent.id
are guarded so the URL is only built when paymentIntent && paymentIntent.id are
truthy.
- Around line 478-491: The current error handler for the putRequest chain uses
.catch(() => new Promise(() => {})) which returns a never-settling promise and
prevents the .finally() block (which dispatches stopLoading()) from running;
update the catch on the putRequest(..., snackbarErrorHandler)(params)(dispatch)
call to return a settled promise (for example Promise.resolve()) or otherwise
swallow the error in a way that resolves, so that .finally() always executes and
stopLoading() is dispatched; locate the chain involving putRequest,
createAction(CART_STATUS_UPDATED), snackbarErrorHandler and stopLoading to apply
this change.
- Around line 59-71: The customErrorHandler function currently dispatches
stopLoading() unconditionally causing duplicate stopLoading calls when callers
(e.g., getPaymentProfile) also call stopLoading() in their .finally(); remove
the stopLoading() dispatch from customErrorHandler and let callers manage
loading state in their .finally() blocks so error paths only call
authErrorHandler(err, res)(dispatch, getState) (and optional callback) without
altering loading state.
- Around line 526-550: The createPaymentIntent thunk assumes
sponsorPageCartListState.cart exists and calls cart.id; add a null-check for
sponsorPageCartListState.cart at the start of createPaymentIntent and fail
gracefully if missing (e.g., dispatch an error action or return a rejected
Promise with a clear message like "No cart available for sponsor" or throw a
descriptive Error). Update the function that builds payload (payload.cart_id =
cart.id) to only run after the guard and ensure any early-return path provides
the same shape (rejected promise or dispatched PAYMENT_INTENT_FAILED/other error
action) so callers handle the failure consistently.
- Around line 589-617: The updatePaymentIntent thunk dereferences cart.id and
paymentIntent.id without null checks which can throw if getState() returns
missing values; update the function (updatePaymentIntent) to guard cart and
paymentIntent (from sponsorPageCartListState) after getState(), and if either is
null/undefined return early or dispatch an error action (or call
snackbarErrorHandler) instead of calling putRequest; ensure the URL construction
uses the validated currentSummit.id, sponsor.id and the safely accessed cart.id
and paymentIntent.id before invoking putRequest.
In `@src/i18n/en.json`:
- Line 2564: Update the user-facing label for the payment table entry keyed by
"code" in the en.json localization: replace the incorrect string "From code"
with the correct "Form code" so the label reads "Form code" in the UI.
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.js`:
- Around line 93-107: The current handlers (the cartIsPendingPayment logic in
the anonymous checkout flow and handlePayInvoice using checkoutCart and
payWithInvoice) leave potential unhandled promise rejections; modify both flows
to handle failures explicitly by awaiting checkoutCart and payWithInvoice inside
try/catch blocks (or chaining .catch()) so navigation via
history.push("cart/payment") and history.push("cart/invoice") only happens on
success, and surface errors (e.g., call an error handler, set error state, or
show a toast) in the catch so failures are handled and logged instead of left
unhandled.
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/payment-view.js`:
- Around line 52-53: The render assumes nested state like currentSummit,
sponsor.company, and cart.notes are always present causing crashes; inside the
PaymentView component (e.g., where redirectUrl is built and where cart/sponsor
fields are referenced) add null-safe guards or an early return (e.g., if
(!currentSummit || !sponsor || !sponsor.company || !cart) return null/loading)
and replace direct property reads with optional chaining (currentSummit?.id,
sponsor?.company, cart?.notes) so the component won't access undefined values;
apply the same pattern to the other referenced blocks that read cart or sponsor
fields.
- Around line 80-83: The success handler handlePaymentSuccess currently calls
confirmPayment(paymentData.id) and assumes it always resolves; if confirmPayment
rejects the error is dropped—wrap the confirmation in a proper rejection path
(either convert handlePaymentSuccess to async and use try/catch around await
confirmPayment(paymentData.id) or add a .catch(...) to the returned promise) and
handle failures by reporting the error (e.g., call the existing error
handler/state updater or logger) and avoid calling history.push(redirectUrl) on
failure; keep references to confirmPayment, handlePaymentSuccess, history.push,
and redirectUrl when implementing the fix.
In `@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/helpers.js`:
- Around line 16-28: The code assumes it.meta_fields and mf.values exist which
can throw if payloads are null/undefined; update the logic around computing
formMetaFields and mapping mf.values so they use null-safe guards (e.g., treat
it.meta_fields as an empty array, use optional chaining or default empty arrays
for mf.values) before filtering/mapping, and ensure mf.current_value lookup uses
safe access (mf.values?.find(...) and fallback to mf.current_value) so item_name
construction in the block referencing showItemDescription is resilient to
missing metafields or values.
---
Outside diff comments:
In `@src/reducers/sponsors/sponsor-page-cart-list-reducer.js`:
- Around line 88-112: The reducer currently mutates the action payload via the
local const cart = payload.response and then assigns cart.forms = ... and
cart.total = ..., which should be avoided; instead create a newCart object
(e.g., const newCart = { ...cart, forms: cart.forms.map(...), total:
currencyAmountFromCents(cart.net_amount) }) and perform immutable maps for forms
and items (reuse amountFromCents, currencyAmountFromCents, DISCOUNT_TYPES logic
inside the maps) so no properties of payload.response are reassigned; finally
use newCart when returning state rather than mutating cart.
---
Duplicate comments:
In `@src/actions/sponsor-cart-actions.js`:
- Around line 575-587: The current chain swallows errors via .catch(console.log)
making callers think the operation succeeded; update the promise handling in the
flow that calls getRequest(...)(params)(dispatch) so errors are propagated
instead of absorbed: remove the trivial .catch(console.log) and either rethrow
the caught error after logging or dispatch an error action (e.g., via
customErrorHandler or a new failure action) so callers and UI can respond; keep
the existing .finally(() => dispatch(stopLoading())) to ensure loading state is
cleared while allowing createPaymentIntent, customErrorHandler, and callers to
receive the rejection.
🪄 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: 37ed0b67-4548-41a5-98b0-fd2b514a0fa5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
.env.examplepackage.jsonsrc/actions/sponsor-cart-actions.jssrc/app.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/invoice-view.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/payment-view.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/helpers.jssrc/reducers/sponsors/sponsor-page-cart-list-reducer.jssrc/utils/constants.js
✅ Files skipped from review due to trivial changes (1)
- src/utils/constants.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/invoice-view.js
src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.js
Outdated
Show resolved
Hide resolved
src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/payment-view.js
Show resolved
Hide resolved
src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/payment-view.js
Show resolved
Hide resolved
src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/helpers.js
Outdated
Show resolved
Hide resolved
santipalenque
force-pushed
the
feature/sponsor-cart-payments
branch
from
e0ec413 to
353dfa7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/actions/sponsor-cart-actions.js (1)
478-490:⚠️ Potential issue | 🔴 CriticalFix the thunk to resolve properly after errors so awaiting callers can complete their logic.
checkoutCart()returns a never-settling promise on error (line 490), which causesawait checkoutCart()in cart-view.js (lines 94, 103) to hang forever—the callers'catchblocks never run. SincesnackbarErrorHandleralready handles the error, either let the promise reject naturally or resolve after error handling, but do not returnnew Promise(() => {}).Error path issue in checkoutCart()
// Current code (lines 478-490) return ( putRequest( null, createAction(CART_STATUS_UPDATED), `${window.PURCHASES_API_URL}/api/v1/summits/${currentSummit.id}/sponsors/${sponsor.id}/carts/current/checkout`, {}, snackbarErrorHandler )(params)(dispatch) .finally(() => { dispatch(stopLoading()); }) // this swallows the error neither rejecting or resolving, so we don't need to handle it down the pipe .catch(() => new Promise(() => {})) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/sponsor-cart-actions.js` around lines 478 - 490, The checkoutCart thunk currently swallows errors by returning a never-settling promise in the final .catch, causing awaiting callers to hang; remove the trailing .catch(() => new Promise(() => {})) so the promise returned by the putRequest chain can either reject normally (allowing callers to catch) or resolve after snackbarErrorHandler runs—keep the existing finally that dispatches stopLoading() and leave snackbarErrorHandler in place; locate this behavior in the checkoutCart thunk (references: checkoutCart, putRequest, createAction(CART_STATUS_UPDATED), snackbarErrorHandler, stopLoading) and delete or replace the swallowing catch so errors propagate to callers.
🧹 Nitpick comments (1)
src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/payment-view.js (1)
82-85: Make this callback matchconfirmPayment's actual API.
confirmPaymentis defined as a zero-arg thunk insrc/actions/sponsor-cart-actions.js, sopaymentData.idis ignored here. Either drop the argument at the call site or thread the identifier into the thunk explicitly so the success flow is not misleading.♻️ Proposed cleanup
- const handlePaymentSuccess = (paymentData) => - confirmPayment(paymentData.id).then(() => { + const handlePaymentSuccess = () => + confirmPayment().then(() => { history.push(redirectUrl); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/payment-view.js` around lines 82 - 85, handlePaymentSuccess currently passes paymentData.id into confirmPayment but confirmPayment is a zero-arg thunk (so the id is ignored); either remove the unused parameter at the call site or update the thunk to accept and forward the id. Fix option A: change handlePaymentSuccess to call confirmPayment() (no args) and keep the existing zero-arg action; Fix option B: modify the action creator confirmPayment to accept an id parameter and use it inside the thunk, then call confirmPayment(paymentData.id) from handlePaymentSuccess so the identifier is actually threaded through; ensure history.push(redirectUrl) remains after the promise resolves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/actions/sponsor-cart-actions.js`:
- Around line 478-490: The checkoutCart thunk currently swallows errors by
returning a never-settling promise in the final .catch, causing awaiting callers
to hang; remove the trailing .catch(() => new Promise(() => {})) so the promise
returned by the putRequest chain can either reject normally (allowing callers to
catch) or resolve after snackbarErrorHandler runs—keep the existing finally that
dispatches stopLoading() and leave snackbarErrorHandler in place; locate this
behavior in the checkoutCart thunk (references: checkoutCart, putRequest,
createAction(CART_STATUS_UPDATED), snackbarErrorHandler, stopLoading) and delete
or replace the swallowing catch so errors propagate to callers.
---
Nitpick comments:
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/payment-view.js`:
- Around line 82-85: handlePaymentSuccess currently passes paymentData.id into
confirmPayment but confirmPayment is a zero-arg thunk (so the id is ignored);
either remove the unused parameter at the call site or update the thunk to
accept and forward the id. Fix option A: change handlePaymentSuccess to call
confirmPayment() (no args) and keep the existing zero-arg action; Fix option B:
modify the action creator confirmPayment to accept an id parameter and use it
inside the thunk, then call confirmPayment(paymentData.id) from
handlePaymentSuccess so the identifier is actually threaded through; ensure
history.push(redirectUrl) remains after the promise resolves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5422c40-ba0f-48e1-9a7a-cd36c91befce
📒 Files selected for processing (6)
src/actions/sponsor-cart-actions.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/edit-form/edit-cart-form.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/payment-view.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/helpers.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/helpers.js
- src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/edit-form/edit-cart-form.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://app.clickup.com/t/86b66n5kw
https://app.clickup.com/t/86b66n5ku
Summary by CodeRabbit
New Features
Refactor
Localization
Chores
Tests