Conversation
Co-authored-by: Michał Jasikowski <mjasikowski@users.noreply.github.com>
|
There's no code fix to push here — every code and test check on this PR is green. The two remaining ❌ are human-action gates, not code failures:
All code checks passing
So: nothing for me to fix. To unblock the merge, |
|
Both failing checks are human-action gates, not code failures — there's nothing for me to fix by pushing code. Every code check (Bun/Jest tests, typecheck, ESLint, Oxfmt, Storybook, perf) is green.
Once the checklist is completed and an independent reviewer approves, both checks will pass. Let me know if you'd like me to change anything in the actual fix. |
|
🚧 mjasikowski has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@MelvinBot remove comments from this PR |
|
|
|
@MelvinBot remove comments from this PR |
|
@chuckdries @mjasikowski One of you needs to 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| value: {[optimisticAccountID]: null}, | ||
| // Clear only the transient optimistic flag rather than nulling the whole detail. Nulling wiped the | ||
| // agent's `avatar` the instant CreateAgent succeeded, so for a beat its report/LHN icon fell back to | ||
| // a default avatar before the real (bot) avatar returned from the response — the flicker reported in |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This comment uses an em dash (—) in its own sentence. Comments should read as plain, natural sentences without stylistic tics like em/en dashes.
Replace the em dash with plain phrasing, e.g.:
// a default avatar before the real (bot) avatar returned from the response. That is the flicker reported in
// issue 97844. Keeping the avatar in place lets the response overwrite the values without that gap.Reviewed at: 34c9cd3 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const promptRollback = findUpdate(successData, `${ONYXKEYS.COLLECTION.SHARED_NVP_AGENT_PROMPT}${accountID}`); | ||
|
|
||
| expect(getPersonalDetailValue(successData, accountID)).toBeNull(); | ||
| // The whole detail is no longer nulled — wiping it briefly dropped the agent's avatar, causing a flicker |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This comment uses an em dash (—) in its own sentence. Comments should read as plain, natural sentences without stylistic tics like em/en dashes.
Replace the em dash with plain phrasing, e.g.:
// The whole detail is no longer nulled. Wiping it briefly dropped the agent's avatar, causing a flicker
// (issue 97844). Only the transient optimistic flag is cleared so the avatar survives until the response lands.Reviewed at: 34c9cd3 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Co-authored-by: Michał Jasikowski <mjasikowski@users.noreply.github.com>
|
🤖 Removed the code comments added by this PR and pushed to
No logic changed — only comment lines were deleted. |
|
@puneetlath 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] |
|
All the code checks are now passing — ESLint, typecheck, The one remaining ❌ is Check independent approval, which isn't a code failure and there's nothing for me to push to fix it. It's red because the PR has no independent human approval yet ( So no fix commit is needed here — the PR just needs a reviewer's approval to go fully green. |
|
🚧 mjasikowski has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@MelvinBot merge main and resolve conflicts |
Resolved conflicts in src/libs/actions/Agent.ts and tests/unit/AgentActionTest.ts. main moved the optimistic personal-detail and agent-prompt cleanup out of createAgent()'s successData into replaceOptimisticAgentWithActualAgent, so that it always runs after the agent settings redirect. That supersedes this branch's successData tweak (nulling only isOptimisticPersonalDetail instead of the whole entry), so main's version of successData is taken and the branch's two entries are dropped. The branch's actual fix is unaffected: the optimistic personal detail is still written with a direct Onyx.merge outside the optimisticData rollback machinery, and AddAgentPage still awaits that merge before navigating. Co-authored-by: Michał Jasikowski <mjasikowski@users.noreply.github.com>
|
Merged Two conflicts, both in the same spot: Your actual fix is untouched: the optimistic personal detail is still written with a direct Resolution detail and verification
I also checked that nothing in Verified:
|
|
🚧 mjasikowski has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@eVoloshchak can you please check if you can reproduce the flicker in the latest ad hoc build? I can't on both Firefox and Chrome, but you had more luck with that before |
|
@MelvinBot merge main resolve conflicts |
Resolved a conflict in src/libs/actions/Agent.ts. main refactored every ONYXKEYS.PERSONAL_DETAILS_LIST write in this file to go through the new buildPersonalDetailsUpdate() helper from PersonalDetailsUtils. The helper is semantically identical (MERGE onto the same key), so the conflict in createAgent()'s optimisticData -- where this branch deleted the personal detail entry and main reformatted it -- resolves in favour of this branch: the entry stays deleted so the optimistic avatar isn't rolled back mid-flight. Carried main's refactor onto the branch's own new line: the direct Onyx.merge(ONYXKEYS.PERSONAL_DETAILS_LIST, ...) is now Onyx.update([buildPersonalDetailsUpdate(...)]), matching how main converted the imperative merge in clearAgentError(). Onyx.update() also returns a promise, so AddAgentPage still awaits the write before navigating. Updated AgentActionTest accordingly: the optimistic-personal-detail spy now watches Onyx.update instead of Onyx.merge. Co-authored-by: Michał Jasikowski <mjasikowski@users.noreply.github.com>
|
Merged One conflict this time. I also carried Resolution detail and verificationWhy take the branch's side: Following Test follow-on: Verified:
|
|
No code fix is needed here — nothing in the diff can make this check pass. The only failing check is Why it fails: it's a policy gate, not a code failure. No formal review has ever been submitted on this PR — the API shows zero To clear it: a requested reviewer other than the author needs to submit an approving review. One thing worth flagging: I made no code changes and pushed nothing. |
|
@eVoloshchak bump |
|
@mjasikowski, still reproducible :( Screen.Recording.2026-09-23.at.15.41.55.movScreen.Recording.2026-09-23.at.15.40.15.mov |
|
🚧 mjasikowski has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🤖 No code fix needed — the one failing check is a human review gate.
The PR simply has no approvals yet. It will go green once an independent reviewer approves. Nothing in the diff causes it, so I didn't push anything. Everything else passed — |
|
🚧 mjasikowski 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! 🧪🧪
|
|
@eVoloshchak one last try with the adhoc build please? this time even GPT can't reproduce it |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppscreen-20260925-133933-1790336328771.mp4Android: mWeb ChromeScreen.Recording.2026-09-25.at.12.57.11.moviOS: HybridAppScreen.Recording.2026-09-25.at.13.10.09.moviOS: mWeb SafariScreen.Recording.2026-09-25.at.13.01.13.movMacOS: Chrome / SafariScreen.Recording.2026-09-25.at.12.44.26.mov |
eVoloshchak
left a comment
eVoloshchak
left a comment
There was a problem hiding this comment.
Sucess!
LGTM and tests well
|
@MelvinBot, please fill out the tests section with the following steps
And mark this as ready for review |
|
I added your 6 steps to the Tests section, above the existing JS console check. I also corrected "buutton" to "button" in step 5. The PR is still a draft. My environment blocked the |
![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: 4857466423
ℹ️ 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".
| } | ||
| const isNarrowLayout = getIsNarrowLayout(); | ||
|
|
||
| optimisticPersonalDetailPromise.then(() => { |
There was a problem hiding this comment.
When the personal-details write is delayed (for example on a busy device) and the user presses the header back button before it resolves, this callback still runs after AddAgentPage has been removed and navigates to the new DM from whichever route is now active. useBeforeRemove only performs draft cleanup and does not cancel the pending callback, so this can unexpectedly pull a user who backed out of the flow into the agent report. Guard the continuation against the page no longer being active, or prevent leaving while this write is pending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3abc407. If you leave the builder before the personal-detail write resolves, the app no longer opens the DM.
- The
beforeRemovehandler now setshasLeftPageRef(AddAgentPage.tsx:119). - The deferred callback checks that ref first. If it's set, the callback clears the avatar draft and skips navigation (AddAgentPage.tsx:155-159). The agent is still created.
- I added a unit test for the back-out case.
AddAgentPageTest and AgentActionTest pass (81 tests). ESLint, typecheck, the React Compiler check, and oxfmt also pass.
…lves Co-authored-by: Michał Jasikowski <mjasikowski@users.noreply.github.com>
This branch has not been deployed
4 participants
Explanation of Change
Follow-up to
Expensify/App#95631, which opens the newly-created agent's DM immediately. Reported in issue 97844: the agent's avatar briefly flickers right as the agent is created (Android & iOS app, staging only).Root cause: On
CREATE_AGENTsuccess,createAgent'ssuccessDatanulled the entire optimistic personal detail for the agent (PERSONAL_DETAILS_LIST[optimisticAccountID] = null). That wiped the agent'savatarfor a beat, so the DM header / LHN icon fell back to a default (accountID-hash) avatar viagetIconsForParticipantsbefore the real bot avatar returned from the response — a visible flicker. Both avatars render as bundled local SVGs (the render path resolves catalog CDN URLs to local assets viaoptimizeAvatarSource), so this is a local default→bot SVG swap, not a network fetch.Fix: On success, clear only the transient
isOptimisticPersonalDetailflag instead of nulling the whole detail, so the avatar stays in place and the response overwrites the values without a gap.AI-run checks (draft — human testing still required):
npm testfortests/unit/AgentActionTest.ts(61 passed) andtests/unit/pages/settings/AddAgentPageTest.tsx(19 passed), pluseslint,tsgotypecheck, andoxfmt. Thesuccess data …unit test was updated to assert the new behavior.Fixed Issues
$ #97844
PROPOSAL:
Tests
Offline tests
QA Steps
Same as #97844
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.