Repository navigation
refactor: remove unsafe assertions from navigation and debug helpers - #100768
Conversation
Seatbelt-Operation: s08-43192ddd-4541881-sign-1
Seatbelt-Operation: recovery-20260924-100768-merge-sign-1
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Seatbelt-Operation: recovery-20260925-100768-knip-sign-1
Seatbelt-Operation: main-refresh-100768-20261014-sign-db381862
Seatbelt-Operation: main-refresh-100768-20261014-26cd-sign-53276dcc
Seatbelt-Operation: remaining-100768-oneoff-sign-c34274b3-afc2-4902-8306-972be711fed8
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] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-10-22.at.15.54.49.mov |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
No hard blockers, but a few code-comments need addressed. Please tag me once that's done.
| const currentTopRoute = currentState.routes[currentState.index]; | ||
| if (currentTopRoute?.name !== NAVIGATORS.TAB_NAVIGATOR && typedPayload.name === NAVIGATORS.TAB_NAVIGATOR) { | ||
| (action as {type: string}).type = CONST.NAVIGATION.ACTION_TYPE.PUSH; | ||
| if (currentTopRoute?.name !== NAVIGATORS.TAB_NAVIGATOR && 'payload' in action && action.payload && 'name' in action.payload && action.payload.name === NAVIGATORS.TAB_NAVIGATOR) { |
There was a problem hiding this comment.
🟡 isNamedActionPayload is already imported in this file, so this inline narrowing can reuse it:
if (currentTopRoute?.name !== NAVIGATORS.TAB_NAVIGATOR && isNamedActionPayload(action.payload) && action.payload.name === NAVIGATORS.TAB_NAVIGATOR) {👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Reused the existing isNamedActionPayload(action.payload) predicate at this site. The route-name comparison and dispatch behavior are unchanged.
| } | ||
|
|
||
| const {action: resolvedAction, isFocusedRouteInDifferentScope} = getMinimalAction(action, navigation.getRootState()); | ||
| const resolvedActionName = resolvedAction.payload && 'name' in resolvedAction.payload && typeof resolvedAction.payload.name === 'string' ? resolvedAction.payload.name : undefined; |
There was a problem hiding this comment.
🟡 Same here, isNamedActionPayload covers this:
const resolvedActionName = isNamedActionPayload(resolvedAction.payload) ? resolvedAction.payload.name : undefined;👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Updated this site to use isNamedActionPayload(resolvedAction.payload) too, retaining the undefined fallback when the payload has no string name.
| window.Onyx.get = function (key: CollectionKeyBase) { | ||
| function get<TKey extends OnyxKey>(key: TKey): Promise<OnyxValue<TKey>> { | ||
| return new Promise((resolve) => { | ||
| const connectionReference: {current?: ReturnType<typeof Onyx.connectWithoutView>} = {}; |
There was a problem hiding this comment.
🟡 The connectionReference / isResolved handling is for a synchronous callback, which can't happen.
OnyxConnectionManager.connect defers it with Promise.resolve().then(...), so "the result connection object is available when the callback fires".
The original get had no assertion, so can we keep its body and only change the signature?
function get<TKey extends OnyxKey>(key: TKey): Promise<OnyxValue<TKey>> {
return new Promise((resolve) => {
const connection = Onyx.connectWithoutView({
key,
callback: (value) => {
Onyx.disconnect(connection);
resolve(value);
},
});
});
}👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Removed the synchronous-callback bookkeeping and now disconnect the captured connection before resolving the one-shot read. The installed Onyx 3.0.117 defers initial delivery through PendingWrites. Tests using real Onyx verify deferred delivery, the exact disconnect handle, and ordinary-key, collection-member, and collection-root reads. I also aligned the public get declaration with the existing producer’s generic key/value signature so those valid calls remain fully typed.
| const parentReportActions = await onyxWithDebugUtils.get(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`); | ||
| const parentAction = parentReportActions?.[report.parentReportActionID]; | ||
| // Debug lookups intentionally prefer the legacy message even when a modern message exists. | ||
| const parentMessage = parentAction ? getOriginalMessage({...parentAction, message: undefined}) : undefined; |
There was a problem hiding this comment.
🟡 getOriginalMessage({...parentAction, message: undefined}) copies the action just to force the legacy field, so it misses the ID when the original message lives in message.
getLinkedTransactionID from ReportActionsUtils already does this lookup (with the isMoneyRequestAction check). Can we use it here and in the fallback loop below?
const parentTransactionID = getLinkedTransactionID(parentReportActions?.[report.parentReportActionID]);
if (parentTransactionID) {
return parentTransactionID;
}
// ...
for (const action of Object.values(reportActions ?? {})) {
const transactionID = getLinkedTransactionID(action);
if (transactionID) {
return transactionID;
}
}👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Replaced both forced-legacy message copies with getLinkedTransactionID. Lookup still prefers the direct route ID, then the parent action, then the first valid fallback action. The tests cover modern and legacy messages, conflicting IDs, IOU-action filtering, missing/empty IDs, and parent-versus-fallback priority using the real message helpers.
| const getReportID = async (params: Record<string, string> | undefined) => { | ||
| if (params?.reportID) { | ||
| async function getReportID(params: ReturnType<typeof getRouteParams>): Promise<string | undefined> { | ||
| if (params && typeof params === 'object' && 'reportID' in params && typeof params.reportID === 'string' && params.reportID) { |
There was a problem hiding this comment.
🟡 This params && typeof params === 'object' && 'X' in params && typeof params.X === 'string' && params.X guard is repeated 5 times in this file.
Can we extract one helper (isRecord from @libs/ObjectUtils)?
function getStringRouteParam(params: ReturnType<typeof getRouteParams>, key: 'reportID' | 'transactionID' | 'policyID'): string | undefined {
if (!isRecord(params)) {
return undefined;
}
const value = params[key];
return typeof value === 'string' && value ? value : undefined;
}👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Added one local getStringRouteParam helper using isRecord and reused it at all five sites. It accepts only nonempty strings without coercion or trimming. The getter tests cover missing, empty, invalid, and valid parameters while preserving direct-route and report/parent fallback priorities.
| newRoutes[index] = {...focusedRoute, state: nestedResult}; | ||
| return {...state, routes: newRoutes, index} as State; | ||
| // Rebuild from the narrowed state to preserve full and partial route categories. | ||
| if (state.stale === false) { |
There was a problem hiding this comment.
🟡 No tests were added, and Codecov shows this file at 63.63% patch coverage (-9.05%).
The existing getPathFromStateTests.ts states have no stale, so only the partial branch runs. This stale === false branch is the one the app hits with a full root state.
Can we add stale: false cases for:
- a nested dynamic route
- removing a sibling route
👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Added two complete stale: false state cases: nested full-state rebuilding and removal of a focused dynamic sibling from the middle of a full root. Both assert the resulting path, the exact reduced state passed to the base serializer, and input immutability. Coverage confirms both full-state branches execute, and mutation probes catch dropping the nested rebuild or removing the wrong sibling.
| } | ||
|
|
||
| let actualSuffix = buildSuffixFromPattern(suffixPattern, focusedRoute?.params as Record<string, unknown> | undefined); | ||
| let actualSuffix = buildSuffixFromPattern(suffixPattern, focusedRoute?.params ? {...focusedRoute.params} : undefined); |
There was a problem hiding this comment.
🟢 {...focusedRoute.params} makes a copy only to get a Record<string, unknown>. isRecord narrows it without the copy, matching the rest of this PR:
isRecord(focusedRoute?.params) ? focusedRoute.params : undefinedSame for both shallowCompare arguments in linkTo/index.ts:61.
👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Removed the params copies in the serializer and both shallow-comparison inputs, using isRecord to narrow the original values directly. Necessary state/action copies remain. Serializer tests and production linkTo probes cover absent, empty, equal, and different record parameters.
Seatbelt-Operation: 2039c3a5-1f02-40d5-aa18-aea6dce4e324
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
Thanks for the changes! All 7 previous threads are fixed in f8f3f3c, the new code breaks nothing, and the PR description matches the code. I posted 1 🟡 and 2 🟢, all in the new code, none of them block - but should be addressed before merging.
Please tag me once addressed 🙌
| } | ||
|
|
||
| function getStringRouteParam(params: ReturnType<typeof getRouteParams>, key: 'reportID' | 'transactionID' | 'policyID'): string | undefined { | ||
| if (!isRecord(params) || !hasKey(params, key)) { |
There was a problem hiding this comment.
🟢 hasKey doesn't add anything after isRecord. params is already a Record<string, unknown>, so params[key] is unknown and the typeof check below handles a missing key.
if (!isRecord(params)) {
return undefined;
}👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Removed hasKey and its import. After the isRecord check, I use a checked local Record<string, unknown> assignment before indexing because TypeScript retains the existing route-parameter union. The lookup remains unknown, and the existing string/nonempty check handles missing or invalid values. No cast or additional runtime guard was added.
| ['report_report' satisfies OnyxKey]: {reportID: 'report', parentReportID: 'parent', parentReportActionID: 'parent-action'} satisfies Report, | ||
| ['reportActions_parent' satisfies OnyxKey]: {[parentAction.reportActionID]: parentAction}, | ||
| ['reportActions_report' satisfies OnyxKey]: { | ||
| fallback: {...iouAction, reportActionID: 'fallback', message: {type: CONST.IOU.REPORT_ACTION_TYPE.CREATE, text: '', IOUTransactionID: 'fallback'}}, | ||
| }, | ||
| ['transactions_modern' satisfies OnyxKey]: modernTransaction, | ||
| ['transactions_legacy' satisfies OnyxKey]: legacyTransaction, | ||
| ['transactions_fallback' satisfies OnyxKey]: fallbackTransaction, |
There was a problem hiding this comment.
🟡 Can we build these keys from ONYXKEYS like the rest of this file already does (${ONYXKEYS.COLLECTION.REPORT}report, L330)?
There are about 25 hard-coded prefixes ('report_', 'reportActions_', 'transactions_', 'policy_'). satisfies OnyxKey only checks that some collection prefix matches, so it won't catch a renamed key.
[`${ONYXKEYS.COLLECTION.REPORT}report`]: {reportID: 'report', parentReportID: 'parent', parentReportActionID: 'parent-action'} satisfies Report,
[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}parent`]: {[parentAction.reportActionID]: parentAction},
[`${ONYXKEYS.COLLECTION.TRANSACTION}modern`]: modernTransaction,👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Replaced all 27 literal collection-prefix occurrences in the fixtures and expected results with the corresponding ONYXKEYS.COLLECTION constants. The suffixes, fixture values, and expected results are preserved.
| // Then resolution follows one disconnect of the returned handle and later writes have no live subscription. | ||
| expect(value).toEqual(modernTransaction); | ||
| expect(disconnect).toHaveBeenCalledTimes(1); | ||
| expect(connect).toHaveReturnedWith(disconnect.mock.calls.at(0)?.[0]); |
There was a problem hiding this comment.
🟢 The same connect, disconnect and exact-handle block is repeated in 4 tests (here, L155, L175, and 3 times in the collection-root test).
Can we extract one helper so each test only shows what's different? It counts relative to the calls already made, so repeated reads in one test still work.
async function readOnce<TKey extends OnyxKey>(key: TKey) {
const connect = jest.spyOn(Onyx, 'connectWithoutView');
const disconnect = jest.spyOn(Onyx, 'disconnect');
const disconnectsBefore = disconnect.mock.calls.length;
const result = window.Onyx.get(key);
expect(disconnect).toHaveBeenCalledTimes(disconnectsBefore);
const value = await result;
expect(disconnect).toHaveBeenCalledTimes(disconnectsBefore + 1);
expect(connect).toHaveLastReturnedWith(disconnect.mock.calls.at(-1)?.[0]);
return value;
}👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Extracted one generic readDebugOnyxOnce helper and reused it at all six read sites. It checks connection/disconnection counts relative to the calls already made, deferred delivery, and the exact successful connection handle. Each test retains its scenario-specific value assertions and later-write checks, including the repeated collection-root reads.
Seatbelt-Operation: 981bc10d-de59-405e-a1ed-f597bf83c62e
|
Hi @ikevin127, addressed the redundant guard, collection keys, and repeated test assertions. Could you please take another look? |
| if (!isRecord(params)) { | ||
| return undefined; | ||
| } | ||
| const recordParams: Record<string, unknown> = params; |
There was a problem hiding this comment.
🟢 Can we type params as unknown instead? Then isRecord narrows it straight to Record<string, unknown> and the extra alias goes away.
function getStringRouteParam(params: unknown, key: 'reportID' | 'transactionID' | 'policyID'): string | undefined {
if (!isRecord(params)) {
return undefined;
}
const value = params[key];
return typeof value === 'string' && value ? value : undefined;
}👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Changed params to unknown, so isRecord now narrows it directly to Record<string, unknown> and the recordParams alias is gone. The guard, key union and nonempty-string check are unchanged, and callers keep their existing types.
Seatbelt-Operation: ea2c4845-1b6f-4086-a320-6f4997a308b7
Seatbelt-Operation: 340c0d02-b546-4639-a70e-7253bfa02749
|
Hi @ikevin127, after your approval, #102135 (Onyx 3.0.119) merged and conflicted with this PR in
The other files are unchanged from what you approved. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 blimpich 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.5.21-0 🚀
|
|
No help site update is needed. This PR removes unsafe type assertions in navigation helpers and development-only debug getters, so users see the same navigation and features as before. |
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.5.23-0 🚀
|
|
No help site update is needed. This PR only removes unsafe type assertions in navigation helpers and development-only debug getters, so user-facing navigation and features are unchanged. |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.5.23-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Remove unsafe assertions from navigation state, nested action parameters, and development debug getters. Known routes keep their navigation behavior. Invalid nested parameters stop the sidebar-marker traversal at the last safe object; debug Onyx reads use Onyx's public
get, and the debug-onlyOnyx.logis declared optional because it exists only after non-production setup. Transaction lookup now prefers modern object messages, using legacyoriginalMessagefor array or absent messages; a modern message without a linked ID does not reuse that action's legacy ID. Lookup preserves direct route ID priority, then the parent IOU action, then the first report IOU action with a nonempty linked ID in iteration order.Fixed Issues
$ #94739
PROPOSAL: #94739 (comment)
Tests
Upstream Jest, typecheck and lint passed. The navigation suites cover path parsing and serialization.
Platforms: Android native, Android mWeb Chrome, iOS native, iOS mWeb Safari and macOS Chrome/Safari. Run desktop-only steps where applicable. Use browser Back/Forward on web and the App back control on native.
Report and message links
Setup: Use a conversation with links to another report and a specific message, plus an existing attachment.
Workspace navigation
Setup: Use two accessible rooms in different workspaces and an account with workspace-member access.
Concierge return
Development debug getters
Setup: Use a development web build and an existing expense thread. These getters are unavailable in production.
await window.report. Verify its ID matches the open report.await window.transaction. Verify its ID matches the open expense.await window.policy. Verify its ID matches that workspace.undefinedwithout hanging or throwing.Onyx.log('session'). Verify the stored session value prints in the console.Offline tests
No additional offline case: request queuing, persistence and reconnect handling are unchanged.
QA Steps
Same as the navigation cases in Tests. Development debug getters require a development build.
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
Recordings (5)
01-report-message-links-narrow.mp4
02-room-context-desktop.mp4
03-members-return-narrow.mp4
04-concierge-return-narrow.mp4
05-marked-message-forward-desktop.mp4