Skip to content

Conversation

@Raubzeug
Copy link
Contributor

@Raubzeug Raubzeug commented Oct 21, 2025

closes #2956

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
378 375 0 1 2
Test Changes Summary ⏭️2

⏭️ Skipped Tests (2)

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

Bundle Size: 🔺

Current: 45.87 MB | Main: 45.86 MB
Diff: +4.71 KB (0.01%)

⚠️ Bundle size increased. Please review.

ℹ️ 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.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

const authUnavailable = useClusterWithoutAuthInUI();

const location = useLocation();
const isClustersPage = location.pathname === '/clusters';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: hardcoded pathname check won't work with basename (e.g., /monitoring/clusters). use location.pathname.endsWith(routes.clusters) or match against the route pattern

Suggested change
const isClustersPage = location.pathname === '/clusters';
const isClustersPage = location.pathname.endsWith(routes.clusters);

const needDatabase = useLoginWithDatabase();

const [authenticate, {isLoading}] = authenticationApi.useAuthenticateMutation(undefined);
const isClustersPage = location.pathname === '/clusters';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: hardcoded pathname check won't work with basename (e.g., /monitoring/clusters). use location.pathname.endsWith(routes.clusters) or match against the route pattern

Suggested change
const isClustersPage = location.pathname === '/clusters';
const isClustersPage = location.pathname.endsWith(routes.clusters);

@astandrik astandrik requested a review from Copilot October 22, 2025 11:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds authentication support for the clusters page using meta API endpoints. When meta login capability is available, the authentication flow uses /meta/login and /meta/whoami instead of the standard viewer endpoints.

Key Changes:

  • Added meta authentication endpoints (/meta/login capability and API methods)
  • Modified authentication flows to conditionally use meta endpoints based on page context and capability availability
  • Enhanced route detection with reusable helper functions

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/types/api/capabilities.ts Added /meta/login capability type
src/store/reducers/capabilities/hooks.ts Added hook to check meta login availability
src/store/reducers/authentication/authentication.ts Modified authentication mutations to support meta endpoints
src/services/api/meta.ts Implemented meta authentication API methods
src/routes.ts Added reusable route checking helper functions
src/containers/Header/Header.tsx Refactored to use new route helper functions
src/containers/Authentication/Authentication.tsx Added meta authentication support in login form
src/containers/App/Content.tsx Added conditional meta user authentication wrapper for clusters page

const {useMeta, ...rest} = params;
let data;
if (useMeta) {
data = await window.api.meta?.metaAuthenticate(rest);
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional chaining on window.api.meta?.metaAuthenticate could result in data being undefined if window.api.meta is null/undefined. This would cause the subsequent dispatch(setIsAuthenticated(true)) to execute even when authentication fails. Add a null check or use non-null assertion if the condition guarantees window.api.meta exists.

Suggested change
data = await window.api.meta?.metaAuthenticate(rest);
if (!window.api.meta) {
throw new Error('Meta API is not available for authentication');
}
data = await window.api.meta.metaAuthenticate(rest);

Copilot uses AI. Check for mistakes.
const isClustersPage = checkIsClustersPage(location.pathname);
const isMetaLoginAvailable = useMetaLoginAvailable();

const [authenticate, {isLoading}] = authenticationApi.useAuthenticateMutation();
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The undefined parameter was removed but the function call was changed from useAuthenticateMutation(undefined) to useAuthenticateMutation(). While functionally equivalent, the explicit change in the diff suggests this may have been intentional. Verify this change is necessary and not just a formatting modification.

Copilot uses AI. Check for mistakes.
Comment on lines +151 to +155
return pathname.endsWith(routes.clusters);
}

export function checkIsTenantPage(pathname: string) {
return pathname.endsWith(routes.tenant);
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using endsWith() for route matching is fragile and will produce false positives. For example, /some/other/clusters would match even though it's not the clusters page. Use exact matching or a more robust pattern matching approach: pathname === routes.clusters or use a library like matchPath from react-router.

Suggested change
return pathname.endsWith(routes.clusters);
}
export function checkIsTenantPage(pathname: string) {
return pathname.endsWith(routes.tenant);
return pathname === routes.clusters;
}
export function checkIsTenantPage(pathname: string) {
return pathname === routes.tenant;

Copilot uses AI. Check for mistakes.
Comment on lines +32 to +33
metaAuthenticate(params: {user: string; password: string}) {
return this.post(this.getPath('/meta/login'), params, {});
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The metaAuthenticate method lacks a return type annotation. Based on the usage in authentication.ts where the result is dispatched, add an explicit return type such as Promise<TUserToken> or the appropriate type for the authentication response.

Suggested change
metaAuthenticate(params: {user: string; password: string}) {
return this.post(this.getPath('/meta/login'), params, {});
metaAuthenticate(params: {user: string; password: string}): Promise<TUserToken> {
return this.post<TUserToken>(this.getPath('/meta/login'), params, {});

Copilot uses AI. Check for mistakes.
@artemmufazalov artemmufazalov added this pull request to the merge queue Oct 22, 2025
Merged via the queue into main with commit e5de2d4 Oct 22, 2025
7 checks passed
@artemmufazalov artemmufazalov deleted the clusters-auth branch October 22, 2025 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Request auth on cluster list page

2 participants