Skip to content

Fix server-side role search behavior across role selectors#27737

Open
harsh-vador wants to merge 2 commits intomainfrom
implement-search-roles-ui-api
Open

Fix server-side role search behavior across role selectors#27737
harsh-vador wants to merge 2 commits intomainfrom
implement-search-roles-ui-api

Conversation

@harsh-vador
Copy link
Copy Markdown
Contributor

Describe your changes:

Fixes

Description

  • Adds consistent server-side role search handling for all role-selection dropdowns using the new GET /v1/roles/search API.
  • Replaces stale option accumulation with “preserve selected values + replace search results” behavior so filtering works correctly in user, bot, create-user, SSO, and LDAP role selectors.
  • Adds debounce to role-search inputs to reduce request spam and align with existing async-search patterns in the UI.
  • Adds loading indicators where needed during in-flight role search requests.
  • Updates Playwright coverage to explicitly wait for /api/v1/roles/search responses instead of relying only on UI state.

Included fixes

  • UserProfileRoles popover search now filters correctly and keeps selected roles visible.
  • BotDetails role search now returns expected filtered results instead of mixing stale options.
  • CreateUser role dropdown preserves selected roles while updating search results.
  • SsoRolesSelectField and LdapRoleMappingWidget now follow the same selected-value preservation pattern.
  • Role-search-related specs/helpers now wait for backend responses during initial load and typed search.

Problem

  • Role-selection dropdowns were moved to server-side search, but several UI flows still handled results incorrectly.
  • Search results were being merged into stale/default option lists, so filtered search often appeared broken even when the API returned correct matches.
  • Some selectors did not preserve selected roles while refreshing options from search.
  • Role search requests were fired on every keystroke without debounce.
  • Existing tests/specs often waited only for UI updates and not for the /v1/roles/search network response, which made them brittle after moving to async server-side search.

Solution

  • Standardized role-search handling across role selectors to preserve currently selected values while replacing the rest of the options with the latest backend search results.
  • Added debounce to role-search inputs to align with existing async search patterns and reduce unnecessary API calls.
  • Added loading indicators for in-flight role search where needed.
  • Updated Playwright coverage to explicitly wait for /api/v1/roles/search responses during initial load and typed searches.
  • Switched role search consumers to consistently use the new GET /v1/roles/search API.

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

@harsh-vador harsh-vador self-assigned this Apr 26, 2026
@harsh-vador harsh-vador requested a review from a team as a code owner April 26, 2026 08:25
@harsh-vador harsh-vador added the safe to test Add this label to run secure Github workflows on PRs label Apr 26, 2026
@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Apr 26, 2026

Code Review ✅ Approved 4 resolved / 4 findings

Refactored role search logic resolves the TDZ dependency issues, stale closures in fetch functions, and improper state clearing. The debounced search implementation is now stable across selectors.

✅ 4 resolved
Bug: TDZ: value used in useMemo deps before its declaration

📄 openmetadata-ui/src/main/resources/ui/src/components/SettingsSso/SSOConfigurationForm/SsoRolesSelectField.tsx:45-59 📄 openmetadata-ui/src/main/resources/ui/src/components/SettingsSso/SSOConfigurationForm/SsoRolesSelectField.tsx:77
In SsoRolesSelectField.tsx, the useMemo at line 45 references value in its dependency array (line 67), but value is declared as a const at line 77 — after the useMemo call. With the project targeting ES6 (tsconfig.json"target": "es6"), const is preserved in output, so accessing value at line 67 triggers a Temporal Dead Zone ReferenceError at runtime.

Even if a bundler (esbuild/Vite) happens to downlevel constvar and avoids the crash, value would be undefined in the dependency array on every render, so the debounced function would never correctly react to value changes — causing stale selected-set filtering inside the debounce callback (line 54).

The inner closure at line 54 (const selectedSet = new Set(value || [])) also captures the pre-declaration binding, meaning the "preserve selected values" logic won't work correctly.

Bug: Error handler clears all options including selected roles

📄 openmetadata-ui/src/main/resources/ui/src/components/Settings/Users/CreateUser/CreateUser.component.tsx:158-163
In CreateUser.component.tsx line 163, the catch block calls setRoleOptions([]), wiping all options — including those for already-selected roles. A transient network error during a search would remove labels from any roles the user already picked, making the dropdown show raw IDs or empty tags.

The other components (UserProfileRoles, BotDetails, LdapRoleMappingWidget) correctly preserve existing state on error by omitting the clear. This component should follow the same pattern.

Performance: Debounced function recreated on every search result

📄 openmetadata-ui/src/main/resources/ui/src/components/SettingsSso/SSOConfigurationForm/SsoRolesSelectField.tsx:45-59 📄 openmetadata-ui/src/main/resources/ui/src/components/common/Form/JSONSchema/JsonSchemaWidgets/LdapRoleMappingWidget/LdapRoleMappingWidget.tsx:219-233
In SsoRolesSelectField.tsx, debouncedSearchRoles depends on [roleOptions, value] (line 67). Since the debounced callback itself updates roleOptions via setRoleOptions (line 59), every search response triggers a re-render that recreates the debounce function, cancelling any pending invocation.

The same pattern appears in LdapRoleMappingWidget.tsx (line 253: [availableRoles, mappings]). If a search response arrives while the user is still typing within the 300ms window, the pending debounce is lost and the keystroke is silently dropped.

A more robust approach is to use a ref to track mutable state, keeping the debounced function stable.

