[dmunozv04]

SUMMARY

The role editor allowed saving changes before the role's existing users, groups and permissions had finished loading. This could unintentionally remove them from the role if a user saved too early.

This change prevents saving until all existing role data has finished loading, or if any of the requests fail.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:
https://github.com/user-attachments/assets/75d0bcc6-76ca-4e60-ba3c-74eaf7374068

The video shows a role being renamed and saved before its users have finished loading (simulated using DevTools network throttling), causing those users to be removed from the role.

TESTING INSTRUCTIONS

1. Open the role editor with network throttling enabled in DevTools, selecting to throttle the request that loads users in that role (api/v1/security/users/?q=(filters:!((col:roles,opr:rel_m_m,value:6))), where 6 is the role ID)
2. Verify that saving is disabled while users, groups, and permissions are loading.
3. Once loading finishes, rename the role and save.
4. Verify that all existing users, groups, and permissions are preserved.

The new tests cover this regression and fail when run against master. (27 failed)

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codeant-ai-for-open-source]

  CodeAnt PR Risk

🟢  Low Risk

• The PR appears safe to merge; no actionable issues were identified in the review.

Assessed commit: 12cdeb1353a4

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.04%. Comparing base (230d9b0) to head (12cdeb1).

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #46006   +/-   ##
=======================================
  Coverage   85.03%   85.04%           
=======================================
  Files        3080     3080           
  Lines      209437   209465   +28     
  Branches    49385    49401   +16     
=======================================
+ Hits       178095   178130   +35     
+ Misses      27960    27953    -7     
  Partials     3382     3382           

Flag	Coverage Δ
javascript	81.38% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[rebenitez1802]

Request changes: the hydration gate fixes a real data-loss bug and the tests are thorough, but one regression needs fixing before merge.

🔴 High — Closing the modal mid-save skips the list refresh, hides failures and can re-grant removed access
RoleListEditModal.tsx only calls onSave and the save toasts when mounted.current. While a save is in flight, Cancel / X / Esc / mask stay enabled, and RolesList unmounts the editor on close. So closing mid-save means refreshData() never runs, and a failed save shows no toast at all. Because the list keeps the stale role row, the following sequence silently re-grants Group 2: remove Group 2, Save, Cancel while saving, reopen the same role, rename it, Save. That sends updateRoleGroups(1, [1, 2]), and permissions behave the same way. All three cases pass on master and fail on this branch.
Fix: drop the mounted ref and its three gates (inline suggestions below). This is master's existing behaviour; there, the stale onSave also closes a reopened modal, which discards the stale role copy. Then update the "a save finishing after closing…" test to expect onSave and the success toast once. With these suggestions applied, the full RoleListEditModal suite (44 tests) and the close-mid-save repros pass, and oxlint/oxfmt are clean.

[dmunozv04]

Thanks @rebenitez1802 for the quick review and the feedback!
I applied all feedback and added a new test to check that a save failing after closing still reports the failure.

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	12cdeb1
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6aeca0311efdfe58406ef078
😎 Deploy Preview	https://deploy-preview-46006--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[madhushreeag]

The fix is correct, the users-vs-permissions/groups asymmetry is sound, and it ships with a genuine regression test 🚀

[madhushreeag]

@dmunozv04 , can you rebase your branch with master?

[rebenitez1802]

Approve: the round-1 blocker is fixed, and the tests guard it. Thanks for the quick turnaround and the extra failure-after-close test! I verified that restoring the round-1 mounted gates fails both after-close tests (each gate is caught on its own). A page-level RolesList probe of a stale save after close/reopen behaves exactly like master, so there's no new risk from removing the gate.

[dmunozv04]

Thanks @rebenitez1802 and @madhushreeag!
I’ve just rebased the PR