Skip to content

Bind category and tag dynamic routes to the screen that builds them - #100501

Merged
roryabraham merged 12 commits into
Expensify:mainfrom
mukhrr:fix/100436
Sep 16, 2026
Merged

roryabraham merged 12 commits into
Expensify:mainfrom
mukhrr:fix/100436

Conversation

@mukhrr

@mukhrr mukhrr commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

The category and tag pages built their navigation targets at press time with a bare createDynamicRoute, which resolves the suffix against whatever route is active at that instant. A second press landing before the first navigation settles reads the route the first press just opened as its base and appends the suffix again, and the doubled path falls through to the not-found wildcard. Three sibling screens shared the pattern, so all four now use useScreenBoundDynamicRoute, which latches the base to the screen that owns the controls.

That hook needed one fix: its focus effect ignored an empty active route on cold start but not a bare /, which stripped the workspace prefix from every route built from it.

Also fixed all 5 cases found here

Fixed Issues

$ #100436
PROPOSAL: #100436 (comment)

Tests

Precondition: an admin on a workspace with Categories and Tags enabled, at least one tag, and a narrow window (the second press only reaches the row at mWeb width). The two taps must land within about 80ms of each other.

  1. Open the workspace and go to Categories.
  2. Double tap a category row fast.
  3. Verify the category settings page opens and the not-found page does not appear.
  4. Open that category's settings and double tap the Name row fast.
  5. Verify the name editor opens.
  6. Go to Tags and double tap a tag row fast.
  7. Verify the tag settings page opens.
  8. Open that tag's settings and double tap the Name row fast.
  9. Verify the name editor opens.
  10. Single tap a category row, a tag row, Add category and Add tag, and verify each opens its own page.
  • Verify that no errors appear in the JS console

Offline tests

None of these routes make a request, so the behavior is identical offline. Repeat steps 1 to 9 with the network turned off and verify each page still opens and the not-found page does not appear.

QA Steps

Same as tests.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
android_app.mp4
Android: mWeb Chrome
android-web.mp4
iOS: Native
ios.app.mp4
iOS: mWeb Safari
IOS_mweb.mp4
MacOS: Chrome / Safari
web.mp4

WorkspaceCategoriesPage, WorkspaceTagsPage, CategorySettingsPage and
DynamicTagSettingsPage all built their navigation targets at press time with a
bare createDynamicRoute, which resolves the suffix against whatever route is
active at that instant. A second press landing before the first navigation
settles reads the route the first press just opened as its base, so the suffix
is appended twice and the doubled path fails entryScreens validation and falls
through to the not-found wildcard.

Use useScreenBoundDynamicRoute on all four pages so the base is latched to the
screen that owns the controls. Both presses then build the same path, and an
identical destination is already dropped before dispatch.

The hook latched a bare "/" when its focus effect ran before a deep link had
resolved, which stripped the workspace prefix from every route built from it.
Treat that the same as the empty route the guard already rejected.
@mukhrr
mukhrr requested review from a team as code owners September 7, 2026 11:42
@melvin-bot
melvin-bot Bot requested review from dmkt9 and dylanexpensify and removed request for a team September 7, 2026 11:42
@melvin-bot

melvin-bot Bot commented Sep 7, 2026

Copy link
Copy Markdown

@dmkt9 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]

@melvin-bot
melvin-bot Bot removed the request for review from a team September 7, 2026 11:42
@codecov

codecov Bot commented Sep 7, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
src/hooks/useScreenBoundDynamicRoute.ts 100.00% <100.00%> (ø)
src/pages/workspace/WorkspaceOverviewPage.tsx 0.00% <0.00%> (ø)
...s/workspace/categories/WorkspaceCategoriesPage.tsx 78.49% <33.33%> (+1.19%) ⬆️
...rc/pages/workspace/tags/DynamicTagSettingsPage.tsx 0.00% <0.00%> (ø)
src/pages/workspace/tags/WorkspaceTagsPage.tsx 66.03% <25.00%> (+0.13%) ⬆️
...ages/workspace/categories/CategorySettingsPage.tsx 0.00% <0.00%> (ø)
... and 15 files with indirect coverage changes

dylanexpensify
dylanexpensify previously approved these changes Sep 7, 2026

@dylanexpensify dylanexpensify left a comment

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.

Looks good from a product POV

roryabraham
roryabraham previously approved these changes Sep 7, 2026
@dmkt9

dmkt9 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Bug: Not Found page appears when clicking the Name field of a category or tag

2026-09-08.08-08-52.mp4

The category and tag settings pages are pushed dynamic screens, so neither the
active route nor a focus-latched base holds once the first press navigates away.
Compose each page's own route from the params it already has and use that as the
base, so a second press produces the same path as the first.
@mukhrr
mukhrr dismissed stale reviews from dylanexpensify and roryabraham via 054bbd1 September 8, 2026 05:36
Company address and Plan type built their targets from the active route at press
time, so a second press appended the suffix onto the first destination and
landed on the not-found page.
roryabraham
roryabraham previously approved these changes Sep 8, 2026

@roryabraham roryabraham left a comment

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.

LGTM, though the distinction between buildDynamicRoute and createDynamicRoute isn't immediately clear from the names. We should consider a separate PR to rename them with more descriptive names.

@dmkt9

dmkt9 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Bug: Not Found page appears when clicking the Name field of a category or tag

2026-09-08.08-08-52.mp4

@mukhrr Can you explain what caused this bug?

@mukhrr

mukhrr commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Bug: Not Found page appears when clicking the Name field of a category or tag
2026-09-08.08-08-52.mp4

@mukhrr Can you explain what caused this bug?

@dmkt9 yes, I used buildDynamicRoute. so binding the route to the screen didn't hold on the category and tag settings pages, they're pushed dynamic screens themselves, so the base came out as /category/<name> with no workspace prefix, and the suffix fails entryScreens validation and falls to not-found:

const focusedRoute = findFocusedRouteWithOnyxTabGuard(getStateFromPath(pathWithoutDynamicSuffix) ?? {});
const entryScreens: ReadonlyArray<Screen | '*'> = DYNAMIC_ROUTES[dynamicRoute as DynamicRouteKey]?.entryScreens ?? [];
if (focusedRoute?.name && entryScreens.some((s) => s === '*' || s === focusedRoute.name)) {
// Merge the base route's params with params extracted from the dynamic suffix.
// This gives the dynamic route screen access to all context it needs.
const mergedParams = {
...(focusedRoute?.params as Record<string, unknown> | undefined),
...pathParams,
};
return getStateForDynamicRoute(normalizedPath, dynamicRoute as DynamicRouteKey, mergedParams, strippedTabPath);
}

@dmkt9

dmkt9 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

@dmkt9 yes, I used buildDynamicRoute. so binding the route to the screen didn't hold on the category and tag settings pages, they're pushed dynamic screens themselves, so the base came out as /category/<name> with no workspace prefix, and the suffix fails entryScreens validation and falls to not-found:

@mukhrr Thanks. So Navigation.getActiveRoute doesn't work correctly, and the bug comes from useScreenBoundDynamicRoute. But it only appears when opening the RHP for the first time. If we go back from the Not Found page, the bug goes away. So we should fix the bug in useScreenBoundDynamicRoute instead, right?

@mukhrr

mukhrr commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

hmm what you say makes sense. let me check if we don't regress anything

A screen pushed into the RHP focuses while the navigation state is still
mid-transition, where getActiveRoute reports a dynamic screen as its suffix
alone, so the latched base dropped the workspace prefix. With the matched path
first, the category and tag settings pages take buildDynamicRoute back.
@mukhrr

mukhrr commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@dmkt9 mind checking once more pls? thanks

@dmkt9

dmkt9 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

@mukhrr Conflicts

@dmkt9

dmkt9 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ffac8fe671

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/hooks/useScreenBoundDynamicRoute.ts Resolved
# Conflicts:
#	src/pages/workspace/categories/CategorySettingsPage.tsx
@dmkt9

dmkt9 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

@MelvinBot review the changes and identify all potential bugs

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: aa51e14398

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@MelvinBot

Copy link
Copy Markdown
Contributor

Review: potential bugs

Three things in the hook change look wrong to me. The page conversions themselves are mechanical and read fine (all createDynamicRoute call sites were converted and both imports removed).


1. The latched base goes stale when a settings page rewrites its own URL — likely reintroduces the Not Found you reported

Both settings pages this PR converts rewrite their own URL in place after a rename, with no blur/focus cycle:

useScreenBoundDynamicRoute latches focusedBasePath once per focus. navigation.setParams keeps the same route key, so no focus event fires and nothing re-latches. Rename Food → Dining and the base stays …/categories/category/Food while the real URL is …/category/Dining. Every subsequent row on that page (Name, GL code, Approver, Default tax rate, Flag amounts over, Required fields, Description hint) then builds …/category/Food/<suffix> → Not Found. Before this PR those rows resolved against the live route at press time, so they were always correct.

Repro to run: open a category's settings → rename it → without leaving the page, tap GL code (or Name again).

Whether this actually bites depends on React Compiler's dependency granularity for the un-memoized useFocusEffect callback (route vs route?.key) — correctness shouldn't hinge on that either way. Re-latch when the screen's route changes.

Why the effect may or may not re-run

useFocusEffect re-runs only when the callback identity changes (React.useEffect(..., [effect, navigation]) in @react-navigation/core). Your stays bound when the screen re-renders test only passes because React Compiler memoizes the inline callback — so the memo block's dependency decides this. If it's keyed on route?.key, setParams won't invalidate it and the stale base survives.


2. latch() gives up permanently instead of waiting, in exactly the cold-start case the comment calls out

useScreenBoundDynamicRoute.ts:36

if (!key || !navigationRef.isReady() || isRouteInState(navigationRef.getRootState(), key)) {
    latch();
    return;
}

When !isReady(), this takes the early-out: latch() reads getActiveRoute() → '' → bails → no listener is registered and there is no retry (the callback is memoized, so the effect won't re-run). The comment says isReady is only there "to keep getRootState quiet on an unattached ref", but placing it in this condition also skips the wait-for-state path precisely when the container isn't ready yet.

isReady is only needed to guard getRootState — createNavigationContainerRef().addListener queues callbacks on an unattached ref and attaches them when current is set, so subscribing early is safe. Suggest splitting the condition:

if (!key) {
    latch();
    return;
}
if (navigationRef.isReady() && isRouteInState(navigationRef.getRootState(), key)) {
    latch();
    return;
}
// subscribe and wait

3. activeRoute === '/' is a symptom-specific patch, and bailing out of latch() drops the retry

useScreenBoundDynamicRoute.ts:27

Your own comment says the unsettled root state renders the screen "as the bare root or its dynamic suffix alone". The guard only rejects '' and '/'. If getActiveRoute() returns the suffix alone (/category/Food) while isRouteInState is already true, that gets latched and every link off the page doubles — the same failure shape as the original bug, just with a different bad value.

Separately, when latch() does reject a value it returns without subscribing, so the hook stays unlatched for the whole focus session. A rejected value should fall through to the addListener path and retry rather than give up.

A positive check ("the active route is this screen's route") would cover '', '/', and suffix-only in one rule instead of enumerating bad values. Note navigationRef.getCurrentRoute()?.key === key won't work for the consumers where the hook is called from a component under a screen that owns a nested navigator, so it needs a bit of care — useRootNavigationState, which useDynamicBackPath already uses, may be a cleaner source than the hand-rolled addListener.


Minor

  • tests/ui/WorkspaceCategoriesTest.tsx:126 — afterEach calls jest.clearAllMocks(), which clears call records but not implementations. The new test's Navigation.getActiveRoute/Navigation.navigate stubs survive into every later test in the file. It's the last test today, so it's green — but the next test appended here silently gets a no-op Navigation.navigate. Use jest.restoreAllMocks().
  • The new hook test stubs appNavigationRef.addListener to return () => {}, so the real listener path is never exercised — including that unsubscribe gets called twice (once inside the listener, once from the focus-effect cleanup).
  • Pre-existing, but you touched the line: the payroll-code upgrade branch in CategorySettingsPage.tsx ignores isQuickSettingsFlow while the GL-code branch right above it honours it.

This is a static review — I couldn't run the app or the Jest suite in this environment, so finding 1 is a repro to try rather than a confirmed failure. Findings 2 and 3 are readable straight off the control flow.

Next Steps: Reply with @MelvinBot <your feedback> to refine this review.

@mukhrr

mukhrr commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor Author

Review: potential bugs

Three things in the hook change look wrong to me. The page conversions themselves are mechanical and read fine (all createDynamicRoute call sites were converted and both imports removed).

1. The latched base goes stale when a settings page rewrites its own URL — likely reintroduces the Not Found you reported

Both settings pages this PR converts rewrite their own URL in place after a rename, with no blur/focus cycle:

useScreenBoundDynamicRoute latches focusedBasePath once per focus. navigation.setParams keeps the same route key, so no focus event fires and nothing re-latches. Rename Food → Dining and the base stays …/categories/category/Food while the real URL is …/category/Dining. Every subsequent row on that page (Name, GL code, Approver, Default tax rate, Flag amounts over, Required fields, Description hint) then builds …/category/Food/<suffix> → Not Found. Before this PR those rows resolved against the live route at press time, so they were always correct.

Repro to run: open a category's settings → rename it → without leaving the page, tap GL code (or Name again).

Whether this actually bites depends on React Compiler's dependency granularity for the un-memoized useFocusEffect callback (route vs route?.key) — correctness shouldn't hinge on that either way. Re-latch when the screen's route changes.

Why the effect may or may not re-run
useFocusEffect re-runs only when the callback identity changes (React.useEffect(..., [effect, navigation]) in @react-navigation/core). Your stays bound when the screen re-renders test only passes because React Compiler memoizes the inline callback — so the memo block's dependency decides this. If it's keyed on route?.key, setParams won't invalidate it and the stale base survives.

2. latch() gives up permanently instead of waiting, in exactly the cold-start case the comment calls out

useScreenBoundDynamicRoute.ts:36

if (!key || !navigationRef.isReady() || isRouteInState(navigationRef.getRootState(), key)) {
    latch();
    return;
}

When !isReady(), this takes the early-out: latch() reads getActiveRoute() → '' → bails → no listener is registered and there is no retry (the callback is memoized, so the effect won't re-run). The comment says isReady is only there "to keep getRootState quiet on an unattached ref", but placing it in this condition also skips the wait-for-state path precisely when the container isn't ready yet.

isReady is only needed to guard getRootState — createNavigationContainerRef().addListener queues callbacks on an unattached ref and attaches them when current is set, so subscribing early is safe. Suggest splitting the condition:

if (!key) {
    latch();
    return;
}
if (navigationRef.isReady() && isRouteInState(navigationRef.getRootState(), key)) {
    latch();
    return;
}
// subscribe and wait

3. activeRoute === '/' is a symptom-specific patch, and bailing out of latch() drops the retry

useScreenBoundDynamicRoute.ts:27

Your own comment says the unsettled root state renders the screen "as the bare root or its dynamic suffix alone". The guard only rejects '' and '/'. If getActiveRoute() returns the suffix alone (/category/Food) while isRouteInState is already true, that gets latched and every link off the page doubles — the same failure shape as the original bug, just with a different bad value.

Separately, when latch() does reject a value it returns without subscribing, so the hook stays unlatched for the whole focus session. A rejected value should fall through to the addListener path and retry rather than give up.

A positive check ("the active route is this screen's route") would cover '', '/', and suffix-only in one rule instead of enumerating bad values. Note navigationRef.getCurrentRoute()?.key === key won't work for the consumers where the hook is called from a component under a screen that owns a nested navigator, so it needs a bit of care — useRootNavigationState, which useDynamicBackPath already uses, may be a cleaner source than the hand-rolled addListener.

Minor

  • tests/ui/WorkspaceCategoriesTest.tsx:126 — afterEach calls jest.clearAllMocks(), which clears call records but not implementations. The new test's Navigation.getActiveRoute/Navigation.navigate stubs survive into every later test in the file. It's the last test today, so it's green — but the next test appended here silently gets a no-op Navigation.navigate. Use jest.restoreAllMocks().
  • The new hook test stubs appNavigationRef.addListener to return () => {}, so the real listener path is never exercised — including that unsubscribe gets called twice (once inside the listener, once from the focus-effect cleanup).
  • Pre-existing, but you touched the line: the payroll-code upgrade branch in CategorySettingsPage.tsx ignores isQuickSettingsFlow while the GL-code branch right above it honours it.

This is a static review — I couldn't run the app or the Jest suite in this environment, so finding 1 is a repro to try rather than a confirmed failure. Findings 2 and 3 are readable straight off the control flow.

Next Steps: Reply with @MelvinBot <your feedback> to refine this review.

i don't think these are valid bugs. i checked cc @dmkt9

@dmkt9

dmkt9 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

i don't think these are valid bugs. i checked cc @dmkt9

@mukhrr The second bug is valid. The third bug will make our code more robust

@mukhrr

mukhrr commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

@mukhrr The second bug is valid.

@dmkt9 can you tell me more about this? i mean I am not able to trigger that early-out. useFocusEffect runs from a useEffect, after the container ref is attached, and on a cold deep link the first focus effect already saw isReady() true with the key missing, then latched 33ms later on the state event. an evven if it did fire, getActiveRoute returns '' there and route.path still seeds the base. do you have a flow where the focus effect runs before the container is ready???

# Conflicts:
#	src/pages/workspace/categories/CategorySettingsPage.tsx
#	src/pages/workspace/categories/WorkspaceCategoriesPage.tsx
@dmkt9

dmkt9 commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

@mukhrr The second bug is valid.

@dmkt9 can you tell me more about this? i mean I am not able to trigger that early-out. useFocusEffect runs from a useEffect, after the container ref is attached, and on a cold deep link the first focus effect already saw isReady() true with the key missing, then latched 33ms later on the state event. an evven if it did fire, getActiveRoute returns '' there and route.path still seeds the base. do you have a flow where the focus effect runs before the container is ready???

@mukhrr I'm not sure what you tested. You can add some console.log statements and reload the app with this deep link to verify it: https://dev.new.expensify.com:8082/workspaces/:policyID:/categories/category/Advertising

route.path still seeds the base

I know that. But let's make it more robust if we can

A screen that focuses before the container is ready, or before the root
state carries its navigator, never got a base, and a rename changed the
route in place without a re-latch. Listen for state events for the whole
focus session and latch whenever the route is in the root state.
@mukhrr

mukhrr commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

@dmkt9 done

@dmkt9

dmkt9 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: a637e88b21

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@dmkt9

dmkt9 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
chrome.mp4

@dmkt9 dmkt9 left a comment

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.

LGTM

@melvin-bot

melvin-bot Bot commented Sep 14, 2026

Copy link
Copy Markdown

We did not find an internal engineer to review this PR, trying to assign a random engineer to #100436 as well as to this PR... Please reach out for help on Slack if no one gets assigned!

@melvin-bot
melvin-bot Bot requested a review from puneetlath September 14, 2026 03:47
@roryabraham
roryabraham merged commit 23485d9 into Expensify:main Sep 16, 2026
35 of 37 checks passed
@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions

Copy link
Copy Markdown
Contributor

🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.81-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 No help site changes are required for this PR. I did not create a docs PR.

Why: Every change here is internal navigation plumbing. The PR swaps createDynamicRoute for the useScreenBoundDynamicRoute hook on four screens and fixes that hook's focus effect. It adds no feature, changes no URL, and touches no user-facing copy, label, tab, or button.

What I checked

Files changed (6 source files, 2 test files):

In the four page files, every diff line is the same mechanical swap: createDynamicRoute(...) → buildDynamicRoute(...), plus the matching import and useCallback/useMemo dependency updates. The route constants in DYNAMIC_ROUTES are unchanged, so no documented URL moves.

Criteria that would have triggered a docs update, and what I found:

Check Result
New or renamed feature, setting, tab, or button None
Changed user-facing copy or labels None
Changed steps a user takes to reach Categories, Tags, or Workspace settings None — the taps and pages are identical
Changed public URL that an article links to None

Help site scan: I searched docs/articles for references to the affected workspace category and tag routes and found no matches, so no existing article documents a path this PR touches.

The only user-visible difference is that a fast double tap on a category or tag row no longer lands on the not-found page. That is a bug fix restoring documented behavior, not new behavior to document.


Since no help site changes are needed, there is no linked docs PR to review or mark Ready for review. mukhrr, if you believe a specific article does need updating here, reply with @MelvinBot and the article, and I'll draft the PR.


view run

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.81-1 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 failure ❌
🍎 iOS 🍎 failure ❌

1 similar comment
@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.81-1 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 failure ❌
🍎 iOS 🍎 failure ❌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants