[WEB-3065] refactor: replace admin services with service packages#6342
[WEB-3065] refactor: replace admin services with service packages#6342sriramveeraghanta merged 3 commits intopreviewfrom
Conversation
WalkthroughThis pull request involves significant refactoring of service modules within the Plane project, focusing on restructuring service imports and modifying service classes. The changes primarily include updating import paths to a centralized Changes
Sequence DiagramsequenceDiagram
participant Client
participant Services
participant API
Client->>Services: Request workspace/instance data
Services->>API: Make HTTP request
API-->>Services: Return response data
Services-->>Client: Provide processed data
Possibly related PRs
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/services/src/auth/auth.service.ts (1)
Line range hint
89-116: Consider environment-agnostic approach for signOutThe
signOutmethod uses direct DOM manipulation which might not work in all environments (e.g., SSR). Consider using the API service's post method instead.async signOut(baseUrl: string): Promise<any> { - await this.requestCSRFToken().then((data) => { - const csrfToken = data?.csrf_token; - - if (!csrfToken) throw Error("CSRF token not found"); - - const form = document.createElement("form"); - const element1 = document.createElement("input"); - - form.method = "POST"; - form.action = `${baseUrl}/auth/sign-out/`; - - element1.value = csrfToken; - element1.name = "csrfmiddlewaretoken"; - element1.type = "hidden"; - form.appendChild(element1); - - document.body.appendChild(form); - - form.submit(); - }); + const { csrf_token } = await this.requestCSRFToken(); + if (!csrf_token) throw Error("CSRF token not found"); + + return this.post( + `${baseUrl}/auth/sign-out/`, + { csrfmiddlewaretoken: csrf_token }, + { + headers: { + "X-CSRFTOKEN": csrf_token + } + } + ); }
🧹 Nitpick comments (16)
packages/services/src/project/view.service.ts (1)
6-6: Add class-level JSDoc documentationConsider adding comprehensive class-level documentation to describe the purpose and responsibilities of this service.
+/** + * Service responsible for managing project views and their operations + * Handles CRUD operations for project views, including custom views and filters + */ export class ProjectViewService extends APIService {packages/services/src/ai/ai.service.ts (1)
Line range hint
21-71: Consider enhancing type safety with specific error and response typesThe implementation is solid with good error handling and documentation. Consider these type safety improvements:
- Define specific error types:
type AIServiceError = { status: number; message: string; details?: Record<string, unknown>; };
- Create detailed response interfaces:
interface GPTTaskResponse { result: string; metadata?: { model: string; processingTime: number; }; } interface GrammarResponse { response: string; corrections?: Array<{ original: string; corrected: string; explanation?: string; }>; }
- Update method signatures to use these types:
async prompt(workspaceSlug: string, data: { prompt: string; task: string }): Promise<GPTTaskResponse> async rephraseGrammar(workspaceSlug: string, data: TTaskPayload): Promise<GrammarResponse>packages/services/src/user/user.service.ts (1)
26-32: Consider adding request parameters type safetyThe
adminDetailsmethod could benefit from explicit typing of the response structure.- async adminDetails(): Promise<IUser> { + async adminDetails(): Promise<{ data: IUser }> { return this.get("/api/instances/admins/me/") - .then((response) => response?.data) + .then((response) => response) .catch((error) => { - throw error?.response; + throw error?.response?.data; }); }packages/services/src/workspace/instance-workspace.service.ts (1)
44-45: Consider using URLSearchParams more efficientlyThe URLSearchParams instantiation and string conversion can be simplified.
- const params = new URLSearchParams({ slug }); - return this.get(`/api/instances/workspace-slug-check/?${params.toString()}`) + return this.get("/api/instances/workspace-slug-check/", { + params: { slug } + })admin/core/store/user.store.ts (2)
59-59: Consider adding error type safetyThe error handling could benefit from stronger typing instead of using
any.- } catch (error: any) { + } catch (error: Error | { status: number; message: string }) {
Line range hint
31-35: Consider implementing dependency injectionServices are directly instantiated in the constructor. Consider implementing dependency injection for better testability.
- constructor(private store: CoreRootStore) { + constructor( + private store: CoreRootStore, + private userService = new UserService(), + private authService = new AuthService() + ) { makeObservable(this, { // ... observable definitions }); - this.userService = new UserService(); - this.authService = new AuthService(); }admin/core/store/workspace.store.ts (2)
Line range hint
87-95: Consider adding error type information.The error handling is present but could be improved by adding type information for better error handling.
- } catch (error) { + } catch (error: unknown) { console.error("Error fetching workspaces", error); - throw error; + throw error instanceof Error ? error : new Error("Failed to fetch workspaces");
Line range hint
138-146: Consider adding data validation.The workspace creation implementation looks correct, but consider adding data validation before making the API call.
createWorkspace = async (data: IWorkspace): Promise<IWorkspace> => { try { this.loader = "mutation"; + // Validate required fields + if (!data.name || !data.slug) { + throw new Error("Workspace name and slug are required"); + } const workspace = await this.instanceWorkspaceService.create(data);admin/core/components/admin-sidebar/sidebar-dropdown.tsx (1)
10-10: Fix import formatting.There's an extra space after
importin the AuthService import statement.-import {AuthService } from "@plane/services"; +import { AuthService } from "@plane/services";admin/core/store/instance.store.ts (3)
5-5: Fix import formatting.The import statement needs consistent spacing.
-import {InstanceService} from "@plane/services"; +import { InstanceService } from "@plane/services";
Line range hint
98-115: Remove console.log statement.There's a debug console.log statement that should be removed before production.
- console.log("instanceInfo: ", instanceInfo); this.isLoading = false;
Line range hint
127-137: Consider implementing retry logic for API calls.The service methods handle errors appropriately, but for better reliability, consider implementing retry logic for transient failures.
You could create a utility function to handle retries:
const withRetry = async <T>( operation: () => Promise<T>, maxRetries: number = 3, delay: number = 1000 ): Promise<T> => { let lastError: Error; for (let i = 0; i < maxRetries; i++) { try { return await operation(); } catch (error) { lastError = error instanceof Error ? error : new Error(String(error)); if (i < maxRetries - 1) await new Promise(resolve => setTimeout(resolve, delay)); } } throw lastError!; };Then use it in your service calls:
updateInstanceInfo = async (data: Partial<IInstance>) => { try { - const instanceResponse = await this.instanceService.update(data); + const instanceResponse = await withRetry(() => this.instanceService.update(data));Also applies to: 146-152, 161-167, 176-189
packages/services/src/instance/instance.service.ts (4)
59-65: Standardize error handling inadminsmethodIn the
admins()method, errors are caught and re-thrown usingthrow error;, whereas other methods likeupdate()andupdateConfigurations()throwerror?.response?.data;. For consistency and to provide more informative error messages, consider standardizing the error handling across all methods to ensure they either all throw the full error object or a specific error response.
73-79: Ensure consistent error handling inupdatemethodThe
update()method throwserror?.response?.data;when an error occurs. To maintain consistency and provide detailed error information, consider standardizing the error handling across all methods. Decide whether to throw the full error object or a specific part of it, and apply this consistently.
86-92: Standardize error handling inconfigurationsmethodThe
configurations()method re-throws errors asthrow error;, differing from methods that throwerror?.response?.data;. To provide consistent and informative error handling, consider aligning this method's error handling with the others.
100-106: Ensure consistent error handling inupdateConfigurationsmethodIn
updateConfigurations(), errors are thrown aserror?.response?.data;. For consistency and clarity in error handling across all methods, consider standardizing the error handling approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
admin/app/email/test-email-modal.tsx(1 hunks)admin/app/workspace/create/form.tsx(2 hunks)admin/core/components/admin-sidebar/sidebar-dropdown.tsx(1 hunks)admin/core/components/instance/setup-form.tsx(1 hunks)admin/core/components/login/sign-in-form.tsx(1 hunks)admin/core/services/api.service.ts(0 hunks)admin/core/services/auth.service.ts(0 hunks)admin/core/services/instance.service.ts(0 hunks)admin/core/services/user.service.ts(0 hunks)admin/core/services/workspace.service.ts(0 hunks)admin/core/store/instance.store.ts(6 hunks)admin/core/store/user.store.ts(2 hunks)admin/core/store/workspace.store.ts(6 hunks)admin/package.json(1 hunks)packages/services/src/ai/ai.service.ts(1 hunks)packages/services/src/auth/auth.service.ts(1 hunks)packages/services/src/cycle/cycle-analytics.service.ts(1 hunks)packages/services/src/cycle/cycle-archive.service.ts(1 hunks)packages/services/src/cycle/cycle-operations.service.ts(1 hunks)packages/services/src/cycle/cycle.service.ts(1 hunks)packages/services/src/developer/api-token.service.ts(1 hunks)packages/services/src/instance/instance.service.ts(2 hunks)packages/services/src/intake/intake.service.ts(1 hunks)packages/services/src/intake/issue.service.ts(1 hunks)packages/services/src/module/link.service.ts(1 hunks)packages/services/src/module/module.service.ts(1 hunks)packages/services/src/module/operations.service.ts(1 hunks)packages/services/src/project/index.ts(1 hunks)packages/services/src/project/view.service.ts(1 hunks)packages/services/src/user/favorite.service.ts(1 hunks)packages/services/src/user/index.ts(1 hunks)packages/services/src/user/user.service.ts(1 hunks)packages/services/src/workspace/index.ts(1 hunks)packages/services/src/workspace/instance-workspace.service.ts(1 hunks)packages/services/src/workspace/invitation.service.ts(1 hunks)packages/services/src/workspace/view.service.ts(1 hunks)packages/services/tsconfig.json(0 hunks)
💤 Files with no reviewable changes (6)
- packages/services/tsconfig.json
- admin/core/services/instance.service.ts
- admin/core/services/api.service.ts
- admin/core/services/auth.service.ts
- admin/core/services/workspace.service.ts
- admin/core/services/user.service.ts
✅ Files skipped from review due to trivial changes (14)
- packages/services/src/project/index.ts
- packages/services/src/module/operations.service.ts
- packages/services/src/cycle/cycle-analytics.service.ts
- packages/services/src/workspace/view.service.ts
- packages/services/src/workspace/invitation.service.ts
- packages/services/src/cycle/cycle-operations.service.ts
- packages/services/src/cycle/cycle-archive.service.ts
- packages/services/src/developer/api-token.service.ts
- packages/services/src/intake/intake.service.ts
- packages/services/src/user/favorite.service.ts
- packages/services/src/intake/issue.service.ts
- packages/services/src/module/link.service.ts
- admin/core/components/instance/setup-form.tsx
- packages/services/src/cycle/cycle.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint-web
- GitHub Check: lint-admin
- GitHub Check: lint-space
- GitHub Check: Analyze (javascript)
🔇 Additional comments (16)
packages/services/src/project/view.service.ts (2)
6-14: Implementation appears incompleteThe service class is currently empty without any methods for handling project view operations. Consider implementing methods for:
- Creating project views
- Retrieving project views
- Updating project views
- Deleting project views
Would you like me to help generate the implementation for these essential project view operations?
1-4: Verify the APIService import pathSince this PR involves refactoring services, let's verify that the APIService import path is correct and the service exists at the specified location.
✅ Verification successful
Import path for APIService is correct
The relative import path "../api.service" correctly points to the APIService class which exists at packages/services/src/api.service.ts and is widely used as the base class across the services package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and location of api.service.ts # Check if api.service.ts exists in the expected location fd -e ts "api.service.ts" packages/services/src/ # Check the content of api.service.ts to verify it exports APIService rg -l "export.*class.*APIService" packages/services/src/Length of output: 1466
packages/services/src/ai/ai.service.ts (2)
Line range hint
7-19: Well-structured type definition with comprehensive documentationThe TTaskPayload type is well-documented with JSDoc comments and has a clear structure that supports both required and optional fields.
4-4: Verify the relative import path resolutionThe change from absolute to relative import path aligns with the service package refactoring. Let's verify that the path resolves correctly.
✅ Verification successful
Relative import path is correctly resolved
The import path
"../api.service"correctly resolves to the APIService class and follows the consistent pattern used by other services in the same package.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and location of api.service.ts # relative to the current file's location # Check if api.service.ts exists at the expected location fd --type f "api.service.ts" --exec echo "Found api.service at: {}" # Verify there are no lingering references to the old import path rg -l "@/api.service"Length of output: 328
Script:
#!/bin/bash # Verify APIService class exists in the target file ast-grep --pattern 'export class APIService { $$$ }' packages/services/src/api.service.ts # Check if there are any other files importing APIService rg "import.*APIService.*from" -g "!api.service.ts"Length of output: 8373
packages/services/src/auth/auth.service.ts (1)
12-12: LGTM: Export change aligns with module system best practicesThe change from default export to named export improves consistency and makes imports more explicit.
admin/core/store/workspace.store.ts (2)
3-4: Service initialization looks good.The migration from
WorkspaceServicetoInstanceWorkspaceServiceis properly implemented with correct import and initialization.Also applies to: 32-32, 51-51
Line range hint
112-120: Verify pagination cursor handling.The pagination implementation looks correct, but ensure that the cursor handling in
list()method ofInstanceWorkspaceServicematches the expected format.✅ Verification successful
Cursor handling implementation verified successfully.
The pagination implementation in both the workspace store and InstanceWorkspaceService is correct and type-safe. The cursor is properly passed to the API and the pagination information is correctly handled in the response.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the implementation of cursor handling in InstanceWorkspaceService ast-grep --pattern 'class InstanceWorkspaceService { $$$ list($cursor) { $$$ } $$$ }'Length of output: 99
Script:
#!/bin/bash # Find files that might contain InstanceWorkspaceService fd -t f workspace # Search for InstanceWorkspaceService class or interface rg "class InstanceWorkspaceService|interface InstanceWorkspaceService" -A 5 # Try alternative ast-grep pattern for TypeScript ast-grep --pattern 'export class InstanceWorkspaceService { $$$ async list($_): Promise<$_> { $$$ } $$$ }'Length of output: 4998
Script:
#!/bin/bash # View the implementation file cat packages/services/src/workspace/instance-workspace.service.ts # Search for pagination-related types rg "interface.*Paginated|type.*Paginated" -A 3Length of output: 2971
Script:
#!/bin/bash # Check the workspace pagination type definition rg "type.*TWorkspacePaginationInfo|interface.*TWorkspacePaginationInfo" -A 5 packages/types/src/Length of output: 287
admin/app/email/test-email-modal.tsx (1)
3-4: LGTM! Clean import refactoring.The import path update aligns with the service package restructuring.
packages/services/src/module/module.service.ts (2)
4-4: LGTM! Import path updated correctly.The change from alias import to relative path aligns with the service package refactoring.
Line range hint
41-42: Improved type safety and REST compliance.The update method now:
- Uses
Partial<IModule>type for better type safety- Uses PATCH instead of PUT, following REST conventions for partial updates
admin/core/components/login/sign-in-form.tsx (1)
8-8: LGTM! Service import updated correctly.Import updated to use the centralized @plane/services package.
admin/app/workspace/create/form.tsx (2)
7-7: LGTM! Service migration implemented correctly.Successfully migrated from WorkspaceService to InstanceWorkspaceService.
Also applies to: 14-14
41-42: Method call updated appropriately.The workspaceSlugCheck method call has been correctly updated to use slugCheck from the new service.
admin/package.json (1)
21-21: LGTM! Required dependency added.Added @plane/services package dependency to support the service refactoring.
packages/services/src/user/index.ts (1)
2-2: Confirming export ofuser.serviceAdding
export * from "./user.service";correctly exposes all exports fromuser.service. This change enhances the module's public API and maintains consistency with other service exports.packages/services/src/workspace/index.ts (1)
6-6: Confirming export ofinstance-workspace.serviceIncluding
export * from "./instance-workspace.service";appropriately expands the workspace service exports. This ensures that functionalities frominstance-workspace.serviceare accessible where needed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/services/src/instance/instance.service.ts (1)
114-122:⚠️ Potential issueAdjust return type or implementation of
sendTestEmailmethodThe method is declared to return
Promise<void>, but it returnsresponse?.data. This mismatch should be addressed.Apply this diff to align the implementation with the declared return type:
async sendTestEmail(receiverEmail: string): Promise<void> { return this.post("/api/instances/email-credentials-check/", { receiver_email: receiverEmail, }) - .then((response) => response?.data) + .then(() => undefined) .catch((error) => { throw error?.response?.data; }); }
🧹 Nitpick comments (1)
packages/services/src/instance/instance.service.ts (1)
Line range hint
19-122: Consider implementing centralized error handling.Given the service's role in the refactoring effort and the observed inconsistencies in error handling, consider implementing a centralized error handling mechanism in the base
APIServiceclass. This would ensure consistent error propagation across all derived services.This could be achieved by:
- Adding an error handling method in
APIService- Using this method consistently across all service methods
- Standardizing error response formats
Would you like me to provide a detailed implementation proposal for this architectural improvement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/services/src/instance/instance.service.ts(2 hunks)packages/services/src/user/user.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/services/src/user/user.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint-web
- GitHub Check: lint-space
- GitHub Check: lint-admin
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/services/src/instance/instance.service.ts (3)
1-19: Well-structured imports and class documentation!The imports are properly organized, and the class documentation follows JSDoc conventions effectively.
73-79: LGTM! Well-implemented update method.The method is properly typed, uses appropriate HTTP method (PATCH), and has consistent error handling.
100-106: LGTM! Well-implemented configuration update method.The method is properly typed, uses appropriate HTTP method (PATCH), and has consistent error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/services/src/instance/instance.service.ts (1)
108-121:⚠️ Potential issueAlign implementation with declared return type.
The method is declared to return
Promise<void>but returnsresponse?.data. This issue was previously identified in the past review.Apply this diff to align the implementation with the declared return type:
async sendTestEmail(receiverEmail: string): Promise<void> { return this.post("/api/instances/email-credentials-check/", { receiver_email: receiverEmail, }) - .then((response) => response?.data) + .then(() => undefined) .catch((error) => { throw error?.response?.data; }); }
🧹 Nitpick comments (1)
packages/services/src/instance/instance.service.ts (1)
54-92: Standardize response data access pattern.Some methods use
response.datawhile others useresponse?.data. Consider standardizing to use optional chaining consistently:async admins(): Promise<IInstanceAdmin[]> { return this.get("/api/instances/admins/") - .then((response) => response.data) + .then((response) => response?.data) .catch((error) => { throw error?.response?.data; }); } async configurations(): Promise<IInstanceConfiguration[]> { return this.get("/api/instances/configurations/") - .then((response) => response.data) + .then((response) => response?.data) .catch((error) => { throw error?.response?.data; }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/services/src/instance/instance.service.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-web
- GitHub Check: build-admin
- GitHub Check: build-space
🔇 Additional comments (2)
packages/services/src/instance/instance.service.ts (2)
1-19: LGTM! Well-organized imports and clear class documentation.The imports are properly grouped and documented, with clear type definitions imported from the shared types package.
Line range hint
26-52: LGTM! Consistent error handling implementation.The error handling in both methods now follows the standardized pattern of throwing
error?.response?.data, addressing the consistency issues raised in previous reviews.
2 participants
Description
In this PR, I have replaced all the admin application services with service package
Type of Change
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
Services Refactoring
@plane/servicespackage.New Services
InstanceWorkspaceServicefor workspace management.ProjectViewServicefor project view operations.UserServicefor retrieving admin user details.Import and Path Changes
Dependency Management
@plane/servicesas a new dependency.These changes represent a significant restructuring of the services layer to improve modularity and consistency across the application.