chore: git api - adding new apis #38681
Conversation
Code Review Summary for Git API Contracts ImplementationWalkthroughThis pull request introduces a comprehensive implementation of new Git API contracts across multiple files in the client-side application. The changes primarily focus on adding a new feature flag Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested ReviewersPoem
Finishing Touches
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
Git Product
…git-api-contracts-1
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (44)
app/client/src/git/requests/discardRequest.ts (1)
8-21: Refactor to reduce code duplicationThe functions
discardRequestNewanddiscardRequestOldshare similar logic for making API requests. Consider refactoring to eliminate duplication and enhance maintainability.app/client/src/git/requests/disconnectRequest.ts (1)
8-21: Refactor to avoid duplicationThe functions
disconnectRequestNewanddisconnectRequestOldhave similar structures. Refactoring them could improve code clarity and reduce repetition.app/client/src/git/requests/pullRequest.ts (1)
8-21: Consider refactoring to eliminate redundancyBoth
pullRequestNewandpullRequestOldfunctions perform similar operations. Refactoring could help maintain cleaner code and reduce duplication.app/client/src/git/requests/fetchMetadataRequest.ts (2)
8-21: Consider refactoring to reduce code duplicationThe functions
fetchMetadataRequestOldandfetchMetadataRequestNewshare similar logic. Refactoring them into a single function can improve maintainability.
23-33: Rename 'isNew' parameter for clarityThe parameter
isNewcould be more descriptive. Consider renaming it touseNewApiContractfor better readability.Apply this diff to update the parameter name:
export default async function fetchMetadataRequest( artifactType: GitArtifactType, baseApplicationId: string, - isNew: boolean, + useNewApiContract: boolean, ): AxiosPromise<FetchMetadataResponse> { - if (isNew) { + if (useNewApiContract) { return fetchMetadataRequestNew(artifactType, baseApplicationId); } else { return fetchMetadataRequestOld(baseApplicationId); } }app/client/src/git/requests/fetchLocalProfileRequest.ts (2)
8-21: Refactor to eliminate duplicated codeThe functions
fetchLocalProfileRequestOldandfetchLocalProfileRequestNeware very similar. Merging them could simplify the code.
23-33: Improve parameter naming for 'isNew'Rename
isNewto something more descriptive likeuseNewApiContractto enhance code clarity.Here’s the suggested change:
export default async function fetchLocalProfileRequest( artifactType: GitArtifactType, baseApplicationId: string, - isNew: boolean, + useNewApiContract: boolean, ): AxiosPromise<FetchLocalProfileResponse> { - if (isNew) { + if (useNewApiContract) { return fetchLocalProfileRequestNew(artifactType, baseApplicationId); } else { return fetchLocalProfileRequestOld(baseApplicationId); } }app/client/src/git/requests/toggleAutocommitRequest.ts (2)
8-15: Reduce redundancy by refactoringThe functions
toggleAutocommitRequestOldandtoggleAutocommitRequestNewhave overlapping functionality. Consider consolidating them.
25-35: Clarify the 'isNew' parameter nameFor better understanding, rename
isNewtouseNewApiContract.Apply this change:
export default async function toggleAutocommitRequest( artifactType: GitArtifactType, baseApplicationId: string, - isNew: boolean, + useNewApiContract: boolean, ): AxiosPromise<ToggleAutocommitResponse> { - if (isNew) { + if (useNewApiContract) { return toggleAutocommitRequestNew(artifactType, baseApplicationId); } else { return toggleAutocommitRequestOld(baseApplicationId); } }app/client/src/git/requests/connectRequest.ts (2)
29-40: RefactorconnectRequestto use feature flag internally.Consider checking the
release_git_api_contracts_enabledfeature flag within the function instead of passingisNewas a parameter. This will simplify the function signature and reduce the need to propagate the flag through multiple layers.
11-27: Eliminate code duplication betweenconnectRequestOldandconnectRequestNew.Refactor to consolidate the two functions into a single
connectRequestthat constructs the API URL based on the feature flag and parameters. This will reduce redundancy and improve maintainability.app/client/src/git/requests/commitRequest.ts (2)
32-43: SimplifycommitRequestfunction by using the feature flag internally.Consider using the
release_git_api_contracts_enabledfeature flag within the function rather than passingisNewas a parameter to streamline the function signature.
Line range hint
11-30: ConsolidatecommitRequestOldandcommitRequestNewto reduce duplication.Refactor into a single function that dynamically constructs the URL based on
artifactTypeand the feature flag, enhancing code maintainability.app/client/src/git/requests/fetchAutocommitProgressRequest.ts (2)
25-35: RefactorfetchAutocommitProgressRequestto use the feature flag internally.Instead of passing
isNewas a parameter, consider checking the feature flag within the function to simplify usage.
8-24: Avoid duplication by merging old and new request functions.Refactor to a single function that builds the API URL based on
artifactTypeand the feature flag, reducing redundancy.app/client/src/git/requests/triggerAutocommitRequest.ts (1)
8-21: Refactor to eliminate code duplicationThe functions
triggerAutocommitRequestOldandtriggerAutocommitRequestNeware similar. Consider combining them to reduce code duplication.Here's a proposed refactor:
-async function triggerAutocommitRequestOld( - branchedApplicationId: string, -): AxiosPromise<TriggerAutocommitResponse> { - return Api.post(`${GIT_BASE_URL}/auto-commit/app/${branchedApplicationId}`); -} - -async function triggerAutocommitRequestNew( - artifactType: GitArtifactType, - branchedApplicationId: string, -): AxiosPromise<TriggerAutocommitResponse> { - return Api.post( - `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${branchedApplicationId}/auto-commit`, - ); -} - -export default async function triggerAutocommitRequest( - artifactType: GitArtifactType, - branchedApplicationId: string, - isNew: boolean, -): AxiosPromise<TriggerAutocommitResponse> { - if (isNew) { - return triggerAutocommitRequestNew(artifactType, branchedApplicationId); - } else { - return triggerAutocommitRequestOld(branchedApplicationId); - } -} +export default async function triggerAutocommitRequest( + artifactType: GitArtifactType, + branchedApplicationId: string, + isNew: boolean, +): AxiosPromise<TriggerAutocommitResponse> { + const url = isNew + ? `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${branchedApplicationId}/auto-commit` + : `${GIT_BASE_URL}/auto-commit/app/${branchedApplicationId}`; + return Api.post(url); +}app/client/src/git/requests/fetchProtectedBranchesRequest.ts (1)
8-21: Refactor to eliminate code duplicationThe functions
fetchProtectedBranchesRequestOldandfetchProtectedBranchesRequestNeware similar. Consider combining them to reduce code duplication.Here's a proposed refactor:
-async function fetchProtectedBranchesRequestOld( - baseApplicationId: string, -): AxiosPromise<FetchProtectedBranchesResponse> { - return Api.get(`${GIT_BASE_URL}/branch/app/${baseApplicationId}/protected`); -} - -async function fetchProtectedBranchesRequestNew( - artifactType: GitArtifactType, - baseApplicationId: string, -): AxiosPromise<FetchProtectedBranchesResponse> { - return Api.get( - `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${baseApplicationId}/protected`, - ); -} - -export default async function fetchProtectedBranchesRequest( - artifactType: GitArtifactType, - baseApplicationId: string, - isNew: boolean, -): AxiosPromise<FetchProtectedBranchesResponse> { - if (isNew) { - return fetchProtectedBranchesRequestNew(artifactType, baseApplicationId); - } else { - return fetchProtectedBranchesRequestOld(baseApplicationId); - } -} +export default async function fetchProtectedBranchesRequest( + artifactType: GitArtifactType, + baseApplicationId: string, + isNew: boolean, +): AxiosPromise<FetchProtectedBranchesResponse> { + const url = isNew + ? `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${baseApplicationId}/protected` + : `${GIT_BASE_URL}/branch/app/${baseApplicationId}/protected`; + return Api.get(url); +}app/client/src/git/requests/mergeRequest.ts (1)
8-24: Refactor to eliminate code duplicationThe functions
mergeRequestOldandmergeRequestNeware similar. Consider combining them to reduce code duplication.Here's a proposed refactor:
-async function mergeRequestOld( - branchedApplicationId: string, - params: MergeRequestParams, -): AxiosPromise<MergeResponse> { - return Api.post(`${GIT_BASE_URL}/merge/app/${branchedApplicationId}`, params); -} - -async function mergeRequestNew( - artifactType: GitArtifactType, - branchedApplicationId: string, - params: MergeRequestParams, -): AxiosPromise<MergeResponse> { - return Api.post( - `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${branchedApplicationId}/merge`, - params, - ); -} - -export default async function mergeRequest( - artifactType: GitArtifactType, - branchedApplicationId: string, - params: MergeRequestParams, - isNew: boolean, -): AxiosPromise<MergeResponse> { - if (isNew) { - return mergeRequestNew(artifactType, branchedApplicationId, params); - } else { - return mergeRequestOld(branchedApplicationId, params); - } -} +export default async function mergeRequest( + artifactType: GitArtifactType, + branchedApplicationId: string, + params: MergeRequestParams, + isNew: boolean, +): AxiosPromise<MergeResponse> { + const url = isNew + ? `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${branchedApplicationId}/merge` + : `${GIT_BASE_URL}/merge/app/${branchedApplicationId}`; + return Api.post(url, params); +}app/client/src/git/requests/fetchStatusRequest.ts (2)
11-17: Refactor to eliminate duplication betweenfetchStatusRequestOldandfetchStatusRequestNewBoth functions perform similar operations with slight differences in URL construction. Consider combining them into a single function to improve maintainability.
Proposed refactor:
import Api from "api/Api"; import type { FetchStatusRequestParams, FetchStatusResponse, } from "./fetchStatusRequest.types"; import { GIT_BASE_URL } from "./constants"; import type { AxiosPromise } from "axios"; import urlArtifactType from "./helpers/urlArtifactType"; import type { GitArtifactType } from "git/constants/enums"; +async function fetchStatus( + branchedApplicationId: string, + params: FetchStatusRequestParams = { compareRemote: true }, + artifactType?: GitArtifactType, +): AxiosPromise<FetchStatusResponse> { + const url = artifactType + ? `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${branchedApplicationId}/status` + : `${GIT_BASE_URL}/status/app/${branchedApplicationId}`; + return Api.get(url, params); +} export default async function fetchStatusRequest( artifactType: GitArtifactType, branchedApplicationId: string, params: FetchStatusRequestParams, isNew: boolean, ): AxiosPromise<FetchStatusResponse> { - if (isNew) { - return fetchStatusRequestNew(artifactType, branchedApplicationId, params); - } else { - return fetchStatusRequestOld(branchedApplicationId, params); - } + return fetchStatus( + branchedApplicationId, + params, + isNew ? artifactType : undefined, + ); }Also applies to: 18-27
29-40: Handle optionalartifactTypeparameterWhen
isNewis false,artifactTypemay not be necessary. Consider makingartifactTypeoptional to prevent potential issues.app/client/src/git/requests/updateLocalProfileRequest.ts (2)
11-17: CombineupdateLocalProfileRequestOldandupdateLocalProfileRequestNewto reduce redundancyThe two functions have similar logic with differences only in URL construction. Refactoring can enhance code readability and maintenance.
Proposed refactor:
import type { AxiosPromise } from "axios"; import type { UpdateLocalProfileRequestParams, UpdateLocalProfileResponse, } from "./updateLocalProfileRequest.types"; import Api from "api/Api"; import { GIT_BASE_URL } from "./constants"; import urlArtifactType from "./helpers/urlArtifactType"; import type { GitArtifactType } from "git/constants/enums"; +async function updateLocalProfile( + baseApplicationId: string, + params: UpdateLocalProfileRequestParams, + artifactType?: GitArtifactType, +): AxiosPromise<UpdateLocalProfileResponse> { + const url = artifactType + ? `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${baseApplicationId}/profile` + : `${GIT_BASE_URL}/profile/app/${baseApplicationId}`; + return Api.put(url, params); +} export default async function updateLocalProfileRequest( artifactType: GitArtifactType, baseApplicationId: string, params: UpdateLocalProfileRequestParams, isNew: boolean, ): AxiosPromise<UpdateLocalProfileResponse> { - if (isNew) { - return updateLocalProfileRequestNew( - artifactType, - baseApplicationId, - params, - ); - } else { - return updateLocalProfileRequestOld(baseApplicationId, params); - } + return updateLocalProfile( + baseApplicationId, + params, + isNew ? artifactType : undefined, + ); }Also applies to: 18-27
29-44: MakeartifactTypeoptional when not requiredSince
artifactTypeisn't used whenisNewis false, consider adjusting the function signature to make it optional.app/client/src/git/requests/fetchMergeStatusRequest.ts (2)
Line range hint
11-20: Refactor to unifyfetchMergeStatusRequestOldandfetchMergeStatusRequestNewThe functions are largely similar except for the URL. Combining them can simplify the codebase.
Proposed refactor:
import type { AxiosPromise } from "axios"; import type { FetchMergeStatusRequestParams, FetchMergeStatusResponse, } from "./fetchMergeStatusRequest.types"; import Api from "api/Api"; import { GIT_BASE_URL } from "./constants"; import urlArtifactType from "./helpers/urlArtifactType"; import type { GitArtifactType } from "git/constants/enums"; +async function fetchMergeStatus( + branchedApplicationId: string, + params: FetchMergeStatusRequestParams, + artifactType?: GitArtifactType, +): AxiosPromise<FetchMergeStatusResponse> { + const url = artifactType + ? `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${branchedApplicationId}/merge/status` + : `${GIT_BASE_URL}/merge/status/app/${branchedApplicationId}`; + return Api.post(url, params); +} export default async function fetchMergeStatusRequest( artifactType: GitArtifactType, branchedApplicationId: string, params: FetchMergeStatusRequestParams, isNew: boolean, ): AxiosPromise<FetchMergeStatusResponse> { - if (isNew) { - return fetchMergeStatusRequestNew( - artifactType, - branchedApplicationId, - params, - ); - } else { - return fetchMergeStatusRequestOld(branchedApplicationId, params); - } + return fetchMergeStatus( + branchedApplicationId, + params, + isNew ? artifactType : undefined, + ); }Also applies to: 21-30
32-47: Consider makingartifactTypeoptionalTo prevent unnecessary parameter requirements when
isNewis false, adjust the function to handleartifactTypeaccordingly.app/client/src/git/requests/updateProtectedBranchesRequest.ts (1)
Line range hint
11-19: Consider renaming functions for better clarityUsing
updateProtectedBranchesRequestOldandupdateProtectedBranchesRequestNewmay lead to confusion over time. Consider renaming these functions to reflect their specific use cases or the API versions they correspond to, enhancing code readability and maintainability.Apply this diff to rename the functions:
-async function updateProtectedBranchesRequestOld( +async function updateProtectedBranchesForApp( -async function updateProtectedBranchesRequestNew( +async function updateProtectedBranchesForArtifactType(Also applies to: 21-30
app/client/src/git/types.ts (2)
1-6: Consider adding type constraints and documentation for GitRef.The
refTypefield would benefit from being a union type of allowed values. Also, consider documenting the purpose ofcreatedFromLocal.+/** Represents a Git reference with metadata */ export interface GitRef { refName: string; - refType: string; + refType: 'branch' | 'tag' | 'remote'; createdFromLocal: string; default: boolean; }
8-11: Consider adding JSDoc documentation for GitBranch.Adding documentation would help clarify the relationship between GitRef and GitBranch.
+/** Represents a Git branch with essential metadata */ export interface GitBranch { branchName: string; default: boolean; }app/client/src/git/requests/fetchRefsRequest.types.ts (1)
4-7: LGTM! Consider adding JSDoc comments.The interface design is clean and type-safe. Consider adding JSDoc comments to document the purpose of each field, especially
pruneRefs.+/** + * Parameters for fetching Git references + */ export interface FetchRefsRequestParams { + /** Whether to remove stale references */ pruneRefs: boolean; + /** Type of reference to fetch */ refType: "branch" | "tag"; }app/client/src/git/requests/checkoutRefRequest.types.ts (1)
4-8: LGTM! Consider documenting the message parameter.The interface is well-designed with proper typing. Consider adding documentation for the optional
messageparameter to clarify its purpose and when it should be provided.export interface CheckoutRefRequestParams { refType: "branch" | "tag"; refName: string; + /** Optional message to be associated with the checkout operation */ message?: string; }app/client/src/git/components/BranchList/hooks/useFilteredBranches.ts (1)
13-21: Consider memoizing the filter and map operations.The implementation is correct, but could benefit from memoization to prevent unnecessary recalculations:
+ const filteredBranches = useMemo(() => { const matched = branches.filter((b) => lowercaseSearchText ? b.branchName.toLowerCase().includes(lowercaseSearchText) : true, ); return [ ...matched.filter((b) => b.default), ...matched.filter((b) => !b.default), ].map((b) => b.branchName); + }, [branches, lowercaseSearchText]); - setFilteredBranches(branchNames); + setFilteredBranches(filteredBranches);app/client/src/git/requests/gitImportRequest.ts (1)
25-35: Consider adding a default value for the isNew parameter.The feature flag implementation looks good, but consider providing a default value for
isNewto maintain backward compatibility with existing callers.- isNew: boolean, + isNew: boolean = false,app/client/src/git/requests/fetchGlobalSSHKeyRequest.ts (2)
25-34: Consider adding a default value for the isNew parameter.For consistency with
gitImportRequest, consider providing a default value for theisNewparameter.- isNew: boolean, + isNew: boolean = false,
17-23: Remove unnecessary empty line.const url = `${GIT_BASE_URL}/artifacts/import/keys?keyType=${params.keyType}`; - return Api.get(url);app/client/src/git/requests/fetchRefsRequest.ts (1)
23-38: Consider adding return type annotation.The main function is missing its return type annotation.
export default async function fetchRefsRequest( artifactType: GitArtifactType, refArtifactId: string, params: FetchRefsRequestParams, isNew: boolean, -) { +): Promise<FetchRefsResponse> {app/client/src/git/requests/deleteRefRequest.ts (1)
23-38: Consider adding return type annotation.The main function is missing its return type annotation.
export default async function deleteRefRequest( artifactType: GitArtifactType, baseArtifactId: string, params: DeleteRefRequestParams, isNew: boolean, -) { +): Promise<DeleteRefResponse> {app/client/src/git/requests/checkoutRefRequest.ts (1)
17-20: Consider extracting common URL construction logic.The endpoint URL construction is duplicated across multiple request files. Consider extracting this into a shared utility function.
// Suggested shared utility: function constructGitEndpoint(artifactType: GitArtifactType, artifactId: string, action: string): string { return `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${artifactId}/${action}`; }app/client/src/git/sagas/mergeSaga.ts (1)
24-34: Consider adding type definition for params.While the implementation is correct, consider adding an interface for the params object to improve type safety.
interface MergeParams { sourceBranch: string; destinationBranch: string; }app/client/src/git/sagas/fetchGlobalSSHKeySaga.ts (1)
25-33: Consider enhancing error type specificity.While the implementation is correct, consider creating a specific error type for SSH key-related errors to improve error handling clarity.
interface SSHKeyError { code: string; message: string; details?: Record<string, unknown>; }app/client/src/git/sagas/fetchStatusSaga.ts (1)
19-22: Consider extracting params to a constantThe params object could be defined as a constant since
compareRemoteis always true.- const params = { compareRemote: true }; + const FETCH_STATUS_PARAMS = { compareRemote: true } as const; + const params = FETCH_STATUS_PARAMS;app/client/src/git/sagas/deleteBranchSaga.ts (1)
24-26: Consider creating a RefType enumSince
refTypeis a string literal with specific valid values, consider creating an enum or const object.const GIT_REF_TYPES = { BRANCH: "branch", TAG: "tag", } as const; type GitRefType = typeof GIT_REF_TYPES[keyof typeof GIT_REF_TYPES];app/client/src/git/sagas/connectSaga.ts (1)
Line range hint
109-124: Consider enhancing error handling specificityThe error handling could be more specific about which operation failed when the repo limit is reached.
if (GitErrorCodes.REPO_LIMIT_REACHED === error.code) { + const errorContext = "Git Connect Operation"; yield put( gitArtifactActions.toggleConnectModal({ artifactDef, open: false, }), ); yield put( gitGlobalActions.toggleRepoLimitErrorModal({ + context: errorContext, open: true, }), ); }app/client/src/git/ce/components/DefaultBranch/DefaultBranchView.tsx (1)
Line range hint
91-97: Consider memoizing the update handler.The
handleUpdatecallback could be optimized by moving the analytics call outside the callback.+ const logAnalytics = useCallback((oldBranch: string | undefined, newBranch: string) => { + AnalyticsUtil.logEvent("GS_DEFAULT_BRANCH_UPDATE", { + old_branch: oldBranch, + new_branch: newBranch, + }); + }, []); const handleUpdate = useCallback(() => { if (selectedValue) { - AnalyticsUtil.logEvent("GS_DEFAULT_BRANCH_UPDATE", { - old_branch: currentDefaultBranch, - new_branch: selectedValue, - }); + logAnalytics(currentDefaultBranch, selectedValue); updateDefaultBranch(selectedValue); } }, [currentDefaultBranch, selectedValue, updateDefaultBranch]);app/client/src/git/hooks/useBranches.ts (1)
126-126: Consider removing the type assertion.The type assertion
as GitBranch[] | nullcan be avoided by properly typing theuseMemoreturn value.- const branches = useMemo(() => { + const branches = useMemo<GitBranch[] | null>(() => { if (!Array.isArray(branchesState?.value)) { return null; } // ... rest of the implementation }, [branchesState?.value, isGitApiContractsEnabled]); - branches: branches as GitBranch[] | null, + branches,app/client/src/git/sagas/disconnectSaga.ts (1)
9-9: Consider adding migration documentation.The Git API contracts feature introduces breaking changes in request signatures. Consider adding migration documentation for downstream consumers.
Also applies to: 31-33
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
app/client/src/ce/entities/FeatureFlag.ts(2 hunks)app/client/src/git/ce/components/DefaultBranch/DefaultBranchView.tsx(2 hunks)app/client/src/git/components/BranchList/BranchListView.tsx(2 hunks)app/client/src/git/components/BranchList/hooks/useFilteredBranches.ts(1 hunks)app/client/src/git/components/GitContextProvider/index.tsx(1 hunks)app/client/src/git/components/OpsModal/TabMerge/TabMergeView.tsx(2 hunks)app/client/src/git/components/ProtectedBranches/ProtectedBranchesView.tsx(2 hunks)app/client/src/git/helpers/refToBranch.ts(1 hunks)app/client/src/git/helpers/refToBranchList.ts(1 hunks)app/client/src/git/hooks/useBranches.ts(3 hunks)app/client/src/git/hooks/useGitFeatureFlags.ts(1 hunks)app/client/src/git/requests/checkoutBranchRequest.ts(1 hunks)app/client/src/git/requests/checkoutRefRequest.ts(1 hunks)app/client/src/git/requests/checkoutRefRequest.types.ts(1 hunks)app/client/src/git/requests/commitRequest.ts(2 hunks)app/client/src/git/requests/connectRequest.ts(1 hunks)app/client/src/git/requests/createBranchRequest.ts(1 hunks)app/client/src/git/requests/createRefRequest.ts(1 hunks)app/client/src/git/requests/createRefRequest.types.ts(1 hunks)app/client/src/git/requests/deleteBranchRequest.ts(1 hunks)app/client/src/git/requests/deleteRefRequest.ts(1 hunks)app/client/src/git/requests/deleteRefRequest.types.ts(1 hunks)app/client/src/git/requests/discardRequest.ts(1 hunks)app/client/src/git/requests/discardRequest.types.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/fetchGlobalSSHKeyRequest.ts(1 hunks)app/client/src/git/requests/fetchLocalProfileRequest.ts(1 hunks)app/client/src/git/requests/fetchMergeStatusRequest.ts(2 hunks)app/client/src/git/requests/fetchMetadataRequest.ts(1 hunks)app/client/src/git/requests/fetchProtectedBranchesRequest.ts(1 hunks)app/client/src/git/requests/fetchRefsRequest.ts(1 hunks)app/client/src/git/requests/fetchRefsRequest.types.ts(1 hunks)app/client/src/git/requests/fetchStatusRequest.ts(1 hunks)app/client/src/git/requests/gitImportRequest.ts(1 hunks)app/client/src/git/requests/helpers/urlArtifactType.ts(1 hunks)app/client/src/git/requests/mergeRequest.ts(1 hunks)app/client/src/git/requests/pullRequest.ts(1 hunks)app/client/src/git/requests/toggleAutocommitRequest.ts(1 hunks)app/client/src/git/requests/triggerAutocommitRequest.ts(1 hunks)app/client/src/git/requests/updateLocalProfileRequest.ts(1 hunks)app/client/src/git/requests/updateProtectedBranchesRequest.ts(2 hunks)app/client/src/git/sagas/checkoutBranchSaga.ts(2 hunks)app/client/src/git/sagas/commitSaga.ts(2 hunks)app/client/src/git/sagas/connectSaga.ts(2 hunks)app/client/src/git/sagas/createBranchSaga.ts(1 hunks)app/client/src/git/sagas/deleteBranchSaga.ts(1 hunks)app/client/src/git/sagas/discardSaga.ts(2 hunks)app/client/src/git/sagas/disconnectSaga.ts(2 hunks)app/client/src/git/sagas/fetchBranchesSaga.ts(1 hunks)app/client/src/git/sagas/fetchGlobalSSHKeySaga.ts(2 hunks)app/client/src/git/sagas/fetchLocalProfileSaga.ts(2 hunks)app/client/src/git/sagas/fetchMergeStatusSaga.ts(2 hunks)app/client/src/git/sagas/fetchMetadataSaga.ts(1 hunks)app/client/src/git/sagas/fetchProtectedBranchesSaga.ts(2 hunks)app/client/src/git/sagas/fetchStatusSaga.ts(2 hunks)app/client/src/git/sagas/gitImportSaga.ts(2 hunks)app/client/src/git/sagas/mergeSaga.ts(2 hunks)app/client/src/git/sagas/pullSaga.ts(2 hunks)app/client/src/git/sagas/toggleAutocommitSaga.ts(2 hunks)app/client/src/git/sagas/triggerAutocommitSaga.ts(3 hunks)app/client/src/git/sagas/updateLocalProfileSaga.ts(2 hunks)app/client/src/git/sagas/updateProtectedBranchesSaga.ts(2 hunks)app/client/src/git/store/actions/checkoutBranchActions.ts(1 hunks)app/client/src/git/store/actions/createBranchActions.ts(1 hunks)app/client/src/git/store/actions/deleteBranchActions.ts(1 hunks)app/client/src/git/store/actions/fetchBranchesActions.ts(2 hunks)app/client/src/git/store/selectors/gitFeatureFlagSelectors.ts(1 hunks)app/client/src/git/store/types.ts(4 hunks)app/client/src/git/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- app/client/src/git/requests/deleteBranchRequest.ts
- app/client/src/git/requests/createBranchRequest.ts
- app/client/src/git/requests/checkoutBranchRequest.ts
🧰 Additional context used
📓 Learnings (8)
app/client/src/git/ce/components/DefaultBranch/DefaultBranchView.tsx (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#38155
File: app/client/src/git/components/ProtectedBranches/index.tsx:18-27
Timestamp: 2024-12-13T22:08:46.336Z
Learning: The `ProtectedBranchesView` component in `app/client/src/git/components/ProtectedBranches/ProtectedBranchesView.tsx` already has a TypeScript interface for its props.
app/client/src/git/sagas/fetchLocalProfileSaga.ts (2)
Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:28-34
Timestamp: 2024-12-10T10:52:38.244Z
Learning: In `app/client/src/git/sagas/fetchLocalProfileSaga.ts`, error handling is managed outside the scope, so casting errors directly to strings is acceptable.
Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:8-13
Timestamp: 2024-12-10T10:52:38.873Z
Learning: In `app/client/src/git/sagas/fetchLocalProfileSaga.ts` and similar Git sagas, error handling for `baseArtifactId` is managed outside the scope, so validation checks for `baseArtifactId` within the saga functions are unnecessary.
app/client/src/git/store/actions/checkoutBranchActions.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#37830
File: app/client/packages/git/src/actions/checkoutBranchActions.ts:11-18
Timestamp: 2024-11-29T05:38:54.262Z
Learning: In the Git Redux actions (`app/client/packages/git/src/actions`), the `createSingleArtifactAction` function already handles loading state and error management, so additional checks in action creators are unnecessary.
app/client/src/git/sagas/updateLocalProfileSaga.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/updateLocalProfileSaga.ts:35-40
Timestamp: 2024-12-10T10:52:51.127Z
Learning: In `app/client/src/git/sagas/updateLocalProfileSaga.ts`, it's acceptable to cast `error` to `string` when passing it to `gitArtifactActions.updateLocalProfileError`.
app/client/src/git/sagas/checkoutBranchSaga.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#38634
File: app/client/src/git/sagas/initGitSaga.ts:38-39
Timestamp: 2025-01-13T18:31:44.457Z
Learning: In Git sagas where operations are primarily Redux `put` effects, error handling with try-catch blocks is unnecessary as these effects don't throw errors.
app/client/src/git/requests/fetchLocalProfileRequest.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:8-13
Timestamp: 2024-12-10T10:52:38.873Z
Learning: In `app/client/src/git/sagas/fetchLocalProfileSaga.ts` and similar Git sagas, error handling for `baseArtifactId` is managed outside the scope, so validation checks for `baseArtifactId` within the saga functions are unnecessary.
app/client/src/git/components/ProtectedBranches/ProtectedBranchesView.tsx (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#38155
File: app/client/src/git/components/ProtectedBranches/index.tsx:18-27
Timestamp: 2024-12-13T22:08:46.336Z
Learning: The `ProtectedBranchesView` component in `app/client/src/git/components/ProtectedBranches/ProtectedBranchesView.tsx` already has a TypeScript interface for its props.
app/client/src/git/requests/fetchAutocommitProgressRequest.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/fetchAutocommitProgressRequest.types.ts:3-7
Timestamp: 2024-12-05T11:02:12.715Z
Learning: When reviewing TypeScript interfaces for API responses in `app/client/src/git/requests/`, note that even if interfaces appear identical, they may be intentionally kept separate because the API responses might change independently in the future.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
🔇 Additional comments (62)
app/client/src/git/requests/discardRequest.ts (1)
23-33:⚠️ Potential issueEnsure parameter consistency when calling 'discardRequestOld'
In the
discardRequestfunction,discardRequestOldis called withrefArtifactId, but it expects abranchedApplicationId. Please verify thatrefArtifactIdcorrectly maps tobranchedApplicationIdto prevent any unexpected behavior.app/client/src/git/requests/disconnectRequest.ts (1)
23-33:⚠️ Potential issueVerify parameter mapping in 'disconnectRequestOld' call
In the
disconnectRequestfunction,disconnectRequestOldis called withbaseArtifactId, but it expects abaseApplicationId. Please ensure thatbaseArtifactIdaligns withbaseApplicationIdto avoid any issues.app/client/src/git/requests/connectRequest.ts (3)
8-9: Imports added for artifact handling support.New imports for
urlArtifactTypeandGitArtifactTypeare correctly included to facilitate artifact-based URL construction.
11-16:connectRequestOldfunction maintains backward compatibility.The function is correctly preserved to handle existing API contracts.
18-27:connectRequestNewfunction accurately implements the new API endpoint.The function correctly constructs the new API URL using
artifactTypeandbaseArtifactId.app/client/src/git/requests/commitRequest.ts (3)
8-9: Imports for artifact handling are appropriately added.The addition of
GitArtifactTypeandurlArtifactTypeimports supports the new functionality.
Line range hint
11-19:commitRequestOldfunction correctly preserved.Maintains existing behavior for backward compatibility.
21-30:commitRequestNewfunction accurately handles the new API contract.The API call is correctly constructed with the updated URL format using
artifactTypeandrefArtifactId.app/client/src/git/requests/fetchAutocommitProgressRequest.ts (3)
5-6: Imports for artifact handling are correctly added.The imports of
urlArtifactTypeandGitArtifactTypesupport the new request structure.
8-15:fetchAutocommitProgressRequestOldfunction maintained appropriately.Ensures existing functionality remains unaffected.
16-24:fetchAutocommitProgressRequestNewfunction accurately implements the new endpoint.The API call is correctly constructed using the updated URL format with
artifactTypeandbaseArtifactId.app/client/src/git/requests/updateProtectedBranchesRequest.ts (1)
32-47: Implementation logic is soundThe conditional selection between the old and new request functions based on
isNewis correctly implemented and aligns with the feature flag strategy.app/client/src/git/sagas/checkoutBranchSaga.ts (1)
33-47: Feature flag integration is correctly appliedThe usage of
selectGitApiContractsEnabledto conditionally adjust the API request aligns with the intended feature flag implementation. The parameters and API call are appropriately modified based on the flag.app/client/src/git/helpers/refToBranch.ts (1)
3-8: Function implementation is correctThe
refToBranchfunction accurately maps aGitRefobject to aGitBranch, ensuring consistency across the application.app/client/src/git/helpers/refToBranchList.ts (1)
4-6: Clean implementation!The function is well-typed, pure, and follows functional programming principles.
app/client/src/git/requests/discardRequest.types.ts (1)
4-6: Type definitions look good!Clean separation of response data type from API response wrapper.
app/client/src/git/requests/disconnectRequest.types.ts (1)
2-4: Clean type restructuring!Good simplification using type alias instead of empty interface extension.
app/client/src/git/requests/deleteRefRequest.types.ts (1)
4-7: LGTM! Clean and consistent implementation.The interface properly uses the union type for
refTypeand maintains consistency with other Git request types.app/client/src/git/store/selectors/gitFeatureFlagSelectors.ts (1)
5-9: Well-structured selector implementation!Good use of
createSelectorfor memoization and nullish coalescing for the default value.app/client/src/git/hooks/useGitFeatureFlags.ts (1)
11-13: Clean integration of the new feature flag!Follows the established pattern consistently.
Also applies to: 18-18
app/client/src/git/store/actions/createBranchActions.ts (1)
4-7: LGTM! Interface simplification improves type safety.The explicit property definitions in
CreateBranchInitPayloadmake the contract clearer and more maintainable.app/client/src/git/store/actions/deleteBranchActions.ts (1)
4-7: LGTM! Consistent with createBranchActions.ts changes.The interface modification follows the same pattern, maintaining consistency across branch-related actions.
app/client/src/git/requests/gitImportRequest.ts (1)
16-23: LGTM! Clean implementation of the new API endpoint.The new implementation properly handles workspace ID as a query parameter, aligning with REST best practices.
app/client/src/git/requests/fetchGlobalSSHKeyRequest.ts (1)
9-34: LGTM! Consistent implementation with gitImportRequest.The implementation follows the same pattern as
gitImportRequest, maintaining consistency across the codebase.app/client/src/git/store/actions/checkoutBranchActions.ts (1)
4-7: LGTM! Clean interface definition.The simplified interface with direct property definitions improves type clarity.
app/client/src/git/store/actions/fetchBranchesActions.ts (2)
5-8: LGTM! Clear interface definition with optional pruning.The optional
pruneBranchesparameter provides good flexibility for branch management.
18-20: Verify the impact of response type change.The switch to
FetchRefsResponseDatamight affect consumers of this action.✅ Verification successful
Type change appears to be safe
The switch to
FetchRefsResponseDatais properly typed throughout the codebase. All consumers access the data through typed selectors and store interfaces, which will enforce type safety.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of branches response data rg -t typescript "branches\.value" app/client/srcLength of output: 86
Script:
#!/bin/bash # Search for action usage rg "fetchBranchesSuccessAction" app/client/src # Search for type usage rg "FetchRefsResponseData" app/client/src # Search for branches-related patterns rg "\.branches" app/client/src # Look for reducer handling ast-grep --pattern 'case $action.type === "GIT_FETCH_BRANCHES_SUCCESS":'Length of output: 2509
app/client/src/git/requests/createRefRequest.ts (2)
12-21: LGTM! Clean implementation of the new API endpoint.The new function correctly constructs the endpoint URL and maintains type safety with proper TypeScript annotations.
23-38: Verify error handling for the old API path.While the implementation looks correct, ensure that error responses from
createBranchRequestOldare compatible with the expected response type of the new API.app/client/src/git/sagas/toggleAutocommitSaga.ts (1)
18-27: LGTM! Clean feature flag integration.The feature flag selector is correctly implemented and the request parameters are properly updated to support the new API contracts.
app/client/src/git/sagas/fetchMetadataSaga.ts (1)
16-25: LGTM! Comprehensive error handling with proper validation.The implementation includes proper response validation, error logging, and Sentry integration while maintaining backward compatibility through the feature flag.
app/client/src/git/sagas/fetchLocalProfileSaga.ts (2)
5-5: LGTM! Clean import organization.The imports are well-organized, separating Redux-Saga effects from feature flag selectors.
Also applies to: 9-9
18-27: LGTM! Feature flag integration looks good.The feature flag implementation follows Redux-Saga patterns correctly and maintains type safety with explicit boolean typing.
app/client/src/git/sagas/fetchProtectedBranchesSaga.ts (1)
18-26: LGTM! Clean feature flag integration.The feature flag implementation is consistent with the established pattern and maintains proper type safety.
app/client/src/git/sagas/fetchBranchesSaga.ts (3)
19-20: Type change looks goodThe transition from
FetchBranchesResponsetoFetchRefsResponsealigns with the new API contracts.
22-25: Parameter transformation is well-handledGood use of optional chaining and default value for
pruneRefs. The refType is correctly set to "branch".
31-37: Verify request parameters orderThe order of parameters in the request call is correct, but let's ensure all callers follow this same pattern.
✅ Verification successful
Parameter order is consistent across all usages
The implementation confirms that the parameter order follows a consistent pattern across all related functions and their calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all fetchRefsRequest calls to verify parameter order consistency ast-grep --pattern 'fetchRefsRequest($_, $_, $_, $_)'Length of output: 758
Script:
#!/bin/bash # Search for function definition and imports rg "fetchRefsRequest" -t ts # Also try to find the function with potential type annotations ast-grep --pattern 'function fetchRefsRequest'Length of output: 981
Script:
#!/bin/bash # Get the content of the fetchRefsRequest implementation cat app/client/src/git/requests/fetchRefsRequest.tsLength of output: 1079
app/client/src/git/sagas/fetchMergeStatusSaga.ts (2)
27-29: Feature flag implementation is consistentThe implementation follows the established pattern seen in other sagas.
31-37: Request parameters are properly orderedThe parameters for
fetchMergeStatusRequestare correctly ordered: artifactType, artifactId, params, and feature flag.app/client/src/git/sagas/updateLocalProfileSaga.ts (2)
28-30: Feature flag selector implementation is correctThe selector implementation follows the established pattern.
33-37: Request parameters are well-structuredGood use of artifactType and baseArtifactId from artifactDef.
app/client/src/git/sagas/updateProtectedBranchesSaga.ts (2)
26-28: Feature flag implementation is consistentThe implementation follows the established pattern seen in other sagas.
31-35: Request parameters are properly orderedThe parameters for
updateProtectedBranchesRequestfollow the consistent pattern: artifactType, baseArtifactId, params, and feature flag.app/client/src/git/sagas/discardSaga.ts (2)
23-25: LGTM: Feature flag selector usage is correctThe type annotation and selector usage follow TypeScript best practices.
27-32: Verify error handling for feature flag statesWhile the implementation looks correct, ensure that the
discardRequesthandles both enabled and disabled feature flag states appropriately.✅ Verification successful
Feature flag handling verified and correct
The implementation properly handles both enabled and disabled states through separate implementations, with appropriate error handling in the saga layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if discardRequest has different handling for feature flag states ast-grep --pattern 'function discardRequest($_: $_, $_: $_, isGitApiContractsEnabled: boolean) { $$$ }'Length of output: 107
Script:
#!/bin/bash # Find discardRequest function definition and usage rg "discardRequest" -A 10 # Also search for the function declaration with potential exports rg "export.*(function|const) discardRequest" -A 10Length of output: 5044
app/client/src/git/sagas/fetchStatusSaga.ts (1)
24-30: LGTM: Request parameters are well-structuredThe request call is properly organized with all required parameters.
app/client/src/git/sagas/deleteBranchSaga.ts (1)
29-38: LGTM: Feature flag and request parameters are properly handledThe integration follows the established pattern and maintains type safety.
app/client/src/git/components/GitContextProvider/index.tsx (1)
34-34: LGTM! Type safety improvementThe change from
keyof typeof GitArtifactType | nulltoGitArtifactType | nullprovides better type safety by directly using the enum value.app/client/src/git/sagas/commitSaga.ts (1)
34-44: LGTM! Clean feature flag integrationThe feature flag integration follows the established pattern and maintains proper error handling.
app/client/src/git/sagas/gitImportSaga.ts (1)
30-39: LGTM! Consistent feature flag implementationThe feature flag integration follows the same pattern as other sagas and maintains proper error handling.
app/client/src/git/sagas/connectSaga.ts (1)
40-50: LGTM! Consistent feature flag implementationThe feature flag integration follows the established pattern across sagas.
app/client/src/git/ce/components/DefaultBranch/DefaultBranchView.tsx (1)
14-14: LGTM! Type definitions are well-structured.The change from
FetchBranchesResponseDatatoGitBranch[]improves type safety and provides better type inference for branch-related operations.Also applies to: 48-48
app/client/src/git/hooks/useBranches.ts (1)
27-40: Well-structured feature flag integration!The conditional transformation of branches based on the feature flag provides a clean migration path.
app/client/src/git/store/types.ts (1)
48-48: Clean type definitions update!The changes improve type safety by:
- Using
FetchRefsResponseDatafor branches state- Directly using
GitArtifactTypeinstead ofkeyof typeof GitArtifactTypeAlso applies to: 71-71, 88-88
app/client/src/ce/entities/FeatureFlag.ts (1)
52-52: LGTM! Feature flag follows naming convention.The new feature flag
release_git_api_contracts_enabledis correctly defined and initialized with a safe default value offalse.Also applies to: 102-102
app/client/src/git/components/ProtectedBranches/ProtectedBranchesView.tsx (1)
19-19: LGTM! Type update maintains consistency.The change from
FetchBranchesResponseDatatoGitBranch[]aligns with the new type system while preserving the component's functionality.Also applies to: 53-53
app/client/src/git/components/OpsModal/TabMerge/TabMergeView.tsx (1)
37-37: LGTM! Type update preserves merge functionality.The migration to
GitBranch[]type maintains the integrity of branch handling in merge operations.Also applies to: 67-67
app/client/src/git/components/BranchList/BranchListView.tsx (1)
37-37: LGTM! Type update maintains branch list functionality.The switch to
GitBranch[]type preserves the branch listing, filtering, and management capabilities.Also applies to: 232-232
app/client/src/git/sagas/pullSaga.ts (2)
25-27: LGTM! Clean feature flag integration.The feature flag selector integration follows Redux-Saga patterns correctly with proper type safety.
29-34: Verify pullRequest implementation compatibility.The pullRequest call signature has changed to include artifactType and isGitApiContractsEnabled. Ensure the pullRequest implementation handles both feature-flag-enabled and disabled cases.
✅ Verification successful
Implementation correctly handles feature flag cases
The pullRequest implementation properly handles both feature-flag enabled and disabled scenarios through separate implementations, maintaining consistent return types and proper type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for pullRequest implementation to verify feature flag handling ast-grep --pattern 'function pullRequest($_) { $$$ }'Length of output: 57
Script:
#!/bin/bash # First locate files containing pullRequest rg "pullRequest" -l # Then search for different function declaration patterns ast-grep --pattern 'const pullRequest = $_' ast-grep --pattern 'export const pullRequest = $_' ast-grep --pattern 'export function pullRequest($_)' ast-grep --pattern 'export async function pullRequest($_)'Length of output: 76848
Script:
#!/bin/bash # View the implementation with context rg -A 10 -B 10 "pullRequest" "app/client/src/git/requests/pullRequest.ts"Length of output: 1076
app/client/src/git/sagas/disconnectSaga.ts (2)
25-28: LGTM! Consistent feature flag implementation.The feature flag integration matches the pattern used in other Git sagas.
30-33: Verify disconnectRequest backward compatibility.The disconnectRequest signature has changed significantly. Based on the AI summary, there are new disconnectRequestOld and disconnectRequestNew implementations.
✅ Verification successful
Backward compatibility is properly implemented.
The disconnectRequest implementation correctly handles both old and new API versions through separate implementations and clean routing logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the presence and implementation of both request handlers ast-grep --pattern 'function disconnectRequestOld($_) { $$$ }' ast-grep --pattern 'function disconnectRequestNew($_) { $$$ }'Length of output: 133
Script:
#!/bin/bash # Search for disconnect-related functions and their implementations rg -g '*.ts' -g '*.js' "disconnect.*\s*=?\s*(async\s+)?function" -A 5 # Search for disconnectRequest usage and definition rg -g '*.ts' -g '*.js' "disconnectRequest" -A 5 # Look for any version or compatibility handling related to disconnect rg -g '*.ts' -g '*.js' "(disconnect.*Old|disconnect.*New|disconnect.*Version)" -A 5Length of output: 130669
Script:
#!/bin/bash # Search for disconnectRequest implementation rg -g '*.ts' -g '*.js' "function\s+disconnectRequest" -A 10 # Search for any version-related checks around disconnect functionality rg -g '*.ts' -g '*.js' "isGitApiContractsEnabled.*disconnect" -A 5Length of output: 2105
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (20)
app/client/src/git/requests/fetchLocalProfileRequest.ts (2)
8-12: Add deprecation documentation.Consider adding a JSDoc comment to indicate this function is for legacy support and will be deprecated in future releases.
+/** + * @deprecated Use fetchLocalProfileRequestNew instead. + * This function is maintained for backward compatibility and will be removed in a future release. + */ async function fetchLocalProfileRequestOld(
23-33: Consider enhancing parameter naming and error handling.Two suggestions for improvement:
- The
isNewparameter name could be more descriptive, e.g.,useNewApiContract- Consider adding error handling for invalid artifact types
export default async function fetchLocalProfileRequest( artifactType: GitArtifactType, baseArtifactId: string, - isNew: boolean, + useNewApiContract: boolean, ): AxiosPromise<FetchLocalProfileResponse> { + if (!baseArtifactId) { + throw new Error('baseArtifactId is required'); + } - if (isNew) { + if (useNewApiContract) { return fetchLocalProfileRequestNew(artifactType, baseArtifactId); } else { return fetchLocalProfileRequestOld(baseArtifactId);app/client/src/git/requests/mergeRequest.ts (3)
8-13: Consider updating parameter name in function body.While the parameter was renamed from
branchedApplicationIdtorefArtifactId, the URL still uses/app/prefix, suggesting this function is app-specific. Consider updating the URL construction to reflect the generic nature of the parameter name.- return Api.post(`${GIT_BASE_URL}/merge/app/${branchedApplicationId}`, params); + return Api.post(`${GIT_BASE_URL}/merge/app/${refArtifactId}`, params);
15-24: Consider adding artifactType validation.The function assumes valid
artifactTypevalues. Consider adding validation or type guards to handle invalid artifact types gracefully.async function mergeRequestNew( artifactType: GitArtifactType, refArtifactId: string, params: MergeRequestParams, ): AxiosPromise<MergeResponse> { + if (!Object.values(GitArtifactType).includes(artifactType)) { + throw new Error(`Invalid artifact type: ${artifactType}`); + } return Api.post( `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${refArtifactId}/merge`, params, ); }
26-37: Consider using an enum for version selection.Using a boolean flag for routing between implementations might be less maintainable as more versions are added. Consider using an enum to make the versioning more explicit and documented.
enum MergeRequestVersion { LEGACY = 'LEGACY', NEW = 'NEW' } export default async function mergeRequest( artifactType: GitArtifactType, refArtifactId: string, params: MergeRequestParams, version: MergeRequestVersion = MergeRequestVersion.LEGACY, ): AxiosPromise<MergeResponse> { switch (version) { case MergeRequestVersion.NEW: return mergeRequestNew(artifactType, refArtifactId, params); default: return mergeRequestOld(refArtifactId, params); } }app/client/src/git/requests/generateSSHKeyRequest.ts (1)
8-15: Add JSDoc comments for better documentation.Consider adding JSDoc comments to document the function parameters and return type, especially since this is part of a public API contract.
+/** + * Generates an SSH key pair for the specified artifact + * @param baseArtifactId - The ID of the base artifact + * @param params - Parameters for SSH key generation + * @returns Promise resolving to the generated SSH key response + */ export default async function generateSSHKeyRequest( baseArtifactId: string, params: GenerateSSHKeyRequestParams, ): AxiosPromise<GenerateSSHKeyResponse> {app/client/src/git/requests/triggerAutocommitRequest.ts (2)
14-21: Consider adding explicit error handling.While the implementation is clean, consider adding explicit error handling for invalid artifact types or failed requests.
async function triggerAutocommitRequestNew( artifactType: GitArtifactType, refArtifactId: string, ): AxiosPromise<TriggerAutocommitResponse> { + if (!refArtifactId) { + throw new Error('refArtifactId is required'); + } return Api.post( `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${refArtifactId}/auto-commit`, ); }
23-33: Consider using an enum instead of boolean flag.The
isNewboolean flag could be replaced with an enum to make the code more maintainable and extensible for future API versions.enum ApiVersion { V1 = 'V1', V2 = 'V2' } export default async function triggerAutocommitRequest( artifactType: GitArtifactType, refArtifactId: string, apiVersion: ApiVersion = ApiVersion.V1 ): AxiosPromise<TriggerAutocommitResponse> { return apiVersion === ApiVersion.V2 ? triggerAutocommitRequestNew(artifactType, refArtifactId) : triggerAutocommitRequestOld(refArtifactId); }app/client/src/git/requests/fetchStatusRequest.ts (2)
18-27: Consider adding parameter defaults and error handling.The new implementation could benefit from:
- Default parameters for consistency with the old implementation
- Error handling for invalid artifact types
async function fetchStatusRequestNew( artifactType: GitArtifactType, baseArtifactId: string, - params: FetchStatusRequestParams, + params: FetchStatusRequestParams = { compareRemote: true }, ): AxiosPromise<FetchStatusResponse> { + if (!artifactType || !baseArtifactId) { + throw new Error("Invalid artifact type or ID"); + } return Api.get( `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${baseArtifactId}/status`, params, ); }
29-40: Add JSDoc and consider deprecation strategy.The router function would benefit from:
- Documentation explaining the
isNewparameter's purpose- Deprecation notice for the old implementation path
+/** + * Fetches git status for an artifact + * @param artifactType - Type of git artifact + * @param baseArtifactId - ID of the base artifact + * @param params - Status request parameters + * @param isNew - Flag to use new API contract (true) or legacy implementation (false) + * @deprecated The legacy implementation (isNew=false) will be removed in future versions + */ export default async function fetchStatusRequest( artifactType: GitArtifactType, baseArtifactId: string, params: FetchStatusRequestParams, isNew: boolean, ): AxiosPromise<FetchStatusResponse> {app/client/src/git/requests/toggleAutocommitRequest.ts (2)
25-29: Consider using an enum instead of a boolean flag.The
isNewboolean parameter could be replaced with an enum to make the code more maintainable and extensible for future API versions.+export enum ApiVersion { + V1 = 'V1', + V2 = 'V2' +} -export default async function toggleAutocommitRequest( - artifactType: GitArtifactType, - baseArtifactId: string, - isNew: boolean, +export default async function toggleAutocommitRequest( + artifactType: GitArtifactType, + baseArtifactId: string, + apiVersion: ApiVersion = ApiVersion.V1,
30-34: Consider adding error handling for invalid parameters.The function should validate input parameters and handle potential errors gracefully.
export default async function toggleAutocommitRequest( artifactType: GitArtifactType, baseArtifactId: string, isNew: boolean, ): AxiosPromise<ToggleAutocommitResponse> { + if (!baseArtifactId) { + throw new Error('baseArtifactId is required'); + } + if (isNew) { + if (!artifactType) { + throw new Error('artifactType is required for new API'); + } return toggleAutocommitRequestNew(artifactType, baseArtifactId); } else { return toggleAutocommitRequestOld(baseArtifactId); }app/client/src/git/requests/updateLocalProfileRequest.ts (1)
29-40: Consider using feature flag instead of boolean parameter.Instead of passing an explicit
isNewboolean, consider using therelease_git_api_contracts_enabledfeature flag mentioned in the PR objectives.-export default async function updateLocalProfileRequest( - artifactType: GitArtifactType, - baseArtifactId: string, - params: UpdateLocalProfileRequestParams, - isNew: boolean, -): AxiosPromise<UpdateLocalProfileResponse> { - if (isNew) { +import { getFeatureFlags } from "selectors/featureFlags"; + +export default async function updateLocalProfileRequest( + artifactType: GitArtifactType, + baseArtifactId: string, + params: UpdateLocalProfileRequestParams, +): AxiosPromise<UpdateLocalProfileResponse> { + if (getFeatureFlags().release_git_api_contracts_enabled) {app/client/src/git/requests/fetchProtectedBranchesRequest.ts (2)
14-21: Consider adding error handling for invalid artifact types.The implementation looks clean, but we should consider adding validation or error handling for invalid artifact types before making the API call.
async function fetchProtectedBranchesRequestNew( artifactType: GitArtifactType, baseArtifactId: string, ): AxiosPromise<FetchProtectedBranchesResponse> { + if (!baseArtifactId) { + throw new Error("baseArtifactId is required"); + } return Api.get( `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${baseArtifactId}/protected`, ); }
23-33: Consider a default value for isNew parameter.The implementation is clean and follows good practices. Consider providing a default value for the
isNewparameter to make the function more flexible in usage.export default async function fetchProtectedBranchesRequest( artifactType: GitArtifactType, baseArtifactId: string, - isNew: boolean, + isNew: boolean = false, ): AxiosPromise<FetchProtectedBranchesResponse> {app/client/src/git/requests/pullRequest.ts (2)
14-21: Consider explicit error handling.The implementation is clean, but consider adding explicit error handling for invalid artifact types.
async function pullRequestNew( artifactType: GitArtifactType, refArtifactId: string, ): AxiosPromise<PullResponse> { + if (!artifactType || !refArtifactId) { + throw new Error('Invalid artifact type or reference ID'); + } return Api.get( `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${refArtifactId}/pull`, ); }
23-33: Consider using enum for API version control.While the boolean flag works, using an enum for API versions would be more maintainable and extensible for future API versions.
+export enum GitApiVersion { + V1 = 'V1', + V2 = 'V2', +} export default async function pullRequest( artifactType: GitArtifactType, refArtifactId: string, - isNew: boolean, + apiVersion: GitApiVersion = GitApiVersion.V1, ): AxiosPromise<PullResponse> { - if (isNew) { + if (apiVersion === GitApiVersion.V2) { return pullRequestNew(artifactType, refArtifactId); } else { return pullRequestOld(refArtifactId); } }app/client/src/git/requests/fetchMetadataRequest.ts (2)
14-21: Consider URL encoding the baseArtifactId.While the implementation is solid, consider URL encoding the baseArtifactId to handle special characters safely.
- `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${baseArtifactId}/metadata`, + `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${encodeURIComponent(baseArtifactId)}/metadata`,
23-33: Add JSDoc to document the feature flag usage.The implementation looks good. Consider adding documentation to clarify the feature flag's purpose and when to use each implementation.
+/** + * Fetches Git metadata using either the new or old implementation based on the feature flag. + * @param artifactType - The type of Git artifact (new implementation only) + * @param baseArtifactId - The ID of the base artifact + * @param isNew - Feature flag to toggle between implementations + */ export default async function fetchMetadataRequest(app/client/src/git/requests/updateProtectedBranchesRequest.ts (1)
32-47: Add JSDoc documentation for the exported function.While the implementation is clean, adding JSDoc documentation would help clarify:
- The purpose of the
isNewparameter- When to use each mode
- Expected values for
artifactType+/** + * Updates protected branches for a git artifact + * @param artifactType - Type of the git artifact + * @param baseArtifactId - ID of the base artifact + * @param params - Protected branches configuration + * @param isNew - Flag to toggle between new and old API contracts + * @returns Promise with the update response + */ export default async function updateProtectedBranchesRequest(
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/client/src/git/requests/fetchLocalProfileRequest.ts(1 hunks)app/client/src/git/requests/fetchMergeStatusRequest.ts(2 hunks)app/client/src/git/requests/fetchMetadataRequest.ts(1 hunks)app/client/src/git/requests/fetchProtectedBranchesRequest.ts(1 hunks)app/client/src/git/requests/fetchSSHKeyRequest.ts(1 hunks)app/client/src/git/requests/fetchStatusRequest.ts(1 hunks)app/client/src/git/requests/generateSSHKeyRequest.ts(1 hunks)app/client/src/git/requests/mergeRequest.ts(1 hunks)app/client/src/git/requests/pullRequest.ts(1 hunks)app/client/src/git/requests/toggleAutocommitRequest.ts(1 hunks)app/client/src/git/requests/triggerAutocommitRequest.ts(1 hunks)app/client/src/git/requests/updateLocalProfileRequest.ts(1 hunks)app/client/src/git/requests/updateProtectedBranchesRequest.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/git/requests/fetchMergeStatusRequest.ts
🧰 Additional context used
📓 Learnings (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
🔇 Additional comments (23)
app/client/src/git/requests/fetchSSHKeyRequest.ts (2)
7-7: LGTM! Parameter rename aligns with artifact-based architecture.The change from
baseApplicationIdtobaseArtifactIdmakes the function more generic and reusable.
9-9: LGTM! API endpoint usage is correct.The endpoint construction properly uses the new parameter while maintaining the existing API structure.
app/client/src/git/requests/fetchLocalProfileRequest.ts (2)
5-6: LGTM! Clean import additions.The new imports are well-structured and properly typed.
14-21: Well-structured new implementation!Clean implementation with proper type safety and good separation of concerns using the URL helper.
app/client/src/git/requests/mergeRequest.ts (1)
5-6: LGTM! Clean type imports.The new imports are well-structured and follow TypeScript best practices.
app/client/src/git/requests/triggerAutocommitRequest.ts (2)
5-6: LGTM! Clean import additions.The new imports are well-structured and properly typed.
8-12: Verify parameter renaming impact.The parameter rename from
branchedApplicationIdtorefArtifactIdsuggests a more generic approach. Ensure this change is reflected in the documentation and doesn't break existing calls.app/client/src/git/requests/fetchStatusRequest.ts (2)
8-9: LGTM! Clean import additions.
11-16: Verify usage of old implementation across the codebase.The rename to
fetchStatusRequestOldis clean, maintaining backward compatibility.✅ Verification successful
Function rename is properly contained within the module
The renamed function
fetchStatusRequestOldis only used within its own module, following a clean refactoring pattern. No external dependencies are affected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any direct usage of the old function name that might need updating rg "fetchStatusRequestOld" --type tsLength of output: 232
app/client/src/git/requests/toggleAutocommitRequest.ts (3)
5-6: LGTM! Clean import additions.The new imports for GitArtifactType and urlArtifactType are well-organized and support the new API contract structure.
8-14: LGTM! Clean refactor of existing functionality.Good practice maintaining backward compatibility by preserving the old implementation with a clear naming convention.
16-23: Verify URL construction across all artifact types.The new implementation looks solid. However, let's verify the URL construction for all possible artifact types.
✅ Verification successful
URL construction verified and correct
The
urlArtifactTypefunction correctly handles all possibleGitArtifactTypevalues with proper REST endpoint mappings.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all possible GitArtifactType values and urlArtifactType usage echo "Searching for GitArtifactType enum definition..." rg -A 10 "export enum GitArtifactType" echo "Searching for urlArtifactType function implementation..." rg -A 10 "function urlArtifactType"Length of output: 1755
app/client/src/git/requests/updateLocalProfileRequest.ts (3)
8-9: LGTM! Clean import additions.The new imports are well-organized and properly typed.
11-16: LGTM! Good backward compatibility preservation.Clean extraction of the original implementation into a separate function.
18-27: Verify URL pattern consistency across Git APIs.The new implementation looks good, using the artifact-type-based URL pattern.
Let's verify the URL pattern consistency across other Git APIs:
✅ Verification successful
URL patterns are consistent across Git profile APIs ✓
The new artifact-type-based URL pattern is consistently implemented across local profile operations, while maintaining backward compatibility. Global profile endpoints correctly remain unchanged.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar URL patterns in other Git API files rg -l "${GIT_BASE_URL}.*profile" --type tsLength of output: 2302
Script:
#!/bin/bash # Show content of all Git profile-related request files rg -l "profile" app/client/src/git/requests/ --type ts | xargs catLength of output: 3286
app/client/src/git/requests/fetchProtectedBranchesRequest.ts (2)
5-6: LGTM! Clean import additions.The new imports are well-typed and necessary for the enhanced functionality.
8-12: Verify existing usages of the API endpoint.The old function is correctly preserved for backward compatibility. However, let's verify all existing usages to ensure they continue to work with the refactored structure.
✅ Verification successful
No issues found with the API endpoint usage
The old function is properly preserved, and all existing endpoint usages are consistent with the refactored structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing API endpoint usages rg -l "branch/app/.+/protected" --type tsLength of output: 242
Script:
#!/bin/bash # Check the context of API endpoint usage in each file echo "=== updateProtectedBranchesRequest.ts ===" rg -A 5 -B 5 "branch/app/.+/protected" app/client/src/git/requests/updateProtectedBranchesRequest.ts echo -e "\n=== GitSyncAPI.tsx ===" rg -A 5 -B 5 "branch/app/.+/protected" app/client/src/api/GitSyncAPI.tsx echo -e "\n=== GitSync.ts ===" rg -A 5 -B 5 "branch/app/.+/protected" app/client/cypress/support/Pages/GitSync.tsLength of output: 1652
app/client/src/git/requests/pullRequest.ts (1)
5-6: LGTM! Clean import additions.The new imports are well-organized and necessary for the artifact type handling functionality.
app/client/src/git/requests/fetchMetadataRequest.ts (2)
5-6: LGTM! Clean import additions.
8-12: Verify existing usages of the old endpoint.The old implementation is correctly preserved. Let's verify all existing usages to ensure they continue to work with the new structure.
app/client/src/git/requests/updateProtectedBranchesRequest.ts (3)
8-9: LGTM! Clean import additions.
Line range hint
11-19: Good approach maintaining backward compatibility.The old implementation is preserved for legacy support while clearly marked with the 'Old' suffix.
21-30: Verify the urlArtifactType helper implementation.The implementation looks clean and type-safe. Let's verify the helper function implementation to ensure correct URL construction.
✅ Verification successful
The urlArtifactType helper implementation is correct and type-safe.
The helper properly maps GitArtifactType enum values to their corresponding URL segments, following RESTful conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for urlArtifactType implementation and its tests rg -t typescript "urlArtifactType" --glob "!node_modules"Length of output: 92
Script:
#!/bin/bash # Search for urlArtifactType implementation without type filter rg "urlArtifactType" # Find all TypeScript files in git-related directories fd -e ts -p "git" # Search for GitArtifactType enum or type definition rg "GitArtifactType"Length of output: 70110
Script:
#!/bin/bash # Find urlArtifactType function implementation ast-grep --pattern 'export default function urlArtifactType($$$) { $$$ }' # Find GitArtifactType definition ast-grep --pattern 'type GitArtifactType = $$$' ast-grep --pattern 'enum GitArtifactType { $$$ }'Length of output: 76772
Script:
#!/bin/bash # Get the helper implementation cat app/client/src/git/requests/helpers/urlArtifactType.ts # Find GitArtifactType definition with context rg -A 5 "export (type|enum) GitArtifactType"Length of output: 803
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
app/client/src/git/requests/checkoutRefRequest.ts (3)
12-21: Consider encoding path parameters for URL safety.While the implementation is solid, consider encoding
refArtifactidwhen constructing the URL to prevent potential injection vulnerabilities.- `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${refArtifactid}/checkout-ref`, + `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${encodeURIComponent(refArtifactid)}/checkout-ref`,
23-28: Add JSDoc documentation for the feature flag parameter.The
isNewparameter's purpose isn't immediately clear. Consider adding documentation to explain its significance and expected values.+/** + * Checks out a Git reference using either the new or legacy implementation + * @param artifactType - Type of the Git artifact + * @param refArtifactid - ID of the reference to checkout + * @param params - Checkout parameters + * @param isNew - When true, uses new implementation; when false, falls back to legacy + */ export default async function checkoutRefRequest(
29-37: Add parameter validation for robustness.The function assumes all parameters are valid. Consider adding validation to handle edge cases gracefully.
export default async function checkoutRefRequest( artifactType: GitArtifactType, refArtifactid: string, params: CheckoutRefRequestParams, isNew: boolean, ) { + if (!refArtifactid || !params?.refName) { + throw new Error("Invalid checkout parameters provided"); + } if (isNew) {app/client/src/git/sagas/createBranchSaga.ts (1)
24-27: Consider using an enum for refType.While the implementation works, using a string literal for
refTypecould be error-prone. Consider defining an enum or constant for supported ref types.+export enum GitRefType { + BRANCH = "branch", + TAG = "tag" +} const params: CreateRefRequestParams = { - refType: "branch", + refType: GitRefType.BRANCH, refName: action.payload.branchName, };app/client/src/git/requests/createRefRequest.ts (2)
12-21: Consider adding input validation.While the function implementation is clean, consider adding validation for
refArtifactIdandparamsto ensure they meet the required format before making the API call.async function createRefRequestNew( artifactType: GitArtifactType, refArtifactId: string, params: CreateRefRequestParams, ): AxiosPromise<CreateRefResponse> { + if (!refArtifactId?.trim()) { + throw new Error('refArtifactId is required'); + } + if (!params?.refName?.trim()) { + throw new Error('refName is required in params'); + } return Api.post( `${GIT_BASE_URL}/${urlArtifactType(artifactType)}/${refArtifactId}/create-ref`, params, ); }
23-38: Add JSDoc and improve type safety.The feature toggle implementation is clean, but could benefit from documentation and type assertions.
+/** + * Creates a Git reference (branch/tag) using either the new or legacy API + * @param artifactType - Type of Git artifact + * @param refArtifactId - ID of the artifact + * @param params - Parameters for reference creation + * @param isNew - Feature flag to toggle between new and old implementation + * @returns Promise with the created reference + */ export default async function createRefRequest( artifactType: GitArtifactType, refArtifactId: string, params: CreateRefRequestParams, isNew: boolean, ) { if (isNew) { return createRefRequestNew(artifactType, refArtifactId, params); } else { const createBranchParams = { branchName: params.refName, } as const; // Type assertion for immutability return createBranchRequestOld(refArtifactId, createBranchParams); } }app/client/src/git/sagas/commitSaga.ts (1)
34-36: Consider adding a comment explaining the feature flag's purpose.A brief comment explaining when and why
isGitApiContractsEnabledaffects the commit behavior would improve maintainability.+ // Controls whether to use new Git API contracts for commit operations const isGitApiContractsEnabled: boolean = yield select( selectGitApiContractsEnabled, );app/client/src/git/sagas/triggerAutocommitSaga.ts (1)
101-107: Consider implementing exponential backoff for polling.While the changes look good, consider implementing exponential backoff instead of fixed delay to reduce server load during longer operations.
const getBackoffDelay = (attempt: number) => Math.min(1000 * Math.pow(2, attempt), 30000);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/src/git/requests/checkoutRefRequest.ts(1 hunks)app/client/src/git/requests/createRefRequest.ts(1 hunks)app/client/src/git/requests/createRefRequest.types.ts(1 hunks)app/client/src/git/sagas/commitSaga.ts(2 hunks)app/client/src/git/sagas/connectSaga.ts(2 hunks)app/client/src/git/sagas/createBranchSaga.ts(1 hunks)app/client/src/git/sagas/triggerAutocommitSaga.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/git/sagas/connectSaga.ts
- app/client/src/git/requests/createRefRequest.types.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
🔇 Additional comments (13)
app/client/src/git/requests/checkoutRefRequest.ts (1)
1-10: Clean and well-organized imports!The imports are properly structured with clear separation of types and runtime dependencies.
app/client/src/git/sagas/createBranchSaga.ts (5)
10-15: LGTM! Clean import restructuring.The imports have been properly updated to support the new ref-based API approach.
21-21: LGTM! Type definition updated correctly.The response type has been properly updated to align with the new ref-based API structure.
29-31: Fix inconsistent variable naming.The variable
isGitApiContractsEnableddiffers from other sagas which useisGitApiContractsEnabled(plural).
33-39: LGTM! API call properly structured.The createRefRequest call is well-structured with all necessary parameters, including the feature flag.
43-49: LGTM! Success flow properly restored.The fetchBranchesInit dispatch has been properly restored, ensuring immediate branch list updates after creation.
app/client/src/git/requests/createRefRequest.ts (2)
1-11: LGTM! Clean imports and type declarations.The imports are well-organized and types are properly defined.
27-27: Verify feature flag integration.The
isNewparameter should be driven by therelease_git_api_contracts_enabledfeature flag mentioned in the PR objectives.✅ Verification successful
Feature flag integration verified.
The
release_git_api_contracts_enabledfeature flag is properly integrated into the codebase through the feature flag system, and theisNewparameter aligns with this implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for feature flag usage rg -l "release_git_api_contracts_enabled.*isNew"Length of output: 50
Script:
#!/bin/bash # Search for feature flag in all variations rg "release_git_api_contracts_enabled" # Search for isNew parameter usage in TypeScript files ast-grep --pattern 'isNew: $_' # Search for feature flags configuration fd "feature" --type f --exec rg -l "release_git_api_contracts"Length of output: 111822
app/client/src/git/sagas/commitSaga.ts (2)
20-20: LGTM! Clean import addition.
34-36: LGTM! Clean feature flag integration.The feature flag check follows Redux-Saga patterns with proper type safety.
app/client/src/git/sagas/triggerAutocommitSaga.ts (3)
30-30: LGTM! Clean import addition for the feature flag selector.
60-62: Good refactor! Feature flag selection is now correctly lifted to a single location, addressing the previous review feedback.
65-70: Verify consistent parameter usage across Git API calls.Let's ensure the new parameter is consistently used across all Git API calls.
✅ Verification successful
Parameter usage is correctly implemented
The
triggerAutocommitRequestfunction is properly implemented with the new parameter and correctly used in the saga. The changes maintain backward compatibility through the old/new implementation pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all triggerAutocommitRequest calls to verify consistent parameter usage ast-grep --pattern 'triggerAutocommitRequest($_, $_, $_)'Length of output: 762
Script:
#!/bin/bash # Find the function definition ast-grep --pattern 'function triggerAutocommitRequest' # Search for any imports or references to this function rg "triggerAutocommitRequest" --type ts -A 2 -B 2Length of output: 3702
| @@ -0,0 +1,8 @@ | |||
| import type { GitBranch, GitRef } from "git/types"; | |||
|
|
|||
| export default function refToBranch(ref: GitRef): GitBranch { | |||
There was a problem hiding this comment.
I didn't quite get the necessity of this helper function?
| @@ -0,0 +1,12 @@ | |||
| import { GitArtifactType } from "git/constants/enums"; | |||
|
|
|||
| export default function urlArtifactType(artifactType: GitArtifactType): string { | |||
There was a problem hiding this comment.
Can't this be an object?
Also can we simplify this by modifying the values of GitArtifactType to applications, packages etc and use GitArtifactType directly everywhere rather than having another function / object to rely on?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (20)
app/client/src/git/requests/fetchMergeStatusRequest.ts (1)
31-42: Consider adding default value for isNew parameter.The dispatcher logic is correct, but the
isNewparameter could benefit from a default value to maintain backward compatibility without requiring explicit boolean passing.export default async function fetchMergeStatusRequest( artifactType: GitArtifactType, refArtifactId: string, params: FetchMergeStatusRequestParams, - isNew: boolean, + isNew: boolean = false, ): AxiosPromise<FetchMergeStatusResponse> {app/client/src/git/requests/fetchLocalProfileRequest.ts (2)
7-11: Add deprecation notice.Consider adding a JSDoc comment to indicate this function is deprecated in favor of the new implementation.
+/** + * @deprecated Use fetchLocalProfileRequestNew instead + */ async function fetchLocalProfileRequestOld(
20-30: Consider improving the feature flag parameter.Two suggestions for better maintainability:
- Rename
isNewto something more descriptive likeuseNewApiContract- Consider using an enum instead of boolean for better extensibility if more variants are expected in the future
- isNew: boolean, + useNewApiContract: boolean, // or consider using an enumapp/client/src/git/requests/fetchStatusRequest.ts (3)
10-15: Consider aligning default params between old and new implementations.The old implementation has a default value for
paramswhile the new one doesn't. This inconsistency might cause confusion.-async function fetchStatusRequestOld( - branchedApplicationId: string, - params: FetchStatusRequestParams = { compareRemote: true }, +async function fetchStatusRequestOld( + branchedApplicationId: string, + params: FetchStatusRequestParams,
17-26: Consider adding error handling for invalid artifact types.While the type system prevents invalid
artifactTypevalues at compile time, runtime validation might be beneficial for debugging and maintainability.async function fetchStatusRequestNew( artifactType: GitArtifactType, baseArtifactId: string, params: FetchStatusRequestParams, ): AxiosPromise<FetchStatusResponse> { + if (!Object.values(GitArtifactType).includes(artifactType)) { + throw new Error(`Invalid artifact type: ${artifactType}`); + } return Api.get( `${GIT_BASE_URL}/${artifactType}/${baseArtifactId}/status`, params, ); }
28-39: Consider using an enum instead of a boolean for API version control.Using a boolean flag for version control might become limiting if more versions are added in the future. Consider using an enum to make the code more maintainable.
+export enum GitApiVersion { + V1 = 'V1', + V2 = 'V2', +} export default async function fetchStatusRequest( artifactType: GitArtifactType, baseArtifactId: string, params: FetchStatusRequestParams, - isNew: boolean, + version: GitApiVersion = GitApiVersion.V1, ): AxiosPromise<FetchStatusResponse> { - if (isNew) { + if (version === GitApiVersion.V2) { return fetchStatusRequestNew(artifactType, baseArtifactId, params); } else { return fetchStatusRequestOld(baseArtifactId, params); } }app/client/src/git/requests/discardRequest.ts (1)
20-30: Consider using feature flag instead of boolean parameter.Since this PR introduces a feature flag
release_git_api_contracts_enabled, consider using it instead of theisNewparameter for better maintainability.+import { getFeatureFlags } from "selectors/featureFlagsSelectors"; + export default async function discardRequest( artifactType: GitArtifactType, refArtifactId: string, - isNew: boolean, ) { - if (isNew) { + if (getFeatureFlags().release_git_api_contracts_enabled) { return discardRequestNew(artifactType, refArtifactId); } else { return discardRequestOld(refArtifactId); } }app/client/src/git/requests/connectRequest.ts (1)
28-39: Add return type annotation to connectRequest functionThe main exported function is missing its return type annotation.
-export default async function connectRequest( +export default async function connectRequest( artifactType: GitArtifactType, baseArtifactId: string, params: ConnectRequestParams, isNew: boolean, -) { +): AxiosPromise<ConnectResponse> {app/client/src/git/requests/fetchAutocommitProgressRequest.ts (1)
24-34: Add return type annotation to fetchAutocommitProgressRequest functionThe main exported function is missing its return type annotation.
-export default async function fetchAutocommitProgressRequest( +export default async function fetchAutocommitProgressRequest( artifactType: GitArtifactType, baseArtifactId: string, isNew: boolean, -) { +): AxiosPromise<FetchAutocommitProgressResponse> {app/client/src/git/requests/checkoutRefRequest.ts (1)
22-37: Add return type annotation to checkoutRefRequest functionThe main exported function is missing its return type annotation.
-export default async function checkoutRefRequest( +export default async function checkoutRefRequest( artifactType: GitArtifactType, refArtifactid: string, params: CheckoutRefRequestParams, isNew: boolean, -) { +): AxiosPromise<CheckoutRefResponse> {app/client/src/git/requests/updateProtectedBranchesRequest.ts (1)
31-46: Consider adding default value for isNew parameter.The implementation looks good, but consider providing a default value for the isNew parameter based on the feature flag.
- isNew: boolean, + isNew: boolean = false,This would maintain backward compatibility while making the transition smoother.
app/client/src/git/requests/mergeRequest.ts (1)
25-36: Consider enhancing the dispatcher implementation.Two suggestions for improvement:
- Consider using the
release_git_api_contracts_enabledfeature flag instead of passingisNewexplicitly- Add validation for
artifactTypeto prevent runtime errorsExample implementation:
export default async function mergeRequest( artifactType: GitArtifactType, refArtifactId: string, params: MergeRequestParams, ): AxiosPromise<MergeResponse> { if (FeatureFlags.release_git_api_contracts_enabled) { if (!Object.values(GitArtifactType).includes(artifactType)) { throw new Error(`Invalid artifact type: ${artifactType}`); } return mergeRequestNew(artifactType, refArtifactId, params); } return mergeRequestOld(refArtifactId, params); }app/client/src/git/requests/triggerAutocommitRequest.ts (2)
13-20: Consider adding artifact type validation.The function assumes valid
artifactTypevalues. Consider adding validation or type guards.async function triggerAutocommitRequestNew( artifactType: GitArtifactType, refArtifactId: string, ): AxiosPromise<TriggerAutocommitResponse> { + if (!Object.values(GitArtifactType).includes(artifactType)) { + throw new Error(`Invalid artifact type: ${artifactType}`); + } return Api.post( `${GIT_BASE_URL}/${artifactType}/${refArtifactId}/auto-commit`, ); }
22-32: Add JSDoc documentation for better API clarity.The exported function would benefit from JSDoc documentation explaining the parameters and their impact on behavior.
+/** + * Triggers an auto-commit request for Git artifacts + * @param artifactType - Type of Git artifact + * @param refArtifactId - Reference ID of the artifact + * @param isNew - Flag to use new API contract when true + * @returns Promise with auto-commit response + */ export default async function triggerAutocommitRequest( artifactType: GitArtifactType, refArtifactId: string, isNew: boolean, ): AxiosPromise<TriggerAutocommitResponse> {app/client/src/git/requests/toggleAutocommitRequest.ts (1)
24-34: Consider deriving isNew from feature flag.Instead of passing isNew as a parameter, consider deriving it from the
release_git_api_contracts_enabledfeature flag. This would centralize the control of which implementation to use.-export default async function toggleAutocommitRequest( - artifactType: GitArtifactType, - baseArtifactId: string, - isNew: boolean, -): AxiosPromise<ToggleAutocommitResponse> { - if (isNew) { +export default async function toggleAutocommitRequest( + artifactType: GitArtifactType, + baseArtifactId: string, +): AxiosPromise<ToggleAutocommitResponse> { + if (featureFlags.release_git_api_contracts_enabled) {app/client/src/git/requests/commitRequest.ts (1)
20-29: Consider adding explicit error handling.While Api.post likely handles errors, consider adding explicit error handling for invalid artifactType values to provide more meaningful error messages.
async function commitRequestNew( artifactType: GitArtifactType, refArtifactId: string, params: CommitRequestParams, ): AxiosPromise<CommitResponse> { + if (!artifactType || !refArtifactId) { + throw new Error('Invalid artifactType or refArtifactId provided'); + } return Api.post( `${GIT_BASE_URL}/${artifactType}/${refArtifactId}/commit`, params, ); }app/client/src/git/requests/fetchProtectedBranchesRequest.ts (1)
20-30: Consider making isNew parameter required.The implementation looks good, but consider making
isNewa required parameter to prevent accidental omission. This would ensure explicit choice between old and new implementations.- isNew: boolean, + isNew: boolean = false,app/client/src/git/requests/updateLocalProfileRequest.ts (1)
28-39: Consider a more descriptive name for theisNewparameter.The parameter could better reflect its purpose, such as
useNewApiContractorisNewGitApi.- isNew: boolean, + useNewApiContract: boolean,And update the usage:
- if (isNew) { + if (useNewApiContract) {app/client/src/git/requests/disconnectRequest.ts (2)
7-11: Consider adding deprecation notice.Since this is the old implementation being preserved for backward compatibility, consider adding a JSDoc deprecation notice with migration guidelines.
+/** + * @deprecated Use the new disconnect request with artifactType parameter instead. + */ async function disconnectRequestOld(
22-32: Enhance type safety for the routing parameter.Consider using a union type or enum instead of a boolean flag for better type safety and future extensibility.
+type DisconnectRequestVersion = 'legacy' | 'v2'; export default async function disconnectRequest( artifactType: GitArtifactType, baseArtifactId: string, - isNew: boolean, + version: DisconnectRequestVersion = 'legacy', ) { - if (isNew) { + if (version === 'v2') { return disconnectRequestNew(artifactType, baseArtifactId); } else { return disconnectRequestOld(baseArtifactId);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
app/client/src/git/constants/enums.ts(1 hunks)app/client/src/git/helpers/refToBranchList.ts(1 hunks)app/client/src/git/requests/checkoutRefRequest.ts(1 hunks)app/client/src/git/requests/commitRequest.ts(2 hunks)app/client/src/git/requests/connectRequest.ts(1 hunks)app/client/src/git/requests/createRefRequest.ts(1 hunks)app/client/src/git/requests/deleteRefRequest.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/fetchAutocommitProgressRequest.ts(1 hunks)app/client/src/git/requests/fetchLocalProfileRequest.ts(1 hunks)app/client/src/git/requests/fetchMergeStatusRequest.ts(2 hunks)app/client/src/git/requests/fetchMetadataRequest.ts(1 hunks)app/client/src/git/requests/fetchProtectedBranchesRequest.ts(1 hunks)app/client/src/git/requests/fetchStatusRequest.ts(1 hunks)app/client/src/git/requests/mergeRequest.ts(1 hunks)app/client/src/git/requests/pullRequest.ts(1 hunks)app/client/src/git/requests/toggleAutocommitRequest.ts(1 hunks)app/client/src/git/requests/triggerAutocommitRequest.ts(1 hunks)app/client/src/git/requests/updateLocalProfileRequest.ts(1 hunks)app/client/src/git/requests/updateProtectedBranchesRequest.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/client/src/git/helpers/refToBranchList.ts
- app/client/src/git/requests/fetchMetadataRequest.ts
- app/client/src/git/requests/createRefRequest.ts
- app/client/src/git/requests/deleteRefRequest.ts
- app/client/src/git/requests/pullRequest.ts
🧰 Additional context used
📓 Learnings (3)
app/client/src/git/requests/fetchAutocommitProgressRequest.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/fetchAutocommitProgressRequest.types.ts:3-7
Timestamp: 2024-12-05T11:02:12.715Z
Learning: When reviewing TypeScript interfaces for API responses in `app/client/src/git/requests/`, note that even if interfaces appear identical, they may be intentionally kept separate because the API responses might change independently in the future.
app/client/src/git/requests/fetchMergeStatusRequest.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#37984
File: app/client/src/git/requests/fetchMergeStatusRequest.ts:9-17
Timestamp: 2024-12-05T10:58:55.922Z
Learning: Error handling and input validation are not required in request functions like `fetchMergeStatusRequest` in `app/client/src/git/requests`.
app/client/src/git/requests/updateLocalProfileRequest.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/requests/updateLocalProfileRequest.ts:9-14
Timestamp: 2024-12-10T10:53:01.591Z
Learning: In `app/client/src/git/requests/updateLocalProfileRequest.ts`, input validation for `baseApplicationId` is not required because TypeScript ensures type safety.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (33)
app/client/src/git/requests/fetchMergeStatusRequest.ts (3)
8-8: LGTM!The import of GitArtifactType is correctly added to support the new API contract.
Line range hint
10-18: LGTM!The original function is correctly preserved and renamed to maintain backward compatibility.
20-29: LGTM!The new function implementation follows the new API contract pattern correctly.
app/client/src/git/requests/fetchLocalProfileRequest.ts (2)
5-5: LGTM! Import added for new type.
13-18: LGTM! Well-structured new implementation.The new endpoint follows RESTful conventions and provides more flexibility with the artifactType parameter.
app/client/src/git/requests/fetchStatusRequest.ts (1)
8-8: LGTM! Clean type import.app/client/src/git/requests/discardRequest.ts (2)
7-11: LGTM! Clear function rename to indicate legacy implementation.The renaming to
discardRequestOldclearly indicates this is the legacy implementation.
13-18: LGTM! Well-structured new API implementation.The new implementation follows RESTful conventions with a clear URL structure.
app/client/src/git/requests/connectRequest.ts (1)
17-26: LGTM: Clean implementation of new API endpointThe new endpoint follows RESTful conventions and maintains type safety.
app/client/src/git/requests/fetchAutocommitProgressRequest.ts (1)
15-22: LGTM: Consistent with API response type separation patternBased on previous learnings from PR #37984, keeping separate response types is the right approach here.
app/client/src/git/requests/checkoutRefRequest.ts (1)
31-35: LGTM: Clean parameter mapping for backward compatibilityThe transformation of new parameters to the old format is handled cleanly.
app/client/src/git/requests/updateProtectedBranchesRequest.ts (3)
8-8: LGTM: Import for new type added.Clean import of GitArtifactType enum for the new API contract implementation.
Line range hint
10-18: LGTM: Original implementation preserved.Good practice maintaining backward compatibility by preserving the old implementation with a clear naming convention.
20-29: Verify error handling for invalid artifact types.The new implementation looks clean, but we should ensure proper error handling for invalid artifact types.
Let's verify the allowed values for GitArtifactType:
✅ Verification successful
TypeScript enum provides sufficient type safety
The
GitArtifactTypeenum is well-defined with specific values (Application, Package, Workflow) and TypeScript's type system ensures type safety at compile time. No additional error handling is needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for GitArtifactType enum definition ast-grep --pattern 'enum GitArtifactType { $$$ }'Length of output: 367
app/client/src/git/requests/mergeRequest.ts (3)
5-5: LGTM! Clean import addition.
14-23: Clean implementation of the new API contract.The URL construction follows RESTful conventions and the typing is precise.
7-12: Consider updating function parameter name for consistency.The parameter name change from
branchedApplicationIdtorefArtifactIdin the function declaration doesn't match the URL construction which still uses/app/. This might cause confusion.Run this script to check for other occurrences of this parameter naming pattern:
app/client/src/git/requests/triggerAutocommitRequest.ts (2)
5-5: LGTM! Clean type import.
7-11: Verify parameter rename impact.The parameter rename from
branchedApplicationIdtorefArtifactIdmight affect existing function calls.app/client/src/git/requests/toggleAutocommitRequest.ts (3)
5-5: LGTM! Import added for new type.
7-13: LGTM! Clean refactor of existing implementation.Good practice maintaining backward compatibility by preserving the old implementation.
15-22: Verify artifactType validation.The new implementation looks good, but we should ensure that artifactType values are validated before being used in the URL construction.
Let's verify the allowed values for artifactType:
✅ Verification successful
Type system ensures valid artifactType values
The GitArtifactType enum provides compile-time type safety, preventing invalid values from being passed to the function. The implementation is secure as-is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for GitArtifactType enum definition and its usage ast-grep --pattern 'enum GitArtifactType { $$$ }'Length of output: 367
app/client/src/git/requests/commitRequest.ts (3)
8-8: LGTM! Clean import addition.
Line range hint
10-18: LGTM! Clean function rename.Good practice keeping the old implementation intact for backward compatibility.
31-42: Consider adding return type annotation and verify usage.The function would benefit from an explicit return type annotation. Also, verify that all callers are updated to provide the new parameters.
export default async function commitRequest( artifactType: GitArtifactType, refArtifactId: string, params: CommitRequestParams, isNew: boolean, -) { +): AxiosPromise<CommitResponse> {Let's verify the usage:
app/client/src/git/requests/fetchProtectedBranchesRequest.ts (3)
5-5: LGTM! Import looks good.
7-11: Good approach maintaining backward compatibility.Clean rename of the original function with preserved functionality.
13-18: Verify URL construction for all artifact types.The implementation looks good, but let's ensure the URL construction works correctly for all possible GitArtifactType values.
✅ Verification successful
URL construction verified successfully
The implementation correctly handles all GitArtifactType values (applications, packages, workflows) and follows the established API endpoint pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for GitArtifactType enum definition and its usage ast-grep --pattern 'enum GitArtifactType { $$$ }'Length of output: 367
Script:
#!/bin/bash # Search for GIT_BASE_URL definition rg "GIT_BASE_URL\s*=" -A 1 # Search for similar URL construction patterns rg "${GIT_BASE_URL}/.+?/protected" --type tsLength of output: 1244
app/client/src/git/requests/updateLocalProfileRequest.ts (3)
8-8: LGTM! Import added for the new type.
10-15: LGTM! Good practice maintaining backward compatibility.The old implementation is preserved correctly for existing consumers.
17-26: Verify URL construction across all artifact types.The implementation looks clean. Let's verify the URL construction works for all possible artifact types.
✅ Verification successful
URL construction is valid and follows established patterns
The implementation follows the same URL structure used by other Git profile endpoints in the codebase and correctly handles artifact types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the URL pattern is consistent with other Git API endpoints # and verify all possible artifact types. # Search for similar Git API endpoint patterns rg -g '*.ts' -g '*.js' "${GIT_BASE_URL}/[a-z]+/[^/]+/[a-z]+" --type-add 'ts:*.{ts,tsx}' # Search for all possible GitArtifactType values ast-grep --pattern 'export enum GitArtifactType { $$$ }'Length of output: 65704
Script:
#!/bin/bash # Search for Git artifact type definitions and usages rg -g '*.ts' -g '*.js' "GitArtifactType\." -B 2 -A 2 # Search for similar Git API endpoint patterns rg -g '*.ts' -g '*.js' "GIT_BASE_URL.*profile" -B 2 -A 2Length of output: 6694
app/client/src/git/requests/disconnectRequest.ts (2)
5-5: LGTM! Clean type import.
13-20: Verify URL pattern consistency.The implementation looks clean. Let's verify that the new URL pattern is consistent with other Git API endpoints.
✅ Verification successful
URL pattern is consistent with modernized Git API endpoints.
The new implementation follows the standardized RESTful pattern
${GIT_BASE_URL}/${artifactType}/${id}/[action]used across other modernized Git endpoints. This is part of a broader API contracts update.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar Git API endpoint patterns rg -g '*.ts' -g '*.js' "${GIT_BASE_URL}/[a-z]+/[^/]+/[a-z]+" app/client/src/Length of output: 65599
Script:
#!/bin/bash # Search for Git API endpoint patterns ast-grep --pattern 'Api.post(`${GIT_BASE_URL}/$_/$_/$_`, $_)' # Also search for similar Git API endpoint patterns with different HTTP methods ast-grep --pattern 'Api.get(`${GIT_BASE_URL}/$_/$_/$_`, $_)' ast-grep --pattern 'Api.put(`${GIT_BASE_URL}/$_/$_/$_`, $_)' ast-grep --pattern 'Api.delete(`${GIT_BASE_URL}/$_/$_/$_`, $_)' # Search for GIT_BASE_URL constant usage rg "GIT_BASE_URL" -A 2 -B 2Length of output: 126
Script:
#!/bin/bash # Search for Git-related files and their content rg -g '*.ts' -g '*.js' "GIT_BASE_URL|gitApi|GitApi" app/client/src/git/ # Search for files with Git API endpoints find app/client/src/git -type f -name "*.ts" -o -name "*.js" -exec cat {} \;Length of output: 17789
Description
release_git_api_contracts_enabledFixes #38500
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/12810595516
Commit: 8f05bbf
Cypress dashboard.
Tags:
@tag.GitSpec:
Thu, 16 Jan 2025 15:05:20 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
release_git_api_contracts_enabledImprovements
Changes
Technical Updates