From d657a42c83081b05b0e9b1fae90e25e8b7fd8ae0 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 23 Jul 2026 14:00:58 -0700 Subject: [PATCH 01/11] improvement(access-control): editable group details, status filters, block tooltips, chat auth colocation - General tab gains editable Name and Description fields wired into the existing dirty buffer, so the header Save/Discard chips and the unsaved-changes guard cover them. Save only sends changed fields and surfaces the route's duplicate-name 409. - Blocks, Model Providers, and Platform tabs gain an All/Enabled/Disabled filter beside their search field, evaluated against the editing buffer. - Every block row carries an Info badge with the block's description. The badge sits outside the label/expand button so it never toggles the row. - The chat deploy toggle moves out of Deploy Tabs into the Chat section alongside its allowed-auth-modes dropdown, mirroring the Files section. --- .../components/group-detail.tsx | 283 +++++++++++++----- 1 file changed, 208 insertions(+), 75 deletions(-) diff --git a/apps/sim/ee/access-control/components/group-detail.tsx b/apps/sim/ee/access-control/components/group-detail.tsx index 47a258f5f9a..9294207f451 100644 --- a/apps/sim/ee/access-control/components/group-detail.tsx +++ b/apps/sim/ee/access-control/components/group-detail.tsx @@ -53,6 +53,7 @@ import { useRemovePermissionGroupMember, useUpdatePermissionGroup, } from '@/ee/access-control/hooks/permission-groups' +import { SettingRow } from '@/ee/components/setting-row' import { useBlacklistedProviders } from '@/hooks/queries/allowed-providers' import { useOrganizationRoster } from '@/hooks/queries/organization' import { useProviderModels } from '@/hooks/queries/providers' @@ -90,6 +91,19 @@ const ALL_CHAT_DEPLOY_AUTH_TYPES: ShareAuthType[] = CHAT_DEPLOY_AUTH_TYPE_OPTION (o) => o.value ) +/** Narrows an allow/deny list to entries that are currently on or off. */ +type StatusFilter = 'all' | 'enabled' | 'disabled' + +const STATUS_FILTER_OPTIONS: { value: StatusFilter; label: string }[] = [ + { value: 'all', label: 'All' }, + { value: 'enabled', label: 'Enabled' }, + { value: 'disabled', label: 'Disabled' }, +] + +function matchesStatusFilter(filter: StatusFilter, enabled: boolean) { + return filter === 'all' || (filter === 'enabled') === enabled +} + interface OrganizationMemberOption { userId: string user: { @@ -521,7 +535,7 @@ function BlockToolRow({ onClick={() => isBlockAllowed && isExpandable && setExpanded((prev) => !prev)} disabled={!isBlockAllowed || !isExpandable} className={cn( - 'flex flex-1 items-center gap-2 text-left', + 'flex min-w-0 items-center gap-2 text-left', isBlockAllowed && isExpandable ? 'cursor-pointer' : 'cursor-default', !isBlockAllowed && 'opacity-60' )} @@ -532,15 +546,18 @@ function BlockToolRow({ {deniedCount} blocked )} - {isBlockAllowed && isExpandable && ( - - )} + + {block.description} + + {isBlockAllowed && isExpandable && ( + + )} {expanded && isBlockAllowed && isExpandable && (
@@ -595,11 +612,15 @@ export function GroupDetail({ */ const [viewingGroup, setViewingGroup] = useState(group) const [editingConfig, setEditingConfig] = useState({ ...group.config }) + const [editingName, setEditingName] = useState(group.name) + const [editingDescription, setEditingDescription] = useState(group.description ?? '') const prevGroupIdRef = useRef(group.id) if (prevGroupIdRef.current !== group.id) { prevGroupIdRef.current = group.id setViewingGroup(group) setEditingConfig({ ...group.config }) + setEditingName(group.name) + setEditingDescription(group.description ?? '') } /** @@ -613,6 +634,9 @@ export function GroupDetail({ const [providerSearchTerm, setProviderSearchTerm] = useState('') const [integrationSearchTerm, setIntegrationSearchTerm] = useState('') const [platformSearchTerm, setPlatformSearchTerm] = useState('') + const [providerStatusFilter, setProviderStatusFilter] = useState('all') + const [blockStatusFilter, setBlockStatusFilter] = useState('all') + const [platformStatusFilter, setPlatformStatusFilter] = useState('all') const [showAddMembersModal, setShowAddMembersModal] = useState(false) const [addMembersError, setAddMembersError] = useState(null) @@ -741,13 +765,6 @@ export function GroupDetail({ configKey: 'hideDeployMcp' as const, hint: 'Hide the MCP server deployment option.', }, - { - id: 'hide-deploy-chatbot', - label: 'Chat', - category: 'Deploy Tabs', - configKey: 'hideDeployChatbot' as const, - hint: 'Hide the chatbot deployment option.', - }, { id: 'disable-mcp', label: 'MCP Tools', @@ -802,12 +819,18 @@ export function GroupDetail({ ) const filteredPlatformFeatures = useMemo(() => { - if (!platformSearchTerm.trim()) return platformFeatures - const search = platformSearchTerm.toLowerCase() - return platformFeatures.filter( - (f) => f.label.toLowerCase().includes(search) || f.category.toLowerCase().includes(search) - ) - }, [platformFeatures, platformSearchTerm]) + const search = platformSearchTerm.trim().toLowerCase() + return platformFeatures.filter((f) => { + if ( + search && + !f.label.toLowerCase().includes(search) && + !f.category.toLowerCase().includes(search) + ) { + return false + } + return matchesStatusFilter(platformStatusFilter, !editingConfig[f.configKey]) + }) + }, [platformFeatures, platformSearchTerm, platformStatusFilter, editingConfig]) const platformCategories = useMemo(() => { const categories: Record = {} @@ -844,19 +867,37 @@ export function GroupDetail({ const hasConfigChanges = useMemo(() => { return JSON.stringify(viewingGroup.config) !== JSON.stringify(editingConfig) }, [viewingGroup.config, editingConfig]) - const guard = useSettingsUnsavedGuard({ isDirty: hasConfigChanges }) + + const trimmedName = editingName.trim() + const trimmedDescription = editingDescription.trim() + const hasDetailChanges = + trimmedName !== viewingGroup.name || trimmedDescription !== (viewingGroup.description ?? '') + const hasChanges = hasConfigChanges || hasDetailChanges + + const guard = useSettingsUnsavedGuard({ isDirty: hasChanges }) const filteredProviders = useMemo(() => { - if (!providerSearchTerm.trim()) return allProviderIds - const query = providerSearchTerm.toLowerCase() - return allProviderIds.filter((id) => id.toLowerCase().includes(query)) - }, [allProviderIds, providerSearchTerm]) + const query = providerSearchTerm.trim().toLowerCase() + const allowed = editingConfig.allowedModelProviders + return allProviderIds.filter((id) => { + if (query && !id.toLowerCase().includes(query)) return false + return matchesStatusFilter(providerStatusFilter, allowed === null || allowed.includes(id)) + }) + }, [ + allProviderIds, + providerSearchTerm, + providerStatusFilter, + editingConfig.allowedModelProviders, + ]) const filteredBlocks = useMemo(() => { - if (!integrationSearchTerm.trim()) return visibleBlocks - const query = integrationSearchTerm.toLowerCase() - return visibleBlocks.filter((b) => b.name.toLowerCase().includes(query)) - }, [visibleBlocks, integrationSearchTerm]) + const query = integrationSearchTerm.trim().toLowerCase() + const allowed = editingConfig.allowedIntegrations + return visibleBlocks.filter((b) => { + if (query && !b.name.toLowerCase().includes(query)) return false + return matchesStatusFilter(blockStatusFilter, allowed === null || allowed.includes(b.type)) + }) + }, [visibleBlocks, integrationSearchTerm, blockStatusFilter, editingConfig.allowedIntegrations]) const filteredCoreBlocks = useMemo( () => filteredBlocks.filter((block) => block.category === 'blocks'), @@ -1139,26 +1180,51 @@ export function GroupDetail({ })) }, []) - /** Persists the editing buffer. */ + /** Persists the editing buffer — name/description are only sent when they changed. */ const handleSaveConfig = useCallback(async () => { + if (!trimmedName) return + const nextDescription = trimmedDescription || null try { await updatePermissionGroup.mutateAsync({ id: viewingGroup.id, organizationId, - config: editingConfig, + ...(hasConfigChanges && { config: editingConfig }), + ...(trimmedName !== viewingGroup.name && { name: trimmedName }), + ...(trimmedDescription !== (viewingGroup.description ?? '') && { + description: nextDescription, + }), }) - setViewingGroup((prev) => ({ ...prev, config: editingConfig })) + setViewingGroup((prev) => ({ + ...prev, + config: editingConfig, + name: trimmedName, + description: nextDescription, + })) + setEditingName(trimmedName) + setEditingDescription(trimmedDescription) } catch (error) { - logger.error('Failed to update config', error) + logger.error('Failed to save permission group', error) toast.error("Couldn't save changes", { description: getErrorMessage(error, 'Please try again in a moment.'), }) } - }, [viewingGroup.id, editingConfig, organizationId, updatePermissionGroup]) + }, [ + viewingGroup.id, + viewingGroup.name, + viewingGroup.description, + editingConfig, + hasConfigChanges, + trimmedName, + trimmedDescription, + organizationId, + updatePermissionGroup, + ]) const handleDiscardConfig = useCallback(() => { setEditingConfig({ ...viewingGroup.config }) - }, [viewingGroup.config]) + setEditingName(viewingGroup.name) + setEditingDescription(viewingGroup.description ?? '') + }, [viewingGroup.config, viewingGroup.name, viewingGroup.description]) const handleBack = useCallback(() => { guard.guardBack(onBack) @@ -1307,10 +1373,11 @@ export function GroupDetail({ description={viewingGroup.description ?? undefined} actions={[ ...saveDiscardActions({ - dirty: hasConfigChanges, + dirty: hasChanges, saving: updatePermissionGroup.isPending, onSave: handleSaveConfig, onDiscard: handleDiscardConfig, + saveDisabled: !trimmedName, }), { text: deletePermissionGroup.isPending ? 'Deleting...' : 'Delete', @@ -1330,6 +1397,29 @@ export function GroupDetail({ {configTab === 'general' && ( <> + +
+ + setEditingName(e.target.value)} + placeholder='e.g., Marketing Team' + maxLength={100} + error={!trimmedName} + className='max-w-[320px]' + /> + + + setEditingDescription(e.target.value)} + placeholder='e.g., Limited access for marketing users' + maxLength={500} + /> + +
+ +
@@ -1457,6 +1547,13 @@ export function GroupDetail({ onChange={(e) => setProviderSearchTerm(e.target.value)} className='min-w-0 flex-1' /> + setProviderStatusFilter(value as StatusFilter)} + options={STATUS_FILTER_OPTIONS} + matchTriggerWidth={false} + className='flex-shrink-0' + /> setProvidersAllowed(filteredProviders, !filteredProvidersAllAllowed)} > @@ -1491,6 +1588,13 @@ export function GroupDetail({ onChange={(e) => setIntegrationSearchTerm(e.target.value)} className='min-w-0 flex-1' /> + setBlockStatusFilter(value as StatusFilter)} + options={STATUS_FILTER_OPTIONS} + matchTriggerWidth={false} + className='flex-shrink-0' + />
{filteredCoreBlocks.length > 0 && ( - toggleIntegration(block.type)} - /> -
+
- {block.name} - + toggleIntegration(block.type)} + /> +
+ {BlockIcon && } +
+ {block.name} + + + {block.description} + +
) })} @@ -1579,6 +1687,13 @@ export function GroupDetail({ onChange={(e) => setPlatformSearchTerm(e.target.value)} className='min-w-0 flex-1' /> + setPlatformStatusFilter(value as StatusFilter)} + options={STATUS_FILTER_OPTIONS} + matchTriggerWidth={false} + className='flex-shrink-0' + /> setEditingConfig((prev) => ({ @@ -1620,26 +1735,44 @@ export function GroupDetail({ ))} -
- - Auth modes chat deployments may use - - +
+ +
+ + Auth modes chat deployments may use + + +
From 0cf8648541e9c4b66e3b4688814cc1d541133cb4 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 23 Jul 2026 14:19:10 -0700 Subject: [PATCH 02/11] improvement(access-control): polish group detail filters, tooltips, and details fields Follows the cleanup review: - reconcile the post-save baseline from the server response instead of local values, matching the scope/default writes - pin the status-filter dropdown width so the search field stops resizing - restore flex-1 on the block-name button and move the row hover surface to the wrapper so Info badges align in a column - add empty states for filter-empty lists and neutralize Select All there - surface a 'Name is required' message next to the disabled Save - align hint text on the field-hint tokens and drop a redundant TSDoc --- .../components/group-detail.tsx | 295 +++++++++++------- 1 file changed, 176 insertions(+), 119 deletions(-) diff --git a/apps/sim/ee/access-control/components/group-detail.tsx b/apps/sim/ee/access-control/components/group-detail.tsx index 9294207f451..a289365f6e1 100644 --- a/apps/sim/ee/access-control/components/group-detail.tsx +++ b/apps/sim/ee/access-control/components/group-detail.tsx @@ -37,6 +37,7 @@ import { } from '@/app/workspace/[workspaceId]/settings/components/member-list' import { RowActionsMenu } from '@/app/workspace/[workspaceId]/settings/components/row-actions-menu' import { saveDiscardActions } from '@/app/workspace/[workspaceId]/settings/components/save-discard-actions/save-discard-actions' +import { SettingsEmptyState } from '@/app/workspace/[workspaceId]/settings/components/settings-empty-state' import { SettingsPanel } from '@/app/workspace/[workspaceId]/settings/components/settings-panel' import { SettingsSection } from '@/app/workspace/[workspaceId]/settings/components/settings-section/settings-section' import { useSettingsUnsavedGuard } from '@/app/workspace/[workspaceId]/settings/hooks/use-settings-unsaved-guard' @@ -91,13 +92,12 @@ const ALL_CHAT_DEPLOY_AUTH_TYPES: ShareAuthType[] = CHAT_DEPLOY_AUTH_TYPE_OPTION (o) => o.value ) -/** Narrows an allow/deny list to entries that are currently on or off. */ type StatusFilter = 'all' | 'enabled' | 'disabled' const STATUS_FILTER_OPTIONS: { value: StatusFilter; label: string }[] = [ - { value: 'all', label: 'All' }, - { value: 'enabled', label: 'Enabled' }, - { value: 'disabled', label: 'Disabled' }, + { value: 'all', label: 'Show all' }, + { value: 'enabled', label: 'Show enabled' }, + { value: 'disabled', label: 'Show disabled' }, ] function matchesStatusFilter(filter: StatusFilter, enabled: boolean) { @@ -535,7 +535,7 @@ function BlockToolRow({ onClick={() => isBlockAllowed && isExpandable && setExpanded((prev) => !prev)} disabled={!isBlockAllowed || !isExpandable} className={cn( - 'flex min-w-0 items-center gap-2 text-left', + 'flex min-w-0 flex-1 items-center gap-2 text-left', isBlockAllowed && isExpandable ? 'cursor-pointer' : 'cursor-default', !isBlockAllowed && 'opacity-60' )} @@ -547,9 +547,11 @@ function BlockToolRow({ )} - - {block.description} - + {block.description && ( + + {block.description} + + )} {isBlockAllowed && isExpandable && ( { + const search = platformSearchTerm.trim().toLowerCase() + if (search && !label.toLowerCase().includes(search)) return false + return matchesStatusFilter(platformStatusFilter, enabled) + } + + const showChatSection = platformSectionVisible('Chat', !editingConfig.hideDeployChatbot) + const showFilesSection = platformSectionVisible('Files', !editingConfig.disablePublicFileSharing) + const hasPlatformMatches = + platformCategorySections.length > 0 || showChatSection || showFilesSection + const hasConfigChanges = useMemo(() => { return JSON.stringify(viewingGroup.config) !== JSON.stringify(editingConfig) }, [viewingGroup.config, editingConfig]) @@ -1185,7 +1204,7 @@ export function GroupDetail({ if (!trimmedName) return const nextDescription = trimmedDescription || null try { - await updatePermissionGroup.mutateAsync({ + const result = await updatePermissionGroup.mutateAsync({ id: viewingGroup.id, organizationId, ...(hasConfigChanges && { config: editingConfig }), @@ -1194,14 +1213,18 @@ export function GroupDetail({ description: nextDescription, }), }) + // Reconcile from the server's copy, like the scope/default writes do, so a + // server-side normalization can't leave the dirty check comparing against a + // baseline that was never persisted. The editing buffers are deliberately + // left alone: edits made while the save was in flight stay, and correctly + // re-mark the form dirty. + const saved = result.permissionGroup setViewingGroup((prev) => ({ ...prev, - config: editingConfig, - name: trimmedName, - description: nextDescription, + config: saved.config, + name: saved.name, + description: saved.description, })) - setEditingName(trimmedName) - setEditingDescription(trimmedDescription) } catch (error) { logger.error('Failed to save permission group', error) toast.error("Couldn't save changes", { @@ -1406,8 +1429,12 @@ export function GroupDetail({ placeholder='e.g., Marketing Team' maxLength={100} error={!trimmedName} - className='max-w-[320px]' /> + {!trimmedName && ( +

+ Name is required. +

+ )} setProviderStatusFilter(value as StatusFilter)} options={STATUS_FILTER_OPTIONS} matchTriggerWidth={false} - className='flex-shrink-0' + className='w-[140px] flex-shrink-0' /> setProvidersAllowed(filteredProviders, !filteredProvidersAllAllowed)} + disabled={filteredProviders.length === 0} > {filteredProvidersAllAllowed ? 'Deselect All' : 'Select All'}
-
- {filteredProviders.map((providerId) => ( - toggleProvider(providerId)} - deniedCount={deniedCountByProvider[providerId] ?? 0} - workspaceId={workspaceId} - isAllowed={isModelAllowed} - onToggle={toggleModel} - onSetDenied={setModelsDenied} - /> - ))} -
+ {filteredProviders.length === 0 ? ( + + No providers match your filters. + + ) : ( +
+ {filteredProviders.map((providerId) => ( + toggleProvider(providerId)} + deniedCount={deniedCountByProvider[providerId] ?? 0} + workspaceId={workspaceId} + isAllowed={isModelAllowed} + onToggle={toggleModel} + onSetDenied={setModelsDenied} + /> + ))} +
+ )} )} @@ -1593,9 +1627,15 @@ export function GroupDetail({ onChange={(value) => setBlockStatusFilter(value as StatusFilter)} options={STATUS_FILTER_OPTIONS} matchTriggerWidth={false} - className='flex-shrink-0' + flush + className='w-[140px] flex-shrink-0' /> + {filteredCoreBlocks.length === 0 && filteredToolBlocks.length === 0 && ( + + No blocks match your filters. + + )} {filteredCoreBlocks.length > 0 && ( +
- - {block.description} - + {block.description && ( + + {block.description} + + )}
) })} @@ -1692,7 +1737,7 @@ export function GroupDetail({ onChange={(value) => setPlatformStatusFilter(value as StatusFilter)} options={STATUS_FILTER_OPTIONS} matchTriggerWidth={false} - className='flex-shrink-0' + className='w-[140px] flex-shrink-0' /> @@ -1703,6 +1748,7 @@ export function GroupDetail({ ), })) } + disabled={filteredPlatformFeatures.length === 0} > {platformAllVisible ? 'Deselect All' : 'Select All'} @@ -1728,94 +1774,105 @@ export function GroupDetail({ /> {feature.label} - {feature.hint} + + {feature.hint} + ))} ))} - -
- -
- - Auth modes chat deployments may use - - + {showChatSection && ( + +
+ +
+ + Auth modes chat deployments may use + + +
-
- - -
- -
- - Auth modes public file-share links may use - - + + )} + {showFilesSection && ( + +
+ +
+ + Auth modes public file-share links may use + + +
-
- + + )} + {!hasPlatformMatches && ( + + No features match your filters. + + )}
)} From 6cd7de6cc1708c781c821c335ca8ed0a53931840 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 23 Jul 2026 14:26:54 -0700 Subject: [PATCH 03/11] refactor(access-control): keep chat and files toggles in the platform registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first pass pulled hideDeployChatbot out of the declarative platformFeatures array and hand-rolled a Chat section beside the existing bespoke Files one. That forfeited search, status filtering, Select All, category grouping and the Info hint, and the replacement platformSectionVisible re-implemented two of those with different semantics — searching 'deploy' or 'deployment' hid the very control named Deployment, and Select All silently skipped both toggles. Both toggles are now ordinary registry entries under their own Chat and Files categories, with an id-keyed featureExtras map supplying the nested auth-mode dropdown. Search, filtering, Select All, hints and the empty state are correct by construction, and the parallel filter pipeline is gone. Also from the review: - index the allow-lists into Sets so per-row membership checks are O(1) - split the search and status passes so the common 'all' filter returns the searched list by reference and a checkbox toggle no longer re-sorts ~180 rows - extract StatusFilterChip and AuthModeField instead of stamping out the dropdowns three and two times - derive nameChanged/descriptionChanged once instead of repeating the comparisons in the save payload - lock the config key-order invariant the dirty check depends on with a test --- .../components/group-detail.tsx | 391 +++++++++--------- .../api/contracts/permission-groups.test.ts | 37 ++ 2 files changed, 233 insertions(+), 195 deletions(-) diff --git a/apps/sim/ee/access-control/components/group-detail.tsx b/apps/sim/ee/access-control/components/group-detail.tsx index a289365f6e1..3e3c01d214e 100644 --- a/apps/sim/ee/access-control/components/group-detail.tsx +++ b/apps/sim/ee/access-control/components/group-detail.tsx @@ -1,6 +1,6 @@ 'use client' -import { useCallback, useMemo, useRef, useState } from 'react' +import { type ReactNode, useCallback, useMemo, useRef, useState } from 'react' import { Checkbox, Chip, @@ -104,6 +104,59 @@ function matchesStatusFilter(filter: StatusFilter, enabled: boolean) { return filter === 'all' || (filter === 'enabled') === enabled } +interface StatusFilterChipProps { + value: StatusFilter + onChange: (value: StatusFilter) => void + /** Set when the chip is the last control in its row, so it sits flush to the edge. */ + flush?: boolean +} + +/** The All/Enabled/Disabled narrowing control shared by the three list tabs. */ +function StatusFilterChip({ value, onChange, flush }: StatusFilterChipProps) { + return ( + onChange(next as StatusFilter)} + options={STATUS_FILTER_OPTIONS} + matchTriggerWidth={false} + flush={flush} + className='w-[140px] flex-shrink-0' + /> + ) +} + +interface AuthModeFieldProps { + label: string + value: ShareAuthType[] + onChange: (values: string[]) => void + options: { value: ShareAuthType; label: string }[] + disabled: boolean +} + +/** + * The allowed-auth-modes multi-select nested under a platform toggle. Dims and + * disables together with the toggle that owns it, so the coupling lives here + * rather than being re-derived at each call site. + */ +function AuthModeField({ label, value, onChange, options, disabled }: AuthModeFieldProps) { + return ( +
+ {label} + +
+ ) +} + interface OrganizationMemberOption { userId: string user: { @@ -816,6 +869,22 @@ export function GroupDetail({ configKey: 'disablePublicApi' as const, hint: 'Disable public API access to deployed workflows.', }, + // Chat and Files get a category of their own so their nested auth-mode + // dropdown (see `featureExtras`) reads as part of the toggle it qualifies. + { + id: 'hide-deploy-chatbot', + label: 'Deployment', + category: 'Chat', + configKey: 'hideDeployChatbot' as const, + hint: 'Hide the chat deployment option.', + }, + { + id: 'disable-public-file-sharing', + label: 'Public Sharing', + category: 'Files', + configKey: 'disablePublicFileSharing' as const, + hint: 'Disable public file-share links.', + }, ], [] ) @@ -855,10 +924,12 @@ export function GroupDetail({ 'Features', 'Settings Tabs', 'Logs', + 'Chat', + 'Files', ] const known = order.filter((c) => platformCategories[c]?.length) const extras = Object.keys(platformCategories).filter( - (c) => c !== 'Files' && !order.includes(c) && platformCategories[c]?.length + (c) => !order.includes(c) && platformCategories[c]?.length ) return [...known, ...extras].map((category) => ({ category, @@ -866,57 +937,79 @@ export function GroupDetail({ })) }, [platformCategories]) - /** - * Chat and Files render as standalone sections (a toggle plus its nested - * auth-mode dropdown) rather than as `platformFeatures` rows, so they need the - * tab's search + status filter applied by hand — otherwise they'd be the only - * Platform controls that ignore both. - */ - const platformSectionVisible = (label: string, enabled: boolean) => { - const search = platformSearchTerm.trim().toLowerCase() - if (search && !label.toLowerCase().includes(search)) return false - return matchesStatusFilter(platformStatusFilter, enabled) - } - - const showChatSection = platformSectionVisible('Chat', !editingConfig.hideDeployChatbot) - const showFilesSection = platformSectionVisible('Files', !editingConfig.disablePublicFileSharing) - const hasPlatformMatches = - platformCategorySections.length > 0 || showChatSection || showFilesSection - const hasConfigChanges = useMemo(() => { return JSON.stringify(viewingGroup.config) !== JSON.stringify(editingConfig) }, [viewingGroup.config, editingConfig]) const trimmedName = editingName.trim() const trimmedDescription = editingDescription.trim() - const hasDetailChanges = - trimmedName !== viewingGroup.name || trimmedDescription !== (viewingGroup.description ?? '') - const hasChanges = hasConfigChanges || hasDetailChanges + const nameChanged = trimmedName !== viewingGroup.name + const descriptionChanged = trimmedDescription !== (viewingGroup.description ?? '') + const hasChanges = hasConfigChanges || nameChanged || descriptionChanged const guard = useSettingsUnsavedGuard({ isDirty: hasChanges }) - const filteredProviders = useMemo(() => { + /** + * `null` means "everything allowed". Indexing the allow-lists once keeps the + * per-row membership checks O(1) — they run for every one of the ~200 block + * rows on each render, and again in the section-wide `every(...)` scans. + */ + const allowedIntegrationSet = useMemo( + () => + editingConfig.allowedIntegrations === null + ? null + : new Set(editingConfig.allowedIntegrations), + [editingConfig.allowedIntegrations] + ) + + const allowedProviderSet = useMemo( + () => + editingConfig.allowedModelProviders === null + ? null + : new Set(editingConfig.allowedModelProviders), + [editingConfig.allowedModelProviders] + ) + + const isIntegrationAllowed = useCallback( + (blockType: string) => allowedIntegrationSet === null || allowedIntegrationSet.has(blockType), + [allowedIntegrationSet] + ) + + const isProviderAllowed = useCallback( + (providerId: string) => allowedProviderSet === null || allowedProviderSet.has(providerId), + [allowedProviderSet] + ) + + const searchedProviders = useMemo(() => { const query = providerSearchTerm.trim().toLowerCase() - const allowed = editingConfig.allowedModelProviders - return allProviderIds.filter((id) => { - if (query && !id.toLowerCase().includes(query)) return false - return matchesStatusFilter(providerStatusFilter, allowed === null || allowed.includes(id)) - }) - }, [ - allProviderIds, - providerSearchTerm, - providerStatusFilter, - editingConfig.allowedModelProviders, - ]) + if (!query) return allProviderIds + return allProviderIds.filter((id) => id.toLowerCase().includes(query)) + }, [allProviderIds, providerSearchTerm]) - const filteredBlocks = useMemo(() => { + /** + * Split from the search pass so the common `all` case returns the searched + * list by reference. Only the status pass depends on the allow-list, so a + * checkbox toggle no longer invalidates the downstream core/tool splits. + */ + const filteredProviders = useMemo(() => { + if (providerStatusFilter === 'all') return searchedProviders + return searchedProviders.filter((id) => + matchesStatusFilter(providerStatusFilter, isProviderAllowed(id)) + ) + }, [searchedProviders, providerStatusFilter, isProviderAllowed]) + + const searchedBlocks = useMemo(() => { const query = integrationSearchTerm.trim().toLowerCase() - const allowed = editingConfig.allowedIntegrations - return visibleBlocks.filter((b) => { - if (query && !b.name.toLowerCase().includes(query)) return false - return matchesStatusFilter(blockStatusFilter, allowed === null || allowed.includes(b.type)) - }) - }, [visibleBlocks, integrationSearchTerm, blockStatusFilter, editingConfig.allowedIntegrations]) + if (!query) return visibleBlocks + return visibleBlocks.filter((b) => b.name.toLowerCase().includes(query)) + }, [visibleBlocks, integrationSearchTerm]) + + const filteredBlocks = useMemo(() => { + if (blockStatusFilter === 'all') return searchedBlocks + return searchedBlocks.filter((b) => + matchesStatusFilter(blockStatusFilter, isIntegrationAllowed(b.type)) + ) + }, [searchedBlocks, blockStatusFilter, isIntegrationAllowed]) const filteredCoreBlocks = useMemo( () => filteredBlocks.filter((block) => block.category === 'blocks'), @@ -946,13 +1039,6 @@ export function GroupDetail({ return organizationMembers.filter((m) => !existingMemberUserIds.has(m.userId)) }, [organizationMembers, members]) - const isIntegrationAllowed = useCallback( - (blockType: string) => - editingConfig.allowedIntegrations === null || - editingConfig.allowedIntegrations.includes(blockType), - [editingConfig.allowedIntegrations] - ) - /** * Drops denied tools whose integration is no longer allowed, keeping the * invariant that `deniedTools` only holds tools of currently-allowed blocks. @@ -1063,13 +1149,6 @@ export function GroupDetail({ return counts }, [editingConfig.deniedTools, allBlocks]) - const isProviderAllowed = useCallback( - (providerId: string) => - editingConfig.allowedModelProviders === null || - editingConfig.allowedModelProviders.includes(providerId), - [editingConfig.allowedModelProviders] - ) - const toggleProvider = useCallback( (providerId: string) => { setEditingConfig((prev) => { @@ -1199,19 +1278,43 @@ export function GroupDetail({ })) }, []) + /** + * Nested controls rendered under a platform feature's checkbox, keyed by + * feature id. Keeping these out of `platformFeatures` leaves that array pure + * data (and its memo dependency-free) while the toggles themselves still flow + * through the tab's search, status filter, Select All, and empty state. + */ + const featureExtras: Partial> = { + 'hide-deploy-chatbot': ( + + ), + 'disable-public-file-sharing': ( + + ), + } + /** Persists the editing buffer — name/description are only sent when they changed. */ const handleSaveConfig = useCallback(async () => { if (!trimmedName) return - const nextDescription = trimmedDescription || null try { const result = await updatePermissionGroup.mutateAsync({ id: viewingGroup.id, organizationId, ...(hasConfigChanges && { config: editingConfig }), - ...(trimmedName !== viewingGroup.name && { name: trimmedName }), - ...(trimmedDescription !== (viewingGroup.description ?? '') && { - description: nextDescription, - }), + ...(nameChanged && { name: trimmedName }), + ...(descriptionChanged && { description: trimmedDescription || null }), }) // Reconcile from the server's copy, like the scope/default writes do, so a // server-side normalization can't leave the dirty check comparing against a @@ -1233,10 +1336,10 @@ export function GroupDetail({ } }, [ viewingGroup.id, - viewingGroup.name, - viewingGroup.description, editingConfig, hasConfigChanges, + nameChanged, + descriptionChanged, trimmedName, trimmedDescription, organizationId, @@ -1574,13 +1677,7 @@ export function GroupDetail({ onChange={(e) => setProviderSearchTerm(e.target.value)} className='min-w-0 flex-1' /> - setProviderStatusFilter(value as StatusFilter)} - options={STATUS_FILTER_OPTIONS} - matchTriggerWidth={false} - className='w-[140px] flex-shrink-0' - /> + setProvidersAllowed(filteredProviders, !filteredProvidersAllAllowed)} disabled={filteredProviders.length === 0} @@ -1622,14 +1719,7 @@ export function GroupDetail({ onChange={(e) => setIntegrationSearchTerm(e.target.value)} className='min-w-0 flex-1' /> - setBlockStatusFilter(value as StatusFilter)} - options={STATUS_FILTER_OPTIONS} - matchTriggerWidth={false} - flush - className='w-[140px] flex-shrink-0' - /> +
{filteredCoreBlocks.length === 0 && filteredToolBlocks.length === 0 && ( @@ -1732,13 +1822,7 @@ export function GroupDetail({ onChange={(e) => setPlatformSearchTerm(e.target.value)} className='min-w-0 flex-1' /> - setPlatformStatusFilter(value as StatusFilter)} - options={STATUS_FILTER_OPTIONS} - matchTriggerWidth={false} - className='w-[140px] flex-shrink-0' - /> + setEditingConfig((prev) => ({ @@ -1753,126 +1837,43 @@ export function GroupDetail({ {platformAllVisible ? 'Deselect All' : 'Select All'} + {platformCategorySections.length === 0 && ( + + No features match your filters. + + )} {platformCategorySections.map(({ category, features }) => (
{features.map((feature) => ( -
- - - {feature.hint} - +
+
+ + + {feature.hint} + +
+ {featureExtras[feature.id]}
))}
))} - {showChatSection && ( - -
- -
- - Auth modes chat deployments may use - - -
-
- - )} - {showFilesSection && ( - -
- -
- - Auth modes public file-share links may use - - -
-
- - )} - {!hasPlatformMatches && ( - - No features match your filters. - - )}
)} diff --git a/apps/sim/lib/api/contracts/permission-groups.test.ts b/apps/sim/lib/api/contracts/permission-groups.test.ts index 0b3bf3fb10f..a2a8132960d 100644 --- a/apps/sim/lib/api/contracts/permission-groups.test.ts +++ b/apps/sim/lib/api/contracts/permission-groups.test.ts @@ -4,8 +4,13 @@ import { describe, expect, it } from 'vitest' import { createPermissionGroupBodySchema, + permissionGroupFullConfigSchema, updatePermissionGroupBodySchema, } from '@/lib/api/contracts/permission-groups' +import { + DEFAULT_PERMISSION_GROUP_CONFIG, + parsePermissionGroupConfig, +} from '@/lib/permission-groups/types' describe('createPermissionGroupBodySchema', () => { it('accepts a name-only body (scope is resolved and validated server-side)', () => { @@ -80,3 +85,35 @@ describe('updatePermissionGroupBodySchema', () => { expect(result.success).toBe(false) }) }) + +/** + * The access-control group detail view decides whether its config buffer is + * dirty by comparing `JSON.stringify(savedConfig)` against + * `JSON.stringify(editingConfig)`, and reconciles the saved baseline from the + * update response. That only works while every config that reaches the client — + * from the list route and from the update route alike — carries the same key + * order, which holds because both pass through `parsePermissionGroupConfig` and + * then `permissionGroupFullConfigSchema`. If the two ever drift, the detail view + * would report unsaved changes forever after a successful save. + */ +describe('permissionGroupFullConfigSchema key order', () => { + it('matches parsePermissionGroupConfig so a saved config compares equal', () => { + const stored = parsePermissionGroupConfig({ + allowedIntegrations: ['slack'], + hideCopilot: true, + }) + const overWire = permissionGroupFullConfigSchema.parse(JSON.parse(JSON.stringify(stored))) + expect(JSON.stringify(overWire)).toBe(JSON.stringify(stored)) + }) + + it('keeps an edited client buffer comparable to the server echo', () => { + const fromList = permissionGroupFullConfigSchema.parse( + JSON.parse(JSON.stringify(parsePermissionGroupConfig(DEFAULT_PERMISSION_GROUP_CONFIG))) + ) + const edited = { ...fromList, hideDeployChatbot: true, deniedTools: ['slack_canvas'] } + const serverEcho = permissionGroupFullConfigSchema.parse( + JSON.parse(JSON.stringify(parsePermissionGroupConfig(edited))) + ) + expect(JSON.stringify(serverEcho)).toBe(JSON.stringify(edited)) + }) +}) From 43edb7914df876f9eb825000d3d8fb337f216d39 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 23 Jul 2026 14:32:13 -0700 Subject: [PATCH 04/11] polish(access-control): apply the second cleanup round - indent the nested auth-mode field so it lines up under its toggle's label instead of reading as a sibling row, and label the dropdown for screen readers - order Chat right after Deploy Tabs so the three deploy targets stay adjacent - flush the trailing Select All chips on the providers and platform rows - drop the doubled margin on the name error (SettingRow already gaps it) - hoist PLATFORM_FEATURES and PLATFORM_CATEGORY_ORDER to module scope - drop the useCallback on the two save/discard handlers; nothing observes their identity and their deps changed on every keystroke anyway - fix a comment that still pointed at a 'Hide Chat' toggle that no longer exists --- .../components/group-detail.tsx | 355 +++++++++--------- 1 file changed, 173 insertions(+), 182 deletions(-) diff --git a/apps/sim/ee/access-control/components/group-detail.tsx b/apps/sim/ee/access-control/components/group-detail.tsx index 3e3c01d214e..67b3f8bc4a7 100644 --- a/apps/sim/ee/access-control/components/group-detail.tsx +++ b/apps/sim/ee/access-control/components/group-detail.tsx @@ -1,6 +1,6 @@ 'use client' -import { type ReactNode, useCallback, useMemo, useRef, useState } from 'react' +import { type ReactNode, useCallback, useId, useMemo, useRef, useState } from 'react' import { Checkbox, Chip, @@ -135,17 +135,22 @@ interface AuthModeFieldProps { /** * The allowed-auth-modes multi-select nested under a platform toggle. Dims and - * disables together with the toggle that owns it, so the coupling lives here - * rather than being re-derived at each call site. + * disables together with the toggle that owns it. The left padding lines the + * sub-label up with its parent's label text (row gutter + checkbox + gap), so + * the field reads as subordinate to the toggle rather than as a sibling row. */ function AuthModeField({ label, value, onChange, options, disabled }: AuthModeFieldProps) { + const labelId = useId() return ( -
- {label} +
+ + {label} + [ - { - id: 'hide-knowledge-base', - label: 'Knowledge Base', - category: 'Sidebar', - configKey: 'hideKnowledgeBaseTab' as const, - hint: 'Hide the Knowledge Base module from the sidebar.', - }, - { - id: 'hide-tables', - label: 'Tables', - category: 'Sidebar', - configKey: 'hideTablesTab' as const, - hint: 'Hide the Tables module from the sidebar.', - }, - { - id: 'hide-copilot', - label: 'Chat', - category: 'Workflow Panel', - configKey: 'hideCopilot' as const, - hint: 'Hide the Chat panel so users cannot build or edit with natural language.', - }, - { - id: 'hide-integrations', - label: 'Integrations', - category: 'Settings Tabs', - configKey: 'hideIntegrationsTab' as const, - hint: 'Hide the Integrations settings tab (OAuth connections).', - }, - { - id: 'hide-secrets', - label: 'Secrets', - category: 'Settings Tabs', - configKey: 'hideSecretsTab' as const, - hint: 'Hide the Secrets (environment variables) settings tab.', - }, - { - id: 'hide-api-keys', - label: 'API Keys', - category: 'Settings Tabs', - configKey: 'hideApiKeysTab' as const, - hint: 'Hide the API Keys settings tab.', - }, - { - id: 'hide-files', - label: 'Files', - category: 'Settings Tabs', - configKey: 'hideFilesTab' as const, - hint: 'Hide the Files settings tab.', - }, - { - id: 'hide-deploy-api', - label: 'API', - category: 'Deploy Tabs', - configKey: 'hideDeployApi' as const, - hint: 'Hide the API deployment option.', - }, - { - id: 'hide-deploy-mcp', - label: 'MCP', - category: 'Deploy Tabs', - configKey: 'hideDeployMcp' as const, - hint: 'Hide the MCP server deployment option.', - }, - { - id: 'disable-mcp', - label: 'MCP Tools', - category: 'Tools', - configKey: 'disableMcpTools' as const, - hint: 'Block agents from calling MCP tools.', - }, - { - id: 'disable-custom-tools', - label: 'Custom Tools', - category: 'Tools', - configKey: 'disableCustomTools' as const, - hint: 'Block agents from calling user-defined custom tools.', - }, - { - id: 'disable-skills', - label: 'Skills', - category: 'Tools', - configKey: 'disableSkills' as const, - hint: 'Block agents from loading skills.', - }, - { - id: 'hide-trace-spans', - label: 'Trace Spans', - category: 'Logs', - configKey: 'hideTraceSpans' as const, - hint: 'Hide per-block trace spans in logs.', - }, - { - id: 'disable-invitations', - label: 'Invitations', - category: 'Collaboration', - configKey: 'disableInvitations' as const, - hint: 'Prevent users from inviting others to workspaces.', - }, - { - id: 'hide-inbox', - label: 'Sim Mailer', - category: 'Features', - configKey: 'hideInboxTab' as const, - hint: 'Hide the Sim Mailer inbox.', - }, - { - id: 'disable-public-api', - label: 'Public API', - category: 'Features', - configKey: 'disablePublicApi' as const, - hint: 'Disable public API access to deployed workflows.', - }, - // Chat and Files get a category of their own so their nested auth-mode - // dropdown (see `featureExtras`) reads as part of the toggle it qualifies. - { - id: 'hide-deploy-chatbot', - label: 'Deployment', - category: 'Chat', - configKey: 'hideDeployChatbot' as const, - hint: 'Hide the chat deployment option.', - }, - { - id: 'disable-public-file-sharing', - label: 'Public Sharing', - category: 'Files', - configKey: 'disablePublicFileSharing' as const, - hint: 'Disable public file-share links.', - }, - ], - [] - ) - const filteredPlatformFeatures = useMemo(() => { const search = platformSearchTerm.trim().toLowerCase() - return platformFeatures.filter((f) => { + return PLATFORM_FEATURES.filter((f) => { if ( search && !f.label.toLowerCase().includes(search) && @@ -901,10 +917,10 @@ export function GroupDetail({ } return matchesStatusFilter(platformStatusFilter, !editingConfig[f.configKey]) }) - }, [platformFeatures, platformSearchTerm, platformStatusFilter, editingConfig]) + }, [platformSearchTerm, platformStatusFilter, editingConfig]) const platformCategories = useMemo(() => { - const categories: Record = {} + const categories: Record = {} for (const feature of filteredPlatformFeatures) { if (!categories[feature.category]) { categories[feature.category] = [] @@ -915,21 +931,9 @@ export function GroupDetail({ }, [filteredPlatformFeatures]) const platformCategorySections = useMemo(() => { - const order = [ - 'Sidebar', - 'Deploy Tabs', - 'Collaboration', - 'Workflow Panel', - 'Tools', - 'Features', - 'Settings Tabs', - 'Logs', - 'Chat', - 'Files', - ] - const known = order.filter((c) => platformCategories[c]?.length) + const known = PLATFORM_CATEGORY_ORDER.filter((c) => platformCategories[c]?.length) const extras = Object.keys(platformCategories).filter( - (c) => !order.includes(c) && platformCategories[c]?.length + (c) => !PLATFORM_CATEGORY_ORDER.includes(c) && platformCategories[c]?.length ) return [...known, ...extras].map((category) => ({ category, @@ -988,8 +992,8 @@ export function GroupDetail({ /** * Split from the search pass so the common `all` case returns the searched - * list by reference. Only the status pass depends on the allow-list, so a - * checkbox toggle no longer invalidates the downstream core/tool splits. + * list by reference — only the status pass depends on the allow-list, so a + * checkbox toggle no longer invalidates downstream consumers. */ const filteredProviders = useMemo(() => { if (providerStatusFilter === 'all') return searchedProviders @@ -1269,7 +1273,7 @@ export function GroupDetail({ const setChatDeployAuthTypes = useCallback((values: string[]) => { // At least one mode must stay allowed while chat deploy is enabled — an empty // allow-list would silently block every chat deployment. To turn chat deploy - // off entirely, use the Hide Chat toggle instead. + // off entirely, uncheck Chat → Deployment instead. if (values.length === 0) return setEditingConfig((prev) => ({ ...prev, @@ -1280,9 +1284,7 @@ export function GroupDetail({ /** * Nested controls rendered under a platform feature's checkbox, keyed by - * feature id. Keeping these out of `platformFeatures` leaves that array pure - * data (and its memo dependency-free) while the toggles themselves still flow - * through the tab's search, status filter, Select All, and empty state. + * feature id. Kept out of `PLATFORM_FEATURES` so that array stays pure data. */ const featureExtras: Partial> = { 'hide-deploy-chatbot': ( @@ -1306,7 +1308,7 @@ export function GroupDetail({ } /** Persists the editing buffer — name/description are only sent when they changed. */ - const handleSaveConfig = useCallback(async () => { + const handleSaveConfig = async () => { if (!trimmedName) return try { const result = await updatePermissionGroup.mutateAsync({ @@ -1318,9 +1320,8 @@ export function GroupDetail({ }) // Reconcile from the server's copy, like the scope/default writes do, so a // server-side normalization can't leave the dirty check comparing against a - // baseline that was never persisted. The editing buffers are deliberately - // left alone: edits made while the save was in flight stay, and correctly - // re-mark the form dirty. + // baseline that was never persisted. Editing buffers are left alone so + // in-flight edits survive and correctly re-mark the form dirty. const saved = result.permissionGroup setViewingGroup((prev) => ({ ...prev, @@ -1334,23 +1335,13 @@ export function GroupDetail({ description: getErrorMessage(error, 'Please try again in a moment.'), }) } - }, [ - viewingGroup.id, - editingConfig, - hasConfigChanges, - nameChanged, - descriptionChanged, - trimmedName, - trimmedDescription, - organizationId, - updatePermissionGroup, - ]) + } - const handleDiscardConfig = useCallback(() => { + const handleDiscardConfig = () => { setEditingConfig({ ...viewingGroup.config }) setEditingName(viewingGroup.name) setEditingDescription(viewingGroup.description ?? '') - }, [viewingGroup.config, viewingGroup.name, viewingGroup.description]) + } const handleBack = useCallback(() => { guard.guardBack(onBack) @@ -1534,9 +1525,7 @@ export function GroupDetail({ error={!trimmedName} /> {!trimmedName && ( -

- Name is required. -

+

Name is required.

)} @@ -1679,6 +1668,7 @@ export function GroupDetail({ /> setProvidersAllowed(filteredProviders, !filteredProvidersAllAllowed)} disabled={filteredProviders.length === 0} > @@ -1832,6 +1822,7 @@ export function GroupDetail({ ), })) } + flush disabled={filteredPlatformFeatures.length === 0} > {platformAllVisible ? 'Deselect All' : 'Select All'} From 5083a2f5e77f61bb2c2b0746c1bbcf65979fa6b4 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 23 Jul 2026 14:38:32 -0700 Subject: [PATCH 05/11] fix(access-control): keep the block disclosure chevron inside its toggle button MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Splitting the chevron out so the Info badge could sit beside the name left the chevron with no click handler — the visible expansion affordance did nothing. It goes back inside the button; Info stays outside it, since an Info trigger is itself a button and cannot nest. --- .../access-control/components/group-detail.tsx | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/apps/sim/ee/access-control/components/group-detail.tsx b/apps/sim/ee/access-control/components/group-detail.tsx index 67b3f8bc4a7..a380d91ff4b 100644 --- a/apps/sim/ee/access-control/components/group-detail.tsx +++ b/apps/sim/ee/access-control/components/group-detail.tsx @@ -749,20 +749,21 @@ function BlockToolRow({ {deniedCount} blocked )} + {isBlockAllowed && isExpandable && ( + + )} + {/* Outside the button: an Info trigger is itself a button and cannot nest. */} {block.description && ( {block.description} )} - {isBlockAllowed && isExpandable && ( - - )}
{expanded && isBlockAllowed && isExpandable && (
From e33710bdf3ff2d6498d752539bd44a1d111b0c27 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 23 Jul 2026 14:48:49 -0700 Subject: [PATCH 06/11] chore(access-control): use structuredClone in the config key-order test check:utils bans JSON.parse(JSON.stringify(...)). The clone only needs to hand the schema a distinct object; structuredClone preserves key order the same way. --- apps/sim/lib/api/contracts/permission-groups.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/sim/lib/api/contracts/permission-groups.test.ts b/apps/sim/lib/api/contracts/permission-groups.test.ts index a2a8132960d..f688119fe3e 100644 --- a/apps/sim/lib/api/contracts/permission-groups.test.ts +++ b/apps/sim/lib/api/contracts/permission-groups.test.ts @@ -102,17 +102,17 @@ describe('permissionGroupFullConfigSchema key order', () => { allowedIntegrations: ['slack'], hideCopilot: true, }) - const overWire = permissionGroupFullConfigSchema.parse(JSON.parse(JSON.stringify(stored))) + const overWire = permissionGroupFullConfigSchema.parse(structuredClone(stored)) expect(JSON.stringify(overWire)).toBe(JSON.stringify(stored)) }) it('keeps an edited client buffer comparable to the server echo', () => { const fromList = permissionGroupFullConfigSchema.parse( - JSON.parse(JSON.stringify(parsePermissionGroupConfig(DEFAULT_PERMISSION_GROUP_CONFIG))) + structuredClone(parsePermissionGroupConfig(DEFAULT_PERMISSION_GROUP_CONFIG)) ) const edited = { ...fromList, hideDeployChatbot: true, deniedTools: ['slack_canvas'] } const serverEcho = permissionGroupFullConfigSchema.parse( - JSON.parse(JSON.stringify(parsePermissionGroupConfig(edited))) + structuredClone(parsePermissionGroupConfig(edited)) ) expect(JSON.stringify(serverEcho)).toBe(JSON.stringify(edited)) }) From 5c17b81c950b9f9ba8b302cc1b395462f430f62a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 23 Jul 2026 14:58:42 -0700 Subject: [PATCH 07/11] fix(access-control): trim descriptions so a padded value can't wedge the form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit descriptionChanged compared a trimmed draft against the raw saved description, so a group whose stored description carried padding opened dirty and could never be cleared — Discard restored the same padded string, and the unsaved-changes guard then blocked navigation until a save rewrote it. The contract now trims description on create and update, matching what name already did, and the dirty check trims both sides so existing padded rows behave too. --- .../components/group-detail.tsx | 6 +++++- .../api/contracts/permission-groups.test.ts | 20 +++++++++++++++++++ .../lib/api/contracts/permission-groups.ts | 4 ++-- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/apps/sim/ee/access-control/components/group-detail.tsx b/apps/sim/ee/access-control/components/group-detail.tsx index a380d91ff4b..d1fde59d02e 100644 --- a/apps/sim/ee/access-control/components/group-detail.tsx +++ b/apps/sim/ee/access-control/components/group-detail.tsx @@ -949,7 +949,11 @@ export function GroupDetail({ const trimmedName = editingName.trim() const trimmedDescription = editingDescription.trim() const nameChanged = trimmedName !== viewingGroup.name - const descriptionChanged = trimmedDescription !== (viewingGroup.description ?? '') + // Compare trimmed on both sides: `name` is trimmed by its contract schema but a + // description stored before this PR (or written straight to the API) can still + // carry padding, and comparing it raw would leave the form dirty on open with + // no way to clear it — Discard restores the same padded value. + const descriptionChanged = trimmedDescription !== (viewingGroup.description ?? '').trim() const hasChanges = hasConfigChanges || nameChanged || descriptionChanged const guard = useSettingsUnsavedGuard({ isDirty: hasChanges }) diff --git a/apps/sim/lib/api/contracts/permission-groups.test.ts b/apps/sim/lib/api/contracts/permission-groups.test.ts index f688119fe3e..e3460557529 100644 --- a/apps/sim/lib/api/contracts/permission-groups.test.ts +++ b/apps/sim/lib/api/contracts/permission-groups.test.ts @@ -117,3 +117,23 @@ describe('permissionGroupFullConfigSchema key order', () => { expect(JSON.stringify(serverEcho)).toBe(JSON.stringify(edited)) }) }) + +describe('permission group description trimming', () => { + it('trims a padded description on create', () => { + const result = createPermissionGroupBodySchema.safeParse({ + name: 'Engineering', + description: ' padded ', + }) + expect(result.success && result.data.description).toBe('padded') + }) + + it('trims a padded description on update', () => { + const result = updatePermissionGroupBodySchema.safeParse({ description: ' padded ' }) + expect(result.success && result.data.description).toBe('padded') + }) + + it('still accepts a null description on update', () => { + const result = updatePermissionGroupBodySchema.safeParse({ description: null }) + expect(result.success && result.data.description).toBeNull() + }) +}) diff --git a/apps/sim/lib/api/contracts/permission-groups.ts b/apps/sim/lib/api/contracts/permission-groups.ts index 273c47f874d..e3a531d8b43 100644 --- a/apps/sim/lib/api/contracts/permission-groups.ts +++ b/apps/sim/lib/api/contracts/permission-groups.ts @@ -148,7 +148,7 @@ function refineWorkspaceScope( export const createPermissionGroupBodySchema = z .object({ name: z.string().trim().min(1).max(100), - description: z.string().max(500).optional(), + description: z.string().trim().max(500).optional(), config: permissionGroupConfigSchema.optional(), isDefault: z.boolean().optional(), workspaceIds: workspaceIdsSchema.optional(), @@ -158,7 +158,7 @@ export const createPermissionGroupBodySchema = z export const updatePermissionGroupBodySchema = z .object({ name: z.string().trim().min(1).max(100).optional(), - description: z.string().max(500).nullable().optional(), + description: z.string().trim().max(500).nullable().optional(), config: permissionGroupConfigSchema.optional(), isDefault: z.boolean().optional(), workspaceIds: workspaceIdsSchema.optional(), From d1ff23bd312e57f3c6b9fd02f0bb4cb67eb0caab Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 23 Jul 2026 15:04:12 -0700 Subject: [PATCH 08/11] improvement(access-control): seed the description buffer trimmed Keeps the editing buffer and the dirty baseline normalized the same way, so a legacy row with padding no longer shows stray whitespace in the input and the buffer never round-trips padding a save would strip anyway. --- .../ee/access-control/components/group-detail.tsx | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/apps/sim/ee/access-control/components/group-detail.tsx b/apps/sim/ee/access-control/components/group-detail.tsx index d1fde59d02e..cdcf4d2934c 100644 --- a/apps/sim/ee/access-control/components/group-detail.tsx +++ b/apps/sim/ee/access-control/components/group-detail.tsx @@ -819,14 +819,14 @@ export function GroupDetail({ const [viewingGroup, setViewingGroup] = useState(group) const [editingConfig, setEditingConfig] = useState({ ...group.config }) const [editingName, setEditingName] = useState(group.name) - const [editingDescription, setEditingDescription] = useState(group.description ?? '') + const [editingDescription, setEditingDescription] = useState((group.description ?? '').trim()) const prevGroupIdRef = useRef(group.id) if (prevGroupIdRef.current !== group.id) { prevGroupIdRef.current = group.id setViewingGroup(group) setEditingConfig({ ...group.config }) setEditingName(group.name) - setEditingDescription(group.description ?? '') + setEditingDescription((group.description ?? '').trim()) } /** @@ -949,10 +949,10 @@ export function GroupDetail({ const trimmedName = editingName.trim() const trimmedDescription = editingDescription.trim() const nameChanged = trimmedName !== viewingGroup.name - // Compare trimmed on both sides: `name` is trimmed by its contract schema but a - // description stored before this PR (or written straight to the API) can still - // carry padding, and comparing it raw would leave the form dirty on open with - // no way to clear it — Discard restores the same padded value. + // `name` is trimmed by its contract schema, but a description stored before this + // PR (or written straight to the API) can still carry padding. The buffer is + // seeded trimmed and compared against a trimmed baseline, so such a row opens + // clean instead of being dirty with no way to clear it. const descriptionChanged = trimmedDescription !== (viewingGroup.description ?? '').trim() const hasChanges = hasConfigChanges || nameChanged || descriptionChanged @@ -1345,7 +1345,7 @@ export function GroupDetail({ const handleDiscardConfig = () => { setEditingConfig({ ...viewingGroup.config }) setEditingName(viewingGroup.name) - setEditingDescription(viewingGroup.description ?? '') + setEditingDescription((viewingGroup.description ?? '').trim()) } const handleBack = useCallback(() => { From f5761d9b2c4d4508b46de7d0d70c4b5b18e2fa67 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 23 Jul 2026 15:14:36 -0700 Subject: [PATCH 09/11] fix(emcn): forward aria-label/aria-labelledby from ChipDropdown to its trigger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ChipDropdown destructures only its known props, so an aria-labelledby passed by a consumer never reached the trigger button — AuthModeField's wiring to its visible label was silently dropped and the control had no accessible name. Both attributes are now explicit, typed props forwarded to the trigger. Kept as two named props rather than a rest spread so the component still owns its chrome and consumers can't smuggle arbitrary attributes onto the button. --- .../src/components/chip-dropdown/chip-dropdown.tsx | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/emcn/src/components/chip-dropdown/chip-dropdown.tsx b/packages/emcn/src/components/chip-dropdown/chip-dropdown.tsx index 244b09b6585..f0e119806b1 100644 --- a/packages/emcn/src/components/chip-dropdown/chip-dropdown.tsx +++ b/packages/emcn/src/components/chip-dropdown/chip-dropdown.tsx @@ -71,6 +71,13 @@ interface ChipDropdownBaseProps extends VariantProps { leftIcon?: ChipIcon /** Forwarded class for the trigger button. */ className?: string + /** + * Accessible name for the trigger. Use when the visible label sits outside + * the component (a field label above it) rather than in the selected value. + */ + 'aria-label'?: string + /** Id of the element naming the trigger. Pair with a visible field label. */ + 'aria-labelledby'?: string } /** @@ -168,6 +175,8 @@ const ChipDropdown = forwardRef( active, fullWidth, flush, + 'aria-label': ariaLabel, + 'aria-labelledby': ariaLabelledBy, } = props const isMultiple = props.multiple === true @@ -287,6 +296,8 @@ const ChipDropdown = forwardRef( ref={ref} type='button' disabled={disabled} + aria-label={ariaLabel} + aria-labelledby={ariaLabelledBy} className={cn( chipVariants({ variant, active, fullWidth, flush }), hasTriggerBorder && TRIGGER_BORDER_CLASS, From 86cec389044e0dd11c26133e0a2a3ef55791035e Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 23 Jul 2026 15:36:48 -0700 Subject: [PATCH 10/11] fix(access-control): correct the nested field indent, platform row hover, and aria name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - aria-labelledby REPLACES the content-derived name, so naming the auth-mode dropdown with its caption alone dropped the selected value from the accessible name — worse than no attribute. It now references caption + trigger, which needed ChipDropdown to forward id as well. - The nested field's pl-[30px] assumed a 14px checkbox; the default is 16px, so the caption sat 2px left of the label it hangs under while the dropdown (not flush, so mx-0.5) sat at 32. Both are pl-8 + flush now. - Platform feature rows kept their hover surface on the label, so the highlight stopped 20px short of the row edge while the Blocks tab ran flush. Moved to the wrapper, matching the core-blocks cell. - Split the platform search and status passes like the provider and block lists, so a toggle no longer recomputes three chained memos. - Dropped an unreachable allLabel and a comment that would go stale on merge. --- .../components/group-detail.tsx | 53 +++++++++++-------- .../chip-dropdown/chip-dropdown.tsx | 11 +++- 2 files changed, 41 insertions(+), 23 deletions(-) diff --git a/apps/sim/ee/access-control/components/group-detail.tsx b/apps/sim/ee/access-control/components/group-detail.tsx index cdcf4d2934c..5cd47fc305a 100644 --- a/apps/sim/ee/access-control/components/group-detail.tsx +++ b/apps/sim/ee/access-control/components/group-detail.tsx @@ -135,22 +135,28 @@ interface AuthModeFieldProps { /** * The allowed-auth-modes multi-select nested under a platform toggle. Dims and - * disables together with the toggle that owns it. The left padding lines the - * sub-label up with its parent's label text (row gutter + checkbox + gap), so - * the field reads as subordinate to the toggle rather than as a sibling row. + * disables together with the toggle that owns it. The left padding lines both + * children up with the parent's label text — row gutter (8) + checkbox (16) + + * gap (8) = 32 — so the field reads as subordinate rather than as a sibling row. + * The dropdown is `flush` so its own `mx-0.5` doesn't push it 2px past the + * caption above it. */ function AuthModeField({ label, value, onChange, options, disabled }: AuthModeFieldProps) { const labelId = useId() + const triggerId = useId() return ( -
+
{label} { + const searchedPlatformFeatures = useMemo(() => { const search = platformSearchTerm.trim().toLowerCase() - return PLATFORM_FEATURES.filter((f) => { - if ( - search && - !f.label.toLowerCase().includes(search) && - !f.category.toLowerCase().includes(search) - ) { - return false - } - return matchesStatusFilter(platformStatusFilter, !editingConfig[f.configKey]) - }) - }, [platformSearchTerm, platformStatusFilter, editingConfig]) + if (!search) return PLATFORM_FEATURES + return PLATFORM_FEATURES.filter( + (f) => f.label.toLowerCase().includes(search) || f.category.toLowerCase().includes(search) + ) + }, [platformSearchTerm]) + + /** Split from the search pass for the same reason as the provider and block lists. */ + const filteredPlatformFeatures = useMemo(() => { + if (platformStatusFilter === 'all') return searchedPlatformFeatures + return searchedPlatformFeatures.filter((f) => + matchesStatusFilter(platformStatusFilter, !editingConfig[f.configKey]) + ) + }, [searchedPlatformFeatures, platformStatusFilter, editingConfig]) const platformCategories = useMemo(() => { const categories: Record = {} @@ -949,8 +957,9 @@ export function GroupDetail({ const trimmedName = editingName.trim() const trimmedDescription = editingDescription.trim() const nameChanged = trimmedName !== viewingGroup.name - // `name` is trimmed by its contract schema, but a description stored before this - // PR (or written straight to the API) can still carry padding. The buffer is + // `name` is trimmed by its contract schema, but a description stored before + // description trimming was added (or written straight to the API) can still + // carry padding. The buffer is // seeded trimmed and compared against a trimmed baseline, so such a row opens // clean instead of being dirty with no way to clear it. const descriptionChanged = trimmedDescription !== (viewingGroup.description ?? '').trim() @@ -1843,10 +1852,10 @@ export function GroupDetail({
{features.map((feature) => (
-
+