Connect popup tests 2 #17173
Connect popup tests 2 #17173
Conversation
mroz22
force-pushed
the
connect-popup-tests
branch
from
5794796 to
e4123c7
Compare
mroz22
added
the
no-project
mroz22
force-pushed
the
connect-popup-tests
branch
from
e4123c7 to
db202fa
Compare
|
🚀 Expo preview is ready!
|
mroz22
force-pushed
the
connect-popup-tests
branch
from
db202fa to
e6d915d
Compare
mroz22
force-pushed
the
connect-popup-tests
branch
from
e6d915d to
5a2def8
Compare
… its version with protobufjs" This reverts commit 3ab195c.
mroz22
force-pushed
the
connect-popup-tests
branch
from
5a2def8 to
361ef84
Compare
WalkthroughThe pull request introduces several changes across multiple packages. In the connect-popup package, the test functions have been updated to replace direct selector-based interactions with explicit calls using test IDs. This includes modifying element click methods and wait operations, as well as adjusting URL navigation logic to conditionally include or exclude the "/index.html" suffix based on a web extension flag. In the protobuf package, the direct dependency on the "long" package and associated configuration lines have been removed, streamlining the dependency management. Conversely, the transport package now adds the "long" dependency and integrates it with protobuf by configuring the utility to use the imported Long library. Additionally, the import statements for the Root entity in the transport package's utility files have been modified to import the value rather than only the type. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/connect-popup/e2e/tests/popup-close.test.ts (1)
219-224: Consider extracting repeated URL formatting logic.The conditional URL formatting with
/index.htmlsuffix is repeated multiple times across different test cases. Consider extracting this logic into a helper function to improve maintainability and reduce duplication.Create a new helper function in
helpers.ts:// In helpers.ts +export const formatMethodUrl = (baseUrl: string, method: string, isWebExtension: boolean) => { + return formatUrl(baseUrl, `${method}${isWebExtension ? '/index.html' : ''}`); +}; // In test file -await explorerPage.goto( - formatUrl( - explorerUrl, - `methods/bitcoin/getAddress` + (isWebExtension ? `/index.html` : ''), - ), -); +await explorerPage.goto(formatMethodUrl(explorerUrl, 'methods/bitcoin/getAddress', isWebExtension));Also applies to: 269-274, 349-354, 409-414, 456-456
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
packages/connect-popup/e2e/support/helpers.ts(2 hunks)packages/connect-popup/e2e/tests/browser-support.test.ts(1 hunks)packages/connect-popup/e2e/tests/popup-close.test.ts(5 hunks)packages/connect-popup/e2e/tests/popup-pages.test.ts(1 hunks)packages/protobuf/package.json(0 hunks)packages/protobuf/src/index.ts(0 hunks)packages/transport/package.json(1 hunks)packages/transport/src/index.ts(1 hunks)packages/transport/src/utils/receive.ts(1 hunks)packages/transport/src/utils/send.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/protobuf/src/index.ts
- packages/protobuf/package.json
✅ Files skipped from review due to trivial changes (1)
- packages/transport/src/utils/receive.ts
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Type Checking
- GitHub Check: Unit Tests
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (8)
packages/transport/src/utils/send.ts (1)
4-4: LGTM! The import change is necessary for runtime functionality.The change from type-only import to value import is correct. The
Rootobject needs to be available at runtime forcreateMessageFromNameto properly handle protobuf messages, which is crucial for handling large numbers (like those in EOS cryptocurrency transactions).packages/connect-popup/e2e/tests/browser-support.test.ts (1)
26-26: LGTM! Improved element selection method.The change to use
getByTestId()is a better practice as it:
- Provides better type safety
- Improves code readability
- Follows Playwright's recommended API usage
packages/connect-popup/e2e/tests/popup-pages.test.ts (1)
78-78: LGTM! Enhanced element waiting logic.The change to use
getByTestId().waitFor()improves the code by:
- Making the visibility requirement explicit
- Using a more idiomatic Playwright API
- Maintaining consistency with other test files
packages/connect-popup/e2e/support/helpers.ts (2)
119-120: LGTM! Consistent element selection improvements.The changes to use
getByTestId()align with the codebase-wide improvements in element selection methods.
152-154:❓ Verification inconclusive
Verify the URL routing behavior in web extension context.
The conditional URL formatting (
settings/index.htmlvssettings) addresses the Next.js routing issues mentioned in the PR objectives. However, ensure this change doesn't affect other scenarios.Run this script to check for any other affected routes:
🏁 Script executed:
#!/bin/bash # Search for other potential routes that might need similar handling rg -g '*.ts' -g '*.tsx' 'index\.html'Length of output: 3787
Ensure Web Extension Routing Remains Consistent
- The conditional URL formatting in
packages/connect-popup/e2e/support/helpers.ts(lines 152–154) follows an approach similar to other parts of the codebase (e.g., inpackages/connect-explorer/src-webextension/pages/extension-popup/ExtensionPopup.tsxand various end-to-end tests).- The search output confirms that appending
index.htmlfor web extension contexts is a standard pattern. Please verify that non-web extension usages (where onlysettingsis used) and other scenarios continue to function as expected.packages/transport/src/index.ts (2)
1-7: LGTM! Well-documented solution for handling large numbers.The addition of Long.js is well-documented with clear references to both the official documentation and a relevant GitHub issue that explains why explicit configuration is needed in browser environments.
11-12:❓ Verification inconclusive
Verify the fix for large number handling.
The configuration looks correct. Since this change is meant to fix issues with EOS cryptocurrency and other coins handling large numbers, we should verify the fix.
Run the following script to find and analyze tests related to large number handling:
🏁 Script executed:
#!/bin/bash # Description: Find tests related to large number handling, particularly for EOS cryptocurrency # Search for test files containing references to large numbers or EOS rg -l "MAX_SAFE_INTEGER|EOS|Long" --type ts --glob "*test*" # Search for protobuf message definitions containing int64/uint64 fields rg "int64|uint64" --type protoLength of output: 578
Large Number Handling for EOS Verification Needed
The configuration in
packages/transport/src/index.ts(lines 11-12) appears correct for setting up protobuf’s usage ofLong. The search for tests returned several files related to number handling (e.g. inpackages/protobuf/tests/encode-decode-basic.test.ts,packages/protocol/tests/protocol-trzd.test.ts, etc.). However, the command searching for.protofiles (to verify int64/uint64 definitions) resulted in an error since theprototype wasn’t recognized.Please take the following actions:
- Manually verify that tests involving EOS and large number handling demonstrate the intended behavior.
- Consider updating the search command using a custom file type (e.g., with
--type-add 'proto:*.proto') to verify protobuf definitions.packages/transport/package.json (1)
70-70:❓ Verification inconclusive
Verify the Long.js version and check for vulnerabilities.
The addition of the Long.js dependency looks good, but let's verify the version and security aspects.
🌐 Web query:
What is the latest stable version of the 'long' npm package and are there any known vulnerabilities?💡 Result:
The latest stable version of the
longnpm package is 5.3.1, published 5 days ago as of February 23, 2025[4][10]. This package provides a class for representing 64-bit integers in JavaScript and is actively maintained with recent updates.No specific vulnerabilities were found for
longin the provided sources[4][10]. However, general NPM security best practices apply:
Check nested dependencies
Vulnerabilities often arise from indirect dependencies[3][5]. Use:npm auditto scan your project’s dependency tree for risks[8].
Update strategy
For security-critical projects:
- Use
npm outdatedto identify outdated packages[6]- Force updates via
overridesinpackage.jsonif needed[3][5]:"overrides": { "long": "5.3.1" }The package has no deprecation warnings[9] and shows recent maintenance activity[4], indicating active support. For ongoing security monitoring, enable automated alerts through GitHub Dependabot or similar tools[5][6].
Citations:
- 1: https://www.npmjs.com/package/latest-version
- 2: https://blog.checkpoint.com/securing-the-cloud/review-of-recent-npm-based-vulnerabilities/
- 3: https://www.niraj.life/blog/understanding-npm-audit-fixing-vulnerabilities-nodejs/
- 4: https://www.npmjs.com/package/long
- 5: https://dev.to/petrussola/steps-to-fix-package-security-vulnerabilities-in-your-project-2ihk
- 6: https://twm.me/how-to-keep-npm-packages-up-to-date/
- 7: https://docs.npmjs.com/try-the-latest-stable-version-of-npm/
- 8: https://docs.npmjs.com/auditing-package-dependencies-for-security-vulnerabilities/
- 9: https://www.aquasec.com/blog/deceptive-deprecation-the-truth-about-npm-deprecated-packages/
- 10: https://www.npmjs.com/package/npm
Verify and Consider Updating Long.js Dependency Version
The current dependency
"long": "^5.2.0"is acceptable as it allows updates up to but not including 6.0.0. However, the latest stable version of Long.js is now 5.3.1, and no vulnerabilities have been reported. Please verify that updating (or allowing an update) to 5.3.1 remains compatible with protobufjs 7.4.0 and your overall project requirements. Also, consider runningnpm auditperiodically to catch any issues from nested dependencies.
commit 2 adds simple CI workflow for that runs for changes in components. It could be nicer, there is some duplication but we are abandoning popup anyway so this is sort of temporaryI am moving it to a separate PR