Skip to content

Fix: scroll to bottom when error appears after content size change#71426

Closed
NagaraniKaleeswaran wants to merge 1 commit into
Expensify:mainfrom
NagaraniKaleeswaran:fix/70601-scroll-on-content-size-change
Closed

Fix: scroll to bottom when error appears after content size change#71426
NagaraniKaleeswaran wants to merge 1 commit into
Expensify:mainfrom
NagaraniKaleeswaran:fix/70601-scroll-on-content-size-change

Conversation

@NagaraniKaleeswaran

@NagaraniKaleeswaran NagaraniKaleeswaran commented Sep 27, 2025

Copy link
Copy Markdown

Explanation of Change

This PR ensures the page auto-scrolls to the bottom whenever a new error message is rendered and causes the content size to grow.

In BookTravelButton.tsx, I centralized error setting in a showError() helper which also toggles a shouldScrollToBottom flag (passed from the parent).

In ManageTrips.tsx, I listen to onContentSizeChange on the page ScrollView and, when shouldScrollToBottom is true, I programmatically call scrollToEnd({animated: true}) and reset the flag.
This addresses the case where validation / eligibility errors appear below the fold (e.g., when booking travel) and were previously off-screen.

###Fixed Issues

$ #70601

PROPOSAL: #70601 (comment)

###Tests
Precondition

  • Sign in to an Expensifail account in OldDot.
  • Ensure you have at least one group workspace (create 1–2 if needed).
  • Set Individual workspace as the default.

Steps

  1. Open dev.new.expensify.com:8082 (local dev).
  2. Go to Workspaces → (any group workspace) → Book travel.
  3. Click Book travel.
  4. Verify: As soon as an error message renders (e.g., “default workspace” error), the page auto-scrolls so the error is fully visible.
  5. Repeat on mWeb platforms (listed below) and confirm the same scrolling behavior.

Extra scenario (BookTravelButton errors)

  • In the same page, trigger an error from BookTravelButton (e.g., phone-as-primary-login or non-paid-policy).
  • Verify the page scrolls instantly to bring the error fully into view.

Offline tests N/A

QA Steps

Same as Tests above.

  • 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: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • Windows: Chrome/Edge
    • 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 verified there are no new alerts related to the canBeMissing param for useOnyx
  • 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 used JaimeGPT to get English > Spanish translation. I then posted it 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • 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.ts 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 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(theme.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • 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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • 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 added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Windows: Chrome ✅

[✔️] Verified that the page auto-scrolls when the error message appears.

scroll-error.mp4
Windows: Edge ✅

[✔️] Verified that the page auto-scrolls when the error message appears.

Scroll-error-edge.webm
Android: Native ❌

Not tested (no device/emulator access)

Android: mWeb Chrome ❌

Not tested (no device/emulator access)

iOS: Native ❌

Not tested (no device/emulator access)

iOS: mWeb Safari ❌

Not tested (no device/emulator access)

MacOS: Desktop ❌

Not tested (no desktop app access)

@NagaraniKaleeswaran NagaraniKaleeswaran requested a review from a team as a code owner September 27, 2025 17:12
@melvin-bot melvin-bot Bot requested review from ikevin127 and removed request for a team September 27, 2025 17:12
@melvin-bot

melvin-bot Bot commented Sep 27, 2025

Copy link
Copy Markdown

@ikevin127 Please 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]

@github-actions

github-actions Bot commented Sep 27, 2025

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@NagaraniKaleeswaran

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

CLABotify added a commit to Expensify/CLA that referenced this pull request Sep 27, 2025
@NagaraniKaleeswaran

Copy link
Copy Markdown
Author

recheck

1 similar comment
@NagaraniKaleeswaran

Copy link
Copy Markdown
Author

recheck

@ikevin127

ikevin127 commented Sep 27, 2025

Copy link
Copy Markdown
Contributor

@NagaraniKaleeswaran Thanks for opening the PR! A few things that need addressed in order to move forward with the review:

  1. The PRs Screenshots/Videos section must include video proof of the fix for all platforms, currently you did not add any (or at least not correctly):
  • Android: Native (you must build the Android: Native app, run it and record the fix)
  • Android: mWeb Chrome (you must access the Web app on Android Chrome mobile browser and record the fix)
  • iOS: Native (you must build the iOS: Native app, run it and record the fix)
  • iOS: mWeb Safari (you must access the Web app on iOS Safari mobile browser and record the fix)
  • MacOS: Chrome / Safari (this is the main Web app)
  • MacOS: Desktop (this is the MacOS: Desktop app)

🟡 Currently you added what looks like a recording of MacOS: Chrome / Safari (main Web app), but you added it to Android: mWeb Chrome instead which is incorrect / ❌ and you're missing the videos for all other platforms which currently causes a blocker the PR review.

ℹ️ Refer to the contributor guidelines / local development for setup details on all environments / platforms.

  1. The PROPOSAL: N/A (small UI behavior fix; no proposal required) section must contain a link to your issue proposal instead of N/A, like PROPOSAL: [Payment 2025-10-20] [$250] WEB - Book Travel - RHP does not auto scroll when an error message appears off-screen #70601 (comment).

  2. For Offline tests you can simply add N/A since this issue / fix does not include offline behaviour, but UI / UX related change.

  3. The Tests section should contain the issue's precondition and steps, then on top of that keep the "verify..." parts you added since those are correct, something like this:

Precondition:
- Sign in to a Expensifail account in OD
- Create one or 2 group workspaces if there is no existing workspaces
- Set the Individual workspace as default

1. Login to ND.
2. Click the FAB > Book Travel.
3. Click the Book travel.
4. Verify: As soon as the error message renders, the page auto-scrolls so the error is fully visible.

☝️ That's it in terms of tests, you only have to specify test steps to verify your fix.

