chore: git pkg - mod fixes #39255
chore: git pkg - mod fixes #39255
Conversation
WalkthroughThis PR enhances Git-related functionalities and data consistency. The Package interface now supports optional Git artifact metadata, and Git connection messages are updated to accept an artifact type. Git permission functions have been renamed and centralized. UI components have been revised to render Git badges more efficiently via memoization, and a new CardBadge component has been introduced. Several helper functions and hooks are added or updated, and many Git sagas have been refactored to use a centralized error handler. Redux actions, selectors, and type definitions are updated, and Git application context providers with related sagas are now included for streamlined Git operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as UI Component
participant Redux as Redux Store
participant Saga as Git Saga
participant Handler as handleApiErrors
User->>UI: Initiates Git connect action
UI->>Redux: Dispatch connect action
Redux->>Saga: Trigger connectSaga
Saga->>Handler: Call handleApiErrors on response/error
alt Success
Handler-->>Saga: Returns no error
Saga->>Redux: Dispatch connectSuccess action
Redux->>UI: Update Git badge & messages (Card renders via memoized CardBadge)
else Error
Handler-->>Saga: Returns processed error
Saga->>Redux: Dispatch connectError action
Redux->>UI: Show error state/message
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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
the
skip-changelog
|
@ashit-rath I know this looks like a lot of changes, but a lot of these changes are redundant. Like same changes across all saga files (for better error handling) |
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (32)
app/client/src/git/requests/updateGlobalProfileRequest.ts (1)
21-30: Consider adding documentation for theisNewparameter.While the implementation is correct, adding JSDoc comments would help clarify the purpose of the
isNewflag and when to use each endpoint.+/** + * Updates the global profile using either the new artifacts endpoint or the legacy endpoint + * @param params - The profile parameters to update + * @param isNew - When true, uses the artifacts endpoint; otherwise uses the legacy endpoint + */ export default async function updateGlobalProfileRequest( params: UpdateGlobalProfileRequestParams, isNew: boolean, ): AxiosPromise<UpdateGlobalProfileResponse> {app/client/src/git/sagas/fetchStatusSaga.ts (1)
47-52: Consider tracking error handling improvements.The commented code suggests specific error handling improvements. Consider creating an issue to track these enhancements rather than keeping them as comments.
Would you like me to help create an issue to track these error handling improvements?
app/client/src/git/sagas/generateSSHKeySaga.ts (1)
49-57: Consider adding type safety for error handling.While the error handling is well-structured, consider adding type safety for the error object.
- const error = handleApiErrors(e as Error, response); + const error = handleApiErrors(e as Error, response as GenerateSSHKeyResponse); - if (error) { + if (error && 'code' in error) {app/client/src/git/requests/generateSSHKeyRequest.ts (3)
10-17: Rename 'Old' function to a more descriptive term.
While it's clear this is for legacy or backward compatibility, consider a more explicit name, e.g.generateSSHKeyRequestLegacy.-async function generateSSHKeyRequestOld( +async function generateSSHKeyRequestLegacy(
19-27: Consider handling potential errors explicitly.
Adding a simple try/catch or error handling logic aroundApi.postmay improve debuggability.
29-40: Unified approach could simplify management.
Using a single function parameterized by artifact type and versioning might reduce branching overhead in the long run. If legacy support is short-lived, clarifying a deprecation path will help.app/client/src/git/sagas/fetchMergeStatusSaga.ts (1)
37-46: Consider moving response validation to the helper.Since we're centralizing error handling, consider moving the response validation logic into
handleApiErrorsto maintain consistency.- const isValidResponse: boolean = yield validateResponse(response); - - if (response && isValidResponse) { + if (response) { yield put( gitArtifactActions.fetchMergeStatusSuccess({ artifactDef, responseData: response.data, }), ); }app/client/src/git/sagas/fetchBranchesSaga.ts (1)
47-53: Consider adding error type information.While the error handling is good, consider adding type information for better type safety:
- } catch (e) { - const error = handleApiErrors(e as Error, response); + } catch (error: unknown) { + const processedError = handleApiErrors(error as Error, response); - if (error) { - yield put(gitArtifactActions.fetchBranchesError({ artifactDef, error })); + if (processedError) { + yield put(gitArtifactActions.fetchBranchesError({ artifactDef, error: processedError }));app/client/src/git/requests/fetchSSHKeyRequest.ts (1)
7-11: Consider merging fetch calls
The old request is nearly identical to the new one. Consider merging them with a parameter switch to minimize duplication.app/client/src/git/sagas/pullSaga.ts (1)
59-59: Success toast message
Notifying the user after a partial success might be confusing if there's still a conflict. Consider clarifying messaging.app/client/src/git/components/RepoLimitErrorModal/RepoLimitErrorModalView.tsx (1)
25-37: Consider removing unused imports to streamline code.Most imports look necessary, but please confirm that all are actively used (e.g., checking for any possible leftover from earlier refactors). Removing unused imports helps keep the code clean.
app/client/src/git/artifact-helpers/ce/package/packageRedirectToClosestEntitySaga.ts (1)
1-5: Consider leaving a clearer placeholder for future logic.An empty generator might be confusing, even with the existing comment. Optionally add a short remark or
TODOnote about the upcoming redirection logic.app/client/src/git/helpers/getBranchParam.ts (1)
3-12: Add explicit return type for better type safety.The function implementation is correct, but would benefit from an explicit return type declaration.
-export default function getBranchParam() { +export default function getBranchParam(): string | undefined {app/client/src/git/helpers/updateBranchParam.ts (1)
4-12: Add input validation for branch parameter.Consider adding validation for the branch parameter to handle empty strings or invalid branch names.
export const updateBranchParam = (branch: string) => { + if (!branch?.trim()) { + return; + } const url = new URL(window.location.href);app/client/src/git/helpers/isAutocommitEnabled.ts (1)
5-11: Simplify boolean return.The function can be simplified to a direct boolean expression.
-function isAutocommitEnabled(artifactDef: GitArtifactDef) { - if (artifactDef.artifactType === GitArtifactType.Application) { - return true; - } - - return false; -} +function isAutocommitEnabled(artifactDef: GitArtifactDef) { + return artifactDef.artifactType === GitArtifactType.Application; +}app/client/src/git/helpers/isProtectedBranchesEnabled.ts (1)
4-11: Document removal timeline for temporary implementation.The comment indicates this is a temporary solution, but doesn't specify when or under what conditions it will be removed. Consider adding more context about the timeline and conditions for supporting this feature in packages.
app/client/src/git/types.ts (1)
15-19: Add TSDoc comments for the new Git artifact types.Consider adding documentation comments to explain the purpose and usage of these types, especially since they're part of the public API.
+/** Represents a Git artifact for an application */ export type GitApplicationArtifact = ApplicationPayload; +/** Represents a Git artifact for a package */ export type GitPackageArtifact = Package; +/** Union type representing either an application or package Git artifact */ export type GitArtifact = GitApplicationArtifact | GitPackageArtifact;app/client/src/ce/constants/PackageConstants.ts (1)
14-23: Consider extracting Git metadata interface and adding documentation.The Git metadata structure could be extracted into a separate interface for better reusability and documentation.
+/** Metadata for Git-connected artifacts */ +export interface GitArtifactMetadata { + branchName: string; + defaultBranchName: string; + remoteUrl: string; + repoName: string; + browserSupportedUrl?: string; + isRepoPrivate?: boolean; + browserSupportedRemoteUrl: string; + defaultApplicationId: string; +} export interface Package { // ... other fields ... - gitArtifactMetadata?: { - branchName: string; - defaultBranchName: string; - remoteUrl: string; - repoName: string; - browserSupportedUrl?: string; - isRepoPrivate?: boolean; - browserSupportedRemoteUrl: string; - defaultApplicationId: string; - }; + gitArtifactMetadata?: GitArtifactMetadata; }app/client/src/git/requests/fetchGlobalProfileRequest.ts (1)
14-22: Consider implementing feature toggle using environment variables.The boolean parameter approach for toggling between old and new endpoints could be replaced with an environment variable or configuration setting for better maintainability.
-export default async function fetchGlobalProfileRequest( - isNew: boolean, -): AxiosPromise<FetchGlobalProfileResponse> { - if (isNew) { - return fetchGlobalProfileRequestNew(); - } else { - return fetchGlobalProfileRequestOld(); - } +const USE_NEW_PROFILE_ENDPOINT = process.env.REACT_APP_USE_NEW_PROFILE_ENDPOINT === 'true'; + +export default async function fetchGlobalProfileRequest(): AxiosPromise<FetchGlobalProfileResponse> { + return USE_NEW_PROFILE_ENDPOINT + ? fetchGlobalProfileRequestNew() + : fetchGlobalProfileRequestOld(); }app/client/src/git/components/CardBadge/CardBadgeView.tsx (1)
21-29: Add aria-label for better accessibility.The icon button should have an aria-label for screen readers.
function CardBadgeView() { return ( <Container> <Tooltip content={createMessage(CONNECTED_TO_GIT)}> - <Icon name="fork" size="md" /> + <Icon name="fork" size="md" aria-label={createMessage(CONNECTED_TO_GIT)} /> </Tooltip> </Container> ); }app/client/src/git/sagas/helpers/handleApiErrors.ts (1)
5-29: Define error types and messages as constants.Consider creating an enum for error codes and a map for error messages to ensure consistency across the application.
enum GitErrorCode { NOT_FOUND = 'NOT_FOUND', UNKNOWN = 'UNKNOWN' } const ERROR_MESSAGES = { [GitErrorCode.NOT_FOUND]: 'Not found', [GitErrorCode.UNKNOWN]: 'Unknown error' } as const; export default function handleApiErrors(error?: Error, response?: ApiResponse) { // ... rest of the implementation using GitErrorCode and ERROR_MESSAGES }app/client/src/git/store/actions/pullActions.ts (1)
4-7: Add JSDoc comments for the interface and its properties.Document the purpose of the interface and the new optional property for better maintainability.
+/** + * Payload for initializing a Git pull operation + */ export interface PullInitPayload { + /** ID of the artifact to pull */ artifactId: string; + /** Whether to display errors in a popup instead of the default location */ showErrorInPopup?: boolean; }app/client/src/git/requests/fetchLocalProfileRequest.ts (1)
23-29: Consider adding deprecation notice for old endpoint.The conditional logic suggests a transition period. Consider adding a deprecation notice for the old endpoint to encourage migration to the new one.
app/client/src/git/sagas/fetchGlobalSSHKeySaga.ts (1)
20-22: Add validation for SSH key parameters.Consider validating the keyType parameter before making the API call to prevent potential issues.
const params: GenerateSSHKeyRequestParams = { keyType: action.payload.keyType, }; + +if (!['rsa', 'ed25519'].includes(params.keyType)) { + throw new Error(`Invalid key type: ${params.keyType}`); +}app/client/src/git/components/StatusChanges/StatusChangesView.tsx (1)
43-43: Consider a safer null check implementation.The current implementation uses optional chaining with length check, which could be problematic if
statusTreeis null. Consider separating the checks for better clarity and safety.- if (!status || !statusTree || statusTree?.length === 0) { + if (!status || !statusTree || (Array.isArray(statusTree) && statusTree.length === 0)) {app/client/src/git/sagas/initGitSaga.ts (1)
30-32: Simplify boolean condition.The double negation is unnecessary and can be simplified.
- if (!!branchName) { + if (branchName) {🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
app/client/src/git/components/ConnectModal/index.tsx (1)
73-73: Consider early return for permission check.The ternary operator could be simplified using an early return pattern.
- onOpenImport={isConnectPermitted ? onOpenImport : null} + onOpenImport={!isConnectPermitted ? null : onOpenImport}app/client/src/git/sagas/disconnectSaga.ts (1)
13-13: LGTM! Error handling improvements look good.The centralized error handling via
handleApiErrorsimproves consistency across sagas.Consider adding a debug log statement before dispatching the error action to help with troubleshooting:
if (error) { + console.debug("[Git] Disconnect failed:", error); yield put(gitArtifactActions.disconnectError({ artifactDef, error })); }Also applies to: 67-71
app/client/src/git/sagas/gitImportSaga.ts (1)
89-100: Consider optimizing error handling based on Redux effects behavior.Based on previous learnings from PR #38634, try-catch blocks might be unnecessary for Redux
puteffects as they don't throw errors. Consider moving only the API call and response validation inside the try-catch block.try { const isGitApiContractsEnabled: boolean = yield select( selectGitApiContractsEnabled, ); response = yield call( gitImportRequest, workspaceId, params, isGitApiContractsEnabled, ); const isValidResponse: boolean = yield validateResponse(response); + } catch (e) { + const error = handleApiErrors(e as Error, response); + if (error) { + yield put(gitGlobalActions.gitImportError({ error })); + if (GitErrorCodes.REPO_LIMIT_REACHED === error.code) { + yield put(gitGlobalActions.toggleImportModal({ open: false })); + yield put(gitGlobalActions.toggleRepoLimitErrorModal({ open: true })); + } + return; + } + } - if (response && isValidResponse) { + if (!response || !isValidResponse) return; const allWorkspaces: Workspace[] = yield select(getFetchedWorkspaces); // ... rest of the success flow - } -} catch (e) { - const error = handleApiErrors(e as Error, response); - if (error) { - yield put(gitGlobalActions.gitImportError({ error })); - if (GitErrorCodes.REPO_LIMIT_REACHED === error.code) { - yield put(gitGlobalActions.toggleImportModal({ open: false })); - yield put(gitGlobalActions.toggleRepoLimitErrorModal({ open: true })); - } - } -}app/client/src/git/components/ConnectSuccessModal/ConnectSuccessModalView.tsx (1)
149-188: Consider extracting conditional content into separate components.The conditional rendering logic could be simplified by extracting the content into separate components for better maintainability.
+const ProtectedBranchesContent = () => ( + <> + <div className="mb-1"> + <Text renderAs="p"> + {createMessage(GIT_CONNECT_SUCCESS_PROTECTION_MSG)} + </Text> + </div> + <LinkText className="inline-block" isBold renderAs="p"> + <Link + data-testid="t--git-success-modal-learn-more-link" + target="_blank" + to={DOCS_BRANCH_PROTECTION_URL} + > + {createMessage(GIT_CONNECT_SUCCESS_PROTECTION_DOC_CTA)} + </Link> + </LinkText> + </> +); + +const GenericContent = ({ artifactType }: { artifactType: GitArtifactType | null }) => ( + <> + <div className="mb-1"> + <Text renderAs="p"> + {createMessage(GIT_CONNECT_SUCCESS_GENERIC_MESSAGE, singular(artifactType ?? ""))} + </Text> + </div> + <LinkText className="inline-block" isBold renderAs="p"> + <Link + data-testid="t--git-success-modal-learn-more-link" + target="_blank" + to="https://docs.appsmith.com/advanced-concepts/version-control-with-git" + > + {createMessage(GIT_CONNECT_SUCCESS_GENERIC_DOC_CTA)} + </Link> + </LinkText> + </> +); -{showProtectedBranchesInfo ? ( - <> - <div className="mb-1"> - <Text renderAs="p"> - {createMessage(GIT_CONNECT_SUCCESS_PROTECTION_MSG)} - </Text> - </div> - <LinkText className="inline-block" isBold renderAs="p"> - <Link - data-testid="t--git-success-modal-learn-more-link" - target="_blank" - to={DOCS_BRANCH_PROTECTION_URL} - > - {createMessage(GIT_CONNECT_SUCCESS_PROTECTION_DOC_CTA)} - </Link> - </LinkText> - </> -) : ( - <> - <div className="mb-1"> - <Text renderAs="p"> - {createMessage(GIT_CONNECT_SUCCESS_GENERIC_MESSAGE, singular(artifactType ?? ""))} - </Text> - </div> - <LinkText className="inline-block" isBold renderAs="p"> - <Link - data-testid="t--git-success-modal-learn-more-link" - target="_blank" - to="https://docs.appsmith.com/advanced-concepts/version-control-with-git" - > - {createMessage(GIT_CONNECT_SUCCESS_GENERIC_DOC_CTA)} - </Link> - </LinkText> - </> -)} +{showProtectedBranchesInfo ? <ProtectedBranchesContent /> : <GenericContent artifactType={artifactType} />}app/client/src/git/store/selectors/gitArtifactSelectors.ts (1)
144-158: Consider simplifying the branch name retrieval logic.The current implementation has repeated null coalescing and could benefit from safer package ID access.
-if (artifactDef?.artifactType === GitArtifactType.Application) { - return ( - state.ui.applications?.currentApplication?.gitApplicationMetadata - ?.branchName ?? null - ); -} else if (artifactDef?.artifactType === GitArtifactType.Package) { - const currentPackageId = state.ui?.editor?.currentPackageId; - - return ( - state.entities.packages?.[currentPackageId ?? ""]?.gitArtifactMetadata - ?.branchName ?? null - ); -} - -return null; +const getBranchName = () => { + switch(artifactDef?.artifactType) { + case GitArtifactType.Application: + return state.ui.applications?.currentApplication?.gitApplicationMetadata?.branchName; + case GitArtifactType.Package: + const currentPackageId = state.ui?.editor?.currentPackageId ?? ""; + return state.entities.packages?.[currentPackageId]?.gitArtifactMetadata?.branchName; + default: + return null; + } +}; + +return getBranchName() ?? null;app/client/src/components/common/Card.tsx (1)
398-398: Consider using early return for conditional rendering.The ternary operator could be simplified for better readability.
-{showGitBadge ? gitBadge : null} +{showGitBadge && gitBadge}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (88)
app/client/src/ce/constants/PackageConstants.ts(1 hunks)app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/ce/utils/permissionHelpers.tsx(2 hunks)app/client/src/components/common/Card.tsx(4 hunks)app/client/src/components/gitContexts/GitApplicationContextProvider.tsx(3 hunks)app/client/src/git/artifact-helpers/application/applicationConnectToGitSaga.ts(1 hunks)app/client/src/git/artifact-helpers/application/applicationRedirectToClosestEntitySaga.ts(1 hunks)app/client/src/git/artifact-helpers/ce/package/packageConnectToGitSaga.ts(1 hunks)app/client/src/git/artifact-helpers/ce/package/packageRedirectToClosestEntitySaga.ts(1 hunks)app/client/src/git/artifact-helpers/ce/package/packageStatusTransformer.ts(1 hunks)app/client/src/git/artifact-helpers/ee/package/packageConnectToGitSaga.ts(1 hunks)app/client/src/git/artifact-helpers/ee/package/packageRedirectToClosestEntitySaga.ts(1 hunks)app/client/src/git/artifact-helpers/ee/package/packageStatusTransformer.ts(1 hunks)app/client/src/git/artifact-helpers/package/packageArtifact.ts(1 hunks)app/client/src/git/artifact-helpers/package/packageConnectToGitSaga.ts(1 hunks)app/client/src/git/artifact-helpers/package/packageRedirectToClosestEntitySaga.ts(1 hunks)app/client/src/git/artifact-helpers/package/packageStatusTransformer.ts(1 hunks)app/client/src/git/components/CardBadge/CardBadgeView.tsx(1 hunks)app/client/src/git/components/CardBadge/index.tsx(1 hunks)app/client/src/git/components/ConnectModal/ConnectModalView.tsx(1 hunks)app/client/src/git/components/ConnectModal/index.tsx(3 hunks)app/client/src/git/components/ConnectSuccessModal/ConnectSuccessModalView.tsx(5 hunks)app/client/src/git/components/ConnectSuccessModal/index.tsx(2 hunks)app/client/src/git/components/DangerZone/index.tsx(1 hunks)app/client/src/git/components/GitContextProvider/index.tsx(6 hunks)app/client/src/git/components/ImportModal/index.tsx(1 hunks)app/client/src/git/components/QuickActions/index.tsx(1 hunks)app/client/src/git/components/RepoLimitErrorModal/RepoLimitErrorModalView.tsx(1 hunks)app/client/src/git/components/SettingsModal/index.tsx(1 hunks)app/client/src/git/components/StatusChanges/StatusChangesView.tsx(1 hunks)app/client/src/git/helpers/getBranchParam.ts(1 hunks)app/client/src/git/helpers/isAutocommitEnabled.ts(1 hunks)app/client/src/git/helpers/isProtectedBranchesEnabled.ts(1 hunks)app/client/src/git/helpers/updateBranchParam.ts(1 hunks)app/client/src/git/hooks/index.ts(1 hunks)app/client/src/git/hooks/useConnect.ts(2 hunks)app/client/src/git/hooks/useGitPermissions.ts(0 hunks)app/client/src/git/index.ts(1 hunks)app/client/src/git/requests/checkoutBranchRequest.types.ts(1 hunks)app/client/src/git/requests/checkoutRefRequest.types.ts(1 hunks)app/client/src/git/requests/deleteRefRequest.types.ts(1 hunks)app/client/src/git/requests/discardRequest.types.ts(1 hunks)app/client/src/git/requests/disconnectRequest.types.ts(1 hunks)app/client/src/git/requests/fetchGlobalProfileRequest.ts(1 hunks)app/client/src/git/requests/fetchLocalProfileRequest.ts(1 hunks)app/client/src/git/requests/fetchSSHKeyRequest.ts(1 hunks)app/client/src/git/requests/generateSSHKeyRequest.ts(1 hunks)app/client/src/git/requests/updateGlobalProfileRequest.ts(1 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(3 hunks)app/client/src/git/sagas/createBranchSaga.ts(2 hunks)app/client/src/git/sagas/deleteBranchSaga.ts(2 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(2 hunks)app/client/src/git/sagas/fetchGlobalProfileSaga.ts(2 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(2 hunks)app/client/src/git/sagas/fetchProtectedBranchesSaga.ts(2 hunks)app/client/src/git/sagas/fetchSSHKeySaga.ts(2 hunks)app/client/src/git/sagas/fetchStatusSaga.ts(2 hunks)app/client/src/git/sagas/generateSSHKeySaga.ts(3 hunks)app/client/src/git/sagas/gitImportSaga.ts(2 hunks)app/client/src/git/sagas/helpers/handleApiErrors.ts(1 hunks)app/client/src/git/sagas/initGitSaga.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/updateGlobalProfileSaga.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/connectActions.ts(1 hunks)app/client/src/git/store/actions/initGitActions.ts(1 hunks)app/client/src/git/store/actions/pullActions.ts(1 hunks)app/client/src/git/store/gitArtifactSlice.ts(2 hunks)app/client/src/git/store/selectors/gitArtifactSelectors.ts(2 hunks)app/client/src/git/store/types.ts(2 hunks)app/client/src/git/types.ts(1 hunks)app/client/src/pages/Editor/IDE/Header/DeployButton.tsx(1 hunks)app/client/src/pages/Editor/gitSync/Tabs/ConnectionSuccess.tsx(1 hunks)app/client/src/pages/Editor/gitSync/hooks/gitPermissionHooks.ts(1 hunks)app/client/src/pages/Editor/gitSync/hooks/modHooks.ts(1 hunks)app/client/src/sagas/ErrorSagas.tsx(1 hunks)app/client/src/selectors/editorSelectors.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/git/hooks/useGitPermissions.ts
✅ Files skipped from review due to trivial changes (10)
- app/client/src/git/artifact-helpers/package/packageStatusTransformer.ts
- app/client/src/git/artifact-helpers/ee/package/packageStatusTransformer.ts
- app/client/src/git/artifact-helpers/package/packageConnectToGitSaga.ts
- app/client/src/git/artifact-helpers/package/packageRedirectToClosestEntitySaga.ts
- app/client/src/git/artifact-helpers/ee/package/packageRedirectToClosestEntitySaga.ts
- app/client/src/git/requests/deleteRefRequest.types.ts
- app/client/src/git/requests/checkoutRefRequest.types.ts
- app/client/src/git/requests/disconnectRequest.types.ts
- app/client/src/git/artifact-helpers/ee/package/packageConnectToGitSaga.ts
- app/client/src/git/requests/discardRequest.types.ts
🧰 Additional context used
🧠 Learnings (14)
app/client/src/git/sagas/deleteBranchSaga.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/deleteBranchSaga.ts:38-45
Timestamp: 2024-12-10T10:52:57.789Z
Learning: In the TypeScript file `app/client/src/git/sagas/deleteBranchSaga.ts`, within the `deleteBranchSaga` function, error handling is managed outside the scope of the catch block. Therefore, casting `error` to `string` in this context is acceptable.
app/client/src/git/sagas/fetchProtectedBranchesSaga.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/sagas/createBranchSaga.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/deleteBranchSaga.ts:38-45
Timestamp: 2024-12-10T10:52:57.789Z
Learning: In the TypeScript file `app/client/src/git/sagas/deleteBranchSaga.ts`, within the `deleteBranchSaga` function, error handling is managed outside the scope of the catch block. Therefore, casting `error` to `string` in this context is acceptable.
app/client/src/git/sagas/gitImportSaga.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/sagas/fetchGlobalSSHKeySaga.ts (2)
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.
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.
app/client/src/git/sagas/fetchStatusSaga.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/sagas/fetchMergeStatusSaga.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/sagas/commitSaga.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/fetchGlobalProfileRequest.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#38031
File: app/client/src/git/requests/fetchGlobalProfileRequest.ts:6-8
Timestamp: 2024-12-07T11:32:14.299Z
Learning: In the Appsmith codebase, the `Api` module already handles request configurations, including error handling and default headers. Therefore, additional configurations in request functions like `fetchGlobalProfileRequest` are unnecessary.
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/sagas/updateLocalProfileSaga.ts (3)
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`.
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.
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.
app/client/src/git/sagas/fetchLocalProfileSaga.ts (3)
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.
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/fetchGlobalProfileSaga.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/sagas/fetchBranchesSaga.ts (2)
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.
Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/sagas/deleteBranchSaga.ts:38-45
Timestamp: 2024-12-10T10:52:57.789Z
Learning: In the TypeScript file `app/client/src/git/sagas/deleteBranchSaga.ts`, within the `deleteBranchSaga` function, error handling is managed outside the scope of the catch block. Therefore, casting `error` to `string` in this context is acceptable.
🪛 Biome (1.9.4)
app/client/src/git/sagas/initGitSaga.ts
[error] 30-30: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
🔇 Additional comments (102)
app/client/src/git/sagas/fetchGlobalProfileSaga.ts (3)
1-1: LGTM! Good organization of imports.The new imports are well-organized, separating external dependencies from internal ones.
Also applies to: 8-9
15-19: LGTM! Clean feature flag implementation.The feature flag check is properly implemented using Redux's select effect, and the result is correctly passed to the request function.
31-35: LGTM! Improved error handling.The error handling aligns with the team's established patterns, as confirmed by the retrieved learnings about error management being handled outside the saga scope.
app/client/src/git/requests/updateGlobalProfileRequest.ts (1)
15-19: LGTM! New endpoint implementation looks clean.The new function maintains consistent error handling through the Api class while introducing the new artifacts endpoint.
app/client/src/git/sagas/fetchStatusSaga.ts (2)
9-9: LGTM! Centralized error handling.The addition of
handleApiErrorshelper aligns with the broader refactoring effort to standardize error handling across Git sagas.
41-45: LGTM! Simplified error handling.The error handling is now more maintainable and consistent with other Git sagas. The use of
handleApiErrorshelper reduces code duplication and centralizes error processing logic.app/client/src/git/sagas/generateSSHKeySaga.ts (2)
10-10: LGTM! Import statements are well-organized.The new imports support the feature flag check and centralized error handling functionality.
Also applies to: 12-12, 14-14
27-30: LGTM! Feature flag integration is well-implemented.The feature flag check is properly integrated using Redux-Saga's select effect and correctly passed to the request function.
Also applies to: 36-36
app/client/src/git/requests/generateSSHKeyRequest.ts (2)
6-6: Import usage looks consistent.
The addition ofGIT_BASE_URLaligns well with the newly introduced SSH key creation pattern.
8-8: Introduction of GitArtifactType is appropriate.
This strongly typed artifact reference should help maintain clarity in future enhancements.app/client/src/git/sagas/fetchMergeStatusSaga.ts (1)
12-12:✅ Verification successful
LGTM! Improved error handling with centralized helper.
The refactoring to use
handleApiErrorsaligns with the broader initiative to standardize error handling across sagas.Let's verify this is consistent with other sagas:
Also applies to: 48-54
🏁 Script executed:
#!/bin/bash # Check if other sagas follow the same error handling pattern ast-grep --pattern 'const error = handleApiErrors($_, $_)'Length of output: 2842
Great job standardizing error handling!
The change in
fetchMergeStatusSaga.ts—importing and usinghandleApiErrors(line 12 and in the error catch block)—is consistent with the approach used in other sagas (e.g.,commitSaga.ts,checkoutBranchSaga.ts, etc.). This refactor aligns well with our broader initiative to centralize error handling, and no further changes are needed.app/client/src/git/sagas/triggerAutocommitSaga.ts (4)
29-29: LGTM!The import of the centralized error handling helper aligns with the PR's objective of improving error handling for sagas.
76-82: LGTM!The error handling has been improved by using the centralized
handleApiErrorshelper. The conditional check ensures errors are only dispatched when present.
127-132: LGTM!The error handling in the second try-catch block follows the same pattern as the first, maintaining consistency in error handling across the saga.
85-85:❓ Verification inconclusive
Verify type change impact.
The type change from
nulltoundefinedmight affect type checking in other parts of the codebase.Let's verify the impact:
🏁 Script executed:
#!/bin/bash # Search for usages of progressResponse that might expect null ast-grep --pattern 'progressResponse === null' ast-grep --pattern 'progressResponse !== null'Length of output: 97
Type Check Impact on
progressResponse:
The automated search didn’t reveal any strict equality comparisons tonull, but the absence of output makes it a low-quality inference. Please manually verify that no parts of the codebase expect anullvalue forprogressResponseand that all conditions account for anundefinedstate.app/client/src/git/sagas/updateLocalProfileSaga.ts (4)
11-12: LGTM! Clean import restructuring.The removal of logging imports and addition of centralized error handling helper improves code organization.
27-29: LGTM! Feature flag integration.Good practice to use feature flags for API contract changes, allowing for gradual rollout.
45-52: LGTM! Improved error handling.The new error handling approach using
handleApiErrorsaligns with the learnings about error handling in Git sagas.
31-37:❓ Verification inconclusive
Verify API contract compatibility.
The
isGitApiContractsEnabledflag is now passed to the request. Ensure the API endpoint can handle both contract versions.
🏁 Script executed:
#!/bin/bash # Check if the API endpoint handles both contract versions ast-grep --pattern 'function updateLocalProfileRequest($_, $_, $_, isGitApiContractsEnabled: boolean)'Length of output: 104
API Contract Handling: Verify Function Signature
Our initial automated check using ast-grep didn’t return any matches for a definition with the expected signature. This makes it unclear whether the function implementation of
updateLocalProfileRequestexplicitly expects theisGitApiContractsEnabled: booleanparameter and handles it appropriately. Please manually verify that the API endpoint (or the corresponding client function) has been updated to support both contract versions.
- Confirm that the function definition of
updateLocalProfileRequest(or its associated module) indeed includes theisGitApiContractsEnabledparameter and that it contains logic to distinguish between the respective API contracts.- Double-check any related integrations or documentation to ensure both versions are supported as intended.
app/client/src/git/sagas/fetchBranchesSaga.ts (2)
12-12: LGTM! Improved error handling with centralized helper.The refactoring to use
handleApiErrorsaligns with the PR's objective of improving saga error handling. The changes simplify the code and provide consistent error processing across sagas.Also applies to: 48-52
48-48:✅ Verification successful
Verify the implementation of handleApiErrors helper.
Let's verify the implementation of the new error handling helper:
🏁 Script executed:
#!/bin/bash # Search for the implementation of handleApiErrors ast-grep --pattern 'function handleApiErrors($_: $_) { $$$ }'Length of output: 65
Manual Verification Required:
handleApiErrorsHelper ImplementationThe automated search did not locate an implementation for
handleApiErrors. Please verify manually that:
- The helper is correctly defined (as an arrow function, a traditional function, or imported from elsewhere).
- Its error‑handling logic meets the intended design when used in
app/client/src/git/sagas/fetchBranchesSaga.ts(line 48).If the helper exists in a nonstandard location or is defined differently than the original search pattern assumed, update the review comment accordingly.
🏁 Script executed:
#!/bin/bash echo "Searching repository for any occurrence of handleApiErrors (arrow function pattern):" rg -n "const handleApiErrors *= *(" . echo "-------------------------------" echo "Searching repository for any occurrence of handleApiErrors (function declaration pattern):" rg -n "function handleApiErrors" .Length of output: 759
Verified Implementation of
handleApiErrorsHelper
- The helper is implemented as a default export in
app/client/src/git/sagas/helpers/handleApiErrors.ts(starting at line 5).- Its existence confirms that the error handling logic invoked in
app/client/src/git/sagas/fetchBranchesSaga.ts(line 48) is backed by a concrete implementation.- Please review the helper’s internals to ensure it handles both the error and response as intended for your application’s requirements.
app/client/src/git/requests/fetchSSHKeyRequest.ts (3)
4-5: Good import usage
These constants and types are correct for referencing the new Git artifact URL.
13-20: Ensure consistent error handling
No error handling is present. Verify upstream usage for robust error coverage.
23-31: Conditional logic is clear
Switching between the old and new request is straightforward.app/client/src/git/sagas/discardSaga.ts (5)
8-8: Minor import revision
No issues found.
10-13: Consolidated error handling
UsinghandleApiErrorsis consistent with the new approach. Good improvement.
36-40: Artifact-based redirect
The new conditional ensures a clear path for both Application and Package artifacts. Looks good.
48-54: Modal toggling
Hiding the ops modal after discard is a better user experience than redirecting abruptly.
57-59: Error state flow
The error is processed and dispatched properly.app/client/src/git/sagas/pullSaga.ts (5)
9-12: Improved imports
Centralizing error handling and artifact-based logic is consistent and simpler.
36-40: Artifact-based redirect
Similar pattern ensures correct redirection for Applications or Packages after pull.
43-44: Simplified block structure
This helps keep the try-catch block organized.
45-45: Refined error handling
Capturing errors with a dedicated helper and dispatching an action is appropriate.Also applies to: 47-49
51-51: Conflict modal
Opening a conflict modal with artifact details is a user-friendly approach.Also applies to: 53-55
app/client/src/git/sagas/checkoutBranchSaga.ts (6)
13-16: Imports look clean and cohesive.These new imports provide a modular approach for API error handling and artifact-based redirection. Nicely structured!
43-46: Ensure edge cases are handled for branch name interpretation.Trimming "origin/" is logical, but confirm no other repository naming patterns will cause issues (e.g., unusual remote names).
48-51: Redirection logic is well-segregated.Leveraging
applicationRedirectToClosestEntitySagaandpackageRedirectToClosestEntitySagasimplifies the flow and keeps the code DRY. Great job!
54-56: Success action dispatch looks straightforward.Toggling the branch popup after success ensures the UI remains consistent. This flow maintains clarity for the user.
59-60: Catching requests is justified here.Since
checkoutRefRequestcan fail, wrapping the saga with try/catch is appropriate. This aligns with the retrieved learning but remains valid given the network call.
62-63: Comprehensive error handling.Passing both
artifactDefand the error object ensures the UI can display detailed messages, facilitating better debugging.app/client/src/git/sagas/connectSaga.ts (4)
14-16: Modular imports align with best practices.Centralized error handling and distinct connect sagas for application vs. package keep the code organized and scalable.
47-50: Delegating artifact-specific logic is clean.This effectively separates the connect flow for applications and packages, ensuring each artifact type can evolve independently.
53-55: Success payload is well-defined.Storing the response data in the Redux slice can help further actions or analytics. Looks good!
70-77: Error handling covers both generic and specialized errors.Using
handleApiErrorsplus explicitREPO_LIMIT_REACHEDchecks ensures seamless user feedback for all scenarios.app/client/src/git/artifact-helpers/application/applicationRedirectToClosestEntitySaga.ts (2)
37-57: Graceful handling of missing pages.Immediately redirecting to the default page is a good user experience. Clean approach to avoid 404 pages upon checkout.
75-117: Thoughtful fallback logic for missing actions or JS objects.By conditionally navigating back to home, you preserve a smooth workflow. The
replacecall also prevents back-navigation loops.app/client/src/git/components/RepoLimitErrorModal/RepoLimitErrorModalView.tsx (3)
70-82: Validate null handling for artifactType.When
artifactTypeisnull, callingsingular(artifactType ?? "")returns an empty string. Confirm that this behavior is acceptable for all scenarios. Otherwise, consider providing a default or validated value.
94-107: Praise for structured analytics logging.The usage of
logEventwith relevant properties helps track user interactions effectively.
167-188: Check internal docs link.Ensure the URL is correct and that it directs to the latest documentation. If needed, keep it in a constants file for easier maintenance.
app/client/src/git/components/CardBadge/index.tsx (1)
4-6: CardBadge usage is straightforward.The wrapper is simple and readable. If
CardBadgeViewever needs props, consider passing them here and exposing them at this level.app/client/src/git/hooks/index.ts (1)
1-4: Well-structured hook exports with consistent naming!The exports follow React hooks best practices with clear, consistent naming and modular organization.
app/client/src/git/artifact-helpers/ce/package/packageConnectToGitSaga.ts (1)
3-7:❓ Verification inconclusive
Verify implementation status of the saga.
The saga is currently empty with only type definitions. Please confirm if this is intentional or if implementation is pending.
🏁 Script executed:
#!/bin/bash # Search for any usage of this saga rg "packageConnectToGitSaga" -A 5Length of output: 1738
Verify the Intentionality of the Empty Saga Implementation
The
packageConnectToGitSagafunction (located atapp/client/src/git/artifact-helpers/ce/package/packageConnectToGitSaga.ts) currently has an empty body—with only type definitions and an ESLint disable comment to preserve the interface. Its usage inapp/client/src/git/sagas/connectSaga.tsconfirms that it is invoked, so please confirm whether this is an intentional stub for future implementation or simply to maintain consistency between the CE and EE modules.app/client/src/git/artifact-helpers/package/packageArtifact.ts (1)
4-9: Clean and type-safe factory function implementation!The function follows single responsibility principle and maintains type safety.
app/client/src/git/requests/checkoutBranchRequest.types.ts (1)
8-8:❓ Verification inconclusive
Verify the impact of response type change.
The response type has changed from extending
ApplicationPayloadto usingGitArtifact. This is a breaking change that might affect consumers of this API.
🏁 Script executed:
#!/bin/bash # Search for usages of CheckoutBranchResponseData rg "CheckoutBranchResponseData" -A 5Length of output: 346
Action Required: Verify API Consumers for Response Type Change
The response type change in
CheckoutBranchResponseData—switching from an interface extendingApplicationPayloadto aliasingGitArtifact—is a breaking change. Although our search shows thatCheckoutBranchResponseDatais only used internally to formCheckoutBranchResponseinapp/client/src/git/requests/checkoutBranchRequest.types.ts, please ensure that:
- API Consumers: Any components or services that expect the previous
ApplicationPayloadstructure are updated to handle the newGitArtifactformat.- Documentation and Integration: Downstream integrations and documentation reflect this change to avoid runtime issues.
app/client/src/git/store/actions/initGitActions.ts (1)
4-6: LGTM! Clean type update.The change to use GitArtifact type aligns well with the new artifact system.
app/client/src/git/requests/fetchLocalProfileRequest.ts (1)
17-17: LGTM! URL structure follows RESTful conventions.The new URL structure
/artifacts/${baseArtifactId}/profileis more RESTful and consistent with artifact-centric design.app/client/src/git/components/SettingsModal/index.tsx (2)
8-14: LGTM! Improved permission management.Consolidating permissions under
useGitContextimproves maintainability and follows the single responsibility principle.
25-25: LGTM! Safe default tab handling.Using nullish coalescing for
settingsModalTabprovides a safe fallback toGitSettingsTab.General.app/client/src/pages/Editor/gitSync/hooks/gitPermissionHooks.ts (1)
3-6: LGTM! Clear permission function naming.The "GitApp" prefix in permission function names clearly indicates their scope.
app/client/src/git/components/ConnectSuccessModal/index.tsx (2)
9-10: LGTM! Safe context and null handling.Good use of destructuring and nullish coalescing for safe context value handling.
22-28: LGTM! Clear prop passing.Props are passed explicitly and consistently, improving component maintainability.
app/client/src/git/store/actions/connectActions.ts (1)
41-46: LGTM! Clean implementation of reset action.The new
resetConnectActionfollows the established pattern and properly handles both loading and error states.app/client/src/git/components/ConnectModal/ConnectModalView.tsx (1)
19-23: LGTM! Consistent prop renaming.The prop rename from
resetSSHKeytoresetConnectStateimproves clarity and all references are properly updated.Also applies to: 25-30, 31-40
app/client/src/git/components/ImportModal/index.tsx (1)
44-44: LGTM! Consistent prop usage.The prop update aligns with the ConnectModalView changes.
app/client/src/pages/Editor/gitSync/hooks/modHooks.ts (2)
1-2: Track technical debt: Temporary file needs cleanup.This file is marked as temporary and should be removed after Git mod is fully rolled out.
Please create a tracking issue for removing this file once Git mod feature is fully enabled.
13-13: LGTM! More specific import path.The import path update improves module organization.
app/client/src/git/sagas/fetchSSHKeySaga.ts (2)
14-24: LGTM! Good addition of feature flag check.The addition of
isGitApiContractsEnabledflag and its proper integration with the request is well implemented.
35-41:✅ Verification successful
Verify error handling behavior.
The new error handling looks cleaner, but let's verify the behavior matches the existing error patterns.
🏁 Script executed:
#!/bin/bash # Search for other sagas using handleApiErrors to verify consistency rg -l "handleApiErrors" "app/client/src/git/sagas/"Length of output: 1257
Error Handling Pattern Verified: Consistent with Other Sagas
The usage ofhandleApiErrorsin this saga follows the same pattern observed across other sagas in the repository. The error handling—catching the exception, processing it viahandleApiErrors, and conditionally dispatching an error action—is consistent with the established error patterns. No changes are required in these lines.app/client/src/git/sagas/fetchMetadataSaga.ts (1)
35-41: LGTM! Consistent error handling implementation.The error handling follows the same pattern as other sagas, maintaining consistency across the codebase.
app/client/src/git/sagas/toggleAutocommitSaga.ts (1)
29-32: LGTM! Good practice with metadata refresh.The implementation correctly refreshes metadata after a successful toggle operation.
app/client/src/git/sagas/fetchLocalProfileSaga.ts (1)
38-44: LGTM! Aligned with established error handling patterns.The implementation correctly follows the team's established error handling patterns, as indicated in the retrieved learnings.
app/client/src/git/artifact-helpers/application/applicationConnectToGitSaga.ts (1)
13-47: Add error handling for failed page fetches.The saga should handle potential failures when fetching the page or updating the application state.
Consider wrapping the critical operations in try-catch blocks:
export default function* applicationConnectToGitSaga( artifactDef: GitArtifactDef, response: ConnectResponse, ) { const pageId: string = yield select(getCurrentPageId); + try { yield put(fetchPageAction(pageId)); const branch = response.data?.gitApplicationMetadata?.branchName; if (branch) { const newUrl = addBranchParam(branch); history.replace(newUrl); } const currentApplication: GitApplicationArtifact = yield select( getCurrentApplication, ); if (currentApplication) { currentApplication.lastDeployedAt = new Date().toISOString(); yield put({ type: ReduxActionTypes.FETCH_APPLICATION_SUCCESS, payload: currentApplication, }); } yield put( gitArtifactActions.initGitForEditor({ artifactDef, artifact: response.data, }), ); + } catch (error) { + yield put(gitArtifactActions.connectError({ + artifactDef, + error: error instanceof Error ? error : new Error(String(error)) + })); + } }app/client/src/git/sagas/mergeSaga.ts (1)
41-45: LGTM! Clean error handling implementation.The error handling has been properly refactored to use the centralized
handleApiErrorshelper.app/client/src/git/sagas/fetchProtectedBranchesSaga.ts (1)
39-45: LGTM! Error handling follows established patterns.The implementation aligns with our learnings about error handling in Git sagas.
app/client/src/git/sagas/fetchGlobalSSHKeySaga.ts (1)
43-47: LGTM! Error handling implementation.The error handling has been properly refactored to use the centralized helper.
app/client/src/git/components/DangerZone/index.tsx (1)
10-15: LGTM! Good refactoring of permission handling.Consolidating permission checks into the Git context improves code organization and maintainability.
app/client/src/git/index.ts (1)
14-14: LGTM! Clean addition of new component export.The export follows the established pattern and maintains good organization.
app/client/src/git/sagas/updateGlobalProfileSaga.ts (1)
25-33: LGTM! Good improvements to error handling and feature flag integration.The saga now properly handles API contracts feature flag and uses centralized error handling.
app/client/src/git/sagas/deleteBranchSaga.ts (1)
14-14: LGTM! Error handling improvements look good.The refactoring to use the centralized
handleApiErrorshelper improves consistency across sagas.Also applies to: 58-60
app/client/src/git/sagas/initGitSaga.ts (1)
22-28: LGTM! Type-safe branch name handling.The explicit type casting and separate handling for different artifact types improves type safety.
app/client/src/git/sagas/updateProtectedBranchesSaga.ts (1)
14-14: LGTM! Consistent error handling implementation.The error handling follows the same pattern as other sagas, using the centralized helper.
Also applies to: 51-53
app/client/src/git/hooks/useConnect.ts (1)
58-62: LGTM! Well-implemented React hook function.The new
resetConnectfunction follows React hooks best practices and maintains consistency with existing patterns.Also applies to: 72-72
app/client/src/git/sagas/createBranchSaga.ts (2)
14-14: LGTM! Consistent error handling approach.The addition of handleApiErrors aligns with the centralized error handling pattern being implemented across Git sagas.
65-69:❓ Verification inconclusive
Verify error handling completeness.
The error handling has been simplified, but we should ensure all error cases are still properly handled.
🏁 Script executed:
#!/bin/bash # Search for other Git sagas to verify consistent error handling ast-grep --pattern 'function* $name($_) { $$$ try { $$$ } catch (e) { const error = handleApiErrors(e as Error, $_); $$$ } }'Length of output: 147
Manual Verification Required for Error Handling Completeness
The simplified error handling in
app/client/src/git/sagas/createBranchSaga.ts(lines 65-69) appears consistent with our expectations. However, our automated search using the provided ast-grep pattern didn’t return matching instances for similar Git sagas. Please manually verify that the handling viahandleApiErrorsefficiently covers all error cases across the sagas and that no edge cases are omitted.app/client/src/git/components/ConnectModal/index.tsx (1)
13-14: LGTM! Improved naming and state management.The renaming from
isCreateArtifactPermittedtoisConnectPermittedbetter reflects the actual permission being checked. The newresetConnectStatefunction properly consolidates all reset operations.Also applies to: 57-61
app/client/src/git/components/QuickActions/index.tsx (1)
15-15: LGTM! Consistent permission management.Moving from
useGitPermissionstouseGitContextaligns with the centralized permission management approach.Also applies to: 18-18
app/client/src/git/sagas/commitSaga.ts (1)
70-78: LGTM! Streamlined error handling.The error handling has been simplified while maintaining the special case for repository limit errors. The use of
handleApiErrorsaligns with the centralized error handling pattern.app/client/src/components/gitContexts/GitApplicationContextProvider.tsx (1)
43-57: LGTM! Good use of memoization for permission checks.The permission checks are properly memoized based on the artifact dependency, which will prevent unnecessary recalculations.
app/client/src/git/components/GitContextProvider/index.tsx (1)
10-25: LGTM! Well-structured interface updates and secure defaults.
- Type updates from ApplicationPayload to GitArtifact improve type safety
- Permission properties default to false, following the principle of least privilege
Also applies to: 75-78
app/client/src/pages/Editor/IDE/Header/DeployButton.tsx (2)
15-20: LGTM! Import restructuring looks good.The Git hooks have been properly organized into their respective modules.
39-40:❓ Verification inconclusive
Verify the deployment flow with protected mode.
The new protection mode adds an additional safeguard for deployments, but we should ensure this doesn't unexpectedly block legitimate deployment scenarios.
🏁 Script executed:
#!/bin/bash # Search for other components that might be affected by the protected mode rg -A 3 "useGitProtectedMode|isProtectedMode" --type typescriptLength of output: 100
Below is a revised review comment. Since the initial search using
--type typescriptfailed to yield results, please re-run the search using the updated file type definition (covering both .ts and .tsx files) to confirm that all protected mode references (i.e. instances ofuseGitProtectedModeandisProtectedMode) are properly accounted for across the codebase before merging.Deployment Flow: Verify Protected Mode Behavior
The new safeguard in the deployment flow should be verified to ensure it doesn’t unexpectedly block legitimate deployments. In particular, please double-check that the use ofuseGitProtectedModeinDeployButton.tsx(lines 39–40) and any associated components only prevents deployments when intended.To aid with this verification, please run the following shell script:
Once you’ve confirmed that all uses of protected mode behave as expected, we can be confident that legitimate deployment scenarios won’t be adversely impacted.
🏁 Script executed:
[run_scripts] #!/bin/bash # Define a custom TypeScript file type that covers both .ts and .tsx files rg -A 3 "useGitProtectedMode|isProtectedMode" --type-add typescript:'*.{ts,tsx}' --type typescriptLength of output: 47374
Verify Deployment Flow with Protected Mode
The deployment logic in
DeployButton.tsxnow disables deployments when eitherisPackageUpgradingorisProtectedModeis true. Our review of the codebase confirms that theuseGitProtectedModehook is widely used throughout various components (e.g., Git selectors, QuickActions, layouts), so it's crucial to manually verify that this safeguard doesn’t inadvertently block legitimate deployments. Please validate that:
- Deployment is correctly enabled when neither package upgrading nor protected mode is active.
- Legitimate deployment scenarios aren’t blocked when only one or the other condition temporarily applies.
app/client/src/pages/Editor/gitSync/Tabs/ConnectionSuccess.tsx (1)
166-166: LGTM! Improved message context.The success message now correctly specifies the artifact type as "application".
app/client/src/git/store/types.ts (1)
116-118:❓ Verification inconclusive
Verify type consistency across the codebase.
The new types for package Git metadata mirror the existing application metadata structure. Ensure all consumers handle both types correctly.
Also applies to: 128-136
🏁 Script executed:
#!/bin/bash # Search for usages of gitArtifactMetadata and currentPackageId rg -A 3 "gitArtifactMetadata|currentPackageId" --type typescriptLength of output: 101
Manual verification needed: Confirm type consistency for Git and app metadata
The previous grep command failed due to using an unrecognized file type flag. Please re-run the search with a glob pattern for TypeScript files. For example, use:
#!/bin/bash # Search for usages of gitArtifactMetadata and currentPackageId in TypeScript files rg -A 3 "gitArtifactMetadata|currentPackageId" -g "*.ts"Then, verify that all consumers of the Git metadata—particularly in
app/client/src/git/store/types.ts(Lines: 116-118 and 128-136)—are handling the new type definitions in a manner consistent with the existing application metadata. This check will ensure that both types mirror each other correctly across the codebase.app/client/src/ce/utils/permissionHelpers.tsx (1)
39-44: LGTM! Clear permission naming.The Git permissions are now properly namespaced under application context.
app/client/src/git/components/ConnectSuccessModal/ConnectSuccessModalView.tsx (1)
70-74: LGTM! Props interface is well-defined.The new props
artifactTypeandshowProtectedBranchesInfoare properly typed and documented.app/client/src/git/store/gitArtifactSlice.ts (1)
8-8: LGTM! Reset action properly integrated.The resetConnect action is correctly imported and added to the reducers list.
Also applies to: 161-161
app/client/src/components/common/Card.tsx (1)
335-343: LGTM! Good use of memoization for the badge component.The gitBadge is properly memoized with the correct dependency array, and the conditional logic is clean.
app/client/src/sagas/ErrorSagas.tsx (2)
139-140: LGTM! Improved null safety with optional chaining.The addition of optional chaining for accessing
response.responseMeta?.error?.codemakes the code more robust by preventing null pointer exceptions.
148-154: LGTM! Enhanced error handling with optional chaining.The addition of optional chaining for error messages in both the API_ERROR action and the thrown error improves the robustness of error handling.
app/client/src/selectors/editorSelectors.tsx (1)
900-900: LGTM! Enhanced type safety.Replacing
anywithstringtype forbaseCollectionIdimproves type safety and code maintainability.app/client/src/ce/constants/messages.ts (2)
1182-1184: LGTM! Good addition of the optional artifactType parameter.The default value "applications" maintains backward compatibility while allowing support for other artifact types.
1188-1189: LGTM! Well-structured new message functions.The new functions GIT_CONNECT_SUCCESS_GENERIC_MESSAGE and GIT_CONNECT_SUCCESS_GENERIC_DOC_CTA provide good abstraction for different artifact types while maintaining consistent messaging patterns.
Also applies to: 1196-1197
app/client/src/git/artifact-helpers/application/applicationRedirectToClosestEntitySaga.ts
Outdated
Show resolved
Hide resolved
app/client/src/git/artifact-helpers/ce/package/packageStatusTransformer.ts
Outdated
Show resolved
Hide resolved
app/client/src/pages/Editor/gitSync/hooks/gitPermissionHooks.ts
Outdated
Show resolved
Hide resolved
app/client/src/components/gitContexts/GitApplicationContextProvider.tsx
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,47 @@ | |||
| import { fetchPageAction } from "actions/pageActions"; | |||
There was a problem hiding this comment.
Why do we have application specific sagas under the git folder?
Wasn't the intention to create a service layer and later to move it out as a separate package?
There was a problem hiding this comment.
So, /artifact-helpers folder is like an anti corruption layer for the core git package. Artifact specific code goes here. The core git package is artifact agnostic. In future, when we move the core git package to a yarn package, we'll have to decide how to handle them. For now, it is out of scope for this PR
There was a problem hiding this comment.
Why is it out of scope of this PR? I see majority of the files being introduced by this PR?
The issue here is that when we start doing these things we are slowly making it a mandate that it's okay to add artifact specific code to the core git and slowly straying away from modularity principle.
There was a problem hiding this comment.
Done! Thanks for the push. I had a brilliant idea to consume these artifact-helpers as hooks for git sagas. They are now consumed outside the core git package
| @@ -0,0 +1 @@ | |||
| export * from "../ee/packages/packageStatusTransformer"; | |||
There was a problem hiding this comment.
Should this file be under the ce folder?
And the import should be the other way around; the ee file should re-export the ce file
There was a problem hiding this comment.
there is a ce file version. I wanted to keep the imports similar. So, this goes something like this -
ce/file -> ee/file -> file
There was a problem hiding this comment.
Ohh okay but you can directly use the ee path itself wherever you want to use this. Such level of path transformation is not necessary and may cause confusion when someone wants to add a piece of code and ends up adding in this file which they shouldn't. CE EE files should be enough to direct a dev where to add the code.
| import React from "react"; | ||
| import CardBadgeView from "./CardBadgeView"; | ||
|
|
||
| function CardBadge() { |
There was a problem hiding this comment.
Do we need such level of segregation?
| defaultBranch={defaultBranch} | ||
| repoName={repoName} | ||
| /> | ||
| <div className="flex gap-x-4 mb-6"> |
There was a problem hiding this comment.
Let's use Flex from ads and avoid tailwind
There was a problem hiding this comment.
Just moved this part around. Will pick this up separately
| payload: currentApplication, | ||
| }); | ||
| } | ||
| yield applicationConnectToGitSaga(artifactDef, response); |
There was a problem hiding this comment.
It would be great if we can decouple artifact specific logic out of the core git module
There was a problem hiding this comment.
Agreed! But let's address this separately
| import type { ApiResponse } from "api/types"; | ||
| import log from "loglevel"; | ||
|
|
||
| export default function handleApiErrors(error?: Error, response?: ApiResponse) { |
There was a problem hiding this comment.
Why do we need to have separate error handler and not use the existing validateResponse helper function?
There was a problem hiding this comment.
There is an issue with validateResponse. It does not throw when there is a 404 error. Because of this a lot of api request states get stuck in loading. I didn't want to make changes to validateResponse so I ended up making a handleApiErrors function for internal git use
|
|
||
| const urlWithBranch = addBranchParam(branch); | ||
| if (artifactDef.artifactType === GitArtifactType.Application) { | ||
| branchName = (artifact as GitApplicationArtifact)?.gitApplicationMetadata |
There was a problem hiding this comment.
Shouldn't this also be gitArtifactMetadata instead of gitApplicationMetadata ?
There was a problem hiding this comment.
Yes, but the backend doesn't support gitArtifactMetadata for applications yet :(
This comment was marked as duplicate.
This comment was marked as duplicate.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
app/client/src/git/components/OpsModal/TabDeploy/index.tsx (1)
25-27: Consider type guard instead of type assertion.The type assertion
as GitApplicationArtifactcould lead to runtime errors if the artifact is of a different type. Consider using a type guard for safer type narrowing.- // ! git tagging: need to handle last deplyed here when tagging is implemented - const lastDeployedAt = - (artifact as GitApplicationArtifact)?.lastDeployedAt ?? null; + // ! git tagging: need to handle last deployed here when tagging is implemented + const lastDeployedAt = isGitApplicationArtifact(artifact) + ? artifact.lastDeployedAt + : null;Add this type guard function:
function isGitApplicationArtifact( artifact: unknown ): artifact is GitApplicationArtifact { return ( typeof artifact === 'object' && artifact !== null && 'lastDeployedAt' in artifact ); }app/client/src/git/store/selectors/gitArtifactSelectors.ts (1)
137-142: Consider documenting interface compatibility requirement.While the implementation is correct, the eslint-disable comment could be better explained. Consider adding a more detailed comment explaining why the parameter must be preserved for interface compatibility.
export const selectCurrentBranch = ( state: GitRootState, - // need this to preserve interface - // eslint-disable-next-line @typescript-eslint/no-unused-vars + // Parameter required to maintain selector interface compatibility across the codebase, + // though not used in current implementation + // eslint-disable-next-line @typescript-eslint/no-unused-vars artifactDef: GitArtifactDef, ) => selectGitArtifact(state, artifactDef)?.ui.currentBranch ?? null;app/client/src/entities/Engine/index.ts (2)
132-143: Consider adding error handling for branch updates.While the branch handling logic is correct, consider adding error handling for the
updateBranchLocallyaction to align with the PR's objective of improved saga error handling.*setupEngine(payload: AppEnginePayload, rootSpan: Span): any { const setupEngineSpan = startNestedSpan("AppEngine.setupEngine", rootSpan); const { branch } = payload; + try { yield put(updateBranchLocally(branch || "")); yield put(setAppMode(this._mode)); yield put(restoreIDEEditorViewMode()); yield put({ type: ReduxActionTypes.START_EVALUATION }); + } catch (error) { + log.error("Failed to setup engine:", error); + // Consider dispatching an error action here + } endSpan(setupEngineSpan); }
31-54: Consider updating interface documentation.The interface contains multiple TODO comments about fixing eslint issues. Since we're modifying the file, consider addressing these TODOs by properly typing the return values.
export interface IAppEngine { - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - setupEngine(payload: AppEnginePayload, rootSpan: Span): any; + setupEngine(payload: AppEnginePayload, rootSpan: Span): Generator; - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - loadAppData(payload: AppEnginePayload, rootSpan: Span): any; + loadAppData(payload: AppEnginePayload, rootSpan: Span): Generator<unknown, { toLoadPageId: string; toLoadBasePageId: string; applicationId: string }, unknown>; // ... similar changes for other methodsapp/client/src/git/sagas/initGitSaga.ts (1)
30-30: Simplify boolean condition.Replace double negation with a direct boolean check:
- if (!!branchName) { + if (branchName) {🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
app/client/src/git/components/RepoLimitErrorModal/RepoLimitErrorModalView.tsx (2)
88-104: Consider using type guards and optional chaining.The current implementation uses type assertions which could be made safer.
Consider this refactor:
- artifacts?.filter((artifact: GitArtifact) => { - const gitMetadata = - (artifact as GitApplicationArtifact).gitApplicationMetadata || - (artifact as GitPackageArtifact).gitArtifactMetadata; + artifacts?.filter((artifact: GitArtifact) => { + const isApplication = (a: GitArtifact): a is GitApplicationArtifact => + 'gitApplicationMetadata' in a; + const isPackage = (a: GitArtifact): a is GitPackageArtifact => + 'gitArtifactMetadata' in a; + + const gitMetadata = isApplication(artifact) + ? artifact.gitApplicationMetadata + : isPackage(artifact) + ? artifact.gitArtifactMetadata + : null; return ( - gitMetadata && - gitMetadata.remoteUrl && - gitMetadata.branchName && - gitMetadata.repoName && - gitMetadata.isRepoPrivate + gitMetadata?.remoteUrl && + gitMetadata?.branchName && + gitMetadata?.repoName && + gitMetadata?.isRepoPrivate );🧰 Tools
🪛 Biome (1.9.4)
[error] 96-97: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
193-209: Apply consistent type safety improvements in rendering.Similar to the filtering logic, the rendering code could benefit from type guards.
Consider this refactor:
- {gitConnectedArtifacts.map((artifact) => { - const gitMetadata = - (artifact as GitApplicationArtifact).gitApplicationMetadata || - (artifact as GitPackageArtifact).gitArtifactMetadata; + {gitConnectedArtifacts.map((artifact) => { + const isApplication = (a: GitArtifact): a is GitApplicationArtifact => + 'gitApplicationMetadata' in a; + const isPackage = (a: GitArtifact): a is GitPackageArtifact => + 'gitArtifactMetadata' in a; + + const gitMetadata = isApplication(artifact) + ? artifact.gitApplicationMetadata + : isPackage(artifact) + ? artifact.gitArtifactMetadata + : null;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/client/src/entities/Engine/AppEditorEngine.ts(3 hunks)app/client/src/entities/Engine/index.ts(2 hunks)app/client/src/git/artifact-helpers/ee/package/packageStatusTransformer.ts(1 hunks)app/client/src/git/artifact-helpers/package/packageStatusTransformer.ts(1 hunks)app/client/src/git/components/OpsModal/TabDeploy/index.tsx(2 hunks)app/client/src/git/components/RepoLimitErrorModal/RepoLimitErrorModalView.tsx(3 hunks)app/client/src/git/requests/createRefRequest.types.ts(1 hunks)app/client/src/git/sagas/initGitSaga.ts(2 hunks)app/client/src/git/store/actions/currentBranchActions.ts(1 hunks)app/client/src/git/store/gitArtifactSlice.ts(4 hunks)app/client/src/git/store/helpers/initialState.ts(1 hunks)app/client/src/git/store/selectors/gitArtifactSelectors.ts(1 hunks)app/client/src/git/store/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/git/requests/createRefRequest.types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/src/git/artifact-helpers/ee/package/packageStatusTransformer.ts
- app/client/src/git/artifact-helpers/package/packageStatusTransformer.ts
- app/client/src/git/store/gitArtifactSlice.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/git/sagas/initGitSaga.ts
[error] 30-30: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
app/client/src/git/components/RepoLimitErrorModal/RepoLimitErrorModalView.tsx
[error] 96-97: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (9)
app/client/src/git/components/OpsModal/TabDeploy/index.tsx (1)
10-10: LGTM! Type import enhances type safety.The addition of the GitApplicationArtifact type import aligns with the PR's focus on artifact-aware permissions.
app/client/src/git/store/selectors/gitArtifactSelectors.ts (1)
1-256: Well-structured selector implementation!The file demonstrates excellent organization with clear grouping of selectors, consistent patterns, and proper TypeScript typing.
app/client/src/entities/Engine/index.ts (1)
82-128: LGTM! Simplified loadAppData method.The removal of branch-related logic from
loadAppDataimproves the method's focus on core application data loading.app/client/src/git/sagas/initGitSaga.ts (2)
23-24: Standardize Git metadata property naming.For consistency with
gitArtifactMetadataused in packages, consider renaminggitApplicationMetadatato maintain a uniform naming convention across all Git artifacts.
39-53: LGTM! Well-structured feature flag handling.Good use of helper functions to encapsulate feature flag checks and conditional action dispatching.
app/client/src/git/components/RepoLimitErrorModal/RepoLimitErrorModalView.tsx (1)
26-33: LGTM! Type definitions align with artifact-aware permissions.The updated types and interface properly support both application and package artifacts.
Also applies to: 68-78
app/client/src/git/store/actions/currentBranchActions.ts (1)
1-19: Well-structured implementation of branch state management!The actions are properly typed and follow Redux best practices with immutable state updates.
app/client/src/git/store/types.ts (1)
67-67: LGTM: Type definition for currentBranch is appropriate.The
string | nulltype correctly represents both the set and unset states of the current branch.app/client/src/git/store/helpers/initialState.ts (1)
16-16:✅ Verification successful
LGTM! Addition of currentBranch property.
The initialization of
currentBranchasnullis consistent with the codebase patterns and aligns with the Git artifact metadata enhancements.Let's verify that the type definition is updated:
🏁 Script executed:
#!/bin/bash # Search for GitArtifactUIReduxState type definition ast-grep --pattern 'type GitArtifactUIReduxState = { $$$ currentBranch: $_ $$$ }'Length of output: 89
Action: Verify that the GitArtifactUIReduxState type definition includes the new
currentBranchproperty.The addition of
currentBranch: nullin the state initialization looks correct and aligns well with our Git metadata enhancements. However, the initial AST grep did not confirm that theGitArtifactUIReduxStatetype was updated with this property. Please ensure—via manual inspection or by running additional searches—that the type definition now includescurrentBranch.• Verify that the type definition for
GitArtifactUIReduxState(likely in a related TypeScript file) includes an entry likecurrentBranch: <Type>.Below is an additional shell script to further search for references to
currentBranchin our TypeScript files:Once verified, we can confidently approve the state changes.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for any occurrence of currentBranch in TypeScript files with context to verify its type definition update rg -n "currentBranch:" -t ts --context=3Length of output: 13783
LGTM – The
currentBranchproperty is correctly added and the corresponding type definition is updated.
- The initial state in
app/client/src/git/store/helpers/initialState.tscorrectly initializescurrentBranchasnull.- The type definition in
app/client/src/git/store/types.tsnow includescurrentBranch: string | null, confirming consistency.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 30, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
app/client/src/selectors/gitModSelectors.ts (2)
8-12: Document the transition strategy for Git selector imports.The aliasing pattern suggests a transition between old and new implementations. Consider adding a comment explaining the transition strategy and timeline for removing the old selectors.
51-56: Consider refactoring to eliminate circular dependency.While the direct state access works, it suggests an architectural issue. Consider extracting the preview mode logic into a separate module that both components can depend on.
app/client/src/git/types.ts (2)
14-17: Add JSDoc comments to document the interface purpose.Consider adding documentation to explain the purpose and usage of this interface.
+/** + * Defines the base structure for Git artifacts. + * @property artifactType - The type of Git artifact + * @property baseArtifactId - The unique identifier of the base artifact + */ export interface GitArtifactDef { artifactType: GitArtifactType; baseArtifactId: string; }
19-51: Extract common properties to a base interface.Both GitApplicationArtifact and GitPackageArtifact share identical properties. Consider refactoring to reduce duplication.
+interface BaseGitArtifact { + id: string; + baseId: string; + name: string; + lastDeployedAt?: string; +} + +interface BaseGitMetadata { + branchName: string; + defaultBranchName: string; + remoteUrl: string; + repoName: string; + browserSupportedUrl?: string; + isRepoPrivate?: boolean; + browserSupportedRemoteUrl: string; + defaultApplicationId: string; +} + -export interface GitApplicationArtifact { - id: string; - baseId: string; - name: string; - lastDeployedAt?: string; +export interface GitApplicationArtifact extends BaseGitArtifact { gitApplicationMetadata?: { - branchName: string; - defaultBranchName: string; - remoteUrl: string; - repoName: string; - browserSupportedUrl?: string; - isRepoPrivate?: boolean; - browserSupportedRemoteUrl: string; - defaultApplicationId: string; + extends BaseGitMetadata; }; } -export interface GitPackageArtifact { - id: string; - baseId: string; - name: string; - lastDeployedAt?: string; +export interface GitPackageArtifact extends BaseGitArtifact { gitArtifactMetadata?: { - branchName: string; - defaultBranchName: string; - remoteUrl: string; - repoName: string; - browserSupportedUrl?: string; - isRepoPrivate?: boolean; - browserSupportedRemoteUrl: string; - defaultApplicationId: string; + extends BaseGitMetadata; }; }app/client/src/git/components/ConnectModal/ConnectInitialize/ChooseGitProvider.tsx (2)
90-96: Remove commented code.The commented-out code block should be removed if it's no longer needed. If it's for future reference, consider documenting it elsewhere.
177-201: Consider extracting conditional rendering logic.The nested conditional rendering logic could be simplified by extracting it into separate components or helper functions for better maintainability.
Example approach:
const renderEmptyRepoGuidance = (gitProvider?: GitProvider, gitEmptyRepoExists?: string) => { if (gitProvider === "others" && gitEmptyRepoExists === "no") { return <Callout kind="warning">{createMessage(NEED_EMPTY_REPO_MESSAGE)}</Callout>; } if (gitProvider && gitProvider !== "others" && gitEmptyRepoExists === "no") { return ( <Collapsible isOpen> {/* ... existing collapsible content ... */} </Collapsible> ); } return null; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/src/git/components/ConnectModal/ConnectInitialize/ChooseGitProvider.tsx(2 hunks)app/client/src/git/components/ConnectModal/ConnectInitialize/constants.ts(1 hunks)app/client/src/git/components/ConnectModal/ConnectInitialize/types.ts(1 hunks)app/client/src/git/store/index.ts(1 hunks)app/client/src/git/store/selectors/index.ts(1 hunks)app/client/src/git/types.ts(1 hunks)app/client/src/selectors/gitModSelectors.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/git/store/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (7)
app/client/src/selectors/gitModSelectors.ts (2)
16-24: Consider handling potential feature flag conflicts.The spread operator usage could mask conflicts between base and overridden flags. Consider explicit conflict resolution or logging when flags are overridden.
// Example of explicit conflict handling: const combinedFlags = { ...baseFlags, ...Object.entries(overriddenFlags).reduce((acc, [key, value]) => { if (key in baseFlags) { console.debug(`Flag ${key} overridden from ${baseFlags[key]} to ${value}`); } acc[key] = value; return acc; }, {}) };
26-29: Validate baseApplicationId before creating artifact.The empty string fallback could mask invalid states. Consider adding validation or throwing an error for invalid baseApplicationId values.
// Example validation: if (!baseApplicationId) { throw new Error('Invalid baseApplicationId: Application context is required for Git operations'); }app/client/src/git/types.ts (2)
1-1: LGTM! Clean import statement.The import of GitArtifactType is well-defined and necessary for the new type definitions.
53-53: LGTM! Well-defined union type.The union type correctly combines both artifact types, enabling polymorphic handling of Git artifacts.
app/client/src/git/components/ConnectModal/ConnectInitialize/types.ts (1)
1-3: LGTM! Good type definition approach.Using
typeof GIT_PROVIDERS[number]creates a type-safe union from the constant, making it more maintainable.app/client/src/git/components/ConnectModal/ConnectInitialize/constants.ts (1)
28-33: LGTM! Well-defined constant with type assertion.The
as constassertion ensures type safety by creating a readonly tuple type.app/client/src/git/components/ConnectModal/ConnectInitialize/ChooseGitProvider.tsx (1)
65-78: LGTM! Excellent type-safe implementation.The provider validation and type casting implementation ensures type safety while providing proper error logging.
|
Deploy-Preview-URL: https://ce-39255.dp.appsmith.com |
brayn003
added
Git Product
brayn003
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
app/client/src/git/sagas/checkoutBranchSaga.ts (1)
20-48: Consider removing the try-catch block.Based on the retrieved learning from PR #38634, try-catch blocks are unnecessary for Redux
puteffects. Since this saga primarily usesputeffects after the API call, we could simplify the code by:
- Moving the error handling to the API call
- Removing the try-catch wrapper
Here's a suggested refactor:
- try { const params: CheckoutRefRequestParams = { refType: "branch", refName: branchName, }; const isGitApiContractsEnabled: boolean = yield select( selectGitApiContractsEnabled, ); response = yield call( checkoutRefRequest, artifactDef.artifactType, artifactId, params, isGitApiContractsEnabled, ); const isValidResponse: boolean = yield validateResponse(response); if (response && isValidResponse) { yield put( gitArtifactActions.checkoutBranchSuccess({ artifactDef, responseData: response.data, }), ); yield put( gitArtifactActions.toggleBranchPopup({ artifactDef, open: false }), ); } - } catch (e) { - const error = handleApiErrors(e as Error, response); - - if (error) { - yield put(gitArtifactActions.checkoutBranchError({ artifactDef, error })); - } - } + const error = handleApiErrors(response); + if (error) { + yield put(gitArtifactActions.checkoutBranchError({ artifactDef, error })); + return; + }app/client/src/git/sagas/pullSaga.ts (1)
40-46: Consider error handling flow optimization.The current implementation might execute the error popup block even after dispatching an error action. Consider adding a
returnafter the error dispatch to prevent further execution.if (error) { yield put(gitArtifactActions.pullError({ artifactDef, error })); + return; }app/client/src/git/artifact-helpers/application/sagas/applicationRedirectToClosestEntitySaga.ts (2)
18-64: Consider splitting the redirect logic.
This function handles multiple responsibilities (page existence checks, URL construction, mode initialization). Splitting the logic into smaller helpers may enhance readability.
66-94: Review concurrency approach for fetching actions.
Usingyield takecan block if multiple fetch actions run concurrently. Confirm it behaves as expected in edge cases.app/client/src/git/store/actions/checkoutBranchActions.ts (1)
19-29: Remove redundant error state reset.The error state reset in
checkoutBranchSuccessActionis redundant sincecreateArtifactActionalready handles this, as per our established patterns.export const checkoutBranchSuccessAction = createArtifactAction<CheckoutBranchSuccessPayload>((state) => { state.apiResponses.checkoutBranch.loading = false; - state.apiResponses.checkoutBranch.error = null; state.ui.checkoutDestBranch = null; return state; });app/client/src/git/types.ts (1)
31-40: Extract common Git metadata interface.The metadata structure is duplicated between
GitApplicationArtifactandGitPackageArtifact. Consider extracting it into a shared interface.+export interface GitCommonMetadata { + branchName: string; + defaultBranchName: string; + remoteUrl: string; + repoName: string; + browserSupportedUrl?: string; + isRepoPrivate?: boolean; + browserSupportedRemoteUrl: string; + defaultApplicationId: string; +} export interface GitApplicationArtifact { id: string; baseId: string; name: string; pages: GitApplicationArtifactPage[]; lastDeployedAt?: string; - gitApplicationMetadata?: { - branchName: string; - defaultBranchName: string; - remoteUrl: string; - repoName: string; - browserSupportedUrl?: string; - isRepoPrivate?: boolean; - browserSupportedRemoteUrl: string; - defaultApplicationId: string; - }; + gitApplicationMetadata?: GitCommonMetadata; } export interface GitPackageArtifact { id: string; baseId: string; name: string; lastDeployedAt?: string; - gitArtifactMetadata?: { - branchName: string; - defaultBranchName: string; - remoteUrl: string; - repoName: string; - browserSupportedUrl?: string; - isRepoPrivate?: boolean; - browserSupportedRemoteUrl: string; - defaultApplicationId: string; - }; + gitArtifactMetadata?: GitCommonMetadata; }Also applies to: 48-57
app/client/src/git/artifact-helpers/application/sagas/applicationConnectToGitSaga.ts (4)
18-20: Add error handling for page fetch operation.Consider wrapping the page fetch in a try-catch block to handle potential failures gracefully.
const pageId: string = yield select(getCurrentPageId); - yield put(fetchPageAction(pageId)); + try { + yield put(fetchPageAction(pageId)); + } catch (error) { + // Handle page fetch error + console.error('Failed to fetch page:', error); + }
22-28: Add error handling for URL updates.The URL update operation should be wrapped in a try-catch block to handle potential history API errors.
if (branch) { const newUrl = addBranchParam(branch); - history.replace(newUrl); + try { + history.replace(newUrl); + } catch (error) { + console.error('Failed to update URL:', error); + } }
30-40: Consider using a more specific action type.Using
FETCH_APPLICATION_SUCCESSfor updating deployment timestamp seems to repurpose an existing action. Consider creating a dedicated action type for deployment updates.- type: ReduxActionTypes.FETCH_APPLICATION_SUCCESS, + type: ReduxActionTypes.UPDATE_APPLICATION_LAST_DEPLOYED, payload: currentApplication,
42-47: Add error handling for Git editor initialization.Consider wrapping the initialization in a try-catch block to handle potential failures.
- yield put( - gitArtifactActions.initGitForEditor({ - artifactDef, - artifact: destArtifact, - }), - ); + try { + yield put( + gitArtifactActions.initGitForEditor({ + artifactDef, + artifact: destArtifact, + }), + ); + } catch (error) { + console.error('Failed to initialize Git editor:', error); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx(0 hunks)app/client/src/ce/sagas/index.tsx(2 hunks)app/client/src/git/artifact-helpers/application/sagas/applicationConnectToGitSaga.ts(1 hunks)app/client/src/git/artifact-helpers/application/sagas/applicationRedirectToClosestEntitySaga.ts(1 hunks)app/client/src/git/artifact-helpers/application/sagas/index.ts(1 hunks)app/client/src/git/sagas/checkoutBranchSaga.ts(2 hunks)app/client/src/git/sagas/connectSaga.ts(2 hunks)app/client/src/git/sagas/discardSaga.ts(2 hunks)app/client/src/git/sagas/pullSaga.ts(2 hunks)app/client/src/git/store/actions/checkoutBranchActions.ts(2 hunks)app/client/src/git/store/actions/discardActions.ts(2 hunks)app/client/src/git/store/actions/pullActions.ts(2 hunks)app/client/src/git/store/actions/types.ts(1 hunks)app/client/src/git/store/index.ts(1 hunks)app/client/src/git/types.ts(1 hunks)app/client/src/pages/UserProfile/index.tsx(2 hunks)app/client/src/pages/common/ImportModal.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx
✅ Files skipped from review due to trivial changes (1)
- app/client/src/git/store/actions/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/src/pages/UserProfile/index.tsx
- app/client/src/git/store/actions/pullActions.ts
- app/client/src/pages/common/ImportModal.tsx
🧰 Additional context used
🧠 Learnings (4)
app/client/src/git/artifact-helpers/application/sagas/index.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/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/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/types.ts (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#38681
File: app/client/src/git/constants/enums.ts:2-4
Timestamp: 2025-01-16T14:25:46.177Z
Learning: The GitArtifactType enum in TypeScript uses lowercase plural values: "applications", "packages", and "workflows" for Application, Package, and Workflow respectively.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (22)
app/client/src/ce/sagas/index.tsx (2)
54-55: LGTM! Well-organized Git-related imports.The imports are properly structured and align with the artifact-aware permissions objective.
114-115: LGTM! Proper integration of Git application sagas.The Git-related sagas are properly integrated into the saga management structure.
app/client/src/git/sagas/discardSaga.ts (3)
8-11: LGTM! Import changes align with the new error handling strategy.The changes support centralized error handling using
handleApiErrorshelper.
34-39: LGTM! Enhanced response handling with proper UI feedback.The changes improve the data flow by:
- Including response data in the success action
- Adding proper modal management after operation completion
Also applies to: 45-51
54-58: LGTM! Streamlined error handling implementation.The error handling now uses the centralized
handleApiErrorshelper, consistent with the broader saga refactoring effort.app/client/src/git/sagas/checkoutBranchSaga.ts (2)
1-1: LGTM! Import cleanup and error handling centralization.The removal of unnecessary imports and addition of the
handleApiErrorshelper aligns with the PR's objective of improving saga error handling.Also applies to: 12-12
45-47: LGTM! Branch popup toggle after successful checkout.The addition of closing the branch popup after a successful checkout improves the user experience.
app/client/src/git/sagas/pullSaga.ts (2)
9-9: LGTM! Good error handling centralization.The addition of
handleApiErrorshelper aligns with the PR's objective of standardizing error handling across sagas.
33-39: LGTM! Enhanced success payload structure.The success action now includes both the artifact definition and response data, providing better context for state updates.
app/client/src/git/sagas/connectSaga.ts (4)
7-7: Good use of centralizing error codes.
ImportingGitErrorCodesfrom a single source improves maintainability.
14-14: Neat extraction of error handling.
Refactoring error-handling logic intohandleApiErrorsfosters reusability across sagas.
61-65: Validate inclusion of all relevant error details.
Ensure thathandleApiErrorsreturns sufficient info (or logs it) for debugging and user feedback.
66-68: Consistent handling of specific error codes.
HandlingREPO_LIMIT_REACHEDhere is neat and keeps code logic clear.app/client/src/git/artifact-helpers/application/sagas/applicationRedirectToClosestEntitySaga.ts (3)
1-9: Organized imports.
No concerns with these import statements.
10-17: Appropriate usage of selectors and types.
The usage ofgetCurrentBasePageIdandGitApplicationArtifactaligns with established patterns.
96-111: Home page fallback logic looks correct.
Replacing history avoids user confusion with a potential 404.app/client/src/git/artifact-helpers/application/sagas/index.ts (2)
1-9: No issues with imports.
Combining these sagas in one place is clean and consistent.
11-21: Good saga orchestration.
takeLatestusage is aligned with standard patterns, and concurrency is well-managed.app/client/src/git/store/index.ts (1)
4-5: LGTM! Well-structured action exports.The new action exports follow a consistent naming pattern and are properly typed through their source slices.
Also applies to: 13-19
app/client/src/git/store/actions/discardActions.ts (1)
1-1: LGTM! Type-safe implementation of discard actions.The success action is properly typed with
DiscardSuccessPayloadand follows the established pattern for handling loading states.Also applies to: 6-6, 23-31
app/client/src/git/artifact-helpers/application/sagas/applicationConnectToGitSaga.ts (2)
1-12: LGTM! Well-organized imports.The imports are properly structured and include all necessary dependencies for the saga's functionality.
13-17: LGTM! Clear function signature and payload handling.The saga function is well-typed and follows Redux-Saga patterns with proper payload destructuring.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/client/src/git/artifact-helpers/application/sagas/applicationRedirectToClosestEntitySaga.ts (2)
26-26: Add explanatory comment for early exit.A short comment clarifying why non-application artifacts are skipped would help future readers quickly grasp the intent.
if (artifactDef.artifactType !== GitArtifactType.Application) return; + // If the artifact type is not Application, no redirection logic is needed.
53-53: Preserve existing search parameters.Reconstructing the
destinationUrlwith"?" + urlParamsmay discard any existing query params. Consider merging them if needed.- destinationUrl += "?" + urlParams.toString(); + const existingParams = new URLSearchParams(url.search); + urlParams.forEach((value, key) => existingParams.set(key, value)); + destinationUrl = `${url.pathname}?${existingParams.toString()}`;app/client/src/git/artifact-helpers/application/sagas/applicationConnectToGitSaga.ts (2)
27-31: Retain existing URL search parameters.Replacing the URL with
addBranchParam(branch)may discard other query parameters. If that’s intentional, clarify with a comment.
37-43: Avoid direct state mutation.
currentApplicationis mutated (lastDeployedAt). Make sure it won’t cause issues with immutability in Redux. Consider cloning or using an immutable update approach.const updatedApplication = { ...currentApplication, lastDeployedAt: new Date().toISOString(), }; yield put({ type: ReduxActionTypes.FETCH_APPLICATION_SUCCESS, payload: updatedApplication, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/git/artifact-helpers/application/sagas/applicationConnectToGitSaga.ts(1 hunks)app/client/src/git/artifact-helpers/application/sagas/applicationRedirectToClosestEntitySaga.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
🔇 Additional comments (4)
app/client/src/git/artifact-helpers/application/sagas/applicationRedirectToClosestEntitySaga.ts (2)
28-33: Check for default-page absence.If there's no
defaultPageand the existing page is not found, the code doesn't handle the scenario explicitly. Ensure there's a fallback path if no pages match.
100-115: Validate replaced URL.For correctness, verify the final replaced URL is distinct from the current one to avoid redundant calls. This can help prevent unexpected re-render cycles.
app/client/src/git/artifact-helpers/application/sagas/applicationConnectToGitSaga.ts (2)
23-23: Confirm page fetch after connecting.Fetching the page right after establishing a git connection serves the flow, but ensure there aren’t any race conditions with subsequent actions.
45-50: Verify editor initialization.Check if any additional Git config or error handling is needed before calling
initGitForEditor, ensuring consistent editor state in all scenarios.
app/client/src/git/artifact-helpers/application/sagas/applicationRedirectToClosestEntitySaga.ts
Outdated
Show resolved
Hide resolved
github-actions
bot
added
Packages & Git Pod
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/git/ce/components/DefaultBranch/DefaultBranchView.tsx (1)
81-91: Consider adding exhaustive type checking to switch case.While the switch case handles known cases well, consider adding exhaustive type checking to catch future enum additions.
const artifactNoun = useMemo(() => { switch (artifactType) { case GitArtifactType.Application: return "app"; case GitArtifactType.Package: return "package"; default: + const _exhaustiveCheck: never = artifactType; return "artifact"; } }, [artifactType]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/ce/constants/messages.ts(2 hunks)app/client/src/git/ce/components/DefaultBranch/DefaultBranchView.tsx(4 hunks)app/client/src/git/ce/components/DefaultBranch/index.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
🔇 Additional comments (3)
app/client/src/git/ce/components/DefaultBranch/index.tsx (1)
4-14: LGTM! Clean integration of Git artifact context.The changes properly integrate the Git artifact context with safe null handling and good default values.
app/client/src/git/ce/components/DefaultBranch/DefaultBranchView.tsx (1)
48-53: LGTM! Clean prop type definition.The artifactType prop is well-defined with proper typing.
app/client/src/ce/constants/messages.ts (1)
1182-1197: LGTM! Well-structured message constants.The changes properly handle different artifact types in messages with good defaults and consistent formatting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/git/requests/fetchStatusRequest.types.ts (1)
35-37: Consider adding JSDoc comments to document the module arrays.The new module-related arrays follow the established pattern for tracking entity changes. Consider adding JSDoc comments to document what constitutes a "module" in this context.
+ /** Array of module identifiers that were added */ modulesAdded?: string[]; + /** Array of module identifiers that were modified */ modulesModified?: string[]; + /** Array of module identifiers that were removed */ modulesRemoved?: string[];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/git/requests/fetchStatusRequest.types.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
🔇 Additional comments (1)
app/client/src/git/requests/fetchStatusRequest.types.ts (1)
28-28: LGTM! The modifiedModules count property.The optional count property follows the existing pattern for tracking modified entities.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/pages/AppIDE/layout/components/Header/DeployButton.tsx (1)
89-109: Consider extracting Git deployment logic.The handleClickDeploy function handles both Git and regular deployment scenarios. Consider extracting the Git-specific logic into a separate function for better maintainability.
+const handleGitDeploy = useCallback(() => { + if (isGitModEnabled) { + toggleOpsModal(true); + } else { + dispatch(showConnectGitModal()); + } + + AnalyticsUtil.logEvent("GS_DEPLOY_GIT_CLICK", { + source: "Deploy button", + }); +}, [dispatch, isGitModEnabled, toggleOpsModal]); const handleClickDeploy = useCallback(() => { - if (isGitConnected) { - if (isGitModEnabled) { - toggleOpsModal(true); - } else { - dispatch(showConnectGitModal()); - } - - AnalyticsUtil.logEvent("GS_DEPLOY_GIT_CLICK", { - source: "Deploy button", - }); - } else { - handlePublish(); - } + isGitConnected ? handleGitDeploy() : handlePublish(); }, [ dispatch, handlePublish, isGitConnected, isGitModEnabled, toggleOpsModal, ]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/pages/AppIDE/AppIDE.tsx(1 hunks)app/client/src/pages/AppIDE/layout/components/Header/DeployButton.tsx(1 hunks)app/client/src/pages/AppIDE/layout/components/Header/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/pages/AppIDE/layout/components/Header/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (5)
app/client/src/pages/AppIDE/AppIDE.tsx (2)
193-206: LGTM! GitApplicationContextProvider integration looks correct.The provider correctly wraps all Git-related components and modals, maintaining the context hierarchy.
49-49:✅ Verification successful
LGTM! Import path aligns with artifact-aware permissions.
The reorganized import path better reflects the component's role in handling Git artifacts.
Let's verify that the new import path is consistently used across the codebase:
🏁 Script executed:
#!/bin/bash # Search for any remaining old imports of GitApplicationContextProvider rg "from ['\"]components/gitContexts/GitApplicationContextProvider['\"]" # Search for usage of the new import path rg "from ['\"]git/artifact-helpers/application/components['\"]"Length of output: 436
LGTM! Import path update verified.
The new import path (git/artifact-helpers/application/components) is consistently applied in bothapp/client/src/pages/AppIDE/AppIDE.tsxandapp/client/src/pages/AppIDE/layout/components/Header/index.tsx, with no legacy imports remaining. This update clearly aligns with the artifact-aware permissions modifications outlined in the PR.app/client/src/pages/AppIDE/layout/components/Header/DeployButton.tsx (3)
15-20: LGTM! Import paths are well organized.The Git-related hooks are properly organized and imported from their respective modules.
111-129: LGTM! UI implementation follows best practices.The button implementation includes:
- Proper tooltip implementation
- Accessibility attributes
- Correct state management
39-40:✅ Verification successful
Verify Git protected mode implementation.
The deploy button is correctly disabled based on package upgrade status and Git protected mode. However, let's verify the implementation of useGitProtectedMode across the codebase.
🏁 Script executed:
#!/bin/bash # Search for other usages of useGitProtectedMode to ensure consistent implementation ast-grep --pattern 'const $_ = useGitProtectedMode()'Length of output: 1099
Git Protected Mode Implementation Verified
The usage of the
useGitProtectedModehook is consistent across the codebase—including in the deploy button component—and correctly determines the disabled state when either a package upgrade is in progress or Git protected mode is active. No changes are required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/client/src/git/components/RepoLimitErrorModal/RepoLimitErrorModalView.tsx (2)
88-104: Improve type safety and null checks in the filtering logic.The current implementation uses type casting which could be unsafe. Consider using type guards and optional chaining for better type safety and null handling.
Here's a suggested improvement:
-artifacts?.filter((artifact: GitArtifact) => { - const gitMetadata = - (artifact as GitApplicationArtifact).gitApplicationMetadata || - (artifact as GitPackageArtifact).gitArtifactMetadata; - - return ( - gitMetadata && - gitMetadata.remoteUrl && - gitMetadata.branchName && - gitMetadata.repoName && - gitMetadata.isRepoPrivate - ); +artifacts?.filter((artifact: GitArtifact) => { + const isApplicationArtifact = (a: GitArtifact): a is GitApplicationArtifact => + 'gitApplicationMetadata' in a; + const isPackageArtifact = (a: GitArtifact): a is GitPackageArtifact => + 'gitArtifactMetadata' in a; + + const gitMetadata = isApplicationArtifact(artifact) + ? artifact.gitApplicationMetadata + : isPackageArtifact(artifact) + ? artifact.gitArtifactMetadata + : null; + + return gitMetadata?.remoteUrl && + gitMetadata?.branchName && + gitMetadata?.repoName && + gitMetadata?.isRepoPrivate;🧰 Tools
🪛 Biome (1.9.4)
[error] 96-97: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
193-209: Apply consistent type handling in the render logic.The rendering logic uses the same type casting pattern. Let's make it consistent with the filtering improvements.
Here's a suggested improvement:
-{gitConnectedArtifacts.map((artifact) => { - const gitMetadata = - (artifact as GitApplicationArtifact).gitApplicationMetadata || - (artifact as GitPackageArtifact).gitArtifactMetadata; +{gitConnectedArtifacts.map((artifact) => { + const isApplicationArtifact = (a: GitArtifact): a is GitApplicationArtifact => + 'gitApplicationMetadata' in a; + const isPackageArtifact = (a: GitArtifact): a is GitPackageArtifact => + 'gitArtifactMetadata' in a; + + const gitMetadata = isApplicationArtifact(artifact) + ? artifact.gitApplicationMetadata + : isPackageArtifact(artifact) + ? artifact.gitArtifactMetadata + : null;app/client/src/git-artifact-helpers/application/sagas/applicationRedirectToClosestEntitySaga.ts (1)
100-116: Avoid repeating URL construction logic
Lines 100–107 resemble earlier URL manipulations. Consider extracting a helper function to ensure DRY principles and simplify maintenance.app/client/src/git-artifact-helpers/application/components/GitApplicationContextProvider.tsx (1)
43-57: Consider consolidating permission checks for better performance.The multiple
useMemohooks for permissions could be consolidated into a single hook returning an object with all permissions. This would reduce the number of hooks and potentially improve performance.- const isManageAutocommitPermitted = useMemo(() => { - return hasGitAppManageAutoCommitPermission(artifact?.userPermissions ?? []); - }, [artifact]); - - const isManageDefaultBranchPermitted = useMemo(() => { - return hasGitAppManageDefaultBranchPermission( - artifact?.userPermissions ?? [], - ); - }, [artifact]); - - const isManageProtectedBranchesPermitted = useMemo(() => { - return hasGitAppManageProtectedBranchesPermission( - artifact?.userPermissions ?? [], - ); - }, [artifact]); + const permissions = useMemo(() => ({ + isManageAutocommitPermitted: hasGitAppManageAutoCommitPermission( + artifact?.userPermissions ?? [] + ), + isManageDefaultBranchPermitted: hasGitAppManageDefaultBranchPermission( + artifact?.userPermissions ?? [] + ), + isManageProtectedBranchesPermitted: hasGitAppManageProtectedBranchesPermission( + artifact?.userPermissions ?? [] + ), + }), [artifact]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/client/src/ce/sagas/index.tsx(2 hunks)app/client/src/entities/Engine/AppEditorEngine.ts(4 hunks)app/client/src/git-artifact-helpers/application/applicationArtifact.ts(1 hunks)app/client/src/git-artifact-helpers/application/components/GitApplicationContextProvider.tsx(4 hunks)app/client/src/git-artifact-helpers/application/components/index.tsx(1 hunks)app/client/src/git-artifact-helpers/application/sagas/applicationConnectToGitSaga.ts(1 hunks)app/client/src/git-artifact-helpers/application/sagas/applicationRedirectToClosestEntitySaga.ts(1 hunks)app/client/src/git-artifact-helpers/application/sagas/index.ts(1 hunks)app/client/src/git/components/RepoLimitErrorModal/RepoLimitErrorModalView.tsx(3 hunks)app/client/src/pages/AppIDE/AppIDE.tsx(1 hunks)app/client/src/pages/AppIDE/layout/components/Header/index.tsx(1 hunks)app/client/src/selectors/gitModSelectors.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/src/git-artifact-helpers/application/applicationArtifact.ts
- app/client/src/git-artifact-helpers/application/components/index.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- app/client/src/pages/AppIDE/layout/components/Header/index.tsx
- app/client/src/pages/AppIDE/AppIDE.tsx
- app/client/src/ce/sagas/index.tsx
- app/client/src/entities/Engine/AppEditorEngine.ts
- app/client/src/selectors/gitModSelectors.ts
🧰 Additional context used
🧠 Learnings (1)
app/client/src/git-artifact-helpers/application/sagas/index.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.
🪛 Biome (1.9.4)
app/client/src/git/components/RepoLimitErrorModal/RepoLimitErrorModalView.tsx
[error] 96-97: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
🔇 Additional comments (10)
app/client/src/git/components/RepoLimitErrorModal/RepoLimitErrorModalView.tsx (1)
27-34: LGTM! Type definitions are well-structured.The updated type imports and interface changes properly support the new artifact-aware permissions system.
Also applies to: 68-78
app/client/src/git-artifact-helpers/application/sagas/applicationRedirectToClosestEntitySaga.ts (3)
19-26: Good use of early return for artifact type check
This pattern prevents unnecessary processing if the artifact type is not relevant.
28-33: Guard against missing or empty pages array
IfdestArtifact.pagesis unexpectedly undefined or empty, the subsequent.find()calls could cause runtime errors. Consider verifying its existence to avoid potential crashes.if (artifactDef.artifactType !== GitArtifactType.Application) return; + if (!destArtifact.pages?.length) { + return; + } const currentBasePageId: string = yield select(getCurrentBasePageId);
70-98: Double-check fetch failures
You're waiting forFETCH_ACTIONS_SUCCESSbut not handling the scenario in which the fetch might fail. Confirm whether the broader app logic ensures this action always succeeds to avoid potential blocking.app/client/src/git-artifact-helpers/application/sagas/index.ts (1)
11-21: Sagas are well-organized
UsingallandtakeLatestfor these related Git actions provides clear concurrency handling. No additional concerns here.app/client/src/git-artifact-helpers/application/sagas/applicationConnectToGitSaga.ts (3)
19-20: Early return for non-Application artifact
Cleanly avoids performing unnecessary logic. Good approach.
21-24: Verifying the page before fetching
Fetching the current page immediately is fine, but consider how you’d handle the case if the page is invalid or missing. This might cause an unintended error flow.
37-43: Handle potential missing application
You're updatinglastDeployedAtonly ifcurrentApplicationexists. Confirm there's no edge case wherecurrentApplicationmight benullunexpectedly, leaving the store in an incomplete state.app/client/src/git-artifact-helpers/application/components/GitApplicationContextProvider.tsx (2)
1-1: LGTM! Well-organized permission helper imports.The new permission helper imports align well with the artifact-aware permissions objective.
Also applies to: 8-13, 22-22
39-41: LGTM! Well-implemented permission checks with proper memoization.The permission checks are correctly memoized based on artifact changes, preventing unnecessary recalculations.
Also applies to: 43-57
…39375) ## Description Context - https://theappsmith.slack.com/archives/C0134BAVDB4/p1740027639910039?thread_ts=1739904050.960349&cid=C0134BAVDB4 The app store wasn't getting hydrated from local storage in view mode because of the changes in the PR #39255 Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13429852595> > Commit: d9c5c62 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13429852595&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Thu, 20 Feb 2025 08:36:38 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No
Description
Fixes #38505
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/13375089313
Commit: 13aa020
Cypress dashboard.
Tags:
@tag.GitSpec:
Mon, 17 Feb 2025 17:44:17 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor
Chores