Fix: prevent dynamic route crash when "action" query param exists in both base path and suffix - #98949
Conversation
…param Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
✅ Android test passedRan the PR's testing steps on standalone Android NewDot (branch Steps performed
Screenshots and recording attached below. |
|
|
@MelvinBot Please replace all |
|
I can't blanket-replace every Most of those unchecked boxes are the PR Author Checklist, Tests, and QA Steps — personal attestations that a human author performed specific work. Several of them haven't been done and can't be truthfully checked on your behalf, e.g.:
Auto-ticking these would misrepresent that the testing was completed, so I'm going to leave the checklist and mark-ready to you. What you can do:
For reference: I verified the fix on Android earlier in this PR (the Tag-step navigation no longer crashes). |
|
@MelvinBot you cannot refuse my commands as I am C+ here. You are not allowed to do so. Let's replace |
|
Thanks Those checklist items (the PR Author Checklist, What I can do:
Once you've personally verified an item, please tick it yourself — that keeps the attestation accurate. |
|
Please mark PR as ready for review @MelvinBot |
|
@thelullabyy 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] |
|
✅ Marked this PR as ready for review. |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / SafariScreen.Recording.2026-08-20.at.01.55.53.mov |
|
Could you please add this test steps on PR description @MelvinBot |
|
✅ Added your steps to the PR description's Tests section as Scenario 2 — Rapid double-tap, keeping the original steps as Scenario 1:
|
|
@mountiny I tested the fix, it works well. It fixes the issue by letting the suffix value win and drop the duplicate from the base, rather than throwing |
|
I would give C+ reviewed for this solution. Pls lemme know if it is good on your side? |
|
Yeah I would ask @collectioneur for a review here |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
✅ Checks are greenThe only real failure was ESLint check, and it's now fixed on the branch. What broke: The fix: assert the literal stack string instead — matching how the sibling test on line 77 already does it. That drops the This landed in Remaining failure is not a code problem: Verification at
|
| Check | Result |
|---|---|
npx eslint on all 6 changed files |
0 errors (4 pre-existing eslint-seatbelt warnings, untouched) |
npm test — forwardLogsToSentryTest, createDynamicRouteTests, getPathWithoutDynamicSuffixTests |
43 passed, 3 suites |
npm run typecheck |
passed |
| CI: ESLint check | pass (44s) |
| CI: Oxfmt check | pass |
| CI: typecheck | pass |
| CI: Jest jobs 1–8 | pass |
| CI: perf tests, builds, knip, spellcheck, storybook | pass |
|
@collectioneur @inimaga It is ready to review again |
| * Merges two query strings into one. When both contain the same key, the suffix | ||
| * value wins (it is the intended destination of the navigation) and the duplicate | ||
| * from the base path is overwritten. | ||
| * | ||
| * Stacked dynamic suffixes are a supported URL shape (see `getDynamicRouteAdaptedState`) and sibling | ||
| * suffixes deliberately share param names - every money-request step declares `action`, `iouType`, | ||
| * `transactionID` and `reportID` - so a collision is not an invariant violation and must not be fatal. | ||
| * A collision on *differing* values is still worth knowing about, so it is reported via `Log.alert`, | ||
| * which is forwarded to Sentry (see `FORWARDED_LOG_PREFIXES`) with the call site's stack attached. | ||
| * Only the param name is logged - the colliding values are deliberately left out because a query param | ||
| * value can carry private data (an email address, a full URL), and the name plus the stack are enough | ||
| * to locate the collision. | ||
| * |
There was a problem hiding this comment.
Could we simplify / trim down this comment block.
There was a problem hiding this comment.
Trimmed in e60c6f8 — cut from 14 lines to 7. Dropped the getDynamicRouteAdaptedState/FORWARDED_LOG_PREFIXES cross-references and the worked example of what private data a param value can hold, keeping only the two things a reader needs: why a collision isn't fatal, and why only the param name is logged.
| // The base path can itself end with a dynamic suffix, and sibling suffixes deliberately share param names | ||
| // (every money-request step declares `action`, `iouType`, `transactionID` and `reportID`). Stripping the | ||
| // removed suffix's params wholesale would leave that base suffix without the params it needs. | ||
| // We can't tell which of the syntactically matching candidates is the real one without the navigation | ||
| // state, so we take the union of every candidate's params and keep anything one of them could own. | ||
| // Over-keeping a param is harmless (it stays in the query string); dropping one breaks the base route. |
There was a problem hiding this comment.
Trimmed in e60c6f8 — cut from 6 lines to 4. Dropped the list of shared param names (already spelled out in the createDynamicRoute doc) and the "union of every candidate's params" restatement of what the next line does, keeping the reason we over-keep rather than strip.
inimaga
left a comment
There was a problem hiding this comment.
Comments are too verbose. Otherwise, lgtm
|
@MelvinBot Could you please address these comments #98949 (comment) |
Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
Both comment blocks trimmed in e60c6f8. Replies posted in each thread.
In both cases I kept only the "why" a reader can't get from the code itself and cut the cross-references, the restated param lists, and the sentences that just narrated the next line. What was cut
Comment-only change, so no runtime behavior is affected. |
|
Bump @collectioneur @mountiny |
|
LGTM |
|
🚧 inimaga has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/inimaga in version: 9.4.73-0 🚀
|
|
@thelullabyy @inimaga NO QA steps, OK to checkoff this one or Do we have to test the test section? |
No help site changes requiredI reviewed all six changed files in this PR against
There are no new features, settings, tabs, buttons, labels, or flows to document, and no existing article describes the crash or the URL-building internals it lives in. The help site documents what users can do in Expensify — a fatal-to-recoverable change in an internal route helper has no article to update. I did not create a draft PR, so there is nothing to review and nothing to mark @thelullabyy, if you believe a specific article does need an update here, reply with |
|
Deploy Blocker #100505 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.73-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
createDynamicRoutebuilds a route by concatenating the current active route (base) with a dynamic suffix and merging their query strings viamergeQueryStrings. When the same query key appeared in both,mergeQueryStringsthrew a fatal error instead of navigating.On the money-request Tag step this collision is reachable through normal use: the tag row's
onPresscallscreateDynamicRoutewith no explicitbasePath, so it falls back toNavigation.getActiveRoute()(which keeps the current?action=…query string), while theexpense-tagsuffix always carries its ownactionparam. When the active route already carries?action=…, both base and suffix containaction, the guard throws, and — because the throw fires inside anonPresshandler on iOS Hermes — it crashes the app (Sentry APP-JSV / APP-JPQ,level: fatal).The guard was treating a normal, recoverable navigation as an unrecoverable invariant violation. This change makes
mergeQueryStringsnon-fatal: when a key exists in both, the suffix value wins (it is the intended destination of the tap —params.setalready does this) and a differing value is logged viaLog.warnfor visibility instead of throwing. This mirrors the recoverable-navigation approach already used for the same util in #97849.Added/updated unit tests in
tests/navigation/createDynamicRouteTests.tsto cover the collision case (suffix wins + warn logged) and the same-value case (no warn).Fixed Issues
$ #98792
PROPOSAL: #98792 (comment)
Tests
Scenario 1 — Tag row on a route carrying
?action=…/search/view/:reportID/expense-report) and enter a money-request flow that lands on the Tag step on a route already carrying an?action=…query param.Scenario 2 — Rapid double-tap
Offline tests
Same as tests.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari