feat: improve component to display information about AssociatedTokenProgram instruction at inspector#484
Merged
ngundotra merged 40 commits intosolana-foundation:masterfrom Mar 14, 2025
Conversation
|
@rogaldh is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 132c528 in 3 minutes and 29 seconds
More details
- Looked at
1203lines of code in26files - Skipped
1files when reviewing. - Skipped posting
21drafted comments based on config settings.
1. app/__tests__/utils.ts:9
-
Draft comment:
This is an exact duplicate of the existing mockUseSearchParams function. Please use the existing implementation instead. -
function
mockUseSearchParams(mocks.ts) -
Reason this comment was not posted:
Marked as duplicate.
2. app/__tests__/mock-stubs.ts:1
- Draft comment:
Typo: "servialized" should be "serialized". - 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%
While this is a real typo, it's in a documentation comment that doesn't affect functionality. The rules say to focus on logic issues, not minor text fixes. However, since this is a new file being added, catching typos early is reasonable. But is this important enough to warrant a comment?
Documentation quality does matter for maintainability. Poor spelling could indicate lack of attention to detail.
However, this is a very minor issue in a test file's documentation. The meaning is still clear despite the typo.
This comment, while technically correct, is too minor to warrant inclusion. It violates the rule about not making unimportant comments.
3. app/__tests__/mocks.ts:8
- Draft comment:
Consider consolidating duplicate implementations of mockUseSearchParams (present in both mocks.ts and utils.ts) to avoid maintenance overhead. - Reason this comment was not posted:
Marked as duplicate.
4. app/features/feature-gate/__tests__/feature-gate.spec.ts:53
- Draft comment:
Consider awaiting the promise returned by fetchFeatureGateInformation to ensure the test handles asynchronous operations properly. - 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.
5. app/components/inspector/utils.ts:127
- Draft comment:
Hardcoded buffer constants (CREATE_BUFFER, CREATE_IDEMPOTENT_BUFFER, CREATE_RECOVER_NESTED_BUFFER) may risk future inconsistency with actual instruction definitions. Consider extracting these to a shared constants module. - 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 code already acknowledges this is temporary with a TODO. The suggestion to move to a shared module doesn't fully address the core issue - keeping in sync with core repositories. Moving to another file doesn't solve the synchronization problem. The author seems aware of the limitation and plans to address it properly later.
The comment does identify a real code quality issue. Moving constants to a shared module could help with maintainability even if it doesn't solve the sync problem.
While the suggestion has merit, the existing TODO indicates the team is already aware and planning a more comprehensive solution. Moving constants without solving the sync issue would be premature.
Delete the comment since the code already acknowledges the issue with a TODO and the team plans to address it more comprehensively.
6. app/components/inspector/associated-token/AssociatedTokenDetailsCard.tsx:24
- Draft comment:
In the try block, catching errors only to return UnknownDetailsCard might hide parsing issues. Consider logging or re-throwing unexpected errors to aid debugging. - 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%
Looking at the file header comments, this is explicitly meant to handle transactions that may not have parsed data. The UnknownDetailsCard seems to be an intentional fallback for unparseable cases. Adding error logging here may just create noise since parsing failures are an expected case. The comment is suggesting a change but it's not clear there's actually a problem to solve.
I could be wrong about the frequency of parsing errors - if they're rare, logging them could help catch real issues. The comment does suggest a concrete improvement to debugging capabilities.
The file's purpose is specifically to handle cases where parsing may fail, so adding error logging would likely just create noise without providing value. The fallback behavior seems intentional.
Delete the comment since the current error handling appears to be an intentional design choice given the component's purpose, and there's no strong evidence that adding error logging would be beneficial.
7. app/components/instruction/__tests__/into-transaction-instruction-from-versioned-message.spec.tsx:15
- Draft comment:
Test cases assume specific structure of the addressTableLookups. Ensure that edge cases, such as empty lookup arrays, are also tested. - 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.
8. app/package.json:55
- Draft comment:
Ensure that dependency versions (e.g., React 18.3.1) are up-to-date and compatible with Next.js 14.2.5 to avoid potential conflicts or deprecated APIs. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
9. app/scss/dashkit/dark/_overrides-dark.scss:33
- Draft comment:
SCSS overrides look fine; ensure color codes like '#33a382' align with design guidelines and are consistent across themes. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
10. jest.setup.js:8
- Draft comment:
Good enabling of fetch-mock in jest.setup.js; consider adding comments if additional global setup is needed for tests. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
11. app/__tests__/mocks.ts:9
- Draft comment:
Avoid using @ts-expect-error for mocking useSearchParams. Instead consider setting up a proper jest.fn() with correct type definitions. - 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%
This is a test file and the @ts-expect-error is a common pattern in test mocks. While better typing would be nice, this works and is clear enough. The comment suggests a refactor that isn't clearly better - setting up proper jest.fn() types could be more complex than this simple ignore. The current approach is pragmatic and commonly used in test files.
The comment might have a point about type safety - @ts-expect-error can hide real type issues. Also, proper typing could help catch breaking changes in the useSearchParams API.
In test mocks, some type gymnastics are often necessary and @ts-expect-error is an accepted pattern. The current solution is simple and clear, while proper typing could add complexity without much benefit.
Delete the comment. While it suggests a possible improvement, the current approach is a common and acceptable pattern for test mocks, and the suggested change isn't clearly better.
12. app/__tests__/utils.ts:8
- Draft comment:
Similar to mocks.ts, avoid using @ts-expect-error for useSearchParams mock – consider refining the type or using jest.fn() with proper type extension. - 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%
This is a new test utility file. The comment suggests a legitimate TypeScript improvement. However, @ts-expect-error is a common and acceptable practice in test files, especially for mocks. The suggestion to use jest.fn() wouldn't necessarily solve the typing issue without additional type setup. The benefit of implementing this suggestion is minimal compared to the current working solution.
The comment might be pointing to a real opportunity to improve type safety. Poor typing in test files can hide real issues.
While type safety is important, @ts-expect-error is a standard practice in test files and the current implementation is clear and functional.
Delete the comment. The current implementation using @ts-expect-error is a reasonable approach for test mocks, and the suggested changes would add complexity without significant benefit.
13. app/components/common/instruction/associated-token/BaseCreateDetailsCard.tsx:37
- Draft comment:
Ensure that elements are rendered within a proper element to maintain semantic HTML. - 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%
While the comment raises a valid HTML semantic concern, we can't see the implementation of InstructionCardComponent. It's quite possible that InstructionCardComponent provides the table context. Without seeing the parent component, we can't be certain this is actually an issue. The comment is also phrased as "Ensure that..." which violates our rules.
The HTML semantics issue could be a real problem if InstructionCardComponent doesn't provide a table context. Invalid HTML structure can cause accessibility issues.
However, this is a speculative comment without full context, and it starts with "Ensure that..." which makes it a verification request rather than a clear issue identification.
Delete the comment because it's speculative without full context and is phrased as a verification request rather than identifying a clear issue. - Draft comment:
Avoid hardcoding buffer constants for instruction types. Consider defining these as named, shared constants to improve maintainability. - 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 constants are part of new code being added. The code itself acknowledges this is temporary with a TODO. The constants are tightly coupled with their usage in this specific function. Moving them could actually make the code harder to understand since they're only used here. The TODO suggests a better solution is planned.
The comment has merit from a general code quality perspective. Shared constants can improve maintainability. The code does seem to be handling important program instruction types.
However, given the TODO and temporary nature of this code, plus the fact these constants are only used in this one place, moving them now would be premature. The author already acknowledges a better solution is needed.
The comment should be deleted. The temporary nature of this code is already acknowledged, and making this change now would be premature given the planned improvements noted in the TODO. - Draft comment:
Reorder the error simulation: call mockRejectOnce before invoking fetchFeatureGateInformation to properly simulate a network error. - 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. - Draft comment:
There is a discrepancy between the version of 'node-fetch' in dependencies (^3.3.2) and the override (^2.6.9). Confirm if this is intentional and compatible. - 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. - Draft comment:
Consider handling errors more gracefully instead of just logging with console.error. This might be refined for production environments. - 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. - Draft comment:
Typographical error: 'servialized' should be corrected to 'serialized'. - 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%
While this is technically correct - there is a typo - the rules say not to make purely informative comments and to only comment if there's clearly a code change required. This is just a documentation typo that doesn't affect functionality. The rules emphasize focusing on logic issues rather than minor text fixes.
The typo could potentially confuse future readers of the code. Documentation quality is important for code maintainability.
While documentation quality matters, this is an extremely minor issue that doesn't impact code functionality. The meaning is still clear despite the typo.
This comment should be deleted as it's too minor and doesn't require a functional code change. - Draft comment:
Typographical error: 'resemblace' on line 2 should be corrected to 'resemblance'. - 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%
While the typo correction is technically accurate, it's in a documentation comment and doesn't affect functionality. The rules state we should not make purely informative comments that don't require code changes. Typos in comments, while good to fix, are not critical enough to warrant a PR comment.
The typo could potentially cause confusion for future developers reading the documentation. Documentation quality is important for maintainability.
While documentation quality matters, this particular typo is minor and obvious enough that it doesn't significantly impact understanding. The meaning is still clear from context.
Delete this comment as it's too minor and doesn't affect code functionality. It violates the rule about not making purely informative comments. - Draft comment:
Typo in comment: "Temporary keep enums hardcoded." Consider using "Temporarily keep enums hardcoded." for better grammar. - 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%
While the grammar correction is technically correct, it's a very minor issue that doesn't affect code functionality or clarity. The meaning is still clear in the original. The rules state we should not make purely informative comments or obvious/unimportant ones. Grammar in comments, unless severely impacting readability, falls into this category.
The comment is technically correct and improves the code quality slightly. Poor grammar in comments could be seen as reducing code professionalism.
While correct, this type of minor grammar fix in comments doesn't meet the bar for "clearly a code change required" as specified in the rules. It's more of a nice-to-have than a must-fix issue.
Delete the comment as it's too minor and doesn't meet the criteria for required code changes. - Draft comment:
Typographical suggestion: In the comment "parsed - allow to pass any data as we can not recover parsed data", consider replacing "can not" with "cannot" for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative and suggests a typographical change that does not impact the functionality or logic of the code. It does not provide a code suggestion or address a potential issue in the code.
14. app/components/inspector/utils.ts:127
15. app/features/feature-gate/__tests__/feature-gate.spec.ts:53
16. package.json:52
17. app/components/instruction/associated-token/AssociatedTokenDetailsCard.tsx:41
18. app/__tests__/mock-stubs.ts:2
19. app/components/inspector/associated-token/AssociatedTokenDetailsCard.tsx:2
20. app/components/inspector/utils.ts:125
21. app/components/inspector/utils.ts:172
Workflow ID: wflow_3WUrSrLQ3jaxKlsH
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
rogaldh
force-pushed
the
feat/decode-ata-data-at-inspector
branch
from
132c528 to
7210505
Compare
Contributor
Author
UPD: it's done |
…a at inspector page
…remove annoying react.act warning
rogaldh
force-pushed
the
feat/decode-ata-data-at-inspector
branch
from
7210505 to
8150f18
Compare
ngundotra
reviewed
Mar 7, 2025
app/components/inspector/utils.ts
Outdated
Comment on lines
+107
to
+114
| /** | ||
| * entity that performs conversion from TransactionInstruction that is created from VersionedMessage into ParsedInstruction. | ||
| * | ||
| * That is needed for backward compatibility with existing InstructionCards that expect ParsedInstruction for rendering. | ||
| * | ||
| * This is temporary solution to reuse existing cards at the Inspector. Might pivot to a better solution in future. | ||
| */ | ||
| export function ParsedInstructionFactory(){ |
Contributor
There was a problem hiding this comment.
This is a super confusing pattern. Do you mind explaining why this is needed with a concrete example?
Contributor
Author
There was a problem hiding this comment.
I did it so just to "connect" two different functions which have similar purpose: transform VersionedMessage into the form which is supported by components that work with ParsedInstruction.
This "factory" have two public methods and several "private" ones.
I can move this into separate file and just export these two methods out of it and keep all the internal functions. This would be the same.
factory.intoParsedInstruction(ti, parsed); > intoParsedInstruction(ti, parsed);
factory.intoParsedTransaction(ti, m); > intoParsedTransaction(ti, m);
Comment on lines
+43
to
+44
| const ix = factory.intoParsedInstruction(ti, parsed); | ||
| const tx = factory.intoParsedTransaction(ti, m); |
Contributor
There was a problem hiding this comment.
why isn't this just done with a normal function? Why is factory pattern needed here?
Contributor
Author
There was a problem hiding this comment.
I've answered at the previous comment
ngundotra
suggested changes
Mar 7, 2025
Contributor
ngundotra
left a comment
ngundotra
left a comment
There was a problem hiding this comment.
This localhost link shows
which doesn't match parity of the original transaction.
Looks like there's a small bug somewhere
Contributor
Author
I didn't use component that parsed instruction and renders addresses according the AddressLookupTable present. Fixed |
…enProgram instruction
rogaldh
force-pushed
the
feat/decode-ata-data-at-inspector
branch
from
e2317c1 to
2171585
Compare
rogaldh
force-pushed
the
feat/decode-ata-data-at-inspector
branch
from
2171585 to
dfa4bcb
Compare
rogaldh
force-pushed
the
feat/decode-ata-data-at-inspector
branch
from
0a4b9c9 to
eadf497
Compare
ngundotra
suggested changes
Mar 14, 2025
Contributor
ngundotra
left a comment
ngundotra
left a comment
There was a problem hiding this comment.
Almost ready to go! Just some final changes for create & parsing
Comment on lines
+71
to
+80
| <td>System Program</td> | ||
| <td className="text-lg-end"> | ||
| <AddressTableLookupAddress accountIndex={raw.accountKeyIndexes[4]} message={message} hideInfo /> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <td>Token Program</td> | ||
| <td className="text-lg-end"> | ||
| <AddressTableLookupAddress accountIndex={raw.accountKeyIndexes[5]} message={message} hideInfo /> | ||
| </td> |
Contributor
There was a problem hiding this comment.
This should be included in the CreateDetailsCard.tsx too
| <tr> | ||
| <td>Account</td> | ||
| <td className="text-lg-end"> | ||
| <AddressTableLookupAddress accountIndex={raw.accountKeyIndexes[1]} message={message} hideInfo /> |
Contributor
There was a problem hiding this comment.
It feels wrong that we've done all this parsing & prop drilling... Only to use accountKeyIndexes[i] to map accounts... Is there anything we can do to use the result of parse....(ix) ? from into-parsed-data.ts ?
Contributor
Author
There was a problem hiding this comment.
Having accounts from the address-lookup-table on the inspector page requires us to calculate the real address by index.
At this step, logic is in the component that "knows" how to deal with the address-lookup-table cache (there is a layer of fetching inside those hooks). To allow calculating real addresses instead of addresses at the lookup table, I need to extract that logic and move it into into-parsed-data.ts. That looks like a separate task that requires tests, and they might be quite complicated.
Here is the sample for CreateIdempotent instruction.
That's why I decided to use accounts by their position and handle them to the component that can resolve the address to the real one.
| const transactionInstruction = intoTransactionInstructionFromVersionedMessage(ix, message); | ||
|
|
||
| if (anchorProgram.program) { | ||
| const programName = getAnchorProgramName(anchorProgram.program) ?? 'Unknown Program'; |
Contributor
There was a problem hiding this comment.
Please remove this. This call can throw, and it would not get caught in the ErrorBoundary.
Also if we fail to deserialize the Anchor instruction, we may not have an Anchor program name
ngundotra
approved these changes
Mar 14, 2025
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds AssociatedTokenDetailsCard at inspector.
Minor addition: added shadow for the "Raw" button to highlight active state.
Type of change
Screenshots
Testing
Run application and check this transaction at inspector.
Checklist
Important
This PR adds a feature to display details of AssociatedTokenProgram instructions in the inspector, including new components, tests, and UI updates.
AssociatedTokenDetailsCardto display details ofAssociatedTokenPrograminstructions inInstructionsSection.tsx.BaseCreateDetailsCard,BaseCreateIdempotentDetailsCard, andBaseRecoverNestedDetailsCardfor specific instruction types.AssociatedTokenDetailsCardinAssociatedTokenDetailsCard.spec.tsx.mock-stubs.ts,mocks.ts, andutils.ts..btn-black.activein_overrides-dark.scssto highlight active state.mocks.tsfrom__tests__/inspectorand replaces it with new mock files.This description was created by
for 132c528. It will automatically update as commits are pushed.