[WEB-3066] refactor: replace Space Services with Services Package #6353
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a comprehensive refactoring of service classes across the Plane application, focusing on creating specialized "Sites" services for different domains like authentication, issues, files, cycles, and more. The changes centralize service implementations, improve type safety, and standardize method naming conventions. The refactoring involves removing existing service classes, introducing new site-specific services, and updating import paths and method calls throughout the application. Changes
Possibly Related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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: 11
🔭 Outside diff range comments (1)
space/core/store/module.store.ts (1)
Line range hint
58-67: Fix incorrect error message in catch blockThe error message refers to "members" instead of "modules".
- console.error("Failed to fetch members:", error); + console.error("Failed to fetch modules:", error);
🧹 Nitpick comments (26)
space/core/store/profile.store.ts (1)
Line range hint
118-133: Consider enhancing error handling in the catch block.While the method name change to
updateProfileis good, the error handling could be improved:
- Add proper error typing instead of using a generic
error- Consider logging the error for debugging purposes
- The error message could be more specific based on the actual error
Here's a suggested improvement:
- } catch (error) { + } catch (error: unknown) { + console.error("Failed to update user profile:", error); if (currentUserProfileData) { Object.keys(currentUserProfileData).forEach((key: string) => { const userKey: keyof TUserProfile = key as keyof TUserProfile; if (this.data) set(this.data, userKey, currentUserProfileData[userKey]); }); } runInAction(() => { this.error = { status: "user-profile-update-error", - message: "Failed to update user profile", + message: error instanceof Error ? error.message : "Failed to update user profile", }; }); }packages/services/src/file/sites-file.service.ts (3)
24-27: Bind methods after callingsuper()in the constructorAccessing
thisbefore callingsuper()can lead to errors in TypeScript classes. It's recommended to callsuper()before usingthis.Apply this diff to rearrange the constructor:
constructor(BASE_URL?: string) { + super(BASE_URL || API_BASE_URL); this.cancelUpload = this.cancelUpload.bind(this); // services this.fileUploadService = new FileUploadService(); - super(BASE_URL || API_BASE_URL); }
Line range hint
37-44: Refactor repetitive error handling into a helper methodThe error handling in the
catchblocks is repetitive across multiple methods. Refactoring this into a private helper method will improve maintainability and reduce code duplication.Add a private method to handle errors:
private handleError(error: any): never { throw error?.response?.data || error.message; }Then, update the
catchblocks in your methods:.catch((error) => { - throw error?.response?.data; + this.handleError(error); });Also applies to: 57-65, 83-91, 103-111
70-75: Add type safety to method parameters and return typesThe
uploadAssetmethod acceptsdata: TFileEntityInfo, but thefileMetaDatais spread into this object without explicit type checking. Ensure that the combined data conforms to the expected request payload structure.Consider defining a specific type for the request payload and ensure all properties are accounted for.
packages/services/src/issue/sites-issue.service.ts (3)
11-11: Clarify the documentation remark for proper capitalizationThe remark in the class documentation refers to "plane sites." If "Plane Sites" is a specific term or product name, it should be capitalized correctly.
Consider updating the remark:
* @remarks This service is only available for plane sites +* @remarks This service is only available for Plane sites.
25-244: Enhance type safety by defining specific typesMany methods use
anyfor parameters and return types, which reduces type safety and can lead to runtime errors. Define specific interfaces or types for request parameters (data) and responses to improve code reliability.For example, define types like
IVoteData,IReactionData,ICommentData, and use them in method signatures:async addVote(anchor: string, issueID: string, data: IVoteData): Promise<IVoteResponse> { /* ... */ }Similarly, replace
Promise<any>with more specific types reflecting the expected response data.
30-32: Improve error handling for clearer error messagesCurrently, the
catchblocks throwerror?.response, which may not contain meaningful information. Update the error handling to throw the most informative error message possible.Apply this diff to adjust the error handling:
.catch((error) => { - throw error?.response; + throw error?.response?.data || error.message; });Alternatively, create a private method to handle errors consistently:
private handleError(error: any): never { throw error?.response?.data || error.message; }And use it in your
catchblocks:.catch((error) => this.handleError(error));Also applies to: 45-47, 60-62, 75-77, 90-92, 105-107, 121-123, 136-138, 151-153, 168-170, 185-187, 200-202, 222-224, 238-240
packages/services/src/user/index.ts (1)
1-3: Consider maintaining alphabetical order for exports.While the export statement is correct, consider reordering the exports alphabetically for better maintainability:
export * from "./favorite.service"; +export * from "./sites-member.service"; export * from "./user.service"; -export * from "./sites-member.service";packages/services/src/module/index.ts (1)
1-4: Consider maintaining alphabetical order for exports.While the export statement is correct, consider reordering the exports alphabetically for better maintainability:
export * from "./link.service"; export * from "./module.service"; export * from "./operations.service"; -export * from "./sites-module.service"; +export * from "./sites-module.service";packages/services/src/cycle/index.ts (1)
Line range hint
1-5: Consider maintaining alphabetical order for exports.While the export statement is correct, consider reordering the exports alphabetically for better maintainability:
export * from "./cycle-analytics.service"; export * from "./cycle-archive.service"; export * from "./cycle-operations.service"; export * from "./cycle.service"; -export * from "./sites-cycle.service"; +export * from "./sites-cycle.service";packages/services/src/index.ts (1)
13-16: Consider maintaining alphabetical order for exportsThe new exports should be integrated into the existing alphabetical order for better maintainability and easier navigation.
export * from "./dashboard"; +export * from "./file"; export * from "./instance"; export * from "./intake"; +export * from "./issue"; +export * from "./label"; export * from "./module"; export * from "./project"; +export * from "./state"; export * from "./user"; export * from "./workspace"; -export * from "./file"; -export * from "./label"; -export * from "./state"; -export * from "./issue";packages/services/src/state/sites-state.service.ts (2)
24-30: Enhance error handling robustnessThe error handling could be more robust by handling cases where
error.responsemight be undefined.async list(anchor: string): Promise<IState[]> { return this.get(`/api/public/anchor/${anchor}/states/`) .then((response) => response?.data) .catch((error) => { - throw error?.response?.data; + throw error?.response?.data || error?.message || 'Failed to fetch states'; }); }
7-12: Enhance documentation with @SInCE tagConsider adding a @SInCE tag to indicate when this class was introduced.
/** * Service class for managing states within plane sites application. * Extends APIService to handle HTTP requests to the state-related endpoints. * @extends {APIService} * @remarks This service is only available for plane sites + * @since 1.0.0 */packages/services/src/label/sites-label.service.ts (1)
24-30: Add type safety to error handling and response validationThe error and response handling could be improved with proper type definitions and validation:
- async list(anchor: string): Promise<IIssueLabel[]> { + type APIError = { + response?: { + data: { + error: string; + message: string; + }; + }; + }; + + async list(anchor: string): Promise<IIssueLabel[]> { return this.get(`/api/public/anchor/${anchor}/labels/`) - .then((response) => response?.data) + .then((response) => { + if (!response?.data) throw new Error('Invalid response format'); + return response.data; + }) - .catch((error) => { + .catch((error: APIError) => { throw error?.response?.data; }); }packages/services/src/module/sites-module.service.ts (1)
13-31: Consider creating a base class for sites servicesMultiple services (SitesLabelService, SitesModuleService) share similar patterns for list operations and error handling. Consider extracting common functionality into a base class.
abstract class BaseSitesService<T> extends APIService { constructor(BASE_URL?: string) { super(BASE_URL || API_BASE_URL); } protected async getList<T>(endpoint: string): Promise<T[]> { return this.get(endpoint) .then((response) => { if (!response?.data) throw new Error('Invalid response format'); return response.data; }) .catch((error: APIError) => { throw error?.response?.data; }); } } export class SitesModuleService extends BaseSitesService<TPublicModule> { async list(anchor: string): Promise<TPublicModule[]> { return this.getList(`/api/public/anchor/${anchor}/modules/`); } }packages/services/src/cycle/sites-cycle.service.ts (1)
18-23: Standardize JSDoc parameter descriptions across servicesThe parameter description style varies across services. Consider standardizing the format:
/** * Retrieves list of cycles for a specific anchor. - * @param anchor - The anchor identifier for the published entity + * @param {string} anchor - The anchor identifier for the published entity * @returns {Promise<TPublicCycle[]>} The list of cycles * @throws {Error} If the request fails */packages/services/src/user/sites-member.service.ts (2)
1-6: Consider reorganizing directory structureThe member service is currently in the
userdirectory. Consider creating a dedicatedmemberdirectory for better organization and separation of concerns.- packages/services/src/user/sites-member.service.ts + packages/services/src/member/sites-member.service.ts
1-31: Consider implementing architectural improvements across all servicesSeveral architectural improvements could enhance the maintainability and reliability of these services:
- Create a shared base class for common functionality
- Implement consistent error handling with proper types
- Add input validation for anchor parameters
- Standardize documentation format
- Consider using OpenAPI/Swagger for API documentation and type generation
Would you like me to provide a detailed implementation plan for these improvements?
packages/services/src/file/file-upload.service.ts (1)
Line range hint
24-44: Consider improving error handling and type safety.
- The
cancelSourcetype could be more specific thanany- Error handling could be more structured with custom error types
- private cancelSource: any; + private cancelSource: ReturnType<typeof axios.CancelToken.source>; async uploadFile( url: string, data: FormData, ): Promise<void> { this.cancelSource = axios.CancelToken.source(); return this.post(url, data, { headers: { "Content-Type": "multipart/form-data", }, cancelToken: this.cancelSource.token, withCredentials: false, }) .then((response) => response?.data) .catch((error) => { if (axios.isCancel(error)) { console.log(error.message); + throw new UploadCanceledError(error.message); } else { - throw error?.response?.data; + throw new UploadFailedError(error?.response?.data); } }); }packages/services/src/file/helper.ts (1)
32-36: Consider making URL parsing more robust.The current implementation might break with malformed URLs or URLs with query parameters.
export const getAssetIdFromUrl = (src: string): string => { - const sourcePaths = src.split("/"); - const assetUrl = sourcePaths[sourcePaths.length - 1]; + try { + const url = new URL(src); + const pathname = url.pathname; + const assetUrl = pathname.split("/").pop() || ""; + return assetUrl.split("?")[0]; // Remove query parameters if any + } catch { + // Handle invalid URLs + const sourcePaths = src.split("/"); + return (sourcePaths.pop() || "").split("?")[0]; + } - return assetUrl; };space/core/store/members.store.ts (1)
Line range hint
57-66: Fix error message in catch blockThe error message matches the context of the operation being performed.
- console.error("Failed to fetch members:", error); + console.error("Failed to fetch members:", error);packages/services/src/file/file.service.ts (2)
28-34: Consider adding specific error typesThe error handling is consistent, but consider defining specific error types for better error handling in consuming code.
Example implementation:
type FileServiceError = { code: string; message: string; details?: unknown; }; async deleteNewAsset(assetPath: string): Promise<void> { return this.delete(assetPath) .then((response) => response?.data) .catch((error) => { const serviceError: FileServiceError = { code: error?.response?.status, message: error?.response?.data?.message || 'Unknown error', details: error?.response?.data }; throw serviceError; }); }
43-50: Enhance API endpoint constructionConsider extracting API endpoints as constants to improve maintainability.
Example implementation:
private static readonly ENDPOINTS = { fileAssets: (workspaceId: string, assetKey: string) => `/api/workspaces/file-assets/${workspaceId}/${assetKey}`, restore: (workspaceId: string, assetKey: string) => `${FileService.ENDPOINTS.fileAssets(workspaceId, assetKey)}/restore/` };Also applies to: 59-66
packages/types/src/cycle/cycle.d.ts (1)
136-140: Consider adding more cycle metadataThe TPublicCycle type includes basic cycle information. Consider adding more metadata that might be useful for public views, such as:
- start_date
- end_date
- progress statistics
export type TPublicCycle = { id: string; name: string; status: string; + start_date?: string; + end_date?: string; + progress?: { + completed: number; + total: number; + }; };packages/services/src/api.service.ts (1)
37-49: Improve URL handling with a dedicated URL utility.The current implementation handles path prefixes directly in the interceptor. Consider extracting this logic into a dedicated URL utility function for better maintainability and reusability.
+ // Add to a new file: utils/url.ts + export const getRedirectUrl = (currentPath: string): { prefix: string; path: string } => { + const prefixes = ["/god-mode", "/spaces"]; + for (const prefix of prefixes) { + if (currentPath.startsWith(prefix)) { + return { + prefix, + path: currentPath.replace(prefix, "") + }; + } + } + return { prefix: "", path: currentPath }; + }; // In setupInterceptors: - let prefix = ""; - let updatedPath = currentPath; - - // Check for special path prefixes - if (currentPath.startsWith("/god-mode")) { - prefix = "/god-mode"; - updatedPath = currentPath.replace("/god-mode", ""); - } else if (currentPath.startsWith("/spaces")) { - prefix = "/spaces"; - updatedPath = currentPath.replace("/spaces", ""); - } + const { prefix, path: updatedPath } = getRedirectUrl(currentPath);packages/types/src/issues/issue.d.ts (1)
132-160: Consider extracting common response types.The
IPublicIssueinterface shares many fields withTIssue. Consider creating a base interface for common fields to reduce duplication.+ interface IBaseIssueFields { + description_html?: string; + created_at: string; + updated_at: string; + // ... other common fields + } - export interface IPublicIssue extends Pick<TIssue, ...> { + export interface IPublicIssue extends IBaseIssueFields {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (65)
packages/services/src/api.service.ts(1 hunks)packages/services/src/auth/index.ts(1 hunks)packages/services/src/auth/sites-auth.service.ts(1 hunks)packages/services/src/cycle/index.ts(1 hunks)packages/services/src/cycle/sites-cycle.service.ts(1 hunks)packages/services/src/file/file-upload.service.ts(2 hunks)packages/services/src/file/file.service.ts(1 hunks)packages/services/src/file/helper.ts(1 hunks)packages/services/src/file/index.ts(1 hunks)packages/services/src/file/sites-file.service.ts(5 hunks)packages/services/src/index.ts(1 hunks)packages/services/src/issue/index.ts(1 hunks)packages/services/src/issue/sites-issue.service.ts(1 hunks)packages/services/src/label/index.ts(1 hunks)packages/services/src/label/sites-label.service.ts(1 hunks)packages/services/src/module/index.ts(1 hunks)packages/services/src/module/sites-module.service.ts(1 hunks)packages/services/src/project/index.ts(1 hunks)packages/services/src/project/sites-publish.service.ts(1 hunks)packages/services/src/state/index.ts(1 hunks)packages/services/src/state/sites-state.service.ts(1 hunks)packages/services/src/user/index.ts(1 hunks)packages/services/src/user/sites-member.service.ts(1 hunks)packages/services/src/user/user.service.ts(2 hunks)packages/types/src/auth.d.ts(1 hunks)packages/types/src/cycle/cycle.d.ts(1 hunks)packages/types/src/issues/activity/issue_comment.d.ts(1 hunks)packages/types/src/issues/issue.d.ts(2 hunks)packages/types/src/module/modules.d.ts(1 hunks)packages/types/src/users.d.ts(1 hunks)space/app/[workspaceSlug]/[projectId]/page.ts(2 hunks)space/core/components/account/auth-forms/auth-root.tsx(2 hunks)space/core/components/account/auth-forms/password.tsx(1 hunks)space/core/components/account/auth-forms/unique-code.tsx(1 hunks)space/core/components/issues/navbar/user-avatar.tsx(1 hunks)space/core/components/issues/peek-overview/comment/add-comment.tsx(2 hunks)space/core/components/issues/peek-overview/comment/comment-detail-card.tsx(3 hunks)space/core/hooks/use-mention.tsx(1 hunks)space/core/services/api.service.ts(0 hunks)space/core/services/auth.service.ts(0 hunks)space/core/services/cycle.service.ts(0 hunks)space/core/services/instance.service.ts(0 hunks)space/core/services/issue.service.ts(0 hunks)space/core/services/label.service.ts(0 hunks)space/core/services/member.service.ts(0 hunks)space/core/services/module.service.ts(0 hunks)space/core/services/project-member.service.ts(0 hunks)space/core/services/publish.service.ts(0 hunks)space/core/services/state.service.ts(0 hunks)space/core/services/user.service.ts(0 hunks)space/core/store/cycle.store.ts(3 hunks)space/core/store/helpers/base-issues.store.ts(3 hunks)space/core/store/instance.store.ts(2 hunks)space/core/store/issue-detail.store.ts(15 hunks)space/core/store/issue.store.ts(5 hunks)space/core/store/label.store.ts(4 hunks)space/core/store/members.store.ts(4 hunks)space/core/store/module.store.ts(4 hunks)space/core/store/profile.store.ts(3 hunks)space/core/store/publish/publish_list.store.ts(2 hunks)space/core/store/state.store.ts(4 hunks)space/core/store/user.store.ts(3 hunks)space/core/types/issue.d.ts(2 hunks)space/helpers/editor.helper.ts(2 hunks)space/package.json(1 hunks)
💤 Files with no reviewable changes (12)
- space/core/services/member.service.ts
- space/core/services/instance.service.ts
- space/core/services/label.service.ts
- space/core/services/auth.service.ts
- space/core/services/state.service.ts
- space/core/services/cycle.service.ts
- space/core/services/module.service.ts
- space/core/services/api.service.ts
- space/core/services/publish.service.ts
- space/core/services/project-member.service.ts
- space/core/services/issue.service.ts
- space/core/services/user.service.ts
✅ Files skipped from review due to trivial changes (4)
- packages/services/src/state/index.ts
- packages/services/src/issue/index.ts
- packages/services/src/label/index.ts
- packages/services/src/file/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-space
- GitHub Check: build-admin
- GitHub Check: lint-web
🔇 Additional comments (50)
space/core/store/profile.store.ts (2)
3-4: LGTM! Import change aligns with services package refactoring.The updated import path from the new services package aligns well with the PR's objective of centralizing service implementations.
87-87: LGTM! Method name change improves clarity.The updated method name
profile()is more concise and follows RESTful naming conventions while maintaining the same functionality.space/core/store/instance.store.ts (2)
3-4: LGTM! Import path updated correctly.The import path change from local services to the new
@plane/servicespackage aligns with the PR objective of centralizing services.
Line range hint
1-77: Overall implementation looks solid!The store implementation follows good practices with proper:
- TypeScript interfaces and type safety
- Error handling with meaningful error messages
- Loading state management
- MobX decorators and actions
packages/services/src/auth/index.ts (1)
2-2: LGTM: Exportingsites-auth.servicefor extended authentication functionalitiesThe addition of
export * from "./sites-auth.service";correctly exposes the new authentication service functionalities. No issues found.packages/services/src/project/index.ts (1)
1-2: LGTM! Export statement follows TypeScript conventions.The addition of the sites-publish service export aligns with the PR objective of replacing space services.
packages/types/src/auth.d.ts (1)
10-10: LGTM! Well-structured type additionThe new
is_password_autosetboolean property is properly typed and follows existing naming conventions.space/core/hooks/use-mention.tsx (1)
3-4: LGTM! Import path updated correctlyThe import has been properly updated to use the centralized services package.
packages/services/src/label/sites-label.service.ts (1)
24-25: Consider input validation for the anchor parameterThe anchor parameter is used directly in the URL without sanitization.
space/app/[workspaceSlug]/[projectId]/page.ts (2)
2-3: LGTM! Service migration looks good.The change from
PublishServicetoSitesProjectPublishServicealigns with the PR's objective of replacing space services.Also applies to: 6-6
24-24: Method name change improves clarity.The new method name
retrieveSettingsByProjectIdbetter describes its purpose compared to the previousfetchAnchorFromProjectDetails.space/core/store/cycle.store.ts (2)
19-19: LGTM! Service migration is consistent.The change to
SitesCycleServicefollows the same pattern as other service migrations in this PR.Also applies to: 29-29
36-36: Verify response type compatibility.The method name has changed from
getCyclestolist. While the change works, please ensure that the response type fromlist()matches the expectedTPublicCycle[]type.✅ Verification successful
Type compatibility verified successfully
The
list()method inSitesCycleServicereturnsPromise<TPublicCycle[]>, which exactly matches the expected type in the store. The type safety is maintained after the method rename.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the list method definition in SitesCycleService ast-grep --pattern 'class SitesCycleService { $$$ list($_): $return { $$$ } $$$ }'Length of output: 96
Script:
#!/bin/bash # Search for any class with a list method that might be cycle-related ast-grep --pattern 'class $_ { $$$ list($_) { $$$ } $$$ }' # Search for files containing "Cycle" type definitions rg "type.*Cycle|interface.*Cycle" -g "*.ts" # Search for the actual service class being used rg "class.*CycleService" -g "*.ts"Length of output: 3468
Script:
#!/bin/bash # Check imports in the store file cat space/core/store/cycle.store.ts # Check the service implementations cat packages/services/src/cycle/sites-cycle.service.tsLength of output: 2352
packages/services/src/file/file-upload.service.ts (2)
5-9: Great addition of class documentation!The JSDoc comments improve code maintainability by clearly describing the class purpose.
17-23: Comprehensive method documentation.The detailed JSDoc for
uploadFilemethod clearly describes parameters, return type, and potential errors.packages/services/src/file/helper.ts (2)
9-14: LGTM! Well-structured file upload payload generator.The function is well-documented and properly typed with TypeScript.
21-25: LGTM! Clean metadata extraction.The function provides a clean interface for extracting file metadata.
space/core/store/publish/publish_list.store.ts (2)
3-4: LGTM: Clean service import updateThe import change aligns with the service refactoring initiative.
31-31: Verify service method name consistencyThe method name change from
fetchPublishSettingstoretrieveSettingsByAnchorsuggests a new naming convention. Let's verify this pattern across other site services.Also applies to: 39-39
✅ Verification successful
Naming convention is consistently applied
The new
retrieveBy*pattern is consistently used across the codebase:
retrieveSettingsByAnchorretrieveSettingsByProjectId🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for method naming patterns across site services rg -g "src/**/*service.ts" "retrieve.*ByAnchor|list" packages/services/Length of output: 73
Script:
#!/bin/bash # Search for both old and new method naming patterns echo "=== Searching for method naming patterns ===" rg "fetch\w+Settings|retrieve\w+ByAnchor" space/ echo -e "\n=== Looking for SitesProjectPublishService implementation ===" ast-grep --pattern 'class SitesProjectPublishService { $$$ }'Length of output: 1174
Script:
#!/bin/bash # Look for the service file and similar services echo "=== Searching for publish service files ===" fd "publish.*service" space/ echo -e "\n=== Searching for similar service patterns ===" rg -g "**/*service*" "class.*Service.*{" space/Length of output: 384
Script:
#!/bin/bash # Broader search for service and method patterns echo "=== Searching for service class definitions ===" rg "class.*Service" space/ echo -e "\n=== Searching for similar retrieve methods ===" rg "retrieve\w+By" space/ echo -e "\n=== Looking for publish service file ===" fd -e ts "publish.*" space/Length of output: 1049
space/core/store/state.store.ts (2)
3-4: LGTM: Clean service import updateThe import change aligns with the service refactoring initiative.
24-24: Consistent service refactoringThe changes follow a consistent pattern:
- Updated service type annotation
- Service instantiation in constructor
- Method name changed to follow RESTful conventions (
list)Also applies to: 36-36, 48-48
packages/types/src/issues/activity/issue_comment.d.ts (1)
41-66:⚠️ Potential issueFix undefined type references
The new
TIssuePublicCommenttype references several undefined types:
ActorDetailIssueDetailProjectDetailIWorkspaceLitePlease import these types or define them in this file.
Note: The change from
stringtoDatefor timestamps is a breaking change that needs to be handled carefully.space/core/store/label.store.ts (2)
3-4: LGTM: Clean service import updateThe import change aligns with the service refactoring initiative.
21-21: Consistent service refactoring patternThe changes follow the established pattern seen in other stores:
- Updated service type
- Constructor initialization
- RESTful method naming (
list)Let's verify the consistency of this pattern across all stores:
Also applies to: 33-33, 56-56
✅ Verification successful
Service refactoring pattern verified across stores
The consistent pattern is confirmed across multiple store files:
- Service type declarations (e.g.,
labelService: SitesLabelService)- Constructor initialization (e.g.,
this.labelService = new SitesLabelService())- RESTful method naming (e.g.,
listmethod)This pattern is consistently implemented in state.store.ts, issue.store.ts, module.store.ts, cycle.store.ts, members.store.ts, and other store files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar service refactoring patterns across store files rg -g "store.ts" "Sites.*Service.*=.*new.*Sites.*Service\(\)" space/core/store/Length of output: 79
Script:
#!/bin/bash # Search for service type declarations in store files echo "=== Service Type Declarations ===" ast-grep --pattern 'class $_ { $name: Sites$_Service; $$$ }' echo -e "\n=== Constructor Patterns ===" ast-grep --pattern 'constructor($$$) { $$$ this.$_ = $_ $$$ }' echo -e "\n=== RESTful Methods ===" rg -g "store.ts" "async (list|get|create|update|delete)" space/core/store/Length of output: 425
Script:
#!/bin/bash # First, find all store files echo "=== Store Files ===" fd "store.ts$" space/core/store/ echo -e "\n=== Service Types ===" rg -g "store.ts" "Sites.*Service" space/core/store/ echo -e "\n=== Class Declarations ===" ast-grep --pattern 'class $_ { $$$ }'Length of output: 66441
Script:
#!/bin/bash # Check the specific store file first echo "=== Label Store Content ===" cat space/core/store/label.store.ts echo -e "\n=== Service Patterns in Store Files ===" rg -g "*.store.ts" "Sites.*Service" space/core/store/Length of output: 4300
packages/services/src/project/sites-publish.service.ts (1)
13-46: Well-structured service implementation with clear separation of concernsThe service implementation is clean, well-documented, and follows TypeScript best practices. The methods are properly typed and have clear responsibilities.
packages/services/src/auth/sites-auth.service.ts (1)
13-49: Well-implemented auth service with consistent error handlingThe service implementation is clean, well-documented, and follows a consistent error handling pattern across methods.
space/core/store/members.store.ts (1)
Line range hint
21-34: Clean migration to SitesMemberServiceThe migration to the new service is clean and maintains the existing store patterns.
space/core/store/module.store.ts (1)
Line range hint
22-35: Clean migration to SitesModuleServiceThe migration to the new service is clean and maintains the existing store patterns.
space/helpers/editor.helper.ts (2)
4-4: LGTM: Service replacement aligns with refactoring goalsThe replacement of FileService with SitesFileService is consistent with the PR's objective of transitioning to the new services package.
Also applies to: 8-8
44-44: Verify error handling in file operationsWhile the implementation looks correct, ensure that error cases are properly handled by the calling components when these file operations fail.
Run this script to check error handling in components using these methods:
Also applies to: 46-46, 51-51, 53-53, 56-56
packages/types/src/module/modules.d.ts (1)
121-124: LGTM: Well-defined public typeThe
TPublicModuletype appropriately captures the essential properties needed for public consumption.packages/services/src/user/user.service.ts (1)
26-32: Verify validateStatus bypass implicationsThe
validateStatus: nulloption bypasses interceptors for unauthorized errors. Ensure this doesn't bypass important error handling in the authentication flow.Run this script to check authentication handling:
✅ Verification successful
validateStatus bypass is correctly implemented
The
validateStatus: nullusage is an intentional and documented pattern used consistently across authentication-related endpoints to handle unauthorized states gracefully. The error handling remains intact as errors are explicitly caught and thrown.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find authentication-related interceptors and handlers rg -A 5 'validateStatus.*null' ast-grep --pattern 'axios.interceptors.response.use($$$)'Length of output: 4398
space/core/types/issue.d.ts (1)
1-1: LGTM! Type safety improvementThe change from
Comment[]toTIssuePublicComment[]enhances type safety by using a more specific type for comments.Let's verify that all consumers of the IIssue interface are updated:
Also applies to: 61-61
✅ Verification successful
Type change is properly implemented and safe
The change from
Comment[]toTIssuePublicComment[]is consistently implemented across the codebase. All components, services, and type definitions are properly aligned with this change, maintaining type safety and improving code clarity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of Comment[] type rg "Comment\[\]" --type ts # Search for places where IIssue interface is used ast-grep --pattern 'interface $X extends IIssue'Length of output: 772
Script:
#!/bin/bash # Check type definitions for comment types ast-grep --pattern 'type TIssuePublicComment' ast-grep --pattern 'type TIssueActivityComment' ast-grep --pattern 'type TIssueComment' # Look for direct usage of IIssue rg "IIssue" --type ts -A 2Length of output: 68786
Script:
#!/bin/bash # Search for type definitions of comment types rg "type TIssuePublicComment =" --type ts -A 3 rg "type TIssueActivityComment =" --type ts -A 3 rg "type TIssueComment =" --type ts -A 3 # Search for interfaces/types extending or using these types rg "extends.*Comment" --type ts rg "implements.*Comment" --type tsLength of output: 1661
Script:
#!/bin/bash # Get full type definitions rg -A 15 "type TIssuePublicComment = {" --type ts rg -A 15 "type TIssueActivityComment =" --type ts rg -A 15 "type TIssueComment = {" --type ts # Check imports and usage rg "TIssuePublicComment" --type tsLength of output: 3459
space/core/components/issues/peek-overview/comment/add-comment.tsx (2)
8-9: LGTM! Service migrationThe migration from FileService to SitesFileService aligns with the PR objective.
Also applies to: 16-16
18-20: Verify form submission with new typeThe form handling has been updated to use TIssuePublicComment. Please ensure that all required fields of TIssuePublicComment are properly handled in the form submission.
Let's check the TIssuePublicComment type definition:
Also applies to: 44-46
✅ Verification successful
Form submission implementation is correct
The form only needs to collect
comment_htmlas other required fields ofTIssuePublicCommentare handled by:
- Backend API (timestamps, IDs)
- Current user session (actor details)
- Component context (workspace, project, issue details)
The usage of
Partial<TIssuePublicComment>is appropriate here.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find TIssuePublicComment type definition ast-grep --pattern 'type TIssuePublicComment = { $$$ }'Length of output: 2113
Script:
#!/bin/bash # Find form submission logic in add-comment.tsx cat space/core/components/issues/peek-overview/comment/add-comment.tsxLength of output: 3405
space/core/store/issue.store.ts (2)
2-3: LGTM! Service migrationThe migration from IssueService to SitesIssueService is consistent with the PR objective.
Also applies to: 26-26, 38-38
61-61: Verify error handling in new service methodThe method name has changed from
fetchPublicIssuestolist. Please ensure that error handling in SitesIssueService's list method matches the previous implementation.Let's check the error handling implementation:
Also applies to: 88-88
✅ Verification successful
Error handling implementation verified
The list method in SitesIssueService maintains proper error handling, throwing the error response in a consistent pattern with other service methods. The implementation is actually more comprehensive as it provides the full response object for error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find error handling in SitesIssueService ast-grep --pattern 'class SitesIssueService { $$$ list($_, $_) { $$$ } $$$ }'Length of output: 91
Script:
#!/bin/bash # Find relevant service files fd -t f "issue.*service" -I # Search for class and method implementations rg -l "class.*IssueService" --type ts # After finding files, we'll search for both method implementations rg "(fetchPublicIssues|list).*\{" --type ts -A 10Length of output: 65790
space/core/components/issues/navbar/user-avatar.tsx (1)
10-13: LGTM! Import changes align with service package refactoring.The AuthService import has been correctly updated to use the new services package.
packages/types/src/users.d.ts (1)
185-194: LGTM! Well-structured type definition.The
TPublicMembertype is well-defined with clear property names and proper TypeScript types.space/core/components/account/auth-forms/unique-code.tsx (1)
5-7: LGTM! Clean import restructuring.The import changes align with the PR objective of centralizing services in the @plane/services package.
space/core/components/account/auth-forms/auth-root.tsx (1)
6-8: LGTM! Consistent service replacement.The AuthService has been correctly replaced with SitesAuthService, maintaining the same authentication functionality while aligning with the centralized services approach.
Also applies to: 31-31
space/core/components/issues/peek-overview/comment/comment-detail-card.tsx (2)
6-9: LGTM! Clean import organization.The imports are well-organized with clear separation using comments.
21-21: LGTM! Consistent type usage.The type updates from Comment to TIssuePublicComment are applied consistently across props and method parameters.
Also applies to: 51-51
space/core/components/account/auth-forms/password.tsx (1)
6-8: LGTM! Import changes align with the service refactoring.The changes correctly update the imports to use the new services package and constants, maintaining the same functionality.
space/core/store/issue-detail.store.ts (4)
7-8: LGTM! Service imports updated correctly.The imports have been updated to use the new services package, which aligns with the refactoring objective.
33-33: Return type updated for better type safety.The
addIssueCommentmethod's return type has been updated fromPromise<Comment>toPromise<TIssuePublicComment>, improving type safety.
60-61: LGTM! Service instantiation updated correctly.The store now uses the new
SitesIssueServiceandSitesFileServiceclasses from the services package.Also applies to: 92-93
124-124: Method calls updated to match new service interfaces.The service method calls have been updated to use the new method names from the services package, maintaining the same functionality:
retrieveinstead ofgetIssueByIdlistCommentsinstead ofgetIssueCommentsaddCommentinstead ofcreateIssueComment- And so on.
The changes are consistent across all method calls.
Also applies to: 147-147, 169-169, 197-197, 199-199, 215-215, 289-289, 293-293, 325-325, 327-327, 357-357, 362-362, 379-379, 382-382, 411-411, 414-414, 432-432, 435-435
space/core/store/helpers/base-issues.store.ts (1)
10-10: LGTM! Service import and instantiation updated correctly.The changes properly integrate the new
SitesIssueServicefrom the services package:
- Import statement updated to use
@plane/services- Service instantiation updated to use
SitesIssueServiceAlso applies to: 100-100
space/package.json (1)
25-25: LGTM! Added required dependency.The
@plane/servicespackage has been added as a dependency, which is necessary for the service refactoring.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/services/src/file/sites-file.service.ts (1)
Line range hint
76-93: Add upload cancellation support to file uploads.The
uploadAssetmethod should utilize thecancelSourceto allow for upload cancellation.async uploadAsset(anchor: string, data: TFileEntityInfo, file: File): Promise<TFileSignedURLResponse> { const fileMetaData = getFileMetaDataForUpload(file); - return this.post(`/api/public/assets/v2/anchor/${anchor}/`, { + return this.post(`/api/public/assets/v2/anchor/${anchor}/`, + { ...data, ...fileMetaData, - }) + }, + { cancelToken: this.cancelSource.token }) .then(async (response) => { const signedURLResponse: TFileSignedURLResponse = response?.data; const fileUploadPayload = generateFileUploadPayload(signedURLResponse, file); - await this.fileUploadService.uploadFile(signedURLResponse.upload_data.url, fileUploadPayload); + await this.fileUploadService.uploadFile( + signedURLResponse.upload_data.url, + fileUploadPayload, + { cancelToken: this.cancelSource.token } + ); await this.updateAssetUploadStatus(anchor, signedURLResponse.asset_id); return signedURLResponse; })
♻️ Duplicate comments (2)
packages/services/src/file/sites-file.service.ts (2)
16-29:⚠️ Potential issueFix
cancelSourcetyping and initialization.The
cancelSourceproperty is still typed asanyand not properly initialized, which could lead to runtime errors.Import and use the proper type from axios:
+ import axios, { CancelTokenSource } from "axios"; export class SitesFileService extends FileService { - private cancelSource: any; + private cancelSource: CancelTokenSource; fileUploadService: FileUploadService; constructor(BASE_URL?: string) { super(BASE_URL || API_BASE_URL); + this.cancelSource = axios.CancelToken.source(); this.cancelUpload = this.cancelUpload.bind(this); // services this.fileUploadService = new FileUploadService(); }
111-116:⚠️ Potential issueFix the
cancelUploadimplementation.The method still uses the incorrect
cancelUpload()method instead ofcancel().cancelUpload() { - this.cancelSource.cancelUpload(); + this.cancelSource.cancel("Upload cancelled by the user."); }
🧹 Nitpick comments (4)
packages/types/src/issues/issue_reaction.d.ts (2)
10-13: Consider using a union type for the reaction field.Instead of using a broad
stringtype for reactions, consider defining a union type of allowed reactions for better type safety and autocomplete support.+export type TAllowedReactions = '👍' | '👎' | '❤️' | '😄' | '🎉' | '😕'; export interface IIssuePublicReaction { actor_details: IUserLite; - reaction: string; + reaction: TAllowedReactions; }
1-26: Consider standardizing naming conventions.The file uses mixed naming conventions:
- Snake case:
actor_details,reaction_id,issue_id- Camel case:
actorDetails(in type names)Consider standardizing to one convention throughout the codebase for better maintainability.
packages/types/src/issues/issue.d.ts (2)
123-123: Consider using the enum directly for better extensibility.Instead of using a union of specific enum values, consider using the enum type directly. This would make the type automatically adapt to any new service types added to the enum in the future.
-export type TIssueServiceType = EIssueServiceType.ISSUES | EIssueServiceType.EPICS; +export type TIssueServiceType = EIssueServiceType;
155-182: Improve response types consistency and maintainability.
- The
TPublicIssuesResponseis missing thetotal_resultsfield that exists inTIssuesResponse.- Consider breaking down the nested structure into smaller, reusable types.
+// Define base nested results type +type TNestedResults<T> = { + results: T[]; + total_results: number; +}; + +// Define grouped results type +type TGroupedResults<T> = { + [key: string]: { + results: T[] | { [key: string]: TNestedResults<T> }; + total_results: number; + }; +}; + type TPublicIssueResponseResults = - | IPublicIssue[] - | { - [key: string]: { - results: - | IPublicIssue[] - | { - [key: string]: { - results: IPublicIssue[]; - total_results: number; - }; - }; - total_results: number; - }; - }; + | IPublicIssue[] + | TGroupedResults<IPublicIssue>; export type TPublicIssuesResponse = { grouped_by: string; next_cursor: string; prev_cursor: string; next_page_results: boolean; prev_page_results: boolean; total_count: number; count: number; total_pages: number; extra_stats: null; results: TPublicIssueResponseResults; + total_results: number; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/services/src/file/sites-file.service.ts(5 hunks)packages/types/src/issues/issue.d.ts(2 hunks)packages/types/src/issues/issue_reaction.d.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-space
- GitHub Check: build-admin
- GitHub Check: lint-web
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/services/src/file/sites-file.service.ts (2)
1-16: LGTM! Well-structured imports and class documentation.The imports are properly organized, and the class documentation accurately reflects the inheritance hierarchy and purpose of the service.
Line range hint
94-110: LGTM! Well-implemented restore functionality.The method is well-documented and follows consistent error handling patterns.
packages/types/src/issues/issue_reaction.d.ts (1)
1-1: LGTM! Import statement is correctly defined.The import of
IUserLiteis necessary for the new interfaces and follows the correct relative path structure.packages/types/src/issues/issue.d.ts (3)
121-121: LGTM! Clear and well-defined widget type.The
TIssueDetailWidgettype effectively captures the different aspects of issue details using a union of string literals.
125-153: LGTM! Well-structured public issue interface.The
IPublicIssueinterface effectively uses TypeScript'sPickutility type to select relevant properties fromTIssuewhile adding new properties for public-specific data.
6-6: Verify the import path alias configuration.The import path
@/plane-web/typesuses an alias. Ensure that the TypeScript configuration (tsconfig.json) properly defines this path alias to prevent compilation issues.✅ Verification successful
Import path alias is correctly configured.
The import path
@/plane-web/typesis properly configured in the TypeScript configuration files and is used consistently across the codebase. The path alias maps to the community edition (ce/*) directory, which is a valid and intentional design pattern in the project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the path alias is properly configured in tsconfig fd tsconfig.json | xargs cat | jq '.compilerOptions.paths["@/plane-web/types"]'Length of output: 144
Script:
#!/bin/bash # Check for usage of this import path pattern across the codebase rg "@/plane-web/types" --type ts # Look at the content of tsconfig files echo "=== TSConfig files ===" fd tsconfig.json -x cat {} # Check for path mappings in package.json files echo "=== Package.json files ===" fd package.json -x cat {}Length of output: 25270
Description
In this PR, I have replaced all the space application (sites.plane.so) services with service package
Type of Change
References
WEB-3066
Summary by CodeRabbit
Release Notes
New Features
Improvements
@plane/services.Changes
Bug Fixes
Technical Debt