chore: git mod - migrating apis for git #37984
Conversation
WalkthroughThis pull request introduces several new asynchronous functions related to Git operations, including branch management, commit requests, and configuration updates. Additionally, it adds new TypeScript interfaces for request parameters and response structures, enhancing type safety. A new enumeration, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (25)
🚧 Files skipped from review as they are similar to previous changes (25)
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
Documentation and Community
|
github-actions
bot
added
Packages & Git Pod
brayn003
added
ok-to-test
github-actions
bot
added
Packages & Git Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Outside diff range and nitpick comments (39)
app/client/src/git/requests/toggleAutocommitRequest.ts (2)
6-12: Add JSDoc documentation for the function.The implementation looks good, but adding documentation would help explain the purpose and behavior of this toggle functionality.
Add documentation like this:
+/** + * Toggles the auto-commit feature for a specific application. + * @param baseApplicationId - The ID of the application to toggle auto-commit for + * @returns A promise that resolves to a boolean indicating the new auto-commit state + * @throws {AxiosError} When the request fails + */ export async function toggleAutocommitRequest( baseApplicationId: string, ): Promise<AxiosResponse<ToggleAutocommitResponse>> {
6-12: Consider adding specific error handling types.While the implementation is correct, consider defining specific error types for better error handling by consumers.
Add error types like this:
export interface ToggleAutocommitError { message: string; code: 'INVALID_APP_ID' | 'PERMISSION_DENIED' | 'GIT_NOT_CONNECTED'; }Then update the return type:
Promise<AxiosResponse<ToggleAutocommitResponse, ToggleAutocommitError>>app/client/src/git/requests/deleteBranchRequest.types.ts (2)
1-3: LGTM! Consider adding validation for branch name.The interface is clean and focused. Consider adding JSDoc comments to document any branch naming constraints.
export interface DeleteBranchRequestParams { + /** Name of the branch to delete. Should follow git branch naming conventions. */ branchName: string; }
5-8: Consider using more descriptive property names.While the comments clarify the purpose, consider using more descriptive property names to improve code readability.
export interface DeleteBranchResponse { - id: string; // applicationId - baseId: string; // baseApplicationId + applicationId: string; + baseApplicationId: string; }app/client/src/git/requests/deleteBranchRequest.ts (1)
9-12: Consider adding input validation.The function should validate the baseApplicationId and branch name before making the API call.
if (!baseApplicationId?.trim()) { throw new Error("baseApplicationId is required"); } if (!params.branchName?.trim()) { throw new Error("branchName is required"); }app/client/src/git/requests/fetchSSHKeyRequest.types.ts (1)
1-6: Consider clarifying or consolidating regenerated key propertiesThe interface has two similar-looking boolean properties:
isRegeneratedKeyandregeneratedKey. These might cause confusion for consumers of this API.Consider either:
- Consolidating into a single property, or
- Adding JSDoc comments to clarify the difference between these properties
export interface FetchSSHKeyResponse { publicKey: string; docUrl: string; + /** Indicates if the key was regenerated in the current request */ isRegeneratedKey: boolean; + /** Indicates if the key was previously regenerated */ regeneratedKey: boolean; }app/client/src/git/requests/updateGlobalConfigRequest.types.ts (2)
1-4: Consider adding email validation constraintThe
authorEmailfield could benefit from a string pattern constraint to ensure valid email formats.export interface UpdateGlobalConfigRequestParams { authorName: string; - authorEmail: string; + authorEmail: string & { __brand: 'Email' }; // TypeScript branded type for email validation }
6-11: Consider making response fields optionalResponse fields should typically be marked as optional to handle partial responses gracefully.
export interface UpdateGlobalConfigResponse { default: { - authorName: string; - authorEmail: string; + authorName?: string; + authorEmail?: string; }; }app/client/src/git/requests/pullRequest.ts (1)
1-4: Consider using path parameters utilityIf you have a URL construction utility, consider using it instead of template literals for better maintainability.
app/client/src/git/requests/fetchMergeStatusRequest.types.ts (2)
7-8: Consider renamingisMergeAbletoisMergeableThe property name should follow conventional camelCase spelling for better consistency.
- isMergeAble: boolean; + isMergeable: boolean;
8-8: Enhance the status property documentationConsider using a string literal type or enum to define possible status values instead of a generic string type with just a comment.
- status: string; // merge status + status: 'MERGEABLE' | 'CONFLICTS' | 'ERROR'; // Define possible merge status valuesapp/client/src/git/requests/updateProtectedBranchesRequest.types.ts (2)
1-3: Add JSDoc documentation and consider branch name validationThe interface would benefit from documentation explaining its purpose and any constraints on branch names.
+/** + * Parameters for updating protected branches in a Git repository + * @property branchNames - Array of branch names to be protected + */ export interface UpdateProtectedBranchesRequestParams { + /** Array of valid Git branch names */ branchNames: string[]; }
5-5: Add documentation for response typeThe response type should include documentation explaining the returned array contents.
+/** Array of successfully protected branch names */ export type UpdateProtectedBranchesResponse = string[];app/client/src/git/requests/fetchBranchesRequest.types.ts (2)
1-3: LGTM! Consider adding JSDoc comments.The interface is well-defined and properly exported. Adding JSDoc comments would improve developer experience.
+/** + * Parameters for fetching Git branches + * @property pruneBranches - When true, removes tracking branches that no longer exist on the remote + */ export interface FetchBranchesRequestParams { pruneBranches: boolean; }
5-11: Consider making properties readonly for immutability.The interface structure is good, but making properties readonly would prevent accidental mutations of response data.
interface SingleBranch { - branchName: string; - createdFromLocal: string; - default: boolean; + readonly branchName: string; + readonly createdFromLocal: string; + readonly default: boolean; }app/client/src/git/requests/createBranchRequest.types.ts (1)
5-8: Consider adding more descriptive JSDoc comments.While the inline comments are helpful, consider using JSDoc format for better IDE integration and documentation.
export interface CreateBranchResponse { + /** The ID of the newly created branch application */ id: string; // applicationId + /** The ID of the original application this branch was created from */ baseId: string; // baseApplicationId }app/client/src/git/requests/checkoutBranchRequest.types.ts (2)
1-3: Consider adding validation constraints for branchNameThe interface looks good, but consider adding validation constraints or a regex pattern for valid Git branch names to prevent invalid branch checkouts.
+import { IsString, Matches } from 'class-validator'; export interface CheckoutBranchRequestParams { + @IsString() + @Matches(/^[a-zA-Z0-9-_/.]+$/, { + message: 'Branch name can only contain letters, numbers, and the characters: - _ / .' + }) branchName: string; }
5-8: Enhance documentation for response interfaceThe interface is well-structured, but the comments could be more descriptive.
export interface CheckoutBranchResponse { - id: string; // applicationId - baseId: string; // baseApplicationId + id: string; // Current application ID after branch checkout + baseId: string; // Original application ID from which the branch was created }app/client/src/git/requests/fetchProtectedBranchesRequest.ts (2)
6-10: Consider URL encoding the baseApplicationId parameterThe implementation looks good, but consider encoding the baseApplicationId to handle special characters safely.
- return Api.get(`${GIT_BASE_URL}/branch/app/${baseApplicationId}/protected`); + return Api.get(`${GIT_BASE_URL}/branch/app/${encodeURIComponent(baseApplicationId)}/protected`);
6-8: Add JSDoc documentation for better API discoverabilityConsider adding JSDoc documentation to describe the function's purpose, parameters, and return type.
+/** + * Fetches the list of protected branches for a given application + * @param baseApplicationId - The ID of the application to fetch protected branches for + * @returns Promise resolving to an array of protected branch names + */ export async function fetchProtectedBranchesRequest( baseApplicationId: string, ): Promise<AxiosResponse<FetchProtectedBranches>> {app/client/src/git/requests/fetchGlobalConfigRequest.types.ts (1)
1-4: Add JSDoc documentation for better API documentation.Consider adding JSDoc documentation to describe the purpose and usage of this interface.
+/** + * Response interface for fetching global Git configuration. + * Contains author details used for Git commits. + */ export interface FetchGlobalConfigResponse { authorName: string; authorEmail: string; }app/client/src/git/requests/commitRequest.types.ts (2)
1-4: Add JSDoc documentation and consider input validation.The interface would benefit from documentation and potentially runtime validation for the commit message.
+/** + * Parameters for making a Git commit request. + * @property commitMessage - The message to be used for the commit + * @property doPush - Whether to push the commit to remote after committing + */ export interface CommitRequestParams { commitMessage: string; doPush: boolean; }
6-6: Consider a more specific response type.Using a plain string type for CommitResponse might be too generic. Consider creating a more specific type that includes commit hash, timestamp, or status.
-export type CommitResponse = string; +export interface CommitResponse { + commitHash: string; + timestamp: string; + status: 'success' | 'failure'; +}app/client/src/git/requests/triggerAutocommitRequest.types.ts (1)
3-7: Consider adding JSDoc and constraining the progress typeThe interface looks well-structured, but could benefit from documentation and type constraints.
+/** Response interface for auto-commit trigger operations */ export interface TriggerAutocommitResponse { + /** Current status of the auto-commit operation */ autoCommitResponse: AutocommitStatus; + /** Progress percentage of the operation (0-100) */ - progress: number; + progress: 0 | 25 | 50 | 75 | 100; + /** Name of the branch where auto-commit is being performed */ branchName: string; }app/client/src/git/requests/updateLocalConfigRequest.types.ts (1)
1-5: Consider adding email validation and optional fieldsThe interface could benefit from stricter email validation and optional fields.
+import { z } from "zod"; + +const emailSchema = z.string().email(); + export interface UpdateLocalConfigRequestParams { authorName: string; - authorEmail: string; + authorEmail: string & z.infer<typeof emailSchema>; useGlobalProfile: boolean; }app/client/src/git/requests/fetchGlobalConfigRequest.ts (2)
6-10: Consider implementing response cachingGlobal config is likely accessed frequently. Consider implementing a caching strategy.
+const configCache = new Map<string, FetchGlobalConfigResponse>(); +const CACHE_TTL = 5 * 60 * 1000; // 5 minutes + export async function fetchGlobalConfigRequest(): Promise< AxiosResponse<FetchGlobalConfigResponse> > { + const cachedConfig = configCache.get('default'); + if (cachedConfig) { + return Promise.resolve({ data: cachedConfig } as AxiosResponse<FetchGlobalConfigResponse>); + } return Api.get(`${GIT_BASE_URL}/profile/default`); }
1-4: Consider using a custom API clientFor better error handling and request interceptors, consider using a custom API client for Git operations.
// Suggestion: Create a GitApiClient class class GitApiClient extends Api { constructor() { super(); this.interceptors.response.use( response => response, error => { // Handle Git-specific errors return Promise.reject(error); } ); } }app/client/src/git/requests/fetchLocalConfigRequest.ts (1)
9-9: Consider adding error handlingThe request could fail due to network issues or invalid application ID.
Consider wrapping the API call in a try-catch block and handling common error scenarios:
export async function fetchLocalConfigRequest( baseApplicationId: string, ): Promise<AxiosResponse<FetchGlobalConfigResponse>> { - return Api.get(`${GIT_BASE_URL}/profile/app/${baseApplicationId}`); + try { + return await Api.get(`${GIT_BASE_URL}/profile/app/${baseApplicationId}`); + } catch (error) { + if (axios.isAxiosError(error)) { + // Handle specific error cases + throw new Error(`Failed to fetch git config: ${error.message}`); + } + throw error; + } }app/client/src/git/requests/triggerAutocommitRequest.ts (1)
6-10: Add timeout handling for auto-commit operationAuto-commit operations can be long-running and might need a timeout mechanism.
Consider adding a timeout configuration:
export async function triggerAutocommitRequest( branchedApplicationId: string, ): Promise<AxiosResponse<TriggerAutocommitResponse>> { - return Api.post(`${GIT_BASE_URL}/auto-commit/app/${branchedApplicationId}`); + return Api.post( + `${GIT_BASE_URL}/auto-commit/app/${branchedApplicationId}`, + null, + { timeout: 30000 } // 30 second timeout + ); }app/client/src/git/requests/importGitRequest.ts (1)
1-14: Consider implementing request cancellationFor long-running Git import operations, it would be helpful to support cancellation.
Consider using AbortController:
import Api from "api/Api"; import { GIT_BASE_URL } from "./constants"; import type { ImportGitRequestParams, ImportGitResponse, } from "./importGitRequest.types"; -import type { AxiosResponse } from "axios"; +import type { AxiosResponse, CancelToken } from "axios"; export async function importGitRequest( workspaceId: string, params: ImportGitRequestParams, + cancelToken?: CancelToken ): Promise<AxiosResponse<ImportGitResponse>> { - return Api.post(`${GIT_BASE_URL}/import/${workspaceId}`, params); + return Api.post(`${GIT_BASE_URL}/import/${workspaceId}`, params, { cancelToken }); }app/client/src/git/requests/connectRequest.ts (1)
9-14: Consider adding error handling for connection failuresThe implementation looks clean but should handle potential connection failures explicitly.
Consider wrapping the API call with try-catch:
export async function connectRequest( baseApplicationId: string, params: ConnectRequestParams, ): Promise<AxiosResponse<ConnectResponse>> { + try { return Api.post(`${GIT_BASE_URL}/connect/app/${baseApplicationId}`, params); + } catch (error) { + throw new Error(`Git connection failed: ${error.message}`); + } }app/client/src/git/requests/fetchStatusRequest.ts (1)
9-14: Consider enhancing error handling and timeout configurationThe implementation looks clean, but consider these improvements:
- Add specific error handling for common Git-related errors
- Configure request timeout for long-running Git operations
export async function fetchStatusRequest( branchedApplicationId: string, params: FetchStatusRequestParams, ): Promise<AxiosResponse<FetchStatusResponse>> { - return Api.get(`${GIT_BASE_URL}/status/app/${branchedApplicationId}`, params); + return Api.get( + `${GIT_BASE_URL}/status/app/${branchedApplicationId}`, + params, + { timeout: 30000 } // 30s timeout for potentially slow Git operations + ); }app/client/src/git/requests/updateLocalConfigRequest.ts (1)
9-14: Add input validation for baseApplicationIdConsider adding validation for the baseApplicationId parameter to ensure it's not empty or malformed.
export async function updateLocalConfigRequest( baseApplicationId: string, params: UpdateLocalConfigRequestParams, ): Promise<AxiosResponse<UpdateLocalConfigResponse>> { + if (!baseApplicationId?.trim()) { + throw new Error('Invalid baseApplicationId'); + } return Api.put(`${GIT_BASE_URL}/profile/app/${baseApplicationId}`, params); }app/client/src/git/requests/connectRequest.types.ts (3)
20-20: Fix typo in property nameThe property
lastCommitedAthas a typo - it should belastCommittedAt(two 't's).
1-8: Consider adding string pattern validationsThe interface could benefit from string pattern validations for
remoteUrlandauthorEmail.+type GitRemoteUrl = string & { readonly brand: unique symbol }; +type GitEmail = string & { readonly brand: unique symbol }; export interface ConnectRequestParams { - remoteUrl: string; + remoteUrl: GitRemoteUrl; gitProfile?: { authorName: string; - authorEmail: string; + authorEmail: GitEmail; useDefaultProfile?: boolean; }; }
10-24: Consider documenting the response interface fieldsThe
ConnectResponseinterface has many fields that would benefit from JSDoc comments explaining their purpose and possible values.app/client/src/git/requests/importGitRequest.types.ts (1)
20-20: Fix typo in property nameThe property
lastCommitedAthas a typo. It should belastCommittedAt.app/client/src/git/constants/enums.ts (1)
29-36: Add JSDoc documentation for AutocommitStatus enumConsider adding JSDoc documentation to describe each status and when they occur. This will help other developers understand the autocommit flow better.
+/** + * Represents the various states of an autocommit operation + * @enum {string} + */ export enum AutocommitStatus { + /** Operation is currently in progress */ IN_PROGRESS = "IN_PROGRESS", + /** Operation is locked and cannot proceed */ LOCKED = "LOCKED", + /** Changes have been successfully published */ PUBLISHED = "PUBLISHED", + /** System is idle and ready for next operation */ IDLE = "IDLE", + /** Autocommit is not required in current context */ NOT_REQUIRED = "NOT_REQUIRED", + /** Application is not Git-enabled */ NON_GIT_APP = "NON_GIT_APP", }app/client/src/git/requests/fetchStatusRequest.types.ts (1)
5-38: Consider reorganizing interface properties for better maintainabilityThe interface could benefit from logical grouping of related properties. Consider restructuring using nested interfaces:
interface GitStatusCounts { aheadCount: number; behindCount: number; modifiedDatasources: number; modifiedJSLibs: number; modifiedJSObjects: number; modifiedModuleInstances: number; modifiedModules: number; modifiedPages: number; modifiedQueries: number; } interface GitStatusChanges { added: string[]; modified: string[]; removed: string[]; conflicting: string[]; } interface ComponentChanges { datasources: GitStatusChanges; jsLibs: GitStatusChanges; jsObjects: GitStatusChanges; pages: GitStatusChanges; queries: GitStatusChanges; } export interface FetchStatusResponse { counts: GitStatusCounts; changes: ComponentChanges; remoteBranch: string; isClean: boolean; discardDocUrl: string; migrationMessage: string; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (52)
app/client/src/git/constants/enums.ts(1 hunks)app/client/src/git/requests/checkoutBranchRequest.ts(1 hunks)app/client/src/git/requests/checkoutBranchRequest.types.ts(1 hunks)app/client/src/git/requests/commitRequest.ts(1 hunks)app/client/src/git/requests/commitRequest.types.ts(1 hunks)app/client/src/git/requests/connectRequest.ts(1 hunks)app/client/src/git/requests/connectRequest.types.ts(1 hunks)app/client/src/git/requests/constants.ts(1 hunks)app/client/src/git/requests/createBranchRequest.ts(1 hunks)app/client/src/git/requests/createBranchRequest.types.ts(1 hunks)app/client/src/git/requests/deleteBranchRequest.ts(1 hunks)app/client/src/git/requests/deleteBranchRequest.types.ts(1 hunks)app/client/src/git/requests/discardRequest.ts(1 hunks)app/client/src/git/requests/disconnectRequest.ts(1 hunks)app/client/src/git/requests/disconnectRequest.types.ts(1 hunks)app/client/src/git/requests/fetchAutocommitProgressRequest.ts(1 hunks)app/client/src/git/requests/fetchAutocommitProgressRequest.types.ts(1 hunks)app/client/src/git/requests/fetchBranchesRequest.ts(1 hunks)app/client/src/git/requests/fetchBranchesRequest.types.ts(1 hunks)app/client/src/git/requests/fetchGitMetadataRequest.ts(1 hunks)app/client/src/git/requests/fetchGitMetadataRequest.types.ts(1 hunks)app/client/src/git/requests/fetchGlobalConfigRequest.ts(1 hunks)app/client/src/git/requests/fetchGlobalConfigRequest.types.ts(1 hunks)app/client/src/git/requests/fetchLocalConfigRequest.ts(1 hunks)app/client/src/git/requests/fetchLocalConfigRequest.types.ts(1 hunks)app/client/src/git/requests/fetchMergeStatusRequest.ts(1 hunks)app/client/src/git/requests/fetchMergeStatusRequest.types.ts(1 hunks)app/client/src/git/requests/fetchProtectedBranchesRequest.ts(1 hunks)app/client/src/git/requests/fetchProtectedBranchesRequest.types.ts(1 hunks)app/client/src/git/requests/fetchSSHKeyRequest.ts(1 hunks)app/client/src/git/requests/fetchSSHKeyRequest.types.ts(1 hunks)app/client/src/git/requests/fetchStatusRequest.ts(1 hunks)app/client/src/git/requests/fetchStatusRequest.types.ts(1 hunks)app/client/src/git/requests/generateSSHKeyRequest.ts(1 hunks)app/client/src/git/requests/generateSSHKeyRequest.types.ts(1 hunks)app/client/src/git/requests/importGitRequest.ts(1 hunks)app/client/src/git/requests/importGitRequest.types.ts(1 hunks)app/client/src/git/requests/mergeRequest.ts(1 hunks)app/client/src/git/requests/mergeRequest.types.ts(1 hunks)app/client/src/git/requests/pullRequest.ts(1 hunks)app/client/src/git/requests/pullRequest.types.ts(1 hunks)app/client/src/git/requests/toggleAutocommitRequest.ts(1 hunks)app/client/src/git/requests/toggleAutocommitRequest.types.ts(1 hunks)app/client/src/git/requests/triggerAutocommitRequest.ts(1 hunks)app/client/src/git/requests/triggerAutocommitRequest.types.ts(1 hunks)app/client/src/git/requests/updateGlobalConfigRequest.ts(1 hunks)app/client/src/git/requests/updateGlobalConfigRequest.types.ts(1 hunks)app/client/src/git/requests/updateLocalConfigRequest.ts(1 hunks)app/client/src/git/requests/updateLocalConfigRequest.types.ts(1 hunks)app/client/src/git/requests/updateProtectedBranchesRequest.ts(1 hunks)app/client/src/git/requests/updateProtectedBranchesRequest.types.ts(1 hunks)app/client/src/git/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- app/client/src/git/requests/constants.ts
- app/client/src/git/requests/fetchProtectedBranchesRequest.types.ts
- app/client/src/git/requests/toggleAutocommitRequest.types.ts
- app/client/src/git/requests/disconnectRequest.types.ts
- app/client/src/git/types.ts
🔇 Additional comments (15)
app/client/src/git/requests/updateGlobalConfigRequest.ts (1)
1-7: LGTM: Clean imports and good type usage
The imports are well-organized and types are properly utilized.
app/client/src/git/requests/disconnectRequest.ts (2)
1-4: LGTM! Clean imports with proper type imports
The imports are well-organized and use the 'type' keyword appropriately for TypeScript types.
9-9: Verify API endpoint pattern consistency
Let's verify that this endpoint follows the same pattern as other Git-related endpoints.
✅ Verification successful
API endpoint pattern is consistent
The endpoint /disconnect/app/${baseApplicationId} follows the same pattern as all other Git-related endpoints in the codebase:
- All endpoints use
GIT_BASE_URLas the base - All follow the pattern
/{action}/app/{applicationId} - All use template literals for consistent string interpolation (except deleteBranchRequest.ts which uses string concatenation)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent endpoint patterns in Git-related requests
echo "Checking Git API endpoint patterns..."
rg -g '*.ts' -g '*.js' "GIT_BASE_URL.*app/.*" app/client/src/git/
Length of output: 2634
app/client/src/git/requests/fetchMergeStatusRequest.ts (1)
13-16: Verify the API endpoint structure
Ensure that the API endpoint structure follows RESTful conventions and matches the backend implementation.
✅ Verification successful
Let me gather more information about the API endpoint structure.
Let me search for the base URL constant definition.
API endpoint structure is correctly implemented
The endpoint /v1/git/merge/status/app/{branchedApplicationId} follows RESTful conventions and matches the backend implementation in GitControllerCE.java. The base URL constant is properly defined and the endpoint structure is consistent across client and server.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other API endpoint definitions to verify consistency
rg -g '*.ts' -g '*.js' "GIT_BASE_URL.*merge.*status" --no-filename
Length of output: 133
Script:
#!/bin/bash
# Search for backend route definitions
rg -g '*.ts' -g '*.js' -g '*.java' "/merge/status/app" -A 3 -B 3
# Search for any other merge status related endpoints
rg -g '*.ts' -g '*.js' -g '*.java' "merge.*status" -A 3 -B 3
# Look for the constant definition
rg "GIT_BASE_URL.*=" -g '*.ts' -g '*.js'
Length of output: 71107
Script:
#!/bin/bash
# Search for GIT_BASE_URL constant definition
rg -g '*.ts' -g '*.js' "GIT_BASE_URL\s*=\s*" -A 1
# Search for any other merge status related API endpoints in the backend
rg -g '*.java' "/merge/status" -A 2 -B 2
Length of output: 908
app/client/src/git/requests/updateProtectedBranchesRequest.ts (1)
13-16: Verify error response handling in Api.post
Ensure that the Api.post utility properly handles and transforms API errors.
app/client/src/git/requests/fetchBranchesRequest.ts (1)
1-8: LGTM! Imports are well-organized.
Good use of type imports and clear organization.
app/client/src/git/requests/createBranchRequest.types.ts (1)
1-3: LGTM! Clean and focused request params interface.
The CreateBranchRequestParams interface is well-defined with a single, clear responsibility.
app/client/src/git/requests/createBranchRequest.ts (1)
9-17: Verify error handling strategy.
The implementation looks clean, but we should ensure proper error handling exists in the Api class for failed branch creation scenarios.
✅ Verification successful
Error handling is properly implemented through global interceptors
The implementation is secure with comprehensive error handling through axios interceptors:
- Global failure interceptor handles multiple error scenarios:
- HTTP 413 (Payload too large)
- Network offline errors
- Request cancellation
- Action execution errors
- Timeout errors
- Server errors (500)
- Unauthorized (401)
- Not found (404)
- Bad requests (400)
- Success interceptor validates response metadata and handles special cases for action execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Api class implements error handling
ast-grep --pattern 'class Api {
$$$
post($_) {
try {
$$$
} catch {
$$$
}
}
$$$
}'
Length of output: 115
Script:
#!/bin/bash
# Let's try a different approach to find the Api class implementation
rg -l "class Api" --type ts
# Then check for error handling patterns in the Api class
rg "class Api" -A 20 --type ts
# Also search for axios interceptors which are commonly used for global error handling
rg "interceptors" --type ts -A 10
Length of output: 25754
Script:
#!/bin/bash
# Let's check the implementation of the failure interceptor to verify error handling
rg -A 20 "apiFailureResponseInterceptor" --type ts
# Also check the success interceptor implementation
rg -A 20 "apiSuccessResponseInterceptor" --type ts
Length of output: 22968
app/client/src/git/requests/checkoutBranchRequest.ts (1)
13-16: Verify API endpoint structure
Let's ensure the API endpoint follows the REST convention and the base URL is properly configured.
✅ Verification successful
Let me check the Git API endpoint usage patterns more specifically.
API endpoint structure follows consistent pattern
The Git API endpoints follow a consistent RESTful pattern across all Git operations:
- Base URL is correctly defined as
/v1/git - Resource paths follow the pattern
/{operation}/app/{applicationId} - The checkout endpoint
/checkout-branch/app/${branchedApplicationId}aligns with this convention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent API endpoint structure across Git operations
# Search for all Git API endpoint usages
rg -l "${GIT_BASE_URL}" --type ts
# Check for consistent endpoint structure
ast-grep --pattern 'Api.get(`${GIT_BASE_URL}/$$$/app/${$_}`, $_)'
Length of output: 65987
Script:
#!/bin/bash
# Search for Git API endpoint patterns in git-related files
rg -A 2 "GIT_BASE_URL" --type ts app/client/src/git/
# Check for all Git API endpoint usages
rg -A 2 "Api\.get\(`\${GIT_BASE_URL}" --type ts
Length of output: 11797
app/client/src/git/requests/fetchProtectedBranchesRequest.ts (1)
1-10: Verify error handling implementation
Let's ensure consistent error handling for this endpoint across the codebase.
✅ Verification successful
Let me analyze the error handling implementation by checking how the request is used in components and what common error handling patterns exist.
Let me analyze the actual error handling implementation in the Api class and how errors are handled in the consuming code.
Error handling follows standard API patterns
The fetchProtectedBranchesRequest follows the established pattern where:
- It uses the centralized
Apiclass which handles HTTP requests uniformly - Error handling is managed at the application level through axios interceptors
- Errors are propagated as promises, allowing consumers to handle them appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of fetchProtectedBranchesRequest to verify error handling
rg -l "fetchProtectedBranchesRequest" | xargs rg -A 10 "fetchProtectedBranchesRequest"
# Search for similar Git API endpoints to compare error handling patterns
ast-grep --pattern 'Api.get(`${GIT_BASE_URL}$$$`)'
Length of output: 1348
Script:
#!/bin/bash
# Search for error handling patterns in components/pages using this request
rg -A 10 "fetchProtectedBranchesRequest.*then|fetchProtectedBranchesRequest.*catch"
# Search for error handling in the Api class
rg -A 10 "class Api"
# Look for common error handling utilities
fd "ApiError|ErrorHandler" --type f
Length of output: 11081
Script:
#!/bin/bash
# Check how errors are handled in the Api class implementation
rg -A 15 "class Api" app/client/src/api/Api.ts
# Look for error handling patterns in sagas or components
rg -A 10 "function\*.*fetchProtectedBranches|function.*fetchProtectedBranches"
# Check the error handler implementation
rg -A 10 "class.*ErrorHandler|function.*ErrorHandler" app/client/src/sagas/EvalErrorHandler.ts
Length of output: 1513
app/client/src/git/requests/mergeRequest.ts (1)
6-11: Implementation looks good, verify error handling
The merge request implementation is clean and well-typed. However, we should verify that the Api class properly handles merge conflicts and error scenarios.
app/client/src/git/requests/fetchGitMetadataRequest.types.ts (2)
1-15: Review required/optional field consistency
There are some inconsistencies in the optional/required fields pattern:
browserSupportedUrlis optional butbrowserSupportedRemoteUrlis required- Consider documenting the difference between these URL fields
Let's verify the API response structure:
Also, consider adding JSDoc comments to document the purpose of each field:
export interface FetchGitMetadataResponse {
+ /** Current branch name */
branchName: string;
+ /** Default branch name (e.g., main, master) */
defaultBranchName: string;
+ /** Remote repository URL */
remoteUrl: string;
repoName: string;
+ /** Browser-friendly URL for repository access */
browserSupportedUrl?: string;
isRepoPrivate?: boolean;
+ /** Browser-friendly remote URL (required for UI operations) */
browserSupportedRemoteUrl: string;
defaultApplicationId: string;
isProtectedBranch: boolean;
+ /** Configuration for automatic commit functionality */
autoCommitConfig: {
enabled: boolean;
};
isAutoDeploymentEnabled?: boolean;
}11-14: Consider expanding autoCommitConfig type
The autoCommitConfig object might need additional properties for configuration flexibility.
Let's check existing usage:
✅ Verification successful
Current autoCommitConfig implementation is sufficient
Based on the codebase analysis, the current autoCommitConfig type with just the enabled property is adequate. The type is consistently used across the codebase (in selectors, sagas, and components) only to check if autocommit is enabled/disabled, with no evidence of additional configuration needs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for autoCommitConfig usage patterns
ast-grep --pattern 'autoCommitConfig: { $$$'
Length of output: 46
Script:
#!/bin/bash
# Let's try a more comprehensive search for autoCommitConfig usage
rg -g '*.{ts,tsx,js,jsx}' 'autoCommitConfig' -B 2 -A 2
# Also search for type definitions
ast-grep --pattern 'interface $_ {
$$$
autoCommitConfig
$$$
}'
# And search for actual object usage
ast-grep --pattern 'autoCommitConfig: {
enabled: $_
}'
Length of output: 3713
app/client/src/git/requests/commitRequest.ts (1)
13-16: Verify the commit endpoint usage across the application
Let's ensure this endpoint is used consistently across the application.
✅ Verification successful
Let me check the implementation details of these files to ensure consistent usage.
The commit endpoint is used consistently across the application
The endpoint usage is consistent with proper separation of concerns:
commitRequest.tsandGitSyncAPI.tsxuse the same endpoint pattern for manual commitstriggerAutocommitRequest.tsand related code correctly use a separate auto-commit endpoint- All implementations use the same API structure and base URL configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for commit endpoint usage patterns
rg -l "commit/app/" --type ts
Length of output: 235
Script:
#!/bin/bash
# Check the implementation details of files using the commit endpoint
rg "commit/app/" -A 5 --type ts
Length of output: 1850
app/client/src/git/requests/fetchStatusRequest.types.ts (1)
1-3: LGTM! Clean and focused interface definition.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/git/requests/generateSSHKeyRequest.ts (1)
13-15: Consider standardizing the API endpoint structureThe function uses different base URLs (
GIT_BASE_URLvsAPPLICATION_BASE_URL) for importing vs generating keys. Consider standardizing under a single API namespace for better maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
app/client/src/git/actions/helpers/singleArtifactInitialState.ts(1 hunks)app/client/src/git/requests/constants.ts(1 hunks)app/client/src/git/requests/deleteBranchRequest.ts(1 hunks)app/client/src/git/requests/fetchLocalConfigRequest.ts(1 hunks)app/client/src/git/requests/fetchLocalConfigRequest.types.ts(1 hunks)app/client/src/git/requests/fetchSSHKeyRequest.ts(1 hunks)app/client/src/git/requests/fetchStatusRequest.types.ts(1 hunks)app/client/src/git/requests/generateSSHKeyRequest.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/git/actions/helpers/singleArtifactInitialState.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/src/git/requests/fetchLocalConfigRequest.types.ts
- app/client/src/git/requests/deleteBranchRequest.ts
- app/client/src/git/requests/fetchLocalConfigRequest.ts
- app/client/src/git/requests/fetchStatusRequest.types.ts
🧰 Additional context used
📓 Learnings (2)
app/client/src/git/requests/generateSSHKeyRequest.ts (2)
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/generateSSHKeyRequest.ts:9-18
Timestamp: 2024-12-05T11:06:06.127Z
Learning: In the TypeScript file `app/client/src/git/requests/generateSSHKeyRequest.ts`, the function `generateSSHKeyRequest` does not require error handling and input validation.
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/fetchSSHKeyRequest.ts:5-9
Timestamp: 2024-12-05T10:56:13.739Z
Learning: In `app/client/src/git/requests/fetchSSHKeyRequest.ts`, the `fetchSSHKeyRequest` function does not require explicit error handling for `baseApplicationId`; the function should throw an error naturally if `baseApplicationId` is missing or invalid.
app/client/src/git/requests/fetchSSHKeyRequest.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/fetchSSHKeyRequest.ts:5-9
Timestamp: 2024-12-05T10:56:13.739Z
Learning: In `app/client/src/git/requests/fetchSSHKeyRequest.ts`, the `fetchSSHKeyRequest` function does not require explicit error handling for `baseApplicationId`; the function should throw an error naturally if `baseApplicationId` is missing or invalid.
🔇 Additional comments (2)
app/client/src/git/requests/constants.ts (1)
1-2: LGTM! Well-defined API base URLs
The constants are appropriately named and follow REST API versioning conventions.
app/client/src/git/requests/generateSSHKeyRequest.ts (1)
9-18: Verify the dual-purpose endpoint implementation
The implementation looks correct but let's verify both endpoints are properly configured on the backend.
✅ Verification successful
Let me check the implementation details of these endpoints to ensure they handle the key type parameter correctly.
Both endpoints are properly implemented and handle the keyType parameter correctly
The backend implementation confirms:
/import/keysGET endpoint in GitControllerCE handles the optional keyType parameter/ssh-keypair/{branchedApplicationId}POST endpoint in ApplicationControllerCE also supports the optional keyType parameter- Both endpoints return appropriate response types (GitAuth/GitAuthDTO)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the backend implementations of both endpoints
rg -g '*.{java,kt}' '/import/keys|/ssh-keypair/'
Length of output: 502
Script:
#!/bin/bash
# Check the implementation of both endpoints
ast-grep --pattern 'GetMapping("/import/keys")
$$$
$_($$_) {
$$$
}'
ast-grep --pattern 'PostMapping("/ssh-keypair/{branchedApplicationId}")
$$$
$_($$_) {
$$$
}'
Length of output: 184
Script:
#!/bin/bash
# Let's try with ripgrep to see the implementation details
rg -g '*.{java,kt}' -B 2 -A 10 '/import/keys|/ssh-keypair/'
Length of output: 5367
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/git/components/QuickActions/index.test.tsx(1 hunks)app/client/src/git/components/QuickActions/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/git/components/QuickActions/index.tsx
🔇 Additional comments (1)
app/client/src/git/components/QuickActions/index.test.tsx (1)
Line range hint 1-250: LGTM! Test coverage is comprehensive
The test suite thoroughly covers all component behaviors including:
- Rendering states (connected/disconnected)
- Button interactions
- Loading states
- Count displays
- Protected mode behavior
| @@ -0,0 +1,8 @@ | |||
| export interface CheckoutBranchRequestParams { | |||
There was a problem hiding this comment.
We are doing one api per file so why segregate further having types.ts? wouldn't it incur more context switching?
There was a problem hiding this comment.
To avoid circular dependencies.
In case of types in the same file, if I import the response type in say the redux store file. This will happen -
redux store file <- git api file <- request lib file <- redux store file
There was a problem hiding this comment.
request lib file <- redux store file
Can you give me a reference to this part?
There was a problem hiding this comment.
Store is used in request interceptor for determining branch and environment to be included in headers
## Description - Add application apis for git package Fixes appsmithorg#37823 ## Automation /ok-to-test tags="@tag.Git" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12194637717> > Commit: e72e02f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12194637717&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Fri, 06 Dec 2024 08:23:16 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced various asynchronous functions for Git operations, including: - `checkoutBranchRequest` - `commitRequest` - `connectRequest` - `createBranchRequest` - `deleteBranchRequest` - `fetchBranchesRequest` - `fetchGitMetadataRequest` - `fetchProtectedBranchesRequest` - `mergeRequest` - `pullRequest` - `toggleAutocommitRequest` - `triggerAutocommitRequest` - `updateGlobalConfigRequest` - `updateLocalConfigRequest` - `updateProtectedBranchesRequest` - Added new enumeration `AutocommitStatus` and interfaces to enhance type safety for Git operations. - **Bug Fixes** - Updated import paths for consistency and clarity across multiple files. - **Documentation** - Enhanced type definitions for various request and response structures. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes #37823
Automation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12194637717
Commit: e72e02f
Cypress dashboard.
Tags:
@tag.GitSpec:
Fri, 06 Dec 2024 08:23:16 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
checkoutBranchRequestcommitRequestconnectRequestcreateBranchRequestdeleteBranchRequestfetchBranchesRequestfetchGitMetadataRequestfetchProtectedBranchesRequestmergeRequestpullRequesttoggleAutocommitRequesttriggerAutocommitRequestupdateGlobalConfigRequestupdateLocalConfigRequestupdateProtectedBranchesRequestAutocommitStatusand interfaces to enhance type safety for Git operations.