[Workspace] Refresh collaborators page from server on mount

[xluo-aws]

Description

The Workspace Collaborators page renders the permission list from the cached workspaces.currentWorkspace$ BehaviorSubject. If a different user (e.g. an admin) revokes someone's access remotely, the affected user's UI keeps showing the old collaborator list until the page is hard reloaded.

This PR mirrors that pattern for the Collaborators page: on mount (and whenever the workspace id changes, plus after a successful permission update), call workspaceClient.get(currentWorkspace.id) and render the rendered permission list from the server response. The cached currentWorkspace$.permissions is used only as a fallback if the request fails.

Testing the changes

Existing snapshot test for WorkspaceCollaborators retained. Added a new test that asserts:

• workspaceClient.get(workspaceId) is called on mount.
• The row for a collaborator that exists in the cached currentWorkspace$ but not in the server response is no longer rendered.

Existing tests updated to provide workspaceClient.get in their mock services.

Check List

• All tests pass
  • yarn test:jest (only the changed test file was run locally; full suite via CI)
  • yarn test:jest_integration
• New functionality includes testing
• New functionality has been documented in the code (inline comment explaining the staleness rationale)
• Update CHANGELOG.md — will add a fragment under changelogs/fragments/ once a PR number is assigned
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit dcc0b20)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Race Condition If currentWorkspace.id changes while a previous workspaceClient.get() call is still pending, the stale response may overwrite latestPermissions after the newer request completes. This occurs because the cleanup function only sets cancelled = true but does not prevent the .then() callback from calling setLatestPermissions() if the promise resolves after the effect re-runs. The result is that the UI displays permissions for the wrong workspace. useEffect(() => { if (!currentWorkspace?.id || !workspaces) { return; } let cancelled = false; workspaceClient .get(currentWorkspace.id) .then((response) => { if (cancelled) { return; } if (response.success) { setLatestPermissions(response.result.permissions ?? {}); } else { // The user no longer has access to this workspace (e.g. another admin // revoked their permissions while this tab was open). Surface the // error via the same channel that WorkspaceValidationService uses for // stale workspaces so the user is redirected to the fatal-error app // instead of seeing a stale collaborator list. workspaces.workspaceError$.next(response.error ?? 'Workspace is no longer accessible'); } }) // eslint-disable-next-line no-console .catch((error) => console.warn('Failed to refresh workspace collaborators', error)); return () => { cancelled = true; }; }, [currentWorkspace?.id, workspaceClient, workspaces]); Stale Permissions After a successful update at line 128, setLatestPermissions(nextPermissions) is called. However, if the server modifies the permissions during the update (e.g., sanitization, defaults, or concurrent changes by another user), the local state will diverge from the server's actual state. The UI will show the client-side nextPermissions instead of what the server persisted. setLatestPermissions(nextPermissions);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to dcc0b20

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle failed fetch state properly When the workspace fetch fails, latestPermissions remains undefined, causing the component to fall back to the stale currentWorkspace?.permissions. This defeats the purpose of the refresh. Reset latestPermissions to an empty object or the current workspace permissions in the error case to ensure consistent state. src/plugins/workspace/public/components/workspace_collaborators/workspace_collaborators.tsx [87-96] if (response.success) { setLatestPermissions(response.result.permissions ?? {}); } else { + setLatestPermissions({}); workspaces.workspaceError$.next(response.error ?? 'Workspace is no longer accessible'); } Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a potential issue where latestPermissions remains undefined on error, but the proposed fix of setting it to an empty object may not be ideal. When an error occurs, the component pushes to workspaceError$ which should trigger a redirect to the fatal-error app, making the state of latestPermissions less critical.	Low
	Add missing dependency to useEffect The useEffect hook is missing setLatestPermissions in its dependency array, which could lead to stale closures. While setLatestPermissions is a state setter from useState and typically stable, explicitly including it ensures correctness and prevents potential issues if React's behavior changes. src/plugins/workspace/public/components/workspace_collaborators/workspace_collaborators.tsx [76-103] useEffect(() => { if (!currentWorkspace?.id || !workspaces) { return; } let cancelled = false; workspaceClient .get(currentWorkspace.id) .then((response) => { if (cancelled) { return; } if (response.success) { setLatestPermissions(response.result.permissions ?? {}); } else { workspaces.workspaceError$.next(response.error ?? 'Workspace is no longer accessible'); } }) .catch((error) => console.warn('Failed to refresh workspace collaborators', error)); return () => { cancelled = true; }; -}, [currentWorkspace?.id, workspaceClient, workspaces]); +}, [currentWorkspace?.id, workspaceClient, workspaces, setLatestPermissions]); Suggestion importance[1-10]: 2 __ Why: While technically correct, setLatestPermissions is a state setter from useState which React guarantees to be stable across renders. Adding it to the dependency array is unnecessary and doesn't provide meaningful value in this context.	Low

---

Previous suggestions

Suggestions up to commit c893657

Category	Suggestion	Impact
General	Reset state on workspace change Reset latestPermissions to undefined when currentWorkspace?.id changes but before the fetch completes. Without this, switching workspaces will briefly display stale permissions from the previous workspace until the new fetch resolves. src/plugins/workspace/public/components/workspace_collaborators/workspace_collaborators.tsx [76-94] useEffect(() => { if (!currentWorkspace?.id) { return; } let cancelled = false; + setLatestPermissions(undefined); workspaceClient .get(currentWorkspace.id) .then((response) => { if (cancelled || !response.success) { return; } setLatestPermissions(response.result.permissions ?? {}); }) // eslint-disable-next-line no-console .catch((error) => console.warn('Failed to refresh workspace collaborators', error)); return () => { cancelled = true; }; }, [currentWorkspace?.id, workspaceClient]); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential race condition where switching workspaces could briefly display stale permissions. Adding setLatestPermissions(undefined) at the start of the effect ensures the UI shows a loading state or falls back to currentWorkspace?.permissions during the fetch, preventing stale data display.	Medium

Suggestions up to commit f2f7be6

Category	Suggestion	Impact
General	Add error logging for failed refresh The refreshCollaborators function silently swallows errors without logging or notifying the user. When the server fetch fails, users won't know they're viewing potentially stale data. Consider logging the error or showing a warning notification to inform users that the displayed permissions may not be current. src/plugins/workspace/public/components/workspace_collaborators/workspace_collaborators.tsx [76-88] const refreshCollaborators = useCallback(async () => { if (!currentWorkspace?.id || !workspaceClient?.get) { return; } try { const response = await workspaceClient.get(currentWorkspace.id); if (response.success) { setLatestPermissions(response.result.permissions ?? {}); } - } catch { + } catch (error) { + console.error('Failed to refresh collaborators:', error); // Fall back to the cached permissions from `currentWorkspace$`. } }, [currentWorkspace?.id, workspaceClient]); Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that errors are silently swallowed in the refreshCollaborators function. Adding error logging would improve debugging and observability. However, the impact is moderate since the fallback behavior (using cached permissions) is already documented and intentional.	Low

Suggestions up to commit 6eb74cb

Category	Suggestion	Impact
General	Handle unsuccessful server response explicitly When response.success is false, the function doesn't handle the error case. This leaves latestPermissions undefined if the server returns an unsuccessful response, potentially causing the UI to show no collaborators. Consider handling the failure case explicitly or setting permissions from the error response. src/plugins/workspace/public/components/workspace_collaborators/workspace_collaborators.tsx [76-88] const refreshCollaborators = useCallback(async () => { if (!currentWorkspace?.id || !workspaceClient?.get) { return; } try { const response = await workspaceClient.get(currentWorkspace.id); if (response.success) { setLatestPermissions(response.result.permissions ?? {}); + } else { + // Fall back to cached permissions on unsuccessful response + console.warn('Failed to fetch workspace permissions:', response.error); } - } catch { + } catch (error) { // Fall back to the cached permissions from `currentWorkspace$`. + console.error('Failed to refresh workspace collaborators:', error); } }, [currentWorkspace?.id, workspaceClient]); Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that response.success === false is not explicitly handled, which could leave latestPermissions undefined. However, the code already has a fallback mechanism via latestPermissions ?? currentWorkspace?.permissions ?? {} at line 71, so this is a minor improvement for clarity and debugging rather than a critical fix.	Low
	Add error logging for debugging The catch block silently swallows errors without logging or notifying the user. This makes debugging difficult and users won't know if the refresh failed. Consider logging the error or showing a non-intrusive notification to inform users that the latest permissions couldn't be fetched. src/plugins/workspace/public/components/workspace_collaborators/workspace_collaborators.tsx [76-88] const refreshCollaborators = useCallback(async () => { if (!currentWorkspace?.id || !workspaceClient?.get) { return; } try { const response = await workspaceClient.get(currentWorkspace.id); if (response.success) { setLatestPermissions(response.result.permissions ?? {}); } - } catch { + } catch (error) { // Fall back to the cached permissions from `currentWorkspace$`. + console.error('Failed to refresh workspace collaborators:', error); } }, [currentWorkspace?.id, workspaceClient]); Suggestion importance[1-10]: 5 __ Why: Adding error logging is a reasonable improvement for debugging, but the current implementation intentionally falls back to cached permissions silently. The suggestion provides marginal value since the fallback behavior is already documented in the comment.	Low

[github-actions]

Persistent review updated to latest commit f2f7be6

[github-actions]

✅ All unit and integration tests passing

🔗 Workflow run · commit dcc0b2096938b89d2cfe6a7154bae1f959e02518

[xluo-aws]

Pushed c893657c58 addressing the review feedback:

1. Race condition on unmount — accepted. Folded into a single useEffect with a cancelled flag so an in-flight workspaceClient.get can't overwrite a newly-mounted workspace's permissions if the user switches workspaces mid-fetch.
2. Loading state — declined. The same fetch-after-render pattern is used by WorkspaceDetailApp for data sources (no spinner), and the cached currentWorkspace$ is used as the initial paint so the common case (cache == server) shows no flicker. Adding a spinner that flashes for a few hundred ms on every mount felt worse than the current UX.
3. Error visibility — accepted. Replaced the empty catch with console.warn so unexpected fetch failures show up in devtools, while still silently falling back to the cached value (no toast spam).
4. workspaceClient?.get guard — accepted. Dropped the optional chain; in production workspaceClient is always the concrete WorkspaceClient and the test mock now provides get explicitly.
5. Double fetch after mutation — accepted. WorkspaceClient.update's response is just IResponse<boolean> so it doesn't carry the new permissions, but we already have them locally — setLatestPermissions(convertPermissionSettingsToPermissions(settings)) after a successful update avoids the extra round-trip and the small chance of a read-after-write inconsistency.

All 5 unit tests in the file still pass.

[github-actions]

Persistent review updated to latest commit c893657

[xluo-aws]

Verified end-to-end against a security-enabled cluster

Setup: local OpenSearch 3.6.0-SNAPSHOT with security plugin (demo install), local OSD with security-dashboards-plugin cloned in, separate admin and bar users, workspace with admin (write) + bar (read).

Manual repro:

1. admin opens Collaborators → sees admin + bar.
2. bar opens Collaborators in incognito → sees admin + bar.
3. admin revokes bar.
4. bar navigates within the workspace (no hard reload) and back to Collaborators.
5. Before this commit → bar still saw the cached list. After → bar is redirected to the fatal-error app.

[github-actions]

Persistent review updated to latest commit dcc0b20

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (7782a80) to head (dcc0b20).
⚠️ Report is 180 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #12044       +/-   ##
===========================================
- Coverage   61.58%        0   -61.59%     
===========================================
  Files        4995        0     -4995     
  Lines      137542        0   -137542     
  Branches    23901        0    -23901     
===========================================
- Hits        84707        0    -84707     
+ Misses      46692        0    -46692     
+ Partials     6143        0     -6143     

Flag	Coverage Δ
Linux_1	?
Linux_2	?
Linux_3	?
Linux_4	?
Linux_5	?

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

☔ View full report in Codecov by Sentry.
📢 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.