Skip to content

fix: transaction history POS#258

Merged
jakubuid merged 5 commits intodevelopfrom
pos_updates
Jan 23, 2026
Merged

fix: transaction history POS#258
jakubuid merged 5 commits intodevelopfrom
pos_updates

Conversation

@jakubuid
Copy link
Collaborator

Summary

  • Fix pagination race conditions by canceling in-flight requests when filter changes
  • Include existing data in LoadingMore state to prevent UI flicker and race conditions
  • Use Locale.ROOT for ISO 8601 date parsing to avoid locale-specific issues
  • Remove tokenSymbol from Transaction model (not returned by API)
  • Remove unused TOKEN_ADDRESS_TO_SYMBOL hardcoded mapping

Changes

  • POSViewModel: Added transactionLoadingJob to track and cancel in-flight requests
  • TransactionHistoryUiState: Changed LoadingMore from object to data class with transactions/stats
  • TransactionHistoryScreen: Fixed state access pattern, fixed date locale
  • Pos.kt: Removed tokenSymbol field from Transaction
  • Mapping.kt: Removed extractTokenSymbol() and token address mapping

Test plan

  • Verify transaction history loads correctly
  • Verify rapid filter changes don't cause duplicate transactions
  • Verify loading more transactions shows spinner without losing existing data
  • Verify date formatting works on devices with non-US locales

🤖 Generated with Claude Code

jakubuid and others added 4 commits January 22, 2026 09:12
- Fix pagination race conditions by canceling in-flight requests on filter change
- Include existing data in LoadingMore state to prevent UI flicker
- Use Locale.ROOT for ISO 8601 date parsing
- Remove tokenSymbol from Transaction (not returned by API)
- Remove unused TOKEN_ADDRESS_TO_SYMBOL mapping

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Jan 23, 2026

Claude finished @jakubuid's task in 1m 18s —— View job


Code Review Complete

Found 3 issue(s)

Issue 1: Potential race on loadedTransactions mutation

ID: posviewmodel-transactions-mutation-b3f7
File: sample/pos/src/main/kotlin/com/walletconnect/sample/pos/POSViewModel.kt:209
Severity: MEDIUM
Category: code_quality

Context:

  • Pattern: Line 209 mutates shared loadedTransactions list after async operation without synchronization
  • Risk: If job cancellation occurs between fold block and state mutation, another coroutine could start modifying the same list
  • Impact: Could lead to concurrent modification, duplicate transactions, or inconsistent state
  • Trigger: Rapid filter changes or pagination while previous request completes

Recommendation: Make loadedTransactions immutable per request:

result.fold(
    onSuccess = { historyResult ->
        val updatedList = if (refresh) {
            historyResult.transactions
        } else {
            loadedTransactions + historyResult.transactions
        }
        loadedTransactions.clear()
        loadedTransactions.addAll(updatedList)
        currentCursor = historyResult.nextCursor
        // ...

Issue 2: Missing CancellationException handling in fold block

ID: posviewmodel-cancellation-propagation-a9e2
File: sample/pos/src/main/kotlin/com/walletconnect/sample/pos/POSViewModel.kt:207-226
Severity: LOW
Category: code_quality

Context:

  • Pattern: result.fold() catches all exceptions including CancellationException at line 222
  • Risk: If PosClient.getTransactionHistory throws CancellationException during cancellation, fold's onFailure catches it instead of propagating
  • Impact: Cancelled jobs may emit Error state instead of silently cancelling, showing false errors to user
  • Trigger: Cancelling transactionLoadingJob while network request is in flight

Recommendation: Check for cancellation in onFailure block:

onFailure = { error ->
    if (error is CancellationException) throw error
    _transactionHistoryState.value = TransactionHistoryUiState.Error(
        error.message ?: "Failed to load transactions"
    )
}

Issue 3: Locale.ROOT used for output formatting

ID: transactionhistoryscreen-locale-output-d4c1
File: sample/pos/src/main/kotlin/com/walletconnect/sample/pos/screens/TransactionHistoryScreen.kt:412
Severity: LOW
Category: code_quality

Context:

  • Pattern: Line 412 uses SimpleDateFormat("MMM dd, yyyy HH:mm", Locale.getDefault()) for output, but comment at 410 says "Use Locale.ROOT for parsing ISO 8601 dates to avoid locale-specific parsing issues"
  • Risk: Comment is misleading - Locale.ROOT is correctly used for input parsing (line 411), but developer may think it applies to output too
  • Impact: No functional issue, but comment could confuse future maintainers
  • Trigger: Code review or maintenance

Recommendation: Clarify comment:

// Parse ISO 8601 with Locale.ROOT to avoid locale-specific parsing issues
val inputFormat = SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss", Locale.ROOT)
// Format output with user's locale for display
val outputFormat = SimpleDateFormat("MMM dd, yyyy HH:mm", Locale.getDefault())

Positive observations:

  • Good use of Job cancellation pattern (line 187)
  • LoadingMore state correctly includes existing data to prevent UI flicker
  • Proper guard in loadMoreTransactions (line 233)
  • CancellationException correctly rethrown in ApiClient (lines 289-290)

@jakubuid jakubuid changed the title fix: transaction history race conditions and API model cleanup fix: transaction history POS Jan 23, 2026
- Build transaction list immutably to avoid race conditions
- Propagate CancellationException instead of showing error state
- Clarify comment about Locale usage in date formatting

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Jan 23, 2026

Claude finished @jakubuid's task in 53s —— View job


🔄 Incremental Review:

✅ All Previously Reported Issues Resolved

Confirmed fixes:

  1. posviewmodel-transactions-mutation-b3f7 → Resolved (POSViewModel.kt:210-217)

    • Now builds list immutably using loadedTransactions + historyResult.transactions before clearing/updating
  2. posviewmodel-cancellation-propagation-a9e2 → Resolved (POSViewModel.kt:231)

    • Added if (error is CancellationException) throw error check in onFailure block
  3. transactionhistoryscreen-locale-output-d4c1 → Resolved (TransactionHistoryScreen.kt:410-411)

    • Clarified comment to distinguish parsing (Locale.ROOT) from output formatting (Locale.getDefault())

✅ No new issues found in latest commits

Code follows Kotlin conventions, proper coroutine handling, and WalletConnect SDK patterns.

@sonarqubecloud
Copy link

@jakubuid jakubuid merged commit b28eda0 into develop Jan 23, 2026
13 of 14 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant