feat: add new role IsDatabaseAllowed and shrink breadcrumbs #2672

Raubzeug · 2025-08-04T14:43:13Z

CI Results

Test Status: ⚠️ FLAKY

Total	Passed	Failed	Flaky	Skipped
354	348	0	4	2

Test Changes Summary ✨1 ⏭️2

✨ New Tests (1)

ACL tab shows redirect message and link to Diagnostics (tenant/summary/objectSummary.test.ts)

⏭️ Skipped Tests (2)

Scroll to row, get shareable link, navigate to URL and verify row is scrolled into view (tenant/diagnostics/tabs/queries.test.ts)
Copy result button copies to clipboard (tenant/queryEditor/queryEditor.test.ts)

Bundle Size: ✅

Current: 85.35 MB | Main: 85.35 MB
Diff: +1.08 KB (0.00%)

✅ Bundle size unchanged.

ℹ️ CI Information

Test recordings for failed tests are available in the full report.
Bundle size is measured for the entire 'dist' directory.
📊 indicates links to detailed reports.
🔺 indicates increase, 🔽 decrease, and ✅ no change in bundle size.

astandrik · 2025-08-05T08:06:29Z

bugbot run

astandrik · 2025-08-05T08:06:38Z

/claude_review

claude · 2025-08-05T08:07:08Z

Claude finished @astandrik's task —— View job

Code Review for PR #2672: Add new role IsDatabaseAllowed and shrink breadcrumbs

I've reviewed all 7 changed files in this PR. Here's my comprehensive feedback:

✅ Positive Aspects

TypeScript Type Safety: Properly adds IsDatabaseAllowed to the TUserToken interface with correct optional boolean type
Redux Patterns: Follows Redux Toolkit patterns correctly with selectors and state management
React Hooks: Adds useIsDatabaseOnlyUser hook following existing patterns
i18n Compliance: All user-facing strings use i18n correctly (no hardcoded strings found)
BEM Naming: Component styling follows the cn() utility pattern correctly

🔴 Critical Issue

src/store/reducers/authentication/authentication.ts:39 - The logic for determining isDatabaseOnlyUser appears incorrect:

state.isDatabaseOnlyUser = IsDatabaseAllowed && IsViewerAllowed === false;

This implementation contradicts the purpose of IsDatabaseAllowed. Based on the field's documentation ("Is user allowed to view only database specific data"), a user with IsDatabaseAllowed: true should be considered a database-only user regardless of other permissions.

Suggested fix:

state.isDatabaseOnlyUser = IsDatabaseAllowed === true;

🟡 Suggestions for Improvement

Type Safety Enhancement: Consider making isDatabaseOnlyUser consistent across types:
- In AuthenticationState, it's optional (isDatabaseOnlyUser?: boolean)
- In breadcrumb options, it's also optional
- Consider whether this should be a required boolean with a default value to avoid undefined checks
Breadcrumb Logic: The breadcrumb hiding implementation is clean, but consider extracting the repeated pattern:
```
if (options.isDatabaseOnlyUser) {
    return [];
}
```
This could be a utility function to reduce duplication across getClustersBreadcrumbs and getClusterBreadcrumbs.
Hook Naming: The hook name useIsDatabaseOnlyUser returns a boolean, which is good. The pattern is consistent with useIsUserAllowedToMakeChanges.

✅ Requirements Met

✅ All API calls use window.api pattern
✅ No state mutations in RTK Query
✅ Proper error handling and loading states preserved
✅ React Router v5 patterns followed
✅ No hardcoded user-facing strings
✅ TypeScript interfaces properly prefixed with 'T'
✅ Gravity UI components used appropriately
✅ Performance considerations maintained (useMemo for breadcrumbs)

Summary

The implementation is mostly solid and follows project conventions well. The main concern is the logic for determining database-only users, which should be corrected to match the field's intended purpose. Once that's fixed, this PR will properly implement the access control feature for database-only users.

Review Progress:

Review authentication logic and types
Check UI components and breadcrumb implementation
Verify hooks and selectors
Check for i18n compliance
Validate overall implementation approach

cursor

✅ Bugbot reviewed your changes and found no bugs!

Raubzeug · 2025-08-05T08:19:03Z

/claude_review

claude · 2025-08-05T08:19:27Z

Claude finished @Raubzeug's task —— View job

Code Review for PR #2672: Add new role IsDatabaseAllowed and shrink breadcrumbs

I've reviewed all 7 changed files in this PR. Here's my comprehensive feedback:

✅ Positive Aspects

TypeScript Type Safety: Properly adds IsDatabaseAllowed to the TUserToken interface with correct optional boolean type
Redux Patterns: Follows Redux Toolkit patterns correctly with selectors and state management
React Hooks: Adds useIsDatabaseOnlyUser hook following existing patterns
i18n Compliance: All user-facing strings use i18n correctly (no hardcoded strings found)
BEM Naming: Component styling follows the cn() utility pattern correctly

🔴 Critical Issue

src/store/reducers/authentication/authentication.ts:39 - The logic for determining isDatabaseOnlyUser appears incorrect:

state.isDatabaseOnlyUser = IsDatabaseAllowed && IsViewerAllowed === false;

This implementation contradicts the purpose of IsDatabaseAllowed. Based on the field's documentation ("Is user allowed to view only database specific data"), a user with IsDatabaseAllowed: true should be considered a database-only user regardless of other permissions.

Suggested fix:

state.isDatabaseOnlyUser = IsDatabaseAllowed === true;

🟡 Suggestions for Improvement

Type Safety Enhancement: Consider making isDatabaseOnlyUser consistent across types:
- In AuthenticationState, it's optional (isDatabaseOnlyUser?: boolean)
- In breadcrumb options, it's also optional
- Consider whether this should be a required boolean with a default value to avoid undefined checks
Breadcrumb Logic: The breadcrumb hiding implementation is clean, but consider extracting the repeated pattern:
```
if (options.isDatabaseOnlyUser) {
    return [];
}
```
This could be a utility function to reduce duplication across getClustersBreadcrumbs and getClusterBreadcrumbs.
Hook Naming: The hook name useIsDatabaseOnlyUser returns a boolean, which is good. The pattern is consistent with useIsUserAllowedToMakeChanges.

✅ Requirements Met

✅ All API calls use window.api pattern
✅ No state mutations in RTK Query
✅ Proper error handling and loading states preserved
✅ React Router v5 patterns followed
✅ No hardcoded user-facing strings
✅ TypeScript interfaces properly prefixed with 'T'
✅ Gravity UI components used appropriately
✅ Performance considerations maintained (useMemo for breadcrumbs)

Summary

The implementation is mostly solid and follows project conventions well. The main concern is the logic for determining database-only users, which should be corrected to match the field's intended purpose. Once that's fixed, this PR will properly implement the access control feature for database-only users.

Review Progress:

Review authentication logic and types
Check UI components and breadcrumb implementation
Verify hooks and selectors
Check for i18n compliance
Validate overall implementation approach

Copilot

Pull Request Overview

This PR introduces a new user role IsDatabaseAllowed to the authentication system and modifies breadcrumb visibility based on user permissions. The changes allow for more granular access control by distinguishing between database-specific viewers and full viewers.

Key changes:

Added IsDatabaseAllowed role to the API type definitions and authentication state
Modified breadcrumbs to be hidden for non-viewer users
Enhanced the authentication system to track viewer user status

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`src/types/api/whoami.ts`	Added new `IsDatabaseAllowed` boolean field to `TUserToken` interface
`src/store/reducers/authentication/types.ts`	Added `isViewerUser` boolean to authentication state
`src/store/reducers/authentication/authentication.ts`	Updated reducer to handle `IsViewerAllowed` flag and added selector
`src/utils/hooks/useIsUserAllowedToMakeChanges.ts`	Exported new `useIsViewerUser` hook
`src/store/reducers/header/types.ts`	Added `isViewerUser` option to breadcrumb configuration
`src/containers/Header/breadcrumbs.tsx`	Modified breadcrumb functions to hide breadcrumbs for non-viewer users
`src/containers/Header/Header.tsx`	Integrated viewer user status into breadcrumb rendering logic

Copilot · 2025-08-05T08:20:00Z

src/store/reducers/authentication/authentication.ts

@@ -35,17 +35,19 @@ export const slice = createSlice({
            // Otherwise every user is allowed to make changes
            // Anyway there will be guards on backend
            state.isUserAllowedToMakeChanges = IsMonitoringAllowed !== false;
+            state.isViewerUser = IsViewerAllowed;


The isViewerUser field should consider both IsViewerAllowed and IsDatabaseAllowed flags. According to the API documentation, IsViewerAllowed includes IsDatabaseAllowed rights, so a user with either flag should be considered a viewer user.

Suggested change

state.isViewerUser = IsViewerAllowed;

state.isViewerUser = IsViewerAllowed || IsDatabaseAllowed;

IsViewerAllowed includes IsDatabaseAllowed and some more rights. So, if user has only IsDatabaseAllowed - it is not enough to be isViewerUser

Copilot · 2025-08-05T08:20:01Z

src/store/reducers/authentication/authentication.ts

@@ -26,7 +26,7 @@ export const slice = createSlice({
            }
        },
        setUser: (state, action: PayloadAction<TUserToken>) => {
-            const {UserSID, AuthType, IsMonitoringAllowed} = action.payload;
+            const {UserSID, AuthType, IsMonitoringAllowed, IsViewerAllowed} = action.payload;


The destructuring should also include IsDatabaseAllowed since this new field was added to the API type and should be considered when determining viewer user status.

Suggested change

const {UserSID, AuthType, IsMonitoringAllowed, IsViewerAllowed} = action.payload;

const {UserSID, AuthType, IsMonitoringAllowed, IsViewerAllowed, IsDatabaseAllowed} = action.payload;

feat: add new role IsDatabaseAllowed and shrink breadcrumbs

02256ea

Raubzeug requested review from artemmufazalov, astandrik and adameat as code owners August 4, 2025 14:43

Raubzeug enabled auto-merge August 4, 2025 15:04

astandrik requested a review from Copilot August 5, 2025 08:06

This comment was marked as outdated.

Sign in to view

cursor bot reviewed Aug 5, 2025

View reviewed changes

fixes

f6d277d

Raubzeug force-pushed the dbuser branch from df2c7b4 to f6d277d Compare August 5, 2025 08:18

Raubzeug requested a review from Copilot August 5, 2025 08:19

Copilot AI reviewed Aug 5, 2025

View reviewed changes

astandrik approved these changes Aug 5, 2025

View reviewed changes

Raubzeug added this pull request to the merge queue Aug 5, 2025

Merged via the queue into main with commit 66b45ee Aug 5, 2025
5 checks passed

Raubzeug deleted the dbuser branch August 5, 2025 08:39

ydb-platform-bot mentioned this pull request Aug 4, 2025

chore(main): release 11.6.0 #2667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add new role IsDatabaseAllowed and shrink breadcrumbs #2672

feat: add new role IsDatabaseAllowed and shrink breadcrumbs #2672

Uh oh!

Raubzeug commented Aug 4, 2025 •

edited by github-actions bot

Loading

Uh oh!

astandrik commented Aug 5, 2025

Uh oh!

astandrik commented Aug 5, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

claude bot commented Aug 5, 2025 •

edited

Loading

Uh oh!

cursor bot left a comment

Uh oh!

Raubzeug commented Aug 5, 2025

Uh oh!

claude bot commented Aug 5, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 5, 2025

Uh oh!

Raubzeug Aug 5, 2025

Uh oh!

Copilot AI Aug 5, 2025

Uh oh!

Uh oh!

Uh oh!

	state.isViewerUser = IsViewerAllowed;
	state.isViewerUser = IsViewerAllowed \|\| IsDatabaseAllowed;

	const {UserSID, AuthType, IsMonitoringAllowed, IsViewerAllowed} = action.payload;
	const {UserSID, AuthType, IsMonitoringAllowed, IsViewerAllowed, IsDatabaseAllowed} = action.payload;

feat: add new role IsDatabaseAllowed and shrink breadcrumbs #2672

feat: add new role IsDatabaseAllowed and shrink breadcrumbs #2672

Uh oh!

Conversation

Raubzeug commented Aug 4, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Results

Test Status: ⚠️ FLAKY

✨ New Tests (1)

⏭️ Skipped Tests (2)

Bundle Size: ✅

Uh oh!

astandrik commented Aug 5, 2025

Uh oh!

astandrik commented Aug 5, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

claude bot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for PR #2672: Add new role IsDatabaseAllowed and shrink breadcrumbs

✅ Positive Aspects

🔴 Critical Issue

🟡 Suggestions for Improvement

✅ Requirements Met

Summary

Review Progress:

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

✅ Bugbot reviewed your changes and found no bugs!

Uh oh!

Raubzeug commented Aug 5, 2025

Uh oh!

claude bot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for PR #2672: Add new role IsDatabaseAllowed and shrink breadcrumbs

✅ Positive Aspects

🔴 Critical Issue

🟡 Suggestions for Improvement

✅ Requirements Met

Summary

Review Progress:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Raubzeug Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Raubzeug commented Aug 4, 2025 •

edited by github-actions bot

Loading

claude bot commented Aug 5, 2025 •

edited

Loading

claude bot commented Aug 5, 2025 •

edited

Loading