Migrate HRSyncResultsModal to @react-navigation modal screen - #97172
Conversation
The HR sync results were rendered by a RIGHT_DOCKED <Modal> shown through the global modal context. Replace it with a dynamic route so the results are a real navigation screen. A sync can finish while the user is on either the HR page or the members list, so the route declares both as entry screens. The sync payload already lives in Onyx under the policy's connection sync progress, so the screen re-reads it and nothing rich has to be serialized into navigation params. policyID is inherited from the workspace parents rather than repeated in the suffix, which also makes it redundant in the hook signature.
|
@parasharrajat 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] |
…96967 # Conflicts: # src/pages/workspace/WorkspaceMembersPage.tsx
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| <Icon | ||
| src={icons.DownArrow} | ||
| fill={theme.icon} | ||
| additionalStyles={isSkippedSectionExpanded ? {transform: [{rotate: '180deg'}]} : undefined} |
There was a problem hiding this comment.
❌ UI-3 (docs)
This passes an inline style object literal ({transform: [{rotate: '180deg'}]}) to additionalStyles, which is a style-typed prop (StyleProp<ViewStyle>). Per STYLING.md, inline style objects are not allowed: they allocate a new object every render (breaking memoization) and duplicate a value that already exists in the shared style system. A direct equivalent already exists as styles.flipUpsideDown (transform: rotate(180deg)).
Use the existing style token instead of an inline object:
<Icon
src={icons.DownArrow}
fill={theme.icon}
additionalStyles={isSkippedSectionExpanded ? styles.flipUpsideDown : undefined}
/>Reviewed at: d628293 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
@trasnake87 Please make sure to reply to each bot comment on each PR.
There was a problem hiding this comment.
Good catch — fixed. Swapped the inline {transform: [{rotate: "180deg"}]} object for the existing styles.flipUpsideDown token (verified it exists at src/styles/index.ts:3198), so nothing is allocated per render.
There was a problem hiding this comment.
Understood, and sorry — both bot comments now have replies. Fixed the UI-3 inline style, and the Codex access-guard point was a genuine defect which I have fixed too.
![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: d6282930e7
ℹ️ 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".
| <ScreenWrapper | ||
| testID="DynamicHRSyncResultsPage" | ||
| enableEdgeToEdgeBottomSafeAreaPadding | ||
| > |
There was a problem hiding this comment.
When this dynamic route is opened directly or restored from a saved URL, this screen reads and renders the HR sync result for policyID without any of the workspace/HR access checks that protected the old modal via the HR/Members entry pages. In that context a user who cannot access those workspace settings can still land on this screen and see the skipped employee list if the Onyx sync-progress entry is present, so please wrap the page in the same workspace/feature access guard before displaying the result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and fixed — this was a real gap introduced by the migration. The old modal only ever rendered inside WorkspaceHRPage, so it inherited that page's AccessOrNotFoundWrapper. As a dynamic route the screen is deep-linkable and had no such check, so a user without workspace HR access could open the URL and read the skipped-employee list straight out of Onyx.
I have wrapped the screen in the same guard the HR page uses:
<AccessOrNotFoundWrapper
accessVariants={[CONST.POLICY.ACCESS_VARIANTS.ADMIN, CONST.POLICY.ACCESS_VARIANTS.CONTROL]}
policyID={policyID}
featureName={CONST.POLICY.MORE_FEATURES.IS_HR_ENABLED}
policyFeature={CONST.POLICY.POLICY_FEATURE.MORE_FEATURES}
>
|
@parasharrajat @mountiny this is ready for review. One thing I want to be upfront about rather than have you find it: the
Happy to fill any of those in if you would rather I chase them down, or to explain a decision if something looks off — in particular I dropped |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Thanks! Please test all platforms every pr unless it really does not affect some of the platforms should be tested on all of them |
…96967 # Conflicts: # src/pages/workspace/WorkspaceMembersPage.tsx
|
@parasharrajat gentle bump on this one — it has been open since the 28th and is ready whenever you have a window. It is mergeable and CI is green apart from the author checklist, which I deliberately left partly unchecked (the offline / High-Traffic boxes are attestations I have not personally performed, and I only verified web + both mWeb variants, not the native apps). I explained that in the description and in my first comment rather than ticking boxes I could not stand behind. Happy to answer anything or make changes — in particular if you would rather I chase down the native-platform checks, or if the |
| * when the sync transitions to the `JOB_DONE` stage with a result payload. | ||
| */ | ||
| function useHRSyncResultsModal(policyID: string, connectionSyncProgress: OnyxEntry<PolicyConnectionSyncProgress>, isFocused: boolean) { | ||
| const modal = useModal(); | ||
| function useHRSyncResults(connectionSyncProgress: OnyxEntry<PolicyConnectionSyncProgress>, isFocused: boolean) { |
There was a problem hiding this comment.
| function useHRSyncResults(connectionSyncProgress: OnyxEntry<PolicyConnectionSyncProgress>, isFocused: boolean) { | |
| function useHRSyncResultsPage(connectionSyncProgress: OnyxEntry<PolicyConnectionSyncProgress>, isFocused: boolean) { |
There was a problem hiding this comment.
Done — renamed to useHRSyncResultsPage (file and all call sites).
| import Navigation from '@navigation/Navigation'; | ||
| import type {SettingsNavigatorParamList} from '@navigation/types'; |
There was a problem hiding this comment.
Let's use @libs/Navigation import.
There was a problem hiding this comment.
Fixed — switched to import Navigation from "@libs/Navigation/Navigation".
|
Update the testing results to show attachment in post itself. Also, test it on all platforms. |
|
Fix Author checklist. |
|
Setting up HR sync for testing. |
|
@parasharrajat all seven comments are addressed — four code changes pushed shortly, and I have replied on every thread including both bot comments (apologies for missing those first time round). Summary of the fixes:
On the checklist and all-platform testing — I want to be straight with you rather than just ticking boxes. I have verified this on web and on both mWeb variants (screenshots in the description). I have not run the native apps, and the offline and High-Traffic items are first-person attestations I have not personally performed, so I left those unchecked deliberately and said so in the description. I would rather show you an honest checklist than a green one. If native coverage is required before this can move, tell me and I will get builds running — it is a time cost, not a refusal. And if you would rather see the results rendered inline in the PR body rather than in collapsed sections, I can restructure that too. |
The screen is a dynamic route and therefore deep-linkable, so it now carries the same AccessOrNotFoundWrapper the HR page uses. Previously it inherited nothing: opening the URL directly bypassed the workspace/HR access checks the old modal got for free by only ever rendering inside WorkspaceHRPage. Also swap the inline transform for the existing styles.flipUpsideDown token, rename the hook to useHRSyncResultsPage, and import Navigation from @libs.
19f3a31 to
217de3f
Compare
Yes, this needs to be tested on all platforms and please fix the screenshots as well. |
| } | ||
|
|
||
| export default HRSyncResultsModal; | ||
| DynamicHRSyncResultsPage.displayName = 'DynamicHRSyncResultsPage'; |
|
Not a casing choice — the header string is
Worth being explicit that this is pre-existing and untouched by this PR: those lines are byte-identical to the current https://github.com/Expensify/App/blob/main/src/components/HRSyncResultsModal.tsx#L44-L45 The migration changed how To get the real title while testing, add the connection alongside the sync progress: Onyx.merge(`policy_${policyID}`, {connections: {merge_hris: {config: {integration: 'bamboohr'}}}});That renders "BambooHR sync complete". Happy to fix the empty-provider wording here if you would rather not leave it as-is — just say so, since it is outside the migration's scope and I did not want to expand the diff unasked. |
|
@parasharrajat anything blocking on my side? The checklist is complete and CI is green, and I'm happy to adjust the empty-provider wording if you'd like that handled here rather than separately. |
|
Can you please merge main? @trasnake87 |
Screenshots🔲 iOS / native🔲 MacOS / Chrome19.08.2026_00.20.39_REC.mp4 |
|
I didn't see the result page loading on So I will run tests with hard-coded values. |
# Conflicts: # src/pages/workspace/WorkspaceMembersPage.tsx # src/pages/workspace/hr/WorkspaceHRPage.tsx
|
BUG: I noticed that use can go back to the status page but back button on browser. Should this be blocked? |
|
Good catch, and I checked it properly — the answer is a bit different from the framing. Browser back doesn't reach the results screen. I couldn't get it to in any sequence I tried: I tried blocking it with https://github.com/Expensify/App/blob/main/src/hooks/useDynamicBackPath.ts#L22-L29
My recommendation is to leave it as-is. Forward-reachability isn't specific to this screen — 291 of the 301 Happy to do it here if you'd prefer — it would need an explicit back target passed to the screen instead of deriving it from navigation state. |
|
@trasnake87 @parasharrajat Lets try to focus on this one to get it over the finish line sooner, thank you |
|
@parasharrajat checking in per Vit's note — everything on my side has been done for a while (checklist complete, CI green apart from the approval gate itself, main merged, the back-button question answered with measurements). Is there anything blocking your checklist testing that I can help with? |
|
I will get this retested tomorrow. |
parasharrajat
left a comment
parasharrajat
left a comment
There was a problem hiding this comment.
Reviewer Checklist
- I have verified the author checklist is complete (all boxes are checked off).
- I verified the correct issue is linked in the
### Fixed Issuessection above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - I verified the steps cover any possible 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 verified the steps for local testing are in the
- I checked that screenshots or videos are included for tests on all platforms
- I included screenshots or videos for tests on all platforms
- I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
- I verified tests pass on all platforms & I tested again on:
- Android: HybridApp
- Android: mWeb Chrome
- iOS: HybridApp
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
- I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
toggleReportand notonIconClick). - 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)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
- If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarhave been tested & I retested again) - If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a
/** comment above it */ - The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
- If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
- 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 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
Avataris modified, I verified thatAvataris 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
Designlabel and/or tagged@Expensify/designso the design team can review the changes.
- For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
- If the
mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps. - I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.
Screenshots
19.08.2026_00.20.39_REC.mp4
25.08.2026_19.47.37_REC.mp4
🎀 👀 🎀 C+ reviewed
|
Conflicts too @jmusial can you please review? |
# Conflicts: # src/pages/workspace/hr/DynamicHRSyncResultsPage.tsx
|
@mountiny done — simplified the file header and the access-guard comment to just state the why, dropped the redundant |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.65-0 🚀
|
|
No help site changes are required for this PR. I did not create a docs PR. WhyThis PR is an internal navigation refactor. It moves the HR sync results UI from a
Existing docs are still accurateThe HR connection articles already describe this UI, and every claim they make still holds after the merge:
Importantly, none of these articles calls it a "modal" or a "pop-up" — they all say sync results panel, which the migrated screen still is. So there is no stale terminology to correct. Two changes I considered and rejected as doc-worthy1. The screen is now deep-linkable ( 2. The screen now gates on HR access via Also worth noting: Zenefits is covered by the existing TriNet article (TriNet acquired Zenefits), so there is no missing-provider gap introduced here. If you would like the sync results section expanded for a different reason — for example, to document the Skipped expand/collapse behavior, which no article currently mentions — reply and I will draft that PR. That gap predates this PR rather than being created by it. @trasnake87, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR (No help site PR was created, since no changes are required — so there is nothing to mark ready. Please confirm you agree with the assessment above.) |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.4.65-3 🚀
Bundle Size Analysis (Sentry): |
5 participants
Details
HRSyncResultsModalwas aRIGHT_DOCKED<Modal>rendered through the global modal context. This migrates it to a dynamic@react-navigationmodal screen,DynamicHRSyncResultsPage, so the sync results live in the navigation stack instead of a parallel modal layer.A sync can finish while the user is on either the workspace HR page or the members list, so the dynamic route declares both as
entryScreens.policyIDis not repeated in the route suffix — both parents are already workspace-scoped, so it is inherited (same asWORKSPACE_INVITE), which in turn made thepolicyIDargument redundant in the hook.The sync payload already lives in Onyx under the policy's connection sync progress, so the screen re-reads it from
policyIDand nothing rich has to be serialized into navigation params.useHRSyncResultsModalis renamed touseHRSyncResultssince it no longer opens a modal, andsrc/components/HRSyncResultsModal.tsxis deleted.Fixed Issues
$ #96755
Tests
hr-sync-resultssegment while the screen is open, and that loading that URL directly opens the screen.Offline steps
The screen renders entirely from data already in Onyx (the policy's connection sync progress), so an already-received sync result stays viewable offline: opening the results screen while offline shows the same counts and skipped list, and back / Got it still dismiss it. No new network request is made by this screen.
QA Steps
Same as the Tests above.
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
MacOS: Chrome / Safari
The migrated screen opening as a right-docked navigation screen from the Members entry point. The members list stays mounted behind it, the URL carries the
hr-sync-resultssegment, and back / Got it return to it:https://github.com/user-attachments/assets/cd70f2b9-7d41-4057-9ca2-f442cf26185f
Android: mWeb Chrome
Pixel 7 viewport (412x915, mobile UA). On a narrow layout the screen takes the full width rather than docking right:
https://github.com/user-attachments/assets/251d315d-5801-4e55-839c-d338beb16800
iOS: mWeb Safari
iPhone viewport (390x844, iOS Safari UA):
https://github.com/user-attachments/assets/f5f9507a-f4e4-4bd4-ae5b-8d461ceb0c19
iOS: Native
iPhone 17e simulator,
DebugDevelopmentbuild. The migrated screen as a real navigation screen, and the skipped-employee section expanding (the chevron flips via the sharedflipUpsideDownstyle). Got it returns to the Members list throughuseDynamicBackPath:Android: Native
Pixel 7 API 35 emulator, local
developmentDebugbuild. The migrated screen reached through the dynamic route, and Got it returning to the Members list viauseDynamicBackPath:Note the counts read
0here:policyConnectionSyncProgress_is transient client state that the Android app clears on every launch, so a pre-seeded payload cannot survive startup (it survives on iOS, hence the populated counts there). What this capture demonstrates is the migration itself — the screen mounts as a navigation screen, the route resolves, and the back path works. The rendering of real counts is covered by the iOS capture above.Checklist notes (per @parasharrajat's request to complete every item):
cubonxrp@gmail.com(member of the Crowded Policy (Do Not Delete) and Big Chat (Do Not Delete) fixtures). Offline was tested by loading the app, disabling the network, and confirming the screen still mounts from local Onyx with the offline indicator shown:High Traffic account
Offline
WorkspaceMembersTest,getStateForDynamicRouteTests,useDynamicBackPathTests,getPathFromStateTests— 73/73).tsc,eslintand the full suite afterwards.Coverage I have not claimed above, so a reviewer knows exactly where this stands:
JOB_DONEtrigger end to end: the workspace I have available has no HR connection, and its HR page 404s. I verified the route, the screen render, the expand/collapse and the dismiss paths from the Members entry point, and the HR entry point is wired through the sameentryScreensarray and registration.tests/ui/WorkspaceMembersTest.tsx(renders a consumer of the changed hook) plusgetStateForDynamicRouteTests,useDynamicBackPathTestsandgetPathFromStateTests: 73/73 passing.tscandeslintare clean.