feat: auto-close dispute UI when order reaches terminal state #503
Conversation
- Auto-close dispute when seller releases sats (settledHoldInvoice/success) - Auto-close dispute on cooperative cancellation (canceled) - Differentiate closure reason via dispute.action: 'user-completed' vs 'cooperative-cancel' - Show distinct messages in dispute list and detail view per closure type - Add localization keys in en/es/it/fr for both closure messages
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughWhen an order reaches a terminal state (success / settledHoldInvoice) or is cooperatively canceled, the app auto-marks any associated in-progress dispute as Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as Mobile App
participant OrderState as OrderState
participant DisputeState as DisputeState
participant UI as Dispute UI
User->>App: Completes or cooperatively cancels order
App->>OrderState: Order transitions to terminal state (success / settledHoldInvoice / canceled)
OrderState->>OrderState: Find dispute by orderId
alt dispute exists and not already terminal
OrderState->>DisputeState: Set dispute.status = "closed", action = "user-completed" / "cooperative-cancel"
end
DisputeState->>UI: Emit updated dispute state
UI->>UI: Select localized message based on dispute.action
UI->>App: Render closed dispute message / update chips
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/order/models/order_state.dart`:
- Around line 194-213: The auto-close branch that sets updatedDispute to action
'cooperative-cancel' currently fires whenever newStatus == Status.canceled;
change it to only run for cooperative cancellation flows by adding a check for
the triggering cancellation action (e.g., require the cancellation Action to be
the cooperative cancel action rather than Action.cancel /
Action.holdInvoicePaymentCanceled / Action.adminCancel). Update the if condition
around updatedDispute.copyWith(... action: 'cooperative-cancel' ...) to verify
the specific cooperative cancellation action (use the existing Action
enum/variable used when status transitions occur) before assigning
'cooperative-cancel' and logging the cooperative message.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/data/models/dispute.dartlib/features/disputes/widgets/dispute_status_content.dartlib/features/order/models/order_state.dartlib/l10n/intl_en.arblib/l10n/intl_es.arblib/l10n/intl_fr.arblib/l10n/intl_it.arb
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/features/order/models/order_state.dart (1)
194-214: LGTM! Past review concern properly addressed.The cooperative-cancel branch now correctly guards against non-cooperative cancellation actions by checking
message.action == Action.cooperativeCancelAccepted(line 208). This ensures'cooperative-cancel'is only assigned for actual cooperative cancellations, not forAction.cancel,Action.holdInvoicePaymentCanceled, orAction.adminCancel.Optional refactor: The terminal dispute status list is duplicated. Consider extracting it:
♻️ Optional: Extract constant for terminal dispute statuses
+ static const _terminalDisputeStatuses = {'resolved', 'seller-refunded', 'closed'}; + // Auto-close dispute when order reaches terminal state by user action if (updatedDispute != null && - !const ['resolved', 'seller-refunded', 'closed'] - .contains(updatedDispute.status?.toLowerCase()) && + !_terminalDisputeStatuses.contains(updatedDispute.status?.toLowerCase()) && const [Status.success, Status.settledHoldInvoice].contains(newStatus)) { // ... } else if (updatedDispute != null && - !const ['resolved', 'seller-refunded', 'closed'] - .contains(updatedDispute.status?.toLowerCase()) && + !_terminalDisputeStatuses.contains(updatedDispute.status?.toLowerCase()) && newStatus == Status.canceled && message.action == Action.cooperativeCancelAccepted) {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/order/models/order_state.dart` around lines 194 - 214, Duplicate literal list of terminal dispute statuses should be extracted to a single constant; create a top-level const like terminalDisputeStatuses = ['resolved', 'seller-refunded', 'closed'] and replace both occurrences of the inline const list in the updatedDispute guards (the first if and the else-if that check updatedDispute.status?.toLowerCase()) with terminalDisputeStatuses to remove duplication and improve readability while keeping the existing logic in the order state update flow (refer to the updatedDispute null checks and the newStatus/ message.action conditions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/features/order/models/order_state.dart`:
- Around line 194-214: Duplicate literal list of terminal dispute statuses
should be extracted to a single constant; create a top-level const like
terminalDisputeStatuses = ['resolved', 'seller-refunded', 'closed'] and replace
both occurrences of the inline const list in the updatedDispute guards (the
first if and the else-if that check updatedDispute.status?.toLowerCase()) with
terminalDisputeStatuses to remove duplication and improve readability while
keeping the existing logic in the order state update flow (refer to the
updatedDispute null checks and the newStatus/ message.action conditions).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/architecture/ORDER_STATUS_HANDLING.mdlib/features/order/models/order_state.dart
✅ Files skipped from review due to trivial changes (1)
- docs/architecture/ORDER_STATUS_HANDLING.md
There was a problem hiding this comment.
Review: Auto-close dispute UI on terminal state
Overall assessment
The approach is sound — inferring dispute closure from order terminal state instead of subscribing to kind 38386 is pragmatic and avoids protocol expansion. The documentation is excellent and the rationale is well-articulated. However, there are issues that need addressing.
🔴 Critical: No tests
Zero test files were added or modified. This PR introduces branching logic in OrderState.updateWith() that determines dispute closure behavior based on status + action combinations. This is exactly the kind of logic that needs unit tests.
The repo's AGENTS.md states: "Add targeted scenarios when expanding complex logic or async workflows."
At minimum, tests should cover:
- Dispute auto-closes to
user-completedwhen status reachessuccess - Dispute auto-closes to
user-completedwhen status reachessettledHoldInvoice - Dispute auto-closes to
cooperative-cancelwhencooperativeCancelAccepted - Already-resolved dispute is NOT overwritten (status
resolvedstaysresolved) - Already-closed dispute is NOT overwritten (idempotency)
- Dispute with status
seller-refundedis NOT overwritten getLocalizedDescriptionreturns correct message foruser-completedactiongetLocalizedDescriptionreturns correct message forcooperative-cancelaction
🟡 DRY violation: duplicated guard condition
In order_state.dart lines 195-213, the guard condition is duplicated:
!const ['resolved', 'seller-refunded', 'closed']
.contains(updatedDispute.status?.toLowerCase())This appears twice in the if/else-if chain. Extract to a local variable:
final disputeAlreadyTerminal = const ['resolved', 'seller-refunded', 'closed']
.contains(updatedDispute?.status?.toLowerCase());
if (updatedDispute != null && !disputeAlreadyTerminal &&
const [Status.success, Status.settledHoldInvoice].contains(newStatus)) {
// ...
} else if (updatedDispute != null && !disputeAlreadyTerminal &&
newStatus == Status.canceled &&
message.action == Action.cooperativeCancelAccepted) {
// ...
}🟡 Magic strings for dispute status
The code uses raw strings 'closed', 'resolved', 'seller-refunded', 'user-completed', 'cooperative-cancel', 'admin-settled', 'admin-canceled' scattered across multiple files. These should be constants (e.g., on Dispute or a DisputeStatus class) to prevent typos and enable refactoring. This is a pre-existing problem that this PR extends — not a blocker for this PR specifically, but worth noting as tech debt.
🟡 closed maps to DisputeDescriptionKey.resolved — misleading?
In dispute.dart line 500, 'closed' falls into the same case as 'resolved':
case 'resolved':
case 'solved':
case 'closed':
return DisputeDescriptionKey.resolved;Then in getLocalizedDescription, the action field disambiguates. This works but is fragile — the description key says "resolved" when the dispute is actually "closed". If anyone adds logic that checks descriptionKey == DisputeDescriptionKey.resolved expecting admin resolution, it will incorrectly match user-closed disputes too. Consider adding a DisputeDescriptionKey.closed value to make the distinction explicit.
✅ What looks good
- CI passes, no conflicts
- Documentation is thorough — the
ORDER_STATUS_HANDLING.mdadditions explain the "why" clearly - Localization covers all 4 languages (EN/ES/FR/IT)
cooperativelyCanceled→ "Canceling" (orange) is correct — it IS a pending state, not a final one- The guard against overwriting already-terminal disputes is correct
- Using
message.action == Action.cooperativeCancelAcceptedto distinguish cooperative cancel from admin cancel is precise
Verdict
Request Changes — primarily for the missing tests. The auto-close logic in OrderState.updateWith() is critical path code for dispute handling. It must have test coverage before merging.
There was a problem hiding this comment.
All requested changes addressed in 8950a99:
- ✅ Tests added: 12 tests covering all 8 requested scenarios + 4 extra edge cases (admin-canceled, field preservation, no-dispute-present, description key mapping)
- ✅ DRY guard extracted:
disputeAlreadyTerminallocal variable replaces duplicated condition - ✅ CI passes
The magic strings note (closed, resolved, seller-refunded etc.) remains as acknowledged tech debt — not a blocker for this PR.
LGTM 👍
fix #494
Summary by CodeRabbit
New Features
Documentation
Tests