Trim verbose comments to the non-obvious whys

Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>

Author: MelvinBot

src/hooks/useIsOneTransactionThread.ts

@@ -9,17 +9,15 @@ import useOneTransactionThreadReportID from './useOneTransactionThreadReportID';
 import useParentReportAction from './useParentReportAction';
 
 /**
- * Whether the report is the transaction thread of its parent report's only expense.
- *
- * Reactive counterpart of `ReportUtils.isOneTransactionThread`, which reads module globals and so cannot drive
- * rendering. The shared derivation does not exclude send money, hence the `isSentMoneyReportAction` guard: a send
- * money thread is not interchangeable with its report.
+ * Whether the report is the transaction thread of its parent report's only expense. Reactive counterpart of
+ * `ReportUtils.isOneTransactionThread`, which reads module globals and so cannot drive rendering.
  */
 function useIsOneTransactionThread(report: OnyxEntry<Report>): boolean {
     const parentReportID = getNonEmptyStringOnyxID(report?.parentReportID);
     const oneTransactionThreadReportID = useOneTransactionThreadReportID(parentReportID);
     const parentReportAction = useParentReportAction(report);
 
+    // A send money thread is not interchangeable with its report, and the shared derivation doesn't exclude it.
     return !!report?.reportID && oneTransactionThreadReportID === report.reportID && !isSentMoneyReportAction(parentReportAction);
 }
 

src/pages/inbox/OneTransactionThreadRedirectHandler.tsx

@@ -38,11 +38,7 @@ type ExpenseReportRouteParams = {
     backTo: string | undefined;
 };
 
-/**
- * The report a `backTo` points at, whatever route shape it uses. The same report is reachable as `r/`, `e/`,
- * `search/view/` and `search/r/`, so the path alone cannot be compared - resolving it the way the navigator itself
- * would avoids hard-coding that list.
- */
+/** The report a `backTo` points at. Resolved through the navigator because the same report is reachable as `r/`, `e/`, `search/view/` and `search/r/`. */
 function getBackToReportID(backTo: Route): string | undefined {
     const focusedRoute = findFocusedRoute(getStateFromPath(backTo));
     const params = focusedRoute?.params;
@@ -55,22 +51,17 @@ function getExpenseReportRoute({routeName, reportID, referrer, backTo}: ExpenseR
         return ROUTES.REPORT_WITH_ID.getRoute(reportID, undefined, referrer, backTo);
     }
 
-    // Clicking the expense of a single-expense report in Search opens the report on this same RHP route rather than a
-    // money request report one, so staying here and swapping the report is what the user would have got by clicking it.
     if (isSearchTopmostFullScreenRoute()) {
         return ROUTES.SEARCH_REPORT.getRoute({reportID, backTo});
     }
 
-    // Outside Search an expense report belongs in the RHP, which is full screen on a narrow layout anyway. Swapping
-    // one RHP route for another keeps the redirect inside the navigator it started in - replacing it with the central
-    // pane `r/` route instead leaves the back stack straddling two navigators.
+    // Swapping one RHP route for another keeps the redirect inside the navigator it started in.
     return ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID, backTo});
 }
 
 /**
  * Renders nothing. A single-expense report renders its only expense inline (see `shouldDisplayReportTableView`), so
- * that expense's transaction thread duplicates the report. This replaces such a route with the report itself - the
- * route-level backstop for entry points that don't check the transaction count themselves.
+ * that expense's transaction thread duplicates the report. This replaces such a route with the report itself.
  */
 function OneTransactionThreadRedirectHandler() {
     const route = useRoute<ReportScreenRoute>();
@@ -84,20 +75,16 @@ function OneTransactionThreadRedirectHandler() {
     // check below, so gate on the count the server keeps on the report itself first.
     const [isParentOneTransactionReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${parentReportID}`, {selector: isOneTransactionReport});
 
-    // The same definition `HeaderView` and `SidebarUtils` use, so the redirect and the thread's own views agree.
     // Passing `undefined` keeps the call inert, so plain comment threads don't subscribe to their chat's action list.
     const isOneTransactionThread = useIsOneTransactionThread(isParentOneTransactionReport ? report : undefined);
 
-    // The prev/next arrows only exist in a thread's header, so the flows that seed a sibling set to step through
-    // expenses (Home "Recently added", "Review N flagged expenses", the duplicate review list) would dead-end
-    // mid-review if we redirected out of the thread.
+    // The prev/next arrows only exist in a thread's header, so redirecting out of a thread that is part of a sibling
+    // set (Home "Recently added", "Review N flagged expenses", the duplicate review list) would dead-end mid-review.
     const [isSteppingThroughExpenses] = useOnyx(ONYXKEYS.TRANSACTION_THREAD_NAVIGATION_TRANSACTION_IDS, {selector: selectIsSteppingThroughExpenses});
 
     // A message deep link points at an action inside the thread, so dropping the thread route would drop its anchor.
     const hasLinkedReportAction = !!route.params?.reportActionID;
 
-    // `referrer=notification` is what lets `useMarkAsRead` mark the report read without waiting on window focus, so it
-    // has to survive the redirect.
     const referrer = route.name === SCREENS.REPORT ? route.params?.referrer : undefined;
     const backTo = route.params?.backTo;
 
@@ -108,15 +95,12 @@ function OneTransactionThreadRedirectHandler() {
             return;
         }
 
-        // A thread opened from its own report carries that report as `backTo`. Handing it back to the report would
-        // point the report at itself, which `linkTo` refuses to navigate to at all on a wide layout, and which sends
-        // Back round in circles between the report's two route shapes on a narrow one. Inherit the report's own
-        // nested `backTo` instead - where it would have returned to had the user opened it directly.
+        // A thread opened from its own report carries that report as `backTo`, which would point the report at itself.
+        // Inherit that route's own nested `backTo` instead.
         const isBackToParentReport = !!backTo && getBackToReportID(backTo) === parentReportID;
         const resolvedBackTo = isBackToParentReport ? (getSearchParamFromPath(backTo ?? '', 'backTo') ?? undefined) : backTo;
 
-        // Replacing rather than pushing keeps the thread we are leaving out of the history, so going back doesn't
-        // land on it again.
+        // Replace rather than push, so going back doesn't land on the thread again.
         Navigation.navigate(getExpenseReportRoute({routeName: route.name, reportID: parentReportID, referrer, backTo: resolvedBackTo}), {forceReplace: true});
     }, [backTo, isFocused, parentReportID, referrer, route.name, shouldRedirectToParentReport]);
 

tests/navigation/OneTransactionThreadRedirectHandlerOnyxTest.tsx

@@ -28,7 +28,6 @@ jest.mock('@libs/Navigation/Navigation', () => ({
     },
 }));
 
-// Onyx, not layout, is what this suite pins down, and the inbox route below never reads the narrow-layout branch.
 jest.mock('@libs/getIsNarrowLayout', () => ({
     __esModule: true,
     default: () => false,
@@ -95,10 +94,7 @@ async function seedOnyx(transactionCount: number, actions: Array<ReportAction<ty
     });
 }
 
-/**
- * `OneTransactionThreadRedirectHandlerTest` mocks the hooks to pin down the decision table. This suite feeds the
- * handler real Onyx data, so it catches a wrong Onyx key or reportID that the mocked suite would pass.
- */
+/** Feeds the handler real Onyx data, catching a wrong Onyx key or reportID that the mocked suite would pass. */
 describe('OneTransactionThreadRedirectHandler with real Onyx data', () => {
     beforeAll(() => {
         Onyx.init({keys: ONYXKEYS});
@@ -131,7 +127,6 @@ describe('OneTransactionThreadRedirectHandler with real Onyx data', () => {
     });
 
     it('keeps the thread route while a multi-expense report is still paginating in', async () => {
-        // The server counter says three expenses, but only one IOU action has arrived so far.
         await seedOnyx(3, [createIOUAction('action1', 'transaction1', THREAD_REPORT_ID)]);
 
         render(<OneTransactionThreadRedirectHandler />, {wrapper: OnyxListItemProvider});

tests/navigation/OneTransactionThreadRedirectHandlerTest.tsx

@@ -18,8 +18,7 @@ const EXPENSE_REPORT_ID = '54321';
 const TRANSACTION_ID = '11111';
 const SIBLING_TRANSACTION_ID = '22222';
 
-// Inlined because a `jest.mock` factory may only close over locals initialized with a literal. A test below asserts
-// the two stay in sync.
+// Inlined because a `jest.mock` factory may only close over locals initialized with a literal.
 const TRANSACTION_THREAD_NAVIGATION_TRANSACTION_IDS_KEY = 'transactionThreadNavigationTransactionIDs';
 
 const mockNavigate = jest.fn();
@@ -117,8 +116,6 @@ describe('OneTransactionThreadRedirectHandler', () => {
     });
 
     it('keeps the thread route while the prev/next carousel is stepping through a sibling set', async () => {
-        // Home "Recently added", "Review N flagged expenses" and the duplicate review list open a thread precisely for
-        // the prev/next arrows, which only exist in the thread's header. Redirecting would dead-end them mid-review.
         mockSiblingTransactionIDs = [TRANSACTION_ID, SIBLING_TRANSACTION_ID];
 
         render(<OneTransactionThreadRedirectHandler />);
@@ -158,7 +155,6 @@ describe('OneTransactionThreadRedirectHandler', () => {
     });
 
     it('still redirects when the parent report only loads after the thread has mounted', async () => {
-        // A cold open: nothing about the parent is in Onyx yet.
         mockParentTransactionCount = undefined;
 
         const {rerender} = render(<OneTransactionThreadRedirectHandler />);
@@ -204,7 +200,6 @@ describe('OneTransactionThreadRedirectHandler', () => {
     });
 
     it('stays in the RHP when redirecting from the search RHP outside Search', async () => {
-        // Swapping one RHP route for another keeps the redirect inside the navigator it started in, on every layout.
         mockRouteName = SCREENS.RIGHT_MODAL.SEARCH_REPORT;
 
         render(<OneTransactionThreadRedirectHandler />);
@@ -223,8 +218,6 @@ describe('OneTransactionThreadRedirectHandler', () => {
     });
 
     it('inherits the nested backTo when the route came from the parent report itself', async () => {
-        // A thread opened from its own report carries that report as `backTo`. Keeping it would leave the report
-        // pointing at itself, which makes `linkTo` refuse to navigate at all.
         mockRouteParams = {reportID: THREAD_REPORT_ID, backTo: `/r/${EXPENSE_REPORT_ID}?backTo=${encodeURIComponent('/search')}`};
 
         render(<OneTransactionThreadRedirectHandler />);
@@ -252,7 +245,6 @@ describe('OneTransactionThreadRedirectHandler', () => {
     });
 
     it('drops a parent backTo anchored at one of the report actions', async () => {
-        // `/r/<parent>/<actionID>` still resolves to the parent, so it is a self-reference like the bare path is.
         mockRouteParams = {reportID: THREAD_REPORT_ID, backTo: `/r/${EXPENSE_REPORT_ID}/9999`};
 
         render(<OneTransactionThreadRedirectHandler />);
@@ -262,8 +254,6 @@ describe('OneTransactionThreadRedirectHandler', () => {
     });
 
     it('drops a parent backTo that reaches the report through another route shape', async () => {
-        // The same report is reachable as `r/`, `e/`, `search/view/` and `search/r/`, and all of them resolve to the
-        // same reportID, so none of them may be handed back to the report.
         mockRouteParams = {reportID: THREAD_REPORT_ID, backTo: `/e/${EXPENSE_REPORT_ID}`};
 
         render(<OneTransactionThreadRedirectHandler />);