Repository navigation
[HOLD] Resolve optimistic agent edit route after creation - #96473
nabi-ebrahimi wants to merge 47 commits into
Conversation
|
@marufsharifi 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] |
trjExpensify
left a comment
There was a problem hiding this comment.
I can't reproduce this bug on staging.
2026-07-20_16-48-07.mp4
|
@trjExpensify, this issue is reproducible only under a slow network connection. @nabi-ebrahimi, could you please update the checklist to include the following precondition?
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-21.at.11.27.40.AM.movAndroid: mWeb ChromeScreen_Recording_20260721_113936_Chrome.mp4iOS: HybridAppScreen.Recording.2026-07-21.at.12.20.55.PM.moviOS: mWeb SafariScreen.Recording.2026-07-21.at.11.51.14.AM.movMacOS: Chrome / SafariScreen.Recording.2026-07-29.at.10.36.23.AM.mov |
trjExpensify
left a comment
There was a problem hiding this comment.
Thanks for updating the tests. 👍
|
@nabi-ebrahimi, could you please check this. thanks. RF-1.mp4 |
@marufsharifi thanks for the review. The current approach in this PR is to disable the pending row until the data comes back from the server, so the user cannot open the agent too early and land on the In the offline case, the newly created agent is still only a pending optimistic row and does not have a stable server identity yet, so the row stays unavailable until creation completes. What do you think about keeping this behavior for offline-created agents? Or would you prefer a different UX here? |
|
@trjExpensify, could you please check this when you get a chance. thanks. |
|
I think how it works offline today (shown below) is what we should be aiming for, but @puneetlath @NicolasBonet might have an opinion as well. 2026-07-20_16-48-07.mp4 |
I think this makes sense, yes |
|
@nabi-ebrahimi, is it clear how it should be? thanks. |
@marufsharifi, I've addressed the issue. Could you please review it again? Thanks! |
|
I found a pre-existing bug here on the Steps
ExpectedThe offline-created agent should sync as a single agent row, including the ActualAfter reconnecting, a duplicate/stale agent row can appear. The row cannot be edited/deleted properly. Root causeOffline agent creation is represented locally with an optimistic personal detail and an optimistic The Screenshots/VideosScreen.Recording.2026-07-23.at.10.58.30.AM.mp4Do you think we should fix this in this PR or handle it separately? Thanks! |
marufsharifi
left a comment
There was a problem hiding this comment.
🔴 The title/description don't match the implementation.
- Title: "Disable edit navigation for pending agents"
- Body: "…temporarily non-interactive… Verify the
Editbutton is disabled while creation is still pending."
But the diff does neither — it does accountID remapping, and AgentsPage.tsx:112 still navigates unconditionally:
action: () => Navigation.navigate(ROUTES.SETTINGS_AGENTS_EDIT.getRoute(accountID)),Test steps 4–6 (row "temporarily unavailable", Edit button disabled while pending) can't pass against this diff because that gating code doesn't exist. Please either (a) rewrite the description to document the ID-remapping approach, or (b) if you pivoted to the "disable while pending" approach, add that logic to AgentsPage/EditAgentPage. This PR currently looks like it's missing the files that implement the described behavior.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@marufsharifi, I've addressed your comment. Could you check it once again? |
|
@nabi-ebrahimi, could you please merge the main. |
| const [agentPrompts] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_AGENT_PROMPT); | ||
| const [allPolicies] = useOnyx(ONYXKEYS.COLLECTION.POLICY); | ||
| // Read here (rather than inside openAgentsPage()) so the action stays a pure function of its params. | ||
| const [optimisticAgentAccountIDMappingCreatedAt] = useOnyx(ONYXKEYS.OPTIMISTIC_AGENT_ACCOUNT_ID_MAPPING_CREATED_AT); |
|
This PR adds a new |
|
@tgolen, addressed your feedback. Could you take another look? Thanks |
tgolen
left a comment
There was a problem hiding this comment.
Thanks for the changes! I think we're getting there, but I'm still not fully understanding how the data can go stale.
| * The one caller that does still trigger it explicitly is openAgentsPage() (Agent.ts): an entry can go stale | ||
| * purely from time passing with no new write to fire this callback, and opening the agents list is the only | ||
| * realistic moment to re-check for that. |
There was a problem hiding this comment.
This still feels like it's somewhat in the same boat as previously (where it is triggered by something in the UI). How come the entries can go stale from time passing? Maybe the comment can be improved to explain why that happens.
If the time component is real for them going stale, wouldn't it be better to use an interval timer to re-run this check on a regular basis, rather than wait for a UI to trigger it?
There was a problem hiding this comment.
Thanks. Improved the comment.
On the interval timer — checked the codebase for precedent first: checkForUpdates.ts is the only periodic-check pattern here, and it's deliberately a no-op on native (platformSetup/index.native.ts), so a raw interval isn't something this app relies on for mobile.
Instead, added AppStateMonitor.addBecameActiveListener() alongside openAgentsPage() — it re-checks whenever the app resumes from background, which is the moment the most time is likely to have silently passed, using a pattern already proven on mobile elsewhere in this codebase.
|
This PR adds a new |
|
@marufsharifi, I’ve addressed your latest feedback. Could you take another look? |
|
I've had some concerns with the solution in this PR and was discussing some alternatives with @NicolasBonet. I'm going to place a HOLD on this PR until we able to resolve it. |
|
I believe we can close this one in favor of #100068 |
Explanation of Change
Opening a newly created agent before the server finishes creating it could land on a "Hmm... it's not here" page, because the page was still using the temporary ID assigned before saving instead of the real one.
This change resolves that temporary ID to the real one once the server responds, so the page keeps showing the agent instead of erroring out — across all the agent screens (edit, name, prompt, avatar), not just the main one. The temporary-to-real mapping is now saved to disk (not just kept in memory) so this also works after a refresh, and old entries are automatically cleaned up over time so it doesn't grow unbounded.
Fixed Issues
$ #93272
PROPOSAL: #93272 comment
Tests
Precondition:
Account > Agents.New agent.CreateAgentto finish.Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Screen.Recording.2026-07-20.at.11.40.58.PM.mov
Android: mWeb Chrome
Screen_Recording_20260720_235440_Chrome.mp4
iOS: Native
Screen.Recording.2026-07-20.at.11.59.49.PM.mov
iOS: mWeb Safari
Screen.Recording.2026-07-21.at.12.00.44.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-07-29.at.10.28.37.AM.mov