feat: Improve Payment Status Indicators on Invoice Dashboard #221
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…174-improve-the-payment-status-indicators-on-the-invoice-dashboard
sstefdev
requested review from
MantisClone,
aimensahnoun and
rodrigopavezi
and removed request for
MantisClone,
aimensahnoun and
rodrigopavezi
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (1)
76-92: LGTM! Consider adding TypeScript interfaces for better type safety.The structured approach to status tracking is clear and maintainable. Each status object has well-defined properties that make the payment flow easy to understand.
Consider adding a TypeScript interface to enforce the status object structure:
interface PaymentStatus { name: 'SIGN_TRANSACTION' | 'PAYMENT_DETECTED' | 'CORRECT_NETWORK'; message: string; done: boolean; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(10 hunks)packages/invoice-dashboard/src/lib/view-requests.svelte(5 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (1)
Learnt from: MantisClone
PR: RequestNetwork/web-components#141
File: packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte:0-0
Timestamp: 2024-11-12T14:52:33.204Z
Learning: In `packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte`, mapping payment network IDs to their respective approval functions in the `checkApproval` function is acceptable and can improve readability and maintainability.
🔇 Additional comments (5)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
606-659: Well-implemented status indicator UI that meets the PR objectives.
The implementation successfully addresses the key requirements:
- Non-button-like appearance using proper list elements
- Clear progress visualization with icons and progress lines
- Controlled visibility of status indicators
Line range hint 664-683: Well-structured payment flow with proper status checks.
The implementation ensures a clear progression through the payment steps while preventing invalid states.
packages/invoice-dashboard/src/lib/view-requests.svelte (3)
332-332: Good use of optional chaining for safer currency symbol access.
The change prevents potential runtime errors when currency information is undefined.
715-716: Clean implementation of loading state with skeleton UI.
Good use of skeleton loading pattern for better user experience during data fetching.
443-448: Well-implemented encryption toggle with clear labeling.
The Switch component provides a good user interface for controlling encryption visibility.
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Outdated
Show resolved
Hide resolved
…174-improve-the-payment-status-indicators-on-the-invoice-dashboard
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (4)
76-92: Consider adding type safety to the status tracking.The status array structure is well-organized, but using
any[]type could lead to maintenance issues. Consider defining an interface for the status object.+interface PaymentStatus { + name: 'SIGN_TRANSACTION' | 'PAYMENT_DETECTED' | 'CORRECT_NETWORK'; + message: string; + done: boolean; +} -let statuses: any[] = [ +let statuses: PaymentStatus[] = [
600-653: Enhance accessibility and code organization of status indicators.The status indicator implementation could benefit from:
- ARIA attributes for better screen reader support
- Moving SVG components to separate files
Add ARIA attributes and consider extracting SVGs:
-<ul class="status-list"> +<ul class="status-list" role="list" aria-label="Payment status progress"> {#each statuses as status, index} - <li class="status-item"> + <li class="status-item" role="listitem" aria-label={`${status.message}: ${status.done ? 'completed' : 'in progress'}`}>
Line range hint
658-677: Improve maintainability of button visibility logic.Consider extracting the button visibility conditions into helper functions for better maintainability.
+const shouldShowSwitchNetwork = () => !correctChain && !isPayee; +const shouldShowApprove = () => !approved && !isPaid && !isPayee && !unsupportedNetwork; +const shouldShowPay = () => approved && !isPaid && !isPayee && !unsupportedNetwork && !statuses[0].done; <div class="invoice-view-actions"> - {#if !correctChain && !isPayee} + {#if shouldShowSwitchNetwork()} <Button type="button" text="Switch Network" padding="px-[12px] py-[6px]" onClick={() => switchNetworkIfNeeded(network || "mainnet")} /> - {:else if !approved && !isPaid && !isPayee && !unsupportedNetwork} + {:else if shouldShowApprove()}
Line range hint
877-1021: Consider improving style maintainability.The styling implementation could benefit from:
- Converting magic numbers to CSS variables
- Moving color values to CSS custom properties
+:root { + --status-item-width: 45%; + --progress-line-width: 300px; + --status-icon-size: 40px; + --color-blue: #759aff; + --color-success: #cdf6e4; + --color-waiting: #c7e7ff; +} .status-item { - width: 45%; + width: var(--status-item-width); } .progress-line { - width: 300px; + width: var(--progress-line-width); } .bg-blue { - background-color: #759aff; + background-color: var(--color-blue); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(10 hunks)packages/invoice-dashboard/src/lib/view-requests.svelte(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/invoice-dashboard/src/lib/view-requests.svelte
🧰 Additional context used
📓 Learnings (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (1)
Learnt from: MantisClone
PR: RequestNetwork/web-components#141
File: packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte:0-0
Timestamp: 2024-11-12T14:52:33.204Z
Learning: In `packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte`, mapping payment network IDs to their respective approval functions in the `checkApproval` function is acceptable and can improve readability and maintainability.
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Outdated
Show resolved
Hide resolved
|
The statuses are in the wrong order. It should check for the correct network first, then get the transaction signature, and finish with the payment detected. |
|
Previously the first button we showed was switch chain. I think we should change this to Pay Now. Then, we show the payment status stepper and prompt to switch chains if necessary. We also need an intermediate step to check for approval in the case of ERC-20s |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (3)
392-414: Enhance error handling and status updates in payment flowThe payment handling logic is well-structured but could benefit from more specific error handling and consistent status updates.
async function handlePayment() { try { if (!correctChain) { + const networkStatus = statuses.find((s) => s.name === "CORRECT_NETWORK"); + if (networkStatus) networkStatus.done = false; await switchNetworkIfNeeded(network || "mainnet"); return; } if ( !approved && paymentCurrencies[0]?.type === Types.RequestLogic.CURRENCY.ERC20 ) { + const approveStatus = statuses.find((s) => s.name === "APPROVE_ERC20"); + if (approveStatus) approveStatus.done = false; await approve(); return; } await payTheRequest(); } catch (err) { console.error("Error during payment process:", err); + // Reset status on error + statuses.forEach(status => status.done = false); toast.error("Payment process failed", { - description: String(err), + description: err instanceof Error ? err.message : String(err), + duration: 5000, }); } }
926-985: Improve style maintainabilityThe status styles use several magic numbers that could be converted to CSS variables for better maintainability. Additionally, the progress line positioning could be more flexible.
+ :root { + --status-item-width: 45%; + --status-item-padding: 50px; + --status-text-top-offset: -30px; + --status-text-left-offset: -30px; + --progress-line-width: 300px; + } .status-item { display: flex; align-items: center; position: relative; text-align: center; - width: 45%; + width: var(--status-item-width); } .status-item:first-child { - padding-left: 50px; + padding-left: var(--status-item-padding); } .status-text { font-size: 14px; color: #272d41; position: absolute; - top: -30px; - left: -30px; + top: var(--status-text-top-offset); + left: var(--status-text-left-offset); }
633-686: Add ARIA attributes for accessibilityTo improve accessibility of the status indicators, consider adding appropriate ARIA attributes.
- <ul class="status-list"> + <ul class="status-list" role="progressbar" aria-label="Payment progress"> {#each statuses as status, index} - <li class="status-item"> + <li + class="status-item" + role="status" + aria-label={`${status.message}: ${status.done ? 'completed' : 'pending'}`} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(12 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (1)
Learnt from: MantisClone
PR: RequestNetwork/web-components#141
File: packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte:0-0
Timestamp: 2024-11-12T14:52:33.204Z
Learning: In `packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte`, mapping payment network IDs to their respective approval functions in the `checkApproval` function is acceptable and can improve readability and maintainability.
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The state of this component is getting too complex and I think we must refactor it to a state machine that is easier to reason about and maintain. I suspect that error states are not adequately expressed.
That said, I don't want to delay the delivery of this feature so if you can update the video clip on the PR description showing that it works, I'll approve it.
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Outdated
Show resolved
Hide resolved
…174-improve-the-payment-status-indicators-on-the-invoice-dashboard
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
783-796: Simplify button visibility conditions.The multiple conditions for button visibility make the logic hard to follow and maintain.
Consider extracting the visibility logic:
+const shouldShowPayButton = !isPayee && !unsupportedNetwork && !isPaid && !isSigningTransaction; + +const getButtonText = () => { + if (!correctChain) return "Switch Network"; + if (!approved && paymentCurrencies[0]?.type === Types.RequestLogic.CURRENCY.ERC20) { + return "Approve"; + } + return "Pay Now"; +}; + -{#if !isPayee && !unsupportedNetwork && !isPaid && !isSigningTransaction} +{#if shouldShowPayButton} <Button className="pay-button" type="button" - text={!correctChain - ? "Switch Network" - : !approved && - paymentCurrencies[0]?.type === Types.RequestLogic.CURRENCY.ERC20 - ? "Approve" - : "Pay Now"} + text={getButtonText()} padding="px-[12px] py-[6px]" onClick={handlePayment} /> {/if}
Line range hint
993-1169: Convert magic numbers to CSS variables.The CSS contains several magic numbers that should be converted to variables for better maintainability.
Consider this improvement:
<style> + :root { + --status-item-width: 45%; + --status-item-width-three: 35%; + --status-last-width: 20%; + --progress-line-width: 300px; + --progress-line-last-width: 170px; + --status-icon-size: 40px; + --status-text-offset: -30px; + } + .status-item { display: flex; align-items: center; position: relative; text-align: center; - width: 45%; + width: var(--status-item-width); } .status-list:has(:nth-child(3):last-child) .status-item { - width: 35%; + width: var(--status-item-width-three); } .status-item:last-child { - width: 20%; + width: var(--status-last-width); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(16 hunks)packages/invoice-dashboard/src/lib/view-requests.svelte(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/invoice-dashboard/src/lib/view-requests.svelte
🔇 Additional comments (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
721-782: Extract SVG icons into separate components.
The SVG icons should be extracted into separate components for better maintainability and reusability, as suggested in previous reviews.
74-90:
Revise status order to match the logical payment flow.
The current status order doesn't match the suggested flow from PR comments. Additionally, the ERC20 approval status should be defined statically.
Apply this diff to fix the status order and include ERC20 approval:
let statuses: any[] = [
{
name: "CORRECT_NETWORK",
message: "Correct Network",
done: correctChain,
},
+ {
+ name: "APPROVE_ERC20",
+ message: "Approve ERC20",
+ done: false,
+ showIf: () => paymentCurrencies[0]?.type === Types.RequestLogic.CURRENCY.ERC20
+ },
{
- name: "SIGN_TRANSACTION",
- message: "Sign Transaction",
- done: false,
+ name: "PAYMENT_DETECTED",
+ message: "Payment Detected",
+ done: false,
},
{
- name: "PAYMENT_DETECTED",
- message: "Payment Detected",
+ name: "SIGN_TRANSACTION",
+ message: "Sign Transaction",
done: false,
},
];Likely invalid or redundant comment.
|
I'm approving to unblock your other work, but I strongly think we must refactor this logic at a later time. |
…-the-invoice-dashboard
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
472-500: Enhance error handling in the payment flow.While the basic error handling is in place, consider these improvements:
- Add specific error messages for different failure scenarios (network switch, approval, payment)
- Handle network switching errors more gracefully
async function handlePayment() { try { await switchNetworkIfNeeded(network || "mainnet"); if ( !approved && paymentCurrencies[0]?.type === Types.RequestLogic.CURRENCY.ERC20 ) { await approve(); const approveStatus = statuses.find((s) => s.name === "APPROVE_ERC20"); if (approveStatus) approveStatus.done = true; return; } await payTheRequest(); } catch (err) { console.error("Error during payment process:", err); + let errorMessage = "Payment process failed"; + let description = String(err); + + if (String(err).includes("network")) { + errorMessage = "Network switch failed"; + description = "Please ensure you're connected to the correct network"; + } else if (String(err).includes("approve")) { + errorMessage = "Approval failed"; + description = "Failed to approve token transfer"; + } + toast.error("Payment process failed", { - description: String(err), + description, }); statuses = statuses.map((status) => ({ ...status, done: false, })); } }
Line range hint
989-1165: Improve responsive design for status indicators.The current implementation uses fixed widths which might cause issues on smaller screens:
.status-list { display: flex; align-items: center; list-style: none; padding: 0; - margin-left: 85px; + margin-left: clamp(20px, 5vw, 85px); } .status-item { display: flex; align-items: center; position: relative; text-align: center; - width: 45%; + width: clamp(30%, 45%, 50%); } .status-item:last-child .progress-line { - width: 170px; + width: clamp(100px, 15vw, 170px); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(16 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
Learnt from: sstefdev
PR: RequestNetwork/web-components#221
File: packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte:472-500
Timestamp: 2024-12-16T19:38:09.647Z
Learning: Timeouts for testing have been removed from the payment flow in `invoice-view.svelte`, so adding timeout handling in the `handlePayment` function is unnecessary.
Learnt from: MantisClone
PR: RequestNetwork/web-components#141
File: packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte:0-0
Timestamp: 2024-11-12T14:52:33.204Z
Learning: In `packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte`, mapping payment network IDs to their respective approval functions in the `checkApproval` function is acceptable and can improve readability and maintainability.
🔇 Additional comments (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (1)
74-90: LGTM! Status management implementation aligns with requirements.
The status array implementation follows the correct order as suggested in the PR comments: network check → transaction signature → payment detection.
sstefdev
deleted the
174-improve-the-payment-status-indicators-on-the-invoice-dashboard
branch
Fixes #174
Problem
The current payment progress indicators in the Invoice Dashboard have the following issues:
Changes
Indicator Redesign:
Behavioral Adjustments:
Placement Optimization:
Moved the indicators to a more intuitive location in the invoice details modal for better visibility.
Added fix for multiple requests error
Added ERC-20 approval flow
Removing Pay Now button when a user signs the transactions
How it looks
Screen.Recording.2024-12-10.at.11.42.59.mov
Updated (With ERC-20 Approval)
Screen.Recording.2024-12-16.at.20.17.55.mov
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores