feat(settings): add sync button to apply default permissions to existing users#3056
Conversation
📝 WalkthroughWalkthroughAdds a backend flag to POST /settings/main to apply updated defaultPermissions to existing non-admin users, and a frontend "Sync to Existing Users" button with an async handler that calls the API, revalidates SWR caches, and shows success/error toasts. ChangesDefault Permissions Sync for Existing Users
Sequence DiagramsequenceDiagram
participant SettingsUsers as SettingsUsers Component
participant API as POST /api/v1/settings/main
participant UserRepo as User Repository
participant SWR as SWR Cache
SettingsUsers->>API: POST with defaultPermissions + applyToExistingUsers: true
API->>UserRepo: Update non-admin users' permissions
UserRepo->>API: Bulk update complete
API->>SettingsUsers: Return updated settings
SettingsUsers->>SWR: Mutate /api/v1/settings/public + revalidate()
SettingsUsers->>SettingsUsers: Show success toast
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hey @swiizyy, thanks for submitting this PR! However, it looks like the PR template hasn't been fully filled out. Issues found:
Please update your PR description to follow the PR template. This check will automatically re-run when you edit your PR description. |
…ing users Adds a "Sync to Existing Users" button in the User Settings page that propagates the current default permissions to all non-admin users. Previously, changing default permissions only affected newly created users.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a UI control and backend support to apply the configured “default permissions” to all existing non-admin users.
Changes:
- Add “Sync to Existing Users” button in Settings → Users to trigger a bulk permissions sync.
- Extend
POST /api/v1/settings/mainwith anapplyToExistingUsersflag that bulk-updates non-admin users’ permissions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/components/Settings/SettingsUsers/index.tsx | Adds sync button + client-side request/toasts/state handling for bulk permission application |
| server/routes/settings/index.ts | Adds server-side handling for applyToExistingUsers and performs a bulk update of user permissions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await settings.save(); | ||
|
|
||
| if ( | ||
| applyToExistingUsers && | ||
| bodyWithoutFlag.defaultPermissions !== undefined | ||
| ) { | ||
| const userRepository = getRepository(User); | ||
| await userRepository | ||
| .createQueryBuilder() | ||
| .update(User) | ||
| .set({ permissions: bodyWithoutFlag.defaultPermissions }) | ||
| .where('(permissions & :adminFlag) = 0', { | ||
| adminFlag: Permission.ADMIN, | ||
| }) | ||
| .execute(); |
| await settings.save(); | ||
|
|
||
| if ( | ||
| applyToExistingUsers && |
| const handleSyncPermissions = async () => { | ||
| setIsSyncing(true); | ||
| try { | ||
| await axios.post('/api/v1/settings/main', { | ||
| defaultPermissions: values.defaultPermissions, | ||
| applyToExistingUsers: true, | ||
| }); | ||
| mutate('/api/v1/settings/public'); | ||
|
|
||
| addToast(intl.formatMessage(messages.syncPermissionsSuccess), { | ||
| autoDismiss: true, | ||
| appearance: 'success', | ||
| }); | ||
| } catch { | ||
| addToast(intl.formatMessage(messages.syncPermissionsFailure), { | ||
| autoDismiss: true, | ||
| appearance: 'error', | ||
| }); | ||
| } finally { | ||
| revalidate(); | ||
| setIsSyncing(false); | ||
| } | ||
| }; |
swiizyy
force-pushed
the
feat/sync-default-permissions-to-existing-users
branch
from
af9e646 to
2e603fa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Settings/SettingsUsers/index.tsx`:
- Around line 165-188: handleSyncPermissions currently calls revalidate() which
causes Formik (with enableReinitialize) to reload the entire form and clobber
any unsaved edits; instead, remove the revalidate() call and after a successful
POST only update the permissions in the form state and cache: call
setFieldValue('defaultPermissions', values.defaultPermissions) (or set the
returned permissions from the API if returned) and
mutate('/api/v1/settings/public') as you already do so the permissions are
refreshed without reinitializing the whole form; keep revalidate out of the
catch/finally to avoid resetting other fields on failure.
- Around line 352-355: The primary save Button currently disables only on
isSubmitting || !isValid but not when a sync is happening, so add the sync guard
by including isSyncing in the disabled condition (e.g., disabled={isSubmitting
|| !isValid || isSyncing}) and also ensure the form submit handler (the function
handling POST /api/v1/settings/main, e.g., onSubmit or handleSubmit)
short-circuits when isSyncing is true to avoid race conditions; update
references to isSyncing where the submit is triggered so both the UI and the
submit logic prevent concurrent saves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74348492-a7fa-48e9-85a7-5d203f5f0d8a
📒 Files selected for processing (2)
server/routes/settings/index.tssrc/components/Settings/SettingsUsers/index.tsx
| {({ isSubmitting, isValid, values, errors, setFieldValue }) => { | ||
| const handleSyncPermissions = async () => { | ||
| setIsSyncing(true); | ||
| try { | ||
| await axios.post('/api/v1/settings/main', { | ||
| defaultPermissions: values.defaultPermissions, | ||
| applyToExistingUsers: true, | ||
| }); | ||
| mutate('/api/v1/settings/public'); | ||
|
|
||
| addToast(intl.formatMessage(messages.syncPermissionsSuccess), { | ||
| autoDismiss: true, | ||
| appearance: 'success', | ||
| }); | ||
| } catch { | ||
| addToast(intl.formatMessage(messages.syncPermissionsFailure), { | ||
| autoDismiss: true, | ||
| appearance: 'error', | ||
| }); | ||
| } finally { | ||
| revalidate(); | ||
| setIsSyncing(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Don't reinitialize the whole form after a permission-only sync.
revalidate() will repopulate Formik while enableReinitialize is on, but this request only persists defaultPermissions. If the admin has unsaved edits in login or quota fields, those changes get reset back to the server values after the sync call, even on failure. Keep the current form state intact here and only refresh the permission value you actually changed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Settings/SettingsUsers/index.tsx` around lines 165 - 188,
handleSyncPermissions currently calls revalidate() which causes Formik (with
enableReinitialize) to reload the entire form and clobber any unsaved edits;
instead, remove the revalidate() call and after a successful POST only update
the permissions in the form state and cache: call
setFieldValue('defaultPermissions', values.defaultPermissions) (or set the
returned permissions from the API if returned) and
mutate('/api/v1/settings/public') as you already do so the permissions are
refreshed without reinitializing the whole form; keep revalidate out of the
catch/finally to avoid resetting other fields on failure.
| <Button | ||
| buttonType="primary" | ||
| type="submit" | ||
| disabled={isSubmitting || !isValid} |
There was a problem hiding this comment.
Disable the primary save while a sync is running.
The sync flow sets isSyncing, but the submit button can still fire another POST /api/v1/settings/main in parallel. That makes the final state order-dependent and can produce misleading toasts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Settings/SettingsUsers/index.tsx` around lines 352 - 355, The
primary save Button currently disables only on isSubmitting || !isValid but not
when a sync is happening, so add the sync guard by including isSyncing in the
disabled condition (e.g., disabled={isSubmitting || !isValid || isSyncing}) and
also ensure the form submit handler (the function handling POST
/api/v1/settings/main, e.g., onSubmit or handleSubmit) short-circuits when
isSyncing is true to avoid race conditions; update references to isSyncing where
the submit is triggered so both the UI and the submit logic prevent concurrent
saves.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/routes/settings/index.ts`:
- Around line 80-95: The current flow saves settings.main (settings.save())
before running the bulk user update
(userRepository.createQueryBuilder().update(User)...), which can leave settings
persisted if the UPDATE fails; wrap both operations in a single database
transaction (e.g., using TypeORM's getManager().transaction or
connection.transaction) so you perform settings.save() and the
userRepository.update inside the same transactional callback and let the
transaction roll back on error, or alternatively reverse the order and update
users first then save settings and return an explicit partial-success response
when only one step succeeds; target the settings.save() and the bulk update call
in your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16364607-f629-491e-857e-500e7652cf5e
📒 Files selected for processing (2)
server/routes/settings/index.tssrc/components/Settings/SettingsUsers/index.tsx
| settings.main = merge(settings.main, bodyWithoutFlag); | ||
| await settings.save(); | ||
|
|
||
| if ( | ||
| applyToExistingUsers && | ||
| bodyWithoutFlag.defaultPermissions !== undefined | ||
| ) { | ||
| const userRepository = getRepository(User); | ||
| await userRepository | ||
| .createQueryBuilder() | ||
| .update(User) | ||
| .set({ permissions: bodyWithoutFlag.defaultPermissions }) | ||
| .where('(permissions & :adminFlag) = 0', { | ||
| adminFlag: Permission.ADMIN, | ||
| }) | ||
| .execute(); |
There was a problem hiding this comment.
Avoid returning an error after partially applying the sync.
When applyToExistingUsers is enabled, this persists settings.main before the bulk user update. If the UPDATE fails, the request still returns an error even though the new defaultPermissions are already saved for future users. Please make these writes transactional/compensating, or return an explicit partial-success response instead of a generic failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/routes/settings/index.ts` around lines 80 - 95, The current flow saves
settings.main (settings.save()) before running the bulk user update
(userRepository.createQueryBuilder().update(User)...), which can leave settings
persisted if the UPDATE fails; wrap both operations in a single database
transaction (e.g., using TypeORM's getManager().transaction or
connection.transaction) so you perform settings.save() and the
userRepository.update inside the same transactional callback and let the
transaction roll back on error, or alternatively reverse the order and update
users first then save settings and return an explicit partial-success response
when only one step succeeds; target the settings.save() and the bulk update call
in your change.
This is not true. Bulk edit button in Users page exists precisely for this..
|
|
There's a beug that when you change these paramit their block in the bulk edit and you can't change them |
|
Or there should be a button in the bulk edit feature to add default permissions; plus, if you just want to add a setting, it doesn't work. If you just want to give everyone “View Request” permission, you can't because it deletes all the other settings. |
Then that should be handled first (it seems to work propely for me so far. So that should be troubleshooted). |
Because bulk edit works by applying the full perms you need. Not just one. And it applies to everyone you selected. |
Summary
POST /api/v1/settings/mainnow accepts anapplyToExistingUsersflag; whentrue, a bulkUPDATEexcludes any user with theADMINpermission bit setMotivation
Previously, changing the default permissions in settings had no effect on existing users. Admins had to update each user manually. This adds a one-click way to propagate the change without altering the default "save-only" flow.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit