Conversation
- fixes for video uploads - removed farcaster mini app submodule.
upload updates
upload updates
lint update
There was a problem hiding this comment.
Pull Request Overview
This PR improves video upload reliability, enhances thumbnail creation with NFT support and progress indicators, and updates API URL configuration across the codebase.
- Added metadata URI handling in video asset updates, including a polling mechanism after upload
- Updated thumbnail creation with live NFT configuration and progress bar for video transcoding
- Refactored environment variable usage and removed the unused mini‑app submodule
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| services/video-assets.ts | Added a new optional metadata_uri field in the video asset update |
| public/sw.js | Updated service worker hash revisions and module definitions |
| package.json | Upgraded dependency versions for account-kit and AA SDK packages |
| lib/sdk/stack/stackClient.ts | Removed an unused crypto import for cleaner code |
| lib/apollo-server-client.ts | Updated API URL env variables and error messages for consistency |
| components/Videos/Upload/index.tsx | Refactored imports and error handling; removed some legacy code |
| components/Videos/Upload/FileUpload.tsx | Introduced polling for metadata URI with a new isPolling state |
| components/Videos/Upload/CreateThumbnailForm.tsx | Refactored NFT config type and UI structure with improved clarity |
| components/Videos/Upload/Create-thumbnail.tsx | Added a progress bar and improved thumbnail submission error handling |
| app/apolloWrapper.tsx | Refactored to use the updated Snapshot API URL |
| .gitmodules | Removed the mini‑app submodule for repository simplification |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @sirgawain0x, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team, gemini-code-assist here with a summary of this pull request titled "Upload fixes". This PR introduces several key improvements and fixes related to the video upload process. The main areas addressed are enhancing video upload reliability and user feedback, improving thumbnail creation with NFT configuration support and transcoding progress, refactoring API URL usage for consistency, performing general code cleanup, and removing an unused git submodule.
Highlights
- Video Upload Reliability: Introduced a polling mechanism in
FileUpload.tsxto wait for the Livepeer metadata URI to be available before updating the database, improving the reliability of the upload flow. A visual indicator for this processing step has also been added. - Thumbnail Creation Enhancements: Updated the thumbnail creation process to include support for NFT configuration, allowing users to specify minting details. A progress bar for video transcoding has been added to
Create-thumbnail.tsxfor better user feedback, and error handling for submitting without selecting a thumbnail is now included. - API URL Consistency: Refactored GraphQL client configurations (
apolloWrapper.tsx,apollo-server-client.ts) to consistently use theNEXT_PUBLIC_SNAPSHOT_API_URLenvironment variable instead ofNEXT_PUBLIC_API_URL. - Code Cleanup: Removed unused imports and commented-out code in
index.tsxand removed an unused import instackClient.tsto streamline the codebase. Import formatting was also standardized inCreateThumbnailWrapper.tsx. - Submodule Removal: The
mini-appgit submodule has been removed from the repository configuration (.gitmodules), simplifying the project structure. - Dependency Updates: Updated several
@account-kitand@aa-sdkdependencies to version4.35.0inpackage.jsonandyarn.lock.
Changelog
Click here to see the changelog
- Removed the submodule commit hash entry.
- .gitmodules
- Removed the entry for the
mini-appsubmodule.
- Removed the entry for the
- app/apolloWrapper.tsx
- Updated the environment variable used for the GraphQL endpoint from
NEXT_PUBLIC_API_URLtoNEXT_PUBLIC_SNAPSHOT_API_URL(lines 9, 14). - Updated the error message to reflect the change in the environment variable name (line 10).
- Updated the environment variable used for the GraphQL endpoint from
- components/Videos/Upload/Create-thumbnail.tsx
- Imported the
Progresscomponent (line 18). - Added a toast error message if the user attempts to submit without selecting a thumbnail (lines 104-106).
- Added a progress bar and percentage display for video transcoding status, visible when the phase is not 'ready' or 'failed' (lines 118-138).
- Modified the
CreateThumbnailFormcomponent usage to handle thumbnail selection and NFT configuration changes separately viaonSelectThumbnailImagesandonNFTConfigChangeprops (lines 160-164).
- Imported the
- components/Videos/Upload/CreateThumbnailForm.tsx
- Updated the
CreateThumbnailFormPropsinterface to include an optionalonNFTConfigChangeprop for NFT configuration updates (lines 36-43). - Added the
onNFTConfigChangeprop to the component's destructuring (line 48). - Added a
useEffecthook to watch for changes in thenftConfigform values and call theonNFTConfigChangeprop if provided (lines 122-143). - Modified
handleSelectionChangeto only update the selected image state and callonSelectThumbnailImages, removing previous navigation logic (line 147). - Moved the 'Generate' button and the image results section to appear immediately after the prompt input field (lines 216-269).
- Removed the duplicate 'Generate' button and image results section from the end of the form (lines 277-330).
- Updated the
- components/Videos/Upload/CreateThumbnailWrapper.tsx
- Standardized import formatting from single quotes to double quotes (line 1).
- components/Videos/Upload/FileUpload.tsx
- Imported
getLivepeerAssetandupdateVideoAsset(lines 7, 8, 22). - Updated the
baseUrlenvironment variable used intranslateTextfromNEXT_PUBLIC_THIRDWEB_AUTH_DOMAINtoNEXT_PUBLIC_URL(line 50). - Added a new asynchronous function
pollForMetadataUrito repeatedly fetch Livepeer asset data until the metadata URI is available or a timeout occurs (lines 135-148). - Added
isPollingstate to track the metadata syncing process (line 172). - Added a new asynchronous function
handlePostUploadDbUpdatewhich usespollForMetadataUrito fetch the metadata URI and then updates the video asset in the database with the URI and other default values (lines 285-304). - Added a text indicator in the UI to show when the metadata syncing process is active (lines 485-488).
- Imported
- components/Videos/Upload/index.tsx
- Removed unused imports:
useAccount,useOrbisContext,stack,db,useUniversalAccount(lines 9, 12, 24, 25, 29). - Imported
useSmartAccountClientandgetLivepeerAsset(lines 30, 31). - Replaced the usage of
useUniversalAccountwithuseSmartAccountClientto get the user's address (line 49). - Added error handling and early return if
livepeerAssetormetadataare missing when attempting to save asset metadata (lines 150-153). - Fetched the latest Livepeer asset data using
getLivepeerAssetbefore updating the video asset in the database to ensure the most currentmetadata_uriis used (lines 161-162). - Included
metadata_uriin theupdateVideoAssetcall (lines 171-172). - Commented out the Stack points tracking and toast notification logic (lines 176-184).
- Updated the success toast message text (line 182 - commented out).
- Removed unused imports:
- lib/apollo-server-client.ts
- Updated the environment variable used for the GraphQL endpoint from
NEXT_PUBLIC_API_URLtoNEXT_PUBLIC_SNAPSHOT_API_URL(lines 4, 10). - Updated the error message to reflect the change in the environment variable name (line 5).
- Updated the environment variable used for the GraphQL endpoint from
- lib/sdk/stack/stackClient.ts
- Removed unused
cryptoimport (line 2).
- Removed unused
- package.json
- Updated
@aa-sdk/*dependencies to version4.35.0(lines 14-20). - Updated
@account-kit/*dependencies to version4.35.0(lines 16-20).
- Updated
- public/sw.js
- Updated build manifest revisions.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Uploads fixed with care,
Thumbnails shine, beyond compare.
Metadata syncs right,
Code is clean and bright,
Ready for the world to share.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the video upload functionality, thumbnail creation, and API URL configurations. The addition of polling mechanisms, progress indicators, and NFT configuration support are valuable enhancements.
The code is generally well-structured, and the changes align with the descriptions provided. I have a few points for discussion and minor suggestions that could further enhance clarity and robustness. Overall, great work on these updates!
Summary of Findings
- Error Message Specificity: Error messages for missing environment variables (e.g.,
NEXT_PUBLIC_SNAPSHOT_API_URL) could be more specific by including the variable name to aid debugging. This was noted inapp/apolloWrapper.tsxandlib/apollo-server-client.ts. - Clarification on
thumbnailUriinFileUpload.tsx: InhandlePostUploadDbUpdate,thumbnailUriis set to an empty string. Clarification is needed on whether this is a final state or if it will be populated later. - Polling Configuration in
FileUpload.tsx: The polling parameters (maxAttempts,interval) inpollForMetadataUriare hardcoded. While reasonable, a brief consideration if these should be configurable or if they are based on specific service SLAs might be useful for future adjustments. - Commented-Out Points Logic: The points awarding system (
stack.track) incomponents/Videos/Upload/index.tsxhas been commented out. The reason for this change (temporary, deprecation, rework) should be clarified. - Database Update Logic for
metadata_uri: Themetadata_urifield is now included inupdateVideoAsset. Ensuring thatnullorundefinedvalues are handled as expected by the database schema is important. - Code Repetition (Low Severity - Not Commented): In
components/Videos/Upload/Create-thumbnail.tsx, the logic for calculating progress percentage (livepeerAssetData.status.progress > 1 ? ... : ... * 100) is repeated. This is a minor point and could be extracted to a variable or helper if desired for further cleanup, but not critical. I did not create a review comment for this since it was low severity and you asked me not to comment on low severity issues.
Merge Readiness
The pull request introduces several valuable features and improvements. The changes are mostly clear and well-implemented. However, there are a few medium-severity items, primarily seeking clarification on certain implementation choices (e.g., the empty thumbnailUri, commented-out points logic) and suggesting minor improvements to error messaging.
I recommend addressing these points or providing clarification before merging to ensure full understanding and robustness of the new features. As an AI, I am not authorized to approve pull requests, but based on this review, I would suggest discussing the highlighted points with the author. Once these are resolved, the PR should be in a good state for merging.
| if (!process.env.NEXT_PUBLIC_SNAPSHOT_API_URL) | ||
| throw new Error("Snapshot URL is not defined"); |
There was a problem hiding this comment.
The new error message "Snapshot URL is not defined" is a bit generic. Could we make it more specific by mentioning the actual environment variable name that's missing, similar to the previous message? This can be helpful for debugging.
| if (!process.env.NEXT_PUBLIC_SNAPSHOT_API_URL) | |
| throw new Error("Snapshot URL is not defined"); | |
| if (!process.env.NEXT_PUBLIC_SNAPSHOT_API_URL) | |
| throw new Error("NEXT_PUBLIC_SNAPSHOT_API_URL is not defined. Please check your environment configuration."); |
| await updateVideoAsset(dbAssetId, { | ||
| metadata_uri: metadataUri, | ||
| thumbnailUri: "", // update as needed | ||
| status: "ready", | ||
| max_supply: null, | ||
| price: null, | ||
| royalty_percentage: null, | ||
| }); |
There was a problem hiding this comment.
In handlePostUploadDbUpdate, thumbnailUri is being set to an empty string with the comment // update as needed. Is this intentionally left blank at this stage, or is there a plan to populate this from the livepeerAsset or another source before this database update? If it's meant to be empty for now, perhaps a more explicit comment explaining why, or if it's populated in a subsequent step, could be beneficial for future maintainers.
await updateVideoAsset(dbAssetId, {
metadata_uri: metadataUri,
thumbnailUri: "", // Intentionally left blank, updated in thumbnail creation step
status: "ready",
max_supply: null,
price: null,
royalty_percentage: null,
});| async function pollForMetadataUri( | ||
| assetId: string, | ||
| maxAttempts = 20, | ||
| interval = 5000 | ||
| ) { | ||
| for (let attempt = 0; attempt < maxAttempts; attempt++) { | ||
| const asset = await getLivepeerAsset(assetId); | ||
| const metadataUri = asset?.storage?.ipfs?.nftMetadata?.url; | ||
| const isReady = asset?.status?.phase === "ready"; | ||
| if (metadataUri && isReady) return metadataUri; | ||
| await new Promise((res) => setTimeout(res, interval)); | ||
| } | ||
| throw new Error("Timed out waiting for metadata_uri"); | ||
| } |
There was a problem hiding this comment.
The polling mechanism in pollForMetadataUri has maxAttempts = 20 and interval = 5000 (5 seconds), leading to a total timeout of 100 seconds. This seems like a reasonable default. Have we considered if these values should be configurable, perhaps via environment variables, or if there are specific Livepeer processing times that guide these defaults? For most cases, this should be fine, but it's worth a quick thought.
| // Ensure all required data is present | ||
| if (!livepeerAsset || !metadata) { | ||
| throw new Error( | ||
| "Error saving assetMetadata: Missing asset metadata" | ||
| ); | ||
| toast.error("Missing asset metadata. Please try again."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
It's good to see user-facing error handling with toast.error here instead of throwing an error that might not be caught gracefully by the UI. Just a thought: for internal consistency or more detailed logging, would it also be beneficial to log this specific error (Missing asset metadata) to the console for developers, in addition to the user-facing toast?
if (!livepeerAsset || !metadata) {
console.error("Missing asset metadata: livepeerAsset or metadata is undefined");
toast.error("Missing asset metadata. Please try again.");
return;
}| // Award points for uploading a video | ||
| await stack.track("video_upload", { | ||
| account: address, | ||
| points: 10, | ||
| }); | ||
|
|
||
| const points = await stack.getPoints(address as string); | ||
| toast.success("Video uploaded successfully!", { | ||
| description: `You earned 10 points! Your total balance is now ${points} points.`, | ||
| }); | ||
| // await stack.track("video_upload", { | ||
| // account: address, | ||
| // points: 10, | ||
| // }); | ||
|
|
||
| // const points = await stack.getPoints(address as string); | ||
| // toast.success("Video uploaded and published successfully!", { | ||
| // description: `You earned 10 points! Your total balance is now ${points} points.`, | ||
| // }); | ||
|
|
There was a problem hiding this comment.
The logic for awarding points using stack.track and the related success toast has been commented out. Is this a temporary measure, or is this feature being deprecated or reworked? Clarifying the intent behind this change would be helpful.
| if (!process.env.NEXT_PUBLIC_SNAPSHOT_API_URL) | ||
| throw new Error("Snapshot URL is not defined"); |
There was a problem hiding this comment.
Similar to the client-side Apollo setup, the error message "Snapshot URL is not defined" is a bit generic. Could this also be made more specific by mentioning NEXT_PUBLIC_SNAPSHOT_API_URL to aid in debugging server-side configurations?
| if (!process.env.NEXT_PUBLIC_SNAPSHOT_API_URL) | |
| throw new Error("Snapshot URL is not defined"); | |
| if (!process.env.NEXT_PUBLIC_SNAPSHOT_API_URL) | |
| throw new Error("NEXT_PUBLIC_SNAPSHOT_API_URL is not defined for server-side client. Please check your environment configuration."); |
| royalty_percentage = ${data.royalty_percentage}, | ||
| metadata_uri = ${data.metadata_uri} |
There was a problem hiding this comment.
The metadata_uri is now being updated in the database. It's correctly typed as potentially null in the function parameters (data.metadata_uri?: string | null). In the SQL query, metadata_uri = ${data.metadata_uri} will insert NULL if data.metadata_uri is null or undefined, which is generally fine. Just confirming this behavior aligns with expectations for cases where latestAsset?.storage?.ipfs?.nftMetadata?.url might be missing in components/Videos/Upload/index.tsx.
1 participant
This pull request introduces several updates across the codebase, focusing on improving video upload functionality, enhancing thumbnail creation, and updating environment variable usage for API URLs. The changes include the addition of progress indicators, better error handling, new NFT configuration support, and refactoring to use updated environment variables.
Video Upload Enhancements:
FileUpload.tsxto wait for metadata URI updates from Livepeer before updating the database. Includes a new functionpollForMetadataUriand ahandlePostUploadDbUpdatemethod to improve video upload reliability. [1] [2]Thumbnail Creation Improvements:
CreateThumbnailForm.tsxto include an NFT configuration section with live updates to the parent component. Added a newonNFTConfigChangeprop and a correspondinguseEffectto handle changes. [1] [2]CreateThumbnail.tsxby adding a progress bar to show video transcoding progress and error handling for unselected thumbnails. [1] [2]API URL Refactoring:
NEXT_PUBLIC_API_URLwithNEXT_PUBLIC_SNAPSHOT_API_URLfor GraphQL endpoints inapolloWrapper.tsxandapollo-server-client.ts. Updated error messages to reflect the new variable. [1] [2]Code Cleanup and Refactoring:
index.tsxto streamline the file. [1] [2]CreateThumbnailWrapper.tsxto use consistent import formatting.Submodule Removal:
mini-appsubmodule from.gitmodules, simplifying the repository structure.