@NagaraniKaleeswaran Also I noticed you applied changes to the BookTravelButton.tsx component which wasn't part of your initial proposal, mind explaining why you added changes there and add test steps for that scenario as well ?

@NagaraniKaleeswaran

Copy link
Copy Markdown
Author

@ikevin127
Thank you for the detailed feedback! 🙏

Here’s how I’ll address each point:

Screenshots/Videos – Understood. I’ll build and run the app on all required platforms and upload the video proof for each in the correct section:

  1. Android: Native
  2. Android: mWeb Chrome
  3. iOS: Native
  4. iOS: mWeb Safari
  5. MacOS: Chrome / Safari
  6. MacOS: Desktop

PROPOSAL section – I’ll update this with a direct link to the original issue proposal instead of “N/A”.

Offline tests – I’ll mark this as N/A since this fix doesn’t involve any offline behavior.

Tests section – I’ll rewrite it to include the preconditions and detailed test steps along with the verification step, as you suggested.

Changes in BookTravelButton.tsx – I updated this component to ensure that any error message rendered (not just the default workspace error) triggers the auto-scroll behavior. Since errors from this button also needed to scroll into view, the change was necessary to fully address the issue. I will add test steps for this scenario too.

I’ll make these changes and push an updated PR shortly ✅

@NagaraniKaleeswaran

NagaraniKaleeswaran commented Sep 28, 2025

Copy link
Copy Markdown
Author

@ikevin127
Thanks once again for the review!
I have verified the fix on Windows Chrome and Windows Edge (web app) and confirmed that the page auto-scrolls when an error message is rendered.

At the moment, I don’t have access to Android/iOS devices or emulators or the Mac desktop app, so I wasn’t able to record the fix for:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Desktop app

Could someone from the review/QA team please help confirm the behavior on those platforms? 🙏

Thanks again, and let me know if I should adjust anything else in the PR.

@NagaraniKaleeswaran

Copy link
Copy Markdown
Author

recheck

@ikevin127

Copy link
Copy Markdown
Contributor

@NagaraniKaleeswaran It's fine if you stop from commenting recheck, it's all good, the CLA was already signed 👍

At the moment, I don’t have access to Android/iOS devices or emulators or the Mac desktop app, so I wasn’t able to record the fix

Could someone from the review/QA team please help confirm the behavior on those platforms? 🙏

Well that's a problem, as all contributors are expected to be able to run the tests on all platforms, it's part of the checklist and there's no way around it - this is also why it's important to be able to test your proposal / fix on all platforms BEFORE you even post a proposal to ensure your fix works on all platforms without breaking things on any of them.

🟡 We're on HOLD for now as this is a blocker, I'll have to take this up with the team and get back to you with an answer regarding moving forward here.

@ikevin127

Copy link
Copy Markdown
Contributor

While waiting for a response from the team, just want to let you know that there are ways for you to access a MacOS machine at a low cost, we have this documented here.

@NagaraniKaleeswaran

Copy link
Copy Markdown
Author

While waiting for a response from the team, just want to let you know that there are ways for you to access a MacOS machine at a low cost, we have this documented here.

I have seen the document will try to test in all platforms

@ikevin127

Copy link
Copy Markdown
Contributor

will try to test in all platforms

@NagaraniKaleeswaran It's been over 1 week since assignment which technically means contract termination - but because it's your first assignment and you might have not known about the testing situation - please let me knoe how is it going, are you able to add test proof for all platforms ?

♻️ Otherwise let me know so we can proceed to next steps here:

  • you check all checkboxes on the PR Author Checklist (even though you didn't add test on all platforms)
  • I will make sure to test thoroughly on all platforms and add proof on my checklist
  • we merge the PR and you get paid when regression period is over (if any regressions occur I will handle them and we both get bounty penalty)
  • you will receive a formal warning because our documentation makes it very clear you need to be able to test all platforms before posting a proposal / when opening a PR
  • make sure that before you post proposals again: you sort out the all platforms testing situation because repeating this will lead to other warnings and 3 warnings = removal from the contributor program

@NagaraniKaleeswaran

Copy link
Copy Markdown
Author

will try to test in all platforms

@NagaraniKaleeswaran It's been over 1 week since assignment which technically means contract termination - but because it's your first assignment and you might have not known about the testing situation - please let me knoe how is it going, are you able to add test proof for all platforms ?

♻️ Otherwise let me know so we can proceed to next steps here:

  • you check all checkboxes on the PR Author Checklist (even though you didn't add test on all platforms)
  • I will make sure to test thoroughly on all platforms and add proof on my checklist
  • we merge the PR and you get paid when regression period is over (if any regressions occur I will handle them and we both get bounty penalty)
  • you will receive a formal warning because our documentation makes it very clear you need to be able to test all platforms before posting a proposal / when opening a PR
  • make sure that before you post proposals again: you sort out the all platforms testing situation because repeating this will lead to other warnings and 3 warnings = removal from the contributor program

I have tested in all platforms will upload tomorrow @ikevin127

@NagaraniKaleeswaran

Copy link
Copy Markdown
Author

@ikevin127
Is this task still with me should i upload videos or Has is it been assigned to someone else sir

@ikevin127

Copy link
Copy Markdown
Contributor

@NagaraniKaleeswaran The task was reassigned, no need to post the videos anymore - feel free to close the PR and stand-by for more info on the issue from the BZ team member regarding payment.

@NagaraniKaleeswaran

Copy link
Copy Markdown
Author

@NagaraniKaleeswaran The task was reassigned, no need to post the videos anymore - feel free to close the PR and stand-by for more info on the issue from the BZ team member regarding payment.

Thanks for informing sorry for the inconvenience for next task I will be fully prepared

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.

2 participants