Repository navigation
Fix: Concierge Anywhere can act on the viewed report on native (source report threading) - #99337
Conversation
…ed report Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
… oxfmt import grouping Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
Fixed the two real failing checks and pushed to the branch (
I wasn't able to run Jest or oxfmt locally in this environment (tool execution is sandboxed here), so CI on the new commit is the confirmation — but both fixes map directly to the exact failure output. Changes
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
Found the remaining Oxfmt failure and pushed a fix ( Root cause: the My earlier commit already handled the other two failures — the I still can't run oxfmt/Jest locally in this environment, so CI on |
|
No code fix needed — the previously failing checks are now green on the latest commit (
The only remaining red check is |
…-params and no-default-id-values disables Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
The earlier oxfmt and Jest fixes are now green. The remaining real failure was
I couldn't run ESLint locally in this environment (tool execution is sandboxed), so CI on the new commit is the confirmation, but each fix maps directly to the reported rule and location. |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
…e import in useSidePanelContext test Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
Fixed the A follow-up commit ( Replaced it with the repo's standard pattern — a top-level The earlier I couldn't run ESLint locally in this sandboxed environment, so CI on the new commit is the confirmation — but the change is a like-for-like swap to an already-lint-passing pattern. |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
All the real checks are green now — my earlier fix resolved both failures:
The only remaining red check is Nothing further to push here. |
|
🤖 Web test result: could not exercise the live flow (environment limitation), no web regression by inspection I attempted the PR's testing steps on Web, adapted to the web equivalent (Concierge Anywhere is opened via the Side Panel / Help button, which builds context from the report you're viewing). I got the fixture set up but could not open the Side Panel on web, so the send/verify steps couldn't run live. Why the live flow was blocked: the Side Panel (and thus web Concierge Anywhere) is gated behind onboarding completion. In Note this PR is fundamentally a native fix — the web path is explicitly unchanged — so a proper live verification really wants iOS/Android native (the What I verified:
Code-level regression check (web path)The PR scopes all new behavior to the native (
So the web flow logic is untouched — no web regression expected. Recommendation: live web verification of this side-panel flow requires a session where onboarding is actually completed ( ScreenshotsExpense Report with two transactions open on webReport top bar (1440x900) — no Concierge/Help iconSame report (1920x1080) — still no Concierge/Help iconSettings > Help page with Concierge menu item (alternate entry point)After clicking Concierge in Help page — no side panel opened |
|
@MelvinBot resolve the conflicts |
…eSourceReport Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com> # Conflicts: # src/libs/actions/Report/index.ts
|
Conflicts resolved and pushed — the PR is now mergeable. One conflicted file: Resolution detailsSignature — took personalDetails?: OnyxEntry<PersonalDetailsList>,
sourceReportID?: string,
): Promise<void> {Tail — took return Promise.resolve();
}
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(conciergeReportID, undefined, undefined, undefined, undefined, sourceReportID), linkToOptions);
return Promise.resolve();The other four files ( Verified:
Every other call site of The remaining |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d7bb1f259
ℹ️ 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".
| // Capture the report the user is viewing (still topmost at press time) so Concierge can act on it | ||
| // after we navigate away. This is the ONLY entry that threads a source report, so context is | ||
| // scoped to Concierge opened via this sidebar button — search/LHN/deep links never carry it. | ||
| const sourceReportID = Navigation.getTopmostReportId(); |
There was a problem hiding this comment.
When Help is pressed outside the Inbox report view, such as from SearchPageHeaderNarrow/Wide or a settings/workspace TopBar, this same button still runs, while getTopmostReportId() scans the inactive Reports split navigator and returns its last-opened report. That stale ID is then sent as Concierge context, so a prompt opened from Search or settings can inspect or modify an unrelated report; only attach the source when the Reports split navigator is currently active (for example, using the already-gated central-pane helper).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot Please verify this comment. I think this behavior occurs on web as well. If so, I don't think this is a bug
There was a problem hiding this comment.
Verified — dmkt9 is right. Web already behaves this way, so this isn't a bug this PR introduces.
Web's side panel context resolves to currentRHPReportID ?? currentReportID (useSidePanelContext.ts:43), and currentReportID is set from Navigation.getTopmostReportId(state) — the same ungated helper the native button calls. Opening Help from Search or settings on web sends the same last-opened report.
Evidence
1. The helper is not gated on the active navigator. getTopmostReportParams.ts:24-35 uses findLast to grab the last REPORTS_SPLIT_NAVIGATOR anywhere in the state and its last REPORT screen. It never checks whether that navigator is focused. Both platforms inherit this.
2. Web deliberately keeps the stale ID on settings. useCurrentReportID.tsx:69-75 early-returns for any Settings_ screen, with the comment "Make sure we don't make the reportID undefined when switching between the chat list and settings tab." So on web, navigating to settings preserves the previous report as the side panel's context by design — exactly the behavior flagged here.
3. Same button placement on both. TopBar.tsx:87 renders SidePanelButton for both platforms, and the only gate is shouldHideHelpButton (onboarding / anonymous / 2FA), which has nothing to do with which navigator is active.
On the suggested "already-gated central-pane helper" — that's getFocusedReportId / getFocusedReportParams, which does respect the active tab via getActiveTabRoute. Web's side panel does not use it. Switching only native to it would make native stricter than web, not consistent with it.
One genuine difference (doesn't change the verdict): native freezes sourceReportID into the route param at press time, while web re-reads currentReportID live. At the moment Help is pressed from Search or settings both resolve to the same report, so the scenario described here is identical on both.
If the ungated lookup is worth tightening, it should change both platforms together in its own PR rather than only the native path here.
| personalDetails?: OnyxEntry<PersonalDetailsList>, | ||
| // The report the user was viewing when they opened Concierge from the side-pane button (native). Threaded onto | ||
| // the Concierge route so the composer can attach it as sidePanelContext, scoped to this navigation entry. | ||
| sourceReportID?: string, |
There was a problem hiding this comment.
When CONCIERGE_REPORT_ID is absent, the function takes the earlier navigateToAndOpenReport() branch and returns without ever using this new sourceReportID parameter; only the existing-report branch forwards it to the route. Consequently, first-time users or clients with an uncached Concierge ID still open Concierge without the viewed report context, leaving the reported native failure unfixed for that path; thread the parameter through the create/open navigation as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 925d0c8.
sourceReportID is now threaded through the create/open branch too, so a first-time user (or any client with no cached CONCIERGE_REPORT_ID) lands on a Concierge route carrying the viewed report.
What changed
The parameter had to travel two more hops to reach the route, since the create path navigates via navigateToAndOpenReport → navigateToReport rather than calling Navigation.navigate directly:
navigateToConciergeChat— passessourceReportIDinto thenavigateToAndOpenReportcall.navigateToAndOpenReport— new optionalsourceReportIDparam, forwarded to all three of itsnavigateToReportcalls (new optimistic chat, existing chat, and the revalidated-chat path), so the Concierge chat gets the context whether it is created or found.navigateToReport— new optionalsourceReportIDoption, passed toROUTES.REPORT_WITH_ID.getRoute. It defaults toundefined, so every other caller is unaffected.
One follow-on this exposed: the native side-panel button drops a self-referencing source report with sourceReportID !== conciergeReportID, but that guard only works when CONCIERGE_REPORT_ID is cached — which is exactly what this path lacks. So useSidePanelContext now also drops it when sourceReportID === reportID (reportID is the Concierge report by the check above it), with a unit test covering it.
Verified: typecheck, lint-changed, cspell, and React Compiler compliance all pass. useSidePanelContext tests 17/17; ReportTest plus the four other suites touching these helpers 340/340.
Not verified on a device — the reported failure is native-only and I can only drive web here, so please confirm on iOS/Android with a fresh account (no cached Concierge report) when you run the test steps.
navigateToConciergeChat only forwarded sourceReportID on the branch where CONCIERGE_REPORT_ID was already cached. First-time users, and any client with an uncached Concierge ID, took the navigateToAndOpenReport branch and opened Concierge with no source report, leaving the native failure unfixed there. Thread sourceReportID through navigateToAndOpenReport and navigateToReport so every path lands on a route carrying the viewed report. Also guard against a self-referencing source report in useSidePanelContext: the side-panel button can only drop one when CONCIERGE_REPORT_ID is known, which is exactly what this path lacks. Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 925d0c8792
ℹ️ 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".
| if (conciergeReportID !== reportID) { | ||
| return undefined; |
There was a problem hiding this comment.
When Concierge Anywhere is opened while CONCIERGE_REPORT_ID is still absent—such as a first-time open on a slow or offline connection—the create/open path now correctly puts sourceReportID on the optimistic Concierge route, but this guard still rejects it because conciergeReportID is undefined. The freshly threaded parameter therefore produces no sidePanelContext, and a message sent before the server populates the Concierge key still cannot act on the viewed report. This is fresh evidence beyond the earlier resolved comment: the navigation path was updated, but the consumer continues to require the missing cache key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Invalid, as CONCIERGE_REPORT_ID is already included in the OpenApp API response
|
@MelvinBot review |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
@MelvinBot merge the latest main branch |
|
Merged the latest There were 4 conflicts, all from
Checks run locally
The last two suites matter most here: One caveat: I could not verify this in a browser — web prep failed in this run, so there is no NewDot session to drive. The resolution is validated by typecheck, lint, and the unit suites above only. Since this PR's behavior change is native-only anyway, it is still worth re-running the Android test steps before merge. |
|
@MelvinBot fix the Oxfmt CI failure |
|
Fixed — Oxfmt now passes (run). The cause was mine: when I resolved the Heads up:
|
|
@MelvinBot merge the latest main branch |
…eSourceReport Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
Merged the latest The branch was 66 commits behind. The previous merge's conflict resolutions all survived cleanly: the Checks run locally — all passed
The web regression check did not run. I tried to verify on web that the side-panel Concierge flow is unaffected, but the attempt was blocked by the test harness before any step executed, so I have no browser evidence either way. The cause is a side effect of this very merge: So this merge is verified by typecheck, lint, formatting, and the unit suites only. Since the behavior change in this PR is native-only, the Android test steps in the PR description are the ones worth re-running before merge. |
|
@pecanoro all yours |
|
@dmkt9 We need to resolve the conflicts again 😞 |
|
@MelvinBot resolve conflicts |
|
🤖 Conflicts resolved and pushed — the PR is mergeable again ( One file conflicted:
Result: // eslint-disable-next-line @typescript-eslint/no-unused-vars
const openConciergeAnywhere = (_options?: {forceConcierge?: boolean; reportID?: string}) => {
navigateToConciergeChat({conciergeReportID, introSelected, currentUserAccountID, isSelfTourViewed, betas});
};
VerificationMerged with
I also confirmed every call site of I did not run the PR's manual test steps — they're iOS/Android native only, and this run is scoped to web. Please re-verify steps 1–4 on a native build before merging, since |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@pecanoro This PR is ready to go again |
|
🚧 pecanoro 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 9.4.80-0 🚀
|
|
Yes — help site changes are required. Draft PR: Docs updates for E/A#99337 Concierge Help icon report context ( Why a docs change is neededThis PR makes the Help icon in a report header carry the report you were viewing into Concierge on native, matching what the side panel already did on web ( I also found a stale UI label while verifying: What changedThree articles in
Conventions applied and scope notes
Typography — matched each file's existing curly quotes Left alone: the mobile Account > Help > Concierge path and the Ask Concierge search entry. This run could only verify the web UI, so re-verifying those was out of scope. Checks: Markdown-only, so the AI test matrix selects spell check alone. @dmkt9, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.80-0 🚀
Bundle Size Analysis (Sentry): |
5 participants
Explanation of Change
On native there is no side panel, so Concierge Anywhere (opened via the sidebar Concierge button) never attached the report the user was viewing to the message it sends. The composer only built
sidePanelContextwhenisInSidePanelwas true (web-only), so on iOS/Android Concierge received no source report and replied that the chat/report "doesn't have any expenses on it right now."This threads the source report through the Concierge navigation entry on native:
Navigation.getTopmostReportId()at press time and passes it as a new optionalsourceReportIDroute param (dropped when it equals the Concierge report, so tapping from within Concierge never self-references).useSidePanelContextreads that route param on the native (!isInSidePanel) branch and returns{reportID: sourceReportID}, mirroring the{reportID}context the side panel builds on web.sidePanelContextbeing present rather than onisInSidePanel, so the native-originated context is attached and sent (the backend already consumessidePanelContext.reportIDfor the web flow).Because the source report is scoped to each Concierge navigation entry (a route param) rather than a global Onyx key, two Concierge entries in the same stack can't collide — each restores its own
sourceReportIDon back-nav. Web behavior is unchanged: it still builds context from the live side-panel state.Fixed Issues
$ #97945
PROPOSAL: #97945 (comment)
Tests
Update all transactions in this report to have category Car.Offline tests
N/A
QA Steps
Same as Tests
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari