Repository navigation
Remove unsafe assertions from useGeographicalStateAndCountryFromRoute - #100001
Conversation
KJ21-ENG
changed the title
|
@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] |
dylanexpensify
left a comment
dylanexpensify
left a comment
There was a problem hiding this comment.
Performance related, product input not required
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-09-28.at.16.29.53.mov |
| function isState(value: string): value is State { | ||
| return Object.hasOwn(COMMON_CONST.STATES, value); | ||
| } | ||
|
|
||
| function isCountry(value: string): value is Country { | ||
| return Object.hasOwn(CONST.ALL_COUNTRIES, value); | ||
| } |
There was a problem hiding this comment.
🟢 src/hooks/useGeographicalStateAndCountryFromRoute.ts:11-17 - fold the typeof check into the guards.
The call sites carry a narrowing step that belongs inside the predicate:
state: typeof stateFromUrl === 'string' && isState(stateFromUrl) ? COMMON_CONST.STATES[stateFromUrl].stateISO : undefined,
country: typeof countryFromUrl === 'string' && isCountry(countryFromUrl) ? countryFromUrl : undefined,routeParams[stateParamName] is unknown, so the typeof is load-bearing, but it is the guard's job. Taking unknown removes it from both call sites and makes each predicate self-contained:
function isState(value: unknown): value is State {
return typeof value === 'string' && Object.hasOwn(COMMON_CONST.STATES, value);
}
function isCountry(value: unknown): value is Country {
return typeof value === 'string' && Object.hasOwn(CONST.ALL_COUNTRIES, value);
}
return {
state: isState(stateFromUrl) ? COMMON_CONST.STATES[stateFromUrl].stateISO : undefined,
country: isCountry(countryFromUrl) ? countryFromUrl : undefined,
};Same behavior, and the non-string values test case already covers it.
There was a problem hiding this comment.
Done. Both guards now accept unknown and check typeof value === 'string' internally. The call sites use the guards directly. The non-string test still passes on the updated head.
| type StateAndCountry = {state?: State; country?: Country}; | ||
|
|
||
| function isState(value: string): value is State { | ||
| return Object.hasOwn(COMMON_CONST.STATES, value); |
There was a problem hiding this comment.
🟢 src/hooks/useGeographicalStateAndCountryFromRoute.ts:12 and :16 - say why it is Object.hasOwn and not hasKey.
ObjectUtils sits right next to isRecord in the same import and exports hasKey, so the next person who touches this will reasonably reach for it. That swap silently reopens the prototype hole this PR just closed, and the prototype keys test is the only thing that would catch it.
One line makes the constraint explicit:
// `Object.hasOwn` rather than `in` or `ObjectUtils.hasKey`: the prototype chain would match
// `constructor` and `toString`, which are not states and would type-narrow incorrectly.
function isState(value: unknown): value is State {There was a problem hiding this comment.
Added a comment explaining that own-key checks prevent prototype properties such as constructor and toString from being treated as state or country codes. The prototype-key test passes.
| @@ -1,4 +1,5 @@ | |||
| import CONST from '@src/CONST'; | |||
| import {isRecord} from '@src/libs/ObjectUtils'; | |||
There was a problem hiding this comment.
🟢 src/hooks/useGeographicalStateAndCountryFromRoute.ts:2 - import path should be @libs/ObjectUtils
Every other isRecord consumer uses @libs/ObjectUtils (Navigation.ts, GetStateForActionHandlers.ts, OAuthClient.ts, getPendingReceiptRequests.native.ts). The only @src/libs/ outlier is a pre-existing one in Table/middlewares/filtering.ts.
No functional difference, but this is a fresh import line so it may as well match.
There was a problem hiding this comment.
Done. The import now uses @libs/ObjectUtils, and the changed files pass ESLint and formatting checks.
|
Note Not a blocker for this PR, but please merge While testing the logged-out deeplink flow I hit a not-found on your branch:
I checked the same flow on staging and it does not reproduce there. Staging resolves to This is not caused by your change. The hook only runs inside The cause is the branch base. This branch is 9240 commits behind
The What I need before merge:
Separately, and worth noting for QA rather than for you to fix here: on current Please also drop the logged-out variant from QA step 6, since the parameter cannot survive sign-in today. Testing the hook while already signed in (paste the URL, then hard-reload on it) covers what this PR actually changes. cc @KJ21-ENG |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Clean PR. Code is correct and behavior-preserving; ☝️ findings are all polish, and the merging of main issue should be addressed before this is merged.
|
All contributors have signed the CLA ✍️ ✅ |
0a00eeb to
1583977
Compare
There was a problem hiding this comment.
@ikevin127 Thanks for the review. I addressed the three polish suggestions and merged main.
|
🚧 blimpich has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.96-0 🚀
|
|
No help site update is needed: this PR is an internal type-safety fix to how the address page reads |
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.99-0 🚀
|
|
No help site update is needed. This PR hardens how the address page validates the |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.99-2 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Removes three unsafe type assertions from the address-route helper and adds 14 focused tests. Valid state and country codes update the address fields; missing or invalid values are ignored.
Fixed Issues
$ #94739
PROPOSAL: #94739 (comment)
Tests
The 14 focused hook tests passed, covering valid, missing and invalid values, custom parameter names and inherited values.
To repeat:
npm test -- tests/unit/hooks/useGeographicalStateAndCountryFromRoute.test.ts.Recorded manual coverage: Windows Chrome against the staging API on commit
2cad3b0, including address links and an API-offline segment. Other platform, High Traffic account and logged-out results are not documented here. The existing author checklist is retained; the attached recording does not establish those additional results.Offline tests
Load the PR build and address form while online. Disconnect the network, then repeat the valid and invalid address-link checks below. Valid values should still update the fields; invalid values should leave existing fields unchanged. Reconnect afterward. Record network failures separately from JavaScript errors. If the app cannot load a route while offline, report that limitation rather than marking the case passed.
QA Steps
Use a build containing this PR while already signed in. Paste each path below into that build's URL and hard-reload after navigating so the query parameter reaches the address page. These checks do not require saving the address. The logged-out sign-in flow currently drops the query parameter on
main, so it does not exercise this hook./settings/profile/address. Note the existing Country and State values so a saved or default value is not mistaken for a value supplied by the URL./settings/profile/address?country=US. Confirm Country shows United States./settings/profile/address?state=MO. Confirm State shows Missouri.state=ASDF,state=,state=constructorandstate=MO-hash-a12341, using the same address path. Before each case, note the current fields. Confirm the invalid value does not replace them and the page does not crash.country=XX,country=,country=toStringandcountry=US-hash-a12341. Also open the address path without query parameters. Existing or default values may remain; invalid values must not become selections.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
Reported Windows Chrome walkthrough against the staging API at commit
2cad3b0. It covers valid, invalid, empty, prototype-key and hash-suffixed address parameters, plus API-offline checks. Expected network failures appear during the offline segment. This recording does not demonstrate other platforms or logged-out coverage.PR100001-v2-correction-walkthrough-8x-captioned.mp4