Refactor/UI ux improvements #1759
Conversation
WalkthroughThe changes involve multiple components in the application, enhancing error handling, modifying control flows, and improving user experience. Key updates include the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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 (
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (15)
web/src/components/ClaimPnkButton.tsx (3)
Line range hint
54-63: Improve error handling for missing wallet clientsThe current implementation silently fails when either
walletClientorpublicClientis unavailable. This doesn't align with the PR's objective of providing better error messages.Consider this improvement:
if (walletClient && publicClient) { wrapWithToast(async () => await walletClient.writeContract(request), publicClient) .finally(() => { setIsSending(false); }) .then(({ result, status }) => { setIsPopupOpen(status); status && setHash(result?.transactionHash); }); + } else { + setIsSending(false); + wrapWithToast( + () => Promise.reject(new Error("Please connect your wallet")), + publicClient + ); }
Line range hint
54-63: Consider using a try-catch block for better state managementThe current implementation might leave the component in an inconsistent state if the contract write fails before
wrapWithToast.Suggested improvement:
- if (walletClient && publicClient) { + try { + if (!walletClient || !publicClient) { + throw new Error("Wallet not connected"); + } wrapWithToast(async () => await walletClient.writeContract(request), publicClient) .finally(() => { setIsSending(false); }) .then(({ result, status }) => { setIsPopupOpen(status); status && setHash(result?.transactionHash); }); + } catch (error) { + setIsSending(false); + console.error("Failed to process transaction:", error); }
Line range hint
54-63: Enhance popup feedback for transaction statesTo improve UX, consider showing the popup with different states (pending/success/error) instead of only showing it on success.
Example implementation:
if (walletClient && publicClient) { + setIsPopupOpen(true); // Show pending state immediately wrapWithToast(async () => await walletClient.writeContract(request), publicClient) .finally(() => { setIsSending(false); }) .then(({ result, status }) => { - setIsPopupOpen(status); status && setHash(result?.transactionHash); + // Popup is already open, just update its state }); }This would require adding a
statusprop to thePopupcomponent to handle different states.web/src/pages/Cases/CaseDetails/Evidence/SubmitEvidenceModal.tsx (3)
94-94: LGTM! Modal closing behavior enhancement.The addition of
shouldCloseOnEscandshouldCloseOnOverlayClickprops improves the modal's usability by providing standard closing behaviors.Consider adding an aria-label for better accessibility:
- <StyledModal {...{ isOpen }} shouldCloseOnEsc shouldCloseOnOverlayClick onRequestClose={close}> + <StyledModal + {...{ isOpen }} + shouldCloseOnEsc + shouldCloseOnOverlayClick + onRequestClose={close} + aria-label="Submit Evidence Modal" + >
Line range hint
42-67: Enhance error messaging for better user experience.While the component handles errors, the error messages could be more descriptive to better guide users, as requested in issue #1749.
Consider enhancing the error handling:
const submitEvidence = useCallback(async () => { try { setIsSending(true); const evidenceJSON = await constructEvidence(uploadFile, message, file); const { request } = await simulateEvidenceModuleSubmitEvidence(wagmiConfig, { args: [BigInt(evidenceGroup), JSON.stringify(evidenceJSON)], }); - if (!walletClient || !publicClient) return; + if (!walletClient || !publicClient) { + toast.error("Please connect your wallet to submit evidence.", toastOptions); + return; + } await wrapWithToast(async () => await walletClient.writeContract(request), publicClient) .then(() => { setMessage(""); close(); }) .finally(() => setIsSending(false)); } catch (error) { - setIsSending(false); - toast.error("Failed to submit evidence.", toastOptions); + setIsSending(false); + const errorMessage = error instanceof Error ? error.message : "Unknown error occurred"; + toast.error(`Failed to submit evidence: ${errorMessage}`, toastOptions); console.error("Error in submitEvidence:", error); } }, [publicClient, wagmiConfig, walletClient, close, evidenceGroup, file, message, setIsSending, uploadFile]);
Line range hint
116-127: Improve file upload feedback and validation.The file upload process could benefit from better user feedback and validation.
Consider enhancing the file upload process:
const constructEvidence = async ( uploadFile: (file: File, role: Roles) => Promise<string | null>, msg: string, file?: File ) => { let fileURI: string | null = null; if (file) { + // Validate file size (e.g., 10MB limit) + if (file.size > 10 * 1024 * 1024) { + throw new Error("File size exceeds 10MB limit"); + } + toast.info("Uploading to IPFS", toastOptions); + // Show upload progress + toast.info("This may take a few moments...", { ...toastOptions, autoClose: false }); fileURI = await uploadFile(file, Roles.Evidence); - if (!fileURI) throw new Error("Error uploading evidence file"); + if (!fileURI) throw new Error("Failed to upload file to IPFS. Please try again."); + toast.success("File uploaded successfully!", toastOptions); } return { name: "Evidence", description: msg, fileURI }; };web/src/pages/Courts/CourtDetails/StakePanel/InputDisplay.tsx (1)
Line range hint
112-120: Consider making error messages more user-friendlyWhile the error handling logic is solid, the error messages could be more descriptive and user-friendly to better align with the PR's UX improvement goals. For example:
- setErrorMsg("You need a non-zero PNK balance to stake"); + setErrorMsg("Unable to stake: Your PNK balance is empty. Please acquire PNK tokens first."); - setErrorMsg("Insufficient balance to stake this amount"); + setErrorMsg("Unable to stake: The amount exceeds your available PNK balance. Please enter a smaller amount."); - setErrorMsg("Insufficient staked amount to withdraw this amount"); + setErrorMsg("Unable to withdraw: The amount exceeds your staked PNK. Please enter a smaller amount.");web/src/pages/Cases/CaseDetails/Appeal/Classic/Fund.tsx (2)
Line range hint
82-89: Enhance accessibility and user experienceThe component could benefit from improved accessibility and user feedback to better align with the PR's UI/UX improvement objectives.
Consider these improvements:
<StyledLabel>How much ETH do you want to contribute?</StyledLabel> <StyledField type="number" + aria-label="ETH amount to fund appeal" + min="0" + step="0.01" value={amount} onChange={(e) => { setAmount(e.target.value); }} placeholder="Amount to fund" />Also, consider adding more descriptive error messages for common scenarios:
{insufficientBalance && ( <ErrorButtonMessage> <ClosedCircleIcon /> - Insufficient balance + Insufficient balance: You need {debouncedAmount} ETH to fund this appeal </ErrorButtonMessage> )}Also applies to: 91-98
Based on the search results, I can now generate the final response as there's clear evidence of how popups are implemented across the codebase:
Implement consistent popup closing behavior using Overlay component
The codebase consistently uses an
Overlaycomponent to handle click-outside behavior for popups. Other components in the codebase implement this pattern:
- In
web/src/components/Popup/index.tsx:<Overlay onClick={closePopup}>
- In
web/src/pages/Courts/StakeMaintenanceButton/index.tsx:<Overlay onClick={() => setIsOpen(false)} />
- In
web/src/pages/Cases/CaseDetails/MaintenanceButtons/index.tsx:<Overlay onClick={() => setIsOpen(false)} />🔗 Analysis chain
Line range hint
82-98: Implement consistent popup closing behaviorTo align with the PR objective of closing popups when clicking outside, this component should implement the same behavior.
Let's check how other components handle popup closing:
Consider implementing a reusable hook or higher-order component to handle popup closing consistently across the application. This would ensure uniform behavior when users click outside or press the escape key.
Let's try to find modal/popup implementations and their closing behavior patterns in the codebase. The previous command failed because the file type wasn't recognized, so I've modified it to use glob patterns instead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for popup/modal closing implementations rg -A 5 "closePopup|setIsOpen.*false" --type tsxLength of output: 78
Script:
#!/bin/bash # Search for popup/modal closing implementations with .tsx extension rg -A 5 "closePopup|setIsOpen.*false" -g "*.tsx" # Also search for useOnClickOutside hook usage rg -A 5 "useOnClickOutside" -g "*.tsx" # Search for modal components to understand closing patterns rg -A 5 "Modal.*" -g "*.tsx"Length of output: 18901
web/src/pages/Resolver/NavigationButtons/SubmitDisputeButton.tsx (2)
66-72: Consider enhancing error message clarityWhile the error handling logic is good, the messages could be more specific and actionable:
Consider this improvement:
const errorMsg = useMemo(() => { - if (insufficientBalance) return "Insufficient balance"; + if (insufficientBalance) return "Insufficient balance to submit dispute. Please add funds to your wallet."; else if (error) { - return error?.shortMessage ?? error.message; + const msg = error?.shortMessage ?? error.message; + return `Unable to submit dispute: ${msg}`; } return null; }, [error, insufficientBalance]);
Line range hint
46-72: Consider adding unit tests for error scenariosThe new error handling logic would benefit from automated testing to ensure consistent behavior across different error scenarios.
Would you like me to help generate unit tests for these error scenarios? We should cover:
- Insufficient balance cases
- Simulation errors
- Various error message formations
web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx (3)
Line range hint
127-134: Consider adding specific error handlingWhile the transaction handling is functional, consider adding specific error cases to provide more detailed feedback to users.
if (setStakeConfig && publicClient) { setIsSending(true); wrapWithToast(async () => await setStake(setStakeConfig.request), publicClient) .then((res) => setIsPopupOpen(res.status)) + .catch((error) => { + if (error.code === 'INSUFFICIENT_FUNDS') { + setErrorMsg('Insufficient funds for staking'); + } else if (error.code === 'USER_REJECTED') { + setErrorMsg('Transaction rejected by user'); + } else { + setErrorMsg('Failed to stake: ' + error.message); + } + }) .finally(() => { setIsSending(false); }); }
155-159: Consider formatting error messages for better readabilityWhile the error handling is functional, the raw error messages might not be user-friendly.
useEffect(() => { if (setStakeError) { - setErrorMsg(setStakeError?.shortMessage ?? setStakeError.message); + const errorMessage = setStakeError?.shortMessage ?? setStakeError.message; + const formattedMessage = errorMessage + .replace(/^execution reverted: /i, '') + .replace(/^Error: /i, '') + .trim(); + setErrorMsg(formattedMessage); } }, [setStakeError]);
Line range hint
166-177: Consider extracting disabled conditions for better maintainabilityThe button's disabled logic combines multiple conditions. Consider extracting these into a separate function for better maintainability.
+const isButtonDisabled = ( + isSending: boolean, + parsedAmount: bigint, + targetStake: bigint | undefined, + courtDetails: any, + checkDisabled: () => boolean, + isStaking: boolean, + isAllowance: boolean, + setStakeConfig: any +) => { + if (isSending || parsedAmount === 0n || isUndefined(targetStake) || isUndefined(courtDetails)) { + return true; + } + if (checkDisabled()) return true; + if (targetStake !== 0n && targetStake < BigInt(courtDetails.court?.minStake)) return true; + if (isStaking && !isAllowance && isUndefined(setStakeConfig)) return true; + return false; +}; <Button text={text} isLoading={isSending} - disabled={ - isSending || - parsedAmount == 0n || - isUndefined(targetStake) || - isUndefined(courtDetails) || - checkDisabled() || - (targetStake !== 0n && targetStake < BigInt(courtDetails.court?.minStake)) || - (isStaking && !isAllowance && isUndefined(setStakeConfig)) - } + disabled={isButtonDisabled( + isSending, + parsedAmount, + targetStake, + courtDetails, + checkDisabled, + isStaking, + isAllowance, + setStakeConfig + )} onClick={onClick} />web/src/components/Popup/index.tsx (1)
Line range hint
1-278: Consider further modularization of popup types.While the current changes improve the popup interaction UX, the component handles multiple popup types with different behaviors. Consider:
- Extracting popup type-specific logic into separate components
- Using custom hooks for shared behaviors
- Implementing a popup context for managing state
This would improve maintainability and make it easier to add new popup types in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
web/src/components/ClaimPnkButton.tsx(1 hunks)web/src/components/Popup/index.tsx(2 hunks)web/src/pages/Cases/CaseDetails/Appeal/Classic/Fund.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Evidence/SubmitEvidenceModal.tsx(1 hunks)web/src/pages/Courts/CourtDetails/StakePanel/InputDisplay.tsx(2 hunks)web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx(7 hunks)web/src/pages/Resolver/NavigationButtons/SubmitDisputeButton.tsx(3 hunks)
🔇 Additional comments (9)
web/src/components/ClaimPnkButton.tsx (1)
Line range hint 54-63: Verify error handling in wrapWithToast utility
Let's ensure the error handling aligns with the PR objective of providing more descriptive error messages.
✅ Verification successful
Let me gather more specific information about the error handling implementation.
Error handling in wrapWithToast is properly implemented
The utility implements comprehensive error handling with descriptive error messages:
- Uses
shortMessagewith fallback tomessagefor user-friendly errors - Handles both transaction failures (
status !== "success") and execution errors - Provides appropriate toast notifications for different states (initiated, success, error)
- Returns consistent response structure with status and result
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of wrapWithToast utility
# Expected: Should find proper error message handling
# Look for the wrapWithToast implementation
rg -A 10 "export const wrapWithToast"
# Check for toast/notification related imports and usage
rg -l "toast|notification|error.*message" "web/src/utils/"
Length of output: 403
Script:
#!/bin/bash
# Check the implementation of wrapWithToast utility
cat web/src/utils/wrapWithToast.ts
# Look for error handling patterns in related utilities
rg -A 5 "catch|error" web/src/utils/wrapWithToast.ts
Length of output: 1872
web/src/pages/Courts/CourtDetails/StakePanel/InputDisplay.tsx (2)
107-107: LGTM! Improved null handling
The change from || to ?? operator is a good improvement as it specifically handles undefined values while properly preserving 0n values.
156-156: LGTM! Enhanced error handling capabilities
Adding setErrorMsg to StakeWithdrawButton aligns well with the PR objective of providing more descriptive error messages during staking operations.
Let's verify the error handling implementation in StakeWithdrawButton:
✅ Verification successful
Error handling implementation is properly integrated and utilized
The verification confirms that setErrorMsg is effectively used in both components:
- In
StakeWithdrawButton.tsx: Handles contract-level errors by setting error messages fromsetStakeError - In
InputDisplay.tsx: Manages validation errors for:- Zero balance checks
- Insufficient balance for staking
- Insufficient staked amount for withdrawals
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the error handling implementation in StakeWithdrawButton
# Check how the setErrorMsg prop is used in StakeWithdrawButton
ast-grep --pattern 'function StakeWithdrawButton({ $$$, setErrorMsg, $$$ }) {
$$$
}'
# Look for error message patterns
rg -A 3 'setErrorMsg\('
Length of output: 1566
web/src/pages/Resolver/NavigationButtons/SubmitDisputeButton.tsx (2)
46-46: LGTM! Good addition of error handling
The extraction of the error from the simulation hook is a good practice that enables better error feedback to users.
102-104: LGTM! Good error message presentation
The error message display with the ClosedCircleIcon provides clear visual feedback to users, following good UI/UX practices.
web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx (2)
46-46: LGTM: Error handling prop addition
The addition of setErrorMsg prop aligns well with the PR objective of improving error messaging for staking operations.
Also applies to: 55-55
119-120: LGTM: Enhanced validation logic
The addition of targetStake >= 0n check prevents negative stake attempts, improving input validation.
web/src/components/Popup/index.tsx (2)
239-242: LGTM! Well-structured centralization of popup closing logic.
The new closePopup function effectively consolidates the popup closing operations, improving code maintainability and reusability.
245-246: LGTM! Improved popup interaction handling.
The implementation correctly handles click events to:
- Close the popup when clicking outside (overlay)
- Prevent closing when clicking inside the popup (stopPropagation)
This directly addresses the PR objective for improving popup interaction UX.
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
github-actions
bot
added
Package: Web
|
Code Climate has analyzed commit dd18317 and detected 0 issues on this pull request. View more on Code Climate. |
|
closes #1749
PR-Codex overview
This PR focuses on improving error handling, adding functionality to components, and refining UI interactions across several files in the codebase.
Detailed summary
ClaimPnkButton.tsxto check bothwalletClientandpublicClient.Fund.tsx.SubmitEvidenceModal.tsxto allow closing via Esc and overlay click.InputDisplay.tsx.closePopupfunction inPopup/index.tsx.SubmitDisputeButton.tsxfor dispute submission.ErrorLabelinStakeWithdrawButton.tsxand added error message handling.publicClientis checked before contract interactions inStakeWithdrawButton.tsx.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
These updates enhance user experience and improve the robustness of error handling across the application.