Open the HR sync results screen when the result arrives after jobDone - #99591
Conversation
|
@mananjadhav 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] |
![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: 4cfd2dc08c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isHRSyncDoneWithResult = isHRConnectionName && stageInProgress === CONST.POLICY.CONNECTIONS.SYNC_STAGE_NAME.JOB_DONE && !!syncResult; | ||
| const didHRSyncComplete = isFocused && isHRSyncDoneWithResult && didWatchSyncRunRef.current; |
There was a problem hiding this comment.
When a screen observes a running sync and then loses focus before JOB_DONE, this condition leaves didWatchSyncRunRef set because isFocused is false. Both WorkspaceHRPage and WorkspaceMembersPage invoke this hook and can remain mounted as frozen stack screens, so if the user moves between them during a sync, the newly focused page can show the result and the original page will show the same result again when revisited. Consume or globally deduplicate the completed run even when this particular hook instance is unfocused.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
(Yuwen's Agent) Valid edge case, thanks. When WorkspaceHRPage and WorkspaceMembersPage are both mounted and both watched the same live run, the focused page shows the result while the unfocused page keeps didWatchSyncRunRef set and re-fires it on refocus, so the result appears twice.
The naive per-instance fix (consume the run on JOB_DONE regardless of isFocused) would regress the intended single-page case this PR exists for: start a sync, navigate away before it finishes, then return to see the result. Correct dedup has to be cross-instance, which needs a per-run identity. The backend doesn't give us one; connectionSyncProgress.timestamp changes on every progress update, so it is not a per-sync key.
Flagging for the author to decide the dedup approach rather than auto-fixing, since a wrong key here reintroduces the "modal never shows" bug.
garrettmknight
left a comment
garrettmknight
left a comment
There was a problem hiding this comment.
Looks good from a product perspective
|
@ShridharGoel bump! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
ShridharGoel
left a comment
ShridharGoel
left a comment
There was a problem hiding this comment.
Code looks fine, though could not test because of lack of credentials.
…odal # Conflicts: # src/hooks/useHRSyncResultsPage.ts
yuwenmemon
changed the title
|
@ShridharGoel I believe we have cleared up your credentials issue? |
|
🚧 Gonals has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/Gonals in version: 9.4.77-0 🚀
|
|
🤖 Yes — help site changes are required. Draft PR: #101003 Your code change itself adds no new copy or flow, so it needs no docs on its own. But checking the HR articles against the current UI turned up three stale things, two of which this PR's behavior makes newly visible:
Also fixed a three-dots-menu convention violation in the TriNet article, per Files changed: the Gusto, TriNet, BambooHR, HiBob, Workday, and shared Connect an HR integration to Expensify articles. No new articles, no renamed files, no URL changes. Label verification and one thing I deliberately left outEvery label was checked against source, not guessed:
I did not add the 2-per-day manual sync limit to the Gusto and TriNet articles, even though the BambooHR, HiBob, and Workday articles carry it. One thing worth a second look: the Gusto and TriNet articles claim that leaving the page mid-sync emails the results to the workspace's technical contact. That's a backend behavior I can't verify from the App repo, so I preserved the claim as written rather than changing it. @yuwenmemon, 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.77-4 🚀
Bundle Size Analysis (Sentry): |
6 participants
@Gonals please review
Explanation of Change
The HR sync results screen never opened because the backend sends the
jobDonestage and the sync result as two separate Onyx updates, in either order. The hook armed the screen only on the render where the stage first becamejobDone, so a result-less update arriving first spent that render and dropped the result.The hook now records that this mount watched a sync run, and it opens the result once for that run, so the order no longer matters. A mount that finds an already-finished sync in Onyx never saw the run, therefore it still opens nothing.
Rebased onto main's migration of the modal to a navigation screen, so this lands on the renamed
useHRSyncResultsPage; that migration kept the same rising-edge guard, so the bug survived it. The hook is shared by every HR provider, so the fix covers TriNet and Merge HR too.Background: where the two updates come from, and why the approved proposal needed a change
Integration-Server sends the
jobDoneevent twice, fromPushOnyxUpdateProgressManager.sendNotification:sendReliableNotificationcallsSendIntegrationProgressUpdatein Auth, which queues an Onyx merge of{stageInProgress: 'jobDone', connectionName, timestamp}. That command has noresultparameter, so this update never carries one.onyxApiUpdatePusher event directly, and that one does carryresult.Whichever lands first wins the rising edge. The result-bearing push arriving first is the working case; the reliable one arriving first is this bug.
The regression came from Prevent stale Gusto sync result modal, which replaced a per-render dedupe with the rising-edge check to stop the modal replaying on remount. That fixed the replay and introduced this drop. The new test covers both directions, so neither can come back.
The approved proposal keyed the dedupe on
connectionSyncProgress.timestamp, treating it as a per-sync identity. It is not one: every progress update stamps a fresh timestamp (PushOnyxUpdateProgressManagerusesZonedDateTime.now(), Auth usesSTimeNow(), and the optimistic write usesnew Date()). A timestamp seen mid-sync therefore never matches the one onjobDone, so that version would have closed the modal permanently. This uses a per-mount flag instead, which is the same idea without the false identity.A follow-up worth filing on the backend: a client that is offline when the sync finishes only ever receives update 1, so it gets
jobDonewith no result at all. Passing the result throughSendIntegrationProgressUpdatewould remove that gap, but it needs matching changes in Integration-Server, Web-Expensify, and Auth, so it does not belong here.Fixed Issues
$ #99006
PROPOSAL: #99006 (comment)
Tests
npx jest tests/unit/hooks/useHRSyncResultsPage.test.tsand verify all cases pass.Offline tests
QA Steps
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