Skip to content

Load PDF worker from blob source#14561

Merged
stitesExpensify merged 1 commit intomainfrom
marcaaron-loadWorkerScriptFromBlob
Jan 26, 2023
Merged

Load PDF worker from blob source#14561
stitesExpensify merged 1 commit intomainfrom
marcaaron-loadWorkerScriptFromBlob

Conversation

@marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Jan 25, 2023

This is a second attempt for #13546

We ran into some CSP issues with that (now resolved) so this should be working now on staging and production.

Details

When someone goes offline before opening the attachment modal they are unable to load the web workers script via HTTPS. This fix allows loading the worker source as a string in the bundle so it can be used later even if we are offline.

Fixed Issues

$ #12512

Tests

Same as QA.

Note: Use a VALID PDF i.e. something that normally works fine while we are online.

I would recommend sending yourself a valid PDF to an iOS and Android device + hosting the local web version via NGROK then test selecting the attachment with the internet disabled (i.e. Wi-Fi disabled).

  • Verify that no errors appear in the JS console

Offline tests

This is related to offline behavior so QA is the same.

QA Steps

  1. Open any chat in the new dot
  2. Go offline
  3. Click ➕ in composer -> Add Attachments
  4. Select a VALID PDF attachment
  5. Verify that a preview shows and there is no "Failed to load PDF file" text
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

2022-12-12_14-45-27

Mobile Web - Chrome

Works. But looks messed up atm and is missing the submit button.

Screenshot_20221212-151035_Chrome

Mobile Web - Safari

image_123986672 (1)

Desktop

2022-12-12_15-07-48

iOS

N/A as this platform is not touched in this PR

Android

N/A as this platform is not touched in this PR

@melvin-bot
Copy link

melvin-bot bot commented Jan 25, 2023

Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work?

@melvin-bot
Copy link

melvin-bot bot commented Jan 25, 2023

@aimane-chnaif @stitesExpensify One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@aimane-chnaif
Copy link
Contributor

@marcaaron I think we can just merge this PR since it was already tested locally in old PR. This should be tested after deploying to staging/production.
If this still requires checklist, I will fill them.

@marcaaron
Copy link
Contributor Author

Thanks @aimane-chnaif, let's go ahead and do the checklist on this one. I think, better to be safe than sorry.

@marcaaron marcaaron self-assigned this Jan 25, 2023
@aimane-chnaif
Copy link
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web web
Mobile Web - Chrome
mchrome.mp4
Mobile Web - Safari
msafari.mp4
Desktop desktop
iOS
ios.mp4
Android
android.mp4

Copy link
Contributor

@stitesExpensify stitesExpensify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stitesExpensify stitesExpensify merged commit 673833c into main Jan 26, 2023
@stitesExpensify stitesExpensify deleted the marcaaron-loadWorkerScriptFromBlob branch January 26, 2023 15:57
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 688.921 ms → 697.956 ms (+9.035 ms, +1.3%)
Open Search Page TTI 601.728 ms → 603.417 ms (+1.689 ms, ±0.0%)
App start runJsBundle 191.750 ms → 193.188 ms (+1.438 ms, +0.7%)
App start regularAppStart 0.014 ms → 0.017 ms (+0.002 ms, +14.9%)
App start nativeLaunch 19.966 ms → 19.793 ms (-0.172 ms, -0.9%)
Show details
Name Duration
App start TTI Baseline
Mean: 688.921 ms
Stdev: 26.215 ms (3.8%)
Runs: 648.3579259999096 649.9119380004704 651.1277859993279 658.6054859999567 658.953730000183 666.1523969992995 667.2957259994 667.5526100005955 669.6661029998213 674.1578509993851 676.2388499993831 676.402698000893 677.9386439993978 678.1388419996947 681.5208540000021 682.7542100008577 686.784530999139 689.6935719996691 691.1531509999186 691.5267169997096 695.6124489996582 699.4228949993849 701.2428660001606 703.6615169998258 707.5844429992139 708.2147589996457 712.9234059993178 713.4621190000325 734.1974439993501 739.1202600002289 739.2190589997917 746.8920310009271