Edge Case: fetchRoles captures stale selectedRoles via closure

📄 openmetadata-ui/src/main/resources/ui/src/components/Settings/Users/UsersProfile/UserProfileRoles/UserProfileRoles.component.tsx:69-83 📄 openmetadata-ui/src/main/resources/ui/src/components/Settings/Bot/BotDetails/BotDetails.component.tsx:78-92
In UserProfileRoles.component.tsx (line 69-91) and BotDetails.component.tsx (line 78-95), fetchRoles is wrapped in useCallback with selectedRoles as a dependency. Inside, it uses selectedRoles to filter preserved roles. Because setRoles uses the callback form but references selectedRoles from the outer closure (not from prevRoles), if selectedRoles changes between when the debounce fires and when the state update runs, the preserved set may be stale.

This is a minor issue because the debounce is also recreated when selectedRoles changes (via the useCallback → useMemo chain), but it creates an unnecessary coupling. Using a functional approach that derives selected IDs from prevRoles and a ref would be cleaner.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@github-actions
Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 61%
61.93% (61814/99798) 42.06% (33047/78561) 45.11% (9785/21689)

@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown
Contributor

🔴 Playwright Results — 5 failure(s), 25 flaky

✅ 3186 passed · ❌ 5 failed · 🟡 25 flaky · ⏭️ 843 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 294 0 5 4
🔴 Shard 2 0 1 1 765
🔴 Shard 3 725 2 5 7
🟡 Shard 4 755 0 4 18
🔴 Shard 5 684 1 2 41
🔴 Shard 6 728 1 8 8

Genuine Failures (failed on all attempts)

auth.setup.ts › authenticate all users (shard 2)
TimeoutError: page.goto: Timeout 60000ms exceeded.
Call log:
�[2m  - navigating to "http://localhost:8585/", waiting until "load"�[22m

Features/Permission.spec.ts › Permissions (shard 3)
�[31mTest timeout of 60000ms exceeded while setting up "userPage".�[39m
Features/SSOConfiguration.spec.ts › should render authReassignRoles as a searchable dropdown and support role selection, removal, and search filtering (shard 3)
�[31mTest timeout of 60000ms exceeded.�[39m
Pages/Entity.spec.ts › User should be denied access to edit description when deny policy rule is applied on an entity (shard 5)
�[31m"beforeAll" hook timeout of 60000ms exceeded.�[39m
Features/AutoPilot.spec.ts › Agents created by AutoPilot should be deleted (shard 6)
�[31mTest timeout of 60000ms exceeded.�[39m
🟡 25 flaky test(s) (passed on retry)
  • Features/DataAssetRulesEnabled.spec.ts › Verify the SearchIndex Entity Action items after rules is Enabled (shard 1, 1 retry)
  • Features/DataAssetRulesEnabled.spec.ts › Verify the Dashboard Service Entity Action items after rules is Enabled (shard 1, 2 retries)
  • Features/DataAssetRulesEnabled.spec.ts › Verify the Drive Service Entity Action items after rules is Enabled (shard 1, 2 retries)
  • Features/DataAssetRulesEnabled.spec.ts › should enforce single domain selection for glossary term when entity rules are enabled (shard 1, 2 retries)
  • Pages/UserCreationWithPersona.spec.ts › Create user with persona and verify on profile (shard 1, 1 retry)
  • entity-data.teardown.ts › cleanup entity data prerequisites (shard 2, 1 retry)
  • Features/Permissions/DataProductPermissions.spec.ts › Data Product allow operations (shard 3, 2 retries)
  • Features/Permissions/DataProductPermissions.spec.ts › Data Product deny operations (shard 3, 2 retries)
  • Features/RTL.spec.ts › Verify Following widget functionality (shard 3, 1 retry)
  • Features/SSOConfiguration.spec.ts › should support full LDAP role mapping flow: add, fill, open roles dropdown, detect and resolve duplicates, and remove (shard 3, 2 retries)
  • Features/Table.spec.ts › should persist current page (shard 3, 1 retry)
  • Pages/CustomProperties.spec.ts › Set & Update all CP types on topic (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for MlModel (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate Owner Rule Is_Set (shard 4, 1 retry)
  • Pages/DomainAdvanced.spec.ts › Remove multiple assets from domain at once (shard 4, 1 retry)
  • Pages/Entity.spec.ts › User should be denied access to edit description when deny policy rule is applied on an entity (shard 5, 2 retries)
  • Pages/Entity.spec.ts › Tag Add, Update and Remove (shard 5, 2 retries)
  • Features/AutoPilot.spec.ts › Create Service and check the AutoPilot status (shard 6, 2 retries)
  • Features/AutoPilot.spec.ts › Create Service and check the AutoPilot status (shard 6, 1 retry)
  • Features/AutoPilot.spec.ts › Create Service and check the AutoPilot status (shard 6, 2 retries)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 6, 1 retry)
  • Pages/Lineage/LineageRightPanel.spec.ts › Verify custom properties tab IS visible for supported type: searchIndex (shard 6, 1 retry)
  • Pages/ODCSImportExport.spec.ts › Multi-object ODCS contract - object selector shows all schema objects (shard 6, 1 retry)
  • Pages/UserDetails.spec.ts › Create team with domain and verify visibility of inherited domain in user profile after team removal (shard 6, 1 retry)
  • Pages/Users.spec.ts › Admin soft & hard delete and restore user (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant