chore: Add additional data to already claimed links and already paid requests#441
chore: Add additional data to already claimed links and already paid requests#441jjramirezn merged 11 commits intodevelopfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces several changes across multiple files. A new asynchronous Changes
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: 10
🧹 Outside diff range and nitpick comments (3)
src/utils/link.utils.ts (1)
1-3: Address the TODO comment for code organizationThe TODO comment indicates that there are link-related utility functions in
general.utils.tsthat should be moved to this file. It's a good practice to keep related utilities together for better code organization and maintainability.Consider creating a separate task to move the link-related utilities from
general.utils.tsto this file. This will help in better organizing the codebase and make it easier to maintain in the future. Would you like me to create a GitHub issue for this task?src/app/request/pay/page.tsx (2)
6-7: Good catch on dynamic metadata. Consider creating a task.The TODO comment about making the metadata dynamic based on payment status is a valuable suggestion. It would indeed improve the user experience by providing more accurate information.
Would you like me to create a GitHub issue to track this enhancement? This would ensure it's not forgotten and can be prioritized appropriately.
8-8: Approved: More descriptive title. Consider further specificity.The updated title "Peanut Protocol - Payment Request" is more informative and aligns well with the page's purpose. Good job on improving clarity for users.
For even more clarity, consider making it action-oriented, e.g., "Peanut Protocol - Submit Payment Request". This could further help users understand the page's function at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- src/app/api/peanut/get-link-info/route.ts (1 hunks)
- src/app/claim/page.tsx (1 hunks)
- src/app/request/pay/page.tsx (1 hunks)
- src/components/Claim/Generic/AlreadyClaimed.view.tsx (1 hunks)
- src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (1 hunks)
- src/utils/link.utils.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (3)
Learnt from: jjramirezn PR: peanutprotocol/peanut-ui#412 File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37 Timestamp: 2024-10-08T20:13:42.967Z Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.Learnt from: jjramirezn PR: peanutprotocol/peanut-ui#412 File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37 Timestamp: 2024-10-03T17:43:19.914Z Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.Learnt from: jjramirezn PR: peanutprotocol/peanut-ui#412 File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37 Timestamp: 2024-10-08T20:13:44.480Z Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.
🪛 Biome
src/components/Claim/Generic/AlreadyClaimed.view.tsx
[error] 15-15: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
src/app/api/peanut/get-link-info/route.ts (1)
1-4: LGTM: Imports and setup are appropriate.The imports are correctly set up for a Next.js API route, and the use of a constants file is a good practice for maintainability.
src/app/claim/page.tsx (2)
40-50: 🛠️ Refactor suggestionImprove readability and handling of token amounts in title construction
The new logic for constructing the title provides more informative feedback to users, which is great. However, consider the following improvements:
- Use template literals instead of string concatenation for better readability.
- Consider using a more precise method to handle small token amounts, as the current approach might not be suitable for all token types.
- Improve the indentation of the multi-line string construction.
Here's a suggested refactor:
if (!linkDetails.claimed) { const formattedAmount = Number(linkDetails.tokenAmount) < 0.01 ? 'a small amount of' : utils.formatAmount(Number(linkDetails.tokenAmount)); title = `You received ${formattedAmount} ${linkDetails.tokenSymbol}!`; } else { title = 'This link has been claimed'; }This refactored version:
- Uses template literals for better readability.
- Separates the amount formatting logic for clarity.
- Uses a more descriptive phrase for small amounts.
- Improves indentation and overall code structure.
To ensure this change doesn't affect other parts of the codebase, let's check for other usages of
utils.formatAmount:#!/bin/bash # Search for other usages of utils.formatAmount rg "utils\.formatAmount" --type typescript
Line range hint
1-93: General improvements for code cleanliness and error handlingWhile the main changes look good, here are some suggestions for overall improvement:
Remove commented-out code: The code for fetching token price (around line 60) is commented out. If it's no longer needed, consider removing it entirely to keep the code clean.
Improve error handling: In the
generateMetadatafunction, consider setting a default title in case of an error. This ensures that a title is always set, even if the link details fetch fails.Consider refactoring
createURL: If this function is used in other parts of the application, it might be beneficial to move it to a utility file for better code organization and reusability.Here's a suggestion for improved error handling in
generateMetadata:try { // ... existing code ... } catch (e) { console.error('Error fetching link details:', e); title = 'Claim your tokens!'; // Set a default title in case of error }To check if
createURLis used elsewhere and might benefit from being moved to a utility file:✅ Verification successful
General improvements for code cleanliness and error handling
While the main changes look good, here are some suggestions for overall improvement:
Remove commented-out code: The code for fetching token price (around line 60) is commented out. If it's no longer needed, consider removing it entirely to keep the code clean.
Improve error handling: In the
generateMetadatafunction, consider setting a default title in case of an error. This ensures that a title is always set, even if the link details fetch fails.Evaluate the necessity of refactoring
createURL: Currently,createURLis only used withinpage.tsx. If you anticipate using it elsewhere in the future, consider moving it to a utility file for better code organization and reusability. Otherwise, keeping it within the current file maintains simplicity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of createURL function rg "createURL\(" --type typescriptLength of output: 79
Script:
#!/bin/bash # Search for other usages of createURL function in TypeScript files rg "createURL\(" -g "*.ts" -g "*.tsx"Length of output: 242
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (1)
Line range hint
86-94: Verify the link text and href for consistency with user flowBased on previous feedback, similar components in the send flow use the link text 'Make a payment yourself!' and href '/send'. Please ensure that 'Request a payment yourself!' and href '/request/create' are appropriate for the request flow in this context.
🧰 Tools
🪛 Biome
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/utils/link.utils.ts
Outdated
| export async function get(bankAccount: string): Promise<boolean> { | ||
| bankAccount = bankAccount.replace(/\s/g, '') | ||
| const response = await fetch(`/api/peanut/iban/validate-bank-account`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| bankAccount, | ||
| }), | ||
| }) | ||
|
|
||
| if (response.status !== 200) { | ||
| return false | ||
| } else { | ||
| return true | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve function naming, error handling, and return value
The current implementation of the get function has several areas for improvement:
-
Function naming:
- The name
getis too generic. Consider renaming it tovalidateBankAccountorisBankAccountValidto better describe its purpose.
- The name
-
Input validation:
- Add a check for empty or null input before processing.
-
Error handling:
- Implement proper error handling for network errors and parsing errors.
- Consider returning more detailed error information.
-
Return value:
- Instead of a boolean, return an object with more details about the validation result.
-
Code style:
- Remove the unnecessary
elseafter thereturnstatement.
- Remove the unnecessary
Here's a suggested refactoring of the function:
export async function validateBankAccount(bankAccount: string): Promise<{ isValid: boolean; error?: string }> {
if (!bankAccount) {
return { isValid: false, error: 'Bank account is required' };
}
const cleanedBankAccount = bankAccount.replace(/\s/g, '');
try {
const response = await fetch(`/api/peanut/iban/validate-bank-account`, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify({ bankAccount: cleanedBankAccount }),
});
if (!response.ok) {
const errorData = await response.json().catch(() => ({}));
return {
isValid: false,
error: errorData.message || `Validation failed with status: ${response.status}`
};
}
const data = await response.json();
return { isValid: true, ...data };
} catch (error) {
return {
isValid: false,
error: error instanceof Error ? error.message : 'An unknown error occurred'
};
}
}This refactored version addresses the issues mentioned above and provides more robust error handling and informative return values.
| export async function POST(request: NextRequest) { | ||
| try { | ||
| const { link } = await request.json() |
There was a problem hiding this comment.
Add input validation for the 'link' parameter.
While the current implementation is concise, it might fail silently if the 'link' is not provided in the request body. Consider adding explicit input validation to handle cases where 'link' is missing or invalid.
Here's a suggested improvement:
export async function POST(request: NextRequest) {
try {
const { link } = await request.json()
+ if (!link || typeof link !== 'string') {
+ return new NextResponse('Invalid or missing link parameter', { status: 400 })
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function POST(request: NextRequest) { | |
| try { | |
| const { link } = await request.json() | |
| export async function POST(request: NextRequest) { | |
| try { | |
| const { link } = await request.json() | |
| if (!link || typeof link !== 'string') { | |
| return new NextResponse('Invalid or missing link parameter', { status: 400 }) | |
| } |
There was a problem hiding this comment.
@jjramirezn, glad to hear that you find the suggestion valid! Let me know if you have any questions or need further assistance.
🐰😊
| const response = await fetch(`${consts.PEANUT_API_URL}/get-link-info`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ | ||
| link, | ||
| apiKey: process.env.PEANUT_API_KEY, | ||
| }), | ||
| }) | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`HTTP error! status: ${response.status}`) | ||
| } | ||
|
|
||
| const data = await response.json() | ||
| return NextResponse.json(data) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security and add documentation for the API call.
The API call is well-structured, but there are two areas for improvement:
- For better security, consider moving the API key to the request headers instead of the body.
- Add a comment explaining the purpose of this API call for better code documentation.
Here's a suggested improvement:
+ // Fetch link information from Peanut API
const response = await fetch(`${consts.PEANUT_API_URL}/get-link-info`, {
method: 'POST',
- headers: { 'Content-Type': 'application/json' },
+ headers: {
+ 'Content-Type': 'application/json',
+ 'X-API-Key': process.env.PEANUT_API_KEY
+ },
body: JSON.stringify({
link,
- apiKey: process.env.PEANUT_API_KEY,
}),
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await fetch(`${consts.PEANUT_API_URL}/get-link-info`, { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ | |
| link, | |
| apiKey: process.env.PEANUT_API_KEY, | |
| }), | |
| }) | |
| if (!response.ok) { | |
| throw new Error(`HTTP error! status: ${response.status}`) | |
| } | |
| const data = await response.json() | |
| return NextResponse.json(data) | |
| // Fetch link information from Peanut API | |
| const response = await fetch(`${consts.PEANUT_API_URL}/get-link-info`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'X-API-Key': process.env.PEANUT_API_KEY | |
| }, | |
| body: JSON.stringify({ | |
| link, | |
| }), | |
| }) | |
| if (!response.ok) { | |
| throw new Error(`HTTP error! status: ${response.status}`) | |
| } | |
| const data = await response.json() | |
| return NextResponse.json(data) |
| } catch (error) { | ||
| console.error('Error fetching user stats:', error) | ||
| return new NextResponse('Internal Server Error', { status: 500 }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling and logging.
The current error handling can be enhanced in the following ways:
- Use a more specific error message in the console.log.
- Consider returning more informative error responses to the client, while being careful not to expose sensitive information.
- Differentiate between different types of errors (e.g., network errors, API errors, validation errors).
Here's a suggested improvement:
} catch (error) {
- console.error('Error fetching user stats:', error)
- return new NextResponse('Internal Server Error', { status: 500 })
+ console.error('Error fetching link info:', error)
+ if (error instanceof Error) {
+ return new NextResponse(error.message, { status: 500 })
+ } else {
+ return new NextResponse('An unexpected error occurred', { status: 500 })
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| console.error('Error fetching user stats:', error) | |
| return new NextResponse('Internal Server Error', { status: 500 }) | |
| } | |
| } | |
| } catch (error) { | |
| console.error('Error fetching link info:', error) | |
| if (error instanceof Error) { | |
| return new NextResponse(error.message, { status: 500 }) | |
| } else { | |
| return new NextResponse('An unexpected error occurred', { status: 500 }) | |
| } | |
| } | |
| } |
| <div className="flex w-full flex-col items-center justify-center gap-2"> | ||
| <div className="flex w-full flex-col items-center justify-center gap-2"> |
There was a problem hiding this comment.
Remove duplicate nested <div> elements
There are two identical <div> elements nested inside each other. This duplication is unnecessary and may lead to unintended styling or layout issues.
Apply this diff to remove the duplicate <div>:
{dataAvailable && (
- <div className="flex w-full flex-col items-center justify-center gap-2">
<div className="flex w-full flex-col items-center justify-center gap-2">Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
@jjramirezn, I have looked into this and confirmed that there are two identical <div> elements nested inside each other in the AlreadyPaidLinkView component when dataAvailable is true. This duplication is unnecessary and may lead to unintended styling or layout issues.
We can simplify the code by removing one of the duplicate <div> elements:
{dataAvailable && (
<div className="flex w-full flex-col items-center justify-center gap-2">
{/* Rest of the content */}
</div>
)}This adjustment will maintain the intended layout without the extra nesting.
🧠 Learnings used
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#412
File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37
Timestamp: 2024-10-03T17:43:19.914Z
Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#412
File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37
Timestamp: 2024-10-08T20:13:42.967Z
Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#412
File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37
Timestamp: 2024-10-08T20:13:44.480Z
Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.
| <div className="flex w-full flex-row items-center justify-between gap-1 px-2 text-h8 text-gray-1"> | ||
| <div className="flex w-max flex-row items-center justify-center gap-1"> | ||
| <label className="font-bold">Token</label> | ||
| </div> | ||
| <span className="flex flex-row items-center justify-center gap-1 text-center text-sm font-normal leading-4"> | ||
| {claimLinkData?.tokenSymbol} | ||
| </span> | ||
| </div> | ||
| )} | ||
| {tokenAmountAvailable && ( | ||
| <div className="flex w-full flex-row items-center justify-between gap-1 px-2 text-h8 text-gray-1"> | ||
| <div className="flex w-max flex-row items-center justify-center gap-1"> | ||
| <label className="font-bold">Amount</label> | ||
| </div> | ||
| <span className="flex flex-row items-center justify-center gap-1 text-center text-sm font-normal leading-4"> | ||
| {claimLinkData?.tokenAmount} | ||
| </span> | ||
| </div> | ||
| )} | ||
| {chainAvailable && ( | ||
| <div className="flex w-full flex-row items-center justify-between gap-1 px-2 text-h8 text-gray-1"> | ||
| <div className="flex w-max flex-row items-center justify-center gap-1"> | ||
| <label className="font-bold">Chain</label> | ||
| </div> | ||
| <span className="flex flex-row items-center justify-center gap-1 text-center text-sm font-normal leading-4"> | ||
| <img | ||
| src={consts.supportedPeanutChains.find((detail) => detail.chainId === claimLinkData?.chainId)?.icon | ||
| .url | ||
| } | ||
| className="h-6 w-6" | ||
| /> | ||
| {chainName} | ||
| </span> | ||
| </div> | ||
| )} | ||
| {senderAddressAvailable && ( | ||
| <div className="flex w-full flex-row items-center justify-between gap-1 px-2 text-h8 text-gray-1"> | ||
| <div className="flex w-max flex-row items-center justify-center gap-1"> | ||
| <label className="font-bold">Sender</label> | ||
| </div> | ||
| <span className="flex flex-row items-center justify-center gap-1 text-center text-sm font-normal leading-4"> | ||
| {utils.shortenAddress(claimLinkData?.senderAddress)} | ||
| </span> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated rendering blocks into a reusable component
The blocks of code from lines 32 to 76 that render token information are repetitive. Consider creating a reusable component or a helper function to handle these sections. This will enhance maintainability and reduce code duplication.
| const tokenSymbolAvailable: boolean = claimLinkData?.tokenSymbol != undefined | ||
| const tokenAmountAvailable: boolean = claimLinkData?.tokenAmount != undefined | ||
| const chainAvailable: boolean = claimLinkData?.chainId != undefined | ||
| const senderAddressAvailable: boolean = claimLinkData?.senderAddress != undefined |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify null checking with a helper function
The checks for tokenSymbolAvailable, tokenAmountAvailable, chainAvailable, and senderAddressAvailable can be simplified by creating a helper function to reduce redundancy.
Consider defining a helper function:
const isAvailable = (value: any): boolean => value != null;And then use it:
-const tokenSymbolAvailable: boolean = claimLinkData?.tokenSymbol != undefined
-const tokenAmountAvailable: boolean = claimLinkData?.tokenAmount != undefined
-const chainAvailable: boolean = claimLinkData?.chainId != undefined
-const senderAddressAvailable: boolean = claimLinkData?.senderAddress != undefined
+const tokenSymbolAvailable = isAvailable(claimLinkData?.tokenSymbol)
+const tokenAmountAvailable = isAvailable(claimLinkData?.tokenAmount)
+const chainAvailable = isAvailable(claimLinkData?.chainId)
+const senderAddressAvailable = isAvailable(claimLinkData?.senderAddress)| src={consts.supportedPeanutChains.find((detail) => detail.chainId === claimLinkData?.chainId)?.icon | ||
| .url | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the image source assignment for clarity
The assignment of the src property for the image can be made more readable by simplifying the expression.
Apply this diff to improve readability:
-src={consts.supportedPeanutChains.find((detail) => detail.chainId === claimLinkData?.chainId)?.icon
- .url
-}
+src={
+ consts.supportedPeanutChains.find(
+ (detail) => detail.chainId === claimLinkData?.chainId
+ )?.icon.url
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| src={consts.supportedPeanutChains.find((detail) => detail.chainId === claimLinkData?.chainId)?.icon | |
| .url | |
| } | |
| src={ | |
| consts.supportedPeanutChains.find( | |
| (detail) => detail.chainId === claimLinkData?.chainId | |
| )?.icon.url | |
| } |
| export const AlreadyClaimedLinkView = ({ claimLinkData }: { claimLinkData: interfaces.ILinkDetails | undefined }) => { | ||
| const router = useRouter() | ||
|
|
||
| const chainName = consts.supportedPeanutChains && consts.supportedPeanutChains.find((chain) => chain.chainId == claimLinkData?.chainId)?.name |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify optional chaining in chainName assignment
You can simplify the assignment of chainName by leveraging optional chaining without the need for the logical AND && operator.
Apply this diff to simplify the code:
-const chainName = consts.supportedPeanutChains && consts.supportedPeanutChains.find((chain) => chain.chainId == claimLinkData?.chainId)?.name
+const chainName = consts.supportedPeanutChains?.find((chain) => chain.chainId === claimLinkData?.chainId)?.name📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const chainName = consts.supportedPeanutChains && consts.supportedPeanutChains.find((chain) => chain.chainId == claimLinkData?.chainId)?.name | |
| const chainName = consts.supportedPeanutChains?.find((chain) => chain.chainId === claimLinkData?.chainId)?.name |
🧰 Tools
🪛 Biome
[error] 15-15: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
2 small nits:
|
panosfilianos
changed the title
| <label className="text-h2">Sorry, this link has already been paid.</label> | ||
| <label className="text-h8 font-bold "> | ||
| This link previously requested{' '} | ||
| {requestLinkData?.tokenSymbol ?? |
There was a problem hiding this comment.
thought: there is loss of functionality here
Before we were trying to get a token symbol if there is none from the data. Now we just trust the data we get. I actually like the new way better but it warrants fixing why sometimes we don't have data from the request link itself. In the short term this change WILL result in some data previously shown to not be shown anymore
There was a problem hiding this comment.
True. I would expect this operation to take place where data is populated (i.e. where requestLinkData is formed, populate as lines 16-19 do). I, actually, do not have a clear idea on why requestLinkData won't have all required fields populated already. Thus, I opted to show the data that is directly available through requestLinkData.
src/utils/link.utils.ts
Outdated
| // This file holds all relevant link utils | ||
| // TODO: bring all link utils from general.utils.ts to here | ||
|
|
||
| export async function get(bankAccount: string): Promise<boolean> { |
There was a problem hiding this comment.
comment(blocking): where is this used?
also, we should probably use better naming
jjramirezn
left a comment
jjramirezn
left a comment
There was a problem hiding this comment.
- The function in link.utils.ts seems to be unused
- Fix deployment (using pnpm build you can check whats failing, but there is a duplicated div tag maybe is that)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (2)
10-16: LGTM! Consider simplifying thechainNameconstant.The introduction of new constants to check the availability of properties from
requestLinkDataimproves code readability and maintainability. ThedataAvailableboolean is a good approach for conditional rendering.Consider simplifying the
chainNameconstant using optional chaining:- const chainName = consts.supportedPeanutChains && consts.supportedPeanutChains.find((chain) => chain.chainId == requestLinkData?.chainId)?.name + const chainName = consts.supportedPeanutChains?.find((chain) => chain.chainId == requestLinkData?.chainId)?.nameThis change makes the code more concise while maintaining the same functionality.
🧰 Tools
🪛 Biome
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
46-61: LGTM! Enhanced chain information display. Consider simplifying icon URL retrieval.The new block for displaying chain information is well-structured and consistent with previous blocks. The addition of the chain icon enhances visual recognition, improving the user experience.
Consider simplifying the chain icon URL retrieval:
- src={consts.supportedPeanutChains.find((detail) => detail.chainId === requestLinkData?.chainId)?.icon - .url - } + src={consts.supportedPeanutChains.find((detail) => detail.chainId === requestLinkData?.chainId)?.icon?.url}This change reduces nesting and improves readability while maintaining the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (3)
Learnt from: jjramirezn PR: peanutprotocol/peanut-ui#412 File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37 Timestamp: 2024-10-08T20:13:42.967Z Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.Learnt from: jjramirezn PR: peanutprotocol/peanut-ui#412 File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37 Timestamp: 2024-10-03T17:43:19.914Z Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.Learnt from: jjramirezn PR: peanutprotocol/peanut-ui#412 File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37 Timestamp: 2024-10-08T20:13:44.480Z Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.
🪛 Biome
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (5)
21-25: LGTM! Improved conditional rendering and user information.The introduction of conditional rendering based on
dataAvailableensures that additional information is only shown when relevant data is present. The label "This link previously contained:" provides clear context for the information that follows, improving user understanding.
26-35: LGTM! Clear presentation of token symbol.The new block for displaying the token symbol is well-structured and consistent with the component's styling. The conditional rendering ensures that this information is only shown when available, preventing potential display issues with incomplete data.
36-45: LGTM! Consistent presentation of token amount.The new block for displaying the token amount maintains consistency with the token symbol block in terms of structure and styling. This addition provides valuable information about the transaction amount, enhancing the user's understanding of the previously paid link.
62-71: LGTM! Well-implemented requester address display.The new block for displaying the requester's address maintains consistency with previous blocks in terms of structure and styling. The use of
utils.shortenAddress()is a good practice, improving readability and maintaining a level of privacy for the requester.
Line range hint
1-73: Great job on enhancing the AlreadyPaidLinkView component!The changes made to this component significantly improve the presentation of information for already paid links. The introduction of conditional rendering for each piece of data (token symbol, amount, chain, and requester address) ensures that users only see relevant information when it's available. This approach enhances the user experience by providing more detailed and structured information about the previously paid link.
The consistent use of styling and layout throughout the component maintains a cohesive look and feel. The addition of the chain icon is a nice touch that improves visual recognition.
Overall, these changes make the component more informative and user-friendly. Great work on improving this part of the UI!
🧰 Tools
🪛 Biome
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
New updates:
|
|
pnpm dev compiles lazily and I don't know if checks types, the error that shows with
Also I think is better not to commit the empty links.utils.ts file (when we start moving some functions there we can create it) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (2)
46-61: LGTM: Chain information display block with a minor suggestion.The addition of this block to display the chain information when available is well-implemented. It maintains consistency with the previous blocks and includes a nice touch with the chain icon.
Consider adding a fallback for the chain icon in case the
findoperation doesn't return a result:<img - src={consts.supportedPeanutChains.find((detail) => detail.chainId === requestLinkData?.chainId)?.icon - .url + src={consts.supportedPeanutChains.find((detail) => detail.chainId === requestLinkData?.chainId)?.icon + ?.url ?? '/path/to/fallback/icon.png' } className="h-6 w-6" />This ensures that even if the chain icon is not found, a default icon is displayed, preventing potential rendering issues.
Line range hint
1-73: Address potential loss of functionality in token symbol display.As mentioned in a previous comment by jjramirezn, there might be a loss of functionality regarding the token symbol fallback. Previously, there was an attempt to get a token symbol if none was present in the data. Now, we're directly trusting the data we receive.
To address this:
- Investigate why
requestLinkDatasometimes doesn't have all required fields populated.- Consider implementing the token symbol fallback logic where
requestLinkDatais formed, ensuring all necessary data is available before reaching this component.- If the above is not feasible in the short term, you might want to reimplement the fallback logic in this component temporarily:
const tokenSymbol = requestLinkData?.tokenSymbol || getTokenSymbolFallback(requestLinkData);This ensures that we maintain the previous functionality while improving the overall display of information.
Would you like assistance in implementing this fallback logic or in investigating why
requestLinkDatamight be incomplete?🧰 Tools
🪛 Biome
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (3)
Learnt from: jjramirezn PR: peanutprotocol/peanut-ui#412 File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37 Timestamp: 2024-10-08T20:13:42.967Z Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.Learnt from: jjramirezn PR: peanutprotocol/peanut-ui#412 File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37 Timestamp: 2024-10-03T17:43:19.914Z Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.Learnt from: jjramirezn PR: peanutprotocol/peanut-ui#412 File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37 Timestamp: 2024-10-08T20:13:44.480Z Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.
🪛 Biome
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (5)
12-16: LGTM: Introduction of availability constants improves code clarity.The introduction of these constants to check the availability of specific properties in
requestLinkDataimproves code readability and maintainability. ThedataAvailableboolean effectively summarizes the availability of any relevant data.
21-25: LGTM: Conditional rendering of additional information.The addition of this conditional rendering block is a good improvement. It ensures that the additional information about the previously paid link is only displayed when relevant data is available, enhancing the user experience.
26-35: LGTM: Token symbol display block.The addition of this block to display the token symbol when available is well-implemented. It follows the component's design patterns and uses conditional rendering appropriately.
36-45: LGTM: Token amount display block.The addition of this block to display the token amount when available is well-implemented. It maintains consistency with the previous block and the overall component design, using conditional rendering appropriately.
62-71: LGTM: Requester's address display block.The addition of this block to display the requester's address when available is well-implemented. It maintains consistency with the previous blocks and appropriately uses the
utils.shortenAddressfunction to display the address in a user-friendly format.
|
|
||
| export const AlreadyPaidLinkView = ({ requestLinkData }: { requestLinkData: _consts.IRequestLinkData | undefined }) => { | ||
|
|
||
| const chainName = consts.supportedPeanutChains && consts.supportedPeanutChains.find((chain) => chain.chainId == requestLinkData?.chainId)?.name |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the chainName constant using optional chaining.
The chainName constant can be simplified using optional chaining for better readability and conciseness.
Apply this diff to refactor the code:
- const chainName = consts.supportedPeanutChains && consts.supportedPeanutChains.find((chain) => chain.chainId == requestLinkData?.chainId)?.name
+ const chainName = consts.supportedPeanutChains?.find((chain) => chain.chainId == requestLinkData?.chainId)?.name📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const chainName = consts.supportedPeanutChains && consts.supportedPeanutChains.find((chain) => chain.chainId == requestLinkData?.chainId)?.name | |
| const chainName = consts.supportedPeanutChains?.find((chain) => chain.chainId == requestLinkData?.chainId)?.name |
🧰 Tools
🪛 Biome
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
jjramirezn
left a comment
jjramirezn
left a comment
There was a problem hiding this comment.
Approved, merging.
Nice PR, Looks much better now!
3 participants
Previously, the claimed link view and already paid request link view did not include all data available. This resulted in poorer UX. This PR: