-
Notifications
You must be signed in to change notification settings - Fork 497
feat: Improvements for InteractiveIDL UI #759
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
feat: Improvements for InteractiveIDL UI #759
Conversation
|
@askov is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 f155048 in 4 minutes and 24 seconds. Click for details.
- Reviewed
5181lines of code in82files - Skipped
2files when reviewing. - Skipped posting
8draft 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/features/idl/ui/IdlCard.tsx:50
- Draft comment:
Consider checking for 'null' explicitly in activeTabIndex to ensure robust tab selection. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. app/features/idl/model/use-tabs.tsx:26
- Draft comment:
Ensure that the useTabs hook properly handles cases when idl is null, so the returned tabs array is non-empty before render. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. app/shared/lib/triggerDownload.ts:2
- Draft comment:
Verify Buffer.from usage is safe in the browser – ensure polyfills are configured via Vite if needed. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. app/components/shared/ui/input.stories.tsx:6
- Draft comment:
Typographical error: the type declaration for 'InputVariant' is missing a closing '>' for the NonNullable generic. It should be: type InputVariant = NonNullable<React.ComponentProps['variant']>; - Reason this comment was not posted:
Comment looked like it was already resolved.
5. app/entities/idl/formatters/formatted-idl.d.ts:58
- Draft comment:
Typo: In the comment, there's inconsistent capitalization: you've writtenTypeFieldfor one seed andTypefieldfor another. Consider using consistent capitalization (e.g., alwaysTypeField). - Reason this comment was not posted:
Comment was on unchanged code.
6. app/features/idl/formatted-idl/ui/stories/AnchorFormattedIdl.stories.tsx:27
- Draft comment:
Typographical note: The export nameDisplayLegacyShankIdlseems inconsistent with the file/component nameAnchorFormattedIdl. If this is an oversight, consider renaming it toDisplayLegacyAnchorIdlfor consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is about a change in the diff (renaming "Primary" to "DisplayLegacyShankIdl"). It's pointing out a potential naming inconsistency - the file and component are about "Anchor" IDL, but the story is named with "Shank". This could be either: (1) an intentional name because the mock data is actually a Shank IDL being displayed by the Anchor formatter, or (2) a typo/mistake. Without seeing the actual mock data or understanding the domain better, I can't be 100% certain. However, the comment is speculative ("seems inconsistent", "If this is an oversight") and asks the author to "consider" renaming - it's not definitively stating there's a problem. This violates the rule about not making speculative comments. The comment might be correct if "Shank" is indeed a mistake and should be "Anchor". However, it's also possible that the naming is intentional - perhaps the mock data represents a legacy Shank IDL that's being formatted using the Anchor formatter. The comment is phrased speculatively ("seems inconsistent", "If this is an oversight"), which suggests uncertainty. While there might be a legitimate naming issue, the speculative nature of the comment ("seems", "if this is an oversight", "consider") means it doesn't meet the bar of STRONG EVIDENCE that there's a problem. The rules explicitly state not to make speculative comments. If the author named it "DisplayLegacyShankIdl", they likely had a reason, and without definitive proof it's wrong, this comment should be removed. This comment should be deleted because it's speculative and doesn't provide strong evidence of an actual problem. The author may have intentionally named it "DisplayLegacyShankIdl" for a valid reason (e.g., the mock represents a Shank IDL being formatted by the Anchor formatter).
7. app/features/idl/ui/IdlCard.tsx:65
- Draft comment:
Typographical note: The class name 'e-border-0' on line 65 might be a typo. It’s worth confirming if this was intended or if it should be 'border-0'. - 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% This comment is speculative ("might be a typo", "worth confirming") rather than definitively identifying an issue. The rules explicitly state "Do NOT make speculative comments" and "Do NOT ask the PR author to confirm their intention, to explain, to double-check things". The comment is asking the author to confirm if something was intended, which violates the rules. Additionally, this is a UI/styling change (CSS class names), and the rules say to assume UI changes were done correctly. Without access to the CSS files or styling system, there's no strong evidence thate-border-0is wrong - it could be a legitimate custom class in their design system. Coulde-border-0actually be a legitimate custom CSS class in this project's styling system? Without seeing the CSS files or knowing the project's conventions, I can't be certain this is a typo. Exactly - this comment is speculative and asks for confirmation, which violates the rules. There's no strong evidence that this is actually wrong, and it could very well be an intentional custom class name in their styling system. This comment should be deleted because it's speculative ("might be a typo"), asks the author to confirm their intention ("worth confirming"), and relates to styling/UI where we should assume the author made the correct choice. There's no strong evidence this is actually wrong.
8. app/styles.css:4
- Draft comment:
Typo: The CSS pseudo-class for the root element should be written as ':root', not '::root'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment appears to be incorrect. The diff shows:root {with a single colon on line 4, which is the correct CSS syntax. The full file context confirms this at L4. The comment claims there's a typo and suggests changing::rootto:root, but the code already has:root(single colon). This suggests the automated tool misread the code or made an error. The comment is factually wrong because it's identifying a problem that doesn't exist. Could the diff or file display be misleading me? Is there any scenario where the comment could be correct despite what I'm seeing? The diff clearly shows+:root {with a single colon (the correct syntax), and the full file context at L4 confirms:rootwith a single colon. There's no ambiguity here - the comment is simply incorrect. The comment should be deleted because it incorrectly identifies a typo that doesn't exist. The code already uses the correct:rootsyntax with a single colon, not::rootas the comment claims.
Workflow ID: wflow_zhbEVDqdhbVjErvQ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
f155048 to
73ac554
Compare
73ac554 to
fb6e635
Compare
notEduardo
left a comment
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.
LGTM but small updates needed. Also this PR conflicts with this PR @jacobcreech @Woody4618
app/shared/lib/triggerDownload.ts
Outdated
| export const triggerDownload = async (data: string, filename: string, type?: string): Promise<void> => { | ||
| const blob = new Blob([Buffer.from(data, 'base64')], type ? { type } : {}); |
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.
I'm not familiar with the type of data being downloaded, but is it possible that the data or filename could be malicious? if so, we could update this method with some validations
| export const triggerDownload = async (data: string, filename: string, type?: string): Promise<void> => { | |
| const blob = new Blob([Buffer.from(data, 'base64')], type ? { type } : {}); | |
| export const triggerDownload = async (data: string, filename: string, type?: string): Promise<void> => { | |
| // Validate base64 format | |
| if (!/^[A-Za-z0-9+/]*={0,2}$/.test(data)) { | |
| throw new Error('Invalid base64 data'); | |
| } | |
| // Sanitize filename to prevent directory traversal | |
| const sanitizedFilename = filename.replace(/[^a-z0-9._-]/gi, '_'); | |
| // Limit file size to prevent DoS (e.g., 10MB) | |
| const decoded = Buffer.from(data, 'base64'); | |
| if (decoded.length > 10 * 1024 * 1024) { | |
| throw new Error('File too large to download'); | |
| } | |
| const blob = new Blob([Buffer.from(data, 'base64')], type ? { type } : {}); |
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.
Good catch! In our case, all these parameters aren't related to user input (we define them with code, and if something bad happens, it occurs before this function call). Moreover, this function is just a copy and paste from the already existing production code (see Downloadable.tsx)
I've improved the function to be more robust and secure.
I haven't included any sanitization because modern browsers already do this job well; see more details in the comments. Also, to prevent memory issues, we should validate the size before decoding, as it creates even more problems (see the comments).
app/features/idl/ui/IdlCard.tsx
Outdated
| } | ||
| }, [tabs]); | ||
|
|
||
| if ((!idl && !programMetadataIdl) || activeTabIndex == undefined) { |
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.
== treats null and undefined as equal, which could hide bugs. Use strict equality for type safety.
| if ((!idl && !programMetadataIdl) || activeTabIndex == undefined) { | |
| if ((!idl && !programMetadataIdl) || activeTabIndex === undefined) { |
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.
In this case it doesn't matter, but thanks for the suggestion. Maybe we should change eslint rules to forbid this usage?
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.
not sure, up to @Woody4618 @jacobcreech but this would be the line to add. More context
"eqeqeq": ["error", "always"]
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.
yeah im fine with changing the rule.
Co-authored-by: Eduardo Gonzalez <[email protected]>
notEduardo
left a comment
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.
LGTM @jacobcreech @Woody4618
## Description Implemented highlight search functionality for IDL components, results counter for tabs. ###⚠️ Important This PR depends on #759, so there is actually only one commit. Should be rebased as soon as the base PR is merged ###⚠️ Stories review Reviewing the stories is optional. This code does not affect production and is intended for developers only. ## Type of change <!-- Check the appropriate options that apply to this PR --> - [ ] Bug fix - [x] New feature - [ ] Protocol integration - [ ] Documentation update - [ ] Other (please describe): ## Screenshots <img width="1710" height="1839" alt="localhost_3000_address_waveQX2yP3H1pVU8djGvEHmYg8uamQ84AuyGtpsrXTF_idl" src="https://github.com/user-attachments/assets/5635ccaf-ebb5-4bed-9fb0-b84c649133df" /> https://github.com/user-attachments/assets/357e4d9f-3ad3-4ce2-b5e9-546a15df1c1d ## Testing - Visit http://localhost:3000/address/waveQX2yP3H1pVU8djGvEHmYg8uamQ84AuyGtpsrXTF/idl - Enter some search prompt - See the counters in tabs - See the highlighted results ## Related Issues N/A ## Checklist <!-- Verify that you have completed the following before requesting review --> - [x] My code follows the project's style guidelines - [x] I have added tests that prove my fix/feature works - [x] All tests pass locally and in CI - [ ] I have updated documentation as needed - [x] CI/CD checks pass - [x] I have included screenshots for protocol screens (if applicable) - [ ] For security-related features, I have included links to related information <!-- ELLIPSIS_HIDDEN --> ---- > [!IMPORTANT] > Implement highlight search functionality and results counter for IDL components, enhancing the developer interface with search and highlight features. > > - **Behavior**: > - Implement highlight search functionality for IDL components in `features/idl/formatted-idl/ui`. > - Add results counter for tabs in `use-tabs.tsx`. > - Search functionality uses `Fuse.js` for fuzzy searching in `search.ts`. > - **Components**: > - Add `HighlightNode` and `BaseHighlightNode` for text highlighting. > - Update `IdlCard`, `IdlSection`, and `IdlRenderer` to integrate search and highlight features. > - Add `SearchHighlightContext` for managing search state. > - **Testing**: > - Add tests for `HighlightNode` in `HighlightNode.test.tsx`. > - Add stories for various IDL components in `stories` directory. > - **Misc**: > - Update `styles.css` and `tailwind.config.ts` for styling changes. > - Add `triggerDownload` utility in `shared/lib` for downloading IDL data. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=solana-foundation%2Fexplorer&utm_source=github&utm_medium=referral)<sup> for 0b9d1c5. You can [customize](https://app.ellipsis.dev/solana-foundation/settings/summaries) this summary. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
Description
Rework the existing IDL tab components for consistency according to the design.
Reviewing the stories is optional. This code does not affect production and is intended for developers only.
Type of change
Screenshots
Testing
Related Issues
N/A
Checklist
Important
Enhance InteractiveIDL UI by refactoring IDL components, adding structured rendering, and improving styling and testing.
IdlCardinIdlCard.tsxto use tabs forprogram-metadataandanchorIDLs.IdlSectioninIdlSection.tsxfor rendering IDL with search and download features.BaseFormattedIdland related components for structured IDL rendering.BaseIdlAccounts,BaseIdlConstants,BaseIdlErrors,BaseIdlEvents,BaseIdlFields,BaseIdlInstructions,BaseIdlPdas,BaseIdlTypesfor detailed IDL sections.AnchorFormattedIdlandCodamaFormattedIdlfor specific IDL formats.triggerDownloadintriggerDownload.tsfor downloading IDL files.tailwind.config.tsandstyles.cssfor styling consistency.IdlCardinIdlCard.spec.tsxto verify rendering logic.This description was created by
for f155048. You can customize this summary. It will automatically update as commits are pushed.