Conversation
This updates our dependencies to address security concerns flagged by dependabot. It also makes a couple small changes based on issues that arose from these dependency updates.
Reviewer's GuideRefactors platform service helpers to accept a Chrome API instance instead of using the useChrome hook directly, updates the useUser hook and its tests accordingly, and removes an obsolete SCSS import along with the node-sass dependency as part of dependency security updates. Sequence diagram for updated useUser hook interactionssequenceDiagram
actor UserComponent
participant useUser
participant useChrome
participant useQuery
participant authenticateUser
participant getUserRbacPermissions
participant chrome
UserComponent->>useUser: call useUser()
useUser->>useChrome: useChrome()
useChrome-->>useUser: ChromeAPI instance
useUser->>useQuery: useQuery({ queryKey: ['user'], queryFn })
activate useQuery
useQuery->>authenticateUser: authenticateUser(chrome)
authenticateUser->>chrome: chrome.auth.getUser()
chrome-->>authenticateUser: user
useQuery->>getUserRbacPermissions: getUserRbacPermissions(chrome)
getUserRbacPermissions->>chrome: chrome.getUserPermissions('subscriptions')
chrome-->>getUserRbacPermissions: RbacPermission[]
useQuery-->>UserComponent: User data with permissions
deactivate useQuery
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
platformServices.ts,useChromeis now unused after switching toauthenticateUser(chrome)andgetUserRbacPermissions(chrome)and can be removed to avoid dead imports. src/utilities/__tests__/platformServices.test.tslikely needs to be updated to callauthenticateUser/getUserRbacPermissionswith a mockedChromeAPIinstead of assuming those functions obtainchromeinternally, otherwise the tests will no longer align with the new API.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `platformServices.ts`, `useChrome` is now unused after switching to `authenticateUser(chrome)` and `getUserRbacPermissions(chrome)` and can be removed to avoid dead imports.
- `src/utilities/__tests__/platformServices.test.ts` likely needs to be updated to call `authenticateUser`/`getUserRbacPermissions` with a mocked `ChromeAPI` instead of assuming those functions obtain `chrome` internally, otherwise the tests will no longer align with the new API.
## Individual Comments
### Comment 1
<location path="src/hooks/useUser.ts" line_range="17-18" />
<code_context>
queryFn: async () => {
- const userStatus = await authenticateUser;
- const rawRbacPermissions = await userRbacPermissions;
+ const userStatus = await authenticateUser(chrome);
+ const rawRbacPermissions = await getUserRbacPermissions(chrome);
const rbacPermissions = rawRbacPermissions.map((rawPermission) => rawPermission.permission);
const user: User = {
</code_context>
<issue_to_address>
**suggestion (performance):** Run authentication and RBAC permission fetching in parallel to reduce latency.
These two awaits are independent, so they’re now adding their latencies (`auth + rbac`). Consider running them in parallel to reduce query time:
```ts
const [userStatus, rawRbacPermissions] = await Promise.all([
authenticateUser(chrome),
getUserRbacPermissions(chrome),
]);
```
This preserves behavior while improving responsiveness.
</issue_to_address>
### Comment 2
<location path="src/hooks/__tests__/useUser.test.tsx" line_range="19-21" />
<code_context>
+ getUserRbacPermissions: jest.fn()
+}));
+
+jest.mock('@redhat-cloud-services/frontend-components/useChrome', () => ({
+ __esModule: true,
+ default: () => ({})
}));
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the useChrome mock by asserting the hook passes the chrome instance into authenticateUser/getUserRbacPermissions
Since `useUser` now creates a `chrome` instance via `useChrome` and passes it into `authenticateUser` / `getUserRbacPermissions`, the test should verify that wiring. For example, have the mock return a stable object (e.g. `const mockChrome = { ... }`) and assert:
- `expect(authenticateUser).toHaveBeenCalledWith(mockChrome)`
- `expect(getUserRbacPermissions).toHaveBeenCalledWith(mockChrome)`
This will cover the new parameterization and catch regressions if `chrome` stops being passed through correctly.
Suggested implementation:
```typescript
const mockChrome = { isBeta: false };
jest.mock('../../utilities/platformServices', () => ({
...(jest.requireActual('../../utilities/platformServices') as Record<string, unknown>),
authenticateUser: jest.fn(),
getUserRbacPermissions: jest.fn()
}));
jest.mock('../../utilities/platformServices', () => ({
```
```typescript
jest.mock('@redhat-cloud-services/frontend-components/useChrome', () => ({
__esModule: true,
default: () => mockChrome
}));
```
To fully implement your suggestion, you also need to update the test assertions (inside the relevant `it`/`test` blocks that exercise `useUser`) to verify that the chrome instance from the hook is passed into `authenticateUser` and `getUserRbacPermissions`.
After the code path that triggers those calls (e.g. after rendering the hook and awaiting any async effects), add assertions like:
```ts
import { authenticateUser, getUserRbacPermissions } from '../../utilities/platformServices';
// ...
expect(authenticateUser).toHaveBeenCalledWith(mockChrome);
expect(getUserRbacPermissions).toHaveBeenCalledWith(mockChrome);
```
If `authenticateUser` or `getUserRbacPermissions` accept additional parameters in your codebase, adjust the expectations accordingly, for example using `toHaveBeenCalledWith(mockChrome, expectedArg2, ...)` or `toHaveBeenCalledWith(expect.objectContaining(mockChrome), ...)`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
lukegrn
changed the title
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
1 participant
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.
This updates our dependencies to address security concerns flagged by dependabot. It also makes a couple small changes based on issues that arose from these dependency updates.
Summary by Sourcery
Refactor user authentication and permission utilities to accept a Chrome API instance instead of using the
useChromehook, and update the user hook, tests, and build dependencies accordingly.Enhancements:
ChromeAPIinstance for authentication and RBAC permissions instead of relying onuseChromeinternally.useUserhook to obtain the Chrome instance itself and call the refactored authentication and permissions helpers.Build:
node-sassdependency from the project.Tests:
Chores: