feat: Add Overdue, Rejected and Canceled statuses #205
Conversation
WalkthroughThe changes introduce a new function, 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 (
|
![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 (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (1)
102-102: Consider making status reactive.The status should be updated whenever
requestDataorrequestchanges.Apply this diff to make the status reactive:
- let status = checkStatus(requestData || request); + $: status = checkStatus(requestData || request);packages/invoice-dashboard/src/lib/view-requests.svelte (1)
386-408: LGTM! Consider these optimizations for better performance and type safety.The implementation correctly handles all required statuses (Paid, Partially Paid, Rejected, Overdue, Canceled) as per PR objectives. However, here are some suggestions for optimization:
- Move the event status mapping outside the function to avoid recreation on each call:
+const EVENT_STATUS = { + reject: "Rejected", + overdue: "Overdue", + cancel: "Canceled", +} as const; const checkStatus = (request: any) => { if (request?.balance?.balance > 0) { return request.balance.balance >= request.expectedAmount ? "Paid" : "Partially Paid"; } - const eventStatus = { - reject: "Rejected", - overdue: "Overdue", - cancel: "Canceled", - }; - for (const [event, status] of Object.entries(eventStatus)) { + for (const [event, status] of Object.entries(EVENT_STATUS)) { if ( request?.events?.some( (e: { name?: string }) => e?.name?.toLowerCase() === event ) ) { return status; } } return capitalize(request?.state); };
- Add TypeScript types for better type safety:
type RequestStatus = "Paid" | "Partially Paid" | "Rejected" | "Overdue" | "Canceled" | string; type RequestEvent = { name?: string }; type Request = { balance?: { balance: number }; expectedAmount: number; events?: RequestEvent[]; state?: string; }; const checkStatus = (request: Request): RequestStatus => { // ... rest of the implementation };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(3 hunks)packages/invoice-dashboard/src/lib/view-requests.svelte(1 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 (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (1)
400-400: 🛠️ Refactor suggestion
Improve status display styling.
The status styling currently only differentiates between paid and unpaid states using the isPaid boolean. With the new status system, the styling should reflect all possible states.
Apply this diff to improve the status display:
- <p class={`invoice-status ${isPaid ? "bg-green" : "bg-zinc"}`}>
+ <p class={`invoice-status ${
+ status === "Paid" ? "bg-green" :
+ status === "Partially Paid" ? "bg-yellow" :
+ status === "Rejected" ? "bg-red" :
+ status === "Overdue" ? "bg-orange" :
+ status === "Canceled" ? "bg-purple" :
+ "bg-zinc"
+ }`}>Also add the new status colors to the style section:
.bg-green {
background-color: #0bb489;
}
+ .bg-yellow {
+ background-color: #f59e0b;
+ }
+ .bg-red {
+ background-color: #ef4444;
+ }
+ .bg-orange {
+ background-color: #f97316;
+ }
+ .bg-purple {
+ background-color: #8b5cf6;
+ }
.bg-zinc {
background-color: #a1a1aa;
}Likely invalid or redundant comment.
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Outdated
Show resolved
Hide resolved
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 (1)
76-101: Consider improving documentation and maintainability.The
checkStatusfunction implementation looks good, but could benefit from these improvements:
- Add JSDoc documentation to describe the function's purpose and return values
- Move the event status mapping to a constant outside the function
- Add explicit null check for the events array
+const EVENT_STATUS_MAP = { + create: "Created", + reject: "Rejected", + overdue: "Overdue", + cancel: "Canceled", +} as const; + +/** + * Determines the status of a request based on its balance and events. + * @param request - The request data with events + * @returns The current status of the request + */ const checkStatus = (request: Types.IRequestDataWithEvents | null) => { if (Number(request?.balance?.balance)! > 0) { return request?.balance?.balance! >= request?.expectedAmount! ? "Paid" : "Partially Paid"; } - const eventStatus = { - create: "Created", - reject: "Rejected", - overdue: "Overdue", - cancel: "Canceled", - }; - for (const [event, reqStatus] of Object.entries(eventStatus)) { + if (!request?.events) return "Created"; + + for (const [event, reqStatus] of Object.entries(EVENT_STATUS_MAP)) { if ( - request?.events?.some( + request.events.some( (e: { name?: string }) => e?.name?.toLowerCase() === event ) ) { return reqStatus; } } return "Created"; };packages/invoice-dashboard/src/lib/view-requests.svelte (2)
Line range hint
679-679: Add visual distinction for different status typesAccording to the PR objectives, different statuses should have distinct visual representations (e.g., red for Canceled, orange for Rejected). Consider adding status-specific styling to improve clarity.
Add a status indicator component or apply status-specific classes:
- <td> {checkStatus(request)}</td> + <td> + <span class="status-indicator {checkStatus(request).toLowerCase()}"> + {checkStatus(request)} + </span> + </td>Add these styles to the
<style>section:.status-indicator { padding: 4px 8px; border-radius: 4px; font-weight: 500; } .status-indicator.canceled { background-color: #fee2e2; color: #dc2626; } .status-indicator.rejected { background-color: #ffedd5; color: #ea580c; } .status-indicator.overdue { background-color: #fef3c7; color: #d97706; } .status-indicator.paid { background-color: #dcfce7; color: #16a34a; } .status-indicator.partially.paid { background-color: #e0f2fe; color: #0284c7; }
385-408: Optimize status checking performanceThe
checkStatusfunction is called for each row on every render. Consider these optimizations:
- Memoize the status calculations
- Pre-compute statuses when requests are loaded
- Use a derived store for filtered and processed requests
Here's how to implement these optimizations:
+ const requestStatusCache = new Map<string, string>(); + const getRequestStatus = (request: Types.IRequestDataWithEvents | null) => { + if (!request) return "Unknown"; + + const cacheKey = `${request.requestId}-${request.state}-${request.balance?.balance}`; + if (requestStatusCache.has(cacheKey)) { + return requestStatusCache.get(cacheKey)!; + } + + const status = checkStatus(request); + requestStatusCache.set(cacheKey, status); + return status; + }; const processedRequests = (filteredRequests ?? []).map( (request): Types.IRequestDataWithEvents & { formattedAmount: string; currencySymbol: string; paymentCurrencies: (CurrencyTypes.ERC20Currency | CurrencyTypes.NativeCurrency | undefined)[]; + status: string; } => { // ... existing processing ... + const status = getRequestStatus(request); return { ...request, formattedAmount, currencySymbol, paymentCurrencies, + status, }; } );Then update the table to use the pre-computed status:
- <td> {checkStatus(request)}</td> + <td> + <span class="status-indicator {request.status.toLowerCase()}"> + {request.status} + </span> + </td>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(3 hunks)packages/invoice-dashboard/src/lib/view-requests.svelte(1 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 (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (1)
103-103: LGTM: Status initialization is correct.
The status initialization properly uses the new checkStatus function with appropriate fallback handling.
MantisClone
left a comment
MantisClone
left a comment
There was a problem hiding this comment.
Thanks @sstefdev It's not quite ready - please see comments 👇
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Outdated
Show resolved
Hide resolved
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Outdated
Show resolved
Hide resolved
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Outdated
Show resolved
Hide resolved
MantisClone
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
shared/utils/checkStatus.ts (3)
5-10: Consider handling additional edge cases in the balance check.While the current implementation handles the basic cases well, consider adding validation for:
- Negative balances
- Invalid number conversions
- Zero expected amount
export const checkStatus = (request: Types.IRequestDataWithEvents | null) => { const balance = Number(request?.balance?.balance ?? 0); const expectedAmount = Number(request?.expectedAmount ?? 0); + if (expectedAmount <= 0) { + return "Invalid Amount"; + } + + if (balance < 0) { + return "Error"; + } + if (balance > 0) { return balance >= expectedAmount ? "Paid" : "Partially Paid"; }
12-15: Consider moving event status mapping to a constant.The event status mapping could be moved to a constant or enum to improve maintainability and reusability across the codebase.
+export const EVENT_STATUS = { + reject: "Rejected", + cancel: "Canceled", +} as const; + export const checkStatus = (request: Types.IRequestDataWithEvents | null) => { // ... - const eventStatus = { - reject: "Rejected", - cancel: "Canceled", - }; + const eventStatus = EVENT_STATUS;
4-33: Add unit tests for comprehensive status checking.The implementation successfully adds all required statuses, but would benefit from comprehensive unit tests to verify:
- All status transitions
- Edge cases
- Date handling
- Event processing
Would you like me to help generate a test suite for this utility function?
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
77-77: Consider simplifying the initial status assignment.The fallback to
requestmight be unnecessary sincerequestDatais initially null. Consider using justrequestfor the initial status.-let status = checkStatus(requestData || request); +let status = checkStatus(request);
192-193: Add error handling for status updates.The status update in
checkInvoiceshould handle errors explicitly to prevent undefined states.- status = checkStatus(requestData || request); + status = checkStatus(requestData || request); + } catch (err: any) { + console.error("Error while checking status: ", err); + status = "Error"; + throw err;packages/invoice-dashboard/src/lib/view-requests.svelte (3)
Line range hint
42-141: Consider breaking down the component for better maintainability.The component handles multiple responsibilities including wallet connection, request fetching, pagination, and sorting. Consider extracting these into separate components or composables:
- RequestManager: Handle request fetching and processing
- PaginationManager: Handle pagination logic
- SortingManager: Handle sorting logic
This would improve:
- Code organization
- Testing
- Reusability
- Maintenance
Line range hint
516-524: Optimize table rendering performance.The table implementation processes and sorts data on every change. Consider implementing:
- Virtual scrolling for large datasets
- Memoization of sorted and filtered results
- Debounced sorting and filtering operations
This would improve performance especially with large datasets by:
- Reducing unnecessary re-renders
- Minimizing DOM updates
- Improving scroll performance
Line range hint
642-658: Enhance error handling and user feedback.The PDF export error handling could be more informative and provide better recovery options:
- Add specific error messages for common failure scenarios
- Provide retry functionality
- Log errors for debugging
Consider this enhancement:
onClick={async () => { try { await exportToPDF( request, getCurrencyFromManager( request.currencyInfo, currencyManager ), request.paymentCurrencies, config.logo ); } catch (error) { + const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + const actionType = errorMessage.includes('network') ? 'Retry' : 'Close'; toast.error(`Failed to export PDF`, { - description: `${error}`, + description: `Error: ${errorMessage}. Please try again later.`, action: { - label: "X", - onClick: () => console.info("Close"), + label: actionType, + onClick: () => actionType === 'Retry' ? + exportToPDF(request, getCurrencyFromManager(request.currencyInfo, currencyManager), request.paymentCurrencies, config.logo) : + console.info("Close"), }, }); - console.error("Failed to export PDF:", error); + console.error("Failed to export PDF:", { error, request: request.requestId }); } }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(4 hunks)packages/invoice-dashboard/src/lib/view-requests.svelte(1 hunks)shared/utils/capitalize.ts(1 hunks)shared/utils/checkStatus.ts(1 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)
shared/utils/capitalize.ts (1)
1-2: Verify usage in status text handling
Since this utility is used for status text capitalization, let's verify that all status values are properly handled.
✅ Verification successful
The capitalize utility is safely handling all status values
Based on the analysis:
- The
capitalizefunction is only used incheckStatus.tsfor handling request states - The function has proper null checks with the
&&operator and returns an empty string as fallback - The state is only capitalized as a last resort after checking for:
- Payment status (Paid/Partially Paid)
- Special event statuses (Rejected/Canceled)
- Due date status (Overdue)
The implementation safely handles all edge cases:
- Null/undefined inputs return empty string
- Non-empty strings are properly capitalized
- The function is used in a controlled context where state values come from the Request Network SDK
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all status text usage to ensure proper string inputs
# Search for status-related code that might use this function
echo "Searching for status-related code..."
rg -l 'status|state' --type ts --type svelte
# Search for direct usage of capitalize
echo -e "\nSearching for capitalize usage..."
rg -A 2 'capitalize\(' --type ts --type svelte
Length of output: 323
Script:
#!/bin/bash
# Let's try a different approach without specifying file types
# First, find all files that might contain status-related code
echo "Searching for status-related code..."
rg -l 'status|state'
# Then search for direct usage of capitalize function
echo -e "\nSearching for capitalize usage..."
rg -A 2 'capitalize\('
# Also search for imports of capitalize to track its usage
echo -e "\nSearching for capitalize imports..."
rg "import.*capitalize.*from"
Length of output: 926
Script:
#!/bin/bash
# Let's examine the checkStatus.ts file to understand how state values are handled
echo "Content of checkStatus.ts:"
cat shared/utils/checkStatus.ts
# Let's also check what possible state values might be by looking at related files
echo -e "\nChecking state values in components:"
rg -A 3 'state=' shared/components/status.svelte
rg -A 3 'state:' packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Length of output: 1211
Script:
#!/bin/bash
# Let's check the Request Network types to understand possible state values
echo "Searching for Request Network type definitions:"
rg -A 5 "enum.*State"
# Also check for any state-related constants or type definitions in our codebase
echo -e "\nSearching for state-related type definitions:"
rg -A 5 "type.*State|interface.*State|enum.*State" --type ts
# Look for actual state values being used
echo -e "\nSearching for state value assignments:"
rg "state.*=.*['\"](\w+)['\"]" --type ts
Length of output: 1424
shared/utils/checkStatus.ts (1)
1-4: LGTM! Well-structured function signature with proper typing.
The function signature and imports are well-defined, with appropriate handling of the nullable parameter.
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
26-26: LGTM! Good refactoring of status checking logic.
Moving the status checking logic to a shared utility improves code reuse and maintainability.
375-375:
Update status styling to support new status types.
The current styling only differentiates between paid and unpaid states. With the new status types (Overdue, Rejected, Canceled), the styling should be updated to reflect these states appropriately.
-<p class={`invoice-status ${isPaid ? "bg-green" : "bg-zinc"}`}>
+<p class={`invoice-status ${getStatusStyle(status)}`}>
{status}
</p>
+<script>
+function getStatusStyle(status: string): string {
+ switch (status) {
+ case "Paid":
+ return "bg-green";
+ case "Rejected":
+ case "Canceled":
+ return "bg-red";
+ case "Overdue":
+ return "bg-orange";
+ default:
+ return "bg-zinc";
+ }
+}
+</script>
+<style>
+.bg-red {
+ background-color: #ef4444;
+}
+.bg-orange {
+ background-color: #f97316;
+}
+</style>Note: This was previously suggested in a past review comment. Please coordinate with the team regarding the styling implementation timeline.
packages/invoice-dashboard/src/lib/view-requests.svelte (1)
34-34: LGTM! Good refactoring of status check logic.
Moving the checkStatus function to a shared utility follows the DRY principle and promotes code reuse across components.
…162-add-the-rejected-status-to-the-invoice-dashboard
…f github.com:RequestNetwork/web-components into 162-add-the-rejected-status-to-the-invoice-dashboard
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
shared/utils/checkStatus.ts(1 hunks)
🔇 Additional comments (1)
shared/utils/checkStatus.ts (1)
1-2: LGTM!
The imports are appropriate and necessary for the functionality.
|
@sstefdev It needs to fix merging conflicts. |
…162-add-the-rejected-status-to-the-invoice-dashboard
…f github.com:RequestNetwork/web-components into 162-add-the-rejected-status-to-the-invoice-dashboard
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
shared/utils/checkStatus.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
shared/utils/checkStatus.ts (1)
Learnt from: MantisClone
PR: RequestNetwork/web-components#205
File: shared/utils/checkStatus.ts:11-24
Timestamp: 2024-11-25T22:26:01.581Z
Learning: When determining the status of a request, use `some()` to check for "cancel" or "reject" events, as they may not be the last events in the sequence.
🪛 Biome (1.9.4)
shared/utils/checkStatus.ts
[error] 15-15: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (2)
shared/utils/checkStatus.ts (2)
1-4: LGTM!
The imports and function signature are well-defined and appropriate for the task.
9-9: 🛠️ Refactor suggestion
Move isPaid declaration after date validation.
The isPaid status is currently determined before date validation, which could lead to incorrect status precedence. Consider moving this after the date validation to ensure proper status handling.
- const isPaid = balance >= expectedAmount ? "Paid" : "Partially Paid";
+ const getPaymentStatus = () => balance >= expectedAmount ? "Paid" : "Partially Paid";Likely invalid or redundant comment.
sstefdev
deleted the
162-add-the-rejected-status-to-the-invoice-dashboard
branch
Fixes #177
Fixes #162
Problem
The Invoice Dashboard lacks clarity in distinguishing canceled invoices based on whether the Payee or the Payer initiated the cancellation. Additionally, the previous approach to loading statuses was slower and less efficient.
Changes
These updates improve clarity in status representation while enhancing performance, providing a more intuitive and responsive user experience.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes