Skip to content

Fix agent chat RHP background reverting to Inbox on refresh - #98589

Draft
MelvinBot wants to merge 3 commits into
mainfrom
claude-agentReportRhpBackground
Draft

Fix agent chat RHP background reverting to Inbox on refresh#98589
MelvinBot wants to merge 3 commits into
mainfrom
claude-agentReportRhpBackground

Conversation

@MelvinBot

Copy link
Copy Markdown
Contributor

Explanation of Change

PR #95631 added a new Right-Hand-Panel (RHP) screen AGENT_REPORT (route settings/agents/r/:reportID) so the newly created agent DM opens in a dedicated RHP over the Account > Agents settings page on wide layouts. The screen was wired into ROUTES, SCREENS, RightModalNavigator, and linkingConfig/config.ts, but it was not registered in any of the RHP → background relation tables under src/libs/Navigation/linkingConfig/RELATIONS/.

On refresh/deep-link, getMatchingFullScreenRoute reconstructs the central-pane screen behind an RHP by looking the RHP screen name up in those relation tables. With no relation entry, it returns undefined and control falls through to getDefaultFullScreenRoute. Because AGENT_REPORT carries a reportID param, the default builds a reports split navigator (Inbox + report) as the background instead of the Agents settings page — which is exactly the reported symptom: after refreshing while the agent chat RHP is open, the background changes to Inbox.

This change registers SCREENS.RIGHT_MODAL.AGENT_REPORT under SCREENS.SETTINGS.AGENTS.ROOT in SETTINGS_TO_RHP.ts. That makes RHP_TO_SETTINGS[AGENT_REPORT] === SCREENS.SETTINGS.AGENTS.ROOT, so getMatchingFullScreenRoute rebuilds the Settings split with the Agents page as the central pane, keeping the background on Account > Agents after a refresh. A regression unit test is added to lock in this relation.

Fixed Issues

$ #97833
PROPOSAL:

Tests

  • Verify that no errors appear in the JS console

Offline tests

QA Steps

// TODO: These must be filled out, or the issue title must include "[No QA]."

  • 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

MelvinBot and others added 2 commits August 13, 2026 13:23
Co-authored-by: Eugene Voloshchak <eVoloshchak@users.noreply.github.com>
Co-authored-by: Eugene Voloshchak <eVoloshchak@users.noreply.github.com>
@MelvinBot

Copy link
Copy Markdown
Contributor Author

I looked into the failing checks — none are genuine code failures, so there's nothing to push a fix for.

What's actually failing:

  • checklist (PR Author Checklist) and Check independent approval — these are process gates (unchecked checklist items / no independent reviewer approval yet), not code problems. They're expected to be red until the human review steps happen.
  • The 8 test (job N) jobs show red, but only because the Jest run was canceled by mountiny mid-run (~11m in) — the annotation on every job is "The run was canceled." That's a manual cancellation, not a test failure.

I verified the change itself is sound. This PR adds SCREENS.RIGHT_MODAL.AGENT_REPORT under SCREENS.SETTINGS.AGENTS.ROOT in SETTINGS_TO_RHP.ts:83. Since RHP_TO_SETTINGS = createInverseRelation(SETTINGS_TO_RHP), this makes RHP_TO_SETTINGS['AgentReport'] === 'Settings_Agents', which is exactly what the new regression test asserts. Both screen constants (RIGHT_MODAL.AGENT_REPORT, SETTINGS.AGENTS.ROOT) exist in src/SCREENS.ts.

Recommendation: Re-run the canceled Jest workflow (or push any trivial update) to get a clean test run. There's no code fix needed from me here.

Co-authored-by: Eugene Voloshchak <eVoloshchak@users.noreply.github.com>
@MelvinBot

Copy link
Copy Markdown
Contributor Author

Re-triggered CI — there was no code defect to fix.

The red test (job 1-8) checks were all from the earlier Jest run that mountiny manually canceled ~11 min in ("The run was canceled" annotation on every job), not real test failures. Every check that actually executes code is green on this branch: typecheck, ESLint check, Bun tests, Storybook tests, Oxfmt, spellcheck, React Compiler Compliance. The only other reds are checklist and Check independent approval, which are process gates.

Since gh run rerun wasn't available to me, I pushed an empty commit (8223ac9) to kick off a fresh CI run. The new Jest run is queued now and should go green.

The PR change itself is verified correct: adding SCREENS.RIGHT_MODAL.AGENT_REPORT under SCREENS.SETTINGS.AGENTS.ROOT in SETTINGS_TO_RHP.ts:83 makes RHP_TO_SETTINGS['AgentReport'] === 'Settings_Agents' (via createInverseRelation), which is exactly what the new regression test asserts.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Update: a fresh CI run is now green — the earlier red test jobs were just a canceled run, not a real failure.

On the current run, every check that has completed passes: typecheck, ESLint check, Bun tests, React Compiler Compliance, Storybook tests, and test shards 2, 3, and 7. The remaining test shards (1, 4, 5, 6, 8) are still queued waiting on GitHub Actions runners — not failing.

The only red checks are checklist (PR Author Checklist) and Check independent approval, which are process gates (unchecked checklist items / no independent reviewer approval yet), not code problems.

There's no code fix needed from me. Once the queued shards get a runner they should pass too, and the two remaining reds resolve through the normal review/checklist steps.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

The one remaining failing check (test (job 1)) is unrelated to this PR — it's a pre-existing broken test, so I'm not pushing a change for it.

What's failing: tests/unit/SignInBackButtonTest.tsxSignInModal › registers no back listener that consumes the press without navigating, with:

TypeError: _Navigation.default.dismissModal is not a function
  at dismissModal (src/pages/signin/SignInModal.tsx:60)

Why it's unrelated: This PR only changes two files — SETTINGS_TO_RHP.ts (a static RHP→background relation table) and the new agentReportBackgroundRelationTests.ts. Neither SignInModal.tsx nor SignInBackButtonTest.tsx is touched, and a data-only relations entry can't affect the Navigation singleton's methods.

The failure is a test-mock gap that exists independently of this PR: the test mocks @libs/Navigation/Navigation with a default that only stubs goBack, but SignInModal's mount effect calls Navigation.dismissModal(). dismissModal is a real exported function (Navigation.ts:875) that the mock never provides, so the effect throws. That test came in via a separate PR (#98247), not this one.

Everything else is green — typecheck, ESLint, Bun tests, and the other 7 Jest shards all pass. The remaining checklist / Check independent approval reds are the usual human-review gates.

Recommendation: The SignInBackButtonTest mock needs dismissModal added — that's a fix for a separate PR (owner of #98247). To unblock this PR, merge the latest main once that mock is patched, or have it treated as a known unrelated failure.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

The one real failing check (test (job 1)) is unrelated to this PR — it's a pre-existing failure in the sign-in test suite, so I'm not pushing a change for it (per policy I report unrelated/flaky failures rather than "fixing" them inside an unrelated PR).

What's failing: tests/unit/SignInBackButtonTest.tsxSignInModal › registers no back listener that consumes the press without navigating, with:

TypeError: _Navigation.default.dismissModal is not a function
  at dismissModal (src/pages/signin/SignInModal.tsx:60:19)

Why it's unrelated: This PR only adds an entry to SETTINGS_TO_RHP.ts plus a navigation regression test — it doesn't touch the sign-in flow, SignInModal, or its test. The failing test's Navigation mock only stubs goBack, so when SignInModal's post-sign-in useEffect calls Navigation.dismissModal() the mock has no such method and throws. The mock and the SignInModal code are byte-identical on main, so this exists independently of this branch (merging main won't help). All other test shards (jobs 2–8) passed.

The remaining reds — checklist and Check independent approval — are process gates, not code.

Recommended next steps:

  • Re-run test (job 1) — if it's timing-dependent flakiness in that effect it may pass on retry.
  • If it's deterministic, it needs a separate fix on main adding dismissModal (and likely navigate) to that test's Navigation mock. Happy to open that as its own PR if you'd like — just say the word.

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.

2 participants