[WEB-5657] feat: add synchronization configuration for multiple providers in authentication adapter #8336
Conversation
- Implemented `check_sync_enabled` method to verify if sync is enabled for Google, GitHub, GitLab, and Gitea. - Added `sync_user_data` method to update user details, including first name, last name, display name, and avatar. - Updated configuration variables to include sync options for each provider. - Integrated sync check into the login/signup process.
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds ENABLE_*_SYNC toggles to admin provider forms and type defs, a reusable ControllerSwitch form component, backend sync and avatar handling in the auth adapter (with S3 bulk-delete support), a User.get_display_name helper, and new instance config variables for provider sync flags. Changes
Sequence DiagramsequenceDiagram
participant AdminUI as Admin UI
participant Form as Admin Form
participant API as Backend API
participant DB as Database
participant Adapter as Auth Adapter
participant IDP as Identity Provider
participant S3 as S3 Storage
AdminUI->>Form: Toggle ENABLE_*_SYNC and submit
Form->>API: POST provider config (includes ENABLE_*_SYNC)
API->>DB: Persist config
API-->>Form: Return updated config
Form-->>AdminUI: Show toast and reset form
Note over IDP,Adapter: Later — user authenticates via provider
IDP->>Adapter: OAuth response (user data + avatar URL)
Adapter->>Adapter: check_sync_enabled(provider)
alt sync enabled
Adapter->>Adapter: sync_user_data(user, idp_data)
Adapter->>S3: download_and_upload_avatar(avatar_url)
S3->>S3: delete_files(old_avatar_objects)
S3-->>Adapter: return FileAsset or fail
Adapter->>DB: save_user_data(updated profile, avatar ref)
else sync disabled
Adapter->>DB: save_user_data(minimal)
end
Adapter-->>API: complete login/signup response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/plane/authentication/adapter/base.py (2)
186-188: Bug: Avatar content downloaded twice, using wrong variable for upload.The code chunks the response into
content(line 178) but then usesresponse.content(line 187) for upload. This causes the avatar to be downloaded twice and wastes bandwidth. Use the already-downloadedcontentvariable.# Create file-like object - file_obj = BytesIO(response.content) + file_obj = BytesIO(content) file_obj.seek(0)
290-292: Fix invertedis_signupboolean logic.Line 292 sets
is_signup = bool(user), which is backwards. When a user is found (existing user, login flow),is_signupbecomesTrue; when no user is found (new user, signup flow), it becomesFalse. This contradicts the codebase semantics whereis_signup=Truedenotes a signup flow andis_signup=Falsedenotes a login flow—as confirmed by usage inprovider/credentials/email.py(which checks for existing users whenis_signup=Trueand raises an error) and the email views (which passis_signup=Falsefor login andis_signup=Truefor signup).Change line 292 to:
is_signup = user is None
🧹 Nitpick comments (10)
apps/api/plane/settings/storage.py (1)
191-201: Fix docstring and add type annotations for consistency.The docstring says "Delete an S3 object" (singular) but the method deletes multiple objects. Also, this method lacks type annotations unlike the adjacent
upload_filemethod.- def delete_files(self, object_names): - """Delete an S3 object""" + def delete_files(self, object_names: list[str]) -> bool: + """Delete multiple S3 objects in a single request"""Additionally, be aware that AWS S3
delete_objectshas a limit of 1000 objects per request. If callers may pass larger lists, consider batching.apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (2)
153-160: Add user feedback on error.The error is only logged to console. Users won't know the save failed. Consider adding an error toast for better UX, matching the success toast pattern.
} catch (err) { console.error(err); + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to save GitLab configuration. Please try again.", + }); }
10-10: Unused import.The
cnutility is imported but not used in this file.-import { cn } from "@plane/utils";apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (2)
176-178: Add user feedback on error.Same issue as other provider forms - errors are only logged to console. Add an error toast for user feedback.
} catch (err) { console.error(err); + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to save GitHub configuration. Please try again.", + }); }
12-12: Unused import.The
cnutility is imported but not used in this file.-import { cn } from "@plane/utils";apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx (1)
151-165: Consider adding user feedback on configuration save failure.The error is logged but the user receives no feedback when saving fails. For consistency with the success case, consider showing an error toast.
} catch (err) { console.error(err); + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to save Google authentication configuration.", + }); }apps/admin/core/components/common/controller-switch.tsx (1)
29-35: Simplify the boolean comparison and consider adding a fallback for undefined values.The
=== truecomparison is redundant. Also,parseInt(undefined)returnsNaN, which coerces tofalse- this works but is implicit.render={({ field: { value, onChange } }) => ( <ToggleSwitch - value={Boolean(parseInt(value))} - onChange={() => (Boolean(parseInt(value)) === true ? onChange("0") : onChange("1"))} + value={Boolean(parseInt(value || "0"))} + onChange={() => (parseInt(value || "0") ? onChange("0") : onChange("1"))} size="sm" /> )}apps/api/plane/authentication/adapter/base.py (3)
112-134: Consider refactoring repetitive provider sync check to use a dictionary lookup.The current implementation repeats the same pattern for each provider. A dict-based approach would be more maintainable.
def check_sync_enabled(self): """Check if sync is enabled for the provider""" - if self.provider == "google": - (ENABLE_GOOGLE_SYNC,) = get_configuration_value([ - {"key": "ENABLE_GOOGLE_SYNC", "default": os.environ.get("ENABLE_GOOGLE_SYNC", "0")} - ]) - return ENABLE_GOOGLE_SYNC == "1" - elif self.provider == "github": - (ENABLE_GITHUB_SYNC,) = get_configuration_value([ - {"key": "ENABLE_GITHUB_SYNC", "default": os.environ.get("ENABLE_GITHUB_SYNC", "0")} - ]) - return ENABLE_GITHUB_SYNC == "1" - elif self.provider == "gitlab": - (ENABLE_GITLAB_SYNC,) = get_configuration_value([ - {"key": "ENABLE_GITLAB_SYNC", "default": os.environ.get("ENABLE_GITLAB_SYNC", "0")} - ]) - return ENABLE_GITLAB_SYNC == "1" - elif self.provider == "gitea": - (ENABLE_GITEA_SYNC,) = get_configuration_value([ - {"key": "ENABLE_GITEA_SYNC", "default": os.environ.get("ENABLE_GITEA_SYNC", "0")} - ]) - return ENABLE_GITEA_SYNC == "1" - return False + sync_keys = { + "google": "ENABLE_GOOGLE_SYNC", + "github": "ENABLE_GITHUB_SYNC", + "gitlab": "ENABLE_GITLAB_SYNC", + "gitea": "ENABLE_GITEA_SYNC", + } + key = sync_keys.get(self.provider) + if not key: + return False + (sync_enabled,) = get_configuration_value([ + {"key": key, "default": os.environ.get(key, "0")} + ]) + return sync_enabled == "1"
233-247: Inconsistent return value and redundant database lookup.
- The method returns
Falseon exception but implicitly returnsNoneon success - this inconsistency could cause issues for callers.- Line 237 queries
FileAsset.objects.get(pk=user.avatar_asset_id)whenuser.avatar_assetis already available from line 236's check.def delete_old_avatar(self, user): """Delete the old avatar if it exists""" try: if user.avatar_asset: - asset = FileAsset.objects.get(pk=user.avatar_asset_id) + asset = user.avatar_asset storage = S3Storage(request=self.request) storage.delete_files(object_names=[asset.asset.name]) user.avatar_asset = None asset.delete() user.save() + return True except FileAsset.DoesNotExist: - pass + return True except Exception as e: log_exception(e) return False
268-277: Avatar is always deleted and re-uploaded even if unchanged.The current implementation deletes the old avatar and uploads a new one on every sync, even if the avatar URL hasn't changed. This wastes S3 operations and could cause brief avatar unavailability. Consider comparing the source URL before deletion.
# Download and upload avatar only if the avatar is different from the one in the storage avatar = self.user_data.get("user", {}).get("avatar", "") - # Delete the old avatar if it exists - self.delete_old_avatar(user=user) - avatar_asset = self.download_and_upload_avatar(avatar_url=avatar, user=user) - if avatar_asset: - user.avatar_asset = avatar_asset - # If avatar upload fails, set the avatar to the original URL - else: - user.avatar = avatar + # Only update avatar if URL has changed + if avatar and avatar != user.avatar: + # Delete the old avatar if it exists + self.delete_old_avatar(user=user) + avatar_asset = self.download_and_upload_avatar(avatar_url=avatar, user=user) + if avatar_asset: + user.avatar_asset = avatar_asset + # If avatar upload fails, set the avatar to the original URL + else: + user.avatar = avatar
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx(5 hunks)apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx(5 hunks)apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx(5 hunks)apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx(5 hunks)apps/admin/core/components/common/controller-switch.tsx(1 hunks)apps/api/plane/authentication/adapter/base.py(5 hunks)apps/api/plane/db/models/user.py(1 hunks)apps/api/plane/settings/storage.py(1 hunks)apps/api/plane/utils/instance_config_variables/core.py(4 hunks)packages/types/src/instance/auth.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsxapps/admin/core/components/common/controller-switch.tsxapps/admin/app/(all)/(dashboard)/authentication/google/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsxpackages/types/src/instance/auth.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Enable TypeScript strict mode and ensure all files are fully typed
Files:
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsxapps/admin/core/components/common/controller-switch.tsxapps/admin/app/(all)/(dashboard)/authentication/google/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsxpackages/types/src/instance/auth.ts
**/*.{js,jsx,ts,tsx,json,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier with Tailwind plugin for code formatting, run
pnpm fix:format
Files:
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsxapps/admin/core/components/common/controller-switch.tsxapps/admin/app/(all)/(dashboard)/authentication/google/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsxpackages/types/src/instance/auth.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use ESLint with shared config across packages, adhering to max warnings limits per package
Use camelCase for variable and function names, PascalCase for components and types
Use try-catch with proper error types and log errors appropriately
Files:
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsxapps/admin/core/components/common/controller-switch.tsxapps/admin/app/(all)/(dashboard)/authentication/google/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsxpackages/types/src/instance/auth.ts
🧠 Learnings (3)
📚 Learning: 2025-07-14T11:22:43.964Z
Learnt from: gakshita
Repo: makeplane/plane PR: 7393
File: apps/admin/app/(all)/(dashboard)/email/email-config-form.tsx:104-104
Timestamp: 2025-07-14T11:22:43.964Z
Learning: In the Plane project's SMTP configuration implementation, the email configuration form (email-config-form.tsx) hardcodes ENABLE_SMTP to "1" in form submission because the form is only rendered when SMTP is enabled. The enable/disable functionality is managed at the page level (page.tsx) with a toggle, and the form only handles configuration details when SMTP is already enabled.
Applied to files:
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsxapps/admin/app/(all)/(dashboard)/authentication/google/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx
📚 Learning: 2025-11-25T10:18:05.172Z
Learnt from: CR
Repo: makeplane/plane PR: 0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-11-25T10:18:05.172Z
Learning: Applies to **/*.{ts,tsx,mts,cts} : Utilize narrowing in `switch(true)` blocks for control flow analysis (TypeScript 5.3+)
Applied to files:
apps/admin/core/components/common/controller-switch.tsx
📚 Learning: 2025-08-06T09:01:17.915Z
Learnt from: shivam-jainn
Repo: makeplane/plane PR: 7545
File: apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx:139-143
Timestamp: 2025-08-06T09:01:17.915Z
Learning: In the Plane project's Gitea authentication form (apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx), the GITEA_HOST field should not be included in the form reset after submission. It should default back to "https://gitea.com" rather than maintaining the submitted value, as this provides a sensible default for most users.
Applied to files:
apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx
🧬 Code graph analysis (7)
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (3)
apps/admin/core/components/common/controller-switch.tsx (2)
TControllerSwitchFormField(11-14)ControllerSwitch(16-40)packages/propel/src/toast/toast.tsx (1)
setToast(245-265)apps/space/core/store/root.store.ts (1)
reset(62-75)
apps/admin/core/components/common/controller-switch.tsx (1)
packages/ui/src/button/toggle-switch.tsx (1)
ToggleSwitch(56-56)
apps/api/plane/settings/storage.py (1)
apps/api/plane/utils/exception_logger.py (1)
log_exception(9-20)
apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx (1)
apps/admin/core/components/common/controller-switch.tsx (2)
TControllerSwitchFormField(11-14)ControllerSwitch(16-40)
apps/api/plane/authentication/adapter/base.py (2)
apps/api/plane/settings/storage.py (2)
S3Storage(15-201)delete_files(191-201)apps/api/plane/db/models/user.py (4)
save(150-168)User(42-176)get_display_name(171-176)avatar_url(129-137)
apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (1)
apps/admin/core/components/common/controller-switch.tsx (2)
TControllerSwitchFormField(11-14)ControllerSwitch(16-40)
apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (1)
apps/admin/core/components/common/controller-switch.tsx (2)
TControllerSwitchFormField(11-14)ControllerSwitch(16-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check:format
- GitHub Check: Build packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (14)
apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (1)
15-16: LGTM on sync toggle integration.The
ENABLE_GITLAB_SYNCtoggle is properly integrated: added to form state, defined as a switch field, rendered viaControllerSwitch, and included in the reset payload after submission. This follows the consistent pattern across other provider forms.Also applies to: 46-46, 114-117, 194-194, 200-200
apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (2)
147-152: Verify GITEA_HOST reset behavior change is intentional.Based on previous learnings, the
GITEA_HOSTfield was intended to reset to the default"https://gitea.com"rather than the submitted value. This change now resets it from the response. Please confirm this behavioral change is intentional.Also, similar to other provider forms, add an error toast for user feedback:
} catch (err) { console.error(err); + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to save Gitea configuration. Please try again.", + }); }
15-16: LGTM on sync toggle integration.The
ENABLE_GITEA_SYNCtoggle is correctly integrated into the form following the same pattern as other provider forms.Also applies to: 45-45, 109-112, 189-189, 194-194
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (1)
17-18: LGTM on sync toggle integration.The
ENABLE_GITHUB_SYNCtoggle is properly integrated following the consistent pattern across provider forms.Also applies to: 48-48, 110-113, 212-212, 218-218
apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx (5)
16-17: LGTM!Imports are correctly added for the new
ControllerSwitchcomponent and its type definition.
43-47: LGTM!The
ENABLE_GOOGLE_SYNCfield is correctly added to the form's default values, maintaining consistency with the existing configuration pattern.
99-102: LGTM!The switch field descriptor is well-typed using
TControllerSwitchFormField<GoogleConfigFormValues>which ensures type safety between the form values and the field configuration.
199-199: LGTM!The
ControllerSwitchis correctly wired with the form control and field configuration.
205-205: LGTM!The
onClickhandler correctly wrapshandleSubmit(onSubmit)withvoidto handle the async form submission while preserving proper event handling.apps/admin/core/components/common/controller-switch.tsx (1)
6-14: LGTM!The type definitions are well-structured with proper generic constraints, enabling type-safe usage across different form configurations.
apps/api/plane/utils/instance_config_variables/core.py (2)
47-52: LGTM!The
ENABLE_GOOGLE_SYNCconfiguration follows the established pattern, with a safe default of"0"(disabled) and correctly marked as non-encrypted since it's a boolean flag.
74-79: LGTM!All provider sync flags (
ENABLE_GITHUB_SYNC,ENABLE_GITLAB_SYNC,ENABLE_GITEA_SYNC) consistently follow the same pattern with safe defaults.Also applies to: 102-107, 135-140
packages/types/src/instance/auth.ts (1)
19-40: LGTM!Type definitions are correctly updated to include the new
ENABLE_*_SYNCkeys for all providers. The union types properly compose to formTInstanceAuthenticationConfigurationKeys.apps/api/plane/authentication/adapter/base.py (1)
337-339: LGTM!The sync integration correctly gates user data synchronization behind the
check_sync_enabled()check, ensuring sync only occurs when explicitly enabled per provider.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/plane/db/models/user.py (1)
170-178: Good fixes from previous review; avoid splitting email twice.The previous issues with using
selfinstead ofclsand missing None checks have been addressed. However, the email is still split twice (lines 175 and 176), which is inefficient.Apply this diff to store the split result:
@classmethod def get_display_name(cls, email): if not email: return "".join(random.choice(string.ascii_letters) for _ in range(6)) + parts = email.split("@") return ( - email.split("@")[0] - if len(email.split("@")) == 2 + parts[0] + if len(parts) == 2 else "".join(random.choice(string.ascii_letters) for _ in range(6)) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/db/models/user.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check:lint
- GitHub Check: check:types
- GitHub Check: Analyze (javascript)
There was a problem hiding this comment.
Pull request overview
This PR adds synchronization configuration for multiple OAuth providers (Google, GitHub, GitLab, Gitea) in the authentication system. It introduces a mechanism to control whether user attributes (name, display name, avatar) should be refreshed from the OAuth provider during each sign-in.
Key changes:
- Adds
check_sync_enabledmethod andsync_user_datamethod to the authentication adapter - Adds
ENABLE_{PROVIDER}_SYNCconfiguration variables for each OAuth provider - Implements a new
ControllerSwitchcomponent for the admin UI to toggle sync settings - Adds
delete_filesmethod to S3Storage for batch avatar deletion - Adds
get_display_nameclass method to User model for generating display names
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/types/src/instance/auth.ts | Adds TypeScript type definitions for new sync configuration keys for all four OAuth providers |
| apps/api/plane/utils/instance_config_variables/core.py | Adds configuration variables for enabling sync for Google, GitHub, GitLab, and Gitea with default value "0" |
| apps/api/plane/settings/storage.py | Adds delete_files method to S3Storage class for batch deletion of S3 objects |
| apps/api/plane/db/models/user.py | Adds get_display_name method to extract display name from email or generate fallback |
| apps/api/plane/authentication/adapter/base.py | Implements check_sync_enabled, sync_user_data, and delete_old_avatar methods; integrates sync into login flow |
| apps/admin/core/components/common/controller-switch.tsx | New reusable switch component for form-controlled toggle switches |
| apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx | Adds sync toggle UI, updates form handling to include ENABLE_GOOGLE_SYNC, improves error handling |
| apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx | Adds sync toggle UI, updates form handling to include ENABLE_GITLAB_SYNC, improves error handling |
| apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx | Adds sync toggle UI, updates form handling to include ENABLE_GITHUB_SYNC, improves error handling |
| apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx | Adds sync toggle UI, updates form handling to include ENABLE_GITEA_SYNC, improves error handling |
Comments suppressed due to low confidence (1)
apps/api/plane/db/models/user.py:171
- Class methods or methods of a type deriving from type should have 'cls', rather than 'self', as their first parameter.
def get_display_name(cls, email):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| GITHUB_CLIENT_ID: config["GITHUB_CLIENT_ID"], | ||
| GITHUB_CLIENT_SECRET: config["GITHUB_CLIENT_SECRET"], | ||
| GITHUB_ORGANIZATION_ID: config["GITHUB_ORGANIZATION_ID"], | ||
| ENABLE_GITHUB_SYNC: config["ENABLE_GITHUB_SYNC"], |
There was a problem hiding this comment.
The form defaultValues should provide a fallback value for ENABLE_GITHUB_SYNC in case config["ENABLE_GITHUB_SYNC"] is undefined. This could cause issues with the ControllerSwitch component which expects a string value to parse. Consider using 'config["ENABLE_GITHUB_SYNC"] || "0"' to ensure a valid default.
| ENABLE_GITHUB_SYNC: config["ENABLE_GITHUB_SYNC"], | |
| ENABLE_GITHUB_SYNC: config["ENABLE_GITHUB_SYNC"] || "0", |
…atar deletion logic
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/authentication/adapter/base.py (1)
286-286: Critical:is_signuplogic is inverted, causing sync to run for new users instead of existing users.Line 286 sets
is_signup = bool(user), which isTruewhen the user already exists (a login) andFalsewhen the user isNone(a new signup). This naming is inverted from the semantic meaning.The condition on line 332
not is_signuptherefore triggers sync for new signups, not existing users. This causes:
- Duplicate work: new user data was just set in lines 310-326
- Missed sync: existing users logging in never get their IdP data synchronized
Fix the logic inversion:
# Check if the user is present user = User.objects.filter(email=email).first() - # Check if sign up case or login - is_signup = bool(user) + # Check if this is an existing user (login) or new user (signup) + is_existing_user = user is not None # If user is not present, create a new user if not user:Then update the sync condition:
- # Check if IDP sync is enabled and user is not signing up - if self.check_sync_enabled() and not is_signup: + # Sync user data from IDP for existing users when enabled + if self.check_sync_enabled() and is_existing_user: user = self.sync_user_data(user=user)And update the callback:
# Call callback if present if self.callback: - self.callback(user, is_signup, self.request) + self.callback(user, not is_existing_user, self.request)Also applies to: 331-333
♻️ Duplicate comments (1)
apps/api/plane/authentication/adapter/base.py (1)
263-271: Avatar deletion before download creates data loss risk.The old avatar is deleted (line 265) before attempting to download the new one (line 266). If the download or upload fails, the user permanently loses their avatar asset and falls back to just a URL reference.
Delete the old avatar only after successfully uploading the new one:
# Download and upload avatar only if the avatar is different from the one in the storage avatar = self.user_data.get("user", {}).get("avatar", "") - # Delete the old avatar if it exists - self.delete_old_avatar(user=user) avatar_asset = self.download_and_upload_avatar(avatar_url=avatar, user=user) if avatar_asset: + # Delete the old avatar only after successful upload + self.delete_old_avatar(user=user) user.avatar_asset = avatar_asset # If avatar upload fails, set the avatar to the original URL else: user.avatar = avatar
🧹 Nitpick comments (2)
apps/api/plane/authentication/adapter/base.py (2)
226-227: Consider using the existing FK reference directly.
user.avatar_assetalready holds theFileAssetinstance. The additionalFileAsset.objects.get(pk=user.avatar_asset_id)query is redundant unless you're deliberately re-fetching to confirm DB state.def delete_old_avatar(self, user): """Delete the old avatar if it exists""" try: if user.avatar_asset: - asset = FileAsset.objects.get(pk=user.avatar_asset_id) + asset = user.avatar_asset storage = S3Storage(request=self.request) storage.delete_files(object_names=[asset.asset.name])
321-326: Clarify intent: bothavatar_assetandavatarURL are stored on success.Both the asset reference and the original URL are stored when upload succeeds (lines 322-323). If this is intentional as a fallback mechanism, a brief comment would clarify the design. Otherwise, line 323 duplicates line 326's fallback behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/authentication/adapter/base.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/authentication/adapter/base.py (4)
apps/api/plane/settings/storage.py (2)
S3Storage(15-201)delete_files(191-201)apps/api/plane/authentication/views/app/gitea.py (2)
get(23-53)get(57-103)apps/api/plane/db/models/asset.py (1)
FileAsset(24-96)apps/api/plane/db/models/user.py (4)
save(150-168)User(42-178)get_display_name(171-178)avatar_url(129-137)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apps/api/plane/authentication/adapter/base.py (3)
24-24: LGTM!Moving the
S3Storageimport to module level improves clarity and follows Python import conventions.
94-96: LGTM!Minor formatting improvement for readability.
112-124: LGTM!Clean implementation using dictionary mapping for provider-to-config-key lookup. The configuration retrieval pattern is consistent with
__check_signup.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (1)
46-46: Fallback value already present.The past review comment suggested adding
|| "0"fallback, but the code already includes it. This ensures the ControllerSwitch component receives a valid string value to parse.apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (1)
45-45: Fallback value already present.The past review comment suggested adding
|| "0"fallback, but it's already implemented correctly. This prevents issues with the ControllerSwitch component.apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (2)
45-45: Fallback value already present.The code already includes the
|| "0"fallback as suggested in past review comments, ensuring proper initialization for the ControllerSwitch component.
194-195: LGTM: Button props are correct.The past review comment suggested adding
size="lg", but it's already present on line 194. The void wrapper on line 195 correctly handles the async submission.
🧹 Nitpick comments (3)
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (1)
161-177: Async/await refactor improves readability.The migration from promise chaining to async/await with try/catch is cleaner. However, consider showing a user-facing error toast on failure for better UX, similar to the success toast.
} catch (err) { console.error(err); + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to save GitHub authentication configuration. Please try again.", + }); }apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (1)
144-160: Async/await refactor improves code clarity.The change to async/await is cleaner than promise chaining. Consider adding an error toast for user feedback on failure, matching the success toast pattern.
} catch (err) { console.error(err); + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to save GitLab authentication configuration. Please try again.", + }); }apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (1)
140-156: Async/await refactor improves maintainability.The migration to async/await is cleaner. Consider adding a user-facing error toast for consistency with the success notification.
} catch (err) { console.error(err); + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to save Gitea authentication configuration. Please try again.", + }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx(5 hunks)apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx(5 hunks)apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx(5 hunks)apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Enable TypeScript strict mode and ensure all files are fully typed
Files:
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx
**/*.{js,jsx,ts,tsx,json,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier with Tailwind plugin for code formatting, run
pnpm fix:format
Files:
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use ESLint with shared config across packages, adhering to max warnings limits per package
Use camelCase for variable and function names, PascalCase for components and types
Use try-catch with proper error types and log errors appropriately
Files:
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx
🧠 Learnings (3)
📚 Learning: 2025-07-14T11:22:43.964Z
Learnt from: gakshita
Repo: makeplane/plane PR: 7393
File: apps/admin/app/(all)/(dashboard)/email/email-config-form.tsx:104-104
Timestamp: 2025-07-14T11:22:43.964Z
Learning: In the Plane project's SMTP configuration implementation, the email configuration form (email-config-form.tsx) hardcodes ENABLE_SMTP to "1" in form submission because the form is only rendered when SMTP is enabled. The enable/disable functionality is managed at the page level (page.tsx) with a toggle, and the form only handles configuration details when SMTP is already enabled.
Applied to files:
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx
📚 Learning: 2025-08-06T09:01:17.915Z
Learnt from: shivam-jainn
Repo: makeplane/plane PR: 7545
File: apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx:139-143
Timestamp: 2025-08-06T09:01:17.915Z
Learning: In the Plane project's Gitea authentication form (apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx), the GITEA_HOST field should not be included in the form reset after submission. It should default back to "https://gitea.com" rather than maintaining the submitted value, as this provides a sensible default for most users.
Applied to files:
apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx
🧬 Code graph analysis (1)
apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (3)
apps/admin/core/components/common/controller-switch.tsx (2)
TControllerSwitchFormField(11-14)ControllerSwitch(16-40)packages/propel/src/toast/toast.tsx (1)
setToast(245-265)apps/space/core/store/root.store.ts (1)
reset(62-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check:lint
- GitHub Check: check:types
- GitHub Check: Analyze (javascript)
🔇 Additional comments (12)
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (4)
15-16: LGTM: ControllerSwitch imports added correctly.The type and component imports are necessary for the new synchronization toggle feature.
108-111: LGTM: Switch field definition is correct.The GITHUB_FORM_SWITCH_FIELD properly defines the field descriptor for the ControllerSwitch component with appropriate name and label.
210-210: LGTM: ControllerSwitch integration is correct.The component is properly bound to the form control and switch field descriptor.
216-216: LGTM: Void wrapper correctly handles async submit.The
voidwrapper properly discards the promise returned byhandleSubmit(onSubmit)(e)to satisfy event handler typing.apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (4)
14-15: LGTM: ControllerSwitch imports added correctly.The imports are consistent with the other authentication forms and necessary for the new synchronization toggle feature.
113-116: LGTM: Switch field definition follows established pattern.The GITLAB_FORM_SWITCH_FIELD is correctly defined and consistent with other provider forms.
193-193: LGTM: ControllerSwitch properly integrated.The component is correctly bound to the form and renders the synchronization toggle.
199-199: LGTM: Void wrapper correctly handles async submission.The
voidoperator properly discards the promise for event handler compatibility.apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (4)
15-16: LGTM: ControllerSwitch imports consistent with other forms.The imports are correct and follow the established pattern across authentication forms.
109-112: LGTM: Switch field definition is correct.The GITEA_FORM_SWITCH_FIELD follows the same pattern as other provider forms.
148-148: Verify GITEA_HOST reset behavior aligns with expectations.Based on learnings, the GITEA_HOST field should default back to "https://gitea.com" rather than maintaining the submitted value. The current implementation resets to the response value, which may differ from the intended behavior.
Based on learnings, GITEA_HOST should not be included in the reset or should default to "https://gitea.com". Please confirm whether the current behavior is intentional.
189-189: LGTM: ControllerSwitch integration is correct.The component is properly bound to form control and renders the synchronization toggle.
Description
Type of Change
Test Scenarios
References
WEB-5657
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.