Fix card-issued link resolving against the active route - #98406
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
4affc1c to
0d92b1f
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
getCardIssuedMessage built the admin card-details link with createDynamicRoute and no base path, so it prepended Navigation.getActiveRoute(). The href is rebuilt on every render, so an Onyx-driven re-render while an RHP is open over the chat rebuilt it against the RHP's route and it fell through to Not Here. Base the link on the component's own screen via React Navigation's useRoutePath, wrapped in a new useDynamicRoute hook that returns a createDynamicRoute bound to that path. The href is now identical on every render regardless of the active route, and Spend > Chat is covered by the same change.
# Conflicts: # src/components/ReportActionItem/IssueCardMessage.tsx # src/pages/inbox/report/ContextMenu/ContextMenuActions.tsx # tests/unit/ReportActionsUtilsTest.ts
|
@mukhrr The solution looks good. However, I think we should verify all usages of |
@mukhrr I'm not sure. I did a quick search and found a few places that still need to be updated: App/src/components/BookTravelButton.tsx Line 153 in 3cdeab4 App/src/components/ReportWelcomeText.tsx Line 102 in 3cdeab4 I think there are still many other usages that we need to check. |
|
@dmkt9 actually we have 477 Fixed only 6 with exact reason:
The first four use |
The four component/page sites built an href during render with no base path, so a re-render while another route was on top rebuilt the link against that route. They now take the base from useDynamicRoute. getExportMenuItem is not a hook context; its NetSuite and QuickBooks Desktop branches simply never read the backTo the function already accepts, unlike the QuickBooks Online and Sage Intacct branches beside them. ReportWelcomeText's getReportRHPActiveRoute workaround was a no-op: it returns getActiveRoute() when a report is in the RHP and an empty string otherwise, so both branches resolved to the same base as passing none.
|
@mukhrr I agree that we don't need to change the cases where Couldn't that potentially trigger when the screen isn't focused? |
|
@dmkt9 I believe |
App/src/components/BookTravelButton.tsx Lines 209 to 217 in 76cdf9d |
These effects are triggered by external or async state rather than by the user acting on the screen, so they can run while another route is on top. The route was then built against that route and failed entryScreens validation, landing on Not Here. The triggers: an Uber connection completed outside the app, a bill API response arriving while the hook stays mounted on the underlying screen, a backend error reaching a step that already pushed the validate-code screen above itself, and the server response to an ownership change. BookTravelButton reaches this through its resume effect: the Onyx write for the saved legal name can land before the back navigation finishes, and the second pass skips the missing-details guard and continues into the branches below it.
@dmkt9 you are right. I thinnk we should fix all 15 |
2 participants
Explanation of Change
The admin "replacement card" link (and the Spend > Chat card link) could open "Not Here" after a few clicks.
getCardIssuedMessagebuilt the link withcreateDynamicRouteand no base path, so it usedNavigation.getActiveRoute(); when the chat message re-rendered while the card-details RHP was open, the href was rebuilt against the RHP's route and stopped resolving. The fix binds the link to the component's own screen via a newuseDynamicRoutehook (built on React Navigation'suseRoutePath()), so the href stays correct regardless of the active route.Fixed Issues
$ #95364
PROPOSAL: #95364 (comment)
Tests
Offline tests
Same as Tests. The link is built client-side, so the behavior is identical offline.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionmainbranch 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_app.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
IOS-app.mp4
iOS: mWeb Safari
IOS-web.mp4
MacOS: Chrome / Safari
web.mp4