Conversation
…tus bar overlap Co-authored-by: Gabriel Valforte <Valforte@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Co-authored-by: Gabriel Valforte <Valforte@users.noreply.github.com>
|
Fixed and pushed — What was failing: only Cause: wrapping the record rows in the new inner Fix: ran VerificationLocal (all passing): CI after the push: The one remaining red check is The manual test steps and the view run · no recording available |
|
🤖 Android test steps verification results: Verified on standalone Android NewDot. I reached both screens this PR changes and confirmed the status-bar overlap fix with read-back evidence, but a real NetSuite OAuth connection is not obtainable on a fresh test account, so the "many custom lists" scrolling case could not be exercised live. The What still needs a human: step 4's scrolling behaviour with a populated custom-list record set, against a live NetSuite-connected workspace. That is the half of the fix I could not reach. One note on the run itself: the app hit a native view-mounting crash early in the session, before onboarding finished. It did not recur after recovery and is unrelated to this PR's changes. view run · no recording available |
|
🤖 Web test steps verification results: Verified on dev NewDot web. I reached Workspace settings → Accounting, but could not establish a real NetSuite OAuth connection, so neither of the two pages this PR changes was reachable and steps 3–5 could not be exercised. Unlike the Android run, the web path has no safe way around this: reaching the routes directly needs a full page reload, which drops the authenticated session. What still needs a human on web: steps 3, 4 and 5, against a workspace with a real NetSuite connection and a populated custom-list record set. Step 4 is the one worth the attention — the PR author notes the clipping is not Android-only and should reproduce in a short desktop window. For reference, the routes a human can jump straight to are Earlier I ran the same steps on Android, where I could reach both pages via deep link. Step 5 passed there; step 4's scrolling case was blocked for the same missing-NetSuite-data reason. view run · no recording available |
|
@MelvinBot could you add regression tests for the header padding and scroll/footer layout for both custom lists and custom segments? |
…oll/footer layout Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
Added and pushed — 13 new tests across two files, run for both New:
Extended: Verification — I confirmed each test fails without the fixA passing test proves nothing on its own, so I temporarily reverted each fix and re-ran:
One assertion detail worth noting: the prop checks read a single scalar off Local checks, all passing:
These are unit-level layout assertions, not a device check — they pin the component structure that the two bugs broke, but they don't replace running the manual steps against a NetSuite-connected workspace. The scrolling behaviour with a populated record set is still the thing worth a human's eyes. view run · no recording available |
|
Resolved ✅ |
The failing checks are not caused by this PR —
|
| Failing check | Cause |
|---|---|
typecheck |
InboxTabSelector.tsx TS2451 ×2 — only errors reported; zero in the NetSuite files |
test (job 4) |
tests/ui/PaginationTest.tsx — renders the sidebar |
test (job 7) |
tests/ui/UnreadIndicatorsTest.tsx, tests/ui/GroupChatNameTests.tsx |
test (job 8) |
tests/ui/SessionTest.tsx |
This PR's own new tests both pass in the same run:
test (job 5) PASS tests/ui/NetSuiteImportCustomFieldPageTest.tsx
test (job 6) PASS tests/ui/NetSuiteCustomListSelectorPageTest.tsx
Check independent approval is expected on a bot-authored PR and is not a code failure. ESLint check was still pending when I looked.
How the breakage got onto main (a semantic merge conflict in #100881)
The duplicate is on the main tip itself (c85bcf4), and the file is byte-identical between main and this PR's merge ref — so the merge did not introduce it:
$ git diff --stat c85bcf4 origin/pr-101107-merge -- src/pages/inbox/sidebar/InboxTabSelector.tsx
(no output — identical)
It landed with the merge of #100881 (ad2bdf17), and neither side of that merge was broken on its own:
mainbefore the merge (50b3c85f) had the declaration at line 40, feedinguseIsSidebarRouteActive.- The Align tab bar, top padding, and search box location across Home/Inbox/Spend/Workspaces on mobile #100881 branch head (
fa1af592) did not contain line 40. It added its own declaration at line 42 for the new tab icons.
The two additions were textually non-overlapping, so Git auto-merged both and kept each one. No conflict was raised, and because the branch never contained the other declaration, 100881's own CI was green. The result only breaks once both lines coexist.
For the record, #101164 (c85bcf4, the current tip) is not the cause — it does not touch this file; it merely sits on top of the already-broken tree.
Next Steps: Reply with @MelvinBot <your feedback> if you want me to look at something else here.
Still
|
|
@MelvinBot please merge main. |
Resolved a conflict in NetSuiteImportCustomFieldPage.tsx: main replaced the record row component (MenuItemWithTopDescription -> MenuItemField with a MenuItem.BrickRoadIndicator child) while this branch moved those same rows inside an inner ScrollView. Kept both: main's MenuItemField rows rendered inside this branch's ScrollView. Updated the row mock in NetSuiteImportCustomFieldPageTest.tsx to mock @components/MenuItem/presets/MenuItemField (reading the record name from value) since the page no longer renders MenuItemWithTopDescription.
|
Merged and pushed — One conflict, in
Resolution: One test needed updating. Also worth noting: the AI tests run locally on the merged tree — all passing
I did not re-run the browser test steps. Both earlier runs (web and Android) could not reach these two pages because a real NetSuite OAuth connection isn't obtainable on a fresh test account, and nothing about this merge changes that. The layout behavior this PR fixes is covered by the regression tests above, but the merge did change what the rows render, so a human with a NetSuite-connected workspace should still confirm the rows look right on the custom lists and custom segments pages. |
|
@MelvinBot please add the Design label. |
|
Added the The PR now carries Heads up: the label alone doesn't notify the design team — the author checklist asks you to also tag |
Updated testing, checklist, and recordingsTestsTested locally with seeded NetSuite data. Prerequisite: A workspace with enough NetSuite custom lists and segments to require scrolling.
Offline tests
QA StepsSign in on staging as an admin of a NetSuite-connected workspace with enough configured custom lists and segments to exceed the viewport, plus an available list to select.
PR Author Checklist
Screenshots/VideosAndroid: NativeAndroid-pr101107.mp4Android: mWeb ChromeAndroid-mWeb-pr101107.mp4iOS: Nativeios-pr101107.mp4iOS: mWeb Safariios-mWeb-pr101107.mp4MacOS: Chrome / SafariChrome-pr101107.mp4@MelvinBot please replace the sections from |
|
Done — replaced I dropped the outer One thing to look at: the last paragraph of Explanation of Change is now stale. It still says runtime verification couldn't be completed and that "these changes have not been exercised against a live NetSuite-connected workspace, so the manual test steps below still need to be run by a human." Your new checklist says the opposite — tested on all five platforms with recordings attached. That paragraph was outside the range you asked me to replace, so I left it alone. Want me to remove it or rewrite it to match? |
|
@MelvinBot please update it. |
|
Updated — the stale paragraph in Explanation of Change now matches the checklist. Was:
Now:
It also drops the outdated Nothing else in the description changed. I diffed the live body against my replacement before and after editing: exactly one line differs, line 30. Everything else — Explanation of Change, Fixed Issues, Tests, Offline tests, QA Steps, the checklist, and all five recording links — is byte-for-byte identical. No code changes and no push; this was a description edit only. |
|
@MelvinBot please mark this PR as ready for review. |
|
@nyomanjyotisa @Valforte 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] |
|
Marked as ready for review. ✅ Reviewers were auto-requested on the transition: Check status: everything green — all 8 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid-pr101107.mp4Android: mWeb ChromeAndroid-mWeb-pr101107.mp4iOS: HybridAppios-pr101107.mp4iOS: mWeb Safariios-mWeb-pr101107.mp4MacOS: Chrome / SafariChrome-pr101107.mp4 |
| />, | ||
| ); | ||
|
|
||
| // `includePaddingTop` defaults to true. This page used to pass it as false, which zeroed the top safe-area |
There was a problem hiding this comment.
Comments shouldn't reference past behavior
There was a problem hiding this comment.
Reworded to describe only what the code does now — tests/ui/NetSuiteCustomListSelectorPageTest.tsx:104-106:
// `includePaddingTop` defaults to true, so `ScreenWrapper` applies the top safe-area inset and
// `HeaderWithBackButton` renders below the status bar. This page has no full-bleed header, so it must not
// opt out.
The assertion is unchanged.
| it('keeps the add button footer outside the scroll area so it stays docked when the list overflows', () => { | ||
| renderPage(importCustomField, 20); | ||
|
|
||
| // The footer used to live inside the scroll area, so a record list taller than the viewport pushed the add |
There was a problem hiding this comment.
Comments referencing previous behavior
There was a problem hiding this comment.
Reworded to describe the current composition — tests/ui/NetSuiteImportCustomFieldPageTest.tsx:158-159:
// The footer is a sibling of the scroll area, not a child of it, so a record list taller than the viewport
// scrolls underneath while the add button stays docked and visible.
The assertions are unchanged.
| it('adds the bottom safe-area padding to both the scroll area and the footer', () => { | ||
| renderPage(importCustomField, 3); | ||
|
|
||
| // The footer sits outside the ConnectionLayout ScrollView that used to supply the bottom inset, so each one |
There was a problem hiding this comment.
Again, referencing previous behavior. Comments should focus on what's happening and not what it was meant to fix
There was a problem hiding this comment.
Reworded to explain why each element needs its own inset today, with no reference to the old layout — tests/ui/NetSuiteImportCustomFieldPageTest.tsx:167-168:
// The scroll area and the footer are siblings, so neither one inherits a bottom inset from the other and
// each has to request its own.
I also applied the same treatment to one comment you didn't flag, tests/ui/NetSuiteImportCustomFieldPageTest.tsx:131, which had the same "previously pushed a header under the status bar" framing.
The assertions are unchanged.
|
@MelvinBot Address @Valforte’s three review comments: describe current behavior, not past bugs. Keep code and assertions unchanged, rerun both test files, and push. |
…r bug Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
@nyomanjyotisa @Valforte 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] |
|
Done — all three comments now describe current behavior, and both test files still pass. Pushed as The diff is comments only: 8 lines changed across the two test files, no code and no assertions touched.
I also applied the same treatment to a fourth comment you didn't flag, VerificationAlso clean: No app testing was run — the change is comment text in test files, so there is no runtime behavior to re-verify beyond the suites above. |






Explanation of Change
Two independent layout bugs on the NetSuite custom fields pages.
1. Header overlapped the status bar on the "List name" selector page
NetSuiteCustomListSelectorPagepassedincludePaddingTop={false}toScreenWrapper. That flag makesScreenWrapperreturn an empty padding style instead of the top safe-area inset, soHeaderWithBackButtonwas drawn at y=0, underneath the Android status bar. This page was the only page undersrc/pages/workspace/accounting/that set the flag, and it has no full-bleed header that would justify it. Removing it restores the default top safe-area padding and matches every sibling selector page.2. "Add custom list" button was hidden and the page did not scroll
NetSuiteImportCustomFieldPagepassedcontentContainerStyle={[styles.flex1]}while leavingConnectionLayout'sScrollViewenabled, soflex: 1landed on the ScrollView's content container. That pins content height to exactly the viewport height, so the content can never exceed the scroll viewport and scrolling never engages. With enough custom lists, the rows filled the viewport and theFixedFooterholding the Add button — which lived inside that same ScrollView — was pushed into the clipped region and disappeared.The fix adopts the pattern already used by the equivalent Sage Intacct page (
SageIntacctUserDimensionsPage), which renders the same "list of records + Add button" layout correctly:shouldUseScrollView={false}soConnectionLayout's outer ScrollView becomes a plainflex: 1View.ScrollViewwithaddBottomSafeAreaPadding.FixedFooteras a sibling outside that inner ScrollView, withaddBottomSafeAreaPaddingre-added (the bottom inset was previously supplied by theConnectionLayoutScrollView).styles.mtAutois no longer needed now that the footer sits outside the scroll area.The empty state stays a direct child of the
flex: 1Viewso it keeps centering vertically.Result: the record list scrolls and the Add button stays docked and always visible.
Notes for the reviewer:
flex: 1clipping is platform-specific — it reproduces anywhere the record list is taller than the viewport. It was reported on Android because the viewport is shortest there. Worth checking a short desktop window too.NetSuiteImportCustomFieldPagerenders for bothcustomListsandcustomSegmentsvia theimportCustomFieldroute param, so the hidden Add button reproduced on the custom segments page as well.Regression coverage:
tests/ui/NetSuiteImportCustomFieldPageTest.tsxandtests/ui/NetSuiteCustomListSelectorPageTest.tsx— 20 tests across bothcustomListsandcustomSegments, pinning the scroll/footer composition and the header's top safe-area padding. Each one was confirmed to fail against the pre-fix code. Also passing locally:typecheck,eslint,react-compiler-compliance-check,spell-changed, andoxfmt. The flows were exercised manually against a NetSuite-connected workspace on all supported platforms — recordings are under Screenshots/Videos below.Fixed Issues
$ #100970
PROPOSAL: #100970 (comment)
Tests
Tested locally with seeded NetSuite data.
Prerequisite: A workspace with enough NetSuite custom lists and segments to require scrolling.
Offline tests
QA Steps
Sign in on staging as an admin of a NetSuite-connected workspace with enough configured custom lists and segments to exceed the viewport, plus an available list to select.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android-pr101107.mp4
Android: mWeb Chrome
Android-mWeb-pr101107.mp4
iOS: Native
ios-pr101107.mp4
iOS: mWeb Safari
ios-mWeb-pr101107.mp4
MacOS: Chrome / Safari
Chrome-pr101107.mp4