-
-
Notifications
You must be signed in to change notification settings - Fork 283
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
WIP: 14743 improve web app speed #17424
Conversation
🚀 Expo preview is ready!
|
WalkthroughThe pull request includes updates across deployment workflows and build configurations. Two GitHub Actions workflows have been modified to add a cache-control header ( ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/suite-build/configs/web.webpack.config.ts (2)
12-12
: Consider using ES imports for consistency.The codebase uses ES imports for other dependencies, but CommonJS
require
for the PreloadWebpackPlugin. For consistency, consider using ES import syntax if the plugin supports it.-const PreloadWebpackPlugin = require('@vue/preload-webpack-plugin'); +import PreloadWebpackPlugin from '@vue/preload-webpack-plugin';
79-85
: Consider standardizing property naming to avoid future deprecation issues.The plugin is using
fileWhitelist
andfileBlacklist
properties, but newer versions of similar plugins often use more inclusive terminology likeinclude
/exclude
orincludeFiles
/excludeFiles
.While the current implementation works, it's worth noting that future versions might deprecate these property names. Consider documenting this in a comment or planning for eventual updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (5)
.github/workflows/release-suite-desktop-web-staging.yml
(1 hunks).github/workflows/test-transport.yml
(1 hunks)packages/suite-build/configs/web.webpack.config.ts
(2 hunks)packages/suite-build/eslint.config.mjs
(1 hunks)packages/suite-build/package.json
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- 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-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (5)
.github/workflows/test-transport.yml (1)
108-111
: Enhanced Caching with Updated Cache-Control HeaderThe updated
aws s3 sync
command now includes the--cache-control "max-age=2592000"
option. This ensures that files are cached for 30 days, aligning well with the PR objective to improve loading speed. Please verify that this extended caching does not interfere with the immediate visibility of critical updates if deployments are frequent..github/workflows/release-suite-desktop-web-staging.yml (1)
133-137
: Consistent Cache-Control Implementation in S3 UploadThe modification to the aws s3 sync command by adding
--cache-control "max-age=2592000"
ensures that the suite-desktop artifacts are cached for 30 days, matching the caching strategy in other workflows. This consistency is beneficial for performance improvements. Consider if any cache invalidation mechanisms are needed in case of urgent updates.packages/suite-build/package.json (1)
28-28
:❓ Verification inconclusive
Dependency added correctly for webpack preloading feature.
The
@vue/preload-webpack-plugin
is a solid choice for implementing resource preloading, which should improve web app performance by prefetching critical JavaScript chunks.
🌐 Web query:
What is the latest version of @vue/preload-webpack-plugin?
Error during web search
Action Required: Verify Dependency Version for Preload Webpack Plugin
The dependency is added correctly in the file (
packages/suite-build/package.json
, line 28) with the version set to^2.0.0
. This version appears to be a solid choice for implementing resource preloading and should help improve web app performance by prefetching critical JavaScript chunks. However, the latest version information could not be retrieved due to a web query error. Please manually verify on the npm registry that^2.0.0
reflects the most up-to-date release of@vue/preload-webpack-plugin
.packages/suite-build/eslint.config.mjs (1)
17-17
: ESLint configuration updated appropriately.Adding the
@vue/preload-webpack-plugin
to the allowed dependencies list in the ESLint configuration ensures that imports of this package won't trigger linting errors.packages/suite-build/configs/web.webpack.config.ts (1)
79-85
:❓ Verification inconclusive
Preload configuration looks well-optimized.
The plugin configuration is well thought out:
- Using
rel: 'prefetch'
is appropriate for non-critical resources that will be needed for future navigation- Focusing on async chunks is efficient as they're loaded on demand
- The whitelist/blacklist patterns are sensible to target only JavaScript files while excluding less critical resources
This implementation should help improve the app's loading speed as intended.
To ensure the preloading strategy is effective, I recommend monitoring the performance impact after deployment using tools like Lighthouse or WebPageTest. You might want to verify which chunks are being prefetched with:
🏁 Script executed:
#!/bin/bash # Check the generated HTML to see which resources are being prefetched npm run build:web grep -r "prefetch" packages/suite-web/build/Length of output: 457
Manual Verification Required: Validate Prefetch Behavior
The preload configuration looks well-optimized. The settings for
rel: 'prefetch'
, limiting to async chunks, and using targeted whitelist/blacklist patterns to select only JavaScript files are appropriate, which should help improve the app's loading speed.However, the automated check using the suggested shell script did not yield any output because the
npm run build:web
script is missing and the expected build output directory (packages/suite-web/build/
) was not found. I recommend manually verifying that the intended async chunk files are being prefetched by:
- Running the correct build command for your web package.
- Inspecting the final generated HTML to confirm the presence of prefetch links.
- Monitoring performance post-deployment using tools like Lighthouse or WebPageTest.
@@ -133,7 +133,7 @@ jobs: | |||
- name: Upload suite-desktop to staging-data.trezor.io canary | |||
# uploads the files to the staging bucket canary folder (canary does not have staging percentage set) | |||
run: | | |||
aws s3 sync --delete ./trezor-suite-files s3://staging-data.trezor.io/suite/releases/desktop/canary | |||
aws s3 sync --delete ./trezor-suite-files s3://staging-data.trezor.io/suite/releases/desktop/canary --cache-control "max-age=2592000" |
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 guess these won't be the files....
It would be better to set cache control more granulary eg. for just particular js. files that include a specific string (are unique - generated with webpack with some proper content hash)
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.
@vdovhanych is the person who knows, right?
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.
@vdovhanych is the person who knows, right?
dacdbc2
to
eff6fbd
Compare
b722e42
to
2ba834a
Compare
2ba834a
to
31fa546
Compare
Description
Attempt to improve loading speed of the Suite app by:
PreloadWebpackPlugin
to preload chunks that are needed for appRelated Issue
Addresses: #14743 (comment)
Screenshots: