From 3796e9db4fae0abc55a0523ba4379f4e9697fe71 Mon Sep 17 00:00:00 2001 From: Waleed Date: Thu, 23 Jul 2026 15:44:13 -0700 Subject: [PATCH] improvement(access-control): edit group details, filter by status, and colocate chat deploy auth (#5902) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. * 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 * refactor(access-control): keep chat and files toggles in the platform registry 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 * 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 * fix(access-control): keep the block disclosure chevron inside its toggle button 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. * 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. * fix(access-control): trim descriptions so a padded value can't wedge the form 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. * 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. * fix(emcn): forward aria-label/aria-labelledby from ChipDropdown to its trigger 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. * fix(access-control): correct the nested field indent, platform row hover, and aria name - 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. * fix(access-control): trim the name comparison the same way as description nameChanged compared a trimmed buffer against an untrimmed baseline — the exact asymmetry already fixed for description. A group stored before the name schema gained .trim() opens permanently dirty: Save/Discard visible and the unsaved-changes modal firing on back, until a save rewrites it. Both buffers now seed trimmed and compare trimmed on both sides. Also dims the Info badge along with the row it belongs to when the block is disallowed. --- .../components/group-detail.tsx | 774 +++++++++++------- .../api/contracts/permission-groups.test.ts | 57 ++ .../lib/api/contracts/permission-groups.ts | 4 +- .../chip-dropdown/chip-dropdown.tsx | 20 + 4 files changed, 564 insertions(+), 291 deletions(-) diff --git a/apps/sim/ee/access-control/components/group-detail.tsx b/apps/sim/ee/access-control/components/group-detail.tsx index 47a258f5f9..1b70ad89a2 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, useId, useMemo, useRef, useState } from 'react' import { Checkbox, Chip, @@ -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' @@ -53,6 +54,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 +92,227 @@ const ALL_CHAT_DEPLOY_AUTH_TYPES: ShareAuthType[] = CHAT_DEPLOY_AUTH_TYPE_OPTION (o) => o.value ) +type StatusFilter = 'all' | 'enabled' | 'disabled' + +const STATUS_FILTER_OPTIONS: { value: StatusFilter; label: string }[] = [ + { value: 'all', label: 'Show all' }, + { value: 'enabled', label: 'Show enabled' }, + { value: 'disabled', label: 'Show disabled' }, +] + +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. 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} + + +
+ ) +} + +/** Render order for the platform-feature category sections; unlisted ones follow. */ +const PLATFORM_CATEGORY_ORDER = [ + 'Sidebar', + 'Deploy Tabs', + 'Chat', + 'Collaboration', + 'Workflow Panel', + 'Tools', + 'Features', + 'Settings Tabs', + 'Logs', + 'Files', +] + +const PLATFORM_FEATURES = [ + { + 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.', + }, +] + interface OrganizationMemberOption { userId: string user: { @@ -521,7 +744,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 flex-1 items-center gap-2 text-left', isBlockAllowed && isExpandable ? 'cursor-pointer' : 'cursor-default', !isBlockAllowed && 'opacity-60' )} @@ -541,6 +764,12 @@ function BlockToolRow({ /> )} + {/* Outside the button: an Info trigger is itself a button and cannot nest. */} + {block.description && ( + + {block.description} + + )} {expanded && isBlockAllowed && isExpandable && (
@@ -595,11 +824,15 @@ export function GroupDetail({ */ const [viewingGroup, setViewingGroup] = useState(group) const [editingConfig, setEditingConfig] = useState({ ...group.config }) + const [editingName, setEditingName] = useState(group.name.trim()) + 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.trim()) + setEditingDescription((group.description ?? '').trim()) } /** @@ -613,6 +846,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) @@ -676,141 +912,24 @@ export function GroupDetail({ return map }, [allBlocks]) - const platformFeatures = useMemo( - () => [ - { - 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: 'hide-deploy-chatbot', - label: 'Chat', - category: 'Deploy Tabs', - configKey: 'hideDeployChatbot' as const, - hint: 'Hide the chatbot 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.', - }, - ], - [] - ) - - const filteredPlatformFeatures = useMemo(() => { - if (!platformSearchTerm.trim()) return platformFeatures - const search = platformSearchTerm.toLowerCase() - return platformFeatures.filter( + const searchedPlatformFeatures = useMemo(() => { + const search = platformSearchTerm.trim().toLowerCase() + if (!search) return PLATFORM_FEATURES + return PLATFORM_FEATURES.filter( (f) => f.label.toLowerCase().includes(search) || f.category.toLowerCase().includes(search) ) - }, [platformFeatures, platformSearchTerm]) + }, [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 = {} + const categories: Record = {} for (const feature of filteredPlatformFeatures) { if (!categories[feature.category]) { categories[feature.category] = [] @@ -821,19 +940,9 @@ export function GroupDetail({ }, [filteredPlatformFeatures]) const platformCategorySections = useMemo(() => { - const order = [ - 'Sidebar', - 'Deploy Tabs', - 'Collaboration', - 'Workflow Panel', - 'Tools', - 'Features', - 'Settings Tabs', - 'Logs', - ] - 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) => c !== 'Files' && !order.includes(c) && platformCategories[c]?.length + (c) => !PLATFORM_CATEGORY_ORDER.includes(c) && platformCategories[c]?.length ) return [...known, ...extras].map((category) => ({ category, @@ -844,20 +953,82 @@ export function GroupDetail({ const hasConfigChanges = useMemo(() => { return JSON.stringify(viewingGroup.config) !== JSON.stringify(editingConfig) }, [viewingGroup.config, editingConfig]) - const guard = useSettingsUnsavedGuard({ isDirty: hasConfigChanges }) - const filteredProviders = useMemo(() => { - if (!providerSearchTerm.trim()) return allProviderIds - const query = providerSearchTerm.toLowerCase() + // Both buffers are seeded trimmed and compared against a trimmed baseline. The + // contract trims name and description on write, but a row stored before those + // schemas gained `.trim()` (or written straight to the API) can still carry + // padding — compared raw it would open dirty with no way to clear it, since + // Discard restores the same padded value. + const trimmedName = editingName.trim() + const trimmedDescription = editingDescription.trim() + const nameChanged = trimmedName !== viewingGroup.name.trim() + const descriptionChanged = trimmedDescription !== (viewingGroup.description ?? '').trim() + const hasChanges = hasConfigChanges || nameChanged || descriptionChanged + + const guard = useSettingsUnsavedGuard({ isDirty: hasChanges }) + + /** + * `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() + if (!query) return allProviderIds return allProviderIds.filter((id) => id.toLowerCase().includes(query)) }, [allProviderIds, providerSearchTerm]) - const filteredBlocks = useMemo(() => { - if (!integrationSearchTerm.trim()) return visibleBlocks - const query = integrationSearchTerm.toLowerCase() + /** + * 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 downstream consumers. + */ + 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() + 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'), [filteredBlocks] @@ -886,13 +1057,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. @@ -1003,13 +1167,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) => { @@ -1130,7 +1287,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, @@ -1139,26 +1296,66 @@ export function GroupDetail({ })) }, []) - /** Persists the editing buffer. */ - const handleSaveConfig = useCallback(async () => { + /** + * Nested controls rendered under a platform feature's checkbox, keyed by + * feature id. Kept out of `PLATFORM_FEATURES` so that array stays pure data. + */ + const featureExtras: Partial> = { + 'hide-deploy-chatbot': ( + + ), + 'disable-public-file-sharing': ( + + ), + } + + /** Persists the editing buffer — name/description are only sent when they changed. */ + const handleSaveConfig = async () => { + if (!trimmedName) return try { - await updatePermissionGroup.mutateAsync({ + const result = await updatePermissionGroup.mutateAsync({ id: viewingGroup.id, organizationId, - config: editingConfig, + ...(hasConfigChanges && { config: editingConfig }), + ...(nameChanged && { name: trimmedName }), + ...(descriptionChanged && { description: trimmedDescription || null }), }) - setViewingGroup((prev) => ({ ...prev, config: editingConfig })) + // 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. Editing buffers are left alone so + // in-flight edits survive and correctly re-mark the form dirty. + const saved = result.permissionGroup + setViewingGroup((prev) => ({ + ...prev, + config: saved.config, + name: saved.name, + description: saved.description, + })) } 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]) + } - const handleDiscardConfig = useCallback(() => { + const handleDiscardConfig = () => { setEditingConfig({ ...viewingGroup.config }) - }, [viewingGroup.config]) + setEditingName(viewingGroup.name.trim()) + setEditingDescription((viewingGroup.description ?? '').trim()) + } const handleBack = useCallback(() => { guard.guardBack(onBack) @@ -1307,10 +1504,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 +1528,31 @@ export function GroupDetail({ {configTab === 'general' && ( <> + +
+ + setEditingName(e.target.value)} + placeholder='e.g., Marketing Team' + maxLength={100} + error={!trimmedName} + /> + {!trimmedName && ( +

Name is required.

+ )} + + + setEditingDescription(e.target.value)} + placeholder='e.g., Limited access for marketing users' + maxLength={500} + /> + +
+ +
@@ -1457,27 +1680,36 @@ export function GroupDetail({ onChange={(e) => setProviderSearchTerm(e.target.value)} className='min-w-0 flex-1' /> + 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} + /> + ))} +
+ )}
)} @@ -1491,7 +1723,13 @@ export function GroupDetail({ onChange={(e) => setIntegrationSearchTerm(e.target.value)} className='min-w-0 flex-1' /> + + {filteredCoreBlocks.length === 0 && filteredToolBlocks.length === 0 && ( + + No blocks match your filters. + + )} {filteredCoreBlocks.length > 0 && ( - toggleIntegration(block.type)} - /> -
- {BlockIcon && } -
- {block.name} - + toggleIntegration(block.type)} + /> +
+ {BlockIcon && } +
+ {block.name} + + {block.description && ( + + {block.description} + + )} + ) })} @@ -1579,6 +1826,7 @@ export function GroupDetail({ onChange={(e) => setPlatformSearchTerm(e.target.value)} className='min-w-0 flex-1' /> + setEditingConfig((prev) => ({ @@ -1588,101 +1836,49 @@ export function GroupDetail({ ), })) } + flush + disabled={filteredPlatformFeatures.length === 0} > {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]}
))}
))} - -
- - Auth modes chat deployments may use - - -
- - -
- -
- - Auth modes public file-share links may use - - -
-
-
)} diff --git a/apps/sim/lib/api/contracts/permission-groups.test.ts b/apps/sim/lib/api/contracts/permission-groups.test.ts index 0b3bf3fb10..e346055752 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,55 @@ 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(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( + structuredClone(parsePermissionGroupConfig(DEFAULT_PERMISSION_GROUP_CONFIG)) + ) + const edited = { ...fromList, hideDeployChatbot: true, deniedTools: ['slack_canvas'] } + const serverEcho = permissionGroupFullConfigSchema.parse( + structuredClone(parsePermissionGroupConfig(edited)) + ) + 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 273c47f874..e3a531d8b4 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(), diff --git a/packages/emcn/src/components/chip-dropdown/chip-dropdown.tsx b/packages/emcn/src/components/chip-dropdown/chip-dropdown.tsx index 244b09b658..67c4b7a8b0 100644 --- a/packages/emcn/src/components/chip-dropdown/chip-dropdown.tsx +++ b/packages/emcn/src/components/chip-dropdown/chip-dropdown.tsx @@ -71,6 +71,20 @@ 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 + /** + * Ids of the elements naming the trigger. Because `aria-labelledby` REPLACES + * the name derived from the trigger's contents, include the trigger's own id + * alongside the external label's — `\`${labelId} ${triggerId}\`` — or the + * selected value is dropped from the accessible name. + */ + 'aria-labelledby'?: string + /** Id for the trigger button. Needed to reference it from `aria-labelledby`. */ + id?: string } /** @@ -168,6 +182,9 @@ const ChipDropdown = forwardRef( active, fullWidth, flush, + 'aria-label': ariaLabel, + 'aria-labelledby': ariaLabelledBy, + id, } = props const isMultiple = props.multiple === true @@ -285,8 +302,11 @@ const ChipDropdown = forwardRef(