[PDFView] no separate text for loading page

[Prince-Mendiratta]

Signed-off-by: Prince Mendiratta prince.mendi@gmail.com

Details

When the size in terms of number of pages in a PDF increases, the PDFView component on all platforms (except native) shows a series of overlapping Loading Page... messages. It is not a required or good looking disclaimer.

Fixed Issues

$ #14358
#14358 (comment)

Tests

Note: The only factor affecting the number of Loading Page... messages is the number of pages in a PDF. So, to test thoroughly, a number of different PDFs can be used with different number of pages. You can use this PDF (having around 350 pages) for testing.

1. Login to NewDot.
2. Open up any chat.
3. Click on the + icon in the composer.
4. Click on Add Attachment.
5. Choose the PDF from the document selector.
6. Notice that a PDF preview will open. It'll show the full screen loading icon and then preview the PDF. Ensure that no messages are displayed between the microseconds when the loading icon disappears and the preview begins.
7. Send the PDF in the chat.
8. Open the attached PDF in the chat.
9. Notice that a PDF preview will open. It'll show the full screen loading icon and then preview the PDF. Ensure that no messages are displayed between the microseconds when the loading icon disappears and the preview begins.

• Verify that no errors appear in the JS console

Offline tests

The issue doesn't affect the offline status. The expected behaviour however, is that if the device is offline and an attachment is clicked in the chat, the preview will show that Failed to load PDF file.
If a new attachment is attached, the PDF Preview is still shown and it should work as expected. Clicking on the send button should show a greyed out Uploading Attachment... in the chat. Turning the internet connection on should upload the PDF file. Verify that the PDF preview works as expected once the PDF is uploaded.

QA Steps

Same as Testing steps.

• 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

web.mp4

Mobile Web - Chrome

mWeb-Chrome.mp4

Mobile Web - Safari

mWeb-safari.mp4

Desktop

desktop.mp4

iOS

iOS.mp4

Android

Android.mp4

[melvin-bot]

@danieldoglas @aimane-chnaif 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]

@Prince-Mendiratta thanks for well written test step.
One thing: when offline, pdf preview should be shown before sending, in favor of #12512

[Prince-Mendiratta]

@aimane-chnaif Thank you! Good catch, I missed out on that. I have updated the description to account for that case.

[danieldoglas]

@aimane-chnaif can you please run the checklist?

[aimane-chnaif]

@Prince-Mendiratta I noticed white pages on mSafari while testing with pdf you shared.

msafari.mp4

Also, when close modal quickly before fully loaded, I see console lots of console warnings per each page:

Since these also happen on production, I think these are out of scope for this GH and should be handled as separate issues.
cc: @danieldoglas

[aimane-chnaif]

@aimane-chnaif can you please run the checklist?

yes, working on it right now

[aimane-chnaif]

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:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • 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.mov

Mobile Web - Chrome

mchrome.mov

Mobile Web - Safari

msafari.mp4

Desktop

desktop.mov

iOS

ios.mp4

Android

android.mov

[danieldoglas]

Since these also happen on production, I think these are out of scope for this GH and should be handled as separate issues.

@aimane-chnaif agreed, can you please report that?

[danieldoglas]

LGTM

[OSBotify]

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

[Prince-Mendiratta]

@Prince-Mendiratta I noticed white pages on mSafari while testing with pdf you shared.

Weirdly enough, I do not face this issue on the simulator. I'm using iPhone 13 - iOS 15.0

Also, when close modal quickly before fully loaded, I see console lots of console warnings per each page:

@aimane-chnaif Yeah, I noticed that too, have been working on the RCA since.

[github-actions]

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	712.271 ms → 731.379 ms (+19.108 ms, +2.7%)
App start runJsBundle	201.156 ms → 202.031 ms (+0.875 ms, ±0.0%)
App start nativeLaunch	21.032 ms → 21.111 ms (+0.079 ms, ±0.0%)
App start regularAppStart	0.014 ms → 0.015 ms (+0.001 ms, +7.0%)
Open Search Page TTI	606.842 ms → 604.638 ms (-2.203 ms, ±0.0%)

Show details

Name	Duration
App start TTI	Baseline Mean: 712.271 ms Stdev: 35.567 ms (5.0%) Runs: 651.9747390002012 659.2282579997554 663.9354180004448 668.4554390003905 668.8589490000159 674.7124720001593 687.3250320004299 690.0463519999757 691.0330140003935 691.7728530000895 692.7558040004224 697.2932900004089 698.1978890001774 698.8492759997025 703.469689999707 708.0716859996319 709.897299000062 710.1010170001537 710.6473139999434 716.0951509997249 721.6187580004334 724.6566439997405 726.5036599999294 737.2424750002101 744.4622900001705 744.4935010001063 746.2381379995495 749.9104300001636 753.6825989997014 768.286724999547 779.3957430003211 803.4676409997046 Current Mean: 731.379 ms Stdev: 25.451 ms (3.5%) Runs: 678.7865289999172 684.9996659997851 697.3745069997385 702.890130000189 707.5746020004153 710.3230799995363 715.2291689999402 715.3833860000595 716.8566709998995 718.6521730003878 719.4267170000821 720.0991810001433 722.417414999567 722.9858889998868 723.3423790000379 724.2179699996486 725.8757800003514 727.0713099995628 739.2446020003408 744.479408999905 744.9010269995779 746.6169670000672 751.1511340001598 751.7914990000427 753.5342920003459 760.4185659997165 761.1724420003593 761.7972699999809 764.6995500000194 767.0159059995785 792.4283739998937
App start runJsBundle	Baseline Mean: 201.156 ms Stdev: 22.081 ms (11.0%) Runs: 170 174 176 178 178 179 183 185 187 187 189 190 190 192 193 194 195 196 198 202 204 204 212 213 217 224 225 229 232 238 240 263 Current Mean: 202.031 ms Stdev: 12.922 ms (6.4%) Runs: 180 181 184 188 190 192 192 192 194 194 195 196 198 198 199 199 200 201 203 203 206 207 207 209 210 211 211 214 223 226 231 231
App start nativeLaunch	Baseline Mean: 21.032 ms Stdev: 1.732 ms (8.2%) Runs: 19 19 19 19 19 20 20 20 20 20 20 20 20 20 20 20 21 21 21 21 21 21 22 22 23 23 24 24 24 24 25 Current Mean: 21.111 ms Stdev: 1.397 ms (6.6%) Runs: 19 19 19 20 20 20 20 20 20 20 20 21 21 21 21 21 21 22 22 22 22 23 23 23 23 23 24
App start regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (5.8%) Runs: 0.0129399998113513 0.0129399998113513 0.013265000656247139 0.013345999643206596 0.013346999883651733 0.013346999883651733 0.01371300034224987 0.013875000178813934 0.013875000178813934 0.013956999406218529 0.014038000255823135 0.014038000255823135 0.014159999787807465 0.014159999787807465 0.014159999787807465 0.014282000251114368 0.014444999396800995 0.01444500032812357 0.014485999941825867 0.014688999392092228 0.014811000786721706 0.014852000400424004 0.014891999773681164 0.014932999387383461 0.015014000236988068 0.015137000009417534 0.015217999927699566 0.015258999541401863 0.015381000004708767 0.015542999841272831 0.016154000535607338 0.016234999522566795 Current Mean: 0.015 ms Stdev: 0.001 ms (5.4%) Runs: 0.013956999406218529 0.014322999864816666 0.014566999860107899 0.014607000164687634 0.014688999392092228 0.014728999696671963 0.014729000627994537 0.0147299999371171 0.014852000400424004 0.0148930000141263 0.014973999932408333 0.015014000236988068 0.015095999464392662 0.015137000009417534 0.015219000168144703 0.015219000168144703 0.015584000386297703 0.015584999695420265 0.015625 0.015666000545024872 0.01570700015872717 0.0157880000770092 0.016032000072300434 0.016154000535607338 0.016195000149309635 0.016195000149309635 0.0167239997535944 0.01733400020748377 0.017537000589072704
Open Search Page TTI	Baseline Mean: 606.842 ms Stdev: 14.892 ms (2.5%) Runs: 573.4370940001681 585.7365719992667 586.8736979998648 588.2440189998597 589.9049489991739 593.9834799999371 594.1634120000526 594.3975430000573 595.2991540003568 595.9093430005014 600.1720789996907 602.2210699999705 604.443074000068 606.1707770004869 606.7073980001733 607.5825200006366 607.9986579995602 608.7258709995076 609.072387999855 609.6527920002118 611.1609710007906 611.7864180002362 613.3936769999564 615.6635739998892 615.8697920003906 617.0498460000381 618.1365159992129 623.8299150001258 623.884766000323 629.8961590006948 633.6813970003277 643.8831789996475 Current Mean: 604.638 ms Stdev: 11.376 ms (1.9%) Runs: 578.0639650002122 585.4353839997202 588.0356040000916 589.2052410002798 593.9811610002071 594.0180660001934 596.2769370004535 597.4750160006806 597.5563559997827 597.790811999701 598.7152509996668 599.4254559995607 599.6743169995025 602.8183190003037 603.3334550000727 603.8260909998789 604.5689290007576 604.933918999508 605.6946210004389 605.9667569994926 608.1073409998789 609.213297999464 614.1875010002404 614.5290930001065 615.8006599992514 617.8807779997587 618.5032969992608 618.9908849997446 619.5959470001981 620.2177330004051 621.3763020001352 623.2236740002409

[OSBotify]

🚀 Deployed to staging by https://github.com/danieldoglas in version: 1.2.64-0 🚀

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

[marcaaron]

Can we add some explanation here or something please? If I saw this here there is nothing that tells me passing the empty string is intentional.

[Prince-Mendiratta]

@marcaaron Sure, I'll add a brief explanation and link to the issue. Should I create a PR for this?

[marcaaron]

It would be nice but no pressure on you @Prince-Mendiratta. We merged it without this suggestion so the comment is for @aimane-chnaif and @danieldoglas. I really think the root cause of the issue in something to do with the PDF library. If we implement a workaround the least we can do is document with a code comment why it exists.

[Prince-Mendiratta]

No worries, I'll send in a PR today with the additions!
Yes, makes sense. Finding empty strings passed to a prop should warrant a explanation.

[aimane-chnaif]

@marcaaron Good catch and suggestion. We should make sure to add comment on these kinds of fixes. It was my fault not to ask @Prince-Mendiratta add comment.
@Prince-Mendiratta Thanks in advance for your PR

[Prince-Mendiratta]

PR is up!

cc @aimane-chnaif @marcaaron

[marcaaron]

Thanks !

[OSBotify]

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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

[OSBotify]

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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

[OSBotify]

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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