-
Notifications
You must be signed in to change notification settings - Fork 494
feat: add new files section for Metaplex tokens #666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
@overbit is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
d6fdf94 to
eac9889
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to eac9889 in 2 minutes and 6 seconds. Click for details.
- Reviewed
748lines of code in7files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. app/address/[address]/layout.tsx:111
- Draft comment:
The tab key was changed from 'metaplexNFT' to 'metaplex'. Ensure that all downstream routing and related logic are updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that all downstream routing and related logic are updated accordingly. This falls under asking the author to ensure something is tested or verified, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue, so it should be removed.
2. app/address/[address]/layout.tsx:58
- Draft comment:
The PR description mentions resolving an issue regarding transaction history for deleted accounts, but the changes here only add a new Files tab for Metaplex NFTs. Please verify that the intended issue is fully addressed. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. app/components/account/__tests__/MetaplexFilesCard.test.tsx:31
- Draft comment:
Global fetch is mocked here. Ensure that this global override does not leak into other tests or consider restoring global.fetch after these tests if necessary. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% 1. The comment is about test infrastructure rather than actual code changes. 2. The test file already follows best practices by using beforeEach() to reset mocks. 3. Global mocks in test files are a common and accepted practice. 4. The comment is speculative about potential issues rather than pointing out an actual problem. The comment raises a valid concern about test isolation, which is important for maintaining a reliable test suite. However, the test file already handles this correctly through vi.clearAllMocks() and proper test isolation. The comment is overly cautious about a non-issue. Delete the comment as it raises speculative concerns about a properly implemented testing pattern.
Workflow ID: wflow_CiOVObTOvatyMUND
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
@ngundotra can I get a review on this? |
|
Why not show the whole metadata json in the tab, instead of just doing showing |
|
@ngundotra we could do that as an another page. This files allows consistency with the attributes page |
|
@ngundotra WDYT? |
Description
This pull request introduces a new "Files" tab for Metaplex NFTs, adding functionality to display associated files and enhancing the user interface with new components and tests. The most important changes include creating new components for rendering Metaplex NFT files, updating the tab configuration to include the "Files" tab, and adding comprehensive tests to ensure functionality.
New Components and Functionality:
app/components/account/MetaplexFilesCard.tsx: Added theMetaplexFilesCardcomponent to fetch and display files associated with Metaplex NFTs. Includes error handling and a loading state.app/address/[address]/files/page-client.tsx: Introduced theMetaplexFilesPageClientcomponent to render theMetaplexFilesCardwithin aParsedAccountRendererandSuspenseboundary. (app/address/[address]/files/page-client.tsxR1-R28)app/address/[address]/files/page.tsx: Added a wrapper component,MetaplexFilesPage, to render theMetaplexFilesPageClient. (app/address/[address]/files/page.tsxR1-R11)Tab Configuration Updates:
app/address/[address]/layout.tsx: UpdatedTABS_LOOKUPand related logic to include the "Files" tab for Metaplex NFTs. Adjusted tab keys frommetaplexNFTtometaplexfor consistency. (app/address/[address]/layout.tsxL111-R111, app/address/[address]/layout.tsxR122-R126, app/address/[address]/layout.tsxL135-R140, app/address/[address]/layout.tsxR151-R155, app/address/[address]/layout.tsxR393, app/address/[address]/layout.tsxL446-R457)Tests:
app/address/[address]/files/__tests__/page-client.test.tsx: Added unit tests forMetaplexFilesPageClient, including mock implementations for dependencies and tests for rendering behavior. (app/address/[address]/files/tests/page-client.test.tsxR1-R97)app/address/[address]/files/__tests__/page.test.tsx: Added unit tests forMetaplexFilesPageto verify correct rendering and parameter passing. (app/address/[address]/files/tests/page.test.tsxR1-R41)Type of change
Screenshots
BEFORE

AFTER

Testing
Manual testing was also performed on devnet, using the following token:
http://localhost:3000/address/9a1nsHmVZsAHhyoHbAvK2CbShg6FsNTkohPfTKz8MCwt/files?cluster=devnet
Related Issues
Fixes #665
Checklist
Additional Notes