Current
Mean: 697.956 ms
Stdev: 25.830 ms (3.7%)
Runs: 643.6525839995593 654.8121140003204 660.6034399997443 664.4086830001324 672.5649430006742 678.9730629995465 680.242336999625 680.3051130007952 681.7048920001835 683.0219589993358 683.1888880003244 684.8751449994743 689.370923999697 689.8695829994977 691.7343969997019 696.9419459998608 699.3519989997149 700.904091000557 705.1443509999663 708.0354849994183 709.1356869991869 713.4094799999148 717.1451900005341 717.7387070003897 723.8179400004447 724.2791479993612 726.5333099998534 729.1587039995939 732.7979019992054 734.0520729999989 758.8684320002794
Open Search Page TTI Baseline
Mean: 601.728 ms
Stdev: 15.338 ms (2.5%)
Runs: 570.9127609990537 573.4629720002413 583.4320880006999 584.2292080000043 584.9522709995508 585.8931479994208 586.5000409986824 591.563069999218 592.3336180001497 593.5404870007187 595.3601890001446 598.1107179988176 599.6760249994695 601.300699999556 601.4133709985763 601.722779000178 602.3489999994636 603.2089850008488 605.1166179999709 605.987223001197 606.0801189988852 608.1456300001591 611.1062819994986 611.3758949991316 612.0976159982383 615.512328999117 615.7401129994541 619.2482909988612 622.0860590003431 630.7953689992428 640.3145749997348

Current
Mean: 603.417 ms
Stdev: 13.169 ms (2.2%)
Runs: 581.2938640005887 583.9150389991701 585.8547770008445 586.5283200014383 589.1392420008779 589.4037679992616 589.9356689993292 590.1005049999803 591.684529999271 592.9249269999564 595.6667890008539 596.7653809990734 597.2813319992274 600.6632899995893 600.8970139995217 601.372599998489 602.6802580002695 603.5248619988561 604.531413000077 607.8088390007615 611.0952560007572 613.2371009998024 614.8424479998648 615.6450190003961 616.2320149987936 616.3671059999615 616.7766119986773 617.1649179998785 620.19165099971 621.0584309995174 624.0483809988946 630.7126059997827
App start runJsBundle Baseline
Mean: 191.750 ms
Stdev: 20.527 ms (10.7%)
Runs: 169 171 171 172 172 172 175 176 177 178 178 180 181 181 181 181 182 184 187 188 191 195 208 210 212 216 217 218 219 220 222 252

Current
Mean: 193.188 ms
Stdev: 17.999 ms (9.3%)
Runs: 164 169 170 171 175 176 176 177 178 178 181 181 183 187 189 190 193 197 198 198 200 201 202 202 208 209 212 217 222 223 227 228
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.6%)
Runs: 0.01314300112426281 0.013224000111222267 0.013305999338626862 0.013345999643206596 0.013630999252200127 0.013671999797224998 0.013753000646829605 0.013753999024629593 0.013833999633789062 0.014078998938202858 0.014118999242782593 0.014322999864816666 0.014363998547196388 0.014364000409841537 0.01448499970138073 0.014526000246405602 0.014527000486850739 0.014527000486850739 0.014606999233365059 0.01464800164103508 0.014728998765349388 0.014973999932408333 0.014973999932408333 0.015135999768972397 0.015300000086426735 0.015463000163435936 0.01550300046801567 0.015543999150395393 0.015543999150395393 0.015584001317620277 0.016439000144600868

Current
Mean: 0.017 ms
Stdev: 0.001 ms (5.9%)
Runs: 0.015300000086426735 0.015463000163435936 0.015463000163435936 0.015543999150395393 0.015665000304579735 0.01574699953198433 0.015951000154018402 0.01607299968600273 0.01615399867296219 0.016154000535607338 0.016234999522566795 0.016235999763011932 0.016276000067591667 0.016276000067591667 0.016277000308036804 0.016397999599575996 0.016398001462221146 0.016519999131560326 0.016520999372005463 0.016560999676585197 0.01676500029861927 0.01700899936258793 0.017048999667167664 0.017374999821186066 0.01782199926674366 0.017945000901818275 0.018187999725341797 0.01822900027036667 0.01892099902033806 0.019001999869942665
App start nativeLaunch Baseline
Mean: 19.966 ms
Stdev: 2.312 ms (11.6%)
Runs: 17 17 18 18 18 18 18 18 18 19 19 19 19 19 20 20 20 20 20 20 20 21 21 21 22 23 24 26 26

Current
Mean: 19.793 ms
Stdev: 1.606 ms (8.1%)
Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 20 20 20 21 21 21 21 21 21 22 22 23 24

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.2.60-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@aimane-chnaif
Copy link
Contributor

Tested on staging and works for me

@marcaaron
Copy link
Contributor Author

Thanks @aimane-chnaif

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.60-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants