Vite as suite devserver #17152
Vite as suite devserver #17152
Conversation
77c9471 to
db851bd
Compare
ab227b5 to
91c45c5
Compare
|
🚀 Expo preview is ready!
|
22d9424 to
2d7ff96
Compare
22a7790 to
bc6dd74
Compare
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
ea4d44f to
0708451
Compare
I reestarted the emulator and it works fine. Wat |
yeah real device isn't doing anything |
got it ! There was wrongly |
9797117 to
fbfae05
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/src/components/Flag/Flag.tsx (3)
23-32: Consider caching loaded flag imagesFor performance optimization, you might want to cache loaded flag images to avoid reloading them if the component is unmounted and remounted with the same country.
- const [src, setSrc] = useState(''); + const [src, setSrc] = useState(''); + const [isLoading, setIsLoading] = useState(true); + + // Simple in-memory cache for loaded flags + const flagCache = useMemo(() => new Map<string, string>(), []); useEffect(() => { + // Check if we have this flag cached + if (flagCache.has(country)) { + setSrc(flagCache.get(country)!); + setIsLoading(false); + return; + } + + setIsLoading(true); import(`../../images/flags/${country.toLowerCase()}.svg`) .then(module => { setSrc(module.default); + flagCache.set(country, module.default); + setIsLoading(false); }) .catch(err => { // NOTE: keep error here as this is not a critical issue console.error('Flag image loading error: ', err); + setIsLoading(false); }); }, [country, flagCache]);
28-31: Consider improving error handlingWhile the current error handling is satisfactory, consider showing a fallback image or indicator when a flag fails to load.
.catch(err => { // NOTE: keep error here as this is not a critical issue console.error('Flag image loading error: ', err); + // Optionally set a fallback image or error state + // setSrc('/path/to/fallback/image.svg'); });
36-40: Suggest adding aria attributes for accessibilityFor better accessibility, consider adding appropriate aria attributes to the image and skeleton.
{src ? ( - <img src={src} width={`${size}px`} alt={`flag-${country}`} className={className} /> + <img + src={src} + width={`${size}px`} + alt={`flag-${country}`} + className={className} + aria-label={`Flag of ${country}`} + /> ) : ( - <SkeletonRectangle width={size} height={size} /> + <SkeletonRectangle + width={size} + height={size} + aria-label="Loading flag" + role="img" + /> )}
📜 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 (19)
README.md(1 hunks)package.json(1 hunks)packages/address-validator/src/crypto/cnBase58.js(1 hunks)packages/components/src/components/Flag/Flag.tsx(2 hunks)packages/connect-web/src/module/index.ts(1 hunks)packages/connect/src/core/method.ts(1 hunks)packages/suite-build/configs/web.webpack.config.ts(1 hunks)packages/suite-build/package.json(1 hunks)packages/suite-web/eslint.config.mjs(1 hunks)packages/suite-web/package.json(3 hunks)packages/suite-web/src/static/index.html(0 hunks)packages/suite-web/src/static/vite-index.ts(1 hunks)packages/suite-web/vite.config.ts(1 hunks)packages/suite/src/hooks/guide/useGuideLoadArticle.ts(1 hunks)packages/suite/src/hooks/suite/useLocales.ts(1 hunks)packages/suite/src/reducers/suite/guideReducer.ts(1 hunks)packages/suite/src/support/workers/graph/index.ts(1 hunks)packages/suite/src/views/dashboard/PortfolioCard/DashboardGraph.tsx(2 hunks)scripts/list-outdated-dependencies/engagement-dependencies.txt(1 hunks)
💤 Files with no reviewable changes (1)
- packages/suite-web/src/static/index.html
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/suite-build/package.json
- scripts/list-outdated-dependencies/engagement-dependencies.txt
- package.json
- packages/suite-build/configs/web.webpack.config.ts
- packages/suite-web/src/static/vite-index.ts
- packages/suite/src/support/workers/graph/index.ts
- packages/connect-web/src/module/index.ts
- packages/suite/src/hooks/guide/useGuideLoadArticle.ts
- packages/suite/src/hooks/suite/useLocales.ts
- packages/suite-web/vite.config.ts
- README.md
- packages/suite-web/eslint.config.mjs
- packages/suite/src/views/dashboard/PortfolioCard/DashboardGraph.tsx
- packages/suite/src/reducers/suite/guideReducer.ts
- packages/address-validator/src/crypto/cnBase58.js
- packages/connect/src/core/method.ts
⏰ Context from checks skipped due to timeout of 90000ms (13)
- 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 / node-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: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (5)
packages/suite-web/package.json (3)
10-10: Approve Vite development script additionThe script correctly clears the build directory and launches Vite with the appropriate project context.
32-32: Approve Vite-related dependenciesThe added dependencies support the new Vite development workflow:
@originjs/vite-plugin-commonjs: Ensures compatibility with CommonJS modules@vitejs/plugin-react: Provides React support in Vitevite: Core Vite packagevite-plugin-wasm: Enables WebAssembly support in ViteThese are appropriate choices for integrating Vite into a React project.
Also applies to: 48-48, 53-54
10-10:✅ Verification successful
Verify Vite configuration location
From the PR objectives, I see there were plans to move the Vite configuration to the
suite-buildpackage, but this proved challenging. Consider documenting this decision and creating a follow-up task to revisit this if necessary.
🏁 Script executed:
#!/bin/bash # Check if there's a vite.config.js/ts file in this package fd -e js -e ts -g "vite.config.*" ./packages/suite-web/ -x echo "Found Vite config: {}" # Check if there's any Vite configuration in suite-build package fd -e js -e ts -g "*vite*" ./packages/suite-build/ -x echo "Found Vite-related file in suite-build: {}"Length of output: 250
Action: Document the Vite Configuration Decision
We verified that the Vite configuration remains in the
packages/suite-webpackage as found invite.config.ts, and no Vite-related files exist in thesuite-buildpackage. Since the original PR objective was to migrate the configuration but encountered challenges, please document the rationale behind keeping the config in place and consider creating a follow-up task to reassess this migration in the future.packages/components/src/components/Flag/Flag.tsx (2)
1-2: Approve dependencies for async loadingThe addition of React hooks and SkeletonRectangle component supports the async loading pattern.
Also applies to: 6-6
21-43: Excellent refactoring to support asynchronous image loadingThe Flag component has been well-refactored to:
- Load SVG images asynchronously using dynamic imports
- Provide a loading state with a skeleton placeholder
- Handle potential errors gracefully
- Follow React best practices with proper hooks usage
This change likely improves compatibility with Vite, which prefers ESM imports over CommonJS requires. The skeleton placeholder also enhances user experience.
fbfae05 to
7c84862
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/src/components/Flag/Flag.tsx (3)
21-32: Well-implemented asynchronous image loading with proper error handling.The refactoring from synchronous to asynchronous image loading is a significant improvement. The implementation:
- Uses React hooks correctly with proper dependency array
- Handles errors gracefully without breaking the UI
- Follows the pattern needed for Vite's module system
One small improvement could be to add a timeout to prevent potential infinite loading states in edge cases.
useEffect(() => { + const timeoutId = setTimeout(() => { + if (!src) { + console.warn(`Flag image for ${country} is taking too long to load`); + } + }, 5000); import(`../../images/flags/${country.toLowerCase()}.svg`) .then(module => { setSrc(module.default); }) .catch(err => { // NOTE: keep error here as this is not a critical issue console.error('Flag image loading error: ', err); }); + + return () => clearTimeout(timeoutId); }, [country]);
36-40: Consider adding a fallback for failed image loads.While there's error logging for failed image loads, the UI only handles the loading state. Adding a fallback image or icon for when loading fails would improve the user experience.
return ( <Wrapper> {src ? ( <img src={src} width={`${size}px`} alt={`flag-${country}`} className={className} /> ) : ( <SkeletonRectangle width={size} height={size} /> )} + {/* Optional: Add this for better error handling */} + {src === null && ( + <div style={{ width: size, height: size, display: 'flex', justifyContent: 'center', alignItems: 'center', backgroundColor: '#f5f5f5', color: '#666' }}> + ? + </div> + )} </Wrapper> );To implement this, you would need to modify the state initialization and error handling:
- const [src, setSrc] = useState(''); + const [src, setSrc] = useState<string | null>(''); // In the catch block .catch(err => { console.error('Flag image loading error: ', err); + setSrc(null); // Indicate load failure });
37-37: Consider enhancing image accessibility.While the implementation works well functionally, consider adding more accessibility attributes to the image element to ensure better screen reader support.
- <img src={src} width={`${size}px`} alt={`flag-${country}`} className={className} /> + <img + src={src} + width={`${size}px`} + height={`${size}px`} + alt={`Flag of ${country}`} + className={className} + loading="lazy" + role="img" + />
📜 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 (19)
README.md(1 hunks)package.json(1 hunks)packages/address-validator/src/crypto/cnBase58.js(1 hunks)packages/components/src/components/Flag/Flag.tsx(2 hunks)packages/connect-web/src/module/index.ts(1 hunks)packages/connect/src/core/method.ts(1 hunks)packages/suite-build/configs/web.webpack.config.ts(1 hunks)packages/suite-build/package.json(1 hunks)packages/suite-web/eslint.config.mjs(1 hunks)packages/suite-web/package.json(3 hunks)packages/suite-web/src/static/index.html(0 hunks)packages/suite-web/src/static/vite-index.ts(1 hunks)packages/suite-web/vite.config.ts(1 hunks)packages/suite/src/hooks/guide/useGuideLoadArticle.ts(1 hunks)packages/suite/src/hooks/suite/useLocales.ts(1 hunks)packages/suite/src/reducers/suite/guideReducer.ts(1 hunks)packages/suite/src/support/workers/graph/index.ts(1 hunks)packages/suite/src/views/dashboard/PortfolioCard/DashboardGraph.tsx(2 hunks)scripts/list-outdated-dependencies/engagement-dependencies.txt(1 hunks)
💤 Files with no reviewable changes (1)
- packages/suite-web/src/static/index.html
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/suite-build/package.json
- packages/suite/src/hooks/guide/useGuideLoadArticle.ts
- packages/suite-build/configs/web.webpack.config.ts
- packages/suite-web/src/static/vite-index.ts
- packages/connect/src/core/method.ts
- packages/suite/src/hooks/suite/useLocales.ts
- scripts/list-outdated-dependencies/engagement-dependencies.txt
- packages/suite/src/support/workers/graph/index.ts
- package.json
- packages/suite-web/eslint.config.mjs
- packages/connect-web/src/module/index.ts
- packages/suite/src/reducers/suite/guideReducer.ts
- packages/suite/src/views/dashboard/PortfolioCard/DashboardGraph.tsx
- packages/suite-web/package.json
- packages/address-validator/src/crypto/cnBase58.js
- README.md
- packages/suite-web/vite.config.ts
⏰ Context from checks skipped due to timeout of 90000ms (12)
- 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: PR-check / node-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (2)
packages/components/src/components/Flag/Flag.tsx (2)
1-6: Good import organization with proper separation of concerns.The addition of React hooks (
useState,useEffect) and theSkeletonRectanglecomponent for loading states shows good planning for asynchronous operations.
34-42: Nice implementation of conditional rendering with fallback loading state.The conditional rendering pattern is clean and provides a good user experience with the skeleton loading state. This is a good approach for handling asynchronous content.
0940517 to
08d1821
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/suite-web/package.json (1)
10-10: Per past comments, some configuration should be moved to@trezor/suite-build.According to past review comments, the Vite configuration should ideally be in the
@trezor/suite-buildpackage rather than directly insuite-web.Consider moving the Vite configuration to the
@trezor/suite-buildpackage to maintain consistency with the existing Webpack configuration approach. This would keep all build tooling centralized.You could modify the
dev:vitescript to call a command fromsuite-build:-"dev:vite": "PROJECT=web yarn rimraf ./build && vite", +"dev:vite": "yarn rimraf ./build && yarn workspace @trezor/suite-build run dev:vite:web",Then add the corresponding script and dependencies to the
suite-buildpackage.Also applies to: 32-32, 48-48, 53-54
packages/address-validator/tests/wallet_address_get_currencies.test.js (1)
1-6: Consider refactoring conditional imports for consistency with Vite.The hardcoded
isNode = truemakes the conditional imports on lines 3 and 6 redundant. Since this PR is integrating Vite as the development server, consider refactoring to use a more modern import approach that's compatible with both Vite and Jest.-var isNode = true; - -var chai = isNode ? require('chai') : window.chai, - expect = chai.expect; - -var WAValidator = isNode ? require('../src') : window.WAValidator; +// ESM style imports work with Vite and can work with Jest with proper configuration +const chai = require('chai'); +const expect = chai.expect; + +const WAValidator = require('../src');Alternatively, if you need to maintain browser compatibility but want to modernize:
-var isNode = true; - -var chai = isNode ? require('chai') : window.chai, - expect = chai.expect; - -var WAValidator = isNode ? require('../src') : window.WAValidator; +// Using process.env check which works with both Node and bundlers +const isNode = typeof process !== 'undefined' && process.versions && process.versions.node; + +const chai = isNode ? require('chai') : window.chai; +const expect = chai.expect; + +const WAValidator = isNode ? require('../src') : window.WAValidator;
📜 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 (19)
package.json(1 hunks)packages/address-validator/jest.config.js(1 hunks)packages/address-validator/package.json(1 hunks)packages/address-validator/tests/wallet_address_get_currencies.test.js(1 hunks)packages/components/src/components/Flag/Flag.tsx(2 hunks)packages/connect-explorer-theme/package.json(1 hunks)packages/connect-explorer-theme/vite.config.ts(0 hunks)packages/suite-build/configs/web.webpack.config.ts(1 hunks)packages/suite-build/package.json(1 hunks)packages/suite-web/eslint.config.mjs(1 hunks)packages/suite-web/package.json(3 hunks)packages/suite-web/src/static/index.html(0 hunks)packages/suite-web/src/static/vite-index.ts(1 hunks)packages/suite-web/vite.config.ts(1 hunks)scripts/list-outdated-dependencies/engagement-dependencies.txt(1 hunks)suite-common/trading/src/thunks/buy/handleRequestThunk.ts(1 hunks)suite-common/trading/src/thunks/buy/index.ts(0 hunks)suite-common/trading/src/thunks/buy/loadInfoThunk.ts(1 hunks)suite-common/trading/src/thunks/buy/selectQuoteThunk.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/suite-web/src/static/index.html
- suite-common/trading/src/thunks/buy/index.ts
- packages/connect-explorer-theme/vite.config.ts
✅ Files skipped from review due to trivial changes (1)
- packages/address-validator/package.json
🚧 Files skipped from review as they are similar to previous changes (11)
- scripts/list-outdated-dependencies/engagement-dependencies.txt
- packages/suite-build/configs/web.webpack.config.ts
- package.json
- packages/suite-web/src/static/vite-index.ts
- packages/suite-web/eslint.config.mjs
- suite-common/trading/src/thunks/buy/handleRequestThunk.ts
- packages/connect-explorer-theme/package.json
- suite-common/trading/src/thunks/buy/selectQuoteThunk.ts
- packages/suite-web/vite.config.ts
- packages/suite-build/package.json
- suite-common/trading/src/thunks/buy/loadInfoThunk.ts
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- 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: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (10)
packages/suite-web/package.json (2)
32-32:✅ Verification successful
Added Vite and necessary plugins as development dependencies.
These new dependencies enable Vite integration for the development workflow:
@originjs/vite-plugin-commonjs: Handles CommonJS modules in Vite@vitejs/plugin-react: Provides React support for Vitevite: The Vite build tool itselfvite-plugin-wasm: Enables WebAssembly support in ViteThe selection of plugins is appropriate for a React application that may use CommonJS modules and WebAssembly.
Let's confirm that a Vite configuration file exists that uses these dependencies:
Also applies to: 48-48, 53-54
🏁 Script executed:
#!/bin/bash # Check for Vite configuration file and verify plugin usage echo "Looking for Vite configuration file:" find packages -name "vite.config.*" | xargs ls -la echo -e "\nVerifying plugin usage in Vite config:" find packages -name "vite.config.*" | xargs grep -l "@originjs/vite-plugin-commonjs\|@vitejs/plugin-react\|vite-plugin-wasm" | xargs catLength of output: 8457
Vite configuration verified successfully.
- The Vite config file at
packages/suite-web/vite.config.tswas found.- It properly uses
@originjs/vite-plugin-commonjs,@vitejs/plugin-react, andvite-plugin-wasmas required.The added dependencies in
package.json(lines 32, 48, and 53-54) are confirmed to integrate correctly into the Vite development setup. No further action is needed.
10-10:✅ Verification successful
New Vite development script added as an alternative to Webpack.
This script provides a Vite-based development environment for the web application, which aligns with the PR objective of integrating Vite as a development server for Trezor Suite.
I notice from the past comments that this is meant to be an alternative development server, with Webpack remaining the primary one. Let's confirm that the setup allows both to coexist:
🏁 Script executed:
#!/bin/bash # Check if both development scripts are properly configured echo "Checking Webpack dev script:" grep -A 1 "\"dev\":" packages/suite-web/package.json echo -e "\nChecking Vite dev script:" grep -A 1 "\"dev:vite\":" packages/suite-web/package.jsonLength of output: 504
Both dev scripts properly coexist
The grep output confirms that the primary Webpack development script ("dev") and the alternative Vite development script ("dev:vite") are both present and correctly configured in
packages/suite-web/package.json. This aligns with the PR objective of integrating Vite as an alternative development environment without replacing Webpack.packages/components/src/components/Flag/Flag.tsx (4)
1-2: Added imports to support new asynchronous image loading pattern.Added imports necessary for the refactored component:
- React hooks for managing state and side effects
- SkeletonRectangle component for the loading state
This supports Vite's handling of assets which differs from Webpack.
Also applies to: 6-6
21-43: Excellent refactoring of the Flag component to support asynchronous image loading.The Flag component has been completely refactored to:
- Load flag images asynchronously using dynamic imports
- Handle loading states with a skeleton placeholder
- Manage errors gracefully
- Re-fetch when the country prop changes
This is a robust solution that works with both Vite and Webpack bundlers, as Vite handles static imports differently from Webpack.
The implementation shows good practices:
- Using React hooks appropriately
- Providing visual feedback during loading
- Proper error handling that doesn't crash the component
- Clean conditional rendering
As noted in a previous review, this is indeed a nice solution.
36-40: Improved UX with conditional rendering and loading state.The conditional rendering provides a better user experience by showing a skeleton loading state while the image is being fetched. Once loaded, it displays the actual flag image.
24-32:✅ Verification successful
Dynamic import with error handling for flag images.
Using dynamic imports with the
import()syntax is the recommended approach for Vite when loading assets that need to be processed. The error handling is also well implemented.Let's verify that the flag images are in the correct location and format:
🏁 Script executed:
#!/bin/bash # Check if flag SVG files exist in the expected location echo "Checking flag SVG files existence:" ls -la packages/components/src/images/flags/*.svg | head -n 5 echo -e "\nChecking total number of flag SVG files:" ls -la packages/components/src/images/flags/*.svg | wc -lLength of output: 23452
Approved: Flag Dynamic Import & Asset Verification
- The dynamic import for flag images is implemented following Vite's recommended approach.
- The error handling is correctly managed, ensuring non-critical issues are logged.
- Verification confirms that the expected SVG files exist in
packages/components/src/images/flags(258 flag files detected).packages/address-validator/jest.config.js (3)
1-9: Well-structured Jest configuration file.This configuration properly extends the base Jest config while adding a specific
transformIgnorePatternssetting to allow transformation of the chai module in node_modules. This approach is clean and ensures proper test execution with the chai dependency.
5-8: Good handling of potentially undefined transformIgnorePatterns.The use of the nullish coalescing operator (
??) to handle the case wherebaseConfig.transformIgnorePatternsmight be undefined is a good defensive programming practice.
7-7:✅ Verification successful
Verify if this exclusion pattern for chai is still necessary.
The pattern
/node_modules/(?!chai/)excludes the chai package from transform ignore patterns. This is likely related to the hardcodedisNodevalue in the test file, to ensure chai can be properly processed in the Jest environment.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any specific chai transformations or issues that require this exclusion # Look for any chai-related imports in the tests echo "Checking for chai usage in tests:" find packages/address-validator/tests -type f -name "*.js" -exec grep -l "chai" {} \; # Check if there are any issues with chai in the test logs echo "Looking for chai-related errors in recent test runs:" git log -n 5 --grep="chai" --onelineLength of output: 417
Chai exclusion pattern appears to be necessary
Our investigation confirms that chai is still in active use by tests (specifically found in
packages/address-validator/tests/wallet_address_validator.test.jsandpackages/address-validator/tests/wallet_address_get_currencies.test.js). Since the exclusion pattern ('/node_modules/(?!chai/)') allows chai to be processed appropriately in the Jest environment and no recent errors have been observed related to its transformation, it seems prudent to retain this configuration.packages/address-validator/tests/wallet_address_get_currencies.test.js (1)
1-1:❓ Verification inconclusive
Hardcoded environment check may limit portability.
Changing from dynamic environment detection (
typeof module !== 'undefined' && typeof module.exports !== 'undefined') to a hardcodedisNode = truemeans this code will always use Node.js-style imports, which could potentially break browser compatibility.
🏁 Script executed:
#!/bin/bash # Description: Check if this file needs to run in both Node and browser environments # Check if there are any browser-specific tests for this module echo "Checking for browser test configurations:" find packages/address-validator -type f -name "*.html" -o -name "*.browser.js" -o -name "karma.conf.js" # Check if the package is explicitly marked as browser-compatible in package.json echo "Checking package.json for browser field:" cat packages/address-validator/package.json | grep -A 10 "\"browser\"" # Look for browser usage patterns in the codebase echo "Looking for references to browser usage in documentation or code:" grep -r --include="*.md" --include="README*" "browser" packages/address-validator/Length of output: 582
Update on Hardcoded Environment Check
The change to hardcodeisNode = trueforces Node.js-specific behavior. Automated checks (i.e., searching for browser test configurations, thebrowserfield in package.json, and browser usage documentation) did not return any indicators of browser support for this package. However, since the inference is based solely on these automated checks—and because no browser-specific tests or configurations were found—it’s advisable to manually verify that this module is indeed intended for Node.js only. If there's any potential for browser usage in the future, consider reverting to dynamic detection usingtypeof module !== 'undefined' && typeof module.exports !== 'undefined'.
- Confirm that
packages/address-validatoris exclusively used in Node environments.- Verify that no browser-specific tests or usage patterns have been overlooked.
…ontains 'react' structure
…" without template variables in the paths
08d1821 to
7c75939
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/src/components/Flag/Flag.tsx (2)
1-1: Well-implemented asynchronous image loading with React hooks!The refactoring to use React hooks for dynamic loading is a good improvement. This approach:
- Reduces initial load time by deferring image loading
- Provides better user experience with loading states
- Follows modern React patterns
Consider extracting this image loading logic into a custom hook (e.g.,
useAsyncImage) if you plan to use similar patterns elsewhere in the codebase. This would promote code reusability across components.Also applies to: 23-32
28-31: Consider adding a fallback image for error cases.While logging errors is good, users won't see console messages. Consider showing a fallback/placeholder image when loading fails for a better user experience.
export const Flag = ({ size = 24, country, className }: FlagProps) => { const [src, setSrc] = useState(''); + const [hasError, setHasError] = useState(false); useEffect(() => { + setHasError(false); import(`../../images/flags/${country.toLowerCase()}.svg`) .then(module => { setSrc(module.default); }) .catch(err => { // NOTE: keep error here as this is not a critical issue console.error('Flag image loading error: ', err); + setHasError(true); }); }, [country]); return ( <Wrapper> - {src ? ( + {src && !hasError ? ( <img src={src} width={`${size}px`} alt={`flag-${country}`} className={className} /> + ) : hasError ? ( + <div style={{ width: `${size}px`, height: `${size}px`, backgroundColor: '#eee' }} /> ) : ( <SkeletonRectangle width={size} height={size} /> )} </Wrapper> ); };
📜 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 (29)
README.md(1 hunks)package.json(1 hunks)packages/address-validator/jest.config.js(1 hunks)packages/address-validator/package.json(1 hunks)packages/address-validator/src/crypto/cnBase58.js(1 hunks)packages/address-validator/tests/wallet_address_get_currencies.test.js(1 hunks)packages/components/src/components/Flag/Flag.tsx(2 hunks)packages/connect-explorer-theme/package.json(1 hunks)packages/connect-explorer-theme/vite.config.ts(0 hunks)packages/connect-web/src/module/index.ts(1 hunks)packages/connect/src/core/method.ts(1 hunks)packages/suite-build/configs/web.webpack.config.ts(1 hunks)packages/suite-build/package.json(1 hunks)packages/suite-web/eslint.config.mjs(1 hunks)packages/suite-web/package.json(3 hunks)packages/suite-web/src/static/index.html(0 hunks)packages/suite-web/src/static/vite-index.ts(1 hunks)packages/suite-web/vite.config.ts(1 hunks)packages/suite/src/hooks/guide/useGuideLoadArticle.ts(1 hunks)packages/suite/src/hooks/suite/useLocales.ts(1 hunks)packages/suite/src/reducers/suite/guideReducer.ts(1 hunks)packages/suite/src/support/workers/graph/index.ts(1 hunks)packages/suite/src/views/dashboard/PortfolioCard/DashboardGraph.tsx(2 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(0 hunks)scripts/list-outdated-dependencies/engagement-dependencies.txt(1 hunks)suite-common/trading/src/thunks/buy/handleRequestThunk.ts(1 hunks)suite-common/trading/src/thunks/buy/index.ts(0 hunks)suite-common/trading/src/thunks/buy/loadInfoThunk.ts(1 hunks)suite-common/trading/src/thunks/buy/selectQuoteThunk.ts(1 hunks)
💤 Files with no reviewable changes (4)
- scripts/list-outdated-dependencies/connect-dependencies.txt
- suite-common/trading/src/thunks/buy/index.ts
- packages/suite-web/src/static/index.html
- packages/connect-explorer-theme/vite.config.ts
🚧 Files skipped from review as they are similar to previous changes (24)
- package.json
- scripts/list-outdated-dependencies/engagement-dependencies.txt
- packages/suite-build/configs/web.webpack.config.ts
- packages/connect/src/core/method.ts
- packages/suite/src/hooks/guide/useGuideLoadArticle.ts
- README.md
- packages/suite-build/package.json
- packages/address-validator/package.json
- packages/address-validator/src/crypto/cnBase58.js
- packages/address-validator/jest.config.js
- suite-common/trading/src/thunks/buy/selectQuoteThunk.ts
- suite-common/trading/src/thunks/buy/loadInfoThunk.ts
- packages/suite/src/hooks/suite/useLocales.ts
- suite-common/trading/src/thunks/buy/handleRequestThunk.ts
- packages/suite/src/support/workers/graph/index.ts
- packages/suite-web/src/static/vite-index.ts
- packages/connect-web/src/module/index.ts
- packages/suite-web/eslint.config.mjs
- packages/address-validator/tests/wallet_address_get_currencies.test.js
- packages/suite/src/reducers/suite/guideReducer.ts
- packages/suite-web/vite.config.ts
- packages/connect-explorer-theme/package.json
- packages/suite-web/package.json
- packages/suite/src/views/dashboard/PortfolioCard/DashboardGraph.tsx
⏰ Context from checks skipped due to timeout of 90000ms (14)
- 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 / node-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: Setup and Cache Dependencies
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (3)
packages/components/src/components/Flag/Flag.tsx (3)
6-6: Good addition of the skeleton component for loading states.Adding the
SkeletonRectangleimport supports the new loading state implementation, providing visual feedback to users during image loading.
36-40: Clean conditional rendering implementation.The conditional rendering between the loaded image and the skeleton is well-implemented. The component correctly shows a loading state until the image is available.
As Lemonexe commented previously: "nice solution 🎉"
24-24:✅ Verification successful
Ensure Vite compatibility with dynamic imports.
The dynamic import path follows the correct pattern for importing SVG files, which works well with the Vite bundler mentioned in the PR objectives.
🏁 Script executed:
#!/bin/bash # Check if the SVG files exist in the expected location echo "Checking for flag SVG files..." fd -e svg . packages/components/src/images/flagsLength of output: 11501
Dynamic Import Verified for Vite Compatibility
The dynamic import statement in
packages/components/src/components/Flag/Flag.tsxcorrectly constructs the path for flag SVG files, and our filesystem check confirms that the expected SVGs (e.g.,ad.svg,ae.svg, etc.) are present inpackages/components/src/images/flags. No changes are necessary.
7c75939 to
2d6d313
Compare
TODO
suite-buildTO CHECK !!!!
For each webpack build, dev server, vite devserer, check this:
Description
Related Issue
Resolve
Screenshots: