fix: add confirmation popup for cross-context user management action#4977
fix: add confirmation popup for cross-context user management action#4977muditbhutani wants to merge 3 commits into
Conversation
The 'Switch to update' button on the team management user detail page silently switched the entire dashboard context (org/merchant/profile) without explaining what was happening, which confused merchants. - Rename the button to 'Manage user' to match the in-context action - Show a confirmation popup (Warning, WithIcon) before switching, naming the target merchant and profile so the user knows their dashboard context will change
|
Review changes with |
After confirming the context switch from the team management user detail page, the user previously had to locate and click the row's 'Manage user' button again in the new context. The switch action now records the target role row in a Recoil atom (the dashboard tree can remount mid-switch, so component state cannot carry the intent) and the row's action consumes it once the new context renders, opening the manage user modal directly.
XyneSpaces
left a comment
XyneSpaces
left a comment
There was a problem hiding this comment.
Automated Review Summary
PR: fix: add confirmation popup for cross-context user management action
Reviewer: XyneSpaces
Findings Overview
| Severity | Count | Issues |
|---|---|---|
| 2 | Recoil atom naming; useEffect dependencies | |
| 💡 Suggestion | 2 | Empty ID fallback; inline function |
✅ Positive Notes
- Confirmation popup pattern follows the established codebase pattern using
PopUpState.useShowPopUpwith(Warning, WithIcon)— consistent with existing implementations - Button redesign from "Switch to update" to "Manage user" with
Secondary/Mediumstyling improves UX clarity - Auto-open modal logic correctly consumes the intent to prevent reopening on subsequent visits
- Home page copy update from "20+" to "180+ gateways" accurately reflects current processor count
Detailed Findings
See inline comments for specific locations and fixes.
Verdict: Clean implementation with minor suggestions. The confirmation flow significantly improves the user experience by making the context switch explicit.
| }, | ||
| ~isRequired=true, | ||
| ) | ||
| // The dashboard can remount while the OMP context switch is in flight, so the |
There was a problem hiding this comment.
The atom name "autoOpenManageUserModal" uses camelCase without file/feature prefix. Other atoms in this codebase typically follow a namespaced pattern for clarity.
Suggestion: Use a more specific name like "userManagement_autoOpenManageUserModal" or "manageUserModal_autoOpenIntent" to match the convention used in other Recoil atoms.
| // than component state. The stored key identifies the exact role row the | ||
| // switch was triggered for. | ||
| let autoOpenManageUserModalAtom: Recoil.recoilAtom<option<string>> = Recoil.atom( | ||
| "autoOpenManageUserModal", |
There was a problem hiding this comment.
Using Option.getOr("") for orgId, merchantId, and profileId means rows with missing IDs will generate identical keys if they share the same userEmail and roleId.
let orgId = userInfoValue.org.id->Option.getOr("")Suggestion: Consider using a sentinel value like "_" or a UUID placeholder, or ensure keys remain unique even when IDs are None:
let orgId = userInfoValue.org.id->Option.getOr("_none_")| if openModalByDefault { | ||
| setAutoOpenRowKey(_ => None) | ||
| } | ||
| None |
There was a problem hiding this comment.
💡 useEffect dependency array
The useEffect uses [openModalByDefault] but references setAutoOpenRowKey inside. While Recoil setters are stable, for correctness consider either:
- Adding
setAutoOpenRowKeyto dependencies (it's stable from Recoil) - Or using the functional update form to avoid the dependency
Current code is functionally correct — this is a minor code hygiene note.
| customButtonStyle="!p-2" | ||
| buttonType={PrimaryOutline} | ||
| onClick={_ => onSwitchForUserAction()->ignore} | ||
| text="Manage user" |
There was a problem hiding this comment.
💡 Unnecessary wrapper function
The onClick handler wraps switchConfirmation() in an arrow function:
onClick={_ => switchConfirmation()}Suggestion: Since switchConfirmation takes unit and returns unit, you can pass it directly:
onClick={_ => switchConfirmation()}This is consistent with the existing pattern in this file (see line 289 where onSwitchForUserAction()->ignore is used).
| let profileId = userInfoValue.profile.id->Option.getOr("") | ||
| `${userEmail}-${orgId}-${merchantId}-${profileId}-${userInfoValue.roleId}` | ||
| } |
There was a problem hiding this comment.
can we check if we can avoid using the concatenated string and maybe use a record ?
| // auto-open intent for the manage user modal is kept in a Recoil atom rather | ||
| // than component state. The stored key identifies the exact role row the | ||
| // switch was triggered for. | ||
| let autoOpenManageUserModalAtom: Recoil.recoilAtom<option<string>> = Recoil.atom( |
There was a problem hiding this comment.
instead of keeping this atom in helper file, can we keep it in a new file like UserManagementAtoms.res
3 participants
Type of Change
Description
On the Team management → user detail page, rows whose org/merchant/profile lineage differs from the current JWT session show a button that swaps the entire dashboard context (via
user/switch/org/user/switch/merchant/user/switch/profile) when clicked.This PR makes that action explicit and seamless:
Commit 1 — confirmation before switching
Secondary,Medium).PopUpState.useShowPopUpwith(Warning, WithIcon), same pattern as the delete-user confirmation inManageUserModal) that names the target merchant and profile and tells the user their dashboard will be switched to that context.name-with-value-fallback logic as the table rows on this page.Commit 2 — auto-open the manage user modal after the switch
autoOpenManageUserModalAtom) keyed by user email + org/merchant/profile lineage + role id, because the dashboard tree can remount while the switch is in flight (activeProductbecomesUnknownProduct), so component state cannot carry it.UserActionconsumes the key once the matching row renders in the new context and passesopenModalByDefaulttoManageUserModal.Commit 3 — home page copy update
HomeUtils.res).Motivation and Context
The "Switch to update" label was confusing to merchants: it didn't say what would be switched, and clicking it silently replaced the whole session context (sidebar merchant, profile, product) as a side effect of a team-management action, leaving the user to discover the renamed button afterwards. Since
user/user/update_roleresolves the target user-role from the JWT's scope, the context switch is unavoidable today — so the UI now explains it, asks for confirmation, and completes the journey by opening the manage modal directly after the switch.How did you test it?
npx rescript buildcompiles cleanly.showPopUppattern already used byDeleteUserRolein the same screen.Manual verification: open Team management → a user with a role under a different merchant/profile → the row action now reads "Manage user" → clicking it shows the confirmation popup naming the target merchant/profile → Confirm switches context and the Manage user modal for that role opens automatically; Cancel leaves the session untouched. The modal does not reopen on subsequent visits to the page (intent is consumed once).
Where to test it?
Backend Dependency
Backend PR URL:
Feature Flag
Feature flag name(s):
Checklist
npm run re:build