Skip to content

improvement(access-control): edit group details, filter by status, and colocate chat deploy auth#5902

Merged
waleedlatif1 merged 11 commits into
stagingfrom
improvement/permission-groups-page
Jul 23, 2026
Merged

improvement(access-control): edit group details, filter by status, and colocate chat deploy auth#5902
waleedlatif1 merged 11 commits into
stagingfrom
improvement/permission-groups-page

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Name and description are now editable from a group's own page — a Details section on the General tab, wired into the existing dirty buffer so Save/Discard and the unsaved-changes guard cover them for free
  • Added a Show all / Show enabled / Show disabled filter to the Blocks, Model Providers, and Platform tabs, with an empty state when nothing matches
  • Every block row carries an info tooltip with the block's description
  • The chat deploy toggle now sits with its allowed-auth-modes dropdown under a Chat section, mirroring Files. Both are ordinary entries in the platform feature registry, so search, filtering, Select All, hints and the empty state apply to them like any other toggle
  • ChipDropdown gained aria-label / aria-labelledby / id props forwarded to its trigger, so a control labelled by an external field label gets a real accessible name

Behavior change worth knowing

Public file sharing was previously a bespoke checkbox outside the platform registry, so Deselect All never touched it. It is a registry entry now, so Deselect All on the Platform tab also disables it. It fails closed and is visibly listed, but it is a real change in what that button does.

Type of Change

  • Improvement

Testing

  • tsc --noEmit clean in apps/sim and packages/emcn; lint:check and all 11 check:* gates pass
  • 747 tests across ee/, lib/api/contracts/, components/settings/, plus 14 in packages/emcn
  • Added contract tests locking the config key-order invariant the dirty check relies on (without it a successful save could leave the form permanently dirty) and the description trimming
  • Not exercised in a browser — the tooltip column, filter rows and new Chat section have not been visually checked

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Jul 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docs Ready Ready Preview, Comment Jul 23, 2026 10:41pm

Request Review

@cursor

cursor Bot commented Jul 23, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes org-wide permission group UI and config persistence (including Platform Deselect All behavior for public file sharing); mistakes could misconfigure access for many users, though scope is admin settings rather than runtime auth paths.

Overview
Permission group detail now supports editing name and description on the General tab, folded into the same dirty buffer, save/discard guard, and partial update flow (trimmed compare, server echo on save).

Blocks, Model Providers, and Platform tabs get a Show all / enabled / disabled filter plus inline empty states when nothing matches. Block rows show description tooltips; integration rows avoid nested buttons for the info control.

Platform toggles move into a shared PLATFORM_FEATURES registry with updated category ordering (Chat, Files). Chat Deployment and Public Sharing are registry entries with nested auth-mode multi-selects (AuthModeField + accessible ChipDropdown). Deselect All on Platform now also toggles public file sharing (previously a one-off checkbox outside the bulk action).

List filtering uses Set-backed allow checks and separates search from status filtering for fewer re-renders. API description fields trim on create/update; tests lock config JSON key order for the client dirty check.

ChipDropdown forwards id, aria-label, and aria-labelledby to its trigger.

Reviewed by Cursor Bugbot for commit 113c306. Configure here.

Comment thread apps/sim/ee/access-control/components/group-detail.tsx Outdated
@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Updates permission-group configuration and controls.

  • Adds editable group details with unified save, discard, and unsaved-change handling.
  • Adds status filtering and empty states for blocks, model providers, and platform features.
  • Moves chat and file-sharing controls into the platform feature registry.
  • Adds block information tooltips and accessible naming props to ChipDropdown.
  • Normalizes permission-group descriptions and adds contract coverage for trimming and config ordering.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains in the corrected disclosure interaction.

Important Files Changed

Filename Overview
apps/sim/ee/access-control/components/group-detail.tsx Adds editable metadata, filtering, tooltips, and registry-based platform controls; the disclosure chevron fix correctly restores it inside the expansion button.
apps/sim/lib/api/contracts/permission-groups.test.ts Adds coverage for config serialization order and permission-group description trimming.
apps/sim/lib/api/contracts/permission-groups.ts Trims descriptions accepted by permission-group create and update contracts.
packages/emcn/src/components/chip-dropdown/chip-dropdown.tsx Forwards trigger IDs and accessible naming attributes through ChipDropdown.

Reviews (9): Last reviewed commit: "fix(access-control): trim the name compa..." | Re-trigger Greptile

Comment thread apps/sim/ee/access-control/components/group-detail.tsx Outdated
@waleedlatif1
waleedlatif1 force-pushed the improvement/permission-groups-page branch from d6ac959 to d7b4aff Compare July 23, 2026 21:39
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit d7b4aff. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/ee/access-control/components/group-detail.tsx
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 1c15b68. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/ee/access-control/components/group-detail.tsx
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 2b7ee90. Configure here.

…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.
…nd 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
… 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
- 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
…gle 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.
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.
…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.
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.
…s 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.
@waleedlatif1
waleedlatif1 force-pushed the improvement/permission-groups-page branch from 2b7ee90 to f5761d9 Compare July 23, 2026 22:32
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

…ver, 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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

…tion

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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 86cec38. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 113c306. Configure here.

@waleedlatif1
waleedlatif1 merged commit 3796e9d into staging Jul 23, 2026
16 checks passed
@waleedlatif1
waleedlatif1 deleted the improvement/permission-groups-page branch July 23, 2026 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant