Skip to content

feat: Add transaction feedback and improve balance visibility - #23

Merged
evanmcfarland merged 1 commit into
mainfrom
feature/ux-feedback-improvements
Oct 10, 2025
Merged

evanmcfarland merged 1 commit into
mainfrom
feature/ux-feedback-improvements

Conversation

@evanmcfarland

Copy link
Copy Markdown
Member

Summary

Fixes critical UX issues where users had no confirmation after minting/redeeming ICPI and couldn't easily see their balance.

Changes

1. Transaction Success Feedback (TradeInterface.tsx)

  • Added successMessage state to show confirmation after mint/redeem
  • Displays green success message with transaction details
  • Auto-clears after 5 seconds

2. Immediate Balance Updates (useICPI.ts)

  • Added TOTAL_SUPPLY to query invalidation lists in both mutations
  • Balance updates now appear within 1-2 seconds instead of 30 seconds

3. User Balance Visibility (Dashboard.tsx)

  • Changed header from "SUPPLY" (total circulation) to "YOUR BALANCE" (user holdings)
  • Changed to green color for better visibility
  • Shows 6 decimals for precision

Impact

Before:

  • User mints → spinner stops → ??? (no feedback)
  • Header shows "SUPPLY: 0.00 ICPI" (confusing - shows total, not user balance)
  • Must wait up to 30s to see balance update

After:

  • User mints → green message: "Successfully minted X ICPI" ✅
  • Header shows "YOUR BALANCE: X.XXXXXX ICPI" in green ✅
  • Balance updates in 1-2s ✅

Code Quality

  • Lines added: 12 lines
  • New dependencies: 0
  • Tech debt: 0
  • Follows existing error message patterns
  • No external toast libraries needed

Testing

Deployed to mainnet and tested:

  • Mint operation shows success message
  • Redeem operation shows success message
  • Header displays user balance correctly
  • Balance updates immediately after transactions
  • Success messages auto-clear after 5 seconds

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

@claude

claude Bot commented Oct 10, 2025

Copy link
Copy Markdown

Code Review - PR #23: Transaction Feedback and Balance Visibility

Summary

This PR addresses critical UX issues by adding transaction success feedback and improving balance visibility. The implementation is clean, well-thought-out, and follows existing patterns. Overall, this is ready to merge with some minor considerations noted below.


✅ Strengths

1. Excellent UX Improvements

  • Success messages provide immediate feedback (5s auto-clear is perfect)
  • Changing from "SUPPLY" to "YOUR BALANCE" significantly improves clarity
  • Green color (#00FF41) for user balance is attention-grabbing and consistent with success messages
  • 6-decimal precision for ICPI balance is appropriate

2. Query Invalidation Fix

  • Adding TOTAL_SUPPLY to invalidation lists is the correct solution
  • This ensures the header updates quickly after mint/redeem operations
  • Smart use of React Query's cache invalidation pattern

3. Code Quality

  • Minimal changes (20 additions, 2 deletions) - adheres to "do one thing well"
  • No new dependencies introduced
  • Follows existing error handling patterns
  • Proper state management with cleanup (setTimeout clear)

💡 Suggestions & Observations

1. Race Condition in Success Message (Minor)

In TradeInterface.tsx:80 and TradeInterface.tsx:117, the success message uses values calculated before the operation:

```typescript
setSuccessMessage(`Successfully minted ${mintReceive.toFixed(6)} ICPI`)
```

Issue: If the user changes the input field while the transaction is processing, mintReceive could be stale when the success message displays.

Suggestion:
```typescript
// Capture the value before the async call
const expectedReceive = mintReceive
await onMint(amount)
setMintAmount('')
setSuccessMessage(`Successfully minted ${expectedReceive.toFixed(6)} ICPI`)
```

Same applies to the redeem operation with redeemReceive.

Priority: Low (edge case, input is disabled during operation anyway)


2. Success Message Timing with Error Clearing (Observation)

When an error occurs, both success and error messages are cleared:
```typescript
} catch (err: any) {
setSuccessMessage('') // This line
const errMsg = err.message || ''
```

This is actually correct behavior, but it's worth noting that if a previous success message is still visible (within the 5s window) when a new operation fails, it will be cleared. This is good defensive programming.


3. Consider Cleanup on Component Unmount (Minor)

The setTimeout calls at lines 81 and 118 don't clean up if the component unmounts:

```typescript
setTimeout(() => setSuccessMessage(''), 5000)
```

Suggestion:
```typescript
const timeoutId = setTimeout(() => setSuccessMessage(''), 5000)
// Store and clear in useEffect cleanup if needed
```

Priority: Very Low (5s is short, React 18 handles this reasonably well)


4. Redeem Success Message Length (UX Note)

For redemptions with multiple tokens, the success message could get quite long:
```typescript
const tokenList = redeemReceive.map(t => `${t.amount.toFixed(6)} ${t.token}`).join(', ')
setSuccessMessage(`Successfully redeemed ${amount} ICPI for ${tokenList}`)
```

Example: "Successfully redeemed 10 ICPI for 2.450000 ALEX, 0.123456 ZERO, 0.000123 KONG, 0.000001 BOB"

Observation: This works fine for 4 tokens. If the token count increases in the future, consider truncating or using a different format (e.g., "Successfully redeemed X ICPI for Y tokens").

Priority: Very Low (current implementation is fine for 4 tokens)


🔒 Security Review

No concerns. The changes are purely UI/UX:

  • No new backend calls introduced
  • No sensitive data exposed
  • State management is local to the component
  • Query invalidation follows existing patterns

🧪 Test Coverage

As noted in the PR description, testing was done on mainnet:

  • ✅ Mint operation shows success message
  • ✅ Redeem operation shows success message
  • ✅ Header displays user balance correctly
  • ✅ Balance updates immediately after transactions
  • ✅ Success messages auto-clear after 5 seconds

Recommendation: Consider adding a brief manual test of the edge case where a user initiates a transaction, then tries to change the input immediately (to verify the disabled state works as expected).


📊 Performance Impact

Negligible. The added query invalidation for TOTAL_SUPPLY might cause one additional query, but:

  • It's a fast query call (icrc1_total_supply on the token canister)
  • It's already configured with 30s refetch interval (line 529 in useICPI.ts)
  • The benefit (immediate balance update) far outweighs the cost

🎯 Recommendations

  1. Merge as-is - The code quality is high and addresses a real UX pain point
  2. Consider implementing the race condition fix (suggestion Implement Zone 3: Kong Liquidity Integration #1) in a follow-up PR if desired
  3. Monitor the success message display in production to ensure 5 seconds is the right duration

Final Verdict: ✅ APPROVED

This PR significantly improves the user experience with minimal code changes. The implementation is clean, follows best practices, and integrates well with existing patterns. The identified concerns are minor edge cases that don't warrant blocking the PR.

Impact:

  • 🎨 UX: Major improvement
  • 🐛 Bugs: None introduced
  • 🔒 Security: No concerns
  • ⚡ Performance: Negligible impact
  • 📝 Code Quality: Excellent

Great work! 🚀


Review generated with assistance from Claude Code

@evanmcfarland
evanmcfarland merged commit 49a3adc into main Oct 10, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant