-
Notifications
You must be signed in to change notification settings - Fork 4
fix wallet transaction flow #42
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
Conversation
|
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 89d7159 in 1 minute and 23 seconds. Click for details.
- Reviewed
541lines of code in9files - Skipped
1files when reviewing. - Skipped posting
10draft 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. apps/docs/app/components/full-page-buynow.tsx:13
- Draft comment:
Defaulting to NEXT_PUBLIC_SOLANA_RPC_URL is fine—ensure the env variable is always set in each deployment. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/connector/src/lib/connector-client.ts:281
- Draft comment:
Removed debug logs for wallet account changes to reduce noise. Confirm that debug mode can still be enabled when needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/connector/src/ui/connector-provider.tsx:26
- Draft comment:
Debug logs on provider mount/unmount were removed; this cleanup is fine if logging isn’t needed in production. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/react/src/__tests__/components/transaction-states.test.tsx:190
- Draft comment:
Tests for the close button in TransactionSuccess were removed—ensure this matches the new UI (close button has been omitted). - Reason this comment was not posted:
Comment looked like it was already resolved.
5. packages/react/src/components/iframe/iframe-wallet-payment.tsx:364
- Draft comment:
Added debug logging for the walletConnect message. Verify that sensitive info is not leaked in production logs. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. packages/react/src/components/ui/secure-iframe-shell.tsx:178
- Draft comment:
The waitForSigner function uses polling every 50ms. Consider potential performance impact if the signer takes longer; an exponential backoff could be beneficial. - 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 is not pointing out a bug or a required change, but rather making a suggestion for a possible optimization. According to the rules, speculative comments or those that do not require a code change should be removed. There is no strong evidence that the current polling interval is problematic, nor is there a clear requirement to change it. The comment is not actionable in the sense required by the review rules. Perhaps the polling interval could be a real performance concern in some edge cases, and the reviewer is preemptively suggesting a best practice. However, without evidence of a problem, this is still speculative. Even if the suggestion is reasonable, the rules require strong evidence of a required code change, not just a possible optimization. Without evidence of a problem, the comment should be removed. The comment is speculative and does not require a code change. It should be deleted according to the review rules.
7. packages/react/src/components/ui/secure-iframe-shell.tsx:609
- Draft comment:
Default case in the onMessage switch logs unhandled message types. This helps with debugging—ensure no critical message types are missed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining the purpose of the default case in the switch statement. It does not provide a specific suggestion or ask for confirmation of an intentional change. It violates the rule against purely informative comments.
8. packages/react/src/index.tsx:205
- Draft comment:
Memoizing arcConfig with useMemo is good practice to prevent re-renders; this change appears solid. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. packages/sdk/src/core/web-client.ts:141
- Draft comment:
Replaced getSnapshot with getConnectorState for consistency with connector changes. Ensure all type contracts are maintained. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that all type contracts are maintained, which violates the rule against asking the author to ensure things. The first part of the comment is informative, explaining the change made, but the second part is not allowed.
10. packages/sdk/src/core/web-client.ts:392
- Draft comment:
The detectVersionedSupport function checks for transaction version support. Verify that this logic correctly handles all wallet feature variations. - 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.
Workflow ID: wflow_TCkSp6aTK07dTBe6
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 63b44a5 in 1 minute and 51 seconds. Click for details.
- Reviewed
13602lines of code in83files - Skipped
0files when reviewing. - Skipped posting
7draft 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. program/clients/rust/src/generated/types/mod.rs:1
- Draft comment:
This file is autogenerated (see header) and warns not to edit manually. Ensure changes are made via visitors so they aren’t overwritten. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. program/clients/rust/src/generated/types/mod.rs:8
- Draft comment:
Reordering of module declarations and re-export blocks alters indentation/ordering. Confirm this change aligns with project formatting and doesn’t affect module visibility. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. program/clients/rust/src/generated/programs.rs:12
- Draft comment:
There's a potential typo in the program ID literal. The value is "commkU28d52cwo2Ma3Marxz4Qr9REtfJtuUfqnDnbhT"; if this is meant to represent a commerce program, should it perhaps be "commerce..." instead of "commk..."? Please verify. - Reason this comment was not posted:
Comment was on unchanged code.
4. program/clients/typescript/src/generated/errors/commerceProgram.ts:2
- Draft comment:
Typographical change: The library name was updated from 'codama' to 'Codama'. Please ensure this is the intended capitalization. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure the capitalization change is intended. This falls under the rule of not asking the author to confirm their intention. The comment is not making a specific code suggestion or pointing out a potential issue with the code itself, but rather asking for confirmation of intent, which is not allowed.
5. program/clients/typescript/src/generated/errors/commerceProgram.ts:4
- Draft comment:
Typographical change: Here 'codama' was updated to 'Codama'. Confirm that this change is consistent with the intended naming convention. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%The comment is asking the author to confirm the consistency of a naming convention change, which is not allowed. However, it does point out a specific change that might be worth considering. The comment could be rephrased to suggest checking the naming convention without asking for confirmation.
6. program/clients/typescript/src/generated/instructions/clearPayment.ts:2
- Draft comment:
Typographical error: 'codama' has been corrected to 'Codama'. Please ensure that the capitalization remains consistent throughout references to the Codama library. - Reason this comment was not posted:
Comment looked like it was already resolved.
7. program/clients/typescript/src/generated/instructions/clearPayment.ts:4
- Draft comment:
Typographical error: 'codama' has been corrected to 'Codama'. Consistent capitalization helps maintain clarity in autogenerated comments. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, pointing out a typographical correction. It doesn't provide a suggestion or ask for a specific action related to the code's functionality or logic. It doesn't align with the rules for useful comments.
Workflow ID: wflow_S97lUes9oGFOUhqk
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
📊 Coverage Report📁 View Coverage ArtifactsCoverage reports have been generated for all test suites:
|
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.
Important
Looks good to me! 👍
Reviewed 6de7d43 in 1 minute and 21 seconds. Click for details.
- Reviewed
42lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft 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. packages/connector/src/__tests__/connector-client.test.ts:688
- Draft comment:
The new test only checks client instantiation with debug enabled, but it no longer verifies any debug logging behavior. If logging output is important, consider testing it explicitly. - 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 makes a valid point - the new tests are less thorough than the old ones. However, I need to consider: 1) Is debug logging behavior important enough to test? 2) Would adding these tests back provide meaningful value? 3) Is this comment actionable and clear enough to be useful? I could be undervaluing the importance of testing debug logging. If debug output is relied upon for development or troubleshooting, having tests for it could be valuable. While debug logging can be useful, testing it is often not critical since it's primarily a development aid. The tests would also be somewhat brittle since they mock console.log and make assumptions about what should be logged. The comment should be deleted. While it makes a valid observation, testing debug logging behavior is not critical enough to warrant a comment, and the suggested change would add complexity for limited benefit.
2. packages/connector/src/__tests__/connector-client.test.ts:693
- Draft comment:
The test for debug mode without config now only asserts that the client is defined. If the debug flag impacts behavior, verify its side effects as well. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment suggests verifying side effects of the debug flag, which is a request for confirmation of behavior. This violates the rule against asking the PR author to confirm their intention or ensure behavior is intended. The comment does not provide a specific code suggestion or ask for a specific test to be written, which would be acceptable.
Workflow ID: wflow_gwlcgG4wpArlJycl
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Improves wallet transaction flow by refining debug mode, transaction success UI, and wallet state management.
connector-client.test.ts.SecureIframeShellandArcWebClientto log debug information conditionally.TransactionSuccess.tsx.transaction-states.test.tsxto reflect UI changes.waitForSignerinsecure-iframe-shell.tsxto ensure signer availability.ArcWebClientto handle connector state changes more robustly.mod.rsfor consistency.This description was created by
for 6de7d43. You can customize this summary. It will automatically update as commits are pushed.