feat: (UI) Hide User/Profile API Key Features in UI if depending on parameter value BED-7137#2418
Open
Useinovski wants to merge 13 commits intomainfrom
Open
feat: (UI) Hide User/Profile API Key Features in UI if depending on parameter value BED-7137#2418Useinovski wants to merge 13 commits intomainfrom
Useinovski wants to merge 13 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds conditional rendering of API token management features throughout the UI based on configuration settings. It includes a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/utils/object.ts (1)
20-20: Consider avoiding runtime API-surface expansion fornoop.If this helper is only for test setup, keep it in test utilities instead of exporting it from shared runtime utils to reduce long-term public API maintenance.
Based on learnings: "Avoid exporting internal, implementation-only modules that aren’t meant for external consumption; document and surface stable APIs for downstream users."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/object.ts` at line 20, The exported runtime helper noop is expanding the public API surface; remove the export from packages/javascript/bh-shared-ui/src/utils/object.ts (stop exporting or delete the noop constant) and move it into your test utilities (e.g., a tests/utils or __tests__/helpers file) so only test code imports it; update any modules that currently import noop to instead import the new test-only helper, or replace usages with inline no-op functions where appropriate, ensuring noop is no longer part of the shared runtime exports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/javascript/bh-shared-ui/src/views/UserProfile/UserProfile.test.tsx`:
- Around line 212-217: The test asserts the "API Key Management" button is
absent before the async configuration query settles; update the test around
QueryClient, UserProfile and configurationKeys.all to wait for the config query
to finish (e.g., use testing-library's waitFor or waitForElementToBeRemoved) and
then assert that screen.queryByRole('button', { name: 'API Key Management' }) is
not in the document; ensure the wait targets a stable indicator of query
completion (like waiting for QueryClient to stop fetching or for a known loading
indicator to disappear) before making the non-presence assertion.
In `@packages/javascript/bh-shared-ui/src/views/Users/UserActionsMenu.test.tsx`:
- Around line 112-124: The test "should not display generate/revoke api tokens
button" races with the async /api/v2/config response; make the assertion
deterministic by awaiting the config fetch completion before checking for the
menu item. Update the test in UserActionsMenu.test.tsx (the it block with name
'should not display generate/revoke api tokens button') to wait for the
fetch—e.g. await screen.findBy... for an element rendered after config loads or
wrap the assertion in await waitFor(() => expect(screen.queryByRole('menuitem',
{ name: /generate \/ revoke api tokens/i })).not.toBeInTheDocument())—so the
query runs only after the mocked /api/v2/config handler has resolved.
---
Nitpick comments:
In `@packages/javascript/bh-shared-ui/src/utils/object.ts`:
- Line 20: The exported runtime helper noop is expanding the public API surface;
remove the export from packages/javascript/bh-shared-ui/src/utils/object.ts
(stop exporting or delete the noop constant) and move it into your test
utilities (e.g., a tests/utils or __tests__/helpers file) so only test code
imports it; update any modules that currently import noop to instead import the
new test-only helper, or replace usages with inline no-op functions where
appropriate, ensuring noop is no longer part of the shared runtime exports.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/javascript/bh-shared-ui/src/utils/object.tspackages/javascript/bh-shared-ui/src/views/UserProfile/UserProfile.test.tsxpackages/javascript/bh-shared-ui/src/views/UserProfile/UserProfile.tsxpackages/javascript/bh-shared-ui/src/views/Users/UserActionsMenu.test.tsxpackages/javascript/bh-shared-ui/src/views/Users/UserActionsMenu.tsx
Comment on lines
+212
to
+217
| const queryClient = new QueryClient(); | ||
| render(<UserProfile />, { queryClient }); | ||
|
|
||
| await queryClient.invalidateQueries(configurationKeys.all); | ||
| const apiKeyManagementButton = screen.queryByRole('button', { name: 'API Key Management' }); | ||
| expect(apiKeyManagementButton).not.toBeInTheDocument(); |
Contributor
There was a problem hiding this comment.
Stabilize the disabled-state assertion to avoid false positives.
At Line 216, absence is asserted immediately; this can pass before the async config query settles. Wait for configuration query completion first, then assert non-presence.
💡 Suggested test hardening
const queryClient = new QueryClient();
render(<UserProfile />, { queryClient });
- await queryClient.invalidateQueries(configurationKeys.all);
- const apiKeyManagementButton = screen.queryByRole('button', { name: 'API Key Management' });
- expect(apiKeyManagementButton).not.toBeInTheDocument();
+ await waitFor(() =>
+ expect(queryClient.getQueryState(configurationKeys.all)?.status).toBe('success')
+ );
+ expect(screen.queryByRole('button', { name: 'API Key Management' })).not.toBeInTheDocument();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const queryClient = new QueryClient(); | |
| render(<UserProfile />, { queryClient }); | |
| await queryClient.invalidateQueries(configurationKeys.all); | |
| const apiKeyManagementButton = screen.queryByRole('button', { name: 'API Key Management' }); | |
| expect(apiKeyManagementButton).not.toBeInTheDocument(); | |
| const queryClient = new QueryClient(); | |
| render(<UserProfile />, { queryClient }); | |
| await waitFor(() => | |
| expect(queryClient.getQueryState(configurationKeys.all)?.status).toBe('success') | |
| ); | |
| expect(screen.queryByRole('button', { name: 'API Key Management' })).not.toBeInTheDocument(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/javascript/bh-shared-ui/src/views/UserProfile/UserProfile.test.tsx`
around lines 212 - 217, The test asserts the "API Key Management" button is
absent before the async configuration query settles; update the test around
QueryClient, UserProfile and configurationKeys.all to wait for the config query
to finish (e.g., use testing-library's waitFor or waitForElementToBeRemoved) and
then assert that screen.queryByRole('button', { name: 'API Key Management' }) is
not in the document; ensure the wait targets a stable indicator of query
completion (like waiting for QueryClient to stop fetching or for a known loading
indicator to disappear) before making the non-presence assertion.
Comment on lines
+112
to
+124
| it('should not display generate/revoke api tokens button', async () => { | ||
| server.use( | ||
| rest.get(`/api/v2/config`, async (_req, res, ctx) => { | ||
| return res(ctx.json(CONFIG_DISABLED_RESPONSE)); | ||
| }) | ||
| ); | ||
| const { user } = setup(); | ||
|
|
||
| const button = screen.getByRole('button', { name: /show user actions/i }); | ||
| await user.click(button); | ||
|
|
||
| const apiKeyManagementButton = screen.queryByRole('menuitem', { name: /generate \/ revoke api tokens/i }); | ||
| expect(apiKeyManagementButton).not.toBeInTheDocument(); |
Contributor
There was a problem hiding this comment.
Make the disabled-path check deterministic after config fetch.
At Line 123, immediate queryByRole can pass before /api/v2/config resolves. Gate the assertion on observing the config request completion.
💡 Suggested test hardening
-import { render, screen } from '../../test-utils';
+import { render, screen, waitFor } from '../../test-utils';
it('should not display generate/revoke api tokens button', async () => {
+ let configRequested = false;
server.use(
rest.get(`/api/v2/config`, async (_req, res, ctx) => {
+ configRequested = true;
return res(ctx.json(CONFIG_DISABLED_RESPONSE));
})
);
const { user } = setup();
const button = screen.getByRole('button', { name: /show user actions/i });
await user.click(button);
+ await waitFor(() => expect(configRequested).toBe(true));
const apiKeyManagementButton = screen.queryByRole('menuitem', { name: /generate \/ revoke api tokens/i });
expect(apiKeyManagementButton).not.toBeInTheDocument();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should not display generate/revoke api tokens button', async () => { | |
| server.use( | |
| rest.get(`/api/v2/config`, async (_req, res, ctx) => { | |
| return res(ctx.json(CONFIG_DISABLED_RESPONSE)); | |
| }) | |
| ); | |
| const { user } = setup(); | |
| const button = screen.getByRole('button', { name: /show user actions/i }); | |
| await user.click(button); | |
| const apiKeyManagementButton = screen.queryByRole('menuitem', { name: /generate \/ revoke api tokens/i }); | |
| expect(apiKeyManagementButton).not.toBeInTheDocument(); | |
| import { render, screen, waitFor } from '../../test-utils'; | |
| it('should not display generate/revoke api tokens button', async () => { | |
| let configRequested = false; | |
| server.use( | |
| rest.get(`/api/v2/config`, async (_req, res, ctx) => { | |
| configRequested = true; | |
| return res(ctx.json(CONFIG_DISABLED_RESPONSE)); | |
| }) | |
| ); | |
| const { user } = setup(); | |
| const button = screen.getByRole('button', { name: /show user actions/i }); | |
| await user.click(button); | |
| await waitFor(() => expect(configRequested).toBe(true)); | |
| const apiKeyManagementButton = screen.queryByRole('menuitem', { name: /generate \/ revoke api tokens/i }); | |
| expect(apiKeyManagementButton).not.toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/javascript/bh-shared-ui/src/views/Users/UserActionsMenu.test.tsx`
around lines 112 - 124, The test "should not display generate/revoke api tokens
button" races with the async /api/v2/config response; make the assertion
deterministic by awaiting the config fetch completion before checking for the
menu item. Update the test in UserActionsMenu.test.tsx (the it block with name
'should not display generate/revoke api tokens button') to wait for the
fetch—e.g. await screen.findBy... for an element rendered after config loads or
wrap the assertion in await waitFor(() => expect(screen.queryByRole('menuitem',
{ name: /generate \/ revoke api tokens/i })).not.toBeInTheDocument())—so the
query runs only after the mocked /api/v2/config handler has resolved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This change was to remove the buttons that manage api tokens in the "Manage Users" page as well as the "Profile" page depending on the config flag.
Motivation and Context
Resolves BED-7137
How Has This Been Tested?
Tested locally by flipping the value in the parameter table between true/false.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests