Testnets #138
Testnets #138
Conversation
fix: photo and video size and dimensions control, plus user feedback
✅ Deploy Preview for proof-of-humanity-v2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
WalkthroughThe changes introduce enhanced error handling mechanisms across multiple components in the application. Specifically, the Changes
Possibly related PRs
Poem
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: 4
🧹 Outside diff range and nitpick comments (7)
src/app/[pohid]/claim/Form.tsx (1)
222-222: Consider enhancing error handling capabilities.The current implementation of error handling via toast notifications is straightforward, but consider these improvements:
- Add error state clearing when the video becomes valid
- Consider using a more accessible error display method for persistent errors
- Add error boundary for catching unexpected video processing errors
Example enhancement:
- videoError={(ErrMsg) => toast.error(ErrMsg)} + videoError={{ + onError: (ErrMsg) => { + toast.error(ErrMsg); + setVideoError(ErrMsg); // Store error state + }, + onClear: () => setVideoError(null), // Clear error when resolved + }}src/app/[pohid]/claim/Video.tsx (4)
16-17: Remove commented-out code.These commented constants should be removed as they're no longer used and could cause confusion.
-//const MAX_DURATION = 20; // Seconds -//const MAX_SIZE = 7; // Megabytes
21-25: Consider type safety for error messages.Define error messages as an enum or const object with string literal types for better type safety and maintainability.
const ERROR_MSG = { duration: `Video is too long. Maximum allowed duration is ${MAX_DURATION} seconds long`, dimensions: `Video dimensions are too small. Minimum dimensions are ${MIN_DIMS.width}px by ${MIN_DIMS.height}px`, size: `Video is oversized. Maximum allowed size is ${MAX_SIZE}mb`, } as const; type ErrorType = keyof typeof ERROR_MSG;
Line range hint
147-161: Improve upload validation and resource management.The upload validation has similar issues to the recording flow. Additionally, there are potential memory leaks from the video element and object URLs.
+ const validateVideo = async (blob: Blob, uri: string): Promise<boolean> => { + try { + const [durationValid, sizeValid] = await Promise.all([ + checkVideoDuration(blob), + Promise.resolve(checkVideoSize(blob)) + ]); + + return durationValid && sizeValid; + } catch (error) { + videoError('Failed to validate video. Please try again.'); + console.error('Video validation error:', error); + URL.revokeObjectURL(uri); + return false; + } + }; const file = received[0]; const blob = new Blob([file], { type: file.type }); const uri = URL.createObjectURL(blob); - await checkVideoDuration(blob); - checkVideoSize(blob); + if (!await validateVideo(blob, uri)) { + return; + } const vid = document.createElement("video"); vid.crossOrigin = "anonymous"; vid.src = uri; vid.preload = "auto"; + const cleanup = () => { + vid.removeEventListener("loadeddata", validateDimensions); + vid.remove(); + }; - vid.addEventListener("loadeddata", async () => { + const validateDimensions = async () => { if ( vid.videoWidth < MIN_DIMS.width || vid.videoHeight < MIN_DIMS.height ) { videoError(ERROR_MSG.dimensions); - return console.error(ERROR_MSG.dimensions); + cleanup(); + URL.revokeObjectURL(uri); + return; } setRecording(false); video$.set({ uri, content: blob }); - }); + cleanup(); + }; + + vid.addEventListener("loadeddata", validateDimensions);
1-13: Consider separating validation logic into a custom hook.The component currently handles multiple responsibilities: UI rendering, video recording, upload handling, and validation. Consider extracting the validation logic into a custom hook for better maintainability and reusability.
Example implementation:
// useVideoValidation.ts export const useVideoValidation = (videoError: (error: string) => void) => { const validateSize = (blob: Blob): boolean => { // ... size validation logic }; const validateDuration = async (blob: Blob): Promise<boolean> => { // ... duration validation logic }; const validateDimensions = (width: number, height: number): boolean => { // ... dimension validation logic }; return { validateSize, validateDuration, validateDimensions, }; };src/app/[pohid]/claim/Photo.tsx (2)
Line range hint
81-102: Fix inconsistent error handling in the crop function.There are several issues in the error handling logic:
- The console.error after toast is redundant
- The commented out return statement for size validation means oversized images will still be processed
- The error handling is inconsistent between dimension and size validations
Apply this diff to fix the issues:
if ( cropPixels.width < MIN_DIMS.width || cropPixels.height < MIN_DIMS.height ) { toast.error(ERROR_MSG.dimensions); - return console.error("Dimensions error"); + return; } loading.start("Cropping photo"); const cropped = await getCroppedPhoto(originalPhoto.uri, cropPixels); if (!cropped) return; try { const sanitized = await sanitizeImage( Buffer.from(base64ToUint8Array(cropped.split(",")[1])), ); if (sanitized.size > MAX_SIZE_BYTES) { toast.error(ERROR_MSG.size); - //return console.error("Size error"); + return; } photo$.set({ content: sanitized, uri: URL.createObjectURL(sanitized) });
Line range hint
1-349: Consider architectural improvements for better maintainability.The component handles multiple complex responsibilities and could benefit from the following improvements:
Break down into smaller sub-components:
- PhotoUploader (file upload + webcam capture)
- PhotoCropper (cropping interface)
- PhotoPreview (final preview)
Extract state management into custom hooks:
- usePhotoCapture (file upload + webcam logic)
- usePhotoCrop (cropping state + validation)
Create a centralized error handling utility:
- Consolidate validation logic
- Standardize error messages
- Provide consistent user feedback
Would you like me to provide example implementations for any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/app/[pohid]/claim/Form.tsx(1 hunks)src/app/[pohid]/claim/Photo.tsx(4 hunks)src/app/[pohid]/claim/Video.tsx(5 hunks)
🔇 Additional comments (3)
src/app/[pohid]/claim/Form.tsx (2)
222-222: Verify error handling behavior in testnet environments.
Since this PR is related to testnets, ensure the error handling is robust enough for testnet-specific issues:
- Network-specific validation errors
- Contract interaction failures
- Different validation rules between testnet and mainnet
Let's check for network-specific handling:
#!/bin/bash
# Check for network-specific error handling
echo "Checking for network-specific error handling..."
rg -A 5 "chainId|networkId|testnet" "src/app/\[pohid\]/claim/"
# Check for contract-related error handling
echo "Checking contract error handling..."
ast-grep --pattern 'usePoHWrite($_, {
onError($_) {
$$$
}
})'222-222: Consider standardizing error handling across form steps.
While adding error handling for the video step is valuable, consider implementing a consistent error handling strategy across all form steps. This could include:
- Creating a shared error handling hook (e.g.,
useFormStepError) - Standardizing error message format and display methods
- Adding error tracking/logging for production monitoring
- Documenting error scenarios and handling strategies
Let's check for consistency in error handling across other step components:
src/app/[pohid]/claim/Photo.tsx (1)
3-3: LGTM! Well-structured constants and error handling setup.
The new constants and error messages are well-defined, providing clear validation boundaries and user-friendly error messages.
Also applies to: 14-14, 16-32
|
|
||
| await checkVideoDuration(blob); | ||
| checkVideoSize(blob); | ||
|
|
There was a problem hiding this comment.
Handle validation results in recording flow.
The validation results are currently ignored, and the code continues processing even if validation fails. Also, there's no error handling for potential getBlobDuration failures.
- await checkVideoDuration(blob);
- checkVideoSize(blob);
+ try {
+ const [durationValid, sizeValid] = await Promise.all([
+ checkVideoDuration(blob),
+ Promise.resolve(checkVideoSize(blob))
+ ]);
+
+ if (!durationValid || !sizeValid) {
+ return;
+ }
+
+ video$.set({ content: blob, uri: URL.createObjectURL(blob) });
+ setShowCamera(false);
+ } catch (error) {
+ videoError('Failed to validate video. Please try again.');
+ console.error('Video validation error:', error);
+ }
- video$.set({ content: blob, uri: URL.createObjectURL(blob) });
- setShowCamera(false);📝 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.
| await checkVideoDuration(blob); | |
| checkVideoSize(blob); | |
| try { | |
| const [durationValid, sizeValid] = await Promise.all([ | |
| checkVideoDuration(blob), | |
| Promise.resolve(checkVideoSize(blob)) | |
| ]); | |
| if (!durationValid || !sizeValid) { | |
| return; | |
| } | |
| video$.set({ content: blob, uri: URL.createObjectURL(blob) }); | |
| setShowCamera(false); | |
| } catch (error) { | |
| videoError('Failed to validate video. Please try again.'); | |
| console.error('Video validation error:', error); | |
| } | |
| const checkVideoSize = (blob: Blob) => { | ||
| if (MAX_SIZE_BYTES && blob.size > MAX_SIZE_BYTES) { | ||
| videoError(ERROR_MSG.size); | ||
| return console.error(ERROR_MSG.size); | ||
| } | ||
| }; | ||
|
|
||
| const checkVideoDuration = async (blob: Blob) => { | ||
| const duration = await getBlobDuration(blob); | ||
| if (duration > MAX_DURATION) { | ||
| videoError(ERROR_MSG.duration); | ||
| return console.error(ERROR_MSG.duration); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve validation function implementation.
The validation functions have several issues:
- Implicit undefined returns after errors
- Redundant console.error calls
- No success case handling
-const checkVideoSize = (blob: Blob) => {
+const checkVideoSize = (blob: Blob): boolean => {
if (MAX_SIZE_BYTES && blob.size > MAX_SIZE_BYTES) {
videoError(ERROR_MSG.size);
- return console.error(ERROR_MSG.size);
+ return false;
}
+ return true;
};
-const checkVideoDuration = async (blob: Blob) => {
+const checkVideoDuration = async (blob: Blob): Promise<boolean> => {
const duration = await getBlobDuration(blob);
if (duration > MAX_DURATION) {
videoError(ERROR_MSG.duration);
- return console.error(ERROR_MSG.duration);
+ return false;
}
+ return true;
};📝 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 checkVideoSize = (blob: Blob) => { | |
| if (MAX_SIZE_BYTES && blob.size > MAX_SIZE_BYTES) { | |
| videoError(ERROR_MSG.size); | |
| return console.error(ERROR_MSG.size); | |
| } | |
| }; | |
| const checkVideoDuration = async (blob: Blob) => { | |
| const duration = await getBlobDuration(blob); | |
| if (duration > MAX_DURATION) { | |
| videoError(ERROR_MSG.duration); | |
| return console.error(ERROR_MSG.duration); | |
| } | |
| }; | |
| const checkVideoSize = (blob: Blob): boolean => { | |
| if (MAX_SIZE_BYTES && blob.size > MAX_SIZE_BYTES) { | |
| videoError(ERROR_MSG.size); | |
| return false; | |
| } | |
| return true; | |
| }; | |
| const checkVideoDuration = async (blob: Blob): Promise<boolean> => { | |
| const duration = await getBlobDuration(blob); | |
| if (duration > MAX_DURATION) { | |
| videoError(ERROR_MSG.duration); | |
| return false; | |
| } | |
| return true; | |
| }; |
| const MAX_DURATION = 60 * 2; // Seconds | ||
| const MAX_SIZE = undefined; | ||
| const MAX_SIZE_BYTES = MAX_SIZE ? 1024 * 1024 * MAX_SIZE : MAX_SIZE; // Bytes |
There was a problem hiding this comment.
💡 Codebase verification
Video size limit removal needs attention
The codebase analysis reveals important context about size limits:
- Photo uploads have a defined 3MB limit (
MAX_SIZE = 3in Photo.tsx) - Video previously had a 7MB limit (commented out
//const MAX_SIZE = 7) - Setting
MAX_SIZE = undefinedeffectively removes all size validation ascheckVideoSize()only runs whenMAX_SIZE_BYTESis defined
Key concerns:
- Inconsistent validation between photo and video uploads
- No server-side upload limits found in configuration files
- Previous 7MB limit was likely set for valid technical/UX reasons
🔗 Analysis chain
Verify the removal of video size limit.
Removing the size limit (MAX_SIZE = undefined) could potentially lead to server-side issues or poor user experience with large file uploads. Consider:
- Server upload limits
- Network bandwidth constraints
- Storage costs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for server-side upload limits in configuration files
rg -i "upload.*limit|max.*size" --type yaml --type json
Length of output: 57
Script:
#!/bin/bash
# Let's check for any video upload size limits in the codebase, including configuration files and source code
rg -i "max.*size.*byte|upload.*limit|file.*size" -A 2 -B 2
# Also check for any video-related configuration or constants
rg -i "video.*size|video.*limit" -A 2 -B 2
# Look for any environment variables related to upload limits
rg -i "UPLOAD_|MAX_SIZE|MAX_FILE" .env* --type env
# Check for any server configuration files with different extensions
fd -e config.js -e config.ts -x cat {}
Length of output: 4754
| if ( | ||
| croppedPixels.width < MIN_DIMS.width || | ||
| croppedPixels.height < MIN_DIMS.height | ||
| ) { | ||
| toast.error(ERROR_MSG.dimensions); | ||
| console.error("Size error"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve dimension validation feedback.
There are several issues with the dimension validation in the crop complete callback:
- The console.error message is incorrect ("Size error" for dimension validation)
- The validation is duplicated between onCrop and onCropComplete
- Users need clearer guidance on how to fix dimension issues
Apply this diff to improve the validation:
if (
croppedPixels.width < MIN_DIMS.width ||
croppedPixels.height < MIN_DIMS.height
) {
- toast.error(ERROR_MSG.dimensions);
- console.error("Size error");
+ toast.warn(
+ `Please adjust the crop area to be at least ${MIN_DIMS.width}x${MIN_DIMS.height} pixels`
+ );
}Additionally, consider removing the dimension validation from the onCrop function since it's already handled here, or consolidate the validation logic into a shared function to avoid duplication.
Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Photocomponent.