Copilot commented on code in PR #38387:
URL: https://github.com/apache/superset/pull/38387#discussion_r2882224290
##########
superset-frontend/src/features/roles/utils.ts:
##########
@@ -51,3 +54,166 @@ export const updateRoleName = async (roleId: number, name:
string) =>
endpoint: `/api/v1/security/roles/${roleId}`,
jsonPayload: { name },
});
+
+export const formatPermissionLabel = (
+ permissionName: string,
+ viewMenuName: string,
+) => `${permissionName.replace(/_/g, ' ')} ${viewMenuName.replace(/_/g, ' ')}`;
+
+type PermissionResult = {
+ id: number;
+ permission: { name: string };
+ view_menu: { name: string };
+};
+
+const mapPermissionResults = (results: PermissionResult[]) =>
+ results.map(item => ({
+ value: item.id,
+ label: formatPermissionLabel(item.permission.name, item.view_menu.name),
+ }));
+
+const PAGE_SIZE = 1000;
+const CONCURRENCY_LIMIT = 3;
+const MAX_CACHE_ENTRIES = 20;
+const permissionSearchCache = new Map<string, SelectOption[]>();
+
+export const clearPermissionSearchCache = () => {
+ permissionSearchCache.clear();
+};
+
+const fetchPermissionPageRaw = async (queryParams: Record<string, unknown>) =>
{
+ const response = await SupersetClient.get({
+ endpoint:
`/api/v1/security/permissions-resources/?q=${rison.encode(queryParams)}`,
+ });
+ return {
+ data: mapPermissionResults(response.json?.result || []),
+ totalCount: response.json?.count ?? 0,
+ };
+};
+
+const fetchAllPermissionPages = async (
+ filters: Record<string, unknown>[],
+): Promise<SelectOption[]> => {
+ const page0 = await fetchPermissionPageRaw({
+ page: 0,
+ page_size: PAGE_SIZE,
+ filters,
+ });
+ if (page0.data.length === 0 || page0.data.length >= page0.totalCount) {
+ return page0.data;
+ }
+
+ // Use actual returned size — backend may cap below PAGE_SIZE
+ const actualPageSize = page0.data.length;
+ const totalPages = Math.ceil(page0.totalCount / actualPageSize);
+ const allResults = [...page0.data];
+
+ // Fetch remaining pages in batches of CONCURRENCY_LIMIT
+ for (let batch = 1; batch < totalPages; batch += CONCURRENCY_LIMIT) {
+ const batchEnd = Math.min(batch + CONCURRENCY_LIMIT, totalPages);
+ const batchResults = await Promise.all(
+ Array.from({ length: batchEnd - batch }, (_, i) =>
+ fetchPermissionPageRaw({
+ page: batch + i,
+ page_size: PAGE_SIZE,
+ filters,
+ }),
+ ),
+ );
+ for (const r of batchResults) {
+ allResults.push(...r.data);
+ if (r.data.length === 0) return allResults;
+ }
+ if (allResults.length >= page0.totalCount) break;
+ }
+
+ return allResults;
+};
+
+export const fetchPermissionOptions = async (
+ filterValue: string,
+ page: number,
+ pageSize: number,
+ addDangerToast: (msg: string) => void,
+) => {
+ if (!filterValue) {
+ permissionSearchCache.clear();
+ try {
+ return await fetchPermissionPageRaw({ page, page_size: pageSize });
+ } catch {
+ addDangerToast(t('There was an error while fetching permissions'));
+ return { data: [], totalCount: 0 };
+ }
+ }
+
+ try {
+ const cacheKey = filterValue;
+ let cached = permissionSearchCache.get(cacheKey);
+ if (!cached) {
Review Comment:
`fetchPermissionOptions` uses the raw `filterValue` as the cache key. This
makes caching case-sensitive even though the `ct` search is case-insensitive
(per PR description/testing instructions), so typing "Dataset" vs "dataset"
will create separate cache entries and redundant API work. Consider normalizing
the cache key (e.g., `filterValue.trim().toLowerCase()`) and updating the
related unit test accordingly.
##########
superset-frontend/src/pages/RolesList/index.tsx:
##########
@@ -209,7 +170,7 @@ function RolesList({ addDangerToast, addSuccessToast, user
}: RolesListProps) {
id: 'group_ids',
Header: t('Groups'),
hidden: true,
- Cell: ({ row: { original } }: any) => original.groups_ids.join(', '),
+ Cell: ({ row: { original } }: any) => original.group_ids.join(', '),
},
Review Comment:
This change still uses `any` for the table cell props. `AGENTS.md`
explicitly calls out avoiding `any` in the frontend; since this line was
touched, it would be a good opportunity to replace `any` with an appropriate
typed row/cell prop (e.g., based on the `ListView`/react-table types and
`RoleObject`).
##########
superset-frontend/src/features/roles/utils.ts:
##########
@@ -51,3 +54,166 @@ export const updateRoleName = async (roleId: number, name:
string) =>
endpoint: `/api/v1/security/roles/${roleId}`,
jsonPayload: { name },
});
+
+export const formatPermissionLabel = (
+ permissionName: string,
+ viewMenuName: string,
+) => `${permissionName.replace(/_/g, ' ')} ${viewMenuName.replace(/_/g, ' ')}`;
+
+type PermissionResult = {
+ id: number;
+ permission: { name: string };
+ view_menu: { name: string };
+};
+
+const mapPermissionResults = (results: PermissionResult[]) =>
+ results.map(item => ({
+ value: item.id,
+ label: formatPermissionLabel(item.permission.name, item.view_menu.name),
+ }));
+
+const PAGE_SIZE = 1000;
+const CONCURRENCY_LIMIT = 3;
+const MAX_CACHE_ENTRIES = 20;
+const permissionSearchCache = new Map<string, SelectOption[]>();
+
+export const clearPermissionSearchCache = () => {
+ permissionSearchCache.clear();
+};
+
+const fetchPermissionPageRaw = async (queryParams: Record<string, unknown>) =>
{
+ const response = await SupersetClient.get({
+ endpoint:
`/api/v1/security/permissions-resources/?q=${rison.encode(queryParams)}`,
+ });
+ return {
+ data: mapPermissionResults(response.json?.result || []),
+ totalCount: response.json?.count ?? 0,
+ };
+};
+
+const fetchAllPermissionPages = async (
+ filters: Record<string, unknown>[],
+): Promise<SelectOption[]> => {
+ const page0 = await fetchPermissionPageRaw({
+ page: 0,
+ page_size: PAGE_SIZE,
+ filters,
+ });
+ if (page0.data.length === 0 || page0.data.length >= page0.totalCount) {
+ return page0.data;
+ }
+
+ // Use actual returned size — backend may cap below PAGE_SIZE
+ const actualPageSize = page0.data.length;
+ const totalPages = Math.ceil(page0.totalCount / actualPageSize);
+ const allResults = [...page0.data];
+
+ // Fetch remaining pages in batches of CONCURRENCY_LIMIT
+ for (let batch = 1; batch < totalPages; batch += CONCURRENCY_LIMIT) {
+ const batchEnd = Math.min(batch + CONCURRENCY_LIMIT, totalPages);
+ const batchResults = await Promise.all(
+ Array.from({ length: batchEnd - batch }, (_, i) =>
+ fetchPermissionPageRaw({
+ page: batch + i,
+ page_size: PAGE_SIZE,
+ filters,
+ }),
+ ),
+ );
+ for (const r of batchResults) {
+ allResults.push(...r.data);
+ if (r.data.length === 0) return allResults;
+ }
+ if (allResults.length >= page0.totalCount) break;
+ }
+
+ return allResults;
+};
+
+export const fetchPermissionOptions = async (
+ filterValue: string,
+ page: number,
+ pageSize: number,
+ addDangerToast: (msg: string) => void,
+) => {
+ if (!filterValue) {
+ permissionSearchCache.clear();
+ try {
+ return await fetchPermissionPageRaw({ page, page_size: pageSize });
+ } catch {
+ addDangerToast(t('There was an error while fetching permissions'));
+ return { data: [], totalCount: 0 };
+ }
+ }
+
+ try {
+ const cacheKey = filterValue;
+ let cached = permissionSearchCache.get(cacheKey);
+ if (!cached) {
+ const [byViewMenu, byPermission] = await Promise.all([
+ fetchAllPermissionPages([
+ { col: 'view_menu.name', opr: 'ct', value: filterValue },
+ ]),
+ fetchAllPermissionPages([
+ { col: 'permission.name', opr: 'ct', value: filterValue },
+ ]),
+ ]);
+
+ const seen = new Set<number>();
+ cached = [...byViewMenu, ...byPermission].filter(item => {
+ if (seen.has(item.value)) return false;
+ seen.add(item.value);
+ return true;
+ });
+ if (permissionSearchCache.size >= MAX_CACHE_ENTRIES) {
+ const oldestKey = permissionSearchCache.keys().next().value;
+ if (oldestKey !== undefined) {
+ permissionSearchCache.delete(oldestKey);
+ }
+ }
+ permissionSearchCache.set(cacheKey, cached);
+ }
Review Comment:
The cache eviction logic is FIFO, not LRU: `Map#get()` does not update
insertion order, so frequently used search terms can still be evicted once
`MAX_CACHE_ENTRIES` is hit. Either implement true LRU behavior (update recency
on access) or adjust the PR description/comments to avoid claiming LRU eviction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
