Skip to content

Fix APP-B2Q (3/3): give notification-preferences a distinct path param to avoid reportID collision #94688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mountiny merged 3 commits into from
Jul 10, 2026
Merged

Fix APP-B2Q (3/3): give notification-preferences a distinct path param to avoid reportID collision #94688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
063ec30
Only throw in createDynamicRoute on a genuine query-param value conflict
MelvinBot Jun 26, 2026
b0cf872
Fix APP-B2Q (3/3): give notification-preferences a distinct path param
MelvinBot Jul 8, 2026
a9b3d42
Merge remote-tracking branch 'origin/main' into claude-fixDynamicRout…
MelvinBot Jul 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/ROUTES.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,10 +568,12 @@ const DYNAMIC_ROUTES = {
entryScreens: [SCREENS.WORKSPACE.DYNAMIC_CATEGORY_SETTINGS, SCREENS.SETTINGS_CATEGORIES.DYNAMIC_SETTINGS_CATEGORY_SETTINGS],
},
NOTIFICATION_PREFERENCES: {
path: 'notification-preferences',
// `reportID` is intentionally carried as a distinct path param (`notificationReportID`) rather than
// `reportID`, so it never collides with a `reportID` inherited from the surrounding report chain's
// query string. This keeps the inherited `?reportID=` intact for back navigation.
path: 'notification-preferences/:notificationReportID',

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep legacy notification preference URLs working

With this required path segment, URLs generated by the previous version such as .../notification-preferences?reportID=123 no longer match this dynamic route: the dynamic suffix matcher only sees the single notification-preferences segment, but the static suffix was removed and the new linking config requires :notificationReportID. On web, users can still land on those URLs from browser history, bookmarks, or a refresh across deploys, so this turns an existing notification-preferences deeplink into a fallback/not-found route; consider accepting the legacy query-param form while using the new path-param form for new navigations.

Useful? React with 👍 / 👎.

@ahmedGaber93 ahmedGaber93 Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think legacy support for old URLs is necessary here, since this is a deeply nested route and isn't commonly used.

entryScreens: [SCREENS.REPORT_SETTINGS.DYNAMIC_ROOT, SCREENS.DYNAMIC_PROFILE],
getRoute: (reportID: string) => getUrlWithParams('notification-preferences', {reportID}),
queryParams: ['reportID'],
getRoute: (notificationReportID: string) => `notification-preferences/${notificationReportID}` as const,
},
POLICY_ACCOUNTING_SAGE_INTACCT_EXPORT: {
path: 'sage-intacct/export',
Expand Down
2 changes: 1 addition & 1 deletion src/libs/Navigation/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1873,7 +1873,7 @@ type ReportSettingsNavigatorParamList = {
reportID: string;
};
[SCREENS.REPORT_SETTINGS.DYNAMIC_NOTIFICATION_PREFERENCES]: {
reportID: string;
notificationReportID: string;
};
[SCREENS.REPORT_SETTINGS.DYNAMIC_SETTINGS_WRITE_CAPABILITY]: {
reportID: string;
Expand Down
17 changes: 11 additions & 6 deletions src/pages/inbox/report/withReportOrNotFound.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,21 @@ type WithReportOrNotFoundProps = WithReportOrNotFoundOnyxProps & {
export default function (shouldRequireReportID = true): <TProps extends WithReportOrNotFoundProps>(WrappedComponent: ComponentType<TProps>) => ComponentType<TProps> {
return function <TProps extends WithReportOrNotFoundProps>(WrappedComponent: ComponentType<TProps>) {
function WithReportOrNotFound(props: TProps) {
const params = props.route.params;
// Most screens carry the report ID under `reportID`. The notification-preferences screen instead
// owns its target report as a distinct path param (`notificationReportID`) so it never collides
// with a `reportID` inherited from the surrounding report chain in the URL.
const reportID = 'notificationReportID' in params ? params.notificationReportID : params.reportID;
const [betas] = useOnyx(ONYXKEYS.BETAS);
const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${props.route.params.reportID}`);
const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`);
const [reportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${props.route.params.reportID}`);
const [reportLoadingState] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${props.route.params.reportID}`);
const [reportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`);
const [reportLoadingState] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${reportID}`);
const [isLoadingReportData] = useOnyx(ONYXKEYS.IS_LOADING_REPORT_DATA);
const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED);
const isFocused = useIsFocused();
const contentShown = React.useRef(false);
const isReportIdInRoute = !!props.route.params.reportID?.length;
const isReportIdInRoute = !!reportID?.length;
const isReportLoaded = !isEmptyObject(report) && !!report?.reportID;
const isReportArchived = useReportIsArchived(report?.reportID);
// The `isLoadingInitialReportActions` value will become `false` only after the first OpenReport API call is finished (either succeeded or failed)
Expand All @@ -102,9 +107,9 @@ export default function (shouldRequireReportID = true): <TProps extends WithRepo
return;
}

openReport({reportID: props.route.params.reportID, introSelected, betas});
openReport({reportID, introSelected, betas});
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [shouldFetchReport, isReportLoaded, props.route.params.reportID]);
}, [shouldFetchReport, isReportLoaded, reportID]);

if (shouldRequireReportID || isReportIdInRoute) {
const shouldShowFullScreenLoadingIndicator = !isReportLoaded && (isLoadingReportData !== false || shouldFetchReport);
Expand Down
7 changes: 6 additions & 1 deletion src/pages/settings/Report/DynamicReportSettingsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,12 @@ function DynamicReportSettingsPage({report, policy}: DynamicReportSettingsPagePr
shouldShowRightIcon
title={notificationPreference}
description={translate('notificationPreferencesPage.label')}
onPress={() => Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.NOTIFICATION_PREFERENCES.path))}
onPress={() => {
if (!reportID) {
return;
}
Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.NOTIFICATION_PREFERENCES.getRoute(reportID)));
}}
/>
)}
{shouldShowWriteCapability &&
Expand Down
Loading