FE: Refactor checkout to create orders after Stripe PaymentSheet success#413
Open
CherryCIC wants to merge 4 commits into
Open
FE: Refactor checkout to create orders after Stripe PaymentSheet success#413CherryCIC wants to merge 4 commits into
CherryCIC wants to merge 4 commits into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughRefactors checkout into a state-driven flow: new CheckoutFlowState, payment intent tracking, submit/retry flows, required paymentIntentId for order creation, enriched order payload builders, and related UI, model, build, string, and test updates. ChangesCheckout Feature Refactor
Build, Service & Minor UI Updates
Sequence DiagramsequenceDiagram
actor User
participant CheckoutPage
participant CheckoutVM as CheckoutViewModel
participant PaymentSheet as Payment Sheet
participant Repository as CheckoutRepository
participant Backend as Backend API
User->>CheckoutPage: Tap Submit or Retry
CheckoutPage->>CheckoutVM: submitCheckout() or retryOrderCreation()
alt First submission
CheckoutVM->>PaymentSheet: payWithPaymentSheet(amount)
PaymentSheet->>User: show payment UI
User->>PaymentSheet: complete payment
PaymentSheet->>CheckoutVM: return paymentIntentId
CheckoutVM->>CheckoutVM: store _lastPaymentIntentId
else Retry
CheckoutVM->>CheckoutVM: use stored _lastPaymentIntentId
end
CheckoutVM->>CheckoutVM: build order payload (shipping, carrier, address/pickup)
CheckoutVM->>Repository: createOrder(paymentIntentId, payload)
Repository->>Backend: POST /api/order/create
Backend->>Repository: returns order + shipment status
Repository->>CheckoutVM: response
CheckoutVM->>CheckoutVM: set checkoutFlowState (shipmentPending/created/failed)
CheckoutVM->>CheckoutPage: notify state change
CheckoutPage->>User: show toast/feedback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
CherryCIC
changed the title
![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)
lib/core/services/network/api_service.dart (1)
179-194:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid logging raw server error messages in checkout paths.
_handleBadResponsestopped logging payloads at Line 183, but Line 193 still logsserverMessageverbatim. Backend error strings can include PII/token-like values, so this remains a privacy/compliance risk.🔧 Suggested fix
switch (statusCode) { case 400: case 403: case 404: case 422: case 429: final serverMessage = _extractErrorMessage(data); - if (serverMessage != null) { - _log.info('Server validation error: $serverMessage'); - } + if (serverMessage != null) { + _log.info('Server validation error received (redacted).'); + } return ErrorStrings.apiError;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/core/services/network/api_service.dart` around lines 179 - 194, In _handleBadResponse, avoid logging the raw server error string returned by _extractErrorMessage to prevent leaking PII/tokens; instead sanitize or redact the value before logging (e.g., replace long token-like substrings with "<redacted>" or only log a generic severity/type and truncated length), or log only the existence of a server validation error and its status code; update the branch that currently does _log.info('Server validation error: $serverMessage') to use the redacted/sanitizedServerMessage (or a generic message) and add a short comment pointing to _extractErrorMessage for future maintainers.
🧹 Nitpick comments (4)
lib/features/profile/profile_page.dart (1)
14-14: 💤 Low valueUnrelated change: consider removing from this PR.
While adding
constto the constructor is a valid Flutter optimization, this ProfilePage change is unrelated to the checkout flow refactoring described in the PR objectives. Per the PR summary, reviewers should ignore unrelated file changes unless they touch the same flows—this file does not.Consider reverting this change and including it in a separate housekeeping PR to keep this PR focused on the checkout refactoring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/profile/profile_page.dart` at line 14, Revert the unrelated constructor change in the ProfilePage widget by removing the added const modifier on the ProfilePage constructor (the const ProfilePage({super.key}) change); restore the original constructor signature used before this PR so this file no longer contains unrelated housekeeping edits and can be moved to a separate follow-up PR.lib/features/products/product_page.dart (1)
89-92: ⚡ Quick winAvoid blank trailing content for inactive donor discount state.
Returning
''for the inactive case removes context in this highlight row; keeping explicit inactive detail reads clearer.♻️ Suggested adjustment
final donorDiscountDetail = isDonorDiscountActive ? AppStrings.productPageBuy2Get1HalfPrice - : ''; + : AppStrings.productPageDonorDiscountInactiveDetail;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/products/product_page.dart` around lines 89 - 92, The highlight row currently sets donorDiscountDetail to an empty string when isDonorDiscountActive is false, which yields blank trailing content; change the ternary fallback to an explicit inactive message (e.g., replace '' with a dedicated constant such as AppStrings.productPageDonorDiscountInactive or a clear literal like 'Not available') so donorDiscountDetail always conveys state; update AppStrings to include productPageDonorDiscountInactive if it doesn't exist and ensure any UI that displays donorDiscountDetail handles that explicit inactive text.lib/features/checkout/checkout_page.dart (1)
190-202: 💤 Low valueRedundant validation checks.
These validation checks for
hasValidDeliveryDetailsandhasSelectedPaymentMethodare unreachable whencanRetryOrderCreationis false, because the button is only enabled whencanSubmitCheckoutis true—which already guarantees both conditions are satisfied.Consider removing them to reduce cognitive load, or keep them as explicit defensive guards if intentional.
♻️ Proposed simplification
if (canRetryOrderCreation) { setState(() => _errorMessage = ''); await viewModel.retryOrderCreation(); return; } - if (!viewModel.hasValidDeliveryDetails) { - setState(() { - _errorMessage = AppStrings.checkoutDeliveryAddressRequired; - }); - return; - } - - if (!hasSelectedPaymentMethod) { - setState(() { - _errorMessage = AppStrings.checkoutPaymentMethodRequired; - }); - return; - } - setState(() => _errorMessage = ''); await viewModel.submitCheckout();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/checkout/checkout_page.dart` around lines 190 - 202, The two validation blocks checking viewModel.hasValidDeliveryDetails and hasSelectedPaymentMethod (which set _errorMessage to AppStrings.checkoutDeliveryAddressRequired and AppStrings.checkoutPaymentMethodRequired) are redundant because canSubmitCheckout already guarantees those conditions when the button is enabled; remove those if statements and their setState calls from the onPressed flow in checkout_page.dart (leaving canRetryOrderCreation and other retry logic intact) to simplify the method and reduce cognitive load, and run/update any tests that asserted those specific error messages if present.lib/features/checkout/checkout_view_model.dart (1)
659-675: ⚡ Quick winConsider injecting
FirebaseAuthfor testability.Directly accessing
FirebaseAuth.instance.currentUsermakes this method difficult to unit test without mocking Firebase at the global instance level. The try-catch fallback works but obscures test behavior.Consider injecting
FirebaseAuth(or a user provider interface) into the ViewModel constructor to improve testability and align with the existing dependency injection pattern used forcheckoutRepositoryandnavigator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/checkout/checkout_view_model.dart` around lines 659 - 675, The _resolveShippingName method uses FirebaseAuth.instance.currentUser which hinders unit testing; inject a FirebaseAuth (or an abstraction like UserProvider) into the ViewModel constructor, store it as a private field (e.g., _firebaseAuth or _userProvider), and update _resolveShippingName to read currentUser from that injected dependency instead of FirebaseAuth.instance; update the ViewModel constructor signature (where checkoutRepository and navigator are injected) to accept the new dependency and adjust tests to pass a mock/fake FirebaseAuth or provider while preserving the existing try/catch and 'cherry customer' fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/core/config/app_strings.dart`:
- Around line 139-140: Update the two user-facing string constants to correct
grammar: change giveYourBuyerText (currently "Give your buyer the option pick a
cause they care about.") to include the missing word (e.g., "Give your buyer the
option to pick a cause they care about.") and change easyWayText (currently "Its
an easy way to make your listing more impactful.") to use the
contraction/apostrophe or possessive correctly (e.g., "It's an easy way to make
your listing more impactful."). Ensure you edit the values of giveYourBuyerText
and easyWayText in app_strings.dart accordingly.
In `@lib/features/donation/widgets/donation_dropdown_field.dart`:
- Line 76: The dropdown is using initialValue (set in initState) so it never
reflects parent changes; replace initialValue with the controlling value
property on DropdownButtonFormField and ensure the local selectedDropdownItem is
kept in sync by updating it in didUpdateWidget when widget.selectedValue changes
(or simply use widget.selectedValue directly as the value); update
DropdownButtonFormField to use value: selectedDropdownItem (or
widget.selectedValue) and add a didUpdateWidget override to
setState(selectedDropdownItem = widget.selectedValue) when the incoming prop
differs.
---
Outside diff comments:
In `@lib/core/services/network/api_service.dart`:
- Around line 179-194: In _handleBadResponse, avoid logging the raw server error
string returned by _extractErrorMessage to prevent leaking PII/tokens; instead
sanitize or redact the value before logging (e.g., replace long token-like
substrings with "<redacted>" or only log a generic severity/type and truncated
length), or log only the existence of a server validation error and its status
code; update the branch that currently does _log.info('Server validation error:
$serverMessage') to use the redacted/sanitizedServerMessage (or a generic
message) and add a short comment pointing to _extractErrorMessage for future
maintainers.
---
Nitpick comments:
In `@lib/features/checkout/checkout_page.dart`:
- Around line 190-202: The two validation blocks checking
viewModel.hasValidDeliveryDetails and hasSelectedPaymentMethod (which set
_errorMessage to AppStrings.checkoutDeliveryAddressRequired and
AppStrings.checkoutPaymentMethodRequired) are redundant because
canSubmitCheckout already guarantees those conditions when the button is
enabled; remove those if statements and their setState calls from the onPressed
flow in checkout_page.dart (leaving canRetryOrderCreation and other retry logic
intact) to simplify the method and reduce cognitive load, and run/update any
tests that asserted those specific error messages if present.
In `@lib/features/checkout/checkout_view_model.dart`:
- Around line 659-675: The _resolveShippingName method uses
FirebaseAuth.instance.currentUser which hinders unit testing; inject a
FirebaseAuth (or an abstraction like UserProvider) into the ViewModel
constructor, store it as a private field (e.g., _firebaseAuth or _userProvider),
and update _resolveShippingName to read currentUser from that injected
dependency instead of FirebaseAuth.instance; update the ViewModel constructor
signature (where checkoutRepository and navigator are injected) to accept the
new dependency and adjust tests to pass a mock/fake FirebaseAuth or provider
while preserving the existing try/catch and 'cherry customer' fallback behavior.
In `@lib/features/products/product_page.dart`:
- Around line 89-92: The highlight row currently sets donorDiscountDetail to an
empty string when isDonorDiscountActive is false, which yields blank trailing
content; change the ternary fallback to an explicit inactive message (e.g.,
replace '' with a dedicated constant such as
AppStrings.productPageDonorDiscountInactive or a clear literal like 'Not
available') so donorDiscountDetail always conveys state; update AppStrings to
include productPageDonorDiscountInactive if it doesn't exist and ensure any UI
that displays donorDiscountDetail handles that explicit inactive text.
In `@lib/features/profile/profile_page.dart`:
- Line 14: Revert the unrelated constructor change in the ProfilePage widget by
removing the added const modifier on the ProfilePage constructor (the const
ProfilePage({super.key}) change); restore the original constructor signature
used before this PR so this file no longer contains unrelated housekeeping edits
and can be moved to a separate follow-up PR.
🪄 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 Plus
Run ID: 72833e98-9765-4625-8ff1-2c253242d546
📒 Files selected for processing (19)
android/app/build.gradle.ktslib/core/config/app_strings.dartlib/core/services/firebase_auth_service.dartlib/core/services/network/api_service.dartlib/features/charity_page/charity_model.dartlib/features/checkout/checkout_page.dartlib/features/checkout/checkout_repository.dartlib/features/checkout/checkout_view_model.dartlib/features/checkout/models/payment_intent.dartlib/features/checkout/models/payment_intent.g.dartlib/features/donation/widgets/donation_dropdown_field.dartlib/features/donation/widgets/photo_tips_bar.dartlib/features/password_flow/widgets/hidden_password_field.dartlib/features/products/product_page.dartlib/features/profile/profile_page.dartlib/main.darttest/auth_ui_test.mocks.darttest/checkout_view_model_test.darttest/checkout_view_model_test.mocks.dart
💤 Files with no reviewable changes (3)
- lib/features/donation/widgets/photo_tips_bar.dart
- lib/features/password_flow/widgets/hidden_password_field.dart
- lib/features/charity_page/charity_model.dart
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c38d7abbf3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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)
lib/features/checkout/checkout_view_model.dart (1)
273-278:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
processCheckout()still blocks valid pickup checkoutsLine 274-275 still enforces
canCheckout+validateShippingAddress(), which requires a shipping address even for pickup orders. That bypasses the newhasValidDeliveryDetailslogic and can reject valid pickup flows.Suggested fix
Future<bool> processCheckout() async { - if (!canCheckout) return false; - if (!validateShippingAddress()) return false; + if (!hasPaymentMethod) return false; + if (!hasValidDeliveryDetails) return false; await submitCheckout(); return hasCompletedCheckout; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/checkout/checkout_view_model.dart` around lines 273 - 278, processCheckout currently blocks pickup orders by always calling validateShippingAddress(); update it to allow pickup by replacing the unconditional validateShippingAddress() call with the new hasValidDeliveryDetails check (or conditionally call validateShippingAddress() only when the order requires delivery). In practice, inside Future<bool> processCheckout() keep the canCheckout guard, then ensure you verify delivery details via hasValidDeliveryDetails (or if deliverySelected then validateShippingAddress()), call submitCheckout() as before, and return hasCompletedCheckout; reference the processCheckout, validateShippingAddress, hasValidDeliveryDetails, submitCheckout, and hasCompletedCheckout symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/checkout/checkout_view_model.dart`:
- Around line 578-600: _buildOrderPayload currently charges the whole basket but
only includes the first product (productId/productName), which can cause
mismatched metadata; update _buildOrderPayload to include a full items array
derived from basketItems (e.g., each item's id, name, quantity,
unitPriceMinorUnits and lineTotalMinorUnits) and remove or deprecate the single
productId/productName fields so the payload reflects the charged amount; ensure
the items array construction uses the same helpers as amount calculation
(_toMinorUnits and any quantity logic) and include any summary fields
(totalAmount, totalQuantity) if needed for backend reconciliation.
- Around line 697-708: The function _resolveShipmentFlowState currently defaults
unknown/absent statuses to CheckoutFlowState.shipmentCreated; change this so
that when _shipmentStatusFromResponse(payload) returns null/empty or an
unrecognized status the function returns CheckoutFlowState.shipmentPending (i.e.
treat unknown as unresolved), by adding an explicit null/empty check on
shipmentStatus before the switch and making the default branch return
shipmentPending; update any callers if they relied on the previous default
behavior.
---
Outside diff comments:
In `@lib/features/checkout/checkout_view_model.dart`:
- Around line 273-278: processCheckout currently blocks pickup orders by always
calling validateShippingAddress(); update it to allow pickup by replacing the
unconditional validateShippingAddress() call with the new
hasValidDeliveryDetails check (or conditionally call validateShippingAddress()
only when the order requires delivery). In practice, inside Future<bool>
processCheckout() keep the canCheckout guard, then ensure you verify delivery
details via hasValidDeliveryDetails (or if deliverySelected then
validateShippingAddress()), call submitCheckout() as before, and return
hasCompletedCheckout; reference the processCheckout, validateShippingAddress,
hasValidDeliveryDetails, submitCheckout, and hasCompletedCheckout symbols when
making the change.
🪄 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 Plus
Run ID: 3d6bd5d3-46b8-4e35-87ab-56814708be9c
📒 Files selected for processing (1)
lib/features/checkout/checkout_view_model.dart
JaseElder
requested changes
May 11, 2026
Contributor
JaseElder
left a comment
JaseElder
left a comment
There was a problem hiding this comment.
The coderabbit comments need to be considered and implemented if necessary
Apply safe review fixes for PR #413: restore root dotenv loading, keep pickup checkout validation aligned with delivery choice, prevent repeat payment attempts after shipment failure, sync the donation dropdown with parent reset state, and correct donation helper copy. Type: frontend UI/state and configuration cleanup. Backend/API contract is unchanged; the multi-item payload and unknown-shipment-status CodeRabbit suggestions were reviewed but not applied because they conflict with the documented /api/order/create contract. Testing: dart format was run on touched files before staging; git diff --check passed. flutter analyze and the focused checkout ViewModel test previously hung in this local toolchain, so they were not completed locally.
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Collaborator
Author
|
@coderabbitai what's the verdict? @JaseElder I believe I have made the relevant changes |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull the current review state from the PR. 🏁 Script executed: Length of output: 93733
Status: 🔴 Changes Requested (blocked by Good news — two items from my first review are marked as resolved in commit 🟠 Major — Must Address
💤 Lower Priority (Nitpicks — address if feasible)
The dotenv revert and the two |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I'm hoping this supports the frontend of #397
POST /api/order/createafter Stripe PaymentSheet completes successfully.paymentIntentId, while keeping the client-secret fallback for the current backend response shape.shipmentStatus: pendinghandling.Notes
paymentIntentId,deliveryType, shipping option fields, andGBcountry codes.shipmentStatus: pendingis treated as a partial success, which means checkout completion now depends on shipment status as well as order creation.Testing
flutter analyzeand the focused Flutter test run were attempted, but both hung in this toolchain and were stopped. No full test run completed; however, I believe this to be a local issue on my device.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests